jj icon indicating copy to clipboard operation
jj copied to clipboard

gerrit: downloading uploaded change creates divergent change

Open prattmic opened this issue 3 months ago • 8 comments

Description

Using the Gerrit-provided commands to download a Gerrit change that was previously uploaded with jj gerrit upload results in divergent jj changes.

There is not much use in downloading a change that you just uploaded. The reason you would do this is because someone has uploaded another Gerrit change based on yours. You want to download their change, and end up fetching your own since it is the parent.

This behavior surprised me. Because jj gerrit upload creates a temporary commit to add the Change-Id footer, I was expecting this temporary commit to be fetched with a new jj change id, identical to the original except for the Change-Id footer. I don't think that behavior is great either, but it is better than divergent changes!

Steps to Reproduce the Problem

  1. Clone a repo used with Gerrit.
$ jj git clone --colocate https://go.googlesource.com/go
$ cd go
  1. Make a commit and upload to Gerrit
$ echo "hello" >> README.md
$ jj commit
Working copy  (@) now at: mrvnorqz 5bafab15 (empty) (no description set)
Parent commit (@-)      : tszqpspr 2e86bc38 (…) test gerrit download
$ jj gerrit upload --remote origin --remote-branch master -r @-

Found 1 heads to push to Gerrit (remote 'origin'), target branch 'master'

Pushing tszqpspr 2e86bc38 DO NOT SUBMIT: test gerrit download
remote: Resolving deltas: 100% (2/2)
remote: Waiting for private key checker: 1/1 objects left
remote: Processing changes: refs: 1, new: 1, done
remote: 
remote: SUCCESS
remote: 
remote:   https://go-review.googlesource.com/c/go/+/706955 DO NOT SUBMIT: test gerrit download [WIP] [NEW]
  1. Download change from Gerrit. Command provided on https://go-review.googlesource.com/c/go/+/706955 in the upper right "..." -> "Download patch"
$ git fetch https://go.googlesource.com/go refs/changes/55/706955/1 && git checkout -b change-706955 FETCH_HEAD
From https://go.googlesource.com/go
 * branch                  refs/changes/55/706955/1 -> FETCH_HEAD
Previous HEAD position was 2e86bc3874 DO NOT SUBMIT: test gerrit download
Switched to a new branch 'change-706955'

Expected Behavior

Because the uploaded commit has the added Change-Id footer, it doesn't match my local change. Thus I expected an (almost) duplicate change with a different JJ change id but the same contents (other than the Change-Id footer) and parent.

Actual Behavior

JJ displays a divergent change with the same change id:

$ jj log                                                       
Reset the working copy parent to the new Git HEAD.
Done importing changes from the underlying Git repo.
@  ykknyzwl [email protected] 2025-09-25 15:52:22 5fe14507
│  (empty) (no description set)
○  tszqpspr?? [email protected] 2025-09-25 15:47:43 change-706955 git_head() b5c434e1
│  DO NOT SUBMIT: test gerrit download
│ ○  tszqpspr?? [email protected] 2025-09-25 15:47:43 2e86bc38
├─╯  DO NOT SUBMIT: test gerrit download
◆  zqnvnuvv [email protected] 2025-09-25 15:06:35 master 9b7a3280
│  crypto/internal/fips140: remove key import PCTs, make keygen PCTs fatal
~

I'm pretty surprised by this because I don't understand how JJ even knows the downloaded commit has the same change id.

My best guess is that the temporary commit created on upload is actually still present in the repo with a change id association (I admit I don't know how that works :)), but just hidden from most tooling?

Specifications

  • Platform: linux-amd64
  • Version: jj 0.33.0-29991bb6d0450353d898f69e321c226a4140bee0

cc @matts1

prattmic avatar Sep 25 '25 19:09 prattmic

My best guess is that the temporary commit created on upload is actually still present in the repo

I can confirm that it is at least present in the git repo by doing git show commitid with the commit id from Gerrit. This shows the uploaded commit even if I never download it. I'm not sure about the JJ change id association though.

prattmic avatar Sep 25 '25 20:09 prattmic

As you said, this is a rather niche case. IMO we would be better off fixing this by implementing the jj gerrit download / patch command that @martinvonz and I discussed in the original PR.

That being said, it probably won't get implemented anytime soon unless you implement it yourself, since it's pretty low on the priority list.

matts1 avatar Sep 26 '25 03:09 matts1

Thanks for the reference to prior discussion (https://github.com/jj-vcs/jj/pull/7245#discussion_r2349669808, for reference).

I agree that we should have a jj gerrit download command. In fact, my goal yesterday was to continue discussion about a jj gerrit download command that I started in https://github.com/jj-vcs/jj/issues/4387#issuecomment-2865107615 (at the end, point 3), based on my understanding that downloading an uploaded change would create a new almost duplicate jj change. I filed this bug after testing and realizing it did not work the way I expected.

I would be interested working on a jj gerrit download command if I find time, though I'm curious how you think jj gerrit download should fix this up. The commit on Gerrit is already associated with the original JJ change id. Do we force change the association to a new change id? Or perhaps the command notices the association and ignores the Gerrit commit and instead rebases any new children on the visible version of the JJ change?

The latter seems workable, but I still can't help but feel like this is ultimately a bug in jj gerrit upload. i.e., it seems that a commit created in a transaction that is never completed should not have a change id association permanently committed.

prattmic avatar Sep 26 '25 15:09 prattmic

The latter seems workable, but I still can't help but feel like this is ultimately a bug in jj gerrit upload. i.e., it seems that a commit created in a transaction that is never completed should not have a change id association permanently committed.

I do agree that it was not intended. However, I think that it's actually a convenient benefit that it acts in this manner. I really like the fact that it's divergent, because that means it's able to match the change with a local change. I think that we should:

  • If the tree is the same as the existing change for that change-ID, print out a log saying that you're using the local version instead, and rebase on top of the local version
  • Otherwise, they truly are divergent and I think it's correct to mark it as such (you probably uploaded, modified the change locally, then downloaded the old version)

matts1 avatar Sep 28 '25 22:09 matts1

As you said, this is a rather niche case

The issue reproduces even after a simple jj git fetch, not just after downloading a change. Steps: jj gerrit upload - Submit change - jj git fetch - divergent changes. This happens for every submitted change, not just in rare download case.

valodzka avatar Oct 11 '25 09:10 valodzka

I think it's a wrong assumption to assume that the commit uploaded will be the same as the one stored in the remote branch.

I recently submitted this CL on gerrit, for example. It added the following to the bottom of the description:

Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7017050
Reviewed-by: Takuto Ikuta <[email protected]>
Commit-Queue: Takuto Ikuta <[email protected]>
Auto-Submit: Matt Stark <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1527305}

The approach I recommend is to use jj git fetch followed by jj rebase --skip-emptied. rebase --skip-emptied is the general approach used to remove the commits that have already been submitted upstream, regardless of whether you're using gerrit or git.

matts1 avatar Oct 14 '25 07:10 matts1

@matts1 so recommended/convenient workflow is to use something like this sync.py script?

valodzka avatar Oct 19 '25 07:10 valodzka

The sync.py script does solve that problem, but I wrote that for chromium specifically and it does a lot of chromium-specific things. It's way overkill for your use case.

In general, my workflow for syncing any git repository with jj is either:

  • Sync all commits to trunk: git fetch origin main && jj rebase --skip-emptied -s 'roots(mutable())' -d 'trunk()'
  • Sync just the current commit to 'trunk()': git fetch origin main && jj rebase --skip-emptied -s 'roots(mutable()::@)' -d 'trunk()'

jj git sync is what you really want but that isn't yet submitted (#6826).

matts1 avatar Oct 24 '25 05:10 matts1

I tried out the gerrit support in 0.35, and the divergent change surprised me. I got it when I merged my change on Gerrit, and then fetched master from Gerrit.

If something (either JJ or gerrit) adds the ChangeId footer to the commit on upload, could the local change be updated (as if you ran jj desc) amended such that it also has the footer?

Alternatively, maybe fetching the upsteam branch from Gerrit should automatically do something when it detects a JJ change was merged that has a divergent local change (eg. rebasing the children of the local change on the submitted change?)

hanwen avatar Nov 21 '25 09:11 hanwen

If something (either JJ or gerrit) adds the ChangeId footer to the commit on upload, could the local change be updated (as if you ran jj desc) amended such that it also has the footer?

The intent was specifically to avoid doing that. Feel free to look at the original PR for the discussion, but the TLDR is that if you add the change ID to the footer, actions like jj split end up duplicating the change ID.

Alternatively, maybe fetching the upsteam branch from Gerrit should automatically do something when it detects a JJ change was merged that has a divergent local change (eg. rebasing the children of the local change on the submitted change?)

jj rebase --skip-emptied should prevent the divergent changes. As should jj git sync, once it's implemented in the future.

matts1 avatar Nov 23 '25 23:11 matts1