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

[mgattozzi]

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

[emesterhazy]

Should this be "Push branches with commits that contain empty descriptions"?

[mgattozzi]

yes it should be!

[martinvonz]

nit: squash this commit into the previous one (see https://martinvonz.github.io/jj/v0.17.1/contributing/#code-reviews)

[mgattozzi]

Ah perfect. I prefer squashing and force pushing, but everyone is a bit different. I've squashed and pushed the changes! Let me know if you need anything else @martinvonz and I can take a look at it when I get the time.

[martinvonz]

It sounds a bit like this would find commits with empty descriptions and push them. Maybe it's clear enough with the --allow part of the flag name, but maybe it's still better to say "Allow pushing commits with empty description" or something like that? I don't think the "branches" in the description really helps either (which is why I didn't include it in my proposal).

[emesterhazy]

Yeah, I agree that adding "allow" is a good idea. I wasn't sure about the terminology, but I think referring to commits instead of branches might be better.

Something like "Allow pushing commits with empty descriptions."

[thoughtpolice]

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]

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]

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]

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]

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).

[emesterhazy]

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]

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

[ilyagr]

Please change the commit title to match the new option name.

[mgattozzi]

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.