All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] Teach cherry-pick and rebase to ignore redundant commits
@ 2016-01-11  5:00 David Greene
  2016-01-11  5:00 ` [PATCH 1/5] Teach cherry-pick to skip redundant commits if asked David Greene
                   ` (4 more replies)
  0 siblings, 5 replies; 9+ messages in thread
From: David Greene @ 2016-01-11  5:00 UTC (permalink / raw)
  To: git; +Cc: gitster, peff, chris, nhorman

This patch set adds a --skip-redundant-commits option to both
cherry-pick and rebase.  Currently, if cherry-pick applies a
commit that happens to become empty after conflict resolution,
it will abort and ask the user what to do.  This behavior
propagates to rebase when rebase is forced to fall back to
cherry-pick in certain situations.

This abort failure mode makes it difficult to script certain
types of operations that users might expect to work: for example
cherry-picking a commit on top of itself or rebasing a set of
commits back onto its own history.

With --skip-redundant commits users and/or scripts can choose
to have cherry-pick/rebase simply ignore the redundant commit
and move on.  There is already a --keep-redundant-commits flag
so this is really just supplying the natural counter-behavior
users might way.

                     -Davod

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 1/5] Teach cherry-pick to skip redundant commits if asked
  2016-01-11  5:00 [PATCH] Teach cherry-pick and rebase to ignore redundant commits David Greene
@ 2016-01-11  5:00 ` David Greene
  2016-01-11 19:28   ` Junio C Hamano
  2016-01-11  5:00 ` [PATCH 2/5] Add test for cherry-pick --skip-redundant-commits David Greene
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 9+ messages in thread
From: David Greene @ 2016-01-11  5:00 UTC (permalink / raw)
  To: git; +Cc: gitster, peff, chris, nhorman

From: "David A. Greene" <greened@obbligato.org>

Add a "--skip-redundant-commits" option to cherry-pick to avoid
aborting if the cherry-picked commit becomes empty due to
conflict resolution.

Signed-off-by: David A. Greene <greened@obbligato.org>
---
 builtin/revert.c |  7 +++++++
 sequencer.c      | 23 +++++++++++++++++++++++
 sequencer.h      |  1 +
 3 files changed, 31 insertions(+)

diff --git a/builtin/revert.c b/builtin/revert.c
index 56a2c36..befd3ce 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -91,6 +91,12 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 			N_("option for merge strategy"), option_parse_x),
 		{ OPTION_STRING, 'S', "gpg-sign", &opts->gpg_sign, N_("key-id"),
 		  N_("GPG sign commit"), PARSE_OPT_OPTARG, NULL, (intptr_t) "" },
+		/*
+		 * There must be enough OPT_END() here to match the
+		 * size of cp_extra below so that parse_options_concat
+		 * will work.
+		 */
+		OPT_END(),
 		OPT_END(),
 		OPT_END(),
 		OPT_END(),
@@ -106,6 +112,7 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 			OPT_BOOL(0, "allow-empty", &opts->allow_empty, N_("preserve initially empty commits")),
 			OPT_BOOL(0, "allow-empty-message", &opts->allow_empty_message, N_("allow commits with empty messages")),
 			OPT_BOOL(0, "keep-redundant-commits", &opts->keep_redundant_commits, N_("keep redundant, empty commits")),
+			OPT_BOOL(0, "skip-redundant-commits", &opts->skip_redundant_commits, N_("skip redundant, empty commits")),
 			OPT_END(),
 		};
 		if (parse_options_concat(options, ARRAY_SIZE(options), cp_extra))
diff --git a/sequencer.c b/sequencer.c
index 8c58fa2..12361e7 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -185,6 +185,7 @@ static void print_advice(int show_hint, struct replay_opts *opts)
 		else
 			advise(_("after resolving the conflicts, mark the corrected paths\n"
 				 "with 'git add <paths>' or 'git rm <paths>'\n"
+
 				 "and commit the result with 'git commit'"));
 	}
 }
@@ -614,6 +615,28 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
 		res = allow;
 		goto leave;
 	}
+
+	// If told, do not try to commit things that don't make any
+	// changes.
+	if (opts->skip_redundant_commits) {
+		int index_unchanged = is_index_unchanged();
+		if (index_unchanged < 0) {
+			// Something bad happened readhing HEAD or the
+			// index.  Abort.
+			res = index_unchanged;
+			goto leave;
+		}
+		if (index_unchanged) {
+			fputs(_("Skipping redundant commit "), stderr);
+			fputs(find_unique_abbrev(commit->object.oid.hash,
+						 GIT_SHA1_HEXSZ),
+			      stderr);
+			fputs("\n", stderr);
+			res = 0;
+			goto leave;
+		}
+	}
+
 	if (!opts->no_commit)
 		res = run_git_commit(git_path_merge_msg(), opts, allow);
 
diff --git a/sequencer.h b/sequencer.h
index 5ed5cb1..ad6145d 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -34,6 +34,7 @@ struct replay_opts {
 	int allow_empty;
 	int allow_empty_message;
 	int keep_redundant_commits;
+	int skip_redundant_commits;
 
 	int mainline;
 
-- 
2.6.1

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 2/5] Add test for cherry-pick --skip-redundant-commits
  2016-01-11  5:00 [PATCH] Teach cherry-pick and rebase to ignore redundant commits David Greene
  2016-01-11  5:00 ` [PATCH 1/5] Teach cherry-pick to skip redundant commits if asked David Greene
@ 2016-01-11  5:00 ` David Greene
  2016-01-11  5:00 ` [PATCH 3/5] Add --skip-redundant-commits option to rebase David Greene
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 9+ messages in thread
From: David Greene @ 2016-01-11  5:00 UTC (permalink / raw)
  To: git; +Cc: gitster, peff, chris, nhorman

From: "David A. Greene" <greened@obbligato.org>

Test that --skip-redundant commits suppresses an abort when
cherry-picking a redundant commit.

Signed-off-by: David A. Greene <greened@obbligato.org>
---
 t/t3514-cherry-pick-redundant.sh | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)
 create mode 100755 t/t3514-cherry-pick-redundant.sh

diff --git a/t/t3514-cherry-pick-redundant.sh b/t/t3514-cherry-pick-redundant.sh
new file mode 100755
index 0000000..c433344
--- /dev/null
+++ b/t/t3514-cherry-pick-redundant.sh
@@ -0,0 +1,26 @@
+#!/bin/sh
+
+test_description='git cherry-pick tests for redundant commits
+
+This test runs git cherry-pick and tests handling of redundant commits.
+'
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+	test_commit one &&
+	test_commit two &&
+	test_commit three
+'
+
+test_expect_success 'cherry-pick redundant commit fails' '
+	git branch test-start &&
+	git reset --hard HEAD~2 &&
+	git cherry-pick test-start &&
+	test_must_fail git cherry-pick test-start
+'
+
+test_expect_success 'cherry-pick with --skip-redundant-commits' '
+	git cherry-pick --skip-redundant-commits test-start
+'
+
+test_done
-- 
2.6.1

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 3/5] Add --skip-redundant-commits option to rebase
  2016-01-11  5:00 [PATCH] Teach cherry-pick and rebase to ignore redundant commits David Greene
  2016-01-11  5:00 ` [PATCH 1/5] Teach cherry-pick to skip redundant commits if asked David Greene
  2016-01-11  5:00 ` [PATCH 2/5] Add test for cherry-pick --skip-redundant-commits David Greene
@ 2016-01-11  5:00 ` David Greene
  2016-01-11  5:00 ` [PATCH 4/5] Add test for redundant rebase David Greene
  2016-01-11  5:00 ` [PATCH 5/5] Add test for rebase with merges amd redundant commits David Greene
  4 siblings, 0 replies; 9+ messages in thread
From: David Greene @ 2016-01-11  5:00 UTC (permalink / raw)
  To: git; +Cc: gitster, peff, chris, nhorman

From: "David A. Greene" <greened@obbligato.org>

Teach rebase to ignore redundant commits if told.  Since rebase
normally automatically skips redundant commits, this only applies
when it has to use cherry-pick to do its work.  In that case,
pass the --skip-redundant-commits flag to cherry-pick.

This allows scripted use of rebase with options like
--preserve-merges that tend to invoke cherry-pick.

Signed-off-by: David A. Greene <greened@obbligato.org>
---
 git-rebase--interactive.sh | 14 ++++++++++++--
 git-rebase.sh              |  5 +++++
 2 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index c0cfe88..5891ff5 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -269,9 +269,14 @@ pick_one () {
 
 	test -d "$rewritten" &&
 		pick_one_preserving_merges "$@" && return
+	redundant_args=
+	if test -n "$skip_redundant_commits"
+	then
+		redundant_args="--skip-redundant-commits"
+	fi
 	output eval git cherry-pick \
 			${gpg_sign_opt:+$(git rev-parse --sq-quote "$gpg_sign_opt")} \
-			"$strategy_args" $empty_args $ff "$@"
+			"$strategy_args" $empty_args $redundant_args $ff "$@"
 
 	# If cherry-pick dies it leaves the to-be-picked commit unrecorded. Reschedule
 	# previous task so this commit is not lost.
@@ -389,9 +394,14 @@ pick_one_preserving_merges () {
 			echo "$sha1 $(git rev-parse HEAD^0)" >> "$rewritten_list"
 			;;
 		*)
+			redundant_args=
+			if test -n "$skip_redundant_commits"
+			then
+				redundant_args="--skip-redundant-commits"
+			fi
 			output eval git cherry-pick \
 				${gpg_sign_opt:+$(git rev-parse --sq-quote "$gpg_sign_opt")} \
-				"$strategy_args" "$@" ||
+				"$strategy_args" $redundant_args "$@" ||
 				die_with_patch $sha1 "Could not pick $sha1"
 			;;
 		esac
diff --git a/git-rebase.sh b/git-rebase.sh
index af7ba5f..420a54f 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -24,6 +24,7 @@ m,merge!           use merging strategies to rebase
 i,interactive!     let the user edit the list of commits to rebase
 x,exec=!           add exec lines after each commit of the editable list
 k,keep-empty	   preserve empty commits during rebase
+skip-redundant-commits ignore redundant commits during rebase
 f,force-rebase!    force rebase even if branch is up to date
 X,strategy-option=! pass the argument through to the merge strategy
 stat!              display a diffstat of what changed upstream
@@ -86,6 +87,7 @@ action=
 preserve_merges=
 autosquash=
 keep_empty=
+skip_redundant_commits=
 test "$(git config --bool rebase.autosquash)" = "true" && autosquash=t
 gpg_sign_opt=
 
@@ -255,6 +257,9 @@ do
 	--keep-empty)
 		keep_empty=yes
 		;;
+	--skip-redundant-commits)
+		skip_redundant_commits=yes
+		;;
 	--preserve-merges)
 		preserve_merges=t
 		test -z "$interactive_rebase" && interactive_rebase=implied
-- 
2.6.1

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 4/5] Add test for redundant rebase
  2016-01-11  5:00 [PATCH] Teach cherry-pick and rebase to ignore redundant commits David Greene
                   ` (2 preceding siblings ...)
  2016-01-11  5:00 ` [PATCH 3/5] Add --skip-redundant-commits option to rebase David Greene
@ 2016-01-11  5:00 ` David Greene
  2016-01-11  5:00 ` [PATCH 5/5] Add test for rebase with merges amd redundant commits David Greene
  4 siblings, 0 replies; 9+ messages in thread
From: David Greene @ 2016-01-11  5:00 UTC (permalink / raw)
  To: git; +Cc: gitster, peff, chris, nhorman

From: "David A. Greene" <greened@obbligato.org>

Test that rebase --skip-redundant-commits works.

Signed-off-by: David A. Greene <greened@obbligato.org>
---
 t/t3428-rebase-redundant.sh | 100 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 100 insertions(+)
 create mode 100755 t/t3428-rebase-redundant.sh

diff --git a/t/t3428-rebase-redundant.sh b/t/t3428-rebase-redundant.sh
new file mode 100755
index 0000000..eea63e9
--- /dev/null
+++ b/t/t3428-rebase-redundant.sh
@@ -0,0 +1,100 @@
+#!/bin/sh
+
+test_description='git rebase tests for redundant commits
+
+This test runs git rebase and tests handling of redundant commits.
+'
+. ./test-lib.sh
+. "$TEST_DIRECTORY"/lib-rebase.sh
+
+test_expect_success 'setup' '
+	test_commit README &&
+	git checkout -b empty &&
+	test_commit empty1 &&
+	test_commit empty2 &&
+	git commit -m "Empty commit 1" --allow-empty &&
+	test_commit empty3 &&
+	git checkout -b onto master &&
+	test_commit onto1 &&
+	test_commit onto2 &&
+	test_commit onto3 &&
+	git checkout -b dev master &&
+	test_commit dev1 &&
+	test_commit dev2 &&
+	test_commit dev3 &&
+	git checkout -b redundant master &&
+	test_commit onto1 onto1.t onto1 onto1-2 &&
+	test_commit onto2 onto2.t onto2 onto2-2 &&
+	test_commit onto3 onto3.t onto3 onto3-2 &&
+	git checkout -b redundant-empty redundant &&
+	git commit -m "Empty commit 2" --allow-empty &&
+	test_commit empty4
+'
+
+test_expect_success 'default rebase without empty commits' '
+	reset_rebase &&
+	git checkout -b default-non-empty dev &&
+	git rebase --preserve-merges --onto onto master
+'
+
+test_expect_success 'default rebase with empty commits' '
+	reset_rebase &&
+	git checkout -b default-empty empty &&
+	git rebase --preserve-merges --onto onto master
+'
+
+test_expect_success 'default rebase with redundant commits' '
+	reset_rebase &&
+	git checkout -b default-redundant redundant &&
+	git rebase --preserve-merges --onto onto --root
+'
+
+test_expect_success 'rebase --skip-redundant without empty commits' '
+	reset_rebase &&
+	git checkout -b skip-redundant-non-empty dev &&
+	git rebase --skip-redundant-commits --preserve-merges --onto onto master
+'
+
+test_expect_success 'rebase --skip-redundant with empty commits' '
+	reset_rebase &&
+	git checkout -b skip-redundant-empty empty &&
+	git rebase --skip-redundant-commits --preserve-merges --onto onto master
+'
+
+test_expect_success 'rebase --skip-redundant with redundant commits' '
+	reset_rebase &&
+	git checkout -b skip-redundant-redundant redundant &&
+	git rebase --skip-redundant-commits --preserve-merges --onto onto --root
+'
+
+test_expect_success 'rebase --skip-redundant with empty and redundant commits' '
+	reset_rebase &&
+	git checkout -b skip-redundant-empty-redundant redundant-empty &&
+	git rebase --skip-redundant-commits --preserve-merges --onto onto --root
+'
+
+test_expect_success 'rebase --keep-empty --skip-redundant without empty commits' '
+	reset_rebase &&
+	git checkout -b keep-empty-skip-redundant-non-empty dev &&
+	git rebase --keep-empty --skip-redundant-commits --preserve-merges --onto onto master
+'
+
+test_expect_success 'rebase --keep-empty --skip-redundant with empty commits' '
+	reset_rebase &&
+	git checkout -b keep-empty-skip-redundant-empty empty &&
+	git rebase --keep-empty --skip-redundant-commits --preserve-merges --onto onto master
+'
+
+test_expect_success 'rebase --keep-empty --skip-redundant with redundant commits' '
+	reset_rebase &&
+	git checkout -b keep-empty-skip-redundant-redundant redundant &&
+	git rebase --keep-empty --skip-redundant-commits --preserve-merges --onto onto --root
+'
+
+test_expect_success 'rebase --keep-empty --skip-redundant with empty and redundant commits' '
+	reset_rebase &&
+	git checkout -b keep-empty-skip-redundant-empty-redundant redundant-empty &&
+	git rebase --skip-redundant-commits --preserve-merges --onto onto --root
+'
+
+test_done
-- 
2.6.1

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 5/5] Add test for rebase with merges amd redundant commits
  2016-01-11  5:00 [PATCH] Teach cherry-pick and rebase to ignore redundant commits David Greene
                   ` (3 preceding siblings ...)
  2016-01-11  5:00 ` [PATCH 4/5] Add test for redundant rebase David Greene
@ 2016-01-11  5:00 ` David Greene
  2016-01-11 23:50   ` Eric Sunshine
  4 siblings, 1 reply; 9+ messages in thread
From: David Greene @ 2016-01-11  5:00 UTC (permalink / raw)
  To: git; +Cc: gitster, peff, chris, nhorman

From: "David A. Greene" <greened@obbligato.org>

This tests rebase --preserve-merges in the presence of redundant
commits when there are actual erges being rebased.  It primarily
exercises the --skip-redundant-commits option.

Signed-off-by: David A. Greene <greened@obbligato.org>
---
 t/t3429-rebase-redundant-merge.sh | 73 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 73 insertions(+)
 create mode 100755 t/t3429-rebase-redundant-merge.sh

diff --git a/t/t3429-rebase-redundant-merge.sh b/t/t3429-rebase-redundant-merge.sh
new file mode 100755
index 0000000..f677bb1
--- /dev/null
+++ b/t/t3429-rebase-redundant-merge.sh
@@ -0,0 +1,73 @@
+#!/bin/sh
+
+test_description='git rebase tests for redundant commits
+
+This test runs git rebase and tests handling of redundant commits.
+'
+. ./test-lib.sh
+. "$TEST_DIRECTORY"/lib-rebase.sh
+
+# Setup this graph:
+#
+# O
+# o-o-o-o-o R/rebaseOR/rebaseSR/rebasemergeOR/rebasemergeSR
+#  \   /
+#   o-o S
+#
+test_expect_success 'setup' '
+	test_commit README &&
+	git branch O &&
+	git checkout -b branch O &&
+	test_commit branch1 &&
+	test_commit branch2 &&
+	git branch S &&
+	git checkout master &&
+	test_commit master1 &&
+	test_commit master2 &&
+	git merge branch -m "Merge branch to master" &&
+	test_commit merged1 &&
+	git branch R &&
+	git branch rebaseOR &&
+	git branch rebaseSR &&
+	git branch rebasemergeOR &&
+	git branch rebasemergeskipOR &&
+	git branch rebasemergeSR &&
+	git branch rebasemergeskipSR
+'
+
+test_expect_success 'rebase O..R --onto S' '
+	reset_rebase &&
+	git checkout rebaseOR &&
+	git rebase O --onto S
+'
+
+test_expect_success 'rebase S..R --onto S' '
+	reset_rebase &&
+	git checkout rebaseSR &&
+	git rebase S --onto S
+'
+
+test_expect_success 'rebase O..R --onto S preserving merges fails' '
+	reset_rebase &&
+	git checkout rebasemergeOR &&
+	test_must_fail git rebase --preserve-merges O --onto S
+'
+
+test_expect_success 'rebase O..R --onto S preserving merges --skip-redundant' '
+	reset_rebase &&
+	git checkout rebasemergeskipOR &&
+	git rebase --skip-redundant-commits --preserve-merges O --onto S
+'
+
+test_expect_success 'rebase S..R --onto S preserving merges' '
+	reset_rebase &&
+	git checkout rebasemergeSR &&
+	git rebase --preserve-merges S --onto S
+'
+
+test_expect_success 'rebase S..R --onto S preserving merges --skip-redundant' '
+	reset_rebase &&
+	git checkout rebasemergeskipSR &&
+	git rebase --skip-redundant-commits --preserve-merges S --onto S
+'
+test_done
-- 
2.6.1

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/5] Teach cherry-pick to skip redundant commits if asked
  2016-01-11  5:00 ` [PATCH 1/5] Teach cherry-pick to skip redundant commits if asked David Greene
@ 2016-01-11 19:28   ` Junio C Hamano
  2016-01-12  3:10     ` David A. Greene
  0 siblings, 1 reply; 9+ messages in thread
From: Junio C Hamano @ 2016-01-11 19:28 UTC (permalink / raw)
  To: David Greene; +Cc: git, peff, chris, nhorman

David Greene <greened@obbligato.org> writes:

> From: "David A. Greene" <greened@obbligato.org>
>
> Add a "--skip-redundant-commits" option to cherry-pick to avoid
> aborting if the cherry-picked commit becomes empty due to
> conflict resolution.
>
> Signed-off-by: David A. Greene <greened@obbligato.org>
> ---
>  builtin/revert.c |  7 +++++++
>  sequencer.c      | 23 +++++++++++++++++++++++
>  sequencer.h      |  1 +
>  3 files changed, 31 insertions(+)
>
> diff --git a/builtin/revert.c b/builtin/revert.c
> index 56a2c36..befd3ce 100644
> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -91,6 +91,12 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
>  			N_("option for merge strategy"), option_parse_x),
>  		{ OPTION_STRING, 'S', "gpg-sign", &opts->gpg_sign, N_("key-id"),
>  		  N_("GPG sign commit"), PARSE_OPT_OPTARG, NULL, (intptr_t) "" },
> +		/*
> +		 * There must be enough OPT_END() here to match the
> +		 * size of cp_extra below so that parse_options_concat
> +		 * will work.
> +		 */

Good ;-)

> +		OPT_END(),
>  		OPT_END(),
>  		OPT_END(),
>  		OPT_END(),
> @@ -106,6 +112,7 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
>  			OPT_BOOL(0, "allow-empty", &opts->allow_empty, N_("preserve initially empty commits")),
>  			OPT_BOOL(0, "allow-empty-message", &opts->allow_empty_message, N_("allow commits with empty messages")),
>  			OPT_BOOL(0, "keep-redundant-commits", &opts->keep_redundant_commits, N_("keep redundant, empty commits")),
> +			OPT_BOOL(0, "skip-redundant-commits", &opts->skip_redundant_commits, N_("skip redundant, empty commits")),
>  			OPT_END(),
>  		};

This however makes me wonder what should happen when both are
specified.  Shouldn't this patch change the keep_redundant_commits
field from a bool to a tristate that tells us what to do with
redundant ones?  int/enum opts.redundant_commit can take 0 (fail,
which would be the default), 1 (keep) or 2 (skip), or something
like that.



> diff --git a/sequencer.c b/sequencer.c
> index 8c58fa2..12361e7 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -185,6 +185,7 @@ static void print_advice(int show_hint, struct replay_opts *opts)
>  		else
>  			advise(_("after resolving the conflicts, mark the corrected paths\n"
>  				 "with 'git add <paths>' or 'git rm <paths>'\n"
> +

???

>  				 "and commit the result with 'git commit'"));
>  	}
>  }
> @@ -614,6 +615,28 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
>  		res = allow;
>  		goto leave;
>  	}
> +
> +	// If told, do not try to commit things that don't make any
> +	// changes.

No C++/C99 comments, please.

> +	if (opts->skip_redundant_commits) {
> +		int index_unchanged = is_index_unchanged();
> +		if (index_unchanged < 0) {
> +			// Something bad happened readhing HEAD or the
> +			// index.  Abort.
> +			res = index_unchanged;
> +			goto leave;
> +		}
> +		if (index_unchanged) {
> +			fputs(_("Skipping redundant commit "), stderr);
> +			fputs(find_unique_abbrev(commit->object.oid.hash,
> +						 GIT_SHA1_HEXSZ),
> +			      stderr);
> +			fputs("\n", stderr);

This is a bad i18n; we do not know the sentence "Skipping commit X"
is translated to have X at the end of the sentence in all languages.

	fprintf(stderr, _("Skipping ... %s\n"), find_unique_abbrev(...));

would allow it to be tranlated to "Commit X is getting skipped", for
example.

> +			res = 0;
> +			goto leave;
> +		}
> +	}
> +
>  	if (!opts->no_commit)
>  		res = run_git_commit(git_path_merge_msg(), opts, allow);
>  
> diff --git a/sequencer.h b/sequencer.h
> index 5ed5cb1..ad6145d 100644
> --- a/sequencer.h
> +++ b/sequencer.h
> @@ -34,6 +34,7 @@ struct replay_opts {
>  	int allow_empty;
>  	int allow_empty_message;
>  	int keep_redundant_commits;
> +	int skip_redundant_commits;

Continuing from the top-part of the comments, this may be better to
be:

	enum {
            REPLAY_REDUNDANT_FAIL = 0,
            REPLAY_REDUNDANT_KEEP,
            REPLAY_REDUNDANT_SKIP
	} redundant_commits;

or something like that.

>  
>  	int mainline;

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 5/5] Add test for rebase with merges amd redundant commits
  2016-01-11  5:00 ` [PATCH 5/5] Add test for rebase with merges amd redundant commits David Greene
@ 2016-01-11 23:50   ` Eric Sunshine
  0 siblings, 0 replies; 9+ messages in thread
From: Eric Sunshine @ 2016-01-11 23:50 UTC (permalink / raw)
  To: David Greene; +Cc: Git List, Junio C Hamano, Jeff King, chris, nhorman

On Mon, Jan 11, 2016 at 12:00 AM, David Greene <greened@obbligato.org> wrote:
> From: "David A. Greene" <greened@obbligato.org>
>
> This tests rebase --preserve-merges in the presence of redundant
> commits when there are actual erges being rebased.  It primarily

s/erges/merges/

> exercises the --skip-redundant-commits option.
>
> Signed-off-by: David A. Greene <greened@obbligato.org>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/5] Teach cherry-pick to skip redundant commits if asked
  2016-01-11 19:28   ` Junio C Hamano
@ 2016-01-12  3:10     ` David A. Greene
  0 siblings, 0 replies; 9+ messages in thread
From: David A. Greene @ 2016-01-12  3:10 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, peff, chris, nhorman

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

>> +		OPT_END(),
>>  		OPT_END(),
>>  		OPT_END(),
>>  		OPT_END(),
>> @@ -106,6 +112,7 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
>>  			OPT_BOOL(0, "allow-empty", &opts->allow_empty, N_("preserve initially empty commits")),
>>  			OPT_BOOL(0, "allow-empty-message", &opts->allow_empty_message, N_("allow commits with empty messages")),
>>  			OPT_BOOL(0, "keep-redundant-commits", &opts->keep_redundant_commits, N_("keep redundant, empty commits")),
>> +			OPT_BOOL(0, "skip-redundant-commits", &opts->skip_redundant_commits, N_("skip redundant, empty commits")),
>>  			OPT_END(),
>>  		};
>
> This however makes me wonder what should happen when both are
> specified.  Shouldn't this patch change the keep_redundant_commits
> field from a bool to a tristate that tells us what to do with
> redundant ones?  int/enum opts.redundant_commit can take 0 (fail,
> which would be the default), 1 (keep) or 2 (skip), or something
> like that.

This makes good sense.

>> diff --git a/sequencer.c b/sequencer.c
>> index 8c58fa2..12361e7 100644
>> --- a/sequencer.c
>> +++ b/sequencer.c
>> @@ -185,6 +185,7 @@ static void print_advice(int show_hint, struct replay_opts *opts)
>>  		else
>>  			advise(_("after resolving the conflicts, mark the corrected paths\n"
>>  				 "with 'git add <paths>' or 'git rm <paths>'\n"
>> +
>
> ???

Oops.  :)

>> @@ -614,6 +615,28 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
>>  		res = allow;
>>  		goto leave;
>>  	}
>> +
>> +	// If told, do not try to commit things that don't make any
>> +	// changes.
>
> No C++/C99 comments, please.

Will fix.

>> +	if (opts->skip_redundant_commits) {
>> +		int index_unchanged = is_index_unchanged();
>> +		if (index_unchanged < 0) {
>> +			// Something bad happened readhing HEAD or the
>> +			// index.  Abort.
>> +			res = index_unchanged;
>> +			goto leave;
>> +		}
>> +		if (index_unchanged) {
>> +			fputs(_("Skipping redundant commit "), stderr);
>> +			fputs(find_unique_abbrev(commit->object.oid.hash,
>> +						 GIT_SHA1_HEXSZ),
>> +			      stderr);
>> +			fputs("\n", stderr);
>
> This is a bad i18n; we do not know the sentence "Skipping commit X"
> is translated to have X at the end of the sentence in all languages.
>
> 	fprintf(stderr, _("Skipping ... %s\n"), find_unique_abbrev(...));
>
> would allow it to be tranlated to "Commit X is getting skipped", for
> example.

Ok, thank you for the guidance.

>> diff --git a/sequencer.h b/sequencer.h
>> index 5ed5cb1..ad6145d 100644
>> --- a/sequencer.h
>> +++ b/sequencer.h
>> @@ -34,6 +34,7 @@ struct replay_opts {
>>  	int allow_empty;
>>  	int allow_empty_message;
>>  	int keep_redundant_commits;
>> +	int skip_redundant_commits;
>
> Continuing from the top-part of the comments, this may be better to
> be:
>
> 	enum {
>             REPLAY_REDUNDANT_FAIL = 0,
>             REPLAY_REDUNDANT_KEEP,
>             REPLAY_REDUNDANT_SKIP
> 	} redundant_commits;
>
> or something like that.

Agreed.

I've also resumed work on my earlier rebase --keep-redundant-commits
change.  I think I'm going to reorganize things and send the cherry-pick
changes separate from the rebase changes since the latter depends on the
former.  Then all of the redundant commit work on rebase can be in one
series for review.

                        -David

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2016-01-12  3:11 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-01-11  5:00 [PATCH] Teach cherry-pick and rebase to ignore redundant commits David Greene
2016-01-11  5:00 ` [PATCH 1/5] Teach cherry-pick to skip redundant commits if asked David Greene
2016-01-11 19:28   ` Junio C Hamano
2016-01-12  3:10     ` David A. Greene
2016-01-11  5:00 ` [PATCH 2/5] Add test for cherry-pick --skip-redundant-commits David Greene
2016-01-11  5:00 ` [PATCH 3/5] Add --skip-redundant-commits option to rebase David Greene
2016-01-11  5:00 ` [PATCH 4/5] Add test for redundant rebase David Greene
2016-01-11  5:00 ` [PATCH 5/5] Add test for rebase with merges amd redundant commits David Greene
2016-01-11 23:50   ` Eric Sunshine

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.