patchgit cherry-pick: Add NULL check to sequencer parsing of HEAD

> 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>
> ---
>   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;

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

>>> +	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

> -	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;

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

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

> -- >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.
>