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

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 31, 2015, 16:04 UTC
Message-ID: <xmqqvbd05n5z.fsf@gitster.dls.corp.google.com>
In-Reply-To: <CACRoPnR1df+uEnpFArJtwEBCh+HiQYDYGOyZ7KQEGtrdiaX3GQ@mail.gmail.com>

Paul Tan <pyokagan@gmail.com> writes:

Show 14 quoted lines

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

Show 21 quoted lines

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

Show 22 quoted lines

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