| Message ID | Z3qN8U2VbZBnUSWj@ArchLinux (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | add more ref consistency checks | expand |
On Sun, Jan 05, 2025 at 09:49:37PM +0800, shejialuo wrote: [snip] > However, when adding the new test for a bad header, the program will > still die in the "create_snapshot" method. This is because we have > checked the files-backend firstly and we use "parse_object" to check > whether the object exists and whether the type is correct. This function > will eventually call "create_snapshot" and "next_record" method, if > there is something wrong with packed-backend, the program just dies. > > It's bad to just die the program because we want to report the problems > as many as possible. We should avoid checking object and its type when > packed-backend is broken. So, we should first check the consistency of > the packed-backend then for files-backend. > > Add a new flag "safe_object_check" in "fsck_options", when there is > anything wrong with the parsing process, set this flag to 0 to avoid > checking objects in the later checks. > Here, I made a mistake. The most simplest way is to call the "disable_replace_refs" function in "builtin/refs". So, there is a lot of code and commit message needs to be fixed in the version 2. I have just realized about this. So, tell the reviewers in advance about this. Thanks, Jialuo
On Sun, Jan 05, 2025 at 09:49:37PM +0800, shejialuo wrote: > Add a new flag "safe_object_check" in "fsck_options", when there is > anything wrong with the parsing process, set this flag to 0 to avoid > checking objects in the later checks. Okay, I understand the motivation: a corrupted refdb may be completely bogus, so checking its objects may not be sensible. For one of the preceding commits I made the suggestion to split out the object checks into a generic part instead, as they aren't specific to the backend. With such a scheme we could adapt the logic to first do the backend-specific checks for the format, and only in case the backend looks sane to us we'd execute those generic checks for that specific backend. That'd allow us to get rid of the "safe object check" flag. > diff --git a/refs/packed-backend.c b/refs/packed-backend.c > index d9eb2f8b71..3b11abe5f8 100644 > --- a/refs/packed-backend.c > +++ b/refs/packed-backend.c > @@ -1748,12 +1748,100 @@ static struct ref_iterator *packed_reflog_iterator_begin(struct ref_store *ref_s > return empty_ref_iterator_begin(); > } > > +static int packed_fsck_ref_next_line(struct fsck_options *o, > + int line_number, const char *start, > + const char *eof, const char **eol) > +{ > + int ret = 0; > + > + *eol = memchr(start, '\n', eof - start); > + if (!*eol) { > + struct strbuf packed_entry = STRBUF_INIT; > + struct fsck_ref_report report = { 0 }; > + > + strbuf_addf(&packed_entry, "packed-refs line %d", line_number); > + report.path = packed_entry.buf; > + ret = fsck_report_ref(o, &report, > + FSCK_MSG_PACKED_REF_ENTRY_NOT_TERMINATED, > + "'%.*s' is not terminated with a newline", > + (int)(eof - start), start); > + > + /* > + * There is no newline but we still want to parse it to the end of > + * the buffer. > + */ > + *eol = eof; I don't quite understand. We've figured out that there isn't a newline, so wouldn't that mean that we _are_ at the end of the buffer already? > + strbuf_release(&packed_entry); > + } > + > + return ret; > +} > + > +static int packed_fsck_ref_header(struct fsck_options *o, const char *start, const char *eol) > +{ > + const char *err_fmt = NULL; > + int fsck_msg_id = -1; > + > + if (!starts_with(start, "# pack-refs with:")) { > + err_fmt = "'%.*s' does not start with '# pack-refs with:'"; > + fsck_msg_id = FSCK_MSG_BAD_PACKED_REF_HEADER; > + } else if (strncmp(start, PACKED_REFS_HEADER, strlen(PACKED_REFS_HEADER))) { > + err_fmt = "'%.*s' is not the official packed-refs header"; I wouldn't say "official", because it could totally be that whatever is official changes in the future, e.g. when a new format is introduced. Unlikely to happen, but saying "unknown packed-refs header" might be a bit more future proof. > + fsck_msg_id = FSCK_MSG_UNKNOWN_PACKED_REF_HEADER; > + } > + > + if (err_fmt && fsck_msg_id >= 0) { > + struct fsck_ref_report report = { 0 }; > + report.path = "packed-refs.header"; > + > + return fsck_report_ref(o, &report, fsck_msg_id, err_fmt, > + (int)(eol - start), start); > + > + } > + > + return 0; > +} > + > +static int packed_fsck_ref_content(struct fsck_options *o, > + const char *start, const char *eof) > +{ > + int line_number = 1; > + const char *eol; > + int ret = 0; > + > + ret |= packed_fsck_ref_next_line(o, line_number, start, eof, &eol); > + if (*start == '#') { > + ret |= packed_fsck_ref_header(o, start, eol); > + > + start = eol + 1; > + line_number++; The header can only appear at the beginning of the file, can't it? But we accept it in every line here. We should likely verify that it's actually a header and not a line at some random place. > + } else { > + struct fsck_ref_report report = { 0 }; > + report.path = "packed-refs"; > + > + ret |= fsck_report_ref(o, &report, > + FSCK_MSG_PACKED_REF_MISSING_HEADER, > + "missing header line"); > + } > + > + /* > + * If there is anything wrong during the parsing of the "packed-refs" > + * file, we should not check the object of the refs. > + */ > + if (ret) > + o->safe_object_check = 0; > + > + > + return ret; > +} > + > static int packed_fsck(struct ref_store *ref_store, > struct fsck_options *o, > struct worktree *wt) > { > struct packed_ref_store *refs = packed_downcast(ref_store, > REF_STORE_READ, "fsck"); > + struct strbuf packed_ref_content = STRBUF_INIT; > struct stat st; > int ret = 0; > > @@ -1779,7 +1867,24 @@ static int packed_fsck(struct ref_store *ref_store, > goto cleanup; > } > > + if (strbuf_read_file(&packed_ref_content, refs->path, 0) < 0) { > + /* > + * Although we have checked that the file exists, there is a possibility > + * that it has been removed between the lstat() and the read attempt by > + * another process. In that case, we should not report an error. > + */ > + if (errno == ENOENT) > + goto cleanup; Unlikely, but good to guard us against that condition regardless. It's still not entirely race-free though because the file could meanwhile have changed into a symlink, and we wouldn't notice now. We could fix that by using open(O_NOFOLLOW), fstat the returne file descriptor and then use `strbuf_read()` to slurp in the file. > + ret = error_errno("could not read %s", refs->path); > + goto cleanup; > + } > + > + ret = packed_fsck_ref_content(o, packed_ref_content.buf, > + packed_ref_content.buf + packed_ref_content.len); > + > cleanup: > + strbuf_release(&packed_ref_content); > return ret; > } > > diff --git a/t/t0602-reffiles-fsck.sh b/t/t0602-reffiles-fsck.sh > index 307f94a3ca..6c729e749a 100755 > --- a/t/t0602-reffiles-fsck.sh > +++ b/t/t0602-reffiles-fsck.sh > @@ -646,4 +646,48 @@ test_expect_success SYMLINKS 'the filetype of packed-refs should be checked' ' > test_cmp expect err > ' > > +test_expect_success 'packed-refs header should be checked' ' > + test_when_finished "rm -rf repo" && > + git init repo && > + cd repo && The same comment applies here as on a preceding test: cd should be executed in a subshell. Patrick
On Thu, Jan 16, 2025 at 02:57:37PM +0100, Patrick Steinhardt wrote: > On Sun, Jan 05, 2025 at 09:49:37PM +0800, shejialuo wrote: > > Add a new flag "safe_object_check" in "fsck_options", when there is > > anything wrong with the parsing process, set this flag to 0 to avoid > > checking objects in the later checks. > > Okay, I understand the motivation: a corrupted refdb may be completely > bogus, so checking its objects may not be sensible. > > For one of the preceding commits I made the suggestion to split out the > object checks into a generic part instead, as they aren't specific to > the backend. With such a scheme we could adapt the logic to first do the > backend-specific checks for the format, and only in case the backend > looks sane to us we'd execute those generic checks for that specific > backend. That'd allow us to get rid of the "safe object check" flag. > Yes, I agree with you here. And I won't touch this topic in the next version. Let me make this patch concentrate on the "packed-ref" format. > > diff --git a/refs/packed-backend.c b/refs/packed-backend.c > > index d9eb2f8b71..3b11abe5f8 100644 > > --- a/refs/packed-backend.c > > +++ b/refs/packed-backend.c > > @@ -1748,12 +1748,100 @@ static struct ref_iterator *packed_reflog_iterator_begin(struct ref_store *ref_s > > return empty_ref_iterator_begin(); > > } > > > > +static int packed_fsck_ref_next_line(struct fsck_options *o, > > + int line_number, const char *start, > > + const char *eof, const char **eol) > > +{ > > + int ret = 0; > > + > > + *eol = memchr(start, '\n', eof - start); > > + if (!*eol) { > > + struct strbuf packed_entry = STRBUF_INIT; > > + struct fsck_ref_report report = { 0 }; > > + > > + strbuf_addf(&packed_entry, "packed-refs line %d", line_number); > > + report.path = packed_entry.buf; > > + ret = fsck_report_ref(o, &report, > > + FSCK_MSG_PACKED_REF_ENTRY_NOT_TERMINATED, > > + "'%.*s' is not terminated with a newline", > > + (int)(eof - start), start); > > + > > + /* > > + * There is no newline but we still want to parse it to the end of > > + * the buffer. > > + */ > > + *eol = eof; > > I don't quite understand. We've figured out that there isn't a newline, > so wouldn't that mean that we _are_ at the end of the buffer already? > In the "packed-refs" file, the last line should end with a newline. If not, this is a fatal error. The motivation why I do this is that for each line, we could pass the "line_start" and "eol" to check. But if there is no newline, the "eol" will be NULL. So, I change it to "eof" to make sure that we could follow the same logic when "eol" is not NULL. I guess I should not handle this in this function which may cause confusion here. I will improve this in the next version. > > + strbuf_release(&packed_entry); > > + } > > + > > + return ret; > > +} > > + > > +static int packed_fsck_ref_header(struct fsck_options *o, const char *start, const char *eol) > > +{ > > + const char *err_fmt = NULL; > > + int fsck_msg_id = -1; > > + > > + if (!starts_with(start, "# pack-refs with:")) { > > + err_fmt = "'%.*s' does not start with '# pack-refs with:'"; > > + fsck_msg_id = FSCK_MSG_BAD_PACKED_REF_HEADER; > > + } else if (strncmp(start, PACKED_REFS_HEADER, strlen(PACKED_REFS_HEADER))) { > > + err_fmt = "'%.*s' is not the official packed-refs header"; > > I wouldn't say "official", because it could totally be that whatever is > official changes in the future, e.g. when a new format is introduced. > Unlikely to happen, but saying "unknown packed-refs header" might be a > bit more future proof. > I will improve this in the next version. > > + fsck_msg_id = FSCK_MSG_UNKNOWN_PACKED_REF_HEADER; > > + } > > + > > + if (err_fmt && fsck_msg_id >= 0) { > > + struct fsck_ref_report report = { 0 }; > > + report.path = "packed-refs.header"; > > + > > + return fsck_report_ref(o, &report, fsck_msg_id, err_fmt, > > + (int)(eol - start), start); > > + > > + } > > + > > + return 0; > > +} > > + > > +static int packed_fsck_ref_content(struct fsck_options *o, > > + const char *start, const char *eof) > > +{ > > + int line_number = 1; > > + const char *eol; > > + int ret = 0; > > + > > + ret |= packed_fsck_ref_next_line(o, line_number, start, eof, &eol); > > + if (*start == '#') { > > + ret |= packed_fsck_ref_header(o, start, eol); > > + > > + start = eol + 1; > > + line_number++; > > The header can only appear at the beginning of the file, can't it? But > we accept it in every line here. We should likely verify that it's > actually a header and not a line at some random place. > Yes. But we don't accept it in every line. Because in here, we are getting the first line "start" and "eol" by using "packed_fsck_ref_next_line". Only it starts with "#", we will check the header consistency. > > + } else { > > + struct fsck_ref_report report = { 0 }; > > + report.path = "packed-refs"; > > + > > + ret |= fsck_report_ref(o, &report, > > + FSCK_MSG_PACKED_REF_MISSING_HEADER, > > + "missing header line"); > > + } > > + > > + /* > > + * If there is anything wrong during the parsing of the "packed-refs" > > + * file, we should not check the object of the refs. > > + */ > > + if (ret) > > + o->safe_object_check = 0; > > + > > + > > + return ret; > > +} > > + > > static int packed_fsck(struct ref_store *ref_store, > > struct fsck_options *o, > > struct worktree *wt) > > { > > struct packed_ref_store *refs = packed_downcast(ref_store, > > REF_STORE_READ, "fsck"); > > + struct strbuf packed_ref_content = STRBUF_INIT; > > struct stat st; > > int ret = 0; > > > > @@ -1779,7 +1867,24 @@ static int packed_fsck(struct ref_store *ref_store, > > goto cleanup; > > } > > > > + if (strbuf_read_file(&packed_ref_content, refs->path, 0) < 0) { > > + /* > > + * Although we have checked that the file exists, there is a possibility > > + * that it has been removed between the lstat() and the read attempt by > > + * another process. In that case, we should not report an error. > > + */ > > + if (errno == ENOENT) > > + goto cleanup; > > Unlikely, but good to guard us against that condition regardless. It's > still not entirely race-free though because the file could meanwhile > have changed into a symlink, and we wouldn't notice now. We could fix > that by using open(O_NOFOLLOW), fstat the returne file descriptor and > then use `strbuf_read()` to slurp in the file. > Would this be too complicated for us to avoid race condition and we will introduce a lot of code to handle above logic. Because there is a possibility that when finishing reading the file content to the memory, the file could be changed into a symlink and we cannot notice. So, I wanna say we can't avoid race condition totally. It would be good if we avoid race, but what I am concern about here is that we would make the logic too complicated. So, could we make it unchanged?
On Fri, Jan 17, 2025 at 10:23:06PM +0800, shejialuo wrote: > On Thu, Jan 16, 2025 at 02:57:37PM +0100, Patrick Steinhardt wrote: > > On Sun, Jan 05, 2025 at 09:49:37PM +0800, shejialuo wrote: > > > @@ -1779,7 +1867,24 @@ static int packed_fsck(struct ref_store *ref_store, > > > goto cleanup; > > > } > > > > > > + if (strbuf_read_file(&packed_ref_content, refs->path, 0) < 0) { > > > + /* > > > + * Although we have checked that the file exists, there is a possibility > > > + * that it has been removed between the lstat() and the read attempt by > > > + * another process. In that case, we should not report an error. > > > + */ > > > + if (errno == ENOENT) > > > + goto cleanup; > > > > Unlikely, but good to guard us against that condition regardless. It's > > still not entirely race-free though because the file could meanwhile > > have changed into a symlink, and we wouldn't notice now. We could fix > > that by using open(O_NOFOLLOW), fstat the returne file descriptor and > > then use `strbuf_read()` to slurp in the file. > > > > Would this be too complicated for us to avoid race condition and we will > introduce a lot of code to handle above logic. Because there is a > possibility that when finishing reading the file content to the memory, > the file could be changed into a symlink and we cannot notice. So, I > wanna say we can't avoid race condition totally. It would be good if we > avoid race, but what I am concern about here is that we would make the > logic too complicated. So, could we make it unchanged? It would ultimately only be two additional function calls, so I don't think it's going to add a ton of complexity. Whether things are changing _after_ we have opened and read the file is a different issue, and I agree that we shouldn't have to care about that case. What we're after is whether things are correct when running consistency checks, it's always a possibility that e.g. the packed-refs file gets rewritten while we do it. Patrick
On Thu, Jan 16, 2025 at 02:57:37PM +0100, Patrick Steinhardt wrote: [snip] > > @@ -1779,7 +1867,24 @@ static int packed_fsck(struct ref_store *ref_store, > > goto cleanup; > > } > > > > + if (strbuf_read_file(&packed_ref_content, refs->path, 0) < 0) { > > + /* > > + * Although we have checked that the file exists, there is a possibility > > + * that it has been removed between the lstat() and the read attempt by > > + * another process. In that case, we should not report an error. > > + */ > > + if (errno == ENOENT) > > + goto cleanup; > > Unlikely, but good to guard us against that condition regardless. It's > still not entirely race-free though because the file could meanwhile > have changed into a symlink, and we wouldn't notice now. We could fix > that by using open(O_NOFOLLOW), fstat the returne file descriptor and > then use `strbuf_read()` to slurp in the file. > I have been looking back to the original discussion. I will follow this advice which eventually avoids the race. Thanks, Jialuo
diff --git a/Documentation/fsck-msgids.txt b/Documentation/fsck-msgids.txt index b14bc44ca4..34375a3143 100644 --- a/Documentation/fsck-msgids.txt +++ b/Documentation/fsck-msgids.txt @@ -16,6 +16,10 @@ `badObjectSha1`:: (ERROR) An object has a bad sha1. +`badPackedRefHeader`:: + (ERROR) The "packed-refs" file contains an invalid + header. + `badParentSha1`:: (ERROR) A commit object has a bad parent sha1. @@ -176,6 +180,13 @@ `nullSha1`:: (WARN) Tree contains entries pointing to a null sha1. +`packedRefEntryNotTerminated`:: + (ERROR) The "packed-refs" file contains an entry that is + not terminated by a newline. + +`packedRefMissingHeader`:: + (INFO) The "packed-refs" file does not contain the header. + `refMissingNewline`:: (INFO) A loose ref that does not end with newline(LF). As valid implementations of Git never created such a loose ref @@ -208,6 +219,11 @@ `treeNotSorted`:: (ERROR) A tree is not properly sorted. +`unknownPackedRefHeader`:: + (INFO) The "packed-refs" header starts with "# pack-refs with:" + but the remaining content is not the same as what `git pack-refs` + would write. + `unknownType`:: (ERROR) Found an unknown object type. diff --git a/fsck.h b/fsck.h index a44c231a5f..026ad1d537 100644 --- a/fsck.h +++ b/fsck.h @@ -30,6 +30,7 @@ enum fsck_msg_type { FUNC(BAD_EMAIL, ERROR) \ FUNC(BAD_NAME, ERROR) \ FUNC(BAD_OBJECT_SHA1, ERROR) \ + FUNC(BAD_PACKED_REF_HEADER, ERROR) \ FUNC(BAD_PARENT_SHA1, ERROR) \ FUNC(BAD_REF_CONTENT, ERROR) \ FUNC(BAD_REF_FILETYPE, ERROR) \ @@ -53,6 +54,7 @@ enum fsck_msg_type { FUNC(MISSING_TYPE, ERROR) \ FUNC(MISSING_TYPE_ENTRY, ERROR) \ FUNC(MULTIPLE_AUTHORS, ERROR) \ + FUNC(PACKED_REF_ENTRY_NOT_TERMINATED, ERROR) \ FUNC(TREE_NOT_SORTED, ERROR) \ FUNC(UNKNOWN_TYPE, ERROR) \ FUNC(ZERO_PADDED_DATE, ERROR) \ @@ -90,6 +92,8 @@ enum fsck_msg_type { FUNC(REF_MISSING_NEWLINE, INFO) \ FUNC(SYMREF_TARGET_IS_NOT_A_REF, INFO) \ FUNC(TRAILING_REF_CONTENT, INFO) \ + FUNC(UNKNOWN_PACKED_REF_HEADER, INFO) \ + FUNC(PACKED_REF_MISSING_HEADER, INFO) \ /* ignored (elevated when requested) */ \ FUNC(EXTRA_HEADER_ENTRY, IGNORE) @@ -163,6 +167,7 @@ struct fsck_options { fsck_error error_func; unsigned strict; unsigned verbose; + int safe_object_check; enum fsck_msg_type *msg_type; struct oidset skip_oids; struct oidset gitmodules_found; @@ -198,6 +203,7 @@ struct fsck_options { } #define FSCK_REFS_OPTIONS_DEFAULT { \ .error_func = fsck_refs_error_function, \ + .safe_object_check = 1, \ } /* descend in all linked child objects diff --git a/refs/files-backend.c b/refs/files-backend.c index 0a4912c009..66eae36184 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -3599,7 +3599,7 @@ static int files_fsck_refs_oid(struct fsck_options *o, struct object *obj; int ret = 0; - if (is_promisor_object(ref_store->repo, oid)) + if (!o->safe_object_check || is_promisor_object(ref_store->repo, oid)) return 0; obj = parse_object(ref_store->repo, oid); @@ -3819,8 +3819,8 @@ static int files_fsck(struct ref_store *ref_store, struct files_ref_store *refs = files_downcast(ref_store, REF_STORE_READ, "fsck"); - return files_fsck_refs(ref_store, o, wt) | - refs->packed_ref_store->be->fsck(refs->packed_ref_store, o, wt); + return refs->packed_ref_store->be->fsck(refs->packed_ref_store, o, wt) | + files_fsck_refs(ref_store, o, wt); } struct ref_storage_be refs_be_files = { diff --git a/refs/packed-backend.c b/refs/packed-backend.c index d9eb2f8b71..3b11abe5f8 100644 --- a/refs/packed-backend.c +++ b/refs/packed-backend.c @@ -1748,12 +1748,100 @@ static struct ref_iterator *packed_reflog_iterator_begin(struct ref_store *ref_s return empty_ref_iterator_begin(); } +static int packed_fsck_ref_next_line(struct fsck_options *o, + int line_number, const char *start, + const char *eof, const char **eol) +{ + int ret = 0; + + *eol = memchr(start, '\n', eof - start); + if (!*eol) { + struct strbuf packed_entry = STRBUF_INIT; + struct fsck_ref_report report = { 0 }; + + strbuf_addf(&packed_entry, "packed-refs line %d", line_number); + report.path = packed_entry.buf; + ret = fsck_report_ref(o, &report, + FSCK_MSG_PACKED_REF_ENTRY_NOT_TERMINATED, + "'%.*s' is not terminated with a newline", + (int)(eof - start), start); + + /* + * There is no newline but we still want to parse it to the end of + * the buffer. + */ + *eol = eof; + strbuf_release(&packed_entry); + } + + return ret; +} + +static int packed_fsck_ref_header(struct fsck_options *o, const char *start, const char *eol) +{ + const char *err_fmt = NULL; + int fsck_msg_id = -1; + + if (!starts_with(start, "# pack-refs with:")) { + err_fmt = "'%.*s' does not start with '# pack-refs with:'"; + fsck_msg_id = FSCK_MSG_BAD_PACKED_REF_HEADER; + } else if (strncmp(start, PACKED_REFS_HEADER, strlen(PACKED_REFS_HEADER))) { + err_fmt = "'%.*s' is not the official packed-refs header"; + fsck_msg_id = FSCK_MSG_UNKNOWN_PACKED_REF_HEADER; + } + + if (err_fmt && fsck_msg_id >= 0) { + struct fsck_ref_report report = { 0 }; + report.path = "packed-refs.header"; + + return fsck_report_ref(o, &report, fsck_msg_id, err_fmt, + (int)(eol - start), start); + + } + + return 0; +} + +static int packed_fsck_ref_content(struct fsck_options *o, + const char *start, const char *eof) +{ + int line_number = 1; + const char *eol; + int ret = 0; + + ret |= packed_fsck_ref_next_line(o, line_number, start, eof, &eol); + if (*start == '#') { + ret |= packed_fsck_ref_header(o, start, eol); + + start = eol + 1; + line_number++; + } else { + struct fsck_ref_report report = { 0 }; + report.path = "packed-refs"; + + ret |= fsck_report_ref(o, &report, + FSCK_MSG_PACKED_REF_MISSING_HEADER, + "missing header line"); + } + + /* + * If there is anything wrong during the parsing of the "packed-refs" + * file, we should not check the object of the refs. + */ + if (ret) + o->safe_object_check = 0; + + + return ret; +} + static int packed_fsck(struct ref_store *ref_store, struct fsck_options *o, struct worktree *wt) { struct packed_ref_store *refs = packed_downcast(ref_store, REF_STORE_READ, "fsck"); + struct strbuf packed_ref_content = STRBUF_INIT; struct stat st; int ret = 0; @@ -1779,7 +1867,24 @@ static int packed_fsck(struct ref_store *ref_store, goto cleanup; } + if (strbuf_read_file(&packed_ref_content, refs->path, 0) < 0) { + /* + * Although we have checked that the file exists, there is a possibility + * that it has been removed between the lstat() and the read attempt by + * another process. In that case, we should not report an error. + */ + if (errno == ENOENT) + goto cleanup; + + ret = error_errno("could not read %s", refs->path); + goto cleanup; + } + + ret = packed_fsck_ref_content(o, packed_ref_content.buf, + packed_ref_content.buf + packed_ref_content.len); + cleanup: + strbuf_release(&packed_ref_content); return ret; } diff --git a/t/t0602-reffiles-fsck.sh b/t/t0602-reffiles-fsck.sh index 307f94a3ca..6c729e749a 100755 --- a/t/t0602-reffiles-fsck.sh +++ b/t/t0602-reffiles-fsck.sh @@ -646,4 +646,48 @@ test_expect_success SYMLINKS 'the filetype of packed-refs should be checked' ' test_cmp expect err ' +test_expect_success 'packed-refs header should be checked' ' + test_when_finished "rm -rf repo" && + git init repo && + cd repo && + test_commit default && + + git refs verify 2>err && + test_must_be_empty err && + + printf "$(git rev-parse main) refs/heads/main\n" >.git/packed-refs && + git refs verify 2>err && + cat >expect <<-EOF && + warning: packed-refs: packedRefMissingHeader: missing header line + EOF + rm .git/packed-refs && + test_cmp expect err && + + for bad_header in "# pack-refs wit: peeled fully-peeled sorted " \ + "# pack-refs with traits: peeled fully-peeled sorted " \ + "# pack-refs with a: peeled fully-peeled" + do + printf "%s\n" "$bad_header" >.git/packed-refs && + test_must_fail git refs verify 2>err && + cat >expect <<-EOF && + error: packed-refs.header: badPackedRefHeader: '\''$bad_header'\'' does not start with '\''# pack-refs with:'\'' + EOF + rm .git/packed-refs && + test_cmp expect err || return 1 + done && + + for unknown_header in "# pack-refs with: peeled fully-peeled sorted garbage" \ + "# pack-refs with: peeled" \ + "# pack-refs with: peeled peeled-fully sort" + do + printf "%s\n" "$unknown_header" >.git/packed-refs && + git refs verify 2>err && + cat >expect <<-EOF && + warning: packed-refs.header: unknownPackedRefHeader: '\''$unknown_header'\'' is not the official packed-refs header + EOF + rm .git/packed-refs && + test_cmp expect err || return 1 + done +' + test_done
In "packed-backend.c::create_snapshot", if there is a header (the line which starts with '#'), we will check whether the line starts with "# pack-refs with:". As we are going to implement the header consistency check, we should port this check into "packed_fsck". However, the above check is not enough, this is because "git pack-refs" will always write "PACKED_REFS_HEADER" which is a constant string to the "packed-refs" file. So, we should check the following things for the header. 1. If the header does not exist, we may report an error to the user because it should exist, but we do allow no header in "packed-refs" file. So, create a new fsck message "packedRefMissingHeader(INFO)" to warn the user and also keep compatibility. 2. If the header content does not start with "# packed-ref with:", we should report an error just like what "create_snapshot" does. So, create a new fsck message "badPackedRefHeader(ERROR)" for this. 3. If the header content is not the same as the constant string "PACKED_REFS_HEADER", ideally, we should report an error to the user. However, we allow other contents as long as the header content starts with "# packed-ref with:". To keep compatibility, create a new fsck message "unknownPackedRefHeader(INFO)" to warn about this. We may tighten this rule in the future. In order to achieve above checks, read the "packed-refs" file via "strbuf_read_file". Like what "create_snapshot" and other functions do, we could split the line by finding the next newline in the buf. If we cannot find a newline, this is an error. So, create a function "packed_fsck_ref_next_line" to find the next newline and if there is no such newline, use "packedRefEntryNotTerminated(INFO)" to report an error to the user. Then, parse the first line to apply the above three checks. Update the test to excise the code. However, when adding the new test for a bad header, the program will still die in the "create_snapshot" method. This is because we have checked the files-backend firstly and we use "parse_object" to check whether the object exists and whether the type is correct. This function will eventually call "create_snapshot" and "next_record" method, if there is something wrong with packed-backend, the program just dies. It's bad to just die the program because we want to report the problems as many as possible. We should avoid checking object and its type when packed-backend is broken. So, we should first check the consistency of the packed-backend then for files-backend. Add a new flag "safe_object_check" in "fsck_options", when there is anything wrong with the parsing process, set this flag to 0 to avoid checking objects in the later checks. Mentored-by: Patrick Steinhardt <ps@pks.im> Mentored-by: Karthik Nayak <karthik.188@gmail.com> Signed-off-by: shejialuo <shejialuo@gmail.com> --- Documentation/fsck-msgids.txt | 16 ++++++ fsck.h | 6 ++ refs/files-backend.c | 6 +- refs/packed-backend.c | 105 ++++++++++++++++++++++++++++++++++ t/t0602-reffiles-fsck.sh | 44 ++++++++++++++ 5 files changed, 174 insertions(+), 3 deletions(-)