On Tue, Feb 28, 2017 at 07:23:38AM -0500, Jeff King wrote:
> > -int interpret_branch_name(const char *name, int namelen, struct strbuf *buf)
> > +int interpret_branch_name(const char *name, int namelen, struct strbuf *buf,
> > + unsigned allowed)
> > {
> > char *at;
> > const char *start;
> > @@ -1254,24 +1275,29 @@ int interpret_branch_name(const char *name, int namelen, struct strbuf *buf)
> > if (len == namelen)
> > return len; /* consumed all */
> > else
> > - return reinterpret(name, namelen, len, buf);
> > + return reinterpret(name, namelen, len, buf, allowed);
> > }
>
> It's hard to see from this context, but a careful reader may note that
> we do not check "allowed" at all before calling
> interpret_nth_prior_checkout(). This is looking for branch names via
> HEAD, so I don't think it can ever return anything but a local name.
>
> Which, hmm. I guess was valid when the flag was "only_branches", but
> would not be valid under INTERPRET_BRANCH_REMOTE. I wonder if
>
> git branch -r -D @{-1}
>
> incorrectly deletes refs/remotes/origin/master if you previously had
> refs/heads/origin/master checked out.
The answer is "yes", it's broken. So interpret_branch_name() should do
an "allowed" check before expanding the nth-prior. The fix should be
easy, especially on top of the earlier 426f76595 (which, incidentally, I
already based this series on).
I'll hold off on re-rolling to see if it collects any other review.
-Peff