#4952 Missing commits after manual PR rebase
Opened 3 years ago by iucar. Modified 2 years ago

Consider this PR: https://src.fedoraproject.org/rpms/arpack/pull-request/3. I've manually merged changes in the main repo and now the PR is missing this commit: https://src.fedoraproject.org/fork/iucar/rpms/arpack/c/03b9a25c2193725e7dfbcc45e1827eddba6c39e5?branch=master, which is not in the upstream master, but it's not shown. I suppose that merging the PR would still include it, right?, that is just a bug in the PR interface.


I don't quite follow what you mean here.

Looking at the commit list https://src.fedoraproject.org/fork/iucar/rpms/arpack/commits/master I see the commit c4463 which is included in the PR but I don't see 03b9a2 which you link to.
Pagure looks at branches, so the PR seems to include the commit that was present in your fork and not the main repo. It looks to me as if you force-pushed to your fork which removed the 03b9a2 commit from it and thus from the PR. Could it be what happened?

Yeah, I think we cleared that up in the PR discussion, some rebase snafu seems to have happened.

Yes, sorry, I needed this to be merged, so I force-pushed a clean commit, and the trail of the problem was lost.

I created a fake project to reproduce the issue here. There's a pull request there that shows 1 commit (the merge commit), but should show 2 commits instead ("a change in the fork" and the merge commit). And note also that the diff (files changed) is incorrect.

There's a pull request there that shows 1 commit (the merge commit), but should show 2 commits instead

Looking at:
https://pagure.io/someproject/commits/master
and the fork:
https://pagure.io/fork/iucar/someproject/commits/master

There is only one commit that is present in the fork and not the main repo, so it seems good to me that the PR only shows that one commit.

And note also that the diff (files changed) is incorrect.

How so?

There are two commits in the main repo:

  • a change in the main project
  • Added the README

There are four commits in the fork:

  • merge commit
  • a change in the main project
  • a change in the fork
  • Added the README

So one is missing. The diff in the PR shows the changes in the main repo with respect to the fork ("a change in the main project"), and it should show the changes in the fork with respect to the main project instead ("a change in the fork").

Ah indeed, you are right a change in the fork is missing from the PR.

The PR only goes up until the first commit it finds in common between the two git repository, assuming the rest of the history is the same.
For very large git repo (think the linux kernel), checking the entire history of the two repositories to find all the commits present in one and missing in the other are not really applicable. Thus stopping the iteration at the first shared commit.

Not sure how we could do different here.

I don't know how performant is this with something like the Linux kernel, but I get (from the fork):

$ git log upstream/master..origin/master --oneline
6b0a3fd (HEAD -> master, origin/master, origin/HEAD) merge commit
6f51cf7 a change in the fork

Other forges do this fast and nicely, and I suppose they use some variant of the command above. Also:

$ git diff upstream/master..origin/master 
diff --git a/README.md b/README.md
index 4cb9e65..32cae0e 100644
--- a/README.md
+++ b/README.md
@@ -2,4 +2,5 @@

 test

+a change in the fork
 a change in the main project

This is the correct diff, because this is what is going to be changed when the PR is merged, not the diff currently showed in the PR.

I'm sorry, I didn't remember this issue and I removed the repo that reproduced it. I created another one, this time with a proper name and description so that future Iñaki doesn't remove it:

Login to comment on this ticket.

Metadata