Page MenuHomePhabricator

commit: add --force-close-branch flag to close a non-head changeset
ClosedPublic

Authored by khanchi97 on Jun 7 2019, 9:55 AM.

Details

Summary

While closing branch from a changeset which is not a branch head
current implementation abort this action in every case but, there
can be the situations where the changeset is not a local head but
could be a remote head. This patch adds the functionality to bypass
the "abort: can only close branch heads" by introducing
--force-close-branch flag.

Test case changes demonstrate the new functionality added.

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

khanchi97 created this revision.Jun 7 2019, 9:55 AM
mharbison72 requested changes to this revision.Jun 7 2019, 1:25 PM
mharbison72 added a subscriber: mharbison72.
mharbison72 added inline comments.
mercurial/commands.py
1684

I didn't realize that --force wasn't already an option for commit, because it is an option to internal commit functions. I still like this idea, but maybe you need to remove this from opts after completing the check here (and also in the --no-close-branch case). Otherwise, it risks bypassing existing safeguards.

https://www.mercurial-scm.org/repo/hg/file/tip/mercurial/localrepo.py#l2498

(I have no idea why that's allowed at all. MQ maybe?)

This revision now requires changes to proceed.Jun 7 2019, 1:25 PM
khanchi97 updated this revision to Diff 15380.Jun 8 2019, 6:22 AM
khanchi97 marked an inline comment as done.Jun 10 2019, 9:02 AM
khanchi97 added inline comments.
mercurial/commands.py
1684

Updated as per suggested changes. Also added a hint to use --force while aborting the user.

mharbison72 accepted this revision.Jun 10 2019, 7:48 PM

LGTM, thanks.

Minor nit that I don't care too much about, but I'm wondering what others think of not-head changeset vs non-head changeset. The latter sounds more natural to me, but I'm not sure if there's another example of this phrasing.

khanchi97 marked an inline comment as done.Jun 11 2019, 1:49 AM

LGTM, thanks.
Minor nit that I don't care too much about, but I'm wondering what others think of not-head changeset vs non-head changeset. The latter sounds more natural to me, but I'm not sure if there's another example of this phrasing.

Yes, I was also confused between these two phrasing when writing the patch. non-head sounds more natural to me too (not sure why I picked first one :P ). What others think about it?

pulkit added a subscriber: pulkit.Jun 11 2019, 10:01 AM

LGTM, thanks.
Minor nit that I don't care too much about, but I'm wondering what others think of not-head changeset vs non-head changeset. The latter sounds more natural to me, but I'm not sure if there's another example of this phrasing.

Yes, I was also confused between these two phrasing when writing the patch. non-head sounds more natural to me too (not sure why I picked first one :P ). What others think about it?

Let's go with non-head.

Also the commit title needs to be changed to commit: add --force flag.. instead of branch: add --force flag..

khanchi97 retitled this revision from branch: add --force flag to close branch from a not-head changeset to commit: add --force flag to close branch from a non-head changeset.Jun 15 2019, 8:00 AM
khanchi97 updated this revision to Diff 15531.

I think adding a --force flag to hg commit is not a nice option. force can mean a lot of things and here except in closing the non-head changeset, it is no-op.

For example, I am in middle of a rebase, tried to commit, it says rebase in progress. I see there is a --force flag, I try to use that to force the commit.

We can do one of the following:

  1. maybe a more specific flag name like --allow-close-branch or something like that
  2. a config option instead of a flag name
In D6490#95305, @pulkit wrote:

I think adding a --force flag to hg commit is not a nice option. force can mean a lot of things and here except in closing the non-head changeset, it is no-op.
For example, I am in middle of a rebase, tried to commit, it says rebase in progress. I see there is a --force flag, I try to use that to force the commit.
We can do one of the following:

  1. maybe a more specific flag name like --allow-close-branch or something like that
  2. a config option instead of a flag name

Fair point. Specific is better.

I'd suggest --force-close-branch as a standalone flag (i.e. you don't need to also use --close-branch; it wasn't clear to me if that's what you meant). --allow-close-branch and --close-branch don't seem different enough to remember the difference.

In D6490#95305, @pulkit wrote:

I think adding a --force flag to hg commit is not a nice option. force can mean a lot of things and here except in closing the non-head changeset, it is no-op.
For example, I am in middle of a rebase, tried to commit, it says rebase in progress. I see there is a --force flag, I try to use that to force the commit.
We can do one of the following:

  1. maybe a more specific flag name like --allow-close-branch or something like that
  2. a config option instead of a flag name

Fair point. Specific is better.
I'd suggest --force-close-branch as a standalone flag (i.e. you don't need to also use --close-branch; it wasn't clear to me if that's what you meant). --allow-close-branch and --close-branch don't seem different enough to remember the difference.

Just my $0.02.

I think that hg commit already has many options that do not even belong to committing (such as -A, --close-branch). Let's not add an option that is not related to commit. To my opinion closing a branch should be done with hg branch not commit (unfortunately this was never the case). Anyway if you really want to do it with commit, I would suggest that --close-branch takes an optional argument rather than adding a new option. This makes UX a bit more convenient

e.g.

hg commit --close-branch --force-close-branch (not that)
hg commit --close-branch forced (or force or something like that)

Could you also update relnotes/next (documenting either the new flag or config option, whatever this patch ends up doing)?

In D6490#95305, @pulkit wrote:

I think adding a --force flag to hg commit is not a nice option. force can mean a lot of things and here except in closing the non-head changeset, it is no-op.
For example, I am in middle of a rebase, tried to commit, it says rebase in progress. I see there is a --force flag, I try to use that to force the commit.
We can do one of the following:

  1. maybe a more specific flag name like --allow-close-branch or something like that
  2. a config option instead of a flag name

Fair point. Specific is better.
I'd suggest --force-close-branch as a standalone flag (i.e. you don't need to also use --close-branch; it wasn't clear to me if that's what you meant). --allow-close-branch and --close-branch don't seem different enough to remember the difference.

Just my $0.02.
I think that hg commit already has many options that do not even belong to committing (such as -A, --close-branch). Let's not add an option that is not related to commit. To my opinion closing a branch should be done with hg branch not commit (unfortunately this was never the case). Anyway if you really want to do it with commit, I would suggest that --close-branch takes an optional argument rather than adding a new option. This makes UX a bit more convenient
e.g.

hg commit --close-branch --force-close-branch (not that)
hg commit --close-branch forced (or force or something like that)

I think it's a little hard to make user understand (by providing a hint like "use force arg with --close-branch") that you need to provide an argument as compared to just providing a standalone flag --force-close-branch as @mharbison72 suggested. I do agree with you that it would make +1 in the no. of flags which don't make sense with hg commit. What do you say?

Could you also update relnotes/next (documenting either the new flag or config option, whatever this patch ends up doing)?

Sure

In D6490#95305, @pulkit wrote:

I think adding a --force flag to hg commit is not a nice option. force can mean a lot of things and here except in closing the non-head changeset, it is no-op.
For example, I am in middle of a rebase, tried to commit, it says rebase in progress. I see there is a --force flag, I try to use that to force the commit.
We can do one of the following:

  1. maybe a more specific flag name like --allow-close-branch or something like that
  2. a config option instead of a flag name

Fair point. Specific is better.
I'd suggest --force-close-branch as a standalone flag (i.e. you don't need to also use --close-branch; it wasn't clear to me if that's what you meant). --allow-close-branch and --close-branch don't seem different enough to remember the difference.

Just my $0.02.
I think that hg commit already has many options that do not even belong to committing (such as -A, --close-branch). Let's not add an option that is not related to commit. To my opinion closing a branch should be done with hg branch not commit (unfortunately this was never the case). Anyway if you really want to do it with commit, I would suggest that --close-branch takes an optional argument rather than adding a new option. This makes UX a bit more convenient
e.g.

hg commit --close-branch --force-close-branch (not that)
hg commit --close-branch forced (or force or something like that)

I think it's a little hard to make user understand (by providing a hint like "use force arg with --close-branch") that you need to provide an argument as compared to just providing a standalone flag --force-close-branch as @mharbison72 suggested. I do agree with you that it would make +1 in the no. of flags which don't make sense with hg commit. What do you say?

What if you mark it (ADVANCED) so that it doesn't show in the help without a -v? Then the hint informs the user when it might be needed, otherwise they don't need to see it all the time.

In D6490#95305, @pulkit wrote:

I think adding a --force flag to hg commit is not a nice option. force can mean a lot of things and here except in closing the non-head changeset, it is no-op.
For example, I am in middle of a rebase, tried to commit, it says rebase in progress. I see there is a --force flag, I try to use that to force the commit.
We can do one of the following:

  1. maybe a more specific flag name like --allow-close-branch or something like that
  2. a config option instead of a flag name

Fair point. Specific is better.
I'd suggest --force-close-branch as a standalone flag (i.e. you don't need to also use --close-branch; it wasn't clear to me if that's what you meant). --allow-close-branch and --close-branch don't seem different enough to remember the difference.

Just my $0.02.
I think that hg commit already has many options that do not even belong to committing (such as -A, --close-branch). Let's not add an option that is not related to commit. To my opinion closing a branch should be done with hg branch not commit (unfortunately this was never the case). Anyway if you really want to do it with commit, I would suggest that --close-branch takes an optional argument rather than adding a new option. This makes UX a bit more convenient
e.g.

hg commit --close-branch --force-close-branch (not that)
hg commit --close-branch forced (or force or something like that)

I think it's a little hard to make user understand (by providing a hint like "use force arg with --close-branch") that you need to provide an argument as compared to just providing a standalone flag --force-close-branch as @mharbison72 suggested. I do agree with you that it would make +1 in the no. of flags which don't make sense with hg commit. What do you say?

What if you mark it (ADVANCED) so that it doesn't show in the help without a -v? Then the hint informs the user when it might be needed, otherwise they don't need to see it all the time.

Nice. So I will update the patch for: adding a standalone option --force-close-branch (ADVANCED) to close a branch from a non-head changeset. Feel free to comments if others still have any opinions on this.

khanchi97 retitled this revision from commit: add --force flag to close branch from a non-head changeset to commit: add --force-close-branch flag to close a non-head changeset.Jun 23 2019, 3:24 AM
khanchi97 edited the summary of this revision. (Show Details)
khanchi97 updated this revision to Diff 15633.
pulkit accepted this revision.Jun 29 2019, 11:53 AM
This revision is now accepted and ready to land.Jun 29 2019, 11:53 AM