On Mon, 14 Jul 2025 18:28:13 +0200,
Johannes Sixt wrote:
>
> Am 03.07.25 um 14:04 schrieb Takashi Iwai:
> > This patch adds the basic support of SHA256 Git repositories.
> > The needed changes were mostly about adjusting the fixed ID length of
> > SHA1 (40) to be variable depending on the repo type.
>
> Thank you. Being precise in the commit message would be very
> appreciated. You say "mostly", which makes me wonder what the cases are
> that fall not under "mostly". How about:
>
> Determine the hash length on startup, then replace the hard-coded
> "40" by the variable value. Also fix <foo> to do <bar> so as to
> account for <baz>.
>
> Or make a bullet list if there is more to enumerate. Or make a
> multi-patch series where each patch has its own topic if this is warranted.
Thanks for the review!
Sure, will add more descriptions in the next respin.
> BTW, there is a case
>
> if {[regexp {^[0-9a-f]{1,39}$} $head]}
>
> around line 3217 in git-gui.sh.
Obviously I didn't look for numbers less than 40 :)
I'll replace it, too.
But I don't understand why it matches up to only 39, not 40 in the
code above.
It seems trying to get the proper hash id if it's no full length id?
If so, the check should be rather like
if {![regexp {^[0-9a-f]{40}$} $head]}
?? It makes the conversion a bit simpler.
> > @@ -436,7 +437,7 @@ method _load {jump} {
> > $i conf -state normal
> > $i delete 0.0 end
> > foreach g [$i tag names] {
> > - if {[regexp {^g[0-9a-f]{40}$} $g]} {
> > + if {[regexp [string map "@@ $hashlength" {^g[0-9a-f]{@@}$}] $g]} {
>
> Github copilot insist that using 'string map' to replace parts of a
> regular expression is idiomatic. However, I could not find a single
> reference that it cited. Tsk, tsk, AI, what were you smoking today?
>
> The alternatives that I tried could come up with were not any better, so
> this is good.
To be honest, my knowledge of Tcl/Tk is decades old (and only casually
revisiting right now), so let me know if there is a better
expression.
> > @@ -648,7 +652,7 @@ method _read_blame {fd cur_w cur_d} {
> > set oln $r_orig_line
> > set cmit $r_commit
> >
> > - if {[regexp {^0{40}$} $cmit]} {
> > + if {[regexp [string map "@@ $hashlength" {^0{@@}$}] $cmit]} {
>
> This is a roundabout way to say 'if {$cmit eq $nullid}'.
OK, noted.
While we're at it, I found that $null_sha1 is identical with $nullid.
I'll prepare a cleanup patch as preliminary.
> > @@ -879,7 +881,7 @@ method _do_clone_full_end {ok} {
> > if {[file exists [gitdir FETCH_HEAD]]} {
> > set fd [open [gitdir FETCH_HEAD] r]
> > while {[gets $fd line] >= 0} {
> > - if {[regexp "^(.{40})\t\t" $line line HEAD]} {
> > + if {[regexp [string map "@@ $hashlength" "^(.{@@})\t\t"] $line line HEAD]} {
> > break
> > }
> > }
>
> The repository picker dialog runs before $hashlength is set. Therefore,
> at the time that this function is executed, $hashlength is not available.
>
> This procedure can depend on the file format, which is to have \t\t
> after the hash regardless of its length.
Oh that's bad. I'll rewrite without the reference to $hashlength.
I guess we can simply replace the above with a range check {40,64}.
> > @@ -965,6 +967,8 @@ method _do_clone_checkout {HEAD} {
> > }
> >
> > method _readtree_wait {fd} {
> > + global hashlength
> > +
> > set buf [read $fd]
> > $o_status_op update_meter $buf
> > append readtree_err $buf
> > @@ -986,7 +990,7 @@ method _readtree_wait {fd} {
> >
> > # -- Run the post-checkout hook.
> > #
> > - set fd_ph [githook_read post-checkout [string repeat 0 40] \
> > + set fd_ph [githook_read post-checkout [string repeat 0 $hashlength] \
>
> Yet another case where $nullid can be used.
But it's also in repo picker code, so we don't have $nullid yet?
I'll rewrite somehow without $hashlength reference here, too.
(e.g. use the length of "git-rev-parse HEAD" output that is called
below)
> > [git rev-parse HEAD] 1]
thanks,
Takashi
