git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] rebase -i: mark commits that begin empty in todo editor
@ 2020-04-09 23:26 Elijah Newren via GitGitGadget
  2020-04-10  0:50 ` Junio C Hamano
  2020-04-10 17:51 ` [PATCH v2 0/3] " Elijah Newren via GitGitGadget
  0 siblings, 2 replies; 27+ messages in thread
From: Elijah Newren via GitGitGadget @ 2020-04-09 23:26 UTC (permalink / raw)
  To: git; +Cc: phillip.wood123, Johannes.Schindelin, Elijah Newren, Elijah Newren

From: Elijah Newren <newren@gmail.com>

While many users who intentionally create empty commits do not want them
thrown away by a rebase, there are third-party tools that generate empty
commits that a user might not want.  In the past, users have used rebase
to get rid of such commits (a side-effect of the fact that the --apply
backend is not currently capable of keeping them).  While such users
could fire up an interactive rebase and just remove the lines
corresponding to empty commits, that might be difficult if the
third-party tool generates many of them.  Simplify this task for users
by marking such lines with a suffix of " # empty" in the todo list.

Suggested-by: Sami Boukortt <sami@boukortt.com>
Signed-off-by: Elijah Newren <newren@gmail.com>
---
    rebase -i: mark commits that begin empty in todo editor
    
    If this isn't enough, we could talk about resurrecting --no-keep-empty
    (and making --keep-empty just exist to countermand an earlier
    --no-keep-empty), but perhaps this is good enough?

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-757%2Fnewren%2Frebase-mark-empty-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-757/newren/rebase-mark-empty-v1
Pull-Request: https://github.com/git/git/pull/757

 Documentation/git-rebase.txt | 3 ++-
 sequencer.c                  | 5 +++++
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index f7a6033607f..8ab0558aca2 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -620,7 +620,8 @@ commits that started empty, though these are rare in practice.  It
 also drops commits that become empty and has no option for controlling
 this behavior.
 
-The merge backend keeps intentionally empty commits.  Similar to the
+The merge backend keeps intentionally empty commits (though with -i
+they are marked as empty in the todo list editor).  Similar to the
 apply backend, by default the merge backend drops commits that become
 empty unless -i/--interactive is specified (in which case it stops and
 asks the user what to do).  The merge backend also has an
diff --git a/sequencer.c b/sequencer.c
index e528225e787..ce9fd27a878 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -4656,6 +4656,9 @@ static int make_script_with_merges(struct pretty_print_context *pp,
 			strbuf_addf(&buf, "%s %s %s", cmd_pick,
 				    oid_to_hex(&commit->object.oid),
 				    oneline.buf);
+			if (is_empty)
+				strbuf_addf(&buf, " %c empty",
+					    comment_line_char);
 
 			FLEX_ALLOC_STR(entry, string, buf.buf);
 			oidcpy(&entry->entry.oid, &commit->object.oid);
@@ -4861,6 +4864,8 @@ int sequencer_make_script(struct repository *r, struct strbuf *out, int argc,
 		strbuf_addf(out, "%s %s ", insn,
 			    oid_to_hex(&commit->object.oid));
 		pretty_print_commit(&pp, commit, out);
+		if (is_empty)
+			strbuf_addf(out, " %c empty", comment_line_char);
 		strbuf_addch(out, '\n');
 	}
 	return 0;

base-commit: 274b9cc25322d9ee79aa8e6d4e86f0ffe5ced925
-- 
gitgitgadget

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

* Re: [PATCH] rebase -i: mark commits that begin empty in todo editor
  2020-04-09 23:26 [PATCH] rebase -i: mark commits that begin empty in todo editor Elijah Newren via GitGitGadget
@ 2020-04-10  0:50 ` Junio C Hamano
  2020-04-10  2:06   ` Bryan Turner
  2020-04-10 17:51 ` [PATCH v2 0/3] " Elijah Newren via GitGitGadget
  1 sibling, 1 reply; 27+ messages in thread
From: Junio C Hamano @ 2020-04-10  0:50 UTC (permalink / raw)
  To: Elijah Newren via GitGitGadget
  Cc: git, phillip.wood123, Johannes.Schindelin, Elijah Newren

"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Elijah Newren <newren@gmail.com>
>
> While many users who intentionally create empty commits do not want them
> thrown away by a rebase, there are third-party tools that generate empty
> commits that a user might not want.  In the past, users have used rebase
> to get rid of such commits (a side-effect of the fact that the --apply
> backend is not currently capable of keeping them).  While such users
> could fire up an interactive rebase and just remove the lines
> corresponding to empty commits, that might be difficult if the
> third-party tool generates many of them.  Simplify this task for users
> by marking such lines with a suffix of " # empty" in the todo list.
>
> Suggested-by: Sami Boukortt <sami@boukortt.com>
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
>     rebase -i: mark commits that begin empty in todo editor
>     
>     If this isn't enough, we could talk about resurrecting --no-keep-empty
>     (and making --keep-empty just exist to countermand an earlier
>     --no-keep-empty), but perhaps this is good enough?

This does look like an unsatisfying substitute for the real thing,
but perhaps looking for " # empty" and turning them a drop is simple
enough?  Emacs types may do something like

    C-x h C-u M-| 
    sed -e '/ # empty$/s/^pick /drop /'
    <RET>

and vi folks have something similar that begins with a ':'.

But it would not beat just being able to say "--no-keep-empty" (or
"--discard-empty"), would it?

On the other hand, even if there were "--discard-empty" available,
there may still be two classes of empty ones (e.g. ones that were
created by third-party tools, the others that were deliberately
empty) that the user may need and want to sift through, and for such 
a use case, the marking this patch does would be very valuable.

So, from that point of view, this may not be enough, but a "throw
away all" option is not enough, either.  We'd want to have both to
serve such users better.

Thanks.

> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-757%2Fnewren%2Frebase-mark-empty-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-757/newren/rebase-mark-empty-v1
> Pull-Request: https://github.com/git/git/pull/757
>
>  Documentation/git-rebase.txt | 3 ++-
>  sequencer.c                  | 5 +++++
>  2 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
> index f7a6033607f..8ab0558aca2 100644
> --- a/Documentation/git-rebase.txt
> +++ b/Documentation/git-rebase.txt
> @@ -620,7 +620,8 @@ commits that started empty, though these are rare in practice.  It
>  also drops commits that become empty and has no option for controlling
>  this behavior.
>  
> -The merge backend keeps intentionally empty commits.  Similar to the
> +The merge backend keeps intentionally empty commits (though with -i
> +they are marked as empty in the todo list editor).  Similar to the
>  apply backend, by default the merge backend drops commits that become
>  empty unless -i/--interactive is specified (in which case it stops and
>  asks the user what to do).  The merge backend also has an
> diff --git a/sequencer.c b/sequencer.c
> index e528225e787..ce9fd27a878 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -4656,6 +4656,9 @@ static int make_script_with_merges(struct pretty_print_context *pp,
>  			strbuf_addf(&buf, "%s %s %s", cmd_pick,
>  				    oid_to_hex(&commit->object.oid),
>  				    oneline.buf);
> +			if (is_empty)
> +				strbuf_addf(&buf, " %c empty",
> +					    comment_line_char);
>  
>  			FLEX_ALLOC_STR(entry, string, buf.buf);
>  			oidcpy(&entry->entry.oid, &commit->object.oid);
> @@ -4861,6 +4864,8 @@ int sequencer_make_script(struct repository *r, struct strbuf *out, int argc,
>  		strbuf_addf(out, "%s %s ", insn,
>  			    oid_to_hex(&commit->object.oid));
>  		pretty_print_commit(&pp, commit, out);
> +		if (is_empty)
> +			strbuf_addf(out, " %c empty", comment_line_char);
>  		strbuf_addch(out, '\n');
>  	}
>  	return 0;
>
> base-commit: 274b9cc25322d9ee79aa8e6d4e86f0ffe5ced925

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

* Re: [PATCH] rebase -i: mark commits that begin empty in todo editor
  2020-04-10  0:50 ` Junio C Hamano
@ 2020-04-10  2:06   ` Bryan Turner
  2020-04-10  4:57     ` Junio C Hamano
  0 siblings, 1 reply; 27+ messages in thread
From: Bryan Turner @ 2020-04-10  2:06 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Elijah Newren via GitGitGadget, Git Users, phillip.wood123,
	Johannes Schindelin, Elijah Newren

On Thu, Apr 9, 2020 at 5:50 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > From: Elijah Newren <newren@gmail.com>
> >
> > While many users who intentionally create empty commits do not want them
> > thrown away by a rebase, there are third-party tools that generate empty
> > commits that a user might not want.  In the past, users have used rebase
> > to get rid of such commits (a side-effect of the fact that the --apply
> > backend is not currently capable of keeping them).  While such users
> > could fire up an interactive rebase and just remove the lines
> > corresponding to empty commits, that might be difficult if the
> > third-party tool generates many of them.  Simplify this task for users
> > by marking such lines with a suffix of " # empty" in the todo list.
> >
> > Suggested-by: Sami Boukortt <sami@boukortt.com>
> > Signed-off-by: Elijah Newren <newren@gmail.com>
> > ---
> >     rebase -i: mark commits that begin empty in todo editor
> >
> >     If this isn't enough, we could talk about resurrecting --no-keep-empty
> >     (and making --keep-empty just exist to countermand an earlier
> >     --no-keep-empty), but perhaps this is good enough?
>
> This does look like an unsatisfying substitute for the real thing,
> but perhaps looking for " # empty" and turning them a drop is simple
> enough?  Emacs types may do something like
>
>     C-x h C-u M-|
>     sed -e '/ # empty$/s/^pick /drop /'
>     <RET>
>
> and vi folks have something similar that begins with a ':'.
>
> But it would not beat just being able to say "--no-keep-empty" (or
> "--discard-empty"), would it?
>
> On the other hand, even if there were "--discard-empty" available,
> there may still be two classes of empty ones (e.g. ones that were
> created by third-party tools, the others that were deliberately
> empty) that the user may need and want to sift through, and for such
> a use case, the marking this patch does would be very valuable.
>
> So, from that point of view, this may not be enough, but a "throw
> away all" option is not enough, either.  We'd want to have both to
> serve such users better.

This was why I wondered whether it might be worth extending the
--empty option to add another possible value, like "drop-all", that
would allow the caller to say they want to drop all empty
commits--both those that started out empty and those that became
empty. That fourth mode would be distinct from the existing three.
(I'm not sure what to call said mode; "drop-all" looks odd alongside
the existing "drop". I just say "drop-all" here to help illustrate the
idea.)

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

* Re: [PATCH] rebase -i: mark commits that begin empty in todo editor
  2020-04-10  2:06   ` Bryan Turner
@ 2020-04-10  4:57     ` Junio C Hamano
  0 siblings, 0 replies; 27+ messages in thread
From: Junio C Hamano @ 2020-04-10  4:57 UTC (permalink / raw)
  To: Bryan Turner
  Cc: Elijah Newren via GitGitGadget, Git Users, phillip.wood123,
	Johannes Schindelin, Elijah Newren

Bryan Turner <bturner@atlassian.com> writes:

>> So, from that point of view, this may not be enough, but a "throw
>> away all" option is not enough, either.  We'd want to have both to
>> serve such users better.
>
> This was why I wondered whether it might be worth extending the
> --empty option to add another possible value, like "drop-all", that
> would allow the caller to say they want to drop all empty
> commits--both those that started out empty and those that became
> empty.

You are talking about "empty from the beginning" and "has become
empty due to rebasing", but the use case you quoted and responding
to is not about the distinction between those two.

The original scenario that triggered this thread was to clean a
history the user created with git-imerge.  Part of using the tool,
apparently, you can choose to leave empty commits the tool
internally needs to create.  To the "git rebase" command that is
tasked to clean up the history, an empty commit that was left due to
what imerge did, and an empty commit that was originally created by
the end user (perhaps deliberately in order to server as some sort
of a marker) look the same---they both are "empty from the
beginning", not "commits that became no-op as the result of
rebasing".

And in order to help manually sift these two apart, " # empty"
marker would help, as there are three kinds of "pick" in the
instruction stream.  Commits that are not no-op (i.e. without the 
" # empty" mark), commits that imerge made no-op (the user wants
to get rid of) and commits that are no-op because the user wanted
them to be.  The latter two classes would be marked with " # empty"
and the user can selectively apply s/pick/drop/ to them.

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

* [PATCH v2 0/3] rebase -i: mark commits that begin empty in todo editor
  2020-04-09 23:26 [PATCH] rebase -i: mark commits that begin empty in todo editor Elijah Newren via GitGitGadget
  2020-04-10  0:50 ` Junio C Hamano
@ 2020-04-10 17:51 ` Elijah Newren via GitGitGadget
  2020-04-10 17:51   ` [PATCH v2 1/3] " Elijah Newren via GitGitGadget
                     ` (4 more replies)
  1 sibling, 5 replies; 27+ messages in thread
From: Elijah Newren via GitGitGadget @ 2020-04-10 17:51 UTC (permalink / raw)
  To: git; +Cc: phillip.wood123, Johannes.Schindelin, bturner, sami, Elijah Newren

Changes since v1:

 * Resurrect --no-keep-empty (for commits which begin empty), which can be
   used orthogonally to --empty (for commits which become empty). Docs
   updated as well
 * Add a commit improving the error message for --apply being used with
   non-apply arguments

I wanted to base this commit on jt/rebase-allow-duplicate, in particular to
add a patch moving his new --[no-]keep-cherry-picks arguments to be close to
the --empty={drop,keep,ask} and --[no-]keep-empty flags, since all three are
related. But unfortunately that series was based on 2.25, and my series
needs to be based on 2.26. So, for now, I'm just keeping the two series
independent and will submit a fixup patch after these two are both merged
down to move all the arguments near each other.

Elijah Newren (3):
  rebase -i: mark commits that begin empty in todo editor
  rebase: reinstate --no-keep-empty
  rebase: fix an incompatible-options error message

 Documentation/git-rebase.txt      | 36 +++++++++++++++++--------------
 builtin/rebase.c                  | 17 +++++++++------
 sequencer.c                       | 11 ++++++++++
 sequencer.h                       |  2 +-
 t/t3421-rebase-topology-linear.sh | 10 ++++-----
 t/t3424-rebase-empty.sh           | 36 +++++++++++++++++++++++++++++++
 6 files changed, 82 insertions(+), 30 deletions(-)


base-commit: 274b9cc25322d9ee79aa8e6d4e86f0ffe5ced925
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-757%2Fnewren%2Frebase-mark-empty-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-757/newren/rebase-mark-empty-v2
Pull-Request: https://github.com/git/git/pull/757

Range-diff vs v1:

 1:  0d94eea376a = 1:  0d94eea376a rebase -i: mark commits that begin empty in todo editor
 -:  ----------- > 2:  e15c599c874 rebase: reinstate --no-keep-empty
 -:  ----------- > 3:  ee5e42361fc rebase: fix an incompatible-options error message

-- 
gitgitgadget

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

* [PATCH v2 1/3] rebase -i: mark commits that begin empty in todo editor
  2020-04-10 17:51 ` [PATCH v2 0/3] " Elijah Newren via GitGitGadget
@ 2020-04-10 17:51   ` Elijah Newren via GitGitGadget
  2020-04-10 17:51   ` [PATCH v2 2/3] rebase: reinstate --no-keep-empty Elijah Newren via GitGitGadget
                     ` (3 subsequent siblings)
  4 siblings, 0 replies; 27+ messages in thread
From: Elijah Newren via GitGitGadget @ 2020-04-10 17:51 UTC (permalink / raw)
  To: git
  Cc: phillip.wood123, Johannes.Schindelin, bturner, sami,
	Elijah Newren, Elijah Newren

From: Elijah Newren <newren@gmail.com>

While many users who intentionally create empty commits do not want them
thrown away by a rebase, there are third-party tools that generate empty
commits that a user might not want.  In the past, users have used rebase
to get rid of such commits (a side-effect of the fact that the --apply
backend is not currently capable of keeping them).  While such users
could fire up an interactive rebase and just remove the lines
corresponding to empty commits, that might be difficult if the
third-party tool generates many of them.  Simplify this task for users
by marking such lines with a suffix of " # empty" in the todo list.

Suggested-by: Sami Boukortt <sami@boukortt.com>
Signed-off-by: Elijah Newren <newren@gmail.com>
---
 Documentation/git-rebase.txt | 3 ++-
 sequencer.c                  | 5 +++++
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index f7a6033607f..8ab0558aca2 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -620,7 +620,8 @@ commits that started empty, though these are rare in practice.  It
 also drops commits that become empty and has no option for controlling
 this behavior.
 
-The merge backend keeps intentionally empty commits.  Similar to the
+The merge backend keeps intentionally empty commits (though with -i
+they are marked as empty in the todo list editor).  Similar to the
 apply backend, by default the merge backend drops commits that become
 empty unless -i/--interactive is specified (in which case it stops and
 asks the user what to do).  The merge backend also has an
diff --git a/sequencer.c b/sequencer.c
index e528225e787..ce9fd27a878 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -4656,6 +4656,9 @@ static int make_script_with_merges(struct pretty_print_context *pp,
 			strbuf_addf(&buf, "%s %s %s", cmd_pick,
 				    oid_to_hex(&commit->object.oid),
 				    oneline.buf);
+			if (is_empty)
+				strbuf_addf(&buf, " %c empty",
+					    comment_line_char);
 
 			FLEX_ALLOC_STR(entry, string, buf.buf);
 			oidcpy(&entry->entry.oid, &commit->object.oid);
@@ -4861,6 +4864,8 @@ int sequencer_make_script(struct repository *r, struct strbuf *out, int argc,
 		strbuf_addf(out, "%s %s ", insn,
 			    oid_to_hex(&commit->object.oid));
 		pretty_print_commit(&pp, commit, out);
+		if (is_empty)
+			strbuf_addf(out, " %c empty", comment_line_char);
 		strbuf_addch(out, '\n');
 	}
 	return 0;
-- 
gitgitgadget


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

* [PATCH v2 2/3] rebase: reinstate --no-keep-empty
  2020-04-10 17:51 ` [PATCH v2 0/3] " Elijah Newren via GitGitGadget
  2020-04-10 17:51   ` [PATCH v2 1/3] " Elijah Newren via GitGitGadget
@ 2020-04-10 17:51   ` Elijah Newren via GitGitGadget
  2020-04-10 20:37     ` Junio C Hamano
  2020-04-10 17:51   ` [PATCH v2 3/3] rebase: fix an incompatible-options error message Elijah Newren via GitGitGadget
                     ` (2 subsequent siblings)
  4 siblings, 1 reply; 27+ messages in thread
From: Elijah Newren via GitGitGadget @ 2020-04-10 17:51 UTC (permalink / raw)
  To: git
  Cc: phillip.wood123, Johannes.Schindelin, bturner, sami,
	Elijah Newren, Elijah Newren

From: Elijah Newren <newren@gmail.com>

Commit d48e5e21da ("rebase (interactive-backend): make --keep-empty the
default", 2020-02-15) turned --keep-empty (for keeping commits which
start empty) into the default.  That commit viewed this, though as
turning that flag into a no-op.  As such, --no-keep-empty was translated
into undoing a no-op, i.e. also a no-op.  The logic underpinning that
commit was:

  1) 'git commit' errors out on the creation of empty commits without an
     override flag
  2) Once someone determines that the override is worthwhile, it's
     annoying and/or harmful to required them to take extra steps in
     order to keep such commits around (and to repeat such steps with
     every rebase).

While the logic on which the decision was made is sound, the result was
a bit of an overcorrection.  Instead of jumping to having --keep-empty
being the default, it jumped to making --keep-empty the only available
behavior.  There was a simple workaround, though, which was thought to
be good enough at the time.  People could still drop commits which
started empty the same way the could drop any commits: by firing up an
interactive rebase and picking out the commits they didn't want from the
list.  However, there are cases where external tools might create enough
empty commits that picking all of them out is painful.  As such, having
a flag to automatically remove start-empty commits may be beneficial.

Provide users a way to drop commits which start empty using a flag that
existed for years: --no-keep-empty.  Interpret --keep-empty as
countermanding any previous --no-keep-empty, but otherwise leaving
--keep-empty as the default.

This might lead to some slight weirdness since commands like
  git rebase --empty=drop --keep-empty
  git rebase --empty=keep --no-keep-empty
look really weird despite making perfect sense (the first will drop
commits which become empty, but keep commits that started empty; the
second will keep commits which become empty, but drop commits which
started empty).

Reported-by: Bryan Turner <bturner@atlassian.com>
Reported-by: Sami Boukortt <sami@boukortt.com>
Signed-off-by: Elijah Newren <newren@gmail.com>
---
 Documentation/git-rebase.txt      | 37 +++++++++++++++++--------------
 builtin/rebase.c                  | 15 ++++++++-----
 sequencer.c                       |  6 +++++
 sequencer.h                       |  2 +-
 t/t3421-rebase-topology-linear.sh | 10 ++++-----
 t/t3424-rebase-empty.sh           | 36 ++++++++++++++++++++++++++++++
 6 files changed, 76 insertions(+), 30 deletions(-)

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 8ab0558aca2..e58ca37786c 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -277,20 +277,21 @@ See also INCOMPATIBLE OPTIONS below.
 	Other options, like --exec, will use the default of drop unless
 	-i/--interactive is explicitly specified.
 +
-Note that commits which start empty are kept, and commits which are
-clean cherry-picks (as determined by `git log --cherry-mark ...`) are
-always dropped.
+Note that commits which start empty are kept (unless --no-keep-empty
+is specified), and commits which are clean cherry-picks (as determined
+by `git log --cherry-mark ...`) are detected and dropped as a preliminary
+step (unless --keep-cherry-picks is passed).
 +
 See also INCOMPATIBLE OPTIONS below.
 
+--no-keep-empty::
 --keep-empty::
-	No-op.  Rebasing commits that started empty (had no change
-	relative to their parent) used to fail and this option would
-	override that behavior, allowing commits with empty changes to
-	be rebased.  Now commits with no changes do not cause rebasing
-	to halt.
+	Do not keep commits that start empty before the rebase
+	(i.e. that do not change anything from its parent) in the
+	result.  For commits which become empty after rebasing, see
+	the --empty and --keep-cherry-pick flags.
 +
-See also BEHAVIORAL DIFFERENCES and INCOMPATIBLE OPTIONS below.
+See also INCOMPATIBLE OPTIONS below.
 
 --allow-empty-message::
 	No-op.  Rebasing commits with an empty message used to fail
@@ -587,7 +588,7 @@ are incompatible with the following options:
  * --preserve-merges
  * --interactive
  * --exec
- * --keep-empty
+ * --no-keep-empty
  * --empty=
  * --edit-todo
  * --root when used in combination with --onto
@@ -620,13 +621,15 @@ commits that started empty, though these are rare in practice.  It
 also drops commits that become empty and has no option for controlling
 this behavior.
 
-The merge backend keeps intentionally empty commits (though with -i
-they are marked as empty in the todo list editor).  Similar to the
-apply backend, by default the merge backend drops commits that become
-empty unless -i/--interactive is specified (in which case it stops and
-asks the user what to do).  The merge backend also has an
---empty={drop,keep,ask} option for changing the behavior of handling
-commits that become empty.
+The merge backend keeps intentionally empty commits by default (though
+with -i they are marked as empty in the todo list editor, or they can
+be dropped automatically with --no-keep-empty).
+
+Similar to the apply backend, by default the merge backend drops
+commits that become empty unless -i/--interactive is specified (in
+which case it stops and asks the user what to do).  The merge backend
+also has an --empty={drop,keep,ask} option for changing the behavior
+of handling commits that become empty.
 
 Directory rename detection
 ~~~~~~~~~~~~~~~~~~~~~~~~~~
diff --git a/builtin/rebase.c b/builtin/rebase.c
index bff53d5d167..022aa2589a5 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -85,6 +85,7 @@ struct rebase_options {
 	const char *action;
 	int signoff;
 	int allow_rerere_autoupdate;
+	int keep_empty;
 	int autosquash;
 	char *gpg_sign_opt;
 	int autostash;
@@ -100,6 +101,7 @@ struct rebase_options {
 #define REBASE_OPTIONS_INIT {			  	\
 		.type = REBASE_UNSPECIFIED,	  	\
 		.empty = EMPTY_UNSPECIFIED,	  	\
+		.keep_empty = 1,			\
 		.default_backend = "merge",	  	\
 		.flags = REBASE_NO_QUIET, 		\
 		.git_am_opts = ARGV_ARRAY_INIT,		\
@@ -379,6 +381,7 @@ static int run_sequencer_rebase(struct rebase_options *opts,
 
 	git_config_get_bool("rebase.abbreviatecommands", &abbreviate_commands);
 
+	flags |= opts->keep_empty ? TODO_LIST_KEEP_EMPTY : 0;
 	flags |= abbreviate_commands ? TODO_LIST_ABBREVIATE_CMDS : 0;
 	flags |= opts->rebase_merges ? TODO_LIST_REBASE_MERGES : 0;
 	flags |= opts->rebase_cousins > 0 ? TODO_LIST_REBASE_COUSINS : 0;
@@ -442,6 +445,7 @@ static int run_sequencer_rebase(struct rebase_options *opts,
 	return ret;
 }
 
+static void imply_merge(struct rebase_options *opts, const char *option);
 static int parse_opt_keep_empty(const struct option *opt, const char *arg,
 				int unset)
 {
@@ -449,10 +453,8 @@ static int parse_opt_keep_empty(const struct option *opt, const char *arg,
 
 	BUG_ON_OPT_ARG(arg);
 
-	/*
-	 * If we ever want to remap --keep-empty to --empty=keep, insert:
-	 * 	opts->empty = unset ? EMPTY_UNSPECIFIED : EMPTY_KEEP;
-	 */
+	imply_merge(opts, unset ? "--no-keep-empty" : "--keep-empty");
+	opts->keep_empty = !unset;
 	opts->type = REBASE_MERGE;
 	return 0;
 }
@@ -471,7 +473,7 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix)
 		OPT_NEGBIT(0, "ff", &opts.flags, N_("allow fast-forward"),
 			   REBASE_FORCE),
 		{ OPTION_CALLBACK, 'k', "keep-empty", &options, NULL,
-			N_("(DEPRECATED) keep empty commits"),
+			N_("keep commits which start empty"),
 			PARSE_OPT_NOARG | PARSE_OPT_HIDDEN,
 			parse_opt_keep_empty },
 		OPT_BOOL_F(0, "allow-empty-message", &opts.allow_empty_message,
@@ -1162,6 +1164,7 @@ static int run_specific_rebase(struct rebase_options *opts, enum action action)
 		opts->allow_rerere_autoupdate ?
 			opts->allow_rerere_autoupdate == RERERE_AUTOUPDATE ?
 			"--rerere-autoupdate" : "--no-rerere-autoupdate" : "");
+	add_var(&script_snippet, "keep_empty", opts->keep_empty ? "yes" : "");
 	add_var(&script_snippet, "autosquash", opts->autosquash ? "t" : "");
 	add_var(&script_snippet, "gpg_sign_opt", opts->gpg_sign_opt);
 	add_var(&script_snippet, "cmd", opts->cmd);
@@ -1547,7 +1550,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 			       N_("how to handle commits that become empty"),
 			       PARSE_OPT_NONEG, parse_opt_empty),
 		{ OPTION_CALLBACK, 'k', "keep-empty", &options, NULL,
-			N_("(DEPRECATED) keep empty commits"),
+			N_("keep commits which start empty"),
 			PARSE_OPT_NOARG | PARSE_OPT_HIDDEN,
 			parse_opt_keep_empty },
 		OPT_BOOL(0, "autosquash", &options.autosquash,
diff --git a/sequencer.c b/sequencer.c
index ce9fd27a878..d973e19729e 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -4591,6 +4591,7 @@ static int make_script_with_merges(struct pretty_print_context *pp,
 				   struct rev_info *revs, struct strbuf *out,
 				   unsigned flags)
 {
+	int keep_empty = flags & TODO_LIST_KEEP_EMPTY;
 	int rebase_cousins = flags & TODO_LIST_REBASE_COUSINS;
 	int root_with_onto = flags & TODO_LIST_ROOT_WITH_ONTO;
 	struct strbuf buf = STRBUF_INIT, oneline = STRBUF_INIT;
@@ -4645,6 +4646,8 @@ static int make_script_with_merges(struct pretty_print_context *pp,
 		is_empty = is_original_commit_empty(commit);
 		if (!is_empty && (commit->object.flags & PATCHSAME))
 			continue;
+		if (is_empty && !keep_empty)
+			continue;
 
 		strbuf_reset(&oneline);
 		pretty_print_commit(pp, commit, &oneline);
@@ -4822,6 +4825,7 @@ int sequencer_make_script(struct repository *r, struct strbuf *out, int argc,
 	struct pretty_print_context pp = {0};
 	struct rev_info revs;
 	struct commit *commit;
+	int keep_empty = flags & TODO_LIST_KEEP_EMPTY;
 	const char *insn = flags & TODO_LIST_ABBREVIATE_CMDS ? "p" : "pick";
 	int rebase_merges = flags & TODO_LIST_REBASE_MERGES;
 
@@ -4861,6 +4865,8 @@ int sequencer_make_script(struct repository *r, struct strbuf *out, int argc,
 
 		if (!is_empty && (commit->object.flags & PATCHSAME))
 			continue;
+		if (is_empty && !keep_empty)
+			continue;
 		strbuf_addf(out, "%s %s ", insn,
 			    oid_to_hex(&commit->object.oid));
 		pretty_print_commit(&pp, commit, out);
diff --git a/sequencer.h b/sequencer.h
index 718a07426da..7ebaa237340 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -134,7 +134,7 @@ int sequencer_rollback(struct repository *repo, struct replay_opts *opts);
 int sequencer_skip(struct repository *repo, struct replay_opts *opts);
 int sequencer_remove_state(struct replay_opts *opts);
 
-/* #define TODO_LIST_KEEP_EMPTY (1U << 0) */ /* No longer used */
+#define TODO_LIST_KEEP_EMPTY (1U << 0)
 #define TODO_LIST_SHORTEN_IDS (1U << 1)
 #define TODO_LIST_ABBREVIATE_CMDS (1U << 2)
 #define TODO_LIST_REBASE_MERGES (1U << 3)
diff --git a/t/t3421-rebase-topology-linear.sh b/t/t3421-rebase-topology-linear.sh
index cf8dfd6c203..4a9204b4b64 100755
--- a/t/t3421-rebase-topology-linear.sh
+++ b/t/t3421-rebase-topology-linear.sh
@@ -220,14 +220,13 @@ test_have_prereq !REBASE_P || test_run_rebase failure -p
 test_run_rebase () {
 	result=$1
 	shift
-	test_expect_$result "rebase $* --keep-empty" "
+	test_expect_$result "rebase $* --no-keep-empty drops begin-empty commits" "
 		reset_rebase &&
-		git rebase $* --keep-empty c l &&
-		test_cmp_rev c HEAD~3 &&
-		test_linear_range 'd k l' c..
+		git rebase $* --no-keep-empty c l &&
+		test_cmp_rev c HEAD~2 &&
+		test_linear_range 'd l' c..
 	"
 }
-test_run_rebase success --apply
 test_run_rebase success -m
 test_run_rebase success -i
 test_have_prereq !REBASE_P || test_run_rebase success -p
@@ -242,7 +241,6 @@ test_run_rebase () {
 		test_linear_range 'd k l' j..
 	"
 }
-test_run_rebase success --apply
 test_run_rebase success -m
 test_run_rebase success -i
 test_have_prereq !REBASE_P || test_run_rebase success -p
diff --git a/t/t3424-rebase-empty.sh b/t/t3424-rebase-empty.sh
index e1e30517ea6..5e1045a0afc 100755
--- a/t/t3424-rebase-empty.sh
+++ b/t/t3424-rebase-empty.sh
@@ -123,6 +123,42 @@ test_expect_success 'rebase --interactive uses default of --empty=ask' '
 	test_cmp expect actual
 '
 
+test_expect_success 'rebase --merge --empty=drop --keep-empty' '
+	git checkout -B testing localmods &&
+	git rebase --merge --empty=drop --keep-empty upstream &&
+
+	test_write_lines D C B A >expect &&
+	git log --format=%s >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'rebase --merge --empty=drop --no-keep-empty' '
+	git checkout -B testing localmods &&
+	git rebase --merge --empty=drop --no-keep-empty upstream &&
+
+	test_write_lines C B A >expect &&
+	git log --format=%s >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'rebase --merge --empty=keep --keep-empty' '
+	git checkout -B testing localmods &&
+	git rebase --merge --empty=keep --keep-empty upstream &&
+
+	test_write_lines D C2 C B A >expect &&
+	git log --format=%s >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'rebase --merge --empty=keep --no-keep-empty' '
+	git checkout -B testing localmods &&
+	git rebase --merge --empty=keep --no-keep-empty upstream &&
+
+	test_write_lines C2 C B A >expect &&
+	git log --format=%s >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'rebase --merge does not leave state laying around' '
 	git checkout -B testing localmods~2 &&
 	git rebase --merge upstream &&
-- 
gitgitgadget


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

* [PATCH v2 3/3] rebase: fix an incompatible-options error message
  2020-04-10 17:51 ` [PATCH v2 0/3] " Elijah Newren via GitGitGadget
  2020-04-10 17:51   ` [PATCH v2 1/3] " Elijah Newren via GitGitGadget
  2020-04-10 17:51   ` [PATCH v2 2/3] rebase: reinstate --no-keep-empty Elijah Newren via GitGitGadget
@ 2020-04-10 17:51   ` Elijah Newren via GitGitGadget
  2020-04-10 20:42   ` [PATCH v2 0/3] rebase -i: mark commits that begin empty in todo editor Junio C Hamano
  2020-04-11  2:44   ` [PATCH v3 0/4] " Elijah Newren via GitGitGadget
  4 siblings, 0 replies; 27+ messages in thread
From: Elijah Newren via GitGitGadget @ 2020-04-10 17:51 UTC (permalink / raw)
  To: git
  Cc: phillip.wood123, Johannes.Schindelin, bturner, sami,
	Elijah Newren, Elijah Newren

From: Elijah Newren <newren@gmail.com>

When the user specifies the apply backend with options that only work
with the merge backend, such as

    git rebase --apply --exec /bin/true HEAD~3

the error message has always been

    fatal: --exec requires an interactive rebase

This error message is misleading and was one of the reasons we renamed
the interactive backend to the merge backend.  Update the error message
to state that these options merely require use of the merge backend.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 builtin/rebase.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 022aa2589a5..0e223a96d46 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -561,7 +561,7 @@ static void imply_merge(struct rebase_options *opts, const char *option)
 {
 	switch (opts->type) {
 	case REBASE_APPLY:
-		die(_("%s requires an interactive rebase"), option);
+		die(_("%s requires the merge backend"), option);
 		break;
 	case REBASE_MERGE:
 	case REBASE_PRESERVE_MERGES:
-- 
gitgitgadget

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

* Re: [PATCH v2 2/3] rebase: reinstate --no-keep-empty
  2020-04-10 17:51   ` [PATCH v2 2/3] rebase: reinstate --no-keep-empty Elijah Newren via GitGitGadget
@ 2020-04-10 20:37     ` Junio C Hamano
  2020-04-10 21:41       ` Elijah Newren
  0 siblings, 1 reply; 27+ messages in thread
From: Junio C Hamano @ 2020-04-10 20:37 UTC (permalink / raw)
  To: Elijah Newren via GitGitGadget
  Cc: git, phillip.wood123, Johannes.Schindelin, bturner, sami, Elijah Newren

"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:

> That commit viewed this, though as
> turning that flag into a no-op.

Sorry, but I do not understand this sentence.

> Provide users a way to drop commits which start empty using a flag that
> existed for years: --no-keep-empty.  Interpret --keep-empty as
> countermanding any previous --no-keep-empty, but otherwise leaving
> --keep-empty as the default.

But everything after that sentence down to here was very clear.

> This might lead to some slight weirdness since commands like
>   git rebase --empty=drop --keep-empty
>   git rebase --empty=keep --no-keep-empty
> look really weird despite making perfect sense (the first will drop
> commits which become empty, but keep commits that started empty; the
> second will keep commits which become empty, but drop commits which
> started empty).

That is true.  Do we leave it to others (or our later selves) to
think about the UI further?  That is fine by me, but in that case we
may want to add " We may want to rethink the option names later",
perhaps?

> +--no-keep-empty::
>  --keep-empty::
> +	Do not keep commits that start empty before the rebase
> +	(i.e. that do not change anything from its parent) in the
> +	result.  For commits which become empty after rebasing, see
> +	the --empty and --keep-cherry-pick flags.

keep-cherry-pick will appear later, not here, right?

Thanks.

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

* Re: [PATCH v2 0/3] rebase -i: mark commits that begin empty in todo editor
  2020-04-10 17:51 ` [PATCH v2 0/3] " Elijah Newren via GitGitGadget
                     ` (2 preceding siblings ...)
  2020-04-10 17:51   ` [PATCH v2 3/3] rebase: fix an incompatible-options error message Elijah Newren via GitGitGadget
@ 2020-04-10 20:42   ` Junio C Hamano
  2020-04-10 21:04     ` Junio C Hamano
  2020-04-10 21:29     ` Junio C Hamano
  2020-04-11  2:44   ` [PATCH v3 0/4] " Elijah Newren via GitGitGadget
  4 siblings, 2 replies; 27+ messages in thread
From: Junio C Hamano @ 2020-04-10 20:42 UTC (permalink / raw)
  To: Elijah Newren via GitGitGadget
  Cc: git, phillip.wood123, Johannes.Schindelin, bturner, sami, Elijah Newren

"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:

> I wanted to base this commit on jt/rebase-allow-duplicate, in particular to
> add a patch moving his new --[no-]keep-cherry-picks arguments to be close to
> the --empty={drop,keep,ask} and --[no-]keep-empty flags, since all three are
> related. But unfortunately that series was based on 2.25, and my series
> needs to be based on 2.26.

Even though this one might qualify as a regression fix to be based
on an older track, the other one is a new "feature" and is not even
in 'next' yet, so there is no reason why we must keep its base on
maintenance tracks (perhaps its earliest round was first queued when
2.25 was the latest tagged release?)

So I am OK to rebase the other topic to v2.26.0; would that help?  I
already saw there was some entanglement with the other topic in one
of the patches in this series, so...

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

* Re: [PATCH v2 0/3] rebase -i: mark commits that begin empty in todo editor
  2020-04-10 20:42   ` [PATCH v2 0/3] rebase -i: mark commits that begin empty in todo editor Junio C Hamano
@ 2020-04-10 21:04     ` Junio C Hamano
  2020-04-10 21:14       ` Junio C Hamano
  2020-04-10 21:29     ` Junio C Hamano
  1 sibling, 1 reply; 27+ messages in thread
From: Junio C Hamano @ 2020-04-10 21:04 UTC (permalink / raw)
  To: Elijah Newren via GitGitGadget
  Cc: git, phillip.wood123, Johannes.Schindelin, bturner, sami, Elijah Newren

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

> "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> I wanted to base this commit on jt/rebase-allow-duplicate, in particular to
>> add a patch moving his new --[no-]keep-cherry-picks arguments to be close to
>> the --empty={drop,keep,ask} and --[no-]keep-empty flags, since all three are
>> related. But unfortunately that series was based on 2.25, and my series
>> needs to be based on 2.26.
>
> Even though this one might qualify as a regression fix to be based
> on an older track, the other one is a new "feature" and is not even
> in 'next' yet, so there is no reason why we must keep its base on
> maintenance tracks (perhaps its earliest round was first queued when
> 2.25 was the latest tagged release?)
>
> So I am OK to rebase the other topic to v2.26.0; would that help?  I
> already saw there was some entanglement with the other topic in one
> of the patches in this series, so...

This is a total tangent, but when I tried to rebase
jt/rebase-allow-duplicate that builds directly on top of v2.25.0 to
a newer base, after resolving conflicts, "commit -a" and "rebase
--continue", somewhere I seem to have mangled the authorship.  It
could entirely be a driver error, or it may be a bug in "rebase",
especially with recent backend change.  I am planning to come back
to it later to figure out if there is such a bug, but I'd need to
recover from the authorship screwup first, so it may take some
time.


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

* Re: [PATCH v2 0/3] rebase -i: mark commits that begin empty in todo editor
  2020-04-10 21:04     ` Junio C Hamano
@ 2020-04-10 21:14       ` Junio C Hamano
  2020-04-10 22:11         ` Elijah Newren
  0 siblings, 1 reply; 27+ messages in thread
From: Junio C Hamano @ 2020-04-10 21:14 UTC (permalink / raw)
  To: Elijah Newren via GitGitGadget
  Cc: git, phillip.wood123, Johannes.Schindelin, bturner, sami, Elijah Newren

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

> This is a total tangent, but when I tried to rebase
> jt/rebase-allow-duplicate that builds directly on top of v2.25.0 to
> a newer base, after resolving conflicts, "commit -a" and "rebase
> --continue", somewhere I seem to have mangled the authorship.  It
> could entirely be a driver error, or it may be a bug in "rebase",
> especially with recent backend change.  I am planning to come back
> to it later to figure out if there is such a bug, but I'd need to
> recover from the authorship screwup first, so it may take some
> time.

I think I know how it happens now.

52e8d1942c662 == jt/rebase-allow-duplicate

Attempting to rebase this on top of the 'master', i.e.

    $ git rebase --onto master 52e8d1942c662^ 52e8d1942c662

results in a merge conflict, but it is easy to resolve in the
working tree.  Then after "git add -u" to record the resolution
in the index

    $ git commit
    $ git rebase --continue

gives me the authorship credit.

Back when the default rebase backend was 'apply', making the above
"mistake" was not even possible; "git commit" would have given me an
empty log buffer to edit, without pre-filling anything, to help me
realize that I shouldn't commit.  

With the sequencer backend, however, the above procedure happily
records the commit under the author's name, it seems.

I am not sure if that is a bug.  I am inclined to say so.

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

* Re: [PATCH v2 0/3] rebase -i: mark commits that begin empty in todo editor
  2020-04-10 20:42   ` [PATCH v2 0/3] rebase -i: mark commits that begin empty in todo editor Junio C Hamano
  2020-04-10 21:04     ` Junio C Hamano
@ 2020-04-10 21:29     ` Junio C Hamano
  2020-04-10 22:13       ` Elijah Newren
  1 sibling, 1 reply; 27+ messages in thread
From: Junio C Hamano @ 2020-04-10 21:29 UTC (permalink / raw)
  To: Elijah Newren, Jonathan Tan
  Cc: git, phillip.wood123, Johannes.Schindelin, bturner, sami,
	Elijah Newren via GitGitGadget

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

> So I am OK to rebase the other topic to v2.26.0; would that help?  I
> already saw there was some entanglement with the other topic in one
> of the patches in this series, so...

So I've done such a rebasing and queued the result to 'pu'.
jt/rebase-allow-duplicate used to be directly on v2.25.0 but now it
is on top of 'master'==9fadedd6, and these three patches are queued
directly on top of that.  Hopefully I can push out the result of
today's integration with other topics in an hour or so.

Thanks.

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

* Re: [PATCH v2 2/3] rebase: reinstate --no-keep-empty
  2020-04-10 20:37     ` Junio C Hamano
@ 2020-04-10 21:41       ` Elijah Newren
  0 siblings, 0 replies; 27+ messages in thread
From: Elijah Newren @ 2020-04-10 21:41 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Elijah Newren via GitGitGadget, Git Mailing List, Phillip Wood,
	Johannes Schindelin, Bryan Turner, Sami Boukortt

On Fri, Apr 10, 2020 at 1:38 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > That commit viewed this, though as
> > turning that flag into a no-op.
>
> Sorry, but I do not understand this sentence.

Hmm...it wouldn't hurt to just drop the two sentences with "no-op" in them.

> > Provide users a way to drop commits which start empty using a flag that
> > existed for years: --no-keep-empty.  Interpret --keep-empty as
> > countermanding any previous --no-keep-empty, but otherwise leaving
> > --keep-empty as the default.
>
> But everything after that sentence down to here was very clear.

:-)

> > This might lead to some slight weirdness since commands like
> >   git rebase --empty=drop --keep-empty
> >   git rebase --empty=keep --no-keep-empty
> > look really weird despite making perfect sense (the first will drop
> > commits which become empty, but keep commits that started empty; the
> > second will keep commits which become empty, but drop commits which
> > started empty).
>
> That is true.  Do we leave it to others (or our later selves) to
> think about the UI further?  That is fine by me, but in that case we
> may want to add " We may want to rethink the option names later",
> perhaps?

The names might not be great, but since --no-keep-empty and
--keep-empty already existed for years and prior to 2.16 and were
about how to handle commits which started empty, it seemed reasonable
to (re)use them.  My personal preference would be that for commits
whose keep/drop state can be determined solely by looking at its
contents of the commit before the rebase (whether that's because they
start empty or for whatever other reasons), I'd rather recommend that
people just fire up an interactive rebase and pick out the commits
they don't want to keep.  So, my own bias would be that I wouldn't
recommend that new people use either --keep-empty or --no-keep-empty.
In my mind, the --[no-]keep-empty flags are just for backward
compatibility, so going through effort to rename or alias doesn't seem
like it makes a lot of sense.

I don't have a strongly held position here and I'm happy to hear
alternate viewpoints, but that's the thought process I went through
when creating this change.

> > +--no-keep-empty::
> >  --keep-empty::
> > +     Do not keep commits that start empty before the rebase
> > +     (i.e. that do not change anything from its parent) in the
> > +     result.  For commits which become empty after rebasing, see
> > +     the --empty and --keep-cherry-pick flags.
>
> keep-cherry-pick will appear later, not here, right?

--keep-cherry-pick is the new flag added by jt/rebase-allow-duplicate.
I guess I should technically probably wait until I create a commit on
top of that series and this one to mention it.  Or rebase Jonathan's
commit, and combine the two series (adding his second since mine might
be viewed as a bugfix).

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

* Re: [PATCH v2 0/3] rebase -i: mark commits that begin empty in todo editor
  2020-04-10 21:14       ` Junio C Hamano
@ 2020-04-10 22:11         ` Elijah Newren
  0 siblings, 0 replies; 27+ messages in thread
From: Elijah Newren @ 2020-04-10 22:11 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Elijah Newren via GitGitGadget, Git Mailing List, Phillip Wood,
	Johannes Schindelin, Bryan Turner, Sami Boukortt

On Fri, Apr 10, 2020 at 2:14 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Junio C Hamano <gitster@pobox.com> writes:
>
> > This is a total tangent, but when I tried to rebase
> > jt/rebase-allow-duplicate that builds directly on top of v2.25.0 to
> > a newer base, after resolving conflicts, "commit -a" and "rebase
> > --continue", somewhere I seem to have mangled the authorship.  It
> > could entirely be a driver error, or it may be a bug in "rebase",
> > especially with recent backend change.  I am planning to come back
> > to it later to figure out if there is such a bug, but I'd need to
> > recover from the authorship screwup first, so it may take some
> > time.
>
> I think I know how it happens now.
>
> 52e8d1942c662 == jt/rebase-allow-duplicate
>
> Attempting to rebase this on top of the 'master', i.e.
>
>     $ git rebase --onto master 52e8d1942c662^ 52e8d1942c662
>
> results in a merge conflict, but it is easy to resolve in the
> working tree.  Then after "git add -u" to record the resolution
> in the index
>
>     $ git commit
>     $ git rebase --continue
>
> gives me the authorship credit.
>
> Back when the default rebase backend was 'apply', making the above
> "mistake" was not even possible; "git commit" would have given me an
> empty log buffer to edit, without pre-filling anything, to help me
> realize that I shouldn't commit.

"was not even possible" and "to help me realize that I shouldn't
commit" seem slightly at odds, but I think you're saying that the UI
was a bit better at helping you abort before you continued with such
an accident.  Perhaps it could be improved even more while we're here?

> With the sequencer backend, however, the above procedure happily
> records the commit under the author's name, it seems.
>
> I am not sure if that is a bug.  I am inclined to say so.

I am inclined to say so too, but it does get a bit more
complicated...with just a few cases I can think of off the top of my
head.

In the case of the merge backend, specifically with --interactive, it
does make sense to use "break" or "edit" commands and then allow the
user to run "git commit" in a stopped rebase.

It also could make sense in some cases to hit a conflict, allow the
user to run "git reset HEAD" and then start fixing and staging pieces
of the changes for a commit and then manually commit the pieces (which
then gain a different author and different commit message and
rearranged/subsetted/modified contents, with the expectation that
they'd probably do a Patched-based-on-work-by tag or equivalent).

But if we're in the middle of a rebase-apply, or in the middle of a
rebase-merge and resolving a conflict (i.e. not cases like the "break"
or "edit" or just ran "git reset HEAD" cases), or in the middle of a
cherry-pick and resolving a conflict, then I'm inclined to agree with
you that calling "git commit" represents an accident by the user that
should result in an error.  For my purposes here, "git add -u"
shouldn't mean we're no longer "busy resolving a conflict" (otherwise
git commit would have already stopped you); that state shouldn't go
away until "rebase --continue" or "cherry-pick --continue" or "git
reset HEAD" is executed.  But it gets slightly weird, though, since
"git reset HEAD" operates differently in merge and apply modes (with
apply still attempting to commit something afterwards and merge
realizing that a reset means that commit was tossed).  So it might
take a little more care and history digging to make sure that the
various cases are handled sanely.

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

* Re: [PATCH v2 0/3] rebase -i: mark commits that begin empty in todo editor
  2020-04-10 21:29     ` Junio C Hamano
@ 2020-04-10 22:13       ` Elijah Newren
  2020-04-10 22:30         ` Junio C Hamano
  0 siblings, 1 reply; 27+ messages in thread
From: Elijah Newren @ 2020-04-10 22:13 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Jonathan Tan, Git Mailing List, Phillip Wood,
	Johannes Schindelin, Bryan Turner, Sami Boukortt,
	Elijah Newren via GitGitGadget

On Fri, Apr 10, 2020 at 2:29 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Junio C Hamano <gitster@pobox.com> writes:
>
> > So I am OK to rebase the other topic to v2.26.0; would that help?  I
> > already saw there was some entanglement with the other topic in one
> > of the patches in this series, so...
>
> So I've done such a rebasing and queued the result to 'pu'.
> jt/rebase-allow-duplicate used to be directly on v2.25.0 but now it
> is on top of 'master'==9fadedd6, and these three patches are queued
> directly on top of that.  Hopefully I can push out the result of
> today's integration with other topics in an hour or so.
>
> Thanks.

Thanks, but wouldn't I want to base his patch on top of mine rather
than vice versa?  (Since mine might be labelled bugfix, and his is a
new feature?)

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

* Re: [PATCH v2 0/3] rebase -i: mark commits that begin empty in todo editor
  2020-04-10 22:13       ` Elijah Newren
@ 2020-04-10 22:30         ` Junio C Hamano
  2020-04-11  0:07           ` Elijah Newren
  0 siblings, 1 reply; 27+ messages in thread
From: Junio C Hamano @ 2020-04-10 22:30 UTC (permalink / raw)
  To: Elijah Newren
  Cc: Jonathan Tan, Git Mailing List, Phillip Wood,
	Johannes Schindelin, Bryan Turner, Sami Boukortt,
	Elijah Newren via GitGitGadget

Elijah Newren <newren@gmail.com> writes:

> On Fri, Apr 10, 2020 at 2:29 PM Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>> > So I am OK to rebase the other topic to v2.26.0; would that help?  I
>> > already saw there was some entanglement with the other topic in one
>> > of the patches in this series, so...
> ...
> Thanks, but wouldn't I want to base his patch on top of mine rather
> than vice versa?  (Since mine might be labelled bugfix, and his is a
> new feature?)

If you are willing to rebase the --[no-]keep-empty so that it does
not depend on v2.26, that may give us a better result.  I just got
an impression that you somehow wanted to base your changes on the
newer codebase, but if you want to base both on the older codebase,
that is fine by me, too.



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

* Re: [PATCH v2 0/3] rebase -i: mark commits that begin empty in todo editor
  2020-04-10 22:30         ` Junio C Hamano
@ 2020-04-11  0:07           ` Elijah Newren
  2020-04-11 21:14             ` Junio C Hamano
  0 siblings, 1 reply; 27+ messages in thread
From: Elijah Newren @ 2020-04-11  0:07 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Jonathan Tan, Git Mailing List, Phillip Wood,
	Johannes Schindelin, Bryan Turner, Sami Boukortt,
	Elijah Newren via GitGitGadget

On Fri, Apr 10, 2020 at 3:30 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Elijah Newren <newren@gmail.com> writes:
>
> > On Fri, Apr 10, 2020 at 2:29 PM Junio C Hamano <gitster@pobox.com> wrote:
> >>
> >> Junio C Hamano <gitster@pobox.com> writes:
> >>
> >> > So I am OK to rebase the other topic to v2.26.0; would that help?  I
> >> > already saw there was some entanglement with the other topic in one
> >> > of the patches in this series, so...
> > ...
> > Thanks, but wouldn't I want to base his patch on top of mine rather
> > than vice versa?  (Since mine might be labelled bugfix, and his is a
> > new feature?)
>
> If you are willing to rebase the --[no-]keep-empty so that it does
> not depend on v2.26, that may give us a better result.  I just got
> an impression that you somehow wanted to base your changes on the
> newer codebase, but if you want to base both on the older codebase,
> that is fine by me, too.

Rebasing my stuff on the older codebase wouldn't make sense; the
relevant code would be riddled with conflicts.  Rather, I was thinking
of rebasing Jonathan's changes and building his series on top of mine
(and then touching up the docs so the flags reference each other).

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

* [PATCH v3 0/4] rebase -i: mark commits that begin empty in todo editor
  2020-04-10 17:51 ` [PATCH v2 0/3] " Elijah Newren via GitGitGadget
                     ` (3 preceding siblings ...)
  2020-04-10 20:42   ` [PATCH v2 0/3] rebase -i: mark commits that begin empty in todo editor Junio C Hamano
@ 2020-04-11  2:44   ` Elijah Newren via GitGitGadget
  2020-04-11  2:44     ` [PATCH v3 1/4] " Elijah Newren via GitGitGadget
                       ` (4 more replies)
  4 siblings, 5 replies; 27+ messages in thread
From: Elijah Newren via GitGitGadget @ 2020-04-11  2:44 UTC (permalink / raw)
  To: git
  Cc: jonathantanmy, phillip.wood123, Johannes.Schindelin, bturner,
	sami, Elijah Newren

Changes since v2:

 * Addressed feedback from Junio in patch 2, including moving some of the
   wording to the new patch (see next point)
 * Edited jt/rebase-allow-duplicate to include the changes Jonathan
   mentioned on the list, then rebased it on top of this series, and made
   the related options mention each other.

Elijah Newren (3):
  rebase -i: mark commits that begin empty in todo editor
  rebase: reinstate --no-keep-empty
  rebase: fix an incompatible-options error message

Jonathan Tan (1):
  rebase --merge: optionally skip upstreamed commits

 Documentation/git-rebase.txt      | 70 +++++++++++++++++++++-------
 builtin/rebase.c                  | 24 +++++++---
 sequencer.c                       | 14 +++++-
 sequencer.h                       |  4 +-
 t/t3402-rebase-merge.sh           | 77 +++++++++++++++++++++++++++++++
 t/t3421-rebase-topology-linear.sh | 10 ++--
 t/t3424-rebase-empty.sh           | 36 +++++++++++++++
 7 files changed, 202 insertions(+), 33 deletions(-)


base-commit: 274b9cc25322d9ee79aa8e6d4e86f0ffe5ced925
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-757%2Fnewren%2Frebase-mark-empty-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-757/newren/rebase-mark-empty-v3
Pull-Request: https://github.com/git/git/pull/757

Range-diff vs v2:

 1:  0d94eea376a = 1:  0d94eea376a rebase -i: mark commits that begin empty in todo editor
 2:  e15c599c874 ! 2:  3a5bedc68d6 rebase: reinstate --no-keep-empty
     @@ Commit message
      
          Commit d48e5e21da ("rebase (interactive-backend): make --keep-empty the
          default", 2020-02-15) turned --keep-empty (for keeping commits which
     -    start empty) into the default.  That commit viewed this, though as
     -    turning that flag into a no-op.  As such, --no-keep-empty was translated
     -    into undoing a no-op, i.e. also a no-op.  The logic underpinning that
     -    commit was:
     +    start empty) into the default.  The logic underpinning that commit was:
      
            1) 'git commit' errors out on the creation of empty commits without an
               override flag
     @@ Commit message
          look really weird despite making perfect sense (the first will drop
          commits which become empty, but keep commits that started empty; the
          second will keep commits which become empty, but drop commits which
     -    started empty).
     +    started empty).  However, --no-keep-empty was named years ago and we are
     +    predominantly keeping it for backward compatibility; also we suspect it
     +    will only be used rarely since folks already have a simple way to drop
     +    commits they don't want with an interactive rebase.
      
          Reported-by: Bryan Turner <bturner@atlassian.com>
          Reported-by: Sami Boukortt <sami@boukortt.com>
     @@ Documentation/git-rebase.txt: See also INCOMPATIBLE OPTIONS below.
      -always dropped.
      +Note that commits which start empty are kept (unless --no-keep-empty
      +is specified), and commits which are clean cherry-picks (as determined
     -+by `git log --cherry-mark ...`) are detected and dropped as a preliminary
     -+step (unless --keep-cherry-picks is passed).
     ++by `git log --cherry-mark ...`) are always dropped.
       +
       See also INCOMPATIBLE OPTIONS below.
       
     @@ Documentation/git-rebase.txt: See also INCOMPATIBLE OPTIONS below.
      -	to halt.
      +	Do not keep commits that start empty before the rebase
      +	(i.e. that do not change anything from its parent) in the
     -+	result.  For commits which become empty after rebasing, see
     -+	the --empty and --keep-cherry-pick flags.
     ++	result.  The default is to keep commits which start empty,
     ++	since creating such commits requires passing the --allow-empty
     ++	override flag to `git commit`, signifying that a user is very
     ++	intentionally creating such a commit and thus wants to keep
     ++	it.
       +
      -See also BEHAVIORAL DIFFERENCES and INCOMPATIBLE OPTIONS below.
     ++Usage of this flag will probably be rare, since you can get rid of
     ++commits that start empty by just firing up an interactive rebase and
     ++removing the lines corresponding to the commits you don't want.  This
     ++flag exists as a convenient shortcut, such as for cases where external
     ++tools generate many empty commits and you want them all removed.
     +++
     ++For commits which do not start empty but become empty after rebasing,
     ++see the --empty flag.
     +++
      +See also INCOMPATIBLE OPTIONS below.
       
       --allow-empty-message::
 3:  ee5e42361fc = 3:  5c8863b9d34 rebase: fix an incompatible-options error message
 -:  ----------- > 4:  20d3a50f5a4 rebase --merge: optionally skip upstreamed commits

-- 
gitgitgadget

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

* [PATCH v3 1/4] rebase -i: mark commits that begin empty in todo editor
  2020-04-11  2:44   ` [PATCH v3 0/4] " Elijah Newren via GitGitGadget
@ 2020-04-11  2:44     ` Elijah Newren via GitGitGadget
  2020-04-15 20:52       ` Junio C Hamano
  2020-04-11  2:44     ` [PATCH v3 2/4] rebase: reinstate --no-keep-empty Elijah Newren via GitGitGadget
                       ` (3 subsequent siblings)
  4 siblings, 1 reply; 27+ messages in thread
From: Elijah Newren via GitGitGadget @ 2020-04-11  2:44 UTC (permalink / raw)
  To: git
  Cc: jonathantanmy, phillip.wood123, Johannes.Schindelin, bturner,
	sami, Elijah Newren, Elijah Newren

From: Elijah Newren <newren@gmail.com>

While many users who intentionally create empty commits do not want them
thrown away by a rebase, there are third-party tools that generate empty
commits that a user might not want.  In the past, users have used rebase
to get rid of such commits (a side-effect of the fact that the --apply
backend is not currently capable of keeping them).  While such users
could fire up an interactive rebase and just remove the lines
corresponding to empty commits, that might be difficult if the
third-party tool generates many of them.  Simplify this task for users
by marking such lines with a suffix of " # empty" in the todo list.

Suggested-by: Sami Boukortt <sami@boukortt.com>
Signed-off-by: Elijah Newren <newren@gmail.com>
---
 Documentation/git-rebase.txt | 3 ++-
 sequencer.c                  | 5 +++++
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index f7a6033607f..8ab0558aca2 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -620,7 +620,8 @@ commits that started empty, though these are rare in practice.  It
 also drops commits that become empty and has no option for controlling
 this behavior.
 
-The merge backend keeps intentionally empty commits.  Similar to the
+The merge backend keeps intentionally empty commits (though with -i
+they are marked as empty in the todo list editor).  Similar to the
 apply backend, by default the merge backend drops commits that become
 empty unless -i/--interactive is specified (in which case it stops and
 asks the user what to do).  The merge backend also has an
diff --git a/sequencer.c b/sequencer.c
index e528225e787..ce9fd27a878 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -4656,6 +4656,9 @@ static int make_script_with_merges(struct pretty_print_context *pp,
 			strbuf_addf(&buf, "%s %s %s", cmd_pick,
 				    oid_to_hex(&commit->object.oid),
 				    oneline.buf);
+			if (is_empty)
+				strbuf_addf(&buf, " %c empty",
+					    comment_line_char);
 
 			FLEX_ALLOC_STR(entry, string, buf.buf);
 			oidcpy(&entry->entry.oid, &commit->object.oid);
@@ -4861,6 +4864,8 @@ int sequencer_make_script(struct repository *r, struct strbuf *out, int argc,
 		strbuf_addf(out, "%s %s ", insn,
 			    oid_to_hex(&commit->object.oid));
 		pretty_print_commit(&pp, commit, out);
+		if (is_empty)
+			strbuf_addf(out, " %c empty", comment_line_char);
 		strbuf_addch(out, '\n');
 	}
 	return 0;
-- 
gitgitgadget


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

* [PATCH v3 2/4] rebase: reinstate --no-keep-empty
  2020-04-11  2:44   ` [PATCH v3 0/4] " Elijah Newren via GitGitGadget
  2020-04-11  2:44     ` [PATCH v3 1/4] " Elijah Newren via GitGitGadget
@ 2020-04-11  2:44     ` Elijah Newren via GitGitGadget
  2020-04-11  2:44     ` [PATCH v3 3/4] rebase: fix an incompatible-options error message Elijah Newren via GitGitGadget
                       ` (2 subsequent siblings)
  4 siblings, 0 replies; 27+ messages in thread
From: Elijah Newren via GitGitGadget @ 2020-04-11  2:44 UTC (permalink / raw)
  To: git
  Cc: jonathantanmy, phillip.wood123, Johannes.Schindelin, bturner,
	sami, Elijah Newren, Elijah Newren

From: Elijah Newren <newren@gmail.com>

Commit d48e5e21da ("rebase (interactive-backend): make --keep-empty the
default", 2020-02-15) turned --keep-empty (for keeping commits which
start empty) into the default.  The logic underpinning that commit was:

  1) 'git commit' errors out on the creation of empty commits without an
     override flag
  2) Once someone determines that the override is worthwhile, it's
     annoying and/or harmful to required them to take extra steps in
     order to keep such commits around (and to repeat such steps with
     every rebase).

While the logic on which the decision was made is sound, the result was
a bit of an overcorrection.  Instead of jumping to having --keep-empty
being the default, it jumped to making --keep-empty the only available
behavior.  There was a simple workaround, though, which was thought to
be good enough at the time.  People could still drop commits which
started empty the same way the could drop any commits: by firing up an
interactive rebase and picking out the commits they didn't want from the
list.  However, there are cases where external tools might create enough
empty commits that picking all of them out is painful.  As such, having
a flag to automatically remove start-empty commits may be beneficial.

Provide users a way to drop commits which start empty using a flag that
existed for years: --no-keep-empty.  Interpret --keep-empty as
countermanding any previous --no-keep-empty, but otherwise leaving
--keep-empty as the default.

This might lead to some slight weirdness since commands like
  git rebase --empty=drop --keep-empty
  git rebase --empty=keep --no-keep-empty
look really weird despite making perfect sense (the first will drop
commits which become empty, but keep commits that started empty; the
second will keep commits which become empty, but drop commits which
started empty).  However, --no-keep-empty was named years ago and we are
predominantly keeping it for backward compatibility; also we suspect it
will only be used rarely since folks already have a simple way to drop
commits they don't want with an interactive rebase.

Reported-by: Bryan Turner <bturner@atlassian.com>
Reported-by: Sami Boukortt <sami@boukortt.com>
Signed-off-by: Elijah Newren <newren@gmail.com>
---
 Documentation/git-rebase.txt      | 48 ++++++++++++++++++++-----------
 builtin/rebase.c                  | 15 ++++++----
 sequencer.c                       |  6 ++++
 sequencer.h                       |  2 +-
 t/t3421-rebase-topology-linear.sh | 10 +++----
 t/t3424-rebase-empty.sh           | 36 +++++++++++++++++++++++
 6 files changed, 87 insertions(+), 30 deletions(-)

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 8ab0558aca2..18d718ac61d 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -277,20 +277,32 @@ See also INCOMPATIBLE OPTIONS below.
 	Other options, like --exec, will use the default of drop unless
 	-i/--interactive is explicitly specified.
 +
-Note that commits which start empty are kept, and commits which are
-clean cherry-picks (as determined by `git log --cherry-mark ...`) are
-always dropped.
+Note that commits which start empty are kept (unless --no-keep-empty
+is specified), and commits which are clean cherry-picks (as determined
+by `git log --cherry-mark ...`) are always dropped.
 +
 See also INCOMPATIBLE OPTIONS below.
 
+--no-keep-empty::
 --keep-empty::
-	No-op.  Rebasing commits that started empty (had no change
-	relative to their parent) used to fail and this option would
-	override that behavior, allowing commits with empty changes to
-	be rebased.  Now commits with no changes do not cause rebasing
-	to halt.
+	Do not keep commits that start empty before the rebase
+	(i.e. that do not change anything from its parent) in the
+	result.  The default is to keep commits which start empty,
+	since creating such commits requires passing the --allow-empty
+	override flag to `git commit`, signifying that a user is very
+	intentionally creating such a commit and thus wants to keep
+	it.
 +
-See also BEHAVIORAL DIFFERENCES and INCOMPATIBLE OPTIONS below.
+Usage of this flag will probably be rare, since you can get rid of
+commits that start empty by just firing up an interactive rebase and
+removing the lines corresponding to the commits you don't want.  This
+flag exists as a convenient shortcut, such as for cases where external
+tools generate many empty commits and you want them all removed.
++
+For commits which do not start empty but become empty after rebasing,
+see the --empty flag.
++
+See also INCOMPATIBLE OPTIONS below.
 
 --allow-empty-message::
 	No-op.  Rebasing commits with an empty message used to fail
@@ -587,7 +599,7 @@ are incompatible with the following options:
  * --preserve-merges
  * --interactive
  * --exec
- * --keep-empty
+ * --no-keep-empty
  * --empty=
  * --edit-todo
  * --root when used in combination with --onto
@@ -620,13 +632,15 @@ commits that started empty, though these are rare in practice.  It
 also drops commits that become empty and has no option for controlling
 this behavior.
 
-The merge backend keeps intentionally empty commits (though with -i
-they are marked as empty in the todo list editor).  Similar to the
-apply backend, by default the merge backend drops commits that become
-empty unless -i/--interactive is specified (in which case it stops and
-asks the user what to do).  The merge backend also has an
---empty={drop,keep,ask} option for changing the behavior of handling
-commits that become empty.
+The merge backend keeps intentionally empty commits by default (though
+with -i they are marked as empty in the todo list editor, or they can
+be dropped automatically with --no-keep-empty).
+
+Similar to the apply backend, by default the merge backend drops
+commits that become empty unless -i/--interactive is specified (in
+which case it stops and asks the user what to do).  The merge backend
+also has an --empty={drop,keep,ask} option for changing the behavior
+of handling commits that become empty.
 
 Directory rename detection
 ~~~~~~~~~~~~~~~~~~~~~~~~~~
diff --git a/builtin/rebase.c b/builtin/rebase.c
index bff53d5d167..022aa2589a5 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -85,6 +85,7 @@ struct rebase_options {
 	const char *action;
 	int signoff;
 	int allow_rerere_autoupdate;
+	int keep_empty;
 	int autosquash;
 	char *gpg_sign_opt;
 	int autostash;
@@ -100,6 +101,7 @@ struct rebase_options {
 #define REBASE_OPTIONS_INIT {			  	\
 		.type = REBASE_UNSPECIFIED,	  	\
 		.empty = EMPTY_UNSPECIFIED,	  	\
+		.keep_empty = 1,			\
 		.default_backend = "merge",	  	\
 		.flags = REBASE_NO_QUIET, 		\
 		.git_am_opts = ARGV_ARRAY_INIT,		\
@@ -379,6 +381,7 @@ static int run_sequencer_rebase(struct rebase_options *opts,
 
 	git_config_get_bool("rebase.abbreviatecommands", &abbreviate_commands);
 
+	flags |= opts->keep_empty ? TODO_LIST_KEEP_EMPTY : 0;
 	flags |= abbreviate_commands ? TODO_LIST_ABBREVIATE_CMDS : 0;
 	flags |= opts->rebase_merges ? TODO_LIST_REBASE_MERGES : 0;
 	flags |= opts->rebase_cousins > 0 ? TODO_LIST_REBASE_COUSINS : 0;
@@ -442,6 +445,7 @@ static int run_sequencer_rebase(struct rebase_options *opts,
 	return ret;
 }
 
+static void imply_merge(struct rebase_options *opts, const char *option);
 static int parse_opt_keep_empty(const struct option *opt, const char *arg,
 				int unset)
 {
@@ -449,10 +453,8 @@ static int parse_opt_keep_empty(const struct option *opt, const char *arg,
 
 	BUG_ON_OPT_ARG(arg);
 
-	/*
-	 * If we ever want to remap --keep-empty to --empty=keep, insert:
-	 * 	opts->empty = unset ? EMPTY_UNSPECIFIED : EMPTY_KEEP;
-	 */
+	imply_merge(opts, unset ? "--no-keep-empty" : "--keep-empty");
+	opts->keep_empty = !unset;
 	opts->type = REBASE_MERGE;
 	return 0;
 }
@@ -471,7 +473,7 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix)
 		OPT_NEGBIT(0, "ff", &opts.flags, N_("allow fast-forward"),
 			   REBASE_FORCE),
 		{ OPTION_CALLBACK, 'k', "keep-empty", &options, NULL,
-			N_("(DEPRECATED) keep empty commits"),
+			N_("keep commits which start empty"),
 			PARSE_OPT_NOARG | PARSE_OPT_HIDDEN,
 			parse_opt_keep_empty },
 		OPT_BOOL_F(0, "allow-empty-message", &opts.allow_empty_message,
@@ -1162,6 +1164,7 @@ static int run_specific_rebase(struct rebase_options *opts, enum action action)
 		opts->allow_rerere_autoupdate ?
 			opts->allow_rerere_autoupdate == RERERE_AUTOUPDATE ?
 			"--rerere-autoupdate" : "--no-rerere-autoupdate" : "");
+	add_var(&script_snippet, "keep_empty", opts->keep_empty ? "yes" : "");
 	add_var(&script_snippet, "autosquash", opts->autosquash ? "t" : "");
 	add_var(&script_snippet, "gpg_sign_opt", opts->gpg_sign_opt);
 	add_var(&script_snippet, "cmd", opts->cmd);
@@ -1547,7 +1550,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 			       N_("how to handle commits that become empty"),
 			       PARSE_OPT_NONEG, parse_opt_empty),
 		{ OPTION_CALLBACK, 'k', "keep-empty", &options, NULL,
-			N_("(DEPRECATED) keep empty commits"),
+			N_("keep commits which start empty"),
 			PARSE_OPT_NOARG | PARSE_OPT_HIDDEN,
 			parse_opt_keep_empty },
 		OPT_BOOL(0, "autosquash", &options.autosquash,
diff --git a/sequencer.c b/sequencer.c
index ce9fd27a878..d973e19729e 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -4591,6 +4591,7 @@ static int make_script_with_merges(struct pretty_print_context *pp,
 				   struct rev_info *revs, struct strbuf *out,
 				   unsigned flags)
 {
+	int keep_empty = flags & TODO_LIST_KEEP_EMPTY;
 	int rebase_cousins = flags & TODO_LIST_REBASE_COUSINS;
 	int root_with_onto = flags & TODO_LIST_ROOT_WITH_ONTO;
 	struct strbuf buf = STRBUF_INIT, oneline = STRBUF_INIT;
@@ -4645,6 +4646,8 @@ static int make_script_with_merges(struct pretty_print_context *pp,
 		is_empty = is_original_commit_empty(commit);
 		if (!is_empty && (commit->object.flags & PATCHSAME))
 			continue;
+		if (is_empty && !keep_empty)
+			continue;
 
 		strbuf_reset(&oneline);
 		pretty_print_commit(pp, commit, &oneline);
@@ -4822,6 +4825,7 @@ int sequencer_make_script(struct repository *r, struct strbuf *out, int argc,
 	struct pretty_print_context pp = {0};
 	struct rev_info revs;
 	struct commit *commit;
+	int keep_empty = flags & TODO_LIST_KEEP_EMPTY;
 	const char *insn = flags & TODO_LIST_ABBREVIATE_CMDS ? "p" : "pick";
 	int rebase_merges = flags & TODO_LIST_REBASE_MERGES;
 
@@ -4861,6 +4865,8 @@ int sequencer_make_script(struct repository *r, struct strbuf *out, int argc,
 
 		if (!is_empty && (commit->object.flags & PATCHSAME))
 			continue;
+		if (is_empty && !keep_empty)
+			continue;
 		strbuf_addf(out, "%s %s ", insn,
 			    oid_to_hex(&commit->object.oid));
 		pretty_print_commit(&pp, commit, out);
diff --git a/sequencer.h b/sequencer.h
index 718a07426da..7ebaa237340 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -134,7 +134,7 @@ int sequencer_rollback(struct repository *repo, struct replay_opts *opts);
 int sequencer_skip(struct repository *repo, struct replay_opts *opts);
 int sequencer_remove_state(struct replay_opts *opts);
 
-/* #define TODO_LIST_KEEP_EMPTY (1U << 0) */ /* No longer used */
+#define TODO_LIST_KEEP_EMPTY (1U << 0)
 #define TODO_LIST_SHORTEN_IDS (1U << 1)
 #define TODO_LIST_ABBREVIATE_CMDS (1U << 2)
 #define TODO_LIST_REBASE_MERGES (1U << 3)
diff --git a/t/t3421-rebase-topology-linear.sh b/t/t3421-rebase-topology-linear.sh
index cf8dfd6c203..4a9204b4b64 100755
--- a/t/t3421-rebase-topology-linear.sh
+++ b/t/t3421-rebase-topology-linear.sh
@@ -220,14 +220,13 @@ test_have_prereq !REBASE_P || test_run_rebase failure -p
 test_run_rebase () {
 	result=$1
 	shift
-	test_expect_$result "rebase $* --keep-empty" "
+	test_expect_$result "rebase $* --no-keep-empty drops begin-empty commits" "
 		reset_rebase &&
-		git rebase $* --keep-empty c l &&
-		test_cmp_rev c HEAD~3 &&
-		test_linear_range 'd k l' c..
+		git rebase $* --no-keep-empty c l &&
+		test_cmp_rev c HEAD~2 &&
+		test_linear_range 'd l' c..
 	"
 }
-test_run_rebase success --apply
 test_run_rebase success -m
 test_run_rebase success -i
 test_have_prereq !REBASE_P || test_run_rebase success -p
@@ -242,7 +241,6 @@ test_run_rebase () {
 		test_linear_range 'd k l' j..
 	"
 }
-test_run_rebase success --apply
 test_run_rebase success -m
 test_run_rebase success -i
 test_have_prereq !REBASE_P || test_run_rebase success -p
diff --git a/t/t3424-rebase-empty.sh b/t/t3424-rebase-empty.sh
index e1e30517ea6..5e1045a0afc 100755
--- a/t/t3424-rebase-empty.sh
+++ b/t/t3424-rebase-empty.sh
@@ -123,6 +123,42 @@ test_expect_success 'rebase --interactive uses default of --empty=ask' '
 	test_cmp expect actual
 '
 
+test_expect_success 'rebase --merge --empty=drop --keep-empty' '
+	git checkout -B testing localmods &&
+	git rebase --merge --empty=drop --keep-empty upstream &&
+
+	test_write_lines D C B A >expect &&
+	git log --format=%s >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'rebase --merge --empty=drop --no-keep-empty' '
+	git checkout -B testing localmods &&
+	git rebase --merge --empty=drop --no-keep-empty upstream &&
+
+	test_write_lines C B A >expect &&
+	git log --format=%s >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'rebase --merge --empty=keep --keep-empty' '
+	git checkout -B testing localmods &&
+	git rebase --merge --empty=keep --keep-empty upstream &&
+
+	test_write_lines D C2 C B A >expect &&
+	git log --format=%s >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'rebase --merge --empty=keep --no-keep-empty' '
+	git checkout -B testing localmods &&
+	git rebase --merge --empty=keep --no-keep-empty upstream &&
+
+	test_write_lines C2 C B A >expect &&
+	git log --format=%s >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'rebase --merge does not leave state laying around' '
 	git checkout -B testing localmods~2 &&
 	git rebase --merge upstream &&
-- 
gitgitgadget


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

* [PATCH v3 3/4] rebase: fix an incompatible-options error message
  2020-04-11  2:44   ` [PATCH v3 0/4] " Elijah Newren via GitGitGadget
  2020-04-11  2:44     ` [PATCH v3 1/4] " Elijah Newren via GitGitGadget
  2020-04-11  2:44     ` [PATCH v3 2/4] rebase: reinstate --no-keep-empty Elijah Newren via GitGitGadget
@ 2020-04-11  2:44     ` Elijah Newren via GitGitGadget
  2020-04-11  2:44     ` [PATCH v3 4/4] rebase --merge: optionally skip upstreamed commits Jonathan Tan via GitGitGadget
  2020-04-14  9:11     ` [PATCH v3 0/4] rebase -i: mark commits that begin empty in todo editor Phillip Wood
  4 siblings, 0 replies; 27+ messages in thread
From: Elijah Newren via GitGitGadget @ 2020-04-11  2:44 UTC (permalink / raw)
  To: git
  Cc: jonathantanmy, phillip.wood123, Johannes.Schindelin, bturner,
	sami, Elijah Newren, Elijah Newren

From: Elijah Newren <newren@gmail.com>

When the user specifies the apply backend with options that only work
with the merge backend, such as

    git rebase --apply --exec /bin/true HEAD~3

the error message has always been

    fatal: --exec requires an interactive rebase

This error message is misleading and was one of the reasons we renamed
the interactive backend to the merge backend.  Update the error message
to state that these options merely require use of the merge backend.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 builtin/rebase.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 022aa2589a5..0e223a96d46 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -561,7 +561,7 @@ static void imply_merge(struct rebase_options *opts, const char *option)
 {
 	switch (opts->type) {
 	case REBASE_APPLY:
-		die(_("%s requires an interactive rebase"), option);
+		die(_("%s requires the merge backend"), option);
 		break;
 	case REBASE_MERGE:
 	case REBASE_PRESERVE_MERGES:
-- 
gitgitgadget


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

* [PATCH v3 4/4] rebase --merge: optionally skip upstreamed commits
  2020-04-11  2:44   ` [PATCH v3 0/4] " Elijah Newren via GitGitGadget
                       ` (2 preceding siblings ...)
  2020-04-11  2:44     ` [PATCH v3 3/4] rebase: fix an incompatible-options error message Elijah Newren via GitGitGadget
@ 2020-04-11  2:44     ` Jonathan Tan via GitGitGadget
  2020-04-11 18:12       ` Jonathan Tan
  2020-04-14  9:11     ` [PATCH v3 0/4] rebase -i: mark commits that begin empty in todo editor Phillip Wood
  4 siblings, 1 reply; 27+ messages in thread
From: Jonathan Tan via GitGitGadget @ 2020-04-11  2:44 UTC (permalink / raw)
  To: git
  Cc: jonathantanmy, phillip.wood123, Johannes.Schindelin, bturner,
	sami, Elijah Newren, Jonathan Tan

From: Jonathan Tan <jonathantanmy@google.com>

When rebasing against an upstream that has had many commits since the
original branch was created:

 O -- O -- ... -- O -- O (upstream)
  \
   -- O (my-dev-branch)

it must read the contents of every novel upstream commit, in addition to
the tip of the upstream and the merge base, because "git rebase"
attempts to exclude commits that are duplicates of upstream ones. This
can be a significant performance hit, especially in a partial clone,
wherein a read of an object may end up being a fetch.

Add a flag to "git rebase" to allow suppression of this feature. This
flag only works when using the "merge" backend.

This flag changes the behavior of sequencer_make_script(), called from
do_interactive_rebase() <- run_rebase_interactive() <-
run_specific_rebase() <- cmd_rebase(). With this flag, limit_list()
(indirectly called from sequencer_make_script() through
prepare_revision_walk()) will no longer call cherry_pick_list(), and
thus PATCHSAME is no longer set. Refraining from setting PATCHSAME both
means that the intermediate commits in upstream are no longer read (as
shown by the test) and means that no PATCHSAME-caused skipping of
commits is done by sequencer_make_script(), either directly or through
make_script_with_merges().

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
Signed-off-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/git-rebase.txt | 25 +++++++++++-
 builtin/rebase.c             |  7 ++++
 sequencer.c                  |  3 +-
 sequencer.h                  |  2 +-
 t/t3402-rebase-merge.sh      | 77 ++++++++++++++++++++++++++++++++++++
 5 files changed, 110 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 18d718ac61d..cccf76dfa4b 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -279,7 +279,8 @@ See also INCOMPATIBLE OPTIONS below.
 +
 Note that commits which start empty are kept (unless --no-keep-empty
 is specified), and commits which are clean cherry-picks (as determined
-by `git log --cherry-mark ...`) are always dropped.
+by `git log --cherry-mark ...`) are detected and dropped as a
+preliminary step (unless --reapply-cherry-picks is passed).
 +
 See also INCOMPATIBLE OPTIONS below.
 
@@ -304,6 +305,24 @@ see the --empty flag.
 +
 See also INCOMPATIBLE OPTIONS below.
 
+--reapply-cherry-picks::
+--no-reapply-cherry-picks::
+	Reapply all clean cherry-picks of any upstream commit instead
+	of preemptively dropping them. (If these commits then become
+	empty after rebasing, because they contain a subset of already
+	upstream changes, the behavior towards them is controlled by
+	the `--empty` flag.)
++
+By default (or if `--no-reapply-cherry-picks` is given), these commits
+will be automatically dropped.  Because this necessitates reading all
+upstream commits, this can be expensive in repos with a large number
+of upstream commits that need to be read.
++
+`--reapply-cherry-picks` allows rebase to forgo reading all upstream
+commits, potentially improving performance.
++
+See also INCOMPATIBLE OPTIONS below.
+
 --allow-empty-message::
 	No-op.  Rebasing commits with an empty message used to fail
 	and this option would override that behavior, allowing commits
@@ -601,6 +620,7 @@ are incompatible with the following options:
  * --exec
  * --no-keep-empty
  * --empty=
+ * --reapply-cherry-picks
  * --edit-todo
  * --root when used in combination with --onto
 
@@ -1017,7 +1037,8 @@ Only works if the changes (patch IDs based on the diff contents) on
 'subsystem' did.
 
 In that case, the fix is easy because 'git rebase' knows to skip
-changes that are already present in the new upstream.  So if you say
+changes that are already present in the new upstream (unless
+`--reapply-cherry-picks` is given). So if you say
 (assuming you're on 'topic')
 ------------
     $ git rebase subsystem
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 0e223a96d46..75c2ebd4c35 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -96,6 +96,7 @@ struct rebase_options {
 	struct strbuf git_format_patch_opt;
 	int reschedule_failed_exec;
 	int use_legacy_rebase;
+	int reapply_cherry_picks;
 };
 
 #define REBASE_OPTIONS_INIT {			  	\
@@ -387,6 +388,7 @@ static int run_sequencer_rebase(struct rebase_options *opts,
 	flags |= opts->rebase_cousins > 0 ? TODO_LIST_REBASE_COUSINS : 0;
 	flags |= opts->root_with_onto ? TODO_LIST_ROOT_WITH_ONTO : 0;
 	flags |= command == ACTION_SHORTEN_OIDS ? TODO_LIST_SHORTEN_IDS : 0;
+	flags |= opts->reapply_cherry_picks ? TODO_LIST_REAPPLY_CHERRY_PICKS : 0;
 
 	switch (command) {
 	case ACTION_NONE: {
@@ -1585,6 +1587,8 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		OPT_BOOL(0, "reschedule-failed-exec",
 			 &reschedule_failed_exec,
 			 N_("automatically re-schedule any `exec` that fails")),
+		OPT_BOOL(0, "reapply-cherry-picks", &options.reapply_cherry_picks,
+			 N_("apply all changes, even those already present upstream")),
 		OPT_END(),
 	};
 	int i;
@@ -1825,6 +1829,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	if (options.empty != EMPTY_UNSPECIFIED)
 		imply_merge(&options, "--empty");
 
+	if (options.reapply_cherry_picks)
+		imply_merge(&options, "--reapply-cherry-picks");
+
 	if (gpg_sign) {
 		free(options.gpg_sign_opt);
 		options.gpg_sign_opt = xstrfmt("-S%s", gpg_sign);
diff --git a/sequencer.c b/sequencer.c
index d973e19729e..667648d6f9d 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -4828,12 +4828,13 @@ int sequencer_make_script(struct repository *r, struct strbuf *out, int argc,
 	int keep_empty = flags & TODO_LIST_KEEP_EMPTY;
 	const char *insn = flags & TODO_LIST_ABBREVIATE_CMDS ? "p" : "pick";
 	int rebase_merges = flags & TODO_LIST_REBASE_MERGES;
+	int reapply_cherry_picks = flags & TODO_LIST_REAPPLY_CHERRY_PICKS;
 
 	repo_init_revisions(r, &revs, NULL);
 	revs.verbose_header = 1;
 	if (!rebase_merges)
 		revs.max_parents = 1;
-	revs.cherry_mark = 1;
+	revs.cherry_mark = !reapply_cherry_picks;
 	revs.limited = 1;
 	revs.reverse = 1;
 	revs.right_only = 1;
diff --git a/sequencer.h b/sequencer.h
index 7ebaa237340..25b396b9171 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -150,7 +150,7 @@ int sequencer_remove_state(struct replay_opts *opts);
  * `--onto`, we do not want to re-generate the root commits.
  */
 #define TODO_LIST_ROOT_WITH_ONTO (1U << 6)
-
+#define TODO_LIST_REAPPLY_CHERRY_PICKS (1U << 7)
 
 int sequencer_make_script(struct repository *r, struct strbuf *out, int argc,
 			  const char **argv, unsigned flags);
diff --git a/t/t3402-rebase-merge.sh b/t/t3402-rebase-merge.sh
index a1ec501a872..6e032716a68 100755
--- a/t/t3402-rebase-merge.sh
+++ b/t/t3402-rebase-merge.sh
@@ -162,4 +162,81 @@ test_expect_success 'rebase --skip works with two conflicts in a row' '
 	git rebase --skip
 '
 
+test_expect_success '--reapply-cherry-picks' '
+	git init repo &&
+
+	# O(1-10) -- O(1-11) -- O(0-10) master
+	#        \
+	#         -- O(1-11) -- O(1-12) otherbranch
+
+	printf "Line %d\n" $(test_seq 1 10) >repo/file.txt &&
+	git -C repo add file.txt &&
+	git -C repo commit -m "base commit" &&
+
+	printf "Line %d\n" $(test_seq 1 11) >repo/file.txt &&
+	git -C repo commit -a -m "add 11" &&
+
+	printf "Line %d\n" $(test_seq 0 10) >repo/file.txt &&
+	git -C repo commit -a -m "add 0 delete 11" &&
+
+	git -C repo checkout -b otherbranch HEAD^^ &&
+	printf "Line %d\n" $(test_seq 1 11) >repo/file.txt &&
+	git -C repo commit -a -m "add 11 in another branch" &&
+
+	printf "Line %d\n" $(test_seq 1 12) >repo/file.txt &&
+	git -C repo commit -a -m "add 12 in another branch" &&
+
+	# Regular rebase fails, because the 1-11 commit is deduplicated
+	test_must_fail git -C repo rebase --merge master 2> err &&
+	test_i18ngrep "error: could not apply.*add 12 in another branch" err &&
+	git -C repo rebase --abort &&
+
+	# With --reapply-cherry-picks, it works
+	git -C repo rebase --merge --reapply-cherry-picks master
+'
+
+test_expect_success '--reapply-cherry-picks refrains from reading unneeded blobs' '
+	git init server &&
+
+	# O(1-10) -- O(1-11) -- O(1-12) master
+	#        \
+	#         -- O(0-10) otherbranch
+
+	printf "Line %d\n" $(test_seq 1 10) >server/file.txt &&
+	git -C server add file.txt &&
+	git -C server commit -m "merge base" &&
+
+	printf "Line %d\n" $(test_seq 1 11) >server/file.txt &&
+	git -C server commit -a -m "add 11" &&
+
+	printf "Line %d\n" $(test_seq 1 12) >server/file.txt &&
+	git -C server commit -a -m "add 12" &&
+
+	git -C server checkout -b otherbranch HEAD^^ &&
+	printf "Line %d\n" $(test_seq 0 10) >server/file.txt &&
+	git -C server commit -a -m "add 0" &&
+
+	test_config -C server uploadpack.allowfilter 1 &&
+	test_config -C server uploadpack.allowanysha1inwant 1 &&
+
+	git clone --filter=blob:none "file://$(pwd)/server" client &&
+	git -C client checkout origin/master &&
+	git -C client checkout origin/otherbranch &&
+
+	# Sanity check to ensure that the blobs from the merge base and "add
+	# 11" are missing
+	git -C client rev-list --objects --all --missing=print >missing_list &&
+	MERGE_BASE_BLOB=$(git -C server rev-parse master^^:file.txt) &&
+	ADD_11_BLOB=$(git -C server rev-parse master^:file.txt) &&
+	grep "[?]$MERGE_BASE_BLOB" missing_list &&
+	grep "[?]$ADD_11_BLOB" missing_list &&
+
+	git -C client rebase --merge --reapply-cherry-picks origin/master &&
+
+	# The blob from the merge base had to be fetched, but not "add 11"
+	git -C client rev-list --objects --all --missing=print >missing_list &&
+	! grep "[?]$MERGE_BASE_BLOB" missing_list &&
+	grep "[?]$ADD_11_BLOB" missing_list
+'
+
 test_done
-- 
gitgitgadget

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

* Re: [PATCH v3 4/4] rebase --merge: optionally skip upstreamed commits
  2020-04-11  2:44     ` [PATCH v3 4/4] rebase --merge: optionally skip upstreamed commits Jonathan Tan via GitGitGadget
@ 2020-04-11 18:12       ` Jonathan Tan
  0 siblings, 0 replies; 27+ messages in thread
From: Jonathan Tan @ 2020-04-11 18:12 UTC (permalink / raw)
  To: gitgitgadget
  Cc: git, jonathantanmy, phillip.wood123, Johannes.Schindelin,
	bturner, sami, newren

> @@ -601,6 +620,7 @@ are incompatible with the following options:
>   * --exec
>   * --no-keep-empty
>   * --empty=
> + * --reapply-cherry-picks
>   * --edit-todo
>   * --root when used in combination with --onto

[snip]

> @@ -1825,6 +1829,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>  	if (options.empty != EMPTY_UNSPECIFIED)
>  		imply_merge(&options, "--empty");
>  
> +	if (options.reapply_cherry_picks)
> +		imply_merge(&options, "--reapply-cherry-picks");
> +
>  	if (gpg_sign) {
>  		free(options.gpg_sign_opt);
>  		options.gpg_sign_opt = xstrfmt("-S%s", gpg_sign);

Thanks - these definitely look cleaner than what I had, and the rest of
the patch looks good too.

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

* Re: [PATCH v2 0/3] rebase -i: mark commits that begin empty in todo editor
  2020-04-11  0:07           ` Elijah Newren
@ 2020-04-11 21:14             ` Junio C Hamano
  0 siblings, 0 replies; 27+ messages in thread
From: Junio C Hamano @ 2020-04-11 21:14 UTC (permalink / raw)
  To: Elijah Newren
  Cc: Jonathan Tan, Git Mailing List, Phillip Wood,
	Johannes Schindelin, Bryan Turner, Sami Boukortt,
	Elijah Newren via GitGitGadget

Elijah Newren <newren@gmail.com> writes:

> On Fri, Apr 10, 2020 at 3:30 PM Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Elijah Newren <newren@gmail.com> writes:
>>
>> > On Fri, Apr 10, 2020 at 2:29 PM Junio C Hamano <gitster@pobox.com> wrote:
>> >>
>> >> Junio C Hamano <gitster@pobox.com> writes:
>> >>
>> >> > So I am OK to rebase the other topic to v2.26.0; would that help?  I
>> >> > already saw there was some entanglement with the other topic in one
>> >> > of the patches in this series, so...
>> > ...
>> > Thanks, but wouldn't I want to base his patch on top of mine rather
>> > than vice versa?  (Since mine might be labelled bugfix, and his is a
>> > new feature?)
>>
>> If you are willing to rebase the --[no-]keep-empty so that it does
>> not depend on v2.26, that may give us a better result.  I just got
>> an impression that you somehow wanted to base your changes on the
>> newer codebase, but if you want to base both on the older codebase,
>> that is fine by me, too.
>
> Rebasing my stuff on the older codebase wouldn't make sense; the
> relevant code would be riddled with conflicts.  Rather, I was thinking
> of rebasing Jonathan's changes and building his series on top of mine
> (and then touching up the docs so the flags reference each other).

That's OK as well.  I didn't know if you wanted the "fix" to be only
for 2.26.x and forward, or wanted to be applicable to 2.25.x series
(which the other topic that is a new feature happened to also work
with).

Thanks.

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

* Re: [PATCH v3 0/4] rebase -i: mark commits that begin empty in todo editor
  2020-04-11  2:44   ` [PATCH v3 0/4] " Elijah Newren via GitGitGadget
                       ` (3 preceding siblings ...)
  2020-04-11  2:44     ` [PATCH v3 4/4] rebase --merge: optionally skip upstreamed commits Jonathan Tan via GitGitGadget
@ 2020-04-14  9:11     ` Phillip Wood
  4 siblings, 0 replies; 27+ messages in thread
From: Phillip Wood @ 2020-04-14  9:11 UTC (permalink / raw)
  To: Elijah Newren via GitGitGadget, git
  Cc: jonathantanmy, Johannes.Schindelin, bturner, sami, Elijah Newren

Hi Elijah

On 11/04/2020 03:44, Elijah Newren via GitGitGadget wrote:
> Changes since v2:
> 
>   * Addressed feedback from Junio in patch 2, including moving some of the
>     wording to the new patch (see next point)
>   * Edited jt/rebase-allow-duplicate to include the changes Jonathan
>     mentioned on the list, then rebased it on top of this series, and made
>     the related options mention each other.
> 
> Elijah Newren (3):
>    rebase -i: mark commits that begin empty in todo editor

I think this is a useful addition

>    rebase: reinstate --no-keep-empty

This looks like a good compromise - we're still changing the default but 
keeping the ability to drop commits that start empty. One thought on 
this patch - previously we would comment out the empty commits in the 
todo list so the user could decide what to do. Now we just don't add 
them to the list so the user has no way to selectively add them back. I 
guess this doesn't matter too much as they can use the new '# empty' 
label and comment out the commits they want to drop rather than 
uncommenting the ones they want to keep

>    rebase: fix an incompatible-options error message

good catch!

Best Wishes

Phillip

> 
> Jonathan Tan (1):
>    rebase --merge: optionally skip upstreamed commits
> 
>   Documentation/git-rebase.txt      | 70 +++++++++++++++++++++-------
>   builtin/rebase.c                  | 24 +++++++---
>   sequencer.c                       | 14 +++++-
>   sequencer.h                       |  4 +-
>   t/t3402-rebase-merge.sh           | 77 +++++++++++++++++++++++++++++++
>   t/t3421-rebase-topology-linear.sh | 10 ++--
>   t/t3424-rebase-empty.sh           | 36 +++++++++++++++
>   7 files changed, 202 insertions(+), 33 deletions(-)
> 
> 
> base-commit: 274b9cc25322d9ee79aa8e6d4e86f0ffe5ced925
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-757%2Fnewren%2Frebase-mark-empty-v3
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-757/newren/rebase-mark-empty-v3
> Pull-Request: https://github.com/git/git/pull/757
> 
> Range-diff vs v2:
> 
>   1:  0d94eea376a = 1:  0d94eea376a rebase -i: mark commits that begin empty in todo editor
>   2:  e15c599c874 ! 2:  3a5bedc68d6 rebase: reinstate --no-keep-empty
>       @@ Commit message
>        
>            Commit d48e5e21da ("rebase (interactive-backend): make --keep-empty the
>            default", 2020-02-15) turned --keep-empty (for keeping commits which
>       -    start empty) into the default.  That commit viewed this, though as
>       -    turning that flag into a no-op.  As such, --no-keep-empty was translated
>       -    into undoing a no-op, i.e. also a no-op.  The logic underpinning that
>       -    commit was:
>       +    start empty) into the default.  The logic underpinning that commit was:
>        
>              1) 'git commit' errors out on the creation of empty commits without an
>                 override flag
>       @@ Commit message
>            look really weird despite making perfect sense (the first will drop
>            commits which become empty, but keep commits that started empty; the
>            second will keep commits which become empty, but drop commits which
>       -    started empty).
>       +    started empty).  However, --no-keep-empty was named years ago and we are
>       +    predominantly keeping it for backward compatibility; also we suspect it
>       +    will only be used rarely since folks already have a simple way to drop
>       +    commits they don't want with an interactive rebase.
>        
>            Reported-by: Bryan Turner <bturner@atlassian.com>
>            Reported-by: Sami Boukortt <sami@boukortt.com>
>       @@ Documentation/git-rebase.txt: See also INCOMPATIBLE OPTIONS below.
>        -always dropped.
>        +Note that commits which start empty are kept (unless --no-keep-empty
>        +is specified), and commits which are clean cherry-picks (as determined
>       -+by `git log --cherry-mark ...`) are detected and dropped as a preliminary
>       -+step (unless --keep-cherry-picks is passed).
>       ++by `git log --cherry-mark ...`) are always dropped.
>         +
>         See also INCOMPATIBLE OPTIONS below.
>         
>       @@ Documentation/git-rebase.txt: See also INCOMPATIBLE OPTIONS below.
>        -	to halt.
>        +	Do not keep commits that start empty before the rebase
>        +	(i.e. that do not change anything from its parent) in the
>       -+	result.  For commits which become empty after rebasing, see
>       -+	the --empty and --keep-cherry-pick flags.
>       ++	result.  The default is to keep commits which start empty,
>       ++	since creating such commits requires passing the --allow-empty
>       ++	override flag to `git commit`, signifying that a user is very
>       ++	intentionally creating such a commit and thus wants to keep
>       ++	it.
>         +
>        -See also BEHAVIORAL DIFFERENCES and INCOMPATIBLE OPTIONS below.
>       ++Usage of this flag will probably be rare, since you can get rid of
>       ++commits that start empty by just firing up an interactive rebase and
>       ++removing the lines corresponding to the commits you don't want.  This
>       ++flag exists as a convenient shortcut, such as for cases where external
>       ++tools generate many empty commits and you want them all removed.
>       +++
>       ++For commits which do not start empty but become empty after rebasing,
>       ++see the --empty flag.
>       +++
>        +See also INCOMPATIBLE OPTIONS below.
>         
>         --allow-empty-message::
>   3:  ee5e42361fc = 3:  5c8863b9d34 rebase: fix an incompatible-options error message
>   -:  ----------- > 4:  20d3a50f5a4 rebase --merge: optionally skip upstreamed commits
> 

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

* Re: [PATCH v3 1/4] rebase -i: mark commits that begin empty in todo editor
  2020-04-11  2:44     ` [PATCH v3 1/4] " Elijah Newren via GitGitGadget
@ 2020-04-15 20:52       ` Junio C Hamano
  0 siblings, 0 replies; 27+ messages in thread
From: Junio C Hamano @ 2020-04-15 20:52 UTC (permalink / raw)
  To: Elijah Newren via GitGitGadget
  Cc: git, jonathantanmy, phillip.wood123, Johannes.Schindelin,
	bturner, sami, Elijah Newren

"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:

> diff --git a/sequencer.c b/sequencer.c
> index e528225e787..ce9fd27a878 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -4656,6 +4656,9 @@ static int make_script_with_merges(struct pretty_print_context *pp,
>  			strbuf_addf(&buf, "%s %s %s", cmd_pick,
>  				    oid_to_hex(&commit->object.oid),
>  				    oneline.buf);
> +			if (is_empty)
> +				strbuf_addf(&buf, " %c empty",
> +					    comment_line_char);
> ...
> @@ -4861,6 +4864,8 @@ int sequencer_make_script(struct repository *r, struct strbuf *out,>  		pretty_print_commit(&pp, commit, out);
> +		if (is_empty)
> +			strbuf_addf(out, " %c empty", comment_line_char);
>  		strbuf_addch(out, '\n');

While I was re-reviewing the topics in flight, it occurred to me
that this change may hurt automation if people feed the insn file to
their own machinery to process, as the title in "git log --oneline"
and in the file no longer matches.  I am not sure how big a deal is
(even when I am scripting, I only use the commit object name and
ignore the title string, but there are people with diferent taste in
this world, so...).  It might be safer to add this as a comment on
its own line, next to the usual "pick <commit> <title>" line, but I
dunno.

This is more about raising awareness of possible regression than a
suggestion to change anything.

Thanks.

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

end of thread, other threads:[~2020-04-15 20:53 UTC | newest]

Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-09 23:26 [PATCH] rebase -i: mark commits that begin empty in todo editor Elijah Newren via GitGitGadget
2020-04-10  0:50 ` Junio C Hamano
2020-04-10  2:06   ` Bryan Turner
2020-04-10  4:57     ` Junio C Hamano
2020-04-10 17:51 ` [PATCH v2 0/3] " Elijah Newren via GitGitGadget
2020-04-10 17:51   ` [PATCH v2 1/3] " Elijah Newren via GitGitGadget
2020-04-10 17:51   ` [PATCH v2 2/3] rebase: reinstate --no-keep-empty Elijah Newren via GitGitGadget
2020-04-10 20:37     ` Junio C Hamano
2020-04-10 21:41       ` Elijah Newren
2020-04-10 17:51   ` [PATCH v2 3/3] rebase: fix an incompatible-options error message Elijah Newren via GitGitGadget
2020-04-10 20:42   ` [PATCH v2 0/3] rebase -i: mark commits that begin empty in todo editor Junio C Hamano
2020-04-10 21:04     ` Junio C Hamano
2020-04-10 21:14       ` Junio C Hamano
2020-04-10 22:11         ` Elijah Newren
2020-04-10 21:29     ` Junio C Hamano
2020-04-10 22:13       ` Elijah Newren
2020-04-10 22:30         ` Junio C Hamano
2020-04-11  0:07           ` Elijah Newren
2020-04-11 21:14             ` Junio C Hamano
2020-04-11  2:44   ` [PATCH v3 0/4] " Elijah Newren via GitGitGadget
2020-04-11  2:44     ` [PATCH v3 1/4] " Elijah Newren via GitGitGadget
2020-04-15 20:52       ` Junio C Hamano
2020-04-11  2:44     ` [PATCH v3 2/4] rebase: reinstate --no-keep-empty Elijah Newren via GitGitGadget
2020-04-11  2:44     ` [PATCH v3 3/4] rebase: fix an incompatible-options error message Elijah Newren via GitGitGadget
2020-04-11  2:44     ` [PATCH v3 4/4] rebase --merge: optionally skip upstreamed commits Jonathan Tan via GitGitGadget
2020-04-11 18:12       ` Jonathan Tan
2020-04-14  9:11     ` [PATCH v3 0/4] rebase -i: mark commits that begin empty in todo editor Phillip Wood

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 NNTP newsgroup(s).