Subject: Re: [PATCH 2/2] builtin/history: implement "drop" subcommand

From: Patrick Steinhardt <[email-masked]>
Date: Wed, 3 Jun 2026 12:06:43 +0200

On Tue, Jun 02, 2026 at 08:43:48AM +0900, Junio C Hamano wrote:
> Patrick Steinhardt <[email-masked]> writes:
[snip]
> > +static int update_worktree(struct repository *repo,
> > +			   const struct commit *old_head,
> > +			   const struct commit *new_head,
> > +			   bool dry_run)
> > +{
> > +...
> > +
> > +out:
> > +	clear_unpack_trees_porcelain(&opts);
> > +	rollback_lock_file(&lock);
> > +	release_index(&index);
> > +	free(desc_buf[0]);
> > +	free(desc_buf[1]);
> > +	return ret;
> > +}
> 
> The function looks very familiar---anybody who wants to perform
> "checkout <other-commit>" needs to do exactly the above.  It is a
> bit surprising and disappointing that this topic needs to *invent*
> its own helper function and carry it as a file-scope static.

It certainly is. We basically have this whole dance in ~8 different
locations by now, and given the verbosity that is required for the whole
setup it's a good hint that the interface is not exactly great.

One of the functions that we might be able to reuse is `reset_head()`...
goes down the rabbit hole... ugh, this is turning out to be somewhat
painful. I'll send a v2 that does the whole exercise, but I'm not a 100%
convinced it's the right thing to do. There's various assumptions that
we have to break:

  - It assumes that the index is always clean.

  - We don't have a dry-run mode.

  - We need to stop it from updating any refs.

  - We need to introduce another field to let the caller decide which
    commit we're moving from.

So I'm 7 commits deep now adapting the function to our needs. But maybe
the end result is ultimately worth it...? We'll see.

Patrick