Re: [PATCH 2/2] rebase -i: use some kind of config file to save author information

Johannes Schindelin [off-list ref] writes:

On Sat, 20 Jun 2009, Christian Couder wrote:

quoted

This is better than saving in a shell script, because it will make
it much easier to port "rebase -i" to C. This also removes some sed
regexps and some "eval"s.

It will not make it easier to port "rebase -i" to C, as this is an 
internal file.  The user is not supposed to touch it at all.  Only "rebase 
-i".  So it would be very easy to just use a different on-disk format when 
turning "rebase -i" into a builtin.

"This is an internal file" is just a declaration you are making, and the
file is observable by anybody after "rebase -i" relinquishes the control
to let the user sort out the mess.  The users do not have any obligation
to honor your declaration, and strictly speaking it is a regression to
change the file format.

For example, when I realize I misspelt somebody's name (perhaps the
mailpath between the sender and me mishandled the encoding headers), I
could edit .git/rebase-merge/author-script and say "git rebase --continue"
to let auto-amend to kick in, which would use the fixed author name from
the file.

	Side note.  The current "rebase --continue" behaviour is somewhat
	inconsistent; if "edit" does not do anything to the tree, nor the
	user runs "git commit --amend', the commit is untouched, but if
	the user updates the index and says --continue without amending,
	the authorship is not taken from the auto-amended commit but is
	taken from the author-script file.  Perhaps something along the
	line of untested patch attached at the end would remedy this a
	bit? 

Having said that, if we were to change the way rebase-i leaves its state
behind so that it can pick up from where it left off, I prefer Christian's
later suggestion to leave the object name of the commit that is being
rebased in the file.  Sure, it makes it harder to lie about the authorship,
but my previous example was purely "I _could_ do this" and not "I rely on
being able to do this".

But I have this nagging feeling that we may be able to get rid of even the
"current commit".

-- >8 --

rebase -i: AUTHOR_{NAME,EMAIL,DATE} are already available in HEAD; use it.

This only changes the codepath of "rebase -i --continue" that auto-amends
the HEAD commit with the change user made but forgot to "commit --amend".

 git-rebase--interactive.sh |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index f96d887..b8608be 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh

@@ -522,8 +522,7 @@ first and then run 'git rebase --continue' again."
 				git reset --soft HEAD^ ||
 				die "Cannot rewind the HEAD"
 			fi
-			export GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL GIT_AUTHOR_DATE &&
-			git commit --no-verify -F "$DOTEST"/message -e || {
+			git commit --no-verify -c HEAD || {
 				test -n "$amend" && git reset --soft $amend
 				die "Could not commit staged changes."
 			}

On Monday 22 June 2009, Junio C Hamano wrote:

Johannes Schindelin [off-list ref] writes:

quoted

On Sat, 20 Jun 2009, Christian Couder wrote:

quoted

This is better than saving in a shell script, because it will make
it much easier to port "rebase -i" to C. This also removes some sed
regexps and some "eval"s.

It will not make it easier to port "rebase -i" to C, as this is an
internal file.  The user is not supposed to touch it at all.  Only
"rebase -i".  So it would be very easy to just use a different on-disk
format when turning "rebase -i" into a builtin.

I'd rather port "rebase -i" to C step by step like I started 
porting "bisect". So changing the format to something easier to deal with 
in C makes it easier to port.

"This is an internal file" is just a declaration you are making, and the
file is observable by anybody after "rebase -i" relinquishes the control
to let the user sort out the mess.  The users do not have any obligation
to honor your declaration, and strictly speaking it is a regression to
change the file format.

For example, when I realize I misspelt somebody's name (perhaps the
mailpath between the sender and me mishandled the encoding headers), I
could edit .git/rebase-merge/author-script and say "git rebase
--continue" to let auto-amend to kick in, which would use the fixed
author name from the file.

	Side note.  The current "rebase --continue" behaviour is somewhat
	inconsistent; if "edit" does not do anything to the tree, nor the
	user runs "git commit --amend', the commit is untouched, but if
	the user updates the index and says --continue without amending,
	the authorship is not taken from the auto-amended commit but is
	taken from the author-script file.  Perhaps something along the
	line of untested patch attached at the end would remedy this a
	bit?

Interesting. I will try it.

Having said that, if we were to change the way rebase-i leaves its state
behind so that it can pick up from where it left off, I prefer
Christian's later suggestion to leave the object name of the commit that
is being rebased in the file.  Sure, it makes it harder to lie about the
authorship, but my previous example was purely "I _could_ do this" and
not "I rely on being able to do this".

I just sent a v3 where the commit sha1 is saved in the file, so you can 
choose between v2 and v3 what behavior you prefer.

But I have this nagging feeling that we may be able to get rid of even
the "current commit".

I will have a look at that.

Thanks,
Christian.

Hi,

On Sun, 21 Jun 2009, Junio C Hamano wrote:

Johannes Schindelin [off-list ref] writes:

quoted

On Sat, 20 Jun 2009, Christian Couder wrote:

quoted

This is better than saving in a shell script, because it will make
it much easier to port "rebase -i" to C. This also removes some sed
regexps and some "eval"s.

It will not make it easier to port "rebase -i" to C, as this is an 
internal file.  The user is not supposed to touch it at all.  Only "rebase 
-i".  So it would be very easy to just use a different on-disk format when 
turning "rebase -i" into a builtin.

"This is an internal file" is just a declaration you are making, and the
file is observable by anybody after "rebase -i" relinquishes the control
to let the user sort out the mess.

It is an observation I am making.  Sure, the file is observable by the 
user.  But it is hidden deep inside .git/ and users who change things 
inside .git/ (with the exception of config) are asking for trouble.

I really do not see the point of changing the file format _before_ turning 
rebase -i into C.

Oh, and I do not see the point of turning rebase -i into C before finally 
polishing sequencer so it can go into git.git's master.

Ciao,
Dscho

On Monday 22 June 2009, Junio C Hamano wrote:

This patch would use the message from HEAD instead of "$DOTEST"/message, but 
it looks like we are changing "$DOTEST"/message sometimes with 
the "make_squash_message" function.

Best regards,
Christian.

On Monday 22 June 2009, Johannes Schindelin wrote:

Hi,

On Sun, 21 Jun 2009, Junio C Hamano wrote:

quoted

Johannes Schindelin [off-list ref] writes:

quoted

On Sat, 20 Jun 2009, Christian Couder wrote:

quoted

This is better than saving in a shell script, because it will make
it much easier to port "rebase -i" to C. This also removes some sed
regexps and some "eval"s.

It will not make it easier to port "rebase -i" to C, as this is an
internal file.  The user is not supposed to touch it at all.  Only
"rebase -i".  So it would be very easy to just use a different
on-disk format when turning "rebase -i" into a builtin.

"This is an internal file" is just a declaration you are making, and
the file is observable by anybody after "rebase -i" relinquishes the
control to let the user sort out the mess.

It is an observation I am making.  Sure, the file is observable by the
user.  But it is hidden deep inside .git/ and users who change things
inside .git/ (with the exception of config) are asking for trouble.

I really do not see the point of changing the file format _before_
turning rebase -i into C.

Oh, and I do not see the point of turning rebase -i into C before finally
polishing sequencer so it can go into git.git's master.

The problem with this is that it will take a lot of time to implement the 
features that have been added to rebase -i since the sequencer stalled, 
then to polish it, and to get it reviewed and so on, and during that time 
other features or changes may be implemented by other people.

So I prefer to use code from the current sequencer (at 
http://repo.or.cz/w/git/sbeyer.git) to start porting step by step rebase -i 
to C.

Regards,
Christian.

Hi,

On Tue, 23 Jun 2009, Christian Couder wrote:

On Monday 22 June 2009, Johannes Schindelin wrote:

quoted

On Sun, 21 Jun 2009, Junio C Hamano wrote:

quoted

Johannes Schindelin [off-list ref] writes:

quoted

On Sat, 20 Jun 2009, Christian Couder wrote:

quoted

This is better than saving in a shell script, because it will 
make it much easier to port "rebase -i" to C. This also removes 
some sed regexps and some "eval"s.

It will not make it easier to port "rebase -i" to C, as this is an 
internal file.  The user is not supposed to touch it at all.  
Only "rebase -i".  So it would be very easy to just use a 
different on-disk format when turning "rebase -i" into a builtin.

"This is an internal file" is just a declaration you are making, and 
the file is observable by anybody after "rebase -i" relinquishes the 
control to let the user sort out the mess.

It is an observation I am making.  Sure, the file is observable by the 
user.  But it is hidden deep inside .git/ and users who change things 
inside .git/ (with the exception of config) are asking for trouble.

I really do not see the point of changing the file format _before_ 
turning rebase -i into C.

Oh, and I do not see the point of turning rebase -i into C before 
finally polishing sequencer so it can go into git.git's master.

The problem with this is that it will take a lot of time to implement 
the features that have been added to rebase -i since the sequencer 
stalled, then to polish it, and to get it reviewed and so on, and during 
that time other features or changes may be implemented by other people.

So I prefer to use code from the current sequencer (at 
http://repo.or.cz/w/git/sbeyer.git) to start porting step by step rebase 
-i to C.

I think that the best way to go forward would be to have something like 
fetch--tool, i.e. a builtin helper that successively takes more and more 
functionality into C.

IMHO a first sensible step would be to implement the commands ("pick", 
"squash", "edit") in such a helper, and call them from do_next().

That should take care of the most difficult part, getting the transition 
started.

But I had the impression that the sequencer started out almost like this, 
but then it also wanted to implement the do_next() and everything.

Ciao,
Dscho

Hi,

On Tuesday 23 June 2009, Johannes Schindelin wrote:

Hi,

On Tue, 23 Jun 2009, Christian Couder wrote:

quoted

The problem with this is that it will take a lot of time to implement
the features that have been added to rebase -i since the sequencer
stalled, then to polish it, and to get it reviewed and so on, and
during that time other features or changes may be implemented by other
people.

So I prefer to use code from the current sequencer (at
http://repo.or.cz/w/git/sbeyer.git) to start porting step by step
rebase -i to C.

I think that the best way to go forward would be to have something like
fetch--tool, i.e. a builtin helper that successively takes more and more
functionality into C.

Yeah, I started working on something like that.

IMHO a first sensible step would be to implement the commands ("pick",
"squash", "edit") in such a helper, and call them from do_next().

That should take care of the most difficult part, getting the transition
started.

I agree that it looks like a good way forward.

But I had the impression that the sequencer started out almost like this,
but then it also wanted to implement the do_next() and everything.

Yeah, I think at some steps we got something close to that.

Thanks,
Christian.