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: 4f2503c

[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 <#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]

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 <#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]

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:

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]

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