Re: [PATCH v7 11/11] builtin/history: implement "drop" subcommand

[Patrick Steinhardt <ps@pks.im>]

On Mon, Jun 29, 2026 at 12:49:42PM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > +static int find_head_tree_change(struct repository *repo,
> > +				 const struct replay_result *result,
> > +				 struct commit **old_head,
> > +				 struct commit **new_head,
> > +				 bool *changed)
> > +{
> > +	const struct replay_ref_update *head_update = NULL;
> > +	struct commit *old_head_commit, *new_head_commit;
> > +	struct tree *old_head_tree, *new_head_tree;
> > +	const char *head_target;
> > +	int head_flags;
> > +
> > +	*changed = false;
> > +
> > +	head_target = refs_resolve_ref_unsafe(get_main_ref_store(repo),
> > +					      "HEAD", RESOLVE_REF_NO_RECURSE,
> > +					      NULL, &head_flags);
> > +	if (!head_target)
> > +		return error(_("cannot look up HEAD"));
> 
> Here head_target would be something like "refs/heads/master", or
> whatever the "HEAD" happens to point at.
> 
> > +	if (!(head_flags & REF_ISSYMREF))
> > +		head_target = "HEAD";
> 
> But if it is not a symref, then it is a detached HEAD.  We manually
> set it to "HEAD" again.  We know head_target was not NULL, so what
> did we receive in it from refs_resolve_ref_unsafe() call, before we
> overwrite it here?

True, this is redundant indeed, as the returned name would already be
"HEAD" in that case.

[snip]
> > +static int cmd_history_drop(int argc,
> > +			    const char **argv,
> > +			    const char *prefix,
> > +			    struct repository *repo)
> > +{
> > +...
> > +	struct option options[] = {
> > +		OPT_CALLBACK_F(0, "update-refs", &action, "(branches|head)",
> > +			       N_("control which refs should be updated"),
> > +			       PARSE_OPT_NONEG, parse_ref_action),
> > ...
> > +		OPT_END(),
> > +	};
> > ...
> > +	ret = compute_pending_ref_updates(&revs, action, original, rewritten,
> > +					  empty, &result);
> 
> Here we call the function.  When action is "--update-refs=head",
> doesn't this code in compute_pending_ref_updates() ... 
> 
> 		if (action == REF_ACTION_HEAD &&
> 		    decoration->type != DECORATION_REF_HEAD)
> 			continue;
> 
> ... skip any reference name (loaded by load_ref_decorations() lazily
> by calling get_name_decoration()) that is not "HEAD", which would
> mean we end up not finding any hits in the find_head_tree_change()
> function call we make later ...
> 
> > +	if (ret) {
> > +		ret = error(_("failed replaying descendants"));
> > +		goto out;
> > +	}
> > + ...
> > +	if (!is_bare_repository()) {
> > +		ret = find_head_tree_change(repo, &result, &old_head,
> > +					    &new_head, &head_moves);
> 
> ... here?

No, because `replay_revisions()` populates the result with the resolved
branch name, and we're also searching for the same branch name inside
`find_head_tree_change()`.

Patrick