git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
* [PATCH 0/3] rebase --keep-empty/--root fixes
@ 2018-03-20 10:03 Phillip Wood
  2018-03-20 10:03 ` [PATCH 1/3] rebase --root: stop assuming squash_onto is unset Phillip Wood
                   ` (3 more replies)
  0 siblings, 4 replies; 10+ messages in thread
From: Phillip Wood @ 2018-03-20 10:03 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Junio C Hamano, Johannes Schindelin, Phillip Wood

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

These patches are based on top of maint. The first two are a reworking
of "[PATCH 2/2] rebase --root -k: fix when root commit is empty" [1]

[1] https://public-inbox.org/git/20180314111127.14217-2-phillip.wood@talktalk.net/

Phillip Wood (3):
  rebase --root: stop assuming squash_onto is unset
  rebase -i --keep-empty: don't prune empty commits
  rebase: respect --no-keep-empty

 git-rebase.sh                     |  4 ++++
 sequencer.c                       | 18 +++++++++++-------
 t/t3421-rebase-topology-linear.sh |  2 +-
 3 files changed, 16 insertions(+), 8 deletions(-)

-- 
2.16.2


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

* [PATCH 1/3] rebase --root: stop assuming squash_onto is unset
  2018-03-20 10:03 [PATCH 0/3] rebase --keep-empty/--root fixes Phillip Wood
@ 2018-03-20 10:03 ` Phillip Wood
  2018-03-20 10:03 ` [PATCH 2/3] rebase -i --keep-empty: don't prune empty commits Phillip Wood
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 10+ messages in thread
From: Phillip Wood @ 2018-03-20 10:03 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Junio C Hamano, Johannes Schindelin, Phillip Wood

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

If the user set the environment variable 'squash_onto', the 'rebase'
command would erroneously assume that the user passed the option
'--root'.

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

diff --git a/git-rebase.sh b/git-rebase.sh
index fd72a35c65..8b1b892d72 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -60,6 +60,7 @@ $(gettext 'Resolve all conflicts manually, mark them as resolved with
 You can instead skip this commit: run "git rebase --skip".
 To abort and get back to the state before "git rebase", run "git rebase --abort".')
 "
+squash_onto=
 unset onto
 unset restrict_revision
 cmd=
-- 
2.16.2


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

* [PATCH 2/3] rebase -i --keep-empty: don't prune empty commits
  2018-03-20 10:03 [PATCH 0/3] rebase --keep-empty/--root fixes Phillip Wood
  2018-03-20 10:03 ` [PATCH 1/3] rebase --root: stop assuming squash_onto is unset Phillip Wood
@ 2018-03-20 10:03 ` Phillip Wood
  2018-03-20 15:33   ` Johannes Schindelin
  2018-03-20 10:03 ` [PATCH 3/3] rebase: respect --no-keep-empty Phillip Wood
       [not found] ` <nycvar.QRO.7.76.6.1803201634260.55@ZVAVAG-6OXH6DA.rhebcr.pbec.zvpebfbsg.pbz>
  3 siblings, 1 reply; 10+ messages in thread
From: Phillip Wood @ 2018-03-20 10:03 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Junio C Hamano, Johannes Schindelin, 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 by --cherry-pick. Fix this by using --cherry-mark instead of
--cherry-pick and 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                       | 18 +++++++++++-------
 t/t3421-rebase-topology-linear.sh |  2 +-
 2 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 4d3f60594c..96ff812c8d 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2470,7 +2470,7 @@ int sequencer_make_script(FILE *out, int argc, const char **argv,
 	init_revisions(&revs, NULL);
 	revs.verbose_header = 1;
 	revs.max_parents = 1;
-	revs.cherry_pick = 1;
+	revs.cherry_mark = 1;
 	revs.limited = 1;
 	revs.reverse = 1;
 	revs.right_only = 1;
@@ -2495,14 +2495,18 @@ int sequencer_make_script(FILE *out, int argc, const char **argv,
 		return error(_("make_script: error preparing revisions"));
 
 	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 68fe2003ef..52fc6885e5 100755
--- a/t/t3421-rebase-topology-linear.sh
+++ b/t/t3421-rebase-topology-linear.sh
@@ -215,7 +215,7 @@ 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
 
 #       m
-- 
2.16.2


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

* [PATCH 3/3] rebase: respect --no-keep-empty
  2018-03-20 10:03 [PATCH 0/3] rebase --keep-empty/--root fixes Phillip Wood
  2018-03-20 10:03 ` [PATCH 1/3] rebase --root: stop assuming squash_onto is unset Phillip Wood
  2018-03-20 10:03 ` [PATCH 2/3] rebase -i --keep-empty: don't prune empty commits Phillip Wood
@ 2018-03-20 10:03 ` Phillip Wood
       [not found] ` <nycvar.QRO.7.76.6.1803201634260.55@ZVAVAG-6OXH6DA.rhebcr.pbec.zvpebfbsg.pbz>
  3 siblings, 0 replies; 10+ messages in thread
From: Phillip Wood @ 2018-03-20 10:03 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Junio C Hamano, Johannes Schindelin, Phillip Wood

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

$OPT_SPEC has always allowed --no-keep-empty so lets start handling
it.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 git-rebase.sh | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/git-rebase.sh b/git-rebase.sh
index 8b1b892d72..37b8f13971 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -263,6 +263,9 @@ do
 	--keep-empty)
 		keep_empty=yes
 		;;
+	--no-keep-empty)
+		keep_empty=
+		;;
 	--preserve-merges)
 		preserve_merges=t
 		test -z "$interactive_rebase" && interactive_rebase=implied
-- 
2.16.2


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

* Re: [PATCH 2/3] rebase -i --keep-empty: don't prune empty commits
  2018-03-20 10:03 ` [PATCH 2/3] rebase -i --keep-empty: don't prune empty commits Phillip Wood
@ 2018-03-20 15:33   ` Johannes Schindelin
  2018-03-20 17:34     ` Junio C Hamano
  0 siblings, 1 reply; 10+ messages in thread
From: Johannes Schindelin @ 2018-03-20 15:33 UTC (permalink / raw)
  To: Phillip Wood; +Cc: Git Mailing List, Junio C Hamano

Hi Phillip,

On Tue, 20 Mar 2018, Phillip Wood wrote:

> diff --git a/sequencer.c b/sequencer.c
> index 4d3f60594c..96ff812c8d 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -2470,7 +2470,7 @@ int sequencer_make_script(FILE *out, int argc, const char **argv,
>  	init_revisions(&revs, NULL);
>  	revs.verbose_header = 1;
>  	revs.max_parents = 1;
> -	revs.cherry_pick = 1;
> +	revs.cherry_mark = 1;

This will conflict with my --recreate-merges patch series, but in a good
way.

> @@ -2495,14 +2495,18 @@ int sequencer_make_script(FILE *out, int argc, const char **argv,
>  		return error(_("make_script: error preparing revisions"));
>  
>  	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)) {

May I suggest inverting the logic here, to make the code more obvious and
also to avoid indenting the block even further?

		if (!is_empty && (commit->object.flags & PATCHSAME))
			continue;

> +			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;

Thanks,
Dscho

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

* Re: [PATCH 0/3] rebase --keep-empty/--root fixes
       [not found] ` <nycvar.QRO.7.76.6.1803201634260.55@ZVAVAG-6OXH6DA.rhebcr.pbec.zvpebfbsg.pbz>
@ 2018-03-20 15:36   ` Johannes Schindelin
  0 siblings, 0 replies; 10+ messages in thread
From: Johannes Schindelin @ 2018-03-20 15:36 UTC (permalink / raw)
  To: Phillip Wood; +Cc: Git Mailing List, Junio C Hamano

... and now also Cc:ing the list and Junio...


On Tue, 20 Mar 2018, Johannes Schindelin wrote:

> Hi Phillip,
> 
> On Tue, 20 Mar 2018, Phillip Wood wrote:
> 
> > Phillip Wood (3):
> >   rebase --root: stop assuming squash_onto is unset
> >   rebase -i --keep-empty: don't prune empty commits
> >   rebase: respect --no-keep-empty
> 
> Those patches look good. I offered a suggestion to 2/3 to avoid indenting
> a code block, but I would also be okay to leave it as-is.
> 
> Thanks,
> Dscho
> 

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

* Re: [PATCH 2/3] rebase -i --keep-empty: don't prune empty commits
  2018-03-20 15:33   ` Johannes Schindelin
@ 2018-03-20 17:34     ` Junio C Hamano
  2018-03-20 18:39       ` Phillip Wood
  2018-03-21 22:38       ` Johannes Schindelin
  0 siblings, 2 replies; 10+ messages in thread
From: Junio C Hamano @ 2018-03-20 17:34 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: Phillip Wood, Git Mailing List

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> +		if (!keep_empty && is_empty)
>>  			strbuf_addf(&buf, "%c ", comment_line_char);

We are not trying to preserve an empty one, and have found an empty
one, so we comment it out, and then...

>> +		if (is_empty || !(commit->object.flags & PATCHSAME)) {
>
> May I suggest inverting the logic here, to make the code more obvious and
> also to avoid indenting the block even further?
>
> 		if (!is_empty && (commit->object.flags & PATCHSAME))
> 			continue;

... if a non-empty one that already appears in the upstream, we do
not do anything to it.  There is no room for keep-empty or lack of
it to affect what happens to these commits.

Otherwise the insn is emitted for the commit.

>> +			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);
>> +		}

I tend to agree that the suggested structure is easier to follow
than Phillip's version.

But I wonder if this is even easier to follow.  It makes it even
more clear that patchsame commits that are not empty are discarded
unconditionally.

	while ((commit = get_revision(&revs))) {
		int is_empty  = is_original_commit_empty(commit);
		if (!is_empty && (commit->object.flags & PATCHSAME))
			continue;
		strbuf_reset(&buf);
		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);
	}

Or did I screw up the rewrite?

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

* Re: [PATCH 2/3] rebase -i --keep-empty: don't prune empty commits
  2018-03-20 17:34     ` Junio C Hamano
@ 2018-03-20 18:39       ` Phillip Wood
  2018-03-21 22:38       ` Johannes Schindelin
  1 sibling, 0 replies; 10+ messages in thread
From: Phillip Wood @ 2018-03-20 18:39 UTC (permalink / raw)
  To: Junio C Hamano, Johannes Schindelin; +Cc: Phillip Wood, Git Mailing List

On 20/03/18 17:34, Junio C Hamano wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
>>> +		if (!keep_empty && is_empty)
>>>  			strbuf_addf(&buf, "%c ", comment_line_char);
> 
> We are not trying to preserve an empty one, and have found an empty
> one, so we comment it out, and then...
> 
>>> +		if (is_empty || !(commit->object.flags & PATCHSAME)) {
>>
>> May I suggest inverting the logic here, to make the code more obvious and
>> also to avoid indenting the block even further?
>>
>> 		if (!is_empty && (commit->object.flags & PATCHSAME))
>> 			continue;
> 
> ... if a non-empty one that already appears in the upstream, we do
> not do anything to it.  There is no room for keep-empty or lack of
> it to affect what happens to these commits.
> 
> Otherwise the insn is emitted for the commit.
> 
>>> +			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);
>>> +		}
> 
> I tend to agree that the suggested structure is easier to follow
> than Phillip's version.
> 
> But I wonder if this is even easier to follow.  It makes it even
> more clear that patchsame commits that are not empty are discarded
> unconditionally.
> 
> 	while ((commit = get_revision(&revs))) {
> 		int is_empty  = is_original_commit_empty(commit);
> 		if (!is_empty && (commit->object.flags & PATCHSAME))
> 			continue;
> 		strbuf_reset(&buf);
> 		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);
> 	}
> 
> Or did I screw up the rewrite?
> 
I've not tested it but I think it's OK, I agree that it is clearer than
my version

Best Wishes

Phillip

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

* Re: [PATCH 2/3] rebase -i --keep-empty: don't prune empty commits
  2018-03-20 17:34     ` Junio C Hamano
  2018-03-20 18:39       ` Phillip Wood
@ 2018-03-21 22:38       ` Johannes Schindelin
  2018-03-29 18:05         ` Junio C Hamano
  1 sibling, 1 reply; 10+ messages in thread
From: Johannes Schindelin @ 2018-03-21 22:38 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Phillip Wood, Git Mailing List

Hi Junio,

On Tue, 20 Mar 2018, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> >> +		if (!keep_empty && is_empty)
> >>  			strbuf_addf(&buf, "%c ", comment_line_char);
> 
> We are not trying to preserve an empty one, and have found an empty
> one, so we comment it out, and then...
> 
> >> +		if (is_empty || !(commit->object.flags & PATCHSAME)) {
> >
> > May I suggest inverting the logic here, to make the code more obvious and
> > also to avoid indenting the block even further?
> >
> > 		if (!is_empty && (commit->object.flags & PATCHSAME))
> > 			continue;
> 
> ... if a non-empty one that already appears in the upstream, we do
> not do anything to it.  There is no room for keep-empty or lack of
> it to affect what happens to these commits.
> 
> Otherwise the insn is emitted for the commit.
> 
> >> +			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);
> >> +		}
> 
> I tend to agree that the suggested structure is easier to follow
> than Phillip's version.
> 
> But I wonder if this is even easier to follow.  It makes it even
> more clear that patchsame commits that are not empty are discarded
> unconditionally.
> 
> 	while ((commit = get_revision(&revs))) {
> 		int is_empty  = is_original_commit_empty(commit);
> 		if (!is_empty && (commit->object.flags & PATCHSAME))
> 			continue;
> 		strbuf_reset(&buf);
> 		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);
> 	}
> 
> Or did I screw up the rewrite?

This looks correct. And the postimage is easier to follow than the one of
my suggested change.

My version is easier to review on the mailing list, of course, as it
minimizes the diff... ;-)

Ciao,
Dscho

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

* Re: [PATCH 2/3] rebase -i --keep-empty: don't prune empty commits
  2018-03-21 22:38       ` Johannes Schindelin
@ 2018-03-29 18:05         ` Junio C Hamano
  0 siblings, 0 replies; 10+ messages in thread
From: Junio C Hamano @ 2018-03-29 18:05 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: Phillip Wood, Git Mailing List

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> But I wonder if this is even easier to follow.  It makes it even
>> more clear that patchsame commits that are not empty are discarded
>> unconditionally.
>> 
>> 	while ((commit = get_revision(&revs))) {
>> 		int is_empty  = is_original_commit_empty(commit);
>> 		if (!is_empty && (commit->object.flags & PATCHSAME))
>> 			continue;
>> 		strbuf_reset(&buf);
>> 		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);
>> 	}
>> 
>> Or did I screw up the rewrite?
>
> This looks correct. And the postimage is easier to follow than the one of
> my suggested change.

OK, let's squash this in and rebuild both pw/rebase-keep-empty-fixes
and also pw/rebase-signoff that builds on this topic, so that they
can be advanced to 'next'.


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

end of thread, other threads:[~2018-03-29 18:22 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-03-20 10:03 [PATCH 0/3] rebase --keep-empty/--root fixes Phillip Wood
2018-03-20 10:03 ` [PATCH 1/3] rebase --root: stop assuming squash_onto is unset Phillip Wood
2018-03-20 10:03 ` [PATCH 2/3] rebase -i --keep-empty: don't prune empty commits Phillip Wood
2018-03-20 15:33   ` Johannes Schindelin
2018-03-20 17:34     ` Junio C Hamano
2018-03-20 18:39       ` Phillip Wood
2018-03-21 22:38       ` Johannes Schindelin
2018-03-29 18:05         ` Junio C Hamano
2018-03-20 10:03 ` [PATCH 3/3] rebase: respect --no-keep-empty Phillip Wood
     [not found] ` <nycvar.QRO.7.76.6.1803201634260.55@ZVAVAG-6OXH6DA.rhebcr.pbec.zvpebfbsg.pbz>
2018-03-20 15:36   ` [PATCH 0/3] rebase --keep-empty/--root fixes Johannes Schindelin

Code repositories for project(s) associated with this inbox:

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

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