Re: [PATCH] Permit refspec source side to parse as a sha1

On Thu, 20 Mar 2008, Junio C Hamano wrote:

Daniel Barkalow [off-list ref] writes:

quoted

This fixes "git push origin HEAD~1:foo". "git fetch origin HEAD~1:foo"
will report "Couldn't find remote ref HEAD~1", while
"git fetch origin HEAD**1:foo" reports "Invalid refspec 'HEAD**1:foo'"

That is, HEAD~1 is something you're not allowed to ask the remote for, 
while HEAD**1 doesn't mean anything.

Signed-off-by: Daniel Barkalow <redacted>
---
Note that this actually tries to look up the source side, so "git 
fetch origin HEAD^3:foo" usually gives a wrongish error message.

This is very wrong.  When you do not have anything on your own (i.e. HEAD
points at an unborn branch), "git fetch origin HEAD:master" would barf?

No, HEAD is syntactically valid as a ref, so it's accepted regardless of 
anything else.

We should _never_ look at the refs we have; the check is about the syntax.

Unfortunately, there's not (so far as I know) a handy way to check the 
syntax of a sha1 name.

Why are fetch and push refspec parsing code share the same function and
enforce exactly the same error checking to begin with?  Is it because we
are too lazy to implement two semantics that has to be different and try
to "share" code by only implementing checks for "common denominator"?

We enforce additional, purpose-specific error-checking after parsing, when 
we're actually trying to use the refspecs. The problem is that we can't 
make the syntactic check sufficient lenient to cover all valid syntax, but 
tight enough to prohibit any syntax errors, without getting tangled in a 
bit of semantics.

I think that is the root of the problem, because their syntax are designed
to look similar, but their semantics are different [*1*].

The syntax is identical. The semantics is not the parser's problem, and is 
more appropriate for the callers to handle.

For a fetch refspec, LHS is the remote end and you do not even know what
refs are available over there; the _only_ thing you care about is that it
is well formed.

On the other hand, for a push refspec, LHS is the local end and _must_
name a valid commit you are pushing (unless you are pushing empty to
delete the ref).

Not really. If you're fetching, and you have a push refspec configured for 
the same ref, and that refspec has a LHS which, for the current HEAD, 
doesn't refer to a valid commit at the moment, that's fine. It's only a 
problem if you try to use it while it doesn't point at a valid commit, or 
if it isn't valid syntax for a sha1 name (like HEAD*2).

I really think we cannot afford piling hacks on top of hacks to hide the
broken interface forever.  We have two different things to validate, and
the callers all know what they have when calling us to validate.  We
should not have a single loose validation that only catches "it cannot be
either fetch nor push refspec" breakage.

So we should call the same code to parse the string, have that code do no 
validation at all, and have the caller validate the return as appropriate. 
The parsing doesn't depend at all on whether it's for fetching or pushing.

        As a side note, I have a vague recollection that we used to treat
        a refspec from the command line and a refspec from the config
        differently in some contexts, and rules applied to them might have
        been different.  I do not think of what the context was offhand,
        but we might need to further split the implementation of the
        rules.

I think it was that we used to break "push = HEAD", but we support it now.

	-Daniel
*This .sig left intentionally blank*