From: Felipe Contreras <hidden> Date: 2016-06-15 22:57:14
Hi,
Since the last series is not merged to master yet, I decided to add more cleanups.
Felipe Contreras (10):
remote-hg: trivial cleanups
remote-hg: get rid of unused exception checks
remote-hg: enable track-branches in hg-git mode
remote-hg: add new get_config_bool() helper
remote-hg: fix new branch creation
remote-hg: disable forced push by default
remote-hg: don't push fake 'master' bookmark
remote-hg: update bookmarks when pulling
remote-hg: test: be a little more quiet
remote-hg: trivial reorganization
contrib/remote-helpers/git-remote-hg | 47 ++++++++++++++++----------------
contrib/remote-helpers/test-hg-hg-git.sh | 3 +-
contrib/remote-helpers/test-hg.sh | 4 +--
3 files changed, 26 insertions(+), 28 deletions(-)
--
1.8.3.rc1.579.g184e698
From: Felipe Contreras <hidden> Date: 2016-06-15 22:57:14
When force_push is disabled, we need to turn the argument to True.
Signed-off-by: Felipe Contreras <redacted>
---
contrib/remote-helpers/git-remote-hg | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: Felipe Contreras <hidden> Date: 2016-06-15 22:57:14
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.
Signed-off-by: Felipe Contreras <redacted>
---
contrib/remote-helpers/git-remote-hg | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: Felipe Contreras <hidden> Date: 2016-06-15 22:57:14
We skip it locally, but not for the remote, so let's do so.
Signed-off-by: Felipe Contreras <redacted>
---
contrib/remote-helpers/git-remote-hg | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
From: Felipe Contreras <hidden> Date: 2016-06-15 22:57:14
We only need to get the remote dict once.
Signed-off-by: Felipe Contreras <redacted>
---
contrib/remote-helpers/git-remote-hg | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
From: Felipe Contreras <hidden> Date: 2016-06-15 22:57:15
This is removing an exception check and since that exception is thrown by
check_output() but not Popen(), this doesn't change anything.
Felipe Contreras wrote:
quoted hunk
We are not calling check_output() anymore.
Signed-off-by: Felipe Contreras <redacted>
---
contrib/remote-helpers/git-remote-hg | 24 +++++++++---------------
1 file changed, 9 insertions(+), 15 deletions(-)
From: Felipe Contreras <hidden> Date: 2016-06-15 22:57:15
This is the first fix, but it's obvious this is what we want: if a user creates
a new branch with git:
% git checkout -b branches/devel
And then pushes this branch
% git push origin branches/devel
(which is the way to push new mercurial branches)
We obviously want to create a branch, but the command would fail, and the fix
is simple: tell the push that we might create new branches.
This only matters when foce_push=False.
Can't possibly introduce regressions, unless you think of the ability to push
new branches as a regression.
Felipe Contreras wrote:
quoted hunk
When force_push is disabled, we need to turn the argument to True.
Signed-off-by: Felipe Contreras <redacted>
---
contrib/remote-helpers/git-remote-hg | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: Felipe Contreras <hidden> Date: 2016-06-15 22:57:15
And here is the important fix. We are essentially reverting back to the old
v1.8.2 behavior, to minimize the possibility of regressions, but in a way the
user can configure.
The cleanups before made it so this patch eas easy and simple.
And the fix before this makes it so the new default force_push=False still is
able to push new branches, so we don't disable other v1.8.3 features.
Felipe Contreras wrote:
quoted hunk
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.
Signed-off-by: Felipe Contreras <redacted>
---
contrib/remote-helpers/git-remote-hg | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: Felipe Contreras <hidden> Date: 2016-06-15 22:57:15
We obviously don't want to push our fake 'master' bookmark to the remote. This
is an obvious good change.
Felipe Contreras wrote:
quoted hunk
We skip it locally, but not for the remote, so let's do so.
Signed-off-by: Felipe Contreras <redacted>
---
contrib/remote-helpers/git-remote-hg | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
From: Felipe Contreras <hidden> Date: 2016-06-15 22:57:15
Another no-brainer; simply shuffling some code.
Felipe Contreras wrote:
quoted hunk
We only need to get the remote dict once.
Signed-off-by: Felipe Contreras <redacted>
---
contrib/remote-helpers/git-remote-hg | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
From: Junio C Hamano <hidden> Date: 2016-06-15 22:57:15
Felipe Contreras [off-list ref] 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.
From: Felipe Contreras <hidden> Date: 2016-06-15 22:57:15
On Tue, May 14, 2013 at 4:59 PM, Junio C Hamano [off-list ref] wrote:
Felipe Contreras [off-list ref] writes:
quoted
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:
quoted
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