[RFC PATCH 0/4] git-gui: support SHA-256 repositories

[RFC PATCH 0/4] git-gui: support SHA-256 repositories

[Carlo Marcelo Arenas Belón]

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(-)

-- 
2.33.0.1081.g099423f5b7

[RFC PATCH 1/4] blame: prefer null_sha1 over nullid and retire later

[Carlo Marcelo Arenas Belón]

a9786bb (git-gui: Fix Blame Parent & Context for working copy lines.,
2008-09-08) adds nullid (and a never used nullid2) for matching locally
modified lines in blame.

Use instead the already available null_sha1 for the same and in
preparation to making that hash independent on a future patch.

Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
 git-gui.sh    |  3 ---
 lib/blame.tcl | 10 +++++-----
 2 files changed, 5 insertions(+), 8 deletions(-)

diff --git a/git-gui.sh b/git-gui.sh
index 201524c..a69b0fe 100755
--- a/git-gui.sh
+++ b/git-gui.sh
@@ -1353,9 +1353,6 @@ set diff_empty_count 0
 set last_revert {}
 set last_revert_enc {}
 
-set nullid "0000000000000000000000000000000000000000"
-set nullid2 "0000000000000000000000000000000000000001"
-
 ######################################################################
 ##
 ## task management
diff --git a/lib/blame.tcl b/lib/blame.tcl
index 8441e10..6ece79d 100644
--- a/lib/blame.tcl
+++ b/lib/blame.tcl
@@ -1056,14 +1056,14 @@ method _format_offset_date {base offset} {
 }
 
 method _gitkcommit {} {
-	global nullid
+	global null_sha1
 
 	set dat [_get_click_amov_info $this]
 	if {$dat ne {}} {
 		set cmit [lindex $dat 0]
 
 		# If the line belongs to the working copy, use HEAD instead
-		if {$cmit eq $nullid} {
+		if {$cmit eq $null_sha1} {
 			if {[catch {set cmit [git rev-parse --verify HEAD]} err]} {
 				error_popup [strcat [mc "Cannot find HEAD commit:"] "\n\n$err"]
 				return;
@@ -1106,7 +1106,7 @@ method _gitkcommit {} {
 }
 
 method _blameparent {} {
-	global nullid
+	global null_sha1
 
 	set dat [_get_click_amov_info $this]
 	if {$dat ne {}} {
@@ -1114,7 +1114,7 @@ method _blameparent {} {
 		set new_path [lindex $dat 1]
 
 		# Allow using Blame Parent on lines modified in the working copy
-		if {$cmit eq $nullid} {
+		if {$cmit eq $null_sha1} {
 			set parent_ref "HEAD"
 		} else {
 			set parent_ref "$cmit^"
@@ -1129,7 +1129,7 @@ method _blameparent {} {
 		# Generate a diff between the commit and its parent,
 		# and use the hunks to update the line number.
 		# Request zero context to simplify calculations.
-		if {$cmit eq $nullid} {
+		if {$cmit eq $null_sha1} {
 			set diffcmd [list diff-index --unified=0 $cparent -- $new_path]
 		} else {
 			set diffcmd [list diff-tree --unified=0 $cparent $cmit -- $new_path]
-- 
2.33.0.1081.g099423f5b7

Re: [RFC PATCH 1/4] blame: prefer null_sha1 over nullid and retire later

[Pratyush Yadav]

On 11/10/21 05:17AM, Carlo Marcelo Arenas Belón wrote:
> a9786bb (git-gui: Fix Blame Parent & Context for working copy lines.,
> 2008-09-08) adds nullid (and a never used nullid2) for matching locally
> modified lines in blame.
> 
> Use instead the already available null_sha1 for the same and in
> preparation to making that hash independent on a future patch.

LGTM.

-- 
Regards,
Pratyush Yadav

[RFC PATCH 2/4] rename all *_sha1 variables and make null_oid hash aware

[Carlo Marcelo Arenas Belón]

Before this change, creating a branch in an SHA-256 repository would
fail because the null_sha1 used was of the wrong size.

Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
 git-gui.sh          | 26 +++++++++++++++-----------
 lib/blame.tcl       | 10 +++++-----
 lib/checkout_op.tcl |  4 ++--
 3 files changed, 22 insertions(+), 18 deletions(-)

diff --git a/git-gui.sh b/git-gui.sh
index a69b0fe..c0dc8ce 100755
--- a/git-gui.sh
+++ b/git-gui.sh
@@ -1820,10 +1820,14 @@ proc short_path {path} {
 }
 
 set next_icon_id 0
-set null_sha1 [string repeat 0 40]
+if { [get_config extensions.objectformat] eq "sha256" } {
+	set null_oid [string repeat 0 64]
+} else {
+	set null_oid [string repeat 0 40]
+}
 
 proc merge_state {path new_state {head_info {}} {index_info {}}} {
-	global file_states next_icon_id null_sha1
+	global file_states next_icon_id null_oid
 
 	set s0 [string index $new_state 0]
 	set s1 [string index $new_state 1]
@@ -1845,7 +1849,7 @@ proc merge_state {path new_state {head_info {}} {index_info {}}} {
 	elseif {$s1 eq {_}} {set s1 _}
 
 	if {$s0 eq {A} && $s1 eq {_} && $head_info eq {}} {
-		set head_info [list 0 $null_sha1]
+		set head_info [list 0 $null_oid]
 	} elseif {$s0 ne {_} && [string index $state 0] eq {_}
 		&& $head_info eq {}} {
 		set head_info $index_info
@@ -2179,21 +2183,21 @@ proc do_gitk {revs {is_submodule false}} {
 			cd $current_diff_path
 			if {$revs eq {--}} {
 				set s $file_states($current_diff_path)
-				set old_sha1 {}
-				set new_sha1 {}
+				set old_oid {}
+				set new_oid {}
 				switch -glob -- [lindex $s 0] {
-				M_ { set old_sha1 [lindex [lindex $s 2] 1] }
-				_M { set old_sha1 [lindex [lindex $s 3] 1] }
+				M_ { set old_oid [lindex [lindex $s 2] 1] }
+				_M { set old_oid [lindex [lindex $s 3] 1] }
 				MM {
 					if {$current_diff_side eq $ui_index} {
-						set old_sha1 [lindex [lindex $s 2] 1]
-						set new_sha1 [lindex [lindex $s 3] 1]
+						set old_oid [lindex [lindex $s 2] 1]
+						set new_oid [lindex [lindex $s 3] 1]
 					} else {
-						set old_sha1 [lindex [lindex $s 3] 1]
+						set old_oid [lindex [lindex $s 3] 1]
 					}
 				}
 				}
-				set revs $old_sha1...$new_sha1
+				set revs $old_oid...$new_oid
 			}
 			# GIT_DIR and GIT_WORK_TREE for the submodule are not the ones
 			# we've been using for the main repository, so unset them.
diff --git a/lib/blame.tcl b/lib/blame.tcl
index 6ece79d..e6d4302 100644
--- a/lib/blame.tcl
+++ b/lib/blame.tcl
@@ -1056,14 +1056,14 @@ method _format_offset_date {base offset} {
 }
 
 method _gitkcommit {} {
-	global null_sha1
+	global null_oid
 
 	set dat [_get_click_amov_info $this]
 	if {$dat ne {}} {
 		set cmit [lindex $dat 0]
 
 		# If the line belongs to the working copy, use HEAD instead
-		if {$cmit eq $null_sha1} {
+		if {$cmit eq $null_oid} {
 			if {[catch {set cmit [git rev-parse --verify HEAD]} err]} {
 				error_popup [strcat [mc "Cannot find HEAD commit:"] "\n\n$err"]
 				return;
@@ -1106,7 +1106,7 @@ method _gitkcommit {} {
 }
 
 method _blameparent {} {
-	global null_sha1
+	global null_oid
 
 	set dat [_get_click_amov_info $this]
 	if {$dat ne {}} {
@@ -1114,7 +1114,7 @@ method _blameparent {} {
 		set new_path [lindex $dat 1]
 
 		# Allow using Blame Parent on lines modified in the working copy
-		if {$cmit eq $null_sha1} {
+		if {$cmit eq $null_oid} {
 			set parent_ref "HEAD"
 		} else {
 			set parent_ref "$cmit^"
@@ -1129,7 +1129,7 @@ method _blameparent {} {
 		# Generate a diff between the commit and its parent,
 		# and use the hunks to update the line number.
 		# Request zero context to simplify calculations.
-		if {$cmit eq $null_sha1} {
+		if {$cmit eq $null_oid} {
 			set diffcmd [list diff-index --unified=0 $cparent -- $new_path]
 		} else {
 			set diffcmd [list diff-tree --unified=0 $cparent $cmit -- $new_path]
diff --git a/lib/checkout_op.tcl b/lib/checkout_op.tcl
index 21ea768..be1ebba 100644
--- a/lib/checkout_op.tcl
+++ b/lib/checkout_op.tcl
@@ -151,7 +151,7 @@ method _finish_fetch {ok} {
 }
 
 method _update_ref {} {
-	global null_sha1 current_branch repo_config
+	global null_oid current_branch repo_config
 
 	set ref $new_ref
 	set new $new_hash
@@ -177,7 +177,7 @@ method _update_ref {} {
 		}
 
 		set reflog_msg "branch: Created from $new_expr"
-		set cur $null_sha1
+		set cur $null_oid
 
 		if {($repo_config(branch.autosetupmerge) eq {true}
 			|| $repo_config(branch.autosetupmerge) eq {always})
-- 
2.33.0.1081.g099423f5b7

Re: [RFC PATCH 2/4] rename all *_sha1 variables and make null_oid hash aware

[Eric Sunshine]

On Mon, Oct 11, 2021 at 8:18 AM Carlo Marcelo Arenas Belón
<carenas@gmail.com> wrote:
> Before this change, creating a branch in an SHA-256 repository would
> fail because the null_sha1 used was of the wrong size.
>
> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
> ---
> diff --git a/git-gui.sh b/git-gui.sh
> @@ -1820,10 +1820,14 @@ proc short_path {path} {
> +if { [get_config extensions.objectformat] eq "sha256" } {
> +       set null_oid [string repeat 0 64]
> +} else {
> +       set null_oid [string repeat 0 40]
> +}

Should this be using:

    git rev-parse --show-object-format

rather than reading the configuration directly?

Re: [RFC PATCH 2/4] rename all *_sha1 variables and make null_oid hash aware

[Pratyush Yadav]

On 11/10/21 05:17AM, Carlo Marcelo Arenas Belón wrote:
> Before this change, creating a branch in an SHA-256 repository would
> fail because the null_sha1 used was of the wrong size.
> 
> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
> ---
>  git-gui.sh          | 26 +++++++++++++++-----------
>  lib/blame.tcl       | 10 +++++-----
>  lib/checkout_op.tcl |  4 ++--
>  3 files changed, 22 insertions(+), 18 deletions(-)
> 
> diff --git a/git-gui.sh b/git-gui.sh
> index a69b0fe..c0dc8ce 100755
> --- a/git-gui.sh
> +++ b/git-gui.sh
> @@ -1820,10 +1820,14 @@ proc short_path {path} {
>  }
>  
>  set next_icon_id 0
> -set null_sha1 [string repeat 0 40]
> +if { [get_config extensions.objectformat] eq "sha256" } {

From the docs I see that this feature is experimental as of now and 
might change in the future. Can we expect this config option to stay 
stable over time? If not I think this might be too early to introduce it 
into git-gui.

Anyway, nitpick: don't add spaces after opening brace and before closing 
brace.

> +	set null_oid [string repeat 0 64]
> +} else {
> +	set null_oid [string repeat 0 40]
> +}
>  
>  proc merge_state {path new_state {head_info {}} {index_info {}}} {
> -	global file_states next_icon_id null_sha1
> +	global file_states next_icon_id null_oid
>  
>  	set s0 [string index $new_state 0]
>  	set s1 [string index $new_state 1]

Rest of the patch looks good to me. Thanks.

-- 
Regards,
Pratyush Yadav

[RFC PATCH 3/4] expand regexp matching an oid to be hash agnostic

[Carlo Marcelo Arenas Belón]

Before this change, listing or blame will fail as it couldn't find the
OID in an SHA-256 repository.

Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
 lib/blame.tcl                | 8 ++++----
 lib/choose_repository.tcl    | 2 +-
 lib/remote_branch_delete.tcl | 2 +-
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/lib/blame.tcl b/lib/blame.tcl
index e6d4302..ee7db9d 100644
--- a/lib/blame.tcl
+++ b/lib/blame.tcl
@@ -436,7 +436,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 {^g[0-9a-f]{40}(?:[0-9a-f]{24})?$} $g]} {
 					$i tag delete $g
 				}
 			}
@@ -513,7 +513,7 @@ method _history_menu {} {
 		set c [lindex $e 0]
 		set f [lindex $e 1]
 
-		if {[regexp {^[0-9a-f]{40}$} $c]} {
+		if {[regexp {^[0-9a-f]{40}(?:[0-9a-f]{24})?$} $c]} {
 			set t [string range $c 0 8]...
 		} elseif {$c eq {}} {
 			set t {Working Directory}
@@ -635,7 +635,7 @@ method _read_blame {fd cur_w cur_d} {
 
 	$cur_w conf -state normal
 	while {[gets $fd line] >= 0} {
-		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 \
 			cmit original_line final_line line_count]} {
 			set r_commit     $cmit
 			set r_orig_line  $original_line
@@ -648,7 +648,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 {^0{40}(?:0{24})?$} $cmit]} {
 				set commit_abbr work
 				set commit_type curr_commit
 			} elseif {$cmit eq $commit} {
diff --git a/lib/choose_repository.tcl b/lib/choose_repository.tcl
index af1fee7..e864f38 100644
--- a/lib/choose_repository.tcl
+++ b/lib/choose_repository.tcl
@@ -904,7 +904,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 "^([0-9a-fA-F]{40}(?:[0-9a-fA-F]{24})?)\t\t" $line line HEAD]} {
 					break
 				}
 			}
diff --git a/lib/remote_branch_delete.tcl b/lib/remote_branch_delete.tcl
index 5ba9fca..57bae9c 100644
--- a/lib/remote_branch_delete.tcl
+++ b/lib/remote_branch_delete.tcl
@@ -330,7 +330,7 @@ method _read {cache fd} {
 
 	while {[gets $fd line] >= 0} {
 		if {[string match {*^{}} $line]} continue
-		if {[regexp {^([0-9a-f]{40})	(.*)$} $line _junk obj ref]} {
+		if {[regexp {^([0-9a-fA-F]{40}(?:[0-9a-fA-F]{24})?)	(.*)$} $line _junk obj ref]} {
 			if {[regsub ^refs/heads/ $ref {} abr]} {
 				lappend head_list $abr
 				lappend head_cache($cache) $abr
-- 
2.33.0.1081.g099423f5b7

Re: [RFC PATCH 3/4] expand regexp matching an oid to be hash agnostic

[Pratyush Yadav]

On 11/10/21 05:17AM, Carlo Marcelo Arenas Belón wrote:
> Before this change, listing or blame will fail as it couldn't find the
> OID in an SHA-256 repository.
> 
> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
> ---
>  lib/blame.tcl                | 8 ++++----
>  lib/choose_repository.tcl    | 2 +-
>  lib/remote_branch_delete.tcl | 2 +-
>  3 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/lib/blame.tcl b/lib/blame.tcl
> index e6d4302..ee7db9d 100644
> --- a/lib/blame.tcl
> +++ b/lib/blame.tcl
> @@ -436,7 +436,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 {^g[0-9a-f]{40}(?:[0-9a-f]{24})?$} $g]} {
>  					$i tag delete $g
>  				}
>  			}
> @@ -513,7 +513,7 @@ method _history_menu {} {
>  		set c [lindex $e 0]
>  		set f [lindex $e 1]
>  
> -		if {[regexp {^[0-9a-f]{40}$} $c]} {
> +		if {[regexp {^[0-9a-f]{40}(?:[0-9a-f]{24})?$} $c]} {
>  			set t [string range $c 0 8]...
>  		} elseif {$c eq {}} {
>  			set t {Working Directory}
> @@ -635,7 +635,7 @@ method _read_blame {fd cur_w cur_d} {
>  
>  	$cur_w conf -state normal
>  	while {[gets $fd line] >= 0} {
> -		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 \

Since we already have oid_size, why not use that to generate the regular 
expression? That would make it much easier to add another hash of a 
different length, and make the regex easier to understand.

You can replace this with:

	regexp "^(\[a-z0-9\]{$oid_size}) (\\d+) (\\d+) (\\d+)$"

And since backslashes for escaping special string characters like '[', 
which can make the regex harder to read, you can use

	set exp [subst -nocommands -nobackslashes \
		{^([a-z0-9]{$oid_size}) (\d+) (\d+) (\d+)$}]

>  			cmit original_line final_line line_count]} {
>  			set r_commit     $cmit
>  			set r_orig_line  $original_line
> @@ -648,7 +648,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 {^0{40}(?:0{24})?$} $cmit]} {
>  				set commit_abbr work
>  				set commit_type curr_commit
>  			} elseif {$cmit eq $commit} {
> diff --git a/lib/choose_repository.tcl b/lib/choose_repository.tcl
> index af1fee7..e864f38 100644
> --- a/lib/choose_repository.tcl
> +++ b/lib/choose_repository.tcl
> @@ -904,7 +904,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 "^([0-9a-fA-F]{40}(?:[0-9a-fA-F]{24})?)\t\t" $line line HEAD]} {
>  					break
>  				}
>  			}
> diff --git a/lib/remote_branch_delete.tcl b/lib/remote_branch_delete.tcl
> index 5ba9fca..57bae9c 100644
> --- a/lib/remote_branch_delete.tcl
> +++ b/lib/remote_branch_delete.tcl
> @@ -330,7 +330,7 @@ method _read {cache fd} {
>  
>  	while {[gets $fd line] >= 0} {
>  		if {[string match {*^{}} $line]} continue
> -		if {[regexp {^([0-9a-f]{40})	(.*)$} $line _junk obj ref]} {
> +		if {[regexp {^([0-9a-fA-F]{40}(?:[0-9a-fA-F]{24})?)	(.*)$} $line _junk obj ref]} {
>  			if {[regsub ^refs/heads/ $ref {} abr]} {
>  				lappend head_list $abr
>  				lappend head_cache($cache) $abr
> -- 
> 2.33.0.1081.g099423f5b7
> 

-- 
Regards,
Pratyush Yadav

[RFC PATCH 4/4] track oid_size to allow for checks that are hash agnostic

[Carlo Marcelo Arenas Belón]

This allows commit to work.

Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
 git-gui.sh     | 5 +++--
 lib/commit.tcl | 3 ++-
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/git-gui.sh b/git-gui.sh
index c0dc8ce..1646124 100755
--- a/git-gui.sh
+++ b/git-gui.sh
@@ -1821,10 +1821,11 @@ proc short_path {path} {
 
 set next_icon_id 0
 if { [get_config extensions.objectformat] eq "sha256" } {
-	set null_oid [string repeat 0 64]
+	set oid_size 64
 } else {
-	set null_oid [string repeat 0 40]
+	set oid_size 40
 }
+set null_oid [string repeat 0 $oid_size]
 
 proc merge_state {path new_state {head_info {}} {index_info {}}} {
 	global file_states next_icon_id null_oid
diff --git a/lib/commit.tcl b/lib/commit.tcl
index 11379f8..1306e8d 100644
--- a/lib/commit.tcl
+++ b/lib/commit.tcl
@@ -337,6 +337,7 @@ proc commit_committree {fd_wt curHEAD msg_p} {
 	global file_states selected_paths rescan_active
 	global repo_config
 	global env
+	global oid_size
 
 	gets $fd_wt tree_id
 	if {[catch {close $fd_wt} err]} {
@@ -356,7 +357,7 @@ proc commit_committree {fd_wt curHEAD msg_p} {
 		close $fd_ot
 
 		if {[string equal -length 5 {tree } $old_tree]
-			&& [string length $old_tree] == 45} {
+			&& [string length $old_tree] == 5 + oid_size} {
 			set old_tree [string range $old_tree 5 end]
 		} else {
 			error [mc "Commit %s appears to be corrupt" $PARENT]
-- 
2.33.0.1081.g099423f5b7

Re: [RFC PATCH 4/4] track oid_size to allow for checks that are hash agnostic

[Pratyush Yadav]

On 11/10/21 05:17AM, Carlo Marcelo Arenas Belón wrote:
> This allows commit to work.

Please explain _why_ it allows commit to work.

> 
> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
> ---
>  git-gui.sh     | 5 +++--
>  lib/commit.tcl | 3 ++-
>  2 files changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/git-gui.sh b/git-gui.sh
> index c0dc8ce..1646124 100755
> --- a/git-gui.sh
> +++ b/git-gui.sh
> @@ -1821,10 +1821,11 @@ proc short_path {path} {
>  
>  set next_icon_id 0
>  if { [get_config extensions.objectformat] eq "sha256" } {
> -	set null_oid [string repeat 0 64]
> +	set oid_size 64
>  } else {
> -	set null_oid [string repeat 0 40]
> +	set oid_size 40
>  }
> +set null_oid [string repeat 0 $oid_size]
>  
>  proc merge_state {path new_state {head_info {}} {index_info {}}} {
>  	global file_states next_icon_id null_oid
> diff --git a/lib/commit.tcl b/lib/commit.tcl
> index 11379f8..1306e8d 100644
> --- a/lib/commit.tcl
> +++ b/lib/commit.tcl
> @@ -337,6 +337,7 @@ proc commit_committree {fd_wt curHEAD msg_p} {
>  	global file_states selected_paths rescan_active
>  	global repo_config
>  	global env
> +	global oid_size
>  
>  	gets $fd_wt tree_id
>  	if {[catch {close $fd_wt} err]} {
> @@ -356,7 +357,7 @@ proc commit_committree {fd_wt curHEAD msg_p} {
>  		close $fd_ot
>  
>  		if {[string equal -length 5 {tree } $old_tree]
> -			&& [string length $old_tree] == 45} {
> +			&& [string length $old_tree] == 5 + oid_size} {
                                           ^ missing '$'

I think you forgot to test this one ;-)

>  			set old_tree [string range $old_tree 5 end]
>  		} else {
>  			error [mc "Commit %s appears to be corrupt" $PARENT]
> -- 
> 2.33.0.1081.g099423f5b7
> 

-- 
Regards,
Pratyush Yadav

Re: [RFC PATCH 4/4] track oid_size to allow for checks that are hash agnostic

[Pratyush Yadav]

On 13/11/21 01:34PM, Pratyush Yadav wrote:
> On 11/10/21 05:17AM, Carlo Marcelo Arenas Belón wrote:
> > This allows commit to work.
> 
> Please explain _why_ it allows commit to work.
> 
> > 
> > Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
> > ---
> >  git-gui.sh     | 5 +++--
> >  lib/commit.tcl | 3 ++-
> >  2 files changed, 5 insertions(+), 3 deletions(-)
> > 
> > diff --git a/git-gui.sh b/git-gui.sh
> > index c0dc8ce..1646124 100755
> > --- a/git-gui.sh
> > +++ b/git-gui.sh
> > @@ -1821,10 +1821,11 @@ proc short_path {path} {
> >  
> >  set next_icon_id 0
> >  if { [get_config extensions.objectformat] eq "sha256" } {
> > -	set null_oid [string repeat 0 64]
> > +	set oid_size 64
> >  } else {
> > -	set null_oid [string repeat 0 40]
> > +	set oid_size 40
> >  }
> > +set null_oid [string repeat 0 $oid_size]
> >  
> >  proc merge_state {path new_state {head_info {}} {index_info {}}} {
> >  	global file_states next_icon_id null_oid
> > diff --git a/lib/commit.tcl b/lib/commit.tcl
> > index 11379f8..1306e8d 100644
> > --- a/lib/commit.tcl
> > +++ b/lib/commit.tcl
> > @@ -337,6 +337,7 @@ proc commit_committree {fd_wt curHEAD msg_p} {
> >  	global file_states selected_paths rescan_active
> >  	global repo_config
> >  	global env
> > +	global oid_size
> >  
> >  	gets $fd_wt tree_id
> >  	if {[catch {close $fd_wt} err]} {
> > @@ -356,7 +357,7 @@ proc commit_committree {fd_wt curHEAD msg_p} {
> >  		close $fd_ot
> >  
> >  		if {[string equal -length 5 {tree } $old_tree]
> > -			&& [string length $old_tree] == 45} {
> > +			&& [string length $old_tree] == 5 + oid_size} {
>                                            ^ missing '$'

I think different tab sizes might end up rendering the ^ in a differnt 
place. So to clarify: Missing '$' before oid_size.

> 
> I think you forgot to test this one ;-)
> 
> >  			set old_tree [string range $old_tree 5 end]
> >  		} else {
> >  			error [mc "Commit %s appears to be corrupt" $PARENT]
> > -- 
> > 2.33.0.1081.g099423f5b7
> > 
> 
> -- 
> Regards,
> Pratyush Yadav

-- 
Regards,
Pratyush Yadav

Re: [RFC PATCH 0/4] git-gui: support SHA-256 repositories

[Ævar Arnfjörð Bjarmason]

On Mon, Oct 11 2021, Carlo Marcelo Arenas Belón wrote:

> 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 :)

Re: [RFC PATCH 0/4] git-gui: support SHA-256 repositories

[Carlo Arenas]

On Mon, Oct 11, 2021 at 7:21 AM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
>
> On Mon, Oct 11 2021, Carlo Marcelo Arenas Belón wrote:
>
> > [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/

This specific series is for git-gui, and the one posted before is for gitk,
but the code is still similar enough, and indeed the gitk part was
included in a reference.

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

It makes me nervous though to see checks like the one I fixed on
commit[1] that use logic to check the correct size of the SHA as an
implication of it being a valid value.

considering the code is very old, maybe that was relevant long ago?,
but agree some checks seem to be unnecessarily strict.

I have relaxed some of the checks in the gitk patch and will be
posting it soon, so hopefully reviews from people that know the code
better could be collected.

Carlo

[1] https://lore.kernel.org/git/20211011121757.627-5-carenas@gmail.com/

Re: [RFC PATCH 0/4] git-gui: support SHA-256 repositories

[Pratyush Yadav]

Hi Carlo,

On 11/10/21 05:17AM, Carlo Marcelo Arenas Belón wrote:
> 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)

Thanks for the patches, and sorry for taking so long to review these.

The changes you sent look good to me for the most part, apart from a few 
comments I made on the patches. I haven't looked too deeply into other 
places that might need updating but the basic functionality seems to 
work fine for me.

> 
> 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(-)
> 
> -- 
> 2.33.0.1081.g099423f5b7
> 

-- 
Regards,
Pratyush Yadav