On Sat, Jan 26, 2013 at 01:26:53PM -0800, Junio C Hamano wrote:
> This looks very good.
>
> I wonder if this lets us get rid of the hack in cmd_log_walk() that
> does this:
>
> while ((commit = get_revision(rev)) != NULL) {
> if (!log_tree_commit(rev, commit) &&
> rev->max_count >= 0)
> rev->max_count++;
> ! if (!rev->reflog_info) {
> ! /* we allow cycles in reflog ancestry */
> free(commit->buffer);
> commit->buffer = NULL;
> ! }
> free_commit_list(commit->parents);
> commit->parents = NULL;
>
> After log_tree_commit() handles the commit, using the buffer, we
> discard the memory associated to it because we know we no longer
> will use it in normal cases.
> [...]
> But that is a performance thing, not a correctness issue, so "we
> allow cycles" implying "therefore if we discard the buffer, we will
> show wrong output" becomes an incorrect justification.
Right. I think the correctness issue goes away with my patches, and it
is just a question of estimating the workload for performance. I doubt
it makes a big difference either way, especially when compared to
actually showing the commit (even a single pathspec limiter, or doing
"-p", would likely dwarf a few extra commit decompressions).
My HEAD has about 400/3000 non-unique commits, which matches your
numbers percentage-wise. Dropping the lines above (and always freeing)
takes my best-of-five for "git log -g" from 0.085s to 0.080s. Which is
well within the noise. Doing "git log -g Makefile" ended up at 0.183s
both before and after.
So I suspect it does not matter at all in normal cases, and the time is
indeed dwarfed by adding even a rudimentary pathspec. I'd be in favor of
dropping the lines just to decrease complexity of the code.
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html