On Sun, Apr 7, 2013 at 3:14 AM, 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. >> >> Signed-off-by: Felipe Contreras <felipe.contreras@xxxxxxxxx> >> --- >> Documentation/config.txt | 6 ++++++ >> Documentation/git-format-patch.txt | 5 +++-- >> builtin/log.c | 22 ++++++++++++++++------ >> 3 files changed, 25 insertions(+), 8 deletions(-) >> >> diff --git a/Documentation/config.txt b/Documentation/config.txt >> index c8e2178..c10195c 100644 >> --- a/Documentation/config.txt >> +++ b/Documentation/config.txt >> @@ -1092,6 +1092,12 @@ format.signoff:: >> the rights to submit this work under the same open source license. >> Please see the 'SubmittingPatches' document for further discussion. >> >> +format.cover-letter:: > > I thought I saw somebody already pointed out that this was wrong. Yeah, I forgot. >> + Allows to configure the --cover-letter option of format-patch by >> + default. >> + In addition, you can set it to 'auto' to automatically >> + determine based on the number of patches (generate if there's more than >> + one). > > I find this somewhat a strange phrasing. It _sets_ the default for > the cover-letter option, either to true or false or auto, so there > is no "in addition" about it. There's no --cover-letter=auto. > Perhaps > > format.coverLetter:: > Setting this variable to `true` (or `false`) tells the > `format-patch` command to pretend the `--cover-letter` (or > `--no-cover-letter`) was given from the command line. When > set to `auto`, `format-patch` assumes `--cover-letter` if > and only if it is working on more than one commit. I prefer the wording suggested by Ramkumar with a minor modification: A boolean that controls whether to generate a cover-letter when format-patch is invoked, but in addition can be set to "auto", to generate a cover-letter only when generating more than one patch. > or something? > > It also is somewhat strange why the command line argument does not > accept `--cover-letter=auto` option. In general, a configuration > variable should not be more flexible mechanism than what the command > line could give the user. As I already said, I think that would be > a better longer-term default, and that makes it even more stand-out. Users know how to count, if the default is no cover-letter, then doing --cover-letter=auto is basically --cover-letter, unless they don't know how to count and thought --cover-letter=auto would do something. The same is true is they configured coverletter=true. I don't see any value in doing that. > This part is nicely done, thanks to the previous two steps. You > might even want to remove the call to the find_branch_name() helper > function from here, and instead call it from make_cover_letter() > when branch_name is not yet there. True. We can also move the 'head' variable inside make_cover_letter() as well. > We would need tests to make sure future changes will not break this. Sigh. All right. -- 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