| Message ID | 4c3077f938435508850727e05ad514035f09bebb.1638939946.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | am: support --empty=(die|drop|keep) option and --allow-empty option to handle empty patches | expand |
""徐沛文 (Aleen)" via GitGitGadget" <gitgitgadget@gmail.com> writes: > @@ -1152,6 +1152,12 @@ static void NORETURN die_user_resolve(const struct am_state *state) > > printf_ln(_("When you have resolved this problem, run \"%s --continue\"."), cmdline); > printf_ln(_("If you prefer to skip this patch, run \"%s --skip\" instead."), cmdline); > + > + if (advice_enabled(ADVICE_AM_WORK_DIR) && > + is_empty_or_missing_file(am_path(state, "patch")) && > + !repo_index_has_changes(the_repository, NULL, NULL)) > + printf_ln(_("To record the empty patch as an empty commit, run \"%s --allow-empty\"."), cmdline); > + OK, so the idea is when we give back control to the user, if the reason we stopped is because we did not have a usable patch in the message and we didn't have any change to the index, we can offer "--allow-empty" as an extra choice. I guess that makes sense. > @@ -1900,19 +1906,31 @@ next: > /** > * Resume the current am session after patch application failure. The user did > * all the hard work, and we do not have to do any patch application. Just > - * trust and commit what the user has in the index and working tree. > + * trust and commit what the user has in the index and working tree. If `allow_empty` > + * is true, commit as an empty commit when index has not changed and lacking a patch. > */ > -static void am_resolve(struct am_state *state) > +static void am_resolve(struct am_state *state, int allow_empty) > { > + int index_changed; > + > validate_resume_state(state); > > say(state, stdout, _("Applying: %.*s"), linelen(state->msg), state->msg); > > - if (!repo_index_has_changes(the_repository, NULL, NULL)) { > - printf_ln(_("No changes - did you forget to use 'git add'?\n" > - "If there is nothing left to stage, chances are that something else\n" > - "already introduced the same changes; you might want to skip this patch.")); > + index_changed = repo_index_has_changes(the_repository, NULL, NULL); > + if (allow_empty && > + !(!index_changed && is_empty_or_missing_file(am_path(state, "patch")))) > die_user_resolve(state); Why do we need this? Intuitively, because "--allow-XYZ" is "we normally die when the condition XYZ holds, but with the option, we do not die in such a case", any new condition that leads to "die" that only kicks in when "--allow-XYZ" is given smell like a BUG. The condition reads: unless we saw an empty patch (i.e. "patch" file is missing or empty, and did not result in any change to the index), it is an error to give "--allow-empty" to the command. That somehow does not make any sense to me. Whether failed patch application and manual tweaking resulted in the same tree as HEAD, or a tree different from HEAD, if the user wants to say "I allow Git to create an empty commit as necessary, instead of stopping", shouldn't the user be allowed to? After all, the option is not "create nothing but an empty commit, it is an error if there is something to commit." It merely allows creation of an empty one. This series has been trying to add code to punish users who give "--allow-empty" when the command would have worked the same way without it at least since the last round, and I am not sure where that wish to stop users comes from. Please explain. I am starting to think I may be missing something and this extra rigidity may be helping a user (but again, I do not see how). > + if (!index_changed) { > + if (allow_empty) { The index_changed variable being false is "the result is an empty change", and is-empty-or-missing on "patch" file is "the input is an empty change". Ideally we would want to create an empty commit only when both input and result are empty, but in order to prevent mistakes, we may want to react to the case where the result is empty but the input is not empty. Here is where we can use is-empty-or-missing on "patch" and give a different message (i.e. "even though the patch file is not empty, the result of your patch application did not change anything"), if we wanted to. Or you could choose to reject such an attempt, which might be simpler. I.e. if (allow_empty && is_empty_or_missing_file(am_path(state, "patch")) "No changes - recorded an empty commit." else ... the original error for no changes ... Hmm?
> > + index_changed = repo_index_has_changes(the_repository, NULL, NULL); > > + if (allow_empty && > > + !(!index_changed && is_empty_or_missing_file(am_path(state, "patch")))) > > die_user_resolve(state); > > Why do we need this? > > Intuitively, because "--allow-XYZ" is "we normally die when the > condition XYZ holds, but with the option, we do not die in such a > case", any new condition that leads to "die" that only kicks in > when "--allow-XYZ" is given smell like a BUG. > > The condition reads: > > unless we saw an empty patch (i.e. "patch" file is missing or > empty, and did not result in any change to the index), it is an > error to give "--allow-empty" to the command. > > That somehow does not make any sense to me. Whether failed patch > application and manual tweaking resulted in the same tree as HEAD, > or a tree different from HEAD, if the user wants to say "I allow > Git to create an empty commit as necessary, instead of stopping", > shouldn't the user be allowed to? After all, the option is not > "create nothing but an empty commit, it is an error if there is > something to commit." It merely allows creation of an empty one. > > This series has been trying to add code to punish users who give > "--allow-empty" when the command would have worked the same way > without it at least since the last round, and I am not sure where > that wish to stop users comes from. Please explain. I am starting > to think I may be missing something and this extra rigidity may be > helping a user (but again, I do not see how). If there is no such code to check, the user can use "--allow-empty" to do the same thing as "--continue" or "--resolved" after resolving conflicts because they all go into `am_resolve()`. In my opinion, "--allow-empty" only makes sense to allow the user to create empty commits when: 1. the result is an empty change (`!index_changed`) 2. the input is an empty change (`is_empty_or_missing_file(am_path(state, "patch"))`) > > + if (!index_changed) { > > + if (allow_empty) { > > The index_changed variable being false is "the result is an empty > change", and is-empty-or-missing on "patch" file is "the input is an > empty change". Ideally we would want to create an empty commit only > when both input and result are empty, but in order to prevent mistakes, > we may want to react to the case where the result is empty but the > input is not empty. > > Here is where we can use is-empty-or-missing on "patch" and give a > different message (i.e. "even though the patch file is not empty, > the result of your patch application did not change anything"), if > we wanted to. Or you could choose to reject such an attempt, which > might be simpler. I.e. > > if (allow_empty && > is_empty_or_missing_file(am_path(state, "patch")) > "No changes - recorded an empty commit." > else > ... the original error for no changes ... > > > Hmm? Based on the previous checking, there is no need to check `is_empty_or_missing_file(am_path(state, "patch"))` here because it is permanently true.
Aleen 徐沛文 <pwxu@coremail.cn> writes: >> This series has been trying to add code to punish users who give >> "--allow-empty" when the command would have worked the same way >> without it at least since the last round, and I am not sure where >> that wish to stop users comes from. Please explain. I am starting >> to think I may be missing something and this extra rigidity may be >> helping a user (but again, I do not see how). > > If there is no such code to check, the user can use > "--allow-empty" to do the same thing as "--continue" or > "--resolved" after resolving conflicts because they all go into > `am_resolve()`. Yes, exactly. That is the point of "allow"; we continue normally, and even in a narrow situation (i.e. input is empty, and no change is made to the index) where we normally stop, we allow it to continue by creating an empty commit. > In my opinion, "--allow-empty" only makes sense > to allow the user to create empty commits when: > > 1. the result is an empty change (`!index_changed`) > 2. the input is an empty change (`is_empty_or_missing_file(am_path(state, "patch"))`) Then you are forcing the user to use it ONLY whe nhe input is empty and no other time. Whatever that is, that is not "allow" empty. >> Here is where we can use is-empty-or-missing on "patch" and give a >> different message (i.e. "even though the patch file is not empty, >> the result of your patch application did not change anything"), if >> we wanted to. Or you could choose to reject such an attempt, which >> might be simpler. I.e. >> >> if (allow_empty && >> is_empty_or_missing_file(am_path(state, "patch")) >> "No changes - recorded an empty commit." >> else >> ... the original error for no changes ... >> >> >> Hmm? > > Based on the previous checking, there is no need to check > `is_empty_or_missing_file(am_path(state, "patch"))` here because it is permanently true. I think you misunderstood. If we correct the overly strict check that makes "--allow-empty" to mean "only create empty, anything else is an error", the "is patch an empty file?" must be checked here.
diff --git a/Documentation/git-am.txt b/Documentation/git-am.txt index 7676bd58ae7..09107fb1067 100644 --- a/Documentation/git-am.txt +++ b/Documentation/git-am.txt @@ -18,7 +18,7 @@ SYNOPSIS [--quoted-cr=<action>] [--empty=(stop|drop|keep)] [(<mbox> | <Maildir>)...] -'git am' (--continue | --skip | --abort | --quit | --show-current-patch[=(diff|raw)]) +'git am' (--continue | --skip | --abort | --quit | --show-current-patch[=(diff|raw)] | --allow-empty) DESCRIPTION ----------- @@ -200,6 +200,11 @@ default. You can use `--no-utf8` to override this. the e-mail message; if `diff`, show the diff portion only. Defaults to `raw`. +--allow-empty:: + After a patch failure on an input e-mail message lacking a patch, + create an empty commit with the contents of the e-mail message + as its log message. + DISCUSSION ---------- diff --git a/builtin/am.c b/builtin/am.c index 7a66ad23737..d73e415bbb0 100644 --- a/builtin/am.c +++ b/builtin/am.c @@ -1152,6 +1152,12 @@ static void NORETURN die_user_resolve(const struct am_state *state) printf_ln(_("When you have resolved this problem, run \"%s --continue\"."), cmdline); printf_ln(_("If you prefer to skip this patch, run \"%s --skip\" instead."), cmdline); + + if (advice_enabled(ADVICE_AM_WORK_DIR) && + is_empty_or_missing_file(am_path(state, "patch")) && + !repo_index_has_changes(the_repository, NULL, NULL)) + printf_ln(_("To record the empty patch as an empty commit, run \"%s --allow-empty\"."), cmdline); + printf_ln(_("To restore the original branch and stop patching, run \"%s --abort\"."), cmdline); } @@ -1900,19 +1906,31 @@ next: /** * Resume the current am session after patch application failure. The user did * all the hard work, and we do not have to do any patch application. Just - * trust and commit what the user has in the index and working tree. + * trust and commit what the user has in the index and working tree. If `allow_empty` + * is true, commit as an empty commit when index has not changed and lacking a patch. */ -static void am_resolve(struct am_state *state) +static void am_resolve(struct am_state *state, int allow_empty) { + int index_changed; + validate_resume_state(state); say(state, stdout, _("Applying: %.*s"), linelen(state->msg), state->msg); - if (!repo_index_has_changes(the_repository, NULL, NULL)) { - printf_ln(_("No changes - did you forget to use 'git add'?\n" - "If there is nothing left to stage, chances are that something else\n" - "already introduced the same changes; you might want to skip this patch.")); + index_changed = repo_index_has_changes(the_repository, NULL, NULL); + if (allow_empty && + !(!index_changed && is_empty_or_missing_file(am_path(state, "patch")))) die_user_resolve(state); + + if (!index_changed) { + if (allow_empty) { + printf_ln(_("No changes - recorded it as an empty commit.")); + } else { + printf_ln(_("No changes - did you forget to use 'git add'?\n" + "If there is nothing left to stage, chances are that something else\n" + "already introduced the same changes; you might want to skip this patch.")); + die_user_resolve(state); + } } if (unmerged_cache()) { @@ -2239,7 +2257,8 @@ enum resume_type { RESUME_SKIP, RESUME_ABORT, RESUME_QUIT, - RESUME_SHOW_PATCH + RESUME_SHOW_PATCH, + RESUME_ALLOW_EMPTY, }; struct resume_mode { @@ -2392,6 +2411,9 @@ int cmd_am(int argc, const char **argv, const char *prefix) N_("show the patch being applied"), PARSE_OPT_CMDMODE | PARSE_OPT_OPTARG | PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP, parse_opt_show_current_patch, RESUME_SHOW_PATCH }, + OPT_CMDMODE(0, "allow-empty", &resume.mode, + N_("record the empty patch as an empty commit"), + RESUME_ALLOW_EMPTY), OPT_BOOL(0, "committer-date-is-author-date", &state.committer_date_is_author_date, N_("lie about committer date")), @@ -2500,7 +2522,8 @@ int cmd_am(int argc, const char **argv, const char *prefix) am_run(&state, 1); break; case RESUME_RESOLVED: - am_resolve(&state); + case RESUME_ALLOW_EMPTY: + am_resolve(&state, resume.mode == RESUME_ALLOW_EMPTY ? 1 : 0); break; case RESUME_SKIP: am_skip(&state); diff --git a/t/t4150-am.sh b/t/t4150-am.sh index f2a7a68eda0..6b23384685b 100755 --- a/t/t4150-am.sh +++ b/t/t4150-am.sh @@ -1202,4 +1202,58 @@ test_expect_success 'record as an empty commit when meeting e-mail message that grep "Creating an empty commit: empty commit" output ' +test_expect_success 'skip an empty patch in the middle of an am session' ' + git checkout empty-commit^ && + test_must_fail git am empty-commit.patch >err && + grep "Patch is empty." err && + grep "To record the empty patch as an empty commit, run \"git am --allow-empty\"." err && + git am --skip && + test_path_is_missing .git/rebase-apply && + git rev-parse empty-commit^ >expected && + git rev-parse HEAD >actual && + test_cmp expected actual +' + +test_expect_success 'record an empty patch as an empty commit in the middle of an am session' ' + git checkout empty-commit^ && + test_must_fail git am empty-commit.patch >err && + grep "Patch is empty." err && + grep "To record the empty patch as an empty commit, run \"git am --allow-empty\"." err && + git am --allow-empty >output && + grep "No changes - recorded it as an empty commit." output && + test_path_is_missing .git/rebase-apply && + git show empty-commit --format="%B" >expected && + git show HEAD --format="%B" >actual && + grep -f actual expected +' + +test_expect_success 'cannot create empty commits when the index is changed' ' + git checkout empty-commit^ && + test_must_fail git am empty-commit.patch >err && + : >empty-file && + git add empty-file && + test_must_fail git am --allow-empty >err && + ! grep "To record the empty patch as an empty commit, run \"git am --allow-empty\"." err +' + +test_expect_success 'cannot create empty commits when there is a clean index due to merge conflicts' ' + test_when_finished "git am --abort || :" && + git rev-parse HEAD >expected && + test_must_fail git am seq.patch && + test_must_fail git am --allow-empty >err && + ! grep "To record the empty patch as an empty commit, run \"git am --allow-empty\"." err && + git rev-parse HEAD >actual && + test_cmp actual expected +' + +test_expect_success 'cannot create empty commits when there is unmerged index due to merge conflicts' ' + test_when_finished "git am --abort || :" && + git rev-parse HEAD >expected && + test_must_fail git am -3 seq.patch && + test_must_fail git am --allow-empty >err && + ! grep "To record the empty patch as an empty commit, run \"git am --allow-empty\"." err && + git rev-parse HEAD >actual && + test_cmp actual expected +' + test_done diff --git a/t/t7512-status-help.sh b/t/t7512-status-help.sh index 7f2956d77ad..2f16d5787ed 100755 --- a/t/t7512-status-help.sh +++ b/t/t7512-status-help.sh @@ -659,6 +659,7 @@ On branch am_empty You are in the middle of an am session. The current patch is empty. (use "git am --skip" to skip this patch) + (use "git am --allow-empty" to record this patch as an empty commit) (use "git am --abort" to restore the original branch) nothing to commit (use -u to show untracked files) diff --git a/wt-status.c b/wt-status.c index 5d215f4e4f1..335e723a71e 100644 --- a/wt-status.c +++ b/wt-status.c @@ -1218,17 +1218,23 @@ static void show_merge_in_progress(struct wt_status *s, static void show_am_in_progress(struct wt_status *s, const char *color) { + int am_empty_patch; + status_printf_ln(s, color, _("You are in the middle of an am session.")); if (s->state.am_empty_patch) status_printf_ln(s, color, _("The current patch is empty.")); if (s->hints) { - if (!s->state.am_empty_patch) + am_empty_patch = s->state.am_empty_patch; + if (!am_empty_patch) status_printf_ln(s, color, _(" (fix conflicts and then run \"git am --continue\")")); status_printf_ln(s, color, _(" (use \"git am --skip\" to skip this patch)")); + if (am_empty_patch) + status_printf_ln(s, color, + _(" (use \"git am --allow-empty\" to record this patch as an empty commit)")); status_printf_ln(s, color, _(" (use \"git am --abort\" to restore the original branch)")); }