range-diff: add --matched-only to skip one-sided commits

Documentation/git-range-diff.adoc +2 -1

index 5cc5e2ed56..58e59e8e3b 100644
@@ -10,7 +10,8 @@ SYNOPSIS
 [synopsis]
 git range-diff [--color=[<when>]] [--no-color] [<diff-options>]
 	[--no-dual-color] [--creation-factor=<factor>]
-	[--left-only | --right-only] [--diff-merges=<format>]
+	[--left-only | --right-only | --matched-only]
+	[--diff-merges=<format>]
 	[--remerge-diff] [--no-notes | --notes[=<ref>]]
 	( <range1> <range2> | <rev1>...<rev2> | <base> <rev1> <rev2> )
 	[[--] <path>...]

I wonder if the implementation actually can be more like

- give "--hide-left-only" and "--hide-right-only" as synonyms to "--right-only" and "--left-only", and deprecate the original;

- allow them to be given together, which will give the new behaviour you are introducing, i.e., skip steps without both sides from the output;

- give a short-hand synonym, "--matched-only", to truly behave the same as giving "--hide-{left,right}-only" together.

Quoted 11 lines

I wonder if the implementation actually can be more like

- give "--hide-left-only" and "--hide-right-only" as synonyms to "--right-only" and "--left-only", and deprecate the original;

- allow them to be given together, which will give the new behaviour you are introducing, i.e., skip steps without both sides from the output;

- give a short-hand synonym, "--matched-only", to truly behave the same as giving "--hide-{left,right}-only" together.

Seems like a big change, and deprecated options are a pain in the neck because we can never actually remove them.

If we decide to go this way, we might name them "--hide-{left,right}" and just not introduce a condition that makes them incompatible. Then "--matched-only" would be pure syntactic sugar and wouldn't even be 100% necessary to have to achieve this.

Quoted 7 lines

Seems like a big change, and deprecated options are a pain in the neck because we can never actually remove them.

If we decide to go this way, we might name them "--hide-{left,right}" and just not introduce a condition that makes them incompatible. Then "--matched-only" would be pure syntactic sugar and wouldn't even be 100% necessary to have to achieve this.

Or we can just keep the code and fix the documentation. I think that would be much less impact.

Quoted 8 lines

Quoted 7 lines

Seems like a big change, and deprecated options are a pain in the neck because we can never actually remove them.

If we decide to go this way, we might name them "--hide-{left,right}" and just not introduce a condition that makes them incompatible. Then "--matched-only" would be pure syntactic sugar and wouldn't even be 100% necessary to have to achieve this.

Quoted 2 lines

Or we can just keep the code and fix the documentation. I think that would be much less impact.

I agree.

Harald

From: Harald Nordgren <[email protected]>

Reviewing a range-diff often means scrolling past commits that were simply added or dropped, when only the ones that correspond between the two ranges are of interest.

--left-only and --right-only already each suppress one of those one-sided groups, but they are defined as "only show this side" and so cannot be given together, which is exactly why show_range_diff() already rejected that combination. Give the "show only the commits that correspond on both sides" behavior its own name, --matched-only, instead of asking users to reach for a combination that errors out.

Extend the existing '--left-only'/'--right-only' conflict check to also reject any combination with --matched-only, since all three narrow the output in ways that cannot be combined.

Signed-off-by Harald Nordgren <[email protected]>

---
range-diff: add --matched-only to skip one-sided commits

Add git range-diff --matched-only to only show commits that correspond between the two ranges, skipping ones that were only added or only removed.

Changes in v2:

* Update docs and commit message.
* Use die_for_incompatible_opt3.

Quoted 21 lines · Harald Nordgren via GitGitGadget

From: Harald Nordgren <[email protected]>

Reviewing a range-diff often means scrolling past commits that were simply added or dropped, when only the ones that correspond between the two ranges are of interest.

--left-only and --right-only already each suppress one of those one-sided groups, but they are defined as "only show this side" and so cannot be given together, which is exactly why show_range_diff() already rejected that combination. Give the "show only the commits that correspond on both sides" behavior its own name, --matched-only, instead of asking users to reach for a combination that errors out.

Extend the existing '--left-only'/'--right-only' conflict check to also reject any combination with --matched-only, since all three narrow the output in ways that cannot be combined.

Signed-off-by Harald Nordgren <[email protected]>

---
range-diff: add --matched-only to skip one-sided commits

Quoted 8 lines

Add git range-diff --matched-only to only show commits that correspond between the two ranges, skipping ones that were only added or only removed.

Changes in v2:

* Update docs and commit message.
* Use die_for_incompatible_opt3.

This is more than 6 weeks old and looking at the archive, nobody (including me) was really interested in the topic. Anybody wants to take a fresh look at the topic?

Unless we see some volunteers to give it a review in a few weeks, I will discard the topic out of my tree, as 'seen' has been crowded with topics that have been collecting dust without getting many reviews.

Quoted 33 lines · Junio C Hamano

Quoted 21 lines · Harald Nordgren via GitGitGadget

From: Harald Nordgren <[email protected]>

Reviewing a range-diff often means scrolling past commits that were simply added or dropped, when only the ones that correspond between the two ranges are of interest.

--left-only and --right-only already each suppress one of those one-sided groups, but they are defined as "only show this side" and so cannot be given together, which is exactly why show_range_diff() already rejected that combination. Give the "show only the commits that correspond on both sides" behavior its own name, --matched-only, instead of asking users to reach for a combination that errors out.

Extend the existing '--left-only'/'--right-only' conflict check to also reject any combination with --matched-only, since all three narrow the output in ways that cannot be combined.

Signed-off-by Harald Nordgren <[email protected]>

---
range-diff: add --matched-only to skip one-sided commits

Quoted 8 lines

Add git range-diff --matched-only to only show commits that correspond between the two ranges, skipping ones that were only added or only removed.

Changes in v2:

* Update docs and commit message.
* Use die_for_incompatible_opt3.

Quoted 8 lines

This is more than 6 weeks old and looking at the archive, nobody (including me) was really interested in the topic. Anybody wants to take a fresh look at the topic?

Unless we see some volunteers to give it a review in a few weeks, I will discard the topic out of my tree, as 'seen' has been crowded with topics that have been collecting dust without getting many reviews.

That's a bit sad. I wonder if no one is commenting on it because the code is too trivial, meaning nothing that stands out to comment on. All it does is allow left_only and right_only to run at the same time, and the majority of the new code is tests.

Harald

Quoted 47 lines · Harald Nordgren

Quoted 33 lines · Junio C Hamano

Quoted 21 lines · Harald Nordgren via GitGitGadget

From: Harald Nordgren <[email protected]>

Reviewing a range-diff often means scrolling past commits that were simply added or dropped, when only the ones that correspond between the two ranges are of interest.

--left-only and --right-only already each suppress one of those one-sided groups, but they are defined as "only show this side" and so cannot be given together, which is exactly why show_range_diff() already rejected that combination. Give the "show only the commits that correspond on both sides" behavior its own name, --matched-only, instead of asking users to reach for a combination that errors out.

Extend the existing '--left-only'/'--right-only' conflict check to also reject any combination with --matched-only, since all three narrow the output in ways that cannot be combined.

Signed-off-by Harald Nordgren <[email protected]>

---
range-diff: add --matched-only to skip one-sided commits

Quoted 8 lines

Add git range-diff --matched-only to only show commits that correspond between the two ranges, skipping ones that were only added or only removed.

Changes in v2:

* Update docs and commit message.
* Use die_for_incompatible_opt3.

Quoted 8 lines

This is more than 6 weeks old and looking at the archive, nobody (including me) was really interested in the topic. Anybody wants to take a fresh look at the topic?

Unless we see some volunteers to give it a review in a few weeks, I will discard the topic out of my tree, as 'seen' has been crowded with topics that have been collecting dust without getting many reviews.

Quoted 7 lines

That's a bit sad. I wonder if no one is commenting on it because the code is too trivial, meaning nothing that stands out to comment on. All it does is allow left_only and right_only to run at the same time, and the majority of the new code is tests.

Harald

I just skimmed the v3 message, and I’m missing the motivation. When looking at a range-diff, why would I want to hide the added/deleted commits?

I suppose when rebasing, I might want to not show the dropped fixup commits or something in a range-diff of @{1}…, but I’m having a hard time coming up with another use case.

Doesn’t mean there aren’t any! [But the patch might get more and better reviews if it is well motivated?] Anyway, I’d be curious to learn what motivated Harald, as it might be useful to my own workflows, in which case I might be able to review more.

Quoted 59 lines · Ben Knoble

Quoted 47 lines · Harald Nordgren

Quoted 33 lines · Junio C Hamano

Quoted 21 lines · Harald Nordgren via GitGitGadget

From: Harald Nordgren <[email protected]>

Reviewing a range-diff often means scrolling past commits that were simply added or dropped, when only the ones that correspond between the two ranges are of interest.

--left-only and --right-only already each suppress one of those one-sided groups, but they are defined as "only show this side" and so cannot be given together, which is exactly why show_range_diff() already rejected that combination. Give the "show only the commits that correspond on both sides" behavior its own name, --matched-only, instead of asking users to reach for a combination that errors out.

Extend the existing '--left-only'/'--right-only' conflict check to also reject any combination with --matched-only, since all three narrow the output in ways that cannot be combined.

Signed-off-by Harald Nordgren <[email protected]>

---
range-diff: add --matched-only to skip one-sided commits

Quoted 8 lines

Add git range-diff --matched-only to only show commits that correspond between the two ranges, skipping ones that were only added or only removed.

Changes in v2:

* Update docs and commit message.
* Use die_for_incompatible_opt3.

Quoted 8 lines

This is more than 6 weeks old and looking at the archive, nobody (including me) was really interested in the topic. Anybody wants to take a fresh look at the topic?

Unless we see some volunteers to give it a review in a few weeks, I will discard the topic out of my tree, as 'seen' has been crowded with topics that have been collecting dust without getting many reviews.

Quoted 7 lines

That's a bit sad. I wonder if no one is commenting on it because the code is too trivial, meaning nothing that stands out to comment on. All it does is allow left_only and right_only to run at the same time, and the majority of the new code is tests.

Harald

I just skimmed the v3 message, and I’m missing the motivation. When looking at a range-diff, why would I want to hide the added/deleted commits?

Quoted 8 lines

I suppose when rebasing, I might want to not show the dropped fixup commits or something in a range-diff of @{1}…, but I’m having a hard time coming up with another use case.

Doesn’t mean there aren’t any! [But the patch might get more and better reviews if it is well motivated?] Anyway, I’d be curious to learn what motivated Harald, as it might be useful to my own workflows, in which case I might be able to review more.

For me, it's simple:

If we take this branch, and I do some other changes, and then rebase against the latest master. Then when I run

git range-diff
pr-git-2401/HaraldNordgren/range-diff-matched-only-v3...range-diff-matched-only

I will have to scroll through 311 unrelated commits before I can see what I actually changed. I want those gone.

```
  -:  ---------- > 282:  b2608c4076 t2104: avoid suppressing git's exit code
  -:  ---------- > 283:  5e106cb22b pack-write: make index-pack output
comment hash agnostic
  -:  ---------- > 284:  21bb64feef pack-write: lift and expose
index-pack report parsing
  -:  ---------- > 285:  dfb7f49776 object-file: explicitly write
".keep" lockfile
  -:  ---------- > 286:  b07099fff9 object-file: write ".keep"
lockfiles during transaction commit
  -:  ---------- > 287:  91ef176b8f branch: strip refs/heads/ from the
--set-upstream-to hint
  -:  ---------- > 288:  8f3d6ad89f checkout: use Q_() for messages with counts
  -:  ---------- > 289:  7e34d8174d The 8th batch
  -:  ---------- > 290:  4ec5266711 branch: return an error from
setup_tracking()
  -:  ---------- > 291:  ad74e02417 checkout: finish switching when
the tracking config cannot be written
  -:  ---------- > 292:  d84678b8cf Documentation: describe
connectivity checking
  -:  ---------- > 293:  97998dcfd1 p5413: add perf tests for connectivity check
  -:  ---------- > 294:  d6d2d8557c connected: drop dependency on
`the_repository`
  -:  ---------- > 295:  9e0207b50b connected: add
transfer.connectivityCheck config
  -:  ---------- > 296:  43157175b5 connected: add incremental
connectivity check via rev-list
  -:  ---------- > 297:  656be5948a connected: add "auto" connectivity
check mode
  -:  ---------- > 298:  9dba0b87e5 rerere: stop using
`the_repository` in `rerere_path()`
  -:  ---------- > 299:  f62947adf0 rerere: stop using
`the_repository` in internal helpers
  -:  ---------- > 300:  a8b75e8eda rerere: remove redundant calls to
`repo_config()`
  -:  ---------- > 301:  87ab289605 rerere: cache result of
`is_rerere_enabled()`
  -:  ---------- > 302:  4f67f74fc8 rerere: stop using `the_repository`
  -:  ---------- > 303:  ea54cda7c2 rerere: bundle global state into a struct
  -:  ---------- > 304:  b948c10c8b rerere: get rid of global rerere cache state
  -:  ---------- > 305:  c3764d15e6 hook: make hook.<event>.enabled
cover the hookdir hook
  -:  ---------- > 306:  3a7784d0d5 hook: accept .enabled and
.parallel given without a value
  -:  ---------- > 307:  37b3475fb5 The 9th batch
  -:  ---------- > 308:  8d1b215c01 The 10th batch
  -:  ---------- > 309:  b9e5cff180 The 11th batch
  -:  ---------- > 310:  50fe5cc63f The 12th batch
  -:  ---------- > 311:  ebdc087dc9 The 13th batch
  1:  ee195c14fc = 312:  f01d7e30ef range-diff: add --matched-only to
skip one-sided commits
```

Harald

Quoted 8 lines

This is more than 6 weeks old and looking at the archive, nobody (including me) was really interested in the topic. Anybody wants to take a fresh look at the topic?

Unless we see some volunteers to give it a review in a few weeks, I will discard the topic out of my tree, as 'seen' has been crowded with topics that have been collecting dust without getting many reviews.

That's a bit sad. I wonder if no one is commenting on it because the code is too trivial, meaning nothing that stands out to comment on.

All it does is allow left_only and right_only to run at the same time, and the majority of the new code is tests.

Quoted 17 lines · Ben Knoble

Quoted 4 lines

Anyway, I’d be curious
to learn what motivated Harald, as it might be useful to my own workflows, in which case I might be able to review more.

For me, it's simple:

If we take this branch, and I do some other changes, and then rebase against the latest master. Then when I run

git range-diff
pr-git-2401/HaraldNordgren/range-diff-matched-only-v3...range-diff-matched-only

I will have to scroll through 311 unrelated commits before I can see what I actually changed. I want those gone.

Quoted 10 lines

```
  -:  ---------- > 282:  b2608c4076 t2104: avoid suppressing git's exit code
  -:  ---------- > 283:  5e106cb22b pack-write: make index-pack output
comment hash agnostic
  [...]
  -:  ---------- > 311:  ebdc087dc9 The 13th batch
  1:  ee195c14fc = 312:  f01d7e30ef range-diff: add --matched-only to
skip one-sided commits
```

Well, in this particular case --left-only solves it.

So maybe that's enough.

Wouldn't invoking as

git range-diff origin/master
pr-git-2401/HaraldNordgren/range-diff-matched-only-v3 range-diff-matched-only

address this case?

Quoted 7 lines · "Matt Hunter

Wouldn't invoking as

git range-diff origin/master
pr-git-2401/HaraldNordgren/range-diff-matched-only-v3 range-diff-matched-only

address this case?

Three-rev form where <base> and two evolutions <rev1> and <rev2> are given, and <base>..<rev1> and <base>..<rev2> are compared?

I thought Harald's case was to take the patches from the previous round of the topic and rebase them on top of a different base (e.g., updated origin/master), so there is no suitable single <base> for the three-rev form, but I may have misunderstood his example.

Quoted 6 lines · Junio C Hamano

I thought Harald's case was to take the patches from the previous round of the topic and rebase them on top of a different base (e.g., updated origin/master), so there is no suitable single <base> for the three-rev form, but I may have misunderstood his example.

I think origin..old-topic and origin..new-topic would both select only the “topic” commits as long as both bases are reachable from origin (which seems to be the described case, to me).

I agree with Matt that better selection of inputs to range-diff makes this specific case better :)

So, I think this <A>...<B> form would yield the expected output if you only rewrote history in-place _on the same base commit_, but not in the general case?

Quoted 12 lines · Junio C Hamano

Quoted 7 lines · "Matt Hunter

Wouldn't invoking as

git range-diff origin/master
pr-git-2401/HaraldNordgren/range-diff-matched-only-v3 range-diff-matched-only

address this case?

Quoted 7 lines

Three-rev form where <base> and two evolutions <rev1> and <rev2> are given, and <base>..<rev1> and <base>..<rev2> are compared?

I thought Harald's case was to take the patches from the previous round of the topic and rebase them on top of a different base (e.g., updated origin/master), so there is no suitable single <base> for the three-rev form, but I may have misunderstood his example.

I think origin..old-topic and origin..new-topic would both select only the “topic” commits as long as both bases are reachable from origin (which seems to be the described case, to me).

I agree with Matt that better selection of inputs to range-diff makes this specific case better :)

Quoted 20 lines · Ben Knoble

Quoted 12 lines

Quoted 1 line · Junio C Hamano

Quoted 7 lines · "Matt Hunter

Wouldn't invoking as

git range-diff origin/master
pr-git-2401/HaraldNordgren/range-diff-matched-only-v3 range-diff-matched-only

address this case?

Quoted 7 lines

Three-rev form where <base> and two evolutions <rev1> and <rev2> are given, and <base>..<rev1> and <base>..<rev2> are compared?

I thought Harald's case was to take the patches from the previous round of the topic and rebase them on top of a different base (e.g., updated origin/master), so there is no suitable single <base> for the three-rev form, but I may have misunderstood his example.

Quoted 7 lines

I think origin..old-topic and origin..new-topic would both select only the “topic” commits as long as both bases are reachable from origin (which seems to be the described case, to me).

I agree with Matt that better selection of inputs to range-diff makes this specific case better :)

I use the

base..old base..new

Form as well.[1] Then it indeed does not matter if you have rebased or not, as far as unrelated commits are concerned. You won’t get unrelated commits.

† 1: The repetition seems a bit inevitable and annoying, but I use a
script for assembling all of the options to git-format-patch(1).

Quoted 11 lines · Kristoffer Haugsbakk

[snip]

I use the

base..old base..new

Form as well.[1] Then it indeed does not matter if you have rebased or not, as far as unrelated commits are concerned. You won’t get unrelated commits.

Quoted 2 lines

† 1: The repetition seems a bit inevitable and annoying, but I use a
script for assembling all of the options to git-format-patch(1).

What repetition?

git range-diff base old new

;)