{"thread":{"id":"56684","subject":"[RFC PATCH 0/4] git-gui: support SHA-256 repositories","startedAt":"2021-10-11T12:18:11Z","lastAt":"2021-11-13T08:10:38Z","messageCount":14,"participants":["Carlo Marcelo Arenas Belón","Ævar Arnfjörð Bjarmason","Carlo Arenas","Eric Sunshine","Pratyush Yadav"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"438454","messageId":"20211011121757.627-1-carenas@gmail.com","threadId":"56684","inReplyTo":null,"subject":"[RFC PATCH 0/4] git-gui: support SHA-256 repositories","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2021-10-11T12:17:53Z","receivedAt":"2021-10-11T12:18:11Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"While poking a SHA-256 hash repository, was surprised to find gitk\nwould fail with a fatal error when called, hence this series.\n\nSending as an RFC, since I am not a git-gui or gitk user, and so\nwhile this fixes the original issue and allows me to call gitk to\nsee the branch merge history (which is usually as much as I do with\nit), it is likey missing some changes, as most of them where found\nby lightly poking at all of the gui menus (except for remote or tool)\n\nIt could also be reordered to reduce unnecessary churn and of course\nalso needs the gitk change[1] that was sent independently, and better\ncommit messages.\n\n[1] https://lore.kernel.org/git/20211011114723.204-1-carenas@gmail.com/\n\nCarlo Marcelo Arenas Belón (4):\n  blame: prefer null_sha1 over nullid and retire later\n  rename all *_sha1 variables and make null_oid hash aware\n  expand regexp matching an oid to be hash agnostic\n  track oid_size to allow for checks that are hash agnostic\n\n git-gui.sh                   | 30 ++++++++++++++++--------------\n lib/blame.tcl                | 18 +++++++++---------\n lib/checkout_op.tcl          |  4 ++--\n lib/choose_repository.tcl    |  2 +-\n lib/commit.tcl               |  3 ++-\n lib/remote_branch_delete.tcl |  2 +-\n 6 files changed, 31 insertions(+), 28 deletions(-)\n\n-- \n2.33.0.1081.g099423f5b7\n\n"},{"id":"438455","messageId":"20211011121757.627-2-carenas@gmail.com","threadId":"56684","inReplyTo":"20211011121757.627-1-carenas@gmail.com","subject":"[RFC PATCH 1/4] blame: prefer null_sha1 over nullid and retire later","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2021-10-11T12:17:54Z","receivedAt":"2021-10-11T12:18:12Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"a9786bb (git-gui: Fix Blame Parent & Context for working copy lines.,\n2008-09-08) adds nullid (and a never used nullid2) for matching locally\nmodified lines in blame.\n\nUse instead the already available null_sha1 for the same and in\npreparation to making that hash independent on a future patch.\n\nSigned-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>\n---\n git-gui.sh    |  3 ---\n lib/blame.tcl | 10 +++++-----\n 2 files changed, 5 insertions(+), 8 deletions(-)\n\ndiff --git a/git-gui.sh b/git-gui.sh\nindex 201524c..a69b0fe 100755\n--- a/git-gui.sh\n+++ b/git-gui.sh\n@@ -1353,9 +1353,6 @@ set diff_empty_count 0\n set last_revert {}\n set last_revert_enc {}\n \n-set nullid \"0000000000000000000000000000000000000000\"\n-set nullid2 \"0000000000000000000000000000000000000001\"\n-\n ######################################################################\n ##\n ## task management\ndiff --git a/lib/blame.tcl b/lib/blame.tcl\nindex 8441e10..6ece79d 100644\n--- a/lib/blame.tcl\n+++ b/lib/blame.tcl\n@@ -1056,14 +1056,14 @@ method _format_offset_date {base offset} {\n }\n \n method _gitkcommit {} {\n-\tglobal nullid\n+\tglobal null_sha1\n \n \tset dat [_get_click_amov_info $this]\n \tif {$dat ne {}} {\n \t\tset cmit [lindex $dat 0]\n \n \t\t# If the line belongs to the working copy, use HEAD instead\n-\t\tif {$cmit eq $nullid} {\n+\t\tif {$cmit eq $null_sha1} {\n \t\t\tif {[catch {set cmit [git rev-parse --verify HEAD]} err]} {\n \t\t\t\terror_popup [strcat [mc \"Cannot find HEAD commit:\"] \"\\n\\n$err\"]\n \t\t\t\treturn;\n@@ -1106,7 +1106,7 @@ method _gitkcommit {} {\n }\n \n method _blameparent {} {\n-\tglobal nullid\n+\tglobal null_sha1\n \n \tset dat [_get_click_amov_info $this]\n \tif {$dat ne {}} {\n@@ -1114,7 +1114,7 @@ method _blameparent {} {\n \t\tset new_path [lindex $dat 1]\n \n \t\t# Allow using Blame Parent on lines modified in the working copy\n-\t\tif {$cmit eq $nullid} {\n+\t\tif {$cmit eq $null_sha1} {\n \t\t\tset parent_ref \"HEAD\"\n \t\t} else {\n \t\t\tset parent_ref \"$cmit^\"\n@@ -1129,7 +1129,7 @@ method _blameparent {} {\n \t\t# Generate a diff between the commit and its parent,\n \t\t# and use the hunks to update the line number.\n \t\t# Request zero context to simplify calculations.\n-\t\tif {$cmit eq $nullid} {\n+\t\tif {$cmit eq $null_sha1} {\n \t\t\tset diffcmd [list diff-index --unified=0 $cparent -- $new_path]\n \t\t} else {\n \t\t\tset diffcmd [list diff-tree --unified=0 $cparent $cmit -- $new_path]\n-- \n2.33.0.1081.g099423f5b7\n\n"},{"id":"438456","messageId":"20211011121757.627-3-carenas@gmail.com","threadId":"56684","inReplyTo":"20211011121757.627-1-carenas@gmail.com","subject":"[RFC PATCH 2/4] rename all *_sha1 variables and make null_oid hash aware","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2021-10-11T12:17:55Z","receivedAt":"2021-10-11T12:18:13Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"Before this change, creating a branch in an SHA-256 repository would\nfail because the null_sha1 used was of the wrong size.\n\nSigned-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>\n---\n git-gui.sh          | 26 +++++++++++++++-----------\n lib/blame.tcl       | 10 +++++-----\n lib/checkout_op.tcl |  4 ++--\n 3 files changed, 22 insertions(+), 18 deletions(-)\n\ndiff --git a/git-gui.sh b/git-gui.sh\nindex a69b0fe..c0dc8ce 100755\n--- a/git-gui.sh\n+++ b/git-gui.sh\n@@ -1820,10 +1820,14 @@ proc short_path {path} {\n }\n \n set next_icon_id 0\n-set null_sha1 [string repeat 0 40]\n+if { [get_config extensions.objectformat] eq \"sha256\" } {\n+\tset null_oid [string repeat 0 64]\n+} else {\n+\tset null_oid [string repeat 0 40]\n+}\n \n proc merge_state {path new_state {head_info {}} {index_info {}}} {\n-\tglobal file_states next_icon_id null_sha1\n+\tglobal file_states next_icon_id null_oid\n \n \tset s0 [string index $new_state 0]\n \tset s1 [string index $new_state 1]\n@@ -1845,7 +1849,7 @@ proc merge_state {path new_state {head_info {}} {index_info {}}} {\n \telseif {$s1 eq {_}} {set s1 _}\n \n \tif {$s0 eq {A} && $s1 eq {_} && $head_info eq {}} {\n-\t\tset head_info [list 0 $null_sha1]\n+\t\tset head_info [list 0 $null_oid]\n \t} elseif {$s0 ne {_} && [string index $state 0] eq {_}\n \t\t&& $head_info eq {}} {\n \t\tset head_info $index_info\n@@ -2179,21 +2183,21 @@ proc do_gitk {revs {is_submodule false}} {\n \t\t\tcd $current_diff_path\n \t\t\tif {$revs eq {--}} {\n \t\t\t\tset s $file_states($current_diff_path)\n-\t\t\t\tset old_sha1 {}\n-\t\t\t\tset new_sha1 {}\n+\t\t\t\tset old_oid {}\n+\t\t\t\tset new_oid {}\n \t\t\t\tswitch -glob -- [lindex $s 0] {\n-\t\t\t\tM_ { set old_sha1 [lindex [lindex $s 2] 1] }\n-\t\t\t\t_M { set old_sha1 [lindex [lindex $s 3] 1] }\n+\t\t\t\tM_ { set old_oid [lindex [lindex $s 2] 1] }\n+\t\t\t\t_M { set old_oid [lindex [lindex $s 3] 1] }\n \t\t\t\tMM {\n \t\t\t\t\tif {$current_diff_side eq $ui_index} {\n-\t\t\t\t\t\tset old_sha1 [lindex [lindex $s 2] 1]\n-\t\t\t\t\t\tset new_sha1 [lindex [lindex $s 3] 1]\n+\t\t\t\t\t\tset old_oid [lindex [lindex $s 2] 1]\n+\t\t\t\t\t\tset new_oid [lindex [lindex $s 3] 1]\n \t\t\t\t\t} else {\n-\t\t\t\t\t\tset old_sha1 [lindex [lindex $s 3] 1]\n+\t\t\t\t\t\tset old_oid [lindex [lindex $s 3] 1]\n \t\t\t\t\t}\n \t\t\t\t}\n \t\t\t\t}\n-\t\t\t\tset revs $old_sha1...$new_sha1\n+\t\t\t\tset revs $old_oid...$new_oid\n \t\t\t}\n \t\t\t# GIT_DIR and GIT_WORK_TREE for the submodule are not the ones\n \t\t\t# we've been using for the main repository, so unset them.\ndiff --git a/lib/blame.tcl b/lib/blame.tcl\nindex 6ece79d..e6d4302 100644\n--- a/lib/blame.tcl\n+++ b/lib/blame.tcl\n@@ -1056,14 +1056,14 @@ method _format_offset_date {base offset} {\n }\n \n method _gitkcommit {} {\n-\tglobal null_sha1\n+\tglobal null_oid\n \n \tset dat [_get_click_amov_info $this]\n \tif {$dat ne {}} {\n \t\tset cmit [lindex $dat 0]\n \n \t\t# If the line belongs to the working copy, use HEAD instead\n-\t\tif {$cmit eq $null_sha1} {\n+\t\tif {$cmit eq $null_oid} {\n \t\t\tif {[catch {set cmit [git rev-parse --verify HEAD]} err]} {\n \t\t\t\terror_popup [strcat [mc \"Cannot find HEAD commit:\"] \"\\n\\n$err\"]\n \t\t\t\treturn;\n@@ -1106,7 +1106,7 @@ method _gitkcommit {} {\n }\n \n method _blameparent {} {\n-\tglobal null_sha1\n+\tglobal null_oid\n \n \tset dat [_get_click_amov_info $this]\n \tif {$dat ne {}} {\n@@ -1114,7 +1114,7 @@ method _blameparent {} {\n \t\tset new_path [lindex $dat 1]\n \n \t\t# Allow using Blame Parent on lines modified in the working copy\n-\t\tif {$cmit eq $null_sha1} {\n+\t\tif {$cmit eq $null_oid} {\n \t\t\tset parent_ref \"HEAD\"\n \t\t} else {\n \t\t\tset parent_ref \"$cmit^\"\n@@ -1129,7 +1129,7 @@ method _blameparent {} {\n \t\t# Generate a diff between the commit and its parent,\n \t\t# and use the hunks to update the line number.\n \t\t# Request zero context to simplify calculations.\n-\t\tif {$cmit eq $null_sha1} {\n+\t\tif {$cmit eq $null_oid} {\n \t\t\tset diffcmd [list diff-index --unified=0 $cparent -- $new_path]\n \t\t} else {\n \t\t\tset diffcmd [list diff-tree --unified=0 $cparent $cmit -- $new_path]\ndiff --git a/lib/checkout_op.tcl b/lib/checkout_op.tcl\nindex 21ea768..be1ebba 100644\n--- a/lib/checkout_op.tcl\n+++ b/lib/checkout_op.tcl\n@@ -151,7 +151,7 @@ method _finish_fetch {ok} {\n }\n \n method _update_ref {} {\n-\tglobal null_sha1 current_branch repo_config\n+\tglobal null_oid current_branch repo_config\n \n \tset ref $new_ref\n \tset new $new_hash\n@@ -177,7 +177,7 @@ method _update_ref {} {\n \t\t}\n \n \t\tset reflog_msg \"branch: Created from $new_expr\"\n-\t\tset cur $null_sha1\n+\t\tset cur $null_oid\n \n \t\tif {($repo_config(branch.autosetupmerge) eq {true}\n \t\t\t|| $repo_config(branch.autosetupmerge) eq {always})\n-- \n2.33.0.1081.g099423f5b7\n\n"},{"id":"438457","messageId":"20211011121757.627-4-carenas@gmail.com","threadId":"56684","inReplyTo":"20211011121757.627-1-carenas@gmail.com","subject":"[RFC PATCH 3/4] expand regexp matching an oid to be hash agnostic","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2021-10-11T12:17:56Z","receivedAt":"2021-10-11T12:18:15Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"Before this change, listing or blame will fail as it couldn't find the\nOID in an SHA-256 repository.\n\nSigned-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>\n---\n lib/blame.tcl                | 8 ++++----\n lib/choose_repository.tcl    | 2 +-\n lib/remote_branch_delete.tcl | 2 +-\n 3 files changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/lib/blame.tcl b/lib/blame.tcl\nindex e6d4302..ee7db9d 100644\n--- a/lib/blame.tcl\n+++ b/lib/blame.tcl\n@@ -436,7 +436,7 @@ method _load {jump} {\n \t\t\t$i conf -state normal\n \t\t\t$i delete 0.0 end\n \t\t\tforeach g [$i tag names] {\n-\t\t\t\tif {[regexp {^g[0-9a-f]{40}$} $g]} {\n+\t\t\t\tif {[regexp {^g[0-9a-f]{40}(?:[0-9a-f]{24})?$} $g]} {\n \t\t\t\t\t$i tag delete $g\n \t\t\t\t}\n \t\t\t}\n@@ -513,7 +513,7 @@ method _history_menu {} {\n \t\tset c [lindex $e 0]\n \t\tset f [lindex $e 1]\n \n-\t\tif {[regexp {^[0-9a-f]{40}$} $c]} {\n+\t\tif {[regexp {^[0-9a-f]{40}(?:[0-9a-f]{24})?$} $c]} {\n \t\t\tset t [string range $c 0 8]...\n \t\t} elseif {$c eq {}} {\n \t\t\tset t {Working Directory}\n@@ -635,7 +635,7 @@ method _read_blame {fd cur_w cur_d} {\n \n \t$cur_w conf -state normal\n \twhile {[gets $fd line] >= 0} {\n-\t\tif {[regexp {^([a-z0-9]{40}) (\\d+) (\\d+) (\\d+)$} $line line \\\n+\t\tif {[regexp {^([a-z0-9]{40}(?:[0-9a-f]{24})?) (\\d+) (\\d+) (\\d+)$} $line line \\\n \t\t\tcmit original_line final_line line_count]} {\n \t\t\tset r_commit     $cmit\n \t\t\tset r_orig_line  $original_line\n@@ -648,7 +648,7 @@ method _read_blame {fd cur_w cur_d} {\n \t\t\tset oln  $r_orig_line\n \t\t\tset cmit $r_commit\n \n-\t\t\tif {[regexp {^0{40}$} $cmit]} {\n+\t\t\tif {[regexp {^0{40}(?:0{24})?$} $cmit]} {\n \t\t\t\tset commit_abbr work\n \t\t\t\tset commit_type curr_commit\n \t\t\t} elseif {$cmit eq $commit} {\ndiff --git a/lib/choose_repository.tcl b/lib/choose_repository.tcl\nindex af1fee7..e864f38 100644\n--- a/lib/choose_repository.tcl\n+++ b/lib/choose_repository.tcl\n@@ -904,7 +904,7 @@ method _do_clone_full_end {ok} {\n \t\tif {[file exists [gitdir FETCH_HEAD]]} {\n \t\t\tset fd [open [gitdir FETCH_HEAD] r]\n \t\t\twhile {[gets $fd line] >= 0} {\n-\t\t\t\tif {[regexp \"^(.{40})\\t\\t\" $line line HEAD]} {\n+\t\t\t\tif {[regexp \"^([0-9a-fA-F]{40}(?:[0-9a-fA-F]{24})?)\\t\\t\" $line line HEAD]} {\n \t\t\t\t\tbreak\n \t\t\t\t}\n \t\t\t}\ndiff --git a/lib/remote_branch_delete.tcl b/lib/remote_branch_delete.tcl\nindex 5ba9fca..57bae9c 100644\n--- a/lib/remote_branch_delete.tcl\n+++ b/lib/remote_branch_delete.tcl\n@@ -330,7 +330,7 @@ method _read {cache fd} {\n \n \twhile {[gets $fd line] >= 0} {\n \t\tif {[string match {*^{}} $line]} continue\n-\t\tif {[regexp {^([0-9a-f]{40})\t(.*)$} $line _junk obj ref]} {\n+\t\tif {[regexp {^([0-9a-fA-F]{40}(?:[0-9a-fA-F]{24})?)\t(.*)$} $line _junk obj ref]} {\n \t\t\tif {[regsub ^refs/heads/ $ref {} abr]} {\n \t\t\t\tlappend head_list $abr\n \t\t\t\tlappend head_cache($cache) $abr\n-- \n2.33.0.1081.g099423f5b7\n\n"},{"id":"438458","messageId":"20211011121757.627-5-carenas@gmail.com","threadId":"56684","inReplyTo":"20211011121757.627-1-carenas@gmail.com","subject":"[RFC PATCH 4/4] track oid_size to allow for checks that are hash agnostic","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2021-10-11T12:17:57Z","receivedAt":"2021-10-11T12:18:17Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"This allows commit to work.\n\nSigned-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>\n---\n git-gui.sh     | 5 +++--\n lib/commit.tcl | 3 ++-\n 2 files changed, 5 insertions(+), 3 deletions(-)\n\ndiff --git a/git-gui.sh b/git-gui.sh\nindex c0dc8ce..1646124 100755\n--- a/git-gui.sh\n+++ b/git-gui.sh\n@@ -1821,10 +1821,11 @@ proc short_path {path} {\n \n set next_icon_id 0\n if { [get_config extensions.objectformat] eq \"sha256\" } {\n-\tset null_oid [string repeat 0 64]\n+\tset oid_size 64\n } else {\n-\tset null_oid [string repeat 0 40]\n+\tset oid_size 40\n }\n+set null_oid [string repeat 0 $oid_size]\n \n proc merge_state {path new_state {head_info {}} {index_info {}}} {\n \tglobal file_states next_icon_id null_oid\ndiff --git a/lib/commit.tcl b/lib/commit.tcl\nindex 11379f8..1306e8d 100644\n--- a/lib/commit.tcl\n+++ b/lib/commit.tcl\n@@ -337,6 +337,7 @@ proc commit_committree {fd_wt curHEAD msg_p} {\n \tglobal file_states selected_paths rescan_active\n \tglobal repo_config\n \tglobal env\n+\tglobal oid_size\n \n \tgets $fd_wt tree_id\n \tif {[catch {close $fd_wt} err]} {\n@@ -356,7 +357,7 @@ proc commit_committree {fd_wt curHEAD msg_p} {\n \t\tclose $fd_ot\n \n \t\tif {[string equal -length 5 {tree } $old_tree]\n-\t\t\t&& [string length $old_tree] == 45} {\n+\t\t\t&& [string length $old_tree] == 5 + oid_size} {\n \t\t\tset old_tree [string range $old_tree 5 end]\n \t\t} else {\n \t\t\terror [mc \"Commit %s appears to be corrupt\" $PARENT]\n-- \n2.33.0.1081.g099423f5b7\n\n"},{"id":"438465","messageId":"87bl3vlk0j.fsf@evledraar.gmail.com","threadId":"56684","inReplyTo":"20211011121757.627-1-carenas@gmail.com","subject":"Re: [RFC PATCH 0/4] git-gui: support SHA-256 repositories","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-11T14:15:36Z","receivedAt":"2021-10-11T14:31:36Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Oct 11 2021, Carlo Marcelo Arenas Belón wrote:\n\n> While poking a SHA-256 hash repository, was surprised to find gitk\n> would fail with a fatal error when called, hence this series.\n>\n> Sending as an RFC, since I am not a git-gui or gitk user, and so\n> while this fixes the original issue and allows me to call gitk to\n> see the branch merge history (which is usually as much as I do with\n> it), it is likey missing some changes, as most of them where found\n> by lightly poking at all of the gui menus (except for remote or tool)\n>\n> It could also be reordered to reduce unnecessary churn and of course\n> also needs the gitk change[1] that was sent independently, and better\n> commit messages.\n>\n> [1] https://lore.kernel.org/git/20211011114723.204-1-carenas@gmail.com/\n>\n> Carlo Marcelo Arenas Belón (4):\n>   blame: prefer null_sha1 over nullid and retire later\n>   rename all *_sha1 variables and make null_oid hash aware\n>   expand regexp matching an oid to be hash agnostic\n>   track oid_size to allow for checks that are hash agnostic\n>\n>  git-gui.sh                   | 30 ++++++++++++++++--------------\n>  lib/blame.tcl                | 18 +++++++++---------\n>  lib/checkout_op.tcl          |  4 ++--\n>  lib/choose_repository.tcl    |  2 +-\n>  lib/commit.tcl               |  3 ++-\n>  lib/remote_branch_delete.tcl |  2 +-\n>  6 files changed, 31 insertions(+), 28 deletions(-)\n\nThere was a similar series earlier this year which didn't make it that\nfixes some of the same issues:\nhttps://lore.kernel.org/git/pull.979.git.1623687519832.gitgitgadget@gmail.com/\n\nMy comment on this one is much the same as that: I don't use this\nsoftware, and if you've tested this I trust that it's better & this\ngoing in as-is would be better than the status quo.\n\nBut also that as noted in the feedback there it seems that:\n\n 1. Figuring out if we're using SHA-1 or SHA-256\n\n 2. Adjusting all regexes to *exactly* math those things, i.e. using\n    things like x{40}(?:x{24})\n\nJust seems like a lot of needless work as opposed to just matching\nx{40,64} or whatever.  Yes that's not the same regex semantically, but I\nthink the current code is just being overly strict, i.e. it's parsing\nsome plumbing output, we can trust that the thing that looks like the\nOID in that position is the OID.\n\nIf anything I'd think we could just match [0-9a-f]{4,} in most/all of\nthese cases, would make things like this easier to read:\n\n-\t\tif {[regexp {^([a-z0-9]{40}) (\\d+) (\\d+) (\\d+)$} $line line \\\n+\t\tif {[regexp {^([a-z0-9]{40}(?:[0-9a-f]{24})?) (\\d+) (\\d+) (\\d+)$} $line line \\\n\nAnd also the pre-existing [a-z0-9]{40} is a very weird mixture of being\noverly permissive and overly strict :)\n"},{"id":"438484","messageId":"CAPUEspjFZDKtP8oJmuA6dCcX9XF1WBFvFikkZTNcKfHbOxJwPA@mail.gmail.com","threadId":"56684","inReplyTo":"87bl3vlk0j.fsf@evledraar.gmail.com","subject":"Re: [RFC PATCH 0/4] git-gui: support SHA-256 repositories","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2021-10-11T19:47:13Z","receivedAt":"2021-10-11T19:47:28Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Mon, Oct 11, 2021 at 7:21 AM Ævar Arnfjörð Bjarmason\n<avarab@gmail.com> wrote:\n>\n> On Mon, Oct 11 2021, Carlo Marcelo Arenas Belón wrote:\n>\n> > [1] https://lore.kernel.org/git/20211011114723.204-1-carenas@gmail.com/\n> >\n> > Carlo Marcelo Arenas Belón (4):\n> >   blame: prefer null_sha1 over nullid and retire later\n> >   rename all *_sha1 variables and make null_oid hash aware\n> >   expand regexp matching an oid to be hash agnostic\n> >   track oid_size to allow for checks that are hash agnostic\n> >\n> >  git-gui.sh                   | 30 ++++++++++++++++--------------\n> >  lib/blame.tcl                | 18 +++++++++---------\n> >  lib/checkout_op.tcl          |  4 ++--\n> >  lib/choose_repository.tcl    |  2 +-\n> >  lib/commit.tcl               |  3 ++-\n> >  lib/remote_branch_delete.tcl |  2 +-\n> >  6 files changed, 31 insertions(+), 28 deletions(-)\n>\n> There was a similar series earlier this year which didn't make it that\n> fixes some of the same issues:\n> https://lore.kernel.org/git/pull.979.git.1623687519832.gitgitgadget@gmail.com/\n\nThis specific series is for git-gui, and the one posted before is for gitk,\nbut the code is still similar enough, and indeed the gitk part was\nincluded in a reference.\n\n> Just seems like a lot of needless work as opposed to just matching\n> x{40,64} or whatever.  Yes that's not the same regex semantically, but I\n> think the current code is just being overly strict, i.e. it's parsing\n> some plumbing output, we can trust that the thing that looks like the\n> OID in that position is the OID.\n>\n> If anything I'd think we could just match [0-9a-f]{4,} in most/all of\n> these cases, would make things like this easier to read:\n\nIt makes me nervous though to see checks like the one I fixed on\ncommit[1] that use logic to check the correct size of the SHA as an\nimplication of it being a valid value.\n\nconsidering the code is very old, maybe that was relevant long ago?,\nbut agree some checks seem to be unnecessarily strict.\n\nI have relaxed some of the checks in the gitk patch and will be\nposting it soon, so hopefully reviews from people that know the code\nbetter could be collected.\n\nCarlo\n\n[1] https://lore.kernel.org/git/20211011121757.627-5-carenas@gmail.com/\n"},{"id":"438489","messageId":"CAPig+cSJentAiOGhqKpgZgXDW9NAK3OMHhKyn6auYN7b=Vk74w@mail.gmail.com","threadId":"56684","inReplyTo":"20211011121757.627-3-carenas@gmail.com","subject":"Re: [RFC PATCH 2/4] rename all *_sha1 variables and make null_oid hash aware","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-10-11T20:07:40Z","receivedAt":"2021-10-11T20:07:54Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Oct 11, 2021 at 8:18 AM Carlo Marcelo Arenas Belón\n<carenas@gmail.com> wrote:\n> Before this change, creating a branch in an SHA-256 repository would\n> fail because the null_sha1 used was of the wrong size.\n>\n> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>\n> ---\n> diff --git a/git-gui.sh b/git-gui.sh\n> @@ -1820,10 +1820,14 @@ proc short_path {path} {\n> +if { [get_config extensions.objectformat] eq \"sha256\" } {\n> +       set null_oid [string repeat 0 64]\n> +} else {\n> +       set null_oid [string repeat 0 40]\n> +}\n\nShould this be using:\n\n    git rev-parse --show-object-format\n\nrather than reading the configuration directly?\n"},{"id":"439819","messageId":"20211027194319.iwa6gx3xuth5rclu@yadavpratyush.com","threadId":"56684","inReplyTo":"20211011121757.627-2-carenas@gmail.com","subject":"Re: [RFC PATCH 1/4] blame: prefer null_sha1 over nullid and retire later","fromName":"Pratyush Yadav","fromEmail":"me@yadavpratyush.com","sentAt":"2021-10-27T19:43:19Z","receivedAt":"2021-10-27T19:43:26Z","isPatch":true,"sender":{"key":"me@yadavpratyush.com","avatar":"https://avatars.githubusercontent.com/u/8817931?v=4"},"body":"On 11/10/21 05:17AM, Carlo Marcelo Arenas Belón wrote:\n> a9786bb (git-gui: Fix Blame Parent & Context for working copy lines.,\n> 2008-09-08) adds nullid (and a never used nullid2) for matching locally\n> modified lines in blame.\n> \n> Use instead the already available null_sha1 for the same and in\n> preparation to making that hash independent on a future patch.\n\nLGTM.\n\n-- \nRegards,\nPratyush Yadav\n"},{"id":"441053","messageId":"20211113065406.z2lqhvh24jjaqty6@yadavpratyush.com","threadId":"56684","inReplyTo":"20211011121757.627-3-carenas@gmail.com","subject":"Re: [RFC PATCH 2/4] rename all *_sha1 variables and make null_oid hash aware","fromName":"Pratyush Yadav","fromEmail":"me@yadavpratyush.com","sentAt":"2021-11-13T06:54:06Z","receivedAt":"2021-11-13T06:54:21Z","isPatch":true,"sender":{"key":"me@yadavpratyush.com","avatar":"https://avatars.githubusercontent.com/u/8817931?v=4"},"body":"On 11/10/21 05:17AM, Carlo Marcelo Arenas Belón wrote:\n> Before this change, creating a branch in an SHA-256 repository would\n> fail because the null_sha1 used was of the wrong size.\n> \n> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>\n> ---\n>  git-gui.sh          | 26 +++++++++++++++-----------\n>  lib/blame.tcl       | 10 +++++-----\n>  lib/checkout_op.tcl |  4 ++--\n>  3 files changed, 22 insertions(+), 18 deletions(-)\n> \n> diff --git a/git-gui.sh b/git-gui.sh\n> index a69b0fe..c0dc8ce 100755\n> --- a/git-gui.sh\n> +++ b/git-gui.sh\n> @@ -1820,10 +1820,14 @@ proc short_path {path} {\n>  }\n>  \n>  set next_icon_id 0\n> -set null_sha1 [string repeat 0 40]\n> +if { [get_config extensions.objectformat] eq \"sha256\" } {\n\nFrom the docs I see that this feature is experimental as of now and \nmight change in the future. Can we expect this config option to stay \nstable over time? If not I think this might be too early to introduce it \ninto git-gui.\n\nAnyway, nitpick: don't add spaces after opening brace and before closing \nbrace.\n\n> +\tset null_oid [string repeat 0 64]\n> +} else {\n> +\tset null_oid [string repeat 0 40]\n> +}\n>  \n>  proc merge_state {path new_state {head_info {}} {index_info {}}} {\n> -\tglobal file_states next_icon_id null_sha1\n> +\tglobal file_states next_icon_id null_oid\n>  \n>  \tset s0 [string index $new_state 0]\n>  \tset s1 [string index $new_state 1]\n\nRest of the patch looks good to me. Thanks.\n\n-- \nRegards,\nPratyush Yadav\n"},{"id":"441056","messageId":"20211113075520.vzy23i6b5kinaeob@yadavpratyush.com","threadId":"56684","inReplyTo":"20211011121757.627-4-carenas@gmail.com","subject":"Re: [RFC PATCH 3/4] expand regexp matching an oid to be hash agnostic","fromName":"Pratyush Yadav","fromEmail":"me@yadavpratyush.com","sentAt":"2021-11-13T07:55:20Z","receivedAt":"2021-11-13T08:06:24Z","isPatch":true,"sender":{"key":"me@yadavpratyush.com","avatar":"https://avatars.githubusercontent.com/u/8817931?v=4"},"body":"On 11/10/21 05:17AM, Carlo Marcelo Arenas Belón wrote:\n> Before this change, listing or blame will fail as it couldn't find the\n> OID in an SHA-256 repository.\n> \n> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>\n> ---\n>  lib/blame.tcl                | 8 ++++----\n>  lib/choose_repository.tcl    | 2 +-\n>  lib/remote_branch_delete.tcl | 2 +-\n>  3 files changed, 6 insertions(+), 6 deletions(-)\n> \n> diff --git a/lib/blame.tcl b/lib/blame.tcl\n> index e6d4302..ee7db9d 100644\n> --- a/lib/blame.tcl\n> +++ b/lib/blame.tcl\n> @@ -436,7 +436,7 @@ method _load {jump} {\n>  \t\t\t$i conf -state normal\n>  \t\t\t$i delete 0.0 end\n>  \t\t\tforeach g [$i tag names] {\n> -\t\t\t\tif {[regexp {^g[0-9a-f]{40}$} $g]} {\n> +\t\t\t\tif {[regexp {^g[0-9a-f]{40}(?:[0-9a-f]{24})?$} $g]} {\n>  \t\t\t\t\t$i tag delete $g\n>  \t\t\t\t}\n>  \t\t\t}\n> @@ -513,7 +513,7 @@ method _history_menu {} {\n>  \t\tset c [lindex $e 0]\n>  \t\tset f [lindex $e 1]\n>  \n> -\t\tif {[regexp {^[0-9a-f]{40}$} $c]} {\n> +\t\tif {[regexp {^[0-9a-f]{40}(?:[0-9a-f]{24})?$} $c]} {\n>  \t\t\tset t [string range $c 0 8]...\n>  \t\t} elseif {$c eq {}} {\n>  \t\t\tset t {Working Directory}\n> @@ -635,7 +635,7 @@ method _read_blame {fd cur_w cur_d} {\n>  \n>  \t$cur_w conf -state normal\n>  \twhile {[gets $fd line] >= 0} {\n> -\t\tif {[regexp {^([a-z0-9]{40}) (\\d+) (\\d+) (\\d+)$} $line line \\\n> +\t\tif {[regexp {^([a-z0-9]{40}(?:[0-9a-f]{24})?) (\\d+) (\\d+) (\\d+)$} $line line \\\n\nSince we already have oid_size, why not use that to generate the regular \nexpression? That would make it much easier to add another hash of a \ndifferent length, and make the regex easier to understand.\n\nYou can replace this with:\n\n\tregexp \"^(\\[a-z0-9\\]{$oid_size}) (\\\\d+) (\\\\d+) (\\\\d+)$\"\n\nAnd since backslashes for escaping special string characters like '[', \nwhich can make the regex harder to read, you can use\n\n\tset exp [subst -nocommands -nobackslashes \\\n\t\t{^([a-z0-9]{$oid_size}) (\\d+) (\\d+) (\\d+)$}]\n\n>  \t\t\tcmit original_line final_line line_count]} {\n>  \t\t\tset r_commit     $cmit\n>  \t\t\tset r_orig_line  $original_line\n> @@ -648,7 +648,7 @@ method _read_blame {fd cur_w cur_d} {\n>  \t\t\tset oln  $r_orig_line\n>  \t\t\tset cmit $r_commit\n>  \n> -\t\t\tif {[regexp {^0{40}$} $cmit]} {\n> +\t\t\tif {[regexp {^0{40}(?:0{24})?$} $cmit]} {\n>  \t\t\t\tset commit_abbr work\n>  \t\t\t\tset commit_type curr_commit\n>  \t\t\t} elseif {$cmit eq $commit} {\n> diff --git a/lib/choose_repository.tcl b/lib/choose_repository.tcl\n> index af1fee7..e864f38 100644\n> --- a/lib/choose_repository.tcl\n> +++ b/lib/choose_repository.tcl\n> @@ -904,7 +904,7 @@ method _do_clone_full_end {ok} {\n>  \t\tif {[file exists [gitdir FETCH_HEAD]]} {\n>  \t\t\tset fd [open [gitdir FETCH_HEAD] r]\n>  \t\t\twhile {[gets $fd line] >= 0} {\n> -\t\t\t\tif {[regexp \"^(.{40})\\t\\t\" $line line HEAD]} {\n> +\t\t\t\tif {[regexp \"^([0-9a-fA-F]{40}(?:[0-9a-fA-F]{24})?)\\t\\t\" $line line HEAD]} {\n>  \t\t\t\t\tbreak\n>  \t\t\t\t}\n>  \t\t\t}\n> diff --git a/lib/remote_branch_delete.tcl b/lib/remote_branch_delete.tcl\n> index 5ba9fca..57bae9c 100644\n> --- a/lib/remote_branch_delete.tcl\n> +++ b/lib/remote_branch_delete.tcl\n> @@ -330,7 +330,7 @@ method _read {cache fd} {\n>  \n>  \twhile {[gets $fd line] >= 0} {\n>  \t\tif {[string match {*^{}} $line]} continue\n> -\t\tif {[regexp {^([0-9a-f]{40})\t(.*)$} $line _junk obj ref]} {\n> +\t\tif {[regexp {^([0-9a-fA-F]{40}(?:[0-9a-fA-F]{24})?)\t(.*)$} $line _junk obj ref]} {\n>  \t\t\tif {[regsub ^refs/heads/ $ref {} abr]} {\n>  \t\t\t\tlappend head_list $abr\n>  \t\t\t\tlappend head_cache($cache) $abr\n> -- \n> 2.33.0.1081.g099423f5b7\n> \n\n-- \nRegards,\nPratyush Yadav\n"},{"id":"441055","messageId":"20211113080435.54vs6ihljtkwcpe4@yadavpratyush.com","threadId":"56684","inReplyTo":"20211011121757.627-5-carenas@gmail.com","subject":"Re: [RFC PATCH 4/4] track oid_size to allow for checks that are hash agnostic","fromName":"Pratyush Yadav","fromEmail":"me@yadavpratyush.com","sentAt":"2021-11-13T08:04:35Z","receivedAt":"2021-11-13T08:06:25Z","isPatch":true,"sender":{"key":"me@yadavpratyush.com","avatar":"https://avatars.githubusercontent.com/u/8817931?v=4"},"body":"On 11/10/21 05:17AM, Carlo Marcelo Arenas Belón wrote:\n> This allows commit to work.\n\nPlease explain _why_ it allows commit to work.\n\n> \n> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>\n> ---\n>  git-gui.sh     | 5 +++--\n>  lib/commit.tcl | 3 ++-\n>  2 files changed, 5 insertions(+), 3 deletions(-)\n> \n> diff --git a/git-gui.sh b/git-gui.sh\n> index c0dc8ce..1646124 100755\n> --- a/git-gui.sh\n> +++ b/git-gui.sh\n> @@ -1821,10 +1821,11 @@ proc short_path {path} {\n>  \n>  set next_icon_id 0\n>  if { [get_config extensions.objectformat] eq \"sha256\" } {\n> -\tset null_oid [string repeat 0 64]\n> +\tset oid_size 64\n>  } else {\n> -\tset null_oid [string repeat 0 40]\n> +\tset oid_size 40\n>  }\n> +set null_oid [string repeat 0 $oid_size]\n>  \n>  proc merge_state {path new_state {head_info {}} {index_info {}}} {\n>  \tglobal file_states next_icon_id null_oid\n> diff --git a/lib/commit.tcl b/lib/commit.tcl\n> index 11379f8..1306e8d 100644\n> --- a/lib/commit.tcl\n> +++ b/lib/commit.tcl\n> @@ -337,6 +337,7 @@ proc commit_committree {fd_wt curHEAD msg_p} {\n>  \tglobal file_states selected_paths rescan_active\n>  \tglobal repo_config\n>  \tglobal env\n> +\tglobal oid_size\n>  \n>  \tgets $fd_wt tree_id\n>  \tif {[catch {close $fd_wt} err]} {\n> @@ -356,7 +357,7 @@ proc commit_committree {fd_wt curHEAD msg_p} {\n>  \t\tclose $fd_ot\n>  \n>  \t\tif {[string equal -length 5 {tree } $old_tree]\n> -\t\t\t&& [string length $old_tree] == 45} {\n> +\t\t\t&& [string length $old_tree] == 5 + oid_size} {\n                                           ^ missing '$'\n\nI think you forgot to test this one ;-)\n\n>  \t\t\tset old_tree [string range $old_tree 5 end]\n>  \t\t} else {\n>  \t\t\terror [mc \"Commit %s appears to be corrupt\" $PARENT]\n> -- \n> 2.33.0.1081.g099423f5b7\n> \n\n-- \nRegards,\nPratyush Yadav\n"},{"id":"441057","messageId":"20211113080858.2cjsd672eh4psdyu@yadavpratyush.com","threadId":"56684","inReplyTo":"20211011121757.627-1-carenas@gmail.com","subject":"Re: [RFC PATCH 0/4] git-gui: support SHA-256 repositories","fromName":"Pratyush Yadav","fromEmail":"me@yadavpratyush.com","sentAt":"2021-11-13T08:08:58Z","receivedAt":"2021-11-13T08:09:05Z","isPatch":true,"sender":{"key":"me@yadavpratyush.com","avatar":"https://avatars.githubusercontent.com/u/8817931?v=4"},"body":"Hi Carlo,\n\nOn 11/10/21 05:17AM, Carlo Marcelo Arenas Belón wrote:\n> While poking a SHA-256 hash repository, was surprised to find gitk\n> would fail with a fatal error when called, hence this series.\n> \n> Sending as an RFC, since I am not a git-gui or gitk user, and so\n> while this fixes the original issue and allows me to call gitk to\n> see the branch merge history (which is usually as much as I do with\n> it), it is likey missing some changes, as most of them where found\n> by lightly poking at all of the gui menus (except for remote or tool)\n\nThanks for the patches, and sorry for taking so long to review these.\n\nThe changes you sent look good to me for the most part, apart from a few \ncomments I made on the patches. I haven't looked too deeply into other \nplaces that might need updating but the basic functionality seems to \nwork fine for me.\n\n> \n> It could also be reordered to reduce unnecessary churn and of course\n> also needs the gitk change[1] that was sent independently, and better\n> commit messages.\n> \n> [1] https://lore.kernel.org/git/20211011114723.204-1-carenas@gmail.com/\n> \n> Carlo Marcelo Arenas Belón (4):\n>   blame: prefer null_sha1 over nullid and retire later\n>   rename all *_sha1 variables and make null_oid hash aware\n>   expand regexp matching an oid to be hash agnostic\n>   track oid_size to allow for checks that are hash agnostic\n> \n>  git-gui.sh                   | 30 ++++++++++++++++--------------\n>  lib/blame.tcl                | 18 +++++++++---------\n>  lib/checkout_op.tcl          |  4 ++--\n>  lib/choose_repository.tcl    |  2 +-\n>  lib/commit.tcl               |  3 ++-\n>  lib/remote_branch_delete.tcl |  2 +-\n>  6 files changed, 31 insertions(+), 28 deletions(-)\n> \n> -- \n> 2.33.0.1081.g099423f5b7\n> \n\n-- \nRegards,\nPratyush Yadav\n"},{"id":"441058","messageId":"20211113081032.rz6ibhf5qfv27ca3@yadavpratyush.com","threadId":"56684","inReplyTo":"20211113080435.54vs6ihljtkwcpe4@yadavpratyush.com","subject":"Re: [RFC PATCH 4/4] track oid_size to allow for checks that are hash agnostic","fromName":"Pratyush Yadav","fromEmail":"me@yadavpratyush.com","sentAt":"2021-11-13T08:10:32Z","receivedAt":"2021-11-13T08:10:38Z","isPatch":true,"sender":{"key":"me@yadavpratyush.com","avatar":"https://avatars.githubusercontent.com/u/8817931?v=4"},"body":"On 13/11/21 01:34PM, Pratyush Yadav wrote:\n> On 11/10/21 05:17AM, Carlo Marcelo Arenas Belón wrote:\n> > This allows commit to work.\n> \n> Please explain _why_ it allows commit to work.\n> \n> > \n> > Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>\n> > ---\n> >  git-gui.sh     | 5 +++--\n> >  lib/commit.tcl | 3 ++-\n> >  2 files changed, 5 insertions(+), 3 deletions(-)\n> > \n> > diff --git a/git-gui.sh b/git-gui.sh\n> > index c0dc8ce..1646124 100755\n> > --- a/git-gui.sh\n> > +++ b/git-gui.sh\n> > @@ -1821,10 +1821,11 @@ proc short_path {path} {\n> >  \n> >  set next_icon_id 0\n> >  if { [get_config extensions.objectformat] eq \"sha256\" } {\n> > -\tset null_oid [string repeat 0 64]\n> > +\tset oid_size 64\n> >  } else {\n> > -\tset null_oid [string repeat 0 40]\n> > +\tset oid_size 40\n> >  }\n> > +set null_oid [string repeat 0 $oid_size]\n> >  \n> >  proc merge_state {path new_state {head_info {}} {index_info {}}} {\n> >  \tglobal file_states next_icon_id null_oid\n> > diff --git a/lib/commit.tcl b/lib/commit.tcl\n> > index 11379f8..1306e8d 100644\n> > --- a/lib/commit.tcl\n> > +++ b/lib/commit.tcl\n> > @@ -337,6 +337,7 @@ proc commit_committree {fd_wt curHEAD msg_p} {\n> >  \tglobal file_states selected_paths rescan_active\n> >  \tglobal repo_config\n> >  \tglobal env\n> > +\tglobal oid_size\n> >  \n> >  \tgets $fd_wt tree_id\n> >  \tif {[catch {close $fd_wt} err]} {\n> > @@ -356,7 +357,7 @@ proc commit_committree {fd_wt curHEAD msg_p} {\n> >  \t\tclose $fd_ot\n> >  \n> >  \t\tif {[string equal -length 5 {tree } $old_tree]\n> > -\t\t\t&& [string length $old_tree] == 45} {\n> > +\t\t\t&& [string length $old_tree] == 5 + oid_size} {\n>                                            ^ missing '$'\n\nI think different tab sizes might end up rendering the ^ in a differnt \nplace. So to clarify: Missing '$' before oid_size.\n\n> \n> I think you forgot to test this one ;-)\n> \n> >  \t\t\tset old_tree [string range $old_tree 5 end]\n> >  \t\t} else {\n> >  \t\t\terror [mc \"Commit %s appears to be corrupt\" $PARENT]\n> > -- \n> > 2.33.0.1081.g099423f5b7\n> > \n> \n> -- \n> Regards,\n> Pratyush Yadav\n\n-- \nRegards,\nPratyush Yadav\n"}]}