* "git am" and then "git am -3" regression?
@ 2015-07-24 17:48 Junio C Hamano
2015-07-24 18:09 ` Jeff King
0 siblings, 1 reply; 26+ messages in thread
From: Junio C Hamano @ 2015-07-24 17:48 UTC (permalink / raw)
To: Remi Lespinet; +Cc: git, Paul Tan
Hmm, there seems to be some glitches around running "am -3"
after a failed "am" between 'maint' and 'master'.
When I try the following sequence, the 'am' from 'maint' succeeds,
but 'am' in 'master' fails:
* Save Eric's "minor documetation improvements" $gmane/274537
to a file.
* "git checkout e177995" (that's "next^0") and then apply them with
"git am" (no -3 necessary).
* "git checkout 272be14" (that's "es/worktree-add-cleanup^0") and
then apply them with "git am" (without -3).
This is expected to stop at 2/6, as the context has changed
between 272be14 and the tip of 'next'.
* "git am -3". This should restart and resolve cleanly.
Reverting d96a275b91bae1800cd43be0651e886e7e042a17 seems to fix it,
so that is what I'll do for 2.5 final.
I think Paul's builtin-am has the same issues, that would need a
separate fix.
commit d96a275b91bae1800cd43be0651e886e7e042a17
Author: Remi Lespinet <remi.lespinet@ensimag.grenoble-inp.fr>
Date: Thu Jun 4 17:04:55 2015 +0200
git-am: add am.threeWay config variable
Add the am.threeWay configuration variable to use the -3 or --3way
option of git am by default. When am.threeway is set and not desired
for a specific git am command, the --no-3way option can be used to
override it.
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: "git am" and then "git am -3" regression? 2015-07-24 17:48 "git am" and then "git am -3" regression? Junio C Hamano @ 2015-07-24 18:09 ` Jeff King 2015-07-26 5:03 ` Paul Tan 0 siblings, 1 reply; 26+ messages in thread From: Jeff King @ 2015-07-24 18:09 UTC (permalink / raw) To: Junio C Hamano; +Cc: Remi Lespinet, git, Paul Tan On Fri, Jul 24, 2015 at 10:48:18AM -0700, Junio C Hamano wrote: > Hmm, there seems to be some glitches around running "am -3" > after a failed "am" between 'maint' and 'master'. > > When I try the following sequence, the 'am' from 'maint' succeeds, > but 'am' in 'master' fails: > > * Save Eric's "minor documetation improvements" $gmane/274537 > to a file. > > * "git checkout e177995" (that's "next^0") and then apply them with > "git am" (no -3 necessary). > > * "git checkout 272be14" (that's "es/worktree-add-cleanup^0") and > then apply them with "git am" (without -3). > > This is expected to stop at 2/6, as the context has changed > between 272be14 and the tip of 'next'. > > * "git am -3". This should restart and resolve cleanly. Thanks for diagnosing. This bit me the other day, but I hadn't had time to look at it yet (and I "am" a lot less than you do, I imagine). > Reverting d96a275b91bae1800cd43be0651e886e7e042a17 seems to fix it, > so that is what I'll do for 2.5 final. Yeah, I think this hunk is to blame (though I just read the code and did not test): @@ -658,6 +665,8 @@ fi if test "$(cat "$dotest/threeway")" = t then threeway=t +else + threeway=f fi It comes after the command-line option parsing, so it overrides our option (I think that running "git am -3" followed by "git am --no-3way" would have the same problem). It cannot just check whether $threeway is unset, though, as it may have come from the config. We'd need a separate variable, the way the code is ordered now. Ideally the code would just be ordered as: - load config from git-config - override that with defaults inherited from a previous run - override that with command-line parsing but I don't know if there are other ordering gotchas that would break. It does look like that is how Paul's builtin/am.c does it, which makes me think it might not be broken. It's also possibly I've horribly misdiagnosed the bug. ;) -Peff ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: "git am" and then "git am -3" regression? 2015-07-24 18:09 ` Jeff King @ 2015-07-26 5:03 ` Paul Tan 2015-07-26 5:21 ` Jeff King 2015-07-27 14:21 ` Junio C Hamano 0 siblings, 2 replies; 26+ messages in thread From: Paul Tan @ 2015-07-26 5:03 UTC (permalink / raw) To: Jeff King, Junio C Hamano; +Cc: Remi Lespinet, Git List On Sat, Jul 25, 2015 at 2:09 AM, Jeff King <peff@peff.net> wrote: > Yeah, I think this hunk is to blame (though I just read the code and did not > test): > > @@ -658,6 +665,8 @@ fi > if test "$(cat "$dotest/threeway")" = t > then > threeway=t > +else > + threeway=f > fi > > It comes after the command-line option parsing, so it overrides our option (I > think that running "git am -3" followed by "git am --no-3way" would have the > same problem). It cannot just check whether $threeway is unset, though, as it > may have come from the config. Thanks for the detailed analysis, I completely agree. Note that the code that handles the --message-id option somewhat handles the case where $messageid is unset: case "$(cat "$dotest/messageid")" in t) messageid=-m ;; f) messageid= ;; esac However, it still does not handle "git am --no-message-id" followed by "git am --message-id", or "git -c am.messageid=true am" followed by "git am --no-message-id". I think the same thing occurs for --scissors/--no-scissors, as well as the git-apply options as well. The real problem is that the state directory loading code comes after the config loading and option parsing code, and thus overrides any variables set. > We'd need a separate variable, the way the code > is ordered now. If we are just fixing --3way, adding one extra variable won't be that bad. However, I think that if we are using this approach to fix all of the options, then it would introduce too much code complexity. > Ideally the code would just be ordered as: > > - load config from git-config > > - override that with defaults inherited from a previous run > > - override that with command-line parsing So I'm more in favor of this solution. It's feels much more natural to me, rather than attempting to workaround the existing code structure. > but I don't know if there are other ordering gotchas that would break. For the C code, there won't be any problem, but yeah, fixing it in git-am.sh might need a bit more effort. > It does look like that is how Paul's builtin/am.c does it, which makes > me think it might not be broken. It's also possibly I've horribly > misdiagnosed the bug. ;) Nah, it follows the same structure as git-am.sh and so will exhibit the same behavior. It currently does something like this: 1. am_state_init() (config settings are loaded) 2. parse_options() 3. if (am_in_progress()) am_load(); else am_setup(); So it would be quite trivial to change the control flow such that it is: 1. am_state_init() 2. if (am_in_progress()) am_load() 3. parse_options(); 4 if (!am_in_progress()) am_setup() The next question is, should any options set on the command-line affect subsequent invocations? If yes, then the control flow will be like: 1. am_state_init(); 2. if (am_in_progress()) am_load(); 3. parse_options(); 4. if (am_in_progress()) am_save_opts(); else am_setup(); where am_save_opts() will write the updated variables back to the state directory. What do you think? Since the builtin-am series is in 'next' already, and the fix in C is straightforward, to save time and effort I'm wondering if we could just do "am.threeWay patch -> builtin-am series -> bugfix patch in C". My university term is starting soon so I may not have so much time, but I'll see what I can do :-/ Junio, how do you want to proceed? Thanks, Paul ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: "git am" and then "git am -3" regression? 2015-07-26 5:03 ` Paul Tan @ 2015-07-26 5:21 ` Jeff King 2015-07-27 8:09 ` Matthieu Moy 2015-07-27 14:21 ` Junio C Hamano 1 sibling, 1 reply; 26+ messages in thread From: Jeff King @ 2015-07-26 5:21 UTC (permalink / raw) To: Paul Tan; +Cc: Junio C Hamano, Remi Lespinet, Git List On Sun, Jul 26, 2015 at 01:03:59PM +0800, Paul Tan wrote: > > Ideally the code would just be ordered as: > > > > - load config from git-config > > > > - override that with defaults inherited from a previous run > > > > - override that with command-line parsing > > So I'm more in favor of this solution. It's feels much more natural to > me, rather than attempting to workaround the existing code structure. Yeah, I really prefer it, too. I just didn't know if there would be other confusing fallouts from changing the ordering. But since you have been deep in this code recently, I trust your judgement. :) > > It does look like that is how Paul's builtin/am.c does it, which makes > > me think it might not be broken. It's also possibly I've horribly > > misdiagnosed the bug. ;) > > Nah, it follows the same structure as git-am.sh and so will exhibit > the same behavior. It currently does something like this: > > 1. am_state_init() (config settings are loaded) > 2. parse_options() > 3. if (am_in_progress()) am_load(); else am_setup(); Ah, right. I took the am_state_init() to be the part where we loaded the existing options, and didn't notice the later am_load(). > The next question is, should any options set on the command-line > affect subsequent invocations? If yes, then the control flow will be > like: > > 1. am_state_init(); > 2. if (am_in_progress()) am_load(); > 3. parse_options(); > 4. if (am_in_progress()) am_save_opts(); else am_setup(); > > where am_save_opts() will write the updated variables back to the > state directory. What do you think? I don't think we need to go that direction. The usual thought process (mine, anyway) is: 1. I want to apply a series, and I want to use option A. 2. Oops, one of the patches didn't apply. Let's retry it with option B (usually "-3"). 3. OK, that worked. Now let's try the rest of the patches. I wouldn't expect in step 3 to have options from step 2 persist. That was just about wiggling that _one_ patch. Whereas options from step 1 are about the whole series. > Since the builtin-am series is in 'next' already, and the fix in C is > straightforward, to save time and effort I'm wondering if we could > just do "am.threeWay patch -> builtin-am series -> bugfix patch in C". > My university term is starting soon so I may not have so much time, > but I'll see what I can do :-/ Yeah, having to worry about two implementations of "git am" is a real pain. If we are close on merging the builtin version, it makes sense to me to hold off on the am.threeway feature until that is merged. Trying to fix the ordering of the script that is going away isn't a good use of anybody's time. -Peff ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: "git am" and then "git am -3" regression? 2015-07-26 5:21 ` Jeff King @ 2015-07-27 8:09 ` Matthieu Moy 2015-07-27 8:32 ` Jeff King 0 siblings, 1 reply; 26+ messages in thread From: Matthieu Moy @ 2015-07-27 8:09 UTC (permalink / raw) To: Jeff King; +Cc: Paul Tan, Junio C Hamano, Remi Lespinet, Git List Jeff King <peff@peff.net> writes: > Yeah, having to worry about two implementations of "git am" is a real > pain. If we are close on merging the builtin version, it makes sense to > me to hold off on the am.threeway feature until that is merged. Trying > to fix the ordering of the script that is going away isn't a good use of > anybody's time. So, the best option seems to be: 1) Revert d96a275 (git-am: add am.threeWay config variable, 2015-06-04) 2) Include the C port of d96a275 together with tests and docs verbatim from d96a275 into Paul's series. Actually, doing 1) is probably a good idea anyway, there's no reason to hold the release for such minor feature. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: "git am" and then "git am -3" regression? 2015-07-27 8:09 ` Matthieu Moy @ 2015-07-27 8:32 ` Jeff King 0 siblings, 0 replies; 26+ messages in thread From: Jeff King @ 2015-07-27 8:32 UTC (permalink / raw) To: Matthieu Moy; +Cc: Paul Tan, Junio C Hamano, Remi Lespinet, Git List On Mon, Jul 27, 2015 at 10:09:43AM +0200, Matthieu Moy wrote: > > Yeah, having to worry about two implementations of "git am" is a real > > pain. If we are close on merging the builtin version, it makes sense to > > me to hold off on the am.threeway feature until that is merged. Trying > > to fix the ordering of the script that is going away isn't a good use of > > anybody's time. > > So, the best option seems to be: > > 1) Revert d96a275 (git-am: add am.threeWay config variable, 2015-06-04) > > 2) Include the C port of d96a275 together with tests and docs verbatim > from d96a275 into Paul's series. > > Actually, doing 1) is probably a good idea anyway, there's no reason to > hold the release for such minor feature. I think step 1 is done already for v2.5.0, in 15dc5b5. We _could_ fix it in the script version for the upcoming cycle, but if we are merging builtin-am during this cycle, I do not see the point. -Peff ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: "git am" and then "git am -3" regression? 2015-07-26 5:03 ` Paul Tan 2015-07-26 5:21 ` Jeff King @ 2015-07-27 14:21 ` Junio C Hamano 2015-07-28 16:43 ` [PATCH] am: let command-line options override saved options Paul Tan 1 sibling, 1 reply; 26+ messages in thread From: Junio C Hamano @ 2015-07-27 14:21 UTC (permalink / raw) To: Paul Tan; +Cc: Jeff King, Remi Lespinet, Git List Paul Tan <pyokagan@gmail.com> writes: > Junio, how do you want to proceed? I'd expect that builtin series would graduate in 2 releases from now at the latest, if not earlier. Let's just revert the regressing change from the scripted version and have it implemented in the builtin one in the meantime. I do not think it is worth adding features to the scripted one at this point. Thanks. ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH] am: let command-line options override saved options 2015-07-27 14:21 ` Junio C Hamano @ 2015-07-28 16:43 ` Paul Tan 2015-07-28 16:48 ` Junio C Hamano ` (3 more replies) 0 siblings, 4 replies; 26+ messages in thread From: Paul Tan @ 2015-07-28 16:43 UTC (permalink / raw) To: Junio C Hamano Cc: Jeff King, Remi Lespinet, Git List, Stefan Beller, Johannes Schindelin When resuming, git-am ignores command-line options. For instance, when a patch fails to apply with "git am patch", subsequently running "git am --3way patch" would not cause git-am to fall back on attempting a threeway merge. This occurs because by default the --3way option is saved as "false", and the saved am options are loaded after the command-line options are parsed, thus overwriting the command-line options when resuming. Fix this by moving the am_load() function call before parse_options(), so that command-line options will override the saved am options. Reported-by: Junio C Hamano <gitster@pobox.com> Helped-by: Jeff King <peff@peff.net> Signed-off-by: Paul Tan <pyokagan@gmail.com> --- builtin/am.c | 9 ++- t/t4153-am-resume-override-opts.sh | 144 +++++++++++++++++++++++++++++++++++++ 2 files changed, 150 insertions(+), 3 deletions(-) create mode 100755 t/t4153-am-resume-override-opts.sh diff --git a/builtin/am.c b/builtin/am.c index 1116304..8a0b0e4 100644 --- a/builtin/am.c +++ b/builtin/am.c @@ -2131,6 +2131,7 @@ int cmd_am(int argc, const char **argv, const char *prefix) int keep_cr = -1; int patch_format = PATCH_FORMAT_UNKNOWN; enum resume_mode resume = RESUME_FALSE; + int in_progress; const char * const usage[] = { N_("git am [options] [(<mbox>|<Maildir>)...]"), @@ -2226,6 +2227,10 @@ int cmd_am(int argc, const char **argv, const char *prefix) am_state_init(&state, git_path("rebase-apply")); + in_progress = am_in_progress(&state); + if (in_progress) + am_load(&state); + argc = parse_options(argc, argv, prefix, options, usage, 0); if (binary >= 0) @@ -2238,7 +2243,7 @@ int cmd_am(int argc, const char **argv, const char *prefix) if (read_index_preload(&the_index, NULL) < 0) die(_("failed to read the index")); - if (am_in_progress(&state)) { + if (in_progress) { /* * Catch user error to feed us patches when there is a session * in progress: @@ -2256,8 +2261,6 @@ int cmd_am(int argc, const char **argv, const char *prefix) if (resume == RESUME_FALSE) resume = RESUME_APPLY; - - am_load(&state); } else { struct argv_array paths = ARGV_ARRAY_INIT; int i; diff --git a/t/t4153-am-resume-override-opts.sh b/t/t4153-am-resume-override-opts.sh new file mode 100755 index 0000000..c49457c --- /dev/null +++ b/t/t4153-am-resume-override-opts.sh @@ -0,0 +1,144 @@ +#!/bin/sh + +test_description='git-am command-line options override saved options' + +. ./test-lib.sh + +test_expect_success 'setup' ' + test_commit initial file && + test_commit first file && + + git checkout -b side initial && + test_commit side-first file && + test_commit side-second file && + + { + echo "Message-Id: <side-first@example.com>" && + git format-patch --stdout -1 side-first | sed -e "1d" + } >side-first.patch && + { + sed -ne "1,/^\$/p" side-first.patch && + echo "-- >8 --" && + sed -e "1,/^\$/d" side-first.patch + } >side-first.scissors && + + { + echo "Message-Id: <side-second@example.com>" && + git format-patch --stdout -1 side-second | sed -e "1d" + } >side-second.patch && + { + sed -ne "1,/^\$/p" side-second.patch && + echo "-- >8 --" && + sed -e "1,/^\$/d" side-second.patch + } >side-second.scissors +' + +test_expect_success '--3way, --no-3way' ' + rm -fr .git/rebase-apply && + git reset --hard && + git checkout first && + test_must_fail git am --3way side-first.patch side-second.patch && + test -n "$(git ls-files -u)" && + echo will-conflict >file && + git add file && + test_must_fail git am --no-3way --continue && + test -z "$(git ls-files -u)" +' + +test_expect_success '--no-quiet, --quiet' ' + rm -fr .git/rebase-apply && + git reset --hard && + git checkout first && + test_must_fail git am --no-quiet side-first.patch side-second.patch && + test_must_be_empty out && + echo side-first >file && + git add file && + git am --quiet --continue >out && + test_must_be_empty out +' + +test_expect_success '--signoff, --no-signoff' ' + rm -fr .git/rebase-apply && + git reset --hard && + git checkout first && + test_must_fail git am --signoff side-first.patch side-second.patch && + echo side-first >file && + git add file && + git am --no-signoff --continue && + + # applied side-first will be signed off + echo "Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>" >expected && + git cat-file commit HEAD^ | grep "Signed-off-by:" >actual && + test_cmp expected actual && + + # applied side-second will not be signed off + test $(git cat-file commit HEAD | grep -c "Signed-off-by:") -eq 0 +' + +test_expect_success '--keep, --no-keep' ' + rm -fr .git/rebase-apply && + git reset --hard && + git checkout first && + test_must_fail git am --keep side-first.patch side-second.patch && + echo side-first >file && + git add file && + git am --no-keep --continue && + + # applied side-first will keep the subject + git cat-file commit HEAD^ >actual && + grep "^\[PATCH\] side-first" actual && + + # applied side-second will not have [PATCH] + git cat-file commit HEAD >actual && + ! grep "^\[PATCH\] side-second" actual +' + +test_expect_success '--message-id, --no-message-id' ' + rm -fr .git/rebase-apply && + git reset --hard && + git checkout first && + test_must_fail git am --message-id side-first.patch side-second.patch && + echo side-first >file && + git add file && + git am --no-message-id --continue && + + # applied side-first will have Message-Id + test -n "$(git cat-file commit HEAD^ | grep Message-Id)" && + + # applied side-second will not have Message-Id + test -z "$(git cat-file commit HEAD | grep Message-Id)" +' + +test_expect_success '--scissors, --no-scissors' ' + rm -fr .git/rebase-apply && + git reset --hard && + git checkout first && + test_must_fail git am --scissors side-first.scissors side-second.scissors && + echo side-first >file && + git add file && + git am --no-scissors --continue && + + # applied side-first will not have scissors line + git cat-file commit HEAD^ >actual && + ! grep "^-- >8 --" actual && + + # applied side-second will have scissors line + git cat-file commit HEAD >actual && + grep "^-- >8 --" actual +' + +test_expect_success '--reject, --no-reject' ' + rm -fr .git/rebase-apply && + git reset --hard && + git checkout first && + rm -f file.rej && + test_must_fail git am --reject side-first.patch side-second.patch && + test_path_is_file file.rej && + rm -f file.rej && + echo will-conflict >file && + git add file && + test_must_fail git am --no-reject --continue && + test_path_is_missing file.rej +' + +test_done -- 2.5.0.77.gd180035 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH] am: let command-line options override saved options 2015-07-28 16:43 ` [PATCH] am: let command-line options override saved options Paul Tan @ 2015-07-28 16:48 ` Junio C Hamano 2015-07-28 17:09 ` Junio C Hamano ` (2 subsequent siblings) 3 siblings, 0 replies; 26+ messages in thread From: Junio C Hamano @ 2015-07-28 16:48 UTC (permalink / raw) To: Paul Tan Cc: Jeff King, Remi Lespinet, Git List, Stefan Beller, Johannes Schindelin Paul Tan <pyokagan@gmail.com> writes: > When resuming, git-am ignores command-line options. For instance, when a > patch fails to apply with "git am patch", subsequently running "git am > --3way patch" would not cause git-am to fall back on attempting a The second one goes without any file argument, i.e. "git am -3". > threeway merge. This occurs because by default the --3way option is > saved as "false", and the saved am options are loaded after the > command-line options are parsed, thus overwriting the command-line > options when resuming. > > Fix this by moving the am_load() function call before parse_options(), > so that command-line options will override the saved am options. Makes sense. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] am: let command-line options override saved options 2015-07-28 16:43 ` [PATCH] am: let command-line options override saved options Paul Tan 2015-07-28 16:48 ` Junio C Hamano @ 2015-07-28 17:09 ` Junio C Hamano 2015-07-31 10:58 ` Paul Tan 2015-08-04 14:05 ` [PATCH v2 0/3] " Paul Tan 2015-08-04 14:08 ` Paul Tan 3 siblings, 1 reply; 26+ messages in thread From: Junio C Hamano @ 2015-07-28 17:09 UTC (permalink / raw) To: Paul Tan Cc: Jeff King, Remi Lespinet, Git List, Stefan Beller, Johannes Schindelin Paul Tan <pyokagan@gmail.com> writes: > diff --git a/t/t4153-am-resume-override-opts.sh b/t/t4153-am-resume-override-opts.sh > new file mode 100755 > index 0000000..c49457c > --- /dev/null > +++ b/t/t4153-am-resume-override-opts.sh > @@ -0,0 +1,144 @@ > +#!/bin/sh > + > +test_description='git-am command-line options override saved options' > + > +. ./test-lib.sh > + > +test_expect_success 'setup' ' > + test_commit initial file && > + test_commit first file && > + > + git checkout -b side initial && > + test_commit side-first file && > + test_commit side-second file && > + > + { > + echo "Message-Id: <side-first@example.com>" && > + git format-patch --stdout -1 side-first | sed -e "1d" > + } >side-first.patch && Hmm, puzzled... Ah, you want to make sure Message-Id comes at the very beginning, and you are going to use a single e-mail per mailbox so it is easier to strip the Beginning of Message marker than to insert Message-Id after it. I can understand what is going on. > + { > + sed -ne "1,/^\$/p" side-first.patch && sed -e "/^\$/q" would work just as well here > + echo "-- >8 --" && > + sed -e "1,/^\$/d" side-first.patch > + } >side-first.scissors && So *.scissors version has -- >8 -- inserted at the beginning of the body. > + { > + echo "Message-Id: <side-second@example.com>" && > + git format-patch --stdout -1 side-second | sed -e "1d" > + } >side-second.patch && > + { > + sed -ne "1,/^\$/p" side-second.patch && > + echo "-- >8 --" && > + sed -e "1,/^\$/d" side-second.patch > + } >side-second.scissors > +' A helper function that takes the branch name may be a good idea, not just to consolidate the implementation but as a place to document how these pairs of files are constructed and why. > +test_expect_success '--3way, --no-3way' ' > + rm -fr .git/rebase-apply && > + git reset --hard && > + git checkout first && > + test_must_fail git am --3way side-first.patch side-second.patch && > + test -n "$(git ls-files -u)" && > + echo will-conflict >file && > + git add file && > + test_must_fail git am --no-3way --continue && > + test -z "$(git ls-files -u)" > +' > + > +test_expect_success '--no-quiet, --quiet' ' > + rm -fr .git/rebase-apply && > + git reset --hard && > + git checkout first && > + test_must_fail git am --no-quiet side-first.patch side-second.patch && > + test_must_be_empty out && Where did this 'out' come from? > + echo side-first >file && > + git add file && > + git am --quiet --continue >out && > + test_must_be_empty out I can see this one, though I am not sure if it is sensible to see what the command says under --quiet option, especially if you are making sure it does not fail, which you already have checked for its exit status. > +' > + > +test_expect_success '--signoff, --no-signoff' ' > + rm -fr .git/rebase-apply && > + git reset --hard && > + git checkout first && > + test_must_fail git am --signoff side-first.patch side-second.patch && > + echo side-first >file && > + git add file && > + git am --no-signoff --continue && > + > + # applied side-first will be signed off > + echo "Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>" >expected && > + git cat-file commit HEAD^ | grep "Signed-off-by:" >actual && > + test_cmp expected actual && > + > + # applied side-second will not be signed off > + test $(git cat-file commit HEAD | grep -c "Signed-off-by:") -eq 0 > +' Hmm, the command was run with --signoff at the start, first gets applied with "am --no-signoff --resolved" so I would expect it does not get signed off, but the second one will apply cleanly on top, so shouldn't it get signed off? Or perhaps somehow I misread Peff's idea to make these override one-shot in $gmane/274635? ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] am: let command-line options override saved options 2015-07-28 17:09 ` Junio C Hamano @ 2015-07-31 10:58 ` Paul Tan 2015-07-31 16:04 ` Junio C Hamano 0 siblings, 1 reply; 26+ messages in thread From: Paul Tan @ 2015-07-31 10:58 UTC (permalink / raw) To: Junio C Hamano Cc: Jeff King, Remi Lespinet, Git List, Stefan Beller, Johannes Schindelin On Wed, Jul 29, 2015 at 1:09 AM, Junio C Hamano <gitster@pobox.com> wrote: > Paul Tan <pyokagan@gmail.com> writes: > >> diff --git a/t/t4153-am-resume-override-opts.sh b/t/t4153-am-resume-override-opts.sh >> new file mode 100755 >> index 0000000..c49457c >> --- /dev/null >> +++ b/t/t4153-am-resume-override-opts.sh >> @@ -0,0 +1,144 @@ >> +#!/bin/sh >> + >> +test_description='git-am command-line options override saved options' >> + >> +. ./test-lib.sh >> + >> +test_expect_success 'setup' ' >> + test_commit initial file && >> + test_commit first file && >> + >> + git checkout -b side initial && >> + test_commit side-first file && >> + test_commit side-second file && >> + >> + { >> + echo "Message-Id: <side-first@example.com>" && >> + git format-patch --stdout -1 side-first | sed -e "1d" >> + } >side-first.patch && >> + { >> + sed -ne "1,/^\$/p" side-first.patch && > > sed -e "/^\$/q" would work just as well here OK. >> + echo "-- >8 --" && >> + sed -e "1,/^\$/d" side-first.patch >> + } >side-first.scissors && >> + { >> + echo "Message-Id: <side-second@example.com>" && >> + git format-patch --stdout -1 side-second | sed -e "1d" >> + } >side-second.patch && >> + { >> + sed -ne "1,/^\$/p" side-second.patch && >> + echo "-- >8 --" && >> + sed -e "1,/^\$/d" side-second.patch >> + } >side-second.scissors >> +' > > A helper function that takes the branch name may be a good idea, > not just to consolidate the implementation but as a place to > document how these pairs of files are constructed and why. I think I will introduce a format_patch() function that takes a single commit-ish so that we can use tag names to name the patches: # Given a single commit $commit, formats the following patches with # git-format-patch: # # 1. $commit.eml: an email patch with a Message-Id header. # 2. $commit.scissors: like $commit.eml but contains a scissors line at the # start of the commit message body. format_patch () { { echo "Message-Id: <$1@example.com>" && git format-patch --stdout -1 "$1" | sed -e '1d' } >"$1".eml && { sed -e '/^$/q' "$1".eml && echo '-- >8 --' && sed -e '1,/^$/d' "$1".eml } >"$1".scissors } >> +' >> + >> +test_expect_success '--signoff, --no-signoff' ' >> + rm -fr .git/rebase-apply && >> + git reset --hard && >> + git checkout first && >> + test_must_fail git am --signoff side-first.patch side-second.patch && >> + echo side-first >file && >> + git add file && >> + git am --no-signoff --continue && >> + >> + # applied side-first will be signed off >> + echo "Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>" >expected && >> + git cat-file commit HEAD^ | grep "Signed-off-by:" >actual && >> + test_cmp expected actual && >> + >> + # applied side-second will not be signed off >> + test $(git cat-file commit HEAD | grep -c "Signed-off-by:") -eq 0 >> +' > > Hmm, the command was run with --signoff at the start, first gets > applied with "am --no-signoff --resolved" so I would expect it does > not get signed off, but the second one will apply cleanly on top, so > shouldn't it get signed off? Or perhaps somehow I misread Peff's > idea to make these override one-shot in $gmane/274635? Ah, I was just following the structure of the code, but stepping back to think about it, I think there are 2 bugs: 1. The signoff is appended during the email-parsing stage. As such, when we are resuming, --no-signoff will have no effect, because the signoff has already been appended at that stage. A solution for this is tricky though, as there are functions of git-am that probably depend on the present behavior of the appended signoff being present in the commit message: * The applypatch-msg hook * The --interactive prompt, where the user can edit the commit message (to remove or edit the signoff maybe?) These functions are called before we attempt to apply the patch, so we should probably call append_signoff before then. However, this still means that --no-signoff will have no effect should the patch application fail and we resume, as the signoff would still have already been appended... So I dunno. I think the cleanest solution would be to change the behavior so the commit message passed to the applypatch-msg hook and --interactive prompt do not contain the appended signoff, and instead only append the signoff just before we commit. That way, both --signoff and --no-signoff overriding will work. What do you think? 2. Re-reading Peff's message, I see that he expects the command-line options to affect just the current patch, which makes sense. This patch would need to be extended to call am_load() after we finish processing the current patch when resuming. Something like: diff --git a/builtin/am.c b/builtin/am.c index 8a0b0e4..228d4b1 100644 --- a/builtin/am.c +++ b/builtin/am.c @@ -1779,7 +1779,6 @@ static void am_run(struct am_state *state, int resume) if (resume) { validate_resume_state(state); - resume = 0; } else { int skip; @@ -1841,6 +1840,10 @@ static void am_run(struct am_state *state, int resume) next: am_next(state); + + if (resume) + am_load(state); + resume = 0; } if (!is_empty_file(am_path(state, "rewritten"))) { @@ -1895,6 +1898,7 @@ static void am_resolve(struct am_state *state) next: am_next(state); + am_load(state); am_run(state, 0); } @@ -2022,6 +2026,7 @@ static void am_skip(struct am_state *state) die(_("failed to clean index")); am_next(state); + am_load(state); am_run(state, 0); } The tests will also need to be modified as well. >> +test_expect_success '--3way, --no-3way' ' >> + rm -fr .git/rebase-apply && >> + git reset --hard && >> + git checkout first && >> + test_must_fail git am --3way side-first.patch side-second.patch && >> + test -n "$(git ls-files -u)" && >> + echo will-conflict >file && >> + git add file && >> + test_must_fail git am --no-3way --continue && >> + test -z "$(git ls-files -u)" >> +' >> + ... Although if I implement the above change, I can't implement the test for --3way, as I think the only way to check if --3way/--no-3way successfully overrides the saved options for the current patch only is to run "git am --3way", but that does not work in the test runner as it expects stdin to be a TTY :-/ So I may have to remove this test. This shouldn't be a problem though, as all the tests in this test suite all test the same mechanism. >> +test_expect_success '--no-quiet, --quiet' ' >> + rm -fr .git/rebase-apply && >> + git reset --hard && >> + git checkout first && >> + test_must_fail git am --no-quiet side-first.patch side-second.patch && >> + test_must_be_empty out && > > Where did this 'out' come from? It was a leftover from a previous iteration. Will fix. > >> + echo side-first >file && >> + git add file && >> + git am --quiet --continue >out && >> + test_must_be_empty out > > I can see this one, though I am not sure if it is sensible to see > what the command says under --quiet option, especially if you are > making sure it does not fail, which you already have checked for its > exit status. Well, if --quiet fails to override --no-quiet, then there would be something written to the stdout, no? But anyway, if we are implementing the above "command-line option overriding only affects current patch" behavior, then this test would be checking if --quiet only affects a single patch, which in practice would be quite silly. Maybe I should flip it around to "--no-quiet overrides --quiet", but even then it may still be unlikely to come up with practice... Still, it may be useful to keep this test to check if the option overriding mechanism is working properly. Thanks, Paul ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH] am: let command-line options override saved options 2015-07-31 10:58 ` Paul Tan @ 2015-07-31 16:04 ` Junio C Hamano 2015-08-01 0:59 ` Paul Tan 0 siblings, 1 reply; 26+ messages in thread From: Junio C Hamano @ 2015-07-31 16:04 UTC (permalink / raw) To: Paul Tan Cc: Jeff King, Remi Lespinet, Git List, Stefan Beller, Johannes Schindelin Paul Tan <pyokagan@gmail.com> writes: > I think I will introduce a format_patch() function that takes a single > commit-ish so that we can use tag names to name the patches: > > # Given a single commit $commit, formats the following patches with > # git-format-patch: > # > # 1. $commit.eml: an email patch with a Message-Id header. > # 2. $commit.scissors: like $commit.eml but contains a scissors line at the > # start of the commit message body. > format_patch () { > { > echo "Message-Id: <$1@example.com>" && > git format-patch --stdout -1 "$1" | sed -e '1d' > } >"$1".eml && I only said I can "understand" what is going on, though. It feels a bit unnatural for a test to feed a message that lack the "From " header line. Perhaps git format-patch --add-header="Message-Id: ..." --stdout -1 or something? > Ah, I was just following the structure of the code, but stepping back > to think about it, I think there are 2 bugs: > > 1. The signoff is appended during the email-parsing stage. As such, > when we are resuming, --no-signoff will have no effect, because the > signoff has already been appended at that stage. > > A solution for this is tricky though, as there are functions of git-am > that probably depend on the present behavior of the appended signoff > being present in the commit message: > > * The applypatch-msg hook > > * The --interactive prompt, where the user can edit the commit message > (to remove or edit the signoff maybe?) > > These functions are called before we attempt to apply the patch, so we > should probably call append_signoff before then. However, this still > means that --no-signoff will have no effect should the patch > application fail and we resume, as the signoff would still have > already been appended... Ah, I see. Let's not worry about this; we cannot change the expectation existing hook scripts depends on. > 2. Re-reading Peff's message, I see that he expects the command-line > options to affect just the current patch, which makes sense. This > patch would need to be extended to call am_load() after we finish > processing the current patch when resuming. Yeah, so the idea is: - upon the very first invocation, we parse the command line options and write the states out; - subsequent invocation, we read from the states and then override with the command line options, but we do not write the states out to update, so that subsequent invocations will keep reading from the very first one. That sounds sensible. > The tests will also need to be modified as well. > >>> +test_expect_success '--3way, --no-3way' ' >>> + rm -fr .git/rebase-apply && >>> + git reset --hard && >>> + git checkout first && >>> + test_must_fail git am --3way side-first.patch side-second.patch && >>> + test -n "$(git ls-files -u)" && >>> + echo will-conflict >file && >>> + git add file && >>> + test_must_fail git am --no-3way --continue && >>> + test -z "$(git ls-files -u)" >>> +' >>> + > > ... Although if I implement the above change, I can't implement the > test for --3way, as I think the only way to check if --3way/--no-3way > successfully overrides the saved options for the current patch only is > to run "git am --3way", but that does not work in the test runner as > it expects stdin to be a TTY :-/ So I may have to remove this test. > This shouldn't be a problem though, as all the tests in this test > suite all test the same mechanism. Sorry, you lost me. Where does the TTY come into the picture only for --3way (but not for other things like --quiet)? ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH] am: let command-line options override saved options 2015-07-31 16:04 ` Junio C Hamano @ 2015-08-01 0:59 ` Paul Tan 0 siblings, 0 replies; 26+ messages in thread From: Paul Tan @ 2015-08-01 0:59 UTC (permalink / raw) To: Junio C Hamano Cc: Jeff King, Remi Lespinet, Git List, Stefan Beller, Johannes Schindelin On Sat, Aug 1, 2015 at 12:04 AM, Junio C Hamano <gitster@pobox.com> wrote: > Paul Tan <pyokagan@gmail.com> writes: > >> I think I will introduce a format_patch() function that takes a single >> commit-ish so that we can use tag names to name the patches: >> >> # Given a single commit $commit, formats the following patches with >> # git-format-patch: >> # >> # 1. $commit.eml: an email patch with a Message-Id header. >> # 2. $commit.scissors: like $commit.eml but contains a scissors line at the >> # start of the commit message body. >> format_patch () { >> { >> echo "Message-Id: <$1@example.com>" && >> git format-patch --stdout -1 "$1" | sed -e '1d' >> } >"$1".eml && > > I only said I can "understand" what is going on, though. > > It feels a bit unnatural for a test to feed a message that lack the > "From " header line. Perhaps > > git format-patch --add-header="Message-Id: ..." --stdout -1 > > or something? Ah, okay. I wasn't aware of the --add-header option, but this is definitely better. >> These functions are called before we attempt to apply the patch, so we >> should probably call append_signoff before then. However, this still >> means that --no-signoff will have no effect should the patch >> application fail and we resume, as the signoff would still have >> already been appended... > > Ah, I see. Let's not worry about this; we cannot change the > expectation existing hook scripts depends on. Okay, although this means that with the below change, --[no-]signoff will be the oddball option that does not work when resuming. >> 2. Re-reading Peff's message, I see that he expects the command-line >> options to affect just the current patch, which makes sense. This >> patch would need to be extended to call am_load() after we finish >> processing the current patch when resuming. > > Yeah, so the idea is: > > - upon the very first invocation, we parse the command line options > and write the states out; > > - subsequent invocation, we read from the states and then override > with the command line options, but we do not write the states out > to update, so that subsequent invocations will keep reading from > the very first one. ... and we also load back the saved options after processing the patch that we resume from, so the command-line options only affect the conflicting patch, which fits in with Peff's idea on "wiggling that _one_ patch". >>>> +test_expect_success '--3way, --no-3way' ' >>>> + rm -fr .git/rebase-apply && >>>> + git reset --hard && >>>> + git checkout first && >>>> + test_must_fail git am --3way side-first.patch side-second.patch && >>>> + test -n "$(git ls-files -u)" && >>>> + echo will-conflict >file && >>>> + git add file && >>>> + test_must_fail git am --no-3way --continue && >>>> + test -z "$(git ls-files -u)" >>>> +' >>>> + >> >> ... Although if I implement the above change, I can't implement the >> test for --3way, as I think the only way to check if --3way/--no-3way >> successfully overrides the saved options for the current patch only is >> to run "git am --3way", but that does not work in the test runner as >> it expects stdin to be a TTY :-/ So I may have to remove this test. >> This shouldn't be a problem though, as all the tests in this test >> suite all test the same mechanism. > > Sorry, you lost me. Where does the TTY come into the picture only > for --3way (but not for other things like --quiet)? Ah, sorry, I should have provided more context. This is due to the following block of code: /* * Catch user error to feed us patches when there is a session * in progress: * * 1. mbox path(s) are provided on the command-line. * 2. stdin is not a tty: the user is trying to feed us a patch * from standard input. This is somewhat unreliable -- stdin * could be /dev/null for example and the caller did not * intend to feed us a patch but wanted to continue * unattended. */ if (argc || (resume == RESUME_FALSE && !isatty(0))) die(_("previous rebase directory %s still exists but mbox given."), state.dir); And it will activate when git-am is run without --continue/--abort/--skip (e.g. "git am --3way") because the test framework sets stdin to /dev/null. Thanks, Paul ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v2 0/3] am: let command-line options override saved options 2015-07-28 16:43 ` [PATCH] am: let command-line options override saved options Paul Tan 2015-07-28 16:48 ` Junio C Hamano 2015-07-28 17:09 ` Junio C Hamano @ 2015-08-04 14:05 ` Paul Tan 2015-08-04 21:12 ` Junio C Hamano 2015-08-04 14:08 ` Paul Tan 3 siblings, 1 reply; 26+ messages in thread From: Paul Tan @ 2015-08-04 14:05 UTC (permalink / raw) To: git Cc: Junio C Hamano, Johannes Schindelin, Stefan Beller, Remi Lespinet, Jeff King, Paul Tan Let command-line options override saved options in git-am when resuming This is a re-roll of [v1]. Previous versions: [v1] http://thread.gmane.org/gmane.comp.version-control.git/274789 When resuming, git-am mistakenly ignores command-line options. For instance, when a patch fails to apply with "git am patch", subsequently running "git am --3way" would not cause git-am to fall back on attempting a threeway merge. This occurs because by default the --3way option is saved as "false", and the saved am options are loaded after the command-line options are parsed, thus overwriting the command-line options when resuming. [PATCH 1/3] tweaks test-terminal.perl to redirect the stdin of the child process to a pty. This is to support the tests in [PATCH 2/3]. [PATCH 2/3] fixes builtin/am.c, enabling command-line options to override saved options. However, even with this patch, the following command-line options have no effect when resuming: * --signoff overriding --no-signoff * --no-keep overriding --keep * --message-id overriding --no-message-id * --scissors overriding --no-scissors This is because they are only taken into account during the mail-parsing stage, which is skipped over when resuming. [PATCH 3/3] adds support for the --signoff option when resuming by recognizing that we can (re-)append the signoff when the user explicitly specifies the --signoff option. Since the --keep, --message-id and --scissors options are handled by git-mailinfo, it is tricky to implement support for them without introducing lots of code complexity, and thus this patch series does not attempt to. Furthermore, it is hard to imagine a use case for e.g. --scissors overriding --no-scissors, and hence it might be preferable to wait until someone comes with a solid use case, instead of implementing potentially undesirable behavior and having to support it. Paul Tan (3): test_terminal: redirect child process' stdin to a pty am: let command-line options override saved options am: let --signoff override --no-signoff builtin/am.c | 42 ++++++++++++--- t/t4153-am-resume-override-opts.sh | 102 +++++++++++++++++++++++++++++++++++++ t/test-terminal.perl | 25 +++++++-- 3 files changed, 158 insertions(+), 11 deletions(-) create mode 100755 t/t4153-am-resume-override-opts.sh -- 2.5.0.280.gd88bd6e ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 0/3] am: let command-line options override saved options 2015-08-04 14:05 ` [PATCH v2 0/3] " Paul Tan @ 2015-08-04 21:12 ` Junio C Hamano 0 siblings, 0 replies; 26+ messages in thread From: Junio C Hamano @ 2015-08-04 21:12 UTC (permalink / raw) To: Paul Tan Cc: git, Johannes Schindelin, Stefan Beller, Remi Lespinet, Jeff King Paul Tan <pyokagan@gmail.com> writes: > Let command-line options override saved options in git-am when resuming > > This is a re-roll of [v1]. Previous versions: > > [v1] http://thread.gmane.org/gmane.comp.version-control.git/274789 > > When resuming, git-am mistakenly ignores command-line options. > > For instance, when a patch fails to apply with "git am patch", subsequently > running "git am --3way" would not cause git-am to fall back on attempting a > threeway merge. This occurs because by default the --3way option is saved as > "false", and the saved am options are loaded after the command-line options are > parsed, thus overwriting the command-line options when resuming. > > [PATCH 1/3] tweaks test-terminal.perl to redirect the stdin of the child > process to a pty. This is to support the tests in [PATCH 2/3]. > > [PATCH 2/3] fixes builtin/am.c, enabling command-line options to override saved > options. However, even with this patch, the following command-line options have > no effect when resuming: > > * --signoff overriding --no-signoff > > * --no-keep overriding --keep > > * --message-id overriding --no-message-id > > * --scissors overriding --no-scissors > > This is because they are only taken into account during the mail-parsing stage, > which is skipped over when resuming. It is more like "which has already happened", so I would tend to think that these are the right things to ignore. Otherwise, a pretty common sequence would not work well ... $ git am mbox ... conflicted ... $ edit .git/rebase-apply/patch $ git am ... if the "resuming" invocation re-split the message by running mailinfo again, the edit by the user will be lost. ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v2 0/3] am: let command-line options override saved options 2015-07-28 16:43 ` [PATCH] am: let command-line options override saved options Paul Tan ` (2 preceding siblings ...) 2015-08-04 14:05 ` [PATCH v2 0/3] " Paul Tan @ 2015-08-04 14:08 ` Paul Tan 2015-08-04 14:08 ` [PATCH v2 1/3] test_terminal: redirect child process' stdin to a pty Paul Tan ` (3 more replies) 3 siblings, 4 replies; 26+ messages in thread From: Paul Tan @ 2015-08-04 14:08 UTC (permalink / raw) To: git Cc: Junio C Hamano, Johannes Schindelin, Stefan Beller, Remi Lespinet, Jeff King, Paul Tan Let command-line options override saved options in git-am when resuming This is a re-roll of [v1]. Previous versions: [v1] http://thread.gmane.org/gmane.comp.version-control.git/274789 When resuming, git-am mistakenly ignores command-line options. For instance, when a patch fails to apply with "git am patch", subsequently running "git am --3way" would not cause git-am to fall back on attempting a threeway merge. This occurs because by default the --3way option is saved as "false", and the saved am options are loaded after the command-line options are parsed, thus overwriting the command-line options when resuming. [PATCH 1/3] tweaks test-terminal.perl to redirect the stdin of the child process to a pty. This is to support the tests in [PATCH 2/3]. [PATCH 2/3] fixes builtin/am.c, enabling command-line options to override saved options. However, even with this patch, the following command-line options have no effect when resuming: * --signoff overriding --no-signoff * --no-keep overriding --keep * --message-id overriding --no-message-id * --scissors overriding --no-scissors This is because they are only taken into account during the mail-parsing stage, which is skipped over when resuming. [PATCH 3/3] adds support for the --signoff option when resuming by recognizing that we can (re-)append the signoff when the user explicitly specifies the --signoff option. Since the --keep, --message-id and --scissors options are handled by git-mailinfo, it is tricky to implement support for them without introducing lots of code complexity, and thus this patch series does not attempt to. Furthermore, it is hard to imagine a use case for e.g. --scissors overriding --no-scissors, and hence it might be preferable to wait until someone comes with a solid use case, instead of implementing potentially undesirable behavior and having to support it. Paul Tan (3): test_terminal: redirect child process' stdin to a pty am: let command-line options override saved options am: let --signoff override --no-signoff builtin/am.c | 42 ++++++++++++--- t/t4153-am-resume-override-opts.sh | 102 +++++++++++++++++++++++++++++++++++++ t/test-terminal.perl | 25 +++++++-- 3 files changed, 158 insertions(+), 11 deletions(-) create mode 100755 t/t4153-am-resume-override-opts.sh -- 2.5.0.280.gd88bd6e ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v2 1/3] test_terminal: redirect child process' stdin to a pty 2015-08-04 14:08 ` Paul Tan @ 2015-08-04 14:08 ` Paul Tan 2015-08-06 22:15 ` Eric Sunshine 2015-08-04 14:08 ` [PATCH v2 2/3] am: let command-line options override saved options Paul Tan ` (2 subsequent siblings) 3 siblings, 1 reply; 26+ messages in thread From: Paul Tan @ 2015-08-04 14:08 UTC (permalink / raw) To: git Cc: Junio C Hamano, Johannes Schindelin, Stefan Beller, Remi Lespinet, Jeff King, Paul Tan, Jonathan Nieder When resuming, git-am detects if we are trying to feed it patches or not by checking if stdin is a TTY. However, the test library redirects stdin to /dev/null. This makes it difficult, for instance, to test the behavior of "git am -3" when resuming, as git-am will think we are trying to feed it patches and error out. Support this use case by extending test-terminal.perl to create a pseudo-tty for the child process' standard input as well. Note that due to the way the code is structured, the child's stdin pseudo-tty will be closed when we finish reading from our stdin. This means that in the common case, where our stdin is attached to /dev/null, the child's stdin pseudo-tty will be closed immediately. Some operations like isatty(), which git-am uses, require the file descriptor to be open, and hence if the success of the command depends on such functions, test_terminal's stdin should be redirected to a source with large amount of data to ensure that the child's stdin is not closed, e.g. test_terminal git am --3way </dev/zero Cc: Jonathan Nieder <jrnieder@gmail.com> Cc: Jeff King <peff@peff.net> Signed-off-by: Paul Tan <pyokagan@gmail.com> --- t/test-terminal.perl | 25 ++++++++++++++++++++----- 1 file changed, 20 insertions(+), 5 deletions(-) diff --git a/t/test-terminal.perl b/t/test-terminal.perl index 1fb373f..f6fc9ae 100755 --- a/t/test-terminal.perl +++ b/t/test-terminal.perl @@ -5,15 +5,17 @@ use warnings; use IO::Pty; use File::Copy; -# Run @$argv in the background with stdio redirected to $out and $err. +# Run @$argv in the background with stdio redirected to $in, $out and $err. sub start_child { - my ($argv, $out, $err) = @_; + my ($argv, $in, $out, $err) = @_; my $pid = fork; if (not defined $pid) { die "fork failed: $!" } elsif ($pid == 0) { + open STDIN, "<&", $in; open STDOUT, ">&", $out; open STDERR, ">&", $err; + close $in; close $out; exec(@$argv) or die "cannot exec '$argv->[0]': $!" } @@ -50,14 +52,23 @@ sub xsendfile { } sub copy_stdio { - my ($out, $err) = @_; + my ($in, $out, $err) = @_; my $pid = fork; + if (!$pid) { + close($out); + close($err); + xsendfile($in, \*STDIN); + exit 0; + } + $pid = fork; defined $pid or die "fork failed: $!"; if (!$pid) { + close($in); close($out); xsendfile(\*STDERR, $err); exit 0; } + close($in); close($err); xsendfile(\*STDOUT, $out); finish_child($pid) == 0 @@ -67,14 +78,18 @@ sub copy_stdio { if ($#ARGV < 1) { die "usage: test-terminal program args"; } +my $master_in = new IO::Pty; my $master_out = new IO::Pty; my $master_err = new IO::Pty; +$master_in->set_raw(); $master_out->set_raw(); $master_err->set_raw(); +$master_in->slave->set_raw(); $master_out->slave->set_raw(); $master_err->slave->set_raw(); -my $pid = start_child(\@ARGV, $master_out->slave, $master_err->slave); +my $pid = start_child(\@ARGV, $master_in->slave, $master_out->slave, $master_err->slave); +close $master_in->slave; close $master_out->slave; close $master_err->slave; -copy_stdio($master_out, $master_err); +copy_stdio($master_in, $master_out, $master_err); exit(finish_child($pid)); -- 2.5.0.280.gd88bd6e ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH v2 1/3] test_terminal: redirect child process' stdin to a pty 2015-08-04 14:08 ` [PATCH v2 1/3] test_terminal: redirect child process' stdin to a pty Paul Tan @ 2015-08-06 22:15 ` Eric Sunshine 2015-08-12 4:16 ` Paul Tan 0 siblings, 1 reply; 26+ messages in thread From: Eric Sunshine @ 2015-08-06 22:15 UTC (permalink / raw) To: Paul Tan Cc: Git List, Junio C Hamano, Johannes Schindelin, Stefan Beller, Remi Lespinet, Jeff King, Jonathan Nieder On Tue, Aug 4, 2015 at 10:08 AM, Paul Tan <pyokagan@gmail.com> wrote: > When resuming, git-am detects if we are trying to feed it patches or not > by checking if stdin is a TTY. > > However, the test library redirects stdin to /dev/null. This makes it > difficult, for instance, to test the behavior of "git am -3" when > resuming, as git-am will think we are trying to feed it patches and > error out. > > Support this use case by extending test-terminal.perl to create a > pseudo-tty for the child process' standard input as well. An alternative would be to have git-am detect that it is being tested and pretend that isatty() returns true. There is some precedent for having core functionality recognize that it is being tested. See, for instance, environment variable TEST_DATE_NOW, and rev-list --test-bitmap. Doing so would allow the tests work on non-Unix platforms, as well. > Note that due to the way the code is structured, the child's stdin > pseudo-tty will be closed when we finish reading from our stdin. This > means that in the common case, where our stdin is attached to /dev/null, > the child's stdin pseudo-tty will be closed immediately. Some operations > like isatty(), which git-am uses, require the file descriptor to be > open, and hence if the success of the command depends on such functions, > test_terminal's stdin should be redirected to a source with large amount > of data to ensure that the child's stdin is not closed, e.g. > > test_terminal git am --3way </dev/zero > > Signed-off-by: Paul Tan <pyokagan@gmail.com> ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 1/3] test_terminal: redirect child process' stdin to a pty 2015-08-06 22:15 ` Eric Sunshine @ 2015-08-12 4:16 ` Paul Tan 0 siblings, 0 replies; 26+ messages in thread From: Paul Tan @ 2015-08-12 4:16 UTC (permalink / raw) To: Eric Sunshine Cc: Git List, Junio C Hamano, Johannes Schindelin, Stefan Beller, Remi Lespinet, Jeff King, Jonathan Nieder On Fri, Aug 7, 2015 at 6:15 AM, Eric Sunshine <sunshine@sunshineco.com> wrote: > An alternative would be to have git-am detect that it is being tested > and pretend that isatty() returns true. I would vastly prefer a solution that would work for everything, for all the C code and scripts, instead of implementing a workaround in git-am :( In this case, I implemented a generic solution in test-terminal.perl that works for POSIX systems, so if there are no problems with its implementation, I do think it's better. Other than the fact that it does not work on non-Unix platforms, of course. The other approach I would consider is to implement a xisatty() function that returns true for xisatty(0) if TEST_TTY=0 or something. However, I do wonder if this would lead us to have to hack around other functions of terminals as well (e.g. if xisatty(0), tcgetattr()), which would be a big can of worms I think... > There is some precedent for > having core functionality recognize that it is being tested. See, for > instance, environment variable TEST_DATE_NOW, (Hmm, I took a look, and it seems that TEST_DATE_NOW is only checked in test-date.c...) > and rev-list --test-bitmap. > Doing so would allow the tests work on non-Unix > platforms, as well. Ehh, if the non-Unix platforms do not implement terminals, it means that the git-am logic to detect if we are attempting to feed it a patch by checking if stdin is a TTY is invalid anyway, so implementing a "yeah-it-is-a-tty" workaround for the sake of tests would be hiding the problem, I think. Thanks, Paul ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v2 2/3] am: let command-line options override saved options 2015-08-04 14:08 ` Paul Tan 2015-08-04 14:08 ` [PATCH v2 1/3] test_terminal: redirect child process' stdin to a pty Paul Tan @ 2015-08-04 14:08 ` Paul Tan 2015-08-04 14:08 ` [PATCH v2 3/3] am: let --signoff override --no-signoff Paul Tan 2015-08-05 15:41 ` [PATCH v2 0/3] am: let command-line options override saved options Junio C Hamano 3 siblings, 0 replies; 26+ messages in thread From: Paul Tan @ 2015-08-04 14:08 UTC (permalink / raw) To: git Cc: Junio C Hamano, Johannes Schindelin, Stefan Beller, Remi Lespinet, Jeff King, Paul Tan When resuming, git-am mistakenly ignores command-line options. For instance, when a patch fails to apply with "git am patch", subsequently running "git am --3way" would not cause git-am to fall back on attempting a threeway merge. This occurs because by default the --3way option is saved as "false", and the saved am options are loaded after the command-line options are parsed, thus overwriting the command-line options when resuming. Fix this by moving the am_load() function call before parse_options(), so that command-line options will override the saved am options. The purpose of supporting this use case is to enable users to "wiggle" that one conflicting patch. As such, it is expected that the command-line options do not affect subsequent applied patches. Implement this by calling am_load() once we apply the conflicting patch successfully. Noticed-by: Junio C Hamano <gitster@pobox.com> Helped-by: Junio C Hamano <gitster@pobox.com> Helped-by: Jeff King <peff@peff.net> Signed-off-by: Paul Tan <pyokagan@gmail.com> --- builtin/am.c | 16 ++++++-- t/t4153-am-resume-override-opts.sh | 82 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 94 insertions(+), 4 deletions(-) create mode 100755 t/t4153-am-resume-override-opts.sh diff --git a/builtin/am.c b/builtin/am.c index 84d57d4..0961304 100644 --- a/builtin/am.c +++ b/builtin/am.c @@ -1777,7 +1777,6 @@ static void am_run(struct am_state *state, int resume) if (resume) { validate_resume_state(state); - resume = 0; } else { int skip; @@ -1839,6 +1838,10 @@ static void am_run(struct am_state *state, int resume) next: am_next(state); + + if (resume) + am_load(state); + resume = 0; } if (!is_empty_file(am_path(state, "rewritten"))) { @@ -1893,6 +1896,7 @@ static void am_resolve(struct am_state *state) next: am_next(state); + am_load(state); am_run(state, 0); } @@ -2020,6 +2024,7 @@ static void am_skip(struct am_state *state) die(_("failed to clean index")); am_next(state); + am_load(state); am_run(state, 0); } @@ -2130,6 +2135,7 @@ int cmd_am(int argc, const char **argv, const char *prefix) int keep_cr = -1; int patch_format = PATCH_FORMAT_UNKNOWN; enum resume_mode resume = RESUME_FALSE; + int in_progress; const char * const usage[] = { N_("git am [options] [(<mbox>|<Maildir>)...]"), @@ -2225,6 +2231,10 @@ int cmd_am(int argc, const char **argv, const char *prefix) am_state_init(&state, git_path("rebase-apply")); + in_progress = am_in_progress(&state); + if (in_progress) + am_load(&state); + argc = parse_options(argc, argv, prefix, options, usage, 0); if (binary >= 0) @@ -2237,7 +2247,7 @@ int cmd_am(int argc, const char **argv, const char *prefix) if (read_index_preload(&the_index, NULL) < 0) die(_("failed to read the index")); - if (am_in_progress(&state)) { + if (in_progress) { /* * Catch user error to feed us patches when there is a session * in progress: @@ -2255,8 +2265,6 @@ int cmd_am(int argc, const char **argv, const char *prefix) if (resume == RESUME_FALSE) resume = RESUME_APPLY; - - am_load(&state); } else { struct argv_array paths = ARGV_ARRAY_INIT; int i; diff --git a/t/t4153-am-resume-override-opts.sh b/t/t4153-am-resume-override-opts.sh new file mode 100755 index 0000000..39fac79 --- /dev/null +++ b/t/t4153-am-resume-override-opts.sh @@ -0,0 +1,82 @@ +#!/bin/sh + +test_description='git-am command-line options override saved options' + +. ./test-lib.sh +. "$TEST_DIRECTORY"/lib-terminal.sh + +format_patch () { + git format-patch --stdout -1 "$1" >"$1".eml +} + +test_expect_success 'setup' ' + test_commit initial file && + test_commit first file && + + git checkout initial && + git mv file file2 && + test_tick && + git commit -m renamed-file && + git tag renamed-file && + + git checkout -b side initial && + test_commit side1 file && + test_commit side2 file && + + format_patch side1 && + format_patch side2 +' + +test_expect_success TTY '--3way overrides --no-3way' ' + rm -fr .git/rebase-apply && + git reset --hard && + git checkout renamed-file && + + # Applying side1 will fail as the file has been renamed. + test_must_fail git am --no-3way side[12].eml && + test_path_is_dir .git/rebase-apply && + test_cmp_rev renamed-file HEAD && + test -z "$(git ls-files -u)" && + + # Applying side1 with am --3way will succeed due to the threeway-merge. + # Applying side2 will fail as --3way does not apply to it. + test_must_fail test_terminal git am --3way </dev/zero && + test_path_is_dir .git/rebase-apply && + test side1 = "$(cat file2)" +' + +test_expect_success '--no-quiet overrides --quiet' ' + rm -fr .git/rebase-apply && + git reset --hard && + git checkout first && + + # Applying side1 will be quiet. + test_must_fail git am --quiet side[123].eml >out && + test_path_is_dir .git/rebase-apply && + ! test_i18ngrep "^Applying: " out && + echo side1 >file && + git add file && + + # Applying side1 will not be quiet. + # Applying side2 will be quiet. + git am --no-quiet --continue >out && + echo "Applying: side1" >expected && + test_i18ncmp expected out +' + +test_expect_success TTY '--reject overrides --no-reject' ' + rm -fr .git/rebase-apply && + git reset --hard && + git checkout first && + rm -f file.rej && + + test_must_fail git am --no-reject side1.eml && + test_path_is_dir .git/rebase-apply && + test_path_is_missing file.rej && + + test_must_fail test_terminal git am --reject </dev/zero && + test_path_is_dir .git/rebase-apply && + test_path_is_file file.rej +' + +test_done -- 2.5.0.280.gd88bd6e ^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v2 3/3] am: let --signoff override --no-signoff 2015-08-04 14:08 ` Paul Tan 2015-08-04 14:08 ` [PATCH v2 1/3] test_terminal: redirect child process' stdin to a pty Paul Tan 2015-08-04 14:08 ` [PATCH v2 2/3] am: let command-line options override saved options Paul Tan @ 2015-08-04 14:08 ` Paul Tan 2015-08-07 9:29 ` Johannes Schindelin 2015-08-05 15:41 ` [PATCH v2 0/3] am: let command-line options override saved options Junio C Hamano 3 siblings, 1 reply; 26+ messages in thread From: Paul Tan @ 2015-08-04 14:08 UTC (permalink / raw) To: git Cc: Junio C Hamano, Johannes Schindelin, Stefan Beller, Remi Lespinet, Jeff King, Paul Tan After resolving a conflicting patch, a user may wish to sign off the patch to declare that the patch has been modified. As such, the user will expect that running "git am --signoff --continue" will append the signoff to the commit message. However, the --signoff option is only taken into account during the mail-parsing stage. If the --signoff option is set, then the signoff will be appended to the commit message. Since the mail-parsing stage comes before the patch application stage, the --signoff option, if provided on the command-line when resuming, will have no effect at all. We cannot move the append_signoff() call to the patch application stage as the applypatch-msg hook and interactive mode, which run before patch application, may expect the signoff to be there. Fix this by taking note if the user explictly set the --signoff option on the command-line, and append the signoff to the commit message when resuming if so. Signed-off-by: Paul Tan <pyokagan@gmail.com> --- builtin/am.c | 28 +++++++++++++++++++++++++--- t/t4153-am-resume-override-opts.sh | 20 ++++++++++++++++++++ 2 files changed, 45 insertions(+), 3 deletions(-) diff --git a/builtin/am.c b/builtin/am.c index 0961304..8c95aec 100644 --- a/builtin/am.c +++ b/builtin/am.c @@ -98,6 +98,12 @@ enum scissors_type { SCISSORS_TRUE /* pass --scissors to git-mailinfo */ }; +enum signoff_type { + SIGNOFF_FALSE = 0, + SIGNOFF_TRUE = 1, + SIGNOFF_EXPLICIT /* --signoff was set on the command-line */ +}; + struct am_state { /* state directory path */ char *dir; @@ -123,7 +129,7 @@ struct am_state { int interactive; int threeway; int quiet; - int signoff; + int signoff; /* enum signoff_type */ int utf8; int keep; /* enum keep_type */ int message_id; @@ -1184,6 +1190,18 @@ static void NORETURN die_user_resolve(const struct am_state *state) } /** + * Appends signoff to the "msg" field of the am_state. + */ +static void am_append_signoff(struct am_state *state) +{ + struct strbuf sb = STRBUF_INIT; + + strbuf_attach(&sb, state->msg, state->msg_len, state->msg_len); + append_signoff(&sb, 0, 0); + state->msg = strbuf_detach(&sb, &state->msg_len); +} + +/** * Parses `mail` using git-mailinfo, extracting its patch and authorship info. * state->msg will be set to the patch message. state->author_name, * state->author_email and state->author_date will be set to the patch author's @@ -2151,8 +2169,9 @@ int cmd_am(int argc, const char **argv, const char *prefix) OPT_BOOL('3', "3way", &state.threeway, N_("allow fall back on 3way merging if needed")), OPT__QUIET(&state.quiet, N_("be quiet")), - OPT_BOOL('s', "signoff", &state.signoff, - N_("add a Signed-off-by line to the commit message")), + OPT_SET_INT('s', "signoff", &state.signoff, + N_("add a Signed-off-by line to the commit message"), + SIGNOFF_EXPLICIT), OPT_BOOL('u', "utf8", &state.utf8, N_("recode into utf8 (default)")), OPT_SET_INT('k', "keep", &state.keep, @@ -2265,6 +2284,9 @@ int cmd_am(int argc, const char **argv, const char *prefix) if (resume == RESUME_FALSE) resume = RESUME_APPLY; + + if (state.signoff == SIGNOFF_EXPLICIT) + am_append_signoff(&state); } else { struct argv_array paths = ARGV_ARRAY_INIT; int i; diff --git a/t/t4153-am-resume-override-opts.sh b/t/t4153-am-resume-override-opts.sh index 39fac79..7c013d8 100755 --- a/t/t4153-am-resume-override-opts.sh +++ b/t/t4153-am-resume-override-opts.sh @@ -64,6 +64,26 @@ test_expect_success '--no-quiet overrides --quiet' ' test_i18ncmp expected out ' +test_expect_success '--signoff overrides --no-signoff' ' + rm -fr .git/rebase-apply && + git reset --hard && + git checkout first && + + test_must_fail git am --no-signoff side[12].eml && + test_path_is_dir .git/rebase-apply && + echo side1 >file && + git add file && + git am --signoff --continue && + + # Applied side1 will be signed off + echo "Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>" >expected && + git cat-file commit HEAD^ | grep "Signed-off-by:" >actual && + test_cmp expected actual && + + # Applied side2 will not be signed off + test $(git cat-file commit HEAD | grep -c "Signed-off-by:") -eq 0 +' + test_expect_success TTY '--reject overrides --no-reject' ' rm -fr .git/rebase-apply && git reset --hard && -- 2.5.0.280.gd88bd6e ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH v2 3/3] am: let --signoff override --no-signoff 2015-08-04 14:08 ` [PATCH v2 3/3] am: let --signoff override --no-signoff Paul Tan @ 2015-08-07 9:29 ` Johannes Schindelin 2015-08-12 3:06 ` Paul Tan 0 siblings, 1 reply; 26+ messages in thread From: Johannes Schindelin @ 2015-08-07 9:29 UTC (permalink / raw) To: Paul Tan; +Cc: git, Junio C Hamano, Stefan Beller, Remi Lespinet, Jeff King Hi Paul, On 2015-08-04 16:08, Paul Tan wrote: > diff --git a/builtin/am.c b/builtin/am.c > index 0961304..8c95aec 100644 > --- a/builtin/am.c > +++ b/builtin/am.c > @@ -2151,8 +2169,9 @@ int cmd_am(int argc, const char **argv, const > [...] > char *prefix) > OPT_BOOL('3', "3way", &state.threeway, > N_("allow fall back on 3way merging if needed")), > OPT__QUIET(&state.quiet, N_("be quiet")), > - OPT_BOOL('s', "signoff", &state.signoff, > - N_("add a Signed-off-by line to the commit message")), > + OPT_SET_INT('s', "signoff", &state.signoff, > + N_("add a Signed-off-by line to the commit message"), > + SIGNOFF_EXPLICIT), > OPT_BOOL('u', "utf8", &state.utf8, > N_("recode into utf8 (default)")), > OPT_SET_INT('k', "keep", &state.keep, > @@ -2265,6 +2284,9 @@ int cmd_am(int argc, const char **argv, const > char *prefix) > > if (resume == RESUME_FALSE) > resume = RESUME_APPLY; > + > + if (state.signoff == SIGNOFF_EXPLICIT) > + am_append_signoff(&state); > } else { 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. Or am I missing something? Ciao, Dscho ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 3/3] am: let --signoff override --no-signoff 2015-08-07 9:29 ` Johannes Schindelin @ 2015-08-12 3:06 ` Paul Tan 2015-08-12 3:07 ` Paul Tan 0 siblings, 1 reply; 26+ messages in thread From: Paul Tan @ 2015-08-12 3:06 UTC (permalink / raw) To: Johannes Schindelin Cc: Git List, Junio C Hamano, Stefan Beller, Remi Lespinet, Jeff King On Fri, Aug 7, 2015 at 5:29 PM, Johannes Schindelin <johannes.schindelin@gmx.de> wrote: >> diff --git a/builtin/am.c b/builtin/am.c >> index 0961304..8c95aec 100644 >> --- a/builtin/am.c >> +++ b/builtin/am.c >> @@ -2265,6 +2284,9 @@ int cmd_am(int argc, const char **argv, const >> char *prefix) >> >> if (resume == RESUME_FALSE) >> resume = RESUME_APPLY; >> + >> + if (state.signoff == SIGNOFF_EXPLICIT) >> + am_append_signoff(&state); >> } else { > > 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. It's not present in this diff context, but this hunk modifies the code path where in_progress is true. In other words, we only check for SIGNOFF_EXPLICIT if ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 3/3] am: let --signoff override --no-signoff 2015-08-12 3:06 ` Paul Tan @ 2015-08-12 3:07 ` Paul Tan 0 siblings, 0 replies; 26+ messages in thread From: Paul Tan @ 2015-08-12 3:07 UTC (permalink / raw) To: Johannes Schindelin Cc: Git List, Junio C Hamano, Stefan Beller, Remi Lespinet, Jeff King On Wed, Aug 12, 2015 at 11:06 AM, Paul Tan <pyokagan@gmail.com> wrote: > It's not present in this diff context, but this hunk modifies the code > path where in_progress is true. In other words, we only check for > SIGNOFF_EXPLICIT if ..we are resuming. (Ugh, butter fingers) Thanks, Paul ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 0/3] am: let command-line options override saved options 2015-08-04 14:08 ` Paul Tan ` (2 preceding siblings ...) 2015-08-04 14:08 ` [PATCH v2 3/3] am: let --signoff override --no-signoff Paul Tan @ 2015-08-05 15:41 ` Junio C Hamano 2015-08-05 17:51 ` Paul Tan 3 siblings, 1 reply; 26+ messages in thread From: Junio C Hamano @ 2015-08-05 15:41 UTC (permalink / raw) To: Paul Tan Cc: git, Johannes Schindelin, Stefan Beller, Remi Lespinet, Jeff King Interesting. This seems to break test under prove. cd t && make T=t4153-am-resume-override-opts.sh prove does not seem to return. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 0/3] am: let command-line options override saved options 2015-08-05 15:41 ` [PATCH v2 0/3] am: let command-line options override saved options Junio C Hamano @ 2015-08-05 17:51 ` Paul Tan 0 siblings, 0 replies; 26+ messages in thread From: Paul Tan @ 2015-08-05 17:51 UTC (permalink / raw) To: Junio C Hamano Cc: git, Johannes Schindelin, Stefan Beller, Remi Lespinet, Jeff King On Wed, Aug 05, 2015 at 08:41:44AM -0700, Junio C Hamano wrote: > Interesting. This seems to break test under prove. > > cd t && make T=t4153-am-resume-override-opts.sh prove > > does not seem to return. The new test-terminal.perl code is the culprit. It seems that if our wrapped process terminates before our stdin-writing fork does, our stdin-writing process will stall. I think this occurs with prove because prove waits until all of its child processes terminate before returning. So, the solution may be to send a SIGTERM to our stdin-writing fork should our wrapped process terminate before it does, in order to ensure that it immediately exits. The following squash fixes it for me. Thanks, Paul -- >8 -- Subject: [PATCH] squash! test_terminal: redirect child process' stdin to a pty When the child process terminates before the copy_stdio() finishes writing all of its data to the child's stdin slave pty, it will stall. As such, we first move the stdin-pty-writing logic out of copy_stdio() into its own subroutine copy_stdin() so that we can manage the forked process ourselves, and then we send SIGTERM to the forked process should the command we are wrapping terminate before copy_stdin() finishes writing all of its data to un-stall it. Signed-off-by: Paul Tan <pyokagan@gmail.com> --- t/test-terminal.perl | 27 ++++++++++++++++++--------- 1 file changed, 18 insertions(+), 9 deletions(-) diff --git a/t/test-terminal.perl b/t/test-terminal.perl index f6fc9ae..96b6a03 100755 --- a/t/test-terminal.perl +++ b/t/test-terminal.perl @@ -51,24 +51,26 @@ sub xsendfile { copy($in, $out, 4096) or $!{EIO} or die "cannot copy from child: $!"; } -sub copy_stdio { - my ($in, $out, $err) = @_; +sub copy_stdin { + my ($in) = @_; my $pid = fork; if (!$pid) { - close($out); - close($err); xsendfile($in, \*STDIN); exit 0; } - $pid = fork; + close($in); + return $pid; +} + +sub copy_stdio { + my ($out, $err) = @_; + my $pid = fork; defined $pid or die "fork failed: $!"; if (!$pid) { - close($in); close($out); xsendfile(\*STDERR, $err); exit 0; } - close($in); close($err); xsendfile(\*STDOUT, $out); finish_child($pid) == 0 @@ -91,5 +93,12 @@ my $pid = start_child(\@ARGV, $master_in->slave, $master_out->slave, $master_err close $master_in->slave; close $master_out->slave; close $master_err->slave; -copy_stdio($master_in, $master_out, $master_err); -exit(finish_child($pid)); +my $in_pid = copy_stdin($master_in); +copy_stdio($master_out, $master_err); +my $ret = finish_child($pid); +# If the child process terminates before our copy_stdin() process is able to +# write all of its data to $master_in, the copy_stdin() process could stall. +# Send SIGTERM to it to ensure it terminates. +kill 'TERM', $in_pid; +finish_child($in_pid); +exit($ret); -- 2.5.0.282.gdd6b4b0 ^ permalink raw reply related [flat|nested] 26+ messages in thread
end of thread, other threads:[~2015-08-12 4:16 UTC | newest] Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-07-24 17:48 "git am" and then "git am -3" regression? Junio C Hamano 2015-07-24 18:09 ` Jeff King 2015-07-26 5:03 ` Paul Tan 2015-07-26 5:21 ` Jeff King 2015-07-27 8:09 ` Matthieu Moy 2015-07-27 8:32 ` Jeff King 2015-07-27 14:21 ` Junio C Hamano 2015-07-28 16:43 ` [PATCH] am: let command-line options override saved options Paul Tan 2015-07-28 16:48 ` Junio C Hamano 2015-07-28 17:09 ` Junio C Hamano 2015-07-31 10:58 ` Paul Tan 2015-07-31 16:04 ` Junio C Hamano 2015-08-01 0:59 ` Paul Tan 2015-08-04 14:05 ` [PATCH v2 0/3] " Paul Tan 2015-08-04 21:12 ` Junio C Hamano 2015-08-04 14:08 ` Paul Tan 2015-08-04 14:08 ` [PATCH v2 1/3] test_terminal: redirect child process' stdin to a pty Paul Tan 2015-08-06 22:15 ` Eric Sunshine 2015-08-12 4:16 ` Paul Tan 2015-08-04 14:08 ` [PATCH v2 2/3] am: let command-line options override saved options Paul Tan 2015-08-04 14:08 ` [PATCH v2 3/3] am: let --signoff override --no-signoff Paul Tan 2015-08-07 9:29 ` Johannes Schindelin 2015-08-12 3:06 ` Paul Tan 2015-08-12 3:07 ` Paul Tan 2015-08-05 15:41 ` [PATCH v2 0/3] am: let command-line options override saved options Junio C Hamano 2015-08-05 17:51 ` Paul Tan
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).