Re: [PATCH 11/14] format-patch: extend --range-diff to accept revision range
On Wed, Jul 25, 2018 at 4:53 PM Junio C Hamano wrote:
> Eric Sunshine writes:
> > When submitting a revised a patch series, the --range-diff option embeds
> > a range-diff in the cover letter showing changes since the previous
> > version of the patch series. The argument to --range-diff is a simple
> > revision naming the tip of the previous series [...]
> >
> > However, it fails if the revision ranges of the old and new versions of
> > the series are disjoint. To address this shortcoming, extend
> > --range-diff to also accept an explicit revision range for the previous
> > series. [...]
>
> > static void infer_range_diff_ranges(struct strbuf *r1,
> > {
> > - strbuf_addf(r1, "%s..%s", head_oid, prev);
> > - strbuf_addf(r2, "%s..%s", prev, head_oid);
>
> I thought "git range-diff" also took the three-dot notation as a
> short-hand but there is no point using that in this application,
> which wants the RHS and the LHS range in separate output variables.
Correct.
> I actually would feel better to see the second range for the normal
> case to be computed exactly the same way, i.e.
>
> int prev_is_range = strstr(prev, "..");
>
> if (prev_is_range)
> strbuf_addstr(r1, prev);
> else
> strbuf_addf(r1, "%s..%s", head_oid, prev);
>
> if (origin)
> strbuf_addf(r2, "%s..%s", oid_to_hex(>object.oid,
> head_oid);
> else if (prev_is_range)
> die("cannot figure out the origin of new series");
> else {
> warning("falling back to use '%s' as the origin of new
> series", prev);
> strbuf_addf(r2, "%s..%s", prev, head_oid);
> }
>
> because origin..HEAD is always the set of the "new" series, no
> matter what "prev" the user chooses to compare that series against,
> when there is a single "origin".
I plan to submit a short series as incremental changes atop
es/format-patch-{interdiff,rangediff} to address the few review
comments[1] (including my own[2]), so I can make this change, as well.
[1]:
https://public-inbox.org/git/CAPig+cSuYUYSPTuKx08wcmQM-G12_-W2T4BS07fA=6grm1b...@mail.gmail.com/T/#rfaf5a563e81b862bd8ed69232040056ab9a86dd8
[2]:
https://public-inbox.org/git/CAPig+cSuYUYSPTuKx08wcmQM-G12_-W2T4BS07fA=6grm1b...@mail.gmail.com/
Re: [PATCH 11/14] format-patch: extend --range-diff to accept revision range
Eric Sunshine writes:
> When submitting a revised a patch series, the --range-diff option embeds
> a range-diff in the cover letter showing changes since the previous
> version of the patch series. The argument to --range-diff is a simple
> revision naming the tip of the previous series, which works fine if the
> previous and current versions of the patch series share a common base.
>
> However, it fails if the revision ranges of the old and new versions of
> the series are disjoint. To address this shortcoming, extend
> --range-diff to also accept an explicit revision range for the previous
> series. For example:
>
> git format-patch --cover-letter --range-diff=v1~3..v1 -3 v2
>
> Signed-off-by: Eric Sunshine
> ---
Makes sense. Even though a single "common rev" would serve as a
feature to discourage rebasing done "just to catch up" without a
good reason, it is a good idea to give an escape hatch like this
to support a case where rebasing is the right thing to do.
> static void infer_range_diff_ranges(struct strbuf *r1,
> struct strbuf *r2,
> const char *prev,
> + struct commit *origin,
> struct commit *head)
> {
> const char *head_oid = oid_to_hex(>object.oid);
>
> - strbuf_addf(r1, "%s..%s", head_oid, prev);
> - strbuf_addf(r2, "%s..%s", prev, head_oid);
I thought "git range-diff" also took the three-dot notation as a
short-hand but there is no point using that in this application,
which wants the RHS and the LHS range in separate output variables.
> + if (!strstr(prev, "..")) {
> + strbuf_addf(r1, "%s..%s", head_oid, prev);
> + strbuf_addf(r2, "%s..%s", prev, head_oid);
> + } else if (!origin) {
> + die(_("failed to infer range-diff ranges"));
> + } else {
> + strbuf_addstr(r1, prev);
> + strbuf_addf(r2, "%s..%s",
> + oid_to_hex(>object.oid), head_oid);
Interesting.
I actually would feel better to see the second range for the normal
case to be computed exactly the same way, i.e.
int prev_is_range = strstr(prev, "..");
if (prev_is_range)
strbuf_addstr(r1, prev);
else
strbuf_addf(r1, "%s..%s", head_oid, prev);
if (origin)
strbuf_addf(r2, "%s..%s", oid_to_hex(>object.oid,
head_oid);
else if (prev_is_range)
die("cannot figure out the origin of new series");
else {
warning("falling back to use '%s' as the origin of new series",
prev);
strbuf_addf(r2, "%s..%s", prev, head_oid);
}
because origin..HEAD is always the set of the "new" series, no
matter what "prev" the user chooses to compare that series against,
when there is a single "origin".
[PATCH 11/14] format-patch: extend --range-diff to accept revision range
When submitting a revised a patch series, the --range-diff option embeds
a range-diff in the cover letter showing changes since the previous
version of the patch series. The argument to --range-diff is a simple
revision naming the tip of the previous series, which works fine if the
previous and current versions of the patch series share a common base.
However, it fails if the revision ranges of the old and new versions of
the series are disjoint. To address this shortcoming, extend
--range-diff to also accept an explicit revision range for the previous
series. For example:
git format-patch --cover-letter --range-diff=v1~3..v1 -3 v2
Signed-off-by: Eric Sunshine
---
Documentation/git-format-patch.txt | 8 +---
builtin/log.c | 16 +---
t/t3206-range-diff.sh | 2 +-
3 files changed, 19 insertions(+), 7 deletions(-)
diff --git a/Documentation/git-format-patch.txt
b/Documentation/git-format-patch.txt
index e7f404be3d..425145f536 100644
--- a/Documentation/git-format-patch.txt
+++ b/Documentation/git-format-patch.txt
@@ -243,10 +243,12 @@ feeding the result to `git send-email`.
As a reviewer aid, insert a range-diff (see linkgit:git-range-diff[1])
into the cover letter showing the differences between the previous
version of the patch series and the series currently being formatted.
- `previous` is a single revision naming the tip of the previous
- series which shares a common base with the series being formatted (for
+ `previous` can be a single revision naming the tip of the previous
+ series if it shares a common base with the series being formatted (for
example `git format-patch --cover-letter --range-diff=feature/v1 -3
- feature/v2`).
+ feature/v2`), or a revision range if the two versions of the series are
+ disjoint (for example `git format-patch --cover-letter
+ --range-diff=feature/v1~3..feature/v1 -3 feature/v2`).
--notes[=]::
Append the notes (see linkgit:git-notes[1]) for the commit
diff --git a/builtin/log.c b/builtin/log.c
index d6e57e8b04..4f937aad15 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1447,12 +1447,21 @@ static const char *diff_title(struct strbuf *sb, int
reroll_count,
static void infer_range_diff_ranges(struct strbuf *r1,
struct strbuf *r2,
const char *prev,
+ struct commit *origin,
struct commit *head)
{
const char *head_oid = oid_to_hex(>object.oid);
- strbuf_addf(r1, "%s..%s", head_oid, prev);
- strbuf_addf(r2, "%s..%s", prev, head_oid);
+ if (!strstr(prev, "..")) {
+ strbuf_addf(r1, "%s..%s", head_oid, prev);
+ strbuf_addf(r2, "%s..%s", prev, head_oid);
+ } else if (!origin) {
+ die(_("failed to infer range-diff ranges"));
+ } else {
+ strbuf_addstr(r1, prev);
+ strbuf_addf(r2, "%s..%s",
+ oid_to_hex(>object.oid), head_oid);
+ }
}
int cmd_format_patch(int argc, const char **argv, const char *prefix)
@@ -1801,7 +1810,8 @@ int cmd_format_patch(int argc, const char **argv, const
char *prefix)
if (!cover_letter)
die(_("--range-diff requires --cover-letter"));
- infer_range_diff_ranges(, , rdiff_prev, list[0]);
+ infer_range_diff_ranges(, , rdiff_prev,
+ origin, list[0]);
rev.rdiff1 = rdiff1.buf;
rev.rdiff2 = rdiff2.buf;
rev.creation_factor = RANGE_DIFF_CREATION_FACTOR_DEFAULT;
diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
index dd854b6ebc..3d7a2d8a4d 100755
--- a/t/t3206-range-diff.sh
+++ b/t/t3206-range-diff.sh
@@ -142,7 +142,7 @@ test_expect_success 'changed message' '
test_cmp expected actual
'
-for prev in topic
+for prev in topic master..topic
do
test_expect_success "format-patch --range-diff=$prev" '
git format-patch --stdout --cover-letter --range-diff=$prev \
--
2.18.0.345.g5c9ce644c3
