From: Junio C Hamano <hidden> Date: 2016-06-15 22:53:50
Jeff King [off-list ref] writes:
So it seems to me like a much simpler set of rules would be:
1. When reading gecos, always fall back to the username if the gecos
field is unavailable or blank.
2. Always die when the name field is blank. That means we will die
when you pass in a bogus empty GIT_COMMITTER_NAME (or an empty
config name), which makes a lot more sense to me than falling back;
those are bogus requests, not system config problems. And we won't
ever have a blank gecos name, because we'll always fall back on the
username.
That certainly sounds very simple to explain and understand, and I do not
offhand think of anything *sane* that would break ;-)
Thanks.
From: Jeff King <hidden> Date: 2016-06-15 22:53:51
This function sets up the default name, email, and date, and
is not publicly available. Let's split it into three public
functions so that callers can get just the parts they need.
While we're at it, let's change the interface to simple
accessors. The original function was called only by fmt_ident,
and contained logic for "if we already have some other
value, don't load the default" which properly belongs in
fmt_ident.
Signed-off-by: Jeff King <redacted>
---
The patch is a pain to read because it's splitting an existing function
in 3; just reading the result is much easier.
cache.h | 3 +++
ident.c | 35 +++++++++++++++++++----------------
2 files changed, 22 insertions(+), 16 deletions(-)
@@ -117,21 +117,20 @@ static void copy_email(const struct passwd *pw)sizeof(git_default_email)-len);}-staticvoidsetup_ident(constchar**name,constchar**emailp)+constchar*ident_default_name(void){-structpasswd*pw=NULL;--/* Get the name ("gecos") */-if(!*name&&!git_default_name[0]){-pw=getpwuid(getuid());+if(!git_default_name[0]){+structpasswd*pw=getpwuid(getuid());if(!pw)die("You don't exist. Go away!");copy_gecos(pw,git_default_name,sizeof(git_default_name));}-if(!*name)-*name=git_default_name;+returngit_default_name;+}-if(!*emailp&&!git_default_email[0]){+constchar*ident_default_email(void)+{+if(!git_default_email[0]){constchar*email=getenv("EMAIL");if(email&&email[0]){
@@ -139,19 +138,20 @@ static void setup_ident(const char **name, const char **emailp)sizeof(git_default_email));user_ident_explicitly_given|=IDENT_MAIL_GIVEN;}else{-if(!pw)-pw=getpwuid(getuid());+structpasswd*pw=getpwuid(getuid());if(!pw)die("You don't exist. Go away!");copy_email(pw);}}-if(!*emailp)-*emailp=git_default_email;+returngit_default_email;+}-/* And set the default date */+constchar*ident_default_date(void)+{if(!git_default_date[0])datestamp(git_default_date,sizeof(git_default_date));+returngit_default_date;}staticintadd_raw(char*buf,size_tsize,intoffset,constchar*str)
From: Jeff King <hidden> Date: 2016-06-15 22:53:51
By calling the ident_default_email accessor, we can be sure
that the default value is actually filled-in.
Signed-off-by: Jeff King <redacted>
---
I believe this is a real, triggerable bug if you don't have your
user.email config set, but I didn't actually test it (and I have no idea
if DAV would actually care about an empty email here anyway).
http-push.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: Jeff King <hidden> Date: 2016-06-15 22:53:51
There's no reason for this to be in config, except that once
upon a time all of the config parsing was there. It makes
more sense to keep the ident code together.
Signed-off-by: Jeff King <redacted>
---
cache.h | 1 +
config.c | 24 +-----------------------
ident.c | 21 +++++++++++++++++++++
3 files changed, 23 insertions(+), 23 deletions(-)
@@ -762,28 +762,6 @@ static int git_default_core_config(const char *var, const char *value)return0;}-staticintgit_default_user_config(constchar*var,constchar*value)-{-if(!strcmp(var,"user.name")){-if(!value)-returnconfig_error_nonbool(var);-strlcpy(git_default_name,value,sizeof(git_default_name));-user_ident_explicitly_given|=IDENT_NAME_GIVEN;-return0;-}--if(!strcmp(var,"user.email")){-if(!value)-returnconfig_error_nonbool(var);-strlcpy(git_default_email,value,sizeof(git_default_email));-user_ident_explicitly_given|=IDENT_MAIL_GIVEN;-return0;-}--/* Add other config variables here and to Documentation/config.txt. */-return0;-}-staticintgit_default_i18n_config(constchar*var,constchar*value){if(!strcmp(var,"i18n.commitencoding"))
From: Jeff King <hidden> Date: 2016-06-15 22:53:51
There's no reason anybody outside of ident.c should access
these directly (they should use the new accessors which make
sure the variables are initialized), so we can make them
file-scope statics.
While we're at it, move user_ident_explicitly_given into
ident.c; while still globally visible, it makes more sense
to reside with the ident code.
Signed-off-by: Jeff King <redacted>
---
cache.h | 3 ---
environment.c | 3 ---
ident.c | 4 ++++
3 files changed, 4 insertions(+), 6 deletions(-)
From: Jeff King <hidden> Date: 2016-06-15 22:53:51
The record_person function just parses out the "name" field
of the person line in a commit and adds it to a string_list.
The only reason we need an extra buffer is that the
string_list functions require a NUL-terminated string.
Instead of the static buffer, we can just allocate a
temporary NUL-terminated copy. In addition to removing a
useless limit, this removes the only user of MAX_GITNAME
outside of ident.c.
Signed-off-by: Jeff King <redacted>
---
This actually introduces an extra malloc() per commit that we analyze. I
doubt we care, but if we do, the right solution IMHO is to introduce
string-list functions which can lookup or add a (buf, len) pair and
avoid the copy altogether.
builtin/fmt-merge-msg.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
From: Jeff King <hidden> Date: 2016-06-15 22:53:51
We try to generate a sane message id for cover letters and
threading by appending some changing bits to the front of
the user's email address. The current code parses the email
out of the results of git_committer_info, but we can do this
much more easily by just calling ident_default_email
ourselves.
Signed-off-by: Jeff King <redacted>
---
Technically this is a regression if you really wanted:
GIT_COMMITTER_EMAIL=some.addr@example.com \
git format-patch --thread=deep
to make your environment variable part of the message-ids. I don't think
it matters, but I can adjust it if we care.
builtin/log.c | 10 ++--------
1 file changed, 2 insertions(+), 8 deletions(-)
From: Jeff King <hidden> Date: 2016-06-15 22:53:51
There are no more callers who want this, so we can drop it.
Signed-off-by: Jeff King <redacted>
---
cache.h | 5 ++---
ident.c | 11 ++++-------
2 files changed, 6 insertions(+), 10 deletions(-)
From: Jeff King <hidden> Date: 2016-06-15 22:53:51
The fmt_ident function gets a flag that tells us whether to
die if the name field is blank. If it is blank and we don't
die, then we fall back to the username from the passwd file.
The current code writes the value into git_default_name.
However, that's not necessarily correct, as the empty value
might have come from git_default_name, or it might have been
passed in. This leads to two potential problems:
1. If we are overriding an empty name in the passed-in
value, then we may be overwriting a perfectly good name
(from gitconfig or gecos) in the git_default_name
buffer. Later calls to fmt_ident will end up using the
fallback name, even though a better name was available.
2. If we override an empty gecos name, we end up with the
fallback name in git_default_name. A later call that
uses IDENT_ERROR_ON_NO_NAME will see the fallback name
and think that it is a good name, instead of producing
an error. In other words, a blank gecos name would
cause an error with this code:
git_committer_info(IDENT_ERROR_ON_NO_NAME);
but not this:
git_committer_info(0);
git_committer_info(IDENT_ERROR_ON_NO_NAME);
because in the latter case, the first call has polluted
the name buffer.
Instead, let's make the fallback a per-invocation variable.
We can just use the pw->pw_name string directly, since it
only needs to persist through the rest of the function (and
we don't do any other getpwent calls).
Note that while this solves (1) for future invocations of
fmt_indent, the current invocation might use the fallback
when it could in theory load a better value from
git_default_name. However, by not passing
IDENT_ERROR_ON_NO_NAME, the caller is indicating that it
does not care too much about the name, anyway, so we don't
bother; this is primarily about protecting future callers
who do care.
Signed-off-by: Jeff King <redacted>
---
You can trigger bug (2) by having an empty gecos field, no user.name,
and running a merge. The recent credit_person code path will call
git_committer_info(0), then the commit-generating code path will call
git_committer_info(IDENT_ERROR_ON_NO_NAME), and you will end
up with a commit with your username in the "name" field (even though the
second call would want to error out).
I don't think (1) is triggerable in the current code, mainly because we
typically are feeding getenv("GIT_*_NAME") to fmt_ident, and a blank
there will end up causing an error later on in the program.
By the way, I left it so that:
GIT_COMMITTER_NAME= git ...
will continue to fallback to the passwd username if a caller doesn't
request to die on a blank name. We could pretty easily die in that case,
but people with flaky scripts might be affected. I figured that callers
who don't give ERROR_ON_NO_NAME don't really care too much what's in the
name anyway, so there's not much point in tightening the rules.
ident.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
From: Jeff King <hidden> Date: 2016-06-15 22:53:51
When we pull the user's name from the GECOS field of the
passwd file (or generate an email address based on their
username and hostname), we put the result into a
static buffer. While it's extremely unlikely that anybody
ever hit these limits (after all, in such a case their
parents must have hated them), we still had to deal with the
error cases in our code.
Converting these static buffers to strbufs lets us simplify
the code and drop some error messages from the documentation
that have confused some users.
The conversion is mostly mechanical: replace string copies
with strbuf equivalents, and access the strbuf.buf directly.
There are a few exceptions:
- copy_gecos and copy_email are the big winners in code
reduction (since they no longer have to manage the
string length manually)
- git_ident_config wants to replace old versions of
the default name (e.g., if we read the config multiple
times), so it must reset+add to the strbuf instead of
just adding
Note that there is still one length limitation: the
gethostname interface requires us to provide a static
buffer, so we arbitrarily choose 1024 bytes for the
hostname.
Signed-off-by: Jeff King <redacted>
---
Documentation/git-commit-tree.txt | 4 --
Documentation/git-var.txt | 4 --
ident.c | 100 +++++++++++++++-----------------------
3 files changed, 39 insertions(+), 69 deletions(-)
@@ -92,10 +92,6 @@ Diagnostics ----------- You don't exist. Go away!:: The passwd(5) gecos field couldn't be read-Your parents must have hated you!::- The passwd(5) gecos field is longer than a giant static buffer.-Your sysadmin must hate you!::- The passwd(5) name field is longer than a giant static buffer. Discussion ----------
@@ -63,10 +63,6 @@ Diagnostics ----------- You don't exist. Go away!:: The passwd(5) gecos field couldn't be read-Your parents must have hated you!::- The passwd(5) gecos field is longer than a giant static buffer.-Your sysadmin must hate you!::- The passwd(5) name field is longer than a giant static buffer. SEE ALSO --------
@@ -19,42 +18,27 @@ int user_ident_explicitly_given;#define get_gecos(struct_passwd) ((struct_passwd)->pw_gecos)#endif-staticvoidcopy_gecos(conststructpasswd*w,char*name,size_tsz)+staticvoidcopy_gecos(conststructpasswd*w,structstrbuf*name){-char*src,*dst;-size_tlen,nlen;--nlen=strlen(w->pw_name);+char*src;/* Traditionally GECOS field had office phone numbers etc, separated*withcommas.Also&standsforcapitalizedformoftheloginname.*/-for(len=0,dst=name,src=get_gecos(w);len<sz;src++){+for(src=get_gecos(w);*src&&*src!=',';src++){intch=*src;-if(ch!='&'){-*dst++=ch;-if(ch==0||ch==',')-break;-len++;-continue;-}-if(len+nlen<sz){+if(ch!='&')+strbuf_addch(name,ch);+else{/* Sorry, Mr. McDonald... */-*dst++=toupper(*w->pw_name);-memcpy(dst,w->pw_name+1,nlen-1);-dst+=nlen-1;-len+=nlen;+strbuf_addch(name,toupper(*w->pw_name));+strbuf_addstr(name,w->pw_name+1);}}-if(len<sz)-name[len]=0;-else-die("Your parents must have hated you!");-}-staticintadd_mailname_host(char*buf,size_tlen)+staticintadd_mailname_host(structstrbuf*buf){FILE*mailname;
@@ -77,78 +61,70 @@ static int add_mailname_host(char *buf, size_t len)return0;}-staticvoidadd_domainname(char*buf,size_tlen)+staticvoidadd_domainname(structstrbuf*out){+charbuf[1024];structhostent*he;-size_tnamelen;constchar*domainname;-if(gethostname(buf,len)){+if(gethostname(buf,sizeof(buf))){warning("cannot get host name: %s",strerror(errno));-strlcpy(buf,"(none)",len);+strbuf_addstr(out,"(none)");return;}-namelen=strlen(buf);-if(memchr(buf,'.',namelen))+strbuf_addstr(out,buf);+if(strchr(buf,'.'))return;he=gethostbyname(buf);-buf[namelen++]='.';-buf+=namelen;-len-=namelen;+strbuf_addch(out,'.');if(he&&(domainname=strchr(he->h_name,'.')))-strlcpy(buf,domainname+1,len);+strbuf_addstr(out,domainname+1);else-strlcpy(buf,"(none)",len);+strbuf_addstr(out,"(none)");}-staticvoidcopy_email(conststructpasswd*pw)+staticvoidcopy_email(conststructpasswd*pw,structstrbuf*email){/**Makeupafakeemailaddress*(name+'@'+hostname[+'.'+domainname])*/-size_tlen=strlen(pw->pw_name);-if(len>sizeof(git_default_email)/2)-die("Your sysadmin must hate you!");-memcpy(git_default_email,pw->pw_name,len);-git_default_email[len++]='@';--if(!add_mailname_host(git_default_email+len,-sizeof(git_default_email)-len))+strbuf_addstr(email,pw->pw_name);+strbuf_addch(email,'@');++if(!add_mailname_host(email))return;/* read from "/etc/mailname" (Debian) */-add_domainname(git_default_email+len,-sizeof(git_default_email)-len);+add_domainname(email);}constchar*ident_default_name(void){-if(!git_default_name[0]){+if(!git_default_name.len){structpasswd*pw=getpwuid(getuid());if(!pw)die("You don't exist. Go away!");-copy_gecos(pw,git_default_name,sizeof(git_default_name));+copy_gecos(pw,&git_default_name);}-returngit_default_name;+returngit_default_name.buf;}constchar*ident_default_email(void){-if(!git_default_email[0]){+if(!git_default_email.len){constchar*email=getenv("EMAIL");if(email&&email[0]){-strlcpy(git_default_email,email,-sizeof(git_default_email));+strbuf_addstr(&git_default_email,email);user_ident_explicitly_given|=IDENT_MAIL_GIVEN;}else{structpasswd*pw=getpwuid(getuid());if(!pw)die("You don't exist. Go away!");-copy_email(pw);+copy_email(pw,&git_default_email);}}-returngit_default_email;+returngit_default_email.buf;}constchar*ident_default_date(void)
From: Jeff King <hidden> Date: 2016-06-15 22:53:51
On Tue, May 15, 2012 at 11:10:36AM -0700, Junio C Hamano wrote:
Jeff King [off-list ref] writes:
quoted
So it seems to me like a much simpler set of rules would be:
1. When reading gecos, always fall back to the username if the gecos
field is unavailable or blank.
2. Always die when the name field is blank. That means we will die
when you pass in a bogus empty GIT_COMMITTER_NAME (or an empty
config name), which makes a lot more sense to me than falling back;
those are bogus requests, not system config problems. And we won't
ever have a blank gecos name, because we'll always fall back on the
username.
That certainly sounds very simple to explain and understand, and I do not
offhand think of anything *sane* that would break ;-)
Actually, it does end up breaking. The "error_on_no_name" check wants to
see the value _before_ the fallback, because it is not just about "do
not hand out an ident with an empty name", but rather about "do not hand
out this crappy fallback name when the gecos field is empty". So we
cannot do an "always fallback" behavior without breaking that check.
I ended up avoiding the issue in a different way, as you'll see in patch
8 below. Here's the series:
[01/13]: ident: split setup_ident into separate functions
[02/13]: http-push: do not access git_default_email directly
[03/13]: fmt-merge-msg: don't use static buffer in record_person
[04/13]: move identity config parsing to ident.c
[05/13]: move git_default_* variables to ident.c
[06/13]: format-patch: use default email for generating message ids
[07/13]: fmt_ident: drop IDENT_WARN_ON_NO_NAME code
[08/13]: ident: don't write fallback username into git_default_name
[09/13]: drop length limitations on gecos-derived names and emails
[10/13]: ident: report passwd errors with a more friendly message
[11/13]: ident: use full dns names to generate email addresses
[12/13]: ident: use a dynamic strbuf in fmt_ident
[13/13]: format-patch: refactor get_patch_filename
Patches 2, 8, and 11 fix actual bugs. The rest of it is refactoring and
cleanup; here's the diffstat:
Documentation/git-commit-tree.txt | 9 --
Documentation/git-var.txt | 9 --
builtin/fmt-merge-msg.c | 8 +-
builtin/log.c | 45 ++------
cache.h | 12 +-
config.c | 24 +----
environment.c | 3 -
git-compat-util.h | 3 +
http-push.c | 2 +-
ident.c | 213 ++++++++++++++++---------------------
log-tree.c | 19 ++--
log-tree.h | 4 +-
wrapper.c | 12 ++
13 files changed, 142 insertions(+), 221 deletions(-)
-Peff
From: Jeff King <hidden> Date: 2016-06-15 22:53:51
When getpwuid fails, we give a cute but cryptic message.
While it makes sense if you know that getpwuid or identity
functions are being called, this code is triggered behind
the scenes by quite a few git commands these days (e.g.,
receive-pack on a remote server might use it for a reflog;
the current message is hard to distinguish from an
authentication error). Let's switch to something that gives
a little more context.
While we're at it, we can factor out all of the
cut-and-pastes of the "you don't exist" message into a
wrapper function. Rather than provide xgetpwuid, let's make
it even more specific to just getting the passwd entry for
the current uid. That's the only way we use getpwuid anyway,
and it lets us make an even more specific error message.
The current message also fails to mention errno. While the
usual cause for getpwuid failing is that the user does not
exist, mentioning errno makes it easier to diagnose these
problems. Note that POSIX specifies that errno remain
untouched if the passwd entry does not exist (but will be
set on actual errors), whereas some systems will return
ENOENT or similar for a missing entry. We handle both cases
in our wrapper.
Signed-off-by: Jeff King <redacted>
---
Documentation/git-commit-tree.txt | 5 -----
Documentation/git-var.txt | 5 -----
git-compat-util.h | 3 +++
ident.c | 20 +++++---------------
wrapper.c | 12 ++++++++++++
5 files changed, 20 insertions(+), 25 deletions(-)
@@ -88,11 +88,6 @@ for one to be entered and terminated with ^D. include::date-formats.txt[]-Diagnostics-------------You don't exist. Go away!::- The passwd(5) gecos field couldn't be read- Discussion ----------
@@ -59,11 +59,6 @@ ifdef::git-default-pager[] The build you are using chose '{git-default-pager}' as the default. endif::git-default-pager[]-Diagnostics-------------You don't exist. Go away!::- The passwd(5) gecos field couldn't be read- SEE ALSO -------- linkgit:git-commit-tree[1]
@@ -595,4 +595,7 @@ int rmdir_or_warn(const char *path);*/intremove_or_warn(unsignedintmode,constchar*path);+/* Get the passwd entry for the UID of the current process. */+structpasswd*xgetpwuid_self(void);+#endif
@@ -402,3 +402,15 @@ int remove_or_warn(unsigned int mode, const char *file){returnS_ISGITLINK(mode)?rmdir_or_warn(file):unlink_or_warn(file);}++structpasswd*xgetpwuid_self(void)+{+structpasswd*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"));+returnpw;+}
From: Jeff King <hidden> Date: 2016-06-15 22:53:51
When we construct an email address from the username and
hostname, we generate the host part of the email with this
procedure:
1. add the result of gethostname
2. if it has a dot, ok, it's fully qualified
3. if not, then look up the unqualified hostname via
gethostbyname; take the domain name of the result and
append it to the hostname
Step 3 can actually produce a bogus result, as the name
returned by gethostbyname may not be related to the hostname
we fed it (e.g., consider a machine "foo" with names
"foo.one.example.com" and "bar.two.example.com"; we may have
the latter returned and generate the bogus name
"foo.two.example.com").
This patch simply uses the full hostname returned by
gethostbyname. In the common case that the first part is the
same as the unqualified hostname, the behavior is identical.
And in the case that it is not the same, we are much more
likely to be generating a valid name.
Signed-off-by: Jeff King <redacted>
---
It takes a pretty odd /etc/hosts setup, but I was able to trigger this
bug. I doubt anyone is affected in practice, but hey, the right code is
5 lines shorter (and easier to understand).
ident.c | 13 ++++---------
1 file changed, 4 insertions(+), 9 deletions(-)
From: Jeff King <hidden> Date: 2016-06-15 22:53:51
Now that we accept arbitrary-sized names and email
addresses, the only remaining limit is in the actual
formatting of the names into a buffer. The current limit is
1000 characters, which is not likely to be reached, but
using a strbuf is one less error condition we have to worry
about.
Signed-off-by: Jeff King <redacted>
---
ident.c | 43 +++++++++++++++----------------------------
1 file changed, 15 insertions(+), 28 deletions(-)
From: Jeff King <hidden> Date: 2016-06-15 22:53:51
The get_patch_filename function expects a commit argument
and uses it to get the sanitized subject line when making a
patch filename. However, we also want to use this same
function for the cover letter, which does not have a commit
object. The current solution is to create a fake commit with
the subject "cover letter". Instead, let's make the
get_patch_filename interface more flexibile, and allow
passing a direct subject.
Signed-off-by: Jeff King <redacted>
---
This one is a bonus. It doesn't really have anything to do with ident,
except that I came across it while trying to figure out how the
committer info is being used in make_cover_letter.
builtin/log.c | 35 +++++++----------------------------
log-tree.c | 19 +++++++++++--------
log-tree.h | 4 ++--
3 files changed, 20 insertions(+), 38 deletions(-)
@@ -299,19 +299,22 @@ static unsigned int digits_in_number(unsigned int number)returnresult;}-voidget_patch_filename(structcommit*commit,intnr,constchar*suffix,-structstrbuf*buf)+voidget_patch_filename(structcommit*commit,constchar*subject,intnr,+constchar*suffix,structstrbuf*buf){intsuffix_len=strlen(suffix)+1;intstart_len=buf->len;-strbuf_addf(buf,commit?"%04d-":"%d",nr);-if(commit){+strbuf_addf(buf,commit||subject?"%04d-":"%d",nr);+if(commit||subject){intmax_len=start_len+FORMAT_PATCH_NAME_MAX-suffix_len;structpretty_print_contextctx={0};-ctx.date_mode=DATE_NORMAL;-format_commit_message(commit,"%f",buf,&ctx);+if(subject)+strbuf_addstr(buf,subject);+elseif(commit)+format_commit_message(commit,"%f",buf,&ctx);+if(max_len<buf->len)strbuf_setlen(buf,max_len);strbuf_addstr(buf,suffix);
From: Junio C Hamano <hidden> Date: 2016-06-15 22:53:51
Jeff King [off-list ref] writes:
We try to generate a sane message id for cover letters and
threading by appending some changing bits to the front of
the user's email address. The current code parses the email
out of the results of git_committer_info, but we can do this
much more easily by just calling ident_default_email
ourselves.
Signed-off-by: Jeff King <redacted>
---
Technically this is a regression if you really wanted:
GIT_COMMITTER_EMAIL=some.addr@example.com \
git format-patch --thread=deep
to make your environment variable part of the message-ids. I don't think
it matters, but I can adjust it if we care.
Is it because you no longer explicitly ask for "committer" and get generic
"who am I" bit from the ident infrastructure?
I wouldn't be surprised if some automated "commit email notification
reacting to push" bot in post-receive hook is using the environment
variable to affect the message ID. I would doubt that would break the
message as long as the message ID generated from this codepath stays
valid, though, so I wouldn't worry about complaints along the lines of
"you started using names different from what you used to use". As long as
we don't die due to "Hey bot, you do not seem to have a valid e-mail
address!", I don't think we need to worry about it.
From: Jeff King <hidden> Date: 2016-06-15 22:53:51
On Sun, May 20, 2012 at 07:58:04PM -0700, Junio C Hamano wrote:
quoted
Technically this is a regression if you really wanted:
GIT_COMMITTER_EMAIL=some.addr@example.com \
git format-patch --thread=deep
to make your environment variable part of the message-ids. I don't think
it matters, but I can adjust it if we care.
Is it because you no longer explicitly ask for "committer" and get generic
"who am I" bit from the ident infrastructure?
Right. Calling git_committer_info will also check
getenv("GIT_COMMITTER_EMAIL"), but we don't bother to do that here. We
could change the code to:
const char *email = getenv("GIT_COMMITTER_EMAIL");
if (!email)
email = ident_default_email();
if it matters. I don't think it's worth building an alternate version of
git_committer_info that doesn't do the "name" half of the info.
I wouldn't be surprised if some automated "commit email notification
reacting to push" bot in post-receive hook is using the environment
variable to affect the message ID. I would doubt that would break the
message as long as the message ID generated from this codepath stays
valid, though, so I wouldn't worry about complaints along the lines of
"you started using names different from what you used to use". As long as
we don't die due to "Hey bot, you do not seem to have a valid e-mail
address!", I don't think we need to worry about it.
No, we'll never die as a result. But if you have a poorly configured
machine, you might get "user@host.(none)". Or I suppose you could leak
information about the username of the post-receive process. In both
cases, we do this already, so the only change is if you are trying to
override that with GIT_COMMITTER_EMAIL.
So it's a little far-fetched, which is why I didn't bother. But the code
above is quite simple, so maybe it is better to be conservative.
-Peff