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

durin42 · 2023-11-25T16:06:40Z

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

Make a change to an existing history that has a buried empty log message
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: 4f2503c

The text was updated successfully, but these errors were encountered:

durin42 · 2023-11-25T16:10:42Z

(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 · 2023-11-25T16:41:37Z

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 · 2023-11-25T17:20:27Z

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 <#2633 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA7OTJYGMYWV6ZW4GCX5NCTYGINUXAVCNFSM6AAAAAA72F4ZWGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMRWGM3TGNRSGI> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

martinvonz · 2023-11-25T17:25:26Z

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

We basically already do. We only look for empty descriptions in main@origin..main (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 · 2023-11-25T18:42:15Z

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

martinvonz · 2023-11-25T19:19:36Z

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 · 2023-11-25T19:40:51Z

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 <#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 <#2633 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAE6LJFLI6OLDHJVXSRHMDYGJAFFAVCNFSM6AAAAAA72F4ZWGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMRWGQYDENRWHE> . You are receiving this because you authored the thread.Message ID: ***@***.***>

mgattozzi · 2024-04-30T16:06:01Z

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 · 2024-04-30T17:12:29Z

Have time to send a PR, @mgattozzi?

mgattozzi · 2024-05-01T22:12:09Z

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

martinvonz · 2024-05-01T22:20:38Z

The error comes from here:

jj/cli/src/commands/git.rs

Line 934 in 19563fe

reasons.push("it has no description");

You would add the flag in the struct here:

jj/cli/src/commands/git.rs

Lines 186 to 233 in 19563fe

    
           /// Push to a Git remote 
        
           /// 
        
           /// By default, pushes any branches pointing to 
        
           /// `remote_branches(remote=<remote>)..@`. Use `--branch` to push specific 
        
           /// branches. Use `--all` to push all branches. Use `--change` to generate 
        
           /// branch names based on the change IDs of specific commits. 
        
           #[derive(clap::Args, Clone, Debug)] 
        
           #[command(group(ArgGroup::new("specific").args(&["branch", "change", "revisions"]).multiple(true)))] 
        
           #[command(group(ArgGroup::new("what").args(&["all", "deleted", "tracked"]).conflicts_with("specific")))] 
        
           pub struct GitPushArgs { 
        
               /// The remote to push to (only named remotes are supported) 
        
               #[arg(long)] 
        
               remote: Option<String>, 
        
               /// Push only this branch, or branches matching a pattern (can be repeated) 
        
               /// 
        
               /// By default, the specified name matches exactly. Use `glob:` prefix to 
        
               /// select branches by wildcard pattern. For details, see 
        
               /// https://martinvonz.github.io/jj/latest/revsets#string-patterns. 
        
               #[arg(long, short, value_parser = StringPattern::parse)] 
        
               branch: Vec<StringPattern>, 
        
               /// Push all branches (including deleted branches) 
        
               #[arg(long)] 
        
               all: bool, 
        
               /// Push all tracked branches (including deleted branches) 
        
               /// 
        
               /// This usually means that the branch was already pushed to or fetched from 
        
               /// the relevant remote. For details, see 
        
               /// https://martinvonz.github.io/jj/latest/branches#remotes-and-tracked-branches 
        
               #[arg(long)] 
        
               tracked: bool, 
        
               /// Push all deleted branches 
        
               /// 
        
               /// Only tracked branches can be successfully deleted on the remote. A 
        
               /// warning will be printed if any untracked branches on the remote 
        
               /// correspond to missing local branches. 
        
               #[arg(long)] 
        
               deleted: bool, 
        
               /// Push branches pointing to these commits (can be repeated) 
        
               #[arg(long, short)] 
        
               revisions: Vec<RevisionArg>, 
        
               /// Push this commit by creating a branch based on its change ID (can be 
        
               /// repeated) 
        
               #[arg(long, short)] 
        
               change: Vec<RevisionArg>, 
        
               /// Only display what will change on the remote 
        
               #[arg(long)] 
        
               dry_run: bool, 
        
           }

And update the test here:

jj/cli/tests/test_git_push.rs

Lines 635 to 645 in 19563fe

    
           #[test] 
        
           fn test_git_push_no_description() { 
        
               let (test_env, workspace_root) = set_up(); 
        
               test_env.jj_cmd_ok(&workspace_root, &["branch", "create", "my-branch"]); 
        
               test_env.jj_cmd_ok(&workspace_root, &["describe", "-m="]); 
        
               let stderr = 
        
                   test_env.jj_cmd_failure(&workspace_root, &["git", "push", "--branch", "my-branch"]); 
        
               insta::assert_snapshot!(stderr, @r###" 
        
               Error: Won't push commit 5b36783cd11c since it has no description 
        
               "###); 
        
           }

Thanks!

mgattozzi · 2024-05-03T15:34:25Z

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

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 martinvonz#2633

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

martinvonz added the good first issue Good for newcomers label Nov 25, 2023

thoughtpolice mentioned this issue Feb 13, 2024

FR: allow pushing commits with an empty author #3029

Open

mgattozzi linked a pull request May 9, 2024 that will close this issue

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

Open

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

durin42 commented Nov 25, 2023

durin42 commented Nov 25, 2023

martinvonz commented Nov 25, 2023

ilyagr commented Nov 25, 2023 via email

martinvonz commented Nov 25, 2023

durin42 commented Nov 25, 2023

martinvonz commented Nov 25, 2023

durin42 commented Nov 25, 2023 via email

mgattozzi commented Apr 30, 2024

martinvonz commented Apr 30, 2024

mgattozzi commented May 1, 2024

martinvonz commented May 1, 2024

mgattozzi commented May 3, 2024

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

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

Comments

durin42 commented Nov 25, 2023

Description

Steps to Reproduce the Problem

Expected Behavior

Actual Behavior

Specifications

durin42 commented Nov 25, 2023

martinvonz commented Nov 25, 2023

ilyagr commented Nov 25, 2023 via email

martinvonz commented Nov 25, 2023

durin42 commented Nov 25, 2023

martinvonz commented Nov 25, 2023

durin42 commented Nov 25, 2023 via email

mgattozzi commented Apr 30, 2024

martinvonz commented Apr 30, 2024

mgattozzi commented May 1, 2024

martinvonz commented May 1, 2024

mgattozzi commented May 3, 2024