git range-diff compares two commit ranges (two versions of
a branch, e.g. before and after a rebase). Its output is difficult to
comprehend, though. It’s a diff between diffs, presented in one dimension
using two columns of pluses/minuses/spaces. Wouldn’t it be better
if we used two dimensions and some nice colors?
Table of Contents
Motivation/Example
To illustrate my point, let’s take a work-in-progress1
pull request from
xmonad-contrib, git
rebase it to autosquash the fixup commits, and then use git
range-diff to see what the rebase actually did:
$ gh pr checkout 836
$ git checkout -b wx-partial-rebase
$ git rebase -i --keep-base --autosquash origin/master
$ git range-diff wx-partial...wx-partial-rebase
1: 05a5291e ! 1: d5c45f98 X.Prelude: Add infinite stream type
@@ XMonad/Prelude.hs: multimediaKeys = filter ((/= noSymbol) . snd) . map (id &&& s
+ type (Item (Stream a)) = a
+
+ fromList :: [a] -> Stream a
-+ fromList [] = errorWithoutStackTrace "TODO"
+ fromList (x : xs) = x :~ fromList xs
++ fromList [] = errorWithoutStackTrace "XMonad.Prelude.Stream.fromList: Can't create stream out of finite list."
+
+ toList :: Stream a -> [a]
+ toList (x :~ xs) = x : toList xs
2: 9dfe5594 = 2: a11625d9 X.L.Groups: Rewrite gen using infinite streams
3: c8a08103 = 3: 06855306 Import X.Prelude unqualified if necessary
4: 90f16434 = 4: 2de422fe Reduce head usage
5: 00a457fd < -: -------- fixup! X.Prelude: Add infinite stream type
This is a simple example—neither the old (05a5291e) nor new (d5c45f98) commits
remove any lines, so the inner column is always +. Some of you can therefore
figure out that the difference is that the old commit adds a line with “TODO”
in it, while the new adds a line (somewhere else!) with a meaningful error
message instead.
Now let’s look at it side by side (in 2 dimensions):
$ git si-range-diff wx-partial...wx-partial-rebase
(note: si-range-diff is my alias; don’t try this at home—just yet)
────────────────────────────────────────────────────────────────────────────────────────────────────────────
1: 05a5291e ! 1: d5c45f98 X.Prelude: Add infinite stream type
──────────────────────────────────────────────────────────────────────────────────┐
XMonad/Prelude.hs: multimediaKeys = filter ((/= noSymbol) . snd) . map (id &&& s │
──────────────────────────────────────────────────────────────────────────────────┘
+ type (Item (Stream a)) = a │ + type (Item (Stream a)) = a
+ │ +
+ fromList :: [a] -> Stream a │ + fromList :: [a] -> Stream a
+ fromList [] = errorWithoutStackTrace "TODO" │
+ fromList (x : xs) = x :~ fromList xs │ + fromList (x : xs) = x :~ fromList xs
│ + fromList [] = errorWithoutStackTrace "XMon↵
│ ad.Prelude.Stream.fromList: Can't create stream out↵
│ of finite list."
+ │ +
+ toList :: Stream a -> [a] │ + toList :: Stream a -> [a]
+ toList (x :~ xs) = x : toList xs │ + toList (x :~ xs) = x : toList xs
────────────────────────────────────────────────────────────────────────────────────────────────────────────
2: 9dfe5594 = 2: a11625d9 X.L.Groups: Rewrite gen using infinite streams
────────────────────────────────────────────────────────────────────────────────────────────────────────────
3: c8a08103 = 3: 06855306 Import X.Prelude unqualified if necessary
────────────────────────────────────────────────────────────────────────────────────────────────────────────
4: 90f16434 = 4: 2de422fe Reduce head usage
────────────────────────────────────────────────────────────────────────────────────────────────────────────
5: 00a457fd < -: -------- fixup! X.Prelude: Add infinite stream type
Much better. It’s easier to see what’s going on here. Left side is the old diff, right side is the new one. Both are syntax-highlighted using foreground (text) colours. The inter-diff diff is syntax-highlighted using background colours—the removed line on the left side is dark red, the added one on the right side is darkish green.
Here’s a longer example from the Linux kernel, which I believe requires
superhuman abilities to understand (made more difficult by the ## and @@
lines appearing in the context of neighbouring diff hunks):
Expand/Collapse
1: 81f8e8008c9a ! 1: 05fce90455a2 mm: abstract the vma_merge()/split_vma() pattern for mprotect() et al.
@@ Commit message
parameters, create inline wrapper functions for each of the modify
operations, parameterised only by what is required to perform the action.
+ We can also significantly simplify the logic - by returning the VMA if we
+ split (or merged VMA if we do not) we no longer need specific handling for
+ merge/split cases in any of the call sites.
+
Note that the userfaultfd_release() case works even though it does not
split VMAs - since start is set to vma->vm_start and end is set to
vma->vm_end, the split logic does not trigger.
@@ Commit message
- vma->vm_start) >> PAGE_SHIFT, and start - vma->vm_start will be 0 in this
instance, this invocation will remain unchanged.
- Signed-off-by: Lorenzo Stoakes <lstoakes@gmail.com>
+ We eliminate a VM_WARN_ON() in mprotect_fixup() as this simply asserts that
+ vma_merge() correctly ensures that flags remain the same, something that is
+ already checked in is_mergeable_vma() and elsewhere, and in any case is not
+ specific to mprotect().
+
Reviewed-by: Vlastimil Babka <vbabka@suse.cz>
+ Signed-off-by: Lorenzo Stoakes <lstoakes@gmail.com>
## fs/userfaultfd.c ##
@@ fs/userfaultfd.c: static int userfaultfd_release(struct inode *inode, struct file *file)
@@ fs/userfaultfd.c: static int userfaultfd_release(struct inode *inode, struct fil
- vma->vm_file, vma->vm_pgoff,
- vma_policy(vma),
- NULL_VM_UFFD_CTX, anon_vma_name(vma));
-+ prev = vma_modify_flags_uffd(&vmi, prev, vma, vma->vm_start,
-+ vma->vm_end, new_flags,
-+ NULL_VM_UFFD_CTX);
+- if (prev) {
+- vma = prev;
+- } else {
+- prev = vma;
+- }
++ vma = vma_modify_flags_uffd(&vmi, prev, vma, vma->vm_start,
++ vma->vm_end, new_flags,
++ NULL_VM_UFFD_CTX);
+
+ vma_start_write(vma);
+ userfaultfd_set_vm_flags(vma, new_flags);
+ vma->vm_userfaultfd_ctx = NULL_VM_UFFD_CTX;
+
- if (prev) {
- vma = prev;
- } else {
++ prev = vma;
+ }
+ mmap_write_unlock(mm);
+ mmput(mm);
@@ fs/userfaultfd.c: static int userfaultfd_register(struct userfaultfd_ctx *ctx,
unsigned long start, end, vma_end;
struct vma_iterator vmi;
@@ fs/userfaultfd.c: static int userfaultfd_register(struct userfaultfd_ctx *ctx,
- /* vma_merge() invalidated the mas */
- vma = prev;
- goto next;
-+ prev = vma_modify_flags_uffd(&vmi, prev, vma, start, vma_end,
-+ new_flags,
-+ (struct vm_userfaultfd_ctx){ctx});
-+ if (IS_ERR(prev)) {
-+ ret = PTR_ERR(prev);
++
++ vma = vma_modify_flags_uffd(&vmi, prev, vma, start, vma_end,
++ new_flags,
++ (struct vm_userfaultfd_ctx){ctx});
++ if (IS_ERR(vma)) {
++ ret = PTR_ERR(vma);
+ break;
}
- if (vma->vm_start < start) {
@@ fs/userfaultfd.c: static int userfaultfd_register(struct userfaultfd_ctx *ctx,
- break;
- }
- next:
-+
-+ if (prev)
-+ vma = prev; /* vma_merge() invalidated the mas */
+
/*
* In the vma_merge() successful mprotect-like case 8:
@@ fs/userfaultfd.c: static int userfaultfd_unregister(struct userfaultfd_ctx *ctx,
- vma_policy(vma),
- NULL_VM_UFFD_CTX, anon_vma_name(vma));
- if (prev) {
-+ prev = vma_modify_flags_uffd(&vmi, prev, vma, start, vma_end,
-+ new_flags, NULL_VM_UFFD_CTX);
-+ if (IS_ERR(prev)) {
+- vma = prev;
+- goto next;
++ vma = vma_modify_flags_uffd(&vmi, prev, vma, start, vma_end,
++ new_flags, NULL_VM_UFFD_CTX);
++ if (IS_ERR(vma)) {
+ ret = PTR_ERR(prev);
+ break;
-+ }
-+
-+ if (prev)
- vma = prev;
-- goto next;
-- }
+ }
- if (vma->vm_start < start) {
- ret = split_vma(&vmi, vma, start, 1);
- if (ret)
@@ fs/userfaultfd.c: static int userfaultfd_unregister(struct userfaultfd_ctx *ctx,
- break;
- }
- next:
++
/*
* In the vma_merge() successful mprotect-like case 8:
* the next vma was merged into the current one and
@@ mm/madvise.c: static int madvise_update_vma(struct vm_area_struct *vma,
struct mm_struct *mm = vma->vm_mm;
int error;
- pgoff_t pgoff;
-+ struct vm_area_struct *merged;
VMA_ITERATOR(vmi, mm, start);
if (new_flags == vma->vm_flags && anon_vma_name_eq(anon_vma_name(vma), anon_name)) {
@@ mm/madvise.c: static int madvise_update_vma(struct vm_area_struct *vma,
- vma = *prev;
- goto success;
- }
-+ merged = vma_modify_flags_name(&vmi, *prev, vma, start, end, new_flags,
-+ anon_name);
-+ if (IS_ERR(merged))
-+ return PTR_ERR(merged);
++ vma = vma_modify_flags_name(&vmi, *prev, vma, start, end, new_flags,
++ anon_name);
++ if (IS_ERR(vma))
++ return PTR_ERR(vma);
-- *prev = vma;
-+ if (merged)
-+ vma = *prev = merged;
-+ else
-+ *prev = vma;
+ *prev = vma;
- if (start != vma->vm_start) {
- error = split_vma(&vmi, vma, start, 1);
@@ mm/madvise.c: static int madvise_update_vma(struct vm_area_struct *vma,
## mm/mempolicy.c ##
@@ mm/mempolicy.c: static int mbind_range(struct vma_iterator *vmi, struct vm_area_struct *vma,
+ struct vm_area_struct **prev, unsigned long start,
+ unsigned long end, struct mempolicy *new_pol)
{
- struct vm_area_struct *merged;
+- struct vm_area_struct *merged;
unsigned long vmstart, vmend;
- pgoff_t pgoff;
- int err;
@@ mm/mempolicy.c: static int mbind_range(struct vma_iterator *vmi, struct vm_area_
- merged = vma_merge(vmi, vma->vm_mm, *prev, vmstart, vmend, vma->vm_flags,
- vma->anon_vma, vma->vm_file, pgoff, new_pol,
- vma->vm_userfaultfd_ctx, anon_vma_name(vma));
-+ merged = vma_modify_policy(vmi, *prev, vma, vmstart, vmend, new_pol);
-+ if (IS_ERR(merged))
-+ return PTR_ERR(merged);
-+
- if (merged) {
- *prev = merged;
- return vma_replace_policy(merged, new_pol);
- }
-
+- if (merged) {
+- *prev = merged;
+- return vma_replace_policy(merged, new_pol);
+- }
+-
- if (vma->vm_start != vmstart) {
- err = split_vma(vmi, vma, vmstart, 1);
- if (err)
@@ mm/mempolicy.c: static int mbind_range(struct vma_iterator *vmi, struct vm_area_
- if (err)
- return err;
- }
--
++ vma = vma_modify_policy(vmi, *prev, vma, vmstart, vmend, new_pol);
++ if (IS_ERR(vma))
++ return PTR_ERR(vma);
+
*prev = vma;
return vma_replace_policy(vma, new_pol);
- }
## mm/mlock.c ##
@@ mm/mlock.c: static int mlock_fixup(struct vma_iterator *vmi, struct vm_area_struct *vma,
@@ mm/mlock.c: static int mlock_fixup(struct vma_iterator *vmi, struct vm_area_stru
int nr_pages;
int ret = 0;
vm_flags_t oldflags = vma->vm_flags;
-+ struct vm_area_struct *merged;
-
- if (newflags == oldflags || (oldflags & VM_SPECIAL) ||
- is_vm_hugetlb_page(vma) || vma == get_gate_vma(current->mm) ||
@@ mm/mlock.c: static int mlock_fixup(struct vma_iterator *vmi, struct vm_area_struct *vma,
/* don't set VM_LOCKED or VM_LOCKONFAULT and don't count */
goto out;
@@ mm/mlock.c: static int mlock_fixup(struct vma_iterator *vmi, struct vm_area_stru
- if (*prev) {
- vma = *prev;
- goto success;
-+ merged = vma_modify_flags(vmi, *prev, vma, start, end, newflags);
-+ if (IS_ERR(merged)) {
-+ ret = PTR_ERR(merged);
++ vma = vma_modify_flags(vmi, *prev, vma, start, end, newflags);
++ if (IS_ERR(vma)) {
++ ret = PTR_ERR(vma);
+ goto out;
}
@@ mm/mlock.c: static int mlock_fixup(struct vma_iterator *vmi, struct vm_area_stru
- if (ret)
- goto out;
- }
-+ if (merged)
-+ vma = *prev = merged;
-
+-
- if (end != vma->vm_end) {
- ret = split_vma(vmi, vma, end, 0);
- if (ret)
@@ mm/mmap.c: int split_vma(struct vma_iterator *vmi, struct vm_area_struct *vma,
+ *
+ * If no merge is possible and the range does not span the entirety of the VMA,
+ * we then need to split the VMA to accommodate the change.
++ *
++ * The function returns either the merged VMA, the original VMA if a split was
++ * required instead, or an error if the split failed.
+ */
+struct vm_area_struct *vma_modify(struct vma_iterator *vmi,
+ struct vm_area_struct *prev,
@@ mm/mmap.c: int split_vma(struct vma_iterator *vmi, struct vm_area_struct *vma,
+ return ERR_PTR(err);
+ }
+
-+ return NULL;
++ return vma;
+}
+
/*
@@ mm/mprotect.c: mprotect_fixup(struct vma_iterator *vmi, struct mmu_gather *tlb,
unsigned int mm_cp_flags = 0;
unsigned long charged = 0;
- pgoff_t pgoff;
-+ struct vm_area_struct *merged;
int error;
if (newflags == oldflags) {
@@ mm/mprotect.c: mprotect_fixup(struct vma_iterator *vmi, struct mmu_gather *tlb,
- vma->vm_userfaultfd_ctx, anon_vma_name(vma));
- if (*pprev) {
- vma = *pprev;
-+ merged = vma_modify_flags(vmi, *pprev, vma, start, end, newflags);
-+ if (IS_ERR(merged)) {
-+ error = PTR_ERR(merged);
-+ goto fail;
-+ }
-+
-+ if (merged) {
-+ vma = *pprev = merged;
- VM_WARN_ON((vma->vm_flags ^ newflags) & ~VM_SOFTDIRTY);
+- VM_WARN_ON((vma->vm_flags ^ newflags) & ~VM_SOFTDIRTY);
- goto success;
-+ } else {
-+ *pprev = vma;
++ vma = vma_modify_flags(vmi, *pprev, vma, start, end, newflags);
++ if (IS_ERR(vma)) {
++ error = PTR_ERR(vma);
++ goto fail;
}
-- *pprev = vma;
--
+ *pprev = vma;
+
- if (start != vma->vm_start) {
- error = split_vma(vmi, vma, start, 1);
- if (error)
─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
1: 81f8e8008c9a ! 1: 05fce90455a2 mm: abstract the vma_merge()/split_vma() pattern for mprotect() et al.
────────────────┐
Commit message │
────────────────┘
parameters, create inline wrapper functions for each of the modify │ parameters, create inline wrapper functions for each of the modify
operations, parameterised only by what is required to perform the action. │ operations, parameterised only by what is required to perform the action.
│
│ We can also significantly simplify the logic - by returning the VMA if we
│ split (or merged VMA if we do not) we no longer need specific handling for
│ merge/split cases in any of the call sites.
│
Note that the userfaultfd_release() case works even though it does not │ Note that the userfaultfd_release() case works even though it does not
split VMAs - since start is set to vma->vm_start and end is set to │ split VMAs - since start is set to vma->vm_start and end is set to
vma->vm_end, the split logic does not trigger. │ vma->vm_end, the split logic does not trigger.
────────────────┐
Commit message │
────────────────┘
- vma->vm_start) >> PAGE_SHIFT, and start - vma->vm_start will be 0 in this │ - vma->vm_start) >> PAGE_SHIFT, and start - vma->vm_start will be 0 in this
instance, this invocation will remain unchanged. │ instance, this invocation will remain unchanged.
│
Signed-off-by: Lorenzo Stoakes <lstoakes@gmail.com> │
│ We eliminate a VM_WARN_ON() in mprotect_fixup() as this simply asserts that
│ vma_merge() correctly ensures that flags remain the same, something that is
│ already checked in is_mergeable_vma() and elsewhere, and in any case is not
│ specific to mprotect().
│
Reviewed-by: Vlastimil Babka <vbabka@suse.cz> │ Reviewed-by: Vlastimil Babka <vbabka@suse.cz>
│ Signed-off-by: Lorenzo Stoakes <lstoakes@gmail.com>
│
## fs/userfaultfd.c ## │ ## fs/userfaultfd.c ##
@@ fs/userfaultfd.c: static int userfaultfd_release(struct inode *inode, struct f↴ │ @@ fs/userfaultfd.c: static int userfaultfd_release(struct inode *inode, struct f↴
…ile *file) │ …ile *file)
──────────────────────────────────────────────────────────────────────────────────┐
fs/userfaultfd.c: static int userfaultfd_release(struct inode *inode, struct fil │
──────────────────────────────────────────────────────────────────────────────────┘
- vma->vm_file, vma->vm_pgoff, │ - vma->vm_file, vma->vm_pgoff,
- vma_policy(vma), │ - vma_policy(vma),
- NULL_VM_UFFD_CTX, anon_vma_name(vma)); │ - NULL_VM_UFFD_CTX, anon_vma_name(vma));
│ - if (prev) {
│ - vma = prev;
│ - } else {
│ - prev = vma;
│ - }
+ prev = vma_modify_flags_uffd(&vmi, prev, vma, vma->vm_start, │ + vma = vma_modify_flags_uffd(&vmi, prev, vma, vma->vm_start,
+ vma->vm_end, new_flags, │ + vma->vm_end, new_flags,
+ NULL_VM_UFFD_CTX); │ + NULL_VM_UFFD_CTX);
│
│ vma_start_write(vma);
│ userfaultfd_set_vm_flags(vma, new_flags);
│ vma->vm_userfaultfd_ctx = NULL_VM_UFFD_CTX;
+ │ +
if (prev) { │
vma = prev; │
} else { │
│ + prev = vma;
│ }
│ mmap_write_unlock(mm);
│ mmput(mm);
@@ fs/userfaultfd.c: static int userfaultfd_register(struct userfaultfd_ctx *ctx, │ @@ fs/userfaultfd.c: static int userfaultfd_register(struct userfaultfd_ctx *ctx,
unsigned long start, end, vma_end; │ unsigned long start, end, vma_end;
struct vma_iterator vmi; │ struct vma_iterator vmi;
────────────────────────────────────────────────────────────────────────────────┐
fs/userfaultfd.c: static int userfaultfd_register(struct userfaultfd_ctx *ctx, │
────────────────────────────────────────────────────────────────────────────────┘
- /* vma_merge() invalidated the mas */ │ - /* vma_merge() invalidated the mas */
- vma = prev; │ - vma = prev;
- goto next; │ - goto next;
│ +
+ prev = vma_modify_flags_uffd(&vmi, prev, vma, start, vma_end, │ + vma = vma_modify_flags_uffd(&vmi, prev, vma, start, vma_end,
+ new_flags, │ + new_flags,
+ (struct vm_userfaultfd_ctx){ctx}); │ + (struct vm_userfaultfd_ctx){ctx});
+ if (IS_ERR(prev)) { │ + if (IS_ERR(vma)) {
+ ret = PTR_ERR(prev); │ + ret = PTR_ERR(vma);
+ break; │ + break;
} │ }
- if (vma->vm_start < start) { │ - if (vma->vm_start < start) {
────────────────────────────────────────────────────────────────────────────────┐
fs/userfaultfd.c: static int userfaultfd_register(struct userfaultfd_ctx *ctx, │
────────────────────────────────────────────────────────────────────────────────┘
- break; │ - break;
- } │ - }
- next: │ - next:
+ │
+ if (prev) │
+ vma = prev; /* vma_merge() invalidated the mas */ │
+ │ +
/* │ /*
* In the vma_merge() successful mprotect-like case 8: │ * In the vma_merge() successful mprotect-like case 8:
──────────────────────────────────────────────────────────────────────────────────┐
fs/userfaultfd.c: static int userfaultfd_unregister(struct userfaultfd_ctx *ctx, │
──────────────────────────────────────────────────────────────────────────────────┘
- vma_policy(vma), │ - vma_policy(vma),
- NULL_VM_UFFD_CTX, anon_vma_name(vma)); │ - NULL_VM_UFFD_CTX, anon_vma_name(vma));
- if (prev) { │ - if (prev) {
│ - vma = prev;
│ - goto next;
+ prev = vma_modify_flags_uffd(&vmi, prev, vma, start, vma_end, │ + vma = vma_modify_flags_uffd(&vmi, prev, vma, start, vma_end,
+ new_flags, NULL_VM_UFFD_CTX); │ + new_flags, NULL_VM_UFFD_CTX);
+ if (IS_ERR(prev)) { │ + if (IS_ERR(vma)) {
+ ret = PTR_ERR(prev); │ + ret = PTR_ERR(prev);
+ break; │ + break;
+ } │ }
+ │
+ if (prev) │
vma = prev; │
- goto next; │
- } │
- if (vma->vm_start < start) { │ - if (vma->vm_start < start) {
- ret = split_vma(&vmi, vma, start, 1); │ - ret = split_vma(&vmi, vma, start, 1);
- if (ret) │ - if (ret)
──────────────────────────────────────────────────────────────────────────────────┐
fs/userfaultfd.c: static int userfaultfd_unregister(struct userfaultfd_ctx *ctx, │
──────────────────────────────────────────────────────────────────────────────────┘
- break; │ - break;
- } │ - }
- next: │ - next:
│ +
/* │ /*
* In the vma_merge() successful mprotect-like case 8: │ * In the vma_merge() successful mprotect-like case 8:
* the next vma was merged into the current one and │ * the next vma was merged into the current one and
─────────────────────────────────────────────────────────────────────────┐
mm/madvise.c: static int madvise_update_vma(struct vm_area_struct *vma, │
─────────────────────────────────────────────────────────────────────────┘
struct mm_struct *mm = vma->vm_mm; │ struct mm_struct *mm = vma->vm_mm;
int error; │ int error;
- pgoff_t pgoff; │ - pgoff_t pgoff;
+ struct vm_area_struct *merged; │
VMA_ITERATOR(vmi, mm, start); │ VMA_ITERATOR(vmi, mm, start);
│
if (new_flags == vma->vm_flags && anon_vma_name_eq(anon_vma_name(vma), anon_↴ │ if (new_flags == vma->vm_flags && anon_vma_name_eq(anon_vma_name(vma), anon_↴
…name)) { │ …name)) {
─────────────────────────────────────────────────────────────────────────┐
mm/madvise.c: static int madvise_update_vma(struct vm_area_struct *vma, │
─────────────────────────────────────────────────────────────────────────┘
- vma = *prev; │ - vma = *prev;
- goto success; │ - goto success;
- } │ - }
+ merged = vma_modify_flags_name(&vmi, *prev, vma, start, end, new_flags, │ + vma = vma_modify_flags_name(&vmi, *prev, vma, start, end, new_flags,
+ anon_name); │ + anon_name);
+ if (IS_ERR(merged)) │ + if (IS_ERR(vma))
+ return PTR_ERR(merged); │ + return PTR_ERR(vma);
│
- *prev = vma; │ *prev = vma;
+ if (merged) │
+ vma = *prev = merged; │
+ else │
+ *prev = vma; │
│
- if (start != vma->vm_start) { │ - if (start != vma->vm_start) {
- error = split_vma(&vmi, vma, start, 1); │ - error = split_vma(&vmi, vma, start, 1);
─────────────────────────────────────────────────────────────────────────┐
mm/madvise.c: static int madvise_update_vma(struct vm_area_struct *vma, │
─────────────────────────────────────────────────────────────────────────┘
│
## mm/mempolicy.c ## │ ## mm/mempolicy.c ##
@@ mm/mempolicy.c: static int mbind_range(struct vma_iterator *vmi, struct vm_are↴ │ @@ mm/mempolicy.c: static int mbind_range(struct vma_iterator *vmi, struct vm_are↴
…a_struct *vma, │ …a_struct *vma,
│ struct vm_area_struct **prev, unsigned long start,
│ unsigned long end, struct mempolicy *new_pol)
{ │ {
struct vm_area_struct *merged; │ - struct vm_area_struct *merged;
unsigned long vmstart, vmend; │ unsigned long vmstart, vmend;
- pgoff_t pgoff; │ - pgoff_t pgoff;
- int err; │ - int err;
──────────────────────────────────────────────────────────────────────────────────┐
mm/mempolicy.c: static int mbind_range(struct vma_iterator *vmi, struct vm_area_ │
──────────────────────────────────────────────────────────────────────────────────┘
- merged = vma_merge(vmi, vma->vm_mm, *prev, vmstart, vmend, vma->vm_flags, │ - merged = vma_merge(vmi, vma->vm_mm, *prev, vmstart, vmend, vma->vm_flags,
- vma->anon_vma, vma->vm_file, pgoff, new_pol, │ - vma->anon_vma, vma->vm_file, pgoff, new_pol,
- vma->vm_userfaultfd_ctx, anon_vma_name(vma)); │ - vma->vm_userfaultfd_ctx, anon_vma_name(vma));
+ merged = vma_modify_policy(vmi, *prev, vma, vmstart, vmend, new_pol); │
+ if (IS_ERR(merged)) │ - if (merged) {
│ - *prev = merged;
+ return PTR_ERR(merged); │ - return vma_replace_policy(merged, new_pol);
+ │
if (merged) { │
*prev = merged; │
return vma_replace_policy(merged, new_pol); │
} │ - }
│
│ -
- if (vma->vm_start != vmstart) { │ - if (vma->vm_start != vmstart) {
- err = split_vma(vmi, vma, vmstart, 1); │ - err = split_vma(vmi, vma, vmstart, 1);
- if (err) │ - if (err)
──────────────────────────────────────────────────────────────────────────────────┐
mm/mempolicy.c: static int mbind_range(struct vma_iterator *vmi, struct vm_area_ │
──────────────────────────────────────────────────────────────────────────────────┘
- if (err) │ - if (err)
- return err; │ - return err;
- } │ - }
- │
│ + vma = vma_modify_policy(vmi, *prev, vma, vmstart, vmend, new_pol);
│ + if (IS_ERR(vma))
│ + return PTR_ERR(vma);
│
*prev = vma; │ *prev = vma;
return vma_replace_policy(vma, new_pol); │ return vma_replace_policy(vma, new_pol);
} │
│
## mm/mlock.c ## │ ## mm/mlock.c ##
@@ mm/mlock.c: static int mlock_fixup(struct vma_iterator *vmi, struct vm_area_st↴ │ @@ mm/mlock.c: static int mlock_fixup(struct vma_iterator *vmi, struct vm_area_st↴
…ruct *vma, │ …ruct *vma,
──────────────────────────────────────────────────────────────────────────────────┐
mm/mlock.c: static int mlock_fixup(struct vma_iterator *vmi, struct vm_area_stru │
──────────────────────────────────────────────────────────────────────────────────┘
int nr_pages; │ int nr_pages;
int ret = 0; │ int ret = 0;
vm_flags_t oldflags = vma->vm_flags; │ vm_flags_t oldflags = vma->vm_flags;
+ struct vm_area_struct *merged; │
│
if (newflags == oldflags || (oldflags & VM_SPECIAL) || │
is_vm_hugetlb_page(vma) || vma == get_gate_vma(current->mm) || │
@@ mm/mlock.c: static int mlock_fixup(struct vma_iterator *vmi, struct vm_area_st↴ │ @@ mm/mlock.c: static int mlock_fixup(struct vma_iterator *vmi, struct vm_area_st↴
…ruct *vma, │ …ruct *vma,
/* don't set VM_LOCKED or VM_LOCKONFAULT and don't count */ │ /* don't set VM_LOCKED or VM_LOCKONFAULT and don't count */
goto out; │ goto out;
──────────────────────────────────────────────────────────────────────────────────┐
mm/mlock.c: static int mlock_fixup(struct vma_iterator *vmi, struct vm_area_stru │
──────────────────────────────────────────────────────────────────────────────────┘
- if (*prev) { │ - if (*prev) {
- vma = *prev; │ - vma = *prev;
- goto success; │ - goto success;
+ merged = vma_modify_flags(vmi, *prev, vma, start, end, newflags); │ + vma = vma_modify_flags(vmi, *prev, vma, start, end, newflags);
+ if (IS_ERR(merged)) { │ + if (IS_ERR(vma)) {
+ ret = PTR_ERR(merged); │ + ret = PTR_ERR(vma);
+ goto out; │ + goto out;
} │ }
│
──────────────────────────────────────────────────────────────────────────────────┐
mm/mlock.c: static int mlock_fixup(struct vma_iterator *vmi, struct vm_area_stru │
──────────────────────────────────────────────────────────────────────────────────┘
- if (ret) │ - if (ret)
- goto out; │ - goto out;
- } │ - }
+ if (merged) │
+ vma = *prev = merged; │
│
│ -
- if (end != vma->vm_end) { │ - if (end != vma->vm_end) {
- ret = split_vma(vmi, vma, end, 0); │ - ret = split_vma(vmi, vma, end, 0);
- if (ret) │ - if (ret)
────────────────────────────────────────────────────────────────────────────────┐
mm/mmap.c: int split_vma(struct vma_iterator *vmi, struct vm_area_struct *vma, │
────────────────────────────────────────────────────────────────────────────────┘
+ * │ + *
+ * If no merge is possible and the range does not span the entirety of the VMA, │ + * If no merge is possible and the range does not span the entirety of the VMA,
+ * we then need to split the VMA to accommodate the change. │ + * we then need to split the VMA to accommodate the change.
│ + *
│ + * The function returns either the merged VMA, the original VMA if a split was
│ + * required instead, or an error if the split failed.
+ */ │ + */
+struct vm_area_struct *vma_modify(struct vma_iterator *vmi, │ +struct vm_area_struct *vma_modify(struct vma_iterator *vmi,
+ struct vm_area_struct *prev, │ + struct vm_area_struct *prev,
────────────────────────────────────────────────────────────────────────────────┐
mm/mmap.c: int split_vma(struct vma_iterator *vmi, struct vm_area_struct *vma, │
────────────────────────────────────────────────────────────────────────────────┘
+ return ERR_PTR(err); │ + return ERR_PTR(err);
+ } │ + }
+ │ +
+ return NULL; │ + return vma;
+} │ +}
+ │ +
/* │ /*
─────────────────────────────────────────────────────────────────────────────────┐
mm/mprotect.c: mprotect_fixup(struct vma_iterator *vmi, struct mmu_gather *tlb, │
─────────────────────────────────────────────────────────────────────────────────┘
unsigned int mm_cp_flags = 0; │ unsigned int mm_cp_flags = 0;
unsigned long charged = 0; │ unsigned long charged = 0;
- pgoff_t pgoff; │ - pgoff_t pgoff;
+ struct vm_area_struct *merged; │
int error; │ int error;
│
if (newflags == oldflags) { │ if (newflags == oldflags) {
─────────────────────────────────────────────────────────────────────────────────┐
mm/mprotect.c: mprotect_fixup(struct vma_iterator *vmi, struct mmu_gather *tlb, │
─────────────────────────────────────────────────────────────────────────────────┘
- vma->vm_userfaultfd_ctx, anon_vma_name(vma)); │ - vma->vm_userfaultfd_ctx, anon_vma_name(vma));
- if (*pprev) { │ - if (*pprev) {
- vma = *pprev; │ - vma = *pprev;
+ merged = vma_modify_flags(vmi, *pprev, vma, start, end, newflags); │
+ if (IS_ERR(merged)) { │
+ error = PTR_ERR(merged); │
+ goto fail; │
+ } │
+ │
+ if (merged) { │
+ vma = *pprev = merged; │
VM_WARN_ON((vma->vm_flags ^ newflags) & ~VM_SOFTDIRTY); │ - VM_WARN_ON((vma->vm_flags ^ newflags) & ~VM_SOFTDIRTY);
- goto success; │ - goto success;
+ } else { │
+ *pprev = vma; │
│ + vma = vma_modify_flags(vmi, *pprev, vma, start, end, newflags);
│ + if (IS_ERR(vma)) {
│ + error = PTR_ERR(vma);
│ + goto fail;
} │ }
│
- *pprev = vma; │ *pprev = vma;
- │
│
- if (start != vma->vm_start) { │ - if (start != vma->vm_start) {
- error = split_vma(vmi, vma, start, 1); │ - error = split_vma(vmi, vma, start, 1);
- if (error) │ - if (error)
Implementation
Ideally, this side-by-side view of git range-diff would
just appear after I configure pager.range-diff='delta -s' in
.gitconfig. Unfortunately, it’s not that easy:
- delta can’t parse the output of
git range-diff - that output isn’t even meant to be machine-readable
Nevertheless, I hacked together a proof-of-concept preprocessor that takes the
output of git range-diff and turns it into something that
delta parses and presents nicely to a human:
#!/usr/bin/env bash
# preprocessor for "git range-diff" output to be fed into "delta" for side-by-side diff
# see ~/.config/git/config for usage (si-range-diff alias)
#
# depends on ansi2txt from https://github.com/kilobyte/colorized-logs
#
# developed against git 2.40 - git range-diff doesn't have stable output, might need adjustments
# see https://github.com/git/git/blob/ee48e70a829d1fa2da82f14787051ad8e7c45b71/range-diff.c#L376
set -eu
coproc ansi2txt {
stdbuf -oL ansi2txt
}
while IFS= read -r l; do
# decolor line for matching/processing
printf "%s\n" "$l" >&"${ansi2txt[1]}"
IFS= read -r ll <&"${ansi2txt[0]}"
if [[ $ll == " @@ "* ]]; then # hunk header line
# fake hunk header for delta
printf "@@ -0,0 +0,0 @@ %s\n" "${ll# @@ }"
elif [[ $ll == " "* ]]; then # hunk diff line
# un-indent diff
printf "%s\n" "${ll# }"
else # pair header line
# horizontal separator before the line
tput setaf 39; tput setab 235; printf %"$(tput cols)"s | sed 's/ /─/g'; tput sgr0
printf "%s\n" "$l"
if [[ $ll =~ ^[[:digit:]][[:digit:]]*:" "([[:alnum:]][[:alnum:]]*)" ! "[[:digit:]][[:digit:]]*:" "([[:alnum:]][[:alnum:]]*)" " ]]; then
# fake file header for delta to syntax highlight as patch
echo "--- ${BASH_REMATCH[1]}.patch"
echo "+++ ${BASH_REMATCH[2]}.patch"
else
# extra line for unmatched commits
echo
fi
fi
done
To use this, override pager.range-diff when invoking git range-diff, like
so:
$ git -c pager.range-diff='
git-range-diff-delta-preproc \
| delta \
--side-by-side \
--file-style=omit \
--line-numbers-left-format="" \
--line-numbers-right-format="│ " \
--hunk-header-style=syntax \
' range-diff wx-partial...wx-partial-rebase
(Or just steal all the delta-related bits from my git config.)
Yeah, I know, it’s a massive hack.
Next steps
To make this easier to use for the general public, we’d need:
- machine-readable output from
git range-diff(other git commands have that, e.g.git status --porcelain) - implement parsing of that output in delta
Unfortunately, I can’t volunteer to do either at this point2, apologies.