Conversation
fallen
left a comment
fallen
left a comment
There was a problem hiding this comment.
Nice PR! Thanks!
Does this need new dependencies that should be added to the pyproject.toml or somewhere else?
Actually, let me put this in its own repository, properly packaged (and with all python linters happy :). I wanted to share this quickly as I referenced it in another PR and it was growing out of control in terms of features, but I'll take the time to clean this up. |
Ok indeed maybe its own repo would be better so that it has its own |
casasnovas
force-pushed
the
quentin-git-review-rebase
branch
2 times, most recently
from
026882d to
2f1b0ec
Compare
Alright I've split into multiple python modules and used pyproject.toml, so it should be ready for review. It passes flake8, ruff check, black and mypy. |
|
Just gave a quick look for now, but I'll review it soon! We have https://github.com/xcp-ng/python-project-template/ to help keep our projects to similar standards. Could you use it here? |
Done! I adapted the workflows so that it should run only for the git-review-rebase project and will let other python scripts integrate as it's out-of-scope for this PR :) |
This script is supposed to help review rebases by presenting the symmetric difference of rebased commits in two ranges. Rebased commits are matched by multiple heuristics (in priority order: commit sha1, patch-ids, title) and it is possible to see a side-by-side diff of matched commits by pressing enter. The list of commits can be filtered by match-type, or fuzzy-filtered based on commit sha1s or titles. The side-by-side diff can be augmented with either full function context, or git blame info by pressing specific keys. Comments on dropped commits can be added, so that the person doing the rebase can justify dropped commits. Demo available at: https://asciinema.org/a/wDwcUTh6aJpvrvyX Patch-ids are cached directly within the git repo, in specific refs, so that patch-ids caches can be shared amongst developers and don't have to be recomputed too often as it is a slow operation. Signed-off-by: Quentin Casasnovas <quentin.casasnovas@vates.tech>
…, uv support). Signed-off-by: Quentin Casasnovas <quentin.casasnovas@vates.tech>
casasnovas
force-pushed
the
quentin-git-review-rebase
branch
from
12ad51b to
da3131f
Compare
Signed-off-by: Quentin Casasnovas <quentin.casasnovas@vates.tech>
The original code had a workaround to avoid highlighting all the spaces from the last character on the left side, but it was a bit too eager depending on the lexer used, for example with the Make lexer we wouldn't get two tokens, one for spaces and one for the newline. Instead, stop on the previous but last token only if it's only spaces/newlines. This doesn't change the highlighting for C source code but allows the highlighting on the left side of a Make diff. Signed-off-by: Quentin Casasnovas <quentin.casasnovas@vates.tech>
casasnovas
force-pushed
the
quentin-git-review-rebase
branch
from
da3131f to
e712fa0
Compare
This makes it more visually appealing with `git blame` information as we won't see the ellipsis at the end of each line, only those actually truncated due to missing space. Signed-off-by: Quentin Casasnovas <quentin.casasnovas@vates.tech>
|
To test the script: # Install the script
cd xcp/scripts/git-review-rebase
pip install -e ./
# Clone a linux repo and use the script on it
git clone git@github.com/xcp-ng/linux.git
cd linux
# This will save you 5min on the first run of the script to calculate the patch-ids
git fetch origin refs/patchids/\*:refs/patchids/\* || cat patch-ids-packed-refs.txt >> .git/packed-refs
git-review-rebase origin/kernel/xcpng-4.19.19-8.0.44.1/pre-base..origin/kernel/xcpng-4.19.19-8.0.44.1/base origin/kernel/xcpng-4.19.325-8.0.44.1/pre-base..origin/kernel/xcpng-4.19.325-8.0.44.1/base |
|
Folks, Given this is a completely new script and it does passes all of the linters and test usage, is it OK to move forward with the merging? I am happy to add features/change things as separate PRs anyway. Cheers, |
You mean, without a review by a human? |
I just mean I don't think it needs a deep/extended review, but more of a quick pass on the code and test usage given this is a completely new script, completely isolated from the rest of the repo and self-contained. |
Only two new patches added, albeit in the middle of the patch-queue so all the patch indexes have changed. Branches imported onto: - https://github.com/xcp-ng/qemu-dp/tree/qemu/xenserver-4.2.1-5.2.17/base - https://github.com/xcp-ng/qemu-dp/tree/qemu/xcpng-4.2.1-5.2.17.1/base You can review the patch-queue changes more easily using the git-review-rebase (PR in review xcp-ng/xcp#782) using: git-review-rebase.py qemu/xenserver-4.2.1-5.2.15/pre-base..qemu/xenserver-4.2.1-5.2.15/base qemu/xenserver-4.2.1-5.2.17/pre-base..qemu/xenserver-4.2.1-5.2.17/base For diffing the spec changes more easily: git diff --word-diff-regex='[^[:space:]]|Patch[0-9]+:' origin/master..QC-XCPNG-2834 -- SPECS The added commits are not merged upstream yet but can be found under review: - https://patchwork.ozlabs.org/project/qemu-devel/patch/20260108132514.1862552-1-ross.lagerwall@citrix.com/ - https://patchwork.ozlabs.org/project/qemu-devel/patch/20260107135824.1681685-1-ross.lagerwall@citrix.com/ Also included is a fix for an earlier bogus date in the changelog entries which I caught by chance.
|
Following discussions with Gaetan and the hypervisor team, who talked with the platform team, I am moving this script to its own repository - I mistakenly assumed this repository was hosting all dev tools and I think this is causing confusion. |
|
I don't object to a separate repository, but I don't understand why it we deemed required to move it outside a repository which historically hosted developer tools. I would have rather waited for the planned discussion on Monday, whose goal was specifically to discuss how to move on, before taking action. |
I can always re-open this PR if the outcome of the meeting is that it should live here, but I personally felt that the tool deserved its own repository because:
My original thought was to have a dedicated repository for this but I thought it would be more trouble to ask for rights to create it and own it (i.e. I was being lazy and assuming extra process :). I was happily surprised to see it's not the case and I could simply click a button to create one, so I just did :-) Again, if the outcome is that it should live here, I can simply remove the new repository. Now, on top of all of these reasons, I am feeling a bit of friction where I wasn't expecting any for a new script that doesn't have any effect on anything/anyone (we're more than three weeks into the PR, and we have a meeting to discuss about it, that does a feel a bit much). I genuinely find it useful, and think others might as well (although nobody has to use it), so rendering it accessible ASAP sounds to me important - I see it as an enabler for the coming |
|
I was just questioning the idea of moving to a separate repo to avoid confusion without waiting for today's meeting, when I don't think there should be confusion related to where the script lives, which is a detail. |
4 participants
This script is supposed to help review rebases by presenting the symmetric difference of rebased commits in two ranges. Rebased commits are matched by multiple heuristics (in priority order: commit sha1, patch-ids, title) and it is possible to see a side-by-side diff of matched commits by pressing enter. The list of commits can be filtered by match-type, or fuzzy-filtered based on commit sha1s or titles.
The side-by-side diff can be augmented with either full function context, or git blame info by pressing specific keys.
Comments on dropped commits can be added, so that the person doing the rebase can justify dropped commits.
Demo available at:
https://asciinema.org/a/wDwcUTh6aJpvrvyX
Patch-ids are cached directly within the git repo, in specific refs, so that patch-ids caches can be shared amongst devellopes and don't have to be recomputed too often as it is a slow operation.