Jonathan Tan <jonathantanmy@xxxxxxxxxx> 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.
