{"thread":{"id":"39932","subject":"\"git am\" and then \"git am -3\" regression?","startedAt":"2015-07-24T17:48:18Z","lastAt":"2015-08-12T04:16:02Z","messageCount":26,"participants":["Junio C Hamano","Jeff King","Paul Tan","Matthieu Moy","Eric Sunshine","Johannes Schindelin"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"266732","messageId":"xmqqr3nxmopp.fsf@gitster.dls.corp.google.com","threadId":"39932","inReplyTo":null,"subject":"\"git am\" and then \"git am -3\" regression?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-07-24T17:48:18Z","receivedAt":"2015-07-24T17:48:18Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Hmm, there seems to be some glitches around running \"am -3\"\nafter a failed \"am\" between 'maint' and 'master'.\n\nWhen I try the following sequence, the 'am' from 'maint' succeeds,\nbut 'am' in 'master' fails:\n\n * Save Eric's \"minor documetation improvements\" $gmane/274537\n to a file. \n\n * \"git checkout e177995\" (that's \"next^0\") and then apply them with\n \"git am\" (no -3 necessary).\n\n * \"git checkout 272be14\" (that's \"es/worktree-add-cleanup^0\") and\n then apply them with \"git am\" (without -3).\n\n This is expected to stop at 2/6, as the context has changed\n between 272be14 and the tip of 'next'.\n\n * \"git am -3\". This should restart and resolve cleanly.\n\nReverting d96a275b91bae1800cd43be0651e886e7e042a17 seems to fix it,\nso that is what I'll do for 2.5 final.\n\nI think Paul's builtin-am has the same issues, that would need a\nseparate fix.\n\ncommit d96a275b91bae1800cd43be0651e886e7e042a17\nAuthor: Remi Lespinet \nDate: Thu Jun 4 17:04:55 2015 +0200\n\n git-am: add am.threeWay config variable\n \n Add the am.threeWay configuration variable to use the -3 or --3way\n option of git am by default. When am.threeway is set and not desired\n for a specific git am command, the --no-3way option can be used to\n override it.\n"},{"id":"266735","messageId":"20150724180921.GA17730@peff.net","threadId":"39932","inReplyTo":"xmqqr3nxmopp.fsf@gitster.dls.corp.google.com","subject":"Re: \"git am\" and then \"git am -3\" regression?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-07-24T18:09:21Z","receivedAt":"2015-07-24T18:09:21Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 24, 2015 at 10:48:18AM -0700, Junio C Hamano wrote:\n\n> Hmm, there seems to be some glitches around running \"am -3\"\n> after a failed \"am\" between 'maint' and 'master'.\n> \n> When I try the following sequence, the 'am' from 'maint' succeeds,\n> but 'am' in 'master' fails:\n> \n> * Save Eric's \"minor documetation improvements\" $gmane/274537\n> to a file. \n> \n> * \"git checkout e177995\" (that's \"next^0\") and then apply them with\n> \"git am\" (no -3 necessary).\n> \n> * \"git checkout 272be14\" (that's \"es/worktree-add-cleanup^0\") and\n> then apply them with \"git am\" (without -3).\n> \n> This is expected to stop at 2/6, as the context has changed\n> between 272be14 and the tip of 'next'.\n> \n> * \"git am -3\". This should restart and resolve cleanly.\n\nThanks for diagnosing. This bit me the other day, but I hadn't had time\nto look at it yet (and I \"am\" a lot less than you do, I imagine).\n\n> Reverting d96a275b91bae1800cd43be0651e886e7e042a17 seems to fix it,\n> so that is what I'll do for 2.5 final.\n\nYeah, I think this hunk is to blame (though I just read the code and did not\ntest):\n\n@@ -658,6 +665,8 @@ fi\n if test \"$(cat \"$dotest/threeway\")\" = t\n then\n threeway=t\n+else\n+ threeway=f\n fi\n\nIt comes after the command-line option parsing, so it overrides our option (I\nthink that running \"git am -3\" followed by \"git am --no-3way\" would have the\nsame problem). It cannot just check whether $threeway is unset, though, as it\nmay have come from the config. We'd need a separate variable, the way the code\nis ordered now.\n\nIdeally the code would just be ordered as:\n\n - load config from git-config\n\n - override that with defaults inherited from a previous run\n\n - override that with command-line parsing\n\nbut I don't know if there are other ordering gotchas that would break.\nIt does look like that is how Paul's builtin/am.c does it, which makes\nme think it might not be broken. It's also possibly I've horribly\nmisdiagnosed the bug. ;)\n\n-Peff\n"},{"id":"266784","messageId":"CACRoPnR=DSETucY78Xo0RNxHKkqDnTCYFvHsSzWAG7X7z3_DKQ@mail.gmail.com","threadId":"39932","inReplyTo":"20150724180921.GA17730@peff.net","subject":"Re: \"git am\" and then \"git am -3\" regression?","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-07-26T05:03:59Z","receivedAt":"2015-07-26T05:03:59Z","isPatch":false,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"On Sat, Jul 25, 2015 at 2:09 AM, Jeff King wrote:\n> Yeah, I think this hunk is to blame (though I just read the code and did not\n> test):\n>\n> @@ -658,6 +665,8 @@ fi\n> if test \"$(cat \"$dotest/threeway\")\" = t\n> then\n> threeway=t\n> +else\n> + threeway=f\n> fi\n>\n> It comes after the command-line option parsing, so it overrides our option (I\n> think that running \"git am -3\" followed by \"git am --no-3way\" would have the\n> same problem). It cannot just check whether $threeway is unset, though, as it\n> may have come from the config.\n\nThanks for the detailed analysis, I completely agree. Note that the\ncode that handles the --message-id option somewhat handles the case\nwhere $messageid is unset:\n\ncase \"$(cat \"$dotest/messageid\")\" in\nt)\n messageid=-m ;;\nf)\n messageid= ;;\nesac\n\nHowever, it still does not handle \"git am --no-message-id\" followed by\n\"git am --message-id\", or \"git -c am.messageid=true am\" followed by\n\"git am --no-message-id\". I think the same thing occurs for\n--scissors/--no-scissors, as well as the git-apply options as well.\n\nThe real problem is that the state directory loading code comes after\nthe config loading and option parsing code, and thus overrides any\nvariables set.\n\n> We'd need a separate variable, the way the code\n> is ordered now.\n\nIf we are just fixing --3way, adding one extra variable won't be that\nbad. However, I think that if we are using this approach to fix all of\nthe options, then it would introduce too much code complexity.\n\n> Ideally the code would just be ordered as:\n>\n> - load config from git-config\n>\n> - override that with defaults inherited from a previous run\n>\n> - override that with command-line parsing\n\nSo I'm more in favor of this solution. It's feels much more natural to\nme, rather than attempting to workaround the existing code structure.\n\n> but I don't know if there are other ordering gotchas that would break.\n\nFor the C code, there won't be any problem, but yeah, fixing it in\ngit-am.sh might need a bit more effort.\n\n> It does look like that is how Paul's builtin/am.c does it, which makes\n> me think it might not be broken. It's also possibly I've horribly\n> misdiagnosed the bug. ;)\n\nNah, it follows the same structure as git-am.sh and so will exhibit\nthe same behavior. It currently does something like this:\n\n1. am_state_init() (config settings are loaded)\n2. parse_options()\n3. if (am_in_progress()) am_load(); else am_setup();\n\nSo it would be quite trivial to change the control flow such that it is:\n\n1. am_state_init()\n2. if (am_in_progress()) am_load()\n3. parse_options();\n4 if (!am_in_progress()) am_setup()\n\nThe next question is, should any options set on the command-line\naffect subsequent invocations? If yes, then the control flow will be\nlike:\n\n1. am_state_init();\n2. if (am_in_progress()) am_load();\n3. parse_options();\n4. if (am_in_progress()) am_save_opts(); else am_setup();\n\nwhere am_save_opts() will write the updated variables back to the\nstate directory. What do you think?\n\nSince the builtin-am series is in 'next' already, and the fix in C is\nstraightforward, to save time and effort I'm wondering if we could\njust do \"am.threeWay patch -> builtin-am series -> bugfix patch in C\".\nMy university term is starting soon so I may not have so much time,\nbut I'll see what I can do :-/\n\nJunio, how do you want to proceed?\n\nThanks,\nPaul\n"},{"id":"266788","messageId":"20150726052100.GA31790@peff.net","threadId":"39932","inReplyTo":"CACRoPnR=DSETucY78Xo0RNxHKkqDnTCYFvHsSzWAG7X7z3_DKQ@mail.gmail.com","subject":"Re: \"git am\" and then \"git am -3\" regression?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-07-26T05:21:00Z","receivedAt":"2015-07-26T05:21:00Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jul 26, 2015 at 01:03:59PM +0800, Paul Tan wrote:\n\n> > Ideally the code would just be ordered as:\n> >\n> > - load config from git-config\n> >\n> > - override that with defaults inherited from a previous run\n> >\n> > - override that with command-line parsing\n> \n> So I'm more in favor of this solution. It's feels much more natural to\n> me, rather than attempting to workaround the existing code structure.\n\nYeah, I really prefer it, too. I just didn't know if there would be\nother confusing fallouts from changing the ordering. But since you have\nbeen deep in this code recently, I trust your judgement. :)\n\n> > It does look like that is how Paul's builtin/am.c does it, which makes\n> > me think it might not be broken. It's also possibly I've horribly\n> > misdiagnosed the bug. ;)\n> \n> Nah, it follows the same structure as git-am.sh and so will exhibit\n> the same behavior. It currently does something like this:\n> \n> 1. am_state_init() (config settings are loaded)\n> 2. parse_options()\n> 3. if (am_in_progress()) am_load(); else am_setup();\n\nAh, right. I took the am_state_init() to be the part where we loaded the\nexisting options, and didn't notice the later am_load().\n\n> The next question is, should any options set on the command-line\n> affect subsequent invocations? If yes, then the control flow will be\n> like:\n> \n> 1. am_state_init();\n> 2. if (am_in_progress()) am_load();\n> 3. parse_options();\n> 4. if (am_in_progress()) am_save_opts(); else am_setup();\n> \n> where am_save_opts() will write the updated variables back to the\n> state directory. What do you think?\n\nI don't think we need to go that direction. The usual thought process\n(mine, anyway) is:\n\n 1. I want to apply a series, and I want to use option A.\n\n 2. Oops, one of the patches didn't apply. Let's retry it with option B\n (usually \"-3\").\n\n 3. OK, that worked. Now let's try the rest of the patches.\n\nI wouldn't expect in step 3 to have options from step 2 persist. That\nwas just about wiggling that _one_ patch. Whereas options from step 1\nare about the whole series.\n\n> Since the builtin-am series is in 'next' already, and the fix in C is\n> straightforward, to save time and effort I'm wondering if we could\n> just do \"am.threeWay patch -> builtin-am series -> bugfix patch in C\".\n> My university term is starting soon so I may not have so much time,\n> but I'll see what I can do :-/\n\nYeah, having to worry about two implementations of \"git am\" is a real\npain. If we are close on merging the builtin version, it makes sense to\nme to hold off on the am.threeway feature until that is merged. Trying\nto fix the ordering of the script that is going away isn't a good use of\nanybody's time.\n\n-Peff\n"},{"id":"266816","messageId":"vpqmvyiauns.fsf@anie.imag.fr","threadId":"39932","inReplyTo":"20150726052100.GA31790@peff.net","subject":"Re: \"git am\" and then \"git am -3\" regression?","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2015-07-27T08:09:43Z","receivedAt":"2015-07-27T08:09:43Z","isPatch":false,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Jeff King writes:\n\n> Yeah, having to worry about two implementations of \"git am\" is a real\n> pain. If we are close on merging the builtin version, it makes sense to\n> me to hold off on the am.threeway feature until that is merged. Trying\n> to fix the ordering of the script that is going away isn't a good use of\n> anybody's time.\n\nSo, the best option seems to be:\n\n1) Revert d96a275 (git-am: add am.threeWay config variable, 2015-06-04)\n\n2) Include the C port of d96a275 together with tests and docs verbatim\n from d96a275 into Paul's series.\n\nActually, doing 1) is probably a good idea anyway, there's no reason to\nhold the release for such minor feature.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"266817","messageId":"20150727083244.GA334@peff.net","threadId":"39932","inReplyTo":"vpqmvyiauns.fsf@anie.imag.fr","subject":"Re: \"git am\" and then \"git am -3\" regression?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-07-27T08:32:44Z","receivedAt":"2015-07-27T08:32:44Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jul 27, 2015 at 10:09:43AM +0200, Matthieu Moy wrote:\n\n> > Yeah, having to worry about two implementations of \"git am\" is a real\n> > pain. If we are close on merging the builtin version, it makes sense to\n> > me to hold off on the am.threeway feature until that is merged. Trying\n> > to fix the ordering of the script that is going away isn't a good use of\n> > anybody's time.\n> \n> So, the best option seems to be:\n> \n> 1) Revert d96a275 (git-am: add am.threeWay config variable, 2015-06-04)\n> \n> 2) Include the C port of d96a275 together with tests and docs verbatim\n> from d96a275 into Paul's series.\n> \n> Actually, doing 1) is probably a good idea anyway, there's no reason to\n> hold the release for such minor feature.\n\nI think step 1 is done already for v2.5.0, in 15dc5b5.\n\nWe _could_ fix it in the script version for the upcoming cycle, but if\nwe are merging builtin-am during this cycle, I do not see the point.\n\n-Peff\n"},{"id":"266832","messageId":"xmqq8ua1k7fc.fsf@gitster.dls.corp.google.com","threadId":"39932","inReplyTo":"CACRoPnR=DSETucY78Xo0RNxHKkqDnTCYFvHsSzWAG7X7z3_DKQ@mail.gmail.com","subject":"Re: \"git am\" and then \"git am -3\" regression?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-07-27T14:21:27Z","receivedAt":"2015-07-27T14:21:27Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Paul Tan writes:\n\n> Junio, how do you want to proceed?\n\nI'd expect that builtin series would graduate in 2 releases from now\nat the latest, if not earlier. Let's just revert the regressing\nchange from the scripted version and have it implemented in the\nbuiltin one in the meantime. I do not think it is worth adding\nfeatures to the scripted one at this point.\n\nThanks.\n"},{"id":"266941","messageId":"20150728164311.GA1948@yoshi.chippynet.com","threadId":"39932","inReplyTo":"xmqq8ua1k7fc.fsf@gitster.dls.corp.google.com","subject":"[PATCH] am: let command-line options override saved options","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-07-28T16:43:11Z","receivedAt":"2015-07-28T16:43:11Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"When resuming, git-am ignores command-line options. For instance, when a\npatch fails to apply with \"git am patch\", subsequently running \"git am\n--3way patch\" would not cause git-am to fall back on attempting a\nthreeway merge. This occurs because by default the --3way option is\nsaved as \"false\", and the saved am options are loaded after the\ncommand-line options are parsed, thus overwriting the command-line\noptions when resuming.\n\nFix this by moving the am_load() function call before parse_options(),\nso that command-line options will override the saved am options.\n\nReported-by: Junio C Hamano \nHelped-by: Jeff King \nSigned-off-by: Paul Tan \n---\n builtin/am.c | 9 ++-\n t/t4153-am-resume-override-opts.sh | 144 +++++++++++++++++++++++++++++++++++++\n 2 files changed, 150 insertions(+), 3 deletions(-)\n create mode 100755 t/t4153-am-resume-override-opts.sh\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 1116304..8a0b0e4 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -2131,6 +2131,7 @@ int cmd_am(int argc, const char **argv, const char *prefix)\n \tint keep_cr = -1;\n \tint patch_format = PATCH_FORMAT_UNKNOWN;\n \tenum resume_mode resume = RESUME_FALSE;\n+\tint in_progress;\n \n \tconst char * const usage[] = {\n \t\tN_(\"git am [options] [(|)...]\"),\n@@ -2226,6 +2227,10 @@ int cmd_am(int argc, const char **argv, const char *prefix)\n \n \tam_state_init(&state, git_path(\"rebase-apply\"));\n \n+\tin_progress = am_in_progress(&state);\n+\tif (in_progress)\n+\t\tam_load(&state);\n+\n \targc = parse_options(argc, argv, prefix, options, usage, 0);\n \n \tif (binary >= 0)\n@@ -2238,7 +2243,7 @@ int cmd_am(int argc, const char **argv, const char *prefix)\n \tif (read_index_preload(&the_index, NULL) < 0)\n \t\tdie(_(\"failed to read the index\"));\n \n-\tif (am_in_progress(&state)) {\n+\tif (in_progress) {\n \t\t/*\n \t\t * Catch user error to feed us patches when there is a session\n \t\t * in progress:\n@@ -2256,8 +2261,6 @@ int cmd_am(int argc, const char **argv, const char *prefix)\n \n \t\tif (resume == RESUME_FALSE)\n \t\t\tresume = RESUME_APPLY;\n-\n-\t\tam_load(&state);\n \t} else {\n \t\tstruct argv_array paths = ARGV_ARRAY_INIT;\n \t\tint i;\ndiff --git a/t/t4153-am-resume-override-opts.sh b/t/t4153-am-resume-override-opts.sh\nnew file mode 100755\nindex 0000000..c49457c\n--- /dev/null\n+++ b/t/t4153-am-resume-override-opts.sh\n@@ -0,0 +1,144 @@\n+#!/bin/sh\n+\n+test_description='git-am command-line options override saved options'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'setup' '\n+\ttest_commit initial file &&\n+\ttest_commit first file &&\n+\n+\tgit checkout -b side initial &&\n+\ttest_commit side-first file &&\n+\ttest_commit side-second file &&\n+\n+\t{\n+\t\techo \"Message-Id: \" &&\n+\t\tgit format-patch --stdout -1 side-first | sed -e \"1d\"\n+\t} >side-first.patch &&\n+\t{\n+\t\tsed -ne \"1,/^\\$/p\" side-first.patch &&\n+\t\techo \"-- >8 --\" &&\n+\t\tsed -e \"1,/^\\$/d\" side-first.patch\n+\t} >side-first.scissors &&\n+\n+\t{\n+\t\techo \"Message-Id: \" &&\n+\t\tgit format-patch --stdout -1 side-second | sed -e \"1d\"\n+\t} >side-second.patch &&\n+\t{\n+\t\tsed -ne \"1,/^\\$/p\" side-second.patch &&\n+\t\techo \"-- >8 --\" &&\n+\t\tsed -e \"1,/^\\$/d\" side-second.patch\n+\t} >side-second.scissors\n+'\n+\n+test_expect_success '--3way, --no-3way' '\n+\trm -fr .git/rebase-apply &&\n+\tgit reset --hard &&\n+\tgit checkout first &&\n+\ttest_must_fail git am --3way side-first.patch side-second.patch &&\n+\ttest -n \"$(git ls-files -u)\" &&\n+\techo will-conflict >file &&\n+\tgit add file &&\n+\ttest_must_fail git am --no-3way --continue &&\n+\ttest -z \"$(git ls-files -u)\"\n+'\n+\n+test_expect_success '--no-quiet, --quiet' '\n+\trm -fr .git/rebase-apply &&\n+\tgit reset --hard &&\n+\tgit checkout first &&\n+\ttest_must_fail git am --no-quiet side-first.patch side-second.patch &&\n+\ttest_must_be_empty out &&\n+\techo side-first >file &&\n+\tgit add file &&\n+\tgit am --quiet --continue >out &&\n+\ttest_must_be_empty out\n+'\n+\n+test_expect_success '--signoff, --no-signoff' '\n+\trm -fr .git/rebase-apply &&\n+\tgit reset --hard &&\n+\tgit checkout first &&\n+\ttest_must_fail git am --signoff side-first.patch side-second.patch &&\n+\techo side-first >file &&\n+\tgit add file &&\n+\tgit am --no-signoff --continue &&\n+\n+\t# applied side-first will be signed off\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+\t# applied side-second will not be signed off\n+\ttest $(git cat-file commit HEAD | grep -c \"Signed-off-by:\") -eq 0\n+'\n+\n+test_expect_success '--keep, --no-keep' '\n+\trm -fr .git/rebase-apply &&\n+\tgit reset --hard &&\n+\tgit checkout first &&\n+\ttest_must_fail git am --keep side-first.patch side-second.patch &&\n+\techo side-first >file &&\n+\tgit add file &&\n+\tgit am --no-keep --continue &&\n+\n+\t# applied side-first will keep the subject\n+\tgit cat-file commit HEAD^ >actual &&\n+\tgrep \"^\\[PATCH\\] side-first\" actual &&\n+\n+\t# applied side-second will not have [PATCH]\n+\tgit cat-file commit HEAD >actual &&\n+\t! grep \"^\\[PATCH\\] side-second\" actual\n+'\n+\n+test_expect_success '--message-id, --no-message-id' '\n+\trm -fr .git/rebase-apply &&\n+\tgit reset --hard &&\n+\tgit checkout first &&\n+\ttest_must_fail git am --message-id side-first.patch side-second.patch &&\n+\techo side-first >file &&\n+\tgit add file &&\n+\tgit am --no-message-id --continue &&\n+\n+\t# applied side-first will have Message-Id\n+\ttest -n \"$(git cat-file commit HEAD^ | grep Message-Id)\" &&\n+\n+\t# applied side-second will not have Message-Id\n+\ttest -z \"$(git cat-file commit HEAD | grep Message-Id)\"\n+'\n+\n+test_expect_success '--scissors, --no-scissors' '\n+\trm -fr .git/rebase-apply &&\n+\tgit reset --hard &&\n+\tgit checkout first &&\n+\ttest_must_fail git am --scissors side-first.scissors side-second.scissors &&\n+\techo side-first >file &&\n+\tgit add file &&\n+\tgit am --no-scissors --continue &&\n+\n+\t# applied side-first will not have scissors line\n+\tgit cat-file commit HEAD^ >actual &&\n+\t! grep \"^-- >8 --\" actual &&\n+\n+\t# applied side-second will have scissors line\n+\tgit cat-file commit HEAD >actual &&\n+\tgrep \"^-- >8 --\" actual\n+'\n+\n+test_expect_success '--reject, --no-reject' '\n+\trm -fr .git/rebase-apply &&\n+\tgit reset --hard &&\n+\tgit checkout first &&\n+\trm -f file.rej &&\n+\ttest_must_fail git am --reject side-first.patch side-second.patch &&\n+\ttest_path_is_file file.rej &&\n+\trm -f file.rej &&\n+\techo will-conflict >file &&\n+\tgit add file &&\n+\ttest_must_fail git am --no-reject --continue &&\n+\ttest_path_is_missing file.rej\n+'\n+\n+test_done\n-- \n2.5.0.77.gd180035\n"},{"id":"266942","messageId":"xmqqio94fcu2.fsf@gitster.dls.corp.google.com","threadId":"39932","inReplyTo":"20150728164311.GA1948@yoshi.chippynet.com","subject":"Re: [PATCH] am: let command-line options override saved options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-07-28T16:48:05Z","receivedAt":"2015-07-28T16:48:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Paul Tan writes:\n\n> When resuming, git-am ignores command-line options. For instance, when a\n> patch fails to apply with \"git am patch\", subsequently running \"git am\n> --3way patch\" would not cause git-am to fall back on attempting a\n\nThe second one goes without any file argument, i.e. \"git am -3\".\n\n> threeway merge. This occurs because by default the --3way option is\n> saved as \"false\", and the saved am options are loaded after the\n> command-line options are parsed, thus overwriting the command-line\n> options when resuming.\n>\n> Fix this by moving the am_load() function call before parse_options(),\n> so that command-line options will override the saved am options.\n\nMakes sense.\n"},{"id":"266944","messageId":"xmqqegjsfbtk.fsf@gitster.dls.corp.google.com","threadId":"39932","inReplyTo":"20150728164311.GA1948@yoshi.chippynet.com","subject":"Re: [PATCH] am: let command-line options override saved options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-07-28T17:09:59Z","receivedAt":"2015-07-28T17:09:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Paul Tan writes:\n\n> diff --git a/t/t4153-am-resume-override-opts.sh b/t/t4153-am-resume-override-opts.sh\n> new file mode 100755\n> index 0000000..c49457c\n> --- /dev/null\n> +++ b/t/t4153-am-resume-override-opts.sh\n> @@ -0,0 +1,144 @@\n> +#!/bin/sh\n> +\n> +test_description='git-am command-line options override saved options'\n> +\n> +. ./test-lib.sh\n> +\n> +test_expect_success 'setup' '\n> +\ttest_commit initial file &&\n> +\ttest_commit first file &&\n> +\n> +\tgit checkout -b side initial &&\n> +\ttest_commit side-first file &&\n> +\ttest_commit side-second file &&\n> +\n> +\t{\n> +\t\techo \"Message-Id: \" &&\n> +\t\tgit format-patch --stdout -1 side-first | sed -e \"1d\"\n> +\t} >side-first.patch &&\n\nHmm, puzzled... Ah, you want to make sure Message-Id comes at the\nvery beginning, and you are going to use a single e-mail per mailbox\nso it is easier to strip the Beginning of Message marker than to\ninsert Message-Id after it. I can understand what is going on.\n\n> +\t{\n> +\t\tsed -ne \"1,/^\\$/p\" side-first.patch &&\n\nsed -e \"/^\\$/q\" would work just as well here\n\n> +\t\techo \"-- >8 --\" &&\n> +\t\tsed -e \"1,/^\\$/d\" side-first.patch\n> +\t} >side-first.scissors &&\n\nSo *.scissors version has -- >8 -- inserted at the beginning of the\nbody.\n\n> +\t{\n> +\t\techo \"Message-Id: \" &&\n> +\t\tgit format-patch --stdout -1 side-second | sed -e \"1d\"\n> +\t} >side-second.patch &&\n> +\t{\n> +\t\tsed -ne \"1,/^\\$/p\" side-second.patch &&\n> +\t\techo \"-- >8 --\" &&\n> +\t\tsed -e \"1,/^\\$/d\" side-second.patch\n> +\t} >side-second.scissors\n> +'\n\nA helper function that takes the branch name may be a good idea,\nnot just to consolidate the implementation but as a place to\ndocument how these pairs of files are constructed and why.\n\n> +test_expect_success '--3way, --no-3way' '\n> +\trm -fr .git/rebase-apply &&\n> +\tgit reset --hard &&\n> +\tgit checkout first &&\n> +\ttest_must_fail git am --3way side-first.patch side-second.patch &&\n> +\ttest -n \"$(git ls-files -u)\" &&\n> +\techo will-conflict >file &&\n> +\tgit add file &&\n> +\ttest_must_fail git am --no-3way --continue &&\n> +\ttest -z \"$(git ls-files -u)\"\n> +'\n> +\n> +test_expect_success '--no-quiet, --quiet' '\n> +\trm -fr .git/rebase-apply &&\n> +\tgit reset --hard &&\n> +\tgit checkout first &&\n> +\ttest_must_fail git am --no-quiet side-first.patch side-second.patch &&\n> +\ttest_must_be_empty out &&\n\nWhere did this 'out' come from?\n\n> +\techo side-first >file &&\n> +\tgit add file &&\n> +\tgit am --quiet --continue >out &&\n> +\ttest_must_be_empty out\n\nI can see this one, though I am not sure if it is sensible to see\nwhat the command says under --quiet option, especially if you are\nmaking sure it does not fail, which you already have checked for its\nexit status.\n\n> +'\n> +\n> +test_expect_success '--signoff, --no-signoff' '\n> +\trm -fr .git/rebase-apply &&\n> +\tgit reset --hard &&\n> +\tgit checkout first &&\n> +\ttest_must_fail git am --signoff side-first.patch side-second.patch &&\n> +\techo side-first >file &&\n> +\tgit add file &&\n> +\tgit am --no-signoff --continue &&\n> +\n> +\t# applied side-first will be signed off\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> +\t# applied side-second will not be signed off\n> +\ttest $(git cat-file commit HEAD | grep -c \"Signed-off-by:\") -eq 0\n> +'\n\nHmm, the command was run with --signoff at the start, first gets\napplied with \"am --no-signoff --resolved\" so I would expect it does\nnot get signed off, but the second one will apply cleanly on top, so\nshouldn't it get signed off? Or perhaps somehow I misread Peff's\nidea to make these override one-shot in $gmane/274635?\n"},{"id":"267194","messageId":"CACRoPnR1df+uEnpFArJtwEBCh+HiQYDYGOyZ7KQEGtrdiaX3GQ@mail.gmail.com","threadId":"39932","inReplyTo":"xmqqegjsfbtk.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] am: let command-line options override saved options","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-07-31T10:58:48Z","receivedAt":"2015-07-31T10:58:48Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"On Wed, Jul 29, 2015 at 1:09 AM, Junio C Hamano wrote:\n> Paul Tan writes:\n>\n>> diff --git a/t/t4153-am-resume-override-opts.sh b/t/t4153-am-resume-override-opts.sh\n>> new file mode 100755\n>> index 0000000..c49457c\n>> --- /dev/null\n>> +++ b/t/t4153-am-resume-override-opts.sh\n>> @@ -0,0 +1,144 @@\n>> +#!/bin/sh\n>> +\n>> +test_description='git-am command-line options override saved options'\n>> +\n>> +. ./test-lib.sh\n>> +\n>> +test_expect_success 'setup' '\n>> + test_commit initial file &&\n>> + test_commit first file &&\n>> +\n>> + git checkout -b side initial &&\n>> + test_commit side-first file &&\n>> + test_commit side-second file &&\n>> +\n>> + {\n>> + echo \"Message-Id: \" &&\n>> + git format-patch --stdout -1 side-first | sed -e \"1d\"\n>> + } >side-first.patch &&\n>> + {\n>> + sed -ne \"1,/^\\$/p\" side-first.patch &&\n>\n> sed -e \"/^\\$/q\" would work just as well here\n\nOK.\n\n>> + echo \"-- >8 --\" &&\n>> + sed -e \"1,/^\\$/d\" side-first.patch\n>> + } >side-first.scissors &&\n>> + {\n>> + echo \"Message-Id: \" &&\n>> + git format-patch --stdout -1 side-second | sed -e \"1d\"\n>> + } >side-second.patch &&\n>> + {\n>> + sed -ne \"1,/^\\$/p\" side-second.patch &&\n>> + echo \"-- >8 --\" &&\n>> + sed -e \"1,/^\\$/d\" side-second.patch\n>> + } >side-second.scissors\n>> +'\n>\n> A helper function that takes the branch name may be a good idea,\n> not just to consolidate the implementation but as a place to\n> document how these pairs of files are constructed and why.\n\nI think I will introduce a format_patch() function that takes a single\ncommit-ish so that we can use tag names to name the patches:\n\n# Given a single commit $commit, formats the following patches with\n# git-format-patch:\n#\n# 1. $commit.eml: an email patch with a Message-Id header.\n# 2. $commit.scissors: like $commit.eml but contains a scissors line at the\n# start of the commit message body.\nformat_patch () {\n {\n echo \"Message-Id: <$1@example.com>\" &&\n git format-patch --stdout -1 \"$1\" | sed -e '1d'\n } >\"$1\".eml &&\n {\n sed -e '/^$/q' \"$1\".eml &&\n echo '-- >8 --' &&\n sed -e '1,/^$/d' \"$1\".eml\n } >\"$1\".scissors\n}\n\n>> +'\n>> +\n>> +test_expect_success '--signoff, --no-signoff' '\n>> + rm -fr .git/rebase-apply &&\n>> + git reset --hard &&\n>> + git checkout first &&\n>> + test_must_fail git am --signoff side-first.patch side-second.patch &&\n>> + echo side-first >file &&\n>> + git add file &&\n>> + git am --no-signoff --continue &&\n>> +\n>> + # applied side-first will be signed off\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>> + # applied side-second will not be signed off\n>> + test $(git cat-file commit HEAD | grep -c \"Signed-off-by:\") -eq 0\n>> +'\n>\n> Hmm, the command was run with --signoff at the start, first gets\n> applied with \"am --no-signoff --resolved\" so I would expect it does\n> not get signed off, but the second one will apply cleanly on top, so\n> shouldn't it get signed off? Or perhaps somehow I misread Peff's\n> idea to make these override one-shot in $gmane/274635?\n\nAh, I was just following the structure of the code, but stepping back\nto think about it, I think there are 2 bugs:\n\n1. The signoff is appended during the email-parsing stage. As such,\nwhen we are resuming, --no-signoff will have no effect, because the\nsignoff has already been appended at that stage.\n\nA solution for this is tricky though, as there are functions of git-am\nthat probably depend on the present behavior of the appended signoff\nbeing present in the commit message:\n\n* The applypatch-msg hook\n\n* The --interactive prompt, where the user can edit the commit message\n(to remove or edit the signoff maybe?)\n\nThese functions are called before we attempt to apply the patch, so we\nshould probably call append_signoff before then. However, this still\nmeans that --no-signoff will have no effect should the patch\napplication fail and we resume, as the signoff would still have\nalready been appended...\n\nSo I dunno. I think the cleanest solution would be to change the\nbehavior so the commit message passed to the applypatch-msg hook and\n--interactive prompt do not contain the appended signoff, and instead\nonly append the signoff just before we commit. That way, both\n--signoff and --no-signoff overriding will work. What do you think?\n\n2. Re-reading Peff's message, I see that he expects the command-line\noptions to affect just the current patch, which makes sense. This\npatch would need to be extended to call am_load() after we finish\nprocessing the current patch when resuming. Something like:\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 8a0b0e4..228d4b1 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -1779,7 +1779,6 @@ static void am_run(struct am_state *state, int resume)\n\n if (resume) {\n validate_resume_state(state);\n- resume = 0;\n } else {\n int skip;\n\n@@ -1841,6 +1840,10 @@ static void am_run(struct am_state *state, int resume)\n\n next:\n am_next(state);\n+\n+ if (resume)\n+ am_load(state);\n+ resume = 0;\n }\n\n if (!is_empty_file(am_path(state, \"rewritten\"))) {\n@@ -1895,6 +1898,7 @@ static void am_resolve(struct am_state *state)\n\n next:\n am_next(state);\n+ am_load(state);\n am_run(state, 0);\n }\n\n@@ -2022,6 +2026,7 @@ static void am_skip(struct am_state *state)\n die(_(\"failed to clean index\"));\n\n am_next(state);\n+ am_load(state);\n am_run(state, 0);\n }\n\nThe tests will also need to be modified as well.\n\n>> +test_expect_success '--3way, --no-3way' '\n>> + rm -fr .git/rebase-apply &&\n>> + git reset --hard &&\n>> + git checkout first &&\n>> + test_must_fail git am --3way side-first.patch side-second.patch &&\n>> + test -n \"$(git ls-files -u)\" &&\n>> + echo will-conflict >file &&\n>> + git add file &&\n>> + test_must_fail git am --no-3way --continue &&\n>> + test -z \"$(git ls-files -u)\"\n>> +'\n>> +\n\n... Although if I implement the above change, I can't implement the\ntest for --3way, as I think the only way to check if --3way/--no-3way\nsuccessfully overrides the saved options for the current patch only is\nto run \"git am --3way\", but that does not work in the test runner as\nit expects stdin to be a TTY :-/ So I may have to remove this test.\nThis shouldn't be a problem though, as all the tests in this test\nsuite all test the same mechanism.\n\n>> +test_expect_success '--no-quiet, --quiet' '\n>> + rm -fr .git/rebase-apply &&\n>> + git reset --hard &&\n>> + git checkout first &&\n>> + test_must_fail git am --no-quiet side-first.patch side-second.patch &&\n>> + test_must_be_empty out &&\n>\n> Where did this 'out' come from?\n\nIt was a leftover from a previous iteration. Will fix.\n\n>\n>> + echo side-first >file &&\n>> + git add file &&\n>> + git am --quiet --continue >out &&\n>> + test_must_be_empty out\n>\n> I can see this one, though I am not sure if it is sensible to see\n> what the command says under --quiet option, especially if you are\n> making sure it does not fail, which you already have checked for its\n> exit status.\n\nWell, if --quiet fails to override --no-quiet, then there would be\nsomething written to the stdout, no?\n\nBut anyway, if we are implementing the above \"command-line option\noverriding only affects current patch\" behavior, then this test would\nbe checking if --quiet only affects a single patch, which in practice\nwould be quite silly. Maybe I should flip it around to \"--no-quiet\noverrides --quiet\", but even then it may still be unlikely to come up\nwith practice... Still, it may be useful to keep this test to check if\nthe option overriding mechanism is working properly.\n\nThanks,\nPaul\n"},{"id":"267200","messageId":"xmqqvbd05n5z.fsf@gitster.dls.corp.google.com","threadId":"39932","inReplyTo":"CACRoPnR1df+uEnpFArJtwEBCh+HiQYDYGOyZ7KQEGtrdiaX3GQ@mail.gmail.com","subject":"Re: [PATCH] am: let command-line options override saved options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-07-31T16:04:08Z","receivedAt":"2015-07-31T16:04:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Paul Tan writes:\n\n> I think I will introduce a format_patch() function that takes a single\n> commit-ish so that we can use tag names to name the patches:\n>\n> # Given a single commit $commit, formats the following patches with\n> # git-format-patch:\n> #\n> # 1. $commit.eml: an email patch with a Message-Id header.\n> # 2. $commit.scissors: like $commit.eml but contains a scissors line at the\n> # start of the commit message body.\n> format_patch () {\n> {\n> echo \"Message-Id: <$1@example.com>\" &&\n> git format-patch --stdout -1 \"$1\" | sed -e '1d'\n> } >\"$1\".eml &&\n\nI only said I can \"understand\" what is going on, though.\n\nIt feels a bit unnatural for a test to feed a message that lack the\n\"From \" header line. Perhaps\n\n\tgit format-patch --add-header=\"Message-Id: ...\" --stdout -1\n\nor something?\n\n> Ah, I was just following the structure of the code, but stepping back\n> to think about it, I think there are 2 bugs:\n>\n> 1. The signoff is appended during the email-parsing stage. As such,\n> when we are resuming, --no-signoff will have no effect, because the\n> signoff has already been appended at that stage.\n>\n> A solution for this is tricky though, as there are functions of git-am\n> that probably depend on the present behavior of the appended signoff\n> being present in the commit message:\n>\n> * The applypatch-msg hook\n>\n> * The --interactive prompt, where the user can edit the commit message\n> (to remove or edit the signoff maybe?)\n>\n> These functions are called before we attempt to apply the patch, so we\n> should probably call append_signoff before then. However, this still\n> means that --no-signoff will have no effect should the patch\n> application fail and we resume, as the signoff would still have\n> already been appended...\n\nAh, I see. Let's not worry about this; we cannot change the\nexpectation existing hook scripts depends on.\n\n> 2. Re-reading Peff's message, I see that he expects the command-line\n> options to affect just the current patch, which makes sense. This\n> patch would need to be extended to call am_load() after we finish\n> processing the current patch when resuming.\n\nYeah, so the idea is:\n\n - upon the very first invocation, we parse the command line options\n and write the states out;\n\n - subsequent invocation, we read from the states and then override\n with the command line options, but we do not write the states out\n to update, so that subsequent invocations will keep reading from\n the very first one.\n\nThat sounds sensible.\n\n> The tests will also need to be modified as well.\n>\n>>> +test_expect_success '--3way, --no-3way' '\n>>> + rm -fr .git/rebase-apply &&\n>>> + git reset --hard &&\n>>> + git checkout first &&\n>>> + test_must_fail git am --3way side-first.patch side-second.patch &&\n>>> + test -n \"$(git ls-files -u)\" &&\n>>> + echo will-conflict >file &&\n>>> + git add file &&\n>>> + test_must_fail git am --no-3way --continue &&\n>>> + test -z \"$(git ls-files -u)\"\n>>> +'\n>>> +\n>\n> ... Although if I implement the above change, I can't implement the\n> test for --3way, as I think the only way to check if --3way/--no-3way\n> successfully overrides the saved options for the current patch only is\n> to run \"git am --3way\", but that does not work in the test runner as\n> it expects stdin to be a TTY :-/ So I may have to remove this test.\n> This shouldn't be a problem though, as all the tests in this test\n> suite all test the same mechanism.\n\nSorry, you lost me. Where does the TTY come into the picture only\nfor --3way (but not for other things like --quiet)?\n"},{"id":"267250","messageId":"CACRoPnQHGtA3BQxLuY4douhBt3=_b5U2ny4uDGfmR=5+La68YQ@mail.gmail.com","threadId":"39932","inReplyTo":"xmqqvbd05n5z.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] am: let command-line options override saved options","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-08-01T00:59:22Z","receivedAt":"2015-08-01T00:59:22Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"On Sat, Aug 1, 2015 at 12:04 AM, Junio C Hamano wrote:\n> Paul Tan writes:\n>\n>> I think I will introduce a format_patch() function that takes a single\n>> commit-ish so that we can use tag names to name the patches:\n>>\n>> # Given a single commit $commit, formats the following patches with\n>> # git-format-patch:\n>> #\n>> # 1. $commit.eml: an email patch with a Message-Id header.\n>> # 2. $commit.scissors: like $commit.eml but contains a scissors line at the\n>> # start of the commit message body.\n>> format_patch () {\n>> {\n>> echo \"Message-Id: <$1@example.com>\" &&\n>> git format-patch --stdout -1 \"$1\" | sed -e '1d'\n>> } >\"$1\".eml &&\n>\n> I only said I can \"understand\" what is going on, though.\n>\n> It feels a bit unnatural for a test to feed a message that lack the\n> \"From \" header line. Perhaps\n>\n> git format-patch --add-header=\"Message-Id: ...\" --stdout -1\n>\n> or something?\n\nAh, okay. I wasn't aware of the --add-header option, but this is\ndefinitely better.\n\n>> These functions are called before we attempt to apply the patch, so we\n>> should probably call append_signoff before then. However, this still\n>> means that --no-signoff will have no effect should the patch\n>> application fail and we resume, as the signoff would still have\n>> already been appended...\n>\n> Ah, I see. Let's not worry about this; we cannot change the\n> expectation existing hook scripts depends on.\n\nOkay, although this means that with the below change, --[no-]signoff\nwill be the oddball option that does not work when resuming.\n\n>> 2. Re-reading Peff's message, I see that he expects the command-line\n>> options to affect just the current patch, which makes sense. This\n>> patch would need to be extended to call am_load() after we finish\n>> processing the current patch when resuming.\n>\n> Yeah, so the idea is:\n>\n> - upon the very first invocation, we parse the command line options\n> and write the states out;\n>\n> - subsequent invocation, we read from the states and then override\n> with the command line options, but we do not write the states out\n> to update, so that subsequent invocations will keep reading from\n> the very first one.\n\n... and we also load back the saved options after processing the patch\nthat we resume from, so the command-line options only affect the\nconflicting patch, which fits in with Peff's idea on \"wiggling that\n_one_ patch\".\n\n>>>> +test_expect_success '--3way, --no-3way' '\n>>>> + rm -fr .git/rebase-apply &&\n>>>> + git reset --hard &&\n>>>> + git checkout first &&\n>>>> + test_must_fail git am --3way side-first.patch side-second.patch &&\n>>>> + test -n \"$(git ls-files -u)\" &&\n>>>> + echo will-conflict >file &&\n>>>> + git add file &&\n>>>> + test_must_fail git am --no-3way --continue &&\n>>>> + test -z \"$(git ls-files -u)\"\n>>>> +'\n>>>> +\n>>\n>> ... Although if I implement the above change, I can't implement the\n>> test for --3way, as I think the only way to check if --3way/--no-3way\n>> successfully overrides the saved options for the current patch only is\n>> to run \"git am --3way\", but that does not work in the test runner as\n>> it expects stdin to be a TTY :-/ So I may have to remove this test.\n>> This shouldn't be a problem though, as all the tests in this test\n>> suite all test the same mechanism.\n>\n> Sorry, you lost me. Where does the TTY come into the picture only\n> for --3way (but not for other things like --quiet)?\n\nAh, sorry, I should have provided more context. This is due to the\nfollowing block of code:\n\n /*\n * Catch user error to feed us patches when there is a session\n * in progress:\n *\n * 1. mbox path(s) are provided on the command-line.\n * 2. stdin is not a tty: the user is trying to feed us a patch\n * from standard input. This is somewhat unreliable -- stdin\n * could be /dev/null for example and the caller did not\n * intend to feed us a patch but wanted to continue\n * unattended.\n */\n if (argc || (resume == RESUME_FALSE && !isatty(0)))\n die(_(\"previous rebase directory %s still exists but mbox given.\"),\n state.dir);\n\nAnd it will activate when git-am is run without\n--continue/--abort/--skip (e.g. \"git am --3way\") because the test\nframework sets stdin to /dev/null.\n\nThanks,\nPaul\n"},{"id":"267465","messageId":"1438697116-27799-1-git-send-email-pyokagan@gmail.com","threadId":"39932","inReplyTo":"20150728164311.GA1948@yoshi.chippynet.com","subject":"[PATCH v2 0/3] am: let command-line options override saved options","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-08-04T14:05:13Z","receivedAt":"2015-08-04T14:05:13Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"Let command-line options override saved options in git-am when resuming\n\nThis is a re-roll of [v1]. Previous versions:\n\n[v1] http://thread.gmane.org/gmane.comp.version-control.git/274789\n\nWhen resuming, git-am mistakenly ignores command-line options.\n\nFor instance, when a patch fails to apply with \"git am patch\", subsequently\nrunning \"git am --3way\" would not cause git-am to fall back on attempting a\nthreeway merge. This occurs because by default the --3way option is saved as\n\"false\", and the saved am options are loaded after the command-line options are\nparsed, thus overwriting the command-line options when resuming.\n\n[PATCH 1/3] tweaks test-terminal.perl to redirect the stdin of the child\nprocess to a pty. This is to support the tests in [PATCH 2/3].\n\n[PATCH 2/3] fixes builtin/am.c, enabling command-line options to override saved\noptions. However, even with this patch, the following command-line options have\nno effect when resuming:\n\n* --signoff overriding --no-signoff\n\n* --no-keep overriding --keep\n\n* --message-id overriding --no-message-id\n\n* --scissors overriding --no-scissors\n\nThis is because they are only taken into account during the mail-parsing stage,\nwhich is skipped over when resuming.\n\n[PATCH 3/3] adds support for the --signoff option when resuming by recognizing\nthat we can (re-)append the signoff when the user explicitly specifies the\n--signoff option.\n\nSince the --keep, --message-id and --scissors options are handled by\ngit-mailinfo, it is tricky to implement support for them without introducing\nlots of code complexity, and thus this patch series does not attempt to.\n\nFurthermore, it is hard to imagine a use case for e.g. --scissors overriding\n--no-scissors, and hence it might be preferable to wait until someone comes\nwith a solid use case, instead of implementing potentially undesirable behavior\nand having to support it.\n\n\nPaul Tan (3):\n test_terminal: redirect child process' stdin to a pty\n am: let command-line options override saved options\n am: let --signoff override --no-signoff\n\n builtin/am.c | 42 ++++++++++++---\n t/t4153-am-resume-override-opts.sh | 102 +++++++++++++++++++++++++++++++++++++\n t/test-terminal.perl | 25 +++++++--\n 3 files changed, 158 insertions(+), 11 deletions(-)\n create mode 100755 t/t4153-am-resume-override-opts.sh\n\n-- \n2.5.0.280.gd88bd6e\n"},{"id":"267466","messageId":"1438697331-29948-1-git-send-email-pyokagan@gmail.com","threadId":"39932","inReplyTo":"20150728164311.GA1948@yoshi.chippynet.com","subject":"[PATCH v2 0/3] am: let command-line options override saved options","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-08-04T14:08:48Z","receivedAt":"2015-08-04T14:08:48Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"Let command-line options override saved options in git-am when resuming\n\nThis is a re-roll of [v1]. Previous versions:\n\n[v1] http://thread.gmane.org/gmane.comp.version-control.git/274789\n\nWhen resuming, git-am mistakenly ignores command-line options.\n\nFor instance, when a patch fails to apply with \"git am patch\", subsequently\nrunning \"git am --3way\" would not cause git-am to fall back on attempting a\nthreeway merge. This occurs because by default the --3way option is saved as\n\"false\", and the saved am options are loaded after the command-line options are\nparsed, thus overwriting the command-line options when resuming.\n\n[PATCH 1/3] tweaks test-terminal.perl to redirect the stdin of the child\nprocess to a pty. This is to support the tests in [PATCH 2/3].\n\n[PATCH 2/3] fixes builtin/am.c, enabling command-line options to override saved\noptions. However, even with this patch, the following command-line options have\nno effect when resuming:\n\n* --signoff overriding --no-signoff\n\n* --no-keep overriding --keep\n\n* --message-id overriding --no-message-id\n\n* --scissors overriding --no-scissors\n\nThis is because they are only taken into account during the mail-parsing stage,\nwhich is skipped over when resuming.\n\n[PATCH 3/3] adds support for the --signoff option when resuming by recognizing\nthat we can (re-)append the signoff when the user explicitly specifies the\n--signoff option.\n\nSince the --keep, --message-id and --scissors options are handled by\ngit-mailinfo, it is tricky to implement support for them without introducing\nlots of code complexity, and thus this patch series does not attempt to.\n\nFurthermore, it is hard to imagine a use case for e.g. --scissors overriding\n--no-scissors, and hence it might be preferable to wait until someone comes\nwith a solid use case, instead of implementing potentially undesirable behavior\nand having to support it.\n\n\nPaul Tan (3):\n test_terminal: redirect child process' stdin to a pty\n am: let command-line options override saved options\n am: let --signoff override --no-signoff\n\n builtin/am.c | 42 ++++++++++++---\n t/t4153-am-resume-override-opts.sh | 102 +++++++++++++++++++++++++++++++++++++\n t/test-terminal.perl | 25 +++++++--\n 3 files changed, 158 insertions(+), 11 deletions(-)\n create mode 100755 t/t4153-am-resume-override-opts.sh\n\n-- \n2.5.0.280.gd88bd6e\n"},{"id":"267467","messageId":"1438697331-29948-2-git-send-email-pyokagan@gmail.com","threadId":"39932","inReplyTo":"1438697331-29948-1-git-send-email-pyokagan@gmail.com","subject":"[PATCH v2 1/3] test_terminal: redirect child process' stdin to a pty","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-08-04T14:08:49Z","receivedAt":"2015-08-04T14:08:49Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"When resuming, git-am detects if we are trying to feed it patches or not\nby checking if stdin is a TTY.\n\nHowever, the test library redirects stdin to /dev/null. This makes it\ndifficult, for instance, to test the behavior of \"git am -3\" when\nresuming, as git-am will think we are trying to feed it patches and\nerror out.\n\nSupport this use case by extending test-terminal.perl to create a\npseudo-tty for the child process' standard input as well.\n\nNote that due to the way the code is structured, the child's stdin\npseudo-tty will be closed when we finish reading from our stdin. This\nmeans that in the common case, where our stdin is attached to /dev/null,\nthe child's stdin pseudo-tty will be closed immediately. Some operations\nlike isatty(), which git-am uses, require the file descriptor to be\nopen, and hence if the success of the command depends on such functions,\ntest_terminal's stdin should be redirected to a source with large amount\nof data to ensure that the child's stdin is not closed, e.g.\n\n\ttest_terminal git am --3way \nCc: Jeff King \nSigned-off-by: Paul Tan \n---\n t/test-terminal.perl | 25 ++++++++++++++++++++-----\n 1 file changed, 20 insertions(+), 5 deletions(-)\n\ndiff --git a/t/test-terminal.perl b/t/test-terminal.perl\nindex 1fb373f..f6fc9ae 100755\n--- a/t/test-terminal.perl\n+++ b/t/test-terminal.perl\n@@ -5,15 +5,17 @@ use warnings;\n use IO::Pty;\n use File::Copy;\n \n-# Run @$argv in the background with stdio redirected to $out and $err.\n+# Run @$argv in the background with stdio redirected to $in, $out and $err.\n sub start_child {\n-\tmy ($argv, $out, $err) = @_;\n+\tmy ($argv, $in, $out, $err) = @_;\n \tmy $pid = fork;\n \tif (not defined $pid) {\n \t\tdie \"fork failed: $!\"\n \t} elsif ($pid == 0) {\n+\t\topen STDIN, \"<&\", $in;\n \t\topen STDOUT, \">&\", $out;\n \t\topen STDERR, \">&\", $err;\n+\t\tclose $in;\n \t\tclose $out;\n \t\texec(@$argv) or die \"cannot exec '$argv->[0]': $!\"\n \t}\n@@ -50,14 +52,23 @@ sub xsendfile {\n }\n \n sub copy_stdio {\n-\tmy ($out, $err) = @_;\n+\tmy ($in, $out, $err) = @_;\n \tmy $pid = fork;\n+\tif (!$pid) {\n+\t\tclose($out);\n+\t\tclose($err);\n+\t\txsendfile($in, \\*STDIN);\n+\t\texit 0;\n+\t}\n+\t$pid = fork;\n \tdefined $pid or die \"fork failed: $!\";\n \tif (!$pid) {\n+\t\tclose($in);\n \t\tclose($out);\n \t\txsendfile(\\*STDERR, $err);\n \t\texit 0;\n \t}\n+\tclose($in);\n \tclose($err);\n \txsendfile(\\*STDOUT, $out);\n \tfinish_child($pid) == 0\n@@ -67,14 +78,18 @@ sub copy_stdio {\n if ($#ARGV < 1) {\n \tdie \"usage: test-terminal program args\";\n }\n+my $master_in = new IO::Pty;\n my $master_out = new IO::Pty;\n my $master_err = new IO::Pty;\n+$master_in->set_raw();\n $master_out->set_raw();\n $master_err->set_raw();\n+$master_in->slave->set_raw();\n $master_out->slave->set_raw();\n $master_err->slave->set_raw();\n-my $pid = start_child(\\@ARGV, $master_out->slave, $master_err->slave);\n+my $pid = start_child(\\@ARGV, $master_in->slave, $master_out->slave, $master_err->slave);\n+close $master_in->slave;\n close $master_out->slave;\n close $master_err->slave;\n-copy_stdio($master_out, $master_err);\n+copy_stdio($master_in, $master_out, $master_err);\n exit(finish_child($pid));\n-- \n2.5.0.280.gd88bd6e\n"},{"id":"267468","messageId":"1438697331-29948-3-git-send-email-pyokagan@gmail.com","threadId":"39932","inReplyTo":"1438697331-29948-1-git-send-email-pyokagan@gmail.com","subject":"[PATCH v2 2/3] am: let command-line options override saved options","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-08-04T14:08:50Z","receivedAt":"2015-08-04T14:08:50Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"When resuming, git-am mistakenly ignores command-line options.\n\nFor instance, when a patch fails to apply with \"git am patch\",\nsubsequently running \"git am --3way\" would not cause git-am to fall\nback on attempting a threeway merge. This occurs because by default\nthe --3way option is saved as \"false\", and the saved am options are\nloaded after the command-line options are parsed, thus overwriting\nthe command-line options when resuming.\n\nFix this by moving the am_load() function call before parse_options(),\nso that command-line options will override the saved am options.\n\nThe purpose of supporting this use case is to enable users to \"wiggle\"\nthat one conflicting patch. As such, it is expected that the\ncommand-line options do not affect subsequent applied patches. Implement\nthis by calling am_load() once we apply the conflicting patch\nsuccessfully.\n\nNoticed-by: Junio C Hamano \nHelped-by: Junio C Hamano \nHelped-by: Jeff King \nSigned-off-by: Paul Tan \n---\n builtin/am.c | 16 ++++++--\n t/t4153-am-resume-override-opts.sh | 82 ++++++++++++++++++++++++++++++++++++++\n 2 files changed, 94 insertions(+), 4 deletions(-)\n create mode 100755 t/t4153-am-resume-override-opts.sh\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 84d57d4..0961304 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -1777,7 +1777,6 @@ static void am_run(struct am_state *state, int resume)\n \n \t\tif (resume) {\n \t\t\tvalidate_resume_state(state);\n-\t\t\tresume = 0;\n \t\t} else {\n \t\t\tint skip;\n \n@@ -1839,6 +1838,10 @@ static void am_run(struct am_state *state, int resume)\n \n next:\n \t\tam_next(state);\n+\n+\t\tif (resume)\n+\t\t\tam_load(state);\n+\t\tresume = 0;\n \t}\n \n \tif (!is_empty_file(am_path(state, \"rewritten\"))) {\n@@ -1893,6 +1896,7 @@ static void am_resolve(struct am_state *state)\n \n next:\n \tam_next(state);\n+\tam_load(state);\n \tam_run(state, 0);\n }\n \n@@ -2020,6 +2024,7 @@ static void am_skip(struct am_state *state)\n \t\tdie(_(\"failed to clean index\"));\n \n \tam_next(state);\n+\tam_load(state);\n \tam_run(state, 0);\n }\n \n@@ -2130,6 +2135,7 @@ int cmd_am(int argc, const char **argv, const char *prefix)\n \tint keep_cr = -1;\n \tint patch_format = PATCH_FORMAT_UNKNOWN;\n \tenum resume_mode resume = RESUME_FALSE;\n+\tint in_progress;\n \n \tconst char * const usage[] = {\n \t\tN_(\"git am [options] [(|)...]\"),\n@@ -2225,6 +2231,10 @@ int cmd_am(int argc, const char **argv, const char *prefix)\n \n \tam_state_init(&state, git_path(\"rebase-apply\"));\n \n+\tin_progress = am_in_progress(&state);\n+\tif (in_progress)\n+\t\tam_load(&state);\n+\n \targc = parse_options(argc, argv, prefix, options, usage, 0);\n \n \tif (binary >= 0)\n@@ -2237,7 +2247,7 @@ int cmd_am(int argc, const char **argv, const char *prefix)\n \tif (read_index_preload(&the_index, NULL) < 0)\n \t\tdie(_(\"failed to read the index\"));\n \n-\tif (am_in_progress(&state)) {\n+\tif (in_progress) {\n \t\t/*\n \t\t * Catch user error to feed us patches when there is a session\n \t\t * in progress:\n@@ -2255,8 +2265,6 @@ int cmd_am(int argc, const char **argv, const char *prefix)\n \n \t\tif (resume == RESUME_FALSE)\n \t\t\tresume = RESUME_APPLY;\n-\n-\t\tam_load(&state);\n \t} else {\n \t\tstruct argv_array paths = ARGV_ARRAY_INIT;\n \t\tint i;\ndiff --git a/t/t4153-am-resume-override-opts.sh b/t/t4153-am-resume-override-opts.sh\nnew file mode 100755\nindex 0000000..39fac79\n--- /dev/null\n+++ b/t/t4153-am-resume-override-opts.sh\n@@ -0,0 +1,82 @@\n+#!/bin/sh\n+\n+test_description='git-am command-line options override saved options'\n+\n+. ./test-lib.sh\n+. \"$TEST_DIRECTORY\"/lib-terminal.sh\n+\n+format_patch () {\n+\tgit format-patch --stdout -1 \"$1\" >\"$1\".eml\n+}\n+\n+test_expect_success 'setup' '\n+\ttest_commit initial file &&\n+\ttest_commit first file &&\n+\n+\tgit checkout initial &&\n+\tgit mv file file2 &&\n+\ttest_tick &&\n+\tgit commit -m renamed-file &&\n+\tgit tag renamed-file &&\n+\n+\tgit checkout -b side initial &&\n+\ttest_commit side1 file &&\n+\ttest_commit side2 file &&\n+\n+\tformat_patch side1 &&\n+\tformat_patch side2\n+'\n+\n+test_expect_success TTY '--3way overrides --no-3way' '\n+\trm -fr .git/rebase-apply &&\n+\tgit reset --hard &&\n+\tgit checkout renamed-file &&\n+\n+\t# Applying side1 will fail as the file has been renamed.\n+\ttest_must_fail git am --no-3way side[12].eml &&\n+\ttest_path_is_dir .git/rebase-apply &&\n+\ttest_cmp_rev renamed-file HEAD &&\n+\ttest -z \"$(git ls-files -u)\" &&\n+\n+\t# Applying side1 with am --3way will succeed due to the threeway-merge.\n+\t# Applying side2 will fail as --3way does not apply to it.\n+\ttest_must_fail test_terminal git am --3way out &&\n+\ttest_path_is_dir .git/rebase-apply &&\n+\t! test_i18ngrep \"^Applying: \" out &&\n+\techo side1 >file &&\n+\tgit add file &&\n+\n+\t# Applying side1 will not be quiet.\n+\t# Applying side2 will be quiet.\n+\tgit am --no-quiet --continue >out &&\n+\techo \"Applying: side1\" >expected &&\n+\ttest_i18ncmp expected out\n+'\n+\n+test_expect_success TTY '--reject overrides --no-reject' '\n+\trm -fr .git/rebase-apply &&\n+\tgit reset --hard &&\n+\tgit checkout first &&\n+\trm -f file.rej &&\n+\n+\ttest_must_fail git am --no-reject side1.eml &&\n+\ttest_path_is_dir .git/rebase-apply &&\n+\ttest_path_is_missing file.rej &&\n+\n+\ttest_must_fail test_terminal git am --reject \n---\n builtin/am.c | 28 +++++++++++++++++++++++++---\n t/t4153-am-resume-override-opts.sh | 20 ++++++++++++++++++++\n 2 files changed, 45 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 0961304..8c95aec 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -98,6 +98,12 @@ enum scissors_type {\n \tSCISSORS_TRUE /* pass --scissors to git-mailinfo */\n };\n \n+enum signoff_type {\n+\tSIGNOFF_FALSE = 0,\n+\tSIGNOFF_TRUE = 1,\n+\tSIGNOFF_EXPLICIT /* --signoff was set on the command-line */\n+};\n+\n struct am_state {\n \t/* state directory path */\n \tchar *dir;\n@@ -123,7 +129,7 @@ struct am_state {\n \tint interactive;\n \tint threeway;\n \tint quiet;\n-\tint signoff;\n+\tint signoff; /* enum signoff_type */\n \tint utf8;\n \tint keep; /* enum keep_type */\n \tint message_id;\n@@ -1184,6 +1190,18 @@ static void NORETURN die_user_resolve(const struct am_state *state)\n }\n \n /**\n+ * Appends signoff to the \"msg\" field of the am_state.\n+ */\n+static void am_append_signoff(struct am_state *state)\n+{\n+\tstruct strbuf sb = STRBUF_INIT;\n+\n+\tstrbuf_attach(&sb, state->msg, state->msg_len, state->msg_len);\n+\tappend_signoff(&sb, 0, 0);\n+\tstate->msg = strbuf_detach(&sb, &state->msg_len);\n+}\n+\n+/**\n * Parses `mail` using git-mailinfo, extracting its patch and authorship info.\n * state->msg will be set to the patch message. state->author_name,\n * state->author_email and state->author_date will be set to the patch author's\n@@ -2151,8 +2169,9 @@ int cmd_am(int argc, const char **argv, const char *prefix)\n \t\tOPT_BOOL('3', \"3way\", &state.threeway,\n \t\t\tN_(\"allow fall back on 3way merging if needed\")),\n \t\tOPT__QUIET(&state.quiet, N_(\"be quiet\")),\n-\t\tOPT_BOOL('s', \"signoff\", &state.signoff,\n-\t\t\tN_(\"add a Signed-off-by line to the commit message\")),\n+\t\tOPT_SET_INT('s', \"signoff\", &state.signoff,\n+\t\t\tN_(\"add a Signed-off-by line to the commit message\"),\n+\t\t\tSIGNOFF_EXPLICIT),\n \t\tOPT_BOOL('u', \"utf8\", &state.utf8,\n \t\t\tN_(\"recode into utf8 (default)\")),\n \t\tOPT_SET_INT('k', \"keep\", &state.keep,\n@@ -2265,6 +2284,9 @@ int cmd_am(int argc, const char **argv, const char *prefix)\n \n \t\tif (resume == RESUME_FALSE)\n \t\t\tresume = RESUME_APPLY;\n+\n+\t\tif (state.signoff == SIGNOFF_EXPLICIT)\n+\t\t\tam_append_signoff(&state);\n \t} else {\n \t\tstruct argv_array paths = ARGV_ARRAY_INIT;\n \t\tint i;\ndiff --git a/t/t4153-am-resume-override-opts.sh b/t/t4153-am-resume-override-opts.sh\nindex 39fac79..7c013d8 100755\n--- a/t/t4153-am-resume-override-opts.sh\n+++ b/t/t4153-am-resume-override-opts.sh\n@@ -64,6 +64,26 @@ test_expect_success '--no-quiet overrides --quiet' '\n \ttest_i18ncmp expected out\n '\n \n+test_expect_success '--signoff overrides --no-signoff' '\n+\trm -fr .git/rebase-apply &&\n+\tgit reset --hard &&\n+\tgit checkout first &&\n+\n+\ttest_must_fail git am --no-signoff side[12].eml &&\n+\ttest_path_is_dir .git/rebase-apply &&\n+\techo side1 >file &&\n+\tgit add file &&\n+\tgit am --signoff --continue &&\n+\n+\t# Applied side1 will be signed off\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+\t# Applied side2 will not be signed off\n+\ttest $(git cat-file commit HEAD | grep -c \"Signed-off-by:\") -eq 0\n+'\n+\n test_expect_success TTY '--reject overrides --no-reject' '\n \trm -fr .git/rebase-apply &&\n \tgit reset --hard &&\n-- \n2.5.0.280.gd88bd6e\n"},{"id":"267479","messageId":"xmqqbnem69mm.fsf@gitster.dls.corp.google.com","threadId":"39932","inReplyTo":"1438697116-27799-1-git-send-email-pyokagan@gmail.com","subject":"Re: [PATCH v2 0/3] am: let command-line options override saved options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-04T21:12:33Z","receivedAt":"2015-08-04T21:12:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Paul Tan writes:\n\n> Let command-line options override saved options in git-am when resuming\n>\n> This is a re-roll of [v1]. Previous versions:\n>\n> [v1] http://thread.gmane.org/gmane.comp.version-control.git/274789\n>\n> When resuming, git-am mistakenly ignores command-line options.\n>\n> For instance, when a patch fails to apply with \"git am patch\", subsequently\n> running \"git am --3way\" would not cause git-am to fall back on attempting a\n> threeway merge. This occurs because by default the --3way option is saved as\n> \"false\", and the saved am options are loaded after the command-line options are\n> parsed, thus overwriting the command-line options when resuming.\n>\n> [PATCH 1/3] tweaks test-terminal.perl to redirect the stdin of the child\n> process to a pty. This is to support the tests in [PATCH 2/3].\n>\n> [PATCH 2/3] fixes builtin/am.c, enabling command-line options to override saved\n> options. However, even with this patch, the following command-line options have\n> no effect when resuming:\n>\n> * --signoff overriding --no-signoff\n>\n> * --no-keep overriding --keep\n>\n> * --message-id overriding --no-message-id\n>\n> * --scissors overriding --no-scissors\n>\n> This is because they are only taken into account during the mail-parsing stage,\n> which is skipped over when resuming.\n\nIt is more like \"which has already happened\", so I would tend to\nthink that these are the right things to ignore. Otherwise, a\npretty common sequence would not work well ...\n\n $ git am mbox\n ... conflicted ...\n $ edit .git/rebase-apply/patch\n $ git am\n\n... if the \"resuming\" invocation re-split the message by running\nmailinfo again, the edit by the user will be lost.\n"},{"id":"267515","messageId":"xmqq37zx68uf.fsf@gitster.dls.corp.google.com","threadId":"39932","inReplyTo":"1438697331-29948-1-git-send-email-pyokagan@gmail.com","subject":"Re: [PATCH v2 0/3] am: let command-line options override saved options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-05T15:41:44Z","receivedAt":"2015-08-05T15:41:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Interesting. This seems to break test under prove.\n\n cd t && make T=t4153-am-resume-override-opts.sh prove\n\ndoes not seem to return.\n"},{"id":"267527","messageId":"20150805175117.GA6759@yoshi.chippynet.com","threadId":"39932","inReplyTo":"xmqq37zx68uf.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2 0/3] am: let command-line options override saved options","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-08-05T17:51:17Z","receivedAt":"2015-08-05T17:51:17Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"On Wed, Aug 05, 2015 at 08:41:44AM -0700, Junio C Hamano wrote:\n> Interesting. This seems to break test under prove.\n> \n> cd t && make T=t4153-am-resume-override-opts.sh prove\n> \n> does not seem to return.\n\nThe new test-terminal.perl code is the culprit. It seems that if our\nwrapped process terminates before our stdin-writing fork does, our\nstdin-writing process will stall. I think this occurs with prove because\nprove waits until all of its child processes terminate before returning.\n\nSo, the solution may be to send a SIGTERM to our stdin-writing fork\nshould our wrapped process terminate before it does, in order to ensure\nthat it immediately exits.\n\nThe following squash fixes it for me.\n\nThanks,\nPaul\n\n-- >8 --\nSubject: [PATCH] squash! test_terminal: redirect child process' stdin to a pty\n\nWhen the child process terminates before the copy_stdio() finishes\nwriting all of its data to the child's stdin slave pty, it will stall.\n\nAs such, we first move the stdin-pty-writing logic out of copy_stdio()\ninto its own subroutine copy_stdin() so that we can manage the forked\nprocess ourselves, and then we send SIGTERM to the forked process should\nthe command we are wrapping terminate before copy_stdin() finishes\nwriting all of its data to un-stall it.\n\nSigned-off-by: Paul Tan \n---\n t/test-terminal.perl | 27 ++++++++++++++++++---------\n 1 file changed, 18 insertions(+), 9 deletions(-)\n\ndiff --git a/t/test-terminal.perl b/t/test-terminal.perl\nindex f6fc9ae..96b6a03 100755\n--- a/t/test-terminal.perl\n+++ b/t/test-terminal.perl\n@@ -51,24 +51,26 @@ sub xsendfile {\n \tcopy($in, $out, 4096) or $!{EIO} or die \"cannot copy from child: $!\";\n }\n \n-sub copy_stdio {\n-\tmy ($in, $out, $err) = @_;\n+sub copy_stdin {\n+\tmy ($in) = @_;\n \tmy $pid = fork;\n \tif (!$pid) {\n-\t\tclose($out);\n-\t\tclose($err);\n \t\txsendfile($in, \\*STDIN);\n \t\texit 0;\n \t}\n-\t$pid = fork;\n+\tclose($in);\n+\treturn $pid;\n+}\n+\n+sub copy_stdio {\n+\tmy ($out, $err) = @_;\n+\tmy $pid = fork;\n \tdefined $pid or die \"fork failed: $!\";\n \tif (!$pid) {\n-\t\tclose($in);\n \t\tclose($out);\n \t\txsendfile(\\*STDERR, $err);\n \t\texit 0;\n \t}\n-\tclose($in);\n \tclose($err);\n \txsendfile(\\*STDOUT, $out);\n \tfinish_child($pid) == 0\n@@ -91,5 +93,12 @@ my $pid = start_child(\\@ARGV, $master_in->slave, $master_out->slave, $master_err\n close $master_in->slave;\n close $master_out->slave;\n close $master_err->slave;\n-copy_stdio($master_in, $master_out, $master_err);\n-exit(finish_child($pid));\n+my $in_pid = copy_stdin($master_in);\n+copy_stdio($master_out, $master_err);\n+my $ret = finish_child($pid);\n+# If the child process terminates before our copy_stdin() process is able to\n+# write all of its data to $master_in, the copy_stdin() process could stall.\n+# Send SIGTERM to it to ensure it terminates.\n+kill 'TERM', $in_pid;\n+finish_child($in_pid);\n+exit($ret);\n-- \n2.5.0.282.gdd6b4b0\n"},{"id":"267591","messageId":"CAPig+cS0RxCexLG+2ZjCMhDEBCa9HgF6JgC3RWbxRgQwd6uiZg@mail.gmail.com","threadId":"39932","inReplyTo":"1438697331-29948-2-git-send-email-pyokagan@gmail.com","subject":"Re: [PATCH v2 1/3] test_terminal: redirect child process' stdin to a pty","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-08-06T22:15:53Z","receivedAt":"2015-08-06T22:15:53Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Aug 4, 2015 at 10:08 AM, Paul Tan wrote:\n> When resuming, git-am detects if we are trying to feed it patches or not\n> by checking if stdin is a TTY.\n>\n> However, the test library redirects stdin to /dev/null. This makes it\n> difficult, for instance, to test the behavior of \"git am -3\" when\n> resuming, as git-am will think we are trying to feed it patches and\n> error out.\n>\n> Support this use case by extending test-terminal.perl to create a\n> pseudo-tty for the child process' standard input as well.\n\nAn alternative would be to have git-am detect that it is being tested\nand pretend that isatty() returns true. There is some precedent for\nhaving core functionality recognize that it is being tested. See, for\ninstance, environment variable TEST_DATE_NOW, and rev-list\n--test-bitmap. Doing so would allow the tests work on non-Unix\nplatforms, as well.\n\n> Note that due to the way the code is structured, the child's stdin\n> pseudo-tty will be closed when we finish reading from our stdin. This\n> means that in the common case, where our stdin is attached to /dev/null,\n> the child's stdin pseudo-tty will be closed immediately. Some operations\n> like isatty(), which git-am uses, require the file descriptor to be\n> open, and hence if the success of the command depends on such functions,\n> test_terminal's stdin should be redirected to a source with large amount\n> of data to ensure that the child's stdin is not closed, e.g.\n>\n> test_terminal git am --3way \n> Signed-off-by: Paul Tan \n"},{"id":"267608","messageId":"16ad6c2da8f85b9f5fc26ee6ebad944b@www.dscho.org","threadId":"39932","inReplyTo":"1438697331-29948-4-git-send-email-pyokagan@gmail.com","subject":"Re: [PATCH v2 3/3] am: let --signoff override --no-signoff","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2015-08-07T09:29:13Z","receivedAt":"2015-08-07T09:29:13Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Paul,\n\nOn 2015-08-04 16:08, Paul Tan wrote:\n\n> diff --git a/builtin/am.c b/builtin/am.c\n> index 0961304..8c95aec 100644\n> --- a/builtin/am.c\n> +++ b/builtin/am.c\n> @@ -2151,8 +2169,9 @@ int cmd_am(int argc, const char **argv, const\n> [...]\n> char *prefix)\n> \t\tOPT_BOOL('3', \"3way\", &state.threeway,\n> \t\t\tN_(\"allow fall back on 3way merging if needed\")),\n> \t\tOPT__QUIET(&state.quiet, N_(\"be quiet\")),\n> -\t\tOPT_BOOL('s', \"signoff\", &state.signoff,\n> -\t\t\tN_(\"add a Signed-off-by line to the commit message\")),\n> +\t\tOPT_SET_INT('s', \"signoff\", &state.signoff,\n> +\t\t\tN_(\"add a Signed-off-by line to the commit message\"),\n> +\t\t\tSIGNOFF_EXPLICIT),\n> \t\tOPT_BOOL('u', \"utf8\", &state.utf8,\n> \t\t\tN_(\"recode into utf8 (default)\")),\n> \t\tOPT_SET_INT('k', \"keep\", &state.keep,\n> @@ -2265,6 +2284,9 @@ int cmd_am(int argc, const char **argv, const\n> char *prefix)\n> \n> \t\tif (resume == RESUME_FALSE)\n> \t\t\tresume = RESUME_APPLY;\n> +\n> +\t\tif (state.signoff == SIGNOFF_EXPLICIT)\n> +\t\t\tam_append_signoff(&state);\n> \t} else {\n\nThis is clever, but I suspect there is now a chance for a double-signoff if we passed `--signoff` to the initial `git am` call and it went through without having to resume.\n\nOr am I missing something?\n\nCiao,\nDscho\n"},{"id":"267891","messageId":"CACRoPnSmeR9ETZ4M6GqkbtEZxM-_J-Dxgsx34ntPnojM1j0ZqQ@mail.gmail.com","threadId":"39932","inReplyTo":"16ad6c2da8f85b9f5fc26ee6ebad944b@www.dscho.org","subject":"Re: [PATCH v2 3/3] am: let --signoff override --no-signoff","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-08-12T03:06:37Z","receivedAt":"2015-08-12T03:06:37Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"On Fri, Aug 7, 2015 at 5:29 PM, Johannes Schindelin\n wrote:\n>> diff --git a/builtin/am.c b/builtin/am.c\n>> index 0961304..8c95aec 100644\n>> --- a/builtin/am.c\n>> +++ b/builtin/am.c\n>> @@ -2265,6 +2284,9 @@ int cmd_am(int argc, const char **argv, const\n>> char *prefix)\n>>\n>> if (resume == RESUME_FALSE)\n>> resume = RESUME_APPLY;\n>> +\n>> + if (state.signoff == SIGNOFF_EXPLICIT)\n>> + am_append_signoff(&state);\n>> } else {\n>\n> This is clever, but I suspect there is now a chance for a double-signoff if we passed `--signoff` to the initial `git am` call and it went through without having to resume.\n\nIt's not present in this diff context, but this hunk modifies the code\npath where in_progress is true. In other words, we only check for\nSIGNOFF_EXPLICIT if\n"},{"id":"267892","messageId":"CACRoPnQufFJuK+JcdZprBRfaFczkMo0Q=ddp2M6=SiThXPDvTw@mail.gmail.com","threadId":"39932","inReplyTo":"CACRoPnSmeR9ETZ4M6GqkbtEZxM-_J-Dxgsx34ntPnojM1j0ZqQ@mail.gmail.com","subject":"Re: [PATCH v2 3/3] am: let --signoff override --no-signoff","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-08-12T03:07:20Z","receivedAt":"2015-08-12T03:07:20Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"On Wed, Aug 12, 2015 at 11:06 AM, Paul Tan wrote:\n> It's not present in this diff context, but this hunk modifies the code\n> path where in_progress is true. In other words, we only check for\n> SIGNOFF_EXPLICIT if\n\n..we are resuming.\n\n(Ugh, butter fingers)\n\nThanks,\nPaul\n"},{"id":"267894","messageId":"CACRoPnTu=rSMZXaP5y_5Fb9o6u3u=s-HO+Ny4CY226rT6Wx2zA@mail.gmail.com","threadId":"39932","inReplyTo":"CAPig+cS0RxCexLG+2ZjCMhDEBCa9HgF6JgC3RWbxRgQwd6uiZg@mail.gmail.com","subject":"Re: [PATCH v2 1/3] test_terminal: redirect child process' stdin to a pty","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-08-12T04:16:02Z","receivedAt":"2015-08-12T04:16:02Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"On Fri, Aug 7, 2015 at 6:15 AM, Eric Sunshine wrote:\n> An alternative would be to have git-am detect that it is being tested\n> and pretend that isatty() returns true.\n\nI would vastly prefer a solution that would work for everything, for\nall the C code and scripts, instead of implementing a workaround in\ngit-am :(\n\nIn this case, I implemented a generic solution in test-terminal.perl\nthat works for POSIX systems, so if there are no problems with its\nimplementation, I do think it's better. Other than the fact that it\ndoes not work on non-Unix platforms, of course.\n\nThe other approach I would consider is to implement a xisatty()\nfunction that returns true for xisatty(0) if TEST_TTY=0 or something.\n\nHowever, I do wonder if this would lead us to have to hack around\nother functions of terminals as well (e.g. if xisatty(0),\ntcgetattr()), which would be a big can of worms I think...\n\n> There is some precedent for\n> having core functionality recognize that it is being tested. See, for\n> instance, environment variable TEST_DATE_NOW,\n\n(Hmm, I took a look, and it seems that TEST_DATE_NOW is only checked\nin test-date.c...)\n\n> and rev-list --test-bitmap.\n> Doing so would allow the tests work on non-Unix\n> platforms, as well.\n\nEhh, if the non-Unix platforms do not implement terminals, it means\nthat the git-am logic to detect if we are attempting to feed it a\npatch by checking if stdin is a TTY is invalid anyway, so implementing\na \"yeah-it-is-a-tty\" workaround for the sake of tests would be hiding\nthe problem, I think.\n\nThanks,\nPaul\n"}]}