Re: [PATCH v3] git cherry-pick: Add NULL check to sequencer parsing of HEAD

On Thu, May 03, 2012 at 09:56:07AM -0700, Junio C Hamano wrote:

Neil Horman [off-list ref] writes:

quoted

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 <nhorman@tuxdriver.com>

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.

quoted

 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