Re: jk/tag-contains: stalled

Jeff King [off-list ref] writes:

On Thu, Aug 05, 2010 at 10:05:58AM -0700, Junio C Hamano wrote:

quoted

quoted

quoted

quoted

* 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

[...]

quoted

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?