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

[Ramkumar Ramachandra <artagnon@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.

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

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