"git am" and then "git am -3" regression?

"git am" and then "git am -3" regression?

[Junio C Hamano]

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.

Re: "git am" and then "git am -3" regression?

[Jeff King]

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

Re: "git am" and then "git am -3" regression?

[Paul Tan]

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

Re: "git am" and then "git am -3" regression?

[Jeff King]

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

Re: "git am" and then "git am -3" regression?

[Matthieu Moy]

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/

Re: "git am" and then "git am -3" regression?

[Jeff King]

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

Re: "git am" and then "git am -3" regression?

[Junio C Hamano]

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.

[PATCH] am: let command-line options override saved options

[Paul Tan]

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

Re: [PATCH] am: let command-line options override saved options

[Junio C Hamano]

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.

Re: [PATCH] am: let command-line options override saved options

[Junio C Hamano]

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?

Re: [PATCH] am: let command-line options override saved options

[Paul Tan]

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

Re: [PATCH] am: let command-line options override saved options

[Junio C Hamano]

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)?

Re: [PATCH] am: let command-line options override saved options

[Paul Tan]

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

[PATCH v2 0/3] am: let command-line options override saved options

[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

[PATCH v2 0/3] am: let command-line options override saved options

[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

[PATCH v2 1/3] test_terminal: redirect child process' stdin to a pty

[Paul Tan]

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

[PATCH v2 2/3] am: let command-line options override saved options

[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

[PATCH v2 3/3] am: let --signoff override --no-signoff

[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

Re: [PATCH v2 0/3] am: let command-line options override saved options

[Junio C Hamano]

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.

Re: [PATCH v2 0/3] am: let command-line options override saved options

[Junio C Hamano]

Interesting.  This seems to break test under prove.

    cd t && make T=t4153-am-resume-override-opts.sh prove

does not seem to return.

Re: [PATCH v2 0/3] am: let command-line options override saved options

[Paul Tan]

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

Re: [PATCH v2 1/3] test_terminal: redirect child process' stdin to a pty

[Eric Sunshine]

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>

Re: [PATCH v2 3/3] am: let --signoff override --no-signoff

[Johannes Schindelin]

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

Re: [PATCH v2 3/3] am: let --signoff override --no-signoff

[Paul Tan]

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

Re: [PATCH v2 3/3] am: let --signoff override --no-signoff

[Paul Tan]

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

Re: [PATCH v2 1/3] test_terminal: redirect child process' stdin to a pty

[Paul Tan]

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