Skip to content

gitk: Addressing error running on MacOS with large repos. #375

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

gitk: Addressing error running on MacOS with large repos. #375

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion gitk-git/gitk
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -250,8 +250,13 @@ proc parseviewargs {n arglist} {
set nextisval 1
lappend glflags $arg
}
"--not" - "--all" {
"--not" {
lappend revargs $arg
}
"--all" {
# we recognize this argument;
# no expansion needed, use it with 'git log' as-is
set allknown 0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I your list done.. let me know what is next.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 0

Maybe 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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, if you are OK with this revision I can submit it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of course! I am not a gatekeeper ;-) And I see that you already submitted it :-)

}
"--merge" {
set vmergeonly($n) 1
Expand Down