git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Thomas Gummerer <t.gummerer@gmail.com>
To: Rohit Ashiwal <rohit.ashiwal265@gmail.com>
Cc: git@vger.kernel.org, newren@gmail.com, phillip.wood123@gmail.com,
	martin.agren@gmail.com, jrnieder@gmail.com, gitster@pobox.com
Subject: Re: [GSoC][PATCH v4 0/4] [GSoC][PATCH 0/3] Teach cherry-pick/revert to skip commits
Date: Mon, 17 Jun 2019 09:39:09 +0100	[thread overview]
Message-ID: <20190617083909.GB15217@hank.intra.tgummerer.com> (raw)
In-Reply-To: <20190616082040.9440-1-rohit.ashiwal265@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
> 

  parent reply	other threads:[~2019-06-17  8:39 UTC|newest]

Thread overview: 95+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-06-08 19:19 [GSoC][PATCH 0/3] Teach cherry-pick/revert to skip commits Rohit Ashiwal
2019-06-08 19:19 ` [GSoC][PATCH 1/3] sequencer: add advice for revert Rohit Ashiwal
2019-06-09 17:52   ` Phillip Wood
2019-06-10  5:13     ` Rohit Ashiwal
2019-06-10 10:39       ` Phillip Wood
2019-06-10 13:25         ` Rohit Ashiwal
2019-06-10 17:46           ` Phillip Wood
2019-06-10 16:34         ` Junio C Hamano
2019-06-08 19:19 ` [GSoC][PATCH 2/3] cherry-pick/revert: add --skip option Rohit Ashiwal
2019-06-09  8:37   ` Thomas Gummerer
2019-06-09 18:01   ` Phillip Wood
2019-06-10  5:57     ` Rohit Ashiwal
2019-06-10 10:40       ` Phillip Wood
2019-06-10 13:43         ` Rohit Ashiwal
2019-06-10 17:47           ` Phillip Wood
2019-06-08 19:19 ` [GSoC][PATCH 3/3] cherry-pick/revert: update hints Rohit Ashiwal
2019-06-09  8:42   ` Thomas Gummerer
2019-06-09 18:03   ` Phillip Wood
2019-06-10  5:28     ` Rohit Ashiwal
2019-06-10 10:40       ` Phillip Wood
2019-06-10 13:33         ` Rohit Ashiwal
2019-06-10 17:47           ` Phillip Wood
2019-06-09  9:02 ` [GSoC][PATCH 0/3] Teach cherry-pick/revert to skip commits Thomas Gummerer
2019-06-09 10:06   ` Rohit Ashiwal
2019-06-09 20:00     ` Thomas Gummerer
2019-06-11  7:31 ` [GSoC][PATCH v2 " Rohit Ashiwal
2019-06-11  7:31   ` [GSoC][PATCH v2 1/3] sequencer: add advice for revert Rohit Ashiwal
2019-06-11 21:25     ` Junio C Hamano
2019-06-11  7:31   ` [GSoC][PATCH v2 2/3] cherry-pick/revert: add --skip option Rohit Ashiwal
2019-06-12 13:31     ` Phillip Wood
2019-06-12 18:11       ` Junio C Hamano
2019-06-12 18:57         ` Phillip Wood
2019-06-11  7:31   ` [GSoC][PATCH v2 3/3] cherry-pick/revert: advise using --skip Rohit Ashiwal
2019-06-12 15:16     ` Phillip Wood
2019-06-13  4:05 ` [GSoC][PATCH v3 0/3] Teach cherry-pick/revert to skip commits Rohit Ashiwal
2019-06-13  4:05   ` [GSoC][PATCH v3 1/3] sequencer: add advice for revert Rohit Ashiwal
2019-06-13 17:45     ` Phillip Wood
2019-06-13 19:21       ` Martin Ågren
2019-06-13 20:59         ` Junio C Hamano
2019-06-14  3:44         ` Rohit Ashiwal
2019-06-14  3:43       ` Rohit Ashiwal
2019-06-13  4:05   ` [GSoC][PATCH v3 2/3] cherry-pick/revert: add --skip option Rohit Ashiwal
2019-06-13 17:56     ` Junio C Hamano
2019-06-13 19:57       ` Junio C Hamano
2019-06-14  3:48         ` Rohit Ashiwal
2019-06-14  3:45       ` Rohit Ashiwal
2019-06-14 15:58         ` Junio C Hamano
2019-06-16  7:03       ` Rohit Ashiwal
2019-06-13 17:59     ` Phillip Wood
2019-06-13  4:05   ` [GSoC][PATCH v3 3/3] cherry-pick/revert: advise using --skip Rohit Ashiwal
2019-06-16  8:20 ` [GSoC][PATCH v4 0/4] [GSoC][PATCH 0/3] Teach cherry-pick/revert to skip commits Rohit Ashiwal
2019-06-16  8:20   ` [GSoC][PATCH v4 1/4] sequencer: add advice for revert Rohit Ashiwal
2019-06-17  5:51     ` Thomas Gummerer
2019-06-16  8:20   ` [GSoC][PATCH v4 2/4] sequencer: rename reset_for_rollback to reset_merge Rohit Ashiwal
2019-06-16  8:20   ` [GSoC][PATCH v4 3/4] cherry-pick/revert: add --skip option Rohit Ashiwal
2019-06-17  8:30     ` Thomas Gummerer
2019-06-16  8:20   ` [GSoC][PATCH v4 4/4] cherry-pick/revert: advise using --skip Rohit Ashiwal
2019-06-17  8:39   ` Thomas Gummerer [this message]
2019-06-18 17:06 ` [GSoC][PATCH v5 0/5] Teach cherry-pick/revert to skip commits Rohit Ashiwal
2019-06-18 17:06   ` [GSoC][PATCH v5 1/5] sequencer: add advice for revert Rohit Ashiwal
2019-06-18 17:06   ` [GSoC][PATCH v5 2/5] sequencer: rename reset_for_rollback to reset_merge Rohit Ashiwal
2019-06-18 17:06   ` [GSoC][PATCH v5 3/5] sequencer: use argv_array in reset_merge Rohit Ashiwal
2019-06-18 17:06   ` [GSoC][PATCH v5 4/5] cherry-pick/revert: add --skip option Rohit Ashiwal
2019-06-20  3:40     ` Junio C Hamano
2019-06-20  9:46       ` Rohit Ashiwal
2019-06-20  9:57       ` Phillip Wood
2019-06-20 20:09         ` Junio C Hamano
2019-06-20 10:02     ` Phillip Wood
2019-06-20 10:34       ` Rohit Ashiwal
2019-06-20 11:42         ` Phillip Wood
2019-06-21  7:47           ` Rohit Ashiwal
2019-06-18 17:06   ` [GSoC][PATCH v5 5/5] cherry-pick/revert: advise using --skip Rohit Ashiwal
2019-06-21  9:17 ` [GSoC][PATCH v6 0/5] Teach cherry-pick/revert to skip commits Rohit Ashiwal
2019-06-21  9:17   ` [GSoC][PATCH v6 1/5] sequencer: add advice for revert Rohit Ashiwal
2019-06-21  9:17   ` [GSoC][PATCH v6 2/5] sequencer: rename reset_for_rollback to reset_merge Rohit Ashiwal
2019-06-21  9:17   ` [GSoC][PATCH v6 3/5] sequencer: use argv_array in reset_merge Rohit Ashiwal
2019-06-21  9:17   ` [GSoC][PATCH v6 4/5] cherry-pick/revert: add --skip option Rohit Ashiwal
2019-06-21  9:18   ` [GSoC][PATCH v6 5/5] cherry-pick/revert: advise using --skip Rohit Ashiwal
2019-06-21 19:09   ` [GSoC][PATCH v6 0/5] Teach cherry-pick/revert to skip commits Junio C Hamano
2019-06-23 20:03 ` [GSoC][PATCH v7 0/6] " Rohit Ashiwal
2019-06-23 20:03   ` [GSoC][PATCH v7 1/6] advice: add sequencerInUse config variable Rohit Ashiwal
2019-06-25  9:18     ` Thomas Gummerer
2019-06-23 20:03   ` [GSoC][PATCH v7 2/6] sequencer: add advice for revert Rohit Ashiwal
2019-06-29 14:05     ` Phillip Wood
2019-06-23 20:03   ` [GSoC][PATCH v7 3/6] sequencer: rename reset_for_rollback to reset_merge Rohit Ashiwal
2019-06-23 20:03   ` [GSoC][PATCH v7 4/6] sequencer: use argv_array in reset_merge Rohit Ashiwal
2019-06-23 20:03   ` [GSoC][PATCH v7 5/6] cherry-pick/revert: add --skip option Rohit Ashiwal
2019-06-23 20:03   ` [GSoC][PATCH v7 6/6] cherry-pick/revert: advise using --skip Rohit Ashiwal
2019-07-02  9:11 ` [GSoC][PATCH v8 0/5] Teach cherry-pick/revert to skip commits Rohit Ashiwal
2019-07-02  9:11   ` [GSoC][PATCH v8 1/5] sequencer: add advice for revert Rohit Ashiwal
2019-07-02  9:11   ` [GSoC][PATCH v8 2/5] sequencer: rename reset_for_rollback to reset_merge Rohit Ashiwal
2019-07-02  9:11   ` [GSoC][PATCH v8 3/5] sequencer: use argv_array in reset_merge Rohit Ashiwal
2019-07-02  9:11   ` [GSoC][PATCH v8 4/5] cherry-pick/revert: add --skip option Rohit Ashiwal
2019-07-02  9:11   ` [GSoC][PATCH v8 5/5] cherry-pick/revert: advise using --skip Rohit Ashiwal
2019-07-02 15:26   ` [GSoC][PATCH v8 0/5] Teach cherry-pick/revert to skip commits Phillip Wood

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190617083909.GB15217@hank.intra.tgummerer.com \
    --to=t.gummerer@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=jrnieder@gmail.com \
    --cc=martin.agren@gmail.com \
    --cc=newren@gmail.com \
    --cc=phillip.wood123@gmail.com \
    --cc=rohit.ashiwal265@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).