gitk: handle long command-lines #1469
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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 <max@max630.net> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
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 git-for-windows#2293 Signed-off-by: Nico Rieck <nico.rieck@gmail.com> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
|
/submit |
|
Submitted as pull.1469.git.1674559397.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
![gitgitgadget[bot]](https://avatars.githubusercontent.com/in/12836?s=60&v=4){kind=small}
| @@ -405,14 +405,16 @@ proc start_rev_list {view} { | |||
| if {$revs eq {}} { | |||
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> 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.
Makes perfect sense. I do not know the point of saying exactly
here, though.
> 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.
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".
> 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`.
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.
|
On the Git mailing list, Johannes Schindelin wrote (reply to this): 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
On Tue, 24 Jan 2023, Johannes Schindelin via GitGitGadget wrote:
> 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
> |
|
On the Git mailing list, Felipe Contreras wrote (reply to this): On Mon, Mar 27, 2023 at 3:41 AM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> 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.
Two months? My patch which contains an obvious fix [1] has been
waiting almost two years.
> 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?
Or just remove it from the Git codebase and maintain it elsewhere.
[1] https://lore.kernel.org/git/20210505211846.1842824-1-felipe.contreras@gmail.com/
--
Felipe Contreras |
|
User |
|
On the Git mailing list, Philip Oakley wrote (reply to this): On 27/03/2023 10:17, Johannes Schindelin wrote:
> 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
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".
>
> On Tue, 24 Jan 2023, Johannes Schindelin via GitGitGadget wrote:
>
>> 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
>>
If there was a way to un-stick this [1] it would be great.
Philip
[1]
https://lore.kernel.org/git/pull.1469.git.1674559397.gitgitgadget@gmail.com/ |
|
User |
|
On the Git mailing list, Felipe Contreras wrote (reply to this): Hello,
Philip Oakley wrote:
> On 27/03/2023 10:17, Johannes Schindelin wrote:
> > 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?
>
> 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".
>
> > On Tue, 24 Jan 2023, Johannes Schindelin via GitGitGadget wrote:
> >
> >> 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.
>
> If there was a way to un-stick this [1] it would be great.
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 |
|
This patch series was integrated into seen via git@d9df2ca. |
|
This patch series was integrated into next via git@adb334d. |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): Philip Oakley <philipoakley@iee.email> writes:
> On 27/03/2023 10:17, Johannes Schindelin wrote:
>> 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
>
> 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".
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/xmqqlfum7epn.fsf@gitster-ct.c.googlers.com/ |
|
This patch series was integrated into seen via git@0d05962. |
|
On the Git mailing list, Felipe Contreras wrote (reply to this): Junio C Hamano wrote:
> Philip Oakley <philipoakley@iee.email> writes:
> > On 27/03/2023 10:17, Johannes Schindelin wrote:
> >> 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?
> >
> > 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".
> * 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).
Why only those patches?
Does patches signed off by GfW developers carry a special status
regardless of their content?
--
Felipe Contreras |
|
This patch series was integrated into seen via git@809bd85. |
|
This patch series was integrated into seen via git@030f9fd. |
|
This patch series was integrated into seen via git@14bd5b0. |
|
This patch series was integrated into seen via git@2fb93bb. |
|
This patch series was integrated into seen via git@29b8a3f. |
|
This patch series was integrated into next via git@29b8a3f. |
|
This patch series was integrated into master via git@29b8a3f. |
|
Closed via 29b8a3f. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Cc: Paul Mackerras paulus@ozlabs.org
cc: Felipe Contreras felipe.contreras@gmail.com
cc: Philip Oakley philipoakley@iee.email