patchAdd committer and author names to top of COMMIT_EDITMSG.

Add committer and author names to top of COMMIT_EDITMSG.

Signed-off-by: Stephen Sinclair <radarsat1@gmail.com>
---
 builtin-commit.c |   12 +++++++++++-
 1 files changed, 11 insertions(+), 1 deletions(-)

Hi,

On Fri, 11 Jan 2008, Stephen Sinclair wrote:

This commit message is severely lacking: it is just a repetition of the commit subject, it is too technical (what is COMMIT_EDITMSG for, anyway?), and even worse, it does not begin to explain _why_ this is a good change.

Ciao, Dscho

"Stephen Sinclair" <radarsat1@gmail.com> writes:

I'd almost agree with this patch if if added AUTHOR but not COMMITTER, and only when AUTHOR is different from me. That would help reassure anybody while amending other's changes. COMMITTER is always me and I should not reminded with extra lines that waste precious screen real estate.

And no, I did not check if your change correctly supports the use case of amending other's changes. But if I recall the code correctly, I suspect that your change doesn't. The recorded author is determined after the log message is prepared, way later.

I strongly agree with Dscho that this change needs to be defended with a good description on the reason why this is good. If the reason is "newbie protection", I do not think this is a good change at all. Newbie protection is never a good reason to make people who graduated that state to pay extra price unconditionally.

The purpose of my patch was to remind _myself_ what my name is, in case I hadn't configured it correctly. But I can see this use case also being useful.

Sure, that's possible.

The patch was really to go along with my RFC about the idea. I guess it was too early to post a possible implementation. (I have only just begun to look at the git code after all..)

I agree. I wouldn't necessarily say it is "newbie protection", so much as a friendly reminder of what username you are using while doing a commit, which, as I said, might not be as expected if you have just sat down at a new machine. Especially if you have been using git for a long time on a single machine, it is something you might easily forget to configure. (As I have, several times now.) I agree, however, that this it is not necessarily worth having this on the screen every time you do a commit, for the exceptional instance where it might be wrong. Perhaps more usefully it could appear only if you haven't yet created a user.email and user.name config entry.

Actually in my honest opinion, the default of using the computer's host name and login is pretty much _never_ right, but I thought this patch might be less intrusive than introducing a new error message.

I do have a slightly better patch now that has a more informative message and uses git_committer_info() and git_author_info(), however I'll wait for any more opinions before posting it.

In retrospect, I guess I could just as easily solve my problem by introducing a post-receive hook for my personal repo that issues a warning for commits not configured to my email address.

Steve

"Stephen Sinclair" <radarsat1@gmail.com> writes:

In that case, I would imagine a rule like this would be more appropriate than unconditionally showing AUTHOR/COMMITTER in all cases:

 * If AUTHOR_NAME+EMAIL is different from AUTHOR_NAME+EMAIL that
   I would normally get for myself, or

 * If AUTHOR_NAME+EMAIL contains garbage identifier commonly
   found when misconfigured (e.g. ".(none)" at the end of
   e-mail),

then show AUTHOR. In addition, if it is the latter case, give hints to configure before casting the mistake in stone.

I thought of this, however if the purpose of this is to handle a case where you do a commit from a new and unconfigured user account, "that I would normally get for myself" is undefined, since this information is (rightfully) not propagated by git-clone. This is why I made it unconditional, (or perhaps something you could could turn off, but would by default be on), but I figured there would be objections since I admit it's not always useful information.

That's more interesting to me. I just checked my logs and I do see that in at least one case, this .(none) was not appended. The computer in question was configured (not by me) with a domain of ".local", so the commit has <machinename>.local as part of the email address. However I would imagine this might solve most cases.

I still don't understand why git generates a default email address instead of just giving an error message; do people actually use this scenario? In my experience an email address must always be explicitly given, but perhaps some people work on the machines that also receive their mail. I rarely do "real" work on an actual server, but I guess some people do. I think they must be in the minority though..

On the other hand, now that I've been thinking about it I think my idea of simply configuring a hook in my personal central git is probably an easier and all-round better solution to my problem. I understand that git relies on system accounts for security, but there's no reason I can't configure a particular repo to issue a warning when it receives incoming commits from an unknown user/email.

Steve

"Stephen Sinclair" <radarsat1@gmail.com> writes:

What are you talking about?

In a properly configured repository, telling you who git thinks you are is _ALWAYS_ useless (that's the definition of "properly configured"). Just admit it.

The only case it is of any use is to remind people who amend other people's change. Showing the AUTHOR for the commit being created would add value (and the knowledge that git shows AUTHOR in that situation would help remind you that it will be recording your own name if you do not see that line).

Yes, and please notice that "e.g." in my description means "I am just giving you an example, not the exhaustive list for the final solution but a hint to one possibly acceptable solution". ".local", "@localhost", "@<distroname>" and ".(none)" are all plausible red-flag raisers. There may be more, but I think we should be able to catch most misconfigurations with simple rules.

The official party line to defend the existing behaviour is that there is no need to configure anything, when the host and gecos is done properly. But I tend to agree with you that quite a lot of systems are not "done properly", and users cannot do much about it in some cases. I think most of misconfigured systems are personal boxes they have control over but not all.

Perhaps we could disable the code that reads from hostname and gecos, and instead always force the users to configure. But that kind of change is not something I'd want to be discussing right now.

Well, I'll admit that I don't really understand you here. Maybe I'm still too much of a git newbie on this. (Fair enough.) Right now the only way to make sure I'm committing as myself with my proper email address is to:

-- remember to "git-config --list", and check that my email is listed. -- "git-commit; git-log", and remember to check the last entry before doing a "git-push".

Am I missing something?

If proper use of git seems to require remembering one of these two things, that's okay with me, I'll just do my best, but it was an area where I thought I might suggest an improvement. (As a rule, I prefer letting the computer remember things for me.)

I'd have to disagree with you here. Most people name their boxes one thing or another, and trying to catch it with some rule is pointless. Especially considering the default name is taken from the hostname anyway -- you're taking the local hostname and then checking with a rule to see if it might be localhost. Personally I think the solution is not to take the hostname in the first place, since in my experience it's rarely equivalent to a valid email address.

Obviously though my personal experience is apparently not the same as that of others'. I do most of my development on personal boxes, or laptops configured by some non-professional guy in my lab at university. Or on virtual machines, which was my recent case.

That would be my preference, and I do think there's a case for it. But whether it's a strong one or not I'm not sure.

That's okay. In the spirit of git, I'll just solve my problem in my own branch.. ;-)

thanks for the comments, Steve

"Stephen Sinclair" <radarsat1@gmail.com> writes:

They are both valid means to make sure you did not misconfigure.

You omitted the part that matters from the part you quoted above, but this discussion was about "showing AUTHOR if it is different from me", which was _one of_ the two conditions I suggested in my counterproposal, and I was saying that it is useless to expect that you would be able to find a misconfiguration when AUTHOR is shown for this first reason. This part is _not_ about catching your misconfiguration.

The other part is about the misconfiguration catching.

Yes, and earlier you said one of the undesirable ones was "yourname@foo.local" (and others were "yourname@foo.(none)"). IOW, "localhost" is one of the things you want to catch as unconfigured bogosity that you want to catch, isn't it?

And that is _the other_ condition in my counterproposal to show AUTHOR.

To rephrase, you would show AUTHOR when one of the conditions holds true, either:

 (1) "not me" (so that we can remind that other's commit is
     being amended); _OR_ 

 (2) "funny me" (so that we can catch misconfiguration.

The latter would not have to trigger once you configure your ~/.gitconfig (or .git/config) properly.

Okay, sorry I guess I was misreading it because that wasn't really my original intention with the patch. However in that context I can see that showing author information when it differs from yourself could definitely be useful.

Well, it was only an example to show that it's easily possible (in fact, common) to have hostnames that are not configured as actual email domains. The fact that the hostname contained the word "local" was an indication, but I'm not really sure that filtering for that word in the hostname would be such a good idea. I'm also not sure how many admins will even end up sticking ".local" as their domain, it's probably just a quirk of the administrator for my lab.

I guess "localhost" really clearly is a "bogus" host name, but other than that I can't think of any real rules that would make sense. Rather, IMHO, the error is earlier in the chain: doing anything with the hostname in the first place.

I definitely agree with (1), though having not really done much amending of other people's commits I can't vouch for it. I think (2) might not be very reliable.

Since submitting the patch, I have added a post-receive hook to my repo which checks all incoming commits and verifies whether any names or email addresses are not in a whitelist. If any are flagged, a warning is displayed. This is actually quite satisfactory for me, since it'll warn me when I accidentally push commits with the wrong name to my private repo, but before I push to public. Let me know if something like this would be useful for the contrib folder..

Steve

"Stephen Sinclair" <radarsat1@gmail.com> writes:

Yeah, I tend to agree that (2) is probably impossible to achieve.

Author ident should be reachable e-mail address but some people seem to prefer committer ident to be tied to the actual machine even if that makes the ident something that merely resembles a valid reachable e-mail address but in fact unreachable. For such a committer ident, taking it from hostname would be a reasonable thing to do, but I suspect that is a minority.

Unfortunately we do not have an easy way (other than using GIT_COMMITTER_EMAIL environment) to define different author and committer idents.

On Fri, Jan 11, 2008 at 05:53:12PM -0800, Junio C Hamano wrote:

I think there are plenty of reasons for the host/gecos information not being useful. Is a workstation whose hostname is not a valid mailing address really not "done properly"?

This is obviously not 1.5.4 material, so I haven't given it that much thought either. But perhaps Stephen's "author message" should simply trigger any time the author is pulled from gecos? I suppose that would annoy people who use this feature all the time, but they can silence the "warning" with a simple git-config.

-Peff

Jeff King <peff@peff.net> writes:

Well, we could certainly do that.

But I am not entirely happy with the idea of having to make the default silly and inconvenient, only because otherwise new people who did not even bother to read and follow the VERY FIRST subsection of the tutorial that tells them that the first thing to do is to use user.name and user.email would not notice their problems, and experts know enough to squelch that broken default. Middle level people (and newbies will quickly become one of them) will be inconvenienced even though they followed the tutorial's instruction, until they find the configuration variable to turn that silly AUTHOR output off.

Junio C Hamano <gitster@pobox.com> writes:

I think what we could do if we wanted to protect people from unconfigured identity is to stop pulling names from gecos and hostname, and respect _only_ environment and config.

And fail any operation that we would want configured name and email (names in reflog does not count as such an operation --- we've made that mistake once, which made "git clone" impossible before setting up $HOME/.gitconfig).

That I think I can live with.

If we do that, we do not have to see two extra lines (COMMITTER and AUTHOR) that spells our own name in the commit message buffer, which almost always will be a total waste of space.

On Fri, Jan 11, 2008 at 09:30:24PM -0800, Junio C Hamano wrote:

I am fine with that, as I have always set up the configuration manually anyway. But I think it would need a significant comment period from the list (ISTR that Linus _likes_ pulling the hostname from gecos).

-Peff

Jeff King <peff@peff.net> writes:

You do not pull hostname from gecos, but I agree with your main point.

On Fri, Jan 11, 2008 at 09:56:15PM -0800, Junio C Hamano wrote:

Heh. Sorry, the sentence made more sense before I edited it.

-Peff