Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gitk: Add a basic support of SHA256 repositories into Gitk #979

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rosti-il
Copy link

@rosti-il rosti-il commented Jun 12, 2021

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

cc: Ævar Arnfjörð Bjarmason avarab@gmail.com
cc: Felipe Contreras felipe.contreras@gmail.com
cc: Rostislav Krasny rosti.bsd@gmail.com

@gitgitgadget
Copy link

gitgitgadget bot commented Jun 12, 2021

Welcome to GitGitGadget

Hi @rosti-il, and welcome to GitGitGadget, the GitHub App to send patch series to the Git mailing list from GitHub Pull Requests.

Please make sure that your Pull Request has a good description, as it will be used as cover letter.

Also, it is a good idea to review the commit messages one last time, as the Git project expects them in a quite specific form:

  • the lines should not exceed 76 columns,
  • the first line should be like a header and typically start with a prefix like "tests:" or "revisions:" to state which subsystem the change is about, and
  • the commit messages' body should be describing the "why?" of the change.
  • Finally, the commit messages should end in a Signed-off-by: line matching the commits' author.

It is in general a good idea to await the automated test ("Checks") in this Pull Request before contributing the patches, e.g. to avoid trivial issues such as unportable code.

Contributing the patches

Before you can contribute the patches, your GitHub username needs to be added to the list of permitted users. Any already-permitted user can do that, by adding a comment to your PR of the form /allow. A good way to find other contributors is to locate recent pull requests where someone has been /allowed:

Both the person who commented /allow and the PR author are able to /allow you.

An alternative is the channel #git-devel on the Libera Chat IRC network:

<newcontributor> I've just created my first PR, could someone please /allow me? https://github.com/gitgitgadget/git/pull/12345
<veteran> newcontributor: it is done
<newcontributor> thanks!

Once on the list of permitted usernames, you can contribute the patches to the Git mailing list by adding a PR comment /submit.

If you want to see what email(s) would be sent for a /submit request, add a PR comment /preview to have the email(s) sent to you. You must have a public GitHub email address for this.

After you submit, GitGitGadget will respond with another comment that contains the link to the cover letter mail in the Git mailing list archive. Please make sure to monitor the discussion in that thread and to address comments and suggestions (while the comments and suggestions will be mirrored into the PR by GitGitGadget, you will still want to reply via mail).

If you do not want to subscribe to the Git mailing list just to be able to respond to a mail, you can download the mbox from the Git mailing list archive (click the (raw) link), then import it into your mail program. If you use GMail, you can do this via:

curl -g --user "<EMailAddress>:<Password>" \
    --url "imaps://imap.gmail.com/INBOX" -T /path/to/raw.txt

To iterate on your change, i.e. send a revised patch or patch series, you will first want to (force-)push to the same branch. You probably also want to modify your Pull Request description (or title). It is a good idea to summarize the revision by adding something like this to the cover letter (read: by editing the first comment on the PR, i.e. the PR description):

Changes since v1:
- Fixed a typo in the commit message (found by ...)
- Added a code comment to ... as suggested by ...
...

To send a new iteration, just add another PR comment with the contents: /submit.

Need help?

New contributors who want advice are encouraged to join git-mentoring@googlegroups.com, where volunteers who regularly contribute to Git are willing to answer newbie questions, give advice, or otherwise provide mentoring to interested contributors. You must join in order to post or view messages, but anyone can join.

You may also be able to find help in real time in the developer IRC channel, #git-devel on Libera Chat. Remember that IRC does not support offline messaging, so if you send someone a private message and log out, they cannot respond to you. The scrollback of #git-devel is archived, though.

@gitgitgadget
Copy link

gitgitgadget bot commented Jun 12, 2021

Invalid author email in 4cc3f1f: "45571812+rosti-il@users.noreply.github.com"

@gitgitgadget
Copy link

gitgitgadget bot commented Jun 12, 2021

Invalid author email in 96f17f5: "45571812+rosti-il@users.noreply.github.com"

@rosti-il
Copy link
Author

/preview

@gitgitgadget
Copy link

gitgitgadget bot commented Jun 12, 2021

Error: User rosti-il is not permitted to use GitGitGadget

@rosti-il
Copy link
Author

/submit

@gitgitgadget
Copy link

gitgitgadget bot commented Jun 12, 2021

Error: User rosti-il is not permitted to use GitGitGadget

@gitgitgadget
Copy link

gitgitgadget bot commented Jun 12, 2021

Invalid author email in 570cd9c: "45571812+rosti-il@users.noreply.github.com"

@rosti-il
Copy link
Author

Force pushed from 96f17f5 to 570cd9c with more fixes of SHA256 support in Gitk.
The "Auto-select SHA256 (length)" configuration preference still not implemented.

@rosti-il
Copy link
Author

/preview

@gitgitgadget
Copy link

gitgitgadget bot commented Jun 12, 2021

Error: User rosti-il is not permitted to use GitGitGadget

@rosti-il
Copy link
Author

I know I don't have permission to use /preview of GitGitGadget yet, but as a side effect it fixes following GutHub issue:

GitGitGadget PR Handler Failing after 54s — Build #94476 failed

@rosti-il
Copy link
Author

@dscho please /allow me.

@dscho
Copy link
Member

dscho commented Jun 12, 2021

I am on vacation, but okay.

@dscho
Copy link
Member

dscho commented Jun 12, 2021

/allow

@gitgitgadget
Copy link

gitgitgadget bot commented Jun 12, 2021

User rosti-il is now allowed to use GitGitGadget.

WARNING: rosti-il has no public email address set on GitHub

@rosti-il
Copy link
Author

/submit

@gitgitgadget
Copy link

gitgitgadget bot commented Jun 12, 2021

Invalid author email in 570cd9c: "45571812+rosti-il@users.noreply.github.com"

@dscho
Copy link
Member

dscho commented Jun 14, 2021

@bk2204 since you are driving a lot of the SHA-256 effort forward, would you maybe interested in taking this patch under your custody? It would appear that @rosti-il is unwilling to disclose their email address (which would be required by the Git project to accept the patch).

Signed-off-by: Rostislav Krasny <rosti.bsd@gmail.com>
@rosti-il
Copy link
Author

/submit

@gitgitgadget
Copy link

gitgitgadget bot commented Jun 14, 2021

Submitted as pull.979.git.1623687519832.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git pr-979/rosti-il/gitk-sha256-v1

To fetch this version to local tag pr-979/rosti-il/gitk-sha256-v1:

git fetch --no-tags https://github.com/gitgitgadget/git tag pr-979/rosti-il/gitk-sha256-v1

WARNING: rosti-il has no public email address set on GitHub

@bk2204
Copy link

bk2204 commented Jun 14, 2021

I unfortunately can't do that because while I'm familiar with many languages and can bumble my way through many others, I've never worked with Tcl and can't respond to any feedback on the patch since I don't know anything about the language. Sorry.

@gitgitgadget
Copy link

gitgitgadget bot commented Jun 16, 2021

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Rostislav Krasny via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Rostislav Krasny <rosti.bsd@gmail.com>
>
> Signed-off-by: Rostislav Krasny <rosti.bsd@gmail.com>
> ---
>     gitk: Add a basic support of SHA256 repositories into Gitk

Looping-in the gitk maintainer.

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

Thanks, Rostislav; please follow Documentation/SubmittingPatches
next time you touch gitk (or git-gui), as they have their own
repositories and maintainers.

>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-979%2Frosti-il%2Fgitk-sha256-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-979/rosti-il/gitk-sha256-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/979
>
>  gitk-git/gitk | 66 ++++++++++++++++++++++++++++++++++++---------------
>  1 file changed, 47 insertions(+), 19 deletions(-)
>
> diff --git a/gitk-git/gitk b/gitk-git/gitk
> index 23d9dd1fe0d0..2da53604cdc8 100755
> --- a/gitk-git/gitk
> +++ b/gitk-git/gitk
> @@ -290,6 +290,7 @@ proc parseviewargs {n arglist} {
>  
>  proc parseviewrevs {view revs} {
>      global vposids vnegids
> +    global hashlength
>  
>      if {$revs eq {}} {
>          set revs HEAD
> @@ -303,7 +304,7 @@ proc parseviewrevs {view revs} {
>          set badrev {}
>          for {set l 0} {$l < [llength $errlines]} {incr l} {
>              set line [lindex $errlines $l]
> -            if {!([string length $line] == 40 && [string is xdigit $line])} {
> +            if {!([string length $line] == $hashlength && [string is xdigit $line])} {
>                  if {[string match "fatal:*" $line]} {
>                      if {[string match "fatal: ambiguous argument*" $line]
>                          && $badrev ne {}} {
> @@ -507,6 +508,7 @@ proc updatecommits {} {
>      global hasworktree
>      global varcid vposids vnegids vflags vrevs
>      global show_notes
> +    global hashlength
>  
>      set hasworktree [hasworktree]
>      rereadrefs
> @@ -540,7 +542,7 @@ proc updatecommits {} {
>              # take out positive refs that we asked for before or
>              # that we have already seen
>              foreach rev $revs {
> -                if {[string length $rev] == 40} {
> +                if {[string length $rev] == $hashlength} {
>                      if {[lsearch -exact $oldpos $rev] < 0
>                          && ![info exists varcid($view,$rev)]} {
>                          lappend newrevs $rev
> @@ -1418,6 +1420,7 @@ proc getcommitlines {fd inst view updating}  {
>      global parents children curview hlview
>      global idpending ordertok
>      global varccommits varcid varctok vtokmod vfilelimit vshortids
> +    global hashlength
>  
>      set stuff [read $fd 500000]
>      # git log doesn't terminate the last commit with a null...
> @@ -1500,7 +1503,7 @@ proc getcommitlines {fd inst view updating}  {
>              }
>              set ok 1
>              foreach id $ids {
> -                if {[string length $id] != 40} {
> +                if {[string length $id] != $hashlength} {
>                      set ok 0
>                      break
>                  }
> @@ -1780,6 +1783,7 @@ proc readrefs {} {
>      global selecthead selectheadid
>      global hideremotes
>      global tclencoding
> +    global hashlength
>  
>      foreach v {tagids idtags headids idheads otherrefids idotherrefs} {
>          unset -nocomplain $v
> @@ -1789,9 +1793,9 @@ proc readrefs {} {
>          fconfigure $refd -encoding $tclencoding
>      }
>      while {[gets $refd line] >= 0} {
> -        if {[string index $line 40] ne " "} continue
> -        set id [string range $line 0 39]
> -        set ref [string range $line 41 end]
> +        if {[string index $line $hashlength] ne " "} continue
> +        set id [string range $line 0 [expr {$hashlength - 1}]]
> +        set ref [string range $line [expr {$hashlength + 1}] end]
>          if {![string match "refs/*" $ref]} continue
>          set name [string range $ref 5 end]
>          if {[string match "remotes/*" $name]} {
> @@ -2082,6 +2086,7 @@ proc makewindow {} {
>      global have_tk85 use_ttk NS
>      global git_version
>      global worddiff
> +    global hashlength hashalgorithm
>  
>      # The "mc" arguments here are purely so that xgettext
>      # sees the following string as needing to be translated
> @@ -2203,11 +2208,11 @@ proc makewindow {} {
>      set sha1entry .tf.bar.sha1
>      set entries $sha1entry
>      set sha1but .tf.bar.sha1label
> -    button $sha1but -text "[mc "SHA1 ID:"] " -state disabled -relief flat \
> +    button $sha1but -text "[mc "$hashalgorithm ID:"] " -state disabled -relief flat \
>          -command gotocommit -width 8
>      $sha1but conf -disabledforeground [$sha1but cget -foreground]
>      pack .tf.bar.sha1label -side left
> -    ${NS}::entry $sha1entry -width 40 -font textfont -textvariable sha1string
> +    ${NS}::entry $sha1entry -width $hashlength -font textfont -textvariable sha1string
>      trace add variable sha1string write sha1change
>      pack $sha1entry -side left -pady 2
>  
> @@ -3926,6 +3931,7 @@ proc stopblaming {} {
>  
>  proc read_line_source {fd inst} {
>      global blamestuff curview commfd blameinst nullid nullid2
> +    global hashlength
>  
>      while {[gets $fd line] >= 0} {
>          lappend blamestuff($inst) $line
> @@ -3946,7 +3952,7 @@ proc read_line_source {fd inst} {
>      set line [split [lindex $blamestuff($inst) 0] " "]
>      set id [lindex $line 0]
>      set lnum [lindex $line 1]
> -    if {[string length $id] == 40 && [string is xdigit $id] &&
> +    if {[string length $id] == $hashlength && [string is xdigit $id] &&
>          [string is digit -strict $lnum]} {
>          # look for "filename" line
>          foreach l $blamestuff($inst) {
> @@ -5269,13 +5275,14 @@ proc get_viewmainhead {view} {
>  # git rev-list should give us just 1 line to use as viewmainheadid($view)
>  proc getviewhead {fd inst view} {
>      global viewmainheadid commfd curview viewinstances showlocalchanges
> +    global hashlength
>  
>      set id {}
>      if {[gets $fd line] < 0} {
>          if {![eof $fd]} {
>              return 1
>          }
> -    } elseif {[string length $line] == 40 && [string is xdigit $line]} {
> +    } elseif {[string length $line] == $hashlength && [string is xdigit $line]} {
>          set id $line
>      }
>      set viewmainheadid($view) $id
> @@ -7039,10 +7046,11 @@ proc commit_descriptor {p} {
>  # Also look for URLs of the form "http[s]://..." and make them web links.
>  proc appendwithlinks {text tags} {
>      global ctext linknum curview
> +    global hashlength
>  
>      set start [$ctext index "end - 1c"]
>      $ctext insert end $text $tags
> -    set links [regexp -indices -all -inline {(?:\m|-g)[0-9a-f]{6,40}\M} $text]
> +    set links [regexp -indices -all -inline [string map "@@ $hashlength" {(?:\m|-g)[0-9a-f]{6,@@}\M}] $text]
>      foreach l $links {
>          set s [lindex $l 0]
>          set e [lindex $l 1]
> @@ -8716,13 +8724,17 @@ proc incrfont {inc} {
>  
>  proc clearsha1 {} {
>      global sha1entry sha1string
> -    if {[string length $sha1string] == 40} {
> +    global hashlength
> +
> +    if {[string length $sha1string] == $hashlength} {
>          $sha1entry delete 0 end
>      }
>  }
>  
>  proc sha1change {n1 n2 op} {
>      global sha1string currentid sha1but
> +    global hashalgorithm
> +
>      if {$sha1string == {}
>          || ([info exists currentid] && $sha1string == $currentid)} {
>          set state disabled
> @@ -8733,12 +8745,13 @@ proc sha1change {n1 n2 op} {
>      if {$state == "normal"} {
>          $sha1but conf -state normal -relief raised -text "[mc "Goto:"] "
>      } else {
> -        $sha1but conf -state disabled -relief flat -text "[mc "SHA1 ID:"] "
> +        $sha1but conf -state disabled -relief flat -text "[mc "$hashalgorithm ID:"] "
>      }
>  }
>  
>  proc gotocommit {} {
>      global sha1string tagids headids curview varcid
> +    global hashlength hashalgorithm
>  
>      if {$sha1string == {}
>          || ([info exists currentid] && $sha1string == $currentid)} return
> @@ -8748,11 +8761,11 @@ proc gotocommit {} {
>          set id $headids($sha1string)
>      } else {
>          set id [string tolower $sha1string]
> -        if {[regexp {^[0-9a-f]{4,39}$} $id]} {
> +        if {[regexp [string map "@@ [expr $hashlength - 1]" {^[0-9a-f]{4,@@}$}] $id]} {
>              set matches [longid $id]
>              if {$matches ne {}} {
>                  if {[llength $matches] > 1} {
> -                    error_popup [mc "Short SHA1 id %s is ambiguous" $id]
> +                    error_popup [mc "Short $hashalgorithm id %s is ambiguous" $id]
>                      return
>                  }
>                  set id [lindex $matches 0]
> @@ -8769,7 +8782,7 @@ proc gotocommit {} {
>          return
>      }
>      if {[regexp {^[0-9a-fA-F]{4,}$} $sha1string]} {
> -        set msg [mc "SHA1 id %s is not known" $sha1string]
> +        set msg [mc "$hashalgorithm id %s is not known" $sha1string]
>      } else {
>          set msg [mc "Revision %s is not in the current view" $sha1string]
>      }
> @@ -9446,10 +9459,11 @@ proc mktaggo {} {
>  
>  proc copyreference {} {
>      global rowmenuid autosellen
> +    global hashlength
>  
>      set format "%h (\"%s\", %ad)"
>      set cmd [list git show -s --pretty=format:$format --date=short]
> -    if {$autosellen < 40} {
> +    if {$autosellen < $hashlength} {
>          lappend cmd --abbrev=$autosellen
>      }
>      set reference [eval exec $cmd $rowmenuid]
> @@ -9460,6 +9474,7 @@ proc copyreference {} {
>  
>  proc writecommit {} {
>      global rowmenuid wrcomtop commitinfo wrcomcmd NS
> +    global hashlength
>  
>      set top .writecommit
>      set wrcomtop $top
> @@ -9469,7 +9484,7 @@ proc writecommit {} {
>      ${NS}::label $top.title -text [mc "Write commit to file"]
>      grid $top.title - -pady 10
>      ${NS}::label $top.id -text [mc "ID:"]
> -    ${NS}::entry $top.sha1 -width 40
> +    ${NS}::entry $top.sha1 -width $hashlength
>      $top.sha1 insert 0 $rowmenuid
>      $top.sha1 conf -state readonly
>      grid $top.id $top.sha1 -sticky w
> @@ -9549,6 +9564,7 @@ proc mvbranch {} {
>  
>  proc branchdia {top valvar uivar} {
>      global NS commitinfo
> +    global hashlength
>      upvar $valvar val $uivar ui
>  
>      catch {destroy $top}
> @@ -9557,7 +9573,7 @@ proc branchdia {top valvar uivar} {
>      ${NS}::label $top.title -text $ui(title)
>      grid $top.title - -pady 10
>      ${NS}::label $top.id -text [mc "ID:"]
> -    ${NS}::entry $top.sha1 -width 40
> +    ${NS}::entry $top.sha1 -width $hashlength
>      $top.sha1 insert 0 $val(id)
>      $top.sha1 conf -state readonly
>      grid $top.id $top.sha1 -sticky w
> @@ -12320,6 +12336,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}"
> +    exit 1
> +}
> +set hashalgorithm [string toupper $objformat]
> +unset objformat
> +
>  set gui_encoding [encoding system]
>  catch {
>      set enc [exec git config --get gui.encoding]
>
> base-commit: ebf3c04b262aa27fbb97f8a0156c2347fecafafb

@gitgitgadget
Copy link

gitgitgadget bot commented Jun 16, 2021

On the Git mailing list, Ævar Arnfjörð Bjarmason wrote (reply to this):


On Wed, Jun 16 2021, Junio C Hamano wrote:

> "Rostislav Krasny via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> From: Rostislav Krasny <rosti.bsd@gmail.com>
>>
>> Signed-off-by: Rostislav Krasny <rosti.bsd@gmail.com>
>> ---
>>     gitk: Add a basic support of SHA256 repositories into Gitk
>
> Looping-in the gitk maintainer.
>
>>     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.
>
> Thanks, Rostislav; please follow Documentation/SubmittingPatches
> next time you touch gitk (or git-gui), as they have their own
> repositories and maintainers.

A comment on the patch at large: I realize that the author isn't
familiar with Tcl, and this is a minimal & immediate fix, so maybe we
should just take it.

But I wonder if this == 40 or == 64 shouldn't just be "accept either" in
this case, these all seem like cases where we disambiguate a hash from
some other name.

Doing so would be nicely forward-compatible in case gitk and others ever
need to deal with viewing a mixed set of hashes, or maybe I'm again
misrecalling the transition plan and that'll never happen (they'll
always be translated?).

Especially stuff like this (grabbing a bit to quote from the patch):

>> -        if {[regexp {^[0-9a-f]{4,39}$} $id]} {
>> +        if {[regexp [string map "@@ [expr $hashlength - 1]" {^[0-9a-f]{4,@@}$}] $id]} {

Would be simpler as just:

    if {[regexp {^[0-9a-f]{4,63}$} $id]} {

@gitgitgadget
Copy link

gitgitgadget bot commented Jun 16, 2021

User Ævar Arnfjörð Bjarmason <avarab@gmail.com> has been added to the cc: list.

@gitgitgadget
Copy link

gitgitgadget bot commented Jun 19, 2021

On the Git mailing list, Felipe Contreras wrote (reply to this):

Ævar Arnfjörð Bjarmason wrote:
> 
> On Wed, Jun 16 2021, Junio C Hamano wrote:
> 
> > "Rostislav Krasny via GitGitGadget" <gitgitgadget@gmail.com> writes:
> >
> >> From: Rostislav Krasny <rosti.bsd@gmail.com>
> >>
> >> Signed-off-by: Rostislav Krasny <rosti.bsd@gmail.com>
> >> ---
> >>     gitk: Add a basic support of SHA256 repositories into Gitk
> >
> > Looping-in the gitk maintainer.
> >
> >>     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.
> >
> > Thanks, Rostislav; please follow Documentation/SubmittingPatches
> > next time you touch gitk (or git-gui), as they have their own
> > repositories and maintainers.
> 
> A comment on the patch at large: I realize that the author isn't
> familiar with Tcl, and this is a minimal & immediate fix, so maybe we
> should just take it.

Plus it takes a huge while for any patch to be merged. There's 0 commits
in 2021 on the gitk repository, and in 2020 there were only 6.

My last simple patch from May [1] is still waiting for any kind of
feedback.

[1] https://lore.kernel.org/git/20210505211846.1842824-1-felipe.contreras@gmail.com/

-- 
Felipe Contreras

@gitgitgadget
Copy link

gitgitgadget bot commented Jun 19, 2021

User Felipe Contreras <felipe.contreras@gmail.com> has been added to the cc: list.

@rosti-il
Copy link
Author

Hello everybody. I created this PR almost a year ago, is there any general progress with supporting SHA256 in gitk? Even some other's patch/PR?

@dscho
Copy link
Member

dscho commented May 2, 2022

@rosti-il if I were you, I'd just reply on-list with a gentle ping.

@gitgitgadget
Copy link

gitgitgadget bot commented May 2, 2022

On the Git mailing list, Rostislav Krasny wrote (reply to this):

Hello everybody,

I created this PR almost a year ago, is there any general progress
with supporting SHA256 in gitk? Even some other's patch/PR?

On Mon, Jun 14, 2021 at 7:18 PM Rostislav Krasny via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Rostislav Krasny <rosti.bsd@gmail.com>
>
> Signed-off-by: Rostislav Krasny <rosti.bsd@gmail.com>
> ---
>     gitk: Add a basic support of SHA256 repositories into Gitk
>
>     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.
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-979%2Frosti-il%2Fgitk-sha256-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-979/rosti-il/gitk-sha256-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/979
>
>  gitk-git/gitk | 66 ++++++++++++++++++++++++++++++++++++---------------
>  1 file changed, 47 insertions(+), 19 deletions(-)
>
> diff --git a/gitk-git/gitk b/gitk-git/gitk
> index 23d9dd1fe0d0..2da53604cdc8 100755
> --- a/gitk-git/gitk
> +++ b/gitk-git/gitk
> @@ -290,6 +290,7 @@ proc parseviewargs {n arglist} {
>
>  proc parseviewrevs {view revs} {
>      global vposids vnegids
> +    global hashlength
>
>      if {$revs eq {}} {
>          set revs HEAD
> @@ -303,7 +304,7 @@ proc parseviewrevs {view revs} {
>          set badrev {}
>          for {set l 0} {$l < [llength $errlines]} {incr l} {
>              set line [lindex $errlines $l]
> -            if {!([string length $line] == 40 && [string is xdigit $line])} {
> +            if {!([string length $line] == $hashlength && [string is xdigit $line])} {
>                  if {[string match "fatal:*" $line]} {
>                      if {[string match "fatal: ambiguous argument*" $line]
>                          && $badrev ne {}} {
> @@ -507,6 +508,7 @@ proc updatecommits {} {
>      global hasworktree
>      global varcid vposids vnegids vflags vrevs
>      global show_notes
> +    global hashlength
>
>      set hasworktree [hasworktree]
>      rereadrefs
> @@ -540,7 +542,7 @@ proc updatecommits {} {
>              # take out positive refs that we asked for before or
>              # that we have already seen
>              foreach rev $revs {
> -                if {[string length $rev] == 40} {
> +                if {[string length $rev] == $hashlength} {
>                      if {[lsearch -exact $oldpos $rev] < 0
>                          && ![info exists varcid($view,$rev)]} {
>                          lappend newrevs $rev
> @@ -1418,6 +1420,7 @@ proc getcommitlines {fd inst view updating}  {
>      global parents children curview hlview
>      global idpending ordertok
>      global varccommits varcid varctok vtokmod vfilelimit vshortids
> +    global hashlength
>
>      set stuff [read $fd 500000]
>      # git log doesn't terminate the last commit with a null...
> @@ -1500,7 +1503,7 @@ proc getcommitlines {fd inst view updating}  {
>              }
>              set ok 1
>              foreach id $ids {
> -                if {[string length $id] != 40} {
> +                if {[string length $id] != $hashlength} {
>                      set ok 0
>                      break
>                  }
> @@ -1780,6 +1783,7 @@ proc readrefs {} {
>      global selecthead selectheadid
>      global hideremotes
>      global tclencoding
> +    global hashlength
>
>      foreach v {tagids idtags headids idheads otherrefids idotherrefs} {
>          unset -nocomplain $v
> @@ -1789,9 +1793,9 @@ proc readrefs {} {
>          fconfigure $refd -encoding $tclencoding
>      }
>      while {[gets $refd line] >= 0} {
> -        if {[string index $line 40] ne " "} continue
> -        set id [string range $line 0 39]
> -        set ref [string range $line 41 end]
> +        if {[string index $line $hashlength] ne " "} continue
> +        set id [string range $line 0 [expr {$hashlength - 1}]]
> +        set ref [string range $line [expr {$hashlength + 1}] end]
>          if {![string match "refs/*" $ref]} continue
>          set name [string range $ref 5 end]
>          if {[string match "remotes/*" $name]} {
> @@ -2082,6 +2086,7 @@ proc makewindow {} {
>      global have_tk85 use_ttk NS
>      global git_version
>      global worddiff
> +    global hashlength hashalgorithm
>
>      # The "mc" arguments here are purely so that xgettext
>      # sees the following string as needing to be translated
> @@ -2203,11 +2208,11 @@ proc makewindow {} {
>      set sha1entry .tf.bar.sha1
>      set entries $sha1entry
>      set sha1but .tf.bar.sha1label
> -    button $sha1but -text "[mc "SHA1 ID:"] " -state disabled -relief flat \
> +    button $sha1but -text "[mc "$hashalgorithm ID:"] " -state disabled -relief flat \
>          -command gotocommit -width 8
>      $sha1but conf -disabledforeground [$sha1but cget -foreground]
>      pack .tf.bar.sha1label -side left
> -    ${NS}::entry $sha1entry -width 40 -font textfont -textvariable sha1string
> +    ${NS}::entry $sha1entry -width $hashlength -font textfont -textvariable sha1string
>      trace add variable sha1string write sha1change
>      pack $sha1entry -side left -pady 2
>
> @@ -3926,6 +3931,7 @@ proc stopblaming {} {
>
>  proc read_line_source {fd inst} {
>      global blamestuff curview commfd blameinst nullid nullid2
> +    global hashlength
>
>      while {[gets $fd line] >= 0} {
>          lappend blamestuff($inst) $line
> @@ -3946,7 +3952,7 @@ proc read_line_source {fd inst} {
>      set line [split [lindex $blamestuff($inst) 0] " "]
>      set id [lindex $line 0]
>      set lnum [lindex $line 1]
> -    if {[string length $id] == 40 && [string is xdigit $id] &&
> +    if {[string length $id] == $hashlength && [string is xdigit $id] &&
>          [string is digit -strict $lnum]} {
>          # look for "filename" line
>          foreach l $blamestuff($inst) {
> @@ -5269,13 +5275,14 @@ proc get_viewmainhead {view} {
>  # git rev-list should give us just 1 line to use as viewmainheadid($view)
>  proc getviewhead {fd inst view} {
>      global viewmainheadid commfd curview viewinstances showlocalchanges
> +    global hashlength
>
>      set id {}
>      if {[gets $fd line] < 0} {
>          if {![eof $fd]} {
>              return 1
>          }
> -    } elseif {[string length $line] == 40 && [string is xdigit $line]} {
> +    } elseif {[string length $line] == $hashlength && [string is xdigit $line]} {
>          set id $line
>      }
>      set viewmainheadid($view) $id
> @@ -7039,10 +7046,11 @@ proc commit_descriptor {p} {
>  # Also look for URLs of the form "http[s]://..." and make them web links.
>  proc appendwithlinks {text tags} {
>      global ctext linknum curview
> +    global hashlength
>
>      set start [$ctext index "end - 1c"]
>      $ctext insert end $text $tags
> -    set links [regexp -indices -all -inline {(?:\m|-g)[0-9a-f]{6,40}\M} $text]
> +    set links [regexp -indices -all -inline [string map "@@ $hashlength" {(?:\m|-g)[0-9a-f]{6,@@}\M}] $text]
>      foreach l $links {
>          set s [lindex $l 0]
>          set e [lindex $l 1]
> @@ -8716,13 +8724,17 @@ proc incrfont {inc} {
>
>  proc clearsha1 {} {
>      global sha1entry sha1string
> -    if {[string length $sha1string] == 40} {
> +    global hashlength
> +
> +    if {[string length $sha1string] == $hashlength} {
>          $sha1entry delete 0 end
>      }
>  }
>
>  proc sha1change {n1 n2 op} {
>      global sha1string currentid sha1but
> +    global hashalgorithm
> +
>      if {$sha1string == {}
>          || ([info exists currentid] && $sha1string == $currentid)} {
>          set state disabled
> @@ -8733,12 +8745,13 @@ proc sha1change {n1 n2 op} {
>      if {$state == "normal"} {
>          $sha1but conf -state normal -relief raised -text "[mc "Goto:"] "
>      } else {
> -        $sha1but conf -state disabled -relief flat -text "[mc "SHA1 ID:"] "
> +        $sha1but conf -state disabled -relief flat -text "[mc "$hashalgorithm ID:"] "
>      }
>  }
>
>  proc gotocommit {} {
>      global sha1string tagids headids curview varcid
> +    global hashlength hashalgorithm
>
>      if {$sha1string == {}
>          || ([info exists currentid] && $sha1string == $currentid)} return
> @@ -8748,11 +8761,11 @@ proc gotocommit {} {
>          set id $headids($sha1string)
>      } else {
>          set id [string tolower $sha1string]
> -        if {[regexp {^[0-9a-f]{4,39}$} $id]} {
> +        if {[regexp [string map "@@ [expr $hashlength - 1]" {^[0-9a-f]{4,@@}$}] $id]} {
>              set matches [longid $id]
>              if {$matches ne {}} {
>                  if {[llength $matches] > 1} {
> -                    error_popup [mc "Short SHA1 id %s is ambiguous" $id]
> +                    error_popup [mc "Short $hashalgorithm id %s is ambiguous" $id]
>                      return
>                  }
>                  set id [lindex $matches 0]
> @@ -8769,7 +8782,7 @@ proc gotocommit {} {
>          return
>      }
>      if {[regexp {^[0-9a-fA-F]{4,}$} $sha1string]} {
> -        set msg [mc "SHA1 id %s is not known" $sha1string]
> +        set msg [mc "$hashalgorithm id %s is not known" $sha1string]
>      } else {
>          set msg [mc "Revision %s is not in the current view" $sha1string]
>      }
> @@ -9446,10 +9459,11 @@ proc mktaggo {} {
>
>  proc copyreference {} {
>      global rowmenuid autosellen
> +    global hashlength
>
>      set format "%h (\"%s\", %ad)"
>      set cmd [list git show -s --pretty=format:$format --date=short]
> -    if {$autosellen < 40} {
> +    if {$autosellen < $hashlength} {
>          lappend cmd --abbrev=$autosellen
>      }
>      set reference [eval exec $cmd $rowmenuid]
> @@ -9460,6 +9474,7 @@ proc copyreference {} {
>
>  proc writecommit {} {
>      global rowmenuid wrcomtop commitinfo wrcomcmd NS
> +    global hashlength
>
>      set top .writecommit
>      set wrcomtop $top
> @@ -9469,7 +9484,7 @@ proc writecommit {} {
>      ${NS}::label $top.title -text [mc "Write commit to file"]
>      grid $top.title - -pady 10
>      ${NS}::label $top.id -text [mc "ID:"]
> -    ${NS}::entry $top.sha1 -width 40
> +    ${NS}::entry $top.sha1 -width $hashlength
>      $top.sha1 insert 0 $rowmenuid
>      $top.sha1 conf -state readonly
>      grid $top.id $top.sha1 -sticky w
> @@ -9549,6 +9564,7 @@ proc mvbranch {} {
>
>  proc branchdia {top valvar uivar} {
>      global NS commitinfo
> +    global hashlength
>      upvar $valvar val $uivar ui
>
>      catch {destroy $top}
> @@ -9557,7 +9573,7 @@ proc branchdia {top valvar uivar} {
>      ${NS}::label $top.title -text $ui(title)
>      grid $top.title - -pady 10
>      ${NS}::label $top.id -text [mc "ID:"]
> -    ${NS}::entry $top.sha1 -width 40
> +    ${NS}::entry $top.sha1 -width $hashlength
>      $top.sha1 insert 0 $val(id)
>      $top.sha1 conf -state readonly
>      grid $top.id $top.sha1 -sticky w
> @@ -12320,6 +12336,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}"
> +    exit 1
> +}
> +set hashalgorithm [string toupper $objformat]
> +unset objformat
> +
>  set gui_encoding [encoding system]
>  catch {
>      set enc [exec git config --get gui.encoding]
>
> base-commit: ebf3c04b262aa27fbb97f8a0156c2347fecafafb
> --
> gitgitgadget

@gitgitgadget
Copy link

gitgitgadget bot commented May 2, 2022

User Rostislav Krasny <rosti.bsd@gmail.com> has been added to the cc: list.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants