{"thread":{"id":"29346","subject":"[PATCH 1/3] mailinfo documentation: accurately describe non -k case","startedAt":"2012-01-11T20:13:42Z","lastAt":"2012-01-20T13:04:03Z","messageCount":9,"participants":["Thomas Rast","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"182377","messageId":"e915a551c9bbf12f4d8fd929e9ed24f3223790ee.1326312730.git.trast@student.ethz.ch","threadId":"29346","inReplyTo":null,"subject":"[PATCH 1/3] mailinfo documentation: accurately describe non -k case","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2012-01-11T20:13:42Z","receivedAt":"2012-01-11T20:13:42Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Since its very first description of -k, the documentation for\ngit-mailinfo claimed that (in the case without -k) after cleaning up\nbracketed strings [blah], it would insert [PATCH].\n\nIt doesn't; on the contrary, one of the important jobs of mailinfo is\nto remove those strings.\n\nSince we're already there, rewrite the paragraph to give a complete\nenumeration of all the transformations.  Specifically, it was missing\nthe whitespace normalization (run of isspace(c) -> ' ') and the\nremoval of leading ':'.\n\nSigned-off-by: Thomas Rast <trast@student.ethz.ch>\n---\n Documentation/git-mailinfo.txt |   25 ++++++++++++++++++-------\n 1 files changed, 18 insertions(+), 7 deletions(-)\n\ndiff --git a/Documentation/git-mailinfo.txt b/Documentation/git-mailinfo.txt\nindex 51dc325..97e7a8e 100644\n--- a/Documentation/git-mailinfo.txt\n+++ b/Documentation/git-mailinfo.txt\n@@ -25,13 +25,24 @@ command directly.  See linkgit:git-am[1] instead.\n OPTIONS\n -------\n -k::\n-\tUsually the program 'cleans up' the Subject: header line\n-\tto extract the title line for the commit log message,\n-\tamong which (1) remove 'Re:' or 're:', (2) leading\n-\twhitespaces, (3) '[' up to ']', typically '[PATCH]', and\n-\tthen prepends \"[PATCH] \".  This flag forbids this\n-\tmunging, and is most useful when used to read back\n-\t'git format-patch -k' output.\n+\tUsually the program removes email cruft from the Subject:\n+\theader line to extract the title line for the commit log\n+\tmessage.  This option prevents this munging, and is most\n+\tuseful when used to read back 'git format-patch -k' output.\n++\n+Specifically, the following are removed until none of them remain:\n++\n+--\n+*\tLeading and trailing whitespace.\n+\n+*\tLeading `Re:`, `re:`, and `:`.\n+\n+*\tLeading bracketed strings (between `[` and `]`, usually\n+\t`[PATCH]`).\n+--\n++\n+Finally, runs of whitespace are normalized to a single ASCII space\n+character.\n \n -b::\n \tWhen -k is not in effect, all leading strings bracketed with '['\n-- \n1.7.9.rc0.168.g3847c\n"},{"id":"182379","messageId":"19539098c07a207f3bd24f5a145ba3b6c5e46766.1326312730.git.trast@student.ethz.ch","threadId":"29346","inReplyTo":"e915a551c9bbf12f4d8fd929e9ed24f3223790ee.1326312730.git.trast@student.ethz.ch","subject":"[PATCH 2/3] am: learn passing -b to mailinfo","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2012-01-11T20:13:43Z","receivedAt":"2012-01-11T20:13:43Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"git-am could pass -k to mailinfo, but not -b.  Introduce an option\nthat does so.  We change the meaning of the 'keep' state file, but are\ncareful not to cause a problem unless you downgrade in the middle of\nan 'am' run.\n\nThis uncovers a bug in mailinfo -b, hence the failing test.\n\nSigned-off-by: Thomas Rast <trast@student.ethz.ch>\n---\n\nSomeone on IRC asked about git-am support for passing through the -b\nflag to mailinfo.  And so it began...\n\n Documentation/git-am.txt |    3 +++\n git-am.sh                |   11 +++++++----\n t/t4150-am.sh            |   14 ++++++++++++--\n 3 files changed, 22 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/git-am.txt b/Documentation/git-am.txt\nindex 887466d..ee6cca2 100644\n--- a/Documentation/git-am.txt\n+++ b/Documentation/git-am.txt\n@@ -40,6 +40,9 @@ OPTIONS\n --keep::\n \tPass `-k` flag to 'git mailinfo' (see linkgit:git-mailinfo[1]).\n \n+--keep-non-patch::\n+\tPass `-b` flag to 'git mailinfo' (see linkgit:git-mailinfo[1]).\n+\n --keep-cr::\n --no-keep-cr::\n \tWith `--keep-cr`, call 'git mailsplit' (see linkgit:git-mailsplit[1])\ndiff --git a/git-am.sh b/git-am.sh\nindex 1c13b13..8dfad7a 100755\n--- a/git-am.sh\n+++ b/git-am.sh\n@@ -15,6 +15,7 @@ q,quiet         be quiet\n s,signoff       add a Signed-off-by line to the commit message\n u,utf8          recode into utf8 (default)\n k,keep          pass -k flag to git-mailinfo\n+keep-non-patch  pass -b flag to git-mailinfo\n keep-cr         pass --keep-cr flag to git-mailsplit for mbox format\n no-keep-cr      do not pass --keep-cr flag to git-mailsplit independent of am.keepcr\n c,scissors      strip everything before a scissors line\n@@ -386,7 +387,9 @@ do\n \t--no-utf8)\n \t\tutf8= ;;\n \t-k|--keep)\n-\t\tkeep=t ;;\n+\t\tkeep=-k ;;\n+\t--keep-non-patch)\n+\t\tkeep=-b ;;\n \t-c|--scissors)\n \t\tscissors=t ;;\n \t--no-scissors)\n@@ -398,7 +401,7 @@ do\n \t--abort)\n \t\tabort=t ;;\n \t--rebasing)\n-\t\trebasing=t threeway=t keep=t scissors=f no_inbody_headers=t ;;\n+\t\trebasing=t threeway=t keep=-k scissors=f no_inbody_headers=t ;;\n \t-d|--dotest)\n \t\tdie \"$(gettext \"-d option is no longer supported.  Do not use.\")\"\n \t\t;;\n@@ -571,8 +574,8 @@ then\n else\n \tutf8=-n\n fi\n-if test \"$(cat \"$dotest/keep\")\" = t\n-then\n+keep=$(cat \"$dotest/keep\")\n+if test \"$keep\" = t\n \tkeep=-k\n fi\n case \"$(cat \"$dotest/scissors\")\" in\ndiff --git a/t/t4150-am.sh b/t/t4150-am.sh\nindex d7d9ccc..7e7c83c 100755\n--- a/t/t4150-am.sh\n+++ b/t/t4150-am.sh\n@@ -237,7 +237,7 @@ test_expect_success 'am stays in branch' '\n \n test_expect_success 'am --signoff does not add Signed-off-by: line if already there' '\n \tgit format-patch --stdout HEAD^ >patch3 &&\n-\tsed -e \"/^Subject/ s,\\[PATCH,Re: Re: Re: & 1/5 v2,\" patch3 >patch4 &&\n+\tsed -e \"/^Subject/ s,\\[PATCH,Re: Re: Re: & 1/5 v2] [foo,\" patch3 >patch4 &&\n \trm -fr .git/rebase-apply &&\n \tgit reset --hard &&\n \tgit checkout HEAD^ &&\n@@ -259,7 +259,17 @@ test_expect_success 'am --keep really keeps the subject' '\n \tgit am --keep patch4 &&\n \t! test -d .git/rebase-apply &&\n \tgit cat-file commit HEAD >actual &&\n-\tgrep \"Re: Re: Re: \\[PATCH 1/5 v2\\] third\" actual\n+\tgrep \"Re: Re: Re: \\[PATCH 1/5 v2\\] \\[foo\\] third\" actual\n+'\n+\n+test_expect_failure 'am --keep-non-patch really keeps the non-patch part' '\n+\trm -fr .git/rebase-apply &&\n+\tgit reset --hard &&\n+\tgit checkout HEAD^ &&\n+\tgit am --keep-non-patch patch4 &&\n+\t! test -d .git/rebase-apply &&\n+\tgit cat-file commit HEAD >actual &&\n+\tgrep \"^\\[foo\\] third\" actual\n '\n \n test_expect_success 'am -3 falls back to 3-way merge' '\n-- \n1.7.9.rc0.168.g3847c\n"},{"id":"182378","messageId":"673657ccd5577ef5c2b6be2c7d0039218c31596d.1326312730.git.trast@student.ethz.ch","threadId":"29346","inReplyTo":"e915a551c9bbf12f4d8fd929e9ed24f3223790ee.1326312730.git.trast@student.ethz.ch","subject":"[PATCH 3/3] mailinfo: with -b, keep space after [foo]","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2012-01-11T20:13:44Z","receivedAt":"2012-01-11T20:13:44Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"The logic for the -b mode, where [PATCH] is dropped but [foo] is not,\nsilently ate all spaces after the ].\n\nFix this by keeping the next isspace() character, if there is any.\nBeing more thorough is pointless, as the later cleanup_space() call\nwill normalize any sequence of whitespace to a single ' '.\n\nSigned-off-by: Thomas Rast <trast@student.ethz.ch>\n---\n builtin/mailinfo.c |   11 ++++++++++-\n t/t4150-am.sh      |    2 +-\n 2 files changed, 11 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/mailinfo.c b/builtin/mailinfo.c\nindex bfb32b7..eaf9e15 100644\n--- a/builtin/mailinfo.c\n+++ b/builtin/mailinfo.c\n@@ -250,8 +250,17 @@ static void cleanup_subject(struct strbuf *subject)\n \t\t\t    (7 <= remove &&\n \t\t\t     memmem(subject->buf + at, remove, \"PATCH\", 5)))\n \t\t\t\tstrbuf_remove(subject, at, remove);\n-\t\t\telse\n+\t\t\telse {\n \t\t\t\tat += remove;\n+\t\t\t\t/*\n+\t\t\t\t * If the input had a space after the ], keep\n+\t\t\t\t * it.  We don't bother with finding the end of\n+\t\t\t\t * the space, since we later normalize it\n+\t\t\t\t * anyway.\n+\t\t\t\t */\n+\t\t\t\tif (isspace(subject->buf[at]))\n+\t\t\t\t\tat += 1;\n+\t\t\t}\n \t\t\tcontinue;\n \t\t}\n \t\tbreak;\ndiff --git a/t/t4150-am.sh b/t/t4150-am.sh\nindex 7e7c83c..8807b60 100755\n--- a/t/t4150-am.sh\n+++ b/t/t4150-am.sh\n@@ -262,7 +262,7 @@ test_expect_success 'am --keep really keeps the subject' '\n \tgrep \"Re: Re: Re: \\[PATCH 1/5 v2\\] \\[foo\\] third\" actual\n '\n \n-test_expect_failure 'am --keep-non-patch really keeps the non-patch part' '\n+test_expect_success 'am --keep-non-patch really keeps the non-patch part' '\n \trm -fr .git/rebase-apply &&\n \tgit reset --hard &&\n \tgit checkout HEAD^ &&\n-- \n1.7.9.rc0.168.g3847c\n"},{"id":"182398","messageId":"7vzkdt4s9l.fsf@alter.siamese.dyndns.org","threadId":"29346","inReplyTo":"19539098c07a207f3bd24f5a145ba3b6c5e46766.1326312730.git.trast@student.ethz.ch","subject":"Re: [PATCH 2/3] am: learn passing -b to mailinfo","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-01-12T01:35:34Z","receivedAt":"2012-01-12T01:35:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Rast <trast@student.ethz.ch> writes:\n\n> @@ -571,8 +574,8 @@ then\n>  else\n>  \tutf8=-n\n>  fi\n> -if test \"$(cat \"$dotest/keep\")\" = t\n> -then\n> +keep=$(cat \"$dotest/keep\")\n> +if test \"$keep\" = t\n>  \tkeep=-k\n>  fi\n\nCurious.\n\nWho writes 't' to $dotest/keep after this patch is applied?\n\nI also do not want to worry about \"echo\" portability issues that may come\nfrom an existing\n\n\techo \"$keep\" >\"$dotest/keep\"\n\nthat this patch does not touch.\n\nI suspect that this patch was not tested in a way to exercise this\ncodepath; shell would have barfed when seeing the lack of \"then\" here, no?\n"},{"id":"182419","messageId":"8762ghxpxw.fsf@thomas.inf.ethz.ch","threadId":"29346","inReplyTo":"7vzkdt4s9l.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/3] am: learn passing -b to mailinfo","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2012-01-12T08:52:59Z","receivedAt":"2012-01-12T08:52:59Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Thomas Rast <trast@student.ethz.ch> writes:\n>\n>> @@ -571,8 +574,8 @@ then\n>>  else\n>>  \tutf8=-n\n>>  fi\n>> -if test \"$(cat \"$dotest/keep\")\" = t\n>> -then\n>> +keep=$(cat \"$dotest/keep\")\n>> +if test \"$keep\" = t\n>>  \tkeep=-k\n>>  fi\n>\n> Curious.\n>\n> Who writes 't' to $dotest/keep after this patch is applied?\n\nNobody; like the commit message says, I was just trying to help users\nupgrading from one version to the next in the middle of an 'am', which\nalmost worked except for\n\n> I suspect that this patch was not tested in a way to exercise this\n> codepath; shell would have barfed when seeing the lack of \"then\" here, no?\n\n(ouch)\n\n> I also do not want to worry about \"echo\" portability issues that may come\n> from an existing\n>\n> \techo \"$keep\" >\"$dotest/keep\"\n>\n> that this patch does not touch.\n\nGood point, thanks.  I'll reroll with printf.  Should I keep the\nupgrade path compatibility?\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"182610","messageId":"a804650f805fd8c89a843302cb92bbbdf36b8c0b.1326710194.git.trast@student.ethz.ch","threadId":"29346","inReplyTo":"8762ghxpxw.fsf@thomas.inf.ethz.ch","subject":"[PATCH v2 1/2] am: learn passing -b to mailinfo","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2012-01-16T10:53:00Z","receivedAt":"2012-01-16T10:53:00Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"git-am could pass -k to mailinfo, but not -b.  Introduce an option\nthat does so.  We change the meaning of the 'keep' state file, but are\ncareful not to cause a problem unless you downgrade in the middle of\nan 'am' run.\n\nThis uncovers a bug in mailinfo -b, hence the failing test.\n\nSigned-off-by: Thomas Rast <trast@student.ethz.ch>\n---\n\nThis fixes the broken 'if', and the use of 'echo' with an argument\nthat starts with '-'.\n\n Documentation/git-am.txt |    3 +++\n git-am.sh                |   12 ++++++++----\n t/t4150-am.sh            |   14 ++++++++++++--\n 3 files changed, 23 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/git-am.txt b/Documentation/git-am.txt\nindex 887466d..ee6cca2 100644\n--- a/Documentation/git-am.txt\n+++ b/Documentation/git-am.txt\n@@ -40,6 +40,9 @@ OPTIONS\n --keep::\n \tPass `-k` flag to 'git mailinfo' (see linkgit:git-mailinfo[1]).\n \n+--keep-non-patch::\n+\tPass `-b` flag to 'git mailinfo' (see linkgit:git-mailinfo[1]).\n+\n --keep-cr::\n --no-keep-cr::\n \tWith `--keep-cr`, call 'git mailsplit' (see linkgit:git-mailsplit[1])\ndiff --git a/git-am.sh b/git-am.sh\nindex 1c13b13..b8adde7 100755\n--- a/git-am.sh\n+++ b/git-am.sh\n@@ -15,6 +15,7 @@ q,quiet         be quiet\n s,signoff       add a Signed-off-by line to the commit message\n u,utf8          recode into utf8 (default)\n k,keep          pass -k flag to git-mailinfo\n+keep-non-patch  pass -b flag to git-mailinfo\n keep-cr         pass --keep-cr flag to git-mailsplit for mbox format\n no-keep-cr      do not pass --keep-cr flag to git-mailsplit independent of am.keepcr\n c,scissors      strip everything before a scissors line\n@@ -386,7 +387,9 @@ do\n \t--no-utf8)\n \t\tutf8= ;;\n \t-k|--keep)\n-\t\tkeep=t ;;\n+\t\tkeep=-k ;;\n+\t--keep-non-patch)\n+\t\tkeep=-b ;;\n \t-c|--scissors)\n \t\tscissors=t ;;\n \t--no-scissors)\n@@ -398,7 +401,7 @@ do\n \t--abort)\n \t\tabort=t ;;\n \t--rebasing)\n-\t\trebasing=t threeway=t keep=t scissors=f no_inbody_headers=t ;;\n+\t\trebasing=t threeway=t keep=-k scissors=f no_inbody_headers=t ;;\n \t-d|--dotest)\n \t\tdie \"$(gettext \"-d option is no longer supported.  Do not use.\")\"\n \t\t;;\n@@ -529,7 +532,7 @@ else\n \techo \"$threeway\" >\"$dotest/threeway\"\n \techo \"$sign\" >\"$dotest/sign\"\n \techo \"$utf8\" >\"$dotest/utf8\"\n-\techo \"$keep\" >\"$dotest/keep\"\n+\tprintf \"%s\" \"$keep\" >\"$dotest/keep\"\n \techo \"$scissors\" >\"$dotest/scissors\"\n \techo \"$no_inbody_headers\" >\"$dotest/no_inbody_headers\"\n \techo \"$GIT_QUIET\" >\"$dotest/quiet\"\n@@ -571,7 +574,8 @@ then\n else\n \tutf8=-n\n fi\n-if test \"$(cat \"$dotest/keep\")\" = t\n+keep=$(cat \"$dotest/keep\")\n+if test \"$keep\" = t\n then\n \tkeep=-k\n fi\ndiff --git a/t/t4150-am.sh b/t/t4150-am.sh\nindex d7d9ccc..7e7c83c 100755\n--- a/t/t4150-am.sh\n+++ b/t/t4150-am.sh\n@@ -237,7 +237,7 @@ test_expect_success 'am stays in branch' '\n \n test_expect_success 'am --signoff does not add Signed-off-by: line if already there' '\n \tgit format-patch --stdout HEAD^ >patch3 &&\n-\tsed -e \"/^Subject/ s,\\[PATCH,Re: Re: Re: & 1/5 v2,\" patch3 >patch4 &&\n+\tsed -e \"/^Subject/ s,\\[PATCH,Re: Re: Re: & 1/5 v2] [foo,\" patch3 >patch4 &&\n \trm -fr .git/rebase-apply &&\n \tgit reset --hard &&\n \tgit checkout HEAD^ &&\n@@ -259,7 +259,17 @@ test_expect_success 'am --keep really keeps the subject' '\n \tgit am --keep patch4 &&\n \t! test -d .git/rebase-apply &&\n \tgit cat-file commit HEAD >actual &&\n-\tgrep \"Re: Re: Re: \\[PATCH 1/5 v2\\] third\" actual\n+\tgrep \"Re: Re: Re: \\[PATCH 1/5 v2\\] \\[foo\\] third\" actual\n+'\n+\n+test_expect_failure 'am --keep-non-patch really keeps the non-patch part' '\n+\trm -fr .git/rebase-apply &&\n+\tgit reset --hard &&\n+\tgit checkout HEAD^ &&\n+\tgit am --keep-non-patch patch4 &&\n+\t! test -d .git/rebase-apply &&\n+\tgit cat-file commit HEAD >actual &&\n+\tgrep \"^\\[foo\\] third\" actual\n '\n \n test_expect_success 'am -3 falls back to 3-way merge' '\n-- \n1.7.9.rc0.168.g3847c\n"},{"id":"182611","messageId":"9d2d51087ec58150bdf0d25c4321dc6b7e75b192.1326710194.git.trast@student.ethz.ch","threadId":"29346","inReplyTo":"a804650f805fd8c89a843302cb92bbbdf36b8c0b.1326710194.git.trast@student.ethz.ch","subject":"[PATCH v2 2/2] mailinfo: with -b, keep space after [foo]","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2012-01-16T10:53:01Z","receivedAt":"2012-01-16T10:53:01Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"The logic for the -b mode, where [PATCH] is dropped but [foo] is not,\nsilently ate all spaces after the ].\n\nFix this by keeping the next isspace() character, if there is any.\nBeing more thorough is pointless, as the later cleanup_space() call\nwill normalize any sequence of whitespace to a single ' '.\n\nSigned-off-by: Thomas Rast <trast@student.ethz.ch>\n---\n\nUnchanged from last time.\n\n builtin/mailinfo.c |   11 ++++++++++-\n t/t4150-am.sh      |    2 +-\n 2 files changed, 11 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/mailinfo.c b/builtin/mailinfo.c\nindex bfb32b7..eaf9e15 100644\n--- a/builtin/mailinfo.c\n+++ b/builtin/mailinfo.c\n@@ -250,8 +250,17 @@ static void cleanup_subject(struct strbuf *subject)\n \t\t\t    (7 <= remove &&\n \t\t\t     memmem(subject->buf + at, remove, \"PATCH\", 5)))\n \t\t\t\tstrbuf_remove(subject, at, remove);\n-\t\t\telse\n+\t\t\telse {\n \t\t\t\tat += remove;\n+\t\t\t\t/*\n+\t\t\t\t * If the input had a space after the ], keep\n+\t\t\t\t * it.  We don't bother with finding the end of\n+\t\t\t\t * the space, since we later normalize it\n+\t\t\t\t * anyway.\n+\t\t\t\t */\n+\t\t\t\tif (isspace(subject->buf[at]))\n+\t\t\t\t\tat += 1;\n+\t\t\t}\n \t\t\tcontinue;\n \t\t}\n \t\tbreak;\ndiff --git a/t/t4150-am.sh b/t/t4150-am.sh\nindex 7e7c83c..8807b60 100755\n--- a/t/t4150-am.sh\n+++ b/t/t4150-am.sh\n@@ -262,7 +262,7 @@ test_expect_success 'am --keep really keeps the subject' '\n \tgrep \"Re: Re: Re: \\[PATCH 1/5 v2\\] \\[foo\\] third\" actual\n '\n \n-test_expect_failure 'am --keep-non-patch really keeps the non-patch part' '\n+test_expect_success 'am --keep-non-patch really keeps the non-patch part' '\n \trm -fr .git/rebase-apply &&\n \tgit reset --hard &&\n \tgit checkout HEAD^ &&\n-- \n1.7.9.rc0.168.g3847c\n"},{"id":"182813","messageId":"7vd3afidt3.fsf@alter.siamese.dyndns.org","threadId":"29346","inReplyTo":"a804650f805fd8c89a843302cb92bbbdf36b8c0b.1326710194.git.trast@student.ethz.ch","subject":"Re: [PATCH v2 1/2] am: learn passing -b to mailinfo","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-01-19T21:26:48Z","receivedAt":"2012-01-19T21:26:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Rast <trast@student.ethz.ch> writes:\n\n> git-am could pass -k to mailinfo, but not -b.  Introduce an option\n> that does so.  We change the meaning of the 'keep' state file, but are\n> careful not to cause a problem unless you downgrade in the middle of\n> an 'am' run.\n>\n> This uncovers a bug in mailinfo -b, hence the failing test.\n>\n> Signed-off-by: Thomas Rast <trast@student.ethz.ch>\n> ---\n>\n> This fixes the broken 'if', and the use of 'echo' with an argument\n> that starts with '-'.\n\nAfter re-reading the code that parses the command line options given to\n\"am\" and the previous invocation state we read from $dotest/*, however, I\nthink the way this change uses $keep makes things somewhat inconsistent\nand harder to follow.\n\nCurrently the variables are given abstract meaning (e.g. \"are we told to\nrecord to utf8? yes or no\") when we parse our command line options and\nread from the previous invocation state, and then based on that abstract\nmeaning, a later code decides what exact option we throw at the git\ncommands we invoke (e.g. \"utf8=t\" -> \"-u\").\n\nHow about doing something like this instead at least for now?  It might be\nbetter to decide when we parse our options and $dotest/* immediately what\noptions we give to the git commands we run (which your patch does but only\nto $keep option), but that kind of change (1) belongs to a separate topic\nand should be done consistently to all options, and (2) I am not convinced\nif it is necessarily a good change.\n\nThanks.\n\ndiff --git a/git-am.sh b/git-am.sh\nindex 6cdd591..8b755d9 100755\n--- a/git-am.sh\n+++ b/git-am.sh\n@@ -15,6 +15,7 @@ q,quiet         be quiet\n s,signoff       add a Signed-off-by line to the commit message\n u,utf8          recode into utf8 (default)\n k,keep          pass -k flag to git-mailinfo\n+keep-non-patch  pass -b flag to git-mailinfo\n keep-cr         pass --keep-cr flag to git-mailsplit for mbox format\n no-keep-cr      do not pass --keep-cr flag to git-mailsplit independent of am.keepcr\n c,scissors      strip everything before a scissors line\n@@ -345,6 +346,8 @@ do\n \t\tutf8= ;;\n \t-k|--keep)\n \t\tkeep=t ;;\n+\t--keep-non-patch)\n+\t\tkeep=b ;;\n \t-c|--scissors)\n \t\tscissors=t ;;\n \t--no-scissors)\n@@ -522,16 +525,25 @@ case \"$resolved\" in\n \tfi\n esac\n \n+# Now, decide what command line options we will give to the git\n+# commands we invoke, based on the result of parsing command line\n+# options and previous invocation state stored in $dotest/ files.\n+\n if test \"$(cat \"$dotest/utf8\")\" = t\n then\n \tutf8=-u\n else\n \tutf8=-n\n fi\n-if test \"$(cat \"$dotest/keep\")\" = t\n-then\n-\tkeep=-k\n-fi\n+keep=$(cat \"$dotest/keep\")\n+case \"$keep\" in\n+t)\n+\tkeep=-k ;;\n+b)\n+\tkeep=-b ;;\n+*)\n+\tkeep= ;;\n+esac\n case \"$(cat \"$dotest/keepcr\")\" in\n t)\n \tkeepcr=--keep-cr ;;\n"},{"id":"182851","messageId":"87ehuucypo.fsf@thomas.inf.ethz.ch","threadId":"29346","inReplyTo":"7vd3afidt3.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 1/2] am: learn passing -b to mailinfo","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2012-01-20T13:04:03Z","receivedAt":"2012-01-20T13:04:03Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> After re-reading the code that parses the command line options given to\n> \"am\" and the previous invocation state we read from $dotest/*, however, I\n> think the way this change uses $keep makes things somewhat inconsistent\n> and harder to follow.\n>\n> Currently the variables are given abstract meaning (e.g. \"are we told to\n> record to utf8? yes or no\") when we parse our command line options and\n> read from the previous invocation state, and then based on that abstract\n> meaning, a later code decides what exact option we throw at the git\n> commands we invoke (e.g. \"utf8=t\" -> \"-u\").\n>\n> How about doing something like this instead at least for now?  It might be\n> better to decide when we parse our options and $dotest/* immediately what\n> options we give to the git commands we run (which your patch does but only\n> to $keep option), but that kind of change (1) belongs to a separate topic\n> and should be done consistently to all options, and (2) I am not convinced\n> if it is necessarily a good change.\n\nYes, at second glance it's probably better to remain consistent.  I\ndidn't like it at first because it's layering complexity on it, but you\nare right, the existing code follows the same pattern.\n\n> diff --git a/git-am.sh b/git-am.sh\n> index 6cdd591..8b755d9 100755\n> --- a/git-am.sh\n> +++ b/git-am.sh\n> @@ -15,6 +15,7 @@ q,quiet         be quiet\n>  s,signoff       add a Signed-off-by line to the commit message\n>  u,utf8          recode into utf8 (default)\n>  k,keep          pass -k flag to git-mailinfo\n> +keep-non-patch  pass -b flag to git-mailinfo\n>  keep-cr         pass --keep-cr flag to git-mailsplit for mbox format\n>  no-keep-cr      do not pass --keep-cr flag to git-mailsplit independent of am.keepcr\n>  c,scissors      strip everything before a scissors line\n> @@ -345,6 +346,8 @@ do\n>  \t\tutf8= ;;\n>  \t-k|--keep)\n>  \t\tkeep=t ;;\n> +\t--keep-non-patch)\n> +\t\tkeep=b ;;\n>  \t-c|--scissors)\n>  \t\tscissors=t ;;\n>  \t--no-scissors)\n> @@ -522,16 +525,25 @@ case \"$resolved\" in\n>  \tfi\n>  esac\n>  \n> +# Now, decide what command line options we will give to the git\n> +# commands we invoke, based on the result of parsing command line\n> +# options and previous invocation state stored in $dotest/ files.\n> +\n>  if test \"$(cat \"$dotest/utf8\")\" = t\n>  then\n>  \tutf8=-u\n>  else\n>  \tutf8=-n\n>  fi\n> -if test \"$(cat \"$dotest/keep\")\" = t\n> -then\n> -\tkeep=-k\n> -fi\n> +keep=$(cat \"$dotest/keep\")\n> +case \"$keep\" in\n> +t)\n> +\tkeep=-k ;;\n> +b)\n> +\tkeep=-b ;;\n> +*)\n> +\tkeep= ;;\n> +esac\n>  case \"$(cat \"$dotest/keepcr\")\" in\n>  t)\n>  \tkeepcr=--keep-cr ;;\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"}]}