> > } else if (use_message) {
> > buffer = strstr(use_message_buffer, "\n\n");
> > - if (!buffer || buffer[2] == '\0')
> > + if (!amend && !edit_message && (!buffer || buffer[2] == '\0'))
> > die(_("commit has empty message"));
>
> Hmm. So "buffer" used to never be NULL (because we would die if it is),
> and now we might not die if we are doing an amend, no? And the next line
> is:
>
> > strbuf_add(&sb, buffer + 2, strlen(buffer + 2));
>
> Doesn't this need to handle the case of NULL buffer (i.e., when it does
> not already have "\n\n" in it)?

I wrote that after looking at just your patch. Looking at
builtin/commit.c, I think use_message_buffer will always be a re-encoded
commit object. So that strstr should _never_ fail unless the commit
object is corrupt. So the right thing is probably:

This is the more drastic approach compared to 2a. Since the code in
pretty.c was broken given such input, I would favor this. It does,
however, remove the possibility of fixing up such a broken commit with
the tools git provides (other than resorting to git-hash-object and
git-replace).

Re: [RFC PATCH 2a] pretty: detect missing \n\n in commit message

On Tue, Feb 28, 2012 at 5:37 PM, Thomas Rast <[hidden email]> wrote:
> get_header()'s exit condition is finding the \n\n that separates the
> commit header from its message. If such a double newline is not
> present, it segfaults. Catch this case and die().

I'd prefer a gentler approach: if there's no \n\n, accept that this
commit has no body. I think I encountered such commits in the test
suite when I tried to convert commit encoding partially and made the
assumption that \n\n must exist.
--
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]More majordomo info at http://vger.kernel.org/majordomo-info.html

Your experiment with hash-object aside (that is like saying "I can write
garbage with a disk editor, and now OS cannot read from that directory"),
if somebody manages to create a commit without any body, it is clear that
the user wanted to record no body. I think all code that tries to run
strstr("\n\n") and increment the resulting pointer by two to find the
beginning of the body should behave as if it found one and the result
pointed at a NUL. Rejecting with fsck does not help anybody, as it
happens after the fact.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]More majordomo info at http://vger.kernel.org/majordomo-info.html

> Thomas Rast <[hidden email]> writes:
>
> > So either there's a lot to be fixed, or fsck needs to catch this.
>
> Your experiment with hash-object aside (that is like saying "I can write
> garbage with a disk editor, and now OS cannot read from that directory"),

Yes, but the difference between "OS cannot read from that directory" and
"OS segfaults" might be worth noticing. :)

> if somebody manages to create a commit without any body, it is clear that
> the user wanted to record no body. I think all code that tries to run
> strstr("\n\n") and increment the resulting pointer by two to find the
> beginning of the body should behave as if it found one and the result
> pointed at a NUL. Rejecting with fsck does not help anybody, as it
> happens after the fact.

Yeah, I agree that treating it like an empty body is reasonable
(possibly with a warning). But given that nobody has actually seen this
in the wild, maybe it is simpler to mark it with fsck, and to just die()
when we see it. That would hopefully alert the author of the broken tool
early, before the tools is made public. If it turns out that such
commits do end up in the wild, then we can relax the behavior then.

> Yeah, I agree that treating it like an empty body is reasonable
> (possibly with a warning). But given that nobody has actually seen this
> in the wild, maybe it is simpler to mark it with fsck, and to just die()
> when we see it. That would hopefully alert the author of the broken tool
> early, before the tools is made public. If it turns out that such
> commits do end up in the wild, then we can relax the behavior then.

Yeah, it is not like we would want to encourage a commit with empty body,
be it preceded with "\n\n" or just a "\n", in the first place.

We would need to locate all the places that expect that strstr("\n\n")
will find something, and add die("commit made with a broken git") at these
places anyway, in addition to the change to fsck. Given that, I suspect
that the extra amount of the work needed to tweak the code to tolerate
such a commit and keep going might not be so big, and going that route
would avoid punishing the users of broken versions of git (or broken
imitations of git, for that matter), so...
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]More majordomo info at http://vger.kernel.org/majordomo-info.html