{"thread":{"id":"44784","subject":"[PATCH v2] am: add am.signoff add config variable","startedAt":"2016-12-28T18:35:30Z","lastAt":"2016-12-29T15:37:26Z","messageCount":9,"participants":["Eduardo Habkost","Stefan Beller","Andreas Schwab","Pranit Bauva"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"308463","messageId":"20161228183501.15068-1-ehabkost@redhat.com","threadId":"44784","inReplyTo":null,"subject":"[PATCH v2] am: add am.signoff add config variable","fromName":"Eduardo Habkost","fromEmail":"ehabkost@redhat.com","sentAt":"2016-12-28T18:35:01Z","receivedAt":"2016-12-28T18:35:30Z","isPatch":true,"sender":{"key":"ehabkost@redhat.com","avatar":null},"body":"git-am has options to enable --message-id and --3way by default,\nbut no option to enable --signoff by default. Add a \"am.signoff\"\nconfig option.\n\nSigned-off-by: Eduardo Habkost <ehabkost@redhat.com>\n---\nChanges v1 -> v2:\n* Added documentation to Documentation/git-am.txt and\n  Documentation/config.txt\n* Added test cases to t4150-am.sh\n---\n Documentation/config.txt |  5 +++++\n Documentation/git-am.txt |  6 ++++--\n builtin/am.c             |  2 ++\n t/t4150-am.sh            | 24 ++++++++++++++++++++++++\n 4 files changed, 35 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 30cb94610..6b2990203 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -822,6 +822,11 @@ am.keepcr::\n \tby giving `--no-keep-cr` from the command line.\n \tSee linkgit:git-am[1], linkgit:git-mailsplit[1].\n \n+am.signoff::\n+\tIf true, git-am will add a `Signed-off-by:` line to the commit\n+\tmessage. See the signoff option in linkgit:git-commit[1] for\n+\tmore information.\n+\n am.threeWay::\n \tBy default, `git am` will fail if the patch does not apply cleanly. When\n \tset to true, this setting tells `git am` to fall back on 3-way merge if\ndiff --git a/Documentation/git-am.txt b/Documentation/git-am.txt\nindex 12879e402..f22f10d40 100644\n--- a/Documentation/git-am.txt\n+++ b/Documentation/git-am.txt\n@@ -9,7 +9,7 @@ git-am - Apply a series of patches from a mailbox\n SYNOPSIS\n --------\n [verse]\n-'git am' [--signoff] [--keep] [--[no-]keep-cr] [--[no-]utf8]\n+'git am' [--[no-]signoff] [--keep] [--[no-]keep-cr] [--[no-]utf8]\n \t [--[no-]3way] [--interactive] [--committer-date-is-author-date]\n \t [--ignore-date] [--ignore-space-change | --ignore-whitespace]\n \t [--whitespace=<option>] [-C<n>] [-p<n>] [--directory=<dir>]\n@@ -32,10 +32,12 @@ OPTIONS\n \tIf you supply directories, they will be treated as Maildirs.\n \n -s::\n---signoff::\n+--[no]-signoff::\n \tAdd a `Signed-off-by:` line to the commit message, using\n \tthe committer identity of yourself.\n \tSee the signoff option in linkgit:git-commit[1] for more information.\n+\tThe `am.signoff` configuration variable can be used to specify the\n+\tdefault behaviour.  `--no-signoff` is useful to override `am.signoff`.\n \n -k::\n --keep::\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 31fb60578..d2e02334f 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -154,6 +154,8 @@ static void am_state_init(struct am_state *state, const char *dir)\n \n \tgit_config_get_bool(\"am.messageid\", &state->message_id);\n \n+\tgit_config_get_bool(\"am.signoff\", &state->signoff);\n+\n \tstate->scissors = SCISSORS_UNSET;\n \n \targv_array_init(&state->git_apply_opts);\ndiff --git a/t/t4150-am.sh b/t/t4150-am.sh\nindex 89a5bacac..41b5481c9 100755\n--- a/t/t4150-am.sh\n+++ b/t/t4150-am.sh\n@@ -479,6 +479,30 @@ test_expect_success 'am --signoff adds Signed-off-by: line' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success '--no-signoff overrides am.signoff' '\n+\trm -fr .git/rebase-apply &&\n+\tgit reset --hard first &&\n+\ttest_config am.signoff true &&\n+\tgit am --no-signoff <patch2 &&\n+\tprintf \"%s\\n\" \"$signoff\" >expected &&\n+\tgit cat-file commit HEAD^ | grep \"Signed-off-by:\" >actual &&\n+\ttest $(git cat-file commit HEAD | grep -c \"Signed-off-by:\") -eq 0\n+'\n+\n+test_expect_success 'am.signoff adds Signed-off-by: line' '\n+\trm -fr .git/rebase-apply &&\n+\tgit reset --hard first &&\n+\ttest_config am.signoff true &&\n+\tgit am <patch2 &&\n+\tprintf \"%s\\n\" \"$signoff\" >expected &&\n+\techo \"Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\" >>expected &&\n+\tgit cat-file commit HEAD^ | grep \"Signed-off-by:\" >actual &&\n+\ttest_cmp expected actual &&\n+\techo \"Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\" >expected &&\n+\tgit cat-file commit HEAD | grep \"Signed-off-by:\" >actual &&\n+\ttest_cmp expected actual\n+'\n+\n test_expect_success 'am stays in branch' '\n \techo refs/heads/master2 >expected &&\n \tgit symbolic-ref HEAD >actual &&\n-- \n2.11.0.259.g40922b1\n\n"},{"id":"308464","messageId":"CAGZ79kaBpC5ym2N_fMZHDmL4gGpU8pFAsupCE4aTdENh+=z72g@mail.gmail.com","threadId":"44784","inReplyTo":"20161228183501.15068-1-ehabkost@redhat.com","subject":"Re: [PATCH v2] am: add am.signoff add config variable","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-12-28T18:51:28Z","receivedAt":"2016-12-28T18:51:46Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Dec 28, 2016 at 10:35 AM, Eduardo Habkost <ehabkost@redhat.com> wrote:\n> git-am has options to enable --message-id and --3way by default,\n> but no option to enable --signoff by default. Add a \"am.signoff\"\n> config option.\n>\n> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>\n> ---\n> Changes v1 -> v2:\n> * Added documentation to Documentation/git-am.txt and\n>   Documentation/config.txt\n> * Added test cases to t4150-am.sh\n\nThanks!\nDocumentation and code looks good to me, for the test a small nit below.\n\n> +test_expect_success '--no-signoff overrides am.signoff' '\n> +       rm -fr .git/rebase-apply &&\n> +       git reset --hard first &&\n> +       test_config am.signoff true &&\n> +       git am --no-signoff <patch2 &&\n> +       printf \"%s\\n\" \"$signoff\" >expected &&\n\n\"expected\" is never read in this test, so we can omit this line?\n\n> +       git cat-file commit HEAD^ | grep \"Signed-off-by:\" >actual &&\n\nSo we check if the previous commit is not tampered with,\n\n> +       test $(git cat-file commit HEAD | grep -c \"Signed-off-by:\") -eq 0\n\nand then we check if the top most commit has zero occurrences\nfor lines grepped for sign off. That certainly works, but took me a\nwhile to understand (TIL about -c in grep :).\n\nAnother way that to write this check, that Git regulars may be more used to is:\n\n    git cat-file commit HEAD | grep \"Signed-off-by:\" >actual\n    test_must_be_empty actual\n\nI would have suggested to grep for $signoff instead of \"Signed-off-by:\",\nbut it turns out being fuzzy here is better and would also catch e.g.\na broken sign off.\n\n> +test_expect_success 'am.signoff adds Signed-off-by: line' '\n> +       rm -fr .git/rebase-apply &&\n> +       git reset --hard first &&\n> +       test_config am.signoff true &&\n> +       git am <patch2 &&\n> +       printf \"%s\\n\" \"$signoff\" >expected &&\n> +       echo \"Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\" >>expected &&\n> +       git cat-file commit HEAD^ | grep \"Signed-off-by:\" >actual &&\n> +       test_cmp expected actual &&\n> +       echo \"Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\" >expected &&\n> +       git cat-file commit HEAD | grep \"Signed-off-by:\" >actual &&\n> +       test_cmp expected actual\n> +'\n\nThis test looks good to me,\n\nThanks,\nStefan\n"},{"id":"308466","messageId":"871swrg9dh.fsf@linux-m68k.org","threadId":"44784","inReplyTo":"20161228183501.15068-1-ehabkost@redhat.com","subject":"Re: [PATCH v2] am: add am.signoff add config variable","fromName":"Andreas Schwab","fromEmail":"schwab@linux-m68k.org","sentAt":"2016-12-28T19:07:54Z","receivedAt":"2016-12-28T19:08:46Z","isPatch":true,"sender":{"key":"schwab@linux-m68k.org","avatar":"https://avatars.githubusercontent.com/u/2175493?v=4"},"body":"On Dez 28 2016, Eduardo Habkost <ehabkost@redhat.com> wrote:\n\n> diff --git a/Documentation/git-am.txt b/Documentation/git-am.txt\n> index 12879e402..f22f10d40 100644\n> --- a/Documentation/git-am.txt\n> +++ b/Documentation/git-am.txt\n> @@ -9,7 +9,7 @@ git-am - Apply a series of patches from a mailbox\n>  SYNOPSIS\n>  --------\n>  [verse]\n> -'git am' [--signoff] [--keep] [--[no-]keep-cr] [--[no-]utf8]\n> +'git am' [--[no-]signoff] [--keep] [--[no-]keep-cr] [--[no-]utf8]\n>  \t [--[no-]3way] [--interactive] [--committer-date-is-author-date]\n>  \t [--ignore-date] [--ignore-space-change | --ignore-whitespace]\n>  \t [--whitespace=<option>] [-C<n>] [-p<n>] [--directory=<dir>]\n> @@ -32,10 +32,12 @@ OPTIONS\n>  \tIf you supply directories, they will be treated as Maildirs.\n>  \n>  -s::\n> ---signoff::\n> +--[no]-signoff::\n\nThat should be --[no-]signoff, as in the synopsis.\n\nAndreas.\n\n-- \nAndreas Schwab, schwab@linux-m68k.org\nGPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5\n\"And now for something completely different.\"\n"},{"id":"308467","messageId":"20161228191142.GF3441@thinpad.lan.raisama.net","threadId":"44784","inReplyTo":"CAGZ79kaBpC5ym2N_fMZHDmL4gGpU8pFAsupCE4aTdENh+=z72g@mail.gmail.com","subject":"Re: [PATCH v2] am: add am.signoff add config variable","fromName":"Eduardo Habkost","fromEmail":"ehabkost@redhat.com","sentAt":"2016-12-28T19:11:42Z","receivedAt":"2016-12-28T19:11:48Z","isPatch":true,"sender":{"key":"ehabkost@redhat.com","avatar":null},"body":"On Wed, Dec 28, 2016 at 10:51:28AM -0800, Stefan Beller wrote:\n> On Wed, Dec 28, 2016 at 10:35 AM, Eduardo Habkost <ehabkost@redhat.com> wrote:\n> > git-am has options to enable --message-id and --3way by default,\n> > but no option to enable --signoff by default. Add a \"am.signoff\"\n> > config option.\n> >\n> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>\n> > ---\n> > Changes v1 -> v2:\n> > * Added documentation to Documentation/git-am.txt and\n> >   Documentation/config.txt\n> > * Added test cases to t4150-am.sh\n> \n> Thanks!\n> Documentation and code looks good to me, for the test a small nit below.\n> \n> > +test_expect_success '--no-signoff overrides am.signoff' '\n> > +       rm -fr .git/rebase-apply &&\n> > +       git reset --hard first &&\n> > +       test_config am.signoff true &&\n> > +       git am --no-signoff <patch2 &&\n> > +       printf \"%s\\n\" \"$signoff\" >expected &&\n> \n> \"expected\" is never read in this test, so we can omit this line?\n> \n\nOops, I have deleted a \"test_cmp expected actual\" line by mistake.\n\n> > +       git cat-file commit HEAD^ | grep \"Signed-off-by:\" >actual &&\n> \n> So we check if the previous commit is not tampered with,\n\nWe do, but only if we have a \"test_cmp expected actual\" line\nhere.\n\nFixed by:\n\ndiff --git a/t/t4150-am.sh b/t/t4150-am.sh\nindex 41b5481c9..d4b6a832f 100755\n--- a/t/t4150-am.sh\n+++ b/t/t4150-am.sh\n@@ -486,6 +486,7 @@ test_expect_success '--no-signoff overrides am.signoff' '\n \tgit am --no-signoff <patch2 &&\n \tprintf \"%s\\n\" \"$signoff\" >expected &&\n \tgit cat-file commit HEAD^ | grep \"Signed-off-by:\" >actual &&\n+\ttest_cmp expected actual &&\n \ttest $(git cat-file commit HEAD | grep -c \"Signed-off-by:\") -eq 0\n '\n \n> \n> > +       test $(git cat-file commit HEAD | grep -c \"Signed-off-by:\") -eq 0\n> \n> and then we check if the top most commit has zero occurrences\n> for lines grepped for sign off. That certainly works, but took me a\n> while to understand (TIL about -c in grep :).\n> \n> Another way that to write this check, that Git regulars may be more used to is:\n> \n>     git cat-file commit HEAD | grep \"Signed-off-by:\" >actual\n>     test_must_be_empty actual\n\ntest_must_be_empty is what I was looking for. But if I do this:\n\ntest_expect_success '--no-signoff overrides am.signoff' '\n\trm -fr .git/rebase-apply &&\n\tgit reset --hard first &&\n\ttest_config am.signoff true &&\n\tgit am --no-signoff <patch2 &&\n\tprintf \"%s\\n\" \"$signoff\" >expected &&\n\tgit cat-file commit HEAD^ | grep \"Signed-off-by:\" >actual &&\n\ttest_cmp expected actual &&\n\tgit cat-file commit HEAD | grep \"Signed-off-by:\" >actual &&\n\ttest_must_be_empty actual\n'\n\nThe test fails because the second \"grep\" command returns a\nnon-zero exit code. Any suggestions to avoid that problem in a\nmore idiomatic way?\n\n> \n> I would have suggested to grep for $signoff instead of \"Signed-off-by:\",\n> but it turns out being fuzzy here is better and would also catch e.g.\n> a broken sign off.\n\nYes, I want to ensure no extra Signed-off-by line is present\nexcept for $signof (that is already present in the original\ne-mail).\n\n> \n> > +test_expect_success 'am.signoff adds Signed-off-by: line' '\n> > +       rm -fr .git/rebase-apply &&\n> > +       git reset --hard first &&\n> > +       test_config am.signoff true &&\n> > +       git am <patch2 &&\n> > +       printf \"%s\\n\" \"$signoff\" >expected &&\n> > +       echo \"Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\" >>expected &&\n> > +       git cat-file commit HEAD^ | grep \"Signed-off-by:\" >actual &&\n> > +       test_cmp expected actual &&\n> > +       echo \"Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\" >expected &&\n> > +       git cat-file commit HEAD | grep \"Signed-off-by:\" >actual &&\n> > +       test_cmp expected actual\n> > +'\n> \n> This test looks good to me,\n> \n> Thanks,\n> Stefan\n\n-- \nEduardo\n"},{"id":"308468","messageId":"20161228191200.GG3441@thinpad.lan.raisama.net","threadId":"44784","inReplyTo":"871swrg9dh.fsf@linux-m68k.org","subject":"Re: [PATCH v2] am: add am.signoff add config variable","fromName":"Eduardo Habkost","fromEmail":"ehabkost@redhat.com","sentAt":"2016-12-28T19:12:00Z","receivedAt":"2016-12-28T19:12:06Z","isPatch":true,"sender":{"key":"ehabkost@redhat.com","avatar":null},"body":"On Wed, Dec 28, 2016 at 08:07:54PM +0100, Andreas Schwab wrote:\n> On Dez 28 2016, Eduardo Habkost <ehabkost@redhat.com> wrote:\n> \n> > diff --git a/Documentation/git-am.txt b/Documentation/git-am.txt\n> > index 12879e402..f22f10d40 100644\n> > --- a/Documentation/git-am.txt\n> > +++ b/Documentation/git-am.txt\n> > @@ -9,7 +9,7 @@ git-am - Apply a series of patches from a mailbox\n> >  SYNOPSIS\n> >  --------\n> >  [verse]\n> > -'git am' [--signoff] [--keep] [--[no-]keep-cr] [--[no-]utf8]\n> > +'git am' [--[no-]signoff] [--keep] [--[no-]keep-cr] [--[no-]utf8]\n> >  \t [--[no-]3way] [--interactive] [--committer-date-is-author-date]\n> >  \t [--ignore-date] [--ignore-space-change | --ignore-whitespace]\n> >  \t [--whitespace=<option>] [-C<n>] [-p<n>] [--directory=<dir>]\n> > @@ -32,10 +32,12 @@ OPTIONS\n> >  \tIf you supply directories, they will be treated as Maildirs.\n> >  \n> >  -s::\n> > ---signoff::\n> > +--[no]-signoff::\n> \n> That should be --[no-]signoff, as in the synopsis.\n\nThanks for catching it. I will fix it in v3.\n\n-- \nEduardo\n"},{"id":"308469","messageId":"20161228191928.GH3441@thinpad.lan.raisama.net","threadId":"44784","inReplyTo":"20161228191142.GF3441@thinpad.lan.raisama.net","subject":"Re: [PATCH v2] am: add am.signoff add config variable","fromName":"Eduardo Habkost","fromEmail":"ehabkost@redhat.com","sentAt":"2016-12-28T19:19:28Z","receivedAt":"2016-12-28T19:19:36Z","isPatch":true,"sender":{"key":"ehabkost@redhat.com","avatar":null},"body":"On Wed, Dec 28, 2016 at 05:11:42PM -0200, Eduardo Habkost wrote:\n> On Wed, Dec 28, 2016 at 10:51:28AM -0800, Stefan Beller wrote:\n> > On Wed, Dec 28, 2016 at 10:35 AM, Eduardo Habkost <ehabkost@redhat.com> wrote:\n[...]\n> > > +       test $(git cat-file commit HEAD | grep -c \"Signed-off-by:\") -eq 0\n> > \n> > and then we check if the top most commit has zero occurrences\n> > for lines grepped for sign off. That certainly works, but took me a\n> > while to understand (TIL about -c in grep :).\n> > \n> > Another way that to write this check, that Git regulars may be more used to is:\n> > \n> >     git cat-file commit HEAD | grep \"Signed-off-by:\" >actual\n> >     test_must_be_empty actual\n> \n> test_must_be_empty is what I was looking for. But if I do this:\n> \n> test_expect_success '--no-signoff overrides am.signoff' '\n> \trm -fr .git/rebase-apply &&\n> \tgit reset --hard first &&\n> \ttest_config am.signoff true &&\n> \tgit am --no-signoff <patch2 &&\n> \tprintf \"%s\\n\" \"$signoff\" >expected &&\n> \tgit cat-file commit HEAD^ | grep \"Signed-off-by:\" >actual &&\n> \ttest_cmp expected actual &&\n> \tgit cat-file commit HEAD | grep \"Signed-off-by:\" >actual &&\n> \ttest_must_be_empty actual\n> '\n> \n> The test fails because the second \"grep\" command returns a\n> non-zero exit code. Any suggestions to avoid that problem in a\n> more idiomatic way?\n\nI just found out that \"test_must_fail grep ...\" is a common\nidiom, so what about:\n\ntest_expect_success '--no-signoff overrides am.signoff' '\n\trm -fr .git/rebase-apply &&\n\tgit reset --hard first &&\n\ttest_config am.signoff true &&\n\tgit am --no-signoff <patch2 &&\n\tprintf \"%s\\n\" \"$signoff\" >expected &&\n\tgit cat-file commit HEAD^ | grep \"Signed-off-by:\" >actual &&\n\ttest_cmp expected actual &&\n\tgit cat-file commit HEAD | test_must_fail grep -q \"Signed-off-by:\"\n'\n\n-- \nEduardo\n"},{"id":"308470","messageId":"CAGZ79kaNd2EAjRC=U3_mXB2Deo4oBcUfBHWXPSDfa-vUtBogkg@mail.gmail.com","threadId":"44784","inReplyTo":"20161228191928.GH3441@thinpad.lan.raisama.net","subject":"Re: [PATCH v2] am: add am.signoff add config variable","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-12-28T19:24:13Z","receivedAt":"2016-12-28T19:24:32Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Dec 28, 2016 at 11:19 AM, Eduardo Habkost <ehabkost@redhat.com> wrote:\n> On Wed, Dec 28, 2016 at 05:11:42PM -0200, Eduardo Habkost wrote:\n>> On Wed, Dec 28, 2016 at 10:51:28AM -0800, Stefan Beller wrote:\n>> > On Wed, Dec 28, 2016 at 10:35 AM, Eduardo Habkost <ehabkost@redhat.com> wrote:\n> [...]\n>> > > +       test $(git cat-file commit HEAD | grep -c \"Signed-off-by:\") -eq 0\n>> >\n>> > and then we check if the top most commit has zero occurrences\n>> > for lines grepped for sign off. That certainly works, but took me a\n>> > while to understand (TIL about -c in grep :).\n>> >\n>> > Another way that to write this check, that Git regulars may be more used to is:\n>> >\n>> >     git cat-file commit HEAD | grep \"Signed-off-by:\" >actual\n>> >     test_must_be_empty actual\n>>\n>> test_must_be_empty is what I was looking for. But if I do this:\n>>\n>> test_expect_success '--no-signoff overrides am.signoff' '\n>>       rm -fr .git/rebase-apply &&\n>>       git reset --hard first &&\n>>       test_config am.signoff true &&\n>>       git am --no-signoff <patch2 &&\n>>       printf \"%s\\n\" \"$signoff\" >expected &&\n>>       git cat-file commit HEAD^ | grep \"Signed-off-by:\" >actual &&\n>>       test_cmp expected actual &&\n>>       git cat-file commit HEAD | grep \"Signed-off-by:\" >actual &&\n>>       test_must_be_empty actual\n>> '\n>>\n>> The test fails because the second \"grep\" command returns a\n>> non-zero exit code. Any suggestions to avoid that problem in a\n>> more idiomatic way?\n>\n> I just found out that \"test_must_fail grep ...\" is a common\n> idiom, so what about:\n\nUh, no please.\n\ntest_must_fail is supposed to be used for commands to be tested, i.e.\ngit commands as this is the git test suite. :)\ntest_must_fail checks, e.g. that the failing command \"properly\" fails\ninstead of bye-bye-segfault. And grep would never do this. (In this\nworld we assume everything to be perfect except git itself)\n\nFor grep just use !\n\n    git cat-file commit HEAD >actual &&\n    ! grep \"Signed-off-by:\" actual\n\n$ git grep \"test_must_fail grep\" returns 20 occurrences, so in case you're\nthat would be a good cleanup patch (if you're interested in such things).\n\nThanks,\nStefan\n"},{"id":"308481","messageId":"CAFZEwPOPMrCXTc+SMhjGSnPKLmefcde4MgJsz7n5rBApACZOug@mail.gmail.com","threadId":"44784","inReplyTo":"20161228191928.GH3441@thinpad.lan.raisama.net","subject":"Re: [PATCH v2] am: add am.signoff add config variable","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-12-29T07:59:33Z","receivedAt":"2016-12-29T07:59:39Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"Hey Eduardo,\n\nOn Thu, Dec 29, 2016 at 12:49 AM, Eduardo Habkost <ehabkost@redhat.com> wrote:\n>> test_expect_success '--no-signoff overrides am.signoff' '\n>>       rm -fr .git/rebase-apply &&\n>>       git reset --hard first &&\n>>       test_config am.signoff true &&\n>>       git am --no-signoff <patch2 &&\n>>       printf \"%s\\n\" \"$signoff\" >expected &&\n>>       git cat-file commit HEAD^ | grep \"Signed-off-by:\" >actual &&\n>>       test_cmp expected actual &&\n>>       git cat-file commit HEAD | grep \"Signed-off-by:\" >actual &&\n>>       test_must_be_empty actual\n>> '\n>>\n>> The test fails because the second \"grep\" command returns a\n>> non-zero exit code. Any suggestions to avoid that problem in a\n>> more idiomatic way?\n>\n> I just found out that \"test_must_fail grep ...\" is a common\n> idiom, so what about:\n\nIs there any particular reason to use \"grep\" instead of \"test_cmp\"? To\ncheck for non-zero error code, you can always use \"! test_cmp\".\n\nRegards,\nPranit Bauva\n"},{"id":"308493","messageId":"20161229153709.GA23595@thinpad.lan.raisama.net","threadId":"44784","inReplyTo":"CAFZEwPOPMrCXTc+SMhjGSnPKLmefcde4MgJsz7n5rBApACZOug@mail.gmail.com","subject":"Re: [PATCH v2] am: add am.signoff add config variable","fromName":"Eduardo Habkost","fromEmail":"ehabkost@redhat.com","sentAt":"2016-12-29T15:37:09Z","receivedAt":"2016-12-29T15:37:26Z","isPatch":true,"sender":{"key":"ehabkost@redhat.com","avatar":null},"body":"On Thu, Dec 29, 2016 at 01:29:33PM +0530, Pranit Bauva wrote:\n> Hey Eduardo,\n> \n> On Thu, Dec 29, 2016 at 12:49 AM, Eduardo Habkost <ehabkost@redhat.com> wrote:\n> >> test_expect_success '--no-signoff overrides am.signoff' '\n> >>       rm -fr .git/rebase-apply &&\n> >>       git reset --hard first &&\n> >>       test_config am.signoff true &&\n> >>       git am --no-signoff <patch2 &&\n> >>       printf \"%s\\n\" \"$signoff\" >expected &&\n> >>       git cat-file commit HEAD^ | grep \"Signed-off-by:\" >actual &&\n> >>       test_cmp expected actual &&\n> >>       git cat-file commit HEAD | grep \"Signed-off-by:\" >actual &&\n> >>       test_must_be_empty actual\n> >> '\n> >>\n> >> The test fails because the second \"grep\" command returns a\n> >> non-zero exit code. Any suggestions to avoid that problem in a\n> >> more idiomatic way?\n> >\n> > I just found out that \"test_must_fail grep ...\" is a common\n> > idiom, so what about:\n> \n> Is there any particular reason to use \"grep\" instead of \"test_cmp\"? To\n> check for non-zero error code, you can always use \"! test_cmp\".\n\nThe test code is checking only the \"Signed-off-by\" lines, not the\nwhole commit message. \"test_cmp\" would require recovering the\nentire contents of the original commit message, which would add\ncomplexity to the test code.\n\n-- \nEduardo\n"}]}