Re: [PATCH] remote: prefer exact matches when using refspecs

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 1, 2018, 00:32 UTC
Message-ID: <xmqqr2jj7wpk.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20180731233332.187328-1-jonathantanmy@google.com>

Jonathan Tan <jonathantanmy@google.com> writes:

> This looks good to me. I've checked that refname_match (and
> branch_merge_matches(), which returns the result of refname_match()
> directly) is only used in "if" contexts, so making it return a value
> other than 1 is fine.

Yes, the log message should say that existing callers only care if the returned value is 0 or not (i.e. if we have any match).

> I would initialize best_score to INT_MAX to avoid needing the
> "best_score < 0" comparison, but don't feel strongly about it.

If we want to lose that "have we seen any possible result?" check, I think defining (ARRAY_SIZE(ref_rev_parse_rules) - p) as the score, so that the "full path" gets score 6 (or whatever) and "some remote tracking name" (like "origin->refs/remotes/origin/HEAD") gets score of 1 (smallest but true) may make more sense. Then, start the best score at 0 and every time we get a score strictly better than the best so far, we overwrite the best. That way, we can even lose the "did we get any positive score?" check, too, and making the condition in the inner loop quite simple, i.e.

	int best_score = 0;
	...
	for (ref = refs; ref; ref = ref->next) {
		int score = refname_match(name, ref->name);

		if (best_score < score) {
			best_match = ref;
			best_score = score;
		}
	}

We need a commit log message (hopefully we can lift most parts from your patch in this thread) and a test update to ensure that the same precedence order used as ref resolution (i.e. tags get higher prio over branches, etc.) in addition to the test you had in your patch.

Thanks.