Re: [PATCH 2/2] transport-helper: update remote helper namespace
Junio C Hamano wrote: > Just rerolling with what _you_ think is an appropriate level of > explanation (either or both in log and in-code) and see what happens > would probably be the best way to proceed, I think, at this > point. Either you hear "It still is wrong and too sketchy", "Yeah, > thinking about it again, this is sufficient" from others. Or a > silent, which I am inclined to take as much closer to the latter > after all the discussion. For future reference, sometimes when I am silent it does not mean agreement but means "I am fed up and don't consider it to be worth the drain on my energy to deal with this mess." Which is pretty close to "let's move on; this is sufficient", yes. 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 2/2] transport-helper: update remote helper namespace
On Sun, Apr 14, 2013 at 11:45:10AM -0700, Junio C Hamano wrote: > Just rerolling with what _you_ think is an appropriate level of > explanation (either or both in log and in-code) and see what happens > would probably be the best way to proceed, I think, at this > point. Either you hear "It still is wrong and too sketchy", "Yeah, > thinking about it again, this is sufficient" from others. Or a > silent, which I am inclined to take as much closer to the latter > after all the discussion. FWIW, the last email I wrote on this patch said: So I can buy the argument that bumping it forward ourselves will not matter for any well-implemented helper. and I was the only reviewer, so I think the code is probably OK. I also said: That is the sort of thing that might be helpful to include in the commit message[...] Felipe of course did not agree, but I have no interest in trying to persuade him on that front, as it seems to just waste everyone's time. -Peff -- 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 2/2] transport-helper: update remote helper namespace
Felipe Contreras writes: > On Sun, Apr 14, 2013 at 12:13 AM, Junio C Hamano wrote: >> Felipe Contreras writes: >> >>> Why wasn't this patch merged to 'pu'? To my knowledge nobody raised >>> any real concerns. >> >> There are many reasons not to queue _everything_ ever posted to the >> list on 'pu', and they are almost always not a deliberate rejection. >> >> The maintainer may have thought he is not the best person to judge >> changes to the area the patch touches, and may be expecting further >> comments from others, but haven't said "Comments?" and waiting for >> them to say something without being asked. Or the maintainer may >> have judged that it is likely to result in wasted work if he queues >> that version of the patch, fixing trivial nits himself, only to see >> a reroll arrive before the day's integration cycle finishes (which >> makes him run the cycle again). Or the maintainer may have been busy >> tending to other topics. Or the maintainer may have pushed the patch >> down the queue for any of the above reasons to deal with it later, >> and after having tended to others' topics, may have forgotten about >> that patch. > > The world is full of possibilities, but most of them are irrelevant, > specially since 'the maintainer' is right here and can mention the > reason himself. Is there anything wrong in asking? An earlier draft of my message starte with "Do you have to be combative to ask a simple 'did you forget this?' question?", but later I removed it. That was what made it irrelevant ;-) Just rerolling with what _you_ think is an appropriate level of explanation (either or both in log and in-code) and see what happens would probably be the best way to proceed, I think, at this point. Either you hear "It still is wrong and too sketchy", "Yeah, thinking about it again, this is sufficient" from others. Or a silent, which I am inclined to take as much closer to the latter after all the discussion. -- 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 2/2] transport-helper: update remote helper namespace
On Sun, Apr 14, 2013 at 12:13 AM, Junio C Hamano wrote: > Felipe Contreras writes: > >> Why wasn't this patch merged to 'pu'? To my knowledge nobody raised >> any real concerns. > > There are many reasons not to queue _everything_ ever posted to the > list on 'pu', and they are almost always not a deliberate rejection. > > The maintainer may have thought he is not the best person to judge > changes to the area the patch touches, and may be expecting further > comments from others, but haven't said "Comments?" and waiting for > them to say something without being asked. Or the maintainer may > have judged that it is likely to result in wasted work if he queues > that version of the patch, fixing trivial nits himself, only to see > a reroll arrive before the day's integration cycle finishes (which > makes him run the cycle again). Or the maintainer may have been busy > tending to other topics. Or the maintainer may have pushed the patch > down the queue for any of the above reasons to deal with it later, > and after having tended to others' topics, may have forgotten about > that patch. The world is full of possibilities, but most of them are irrelevant, specially since 'the maintainer' is right here and can mention the reason himself. Is there anything wrong in asking? >> I know I said I was going >> to update the commit message, but I don't think that reason to not put >> it in 'pu'. > > For this particular case, I think that was exactly the reason why it > is not in 'pu' today. I actually did not remember the reason when > you asked above, until I read the above "I said I'll update". > > One major reason why I queue a patch that is clearly not ready for > 'next' is because I do not want to forget about the topic and not > because I want to keep the particular version of the patch. I do so > especially when contributor is unlikely to come back soon. If you > said you would come back soon, I do not even have to judge if it is > "clearly not ready" or "it is good enough" for next, and I have to > remember one less thing. The more I can put in "this will come back > so I do not have to do anything" bin, not in the "queue it as-is for > now, because it is likely that it won't be rerolled soon and I'll > forget about it" bin, the easier for me and we as the development > process as a whole can scale better. Well, I'm telling you I think it's good as it is. Other people might disagree, but if they do so, it's possibly on the basis that other transport-helpers might not be using marks, which is something I have said time and again is not possible, and to think it is, is a mistake. I am willing to demonstrate the above claim beyond any reasonable doubt to any sensible person (haven't I done so already?), so that we can avoid these discussions again in the future, and include in the documentation that marks are necessary, or throw a warning for the people developing a remote helper, or something along those lines. But if that's not what you want me to do, then what is needed to get this patch into 'pu'? That's what I'm wondering. Cheers. -- Felipe Contreras -- 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 2/2] transport-helper: update remote helper namespace
Felipe Contreras writes: > Why wasn't this patch merged to 'pu'? To my knowledge nobody raised > any real concerns. There are many reasons not to queue _everything_ ever posted to the list on 'pu', and they are almost always not a deliberate rejection. The maintainer may have thought he is not the best person to judge changes to the area the patch touches, and may be expecting further comments from others, but haven't said "Comments?" and waiting for them to say something without being asked. Or the maintainer may have judged that it is likely to result in wasted work if he queues that version of the patch, fixing trivial nits himself, only to see a reroll arrive before the day's integration cycle finishes (which makes him run the cycle again). Or the maintainer may have been busy tending to other topics. Or the maintainer may have pushed the patch down the queue for any of the above reasons to deal with it later, and after having tended to others' topics, may have forgotten about that patch. Do I need to go on? > I know I said I was going > to update the commit message, but I don't think that reason to not put > it in 'pu'. For this particular case, I think that was exactly the reason why it is not in 'pu' today. I actually did not remember the reason when you asked above, until I read the above "I said I'll update". One major reason why I queue a patch that is clearly not ready for 'next' is because I do not want to forget about the topic and not because I want to keep the particular version of the patch. I do so especially when contributor is unlikely to come back soon. If you said you would come back soon, I do not even have to judge if it is "clearly not ready" or "it is good enough" for next, and I have to remember one less thing. The more I can put in "this will come back so I do not have to do anything" bin, not in the "queue it as-is for now, because it is likely that it won't be rerolled soon and I'll forget about it" bin, the easier for me and we as the development process as a whole can scale better. -- 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 2/2] transport-helper: update remote helper namespace
Hi, Why wasn't this patch merged to 'pu'? To my knowledge nobody raised any real concerns. Should I explain in every commit that touches transport-helper how remote-helpers without marks are impossible? I know I said I was going to update the commit message, but I don't think that reason to not put it in 'pu'. Also, the only reason I said so was to make Jeff happy, but now that I think again, it doesn't really belong there; remote helpers cannot be using these refs, they just can't. They cannot work without marks, it's not possible. To think otherwise is simply a mistake. On Thu, Apr 11, 2013 at 12:18 AM, Felipe Contreras wrote: > On Thu, Apr 11, 2013 at 12:05 AM, Jeff King wrote: >> On Wed, Apr 10, 2013 at 11:53:38PM -0500, Felipe Contreras wrote: >> >>> > But if we push some commits to the helper, moving Y up to Z, then it >>> > would build the new commit (which contains the foreign-vcs's equivalent of >>> > Y..Z) on top of Z, not Y. >>> >>> Why would it do that? If X points to say revision 100, presumably it >>> was stored somewhere while doing a fetch. Similarly, if foreign >>> version of Z is 150, it can update that number while doing a push. The >>> next fetch it would start from 151. >> >> I think the only reason not to bump the marker forward during the push >> would be if the helper wants for some reason to "re-import" from the >> foreign source rather than accepting the git versions of the commits. >> Something like git-svn's markup of the commit messages with revision ids >> comes to mind. > > Yeah, but that's already a second level hypothesis. First, > remote-helpers would need to be able to work without marks, and they > can't. > >> But if it matters, then by definition that would mean >> that the import/export is not bidirectionally clean. > > I don't see how would that matter. > >> So I can buy the argument that bumping it forward ourselves will not >> matter for any well-implemented helper. > > Or any helper. > >> That is the sort of thing that might be helpful to include in the commit >> message; if somebody does run across such a helper and bisects to your >> commit, then they can understand the rationale for the decision. > > If it did matter, it would be mentioned. I will updated it later if > there's no further comments. -- Felipe Contreras -- 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 2/2] transport-helper: update remote helper namespace
On Thu, Apr 11, 2013 at 12:05 AM, Jeff King wrote: > On Wed, Apr 10, 2013 at 11:53:38PM -0500, Felipe Contreras wrote: > >> > But if we push some commits to the helper, moving Y up to Z, then it >> > would build the new commit (which contains the foreign-vcs's equivalent of >> > Y..Z) on top of Z, not Y. >> >> Why would it do that? If X points to say revision 100, presumably it >> was stored somewhere while doing a fetch. Similarly, if foreign >> version of Z is 150, it can update that number while doing a push. The >> next fetch it would start from 151. > > I think the only reason not to bump the marker forward during the push > would be if the helper wants for some reason to "re-import" from the > foreign source rather than accepting the git versions of the commits. > Something like git-svn's markup of the commit messages with revision ids > comes to mind. Yeah, but that's already a second level hypothesis. First, remote-helpers would need to be able to work without marks, and they can't. > But if it matters, then by definition that would mean > that the import/export is not bidirectionally clean. I don't see how would that matter. > So I can buy the argument that bumping it forward ourselves will not > matter for any well-implemented helper. Or any helper. > That is the sort of thing that might be helpful to include in the commit > message; if somebody does run across such a helper and bisects to your > commit, then they can understand the rationale for the decision. If it did matter, it would be mentioned. I will updated it later if there's no further comments. -- Felipe Contreras -- 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 2/2] transport-helper: update remote helper namespace
On Wed, Apr 10, 2013 at 11:53:38PM -0500, Felipe Contreras wrote: > > But if we push some commits to the helper, moving Y up to Z, then it > > would build the new commit (which contains the foreign-vcs's equivalent of > > Y..Z) on top of Z, not Y. > > Why would it do that? If X points to say revision 100, presumably it > was stored somewhere while doing a fetch. Similarly, if foreign > version of Z is 150, it can update that number while doing a push. The > next fetch it would start from 151. I think the only reason not to bump the marker forward during the push would be if the helper wants for some reason to "re-import" from the foreign source rather than accepting the git versions of the commits. Something like git-svn's markup of the commit messages with revision ids comes to mind. But if it matters, then by definition that would mean that the import/export is not bidirectionally clean. git-svn is definitely not that, but I hope that vcs-svn will be (I have not kept up on its status). So I can buy the argument that bumping it forward ourselves will not matter for any well-implemented helper. That is the sort of thing that might be helpful to include in the commit message; if somebody does run across such a helper and bisects to your commit, then they can understand the rationale for the decision. -Peff -- 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 2/2] transport-helper: update remote helper namespace
On Wed, Apr 10, 2013 at 11:33 PM, Jeff King wrote: > On Wed, Apr 10, 2013 at 07:07:12PM -0500, Felipe Contreras wrote: > >> When pushing, the remote namespace is updated correctly >> (e.g. refs/origin/master), but not the remote helper's >> (e.g. refs/testgit/origin/master). >> >> Let's update it correctly. > > I would have thought it was the helper's responsibility to update these. > Obviously remote-testgit can handle this fine, but will any other > helpers be using these refs as a marker to know the last point they > imported, and get confused if we update the refs behind their back? > > For example, during the import, a helper might know that it has imported > up to X on a foreign vcs, and that resulted in commit Y in git, which it > stored in refs/$helper/heads/master during the last import. When we > fetch from it again, it picks up from X to the tip of the foreign vcs, > and then imports that history on top of commit Y. > > But if we push some commits to the helper, moving Y up to Z, then it > would build the new commit (which contains the foreign-vcs's equivalent of > Y..Z) on top of Z, not Y. Why would it do that? If X points to say revision 100, presumably it was stored somewhere while doing a fetch. Similarly, if foreign version of Z is 150, it can update that number while doing a push. The next fetch it would start from 151. All this is hypothetical of course, because... > I do not offhand know of any helpers that are implemented this way, > though. vcs-svn does not seem to use the refspec feature at all, and I > assume that your hg/bzr helpers do not have this problem. So perhaps it > is not worth worrying about. They cannot be implemented this way, because as I have already argued and shown[1], remote helpers must be using marks, they don't work otherwise (transport helper is broken for those cases). Cheers. [1] http://article.gmane.org/gmane.comp.version-control.git/210306 -- Felipe Contreras -- 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 2/2] transport-helper: update remote helper namespace
On Wed, Apr 10, 2013 at 07:07:12PM -0500, Felipe Contreras wrote: > When pushing, the remote namespace is updated correctly > (e.g. refs/origin/master), but not the remote helper's > (e.g. refs/testgit/origin/master). > > Let's update it correctly. I would have thought it was the helper's responsibility to update these. Obviously remote-testgit can handle this fine, but will any other helpers be using these refs as a marker to know the last point they imported, and get confused if we update the refs behind their back? For example, during the import, a helper might know that it has imported up to X on a foreign vcs, and that resulted in commit Y in git, which it stored in refs/$helper/heads/master during the last import. When we fetch from it again, it picks up from X to the tip of the foreign vcs, and then imports that history on top of commit Y. But if we push some commits to the helper, moving Y up to Z, then it would build the new commit (which contains the foreign-vcs's equivalent of Y..Z) on top of Z, not Y. I do not offhand know of any helpers that are implemented this way, though. vcs-svn does not seem to use the refspec feature at all, and I assume that your hg/bzr helpers do not have this problem. So perhaps it is not worth worrying about. -Peff -- 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
