Re: [GSoC][PATCH 0/3] Teach cherry-pick/revert to skip commits

From: Thomas Gummerer <t.gummerer@gmail.com>
Date: Jun 9, 2019, 09:02 UTC
Message-ID: <20190609090235.GC28007@hank.intra.tgummerer.com>
In-Reply-To: <20190608191958.4593-1-rohit.ashiwal265@gmail.com>

On 06/09, Rohit Ashiwal wrote:

Show 6 quoted lines

> git am or rebase advice user to use git am --skip or git rebase --skip
> to skip the commit that has become empty or has risen conflicts. OTOH,
> cherry-pick advice user to use git reset HEAD which on the user’s part
> is annoying and sometimes confusing. This patch series will bring
> consistency between advices of these commands with introduction of
> `--skip` flag to cherry-pick and revert.

Thanks! I found a few minor nits that I missed in my off-list review on the PR. We should give others some time to comment now before you re-send it with the nits fixed (if you agree with them).

One thing that would be good to add to future cover letters is a reminder of how this fits in to your GSoC project, where the overall end goal is to improve the consistency of sequencer commands, and this is one step along that way.

Show 18 quoted lines

> Rohit Ashiwal (3):
>   sequencer: add advice for revert
>   cherry-pick/revert: add --skip option
>   cherry-pick/revert: update hints
> 
>  Documentation/git-cherry-pick.txt |  4 +-
>  Documentation/git-revert.txt      |  4 +-
>  Documentation/sequencer.txt       |  4 ++
>  builtin/commit.c                  | 13 ++++---
>  builtin/revert.c                  |  5 +++
>  sequencer.c                       | 26 ++++++++++++-
>  sequencer.h                       |  1 +
>  t/t3510-cherry-pick-sequence.sh   | 63 +++++++++++++++++++++++++++++++
>  8 files changed, 108 insertions(+), 12 deletions(-)
> 
> PR: https://github.com/r1walz/git/pull/1
> Reviewed-by: Elijah Newren <newren@gmail.com>
> Reviewed-by: Thomas Gummerer <t.gummerer@gmail.com>

Note that the 'Reviewed-by' footer is something that is "given" by the reviewers, and should only be added after they have explicitly given it for the patch/series in question. Instead this should probably mention that Elijah and me reviewed this off-list in the PR you linked to above.

> -- 
> 2.21.0
>