Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

am: support --empty=(stop|drop|keep) option and --allow-empty option to handle empty patches #1076

Closed
wants to merge 3 commits into from

Conversation

aleen42
Copy link

@aleen42 aleen42 commented Nov 11, 2021

Since that git has supported the --always option for the git-format-patch command to create a patch with an empty commit message, git-am should support applying and committing with empty patches.


Changes since v1:

  1. add a case when not passing the --always option.
  2. rename the --always option to --allow-empty.

Changes since v2:

  1. rename the --allow-empty option to --empty-commit.
  2. introduce three different strategies (die|skip|asis) when trying to record empty patches as empty commits.

Changes since v3:

  1. generate the missed file for test cases.
  2. grep -f cannot be used under Mac OS.

Changes since v4:

  1. rename the --empty-commit option to --empty.
  2. rename three different strategies (die|skip|asis) to die, drop and keep correspondingly.

Changes since v5:

  1. throw an error when passing --empty option without value.

Changes since v6:

  1. add i18n resources.

Changes since v7:

  1. update code according to the seen branch.
  2. fix the wrong document of git-am.
  3. sign off commits by a real name.

Changes since v8:

  1. update the committer's name with my real name to fix DCO of GGG.

Changes since v9:

  1. imitate the signed name format of https://lore.kernel.org/git/pull.1143.git.git.1637347813367.gitgitgadget@gmail.com .

Changes since v11:

  1. introduce an interactive option --allow-empty for git-am to record empty patches in the middle of an am session.

Changes since v12:

  1. record the empty patch as an empty commit only when there are no changes.
  2. fix indentation problems.
  3. simplify "to keep recording" to "to record".
  4. add a test case for skipping empty patches via the --skip option.

Changes since v13:

  1. add an additional description about the 'die' value.

Changes since v14:

  1. reimplement the 'die' value and stop the whole session. (Expected a reroll)
  2. the --allow-empty option is a valid resume value only when: (Expected a reroll)
    • index has not changed
    • lacking a patch

Changes since v15:

  1. rename "die" to "stop".

Changes since v16:

  1. fix the typo from "had" to "has" in the comment.

Changes since v17:

  1. add trailing comma, show tips of creating an empty commit, and use "%B" to construct test cases.
  2. remove the error "Invalid resume value.".
  3. hint "--allow-empty" after "--skip".

Changes since v18:

  1. remove strict checking of "--allow-empty".

cc: René Scharfe l.s.r@web.de
cc: Phillip Wood phillip.wood123@gmail.com
cc: Johannes Schindelin Johannes.Schindelin@gmx.de
cc: Elijah Newren newren@gmail.com
cc: Aleen 徐沛文 pwxu@coremail.cn
cc: Bagas Sanjaya bagasdotme@gmail.com

@gitgitgadget
Copy link

gitgitgadget bot commented Nov 11, 2021

Welcome to GitGitGadget

Hi @aleen42, and welcome to GitGitGadget, the GitHub App to send patch series to the Git mailing list from GitHub Pull Requests.

Please make sure that your Pull Request has a good description, as it will be used as cover letter.

Also, it is a good idea to review the commit messages one last time, as the Git project expects them in a quite specific form:

  • the lines should not exceed 76 columns,
  • the first line should be like a header and typically start with a prefix like "tests:" or "revisions:" to state which subsystem the change is about, and
  • the commit messages' body should be describing the "why?" of the change.
  • Finally, the commit messages should end in a Signed-off-by: line matching the commits' author.

It is in general a good idea to await the automated test ("Checks") in this Pull Request before contributing the patches, e.g. to avoid trivial issues such as unportable code.

Contributing the patches

Before you can contribute the patches, your GitHub username needs to be added to the list of permitted users. Any already-permitted user can do that, by adding a comment to your PR of the form /allow. A good way to find other contributors is to locate recent pull requests where someone has been /allowed:

Both the person who commented /allow and the PR author are able to /allow you.

An alternative is the channel #git-devel on the Libera Chat IRC network:

<newcontributor> I've just created my first PR, could someone please /allow me? https://github.com/gitgitgadget/git/pull/12345
<veteran> newcontributor: it is done
<newcontributor> thanks!

Once on the list of permitted usernames, you can contribute the patches to the Git mailing list by adding a PR comment /submit.

If you want to see what email(s) would be sent for a /submit request, add a PR comment /preview to have the email(s) sent to you. You must have a public GitHub email address for this.

After you submit, GitGitGadget will respond with another comment that contains the link to the cover letter mail in the Git mailing list archive. Please make sure to monitor the discussion in that thread and to address comments and suggestions (while the comments and suggestions will be mirrored into the PR by GitGitGadget, you will still want to reply via mail).

If you do not want to subscribe to the Git mailing list just to be able to respond to a mail, you can download the mbox from the Git mailing list archive (click the (raw) link), then import it into your mail program. If you use GMail, you can do this via:

curl -g --user "<EMailAddress>:<Password>" \
    --url "imaps://imap.gmail.com/INBOX" -T /path/to/raw.txt

To iterate on your change, i.e. send a revised patch or patch series, you will first want to (force-)push to the same branch. You probably also want to modify your Pull Request description (or title). It is a good idea to summarize the revision by adding something like this to the cover letter (read: by editing the first comment on the PR, i.e. the PR description):

Changes since v1:
- Fixed a typo in the commit message (found by ...)
- Added a code comment to ... as suggested by ...
...

To send a new iteration, just add another PR comment with the contents: /submit.

Need help?

New contributors who want advice are encouraged to join git-mentoring@googlegroups.com, where volunteers who regularly contribute to Git are willing to answer newbie questions, give advice, or otherwise provide mentoring to interested contributors. You must join in order to post or view messages, but anyone can join.

You may also be able to find help in real time in the developer IRC channel, #git-devel on Libera Chat. Remember that IRC does not support offline messaging, so if you send someone a private message and log out, they cannot respond to you. The scrollback of #git-devel is archived, though.

@dscho
Copy link
Member

dscho commented Nov 11, 2021

/allow

@gitgitgadget
Copy link

gitgitgadget bot commented Nov 11, 2021

User aleen42 is now allowed to use GitGitGadget.

@dscho
Copy link
Member

dscho commented Nov 11, 2021

Looks like the added test case is failing:

[...]
+ git am --always empty-commit.patch
error: unrecognized input
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Applying: empty commit
Patch failed at 0001 empty commit
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
error: last command exited with $?=128
not ok 72 - am empty commits
[...]

@dscho
Copy link
Member

dscho commented Nov 11, 2021

FWIW you can run those tests manually, on your local machine. See e.g. this wiki page.

@aleen42
Copy link
Author

aleen42 commented Nov 11, 2021

@dscho OK, I'll check it individually on my local machine.

@aleen42 aleen42 changed the title am: support --always option to am empty commits Draft: am: support --always option to am empty commits Nov 11, 2021
@aleen42 aleen42 changed the title Draft: am: support --always option to am empty commits am: support --always option to am empty commits Nov 12, 2021
@aleen42
Copy link
Author

aleen42 commented Nov 12, 2021

@dscho PR is ready to submit.

@aleen42
Copy link
Author

aleen42 commented Nov 12, 2021

/submit

@gitgitgadget
Copy link

gitgitgadget bot commented Nov 12, 2021

Submitted as pull.1076.git.1636693095.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git pr-1076/aleen42/next-v1

To fetch this version to local tag pr-1076/aleen42/next-v1:

git fetch --no-tags https://github.com/gitgitgadget/git tag pr-1076/aleen42/next-v1

@gitgitgadget
Copy link

gitgitgadget bot commented Nov 12, 2021

On the Git mailing list, René Scharfe wrote (reply to this):

Am 12.11.21 um 05:58 schrieb Aleen via GitGitGadget:
> Since that git has supported the --always option for the git-format-patch
> command to create a patch with empty commit message, git-am should support
> applying and committing with empty patches.

The symmetry is compelling, but "always" is quite generic.  I can see
e.g. someone expecting "git am --always" to imply --keep-non-patch.

git commit and cherry-pick have --allow-empty, which is (a bit) more
specific.  That seems to me a better option name to copy for a commit-
creating command like git am.

René

@gitgitgadget
Copy link

gitgitgadget bot commented Nov 12, 2021

User René Scharfe <l.s.r@web.de> has been added to the cc: list.

@gitgitgadget
Copy link

gitgitgadget bot commented Nov 12, 2021

On the Git mailing list, Junio C Hamano wrote (reply to this):

René Scharfe <l.s.r@web.de> writes:

> Am 12.11.21 um 05:58 schrieb Aleen via GitGitGadget:
>> Since that git has supported the --always option for the git-format-patch
>> command to create a patch with empty commit message, git-am should support
>> applying and committing with empty patches.
>
> The symmetry is compelling, but "always" is quite generic.  I can see
> e.g. someone expecting "git am --always" to imply --keep-non-patch.

What symmetry?

> git commit and cherry-pick have --allow-empty, which is (a bit) more
> specific.  That seems to me a better option name to copy for a commit-
> creating command like git am.

That one I can believe, even though I do not necessarily think it is
a good idea to add such an option.

@aleen42 aleen42 changed the title am: support --always option to am empty commits am: support --allow-empty option to am empty commits Nov 12, 2021
@aleen42
Copy link
Author

aleen42 commented Nov 12, 2021

/submit

@gitgitgadget
Copy link

gitgitgadget bot commented Nov 12, 2021

Submitted as pull.1076.v2.git.1636700040.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git pr-1076/aleen42/next-v2

To fetch this version to local tag pr-1076/aleen42/next-v2:

git fetch --no-tags https://github.com/gitgitgadget/git tag pr-1076/aleen42/next-v2

@gitgitgadget
Copy link

gitgitgadget bot commented Nov 12, 2021

On the Git mailing list, "Aleen" wrote (reply to this):

Dears Ren�,

> The symmetry is compelling, but "always" is quite generic.  I can see
> e.g. someone expecting "git am --always" to imply --keep-non-patch.

> git commit and cherry-pick have --allow-empty, which is (a bit) more
> specific.  That seems to me a better option name to copy for a commit-
> creating command like git am.

It was designed corresponding to --always option in git-format-patch, which
will be pssed into git-diff-tree. As a commit-creating command, --allow-empty
is apparently a better choice. I will re-submit it, thank you.

@gitgitgadget
Copy link

gitgitgadget bot commented Nov 12, 2021

On the Git mailing list, Aleen 徐沛文 wrote (reply to this):

Dears Junio C Hamano:

> René Scharfe <l.s.r@web.de> writes:
> 
> > Am 12.11.21 um 05:58 schrieb Aleen via GitGitGadget:
> >> Since that git has supported the --always option for the git-format-patch
> >> command to create a patch with empty commit message, git-am should support
> >> applying and committing with empty patches.
> >
> > The symmetry is compelling, but "always" is quite generic.  I can see
> > e.g. someone expecting "git am --always" to imply --keep-non-patch.
> 
> What symmetry?
> 
> > git commit and cherry-pick have --allow-empty, which is (a bit) more
> > specific.  That seems to me a better option name to copy for a commit-
> > creating command like git am.
> 
> That one I can believe, even though I do not necessarily think it is
> a good idea to add such an option.

In some cases when we need to migrate commits from one branch to another branch,
from one repo to another repo, it should be necessary.

Aleen

@gitgitgadget
Copy link

gitgitgadget bot commented Nov 12, 2021

User Aleen 徐沛文 <pwxu@coremail.cn> has been added to the cc: list.

@gitgitgadget
Copy link

gitgitgadget bot commented Nov 12, 2021

On the Git mailing list, René Scharfe wrote (reply to this):

Am 12.11.21 um 07:47 schrieb Junio C Hamano:
> René Scharfe <l.s.r@web.de> writes:
>
>> Am 12.11.21 um 05:58 schrieb Aleen via GitGitGadget:
>>> Since that git has supported the --always option for the git-format-patch
>>> command to create a patch with empty commit message, git-am should support
>>> applying and committing with empty patches.
>>
>> The symmetry is compelling, but "always" is quite generic.  I can see
>> e.g. someone expecting "git am --always" to imply --keep-non-patch.
>
> What symmetry?

To have the same option in both producer and consumer.

René

@gitgitgadget
Copy link

gitgitgadget bot commented Nov 12, 2021

User René Scharfe <l.s.r@web.de> has been added to the cc: list.

@gitgitgadget
Copy link

gitgitgadget bot commented Nov 12, 2021

On the Git mailing list, Junio C Hamano wrote (reply to this):

René Scharfe <l.s.r@web.de> writes:

>>> The symmetry is compelling, but "always" is quite generic.  I can see
>>> e.g. someone expecting "git am --always" to imply --keep-non-patch.
>>
>> What symmetry?
>
> To have the same option in both producer and consumer.

Well, "--always" that stands for "--always-show-header" seeps
through to the command line parser from revision.c layer but that is
by accident, and is not even documented.  We may want to file it as
a bug and fix it later.

As you mentioned, allow-empty-commit does sound like a much better
name.


@gitgitgadget
Copy link

gitgitgadget bot commented Dec 15, 2021

This patch series was integrated into seen via git@ee90fda.

@gitgitgadget
Copy link

gitgitgadget bot commented Dec 16, 2021

There was a status update in the "Cooking" section about the branch xw/am-empty on the Git mailing list:

"git am" learns "--empty=3D(die|drop|keep)" option to tweak what is
done to a piece of e-mail without a patch in it.

Almost there.
source: <pull.1076.v18.git.1638939946.gitgitgadget@gmail.com>

@gitgitgadget
Copy link

gitgitgadget bot commented Dec 16, 2021

This patch series was integrated into seen via git@02e156f.

@gitgitgadget
Copy link

gitgitgadget bot commented Dec 16, 2021

This patch series was integrated into seen via git@5082b96.

@gitgitgadget
Copy link

gitgitgadget bot commented Dec 17, 2021

This patch series was integrated into seen via git@ed41fbb.

@gitgitgadget
Copy link

gitgitgadget bot commented Dec 20, 2021

This patch series was integrated into seen via git@dd867f3.

@gitgitgadget
Copy link

gitgitgadget bot commented Dec 21, 2021

This patch series was integrated into seen via git@2ea70b8.

@aleen42 aleen42 changed the title am: support --empty=(die|drop|keep) option and --allow-empty option to handle empty patches am: support --empty=(stop|drop|keep) option and --allow-empty option to handle empty patches Dec 21, 2021
@gitgitgadget
Copy link

gitgitgadget bot commented Dec 21, 2021

This patch series was integrated into seen via git@d93b5f8.

@gitgitgadget
Copy link

gitgitgadget bot commented Dec 21, 2021

This patch series was integrated into seen via git@3aec6e7.

@gitgitgadget
Copy link

gitgitgadget bot commented Dec 22, 2021

This patch series was integrated into seen via git@079d809.

@gitgitgadget
Copy link

gitgitgadget bot commented Dec 22, 2021

This patch series was integrated into seen via git@6609472.

@gitgitgadget
Copy link

gitgitgadget bot commented Dec 23, 2021

This patch series was integrated into seen via git@5f9d6e7.

@gitgitgadget
Copy link

gitgitgadget bot commented Dec 23, 2021

This patch series was integrated into seen via git@f6a8daf.

@gitgitgadget
Copy link

gitgitgadget bot commented Dec 24, 2021

There was a status update in the "Cooking" section about the branch xw/am-empty on the Git mailing list:

"git am" learns "--empty=3D(stop|drop|keep)" option to tweak what is
done to a piece of e-mail without a patch in it.

Will merge to 'next'.
source: <pull.1076.v19.git.1639034755.gitgitgadget@gmail.com>

@gitgitgadget
Copy link

gitgitgadget bot commented Dec 25, 2021

This patch series was integrated into seen via git@69e7ad7.

@gitgitgadget
Copy link

gitgitgadget bot commented Dec 27, 2021

This patch series was integrated into seen via git@ce004a4.

@gitgitgadget
Copy link

gitgitgadget bot commented Dec 27, 2021

This patch series was integrated into next via git@562e155.

@gitgitgadget gitgitgadget bot added the next label Dec 27, 2021
@gitgitgadget
Copy link

gitgitgadget bot commented Dec 28, 2021

There was a status update in the "Cooking" section about the branch xw/am-empty on the Git mailing list:

"git am" learns "--empty=3D(stop|drop|keep)" option to tweak what is
done to a piece of e-mail without a patch in it.

Will merge to 'master'.
source: <pull.1076.v19.git.1639034755.gitgitgadget@gmail.com>

@gitgitgadget
Copy link

gitgitgadget bot commented Dec 30, 2021

This patch series was integrated into seen via git@c053bd9.

@gitgitgadget
Copy link

gitgitgadget bot commented Jan 4, 2022

This patch series was integrated into seen via git@e6c78ac.

@gitgitgadget
Copy link

gitgitgadget bot commented Jan 4, 2022

There was a status update in the "Cooking" section about the branch xw/am-empty on the Git mailing list:

"git am" learns "--empty=3D(stop|drop|keep)" option to tweak what is
done to a piece of e-mail without a patch in it.

Will merge to 'master'.
source: <pull.1076.v19.git.1639034755.gitgitgadget@gmail.com>

@gitgitgadget
Copy link

gitgitgadget bot commented Jan 5, 2022

This patch series was integrated into seen via git@2080e95.

@gitgitgadget
Copy link

gitgitgadget bot commented Jan 5, 2022

This patch series was integrated into seen via git@ead6767.

@gitgitgadget
Copy link

gitgitgadget bot commented Jan 5, 2022

This patch series was integrated into next via git@ead6767.

@gitgitgadget
Copy link

gitgitgadget bot commented Jan 5, 2022

This patch series was integrated into master via git@ead6767.

@gitgitgadget gitgitgadget bot added the master label Jan 5, 2022
@gitgitgadget gitgitgadget bot closed this Jan 5, 2022
@gitgitgadget
Copy link

gitgitgadget bot commented Jan 5, 2022

Closed via ead6767.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants