Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix gitk (long cmdline) #2170

Merged
merged 4 commits into from
Apr 26, 2019
Merged

Conversation

dscho
Copy link
Member

@dscho dscho commented Apr 25, 2019

When gitk is faced with a lot of refs, it simply passes them to git rev-list and git log via the command-line without caring about any command-line size limits. On Windows, this fails as soon as the command-line is longer than roughly 32k characters (which is a lot, but not if you have over a thousand tags/branches that would amount to a longer command-line than 32k characters, which apparently some users do have).

Based on the excellent groundwork performed in #2053, this PR addresses that problem by specifying the revs/files via stdin. In contrast to #2053, we try to avoid the full bidirectional inter-process communication: we know in advance what needs to be passed in via stdin (it does not depend on any interactive output of the spawned process), so we can use the (otherwise rarely used) <<value feature of Tcl's open/exec commands.

This fixes #1987

@dscho
Copy link
Member Author

dscho commented Apr 25, 2019

@max630 Thank you for your initial PR, this here PR (which I still need to touch up with a better description and proper attribution, sorry for that, I am about to run to meetings) would not have been possible.

Could you have a look at the patches?

This was an ugly hack, and in preparation for fixing it the right way,
we revert this hack.

Signed-off-by: Johannes Schindelin <[email protected]>
This was an ugly hack, and in preparation for fixing it the right way,
we revert this hack.

Signed-off-by: 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 git-for-windows#1987

Analysis-and-initial-patch-by: Max Kirillov <[email protected]>
Signed-off-by: Johannes Schindelin <[email protected]>
@max630
Copy link

max630 commented Apr 25, 2019

32k characters (which is a lot, but not if you have 40k tags/branches

a branch takes way more than 1 character, so it is less than 40k. In the #1987 (comment) the number is 1000 branches.

gitk-git/gitk Outdated Show resolved Hide resolved
@max630
Copy link

max630 commented Apr 25, 2019

Thanks, I did not know it can be done that simple.

@max630
Copy link

max630 commented Apr 25, 2019

After fixing the closing bracket no smoke for me, seems working as intended.

@max630
Copy link

max630 commented Apr 25, 2019

What expectedly does not work are files with newlines in file name. Maybe they could be filtered out, just to avoid accidetntal inclusion of other files.

@dscho
Copy link
Member Author

dscho commented Apr 26, 2019

What expectedly does not work are files with newlines in file name

But that did not even work before this change, did it?

This adds a missing closing bracket.

Co-Authored-By: Johannes Schindelin <[email protected]>
Signed-off-by: Max A.K <[email protected]>
@dscho
Copy link
Member Author

dscho commented Apr 26, 2019

After fixing the closing bracket no smoke for me, seems working as intended.

Excellent!

@dscho dscho merged commit 233535e into git-for-windows:master Apr 26, 2019
@dscho dscho deleted the gitk-long-cmdline branch April 26, 2019 12:37
@dscho dscho mentioned this pull request Apr 26, 2019
@dscho dscho added this to the v2.21.0(2) milestone Apr 26, 2019
dscho added a commit to git-for-windows/build-extra that referenced this pull request Apr 26, 2019
gitk [no longer fails with "filename too
long"](git-for-windows/git#2170) when there
are 1,000+ branches/tags.

Signed-off-by: Johannes Schindelin <[email protected]>
dscho added a commit that referenced this pull request Apr 27, 2019
dscho added a commit that referenced this pull request Apr 27, 2019
dscho added a commit that referenced this pull request Apr 27, 2019
dscho added a commit that referenced this pull request Apr 27, 2019
dscho added a commit that referenced this pull request Apr 28, 2023
git-for-windows-ci pushed a commit that referenced this pull request Apr 28, 2023
git-for-windows-ci pushed a commit that referenced this pull request Apr 28, 2023
git-for-windows-ci pushed a commit that referenced this pull request Apr 28, 2023
git-for-windows-ci pushed a commit that referenced this pull request May 1, 2023
git-for-windows-ci pushed a commit that referenced this pull request May 1, 2023
git-for-windows-ci pushed a commit that referenced this pull request May 1, 2023
dscho added a commit that referenced this pull request May 2, 2023
git-for-windows-ci pushed a commit that referenced this pull request May 2, 2023
git-for-windows-ci pushed a commit that referenced this pull request May 2, 2023
git-for-windows-ci pushed a commit that referenced this pull request May 3, 2023
git-for-windows-ci pushed a commit that referenced this pull request May 3, 2023
git-for-windows-ci pushed a commit that referenced this pull request May 3, 2023
git-for-windows-ci pushed a commit that referenced this pull request May 3, 2023
git-for-windows-ci pushed a commit that referenced this pull request May 5, 2023
git-for-windows-ci pushed a commit that referenced this pull request May 5, 2023
git-for-windows-ci pushed a commit that referenced this pull request May 5, 2023
git-for-windows-ci pushed a commit that referenced this pull request May 6, 2023
dscho added a commit to dscho/git that referenced this pull request May 8, 2023
git-for-windows-ci pushed a commit that referenced this pull request May 8, 2023
dscho added a commit to dscho/git that referenced this pull request May 11, 2023
git-for-windows-ci pushed a commit that referenced this pull request May 13, 2023
dscho added a commit that referenced this pull request May 13, 2023
git-for-windows-ci pushed a commit that referenced this pull request May 13, 2023
dscho added a commit that referenced this pull request May 15, 2023
git-for-windows-ci pushed a commit that referenced this pull request May 15, 2023
git-for-windows-ci pushed a commit that referenced this pull request May 15, 2023
dscho added a commit that referenced this pull request May 20, 2023
dscho added a commit that referenced this pull request May 25, 2023
dscho added a commit that referenced this pull request Jun 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

gitk pops up Error: couldn't execute "git": filename too long
3 participants