On Mon, Feb 27, 2017 at 02:28:09PM -0800, Junio C Hamano wrote:
> Jeff King <peff@xxxxxxxx> writes:
>
> > I guess something like the patch below works, but I wonder if there is a
> > less-horrible way to accomplish the same thing.
>
> I suspect that a less-horrible would be a lot more intrusive. It
> would go like "interpret-branch-name only gives local branch name,
> and when it does not show it, the callers that know they do not
> necessarily need local branch name would call other at-mark things".
> As you pointed out with the @{upstream} that potentially point at a
> local branch, it will quickly get more involved, I would think, and
> I tend to think that this patch of yours is probably the least evil
> one among possible solutions.
>
> Perhaps with s/not_in_refs_heads/not_a_branch_name/ (or swapping
> polarity, "is_a_branch_name"), the resulting code may not be too
> hard to read?
I actually started with not_a_branch_name, but I wanted specifically to
talk about refs_heads, because we sometimes refer to remote-tracking
branches as "branches" (and the function is called interpret_branch_name(),
after all).
I agree it would be easier to read if the logic were flipped, but I'm
not sure that would be correct. The function knows when it has done a
replacement that takes us outside of a normal branch name. But when it
hasn't, it doesn't really know how the result should be interpreted.
For our purposes in this caller it is enough to say that "foo" should be
treated as "refs/heads/foo", but I don't think that is generally true of
interpret_branch_name(), which might be called as part of get_sha1().
So one alternative is to leave the logic the same way but try to give it
a better name. E.g., call it something like "replaced_with_non_branch"
or something. But there, another negation snuck in. The correct
non-negated thing is really "replaced_with_HEAD_or_refs_remotes" but
that is rather awkward.
-Peff