[Backport] A specialized Trie for Block Tree Index

[gf2121]

Backport of #14333

[gf2121]

I have to force push to resolve conflicts and keep original commits.

I plan to close this and push directly to branch_10x (no-squash) this Friday if no one beats me :)

[dweiss]

If you have to force-push then you will loose commit history. Force pushing is basically moving the branch's tag to a different commit instead of advancing it over new commits or merge commits.

Force-pushes should really be avoided.

[gf2121]

Thank you for suggestions @dweiss ! I'm not sure i exactly get your point, but i think i did not make myself clear. Let me clarify a bit -

By 'force-push', i was meaning that i did a force push to this branch (gf2121:backport_14333), not planning to force push to branch_10x.

@gf2121 gf2121 force-pushed the backport_14333 branch from a972c58 to d1f5b2d
3 hours ago

1. Some changes made to branch_10x after this PR opened and they caused some conflicts. I wanted to make the conflicts resolved within the cherry-pick commits, like git cherry-pick xxx && {resolve conflicts} && git add . && git cherry-pick -- continue, instead of git merge branch_10x, which will introduce another commit. This is why i did a force push.

2. And by 'push directly to branch_10x', I meant that i wanted to make the split commits to branch_10x instead of a squash and merge.

With these two steps, commits to branch_10x can be same as how they are merged to main.

I wish my poor english explained things clear :)

[dweiss]

When you force-push to a PR branch, some of that PR's commits are (or may be) gone. This effectively rewrites the history of the PR branch and comments made against those commits.

There is nothing wrong with merging the base branch (and fixing conflicts in the merge commit or thereafter) while you're working on a PR. I consider it normal for long-lived PRs. This keeps any previous commits in history and the diff will still only show the changes against the branch you're trying to merge with. So it'll be something like this, typically:

          X---Y---(m)---Z---(m)--- PR
         /       /          /       
    A---B---C---D---E---F--------- main

where (m) is a merge against the target of the PR (main here). Eventually all of that PR's diff is squashed into a single commit (which we prefer over merging back the entire PR history) but the PR shows how the patch evolved.

Hope this makes sense?

[gf2121]

where (m) is a merge against the target of the PR (main here).

Emmm this is actually a backport PR (target at branch_10x), and all my effort was to keep the commits exactly same as how they were merged into main. I knew squash should work as well but there are some commits come from other contributors so i'd want to keep the honor.

[gf2121]

My original purpose of this PR is mainly to notify reviewers these commits will be backport and allow some additional checks for this big diff. When reviewers think they are ready, i'll close this PR and push directly. So commit history is mostly to show how commits will be pushed to branch_10x. So this might not be a 'traditional PR' :)

[dweiss]

We don't understand each other - it's fine to have a PR for a backport (more than fine). And it's fine to cherry pick commits against it. This isn't right though (suspicious):

Even if there were some conflicting commits to branch_10x, you should just merge against branch_10x, consolidate these and then commit instead of force-pushing a new set of commits. I'll take a look at it later to perhaps understand what the force-push was trying to dodge (can't do it now).

[dweiss]

(Note: Please don't take this personally. I just verbalize my opinion on your PR's example but it's nothing against you.)

So, here's the problem that I see with force pushes. Let's say I wanted to take a look at the PR's history (backport or otherwise). If I wanted to see what Adrien reviewed last week, here:

I can no longer see it. The commit a972c58 still exists on github (as a detached commit) -

a972c58

but if I clone (or pull) a fresh content of your repository, it's not referenced from anywhere and won't be present in my local clone. Github may eventually release it too. Force-pushing is essentially moving a branch/tag (or other label) to a different commit; it is a mechanism of "forgetting" about certain commits - those that are no longer referenced by anything can be garbage collected, eventually (this is more complex than branches and tags, but eventually such commits will get removed).

In the context of public discussion, I think this shouldn't be done. The history of a PR and its reviews should be preserved. I personally also don't think you need to credit anybody that much when backporting... Their credit is already mentioned on the original PR and perhaps in the changes.txt file.

Finally, thank you for working on this, of course! ;)

[gf2121]

I personally also don't think you need to credit anybody that much when backporting... Their credit is already mentioned on the original PR and perhaps in the changes.txt file.

Thank you for your patience @dweiss ! This makes sense to me. I was forcing myself to keep a style like '5 commits in main, 5 commits in branch_10x', which seems to be a totally unnecessary nit-pick :)

[dweiss]

Exactly! This would matter more if we actually merged PRs back to dev branches... but we squash them anyway so it can be a bit messy in PR history. Thank you for understanding.