Re: [PATCH 4/4] ident: do not ignore empty config name/email

[Christian Couder]

On Mon, Feb 27, 2017 at 9:42 PM, Junio C Hamano  wrote:
> Dennis Kaarsemaker  writes:
>
>> On Thu, 2017-02-23 at 23:18 -0500, Jeff King wrote:
>>> On Thu, Feb 23, 2017 at 08:11:11PM -0800, Junio C Hamano wrote:
>>>
>>> > > So I dunno. I could really go either way on it. Feel free to drop it, or
>>> > > even move it into a separate topic to be cooked longer.
>>> >
>>> > If it were 5 years ago, it would have been different, but I do not
>>> > think cooking it longer in 'next' would smoke out breakages in
>>> > obscure scripts any longer.  Git is used by too many people who have
>>> > never seen its source these days.
>>>
>>> Yeah, I have noticed that, too. I wonder if it would be interesting to
>>> cut "weeklies" or something of "master" or even "next" that people could
>>> install with a single click.
>>>
>>> Of course it's not like we have a binary installer in the first place,
>>> so I guess that's a prerequisite.
>>
>> I provide daily[*] snapshots of git's master and next tree as packages
>> for Ubuntu, Debian, Fedora and CentOS on launchpad and SuSE's
>> openbuildservice. If there's sufficient interest in this (I know of
>> only a few users), I can try to put more effort into this.
>
> That sounds handy for people who do not build from the source
> themselves.
>
> Christian, perhaps rev-news can help advertising Dennis's effort to
> recruit like-minded souls?

Yeah, I had already noticed this thread and now Jakub has mentioned it on:

https://github.com/git/git.github.io/issues/231

so yeah we will advertise it one way or another.

Re: [PATCH 4/4] ident: do not ignore empty config name/email

[Junio C Hamano]

Dennis Kaarsemaker  writes:

> On Thu, 2017-02-23 at 23:18 -0500, Jeff King wrote:
>> On Thu, Feb 23, 2017 at 08:11:11PM -0800, Junio C Hamano wrote:
>> 
>> > > So I dunno. I could really go either way on it. Feel free to drop it, or
>> > > even move it into a separate topic to be cooked longer.
>> > 
>> > If it were 5 years ago, it would have been different, but I do not
>> > think cooking it longer in 'next' would smoke out breakages in
>> > obscure scripts any longer.  Git is used by too many people who have
>> > never seen its source these days.
>> 
>> Yeah, I have noticed that, too. I wonder if it would be interesting to
>> cut "weeklies" or something of "master" or even "next" that people could
>> install with a single click.
>> 
>> Of course it's not like we have a binary installer in the first place,
>> so I guess that's a prerequisite.
>
> I provide daily[*] snapshots of git's master and next tree as packages
> for Ubuntu, Debian, Fedora and CentOS on launchpad and SuSE's
> openbuildservice. If there's sufficient interest in this (I know of
> only a few users), I can try to put more effort into this.

That sounds handy for people who do not build from the source
themselves.

Christian, perhaps rev-news can help advertising Dennis's effort to
recruit like-minded souls?

Re: [PATCH 4/4] ident: do not ignore empty config name/email

[Dennis Kaarsemaker]

On Thu, 2017-02-23 at 23:18 -0500, Jeff King wrote:
> On Thu, Feb 23, 2017 at 08:11:11PM -0800, Junio C Hamano wrote:
> 
> > > So I dunno. I could really go either way on it. Feel free to drop it, or
> > > even move it into a separate topic to be cooked longer.
> > 
> > If it were 5 years ago, it would have been different, but I do not
> > think cooking it longer in 'next' would smoke out breakages in
> > obscure scripts any longer.  Git is used by too many people who have
> > never seen its source these days.
> 
> Yeah, I have noticed that, too. I wonder if it would be interesting to
> cut "weeklies" or something of "master" or even "next" that people could
> install with a single click.
> 
> Of course it's not like we have a binary installer in the first place,
> so I guess that's a prerequisite.

I provide daily[*] snapshots of git's master and next tree as packages
for Ubuntu, Debian, Fedora and CentOS on launchpad and SuSE's
openbuildservice. If there's sufficient interest in this (I know of
only a few users), I can try to put more effort into this.

-- 
Dennis Kaarsemaker
http://www.kaarsemaker.net

[*]When the tooling isn't broken for some reason.

Re: [PATCH 4/4] ident: do not ignore empty config name/email

[Jeff King]

On Thu, Feb 23, 2017 at 08:11:11PM -0800, Junio C Hamano wrote:

> > So I dunno. I could really go either way on it. Feel free to drop it, or
> > even move it into a separate topic to be cooked longer.
> 
> If it were 5 years ago, it would have been different, but I do not
> think cooking it longer in 'next' would smoke out breakages in
> obscure scripts any longer.  Git is used by too many people who have
> never seen its source these days.

Yeah, I have noticed that, too. I wonder if it would be interesting to
cut "weeklies" or something of "master" or even "next" that people could
install with a single click.

Of course it's not like we have a binary installer in the first place,
so I guess that's a prerequisite.

-Peff

Re: [PATCH 4/4] ident: do not ignore empty config name/email

[Junio C Hamano]

Jeff King  writes:

> Keep in mind this _only_ affects Git's config variables. So a script
> feeding git via GIT_AUTHOR_NAME, etc, shouldn't change at all with this
> code.

Ah, that changes the equation somewhat ;-)

> So I dunno. I could really go either way on it. Feel free to drop it, or
> even move it into a separate topic to be cooked longer.

If it were 5 years ago, it would have been different, but I do not
think cooking it longer in 'next' would smoke out breakages in
obscure scripts any longer.  Git is used by too many people who have
never seen its source these days.

Re: [PATCH 4/4] ident: do not ignore empty config name/email

[Jeff King]

On Thu, Feb 23, 2017 at 12:58:39PM -0800, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > This one is perhaps questionable. Maybe somebody is relying on setting a
> > per-repo user.name to override a ~/.gitconfig value and enforce
> > auto-detection?
> 
> Thanks for splitting this step out.  1/4 and 2/4 are obvious
> improvements, and 3/4 is a very sensible fix.  Compared to those
> three, this one does smell questionable, because I do not quite see
> any other reasonable fallback other than the auto-detection if the
> user gives an empty ident on purpose.

The outcomes are basically:

  1. In strict mode (making a commit, etc), we'll die with "empty name
 not allowed". My thinking was that this is less confusing for the
 user.

  2. In non-strict mode, we'd use a blank name instead of trying your
 username (or dying if you don't have an /etc/passwd entry).

> Erroring out to say "don't do that" is probably not too bad, but
> perhaps we are being run by a script that is doing a best-effort
> conversion from $ANOTHER_SCM using a list of known authors that is
> incomplete, ending up feeding empty ident and allowing us to fall
> back to attribute them to the user who runs the script.  I do not
> see a point in breaking that user and having her or him update the
> script to stuff in a truly bogus "Unknown " name.

Keep in mind this _only_ affects Git's config variables. So a script
feeding git via GIT_AUTHOR_NAME, etc, shouldn't change at all with this
code. If your script is doing "git -c user.name=whatever commit", I
think you should reconsider your script. :)

So I dunno. I could really go either way on it. Feel free to drop it, or
even move it into a separate topic to be cooked longer.

-Peff

Re: [PATCH 4/4] ident: do not ignore empty config name/email

[Junio C Hamano]

Jeff King  writes:

> This one is perhaps questionable. Maybe somebody is relying on setting a
> per-repo user.name to override a ~/.gitconfig value and enforce
> auto-detection?

Thanks for splitting this step out.  1/4 and 2/4 are obvious
improvements, and 3/4 is a very sensible fix.  Compared to those
three, this one does smell questionable, because I do not quite see
any other reasonable fallback other than the auto-detection if the
user gives an empty ident on purpose.  

Erroring out to say "don't do that" is probably not too bad, but
perhaps we are being run by a script that is doing a best-effort
conversion from $ANOTHER_SCM using a list of known authors that is
incomplete, ending up feeding empty ident and allowing us to fall
back to attribute them to the user who runs the script.  I do not
see a point in breaking that user and having her or him update the
script to stuff in a truly bogus "Unknown " name.

>
>  ident.c   |  4 ++--
>  t/t7518-ident-corner-cases.sh | 11 +++
>  2 files changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/ident.c b/ident.c
> index ead09ff7f..c0364fe3a 100644
> --- a/ident.c
> +++ b/ident.c
> @@ -153,7 +153,7 @@ static void copy_email(const struct passwd *pw, struct 
> strbuf *email,
>  
>  const char *ident_default_name(void)
>  {
> - if (!git_default_name.len) {
> + if (!(ident_config_given & IDENT_NAME_GIVEN) && !git_default_name.len) {
>   copy_gecos(xgetpwuid_self(_name_is_bogus), 
> _default_name);
>   strbuf_trim(_default_name);
>   }
> @@ -162,7 +162,7 @@ const char *ident_default_name(void)
>  
>  const char *ident_default_email(void)
>  {
> - if (!git_default_email.len) {
> + if (!(ident_config_given & IDENT_MAIL_GIVEN) && !git_default_email.len) 
> {
>   const char *email = getenv("EMAIL");
>  
>   if (email && email[0]) {
> diff --git a/t/t7518-ident-corner-cases.sh b/t/t7518-ident-corner-cases.sh
> index 3d2560c3c..ef570ac62 100755
> --- a/t/t7518-ident-corner-cases.sh
> +++ b/t/t7518-ident-corner-cases.sh
> @@ -22,4 +22,15 @@ test_expect_success 'commit rejects all-crud name' '
>   git commit --allow-empty -m foo
>  '
>  
> +# We must test the actual error message here, as an unwanted
> +# auto-detection could fail for other reasons.
> +test_expect_success 'empty configured name does not auto-detect' '
> + (
> + sane_unset GIT_AUTHOR_NAME &&
> + test_must_fail \
> + git -c user.name= commit --allow-empty -m foo 2>err &&
> + test_i18ngrep "empty ident name" err
> + )
> +'
> +
>  test_done

[PATCH 4/4] ident: do not ignore empty config name/email

[Jeff King]

When we read user.name and user.email from a config file,
they go into strbufs. When a caller asks ident_default_name()
for the value, we fallback to auto-detecting if the strbuf
is empty.

That means that explicitly setting an empty string in the
config is identical to not setting it at all. This is
potentially confusing, as we usually accept a configured
value as the final value.

Signed-off-by: Jeff King 
---
This one is perhaps questionable. Maybe somebody is relying on setting a
per-repo user.name to override a ~/.gitconfig value and enforce
auto-detection?

 ident.c   |  4 ++--
 t/t7518-ident-corner-cases.sh | 11 +++
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/ident.c b/ident.c
index ead09ff7f..c0364fe3a 100644
--- a/ident.c
+++ b/ident.c
@@ -153,7 +153,7 @@ static void copy_email(const struct passwd *pw, struct 
strbuf *email,
 
 const char *ident_default_name(void)
 {
-   if (!git_default_name.len) {
+   if (!(ident_config_given & IDENT_NAME_GIVEN) && !git_default_name.len) {
copy_gecos(xgetpwuid_self(_name_is_bogus), 
_default_name);
strbuf_trim(_default_name);
}
@@ -162,7 +162,7 @@ const char *ident_default_name(void)
 
 const char *ident_default_email(void)
 {
-   if (!git_default_email.len) {
+   if (!(ident_config_given & IDENT_MAIL_GIVEN) && !git_default_email.len) 
{
const char *email = getenv("EMAIL");
 
if (email && email[0]) {
diff --git a/t/t7518-ident-corner-cases.sh b/t/t7518-ident-corner-cases.sh
index 3d2560c3c..ef570ac62 100755
--- a/t/t7518-ident-corner-cases.sh
+++ b/t/t7518-ident-corner-cases.sh
@@ -22,4 +22,15 @@ test_expect_success 'commit rejects all-crud name' '
git commit --allow-empty -m foo
 '
 
+# We must test the actual error message here, as an unwanted
+# auto-detection could fail for other reasons.
+test_expect_success 'empty configured name does not auto-detect' '
+   (
+   sane_unset GIT_AUTHOR_NAME &&
+   test_must_fail \
+   git -c user.name= commit --allow-empty -m foo 2>err &&
+   test_i18ngrep "empty ident name" err
+   )
+'
+
 test_done
-- 
2.12.0.rc2.597.g959f68882