[PATCH 1/2] vcs_info git: Flatten a nested if.

[PATCH 1/2] vcs_info git: Flatten a nested if.

[Daniel Shahaf]

No functional change.
---
 Functions/VCS_Info/Backends/VCS_INFO_get_data_git | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/Functions/VCS_Info/Backends/VCS_INFO_get_data_git b/Functions/VCS_Info/Backends/VCS_INFO_get_data_git
index f3dd95dcb..a44a62c79 100644
--- a/Functions/VCS_Info/Backends/VCS_INFO_get_data_git
+++ b/Functions/VCS_Info/Backends/VCS_INFO_get_data_git
@@ -104,16 +104,11 @@ VCS_INFO_git_getbranch () {
     elif [[ -d "${gitdir}/.dotest-merge" ]] ; then
         gitbranch="$(< ${gitdir}/.dotest-merge/head-name)"
 
+    elif gitbranch="$(${(z)gitsymref} 2> /dev/null)" ; then
+    elif gitbranch="refs/tags/$(${vcs_comm[cmd]} describe --all --exact-match HEAD 2>/dev/null)" ; then
+    elif gitbranch="${${"$(< $gitdir/HEAD)"}[1,7]}..." ; then
     else
-        gitbranch="$(${(z)gitsymref} 2> /dev/null)"
-
-        if [[ $? -ne 0 ]] ; then
-            gitbranch="refs/tags/$(${vcs_comm[cmd]} describe --all --exact-match HEAD 2>/dev/null)"
-
-            if [[ $? -ne 0 ]] ; then
-                gitbranch="${${"$(< $gitdir/HEAD)"}[1,7]}..."
-            fi
-        fi
+        # Can't happen
     fi
 
     return 0

[PATCH 2/2] vcs_info git: For the branch name, try to find a symbolic name before falling back to a raw commit hex.

[Daniel Shahaf]

Ask git-describe(1) and git-name-rev(1) to compute a gitrevisions(1)
name in terms of a branch or tag that contains (= is a descendant of,
is younger than) HEAD.

In this repository, the output changes from "9567bfe..." to
"master~1" or "remotes/origin/HEAD~1".
---
WDYT?

Cheers,

Daniel
(In HEAD, the branch name is already set to "heads/master" after `git checkout --detach master`.)

 Functions/VCS_Info/Backends/VCS_INFO_get_data_git | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Functions/VCS_Info/Backends/VCS_INFO_get_data_git b/Functions/VCS_Info/Backends/VCS_INFO_get_data_git
index a44a62c79..c3e62db3a 100644
--- a/Functions/VCS_Info/Backends/VCS_INFO_get_data_git
+++ b/Functions/VCS_Info/Backends/VCS_INFO_get_data_git
@@ -106,6 +106,10 @@ VCS_INFO_git_getbranch () {
 
     elif gitbranch="$(${(z)gitsymref} 2> /dev/null)" ; then
     elif gitbranch="refs/tags/$(${vcs_comm[cmd]} describe --all --exact-match HEAD 2>/dev/null)" ; then
+    elif gitbranch="$(${vcs_comm[cmd]} describe --contains HEAD 2>/dev/null)" ; then
+    ## Commented out because we don't know of a case in which 'describe --contains' fails and 'name-rev --tags' succeeds.
+    #elif gitbranch="$(${vcs_comm[cmd]} name-rev --name-only --no-undefined --tags HEAD 2>/dev/null)" ; then
+    elif gitbranch="$(${vcs_comm[cmd]} name-rev --name-only --no-undefined --always HEAD 2>/dev/null)" ; then
     elif gitbranch="${${"$(< $gitdir/HEAD)"}[1,7]}..." ; then
     else
         # Can't happen

Re: [PATCH 2/2] vcs_info git: For the branch name, try to find a symbolic name before falling back to a raw commit hex.

[Frank Terbeck]

Daniel Shahaf wrote:
> Ask git-describe(1) and git-name-rev(1) to compute a gitrevisions(1)
> name in terms of a branch or tag that contains (= is a descendant of,
> is younger than) HEAD.
>
> In this repository, the output changes from "9567bfe..." to
> "master~1" or "remotes/origin/HEAD~1".
> ---
> WDYT?

I like it. Maybe use a style to make it possible to get the old
behavior back, if someone prefers it?

Regards, Frank

Re: [PATCH 2/2] vcs_info git: For the branch name, try to find a symbolic name before falling back to a raw commit hex.

[Daniel Shahaf]

Frank Terbeck wrote on Sat, 18 Aug 2018 22:41 +0200:
> Daniel Shahaf wrote:
> > Ask git-describe(1) and git-name-rev(1) to compute a gitrevisions(1)
> > name in terms of a branch or tag that contains (= is a descendant of,
> > is younger than) HEAD.
> >
> > In this repository, the output changes from "9567bfe..." to
> > "master~1" or "remotes/origin/HEAD~1".
> > ---
> > WDYT?
> 
> I like it. Maybe use a style to make it possible to get the old
> behavior back, if someone prefers it?

Thanks for the review.  Two thoughts about this:

1. Adding knobs induces a maintenance cost going forward.  Is that cost
   justified in this case?  We can always add the knob later if there's
   demand for it, but it's harder to take away behaviour once it has
   been released.

2. At what point in the if-chain do we raise the white flag^W^W^W^W
   check the "early exit?" style?

I suppose we could overload the `use-simple` style for this purpose (the
git backend doesn't currently use that style).  The `get-revision` style
is also relevant.

Cheers,

Daniel

P.S. This also kicks in during bisects, resulting in values such as "bisect/bad~20".

Re: [PATCH 2/2] vcs_info git: For the branch name, try to find a symbolic name before falling back to a raw commit hex.

[Frank Terbeck]

Daniel Shahaf wrote:
> 1. Adding knobs induces a maintenance cost going forward.  Is that cost
>    justified in this case?  We can always add the knob later if there's
>    demand for it, but it's harder to take away behaviour once it has
>    been released.
>
> 2. At what point in the if-chain do we raise the white flag^W^W^W^W
>    check the "early exit?" style?
>
> I suppose we could overload the `use-simple` style for this purpose (the
> git backend doesn't currently use that style).  The `get-revision` style
> is also relevant.

Yeah. The git backend is pretty complex as it is. Maybe it's not worth
it, to make this configurable. I don't know.

Re: [PATCH 2/2] vcs_info git: For the branch name, try to find a symbolic name before falling back to a raw commit hex.

[Daniel Shahaf]

Frank Terbeck wrote on Mon, 20 Aug 2018 13:45 +0200:
> Daniel Shahaf wrote:
> > 1. Adding knobs induces a maintenance cost going forward.  Is that cost
> >    justified in this case?  We can always add the knob later if there's
> >    demand for it, but it's harder to take away behaviour once it has
> >    been released.
> >
> > 2. At what point in the if-chain do we raise the white flag^W^W^W^W
> >    check the "early exit?" style?
> >
> > I suppose we could overload the `use-simple` style for this purpose (the
> > git backend doesn't currently use that style).  The `get-revision` style
> > is also relevant.
> 
> Yeah. The git backend is pretty complex as it is. Maybe it's not worth
> it, to make this configurable. I don't know.

Let's leave the style out until/unless someone asks for it.  It'd be trivial to add
later if we want it.