Re: [PATCH v2 2/2] rebase -i: use config file format to save author information

Christian Couder [off-list ref] writes:

This is better than saving in a shell script, because it will make
it much easier to port "rebase -i" to C.

Hmph.

We used to parse a commit object in one go into variables, and I would
have imagined that a rewrite in C will read a commit object to get the
author information in variables in-core, without having to write any
temporary file.

But with your patch, it starts to use a temporary file, and forces the C
rewrite to do the same.  It closes the door for a more efficient rewrite.

Why is this a good change?

This also removes some sed regexps and some "eval"s.

That is a correct description of what the patch does.

But the use of sed and eval is an implementation detail.  As long as they
are used correctly, I do not think it is a grave offense to use them, and
removal is not an improvement.

A possible argument (or excuse) you _could_ make to justify this change, I
think, is that such a temporary file with known name will allow a hook
script that is run during a rebase to learn the authorship information of
the commit being rebased.  This has been hidden inside variables without
getting exported, and this patch defines a way for the scripts to get at
it if they wanted to.

I didn't check if there is such a codepath that involves hooks, if there
are plausible scenarios that they would want to learn that information,
nor if the information is hard to get otherwise, though.

On Sat, 20 Jun 2009, Junio C Hamano wrote:

Christian Couder [off-list ref] writes:

quoted

This is better than saving in a shell script, because it will make
it much easier to port "rebase -i" to C.

Hmph.

We used to parse a commit object in one go into variables, and I would
have imagined that a rewrite in C will read a commit object to get the
author information in variables in-core, without having to write any
temporary file.

But with your patch, it starts to use a temporary file, and forces the C
rewrite to do the same.  It closes the door for a more efficient rewrite.

Why is this a good change?

It was always using a temporary file; it just used to use a temporary file 
that was a shell script fragment and needed to be read with "eval". It 
can't be done entirely in core because it may be determined before a 
conflict and only used when run with --continue after the user resolves 
the conflict.

	-Daniel
*This .sig left intentionally blank*