git-clone fails when current user is not in /etc/passwd

git-clone fails when current user is not in /etc/passwd

[Taylor Braun-Jones]

My use case it running git clone inside a docker container with
`docker run --user $(id -u):$(id -g) --volume /foo:/foo ...`. I want
all /foo/* file creation/access from inside the Docker container to be
done as the current uid/gid of the host system.

Steps to reproduce:

mkdir /tmp/docker-git
cat > /tmp/docker-git/Dockerfile <<EOF
FROM ubuntu
RUN apt-get update && apt-get install -y git-core
EOF
docker build -t git /tmp/docker-git/
docker run --user $(id -u):$(id -g) git git clone
https://github.com/git/git.git /tmp/git
# fatal: unable to look up current user in the passwd file: no such user
echo $? # 128

My current workaround is:

cat >> /tmp/docker-git/Dockerfile <<EOF
RUN git config --system user.name Docker && git config --system
user.email docker@localhost
EOF

But I don't see why this should be necessary just to clone a repo. I
run complex build jobs inside a docker container using this approach
and git-clone is the first command to fail due to the lack of a passwd
file entry for the current user. Some complain to stderr, but still
succeed.

Regards,
Taylor

Re: git-clone fails when current user is not in /etc/passwd

[Taylor Braun-Jones]

What's the feeling on this one? If there's agreement in principle that
git-clone should not fail when the current UID cannot be found in
/etc/passwd then I'm happy to submit a patch to fix it.

Thanks,
Taylor

On Wed, Dec 2, 2015 at 3:10 PM, Taylor Braun-Jones
<taylor@braun-jones.org> wrote:
> My use case it running git clone inside a docker container with
> `docker run --user $(id -u):$(id -g) --volume /foo:/foo ...`. I want
> all /foo/* file creation/access from inside the Docker container to be
> done as the current uid/gid of the host system.
>
> Steps to reproduce:
>
> mkdir /tmp/docker-git
> cat > /tmp/docker-git/Dockerfile <<EOF
> FROM ubuntu
> RUN apt-get update && apt-get install -y git-core
> EOF
> docker build -t git /tmp/docker-git/
> docker run --user $(id -u):$(id -g) git git clone
> https://github.com/git/git.git /tmp/git
> # fatal: unable to look up current user in the passwd file: no such user
> echo $? # 128
>
> My current workaround is:
>
> cat >> /tmp/docker-git/Dockerfile <<EOF
> RUN git config --system user.name Docker && git config --system
> user.email docker@localhost
> EOF
>
> But I don't see why this should be necessary just to clone a repo. I
> run complex build jobs inside a docker container using this approach
> and git-clone is the first command to fail due to the lack of a passwd
> file entry for the current user. Some complain to stderr, but still
> succeed.
>
> Regards,
> Taylor

Re: git-clone fails when current user is not in /etc/passwd

[Duy Nguyen]

On Wed, Dec 2, 2015 at 9:10 PM, Taylor Braun-Jones
<taylor@braun-jones.org> wrote:
> My use case it running git clone inside a docker container with
> `docker run --user $(id -u):$(id -g) --volume /foo:/foo ...`. I want
> all /foo/* file creation/access from inside the Docker container to be
> done as the current uid/gid of the host system.
>
> Steps to reproduce:
>
> mkdir /tmp/docker-git
> cat > /tmp/docker-git/Dockerfile <<EOF
> FROM ubuntu
> RUN apt-get update && apt-get install -y git-core
> EOF
> docker build -t git /tmp/docker-git/
> docker run --user $(id -u):$(id -g) git git clone
> https://github.com/git/git.git /tmp/git
> # fatal: unable to look up current user in the passwd file: no such user

It probably helps if you could get the stack trace to this message
(printed from function xgetpwuid_self). I haven't checked if my
personal laptop has docker to reproduce this.. In general we won't ask
passwd if the user specifies name/email in the config file. But I
still don't see why git-clone needs that info in the first place.
-- 
Duy

Re: git-clone fails when current user is not in /etc/passwd

[Duy Nguyen]

On Wed, Dec 9, 2015 at 5:08 PM, Duy Nguyen <pclouds@gmail.com> wrote:
> On Wed, Dec 2, 2015 at 9:10 PM, Taylor Braun-Jones
> <taylor@braun-jones.org> wrote:
>> My use case it running git clone inside a docker container with
>> `docker run --user $(id -u):$(id -g) --volume /foo:/foo ...`. I want
>> all /foo/* file creation/access from inside the Docker container to be
>> done as the current uid/gid of the host system.
>>
>> Steps to reproduce:
>>
>> mkdir /tmp/docker-git
>> cat > /tmp/docker-git/Dockerfile <<EOF
>> FROM ubuntu
>> RUN apt-get update && apt-get install -y git-core
>> EOF
>> docker build -t git /tmp/docker-git/
>> docker run --user $(id -u):$(id -g) git git clone
>> https://github.com/git/git.git /tmp/git
>> # fatal: unable to look up current user in the passwd file: no such user
>
> It probably helps if you could get the stack trace to this message
> (printed from function xgetpwuid_self). I haven't checked if my
> personal laptop has docker to reproduce this.. In general we won't ask
> passwd if the user specifies name/email in the config file. But I
> still don't see why git-clone needs that info in the first place.

Well.. reflog needs it. So either you disable reflog at clone time or
define name/email via config file. I don't see anything wrong with
this behavior. Stack trace

(gdb) bt
#0  0x0000000000585aaf in xgetpwuid_self () at wrapper.c:608
#1  0x00000000004ef2c4 in ident_default_name () at ident.c:108
#2  0x00000000004ef851 in fmt_ident (name=0x0, email=0x0,
date_str=0x0, flag=0) at ident.c:300
#3  0x00000000004efb25 in git_committer_info (flag=0) at ident.c:366
#4  0x000000000052fc6f in log_ref_write_1 (refname=0x16236c0
"refs/remotes/origin/HEAD", old_sha1=0x7ffc91e44190 "",
new_sha1=0x7ffc91e44170
"\355\025\033\215p{o\177V\234qX\212j?!v|}D\177", msg=0x1620e50 "clone:
from https://github.com/rswier/c4.git", logfile=0x7ffc91e44100,
flags=0, err=0x7ffc91e44150) at refs.c:3403
#5  0x000000000052fd85 in log_ref_write (refname=0x16236c0
"refs/remotes/origin/HEAD", old_sha1=0x7ffc91e44190 "",
new_sha1=0x7ffc91e44170
"\355\025\033\215p{o\177V\234qX\212j?!v|}D\177", msg=0x1620e50 "clone:
from https://github.com/rswier/c4.git", flags=0, err=0x7ffc91e44150)
at refs.c:3424
#6  0x00000000005304af in create_symref (ref_target=0x16236c0
"refs/remotes/origin/HEAD", refs_heads_master=0x1624cc0
"refs/remotes/origin/master", logmsg=0x1620e50 "clone: from
https://github.com/rswier/c4.git") at refs.c:3592
#7  0x000000000042e50c in update_remote_refs (refs=0x1622d40,
mapped_refs=0x1622cc0, remote_head_points_at=0x1624bc0,
branch_top=0x1621ca0 "refs/remotes/origin/", msg=0x1620e50 "clone:
from https://github.com/rswier/c4.git", transport=0x1622960,
check_connectivity=1) at builtin/clone.c:639
#8  0x000000000042f80c in cmd_clone (argc=2, argv=0x7ffc91e44ab8,
prefix=0x0) at builtin/clone.c:1066
#9  0x0000000000405fda in run_builtin (p=0x825948 <commands+456>,
argc=5, argv=0x7ffc91e44ab8) at git.c:352
#10 0x00000000004061d0 in handle_builtin (argc=5, argv=0x7ffc91e44ab8)
at git.c:542
#11 0x00000000004062ec in run_argv (argcp=0x7ffc91e4498c,
argv=0x7ffc91e449a0) at git.c:588
#12 0x00000000004064df in main (argc=5, av=0x7ffc91e44aa8) at git.c:695
-- 
Duy

Re: git-clone fails when current user is not in /etc/passwd

[Taylor Braun-Jones]

On Wed, Dec 9, 2015 at 1:24 PM, Duy Nguyen <pclouds@gmail.com> wrote:
> On Wed, Dec 9, 2015 at 5:08 PM, Duy Nguyen <pclouds@gmail.com> wrote:
>> On Wed, Dec 2, 2015 at 9:10 PM, Taylor Braun-Jones
>> <taylor@braun-jones.org> wrote:
>>> My use case it running git clone inside a docker container with
>>> `docker run --user $(id -u):$(id -g) --volume /foo:/foo ...`. I want
>>> all /foo/* file creation/access from inside the Docker container to be
>>> done as the current uid/gid of the host system.
>>>
>>> Steps to reproduce:
>>>
>>> mkdir /tmp/docker-git
>>> cat > /tmp/docker-git/Dockerfile <<EOF
>>> FROM ubuntu
>>> RUN apt-get update && apt-get install -y git-core
>>> EOF
>>> docker build -t git /tmp/docker-git/
>>> docker run --user $(id -u):$(id -g) git git clone
>>> https://github.com/git/git.git /tmp/git
>>> # fatal: unable to look up current user in the passwd file: no such user
>>
>> It probably helps if you could get the stack trace to this message
>> (printed from function xgetpwuid_self). I haven't checked if my
>> personal laptop has docker to reproduce this.. In general we won't ask
>> passwd if the user specifies name/email in the config file. But I
>> still don't see why git-clone needs that info in the first place.
>
> Well.. reflog needs it. So either you disable reflog at clone time or
> define name/email via config file. I don't see anything wrong with
> this behavior.

Can't git just use "unknown" and "unknown@localhost" if the name or
email can not be determined from xgetpwuid? This seems to be the way
that Mercurial behaves.

Re: git-clone fails when current user is not in /etc/passwd

[Taylor Braun-Jones]

[-- Attachment #1: Type: text/plain, Size: 1998 bytes --]

On Wed, Dec 9, 2015 at 5:35 PM, Taylor Braun-Jones
<taylor@braun-jones.org> wrote:
>
> On Wed, Dec 9, 2015 at 1:24 PM, Duy Nguyen <pclouds@gmail.com> wrote:
> > On Wed, Dec 9, 2015 at 5:08 PM, Duy Nguyen <pclouds@gmail.com> wrote:
> >> On Wed, Dec 2, 2015 at 9:10 PM, Taylor Braun-Jones
> >> <taylor@braun-jones.org> wrote:
> >>> My use case it running git clone inside a docker container with
> >>> `docker run --user $(id -u):$(id -g) --volume /foo:/foo ...`. I want
> >>> all /foo/* file creation/access from inside the Docker container to be
> >>> done as the current uid/gid of the host system.
> >>>
> >>> Steps to reproduce:
> >>>
> >>> mkdir /tmp/docker-git
> >>> cat > /tmp/docker-git/Dockerfile <<EOF
> >>> FROM ubuntu
> >>> RUN apt-get update && apt-get install -y git-core
> >>> EOF
> >>> docker build -t git /tmp/docker-git/
> >>> docker run --user $(id -u):$(id -g) git git clone
> >>> https://github.com/git/git.git /tmp/git
> >>> # fatal: unable to look up current user in the passwd file: no such user
> >>
> >> It probably helps if you could get the stack trace to this message
> >> (printed from function xgetpwuid_self). I haven't checked if my
> >> personal laptop has docker to reproduce this.. In general we won't ask
> >> passwd if the user specifies name/email in the config file. But I
> >> still don't see why git-clone needs that info in the first place.
> >
> > Well.. reflog needs it. So either you disable reflog at clone time or
> > define name/email via config file. I don't see anything wrong with
> > this behavior.
>
> Can't git just use "unknown" and "unknown@localhost" if the name or
> email can not be determined from xgetpwuid? This seems to be the way
> that Mercurial behaves.

See attached patch for an implementation of the fallback-to-unknown
idea. With this, I'm able to clone a repository under a UID that does
not exist in /etc/passwd and do everything that I'd expect to do with
a local git repository. Is there something that I've missed?

Taylor

[-- Attachment #2: 0001-Warn-not-die-when-unable-to-look-up-current-user-in-.patch --]
[-- Type: text/x-patch, Size: 1296 bytes --]

From dd5c2ccf5df4104e677f8488fbb4be98ff1831c7 Mon Sep 17 00:00:00 2001
From: Taylor Braun-Jones <taylor@braun-jones.org>
Date: Thu, 10 Dec 2015 13:21:25 -0500
Subject: [PATCH] Warn (not die) when unable to look up current user in the
 passwd file

---
 wrapper.c | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/wrapper.c b/wrapper.c
index 6fcaa4d..8119068 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -604,12 +604,26 @@ int access_or_die(const char *path, int mode, unsigned flag)
 struct passwd *xgetpwuid_self(void)
 {
 	struct passwd *pw;
+	static struct passwd pw_unknown = {0};
 
+	uid_t uid = getuid();
 	errno = 0;
-	pw = getpwuid(getuid());
-	if (!pw)
-		die(_("unable to look up current user in the passwd file: %s"),
-		    errno ? strerror(errno) : _("no such user"));
+	pw = getpwuid(uid);
+	if (!pw) {
+		warning(_("unable to look up current user in the passwd file: %s"),
+		        errno ? strerror(errno) : _("no such user"));
+		pw_unknown.pw_name = "unknown";
+		pw_unknown.pw_passwd = NULL;
+		pw_unknown.pw_uid = uid;
+		pw_unknown.pw_gid = getgid();
+		pw_unknown.pw_dir = NULL;
+		pw_unknown.pw_shell = NULL;
+ #ifndef NO_GECOS_IN_PWENT
+		pw_unknown.pw_gecos = NULL;
+ #endif
+		pw = &pw_unknown;
+	}
+
 	return pw;
 }
 
-- 
2.6.3

Re: git-clone fails when current user is not in /etc/passwd

[Jeff King]

On Wed, Dec 09, 2015 at 05:35:52PM -0500, Taylor Braun-Jones wrote:

> > Well.. reflog needs it. So either you disable reflog at clone time or
> > define name/email via config file. I don't see anything wrong with
> > this behavior.
> 
> Can't git just use "unknown" and "unknown@localhost" if the name or
> email can not be determined from xgetpwuid? This seems to be the way
> that Mercurial behaves.

Yeah, I think there is a mismatch here in the ident code. When we are
making a commit (which is likely to get published and is set in stone),
we ask for "IDENT_STRICT", which will barf if we can't come up with
something sensible. For reflogs, we are OK putting whatever
auto-generated crap in there we can come up with.

But before we even hit the strict-check, we call xgetpwuid_self(), which
unconditionally dies on failure. I think that function needs to be
taught a "gently" form which we use for non-strict ident lookups.
Unfortunately it's a little non-trivial because the strictness will need
to get passed all the way down to ident_default_name() (and we need to
make sure that a non-strict check followed by a strict one does not
fail; i.e., that the first does not pollute the contents of
git_default_name).

-Peff

Re: git-clone fails when current user is not in /etc/passwd

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

> Well.. reflog needs it. So either you disable reflog at clone time or
> define name/email via config file. I don't see anything wrong with
> this behavior.

Hmm, I am not quite sure about that.

In the codepath that computes ident_default_email(), which is one
half of what the "reflog" code you cite wants to do, copy_email()
calls copy_email() which in turn calls add_domainname().  If your
getpwuid() gave you some username, but your gethostname() gave you a
NULL, we do not barf but add "(none)" as and then issue a warning.

Perhaps we can do the same by returning a structure with members set
to a set of fake values.  Because we already have checks in places
that really matter to the recorded history (read: not reflog) in the
form of *_ident_sufficiently_given() functions, potential damage due
to having phoney names returned from here would not be too bad.

Totally untested...

 ident.c   | 13 ++++++++++---
 wrapper.c |  4 ----
 2 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/ident.c b/ident.c
index 4e7f99d..2ccae2c 100644
--- a/ident.c
+++ b/ident.c
@@ -31,7 +31,7 @@ static void copy_gecos(const struct passwd *w, struct strbuf *name)
 	 * with commas.  Also & stands for capitalized form of the login name.
 	 */
 
-	for (src = get_gecos(w); *src && *src != ','; src++) {
+	for (src = w ? get_gecos(w) : "&"; *src && *src != ','; src++) {
 		int ch = *src;
 		if (ch != '&')
 			strbuf_addch(name, ch);
@@ -117,7 +117,7 @@ static void copy_email(const struct passwd *pw, struct strbuf *email)
 	 * Make up a fake email address
 	 * (name + '@' + hostname [+ '.' + domainname])
 	 */
-	strbuf_addstr(email, pw->pw_name);
+	strbuf_addstr(email, pw ? pw->pw_name : "unknown");
 	strbuf_addch(email, '@');
 
 	if (!add_mailname_host(email))
@@ -332,8 +332,15 @@ const char *fmt_ident(const char *name, const char *email,
 				fputs(env_hint, stderr);
 			die("empty ident name (for <%s>) not allowed", email);
 		}
+		errno = 0;
 		pw = xgetpwuid_self();
-		name = pw->pw_name;
+		if (!pw) {
+			warning(_("unable to look up current user: %s"),
+				errno ? strerror(errno) : _("no such user"));
+			name = "unknown";
+		} else {
+			name = pw->pw_name;
+		}
 	}
 
 	if (strict && email == git_default_email.buf &&
diff --git a/wrapper.c b/wrapper.c
index 6fcaa4d..16ab45f 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -605,11 +605,7 @@ struct passwd *xgetpwuid_self(void)
 {
 	struct passwd *pw;
 
-	errno = 0;
 	pw = getpwuid(getuid());
-	if (!pw)
-		die(_("unable to look up current user in the passwd file: %s"),
-		    errno ? strerror(errno) : _("no such user"));
 	return pw;
 }
 

Re: git-clone fails when current user is not in /etc/passwd

[Taylor Braun-Jones]

On Thu, Dec 10, 2015 at 1:43 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Duy Nguyen <pclouds@gmail.com> writes:
>
>> Well.. reflog needs it. So either you disable reflog at clone time or
>> define name/email via config file. I don't see anything wrong with
>> this behavior.
>
> Hmm, I am not quite sure about that.
>
> In the codepath that computes ident_default_email(), which is one
> half of what the "reflog" code you cite wants to do, copy_email()
> calls copy_email() which in turn calls add_domainname().  If your
> getpwuid() gave you some username, but your gethostname() gave you a
> NULL, we do not barf but add "(none)" as and then issue a warning.
>
> Perhaps we can do the same by returning a structure with members set
> to a set of fake values.  Because we already have checks in places
> that really matter to the recorded history (read: not reflog) in the
> form of *_ident_sufficiently_given() functions, potential damage due
> to having phoney names returned from here would not be too bad.
>
> Totally untested...
>
>  ident.c   | 13 ++++++++++---
>  wrapper.c |  4 ----
>  2 files changed, 10 insertions(+), 7 deletions(-)
>
> diff --git a/ident.c b/ident.c
> index 4e7f99d..2ccae2c 100644
> --- a/ident.c
> +++ b/ident.c
> @@ -31,7 +31,7 @@ static void copy_gecos(const struct passwd *w, struct strbuf *name)
>          * with commas.  Also & stands for capitalized form of the login name.
>          */
>
> -       for (src = get_gecos(w); *src && *src != ','; src++) {
> +       for (src = w ? get_gecos(w) : "&"; *src && *src != ','; src++) {
>                 int ch = *src;
>                 if (ch != '&')
>                         strbuf_addch(name, ch);
> @@ -117,7 +117,7 @@ static void copy_email(const struct passwd *pw, struct strbuf *email)
>          * Make up a fake email address
>          * (name + '@' + hostname [+ '.' + domainname])
>          */
> -       strbuf_addstr(email, pw->pw_name);
> +       strbuf_addstr(email, pw ? pw->pw_name : "unknown");
>         strbuf_addch(email, '@');
>
>         if (!add_mailname_host(email))
> @@ -332,8 +332,15 @@ const char *fmt_ident(const char *name, const char *email,
>                                 fputs(env_hint, stderr);
>                         die("empty ident name (for <%s>) not allowed", email);
>                 }
> +               errno = 0;
>                 pw = xgetpwuid_self();
> -               name = pw->pw_name;
> +               if (!pw) {
> +                       warning(_("unable to look up current user: %s"),
> +                               errno ? strerror(errno) : _("no such user"));
> +                       name = "unknown";
> +               } else {
> +                       name = pw->pw_name;
> +               }
>         }
>
>         if (strict && email == git_default_email.buf &&
> diff --git a/wrapper.c b/wrapper.c
> index 6fcaa4d..16ab45f 100644
> --- a/wrapper.c
> +++ b/wrapper.c
> @@ -605,11 +605,7 @@ struct passwd *xgetpwuid_self(void)
>  {
>         struct passwd *pw;
>
> -       errno = 0;
>         pw = getpwuid(getuid());
> -       if (!pw)
> -               die(_("unable to look up current user in the passwd file: %s"),
> -                   errno ? strerror(errno) : _("no such user"));
>         return pw;
>  }
>

Yes, this looks better.

+1

Re: git-clone fails when current user is not in /etc/passwd

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> But before we even hit the strict-check, we call xgetpwuid_self(), which
> unconditionally dies on failure. I think that function needs to be
> taught a "gently" form which we use for non-strict ident lookups.
> Unfortunately it's a little non-trivial because the strictness will need
> to get passed all the way down to ident_default_name() (and we need to
> make sure that a non-strict check followed by a strict one does not
> fail; i.e., that the first does not pollute the contents of
> git_default_name).

All true.  The adding of "(none)" in add_domainname() I used as an
excuse to make the function stop barfing is a symptom coming from
the above.  That one should die when asked to do a strict thing
(there is a corresponding kludge to do strstr "(none)" to cover it
up, which is even uglier X-<).

Re: git-clone fails when current user is not in /etc/passwd

[Jeff King]

On Thu, Dec 10, 2015 at 11:57:41AM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > But before we even hit the strict-check, we call xgetpwuid_self(), which
> > unconditionally dies on failure. I think that function needs to be
> > taught a "gently" form which we use for non-strict ident lookups.
> > Unfortunately it's a little non-trivial because the strictness will need
> > to get passed all the way down to ident_default_name() (and we need to
> > make sure that a non-strict check followed by a strict one does not
> > fail; i.e., that the first does not pollute the contents of
> > git_default_name).
> 
> All true.  The adding of "(none)" in add_domainname() I used as an
> excuse to make the function stop barfing is a symptom coming from
> the above.  That one should die when asked to do a strict thing
> (there is a corresponding kludge to do strstr "(none)" to cover it
> up, which is even uglier X-<).

Yes, I have always hated that. It seems like we should just be able to
carry a "strict" flag (both from xgetpwuid() and from the "(none)" hack)
along with ident_default_*, and barf at the right time if it is not set.

-Peff

[PATCH 0/3] git-clone fails when current user is not in /etc/passwd

[Jeff King]

On Thu, Dec 10, 2015 at 03:40:35PM -0500, Jeff King wrote:

> > All true.  The adding of "(none)" in add_domainname() I used as an
> > excuse to make the function stop barfing is a symptom coming from
> > the above.  That one should die when asked to do a strict thing
> > (there is a corresponding kludge to do strstr "(none)" to cover it
> > up, which is even uglier X-<).
> 
> Yes, I have always hated that. It seems like we should just be able to
> carry a "strict" flag (both from xgetpwuid() and from the "(none)" hack)
> along with ident_default_*, and barf at the right time if it is not set.

I don't think we want to pass down a "be strict" flag to the low-level
code filling in default_{name,email}. We might be strict in one call,
and non-strict in another within the same program. Worse, we actually
call ident_default_name() even when we don't actually want the name (we
could fix that, too, but it just adds more cases to the code).

So here's my solution, which instead carries the "is it bogus" flag
along with the default strings.

  [1/3]: ident: make xgetpwuid_self() a static local helper
  [2/3]: ident: keep a flag for bogus default_email
  [3/3]: ident: loosen getpwuid error in non-strict mode

-Peff

[PATCH 1/3] ident: make xgetpwuid_self() a static local helper

[Jeff King]

This function is defined in wrapper.c, but nobody besides
ident.c uses it. And nobody is likely to in the future,
either, as anything that cares about the user's name should
be going through the ident code.

Moving it here is a cleanup of the global namespace, but it
will also enable further cleanups inside ident.c.

Signed-off-by: Jeff King <peff@peff.net>
---
 git-compat-util.h |  3 ---
 ident.c           | 12 ++++++++++++
 wrapper.c         | 12 ------------
 3 files changed, 12 insertions(+), 15 deletions(-)

diff --git a/git-compat-util.h b/git-compat-util.h
index 8e39867..2da0a75 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -929,9 +929,6 @@ int access_or_die(const char *path, int mode, unsigned flag);
 /* Warn on an inaccessible file that ought to be accessible */
 void warn_on_inaccessible(const char *path);
 
-/* Get the passwd entry for the UID of the current process. */
-struct passwd *xgetpwuid_self(void);
-
 #ifdef GMTIME_UNRELIABLE_ERRORS
 struct tm *git_gmtime(const time_t *);
 struct tm *git_gmtime_r(const time_t *, struct tm *);
diff --git a/ident.c b/ident.c
index 5ff1aad..d7c70e2 100644
--- a/ident.c
+++ b/ident.c
@@ -23,6 +23,18 @@ static int author_ident_explicitly_given;
 #define get_gecos(struct_passwd) ((struct_passwd)->pw_gecos)
 #endif
 
+static struct passwd *xgetpwuid_self(void)
+{
+	struct passwd *pw;
+
+	errno = 0;
+	pw = getpwuid(getuid());
+	if (!pw)
+		die(_("unable to look up current user in the passwd file: %s"),
+		    errno ? strerror(errno) : _("no such user"));
+	return pw;
+}
+
 static void copy_gecos(const struct passwd *w, struct strbuf *name)
 {
 	char *src;
diff --git a/wrapper.c b/wrapper.c
index 6fcaa4d..c95e290 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -601,18 +601,6 @@ int access_or_die(const char *path, int mode, unsigned flag)
 	return ret;
 }
 
-struct passwd *xgetpwuid_self(void)
-{
-	struct passwd *pw;
-
-	errno = 0;
-	pw = getpwuid(getuid());
-	if (!pw)
-		die(_("unable to look up current user in the passwd file: %s"),
-		    errno ? strerror(errno) : _("no such user"));
-	return pw;
-}
-
 char *xgetcwd(void)
 {
 	struct strbuf sb = STRBUF_INIT;
-- 
2.6.3.636.g1460207

[PATCH 2/3] ident: keep a flag for bogus default_email

[Jeff King]

If we have to deduce the user's email address and can't come
up with something plausible for the hostname, we simply
write "(none)" or ".(none)" in the hostname.

Later, our strict-check is forced to use strstr to look for
this magic string. This is probably not a problem in
practice, but it's rather ugly. Let's keep an extra flag
that tells us the email is bogus, and check that instead.

We could get away with simply setting the global in
add_domainname(); it only gets called to write into
git_default_email. However, let's make the code a little
more obvious to future readers by actually passing a pointer
to our "bogus" flag down the call-chain.

Signed-off-by: Jeff King <peff@peff.net>
---
I waffled on the "passing it around" thing. The patch is a lot smaller
if we just set the global directly, and I doubt these static helpers
will ever get reused for anything else. But the pattern does get reused
in the next patch, which sets the "email" and "name" flags separately
depending on context.

I was also tempted to have a single "default_ident_is_flaky", but that
does get some corner cases wrong (e.g., you have a bogus auto-guessed
name, an OK auto-guessed email, and set GIT_COMMITTER_NAME but not
GIT_COMMITTER_EMAIL). I doubt those come up in practice, but it's not
that hard to do the right thing.

 ident.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/ident.c b/ident.c
index d7c70e2..085cfbe 100644
--- a/ident.c
+++ b/ident.c
@@ -10,6 +10,7 @@
 static struct strbuf git_default_name = STRBUF_INIT;
 static struct strbuf git_default_email = STRBUF_INIT;
 static struct strbuf git_default_date = STRBUF_INIT;
+static int default_email_is_bogus;
 
 #define IDENT_NAME_GIVEN 01
 #define IDENT_MAIL_GIVEN 02
@@ -82,7 +83,7 @@ static int add_mailname_host(struct strbuf *buf)
 	return 0;
 }
 
-static void add_domainname(struct strbuf *out)
+static void add_domainname(struct strbuf *out, int *is_bogus)
 {
 	char buf[1024];
 	struct hostent *he;
@@ -90,17 +91,21 @@ static void add_domainname(struct strbuf *out)
 	if (gethostname(buf, sizeof(buf))) {
 		warning("cannot get host name: %s", strerror(errno));
 		strbuf_addstr(out, "(none)");
+		*is_bogus = 1;
 		return;
 	}
 	if (strchr(buf, '.'))
 		strbuf_addstr(out, buf);
 	else if ((he = gethostbyname(buf)) && strchr(he->h_name, '.'))
 		strbuf_addstr(out, he->h_name);
-	else
+	else {
 		strbuf_addf(out, "%s.(none)", buf);
+		*is_bogus = 1;
+	}
 }
 
-static void copy_email(const struct passwd *pw, struct strbuf *email)
+static void copy_email(const struct passwd *pw, struct strbuf *email,
+		       int *is_bogus)
 {
 	/*
 	 * Make up a fake email address
@@ -111,7 +116,7 @@ static void copy_email(const struct passwd *pw, struct strbuf *email)
 
 	if (!add_mailname_host(email))
 		return;	/* read from "/etc/mailname" (Debian) */
-	add_domainname(email);
+	add_domainname(email, is_bogus);
 }
 
 const char *ident_default_name(void)
@@ -133,7 +138,8 @@ const char *ident_default_email(void)
 			committer_ident_explicitly_given |= IDENT_MAIL_GIVEN;
 			author_ident_explicitly_given |= IDENT_MAIL_GIVEN;
 		} else
-			copy_email(xgetpwuid_self(), &git_default_email);
+			copy_email(xgetpwuid_self(), &git_default_email,
+				   &default_email_is_bogus);
 		strbuf_trim(&git_default_email);
 	}
 	return git_default_email.buf;
@@ -325,8 +331,7 @@ const char *fmt_ident(const char *name, const char *email,
 		name = pw->pw_name;
 	}
 
-	if (strict && email == git_default_email.buf &&
-	    strstr(email, "(none)")) {
+	if (strict && email == git_default_email.buf && default_email_is_bogus) {
 		fputs(env_hint, stderr);
 		die("unable to auto-detect email address (got '%s')", email);
 	}
-- 
2.6.3.636.g1460207

[PATCH 3/3] ident: loosen getpwuid error in non-strict mode

[Jeff King]

If the user has not specified an identity and we have to
turn to getpwuid() to find the username or gecos field, we
die immediately when getpwuid fails (e.g., because the user
does not exist). This is OK for making a commit, where we
have set IDENT_STRICT and would want to bail on bogus input.

But for something like a reflog, where the ident is "best
effort", it can be pain. For instance, even running "git
clone" with a UID that is not in /etc/passwd will result in
git barfing, just because we can't find an ident to put in
the reflog.

Instead of dying in xgetpwuid_self, we can instead return a
fallback value, and set a "bogus" flag. For the username in
an email, we already have a "default_email_is_bogus" flag.
For the name field, we introduce (and check) a matching
"default_name_is_bogus" flag. As a bonus, this means you now
get the usual "tell me who you are" advice instead of just a
"no such user" error.

No tests, as this is dependent on configuration outside of
git's control. However, I did confirm that it behaves
sensibly when I delete myself from the local /etc/passwd
(reflogs get written, and commits complain).

Signed-off-by: Jeff King <peff@peff.net>
---
I dislike #ifdefs in the middle of functions like the one below, but I
couldn't come up with an elegant way to avoid it. We're reusing "struct
passwd" for our fallback, so we need to know whether we have that field
to set or not.

An obvious alternative would be to always return our own

  struct git_passwd_struct {
	const char *name;
	const char *gecos;
  };

and translate the existing getpwuid() on the fly. That still needs an
#ifdef, but we could then get rid of the #ifdef for the get_gecos()
macro.

Yet another alternative is to simply zero the fallback passwd struct,
and pw->pw_gecos will either not exist, or will be NULL. But then the
callers have to do something sane with the NULL, and the whole point of
this exercise was to avoid callers having to deal with the error cases
directly, so...

 ident.c | 30 ++++++++++++++++++++++--------
 1 file changed, 22 insertions(+), 8 deletions(-)

diff --git a/ident.c b/ident.c
index 085cfbe..2d5b876 100644
--- a/ident.c
+++ b/ident.c
@@ -11,6 +11,7 @@ static struct strbuf git_default_name = STRBUF_INIT;
 static struct strbuf git_default_email = STRBUF_INIT;
 static struct strbuf git_default_date = STRBUF_INIT;
 static int default_email_is_bogus;
+static int default_name_is_bogus;
 
 #define IDENT_NAME_GIVEN 01
 #define IDENT_MAIL_GIVEN 02
@@ -24,15 +25,22 @@ static int author_ident_explicitly_given;
 #define get_gecos(struct_passwd) ((struct_passwd)->pw_gecos)
 #endif
 
-static struct passwd *xgetpwuid_self(void)
+static struct passwd *xgetpwuid_self(int *is_bogus)
 {
 	struct passwd *pw;
 
 	errno = 0;
 	pw = getpwuid(getuid());
-	if (!pw)
-		die(_("unable to look up current user in the passwd file: %s"),
-		    errno ? strerror(errno) : _("no such user"));
+	if (!pw) {
+		struct passwd fallback;
+		fallback.pw_name = "unknown";
+#ifndef NO_GECOS_IN_PWENT
+		fallback.pw_gecos = "Unknown";
+#endif
+		pw = &fallback;
+		if (is_bogus)
+			*is_bogus = 1;
+	}
 	return pw;
 }
 
@@ -122,7 +130,7 @@ static void copy_email(const struct passwd *pw, struct strbuf *email,
 const char *ident_default_name(void)
 {
 	if (!git_default_name.len) {
-		copy_gecos(xgetpwuid_self(), &git_default_name);
+		copy_gecos(xgetpwuid_self(&default_name_is_bogus), &git_default_name);
 		strbuf_trim(&git_default_name);
 	}
 	return git_default_name.buf;
@@ -138,8 +146,8 @@ const char *ident_default_email(void)
 			committer_ident_explicitly_given |= IDENT_MAIL_GIVEN;
 			author_ident_explicitly_given |= IDENT_MAIL_GIVEN;
 		} else
-			copy_email(xgetpwuid_self(), &git_default_email,
-				   &default_email_is_bogus);
+			copy_email(xgetpwuid_self(&default_email_is_bogus),
+				   &git_default_email, &default_email_is_bogus);
 		strbuf_trim(&git_default_email);
 	}
 	return git_default_email.buf;
@@ -327,10 +335,16 @@ const char *fmt_ident(const char *name, const char *email,
 				fputs(env_hint, stderr);
 			die("empty ident name (for <%s>) not allowed", email);
 		}
-		pw = xgetpwuid_self();
+		pw = xgetpwuid_self(NULL);
 		name = pw->pw_name;
 	}
 
+	if (want_name && strict &&
+	    name == git_default_name.buf && default_name_is_bogus) {
+		fputs(env_hint, stderr);
+		die("unable to auto-detect name (got '%s')", name);
+	}
+
 	if (strict && email == git_default_email.buf && default_email_is_bogus) {
 		fputs(env_hint, stderr);
 		die("unable to auto-detect email address (got '%s')", email);
-- 
2.6.3.636.g1460207

Re: [PATCH 2/3] ident: keep a flag for bogus default_email

[Jeff King]

On Thu, Dec 10, 2015 at 04:35:35PM -0500, Jeff King wrote:

> @@ -82,7 +83,7 @@ static int add_mailname_host(struct strbuf *buf)
>  	return 0;
>  }
>  
> -static void add_domainname(struct strbuf *out)
> +static void add_domainname(struct strbuf *out, int *is_bogus)
>  {
>  	char buf[1024];
>  	struct hostent *he;
> @@ -90,17 +91,21 @@ static void add_domainname(struct strbuf *out)
>  	if (gethostname(buf, sizeof(buf))) {
>  		warning("cannot get host name: %s", strerror(errno));
>  		strbuf_addstr(out, "(none)");
> +		*is_bogus = 1;
>  		return;
>  	}
>  	if (strchr(buf, '.'))
>  		strbuf_addstr(out, buf);
>  	else if ((he = gethostbyname(buf)) && strchr(he->h_name, '.'))
>  		strbuf_addstr(out, he->h_name);
> -	else
> +	else {
>  		strbuf_addf(out, "%s.(none)", buf);
> +		*is_bogus = 1;
> +	}

This hunk has some minor conflicts with ep/ident-with-getaddrinfo, which
graduated to master in the tips you just pushed out.

Here's a rebased version on top of the current master (the other patches
can remain the same).

-- >8 --
Subject: [PATCH] ident: keep a flag for bogus default_email

If we have to deduce the user's email address and can't come
up with something plausible for the hostname, we simply
write "(none)" or ".(none)" in the hostname.

Later, our strict-check is forced to use strstr to look for
this magic string. This is probably not a problem in
practice, but it's rather ugly. Let's keep an extra flag
that tells us the email is bogus, and check that instead.

We could get away with simply setting the global in
add_domainname(); it only gets called to write into
git_default_email. However, let's make the code a little
more obvious to future readers by actually passing a pointer
to our "bogus" flag down the call-chain.

Signed-off-by: Jeff King <peff@peff.net>
---
 ident.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/ident.c b/ident.c
index 140dd63..fde3244 100644
--- a/ident.c
+++ b/ident.c
@@ -10,6 +10,7 @@
 static struct strbuf git_default_name = STRBUF_INIT;
 static struct strbuf git_default_email = STRBUF_INIT;
 static struct strbuf git_default_date = STRBUF_INIT;
+static int default_email_is_bogus;
 
 #define IDENT_NAME_GIVEN 01
 #define IDENT_MAIL_GIVEN 02
@@ -108,22 +109,26 @@ static int canonical_name(const char *host, struct strbuf *out)
 	return status;
 }
 
-static void add_domainname(struct strbuf *out)
+static void add_domainname(struct strbuf *out, int *is_bogus)
 {
 	char buf[1024];
 
 	if (gethostname(buf, sizeof(buf))) {
 		warning("cannot get host name: %s", strerror(errno));
 		strbuf_addstr(out, "(none)");
+		*is_bogus = 1;
 		return;
 	}
 	if (strchr(buf, '.'))
 		strbuf_addstr(out, buf);
-	else if (canonical_name(buf, out) < 0)
+	else if (canonical_name(buf, out) < 0) {
 		strbuf_addf(out, "%s.(none)", buf);
+		*is_bogus = 1;
+	}
 }
 
-static void copy_email(const struct passwd *pw, struct strbuf *email)
+static void copy_email(const struct passwd *pw, struct strbuf *email,
+		       int *is_bogus)
 {
 	/*
 	 * Make up a fake email address
@@ -134,7 +139,7 @@ static void copy_email(const struct passwd *pw, struct strbuf *email)
 
 	if (!add_mailname_host(email))
 		return;	/* read from "/etc/mailname" (Debian) */
-	add_domainname(email);
+	add_domainname(email, is_bogus);
 }
 
 const char *ident_default_name(void)
@@ -156,7 +161,8 @@ const char *ident_default_email(void)
 			committer_ident_explicitly_given |= IDENT_MAIL_GIVEN;
 			author_ident_explicitly_given |= IDENT_MAIL_GIVEN;
 		} else
-			copy_email(xgetpwuid_self(), &git_default_email);
+			copy_email(xgetpwuid_self(), &git_default_email,
+				   &default_email_is_bogus);
 		strbuf_trim(&git_default_email);
 	}
 	return git_default_email.buf;
@@ -348,8 +354,7 @@ const char *fmt_ident(const char *name, const char *email,
 		name = pw->pw_name;
 	}
 
-	if (strict && email == git_default_email.buf &&
-	    strstr(email, "(none)")) {
+	if (strict && email == git_default_email.buf && default_email_is_bogus) {
 		fputs(env_hint, stderr);
 		die("unable to auto-detect email address (got '%s')", email);
 	}
-- 
2.6.3.636.g1460207

Re: [PATCH 1/3] ident: make xgetpwuid_self() a static local helper

[Junio C Hamano]

Makes tons of sense (even without patches 2 and 3).  Thanks.

Re: [PATCH 0/3] git-clone fails when current user is not in /etc/passwd

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I don't think we want to pass down a "be strict" flag to the low-level
> code filling in default_{name,email}. We might be strict in one call,
> and non-strict in another within the same program. Worse, we actually
> call ident_default_name() even when we don't actually want the name (we
> could fix that, too, but it just adds more cases to the code).
>
> So here's my solution, which instead carries the "is it bogus" flag
> along with the default strings.
>
>   [1/3]: ident: make xgetpwuid_self() a static local helper
>   [2/3]: ident: keep a flag for bogus default_email
>   [3/3]: ident: loosen getpwuid error in non-strict mode

Thanks.  Looks sensible.

Re: [PATCH 0/3] git-clone fails when current user is not in /etc/passwd

[Taylor Braun-Jones]

On Thu, Dec 10, 2015 at 4:32 PM, Jeff King <peff@peff.net> wrote:

> So here's my solution, which instead carries the "is it bogus" flag
> along with the default strings.
>
>   [1/3]: ident: make xgetpwuid_self() a static local helper
>   [2/3]: ident: keep a flag for bogus default_email
>   [3/3]: ident: loosen getpwuid error in non-strict mode

1 definitely makes sense to me. As a newcomer to the git code base, I
was wondering why it wasn't this way already.

2 and 3 solve my original problem - thanks!

Taylor

Re: [PATCH 3/3] ident: loosen getpwuid error in non-strict mode

[Jeff King]

On Thu, Dec 10, 2015 at 04:41:29PM -0500, Jeff King wrote:

> -static struct passwd *xgetpwuid_self(void)
> +static struct passwd *xgetpwuid_self(int *is_bogus)
>  {
>  	struct passwd *pw;
>  
>  	errno = 0;
>  	pw = getpwuid(getuid());
> -	if (!pw)
> -		die(_("unable to look up current user in the passwd file: %s"),
> -		    errno ? strerror(errno) : _("no such user"));
> +	if (!pw) {
> +		struct passwd fallback;
> +		fallback.pw_name = "unknown";
> +#ifndef NO_GECOS_IN_PWENT
> +		fallback.pw_gecos = "Unknown";
> +#endif
> +		pw = &fallback;
> +		if (is_bogus)
> +			*is_bogus = 1;
> +	}
>  	return pw;

Ugh. The fallback struct should be static, of course, as we are
returning its address from the function.

Anybody have a brown paper bag I can borrow?

diff --git a/ident.c b/ident.c
index 74de079..831072c 100644
--- a/ident.c
+++ b/ident.c
@@ -32,7 +32,7 @@ static struct passwd *xgetpwuid_self(int *is_bogus)
 	errno = 0;
 	pw = getpwuid(getuid());
 	if (!pw) {
-		struct passwd fallback;
+		static struct passwd fallback;
 		fallback.pw_name = "unknown";
 #ifndef NO_GECOS_IN_PWENT
 		fallback.pw_gecos = "Unknown";

-Peff