zsh-workers
 help / color / mirror / code / Atom feed
* [PATCH] Remove _stgit completion script
@ 2022-10-25 20:43 Peter Grayson
  2022-10-31  9:19 ` Daniel Shahaf
  0 siblings, 1 reply; 16+ messages in thread
From: Peter Grayson @ 2022-10-25 20:43 UTC (permalink / raw)
  To: zsh-workers; +Cc: Peter Grayson

The StGit project ships its own zsh completion script which is more
complete and up-to-date than those shipped with zsh.

https://github.com/stacked-git/stgit/blob/master/completion/stgit.zsh

Also, the _stgit completions that ship with zsh, which dynamically parse
StGit's help output, will be broken by the upcoming StGit 2.0 release
due to changes in the help output.

Signed-off-by: Peter Grayson <pete@jpgrayson.net>
---

N.B. I am the StGit maintainer and primary author of StGit's zsh
completion script.

 Completion/Unix/Command/_stgit | 52 ----------------------------------
 1 file changed, 52 deletions(-)
 delete mode 100644 Completion/Unix/Command/_stgit

diff --git a/Completion/Unix/Command/_stgit b/Completion/Unix/Command/_stgit
deleted file mode 100644
index e31af460a..000000000
--- a/Completion/Unix/Command/_stgit
+++ /dev/null
@@ -1,52 +0,0 @@
-#compdef stg
-
-typeset -a subcmds
-
-subcmds=( ${${${(M)${(f)"$(stg help 2> /dev/null)"}## *}#  }/#(#b)([^[:space:]]##)[[:space:]]##(*)/$match[1]:$match[2]} )
-
-local curcontext="$curcontext" expl
-local subcmd
-local ret=1
-
-if (( CURRENT == 2 )); then
-  _describe -t commands 'stgit command' subcmds && ret=0
-else
-  shift words
-  (( CURRENT-- ))
-  subcmd="$words[1]"
-  curcontext="${curcontext%:*}-${subcmd}:"
-
-  case $subcmd in
-    (push)
-      _wanted -V unapplied-patches expl "patch" \
-       	compadd ${${(M)${(f)"$(stg series 2> /dev/null)"}##- *}#- } \
-		&& ret=0
-    ;;
-    (pop)
-      _wanted -V applied-patches expl "patch" \
-	compadd ${${(M)${(f)"$(stg series 2> /dev/null)"}##[+>] *}#[+>] } \
-		&& ret=0
-    ;;
-    (edit|files|goto|rename|log|float|delete|sink|mail|sync|show|pick|hide|squash)
-      _wanted -V patches expl "patch" \
-	compadd $(stg series --noprefix 2> /dev/null) \
-		&& ret=0
-    ;;
-    (ref*)
-      last_word="$words[$CURRENT-1]"
-      refresh_patch_options=( -p --patch )
-      if [[ -n ${refresh_patch_options[(r)$last_word]} ]]; then
-	_wanted -V applied-patches expl "patch" \
-	  compadd ${${(M)${(f)"$(stg series 2> /dev/null)"}##[+>] *}#[+>] } \
-		  && ret=0
-      else
-	_files && ret=0
-      fi
-    ;;
-    (*)
-      _files && ret=0
-    ;;
-  esac
-fi
-
-return ret
-- 
2.38.1



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] Remove _stgit completion script
  2022-10-25 20:43 [PATCH] Remove _stgit completion script Peter Grayson
@ 2022-10-31  9:19 ` Daniel Shahaf
  2022-10-31 10:13   ` StGit 2.0 and vcs_info (was: Re: [PATCH] Remove _stgit completion script) Daniel Shahaf
  2022-11-01  3:00   ` [PATCH] Remove _stgit completion script Peter Grayson
  0 siblings, 2 replies; 16+ messages in thread
From: Daniel Shahaf @ 2022-10-31  9:19 UTC (permalink / raw)
  To: Peter Grayson; +Cc: zsh-workers

Peter Grayson wrote on Tue, Oct 25, 2022 at 16:43:21 -0400:
> The StGit project ships its own zsh completion script which is more
> complete and up-to-date than those shipped with zsh.
> 
> https://github.com/stacked-git/stgit/blob/master/completion/stgit.zsh
> 

+ I see that completion script gets updated in lockstep with stg(1)'s
  argv parser.

+ Given the two projects' release schedules it seems safe to assume that
  by zsh's next release stg 2.0 will have been released as well, so most
  users won't end up in a situation where they have neither _stgit nor
  stgit.zsh. 

+ No objections in a week.

- _stgit and stgit.zsh are licenced differently.

Being licenced under the same terms as zsh itself seems to be outweighed
by not being regularly updated.  So, I've pushed this.  The last version
is of course still available in our version control history.

> Also, the _stgit completions that ship with zsh, which dynamically parse
> StGit's help output, will be broken by the upcoming StGit 2.0 release
> due to changes in the help output.
> 

StGit is also used by vcs_info, here:

https://github.com/zsh-users/zsh/blob/b76dcecfe3461aa9a9e29dffe2484f097611f9ff/Functions/VCS_Info/Backends/VCS_INFO_get_data_git#L187-L194

(There doesn't seem to be StGit-specific code elsewhere in the file or
in the sibling file VCS_INFO_detect_git.)

Any chance that someone who uses both StGit and zsh could review that?
That code is supposed to populate $gitmisc (and thus the %m expando)
with information about pushed and not-yet-pushed patches.

To test that:

[[[
autoload vcs_info
precmd() { vcs_info }
zstyle ':vcs_info:*' formats %m
zstyle ':vcs_info:*' actionformats %m
]]]

> Signed-off-by: Peter Grayson <pete@jpgrayson.net>
> ---
> 
> N.B. I am the StGit maintainer and primary author of StGit's zsh
> completion script.
> 

Welcome :-)

Cheers,

Daniel

>  Completion/Unix/Command/_stgit | 52 ----------------------------------
>  1 file changed, 52 deletions(-)
>  delete mode 100644 Completion/Unix/Command/_stgit
> 
> diff --git a/Completion/Unix/Command/_stgit b/Completion/Unix/Command/_stgit
> deleted file mode 100644
> index e31af460a..000000000
> --- a/Completion/Unix/Command/_stgit
> +++ /dev/null
> @@ -1,52 +0,0 @@
> -#compdef stg
> -return ret


^ permalink raw reply	[flat|nested] 16+ messages in thread

* StGit 2.0 and vcs_info (was: Re: [PATCH] Remove _stgit completion script)
  2022-10-31  9:19 ` Daniel Shahaf
@ 2022-10-31 10:13   ` Daniel Shahaf
  2022-11-01  3:00   ` [PATCH] Remove _stgit completion script Peter Grayson
  1 sibling, 0 replies; 16+ messages in thread
From: Daniel Shahaf @ 2022-10-31 10:13 UTC (permalink / raw)
  To: Peter Grayson; +Cc: zsh-workers

Daniel Shahaf wrote on Mon, 31 Oct 2022 09:19 +00:00:
> Peter Grayson wrote on Tue, Oct 25, 2022 at 16:43:21 -0400:
>> Also, the _stgit completions that ship with zsh, which dynamically parse
>> StGit's help output, will be broken by the upcoming StGit 2.0 release
>> due to changes in the help output.
>> 
>
> StGit is also used by vcs_info, here:
>
> https://github.com/zsh-users/zsh/blob/b76dcecfe3461aa9a9e29dffe2484f097611f9ff/Functions/VCS_Info/Backends/VCS_INFO_get_data_git#L187-L194
>
> (There doesn't seem to be StGit-specific code elsewhere in the file or
> in the sibling file VCS_INFO_detect_git.)
>
> Any chance that someone who uses both StGit and zsh could review that?
> That code is supposed to populate $gitmisc (and thus the %m expando)
> with information about pushed and not-yet-pushed patches.
>
> To test that:
>
> [[[
> autoload vcs_info
> precmd() { vcs_info }

Need to emit ${vcs_info_msg_0_} as well (see Misc/vcs_info-examples).

> zstyle ':vcs_info:*' formats %m
> zstyle ':vcs_info:*' actionformats %m
> ]]]

Cheers,

Daniel


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] Remove _stgit completion script
  2022-10-31  9:19 ` Daniel Shahaf
  2022-10-31 10:13   ` StGit 2.0 and vcs_info (was: Re: [PATCH] Remove _stgit completion script) Daniel Shahaf
@ 2022-11-01  3:00   ` Peter Grayson
  2022-11-01  3:04     ` [PATCH] Remove StGit patch detection from vcs_info Peter Grayson
  1 sibling, 1 reply; 16+ messages in thread
From: Peter Grayson @ 2022-11-01  3:00 UTC (permalink / raw)
  To: Daniel Shahaf; +Cc: zsh-workers

Thank you for taking the time to consider and merge this patch.

On Mon, Oct 31, 2022, at 5:19 AM, Daniel Shahaf wrote:
> Peter Grayson wrote on Tue, Oct 25, 2022 at 16:43:21 -0400:
>> The StGit project ships its own zsh completion script which is more
>> complete and up-to-date than those shipped with zsh.
>> 
>> https://github.com/stacked-git/stgit/blob/master/completion/stgit.zsh
>> 
>
> + I see that completion script gets updated in lockstep with stg(1)'s
>   argv parser.

Yes, maintaining that tight coupling between stg(1)'s command line
parsing and its comprehensive completion script has been important for
StGit development velocity. Although, maybe less velocity on the horizon
post StGit 2.0.

> + Given the two projects' release schedules it seems safe to assume that
>   by zsh's next release stg 2.0 will have been released as well, so most
>   users won't end up in a situation where they have neither _stgit nor
>   stgit.zsh.

StGit has been releasing its own zsh completion script since at least 1.0,
so there is not even a minor release timing concern.
 
> + No objections in a week.
>
> - _stgit and stgit.zsh are licenced differently.
>
> Being licenced under the same terms as zsh itself seems to be outweighed
> by not being regularly updated.  So, I've pushed this.  The last version
> is of course still available in our version control history.

If at any point we think it makes sense to migrate StGit's completion
script [back] to the zsh project, I would be happy to relicense.

> StGit is also used by vcs_info, here:
>
> https://github.com/zsh-users/zsh/blob/b76dcecfe3461aa9a9e29dffe2484f097611f9ff/Functions/VCS_Info/Backends/VCS_INFO_get_data_git#L187-L194
>
> (There doesn't seem to be StGit-specific code elsewhere in the file or
> in the sibling file VCS_INFO_detect_git.)
>
> Any chance that someone who uses both StGit and zsh could review that?
> That code is supposed to populate $gitmisc (and thus the %m expando)
> with information about pushed and not-yet-pushed patches.

Thanks for pointing this out--I didn't know about it. I've taken a look
and my conclusion is that the StGit patch state interrogation currently
found in VCS_INFO_get_data_git should also just simply be removed. I'll
send another patch shortly with the rationale in the commentary.

Cheers,
Pete


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH] Remove StGit patch detection from vcs_info
  2022-11-01  3:00   ` [PATCH] Remove _stgit completion script Peter Grayson
@ 2022-11-01  3:04     ` Peter Grayson
  2022-11-11 11:49       ` Daniel Shahaf
  0 siblings, 1 reply; 16+ messages in thread
From: Peter Grayson @ 2022-11-01  3:04 UTC (permalink / raw)
  To: zsh-workers, Daniel Shahaf; +Cc: Peter Grayson

The vcs_info patch detection code attempted to interrogate StGit patch
stack state by inspecting .git/patches/applied and
.git/patches/unapplied. As of StGit 1.0, StGit stack and patch state is
no longer maintained via files in the .git/ directory, but is instead
captured in the repo's object database, accessible via the
refs/stacks/<branch> reference.

Thus zsh's approach for interrogating StGit patch state is long
obsoleted.

This patch excises the code that attempts to interrogate StGit stack
state. It would alternatively be possible to repair this code to
interrogate the StGit stack state in a different manner, but:

- It is not clear that StGit is a sufficiently popular tool to warrant
  direct inclusion in zsh.
- Interrogating the StGit stack needs to be done using StGit's
  proscribed interface, the stg executable, and not by interrogating
  either the .git filesystem nor by inspecting the git object database
  with the git executable.
- StGit versions prior to 2.0 are implemented in Python and thus have an
  unacceptable runtime overhead to be included in vcs_info (on the order
  of hundreds of milliseconds).
- StGit 2.0 is implemented in Rust is considerably faster (a few tens of
  milliseconds to interrogate stack state), but any non-zero overhead to
  vcs_info still seems like too much given the value of this patch
  information and the relative non-popularity of StGit.
- Just removing the code is the lowest-risk approach for the zsh code
  base.

Signed-off-by: Peter Grayson <pete@jpgrayson.net>
---
 Functions/VCS_Info/Backends/VCS_INFO_get_data_git | 11 ++---------
 1 file changed, 2 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 e45eebc8e..a3f4dbdf0 100644
--- a/Functions/VCS_Info/Backends/VCS_INFO_get_data_git
+++ b/Functions/VCS_Info/Backends/VCS_INFO_get_data_git
@@ -184,15 +184,8 @@ fi
 VCS_INFO_adjust
 VCS_INFO_git_getaction ${gitdir}
 
-local patchdir=${gitdir}/patches/${gitbranch}
-if [[ -d $patchdir ]] && [[ -f $patchdir/applied ]] \
-   && [[ -f $patchdir/unapplied ]]
-then
-    # stgit
-    git_patches_applied=(${(f)"$(< "${patchdir}/applied")"})
-    git_patches_unapplied=(${(f)"$(< "${patchdir}/unapplied")"})
-    VCS_INFO_git_handle_patches
-elif [[ -d "${gitdir}/rebase-merge" ]]; then
+local patchdir
+if [[ -d "${gitdir}/rebase-merge" ]]; then
     # 'git rebase -i'
     patchdir="${gitdir}/rebase-merge"
     local p
-- 
2.38.1



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] Remove StGit patch detection from vcs_info
  2022-11-01  3:04     ` [PATCH] Remove StGit patch detection from vcs_info Peter Grayson
@ 2022-11-11 11:49       ` Daniel Shahaf
  2022-11-12 14:46         ` Peter Grayson
  0 siblings, 1 reply; 16+ messages in thread
From: Daniel Shahaf @ 2022-11-11 11:49 UTC (permalink / raw)
  To: Peter Grayson; +Cc: zsh-workers

Sorry for my long RTT on this.

Peter Grayson wrote on Mon, Oct 31, 2022 at 23:04:30 -0400:
> The vcs_info patch detection code attempted to interrogate StGit patch
> stack state by inspecting .git/patches/applied and
> .git/patches/unapplied. As of StGit 1.0, StGit stack and patch state is
> no longer maintained via files in the .git/ directory, but is instead
> captured in the repo's object database, accessible via the
> refs/stacks/<branch> reference.
> 
> Thus zsh's approach for interrogating StGit patch state is long
> obsoleted.
> 

*nod*

> This patch excises the code that attempts to interrogate StGit stack
> state. It would alternatively be possible to repair this code to
> interrogate the StGit stack state in a different manner, but:
> 

Hang on.  This isn't a binary decision, "remove the code or update it to
support StGit 2.x".  This is actually two independent decisions:

1. Whether to keep or remove the StGit 0.x support code.

2. Whether patches adding StGit 2.x support code would be considered.

> - It is not clear that StGit is a sufficiently popular tool to warrant
>   direct inclusion in zsh.

[re #1]: The code is already written (i.e., a sunk cost).  The ongoing
costs for vcs_info users who don't use StGit, for vcs_info users who use
stg≥1.x, and for vcs_info maintainers are minimal.  Conversely,
/removing/ the code might break some users' proverbial toaster heating —
e.g., users who self-compile latest zsh on LTS distros, and users on
current distros that still ship stgit 0.x <https://repology.org/project/stgit/versions>.
We can document (in comments, for users, or both) that the version of
StGit vcs_info supports is EOL.

[re #2]:

- I expect implementing StGit 2.x support would be a nearly one-time
  cost with long-time returns (especially as StGit 3.x isn't on the
  horizon).  Low popularity over a long time is still a fair justification.

- Adding StGit support to vcs_info might cause some zsh users to
  discover and begin to use StGit.  (That's not a reason for including
  StGit support; that's just a cost/benefit analysis of implementing
  StGit 2.x support.  The number of people who use both vcs_info and
  StGit relates to the denominator.)

- StGit support can be written in such a way that the effect on zsh
  users who don't use StGit would be minimal.

- Maintainers who advise against their own tools are generally exactly
  those we'd like to work with :)

> - Interrogating the StGit stack needs to be done using StGit's
>   proscribed interface, the stg executable, and not by interrogating
("prescribed"?)
>   either the .git filesystem nor by inspecting the git object database
>   with the git executable.

[re #1]: Considering there are unlikely to be any further stg-0.x
releases, and that newer stg's internals are different, the layering
violation in VCS_INFO_get_data_git — though admittedly not ideal — seems
unlikely to cause any breakage down the road.  [Famous last words, yes.]

[re #2]: Fair enough.  VCS_INFO_get_data_git does tend to poke around in
.git dirs directly — partly for performance, partly because when the
code was written there mightn't have been APIs for what we needed — but
agreed that new code should use APIs if possible.

> - StGit versions prior to 2.0 are implemented in Python and thus have an
>   unacceptable runtime overhead to be included in vcs_info (on the order
>   of hundreds of milliseconds).

[re both stg-0.x and stg-1.x]:

And why would that be unacceptable?  A few hundreds of milliseconds —
let's round up and say a microsecond — is short enough that "Press
<Enter> on an empty prompt and get a new one instantly" would still be
the user's experience, wouldn't it?

> - StGit 2.0 is implemented in Rust is considerably faster (a few tens of
>   milliseconds to interrogate stack state), but any non-zero overhead to
>   vcs_info still seems like too much given the value of this patch
>   information and the relative non-popularity of StGit.

[re #2]: If the overhead is more than we consider acceptable, couldn't
this be addressed by implementing StGit 2.x support but making it off by
default (require an opt-in)?  Compare the «check-for-staged-changes» and
«get-unapplied» styles, and contrast «use-simple» and «enable».

For reference, the stg-0.x support code performs one stat(2) for people
who don't use StGit, v. three stat(2)s and two cat(1)s for those who do,
and seemed to work well enough when I last tested it (on a toy example).

> - Just removing the code is the lowest-risk approach for the zsh code
>   base.

[re #1]: I would hesitate to remove that code.  It supports a now-EOL
version, true, but it may still have users, and induces little cost for
other users and for vcs_info maintainers.  WDYT?

[re #2]: It sounds like StGit 2.x support can be implemented at the cost
of one fork(2) for those who don't use StGit and under a microsecond for
those who do.  That doesn't sound like a deal breaker at all.

Furthermore, will StGit 2.x users want to show in the prompt information
about the patch stack?  If so, I'd say that's an argument in favour of
including StGit 2.x support in vcs_info, since vcs_info is designed to
allow this.  (Also, code — say, applied-string hooks — can be reused
across stg(1) and other kinds of patch series.)

Your points — cost/benefit, forward compatibility, performancy, etc. —
are valid and, should a patch implementing support for stg-2.x be
(UK)tabled, would be good to keep in mind during review.  I'd welcome
your review of any such particular patch, too, should you have the time
and inclination.  However, I don't see any obvious roadblock to
implementing StGit 2.x support in zsh.

Thanks for the willingness (elsethread) to relicence, and Congrats on
the 2.0 release :)

Cheers,

Daniel

> Signed-off-by: Peter Grayson <pete@jpgrayson.net>
> ---
>  Functions/VCS_Info/Backends/VCS_INFO_get_data_git | 11 ++---------
>  1 file changed, 2 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 e45eebc8e..a3f4dbdf0 100644
> --- a/Functions/VCS_Info/Backends/VCS_INFO_get_data_git
> +++ b/Functions/VCS_Info/Backends/VCS_INFO_get_data_git
> @@ -184,15 +184,8 @@ fi
>  VCS_INFO_adjust
>  VCS_INFO_git_getaction ${gitdir}
>  
> -local patchdir=${gitdir}/patches/${gitbranch}
> -if [[ -d $patchdir ]] && [[ -f $patchdir/applied ]] \
> -   && [[ -f $patchdir/unapplied ]]
> -then
> -    # stgit
> -    git_patches_applied=(${(f)"$(< "${patchdir}/applied")"})
> -    git_patches_unapplied=(${(f)"$(< "${patchdir}/unapplied")"})
> -    VCS_INFO_git_handle_patches
> -elif [[ -d "${gitdir}/rebase-merge" ]]; then
> +local patchdir
> +if [[ -d "${gitdir}/rebase-merge" ]]; then
>      # 'git rebase -i'
>      patchdir="${gitdir}/rebase-merge"
>      local p
> -- 
> 2.38.1
> 
> 



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] Remove StGit patch detection from vcs_info
  2022-11-11 11:49       ` Daniel Shahaf
@ 2022-11-12 14:46         ` Peter Grayson
  2022-11-12 15:42           ` [PATCH] Updated StGit patch detection in vcs_info Peter Grayson
  2022-11-13  4:30           ` [PATCH] Remove StGit patch detection from vcs_info Daniel Shahaf
  0 siblings, 2 replies; 16+ messages in thread
From: Peter Grayson @ 2022-11-12 14:46 UTC (permalink / raw)
  To: Daniel Shahaf; +Cc: zsh-workers

On Fri, Nov 11, 2022, at 6:49 AM, Daniel Shahaf wrote:
> Sorry for my long RTT on this.

Thank you for this detailed response.

> Peter Grayson wrote on Mon, Oct 31, 2022 at 23:04:30 -0400:
>> The vcs_info patch detection code attempted to interrogate StGit patch
>> stack state by inspecting .git/patches/applied and
>> .git/patches/unapplied. As of StGit 1.0, StGit stack and patch state is
>> no longer maintained via files in the .git/ directory, but is instead
>> captured in the repo's object database, accessible via the
>> refs/stacks/<branch> reference.
>> 
>> Thus zsh's approach for interrogating StGit patch state is long
>> obsoleted. 
>
> *nod*
>
>> This patch excises the code that attempts to interrogate StGit stack
>> state. It would alternatively be possible to repair this code to
>> interrogate the StGit stack state in a different manner, but:
>
> Hang on.  This isn't a binary decision, "remove the code or update it to
> support StGit 2.x".  This is actually two independent decisions:
>
> 1. Whether to keep or remove the StGit 0.x support code.
>
> 2. Whether patches adding StGit 2.x support code would be considered.

Or option 3, which would be to support all versions of StGit by using
stg command lines that are compatible with all versions of StGit going
back to 2008.

>> - It is not clear that StGit is a sufficiently popular tool to warrant
>>   direct inclusion in zsh.
>
> [re #1]: The code is already written (i.e., a sunk cost).  The ongoing
> costs for vcs_info users who don't use StGit, for vcs_info users who use
> stg≥1.x, and for vcs_info maintainers are minimal.  Conversely,
> /removing/ the code might break some users' proverbial toaster heating —
> e.g., users who self-compile latest zsh on LTS distros, and users on
> current distros that still ship stgit 0.x 
> <https://repology.org/project/stgit/versions>.
> We can document (in comments, for users, or both) that the version of
> StGit vcs_info supports is EOL.
>
> [re #2]:
>
> - I expect implementing StGit 2.x support would be a nearly one-time
>   cost with long-time returns (especially as StGit 3.x isn't on the
>   horizon).  Low popularity over a long time is still a fair justification.
>
> - Adding StGit support to vcs_info might cause some zsh users to
>   discover and begin to use StGit.  (That's not a reason for including
>   StGit support; that's just a cost/benefit analysis of implementing
>   StGit 2.x support.  The number of people who use both vcs_info and
>   StGit relates to the denominator.)
>
> - StGit support can be written in such a way that the effect on zsh
>   users who don't use StGit would be minimal.
>
> - Maintainers who advise against their own tools are generally exactly
>   those we'd like to work with :)
>
>> - Interrogating the StGit stack needs to be done using StGit's
>>   proscribed interface, the stg executable, and not by interrogating
> ("prescribed"?)
>>   either the .git filesystem nor by inspecting the git object database
>>   with the git executable.
>
> [re #1]: Considering there are unlikely to be any further stg-0.x
> releases, and that newer stg's internals are different, the layering
> violation in VCS_INFO_get_data_git — though admittedly not ideal — seems
> unlikely to cause any breakage down the road.  [Famous last words, yes.]
>
> [re #2]: Fair enough.  VCS_INFO_get_data_git does tend to poke around in
> .git dirs directly — partly for performance, partly because when the
> code was written there mightn't have been APIs for what we needed — but
> agreed that new code should use APIs if possible.
>
>> - StGit versions prior to 2.0 are implemented in Python and thus have an
>>   unacceptable runtime overhead to be included in vcs_info (on the order
>>   of hundreds of milliseconds).
>
> [re both stg-0.x and stg-1.x]:
>
> And why would that be unacceptable?  A few hundreds of milliseconds —
> let's round up and say a microsecond — is short enough that "Press
> <Enter> on an empty prompt and get a new one instantly" would still be
> the user's experience, wouldn't it?

Rounding up a few hundred milliseconds gets into the *seconds* domain, not
microseconds.

That said, I think StGit can be supported in zsh such that only users with
StGit <2.0 installed would possibly be exposed to undue overhead.

>> - StGit 2.0 is implemented in Rust is considerably faster (a few tens of
>>   milliseconds to interrogate stack state), but any non-zero overhead to
>>   vcs_info still seems like too much given the value of this patch
>>   information and the relative non-popularity of StGit.
>
> [re #2]: If the overhead is more than we consider acceptable, couldn't
> this be addressed by implementing StGit 2.x support but making it off by
> default (require an opt-in)?  Compare the «check-for-staged-changes» and
> «get-unapplied» styles, and contrast «use-simple» and «enable».
>
> For reference, the stg-0.x support code performs one stat(2) for people
> who don't use StGit, v. three stat(2)s and two cat(1)s for those who do,
> and seemed to work well enough when I last tested it (on a toy example).
>
>> - Just removing the code is the lowest-risk approach for the zsh code
>>   base.
>
> [re #1]: I would hesitate to remove that code.  It supports a now-EOL
> version, true, but it may still have users, and induces little cost for
> other users and for vcs_info maintainers.  WDYT?

You've convinced me that retaining StGit support in zsh's vcs_info is the
way to go. And I've convinced myself that supporting all versions of StGit
is straightforward. So let's do that.

> [re #2]: It sounds like StGit 2.x support can be implemented at the cost
> of one fork(2) for those who don't use StGit and under a microsecond for
> those who do.  That doesn't sound like a deal breaker at all.

Running `stg series` with StGit 2.0 takes about 12ms in my environment.
StGit 1.5 it is about 32ms. Not a microsecond, but perhaps acceptable
nonetheless.

> Furthermore, will StGit 2.x users want to show in the prompt information
> about the patch stack?  If so, I'd say that's an argument in favour of
> including StGit 2.x support in vcs_info, since vcs_info is designed to
> allow this.  (Also, code — say, applied-string hooks — can be reused
> across stg(1) and other kinds of patch series.)

I need to admit that I have been a long-time user of prezto and its
sorin prompt which uses its own async vcs_info alternative (git-info),
and so I have not been a user of vcs_info.

This discussion prompted me to try out vcs_info, including its support
for applied/unapplied patches. And I've patched it to support StGit
2.0 as well as older StGit versions by using stg commands as discussed.
Its working great and having patch series info in my prompt is nice.

> Your points — cost/benefit, forward compatibility, performancy, etc. —
> are valid and, should a patch implementing support for stg-2.x be
> (UK)tabled, would be good to keep in mind during review.  I'd welcome
> your review of any such particular patch, too, should you have the time
> and inclination.  However, I don't see any obvious roadblock to
> implementing StGit 2.x support in zsh.

Patch to update StGit support will be forthcoming.

> Thanks for the willingness (elsethread) to relicence, and Congrats on
> the 2.0 release :)

Thank you!

> Cheers,
>
> Daniel
>
>> Signed-off-by: Peter Grayson <pete@jpgrayson.net>
>> ---
>>  Functions/VCS_Info/Backends/VCS_INFO_get_data_git | 11 ++---------
>>  1 file changed, 2 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 e45eebc8e..a3f4dbdf0 100644
>> --- a/Functions/VCS_Info/Backends/VCS_INFO_get_data_git
>> +++ b/Functions/VCS_Info/Backends/VCS_INFO_get_data_git
>> @@ -184,15 +184,8 @@ fi
>>  VCS_INFO_adjust
>>  VCS_INFO_git_getaction ${gitdir}
>>  
>> -local patchdir=${gitdir}/patches/${gitbranch}
>> -if [[ -d $patchdir ]] && [[ -f $patchdir/applied ]] \
>> -   && [[ -f $patchdir/unapplied ]]
>> -then
>> -    # stgit
>> -    git_patches_applied=(${(f)"$(< "${patchdir}/applied")"})
>> -    git_patches_unapplied=(${(f)"$(< "${patchdir}/unapplied")"})
>> -    VCS_INFO_git_handle_patches
>> -elif [[ -d "${gitdir}/rebase-merge" ]]; then
>> +local patchdir
>> +if [[ -d "${gitdir}/rebase-merge" ]]; then
>>      # 'git rebase -i'
>>      patchdir="${gitdir}/rebase-merge"
>>      local p
>> -- 
>> 2.38.1
>> 
>>


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH] Updated StGit patch detection in vcs_info
  2022-11-12 14:46         ` Peter Grayson
@ 2022-11-12 15:42           ` Peter Grayson
  2022-11-13  4:58             ` Daniel Shahaf
  2022-11-16 20:45             ` [PATCH v2] " Peter Grayson
  2022-11-13  4:30           ` [PATCH] Remove StGit patch detection from vcs_info Daniel Shahaf
  1 sibling, 2 replies; 16+ messages in thread
From: Peter Grayson @ 2022-11-12 15:42 UTC (permalink / raw)
  To: zsh-workers; +Cc: d.s, Peter Grayson

The vcs_info patch detection code attempted to interrogate StGit patch
stack state by inspecting .git/patches/applied and
.git/patches/unapplied. As of StGit 1.0, StGit stack and patch state is
no longer maintained via files in the .git/ directory, but is instead
captured in the repo's object database, accessible via the
refs/stacks/<branch> reference.

Thus zsh's approach for interrogating StGit patch state is long
obsoleted.

This patch updates vcs_info to use StGit's prescribed interface for
interrogating applied and unapplied patch state: the `stg series` command.
This approach will work with effectively all versions of StGit, pre-1.0,
1.x, and 2.x.

The new test for StGit patches is moved to the end of the if/elif chain
that also tests for git rebase, merge, and cherrypick states. If any of
those operations are in progress, a StGit user will want to know about that
instead of the StGit stack state.

N.B. it may be possible for vcs_info to interrogate StGit stack state
in a faster manner by probing the filesystem and git object database
directly instead of executing `stg series`, but this would require vcs_info
to have knowledge of StGit internals across many versions.

Signed-off-by: Peter Grayson <pete@jpgrayson.net>
---
 .../VCS_Info/Backends/VCS_INFO_get_data_git    | 18 +++++++++---------
 1 file changed, 9 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 e45eebc8e..9edc2d140 100644
--- a/Functions/VCS_Info/Backends/VCS_INFO_get_data_git
+++ b/Functions/VCS_Info/Backends/VCS_INFO_get_data_git
@@ -184,15 +184,8 @@ fi
 VCS_INFO_adjust
 VCS_INFO_git_getaction ${gitdir}
 
-local patchdir=${gitdir}/patches/${gitbranch}
-if [[ -d $patchdir ]] && [[ -f $patchdir/applied ]] \
-   && [[ -f $patchdir/unapplied ]]
-then
-    # stgit
-    git_patches_applied=(${(f)"$(< "${patchdir}/applied")"})
-    git_patches_unapplied=(${(f)"$(< "${patchdir}/unapplied")"})
-    VCS_INFO_git_handle_patches
-elif [[ -d "${gitdir}/rebase-merge" ]]; then
+local patchdir
+if [[ -d "${gitdir}/rebase-merge" ]]; then
     # 'git rebase -i'
     patchdir="${gitdir}/rebase-merge"
     local p
@@ -389,6 +382,13 @@ elif [[ -f "${gitdir}/CHERRY_PICK_HEAD" ]]; then
         git_patches_unapplied=()
     fi
     VCS_INFO_git_handle_patches
+elif git_patches_applied=(${(f)"$(stg series --noprefix --applied 2>/dev/null)"}); then
+    # N.B. the "--noprefix" option is available in StGit 2.x as an alias for --no-prefix.
+    # The former is compatible with StGit versions going back to 2008.
+    if zstyle -t ":vcs_info:${vcs}:${usercontext}:${rrn}" get-unapplied; then
+        git_patches_unapplied=(${(f)"$(stg series --noprefix --unapplied 2>/dev/null)"})
+    fi
+    VCS_INFO_git_handle_patches
 else
     gitmisc=''
 fi
-- 
2.38.1



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] Remove StGit patch detection from vcs_info
  2022-11-12 14:46         ` Peter Grayson
  2022-11-12 15:42           ` [PATCH] Updated StGit patch detection in vcs_info Peter Grayson
@ 2022-11-13  4:30           ` Daniel Shahaf
  2022-11-14  3:38             ` Peter Grayson
  1 sibling, 1 reply; 16+ messages in thread
From: Daniel Shahaf @ 2022-11-13  4:30 UTC (permalink / raw)
  To: Peter Grayson; +Cc: zsh-workers

Peter Grayson wrote on Sat, Nov 12, 2022 at 09:46:02 -0500:
> On Fri, Nov 11, 2022, at 6:49 AM, Daniel Shahaf wrote:
> > [re #2]: It sounds like StGit 2.x support can be implemented at the cost
> > of one fork(2) for those who don't use StGit and under a microsecond for
> > those who do.  That doesn't sound like a deal breaker at all.
> 
> Running `stg series` with StGit 2.0 takes about 12ms in my environment.
> StGit 1.5 it is about 32ms. Not a microsecond, but perhaps acceptable
> nonetheless.
> 

To be clear, are these figures the duration of the «stg series
--noprefix --applied» invocation?

What's the impact on people who don't have stg(1) installed, or who have
stg(1) installed but are currently in a worktree that doesn't use StGit?
I.e., are those figures immediately after `git init`, or in a worktree
that has a StGit patch stack, or?

In general, 32ms for everyone might be too much (what if another
third-party tool wanted to do its own elif branch that also spent 32ms
for everyone?  That'd be 64ms in total and counting…).  However, if the
32ms would only be seen by users of an EOL'd stg(1), we can relax the
threshold a bit.  On the other hand, if it's 32ms just in cases where
stg(1) is used… well, there doesn't seem to be much we /can/ do there.
Perhaps hide some or all of the work behind an opt-in switch.  (For
instance, the default settings show only the topmost applied patch, so
if it's possible to tell stg(1) not to emit any other patches, that
might help the code finish more quickly.)

From the "Is your computer plugged in?" department: Is that 32ms figure
on hot disk cache with .pyc files already having been generated?

Cheers,

Daniel

> 
> > Cheers,
> >
> > Daniel
> >
> >> Signed-off-by: Peter Grayson <pete@jpgrayson.net>
> >> ---
> >>  Functions/VCS_Info/Backends/VCS_INFO_get_data_git | 11 ++---------
> >>  1 file changed, 2 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 e45eebc8e..a3f4dbdf0 100644
> >> --- a/Functions/VCS_Info/Backends/VCS_INFO_get_data_git
> >> +++ b/Functions/VCS_Info/Backends/VCS_INFO_get_data_git
> >> @@ -184,15 +184,8 @@ fi
> >>  VCS_INFO_adjust
> >>  VCS_INFO_git_getaction ${gitdir}
> >>  
> >> -local patchdir=${gitdir}/patches/${gitbranch}
> >> -if [[ -d $patchdir ]] && [[ -f $patchdir/applied ]] \
> >> -   && [[ -f $patchdir/unapplied ]]
> >> -then
> >> -    # stgit
> >> -    git_patches_applied=(${(f)"$(< "${patchdir}/applied")"})
> >> -    git_patches_unapplied=(${(f)"$(< "${patchdir}/unapplied")"})
> >> -    VCS_INFO_git_handle_patches
> >> -elif [[ -d "${gitdir}/rebase-merge" ]]; then
> >> +local patchdir
> >> +if [[ -d "${gitdir}/rebase-merge" ]]; then
> >>      # 'git rebase -i'
> >>      patchdir="${gitdir}/rebase-merge"
> >>      local p
> >> -- 
> >> 2.38.1
> >> 
> >>
> 


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] Updated StGit patch detection in vcs_info
  2022-11-12 15:42           ` [PATCH] Updated StGit patch detection in vcs_info Peter Grayson
@ 2022-11-13  4:58             ` Daniel Shahaf
  2022-11-16 20:45             ` [PATCH v2] " Peter Grayson
  1 sibling, 0 replies; 16+ messages in thread
From: Daniel Shahaf @ 2022-11-13  4:58 UTC (permalink / raw)
  To: Peter Grayson; +Cc: zsh-workers

Peter Grayson wrote on Sat, Nov 12, 2022 at 10:42:21 -0500:
> The vcs_info patch detection code attempted to interrogate StGit patch
> stack state by inspecting .git/patches/applied and
> .git/patches/unapplied. As of StGit 1.0, StGit stack and patch state is
> no longer maintained via files in the .git/ directory, but is instead
> captured in the repo's object database, accessible via the
> refs/stacks/<branch> reference.
> 
> Thus zsh's approach for interrogating StGit patch state is long
> obsoleted.
> 
> This patch updates vcs_info to use StGit's prescribed interface for
> interrogating applied and unapplied patch state: the `stg series` command.
> This approach will work with effectively all versions of StGit, pre-1.0,
> 1.x, and 2.x.
> 

Great!

> The new test for StGit patches is moved to the end of the if/elif chain
> that also tests for git rebase, merge, and cherrypick states. If any of
> those operations are in progress, a StGit user will want to know about that
> instead of the StGit stack state.
> 
> N.B. it may be possible for vcs_info to interrogate StGit stack state
> in a faster manner by probing the filesystem and git object database
> directly instead of executing `stg series`, but this would require vcs_info
> to have knowledge of StGit internals across many versions.
> 

Either or both of these paragraphs might work better as a source
comment, so as to ensure future editors of this file will be aware of
these considerations.

> Signed-off-by: Peter Grayson <pete@jpgrayson.net>
> ---
>  .../VCS_Info/Backends/VCS_INFO_get_data_git    | 18 +++++++++---------
>  1 file changed, 9 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 e45eebc8e..9edc2d140 100644
> --- a/Functions/VCS_Info/Backends/VCS_INFO_get_data_git
> +++ b/Functions/VCS_Info/Backends/VCS_INFO_get_data_git
> @@ -184,15 +184,8 @@ fi
>  VCS_INFO_adjust
>  VCS_INFO_git_getaction ${gitdir}
>  
> -local patchdir=${gitdir}/patches/${gitbranch}
> -if [[ -d $patchdir ]] && [[ -f $patchdir/applied ]] \
> -   && [[ -f $patchdir/unapplied ]]
> -then
> -    # stgit
> -    git_patches_applied=(${(f)"$(< "${patchdir}/applied")"})
> -    git_patches_unapplied=(${(f)"$(< "${patchdir}/unapplied")"})
> -    VCS_INFO_git_handle_patches
> -elif [[ -d "${gitdir}/rebase-merge" ]]; then
> +local patchdir
> +if [[ -d "${gitdir}/rebase-merge" ]]; then
>      # 'git rebase -i'
>      patchdir="${gitdir}/rebase-merge"
>      local p
> @@ -389,6 +382,13 @@ elif [[ -f "${gitdir}/CHERRY_PICK_HEAD" ]]; then
>          git_patches_unapplied=()
>      fi
>      VCS_INFO_git_handle_patches
> +elif git_patches_applied=(${(f)"$(stg series --noprefix --applied 2>/dev/null)"}); then
> +    # N.B. the "--noprefix" option is available in StGit 2.x as an alias for --no-prefix.
> +    # The former is compatible with StGit versions going back to 2008.
> +    if zstyle -t ":vcs_info:${vcs}:${usercontext}:${rrn}" get-unapplied; then
> +        git_patches_unapplied=(${(f)"$(stg series --noprefix --unapplied 2>/dev/null)"})
> +    fi
> +    VCS_INFO_git_handle_patches
>  else
>      gitmisc=''
>  fi

+1

If you wish, you can add the patches' log messages here, too (as printed
by «stg series --description»); a gen-applied-string hook can be used to
show those in the prompt.  There's an example in Misc/vcs_info-examples.
(But I'd be happy to merge the patch without this, too :))

Cheers,

Daniel


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] Remove StGit patch detection from vcs_info
  2022-11-13  4:30           ` [PATCH] Remove StGit patch detection from vcs_info Daniel Shahaf
@ 2022-11-14  3:38             ` Peter Grayson
  2022-11-14  5:17               ` Mikael Magnusson
  2022-11-14 13:15               ` Daniel Shahaf
  0 siblings, 2 replies; 16+ messages in thread
From: Peter Grayson @ 2022-11-14  3:38 UTC (permalink / raw)
  To: Daniel Shahaf; +Cc: zsh-workers

On Sat, Nov 12, 2022, at 11:30 PM, Daniel Shahaf wrote:
> Peter Grayson wrote on Sat, Nov 12, 2022 at 09:46:02 -0500:
>> On Fri, Nov 11, 2022, at 6:49 AM, Daniel Shahaf wrote:
>> > [re #2]: It sounds like StGit 2.x support can be implemented at the cost
>> > of one fork(2) for those who don't use StGit and under a microsecond for
>> > those who do.  That doesn't sound like a deal breaker at all.
>> 
>> Running `stg series` with StGit 2.0 takes about 12ms in my environment.
>> StGit 1.5 it is about 32ms. Not a microsecond, but perhaps acceptable
>> nonetheless.
>> 
>
> To be clear, are these figures the duration of the «stg series
> --noprefix --applied» invocation?

Yes.

> What's the impact on people who don't have stg(1) installed, or who have
> stg(1) installed but are currently in a worktree that doesn't use StGit?
> I.e., are those figures immediately after `git init`, or in a worktree
> that has a StGit patch stack, or?

Without stg(1) installed, the cost would be however long it takes zsh to
determine that the executable is not available in $path, which is
ostensibly very fast (microseconds?).

If stg(1) is installed, but run in a repo with a branch that has not been
initialized with `stg init`, it's still about 12ms. Almost all that time
is taken just to initialize a libgit2 Repository structure, which is
used to interrogate the object database to determine whether a StGit
stack is initialized.

> In general, 32ms for everyone might be too much (what if another
> third-party tool wanted to do its own elif branch that also spent 32ms
> for everyone?  That'd be 64ms in total and counting…).

Well, that was my original thought as well and why my first patch simply
removed StGit support from vcs_info. Note that 32ms is on a particularly
fast machine and I stand by my assertion that Python startup time can be
on the order of hundreds of milliseconds on more modest machines and/or
with older versions of Python.

> However, if the 32ms would only be seen by users of an EOL'd stg(1),
> we can relax the threshold a bit.  On the other hand, if it's 32ms
> just in cases where stg(1) is used… well, there doesn't seem to be
> much we /can/ do there.

The relevant scenarios:

* stg(1) not installed => ~free
* stg(1) < 2.0 installed => 30+ ms
* stg(1) >= 2.0 installed => ~10 ms

The worst case scenario would be for someone where stg(1) <2.0 is
installed, but not used. This would create overhead in vcs_info without
any value for the user. All other scenarios seem to end up with the
user getting what they paid for.

> Perhaps hide some or all of the work behind an opt-in switch.  (For
> instance, the default settings show only the topmost applied patch, so
> if it's possible to tell stg(1) not to emit any other patches, that
> might help the code finish more quickly.)

Limiting the output to only the topmost patch will not have a material
effect on the runtime because of how the overhead is dominated by
libgit2 initialization. So an opt-in switch would have be all or
nothing; either try to run stg(1) or not.

Also worth noting is that changing StGit support in vcs_info opt-in
would be a bit of a regression and would add more configuration
surface area. Not sure if/how the zsh project does versioning for
something like this. I did not include such a switch in my latest
patch because of these issues.

> From the "Is your computer plugged in?" department: Is that 32ms figure
> on hot disk cache with .pyc files already having been generated?

Yes. I used hyperfine(1) to do these measurements.

Pete


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] Remove StGit patch detection from vcs_info
  2022-11-14  3:38             ` Peter Grayson
@ 2022-11-14  5:17               ` Mikael Magnusson
  2022-11-14 13:21                 ` Daniel Shahaf
  2022-11-14 13:15               ` Daniel Shahaf
  1 sibling, 1 reply; 16+ messages in thread
From: Mikael Magnusson @ 2022-11-14  5:17 UTC (permalink / raw)
  To: Peter Grayson; +Cc: Daniel Shahaf, zsh-workers

On 11/14/22, Peter Grayson <pete@jpgrayson.net> wrote:
> On Sat, Nov 12, 2022, at 11:30 PM, Daniel Shahaf wrote:
>> Peter Grayson wrote on Sat, Nov 12, 2022 at 09:46:02 -0500:
>>> On Fri, Nov 11, 2022, at 6:49 AM, Daniel Shahaf wrote:
>>> > [re #2]: It sounds like StGit 2.x support can be implemented at the
>>> > cost
>>> > of one fork(2) for those who don't use StGit and under a microsecond
>>> > for
>>> > those who do.  That doesn't sound like a deal breaker at all.
>>>
>>> Running `stg series` with StGit 2.0 takes about 12ms in my environment.
>>> StGit 1.5 it is about 32ms. Not a microsecond, but perhaps acceptable
>>> nonetheless.
>>>
>>
>> To be clear, are these figures the duration of the «stg series
>> --noprefix --applied» invocation?
>
> Yes.
>
>> What's the impact on people who don't have stg(1) installed, or who have
>> stg(1) installed but are currently in a worktree that doesn't use StGit?
>> I.e., are those figures immediately after `git init`, or in a worktree
>> that has a StGit patch stack, or?
>
> Without stg(1) installed, the cost would be however long it takes zsh to
> determine that the executable is not available in $path, which is
> ostensibly very fast (microseconds?).
>
> If stg(1) is installed, but run in a repo with a branch that has not been
> initialized with `stg init`, it's still about 12ms. Almost all that time
> is taken just to initialize a libgit2 Repository structure, which is
> used to interrogate the object database to determine whether a StGit
> stack is initialized.

fwiw, vcs_info lets you configure which vcs systems it looks for,
whether or not you have them installed. eg, I use:
  zstyle ':vcs_info:*:*' enable git hg svn
although these days i could surely set that to just git...

-- 
Mikael Magnusson


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] Remove StGit patch detection from vcs_info
  2022-11-14  3:38             ` Peter Grayson
  2022-11-14  5:17               ` Mikael Magnusson
@ 2022-11-14 13:15               ` Daniel Shahaf
  2022-11-16 19:38                 ` Peter Grayson
  1 sibling, 1 reply; 16+ messages in thread
From: Daniel Shahaf @ 2022-11-14 13:15 UTC (permalink / raw)
  To: Peter Grayson; +Cc: zsh-workers

Peter Grayson wrote on Sun, Nov 13, 2022 at 22:38:50 -0500:
> On Sat, Nov 12, 2022, at 11:30 PM, Daniel Shahaf wrote:
> > What's the impact on people who don't have stg(1) installed, or who have
> > stg(1) installed but are currently in a worktree that doesn't use StGit?
> > I.e., are those figures immediately after `git init`, or in a worktree
> > that has a StGit patch stack, or?
> 
> Without stg(1) installed, the cost would be however long it takes zsh to
> determine that the executable is not available in $path, which is
> ostensibly very fast (microseconds?).
> 
> If stg(1) is installed, but run in a repo with a branch that has not been
> initialized with `stg init`, it's still about 12ms. Almost all that time
> is taken just to initialize a libgit2 Repository structure, which is
> used to interrogate the object database to determine whether a StGit
> stack is initialized.
> 

I see.

> > In general, 32ms for everyone might be too much (what if another
> > third-party tool wanted to do its own elif branch that also spent 32ms
> > for everyone?  That'd be 64ms in total and counting…).
> 
> Well, that was my original thought as well and why my first patch simply
> removed StGit support from vcs_info. Note that 32ms is on a particularly
> fast machine and I stand by my assertion that Python startup time can be
> on the order of hundreds of milliseconds on more modest machines and/or
> with older versions of Python.
> 

*nod*

> > However, if the 32ms would only be seen by users of an EOL'd stg(1),
> > we can relax the threshold a bit.  On the other hand, if it's 32ms
> > just in cases where stg(1) is used… well, there doesn't seem to be
> > much we /can/ do there.
> 
> The relevant scenarios:
> 
> * stg(1) not installed => ~free
> * stg(1) < 2.0 installed => 30+ ms
> * stg(1) >= 2.0 installed => ~10 ms
> 
> The worst case scenario would be for someone where stg(1) <2.0 is
> installed, but not used. This would create overhead in vcs_info without
> any value for the user.

Yeah, the majority of git users don't use stg, so let's try not to make
them wait an extra 30ms for every prompt.

Fortunately, that delay only happens with EOL versions of stg(1).  That
gives us some leeway in our design choices.  Thinking out loud:

How likely is it for a vcs_info git user to have stg(1) 0.x or 1.x
installed and /not/ use it?

Unfortunately, the prior probability of having 0.x installed isn't
exactly low, since 0.x is packaged in Debian and derivatives and they
don't seem likely to upgrade soon (see the inactivity on
<https://bugs.debian.org/995869>).  That's aside from how quick distros
may or may not be on upgradomg from 1.x (so far only Homebrew and Arch's
AUR have it, according to repology).

Next, in case 0.x or 1.x is installed, for it to be unused seems to mean
the OS installation is either shared (more than one human user) or
has a single user who uses stg(1) only in some of their repositories.

In the former case, the box is likely relatively performant… but so is
the box you took the measurements on.  In the latter case, opt-in zstyle
settings with repo-root-name in the context patterns could be used to
selectively enable/disable stg(1) use… but that's hardly ideal.

All of this only affects people with an EOL version of stg(1) installed,
but since it affects people who /don't/ use stg(1), we can't really say
"Patches welcome" to the affected people, can we.

It seems the better course of action here is to get distros to upgrade
to 2.x, so the problem situation doesn't arise in the first place.

Until that happens, how about making VCS_INFO_get_data_git integrate
only with stg ≥2.x by default, citing the EOL status and performance
concerns?  That is, guard the code being added by «if [[ stg version is
2.x or newer ]] || user has opted in».

For the first disjunct, normally we would prefer «stg --version», but
that likewise takes 30ms here (stg 0.x in a chroot), so I reckon we'll
want some other approach.  Any ideas?  I've thought of two options, but
they're both hacky and I'm sure there are much better ones.  (For the
record, they are to check whether «=stg» is a Python script and to check
whether it's smaller than 2KB.)

> All other scenarios seem to end up with the user getting what they
> paid for.

Is there any chance of optimizing the 10ms case as well?

For instance, consider a has-stg(1) executable (or shell function) that
returns 0 if the Git repository in cwd has an initialized StGit stack
and returns 1 otherwise.  Would it be possible for has-stg to run faster
than 10ms?  If so, vcs_info could use this (unconditionally if has-stg
were maintained and shipped as part of StGit; otherwise, conditional on
«use-simple» due to the layering violation).  WDYT?

> > Perhaps hide some or all of the work behind an opt-in switch.  (For
> > instance, the default settings show only the topmost applied patch, so
> > if it's possible to tell stg(1) not to emit any other patches, that
> > might help the code finish more quickly.)
> 
> Limiting the output to only the topmost patch will not have a material
> effect on the runtime because of how the overhead is dominated by
> libgit2 initialization. So an opt-in switch would have be all or
> nothing; either try to run stg(1) or not.
> 
> Also worth noting is that changing StGit support in vcs_info opt-in
> would be a bit of a regression and would add more configuration
> surface area. Not sure if/how the zsh project does versioning for
> something like this. I did not include such a switch in my latest
> patch because of these issues.
> 

When we make an incompatible change, we document it in the list of
incompatibilities in our NEWS/README files.  We can consider doing more
than that (e.g., printing a message at runtime informing the user of the
opt-in) as appropriate.

In this instance, since released zsh only supports StGit 0.x,
introducing an opt-in would be a regression only for users who upgrade
to zsh 5.10 not after upgrading stg to 1.x or 2.x — i.e., typically,
users of LTS distros as opposed to rolling distros.

That being the case, perhaps adding a paragraph to the list of
incompatibilities in NEWS/README would suffice?  Upgrading an LTS distro
does usually involve reading through a long list of incompatible changes
that affect some users only.

Alternatively, perhaps add a runtime message along the lines of:
.
    if (( ! OPTED_OUT_OF_THIS_MESSAGE )) &&
       type stg > /dev/null &&
       [[ -z ${(M)${(f)$(git for-each-ref)}:#*stgit*} ]]
    then
        # tell the user they may want to set the opt-in
    fi
.
, though I wonder now whether that last conjunct is fast enough…

> > From the "Is your computer plugged in?" department: Is that 32ms figure
> > on hot disk cache with .pyc files already having been generated?
> 
> Yes. I used hyperfine(1) to do these measurements.

*nod*

Cheers,

Daniel


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] Remove StGit patch detection from vcs_info
  2022-11-14  5:17               ` Mikael Magnusson
@ 2022-11-14 13:21                 ` Daniel Shahaf
  0 siblings, 0 replies; 16+ messages in thread
From: Daniel Shahaf @ 2022-11-14 13:21 UTC (permalink / raw)
  To: Mikael Magnusson; +Cc: Peter Grayson, zsh-workers

Mikael Magnusson wrote on Mon, Nov 14, 2022 at 06:17:02 +0100:
> On 11/14/22, Peter Grayson <pete@jpgrayson.net> wrote:
> > On Sat, Nov 12, 2022, at 11:30 PM, Daniel Shahaf wrote:
> >> Peter Grayson wrote on Sat, Nov 12, 2022 at 09:46:02 -0500:
> >>> On Fri, Nov 11, 2022, at 6:49 AM, Daniel Shahaf wrote:
> >>> > [re #2]: It sounds like StGit 2.x support can be implemented at the
> >>> > cost
> >>> > of one fork(2) for those who don't use StGit and under a microsecond
> >>> > for
> >>> > those who do.  That doesn't sound like a deal breaker at all.
> >>>
> >>> Running `stg series` with StGit 2.0 takes about 12ms in my environment.
> >>> StGit 1.5 it is about 32ms. Not a microsecond, but perhaps acceptable
> >>> nonetheless.
> >>>
> >>
> >> To be clear, are these figures the duration of the «stg series
> >> --noprefix --applied» invocation?
> >
> > Yes.
> >
> >> What's the impact on people who don't have stg(1) installed, or who have
> >> stg(1) installed but are currently in a worktree that doesn't use StGit?
> >> I.e., are those figures immediately after `git init`, or in a worktree
> >> that has a StGit patch stack, or?
> >
> > Without stg(1) installed, the cost would be however long it takes zsh to
> > determine that the executable is not available in $path, which is
> > ostensibly very fast (microseconds?).
> >
> > If stg(1) is installed, but run in a repo with a branch that has not been
> > initialized with `stg init`, it's still about 12ms. Almost all that time
> > is taken just to initialize a libgit2 Repository structure, which is
> > used to interrogate the object database to determine whether a StGit
> > stack is initialized.
> 
> fwiw, vcs_info lets you configure which vcs systems it looks for,
> whether or not you have them installed. eg, I use:
>   zstyle ':vcs_info:*:*' enable git hg svn

Right, but this just determines whether VCS_INFO_get_data_(git|hg|svn)
may ever run at all.  The situation we're talking about is that someone
has «git» in the «enable» style but doesn't use StGit, so would want
VCS_INFO_get_data_git to (1) run, but (2) not look for StGit patch
stacks.  There's no way to selectively disable just that one «elif» in
the middle of VCS_INFO_get_data_git.

> although these days i could surely set that to just git...


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] Remove StGit patch detection from vcs_info
  2022-11-14 13:15               ` Daniel Shahaf
@ 2022-11-16 19:38                 ` Peter Grayson
  0 siblings, 0 replies; 16+ messages in thread
From: Peter Grayson @ 2022-11-16 19:38 UTC (permalink / raw)
  To: Daniel Shahaf; +Cc: zsh-workers

On Mon, Nov 14, 2022, at 8:15 AM, Daniel Shahaf wrote:
> Peter Grayson wrote on Sun, Nov 13, 2022 at 22:38:50 -0500:
>> On Sat, Nov 12, 2022, at 11:30 PM, Daniel Shahaf wrote:
>> > What's the impact on people who don't have stg(1) installed, or who have
>> > stg(1) installed but are currently in a worktree that doesn't use StGit?
>> > I.e., are those figures immediately after `git init`, or in a worktree
>> > that has a StGit patch stack, or?
>> 
>> Without stg(1) installed, the cost would be however long it takes zsh to
>> determine that the executable is not available in $path, which is
>> ostensibly very fast (microseconds?).
>> 
>> If stg(1) is installed, but run in a repo with a branch that has not been
>> initialized with `stg init`, it's still about 12ms. Almost all that time
>> is taken just to initialize a libgit2 Repository structure, which is
>> used to interrogate the object database to determine whether a StGit
>> stack is initialized.
>> 
>
> I see.
>
>> > In general, 32ms for everyone might be too much (what if another
>> > third-party tool wanted to do its own elif branch that also spent 32ms
>> > for everyone?  That'd be 64ms in total and counting…).
>> 
>> Well, that was my original thought as well and why my first patch simply
>> removed StGit support from vcs_info. Note that 32ms is on a particularly
>> fast machine and I stand by my assertion that Python startup time can be
>> on the order of hundreds of milliseconds on more modest machines and/or
>> with older versions of Python.
>> 
>
> *nod*
>
>> > However, if the 32ms would only be seen by users of an EOL'd stg(1),
>> > we can relax the threshold a bit.  On the other hand, if it's 32ms
>> > just in cases where stg(1) is used… well, there doesn't seem to be
>> > much we /can/ do there.
>> 
>> The relevant scenarios:
>> 
>> * stg(1) not installed => ~free
>> * stg(1) < 2.0 installed => 30+ ms
>> * stg(1) >= 2.0 installed => ~10 ms
>> 
>> The worst case scenario would be for someone where stg(1) <2.0 is
>> installed, but not used. This would create overhead in vcs_info without
>> any value for the user.
>
> Yeah, the majority of git users don't use stg, so let's try not to make
> them wait an extra 30ms for every prompt.
>
> Fortunately, that delay only happens with EOL versions of stg(1).  That
> gives us some leeway in our design choices.  Thinking out loud:
>
> How likely is it for a vcs_info git user to have stg(1) 0.x or 1.x
> installed and /not/ use it?
>
> Unfortunately, the prior probability of having 0.x installed isn't
> exactly low, since 0.x is packaged in Debian and derivatives and they
> don't seem likely to upgrade soon (see the inactivity on
> <https://bugs.debian.org/995869>).  That's aside from how quick distros
> may or may not be on upgradomg from 1.x (so far only Homebrew and Arch's
> AUR have it, according to repology).
>
> Next, in case 0.x or 1.x is installed, for it to be unused seems to mean
> the OS installation is either shared (more than one human user) or
> has a single user who uses stg(1) only in some of their repositories.
>
> In the former case, the box is likely relatively performant… but so is
> the box you took the measurements on.  In the latter case, opt-in zstyle
> settings with repo-root-name in the context patterns could be used to
> selectively enable/disable stg(1) use… but that's hardly ideal.
>
> All of this only affects people with an EOL version of stg(1) installed,
> but since it affects people who /don't/ use stg(1), we can't really say
> "Patches welcome" to the affected people, can we.
>
> It seems the better course of action here is to get distros to upgrade
> to 2.x, so the problem situation doesn't arise in the first place.
>
> Until that happens, how about making VCS_INFO_get_data_git integrate
> only with stg ≥2.x by default, citing the EOL status and performance
> concerns?  That is, guard the code being added by «if [[ stg version is
> 2.x or newer ]] || user has opted in».
>
> For the first disjunct, normally we would prefer «stg --version», but
> that likewise takes 30ms here (stg 0.x in a chroot), so I reckon we'll
> want some other approach.  Any ideas?  I've thought of two options, but
> they're both hacky and I'm sure there are much better ones.  (For the
> record, they are to check whether «=stg» is a Python script and to check
> whether it's smaller than 2KB.)

Another option would be to have zsh invoke `git rev-parse
refs/stacks/$gitbranch` to test whether a StGit stack is initialized on
the current branch. This takes less than half a millisecond. We could
pair this with testing for the existence of the stg executable to make
it even faster. The downside of this approach is that it leaks StGit
internals into zsh, but this is a pretty small leak and zsh would still
use `stg series` to actually get the patch lists.

This seems like a good way forward to me.

>> All other scenarios seem to end up with the user getting what they
>> paid for.
>
> Is there any chance of optimizing the 10ms case as well?
>
> For instance, consider a has-stg(1) executable (or shell function) that
> returns 0 if the Git repository in cwd has an initialized StGit stack
> and returns 1 otherwise.  Would it be possible for has-stg to run faster
> than 10ms?  If so, vcs_info could use this (unconditionally if has-stg
> were maintained and shipped as part of StGit; otherwise, conditional on
> «use-simple» due to the layering violation).  WDYT?

I've been thinking about this. For the simplest cases of `stg series`,
the stack metadata needed to service the command could be obtained using
`git cat-file -p refs/stacks/$gitbranch:stack.json` which takes about
0.5ms versus the libgit2 initialization of 11ms. So an optimized `stg
series --noprefix --applied` would be closer to 1ms. This same approach
could be applied to several stg subcommands, so it wouldn't necessarily
be a one-off optimization.

Further down the road, I have hopes of the gitoxide[1] project maturing
enough to replace libgit2 in StGit.

[1] https://github.com/Byron/gitoxide

>> > Perhaps hide some or all of the work behind an opt-in switch.  (For
>> > instance, the default settings show only the topmost applied patch, so
>> > if it's possible to tell stg(1) not to emit any other patches, that
>> > might help the code finish more quickly.)
>> 
>> Limiting the output to only the topmost patch will not have a material
>> effect on the runtime because of how the overhead is dominated by
>> libgit2 initialization. So an opt-in switch would have be all or
>> nothing; either try to run stg(1) or not.
>> 
>> Also worth noting is that changing StGit support in vcs_info opt-in
>> would be a bit of a regression and would add more configuration
>> surface area. Not sure if/how the zsh project does versioning for
>> something like this. I did not include such a switch in my latest
>> patch because of these issues.
>> 
>
> When we make an incompatible change, we document it in the list of
> incompatibilities in our NEWS/README files.  We can consider doing more
> than that (e.g., printing a message at runtime informing the user of the
> opt-in) as appropriate.
>
> In this instance, since released zsh only supports StGit 0.x,
> introducing an opt-in would be a regression only for users who upgrade
> to zsh 5.10 not after upgrading stg to 1.x or 2.x — i.e., typically,
> users of LTS distros as opposed to rolling distros.
>
> That being the case, perhaps adding a paragraph to the list of
> incompatibilities in NEWS/README would suffice?  Upgrading an LTS distro
> does usually involve reading through a long list of incompatible changes
> that affect some users only.
>
> Alternatively, perhaps add a runtime message along the lines of:
> .
>     if (( ! OPTED_OUT_OF_THIS_MESSAGE )) &&
>        type stg > /dev/null &&
>        [[ -z ${(M)${(f)$(git for-each-ref)}:#*stgit*} ]]
>     then
>         # tell the user they may want to set the opt-in
>     fi
> .
> , though I wonder now whether that last conjunct is fast enough…

I'm going to take a shot at making the StGit stack detection cheap enough
that vcs_info can just merrily support StGit without configuration
switches, user-facing messages, or incompatibility notices.

Thanks again for your detailed consideration of this topic.

Pete


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH v2] Updated StGit patch detection in vcs_info
  2022-11-12 15:42           ` [PATCH] Updated StGit patch detection in vcs_info Peter Grayson
  2022-11-13  4:58             ` Daniel Shahaf
@ 2022-11-16 20:45             ` Peter Grayson
  1 sibling, 0 replies; 16+ messages in thread
From: Peter Grayson @ 2022-11-16 20:45 UTC (permalink / raw)
  To: zsh-workers; +Cc: d.s, Peter Grayson

The vcs_info patch detection code attempted to interrogate StGit patch
stack state by inspecting .git/patches/applied and
.git/patches/unapplied.

As of StGit 0.15 (2009), patch stack metadata is captured in the repo's
object database. And as of StGit 1.0 (2021), no stack or patch state is
maintained in any files in the .git/ directory.

Zsh's approach for interrogating StGit patch state is thus obsoleted.

This patch updates vcs_info to determine whether StGit is initialized on a
branch by looking at the appropriate git refs and uses StGit's prescribed
interface for interrogating applied and unapplied patch state via the `stg
series` command. This approach will work with all versions of StGit >=0.15.

Signed-off-by: Peter Grayson <pete@jpgrayson.net>
---
 .../VCS_Info/Backends/VCS_INFO_get_data_git   | 38 ++++++++++++++-----
 1 file changed, 29 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 e45eebc8e..e275abbb2 100644
--- a/Functions/VCS_Info/Backends/VCS_INFO_get_data_git
+++ b/Functions/VCS_Info/Backends/VCS_INFO_get_data_git
@@ -184,15 +184,8 @@ fi
 VCS_INFO_adjust
 VCS_INFO_git_getaction ${gitdir}
 
-local patchdir=${gitdir}/patches/${gitbranch}
-if [[ -d $patchdir ]] && [[ -f $patchdir/applied ]] \
-   && [[ -f $patchdir/unapplied ]]
-then
-    # stgit
-    git_patches_applied=(${(f)"$(< "${patchdir}/applied")"})
-    git_patches_unapplied=(${(f)"$(< "${patchdir}/unapplied")"})
-    VCS_INFO_git_handle_patches
-elif [[ -d "${gitdir}/rebase-merge" ]]; then
+local patchdir
+if [[ -d "${gitdir}/rebase-merge" ]]; then
     # 'git rebase -i'
     patchdir="${gitdir}/rebase-merge"
     local p
@@ -389,6 +382,33 @@ elif [[ -f "${gitdir}/CHERRY_PICK_HEAD" ]]; then
         git_patches_unapplied=()
     fi
     VCS_INFO_git_handle_patches
+elif command -v stg >/dev/null &&
+        (
+            ${vcs_comm[cmd]} rev-parse --quiet --verify refs/stacks/${gitbranch} >/dev/null ||
+            ${vcs_comm[cmd]} rev-parse --quiet --verify refs/heads/${gitbranch}.stgit >/dev/null \
+        ) && git_patches_applied=(${(f)"$(stg series --noprefix --applied 2>/dev/null)"})
+then
+    # Testing for StGit patches is done after testing for all git-proper
+    # patches/states. If a StGit user's repo is in one of those states, they
+    # will want to see that instead of the StGit patch info.
+
+    # Performance note: testing for the stg executable is done first because it
+    # is extremely cheap and there is nothing else to do if it isn't present.
+    # Using git to test whether a StGit stack is initialized is cheaper than
+    # running stg itself; especially for versions of StGit <= 2.0. Thus getting
+    # StGit patch info should only have a material runtime cost if StGit is
+    # installed and in-use for the current branch.
+
+    # In StGit >=1.2, the existence of refs/stacks/<branch> indicates StGit is
+    # initialized. In StGit >=0.15, it is refs/heads/<branch>.stgit.
+
+    # N.B. the "--noprefix" option is available in StGit 2.x as an alias for
+    # --no-prefix.  The former is compatible with StGit versions going back to
+    # 2008.
+    if zstyle -t ":vcs_info:${vcs}:${usercontext}:${rrn}" get-unapplied; then
+        git_patches_unapplied=(${(f)"$(stg series --noprefix --unapplied 2>/dev/null)"})
+    fi
+    VCS_INFO_git_handle_patches
 else
     gitmisc=''
 fi
-- 
2.38.1



^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2022-11-16 20:52 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-10-25 20:43 [PATCH] Remove _stgit completion script Peter Grayson
2022-10-31  9:19 ` Daniel Shahaf
2022-10-31 10:13   ` StGit 2.0 and vcs_info (was: Re: [PATCH] Remove _stgit completion script) Daniel Shahaf
2022-11-01  3:00   ` [PATCH] Remove _stgit completion script Peter Grayson
2022-11-01  3:04     ` [PATCH] Remove StGit patch detection from vcs_info Peter Grayson
2022-11-11 11:49       ` Daniel Shahaf
2022-11-12 14:46         ` Peter Grayson
2022-11-12 15:42           ` [PATCH] Updated StGit patch detection in vcs_info Peter Grayson
2022-11-13  4:58             ` Daniel Shahaf
2022-11-16 20:45             ` [PATCH v2] " Peter Grayson
2022-11-13  4:30           ` [PATCH] Remove StGit patch detection from vcs_info Daniel Shahaf
2022-11-14  3:38             ` Peter Grayson
2022-11-14  5:17               ` Mikael Magnusson
2022-11-14 13:21                 ` Daniel Shahaf
2022-11-14 13:15               ` Daniel Shahaf
2022-11-16 19:38                 ` Peter Grayson

Code repositories for project(s) associated with this public inbox

	https://git.vuxu.org/mirror/zsh/

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).