git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
* [PATCH 0/1] rebase: drop the unwanted -y
@ 2019-02-05 11:49 Johannes Schindelin via GitGitGadget
  2019-02-05 11:49 ` [PATCH 1/1] Revert "rebase: introduce a shortcut for --reschedule-failed-exec" Johannes Schindelin via GitGitGadget
  2019-02-06 18:45 ` [PATCH v2 0/1] rebase: drop the unwanted -y Johannes Schindelin via GitGitGadget
  0 siblings, 2 replies; 6+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2019-02-05 11:49 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano

I totally missed that this patch, despite my pledge to not include it, made
it into master. Sorry about that.

Johannes Schindelin (1):
  Revert "rebase: introduce a shortcut for --reschedule-failed-exec"

 Documentation/git-rebase.txt |  6 ------
 builtin/rebase.c             | 21 ---------------------
 git-legacy-rebase.sh         |  6 ------
 t/t3418-rebase-continue.sh   |  3 ---
 4 files changed, 36 deletions(-)


base-commit: b5101f929789889c2e536d915698f58d5c5c6b7a
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-118%2Fdscho%2Frevert-rebase-y-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-118/dscho/revert-rebase-y-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/118
-- 
gitgitgadget

^ permalink raw reply	[flat|nested] 6+ messages in thread
* [PATCH 1/1] Revert "rebase: introduce a shortcut for --reschedule-failed-exec"
  2019-02-05 11:49 [PATCH 0/1] rebase: drop the unwanted -y Johannes Schindelin via GitGitGadget
@ 2019-02-05 11:49 ` Johannes Schindelin via GitGitGadget
  2019-02-05 17:41   ` Junio C Hamano
  2019-02-06 18:45 ` [PATCH v2 0/1] rebase: drop the unwanted -y Johannes Schindelin via GitGitGadget
  1 sibling, 1 reply; 6+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2019-02-05 11:49 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Johannes Schindelin

From: Johannes Schindelin <johannes.schindelin@gmx.de>

When contributing the patch series, the cover letter tried to convey
clearly that the patch introducing the shortcut -y was included only to
show that it is possible, with a slight bias against it.

During the review, there were a couple reviewers who agreed with this
sentiment, and the author was happy that this patch was not needed and
concurred that it should be dropped. See e.g. Stefan Beller's reply:
<CAGZ79kZL5CRqCDRb6B-EedUm8Z_i4JuSF2=UtwwdRXMitrrOBw@mail.gmail.com>

However, it slipped by the original patch author (yours truly) that the
patch *was* included when the branch made it to `next` and then when it
made it to `master`.

So let's back out that patch before it even slips into an official
release (in which case we would even have to support this unwanted
flag).

This reverts commit 81ef8ee75d5f348d3c71ff633d13d302124e1a5e.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 Documentation/git-rebase.txt |  6 ------
 builtin/rebase.c             | 21 ---------------------
 git-legacy-rebase.sh         |  6 ------
 t/t3418-rebase-continue.sh   |  3 ---
 4 files changed, 36 deletions(-)

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 4dd5853d6e..44ffe2c8c5 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -462,12 +462,6 @@ without an explicit `--interactive`.
 +
 See also INCOMPATIBLE OPTIONS below.
 
--y <cmd>::
-	This is the same as passing `--reschedule-failed-exec` before
-	`-x <cmd>`, i.e. it appends the specified `exec` command and
-	turns on the mode where failed `exec` commands are automatically
-	rescheduled.
-
 --root::
 	Rebase all commits reachable from <branch>, instead of
 	limiting them with an <upstream>.  This allows you to rebase
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 774264bae8..7c77a80687 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -760,23 +760,6 @@ static int parse_opt_interactive(const struct option *opt, const char *arg,
 	return 0;
 }
 
-struct opt_y {
-	struct string_list *list;
-	struct rebase_options *options;
-};
-
-static int parse_opt_y(const struct option *opt, const char *arg, int unset)
-{
-	struct opt_y *o = opt->value;
-
-	if (unset || !arg)
-		return -1;
-
-	o->options->reschedule_failed_exec = 1;
-	string_list_append(o->list, arg);
-	return 0;
-}
-
 static void NORETURN error_on_missing_default_upstream(void)
 {
 	struct branch *current_branch = branch_get(NULL);
@@ -857,7 +840,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	struct string_list strategy_options = STRING_LIST_INIT_NODUP;
 	struct object_id squash_onto;
 	char *squash_onto_name = NULL;
-	struct opt_y opt_y = { .list = &exec, .options = &options };
 	struct option builtin_rebase_options[] = {
 		OPT_STRING(0, "onto", &options.onto_name,
 			   N_("revision"),
@@ -935,9 +917,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		OPT_STRING_LIST('x', "exec", &exec, N_("exec"),
 				N_("add exec lines after each commit of the "
 				   "editable list")),
-		{ OPTION_CALLBACK, 'y', NULL, &opt_y, N_("<cmd>"),
-			N_("same as --reschedule-failed-exec -x <cmd>"),
-			PARSE_OPT_NONEG, parse_opt_y },
 		OPT_BOOL(0, "allow-empty-message",
 			 &options.allow_empty_message,
 			 N_("allow rebasing commits with empty messages")),
diff --git a/git-legacy-rebase.sh b/git-legacy-rebase.sh
index 3bb0682db5..37db5a7ca4 100755
--- a/git-legacy-rebase.sh
+++ b/git-legacy-rebase.sh
@@ -26,7 +26,6 @@ f,force-rebase!    cherry-pick all commits, even if unchanged
 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
-y=!                same as --reschedule-failed-exec -x
 k,keep-empty	   preserve empty commits during rebase
 allow-empty-message allow rebasing commits with empty messages
 stat!              display a diffstat of what changed upstream
@@ -263,11 +262,6 @@ do
 		cmd="${cmd}exec ${1#--exec=}${LF}"
 		test -z "$interactive_rebase" && interactive_rebase=implied
 		;;
-	-y*)
-		reschedule_failed_exec=--reschedule-failed-exec
-		cmd="${cmd}exec ${1#-y}${LF}"
-		test -z "$interactive_rebase" && interactive_rebase=implied
-		;;
 	--interactive)
 		interactive_rebase=explicit
 		;;
diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh
index 25aaacacfc..bdaa511bb0 100755
--- a/t/t3418-rebase-continue.sh
+++ b/t/t3418-rebase-continue.sh
@@ -262,9 +262,6 @@ test_expect_success '--reschedule-failed-exec' '
 	test_must_fail git -c rebase.rescheduleFailedExec=true \
 		rebase -x false HEAD^ 2>err &&
 	grep "^exec false" .git/rebase-merge/git-rebase-todo &&
-	test_i18ngrep "has been rescheduled" err &&
-	git rebase --abort &&
-	test_must_fail git rebase -y false HEAD^ 2>err &&
 	test_i18ngrep "has been rescheduled" err
 '
 
-- 
gitgitgadget

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] Revert "rebase: introduce a shortcut for --reschedule-failed-exec"
  2019-02-05 11:49 ` [PATCH 1/1] Revert "rebase: introduce a shortcut for --reschedule-failed-exec" Johannes Schindelin via GitGitGadget
@ 2019-02-05 17:41   ` Junio C Hamano
  0 siblings, 0 replies; 6+ messages in thread
From: Junio C Hamano @ 2019-02-05 17:41 UTC (permalink / raw)
  To: Johannes Schindelin via GitGitGadget; +Cc: git, Johannes Schindelin

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> When contributing the patch series, the cover letter tried to convey
> clearly that the patch introducing the shortcut -y was included only to
> show that it is possible, with a slight bias against it.
>
> During the review, there were a couple reviewers who agreed with this
> sentiment, and the author was happy that this patch was not needed and
> concurred that it should be dropped. See e.g. Stefan Beller's reply:
> <CAGZ79kZL5CRqCDRb6B-EedUm8Z_i4JuSF2=UtwwdRXMitrrOBw@mail.gmail.com>
>
> However, it slipped by the original patch author (yours truly) that the
> patch *was* included when the branch made it to `next` and then when it
> made it to `master`.
>
> So let's back out that patch before it even slips into an official
> release (in which case we would even have to support this unwanted
> flag).
>
> This reverts commit 81ef8ee75d5f348d3c71ff633d13d302124e1a5e.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---

Thanks for catching before feature freeze, but read the above again
with cooler head.  The revert message is less useful than if you
said 

    The patch was sent for completeness just in case it turns out to
    be too cumbersome not to have a short-hand option, but during
    the discussion, reviewers agreed that [FOR SUCH AND SUCH REASONS
    --- fill in the blank here] we are better off without.  The
    maintainer missed that conclusion and forgot to drop it while
    merging the topic down, and contributors did not notice the
    mistake, either.

As the reason is missing, the only thing a reader can get from it is
"the patch was not intended to be included, but we screwed up".  I
do not see why a more useful "why it wasn't intended to be included"
needs to be hidden behind an external reference.


^ permalink raw reply	[flat|nested] 6+ messages in thread
* [PATCH v2 0/1] rebase: drop the unwanted -y
  2019-02-05 11:49 [PATCH 0/1] rebase: drop the unwanted -y Johannes Schindelin via GitGitGadget
  2019-02-05 11:49 ` [PATCH 1/1] Revert "rebase: introduce a shortcut for --reschedule-failed-exec" Johannes Schindelin via GitGitGadget
@ 2019-02-06 18:45 ` Johannes Schindelin via GitGitGadget
  2019-02-06 18:45   ` [PATCH v2 1/1] Revert "rebase: introduce a shortcut for --reschedule-failed-exec" Johannes Schindelin via GitGitGadget
  2019-02-06 19:28   ` [PATCH v2 0/1] rebase: drop the unwanted -y Junio C Hamano
  1 sibling, 2 replies; 6+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2019-02-06 18:45 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano

I totally missed that this patch made it into next, and then master. Sorry
about that.

Changes since v1:

 * Focused on the "why?" in the commit message.

Johannes Schindelin (1):
  Revert "rebase: introduce a shortcut for --reschedule-failed-exec"

 Documentation/git-rebase.txt |  6 ------
 builtin/rebase.c             | 21 ---------------------
 git-legacy-rebase.sh         |  6 ------
 t/t3418-rebase-continue.sh   |  3 ---
 4 files changed, 36 deletions(-)


base-commit: b5101f929789889c2e536d915698f58d5c5c6b7a
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-118%2Fdscho%2Frevert-rebase-y-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-118/dscho/revert-rebase-y-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/118

Range-diff vs v1:

 1:  e61ebc3060 ! 1:  a4857fb32d Revert "rebase: introduce a shortcut for --reschedule-failed-exec"
     @@ -2,22 +2,20 @@
      
          Revert "rebase: introduce a shortcut for --reschedule-failed-exec"
      
     -    When contributing the patch series, the cover letter tried to convey
     -    clearly that the patch introducing the shortcut -y was included only to
     -    show that it is possible, with a slight bias against it.
     +    This patch was contributed only as a tentative "we could introduce a
     +    convenient short option if we do not want to change the default behavior
     +    in the long run" patch, opening the discussion whether other people
     +    agree with deprecating the current behavior in favor of the rescheduling
     +    behavior.
      
     -    During the review, there were a couple reviewers who agreed with this
     -    sentiment, and the author was happy that this patch was not needed and
     -    concurred that it should be dropped. See e.g. Stefan Beller's reply:
     +    But the consensus on the Git mailing list was that it would make sense
     +    to show a warning in the near future, and flip the default
     +    rebase.rescheduleFailedExec to reschedule failed `exec` commands by
     +    default. See e.g.
          <CAGZ79kZL5CRqCDRb6B-EedUm8Z_i4JuSF2=UtwwdRXMitrrOBw@mail.gmail.com>
      
     -    However, it slipped by the original patch author (yours truly) that the
     -    patch *was* included when the branch made it to `next` and then when it
     -    made it to `master`.
     -
     -    So let's back out that patch before it even slips into an official
     -    release (in which case we would even have to support this unwanted
     -    flag).
     +    So let's back out that patch that added the `-y` short option that we
     +    agreed was not necessary or desirable.
      
          This reverts commit 81ef8ee75d5f348d3c71ff633d13d302124e1a5e.
      

-- 
gitgitgadget

^ permalink raw reply	[flat|nested] 6+ messages in thread
* [PATCH v2 1/1] Revert "rebase: introduce a shortcut for --reschedule-failed-exec"
  2019-02-06 18:45 ` [PATCH v2 0/1] rebase: drop the unwanted -y Johannes Schindelin via GitGitGadget
@ 2019-02-06 18:45   ` Johannes Schindelin via GitGitGadget
  2019-02-06 19:28   ` [PATCH v2 0/1] rebase: drop the unwanted -y Junio C Hamano
  1 sibling, 0 replies; 6+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2019-02-06 18:45 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Johannes Schindelin

From: Johannes Schindelin <johannes.schindelin@gmx.de>

This patch was contributed only as a tentative "we could introduce a
convenient short option if we do not want to change the default behavior
in the long run" patch, opening the discussion whether other people
agree with deprecating the current behavior in favor of the rescheduling
behavior.

But the consensus on the Git mailing list was that it would make sense
to show a warning in the near future, and flip the default
rebase.rescheduleFailedExec to reschedule failed `exec` commands by
default. See e.g.
<CAGZ79kZL5CRqCDRb6B-EedUm8Z_i4JuSF2=UtwwdRXMitrrOBw@mail.gmail.com>

So let's back out that patch that added the `-y` short option that we
agreed was not necessary or desirable.

This reverts commit 81ef8ee75d5f348d3c71ff633d13d302124e1a5e.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 Documentation/git-rebase.txt |  6 ------
 builtin/rebase.c             | 21 ---------------------
 git-legacy-rebase.sh         |  6 ------
 t/t3418-rebase-continue.sh   |  3 ---
 4 files changed, 36 deletions(-)

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 4dd5853d6e..44ffe2c8c5 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -462,12 +462,6 @@ without an explicit `--interactive`.
 +
 See also INCOMPATIBLE OPTIONS below.
 
--y <cmd>::
-	This is the same as passing `--reschedule-failed-exec` before
-	`-x <cmd>`, i.e. it appends the specified `exec` command and
-	turns on the mode where failed `exec` commands are automatically
-	rescheduled.
-
 --root::
 	Rebase all commits reachable from <branch>, instead of
 	limiting them with an <upstream>.  This allows you to rebase
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 774264bae8..7c77a80687 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -760,23 +760,6 @@ static int parse_opt_interactive(const struct option *opt, const char *arg,
 	return 0;
 }
 
-struct opt_y {
-	struct string_list *list;
-	struct rebase_options *options;
-};
-
-static int parse_opt_y(const struct option *opt, const char *arg, int unset)
-{
-	struct opt_y *o = opt->value;
-
-	if (unset || !arg)
-		return -1;
-
-	o->options->reschedule_failed_exec = 1;
-	string_list_append(o->list, arg);
-	return 0;
-}
-
 static void NORETURN error_on_missing_default_upstream(void)
 {
 	struct branch *current_branch = branch_get(NULL);
@@ -857,7 +840,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	struct string_list strategy_options = STRING_LIST_INIT_NODUP;
 	struct object_id squash_onto;
 	char *squash_onto_name = NULL;
-	struct opt_y opt_y = { .list = &exec, .options = &options };
 	struct option builtin_rebase_options[] = {
 		OPT_STRING(0, "onto", &options.onto_name,
 			   N_("revision"),
@@ -935,9 +917,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		OPT_STRING_LIST('x', "exec", &exec, N_("exec"),
 				N_("add exec lines after each commit of the "
 				   "editable list")),
-		{ OPTION_CALLBACK, 'y', NULL, &opt_y, N_("<cmd>"),
-			N_("same as --reschedule-failed-exec -x <cmd>"),
-			PARSE_OPT_NONEG, parse_opt_y },
 		OPT_BOOL(0, "allow-empty-message",
 			 &options.allow_empty_message,
 			 N_("allow rebasing commits with empty messages")),
diff --git a/git-legacy-rebase.sh b/git-legacy-rebase.sh
index 3bb0682db5..37db5a7ca4 100755
--- a/git-legacy-rebase.sh
+++ b/git-legacy-rebase.sh
@@ -26,7 +26,6 @@ f,force-rebase!    cherry-pick all commits, even if unchanged
 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
-y=!                same as --reschedule-failed-exec -x
 k,keep-empty	   preserve empty commits during rebase
 allow-empty-message allow rebasing commits with empty messages
 stat!              display a diffstat of what changed upstream
@@ -263,11 +262,6 @@ do
 		cmd="${cmd}exec ${1#--exec=}${LF}"
 		test -z "$interactive_rebase" && interactive_rebase=implied
 		;;
-	-y*)
-		reschedule_failed_exec=--reschedule-failed-exec
-		cmd="${cmd}exec ${1#-y}${LF}"
-		test -z "$interactive_rebase" && interactive_rebase=implied
-		;;
 	--interactive)
 		interactive_rebase=explicit
 		;;
diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh
index 25aaacacfc..bdaa511bb0 100755
--- a/t/t3418-rebase-continue.sh
+++ b/t/t3418-rebase-continue.sh
@@ -262,9 +262,6 @@ test_expect_success '--reschedule-failed-exec' '
 	test_must_fail git -c rebase.rescheduleFailedExec=true \
 		rebase -x false HEAD^ 2>err &&
 	grep "^exec false" .git/rebase-merge/git-rebase-todo &&
-	test_i18ngrep "has been rescheduled" err &&
-	git rebase --abort &&
-	test_must_fail git rebase -y false HEAD^ 2>err &&
 	test_i18ngrep "has been rescheduled" err
 '
 
-- 
gitgitgadget

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH v2 0/1] rebase: drop the unwanted -y
  2019-02-06 18:45 ` [PATCH v2 0/1] rebase: drop the unwanted -y Johannes Schindelin via GitGitGadget
  2019-02-06 18:45   ` [PATCH v2 1/1] Revert "rebase: introduce a shortcut for --reschedule-failed-exec" Johannes Schindelin via GitGitGadget
@ 2019-02-06 19:28   ` Junio C Hamano
  1 sibling, 0 replies; 6+ messages in thread
From: Junio C Hamano @ 2019-02-06 19:28 UTC (permalink / raw)
  To: Johannes Schindelin via GitGitGadget; +Cc: git

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> I totally missed that this patch made it into next, and then master. Sorry
> about that.

Will queue.  Let's make sure -y won't escape to the released
version(s).

Thanks for a quick update.  


^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-02-06 19:28 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-05 11:49 [PATCH 0/1] rebase: drop the unwanted -y Johannes Schindelin via GitGitGadget
2019-02-05 11:49 ` [PATCH 1/1] Revert "rebase: introduce a shortcut for --reschedule-failed-exec" Johannes Schindelin via GitGitGadget
2019-02-05 17:41   ` Junio C Hamano
2019-02-06 18:45 ` [PATCH v2 0/1] rebase: drop the unwanted -y Johannes Schindelin via GitGitGadget
2019-02-06 18:45   ` [PATCH v2 1/1] Revert "rebase: introduce a shortcut for --reschedule-failed-exec" Johannes Schindelin via GitGitGadget
2019-02-06 19:28   ` [PATCH v2 0/1] rebase: drop the unwanted -y Junio C Hamano

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).