Jeff King [off-list ref] writes:
On Mon, May 14, 2012 at 05:13:24PM -0400, Jeff King wrote:
We call setup_ident with our name pointer, which usually comes from
getenv("GIT_*_NAME"), although could also come from something like "git
commit -c $commit". We feed that to setup_ident. If name is NULL, then
setup_ident will use git_default_name (filling it in from gecos or
config). If it's not NULL, then we use it literally. And then we check
_that_ result to see if it's empty. If it is, we either die or warn,
depending on the flags. In the latter case, we fallback to using the
username as the name.
And that's what confuses me. Depending on what was passed in, we may
have checked that GIT_COMMITTER_NAME is an empty string, or we may have
checked that the config or gecos field yielded an empty string.
Sounds quite sensible to me, though.
In the
latter case, it makes sense to fall back to the username.
I agree that we should use something like "Sorry, Mr. McDonald" codepath
when the GECOS field returns an empty string---after all that is what we
do when we are built with NO_GECOS_IN_PWENT.
But in the
former case, it doesn't; we should fall back to the config name or the
gecos name.
If the user said GIT_COMMITTER_NAME is empty with "GIT_COMMITTER_NAME=",
that is different from saying with "unset GIT_COMMITTER_NAME" that the
user does not want the environment to take effect, no? So I do not think
falling back to configured or gecos in the former case is the right thing
to do, even though that would mean explicitly giving an empty string in
that configuration variable is asking only for an error without any
recourse, which is not useful at all.
On Tue, May 15, 2012 at 08:03:38AM -0700, Junio C Hamano wrote:
Jeff King [off-list ref] writes:
quoted
On Mon, May 14, 2012 at 05:13:24PM -0400, Jeff King wrote:
We call setup_ident with our name pointer, which usually comes from
getenv("GIT_*_NAME"), although could also come from something like "git
commit -c $commit". We feed that to setup_ident. If name is NULL, then
setup_ident will use git_default_name (filling it in from gecos or
config). If it's not NULL, then we use it literally. And then we check
_that_ result to see if it's empty. If it is, we either die or warn,
depending on the flags. In the latter case, we fallback to using the
username as the name.
And that's what confuses me. Depending on what was passed in, we may
have checked that GIT_COMMITTER_NAME is an empty string, or we may have
checked that the config or gecos field yielded an empty string.
Sounds quite sensible to me, though.
Yes, I think it is OK to check what was given to us (or our fallback).
But using that check to decide which next step to take doesn't seem
right.
quoted
In the
latter case, it makes sense to fall back to the username.
I agree that we should use something like "Sorry, Mr. McDonald" codepath
when the GECOS field returns an empty string---after all that is what we
do when we are built with NO_GECOS_IN_PWENT.
Right, and that is more or less what we do (just without the
capitalization).
quoted
But in the
former case, it doesn't; we should fall back to the config name or the
gecos name.
If the user said GIT_COMMITTER_NAME is empty with "GIT_COMMITTER_NAME=",
that is different from saying with "unset GIT_COMMITTER_NAME" that the
user does not want the environment to take effect, no?
I agree two the cases are different. And for the most part, you are
insane to pass an empty GIT_COMMITTER_NAME. But if you do, why would the
right behavior be to fall back to sticking the username into the name
field, and not the gecos name?
Part of me is wondering why we should fall back at all in that case. If
a caller does not pass ERROR_ON_NO_NAME, then they don't really care
what the name is, do they? The current callers that do not pass it are:
- blame.c:fake_working_tree_commit, which is passing in a fake name
buffer anyway (so will never trigger this code path)
- log.c:gen_message_id, which only cares about the email
portion anyway
- fmt-merge-msg.c:credit_people; this caller compares the name field
to what's in the commits, checking for differences. So it could just
as easily be "(none)" or some other token
- commit.c:prepare_to_commit; this compares and shows author and
commiter ids, and does not care about a blank name for the committer
(but does for the author). The commit can't go through anyway with a
blank committer name, so should it not just use ERROR_ON_NO_NAME?
- log.c:make_cover_letter; this uses the committer information to make
a fake commit that we ultimately use just to get the "%f" pretty
userformat from it. In other words, we don't care about the
committer at all, and this is really just working around an
absolutely horrific interface.
- refs.c:log_ref_write; finally, a caller who actually cares about the
name, but doesn't want to die if we don't have a good name. We are
happy enough with the username, though if somebody passes
GIT_COMMITTER_NAME=, wouldn't it be OK to fail?
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.
Again, I'm sorry to belabor this, and we can just drop it; I don't think
there's currently a bug. It's just that I'm cleaning up in the area, and
the current behavior seems overly complex; in particular, I'm worried
that writing the username into the git_default_name field (overwriting
the _real_ name the user gave us!) is a maintenance time-bomb that will
bite us later.
If I'm not being clear, I can express it in the form of patches, which
might be more obvious.
-Peff