* [PATCH] git cherry-pick: Add NULL check to sequencer parsing of HEAD
@ 2012-05-03 11:20 Neil Horman
2012-05-03 11:45 ` René Scharfe
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Neil Horman @ 2012-05-03 11:20 UTC (permalink / raw)
To: git; +Cc: mmueller, rene.scharfe, Matthieu.Moy, gitster, 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;
+
+ if (parse_commit(head_commit))
return error(_("could not parse commit %s\n"),
sha1_to_hex(head_commit->object.sha1));
--
1.7.7.6
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] git cherry-pick: Add NULL check to sequencer parsing of HEAD
2012-05-03 11:20 [PATCH] git cherry-pick: Add NULL check to sequencer parsing of HEAD Neil Horman
@ 2012-05-03 11:45 ` René Scharfe
2012-05-03 12:08 ` Neil Horman
2012-05-03 12:10 ` [PATCH v2] " Neil Horman
2012-05-03 14:09 ` [PATCH v3] " Neil Horman
2 siblings, 1 reply; 9+ messages in thread
From: René Scharfe @ 2012-05-03 11:45 UTC (permalink / raw)
To: Neil Horman; +Cc: git, mmueller, Matthieu.Moy, gitster
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.
René
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] git cherry-pick: Add NULL check to sequencer parsing of HEAD
2012-05-03 11:45 ` René Scharfe
@ 2012-05-03 12:08 ` Neil Horman
2012-05-03 12:26 ` René Scharfe
0 siblings, 1 reply; 9+ messages in thread
From: Neil Horman @ 2012-05-03 12:08 UTC (permalink / raw)
To: René Scharfe; +Cc: git, mmueller, Matthieu.Moy, gitster
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 <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?
>
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é
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2] git cherry-pick: Add NULL check to sequencer parsing of HEAD
2012-05-03 11:20 [PATCH] git cherry-pick: Add NULL check to sequencer parsing of HEAD Neil Horman
2012-05-03 11:45 ` René Scharfe
@ 2012-05-03 12:10 ` Neil Horman
2012-05-03 14:09 ` [PATCH v3] " Neil Horman
2 siblings, 0 replies; 9+ messages in thread
From: Neil Horman @ 2012-05-03 12:10 UTC (permalink / raw)
To: git; +Cc: mmueller, rene.scharfe, Matthieu.Moy, gitster, 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..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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] git cherry-pick: Add NULL check to sequencer parsing of HEAD
2012-05-03 12:08 ` Neil Horman
@ 2012-05-03 12:26 ` René Scharfe
0 siblings, 0 replies; 9+ messages in thread
From: René Scharfe @ 2012-05-03 12:26 UTC (permalink / raw)
To: Neil Horman; +Cc: git, mmueller, Matthieu.Moy, gitster
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é
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v3] git cherry-pick: Add NULL check to sequencer parsing of HEAD
2012-05-03 11:20 [PATCH] git cherry-pick: Add NULL check to sequencer parsing of HEAD Neil Horman
2012-05-03 11:45 ` René Scharfe
2012-05-03 12:10 ` [PATCH v2] " Neil Horman
@ 2012-05-03 14:09 ` Neil Horman
2012-05-03 16:48 ` Matthieu Moy
[not found] ` <7vsjfhfbko.fsf@alter.siamese.dyndns.org>
2 siblings, 2 replies; 9+ messages in thread
From: Neil Horman @ 2012-05-03 14:09 UTC (permalink / raw)
To: git; +Cc: mmueller, rene.scharfe, Matthieu.Moy, gitster, 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 | 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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v3] git cherry-pick: Add NULL check to sequencer parsing of HEAD
2012-05-03 14:09 ` [PATCH v3] " Neil Horman
@ 2012-05-03 16:48 ` Matthieu Moy
2012-05-03 17:13 ` Junio C Hamano
[not found] ` <7vsjfhfbko.fsf@alter.siamese.dyndns.org>
1 sibling, 1 reply; 9+ messages in thread
From: Matthieu Moy @ 2012-05-03 16:48 UTC (permalink / raw)
To: Neil Horman; +Cc: git, mmueller, rene.scharfe, gitster
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.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3] git cherry-pick: Add NULL check to sequencer parsing of HEAD
2012-05-03 16:48 ` Matthieu Moy
@ 2012-05-03 17:13 ` Junio C Hamano
0 siblings, 0 replies; 9+ messages in thread
From: Junio C Hamano @ 2012-05-03 17:13 UTC (permalink / raw)
To: Matthieu Moy; +Cc: Neil Horman, git, mmueller, rene.scharfe
Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
> 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.
Yeah, I think v2 in this series is the appropriate fix.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3] git cherry-pick: Add NULL check to sequencer parsing of HEAD
[not found] ` <7vsjfhfbko.fsf@alter.siamese.dyndns.org>
@ 2012-05-03 17:34 ` Neil Horman
0 siblings, 0 replies; 9+ messages in thread
From: Neil Horman @ 2012-05-03 17:34 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, mmueller, rene.scharfe, Matthieu.Moy
On Thu, May 03, 2012 at 09:56:07AM -0700, Junio C Hamano wrote:
> 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.
>
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
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2012-05-03 17:35 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-05-03 11:20 [PATCH] git cherry-pick: Add NULL check to sequencer parsing of HEAD Neil Horman
2012-05-03 11:45 ` René Scharfe
2012-05-03 12:08 ` Neil Horman
2012-05-03 12:26 ` René Scharfe
2012-05-03 12:10 ` [PATCH v2] " Neil Horman
2012-05-03 14:09 ` [PATCH v3] " Neil Horman
2012-05-03 16:48 ` Matthieu Moy
2012-05-03 17:13 ` Junio C Hamano
[not found] ` <7vsjfhfbko.fsf@alter.siamese.dyndns.org>
2012-05-03 17:34 ` Neil Horman
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.