Re: [PATCH] branch: make --show-current use already resolved HEAD

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

Rafael Ascensão <rafa.almas@gmail.com> writes:

> print_current_branch_name() tries to resolve HEAD and die() when it
> doesn't resolve it successfully. But the conditions being tested are
> always unreachable because early in branch:cmd_branch() the same logic
> is performed.
>
> Eliminate the duplicate and unreachable code, and update the current
> logic to the more reliable check for the detached head.

Nice.

> I still think the mention about scripting should be removed from the
> original commit message, leaving it open to being taught other tricks
> like --verbose that aren't necessarily script-friendly.

I'd prefer to see scriptors avoid using "git branch", too.

Unlike end-user facing documentation where we promise "we do X and
will continue to do so because of Y" to the readers, the log message
is primarily for recording the original motivation of the change, so
that we can later learn "we did X back then because we thought Y".
When we want to revise X, we revisit if the reason Y is still valid.

So in that sense, the door to "break" the scriptability is still
open.

> But the main goal of this patch is to just bring some attention to this,
> as I mentioned it in a previous thread but it got lost.

This idea of yours seems to lead to a better implementation, and
indeed "got lost" is a good way to describe what happened---I do not
recall seeing it, for example.  Thanks for bringing it back.

> diff --git a/builtin/branch.c b/builtin/branch.c
> index 46f91dc06d..1c51d0a8ca 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -38,6 +38,7 @@ static const char * const builtin_branch_usage[] = {
>  
>  static const char *head;
>  static struct object_id head_oid;
> +static int head_flags = 0;

You've eliminated the "now unnecessary" helper and do everything
inside cmd_branch(), so perhaps this can be made function local, no?

> @@ -668,10 +654,10 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
>  
>  	track = git_branch_track;
>  
> -	head = resolve_refdup("HEAD", 0, &head_oid, NULL);
> +	head = resolve_refdup("HEAD", 0, &head_oid, &head_flags);
>  	if (!head)
>  		die(_("Failed to resolve HEAD as a valid ref."));
> -	if (!strcmp(head, "HEAD"))
> +	if (!(head_flags & REF_ISSYMREF))
>  		filter.detached = 1;

Nice to see we can reuse the resolve_refdup() we already have.

>  	else if (!skip_prefix(head, "refs/heads/", &head))
>  		die(_("HEAD not found below refs/heads!"));
> @@ -716,7 +702,8 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
>  			die(_("branch name required"));
>  		return delete_branches(argc, argv, delete > 1, filter.kind, quiet);
>  	} else if (show_current) {
> -		print_current_branch_name();
> +		if (!filter.detached)
> +			puts(head);

Ah, I wondered why we do not have to skip-prefix, but it is already
done for us when we validated that an attached HEAD points at a
local branch.  Good.

>  		return 0;
>  	} else if (list) {
>  		/*  git branch --local also shows HEAD when it is detached */