Re: [PATCH v2 9/9] builtin/history: implement "drop" subcommand

[Patrick Steinhardt <ps@pks.im>]

On Thu, Jun 04, 2026 at 08:58:07AM +0900, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > +static int update_worktree(struct repository *repo,
> > +			   const struct commit *old_head,
> > +			   const struct commit *new_head,
> > +			   bool dry_run)
> > +{
> > +	struct reset_head_opts opts = {
> > +		.oid_from = &old_head->object.oid,
> > +		.oid = &new_head->object.oid,
> > +		.flags = RESET_HEAD_SKIP_REF_UPDATES,
> > +	};
> > +	if (dry_run)
> > +		opts.flags |= RESET_HEAD_DRY_RUN;
> > +	return reset_head(repo, &opts);
> > +}
> > + ...
> > +	/*
> > +	 * If HEAD will move as a result of the rewrite then we'll have to
> > +	 * merge in the changes into the worktree and index. This merge can of
> > +	 * course conflict, which will cause the whole operation to abort.
> > +	 *
> > +	 * If we had already updated the refs at that point then we'd have an
> > +	 * inconsistent repository state. So we first perform a dry-run merge
> > +	 * here before updating refs.
> > +	 */
> > +	if (!dry_run && !is_bare_repository()) {
> > +		ret = find_head_tree_change(repo, &result, &old_head,
> > +					    &new_head, &head_moves);
> > +		if (ret < 0)
> > +			goto out;
> > +
> > +		if (head_moves && update_worktree(repo, old_head, new_head, true) < 0) {
> > +			ret = error(_("dropping this commit would "
> > +				      "overwrite local changes; aborting"));
> > +			goto out;
> > +		}
> > +	}
> 
> This block is skipped under --dry-run, but update_worktree is
> equipped to (and indeed run unconditionally here) run in the dry-run
> mode.  Does it mean that "git history drop --dry-run" that user runs
> to see which refs may be updated will not get warned about possible
> worktree conflicts that would prevent the real run from happening?
> Unless there is a compelling reason not to, I think --dry-run should
> be a close simulation of what would happen without it.

That's an oversight on my part indeed. We _should_ run this block with
dry-run, and it shows that we're definitely lacking test coverage here.
Will fix, thanks!

Patrick