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

[Rafael Ascensão]

I did something that resulted in the mailing list not being cc'd.
Apologies to Junio and Daniels for the double send. :(

On Thu, Nov 08, 2018 at 10:11:02AM +0900, Junio C Hamano wrote:
> 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.
> 

Over at #git, commit messages are sometimes consulted to disambiguate or
clarify certain details. Often the documentation is correct but people
dispute over interpretations.

If someone came asking if `git branch` is parsable, I would advise
against and direct them to the plumbing or format alternative. But if
someone came over with a link to this commit asking the same question,
I suspect the answer would be: it's probably safe to parse the output of
this specific option because the commit says so. Thanks for clarifying
this is wrong.

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

I was not sure if these 3 lines were global intentionally or if it was
just an artifact from the past. Since it looks like the latter, I'll
make them local.

--
Rafael Ascensão

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

[Junio C Hamano]

Rafael Ascensão  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, _oid, NULL);
> + head = resolve_refdup("HEAD", 0, _oid, _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/", ))
>   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 */