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

[Junio C Hamano <gitster@pobox.com>]

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?

> +	for (size_t i = 0; i < result->updates_nr; i++) {
> +		if (!strcmp(result->updates[i].refname, head_target)) {
> +			head_update = &result->updates[i];
> +			break;
> +		}
> +	}

We see if we are going to update the currently checked out branch.

The .updates[] array was earlier populated by the caller that called
compute_pending_ref_updates() before it called us.

> +	if (!head_update)
> +		return 0;

And we return if not.  That is a simple and happy case.  Otherwise
we'd tell the caller about the updated commit and tree (strictly
speaking that would be redundant, but since we have it already, it
is better to give them to the caller instead of forcing the caller
to unwrap the commit).

> +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?