rebase -p confusion in 1.6.1

rebase -p confusion in 1.6.1

[Sitaram Chamarty]

Hello all,

While trying to understand "rebase -p", I came across some
very unexpected behaviour that made me throw in the towel
and ask for help!

The outputs I got really confused me.  Before the "rebase
-p", the tree looked like
    
    * 78ffda9... (refs/heads/work) b4
    * be1e3a4... b3
    *   cd8d893... Merge branch 'master' into work
    |\
    | * 0153c27... (refs/heads/master) a4
    | * 74f4387... a3
    * | f1b0c1c... b2
    * | 2e202d0... b1
    |/
    * b37ae36... a2
    * ed1e1bc... a1

But afterward, this is what it looks like -- all the "b"
commits are gone!

    * 0153c27... (refs/heads/work, refs/heads/master) a4
    * 74f4387... a3
    * b37ae36... a2
    * ed1e1bc... a1

What did I do wrong/misunderstand?

Here's how to recreate.  Note that "testci" is a shell
function and "lg" is a git alias.  They are, respectively,
(1) testci() { for i; do echo $i > $i; git add $i; git commit -m $i; done; }
(2) git config alias.lg log --graph --pretty=oneline --abbrev-commit --decorate

    git init
    testci a1 a2
    git checkout -b work
    testci b1 b2
    git checkout master
    testci a3 a4
    git checkout work
    git merge master
    testci b3 b4
    git --no-pager lg   # graph before rebase -p
    git rebase -p master
    git --no-pager lg   # graph after rebase -p

Re: rebase -p confusion in 1.6.1

[Johannes Schindelin]

Hi,

On Thu, 15 Jan 2009, Sitaram Chamarty wrote:

> While trying to understand "rebase -p", I came across some
> very unexpected behaviour that made me throw in the towel
> and ask for help!
>
> [... some script with some aliases ...]

I turned this into a proper test case (to show what would be most helpful 
if you report bugs like this in the future).

If nobody beats me to it, I'll work on it tonight.

-- snipsnap --
 t/t3409-rebase-preserve-merges.sh |   28 ++++++++++++++++++++++++++++
 1 files changed, 28 insertions(+), 0 deletions(-)

diff --git a/t/t3409-rebase-preserve-merges.sh b/t/t3409-rebase-preserve-merges.sh
index e6c8327..5e2128c 100755
--- a/t/t3409-rebase-preserve-merges.sh
+++ b/t/t3409-rebase-preserve-merges.sh
@@ -92,4 +92,32 @@ test_expect_success '--continue works after a conflict' '
 	)
 '
 
+test_commit () {
+	: > "$1" &&
+	git add "$1" &&
+	test_tick &&
+	git commit -m "$1" "$1"
+}
+
+test_expect_success 'test case from Sitaram' '
+
+	git checkout master &&
+	test_commit a1 &&
+	git checkout -b work &&
+	test_commit b1 &&
+	git checkout master &&
+	test_commit a3 &&
+	git checkout work &&
+	git merge master &&
+	test_commit b3 &&
+	echo before: &&
+	git log --graph --pretty=oneline --decorate --abbrev-commit &&
+	test -f b3 &&
+	git rebase -p master &&
+	echo after: &&
+	git log --graph --pretty=oneline --decorate --abbrev-commit &&
+	test -f b3
+
+'
+
 test_done

Re: rebase -p confusion in 1.6.1

[Sitaram Chamarty]

On 2009-01-15, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

> I turned this into a proper test case (to show what would be most helpful 
> if you report bugs like this in the future).

Thanks.  I'll keep that in mind.

What is the significance of test_tick?  I can see what it is
doing, but am trying to understand why.

Regards,

Sitaram

Re: rebase -p confusion in 1.6.1

[Stephan Beyer]

Sitaram Chamarty wrote:
> On 2009-01-15, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> 
> > I turned this into a proper test case (to show what would be most helpful 
> > if you report bugs like this in the future).
> 
> Thanks.  I'll keep that in mind.
> 
> What is the significance of test_tick?  I can see what it is
> doing, but am trying to understand why.

When you run the test case a second, third, fourth time, the commit
times would be all different without test_tick. This is bad for
bugfixing when you need to inspect the test case repo a little
further. (When the commit times change, the commit ids change,
too.)
So setting the time and counting it artificially up is nice
to get the same SHAs over and over.

Regards,
  Stephan

-- 
Stephan Beyer <s-beyer@gmx.net>, PGP 0x6EDDD207FCC5040F

Re: rebase -p confusion in 1.6.1

[Sitaram Chamarty]

On 2009-01-15, Stephan Beyer <s-beyer@gmx.net> wrote:
> Sitaram Chamarty wrote:
>> What is the significance of test_tick?  I can see what it is
>> doing, but am trying to understand why.

> So setting the time and counting it artificially up is nice
> to get the same SHAs over and over.

I should have been clearer...

I was trying to understand why the "counting up" part is
needed.

Regards,

Sitaram

Re: rebase -p confusion in 1.6.1

[Stephan Beyer]

Hi Sitaram,

Sitaram Chamarty wrote:
> The outputs I got really confused me.  Before the "rebase
> -p", the tree looked like
>     
>     * 78ffda9... (refs/heads/work) b4
>     * be1e3a4... b3
>     *   cd8d893... Merge branch 'master' into work
>     |\
>     | * 0153c27... (refs/heads/master) a4
>     | * 74f4387... a3
>     * | f1b0c1c... b2
>     * | 2e202d0... b1
>     |/
>     * b37ae36... a2
>     * ed1e1bc... a1
> 
> But afterward, this is what it looks like -- all the "b"
> commits are gone!
> 
>     * 0153c27... (refs/heads/work, refs/heads/master) a4
>     * 74f4387... a3
>     * b37ae36... a2
>     * ed1e1bc... a1
> 
> What did I do wrong/misunderstand?

Hmm, you are rebasing onto master which is merged into the branch you
want to rebase. So, I think the correct output should be the same like
git rebase without -p, ie

* 1337bee... (refs/heads/work) b4
* deadbee... b3
* badbeef... b2
* fa1afe1... b1
* 0153c27... (refs/heads/master) a4
* 74f4387... a3
* b37ae36... a2
* ed1e1bc... a1

This is because master is already merged into work and a preserved
merge will see that everything is already merged in.

Well, so I think you've discovered a bug.

> (2) git config alias.lg log --graph --pretty=oneline --abbrev-commit --decorate

Funny, I have exactly the same alias, but named "logk".

>     git init
>     testci a1 a2
>     git checkout -b work
>     testci b1 b2
>     git checkout master
>     testci a3 a4
>     git checkout work
>     git merge master
>     testci b3 b4
>     git --no-pager lg   # graph before rebase -p
>     git rebase -p master
>     git --no-pager lg   # graph after rebase -p

Thanks and regards,
  Stephan

-- 
Stephan Beyer <s-beyer@gmx.net>, PGP 0x6EDDD207FCC5040F

Re: rebase -p confusion in 1.6.1

[Johannes Schindelin]

Hi,

On Thu, 15 Jan 2009, Stephan Beyer wrote:

> Hmm, you are rebasing onto master which is merged into the branch you 
> want to rebase. So, I think the correct output should be the same like 
> git rebase without -p, ie
> 
> * 1337bee... (refs/heads/work) b4
> * deadbee... b3
> * badbeef... b2
> * fa1afe1... b1
> * 0153c27... (refs/heads/master) a4
> * 74f4387... a3
> * b37ae36... a2
> * ed1e1bc... a1
> 
> This is because master is already merged into work and a preserved
> merge will see that everything is already merged in.

I guess the problem is that the list shown in the editor says 'noop'.  
This does not happen without -p, so something is borked in the commit 
listing with -p.

Which is no wonder: the code after line 652 in git-rebase--interactive.sh 
that handles -p is utterly incomprehensible.

Remember: there is code that is so simple that it has no obvious flaws, 
and there is code that is so complicated that it has no obvious flaws.

IMO (and I said so much back then), it was the biggest mistake of the 
whole patch series that it was so intrusive and changed everything.  It's 
not like I did not warn anybody.

The point is: you could _easily_ handle -p with _almost the same_ 
rev-list command; you'd just have to make sure that --no-merges is 
skipped.

And then you only have to make sure that the current commit is the 
(possibly rewritten version of the) first parent of the next commit to 
pick.

Ciao,
Dscho

Re: rebase -p confusion in 1.6.1

[Sitaram Chamarty]

On 2009-01-15, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> Remember: there is code that is so simple that it has no obvious flaws, 
> and there is code that is so complicated that it has no obvious flaws.
I've always heard the first part as "obviously no flaws"...

Re: rebase -p confusion in 1.6.1

[Michael J Gruber]

Sitaram Chamarty venit, vidit, dixit 15.01.2009 11:39:
> Hello all,
> 
> While trying to understand "rebase -p", I came across some
> very unexpected behaviour that made me throw in the towel
> and ask for help!
> 
> The outputs I got really confused me.  Before the "rebase
> -p", the tree looked like
>     
>     * 78ffda9... (refs/heads/work) b4
>     * be1e3a4... b3
>     *   cd8d893... Merge branch 'master' into work
>     |\
>     | * 0153c27... (refs/heads/master) a4
>     | * 74f4387... a3
>     * | f1b0c1c... b2
>     * | 2e202d0... b1
>     |/
>     * b37ae36... a2
>     * ed1e1bc... a1
> 
> But afterward, this is what it looks like -- all the "b"
> commits are gone!
> 
>     * 0153c27... (refs/heads/work, refs/heads/master) a4
>     * 74f4387... a3
>     * b37ae36... a2
>     * ed1e1bc... a1
> 
> What did I do wrong/misunderstand?
> 
> Here's how to recreate.  Note that "testci" is a shell
> function and "lg" is a git alias.  They are, respectively,
> (1) testci() { for i; do echo $i > $i; git add $i; git commit -m $i; done; }
> (2) git config alias.lg log --graph --pretty=oneline --abbrev-commit --decorate
> 
>     git init
>     testci a1 a2
>     git checkout -b work
>     testci b1 b2
>     git checkout master
>     testci a3 a4
>     git checkout work
>     git merge master
>     testci b3 b4
>     git --no-pager lg   # graph before rebase -p
>     git rebase -p master
>     git --no-pager lg   # graph after rebase -p
> 

First of all: git 1.6.0.6 gives you the unchanged graph after using
rebase -i -p (git 1.6.1 adds -i behind you back and sets up a dummy
editor). In any case, git rebase should not simply eat those commits -
either leave them alone or rewrite them. git bisect says

d80d6bc146232d81f1bb4bc58e5d89263fd228d4 is first bad commit
commit d80d6bc146232d81f1bb4bc58e5d89263fd228d4
Author: Stephen Haberman <stephen@exigencecorp.com>
Date:   Wed Oct 15 02:44:39 2008 -0500

    rebase-i-p: do not include non-first-parent commits touching UPSTREAM

so I'll cc the bad guy ;)

Second, what result do you expect? If the merge is to be preserved then
b1, b2 can't be simply ripped out - or else you get the linear structure
which rebase without '-p' delivers. The merge base (as returned by git
merge-base) between work and master is a4, i.e. master, so that the
expected result with '-p' is the one from 1.6.0.6 (unchanged graph).

Cheers,
Michael

Re: rebase -p confusion in 1.6.1

[Stephan Beyer]

Hi,

> First of all: git 1.6.0.6 gives you the unchanged graph after using
> rebase -i -p.

This is true and it is a far better behavior than now, but I think it's
not the expected behavior. (I have written about the behavior I'd expect
in another reply to the original mail.)

Also

	git rebase -i -p master

should do the same as

	git rebase -i -p --onto master master

or am I wrong?

But the latter does

	$ git rebase --onto master -i -p master
	fatal: Needed a single revision
	Invalid base

instead of resulting in an unchanged graph.
(Tested with 1.5.6.5, the only other version I have installed besides
my master branch)

Regards,
  Stephan

-- 
Stephan Beyer <s-beyer@gmx.net>, PGP 0x6EDDD207FCC5040F

Re: rebase -p confusion in 1.6.1

[Michael J Gruber]

Stephan Beyer venit, vidit, dixit 15.01.2009 14:55:
> Hi,
> 
>> First of all: git 1.6.0.6 gives you the unchanged graph after using
>> rebase -i -p.
> 
> This is true and it is a far better behavior than now, but I think it's
> not the expected behavior. (I have written about the behavior I'd expect
> in another reply to the original mail.)

Yep, I think -p should preserve only merges in side branches (and
therefore produce what you suggest, and what you get without -p). If it
preserves all merges then there is nothing to rewrite here.

BTW: How does the sequencer based rebase do in this case, and what's the
general status? If it's about to be integrated we can do without the
present script...

Michael

Re: rebase -p confusion in 1.6.1

[Johannes Schindelin]

Hi,

On Thu, 15 Jan 2009, Michael J Gruber wrote:

> Stephan Beyer venit, vidit, dixit 15.01.2009 14:55:
> 
> >> First of all: git 1.6.0.6 gives you the unchanged graph after using
> >> rebase -i -p.
> > 
> > This is true and it is a far better behavior than now, but I think it's
> > not the expected behavior. (I have written about the behavior I'd expect
> > in another reply to the original mail.)
> 
> Yep, I think -p should preserve only merges in side branches

you mean everything in master..work?

> (and therefore produce what you suggest, and what you get without -p). 
> If it preserves all merges then there is nothing to rewrite here.

The merge _is_ outside of master, so I do not understand what the heck you 
are talking about.

The more I think about it, I think it's possible I broke it with the 
introduction of the "noop".

However, there could be a _different_ test case where the current -p 
handling shows the same error.  Dunno.

Ciao,
Dscho

Re: rebase -p confusion in 1.6.1

[Michael J Gruber]

Johannes Schindelin venit, vidit, dixit 15.01.2009 15:25:
> Hi,
> 
> On Thu, 15 Jan 2009, Michael J Gruber wrote:
> 
>> Stephan Beyer venit, vidit, dixit 15.01.2009 14:55:
>>
>>>> First of all: git 1.6.0.6 gives you the unchanged graph after using
>>>> rebase -i -p.
>>> This is true and it is a far better behavior than now, but I think it's
>>> not the expected behavior. (I have written about the behavior I'd expect
>>> in another reply to the original mail.)
>> Yep, I think -p should preserve only merges in side branches
> 
> you mean everything in master..work?
> 
>> (and therefore produce what you suggest, and what you get without -p). 
>> If it preserves all merges then there is nothing to rewrite here.
> 
> The merge _is_ outside of master, so I do not understand what the heck you 
> are talking about.

Easy Dscho, easy ;)
[meaning "take it such..."]

I'm not sure what -p is supposed to do:

A) Should it preserve all merge commits which it would need to rewrite?
That is lot to ask. Previous behaviour (intended or not) seemed to be to
do nothing in this case where the merge connects master and work.

B) Should it preserve only merges in side branches? I seem to mean by
that branches where the parents are on work and other branches but not
on master.

So at least on my side there is confusion about the intention behind
'-p' (say design goal), and therefore about the expectation.

> The more I think about it, I think it's possible I broke it with the 
> introduction of the "noop".
> 
> However, there could be a _different_ test case where the current -p 
> handling shows the same error.  Dunno.

It certainly worked after the noop introduction before the r-i-p series,
but not any more after. "worked" meaning it at least didn't leave out
commits in this case (but reproduced the existing DAG). I'm getting the
impression you suggest R.I.P. for r-i-p series ;) Fine with me...

Cheers,
Michael

Re: rebase -p confusion in 1.6.1

[Johannes Schindelin]

Hi,

On Thu, 15 Jan 2009, Michael J Gruber wrote:

> I'm not sure what -p is supposed to do:
> 
> A) Should it preserve all merge commits which it would need to rewrite?
> That is lot to ask. Previous behaviour (intended or not) seemed to be to
> do nothing in this case where the merge connects master and work.
> 
> B) Should it preserve only merges in side branches? I seem to mean by
> that branches where the parents are on work and other branches but not
> on master.

The intention was this:

	$ git rebase -p master

would need to rewrite _all_ commits that are in "master..".  All of them, 
including the merge commits.

The fact that I implemented it as "-i -p" is only due to technical 
reasons; I know (ahem, now I should put that into the past tense) the code 
base pretty well.

An additional shortcut was to avoid rewriting commits when they did not 
need rewriting.  IOW if the commit-to-pick has only parents that were 
_not_ rewritten, we can avoid cherry-picking or merging, and just reset 
--hard <original commit>.

There was a problem, though; for some reason, the code as I did it fscked 
up the order of the commits when -p was specified.  Therefore, rewritten 
commits had the wrong parents.

I thought it should be easy to fix, but then I got a job that I actually 
like, so my Git time budget was tremendously reduced.

> > The more I think about it, I think it's possible I broke it with the 
> > introduction of the "noop".
> 
> It certainly worked after the noop introduction before the r-i-p series, 
> but not any more after.

Umm... which rebase -i -p series do you mean?  "noop" was introduced 
pretty recently if my Alzheimered brain does not fool me.

Ciao,
Dscho

Re: rebase -p confusion in 1.6.1

[Sitaram Chamarty]

On 2009-01-15, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

> The intention was this:
>
> 	$ git rebase -p master
>
> would need to rewrite _all_ commits that are in "master..".  All of them, 
> including the merge commits.

I went hog wild with all sorts of test cases and my head is
spinning, but -- even when things happen more predictably,
I'm unable to make "rebase -p" carry an evil merge over.
The "evil" part stays behind.

I'm not sure if that is intentional or not, or (more likely)
my brain has become addled and I missed something somewhere.

Regards,

Sitaram

Re: rebase -p confusion in 1.6.1

[Johannes Schindelin]

Hi,



if you would like me to respond to your questions in the future, it is 
mandatory to keep me in the Cc: list.



On Thu, 15 Jan 2009, Sitaram Chamarty wrote:

> On 2009-01-15, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> 
> > The intention was this:
> >
> > 	$ git rebase -p master
> >
> > would need to rewrite _all_ commits that are in "master..".  All of them, 
> > including the merge commits.
> 
> I went hog wild with all sorts of test cases and my head is
> spinning, but -- even when things happen more predictably,
> I'm unable to make "rebase -p" carry an evil merge over.
> The "evil" part stays behind.
> 
> I'm not sure if that is intentional or not, or (more likely)
> my brain has become addled and I missed something somewhere.

Yes, this is intentional.

	Instead of ignoring merges, try to recreate them.

That means it tries to recreate them.  Not that it is successful.  And not 
even that it realizes when it failed.

Hth,
Dscho

Re: rebase -p confusion in 1.6.1

[Sitaram Chamarty]

On 2009-01-15, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

> if you would like me to respond to your questions in the future, it is 
> mandatory to keep me in the Cc: list.

OK.  [Is that the list convention too?]

> On Thu, 15 Jan 2009, Sitaram Chamarty wrote:

>> I'm unable to make "rebase -p" carry an evil merge over.
>> The "evil" part stays behind.
>> 
>> I'm not sure if that is intentional or not, or (more likely)
>> my brain has become addled and I missed something somewhere.
>
> Yes, this is intentional.
>
> 	Instead of ignoring merges, try to recreate them.
>
> That means it tries to recreate them.  Not that it is successful.  And not 
> even that it realizes when it failed.

Is a conflicted merge that was resolved by making a change
that was in neither parent, an evil merge?

Regardless, I suspect rebase -p will not be able to carry
such a merge over.

But if it won't carry over the evil in an evil merge, what
other uses are there for "rebase -p" as opposed to rebase?
Seems to me that the DAG may be different but the tree you
end up with is the same then.

I'm sure someone has a blog post or a bookmark or an article
or something they wrote long ago about "rebase -i -p".
Anyone?

Re: rebase -p confusion in 1.6.1

[Michael J Gruber]

Johannes Schindelin venit, vidit, dixit 15.01.2009 17:04:
...
>>> The more I think about it, I think it's possible I broke it with the 
>>> introduction of the "noop".
>> It certainly worked after the noop introduction before the r-i-p series, 
>> but not any more after.
> 
> Umm... which rebase -i -p series do you mean?  "noop" was introduced 
> pretty recently if my Alzheimered brain does not fool me.

This one introduced noop:

commit ff74126c03a8dfd04e7533573a5c420f2a7112ac
Author: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Date:   Fri Oct 10 13:42:12 2008 +0200

    rebase -i: do not fail when there is no commit to cherry-pick

This is the bad one from bisect:

commit d80d6bc146232d81f1bb4bc58e5d89263fd228d4
Author: Stephen Haberman <stephen@exigencecorp.com>
Date:   Wed Oct 15 02:44:39 2008 -0500

    rebase-i-p: do not include non-first-parent commits touching UPSTREAM

It's the last one in a longer series. And that series is after the noop
introduction.

Michael

Re: rebase -p confusion in 1.6.1

[Johannes Schindelin]

Hi,

On Thu, 15 Jan 2009, Michael J Gruber wrote:

> Johannes Schindelin venit, vidit, dixit 15.01.2009 17:04:
> ...
> >>> The more I think about it, I think it's possible I broke it with the 
> >>> introduction of the "noop".
> >> It certainly worked after the noop introduction before the r-i-p series, 
> >> but not any more after.
> > 
> > Umm... which rebase -i -p series do you mean?  "noop" was introduced 
> > pretty recently if my Alzheimered brain does not fool me.
> 
> This one introduced noop:
> 
> commit ff74126c03a8dfd04e7533573a5c420f2a7112ac
> Author: Johannes Schindelin <Johannes.Schindelin@gmx.de>
> Date:   Fri Oct 10 13:42:12 2008 +0200
> 
>     rebase -i: do not fail when there is no commit to cherry-pick
> 
> This is the bad one from bisect:
> 
> commit d80d6bc146232d81f1bb4bc58e5d89263fd228d4
> Author: Stephen Haberman <stephen@exigencecorp.com>
> Date:   Wed Oct 15 02:44:39 2008 -0500
> 
>     rebase-i-p: do not include non-first-parent commits touching UPSTREAM
> 
> It's the last one in a longer series. And that series is after the noop
> introduction.

Ohhh....

Thanks,
Dscho

Re: [PATCH] do not drop commits before the merge base

[Johannes Schindelin]

Hi,

On Sat, 17 Jan 2009, Stephen Haberman wrote:

> [... no patch, despite the subject...]

You probably wanted to use -n --cover-letter...

Ciao,
Dscho

Re: [PATCH] rebase -p: seed first commit in case it's before the merge bases.

[Johannes Schindelin]

Hi,

On Sat, 17 Jan 2009, Stephen Haberman wrote:

> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index c8b0861..e800e07 100755
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -604,11 +604,18 @@ first and then run 'git rebase --continue' again."
>  				echo $ONTO > "$REWRITTEN"/$c ||
>  					die "Could not init rewritten commits"
>  			done
> +			# Along with the merge bases, look at the first commit's
> +			# parent (which may be before the merge base) and mark it
> +			# as rewritten to ONTO
> +			FIRST="$(git rev-list --reverse --first-parent $UPSTREAM..$HEAD | head -n 1)"
> +			for p in $(git rev-list --parents -1 $FIRST | cut -d' ' -f2)
> +			do
> +				echo $ONTO > "$REWRITTEN/$p"
> +			done

AFAICT this is wrong.  You have no guarantee whatsoever that the output of

	$ git rev-list --reverse --first-parent $UPSTREAM..$HEAD | head -n 1

has any parents at all.  Take for example a coolest-merge-ever, i.e. a 
merge of an independent project.

Instead, what you _actually_ are looking for are the boundary objects of 
$UPSTREAM..$HEAD, which would be easy to get at.

However, I have a strong feeling that just piling onto the current code 
will not fix the underlying issues.

Ciao,
Dscho

Re: [PATCH] rebase -p: seed first commit in case it's before the merge bases.

[Stephen Haberman]

> > +			# Along with the merge bases, look at the first commit's
> > +			# parent (which may be before the merge base) and mark it
> > +			# as rewritten to ONTO
> > +			FIRST="$(git rev-list --reverse --first-parent $UPSTREAM..$HEAD | head -n 1)"
> > +			for p in $(git rev-list --parents -1 $FIRST | cut -d' ' -f2)
> > +			do
> > +				echo $ONTO > "$REWRITTEN/$p"
> > +			done
> 
> AFAICT this is wrong.  You have no guarantee whatsoever that the output of
> 
> 	$ git rev-list --reverse --first-parent $UPSTREAM..$HEAD | head -n 1
> 
> has any parents at all.  Take for example a coolest-merge-ever, i.e. a 
> merge of an independent project.
> 
> Instead, what you _actually_ are looking for are the boundary objects
> of $UPSTREAM..$HEAD,

Agreed.

> which would be easy to get at.

That would be great, but I'm not seeing it, obviously. Suggestions
would be appreciated.

> However, I have a strong feeling that just piling onto the current
> code will not fix the underlying issues.

Also agreed.

So...not that it really matters, but did my patches go out to the git
list or not? It looks like both Johannes and I got them from the cc
entries.

I tried to use format-patch and the files looked great, cc's including
Michael, Stephan, and Sitaram. Then I ran send-email with the three
files as arguments and it stripped all the cc's but Johannes and
myself. Then I got all three delivered due to my cc entry, but I didn't
see any entries arrive from the list even though the cc-delivered
copies all had "To: git@vger.kernel.org" in them (and that is what I
had pasted into the send-email prompt). I guess I did something wrong
but it's frustrating to not know what it was.

- Stephen

Re: [PATCH] rebase -p: seed first commit in case it's before the merge bases.

[Johannes Schindelin]

Hi,

On Sun, 18 Jan 2009, Johannes Schindelin wrote:

> However, I have a strong feeling that just piling onto the current code 
> will not fix the underlying issues.

BTW just to clarify what I mean by "underlying issues": if you say "git 
rebase -i" in Sitaram's test case, you will see the two commits -- as 
expected.

However, if you add "-p", all of a sudden you will only see "noop".  IMO 
there is no excuse that the code can hide them at all.  If the commits are 
reachable from HEAD but not from $UPSTREAM, they have to be in the list.  
As simple as that.

Another thing that I find horribly wrong: there is a "touch 
$REWRITTEN/sha1".  There was a simple design in the beginning: the files 
in $REWRITTEN are actually a mapping from old SHA-1 (file name) to new 
SHA-1 (content).  This was broken, without any good explanation.

Ciao,
Dscho

Re: [PATCH] rebase -p: seed first commit in case it's before the merge bases.

[Stephen Haberman]

> > However, I have a strong feeling that just piling onto the current
> > code will not fix the underlying issues.
> 
> BTW just to clarify what I mean by "underlying issues": if you say
> "git rebase -i" in Sitaram's test case, you will see the two commits
> -- as expected.
> 
> However, if you add "-p", all of a sudden you will only see "noop".
> IMO there is no excuse that the code can hide them at all.  If the
> commits are reachable from HEAD but not from $UPSTREAM, they have to
> be in the list.  As simple as that.

Agreed--the rewritten-parent probing being rooted at the merge bases
was not good enough.

> Another thing that I find horribly wrong: there is a "touch
> $REWRITTEN/sha1".  There was a simple design in the beginning: the
> files in $REWRITTEN are actually a mapping from old SHA-1 (file name)
> to new SHA-1 (content).  This was broken, without any good
> explanation.

Perhaps it is not "good", but the explanation a blank REWRITTEN/sha1 is
used a marker during the probe phase that this commit will be rewritten.
So when looking at any of its children commits, they should be rewritten
if a REWRITTEN/parentSha1 exists. Then as the rewriting actually happens,
they get filled in with the new sha1. I cribbed this approach from
Stephan's sequencer rewrite of rebase-i-p.

If you want a different data structure, be it file based, or bash/list
based, or whatever, to track "this commit will eventually be rewritten
but we haven't gotten there yet" during the probe, then we could go back
to leaving REWRITTEN/sha1 alone until after the sha1 commit has been
rebased.

I'm open to suggestions.

Also, as you seem to realize, the current bug stems from not knowing how
to initialize the rewritten data structure. For Sitaram's case, the
first commit is behind any of the merge bases, so marking its parents
(if they exist) as rewritten to ONTO seems reasonable.

If there are no parents, as you point out, I added a "-o sha1 = FIRST"
that should also get the ball rolling. It's another hack, but does this
address your concern until a large refactoring happens?

-------------------------- git-rebase--interactive.sh --------------------------
index c8b0861..8740d9f 100755
@@ -604,11 +604,18 @@ first and then run 'git rebase --continue' again."
 				echo $ONTO > "$REWRITTEN"/$c ||
 					die "Could not init rewritten commits"
 			done
+			# Along with the merge bases, look at the first commit's
+			# parent (which may be before the merge base) and mark it
+			# as rewritten to ONTO
+			FIRST="$(git rev-list --reverse --first-parent $UPSTREAM..$HEAD | head -n 1)"
+			for p in $(git rev-list --parents -1 $FIRST | cut -d' ' -f2)
+			do
+				echo $ONTO > "$REWRITTEN/$p"
+			done
 			# No cherry-pick because our first pass is to determine
 			# parents to rewrite and skipping dropped commits would
 			# prematurely end our probe
 			MERGES_OPTION=
-			first_after_upstream="$(git rev-list --reverse --first-parent $UPSTREAM..$HEAD | head -n 1)"
 		else
 			MERGES_OPTION="--no-merges --cherry-pick"
 		fi
@@ -629,12 +636,12 @@ first and then run 'git rebase --continue' again."
 				preserve=t
 				for p in $(git rev-list --parents -1 $sha1 | cut -d' ' -f2-)
 				do
-					if test -f "$REWRITTEN"/$p -a \( $p != $UPSTREAM -o $sha1 = $first_after_upstream \)
+					if test -f "$REWRITTEN"/$p -a $p != $UPSTREAM
 					then
 						preserve=f
 					fi
 				done
-				if test f = "$preserve"
+				if test f = "$preserve" -o $sha1 = $FIRST
 				then
 					touch "$REWRITTEN"/$sha1
 					echo "pick $shortsha1 $rest" >> "$TODO"

(I'm adding the other 3 cc's back after my failed patch attempt
stripped them out--sorry, guys.)

- Stephen

Re: [PATCH] rebase -p: seed first commit in case it's before the merge bases.

[Stephen Haberman]

> Perhaps it is not "good", but the explanation a blank REWRITTEN/sha1 is
> used a marker during the probe phase that this commit will be rewritten.
> So when looking at any of its children commits, they should be rewritten
> if a REWRITTEN/parentSha1 exists.

Ugh, fixing several typos:

Perhaps it is not "good", but the explanation /is that/ a blank
REWRITTEN/sha1 is used /as/ a marker during the probe phase that this
commit will be rewritten. So when looking at any of its children
commits, /the children/ should be rewritten if a REWRITTEN/parentSha1
exists.

Sorry about that.

- Stephen

Re: rebase -p confusion in 1.6.1

[Stephan Beyer]

Michael J Gruber wrote:
> BTW: How does the sequencer based rebase do in this case,

This was the first thing I checked :-)
I had to rework the rebase -i -p code for sequencer a bit, but this
case was not something I had thought about (although it may not be
too seldom), so I'm glad it comes up now.

The result is that it eats the commits a3 and a4. (But at least it does
the same with and without --onto master.) :-)
I think it's not too hard to fix.

> and what's the general status?

I'm currently highly motivated to get it done soon and I hope that it
gets into pu or next before the end of January.

Depending on how productive I am over the weekend and depending on how
many further bugs (often hidden in such special cases) I find, it
could be sent to the list next week.

> If it's about to be integrated we can do without the
> present script...

I think it will take some time and some discussions on the list until
it will be integrated.  I remember, for example, Dscho, who has, since
it had first come up, always been opposed to the mark-reset /
mark-reset-merge scheme (in rebase -i -p, at least).
Other users said "Wow, this is much more flexible." ...
and this is perhaps only one thing that can lead to some bigger
discussion.

Regards,
  Stephan

-- 
Stephan Beyer <s-beyer@gmx.net>, PGP 0x6EDDD207FCC5040F

Re: rebase -p confusion in 1.6.1

[Johannes Schindelin]

Hi,

On Thu, 15 Jan 2009, Stephan Beyer wrote:

> Michael J Gruber wrote:
> > If it's about to be integrated we can do without the
> > present script...
> 
> I think it will take some time and some discussions on the list until it 
> will be integrated.  I remember, for example, Dscho, who has, since it 
> had first come up, always been opposed to the mark-reset / 
> mark-reset-merge scheme (in rebase -i -p, at least). Other users said 
> "Wow, this is much more flexible." ... and this is perhaps only one 
> thing that can lead to some bigger discussion.

Wow, much more flexible.  Except that you should not need this kind of 
flexibility.  If you need to do something complicated, it would be better 
to use "rebase -i -p" for the parts that do _not_ need to pick _other_ 
parents than are recorded in the commits.

And then you do an "edit" (or "pause" or whatever), and cherry-pick/merge 
_explicitely_ what you want.

Further, keep in mind that not only is that flexibility of dubitable value 
to the most users, it is also confusing, _and_ it adds code that is so 
rarely exercized that bugs can lurk in there for years... as you can 
experience right now.

So no, nothing has changed, I find that mark idea still horrible, 
horrible, horrible.

Ciao,
Dscho

Re: rebase -p confusion in 1.6.1

[Sitaram Chamarty]

On 2009-01-15, Michael J Gruber <git@drmicha.warpmail.net> wrote:
> Second, what result do you expect? If the merge is to be preserved then

I don't know.  I did this while trying to understand where
and how "-p" makes a difference.  If there's anything you
can point me to that explains rebase -p, especially from a
"here's where it comes useful" point of view, I'd appreciate
it.