Re: [PATCH] Fix overwritten remote ref on with fast-import.

[Florian Achleitner <florian.achleitner.2.6.31@gmail.com>]

On Sunday 15 July 2012 19:30:25 Jonathan Nieder wrote:
> Hi Florian,
> 
> Florian Achleitner wrote:
> > After importing new commits on top of refs/remotes/* the
> > ref was overwritten with the local refs/heads/master, because the name
> > of the remote reference to fetch, i.e. refs/heads/master, was used to
> > retrieve old_sha1 for it's local counterpart. Therefore, old_sha1 pointed
> > to the local head which was not contained in the remote branch and
> > couldn't
> > be updated (printing a warning ..).
> 
> I assume you are talking about the status quo here.  It's easy to
> forget that others have not already applied your patch, but using the
> present tense would make reading easier.  Think of the patch
> description as a special kind of bug report.
> 
> Unfortunately, as a bug report, the above is lacking some detail.  Do
> I understand correctly that some remote helper is failing when git
> invokes its 'import' command?  What are the symptoms?  If it prints a
> warning, what is the exact warning?

I got that problem when I wanted to make remote-svn fetch
to refs/remotes/ instead of the formerly hardcoded (in fast-export.c)
refs/heads/master. 
I didn't yet have the refspec capability (now I have it), and that seems to be 
a significant part of the problem.
The scenario is as follows:
The fast import stream contains 'commit refs/remotes/svnfile/master' and fast-
import adds all the commits on top of it and updates the ref correctly.
But the string affected by the patch, 'private', contains the remote name of 
the branch because it is duped from ref->name, namely refs/heads/master.
As a consequence, subsequent processing leads to:

fatal: bad object 0000000000000000000000000000000000000000
error: svn::file:///anypath did not send all necessary objects

..because it expects something to have arrived on refs/heads/master.

If ref/heads/master already exists, it works, but the resulting refs are 
wrong.
refs/remotes/svnfile/master points to the same commit as ref/heads/master does, 
which is the one created locally. There is no ref to the remote commits.

It follows that, on re-fetching the same remote it fails and fast-import 
refuses to update the ref:
warning: Not updating refs/remotes/svnfile/master (new tip 
5479b212afab5ef541c142bf75357405b7888e4d does not contain 
4e49b15fcc9797bb90e36ec90c14de3d5437a94d)

That's because the  refs/remotes/svnfile/master points to the wrong local-only 
commit.

After exploring the whole fetch process by inserting dozens of printfs, I 
concluded that it's wrong to retrieve the sha1 to update by passing the branch 
name on the remote side (in private) to read_ref, which gives the sha1 of a 
local branch, but that the correct ref is stored in ->peer_ref. I wasn't 
really sure what peer-ref is meant to be. That's what lead to the patch, but..
> 
> Does that remote helper advertise the 'refspec' capability?  If so,
> what refspec does it use?  If not, why not?

When it does advertise refspec like:
Debug: Remote helper: <- refspec refs/heads/master:refs/remotes/svnfile/master
it all works. Unfortunatly I didn't understand that a day ago.

But I'm still not completely sure about what the line I wanted to patch is 
for.
Doc about git-remote-helpers says: "If no refspec capability is advertised, 
there is an implied refspec *:*."
Hmm..so if the helper doesn't advertise 'refspec' the remote refs/heads/master 
is always fetched to the local refs/heads/master?
If yes, it makes sense now! A little comment in the sources would help a lot.

> 
> It might seem silly to ask for these things when you're providing a
> fix along with the report!  However, if someone else runs into the
> same symptoms, they need to be able to find your patch quickly; if
> your patch has a bad side-effect then we need to know why not to
> revert it; and if someone new starts working on the same area of code,
> they need to know what bugs to avoid reintroducing.

I think we can throw that patch away.

> 
> Curious,
> Jonathan