threads / patch / 12320

patch, 2 partsImprove collection of information for format-patch --cover-letter

Subject: [PATCH 2/2] Improve collection of information for format-patch --cover-letter

## tl;dr

8 messages between Feb 25, 2008 and Feb 28, 2008. Diffs are folded; open one to read it.

replies: 7people: 2as markdown or json

Daniel Barkalow· Feb 25, 2008, 23:24 UTC · lore

Use the "boundary" feature to find the origin (or find that there are multiple origins), and use the actual list of commits to pass to shortlog.

This makes all cover letter include shortlogs, and all cover letters for series with a single boundary commit include diffstats (if there are multiple boundary commits it's unclear what would be meaningful as a diffstat). Note that the single boundary test is empirical, not theoretical; even a -2 limiting condition will give a diffstat if there's only one boundary commit in this particular case.

Signed-off-by: Daniel Barkalow <barkalow@iabervon.org>
---
 builtin-log.c |   49 ++++++++++++++++++++++++++-----------------------
 1 files changed, 26 insertions(+), 23 deletions(-)
Show changes to builtin-log.c +26 −23
diff --git a/builtin-log.c b/builtin-log.c
index 2b70aa9..3209ea5 100644
--- a/builtin-log.c
+++ b/builtin-log.c
@@ -16,6 +16,7 @@
 #include "patch-ids.h"
 #include "refs.h"
 #include "run-command.h"
+#include "shortlog.h"
 
 static int default_show_root = 1;
 static const char *fmt_patch_subject_prefix = "PATCH";
@@ -640,9 +641,10 @@ static void gen_message_id(struct rev_info *info, char *base)
 	info->message_id = strbuf_detach(&buf, NULL);
 }
 
-static void make_cover_letter(struct rev_info *rev,
-		int use_stdout, int numbered, int numbered_files,
-			      struct commit *origin, struct commit *head)
+static void make_cover_letter(struct rev_info *rev, int use_stdout,
+			      int numbered, int numbered_files,
+			      struct commit *origin,
+			      int nr, struct commit **list, struct commit *head)
 {
 	const char *committer;
 	const char *origin_sha1, *head_sha1;
@@ -651,7 +653,9 @@ static void make_cover_letter(struct rev_info *rev,
 	const char *body = "*** SUBJECT HERE ***\n\n*** BLURB HERE ***\n";
 	const char *msg;
 	const char *extra_headers = rev->extra_headers;
+	struct shortlog log;
 	struct strbuf sb;
+	int i;
 	const char *encoding = "utf-8";
 
 	if (rev->commit_format != CMIT_FMT_EMAIL)
@@ -661,7 +665,6 @@ static void make_cover_letter(struct rev_info *rev,
 				NULL : "cover-letter", 0, rev->total))
 		return;
 
-	origin_sha1 = sha1_to_hex(origin ? origin->object.sha1 : null_sha1);
 	head_sha1 = sha1_to_hex(head->object.sha1);
 
 	log_write_email_headers(rev, head_sha1, &subject_start, &extra_headers);
@@ -679,21 +682,19 @@ static void make_cover_letter(struct rev_info *rev,
 
 	strbuf_release(&sb);
 
+	shortlog_init(&log);
+	for (i = 0; i < nr; i++)
+		shortlog_add_commit(&log, list[i]);
+
+	shortlog_output(&log);
+
 	/*
-	 * We can only do diffstat with a unique reference point, and
-	 * log is a bit tricky, so just skip it.
+	 * We can only do diffstat with a unique reference point
 	 */
 	if (!origin)
 		return;
 
-	argv[0] = "shortlog";
-	argv[1] = head_sha1;
-	argv[2] = "--not";
-	argv[3] = origin_sha1;
-	argv[4] = "--";
-	argv[5] = NULL;
-	fflush(stdout);
-	run_command_v_opt(argv, RUN_GIT_CMD);
+	origin_sha1 = sha1_to_hex(origin->object.sha1);
 
 	argv[0] = "diff";
 	argv[1] = "--stat";
@@ -746,6 +747,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	int ignore_if_in_upstream = 0;
 	int thread = 0;
 	int cover_letter = 0;
+	int boundary_count = 0;
 	struct commit *origin = NULL, *head = NULL;
 	const char *in_reply_to = NULL;
 	struct patch_ids ids;
@@ -936,19 +938,12 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	}
 	if (cover_letter) {
 		/* remember the range */
-		int negative_count = 0;
 		int i;
 		for (i = 0; i < rev.pending.nr; i++) {
 			struct object *o = rev.pending.objects[i].item;
-			if (o->flags & UNINTERESTING) {
-				origin = (struct commit *)o;
-				negative_count++;
-			} else
+			if (!(o->flags & UNINTERESTING))
 				head = (struct commit *)o;
 		}
-		/* Multiple origins don't work for diffstat. */
-		if (negative_count > 1)
-			origin = NULL;
 		/* We can't generate a cover letter without any patches */
 		if (!head)
 			return 0;
@@ -962,7 +957,15 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 
 	if (prepare_revision_walk(&rev))
 		die("revision walk setup failed");
+	rev.boundary = 1;
 	while ((commit = get_revision(&rev)) != NULL) {
+		if (commit->object.flags & BOUNDARY) {
+			fprintf(stderr, "Boundary %s\n", sha1_to_hex(commit->object.sha1));
+			boundary_count++;
+			origin = (boundary_count == 1) ? commit : NULL;
+			continue;
+		}
+
 		/* ignore merges */
 		if (commit->parents && commit->parents->next)
 			continue;
@@ -986,7 +989,7 @@ 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, numbered, numbered_files,
-				  origin, head);
+				  origin, nr, list, head);
 		total++;
 		start_number--;
 	}
-- 
1.5.4.3.330.g53ab.dirty
Junio C Hamano· Feb 26, 2008, 00:07 UTC · re: Daniel Barkalow · lore

Re: [PATCH 2/2] Improve collection of information for format-patch --cover-letter

Daniel Barkalow <barkalow@iabervon.org> writes:
> ... Note that the single boundary test is empirical, not 
> theoretical; even a -2 limiting condition will give a diffstat if there's 
> only one boundary commit in this particular case.
It is unclear what "a -2 limiting condition" means here...
Show 10 quoted lines
> @@ -679,21 +682,19 @@ static void make_cover_letter(struct rev_info *rev,
>  
>  	strbuf_release(&sb);
>  
> +	shortlog_init(&log);
> +	for (i = 0; i < nr; i++)
> +		shortlog_add_commit(&log, list[i]);
> +
> +	shortlog_output(&log);
> +
Nice ;-)
Show 8 quoted lines
> @@ -962,7 +957,15 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>  
>  	if (prepare_revision_walk(&rev))
>  		die("revision walk setup failed");
> +	rev.boundary = 1;
>  	while ((commit = get_revision(&rev)) != NULL) {
> +		if (commit->object.flags & BOUNDARY) {
> +			fprintf(stderr, "Boundary %s\n", sha1_to_hex(commit->object.sha1));
Leftover debugging?
Daniel Barkalow· Feb 26, 2008, 16:27 UTC · re: Junio C Hamano · lore

Re: [PATCH 2/2] Improve collection of information for format-patch --cover-letter

On Mon, 25 Feb 2008, Junio C Hamano wrote:
Show 7 quoted lines
> Daniel Barkalow <barkalow@iabervon.org> writes:
> 
> > ... Note that the single boundary test is empirical, not 
> > theoretical; even a -2 limiting condition will give a diffstat if there's 
> > only one boundary commit in this particular case.
> 
> It is unclear what "a -2 limiting condition" means here...
Like "git format-patch -2". What is that actually called?
Show 10 quoted lines
> > @@ -962,7 +957,15 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
> >  
> >  	if (prepare_revision_walk(&rev))
> >  		die("revision walk setup failed");
> > +	rev.boundary = 1;
> >  	while ((commit = get_revision(&rev)) != NULL) {
> > +		if (commit->object.flags & BOUNDARY) {
> > +			fprintf(stderr, "Boundary %s\n", sha1_to_hex(commit->object.sha1));
> 
> Leftover debugging?

That last line, yes. I need a checkpatch for new uses of stderr or something.

	-Daniel
*This .sig left intentionally blank*
Junio C Hamano· Feb 26, 2008, 18:46 UTC · re: Daniel Barkalow · lore

Re: [PATCH 2/2] Improve collection of information for format-patch --cover-letter

Daniel Barkalow <barkalow@iabervon.org> writes:
Show 11 quoted lines
> On Mon, 25 Feb 2008, Junio C Hamano wrote:
>
>> Daniel Barkalow <barkalow@iabervon.org> writes:
>> 
>> > ... Note that the single boundary test is empirical, not 
>> > theoretical; even a -2 limiting condition will give a diffstat if there's 
>> > only one boundary commit in this particular case.
>> 
>> It is unclear what "a -2 limiting condition" means here...
>
> Like "git format-patch -2". What is that actually called?

"max count"? So you wanted to say that the new code sometimes figures out (with --boundary) what to compare against even when the user does not explicitly specify where the range begins from the command line. Thanks for a clarification.

Junio C Hamano· Feb 28, 2008, 06:18 UTC · re: Junio C Hamano · lore

[PATCH] format-patch: remove a leftover debugging message

Sorry, Daniel ;-)
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin-log.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)
Show changes to builtin-log.c +0 −1
diff --git a/builtin-log.c b/builtin-log.c
index 3209ea5..836b61e 100644
--- a/builtin-log.c
+++ b/builtin-log.c
@@ -960,7 +960,6 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	rev.boundary = 1;
 	while ((commit = get_revision(&rev)) != NULL) {
 		if (commit->object.flags & BOUNDARY) {
-			fprintf(stderr, "Boundary %s\n", sha1_to_hex(commit->object.sha1));
 			boundary_count++;
 			origin = (boundary_count == 1) ? commit : NULL;
 			continue;
-- 
1.5.4.3.393.g55409
Daniel Barkalow· Feb 28, 2008, 15:20 UTC · re: Junio C Hamano · lore

Re: [PATCH] format-patch: remove a leftover debugging message

On Wed, 27 Feb 2008, Junio C Hamano wrote:
> Sorry, Daniel ;-)
> 
> Signed-off-by: Junio C Hamano <gitster@pobox.com>

I wrote this patch, added it to that topic locally, and then forgot I hadn't sent it. (But my version had the advantage of mentioning which commit it should be squashed into when going to master.) Then I was confused to see it in my incoming email instead of in my outgoing email (Why are you forwarding me my own patch? Oh, because I didn't send it.)

Acked-by: Daniel Barkalow <barkalow@iabervon.org>
Junio C Hamano· Feb 26, 2008, 09:19 UTC · re: Daniel Barkalow · lore

Re: [PATCH 2/2] Improve collection of information for format-patch --cover-letter

This passes all the tests fine isolated, but when merged to 'next' or 'pu' the result seem to barf at test #105 in t4013, complaining that it does not know about --cover-letter.

Sorry, but I've run out of time tonight, so 'pu' tonight has this at the tip.

Junio C Hamano· Feb 26, 2008, 09:24 UTC · re: Junio C Hamano · lore

Re: [PATCH 2/2] Improve collection of information for format-patch --cover-letter

Junio C Hamano <gitster@pobox.com> writes:
Show 6 quoted lines
> This passes all the tests fine isolated, but when merged to
> 'next' or 'pu' the result seem to barf at test #105 in t4013,
> complaining that it does not know about --cover-letter.
>
> Sorry, but I've run out of time tonight, so 'pu' tonight has
> this at the tip.
False alarm.  It seems to be Ok.

← back to recent threads