git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Johannes Schindelin <johannes.schindelin@gmx.de>
Date
Jul 17, 2018, 10:30 UTC
Message-ID
<nycvar.QRO.7.76.6.1807171219480.71@tvgsbejvaqbjf.bet>
In-Reply-To
<20180530080325.37520-3-sunshine@sunshineco.com>
Hi Eric,
On Wed, 30 May 2018, Eric Sunshine wrote:
Show 25 quoted lines
> 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(&head->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.

Show 6 quoted lines
> +			  const struct argv_array *ranges)
> +{
> +	struct child_process cp = CHILD_PROCESS_INIT;
> +
> +	cp.git_cmd = 1;
> +	argv_array_pushl(&cp.args, "branch-diff", "--no-color", NULL);
Obviously, this needs to be "range-diff" now.
> +	argv_array_pushv(&cp.args, 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.

Show 21 quoted lines
> +	return capture_command(&cp, 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`.
Show 11 quoted lines
>  	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(&ranges, range_diff, head);
> +		if (get_range_diff(&diff, &ranges))
> +			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.

Show 7 quoted lines
> +		argv_array_clear(&ranges);
> +	}
> +
>  	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(&diff)` before `return`ing.

Show 25 quoted lines
>  	log_write_email_headers(rev, head, &pp.after_subject, &need_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(&diff);
>  }
>  
>  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.

Show 17 quoted lines
>  	const struct option builtin_format_patch_options[] = {
>  		{ OPTION_CALLBACK, 'n', "numbered", &numbered, NULL,
> @@ -1511,6 +1554,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>  		OPT__QUIET(&quiet, N_("don't print the patch filenames")),
>  		OPT_BOOL(0, "progress", &show_progress,
>  			 N_("show progress while generating patches")),
> +		OPT_STRING(0, "range-diff", &range_diff, N_("rev-range"),
> +			   N_("show changes against <rev-range> in cover letter")),
>  		OPT_END()
>  	};
>  
> @@ -1733,6 +1778,9 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>  	if (numbered)
>  		rev.total = total + start_number - 1;
>  
> +	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?

Show 32 quoted lines
> +
>  	if (!signature) {
>  		; /* --no-signature inhibits all signatures */
>  	} else if (signature && signature != git_version_string) {
> @@ -1764,7 +1812,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>  		if (thread)
>  			gen_message_id(&rev, "cover");
>  		make_cover_letter(&rev, use_stdout,
> -				  origin, nr, list, branch_name, quiet);
> +				  origin, nr, list, branch_name, quiet,
> +				  range_diff);
>  		print_bases(&bases, rev.diffopt.file);
>  		print_signature(rev.diffopt.file);
>  		total++;
> diff --git a/t/t7910-branch-diff.sh b/t/t7910-branch-diff.sh
> index a7fece8804..edbd69b6f8 100755
> --- a/t/t7910-branch-diff.sh
> +++ b/t/t7910-branch-diff.sh
> @@ -141,4 +141,19 @@ test_expect_success 'changed message' '
>  	test_cmp expected actual
>  '
>  
> +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?

Show 8 quoted lines
> +	'
> +}
> +
> +format_patch 'B...C' topic
> +
>  test_done
> -- 
> 2.17.1.1235.ge295dfb56e

Thanks, Dscho

Previous: Eric SunshineNext: Eric Sunshine
Message 3 of 29 in “format-patch: automate cover letter range-diff”
  1. 0/5 format-patch: automate cover letter range-diffEric Sunshine, May 30, 2018
  2. 2/5 format-patch: add --range-diff option to embed diff in cover letterEric Sunshine, May 30, 2018
  3. Johannes SchindelinJul 17, 2018
  4. Eric SunshineJul 17, 2018
  5. Johannes SchindelinJul 26, 2018
  6. Eric SunshineJul 26, 2018
  7. Johannes SchindelinJul 27, 2018
  8. 1/5 format-patch: allow additional generated content in make_cover_letter()Eric Sunshine, May 30, 2018
  9. Johannes SchindelinJul 17, 2018
  10. Eric SunshineJul 17, 2018
  11. 3/5 format-patch: extend --range-diff to accept revision rangeEric Sunshine, May 30, 2018
  12. Stefan BellerMay 30, 2018
  13. Eric SunshineMay 30, 2018
  14. Johannes SchindelinJul 17, 2018
  15. Eric SunshineJul 17, 2018
  16. 4/5 format-patch: teach --range-diff to respect -v/--reroll-countEric Sunshine, May 30, 2018
  17. Stefan BellerMay 30, 2018
  18. Eric SunshineMay 30, 2018
  19. Stefan BellerMay 30, 2018
  20. Eric SunshineMay 30, 2018
  21. 5/5 format-patch: add --creation-weight tweak for --range-diffEric Sunshine, May 30, 2018
  22. Johannes SchindelinJul 17, 2018
  23. Ævar Arnfjörð BjarmasonMay 30, 2018
  24. Duy NguyenJun 6, 2018
  25. Eric SunshineJun 7, 2018
  26. Duy NguyenJun 7, 2018
  27. Johannes SchindelinJul 17, 2018
  28. Andrei RybakJul 26, 2018
  29. Johannes SchindelinJul 26, 2018

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.