Re: [RFC PATCH 0/4] git-gui: support SHA-256 repositories
On Mon, Oct 11 2021, Carlo Marcelo Arenas Belón wrote:
Show 28 quoted lines
> While poking a SHA-256 hash repository, was surprised to find gitk
> would fail with a fatal error when called, hence this series.
>
> Sending as an RFC, since I am not a git-gui or gitk user, and so
> while this fixes the original issue and allows me to call gitk to
> see the branch merge history (which is usually as much as I do with
> it), it is likey missing some changes, as most of them where found
> by lightly poking at all of the gui menus (except for remote or tool)
>
> It could also be reordered to reduce unnecessary churn and of course
> also needs the gitk change[1] that was sent independently, and better
> commit messages.
>
> [1] https://lore.kernel.org/git/20211011114723.204-1-carenas@gmail.com/
>
> Carlo Marcelo Arenas Belón (4):
> blame: prefer null_sha1 over nullid and retire later
> rename all *_sha1 variables and make null_oid hash aware
> expand regexp matching an oid to be hash agnostic
> track oid_size to allow for checks that are hash agnostic
>
> git-gui.sh | 30 ++++++++++++++++--------------
> lib/blame.tcl | 18 +++++++++---------
> lib/checkout_op.tcl | 4 ++--
> lib/choose_repository.tcl | 2 +-
> lib/commit.tcl | 3 ++-
> lib/remote_branch_delete.tcl | 2 +-
> 6 files changed, 31 insertions(+), 28 deletions(-)
There was a similar series earlier this year which didn't make it that fixes some of the same issues: https://lore.kernel.org/git/pull.979.git.1623687519832.gitgitgadget@gmail.com/
My comment on this one is much the same as that: I don't use this software, and if you've tested this I trust that it's better & this going in as-is would be better than the status quo.
But also that as noted in the feedback there it seems that:
1. Figuring out if we're using SHA-1 or SHA-256
2. Adjusting all regexes to *exactly* math those things, i.e. using
things like x{40}(?:x{24})Just seems like a lot of needless work as opposed to just matching x{40,64} or whatever. Yes that's not the same regex semantically, but I think the current code is just being overly strict, i.e. it's parsing some plumbing output, we can trust that the thing that looks like the OID in that position is the OID.
If anything I'd think we could just match [0-9a-f]{4,} in most/all of these cases, would make things like this easier to read:
- if {[regexp {^([a-z0-9]{40}) (\d+) (\d+) (\d+)$} $line line \ + if {[regexp {^([a-z0-9]{40}(?:[0-9a-f]{24})?) (\d+) (\d+) (\d+)$} $line line \
And also the pre-existing [a-z0-9]{40} is a very weird mixture of being overly permissive and overly strict :)
Carlo Marcelo Arenas Belón
