Re: [GSoC] What is status of Git's Google Summer of Code 2008 projects?

Stephan Beyer [off-list ref] writes:

Imho the prototype patchset needs some more review from others.

Yes, very much.  Not just from others and not just from me.

(Well, I think the
   http://thread.gmane.org/gmane.comp.version-control.git/86985
thread died in the last days, but I hope some responses will come and
also that Junio's patch for cherry-picking root commits will be
included. I try to be patient...) ;-)

Please don't be patient but actively re-review what you sent out.

I _really_ wanted to merge the basic bits and rewrite of "am" at least to
pu tonight, but I had to drop them after noticing that it does not seem to
handle --rebasing at all (it parses to set $rebasing but after that where
does that bit go?  bash completion wants to see rebasing or applying
markers in .dotest), which made it a non-starter especially I'll be
cooking the other ORIG_HEAD in 'next' as well.

About the "rewrite rebase to use sequencer" bits, because we've dropped
the older rebase-i change, I do not want your series to depend on it.

Hi,

Junio C Hamano [off-list ref] wrote:

Stephan Beyer [off-list ref] writes:

quoted

(Well, I think the
   http://thread.gmane.org/gmane.comp.version-control.git/86985
thread died in the last days, but I hope some responses will come and
also that Junio's patch for cherry-picking root commits will be
included. I try to be patient...) ;-)

Please don't be patient but actively re-review what you sent out.

I _really_ wanted to merge the basic bits and rewrite of "am" at least to
pu tonight, but I had to drop them after noticing that it does not seem to
handle --rebasing at all (it parses to set $rebasing but after that where
does that bit go?

Yes, you are right that am --rebasing is a no-op.
That option was a little mystery to me, because it seemed to do nothing
special, but I'll check again (bash-completion etc) and do appropriate
changes.

About the "rewrite rebase to use sequencer" bits, because we've dropped
the older rebase-i change, I do not want your series to depend on it.

Yes, I made the mistake that I started developing on "next" and I did
not expect that js/rebase-i-sequencer could be dropped. (I somehow
expected the opposite, that it would be migrated to master some day.)

Then I wanted to make it work on master, and thought this is simply
done by removing the "additional features" (-f and -p option using extended
todo list), but then I noticed that master also has a -p which works
entirely different and I wanted to see that stuff on the list before
July 1st and I had no idea what the status of js/rebase-i-sequencer is.
So I just tried and sent that last patch to the list in that way.

But now I have a clear statement.

So I'm going to send a new patchset (also with an EXAMPLES section in 
the docs) to the list in a few days.

Big thanks for the feedback,
  Stephan

-- 
Stephan Beyer [off-list ref], PGP 0x6EDDD207FCC5040F

On Tue, 8 July 2008, Stephan Beyer wrote:

Junio C Hamano [off-list ref] wrote:

quoted

I _really_ wanted to merge the basic bits and rewrite of "am" at least to
pu tonight, but I had to drop them after noticing that it does not seem to
handle --rebasing at all (it parses to set $rebasing but after that where
does that bit go?

Yes, you are right that am --rebasing is a no-op.
That option was a little mystery to me, because it seemed to do nothing
special, but I'll check again (bash-completion etc) and do appropriate
changes.

Undocumented option '--rebasing' to git-am is internal option changing
git-am behavior to be better used by git-rebase, namely it does not
change commit message even if it doesn't follow git commit message
convention, for example if it begins not with single line summary
of commit, separated by empty line, but by multi-line paragraph.
See also t/t3405-rebase-malformed.sh

Although I am not sure if when rebase is rewritten using git-sequencer
implementing "git am --rebasing" would be truly needed.  On the other
hand side it would be nice to have some _documented_ option which would
allow to git-am mail messages with commits not following git commit
messages convention...

-- 
Jakub Narebski
Poland

Hi,

Jakub Narebski wrote:

quoted

Yes, you are right that am --rebasing is a no-op.
That option was a little mystery to me, because it seemed to do nothing
special, but I'll check again (bash-completion etc) and do appropriate
changes.

Undocumented option '--rebasing' to git-am is internal option changing
git-am behavior to be better used by git-rebase, namely it does not
change commit message even if it doesn't follow git commit message
convention,

Ah yes, I've seen it now.
It is taking the commit message from the commit in the "From <commit> .*"
line, does *not* change it in any way and then applies the changes using
threeway merge.

Keeping that in mind what about dealing with --rebasing like that:
if --rebasing is given, git am simply generates
	pick <commit>
lines, instead of
	patch -3 -k <msg>
as it is now (and this is not enough, as it seems).

Does someone have strong objections against that?

Speed could be one point in the case that git-apply just works without
needing threeway-fallback, but in the case of the fallback this will be
slower than pick, I think. So I'd not value that too high, but perhaps
there are opinions against my view.
Perhaps I am missing another point, too?


The alternative for doing "pick" is teaching git-sequencer's "patch"
insn an option that emulates the --rebasing behavior.

For me this feels somehow unclean. But perhaps there are good reasons.

for example if it begins not with single line summary
of commit, separated by empty line, but by multi-line paragraph.
See also t/t3405-rebase-malformed.sh

Well, I have a test script that runs
 for i in t0023* t3350* t340* t3901* t4014* t4150* t5520* t7402*
and I run that script before I do a commit and after I rebased.
And I ran the whole test suite before I posted the patchset to the list.
What I want to say is: t3405 did not fail with my --rebasing no-op.

That's perhaps one reason why I forgot about implementing --rebasing
correctly.

Although I am not sure if when rebase is rewritten using git-sequencer
implementing "git am --rebasing" would be truly needed.

I didn't want to touch that behavior for several reasons.

Of course, somehow I think that rebase and rebase-i should be merged,
calling sequencer directly, with the main difference that -i will
invoke an editor to allow editing of the TODO file.
But nobody is hurt, if I put such a change far far away.


Regards,
  Stephan

-- 
Stephan Beyer [off-list ref], PGP 0x6EDDD207FCC5040F

Stephan Beyer wrote:

Jakub Narebski wrote:

quoted

quoted

Yes, you are right that am --rebasing is a no-op.
That option was a little mystery to me, because it seemed to do nothing
special, but I'll check again (bash-completion etc) and do appropriate
changes.

Undocumented option '--rebasing' to git-am is internal option changing
git-am behavior to be better used by git-rebase, namely it does not
change commit message even if it doesn't follow git commit message
convention,

Ah yes, I've seen it now.

It is taking the commit message from the commit in the "From <commit> .*"
line, does *not* change it in any way and then applies the changes using
threeway merge.

Not exactly.  "git am --rebasing" still tries to first just *apply*
the patch, then (I think) it falls back on blob-id based 3way merge.
 

Keeping that in mind what about dealing with --rebasing like that:
if --rebasing is given, git am simply generates
	pick <commit>
lines, instead of
	patch -3 -k <msg>
as it is now (and this is not enough, as it seems).

It is not.

Nevertheless it would be I think better for ordinary patch based rebase
to fall back not on git-am 3way merge, but on cherry-pick based merge
(i.e. on pick).

The alternative for doing "pick" is teaching git-sequencer's "patch"
insn an option that emulates the --rebasing behavior.

For me this feels somehow unclean. But perhaps there are good reasons.

Why unclean?

But I agree that it would be nice to simplify '--rebasing' logic, for
example using patch or 2way merge to generate tree, and commit message
taken directly from commit, not via 'format-patch | am' pipeline.
 

Of course, somehow I think that rebase and rebase-i should be merged,
calling sequencer directly, with the main difference that -i will
invoke an editor to allow editing of the TODO file.
But nobody is hurt, if I put such a change far far away.

rebase-m and rebase-i can be merged; ordinary rebase uses other
mechanism: git-am pipeline, and not cherry-picking.

-- 
Jakub Narebski
Poland

Hi,

Jakub Narebski wrote:

quoted

It is taking the commit message from the commit in the "From <commit> .*"
line, does *not* change it in any way and then applies the changes using
threeway merge.

Not exactly.  "git am --rebasing" still tries to first just *apply*
the patch, then (I think) it falls back on blob-id based 3way merge.

That's of course totaly right and what I've meant, but unfortunately not
what I've written ;-)

quoted

Keeping that in mind what about dealing with --rebasing like that:
if --rebasing is given, git am simply generates
	pick <commit>
lines, instead of
	patch -3 -k <msg>
as it is now (and this is not enough, as it seems).

It is not.

Nevertheless it would be I think better for ordinary patch based rebase
to fall back not on git-am 3way merge, but on cherry-pick based merge
(i.e. on pick).

Hmm, if I get you right you _partly_ agree with me in choosing "pick" for
am --rebasing... But cherry-pick should only be chosen if a simple git-apply
failed first. Right?

I just got another idea which could easily be done and perhaps is the
right thing :)
Generating
	patch -C <commit> -3 <file>

This takes authorship and message from <commit> and does the usual
threeway-fallback behavior.

What do you think?

But I agree that it would be nice to simplify '--rebasing' logic, for
example using patch or 2way merge to generate tree, and commit message
taken directly from commit, not via 'format-patch | am' pipeline.

That's right, but that would require me to hack around in git-rebase
which I tried to avoid for now. :)

Regards,
  Stephan

-- 
Stephan Beyer [off-list ref], PGP 0x6EDDD207FCC5040F

Hi!

On Wed, 8 July 2008, Stephan Beyer wrote:

Jakub Narebski wrote:

quoted

Stephan Beyer wrote:

quoted

It is taking the commit message from the commit in the "From <commit> .*"
line, does *not* change it in any way and then applies the changes using
threeway merge.

Not exactly.  "git am --rebasing" still tries to first just *apply*
the patch, then (I think) it falls back on blob-id based 3way merge.

That's of course totaly right and what I've meant, but unfortunately not
what I've written ;-)

quoted

quoted

Keeping that in mind what about dealing with --rebasing like that:
if --rebasing is given, git am simply generates
	pick <commit>
lines, instead of
	patch -3 -k <msg>
as it is now (and this is not enough, as it seems).

It is not.

Nevertheless it would be I think better for ordinary patch based rebase
to fall back not on git-am 3way merge, but on cherry-pick based merge
(i.e. on pick).

Hmm, if I get you right you _partly_ agree with me in choosing "pick" for
am --rebasing... But cherry-pick should only be chosen if a simple git-apply
failed first. Right?

Right.

I just got another idea which could easily be done and perhaps is the
right thing :)
Generating
	patch -C <commit> -3 <file>

This takes authorship and message from <commit> and does the usual
threeway-fallback behavior.

What do you think?

Very good idea (I have proposed something similar either here on in
another thread).  It would avoid some unnecessary "marshalling" and
"unmarshalling" which is needed to transfer commit message [unchanged]
through git-format-patch -> git-am pipeline, namely putting first
paragraph into subject line, generating then parsing RFC-2822 date,
using quoted printable encoding for first paragraph / subject header
(I think).

It would be still better to fallback to _pick_, not "git am --3way",
as the latter IIRC use _shortened_ _blob_ identifiers for pre- and
post-image to find common ancestor (merge base) for 3way merge.
Which is not necessary as we can find merge base and base commits
easier.

-- 
Jakub Narebski
Poland