Re: [PATCH v1 2/2] log: add option to choose which refs to decorate

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 10, 2017, 17:42 UTC
Message-ID: <xmqqo9oaf2ss.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <89e7f8e0-8b0d-fde0-5e28-31173213a26e@gmail.com>

Rafael Ascensão <rafa.almas@gmail.com> writes:

Show 8 quoted lines

> I agree that describe should receive the "normalize" treatment. However,
> and following the same reasoning, why should describe users adopt the
> rules imposed by --glob? I could argue they're also used to the way it
> works now.
>
> That being said, the suggestion I mentioned earlier would allow to keep
> both current behaviors consistent at the expense of the extra call to
> refs.c::ref_exists().

In any case, updating the "describe" for consistency is something we can and should leave for later, to be done as a separate topic.

While I agree with you that the consistent behaviour between commands is desirable, and also I agree with you that given a pattern $X that does not have any glob char, trying to match $X when a ref whose name exactly is $X exists and trying to match $X/* otherwise would give us a consistent semantics without hurting any existing uses, I do not think you need to pay any extra expense of calling ref_exists() at all to achieve that.

That is because when $X exists, you already know $X/otherthing does not exist. And when $X does not exist, $X/otherthing might. So a naive implementation would be just to add two patterns $X and $X/* to the filter list and be done with it. If you exactly have refs/heads/master, even with the naive logic may throw both refs/heads/master and refs/heads/master/* to the filter list, nothing will match the latter to contaminate your result (and vice versa).

A bit more clever implementation "just throw in two items" would go like this. It is not all that involved:

 - In load_ref_decorations(), before running add_ref_decoration for
   each ref and head ref, iterate over the elements in the refname
   filter list.  For each element:

   - if item->string has a trailing '/', trim that.

   - store NULL in the item->util field for item whose string field
     has a glob char.

   - store something non-NULL (e.g. item->string) for item whose
     string field does not have a glob char.

 - In add_ref_decoration(), where your previous round iterates over
   filter->{include,exclude}, get rid of normalize_glob_ref() and
   use of real_pattern.  Instead do something like:

	matched = 0;
	if (item->util == NULL) {
		if (!wildmatch(item->string, refname, 0))
                	matched = 1;
	} else {
		const char *rest;
		if (skip_prefix(refname, item->string, &rest) &&
                    (!*rest || *rest == '/'))
			matched = 1;
	}
	if (matched)
		...

   Of course, you would probably want to encapsulate the logic to
   set matched = 1/0 in a helper function, e.g.

	static int match_ref_pattern(const char *refname,
				     const struct string_list_item *item) {
		int matched = 0;
		... do either wildmatch or head match with tail validation
		... depending on the item->util's NULLness (see above)
		return matched;
	}

   and call that from the two loops for exclude and include list.

Hmm?