Zsh Mailing List Archive
Messages sorted by: Reverse Date, Date, Thread, Author

Re: [PATCH] Remove StGit patch detection from vcs_info



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@xxxxxxxxxxxxx>
> ---
>  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
> 
> 





Messages sorted by: Reverse Date, Date, Thread, Author