Re: [PATCH 1/2] gitk: Add a basic support of SHA256 repositories into Gitk

From: Takashi Iwai <tiwai@suse.de>
Date: May 12, 2025, 14:29 UTC
Message-ID: <87v7q5ucll.wl-tiwai@suse.de>
In-Reply-To: <9bb1699a-ec99-40b3-bd6a-00e809d77d0d@kdbg.org>

On Thu, 08 May 2025 08:20:40 +0200, Johannes Sixt wrote:

Show 42 quoted lines

> 
> Am 20.03.25 um 16:41 schrieb Takashi Iwai:
> > From: Rostislav Krasny <rosti.bsd@gmail.com>
> > 
> > This PR makes Gitk working on both SHA256 and SHA1 repositories without
> > errors/crashes. I made it by changing and testing the gitk script of Git
> > for Windows [https://gitforwindows.org/] version 2.32.0.windows.1 that
> > is a little bit different than the mainstream 2.32.0 version.
> > 
> > Still not fixed functionality: [1] There is the "Auto-select SHA1
> > (length)" configuration preference that affects "Copy commit reference"
> > on both SHA1 and SHA256 repositories.
> > 
> > A new "Auto-select SHA256 (length)" configuration preference should be
> > added and used on SHA256 repositories instead of the old one. Since I'm
> > not familiar with Tcl/Tk and this issue isn't critical I didn't
> > implement it.
> > 
> > [ Changes from the original patch:
> >   * Discard the changes for generic words (e.g. "Commit ID"), so that
> >     translations can be still applied after this patch
> >   * Simplify the regexp check in gotocommit as suggested in the
> >     previous review
> >   -- tiwai ]
> 
> The message should be updated to not mention the evolution of the change
> and what is not relevant anymore or not relevant in this patch.
> 
> > 
> > Signed-off-by: Rostislav Krasny <rosti.bsd@gmail.com>
> > Link: https://patchwork.kernel.org/project/git/patch/pull.979.git.1623687519832.gitgitgadget@gmail.com
> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > ---
> 
> > @@ -8920,11 +8932,11 @@ proc gotocommit {} {
> >          set id $headids($sha1string)
> >      } else {
> >          set id [string tolower $sha1string]
> > -        if {[regexp {^[0-9a-f]{4,39}$} $id]} {
> > +	if {[regexp {^[0-9a-f]{4,63}$} $id]} {
> 
> This doesn't use $hashlength. Should it?

Not needed. It's a range match, and can work in a shorter string, too. And, that's what suggested in previous reviews (years ago!).

> Also watch out space vs. TAB.

OK.

Show 13 quoted lines

> > @@ -12524,6 +12539,18 @@ if {$tclencoding == {}} {
> >      puts stderr "Warning: encoding $gitencoding is not supported by Tcl/Tk"
> >  }
> >  
> > +set objformat [exec git rev-parse --show-object-format]
> > +if {$objformat eq "sha1"} {
> > +    set hashlength 40
> > +} elseif {$objformat eq "sha256"} {
> > +    set hashlength 64
> > +} else {
> > +    error_popup "[mc "Not supported hash algorithm:"] {$objformat}"
> 
> This looks strange. Where is the $objformat substituted?

Sorry, I don't understand your question here. Isn't it what you see in your quoted line...?

Show 7 quoted lines

> > +    exit 1
> > +}
> > +set hashalgorithm [string toupper $objformat]
> > +unset objformat
> 
> Why not set hashalgorithm right away, without using a temporary
> objformat? Why set it at all here? It's unused.

The objformat is what git-rev-parse gives, and it's also referred to show in the error message. You shouldn't convert it to the upper letters blindly there.

The hashalgorithm is with upper letters for SHA1 and SHA256 to be shown correctly in other places. It could be "SHA-1" or other strings, but it's done in that way because of simpleness.

thanks,

Takashi