Re: [PATCH] remote.c: use shorten_unambiguous_ref

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 14, 2009, 16:55 UTC
Message-ID: <7vprff41lf.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <20090410171458.GA26478@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 17 quoted lines

> Hmm. I was thinking we might be able to just do away with prettify_ref,
> but I didn't consider the fact that we need to prettify remote things. I
> think you could still unambiguously prettify the local half of those
> callsites, though.
>
> Given that the two functions are closely related, should we perhaps
> rename them to
>
>   const char *shorten_ref(const char *);
>   const char *shorten_ref_unambiguous(const char *);
>
> ? The implementations are quite different, with prettify_ref not really
> respecting the ref lookup rules, but rather just considering a few
> pre-determined bits of the hierarchy as uninteresting. It shouldn't be
> that hard to have them both use the same implementation, like:
>
>   const char *shorten_ref(const char *, int unambiguous);

I was hoping that a single "shorten" function that does not even take "unambiguous" parameter would be used by almost everybody. As far as I can see, Bert's "rev-parse --abbrev-ref" RFC is the only caller that might need to use a value different from warn_ambiguous_refs, and all the other existing callers (including fill_tracking_info() for "upstream" report by git-branch) do not have to pass "0" but can use the default. IOW, we can have:

	const char *shorten_ref_unambiguous(const char *ref, int strict);
	const char *shorten_ref(const char *ref)
        {
        	return shorten_ref_unambiguous(ref, warn_ambiguous_refs);
	}

and only specialized callers that really care use shorten_ref_unambiguous (without Bert's [PATCH-RFC 3/2] we do not have any such specialized caller, I think).

But I am not sure how well prettify_ref() fits into this picture. It is called only from transport and is meant to deal with refs that exist on the remote side, so ambiguity check against our local namespace would not make much sense. We could:

	const char *shorten_ref_internal(const char *ref, int mode);
	const char *shorten_ref(const char *ref)
        {
		unsigned mode = warn_ambiguous_refs ? SHORTEN_STRICT : 0;
        	return shorten_ref_internal(ref, mode);
	}
	const char *prettify_ref(const char *ref)
        {
        	return shorten_ref_internal(ref, SHORTEN_PREFIX_ONLY);
        }

and have the SHORTEN_PREFIX_ONLY logic inherit from what the current prettify_ref() does, but at that point it I do not think it makes sense anymore.