{"thread":{"id":"19882","subject":"[PATCH v2 2/2] rebase -i: use config file format to save author information","startedAt":"2009-06-21T05:08:45Z","lastAt":"2009-06-21T10:19:36Z","messageCount":8,"participants":["Christian Couder","Junio C Hamano","Daniel Barkalow","Jakub Narebski"],"isPatch":true,"patchVersion":2,"patchTotal":2},"messages":[{"id":"116690","messageId":"20090621050846.3554.31660.chriscool@tuxfamily.org","threadId":"19882","inReplyTo":null,"subject":"[PATCH v2 2/2] rebase -i: use config file format to save author information","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2009-06-21T05:08:45Z","receivedAt":"2009-06-21T05:08:45Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"This is better than saving in a shell script, because it will make\nit much easier to port \"rebase -i\" to C. This also removes some sed\nregexps and some \"eval\"s.\n\nSigned-off-by: Christian Couder <chriscool@tuxfamily.org>\n---\n git-rebase--interactive.sh |   78 +++++++++++++++++++++++---------------------\n 1 files changed, 41 insertions(+), 37 deletions(-)\n\ndiff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\nindex 92e2523..3be49dd 100755\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -40,6 +40,7 @@ MSG=\"$DOTEST\"/message\n SQUASH_MSG=\"$DOTEST\"/message-squash\n REWRITTEN=\"$DOTEST\"/rewritten\n DROPPED=\"$DOTEST\"/dropped\n+SAVE_AUTHOR_INFO=\"$DOTEST\"/save-author-info\n PRESERVE_MERGES=\n STRATEGY=\n ONTO=\n@@ -117,30 +118,31 @@ mark_action_done () {\n }\n \n get_author_ident_from_commit () {\n-\tpick_author_script='\n-\t/^author /{\n-\t\ts/'\\''/'\\''\\\\'\\'\\''/g\n-\t\th\n-\t\ts/^author \\([^<]*\\) <[^>]*> .*$/\\1/\n-\t\ts/'\\''/'\\''\\'\\'\\''/g\n-\t\ts/.*/GIT_AUTHOR_NAME='\\''&'\\''/p\n-\n-\t\tg\n-\t\ts/^author [^<]* <\\([^>]*\\)> .*$/\\1/\n-\t\ts/'\\''/'\\''\\'\\'\\''/g\n-\t\ts/.*/GIT_AUTHOR_EMAIL='\\''&'\\''/p\n-\n-\t\tg\n-\t\ts/^author [^<]* <[^>]*> \\(.*\\)$/\\1/\n-\t\ts/'\\''/'\\''\\'\\'\\''/g\n-\t\ts/.*/GIT_AUTHOR_DATE='\\''&'\\''/p\n-\n-\t\tq\n-\t}\n-\t'\n-\tencoding=$(git config i18n.commitencoding || echo UTF-8)\n-\tgit show -s --pretty=raw --encoding=\"$encoding\" \"$1\" -- |\n-\tLANG=C LC_ALL=C sed -ne \"$pick_author_script\"\n+\tencoding=$(git config i18n.commitencoding || echo UTF-8) &&\n+\tauthor_ident_name=$(git show -s --pretty=\"format:%an\" \\\n+\t\t--encoding=\"$encoding\" \"$1\" --) &&\n+\tauthor_ident_mail=$(git show -s --pretty=\"format:%ae\" \\\n+\t\t--encoding=\"$encoding\" \"$1\" --) &&\n+\tauthor_ident_date=$(git show -s --pretty=\"format:%ai\" \\\n+\t\t--encoding=\"$encoding\" \"$1\" --)\n+}\n+\n+save_author_ident () {\n+\tgit config --file=\"$SAVE_AUTHOR_INFO\" rebase.author.name \\\n+\t\t\"$author_ident_name\" &&\n+\tgit config --file=\"$SAVE_AUTHOR_INFO\" rebase.author.mail \\\n+\t\t\"$author_ident_mail\" &&\n+\tgit config --file=\"$SAVE_AUTHOR_INFO\" rebase.author.date \\\n+\t\t\"$author_ident_date\"\n+}\n+\n+load_author_ident () {\n+\tGIT_AUTHOR_NAME=$(git config --file=\"$SAVE_AUTHOR_INFO\" \\\n+\t\trebase.author.name) &&\n+\tGIT_AUTHOR_EMAIL=$(git config --file=\"$SAVE_AUTHOR_INFO\" \\\n+\t\trebase.author.mail) &&\n+\tGIT_AUTHOR_DATE=$(git config --file=\"$SAVE_AUTHOR_INFO\" \\\n+\t\trebase.author.date)\n }\n \n make_patch () {\n@@ -158,8 +160,10 @@ make_patch () {\n \tesac > \"$DOTEST\"/patch\n \ttest -f \"$DOTEST\"/message ||\n \t\tgit cat-file commit \"$1\" | sed \"1,/^$/d\" > \"$DOTEST\"/message\n-\ttest -f \"$DOTEST\"/author-script ||\n-\t\tget_author_ident_from_commit \"$1\" > \"$DOTEST\"/author-script\n+\ttest -f \"$SAVE_AUTHOR_INFO\" || {\n+\t\tget_author_ident_from_commit \"$1\"\n+\t\tsave_author_ident\n+\t}\n }\n \n die_with_patch () {\n@@ -294,8 +298,10 @@ pick_one_preserving_merges () {\n \t\t\ttest \"a$1\" = a-n && die \"Refusing to squash a merge: $sha1\"\n \n \t\t\t# redo merge\n-\t\t\tauthor_script=$(get_author_ident_from_commit $sha1)\n-\t\t\teval \"$author_script\"\n+\t\t\tget_author_ident_from_commit $sha1\n+\t\t\tGIT_AUTHOR_NAME=\"$author_ident_name\"\n+\t\t\tGIT_AUTHOR_EMAIL=\"$author_ident_mail\"\n+\t\t\tGIT_AUTHOR_DATE=\"$author_ident_date\"\n \t\t\tmsg=\"$(git cat-file commit $sha1 | sed -e '1,/^$/d')\"\n \t\t\t# No point in merging the first parent, that's HEAD\n \t\t\tnew_parents=${new_parents# $first_parent}\n@@ -353,8 +359,7 @@ peek_next_command () {\n }\n \n do_next () {\n-\trm -f \"$DOTEST\"/message \"$DOTEST\"/author-script \\\n-\t\t\"$DOTEST\"/amend || exit\n+\trm -f \"$DOTEST\"/message \"$DOTEST\"/amend \"$SAVE_AUTHOR_INFO\" || exit\n \tread command sha1 rest < \"$TODO\"\n \tcase \"$command\" in\n \t'#'*|''|noop)\n@@ -395,7 +400,7 @@ do_next () {\n \t\tmark_action_done\n \t\tmake_squash_message $sha1 > \"$MSG\"\n \t\tfailed=f\n-\t\tauthor_script=$(get_author_ident_from_commit HEAD)\n+\t\tget_author_ident_from_commit HEAD\n \t\toutput git reset --soft HEAD^\n \t\tpick_one -n $sha1 || failed=t\n \t\tcase \"$(peek_next_command)\" in\n@@ -414,14 +419,13 @@ do_next () {\n \t\t\trm -f \"$GIT_DIR\"/MERGE_MSG || exit\n \t\t\t;;\n \t\tesac\n-\t\techo \"$author_script\" > \"$DOTEST\"/author-script\n+\t\tsave_author_ident\n \t\tif test $failed = f\n \t\tthen\n \t\t\t# This is like --amend, but with a different message\n-\t\t\teval \"$author_script\"\n-\t\t\tGIT_AUTHOR_NAME=\"$GIT_AUTHOR_NAME\" \\\n-\t\t\tGIT_AUTHOR_EMAIL=\"$GIT_AUTHOR_EMAIL\" \\\n-\t\t\tGIT_AUTHOR_DATE=\"$GIT_AUTHOR_DATE\" \\\n+\t\t\tGIT_AUTHOR_NAME=\"$author_ident_name\" \\\n+\t\t\tGIT_AUTHOR_EMAIL=\"$author_ident_mail\" \\\n+\t\t\tGIT_AUTHOR_DATE=\"$author_ident_date\" \\\n \t\t\t$USE_OUTPUT git commit --no-verify \\\n \t\t\t\t$MSG_OPT \"$EDIT_OR_FILE\" || failed=t\n \t\tfi\n@@ -536,7 +540,7 @@ do\n \t\tthen\n \t\t\t: Nothing to commit -- skip this\n \t\telse\n-\t\t\t. \"$DOTEST\"/author-script ||\n+\t\t\tload_author_ident ||\n \t\t\t\tdie \"Cannot find the author identity\"\n \t\t\tamend=\n \t\t\tif test -f \"$DOTEST\"/amend\n-- \n1.6.3.GIT\n"},{"id":"116695","messageId":"7vprcyt7fk.fsf@alter.siamese.dyndns.org","threadId":"19882","inReplyTo":"20090621050846.3554.31660.chriscool@tuxfamily.org","subject":"Re: [PATCH v2 2/2] rebase -i: use config file format to save author information","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-06-21T06:41:35Z","receivedAt":"2009-06-21T06:41:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder <chriscool@tuxfamily.org> writes:\n\n> This is better than saving in a shell script, because it will make\n> it much easier to port \"rebase -i\" to C.\n\nHmph.\n\nWe used to parse a commit object in one go into variables, and I would\nhave imagined that a rewrite in C will read a commit object to get the\nauthor information in variables in-core, without having to write any\ntemporary file.\n\nBut with your patch, it starts to use a temporary file, and forces the C\nrewrite to do the same.  It closes the door for a more efficient rewrite.\n\nWhy is this a good change?\n\n> This also removes some sed regexps and some \"eval\"s.\n\nThat is a correct description of what the patch does.\n\nBut the use of sed and eval is an implementation detail.  As long as they\nare used correctly, I do not think it is a grave offense to use them, and\nremoval is not an improvement.\n\nA possible argument (or excuse) you _could_ make to justify this change, I\nthink, is that such a temporary file with known name will allow a hook\nscript that is run during a rebase to learn the authorship information of\nthe commit being rebased.  This has been hidden inside variables without\ngetting exported, and this patch defines a way for the scripts to get at\nit if they wanted to.\n\nI didn't check if there is such a codepath that involves hooks, if there\nare plausible scenarios that they would want to learn that information,\nnor if the information is hard to get otherwise, though.\n"},{"id":"116696","messageId":"alpine.LNX.2.00.0906210244510.2147@iabervon.org","threadId":"19882","inReplyTo":"7vprcyt7fk.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 2/2] rebase -i: use config file format to save author information","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2009-06-21T06:51:53Z","receivedAt":"2009-06-21T06:51:53Z","isPatch":true,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Sat, 20 Jun 2009, Junio C Hamano wrote:\n\n> Christian Couder <chriscool@tuxfamily.org> writes:\n> \n> > This is better than saving in a shell script, because it will make\n> > it much easier to port \"rebase -i\" to C.\n> \n> Hmph.\n> \n> We used to parse a commit object in one go into variables, and I would\n> have imagined that a rewrite in C will read a commit object to get the\n> author information in variables in-core, without having to write any\n> temporary file.\n> \n> But with your patch, it starts to use a temporary file, and forces the C\n> rewrite to do the same.  It closes the door for a more efficient rewrite.\n> \n> Why is this a good change?\n\nIt was always using a temporary file; it just used to use a temporary file \nthat was a shell script fragment and needed to be read with \"eval\". It \ncan't be done entirely in core because it may be determined before a \nconflict and only used when run with --continue after the user resolves \nthe conflict.\n\n\t-Daniel\n*This .sig left intentionally blank*\n"},{"id":"116697","messageId":"7vhbyat6qc.fsf@alter.siamese.dyndns.org","threadId":"19882","inReplyTo":"alpine.LNX.2.00.0906210244510.2147@iabervon.org","subject":"Re: [PATCH v2 2/2] rebase -i: use config file format to save author information","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-06-21T06:56:43Z","receivedAt":"2009-06-21T06:56:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Daniel Barkalow <barkalow@iabervon.org> writes:\n\n>> Why is this a good change?\n>\n> It was always using a temporary file; it just used to use a temporary file \n> that was a shell script fragment and needed to be read with \"eval\". It \n> can't be done entirely in core because it may be determined before a \n> conflict and only used when run with --continue after the user resolves \n> the conflict.\n\nAhh, Ok.\n\nUsing a _known_ and defined format, instead of ad-hoc scriptlet, is an\nimprovement.\n\nI still wonder if we can avoid using three separate \"git show\" and \"git\nconfig\" invocations, though.  But a half of that inefficiency will go away\nwhen this is migrated to C, as a single git_config() will grab all three, \nalthough the writing side is still very inefficient X-<.\n"},{"id":"116698","messageId":"200906211001.42254.jnareb@gmail.com","threadId":"19882","inReplyTo":"7vhbyat6qc.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 2/2] rebase -i: use config file format to save author information","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2009-06-21T08:01:41Z","receivedAt":"2009-06-21T08:01:41Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Sun, 21 June 2009, Junio C Hamano wrote:\n> Daniel Barkalow <barkalow@iabervon.org> writes:\n> \n>>> Why is this a good change?\n>>\n>> It was always using a temporary file; it just used to use a temporary file \n>> that was a shell script fragment and needed to be read with \"eval\". It \n>> can't be done entirely in core because it may be determined before a \n>> conflict and only used when run with --continue after the user resolves \n>> the conflict.\n> \n> Ahh, Ok.\n> \n> Using a _known_ and defined format, instead of ad-hoc scriptlet, is an\n> improvement.\n> \n> I still wonder if we can avoid using three separate \"git show\" and \"git\n> config\" invocations, though.  But a half of that inefficiency will go away\n> when this is migrated to C, as a single git_config() will grab all three, \n> although the writing side is still very inefficient X-<.\n\nI think we can on the reading side: just use \"git config --list\", or\nperhaps \"git config --get-regexp <sth>\" (where <sth> can be \".*\") which\nconveniently has SPC as separator, and feed it to appropriate 3 x 'read'.\n\nOn the writing side we can simply write in the config file format, we\ndon't need to use git-config for that.  Although I wonder if there won't\nbe trouble with shell escaping and quoting rules (eval / sed, which\nI guess does shell quoting / shell unquoting).\n\nOn getting the information side we can use git-show with custom format\nor git-cat-file fed to while-read-case construct.\n\n\nThis way from 3 x 3 = 9 git commands git-rebase--interactive.sh would\nuse only 2.\n\n-- \nJakub Narebski\nPoland\n"},{"id":"116699","messageId":"7vbpoit3b2.fsf@alter.siamese.dyndns.org","threadId":"19882","inReplyTo":"200906211001.42254.jnareb@gmail.com","subject":"Re: [PATCH v2 2/2] rebase -i: use config file format to save author information","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-06-21T08:10:41Z","receivedAt":"2009-06-21T08:10:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narebski <jnareb@gmail.com> writes:\n\n> On Sun, 21 June 2009, Junio C Hamano wrote:\n>> Daniel Barkalow <barkalow@iabervon.org> writes:\n>> \n>>>> Why is this a good change?\n>>>\n>>> It was always using a temporary file; it just used to use a temporary file \n>>> that was a shell script fragment and needed to be read with \"eval\". It \n>>> can't be done entirely in core because it may be determined before a \n>>> conflict and only used when run with --continue after the user resolves \n>>> the conflict.\n>> \n>> Ahh, Ok.\n>> \n>> Using a _known_ and defined format, instead of ad-hoc scriptlet, is an\n>> improvement.\n>> \n>> I still wonder if we can avoid using three separate \"git show\" and \"git\n>> config\" invocations, though.  But a half of that inefficiency will go away\n>> when this is migrated to C, as a single git_config() will grab all three, \n>> although the writing side is still very inefficient X-<.\n>\n> I think we can on the reading side: just use \"git config --list\", or\n> perhaps \"git config --get-regexp <sth>\" (where <sth> can be \".*\") which\n> conveniently has SPC as separator, and feed it to appropriate 3 x 'read'.\n>\n> On the writing side we can simply write in the config file format, we\n> don't need to use git-config for that.  Although I wonder if there won't\n> be trouble with shell escaping and quoting rules (eval / sed, which\n> I guess does shell quoting / shell unquoting).\n>\n> On getting the information side we can use git-show with custom format\n> or git-cat-file fed to while-read-case construct.\n>\n> This way from 3 x 3 = 9 git commands git-rebase--interactive.sh would\n> use only 2.\n\nThat's not what I meant.\n"},{"id":"116708","messageId":"200906211153.03969.chriscool@tuxfamily.org","threadId":"19882","inReplyTo":"7vhbyat6qc.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 2/2] rebase -i: use config file format to save author information","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2009-06-21T09:53:03Z","receivedAt":"2009-06-21T09:53:03Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Sunday 21 June 2009, Junio C Hamano wrote:\n> Daniel Barkalow <barkalow@iabervon.org> writes:\n> >> Why is this a good change?\n> >\n> > It was always using a temporary file; it just used to use a temporary\n> > file that was a shell script fragment and needed to be read with\n> > \"eval\". It can't be done entirely in core because it may be determined\n> > before a conflict and only used when run with --continue after the user\n> > resolves the conflict.\n>\n> Ahh, Ok.\n>\n> Using a _known_ and defined format, instead of ad-hoc scriptlet, is an\n> improvement.\n>\n> I still wonder if we can avoid using three separate \"git show\" and \"git\n> config\" invocations, though.  But a half of that inefficiency will go\n> away when this is migrated to C, as a single git_config() will grab all\n> three, although the writing side is still very inefficient X-<.\n\nIt may be possible to write a ref to the commit we need information from. \nAnd then when \"git rebase --continue\" is called, we would read this ref and \nthen get information from the referenced commit?\n\nBest regards,\nChristian.\n"},{"id":"116711","messageId":"7v8wjlsxc7.fsf@alter.siamese.dyndns.org","threadId":"19882","inReplyTo":"200906211153.03969.chriscool@tuxfamily.org","subject":"Re: [PATCH v2 2/2] rebase -i: use config file format to save author information","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-06-21T10:19:36Z","receivedAt":"2009-06-21T10:19:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder <chriscool@tuxfamily.org> writes:\n\n> It may be possible to write a ref to the commit we need information from. \n> And then when \"git rebase --continue\" is called, we would read this ref and \n> then get information from the referenced commit?\n\nYou mean replacing author-script temporary file with the name of the\ncommit object we are \"pick\"ing?\n\nThat surely sounds like a more direct approach.  I wonder why the script\ndidn't do that in the first place (I suspect it was just because it was\neasier to implement by borrowing the logic to parse authorship information\nfrom git-revert-script).\n"}]}