Re: [PATCH v3 08/10] remote-hg: update bookmarks when pulling
On Tue, May 14, 2013 at 6:32 PM, Junio C Hamano wrote: > Felipe Contreras writes: > >> ... After all, this is in the contrib area, >> so if there's a time for a possible future maintainer of a core part >> of git to make mistakes, it would be now. > > That sounds reasonable. > > Incidentally, before I had to stop working in order to respond to > your endless arguments, I already queued the 8 patches to 'master' > (also remote-bzr one is in below that). Cool. But FTR, the "endless arguments" come from both sides. > I had no time checking other topics in flight nor merging the result > up to 'next' and 'pu' yet, and it will take a while for the result > to be published, but we'll see these in 1.8.3. Well, the priority is 1.8.3. 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 v3 08/10] remote-hg: update bookmarks when pulling
Felipe Contreras writes: > ... After all, this is in the contrib area, > so if there's a time for a possible future maintainer of a core part > of git to make mistakes, it would be now. That sounds reasonable. Incidentally, before I had to stop working in order to respond to your endless arguments, I already queued the 8 patches to 'master' (also remote-bzr one is in below that). I had no time checking other topics in flight nor merging the result up to 'next' and 'pu' yet, and it will take a while for the result to be published, but we'll see these in 1.8.3. -- 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 v3 08/10] remote-hg: update bookmarks when pulling
On Tue, May 14, 2013 at 5:49 PM, Junio C Hamano wrote: > Felipe Contreras writes: > >> The reason for the "only regression" period is to avoid more >> regressions. If you show me how any of the fixes I sent in this series >> could potentially cause a regression, > > I already said that "You can see these patches are so trivially > correct" is not a valid argument. You can say so, it doesn't make it so. > The original patches would also > have been looked correct when they were sent to the list. No, they didn't look correct, and I made it clear when I sent the patches to be *tested*. > Things > take time and actual use by the users to mature. Some things, not all things. That's a hasty generalization fallacy. >>> You cannot be both. Which is it? >> >> I marked the patch that fix a regression as such, I marked the patches >> that are obvious fixes with no possibility of regressions as such, and >> I marked the trivial cleanups with no possibility of regressions as >> such. > > I think you mean 6/10 by "the patch that fix a regression", but if > that is the case, please send only the regression fix that cleanly > apply to the tip of 'master', without any other dependencies, with a > proper description of what breaks and how it fixes. I did that for v2, but you didn't merge that. > We know you can do better than "certain" and "might". > >> In certain situations we might end up pushing garbage revisions (e.g. in >> a rebase), and the patches to deal with that haven't been merged yet. >> >> So let's disable forced pushes by default. I have done more than enough. I'm not going to write tests for this, and I'm not going to find out for sure. There is a *potential* that there's a regression, and that's reason enough to apply this patch as it is. I have done way more than my fair share already. I'm 98% certain that this patch series as it is would not introduce a regression for v1.8.3, and that's good enough for me. If anybody has any problem with that, they can pick this series and do whatever they want with them. If I'm supposed to be the owner of contrib/remote-helpers, it certainly doesn't feel that way. If I am the owner, but you are worried, why don't you let me make the decision, and if something blows in v1.8.3, you can tell me; "see? you are not ready to have the full maintainership of this". After all, this is in the contrib area, so if there's a time for a possible future maintainer of a core part of git to make mistakes, it would be now. But of course, this would *not* happen, because the patches are obviously correct and I stand by that claim. As things currently stand, v1.8.3 *might* introduce a regression. -- 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 v3 08/10] remote-hg: update bookmarks when pulling
Felipe Contreras writes: > The reason for the "only regression" period is to avoid more > regressions. If you show me how any of the fixes I sent in this series > could potentially cause a regression, I already said that "You can see these patches are so trivially correct" is not a valid argument. The original patches would also have been looked correct when they were sent to the list. Things take time and actual use by the users to mature. >> You cannot be both. Which is it? > > I marked the patch that fix a regression as such, I marked the patches > that are obvious fixes with no possibility of regressions as such, and > I marked the trivial cleanups with no possibility of regressions as > such. I think you mean 6/10 by "the patch that fix a regression", but if that is the case, please send only the regression fix that cleanly apply to the tip of 'master', without any other dependencies, with a proper description of what breaks and how it fixes. We know you can do better than "certain" and "might". > In certain situations we might end up pushing garbage revisions (e.g. in > a rebase), and the patches to deal with that haven't been merged yet. > > So let's disable forced pushes by default. 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 v3 08/10] remote-hg: update bookmarks when pulling
On Tue, May 14, 2013 at 5:25 PM, Junio C Hamano wrote: > Felipe Contreras writes: > >> On Tue, May 14, 2013 at 4:59 PM, Junio C Hamano wrote: >>> Felipe Contreras writes: >>> Without this fix, the user would never ever see new bookmarks, only the ones that (s)he initially cloned. >>> >>> Now, think again and realize how long it took you (the original >>> author) to discover issues and come up with these fixes and >>> explanation since the series was merged before -rc0. >> >> This issue has *always* been there, it's not a regression. > > Then why are you rushing it? -rc is a "regression-fix-only" period. I'm not rushing this patch, I'm rushing the regression fix. I added the cleanup patches because they help the fix, and I added this patch because it's obviously good. >> Define "ready". It's probably more "ready" than any other bridge >> tool out there. > > Anything that needs "oh, we need to push these ten patches to avoid > regressions at the last minute" is not mature enough to be relied > upon by end users for everyday use. That is what I meant. Only a single patch is needed to fix the regression and I sent that patch standalone in v2 of this series, but you didn't pick it, so I sent the cleanups as well. > But now you are saying these are not regression fixes, in which case > they have to wait because the users have known about the limitations > that existed for a long time and learned to live with them. That is > a sign of mature (not "not ready") software. No, they don't have to wait. And we don't have to mindlessly apply guidelines as dogma. The reason for the "only regression" period is to avoid more regressions. If you show me how any of the fixes I sent in this series could potentially cause a regression, I would agree with you, even if remote, the possibility would be there, therefore the patch wouldn't be material for 1.8.3. But the fact of the matter is that the possibility is not there, we are not decreasing the possibility of regressions, we are simply removing a feature users could enjoy for no gain at all. > You cannot be both. Which is it? I marked the patch that fix a regression as such, I marked the patches that are obvious fixes with no possibility of regressions as such, and I marked the trivial cleanups with no possibility of regressions as such. -- 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 v3 08/10] remote-hg: update bookmarks when pulling
Felipe Contreras writes: > On Tue, May 14, 2013 at 4:59 PM, Junio C Hamano wrote: >> Felipe Contreras writes: >> >>> Without this fix, the user would never ever see new bookmarks, only the ones >>> that (s)he initially cloned. >> >> Now, think again and realize how long it took you (the original >> author) to discover issues and come up with these fixes and >> explanation since the series was merged before -rc0. > > This issue has *always* been there, it's not a regression. Then why are you rushing it? -rc is a "regression-fix-only" period. > Define "ready". It's probably more "ready" than any other bridge > tool out there. Anything that needs "oh, we need to push these ten patches to avoid regressions at the last minute" is not mature enough to be relied upon by end users for everyday use. That is what I meant. But now you are saying these are not regression fixes, in which case they have to wait because the users have known about the limitations that existed for a long time and learned to live with them. That is a sign of mature (not "not ready") software. You cannot be both. Which is it? -- 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 v3 08/10] remote-hg: update bookmarks when pulling
On Tue, May 14, 2013 at 4:59 PM, Junio C Hamano wrote: > Felipe Contreras writes: > >> Without this fix, the user would never ever see new bookmarks, only the ones >> that (s)he initially cloned. > > Now, think again and realize how long it took you (the original > author) to discover issues and come up with these fixes and > explanation since the series was merged before -rc0. This issue has *always* been there, it's not a regression. > Are we giving the users enough time to discover and complain issues > that these 10 patches may introduce before the final release? Yes, because the time needed is *zero*. > "You > can see these patches are so trivially correct" is not a valid > argument. The original patches would also have been looked correct > when they were sent to the list. Things take time and actual use by > the users to mature. There was no original patches that introduced this regression, because it's not a regression. When I say these are trivially correct, I mean it; the regressions you talk about were on patches that were marked as *not* trivially correct, and potentially dangerous. > Having said that, the impression I am getting is that whatever we > pushed out in 1.8.2 and 1.8.3-rc0 was far from ready for real use, > and with your explanations (by the way, I found that many of them > deserve to be in the log message), the end result of applying these > patches, up to 8/10, will still not be as it is very likely that you > and users will discover issues at a similar rate, but at least it > will be much closer to be ready than they currently are. Define "ready". It's probably more "ready" than any other bridge tool out there. > In other words, it still seems to be in "something is better than > nothing, newer is better than older" stage before stabilization. remote-hg is already stable, this patch has nothing to do with stabilization, neither do any of the 47 patches I sent to the list. > And that is to be expected for a contrib/ material; nothing for us > to be ashamed of. So I changed my mind. As long as it is clearly > marked as "still experimental, possibly with rough edges", I think > it is better to ship 1.8.3 with these 8 patches than without. > > I am unhappy with 3/10, though. It is spreading existing mistake by > adding another configuration variable with dash in its name, which > goes against the recommended practice, and making it more cumbersome > to eventually fix them, because we would need to break end user's > configuration. It's not adding any configuration; remote-hg.track-branches is already present, even in v1.8.2. > Things like 1/10 and 2/10 that can be characterized as: > >> This is a trivial cleanup, cannot cause regressions. > > may probably be a good clean-up to build the next development cycle > on top, but are not at all urgent for it to deserve to be included > in the upcoming release. But it seems that 3-8 textually depend on > at least 2, so I'll queue the first eight for 1.8.3 and exclude the > rest for now. If these are identical to the early part of the > 47-patch series (I didn't check; they are for the next cycle), it > would make the next cycle shorter by 8 patches. Indeed, they are exactly the same as the first 10 patches of the 47-patch series. I think this makes sense. -- 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 v3 08/10] remote-hg: update bookmarks when pulling
Felipe Contreras writes: > Without this fix, the user would never ever see new bookmarks, only the ones > that (s)he initially cloned. Now, think again and realize how long it took you (the original author) to discover issues and come up with these fixes and explanation since the series was merged before -rc0. Are we giving the users enough time to discover and complain issues that these 10 patches may introduce before the final release? "You can see these patches are so trivially correct" is not a valid argument. The original patches would also have been looked correct when they were sent to the list. Things take time and actual use by the users to mature. Having said that, the impression I am getting is that whatever we pushed out in 1.8.2 and 1.8.3-rc0 was far from ready for real use, and with your explanations (by the way, I found that many of them deserve to be in the log message), the end result of applying these patches, up to 8/10, will still not be as it is very likely that you and users will discover issues at a similar rate, but at least it will be much closer to be ready than they currently are. In other words, it still seems to be in "something is better than nothing, newer is better than older" stage before stabilization. And that is to be expected for a contrib/ material; nothing for us to be ashamed of. So I changed my mind. As long as it is clearly marked as "still experimental, possibly with rough edges", I think it is better to ship 1.8.3 with these 8 patches than without. I am unhappy with 3/10, though. It is spreading existing mistake by adding another configuration variable with dash in its name, which goes against the recommended practice, and making it more cumbersome to eventually fix them, because we would need to break end user's configuration. Things like 1/10 and 2/10 that can be characterized as: > This is a trivial cleanup, cannot cause regressions. may probably be a good clean-up to build the next development cycle on top, but are not at all urgent for it to deserve to be included in the upcoming release. But it seems that 3-8 textually depend on at least 2, so I'll queue the first eight for 1.8.3 and exclude the rest for now. If these are identical to the early part of the 47-patch series (I didn't check; they are for the next cycle), it would make the next cycle shorter by 8 patches. -- 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 v3 08/10] remote-hg: update bookmarks when pulling
Without this fix, the user would never ever see new bookmarks, only the ones
that (s)he initially cloned.
Felipe Contreras wrote:
> Signed-off-by: Felipe Contreras
> ---
> contrib/remote-helpers/git-remote-hg | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/contrib/remote-helpers/git-remote-hg
> b/contrib/remote-helpers/git-remote-hg
> index beb864b..dc276af 100755
> --- a/contrib/remote-helpers/git-remote-hg
> +++ b/contrib/remote-helpers/git-remote-hg
> @@ -363,6 +363,9 @@ def get_repo(url, alias):
> die('Repository error')
> repo.pull(peer, heads=None, force=True)
>
> +rb = peer.listkeys('bookmarks')
> +bookmarks.updatefromremote(myui, repo, rb, url)
> +
> return repo
>
> def rev_to_mark(rev):
> --
> 1.8.3.rc1.579.g184e698
--
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
