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

cli: add --allow-empty-description flag to push #3653

Merged
merged 1 commit into from
Jun 5, 2024

Conversation

mgattozzi
Copy link
Contributor

This commit adds an optional flag to be able to push commits with an empty description to a remote git repo. While the default behavior is ideal we might need to interact with a repo that has an empty commit description in it. I ran into this issue a few weeks ago pushing commits from an open source repo to an empty repo and had to go back to using git for that push as I would not want to rewrite the history which was many many years long just for that.

This flag allows users an escape hatch for pushing empty descriptions for commits and they're sure that they want that behavior.

This commit adds the flag to the git push command and updates the docs for the command. It also adds a new test to make sure that the flag works as intended alongside the old test which makes sure to reject an empty message for a commit.

Closes #2633

@martinvonz here's the PR I said I would drop by this week to fix my issue. Thanks for pointing things out. It was quite an easy PR to do and the codebase is well organized. I've also signed the CLA since this is the first time I'm contributing to the repo.

Checklist

If applicable:

  • I have updated CHANGELOG.md
  • I have updated the documentation (README.md, docs/, demos/)
  • I have updated the config schema (cli/src/config-schema.json)
  • I have added tests to cover my changes

cli/src/commands/git.rs Outdated Show resolved Hide resolved
cli/tests/cli-reference@.md.snap Outdated Show resolved Hide resolved
cli/tests/test_git_push.rs Outdated Show resolved Hide resolved
cli/src/commands/git.rs Outdated Show resolved Hide resolved
cli/src/commands/git.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@thoughtpolice thoughtpolice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, this also requires some kind of persistent configuration option, because in practice you would basically have to add it 100% of the time you wanted to jj git push if your revision has an empty working copy as an ancestor in the repository; nixpkgs is an example of this (a completely empty commit snuck in a few months ago so now jj git push is FUBAR.) EDIT: this was rather wrong, see my comment #3653 (comment) for the exact scenario I was thinking of.

See db14f33 for an example of how to add such an option; maybe git.allow-empty-push = true or something would be a good name.

@emesterhazy
Copy link
Collaborator

IMO, this also requires some kind of persistent configuration option, because in practice you would basically have to add it 100% of the time you wanted to jj git push if your revision has an empty working copy as an ancestor in the repository; nixpkgs is an example of this (a completely empty commit snuck in a few months ago so now jj git push is FUBAR.)

See db14f33 for an example of how to add such an option; maybe git.allow-empty-push = true or something would be a good name.

Ugh, good point. But if that's the case then we really should have a way to only allow pushing immutable empty commits. Otherwise once the repo is poisoned it's easy to accidentally push more empty commits, right?

@martinvonz
Copy link
Owner

IMO, this also requires some kind of persistent configuration option, because in practice you would basically have to add it 100% of the time you wanted to jj git push if your revision has an empty working copy as an ancestor in the repository;

Is that really true? We only check the commits that are not already on the server (according to our records), right?

@thoughtpolice
Copy link
Collaborator

IMO, this also requires some kind of persistent configuration option, because in practice you would basically have to add it 100% of the time you wanted to jj git push if your revision has an empty working copy as an ancestor in the repository;

Is that really true? We only check the commits that are not already on the server (according to our records), right?

Ah, I think you're right. I managed to recreate the scenario in question. Basically, if you:

  • Have a master@origin that has an empty commit merged in is history
  • Then you have a fork master@gh (your GH fork) that you want to push to,
  • But the fork does not have that empty commit in its history, because it is older/lagging,
  • jj git push fails, because you try to push an empty commit

So, in this case it's because the remote fork was lagging a bit; I hadn't updated it in a few months. Thanks for double checking me. However, it's a bit of a strange edge case. But maybe my earlier statement still applies about adding a config option. Not as hard of a requirement though, since using --ignore-empty-description in a once-off way can solve that problem.

@martinvonz
Copy link
Owner

But maybe my earlier statement still applies about adding a config option.

IMO, let's start with the flag and then add a config option later if we hear that it's somewhat common that people want to always pass the flag (perhaps because they push to their own private repo where they're okay with missing descriptions).

Copy link
Collaborator

@emesterhazy emesterhazy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks pretty good to me but I'll let someone else approve since I'm traveling and don't have access to a computer right now.

@ilyagr ilyagr changed the title cli: add --ignore-empty-description flag to push cli: add --allow-empty-description flag to push May 13, 2024
@ilyagr
Copy link
Collaborator

ilyagr commented May 13, 2024

I renamed the PR since it seems people agreed on --allow-empty-description (which is also my preference over --ignore...)

CHANGELOG.md Outdated Show resolved Hide resolved
@mgattozzi mgattozzi force-pushed the push-qlylxqqkntlu branch 2 times, most recently from 745a572 to a14f39c Compare May 16, 2024 14:44
@mgattozzi
Copy link
Contributor Author

I've updated the PR with the proper description, changes to the flag docs, and dealt with the CHANGELOG conflict as well. I think this should be good to go.

@mgattozzi
Copy link
Contributor Author

I've updated the PR @martinvonz so that it didn't have the conflict with the CHANGELOG again. Hopefully with this we can finally get this merged.

Copy link
Collaborator

@yuja yuja left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, thanks.

This commit adds an optional flag to be able to push commits with an
empty description to a remote git repo. While the default behavior is
ideal we might need to interact with a repo that has an empty commit
description in it. I ran into this issue a few weeks ago pushing commits
from an open source repo to an empty repo and had to go back to using
git for that push as I would not want to rewrite the history which was
many many years long just for that.

This flag allows users an escape hatch for pushing empty descriptions
for commits and they're sure that they want that behavior.

This commit adds the flag to the `git push` command and updates the docs
for the command. It also updates the original test to make sure that the
flag works as intended to reject the commit when not set and to allow
the commit when the flag is set.

Closes martinvonz#2633
@mgattozzi
Copy link
Contributor Author

Updated again with the changelog and everything marked resolved. Just needs someone to merge it.

@emesterhazy
Copy link
Collaborator

LGTM. I'll merge this now then since Yuya also approved.

@emesterhazy emesterhazy merged commit 3bc361a into martinvonz:main Jun 5, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can't push changes to a repo that already has a change with an empty log message
6 participants