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

[Thomas Gummerer <t.gummerer@gmail.com>]

On 06/16, Rohit Ashiwal wrote:
> Yet another iteration of my patch. We have changed the series a little bit. We
> now have a commit that rename `reset_for_rollback` to `reset_merge`. A lot of
> nit-picks were handled in this revision.

Thanks for your work!  I allowed myself to nitpick a bit more at this
stage :)

One other thing I wanted to point out here is range-diff, which can be
helpful to include for the benefit of reviewers that saw this series
before.  See 'man git-range-diff' or the --range-diff flag in 'git
format-patch' for more info on how it works.  It helps seeing at a
glance what changed between versions of a series.

For the benefit of other reviewers that might find it helpful, here's
one I generated between v3 and v4 of the series::

1:  8f29142755 ! 1:  99279e617c sequencer: add advice for revert
    @@ -25,8 +25,8 @@
     -		error(_("a cherry-pick or revert is already in progress"));
     -		advise(_("try \"git cherry-pick (--continue | --quit | --abort)\""));
     +	enum replay_action action;
    -+	const char *in_progress_advice;
     +	const char *in_progress_error = NULL;
    ++	const char *in_progress_advice = NULL;
     +
     +	if (!sequencer_get_last_command(r, &action)) {
     +		switch (action) {
    @@ -41,7 +41,7 @@
     +			_("try \"git cherry-pick (--continue | --abort | --quit)\"");
     +			break;
     +		default:
    -+			BUG(_("the control must not reach here"));
    ++			BUG(_("unexpected action in create_seq_dir"));
     +		}
     +	}
     +	if (in_progress_error) {
-:  ---------- > 2:  c64aabf2d2 sequencer: rename reset_for_rollback to reset_merge
2:  3bc8678df4 ! 3:  8b483815ca cherry-pick/revert: add --skip option
    @@ -27,7 +27,7 @@
     -'git cherry-pick' --continue
     -'git cherry-pick' --quit
     -'git cherry-pick' --abort
    -+'git cherry-pick' --continue | --skip | --abort | --quit
    ++'git cherry-pick' (--continue | --skip | --abort | --quit)
      
      DESCRIPTION
      -----------
    @@ -42,7 +42,7 @@
     -'git revert' --continue
     -'git revert' --quit
     -'git revert' --abort
    -+'git revert' --continue | --skip | --abort | --quit
    ++'git revert' (--continue | --skip | --abort | --quit)
      
      DESCRIPTION
      -----------
    @@ -97,10 +97,11 @@
      +++ b/sequencer.c
     @@
      
    - static int reset_for_rollback(const struct object_id *oid)
    + static int reset_merge(const struct object_id *oid)
      {
     -	const char *argv[4];	/* reset --merge <arg> + NULL */
    -+	struct argv_array argv = ARGV_ARRAY_INIT;	/* reset --merge <arg> + NULL */
    ++	int ret;
    ++	struct argv_array argv = ARGV_ARRAY_INIT;
      
     -	argv[0] = "reset";
     -	argv[1] = "--merge";
    @@ -112,34 +113,29 @@
     +	if (!is_null_oid(oid))
     +		argv_array_push(&argv, oid_to_hex(oid));
     +
    -+	return run_command_v_opt(argv.argv, RUN_GIT_CMD);
    ++	ret = run_command_v_opt(argv.argv, RUN_GIT_CMD);
    ++	argv_array_clear(&argv);
    ++
    ++	return ret;
      }
      
    --static int rollback_single_pick(struct repository *r)
    -+static int rollback_single_pick(struct repository *r, unsigned int is_skip)
    - {
    - 	struct object_id head_oid;
    - 
    - 	if (!file_exists(git_path_cherry_pick_head(r)) &&
    --	    !file_exists(git_path_revert_head(r)))
    -+	    !file_exists(git_path_revert_head(r)) && !is_skip)
    - 		return error(_("no cherry-pick or revert in progress"));
    - 	if (read_ref_full("HEAD", 0, &head_oid, NULL))
    - 		return error(_("cannot resolve HEAD"));
    --	if (is_null_oid(&head_oid))
    -+	if (is_null_oid(&head_oid) && !is_skip)
    - 		return error(_("cannot abort from a branch yet to be born"));
    - 	return reset_for_rollback(&head_oid);
    - }
    + static int rollback_single_pick(struct repository *r)
     @@
    - 		 * If CHERRY_PICK_HEAD or REVERT_HEAD indicates
    - 		 * a single-cherry-pick in progress, abort that.
    - 		 */
    --		return rollback_single_pick(r);
    -+		return rollback_single_pick(r, 0);
    - 	}
    - 	if (!f)
    - 		return error_errno(_("cannot open '%s'"), git_path_head_file());
    + 	return reset_merge(&head_oid);
    + }
    + 
    ++static int skip_single_pick(void)
    ++{
    ++	struct object_id head;
    ++
    ++	if (read_ref_full("HEAD", 0, &head, NULL))
    ++		return error(_("cannot resolve HEAD"));
    ++	return reset_merge(&head);
    ++}
    ++
    + int sequencer_rollback(struct repository *r, struct replay_opts *opts)
    + {
    + 	FILE *f;
     @@
      	return -1;
      }
    @@ -149,13 +145,35 @@
     +	enum replay_action action = -1;
     +	sequencer_get_last_command(r, &action);
     +
    ++	/*
    ++	 * opts->action tells us which subcommand requested to skip
    ++	 * the commit.
    ++	 */
     +	switch (opts->action) {
     +	case REPLAY_REVERT:
    ++		/*
    ++		 * If .git/REVERT_HEAD exists then we are sure that we are in
    ++		 * the middle of a revert and we allow to skip the commit.
    ++		 */
     +		if (!file_exists(git_path_revert_head(r))) {
    ++			/*
    ++			 * Check if the last instruction executed was related to
    ++			 * revert. If so, we are sure that a revert is in progress.
    ++			 *
    ++			 * NB: single commit revert is also counted in this
    ++			 * definition of "progress" (and was dealt with in the
    ++			 * previous check).
    ++			 */
     +			if (action == REPLAY_REVERT) {
    ++				/*
    ++				 * Check if the user has moved the HEAD, i.e.,
    ++				 * already committed. In this case, we would like
    ++				 * to advise instead of skipping.
    ++				 */
     +				if (!rollback_is_safe())
     +					goto give_advice;
     +				else
    ++					/* skip commit :) */
     +					break;
     +			}
     +			return error(_("no revert in progress"));
    @@ -173,10 +191,10 @@
     +		}
     +		break;
     +	default:
    -+		BUG("the control must not reach here");
    ++		BUG("unexpected action in sequencer_skip");
     +	}
     +
    -+	if (rollback_single_pick(r, 1))
    ++	if (skip_single_pick())
     +		return error(_("failed to skip the commit"));
     +	if (!is_directory(git_path_seq_dir()))
     +		return 0;
    @@ -289,7 +307,7 @@
     +	git commit -a &&
     +	test_path_is_missing .git/CHERRY_PICK_HEAD &&
     +	test_must_fail git cherry-pick --skip 2>advice &&
    -+	test_cmp expect advice
    ++	test_i18ncmp expect advice
     +'
     +
     +test_expect_success 'allow skipping commit but not abort for a new history' '
    @@ -303,7 +321,7 @@
     +	test_must_fail git cherry-pick anotherpick &&
     +	test_must_fail git cherry-pick --abort 2>advice &&
     +	git cherry-pick --skip &&
    -+	test_cmp expect advice
    ++	test_i18ncmp expect advice
     +'
     +
     +test_expect_success 'allow skipping stopped cherry-pick because of untracked file modifications' '
3:  a3cd17540b ! 4:  98618e08f4 cherry-pick/revert: advise using --skip
    @@ -42,8 +42,8 @@
      +++ b/sequencer.c
     @@
      	enum replay_action action;
    - 	const char *in_progress_advice;
      	const char *in_progress_error = NULL;
    + 	const char *in_progress_advice = NULL;
     +	unsigned int advise_skip = file_exists(git_path_revert_head(r)) ||
     +				file_exists(git_path_cherry_pick_head(r));
      
    @@ -62,7 +62,7 @@
     +			_("try \"git cherry-pick (--continue | %s--abort | --quit)\"");
      			break;
      		default:
    - 			BUG(_("the control must not reach here"));
    + 			BUG(_("unexpected action in create_seq_dir"));
     @@
      	}
      	if (in_progress_error) {
    @@ -80,7 +80,7 @@
      --- a/t/t3510-cherry-pick-sequence.sh
      +++ b/t/t3510-cherry-pick-sequence.sh
     @@
    - 	test_cmp expect advice
    + 	test_i18ncmp expect advice
      '
      
     +test_expect_success 'selectively advise --skip while launching another sequence' '
    @@ -92,7 +92,7 @@
     +	EOF
     +	test_must_fail git cherry-pick picked..yetanotherpick &&
     +	test_must_fail git cherry-pick picked..yetanotherpick 2>advice &&
    -+	test_cmp expect advice &&
    ++	test_i18ncmp expect advice &&
     +	cat >expect <<-EOF &&
     +	error: cherry-pick is already in progress
     +	hint: try "git cherry-pick (--continue | --abort | --quit)"
    @@ -100,7 +100,7 @@
     +	EOF
     +	git reset --merge &&
     +	test_must_fail git cherry-pick picked..yetanotherpick 2>advice &&
    -+	test_cmp expect advice
    ++	test_i18ncmp expect advice
     +'
     +
      test_expect_success 'allow skipping commit but not abort for a new history' '

> Rohit Ashiwal (4):
>   sequencer: add advice for revert
>   sequencer: rename reset_for_rollback to reset_merge
>   cherry-pick/revert: add --skip option
>   cherry-pick/revert: advise using --skip
> 
>  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                       | 139 ++++++++++++++++++++++++++----
>  sequencer.h                       |   1 +
>  t/t3510-cherry-pick-sequence.sh   | 122 ++++++++++++++++++++++++++
>  8 files changed, 266 insertions(+), 26 deletions(-)
> 
> -- 
> 2.21.0
>