threads / patch / 30916

patchadd test case for rebase of empty commit

Subject: [PATCH] add test case for rebase of empty commit

## tl;dr

8 messages between Jun 27, 2012 and Jul 3, 2012. Diffs are folded; open one to read it.

replies: 7people: 3as markdown or json

Martin von Zweigbergk· Jun 27, 2012, 16:22 UTC · lore

---
 t/t3401-rebase-partial.sh |    8 ++++++++
 1 file changed, 8 insertions(+)

Show changes to t/t3401-rebase-partial.sh +8 −0

diff --git a/t/t3401-rebase-partial.sh b/t/t3401-rebase-partial.sh
index 7ba1797..7f8693b 100755
--- a/t/t3401-rebase-partial.sh
+++ b/t/t3401-rebase-partial.sh
@@ -42,4 +42,12 @@ test_expect_success 'rebase --merge topic branch that was partially merged upstr
 	test_path_is_missing .git/rebase-merge
 '
 
+test_expect_success 'rebase ignores empty commit' '
+	git reset --hard A &&
+	git commit --allow-empty -m empty &&
+	test_commit D &&
+	git rebase C &&
+	test $(git log --format=%s C..) = "D"
+'
+
 test_done

-- 
1.7.9.3.327.g2980b

Junio C Hamano· Jun 27, 2012, 21:02 UTC · re: Martin von Zweigbergk · lore

Re: [PATCH] add test case for rebase of empty commit

Thanks.

We recently had a topic to add an option to allow rebase to carry empty commits forward, but I notice that it only had tests for the component cherry-pick to keep empty or redundant commits, so it may not be a bad idea to add tests for that series to the same t3401 after this commit (Neil Horman CC'ed).

Neil Horman· Jun 28, 2012, 11:30 UTC · re: Junio C Hamano · lore

Re: [PATCH] add test case for rebase of empty commit

On Wed, Jun 27, 2012 at 02:02:34PM -0700, Junio C Hamano wrote:

Show 9 quoted lines

> Thanks.
> 
> We recently had a topic to add an option to allow rebase to carry
> empty commits forward, but I notice that it only had tests for the
> component cherry-pick to keep empty or redundant commits, so it may
> not be a bad idea to add tests for that series to the same t3401
> after this commit (Neil Horman CC'ed).
> 
>

So if I understand correctly, the desire is to augment t3401 such that it adds a test in which both of the commits in the local branch are empty, and still correctly identifies the one that was cherry-picked and only adds the remaining one during the rebase?

Yes, I think that sounds like a good idea. I'm in the middle of an sctp project at the moment, but I expect to complete it in the next few days. I can look into writing this next week if you like.

Thanks & Regards
Neil

Neil Horman· Jul 3, 2012, 18:20 UTC · re: Junio C Hamano · lore

Re: [PATCH] add test case for rebase of empty commit

On Wed, Jun 27, 2012 at 02:02:34PM -0700, Junio C Hamano wrote:

Show 9 quoted lines

> Thanks.
> 
> We recently had a topic to add an option to allow rebase to carry
> empty commits forward, but I notice that it only had tests for the
> component cherry-pick to keep empty or redundant commits, so it may
> not be a bad idea to add tests for that series to the same t3401
> after this commit (Neil Horman CC'ed).
> 
>

So, I've been thinking about this some, and I'm a bit stuck on it. Reading the test description for t3401, I see that we're testing gits ability to detect patches merged upstream when doing a rebase. That said, how are we supposed to differentiate between upstream empty patches that have been cherry-picked or merged, and local branch empty changes that haven't. As humans we can see that the changelog might be the same, but git has no way to detect that, and if --allow-empty is specified will just apply any empty patch it finds between the two branches merge base and the topic branch head. Does anyone have an idea as to how we should detect such duplication?

Neil

Junio C Hamano· Jul 3, 2012, 19:00 UTC · re: Neil Horman · lore

Re: [PATCH] add test case for rebase of empty commit

Neil Horman <nhorman@tuxdriver.com> writes:

Show 9 quoted lines

> So, I've been thinking about this some, and I'm a bit stuck on it.  Reading the
> test description for t3401, I see that we're testing gits ability to detect
> patches merged upstream when doing a rebase.  That said, how are we supposed to
> differentiate between upstream empty patches that have been cherry-picked or
> merged, and local branch empty changes that haven't.  As humans we can see that
> the changelog might be the same, but git has no way to detect that, and if
> --allow-empty is specified will just apply any empty patch it finds between the
> two branches merge base and the topic branch head.  Does anyone have an idea as
> to how we should detect such duplication?

The changelog might be similar or textually identical, but it is entirely a different matter if it makes sense taken out of the context (i.e. cherry-picked). So I would personally do not bother "filtering" about them too much---if you ask for empties, you will get all empties.

Neil Horman· Jul 3, 2012, 20:31 UTC · re: Junio C Hamano · lore

Re: [PATCH] add test case for rebase of empty commit

On Tue, Jul 03, 2012 at 12:00:27PM -0700, Junio C Hamano wrote:

Show 18 quoted lines

> Neil Horman <nhorman@tuxdriver.com> writes:
> 
> > So, I've been thinking about this some, and I'm a bit stuck on it.  Reading the
> > test description for t3401, I see that we're testing gits ability to detect
> > patches merged upstream when doing a rebase.  That said, how are we supposed to
> > differentiate between upstream empty patches that have been cherry-picked or
> > merged, and local branch empty changes that haven't.  As humans we can see that
> > the changelog might be the same, but git has no way to detect that, and if
> > --allow-empty is specified will just apply any empty patch it finds between the
> > two branches merge base and the topic branch head.  Does anyone have an idea as
> > to how we should detect such duplication?
> 
> The changelog might be similar or textually identical, but it is
> entirely a different matter if it makes sense taken out of the
> context (i.e. cherry-picked).  So I would personally do not bother
> "filtering" about them too much---if you ask for empties, you will
> get all empties.
>

Ok, copy that. Thanks! Neil

Junio C Hamano· Jul 3, 2012, 21:13 UTC · re: Neil Horman · lore

Re: [PATCH] add test case for rebase of empty commit

Neil Horman <nhorman@tuxdriver.com> writes:

Show 9 quoted lines

> On Tue, Jul 03, 2012 at 12:00:27PM -0700, Junio C Hamano wrote:
>> 
>> The changelog might be similar or textually identical, but it is
>> entirely a different matter if it makes sense taken out of the
>> context (i.e. cherry-picked).  So I would personally do not bother
>> "filtering" about them too much---if you ask for empties, you will
>> get all empties.
>> 
> Ok, copy that.

That was somewhat unexpected, though ;-) It was 30% tongue-in-cheek comment. People who want to keep the empty commits in the history may want some filtering. As I am not among them, I do not think of anything useful (other than "filter all empty ones away", that is).

Neil Horman· Jul 3, 2012, 23:40 UTC · re: Junio C Hamano · lore

Re: [PATCH] add test case for rebase of empty commit

On Tue, Jul 03, 2012 at 02:13:57PM -0700, Junio C Hamano wrote:

Show 18 quoted lines

> Neil Horman <nhorman@tuxdriver.com> writes:
> 
> > On Tue, Jul 03, 2012 at 12:00:27PM -0700, Junio C Hamano wrote:
> >> 
> >> The changelog might be similar or textually identical, but it is
> >> entirely a different matter if it makes sense taken out of the
> >> context (i.e. cherry-picked).  So I would personally do not bother
> >> "filtering" about them too much---if you ask for empties, you will
> >> get all empties.
> >> 
> > Ok, copy that.
> 
> That was somewhat unexpected, though ;-) It was 30% tongue-in-cheek
> comment.  People who want to keep the empty commits in the history
> may want some filtering. As I am not among them, I do not think of
> anything useful (other than "filter all empty ones away", that is).
> 
>

I understand what you're saying (for the record, I'm ok with the duplicates, to be fixed up at a later date, as opposed to dropping them all). But the fact remains, theres not obvious differentiator, other than some fuzzy search on the changelog we can use to differentiate empty commits. Let me think about it some more, maybe theres some sort of policy specification we can make regarding the changelog that would let us intellegently filter empty commits appropriately. Best Neil