[PATCH gitk 0/4] gitk support for git log -L
Now that git log -L has hit master, I figure it's time to discuss the corresponding change to gitk. The hard part is that AFAICS this is the first history display accessible through gitk that can only be computed in one go. The existing displays all work by having git-log perform a preliminary search for the involved commits (or in some cases, only part of the range while we fetch more). log -L has to compute all the diffs anyway, so nothing can be saved by attempting this; it is better to load everything in bulk from a single git-log invocation. Thus, patches 1--3 implement the infrastructure required to be able to work from a single git-log command. I would have loved to instead make a feature that also generalizes to git log --parents | gitk --read-stdin (or some other similar option). This would make for much easier testing of new git-log options. Unfortunately this seems much harder to achieve in the current structure of gitk. Note: all my Tk-ing is computationally indistinguishable from cargo culting. Please review with a grain of salt. Thomas Rast (4): gitk: refactor per-line part of getblobdiffline and its support gitk: split out diff part in $commitinfo gitk: support showing the gathered inline diffs gitk: recognize -L option gitk | 462 ++- 1 file changed, 266 insertions(+), 196 deletions(-) -- 1.8.3.496.g0d0267b -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH gitk 0/4] gitk support for git log -L
Thomas Rast writes: > Now that git log -L has hit master, I figure it's time to discuss the > corresponding change to gitk. Paul, any news on this? Any chance we can get it into the next release, since that will also be the first release to ship with 'git log -L'? -- Thomas Rast trast@{inf,student}.ethz.ch -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH gitk 0/4] gitk support for git log -L
Thomas Rast writes: > Thomas Rast writes: > >> Now that git log -L has hit master, I figure it's time to discuss the >> corresponding change to gitk. > > Paul, any news on this? Any chance we can get it into the next release, > since that will also be the first release to ship with 'git log -L'? Jens pointed out privately that the handling of unstuck -L options is unfortunate, to put it mildly. I'll send a reroll. -- Thomas Rast trast@{inf,student}.ethz.ch -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH gitk 0/4] gitk support for git log -L
Am 29.07.2013 21:37, schrieb Thomas Rast: > Thomas Rast writes: > >> Thomas Rast writes: >> >>> Now that git log -L has hit master, I figure it's time to discuss the >>> corresponding change to gitk. >> >> Paul, any news on this? Any chance we can get it into the next release, >> since that will also be the first release to ship with 'git log -L'? > > Jens pointed out privately that the handling of unstuck -L options is > unfortunate, to put it mildly. I'll send a reroll. But as soon as that is fixed I'd really like to see this applied, as I think gitk is the perfect tool to show history information. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH gitk 0/4] gitk support for git log -L
Jens Lehmann writes: > Am 29.07.2013 21:37, schrieb Thomas Rast: >> Thomas Rast writes: >> >>> Thomas Rast writes: >>> Now that git log -L has hit master, I figure it's time to discuss the corresponding change to gitk. >>> >>> Paul, any news on this? Any chance we can get it into the next release, >>> since that will also be the first release to ship with 'git log -L'? >> >> Jens pointed out privately that the handling of unstuck -L options is >> unfortunate, to put it mildly. I'll send a reroll. > > But as soon as that is fixed I'd really like to see this applied, as > I think gitk is the perfect tool to show history information. Unfortunately it's turning out to be harder than I hoped. gitk runs the arguments through git-rev-parse, which only knows that -n gets an unstuck argument. Consequently, gitk accepts an unstuck -n but only stuck forms of -S and -G. Fixing it through git-rev-parse feels wrong; rev-parse is supposed to know about rev-list options, but -S and -G only make sense in diff-generating walks, and -L only makes any sense at all for git-log. I'm tempted to leave it at the existing patches for now. That does mean that -L can only be used in the stuck form; it's the same for -S and -G already. Then in a later series we can change gitk's argument parsing to properly treat the options directly, passing only the remaining arguments through to rev-parse to use the usual revision/filename distinction logic. -- Thomas Rast trast@{inf,student}.ethz.ch -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH gitk 0/4] gitk support for git log -L
Hi Thomas, On Wed, Jul 31, 2013 at 03:17:41PM +0200, Thomas Rast wrote: > Jens Lehmann writes: > > > Am 29.07.2013 21:37, schrieb Thomas Rast: > >> Thomas Rast writes: > >> > >>> Thomas Rast writes: > >>> > Now that git log -L has hit master, I figure it's time to discuss the > corresponding change to gitk. > >>> > >>> Paul, any news on this? Any chance we can get it into the next release, > >>> since that will also be the first release to ship with 'git log -L'? > >> > >> Jens pointed out privately that the handling of unstuck -L options is > >> unfortunate, to put it mildly. I'll send a reroll. > > > > But as soon as that is fixed I'd really like to see this applied, as > > I think gitk is the perfect tool to show history information. One thing I worry about is having gitk storing in memory not just the history graph but also all the diffs (assuming I have understood correctly what you're doing). Gitk's memory consumption is already pretty large. However, I can't see an alternative at this point. > Unfortunately it's turning out to be harder than I hoped. gitk runs the > arguments through git-rev-parse, which only knows that -n gets an > unstuck argument. Consequently, gitk accepts an unstuck -n but only > stuck forms of -S and -G. Excuse my ignorance, but what do you mean by "stuck" vs. "unstuck"? Thanks, Paul. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH gitk 0/4] gitk support for git log -L
Paul Mackerras writes: > Hi Thomas, > > On Wed, Jul 31, 2013 at 03:17:41PM +0200, Thomas Rast wrote: >> Jens Lehmann writes: >> >> > Am 29.07.2013 21:37, schrieb Thomas Rast: >> >> Thomas Rast writes: >> >> >> >>> Thomas Rast writes: >> >>> >> Now that git log -L has hit master, I figure it's time to discuss the >> corresponding change to gitk. [...] > > One thing I worry about is having gitk storing in memory not just the > history graph but also all the diffs (assuming I have understood > correctly what you're doing). Gitk's memory consumption is already > pretty large. However, I can't see an alternative at this point. I don't think there is one. log -L is pretty much an "all or nothing" thing at this point. I suppose if we really found that the diffs are regularly too big to be manageable for gitk, we could invent a porcelain mode where 'log -L' just prints the detected commits and corresponding line ranges, and then have a new option to diff-tree to let it again filter that range. But note that ordinary 'git log -L' also buffers the entire set of diffs within less. The memory consumption of gitk to hold the same diffs in memory should be only a small factor of what less uses in the same scenario. Furthermore, users will typically ask for a small region of code (one function, or some such), so the diffs themselves are usually quite small, nowhere near the size of the full commit diffs. >> Unfortunately it's turning out to be harder than I hoped. gitk runs the >> arguments through git-rev-parse, which only knows that -n gets an >> unstuck argument. Consequently, gitk accepts an unstuck -n but only >> stuck forms of -S and -G. > > Excuse my ignorance, but what do you mean by "stuck" vs. "unstuck"? Whether the option value is a separate argument in argv, or directly stuck to the option. stuck: gitk -L:foo:main.c unstuck: gitk -L :foo:main.c Existing gitk chokes on 'gitk -S foo', but works with 'git -Sfoo'. -- Thomas Rast trast@{inf,student}.ethz.ch -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH gitk 0/4] gitk support for git log -L
Thomas Rast writes: > Whether the option value is a separate argument in argv, or directly > stuck to the option. > > stuck: gitk -L:foo:main.c > unstuck: gitk -L :foo:main.c > > Existing gitk chokes on 'gitk -S foo', but works with 'git -Sfoo'. I somehow thought that we encourage the "stuck/sticked" form, to reduce things the users need to remember to cope better with options with optional value. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH gitk 0/4] gitk support for git log -L
Junio C Hamano writes: > Thomas Rast writes: > >> Whether the option value is a separate argument in argv, or directly >> stuck to the option. >> >> stuck: gitk -L:foo:main.c >> unstuck: gitk -L :foo:main.c >> >> Existing gitk chokes on 'gitk -S foo', but works with 'git -Sfoo'. > > I somehow thought that we encourage the "stuck/sticked" form, to > reduce things the users need to remember to cope better with options > with optional value. I just looked into this again, to get it rolling. Am I reading you correctly as saying that any support for the unstuck form is entirely coincidental, and it's okay to support only the stuck version in new gitk? Note that the support for 'git log -L' supports both, and it irks me that the user can't switch back and forth between using 'gitk' and 'git log' while leaving the rest of the command intact. -- Thomas Rast trast@{inf,student}.ethz.ch -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH gitk 0/4] gitk support for git log -L
Thomas Rast wrote: > Junio C Hamano writes: >> Thomas Rast writes: >>> Existing gitk chokes on 'gitk -S foo', but works with 'git -Sfoo'. >> >> I somehow thought that we encourage the "stuck/sticked" form, to >> reduce things the users need to remember to cope better with options >> with optional value. > > I just looked into this again, to get it rolling. > > Am I reading you correctly as saying that any support for the unstuck > form is entirely coincidental, and it's okay to support only the stuck > version in new gitk? Sort of. :) gitcli(7) says that the sticked form is to be preferred "when you are scripting git". But most git commands use parse-options, which of course supports both forms and makes life easier for humans. Support for just the sticked form is better than nothing, especially if the gitk(1) manpage gains a note about it. In the long run I guess the ideal would be to add a parse-options-like library to the tcl support. My two cents, Jonathan -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html