On Thu, May 03, 2012 at 09:56:07AM -0700, Junio C Hamano wrote:
> Neil Horman <nhorman@xxxxxxxxxxxxx> 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 <nhorman@xxxxxxxxxxxxx>
>
> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html