* [PATCH v2 0/4] add git-check-mailmap command @ 2013-07-11 14:55 Eric Sunshine 2013-07-11 14:55 ` [PATCH v2 1/4] builtin: " Eric Sunshine ` (3 more replies) 0 siblings, 4 replies; 18+ messages in thread From: Eric Sunshine @ 2013-07-11 14:55 UTC (permalink / raw) To: git; +Cc: Duy Nguyen, Antoine Pelisse, Eric Sunshine This is a re-roll of [1] which introduces git-check-mailmap. The primary motivation for this command is to expose git's stable, well-tested C-implementation of .mailmap functionality to scripts and porcelains, thus relieving them of the need to reimplement support themselves. The git-contacts [2] script (currently at es/contacts in 'pu') would be one such client. v2 removes the RFC status and addresses comments from reviewers [1]. Changes since v1: * Add Documentation/git-check-mailmap.txt. * Add --null alias for -z. * Use OPT_BOOL rather than deprecated OPT_BOOLEAN. * Simplify code which outputs normalized contact. * Settle on "stdout" as argument to maybe_flush_or_die(). * Eliminate diff noise from patch 4/4. [1]: http://thread.gmane.org/gmane.comp.version-control.git/230068/ [2]: http://thread.gmane.org/gmane.comp.version-control.git/229533/ Eric Sunshine (4): builtin: add git-check-mailmap command t4203: test check-mailmap command invocation t4203: test mailmap functionality directly rather than indirectly t4203: consolidate test-repository setup .gitignore | 1 + Documentation/git-check-mailmap.txt | 55 ++++++++ Makefile | 1 + builtin.h | 1 + builtin/check-mailmap.c | 69 ++++++++++ command-list.txt | 1 + contrib/completion/git-completion.bash | 1 + git.c | 1 + t/t4203-mailmap.sh | 234 +++++++++++++++++---------------- 9 files changed, 251 insertions(+), 113 deletions(-) create mode 100644 Documentation/git-check-mailmap.txt create mode 100644 builtin/check-mailmap.c -- 1.8.3.2 ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v2 1/4] builtin: add git-check-mailmap command 2013-07-11 14:55 [PATCH v2 0/4] add git-check-mailmap command Eric Sunshine @ 2013-07-11 14:55 ` Eric Sunshine 2013-07-11 19:04 ` Junio C Hamano 2013-07-11 14:55 ` [PATCH v2 2/4] t4203: test check-mailmap command invocation Eric Sunshine ` (2 subsequent siblings) 3 siblings, 1 reply; 18+ messages in thread From: Eric Sunshine @ 2013-07-11 14:55 UTC (permalink / raw) To: git; +Cc: Duy Nguyen, Antoine Pelisse, Eric Sunshine Introduce command check-mailmap, similar to check-attr and check-ignore, which allows direct testing of .mailmap configuration. As plumbing accessible to scripts and other porcelain, check-mailmap publishes the stable, well-tested .mailmap functionality employed by built-in Git commands. Consequently, script authors need not re-implement .mailmap functionality manually, thus avoiding potential quirks and behavioral differences. Signed-off-by: Eric Sunshine <sunshine@sunshineco.com> --- .gitignore | 1 + Documentation/git-check-mailmap.txt | 55 +++++++++++++++++++++++++++ Makefile | 1 + builtin.h | 1 + builtin/check-mailmap.c | 69 ++++++++++++++++++++++++++++++++++ command-list.txt | 1 + contrib/completion/git-completion.bash | 1 + git.c | 1 + 8 files changed, 130 insertions(+) create mode 100644 Documentation/git-check-mailmap.txt create mode 100644 builtin/check-mailmap.c diff --git a/.gitignore b/.gitignore index efa8db0..6b1fd1b 100644 --- a/.gitignore +++ b/.gitignore @@ -23,6 +23,7 @@ /git-cat-file /git-check-attr /git-check-ignore +/git-check-mailmap /git-check-ref-format /git-checkout /git-checkout-index diff --git a/Documentation/git-check-mailmap.txt b/Documentation/git-check-mailmap.txt new file mode 100644 index 0000000..3040bbc --- /dev/null +++ b/Documentation/git-check-mailmap.txt @@ -0,0 +1,55 @@ +git-check-mailmap(1) +==================== + +NAME +---- +git-check-mailmap - Show canonical mappings of names and email addresses + + +SYNOPSIS +-------- +[verse] +'git check-mailmap' [options] <contact>... + + +DESCRIPTION +----------- + +For each ``Name $$<email@address>$$'' or ``$$<email@address>$$'' provided on +the command-line or standard input (when using `--stdin`), prints a line with +the canonical contact information for that person according to the 'mailmap' +(see "Mapping Authors" below). If no mapping exists for the person, echoes the +provided contact information. + + +OPTIONS +------- +--stdin:: + Read contacts, one per line, from the standard input after exhausting + contacts provided on the command-line. + +-z:: +--null:: + Terminate each printed contact line with a null character `\0` rather + than a newline. + + +OUTPUT +------ + +For each contact, a single line is printed of the form ``Name +$$<email@address>$$'' if the name is provided or known to the 'mailmap'; or of +the form ``$$<email@address>$$'' if no name is provided or known. Each line is +terminated by a newline, or by a null character `\0` when `-z` or `--null` is +specified. + + +MAPPING AUTHORS +--------------- + +include::mailmap.txt[] + + +GIT +--- +Part of the linkgit:git[1] suite diff --git a/Makefile b/Makefile index 0600eb4..ef442eb 100644 --- a/Makefile +++ b/Makefile @@ -912,6 +912,7 @@ BUILTIN_OBJS += builtin/bundle.o BUILTIN_OBJS += builtin/cat-file.o BUILTIN_OBJS += builtin/check-attr.o BUILTIN_OBJS += builtin/check-ignore.o +BUILTIN_OBJS += builtin/check-mailmap.o BUILTIN_OBJS += builtin/check-ref-format.o BUILTIN_OBJS += builtin/checkout-index.o BUILTIN_OBJS += builtin/checkout.o diff --git a/builtin.h b/builtin.h index 1ed8edb..8afa2de 100644 --- a/builtin.h +++ b/builtin.h @@ -40,6 +40,7 @@ extern int cmd_checkout(int argc, const char **argv, const char *prefix); extern int cmd_checkout_index(int argc, const char **argv, const char *prefix); extern int cmd_check_attr(int argc, const char **argv, const char *prefix); extern int cmd_check_ignore(int argc, const char **argv, const char *prefix); +extern int cmd_check_mailmap(int argc, const char **argv, const char *prefix); extern int cmd_check_ref_format(int argc, const char **argv, const char *prefix); extern int cmd_cherry(int argc, const char **argv, const char *prefix); extern int cmd_cherry_pick(int argc, const char **argv, const char *prefix); diff --git a/builtin/check-mailmap.c b/builtin/check-mailmap.c new file mode 100644 index 0000000..c36a61c --- /dev/null +++ b/builtin/check-mailmap.c @@ -0,0 +1,69 @@ +#include "builtin.h" +#include "mailmap.h" +#include "parse-options.h" +#include "string-list.h" + +static int use_stdin; +static int null_out; +static const char * const check_mailmap_usage[] = { +N_("git check-mailmap [options] <contact>..."), +NULL +}; + +static const struct option check_mailmap_options[] = { + OPT_BOOL(0, "stdin", &use_stdin, N_("also read contacts from stdin")), + OPT_BOOL('z', "null", &null_out, N_("null-terminate output lines")), + OPT_END() +}; + +static void check_mailmap(struct string_list *mailmap, const char *contact) +{ + const char *name, *mail; + size_t namelen, maillen; + struct ident_split ident; + char term = null_out ? '\0' : '\n'; + + if (split_ident_line(&ident, contact, strlen(contact))) + die(_("unable to parse contact: %s"), contact); + + name = ident.name_begin; + namelen = ident.name_end - ident.name_begin; + mail = ident.mail_begin; + maillen = ident.mail_end - ident.mail_begin; + + map_user(mailmap, &mail, &maillen, &name, &namelen); + + if (namelen) + printf("%.*s ", (int)namelen, name); + printf("<%.*s>%c", (int)maillen, mail, term); +} + +int cmd_check_mailmap(int argc, const char **argv, const char *prefix) +{ + int i; + struct string_list mailmap = STRING_LIST_INIT_NODUP; + + git_config(git_default_config, NULL); + argc = parse_options(argc, argv, prefix, check_mailmap_options, + check_mailmap_usage, 0); + if (argc == 0 && !use_stdin) + die(_("no contacts specified")); + + read_mailmap(&mailmap, NULL); + + for (i = 0; i < argc; ++i) + check_mailmap(&mailmap, argv[i]); + maybe_flush_or_die(stdout, "stdout"); + + if (use_stdin) { + struct strbuf buf = STRBUF_INIT; + while (strbuf_getline(&buf, stdin, '\n') != EOF) { + check_mailmap(&mailmap, buf.buf); + maybe_flush_or_die(stdout, "stdout"); + } + strbuf_release(&buf); + } + + clear_mailmap(&mailmap); + return 0; +} diff --git a/command-list.txt b/command-list.txt index bf83303..08b04e2 100644 --- a/command-list.txt +++ b/command-list.txt @@ -13,6 +13,7 @@ git-bundle mainporcelain git-cat-file plumbinginterrogators git-check-attr purehelpers git-check-ignore purehelpers +git-check-mailmap purehelpers git-checkout mainporcelain common git-checkout-index plumbingmanipulators git-check-ref-format purehelpers diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash index ebc40d4..c118550 100644 --- a/contrib/completion/git-completion.bash +++ b/contrib/completion/git-completion.bash @@ -648,6 +648,7 @@ __git_list_porcelain_commands () cat-file) : plumbing;; check-attr) : plumbing;; check-ignore) : plumbing;; + check-mailmap) : plumbing;; check-ref-format) : plumbing;; checkout-index) : plumbing;; commit-tree) : plumbing;; diff --git a/git.c b/git.c index 4359086..02c3035 100644 --- a/git.c +++ b/git.c @@ -324,6 +324,7 @@ static void handle_internal_command(int argc, const char **argv) { "cat-file", cmd_cat_file, RUN_SETUP }, { "check-attr", cmd_check_attr, RUN_SETUP }, { "check-ignore", cmd_check_ignore, RUN_SETUP | NEED_WORK_TREE }, + { "check-mailmap", cmd_check_mailmap, RUN_SETUP }, { "check-ref-format", cmd_check_ref_format }, { "checkout", cmd_checkout, RUN_SETUP | NEED_WORK_TREE }, { "checkout-index", cmd_checkout_index, -- 1.8.3.2 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v2 1/4] builtin: add git-check-mailmap command 2013-07-11 14:55 ` [PATCH v2 1/4] builtin: " Eric Sunshine @ 2013-07-11 19:04 ` Junio C Hamano 2013-07-12 3:28 ` Eric Sunshine 0 siblings, 1 reply; 18+ messages in thread From: Junio C Hamano @ 2013-07-11 19:04 UTC (permalink / raw) To: Eric Sunshine; +Cc: git, Duy Nguyen, Antoine Pelisse Eric Sunshine <sunshine@sunshineco.com> writes: > +DESCRIPTION > +----------- > + > +For each ``Name $$<email@address>$$'' or ``$$<email@address>$$'' provided on > +the command-line or standard input (when using `--stdin`), prints a line with > +the canonical contact information for that person according to the 'mailmap' > +(see "Mapping Authors" below). If no mapping exists for the person, echoes the > +provided contact information. OK. The last part needed a reading and a half for me to realize the "echoes the provided contact information" is the same thing as "the "input string is printed as-is", especially because "contact information" is not defined anywhere in the previous part of the DESCRIPTION section, though. I might be worth starting the paragraph with: For each contact information (either in the form of ``Name <user@host>'' or ...) in order to clarify that the two forms of input is what you call "contact information". > diff --git a/builtin/check-mailmap.c b/builtin/check-mailmap.c > new file mode 100644 > index 0000000..c36a61c > --- /dev/null > +++ b/builtin/check-mailmap.c > @@ -0,0 +1,69 @@ > +#include "builtin.h" > +#include "mailmap.h" > +#include "parse-options.h" > +#include "string-list.h" > + > +static int use_stdin; > +static int null_out; Is there a reason why the variable name should not match that of the corresponding variables in check-ignore and check-attr, which you copied the bulk of the code from? If there isn't, use "null_term_line" like they do. > +static const char * const check_mailmap_usage[] = { > +N_("git check-mailmap [options] <contact>..."), > +NULL > +}; This mis-indentation looks somewhat unfortunate, but they are shared with other check-blah, and they are meant to contain a rather long string, so let's keep it like so. > +static void check_mailmap(struct string_list *mailmap, const char *contact) > +{ > + const char *name, *mail; > + size_t namelen, maillen; > + struct ident_split ident; > + char term = null_out ? '\0' : '\n'; Is there a reason why the variable name "term" should not match that of the corresponding variables in check-ignore and check-attr, which you copied the bulk of the code from? If there isn't, use "line_termination" like they do. > + if (split_ident_line(&ident, contact, strlen(contact))) > + die(_("unable to parse contact: %s"), contact); > + > + name = ident.name_begin; > + namelen = ident.name_end - ident.name_begin; > + mail = ident.mail_begin; > + maillen = ident.mail_end - ident.mail_begin; > + > + map_user(mailmap, &mail, &maillen, &name, &namelen); > + > + if (namelen) > + printf("%.*s ", (int)namelen, name); > + printf("<%.*s>%c", (int)maillen, mail, term); > +} > + > +int cmd_check_mailmap(int argc, const char **argv, const char *prefix) > +{ > + int i; > + struct string_list mailmap = STRING_LIST_INIT_NODUP; > + > + git_config(git_default_config, NULL); > + argc = parse_options(argc, argv, prefix, check_mailmap_options, > + check_mailmap_usage, 0); It is a bit distracting that the second line that is not indented enough. Either (1) with enough HT and with minimum number of SP, align "argc" and "check_mailmap_usage", or (2) with minimum number of HT and no SP, push "check_mailmap_usage" to the right of opening parenthesis of "(argc". Our code tend to prefer (1). > + if (argc == 0 && !use_stdin) > + die(_("no contacts specified")); > + > + read_mailmap(&mailmap, NULL); > + > + for (i = 0; i < argc; ++i) > + check_mailmap(&mailmap, argv[i]); > + maybe_flush_or_die(stdout, "stdout"); > + > + if (use_stdin) { > + struct strbuf buf = STRBUF_INIT; > + while (strbuf_getline(&buf, stdin, '\n') != EOF) { > + check_mailmap(&mailmap, buf.buf); > + maybe_flush_or_die(stdout, "stdout"); > + } > + strbuf_release(&buf); > + } > + > + clear_mailmap(&mailmap); > + return 0; > +} > diff --git a/command-list.txt b/command-list.txt > index bf83303..08b04e2 100644 > --- a/command-list.txt > +++ b/command-list.txt > @@ -13,6 +13,7 @@ git-bundle mainporcelain > git-cat-file plumbinginterrogators > git-check-attr purehelpers > git-check-ignore purehelpers > +git-check-mailmap purehelpers > git-checkout mainporcelain common > git-checkout-index plumbingmanipulators > git-check-ref-format purehelpers > diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash > index ebc40d4..c118550 100644 > --- a/contrib/completion/git-completion.bash > +++ b/contrib/completion/git-completion.bash > @@ -648,6 +648,7 @@ __git_list_porcelain_commands () > cat-file) : plumbing;; > check-attr) : plumbing;; > check-ignore) : plumbing;; > + check-mailmap) : plumbing;; > check-ref-format) : plumbing;; > checkout-index) : plumbing;; > commit-tree) : plumbing;; > diff --git a/git.c b/git.c > index 4359086..02c3035 100644 > --- a/git.c > +++ b/git.c > @@ -324,6 +324,7 @@ static void handle_internal_command(int argc, const char **argv) > { "cat-file", cmd_cat_file, RUN_SETUP }, > { "check-attr", cmd_check_attr, RUN_SETUP }, > { "check-ignore", cmd_check_ignore, RUN_SETUP | NEED_WORK_TREE }, > + { "check-mailmap", cmd_check_mailmap, RUN_SETUP }, As we can read from HEAD:.mailmap these days, the patch is correct that it does not require NEED_WORK_TREE here. Thanks. > { "check-ref-format", cmd_check_ref_format }, > { "checkout", cmd_checkout, RUN_SETUP | NEED_WORK_TREE }, > { "checkout-index", cmd_checkout_index, ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 1/4] builtin: add git-check-mailmap command 2013-07-11 19:04 ` Junio C Hamano @ 2013-07-12 3:28 ` Eric Sunshine 2013-07-12 5:47 ` Junio C Hamano 0 siblings, 1 reply; 18+ messages in thread From: Eric Sunshine @ 2013-07-12 3:28 UTC (permalink / raw) To: Junio C Hamano; +Cc: Git List, Duy Nguyen, Antoine Pelisse On Thu, Jul 11, 2013 at 3:04 PM, Junio C Hamano <gitster@pobox.com> wrote: > Eric Sunshine <sunshine@sunshineco.com> writes: > >> +DESCRIPTION >> +----------- >> + >> +For each ``Name $$<email@address>$$'' or ``$$<email@address>$$'' provided on >> +the command-line or standard input (when using `--stdin`), prints a line with >> +the canonical contact information for that person according to the 'mailmap' >> +(see "Mapping Authors" below). If no mapping exists for the person, echoes the >> +provided contact information. > > OK. The last part needed a reading and a half for me to realize the > "echoes the provided contact information" is the same thing as "the > "input string is printed as-is", especially because "contact > information" is not defined anywhere in the previous part of the > DESCRIPTION section, though. I might be worth starting the > paragraph with: > > For each contact information (either in the form of ``Name > <user@host>'' or ...) > > in order to clarify that the two forms of input is what you call > "contact information". Is this easier to read? For each ``Name $$<user@host>$$'' or ``$$<user@host>$$'' from the command-line or standard input (when using `--stdin`), print a line showing either the canonical name and email address (see "Mapping Authors" below), or the input ``Name $$<user@host>$$'' or ``$$<user@host>$$'' if there is no mapping for that person. >> diff --git a/builtin/check-mailmap.c b/builtin/check-mailmap.c >> new file mode 100644 >> index 0000000..c36a61c >> --- /dev/null >> +++ b/builtin/check-mailmap.c >> @@ -0,0 +1,69 @@ >> +#include "builtin.h" >> +#include "mailmap.h" >> +#include "parse-options.h" >> +#include "string-list.h" >> + >> +static int use_stdin; >> +static int null_out; > > Is there a reason why the variable name should not match that of the > corresponding variables in check-ignore and check-attr, which you > copied the bulk of the code from? > > If there isn't, use "null_term_line" like they do. In check-attr, null_term_line indicates that _input_ lines are null-terminated. In check-ignore, null_term_lines is overloaded (and perhaps abused) to mean that both _input_ and _output_ lines are null-terminated. In check-mailmap, -z affects only _output_ lines. A reader of the code can easily be misled into thinking that the variable controls the same function in all three programs, hence null_out was chosen to make it clear that it applies only to output. A lesser reason is that, in the future, someone might add an option to null terminate input lines (distinct from output), and null_in would be a reasonable name for that variable. Other than the above reasoning, I don't feel strongly about it and can revert the name if you prefer. >> +static void check_mailmap(struct string_list *mailmap, const char *contact) >> +{ >> + const char *name, *mail; >> + size_t namelen, maillen; >> + struct ident_split ident; >> + char term = null_out ? '\0' : '\n'; > > Is there a reason why the variable name "term" should not match that > of the corresponding variables in check-ignore and check-attr, which > you copied the bulk of the code from? > > If there isn't, use "line_termination" like they do. No strong justification. As with 'buf' vs. 'buffer', 'line_termination' required more reading effort without conveying any more information than 'term'. >> + if (split_ident_line(&ident, contact, strlen(contact))) >> + die(_("unable to parse contact: %s"), contact); >> + >> + name = ident.name_begin; >> + namelen = ident.name_end - ident.name_begin; >> + mail = ident.mail_begin; >> + maillen = ident.mail_end - ident.mail_begin; >> + >> + map_user(mailmap, &mail, &maillen, &name, &namelen); >> + >> + if (namelen) >> + printf("%.*s ", (int)namelen, name); >> + printf("<%.*s>%c", (int)maillen, mail, term); >> +} >> + >> +int cmd_check_mailmap(int argc, const char **argv, const char *prefix) >> +{ >> + int i; >> + struct string_list mailmap = STRING_LIST_INIT_NODUP; >> + >> + git_config(git_default_config, NULL); >> + argc = parse_options(argc, argv, prefix, check_mailmap_options, >> + check_mailmap_usage, 0); > > It is a bit distracting that the second line that is not indented > enough. Either (1) with enough HT and with minimum number of SP, > align "argc" and "check_mailmap_usage", or (2) with minimum number > of HT and no SP, push "check_mailmap_usage" to the right of opening > parenthesis of "(argc". Our code tend to prefer (1). Okay. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 1/4] builtin: add git-check-mailmap command 2013-07-12 3:28 ` Eric Sunshine @ 2013-07-12 5:47 ` Junio C Hamano 2013-07-12 6:24 ` Eric Sunshine 0 siblings, 1 reply; 18+ messages in thread From: Junio C Hamano @ 2013-07-12 5:47 UTC (permalink / raw) To: Eric Sunshine; +Cc: Git List, Duy Nguyen, Antoine Pelisse Eric Sunshine <sunshine@sunshineco.com> writes: >> For each contact information (either in the form of ``Name >> <user@host>'' or ...) >> >> in order to clarify that the two forms of input is what you call >> "contact information". > > Is this easier to read? > > For each ``Name $$<user@host>$$'' or ``$$<user@host>$$'' from the > command-line or standard input (when using `--stdin`), print a line > showing either the canonical name and email address (see "Mapping > Authors" below), or the input ``Name $$<user@host>$$'' or > ``$$<user@host>$$'' if there is no mapping for that person. I find it easier than your original, but I do not know if you would want to repeat the "Name... or <user@host>" at the end. It does not seem to add much useful information and is distracting. >> If there isn't, use "null_term_line" like they do. > > In check-attr, null_term_line indicates that _input_ lines are > null-terminated. In check-ignore, null_term_lines is overloaded (and > perhaps abused) to mean that both _input_ and _output_ lines are > null-terminated. That is unfortunate but it is good that you found the breakage. As we do not have --nul-terminated-input and --nul-terminated-output options separtely, -z should apply to both input and output. What b4666852 (check-attr: Add --stdin option, 2008-10-07) did is broken. What check-ignore does We should find a way to fix it. I have a feeling that silently fixing it and seeing if anybody screams might be the best course of action ;-). Also "git check-ignore -h" advertises "-z" as only affecting "--stdin", which is also wrong. It does affect both input and output as it should, so it should be described as such, I think. Thanks for noticing. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 1/4] builtin: add git-check-mailmap command 2013-07-12 5:47 ` Junio C Hamano @ 2013-07-12 6:24 ` Eric Sunshine 2013-07-12 6:34 ` Junio C Hamano 0 siblings, 1 reply; 18+ messages in thread From: Eric Sunshine @ 2013-07-12 6:24 UTC (permalink / raw) To: Junio C Hamano; +Cc: Git List, Duy Nguyen, Antoine Pelisse On Fri, Jul 12, 2013 at 1:47 AM, Junio C Hamano <gitster@pobox.com> wrote: > Eric Sunshine <sunshine@sunshineco.com> writes: > >>> For each contact information (either in the form of ``Name >>> <user@host>'' or ...) >>> >>> in order to clarify that the two forms of input is what you call >>> "contact information". >> >> Is this easier to read? >> >> For each ``Name $$<user@host>$$'' or ``$$<user@host>$$'' from the >> command-line or standard input (when using `--stdin`), print a line >> showing either the canonical name and email address (see "Mapping >> Authors" below), or the input ``Name $$<user@host>$$'' or >> ``$$<user@host>$$'' if there is no mapping for that person. > > I find it easier than your original, but I do not know if you would > want to repeat the "Name... or <user@host>" at the end. It does not > seem to add much useful information and is distracting. Next attempt: For each ``Name $$<user@host>$$'' or ``$$<user@host>$$'' from the command-line or standard input (when using `--stdin`) look up the person's canonical name and email address (see "Mapping Authors" below). If found, print them; otherwise print the input as-is. >> In check-attr, null_term_line indicates that _input_ lines are >> null-terminated. In check-ignore, null_term_lines is overloaded (and >> perhaps abused) to mean that both _input_ and _output_ lines are >> null-terminated. > > That is unfortunate but it is good that you found the breakage. As > we do not have --nul-terminated-input and --nul-terminated-output > options separtely, -z should apply to both input and output. What > b4666852 (check-attr: Add --stdin option, 2008-10-07) did is broken. I can make git-check-mailmap behave this way, however, other than git-check-ignore (which is quite new), there doesn't seem to be any precedence (that I can find) anywhere else in git which ties input and output null-termination to a single switch. Is it desirable to do so or should the user have more fine-grained control? ("xargs -0" comes to mind when thinking of a null-termination input switch.) > Also "git check-ignore -h" advertises "-z" as only affecting "--stdin", > which is also wrong. It does affect both input and output as it should, > so it should be described as such, I think. I also noticed this. (It was copied from check-attr.c). ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 1/4] builtin: add git-check-mailmap command 2013-07-12 6:24 ` Eric Sunshine @ 2013-07-12 6:34 ` Junio C Hamano 2013-07-12 6:39 ` Eric Sunshine 0 siblings, 1 reply; 18+ messages in thread From: Junio C Hamano @ 2013-07-12 6:34 UTC (permalink / raw) To: Eric Sunshine; +Cc: Git List, Duy Nguyen, Antoine Pelisse Eric Sunshine <sunshine@sunshineco.com> writes: >> I find it easier than your original, but I do not know if you would >> want to repeat the "Name... or <user@host>" at the end. It does not >> seem to add much useful information and is distracting. > > Next attempt: > > For each ``Name $$<user@host>$$'' or ``$$<user@host>$$'' from the > command-line or standard input (when using `--stdin`) look up the > person's canonical name and email address (see "Mapping Authors" > below). If found, print them; otherwise print the input as-is. Nice. > ... Is it desirable to do so > or should the user have more fine-grained control? ("xargs -0" comes > to mind when thinking of a null-termination input switch.) For the purposes of check-attr and check-ignore, a single "-z" governing both is sufficient. I think you already got that from my 4-patch series, but the core reason for that is : - when "-z" is used, the user knows the input paths may need protection against LF. - our output contains these same paths. - which means our output cannot be expressed unambiguously using LF as record separator. For the purpose of check-mailmap, I actually do not see much point in supporting "-z" format. We do not even handle names or addresses with LF in it. The mailmap format would not let you express such records in the first place, no? ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 1/4] builtin: add git-check-mailmap command 2013-07-12 6:34 ` Junio C Hamano @ 2013-07-12 6:39 ` Eric Sunshine 0 siblings, 0 replies; 18+ messages in thread From: Eric Sunshine @ 2013-07-12 6:39 UTC (permalink / raw) To: Junio C Hamano; +Cc: Git List, Duy Nguyen, Antoine Pelisse On Fri, Jul 12, 2013 at 2:34 AM, Junio C Hamano <gitster@pobox.com> wrote: > Eric Sunshine <sunshine@sunshineco.com> writes: > >> ... Is it desirable to do so >> or should the user have more fine-grained control? ("xargs -0" comes >> to mind when thinking of a null-termination input switch.) > > For the purposes of check-attr and check-ignore, a single "-z" > governing both is sufficient. I think you already got that from my > 4-patch series, but the core reason for that is : I'm reading it right now. > - when "-z" is used, the user knows the input paths may need > protection against LF. > > - our output contains these same paths. > > - which means our output cannot be expressed unambiguously using LF > as record separator. > > For the purpose of check-mailmap, I actually do not see much point > in supporting "-z" format. We do not even handle names or addresses > with LF in it. The mailmap format would not let you express such > records in the first place, no? You're right. I had this exact argument in mind for why null-termination was not needed on the input side of check-mailmap, but for some reason I had a blind spot concerning the output side. I'll drop this option in the next re-roll. ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v2 2/4] t4203: test check-mailmap command invocation 2013-07-11 14:55 [PATCH v2 0/4] add git-check-mailmap command Eric Sunshine 2013-07-11 14:55 ` [PATCH v2 1/4] builtin: " Eric Sunshine @ 2013-07-11 14:55 ` Eric Sunshine 2013-07-11 14:55 ` [PATCH v2 3/4] t4203: test mailmap functionality directly rather than indirectly Eric Sunshine 2013-07-11 14:55 ` [PATCH v2 4/4] t4203: consolidate test-repository setup Eric Sunshine 3 siblings, 0 replies; 18+ messages in thread From: Eric Sunshine @ 2013-07-11 14:55 UTC (permalink / raw) To: git; +Cc: Duy Nguyen, Antoine Pelisse, Eric Sunshine Test the command-line interface of check-mailmap. (Actual .mailmap functionality is already covered by existing tests.) Signed-off-by: Eric Sunshine <sunshine@sunshineco.com> --- t/t4203-mailmap.sh | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 50 insertions(+) diff --git a/t/t4203-mailmap.sh b/t/t4203-mailmap.sh index 842b754..8645492 100755 --- a/t/t4203-mailmap.sh +++ b/t/t4203-mailmap.sh @@ -13,6 +13,11 @@ fuzz_blame () { } test_expect_success setup ' + cat >contacts <<-\EOF && + A U Thor <author@example.com> + nick1 <bugs@company.xx> + EOF + echo one >one && git add one && test_tick && @@ -23,6 +28,51 @@ test_expect_success setup ' git commit --author "nick1 <bugs@company.xx>" -m second ' +test_expect_success 'check-mailmap no arguments' ' + test_must_fail git check-mailmap +' + +test_expect_success 'check-mailmap arguments' ' + cat >expect <<-\EOF && + A U Thor <author@example.com> + nick1 <bugs@company.xx> + EOF + git check-mailmap \ + "A U Thor <author@example.com>" \ + "nick1 <bugs@company.xx>" >actual && + test_cmp expect actual +' + +test_expect_success 'check-mailmap --stdin' ' + cat >expect <<-\EOF && + A U Thor <author@example.com> + nick1 <bugs@company.xx> + EOF + git check-mailmap --stdin <contacts >actual && + test_cmp expect actual +' + +test_expect_success 'check-mailmap --stdin arguments' ' + cat >expect <<-\EOF && + Internal Guy <bugs@company.xy> + EOF + cat <contacts >>expect && + git check-mailmap --stdin "Internal Guy <bugs@company.xy>" \ + <contacts >actual && + test_cmp expect actual +' + +test_expect_success 'check-mailmap -z' ' + printf "A U Thor <author@example.com>\0" >expect && + printf "nick1 <bugs@company.xx>\0" >>expect && + git check-mailmap -z --stdin <contacts >actual && + test_cmp expect actual +' + +test_expect_success 'check-mailmap bogus contact' ' + test_must_fail git check-mailmap bogus +' + cat >expect <<\EOF A U Thor (1): initial -- 1.8.3.2 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v2 3/4] t4203: test mailmap functionality directly rather than indirectly 2013-07-11 14:55 [PATCH v2 0/4] add git-check-mailmap command Eric Sunshine 2013-07-11 14:55 ` [PATCH v2 1/4] builtin: " Eric Sunshine 2013-07-11 14:55 ` [PATCH v2 2/4] t4203: test check-mailmap command invocation Eric Sunshine @ 2013-07-11 14:55 ` Eric Sunshine 2013-07-11 19:07 ` Junio C Hamano 2013-07-11 14:55 ` [PATCH v2 4/4] t4203: consolidate test-repository setup Eric Sunshine 3 siblings, 1 reply; 18+ messages in thread From: Eric Sunshine @ 2013-07-11 14:55 UTC (permalink / raw) To: git; +Cc: Duy Nguyen, Antoine Pelisse, Eric Sunshine With the introduction of check-mailmap, it is now possible to check .mailmap functionality directly rather than indirectly as a side-effect of other commands (such as git-shortlog), therefore, do so. Signed-off-by: Eric Sunshine <sunshine@sunshineco.com> --- t/t4203-mailmap.sh | 133 ++++++++++++++++++----------------------------------- 1 file changed, 45 insertions(+), 88 deletions(-) diff --git a/t/t4203-mailmap.sh b/t/t4203-mailmap.sh index 8645492..48a000b 100755 --- a/t/t4203-mailmap.sh +++ b/t/t4203-mailmap.sh @@ -74,128 +74,96 @@ test_expect_success 'check-mailmap bogus contact' ' ' cat >expect <<\EOF -A U Thor (1): - initial - -nick1 (1): - second - +A U Thor <author@example.com> +nick1 <bugs@company.xx> EOF test_expect_success 'No mailmap' ' - git shortlog HEAD >actual && + git check-mailmap --stdin <contacts >actual && test_cmp expect actual ' cat >expect <<\EOF -Repo Guy (1): - initial - -nick1 (1): - second - +Repo Guy <author@example.com> +nick1 <bugs@company.xx> EOF test_expect_success 'default .mailmap' ' echo "Repo Guy <author@example.com>" > .mailmap && - git shortlog HEAD >actual && + git check-mailmap --stdin <contacts >actual && test_cmp expect actual ' # Using a mailmap file in a subdirectory of the repo here, but # could just as well have been a file outside of the repository cat >expect <<\EOF -Internal Guy (1): - second - -Repo Guy (1): - initial - +Repo Guy <author@example.com> +Internal Guy <bugs@company.xx> EOF test_expect_success 'mailmap.file set' ' mkdir -p internal_mailmap && echo "Internal Guy <bugs@company.xx>" > internal_mailmap/.mailmap && git config mailmap.file internal_mailmap/.mailmap && - git shortlog HEAD >actual && + git check-mailmap --stdin <contacts >actual && test_cmp expect actual ' cat >expect <<\EOF -External Guy (1): - initial - -Internal Guy (1): - second - +External Guy <author@example.com> +Internal Guy <bugs@company.xx> EOF test_expect_success 'mailmap.file override' ' echo "External Guy <author@example.com>" >> internal_mailmap/.mailmap && git config mailmap.file internal_mailmap/.mailmap && - git shortlog HEAD >actual && + git check-mailmap --stdin <contacts >actual && test_cmp expect actual ' cat >expect <<\EOF -Repo Guy (1): - initial - -nick1 (1): - second - +Repo Guy <author@example.com> +nick1 <bugs@company.xx> EOF test_expect_success 'mailmap.file non-existent' ' rm internal_mailmap/.mailmap && rmdir internal_mailmap && - git shortlog HEAD >actual && + git check-mailmap --stdin <contacts >actual && test_cmp expect actual ' cat >expect <<\EOF -Internal Guy (1): - second - -Repo Guy (1): - initial - +Repo Guy <author@example.com> +Internal Guy <bugs@company.xy> EOF test_expect_success 'name entry after email entry' ' mkdir -p internal_mailmap && echo "<bugs@company.xy> <bugs@company.xx>" >internal_mailmap/.mailmap && echo "Internal Guy <bugs@company.xx>" >>internal_mailmap/.mailmap && - git shortlog HEAD >actual && + git check-mailmap --stdin <contacts >actual && test_cmp expect actual ' cat >expect <<\EOF -Internal Guy (1): - second - -Repo Guy (1): - initial - +Repo Guy <author@example.com> +Internal Guy <bugs@company.xy> EOF test_expect_success 'name entry after email entry, case-insensitive' ' mkdir -p internal_mailmap && echo "<bugs@company.xy> <bugs@company.xx>" >internal_mailmap/.mailmap && echo "Internal Guy <BUGS@Company.xx>" >>internal_mailmap/.mailmap && - git shortlog HEAD >actual && + git check-mailmap --stdin <contacts >actual && test_cmp expect actual ' cat >expect <<\EOF -A U Thor (1): - initial - -nick1 (1): - second - +A U Thor <author@example.com> +nick1 <bugs@company.xx> EOF test_expect_success 'No mailmap files, but configured' ' rm -f .mailmap internal_mailmap/.mailmap && - git shortlog HEAD >actual && + git check-mailmap --stdin <contacts >actual && test_cmp expect actual ' @@ -217,54 +185,43 @@ test_expect_success 'setup mailmap blob tests' ' test_expect_success 'mailmap.blob set' ' cat >expect <<-\EOF && - Blob Guy (1): - second - - Repo Guy (1): - initial - + Repo Guy <author@example.com> + Blob Guy <bugs@company.xx> EOF - git -c mailmap.blob=map:just-bugs shortlog HEAD >actual && + git -c mailmap.blob=map:just-bugs check-mailmap --stdin \ + <contacts >actual && test_cmp expect actual ' test_expect_success 'mailmap.blob overrides .mailmap' ' cat >expect <<-\EOF && - Blob Guy (2): - initial - second - + Blob Guy <author@example.com> + Blob Guy <bugs@company.xx> EOF - git -c mailmap.blob=map:both shortlog HEAD >actual && + git -c mailmap.blob=map:both check-mailmap --stdin \ + <contacts >actual && test_cmp expect actual ' test_expect_success 'mailmap.file overrides mailmap.blob' ' cat >expect <<-\EOF && - Blob Guy (1): - second - - Internal Guy (1): - initial - + Internal Guy <author@example.com> + Blob Guy <bugs@company.xx> EOF git \ -c mailmap.blob=map:both \ -c mailmap.file=internal.map \ - shortlog HEAD >actual && + check-mailmap --stdin <contacts >actual && test_cmp expect actual ' test_expect_success 'mailmap.blob can be missing' ' cat >expect <<-\EOF && - Repo Guy (1): - initial - - nick1 (1): - second - + Repo Guy <author@example.com> + nick1 <bugs@company.xx> EOF - git -c mailmap.blob=map:nonexistent shortlog HEAD >actual && + git -c mailmap.blob=map:nonexistent check-mailmap --stdin \ + <contacts >actual && test_cmp expect actual ' @@ -273,12 +230,12 @@ test_expect_success 'mailmap.blob defaults to off in non-bare repo' ' ( cd non-bare && test_commit one .mailmap "Fake Name <author@example.com>" && - echo " 1 Fake Name" >expect && - git shortlog -ns HEAD >actual && + echo "Fake Name <author@example.com>" >expect && + git check-mailmap "A U Thor <author@example.com>" >actual && test_cmp expect actual && rm .mailmap && - echo " 1 A U Thor" >expect && - git shortlog -ns HEAD >actual && + echo "A U Thor <author@example.com>" >expect && + git check-mailmap "A U Thor <author@example.com>" >actual && test_cmp expect actual ) ' @@ -287,8 +244,8 @@ test_expect_success 'mailmap.blob defaults to HEAD:.mailmap in bare repo' ' git clone --bare non-bare bare && ( cd bare && - echo " 1 Fake Name" >expect && - git shortlog -ns HEAD >actual && + echo "Fake Name <author@example.com>" >expect && + git check-mailmap "A U Thor <author@example.com>" >actual && test_cmp expect actual ) ' -- 1.8.3.2 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v2 3/4] t4203: test mailmap functionality directly rather than indirectly 2013-07-11 14:55 ` [PATCH v2 3/4] t4203: test mailmap functionality directly rather than indirectly Eric Sunshine @ 2013-07-11 19:07 ` Junio C Hamano 2013-07-12 0:35 ` Eric Sunshine 0 siblings, 1 reply; 18+ messages in thread From: Junio C Hamano @ 2013-07-11 19:07 UTC (permalink / raw) To: Eric Sunshine; +Cc: git, Duy Nguyen, Antoine Pelisse Eric Sunshine <sunshine@sunshineco.com> writes: > With the introduction of check-mailmap, it is now possible to check > .mailmap functionality directly rather than indirectly as a side-effect > of other commands (such as git-shortlog), therefore, do so. Does this patch mean that we will now ignore future breakages in shortlog and blame if their mailmap integration becomes buggy? I am not convinced it is a good idea if that is what is going on. > > Signed-off-by: Eric Sunshine <sunshine@sunshineco.com> > --- > t/t4203-mailmap.sh | 133 ++++++++++++++++++----------------------------------- > 1 file changed, 45 insertions(+), 88 deletions(-) > > diff --git a/t/t4203-mailmap.sh b/t/t4203-mailmap.sh > index 8645492..48a000b 100755 > --- a/t/t4203-mailmap.sh > +++ b/t/t4203-mailmap.sh > @@ -74,128 +74,96 @@ test_expect_success 'check-mailmap bogus contact' ' > ' > > cat >expect <<\EOF > -A U Thor (1): > - initial > - > -nick1 (1): > - second > - > +A U Thor <author@example.com> > +nick1 <bugs@company.xx> > EOF > > test_expect_success 'No mailmap' ' > - git shortlog HEAD >actual && > + git check-mailmap --stdin <contacts >actual && > test_cmp expect actual > ' > > cat >expect <<\EOF > -Repo Guy (1): > - initial > - > -nick1 (1): > - second > - > +Repo Guy <author@example.com> > +nick1 <bugs@company.xx> > EOF > > test_expect_success 'default .mailmap' ' > echo "Repo Guy <author@example.com>" > .mailmap && > - git shortlog HEAD >actual && > + git check-mailmap --stdin <contacts >actual && > test_cmp expect actual > ' > > # Using a mailmap file in a subdirectory of the repo here, but > # could just as well have been a file outside of the repository > cat >expect <<\EOF > -Internal Guy (1): > - second > - > -Repo Guy (1): > - initial > - > +Repo Guy <author@example.com> > +Internal Guy <bugs@company.xx> > EOF > test_expect_success 'mailmap.file set' ' > mkdir -p internal_mailmap && > echo "Internal Guy <bugs@company.xx>" > internal_mailmap/.mailmap && > git config mailmap.file internal_mailmap/.mailmap && > - git shortlog HEAD >actual && > + git check-mailmap --stdin <contacts >actual && > test_cmp expect actual > ' > > cat >expect <<\EOF > -External Guy (1): > - initial > - > -Internal Guy (1): > - second > - > +External Guy <author@example.com> > +Internal Guy <bugs@company.xx> > EOF > test_expect_success 'mailmap.file override' ' > echo "External Guy <author@example.com>" >> internal_mailmap/.mailmap && > git config mailmap.file internal_mailmap/.mailmap && > - git shortlog HEAD >actual && > + git check-mailmap --stdin <contacts >actual && > test_cmp expect actual > ' > > cat >expect <<\EOF > -Repo Guy (1): > - initial > - > -nick1 (1): > - second > - > +Repo Guy <author@example.com> > +nick1 <bugs@company.xx> > EOF > > test_expect_success 'mailmap.file non-existent' ' > rm internal_mailmap/.mailmap && > rmdir internal_mailmap && > - git shortlog HEAD >actual && > + git check-mailmap --stdin <contacts >actual && > test_cmp expect actual > ' > > cat >expect <<\EOF > -Internal Guy (1): > - second > - > -Repo Guy (1): > - initial > - > +Repo Guy <author@example.com> > +Internal Guy <bugs@company.xy> > EOF > > test_expect_success 'name entry after email entry' ' > mkdir -p internal_mailmap && > echo "<bugs@company.xy> <bugs@company.xx>" >internal_mailmap/.mailmap && > echo "Internal Guy <bugs@company.xx>" >>internal_mailmap/.mailmap && > - git shortlog HEAD >actual && > + git check-mailmap --stdin <contacts >actual && > test_cmp expect actual > ' > > cat >expect <<\EOF > -Internal Guy (1): > - second > - > -Repo Guy (1): > - initial > - > +Repo Guy <author@example.com> > +Internal Guy <bugs@company.xy> > EOF > > test_expect_success 'name entry after email entry, case-insensitive' ' > mkdir -p internal_mailmap && > echo "<bugs@company.xy> <bugs@company.xx>" >internal_mailmap/.mailmap && > echo "Internal Guy <BUGS@Company.xx>" >>internal_mailmap/.mailmap && > - git shortlog HEAD >actual && > + git check-mailmap --stdin <contacts >actual && > test_cmp expect actual > ' > > cat >expect <<\EOF > -A U Thor (1): > - initial > - > -nick1 (1): > - second > - > +A U Thor <author@example.com> > +nick1 <bugs@company.xx> > EOF > test_expect_success 'No mailmap files, but configured' ' > rm -f .mailmap internal_mailmap/.mailmap && > - git shortlog HEAD >actual && > + git check-mailmap --stdin <contacts >actual && > test_cmp expect actual > ' > > @@ -217,54 +185,43 @@ test_expect_success 'setup mailmap blob tests' ' > > test_expect_success 'mailmap.blob set' ' > cat >expect <<-\EOF && > - Blob Guy (1): > - second > - > - Repo Guy (1): > - initial > - > + Repo Guy <author@example.com> > + Blob Guy <bugs@company.xx> > EOF > - git -c mailmap.blob=map:just-bugs shortlog HEAD >actual && > + git -c mailmap.blob=map:just-bugs check-mailmap --stdin \ > + <contacts >actual && > test_cmp expect actual > ' > > test_expect_success 'mailmap.blob overrides .mailmap' ' > cat >expect <<-\EOF && > - Blob Guy (2): > - initial > - second > - > + Blob Guy <author@example.com> > + Blob Guy <bugs@company.xx> > EOF > - git -c mailmap.blob=map:both shortlog HEAD >actual && > + git -c mailmap.blob=map:both check-mailmap --stdin \ > + <contacts >actual && > test_cmp expect actual > ' > > test_expect_success 'mailmap.file overrides mailmap.blob' ' > cat >expect <<-\EOF && > - Blob Guy (1): > - second > - > - Internal Guy (1): > - initial > - > + Internal Guy <author@example.com> > + Blob Guy <bugs@company.xx> > EOF > git \ > -c mailmap.blob=map:both \ > -c mailmap.file=internal.map \ > - shortlog HEAD >actual && > + check-mailmap --stdin <contacts >actual && > test_cmp expect actual > ' > > test_expect_success 'mailmap.blob can be missing' ' > cat >expect <<-\EOF && > - Repo Guy (1): > - initial > - > - nick1 (1): > - second > - > + Repo Guy <author@example.com> > + nick1 <bugs@company.xx> > EOF > - git -c mailmap.blob=map:nonexistent shortlog HEAD >actual && > + git -c mailmap.blob=map:nonexistent check-mailmap --stdin \ > + <contacts >actual && > test_cmp expect actual > ' > > @@ -273,12 +230,12 @@ test_expect_success 'mailmap.blob defaults to off in non-bare repo' ' > ( > cd non-bare && > test_commit one .mailmap "Fake Name <author@example.com>" && > - echo " 1 Fake Name" >expect && > - git shortlog -ns HEAD >actual && > + echo "Fake Name <author@example.com>" >expect && > + git check-mailmap "A U Thor <author@example.com>" >actual && > test_cmp expect actual && > rm .mailmap && > - echo " 1 A U Thor" >expect && > - git shortlog -ns HEAD >actual && > + echo "A U Thor <author@example.com>" >expect && > + git check-mailmap "A U Thor <author@example.com>" >actual && > test_cmp expect actual > ) > ' > @@ -287,8 +244,8 @@ test_expect_success 'mailmap.blob defaults to HEAD:.mailmap in bare repo' ' > git clone --bare non-bare bare && > ( > cd bare && > - echo " 1 Fake Name" >expect && > - git shortlog -ns HEAD >actual && > + echo "Fake Name <author@example.com>" >expect && > + git check-mailmap "A U Thor <author@example.com>" >actual && > test_cmp expect actual > ) > ' ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 3/4] t4203: test mailmap functionality directly rather than indirectly 2013-07-11 19:07 ` Junio C Hamano @ 2013-07-12 0:35 ` Eric Sunshine 2013-07-12 0:55 ` Jonathan Nieder 0 siblings, 1 reply; 18+ messages in thread From: Eric Sunshine @ 2013-07-12 0:35 UTC (permalink / raw) To: Junio C Hamano; +Cc: Git List, Duy Nguyen, Antoine Pelisse On Thu, Jul 11, 2013 at 3:07 PM, Junio C Hamano <gitster@pobox.com> wrote: > Eric Sunshine <sunshine@sunshineco.com> writes: > >> With the introduction of check-mailmap, it is now possible to check >> .mailmap functionality directly rather than indirectly as a side-effect >> of other commands (such as git-shortlog), therefore, do so. > > Does this patch mean that we will now ignore future breakages in > shortlog and blame if their mailmap integration becomes buggy? > > I am not convinced it is a good idea if that is what is going on. I meant to mention in the cover letter that this patch was open for debate, however, it does not eliminate all testing of these other commands. The tests in which git-check-mailmap is substituted for git-shortlog all worked against a simplistic two-commit repository. Those tests were checking the low-level mailmap functionality under various conditions and configurations; they were not especially checking any particular behavior of git-shortlog. There still remain a final eight tests which cover git-shortlog, git-log, and git-blame. These tests do check mailmap-related behavior of those commands, and they do so using a more involved seven-commit repository with "complex" mappings, so we have not necessarily lost any checks of mailmap integration for those commands. Would this patch become more palatable if I stated the above in the commit message? -- ES ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 3/4] t4203: test mailmap functionality directly rather than indirectly 2013-07-12 0:35 ` Eric Sunshine @ 2013-07-12 0:55 ` Jonathan Nieder 2013-07-12 2:37 ` Eric Sunshine 2013-07-12 5:48 ` Junio C Hamano 0 siblings, 2 replies; 18+ messages in thread From: Jonathan Nieder @ 2013-07-12 0:55 UTC (permalink / raw) To: Eric Sunshine; +Cc: Junio C Hamano, Git List, Duy Nguyen, Antoine Pelisse Eric Sunshine wrote: > On Thu, Jul 11, 2013 at 3:07 PM, Junio C Hamano <gitster@pobox.com> wrote: >> Eric Sunshine <sunshine@sunshineco.com> writes: >>> With the introduction of check-mailmap, it is now possible to check >>> .mailmap functionality directly rather than indirectly as a side-effect >>> of other commands (such as git-shortlog), therefore, do so. >> >> Does this patch mean that we will now ignore future breakages in >> shortlog and blame if their mailmap integration becomes buggy? >> >> I am not convinced it is a good idea if that is what is going on. > > I meant to mention in the cover letter that this patch was open for > debate, however, it does not eliminate all testing of these other > commands. > > The tests in which git-check-mailmap is substituted for git-shortlog > all worked against a simplistic two-commit repository. Those tests > were checking the low-level mailmap functionality under various > conditions and configurations; they were not especially checking any > particular behavior of git-shortlog. > > There still remain a final eight tests which cover git-shortlog, > git-log, and git-blame. These tests do check mailmap-related behavior > of those commands, and they do so using a more involved seven-commit > repository with "complex" mappings, so we have not necessarily lost > any checks of mailmap integration for those commands. > > Would this patch become more palatable if I stated the above in the > commit message? My current thinking is "no" --- the patch has as a justification "Now we can test these aspects of .mailmap handling directly with a low-level tool instead of using the tool most people will use, so do so", which sounds an awful lot like "Reduce test coverage of commonly used tools, because we can". But I imagine the actual motivation was something other than "because we can". For example, maybe the idea is that after this patch, it should be easier to make cosmetic improvements to the shortlog, log, and blame output and only have to change those final 8 tests that are adequately covering the output? If you have such plans and this patch makes them easier, it could sound like a good patch as a stepping stone toward that. Thanks and hope that helps, Jonathan ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 3/4] t4203: test mailmap functionality directly rather than indirectly 2013-07-12 0:55 ` Jonathan Nieder @ 2013-07-12 2:37 ` Eric Sunshine 2013-07-12 5:48 ` Junio C Hamano 1 sibling, 0 replies; 18+ messages in thread From: Eric Sunshine @ 2013-07-12 2:37 UTC (permalink / raw) To: Jonathan Nieder; +Cc: Junio C Hamano, Git List, Duy Nguyen, Antoine Pelisse On Thu, Jul 11, 2013 at 8:55 PM, Jonathan Nieder <jrnieder@gmail.com> wrote: > Eric Sunshine wrote: >> On Thu, Jul 11, 2013 at 3:07 PM, Junio C Hamano <gitster@pobox.com> wrote: >>> Eric Sunshine <sunshine@sunshineco.com> writes: > >>>> With the introduction of check-mailmap, it is now possible to check >>>> .mailmap functionality directly rather than indirectly as a side-effect >>>> of other commands (such as git-shortlog), therefore, do so. >>> >>> Does this patch mean that we will now ignore future breakages in >>> shortlog and blame if their mailmap integration becomes buggy? >>> >>> I am not convinced it is a good idea if that is what is going on. >> >> I meant to mention in the cover letter that this patch was open for >> debate, however, it does not eliminate all testing of these other >> commands. >> >> The tests in which git-check-mailmap is substituted for git-shortlog >> all worked against a simplistic two-commit repository. Those tests >> were checking the low-level mailmap functionality under various >> conditions and configurations; they were not especially checking any >> particular behavior of git-shortlog. >> >> There still remain a final eight tests which cover git-shortlog, >> git-log, and git-blame. These tests do check mailmap-related behavior >> of those commands, and they do so using a more involved seven-commit >> repository with "complex" mappings, so we have not necessarily lost >> any checks of mailmap integration for those commands. >> >> Would this patch become more palatable if I stated the above in the >> commit message? > > My current thinking is "no" --- the patch has as a justification "Now > we can test these aspects of .mailmap handling directly with a > low-level tool instead of using the tool most people will use, so do > so", which sounds an awful lot like "Reduce test coverage of commonly > used tools, because we can". > > But I imagine the actual motivation was something other than "because > we can". The motivation is as stated in the subject: Direct rather than indirect testing of mailmap functionality. The tests in question are properly crafted to check only low-level mailmap (not git-shortlog) functionality under various conditions and configurations, yet the mental load on the reader is high because he has to keep in mind the authors and commits in the repository underlying git-shortlog's output. When testing via git-check-mailmap, mental load is low: there is a direct correlation between the "Name <email@address>" which goes in and that which comes out. What is being tested is the same, but it's easier to reason about and understand the the tests when done directly. When converting the tests, I also discovered an additional, perhaps more important motivation. Most of the tests check only that name remapping functions correctly, however, a couple also attempt to check address remapping (bugs@company.xx => bugs@company.xy). Even though the tests succeed, they don't actually prove that address remapping was correct (or occurred at all) since git-shortlog does not display the email address. git-check-mailmap always displays the email address, thus the check actually tests the intended email address remapping. > For example, maybe the idea is that after this patch, it > should be easier to make cosmetic improvements to the shortlog, log, > and blame output and only have to change those final 8 tests that are > adequately covering the output? If you have such plans and this patch > makes them easier, it could sound like a good patch as a stepping > stone toward that. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 3/4] t4203: test mailmap functionality directly rather than indirectly 2013-07-12 0:55 ` Jonathan Nieder 2013-07-12 2:37 ` Eric Sunshine @ 2013-07-12 5:48 ` Junio C Hamano 2013-07-12 6:05 ` Eric Sunshine 1 sibling, 1 reply; 18+ messages in thread From: Junio C Hamano @ 2013-07-12 5:48 UTC (permalink / raw) To: Jonathan Nieder; +Cc: Eric Sunshine, Git List, Duy Nguyen, Antoine Pelisse Jonathan Nieder <jrnieder@gmail.com> writes: > My current thinking is "no" --- the patch has as a justification "Now > we can test these aspects of .mailmap handling directly with a > low-level tool instead of using the tool most people will use, so do > so", which sounds an awful lot like "Reduce test coverage of commonly > used tools, because we can". Yes, that was exactly my reaction that prompted my response. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 3/4] t4203: test mailmap functionality directly rather than indirectly 2013-07-12 5:48 ` Junio C Hamano @ 2013-07-12 6:05 ` Eric Sunshine 2013-07-12 6:25 ` Junio C Hamano 0 siblings, 1 reply; 18+ messages in thread From: Eric Sunshine @ 2013-07-12 6:05 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jonathan Nieder, Git List, Duy Nguyen, Antoine Pelisse On Fri, Jul 12, 2013 at 1:48 AM, Junio C Hamano <gitster@pobox.com> wrote: > Jonathan Nieder <jrnieder@gmail.com> writes: > >> My current thinking is "no" --- the patch has as a justification "Now >> we can test these aspects of .mailmap handling directly with a >> low-level tool instead of using the tool most people will use, so do >> so", which sounds an awful lot like "Reduce test coverage of commonly >> used tools, because we can". > > Yes, that was exactly my reaction that prompted my response. Does any of my follow-up commentary result in a different reaction? If not, I'll drop patches 3 &4 in the re-roll. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 3/4] t4203: test mailmap functionality directly rather than indirectly 2013-07-12 6:05 ` Eric Sunshine @ 2013-07-12 6:25 ` Junio C Hamano 0 siblings, 0 replies; 18+ messages in thread From: Junio C Hamano @ 2013-07-12 6:25 UTC (permalink / raw) To: Eric Sunshine; +Cc: Jonathan Nieder, Git List, Duy Nguyen, Antoine Pelisse Eric Sunshine <sunshine@sunshineco.com> writes: > On Fri, Jul 12, 2013 at 1:48 AM, Junio C Hamano <gitster@pobox.com> wrote: >> Jonathan Nieder <jrnieder@gmail.com> writes: >> >>> My current thinking is "no" --- the patch has as a justification "Now >>> we can test these aspects of .mailmap handling directly with a >>> low-level tool instead of using the tool most people will use, so do >>> so", which sounds an awful lot like "Reduce test coverage of commonly >>> used tools, because we can". >> >> Yes, that was exactly my reaction that prompted my response. > > Does any of my follow-up commentary result in a different > reaction? Not really. While I _do_ think direct testing has merits, I think that should be done by adding direct tests, not by removing the tests that are meant to protect higher level _users_ of the underlying mechanism from breakages. After all, their breakages may not come from new breakages of the lower level mechanism (i.e. the mailmap.c code) but the way these higher level code makes calls into the mechanism, and direct test of the lower level mechanism will not protect them from the latter kind of breakages. ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v2 4/4] t4203: consolidate test-repository setup 2013-07-11 14:55 [PATCH v2 0/4] add git-check-mailmap command Eric Sunshine ` (2 preceding siblings ...) 2013-07-11 14:55 ` [PATCH v2 3/4] t4203: test mailmap functionality directly rather than indirectly Eric Sunshine @ 2013-07-11 14:55 ` Eric Sunshine 3 siblings, 0 replies; 18+ messages in thread From: Eric Sunshine @ 2013-07-11 14:55 UTC (permalink / raw) To: git; +Cc: Duy Nguyen, Antoine Pelisse, Eric Sunshine The simple two-commit test-repository created by 'setup' is no longer needed by any of the tests retrofitted to use check-mailmap. Subsequent, more complex tests of git-shortlog, git-log, and git-blame functionality expand the repository by adding five commits. Consolidate the creation of this seven-commit repository into a single setup function. Signed-off-by: Eric Sunshine <sunshine@sunshineco.com> --- t/t4203-mailmap.sh | 53 +++++++++++++++++++++++++++-------------------------- 1 file changed, 27 insertions(+), 26 deletions(-) diff --git a/t/t4203-mailmap.sh b/t/t4203-mailmap.sh index 48a000b..9c3c83a 100755 --- a/t/t4203-mailmap.sh +++ b/t/t4203-mailmap.sh @@ -22,10 +22,36 @@ test_expect_success setup ' git add one && test_tick && git commit -m initial && + echo two >>one && git add one && test_tick && - git commit --author "nick1 <bugs@company.xx>" -m second + git commit --author "nick1 <bugs@company.xx>" -m second && + + echo three >>one && + git add one && + test_tick && + git commit --author "nick2 <bugs@company.xx>" -m third && + + echo four >>one && + git add one && + test_tick && + git commit --author "nick2 <nick2@company.xx>" -m fourth && + + echo five >>one && + git add one && + test_tick && + git commit --author "santa <me@company.xx>" -m fifth && + + echo six >>one && + git add one && + test_tick && + git commit --author "claus <me@company.xx>" -m sixth && + + echo seven >>one && + git add one && + test_tick && + git commit --author "CTO <cto@coompany.xx>" -m seventh ' test_expect_success 'check-mailmap no arguments' ' @@ -276,31 +302,6 @@ Some Dude <some@dude.xx> (1): EOF test_expect_success 'Shortlog output (complex mapping)' ' - echo three >>one && - git add one && - test_tick && - git commit --author "nick2 <bugs@company.xx>" -m third && - - echo four >>one && - git add one && - test_tick && - git commit --author "nick2 <nick2@company.xx>" -m fourth && - - echo five >>one && - git add one && - test_tick && - git commit --author "santa <me@company.xx>" -m fifth && - - echo six >>one && - git add one && - test_tick && - git commit --author "claus <me@company.xx>" -m sixth && - - echo seven >>one && - git add one && - test_tick && - git commit --author "CTO <cto@coompany.xx>" -m seventh && - mkdir -p internal_mailmap && echo "Committed <committer@example.com>" > internal_mailmap/.mailmap && echo "<cto@company.xx> <cto@coompany.xx>" >> internal_mailmap/.mailmap && -- 1.8.3.2 ^ permalink raw reply related [flat|nested] 18+ messages in thread
end of thread, other threads:[~2013-07-12 6:39 UTC | newest] Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2013-07-11 14:55 [PATCH v2 0/4] add git-check-mailmap command Eric Sunshine 2013-07-11 14:55 ` [PATCH v2 1/4] builtin: " Eric Sunshine 2013-07-11 19:04 ` Junio C Hamano 2013-07-12 3:28 ` Eric Sunshine 2013-07-12 5:47 ` Junio C Hamano 2013-07-12 6:24 ` Eric Sunshine 2013-07-12 6:34 ` Junio C Hamano 2013-07-12 6:39 ` Eric Sunshine 2013-07-11 14:55 ` [PATCH v2 2/4] t4203: test check-mailmap command invocation Eric Sunshine 2013-07-11 14:55 ` [PATCH v2 3/4] t4203: test mailmap functionality directly rather than indirectly Eric Sunshine 2013-07-11 19:07 ` Junio C Hamano 2013-07-12 0:35 ` Eric Sunshine 2013-07-12 0:55 ` Jonathan Nieder 2013-07-12 2:37 ` Eric Sunshine 2013-07-12 5:48 ` Junio C Hamano 2013-07-12 6:05 ` Eric Sunshine 2013-07-12 6:25 ` Junio C Hamano 2013-07-11 14:55 ` [PATCH v2 4/4] t4203: consolidate test-repository setup Eric Sunshine
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).