| Message ID | Zvj-osCNDMrUQv83@ArchLinux (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | add ref content check for files backend | expand |
On Sun, Sep 29, 2024 at 03:15:46PM +0800, shejialuo wrote: > "git-fsck(1)" has some consistency checks for regular refs. As we want > to align the checks "git refs verify" performs with them (and eventually > call the unified code that checks refs from both), port the logic > "git-fsck" has to "git refs verify". What's missing here is the actual intent of this commit, namely why we want to align the checks. I assume that this prepares us for calling `git refs verify` as part of git-fsck(1), but readers not familiar with the larger picture may be left wondering. > "git-fsck(1)" will report an error when the ref content is invalid. > Following this, add a similar check to "git refs verify". Then add a new > fsck error message "badRefContent(ERROR)" to represent that a ref has an > invalid content. It would help readers to know where the code is that you're porting over to `git refs verify` so that one can double check that the port is done faithfully to the original. Patrick
On Mon, Oct 07, 2024 at 08:58:34AM +0200, Patrick Steinhardt wrote: > On Sun, Sep 29, 2024 at 03:15:46PM +0800, shejialuo wrote: > > "git-fsck(1)" has some consistency checks for regular refs. As we want > > to align the checks "git refs verify" performs with them (and eventually > > call the unified code that checks refs from both), port the logic > > "git-fsck" has to "git refs verify". > > What's missing here is the actual intent of this commit, namely why we > want to align the checks. I assume that this prepares us for calling > `git refs verify` as part of git-fsck(1), but readers not familiar with > the larger picture may be left wondering. > Indeed, I will improve this in the next version. > > "git-fsck(1)" will report an error when the ref content is invalid. > > Following this, add a similar check to "git refs verify". Then add a new > > fsck error message "badRefContent(ERROR)" to represent that a ref has an > > invalid content. > > It would help readers to know where the code is that you're porting over > to `git refs verify` so that one can double check that the port is done > faithfully to the original. > I am a little confused here. There are too many codes in "git-fsck(1)" to check the ref consistency. How could I accurately express this info in the commit message? > Patrick Thanks, Jialuo
On Mon, Oct 07, 2024 at 04:42:44PM +0800, shejialuo wrote: > On Mon, Oct 07, 2024 at 08:58:34AM +0200, Patrick Steinhardt wrote: > > On Sun, Sep 29, 2024 at 03:15:46PM +0800, shejialuo wrote: > > > "git-fsck(1)" will report an error when the ref content is invalid. > > > Following this, add a similar check to "git refs verify". Then add a new > > > fsck error message "badRefContent(ERROR)" to represent that a ref has an > > > invalid content. > > > > It would help readers to know where the code is that you're porting over > > to `git refs verify` so that one can double check that the port is done > > faithfully to the original. > > > > I am a little confused here. There are too many codes in "git-fsck(1)" > to check the ref consistency. How could I accurately express this info > in the commit message? Well, you say you ported over a specific consistency check from git-fsck(1) to `git refs verify` in the commit message. So I assume that it should match a specific check in git-fsck(1), shouldn't it? Patrick
On Mon, Oct 07, 2024 at 11:18:24AM +0200, Patrick Steinhardt wrote: > On Mon, Oct 07, 2024 at 04:42:44PM +0800, shejialuo wrote: > > On Mon, Oct 07, 2024 at 08:58:34AM +0200, Patrick Steinhardt wrote: > > > On Sun, Sep 29, 2024 at 03:15:46PM +0800, shejialuo wrote: > > > > "git-fsck(1)" will report an error when the ref content is invalid. > > > > Following this, add a similar check to "git refs verify". Then add a new > > > > fsck error message "badRefContent(ERROR)" to represent that a ref has an > > > > invalid content. > > > > > > It would help readers to know where the code is that you're porting over > > > to `git refs verify` so that one can double check that the port is done > > > faithfully to the original. > > > > > > > I am a little confused here. There are too many codes in "git-fsck(1)" > > to check the ref consistency. How could I accurately express this info > > in the commit message? > > Well, you say you ported over a specific consistency check from > git-fsck(1) to `git refs verify` in the commit message. So I assume that > it should match a specific check in git-fsck(1), shouldn't it? > I understand your meaning here. I will improve the commit message in the next version. > Patrick
shejialuo <shejialuo@gmail.com> writes: [snip] > + if (strbuf_read_file(&ref_content, iter->path.buf, 0) < 0) { > + ret = fsck_report_ref(o, &report, > + FSCK_MSG_BAD_REF_CONTENT, > + "cannot read ref file"); > + goto cleanup; > + } > + Shouldn't we use `die_errno` here instead? I mean, this is not really a bad ref content issue. If we don't want to die here, it would still probably be nice to get the actual issue using `strerror` instead and use that instead of the generic message we have here. [snip]
On Tue, Oct 08, 2024 at 12:43:20AM -0700, Karthik Nayak wrote: > shejialuo <shejialuo@gmail.com> writes: > > [snip] > > > + if (strbuf_read_file(&ref_content, iter->path.buf, 0) < 0) { > > + ret = fsck_report_ref(o, &report, > > + FSCK_MSG_BAD_REF_CONTENT, > > + "cannot read ref file"); > > + goto cleanup; > > + } > > + > > Shouldn't we use `die_errno` here instead? I mean, this is not really a > bad ref content issue. If we don't want to die here, it would still > probably be nice to get the actual issue using `strerror` instead and > use that instead of the generic message we have here. > Well, I think I need to dive into the "open" system call here. Actually, we have two opinions now. Junio thought that we should use "fsck_report_ref" to report. Karthik, Patrick and I thought that we should report using "*errno" because this is a general error. Let me investigate what situations will make "open" system call fail. Thus, we could fully explain which choice we will choose. Thanks, Jialuo
shejialuo <shejialuo@gmail.com> writes: > On Tue, Oct 08, 2024 at 12:43:20AM -0700, Karthik Nayak wrote: >> shejialuo <shejialuo@gmail.com> writes: >> >> [snip] >> >> > + if (strbuf_read_file(&ref_content, iter->path.buf, 0) < 0) { >> > + ret = fsck_report_ref(o, &report, >> > + FSCK_MSG_BAD_REF_CONTENT, >> > + "cannot read ref file"); >> > + goto cleanup; >> > + } >> > + >> >> Shouldn't we use `die_errno` here instead? I mean, this is not really a >> bad ref content issue. If we don't want to die here, it would still >> probably be nice to get the actual issue using `strerror` instead and >> use that instead of the generic message we have here. >> > > Well, I think I need to dive into the "open" system call here. Actually, > we have two opinions now. Junio thought that we should use > "fsck_report_ref" to report. Karthik, Patrick and I thought that we > should report using "*errno" because this is a general error. What do you mean by "a general error"? It is true that we failed to read a ref file, so even if it is an I/O error, I'd think it is OK to report it as an error while reading one particular ref. Giving more information is a separate issue. If fsck_report_ref() can be extended to take something like "cannot read ref file '%s': (%s)", iter->path.buf, strerror(errno) that would give the user necessary information. And I agree with half-of what Karthik said, i.e., we do not want to die here if this is meant to run as a part of "git fsck". I may have said this before, but quite frankly, the API into the fsck_report_ref() function is misdesigned. If the single constant string "cannot read ref file" cnanot give more information than FSCK_MSG_BAD_REF_CONTENT, the parameter has no value. The fsck.c:report() function, which is the main function to report fsck's findings before fsck_report_ref() was introduced, did not have such a problem, as it allowed "const char *fmt, ..." at the end. Is it too late to fix the fsck_report_ref()? Thanks.
On Tue, Oct 08, 2024 at 10:44:53AM -0700, Junio C Hamano wrote: > shejialuo <shejialuo@gmail.com> writes: > > > On Tue, Oct 08, 2024 at 12:43:20AM -0700, Karthik Nayak wrote: > >> shejialuo <shejialuo@gmail.com> writes: > >> > >> [snip] > >> > >> > + if (strbuf_read_file(&ref_content, iter->path.buf, 0) < 0) { > >> > + ret = fsck_report_ref(o, &report, > >> > + FSCK_MSG_BAD_REF_CONTENT, > >> > + "cannot read ref file"); > >> > + goto cleanup; > >> > + } > >> > + > >> > >> Shouldn't we use `die_errno` here instead? I mean, this is not really a > >> bad ref content issue. If we don't want to die here, it would still > >> probably be nice to get the actual issue using `strerror` instead and > >> use that instead of the generic message we have here. > >> > > > > Well, I think I need to dive into the "open" system call here. Actually, > > we have two opinions now. Junio thought that we should use > > "fsck_report_ref" to report. Karthik, Patrick and I thought that we > > should report using "*errno" because this is a general error. > > What do you mean by "a general error"? It is true that we failed to > read a ref file, so even if it is an I/O error, I'd think it is OK > to report it as an error while reading one particular ref. > > Giving more information is a separate issue. If fsck_report_ref() > can be extended to take something like > > "cannot read ref file '%s': (%s)", iter->path.buf, strerror(errno) > > that would give the user necessary information. Yeah, this is also in line with what I proposed elsewhere, where we have been discussing the 1:1 mapping between error codes and error messages. If the error messages were dynamic they'd be a whole lot useful overall and could provide more context. > And I agree with half-of what Karthik said, i.e., we do not want to > die here if this is meant to run as a part of "git fsck". > > I may have said this before, but quite frankly, the API into the > fsck_report_ref() function is misdesigned. If the single constant > string "cannot read ref file" cnanot give more information than > FSCK_MSG_BAD_REF_CONTENT, the parameter has no value. True in the current form, yeah. If `fsck_report_ref()` learned to take a vararg argument and treat its first argument as a string format it would be justified though, as the message is now dynamic and can contain more context around the specific failure that cannot be provided statically via the 1:1 mapping between error code and message. > The fsck.c:report() function, which is the main function to report > fsck's findings before fsck_report_ref() was introduced, did not > have such a problem, as it allowed "const char *fmt, ..." at the > end. Is it too late to fix the fsck_report_ref()? I don't think so, I think we should be able to refactor the code rather easily to do so. Patrick
On Tue, Oct 08, 2024 at 10:44:53AM -0700, Junio C Hamano wrote: > shejialuo <shejialuo@gmail.com> writes: > > > On Tue, Oct 08, 2024 at 12:43:20AM -0700, Karthik Nayak wrote: > >> shejialuo <shejialuo@gmail.com> writes: > >> > >> [snip] > >> > >> > + if (strbuf_read_file(&ref_content, iter->path.buf, 0) < 0) { > >> > + ret = fsck_report_ref(o, &report, > >> > + FSCK_MSG_BAD_REF_CONTENT, > >> > + "cannot read ref file"); > >> > + goto cleanup; > >> > + } > >> > + > >> > >> Shouldn't we use `die_errno` here instead? I mean, this is not really a > >> bad ref content issue. If we don't want to die here, it would still > >> probably be nice to get the actual issue using `strerror` instead and > >> use that instead of the generic message we have here. > >> > > > > Well, I think I need to dive into the "open" system call here. Actually, > > we have two opinions now. Junio thought that we should use > > "fsck_report_ref" to report. Karthik, Patrick and I thought that we > > should report using "*errno" because this is a general error. > > What do you mean by "a general error"? It is true that we failed to > read a ref file, so even if it is an I/O error, I'd think it is OK > to report it as an error while reading one particular ref. Make sense. > Giving more information is a separate issue. If fsck_report_ref() > can be extended to take something like > > "cannot read ref file '%s': (%s)", iter->path.buf, strerror(errno) > > that would give the user necessary information. At current, the `fsck_report_ref` can do this. I think I used `fsck_report_ref` function badly in this case. > And I agree with half-of what Karthik said, i.e., we do not want to > die here if this is meant to run as a part of "git fsck". Yes, we should not die the program. Instead, we need to continuously check other refs. > I may have said this before, but quite frankly, the API into the > fsck_report_ref() function is misdesigned. If the single constant > string "cannot read ref file" cnanot give more information than > FSCK_MSG_BAD_REF_CONTENT, the parameter has no value. > > The fsck.c:report() function, which is the main function to report > fsck's findings before fsck_report_ref() was introduced, did not > have such a problem, as it allowed "const char *fmt, ..." at the > end. Is it too late to fix the fsck_report_ref()? I agree that if the FSCK message id could explain the error well, there is no need for us to provide extra message. But, I want to say the `fsck_report_ref` is not misdesigned here. It is just the same as the "fsck.c::report" function which has "const char *fmt, ..." at the end like the following shows: int fsck_report_ref(struct fsck_options *options, struct fsck_ref_report *report, enum fsck_msg_id msg_id, const char *fmt, ...) And I do think "fsck.c::report" function also has the above problems. Let me give you some examples here in "fsck.c": report(options, tree_oid, OBJ_TREE, FSCK_MSG_BAD_FILEMODE, "contains bad file modes"); report(options, tree_oid, OBJ_TREE, FSCK_MSG_DUPLICATE_ENTRIES, "contains duplicate file entries"); ... So, I want to say there is no difference between "fsck_ref_report" and "fsck.c::report". When I refactored the code in GSoC journey, the main problem is that we should reuse the original "fsck.c::report" code instead of writing redundant codes. The final result is I extract a new function "fsck_vreport" here (I leverage the original "fsck.c::report" function) which will be called by "fsck_ref_report" and "fsck.c::report". static int fsck_vreport(struct fsck_options *options, void *fsck_report, enum fsck_msg_id msg_id, const char *fmt, va_list ap) From my perspective, if we decide to refactor, we should allow the user call the followings: fsck_ref_report(..., FSCK_MSG_BAD_REF_CONTENT, NULL); report(..., FSCK_MSG_DUPLICATE_ENTRIES, NULL); So, we should check whether `fmt` is NULL in the `fsck_vreport` function to make sure that if FSCK message is good enough to explain what happens, we should not pass any message. > Thanks. Thanks, Jialuo
On Wed, Oct 09, 2024 at 10:05:19AM +0200, Patrick Steinhardt wrote: [snip] > > I may have said this before, but quite frankly, the API into the > > fsck_report_ref() function is misdesigned. If the single constant > > string "cannot read ref file" cnanot give more information than > > FSCK_MSG_BAD_REF_CONTENT, the parameter has no value. > > True in the current form, yeah. If `fsck_report_ref()` learned to take a > vararg argument and treat its first argument as a string format it would > be justified though, as the message is now dynamic and can contain more > context around the specific failure that cannot be provided statically > via the 1:1 mapping between error code and message. > It is not "learned". At current, `fsck_report_ref` can do this and is the same as "fsck.c::report". I have explained this when replying to Junio. > > The fsck.c:report() function, which is the main function to report > > fsck's findings before fsck_report_ref() was introduced, did not > > have such a problem, as it allowed "const char *fmt, ..." at the > > end. Is it too late to fix the fsck_report_ref()? > > I don't think so, I think we should be able to refactor the code rather > easily to do so. > It's not hard to refactor the code. But this is not the problem. I am a little confused here. Because we already allowed "fsck_report_ref" having "const char *fmt, ..." at the end. > Patrick Thanks, Jialuo
On Wed, Oct 09, 2024 at 07:59:20PM +0800, shejialuo wrote: > On Wed, Oct 09, 2024 at 10:05:19AM +0200, Patrick Steinhardt wrote: > > > The fsck.c:report() function, which is the main function to report > > > fsck's findings before fsck_report_ref() was introduced, did not > > > have such a problem, as it allowed "const char *fmt, ..." at the > > > end. Is it too late to fix the fsck_report_ref()? > > > > I don't think so, I think we should be able to refactor the code rather > > easily to do so. > > > > It's not hard to refactor the code. But this is not the problem. I am a > little confused here. Because we already allowed "fsck_report_ref" > having "const char *fmt, ..." at the end. Ah, I didn't double check, but was operating on what I understood from this thread. In that case I think that the current interface is okay. Patrick
Patrick Steinhardt <ps@pks.im> writes: > On Wed, Oct 09, 2024 at 07:59:20PM +0800, shejialuo wrote: >> On Wed, Oct 09, 2024 at 10:05:19AM +0200, Patrick Steinhardt wrote: >> > > The fsck.c:report() function, which is the main function to report >> > > fsck's findings before fsck_report_ref() was introduced, did not >> > > have such a problem, as it allowed "const char *fmt, ..." at the >> > > end. Is it too late to fix the fsck_report_ref()? >> > >> > I don't think so, I think we should be able to refactor the code rather >> > easily to do so. >> > >> >> It's not hard to refactor the code. But this is not the problem. I am a >> little confused here. Because we already allowed "fsck_report_ref" >> having "const char *fmt, ..." at the end. > > Ah, I didn't double check, but was operating on what I understood from > this thread. In that case I think that the current interface is okay. I didn't, either. So there is an obvious way out for "why aren't we telling the errno to users" issue? That's good.
diff --git a/Documentation/fsck-msgids.txt b/Documentation/fsck-msgids.txt index 68a2801f15..22c385ea22 100644 --- a/Documentation/fsck-msgids.txt +++ b/Documentation/fsck-msgids.txt @@ -19,6 +19,9 @@ `badParentSha1`:: (ERROR) A commit object has a bad parent sha1. +`badRefContent`:: + (ERROR) A ref has bad content. + `badRefFiletype`:: (ERROR) A ref has a bad file type. diff --git a/fsck.h b/fsck.h index 500b4c04d2..0d99a87911 100644 --- a/fsck.h +++ b/fsck.h @@ -31,6 +31,7 @@ enum fsck_msg_type { FUNC(BAD_NAME, ERROR) \ FUNC(BAD_OBJECT_SHA1, ERROR) \ FUNC(BAD_PARENT_SHA1, ERROR) \ + FUNC(BAD_REF_CONTENT, ERROR) \ FUNC(BAD_REF_FILETYPE, ERROR) \ FUNC(BAD_REF_NAME, ERROR) \ FUNC(BAD_TIMEZONE, ERROR) \ diff --git a/refs/files-backend.c b/refs/files-backend.c index 57318b4c4e..35b3fa983e 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -3504,6 +3504,50 @@ typedef int (*files_fsck_refs_fn)(struct ref_store *ref_store, const char *refs_check_dir, struct dir_iterator *iter); +static int files_fsck_refs_content(struct ref_store *ref_store, + struct fsck_options *o, + const char *refs_check_dir, + struct dir_iterator *iter) +{ + struct strbuf ref_content = STRBUF_INIT; + struct strbuf referent = STRBUF_INIT; + struct strbuf refname = STRBUF_INIT; + struct fsck_ref_report report = { 0 }; + unsigned int type = 0; + int failure_errno = 0; + struct object_id oid; + int ret = 0; + + strbuf_addf(&refname, "%s/%s", refs_check_dir, iter->relative_path); + report.path = refname.buf; + + if (S_ISLNK(iter->st.st_mode)) + goto cleanup; + + if (strbuf_read_file(&ref_content, iter->path.buf, 0) < 0) { + ret = fsck_report_ref(o, &report, + FSCK_MSG_BAD_REF_CONTENT, + "cannot read ref file"); + goto cleanup; + } + + if (parse_loose_ref_contents(ref_store->repo->hash_algo, + ref_content.buf, &oid, &referent, + &type, &failure_errno)) { + strbuf_rtrim(&ref_content); + ret = fsck_report_ref(o, &report, + FSCK_MSG_BAD_REF_CONTENT, + "%s", ref_content.buf); + goto cleanup; + } + +cleanup: + strbuf_release(&refname); + strbuf_release(&ref_content); + strbuf_release(&referent); + return ret; +} + static int files_fsck_refs_name(struct ref_store *ref_store UNUSED, struct fsck_options *o, const char *refs_check_dir, @@ -3586,6 +3630,7 @@ static int files_fsck_refs(struct ref_store *ref_store, { files_fsck_refs_fn fsck_refs_fn[]= { files_fsck_refs_name, + files_fsck_refs_content, NULL, }; diff --git a/t/t0602-reffiles-fsck.sh b/t/t0602-reffiles-fsck.sh index 4c6cd6f7d0..628f9bcc46 100755 --- a/t/t0602-reffiles-fsck.sh +++ b/t/t0602-reffiles-fsck.sh @@ -148,4 +148,70 @@ test_expect_success 'ref name check should work for multiple worktrees' ' ) ' +test_expect_success 'regular ref content should be checked (individual)' ' + test_when_finished "rm -rf repo" && + git init repo && + branch_dir_prefix=.git/refs/heads && + tag_dir_prefix=.git/refs/tags && + cd repo && + test_commit default && + mkdir -p "$branch_dir_prefix/a/b" && + + git refs verify 2>err && + test_must_be_empty err && + + bad_content=$(git rev-parse main)x && + printf "%s" $bad_content >$tag_dir_prefix/tag-bad-1 && + test_must_fail git refs verify 2>err && + cat >expect <<-EOF && + error: refs/tags/tag-bad-1: badRefContent: $bad_content + EOF + rm $tag_dir_prefix/tag-bad-1 && + test_cmp expect err && + + bad_content=xfsazqfxcadas && + printf "%s" $bad_content >$tag_dir_prefix/tag-bad-2 && + test_must_fail git refs verify 2>err && + cat >expect <<-EOF && + error: refs/tags/tag-bad-2: badRefContent: $bad_content + EOF + rm $tag_dir_prefix/tag-bad-2 && + test_cmp expect err && + + bad_content=Xfsazqfxcadas && + printf "%s" $bad_content >$branch_dir_prefix/a/b/branch-bad && + test_must_fail git refs verify 2>err && + cat >expect <<-EOF && + error: refs/heads/a/b/branch-bad: badRefContent: $bad_content + EOF + rm $branch_dir_prefix/a/b/branch-bad && + test_cmp expect err +' + +test_expect_success 'regular ref content should be checked (aggregate)' ' + test_when_finished "rm -rf repo" && + git init repo && + branch_dir_prefix=.git/refs/heads && + tag_dir_prefix=.git/refs/tags && + cd repo && + test_commit default && + mkdir -p "$branch_dir_prefix/a/b" && + + bad_content_1=$(git rev-parse main)x && + bad_content_2=xfsazqfxcadas && + bad_content_3=Xfsazqfxcadas && + printf "%s" $bad_content_1 >$tag_dir_prefix/tag-bad-1 && + printf "%s" $bad_content_2 >$tag_dir_prefix/tag-bad-2 && + printf "%s" $bad_content_3 >$branch_dir_prefix/a/b/branch-bad && + + test_must_fail git refs verify 2>err && + cat >expect <<-EOF && + error: refs/heads/a/b/branch-bad: badRefContent: $bad_content_3 + error: refs/tags/tag-bad-1: badRefContent: $bad_content_1 + error: refs/tags/tag-bad-2: badRefContent: $bad_content_2 + EOF + sort err >sorted_err && + test_cmp expect sorted_err +' + test_done
"git-fsck(1)" has some consistency checks for regular refs. As we want to align the checks "git refs verify" performs with them (and eventually call the unified code that checks refs from both), port the logic "git-fsck" has to "git refs verify". "git-fsck(1)" will report an error when the ref content is invalid. Following this, add a similar check to "git refs verify". Then add a new fsck error message "badRefContent(ERROR)" to represent that a ref has an invalid content. 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 | 3 ++ fsck.h | 1 + refs/files-backend.c | 45 ++++++++++++++++++++++++ t/t0602-reffiles-fsck.sh | 66 +++++++++++++++++++++++++++++++++++ 4 files changed, 115 insertions(+)