[PATCH 0/2] completion: complete --skip for cherry-pick and revert

[PATCH 0/2] completion: complete --skip for cherry-pick and revert

[Denton Liu]

Before, the completion script would not complete `--skip` for
cherry-pick and revert, even though it is a valid option while that
operation is in progress. Add that missing completion.


Denton Liu (2):
  completion: merge options for cherry-pick and revert
  completion: add --skip for cherry-pick and revert

 contrib/completion/git-completion.bash | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

-- 
2.23.0.248.g3a9dd8fb08

[PATCH 1/2] completion: merge options for cherry-pick and revert

[Denton Liu]

Since revert and cherry-pick share the same sequencer code, they should
both accept the same command-line options. Merge the
`__git_cherry_pick_inprogress_options` and
`__git_revert_inprogress_options` variables together into
`__git_cherry_pick_revert_inprogress_options` so that the options aren't
unnecessarily duplicated twice.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 contrib/completion/git-completion.bash | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index e087c4bf00..62f8ef600d 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1361,13 +1361,13 @@ _git_checkout ()
 	esac
 }
 
-__git_cherry_pick_inprogress_options="--continue --quit --abort"
+__git_cherry_pick_revert_inprogress_options="--continue --quit --abort"
 
 _git_cherry_pick ()
 {
 	__git_find_repo_path
 	if [ -f "$__git_repo_path"/CHERRY_PICK_HEAD ]; then
-		__gitcomp "$__git_cherry_pick_inprogress_options"
+		__gitcomp "$__git_cherry_pick_revert_inprogress_options"
 		return
 	fi
 
@@ -1376,7 +1376,7 @@ _git_cherry_pick ()
 	case "$cur" in
 	--*)
 		__gitcomp_builtin cherry-pick "" \
-			"$__git_cherry_pick_inprogress_options"
+			"$__git_cherry_pick_revert_inprogress_options"
 		;;
 	*)
 		__git_complete_refs
@@ -2512,20 +2512,18 @@ _git_restore ()
 	esac
 }
 
-__git_revert_inprogress_options="--continue --quit --abort"
-
 _git_revert ()
 {
 	__git_find_repo_path
 	if [ -f "$__git_repo_path"/REVERT_HEAD ]; then
-		__gitcomp "$__git_revert_inprogress_options"
+		__gitcomp "$__git_cherry_pick_revert_inprogress_options"
 		return
 	fi
 	__git_complete_strategy && return
 	case "$cur" in
 	--*)
 		__gitcomp_builtin revert "" \
-			"$__git_revert_inprogress_options"
+			"$__git_cherry_pick_revert_inprogress_options"
 		return
 		;;
 	esac
-- 
2.23.0.248.g3a9dd8fb08

[PATCH 2/2] completion: add --skip for cherry-pick and revert

[Denton Liu]

Even though `--skip` is a valid command-line option for cherry-pick and
revert while they are in progress, it is not completed. Add this missing
option to the completion script.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 contrib/completion/git-completion.bash | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 62f8ef600d..ebcfb5a5af 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1361,7 +1361,7 @@ _git_checkout ()
 	esac
 }
 
-__git_cherry_pick_revert_inprogress_options="--continue --quit --abort"
+__git_cherry_pick_revert_inprogress_options="--continue --quit --abort --skip"
 
 _git_cherry_pick ()
 {
-- 
2.23.0.248.g3a9dd8fb08

Re: [PATCH 1/2] completion: merge options for cherry-pick and revert

[Junio C Hamano]

Denton Liu <liu.denton@gmail.com> writes:

> Since revert and cherry-pick share the same sequencer code, they should
> both accept the same command-line options. Merge the
> `__git_cherry_pick_inprogress_options` and
> `__git_revert_inprogress_options` variables together into
> `__git_cherry_pick_revert_inprogress_options` so that the options aren't
> unnecessarily duplicated twice.

Hmm, will the claim hold true in the future?  I do agree that they
will share continue, quit and abort (and skip) forever, but I am not
bold enough to declare that they will never have some unique option
in addition to the common one only because they "share the same
sequencer" machinery.  It is trivial to add a "if we are in revert,
do this" to the code, and it already works that way.

__git_sequencer_inprogress_common_options="--continue --quit --abort"
__git_cherry_pick_inprogress_options=$__git_sequencer_inprogress_common_options
__git_revert_inprogress_options=$__git_sequencer_inprogress_common_options

may be a bit more future-proof way, perhaps?  The places that use
the variable(s) already correctly distinguish cherry-pick and
revert, so even though the above and your version equally "unify"
the set of common options and allow adding a new common option
(i.e. skip) with equal ease, yours makes giving unique option to one
but not to the other more difficult.

[PATCH v2 0/3] advertise --skip for cherry-pick and revert better

[Denton Liu]

Hi Junio, thanks for the suggestion. I incorporated your suggestion.
Also, I decided to incorporate another patch (3/3) I was planning on
submitting later since both these patchsets have a common purpose:
advertising the `--skip` option better.


Before, the completion script would not complete `--skip` for
cherry-pick and revert, even though it is a valid option while that
operation is in progress. Add that missing completion.

Also, if a cherry-pick or revert is still in progress, mention the
`--skip` flag in the status message so that users are more aware of
their options.

Changes since v1:

* Instead of outright replacing the old completion variables, make them derive
  from a common variable.
* Add patch 3/3 and change the focus of this patchset so that it's focused on
  advertising the `--skip` option better.


Denton Liu (3):
  completion: merge options for cherry-pick and revert
  completion: add --skip for cherry-pick and revert
  status: mention --skip for revert and cherry-pick

 contrib/completion/git-completion.bash | 6 ++++--
 t/t7512-status-help.sh                 | 6 ++++++
 wt-status.c                            | 4 ++++
 3 files changed, 14 insertions(+), 2 deletions(-)

Range-diff against v1:
1:  cdcac97554 < -:  ---------- completion: merge options for cherry-pick and revert
2:  75adf58158 < -:  ---------- completion: add --skip for cherry-pick and revert
-:  ---------- > 1:  862802366a completion: merge options for cherry-pick and revert
-:  ---------- > 2:  c83feb3d6e completion: add --skip for cherry-pick and revert
-:  ---------- > 3:  be64ce1e92 status: mention --skip for revert and cherry-pick
-- 
2.23.0.248.g3a9dd8fb08

[PATCH v2 1/3] completion: merge options for cherry-pick and revert

[Denton Liu]

Since revert and cherry-pick share the same sequencer code, they should
both accept the same command-line options. Derive the
`__git_cherry_pick_inprogress_options` and
`__git_revert_inprogress_options` variables from
`__git_sequencer_inprogress_options` so that the options aren't
unnecessarily duplicated twice.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 contrib/completion/git-completion.bash | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index e087c4bf00..a7d3f58627 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1361,7 +1361,9 @@ _git_checkout ()
 	esac
 }
 
-__git_cherry_pick_inprogress_options="--continue --quit --abort"
+__git_sequencer_inprogress_options="--continue --quit --abort"
+
+__git_cherry_pick_inprogress_options=$__git_sequencer_inprogress_options
 
 _git_cherry_pick ()
 {
@@ -2512,7 +2514,7 @@ _git_restore ()
 	esac
 }
 
-__git_revert_inprogress_options="--continue --quit --abort"
+__git_revert_inprogress_options=$__git_sequencer_inprogress_options
 
 _git_revert ()
 {
-- 
2.23.0.248.g3a9dd8fb08

[PATCH v2 2/3] completion: add --skip for cherry-pick and revert

[Denton Liu]

Even though `--skip` is a valid command-line option for cherry-pick and
revert while they are in progress, it is not completed. Add this missing
option to the completion script.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 contrib/completion/git-completion.bash | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index a7d3f58627..b4651411b2 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1361,7 +1361,7 @@ _git_checkout ()
 	esac
 }
 
-__git_sequencer_inprogress_options="--continue --quit --abort"
+__git_sequencer_inprogress_options="--continue --quit --abort --skip"
 
 __git_cherry_pick_inprogress_options=$__git_sequencer_inprogress_options
 
-- 
2.23.0.248.g3a9dd8fb08

[PATCH v2 3/3] status: mention --skip for revert and cherry-pick

[Denton Liu]

When reverting or cherry-picking, one of the options we can pass the
sequencer is `--skip`. However, unlike rebasing, `--skip` is not
mentioned as a possible option in the status message. Mention it so that
users are more aware of their options.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t7512-status-help.sh | 6 ++++++
 wt-status.c            | 4 ++++
 2 files changed, 10 insertions(+)

diff --git a/t/t7512-status-help.sh b/t/t7512-status-help.sh
index e01c285cbf..66d7a62797 100755
--- a/t/t7512-status-help.sh
+++ b/t/t7512-status-help.sh
@@ -733,6 +733,7 @@ test_expect_success 'status when cherry-picking before resolving conflicts' '
 On branch cherry_branch
 You are currently cherry-picking commit $TO_CHERRY_PICK.
   (fix conflicts and run "git cherry-pick --continue")
+  (use "git cherry-pick --skip" to skip this patch)
   (use "git cherry-pick --abort" to cancel the cherry-pick operation)
 
 Unmerged paths:
@@ -757,6 +758,7 @@ test_expect_success 'status when cherry-picking after resolving conflicts' '
 On branch cherry_branch
 You are currently cherry-picking commit $TO_CHERRY_PICK.
   (all conflicts fixed: run "git cherry-pick --continue")
+  (use "git cherry-pick --skip" to skip this patch)
   (use "git cherry-pick --abort" to cancel the cherry-pick operation)
 
 Changes to be committed:
@@ -778,6 +780,7 @@ test_expect_success 'status when cherry-picking after committing conflict resolu
 On branch cherry_branch
 Cherry-pick currently in progress.
   (run "git cherry-pick --continue" to continue)
+  (use "git cherry-pick --skip" to skip this patch)
   (use "git cherry-pick --abort" to cancel the cherry-pick operation)
 
 nothing to commit (use -u to show untracked files)
@@ -835,6 +838,7 @@ test_expect_success 'status while reverting commit (conflicts)' '
 On branch master
 You are currently reverting commit $TO_REVERT.
   (fix conflicts and run "git revert --continue")
+  (use "git revert --skip" to skip this patch)
   (use "git revert --abort" to cancel the revert operation)
 
 Unmerged paths:
@@ -855,6 +859,7 @@ test_expect_success 'status while reverting commit (conflicts resolved)' '
 On branch master
 You are currently reverting commit $TO_REVERT.
   (all conflicts fixed: run "git revert --continue")
+  (use "git revert --skip" to skip this patch)
   (use "git revert --abort" to cancel the revert operation)
 
 Changes to be committed:
@@ -887,6 +892,7 @@ test_expect_success 'status while reverting after committing conflict resolution
 On branch master
 Revert currently in progress.
   (run "git revert --continue" to continue)
+  (use "git revert --skip" to skip this patch)
   (use "git revert --abort" to cancel the revert operation)
 
 nothing to commit (use -u to show untracked files)
diff --git a/wt-status.c b/wt-status.c
index 9f6c65a580..ad6282c7f8 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1433,6 +1433,8 @@ static void show_cherry_pick_in_progress(struct wt_status *s,
 		else
 			status_printf_ln(s, color,
 				_("  (all conflicts fixed: run \"git cherry-pick --continue\")"));
+		status_printf_ln(s, color,
+			_("  (use \"git cherry-pick --skip\" to skip this patch)"));
 		status_printf_ln(s, color,
 			_("  (use \"git cherry-pick --abort\" to cancel the cherry-pick operation)"));
 	}
@@ -1460,6 +1462,8 @@ static void show_revert_in_progress(struct wt_status *s,
 		else
 			status_printf_ln(s, color,
 				_("  (all conflicts fixed: run \"git revert --continue\")"));
+		status_printf_ln(s, color,
+			_("  (use \"git revert --skip\" to skip this patch)"));
 		status_printf_ln(s, color,
 			_("  (use \"git revert --abort\" to cancel the revert operation)"));
 	}
-- 
2.23.0.248.g3a9dd8fb08

Re: [PATCH v2 3/3] status: mention --skip for revert and cherry-pick

[Junio C Hamano]

Denton Liu <liu.denton@gmail.com> writes:

> When reverting or cherry-picking, one of the options we can pass the
> sequencer is `--skip`. However, unlike rebasing, `--skip` is not
> mentioned as a possible option in the status message. Mention it so that
> users are more aware of their options.

Is this a good thing, though?

Giving up (because you do not have enough time or concentration to
finish the cherry-pick or revert in progress) with --abort, and
committing to the resolution after spending effort to deal with a
conflicted cherry-pick or revert with --continue, are both sensible
actions after seeing the command stop due to conflicts.  Is "--skip"
a recommendable action in the same way?  Doesn't a multi-commit
series often break if you drop just one in the middle, especially
if the series is sensibly structured as a logical progression?

Re: [PATCH v2 3/3] status: mention --skip for revert and cherry-pick

[Denton Liu]

On Tue, Aug 27, 2019 at 02:56:57PM -0700, Junio C Hamano wrote:
> Denton Liu <liu.denton@gmail.com> writes:
> 
> > When reverting or cherry-picking, one of the options we can pass the
> > sequencer is `--skip`. However, unlike rebasing, `--skip` is not
> > mentioned as a possible option in the status message. Mention it so that
> > users are more aware of their options.
> 
> Is this a good thing, though?
> 
> Giving up (because you do not have enough time or concentration to
> finish the cherry-pick or revert in progress) with --abort, and
> committing to the resolution after spending effort to deal with a
> conflicted cherry-pick or revert with --continue, are both sensible
> actions after seeing the command stop due to conflicts.  Is "--skip"
> a recommendable action in the same way?  Doesn't a multi-commit
> series often break if you drop just one in the middle, especially
> if the series is sensibly structured as a logical progression?

I think that the same argument for or against recommending `--skip`
could be made for rebases as well. However, in the rebase case, `--skip`
is recommended whenever `--abort` is recommended. With this patch, I
made it so that revert and cherry-pick would have `--skip` and `--abort`
paired as well.

I'm pretty impartial about making this change but I would suggest if we
choose not to pursue this then we should also drop the `--skip`
recommendation from rebase as well.

Re: [PATCH v2 3/3] status: mention --skip for revert and cherry-pick

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Is this a good thing, though?
>
> Giving up (because you do not have enough time or concentration to
> finish the cherry-pick or revert in progress) with --abort, and
> committing to the resolution after spending effort to deal with a
> conflicted cherry-pick or revert with --continue, are both sensible
> actions after seeing the command stop due to conflicts.  Is "--skip"
> a recommendable action in the same way?  Doesn't a multi-commit
> series often break if you drop just one in the middle, especially
> if the series is sensibly structured as a logical progression?

Addendum.

"rebase" (especially with "-i") is fundamentally different from
"cherry-pick" and it makes tons of sense to suggest "--skip" in the
former.  "rebase -i" is a tool to take a messy work in progress and
polish it by reordering, discarding and combining commits.
"cherry-pick" is to take a finished work already in one integration
track, and transplant to another, often an older maintenance track,
and there is no place for "this conflict is too much to resolve so
let's drop it".

Re: [PATCH v2 3/3] status: mention --skip for revert and cherry-pick

[Denton Liu]

On Tue, Aug 27, 2019 at 07:47:33PM -0700, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
> > Is this a good thing, though?
> >
> > Giving up (because you do not have enough time or concentration to
> > finish the cherry-pick or revert in progress) with --abort, and
> > committing to the resolution after spending effort to deal with a
> > conflicted cherry-pick or revert with --continue, are both sensible
> > actions after seeing the command stop due to conflicts.  Is "--skip"
> > a recommendable action in the same way?  Doesn't a multi-commit
> > series often break if you drop just one in the middle, especially
> > if the series is sensibly structured as a logical progression?
> 
> Addendum.
> 
> "rebase" (especially with "-i") is fundamentally different from
> "cherry-pick" and it makes tons of sense to suggest "--skip" in the
> former.  "rebase -i" is a tool to take a messy work in progress and
> polish it by reordering, discarding and combining commits.
> "cherry-pick" is to take a finished work already in one integration
> track, and transplant to another, often an older maintenance track,
> and there is no place for "this conflict is too much to resolve so
> let's drop it".
> 

I still believe that it's a good idea to let users know what all of
their options are, including the --skip for cherry-pick and revert even
if it may be rare that it's the right thing to do.

That being said, I'm not passionate enough about this change to pursue
it so when you queue this patchset, please drop this patch.