# [PATCH] git cherry-pick: Add NULL check to sequencer parsing of HEAD 9 messages from 2012-05-03 to 2012-05-03. Participants: Neil Horman, René Scharfe, Matthieu Moy, Junio C Hamano. Thread: https://gitlist.dev/t/30404 ## Neil Horman, 2012-05-03 11:20 Subject: [PATCH] git cherry-pick: Add NULL check to sequencer parsing of HEAD Message-ID: <1336044026-16897-1-git-send-email-nhorman@tuxdriver.com> URL: https://gitlist.dev/e/1336044026-16897-1-git-send-email-nhorman%40tuxdriver.com ``` Michael Mueller noted that a feature I recently added failed to check the return of lookup_commit to ensure that it was not NULL. I don't think a NULL can actually happen in the this particular use case, but regardless it seems a good idea to check. Signed-off-by: Neil Horman --- sequencer.c | 11 ++++++++++- 1 files changed, 10 insertions(+), 1 deletions(-) diff --git a/sequencer.c b/sequencer.c index f83cdfd..ad4d781 100644 --- a/sequencer.c +++ b/sequencer.c @@ -261,7 +261,16 @@ static int is_index_unchanged(void) return error(_("Could not resolve HEAD commit\n")); head_commit = lookup_commit(head_sha1); - if (!head_commit || parse_commit(head_commit)) + + /* + * If head_commit is NULL, just return, as check_commit, + * called from lookup_commit, would have indicated that + * head_commit is not a commit object already. + */ + if (!head_commit) + return; + + if (parse_commit(head_commit)) return error(_("could not parse commit %s\n"), sha1_to_hex(head_commit->object.sha1)); -- 1.7.7.6 ``` ## René Scharfe, 2012-05-03 11:45 Subject: Re: [PATCH] git cherry-pick: Add NULL check to sequencer parsing of HEAD Message-ID: <4FA26FF2.2050607@lsrfire.ath.cx> URL: https://gitlist.dev/e/4FA26FF2.2050607%40lsrfire.ath.cx In-Reply-To: <1336044026-16897-1-git-send-email-nhorman@tuxdriver.com> ``` Am 03.05.2012 13:20, schrieb Neil Horman: > Michael Mueller noted that a feature I recently added failed to check the return > of lookup_commit to ensure that it was not NULL. I don't think a NULL can > actually happen in the this particular use case, but regardless it seems a good > idea to check. > > Signed-off-by: Neil Horman > --- > sequencer.c | 11 ++++++++++- > 1 files changed, 10 insertions(+), 1 deletions(-) > > diff --git a/sequencer.c b/sequencer.c > index f83cdfd..ad4d781 100644 > --- a/sequencer.c > +++ b/sequencer.c > @@ -261,7 +261,16 @@ static int is_index_unchanged(void) > return error(_("Could not resolve HEAD commit\n")); > > head_commit = lookup_commit(head_sha1); > - if (!head_commit || parse_commit(head_commit)) > + > + /* > + * If head_commit is NULL, just return, as check_commit, > + * called from lookup_commit, would have indicated that > + * head_commit is not a commit object already. > + */ > + if (!head_commit) > + return; A return value is missing. Perhaps -1? > + > + if (parse_commit(head_commit)) > return error(_("could not parse commit %s\n"), > sha1_to_hex(head_commit->object.sha1)); Note: parse_commit() can handle NULL, and it already reports error details itself. René ``` ## Neil Horman, 2012-05-03 12:08 Subject: Re: [PATCH] git cherry-pick: Add NULL check to sequencer parsing of HEAD Message-ID: <20120503120857.GA3085@hmsreliant.think-freely.org> URL: https://gitlist.dev/e/20120503120857.GA3085%40hmsreliant.think-freely.org In-Reply-To: <4FA26FF2.2050607@lsrfire.ath.cx> ``` On Thu, May 03, 2012 at 01:45:54PM +0200, René Scharfe wrote: > Am 03.05.2012 13:20, schrieb Neil Horman: > >Michael Mueller noted that a feature I recently added failed to check the return > >of lookup_commit to ensure that it was not NULL. I don't think a NULL can > >actually happen in the this particular use case, but regardless it seems a good > >idea to check. > > > >Signed-off-by: Neil Horman > >--- > > sequencer.c | 11 ++++++++++- > > 1 files changed, 10 insertions(+), 1 deletions(-) > > > >diff --git a/sequencer.c b/sequencer.c > >index f83cdfd..ad4d781 100644 > >--- a/sequencer.c > >+++ b/sequencer.c > >@@ -261,7 +261,16 @@ static int is_index_unchanged(void) > > return error(_("Could not resolve HEAD commit\n")); > > > > head_commit = lookup_commit(head_sha1); > >- if (!head_commit || parse_commit(head_commit)) > >+ > >+ /* > >+ * If head_commit is NULL, just return, as check_commit, > >+ * called from lookup_commit, would have indicated that > >+ * head_commit is not a commit object already. > >+ */ > >+ if (!head_commit) > >+ return; > > A return value is missing. Perhaps -1? > Yeah, sorry, not sure how I missed the compiler warning. > >+ > >+ if (parse_commit(head_commit)) > > return error(_("could not parse commit %s\n"), > > sha1_to_hex(head_commit->object.sha1)); > > Note: parse_commit() can handle NULL, and it already reports error > details itself. No, it doesn't. parse_commit checks NULL already, true, but it just returns -1. No error message is provided to the user Neil > > René > ``` ## Neil Horman, 2012-05-03 12:10 Subject: [PATCH v2] git cherry-pick: Add NULL check to sequencer parsing of HEAD Message-ID: <1336047022-3194-1-git-send-email-nhorman@tuxdriver.com> URL: https://gitlist.dev/e/1336047022-3194-1-git-send-email-nhorman%40tuxdriver.com In-Reply-To: <1336044026-16897-1-git-send-email-nhorman@tuxdriver.com> ``` Michael Mueller noted that a feature I recently added failed to check the return of lookup_commit to ensure that it was not NULL. I don't think a NULL can actually happen in the this particular use case, but regardless it seems a good idea to check. Signed-off-by: Neil Horman --- sequencer.c | 11 ++++++++++- 1 files changed, 10 insertions(+), 1 deletions(-) diff --git a/sequencer.c b/sequencer.c index f83cdfd..ded0b76 100644 --- a/sequencer.c +++ b/sequencer.c @@ -261,7 +261,16 @@ static int is_index_unchanged(void) return error(_("Could not resolve HEAD commit\n")); head_commit = lookup_commit(head_sha1); - if (!head_commit || parse_commit(head_commit)) + + /* + * If head_commit is NULL, just return, as check_commit, + * called from lookup_commit, would have indicated that + * head_commit is not a commit object already. + */ + if (!head_commit) + return -1; + + if (parse_commit(head_commit)) return error(_("could not parse commit %s\n"), sha1_to_hex(head_commit->object.sha1)); -- 1.7.7.6 ``` ## René Scharfe, 2012-05-03 12:26 Subject: Re: [PATCH] git cherry-pick: Add NULL check to sequencer parsing of HEAD Message-ID: <4FA27986.7010502@lsrfire.ath.cx> URL: https://gitlist.dev/e/4FA27986.7010502%40lsrfire.ath.cx In-Reply-To: <20120503120857.GA3085@hmsreliant.think-freely.org> ``` Am 03.05.2012 14:08, schrieb Neil Horman: >>> + if (parse_commit(head_commit)) >>> return error(_("could not parse commit %s\n"), >>> sha1_to_hex(head_commit->object.sha1)); >> >> Note: parse_commit() can handle NULL, and it already reports error >> details itself. > No, it doesn't. parse_commit checks NULL already, true, but it just returns -1. > No error message is provided to the user Sorry, was too terse again: It handles NULL, without printing anything. And it reports details for (other) errors. So you could return -1 if parse_commit() returns non-zero and be done with it. Just saying. René ``` ## Neil Horman, 2012-05-03 14:09 Subject: [PATCH v3] git cherry-pick: Add NULL check to sequencer parsing of HEAD Message-ID: <1336054159-5123-1-git-send-email-nhorman@tuxdriver.com> URL: https://gitlist.dev/e/1336054159-5123-1-git-send-email-nhorman%40tuxdriver.com In-Reply-To: <1336044026-16897-1-git-send-email-nhorman@tuxdriver.com> ``` Michael Mueller noted that a feature I recently added failed to check the return of lookup_commit to ensure that it was not NULL. I don't think a NULL can actually happen in the this particular use case, but regardless it seems a good idea to check. Signed-off-by: Neil Horman --- sequencer.c | 6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-) diff --git a/sequencer.c b/sequencer.c index f83cdfd..f7eac1d 100644 --- a/sequencer.c +++ b/sequencer.c @@ -261,9 +261,9 @@ static int is_index_unchanged(void) return error(_("Could not resolve HEAD commit\n")); head_commit = lookup_commit(head_sha1); - if (!head_commit || parse_commit(head_commit)) - return error(_("could not parse commit %s\n"), - sha1_to_hex(head_commit->object.sha1)); + + if (parse_commit(head_commit)) + return -1; if (!active_cache_tree) active_cache_tree = cache_tree(); -- 1.7.7.6 ``` ## Matthieu Moy, 2012-05-03 16:48 Subject: Re: [PATCH v3] git cherry-pick: Add NULL check to sequencer parsing of HEAD Message-ID: URL: https://gitlist.dev/e/vpqtxzx5hxz.fsf%40bauges.imag.fr In-Reply-To: <1336054159-5123-1-git-send-email-nhorman@tuxdriver.com> ``` Neil Horman writes: > - if (!head_commit || parse_commit(head_commit)) > - return error(_("could not parse commit %s\n"), > - sha1_to_hex(head_commit->object.sha1)); > + > + if (parse_commit(head_commit)) > + return -1; Why did you replace the error("...") with only a -1? error() also returns -1, but displays a message before, which I think was fine. If you want to remove the message, then explain why in the commit message. If you do not test for head_commit to be null, you can't use it in the error message. But from the context, it seems you can use head_sha1. If not, a message like "Could not parse HEAD commit" seems better than nothing. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ ``` ## Junio C Hamano, 2012-05-03 17:13 Subject: Re: [PATCH v3] git cherry-pick: Add NULL check to sequencer parsing of HEAD Message-ID: <7vobq5fasa.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vobq5fasa.fsf%40alter.siamese.dyndns.org In-Reply-To: ``` Matthieu Moy writes: > Neil Horman writes: > >> - if (!head_commit || parse_commit(head_commit)) >> - return error(_("could not parse commit %s\n"), >> - sha1_to_hex(head_commit->object.sha1)); >> + >> + if (parse_commit(head_commit)) >> + return -1; > > Why did you replace the error("...") with only a -1? error() also > returns -1, but displays a message before, which I think was fine. If > you want to remove the message, then explain why in the commit message. > > If you do not test for head_commit to be null, you can't use it in the > error message. But from the context, it seems you can use head_sha1. If > not, a message like "Could not parse HEAD commit" seems better than > nothing. Yeah, I think v2 in this series is the appropriate fix. ``` ## Neil Horman, 2012-05-03 17:34 Subject: Re: [PATCH v3] git cherry-pick: Add NULL check to sequencer parsing of HEAD Message-ID: <20120503173443.GA2957@neilslaptop.think-freely.org> URL: https://gitlist.dev/e/20120503173443.GA2957%40neilslaptop.think-freely.org In-Reply-To: <7vsjfhfbko.fsf@alter.siamese.dyndns.org> ``` On Thu, May 03, 2012 at 09:56:07AM -0700, Junio C Hamano wrote: > Neil Horman writes: > > > Michael Mueller noted that a feature I recently added failed to check the return > > of lookup_commit to ensure that it was not NULL. I don't think a NULL can > > actually happen in the this particular use case, but regardless it seems a good > > idea to check. > > > > Signed-off-by: Neil Horman > > Make a mental note here to remember what we just read above: Earlier code > was missing a check for NULL and the patch should be about adding a new > check. > > > sequencer.c | 6 +++--- > > 1 files changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/sequencer.c b/sequencer.c > > index f83cdfd..f7eac1d 100644 > > --- a/sequencer.c > > +++ b/sequencer.c > > @@ -261,9 +261,9 @@ static int is_index_unchanged(void) > > return error(_("Could not resolve HEAD commit\n")); > > > > head_commit = lookup_commit(head_sha1); > > - if (!head_commit || parse_commit(head_commit)) > > - return error(_("could not parse commit %s\n"), > > - sha1_to_hex(head_commit->object.sha1)); > > + > > + if (parse_commit(head_commit)) > > + return -1; > > Whoa? This patch is not about adding any new check. It removes > conditions from if clause and removes an error message. > > What does that mean? 6 months down the road, when you read this commit, > you will be very confused. The resulting code may be correct, but the > explanation is way off. Perhaps explain it like the attached? > > Having said that, if you had HEAD that is corrupt (perhaps filesystem > corruption), you *WILL* get NULL in head_commit, and with the updated code > you won't issue any error message from parse_commit(), so I do not think > the patched result is entirely correct. > See check_commit, as called from lookup_commit, it issues a user visible error message as part of its parsing. > -- >8 -- > Subject: [PATCH] git cherry-pick: remove bogus error message generation > > The code to issue an error message tried to access the pointer head_commit > that is potentially NULL. Just calling parse_commit() will give us the > necessary "is the commit object valid?" check and issue an error message, > so we do not need an error message here. > This seems reasonable to me Neil ```