Re: [RFC PATCH 2/5] format-patch: add --range-diff option to embed diff in cover letter

2018-07-27 Thread Johannes Schindelin
Hi Eric,

On Thu, 26 Jul 2018, Eric Sunshine wrote:

> On Thu, Jul 26, 2018 at 6:56 AM Johannes Schindelin
>  wrote:
> > On Tue, 17 Jul 2018, Eric Sunshine wrote:
> > > On Tue, Jul 17, 2018 at 6:31 AM Johannes Schindelin
> > >  wrote:
> > > > BTW I like to have an extra space in front of all the range-diff
> > > > lines, to make it easier to discern them from the rest.
> > >
> > > I'm not sure what you mean. Perhaps I'm misreading your comment.
> >
> > Sorry, I was really unclear.
> >
> > In the cover letters sent out by GitGitGadget (or earlier, my
> > mail-patch-series.sh command), I took pains to indent the entire
> > range-diff (or interdiff) with a single space. That is, the footer
> > "Range-diff vs v:" is not indented at all, but all subsequent lines
> > of the range-diff have a leading space.
> >
> > The original reason was to stop confusing `git apply` when sending an
> > interdiff as part of a single patch without a cover letter (in which
> > case mail-patch-series.sh inserted the interdiff below the `---`
> > marker, and the interdiff would have looked like the start of the real
> > diff otherwise).
> 
> The new version[1] likewise indents the interdiff to avoid confusing
> git-am / git-apply.
> 
> [1]: 
> https://public-inbox.org/git/20180722095717.17912-1-sunsh...@sunshineco.com/

Great!

> > In the meantime, I got used to this indentation so much that I do not
> > want to miss it, it is a relatively easy and intuitive visual marker.
> >
> > This, however, will be harder to achieve now that you are using the
> > libified range-diff.
> 
> I toyed with indenting the range-diff in both the cover letter and
> below the "---" line in a patch. With the libified range-diff, doing
> so involves modifying the range-diff implementation (rather than
> having the consumer of the range-diff manage the indentation locally),
> so it adds a bit of complexity to show_range_diff(), though perhaps
> not too much.
> 
> However, I opted against it for a few reasons. First, "header" lines
> apart, all lines of the range-diff are already indented, and the
> existing indentation was sufficient (for me, at least) as a visual
> marker. Second, range-diffs tend to be _wide_, especially the header
> lines, and I was loath to make it wider by indenting more. Third, due
> to the existing indentation of the diff proper, a range-diff won't
> confuse git-am / git-apply, nor will the unindented header lines, so
> extra indentation seemed superfluous.

Totally understandable. For some reason, I never thought about that fact
(a range-diff is *not* a diff) when changing mail-patch-series.ts to use
range-diffs instead of interdiffs.

> > > > > @@ -1438,6 +1480,7 @@ int cmd_format_patch(int argc, const char 
> > > > > **argv, const char *prefix)
> > > > > + const char *range_diff = NULL;
> > > >
> > > > Maybe `range_diff_opt`? It's not exactly the range diff that is
> > > > contained in this variable.
> > >
> > > I could, though I was trying to keep it shorter rather than longer.
> > > This is still the same in the re-roll, but I can rename it if you
> > > insist.
> >
> > I think it will confuse me in the future if I read `range_diff` and
> > even the data type suggests that it could hold the output of a `git
> > range-diff ` run.
> >
> > So I would like to insist.
> 
> In the new version[1], this variable is named 'rdiff_prev' (the
> "previous" version against which the range-diff is to be generated).

Thank you.

> > > > > +format_patch () {
> > > > > + title=$1 &&
> > > > > + range=$2 &&
> > > > > + test_expect_success "format-patch --range-diff ($title)" '
> > > > > + git format-patch --stdout --cover-letter 
> > > > > --range-diff=$range \
> > > > > + master..unmodified >actual &&
> > > > > + grep "= 1: .* s/5/A" actual &&
> > > > > + grep "= 2: .* s/4/A" actual &&
> > > > > + grep "= 3: .* s/11/B" actual &&
> > > > > + grep "= 4: .* s/12/B" actual
> > > >
> > > > I guess this might make sense if `format_patch` was not a
> > > > function, but it is specifically marked as a function... so...
> > > > shouldn't these `grep`s also be using function parameters?
> > >
> > > A later patch adds a second test which specifies the same ranges but
> > > in a different way, so the result will be the same, hence the
> > > hard-coded grep'ing. The function avoids repetition across the two
> > > tests. I suppose I could do this a bit differently, though, to avoid
> > > pretending it's a general-purpose function.
> >
> > If you can think of a way that would make this easier to read for,
> > say, myself if I ever find myself debugging a regression caught by
> > this test, I would appreciate that.
> 
> In the new version, the function is gone; it looks like this:
> 
> --- >8 ---
> for prev in topic master..topic
> do
> test_expect_success "format-patch --range-diff=$prev" '
> git format-patch --stdout --cover-letter --range-diff=$prev \
> 

Re: [RFC PATCH 2/5] format-patch: add --range-diff option to embed diff in cover letter

2018-07-26 Thread Eric Sunshine
On Thu, Jul 26, 2018 at 6:56 AM Johannes Schindelin
 wrote:
> On Tue, 17 Jul 2018, Eric Sunshine wrote:
> > On Tue, Jul 17, 2018 at 6:31 AM Johannes Schindelin
> >  wrote:
> > > BTW I like to have an extra space in front of all the range-diff lines, to
> > > make it easier to discern them from the rest.
> >
> > I'm not sure what you mean. Perhaps I'm misreading your comment.
>
> Sorry, I was really unclear.
>
> In the cover letters sent out by GitGitGadget (or earlier, my
> mail-patch-series.sh command), I took pains to indent the entire
> range-diff (or interdiff) with a single space. That is, the footer
> "Range-diff vs v:" is not indented at all, but all subsequent lines of
> the range-diff have a leading space.
>
> The original reason was to stop confusing `git apply` when sending an
> interdiff as part of a single patch without a cover letter (in which case
> mail-patch-series.sh inserted the interdiff below the `---` marker, and
> the interdiff would have looked like the start of the real diff
> otherwise).

The new version[1] likewise indents the interdiff to avoid confusing
git-am / git-apply.

[1]: 
https://public-inbox.org/git/20180722095717.17912-1-sunsh...@sunshineco.com/

> In the meantime, I got used to this indentation so much that I do not want
> to miss it, it is a relatively easy and intuitive visual marker.
>
> This, however, will be harder to achieve now that you are using the
> libified range-diff.

I toyed with indenting the range-diff in both the cover letter and
below the "---" line in a patch. With the libified range-diff, doing
so involves modifying the range-diff implementation (rather than
having the consumer of the range-diff manage the indentation locally),
so it adds a bit of complexity to show_range_diff(), though perhaps
not too much.

However, I opted against it for a few reasons. First, "header" lines
apart, all lines of the range-diff are already indented, and the
existing indentation was sufficient (for me, at least) as a visual
marker. Second, range-diffs tend to be _wide_, especially the header
lines, and I was loath to make it wider by indenting more. Third, due
to the existing indentation of the diff proper, a range-diff won't
confuse git-am / git-apply, nor will the unindented header lines, so
extra indentation seemed superfluous.

> > > > @@ -1438,6 +1480,7 @@ int cmd_format_patch(int argc, const char **argv, 
> > > > const char *prefix)
> > > > + const char *range_diff = NULL;
> > >
> > > Maybe `range_diff_opt`? It's not exactly the range diff that is contained
> > > in this variable.
> >
> > I could, though I was trying to keep it shorter rather than longer.
> > This is still the same in the re-roll, but I can rename it if you
> > insist.
>
> I think it will confuse me in the future if I read `range_diff` and even
> the data type suggests that it could hold the output of a `git range-diff
> ` run.
>
> So I would like to insist.

In the new version[1], this variable is named 'rdiff_prev' (the
"previous" version against which the range-diff is to be generated).

> > > > +format_patch () {
> > > > + title=$1 &&
> > > > + range=$2 &&
> > > > + test_expect_success "format-patch --range-diff ($title)" '
> > > > + git format-patch --stdout --cover-letter 
> > > > --range-diff=$range \
> > > > + master..unmodified >actual &&
> > > > + grep "= 1: .* s/5/A" actual &&
> > > > + grep "= 2: .* s/4/A" actual &&
> > > > + grep "= 3: .* s/11/B" actual &&
> > > > + grep "= 4: .* s/12/B" actual
> > >
> > > I guess this might make sense if `format_patch` was not a function, but it
> > > is specifically marked as a function... so... shouldn't these `grep`s also
> > > be using function parameters?
> >
> > A later patch adds a second test which specifies the same ranges but
> > in a different way, so the result will be the same, hence the
> > hard-coded grep'ing. The function avoids repetition across the two
> > tests. I suppose I could do this a bit differently, though, to avoid
> > pretending it's a general-purpose function.
>
> If you can think of a way that would make this easier to read for, say,
> myself if I ever find myself debugging a regression caught by this test, I
> would appreciate that.

In the new version, the function is gone; it looks like this:

--- >8 ---
for prev in topic master..topic
do
test_expect_success "format-patch --range-diff=$prev" '
git format-patch --stdout --cover-letter --range-diff=$prev \
master..unmodified >actual &&
grep "= 1: .* s/5/A" actual &&
grep "= 2: .* s/4/A" actual &&
grep "= 3: .* s/11/B" actual &&
grep "= 4: .* s/12/B" actual
'
done
--- >8 ---


Re: [RFC PATCH 2/5] format-patch: add --range-diff option to embed diff in cover letter

2018-07-26 Thread Johannes Schindelin
Hi Eric,

On Tue, 17 Jul 2018, Eric Sunshine wrote:

> On Tue, Jul 17, 2018 at 6:31 AM Johannes Schindelin
>  wrote:
> > On Wed, 30 May 2018, Eric Sunshine wrote:
> 
> > > + if (range_diff) {
> > > + struct argv_array ranges = ARGV_ARRAY_INIT;
> > > + infer_diff_ranges(, range_diff, head);
> > > + if (get_range_diff(, ))
> > > + die(_("failed to generate range-diff"));
> >
> > BTW I like to have an extra space in front of all the range-diff lines, to
> > make it easier to discern them from the rest.
> 
> I'm not sure what you mean. Perhaps I'm misreading your comment.

Sorry, I was really unclear.

In the cover letters sent out by GitGitGadget (or earlier, my
mail-patch-series.sh command), I took pains to indent the entire
range-diff (or interdiff) with a single space. That is, the footer
"Range-diff vs v:" is not indented at all, but all subsequent lines of
the range-diff have a leading space.

The original reason was to stop confusing `git apply` when sending an
interdiff as part of a single patch without a cover letter (in which case
mail-patch-series.sh inserted the interdiff below the `---` marker, and
the interdiff would have looked like the start of the real diff
otherwise).

In the meantime, I got used to this indentation so much that I do not want
to miss it, it is a relatively easy and intuitive visual marker.

This, however, will be harder to achieve now that you are using the
libified range-diff.

> > > @@ -1438,6 +1480,7 @@ int cmd_format_patch(int argc, const char **argv, 
> > > const char *prefix)
> > > + const char *range_diff = NULL;
> >
> > Maybe `range_diff_opt`? It's not exactly the range diff that is contained
> > in this variable.
> 
> I could, though I was trying to keep it shorter rather than longer.
> This is still the same in the re-roll, but I can rename it if you
> insist.

I think it will confuse me in the future if I read `range_diff` and even
the data type suggests that it could hold the output of a `git range-diff
` run.

So I would like to insist.

> > > +format_patch () {
> > > + title=$1 &&
> > > + range=$2 &&
> > > + test_expect_success "format-patch --range-diff ($title)" '
> > > + git format-patch --stdout --cover-letter 
> > > --range-diff=$range \
> > > + master..unmodified >actual &&
> > > + grep "= 1: .* s/5/A" actual &&
> > > + grep "= 2: .* s/4/A" actual &&
> > > + grep "= 3: .* s/11/B" actual &&
> > > + grep "= 4: .* s/12/B" actual
> >
> > I guess this might make sense if `format_patch` was not a function, but it
> > is specifically marked as a function... so... shouldn't these `grep`s also
> > be using function parameters?
> 
> A later patch adds a second test which specifies the same ranges but
> in a different way, so the result will be the same, hence the
> hard-coded grep'ing. The function avoids repetition across the two
> tests. I suppose I could do this a bit differently, though, to avoid
> pretending it's a general-purpose function.

If you can think of a way that would make this easier to read for, say,
myself if I ever find myself debugging a regression caught by this test, I
would appreciate that.

Ciao,
Dscho


Re: [RFC PATCH 2/5] format-patch: add --range-diff option to embed diff in cover letter

2018-07-17 Thread Eric Sunshine
Thanks for the review comments. In the new version of the series
(almost complete), almost the entire implementation has changed,
including most of the stuff on which you commented. Anyhow, see my
responses to your comments below...

On Tue, Jul 17, 2018 at 6:31 AM Johannes Schindelin
 wrote:
> On Wed, 30 May 2018, Eric Sunshine wrote:
> > +static int get_range_diff(struct strbuf *sb,
>
> If you rename `sb` to `out`, it makes it more obvious to the casual reader
> that this strbuf will receive the output.

This is gone in the re-roll.

> > + cp.git_cmd = 1;
> > + argv_array_pushl(, "branch-diff", "--no-color", NULL);
>
> Obviously, this needs to be "range-diff" now.

The re-roll takes advantage of the libified range-diff rather than
invoking it as a command.

> > + argv_array_pushv(, ranges->argv);
>
> As there must be exactly two ranges, it would make more sense to pass them
> explicitly. And then you can use one single call to `argv_array_pushl()`
> instead.

Gone in the re-roll.

> > + struct strbuf diff = STRBUF_INIT;
>
> I guess you want to call it `diff` instead of `range_diff` because a
> future change will reuse this for the interdiff instead? And then also to
> avoid a naming conflict with the parameter.
>
> Dunno. I would still call it `range_diff` until the day comes (if ever)
> when `--interdiff` is implemented. And I would call the `range_diff`
> parameter `range_diff_opt` instead, or some such.

Sharing the variable between interdiff and range-diff was the idea
initially, however, I later decided that the --range-diff and
--interdiff options didn't need to be mutually-exclusive, so, in the
re-roll, all variables now have distinct names (no commonality between
them).

> > + if (range_diff) {
> > + struct argv_array ranges = ARGV_ARRAY_INIT;
> > + infer_diff_ranges(, range_diff, head);
> > + if (get_range_diff(, ))
> > + die(_("failed to generate range-diff"));
>
> BTW I like to have an extra space in front of all the range-diff lines, to
> make it easier to discern them from the rest.

I'm not sure what you mean. Perhaps I'm misreading your comment.

> >   if (!use_stdout &&
> >   open_next_file(NULL, rev->numbered_files ? NULL : "cover-letter", 
> > rev, quiet))
> > - return;
> > + goto done;
>
> Hmm. If you think you will add more `goto done`s in the future, I guess
> this is okay. Otherwise, it would probably make sense to go ahead and
> simply `strbuf_release()` before `return`ing.

In the re-roll, there are several more things which need to be cleaned
up, so the 'goto' makes life easier.

> > @@ -1438,6 +1480,7 @@ int cmd_format_patch(int argc, const char **argv, 
> > const char *prefix)
> > + const char *range_diff = NULL;
>
> Maybe `range_diff_opt`? It's not exactly the range diff that is contained
> in this variable.

I could, though I was trying to keep it shorter rather than longer.
This is still the same in the re-roll, but I can rename it if you
insist.

> > + if (range_diff && !cover_letter)
> > + die(_("--range-diff requires --cover-letter"));
>
> I guess this will be changed in a future patch, allowing a single patch to
> ship with a range diff, too?

Yes, that's already the case in the re-roll.

> > +format_patch () {
> > + title=$1 &&
> > + range=$2 &&
> > + test_expect_success "format-patch --range-diff ($title)" '
> > + git format-patch --stdout --cover-letter --range-diff=$range \
> > + master..unmodified >actual &&
> > + grep "= 1: .* s/5/A" actual &&
> > + grep "= 2: .* s/4/A" actual &&
> > + grep "= 3: .* s/11/B" actual &&
> > + grep "= 4: .* s/12/B" actual
>
> I guess this might make sense if `format_patch` was not a function, but it
> is specifically marked as a function... so... shouldn't these `grep`s also
> be using function parameters?

A later patch adds a second test which specifies the same ranges but
in a different way, so the result will be the same, hence the
hard-coded grep'ing. The function avoids repetition across the two
tests. I suppose I could do this a bit differently, though, to avoid
pretending it's a general-purpose function.


Re: [RFC PATCH 2/5] format-patch: add --range-diff option to embed diff in cover letter

2018-07-17 Thread Johannes Schindelin
Hi Eric,

On Wed, 30 May 2018, Eric Sunshine wrote:

> diff --git a/builtin/log.c b/builtin/log.c
> index e01a256c11..460c31a293 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -28,6 +28,7 @@
>  #include "mailmap.h"
>  #include "gpg-interface.h"
>  #include "progress.h"
> +#include "run-command.h"
>  
>  #define MAIL_DEFAULT_WRAP 72
>  
> @@ -992,6 +993,25 @@ static char *find_branch_name(struct rev_info *rev)
>   return branch;
>  }
>  
> +static void infer_diff_ranges(struct argv_array *args,
> +   const char *prev,
> +   struct commit *head)
> +{
> + argv_array_pushf(args, "%s...%s", prev,
> +  oid_to_hex(>object.oid));
> +}
> +
> +static int get_range_diff(struct strbuf *sb,

If you rename `sb` to `out`, it makes it more obvious to the casual reader
that this strbuf will receive the output.

> +   const struct argv_array *ranges)
> +{
> + struct child_process cp = CHILD_PROCESS_INIT;
> +
> + cp.git_cmd = 1;
> + argv_array_pushl(, "branch-diff", "--no-color", NULL);

Obviously, this needs to be "range-diff" now.

> + argv_array_pushv(, ranges->argv);

As there must be exactly two ranges, it would make more sense to pass them
explicitly. And then you can use one single call to `argv_array_pushl()`
instead.

> + return capture_command(, sb, 0);
> +}
> +
>  static void emit_diffstat(struct rev_info *rev,
> struct commit *origin, struct commit *head)
>  {
> @@ -1016,7 +1036,8 @@ static void make_cover_letter(struct rev_info *rev, int 
> use_stdout,
> struct commit *origin,
> int nr, struct commit **list,
> const char *branch_name,
> -   int quiet)
> +   int quiet,
> +   const char *range_diff)
>  {
>   const char *committer;
>   const char *body = "*** SUBJECT HERE ***\n\n*** BLURB HERE ***\n";
> @@ -1028,15 +1049,25 @@ static void make_cover_letter(struct rev_info *rev, 
> int use_stdout,
>   int need_8bit_cte = 0;
>   struct pretty_print_context pp = {0};
>   struct commit *head = list[0];
> + struct strbuf diff = STRBUF_INIT;

I guess you want to call it `diff` instead of `range_diff` because a
future change will reuse this for the interdiff instead? And then also to
avoid a naming conflict with the parameter.

Dunno. I would still call it `range_diff` until the day comes (if ever)
when `--interdiff` is implemented. And I would call the `range_diff`
parameter `range_diff_opt` instead, or some such.

Or maybe use `extra_footer` instead of `diff`.

>   if (!cmit_fmt_is_mail(rev->commit_format))
>   die(_("Cover letter needs email format"));
>  
>   committer = git_committer_info(0);
>  
> + /* might die from bad user input so try before creating cover letter */
> + if (range_diff) {
> + struct argv_array ranges = ARGV_ARRAY_INIT;
> + infer_diff_ranges(, range_diff, head);
> + if (get_range_diff(, ))
> + die(_("failed to generate range-diff"));

BTW I like to have an extra space in front of all the range-diff lines, to
make it easier to discern them from the rest.

> + argv_array_clear();
> + }
> +
>   if (!use_stdout &&
>   open_next_file(NULL, rev->numbered_files ? NULL : "cover-letter", 
> rev, quiet))
> - return;
> + goto done;

Hmm. If you think you will add more `goto done`s in the future, I guess
this is okay. Otherwise, it would probably make sense to go ahead and
simply `strbuf_release()` before `return`ing.

>   log_write_email_headers(rev, head, _subject, _8bit_cte);
>  
> @@ -1077,6 +1108,17 @@ static void make_cover_letter(struct rev_info *rev, 
> int use_stdout,
>   /* We can only do diffstat with a unique reference point */
>   if (origin)
>   emit_diffstat(rev, origin, head);
> +
> + if (diff.len) {
> + FILE *fp = rev->diffopt.file;
> + fputs(_("Changes since previous version:"), fp);
> + fputs("\n\n", fp);
> + fputs(diff.buf, fp);
> + fputc('\n', fp);
> + }
> +
> +done:
> + strbuf_release();
>  }
>  
>  static const char *clean_message_id(const char *msg_id)
> @@ -1438,6 +1480,7 @@ int cmd_format_patch(int argc, const char **argv, const 
> char *prefix)
>   struct base_tree_info bases;
>   int show_progress = 0;
>   struct progress *progress = NULL;
> + const char *range_diff = NULL;

Maybe `range_diff_opt`? It's not exactly the range diff that is contained
in this variable.

>   const struct option builtin_format_patch_options[] = {
>   { OPTION_CALLBACK, 'n', "numbered", , NULL,
> @@ -1511,6 +1554,8 @@ int cmd_format_patch(int argc, const char **argv, const 
> char *prefix)
>   OPT__QUIET(, 

[RFC PATCH 2/5] format-patch: add --range-diff option to embed diff in cover letter

2018-05-30 Thread Eric Sunshine
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 an interdiff or range-diff, however, doing so
involves manually copy/pasting the diff into the cover letter.

Add a --range-diff option to automate this process for range-diffs. 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 early 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  | 55 --
 t/t7910-branch-diff.sh | 15 
 3 files changed, 77 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-format-patch.txt 
b/Documentation/git-format-patch.txt
index 6cbe462a77..f4c70e6b64 100644
--- a/Documentation/git-format-patch.txt
+++ b/Documentation/git-format-patch.txt
@@ -23,6 +23,7 @@ SYNOPSIS
   [(--reroll-count|-v) ]
   [--to=] [--cc=]
   [--[no-]cover-letter] [--quiet] [--notes[=]]
+  [--range-diff=]
   [--progress]
   []
   [  |  ]
@@ -228,6 +229,15 @@ feeding the result to `git send-email`.
containing the branch description, shortlog and the overall diffstat.  
You can
fill in a description in the file before sending it out.
 
+--range-diff=::
+   As a reviewer aid, insert a range-diff (see linkgit:git-branch-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 e01a256c11..460c31a293 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -28,6 +28,7 @@
 #include "mailmap.h"
 #include "gpg-interface.h"
 #include "progress.h"
+#include "run-command.h"
 
 #define MAIL_DEFAULT_WRAP 72
 
@@ -992,6 +993,25 @@ static char *find_branch_name(struct rev_info *rev)
return branch;
 }
 
+static void infer_diff_ranges(struct argv_array *args,
+ const char *prev,
+ struct commit *head)
+{
+   argv_array_pushf(args, "%s...%s", prev,
+oid_to_hex(>object.oid));
+}
+
+static int get_range_diff(struct strbuf *sb,
+ const struct argv_array *ranges)
+{
+   struct child_process cp = CHILD_PROCESS_INIT;
+
+   cp.git_cmd = 1;
+   argv_array_pushl(, "branch-diff", "--no-color", NULL);
+   argv_array_pushv(, ranges->argv);
+   return capture_command(, sb, 0);
+}
+
 static void emit_diffstat(struct rev_info *rev,
  struct commit *origin, struct commit *head)
 {
@@ -1016,7 +1036,8 @@ static void make_cover_letter(struct rev_info *rev, int 
use_stdout,
  struct commit *origin,
  int nr, struct commit **list,
  const char *branch_name,
- int quiet)
+ int quiet,
+ const char *range_diff)
 {
const char *committer;
const char *body = "*** SUBJECT HERE ***\n\n*** BLURB HERE ***\n";
@@ -1028,15 +1049,25 @@ static void make_cover_letter(struct rev_info *rev, int 
use_stdout,
int need_8bit_cte = 0;
struct pretty_print_context pp = {0};
struct commit *head = list[0];
+   struct strbuf diff = STRBUF_INIT;
 
if (!cmit_fmt_is_mail(rev->commit_format))
die(_("Cover letter needs email format"));
 
committer = git_committer_info(0);
 
+   /* might die from bad user input so try before creating cover letter */
+   if (range_diff) {
+   struct argv_array ranges = ARGV_ARRAY_INIT;
+   infer_diff_ranges(, range_diff, head);
+   if (get_range_diff(, ))
+   die(_("failed to generate range-diff"));
+   argv_array_clear();
+   }
+
if (!use_stdout &&
open_next_file(NULL, rev->numbered_files ? NULL : "cover-letter", 
rev, quiet))
-   return;
+   goto done;
 
log_write_email_headers(rev, head, _subject, _8bit_cte);
 
@@ -1077,6 +1108,17 @@ static void