Re: [PATCH v3 3/4] name-rev: provide debug output

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 31, 2017, 16:52 UTC
Message-ID: <xmqqtw69z8vz.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <21cf9d6f55d17463ab6eccdd78d57cf4a1b8e9e1.1490967948.git.git@grubix.eu>

Michael J Gruber <git@grubix.eu> writes:

Show 21 quoted lines

> Currently, `git describe --contains --debug` does not create any debug
> output because it does not pass the flag down to `git name-rev`, which
> does not know that flag.
>
> Teach the latter that flag, so that the former can pass it down (in
> the following commit).
>
> The output is patterned after that of `git describe --debug`, with the
> following differences:
>
> describe loops over all args to describe, then over all possible
> descriptions; name-rev does it the other way round. Therefore, we need
> to amend each possible description by the arg that it is for (and we
> leave out the "searching to describe" header).
>
> The date cut-off for name-rev kicks in way more often than the candidate
> number cut-off of describe, so we do not clutter the output with the
> cut-off.
>
> Signed-off-by: Michael J Gruber <git@grubix.eu>
> ---

Show 13 quoted lines

>  static void name_rev(struct commit *commit,
>  		const char *tip_name, unsigned long taggerdate,
>  		int generation, int distance, int from_tag,
> -		int deref)
> +		int deref, struct name_ref_data *data)
>  {
>  	struct rev_name *name = (struct rev_name *)commit->util;
>  	struct commit_list *parents;
> @@ -75,6 +88,7 @@ static void name_rev(struct commit *commit,
>  
>  	if (deref) {
>  		tip_name = xstrfmt("%s^0", tip_name);
> +		from_tag += 1;

Why this change? I didn't see it explained in the proposed log message. "deref" is true only when our immediate caller is the one that inspected the object at the tip and found it to be a tag object (i.e. not a lightweight tag or a branch). from_tag is about "is the tip within refs/tags/ hierarchy? Yes/No?" and such a caller will set it appropriately when calling us. This function just passes it down when it recursively calls itself.

We shouldn't be mucking with that value ourselves here, should we?

The only case that this change may make a difference I can think of is when you have a tag object pointed at from outside refs/tags (e.g. refs/heads/foo is a tag object); if you are trying to change the definition of "from_tag" from the current "Is the tip inside refs/tags/?" to "Is the tip either inside refs/tags/ or is it a tag object anywhere?", that may be a good change (I didn't think things through, though), but that shouldn't be hidden inside a commit that claims to only add support for debugging.

What problem are you solving?

Show 8 quoted lines

> @@ -236,7 +273,6 @@ static int name_ref(const char *path, const struct object_id *oid, int flags, vo
>  	}
>  
>  	add_to_tip_table(oid->hash, path, can_abbreviate_output);
> -
>  	while (o && o->type == OBJ_TAG) {
>  		struct tag *t = (struct tag *) o;
>  		if (!t->tagged)

This is a patch noise we can do without.

Thanks.