Re: [regression] Re: [PATCHv2 10/15] drop length limitations on gecos-derived names and emails
- From
Jeff King <peff@peff.net>
- Date
- Jan 25, 2013, 01:05 UTC
- Message-ID
- <20130125010559.GA27657@sigill.intra.peff.net>
- In-Reply-To
- <20130124232146.GA17458@google.com>
On Thu, Jan 24, 2013 at 03:21:46PM -0800, Jonathan Nieder wrote:
Show 9 quoted lines
> This broke /etc/mailname handling. Before: > > $ git var GIT_COMMITTER_IDENT > Jonathan Nieder <jrn@mailname.example.com> 1359069165 -0800 > > After: > > $ git var GIT_COMMITTER_IDENT > Jonathan Nieder <mailname.example.com> 1359069192 -0800
Ick. I wonder how that slipped through...I know I was testing with /etc/mailname when developing the series, because I'm on a Debian system. We do even check this code path in t7502 (if you have the AUTOIDENT prereq), but of course we can't verify the actual value automatically, because it could be anything. So I guess I just missed it during my manual testing, and the automated testing is insufficient to catch this particular breakage.
> > - if (!fgets(buf, len, mailname)) {
> > + if (strbuf_getline(buf, mailname, '\n') == EOF) {
>
> This clears the strbuf.Right. Definitely the problem.
Show 36 quoted lines
> How about something like this as a quick fix?
>
> Reported-by: Mihai Rusu <dizzy@google.com>
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
>
> diff --git a/ident.c b/ident.c
> index 73a06a1..cabd73f 100644
> --- a/ident.c
> +++ b/ident.c
> @@ -41,6 +41,7 @@ static void copy_gecos(const struct passwd *w, struct strbuf *name)
> static int add_mailname_host(struct strbuf *buf)
> {
> FILE *mailname;
> + struct strbuf mailnamebuf = STRBUF_INIT;
>
> mailname = fopen("/etc/mailname", "r");
> if (!mailname) {
> @@ -49,14 +50,17 @@ static int add_mailname_host(struct strbuf *buf)
> strerror(errno));
> return -1;
> }
> - if (strbuf_getline(buf, mailname, '\n') == EOF) {
> + if (strbuf_getline(&mailnamebuf, mailname, '\n') == EOF) {
> if (ferror(mailname))
> warning("cannot read /etc/mailname: %s",
> strerror(errno));
> + strbuf_release(&mailnamebuf);
> fclose(mailname);
> return -1;
> }
> /* success! */
> + strbuf_addbuf(buf, &mailnamebuf);
> + strbuf_release(&mailnamebuf);
> fclose(mailname);
> return 0;
> }I think that is the only reasonable fix. Thanks for figuring it out.
We could expand the test in t7502 to check for "@" in the email, but it feels weirdly specific to this bug. Either way,
Acked-by: Jeff King <peff@peff.net>
(with a proper commit message, of course).
-Peff