cli: implement `jj gerrit send` for sending changes to Gerrit #2845

thoughtpolice · 2024-01-18T20:41:13Z

Warning

This PR is very rough, and needs significant polish to handle edge cases and other small nits, but functions for basic uses just fine, I think. Please only use it on copies of your existing repositories, or throwaway repositories that you can jj undo on. This warning will be removed once I feel it's more stable.

Please remember: Do not taunt Happy Fun Ball.

This implements the most basic workflow for submitting changes to Gerrit, through a verb called 'send'. Send was chosen to avoid conflicting terminology with terms like "submit" (which merges a change in Gerrit) or "review" which is ambiguous.

Given a list of revsets (specified by multiple -r options), this will parse the footers of every commit, collect them, insert a Change-Id (if one doesn't already exist), and then push them into the given remote.

Because the argument is a revset, you may submit entire trees of changes at once, including multiple trees of independent changes, e.g.

jj gerrit send -r foo:: -r baz::

The remote to push into is, by default, main@origin, and will translate to refs/for/main at that remote. I.e. it is expected that you are using Gerrit as the source of truth, of course. You can specify another remote using the argument --for however, e.g. --for main@gerrit which will change the remote. This syntax is intended to resemble existing syntax for remote references in jj for the sake of UX consistency, rather than blindly exposing the Git refs/ syntax to users, which isn't needed, practically speaking.

There are many other improvements that can be applied on top of this, including a ton of consistency and "does this make sense?" checks. However, it is flexible and a good starting point, and you can in fact both submit and cycle reviews with this interface.

martinvonz

I still haven't reviewed the last commit, but it's getting late, so here's what I have for now.

cli/src/cli_util.rs

lib/src/footer.rs

cli/src/commands/gerrit.rs

lib/src/footer.rs

yuja · 2024-01-20T07:28:52Z

lib/src/footer.rs

+    // to ensure we parse the footer in an unambiguous manner; this avoids cases
+    // where a colon in the body of the message is mistaken for a footer line
+    let mut footer = IndexMap::new();
+    let lines: Vec<&str> = body.trim().lines().rev().collect();


nit: the first line could be skipped by .next() and could do .rev().map().take_while() or something.

cli/src/commands/gerrit.rs

yuja · 2024-01-20T07:33:09Z

cli/src/commands/gerrit.rs

+    /// the form `branch@remote`, where `branch` is the target branch name that
+    /// the change will be submitted to, and `remote` is the remote to push to.
+    /// This remote MUST be a Git push URL for your Gerrit instance.
+    #[arg(long = "for", short = 'f', default_value = "main@origin")]


might be better to use trunk() as the default. We can't determine the remote if the trunk revision had multiple remote branches, though.

So while trunk() probably does make some sense since it tracks what the upstream would be implicitly (without hardcoding pre-existing branch/remote names), I think it's rather confusing because revsets are already used to specify the trees you want to push, and can be specified multiple times, and trunk() looks quite like one at a glance. For example,

jj gerrit mail -r 'open() ~ empty()' -f 'trunk()'

reads quite strangely to me, especially because trunk() would be rather special in this context where it actually specifies a pair of (branch, remote) to send the change to; it's not clear at a glance what that means. I think that the branch@remote syntax is more familiar in this case since it's what shows up in the default log output, it's used for all other @git refs, etc. At the same time, it's also reasonably easy to translate to Gerrit's git push origin HEAD:refs/for/main so it's not too unfamiliar. It's half-way between us and them.

Related, but what I would actually like to do is have this default be over-rideable in the repository configuration, so that people can also do things like set the URL to the Gerrit instance. e.g.

jj config set --repo gerrit.default-remote canon@upstream

or something of this manner to override main@origin as the default --for. We could also have a nested TOML list in the same manner so that remotes can have individually specified default branch targets, e.g. set gerrit.<remote>.default-for "master" where <remote> can be any Git remote.

jj gerrit mail -r 'open() ~ empty()' -f 'trunk()'

Yeah, it's a bit odd. I don't think --for should support a revset in general, but I mean the default could be derived from the trunk(). If the user configured per-repository trunk(), it would be something like trunk() = "foo@origin", so the purpose is quite similar to gerrit.default-remote. Maybe we should have a parsable configuration for that, and both trunk() and the gerrit --for should default to it?

BTW, is this origin in --for foo@origin the gerrit endpoint? The project I worked before had a gerrit-specific SSH remote, and we usually don't pull from it. In that setup, the user-visible remote branch is something like master@origin, but the remote to push to is gerrit.

cli/src/commands/gerrit.rs

yuja · 2024-01-20T07:44:09Z

cli/src/commands/gerrit.rs

+            // mark the old commit as rewritten to itself, but only because it
+            // was a --dry-run, so we can give better error messages later
+            old_to_new.insert(original_commit.clone(), (original_commit.clone(), true));
+            continue;


Maybe we can just rewrite the commits, and discard the transaction? iirc, git push --dry-run -c does a similar thing.

yuja · 2024-01-20T07:49:17Z

cli/src/commands/gerrit.rs

+        } else {
+            write!(ui.stderr(), "Skipped Change-Id (it already exists) for ")?;
+        }
+        tx.write_commit_summary(ui.stderr_formatter().as_mut(), new)?;


Perhaps, this loop can be moved to the previous "rewriting" loop, and we wouldn't have to maintain the mapping between old and new commits.

I think the commit summary can be printed by tx.base_workspace_helper() to suppress scary markers.

cli/src/commands/gerrit.rs

necauqua · 2024-01-20T12:38:19Z

From an outside perspective of a non-gerrit user, the word 'mail' in something seemingly unrelated to email (it is, right?) feels.. really weird

Just my input, I've no real arguments for or against

martinvonz · 2024-01-23T06:26:55Z

From an outside perspective of a non-gerrit user, the word 'mail' in something seemingly unrelated to email (it is, right?) feels.. really weird

What do you think we should call it? @thoughtpolice's first suggestion was review. I think the main problem with that is that it sounds more like "perform review" than "send for review". send-for-review seems too long. Or is it okay given tab completion? Any other options? get-review? (Maybe just review is still the best?)

thoughtpolice · 2024-01-23T13:34:24Z

I think send as Yuya mentioned in his review wasn't bad, either. I admit, I find mail to be very "fun", so I quite like it. (Maybe that's the inner Haskell programmer coming out...) I'd prefer we avoid hyphenated-words but, that's mostly aesthetic, I admit.

I also plan on adding some other commands to fetch changes from Gerrit too (though sending them is the most important since fetching them has some workarounds.) Not sure how that factors in to the vocabulary, but something to consider...

joyously · 2024-01-23T14:59:31Z

I suggest send also, since that's what Breezy uses. See Breezy doc.

Mail or create a merge-directive for submitting changes.

Oh, and Git has send-email and send-pack,

git-send-email - Send a collection of patches as emails
git-send-pack - Push objects over Git protocol to another repository

and Darcs has send.

Prepare a bundle of patches to be applied to some target repository.

benbrittain · 2024-02-03T02:04:41Z

I too am very supportive of send over mail. There might be a better word but I appreciate not overloading the gerrit terminology.

matts1 · 2024-05-14T04:17:01Z

This PR appears to be maintained, constantly rebasing on top of main, but it's been two months since the last comment.

Could we either get this merged, or come up with a list of blockers that we need resolved before it can be merged? I see the tests aren't passing on CI - is that the only blocker?

From what I could tell after looking at the PR, there are some minor improvements that could be made, but as @thoughtpolice said, it works, and I think it'd be nice to submit now and iterate on this later.

martinvonz · 2024-05-14T04:36:14Z

As it happens, there was a thread about this on Discord today. @benbrittain also wanted to this merged. IIUC, the main reason it's not merged is that @thoughtpolice has run into some libgit2 (?) bug once in a while which manifests like this: https://stackoverflow.com/questions/16586642/git-unpack-error-on-push-to-gerrit. It still seems useful even if that happens sometimes.

It looks like there are some unresolved comments too, and maybe we'll want another review of the code since it's been a while. I basically don't remember anything from my previous rounds of review. Do you have time to review it, @matts1?

matts1 · 2024-05-14T04:41:15Z

Sure, I can review - I'll take a look once CI is passing.

mlcui-corp

Random drive-by comments.

lib/src/footer.rs

mlcui-corp · 2024-05-27T04:35:03Z

lib/src/footer.rs

+///
+/// In this case, there are four footer lines: two `Co-authored-by` lines, one
+/// `Reviewed-by` line, and one `Change-Id` line.
+pub fn get_footer_lines(body: &str) -> Vec<FooterEntry> {


Is it worth handling this case from git-interpret-trailers' man page?

The may be split over multiple lines with each subsequent line starting with at least one whitespace, like the "folding" in RFC 822. Example:

key: This is a very long value, with spaces and newlines in it.

Gerrit code uses RevCommit#getFooterLines which seems to handle this case.

Nice catch. Yes, I'll look into fixing this.