Re: [PATCH/RFC v2 2/2] diff.tcl: call "apply_tab_size 1" to fix alignment instead of spaces.

[Junio C Hamano <gitster@pobox.com>]

"Chris Idema via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Chris Idema <github_chris_idema@proton.me>
>
> Signed-off-by: Chris Idema <github_chris_idema@proton.me>
> ---
>  git-gui/lib/diff.tcl | 24 +-----------------------
>  1 file changed, 1 insertion(+), 23 deletions(-)
>
> diff --git a/git-gui/lib/diff.tcl b/git-gui/lib/diff.tcl
> index 2e13f8c776..0f0951cc57 100644
> --- a/git-gui/lib/diff.tcl
> +++ b/git-gui/lib/diff.tcl
> @@ -12,27 +12,6 @@ proc apply_tab_size {{firsttab {}}} {
>  	}
>  }
>  
> -proc expand_tabs {line {startcol -1}} {
> -	# startcol set to -1, because in preview the lines start with a '+', '-', or ' '
> -	global repo_config
> -
> -	set col $startcol
> -	set out ""
> -
> -	foreach char [split $line ""] {
> -		if {$char eq "\t"} {
> -			set spaces [expr {$repo_config(gui.tabsize) - ($col % $repo_config(gui.tabsize))}]
> -			append out [string repeat " " $spaces]
> -			incr col $spaces
> -		} else {
> -			append out $char
> -			incr col
> -		}
> -	}
> -
> -	return $out
> -}
> -
>  proc clear_diff {} {
>  	global ui_diff current_diff_path current_diff_header
>  	global ui_index ui_workdir
> @@ -516,9 +495,8 @@ proc read_diff {fd conflict_size cont_info} {
>  			}
>  		}
>  		set mark [$ui_diff index "end - 1 line linestart"]
> -		set line [expand_tabs $line]
> +		apply_tab_size 1
>  		$ui_diff insert end "$line" $tags
> -

Why does this series first add proc expand_tabs, only to remove its
use in this second step?  Shouldn't these two patches be squashed
into one, and explain why we want to use "apply_tab_size 1" here?

It smells fishy to do "apply_tab_size 1" here in "proc clear_diff".

It is called from "proc show_diff" but the latter, after it calls
clear_diff, calls "apply_tab_size 0".  Doesn't that defeat the
effect of this new call added to "proc clear_diff"?

By the way, this has nothing to do with your change, but the only
existing use of "apply_tab_size 1" is also somewhat curious.  When
"proc read_diff" detects that a patch hunk header has three (not the
usual two) at-signs, it calls "apply_tab_size 1", presumably to
adjust to the fact that combined diff has two leading columns used
to signal added/removed/context lines, instead of one.

Apparently the author of the original code thought that it is a good
idea for such a payload if first tab moves 1 column, and second and
subsequent tabs taking gui.tabsize after that tabstop.  A line in
combined diff uses two leading columns for line prefix.  Isn't it
curious that these two patches under discussion claim that the same
exact setting of "apply_tab_size 1" is appropriate for _anything_
that is shown in the $ui_diff widget prepared with "proc
clear_diff"?

Presumably most of the time, the output format would use just a
single leading column for line prefix added (+), removed (-), or
context ( ).

Both cannot be correct at the same time, can they?

So, either the original author is wrong and the current code is
broken with or without your change when it shows a combined diff, or
these patches is wrong and there is off-by-one bug somwhere.

Puzzled and curious ...