git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: "\"\"徐沛文  via GitGitGadget" <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, "René Scharfe" <l.s.r@web.de>,
	"Phillip Wood" <phillip.wood123@gmail.com>,
	"Johannes Schindelin" <Johannes.Schindelin@gmx.de>,
	"Elijah Newren" <newren@gmail.com>,
	"Aleen 徐沛文" <pwxu@coremail.cn>, Aleen <aleen42@vip.qq.com>
Subject: Re: [PATCH v14 3/3] am: support --allow-empty to record specific empty patches
Date: Wed, 01 Dec 2021 16:58:05 -0800	[thread overview]
Message-ID: <xmqqy253x1pe.fsf@gitster.g> (raw)
In-Reply-To: <cbd822d4340f8f3d832e9d6209ee8db6e538fca1.1638329848.git.gitgitgadget@gmail.com> ("\"\"徐沛文 (Aleen)\" via GitGitGadget\""'s message of "Wed, 01 Dec 2021 03:37:28 +0000")

""徐沛文 (Aleen)" via GitGitGadget"  <gitgitgadget@gmail.com>
writes:

> +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 "If you want to record it 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 "If you want to record it as an empty commit, run \"git am --allow-empty\"." err &&
> +	git am --allow-empty &&
> +	test_path_is_missing .git/rebase-apply &&
> +	git show empty-commit --format="%s" >expected &&
> +	git show HEAD --format="%s" >actual &&
> +	test_cmp actual expected
> +'

These two are "positive" tests, i.e. the feature does the right
thing when used in the expected situation.

I am worried about the cases where "--allow-empty" is used and
creates a commit that is empty, when it is reasonably expected to
either create a non-empty commit or still fail the same way to give
the user another chance to try.

For example, when "git am -3" fails on such an email message (there
are two failure modes: (a) one results in an unmerged index due to
merge conflicts, (b) the other results in a clean index due to not
finding the blob object recorded as a preimage), what happens when
the user runs "git am --allow-empty", after:

 (1) doing nothing since the failed "git am -3" application,
 (2) resolving the conflicts in working tree, or
 (3) resolving the conflicts in working tree and recording the
     resolution in the index.

There are 6 cases in the above matrix, and "git am --allow-empty"
should not create an empty commit in any one of them.  In 1a and 1b,
"git am --allow-empty" should continue to fail ("git am --skip"
would be a way to ignore and proceed).  3a and 3b should record a
non-empty commit as the result.

Similarly, when an email message with a patch is given to "git am"
and the application fails, and the user runs "git am --allow-empty"
without doing anything in the working tree, what happens?  It should
not happily create an empty commit with only the log message,
without the change, but continue to fail.  If the user resolves and
registers the resolution to the index before "git am --allow-empty",
then the command should create a non-empty commit.

Having positive tests are of course important to avoid regression in
the future, but negative tests, the new and shiny feature does not
misbehave when it shouldn't even kick in, is even more important.

Thanks.

  reply	other threads:[~2021-12-02  0:58 UTC|newest]

Thread overview: 129+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-11-12  4:58 [PATCH 0/2] am: support --always option to am empty commits Aleen via GitGitGadget
2021-11-12  4:58 ` [PATCH 1/2] doc: git-format-patch: specify the option --always Aleen via GitGitGadget
2021-11-12  4:58 ` [PATCH 2/2] am: support --always option to am empty commits Aleen via GitGitGadget
2021-11-12  6:17 ` [PATCH 0/2] " René Scharfe
2021-11-12  6:42   ` Aleen
2021-11-12  6:47   ` Junio C Hamano
2021-11-12  7:10     ` Aleen 徐沛文
2021-11-12 15:28     ` René Scharfe
2021-11-12 16:08       ` Junio C Hamano
2021-11-12  6:53 ` [PATCH v2 0/4] am: support --allow-empty " Aleen via GitGitGadget
2021-11-12  6:53   ` [PATCH v2 1/4] doc: git-format-patch: specify the option --always Aleen via GitGitGadget
2021-11-12 22:17     ` Junio C Hamano
2021-11-12  6:53   ` [PATCH v2 2/4] am: support --always option to am empty commits Aleen via GitGitGadget
2021-11-12 22:23     ` Junio C Hamano
2021-11-12  6:53   ` [PATCH v2 3/4] test: am: add the case when not passing the --always option Aleen via GitGitGadget
2021-11-12  6:54   ` [PATCH v2 4/4] chore: am: rename the --always option to --allow-empty Aleen via GitGitGadget
2021-11-15 10:39   ` [PATCH v3 0/2] am: support --empty-commit=(die|skip|asis) option to am empty commits Aleen via GitGitGadget
2021-11-15 10:39     ` [PATCH v3 1/2] doc: git-format-patch: describe the option --always Aleen via GitGitGadget
2021-11-15 10:39     ` [PATCH v3 2/2] am: support --empty-commit option to handle empty patches Aleen via GitGitGadget
2021-11-15 11:13       ` Aleen 徐沛文
2021-11-16  5:18     ` [PATCH v4 0/2] am: support --empty-commit=(die|skip|asis) option to am empty commits Aleen via GitGitGadget
2021-11-16  5:18       ` [PATCH v4 1/2] doc: git-format-patch: describe the option --always Aleen via GitGitGadget
2021-11-16  5:18       ` [PATCH v4 2/2] am: support --empty-commit option to handle empty patches Aleen via GitGitGadget
2021-11-16 10:07         ` Phillip Wood
2021-11-16 10:31           ` Aleen 徐沛文
2021-11-17  8:39           ` Junio C Hamano
2021-11-17  9:33       ` [PATCH v5 0/2] am: support --empty-commit=(die|skip|asis) option to am empty commits Aleen via GitGitGadget
2021-11-17  9:33         ` [PATCH v5 1/2] doc: git-format-patch: describe the option --always Aleen via GitGitGadget
2021-11-17  9:33         ` [PATCH v5 2/2] am: support --empty option to handle empty patches Aleen via GitGitGadget
2021-11-18 10:50         ` [PATCH v6 0/3] am: support --empty=(die|drop|keep) " Aleen via GitGitGadget
2021-11-18 10:50           ` [PATCH v6 1/3] doc: git-format-patch: describe the option --always Aleen via GitGitGadget
2021-11-18 10:50           ` [PATCH v6 2/3] am: support --empty option to handle empty patches Aleen via GitGitGadget
2021-11-19  0:56             ` Junio C Hamano
2021-11-19 10:33             ` Bagas Sanjaya
2021-11-19 12:10               ` Ævar Arnfjörð Bjarmason
2021-11-19 12:20                 ` Eric Sunshine
2021-11-19 16:49                   ` Junio C Hamano
2021-11-19 16:46               ` Junio C Hamano
2021-11-18 10:50           ` [PATCH v6 3/3] am: throw an error when passing --empty option without value Aleen via GitGitGadget
2021-11-19  1:13             ` Junio C Hamano
2021-11-19  2:11               ` Aleen 徐沛文
2021-11-18 23:47           ` [PATCH v6 0/3] am: support --empty=(die|drop|keep) option to handle empty patches Junio C Hamano
2021-11-19  1:45             ` Aleen 徐沛文
2021-11-19  5:46               ` Junio C Hamano
2021-11-19  7:23                 ` Aleen 徐沛文
2021-11-19  7:25                   ` =?gb18030?B?QWxlZW4=?=
2021-11-19 16:54                   ` Junio C Hamano
2021-11-19 17:14                     ` Aleen 徐沛文
2021-11-19 19:25                       ` Junio C Hamano
2021-11-22 11:57                     ` Johannes Schindelin
2021-11-19  4:16             ` Aleen 徐沛文
2021-11-19  5:04           ` [PATCH v7 0/2] " Aleen via GitGitGadget
2021-11-19  5:04             ` [PATCH v7 1/2] doc: git-format-patch: describe the option --always Aleen via GitGitGadget
2021-11-19  5:04             ` [PATCH v7 2/2] am: support --empty=<option> to handle empty patches Aleen via GitGitGadget
2021-11-19 22:50               ` Junio C Hamano
2021-11-19 23:07               ` Junio C Hamano
2021-11-22  6:46             ` [PATCH v8 0/2] am: support --empty=(die|drop|keep) option " Aleen via GitGitGadget
2021-11-22  6:46               ` [PATCH v8 1/2] doc: git-format-patch: describe the option --always Aleen via GitGitGadget
2021-11-22  6:46               ` [PATCH v8 2/2] am: support --empty=<option> to handle empty patches Aleen via GitGitGadget
2021-11-22  7:06                 ` Junio C Hamano
2021-11-22  7:19                   ` Aleen 徐沛文
2021-11-22  7:02               ` [PATCH v9 0/2] am: support --empty=(die|drop|keep) option " Aleen via GitGitGadget
2021-11-22  7:02                 ` [PATCH v9 1/2] doc: git-format-patch: describe the option --always Aleen 徐沛文 via GitGitGadget
2021-11-22  7:02                 ` [PATCH v9 2/2] am: support --empty=<option> to handle empty patches Aleen 徐沛文 via GitGitGadget
2021-11-22  7:04                   ` Aleen 徐沛文
2021-11-22  7:51                 ` [PATCH v10 0/2] am: support --empty=(die|drop|keep) option " Aleen via GitGitGadget
2021-11-22  7:51                   ` [PATCH v10 1/2] doc: git-format-patch: describe the option --always Aleen via GitGitGadget
2021-11-22 12:00                     ` Johannes Schindelin
2021-11-23  1:25                       ` Aleen 徐沛文
2021-11-23 12:30                         ` Johannes Schindelin
2021-11-22  7:51                   ` [PATCH v10 2/2] am: support --empty=<option> to handle empty patches Aleen via GitGitGadget
2021-11-23 15:26                   ` [PATCH v11 0/2] am: support --empty=(die|drop|keep) option " Aleen via GitGitGadget
2021-11-23 15:26                     ` [PATCH v11 1/2] doc: git-format-patch: describe the option --always 徐沛文 (Aleen) via GitGitGadget
2021-11-23 16:12                       ` Johannes Schindelin
2021-11-23 22:02                         ` Junio C Hamano
2021-11-23 15:26                     ` [PATCH v11 2/2] am: support --empty=<option> to handle empty patches 徐沛文 (Aleen) via GitGitGadget
2021-11-26 20:14                       ` Elijah Newren
2021-11-29  9:19                         ` Aleen 徐沛文
2021-11-29 10:00                           ` Aleen 徐沛文
2021-11-29 17:10                             ` Elijah Newren
2021-11-30  5:45                               ` [PATCH v12 3/3] am: support --allow-empty to record specific " Aleen 徐沛文
2021-11-29 18:17                         ` [PATCH v11 2/2] am: support --empty=<option> to handle " Junio C Hamano
2021-11-29 18:57                           ` Elijah Newren
2021-11-30  5:37                     ` [PATCH v12 0/3] am: support --empty=(die|drop|keep) option and --allow-empty option " Aleen via GitGitGadget
2021-11-30  5:37                       ` [PATCH v12 1/3] doc: git-format-patch: describe the option --always 徐沛文 (Aleen) via GitGitGadget
2021-11-30  5:37                       ` [PATCH v12 2/3] am: support --empty=<option> to handle empty patches 徐沛文 (Aleen) via GitGitGadget
2021-11-30  5:37                       ` [PATCH v12 3/3] am: support --allow-empty to record specific " 徐沛文 (Aleen) via GitGitGadget
2021-11-30  7:21                         ` Junio C Hamano
2021-11-30  9:55                       ` [PATCH v13 0/3] am: support --empty=(die|drop|keep) option and --allow-empty option to handle " Aleen via GitGitGadget
2021-11-30  9:55                         ` [PATCH v13 1/3] doc: git-format-patch: describe the option --always 徐沛文 (Aleen) via GitGitGadget
2021-11-30  9:55                         ` [PATCH v13 2/3] am: support --empty=<option> to handle empty patches 徐沛文 (Aleen) via GitGitGadget
2021-11-30  9:55                         ` [PATCH v13 3/3] am: support --allow-empty to record specific " 徐沛文 (Aleen) via GitGitGadget
2021-12-01  3:37                         ` [PATCH v14 0/3] am: support --empty=(die|drop|keep) option and --allow-empty option to handle " Aleen via GitGitGadget
2021-12-01  3:37                           ` [PATCH v14 1/3] doc: git-format-patch: describe the option --always 徐沛文 (Aleen) via GitGitGadget
2021-12-01  3:37                           ` [PATCH v14 2/3] am: support --empty=<option> to handle empty patches 徐沛文 (Aleen) via GitGitGadget
2021-12-03 22:30                             ` Johannes Schindelin
2021-12-01  3:37                           ` [PATCH v14 3/3] am: support --allow-empty to record specific " 徐沛文 (Aleen) via GitGitGadget
2021-12-02  0:58                             ` Junio C Hamano [this message]
2021-12-06  1:35                               ` Aleen 徐沛文
2021-12-06  9:41                           ` [PATCH v15 0/3] am: support --empty=(die|drop|keep) option and --allow-empty option to handle " Aleen via GitGitGadget
2021-12-06  9:41                             ` [PATCH v15 1/3] doc: git-format-patch: describe the option --always 徐沛文 (Aleen) via GitGitGadget
2021-12-06  9:41                             ` [PATCH v15 2/3] am: support --empty=<option> to handle empty patches 徐沛文 (Aleen) via GitGitGadget
2021-12-06  9:41                             ` [PATCH v15 3/3] am: support --allow-empty to record specific " 徐沛文 (Aleen) via GitGitGadget
2021-12-07  5:01                             ` [PATCH v16 0/3] am: support --empty=(die|drop|keep) option and --allow-empty option to handle " Aleen via GitGitGadget
2021-12-07  5:01                               ` [PATCH v16 1/3] doc: git-format-patch: describe the option --always 徐沛文 (Aleen) via GitGitGadget
2021-12-07  5:01                               ` [PATCH v16 2/3] am: support --empty=<option> to handle empty patches 徐沛文 (Aleen) via GitGitGadget
2021-12-07  5:01                               ` [PATCH v16 3/3] am: support --allow-empty to record specific " 徐沛文 (Aleen) via GitGitGadget
2021-12-07  8:31                               ` [PATCH v17 0/3] am: support --empty=(die|drop|keep) option and --allow-empty option to handle " Aleen via GitGitGadget
2021-12-07  8:31                                 ` [PATCH v17 1/3] doc: git-format-patch: describe the option --always 徐沛文 (Aleen) via GitGitGadget
2021-12-07  8:31                                 ` [PATCH v17 2/3] am: support --empty=<option> to handle empty patches 徐沛文 (Aleen) via GitGitGadget
2021-12-07 18:12                                   ` Junio C Hamano
2021-12-07  8:31                                 ` [PATCH v17 3/3] am: support --allow-empty to record specific " 徐沛文 (Aleen) via GitGitGadget
2021-12-07 18:23                                   ` Junio C Hamano
2021-12-07 18:24                                 ` [PATCH v17 0/3] am: support --empty=(die|drop|keep) option and --allow-empty option to handle " Junio C Hamano
2021-12-08  5:05                                 ` [PATCH v18 " Aleen via GitGitGadget
2021-12-08  5:05                                   ` [PATCH v18 1/3] doc: git-format-patch: describe the option --always 徐沛文 (Aleen) via GitGitGadget
2021-12-08  5:05                                   ` [PATCH v18 2/3] am: support --empty=<option> to handle empty patches 徐沛文 (Aleen) via GitGitGadget
2021-12-08  5:05                                   ` [PATCH v18 3/3] am: support --allow-empty to record specific " 徐沛文 (Aleen) via GitGitGadget
2021-12-08  6:22                                     ` Junio C Hamano
2021-12-08  6:46                                       ` Aleen 徐沛文
2021-12-08 11:32                                         ` Junio C Hamano
2021-12-09  7:25                                   ` [PATCH v19 0/3] am: support --empty=(die|drop|keep) option and --allow-empty option to handle " Aleen via GitGitGadget
2021-12-09  7:25                                     ` [PATCH v19 1/3] doc: git-format-patch: describe the option --always 徐沛文 (Aleen) via GitGitGadget
2021-12-09  9:28                                       ` Bagas Sanjaya
2021-12-10  1:26                                         ` Aleen 徐沛文
2021-12-10  6:50                                           ` Bagas Sanjaya
2021-12-11  9:22                                             ` Junio C Hamano
2021-12-09  7:25                                     ` [PATCH v19 2/3] am: support --empty=<option> to handle empty patches 徐沛文 (Aleen) via GitGitGadget
2021-12-09  7:25                                     ` [PATCH v19 3/3] am: support --allow-empty to record specific " 徐沛文 (Aleen) via GitGitGadget

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqy253x1pe.fsf@gitster.g \
    --to=gitster@pobox.com \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=aleen42@vip.qq.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=l.s.r@web.de \
    --cc=newren@gmail.com \
    --cc=phillip.wood123@gmail.com \
    --cc=pwxu@coremail.cn \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).