Re: jk/tag-contains: stalled

[Junio C Hamano <gitster@pobox.com>]

Jeff King <peff@peff.net> writes:

> 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?