* Add author and committer configuration settings @ 2019-01-25 21:59 William Hubbs 2019-01-25 21:59 ` [PATCH v2 1/2] config: allow giving separate author and committer idents William Hubbs 2019-01-25 21:59 ` [PATCH v2 2/2] tests: add test for " William Hubbs 0 siblings, 2 replies; 15+ messages in thread From: William Hubbs @ 2019-01-25 21:59 UTC (permalink / raw) To: git; +Cc: williamh, chutzpah All, this is a re-roll of my previous patch to add separate author and committer settings.:s/committer/committer configuration/ A I attempted to encorporate everything from the last iteration, and all tests still pass. This applies to master. Please review. [PATCH v2 1/2] config: allow giving separate author and committer [PATCH v2 2/2] tests: add test for separate author and committer Thanks, William ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v2 1/2] config: allow giving separate author and committer idents 2019-01-25 21:59 Add author and committer configuration settings William Hubbs @ 2019-01-25 21:59 ` William Hubbs 2019-01-25 22:58 ` Ævar Arnfjörð Bjarmason 2019-01-25 21:59 ` [PATCH v2 2/2] tests: add test for " William Hubbs 1 sibling, 1 reply; 15+ messages in thread From: William Hubbs @ 2019-01-25 21:59 UTC (permalink / raw) To: git; +Cc: williamh, chutzpah The author.email, author.name, committer.email and committer.name settings are analogous to the GIT_AUTHOR_* and GIT_COMMITTER_* environment variables, but for the git config system. This allows them to be set separately for each repository. Git supports setting different authorship and committer information with environment variables. However, environment variables are set in the shell, so if different authorship and committer information is needed for different repositories an external tool is required. This adds support to git config for author.email, author.name, committer.email and committer.name settings so this information can be set per repository. Also, it generalizes the fmt_ident function so it can handle author vs committer identification. Signed-off-by: William Hubbs <williamh@gentoo.org> --- Documentation/config/user.txt | 31 +++++++++++++- blame.c | 3 +- builtin/am.c | 2 +- builtin/commit.c | 3 +- cache.h | 13 +++++- config.c | 6 +++ ident.c | 81 +++++++++++++++++++++++++++++++++-- log-tree.c | 3 +- sequencer.c | 5 +-- 9 files changed, 132 insertions(+), 15 deletions(-) diff --git a/Documentation/config/user.txt b/Documentation/config/user.txt index b5b2ba1199..18e1ec3c1b 100644 --- a/Documentation/config/user.txt +++ b/Documentation/config/user.txt @@ -1,12 +1,39 @@ +author.email:: + The email address used for the author of newly + created commits. Defaults to the value of the + `GIT_AUTHOR_EMAIL` environment variable, or if + the environment variable is not set, the `user.email` + configuration variable. + +author.name:: + The full name used for the author of newly created commits. + Defaults to the value of the `GIT_AUTHOR_NAME` environment variable, or + if the environment variable is not set, + the `user.email` configuration variable. + +committer.email:: + The email address used for the committer of newly created commits. + Defaults to the value of the `GIT_COMMITTER_EMAIL` environment + variable, or if the environment variable is not set, the `user.email` + configuration variable. + +committer.name:: + The full name used for the committer of newly created commits. + Defaults to the value of the `GIT_COMMITTER_NAME` environment + variable, or if the environment variable is not set, the `user.name` + configuration variable. + user.email:: Your email address to be recorded in any newly created commits. Can be overridden by the `GIT_AUTHOR_EMAIL`, `GIT_COMMITTER_EMAIL`, and - `EMAIL` environment variables. See linkgit:git-commit-tree[1]. + `EMAIL` environment variables or the `author.email` or + `committer.email` settings discussed above. See linkgit:git-commit-tree[1]. user.name:: Your full name to be recorded in any newly created commits. Can be overridden by the `GIT_AUTHOR_NAME` and `GIT_COMMITTER_NAME` - environment variables. See linkgit:git-commit-tree[1]. + environment variables or the `author.name` or `committer.name` + settings discussed above. See linkgit:git-commit-tree[1]. user.useConfigOnly:: Instruct Git to avoid trying to guess defaults for `user.email` diff --git a/blame.c b/blame.c index 43861437f7..c9c351eb36 100644 --- a/blame.c +++ b/blame.c @@ -204,7 +204,8 @@ static struct commit *fake_working_tree_commit(struct repository *r, origin = make_origin(commit, path); - ident = fmt_ident("Not Committed Yet", "not.committed.yet", NULL, 0); + ident = fmt_ident("Not Committed Yet", "not.committed.yet", + WANT_BLANK_IDENT, NULL, 0); strbuf_addstr(&msg, "tree 0000000000000000000000000000000000000000\n"); for (parent = commit->parents; parent; parent = parent->next) strbuf_addf(&msg, "parent %s\n", diff --git a/builtin/am.c b/builtin/am.c index 95370313b6..53fdd22c45 100644 --- a/builtin/am.c +++ b/builtin/am.c @@ -1594,7 +1594,7 @@ static void do_commit(const struct am_state *state) } author = fmt_ident(state->author_name, state->author_email, - state->ignore_date ? NULL : state->author_date, + WANT_AUTHOR_IDENT, state->ignore_date ? NULL : state->author_date, IDENT_STRICT); if (state->committer_date_is_author_date) diff --git a/builtin/commit.c b/builtin/commit.c index 004b816635..f96b90daeb 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -607,7 +607,8 @@ static void determine_author_info(struct strbuf *author_ident) set_ident_var(&date, strbuf_detach(&date_buf, NULL)); } - strbuf_addstr(author_ident, fmt_ident(name, email, date, IDENT_STRICT)); + strbuf_addstr(author_ident, fmt_ident(name, email, WANT_AUTHOR_IDENT, date, + IDENT_STRICT)); assert_split_ident(&author, author_ident); export_one("GIT_AUTHOR_NAME", author.name_begin, author.name_end, 0); export_one("GIT_AUTHOR_EMAIL", author.mail_begin, author.mail_end, 0); diff --git a/cache.h b/cache.h index 49713cc5a5..f13c6e244f 100644 --- a/cache.h +++ b/cache.h @@ -1479,10 +1479,19 @@ int date_overflows(timestamp_t date); #define IDENT_STRICT 1 #define IDENT_NO_DATE 2 #define IDENT_NO_NAME 4 + +enum want_ident { + WANT_BLANK_IDENT, + WANT_AUTHOR_IDENT, + WANT_COMMITTER_IDENT, +}; + extern const char *git_author_info(int); extern const char *git_committer_info(int); -extern const char *fmt_ident(const char *name, const char *email, const char *date_str, int); -extern const char *fmt_name(const char *name, const char *email); +extern const char *fmt_ident(const char *name, const char *email, + enum want_ident whose_ident, + const char *date_str, int); +extern const char *fmt_name(enum want_ident); extern const char *ident_default_name(void); extern const char *ident_default_email(void); extern const char *git_editor(void); diff --git a/config.c b/config.c index ff521eb27a..4bd5920dea 100644 --- a/config.c +++ b/config.c @@ -1484,6 +1484,12 @@ int git_default_config(const char *var, const char *value, void *cb) return 0; } + if (starts_with(var, "author.")) + return git_ident_config(var, value, cb); + + if (starts_with(var, "committer.")) + return git_ident_config(var, value, cb); + /* Add other config variables here and to Documentation/config.txt. */ return 0; } diff --git a/ident.c b/ident.c index 33bcf40644..a1e774c3e5 100644 --- a/ident.c +++ b/ident.c @@ -11,6 +11,10 @@ static struct strbuf git_default_name = STRBUF_INIT; static struct strbuf git_default_email = STRBUF_INIT; static struct strbuf git_default_date = STRBUF_INIT; +static struct strbuf git_author_name = STRBUF_INIT; +static struct strbuf git_author_email = STRBUF_INIT; +static struct strbuf git_committer_name = STRBUF_INIT; +static struct strbuf git_committer_email = STRBUF_INIT; static int default_email_is_bogus; static int default_name_is_bogus; @@ -355,13 +359,19 @@ N_("\n" "\n"); const char *fmt_ident(const char *name, const char *email, - const char *date_str, int flag) + enum want_ident whose_ident, const char *date_str, int flag) { static struct strbuf ident = STRBUF_INIT; int strict = (flag & IDENT_STRICT); int want_date = !(flag & IDENT_NO_DATE); int want_name = !(flag & IDENT_NO_NAME); + if (!email) { + if (whose_ident == WANT_AUTHOR_IDENT && git_author_email.len) + email = git_author_email.buf; + else if (whose_ident == WANT_COMMITTER_IDENT && git_committer_email.len) + email = git_committer_email.buf; + } if (!email) { if (strict && ident_use_config_only && !(ident_config_given & IDENT_MAIL_GIVEN)) { @@ -377,6 +387,13 @@ const char *fmt_ident(const char *name, const char *email, if (want_name) { int using_default = 0; + if (!name) { + if (whose_ident == WANT_AUTHOR_IDENT && git_author_name.len) + name = git_author_name.buf; + else if (whose_ident == WANT_COMMITTER_IDENT && + git_committer_name.len) + name = git_committer_name.buf; + } if (!name) { if (strict && ident_use_config_only && !(ident_config_given & IDENT_NAME_GIVEN)) { @@ -425,9 +442,25 @@ const char *fmt_ident(const char *name, const char *email, return ident.buf; } -const char *fmt_name(const char *name, const char *email) +const char *fmt_name(enum want_ident whose_ident) { - return fmt_ident(name, email, NULL, IDENT_STRICT | IDENT_NO_DATE); + char *name = NULL; + char *email = NULL; + + switch (whose_ident) { + case WANT_BLANK_IDENT: + break; + case WANT_AUTHOR_IDENT: + name = getenv("GIT_AUTHOR_NAME"); + email = getenv("GIT_AUTHOR_EMAIL"); + break; + case WANT_COMMITTER_IDENT: + name = getenv("GIT_COMMITTER_NAME"); + email = getenv("GIT_COMMITTER_EMAIL"); + break; + } + return fmt_ident(name, email, whose_ident, NULL, + IDENT_STRICT | IDENT_NO_DATE); } const char *git_author_info(int flag) @@ -438,6 +471,7 @@ const char *git_author_info(int flag) author_ident_explicitly_given |= IDENT_MAIL_GIVEN; return fmt_ident(getenv("GIT_AUTHOR_NAME"), getenv("GIT_AUTHOR_EMAIL"), + WANT_AUTHOR_IDENT, getenv("GIT_AUTHOR_DATE"), flag); } @@ -450,6 +484,7 @@ const char *git_committer_info(int flag) committer_ident_explicitly_given |= IDENT_MAIL_GIVEN; return fmt_ident(getenv("GIT_COMMITTER_NAME"), getenv("GIT_COMMITTER_EMAIL"), + WANT_COMMITTER_IDENT, getenv("GIT_COMMITTER_DATE"), flag); } @@ -480,6 +515,46 @@ int git_ident_config(const char *var, const char *value, void *data) return 0; } + if (!strcmp(var, "author.name")) { + if (!value) + return config_error_nonbool(var); + strbuf_reset(&git_author_name); + strbuf_addstr(&git_author_name, value); + author_ident_explicitly_given |= IDENT_NAME_GIVEN; + ident_config_given |= IDENT_NAME_GIVEN; + return 0; + } + + if (!strcmp(var, "author.email")) { + if (!value) + return config_error_nonbool(var); + strbuf_reset(&git_author_email); + strbuf_addstr(&git_author_email, value); + author_ident_explicitly_given |= IDENT_MAIL_GIVEN; + ident_config_given |= IDENT_MAIL_GIVEN; + return 0; + } + + if (!strcmp(var, "committer.name")) { + if (!value) + return config_error_nonbool(var); + strbuf_reset(&git_committer_name); + strbuf_addstr(&git_committer_name, value); + committer_ident_explicitly_given |= IDENT_NAME_GIVEN; + ident_config_given |= IDENT_NAME_GIVEN; + return 0; + } + + if (!strcmp(var, "committer.email")) { + if (!value) + return config_error_nonbool(var); + strbuf_reset(&git_committer_email); + strbuf_addstr(&git_committer_email, value); + committer_ident_explicitly_given |= IDENT_MAIL_GIVEN; + ident_config_given |= IDENT_MAIL_GIVEN; + return 0; + } + if (!strcmp(var, "user.name")) { if (!value) return config_error_nonbool(var); diff --git a/log-tree.c b/log-tree.c index 10680c139e..43ef4f4300 100644 --- a/log-tree.c +++ b/log-tree.c @@ -687,8 +687,7 @@ void show_log(struct rev_info *opt) */ if (ctx.need_8bit_cte >= 0 && opt->add_signoff) ctx.need_8bit_cte = - has_non_ascii(fmt_name(getenv("GIT_COMMITTER_NAME"), - getenv("GIT_COMMITTER_EMAIL"))); + has_non_ascii(fmt_name(WANT_COMMITTER_IDENT)); ctx.date_mode = opt->date_mode; ctx.date_mode_explicit = opt->date_mode_explicit; ctx.abbrev = opt->diffopt.abbrev; diff --git a/sequencer.c b/sequencer.c index f5370f4965..3505d52bb9 100644 --- a/sequencer.c +++ b/sequencer.c @@ -836,7 +836,7 @@ static const char *read_author_ident(struct strbuf *buf) } strbuf_reset(&out); - strbuf_addstr(&out, fmt_ident(name, email, date, 0)); + strbuf_addstr(&out, fmt_ident(name, email, WANT_AUTHOR_IDENT, date, 0)); strbuf_swap(buf, &out); strbuf_release(&out); free(name); @@ -4087,8 +4087,7 @@ void append_signoff(struct strbuf *msgbuf, size_t ignore_footer, unsigned flag) int has_footer; strbuf_addstr(&sob, sign_off_header); - strbuf_addstr(&sob, fmt_name(getenv("GIT_COMMITTER_NAME"), - getenv("GIT_COMMITTER_EMAIL"))); + strbuf_addstr(&sob, fmt_name(WANT_COMMITTER_IDENT)); strbuf_addch(&sob, '\n'); if (!ignore_footer) -- 2.19.2 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v2 1/2] config: allow giving separate author and committer idents 2019-01-25 21:59 ` [PATCH v2 1/2] config: allow giving separate author and committer idents William Hubbs @ 2019-01-25 22:58 ` Ævar Arnfjörð Bjarmason 2019-01-28 18:58 ` William Hubbs 2019-01-29 22:42 ` William Hubbs 0 siblings, 2 replies; 15+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2019-01-25 22:58 UTC (permalink / raw) To: William Hubbs; +Cc: git, chutzpah On Fri, Jan 25 2019, William Hubbs wrote: > The author.email, author.name, committer.email and committer.name > settings are analogous to the GIT_AUTHOR_* and GIT_COMMITTER_* > environment variables, but for the git config system. This allows them > to be set separately for each repository. > > Git supports setting different authorship and committer > information with environment variables. However, environment variables > are set in the shell, so if different authorship and committer > information is needed for different repositories an external tool is > required. > > This adds support to git config for author.email, author.name, > committer.email and committer.name settings so this information > can be set per repository. > > Also, it generalizes the fmt_ident function so it can handle author vs > committer identification. I have not tested this in any detail... > diff --git a/Documentation/config/user.txt b/Documentation/config/user.txt > index b5b2ba1199..18e1ec3c1b 100644 > --- a/Documentation/config/user.txt > +++ b/Documentation/config/user.txt > @@ -1,12 +1,39 @@ > +author.email:: > + The email address used for the author of newly > + created commits. Defaults to the value of the > + `GIT_AUTHOR_EMAIL` environment variable, or if > + the environment variable is not set, the `user.email` > + configuration variable. > + > +author.name:: > + The full name used for the author of newly created commits. > + Defaults to the value of the `GIT_AUTHOR_NAME` environment variable, or > + if the environment variable is not set, > + the `user.email` configuration variable. > + > +committer.email:: > + The email address used for the committer of newly created commits. > + Defaults to the value of the `GIT_COMMITTER_EMAIL` environment > + variable, or if the environment variable is not set, the `user.email` > + configuration variable. > + > +committer.name:: > + The full name used for the committer of newly created commits. > + Defaults to the value of the `GIT_COMMITTER_NAME` environment > + variable, or if the environment variable is not set, the `user.name` > + configuration variable. > + > user.email:: > Your email address to be recorded in any newly created commits. > Can be overridden by the `GIT_AUTHOR_EMAIL`, `GIT_COMMITTER_EMAIL`, and > - `EMAIL` environment variables. See linkgit:git-commit-tree[1]. > + `EMAIL` environment variables or the `author.email` or > + `committer.email` settings discussed above. See linkgit:git-commit-tree[1]. > > user.name:: > Your full name to be recorded in any newly created commits. > Can be overridden by the `GIT_AUTHOR_NAME` and `GIT_COMMITTER_NAME` > - environment variables. See linkgit:git-commit-tree[1]. > + environment variables or the `author.name` or `committer.name` > + settings discussed above. See linkgit:git-commit-tree[1]. Looks correct, although I wonder if we're at the point where it would be better to present this info as a table. > diff --git a/builtin/am.c b/builtin/am.c > index 95370313b6..53fdd22c45 100644 > --- a/builtin/am.c > +++ b/builtin/am.c > @@ -1594,7 +1594,7 @@ static void do_commit(const struct am_state *state) > } > > author = fmt_ident(state->author_name, state->author_email, > - state->ignore_date ? NULL : state->author_date, > + WANT_AUTHOR_IDENT, state->ignore_date ? NULL : state->author_date, This & a few other things in this series take the code beyond 79 characters. > IDENT_STRICT); > > if (state->committer_date_is_author_date) > diff --git a/builtin/commit.c b/builtin/commit.c > index 004b816635..f96b90daeb 100644 > --- a/builtin/commit.c > +++ b/builtin/commit.c > @@ -607,7 +607,8 @@ static void determine_author_info(struct strbuf *author_ident) > set_ident_var(&date, strbuf_detach(&date_buf, NULL)); > } > > - strbuf_addstr(author_ident, fmt_ident(name, email, date, IDENT_STRICT)); > + strbuf_addstr(author_ident, fmt_ident(name, email, WANT_AUTHOR_IDENT, date, > + IDENT_STRICT)); > assert_split_ident(&author, author_ident); > export_one("GIT_AUTHOR_NAME", author.name_begin, author.name_end, 0); > export_one("GIT_AUTHOR_EMAIL", author.mail_begin, author.mail_end, 0); > diff --git a/cache.h b/cache.h > index 49713cc5a5..f13c6e244f 100644 > --- a/cache.h > +++ b/cache.h > @@ -1479,10 +1479,19 @@ int date_overflows(timestamp_t date); > #define IDENT_STRICT 1 > #define IDENT_NO_DATE 2 > #define IDENT_NO_NAME 4 > + > +enum want_ident { > + WANT_BLANK_IDENT, > + WANT_AUTHOR_IDENT, > + WANT_COMMITTER_IDENT, > +}; > + > extern const char *git_author_info(int); > extern const char *git_committer_info(int); > -extern const char *fmt_ident(const char *name, const char *email, const char *date_str, int); > -extern const char *fmt_name(const char *name, const char *email); > +extern const char *fmt_ident(const char *name, const char *email, > + enum want_ident whose_ident, > + const char *date_str, int); > +extern const char *fmt_name(enum want_ident); > extern const char *ident_default_name(void); > extern const char *ident_default_email(void); > extern const char *git_editor(void); > diff --git a/config.c b/config.c > index ff521eb27a..4bd5920dea 100644 > --- a/config.c > +++ b/config.c > @@ -1484,6 +1484,12 @@ int git_default_config(const char *var, const char *value, void *cb) > return 0; > } > > + if (starts_with(var, "author.")) > + return git_ident_config(var, value, cb); > + > + if (starts_with(var, "committer.")) > + return git_ident_config(var, value, cb); > + > /* Add other config variables here and to Documentation/config.txt. */ > return 0; > } > diff --git a/ident.c b/ident.c > index 33bcf40644..a1e774c3e5 100644 > --- a/ident.c > +++ b/ident.c > @@ -11,6 +11,10 @@ > static struct strbuf git_default_name = STRBUF_INIT; > static struct strbuf git_default_email = STRBUF_INIT; > static struct strbuf git_default_date = STRBUF_INIT; > +static struct strbuf git_author_name = STRBUF_INIT; > +static struct strbuf git_author_email = STRBUF_INIT; > +static struct strbuf git_committer_name = STRBUF_INIT; > +static struct strbuf git_committer_email = STRBUF_INIT; > static int default_email_is_bogus; > static int default_name_is_bogus; > > @@ -355,13 +359,19 @@ N_("\n" > "\n"); > > const char *fmt_ident(const char *name, const char *email, > - const char *date_str, int flag) > + enum want_ident whose_ident, const char *date_str, int flag) > { > static struct strbuf ident = STRBUF_INIT; > int strict = (flag & IDENT_STRICT); > int want_date = !(flag & IDENT_NO_DATE); > int want_name = !(flag & IDENT_NO_NAME); > > + if (!email) { > + if (whose_ident == WANT_AUTHOR_IDENT && git_author_email.len) > + email = git_author_email.buf; > + else if (whose_ident == WANT_COMMITTER_IDENT && git_committer_email.len) > + email = git_committer_email.buf; > + } > if (!email) { > if (strict && ident_use_config_only > && !(ident_config_given & IDENT_MAIL_GIVEN)) { > @@ -377,6 +387,13 @@ const char *fmt_ident(const char *name, const char *email, > > if (want_name) { > int using_default = 0; > + if (!name) { > + if (whose_ident == WANT_AUTHOR_IDENT && git_author_name.len) > + name = git_author_name.buf; > + else if (whose_ident == WANT_COMMITTER_IDENT && > + git_committer_name.len) > + name = git_committer_name.buf; > + } > if (!name) { > if (strict && ident_use_config_only > && !(ident_config_given & IDENT_NAME_GIVEN)) { > @@ -425,9 +442,25 @@ const char *fmt_ident(const char *name, const char *email, > return ident.buf; > } > > -const char *fmt_name(const char *name, const char *email) > +const char *fmt_name(enum want_ident whose_ident) > { > - return fmt_ident(name, email, NULL, IDENT_STRICT | IDENT_NO_DATE); > + char *name = NULL; > + char *email = NULL; > + > + switch (whose_ident) { > + case WANT_BLANK_IDENT: > + break; > + case WANT_AUTHOR_IDENT: > + name = getenv("GIT_AUTHOR_NAME"); > + email = getenv("GIT_AUTHOR_EMAIL"); > + break; > + case WANT_COMMITTER_IDENT: > + name = getenv("GIT_COMMITTER_NAME"); > + email = getenv("GIT_COMMITTER_EMAIL"); > + break; > + } > + return fmt_ident(name, email, whose_ident, NULL, > + IDENT_STRICT | IDENT_NO_DATE); > } > > const char *git_author_info(int flag) > @@ -438,6 +471,7 @@ const char *git_author_info(int flag) > author_ident_explicitly_given |= IDENT_MAIL_GIVEN; > return fmt_ident(getenv("GIT_AUTHOR_NAME"), > getenv("GIT_AUTHOR_EMAIL"), > + WANT_AUTHOR_IDENT, > getenv("GIT_AUTHOR_DATE"), > flag); > } > @@ -450,6 +484,7 @@ const char *git_committer_info(int flag) > committer_ident_explicitly_given |= IDENT_MAIL_GIVEN; > return fmt_ident(getenv("GIT_COMMITTER_NAME"), > getenv("GIT_COMMITTER_EMAIL"), > + WANT_COMMITTER_IDENT, > getenv("GIT_COMMITTER_DATE"), > flag); > } > @@ -480,6 +515,46 @@ int git_ident_config(const char *var, const char *value, void *data) > return 0; > } > > + if (!strcmp(var, "author.name")) { > + if (!value) > + return config_error_nonbool(var); > + strbuf_reset(&git_author_name); > + strbuf_addstr(&git_author_name, value); > + author_ident_explicitly_given |= IDENT_NAME_GIVEN; > + ident_config_given |= IDENT_NAME_GIVEN; > + return 0; > + } > + > + if (!strcmp(var, "author.email")) { > + if (!value) > + return config_error_nonbool(var); > + strbuf_reset(&git_author_email); > + strbuf_addstr(&git_author_email, value); > + author_ident_explicitly_given |= IDENT_MAIL_GIVEN; > + ident_config_given |= IDENT_MAIL_GIVEN; > + return 0; > + } > + > + if (!strcmp(var, "committer.name")) { > + if (!value) > + return config_error_nonbool(var); > + strbuf_reset(&git_committer_name); > + strbuf_addstr(&git_committer_name, value); > + committer_ident_explicitly_given |= IDENT_NAME_GIVEN; > + ident_config_given |= IDENT_NAME_GIVEN; > + return 0; > + } > + > + if (!strcmp(var, "committer.email")) { > + if (!value) > + return config_error_nonbool(var); > + strbuf_reset(&git_committer_email); > + strbuf_addstr(&git_committer_email, value); > + committer_ident_explicitly_given |= IDENT_MAIL_GIVEN; > + ident_config_given |= IDENT_MAIL_GIVEN; > + return 0; > + } > + This whole thing should be split into a static function. It's the same code copy/pasted 4x times just with a differnet value for "var", the strbuf variable & IDENT_*_GIVEN. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 1/2] config: allow giving separate author and committer idents 2019-01-25 22:58 ` Ævar Arnfjörð Bjarmason @ 2019-01-28 18:58 ` William Hubbs 2019-01-28 19:00 ` Junio C Hamano 2019-01-28 20:04 ` Ævar Arnfjörð Bjarmason 2019-01-29 22:42 ` William Hubbs 1 sibling, 2 replies; 15+ messages in thread From: William Hubbs @ 2019-01-28 18:58 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason; +Cc: git, chutzpah, williamh On Fri, Jan 25, 2019 at 11:58:10PM +0100, Ævar Arnfjörð Bjarmason wrote: > > On Fri, Jan 25 2019, William Hubbs wrote: > > > diff --git a/Documentation/config/user.txt b/Documentation/config/user.txt > > index b5b2ba1199..18e1ec3c1b 100644 > > --- a/Documentation/config/user.txt > > +++ b/Documentation/config/user.txt > > @@ -1,12 +1,39 @@ > > +author.email:: > > + The email address used for the author of newly > > + created commits. Defaults to the value of the > > + `GIT_AUTHOR_EMAIL` environment variable, or if > > + the environment variable is not set, the `user.email` > > + configuration variable. > > + > > +author.name:: > > + The full name used for the author of newly created commits. > > + Defaults to the value of the `GIT_AUTHOR_NAME` environment variable, or > > + if the environment variable is not set, > > + the `user.email` configuration variable. > > + > > +committer.email:: > > + The email address used for the committer of newly created commits. > > + Defaults to the value of the `GIT_COMMITTER_EMAIL` environment > > + variable, or if the environment variable is not set, the `user.email` > > + configuration variable. > > + > > +committer.name:: > > + The full name used for the committer of newly created commits. > > + Defaults to the value of the `GIT_COMMITTER_NAME` environment > > + variable, or if the environment variable is not set, the `user.name` > > + configuration variable. > > + > > user.email:: > > Your email address to be recorded in any newly created commits. > > Can be overridden by the `GIT_AUTHOR_EMAIL`, `GIT_COMMITTER_EMAIL`, and > > - `EMAIL` environment variables. See linkgit:git-commit-tree[1]. > > + `EMAIL` environment variables or the `author.email` or > > + `committer.email` settings discussed above. See linkgit:git-commit-tree[1]. > > > > user.name:: > > Your full name to be recorded in any newly created commits. > > Can be overridden by the `GIT_AUTHOR_NAME` and `GIT_COMMITTER_NAME` > > - environment variables. See linkgit:git-commit-tree[1]. > > + environment variables or the `author.name` or `committer.name` > > + settings discussed above. See linkgit:git-commit-tree[1]. > > Looks correct, although I wonder if we're at the point where it would be > better to present this info as a table. Maybe, but can we have someone do that in a separate patch? I ask because the documentation is not in a markup language and that would make setting up a table difficult for me at best with my screen reader. > > diff --git a/builtin/am.c b/builtin/am.c > > index 95370313b6..53fdd22c45 100644 > > --- a/builtin/am.c > > +++ b/builtin/am.c > > @@ -1594,7 +1594,7 @@ static void do_commit(const struct am_state *state) > > } > > > > author = fmt_ident(state->author_name, state->author_email, > > - state->ignore_date ? NULL : state->author_date, > > + WANT_AUTHOR_IDENT, state->ignore_date ? NULL : state->author_date, > > This & a few other things in this series take the code beyond 79 > characters. This doesn't look like it is beyond 79 characters to me, but that may be because I use a tab stop width of 4. Can you reply again and flag the lines that are longer than 79 characters? Thanks, William ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 1/2] config: allow giving separate author and committer idents 2019-01-28 18:58 ` William Hubbs @ 2019-01-28 19:00 ` Junio C Hamano 2019-01-28 20:04 ` Ævar Arnfjörð Bjarmason 1 sibling, 0 replies; 15+ messages in thread From: Junio C Hamano @ 2019-01-28 19:00 UTC (permalink / raw) To: William Hubbs; +Cc: Ævar Arnfjörð Bjarmason, git, chutzpah William Hubbs <williamh@gentoo.org> writes: > This doesn't look like it is beyond 79 characters to me, but that may be > because I use a tab stop width of 4. In this project, a tab skips to multiple of 8. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 1/2] config: allow giving separate author and committer idents 2019-01-28 18:58 ` William Hubbs 2019-01-28 19:00 ` Junio C Hamano @ 2019-01-28 20:04 ` Ævar Arnfjörð Bjarmason 2019-01-28 21:33 ` Junio C Hamano 2019-01-28 23:30 ` William Hubbs 1 sibling, 2 replies; 15+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2019-01-28 20:04 UTC (permalink / raw) To: William Hubbs; +Cc: git, chutzpah On Mon, Jan 28 2019, William Hubbs wrote: > On Fri, Jan 25, 2019 at 11:58:10PM +0100, Ævar Arnfjörð Bjarmason wrote: >> >> On Fri, Jan 25 2019, William Hubbs wrote: >> >> > diff --git a/Documentation/config/user.txt b/Documentation/config/user.txt >> > index b5b2ba1199..18e1ec3c1b 100644 >> > --- a/Documentation/config/user.txt >> > +++ b/Documentation/config/user.txt >> > @@ -1,12 +1,39 @@ >> > +author.email:: >> > + The email address used for the author of newly >> > + created commits. Defaults to the value of the >> > + `GIT_AUTHOR_EMAIL` environment variable, or if >> > + the environment variable is not set, the `user.email` >> > + configuration variable. >> > + >> > +author.name:: >> > + The full name used for the author of newly created commits. >> > + Defaults to the value of the `GIT_AUTHOR_NAME` environment variable, or >> > + if the environment variable is not set, >> > + the `user.email` configuration variable. >> > + >> > +committer.email:: >> > + The email address used for the committer of newly created commits. >> > + Defaults to the value of the `GIT_COMMITTER_EMAIL` environment >> > + variable, or if the environment variable is not set, the `user.email` >> > + configuration variable. >> > + >> > +committer.name:: >> > + The full name used for the committer of newly created commits. >> > + Defaults to the value of the `GIT_COMMITTER_NAME` environment >> > + variable, or if the environment variable is not set, the `user.name` >> > + configuration variable. >> > + >> > user.email:: >> > Your email address to be recorded in any newly created commits. >> > Can be overridden by the `GIT_AUTHOR_EMAIL`, `GIT_COMMITTER_EMAIL`, and >> > - `EMAIL` environment variables. See linkgit:git-commit-tree[1]. >> > + `EMAIL` environment variables or the `author.email` or >> > + `committer.email` settings discussed above. See linkgit:git-commit-tree[1]. >> > >> > user.name:: >> > Your full name to be recorded in any newly created commits. >> > Can be overridden by the `GIT_AUTHOR_NAME` and `GIT_COMMITTER_NAME` >> > - environment variables. See linkgit:git-commit-tree[1]. >> > + environment variables or the `author.name` or `committer.name` >> > + settings discussed above. See linkgit:git-commit-tree[1]. >> >> Looks correct, although I wonder if we're at the point where it would be >> better to present this info as a table. > > Maybe, but can we have someone do that in a separate patch? I ask > because the documentation is not in a markup language and that would > make setting up a table difficult for me at best with my screen reader. I'm happy to help if you'd like. I had a thinko with "table", and I think our asciidoc dialect doesn't support it (maybe I'm wrong), but thinking about it again we could just describe these variables all in the same documentation. As in this hunk (which you could squash in): BEGIN QUOTE diff --git a/Documentation/config/user.txt b/Documentation/config/user.txt index 18e1ec3c1b..ad3c43cf47 100644 --- a/Documentation/config/user.txt +++ b/Documentation/config/user.txt @@ -1,39 +1,20 @@ -author.email:: - The email address used for the author of newly - created commits. Defaults to the value of the - `GIT_AUTHOR_EMAIL` environment variable, or if - the environment variable is not set, the `user.email` - configuration variable. - +user.name:: +user.email:: author.name:: - The full name used for the author of newly created commits. - Defaults to the value of the `GIT_AUTHOR_NAME` environment variable, or - if the environment variable is not set, - the `user.email` configuration variable. - -committer.email:: - The email address used for the committer of newly created commits. - Defaults to the value of the `GIT_COMMITTER_EMAIL` environment - variable, or if the environment variable is not set, the `user.email` - configuration variable. - +author.email:: committer.name:: - The full name used for the committer of newly created commits. - Defaults to the value of the `GIT_COMMITTER_NAME` environment - variable, or if the environment variable is not set, the `user.name` - configuration variable. - -user.email:: - Your email address to be recorded in any newly created commits. - Can be overridden by the `GIT_AUTHOR_EMAIL`, `GIT_COMMITTER_EMAIL`, and - `EMAIL` environment variables or the `author.email` or - `committer.email` settings discussed above. See linkgit:git-commit-tree[1]. - -user.name:: - Your full name to be recorded in any newly created commits. - Can be overridden by the `GIT_AUTHOR_NAME` and `GIT_COMMITTER_NAME` - environment variables or the `author.name` or `committer.name` - settings discussed above. See linkgit:git-commit-tree[1]. +committer.email:: + The `user.name` and `user.email` variables determine what ends + up in the `author` and `committer` field of commit + objects. These config variables will be overridden by + `GIT_COMMITTER_NAME` and `GIT_COMMITTER_EMAIL`, ++ +Most users should have no reason to set the `author.*` and +`committer.*` variables, but can do so to e.g. set different a +different E-Mail for the `committer` field. Like the `user.name` and +`user.email` variables, these can be overridden in the environment +with `GIT_AUTHOR_NAME`, `GIT_AUTHOR_EMAIL`, `GIT_COMMITTER_NAME` and +`GIT_COMMITTER_EMAIL`. user.useConfigOnly:: Instruct Git to avoid trying to guess defaults for `user.email` END QUOTE Another thing I spotted while hacking that up is that the git-commit-tree docs we're pointing to as the full explanation haven't been updated. They still just talk about user.{name,email}. And poking at this a bit more I see that something about this is introducing new edge cases into our "you haven't set user.name or user.email" logic. I.e. if I do: GIT_AUTHOR_NAME=hi GIT_AUTHOR_EMAIL=blah GIT_COMMITTER_NAME="hello" ./git-commit -a -m"hi" I end up with an object like: author hi <blah> 1548705397 +0100 committer hello <avar@nix.is> 1548705397 +0100 I.e. here I haven't supplied the committer E-Mail but it was inferred (and a warning was printed to that effect). But if I do the same thing with setting author.name etc. for all fields except committer.email I'll get an empty ("<>") field for the committer E-Mail, even though it printed "Your name and email address were configured automatically based on your username and hostname". >> > diff --git a/builtin/am.c b/builtin/am.c >> > index 95370313b6..53fdd22c45 100644 >> > --- a/builtin/am.c >> > +++ b/builtin/am.c >> > @@ -1594,7 +1594,7 @@ static void do_commit(const struct am_state *state) >> > } >> > >> > author = fmt_ident(state->author_name, state->author_email, >> > - state->ignore_date ? NULL : state->author_date, >> > + WANT_AUTHOR_IDENT, state->ignore_date ? NULL : state->author_date, >> >> This & a few other things in this series take the code beyond 79 >> characters. > > This doesn't look like it is beyond 79 characters to me, but that may be > because I use a tab stop width of 4. > > Can you reply again and flag the lines that are longer than 79 > characters? I see Junio replied to this already. Adjusting for limited time, I'm happy to help out with this series, particularly if you have visual (screen reader) issues that make some of this prohibitive for you. Just say what you need. ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v2 1/2] config: allow giving separate author and committer idents 2019-01-28 20:04 ` Ævar Arnfjörð Bjarmason @ 2019-01-28 21:33 ` Junio C Hamano 2019-01-28 23:30 ` William Hubbs 1 sibling, 0 replies; 15+ messages in thread From: Junio C Hamano @ 2019-01-28 21:33 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason; +Cc: William Hubbs, git, chutzpah Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > I'm happy to help if you'd like. I had a thinko with "table", and I > think our asciidoc dialect doesn't support it (maybe I'm wrong), but > thinking about it again we could just describe these variables all in > the same documentation. As in this hunk (which you could squash in): > > BEGIN QUOTE > diff --git a/Documentation/config/user.txt b/Documentation/config/user.txt > index 18e1ec3c1b..ad3c43cf47 100644 > --- a/Documentation/config/user.txt > +++ b/Documentation/config/user.txt > @@ -1,39 +1,20 @@ > +user.name:: > +user.email:: > author.name:: > +author.email:: > committer.name:: > +committer.email:: > + The `user.name` and `user.email` variables determine what ends > + up in the `author` and `committer` field of commit > + objects. These config variables will be overridden by > + `GIT_COMMITTER_NAME` and `GIT_COMMITTER_EMAIL`, You forgot to list two more obvious ones here. > ++ > +Most users should have no reason to set the `author.*` and > +`committer.*` variables, but can do so to e.g. set different a > +different E-Mail for the `committer` field. Like the `user.name` and > +`user.email` variables, these can be overridden in the environment > +with `GIT_AUTHOR_NAME`, `GIT_AUTHOR_EMAIL`, `GIT_COMMITTER_NAME` and > +`GIT_COMMITTER_EMAIL`. I do not see a strong reason to say "most users should have no reason", especially without arguing why (and I do not think this is a place to make such an argument, either). The `user.*` variables can be used to set both author and committer names and e-mail addresses to the same value; users who want to set the committer and author identities differently can use the `author.*` and `committer.*` variables. or something along that line, perhaps? ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 1/2] config: allow giving separate author and committer idents 2019-01-28 20:04 ` Ævar Arnfjörð Bjarmason 2019-01-28 21:33 ` Junio C Hamano @ 2019-01-28 23:30 ` William Hubbs 1 sibling, 0 replies; 15+ messages in thread From: William Hubbs @ 2019-01-28 23:30 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason; +Cc: git, chutzpah, williamh On Mon, Jan 28, 2019 at 09:04:15PM +0100, Ævar Arnfjörð Bjarmason wrote: > > Adjusting for limited time, I'm happy to help out with this series, > particularly if you have visual (screen reader) issues that make some of > this prohibitive for you. Just say what you need. I guess the best way to handle formatting issues would be, during your normal review, make a note directly below the affected line about what is wrong with the formatting and I'll fix it during the next pass. Or, another option would be to reply with a diff that I can apply on top of my patch. Thanks, William ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 1/2] config: allow giving separate author and committer idents 2019-01-25 22:58 ` Ævar Arnfjörð Bjarmason 2019-01-28 18:58 ` William Hubbs @ 2019-01-29 22:42 ` William Hubbs 1 sibling, 0 replies; 15+ messages in thread From: William Hubbs @ 2019-01-29 22:42 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason; +Cc: git, chutzpah, williamh On Fri, Jan 25, 2019 at 11:58:10PM +0100, Ævar Arnfjörð Bjarmason wrote: > > On Fri, Jan 25 2019, William Hubbs wrote: > > > @@ -480,6 +515,46 @@ int git_ident_config(const char *var, const char *value, void *data) > > return 0; > > } > > > > + if (!strcmp(var, "author.name")) { > > + if (!value) > > + return config_error_nonbool(var); > > + strbuf_reset(&git_author_name); > > + strbuf_addstr(&git_author_name, value); > > + author_ident_explicitly_given |= IDENT_NAME_GIVEN; > > + ident_config_given |= IDENT_NAME_GIVEN; > > + return 0; > > + } > > + > > + if (!strcmp(var, "author.email")) { > > + if (!value) > > + return config_error_nonbool(var); > > + strbuf_reset(&git_author_email); > > + strbuf_addstr(&git_author_email, value); > > + author_ident_explicitly_given |= IDENT_MAIL_GIVEN; > > + ident_config_given |= IDENT_MAIL_GIVEN; > > + return 0; > > + } > > + > > + if (!strcmp(var, "committer.name")) { > > + if (!value) > > + return config_error_nonbool(var); > > + strbuf_reset(&git_committer_name); > > + strbuf_addstr(&git_committer_name, value); > > + committer_ident_explicitly_given |= IDENT_NAME_GIVEN; > > + ident_config_given |= IDENT_NAME_GIVEN; > > + return 0; > > + } > > + > > + if (!strcmp(var, "committer.email")) { > > + if (!value) > > + return config_error_nonbool(var); > > + strbuf_reset(&git_committer_email); > > + strbuf_addstr(&git_committer_email, value); > > + committer_ident_explicitly_given |= IDENT_MAIL_GIVEN; > > + ident_config_given |= IDENT_MAIL_GIVEN; > > + return 0; > > + } > > + > > This whole thing should be split into a static function. It's the same > code copy/pasted 4x times just with a differnet value for "var", the > strbuf variable & IDENT_*_GIVEN. I have moved most of this into a separate function in the next version of the patch. However, I do not see a way to factor it down further. Let me know what you think when I resend. Also, if you see anything longer than 79 characters, please let me know where the long lines are and I have no problem reformatting them. Thanks much. William ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v2 2/2] tests: add test for separate author and committer idents 2019-01-25 21:59 Add author and committer configuration settings William Hubbs 2019-01-25 21:59 ` [PATCH v2 1/2] config: allow giving separate author and committer idents William Hubbs @ 2019-01-25 21:59 ` William Hubbs 2019-01-25 23:05 ` Ævar Arnfjörð Bjarmason 1 sibling, 1 reply; 15+ messages in thread From: William Hubbs @ 2019-01-25 21:59 UTC (permalink / raw) To: git; +Cc: williamh, chutzpah Signed-off-by: William Hubbs <williamh@gentoo.org> --- t/t7517-per-repo-email.sh | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/t/t7517-per-repo-email.sh b/t/t7517-per-repo-email.sh index 231b8cc19d..06c7c0fb78 100755 --- a/t/t7517-per-repo-email.sh +++ b/t/t7517-per-repo-email.sh @@ -85,4 +85,21 @@ test_expect_success REBASE_P \ test_must_fail git rebase -p master ' +test_expect_success \ + 'author and committer config settings override user config settings' ' + sane_unset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL && + sane_unset GIT_COMMITTER_NAME GIT_COMMITTER_EMAIL && + git config user.name user && + git config user.email user@example.com && + git config author.name author && + git config author.email author@example.com && + git config committer.name committer && + git config committer.email committer@example.com && + test_commit config-names && + [ "$(git log --format=%an -1)" = "author" ] && + [ "$(git log --format=%ae -1)" = "author@example.com" ] && + [ "$(git log --format=%cn -1)" = "committer" ] && + [ "$(git log --format=%ce -1)" = "committer@example.com" ] +' + test_done -- 2.19.2 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v2 2/2] tests: add test for separate author and committer idents 2019-01-25 21:59 ` [PATCH v2 2/2] tests: add test for " William Hubbs @ 2019-01-25 23:05 ` Ævar Arnfjörð Bjarmason 2019-01-26 1:06 ` William Hubbs 0 siblings, 1 reply; 15+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2019-01-25 23:05 UTC (permalink / raw) To: William Hubbs; +Cc: git, chutzpah On Fri, Jan 25 2019, William Hubbs wrote: > Signed-off-by: William Hubbs <williamh@gentoo.org> > --- > t/t7517-per-repo-email.sh | 17 +++++++++++++++++ > 1 file changed, 17 insertions(+) > > diff --git a/t/t7517-per-repo-email.sh b/t/t7517-per-repo-email.sh > index 231b8cc19d..06c7c0fb78 100755 > --- a/t/t7517-per-repo-email.sh > +++ b/t/t7517-per-repo-email.sh > @@ -85,4 +85,21 @@ test_expect_success REBASE_P \ > test_must_fail git rebase -p master > ' Let's include this in the main patch. We don't split up tests into their own patches like this. > +test_expect_success \ > + 'author and committer config settings override user config settings' ' This can just be on one line. We're not strict about 79 characters in tests. > + sane_unset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL && > + sane_unset GIT_COMMITTER_NAME GIT_COMMITTER_EMAIL && Fine, but FYI sets these variables for the rest of the test. But more importantly there should be a test for how the various override interactions between the config & env variables work. I.e. whether GIT_COMMITTER_NAME set in the env will override "user.email" etc. > + git config user.name user && > + git config user.email user@example.com && > + git config author.name author && > + git config author.email author@example.com && > + git config committer.name committer && > + git config committer.email committer@example.com && This should use "test_config" so it'll be unset after this test. > + test_commit config-names && > + [ "$(git log --format=%an -1)" = "author" ] && > + [ "$(git log --format=%ae -1)" = "author@example.com" ] && > + [ "$(git log --format=%cn -1)" = "committer" ] && > + [ "$(git log --format=%ce -1)" = "committer@example.com" ] Should use something like test_cmp so that on failure we see what the difference is. I'd just do: cat >expected <<EOF... && git log --format="an:%an%nae:%ae[...]" -1 >actual && test_cmp ... > +' > + > test_done ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 2/2] tests: add test for separate author and committer idents 2019-01-25 23:05 ` Ævar Arnfjörð Bjarmason @ 2019-01-26 1:06 ` William Hubbs 2019-01-26 8:53 ` Ævar Arnfjörð Bjarmason 0 siblings, 1 reply; 15+ messages in thread From: William Hubbs @ 2019-01-26 1:06 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason; +Cc: git, chutzpah [-- Attachment #1: Type: text/plain, Size: 614 bytes --] On Sat, Jan 26, 2019 at 12:05:08AM +0100, Ævar Arnfjörð Bjarmason wrote: > > On Fri, Jan 25 2019, William Hubbs wrote: ... > > + sane_unset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL && > > + sane_unset GIT_COMMITTER_NAME GIT_COMMITTER_EMAIL && > > Fine, but FYI sets these variables for the rest of the test. I'm not quite sure what you mean by this. I want the environment variables to be *unset*. I don't want them to override anything in the config file for this test. Are you saying they will not be set for the test unless I set them, so I don't need the SANE_UNSET calls? Thanks, William [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 195 bytes --] ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 2/2] tests: add test for separate author and committer idents 2019-01-26 1:06 ` William Hubbs @ 2019-01-26 8:53 ` Ævar Arnfjörð Bjarmason 2019-01-27 4:48 ` Eric Sunshine 0 siblings, 1 reply; 15+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2019-01-26 8:53 UTC (permalink / raw) To: William Hubbs; +Cc: git, chutzpah On Sat, Jan 26 2019, William Hubbs wrote: > On Sat, Jan 26, 2019 at 12:05:08AM +0100, Ævar Arnfjörð Bjarmason wrote: >> >> On Fri, Jan 25 2019, William Hubbs wrote: > > ... > >> > + sane_unset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL && >> > + sane_unset GIT_COMMITTER_NAME GIT_COMMITTER_EMAIL && >> >> Fine, but FYI sets these variables for the rest of the test. > > I'm not quite sure what you mean by this. I want the environment > variables to be *unset*. I don't want them to override anything in the > config file for this test. > > Are you saying they will not be set for the test unless I set them, > so I don't need the SANE_UNSET calls? Sorry for not being clear. I just meant that unlike "test_config" the "sane_unset" function won't reset the state at the end of the "test_expect_success". Right now it doesn't matter in practice since this is the last test before "test_done", but as tests are added we tend to leak state between them, which is why we use these "unset at the end" helper functions. But unlike with config that doesn't matter in this case, since we want these unset anyway. Which, looking at this again, you'd only want if a previous test in the file was leaking its state. That's not the case, so this isn't needed and you can just apply this on top: diff --git a/t/t7517-per-repo-email.sh b/t/t7517-per-repo-email.sh index 06c7c0fb78..e5845b0b86 100755 --- a/t/t7517-per-repo-email.sh +++ b/t/t7517-per-repo-email.sh @@ -87,8 +87,6 @@ test_expect_success REBASE_P \ test_expect_success \ 'author and committer config settings override user config settings' ' - sane_unset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL && - sane_unset GIT_COMMITTER_NAME GIT_COMMITTER_EMAIL && git config user.name user && git config user.email user@example.com && git config author.name author && You don't need to be paranoid and unset these, we already unset GIT_* variables that aren't on a whitelist in test-lib.sh, see 'A call to "unset"' there. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 2/2] tests: add test for separate author and committer idents 2019-01-26 8:53 ` Ævar Arnfjörð Bjarmason @ 2019-01-27 4:48 ` Eric Sunshine 2019-01-28 19:05 ` Ævar Arnfjörð Bjarmason 0 siblings, 1 reply; 15+ messages in thread From: Eric Sunshine @ 2019-01-27 4:48 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason; +Cc: William Hubbs, Git List, chutzpah On Sat, Jan 26, 2019 at 3:53 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > Which, looking at this again, you'd only want if a previous test in the > file was leaking its state. That's not the case, so this isn't needed > and you can just apply this on top: > > test_expect_success \ > 'author and committer config settings override user config settings' ' > - sane_unset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL && > - sane_unset GIT_COMMITTER_NAME GIT_COMMITTER_EMAIL && > git config user.name user && > git config user.email user@example.com && > git config author.name author && Aside from future-proofing against a test being inserted before this one which does set those environment variables, these invocations of sane_unset() serve the additional purpose of documenting the interplay of configuration and environment, and further indicate to readers that the test author took this into consideration (rather than merely slapping together the test without thought). As a reviewer and reader of the test, I appreciate the additional context the sane_unset() calls provide, thus think it makes sense to retain them. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 2/2] tests: add test for separate author and committer idents 2019-01-27 4:48 ` Eric Sunshine @ 2019-01-28 19:05 ` Ævar Arnfjörð Bjarmason 0 siblings, 0 replies; 15+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2019-01-28 19:05 UTC (permalink / raw) To: Eric Sunshine; +Cc: William Hubbs, Git List, chutzpah On Sun, Jan 27 2019, Eric Sunshine wrote: > On Sat, Jan 26, 2019 at 3:53 AM Ævar Arnfjörð Bjarmason > <avarab@gmail.com> wrote: >> Which, looking at this again, you'd only want if a previous test in the >> file was leaking its state. That's not the case, so this isn't needed >> and you can just apply this on top: >> >> test_expect_success \ >> 'author and committer config settings override user config settings' ' >> - sane_unset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL && >> - sane_unset GIT_COMMITTER_NAME GIT_COMMITTER_EMAIL && >> git config user.name user && >> git config user.email user@example.com && >> git config author.name author && > > Aside from future-proofing against a test being inserted before this > one which does set those environment variables, these invocations of > sane_unset() serve the additional purpose of documenting the interplay > of configuration and environment, and further indicate to readers that > the test author took this into consideration (rather than merely > slapping together the test without thought). As a reviewer and reader > of the test, I appreciate the additional context the sane_unset() > calls provide, thus think it makes sense to retain them. As noted in <875zuc49uj.fsf@evledraar.gmail.com> ("various override interactions") there should definitely be more tests where the combination of config & env is tested for. But I don't see how it makes things clearer to unset a bunch of variables previous tests didn't set. If we applied that to our test suite much of it would be pointlessly unsetting various GIT_* variables. Better to assume other tests have cleaned up their own state, and when it's not the case fix it. ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2019-01-29 22:42 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-01-25 21:59 Add author and committer configuration settings William Hubbs 2019-01-25 21:59 ` [PATCH v2 1/2] config: allow giving separate author and committer idents William Hubbs 2019-01-25 22:58 ` Ævar Arnfjörð Bjarmason 2019-01-28 18:58 ` William Hubbs 2019-01-28 19:00 ` Junio C Hamano 2019-01-28 20:04 ` Ævar Arnfjörð Bjarmason 2019-01-28 21:33 ` Junio C Hamano 2019-01-28 23:30 ` William Hubbs 2019-01-29 22:42 ` William Hubbs 2019-01-25 21:59 ` [PATCH v2 2/2] tests: add test for " William Hubbs 2019-01-25 23:05 ` Ævar Arnfjörð Bjarmason 2019-01-26 1:06 ` William Hubbs 2019-01-26 8:53 ` Ævar Arnfjörð Bjarmason 2019-01-27 4:48 ` Eric Sunshine 2019-01-28 19:05 ` Ævar Arnfjörð Bjarmason
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).