* [PATCH 1/6] ident: make user_ident_explicitly_given private
2012-11-13 16:48 ` Jeff King
@ 2012-11-13 16:49 ` Jeff King
2012-11-14 16:44 ` Jonathan Nieder
2012-11-13 16:52 ` [PATCH 2/6] ident: keep separate "explicit" flags for author and committer Jeff King
` (6 subsequent siblings)
7 siblings, 1 reply; 76+ messages in thread
From: Jeff King @ 2012-11-13 16:49 UTC (permalink / raw)
To: Felipe Contreras; +Cc: git, Thomas Rast, Junio C Hamano, Jonathan Nieder
There are no users of this global variable, as queriers
go through the user_ident_sufficiently_given accessor.
Let's make it private, which will enable further
refactoring.
Signed-off-by: Jeff King <peff@peff.net>
---
cache.h | 4 ----
ident.c | 6 +++++-
2 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/cache.h b/cache.h
index dbd8018..50d9eea 100644
--- a/cache.h
+++ b/cache.h
@@ -1149,10 +1149,6 @@ struct config_include_data {
#define CONFIG_INCLUDE_INIT { 0 }
extern int git_config_include(const char *name, const char *value, void *data);
-#define IDENT_NAME_GIVEN 01
-#define IDENT_MAIL_GIVEN 02
-#define IDENT_ALL_GIVEN (IDENT_NAME_GIVEN|IDENT_MAIL_GIVEN)
-extern int user_ident_explicitly_given;
extern int user_ident_sufficiently_given(void);
extern const char *git_commit_encoding;
diff --git a/ident.c b/ident.c
index a4bf206..733d69d 100644
--- a/ident.c
+++ b/ident.c
@@ -10,7 +10,11 @@
static struct strbuf git_default_name = STRBUF_INIT;
static struct strbuf git_default_email = STRBUF_INIT;
static char git_default_date[50];
-int user_ident_explicitly_given;
+
+#define IDENT_NAME_GIVEN 01
+#define IDENT_MAIL_GIVEN 02
+#define IDENT_ALL_GIVEN (IDENT_NAME_GIVEN|IDENT_MAIL_GIVEN)
+static int user_ident_explicitly_given;
#ifdef NO_GECOS_IN_PWENT
#define get_gecos(ignored) "&"
--
1.8.0.207.gdf2154c
^ permalink raw reply related [flat|nested] 76+ messages in thread* Re: [PATCH 1/6] ident: make user_ident_explicitly_given private
2012-11-13 16:49 ` [PATCH 1/6] ident: make user_ident_explicitly_given private Jeff King
@ 2012-11-14 16:44 ` Jonathan Nieder
2012-11-14 19:11 ` Jeff King
0 siblings, 1 reply; 76+ messages in thread
From: Jonathan Nieder @ 2012-11-14 16:44 UTC (permalink / raw)
To: Jeff King
Cc: Felipe Contreras, git, Thomas Rast, Junio C Hamano,
Santi Béjar
Jeff King wrote:
> There are no users of this global variable, as queriers
> go through the user_ident_sufficiently_given accessor.
> Let's make it private, which will enable further
> refactoring.
[...]
> --- a/cache.h
> +++ b/cache.h
> @@ -1149,10 +1149,6 @@ struct config_include_data {
> #define CONFIG_INCLUDE_INIT { 0 }
> extern int git_config_include(const char *name, const char *value, void *data);
>
> -#define IDENT_NAME_GIVEN 01
> -#define IDENT_MAIL_GIVEN 02
> -#define IDENT_ALL_GIVEN (IDENT_NAME_GIVEN|IDENT_MAIL_GIVEN)
> -extern int user_ident_explicitly_given;
> extern int user_ident_sufficiently_given(void);
In v1.5.6-rc0~56^2 (2008-05-04) "user_ident_explicitly_given" was
introduced as a global for communication between config, ident, and
builtin-commit. In v1.7.0-rc0~72^2 (2010-01-07) readers switched to
using the common wrapper user_ident_sufficiently_given(). After
v1.7.11-rc1~15^2~18 (2012-05-21) the var is only written in ident.c,
and the variable can finally be made static.
This patch finally does that, which is a nice way to make cache.h
easier to read and change less often.
For what it's worth,
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
^ permalink raw reply [flat|nested] 76+ messages in thread* Re: [PATCH 1/6] ident: make user_ident_explicitly_given private
2012-11-14 16:44 ` Jonathan Nieder
@ 2012-11-14 19:11 ` Jeff King
0 siblings, 0 replies; 76+ messages in thread
From: Jeff King @ 2012-11-14 19:11 UTC (permalink / raw)
To: Jonathan Nieder
Cc: Felipe Contreras, git, Thomas Rast, Junio C Hamano,
Santi Béjar
On Wed, Nov 14, 2012 at 08:44:57AM -0800, Jonathan Nieder wrote:
> > -#define IDENT_NAME_GIVEN 01
> > -#define IDENT_MAIL_GIVEN 02
> > -#define IDENT_ALL_GIVEN (IDENT_NAME_GIVEN|IDENT_MAIL_GIVEN)
> > -extern int user_ident_explicitly_given;
> > extern int user_ident_sufficiently_given(void);
>
> In v1.5.6-rc0~56^2 (2008-05-04) "user_ident_explicitly_given" was
> introduced as a global for communication between config, ident, and
> builtin-commit. In v1.7.0-rc0~72^2 (2010-01-07) readers switched to
> using the common wrapper user_ident_sufficiently_given(). After
> v1.7.11-rc1~15^2~18 (2012-05-21) the var is only written in ident.c,
> and the variable can finally be made static.
Thanks for digging up the history. I remembered trying to do this before
during the ident refactoring that went into v1.7.11, but it didn't work
then for some reason (I think it was probably an earlier iteration of
the series, and then I forgot to revisit it during the final one).
I'll add your explanation to the commit message if I re-roll.
-Peff
^ permalink raw reply [flat|nested] 76+ messages in thread
* [PATCH 2/6] ident: keep separate "explicit" flags for author and committer
2012-11-13 16:48 ` Jeff King
2012-11-13 16:49 ` [PATCH 1/6] ident: make user_ident_explicitly_given private Jeff King
@ 2012-11-13 16:52 ` Jeff King
2012-11-13 16:52 ` [PATCH 3/6] var: accept multiple variables on the command line Jeff King
` (5 subsequent siblings)
7 siblings, 0 replies; 76+ messages in thread
From: Jeff King @ 2012-11-13 16:52 UTC (permalink / raw)
To: Felipe Contreras; +Cc: git, Thomas Rast, Junio C Hamano, Jonathan Nieder
We keep track of whether the user ident was given to us
explicitly, or if we guessed at it from system parameters
like username and hostname. However, we kept only a single
variable. This covers the common cases (because the author
and committer will usually come from the same explicit
source), but can miss two cases:
1. GIT_COMMITTER_* is set explicitly, but we fallback for
GIT_AUTHOR. We claim the ident is explicit, even though
the author is not.
2. GIT_AUTHOR_* is set and we ask for author ident, but
not committer ident. We will claim the ident is
implicit, even though it is explicit.
This patch uses two variables instead of one, updates both
when we set the "fallback" values, and updates them
individually when we read from the environment.
Rather than keep user_ident_sufficiently_given as a
compatibility wrapper, we update the only two callers to
check the committer_ident, which matches their intent and
what was happening already.
Signed-off-by: Jeff King <peff@peff.net>
---
Case 1 made me initially think might need to check
author_ident_sufficiently_given when deciding whether to show the
"Author:" line during commit. But I don't think it is necessary, as we
already check !strcmp(author, committer); if the committer is explicit
and the author is identical, even if it is implicit, there is no point
in telling the user.
builtin/commit.c | 4 ++--
cache.h | 3 ++-
ident.c | 32 +++++++++++++++++++++++++-------
3 files changed, 29 insertions(+), 10 deletions(-)
diff --git a/builtin/commit.c b/builtin/commit.c
index 1dd2ec5..d6dd3df 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -755,7 +755,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
ident_shown++ ? "" : "\n",
author_ident->buf);
- if (!user_ident_sufficiently_given())
+ if (!committer_ident_sufficiently_given())
status_printf_ln(s, GIT_COLOR_NORMAL,
_("%s"
"Committer: %s"),
@@ -1265,7 +1265,7 @@ static void print_summary(const char *prefix, const unsigned char *sha1,
strbuf_addstr(&format, "\n Author: ");
strbuf_addbuf_percentquote(&format, &author_ident);
}
- if (!user_ident_sufficiently_given()) {
+ if (!committer_ident_sufficiently_given()) {
strbuf_addstr(&format, "\n Committer: ");
strbuf_addbuf_percentquote(&format, &committer_ident);
if (advice_implicit_identity) {
diff --git a/cache.h b/cache.h
index 50d9eea..18fdd18 100644
--- a/cache.h
+++ b/cache.h
@@ -1149,7 +1149,8 @@ struct config_include_data {
#define CONFIG_INCLUDE_INIT { 0 }
extern int git_config_include(const char *name, const char *value, void *data);
-extern int user_ident_sufficiently_given(void);
+extern int committer_ident_sufficiently_given(void);
+extern int author_ident_sufficiently_given(void);
extern const char *git_commit_encoding;
extern const char *git_log_output_encoding;
diff --git a/ident.c b/ident.c
index 733d69d..ac9672f 100644
--- a/ident.c
+++ b/ident.c
@@ -14,7 +14,8 @@ static char git_default_date[50];
#define IDENT_NAME_GIVEN 01
#define IDENT_MAIL_GIVEN 02
#define IDENT_ALL_GIVEN (IDENT_NAME_GIVEN|IDENT_MAIL_GIVEN)
-static int user_ident_explicitly_given;
+static int committer_ident_explicitly_given;
+static int author_ident_explicitly_given;
#ifdef NO_GECOS_IN_PWENT
#define get_gecos(ignored) "&"
@@ -113,7 +114,8 @@ const char *ident_default_email(void)
if (email && email[0]) {
strbuf_addstr(&git_default_email, email);
- user_ident_explicitly_given |= IDENT_MAIL_GIVEN;
+ committer_ident_explicitly_given |= IDENT_MAIL_GIVEN;
+ author_ident_explicitly_given |= IDENT_MAIL_GIVEN;
} else
copy_email(xgetpwuid_self(), &git_default_email);
strbuf_trim(&git_default_email);
@@ -331,6 +333,10 @@ const char *fmt_name(const char *name, const char *email)
const char *git_author_info(int flag)
{
+ if (getenv("GIT_AUTHOR_NAME"))
+ author_ident_explicitly_given |= IDENT_NAME_GIVEN;
+ if (getenv("GIT_AUTHOR_EMAIL"))
+ author_ident_explicitly_given |= IDENT_MAIL_GIVEN;
return fmt_ident(getenv("GIT_AUTHOR_NAME"),
getenv("GIT_AUTHOR_EMAIL"),
getenv("GIT_AUTHOR_DATE"),
@@ -340,16 +346,16 @@ const char *git_author_info(int flag)
const char *git_committer_info(int flag)
{
if (getenv("GIT_COMMITTER_NAME"))
- user_ident_explicitly_given |= IDENT_NAME_GIVEN;
+ committer_ident_explicitly_given |= IDENT_NAME_GIVEN;
if (getenv("GIT_COMMITTER_EMAIL"))
- user_ident_explicitly_given |= IDENT_MAIL_GIVEN;
+ committer_ident_explicitly_given |= IDENT_MAIL_GIVEN;
return fmt_ident(getenv("GIT_COMMITTER_NAME"),
getenv("GIT_COMMITTER_EMAIL"),
getenv("GIT_COMMITTER_DATE"),
flag);
}
-int user_ident_sufficiently_given(void)
+static int ident_is_sufficient(int user_ident_explicitly_given)
{
#ifndef WINDOWS
return (user_ident_explicitly_given & IDENT_MAIL_GIVEN);
@@ -358,6 +364,16 @@ int user_ident_sufficiently_given(void)
#endif
}
+int committer_ident_sufficiently_given(void)
+{
+ return ident_is_sufficient(committer_ident_explicitly_given);
+}
+
+int author_ident_sufficiently_given(void)
+{
+ return ident_is_sufficient(author_ident_explicitly_given);
+}
+
int git_ident_config(const char *var, const char *value, void *data)
{
if (!strcmp(var, "user.name")) {
@@ -365,7 +381,8 @@ int git_ident_config(const char *var, const char *value, void *data)
return config_error_nonbool(var);
strbuf_reset(&git_default_name);
strbuf_addstr(&git_default_name, value);
- user_ident_explicitly_given |= IDENT_NAME_GIVEN;
+ committer_ident_explicitly_given |= IDENT_NAME_GIVEN;
+ author_ident_explicitly_given |= IDENT_NAME_GIVEN;
return 0;
}
@@ -374,7 +391,8 @@ int git_ident_config(const char *var, const char *value, void *data)
return config_error_nonbool(var);
strbuf_reset(&git_default_email);
strbuf_addstr(&git_default_email, value);
- user_ident_explicitly_given |= IDENT_MAIL_GIVEN;
+ committer_ident_explicitly_given |= IDENT_MAIL_GIVEN;
+ author_ident_explicitly_given |= IDENT_MAIL_GIVEN;
return 0;
}
--
1.8.0.207.gdf2154c
^ permalink raw reply related [flat|nested] 76+ messages in thread* [PATCH 3/6] var: accept multiple variables on the command line
2012-11-13 16:48 ` Jeff King
2012-11-13 16:49 ` [PATCH 1/6] ident: make user_ident_explicitly_given private Jeff King
2012-11-13 16:52 ` [PATCH 2/6] ident: keep separate "explicit" flags for author and committer Jeff King
@ 2012-11-13 16:52 ` Jeff King
2012-11-14 17:01 ` Jonathan Nieder
2012-11-13 16:53 ` [PATCH 4/6] var: provide explicit/implicit ident information Jeff King
` (4 subsequent siblings)
7 siblings, 1 reply; 76+ messages in thread
From: Jeff King @ 2012-11-13 16:52 UTC (permalink / raw)
To: Felipe Contreras; +Cc: git, Thomas Rast, Junio C Hamano, Jonathan Nieder
Git-var currently only accepts a single value to print. This
is inefficient if the caller is interested in finding
multiple values, as they must invoke git-var multiple times.
This patch lets callers specify multiple variables, and
prints one per line.
Signed-off-by: Jeff King <peff@peff.net>
---
This will later let us get the "explicit" flag for free.
Documentation/git-var.txt | 9 +++++++--
builtin/var.c | 13 +++++++------
t/t0007-git-var.sh | 29 +++++++++++++++++++++++++++++
3 files changed, 43 insertions(+), 8 deletions(-)
create mode 100755 t/t0007-git-var.sh
diff --git a/Documentation/git-var.txt b/Documentation/git-var.txt
index 67edf58..53abba5 100644
--- a/Documentation/git-var.txt
+++ b/Documentation/git-var.txt
@@ -9,11 +9,16 @@ git-var - Show a git logical variable
SYNOPSIS
--------
[verse]
-'git var' ( -l | <variable> )
+'git var' ( -l | <variable>... )
DESCRIPTION
-----------
-Prints a git logical variable.
+Prints one or more git logical variables, separated by newlines.
+
+Note that some variables may contain newlines themselves (e.g.,
+`GIT_EDITOR`), and it is therefore possible to receive ambiguous output
+when requesting multiple variables. This can be mitigated by putting any
+such variables at the end of the list.
OPTIONS
-------
diff --git a/builtin/var.c b/builtin/var.c
index aedbb53..49b48f5 100644
--- a/builtin/var.c
+++ b/builtin/var.c
@@ -73,8 +73,7 @@ static int show_config(const char *var, const char *value, void *cb)
int cmd_var(int argc, const char **argv, const char *prefix)
{
- const char *val = NULL;
- if (argc != 2)
+ if (argc < 2)
usage(var_usage);
if (strcmp(argv[1], "-l") == 0) {
@@ -83,11 +82,13 @@ int cmd_var(int argc, const char **argv, const char *prefix)
return 0;
}
git_config(git_default_config, NULL);
- val = read_var(argv[1]);
- if (!val)
- usage(var_usage);
- printf("%s\n", val);
+ while (*++argv) {
+ const char *val = read_var(*argv);
+ if (!val)
+ usage(var_usage);
+ printf("%s\n", val);
+ }
return 0;
}
diff --git a/t/t0007-git-var.sh b/t/t0007-git-var.sh
new file mode 100755
index 0000000..45a5f66
--- /dev/null
+++ b/t/t0007-git-var.sh
@@ -0,0 +1,29 @@
+#!/bin/sh
+
+test_description='basic sanity checks for git var'
+. ./test-lib.sh
+
+test_expect_success 'get GIT_AUTHOR_IDENT' '
+ test_tick &&
+ echo "A U Thor <author@example.com> 1112911993 -0700" >expect &&
+ git var GIT_AUTHOR_IDENT >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'get GIT_COMMITTER_IDENT' '
+ test_tick &&
+ echo "C O Mitter <committer@example.com> 1112912053 -0700" >expect &&
+ git var GIT_COMMITTER_IDENT >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'git var can show multiple values' '
+ cat >expect <<-\EOF &&
+ A U Thor <author@example.com> 1112912053 -0700
+ C O Mitter <committer@example.com> 1112912053 -0700
+ EOF
+ git var GIT_AUTHOR_IDENT GIT_COMMITTER_IDENT >actual &&
+ test_cmp expect actual
+'
+
+test_done
--
1.8.0.207.gdf2154c
^ permalink raw reply related [flat|nested] 76+ messages in thread* Re: [PATCH 3/6] var: accept multiple variables on the command line
2012-11-13 16:52 ` [PATCH 3/6] var: accept multiple variables on the command line Jeff King
@ 2012-11-14 17:01 ` Jonathan Nieder
2012-11-14 19:26 ` Jeff King
0 siblings, 1 reply; 76+ messages in thread
From: Jonathan Nieder @ 2012-11-14 17:01 UTC (permalink / raw)
To: Jeff King; +Cc: Felipe Contreras, git, Thomas Rast, Junio C Hamano
Jeff King wrote:
> This patch lets callers specify multiple variables, and
> prints one per line.
Yay!
[...]
> --- a/Documentation/git-var.txt
> +++ b/Documentation/git-var.txt
> @@ -9,11 +9,16 @@ git-var - Show a git logical variable
> SYNOPSIS
> --------
> [verse]
> -'git var' ( -l | <variable> )
> +'git var' ( -l | <variable>... )
>
> DESCRIPTION
> -----------
> -Prints a git logical variable.
> +Prints one or more git logical variables, separated by newlines.
> +
> +Note that some variables may contain newlines themselves
Maybe a -z option to NUL-terminate values would be useful some day.
> --- a/builtin/var.c
> +++ b/builtin/var.c
> @@ -73,8 +73,7 @@ static int show_config(const char *var, const char *value, void *cb)
>
> int cmd_var(int argc, const char **argv, const char *prefix)
> {
> - const char *val = NULL;
> - if (argc != 2)
> + if (argc < 2)
> usage(var_usage);
>
> if (strcmp(argv[1], "-l") == 0) {
What should happen if I pass "-l" followed by other arguments?
[...]
> --- /dev/null
> +++ b/t/t0007-git-var.sh
> @@ -0,0 +1,29 @@
> +#!/bin/sh
> +
> +test_description='basic sanity checks for git var'
> +. ./test-lib.sh
> +
> +test_expect_success 'get GIT_AUTHOR_IDENT' '
> + test_tick &&
> + echo "A U Thor <author@example.com> 1112911993 -0700" >expect &&
Do we need to hardcode the timestamp? Something like
test_cmp_filtered () {
expect=$1 actual=$2 &&
sed -e 's/[0-9][0-9]* [-+][0-9][0-9][0-9][0-9]/TIMESTAMP" \
<"$actual" >"$actual.filtered" &&
test_cmp "$expect" "$actual.filtered"
}
...
echo "A U Thor <author@example.com> $timestamp" >expect &&
git var GIT_AUTHOR_IDENT >actual &&
test_cmp_filtered expect actual
should make reordering tests a lot easier, though it has the downside
of not being able to catch a weird bug that would make the timestamp
out of sync with reality.
Hope that helps,
Jonathan
^ permalink raw reply [flat|nested] 76+ messages in thread* Re: [PATCH 3/6] var: accept multiple variables on the command line
2012-11-14 17:01 ` Jonathan Nieder
@ 2012-11-14 19:26 ` Jeff King
0 siblings, 0 replies; 76+ messages in thread
From: Jeff King @ 2012-11-14 19:26 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Felipe Contreras, git, Thomas Rast, Junio C Hamano
On Wed, Nov 14, 2012 at 09:01:48AM -0800, Jonathan Nieder wrote:
> > DESCRIPTION
> > -----------
> > -Prints a git logical variable.
> > +Prints one or more git logical variables, separated by newlines.
> > +
> > +Note that some variables may contain newlines themselves
>
> Maybe a -z option to NUL-terminate values would be useful some day.
Yeah, I thought about that but stopped short. The intended caller in my
series is Git.pm, whose command() splits on newlines. Although it is
perl...I suspect doing:
local $/ = "\0";
my @entries = command(...);
would work. For ident variables, we know they don't contain a newline,
though.
> > --- a/builtin/var.c
> > +++ b/builtin/var.c
> > @@ -73,8 +73,7 @@ static int show_config(const char *var, const char *value, void *cb)
> >
> > int cmd_var(int argc, const char **argv, const char *prefix)
> > {
> > - const char *val = NULL;
> > - if (argc != 2)
> > + if (argc < 2)
> > usage(var_usage);
> >
> > if (strcmp(argv[1], "-l") == 0) {
>
> What should happen if I pass "-l" followed by other arguments?
Good catch. Probably we should just call usage() once we see "-l"
and (argc > 2), which matches the previous behavior. I don't see much
point in listing specific variables after having listed them all.
I was also tempted to convert to parse_options, but I don't think that
really buys us anything (we could detect the option in "git var foo -l
bar", but since we are not going to do anything useful in such a case,
there is not much point).
> > + test_tick &&
> > + echo "A U Thor <author@example.com> 1112911993 -0700" >expect &&
>
> Do we need to hardcode the timestamp? Something like
>
> test_cmp_filtered () {
> expect=$1 actual=$2 &&
> sed -e 's/[0-9][0-9]* [-+][0-9][0-9][0-9][0-9]/TIMESTAMP" \
> <"$actual" >"$actual.filtered" &&
> test_cmp "$expect" "$actual.filtered"
> }
No, we don't have to. I was just hoping to keep the tests simple by not
doing any parsing trickery. The test_tick keeps it stable, but as you
note, it is not robust to reordering. I think it would be sufficient to
just put $GIT_COMMITTER_DATE into the expected output.
I'll fix both in a re-roll.
Thanks.
-Peff
^ permalink raw reply [flat|nested] 76+ messages in thread
* [PATCH 4/6] var: provide explicit/implicit ident information
2012-11-13 16:48 ` Jeff King
` (2 preceding siblings ...)
2012-11-13 16:52 ` [PATCH 3/6] var: accept multiple variables on the command line Jeff King
@ 2012-11-13 16:53 ` Jeff King
2012-11-14 17:06 ` Jonathan Nieder
2012-11-13 16:53 ` [PATCH 5/6] Git.pm: teach "ident" to query explicitness Jeff King
` (3 subsequent siblings)
7 siblings, 1 reply; 76+ messages in thread
From: Jeff King @ 2012-11-13 16:53 UTC (permalink / raw)
To: Felipe Contreras; +Cc: git, Thomas Rast, Junio C Hamano, Jonathan Nieder
Internally, we keep track of whether the author or committer
ident information was provided by the user, or whether it
was implicitly determined by the system. However, there is
currently no way for external programs or scripts to get
this information without re-implementing the ident logic
themselves. Let's provide a way for them to do so.
Signed-off-by: Jeff King <peff@peff.net>
---
Documentation/git-var.txt | 8 ++++++++
builtin/var.c | 27 +++++++++++++++++++++++++++
t/t0007-git-var.sh | 36 ++++++++++++++++++++++++++++++++++++
3 files changed, 71 insertions(+)
diff --git a/Documentation/git-var.txt b/Documentation/git-var.txt
index 53abba5..963b8d4 100644
--- a/Documentation/git-var.txt
+++ b/Documentation/git-var.txt
@@ -39,9 +39,17 @@ VARIABLES
GIT_AUTHOR_IDENT::
The author of a piece of code.
+GIT_AUTHOR_EXPLICIT::
+ Outputs "1" if the author identity was provided explicitly by the
+ user (in the config or environment), or "0" if it was determined
+ implicitly from the system.
+
GIT_COMMITTER_IDENT::
The person who put a piece of code into git.
+GIT_COMMITTER_EXPLICIT::
+ Like GIT_AUTHOR_EXPLICIT, but for the committer ident.
+
GIT_EDITOR::
Text editor for use by git commands. The value is meant to be
interpreted by the shell when it is used. Examples: `~/bin/vi`,
diff --git a/builtin/var.c b/builtin/var.c
index 49b48f5..ce28101 100644
--- a/builtin/var.c
+++ b/builtin/var.c
@@ -26,13 +26,40 @@ static const char *pager(int flag)
return pgm;
}
+static const char *explicit_ident(const char * (*get_ident)(int),
+ int (*check_ident)(void))
+{
+ /*
+ * Prime the "explicit" flag by getting the identity.
+ * We do not want to be strict here, because we would not want
+ * to die on bogus implicit values, but instead just report that they
+ * are not explicit.
+ */
+ get_ident(0);
+ return check_ident() ? "1" : "0";
+}
+
+static const char *committer_explicit(int flag)
+{
+ return explicit_ident(git_committer_info,
+ committer_ident_sufficiently_given);
+}
+
+static const char *author_explicit(int flag)
+{
+ return explicit_ident(git_author_info,
+ author_ident_sufficiently_given);
+}
+
struct git_var {
const char *name;
const char *(*read)(int);
};
static struct git_var git_vars[] = {
{ "GIT_COMMITTER_IDENT", git_committer_info },
+ { "GIT_COMMITTER_EXPLICIT", committer_explicit },
{ "GIT_AUTHOR_IDENT", git_author_info },
+ { "GIT_AUTHOR_EXPLICIT", author_explicit },
{ "GIT_EDITOR", editor },
{ "GIT_PAGER", pager },
{ "", NULL },
diff --git a/t/t0007-git-var.sh b/t/t0007-git-var.sh
index 45a5f66..66b9810 100755
--- a/t/t0007-git-var.sh
+++ b/t/t0007-git-var.sh
@@ -26,4 +26,40 @@ test_expect_success 'git var can show multiple values' '
test_cmp expect actual
'
+for type in AUTHOR COMMITTER; do
+ test_expect_success "$type ident can detect implicit values" "
+ echo 0 >expect &&
+ (
+ sane_unset GIT_${type}_NAME &&
+ sane_unset GIT_${type}_EMAIL &&
+ sane_unset EMAIL &&
+ git var GIT_${type}_EXPLICIT >actual
+ ) &&
+ test_cmp expect actual
+ "
+
+ test_expect_success "$type ident is explicit via environment" "
+ echo 1 >expect &&
+ (
+ GIT_${type}_NAME='A Name' &&
+ export GIT_${type}_NAME &&
+ GIT_${type}_EMAIL='name@example.com' &&
+ export GIT_${type}_EMAIL &&
+ git var GIT_${type}_EXPLICIT >actual
+ ) &&
+ test_cmp expect actual
+ "
+
+ test_expect_success "$type ident is explicit via config" "
+ echo 1 >expect &&
+ test_config user.name 'A Name' &&
+ test_config user.email 'name@example.com' &&
+ (
+ sane_unset GIT_${type}_NAME &&
+ sane_unset GIT_${type}_EMAIL &&
+ git var GIT_${type}_EXPLICIT >actual
+ )
+ "
+done
+
test_done
--
1.8.0.207.gdf2154c
^ permalink raw reply related [flat|nested] 76+ messages in thread* Re: [PATCH 4/6] var: provide explicit/implicit ident information
2012-11-13 16:53 ` [PATCH 4/6] var: provide explicit/implicit ident information Jeff King
@ 2012-11-14 17:06 ` Jonathan Nieder
2012-11-14 19:53 ` Jeff King
0 siblings, 1 reply; 76+ messages in thread
From: Jonathan Nieder @ 2012-11-14 17:06 UTC (permalink / raw)
To: Jeff King; +Cc: Felipe Contreras, git, Thomas Rast, Junio C Hamano
Jeff King wrote:
> Internally, we keep track of whether the author or committer
> ident information was provided by the user, or whether it
> was implicitly determined by the system. However, there is
> currently no way for external programs or scripts to get
> this information
What are the intended semantics? If my machine has /etc/mailname
filled out, is that an implicit identity? How about if I set the
EMAIL envvar but not GIT_COMMITTER_EMAIL?
If external scripts are going to start using this mechanism, they will
need answers to these questions to support users that run into
configuration problems. A few words on this in the documentation
could probably help.
On most machines I have the EMAIL envvar set explicitly, but in the
recent past I relied on /etc/mailname on some others, so I'm also
genuinely curious about the use case here (and too lazy to dig it up).
Thanks,
Jonathan
^ permalink raw reply [flat|nested] 76+ messages in thread
* Re: [PATCH 4/6] var: provide explicit/implicit ident information
2012-11-14 17:06 ` Jonathan Nieder
@ 2012-11-14 19:53 ` Jeff King
0 siblings, 0 replies; 76+ messages in thread
From: Jeff King @ 2012-11-14 19:53 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Felipe Contreras, git, Thomas Rast, Junio C Hamano
On Wed, Nov 14, 2012 at 09:06:57AM -0800, Jonathan Nieder wrote:
> Jeff King wrote:
>
> > Internally, we keep track of whether the author or committer
> > ident information was provided by the user, or whether it
> > was implicitly determined by the system. However, there is
> > currently no way for external programs or scripts to get
> > this information
>
> What are the intended semantics? If my machine has /etc/mailname
> filled out, is that an implicit identity? How about if I set the
> EMAIL envvar but not GIT_COMMITTER_EMAIL?
>
> If external scripts are going to start using this mechanism, they will
> need answers to these questions to support users that run into
> configuration problems. A few words on this in the documentation
> could probably help.
The intent is to make git's internal rules (whatever they may be)
available to calling scripts. But I agree those rules should be spelled
out now that they are becoming a more visible interface. I'll update the
documentation.
> On most machines I have the EMAIL envvar set explicitly, but in the
> recent past I relied on /etc/mailname on some others, so I'm also
> genuinely curious about the use case here (and too lazy to dig it up).
Right now EMAIL is explicit, but `id`@`cat /etc/mailname` is not. I
think changing that would be a separate issue, though (and I'm not sure
whether it is a good idea; my /etc/mailname is not an email address I
would want to use).
-Peff
^ permalink raw reply [flat|nested] 76+ messages in thread
* [PATCH 5/6] Git.pm: teach "ident" to query explicitness
2012-11-13 16:48 ` Jeff King
` (3 preceding siblings ...)
2012-11-13 16:53 ` [PATCH 4/6] var: provide explicit/implicit ident information Jeff King
@ 2012-11-13 16:53 ` Jeff King
[not found] ` <20121113172300.GA16241@ftbfs.org>
2012-11-14 17:12 ` Jonathan Nieder
2012-11-13 16:53 ` [PATCH 6/6] send-email: do not prompt for explicit repo ident Jeff King
` (2 subsequent siblings)
7 siblings, 2 replies; 76+ messages in thread
From: Jeff King @ 2012-11-13 16:53 UTC (permalink / raw)
To: Felipe Contreras; +Cc: git, Thomas Rast, Junio C Hamano, Jonathan Nieder
"git var" recently learned to report on whether an ident we
fetch from it was configured explicitly or implicitly. Let's
make that information available to callers of the ident
function.
Because evaluating "ident" in an array versus scalar context
already has a meaning, we cannot return our extra value in a
backwards compatible way. Instead, we require the caller to
add an extra "explicit" flag to request the information.
The ident_person function, on the other hand, always returns
a scalar, so we are free to overload it in an array context.
Signed-off-by: Jeff King <peff@peff.net>
---
perl/Git.pm | 28 ++++++++++++++++++++--------
1 file changed, 20 insertions(+), 8 deletions(-)
diff --git a/perl/Git.pm b/perl/Git.pm
index 497f420..1994ec1 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -737,7 +737,7 @@ sub remote_refs {
}
-=item ident ( TYPE | IDENTSTR )
+=item ident ( TYPE | IDENTSTR [, options] )
=item ident_person ( TYPE | IDENTSTR | IDENTARRAY )
@@ -750,6 +750,10 @@ and either returns it as a scalar string or as an array with the fields parsed.
Alternatively, it can take a prepared ident string (e.g. from the commit
object) and just parse it.
+If the C<explicit> option is set to 1, the returned array will contain an
+additional boolean specifying whether the ident was configure explicitly by the
+user.
+
C<ident_person> returns the person part of the ident - name and email;
it can take the same arguments as C<ident> or the array returned by C<ident>.
@@ -763,17 +767,22 @@ The synopsis is like:
=cut
sub ident {
- my ($self, $type) = _maybe_self(@_);
- my $identstr;
+ my ($self, $type, %options) = _maybe_self(@_);
+ my ($identstr, $explicit);
if (lc $type eq lc 'committer' or lc $type eq lc 'author') {
- my @cmd = ('var', 'GIT_'.uc($type).'_IDENT');
+ my $uc = uc($type);
+ my @cmd = ('var', "GIT_${uc}_IDENT", "GIT_${uc}_EXPLICIT");
unshift @cmd, $self if $self;
- $identstr = command_oneline(@cmd);
+ ($identstr, $explicit) = command(@cmd);
} else {
$identstr = $type;
}
if (wantarray) {
- return $identstr =~ /^(.*) <(.*)> (\d+ [+-]\d{4})$/;
+ my @ret = $identstr =~ /^(.*) <(.*)> (\d+ [+-]\d{4})$/;
+ if ($options{explicit} && defined $explicit) {
+ push @ret, $explicit if defined $explicit;
+ }
+ return @ret;
} else {
return $identstr;
}
@@ -781,8 +790,11 @@ sub ident {
sub ident_person {
my ($self, @ident) = _maybe_self(@_);
- $#ident == 0 and @ident = $self ? $self->ident($ident[0]) : ident($ident[0]);
- return "$ident[0] <$ident[1]>";
+ $#ident == 0 and @ident = $self ?
+ $self->ident($ident[0], explicit => 1) :
+ ident($ident[0], explicit => 1);
+ my $ret = "$ident[0] <$ident[1]>";
+ return wantarray ? ($ret, @ident[3]) : $ret;
}
--
1.8.0.207.gdf2154c
^ permalink raw reply related [flat|nested] 76+ messages in thread[parent not found: <20121113172300.GA16241@ftbfs.org>]
* Re: [PATCH 5/6] Git.pm: teach "ident" to query explicitness
[not found] ` <20121113172300.GA16241@ftbfs.org>
@ 2012-11-13 17:25 ` Jeff King
0 siblings, 0 replies; 76+ messages in thread
From: Jeff King @ 2012-11-13 17:25 UTC (permalink / raw)
To: Matt Kraai; +Cc: git
On Tue, Nov 13, 2012 at 09:23:00AM -0800, Matt Kraai wrote:
> Minor nits:
>
> On Tue, Nov 13, 2012 at 11:53:20AM -0500, Jeff King wrote:
> > @@ -750,6 +750,10 @@ and either returns it as a scalar string or as an array with the fields parsed.
> > Alternatively, it can take a prepared ident string (e.g. from the commit
> > object) and just parse it.
> >
> > +If the C<explicit> option is set to 1, the returned array will contain an
> > +additional boolean specifying whether the ident was configure explicitly by the
>
> s/configure/configured/
Thanks.
> > if (wantarray) {
> > - return $identstr =~ /^(.*) <(.*)> (\d+ [+-]\d{4})$/;
> > + my @ret = $identstr =~ /^(.*) <(.*)> (\d+ [+-]\d{4})$/;
> > + if ($options{explicit} && defined $explicit) {
> > + push @ret, $explicit if defined $explicit;
>
> Isn't the test on this line redundant given that defined $explicit is
> already guaranteed by the condition on the previous line?
Yes, thanks (left over from an earlier attempt that tried to avoid
having $options{explicit}).
-Peff
^ permalink raw reply [flat|nested] 76+ messages in thread
* Re: [PATCH 5/6] Git.pm: teach "ident" to query explicitness
2012-11-13 16:53 ` [PATCH 5/6] Git.pm: teach "ident" to query explicitness Jeff King
[not found] ` <20121113172300.GA16241@ftbfs.org>
@ 2012-11-14 17:12 ` Jonathan Nieder
2012-11-14 19:54 ` Jeff King
1 sibling, 1 reply; 76+ messages in thread
From: Jonathan Nieder @ 2012-11-14 17:12 UTC (permalink / raw)
To: Jeff King; +Cc: Felipe Contreras, git, Thomas Rast, Junio C Hamano
Jeff King wrote:
> "git var" recently learned to report on whether an ident we
> fetch from it was configured explicitly or implicitly. Let's
> make that information available to callers of the ident
> function.
Sounds sensible. Quick nits:
[...]
> --- a/perl/Git.pm
> +++ b/perl/Git.pm
> @@ -737,7 +737,7 @@ sub remote_refs {
> }
>
>
> -=item ident ( TYPE | IDENTSTR )
> +=item ident ( TYPE | IDENTSTR [, options] )
>
> =item ident_person ( TYPE | IDENTSTR | IDENTARRAY )
>
> @@ -750,6 +750,10 @@ and either returns it as a scalar string or as an array with the fields parsed.
> Alternatively, it can take a prepared ident string (e.g. from the commit
> object) and just parse it.
>
> +If the C<explicit> option is set to 1, the returned array will contain an
> +additional boolean specifying whether the ident was configure explicitly by the
> +user.
s/configure/configured/
I'd suggest adding "See GIT_COMMITTER_EXPLICIT in git-var(1) for
details" to make the semantics crystal clear. What do you think?
Thanks,
Jonathan
^ permalink raw reply [flat|nested] 76+ messages in thread* Re: [PATCH 5/6] Git.pm: teach "ident" to query explicitness
2012-11-14 17:12 ` Jonathan Nieder
@ 2012-11-14 19:54 ` Jeff King
0 siblings, 0 replies; 76+ messages in thread
From: Jeff King @ 2012-11-14 19:54 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Felipe Contreras, git, Thomas Rast, Junio C Hamano
On Wed, Nov 14, 2012 at 09:12:13AM -0800, Jonathan Nieder wrote:
> > --- a/perl/Git.pm
> > +++ b/perl/Git.pm
> > @@ -737,7 +737,7 @@ sub remote_refs {
> > }
> >
> >
> > -=item ident ( TYPE | IDENTSTR )
> > +=item ident ( TYPE | IDENTSTR [, options] )
> >
> > =item ident_person ( TYPE | IDENTSTR | IDENTARRAY )
> >
> > @@ -750,6 +750,10 @@ and either returns it as a scalar string or as an array with the fields parsed.
> > Alternatively, it can take a prepared ident string (e.g. from the commit
> > object) and just parse it.
> >
> > +If the C<explicit> option is set to 1, the returned array will contain an
> > +additional boolean specifying whether the ident was configure explicitly by the
> > +user.
>
> s/configure/configured/
>
> I'd suggest adding "See GIT_COMMITTER_EXPLICIT in git-var(1) for
> details" to make the semantics crystal clear. What do you think?
Good suggestion. Thanks.
-Peff
^ permalink raw reply [flat|nested] 76+ messages in thread
* [PATCH 6/6] send-email: do not prompt for explicit repo ident
2012-11-13 16:48 ` Jeff King
` (4 preceding siblings ...)
2012-11-13 16:53 ` [PATCH 5/6] Git.pm: teach "ident" to query explicitness Jeff King
@ 2012-11-13 16:53 ` Jeff King
2012-11-14 17:18 ` Jonathan Nieder
2012-11-13 20:35 ` [PATCH] send-email: add proper default sender Felipe Contreras
2012-11-15 0:30 ` [PATCHv2 0/8] loosening "sender" prompt in send-email Jeff King
7 siblings, 1 reply; 76+ messages in thread
From: Jeff King @ 2012-11-13 16:53 UTC (permalink / raw)
To: Felipe Contreras; +Cc: git, Thomas Rast, Junio C Hamano, Jonathan Nieder
If git-send-email is configured with sendemail.from, we will
not prompt the user for the "From" address of the emails.
If it is not configured, we prompt the user, but provide the
repo author or committer as a default. Even though we
probably have a sensible value for the default, the prompt
is a safety check in case git generated an incorrect
implicit ident string.
Now that Git.pm will tell us whether the ident is implicit or
explicit, we can stop prompting in the explicit case, saving
most users from having to see the prompt at all.
The test scripts need to be adjusted to not expect a prompt
for the sender, since they always have the author explicitly
defined in the environment. Unfortunately, we cannot
reliably test that prompting still happens in the implicit
case, as send-email will produce inconsistent results
depending on the machine config (if we cannot find a FQDN,
"git var" will barf, causing us to exit early; if we can,
send-email will continue but prompt).
Signed-off-by: Jeff King <peff@peff.net>
---
git-send-email.perl | 22 +++++++++++++---------
t/t9001-send-email.sh | 5 ++---
2 files changed, 15 insertions(+), 12 deletions(-)
diff --git a/git-send-email.perl b/git-send-email.perl
index 5a7c29d..0c49b32 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -436,9 +436,8 @@ if (0) {
}
}
-my ($repoauthor, $repocommitter);
-($repoauthor) = Git::ident_person(@repo, 'author');
-($repocommitter) = Git::ident_person(@repo, 'committer');
+my ($repoauthor, $author_explicit) = Git::ident_person(@repo, 'author');
+my ($repocommitter, $committer_explicit) = Git::ident_person(@repo, 'committer');
# Verify the user input
@@ -755,12 +754,17 @@ if (!$force) {
my $prompting = 0;
if (!defined $sender) {
- $sender = $repoauthor || $repocommitter || '';
- $sender = ask("Who should the emails appear to be from? [$sender] ",
- default => $sender,
- valid_re => qr/\@.*\./, confirm_only => 1);
- print "Emails will be sent from: ", $sender, "\n";
- $prompting++;
+ ($sender, my $explicit) =
+ defined $repoauthor ? ($repoauthor, $author_explicit) :
+ defined $repocommitter ? ($repocommitter, $committer_explicit) :
+ ('', 0);
+ if (!$explicit) {
+ $sender = ask("Who should the emails appear to be from? [$sender] ",
+ default => $sender,
+ valid_re => qr/\@.*\./, confirm_only => 1);
+ print "Emails will be sent from: ", $sender, "\n";
+ $prompting++;
+ }
}
if (!@initial_to && !defined $to_cmd) {
diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index 6c6af7d..c5d66cf 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -191,14 +191,13 @@ test_expect_success $PREREQ 'Show all headers' '
test_expect_success $PREREQ 'Prompting works' '
clean_fake_sendmail &&
- (echo "Example <from@example.com>"
- echo "to@example.com"
+ (echo "to@example.com"
echo ""
) | GIT_SEND_EMAIL_NOTTY=1 git send-email \
--smtp-server="$(pwd)/fake.sendmail" \
$patches \
2>errors &&
- grep "^From: Example <from@example.com>\$" msgtxt1 &&
+ grep "^From: A U Thor <author@example.com>\$" msgtxt1 &&
grep "^To: to@example.com\$" msgtxt1
'
--
1.8.0.207.gdf2154c
^ permalink raw reply related [flat|nested] 76+ messages in thread* Re: [PATCH 6/6] send-email: do not prompt for explicit repo ident
2012-11-13 16:53 ` [PATCH 6/6] send-email: do not prompt for explicit repo ident Jeff King
@ 2012-11-14 17:18 ` Jonathan Nieder
2012-11-14 20:05 ` Jeff King
0 siblings, 1 reply; 76+ messages in thread
From: Jonathan Nieder @ 2012-11-14 17:18 UTC (permalink / raw)
To: Jeff King; +Cc: Felipe Contreras, git, Thomas Rast, Junio C Hamano
Jeff King wrote:
> If git-send-email is configured with sendemail.from, we will
> not prompt the user for the "From" address of the emails.
> If it is not configured, we prompt the user, but provide the
> repo author or committer as a default. Even though we
> probably have a sensible value for the default, the prompt
> is a safety check in case git generated an incorrect
> implicit ident string.
I haven't read the code carefully, but this behavior sounds sensible,
so for what it's worth,
Acked-by: Jonathan Nieder <jrnieder@gmail.com>
[...]
> The test scripts need to be adjusted to not expect a prompt
> for the sender, since they always have the author explicitly
> defined in the environment. Unfortunately, we cannot
> reliably test that prompting still happens in the implicit
> case, as send-email will produce inconsistent results
> depending on the machine config (if we cannot find a FQDN,
> "git var" will barf, causing us to exit early;
At first this sounded like a bug to me --- how could the user keep
working without the sysadmin intervening?
But then I remembered that the user can set her name and email in
.gitconfig and probably would want to in such a setup anyway.
When someone writes such a test, I think it could check that git
either prompts or writes a message advising to configure the user
email, no? Waiting until later for that seems fine to me, though.
Thanks,
Jonathan
^ permalink raw reply [flat|nested] 76+ messages in thread
* Re: [PATCH 6/6] send-email: do not prompt for explicit repo ident
2012-11-14 17:18 ` Jonathan Nieder
@ 2012-11-14 20:05 ` Jeff King
2012-11-14 20:26 ` Jeff King
0 siblings, 1 reply; 76+ messages in thread
From: Jeff King @ 2012-11-14 20:05 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Felipe Contreras, git, Thomas Rast, Junio C Hamano
On Wed, Nov 14, 2012 at 09:18:27AM -0800, Jonathan Nieder wrote:
> > The test scripts need to be adjusted to not expect a prompt
> > for the sender, since they always have the author explicitly
> > defined in the environment. Unfortunately, we cannot
> > reliably test that prompting still happens in the implicit
> > case, as send-email will produce inconsistent results
> > depending on the machine config (if we cannot find a FQDN,
> > "git var" will barf, causing us to exit early;
>
> At first this sounded like a bug to me --- how could the user keep
> working without the sysadmin intervening?
>
> But then I remembered that the user can set her name and email in
> .gitconfig and probably would want to in such a setup anyway.
Right. They would already have to to make commits, for example.
> When someone writes such a test, I think it could check that git
> either prompts or writes a message advising to configure the user
> email, no? Waiting until later for that seems fine to me, though.
Yes. The problem is that the behavior and output are dependent on
factors outside the test suite, so we would have to check that one of
the possible expected outcomes happens. But I think there are really
only two such outcomes (neglecting that the ident itself can have
arbitrary content, but we do not have to check the actual content).
-Peff
^ permalink raw reply [flat|nested] 76+ messages in thread
* Re: [PATCH 6/6] send-email: do not prompt for explicit repo ident
2012-11-14 20:05 ` Jeff King
@ 2012-11-14 20:26 ` Jeff King
0 siblings, 0 replies; 76+ messages in thread
From: Jeff King @ 2012-11-14 20:26 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Felipe Contreras, git, Thomas Rast, Junio C Hamano
On Wed, Nov 14, 2012 at 12:05:05PM -0800, Jeff King wrote:
> > When someone writes such a test, I think it could check that git
> > either prompts or writes a message advising to configure the user
> > email, no? Waiting until later for that seems fine to me, though.
>
> Yes. The problem is that the behavior and output are dependent on
> factors outside the test suite, so we would have to check that one of
> the possible expected outcomes happens. But I think there are really
> only two such outcomes (neglecting that the ident itself can have
> arbitrary content, but we do not have to check the actual content).
Actually, I think the simplest thing is to add a prerequisite, like:
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 489bc80..8d192ff 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -738,6 +738,14 @@ test_lazy_prereq UTF8_NFD_TO_NFC '
esac
'
+test_lazy_prereq IMPLICIT_IDENT '
+ sane_unset GIT_AUTHOR_NAME &&
+ sane_unset GIT_AUTHOR_EMAIL &&
+ git var GIT_AUTHOR_IDENT &&
+ # double check that we were not polluted by config
+ test "$(git var GIT_AUTHOR_EXPLICIT)" = 0
+'
+
# When the tests are run as root, permission tests will report that
# things are writable when they shouldn't be.
test -w / || test_set_prereq SANITY
We can't have one test machine that will cover all of the cases, but
given that the test suite is run by many people across many machines, we
will get coverage (and I know that some people have machines which would
not pass that prereq, because I got test failure reports during the last
ident refactoring).
I'll include something like that in my re-roll (and it should let us
test "git commit" more thoroughly, too).
-Peff
^ permalink raw reply related [flat|nested] 76+ messages in thread
* Re: [PATCH] send-email: add proper default sender
2012-11-13 16:48 ` Jeff King
` (5 preceding siblings ...)
2012-11-13 16:53 ` [PATCH 6/6] send-email: do not prompt for explicit repo ident Jeff King
@ 2012-11-13 20:35 ` Felipe Contreras
2012-11-15 0:07 ` Jeff King
2012-11-15 0:30 ` [PATCHv2 0/8] loosening "sender" prompt in send-email Jeff King
7 siblings, 1 reply; 76+ messages in thread
From: Felipe Contreras @ 2012-11-13 20:35 UTC (permalink / raw)
To: Jeff King; +Cc: git, Thomas Rast, Junio C Hamano, Jonathan Nieder
On Tue, Nov 13, 2012 at 5:48 PM, Jeff King <peff@peff.net> wrote:
> On Tue, Nov 13, 2012 at 10:06:26AM +0100, Felipe Contreras wrote:
>> I think you are the one that is not understanding what I'm saying. But
>> I don't think it matters.
>>
>> This is what I'm saying; the current situation with 'git commit' is
>> not OK, _both_ 'git commit' and 'git send-email' should change.
>
> Perhaps I am being dense, but this is the first time it became apparent
> to me that you are arguing for a change in "git commit".
Miscomunication then. When you mentioned 'it has always been the case
that you can use git without setting
user.*', I directed my comments at git in general, not git send-email.
>> Indeed I would, but there's other people that would benefit from this
>> patch. I'm sure I'm not the only person that doesn't have
>> sendmail.from configured, but does have user.name/user.email, and is
>> constantly typing enter.
>>
>> And the difference is that I'm _real_, the hypothetical user that
>> sends patches with GIT_AUTHOR_NAME/EMAIL is not. I would be convinced
>> otherwise if some evidence was presented that such a user is real
>> though.
>
> Sadly we cannot poll the configuration of every user, nor expect them
> all to pay attention to this discussion on the list. So we cannot take
> the absence of comment from such users as evidence that they do not
> exist.
And we cannot take it as evidence that these users do exist either.
The absence of evidence simply means that... we don't have evidence.
> Instead we must use our judgement about what is being changed,
> and we tend to err on the side of keeping the status quo, since that is
> what the silent majority is busy _not_ complaining about.
Yes, the keyword is *tend*; this wouldn't be first or the last time
that a behavior change happens.
We have to be careful about these changes, but we do them.
> We use the same judgement on the other side, too. Right now your
> evidence is "1 user wants this, 0 users do not".
1 is infinitely greater than 0.
> But we can guess that
> there are other people who would like the intent of your patch, but did
> not care enough or are not active enough on the list to write the patch
> themselves or comment on this thread.
Yes, that would be an educated guess. IMO the fact that people use
GIT_AUTHOR_ variables to send mail is not. There's many ways to
achieve that: sendemail.from, --from, user configuration, $EMAIL,
--compose and From:, etc. each and every one of them much more likely
to be used than GIT_AUTHOR_.
But I'm tired of arguing how extremely unlikely it is that such people
don't exist. Lets agree to disagree. Either way it doesn't matter,
because nobody is proposing a patch that would affect these
hypothetical users.
>> And to balance you need to *measure*, and that means taking into
>> consideration who actually uses the features, if there's any. And it
>> looks to me this is a feature nobody uses.
>
> You said "measure" and then "it looks to me like". What did you measure?
> Did you consider systematic bias in your measurement, like the fact that
> people who are using the feature have no reason to come on the list and
> announce it?
measure != scientific measurement.
I used common sense, because that's the only tool available.
GIT_AUTHOR is plumbing, not very well known, it's cumbersome (you need
to export two variables), it can be easily confused with
GIT_COMMITTER, which wouldn't work on this case. And there's plenty of
tools that much simpler to use, starting with 'git send-email --from',
which is so user friendly it's in the --help. There's absolutely no
reason why anybody would want to use GIT_AUTHOR.
But lets agree to disagree.
>> But listen closely to what you said:
>>
>> > I actually think it would make more sense to drop the prompt entirely and just die when the user has not given us a usable ident.
>>
>> Suppose somebody has a full name, and a fully qualified domain name,
>> and he can receive mails to it directly. Such a user would not need a
>> git configuration, and would not need $EMAIL, or anything.
>>
>> Currently 'git send-email' will throw 'Felipe Contreras
>> <felipec@felipec.org>' which would actually work, but is not explicit.
>>
>> You are suggesting to break that use-case. You are introducing a
>> regression. And this case is realistic, unlike the
>> GIT_AUTHOR_NAME/EMAIL. Isn't it?
>
> Yes, dying would be a regression, in that you would have to configure
> your name via the environment and re-run rather than type it at the
> prompt. You raise a good point that for people who _could_ take the
> implicit default, hitting "enter" is working fine now, and we would lose
> that. I'd be fine with also just continuing to prompt in the implicit
> case.
>
> But that is a much smaller issue to me than having send-email fail to
> respect environment variables and silently use user.*, which is what
> started this whole discussion. And I agree it is worth considering as a
> regression we should avoid.
It might be smaller, I don't think so. A hypothetical user that was
relying on GIT_AUTHOR for whatever reason can switch to 'git
send-email --from' (which is much easier) when they notice the
failure, the same way somebody relying on fqdn would. The difference
is that people with fqdn do exist, and they might be relying on this.
Both are small issues, that I agree with.
But the point is that you seem to be very adamant about _my_
regressions, and not pay much attention about yours.
>> I prefer to concentrate on real issues, but that's just me.
>
> To be honest, I am confused at this point what you actually want. Do you
> think we should take your original patch? I think its regression is too
> great. And I am not sure if you agree or not. You seem to be arguing
> that the regression is not important, yet you simultaneously argue that
> we should be making all ident more strict, which would mean that we
> should indeed take my suggestion and use "git var" instead of preferring
> the config.
No, I was proposing my second patch. That's why I sent a second patch.
But forget it. I don't think it's worth trying to do the right thing
here (make both 'git commit' and 'git send-email' be stricter). I do
think that would be simpler than all the required changes to var, and
better, but that's not my itch atm.
>> > As for whether they exist, what data do you have?
>>
>> What data do _you_ have?
>>
>> When there's no evidence either way, the rational response is to don't
>> believe. That's the default position.
>
> This is not religion. It is a software project.
This has absolutely nothing to with religion, this is rationality. If
you don't want to make a distinction between the different levels at
which we _know_ something, suit yourself.
> In the absence of data,
> the sane thing is not to break existing users.
What you decide to do has absolutely nothing to do with what is true.
These hypothetical users exist, or they don't. I don't have data for
their existence, you don't have data for their non-existence. In the
absence of data the rational response is to don't believe the claim,
and this one is particularly clear because you cannot prove a
negative, so the burden of proof is clearly on your side, as it's
*impossible* for me to prove that there are no users that use certain
feature, and it's very easy for you to do so, as all you need is
*one*.
Presumably you are not interested in fulfilling your burden of proof,
which is fine, you can choose the safest action without proof that
such users exist, but that doesn't change the fact that you don't know
that if they exist or not.
And yet, you continue to say they do exist, and try to win the
argument by means of rhetoric: 'not to break existing users'; they are
not *existing* users, they are hypothetical.
> The burden is on you to
> argue that there is no such breakage.
That's impossible to prove. *Nobody* can prove that people are not
using even the most obscure feature of git.
All you do is make educated guesses, and try to minimize the damage.
>> > Are you aware that the
>> > test suite, for example, relies on setting GIT_AUTHOR_NAME but not
>> > having any user.* config?
>>
>> What tests? My patch doesn't seem to break anything there:
>> % make -C t t9001-send-email.sh
>> # passed all 96 test(s)
>
> My point was that there is at least one known setup that uses only
> environment variables and not a config file, and that the rest of git is
> intended to work with that. It is not a test failure, but t9001.18
> reveals the fact that your change does not handle this situation; we
> continue to prompt under the test suite's configuration. In the proper
> fix, the test needs to be adjusted.
>
> You don't see any test failures with your patch because we do not cover
> the case that you regress (GIT_AUTHOR_EMAIL and user.email both set).
Exactly, but I was not the one that brought the test suite. My patch
is not a problem for our current tests. So this hypothetical user
remains evasive.
>> > When somebody comes on the list and asks why
>> > every git program in the entire system respects GIT_* environment
>> > variables as an override to user.* configuration _except_ for
>> > send-email, what should I say?
>>
>> The same thing you say when somebody comes reporting a bug: "yeah, we
>> should probably fix that".
>
> It is a larger hassle for both the developer and the user to fix the bug
> after the fact,
*If* that happens, which in all likelihood it won't.
> ship a new version, and then have the user upgrade their
> git. Why not just not introduce the bug in the first place?
Because we are spending more time arguing about something that most
likely won't happen.
There's a chance the next time you sit the chair will break because of
a bad leg, but you don't engage in philosophical discussions each time
you are going to sit, you go ahead and sit down, and take your
chances, and 99.9% of the time you would be fine.
>> It's all about proportion. Is it possible that we all are going to die
>> tomorrow because of an asteroid? Sure... but what's the point of
>> worrying about it if it's not likely?
>
> If you want to talk about risk assessment, then the right computation is
> to compare the cost of fixing it now versus the cost of fixing it later
> times the probability of it happening. The development cost is probably
> a little higher later (because we have to refresh ourselves on the
> issue), but the deployment cost is much higher (e.g., users whose
> distros ship a broken version, or who have IT policy that does not let
> them upgrade git as soon as the bug-fix ships).
Times 0.01 (at best), and you get barely nothing.
>> >> That's right, AUTHOR_IDENT would fall back to the default email and full name.
>> >
>> > Yeah, I find that somewhat questionable in the current behavior, and I'd
>> > consider it a bug. Typically we prefer the committer ident when given a
>> > choice (e.g., for writing reflog entries).
>>
>> Yeah, but clearly the intention of the code was to use the committer
>> if the author wasn't available, which is the case here.
>
> Yes. It would make sense to me to respect an explicit committer over an
> implicit author. Though the fact that author is preferred over committer
> is inconsistent with most of the rest of git. I'm undecided whether that
> should be changed.
Indeed. And that's something I won't worry about, because I honestly
believe nobody is using neither GIT_COMMITTER, nor GIT_AUTHOR, and the
fact that nobody has found this breakage only increases my certainty.
>> >> What about after my change?
>> >>
>> >> 6.1) GIT_AUTHOR without anything else
>> >>
>> >> fatal: empty ident name (for <felipec@nysa.(none)>) not allowed
>> >> var GIT_COMMITTER_IDENT: command returned error: 128
>> >
>> > Doesn't that seem like a regression? It used to work.
>>
>> No, this is *before* my change.
>>
>> I's the same bug as 5.1):
>
> Ah, I see. We are loading both, whether or not they end up being used,
> and barfing prematurely on errors. That seems like a bug.
Yes.
>> >> And what about your proposed change?
>> >
>> > Let me be clear that I sent you a "something like this" patch to try to
>> > point you in the right direction. If it has a bug or is incomplete, that
>> > does not mean the direction is wrong, but only that I did not spend very
>> > much time on the patch.
>>
>> It doesn't matter, the idea was to use user_ident_sufficiently_given().
>
> Right. Which it sounds like now agree with me on?
No. I tried the idea, it didn't work.
I saw your patches about
{committer,author}_ident_sufficiently_given(), those might work, but I
don't think they are _needed_. They might be nice though.
>> > I think that respecting the usual ident lookup but disallowing implicit
>> > identities (either totally, or causing them to fallback to prompting) is
>> > the right direction. I agree my patch was not a complete solution. I'm
>> > sorry if it led you astray in terms of implementation, but I also think
>> > I've been very clear in my text about what the behavior should be.
>>
>> I think that is orthogonal to what I'm trying accomplish.
>
> What is it you are trying to accomplish? Eliminating the prompt in some
> cases, as in your original patch? Dealing with implicit identities may
> be orthogonal to your goal, but your original patch is not sufficient,
> as it reverses the precedence of config and environment variables.
No, it doesn't.
Why do you hold on to my first patch? The second patch doesn't have
this issue. It does change the behavior of 'git commit', yeah, but I
think that's a benefit.
You might disagree, but there's no point in going back to discuss the
first patch.
> So you can either:
>
> 1. Reimplement the environment variable lookup that ident.c does,
> leaving implicit ident logic out completely.
>
> 2. Modify ident.c and "git var" to let send-email reuse the logic in
> ident.c, but avoid dropping the prompt when an implicit ident is
> used.
>
> Doing (2) sounds a lot more maintainable to me in the long run.
Or:
3. Change the meaning of the STRICT flag so that the values are
explicit, which has benefits outside 'git send-email'. Yes, this would
change the behavior in 'git commit' and other tools, but it's worth to
investigate these changes, and most likely they would be desirable.
Or:
4. Just stop prompting
I already sent a patch for 4. with all the details of why nobody (or
very few, if any) would be affected negatively.
>> > Don't get me wrong. I think the spirit of your patch is correct, and it
>> > helps some git users. But it also hurts others. And it is not that hard
>> > to do it right.
>>
>> And I disagree, I think it hurts nobody, and I think it's hard to do it right.
>
> I am tired of talking about this. The patch series below took me less
> time to write than I have spent arguing with you. It was not that hard.
>
> [1/6]: ident: make user_ident_explicitly_given private
> [2/6]: ident: keep separate "explicit" flags for author and committer
> [3/6]: var: accept multiple variables on the command line
> [4/6]: var: provide explicit/implicit ident information
> [5/6]: Git.pm: teach "ident" to query explicitness
> [6/6]: send-email: do not prompt for explicit repo ident
I think this adds a lot of code that nobody would use.
The prompt will happen only if:
1) No configured user.name/user.email
2) No specified $EMAIL
3) No configured sendemail.from
4) No specified --from argument
5) A fully qualified domain name
6) A full name in the geckos field
Very, very unlikely.
And if:
* A sendmail configuration doesn't allow sending from this domain name
* The user sees the problem in the confirmation part
Then the user would see a problem anyway, so the prompt is not adding
that much value.
>> Fixing a regression that nobody would notice is not my itch either,
>
> Sorry, but it is part of your itch. The git project is at state A. With
> your patch, we are at a regressed state B.
And that's fine.
> If you then fix it, we are at
> state C. From your perspective, it may be "I do not want to make the fix
> to go from B to C". But from the project's perspective, it is "we do not
> want to go from state A to state B; state C would be acceptable". So
> from the perspective of people who do not care about your feature but do
> care about the regression in state B, it is inextricably linked to your
> itch.
No it's not. It's a conflict of interests. It's only linked as long as
people care about the "regression" in state B. If they realize this
"regression" is irrelevant, then there wouldn't be any problem. This,
is now clear is not going to happen. But I don't think your opinions
define whether or not I have an itch.
>> yet the patch I sent above does it, and it even fixes 'git commit'
>> (IMO). But it's also not good enough.
>
> I do not necessarily agree on "git commit". Moreover, I feel like it is
> a separate issue. My series above _just_ implements the "do not prompt
> when explicit" behavior. It does not deal with git-commit at all, nor
> does it address the author/committer fallback questions. Those can
> easily go on top.
Yes, at the cost of adding a lot of code. If we end up agreeing that
the changes to 'git commit' are desirable (which I hope at some point
we will), then this code would be all for nothing.
I sent another patch series that is very simple[1]. This deals with
the hypothetical GIT_AUTHOR users, but might hit another set of
hypothetical users, but the damage would be very small, and the
possibility of these users existing very low. IOW; very low risk.
I want clarify that this is merely a disagreement to at which level
should we worry about regressions. On one side of the spectrum you
have projects like GNOME, who don't have any problem breaking the
user-experience from one release to the next, I'm not proposing
anything like that. On the other side I think it's you, because I
don't recall encountering anybody with such an extreme position of
never introducing a regression ever if there's absolutely no evidence
that anybody is using certain feature. That's fine, it's your opinion,
I respectfully disagree. Personally, I think the sweet spot is between
the two, far from GNOME, but not quite to the point where no
regressions happen ever. When there's a good chance that very few
people will be hit, and the drawback is small, then it's OK to
introduce regressions. Always keeping in mind that these risk
assessments might be wrong, and there might be more people hit than
expected and/or the problems were greater, but if/when that happens,
*then* we simply deal with it.
Cheers.
[1] http://mid.gmane.org/1352834364-2674-1-git-send-email-felipe.contreras@gmail.com
--
Felipe Contreras
^ permalink raw reply [flat|nested] 76+ messages in thread* Re: [PATCH] send-email: add proper default sender
2012-11-13 20:35 ` [PATCH] send-email: add proper default sender Felipe Contreras
@ 2012-11-15 0:07 ` Jeff King
2012-11-15 1:41 ` Felipe Contreras
0 siblings, 1 reply; 76+ messages in thread
From: Jeff King @ 2012-11-15 0:07 UTC (permalink / raw)
To: Felipe Contreras; +Cc: git, Thomas Rast, Junio C Hamano, Jonathan Nieder
On Tue, Nov 13, 2012 at 09:35:18PM +0100, Felipe Contreras wrote:
> > Yes, dying would be a regression, in that you would have to configure
> > your name via the environment and re-run rather than type it at the
> > prompt. You raise a good point that for people who _could_ take the
> > implicit default, hitting "enter" is working fine now, and we would lose
> > that. I'd be fine with also just continuing to prompt in the implicit
> > case.
> >
> > But that is a much smaller issue to me than having send-email fail to
> > respect environment variables and silently use user.*, which is what
> > started this whole discussion. And I agree it is worth considering as a
> > regression we should avoid.
>
> It might be smaller, I don't think so. A hypothetical user that was
> relying on GIT_AUTHOR for whatever reason can switch to 'git
> send-email --from' (which is much easier) when they notice the
> failure, the same way somebody relying on fqdn would. The difference
> is that people with fqdn do exist, and they might be relying on this.
>
> Both are small issues, that I agree with.
>
> But the point is that you seem to be very adamant about _my_
> regressions, and not pay much attention about yours.
Really? I mentioned initially the possibility of dying instead of
prompting. You raised the point that it would regress a certain use
case. And then what happened? I said above "you raise a good point[...].
I'd be fine with also just continuing to prompt[...]. I agree it is
worth considering as a regression we should avoid". And then I sent out
a patch series which does not have the regression.
In other words, my suggestion was a bad one, and once it was pointed
out, I did not pursue it. If you want to call that "not paying much
attention", feel free. But I'd much rather you point out problems in my
actual patch series.
> Why do you hold on to my first patch?
Because clearly I am confused by your behavior. You continued to argue
that the initial regression was not worth worrying about. Which I
thought meant you were still interested in pursuing that patch. But if
you are not, then there is not even any point in discussing it. Which
is just fine with me, as that discussion seemed to be going nowhere.
> The second patch doesn't have this issue. It does change the behavior
> of 'git commit', yeah, but I think that's a benefit.
Changing "git commit" is even something I would entertain. It would be a
regression for some people, but at least it buys us something (increased
safety against people making bogus commits and failing to notice the
warning). I'm undecided on whether that is worth it or not.
But when you presented it, as far as I could tell the change in behavior
to "git commit" was accidental (which is why I pointed it out in
response). And as it was in the middle of a discussion about whether
regressions matter, it was not clear to me that your argument was "I
have thought this through and the inconvenience is outweighted by the
benefit" and not "I did not bother to consider the implications for
users who are currently using this feature".
If you want to seriously propose changing the behavior of "git commit",
I think the best thing would be to make a real patch, laying out the
pros and cons in the commit message, and post it. I would not be
surprised if the other list participants have stopped reading our thread
at this point, and the idea is going otherwise unnoticed.
> > So you can either:
> >
> > 1. Reimplement the environment variable lookup that ident.c does,
> > leaving implicit ident logic out completely.
> >
> > 2. Modify ident.c and "git var" to let send-email reuse the logic in
> > ident.c, but avoid dropping the prompt when an implicit ident is
> > used.
> >
> > Doing (2) sounds a lot more maintainable to me in the long run.
>
> Or:
>
> 3. Change the meaning of the STRICT flag so that the values are
> explicit, which has benefits outside 'git send-email'. Yes, this would
> change the behavior in 'git commit' and other tools, but it's worth to
> investigate these changes, and most likely they would be desirable.
Yes, I think that is fine _if_ we want to change git-commit. Which is
not clear to me. If we don't, then it is not an option.
> Or:
>
> 4. Just stop prompting
>
> I already sent a patch for 4. with all the details of why nobody (or
> very few, if any) would be affected negatively.
If doing (2) were really hard, that might be worth considering. But it's
not. I already did it. So I don't see how this is an attractive option,
unless my series is so unpalatable that we would rather accept a
regression.
> > [1/6]: ident: make user_ident_explicitly_given private
> > [2/6]: ident: keep separate "explicit" flags for author and committer
> > [3/6]: var: accept multiple variables on the command line
> > [4/6]: var: provide explicit/implicit ident information
> > [5/6]: Git.pm: teach "ident" to query explicitness
> > [6/6]: send-email: do not prompt for explicit repo ident
>
> I think this adds a lot of code that nobody would use.
A lot of code? It is mostly refactoring, which IMHO makes the resulting
code cleaner, and it increases the utility of "git var", and our test
coverage. If you have review comments, then by all means, respond to the
series.
> > I do not necessarily agree on "git commit". Moreover, I feel like it is
> > a separate issue. My series above _just_ implements the "do not prompt
> > when explicit" behavior. It does not deal with git-commit at all, nor
> > does it address the author/committer fallback questions. Those can
> > easily go on top.
>
> Yes, at the cost of adding a lot of code. If we end up agreeing that
> the changes to 'git commit' are desirable (which I hope at some point
> we will), then this code would be all for nothing.
If we are going to change "git commit" immediately, then I agree there
is not much point merging my series. But even if we do change it, will
we do so immediately? Will there be a deprecation period? If so, then my
series helps send-email in the meantime. And it's already written, so
you do not even have to do anything.
> I want clarify that this is merely a disagreement to at which level
> should we worry about regressions. On one side of the spectrum you
> have projects like GNOME, who don't have any problem breaking the
> user-experience from one release to the next, I'm not proposing
> anything like that. On the other side I think it's you, because I
> don't recall encountering anybody with such an extreme position of
> never introducing a regression ever if there's absolutely no evidence
> that anybody is using certain feature.
I don't think that's a fair characterization of my position. I am fine
with introducing a regression if there is a large benefit to it, and
especially if the regression is mutually exclusive with the benefit. For
example, look at IDENT_STRICT. We used to allow broken email addresses
in commits, and it was _me_ who pushed forward the change to disallow
it. That potentially regressed people who would rather have junk in the
commit objects than configure their identity (e.g., because they are
creating commits on the backend of some automated process). But we
discussed it, and the breakage was worth the increased safety for normal
users. We could not have it both ways, since the safety came at the
expense of switching the default.
But with this topic, we had a too-safe default (a safety prompt that was
sometimes overkill). We can have our cake and eat it, too: drop the
prompt for the overkill cases, but leave the other cases untouched. And
that is what I tried to do in my series. Note that this _still_
regresses certain use cases. What if I have configured my user.email,
but I am expecting send-email to prompt me so I can put in some other
random value. But we can't improve the prompting and leave that case
there; they are mutually exclusive. But IMHO, the benefit outweighs the
possibility of breakage.
-Peff
^ permalink raw reply [flat|nested] 76+ messages in thread
* Re: [PATCH] send-email: add proper default sender
2012-11-15 0:07 ` Jeff King
@ 2012-11-15 1:41 ` Felipe Contreras
2012-11-15 1:50 ` Jeff King
0 siblings, 1 reply; 76+ messages in thread
From: Felipe Contreras @ 2012-11-15 1:41 UTC (permalink / raw)
To: Jeff King; +Cc: git, Thomas Rast, Junio C Hamano, Jonathan Nieder
On Thu, Nov 15, 2012 at 1:07 AM, Jeff King <peff@peff.net> wrote:
> On Tue, Nov 13, 2012 at 09:35:18PM +0100, Felipe Contreras wrote:
>
>> > Yes, dying would be a regression, in that you would have to configure
>> > your name via the environment and re-run rather than type it at the
>> > prompt. You raise a good point that for people who _could_ take the
>> > implicit default, hitting "enter" is working fine now, and we would lose
>> > that. I'd be fine with also just continuing to prompt in the implicit
>> > case.
>> >
>> > But that is a much smaller issue to me than having send-email fail to
>> > respect environment variables and silently use user.*, which is what
>> > started this whole discussion. And I agree it is worth considering as a
>> > regression we should avoid.
>>
>> It might be smaller, I don't think so. A hypothetical user that was
>> relying on GIT_AUTHOR for whatever reason can switch to 'git
>> send-email --from' (which is much easier) when they notice the
>> failure, the same way somebody relying on fqdn would. The difference
>> is that people with fqdn do exist, and they might be relying on this.
>>
>> Both are small issues, that I agree with.
>>
>> But the point is that you seem to be very adamant about _my_
>> regressions, and not pay much attention about yours.
>
> Really? I mentioned initially the possibility of dying instead of
> prompting. You raised the point that it would regress a certain use
> case. And then what happened? I said above "you raise a good point[...].
> I'd be fine with also just continuing to prompt[...]. I agree it is
> worth considering as a regression we should avoid". And then I sent out
> a patch series which does not have the regression.
>
> In other words, my suggestion was a bad one, and once it was pointed
> out, I did not pursue it. If you want to call that "not paying much
> attention", feel free. But I'd much rather you point out problems in my
> actual patch series.
But that I meant that when I introduce a regression it's like I'm
killing all that is good and sacred about git, and when you do it's
everything but that.
Yes, you sent a new patch. So did I.
>> The second patch doesn't have this issue. It does change the behavior
>> of 'git commit', yeah, but I think that's a benefit.
>
> Changing "git commit" is even something I would entertain. It would be a
> regression for some people, but at least it buys us something (increased
> safety against people making bogus commits and failing to notice the
> warning). I'm undecided on whether that is worth it or not.
>
> But when you presented it, as far as I could tell the change in behavior
> to "git commit" was accidental (which is why I pointed it out in
> response).
How could it be accidental if I said this: "Not only will this fix
'git send-email', but it will also fix 'git
commit'".
> And as it was in the middle of a discussion about whether
> regressions matter,
That was not the discussion at all.
You can't say that all regressions are the same, and if I say
"regression X doesn't matter", that means ALL regressions don't
matter. That's a hasty generalization.
> If you want to seriously propose changing the behavior of "git commit",
> I think the best thing would be to make a real patch, laying out the
> pros and cons in the commit message, and post it. I would not be
> surprised if the other list participants have stopped reading our thread
> at this point, and the idea is going otherwise unnoticed.
I would, if I saw any chance in it actually going through.
>> Or:
>>
>> 4. Just stop prompting
>>
>> I already sent a patch for 4. with all the details of why nobody (or
>> very few, if any) would be affected negatively.
>
> If doing (2) were really hard, that might be worth considering. But it's
> not. I already did it. So I don't see how this is an attractive option,
> unless my series is so unpalatable that we would rather accept a
> regression.
A matter of opinion. I think that series introduces way too much code
for a very very small gain that eventually would probably disappear.
>> > [1/6]: ident: make user_ident_explicitly_given private
>> > [2/6]: ident: keep separate "explicit" flags for author and committer
>> > [3/6]: var: accept multiple variables on the command line
>> > [4/6]: var: provide explicit/implicit ident information
>> > [5/6]: Git.pm: teach "ident" to query explicitness
>> > [6/6]: send-email: do not prompt for explicit repo ident
>>
>> I think this adds a lot of code that nobody would use.
>
> A lot of code? It is mostly refactoring,
Patch #1 and #3 are refactoring, the rest are not.
> which IMHO makes the resulting
> code cleaner, and it increases the utility of "git var", and our test
> coverage. If you have review comments, then by all means, respond to the
> series.
I don't have any comments, except that I don't think all that code is
needed. And why would I bother commenting there, if my opinion will be
ignored?
>> > I do not necessarily agree on "git commit". Moreover, I feel like it is
>> > a separate issue. My series above _just_ implements the "do not prompt
>> > when explicit" behavior. It does not deal with git-commit at all, nor
>> > does it address the author/committer fallback questions. Those can
>> > easily go on top.
>>
>> Yes, at the cost of adding a lot of code. If we end up agreeing that
>> the changes to 'git commit' are desirable (which I hope at some point
>> we will), then this code would be all for nothing.
>
> If we are going to change "git commit" immediately, then I agree there
> is not much point merging my series. But even if we do change it, will
> we do so immediately? Will there be a deprecation period? If so, then my
> series helps send-email in the meantime. And it's already written, so
> you do not even have to do anything.
Yes, but it still adds a lot of code.
>> I want clarify that this is merely a disagreement to at which level
>> should we worry about regressions. On one side of the spectrum you
>> have projects like GNOME, who don't have any problem breaking the
>> user-experience from one release to the next, I'm not proposing
>> anything like that. On the other side I think it's you, because I
>> don't recall encountering anybody with such an extreme position of
>> never introducing a regression ever if there's absolutely no evidence
>> that anybody is using certain feature.
>
> I don't think that's a fair characterization of my position. I am fine
> with introducing a regression if there is a large benefit to it, and
> especially if the regression is mutually exclusive with the benefit. For
> example, look at IDENT_STRICT. We used to allow broken email addresses
> in commits, and it was _me_ who pushed forward the change to disallow
> it. That potentially regressed people who would rather have junk in the
> commit objects than configure their identity (e.g., because they are
> creating commits on the backend of some automated process). But we
> discussed it, and the breakage was worth the increased safety for normal
> users. We could not have it both ways, since the safety came at the
> expense of switching the default.
>
> But with this topic, we had a too-safe default (a safety prompt that was
> sometimes overkill). We can have our cake and eat it, too: drop the
> prompt for the overkill cases, but leave the other cases untouched. And
> that is what I tried to do in my series. Note that this _still_
> regresses certain use cases. What if I have configured my user.email,
> but I am expecting send-email to prompt me so I can put in some other
> random value. But we can't improve the prompting and leave that case
> there; they are mutually exclusive. But IMHO, the benefit outweighs the
> possibility of breakage.
That's true, so we will be introducing a regression regardless.
Now, if we go with this patch:
http://article.gmane.org/gmane.comp.version-control.git/209660
Instead of your patch series, who will get hurt? Hint: I already
answered that question.
Cheers.
--
Felipe Contreras
^ permalink raw reply [flat|nested] 76+ messages in thread
* Re: [PATCH] send-email: add proper default sender
2012-11-15 1:41 ` Felipe Contreras
@ 2012-11-15 1:50 ` Jeff King
2012-11-15 2:14 ` Felipe Contreras
0 siblings, 1 reply; 76+ messages in thread
From: Jeff King @ 2012-11-15 1:50 UTC (permalink / raw)
To: Felipe Contreras; +Cc: git, Thomas Rast, Junio C Hamano, Jonathan Nieder
On Thu, Nov 15, 2012 at 02:41:50AM +0100, Felipe Contreras wrote:
> But that I meant that when I introduce a regression it's like I'm
> killing all that is good and sacred about git, and when you do it's
> everything but that.
The rhetoric in this statement is a good indication that there is
nothing productive to come from our discussing it anymore.
> > If you want to seriously propose changing the behavior of "git commit",
> > I think the best thing would be to make a real patch, laying out the
> > pros and cons in the commit message, and post it. I would not be
> > surprised if the other list participants have stopped reading our thread
> > at this point, and the idea is going otherwise unnoticed.
>
> I would, if I saw any chance in it actually going through.
Well, it certainly will not go through if you do not try.
-Peff
^ permalink raw reply [flat|nested] 76+ messages in thread
* Re: [PATCH] send-email: add proper default sender
2012-11-15 1:50 ` Jeff King
@ 2012-11-15 2:14 ` Felipe Contreras
0 siblings, 0 replies; 76+ messages in thread
From: Felipe Contreras @ 2012-11-15 2:14 UTC (permalink / raw)
To: Jeff King; +Cc: git, Thomas Rast, Junio C Hamano, Jonathan Nieder
On Thu, Nov 15, 2012 at 2:50 AM, Jeff King <peff@peff.net> wrote:
> On Thu, Nov 15, 2012 at 02:41:50AM +0100, Felipe Contreras wrote:
>
>> But that I meant that when I introduce a regression it's like I'm
>> killing all that is good and sacred about git, and when you do it's
>> everything but that.
>
> The rhetoric in this statement is a good indication that there is
> nothing productive to come from our discussing it anymore.
The point is still true.
>> > If you want to seriously propose changing the behavior of "git commit",
>> > I think the best thing would be to make a real patch, laying out the
>> > pros and cons in the commit message, and post it. I would not be
>> > surprised if the other list participants have stopped reading our thread
>> > at this point, and the idea is going otherwise unnoticed.
>>
>> I would, if I saw any chance in it actually going through.
>
> Well, it certainly will not go through if you do not try.
At least I wouldn't be wasting my time.
Cheers.
--
Felipe Contreras
^ permalink raw reply [flat|nested] 76+ messages in thread
* [PATCHv2 0/8] loosening "sender" prompt in send-email
2012-11-13 16:48 ` Jeff King
` (6 preceding siblings ...)
2012-11-13 20:35 ` [PATCH] send-email: add proper default sender Felipe Contreras
@ 2012-11-15 0:30 ` Jeff King
2012-11-15 0:33 ` [PATCHv2 1/8] test-lib: allow negation of prerequisites Jeff King
` (7 more replies)
7 siblings, 8 replies; 76+ messages in thread
From: Jeff King @ 2012-11-15 0:30 UTC (permalink / raw)
To: git; +Cc: Felipe Contreras, git, Thomas Rast, Junio C Hamano,
Jonathan Nieder
On Tue, Nov 13, 2012 at 11:48:45AM -0500, Jeff King wrote:
> [1/6]: ident: make user_ident_explicitly_given private
> [2/6]: ident: keep separate "explicit" flags for author and committer
> [3/6]: var: accept multiple variables on the command line
> [4/6]: var: provide explicit/implicit ident information
> [5/6]: Git.pm: teach "ident" to query explicitness
> [6/6]: send-email: do not prompt for explicit repo ident
Here is a re-roll of the series based on comments from Jonathan (changes
from the first version noted below):
[1/8]: test-lib: allow negation of prerequisites
[2/8]: t7502: factor out autoident prerequisite
These two are new, and allow us to use the same prereq from t7502 in
t9001.
[3/8]: ident: make user_ident_explicitly_given static
Expanded the commit message to explain history of the variable.
[4/8]: ident: keep separate "explicit" flags for author and committer
Same as before.
[5/8]: var: accept multiple variables on the command line
- Expanded tests to include "git var -l"
- Use $GIT_* variables to create test "expect" files, which should make
them less brittle.
- Fixed "git var -l foo" to print usage instead of ignoring foo (i.e.,
keeping the behavior the same).
- Update usage message to match new feature.
[6/8]: var: provide explicit/implicit ident information
Updated documentation to describe what "explicit" means. No functional
changes.
[7/8]: Git.pm: teach "ident" to query explicitness
Improved Git.pm documentation. No functional changes.
[8/8]: send-email: do not prompt for explicit repo ident
Test the implicit ident case, both when we have implicit ident and when
we don't. No changes to send-email.
-Peff
^ permalink raw reply [flat|nested] 76+ messages in thread* [PATCHv2 1/8] test-lib: allow negation of prerequisites
2012-11-15 0:30 ` [PATCHv2 0/8] loosening "sender" prompt in send-email Jeff King
@ 2012-11-15 0:33 ` Jeff King
2012-11-15 7:46 ` Jonathan Nieder
2012-11-15 0:33 ` [PATCHv2 2/8] t7502: factor out autoident prerequisite Jeff King
` (6 subsequent siblings)
7 siblings, 1 reply; 76+ messages in thread
From: Jeff King @ 2012-11-15 0:33 UTC (permalink / raw)
To: git; +Cc: Felipe Contreras, git, Thomas Rast, Junio C Hamano,
Jonathan Nieder
You can set and test a prerequisite like this:
test_set_prereq FOO
test_have_prereq FOO && echo yes
You can negate the test in the shell like this:
! test_have_prereq && echo no
However, when you are using the automatic prerequisite
checking in test_expect_*, there is no opportunity to use
the shell negation. This patch introduces the syntax "!FOO"
to indicate that the test should only run if a prerequisite
is not meant.
One alternative is to set an explicit negative prerequisite,
like:
if system_has_foo; then
test_set_prereq FOO
else
test_set_prereq NO_FOO
fi
However, this doesn't work for lazy prerequisites, which
associate a single test with a single name. We could teach
the lazy prereq evaluator to set both forms, but the code
change ends up quite similar to this one (because we still
need to convert NO_FOO into FOO to find the correct lazy
script).
Signed-off-by: Jeff King <peff@peff.net>
---
I chose "!" as the negation prefix because it will never conflict with
another prerequisite. But we could also use "NO_" if that's more
aesthetically pleasing. I don't have a strong preference.
t/t0000-basic.sh | 32 ++++++++++++++++++++++++++++++++
t/test-lib-functions.sh | 21 ++++++++++++++++++++-
2 files changed, 52 insertions(+), 1 deletion(-)
diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
index 08677df..562cf41 100755
--- a/t/t0000-basic.sh
+++ b/t/t0000-basic.sh
@@ -115,6 +115,38 @@ then
exit 1
fi
+test_lazy_prereq LAZY_TRUE true
+havetrue=no
+test_expect_success LAZY_TRUE 'test runs if lazy prereq is satisfied' '
+ havetrue=yes
+'
+donthavetrue=yes
+test_expect_success !LAZY_TRUE 'missing lazy prereqs skip tests' '
+ donthavetrue=no
+'
+
+if test "$havetrue$donthavetrue" != yesyes
+then
+ say 'bug in test framework: lazy prerequisites do not work'
+ exit 1
+fi
+
+test_lazy_prereq LAZY_FALSE false
+nothavefalse=no
+test_expect_success !LAZY_FALSE 'negative lazy prereqs checked' '
+ nothavefalse=yes
+'
+havefalse=yes
+test_expect_success LAZY_FALSE 'missing negative lazy prereqs will skip' '
+ havefalse=no
+'
+
+if test "$nothavefalse$havefalse" != yesyes
+then
+ say 'bug in test framework: negative lazy prerequisites do not work'
+ exit 1
+fi
+
clean=no
test_expect_success 'tests clean up after themselves' '
test_when_finished clean=yes
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 8889ba5..22a4f8f 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -275,6 +275,15 @@ test_have_prereq () {
for prerequisite
do
+ case "$prerequisite" in
+ !*)
+ negative_prereq=t
+ prerequisite=${prerequisite#!}
+ ;;
+ *)
+ negative_prereq=
+ esac
+
case " $lazily_tested_prereq " in
*" $prerequisite "*)
;;
@@ -294,10 +303,20 @@ test_have_prereq () {
total_prereq=$(($total_prereq + 1))
case "$satisfied_prereq" in
*" $prerequisite "*)
+ satisfied_this_prereq=t
+ ;;
+ *)
+ satisfied_this_prereq=
+ esac
+
+ case "$satisfied_this_prereq,$negative_prereq" in
+ t,|,t)
ok_prereq=$(($ok_prereq + 1))
;;
*)
- # Keep a list of missing prerequisites
+ # Keep a list of missing prerequisites; restore
+ # the negative marker if necessary.
+ prerequisite=${negative_prereq:+!}$prerequisite
if test -z "$missing_prereq"
then
missing_prereq=$prerequisite
--
1.8.0.207.gdf2154c
^ permalink raw reply related [flat|nested] 76+ messages in thread* Re: [PATCHv2 1/8] test-lib: allow negation of prerequisites
2012-11-15 0:33 ` [PATCHv2 1/8] test-lib: allow negation of prerequisites Jeff King
@ 2012-11-15 7:46 ` Jonathan Nieder
2012-11-15 16:42 ` Jeff King
0 siblings, 1 reply; 76+ messages in thread
From: Jonathan Nieder @ 2012-11-15 7:46 UTC (permalink / raw)
To: Jeff King; +Cc: git, Felipe Contreras, Thomas Rast, Junio C Hamano
Jeff King wrote:
> +test_expect_success !LAZY_TRUE 'missing lazy prereqs skip tests' '
I have a visceral nervousness when reading this code, from too much
unpleasant experience of bash's csh-style !history expansion. Luckily
bash does not treat ! specially in the '-o sh' mode used by tests.
Does this feature work when running a test explicitly using
"bash <name of test>"? That's something I do from time to time to
figure out whether a weird behavior is shell-specific.
If it works everywhere, this patch would help me conquer my fear of
exclamation points in git's tests, which would be a comfort to me and
a very good thing.
^ permalink raw reply [flat|nested] 76+ messages in thread
* Re: [PATCHv2 1/8] test-lib: allow negation of prerequisites
2012-11-15 7:46 ` Jonathan Nieder
@ 2012-11-15 16:42 ` Jeff King
2012-11-15 16:49 ` Jonathan Nieder
0 siblings, 1 reply; 76+ messages in thread
From: Jeff King @ 2012-11-15 16:42 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: git, Felipe Contreras, Thomas Rast, Junio C Hamano
On Wed, Nov 14, 2012 at 11:46:58PM -0800, Jonathan Nieder wrote:
> > +test_expect_success !LAZY_TRUE 'missing lazy prereqs skip tests' '
>
> I have a visceral nervousness when reading this code, from too much
> unpleasant experience of bash's csh-style !history expansion. Luckily
> bash does not treat ! specially in the '-o sh' mode used by tests.
I can understand, having been bitten by it many times myself (not only
is it counter-intuitively expanded inside double quotes, and not only
does it not do what you wanted, but it does not insert the command into
the history, so you cannot even go back to fix it easily).
> Does this feature work when running a test explicitly using
> "bash <name of test>"? That's something I do from time to time to
> figure out whether a weird behavior is shell-specific.
Yes. You can test it yourself with "bash t0000-basic.sh". The reason is
that the "!" is part of history expansion, which is only enabled by
default for interactive shells.
> If it works everywhere, this patch would help me conquer my fear of
> exclamation points in git's tests, which would be a comfort to me and
> a very good thing.
As far as I know, "!" should be safe in a non-interactive shell script.
-Peff
^ permalink raw reply [flat|nested] 76+ messages in thread
* Re: [PATCHv2 1/8] test-lib: allow negation of prerequisites
2012-11-15 16:42 ` Jeff King
@ 2012-11-15 16:49 ` Jonathan Nieder
0 siblings, 0 replies; 76+ messages in thread
From: Jonathan Nieder @ 2012-11-15 16:49 UTC (permalink / raw)
To: Jeff King; +Cc: git, Felipe Contreras, Thomas Rast, Junio C Hamano
Jeff King wrote:
> Yes. You can test it yourself with "bash t0000-basic.sh". The reason is
> that the "!" is part of history expansion, which is only enabled by
> default for interactive shells.
Nice to hear. Thanks much for looking into it.
> On Wed, Nov 14, 2012 at 11:46:58PM -0800, Jonathan Nieder wrote:
>> If it works everywhere, this patch would help me conquer my fear of
>> exclamation points in git's tests, which would be a comfort to me and
>> a very good thing.
^ permalink raw reply [flat|nested] 76+ messages in thread
* [PATCHv2 2/8] t7502: factor out autoident prerequisite
2012-11-15 0:30 ` [PATCHv2 0/8] loosening "sender" prompt in send-email Jeff King
2012-11-15 0:33 ` [PATCHv2 1/8] test-lib: allow negation of prerequisites Jeff King
@ 2012-11-15 0:33 ` Jeff King
2012-11-15 7:49 ` Jonathan Nieder
2012-11-15 0:34 ` [PATCHv2 3/8] ident: make user_ident_explicitly_given static Jeff King
` (5 subsequent siblings)
7 siblings, 1 reply; 76+ messages in thread
From: Jeff King @ 2012-11-15 0:33 UTC (permalink / raw)
To: git; +Cc: Felipe Contreras, git, Thomas Rast, Junio C Hamano,
Jonathan Nieder
t7502 checks the behavior of commit when we can and cannot
determine a valid committer ident. Let's move that into
test-lib as a lazy prerequisite so other scripts can use it.
Signed-off-by: Jeff King <peff@peff.net>
---
t/t7502-commit.sh | 12 +-----------
t/test-lib.sh | 6 ++++++
2 files changed, 7 insertions(+), 11 deletions(-)
diff --git a/t/t7502-commit.sh b/t/t7502-commit.sh
index deb187e..1a5cb69 100755
--- a/t/t7502-commit.sh
+++ b/t/t7502-commit.sh
@@ -243,16 +243,6 @@ test_expect_success 'message shows author when it is not equal to committer' '
.git/COMMIT_EDITMSG
'
-test_expect_success 'setup auto-ident prerequisite' '
- if (sane_unset GIT_COMMITTER_EMAIL &&
- sane_unset GIT_COMMITTER_NAME &&
- git var GIT_COMMITTER_IDENT); then
- test_set_prereq AUTOIDENT
- else
- test_set_prereq NOAUTOIDENT
- fi
-'
-
test_expect_success AUTOIDENT 'message shows committer when it is automatic' '
echo >>negative &&
@@ -271,7 +261,7 @@ echo editor started > "$(pwd)/.git/result"
exit 0
EOF
-test_expect_success NOAUTOIDENT 'do not fire editor when committer is bogus' '
+test_expect_success !AUTOIDENT 'do not fire editor when committer is bogus' '
>.git/result
>expect &&
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 489bc80..0334a9e 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -738,6 +738,12 @@ test_lazy_prereq UTF8_NFD_TO_NFC '
esac
'
+test_lazy_prereq AUTOIDENT '
+ sane_unset GIT_AUTHOR_NAME &&
+ sane_unset GIT_AUTHOR_EMAIL &&
+ git var GIT_AUTHOR_IDENT
+'
+
# When the tests are run as root, permission tests will report that
# things are writable when they shouldn't be.
test -w / || test_set_prereq SANITY
--
1.8.0.207.gdf2154c
^ permalink raw reply related [flat|nested] 76+ messages in thread* [PATCHv2 3/8] ident: make user_ident_explicitly_given static
2012-11-15 0:30 ` [PATCHv2 0/8] loosening "sender" prompt in send-email Jeff King
2012-11-15 0:33 ` [PATCHv2 1/8] test-lib: allow negation of prerequisites Jeff King
2012-11-15 0:33 ` [PATCHv2 2/8] t7502: factor out autoident prerequisite Jeff King
@ 2012-11-15 0:34 ` Jeff King
2012-11-15 7:51 ` Jonathan Nieder
2012-11-15 0:34 ` [PATCHv2 4/8] ident: keep separate "explicit" flags for author and committer Jeff King
` (4 subsequent siblings)
7 siblings, 1 reply; 76+ messages in thread
From: Jeff King @ 2012-11-15 0:34 UTC (permalink / raw)
To: git; +Cc: Felipe Contreras, git, Thomas Rast, Junio C Hamano,
Jonathan Nieder
In v1.5.6-rc0~56^2 (2008-05-04) "user_ident_explicitly_given"
was introduced as a global for communication between config,
ident, and builtin-commit. In v1.7.0-rc0~72^2 (2010-01-07)
readers switched to using the common wrapper
user_ident_sufficiently_given(). After v1.7.11-rc1~15^2~18
(2012-05-21), the var is only written in ident.c.
Now we can make it static, which will enable further
refactoring without worrying about upsetting other code.
Signed-off-by: Jeff King <peff@peff.net>
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
---
cache.h | 4 ----
ident.c | 6 +++++-
2 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/cache.h b/cache.h
index dbd8018..50d9eea 100644
--- a/cache.h
+++ b/cache.h
@@ -1149,10 +1149,6 @@ struct config_include_data {
#define CONFIG_INCLUDE_INIT { 0 }
extern int git_config_include(const char *name, const char *value, void *data);
-#define IDENT_NAME_GIVEN 01
-#define IDENT_MAIL_GIVEN 02
-#define IDENT_ALL_GIVEN (IDENT_NAME_GIVEN|IDENT_MAIL_GIVEN)
-extern int user_ident_explicitly_given;
extern int user_ident_sufficiently_given(void);
extern const char *git_commit_encoding;
diff --git a/ident.c b/ident.c
index a4bf206..733d69d 100644
--- a/ident.c
+++ b/ident.c
@@ -10,7 +10,11 @@
static struct strbuf git_default_name = STRBUF_INIT;
static struct strbuf git_default_email = STRBUF_INIT;
static char git_default_date[50];
-int user_ident_explicitly_given;
+
+#define IDENT_NAME_GIVEN 01
+#define IDENT_MAIL_GIVEN 02
+#define IDENT_ALL_GIVEN (IDENT_NAME_GIVEN|IDENT_MAIL_GIVEN)
+static int user_ident_explicitly_given;
#ifdef NO_GECOS_IN_PWENT
#define get_gecos(ignored) "&"
--
1.8.0.207.gdf2154c
^ permalink raw reply related [flat|nested] 76+ messages in thread* Re: [PATCHv2 3/8] ident: make user_ident_explicitly_given static
2012-11-15 0:34 ` [PATCHv2 3/8] ident: make user_ident_explicitly_given static Jeff King
@ 2012-11-15 7:51 ` Jonathan Nieder
0 siblings, 0 replies; 76+ messages in thread
From: Jonathan Nieder @ 2012-11-15 7:51 UTC (permalink / raw)
To: Jeff King; +Cc: git, Felipe Contreras, Thomas Rast, Junio C Hamano
Jeff King wrote:
> In v1.5.6-rc0~56^2 (2008-05-04) "user_ident_explicitly_given"
> was introduced as a global for communication between config,
> ident, and builtin-commit. In v1.7.0-rc0~72^2 (2010-01-07)
> readers switched to using the common wrapper
> user_ident_sufficiently_given(). After v1.7.11-rc1~15^2~18
> (2012-05-21), the var is only written in ident.c.
>
> Now we can make it static, which will enable further
> refactoring without worrying about upsetting other code.
>
> Signed-off-by: Jeff King <peff@peff.net>
For what it's worth I liked the old commit message more. But I don't
mind either.
Hope that helps,
Jonathan
^ permalink raw reply [flat|nested] 76+ messages in thread
* [PATCHv2 4/8] ident: keep separate "explicit" flags for author and committer
2012-11-15 0:30 ` [PATCHv2 0/8] loosening "sender" prompt in send-email Jeff King
` (2 preceding siblings ...)
2012-11-15 0:34 ` [PATCHv2 3/8] ident: make user_ident_explicitly_given static Jeff King
@ 2012-11-15 0:34 ` Jeff King
2012-11-15 8:04 ` Jonathan Nieder
2012-11-15 0:35 ` [PATCHv2 5/8] var: accept multiple variables on the command line Jeff King
` (3 subsequent siblings)
7 siblings, 1 reply; 76+ messages in thread
From: Jeff King @ 2012-11-15 0:34 UTC (permalink / raw)
To: git; +Cc: Felipe Contreras, git, Thomas Rast, Junio C Hamano,
Jonathan Nieder
We keep track of whether the user ident was given to us
explicitly, or if we guessed at it from system parameters
like username and hostname. However, we kept only a single
variable. This covers the common cases (because the author
and committer will usually come from the same explicit
source), but can miss two cases:
1. GIT_COMMITTER_* is set explicitly, but we fallback for
GIT_AUTHOR. We claim the ident is explicit, even though
the author is not.
2. GIT_AUTHOR_* is set and we ask for author ident, but
not committer ident. We will claim the ident is
implicit, even though it is explicit.
This patch uses two variables instead of one, updates both
when we set the "fallback" values, and updates them
individually when we read from the environment.
Rather than keep user_ident_sufficiently_given as a
compatibility wrapper, we update the only two callers to
check the committer_ident, which matches their intent and
what was happening already.
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/commit.c | 4 ++--
cache.h | 3 ++-
ident.c | 32 +++++++++++++++++++++++++-------
3 files changed, 29 insertions(+), 10 deletions(-)
diff --git a/builtin/commit.c b/builtin/commit.c
index 1dd2ec5..d6dd3df 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -755,7 +755,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
ident_shown++ ? "" : "\n",
author_ident->buf);
- if (!user_ident_sufficiently_given())
+ if (!committer_ident_sufficiently_given())
status_printf_ln(s, GIT_COLOR_NORMAL,
_("%s"
"Committer: %s"),
@@ -1265,7 +1265,7 @@ static void print_summary(const char *prefix, const unsigned char *sha1,
strbuf_addstr(&format, "\n Author: ");
strbuf_addbuf_percentquote(&format, &author_ident);
}
- if (!user_ident_sufficiently_given()) {
+ if (!committer_ident_sufficiently_given()) {
strbuf_addstr(&format, "\n Committer: ");
strbuf_addbuf_percentquote(&format, &committer_ident);
if (advice_implicit_identity) {
diff --git a/cache.h b/cache.h
index 50d9eea..18fdd18 100644
--- a/cache.h
+++ b/cache.h
@@ -1149,7 +1149,8 @@ struct config_include_data {
#define CONFIG_INCLUDE_INIT { 0 }
extern int git_config_include(const char *name, const char *value, void *data);
-extern int user_ident_sufficiently_given(void);
+extern int committer_ident_sufficiently_given(void);
+extern int author_ident_sufficiently_given(void);
extern const char *git_commit_encoding;
extern const char *git_log_output_encoding;
diff --git a/ident.c b/ident.c
index 733d69d..ac9672f 100644
--- a/ident.c
+++ b/ident.c
@@ -14,7 +14,8 @@ static char git_default_date[50];
#define IDENT_NAME_GIVEN 01
#define IDENT_MAIL_GIVEN 02
#define IDENT_ALL_GIVEN (IDENT_NAME_GIVEN|IDENT_MAIL_GIVEN)
-static int user_ident_explicitly_given;
+static int committer_ident_explicitly_given;
+static int author_ident_explicitly_given;
#ifdef NO_GECOS_IN_PWENT
#define get_gecos(ignored) "&"
@@ -113,7 +114,8 @@ const char *ident_default_email(void)
if (email && email[0]) {
strbuf_addstr(&git_default_email, email);
- user_ident_explicitly_given |= IDENT_MAIL_GIVEN;
+ committer_ident_explicitly_given |= IDENT_MAIL_GIVEN;
+ author_ident_explicitly_given |= IDENT_MAIL_GIVEN;
} else
copy_email(xgetpwuid_self(), &git_default_email);
strbuf_trim(&git_default_email);
@@ -331,6 +333,10 @@ const char *fmt_name(const char *name, const char *email)
const char *git_author_info(int flag)
{
+ if (getenv("GIT_AUTHOR_NAME"))
+ author_ident_explicitly_given |= IDENT_NAME_GIVEN;
+ if (getenv("GIT_AUTHOR_EMAIL"))
+ author_ident_explicitly_given |= IDENT_MAIL_GIVEN;
return fmt_ident(getenv("GIT_AUTHOR_NAME"),
getenv("GIT_AUTHOR_EMAIL"),
getenv("GIT_AUTHOR_DATE"),
@@ -340,16 +346,16 @@ const char *git_author_info(int flag)
const char *git_committer_info(int flag)
{
if (getenv("GIT_COMMITTER_NAME"))
- user_ident_explicitly_given |= IDENT_NAME_GIVEN;
+ committer_ident_explicitly_given |= IDENT_NAME_GIVEN;
if (getenv("GIT_COMMITTER_EMAIL"))
- user_ident_explicitly_given |= IDENT_MAIL_GIVEN;
+ committer_ident_explicitly_given |= IDENT_MAIL_GIVEN;
return fmt_ident(getenv("GIT_COMMITTER_NAME"),
getenv("GIT_COMMITTER_EMAIL"),
getenv("GIT_COMMITTER_DATE"),
flag);
}
-int user_ident_sufficiently_given(void)
+static int ident_is_sufficient(int user_ident_explicitly_given)
{
#ifndef WINDOWS
return (user_ident_explicitly_given & IDENT_MAIL_GIVEN);
@@ -358,6 +364,16 @@ int user_ident_sufficiently_given(void)
#endif
}
+int committer_ident_sufficiently_given(void)
+{
+ return ident_is_sufficient(committer_ident_explicitly_given);
+}
+
+int author_ident_sufficiently_given(void)
+{
+ return ident_is_sufficient(author_ident_explicitly_given);
+}
+
int git_ident_config(const char *var, const char *value, void *data)
{
if (!strcmp(var, "user.name")) {
@@ -365,7 +381,8 @@ int git_ident_config(const char *var, const char *value, void *data)
return config_error_nonbool(var);
strbuf_reset(&git_default_name);
strbuf_addstr(&git_default_name, value);
- user_ident_explicitly_given |= IDENT_NAME_GIVEN;
+ committer_ident_explicitly_given |= IDENT_NAME_GIVEN;
+ author_ident_explicitly_given |= IDENT_NAME_GIVEN;
return 0;
}
@@ -374,7 +391,8 @@ int git_ident_config(const char *var, const char *value, void *data)
return config_error_nonbool(var);
strbuf_reset(&git_default_email);
strbuf_addstr(&git_default_email, value);
- user_ident_explicitly_given |= IDENT_MAIL_GIVEN;
+ committer_ident_explicitly_given |= IDENT_MAIL_GIVEN;
+ author_ident_explicitly_given |= IDENT_MAIL_GIVEN;
return 0;
}
--
1.8.0.207.gdf2154c
^ permalink raw reply related [flat|nested] 76+ messages in thread* Re: [PATCHv2 4/8] ident: keep separate "explicit" flags for author and committer
2012-11-15 0:34 ` [PATCHv2 4/8] ident: keep separate "explicit" flags for author and committer Jeff King
@ 2012-11-15 8:04 ` Jonathan Nieder
0 siblings, 0 replies; 76+ messages in thread
From: Jonathan Nieder @ 2012-11-15 8:04 UTC (permalink / raw)
To: Jeff King; +Cc: git, Felipe Contreras, Thomas Rast, Junio C Hamano
Jeff King wrote:
> 1. GIT_COMMITTER_* is set explicitly, but we fallback for
> GIT_AUTHOR. We claim the ident is explicit, even though
> the author is not.
>
> 2. GIT_AUTHOR_* is set and we ask for author ident, but
> not committer ident. We will claim the ident is
> implicit, even though it is explicit.
>
> This patch uses two variables instead of one, updates both
> when we set the "fallback" values, and updates them
> individually when we read from the environment.
Nice problem description. The fixed behavior makes sense to me, for
what it's worth.
Not about this patch, but: in case (1), shouldn't the author fall
back to $GIT_COMMITER_NAME?
^ permalink raw reply [flat|nested] 76+ messages in thread
* [PATCHv2 5/8] var: accept multiple variables on the command line
2012-11-15 0:30 ` [PATCHv2 0/8] loosening "sender" prompt in send-email Jeff King
` (3 preceding siblings ...)
2012-11-15 0:34 ` [PATCHv2 4/8] ident: keep separate "explicit" flags for author and committer Jeff King
@ 2012-11-15 0:35 ` Jeff King
2012-11-15 8:10 ` Jonathan Nieder
2012-11-15 0:35 ` [PATCHv2 6/8] var: provide explicit/implicit ident information Jeff King
` (2 subsequent siblings)
7 siblings, 1 reply; 76+ messages in thread
From: Jeff King @ 2012-11-15 0:35 UTC (permalink / raw)
To: git; +Cc: Felipe Contreras, git, Thomas Rast, Junio C Hamano,
Jonathan Nieder
Git-var currently only accepts a single value to print. This
is inefficient if the caller is interested in finding
multiple values, as they must invoke git-var multiple times.
This patch lets callers specify multiple variables, and
prints one per line.
While we're in the area, let's add some basic tests for "git
var", which until now was largely untested (and of course a
test for multiple values on top).
Signed-off-by: Jeff King <peff@peff.net>
---
Documentation/git-var.txt | 9 +++++++--
builtin/var.c | 17 +++++++++-------
t/t0007-git-var.sh | 50 +++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 67 insertions(+), 9 deletions(-)
create mode 100755 t/t0007-git-var.sh
diff --git a/Documentation/git-var.txt b/Documentation/git-var.txt
index 67edf58..53abba5 100644
--- a/Documentation/git-var.txt
+++ b/Documentation/git-var.txt
@@ -9,11 +9,16 @@ git-var - Show a git logical variable
SYNOPSIS
--------
[verse]
-'git var' ( -l | <variable> )
+'git var' ( -l | <variable>... )
DESCRIPTION
-----------
-Prints a git logical variable.
+Prints one or more git logical variables, separated by newlines.
+
+Note that some variables may contain newlines themselves (e.g.,
+`GIT_EDITOR`), and it is therefore possible to receive ambiguous output
+when requesting multiple variables. This can be mitigated by putting any
+such variables at the end of the list.
OPTIONS
-------
diff --git a/builtin/var.c b/builtin/var.c
index aedbb53..49ab300 100644
--- a/builtin/var.c
+++ b/builtin/var.c
@@ -5,7 +5,7 @@
*/
#include "builtin.h"
-static const char var_usage[] = "git var (-l | <variable>)";
+static const char var_usage[] = "git var (-l | <variable>...)";
static const char *editor(int flag)
{
@@ -73,21 +73,24 @@ static int show_config(const char *var, const char *value, void *cb)
int cmd_var(int argc, const char **argv, const char *prefix)
{
- const char *val = NULL;
- if (argc != 2)
+ if (argc < 2)
usage(var_usage);
if (strcmp(argv[1], "-l") == 0) {
+ if (argc > 2)
+ usage(var_usage);
git_config(show_config, NULL);
list_vars();
return 0;
}
git_config(git_default_config, NULL);
- val = read_var(argv[1]);
- if (!val)
- usage(var_usage);
- printf("%s\n", val);
+ while (*++argv) {
+ const char *val = read_var(*argv);
+ if (!val)
+ usage(var_usage);
+ printf("%s\n", val);
+ }
return 0;
}
diff --git a/t/t0007-git-var.sh b/t/t0007-git-var.sh
new file mode 100755
index 0000000..ec5d946
--- /dev/null
+++ b/t/t0007-git-var.sh
@@ -0,0 +1,50 @@
+#!/bin/sh
+
+test_description='basic sanity checks for git var'
+. ./test-lib.sh
+
+test_expect_success 'get GIT_AUTHOR_IDENT' '
+ test_tick &&
+ echo "$GIT_AUTHOR_NAME <$GIT_AUTHOR_EMAIL> $GIT_AUTHOR_DATE" >expect &&
+ git var GIT_AUTHOR_IDENT >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'get GIT_COMMITTER_IDENT' '
+ test_tick &&
+ echo "$GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE" >expect &&
+ git var GIT_COMMITTER_IDENT >actual &&
+ test_cmp expect actual
+'
+
+# For git var -l, we check only a representative variable;
+# testing the whole output would make our test too brittle with
+# respect to unrelated changes in the test suite's environment.
+test_expect_success 'git var -l lists variables' '
+ git var -l >actual &&
+ echo "$GIT_AUTHOR_NAME <$GIT_AUTHOR_EMAIL> $GIT_AUTHOR_DATE" >expect &&
+ sed -n s/GIT_AUTHOR_IDENT=//p <actual >actual.author &&
+ test_cmp expect actual.author
+'
+
+test_expect_success 'git var -l lists config' '
+ git var -l >actual &&
+ echo false >expect &&
+ sed -n s/core\\.bare=//p <actual >actual.bare &&
+ test_cmp expect actual.bare
+'
+
+test_expect_success 'listing and asking for variables are exclusive' '
+ test_must_fail git var -l GIT_COMMITTER_IDENT
+'
+
+test_expect_success 'git var can show multiple values' '
+ cat >expect <<-EOF &&
+ $GIT_AUTHOR_NAME <$GIT_AUTHOR_EMAIL> $GIT_AUTHOR_DATE
+ $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
+ EOF
+ git var GIT_AUTHOR_IDENT GIT_COMMITTER_IDENT >actual &&
+ test_cmp expect actual
+'
+
+test_done
--
1.8.0.207.gdf2154c
^ permalink raw reply related [flat|nested] 76+ messages in thread* [PATCHv2 6/8] var: provide explicit/implicit ident information
2012-11-15 0:30 ` [PATCHv2 0/8] loosening "sender" prompt in send-email Jeff King
` (4 preceding siblings ...)
2012-11-15 0:35 ` [PATCHv2 5/8] var: accept multiple variables on the command line Jeff King
@ 2012-11-15 0:35 ` Jeff King
2012-11-15 0:36 ` [PATCHv2 7/8] Git.pm: teach "ident" to query explicitness Jeff King
2012-11-15 0:36 ` [PATCHv2 8/8] send-email: do not prompt for explicit repo ident Jeff King
7 siblings, 0 replies; 76+ messages in thread
From: Jeff King @ 2012-11-15 0:35 UTC (permalink / raw)
To: git; +Cc: Felipe Contreras, git, Thomas Rast, Junio C Hamano,
Jonathan Nieder
Internally, we keep track of whether the author or committer
ident information was provided by the user, or whether it
was implicitly determined by the system. However, there is
currently no way for external programs or scripts to get
this information without re-implementing the ident logic
themselves. Let's provide a way for them to do so.
Signed-off-by: Jeff King <peff@peff.net>
---
Documentation/git-var.txt | 12 ++++++++++++
builtin/var.c | 27 +++++++++++++++++++++++++++
t/t0007-git-var.sh | 36 ++++++++++++++++++++++++++++++++++++
3 files changed, 75 insertions(+)
diff --git a/Documentation/git-var.txt b/Documentation/git-var.txt
index 53abba5..bb2997d 100644
--- a/Documentation/git-var.txt
+++ b/Documentation/git-var.txt
@@ -39,9 +39,21 @@ VARIABLES
GIT_AUTHOR_IDENT::
The author of a piece of code.
+GIT_AUTHOR_EXPLICIT::
+ Outputs "1" if the author identity was provided explicitly by the
+ user, or "0" if it was determined implicitly from the system.
+ Identity is considered explicit if it comes from one of git's config
+ file (i.e., via the `user.*` variables), from the `$GIT_AUTHOR_*`
+ environment variables, or from the `$EMAIL` environment variable. It
+ is considered implicit if it was generated from system variables
+ like the username and hostname.
+
GIT_COMMITTER_IDENT::
The person who put a piece of code into git.
+GIT_COMMITTER_EXPLICIT::
+ Like GIT_AUTHOR_EXPLICIT, but for the committer ident.
+
GIT_EDITOR::
Text editor for use by git commands. The value is meant to be
interpreted by the shell when it is used. Examples: `~/bin/vi`,
diff --git a/builtin/var.c b/builtin/var.c
index 49ab300..9503df9 100644
--- a/builtin/var.c
+++ b/builtin/var.c
@@ -26,13 +26,40 @@ static const char *pager(int flag)
return pgm;
}
+static const char *explicit_ident(const char * (*get_ident)(int),
+ int (*check_ident)(void))
+{
+ /*
+ * Prime the "explicit" flag by getting the identity.
+ * We do not want to be strict here, because we would not want
+ * to die on bogus implicit values, but instead just report that they
+ * are not explicit.
+ */
+ get_ident(0);
+ return check_ident() ? "1" : "0";
+}
+
+static const char *committer_explicit(int flag)
+{
+ return explicit_ident(git_committer_info,
+ committer_ident_sufficiently_given);
+}
+
+static const char *author_explicit(int flag)
+{
+ return explicit_ident(git_author_info,
+ author_ident_sufficiently_given);
+}
+
struct git_var {
const char *name;
const char *(*read)(int);
};
static struct git_var git_vars[] = {
{ "GIT_COMMITTER_IDENT", git_committer_info },
+ { "GIT_COMMITTER_EXPLICIT", committer_explicit },
{ "GIT_AUTHOR_IDENT", git_author_info },
+ { "GIT_AUTHOR_EXPLICIT", author_explicit },
{ "GIT_EDITOR", editor },
{ "GIT_PAGER", pager },
{ "", NULL },
diff --git a/t/t0007-git-var.sh b/t/t0007-git-var.sh
index ec5d946..47907de 100755
--- a/t/t0007-git-var.sh
+++ b/t/t0007-git-var.sh
@@ -47,4 +47,40 @@ test_expect_success 'git var can show multiple values' '
test_cmp expect actual
'
+for type in AUTHOR COMMITTER; do
+ test_expect_success "$type ident can detect implicit values" "
+ echo 0 >expect &&
+ (
+ sane_unset GIT_${type}_NAME &&
+ sane_unset GIT_${type}_EMAIL &&
+ sane_unset EMAIL &&
+ git var GIT_${type}_EXPLICIT >actual
+ ) &&
+ test_cmp expect actual
+ "
+
+ test_expect_success "$type ident is explicit via environment" "
+ echo 1 >expect &&
+ (
+ GIT_${type}_NAME='A Name' &&
+ export GIT_${type}_NAME &&
+ GIT_${type}_EMAIL='name@example.com' &&
+ export GIT_${type}_EMAIL &&
+ git var GIT_${type}_EXPLICIT >actual
+ ) &&
+ test_cmp expect actual
+ "
+
+ test_expect_success "$type ident is explicit via config" "
+ echo 1 >expect &&
+ test_config user.name 'A Name' &&
+ test_config user.email 'name@example.com' &&
+ (
+ sane_unset GIT_${type}_NAME &&
+ sane_unset GIT_${type}_EMAIL &&
+ git var GIT_${type}_EXPLICIT >actual
+ )
+ "
+done
+
test_done
--
1.8.0.207.gdf2154c
^ permalink raw reply related [flat|nested] 76+ messages in thread* [PATCHv2 7/8] Git.pm: teach "ident" to query explicitness
2012-11-15 0:30 ` [PATCHv2 0/8] loosening "sender" prompt in send-email Jeff King
` (5 preceding siblings ...)
2012-11-15 0:35 ` [PATCHv2 6/8] var: provide explicit/implicit ident information Jeff King
@ 2012-11-15 0:36 ` Jeff King
2012-11-15 8:13 ` Jonathan Nieder
2012-11-15 0:36 ` [PATCHv2 8/8] send-email: do not prompt for explicit repo ident Jeff King
7 siblings, 1 reply; 76+ messages in thread
From: Jeff King @ 2012-11-15 0:36 UTC (permalink / raw)
To: git; +Cc: Felipe Contreras, git, Thomas Rast, Junio C Hamano,
Jonathan Nieder
"git var" recently learned to report on whether an ident we
fetch from it was configured explicitly or implicitly. Let's
make that information available to callers of the ident
function.
Because evaluating "ident" in an array versus scalar context
already has a meaning, we cannot return our extra value in a
backwards compatible way. Instead, we require the caller to
add an extra "explicit" flag to request the information.
The ident_person function, on the other hand, always returns
a scalar, so we are free to overload it in an array context.
Signed-off-by: Jeff King <peff@peff.net>
---
perl/Git.pm | 32 ++++++++++++++++++++++++--------
1 file changed, 24 insertions(+), 8 deletions(-)
diff --git a/perl/Git.pm b/perl/Git.pm
index 497f420..28075a9 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -737,7 +737,7 @@ sub remote_refs {
}
-=item ident ( TYPE | IDENTSTR )
+=item ident ( TYPE | IDENTSTR [, options] )
=item ident_person ( TYPE | IDENTSTR | IDENTARRAY )
@@ -750,8 +750,16 @@ and either returns it as a scalar string or as an array with the fields parsed.
Alternatively, it can take a prepared ident string (e.g. from the commit
object) and just parse it.
+If the C<explicit> option is set to 1, we are in an array context, and we are
+using C<git var> to lookup the ident, the returned array will contain an
+additional boolean element specifying whether the ident was configured
+explicitly by the user. See the description of GIT_*_EXPLICIT in git-var(1) for
+details.
+
C<ident_person> returns the person part of the ident - name and email;
it can take the same arguments as C<ident> or the array returned by C<ident>.
+In an array context, C<ident_person> returns both the person string and the
+C<explicit> boolean.
The synopsis is like:
@@ -763,17 +771,22 @@ The synopsis is like:
=cut
sub ident {
- my ($self, $type) = _maybe_self(@_);
- my $identstr;
+ my ($self, $type, %options) = _maybe_self(@_);
+ my ($identstr, $explicit);
if (lc $type eq lc 'committer' or lc $type eq lc 'author') {
- my @cmd = ('var', 'GIT_'.uc($type).'_IDENT');
+ my $uc = uc($type);
+ my @cmd = ('var', "GIT_${uc}_IDENT", "GIT_${uc}_EXPLICIT");
unshift @cmd, $self if $self;
- $identstr = command_oneline(@cmd);
+ ($identstr, $explicit) = command(@cmd);
} else {
$identstr = $type;
}
if (wantarray) {
- return $identstr =~ /^(.*) <(.*)> (\d+ [+-]\d{4})$/;
+ my @ret = $identstr =~ /^(.*) <(.*)> (\d+ [+-]\d{4})$/;
+ if ($options{explicit} && defined $explicit) {
+ push @ret, $explicit;
+ }
+ return @ret;
} else {
return $identstr;
}
@@ -781,8 +794,11 @@ sub ident {
sub ident_person {
my ($self, @ident) = _maybe_self(@_);
- $#ident == 0 and @ident = $self ? $self->ident($ident[0]) : ident($ident[0]);
- return "$ident[0] <$ident[1]>";
+ $#ident == 0 and @ident = $self ?
+ $self->ident($ident[0], explicit => 1) :
+ ident($ident[0], explicit => 1);
+ my $ret = "$ident[0] <$ident[1]>";
+ return wantarray ? ($ret, @ident[3]) : $ret;
}
--
1.8.0.207.gdf2154c
^ permalink raw reply related [flat|nested] 76+ messages in thread* [PATCHv2 8/8] send-email: do not prompt for explicit repo ident
2012-11-15 0:30 ` [PATCHv2 0/8] loosening "sender" prompt in send-email Jeff King
` (6 preceding siblings ...)
2012-11-15 0:36 ` [PATCHv2 7/8] Git.pm: teach "ident" to query explicitness Jeff King
@ 2012-11-15 0:36 ` Jeff King
2012-11-15 2:08 ` Felipe Contreras
2012-11-15 8:23 ` Jonathan Nieder
7 siblings, 2 replies; 76+ messages in thread
From: Jeff King @ 2012-11-15 0:36 UTC (permalink / raw)
To: git; +Cc: Felipe Contreras, git, Thomas Rast, Junio C Hamano,
Jonathan Nieder
If git-send-email is configured with sendemail.from, we will
not prompt the user for the "From" address of the emails.
If it is not configured, we prompt the user, but provide the
repo author or committer as a default. Even though we
probably have a sensible value for the default, the prompt
is a safety check in case git generated an incorrect
implicit ident string.
Now that Git.pm will tell us whether the ident is implicit or
explicit, we can stop prompting in the explicit case, saving
most users from having to see the prompt at all.
Signed-off-by: Jeff King <peff@peff.net>
---
git-send-email.perl | 22 +++++++++++++---------
t/t9001-send-email.sh | 36 ++++++++++++++++++++++++++++++++++--
2 files changed, 47 insertions(+), 11 deletions(-)
diff --git a/git-send-email.perl b/git-send-email.perl
index 5a7c29d..0c49b32 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -436,9 +436,8 @@ if (0) {
}
}
-my ($repoauthor, $repocommitter);
-($repoauthor) = Git::ident_person(@repo, 'author');
-($repocommitter) = Git::ident_person(@repo, 'committer');
+my ($repoauthor, $author_explicit) = Git::ident_person(@repo, 'author');
+my ($repocommitter, $committer_explicit) = Git::ident_person(@repo, 'committer');
# Verify the user input
@@ -755,12 +754,17 @@ if (!$force) {
my $prompting = 0;
if (!defined $sender) {
- $sender = $repoauthor || $repocommitter || '';
- $sender = ask("Who should the emails appear to be from? [$sender] ",
- default => $sender,
- valid_re => qr/\@.*\./, confirm_only => 1);
- print "Emails will be sent from: ", $sender, "\n";
- $prompting++;
+ ($sender, my $explicit) =
+ defined $repoauthor ? ($repoauthor, $author_explicit) :
+ defined $repocommitter ? ($repocommitter, $committer_explicit) :
+ ('', 0);
+ if (!$explicit) {
+ $sender = ask("Who should the emails appear to be from? [$sender] ",
+ default => $sender,
+ valid_re => qr/\@.*\./, confirm_only => 1);
+ print "Emails will be sent from: ", $sender, "\n";
+ $prompting++;
+ }
}
if (!@initial_to && !defined $to_cmd) {
diff --git a/t/t9001-send-email.sh b/t/t9001-send-email.sh
index 6c6af7d..0fe0b8e 100755
--- a/t/t9001-send-email.sh
+++ b/t/t9001-send-email.sh
@@ -191,15 +191,47 @@ test_expect_success $PREREQ 'Show all headers' '
test_expect_success $PREREQ 'Prompting works' '
clean_fake_sendmail &&
- (echo "Example <from@example.com>"
- echo "to@example.com"
+ (echo "to@example.com"
echo ""
) | GIT_SEND_EMAIL_NOTTY=1 git send-email \
--smtp-server="$(pwd)/fake.sendmail" \
$patches \
2>errors &&
+ grep "^From: A U Thor <author@example.com>\$" msgtxt1 &&
+ grep "^To: to@example.com\$" msgtxt1
+'
+
+test_expect_success $PREREQ,AUTOIDENT 'implicit ident prompts for sender' '
+ clean_fake_sendmail &&
+ (echo "Example <from@example.com>" &&
+ echo "to@example.com" &&
+ echo ""
+ ) |
+ (sane_unset GIT_AUTHOR_NAME &&
+ sane_unset GIT_AUTHOR_EMAIL &&
+ sane_unset GIT_COMMITTER_NAME &&
+ sane_unset GIT_COMMITTER_EMAIL &&
+ GIT_SEND_EMAIL_NOTTY=1 git send-email \
+ --smtp-server="$(pwd)/fake.sendmail" \
+ $patches \
+ 2>errors &&
grep "^From: Example <from@example.com>\$" msgtxt1 &&
grep "^To: to@example.com\$" msgtxt1
+ )
+'
+
+test_expect_success $PREREQ,!AUTOIDENT 'broken implicit ident aborts send-email' '
+ clean_fake_sendmail &&
+ (sane_unset GIT_AUTHOR_NAME &&
+ sane_unset GIT_AUTHOR_EMAIL &&
+ sane_unset GIT_COMMITTER_NAME &&
+ sane_unset GIT_COMMITTER_EMAIL &&
+ GIT_SEND_EMAIL_NOTTY=1 && export GIT_SEND_EMAIL_NOTTY &&
+ test_must_fail git send-email \
+ --smtp-server="$(pwd)/fake.sendmail" \
+ $patches </dev/null 2>errors.out &&
+ test_i18ngrep "tell me who you are" errors.out
+ )
'
test_expect_success $PREREQ 'tocmd works' '
--
1.8.0.207.gdf2154c
^ permalink raw reply related [flat|nested] 76+ messages in thread* Re: [PATCHv2 8/8] send-email: do not prompt for explicit repo ident
2012-11-15 0:36 ` [PATCHv2 8/8] send-email: do not prompt for explicit repo ident Jeff King
@ 2012-11-15 2:08 ` Felipe Contreras
2012-11-15 8:33 ` Jeff King
2012-11-15 8:23 ` Jonathan Nieder
1 sibling, 1 reply; 76+ messages in thread
From: Felipe Contreras @ 2012-11-15 2:08 UTC (permalink / raw)
To: Jeff King; +Cc: git, Thomas Rast, Junio C Hamano, Jonathan Nieder
On Thu, Nov 15, 2012 at 1:36 AM, Jeff King <peff@peff.net> wrote:
> diff --git a/git-send-email.perl b/git-send-email.perl
> index 5a7c29d..0c49b32 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -436,9 +436,8 @@ if (0) {
> }
> }
>
> -my ($repoauthor, $repocommitter);
> -($repoauthor) = Git::ident_person(@repo, 'author');
> -($repocommitter) = Git::ident_person(@repo, 'committer');
> +my ($repoauthor, $author_explicit) = Git::ident_person(@repo, 'author');
> +my ($repocommitter, $committer_explicit) = Git::ident_person(@repo, 'committer');
>
> # Verify the user input
>
> @@ -755,12 +754,17 @@ if (!$force) {
>
> my $prompting = 0;
> if (!defined $sender) {
> - $sender = $repoauthor || $repocommitter || '';
> - $sender = ask("Who should the emails appear to be from? [$sender] ",
> - default => $sender,
> - valid_re => qr/\@.*\./, confirm_only => 1);
> - print "Emails will be sent from: ", $sender, "\n";
> - $prompting++;
> + ($sender, my $explicit) =
> + defined $repoauthor ? ($repoauthor, $author_explicit) :
> + defined $repocommitter ? ($repocommitter, $committer_explicit) :
> + ('', 0);
> + if (!$explicit) {
> + $sender = ask("Who should the emails appear to be from? [$sender] ",
> + default => $sender,
> + valid_re => qr/\@.*\./, confirm_only => 1);
> + print "Emails will be sent from: ", $sender, "\n";
> + $prompting++;
> + }
> }
>
> if (!@initial_to && !defined $to_cmd) {
I don't think there's any need for all that, this does the trick:
diff --git a/git-send-email.perl b/git-send-email.perl
index aea66a0..503e551 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -748,16 +748,11 @@ if (!$force) {
}
}
-my $prompting = 0;
if (!defined $sender) {
$sender = $repoauthor || $repocommitter || '';
- $sender = ask("Who should the emails appear to be from? [$sender] ",
- default => $sender,
- valid_re => qr/\@.*\./, confirm_only => 1);
- print "Emails will be sent from: ", $sender, "\n";
- $prompting++;
}
+my $prompting = 0;
This passes all the current tests and the ones you added.
Which kind of user will get the prompt with your patch, that would
miss it with mine?
Cheers.
--
Felipe Contreras
^ permalink raw reply related [flat|nested] 76+ messages in thread* Re: [PATCHv2 8/8] send-email: do not prompt for explicit repo ident
2012-11-15 2:08 ` Felipe Contreras
@ 2012-11-15 8:33 ` Jeff King
2012-11-15 10:28 ` Felipe Contreras
0 siblings, 1 reply; 76+ messages in thread
From: Jeff King @ 2012-11-15 8:33 UTC (permalink / raw)
To: Felipe Contreras; +Cc: git, Thomas Rast, Junio C Hamano, Jonathan Nieder
On Thu, Nov 15, 2012 at 03:08:42AM +0100, Felipe Contreras wrote:
> I don't think there's any need for all that, this does the trick:
>
> diff --git a/git-send-email.perl b/git-send-email.perl
> index aea66a0..503e551 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -748,16 +748,11 @@ if (!$force) {
> }
> }
>
> -my $prompting = 0;
> if (!defined $sender) {
> $sender = $repoauthor || $repocommitter || '';
> - $sender = ask("Who should the emails appear to be from? [$sender] ",
> - default => $sender,
> - valid_re => qr/\@.*\./, confirm_only => 1);
> - print "Emails will be sent from: ", $sender, "\n";
> - $prompting++;
> }
>
> +my $prompting = 0;
>
> This passes all the current tests and the ones you added.
It may pass on your system, but it will not on a system that meets the
AUTOIDENT prerequisite (it fails the new t9001.19 on my system; I
suspect your system config is such that we skip t9001.19 and run
t9001.20, whereas mine is the opposite).
> Which kind of user will get the prompt with your patch, that would
> miss it with mine?
One whose system is configured in such a way that git can produce an
automatic ident (i.e., has a non-blank GECOS name and a FQDN).
-Peff
^ permalink raw reply [flat|nested] 76+ messages in thread* Re: [PATCHv2 8/8] send-email: do not prompt for explicit repo ident
2012-11-15 8:33 ` Jeff King
@ 2012-11-15 10:28 ` Felipe Contreras
2012-11-15 10:43 ` Jeff King
0 siblings, 1 reply; 76+ messages in thread
From: Felipe Contreras @ 2012-11-15 10:28 UTC (permalink / raw)
To: Jeff King; +Cc: git, Thomas Rast, Junio C Hamano, Jonathan Nieder
On Thu, Nov 15, 2012 at 9:33 AM, Jeff King <peff@peff.net> wrote:
> On Thu, Nov 15, 2012 at 03:08:42AM +0100, Felipe Contreras wrote:
>
>> I don't think there's any need for all that, this does the trick:
>>
>> diff --git a/git-send-email.perl b/git-send-email.perl
>> index aea66a0..503e551 100755
>> --- a/git-send-email.perl
>> +++ b/git-send-email.perl
>> @@ -748,16 +748,11 @@ if (!$force) {
>> }
>> }
>>
>> -my $prompting = 0;
>> if (!defined $sender) {
>> $sender = $repoauthor || $repocommitter || '';
>> - $sender = ask("Who should the emails appear to be from? [$sender] ",
>> - default => $sender,
>> - valid_re => qr/\@.*\./, confirm_only => 1);
>> - print "Emails will be sent from: ", $sender, "\n";
>> - $prompting++;
>> }
>>
>> +my $prompting = 0;
>>
>> This passes all the current tests and the ones you added.
>
> It may pass on your system, but it will not on a system that meets the
> AUTOIDENT prerequisite (it fails the new t9001.19 on my system; I
> suspect your system config is such that we skip t9001.19 and run
> t9001.20, whereas mine is the opposite).
I tried both:
ok 19 # skip implicit ident prompts for sender (missing AUTOIDENT of
PERL,AUTOIDENT)
ok 20 - broken implicit ident aborts send-email
ok 19 - implicit ident prompts for sender
ok 20 # skip broken implicit ident aborts send-email (missing
!AUTOIDENT of PERL,!AUTOIDENT)
However, it would be much easier if ident learned to check
GIT_TEST_FAKE_HOSTNAME, or something.
But then I realized I had to run 'make' again. Yes, my patch breaks
test 19, but see below.
>> Which kind of user will get the prompt with your patch, that would
>> miss it with mine?
>
> One whose system is configured in such a way that git can produce an
> automatic ident (i.e., has a non-blank GECOS name and a FQDN).
And doesn't have any of the following:
* configured user.name/user.email
* specified $EMAIL
* configured sendemail.from
* specified --from argument
Very unlikely. And then, what would be the consequences of not
receiving this prompt?
Cheers.
--
Felipe Contreras
^ permalink raw reply [flat|nested] 76+ messages in thread* Re: [PATCHv2 8/8] send-email: do not prompt for explicit repo ident
2012-11-15 10:28 ` Felipe Contreras
@ 2012-11-15 10:43 ` Jeff King
2012-11-15 11:13 ` Jeff King
0 siblings, 1 reply; 76+ messages in thread
From: Jeff King @ 2012-11-15 10:43 UTC (permalink / raw)
To: Felipe Contreras; +Cc: git, Thomas Rast, Junio C Hamano, Jonathan Nieder
On Thu, Nov 15, 2012 at 11:28:46AM +0100, Felipe Contreras wrote:
> I tried both:
>
> ok 19 # skip implicit ident prompts for sender (missing AUTOIDENT of
> PERL,AUTOIDENT)
> ok 20 - broken implicit ident aborts send-email
>
> ok 19 - implicit ident prompts for sender
> ok 20 # skip broken implicit ident aborts send-email (missing
> !AUTOIDENT of PERL,!AUTOIDENT)
>
> However, it would be much easier if ident learned to check
> GIT_TEST_FAKE_HOSTNAME, or something.
Yes, it would be. It has two downsides:
1. The regular git code has to be instrumented to respect the
variable, so it can potentially affect git in production use
outside of the test suite. Since such code is simple, though, it is
probably not a big risk.
2. We would not actually exercise the code paths for doing
hostname and GECOS lookup. We do not test their resulting values,
so the coverage is not great now, but we do at least run the code,
which would let a run with "--valgrind" check it. I guess we could
go through the motions of assembling the ident and then replace
it at the end with the fake value.
I don't have a strong opinion either way.
> > One whose system is configured in such a way that git can produce an
> > automatic ident (i.e., has a non-blank GECOS name and a FQDN).
>
> And doesn't have any of the following:
>
> * configured user.name/user.email
> * specified $EMAIL
> * configured sendemail.from
> * specified --from argument
>
> Very unlikely.
That is certainly the opinion you have stated already. I'm not sure I
agree. Linus, for example, was an advocate of such a configuration early
on in git's history. I don't think he still runs that way, though.
> And then, what would be the consequences of not receiving this prompt?
An email would be sent with the generated identity.
-Peff
^ permalink raw reply [flat|nested] 76+ messages in thread* Re: [PATCHv2 8/8] send-email: do not prompt for explicit repo ident
2012-11-15 10:43 ` Jeff King
@ 2012-11-15 11:13 ` Jeff King
2012-11-15 11:50 ` Jeff King
2012-11-15 16:56 ` Junio C Hamano
0 siblings, 2 replies; 76+ messages in thread
From: Jeff King @ 2012-11-15 11:13 UTC (permalink / raw)
To: Felipe Contreras; +Cc: git, Thomas Rast, Junio C Hamano, Jonathan Nieder
On Thu, Nov 15, 2012 at 02:43:58AM -0800, Jeff King wrote:
> > And doesn't have any of the following:
> >
> > * configured user.name/user.email
> > * specified $EMAIL
> > * configured sendemail.from
> > * specified --from argument
> >
> > Very unlikely.
>
> That is certainly the opinion you have stated already. I'm not sure I
> agree. Linus, for example, was an advocate of such a configuration early
> on in git's history. I don't think he still runs that way, though.
>
> > And then, what would be the consequences of not receiving this prompt?
>
> An email would be sent with the generated identity.
I suspect you did not need me to answer that question, but were setting
it up as a rhetorical trap to mention the final confirmation, which I
failed to note in my response.
I think a much more compelling argument/commit message for your
suggested patch would be:
We currently prompt the user for the "From" address. This is an
inconvenience in the common case that the user has configured their
identity in the environment, but is meant as a safety check for when
git falls back to an implicitly generated identity (which may or may
not be valid).
That safety check is not really necessary, though, as by default
send-email will prompt the user for a final confirmation before
sending out any message. The likelihood that a user has both bothered
to turn off this default _and_ not configured any identity (nor
checked that the automatic identity is valid) is rather low.
I could accept that line of reasoning. I see that this argument is
buried deep in your commit message, but I will admit to not reading your
9-point list of conditions all that closely, as the first 7 points are,
in my opinion, not relevant (and I had already read and disagreed with
them in other messages).
-Peff
^ permalink raw reply [flat|nested] 76+ messages in thread
* Re: [PATCHv2 8/8] send-email: do not prompt for explicit repo ident
2012-11-15 11:13 ` Jeff King
@ 2012-11-15 11:50 ` Jeff King
2012-11-15 16:56 ` Junio C Hamano
1 sibling, 0 replies; 76+ messages in thread
From: Jeff King @ 2012-11-15 11:50 UTC (permalink / raw)
To: Felipe Contreras; +Cc: git, Thomas Rast, Junio C Hamano, Jonathan Nieder
On Thu, Nov 15, 2012 at 03:13:47AM -0800, Jeff King wrote:
> I think a much more compelling argument/commit message for your
> suggested patch would be:
>
> We currently prompt the user for the "From" address. This is an
> inconvenience in the common case that the user has configured their
> identity in the environment, but is meant as a safety check for when
> git falls back to an implicitly generated identity (which may or may
> not be valid).
>
> That safety check is not really necessary, though, as by default
> send-email will prompt the user for a final confirmation before
> sending out any message. The likelihood that a user has both bothered
> to turn off this default _and_ not configured any identity (nor
> checked that the automatic identity is valid) is rather low.
If that is the route we want to go, then we should obviously drop my
series in favor of your final patch. I think it would also need a test
update, no?
I think a more concise commit message would help, too. I disagree with a
great deal of the reasoning in your existing message, but those parts
turn out not to be relevant. The crux of the issue is that the safety
check is not necessary because there is already one (i.e., point 8 of
your list). Feel free to use any or all of my text above.
>From my series, there were a few cleanups that might be worth salvaging.
Here is a rundown by patch:
[1/8]: test-lib: allow negation of prerequisites
This stands on its own, and is something I have wanted a few times in
the past. However, since there is no immediate user, I don't know if it
is worth doing or not.
[2/8]: t7502: factor out autoident prerequisite
A minor cleanup and possible help to future tests, but since there are
no other callers now, not sure if it is worth it.
[3/8]: ident: make user_ident_explicitly_given static
A cleanup that is worth doing, I think.
[4/8]: ident: keep separate "explicit" flags for author and committer
Another cleanup. This is "more correct", in that it handles the corner
cases I mentioned in the commit message. But no current code cares about
those corner cases, because the only real caller is git-commit, and this
is a purely internal interface. I could take or leave it.
[5/8]: var: accept multiple variables on the command line
The tests for this can be split out; we currently don't have "git var"
tests at all, so increasing our test coverage is reasonable. The
multiple variables thing is potentially useful, but there are simply not
that many callers of "git var", and nobody has been asking for such a
feature (we could use it to save a process in git-send-email, but it is
probably not worth the complexity).
[6/8]: var: provide explicit/implicit ident information
[7/8]: Git.pm: teach "ident" to query explicitness
These two should probably be dropped. They would lock us into supporting
the explicit/implicit variables in "git var", for no immediate benefit.
[8/8]: send-email: do not prompt for explicit repo ident
Obviously drop.
> I could accept that line of reasoning. I see that this argument is
> buried deep in your commit message, but I will admit to not reading your
> 9-point list of conditions all that closely, as the first 7 points are,
> in my opinion, not relevant (and I had already read and disagreed with
> them in other messages).
If it sounds like I am blaming you here, I am to some degree. But I am
also blaming myself. I should have read your commit message more
carefully, and I'm sorry for not doing so. I hope we can both try harder
to avoid getting side-tracked on arguing about issues that turned out
not to be important at all (of course, we cannot know which part of the
discussion will turn out to be important, but I think there some
obviously unproductive parts of this discussion).
-Peff
^ permalink raw reply [flat|nested] 76+ messages in thread
* Re: [PATCHv2 8/8] send-email: do not prompt for explicit repo ident
2012-11-15 11:13 ` Jeff King
2012-11-15 11:50 ` Jeff King
@ 2012-11-15 16:56 ` Junio C Hamano
2012-11-15 17:28 ` Jeff King
1 sibling, 1 reply; 76+ messages in thread
From: Junio C Hamano @ 2012-11-15 16:56 UTC (permalink / raw)
To: Jeff King; +Cc: Felipe Contreras, git, Thomas Rast, Jonathan Nieder
Jeff King <peff@peff.net> writes:
> I think a much more compelling argument/commit message for your
> suggested patch would be:
>
> We currently prompt the user for the "From" address. This is an
> inconvenience in the common case that the user has configured their
> identity in the environment, but is meant as a safety check for when
> git falls back to an implicitly generated identity (which may or may
> not be valid).
>
> That safety check is not really necessary, though, as by default
> send-email will prompt the user for a final confirmation before
> sending out any message. The likelihood that a user has both bothered
> to turn off this default _and_ not configured any identity (nor
> checked that the automatic identity is valid) is rather low.
This somehow reminds me of the first paragraph of f20f387 (commit:
check committer identity more strictly, 2012-07-23).
I never use "send-email driving format-patch" workflow myself, but I
suspect there are people among who do so who are using --compose to
do the cover letter of their series. Does the "confirmation as the
last step" help them, or would they have to retype their message?
^ permalink raw reply [flat|nested] 76+ messages in thread
* Re: [PATCHv2 8/8] send-email: do not prompt for explicit repo ident
2012-11-15 16:56 ` Junio C Hamano
@ 2012-11-15 17:28 ` Jeff King
2012-11-16 5:17 ` Junio C Hamano
0 siblings, 1 reply; 76+ messages in thread
From: Jeff King @ 2012-11-15 17:28 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Felipe Contreras, git, Thomas Rast, Jonathan Nieder
On Thu, Nov 15, 2012 at 08:56:37AM -0800, Junio C Hamano wrote:
> > I think a much more compelling argument/commit message for your
> > suggested patch would be:
> >
> > We currently prompt the user for the "From" address. This is an
> > inconvenience in the common case that the user has configured their
> > identity in the environment, but is meant as a safety check for when
> > git falls back to an implicitly generated identity (which may or may
> > not be valid).
> >
> > That safety check is not really necessary, though, as by default
> > send-email will prompt the user for a final confirmation before
> > sending out any message. The likelihood that a user has both bothered
> > to turn off this default _and_ not configured any identity (nor
> > checked that the automatic identity is valid) is rather low.
>
> This somehow reminds me of the first paragraph of f20f387 (commit:
> check committer identity more strictly, 2012-07-23).
>
> I never use "send-email driving format-patch" workflow myself, but I
> suspect there are people among who do so who are using --compose to
> do the cover letter of their series. Does the "confirmation as the
> last step" help them, or would they have to retype their message?
That is a good question. That confirmation step does come after they
have typed their cover letter. However, if they are using --compose,
they are dumped in their editor with something like:
From Jeff King <peff@peff.net> # This line is ignored.
GIT: Lines beginning in "GIT:" will be removed.
GIT: Consider including an overall diffstat or table of contents
GIT: for the patch you are writing.
GIT:
GIT: Clear the body content if you don't wish to send a summary.
From: Jeff King <peff@peff.net>
Subject:
In-Reply-To:
which I think would count as sufficient notice of the address being
used.
-Peff
^ permalink raw reply [flat|nested] 76+ messages in thread
* Re: [PATCHv2 8/8] send-email: do not prompt for explicit repo ident
2012-11-15 17:28 ` Jeff King
@ 2012-11-16 5:17 ` Junio C Hamano
2012-11-16 19:08 ` Jeff King
0 siblings, 1 reply; 76+ messages in thread
From: Junio C Hamano @ 2012-11-16 5:17 UTC (permalink / raw)
To: Jeff King; +Cc: Felipe Contreras, git, Thomas Rast, Jonathan Nieder
Jeff King <peff@peff.net> writes:
> That is a good question. That confirmation step does come after they
> have typed their cover letter. However, if they are using --compose,
> they are dumped in their editor with something like:
>
> From Jeff King <peff@peff.net> # This line is ignored.
> GIT: Lines beginning in "GIT:" will be removed.
> GIT: Consider including an overall diffstat or table of contents
> GIT: for the patch you are writing.
> GIT:
> GIT: Clear the body content if you don't wish to send a summary.
> From: Jeff King <peff@peff.net>
> Subject:
> In-Reply-To:
>
> which I think would count as sufficient notice of the address being
> used.
OK. Tentatively I replaced your old series with these 8 patches
including the last one, as I tend to agree with the value the
earlier clean-up in the series gives us in the longer term. As you
and Felipe discussed, we may want to replace the last one with a
simpler "don't bother asking" patch, but I think that is more or
less an orthogonal issue.
Thanks.
^ permalink raw reply [flat|nested] 76+ messages in thread
* Re: [PATCHv2 8/8] send-email: do not prompt for explicit repo ident
2012-11-16 5:17 ` Junio C Hamano
@ 2012-11-16 19:08 ` Jeff King
2012-11-16 19:57 ` Felipe Contreras
2012-11-16 20:04 ` Junio C Hamano
0 siblings, 2 replies; 76+ messages in thread
From: Jeff King @ 2012-11-16 19:08 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Felipe Contreras, git, Thomas Rast, Jonathan Nieder
On Thu, Nov 15, 2012 at 09:17:30PM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > That is a good question. That confirmation step does come after they
> > have typed their cover letter. However, if they are using --compose,
> > they are dumped in their editor with something like:
> >
> > From Jeff King <peff@peff.net> # This line is ignored.
> > GIT: Lines beginning in "GIT:" will be removed.
> > GIT: Consider including an overall diffstat or table of contents
> > GIT: for the patch you are writing.
> > GIT:
> > GIT: Clear the body content if you don't wish to send a summary.
> > From: Jeff King <peff@peff.net>
> > Subject:
> > In-Reply-To:
> >
> > which I think would count as sufficient notice of the address being
> > used.
>
> OK. Tentatively I replaced your old series with these 8 patches
> including the last one, as I tend to agree with the value the
> earlier clean-up in the series gives us in the longer term. As you
> and Felipe discussed, we may want to replace the last one with a
> simpler "don't bother asking" patch, but I think that is more or
> less an orthogonal issue.
I'm not sure how orthogonal it is. The latter half of my series is about
exposing the user_ident_sufficiently_given() flag. If we go with
Felipe's patch, then that exposed information has no users, and it may
not be worth it (OTOH, it's possible that some third-party script may
want it).
-Peff
^ permalink raw reply [flat|nested] 76+ messages in thread
* Re: [PATCHv2 8/8] send-email: do not prompt for explicit repo ident
2012-11-16 19:08 ` Jeff King
@ 2012-11-16 19:57 ` Felipe Contreras
2012-11-16 20:11 ` Jeff King
2012-11-16 20:04 ` Junio C Hamano
1 sibling, 1 reply; 76+ messages in thread
From: Felipe Contreras @ 2012-11-16 19:57 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git, Thomas Rast, Jonathan Nieder
On Fri, Nov 16, 2012 at 8:08 PM, Jeff King <peff@peff.net> wrote:
> On Thu, Nov 15, 2012 at 09:17:30PM -0800, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>>
>> > That is a good question. That confirmation step does come after they
>> > have typed their cover letter. However, if they are using --compose,
>> > they are dumped in their editor with something like:
>> >
>> > From Jeff King <peff@peff.net> # This line is ignored.
>> > GIT: Lines beginning in "GIT:" will be removed.
>> > GIT: Consider including an overall diffstat or table of contents
>> > GIT: for the patch you are writing.
>> > GIT:
>> > GIT: Clear the body content if you don't wish to send a summary.
>> > From: Jeff King <peff@peff.net>
>> > Subject:
>> > In-Reply-To:
>> >
>> > which I think would count as sufficient notice of the address being
>> > used.
>>
>> OK. Tentatively I replaced your old series with these 8 patches
>> including the last one, as I tend to agree with the value the
>> earlier clean-up in the series gives us in the longer term. As you
>> and Felipe discussed, we may want to replace the last one with a
>> simpler "don't bother asking" patch, but I think that is more or
>> less an orthogonal issue.
>
> I'm not sure how orthogonal it is. The latter half of my series is about
> exposing the user_ident_sufficiently_given() flag. If we go with
> Felipe's patch, then that exposed information has no users, and it may
> not be worth it (OTOH, it's possible that some third-party script may
> want it).
Well, who is using user_ident_sufficiently_given() in the first place?
I think 'git commit' might be suffering from the same problem that
prompted you to split it.
Cheers.
--
Felipe Contreras
^ permalink raw reply [flat|nested] 76+ messages in thread
* Re: [PATCHv2 8/8] send-email: do not prompt for explicit repo ident
2012-11-16 19:57 ` Felipe Contreras
@ 2012-11-16 20:11 ` Jeff King
0 siblings, 0 replies; 76+ messages in thread
From: Jeff King @ 2012-11-16 20:11 UTC (permalink / raw)
To: Felipe Contreras; +Cc: Junio C Hamano, git, Thomas Rast, Jonathan Nieder
On Fri, Nov 16, 2012 at 08:57:43PM +0100, Felipe Contreras wrote:
> > I'm not sure how orthogonal it is. The latter half of my series is about
> > exposing the user_ident_sufficiently_given() flag. If we go with
> > Felipe's patch, then that exposed information has no users, and it may
> > not be worth it (OTOH, it's possible that some third-party script may
> > want it).
>
> Well, who is using user_ident_sufficiently_given() in the first place?
> I think 'git commit' might be suffering from the same problem that
> prompted you to split it.
It is just `git commit` now. It does not suffer from the problems that
prompted the author/committer split:
http://article.gmane.org/gmane.comp.version-control.git/209635
To expand on what I wrote there, we cannot hit case 2 because we always
ask for the committer within the same process. Case 1 is not
interesting, because we would only fail to show it if is identical to a
non-implicit committer (so even if it was implicit, we know that it is a
sane value).
-Peff
^ permalink raw reply [flat|nested] 76+ messages in thread
* Re: [PATCHv2 8/8] send-email: do not prompt for explicit repo ident
2012-11-16 19:08 ` Jeff King
2012-11-16 19:57 ` Felipe Contreras
@ 2012-11-16 20:04 ` Junio C Hamano
1 sibling, 0 replies; 76+ messages in thread
From: Junio C Hamano @ 2012-11-16 20:04 UTC (permalink / raw)
To: Jeff King; +Cc: Felipe Contreras, git, Thomas Rast, Jonathan Nieder
Jeff King <peff@peff.net> writes:
> On Thu, Nov 15, 2012 at 09:17:30PM -0800, Junio C Hamano wrote:
> ...
>> OK. Tentatively I replaced your old series with these 8 patches
>> including the last one, as I tend to agree with the value the
>> earlier clean-up in the series gives us in the longer term. As you
>> and Felipe discussed, we may want to replace the last one with a
>> simpler "don't bother asking" patch, but I think that is more or
>> less an orthogonal issue.
>
> I'm not sure how orthogonal it is. The latter half of my series is about
> exposing the user_ident_sufficiently_given() flag. If we go with
> Felipe's patch, then that exposed information has no users, and it may
> not be worth it (OTOH, it's possible that some third-party script may
> want it).
Oh, no, you are correct. I shouldn't have said "the last one", but
instead "the later ones". The first two we should definitely keep,
at least, but the other three are not so clear.
^ permalink raw reply [flat|nested] 76+ messages in thread
* Re: [PATCHv2 8/8] send-email: do not prompt for explicit repo ident
2012-11-15 0:36 ` [PATCHv2 8/8] send-email: do not prompt for explicit repo ident Jeff King
2012-11-15 2:08 ` Felipe Contreras
@ 2012-11-15 8:23 ` Jonathan Nieder
1 sibling, 0 replies; 76+ messages in thread
From: Jonathan Nieder @ 2012-11-15 8:23 UTC (permalink / raw)
To: Jeff King; +Cc: git, Felipe Contreras, Thomas Rast, Junio C Hamano
Jeff King wrote:
> --- a/t/t9001-send-email.sh
> +++ b/t/t9001-send-email.sh
> @@ -191,15 +191,47 @@ test_expect_success $PREREQ 'Show all headers' '
>
> test_expect_success $PREREQ 'Prompting works' '
> clean_fake_sendmail &&
> - (echo "Example <from@example.com>"
> - echo "to@example.com"
> + (echo "to@example.com"
> echo ""
> ) | GIT_SEND_EMAIL_NOTTY=1 git send-email \
> --smtp-server="$(pwd)/fake.sendmail" \
> $patches \
> 2>errors &&
> + grep "^From: A U Thor <author@example.com>\$" msgtxt1 &&
> + grep "^To: to@example.com\$" msgtxt1
> +'
The indentation seems strange here --- are the new "grep" lines
continuations of the git send-email line?
It's probably easier to change the structure completely:
clean_fake_sendmail &&
echo to@examples.com >prompt.input &&
echo >>prompt.input &&
GIT_SEND_EMAIL_NOTTY=1 \
git send-email --smtp-server=... $patches <prompt.input &&
grep "^From: A U Thor <authorident@example.com>\$" msgtxt1 &&
grep "^To: to@example.com\$" msgtxt1
> +test_expect_success $PREREQ,AUTOIDENT 'implicit ident prompts for sender' '
> + clean_fake_sendmail &&
> + (echo "Example <from@example.com>" &&
> + echo "to@example.com" &&
> + echo ""
> + ) |
> + (sane_unset GIT_AUTHOR_NAME &&
> + sane_unset GIT_AUTHOR_EMAIL &&
> + sane_unset GIT_COMMITTER_NAME &&
> + sane_unset GIT_COMMITTER_EMAIL &&
> + GIT_SEND_EMAIL_NOTTY=1 git send-email \
> + --smtp-server="$(pwd)/fake.sendmail" \
> + $patches \
> + 2>errors &&
> grep "^From: Example <from@example.com>\$" msgtxt1 &&
> grep "^To: to@example.com\$" msgtxt1
> + )
> +'
Likewise:
clean_fake_sendmail &&
echo "Example <from@example.com>" >prompt.in &&
echo to@example.com >>prompt.in
echo >>prompt.in &&
(
sane_unset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL &&
sane_unset GIT_COMMITTER_NAME GIT_COMMITTER_EMAIL &&
GIT_SEND_EMAIL_NOTTY=1 \
git send-email --smtp-server=... $patches <prompt.in
) &&
grep "^From: Example <from@example.com>\$" msgtxt1 &&
grep "^To: to@example.com\$" msgtxt1
> +test_expect_success $PREREQ,!AUTOIDENT 'broken implicit ident aborts send-email' '
> + clean_fake_sendmail &&
> + (sane_unset GIT_AUTHOR_NAME &&
> + sane_unset GIT_AUTHOR_EMAIL &&
> + sane_unset GIT_COMMITTER_NAME &&
> + sane_unset GIT_COMMITTER_EMAIL &&
> + GIT_SEND_EMAIL_NOTTY=1 && export GIT_SEND_EMAIL_NOTTY &&
> + test_must_fail git send-email \
> + --smtp-server="$(pwd)/fake.sendmail" \
> + $patches </dev/null 2>errors.out &&
> + test_i18ngrep "tell me who you are" errors.out
> + )
> '
Likewise:
clean_fake_sendmail &&
(
sane_unset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL &&
sane_unset GIT_COMMITTER_NAME GIT_COMMITTER_EMAIL &&
GIT_SEND_EMAIL_NOTTY=1 \
git send-email --smtp-server=... $patches </dev/null 2>err
) &&
test_i18ngrep "[Tt]ell me who you are" err
For what it's worth, with or without such changes,
Acked-by: Jonathan Nieder <jrnieder@gmail.com>
^ permalink raw reply [flat|nested] 76+ messages in thread