Re: [PATCH] fsck: it is OK for a tag and a commit to lack the body
Hi Junio, On 2015-06-29 07:42, Junio C Hamano wrote: Johannes Schindelin johannes.schinde...@gmx.de writes: Hmm. Maybe we should still warn when there is no empty line finishing the header explicitly, or at least make it FSCK_IGNORE by default so that maintainers who like a stricter check can upgrade the condition to an error? [...] And such a check can certainly be added in the future True. I take my suggestion back. Thanks for the reality check, Dscho -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] fsck: it is OK for a tag and a commit to lack the body
On Sun, Jun 28, 2015 at 2:18 PM, Junio C Hamano gits...@pobox.com wrote: When fsck validates a commit or a tag object, it scans each line in the header the object using helper functions such as start_with(), s/header/ of/ etc. that work on a NUL terminated buffer, but before a1e920a0 (index-pack: terminate object buffers with NUL, 2014-12-08), the validation functions were fed the object data as counted strings, not necessarily terminated with a NUL. We added a helper function require_end_of_header() to be called at the beginning of these validation functions to insist that the object data contains an empty line before its end. The theory is that the validating functions will notice and stop when it hits an empty line as a normal end of header (or a required header line that is missing) before scanning past the end of potentially not NUL-terminated buffer. But the theory forgot that in the older days, Git itself happily created objects with only the header lines without a body. This caused Git 2.2 and later to issue an unnecessary warning on some existing repositories. With a1e920a0, we do not need to require an empty line (or the body) in these objects to safely parse and validate them. Drop the offending must have an empty line check from this helper function, while keeping the other check to make sure that there is no NUL in the header part of the object, and adjust the name of the helper to what it does accordingly. Noticed-by: Wolfgang Denk w...@denx.de Helped-by: Jeff King p...@peff.net Signed-off-by: Junio C Hamano gits...@pobox.com -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] fsck: it is OK for a tag and a commit to lack the body
When fsck validates a commit or a tag object, it scans each line in
the header the object using helper functions such as start_with(),
etc. that work on a NUL terminated buffer, but before a1e920a0
(index-pack: terminate object buffers with NUL, 2014-12-08), the
validation functions were fed the object data as counted strings,
not necessarily terminated with a NUL. We added a helper function
require_end_of_header() to be called at the beginning of these
validation functions to insist that the object data contains an
empty line before its end. The theory is that the validating
functions will notice and stop when it hits an empty line as a
normal end of header (or a required header line that is missing)
before scanning past the end of potentially not NUL-terminated
buffer.
But the theory forgot that in the older days, Git itself happily
created objects with only the header lines without a body. This
caused Git 2.2 and later to issue an unnecessary warning on some
existing repositories.
With a1e920a0, we do not need to require an empty line (or the body)
in these objects to safely parse and validate them. Drop the
offending must have an empty line check from this helper function,
while keeping the other check to make sure that there is no NUL in
the header part of the object, and adjust the name of the helper to
what it does accordingly.
Noticed-by: Wolfgang Denk w...@denx.de
Helped-by: Jeff King p...@peff.net
Signed-off-by: Junio C Hamano gits...@pobox.com
---
* With a proper commit message this time. I did this directly on
top of a1e920a0 (index-pack: terminate object buffers with NUL,
2014-12-08); it has trivial merge conflicts with more recent
codebase, whose resolutions I'll push out later on 'pu'.
fsck.c | 17 +
1 file changed, 13 insertions(+), 4 deletions(-)
diff --git a/fsck.c b/fsck.c
index 88c92e8..3f264e7 100644
--- a/fsck.c
+++ b/fsck.c
@@ -238,8 +238,8 @@ static int fsck_tree(struct tree *item, int strict,
fsck_error error_func)
return retval;
}
-static int require_end_of_header(const void *data, unsigned long size,
- struct object *obj, fsck_error error_func)
+static int verify_headers(const void *data, unsigned long size,
+ struct object *obj, fsck_error error_func)
{
const char *buffer = (const char *)data;
unsigned long i;
@@ -255,6 +255,15 @@ static int require_end_of_header(const void *data,
unsigned long size,
}
}
+ /*
+* We did not find double-LF that separates the header
+* and the body. Not having a body is not a crime but
+* we do want to see the terminating LF for the last header
+* line.
+*/
+ if (size buffer[size - 1] == '\n')
+ return 0;
+
return error_func(obj, FSCK_ERROR, unterminated header);
}
@@ -305,7 +314,7 @@ static int fsck_commit_buffer(struct commit *commit, const
char *buffer,
unsigned parent_count, parent_line_count = 0;
int err;
- if (require_end_of_header(buffer, size, commit-object, error_func))
+ if (verify_headers(buffer, size, commit-object, error_func))
return -1;
if (!skip_prefix(buffer, tree , buffer))
@@ -384,7 +393,7 @@ static int fsck_tag_buffer(struct tag *tag, const char
*data,
}
}
- if (require_end_of_header(buffer, size, tag-object, error_func))
+ if (verify_headers(buffer, size, tag-object, error_func))
goto done;
if (!skip_prefix(buffer, object , buffer)) {
--
2.5.0-rc0-151-g019519d
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] fsck: it is OK for a tag and a commit to lack the body
Johannes Schindelin johannes.schinde...@gmx.de writes: +/* + * We did not find double-LF that separates the header + * and the body. Not having a body is not a crime but + * we do want to see the terminating LF for the last header + * line. + */ +if (size buffer[size - 1] == '\n') +return 0; + return error_func(obj, FSCK_ERROR, unterminated header); } Hmm. Maybe we should still warn when there is no empty line finishing the header explicitly, or at least make it FSCK_IGNORE by default so that maintainers who like a stricter check can upgrade the condition to an error? Wolfgang, do you know how these old tags without messages were created? I think in the old days we didn't advertise git tag as the sole way to create a tag object and more people drove git mktag from their script, and mktag until e0aaf781 (mktag.c: improve verification of tagger field and tests, 2008-03-27) did not complain if the header were not terminated with double-LF even when the tag did not have a body (hence there is no need for double-LF). Dscho, I do not think it is reasonable to force all repository owners of projects that started using Git before early 2008 to set configuration variable to ignore warnings for something that was perfectly kosher back when their project started. More importantly, even though Git core itself adds unnecessary double-LF after the header in a tag or a commit that does not have any body, I am not sure if we punish people who use current versions of third-party reimplementations of Git that do not write that unnecessary double-LF at the end an object without a body (I am not saying that there is any known third-party reimplementation to do so---I am saying that I do not think it is their bug if such implementations existed today). Do we have check marked as FSCK_IGNORE by default? I think a more interesting stricter check may be to flag a tag object or a commit object that does not have any body, with or without the double-LF at the end. And such a check can certainly be added in the future, but what I sent was a fix to a regression that caused us to start whining on a syntactically valid object in the v2.2 timeframe, and is not about adding a new feature. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] fsck: it is OK for a tag and a commit to lack the body
Hi Junio, On 2015-06-28 20:18, Junio C Hamano wrote: diff --git a/fsck.c b/fsck.c index 88c92e8..3f264e7 100644 --- a/fsck.c +++ b/fsck.c @@ -255,6 +255,15 @@ static int require_end_of_header(const void *data, unsigned long size, } } + /* + * We did not find double-LF that separates the header + * and the body. Not having a body is not a crime but + * we do want to see the terminating LF for the last header + * line. + */ + if (size buffer[size - 1] == '\n') + return 0; + return error_func(obj, FSCK_ERROR, unterminated header); } Hmm. Maybe we should still warn when there is no empty line finishing the header explicitly, or at least make it FSCK_IGNORE by default so that maintainers who like a stricter check can upgrade the condition to an error? Otherwise: ACK. Ciao, Dscho -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
