| Message ID | 96d8573dc808bc32990842675ca32a0d1e8a8cef.1637232636.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | am: support --empty=(die|drop|keep) option to handle empty patches | expand |
"Aleen via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Aleen <aleen42@vip.qq.com> Here is a place to explain to readers what it exactly means to "support --empty option to handle empty patches". You know what you meant, I happen to know what you meant because I haven't forgotten yet, but we are writing for developers in the future who need to fix bugs in 6 months down the road to decide if this commit is involved in the bug in "git am" they are hunting. > Signed-off-by: Aleen <aleen42@vip.qq.com> > --- > Documentation/git-am.txt | 9 +++++ > builtin/am.c | 48 +++++++++++++++++++++++--- > t/t4150-am.sh | 73 ++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 125 insertions(+), 5 deletions(-) > > diff --git a/Documentation/git-am.txt b/Documentation/git-am.txt > index 0a4a984dfde..665bc89ca9f 100644 > --- a/Documentation/git-am.txt > +++ b/Documentation/git-am.txt > @@ -16,6 +16,7 @@ SYNOPSIS > [--exclude=<path>] [--include=<path>] [--reject] [-q | --quiet] > [--[no-]scissors] [-S[<keyid>]] [--patch-format=<format>] > [--quoted-cr=<action>] > + [--empty=(die|drop|keep)] > [(<mbox> | <Maildir>)...] > 'git am' (--continue | --skip | --abort | --quit | --show-current-patch[=(diff|raw)]) > > @@ -63,6 +64,14 @@ OPTIONS > --quoted-cr=<action>:: > This flag will be passed down to 'git mailinfo' (see linkgit:git-mailinfo[1]). > > +--empty-commit=(die|drop|keep):: > + The command usually errors out when seeing an input e-mail > + message that lacks a patch. When this option is set to > + 'drop', skip such an e-mail message without outputting error. "without outputting error" -> "instead". The first sentence used "to error" as a verb to mean "stop the operation in the middle with a message that diagnoses what is wrong". Saying "outputting error" to use the same word as a noun is not a good idea, especially the use of verb "to output", as it makes the word "error" look as if it is only the "message" part, and it becomes unclear if the "stopping due to an error" is also skipped. I actually think it makes sense to give a notice/info message when "drop" skips an empty patch, unless "--quiet" is given. > + When this option is set to 'keep', create an empty commit, > + recording the contents of the e-mail message as its log. > + 'die' is specified by default. "is specified by default" sounds a bit strange. Perhaps lose the last sentence, and update the first line like so: By default, or when the option is set to 'die', the command errors out on an input e-mail message that lacks a patch. > diff --git a/builtin/am.c b/builtin/am.c > index 8677ea2348a..1a3ed87b445 100644 > --- a/builtin/am.c > +++ b/builtin/am.c > @@ -87,6 +87,12 @@ enum show_patch_type { > SHOW_PATCH_DIFF = 1, > }; > > +enum empty_action { > + DIE_EMPTY_COMMIT = 0, /* output errors */ > + DROP_EMPTY_COMMIT, /* skip without outputting errors */ I think the user deserves "info:" (unless "--quiet"). > + KEEP_EMPTY_COMMIT /* keep recording as empty commits */ > +}; > + > struct am_state { > /* state directory path */ > char *dir; > @@ -118,6 +124,7 @@ struct am_state { > int message_id; > int scissors; /* enum scissors_type */ > int quoted_cr; /* enum quoted_cr_action */ > + int empty_type; /* enum empty_action */ > struct strvec git_apply_opts; > const char *resolvemsg; > int committer_date_is_author_date; > @@ -178,6 +185,23 @@ static int am_option_parse_quoted_cr(const struct option *opt, > return 0; > } > > +static int am_option_parse_empty_commit(const struct option *opt, > + const char *arg, int unset) > +{ > + int *opt_value = opt->value; > + > + if (unset || !strcmp(arg, "die")) > + *opt_value = DIE_EMPTY_COMMIT; > + else if (!strcmp(arg, "drop")) > + *opt_value = DROP_EMPTY_COMMIT; > + else if (!strcmp(arg, "keep")) > + *opt_value = KEEP_EMPTY_COMMIT; > + else > + return error(_("Invalid value for --empty: %s"), arg); > + > + return 0; > +} OK. > @@ -1792,6 +1811,20 @@ static void am_run(struct am_state *state, int resume) > if (state->interactive && do_interactive(state)) > goto next; > > + if (is_empty_or_missing_file(am_path(state, "patch"))) { > + if (state->empty_type == DROP_EMPTY_COMMIT) > + goto next; > + else if (state->empty_type == KEEP_EMPTY_COMMIT) { > + if (run_applypatch_msg_hook(state)) > + exit(1); > + else > + goto commit; It probably is easier to read without "else", i.e. else if (state->empty_type == KEEP_EMPTY_COMMIT) { if (run_applypatch_msg_hook(state)) exit(1); goto commit; Running hook here and outside this if() block a bit ugly. Can't we instead set a flag variable or something and let the existing call to run_applypatch_msg_hook() used? > + } else if (state->empty_type == DIE_EMPTY_COMMIT) { > + printf_ln(_("Patch is empty.")); > + die_user_resolve(state); > + } Also, if developers ever add another possible value for the state->empty_type member (by the way, "empty" in its name is good, but "type" is iffy), it would be a bug if they do not consider what should happen inside this "if (is_empty_or_missing_file())" block, no? In such a case, instead of if/else if/... cascade, consider using switch(). Taking them together. while (state->cur <= state->last) { const char *mail = am_path(state, msgnum(state)); int apply_status; int to_keep; ... to_keep = 0; if (is_empty_or_missing_file(am_path(state, "patch"))) { switch (state->empty_type) { case DROP_EMPTY_COMMIT: say(state, stdout, _("Skipping empty patch '%.*s", linelen(state->msg0, state->msg); goto next; break; case KEEP_EMPTY_COMMIT: to_keep = 1; break; case DIE_EMPTY_COMMIT: pritnf_ln(_("Patch is empty.")); die_user_resolve(state); break; } } if (run_applypatch_msg_hook(state)) exit(1); if (to_keep) goto commit; /* empty: just record a message */ or something like that. > diff --git a/t/t4150-am.sh b/t/t4150-am.sh > index 2aaaa0d7ded..3119556884d 100755 > --- a/t/t4150-am.sh > +++ b/t/t4150-am.sh > @@ -196,6 +196,13 @@ test_expect_success setup ' > > git format-patch -M --stdout lorem^ >rename-add.patch && > > + git checkout -b empty-commit && > + git commit -m "empty commit" --allow-empty && > + > + git format-patch --stdout empty-commit^ >empty.patch && Unless this is a test for "format-patch" command to make sure that the above command will not output a single byte, I would rather see the above line written like this: : >empty.patch && > + git format-patch --stdout --cover-letter empty-commit^ >cover-letter.patch && This only creates a cover letter without anything, which sort of looks like an empty commit. > + git format-patch --always --stdout empty-commit^ >empty-commit.patch && And this gives another empty patch with only a message. I would sort-of understand if there is git format-patch --stdout --always --cover-letter HEAD^ >both.patch that has two e-mail messages in it, but what's the significance of testing both cover-letter.patch and empty-commit.patch? The receiving end would not care what's in the actual e-mail message, as long as it lacks the patch part, no? Or am I missing some subtle reason why we want to test both? > @@ -1152,4 +1159,70 @@ test_expect_success 'apply binary blob in partial clone' ' > git -C client am ../patch > ' > > +test_expect_success 'still output error with --empty when meeting empty files' ' > + test_must_fail git am --empty=drop empty.patch 2>actual && Nobody will understand "still", unless they know it used to be unconditionally a failure to feed an e-mail message that lacks a patch. test_expect_success 'An empty input file is error regardless of --empty option' ' or something like that? > + echo Patch format detection failed. >expected && > + test_cmp expected actual > +' > + > +test_expect_success 'error when meeting e-mail message that lacks a patch by default' ' > + git checkout empty-commit^ && > + test_must_fail git am empty-commit.patch >err && > + test_path_is_dir .git/rebase-apply && > + test_i18ngrep "Patch is empty." err && > + rm -fr .git/rebase-apply && If any command before "rm" fails, it won't be cleaned up. test_expect_success 'a message without a patch is an error (default)' ' test_when_finished "git am --abort || :" && git checkout --detach empty-commit^ && test_must_fail git am empty-commit.patch >err && grep "Patch is empty" err ' or something like that. The title says it tests the default case, so do not test --empty=die case in the same test. Use a separate test to do the same for the case where an explicit "--empty=die" is given. As I said, I do not quite see the point of testing with both empty-commit.patch and cover-letter.patch (unless the latter has an empty commit and a cover letter, totally in two messages). > + test_must_fail git am --empty=die empty-commit.patch >err && > + test_path_is_dir .git/rebase-apply && > + test_i18ngrep "Patch is empty." err && > + rm -fr .git/rebase-apply && > + > + test_must_fail git am --empty=die cover-letter.patch >err && > + test_path_is_dir .git/rebase-apply && > + test_i18ngrep "Patch is empty." err && > + rm -fr .git/rebase-apply > +' > + > +test_expect_success 'skip without error when meeting e-mail message that lacks a patch' ' > + git am --empty=drop empty-commit.patch >err && The output 'err' is used by nobody; do not redirect it and just leave the standard output alone. > + test_path_is_missing .git/rebase-apply && Don't test this condition when testing success or failure from "am". That is too much implementation detail. > + git rev-parse empty-commit^ >expected && > + git rev-parse HEAD >actual && > + test_cmp expected actual && OK, we make sure that HEAD does not move. > + git am --empty=drop cover-letter.patch >err && > + test_path_is_missing .git/rebase-apply && > + test_cmp_rev empty-commit^ HEAD Lose this one? As I said, I may be missing some subtle reason why both need to be tested---and if it is the case, test it in a separate test, not 2 of them in a single test like this. > +' > + > +test_expect_success 'record as an empty commit when meeting e-mail message that lacks a patch' ' > + git am --empty=keep empty-commit.patch && > + test_path_is_missing .git/rebase-apply && > + { > + git show empty-commit --format="%B" && > + echo "--" && Did this test pass? Without a trailing whitespace after double-dash, I do not think this shell snippet reproduces a patch e-mail correctly In any case, don't do this. Imagine a future developer may want to improve the patch output by adding other things after the "-- " signature line, in addition to the "git version". Or perhaps the "git version" trailer gets removed by default for privacy concerns. Should such changes be prevented from being made, only because this test expects exact output we have today? No. Verify what we _care_, e.g. "does the title of the resulting commit reproduce the one-liner message given to '-m' when the empty commit was made?" is a sensible question to ask. > + git version | sed -e "s/^git version //" && > + echo > + } >expected && > + git show HEAD --format="%B" >actual && > + test_cmp actual expected && > + > + git am --empty=keep cover-letter.patch && > + test_path_is_missing .git/rebase-apply && > + { > + echo "*** SUBJECT HERE ***" && > + echo && > + echo "*** BLURB HERE ***" && > + echo && > + echo "A U Thor (1):" && > + printf " " && > + git show empty-commit --format="%B" && > + echo "--" && > + git version | sed -e "s/^git version //" && > + echo Don't do this. Imagine a future developer may want to improve the cover letter template. Should that change be prevented from being made, because this test expects exact output we have today? No. > + } >expected && > + git show HEAD --format="%B" >actual && > + test_cmp actual expected > +' > + > test_done Thanks.
On 18/11/21 17.50, Aleen via GitGitGadget wrote: > +test_expect_success 'still output error with --empty when meeting empty files' ' > + test_must_fail git am --empty=drop empty.patch 2>actual && > + echo Patch format detection failed. >expected && > + test_cmp expected actual > +' Why isn't the echo string quoted?
On Fri, Nov 19 2021, Bagas Sanjaya wrote: > On 18/11/21 17.50, Aleen via GitGitGadget wrote: >> +test_expect_success 'still output error with --empty when meeting empty files' ' >> + test_must_fail git am --empty=drop empty.patch 2>actual && >> + echo Patch format detection failed. >expected && >> + test_cmp expected actual >> +' > > Why isn't the echo string quoted? There's no need to quote the arguments given to echo in cases like these. It's not the same, i.e. it'll get N arguments on argv and not on, but it makes it easier to spot things that do need quoting, rather than over-quoting everything.
On Fri, Nov 19, 2021 at 7:12 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > On Fri, Nov 19 2021, Bagas Sanjaya wrote: > > On 18/11/21 17.50, Aleen via GitGitGadget wrote: > >> +test_expect_success 'still output error with --empty when meeting empty files' ' > >> + test_must_fail git am --empty=drop empty.patch 2>actual && > >> + echo Patch format detection failed. >expected && > >> + test_cmp expected actual > >> +' > > > > Why isn't the echo string quoted? > > There's no need to quote the arguments given to echo in cases like > these. > > It's not the same, i.e. it'll get N arguments on argv and not on, but it > makes it easier to spot things that do need quoting, rather than > over-quoting everything. I recently expressed an opposing opinion in [1], stating effectively that omitting the quotes like this is "an accident waiting to happen": ... the lack of quotes ... in the `echo ... >expect` statement gives me a moment's pause since it relies upon the fact that `echo` will insert exactly one space between the ... arguments (which happens to match the single space in the [command's output] ). For clarity and that extra bit of robustness, I'd probably have used a single double-quoted string argument with `echo`. But, it's a fairly minor objection. [1]: https://lore.kernel.org/git/CAPig+cQVSUg1aqry_hMydJ=Uo=-VhOog6TUTpG=0on0LUcw8Dg@mail.gmail.com/
Bagas Sanjaya <bagasdotme@gmail.com> writes: > On 18/11/21 17.50, Aleen via GitGitGadget wrote: >> +test_expect_success 'still output error with --empty when meeting empty files' ' >> + test_must_fail git am --empty=drop empty.patch 2>actual && >> + echo Patch format detection failed. >expected && >> + test_cmp expected actual >> +' > > Why isn't the echo string quoted? You may doing so without realizing, but such a "why does the code not do X" question, especially without stating why you think the code should do X, gets irritating quickly. I think it is better to quote the arguments to echo to make it a single string, because ... would have helped the author of the patch. If this were one-off, I would have let it go, but I thought it may be worth commenting, as you seem to often ask this kind of "questions".
Eric Sunshine <sunshine@sunshineco.com> writes: > I recently expressed an opposing opinion in [1], stating effectively > that omitting the quotes like this is "an accident waiting to happen": > > ... the lack of quotes ... in the `echo ... >expect` statement > gives me a moment's pause since it relies upon the fact that > `echo` will insert exactly one space between the ... arguments > (which happens to match the single space in the [command's output] > ). For clarity and that extra bit of robustness, I'd probably have > used a single double-quoted string argument with `echo`. > > But, it's a fairly minor objection. It indeed is minor enough that a patch to turn an existing echo A B C >expect && test_cmp expect actual into echo "A B C" >expect && test_cmp expect actual is not welcome. But it still is worth pointing out and correcting in a patch to add new code, I would think. It all depends on what we care about, and the use of test_cmp means we do care about exact shape of the string, including the inter-word spacing. Thanks.
diff --git a/Documentation/git-am.txt b/Documentation/git-am.txt index 0a4a984dfde..665bc89ca9f 100644 --- a/Documentation/git-am.txt +++ b/Documentation/git-am.txt @@ -16,6 +16,7 @@ SYNOPSIS [--exclude=<path>] [--include=<path>] [--reject] [-q | --quiet] [--[no-]scissors] [-S[<keyid>]] [--patch-format=<format>] [--quoted-cr=<action>] + [--empty=(die|drop|keep)] [(<mbox> | <Maildir>)...] 'git am' (--continue | --skip | --abort | --quit | --show-current-patch[=(diff|raw)]) @@ -63,6 +64,14 @@ OPTIONS --quoted-cr=<action>:: This flag will be passed down to 'git mailinfo' (see linkgit:git-mailinfo[1]). +--empty-commit=(die|drop|keep):: + The command usually errors out when seeing an input e-mail + message that lacks a patch. When this option is set to + 'drop', skip such an e-mail message without outputting error. + When this option is set to 'keep', create an empty commit, + recording the contents of the e-mail message as its log. + 'die' is specified by default. + -m:: --message-id:: Pass the `-m` flag to 'git mailinfo' (see linkgit:git-mailinfo[1]), diff --git a/builtin/am.c b/builtin/am.c index 8677ea2348a..1a3ed87b445 100644 --- a/builtin/am.c +++ b/builtin/am.c @@ -87,6 +87,12 @@ enum show_patch_type { SHOW_PATCH_DIFF = 1, }; +enum empty_action { + DIE_EMPTY_COMMIT = 0, /* output errors */ + DROP_EMPTY_COMMIT, /* skip without outputting errors */ + KEEP_EMPTY_COMMIT /* keep recording as empty commits */ +}; + struct am_state { /* state directory path */ char *dir; @@ -118,6 +124,7 @@ struct am_state { int message_id; int scissors; /* enum scissors_type */ int quoted_cr; /* enum quoted_cr_action */ + int empty_type; /* enum empty_action */ struct strvec git_apply_opts; const char *resolvemsg; int committer_date_is_author_date; @@ -178,6 +185,23 @@ static int am_option_parse_quoted_cr(const struct option *opt, return 0; } +static int am_option_parse_empty_commit(const struct option *opt, + const char *arg, int unset) +{ + int *opt_value = opt->value; + + if (unset || !strcmp(arg, "die")) + *opt_value = DIE_EMPTY_COMMIT; + else if (!strcmp(arg, "drop")) + *opt_value = DROP_EMPTY_COMMIT; + else if (!strcmp(arg, "keep")) + *opt_value = KEEP_EMPTY_COMMIT; + else + return error(_("Invalid value for --empty: %s"), arg); + + return 0; +} + /** * Returns path relative to the am_state directory. */ @@ -1248,11 +1272,6 @@ static int parse_mail(struct am_state *state, const char *mail) goto finish; } - if (is_empty_or_missing_file(am_path(state, "patch"))) { - printf_ln(_("Patch is empty.")); - die_user_resolve(state); - } - strbuf_addstr(&msg, "\n\n"); strbuf_addbuf(&msg, &mi.log_message); strbuf_stripspace(&msg, 0); @@ -1792,6 +1811,20 @@ static void am_run(struct am_state *state, int resume) if (state->interactive && do_interactive(state)) goto next; + if (is_empty_or_missing_file(am_path(state, "patch"))) { + if (state->empty_type == DROP_EMPTY_COMMIT) + goto next; + else if (state->empty_type == KEEP_EMPTY_COMMIT) { + if (run_applypatch_msg_hook(state)) + exit(1); + else + goto commit; + } else if (state->empty_type == DIE_EMPTY_COMMIT) { + printf_ln(_("Patch is empty.")); + die_user_resolve(state); + } + } + if (run_applypatch_msg_hook(state)) exit(1); @@ -1827,6 +1860,7 @@ static void am_run(struct am_state *state, int resume) die_user_resolve(state); } +commit: do_commit(state); next: @@ -2357,6 +2391,10 @@ int cmd_am(int argc, const char **argv, const char *prefix) { OPTION_STRING, 'S', "gpg-sign", &state.sign_commit, N_("key-id"), N_("GPG-sign commits"), PARSE_OPT_OPTARG, NULL, (intptr_t) "" }, + { OPTION_CALLBACK, 0, "empty", &state.empty_type, + "(die|drop|keep)", + N_("specify how to handle empty patches"), + PARSE_OPT_OPTARG, am_option_parse_empty_commit }, OPT_HIDDEN_BOOL(0, "rebasing", &state.rebasing, N_("(internal use for git-rebase)")), OPT_END() diff --git a/t/t4150-am.sh b/t/t4150-am.sh index 2aaaa0d7ded..3119556884d 100755 --- a/t/t4150-am.sh +++ b/t/t4150-am.sh @@ -196,6 +196,13 @@ test_expect_success setup ' git format-patch -M --stdout lorem^ >rename-add.patch && + git checkout -b empty-commit && + git commit -m "empty commit" --allow-empty && + + git format-patch --stdout empty-commit^ >empty.patch && + git format-patch --stdout --cover-letter empty-commit^ >cover-letter.patch && + git format-patch --always --stdout empty-commit^ >empty-commit.patch && + # reset time sane_unset test_tick && test_tick @@ -1152,4 +1159,70 @@ test_expect_success 'apply binary blob in partial clone' ' git -C client am ../patch ' +test_expect_success 'still output error with --empty when meeting empty files' ' + test_must_fail git am --empty=drop empty.patch 2>actual && + echo Patch format detection failed. >expected && + test_cmp expected actual +' + +test_expect_success 'error when meeting e-mail message that lacks a patch by default' ' + git checkout empty-commit^ && + test_must_fail git am empty-commit.patch >err && + test_path_is_dir .git/rebase-apply && + test_i18ngrep "Patch is empty." err && + rm -fr .git/rebase-apply && + + test_must_fail git am --empty=die empty-commit.patch >err && + test_path_is_dir .git/rebase-apply && + test_i18ngrep "Patch is empty." err && + rm -fr .git/rebase-apply && + + test_must_fail git am --empty=die cover-letter.patch >err && + test_path_is_dir .git/rebase-apply && + test_i18ngrep "Patch is empty." err && + rm -fr .git/rebase-apply +' + +test_expect_success 'skip without error when meeting e-mail message that lacks a patch' ' + git am --empty=drop empty-commit.patch >err && + test_path_is_missing .git/rebase-apply && + git rev-parse empty-commit^ >expected && + git rev-parse HEAD >actual && + test_cmp expected actual && + + git am --empty=drop cover-letter.patch >err && + test_path_is_missing .git/rebase-apply && + test_cmp_rev empty-commit^ HEAD +' + +test_expect_success 'record as an empty commit when meeting e-mail message that lacks a patch' ' + git am --empty=keep empty-commit.patch && + test_path_is_missing .git/rebase-apply && + { + git show empty-commit --format="%B" && + echo "--" && + git version | sed -e "s/^git version //" && + echo + } >expected && + git show HEAD --format="%B" >actual && + test_cmp actual expected && + + git am --empty=keep cover-letter.patch && + test_path_is_missing .git/rebase-apply && + { + echo "*** SUBJECT HERE ***" && + echo && + echo "*** BLURB HERE ***" && + echo && + echo "A U Thor (1):" && + printf " " && + git show empty-commit --format="%B" && + echo "--" && + git version | sed -e "s/^git version //" && + echo + } >expected && + git show HEAD --format="%B" >actual && + test_cmp actual expected +' + test_done