threads / patch / 51720
Subject: [PATCH 0/2] completion: complete --skip for cherry-pick and revert
Denton LiuBefore, 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
Denton Liu[PATCH 2/2] completion: add --skip for cherry-pick and revert
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
Denton Liu[PATCH 1/2] completion: merge options for cherry-pick and revert
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
Junio C HamanoRe: [PATCH 1/2] completion: merge options for cherry-pick and revert
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.
Denton Liu[PATCH v2 0/3] advertise --skip for cherry-pick and revert better
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
Denton Liu[PATCH v2 1/3] completion: merge options for cherry-pick and revert
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
Denton Liu[PATCH v2 2/3] completion: add --skip for cherry-pick and revert
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
Denton Liu[PATCH v2 3/3] status: mention --skip for revert and cherry-pick
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(+)
t/t7512-status-help.sh, wt-status.c
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
Junio C HamanoRe: [PATCH v2 3/3] status: mention --skip for revert and cherry-pick
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?
Denton LiuRe: [PATCH v2 3/3] status: mention --skip for revert and cherry-pick
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.
Junio C HamanoRe: [PATCH v2 3/3] status: mention --skip for revert and cherry-pick
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".
Denton LiuRe: [PATCH v2 3/3] status: mention --skip for revert and cherry-pick
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.