From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jeff King Subject: [PATCH 1/2] commit: loosen ident checks when generating template Date: Wed, 10 Dec 2014 10:42:10 -0500 Message-ID: <20141210154209.GA20771@peff.net> References: <20141210153952.GA14910@peff.net> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Cc: Junio C Hamano , git@vger.kernel.org To: Simon X-From: git-owner@vger.kernel.org Wed Dec 10 16:42:19 2014 Return-path: Envelope-to: gcvg-git-2@plane.gmane.org Received: from vger.kernel.org ([209.132.180.67]) by plane.gmane.org with esmtp (Exim 4.69) (envelope-from ) id 1XyjP6-0000s0-IO for gcvg-git-2@plane.gmane.org; Wed, 10 Dec 2014 16:42:16 +0100 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757786AbaLJPmN (ORCPT ); Wed, 10 Dec 2014 10:42:13 -0500 Received: from cloud.peff.net ([50.56.180.127]:51098 "HELO cloud.peff.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with SMTP id S1757060AbaLJPmM (ORCPT ); Wed, 10 Dec 2014 10:42:12 -0500 Received: (qmail 10697 invoked by uid 102); 10 Dec 2014 15:42:12 -0000 Received: from Unknown (HELO peff.net) (10.0.1.1) by cloud.peff.net (qpsmtpd/0.84) with SMTP; Wed, 10 Dec 2014 09:42:12 -0600 Received: (qmail 10345 invoked by uid 107); 10 Dec 2014 15:42:17 -0000 Received: from sigill.intra.peff.net (HELO sigill.intra.peff.net) (10.0.0.7) by peff.net (qpsmtpd/0.84) with SMTP; Wed, 10 Dec 2014 10:42:17 -0500 Received: by sigill.intra.peff.net (sSMTP sendmail emulation); Wed, 10 Dec 2014 10:42:10 -0500 Content-Disposition: inline In-Reply-To: <20141210153952.GA14910@peff.net> Sender: git-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org Archived-At: When we generate the commit-message template, we try to report an author or committer ident that will be of interest to the user: an author that does not match the committer, or a committer that was auto-configured. When doing so, if we encounter what we consider to be a bogus ident, we immediately die. This is a bad idea, because our use of the idents here is purely informational. Any ident rules should be enforced elsewhere, because commits that do not invoke the editor will not even hit this code path (e.g., "git commit -mfoo" would work, but "git commit" would not). So at best, we are redundant with other checks, and at worse, we actively prevent commits that should otherwise be allowed. We should therefore do the minimal parsing we can to get a value and not do any validation (i.e., drop the call to sane_ident_split()). In theory we could notice when even our minimal parsing fails to work, and do the sane thing for each check (e.g., if we have an author but can't parse the committer, assume they are different and print the author). But we can actually simplify this even further. We know that the author and committer strings we are parsing have been generated by us earlier in the program, and therefore they must be parseable. We could just call split_ident_line without even checking its return value, knowing that it will put _something_ in the name/mail fields. Of course, to protect ourselves against future changes to the code, it makes sense to turn this into an assert, so we are not surprised if our assumption fails. Signed-off-by: Jeff King --- builtin/commit.c | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/builtin/commit.c b/builtin/commit.c index d1c90db..2be5506 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -502,6 +502,12 @@ static int is_a_merge(const struct commit *current_head) return !!(current_head->parents && current_head->parents->next); } +static void assert_split_ident(struct ident_split *id, const struct strbuf *buf) +{ + if (split_ident_line(id, buf->buf, buf->len)) + die("BUG: unable to parse our own ident: %s", buf->buf); +} + static void export_one(const char *var, const char *s, const char *e, int hack) { struct strbuf buf = STRBUF_INIT; @@ -608,13 +614,6 @@ static void determine_author_info(struct strbuf *author_ident) } } -static void split_ident_or_die(struct ident_split *id, const struct strbuf *buf) -{ - if (split_ident_line(id, buf->buf, buf->len) || - !sane_ident_split(id)) - die(_("Malformed ident string: '%s'"), buf->buf); -} - static int author_date_is_interesting(void) { return author_message || force_date; @@ -822,8 +821,14 @@ static int prepare_to_commit(const char *index_file, const char *prefix, status_printf_ln(s, GIT_COLOR_NORMAL, "%s", only_include_assumed); - split_ident_or_die(&ai, author_ident); - split_ident_or_die(&ci, &committer_ident); + /* + * These should never fail because we they come from our own + * fmt_ident. They may fail the sane_ident test, but we know + * that the name and mail pointers will at least be valid, + * which is enough for our tests and printing here. + */ + assert_split_ident(&ai, author_ident); + assert_split_ident(&ci, &committer_ident); if (ident_cmp(&ai, &ci)) status_printf_ln(s, GIT_COLOR_NORMAL, -- 2.2.0.454.g7eca6b7