Re: [PATCH] Re: Gitk --all error when there are more than 797 refs in a repository

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 22, 2009, 01:47 UTC
Message-ID: <7vws3ru4w8.fsf@alter.siamese.dyndns.org>
In-Reply-To: <7v1vlzvjtg.fsf@alter.siamese.dyndns.org>

Junio C Hamano <gitster@pobox.com> writes:

Show 31 quoted lines

> Pat Thoyts <patthoyts@users.sourceforge.net> writes:
>
>> commit 7f289ca8370e5e2f9622a4fbc30b934eb97b984f
>> Author: Pat Thoyts <patthoyts@users.sourceforge.net>
>> Date:   Tue Sep 22 00:55:50 2009 +0100
>>
>>     Avoid expanding --all when passing arguments to git log.
>>     There is no need to expand --all into a list of all revisions as
>>     git log can accept --all as an argument. This avoids any
>>     command-line
>>     length limitations caused by expanding --all into a list of all
>>     revision ids.
>>
>>     Signed-off-by: Pat Thoyts <patthoyts@users.sourceforge.net>
>>
>> diff --git a/gitk b/gitk
>> index a0214b7..635b97e 100755
>> --- a/gitk
>> +++ b/gitk
>> @@ -241,6 +241,8 @@ proc parseviewrevs {view revs} {
>>
>>      if {$revs eq {}} {
>>         set revs HEAD
>> +    } elseif {$revs eq "--all"} {
>> +        return $revs
>>      }
>
> That looks like an ugly hack (aka sweeping the issue under the rug).
>
> What if there are many tags and the user used --tags?  Don't you have
> exactly the same problem?  Likewise, what if $revs were "..master"?

Sorry, I meant "--all --not master" to grab all the topics not merged to master yet.

But my point still stands.

I do not understand what computed values storedin vposids() and vnegids() arrays are being used in the other parts of the program that rely on this function to do what it was asked to do, but if this patch can ever be correct, a much simpler solution to make this function almost no-op and always return {} (empty array ret is initialized to) would be an equally valid fix, no? And my gut feeling tells me that such a change to make this function a no-op _can't_ be a valid fix.

Show 6 quoted lines

> The right approach would be to understand what limit it is busting (it is
> not likely to be the command line length limit for this particular "exec",
> as it only gets "git" "rev-parse" "--all") first, and then fix that.
>
>>      if {[catch {set ids [eval exec git rev-parse $revs]} err]} {
>>         # we get stdout followed by stderr in $err