Re: [PATCH 1/5] t1508 (at-combinations): more tests; document failures

From: Ramkumar Ramachandra <artagnon@gmail.com>
Date: May 2, 2013, 11:03 UTC
Message-ID: <CALkWK0k5zyGwZaP84YMON8VZbfjRxQ3PG2ML2ds8GdQY38s7SA@mail.gmail.com>
In-Reply-To: <CAMP44s1x8pu2B2h3wKmuBDXz3Tqyk9qyijshcD6iN+NpYDCQdQ@mail.gmail.com>

Felipe Contreras wrote:

> But HEAD is special, @ is not. HEAD is documented, @ is not.

Your point being?  That we should document @?  Yes, I agree.

Show 5 quoted lines

> Where is it documented that @ points to HEAD? Where is it documented
> that 'branch -u foo @' would replace @ with HEAD?
> Documentation/revisions.txt? Sorry, 'git branch -u foo' does not parse
> revisions, so that's not the answer. And there's many other places
> that don't do revision parsing either.

You're confusing parsing revisions with parsing symbolic-refs. I've documented @ right next to HEAD, FETCH_HEAD and the other symbolic refs in revisions.txt. Yes, we have to update the documentation of commands like 'git branch -u foo' to make it clear that they can operate on symbolic-refs (not just "HEAD") that point to branches, not just plain branch refs.

Maybe even a fresh page on symbolic refs?

> Your approach is more like a hack,

Now you're just saying rubbish. Neither of the approaches is more of a "hack" than the other. You've implemented @ as a revision, while I've implemented it as a symbolic-ref.

Your approach requires less effort to document, and my approach yields an implementation that is almost trivial. That is not the basis for determining which approach is "better".

> it has the consequences we want,
> but it has many other unintentional and undocumented consequences.

Who said it wasn't intentional? Yes, I agree with your criticism about it being undocumented: please help fixing this.

It's very much intentional.  I _want_ these to work:

% git symbolic-ref M refs/heads/master % git show M@{u} % git branch -u ram/master M

In other words, I want commands that operate on "HEAD" to also operate on other symbolic refs similarly. Is this an unreasonable request?

> If I find a single place where 'HEAD' is hard-coded, and your patch
> doesn't replace '@' correctly, would you then accept that there are
> unintentional consequences, and that this approach is no the best
> precisely for that reason?

You'd have found a bug then, and we must fix it. Why are you throwing useful features out the window simply because of difficulty of documentation/ historical inertia?

Show 12 quoted lines

> Ramkumar Ramachandra wrote:
>> git branch X <any
>> expression with or without a symbolic ref> works fine, and it has
>> nothing to do with my series.
>
> No, it doesn't.
>
> % git symbolic-ref TEST refs/heads/master
> % git branch -u origin/master TEST
> fatal: branch 'TEST' does not exist
> % git branch --edit-description TEST
> error: No branch named 'TEST'.

Are you reading what you're responding to?  I said:

% git branch X @{-1} % git branch X HEAD % git symbolic-ref M refs/heads/master % git branch X M

Will work with or without my patch. This is because git branch <1> <2> runs <2> through the revision parser.

This will work with my patch:

% git symbolic-ref M refs/heads/master % git branch -u origin/master M

precisely because:

% git branch -u origin/master HEAD

works.  And precisely because this does not:

% git branch -u origin/master @{-1}

In other words, git branch -u <1> <2> expects <2> to be a ref or symbolic-ref (currently limited to "HEAD"), not a revision. It doesn't run <2> through the revision parser to check if it resolves to a ref.

The following will not work:

% git symbolic-ref M refs/heads/master % git branch --edit-description M

precisely because:

% git branch --edit-description HEAD

does not work. This is because git branch --edit-description <1> expects <1> to be a non-symbolic ref. It doesn't even hard-code "HEAD".

Why are you blaming my patch for existing inconsistencies in the UI?

There is one limitation worth nothing: a symbolic-ref can only point to a ref (or another symbolic ref). The revision parser doesn't kick in at the resolve_ref_unsafe() stage. So, it's quite non-trivial to implement what Thomas asked for (git symbolic-ref U @{u}). However, I think my series is one step in the right direction. I'd really love symbolic refs I can take along with me (so M -> master can be in my .gitconfig): we have to hook resolve_ref_unsafe() to the config API to achieve this.

In other words, I'm thinking about the future of symbolic refs.