[PATCH] add test case for rebase of empty commit

[PATCH] add test case for rebase of empty commit

[Martin von Zweigbergk]

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

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

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

[Junio C Hamano]

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

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

[Neil Horman]

On Wed, Jun 27, 2012 at 02:02:34PM -0700, Junio C Hamano wrote:
> 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
 

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

[Neil Horman]

On Wed, Jun 27, 2012 at 02:02:34PM -0700, Junio C Hamano wrote:
> 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

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

[Junio C Hamano]

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.

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

[Neil Horman]

On Tue, Jul 03, 2012 at 12:00:27PM -0700, Junio C Hamano wrote:
> 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

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

[Junio C Hamano]

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

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

[Neil Horman]

On Tue, Jul 03, 2012 at 02:13:57PM -0700, Junio C Hamano wrote:
> 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