gitk: handle long command-lines

These patches have been cooking for such a long time in Git for Windows that you might think they turned into broth. Yummy broth, to be sure. But broth. 'Tis beyond time for the patches to make it upstream.

Johannes Schindelin (1):
gitk: prevent overly long command lines

Nico Rieck (1):
gitk: escape file paths before piping to git log

 gitk | 36 +++++++++++++++++++++++++++++++-----
 1 file changed, 31 insertions(+), 5 deletions(-)

base-commit: 465f03869ae11acd04abfa1b83c67879c867410c
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1469%2Fdscho%2Fgitk-long-cmdline-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1469/dscho/gitk-long-cmdline-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1469

gitgitgadget

From: Johannes Schindelin <[email protected]>

To avoid running into command line limitations, some of Git's commands support the --stdin option.

Let's use exactly this option in the three rev-list/log invocations in gitk that would otherwise possibly run the danger of trying to invoke a too-long command line.

While it is easy to redirect either stdin or stdout in Tcl/Tk scripts, what we need here is both. We need to capture the output, yet we also need to pipe in the revs/files arguments via stdin (because stdin does not have any limit, unlike the command line). To help this, we use the neat Tcl feature where you can capture stdout and at the same time feed a fixed string as stdin to the spawned process.

One non-obvious aspect about this change is that the --stdin option allows to specify revs, the double-dash, and files, but *no* other options such as --not. This is addressed by prefixing the "negative" revs with ^ explicitly rather than relying on the --not option (thanks for coming up with that idea, Max!).

This fixes https://github.com/git-for-windows/git/issues/1987

---
 gitk | 24 +++++++++++++++++++-----
 1 file changed, 19 insertions(+), 5 deletions(-)

index 0ae7d685904..92375ca6a2a 100755
@@ -405,14 +405,16 @@ proc start_rev_list {view} {
         if {$revs eq {}} {
             return 0
         }
-        set args [concat $vflags($view) $revs]
+        set args $vflags($view)
     } else {
+        set revs {}
         set args $vorigargs($view)
     }
 
     if {[catch {
         set fd [open [concat | git log --no-color -z --pretty=raw $show_notes \
-                        --parents --boundary $args "--" $files] r]
+                        --parents --boundary $args --stdin \
+                        "<<[join [concat $revs "--" $files] "\\n"]"] r]
     } err]} {
         error_popup "[mc "Error executing git log:"] $err"
         return 0

@@ -554,13 +556,19 @@ proc updatecommits {} {
             set revs $newrevs
             set vposids($view) [lsort -unique [concat $oldpos $vposids($view)]]
         }
-        set args [concat $vflags($view) $revs --not $oldpos]
+        set args $vflags($view)
+        foreach r $oldpos {
+                lappend revs "^$r"
+        }
     } else {
+        set revs {}
         set args $vorigargs($view)
     }
     if {[catch {
         set fd [open [concat | git log --no-color -z --pretty=raw $show_notes \
-                        --parents --boundary $args "--" $vfilelimit($view)] r]
+                        --parents --boundary $args --stdin \
+                        "<<[join [concat $revs "--" \
+                                $vfilelimit($view)] "\\n"]"] r]
     } err]} {
         error_popup "[mc "Error executing git log:"] $err"
         return

@@ -10231,10 +10239,16 @@ proc getallcommits {} {
             foreach id $seeds {
                 lappend ids "^$id"
             }
+            lappend ids "--"
         }
     }
     if {$ids ne {}} {
-        set fd [open [concat $cmd $ids] r]
+        if {$ids eq "--all"} {
+            set cmd [concat $cmd "--all"]
+        } else {
+            set cmd [concat $cmd --stdin "<<[join $ids "\\n"]"]
+        }
+        set fd [open $cmd r]
         fconfigure $fd -blocking 0
         incr allcommits
         nowbusy allcommits

gitgitgadget

From: Nico Rieck <[email protected]>

We just started piping the file paths via stdin instead of passing them via the command-line, to avoid running into command-line limitations.

However, since we now pipe the file paths, we need to take care of special characters.

This fixes https://github.com/git-for-windows/git/issues/2293

---
 gitk | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

index 92375ca6a2a..df3ba2ea99b 100755
@@ -353,6 +353,16 @@ proc parseviewrevs {view revs} {
     return $ret
 }
 
+# Escapes a list of filter paths to be passed to git log via stdin. Note that
+# paths must not be quoted.
+proc escape_filter_paths {paths} {
+	set escaped [list]
+	foreach path $paths {
+		lappend escaped [string map {\\ \\\\ "\ " "\\\ "} $path]
+	}
+	return $escaped
+}
+
 # Start off a git log process and arrange to read its output
 proc start_rev_list {view} {
     global startmsecs commitidx viewcomplete curview

@@ -414,7 +424,8 @@ proc start_rev_list {view} {
     if {[catch {
         set fd [open [concat | git log --no-color -z --pretty=raw $show_notes \
                         --parents --boundary $args --stdin \
-                        "<<[join [concat $revs "--" $files] "\\n"]"] r]
+                        "<<[join [concat $revs "--" \
+                                [escape_filter_paths $files]] "\\n"]"] r]
     } err]} {
         error_popup "[mc "Error executing git log:"] $err"
         return 0

@@ -568,7 +579,8 @@ proc updatecommits {} {
         set fd [open [concat | git log --no-color -z --pretty=raw $show_notes \
                         --parents --boundary $args --stdin \
                         "<<[join [concat $revs "--" \
-                                $vfilelimit($view)] "\\n"]"] r]
+                                [escape_filter_paths \
+                                        $vfilelimit($view)]] "\\n"]"] r]
     } err]} {
         error_popup "[mc "Error executing git log:"] $err"
         return

gitgitgadget

"Johannes Schindelin via GitGitGadget" <[email protected]>

Makes perfect sense. I do not know the point of saying exactly here, though.

Nice, so this is not about "we may have too many args to fit in our memory", but about "we may have too many args for system to spawn the subprocess with".

It sounds like a design mistake, which may want to be fixed, but of course gitk cannot depend on Git it runs with having such a fix, and use of "^" prefix is a good alternative (after all, "--not" was invented to save us writing ^ in front of many revs).

Good.

Hi Pau & Junio,

this patch series saw a positive review from Junio (thank you! I know that you try to stay away from Tcl code, so I appreciate the effort very much), but apart from that it simply languished on the mailing list for more than two months now.

Paul, is there anything I can do to help you integrate this into gitk? Or is it time to pass over gitk maintenance to the Git project?

Ciao,
Johannes

Two months? My patch which contains an obvious fix [1] has been waiting almost two years.

Or just remove it from the Git codebase and maintain it elsewhere.

[1] https://lore.kernel.org/git/[email protected]/

Felipe Contreras

I just tripped over this problem while trying to de-stack my Git backlog
https://github.com/git-for-windows/git/issues/4408 "sdk gitk interaction".

If there was a way to un-stick this [1] it would be great.

Philip
[1]
https://lore.kernel.org/git/[email protected]/

Hello,

One of the main selling points of git is that it was a *distributed* version control system, which means there wasn't any centralized repository.

We are not forced to use Paul Mackerras' repository, so I created a fork that includes the outstanding patches [1]. Junio can simply pull from there from now on.

I don't want to maintain this, but as I've often found out is the case in open
source: if I don't do it, nobody else will.

If anyone else wants to step up and maintain gitk, that would be great.

Cheers.

[1] https://github.com/felipec/gitk

Felipe Contreras

I have done the same "create temporary fork of gitk, queue the patches, merge the result down while asking Paul to pull from me" dance I did every once in a while in the past (it seems the last I did it was in Sep 2019 [*]).

* The last merge from Paulus is ef9b086d (Merge branch 'master' of
git://git.ozlabs.org/~paulus/gitk, 2022-05-11), whose second parent (i.e. Paulus's tip) was 465f0386 (gitk: include y coord in recorded sash position, 2022-02-20).

* The two patches that originate from GfW have been applied on top of 465f0386 in the gitk history; the result of this is 7dd272ec (gitk: escape file paths before piping to git log, 2023-01-24).

* The js/gitk-fixes-from-gfw topic holds a -Xsubtree=gitk-git merge of 7dd272ec into my tree. This was merged to 'next' and hopefully will finish the usual 'next' to 'master' journey soonish.

Paul, can you fetch js/gitk-fixes-from-gfw branch of
https://github.com/gitster/git/, which will give you 7dd272ec that you can use to

 $ git merge 7dd272ec

to obtain these two commits from Johannes and Nico?

Alternatively, you can fetch 'seen' from any of the mirrors, as it hopefully should always contain that topic from now on.

Thanks.

[References]

* https://lore.kernel.org/git/[email protected]/

Why only those patches?

Does patches signed off by GfW developers carry a special status regardless of their content?

Felipe Contreras