gitk: Addressing error running on MacOS with large repos. #375
Conversation
Welcome to GitGitGadgetHi @axbannaz, and welcome to GitGitGadget, the GitHub App to send patch series to the Git mailing list from GitHub Pull Requests. Please make sure that this Pull Request has a good description, as it will be used as cover letter. Also, it is a good idea to review the commit messages one last time, as the Git project expects them in a quite specific form:
It is in general a good idea to await the automated test ("Checks") in this Pull Request before contributing the patches, e.g. to avoid trivial issues such as unportable code. Contributing the patchesBefore you can contribute the patches, your GitHub username needs to be added to the list of permitted users. Any already-permitted user can do that, by adding a PR comment of the form Once on the list of permitted usernames, you can contribute the patches to the Git mailing list by adding a PR comment After you submit, GitGitGadget will respond with another comment that contains the link to the cover letter mail in the Git mailing list archive. Please make sure to monitor the discussion in that thread and to address comments and suggestions. If you do not want to subscribe to the Git mailing list just to be able to respond to a mail, you can download the mbox ("raw") file corresponding to the mail you want to reply to from the Git mailing list. If you use GMail, you can upload that raw mbox file via: curl -g --user "<EMailAddress>:<Password>" --url "imaps://imap.gmail.com/INBOX" -T /path/to/raw.txt |
|
/allow |
Please note that the description of the PR will be sent as cover letter, so you will want to remove this part... |
|
User axbannaz is now allowed to use GitGitGadget. WARNING: axbannaz has no public email address set on GitHub |
| lappend revargs $arg | ||
| } | ||
| "--all" { | ||
| set allknown 0 |
There was a problem hiding this comment.
The diff does not immediately explain what the existing purpose of the allknown variable is. To avoid reviewer puzzlement, the commit message offers an excellent canvas to describe the reasoning behind this diff.
Also: you might want to prefix the first line of the commit message with gitk: to make it easier to read the resulting commit history.
There was a problem hiding this comment.
I think I your list done.. let me know what is next.
There was a problem hiding this comment.
The first line of the commit message seems still to be too long, see 5c77ec6 (the ... at the end of the first line, and the fact that it is broken into two lines, suggest this to me).
Further, I was still puzzled about the role of allknown. So I dug a little deeper and found this:
# a flag argument that we don't recognize;
# that means we can't optimize
set allknown 0Maybe you can copy that comment, that really helped me understand.
Finally, your commit message seems to reflect only the analysis part, not the part where it actually describes the change (something like a paragraph at the end would do wonders, e.g. starting with "So let's change the code to prevent that --all option from being expanded, imitating how -<n> is handled.").
It would likely be a good idea to check the grammar once more (start sentences with an upper-case letter, end them in a period character) and amend.
Finally send the patch to the mailing list for review. You can use GitGitGadget, submitGit or send it manually.
There was a problem hiding this comment.
Thanks, if you are OK with this revision I can submit it?
There was a problem hiding this comment.
Of course! I am not a gatekeeper ;-) And I see that you already submitted it :-)
axbannaz
force-pushed
the
use.all
branch
3 times, most recently
from
19d77c5 to
0c90f1f
Compare
The change is stemmed from a problem on the MacOS where, if --all is passed to gitk it should show all the refs/commits graphically. However, on really large git repos, in my instance a git repo with over 52,000 commits, gitk will report an error, "Error executing git log: couldn't execute "git": argument list too long". Mac OS has a limit of which my large repo exceeds. This works fine on Linux, however, not sure about Windows. Looking at gitk script, the decision to have all commit-ids on the command line comes from return value of parseviewargs() function which uses the value of "allknown" to return. If it is '1' then --all is translated to a string of all the commit-ids in the repo, otherwise --all is passed as-is to `git log` cli, which according to git-log man page it is the same as listing all the commit-ids. So, this change is to prevent --all option from being expanded into list of all refs on the command line. Signed-off-by: Arash Bannazadeh-Mahani <[email protected]>
axbannaz
changed the title
|
/submit |
|
Submitted as [email protected] |
|
Was this merged? If not, a gentle ping might help. |
gitk: no need to specify all refs, since
git log --allis the same as list is all the refs/commit ids. Also Mac OS has a limit on size of the list of params for a command line