On Sat, Apr 6, 2013 at 9:32 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Felipe Contreras <felipe.contreras@xxxxxxxxx> writes:
>
>> Also, add a new option: 'auto', so if there's more than one patch, the
>> cover letter is generated, otherwise it's not.
>
> Very sensible goal.
>
>> This has the slight disadvantage that a piece of code will always be run
>> even if the user doesn't want a cover letter, and thus waste a few
>> cycles.
>
> I am not sure what overhead you are referring to.
>
> We need to count how many we are emitting with or without the cover
> letter, and count is stored in "total". Even when the user said
> "auto" (which I personally think should become the default in the
> longer term, but that is a separate issue), we shouldn't have to
> spend any extra cost if you moved the code that does anything heavy
> for cover letter generation after we know what that "total" is, no?
>
> I think the reason you did not move the "find the branch for use in
> the cover letter" code down was because it uses the rev->pending
> interface, which you cannot read out of after you count the commits,
> but that logic to use rev->pending predates the introduction of a
> more modern rev->cmdline mechanism, which is already used by the
> find_branch_name() function in the same codepath, and it is not
> clobbered by prepare_revision_walk().
>
> So perhaps by moving that code down after we know what value "total"
> has, and rewriting "what was the positive commit the user gave us"
> logic using rev->cmdline, you do not have to do unnecessary work at
> all.
I did try to do that, but somehow missed a few possibilities that I
see now. Perhaps something like this (after this, it's possible to
move the find_branch_name() code):
diff --git a/builtin/log.c b/builtin/log.c
index ed89c10..32add91 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1061,15 +1061,6 @@ static char *find_branch_name(struct rev_info *rev)
if (0 <= positive) {
ref = rev->cmdline.rev[positive].name;
tip_sha1 = rev->cmdline.rev[positive].item->sha1;
- } else if (!rev->cmdline.nr && rev->pending.nr == 1 &&
- !strcmp(rev->pending.objects[0].name, "HEAD")) {
- /*
- * No actual ref from command line, but "HEAD" from
- * rev->def was added in setup_revisions()
- * e.g. format-patch --cover-letter -12
- */
- ref = "HEAD";
- tip_sha1 = rev->pending.objects[0].item->sha1;
} else {
return NULL;
}
@@ -1299,28 +1290,41 @@ int cmd_format_patch(int argc, const char
**argv, const char *prefix)
}
if (rev.pending.nr == 1) {
+ unsigned char sha1[20];
+ const char *ref;
+ int check_head = 0;
+
if (rev.max_count < 0 && !rev.show_root_diff) {
/*
* This is traditional behaviour of "git format-patch
* origin" that prepares what the origin side still
* does not have.
*/
- unsigned char sha1[20];
- const char *ref;
-
rev.pending.objects[0].item->flags |= UNINTERESTING;
add_head_to_pending(&rev);
- ref = resolve_ref_unsafe("HEAD", sha1, 1, NULL);
- if (ref && !prefixcmp(ref, "refs/heads/"))
- branch_name = xstrdup(ref + strlen("refs/heads/"));
- else
- branch_name = xstrdup(""); /* no branch */
+ check_head = 1;
}
/*
* Otherwise, it is "format-patch -22 HEAD", and/or
* "format-patch --root HEAD". The user wants
* get_revision() to do the usual traversal.
*/
+
+ if (!strcmp(rev.pending.objects[0].name, "HEAD"))
+ /*
+ * No actual ref from command line, but "HEAD" from
+ * rev->def was added in setup_revisions()
+ * e.g. format-patch --cover-letter -12
+ */
+ check_head = 1;
+
+ if (check_head) {
+ ref = resolve_ref_unsafe("HEAD", sha1, 1, NULL);
+ if (ref && !prefixcmp(ref, "refs/heads/"))
+ branch_name = xstrdup(ref + strlen("refs/heads/"));
+ else
+ branch_name = xstrdup(""); /* no branch */
+ }
}
/*
@@ -1329,24 +1333,6 @@ int cmd_format_patch(int argc, const char
**argv, const char *prefix)
*/
rev.show_root_diff = 1;
- /*
- * NEEDSWORK:randomly pick one positive commit to show
- * diffstat; this is often the tip and the command
- * happens to do the right thing in most cases, but a
- * complex command like "--cover-letter a b c ^bottom"
- * picks "c" and shows diffstat between bottom..c
- * which may not match what the series represents at
- * all and totally broken.
- */
- for (i = 0; i < rev.pending.nr; i++) {
- struct object *o = rev.pending.objects[i].item;
- if (!(o->flags & UNINTERESTING))
- head = (struct commit *)o;
- }
- /* There is nothing to show; it is not an error, though. */
- if (!head)
- return 0;
-
if (!branch_name)
branch_name = find_branch_name(&rev);
@@ -1381,6 +1367,16 @@ int cmd_format_patch(int argc, const char
**argv, const char *prefix)
list = xrealloc(list, nr * sizeof(list[0]));
list[nr - 1] = commit;
}
+ /*
+ * NEEDSWORK:randomly pick one positive commit to show
+ * diffstat; this is often the tip and the command
+ * happens to do the right thing in most cases, but a
+ * complex command like "--cover-letter a b c ^bottom"
+ * picks "c" and shows diffstat between bottom..c
+ * which may not match what the series represents at
+ * all and totally broken.
+ */
+ head = list[0];
total = nr;
if (!keep_subject && auto_number && total > 1)
numbered = 1;
--
Felipe Contreras
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html