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

From: Paul Tan <pyokagan@gmail.com>
Date: Jul 31, 2015, 10:58 UTC
Message-ID: <CACRoPnR1df+uEnpFArJtwEBCh+HiQYDYGOyZ7KQEGtrdiaX3GQ@mail.gmail.com>
In-Reply-To: <xmqqegjsfbtk.fsf@gitster.dls.corp.google.com>

On Wed, Jul 29, 2015 at 1:09 AM, Junio C Hamano <gitster@pobox.com> wrote:

Show 30 quoted lines

> 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.

Show 17 quoted lines

>> +             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
}

Show 25 quoted lines

>> +'
>> +
>> +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