Re: [PATCH] am: let command-line options override saved options
On Sat, Aug 1, 2015 at 12:04 AM, Junio C Hamano wrote:
> Paul Tan 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] am: let command-line options override saved options
Paul Tan 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)?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] am: let command-line options override saved options
On Wed, Jul 29, 2015 at 1:09 AM, Junio C Hamano wrote:
> Paul Tan writes:
>
>> diff --git a/t/t4153-am-resume-override-opts.sh
>> b/t/t4153-am-resume-override-opts.sh
>> new file mode 100755
>> index 000..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: " &&
>> + 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: " &&
>> + 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
Re: [PATCH] am: let command-line options override saved options
Paul Tan writes:
> diff --git a/t/t4153-am-resume-override-opts.sh
> b/t/t4153-am-resume-override-opts.sh
> new file mode 100755
> index 000..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: " &&
> + 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: " &&
> + 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?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] am: let command-line options override saved options
Paul Tan 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. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] am: let command-line options override saved options
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
Helped-by: Jeff King
Signed-off-by: Paul Tan
---
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] [(|)...]"),
@@ -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 000..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: " &&
+ 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: " &&
+ 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
