{"thread":{"id":"22339","subject":"[PATCH 0/3] Fixes to mh/rebase-fixup","startedAt":"2010-01-22T09:22:28Z","lastAt":"2010-01-24T05:33:26Z","messageCount":7,"participants":["Michael Haggerty","Johannes Schindelin","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"132391","messageId":"cover.1264151435.git.mhagger@alum.mit.edu","threadId":"22339","inReplyTo":null,"subject":"[PATCH 0/3] Fixes to mh/rebase-fixup","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2010-01-22T09:22:28Z","receivedAt":"2010-01-22T09:22:28Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"This patch series contains fixes to the mh/rebase-fixup patch series\nas suggested by reviewers on the mailing list.\n\nSince the mh/rebase-fixup patch series is already in \"next\", I have\nwritten these fixup patches against \"next\".  I hope that is OK.\n\nThe third patch removes the attempt to use one-shot export of\nvariables in do_with_author(), as the \"$@\" command that it calls might\nbe a shell function.  This issue was discussed on the mailing list\n[1].  This change has the side effect that the GIT_AUTHOR_* variables\nremain exported after do_with_author() returns, but I don't think that\nthis is a problem in the context of git-rebase--interactive.sh.\nHowever, additional review would be welcome here.\n\n[1] http://marc.info/?l=git&m=126345851831751&w=2\n\nMichael Haggerty (3):\n  rebase -i: Avoid non-portable \"test X -a Y\"\n  rebase -i: Enclose sed command substitution in quotes\n  rebase -i: Export GIT_AUTHOR_* variables explicitly\n\n git-rebase--interactive.sh |    6 ++----\n t/lib-rebase.sh            |    2 +-\n 2 files changed, 3 insertions(+), 5 deletions(-)\n"},{"id":"132393","messageId":"0c2c17066d3bc369e4fb8f96839f63b50606fd5f.1264151435.git.mhagger@alum.mit.edu","threadId":"22339","inReplyTo":"cover.1264151435.git.mhagger@alum.mit.edu","subject":"[PATCH 1/3] rebase -i: Avoid non-portable \"test X -a Y\"","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2010-01-22T09:22:29Z","receivedAt":"2010-01-22T09:22:29Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Reported by: Eric Blake <ebb9@byu.net>\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n git-rebase--interactive.sh |    2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\nindex e551906..c2f6089 100755\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -235,7 +235,7 @@ pick_one () {\n \tparent_sha1=$(git rev-parse --verify $sha1^) ||\n \t\tdie \"Could not get the parent of $sha1\"\n \tcurrent_sha1=$(git rev-parse --verify HEAD)\n-\tif test -z \"$no_ff\" -a \"$current_sha1\" = \"$parent_sha1\"\n+\tif test -z \"$no_ff\" && test \"$current_sha1\" = \"$parent_sha1\"\n \tthen\n \t\toutput git reset --hard $sha1\n \t\toutput warn Fast-forward to $(git rev-parse --short $sha1)\n-- \n1.6.6\n"},{"id":"132394","messageId":"45e8a3c5dc32a67e4a8ed82c8990e3ee85232581.1264151435.git.mhagger@alum.mit.edu","threadId":"22339","inReplyTo":"cover.1264151435.git.mhagger@alum.mit.edu","subject":"[PATCH 2/3] rebase -i: Enclose sed command substitution in quotes","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2010-01-22T09:22:30Z","receivedAt":"2010-01-22T09:22:30Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Reported by: Johannes Sixt <j.sixt@viscovery.net>\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n t/lib-rebase.sh |    2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/t/lib-rebase.sh b/t/lib-rebase.sh\nindex 2d922ae..6aefe27 100644\n--- a/t/lib-rebase.sh\n+++ b/t/lib-rebase.sh\n@@ -27,7 +27,7 @@ set_fake_editor () {\n case \"$1\" in\n */COMMIT_EDITMSG)\n \ttest -z \"$EXPECT_HEADER_COUNT\" ||\n-\t\ttest \"$EXPECT_HEADER_COUNT\" = $(sed -n '1s/^# This is a combination of \\(.*\\) commits\\./\\1/p' < \"$1\") ||\n+\t\ttest \"$EXPECT_HEADER_COUNT\" = \"$(sed -n '1s/^# This is a combination of \\(.*\\) commits\\./\\1/p' < \"$1\")\" ||\n \t\texit\n \ttest -z \"$FAKE_COMMIT_MESSAGE\" || echo \"$FAKE_COMMIT_MESSAGE\" > \"$1\"\n \ttest -z \"$FAKE_COMMIT_AMEND\" || echo \"$FAKE_COMMIT_AMEND\" >> \"$1\"\n-- \n1.6.6\n"},{"id":"132392","messageId":"c6efda03848abc00cf8bf8d84fc34ef0d652b64c.1264151435.git.mhagger@alum.mit.edu","threadId":"22339","inReplyTo":"cover.1264151435.git.mhagger@alum.mit.edu","subject":"[PATCH 3/3] rebase -i: Export GIT_AUTHOR_* variables explicitly","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2010-01-22T09:22:31Z","receivedAt":"2010-01-22T09:22:31Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"As pointed out on the mailing list, one-shot shell variable exports do\nnot necessarily work with shell functions.  So export the GIT_AUTHOR_*\nvariables explicitly using \"export\".\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n git-rebase--interactive.sh |    4 +---\n 1 files changed, 1 insertions(+), 3 deletions(-)\n\ndiff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\nindex c2f6089..c8603f0 100755\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -215,9 +215,7 @@ has_action () {\n # Run command with GIT_AUTHOR_NAME, GIT_AUTHOR_EMAIL, and\n # GIT_AUTHOR_DATE exported from the current environment.\n do_with_author () {\n-\tGIT_AUTHOR_NAME=\"$GIT_AUTHOR_NAME\" \\\n-\tGIT_AUTHOR_EMAIL=\"$GIT_AUTHOR_EMAIL\" \\\n-\tGIT_AUTHOR_DATE=\"$GIT_AUTHOR_DATE\" \\\n+\texport GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL GIT_AUTHOR_DATE &&\n \t\"$@\"\n }\n \n-- \n1.6.6\n"},{"id":"132396","messageId":"alpine.DEB.1.00.1001221212350.4985@pacific.mpi-cbg.de","threadId":"22339","inReplyTo":"c6efda03848abc00cf8bf8d84fc34ef0d652b64c.1264151435.git.mhagger@alum.mit.edu","subject":"Re: [PATCH 3/3] rebase -i: Export GIT_AUTHOR_* variables explicitly","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2010-01-22T11:16:16Z","receivedAt":"2010-01-22T11:16:16Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Fri, 22 Jan 2010, Michael Haggerty wrote:\n\n> As pointed out on the mailing list, one-shot shell variable exports do\n> not necessarily work with shell functions.  So export the GIT_AUTHOR_*\n> variables explicitly using \"export\".\n\nThis one's a bit hairy; I really was not sure about unintended side \neffects, that is why I avoided the export.\n\nJust imagine, for example, some git commit --amend which forgets to set \nthe author information; I am not saying that this is happening, but I \ncannot be sure, because every possible code path to a git \ncommit/commit-tree has to be checked, and this does not mean that future \npatches will not introduce such broken code, either.\n\nIt might also be possible that some people scripted rebase -i with a \ncustom \"editor\" as in the tests (I have done so in the past).  They would \nbe affected.\n\nCiao,\nDscho\n"},{"id":"132423","messageId":"7viqatu9rv.fsf@alter.siamese.dyndns.org","threadId":"22339","inReplyTo":"alpine.DEB.1.00.1001221212350.4985@pacific.mpi-cbg.de","subject":"Re: [PATCH 3/3] rebase -i: Export GIT_AUTHOR_* variables explicitly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-01-22T21:09:24Z","receivedAt":"2010-01-22T21:09:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n> On Fri, 22 Jan 2010, Michael Haggerty wrote:\n>\n>> As pointed out on the mailing list, one-shot shell variable exports do\n>> not necessarily work with shell functions.  So export the GIT_AUTHOR_*\n>> variables explicitly using \"export\".\n>\n> This one's a bit hairy; I really was not sure about unintended side \n> effects, that is why I avoided the export.\n> ... (sensible worries omitted) ...\n\nMy reading of \"git grep -C3 AUTHOR git-rebase--interactive.sh\" is that\nGIT_AUTHOR_NAME/EMAIL/DATE are set by:\n \n   - redoing a merge (while preserving history) by reading from the merge;\n\n   - squashing/fixing into the last one (relying on the last one did the\n     right thing) by reading from the HEAD; or\n\n   - sourcing AUTHOR_SCRIPT upon --continue, reading from the file left\n     by make_patch immediately before the previous invocation gave control\n     back to the user (e.g. \"e\"dit, or conflict).\n\nThe places we want to see these variables in effect are while doing one of\nthe above three places, and they all do this with do_with_author.\n\nNothing that is called with do_with_author has a side effect of setting\nshell variables for the caller of do_with_author.\n\nSo I tend to think this patch would be the cleanest and safest\nalternative, albeit it may cost an extra fork.\n\nWhat do you think?\n\n-- >8 --\nSubject: rebase -i: Export GIT_AUTHOR_* variables explicitly\n\nThere is no point doing self-assignments of these variables.  Instead,\njust export them to the environment, but do so in a sub-shell, because\n\n\tVAR1=VAL1 VAR2=VAL2 ... command arg1 arg2...\n\ndoes not mark the variables exported if command that is run\nis a shell function, according to POSIX.1.\n\nThe callers of do_with_author do not rely on seeing the effect of any\nshell variable assignments that may happen inside what was called through\nthis shell function (currently \"output\" is the only one), so running it in\nthe subshell doesn't have an adverse semantic effect.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n git-rebase--interactive.sh |    8 ++++----\n 1 files changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\nindex c2f6089..9187e9b 100755\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -215,10 +215,10 @@ has_action () {\n # Run command with GIT_AUTHOR_NAME, GIT_AUTHOR_EMAIL, and\n # GIT_AUTHOR_DATE exported from the current environment.\n do_with_author () {\n-\tGIT_AUTHOR_NAME=\"$GIT_AUTHOR_NAME\" \\\n-\tGIT_AUTHOR_EMAIL=\"$GIT_AUTHOR_EMAIL\" \\\n-\tGIT_AUTHOR_DATE=\"$GIT_AUTHOR_DATE\" \\\n-\t\"$@\"\n+\t(\n+\t\texport GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL GIT_AUTHOR_DATE\n+\t\t\"$@\"\n+\t)\n }\n \n pick_one () {\n"},{"id":"132524","messageId":"4B5BDBA6.9090307@alum.mit.edu","threadId":"22339","inReplyTo":"7viqatu9rv.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 3/3] rebase -i: Export GIT_AUTHOR_* variables explicitly","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2010-01-24T05:33:26Z","receivedAt":"2010-01-24T05:33:26Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Junio C Hamano wrote:\n> [...]\n> So I tend to think this patch would be the cleanest and safest\n> alternative, albeit it may cost an extra fork.\n> \n> What do you think?\n> \n> -- >8 --\n> Subject: rebase -i: Export GIT_AUTHOR_* variables explicitly\n\nYour version is fine with me.\n\nMichael\n"}]}