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

[Jeff King <peff@peff.net>]

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.

> > 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