Re: [PATCH] Fix overwritten remote ref on with fast-import.
On Tuesday 17 July 2012 16:02:12 Jonathan Nieder wrote: > Hi, > > Florian Achleitner wrote: > > So we want the transport-helper to touch only private refs, i.e. some > > subdir of refs/, ok. > > On the other hand I thought we expect git-fetch to update the RHS of the > > passed refspec (or the default one ). How? > > Now I am getting confused by terminology. By "the transport-helper" > do you mean the remote helper (e.g., git-remote-svn) or > transport-helper.c? I was confused too. It should say remote-helper. > > By the "default" refspec do you mean the one specified in .git/config > or some default when none is specified there? "git fetch" updates > refs according to the specified fetch refspec in > builtin/fetch.c::store_updated_refs(). .. and I didn't realize that the two different refspecs involved here can be different and shall be different because they get post-processed accordingly. I thought the remote-helper has to import according to the fetch refspec. > > > Btw, whats FETCH_HEAD for? > > "grep FETCH_HEAD Documentation/*.txt" gives some hints. Most notably: > > git-fetch(1) > > The ref names and their object names of fetched refs are stored > in ".git/FETCH_HEAD". This information is left for a later merge > operation done by 'git merge'. > > gittutorial(7) > -- > Alice can peek at what Bob did without merging first, using the "fetch" > command; this allows Alice to inspect what Bob did, using a special > symbol "FETCH_HEAD", in order to determine if he has anything worth > pulling, like this: > > > alice$ git fetch /home/bob/myrepo master > alice$ git log -p HEAD..FETCH_HEAD > > > Hope that helps, > Jonathan Thanks, Florian -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Fix overwritten remote ref on with fast-import.
Hi, Florian Achleitner wrote: > So we want the transport-helper to touch only private refs, i.e. some subdir > of refs/, ok. > On the other hand I thought we expect git-fetch to update the RHS of the > passed refspec (or the default one ). How? Now I am getting confused by terminology. By "the transport-helper" do you mean the remote helper (e.g., git-remote-svn) or transport-helper.c? By the "default" refspec do you mean the one specified in .git/config or some default when none is specified there? "git fetch" updates refs according to the specified fetch refspec in builtin/fetch.c::store_updated_refs(). > Btw, whats FETCH_HEAD for? "grep FETCH_HEAD Documentation/*.txt" gives some hints. Most notably: git-fetch(1) The ref names and their object names of fetched refs are stored in ".git/FETCH_HEAD". This information is left for a later merge operation done by 'git merge'. gittutorial(7) -- Alice can peek at what Bob did without merging first, using the "fetch" command; this allows Alice to inspect what Bob did, using a special symbol "FETCH_HEAD", in order to determine if he has anything worth pulling, like this: alice$ git fetch /home/bob/myrepo master alice$ git log -p HEAD..FETCH_HEAD Hope that helps, Jonathan -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Fix overwritten remote ref on with fast-import.
On Tuesday 17 July 2012 08:48:20 Jonathan Nieder wrote: > Florian Achleitner wrote: > > On Monday 16 July 2012 22:27:25 Jonathan Nieder wrote: > >> Hm, that still doesn't look right. The RHS of the refspec is supposed to > >> be a _private_ namespace for the remote helper, and refs/remotes/ is > >> not private. > > [...] > > > remote-svn now uses get_fetch_map to retrieve the local refs. So it > > respects the fetch refspec in the config. > > No no no no no. That's transport-helper's job. > > The RHS of the remote helper's refspec really is supposed to be > _private_. Improvements to the documentation to clarify this would be > welcome. So we want the transport-helper to touch only private refs, i.e. some subdir of refs/, ok. On the other hand I thought we expect git-fetch to update the RHS of the passed refspec (or the default one ). How? Btw, whats FETCH_HEAD for? > > Thanks, > Jonathan Thanks, Florian -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Fix overwritten remote ref on with fast-import.
Florian Achleitner wrote: > On Monday 16 July 2012 22:27:25 Jonathan Nieder wrote: >> Hm, that still doesn't look right. The RHS of the refspec is supposed to >> be a _private_ namespace for the remote helper, and refs/remotes/ is >> not private. [...] > remote-svn now uses get_fetch_map to retrieve the local refs. So it respects > the fetch refspec in the config. No no no no no. That's transport-helper's job. The RHS of the remote helper's refspec really is supposed to be _private_. Improvements to the documentation to clarify this would be welcome. Thanks, Jonathan -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Fix overwritten remote ref on with fast-import.
On Monday 16 July 2012 22:27:25 Jonathan Nieder wrote: > Florian Achleitner wrote: > > 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. > > Hm, that still doesn't look right. The RHS of the refspec is supposed to > be a _private_ namespace for the remote helper, and refs/remotes/ is > not private. > > Would something like > > refspec refs/heads/*:refs/svn//* > > work? remote-svn now uses get_fetch_map to retrieve the local refs. So it respects the fetch refspec in the config. If I change the default fetch refspec in .git/config to 'fetch = +refs/heads/*:refs/svn/svnfile/*' it works. If the remote-helper only advertises the private refspec, but the config and remote->fetch is unchanged, it works also as long as the import target is consistent (of course). So we do not want helpers to import to remotes/ but to their private svn/ in this case? (ignoring the config?) > > [...] > > > If yes, it makes sense now! A little comment in the sources would help a > > lot. > Now that you know what the reader of this code needs to know, a patch > would be very welcome. > > "git blame" or "git log -S" can be useful to find what the authors > were thinking when they wrote that line, or to find wording to steal > for a comment. :) coming .. > > Hope that helps, > Jonathan Florian -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Fix overwritten remote ref on with fast-import.
Florian Achleitner wrote: > 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. Hm, that still doesn't look right. The RHS of the refspec is supposed to be a _private_ namespace for the remote helper, and refs/remotes/ is not private. Would something like refspec refs/heads/*:refs/svn//* work? [...] > If yes, it makes sense now! A little comment in the sources would help a lot. Now that you know what the reader of this code needs to know, a patch would be very welcome. "git blame" or "git log -S" can be useful to find what the authors were thinking when they wrote that line, or to find wording to steal for a comment. :) Hope that helps, Jonathan -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Fix overwritten remote ref on with fast-import.
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 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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Fix overwritten remote ref on with fast-import.
Jonathan Nieder writes: > 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? > > Does that remote helper advertise the 'refspec' capability? If so, > what refspec does it use? If not, why not? > > It might seem silly to ask for these things when you're providing a > fix along with the report! I share the puzzlement you expressed above, and it is unclear if this is a fix or covering a user error with butchering a code that is working as intended, making two wrongs happen to collide into producing a right. At least it needs a better description of the problem it tries to fix, preferrably in the form of a new test for the transport helper. > 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. That, too. Thanks. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Fix overwritten remote ref on with fast-import.
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? Does that remote helper advertise the 'refspec' capability? If so, what refspec does it use? If not, why not? 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. Curious, Jonathan -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
