Re: [PATCH 2/2] doc/git-branch: Document the --current option

Daniels Umanovskis <daniels@umanovskis.se> writes:

Where does "if otherwise impossible to resolve" come from? In the code in [PATCH 1/2], we see this bit

+ const char *refname = resolve_ref_unsafe("HEAD", 0, NULL, NULL); + char *shortname = shorten_unambiguous_ref(refname, 0);

and the output phase would become puts(shortname).

 * Under what condition resolve_ref_unsafe(HEAD) fail to resolve,
   and when that happens what does it return?  "HEAD"?  Can the
   caller tell the case in which .git/HEAD is a symref that points
   at refs/heads/HEAD (i.e. we are on a branch whose name is "HEAD")
   and the case in which .git/HEAD fails to resolve and you get
   "HEAD" back?

 * Or does the function return NULL in "otherwise impossible" case?
   Does shorten_unambiguous_ref() deal with refname==NULL
   gracefully?

 * Under what condition shorten_unambiguous_ref() fail to compute
   the branch name discovered by resolve_ref_unsafe()?

Also, I do not think the implementation is correct. When you are on the 'frotz' branch, and if you happen to have a tag whose name also is 'frotz', then

 - Your .git/HEAD points at refs/heads/frotz, and refs/heads/frotz
   is what resolve_ref_unsafe() gives you.

 - You have refs/heads/frotz and refs/tags/frotz in the repository.
   Asking to shorten the ref refs/heads/frotz unambiguously will
   *not* yield 'frotz'.  It will give you something like 'heads/frotz'
   to avoid getting it confused with tags/frotz

 - Still "git branch --list" would show 'frotz' in such a case, and
   your "--current" would definitely want to match the behaviour.

I think the correct implementation should be more like:

 - Ask resolve-ref-unsafe about HEAD; if it is not a symbolic ref,
   then we are on a detached HEAD.  Silently exit with status 0.

 - If it is a symbolic ref, see if the target of the symblic ref
   (i.e. returned refname) begins with "refs/heads/".  Otherwise, we
   have a repository corruption.  Diagnose it as an error and die().

 - Otherwise, strip that leading "refs/heads/"; the remainder is the
   name of the "current branch".

I already said "current" by itself is an unacceptable name for this option, so I won't be repeating myself.