{"thread":{"id":"33404","subject":"[PATCH v2 0/2] send-email: couple of improvements","startedAt":"2013-04-06T09:03:30Z","lastAt":"2013-04-07T06:54:30Z","messageCount":12,"participants":["Felipe Contreras","Ramkumar Ramachandra","Matthieu Moy","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":2},"messages":[{"id":"213325","messageId":"1365239012-15079-1-git-send-email-felipe.contreras@gmail.com","threadId":"33404","inReplyTo":null,"subject":"[PATCH v2 0/2] send-email: couple of improvements","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-04-06T09:03:30Z","receivedAt":"2013-04-06T09:03:30Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Hi,\n\nFirst patch was already sent, I just added the --no-annotate option. Second one\nis new, it adds a configuration for --cover-letter, so it can automatically\ndetermine when to generated: 1 patch, no cover, otherwise there is.\n\nFelipe Contreras (2):\n  send-email: make annotate configurable\n  format-patch: add format.cover-letter configuration\n\n Documentation/config.txt           |  7 +++++\n Documentation/git-format-patch.txt |  5 ++--\n Documentation/git-send-email.txt   |  5 ++--\n builtin/log.c                      | 55 +++++++++++++++++++++++---------------\n git-send-email.perl                | 12 ++++++---\n 5 files changed, 55 insertions(+), 29 deletions(-)\n\n-- \n1.8.2\n"},{"id":"213328","messageId":"1365239012-15079-2-git-send-email-felipe.contreras@gmail.com","threadId":"33404","inReplyTo":"1365239012-15079-1-git-send-email-felipe.contreras@gmail.com","subject":"[PATCH v2 1/2] send-email: make annotate configurable","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-04-06T09:03:31Z","receivedAt":"2013-04-06T09:03:31Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Some people always do --annotate, lets not force them to always type\nthat.\n\nSigned-off-by: Felipe Contreras <felipe.contreras@gmail.com>\n---\n Documentation/config.txt         |  1 +\n Documentation/git-send-email.txt |  5 +++--\n git-send-email.perl              | 12 +++++++++---\n 3 files changed, 13 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex bbba728..c8e2178 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1994,6 +1994,7 @@ sendemail.<identity>.*::\n \n sendemail.aliasesfile::\n sendemail.aliasfiletype::\n+sendemail.annotate::\n sendemail.bcc::\n sendemail.cc::\n sendemail.cccmd::\ndiff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt\nindex 44a1f7c..2facc18 100644\n--- a/Documentation/git-send-email.txt\n+++ b/Documentation/git-send-email.txt\n@@ -45,8 +45,9 @@ Composing\n ~~~~~~~~~\n \n --annotate::\n-\tReview and edit each patch you're about to send. See the\n-\tCONFIGURATION section for 'sendemail.multiedit'.\n+\tReview and edit each patch you're about to send. Default is the value\n+\tof 'sendemail.annotate'. See the CONFIGURATION section for\n+\t'sendemail.multiedit'.\n \n --bcc=<address>::\n \tSpecify a \"Bcc:\" value for each email. Default is the value of\ndiff --git a/git-send-email.perl b/git-send-email.perl\nindex be809e5..e7fe9fb 100755\n--- a/git-send-email.perl\n+++ b/git-send-email.perl\n@@ -54,7 +54,7 @@ git send-email [options] <file | directory | rev-list options >\n     --[no-]bcc              <str>  * Email Bcc:\n     --subject               <str>  * Email \"Subject:\"\n     --in-reply-to           <str>  * Email \"In-Reply-To:\"\n-    --annotate                     * Review each patch that will be sent in an editor.\n+    --[no-]annotate                * Review each patch that will be sent in an editor.\n     --compose                      * Open an editor for introduction.\n     --compose-encoding      <str>  * Encoding to assume for introduction.\n     --8bit-encoding         <str>  * Encoding to assume 8bit mails if undeclared\n@@ -143,7 +143,7 @@ my $auth;\n # Variables we fill in automatically, or via prompting:\n my (@to,$no_to,@initial_to,@cc,$no_cc,@initial_cc,@bcclist,$no_bcc,@xh,\n \t$initial_reply_to,$initial_subject,@files,\n-\t$author,$sender,$smtp_authpass,$annotate,$compose,$time);\n+\t$author,$sender,$smtp_authpass,$annotate,$no_annotate,$compose,$time);\n \n my $envelope_sender;\n \n@@ -212,7 +212,8 @@ my %config_bool_settings = (\n     \"signedoffbycc\" => [\\$signed_off_by_cc, undef],\n     \"signedoffcc\" => [\\$signed_off_by_cc, undef],      # Deprecated\n     \"validate\" => [\\$validate, 1],\n-    \"multiedit\" => [\\$multiedit, undef]\n+    \"multiedit\" => [\\$multiedit, undef],\n+    \"annotate\" => [\\$annotate, undef]\n );\n \n my %config_settings = (\n@@ -305,6 +306,7 @@ my $rc = GetOptions(\"h\" => \\$help,\n \t\t    \"smtp-domain:s\" => \\$smtp_domain,\n \t\t    \"identity=s\" => \\$identity,\n \t\t    \"annotate\" => \\$annotate,\n+\t\t    \"no-annotate\" => \\$no_annotate,\n \t\t    \"compose\" => \\$compose,\n \t\t    \"quiet\" => \\$quiet,\n \t\t    \"cc-cmd=s\" => \\$cc_cmd,\n@@ -389,6 +391,10 @@ foreach my $setting (values %config_bool_settings) {\n \t${$setting->[0]} = $setting->[1] unless (defined (${$setting->[0]}));\n }\n \n+if ($no_annotate) {\n+\t$annotate = 0;\n+}\n+\n # 'default' encryption is none -- this only prevents a warning\n $smtp_encryption = '' unless (defined $smtp_encryption);\n \n-- \n1.8.2\n"},{"id":"213331","messageId":"1365239012-15079-3-git-send-email-felipe.contreras@gmail.com","threadId":"33404","inReplyTo":"1365239012-15079-1-git-send-email-felipe.contreras@gmail.com","subject":"[PATCH v2 2/2] format-patch: add format.cover-letter configuration","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-04-06T09:03:32Z","receivedAt":"2013-04-06T09:03:32Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Also, add a new option: 'auto', so if there's more than one patch, the\ncover letter is generated, otherwise it's not.\n\nThis has the slight disadvantage that a piece of code will always be run\neven if the user doesn't want a cover letter, and thus waste a few\ncycles. But the convenience is well worth it.\n\nSigned-off-by: Felipe Contreras <felipe.contreras@gmail.com>\n---\n Documentation/config.txt           |  6 +++++\n Documentation/git-format-patch.txt |  5 ++--\n builtin/log.c                      | 55 +++++++++++++++++++++++---------------\n 3 files changed, 42 insertions(+), 24 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex c8e2178..c10195c 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1092,6 +1092,12 @@ format.signoff::\n     the rights to submit this work under the same open source license.\n     Please see the 'SubmittingPatches' document for further discussion.\n \n+format.cover-letter::\n+\tAllows to configure the --cover-letter option of format-patch by\n+\tdefault. In addition, you can set it to 'auto' to automatically\n+\tdetermine based on the number of patches (generate if there's more than\n+\tone).\n+\n filter.<driver>.clean::\n \tThe command which is used to convert the content of a worktree\n \tfile to a blob upon checkin.  See linkgit:gitattributes[5] for\ndiff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt\nindex 3a62f50..e1f5730 100644\n--- a/Documentation/git-format-patch.txt\n+++ b/Documentation/git-format-patch.txt\n@@ -20,7 +20,7 @@ SYNOPSIS\n \t\t   [--ignore-if-in-upstream]\n \t\t   [--subject-prefix=Subject-Prefix] [(--reroll-count|-v) <n>]\n \t\t   [--to=<email>] [--cc=<email>]\n-\t\t   [--cover-letter] [--quiet] [--notes[=<ref>]]\n+\t\t   [--[no-]cover-letter] [--quiet] [--notes[=<ref>]]\n \t\t   [<common diff options>]\n \t\t   [ <since> | <revision range> ]\n \n@@ -195,7 +195,7 @@ will want to ensure that threading is disabled for `git send-email`.\n \t`Cc:`, and custom) headers added so far from config or command\n \tline.\n \n---cover-letter::\n+--[no-]cover-letter::\n \tIn addition to the patches, generate a cover letter file\n \tcontaining the shortlog and the overall diffstat.  You can\n \tfill in a description in the file before sending it out.\n@@ -260,6 +260,7 @@ attachments, and sign off patches with configuration variables.\n \tcc = <email>\n \tattach [ = mime-boundary-string ]\n \tsignoff = true\n+\tcover-letter = auto\n ------------\n \n \ndiff --git a/builtin/log.c b/builtin/log.c\nindex 8f0b2e8..ed89c10 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -618,6 +618,7 @@ static void add_header(const char *value)\n #define THREAD_DEEP 2\n static int thread;\n static int do_signoff;\n+static int cover_letter;\n static const char *signature = git_version_string;\n \n static int git_format_config(const char *var, const char *value, void *cb)\n@@ -680,6 +681,17 @@ static int git_format_config(const char *var, const char *value, void *cb)\n \t}\n \tif (!strcmp(var, \"format.signature\"))\n \t\treturn git_config_string(&signature, var, value);\n+\tif (!strcmp(var, \"format.cover-letter\")) {\n+\t\tif (cover_letter != -1)\n+\t\t\t/* user overrode it */\n+\t\t\treturn 0;\n+\t\tif (value && !strcasecmp(value, \"auto\")) {\n+\t\t\tcover_letter = -1;\n+\t\t\treturn 0;\n+\t\t}\n+\t\tcover_letter = git_config_bool(var, value);\n+\t\treturn 0;\n+\t}\n \n \treturn git_log_config(var, value, cb);\n }\n@@ -1080,7 +1092,6 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \tint start_number = -1;\n \tint just_numbers = 0;\n \tint ignore_if_in_upstream = 0;\n-\tint cover_letter = 0;\n \tint boundary_count = 0;\n \tint no_binary_diff = 0;\n \tstruct commit *origin = NULL, *head = NULL;\n@@ -1318,28 +1329,26 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \t */\n \trev.show_root_diff = 1;\n \n-\tif (cover_letter) {\n-\t\t/*\n-\t\t * NEEDSWORK:randomly pick one positive commit to show\n-\t\t * diffstat; this is often the tip and the command\n-\t\t * happens to do the right thing in most cases, but a\n-\t\t * complex command like \"--cover-letter a b c ^bottom\"\n-\t\t * picks \"c\" and shows diffstat between bottom..c\n-\t\t * which may not match what the series represents at\n-\t\t * all and totally broken.\n-\t\t */\n-\t\tint i;\n-\t\tfor (i = 0; i < rev.pending.nr; i++) {\n-\t\t\tstruct object *o = rev.pending.objects[i].item;\n-\t\t\tif (!(o->flags & UNINTERESTING))\n-\t\t\t\thead = (struct commit *)o;\n-\t\t}\n-\t\t/* There is nothing to show; it is not an error, though. */\n-\t\tif (!head)\n-\t\t\treturn 0;\n-\t\tif (!branch_name)\n-\t\t\tbranch_name = find_branch_name(&rev);\n+\t/*\n+\t * NEEDSWORK:randomly pick one positive commit to show\n+\t * diffstat; this is often the tip and the command\n+\t * happens to do the right thing in most cases, but a\n+\t * complex command like \"--cover-letter a b c ^bottom\"\n+\t * picks \"c\" and shows diffstat between bottom..c\n+\t * which may not match what the series represents at\n+\t * all and totally broken.\n+\t */\n+\tfor (i = 0; i < rev.pending.nr; i++) {\n+\t\tstruct object *o = rev.pending.objects[i].item;\n+\t\tif (!(o->flags & UNINTERESTING))\n+\t\t\thead = (struct commit *)o;\n \t}\n+\t/* There is nothing to show; it is not an error, though. */\n+\tif (!head)\n+\t\treturn 0;\n+\n+\tif (!branch_name)\n+\t\tbranch_name = find_branch_name(&rev);\n \n \tif (ignore_if_in_upstream) {\n \t\t/* Don't say anything if head and upstream are the same. */\n@@ -1377,6 +1386,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \t\tnumbered = 1;\n \tif (numbered)\n \t\trev.total = total + start_number - 1;\n+\tif (cover_letter == -1)\n+\t\tcover_letter = (total > 1);\n \tif (in_reply_to || thread || cover_letter)\n \t\trev.ref_message_ids = xcalloc(1, sizeof(struct string_list));\n \tif (in_reply_to) {\n-- \n1.8.2\n"},{"id":"213339","messageId":"CALkWK0kMTt6wDv6sUVe9CbX-WSx=XH+PF6eQ6oXYFjcp_Yi+aQ@mail.gmail.com","threadId":"33404","inReplyTo":"1365239012-15079-3-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH v2 2/2] format-patch: add format.cover-letter configuration","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-04-06T12:43:58Z","receivedAt":"2013-04-06T12:43:58Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Felipe Contreras wrote:\n> Also, add a new option: 'auto', so if there's more than one patch, the\n> cover letter is generated, otherwise it's not.\n\nAwesome!  I wanted to fix this myself, but got sidetracked with the\nwhole submodules thing.\n\n> +format.cover-letter::\n> +       Allows to configure the --cover-letter option of format-patch by\n> +       default. In addition, you can set it to 'auto' to automatically\n> +       determine based on the number of patches (generate if there's more than\n> +       one).\n> +\n\nPerhaps you can clarify this: Controls whether to generate a\ncover-letter when format-patch is invoked.  Can be true, false, or\nauto.  \"auto\" generates a cover-letter only when generating more than\none patch.\n\nThanks.\n"},{"id":"213348","messageId":"CAMP44s0Zzdb-1A0ej5Ly_501sv0OOOrHi86GWT9NYkndbiggeg@mail.gmail.com","threadId":"33404","inReplyTo":"CALkWK0kMTt6wDv6sUVe9CbX-WSx=XH+PF6eQ6oXYFjcp_Yi+aQ@mail.gmail.com","subject":"Re: [PATCH v2 2/2] format-patch: add format.cover-letter configuration","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-04-06T16:02:32Z","receivedAt":"2013-04-06T16:02:32Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Sat, Apr 6, 2013 at 6:43 AM, Ramkumar Ramachandra <artagnon@gmail.com> wrote:\n> Felipe Contreras wrote:\n>> Also, add a new option: 'auto', so if there's more than one patch, the\n>> cover letter is generated, otherwise it's not.\n>\n> Awesome!  I wanted to fix this myself, but got sidetracked with the\n> whole submodules thing.\n>\n>> +format.cover-letter::\n>> +       Allows to configure the --cover-letter option of format-patch by\n>> +       default. In addition, you can set it to 'auto' to automatically\n>> +       determine based on the number of patches (generate if there's more than\n>> +       one).\n>> +\n>\n> Perhaps you can clarify this: Controls whether to generate a\n> cover-letter when format-patch is invoked.  Can be true, false, or\n> auto.  \"auto\" generates a cover-letter only when generating more than\n> one patch.\n\nThat's good, but I believe if we say it's a boolean, true and false\nare implied, and then we have an extra \"auto\".\n\n-- \nFelipe Contreras\n"},{"id":"213356","messageId":"vpq8v4vimhn.fsf@grenoble-inp.fr","threadId":"33404","inReplyTo":"CAMP44s0Zzdb-1A0ej5Ly_501sv0OOOrHi86GWT9NYkndbiggeg@mail.gmail.com","subject":"Re: [PATCH v2 2/2] format-patch: add format.cover-letter configuration","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2013-04-06T17:43:00Z","receivedAt":"2013-04-06T17:43:00Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Felipe Contreras <felipe.contreras@gmail.com> writes:\n\n>> Perhaps you can clarify this: Controls whether to generate a\n>> cover-letter when format-patch is invoked.  Can be true, false, or\n>> auto.  \"auto\" generates a cover-letter only when generating more than\n>> one patch.\n>\n> That's good, but I believe if we say it's a boolean, true and false\n> are implied, and then we have an extra \"auto\".\n\nAt the moment, we don't say it's Boolean. I'm not sure it's a good idea\nto say Boolean here, since it would also imply that there's no value\nother than true and false.\n\nAlso, you should mention the default value. Something like \"Set to true\nto always generate a cover letter. Defaults to false.\" or so.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"213362","messageId":"vpq4nfjimg3.fsf@grenoble-inp.fr","threadId":"33404","inReplyTo":"1365239012-15079-3-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH v2 2/2] format-patch: add format.cover-letter configuration","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2013-04-06T17:43:56Z","receivedAt":"2013-04-06T17:43:56Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Felipe Contreras <felipe.contreras@gmail.com> writes:\n\n> +format.cover-letter::\n\nWe normally use camelCase, so format.coverLetter, not cover-letter.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"213357","messageId":"vpqwqsfh7qi.fsf@grenoble-inp.fr","threadId":"33404","inReplyTo":"1365239012-15079-1-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH v2 0/2] send-email: couple of improvements","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2013-04-06T17:47:01Z","receivedAt":"2013-04-06T17:47:01Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Felipe Contreras <felipe.contreras@gmail.com> writes:\n\n> First patch was already sent, I just added the --no-annotate option. Second one\n> is new, it adds a configuration for --cover-letter, so it can automatically\n> determine when to generated: 1 patch, no cover, otherwise there is.\n\nVery good. I unconditionnally run --annotate, but do that with an alias.\nAnd indeed, I use --cover-letter almost if and only if I have several\npatches to send. These patches do exactly what I need!\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"213372","messageId":"7vr4in818e.fsf@alter.siamese.dyndns.org","threadId":"33404","inReplyTo":"1365239012-15079-2-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH v2 1/2] send-email: make annotate configurable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-07T03:32:17Z","receivedAt":"2013-04-07T03:32:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras <felipe.contreras@gmail.com> writes:\n\n> @@ -305,6 +306,7 @@ my $rc = GetOptions(\"h\" => \\$help,\n>  \t\t    \"smtp-domain:s\" => \\$smtp_domain,\n>  \t\t    \"identity=s\" => \\$identity,\n>  \t\t    \"annotate\" => \\$annotate,\n> +\t\t    \"no-annotate\" => \\$no_annotate,\n\nWouldn't it be much better to let GetOptions know that --no-annotate\nis allowed by saying\n\n\t\"annotate!\" => \\$annotate\n\nwhich also let us lose an extra $no_annotate variable?\n"},{"id":"213374","messageId":"7vk3of817v.fsf@alter.siamese.dyndns.org","threadId":"33404","inReplyTo":"1365239012-15079-3-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH v2 2/2] format-patch: add format.cover-letter configuration","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-07T03:32:36Z","receivedAt":"2013-04-07T03:32:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras <felipe.contreras@gmail.com> writes:\n\n> Also, add a new option: 'auto', so if there's more than one patch, the\n> cover letter is generated, otherwise it's not.\n\nVery sensible goal.\n\n> This has the slight disadvantage that a piece of code will always be run\n> even if the user doesn't want a cover letter, and thus waste a few\n> cycles.\n\nI am not sure what overhead you are referring to.\n\nWe need to count how many we are emitting with or without the cover\nletter, and count is stored in \"total\".  Even when the user said\n\"auto\" (which I personally think should become the default in the\nlonger term, but that is a separate issue), we shouldn't have to\nspend any extra cost if you moved the code that does anything heavy\nfor cover letter generation after we know what that \"total\" is, no?\n\nI think the reason you did not move the \"find the branch for use in\nthe cover letter\" code down was because it uses the rev->pending\ninterface, which you cannot read out of after you count the commits,\nbut that logic to use rev->pending predates the introduction of a\nmore modern rev->cmdline mechanism, which is already used by the\nfind_branch_name() function in the same codepath, and it is not\nclobbered by prepare_revision_walk().\n\nSo perhaps by moving that code down after we know what value \"total\"\nhas, and rewriting \"what was the positive commit the user gave us\"\nlogic using rev->cmdline, you do not have to do unnecessary work at\nall.\n"},{"id":"213375","messageId":"CAMP44s14Bd4U2w=wso6MAF3rQO_=m4o9R4Mr8z_b9w6BO5vcvg@mail.gmail.com","threadId":"33404","inReplyTo":"7vk3of817v.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 2/2] format-patch: add format.cover-letter configuration","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-04-07T05:46:51Z","receivedAt":"2013-04-07T05:46:51Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Sat, Apr 6, 2013 at 9:32 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Felipe Contreras <felipe.contreras@gmail.com> writes:\n>\n>> Also, add a new option: 'auto', so if there's more than one patch, the\n>> cover letter is generated, otherwise it's not.\n>\n> Very sensible goal.\n>\n>> This has the slight disadvantage that a piece of code will always be run\n>> even if the user doesn't want a cover letter, and thus waste a few\n>> cycles.\n>\n> I am not sure what overhead you are referring to.\n>\n> We need to count how many we are emitting with or without the cover\n> letter, and count is stored in \"total\".  Even when the user said\n> \"auto\" (which I personally think should become the default in the\n> longer term, but that is a separate issue), we shouldn't have to\n> spend any extra cost if you moved the code that does anything heavy\n> for cover letter generation after we know what that \"total\" is, no?\n>\n> I think the reason you did not move the \"find the branch for use in\n> the cover letter\" code down was because it uses the rev->pending\n> interface, which you cannot read out of after you count the commits,\n> but that logic to use rev->pending predates the introduction of a\n> more modern rev->cmdline mechanism, which is already used by the\n> find_branch_name() function in the same codepath, and it is not\n> clobbered by prepare_revision_walk().\n>\n> So perhaps by moving that code down after we know what value \"total\"\n> has, and rewriting \"what was the positive commit the user gave us\"\n> logic using rev->cmdline, you do not have to do unnecessary work at\n> all.\n\nI did try to do that, but somehow missed a few possibilities that I\nsee now. Perhaps something like this (after this, it's possible to\nmove the find_branch_name() code):\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex ed89c10..32add91 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -1061,15 +1061,6 @@ static char *find_branch_name(struct rev_info *rev)\n \tif (0 <= positive) {\n \t\tref = rev->cmdline.rev[positive].name;\n \t\ttip_sha1 = rev->cmdline.rev[positive].item->sha1;\n-\t} else if (!rev->cmdline.nr && rev->pending.nr == 1 &&\n-\t\t   !strcmp(rev->pending.objects[0].name, \"HEAD\")) {\n-\t\t/*\n-\t\t * No actual ref from command line, but \"HEAD\" from\n-\t\t * rev->def was added in setup_revisions()\n-\t\t * e.g. format-patch --cover-letter -12\n-\t\t */\n-\t\tref = \"HEAD\";\n-\t\ttip_sha1 = rev->pending.objects[0].item->sha1;\n \t} else {\n \t\treturn NULL;\n \t}\n@@ -1299,28 +1290,41 @@ int cmd_format_patch(int argc, const char\n**argv, const char *prefix)\n \t}\n\n \tif (rev.pending.nr == 1) {\n+\t\tunsigned char sha1[20];\n+\t\tconst char *ref;\n+\t\tint check_head = 0;\n+\n \t\tif (rev.max_count < 0 && !rev.show_root_diff) {\n \t\t\t/*\n \t\t\t * This is traditional behaviour of \"git format-patch\n \t\t\t * origin\" that prepares what the origin side still\n \t\t\t * does not have.\n \t\t\t */\n-\t\t\tunsigned char sha1[20];\n-\t\t\tconst char *ref;\n-\n \t\t\trev.pending.objects[0].item->flags |= UNINTERESTING;\n \t\t\tadd_head_to_pending(&rev);\n-\t\t\tref = resolve_ref_unsafe(\"HEAD\", sha1, 1, NULL);\n-\t\t\tif (ref && !prefixcmp(ref, \"refs/heads/\"))\n-\t\t\t\tbranch_name = xstrdup(ref + strlen(\"refs/heads/\"));\n-\t\t\telse\n-\t\t\t\tbranch_name = xstrdup(\"\"); /* no branch */\n+\t\t\tcheck_head = 1;\n \t\t}\n \t\t/*\n \t\t * Otherwise, it is \"format-patch -22 HEAD\", and/or\n \t\t * \"format-patch --root HEAD\".  The user wants\n \t\t * get_revision() to do the usual traversal.\n \t\t */\n+\n+\t\tif (!strcmp(rev.pending.objects[0].name, \"HEAD\"))\n+\t\t\t/*\n+\t\t\t * No actual ref from command line, but \"HEAD\" from\n+\t\t\t * rev->def was added in setup_revisions()\n+\t\t\t * e.g. format-patch --cover-letter -12\n+\t\t\t */\n+\t\t\tcheck_head = 1;\n+\n+\t\tif (check_head) {\n+\t\t\tref = resolve_ref_unsafe(\"HEAD\", sha1, 1, NULL);\n+\t\t\tif (ref && !prefixcmp(ref, \"refs/heads/\"))\n+\t\t\t\tbranch_name = xstrdup(ref + strlen(\"refs/heads/\"));\n+\t\t\telse\n+\t\t\t\tbranch_name = xstrdup(\"\"); /* no branch */\n+\t\t}\n \t}\n\n \t/*\n@@ -1329,24 +1333,6 @@ int cmd_format_patch(int argc, const char\n**argv, const char *prefix)\n \t */\n \trev.show_root_diff = 1;\n\n-\t/*\n-\t * NEEDSWORK:randomly pick one positive commit to show\n-\t * diffstat; this is often the tip and the command\n-\t * happens to do the right thing in most cases, but a\n-\t * complex command like \"--cover-letter a b c ^bottom\"\n-\t * picks \"c\" and shows diffstat between bottom..c\n-\t * which may not match what the series represents at\n-\t * all and totally broken.\n-\t */\n-\tfor (i = 0; i < rev.pending.nr; i++) {\n-\t\tstruct object *o = rev.pending.objects[i].item;\n-\t\tif (!(o->flags & UNINTERESTING))\n-\t\t\thead = (struct commit *)o;\n-\t}\n-\t/* There is nothing to show; it is not an error, though. */\n-\tif (!head)\n-\t\treturn 0;\n-\n \tif (!branch_name)\n \t\tbranch_name = find_branch_name(&rev);\n\n@@ -1381,6 +1367,16 @@ int cmd_format_patch(int argc, const char\n**argv, const char *prefix)\n \t\tlist = xrealloc(list, nr * sizeof(list[0]));\n \t\tlist[nr - 1] = commit;\n \t}\n+\t/*\n+\t * NEEDSWORK:randomly pick one positive commit to show\n+\t * diffstat; this is often the tip and the command\n+\t * happens to do the right thing in most cases, but a\n+\t * complex command like \"--cover-letter a b c ^bottom\"\n+\t * picks \"c\" and shows diffstat between bottom..c\n+\t * which may not match what the series represents at\n+\t * all and totally broken.\n+\t */\n+\thead = list[0];\n \ttotal = nr;\n \tif (!keep_subject && auto_number && total > 1)\n \t\tnumbered = 1;\n\n\n-- \nFelipe Contreras\n"},{"id":"213377","messageId":"CAMP44s1+XDQNctRe_N1u=cmF+e83bzu4LFK6GUaz-UKagvYLBg@mail.gmail.com","threadId":"33404","inReplyTo":"7vr4in818e.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 1/2] send-email: make annotate configurable","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-04-07T06:54:30Z","receivedAt":"2013-04-07T06:54:30Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Sat, Apr 6, 2013 at 9:32 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Felipe Contreras <felipe.contreras@gmail.com> writes:\n>\n>> @@ -305,6 +306,7 @@ my $rc = GetOptions(\"h\" => \\$help,\n>>                   \"smtp-domain:s\" => \\$smtp_domain,\n>>                   \"identity=s\" => \\$identity,\n>>                   \"annotate\" => \\$annotate,\n>> +                 \"no-annotate\" => \\$no_annotate,\n>\n> Wouldn't it be much better to let GetOptions know that --no-annotate\n> is allowed by saying\n>\n>         \"annotate!\" => \\$annotate\n>\n> which also let us lose an extra $no_annotate variable?\n\nRight. Will resend.\n\n-- \nFelipe Contreras\n"}]}