This patch improves the example hook that shows +N/-N when the local git branch is ahead-of or behind the remote HEAD. Improvements: * Use git-rev-list's `--count` option instead of piping to `wc -l`. `--count` has been available since git 1.7.2 (https://github.com/git/git/commit/f69c501832ecd6880602c55565508e70c3a013d5) * Remove unnecessary use of `${hook_com[branch]}` because `@{upstream}` defaults to the current branch when no branch name is provided. `@{upstream}` was introduced in git 1.7.0 (https://github.com/git/git/commit/28fb84382b0eb728534dbe2972bbfec3f3d83dd9) diff --git a/Misc/vcs_info-examples b/Misc/vcs_info-examples index 94b8a7b5e..36d4d3bf8 100644 --- a/Misc/vcs_info-examples +++ b/Misc/vcs_info-examples @@ -179,14 +179,18 @@ function +vi-git-st() { local ahead behind local -a gitstatus - # for git prior to 1.7 + # for git prior to 1.7.0 # ahead=$(git rev-list origin/${hook_com[branch]}..HEAD | wc -l) - ahead=$(git rev-list ${hook_com[branch]}@{upstream}..HEAD 2>/dev/null | wc -l) + # for git 1.7.0 and 1.7.1 + # ahead=$(git rev-list @{upstream}..HEAD 2>/dev/null | wc -l) + ahead=$(git rev-list --count @{upstream}..HEAD 2>/dev/null) (( $ahead )) && gitstatus+=( "+${ahead}" ) - # for git prior to 1.7 + # for git prior to 1.7.0 # behind=$(git rev-list HEAD..origin/${hook_com[branch]} | wc -l) - behind=$(git rev-list HEAD..${hook_com[branch]}@{upstream} 2>/dev/null | wc -l) + # for git 1.7.0 and 1.7.1 + # behind=$(git rev-list HEAD..@{upstream} 2>/dev/null | wc -l) + behind=$(git rev-list --count HEAD..@{upstream} 2>/dev/null) (( $behind )) && gitstatus+=( "-${behind}" ) hook_com[misc]+=${(j:/:)gitstatus}
Tim Lee wrote on Mon, Mar 29, 2021 at 05:36:28 +0800: > This patch improves the example hook that shows +N/-N when the > local git branch is ahead-of or behind the remote HEAD. Thanks. Review: > Improvements: > > * Use git-rev-list's `--count` option instead of piping to `wc -l`. > `--count` has been available since git 1.7.2 > (https://github.com/git/git/commit/f69c501832ecd6880602c55565508e70c3a013d5) Sure. > * Remove unnecessary use of `${hook_com[branch]}` because `@{upstream}` > defaults to the current branch when no branch name is provided. > `@{upstream}` was introduced in git 1.7.0 > (https://github.com/git/git/commit/28fb84382b0eb728534dbe2972bbfec3f3d83dd9) I'm not sure that's unnecessary, for two reasons: 1. hook_com[branch], as opposed to hook_com[branch_orig], may have been changed by a previous hook. 2. There may be educational value to demonstrating a use of hook_com[branch], even if it's implied. WDYT? > > diff --git a/Misc/vcs_info-examples b/Misc/vcs_info-examples > index 94b8a7b5e..36d4d3bf8 100644 > --- a/Misc/vcs_info-examples > +++ b/Misc/vcs_info-examples > @@ -179,14 +179,18 @@ function +vi-git-st() { > local ahead behind > local -a gitstatus > > - # for git prior to 1.7 > + # for git prior to 1.7.0 > # ahead=$(git rev-list origin/${hook_com[branch]}..HEAD | wc -l) > - ahead=$(git rev-list ${hook_com[branch]}@{upstream}..HEAD 2>/dev/null | wc -l) > + # for git 1.7.0 and 1.7.1 > + # ahead=$(git rev-list @{upstream}..HEAD 2>/dev/null | wc -l) > + ahead=$(git rev-list --count @{upstream}..HEAD 2>/dev/null) > (( $ahead )) && gitstatus+=( "+${ahead}" ) The version of this function in my zshrc starts like this: . git rev-parse @{upstream} >/dev/null 2>&1 || return 0 local -a x=( $(git rev-list --left-right --count HEAD...@{upstream} ) ) The first line is to handle a detached HEAD, I think, and should presumably be added? The second line saves a fork(), which is valuable on some platforms. (I actually posted my function in users/21810, but didn't notice these two issues back then. Daniel Hahler posted his version too in users/21813.) Other than that, the patch LGTM. Personally I might not have bothered to add instructions for a 2010-vintage Git in 2021, though. Cheers, Daniel > - # for git prior to 1.7 > + # for git prior to 1.7.0 > # behind=$(git rev-list HEAD..origin/${hook_com[branch]} | wc -l) > - behind=$(git rev-list HEAD..${hook_com[branch]}@{upstream} 2>/dev/null | wc -l) > + # for git 1.7.0 and 1.7.1 > + # behind=$(git rev-list HEAD..@{upstream} 2>/dev/null | wc -l) > + behind=$(git rev-list --count HEAD..@{upstream} 2>/dev/null) > (( $behind )) && gitstatus+=( "-${behind}" ) > > hook_com[misc]+=${(j:/:)gitstatus} >
> > * Remove unnecessary use of `${hook_com[branch]}` because `@{upstream}` > > defaults to the current branch when no branch name is provided. > > `@{upstream}` was introduced in git 1.7.0 > > (https://github.com/git/git/commit/28fb84382b0eb728534dbe2972bbfec3f3d83dd9) > > I'm not sure that's unnecessary, for two reasons: > > 1. hook_com[branch], as opposed to hook_com[branch_orig], may have been > changed by a previous hook. > > 2. There may be educational value to demonstrating a use of > hook_com[branch], even if it's implied. > > WDYT? Okay. I was not aware of these reasons. I am still very much a beginner (both to zsh and to mailing lists ...). > > diff --git a/Misc/vcs_info-examples b/Misc/vcs_info-examples > > index 94b8a7b5e..36d4d3bf8 100644 > > --- a/Misc/vcs_info-examples > > +++ b/Misc/vcs_info-examples > > @@ -179,14 +179,18 @@ function +vi-git-st() { > > local ahead behind > > local -a gitstatus > > > > - # for git prior to 1.7 > > + # for git prior to 1.7.0 > > # ahead=$(git rev-list origin/${hook_com[branch]}..HEAD | wc -l) > > - ahead=$(git rev-list ${hook_com[branch]}@{upstream}..HEAD 2>/dev/null | wc -l) > > + # for git 1.7.0 and 1.7.1 > > + # ahead=$(git rev-list @{upstream}..HEAD 2>/dev/null | wc -l) > > + ahead=$(git rev-list --count @{upstream}..HEAD 2>/dev/null) > > (( $ahead )) && gitstatus+=( "+${ahead}" ) > > The version of this function in my zshrc starts like this: > . > git rev-parse @{upstream} >/dev/null 2>&1 || return 0 > local -a x=( $(git rev-list --left-right --count HEAD...@{upstream} ) ) The snippet below displays N/M. How would you make it display +N/-M instead? git rev-parse @{upstream} >/dev/null 2>&1 || return 0 local -a x=( $(git rev-list --left-right --count HEAD...@{upstream} ) ) hook_com[misc]="${(j:/:)x}" > The first line is to handle a detached HEAD, I think, and should > presumably be added? Why not do this instead: local -a x=( $(git rev-list --left-right --count HEAD...@{upstream} 2>/dev/null ) ) --- Note: when replying to my email, please ensure that my email address is included in the 'To:' field, since I am not subscribed to the mailing list.
Tim Lee wrote on Mon, 29 Mar 2021 09:30 +00:00: > > > * Remove unnecessary use of `${hook_com[branch]}` because `@{upstream}` > > > defaults to the current branch when no branch name is provided. > > > `@{upstream}` was introduced in git 1.7.0 > > > (https://github.com/git/git/commit/28fb84382b0eb728534dbe2972bbfec3f3d83dd9) > > > > I'm not sure that's unnecessary, for two reasons: > > > > 1. hook_com[branch], as opposed to hook_com[branch_orig], may have been > > changed by a previous hook. > > > > 2. There may be educational value to demonstrating a use of > > hook_com[branch], even if it's implied. > > > > WDYT? > > Okay. I was not aware of these reasons. I am still very much a beginner > (both to zsh and to mailing lists ...). > I couldn't tell that from your original mail ☺ > > > diff --git a/Misc/vcs_info-examples b/Misc/vcs_info-examples > > > index 94b8a7b5e..36d4d3bf8 100644 > > > --- a/Misc/vcs_info-examples > > > +++ b/Misc/vcs_info-examples > > > @@ -179,14 +179,18 @@ function +vi-git-st() { > > > local ahead behind > > > local -a gitstatus > > > > > > - # for git prior to 1.7 > > > + # for git prior to 1.7.0 > > > # ahead=$(git rev-list origin/${hook_com[branch]}..HEAD | wc -l) > > > - ahead=$(git rev-list ${hook_com[branch]}@{upstream}..HEAD 2>/dev/null | wc -l) > > > + # for git 1.7.0 and 1.7.1 > > > + # ahead=$(git rev-list @{upstream}..HEAD 2>/dev/null | wc -l) > > > + ahead=$(git rev-list --count @{upstream}..HEAD 2>/dev/null) > > > (( $ahead )) && gitstatus+=( "+${ahead}" ) > > > > The version of this function in my zshrc starts like this: > > . > > git rev-parse @{upstream} >/dev/null 2>&1 || return 0 > > local -a x=( $(git rev-list --left-right --count HEAD...@{upstream} ) ) > > The snippet below displays N/M. How would you make it display +N/-M > instead? Change the last line to hook_com[misc]="+${x[1]} -${x[2]}". > git rev-parse @{upstream} >/dev/null 2>&1 || return 0 > local -a x=( $(git rev-list --left-right --count HEAD...@{upstream} ) ) > hook_com[misc]="${(j:/:)x}" > > > The first line is to handle a detached HEAD, I think, and should > > presumably be added? > > Why not do this instead: > > local -a x=( $(git rev-list --left-right --count HEAD...@{upstream} 2>/dev/null ) ) You mean, why not discard stderr? Out of general principles ("Errors shouldn't be silenced"), plus the fact that I've never had that line generate an error that needed to be silenced — not with the «git rev-parse … || return» right above it. Or do you mean that line instead of the git-rev-parse(1) call too? First, that would run the rest of the function even though $x might be an empty array rather than a two-element one. Two, suppose there is an error after resolving «@{upstream}» to a commit but before rev-list finishes (for instance, an error while walking the commits history). With an explicit git-rev-parse(1) call that error would be reported, but if the rev-parse and the history walking are lumped into one step any failure from which is treated the same way, history walking failures would be masked — even though they're probably interesting. > --- > Note: when replying to my email, please ensure that my email address is > included in the 'To:' field, since I am not subscribed to the mailing > list. Ack, but for future reference, the customary signature delimiter is "-- " (ASCII 0x2D 0x2D 0x20). Many MUAs syntax-highlight that, for example. Cheers, Daniel
> > > > diff --git a/Misc/vcs_info-examples b/Misc/vcs_info-examples > > > > index 94b8a7b5e..36d4d3bf8 100644 > > > > --- a/Misc/vcs_info-examples > > > > +++ b/Misc/vcs_info-examples > > > > @@ -179,14 +179,18 @@ function +vi-git-st() { > > > > local ahead behind > > > > local -a gitstatus > > > > > > > > - # for git prior to 1.7 > > > > + # for git prior to 1.7.0 > > > > # ahead=$(git rev-list origin/${hook_com[branch]}..HEAD | wc -l) > > > > - ahead=$(git rev-list ${hook_com[branch]}@{upstream}..HEAD 2>/dev/null | wc -l) > > > > + # for git 1.7.0 and 1.7.1 > > > > + # ahead=$(git rev-list @{upstream}..HEAD 2>/dev/null | wc -l) > > > > + ahead=$(git rev-list --count @{upstream}..HEAD 2>/dev/null) > > > > (( $ahead )) && gitstatus+=( "+${ahead}" ) > > > > > > The version of this function in my zshrc starts like this: > > > . > > > git rev-parse @{upstream} >/dev/null 2>&1 || return 0 > > > local -a x=( $(git rev-list --left-right --count HEAD...@{upstream} ) ) > > > > The snippet below displays N/M. How would you make it display +N/-M > > instead? > > Change the last line to hook_com[misc]="+${x[1]} -${x[2]}". But hook_com[misc]="+${x[1]} -${x[2]}" would display a '+0' and/or '-0' when the local git branch is not ahead-of or behind the remote branch. The existing example does not display '+0' or '-0'. > > git rev-parse @{upstream} >/dev/null 2>&1 || return 0 > > local -a x=( $(git rev-list --left-right --count HEAD...@{upstream} ) ) > > hook_com[misc]="${(j:/:)x}" > > > > > The first line is to handle a detached HEAD, I think, and should > > > presumably be added? > > > > Why not do this instead: > > > > local -a x=( $(git rev-list --left-right --count HEAD...@{upstream} 2>/dev/null ) ) > > You mean, why not discard stderr? Out of general principles ("Errors > shouldn't be silenced"), plus the fact that I've never had that line > generate an error that needed to be silenced — not with the «git > rev-parse … || return» right above it. > > Or do you mean that line instead of the git-rev-parse(1) call too? Yes, I mean: instead of this: git rev-parse @{upstream} >/dev/null 2>&1 || return 0 local -a x=( $(git rev-list --left-right --count HEAD...@{upstream} ) ) Replace both of the lines above with this: local -a x=( $(git rev-list --left-right --count HEAD...@{upstream} 2>/dev/null ) ) > First, that would run the rest of the function even though $x might be > an empty array rather than a two-element one. Two, suppose there is an > error after resolving «@{upstream}» to a commit but before rev-list > finishes (for instance, an error while walking the commits history). > With an explicit git-rev-parse(1) call that error would be reported, but > if the rev-parse and the history walking are lumped into one step any > failure from which is treated the same way, history walking failures > would be masked — even though they're probably interesting. > > > --- > > Note: when replying to my email, please ensure that my email address is > > included in the 'To:' field, since I am not subscribed to the mailing > > list. > > Ack, but for future reference, the customary signature delimiter is "-- " > (ASCII 0x2D 0x2D 0x20). Many MUAs syntax-highlight that, for example. Thank you for all this information. I've learned so much today. :-)
Tim Lee wrote on Mon, 29 Mar 2021 10:39 +00:00:
> > > > > diff --git a/Misc/vcs_info-examples b/Misc/vcs_info-examples
> > > > > index 94b8a7b5e..36d4d3bf8 100644
> > > > > --- a/Misc/vcs_info-examples
> > > > > +++ b/Misc/vcs_info-examples
> > > > > @@ -179,14 +179,18 @@ function +vi-git-st() {
> > > > > local ahead behind
> > > > > local -a gitstatus
> > > > >
> > > > > - # for git prior to 1.7
> > > > > + # for git prior to 1.7.0
> > > > > # ahead=$(git rev-list origin/${hook_com[branch]}..HEAD | wc -l)
> > > > > - ahead=$(git rev-list ${hook_com[branch]}@{upstream}..HEAD 2>/dev/null | wc -l)
> > > > > + # for git 1.7.0 and 1.7.1
> > > > > + # ahead=$(git rev-list @{upstream}..HEAD 2>/dev/null | wc -l)
> > > > > + ahead=$(git rev-list --count @{upstream}..HEAD 2>/dev/null)
> > > > > (( $ahead )) && gitstatus+=( "+${ahead}" )
> > > >
> > > > The version of this function in my zshrc starts like this:
> > > > .
> > > > git rev-parse @{upstream} >/dev/null 2>&1 || return 0
> > > > local -a x=( $(git rev-list --left-right --count HEAD...@{upstream} ) )
> > >
> > > The snippet below displays N/M. How would you make it display +N/-M
> > > instead?
> >
> > Change the last line to hook_com[misc]="+${x[1]} -${x[2]}".
>
> But hook_com[misc]="+${x[1]} -${x[2]}" would display a '+0' and/or '-0'
> when the local git branch is not ahead-of or behind the remote branch.
>
> The existing example does not display '+0' or '-0'.
You didn't specify this when you asked. Anyway, if you want not to show
+0 or -0, then add the appropriate logic — e.g., «if (( x[2] )); then».
We're on -workers@ so I thought that'd go without saying.
A bit further out along the "readable"–"line noise" spectrum there is
this alternative:
hook_com[misc]="+$x[1]/-$x[2]"
hook_com[misc]=${${hook_com[misc]#'+0/'}%'/-0'}
Cheers,
Daniel
On Mon, Mar 29, 2021, at 5:52 AM, Daniel Shahaf wrote: > Tim Lee wrote on Mon, 29 Mar 2021 09:30 +00:00: > > --- > > Note: when replying to my email, please ensure that my email address is > > included in the 'To:' field, since I am not subscribed to the mailing > > list. what, too good for cc/bcc? ;) > Ack, but for future reference, the customary signature delimiter is "-- " > (ASCII 0x2D 0x2D 0x20). Many MUAs syntax-highlight that, for example. [whoa.gif] vq
Anything else on this? vq
Lawrence Velázquez wrote on Sat, Apr 10, 2021 at 16:51:43 -0400: > Anything else on this? Tim: Would you have time to address the remaining points from the original review, https://zsh.org/workers/48306, either by implementing them and sending a revised patch, or by counterarguing them? Cheers, Daniel
> Tim: Would you have time to address the remaining points from the > original review, https://zsh.org/workers/48306, either by implementing > them and sending a revised patch, or by counterarguing them? I think I'll just porpose something simple, since I'm not very familiar with ZSH. Patch: diff --git a/Misc/vcs_info-examples b/Misc/vcs_info-examples index 94b8a7b5e..d33d0ceed 100644 --- a/Misc/vcs_info-examples +++ b/Misc/vcs_info-examples @@ -179,14 +179,10 @@ function +vi-git-st() { local ahead behind local -a gitstatus - # for git prior to 1.7 - # ahead=$(git rev-list origin/${hook_com[branch]}..HEAD | wc -l) - ahead=$(git rev-list ${hook_com[branch]}@{upstream}..HEAD 2>/dev/null | wc -l) + ahead=$(git rev-list --count ${hook_com[branch]}@{upstream}..HEAD 2>/dev/null) (( $ahead )) && gitstatus+=( "+${ahead}" ) - # for git prior to 1.7 - # behind=$(git rev-list HEAD..origin/${hook_com[branch]} | wc -l) - behind=$(git rev-list HEAD..${hook_com[branch]}@{upstream} 2>/dev/null | wc -l) + behind=$(git rev-list --count HEAD..${hook_com[branch]}@{upstream} 2>/dev/null) (( $behind )) && gitstatus+=( "-${behind}" ) hook_com[misc]+=${(j:/:)gitstatus}
[-- Attachment #1: Type: text/plain, Size: 559 bytes --] Tim Lee wrote on Tue, Apr 13, 2021 at 19:58:30 +0800: > > Tim: Would you have time to address the remaining points from the > > original review, https://zsh.org/workers/48306, either by implementing > > them and sending a revised patch, or by counterarguing them? > > I think I'll just porpose something simple, since I'm not very familiar > with ZSH. > > Patch: Thanks, applied, and here's a patch series for implementing the other outstanding points. Would you review it, please? See anything that could go wrong, or that's missing? Cheers, Daniel [-- Attachment #2: 0001-vcs_info-git-docs-ahead-behind-commits-Don-t-run.patch.txt --] [-- Type: text/plain, Size: 850 bytes --] From 8a521515ce88802846df8884cabdd3966d0f32a8 Mon Sep 17 00:00:00 2001 From: Daniel Shahaf <d.s@daniel.shahaf.name> Date: Tue, 13 Apr 2021 13:56:36 +0000 Subject: [PATCH 1/2] vcs_info git docs: ahead/behind commits: Don't run rev-list when that would fail --- Misc/vcs_info-examples | 3 +++ 1 file changed, 3 insertions(+) diff --git a/Misc/vcs_info-examples b/Misc/vcs_info-examples index d33d0ceed..065ea6cb3 100644 --- a/Misc/vcs_info-examples +++ b/Misc/vcs_info-examples @@ -179,6 +179,9 @@ function +vi-git-st() { local ahead behind local -a gitstatus + # Exit early in case the worktree is on a detached HEAD + git rev-parse ${hook_com[branch]}@{upstream} >/dev/null 2>&1 || return 0 + ahead=$(git rev-list --count ${hook_com[branch]}@{upstream}..HEAD 2>/dev/null) (( $ahead )) && gitstatus+=( "+${ahead}" ) [-- Attachment #3: 0002-vcs_info-git-docs-ahead-behind-commits-Reduce-th.patch.txt --] [-- Type: text/plain, Size: 1245 bytes --] From d084af57318a98f5e3ff197e269f42611e5d2443 Mon Sep 17 00:00:00 2001 From: Daniel Shahaf <d.s@daniel.shahaf.name> Date: Tue, 13 Apr 2021 13:56:31 +0000 Subject: [PATCH 2/2] vcs_info git docs: ahead/behind commits: Reduce the number of forks --- Misc/vcs_info-examples | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/Misc/vcs_info-examples b/Misc/vcs_info-examples index 065ea6cb3..ba94cca29 100644 --- a/Misc/vcs_info-examples +++ b/Misc/vcs_info-examples @@ -182,10 +182,14 @@ function +vi-git-st() { # Exit early in case the worktree is on a detached HEAD git rev-parse ${hook_com[branch]}@{upstream} >/dev/null 2>&1 || return 0 - ahead=$(git rev-list --count ${hook_com[branch]}@{upstream}..HEAD 2>/dev/null) - (( $ahead )) && gitstatus+=( "+${ahead}" ) + local -a ahead_and_behind=( + $(git rev-list --left-right --count HEAD...${hook_com[branch]}@{upstream} 2>/dev/null) + ) - behind=$(git rev-list --count HEAD..${hook_com[branch]}@{upstream} 2>/dev/null) + ahead=${ahead_and_behind[1]} + behind=${ahead_and_behind[2]} + + (( $ahead )) && gitstatus+=( "+${ahead}" ) (( $behind )) && gitstatus+=( "-${behind}" ) hook_com[misc]+=${(j:/:)gitstatus}
> Thanks, applied, and here's a patch series for implementing the other
> outstanding points. Would you review it, please? See anything that
> could go wrong, or that's missing?
Looks good to me. I would use this myself. However, you should take my
review with a grain of salt.