{"thread":{"id":"30404","subject":"[PATCH] git cherry-pick: Add NULL check to sequencer parsing of HEAD","startedAt":"2012-05-03T11:20:26Z","lastAt":"2012-05-03T17:34:44Z","messageCount":9,"participants":["Neil Horman","René Scharfe","Matthieu Moy","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"190632","messageId":"1336044026-16897-1-git-send-email-nhorman@tuxdriver.com","threadId":"30404","inReplyTo":null,"subject":"[PATCH] git cherry-pick: Add NULL check to sequencer parsing of HEAD","fromName":"Neil Horman","fromEmail":"nhorman@tuxdriver.com","sentAt":"2012-05-03T11:20:26Z","receivedAt":"2012-05-03T11:20:26Z","isPatch":true,"sender":{"key":"nhorman@tuxdriver.com","avatar":"https://avatars.githubusercontent.com/u/1032926?v=4"},"body":"Michael Mueller noted that a feature I recently added failed to check the return\nof lookup_commit to ensure that it was not NULL.  I don't think a NULL can\nactually happen in the this particular use case, but regardless it seems a good\nidea to check.\n\nSigned-off-by: Neil Horman <nhorman@tuxdriver.com>\n---\n sequencer.c |   11 ++++++++++-\n 1 files changed, 10 insertions(+), 1 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex f83cdfd..ad4d781 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -261,7 +261,16 @@ static int is_index_unchanged(void)\n \t\treturn error(_(\"Could not resolve HEAD commit\\n\"));\n \n \thead_commit = lookup_commit(head_sha1);\n-\tif (!head_commit || parse_commit(head_commit))\n+\n+\t/*\n+\t * If head_commit is NULL, just return, as check_commit,\n+\t * called from lookup_commit, would have indicated that\n+\t * head_commit is not a commit object already.\n+\t */\n+\tif (!head_commit)\n+\t\treturn;\n+\n+\tif (parse_commit(head_commit))\n \t\treturn error(_(\"could not parse commit %s\\n\"),\n \t\t\t     sha1_to_hex(head_commit->object.sha1));\n \n-- \n1.7.7.6\n"},{"id":"190633","messageId":"4FA26FF2.2050607@lsrfire.ath.cx","threadId":"30404","inReplyTo":"1336044026-16897-1-git-send-email-nhorman@tuxdriver.com","subject":"Re: [PATCH] git cherry-pick: Add NULL check to sequencer parsing of HEAD","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2012-05-03T11:45:54Z","receivedAt":"2012-05-03T11:45:54Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 03.05.2012 13:20, schrieb Neil Horman:\n> Michael Mueller noted that a feature I recently added failed to check the return\n> of lookup_commit to ensure that it was not NULL.  I don't think a NULL can\n> actually happen in the this particular use case, but regardless it seems a good\n> idea to check.\n>\n> Signed-off-by: Neil Horman <nhorman@tuxdriver.com>\n> ---\n>   sequencer.c |   11 ++++++++++-\n>   1 files changed, 10 insertions(+), 1 deletions(-)\n>\n> diff --git a/sequencer.c b/sequencer.c\n> index f83cdfd..ad4d781 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -261,7 +261,16 @@ static int is_index_unchanged(void)\n>   \t\treturn error(_(\"Could not resolve HEAD commit\\n\"));\n>\n>   \thead_commit = lookup_commit(head_sha1);\n> -\tif (!head_commit || parse_commit(head_commit))\n> +\n> +\t/*\n> +\t * If head_commit is NULL, just return, as check_commit,\n> +\t * called from lookup_commit, would have indicated that\n> +\t * head_commit is not a commit object already.\n> +\t */\n> +\tif (!head_commit)\n> +\t\treturn;\n\nA return value is missing.  Perhaps -1?\n\n> +\n> +\tif (parse_commit(head_commit))\n>   \t\treturn error(_(\"could not parse commit %s\\n\"),\n>   \t\t\t     sha1_to_hex(head_commit->object.sha1));\n\nNote: parse_commit() can handle NULL, and it already reports error \ndetails itself.\n\nRené\n"},{"id":"190636","messageId":"20120503120857.GA3085@hmsreliant.think-freely.org","threadId":"30404","inReplyTo":"4FA26FF2.2050607@lsrfire.ath.cx","subject":"Re: [PATCH] git cherry-pick: Add NULL check to sequencer parsing of HEAD","fromName":"Neil Horman","fromEmail":"nhorman@tuxdriver.com","sentAt":"2012-05-03T12:08:57Z","receivedAt":"2012-05-03T12:08:57Z","isPatch":true,"sender":{"key":"nhorman@tuxdriver.com","avatar":"https://avatars.githubusercontent.com/u/1032926?v=4"},"body":"On Thu, May 03, 2012 at 01:45:54PM +0200, René Scharfe wrote:\n> Am 03.05.2012 13:20, schrieb Neil Horman:\n> >Michael Mueller noted that a feature I recently added failed to check the return\n> >of lookup_commit to ensure that it was not NULL.  I don't think a NULL can\n> >actually happen in the this particular use case, but regardless it seems a good\n> >idea to check.\n> >\n> >Signed-off-by: Neil Horman <nhorman@tuxdriver.com>\n> >---\n> >  sequencer.c |   11 ++++++++++-\n> >  1 files changed, 10 insertions(+), 1 deletions(-)\n> >\n> >diff --git a/sequencer.c b/sequencer.c\n> >index f83cdfd..ad4d781 100644\n> >--- a/sequencer.c\n> >+++ b/sequencer.c\n> >@@ -261,7 +261,16 @@ static int is_index_unchanged(void)\n> >  \t\treturn error(_(\"Could not resolve HEAD commit\\n\"));\n> >\n> >  \thead_commit = lookup_commit(head_sha1);\n> >-\tif (!head_commit || parse_commit(head_commit))\n> >+\n> >+\t/*\n> >+\t * If head_commit is NULL, just return, as check_commit,\n> >+\t * called from lookup_commit, would have indicated that\n> >+\t * head_commit is not a commit object already.\n> >+\t */\n> >+\tif (!head_commit)\n> >+\t\treturn;\n> \n> A return value is missing.  Perhaps -1?\n> \nYeah, sorry, not sure how I missed the compiler warning.\n\n> >+\n> >+\tif (parse_commit(head_commit))\n> >  \t\treturn error(_(\"could not parse commit %s\\n\"),\n> >  \t\t\t     sha1_to_hex(head_commit->object.sha1));\n> \n> Note: parse_commit() can handle NULL, and it already reports error\n> details itself.\nNo, it doesn't.  parse_commit checks NULL already, true, but it just returns -1.\nNo error message is provided to the user\nNeil\n\n> \n> René\n> \n"},{"id":"190637","messageId":"1336047022-3194-1-git-send-email-nhorman@tuxdriver.com","threadId":"30404","inReplyTo":"1336044026-16897-1-git-send-email-nhorman@tuxdriver.com","subject":"[PATCH v2] git cherry-pick: Add NULL check to sequencer parsing of HEAD","fromName":"Neil Horman","fromEmail":"nhorman@tuxdriver.com","sentAt":"2012-05-03T12:10:22Z","receivedAt":"2012-05-03T12:10:22Z","isPatch":true,"sender":{"key":"nhorman@tuxdriver.com","avatar":"https://avatars.githubusercontent.com/u/1032926?v=4"},"body":"Michael Mueller noted that a feature I recently added failed to check the return\nof lookup_commit to ensure that it was not NULL.  I don't think a NULL can\nactually happen in the this particular use case, but regardless it seems a good\nidea to check.\n\nSigned-off-by: Neil Horman <nhorman@tuxdriver.com>\n---\n sequencer.c |   11 ++++++++++-\n 1 files changed, 10 insertions(+), 1 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex f83cdfd..ded0b76 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -261,7 +261,16 @@ static int is_index_unchanged(void)\n \t\treturn error(_(\"Could not resolve HEAD commit\\n\"));\n \n \thead_commit = lookup_commit(head_sha1);\n-\tif (!head_commit || parse_commit(head_commit))\n+\n+\t/*\n+\t * If head_commit is NULL, just return, as check_commit,\n+\t * called from lookup_commit, would have indicated that\n+\t * head_commit is not a commit object already.\n+\t */\n+\tif (!head_commit)\n+\t\treturn -1;\n+\n+\tif (parse_commit(head_commit))\n \t\treturn error(_(\"could not parse commit %s\\n\"),\n \t\t\t     sha1_to_hex(head_commit->object.sha1));\n \n-- \n1.7.7.6\n"},{"id":"190639","messageId":"4FA27986.7010502@lsrfire.ath.cx","threadId":"30404","inReplyTo":"20120503120857.GA3085@hmsreliant.think-freely.org","subject":"Re: [PATCH] git cherry-pick: Add NULL check to sequencer parsing of HEAD","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2012-05-03T12:26:46Z","receivedAt":"2012-05-03T12:26:46Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 03.05.2012 14:08, schrieb Neil Horman:\n>>> +\tif (parse_commit(head_commit))\n>>>   \t\treturn error(_(\"could not parse commit %s\\n\"),\n>>>   \t\t\t     sha1_to_hex(head_commit->object.sha1));\n>>\n>> Note: parse_commit() can handle NULL, and it already reports error\n>> details itself.\n> No, it doesn't.  parse_commit checks NULL already, true, but it just returns -1.\n> No error message is provided to the user\n\nSorry, was too terse again: It handles NULL, without printing anything. \n  And it reports details for (other) errors.  So you could return -1 if \nparse_commit() returns non-zero and be done with it.  Just saying.\n\nRené\n"},{"id":"190643","messageId":"1336054159-5123-1-git-send-email-nhorman@tuxdriver.com","threadId":"30404","inReplyTo":"1336044026-16897-1-git-send-email-nhorman@tuxdriver.com","subject":"[PATCH v3] git cherry-pick: Add NULL check to sequencer parsing of HEAD","fromName":"Neil Horman","fromEmail":"nhorman@tuxdriver.com","sentAt":"2012-05-03T14:09:19Z","receivedAt":"2012-05-03T14:09:19Z","isPatch":true,"sender":{"key":"nhorman@tuxdriver.com","avatar":"https://avatars.githubusercontent.com/u/1032926?v=4"},"body":"Michael Mueller noted that a feature I recently added failed to check the return\nof lookup_commit to ensure that it was not NULL.  I don't think a NULL can\nactually happen in the this particular use case, but regardless it seems a good\nidea to check.\n\nSigned-off-by: Neil Horman <nhorman@tuxdriver.com>\n---\n sequencer.c |    6 +++---\n 1 files changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex f83cdfd..f7eac1d 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -261,9 +261,9 @@ static int is_index_unchanged(void)\n \t\treturn error(_(\"Could not resolve HEAD commit\\n\"));\n \n \thead_commit = lookup_commit(head_sha1);\n-\tif (!head_commit || parse_commit(head_commit))\n-\t\treturn error(_(\"could not parse commit %s\\n\"),\n-\t\t\t     sha1_to_hex(head_commit->object.sha1));\n+\n+\tif (parse_commit(head_commit))\n+\t\treturn -1;\n \n \tif (!active_cache_tree)\n \t\tactive_cache_tree = cache_tree();\n-- \n1.7.7.6\n"},{"id":"190655","messageId":"vpqtxzx5hxz.fsf@bauges.imag.fr","threadId":"30404","inReplyTo":"1336054159-5123-1-git-send-email-nhorman@tuxdriver.com","subject":"Re: [PATCH v3] git cherry-pick: Add NULL check to sequencer parsing of HEAD","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2012-05-03T16:48:40Z","receivedAt":"2012-05-03T16:48:40Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Neil Horman <nhorman@tuxdriver.com> writes:\n\n> -\tif (!head_commit || parse_commit(head_commit))\n> -\t\treturn error(_(\"could not parse commit %s\\n\"),\n> -\t\t\t     sha1_to_hex(head_commit->object.sha1));\n> +\n> +\tif (parse_commit(head_commit))\n> +\t\treturn -1;\n\nWhy did you replace the error(\"...\") with only a -1? error() also\nreturns -1, but displays a message before, which I think was fine. If\nyou want to remove the message, then explain why in the commit message.\n\nIf you do not test for head_commit to be null, you can't use it in the\nerror message. But from the context, it seems you can use head_sha1. If\nnot, a message like \"Could not parse HEAD commit\" seems better than\nnothing.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"190657","messageId":"7vobq5fasa.fsf@alter.siamese.dyndns.org","threadId":"30404","inReplyTo":"vpqtxzx5hxz.fsf@bauges.imag.fr","subject":"Re: [PATCH v3] git cherry-pick: Add NULL check to sequencer parsing of HEAD","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-05-03T17:13:09Z","receivedAt":"2012-05-03T17:13:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:\n\n> Neil Horman <nhorman@tuxdriver.com> writes:\n>\n>> -\tif (!head_commit || parse_commit(head_commit))\n>> -\t\treturn error(_(\"could not parse commit %s\\n\"),\n>> -\t\t\t     sha1_to_hex(head_commit->object.sha1));\n>> +\n>> +\tif (parse_commit(head_commit))\n>> +\t\treturn -1;\n>\n> Why did you replace the error(\"...\") with only a -1? error() also\n> returns -1, but displays a message before, which I think was fine. If\n> you want to remove the message, then explain why in the commit message.\n>\n> If you do not test for head_commit to be null, you can't use it in the\n> error message. But from the context, it seems you can use head_sha1. If\n> not, a message like \"Could not parse HEAD commit\" seems better than\n> nothing.\n\nYeah, I think v2 in this series is the appropriate fix.\n"},{"id":"190659","messageId":"20120503173443.GA2957@neilslaptop.think-freely.org","threadId":"30404","inReplyTo":"7vsjfhfbko.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v3] git cherry-pick: Add NULL check to sequencer parsing of HEAD","fromName":"Neil Horman","fromEmail":"nhorman@tuxdriver.com","sentAt":"2012-05-03T17:34:44Z","receivedAt":"2012-05-03T17:34:44Z","isPatch":true,"sender":{"key":"nhorman@tuxdriver.com","avatar":"https://avatars.githubusercontent.com/u/1032926?v=4"},"body":"On Thu, May 03, 2012 at 09:56:07AM -0700, Junio C Hamano wrote:\n> Neil Horman <nhorman@tuxdriver.com> writes:\n> \n> > Michael Mueller noted that a feature I recently added failed to check the return\n> > of lookup_commit to ensure that it was not NULL.  I don't think a NULL can\n> > actually happen in the this particular use case, but regardless it seems a good\n> > idea to check.\n> >\n> > Signed-off-by: Neil Horman <nhorman@tuxdriver.com>\n> \n> Make a mental note here to remember what we just read above: Earlier code\n> was missing a check for NULL and the patch should be about adding a new\n> check.\n> \n> >  sequencer.c |    6 +++---\n> >  1 files changed, 3 insertions(+), 3 deletions(-)\n> >\n> > diff --git a/sequencer.c b/sequencer.c\n> > index f83cdfd..f7eac1d 100644\n> > --- a/sequencer.c\n> > +++ b/sequencer.c\n> > @@ -261,9 +261,9 @@ static int is_index_unchanged(void)\n> >  \t\treturn error(_(\"Could not resolve HEAD commit\\n\"));\n> >  \n> >  \thead_commit = lookup_commit(head_sha1);\n> > -\tif (!head_commit || parse_commit(head_commit))\n> > -\t\treturn error(_(\"could not parse commit %s\\n\"),\n> > -\t\t\t     sha1_to_hex(head_commit->object.sha1));\n> > +\n> > +\tif (parse_commit(head_commit))\n> > +\t\treturn -1;\n> \n> Whoa?  This patch is not about adding any new check.  It removes\n> conditions from if clause and removes an error message.\n> \n> What does that mean?  6 months down the road, when you read this commit,\n> you will be very confused.  The resulting code may be correct, but the\n> explanation is way off.  Perhaps explain it like the attached?\n> \n> Having said that, if you had HEAD that is corrupt (perhaps filesystem\n> corruption), you *WILL* get NULL in head_commit, and with the updated code\n> you won't issue any error message from parse_commit(), so I do not think\n> the patched result is entirely correct.\n> \nSee check_commit, as called from lookup_commit, it issues a user visible error\nmessage as part of its parsing.\n\n> -- >8 --\n> Subject: [PATCH] git cherry-pick: remove bogus error message generation\n> \n> The code to issue an error message tried to access the pointer head_commit\n> that is potentially NULL.  Just calling parse_commit() will give us the\n> necessary \"is the commit object valid?\" check and issue an error message,\n> so we do not need an error message here.\n> \nThis seems reasonable to me\nNeil\n"}]}