upload: Don't crash when a relative PR has no remote commits

[aaron-skydio]

A relative topic's PR can have pr_info but an empty remote_commits list
when its head is already contained in its base, typically after merge.
Guard that case instead of indexing remote_commits[-1].

Topic: empty-relative-rebase
Co-authored-by: Cursor cursoragent@cursor.com

[aaron-skydio]

Reviews in this chain:
└#291 upload: Don't crash when a relative PR has no remote commits

[aaron-skydio]

#	head	base	diff	date	summary
0	40a78330	00cec75f	diff	Sep 2 19:36 PM	1 file changed, 12 insertions(+), 5 deletions(-)
1	1989de3b	00cec75f	diff	Sep 10 13:22 PM	1 file changed, 1 insertion(+), 2 deletions(-)

[aaron-skydio]

i'm not actually sure i buy cursor's reasoning here completely. the stack i was working on didn't have any PRs with 0 commits. this code change does fix the crash i was seeing though.

[jerry-skydio]

is the crash reproducible? the change is reasonable defensively i guess but the explanation is suspect

[aaron-skydio]

it would take some work to reproduce, but it was repeatable when i had the right PRs up on the right branches

cursor now says:

  On your actual question: yes, and my "already merged" explanation was probably not what hit you. The important detail is that pr_info.baseRefOid and headRefOid are not
  GitHub's PR base/head refs. Revup derives them from the PR's commit list:

   revup/github/github.py lines 32-49

                  baseCommit: commits(first: 1) {{
                      nodes {{
                          commit {{
                              parents (first: 1) {{
                                  nodes {{
                                      oid
                                  }}
                              }}
                          }}
                      }}
                  }}
                  headCommit: commits(last: 1) {{
                      nodes {{
                          commit {{
                              oid
                          }}
                      }}
                  }}

  So remote_commits is rev-list --first-parent <last listed commit> ^<parent of first listed commit>, and it comes back empty whenever that last-listed commit is already
  reachable from the parent of the first-listed one. Being merged is only one way to get there. It also happens if GitHub's commits connection returns the list in something
  other than ancestry order (it is ordered by commit date, so a multi-commit PR with out-of-order committer dates can invert and make "last" an ancestor of "first"'s parent),
  or if GitHub recomputed the list against a moved merge base so it only covers part of the branch.

  That also narrows down which PR did it. A single-commit PR can't hit this, since first and last are the same commit and you always get at least one. Of your four topics
  only two have multiple commits, and the branch that was not re-pushed at 19:37 was skymarshal-member-notations while its child and everything above it were — which is
  exactly the cascade the fix produces when the parent's remote_commits is empty. So PR 151414 is the likely culprit. I couldn't reproduce it directly, since the PR data
  reads healthy now, so that's inference rather than proof.

  Given the derived oids are ordering-sensitive, treating "empty" as "can't verify, so re-push" is the right conservative call, which is what the committed fix does. I've
  reverted the temporary instrumentation I added while investigating; your working tree is clean and matches what's in PR 291 (https://github.com/Skydio/revup/pull/291).

but i might lean towards just closing this until i can actually reproduce?

[jerry-skydio]

yeah up to you - there should be no harm in merging this other than possibly obscuring the original issue

[aaron-skydio]

i think i'd just leave open for now and not merge or close - i don't really like the idea of not knowing why this happens

[jerry-skydio]

oooh i just repro'd this. its very subtle

Why cleanup1.remote_commits is empty

  revup/topic_stack.py:804-812 computes it as rev-list --first-parent headRefOid ^baseRefOid. From the -v log:

  $ git rev-list --reverse f997c828 --first-parent --header --not f997c828
  (empty)
  Review origin/master/cleanup1 is rebase False pure False

  baseRefOid == headRefOid == f997c828, so the rev-list is empty by construction.

  Why both oids came out as the same commit

  revup/github/github.py:32-49 doesn't ask GitHub for the head and base oids. It derives them from the PR's
  commit list:

  baseCommit: commits(first: 1) { nodes { commit { parents(first: 1) { nodes { oid }}}}}
  headCommit: commits(last: 1)  { nodes { commit { oid }}}

  i.e. baseRefOid = parent(commits[0]) and headRefOid = commits[-1]. This assumes GitHub returns the PR's
  commits in topological order. It does not.

  For PR 153198 (jerry/revup/master/cleanup1), GitHub returns [a364b6d1, c527fa7e, f997c828] — identically from
  the GraphQL commits connection, the REST /pulls/153198/commits, and the PullRequestCommit timeline items.
  The real topology, verified with local git:

  8648ebb1 (on master) → c527fa7e → f997c828 → a364b6d1 (branch tip)

  So GitHub's order is neither topological nor its reverse. Applying revup's assumption:

  ┌────────────┬─────────────────────────────┬──────────┐
  │            │        revup derived        │  actual  │
  ├────────────┼─────────────────────────────┼──────────┤
  │ baseRefOid │ parent(a364b6d1) = f997c828 │ 8648ebb1 │
  ├────────────┼─────────────────────────────┼──────────┤
  │ headRefOid │ f997c828                    │ a364b6d1 │
  └────────────┴─────────────────────────────┴──────────┘

  Both collapse onto f997c828, a mid-branch commit.

  Why GitHub's order is what it is

  The order is the order commits were associated with the PR, and this PR's commit set grew after the fact:

  1. 06:52:37Z — revup pushed cleanup1 as a single commit a364b6d1 on top of topic B's branch tip f997c828. PR
     153198's base was jerry/revup/master/B, 1 commit. revup's derivation was correct at this point:
     parent(a364b6d1) == f997c828, head == a364b6d1. Its own patchsets comment records exactly that: | 2 |
     a364b6d1 | f997c828 | rebase |.
  2. 07:07Z, 08:02Z — revup force-pushed branch B twice more (f997c828 → 8ffe5a17 → c65c4213), visible as
     BaseRefForcePushedEvents on 153198. cleanup1 was not re-pushed, so it stayed stacked on the now-obsolete
     f997c828.
  3. 09:13:26Z — PR 152113 (jerry/revup/master/B) merged, landing on master as a new commit f20bb453. The old
     branch commits c527fa7e/f997c828 are therefore not ancestors of master (git merge-base --is-ancestor
     c527fa7e origin/master → false).
  4. 09:13:31Z — GitHub fired AutomaticBaseChangeSucceededEvent on 153198, retargeting its base from the merged
     jerry/revup/master/B to master. The merge-base moved back to 8648ebb1, so the PR's commit count went 1
     → 3.
  5. The two newly-exposed ancestors were appended after the already-associated a364b6d1 instead of the list
     being re-sorted. Hence [a364b6d1, c527fa7e, f997c828].

  Step 5 is inference about GitHub's internals — I can't see their storage. Steps 1-4 and the resulting order
  are all directly verified. But the fix doesn't depend on step 5: the order is demonstrably not topological,
  and that's sufficient.