threads / patch / 19882

v2, 2 partsrebase -i: use config file format to save author information

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

## tl;dr

8 messages between Jun 21, 2009 and Jun 21, 2009. Diffs are folded; open one to read it.

replies: 7people: 4as markdown or json

Christian Couder· Jun 21, 2009, 05:08 UTC · lore

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(-)
Show changes to git-rebase--interactive.sh +41 −37
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
Junio C Hamano· Jun 21, 2009, 06:41 UTC · re: Christian Couder · lore

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

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.

Daniel Barkalow· Jun 21, 2009, 06:51 UTC · re: Junio C Hamano · lore

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

On Sat, 20 Jun 2009, Junio C Hamano wrote:
Show 16 quoted lines
> 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*
Junio C Hamano· Jun 21, 2009, 06:56 UTC · re: Daniel Barkalow · lore

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

Daniel Barkalow <barkalow@iabervon.org> writes:
Show 7 quoted lines
>> 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-<.

Jakub Narebski· Jun 21, 2009, 08:01 UTC · re: Junio C Hamano · lore

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

On Sun, 21 June 2009, Junio C Hamano wrote:
Show 19 quoted lines
> 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
Junio C Hamano· Jun 21, 2009, 08:10 UTC · re: Jakub Narebski · lore

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

Jakub Narebski <jnareb@gmail.com> writes:
Show 35 quoted lines
> 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.
Christian Couder· Jun 21, 2009, 09:53 UTC · re: Junio C Hamano · lore

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

On Sunday 21 June 2009, Junio C Hamano wrote:
Show 18 quoted lines
> 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.

Junio C Hamano· Jun 21, 2009, 10:19 UTC · re: Christian Couder · lore

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

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

← back to recent threads