Re: jk/tag-contains: stalled
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Aug 5, 2010, 18:47 UTC
- Message-ID
- <7vy6ckdhhu.fsf@alter.siamese.dyndns.org>
- In-Reply-To
- <20100805173635.GA15760@sigill>
Jeff King <peff@peff.net> writes:
Show 22 quoted lines
> On Thu, Aug 05, 2010 at 10:05:58AM -0700, Junio C Hamano wrote: > >> >>> * jk/tag-contains (2010-07-05) 4 commits >> >>> - Why is "git tag --contains" so slow? >> >>> - default core.clockskew variable to one day >> >>> - limit "contains" traversals based on commit timestamp >> >>> - tag: speed up --contains calculation >> [...] >> > I agree in principle; the log messages need to be cleaned up first >> > at the least, though. >> >> To reduce the risk of double-work, I need to clarify. >> >> I meant to say that I can find enough material, especially what Peff >> wrote, in the discussion that followed in the thread to do the clean-up >> myself. No need to resend by anybody unless there are material >> differences from what have been discussed so far that need to be >> incorporated in the final series. > > The only bad log message should be the final one, which should be > dropped anyway. I would recommend just merging the first two for now, > and Ted can tweak his core.clockskew manually.
After re-reviewing the one that is queued, the use of TMP_MARK smelled somewhat bad to me. It is named TMP_ exactly because it is meant to be used in a closed callpath---you can use it but you are supposed to clean it before you return the control to the caller, so that the caller can rely on TMP_MARK absent from any objects.
Use of UNINTERESTING is similarly not kosher if this were to be used in larger context outside of "do 'tags --contains' and exit". You noted these two points in your original RFC patch.
Besides, "contains()" is too generic a name to live in commit.h.
My gut feeling is that it is probably Ok if contains() and its recursive helper are moved to builtin/tag.c and are made static, to make it clear that this should not be reused outside the current context as a generic "contains" function. It would probably help to have a comment at the end of list_tags() to say that TMP_MARK _ought_ to be cleaned before leaving the function but we don't do that because we know it is the last function in the callchain before we exit.
By the way, I wonder why pop_most_recent_commit() with a commit_list, which is the usual revision traversal ingredient for doing something like this, was not used in the patch, though. Is it because depth-first was necessary?