[PATCH] Add committer and author names to top of COMMIT_EDITMSG.

[PATCH] Add committer and author names to top of COMMIT_EDITMSG.

[Stephen Sinclair]

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(-)

diff --git a/builtin-commit.c b/builtin-commit.c
index 73f1e35..4fd9367 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -423,8 +423,18 @@ static int prepare_log_message(const char
*index_file, const char *prefix)
 			"#\n",
 			git_path("MERGE_HEAD"));

+    fprintf(fp, "\n");
+
+    fprintf(fp,
+            "# Committer: %s\n"
+            "# Author:    %s\n"
+            "#\n",
+            fmt_name(getenv("GIT_AUTHOR_NAME"),
+                     getenv("GIT_AUTHOR_EMAIL")),
+            fmt_name(getenv("GIT_COMMITTER_NAME"),
+                     getenv("GIT_COMMITTER_EMAIL")));
+
 	fprintf(fp,
-		"\n"
 		"# Please enter the commit message for your changes.\n"
 		"# (Comment lines starting with '#' will ");
 	if (cleanup_mode == CLEANUP_ALL)
-- 
1.5.4.rc2.85.ga7943-dirty

Re: [PATCH] Add committer and author names to top of COMMIT_EDITMSG.

[Johannes Schindelin]

Hi,

On Fri, 11 Jan 2008, Stephen Sinclair wrote:

> Add committer and author names to top of COMMIT_EDITMSG.

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

Re: [PATCH] Add committer and author names to top of COMMIT_EDITMSG.

[Junio C Hamano]

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

> @@ -423,8 +423,18 @@ static int prepare_log_message(const char
> *index_file, const char *prefix)
>  			"#\n",
>  			git_path("MERGE_HEAD"));
>
> +    fprintf(fp, "\n");
> +
> +    fprintf(fp,
> +            "# Committer: %s\n"
> +            "# Author:    %s\n"
> +            "#\n",
> +            fmt_name(getenv("GIT_AUTHOR_NAME"),
> +                     getenv("GIT_AUTHOR_EMAIL")),
> +            fmt_name(getenv("GIT_COMMITTER_NAME"),
> +                     getenv("GIT_COMMITTER_EMAIL")));
> +

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.

Re: [PATCH] Add committer and author names to top of COMMIT_EDITMSG.

[Stephen Sinclair]

> 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.

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.


> 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.

Sure, that's possible.


> I strongly agree with Dscho that this change needs to be
> defended with a good description on the reason why this is good.

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..)


> 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.

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

Re: [PATCH] Add committer and author names to top of COMMIT_EDITMSG.

[Junio C Hamano]

"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.
>
> The purpose of my patch was to remind _myself_ what my name is, in
> case I hadn't configured it correctly.

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.

Re: [PATCH] Add committer and author names to top of COMMIT_EDITMSG.

[Stephen Sinclair]

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

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.

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

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

Re: [PATCH] Add committer and author names to top of COMMIT_EDITMSG.

[Junio C Hamano]

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

>>  * If AUTHOR_NAME+EMAIL is different from AUTHOR_NAME+EMAIL that
>>    I would normally get for myself, or
>
> 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.

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).

>>  * If AUTHOR_NAME+EMAIL contains garbage identifier commonly
>>    found when misconfigured (e.g. ".(none)" at the end of
>>    e-mail),
>
> 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.

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.

> 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?

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.

Re: [PATCH] Add committer and author names to top of COMMIT_EDITMSG.

[Stephen Sinclair]

> 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.

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.)


> ".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.

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.


> Perhaps we could disable the code that reads from hostname and
> gecos, and instead always force the users to configure.

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.

> But that kind of change is not something I'd want to be
> discussing right now.

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

thanks for the comments,
Steve

Re: [PATCH] Add committer and author names to top of COMMIT_EDITMSG.

[Jeff King]

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

> 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.

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"?

> 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.

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

Re: [PATCH] Add committer and author names to top of COMMIT_EDITMSG.

[Junio C Hamano]

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

>> 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.
>
> 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?

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.

> 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.

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.

Re: [PATCH] Add committer and author names to top of COMMIT_EDITMSG.

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> 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.

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.

Re: [PATCH] Add committer and author names to top of COMMIT_EDITMSG.

[Junio C Hamano]

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

> Jeff King <peff@peff.net> writes:
>
>> 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 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.

Re: [PATCH] Add committer and author names to top of COMMIT_EDITMSG.

[Jeff King]

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

> 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.

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

Re: [PATCH] Add committer and author names to top of COMMIT_EDITMSG.

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Fri, Jan 11, 2008 at 09:30:24PM -0800, Junio C Hamano wrote:
>
>> 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.
>
> 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).

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

Re: [PATCH] Add committer and author names to top of COMMIT_EDITMSG.

[Jeff King]

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

> > list (ISTR that Linus _likes_ pulling the hostname from gecos).
> 
> You do not pull hostname from gecos, but I agree with your main
> point.

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

-Peff

Re: [PATCH] Add committer and author names to top of COMMIT_EDITMSG.

[Stephen Sinclair]

> 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.

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.


> 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?

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.


> 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.

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

Re: [PATCH] Add committer and author names to top of COMMIT_EDITMSG.

[Junio C Hamano]

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

>> 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.
>
> 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.

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.