Subsequent pushes to the same branch fail: Invalid revision range

[kriswest]

Describe the bug
After successfully reviewing and pushing a commit to branch through git proxy, subsequent commits to the same branch fail:

Enumerating objects: 5, done.
Counting objects: 100% (5/5), done.
Delta compression using up to 4 threads
Compressing objects: 100% (3/3), done.
Writing objects: 100% (3/3), 309 bytes | 309.00 KiB/s, done.
Total 3 (delta 2), reused 0 (delta 0), pack-reused 0 (from 0)
remote:         Error: fatal: Invalid revision range <SHA redacted1>..<SHA redacted2>
remote:
fatal: the remote end hung up unexpectedly
fatal: the remote end hung up unexpectedly
error: failed to push some refs to '<Git Proxy URL redacted>'

• <SHA_redacted1> is the SHA of the commit in the previous push that egressed through the proxy - it's the HEAD of the branch.
• <SHA_redacted2> is the SHA of the commit in the subsequent push that is not yet at the remote repo - hence it would be invalid as its yet to hear of it...

I've yet to track down why this would happen, but it is reproducible with v2.0.0-rc3 and represents a serious issue. I have not yet tried to replicate on main.

To Reproduce
Steps to reproduce the behavior:

1. Create a branch on your repo
2. Add a commit to it and push to git proxy
3. Complete review and approve the commit
4. Push again to egress.
5. Make a further change on the branch and commit it
6. Push to git proxy again - and observe error shown/push rejected due to the error.

[kriswest]

I suspect htis was introduced in the 2.0.0 line as we've definitely pushed to the same branch multiple times in the past!

@jescalada @fabiovincenzi

[jescalada]

@kriswest Will investigate ASAP!

[jescalada]

@kriswest I just confirmed that this bug is happening in the latest main.

Note that pushing directly to main seems to work fine. It happens in subsequent pushes on new branches.

Will try to fix this soon!

[kriswest]

Thanks @jescalada that matches the behaviour I'm seeing. I haven't got a clue why its happening however... That's probably because I don't yet understand how the changes in the push make it into the diff. Looking at it now:

• pullRemote.ts clone the main default branch of the repo
• writePack.ts adds the in pack files from the push
• getDiff.ts runs a diff between base of the push and tip

I think the problem is that if we're we're pushing to a branch we've pushed to previously, then it may contain commits that are not on the default branch. We will not then have them locally if pullRemote always pulls the default branch. It needs to have pulled the branch we are adding to... If so I also wonder how long we've had this flaw or whether it was recently introduced.

The above at least sounds plausible. It may impact @fabiovincenzi's caching work (needs to be able to switch branches? are we sure there won't concurrent usage issues if two different devs were pushing to the same upstream repo at the same time?).

@grovesy, your insight might be useful on this one!

[jescalada]

I've given this a deeper look. I think the problem might be either in the writePack or checkHiddenCommits actions. I ran the example flow on both the latest main and on 1.19.2, and I compared the action object right before running getDiff: https://www.diffchecker.com/92Ih8k9E/

Left side is main, right side is the 1.19.2 tag. Two things stand out:

• The writePack step isn't finding new files:

• checkHiddenCommits is no longer detecting the new commit

Another interesting thing to note (especially for easy testing), is that the flow works fine even if making the first commit/push on main but the subsequent commit and push on 1.19.2. I think this discards the possibility that something related to cloning might be at fault.

Will narrow this down further!

CC: @kriswest, @fabiovincenzi, @grovesy

[jescalada]

So the problem was introduced during the pullRemote optimizations in #1189, narrowing it down to a single line:

await git.clone({
  fs,
  http: gitHttpClient,
  url: action.url,
  dir: `${action.proxyGitPath}/${action.repoName}`,
  onAuth: () => ({ username, password }),
  singleBranch: true, // This line
  depth: 1,
});

I suspect that the real speed optimizations are coming from setting depth: 1, I'll profile this again and make a PR to fix it.

---

I wonder if there's a way to set singleBranch to true and keep the expected getDiff behaviour. Perhaps we could check if pushing to a default branch (main, master, etc.) and then set singleBranch to true just for that case?

---

@kriswest mentioned this exact issue with singleBranch here: #1189 (comment). I apologize for not doing those smoke tests to find the bug earlier...

CC: @coopernetes

[andypols]

@kriswest @jescalada apologies if this is a dumb question, but do you know why we need all of this? Isn’t the diff data already in the packfile in the request? The code seems to be jumping through a lot of hoops for no reason?

[kriswest]

I wonder if there's a way to set singleBranch to true and keep the expected getDiff behaviour. Perhaps we could check if pushing to a default branch (main, master, etc.) and then set singleBranch to true just for that case?

I don't think users will push to the main branch often enough for that to be worthwhile.

I think a workaround would be to switch to the target branch for the push if it exists (as that will get you the missing refs, but that will add another operation/possible slowdown - but perhaps still be quicker than a much larger clone operation as it will only go and get the missing refs for the branch??

@kriswest @jescalada apologies if this is a dumb question, but do you know why we need all of this? Isn’t the diff data already in the packfile in the request? The code seems to be jumping through a lot of hoops for no reason?

It may be possible to display a diff from the pack file data (now that it reliably decodes). However, I'm not sure we get the lines before/after changes and there are a number of different ways (3 I think) that changes can be encoded (pack file != patch file - a patch can be produced from a pack with access to a clone of the repo I think). Hence, I suspect you need a checkout to assemble the diff from the pack data. What the current code does is clone the repo, write the pack data into the clone and then use it to produce a diff. I will be impressed if you can figure out a way to produce it just from the pack data.