Re: [PATCH] Remove StGit patch detection from vcs_info

["Peter Grayson" <pete@xxxxxxxxxxxxx>]

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