Re: [PATCH] symbolic ref: refuse non-ref targets in HEAD

From: Jeff King <peff@peff.net>
Date: Jan 29, 2009, 08:01 UTC
Message-ID: <20090129080145.GA777@coredump.intra.peff.net>
In-Reply-To: <7vljsuh7kf.fsf@gitster.siamese.dyndns.org>

On Wed, Jan 28, 2009 at 11:53:20PM -0800, Junio C Hamano wrote:

> I generally do not like adding artificial limitation to plumbing like this
> patch does, because the end user making silly mistake using plumbing is a
> sign that there was something lacking in the Porcelain.

Nor do I. I only propose it in this case because

  1. We cannot possibly be hurting somebody's workflow, since it
     produces nonsensical results currently.

  2. The damage it does is so annoying to recover from.

I have no problem with symbolic-ref eventually learning to handle these situations in some more sane manner; in the meantime, I think it makes sense to prevent nasty breakage. And I don't think we're closing any doors for the future; the behavior will switch from "you aren't allowed to do this" to "does something sensible".

> But for this particular case, I do not think any future usage of
> symbolic-ref plubming will get inconvenienced with the change.  I would
> even suggest making the check tighter to insist on refs/heads/ (not just
> refs/) and tighten validate_headref() in path.c to match.

Reasonable. Updated series to follow.

Show 8 quoted lines

> > Please beware that running the test script on the current "master" will
> > actually hose your git repo (test 3 kills the trash directory's
> > .git/HEAD, which means test 4 thinks your parent .git/ is its current
> > repo). Maybe it makes sense to do a precautionary reset in between.
> 
> In addition, perhaps it may make sense to use test_create_repo to go one
> level deeper before starting to play around, so that trash directory's
> repository will prevent you from going any further up.

That sort of helps, but only by luck. Each test kills off one layer of repo. So the first one kills the test_create_repo, and the second one kills the trash directory. Adding another test would kill off the main repo. :) So you have to do something per-test. I'll do that in the re-roll.

-Peff