[PATCH 0/2] rebase --recreate-merges --keep-empty: don't prune empty

[PATCH 0/2] rebase --recreate-merges --keep-empty: don't prune empty

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

These patches apply on top of js/rebase-recreate-merge. They extend
the --keep-empty fix from maint [1] to work with --recreate-merges.

[1] https://public-inbox.org/git/20180320100315.15261-3-phillip.wood@talktalk.net/T/#u

Phillip Wood (2):
  add failing test for rebase --recreate-merges --keep-empty
  rebase --recreate-merges --keep-empty: don't prune empty commits

 sequencer.c                       | 30 ++++++++++++++++--------------
 t/t3421-rebase-topology-linear.sh |  3 ++-
 2 files changed, 18 insertions(+), 15 deletions(-)

-- 
2.16.2

[PATCH 1/2] add failing test for rebase --recreate-merges --keep-empty

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

If there are empty commits on the left hand side of $upstream...HEAD
then the empty commits on the right hand side that we want to keep are
being pruned. This will be fixed in the next commit.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 t/t3421-rebase-topology-linear.sh | 1 +
 1 file changed, 1 insertion(+)

diff --git a/t/t3421-rebase-topology-linear.sh b/t/t3421-rebase-topology-linear.sh
index 68fe2003ef..cb7f176f1d 100755
--- a/t/t3421-rebase-topology-linear.sh
+++ b/t/t3421-rebase-topology-linear.sh
@@ -217,6 +217,7 @@ test_run_rebase success ''
 test_run_rebase failure -m
 test_run_rebase failure -i
 test_run_rebase failure -p
+test_run_rebase failure --recreate-merges
 
 #       m
 #      /
-- 
2.16.2

[PATCH 2/2] rebase --recreate-merges --keep-empty: don't prune empty commits

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

If there are empty commits on the left hand side of $upstream...HEAD
then the empty commits on the right hand side that we want to keep are
pruned because they are marked as cherry-picks. Fix this by keeping
the commits that are empty or are not marked as cherry-picks.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 sequencer.c                       | 30 ++++++++++++++++--------------
 t/t3421-rebase-topology-linear.sh |  4 ++--
 2 files changed, 18 insertions(+), 16 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index d8cc63dbe4..da87c390ed 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2975,14 +2975,15 @@ static int make_script_with_merges(struct pretty_print_context *pp,
 	 */
 	while ((commit = get_revision(revs))) {
 		struct commit_list *to_merge;
-		int is_octopus;
+		int is_octopus, is_empty;
 		const char *p1, *p2;
 		struct object_id *oid;
 
 		tail = &commit_list_insert(commit, tail)->next;
 		oidset_insert(&interesting, &commit->object.oid);
 
-		if ((commit->object.flags & PATCHSAME))
+		is_empty = is_original_commit_empty(commit);
+		if (!is_empty && (commit->object.flags & PATCHSAME))
 			continue;
 
 		strbuf_reset(&oneline);
@@ -2992,7 +2993,7 @@ static int make_script_with_merges(struct pretty_print_context *pp,
 		if (!to_merge) {
 			/* non-merge commit: easy case */
 			strbuf_reset(&buf);
-			if (!keep_empty && is_original_commit_empty(commit))
+			if (!keep_empty && is_empty)
 				strbuf_addf(&buf, "%c ", comment_line_char);
 			strbuf_addf(&buf, "%s %s %s", cmd_pick,
 				    oid_to_hex(&commit->object.oid),
@@ -3172,12 +3173,9 @@ int sequencer_make_script(FILE *out, int argc, const char **argv,
 
 	init_revisions(&revs, NULL);
 	revs.verbose_header = 1;
-	if (recreate_merges)
-		revs.cherry_mark = 1;
-	else {
+	if (!recreate_merges)
 		revs.max_parents = 1;
-		revs.cherry_pick = 1;
-	}
+	revs.cherry_mark = 1;
 	revs.limited = 1;
 	revs.reverse = 1;
 	revs.right_only = 1;
@@ -3205,14 +3203,18 @@ int sequencer_make_script(FILE *out, int argc, const char **argv,
 		return make_script_with_merges(&pp, &revs, out, flags);
 
 	while ((commit = get_revision(&revs))) {
+		int is_empty  = is_original_commit_empty(commit);
+
 		strbuf_reset(&buf);
-		if (!keep_empty && is_original_commit_empty(commit))
+		if (!keep_empty && is_empty)
 			strbuf_addf(&buf, "%c ", comment_line_char);
-		strbuf_addf(&buf, "%s %s ", insn,
-			    oid_to_hex(&commit->object.oid));
-		pretty_print_commit(&pp, commit, &buf);
-		strbuf_addch(&buf, '\n');
-		fputs(buf.buf, out);
+		if (is_empty || !(commit->object.flags & PATCHSAME)) {
+			strbuf_addf(&buf, "%s %s ", insn,
+				    oid_to_hex(&commit->object.oid));
+			pretty_print_commit(&pp, commit, &buf);
+			strbuf_addch(&buf, '\n');
+			fputs(buf.buf, out);
+		}
 	}
 	strbuf_release(&buf);
 	return 0;
diff --git a/t/t3421-rebase-topology-linear.sh b/t/t3421-rebase-topology-linear.sh
index cb7f176f1d..7384010075 100755
--- a/t/t3421-rebase-topology-linear.sh
+++ b/t/t3421-rebase-topology-linear.sh
@@ -215,9 +215,9 @@ test_run_rebase () {
 }
 test_run_rebase success ''
 test_run_rebase failure -m
-test_run_rebase failure -i
+test_run_rebase success -i
 test_run_rebase failure -p
-test_run_rebase failure --recreate-merges
+test_run_rebase success --recreate-merges
 
 #       m
 #      /
-- 
2.16.2

Re: [PATCH 2/2] rebase --recreate-merges --keep-empty: don't prune empty commits

[Johannes Schindelin]

Hi Phillip,

On Tue, 20 Mar 2018, Phillip Wood wrote:

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
> 
> If there are empty commits on the left hand side of $upstream...HEAD
> then the empty commits on the right hand side that we want to keep are
> pruned because they are marked as cherry-picks. Fix this by keeping
> the commits that are empty or are not marked as cherry-picks.

Thank you!

> @@ -3172,12 +3173,9 @@ int sequencer_make_script(FILE *out, int argc, const char **argv,
>  
>  	init_revisions(&revs, NULL);
>  	revs.verbose_header = 1;
> -	if (recreate_merges)
> -		revs.cherry_mark = 1;
> -	else {
> +	if (!recreate_merges)
>  		revs.max_parents = 1;
> -		revs.cherry_pick = 1;
> -	}
> +	revs.cherry_mark = 1;
>  	revs.limited = 1;
>  	revs.reverse = 1;
>  	revs.right_only = 1;

Yeah, this looks ugly. I'd rather have your patch series applied first and
then rebase my --recreate-merges patch series on top.

Ciao,
Dscho

Re: [PATCH 0/2] rebase --recreate-merges --keep-empty: don't prune empty

[Johannes Schindelin]

Hi Phillip,

On Tue, 20 Mar 2018, Phillip Wood wrote:

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
> 
> These patches apply on top of js/rebase-recreate-merge. They extend
> the --keep-empty fix from maint [1] to work with --recreate-merges.

As indicated in another reply, I'd rather rebase the --recreate-merges
patches on top of your --keep-empty patch series. This obviously means
that I would fold essentially all of your 2/2 changes into my
"rebase-helper --make-script: introduce a flag to recreate merges"

The 1/2 (with s/failure/success/g) would then be added to the
--recreate-merges patch series at the end.

Would that be okay with you?

Ciao,
Dscho

Re: [PATCH 0/2] rebase --recreate-merges --keep-empty: don't prune empty

[Phillip Wood]

On 20/03/18 15:42, Johannes Schindelin wrote:
> Hi Phillip,
> 
> On Tue, 20 Mar 2018, Phillip Wood wrote:
> 
>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>
>> These patches apply on top of js/rebase-recreate-merge. They extend
>> the --keep-empty fix from maint [1] to work with --recreate-merges.
> 
> As indicated in another reply, I'd rather rebase the --recreate-merges
> patches on top of your --keep-empty patch series. This obviously means
> that I would fold essentially all of your 2/2 changes into my
> "rebase-helper --make-script: introduce a flag to recreate merges"
> 
> The 1/2 (with s/failure/success/g) would then be added to the
> --recreate-merges patch series at the end.
> 
> Would that be okay with you?

Yes, that's fine, it would give a clearer history

Best Wishes

Phillip

Re: [PATCH 0/2] rebase --recreate-merges --keep-empty: don't prune empty

[Junio C Hamano]

Phillip Wood <phillip.wood@talktalk.net> writes:

> On 20/03/18 15:42, Johannes Schindelin wrote:
> ...
>> As indicated in another reply, I'd rather rebase the --recreate-merges
>> patches on top of your --keep-empty patch series. This obviously means
>> that I would fold essentially all of your 2/2 changes into my
>> "rebase-helper --make-script: introduce a flag to recreate merges"
>> 
>> The 1/2 (with s/failure/success/g) would then be added to the
>> --recreate-merges patch series at the end.
>> 
>> Would that be okay with you?
>
> Yes, that's fine, it would give a clearer history

With or without the above plan, what we saw from you were a bit
messy to queue.  The --keep-empty fix series is based on 'maint',
while the --signoff series depends on changes that happened to
sequencer between 'maint' and 'master', but yet depends on the
former.

In what I'll be pushing out at the end of today's integration run,
I'll have two topics organized this way:

 - pw/rebase-keep-empty-fixes: built by applying the three
   '--keep-empty' patches on top of 'maint'.

 - pw/rebase-signoff: built by first merging the above to 0f57f731
   ("Merge branch 'pw/sequencer-in-process-commit'", 2018-02-13) and
   then applying "rebase --signoff" series.

Also, I'll revert merge of Dscho's recreate-merges topic to 'next';
doing so would probably have to invalidate a few evil merges I've
been carrying to resolve conflicts between it and bc/object-id
topic, so today's integration cycle may take a bit longer than
usual.

Re: [PATCH 0/2] rebase --recreate-merges --keep-empty: don't prune empty

[Phillip Wood]

On 20/03/18 19:32, Junio C Hamano wrote:
> Phillip Wood <phillip.wood@talktalk.net> writes:
> 
>> On 20/03/18 15:42, Johannes Schindelin wrote:
>> ...
>>> As indicated in another reply, I'd rather rebase the --recreate-merges
>>> patches on top of your --keep-empty patch series. This obviously means
>>> that I would fold essentially all of your 2/2 changes into my
>>> "rebase-helper --make-script: introduce a flag to recreate merges"
>>>
>>> The 1/2 (with s/failure/success/g) would then be added to the
>>> --recreate-merges patch series at the end.
>>>
>>> Would that be okay with you?
>>
>> Yes, that's fine, it would give a clearer history
> 
> With or without the above plan, what we saw from you were a bit
> messy to queue.  The --keep-empty fix series is based on 'maint',
> while the --signoff series depends on changes that happened to
> sequencer between 'maint' and 'master', but yet depends on the
> former.

Yes, that is awkward and unfortunate but the idea behind splitting them
into two separate series was to have a single set of bug fixes in the
history. The feature needed to be based on master, so if I'd had the bug
fixes in the same series you'd of had to cherry-pick them to maint which
would break branch/tag --contains. I'm not sure if that is a better option.

Best Wishes

Phillip

> In what I'll be pushing out at the end of today's integration run,
> I'll have two topics organized this way:
> 
>  - pw/rebase-keep-empty-fixes: built by applying the three
>    '--keep-empty' patches on top of 'maint'.
> 
>  - pw/rebase-signoff: built by first merging the above to 0f57f731
>    ("Merge branch 'pw/sequencer-in-process-commit'", 2018-02-13) and
>    then applying "rebase --signoff" series.
> 
> Also, I'll revert merge of Dscho's recreate-merges topic to 'next';
> doing so would probably have to invalidate a few evil merges I've
> been carrying to resolve conflicts between it and bc/object-id
> topic, so today's integration cycle may take a bit longer than
> usual.
> 

Re: [PATCH 0/2] rebase --recreate-merges --keep-empty: don't prune empty

[Junio C Hamano]

Phillip Wood <phillip.wood@talktalk.net> writes:

> On 20/03/18 19:32, Junio C Hamano wrote:
>
>> With or without the above plan, what we saw from you were a bit
>> messy to queue.  The --keep-empty fix series is based on 'maint',
>> while the --signoff series depends on changes that happened to
>> sequencer between 'maint' and 'master', but yet depends on the
>> former.
>
> Yes, that is awkward and unfortunate but the idea behind splitting them
> into two separate series was to have a single set of bug fixes in the
> history. The feature needed to be based on master, so if I'd had the bug
> fixes in the same series you'd of had to cherry-pick them to maint which
> would break branch/tag --contains. I'm not sure if that is a better option.

I said "a bit messy" but that was a statement of a fact, not a
complaint.  Sometimes, we cannot avoid that necessary solutions to
real-life problems must be messy.

I still think what you sent was the best organization, given the
constraints ;-).

Thanks.