Re: [PATCH 10/14] format-patch: add --range-diff option to embed diff in cover letter
Eric Sunshine writes:
> I did consider other approaches, such as a more generic option. For
> example, --embed=range-diff:. I also considered supporting more
> complex use-cases. For instance, git-range-diff itself accepts the two
> ranges in several formats ("a..b c..d" or "x...z" or "i j k"), plus
> the creation-factor can be tweaked, so I weighed ways of incorporating
> all that detail into the single argument to --range-diff.
>
> However, those are all difficult to use (onerous to type and remember)
> and difficult to document. Thus, I opted for simplicity of individual,
> straight-forward options (--interdiff, --range-diff,
> --creation-factor), which are easy to type, remember, and document.
Yes, that matches my conclusion that it is OK to have these on
separate options that can grow as many meta-diff formats we would
support.
Re: [PATCH 10/14] format-patch: add --range-diff option to embed diff in cover letter
On Wed, Jul 25, 2018 at 1:29 PM Junio C Hamano wrote:
> A few random thoughts.
>
> * I find it somewhat disturbing that use of dash is inconsistent
>between "--interdiff=" and "--range-diff=".
I went with the common spellings we've seen on the mailing lists.
"Interdiff", in particular, seems well established. "Range-diff" is
new, so we don't have much data other than what we saw during the
discussion when renaming git-branch-diff, and, of course,
git-branch-diff itself is hyphenated. Another consideration: "inter"
is a prefix, whereas "range" stands on its own.
I don't have a super strong opinion, as I'm used to both the
hyphenated name (from discussion and the command name itself), and the
unhyphenated name ("rangediff" was my local branch name). I'm open to
opinions.
We probably wouldn't want to do so, but another possibility is to
recognize both --range-diff and --rangediff.
> * Perhaps "--interdiff= --range-diff" may be a possible
>way to say "use the same and show both"? Do we want
>"--range-diff= --interdiff" to mean the same two
>meta-diff but shown in different order?
That's not at all a bad shorthand. I like it better than the
"--all-kinds-of-diff=" mentioned earlier. We don't need to make a
decision for this series since such functionality can be added later.
The order of specification of the two options affecting the order of
output is also an interesting idea. We may want to decide that before
graduating this topic.
> * Do we expect that we may find a third-kind of "meta-diff" that
>sits next to interdiff and range-diff in the future? I *think*
>two separate options "--interdiff=..." and "--range-diff=..." is
>still a good way forward, and we'd add "--frotzdiff=..." when
>such a third-kind of "meta-diff" turns out to be useful, without
>fearing proliferation of options, and that would be OK, but I am
>just thinking aloud to see if other people have better ideas.
I did consider other approaches, such as a more generic option. For
example, --embed=range-diff:. I also considered supporting more
complex use-cases. For instance, git-range-diff itself accepts the two
ranges in several formats ("a..b c..d" or "x...z" or "i j k"), plus
the creation-factor can be tweaked, so I weighed ways of incorporating
all that detail into the single argument to --range-diff.
However, those are all difficult to use (onerous to type and remember)
and difficult to document. Thus, I opted for simplicity of individual,
straight-forward options (--interdiff, --range-diff,
--creation-factor), which are easy to type, remember, and document.
We may want to revisit this later if git-format-patch grows additional
similar options.
Re: [PATCH 10/14] format-patch: add --range-diff option to embed diff in cover letter
On Mon, Jul 23, 2018 at 9:59 PM Eric Sunshine wrote: > > On Mon, Jul 23, 2018 at 12:28 PM Duy Nguyen wrote: > > On Sun, Jul 22, 2018 at 11:58 AM Eric Sunshine > > wrote: > > > diff --git a/Documentation/git-format-patch.txt > > > b/Documentation/git-format-patch.txt > > > index f8a061794d..e7f404be3d 100644 > > > --- a/Documentation/git-format-patch.txt > > > +++ b/Documentation/git-format-patch.txt > > > @@ -24,6 +24,7 @@ SYNOPSIS > > >[--to=] [--cc=] > > >[--[no-]cover-letter] [--quiet] [--notes[=]] > > >[--interdiff=] > > > + [--range-diff=] > > > > I wonder if people will use both --interdiff= and > > --range-diff= often enough to justify a shortcut > > "--all-kinds-of-diff=" so that we don't have to type > > twice. But I guess we don't have to worry about this right now. > > My original thought was that --interdiff and --range-diff would be > mutually exclusive, however, I quickly realized that some people might > like to use both options together since each format has its strengths > and weaknesses. (I've used both types of diffs together when preparing > rerolls of my own series and found that, together, they provided a > better picture of the reroll than either would have alone.) I actually had another question that I answered myself: how do I know which one to choose? There's no preview option (and I'm lazy, I don't want to do separate diff commands myself). So my answer was "choose both, then delete the one that does not look good (and explain it in the cover too when I delete it)" > And, as you note, it's something that can be added later if > warranted (plus, this series is already long and I'd like to avoid > making it longer for something like this whose value is only > speculative). Yes of course. We can revisit this later. -- Duy
Re: [PATCH 10/14] format-patch: add --range-diff option to embed diff in cover letter
Eric Sunshine writes: > On Mon, Jul 23, 2018 at 12:28 PM Duy Nguyen wrote: >> On Sun, Jul 22, 2018 at 11:58 AM Eric Sunshine >> wrote: >> > diff --git a/Documentation/git-format-patch.txt >> > b/Documentation/git-format-patch.txt >> > index f8a061794d..e7f404be3d 100644 >> > --- a/Documentation/git-format-patch.txt >> > +++ b/Documentation/git-format-patch.txt >> > @@ -24,6 +24,7 @@ SYNOPSIS >> >[--to=] [--cc=] >> >[--[no-]cover-letter] [--quiet] [--notes[=]] >> >[--interdiff=] >> > + [--range-diff=] >> >> I wonder if people will use both --interdiff= and >> --range-diff= often enough to justify a shortcut >> "--all-kinds-of-diff=" so that we don't have to type >> twice. But I guess we don't have to worry about this right now. > > My original thought was that --interdiff and --range-diff would be > mutually exclusive, however, I quickly realized that some people might > like to use both options together since each format has its strengths > and weaknesses. (I've used both types of diffs together when preparing > rerolls of my own series and found that, together, they provided a > better picture of the reroll than either would have alone.) > > Based upon experience on this mailing list, I'd guess that most people > would use only one or the other, though that doesn't speak for other > projects. And, as you note, it's something that can be added later if > warranted (plus, this series is already long and I'd like to avoid > making it longer for something like this whose value is only > speculative). A few random thoughts. * I find it somewhat disturbing that use of dash is inconsistent between "--interdiff=" and "--range-diff=". * Perhaps "--interdiff= --range-diff" may be a possible way to say "use the same and show both"? Do we want "--range-diff= --interdiff" to mean the same two meta-diff but shown in different order? * Do we expect that we may find a third-kind of "meta-diff" that sits next to interdiff and range-diff in the future? I *think* two separate options "--interdiff=..." and "--range-diff=..." is still a good way forward, and we'd add "--frotzdiff=..." when such a third-kind of "meta-diff" turns out to be useful, without fearing proliferation of options, and that would be OK, but I am just thinking aloud to see if other people have better ideas.
Re: [PATCH 10/14] format-patch: add --range-diff option to embed diff in cover letter
On Mon, Jul 23, 2018 at 12:28 PM Duy Nguyen wrote: > On Sun, Jul 22, 2018 at 11:58 AM Eric Sunshine > wrote: > > diff --git a/Documentation/git-format-patch.txt > > b/Documentation/git-format-patch.txt > > index f8a061794d..e7f404be3d 100644 > > --- a/Documentation/git-format-patch.txt > > +++ b/Documentation/git-format-patch.txt > > @@ -24,6 +24,7 @@ SYNOPSIS > >[--to=] [--cc=] > >[--[no-]cover-letter] [--quiet] [--notes[=]] > >[--interdiff=] > > + [--range-diff=] > > I wonder if people will use both --interdiff= and > --range-diff= often enough to justify a shortcut > "--all-kinds-of-diff=" so that we don't have to type > twice. But I guess we don't have to worry about this right now. My original thought was that --interdiff and --range-diff would be mutually exclusive, however, I quickly realized that some people might like to use both options together since each format has its strengths and weaknesses. (I've used both types of diffs together when preparing rerolls of my own series and found that, together, they provided a better picture of the reroll than either would have alone.) Based upon experience on this mailing list, I'd guess that most people would use only one or the other, though that doesn't speak for other projects. And, as you note, it's something that can be added later if warranted (plus, this series is already long and I'd like to avoid making it longer for something like this whose value is only speculative).
Re: [PATCH 10/14] format-patch: add --range-diff option to embed diff in cover letter
On Sun, Jul 22, 2018 at 11:58 AM Eric Sunshine wrote: > diff --git a/Documentation/git-format-patch.txt > b/Documentation/git-format-patch.txt > index f8a061794d..e7f404be3d 100644 > --- a/Documentation/git-format-patch.txt > +++ b/Documentation/git-format-patch.txt > @@ -24,6 +24,7 @@ SYNOPSIS >[--to=] [--cc=] >[--[no-]cover-letter] [--quiet] [--notes[=]] >[--interdiff=] > + [--range-diff=] I wonder if people will use both --interdiff= and --range-diff= often enough to justify a shortcut "--all-kinds-of-diff=" so that we don't have to type twice. But I guess we don't have to worry about this right now. -- Duy
[PATCH 10/14] format-patch: add --range-diff option to embed diff in cover letter
When submitting a revised version of a patch series, it can be helpful
(to reviewers) to include a summary of changes since the previous
attempt in the form of a range-diff, however, doing so involves manually
copy/pasting the diff into the cover letter.
Add a --range-diff option to automate this process. The argument to
--range-diff specifies the tip of the previous attempt against which to
generate the range-diff. For example:
git format-patch --cover-letter --range-diff=v1 -3 v2
(At this stage, the previous attempt and the patch series being
formatted must share a common base, however, a subsequent enhancement
will make it possible to specify an explicit revision range for the
previous attempt.)
Signed-off-by: Eric Sunshine
---
Documentation/git-format-patch.txt | 10 +
builtin/log.c | 35 ++
revision.h | 5 +
t/t3206-range-diff.sh | 12 ++
4 files changed, 62 insertions(+)
diff --git a/Documentation/git-format-patch.txt
b/Documentation/git-format-patch.txt
index f8a061794d..e7f404be3d 100644
--- a/Documentation/git-format-patch.txt
+++ b/Documentation/git-format-patch.txt
@@ -24,6 +24,7 @@ SYNOPSIS
[--to=] [--cc=]
[--[no-]cover-letter] [--quiet] [--notes[=]]
[--interdiff=]
+ [--range-diff=]
[--progress]
[]
[ | ]
@@ -238,6 +239,15 @@ feeding the result to `git send-email`.
the series being formatted (for example `git format-patch
--cover-letter --interdiff=feature/v1 -3 feature/v2`).
+--range-diff=::
+ 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
+ example `git format-patch --cover-letter --range-diff=feature/v1 -3
+ feature/v2`).
+
--notes[=]::
Append the notes (see linkgit:git-notes[1]) for the commit
after the three-dash line.
diff --git a/builtin/log.c b/builtin/log.c
index e990027c28..d6e57e8b04 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -31,6 +31,7 @@
#include "progress.h"
#include "commit-slab.h"
#include "interdiff.h"
+#include "range-diff.h"
#define MAIL_DEFAULT_WRAP 72
@@ -1088,6 +1089,12 @@ static void make_cover_letter(struct rev_info *rev, int
use_stdout,
fprintf_ln(rev->diffopt.file, "%s", rev->idiff_title);
show_interdiff(rev, 0);
}
+
+ if (rev->rdiff1) {
+ fprintf_ln(rev->diffopt.file, "%s", _("Range-diff:"));
+ show_range_diff(rev->rdiff1, rev->rdiff2,
+ rev->creation_factor, 1, >diffopt);
+ }
}
static const char *clean_message_id(const char *msg_id)
@@ -1437,6 +1444,17 @@ static const char *diff_title(struct strbuf *sb, int
reroll_count,
return sb->buf;
}
+static void infer_range_diff_ranges(struct strbuf *r1,
+ struct strbuf *r2,
+ const char *prev,
+ 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);
+}
+
int cmd_format_patch(int argc, const char **argv, const char *prefix)
{
struct commit *commit;
@@ -1466,6 +1484,9 @@ int cmd_format_patch(int argc, const char **argv, const
char *prefix)
struct progress *progress = NULL;
struct oid_array idiff_prev = OID_ARRAY_INIT;
struct strbuf idiff_title = STRBUF_INIT;
+ const char *rdiff_prev = NULL;
+ struct strbuf rdiff1 = STRBUF_INIT;
+ struct strbuf rdiff2 = STRBUF_INIT;
const struct option builtin_format_patch_options[] = {
{ OPTION_CALLBACK, 'n', "numbered", , NULL,
@@ -1542,6 +1563,8 @@ int cmd_format_patch(int argc, const char **argv, const
char *prefix)
OPT_CALLBACK(0, "interdiff", _prev, N_("rev"),
N_("show changes against in cover letter or
single patch"),
parse_opt_object_name),
+ OPT_STRING(0, "range-diff", _prev, N_("refspec"),
+ N_("show changes against in cover
letter")),
OPT_END()
};
@@ -1774,6 +1797,16 @@ int cmd_format_patch(int argc, const char **argv, const
char *prefix)
_("Interdiff against v%d:"));
}
+ if (rdiff_prev) {
+ if (!cover_letter)
+ die(_("--range-diff requires --cover-letter"));
+
+
