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

[durin42]

Description

I wanted to push a trivial cleanup change to a friend's project, but this commit already deep in the history has no log message, and prevents me from pushing to my fork entirely.

Steps to Reproduce the Problem

1. Make a change to an existing history that has a buried empty log message
2. Try to push

Expected Behavior

The push isn't blocked because the outgoing set doesn't contain any empty log messages.

Actual Behavior

Creating branch push-wsksktrvyzky for revision wsks Error: Won't push commit dcc574ca60a9 since it has no description

Specifications

• Platform: macOS 14.1.1, aarch64
• Version: martinvonz/jj@4f2503cbced88490e1b700e8a47d5a86e5153811

[durin42]

(you can work around this by doing fetch on the remote before push, but this took me thinking like a git implementor to figure out, so probably is a rough edge that should be fixed)

[martinvonz]

Oh, fun.

We could make it depend on whether the commit was created by jj or just imported from git, but we don't keep track of that right now. Actually, we can kind of tell from the change id; if the change id is a bit-reversed commit id, then it's imported. But relying on that would be a bit of a hack.

Another option is to simply print a hint about first running jj git fetch. However, we'd then still prevent pushing to a remote that doesn't have the commit.

So maybe the best option is to add a --allow-empty-description flag (we have talked about having a --allow-conflicts flag too, btw).

[ilyagr]

Adding a flag seems like a good idea. We could also silently allow pushing empty descriptions for immutable commits.

On Sat, Nov 25, 2023, 8:41 AM Martin von Zweigbergk < @.***> wrote:

Oh, fun.

We could make it depend on whether the commit was created by jj or just imported from git, but we don't keep track of that right now. Actually, we can kind of tell from the change id; if the change id is a bit-reversed commit id, then it's imported. But relying on that would be a bit of a hack.

Another option is to simply print a hint about first running jj git fetch. However, we'd then still prevent pushing to a remote that doesn't have the commit.

So maybe the best option is to add a --allow-empty-description flag (we have talked about having a --allow-conflicts flag too, btw).

— Reply to this email directly, view it on GitHub https://github.com/martinvonz/jj/issues/2633#issuecomment-1826373622, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA7OTJYGMYWV6ZW4GCX5NCTYGINUXAVCNFSM6AAAAAA72F4ZWGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMRWGM3TGNRSGI . You are receiving this because you are subscribed to this thread.Message ID: @.***>

[martinvonz]

We could also silently allow pushing empty descriptions for immutable commits.

We basically already do. We only look for empty descriptions in [email protected] (or whatever you're pushing). The problem above happens when you push without ever having fetched from the remote (or at least not having fetched the problematic commit from it).

[durin42]

Suggesting fetch seems like a worthy thing. Is there a reason to not auto-fetch before doing the push?

[martinvonz]

Is there a reason to not auto-fetch before doing the push?

Good question. Fetching becomes slow over time due to all the refs/jj/keep/* refs we create (#293), so maybe that's the main reason not to. Once we've fixed that, we can probably auto-fetch before push.

[durin42]

Maybe auto-fetch only on remotes with no refs?

On Sat, Nov 25, 2023, 14:19 Martin von Zweigbergk @.***> wrote:

Is there a reason to not auto-fetch before doing the push?

Good question. Fetching becomes slow over time due to all the refs/jj/keep/* refs we create (#293 https://github.com/martinvonz/jj/issues/293), so maybe that's the main reason not to. Once we've fixed that, we can probably auto-fetch before push.

— Reply to this email directly, view it on GitHub https://github.com/martinvonz/jj/issues/2633#issuecomment-1826402669, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAE6LJFLI6OLDHJVXSRHMDYGJAFFAVCNFSM6AAAAAA72F4ZWGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMRWGQYDENRWHE . You are receiving this because you authored the thread.Message ID: @.***>

[mgattozzi]

So maybe the best option is to add a --allow-empty-description flag (we have talked about having a --allow-conflicts flag too, btw).

I ran into this today as well and had to fall back to using git so I could do the push. I can't control what others do with their history. It's a great sanity check and having the option to opt out for edge cases like this would be great.

[martinvonz]

Have time to send a PR, @mgattozzi?

[mgattozzi]

Yeah I'd love to @martinvonz. Where would be the best place to make those changes so I can add this flag?

[martinvonz]

The error comes from here: https://github.com/martinvonz/jj/blob/19563fee7483ecd6b5f56200520ac81c84ea16c5/cli/src/commands/git.rs#L934

You would add the flag in the struct here: https://github.com/martinvonz/jj/blob/19563fee7483ecd6b5f56200520ac81c84ea16c5/cli/src/commands/git.rs#L186-L233

And update the test here: https://github.com/martinvonz/jj/blob/19563fee7483ecd6b5f56200520ac81c84ea16c5/cli/tests/test_git_push.rs#L635-L645

Thanks!

[mgattozzi]

Awesome thanks for the pointers. I'll be able to get to it next week 😁