Re: [PATCH/RFC v2 2/2] diff.tcl: call "apply_tab_size 1" to fix alignment instead of spaces.
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jan 27, 2026, 22:19 UTC
- Message-ID
- <xmqqsebqem1n.fsf@gitster.g>
- In-Reply-To
- <e11aa6d811dcf868fd0f91b74cdceb8bc3f4229e.1769545996.git.gitgitgadget@gmail.com>
"Chris Idema via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 47 quoted lines
> 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 ...