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

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

[Christian Couder]

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.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 git-rebase--interactive.sh |   78 +++++++++++++++++++++++---------------------
 1 files changed, 41 insertions(+), 37 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 92e2523..3be49dd 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -40,6 +40,7 @@ MSG="$DOTEST"/message
 SQUASH_MSG="$DOTEST"/message-squash
 REWRITTEN="$DOTEST"/rewritten
 DROPPED="$DOTEST"/dropped
+SAVE_AUTHOR_INFO="$DOTEST"/save-author-info
 PRESERVE_MERGES=
 STRATEGY=
 ONTO=
@@ -117,30 +118,31 @@ mark_action_done () {
 }
 
 get_author_ident_from_commit () {
-	pick_author_script='
-	/^author /{
-		s/'\''/'\''\\'\'\''/g
-		h
-		s/^author \([^<]*\) <[^>]*> .*$/\1/
-		s/'\''/'\''\'\'\''/g
-		s/.*/GIT_AUTHOR_NAME='\''&'\''/p
-
-		g
-		s/^author [^<]* <\([^>]*\)> .*$/\1/
-		s/'\''/'\''\'\'\''/g
-		s/.*/GIT_AUTHOR_EMAIL='\''&'\''/p
-
-		g
-		s/^author [^<]* <[^>]*> \(.*\)$/\1/
-		s/'\''/'\''\'\'\''/g
-		s/.*/GIT_AUTHOR_DATE='\''&'\''/p
-
-		q
-	}
-	'
-	encoding=$(git config i18n.commitencoding || echo UTF-8)
-	git show -s --pretty=raw --encoding="$encoding" "$1" -- |
-	LANG=C LC_ALL=C sed -ne "$pick_author_script"
+	encoding=$(git config i18n.commitencoding || echo UTF-8) &&
+	author_ident_name=$(git show -s --pretty="format:%an" \
+		--encoding="$encoding" "$1" --) &&
+	author_ident_mail=$(git show -s --pretty="format:%ae" \
+		--encoding="$encoding" "$1" --) &&
+	author_ident_date=$(git show -s --pretty="format:%ai" \
+		--encoding="$encoding" "$1" --)
+}
+
+save_author_ident () {
+	git config --file="$SAVE_AUTHOR_INFO" rebase.author.name \
+		"$author_ident_name" &&
+	git config --file="$SAVE_AUTHOR_INFO" rebase.author.mail \
+		"$author_ident_mail" &&
+	git config --file="$SAVE_AUTHOR_INFO" rebase.author.date \
+		"$author_ident_date"
+}
+
+load_author_ident () {
+	GIT_AUTHOR_NAME=$(git config --file="$SAVE_AUTHOR_INFO" \
+		rebase.author.name) &&
+	GIT_AUTHOR_EMAIL=$(git config --file="$SAVE_AUTHOR_INFO" \
+		rebase.author.mail) &&
+	GIT_AUTHOR_DATE=$(git config --file="$SAVE_AUTHOR_INFO" \
+		rebase.author.date)
 }
 
 make_patch () {
@@ -158,8 +160,10 @@ make_patch () {
 	esac > "$DOTEST"/patch
 	test -f "$DOTEST"/message ||
 		git cat-file commit "$1" | sed "1,/^$/d" > "$DOTEST"/message
-	test -f "$DOTEST"/author-script ||
-		get_author_ident_from_commit "$1" > "$DOTEST"/author-script
+	test -f "$SAVE_AUTHOR_INFO" || {
+		get_author_ident_from_commit "$1"
+		save_author_ident
+	}
 }
 
 die_with_patch () {
@@ -294,8 +298,10 @@ pick_one_preserving_merges () {
 			test "a$1" = a-n && die "Refusing to squash a merge: $sha1"
 
 			# redo merge
-			author_script=$(get_author_ident_from_commit $sha1)
-			eval "$author_script"
+			get_author_ident_from_commit $sha1
+			GIT_AUTHOR_NAME="$author_ident_name"
+			GIT_AUTHOR_EMAIL="$author_ident_mail"
+			GIT_AUTHOR_DATE="$author_ident_date"
 			msg="$(git cat-file commit $sha1 | sed -e '1,/^$/d')"
 			# No point in merging the first parent, that's HEAD
 			new_parents=${new_parents# $first_parent}
@@ -353,8 +359,7 @@ peek_next_command () {
 }
 
 do_next () {
-	rm -f "$DOTEST"/message "$DOTEST"/author-script \
-		"$DOTEST"/amend || exit
+	rm -f "$DOTEST"/message "$DOTEST"/amend "$SAVE_AUTHOR_INFO" || exit
 	read command sha1 rest < "$TODO"
 	case "$command" in
 	'#'*|''|noop)
@@ -395,7 +400,7 @@ do_next () {
 		mark_action_done
 		make_squash_message $sha1 > "$MSG"
 		failed=f
-		author_script=$(get_author_ident_from_commit HEAD)
+		get_author_ident_from_commit HEAD
 		output git reset --soft HEAD^
 		pick_one -n $sha1 || failed=t
 		case "$(peek_next_command)" in
@@ -414,14 +419,13 @@ do_next () {
 			rm -f "$GIT_DIR"/MERGE_MSG || exit
 			;;
 		esac
-		echo "$author_script" > "$DOTEST"/author-script
+		save_author_ident
 		if test $failed = f
 		then
 			# This is like --amend, but with a different message
-			eval "$author_script"
-			GIT_AUTHOR_NAME="$GIT_AUTHOR_NAME" \
-			GIT_AUTHOR_EMAIL="$GIT_AUTHOR_EMAIL" \
-			GIT_AUTHOR_DATE="$GIT_AUTHOR_DATE" \
+			GIT_AUTHOR_NAME="$author_ident_name" \
+			GIT_AUTHOR_EMAIL="$author_ident_mail" \
+			GIT_AUTHOR_DATE="$author_ident_date" \
 			$USE_OUTPUT git commit --no-verify \
 				$MSG_OPT "$EDIT_OR_FILE" || failed=t
 		fi
@@ -536,7 +540,7 @@ do
 		then
 			: Nothing to commit -- skip this
 		else
-			. "$DOTEST"/author-script ||
+			load_author_ident ||
 				die "Cannot find the author identity"
 			amend=
 			if test -f "$DOTEST"/amend
-- 
1.6.3.GIT

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

[Junio C Hamano]

Christian Couder <chriscool@tuxfamily.org> 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.

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

[Daniel Barkalow]

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

> Christian Couder <chriscool@tuxfamily.org> 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?

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*

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

[Junio C Hamano]

Daniel Barkalow <barkalow@iabervon.org> writes:

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

Ahh, Ok.

Using a _known_ and defined format, instead of ad-hoc scriptlet, is an
improvement.

I still wonder if we can avoid using three separate "git show" and "git
config" invocations, though.  But a half of that inefficiency will go away
when this is migrated to C, as a single git_config() will grab all three, 
although the writing side is still very inefficient X-<.

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

[Jakub Narebski]

On Sun, 21 June 2009, Junio C Hamano wrote:
> Daniel Barkalow <barkalow@iabervon.org> writes:
> 
>>> 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.
> 
> Ahh, Ok.
> 
> Using a _known_ and defined format, instead of ad-hoc scriptlet, is an
> improvement.
> 
> I still wonder if we can avoid using three separate "git show" and "git
> config" invocations, though.  But a half of that inefficiency will go away
> when this is migrated to C, as a single git_config() will grab all three, 
> although the writing side is still very inefficient X-<.

I think we can on the reading side: just use "git config --list", or
perhaps "git config --get-regexp <sth>" (where <sth> can be ".*") which
conveniently has SPC as separator, and feed it to appropriate 3 x 'read'.

On the writing side we can simply write in the config file format, we
don't need to use git-config for that.  Although I wonder if there won't
be trouble with shell escaping and quoting rules (eval / sed, which
I guess does shell quoting / shell unquoting).

On getting the information side we can use git-show with custom format
or git-cat-file fed to while-read-case construct.


This way from 3 x 3 = 9 git commands git-rebase--interactive.sh would
use only 2.

-- 
Jakub Narebski
Poland

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

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> On Sun, 21 June 2009, Junio C Hamano wrote:
>> Daniel Barkalow <barkalow@iabervon.org> writes:
>> 
>>>> 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.
>> 
>> Ahh, Ok.
>> 
>> Using a _known_ and defined format, instead of ad-hoc scriptlet, is an
>> improvement.
>> 
>> I still wonder if we can avoid using three separate "git show" and "git
>> config" invocations, though.  But a half of that inefficiency will go away
>> when this is migrated to C, as a single git_config() will grab all three, 
>> although the writing side is still very inefficient X-<.
>
> I think we can on the reading side: just use "git config --list", or
> perhaps "git config --get-regexp <sth>" (where <sth> can be ".*") which
> conveniently has SPC as separator, and feed it to appropriate 3 x 'read'.
>
> On the writing side we can simply write in the config file format, we
> don't need to use git-config for that.  Although I wonder if there won't
> be trouble with shell escaping and quoting rules (eval / sed, which
> I guess does shell quoting / shell unquoting).
>
> On getting the information side we can use git-show with custom format
> or git-cat-file fed to while-read-case construct.
>
> This way from 3 x 3 = 9 git commands git-rebase--interactive.sh would
> use only 2.

That's not what I meant.

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

[Christian Couder]

On Sunday 21 June 2009, Junio C Hamano wrote:
> Daniel Barkalow <barkalow@iabervon.org> writes:
> >> 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.
>
> Ahh, Ok.
>
> Using a _known_ and defined format, instead of ad-hoc scriptlet, is an
> improvement.
>
> I still wonder if we can avoid using three separate "git show" and "git
> config" invocations, though.  But a half of that inefficiency will go
> away when this is migrated to C, as a single git_config() will grab all
> three, although the writing side is still very inefficient X-<.

It may be possible to write a ref to the commit we need information from. 
And then when "git rebase --continue" is called, we would read this ref and 
then get information from the referenced commit?

Best regards,
Christian.

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

[Junio C Hamano]

Christian Couder <chriscool@tuxfamily.org> writes:

> It may be possible to write a ref to the commit we need information from. 
> And then when "git rebase --continue" is called, we would read this ref and 
> then get information from the referenced commit?

You mean replacing author-script temporary file with the name of the
commit object we are "pick"ing?

That surely sounds like a more direct approach.  I wonder why the script
didn't do that in the first place (I suspect it was just because it was
easier to implement by borrowing the logic to parse authorship information
from git-revert-script).