Implement ref content consistency check
Re: [PATCH v6 5/9] ref: port git-fsck(1) regular refs check for files backend
In reply to shejialuoQuoted 23 lines · shejialuo
refs/files-backend.c +15
-0@@ -3505,6 +3505,48 @@ typedef int (*files_fsck_refs_fn)(struct ref_store *ref_store,const char *target_name, struct dir_iterator *iter); +static int files_fsck_refs_content (struct ref_store *ref_store, +struct fsck_options *o, +const char *target_name, +struct dir_iterator *iter) +{ +struct strbuf ref_content =STRBUF_INIT ; +struct strbuf referent =STRBUF_INIT ; +struct fsck_ref_report report = {0 }; +unsigned int type =0 ; +int failure_errno =0 ; +struct object_id oid; +int ret =0 ; + + report.path = target_name; +
refs/files-backend.c typedef int (*files_fsck_refs_fn)(struct ref_store *ref_store, · line 3532
Quoted 10 lines
refs/files-backend.c +10
-0+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 '%s': (%s)" , + iter->path.buf,strerror (errno)); + goto cleanup; + }
Let's drop the braces around (%s), we don't print such braces in warning_errno() or die_errno(), either.
Patrick
PATCH v6 6/9 ref: add more strict checks for regular refs
In reply to shejialuoWe have already used "parse_loose_ref_contents" function to check whether the ref content is valid in files backend. However, by using "parse_loose_ref_contents", we allow the ref's content to end with garbage or without a newline.
Even though we never create such loose refs ourselves, we have accepted such loose refs. So, it is entirely possible that some third-party tools may rely on such loose refs being valid. We should not report an error fsck message at current. We should notify the users about such "curiously formatted" loose refs so that adequate care is taken before we decide to tighten the rules in the future.
And it's not suitable either to report a warn fsck message to the user. We don't yet want the "--strict" flag that controls this bit to end up generating errors for such weirdly-formatted reference contents, as we first want to assess whether this retroactive tightening will cause issues for any tools out there. It may cause compatibility issues which may break the repository. So, we add the following two fsck infos to represent the situation where the ref content ends without newline or has trailing garbages:
1. refMissingNewline(INFO): A loose ref that does not end with newline(LF).
2. trailingRefContent(INFO): A loose ref has trailing content.
It might appear that we can't provide the user with any warnings by using FSCK_INFO. However, in "fsck.c::fsck_vreport", we will convert FSCK_INFO to FSCK_WARN and we can still warn the user about these situations when using "git refs verify" without introducing compatibility issues.
--- Documentation/fsck-msgids.txt | 14 ++++++++ fsck.h | 2 ++ refs.c | 2 +- refs/files-backend.c | 26 ++++++++++++-- refs/refs-internal.h | 2 +- t/t0602-reffiles-fsck.sh | 67 +++++++++++++++++++++++++++++++++++ 6 files changed, 108 insertions(+), 5 deletions(-)
Documentation/fsck-msgids.txt +14 -0
@@ -173,6 +173,20 @@ `nullSha1`:: (WARN) Tree contains entries pointing to a null sha1. +`refMissingNewline`:: + (INFO) A loose ref that does not end with newline(LF). As + valid implementations of Git never created such a loose ref + file, it may become an error in the future. Report to the + [email protected] mailing list if you see this error, as + we need to know what tools created such a file. + +`trailingRefContent`:: + (INFO) A loose ref has trailing content. As valid implementations + of Git never created such a loose ref file, it may become an + error in the future. Report to the [email protected] mailing + list if you see this error, as we need to know what tools + created such a file. + `treeNotSorted`:: (ERROR) A tree is not properly sorted.
fsck.h +2 -0
@@ -85,6 +85,8 @@ enum fsck_msg_type { FUNC(MAILMAP_SYMLINK, INFO) \ FUNC(BAD_TAG_NAME, INFO) \ FUNC(MISSING_TAGGER_ENTRY, INFO) \ + FUNC(REF_MISSING_NEWLINE, INFO) \ +FUNC (TRAILING_REF_CONTENT ,INFO ) \ /* ignored (elevated when requested) */ \FUNC (EXTRA_HEADER_ENTRY ,IGNORE )
refs.c +1 -1
@@ -1789,7 +1789,7 @@ static int refs_read_special_head(struct ref_store *ref_store, } result =parse_loose_ref_contents (ref_store->repo->hash_algo, content.buf, - oid, referent, type, failure_errno); + oid, referent, type,NULL , failure_errno); done:strbuf_release (&full_path);
refs/files-backend.c +23 -3
@@ -569,7 +569,7 @@ static int read_ref_internal(struct ref_store *ref_store, const char *refname, buf = sb_contents.buf; ret =parse_loose_ref_contents (ref_store->repo->hash_algo, buf, - oid, referent, type, &myerr); + oid, referent, type,NULL , &myerr); out:if (ret && !myerr)
@@ -606,7 +606,7 @@ static int files_read_symbolic_ref(struct ref_store *ref_store, const char *refnint parse_loose_ref_contents (const struct git_hash_algo *algop,const char *buf,struct object_id *oid,struct strbuf *referent,unsigned int *type, -int *failure_errno) +const char **trailing,int *failure_errno) {const char *p;if (skip_prefix (buf,"ref:" , &buf)) {
@@ -628,6 +628,10 @@ int parse_loose_ref_contents(const struct git_hash_algo *algop, *failure_errno =EINVAL ;return -1 ; } + +if (trailing) + *trailing = p; +return 0 ; }
@@ -3513,6 +3517,7 @@ static int files_fsck_refs_content(struct ref_store *ref_store,struct strbuf ref_content =STRBUF_INIT ;struct strbuf referent =STRBUF_INIT ;struct fsck_ref_report report = {0 }; +const char *trailing =NULL ;unsigned int type =0 ;int failure_errno =0 ;struct object_id oid;
@@ -3533,7 +3538,7 @@ static int files_fsck_refs_content(struct ref_store *ref_store,if (parse_loose_ref_contents (ref_store->repo->hash_algo, ref_content.buf, &oid, &referent, - &type, &failure_errno)) { + &type, &trailing, &failure_errno)) {strbuf_rtrim (&ref_content); ret = fsck_report_ref(o, &report, FSCK_MSG_BAD_REF_CONTENT,
@@ -3541,6 +3546,21 @@ static int files_fsck_refs_content(struct ref_store *ref_store, goto cleanup; } +if (!(type &REF_ISSYMREF )) { +if (!*trailing) { + ret =fsck_report_ref (o, &report, +FSCK_MSG_REF_MISSING_NEWLINE , +"misses LF at the end" ); + goto cleanup; + } +if (*trailing !='\n' || *(trailing +1 )) { + ret =fsck_report_ref (o, &report, +FSCK_MSG_TRAILING_REF_CONTENT , +"has trailing garbage: '%s'" , trailing); + goto cleanup; + } + } + cleanup:strbuf_release (&ref_content);strbuf_release (&referent);
refs/refs-internal.h +1 -1
@@ -716,7 +716,7 @@ struct ref_store {int parse_loose_ref_contents (const struct git_hash_algo *algop,const char *buf,struct object_id *oid,struct strbuf *referent,unsigned int *type, -int *failure_errno); +const char **trailing,int *failure_errno);/* * Fill in the generic part of refs and add it to our collection of
t/t0602-reffiles-fsck.sh +67 -0
91 lines not on this page · raw message
Signature · 1 line
2.47.0
PATCH v6 7/9 ref: add basic symref content check for files backend
In reply to shejialuoWe have code that checks regular ref contents, but we do not yet check the contents of symbolic refs. By using "parse_loose_ref_content" for symbolic refs, we will get the information of the "referent".
We do not need to check the "referent" by opening the file. This is because if "referent" exists in the file system, we will eventually check its correctness by inspecting every file in the "refs" directory. If the "referent" does not exist in the filesystem, this is OK as it is seen as the dangling symref.
So we just need to check the "referent" string content. A regular ref could be accepted as a textual symref if it begins with "ref:", followed by zero or more whitespaces, followed by the full refname, followed only by whitespace characters. However, we always write a single SP after "ref:" and a single LF after the refname. It may seem that we should report a fsck error message when the "referent" does not apply above rules and we should not be so aggressive because third-party reimplementations of Git may have taken advantage of the looser syntax. Put it more specific, we accept the following contents:
1. "ref: refs/heads/master "
2. "ref: refs/heads/master \n \n"
3. "ref: refs/heads/master\n\n"
When introducing the regular ref content checks, we created two fsck infos "refMissingNewline" and "trailingRefContent" which exactly represents above situations. So we will reuse these two fsck messages to write checks to info the user about these situations.
But we do not allow any other trailing garbage. The followings are bad symref contents which will be reported as fsck error by "git-fsck(1)".
1. "ref: refs/heads/master garbage\n"
2. "ref: refs/heads/master \n\n\n garbage "
And we introduce a new "badReferentName(ERROR)" fsck message to report above errors by using "is_root_ref" and "check_refname_format" to check the "referent". Since both "is_root_ref" and "check_refname_format" don't work with whitespaces, we use the trimmed version of "referent" with these functions.
In order to add checks, we will do the following things:
1. Record the untrimmed length "orig_len" and untrimmed last byte "orig_last_byte".
2. Use "strbuf_rtrim" to trim the whitespaces or newlines to make sure "is_root_ref" and "check_refname_format" won't be failed by them.
3. Use "orig_len" and "orig_last_byte" to check whether the "referent" misses '\n' at the end or it has trailing whitespaces or newlines.
--- Documentation/fsck-msgids.txt | 3 + fsck.h | 1 + refs/files-backend.c | 40 ++++++++++++ t/t0602-reffiles-fsck.sh | 111 ++++++++++++++++++++++++++++++++++ 4 files changed, 155 insertions(+)
Documentation/fsck-msgids.txt +3 -0
@@ -28,6 +28,9 @@ `badRefName`:: (ERROR) A ref has an invalid format. +`badReferentName`:: + (ERROR) The referent name of a symref is invalid. + `badTagName`:: (INFO) A tag has an invalid format.
fsck.h +1 -0
@@ -34,6 +34,7 @@ enum fsck_msg_type { FUNC(BAD_REF_CONTENT, ERROR) \ FUNC(BAD_REF_FILETYPE, ERROR) \ FUNC(BAD_REF_NAME, ERROR) \ + FUNC(BAD_REFERENT_NAME, ERROR) \ FUNC(BAD_TIMEZONE, ERROR) \ FUNC(BAD_TREE, ERROR) \FUNC (BAD_TREE_SHA1 ,ERROR ) \
refs/files-backend.c +40 -0
@@ -3509,6 +3509,43 @@ typedef int (*files_fsck_refs_fn)(struct ref_store *ref_store,const char *target_name, struct dir_iterator *iter); +static int files_fsck_symref_target (struct fsck_options *o, +struct fsck_ref_report *report, +struct strbuf *referent) +{ +char orig_last_byte; +size_t orig_len; +int ret =0 ; + + orig_len = referent->len; + orig_last_byte = referent->buf[orig_len -1 ]; +strbuf_rtrim (referent); + +if (!is_root_ref (referent->buf) && +check_refname_format (referent->buf,0 )) { + ret =fsck_report_ref (o, report, +FSCK_MSG_BAD_REFERENT_NAME , +"points to invalid refname '%s'" , referent->buf); + goto out; + } + +if (referent->len == orig_len || + (referent->len < orig_len && orig_last_byte !='\n' )) { + ret =fsck_report_ref (o, report, +FSCK_MSG_REF_MISSING_NEWLINE , +"misses LF at the end" ); + } + +if (referent->len != orig_len && referent->len != orig_len -1 ) { + ret =fsck_report_ref (o, report, +FSCK_MSG_TRAILING_REF_CONTENT , +"has trailing whitespaces or newlines" ); + } + +out: +return ret; +} + static int files_fsck_refs_content(struct ref_store *ref_store, struct fsck_options *o, const char *target_name,
@@ -3559,6 +3596,9 @@ static int files_fsck_refs_content(struct ref_store *ref_store, "has trailing garbage: '%s'", trailing); goto cleanup; } + }else { + ret =files_fsck_symref_target (o, &report, &referent); + goto cleanup; } cleanup:
t/t0602-reffiles-fsck.sh +111 -0
123 lines not on this page · raw message
Signature · 1 line
2.47.0
PATCH v6 8/9 ref: check whether the target of the symref is a ref
In reply to shejialuoIdeally, we want to the users use "git symbolic-ref" to create symrefs instead of writing raw contents into the filesystem. However, "git symbolic-ref" is strict with the refname but not strict with the referent. For example, we can make the "referent" located at the "$(gitdir)/logs/aaa" and manually write the content into this where we can still successfully parse this symref by using "git rev-parse".
$ git init repo && cd repo && git commit --allow-empty -mx $ git symbolic-ref refs/heads/test logs/aaa $ echo $(git rev-parse HEAD) > .git/logs/aaa $ git rev-parse test
We may need to add some restrictions for "referent" parameter when using "git symbolic-ref" to create symrefs because ideally all the nonpseudo-refs should be located under the "refs" directory and we may tighten this in the future.
In order to tell the user we may tighten the above situation, create a new fsck message "symrefTargetIsNotARef" to notify the user that this may become an error in the future.
--- Documentation/fsck-msgids.txt | 9 +++++++++ fsck.h | 1 + refs/files-backend.c | 14 ++++++++++++-- t/t0602-reffiles-fsck.sh | 28 ++++++++++++++++++++++++++++ 4 files changed, 50 insertions(+), 2 deletions(-)
Documentation/fsck-msgids.txt +9 -0
@@ -183,6 +183,15 @@ [email protected] mailing list if you see this error, as we need to know what tools created such a file. +`symrefTargetIsNotARef`:: + (INFO) The target of a symbolic reference points neither to + a root reference nor to a reference starting with "refs/". + Although we allow create a symref pointing to the referent which + is outside the "ref" by using `git symbolic-ref`, we may tighten + the rule in the future. Report to the [email protected] + mailing list if you see this error, as we need to know what tools + created such a file. + `trailingRefContent`:: (INFO) A loose ref has trailing content. As valid implementations of Git never created such a loose ref file, it may become an
fsck.h +1 -0
@@ -87,6 +87,7 @@ enum fsck_msg_type { FUNC(BAD_TAG_NAME, INFO) \ FUNC(MISSING_TAGGER_ENTRY, INFO) \ FUNC(REF_MISSING_NEWLINE, INFO) \ + FUNC(SYMREF_TARGET_IS_NOT_A_REF, INFO) \FUNC (TRAILING_REF_CONTENT ,INFO ) \ /* ignored (elevated when requested) */ \FUNC (EXTRA_HEADER_ENTRY ,IGNORE )
refs/files-backend.c +12 -2
@@ -3513,6 +3513,7 @@ static int files_fsck_symref_target(struct fsck_options *o,struct fsck_ref_report *report, struct strbuf *referent) { + int is_referent_root;char orig_last_byte;size_t orig_len;int ret =0 ;
@@ -3521,8 +3522,17 @@ static int files_fsck_symref_target(struct fsck_options *o, orig_last_byte = referent->buf[orig_len -1 ];strbuf_rtrim (referent); -if (!is_root_ref (referent->buf) && -check_refname_format (referent->buf,0 )) { + is_referent_root =is_root_ref (referent->buf); +if (!is_referent_root && + !starts_with (referent->buf,"refs/" ) && + !starts_with (referent->buf,"worktrees/" )) { + ret =fsck_report_ref (o, report, +FSCK_MSG_SYMREF_TARGET_IS_NOT_A_REF , +"points to non-ref target '%s'" , referent->buf); + + } + +if (!is_referent_root &&check_refname_format (referent->buf,0 )) { ret =fsck_report_ref (o, report,FSCK_MSG_BAD_REFERENT_NAME ,"points to invalid refname '%s'" , referent->buf);
t/t0602-reffiles-fsck.sh +28 -0
@@ -392,6 +392,34 @@ test_expect_success 'textual symref content should be checked (aggregate)' ' test_cmp expect sorted_err ' +test_expect_success 'the target of the textual symref should be checked' ' + 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" && + + printf "ref: HEAD\n" >$branch_dir_prefix/branch-good && + git refs verify 2>err && + rm $branch_dir_prefix/branch-good && + test_must_be_empty err && + + printf "ref: refs/foo\n" >$branch_dir_prefix/branch-good && + git refs verify 2>err && + rm $branch_dir_prefix/branch-good && + test_must_be_empty err && + + printf "ref: refs-back/heads/main\n" >$branch_dir_prefix/branch-bad-1 && + git refs verify 2>err && + cat >expect <<-EOF && + warning: refs/heads/branch-bad-1: symrefTargetIsNotARef: points to non-ref target '\''refs-back/heads/main'\'' + EOF + rm $branch_dir_prefix/branch-bad-1 && + test_cmp expect err +' + test_expect_success 'ref content checks should work with worktrees' ' test_when_finished "rm -rf repo" && git init repo &&
Signature · 1 line
2.47.0
PATCH v6 9/9 ref: add symlink ref content check for files backend
In reply to shejialuoBesides the textual symref, we also allow symbolic links as the symref. So, we should also provide the consistency check as what we have done for textual symref. And also we consider deprecating writing the symbolic links. We first need to access whether symbolic links still be used. So, add a new fsck message "symlinkRef(INFO)" to tell the user be aware of this information.
We have already introduced "files_fsck_symref_target". We should reuse this function to handle the symrefs which use legacy symbolic links. We should not check the trailing garbage for symbolic refs. Add a new parameter "symbolic_link" to disable some checks which should only be executed for textual symrefs.
And we need to also generate the "referent" parameter for reusing "files_fsck_symref_target" by the following steps:
1. Use "strbuf_add_real_path" to resolve the symlink and get the absolute path "ref_content" which the symlink ref points to.
2. Generate the absolute path "abs_gitdir" of "gitdir" and combine "ref_content" and "abs_gitdir" to extract the relative path "relative_referent_path".
3. If "ref_content" is outside of "gitdir", we just set "referent" with "ref_content". Instead, we set "referent" with "relative_referent_path".
--- Documentation/fsck-msgids.txt | 6 +++++ fsck.h | 1 + refs/files-backend.c | 38 +++++++++++++++++++++++++---- t/t0602-reffiles-fsck.sh | 45 +++++++++++++++++++++++++++++++++++ 4 files changed, 86 insertions(+), 4 deletions(-)
Documentation/fsck-msgids.txt +6 -0
@@ -183,6 +183,12 @@ [email protected] mailing list if you see this error, as we need to know what tools created such a file. +`symlinkRef`:: + (INFO) A symbolic link is used as a symref. Report to the + [email protected] mailing list if you see this error, as we + are assessing the feasibility of dropping the support to drop + creating symbolic links as symrefs. + `symrefTargetIsNotARef`:: (INFO) The target of a symbolic reference points neither to a root reference nor to a reference starting with "refs/".
fsck.h +1 -0
@@ -86,6 +86,7 @@ enum fsck_msg_type { FUNC(MAILMAP_SYMLINK, INFO) \ FUNC(BAD_TAG_NAME, INFO) \ FUNC(MISSING_TAGGER_ENTRY, INFO) \ + FUNC(SYMLINK_REF, INFO) \ FUNC(REF_MISSING_NEWLINE, INFO) \ FUNC(SYMREF_TARGET_IS_NOT_A_REF, INFO) \FUNC (TRAILING_REF_CONTENT ,INFO ) \
refs/files-backend.c +34 -4
@@ -1,6 +1,7 @@#define USE_THE_REPOSITORY_VARIABLE #include "../git-compat-util.h" +#include "../abspath.h" #include "../config.h" #include "../copy.h" #include "../environment.h"
@@ -3511,7 +3512,8 @@ typedef int (*files_fsck_refs_fn)(struct ref_store *ref_store,static int files_fsck_symref_target (struct fsck_options *o,struct fsck_ref_report *report, -struct strbuf *referent) +struct strbuf *referent, +unsigned int symbolic_link) {int is_referent_root;char orig_last_byte;
@@ -3520,7 +3522,8 @@ static int files_fsck_symref_target(struct fsck_options *o, orig_len = referent->len; orig_last_byte = referent->buf[orig_len -1 ]; -strbuf_rtrim (referent); +if (!symbolic_link) +strbuf_rtrim (referent); is_referent_root =is_root_ref (referent->buf); if (!is_referent_root &&
@@ -3539,6 +3542,9 @@ static int files_fsck_symref_target(struct fsck_options *o, goto out; } +if (symbolic_link) + goto out; +if (referent->len == orig_len || (referent->len < orig_len && orig_last_byte !='\n' )) { ret = fsck_report_ref(o, report,
@@ -3562,6 +3568,7 @@ static int files_fsck_refs_content(struct ref_store *ref_store, struct dir_iterator *iter) {struct strbuf ref_content =STRBUF_INIT ; +struct strbuf abs_gitdir =STRBUF_INIT ;struct strbuf referent =STRBUF_INIT ;struct fsck_ref_report report = {0 };const char *trailing =NULL ;
@@ -3572,8 +3579,30 @@ static int files_fsck_refs_content(struct ref_store *ref_store, report.path = target_name; -if (S_ISLNK (iter->st.st_mode)) +if (S_ISLNK (iter->st.st_mode)) { +const char * relative_referent_path =NULL ; + + ret =fsck_report_ref (o, &report, +FSCK_MSG_SYMLINK_REF , +"use deprecated symbolic link for symref" ); + +strbuf_add_absolute_path (&abs_gitdir, ref_store->gitdir); +strbuf_normalize_path (&abs_gitdir); +if (!is_dir_sep (abs_gitdir.buf[abs_gitdir.len -1 ])) +strbuf_addch (&abs_gitdir,'/' ); + +strbuf_add_real_path (&ref_content, iter->path.buf); +skip_prefix (ref_content.buf, abs_gitdir.buf, + &relative_referent_path); + +if (relative_referent_path) +strbuf_addstr (&referent, relative_referent_path); +else +strbuf_addbuf (&referent, &ref_content); + + ret |=files_fsck_symref_target (o, &report, &referent,1 ); goto cleanup; + }if (strbuf_read_file (&ref_content, iter->path.buf,0 ) <0 ) { ret = fsck_report_ref(o, &report,
@@ -3607,13 +3636,14 @@ static int files_fsck_refs_content(struct ref_store *ref_store, goto cleanup; } }else { - ret =files_fsck_symref_target (o, &report, &referent); + ret =files_fsck_symref_target (o, &report, &referent,0 ); goto cleanup; } cleanup:strbuf_release (&ref_content);strbuf_release (&referent); +strbuf_release (&abs_gitdir);return ret; }
t/t0602-reffiles-fsck.sh +45 -0
51 lines not on this page · raw message
Signature · 1 line
2.47.0
Re: [PATCH v6 0/9] add ref content check for files backend
In reply to shejialuoQuoted 3 lines · shejialuo
Hi All:
This new version updates the following things.
I am assuming that this new round was rebased onto the tip of 'master', since I could not apply it on top of its original base
b3d175409d9 (Merge branch 'sj/ref-fsck', 2024-08-16)
In the future, please indicate when you rebase your series so that I know what the correct base is for that round.
Thanks,
Taylor
Re: [PATCH v6 0/9] add ref content check for files backend
Quoted 5 lines · Taylor Blau
Quoted 3 lines · shejialuo
Hi All:
This new version updates the following things.
Quoted 8 lines
I am assuming that this new round was rebased onto the tip of 'master', since I could not apply it on top of its original base
b3d175409d9(Merge branch 'sj/ref-fsck', 2024-08-16)In the future, please indicate when you rebase your series so that I know what the correct base is for that round.
Sorry for that Taylor. I have told Junio that I rebased the series in the previous version. And I forgot you have become the intermediate maintainer and didn't provide this information for you.
Thanks,
Jiauo
Quoted 2 lines
Thanks,
Taylor
Re: [PATCH v6 0/9] add ref content check for files backend
In reply to shejialuoQuoted 11 lines · shejialuo
shejialuo (9):
ref: initialize "fsck_ref_report" with zero
ref: check the full refname instead of basename
ref: initialize target name outside of check functions
ref: support multiple worktrees check for refs
ref: port git-fsck(1) regular refs check for files backend
ref: add more strict checks for regular refs
ref: add basic symref content check for files backend
ref: check whether the target of the symref is a ref
ref: add symlink ref content check for files backend
Quoted 12 lines
Documentation/fsck-msgids.txt | 35 +++ builtin/refs.c | 12 +- fsck.h | 6 + refs.c | 7 +- refs.h | 3 +- refs/debug.c | 5 +- refs/files-backend.c | 187 ++++++++++++-- refs/packed-backend.c | 8 +- refs/refs-internal.h | 5 +- refs/reftable-backend.c | 3 +- t/t0602-reffiles-fsck.sh | 457 +++++++++++++++++++++++++++++++++- 11 files changed, 693 insertions(+), 35 deletions(-)
Great, thanks for the new round. Looking at the inter-diff, it looks like this round also needs a fresh review. I'm catching up on new threads from the weekend, so I'll put this on my review queue. But in the meantime, if your mentors can look at it, that would be much appreciated.
Thanks,
Taylor
v7 cover add ref content check for files backend
In reply to shejialuoHi All:
This new version solves the follow problems:
1. Enhance the commit message suggested by Patrick.
2. Rename "target_name" to "refname".
3. Enhance the shell scripts to use for in to avoid repetition. And this is the main change of this new version.
Thanks,
Jialuo
shejialuo (9):
ref: initialize "fsck_ref_report" with zero
ref: check the full refname instead of basename
ref: initialize ref name outside of check functions
ref: support multiple worktrees check for refs
ref: port git-fsck(1) regular refs check for files backend
ref: add more strict checks for regular refs
ref: add basic symref content check for files backend
ref: check whether the target of the symref is a ref
ref: add symlink ref content check for files backend
Documentation/fsck-msgids.txt | 35 +++ builtin/refs.c | 10 +- fsck.h | 6 + refs.c | 7 +- refs.h | 3 +- refs/debug.c | 5 +- refs/files-backend.c | 190 ++++++++++++-- refs/packed-backend.c | 8 +- refs/refs-internal.h | 5 +- refs/reftable-backend.c | 3 +- t/t0602-reffiles-fsck.sh | 480 +++++++++++++++++++++++++++++++--- 11 files changed, 690 insertions(+), 62 deletions(-)
Range-diff against v6:
1: 319f384f1c = 1: bfb2a21af4 ref: initialize "fsck_ref_report" with zero
2: 8662fc9679 ! 2: 9efc83f7ea ref: check the full refname instead of basename
@@ Commit message
In order to fix the above problem, enhance "files_fsck_refs_name" to use
the full name for "check_refname_format". Then, replace the tests which
- are related to "@" and add tests to exercise the above situations.
+ are related to "@" and add tests to exercise the above situations using
+ for loop to avoid repetition.
Mentored-by: Patrick Steinhardt <[email protected]>
Mentored-by: Karthik Nayak <[email protected]>
@@ refs/files-backend.c: static int files_fsck_refs_name(struct ref_store *ref_stor
goto cleanup;
- if (check_refname_format(iter->basename, REFNAME_ALLOW_ONELEVEL)) {
++ /*
++ * This works right now because we never check the root refs.
++ */
+ strbuf_addf(&sb, "%s/%s", refs_check_dir, iter->relative_path);
+ if (check_refname_format(sb.buf, 0)) {
struct fsck_ref_report report = { 0 };
@@ refs/files-backend.c: static int files_fsck_refs_name(struct ref_store *ref_stor
## t/t0602-reffiles-fsck.sh ##
@@ t/t0602-reffiles-fsck.sh: test_expect_success 'ref name should be checked' '
- git tag tag-2 &&
- git tag multi_hierarchy/tag-2 &&
+ cd repo &&
-+ cp $branch_dir_prefix/branch-1 $branch_dir_prefix/@ &&
-+ git refs verify 2>err &&
-+ cat >expect <<-EOF &&
-+ EOF
-+ test_must_be_empty err &&
-+ rm $branch_dir_prefix/@ &&
-+
- cp $branch_dir_prefix/branch-1 $branch_dir_prefix/.branch-1 &&
- test_must_fail git refs verify 2>err &&
- cat >expect <<-EOF &&
-@@ t/t0602-reffiles-fsck.sh: test_expect_success 'ref name should be checked' '
- rm $branch_dir_prefix/.branch-1 &&
- test_cmp expect err &&
+ git commit --allow-empty -m initial &&
+- git checkout -b branch-1 &&
+- git tag tag-1 &&
+- git commit --allow-empty -m second &&
+- git checkout -b branch-2 &&
+- git tag tag-2 &&
+- git tag multi_hierarchy/tag-2 &&
++ git checkout -b default-branch &&
++ git tag default-tag &&
++ git tag multi_hierarchy/default-tag &&
+- cp $branch_dir_prefix/branch-1 $branch_dir_prefix/.branch-1 &&
+- test_must_fail git refs verify 2>err &&
+- cat >expect <<-EOF &&
+- error: refs/heads/.branch-1: badRefName: invalid refname format
+- EOF
+- rm $branch_dir_prefix/.branch-1 &&
+- test_cmp expect err &&
+-
- cp $branch_dir_prefix/branch-1 $branch_dir_prefix/@ &&
-+ cp $branch_dir_prefix/branch-1 $branch_dir_prefix/'\'' branch-1'\'' &&
- test_must_fail git refs verify 2>err &&
- cat >expect <<-EOF &&
+- test_must_fail git refs verify 2>err &&
+- cat >expect <<-EOF &&
- error: refs/heads/@: badRefName: invalid refname format
-+ error: refs/heads/ branch-1: badRefName: invalid refname format
- EOF
-- rm $branch_dir_prefix/@ &&
-+ rm $branch_dir_prefix/'\'' branch-1'\'' &&
- test_cmp expect err &&
+- EOF
++ cp $branch_dir_prefix/default-branch $branch_dir_prefix/@ &&
++ git refs verify 2>err &&
++ test_must_be_empty err &&
+ rm $branch_dir_prefix/@ &&
+- test_cmp expect err &&
- cp $tag_dir_prefix/multi_hierarchy/tag-2 $tag_dir_prefix/multi_hierarchy/@ &&
-+ cp $tag_dir_prefix/multi_hierarchy/tag-2 $tag_dir_prefix/multi_hierarchy/'\''~tag-2'\'' &&
- test_must_fail git refs verify 2>err &&
- cat >expect <<-EOF &&
+- test_must_fail git refs verify 2>err &&
+- cat >expect <<-EOF &&
- error: refs/tags/multi_hierarchy/@: badRefName: invalid refname format
-+ error: refs/tags/multi_hierarchy/~tag-2: badRefName: invalid refname format
- EOF
+- EOF
- rm $tag_dir_prefix/multi_hierarchy/@ &&
-+ rm $tag_dir_prefix/multi_hierarchy/'\''~tag-2'\'' &&
- test_cmp expect err &&
+- test_cmp expect err &&
+-
+- cp $tag_dir_prefix/tag-1 $tag_dir_prefix/tag-1.lock &&
++ cp $tag_dir_prefix/default-tag $tag_dir_prefix/tag-1.lock &&
+ git refs verify 2>err &&
+ rm $tag_dir_prefix/tag-1.lock &&
+ test_must_be_empty err &&
- cp $tag_dir_prefix/tag-1 $tag_dir_prefix/tag-1.lock &&
-@@ t/t0602-reffiles-fsck.sh: test_expect_success 'ref name should be checked' '
+- cp $tag_dir_prefix/tag-1 $tag_dir_prefix/.lock &&
++ cp $tag_dir_prefix/default-tag $tag_dir_prefix/.lock &&
+ test_must_fail git refs verify 2>err &&
+ cat >expect <<-EOF &&
error: refs/tags/.lock: badRefName: invalid refname format
EOF
rm $tag_dir_prefix/.lock &&
+- test_cmp expect err
+ test_cmp expect err &&
+
-+ mkdir $tag_dir_prefix/'\''~new-feature'\'' &&
-+ cp $tag_dir_prefix/tag-1 $tag_dir_prefix/'\''~new-feature'\''/tag-1 &&
-+ test_must_fail git refs verify 2>err &&
-+ cat >expect <<-EOF &&
-+ error: refs/tags/~new-feature/tag-1: badRefName: invalid refname format
-+ EOF
-+ rm -rf $tag_dir_prefix/'\''~new-feature'\'' &&
- test_cmp expect err
++ for refname in ".refname-starts-with-dot" "~refname-has-stride"
++ do
++ cp $branch_dir_prefix/default-branch "$branch_dir_prefix/$refname" &&
++ test_must_fail git refs verify 2>err &&
++ cat >expect <<-EOF &&
++ error: refs/heads/$refname: badRefName: invalid refname format
++ EOF
++ rm "$branch_dir_prefix/$refname" &&
++ test_cmp expect err || return 1
++ done &&
++
++ for refname in ".refname-starts-with-dot" "~refname-has-stride"
++ do
++ cp $tag_dir_prefix/default-tag "$tag_dir_prefix/$refname" &&
++ test_must_fail git refs verify 2>err &&
++ cat >expect <<-EOF &&
++ error: refs/tags/$refname: badRefName: invalid refname format
++ EOF
++ rm "$tag_dir_prefix/$refname" &&
++ test_cmp expect err || return 1
++ done &&
++
++ for refname in ".refname-starts-with-dot" "~refname-has-stride"
++ do
++ cp $tag_dir_prefix/multi_hierarchy/default-tag "$tag_dir_prefix/multi_hierarchy/$refname" &&
++ test_must_fail git refs verify 2>err &&
++ cat >expect <<-EOF &&
++ error: refs/tags/multi_hierarchy/$refname: badRefName: invalid refname format
++ EOF
++ rm "$tag_dir_prefix/multi_hierarchy/$refname" &&
++ test_cmp expect err || return 1
++ done &&
++
++ for refname in ".refname-starts-with-dot" "~refname-has-stride"
++ do
++ mkdir "$branch_dir_prefix/$refname" &&
++ cp $branch_dir_prefix/default-branch "$branch_dir_prefix/$refname/default-branch" &&
++ test_must_fail git refs verify 2>err &&
++ cat >expect <<-EOF &&
++ error: refs/heads/$refname/default-branch: badRefName: invalid refname format
++ EOF
++ rm -r "$branch_dir_prefix/$refname" &&
++ test_cmp expect err || return 1
++ done
'
+ test_expect_success 'ref name check should be adapted into fsck messages' '
+ test_when_finished "rm -rf repo" &&
+ git init repo &&
+ branch_dir_prefix=.git/refs/heads &&
+- tag_dir_prefix=.git/refs/tags &&
+ cd repo &&
+ git commit --allow-empty -m initial &&
+ git checkout -b branch-1 &&
+- git tag tag-1 &&
+- git commit --allow-empty -m second &&
+- git checkout -b branch-2 &&
+- git tag tag-2 &&
+
+ cp $branch_dir_prefix/branch-1 $branch_dir_prefix/.branch-1 &&
+ git -c fsck.badRefName=warn refs verify 2>err &&
@@ t/t0602-reffiles-fsck.sh: test_expect_success 'ref name check should be adapted into fsck messages' '
rm $branch_dir_prefix/.branch-1 &&
test_cmp expect err &&
- cp $branch_dir_prefix/branch-1 $branch_dir_prefix/@ &&
-+ cp $branch_dir_prefix/branch-1 $branch_dir_prefix/'\''~branch-1'\'' &&
++ cp $branch_dir_prefix/branch-1 $branch_dir_prefix/.branch-1 &&
git -c fsck.badRefName=ignore refs verify 2>err &&
test_must_be_empty err
'
3: 96144756fe ! 3: 5ea7d18203 ref: initialize target name outside of check functions
@@ Metadata
Author: shejialuo <[email protected]>
## Commit message ##
- ref: initialize target name outside of check functions
+ ref: initialize ref name outside of check functions
We passes "refs_check_dir" to the "files_fsck_refs_name" function which
allows it to create the checked ref name later. However, when we
- introduce a new check function, we have to re-calculate the target name.
- It's bad for us to do repeat calculation. Instead, we should calculate
- it only once and pass the target name to the check functions.
+ introduce a new check function, we have to allocate redundant memory and
+ re-calculate the ref name. It's bad for us to allocate redundant memory
+ and duplicate logic. Instead, we should allocate and calculate it only
+ once and pass the ref name to the check functions.
In order not to do repeat calculation, rename "refs_check_dir" to
- "target_name". And in "files_fsck_refs_dir", create a new strbuf
- "target_name", thus whenever we handle a new target, calculate the
- name and call the check functions one by one.
+ "refname". And in "files_fsck_refs_dir", create a new strbuf "refname",
+ thus whenever we handle a new ref, calculate the name and call the check
+ functions one by one.
Mentored-by: Patrick Steinhardt <[email protected]>
Mentored-by: Karthik Nayak <[email protected]>
@@ refs/files-backend.c: static int files_ref_store_remove_on_disk(struct ref_store
typedef int (*files_fsck_refs_fn)(struct ref_store *ref_store,
struct fsck_options *o,
- const char *refs_check_dir,
-+ const char *target_name,
++ const char *refname,
struct dir_iterator *iter);
static int files_fsck_refs_name(struct ref_store *ref_store UNUSED,
struct fsck_options *o,
- const char *refs_check_dir,
-+ const char *target_name,
++ const char *refname,
struct dir_iterator *iter)
{
struct strbuf sb = STRBUF_INIT;
@@ refs/files-backend.c: static int files_fsck_refs_name(struct ref_store *ref_store UNUSED,
- if (iter->basename[0] != '.' && ends_with(iter->basename, ".lock"))
- goto cleanup;
-
+ /*
+ * This works right now because we never check the root refs.
+ */
- strbuf_addf(&sb, "%s/%s", refs_check_dir, iter->relative_path);
- if (check_refname_format(sb.buf, 0)) {
-+ if (check_refname_format(target_name, 0)) {
++ if (check_refname_format(refname, 0)) {
struct fsck_ref_report report = { 0 };
- report.path = sb.buf;
-+ report.path = target_name;
++ report.path = refname;
ret = fsck_report_ref(o, &report,
FSCK_MSG_BAD_REF_NAME,
"invalid refname format");
@@ refs/files-backend.c: static int files_fsck_refs_dir(struct ref_store *ref_store
const char *refs_check_dir,
files_fsck_refs_fn *fsck_refs_fn)
{
-+ struct strbuf target_name = STRBUF_INIT;
++ struct strbuf refname = STRBUF_INIT;
struct strbuf sb = STRBUF_INIT;
struct dir_iterator *iter;
int iter_status;
@@ refs/files-backend.c: static int files_fsck_refs_dir(struct ref_store *ref_store
continue;
} else if (S_ISREG(iter->st.st_mode) ||
S_ISLNK(iter->st.st_mode)) {
-+ strbuf_reset(&target_name);
-+ strbuf_addf(&target_name, "%s/%s", refs_check_dir,
++ strbuf_reset(&refname);
++ strbuf_addf(&refname, "%s/%s", refs_check_dir,
+ iter->relative_path);
+
if (o->verbose)
- fprintf_ln(stderr, "Checking %s/%s",
- refs_check_dir, iter->relative_path);
-+ fprintf_ln(stderr, "Checking %s", target_name.buf);
++ fprintf_ln(stderr, "Checking %s", refname.buf);
+
for (size_t i = 0; fsck_refs_fn[i]; i++) {
- if (fsck_refs_fn[i](ref_store, o, refs_check_dir, iter))
-+ if (fsck_refs_fn[i](ref_store, o, target_name.buf, iter))
++ if (fsck_refs_fn[i](ref_store, o, refname.buf, iter))
ret = -1;
}
} else {
@@ refs/files-backend.c: static int files_fsck_refs_dir(struct ref_store *ref_store
out:
strbuf_release(&sb);
-+ strbuf_release(&target_name);
++ strbuf_release(&refname);
return ret;
} 4: b396bf6bc2 ! 4: cb4669b64d ref: support multiple worktrees check for refs
@@ Commit message
ref: support multiple worktrees check for refs
We have already set up the infrastructure to check the consistency for
- refs, but we do not support multiple worktrees. As we decide to add more
- checks for ref content, we need to set up support for multiple
- worktrees.
+ refs, but we do not support multiple worktrees. However, "git-fsck(1)"
+ will check the refs of worktrees. As we decide to get feature parity
+ with "git-fsck(1)", we need to set up support for multiple worktrees.
Because each worktree has its own specific refs, instead of just showing
the users "refs/worktree/foo", we need to display the full name such as
@@ builtin/refs.c: static int cmd_refs_migrate(int argc, const char **argv, const c
static int cmd_refs_verify(int argc, const char **argv, const char *prefix)
{
struct fsck_options fsck_refs_options = FSCK_REFS_OPTIONS_DEFAULT;
-+ struct worktree **worktrees, **p;
++ struct worktree **worktrees;
const char * const verify_usage[] = {
REFS_VERIFY_USAGE,
NULL,
@@ builtin/refs.c: static int cmd_refs_verify(int argc, const char **argv, const ch
- ret = refs_fsck(get_main_ref_store(the_repository), &fsck_refs_options);
+ worktrees = get_worktrees();
-+ for (p = worktrees; *p; p++) {
-+ struct worktree *wt = *p;
-+ ret |= refs_fsck(get_worktree_ref_store(wt), &fsck_refs_options, wt);
-+ }
-+
++ for (size_t i = 0; worktrees[i]; i++)
++ ret |= refs_fsck(get_worktree_ref_store(worktrees[i]),
++ &fsck_refs_options, worktrees[i]);
fsck_options_clear(&fsck_refs_options);
+ free_worktrees(worktrees);
@@ refs/files-backend.c: static int files_fsck_refs_name(struct ref_store *ref_stor
+ struct worktree *wt,
files_fsck_refs_fn *fsck_refs_fn)
{
- struct strbuf target_name = STRBUF_INIT;
+ struct strbuf refname = STRBUF_INIT;
@@ refs/files-backend.c: static int files_fsck_refs_dir(struct ref_store *ref_store,
} else if (S_ISREG(iter->st.st_mode) ||
S_ISLNK(iter->st.st_mode)) {
- strbuf_reset(&target_name);
+ strbuf_reset(&refname);
+
+ if (!is_main_worktree(wt))
-+ strbuf_addf(&target_name, "worktrees/%s/", wt->id);
- strbuf_addf(&target_name, "%s/%s", refs_check_dir,
++ strbuf_addf(&refname, "worktrees/%s/", wt->id);
+ strbuf_addf(&refname, "%s/%s", refs_check_dir,
iter->relative_path);
@@ refs/files-backend.c: static int files_fsck_refs_dir(struct ref_store *ref_store,
@@ t/t0602-reffiles-fsck.sh: test_expect_success 'ref name check should be adapted
+ sort err >sorted_err &&
+ test_cmp expect sorted_err &&
+
-+ (
-+ cd worktree-1 &&
-+ test_must_fail git refs verify 2>err &&
-+ cat >expect <<-EOF &&
-+ error: worktrees/worktree-1/refs/worktree/ branch-5: badRefName: invalid refname format
-+ error: worktrees/worktree-2/refs/worktree/~branch-6: badRefName: invalid refname format
-+ EOF
-+ sort err >sorted_err &&
-+ test_cmp expect sorted_err
-+ ) &&
-+
-+ (
-+ cd worktree-2 &&
-+ test_must_fail git refs verify 2>err &&
-+ cat >expect <<-EOF &&
-+ error: worktrees/worktree-1/refs/worktree/ branch-5: badRefName: invalid refname format
-+ error: worktrees/worktree-2/refs/worktree/~branch-6: badRefName: invalid refname format
-+ EOF
-+ sort err >sorted_err &&
-+ test_cmp expect sorted_err
-+ )
++ for worktree in "worktree-1" "worktree-2"
++ do
++ (
++ cd $worktree &&
++ test_must_fail git refs verify 2>err &&
++ cat >expect <<-EOF &&
++ error: worktrees/worktree-1/refs/worktree/ branch-5: badRefName: invalid refname format
++ error: worktrees/worktree-2/refs/worktree/~branch-6: badRefName: invalid refname format
++ EOF
++ sort err >sorted_err &&
++ test_cmp expect sorted_err || return 1
++ )
++ done
+'
+
test_done
5: 6a9e297dfc ! 5: 4e1add6465 ref: port git-fsck(1) regular refs check for files backend
@@ fsck.h: enum fsck_msg_type {
## refs/files-backend.c ##
@@ refs/files-backend.c: typedef int (*files_fsck_refs_fn)(struct ref_store *ref_store,
- const char *target_name,
+ const char *refname,
struct dir_iterator *iter);
+static int files_fsck_refs_content(struct ref_store *ref_store,
@@ refs/files-backend.c: typedef int (*files_fsck_refs_fn)(struct ref_store *ref_st
+ 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 '%s': (%s)",
++ "cannot read ref file '%s': %s",
+ iter->path.buf, strerror(errno));
+ goto cleanup;
+ }
@@ refs/files-backend.c: typedef int (*files_fsck_refs_fn)(struct ref_store *ref_st
+
static int files_fsck_refs_name(struct ref_store *ref_store UNUSED,
struct fsck_options *o,
- const char *target_name,
+ const char *refname,
@@ refs/files-backend.c: static int files_fsck_refs(struct ref_store *ref_store,
{
files_fsck_refs_fn fsck_refs_fn[]= {
@@ refs/files-backend.c: static int files_fsck_refs(struct ref_store *ref_store,
## t/t0602-reffiles-fsck.sh ##
@@ t/t0602-reffiles-fsck.sh: test_expect_success 'ref name check should work for multiple worktrees' '
- )
+ done
'
+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" &&
@@ t/t0602-reffiles-fsck.sh: test_expect_success 'ref name check should work for mu
+ 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
++ for bad_content in "$(git rev-parse main)x" "xfsazqfxcadas" "Xfsazqfxcadas"
++ do
++ printf "%s" $bad_content >$branch_dir_prefix/branch-bad &&
++ test_must_fail git refs verify 2>err &&
++ cat >expect <<-EOF &&
++ error: refs/heads/branch-bad: badRefContent: $bad_content
++ EOF
++ rm $branch_dir_prefix/branch-bad &&
++ test_cmp expect err || return 1
++ done &&
++
++ for bad_content in "$(git rev-parse main)x" "xfsazqfxcadas" "Xfsazqfxcadas"
++ do
++ 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 || return 1
++ done
+'
+
+test_expect_success 'regular ref content should be checked (aggregate)' '
@@ t/t0602-reffiles-fsck.sh: test_expect_success 'ref name check should work for mu
+ git update-ref refs/worktree/branch-4 refs/heads/branch-1
+ ) &&
+
-+ bad_content_1=$(git rev-parse HEAD)x &&
-+ bad_content_2=xfsazqfxcadas &&
-+ bad_content_3=Xfsazqfxcadas &&
-+
-+ printf "%s" $bad_content_1 >$worktree1_refdir_prefix/bad-branch-1 &&
-+ test_must_fail git refs verify 2>err &&
-+ cat >expect <<-EOF &&
-+ error: worktrees/worktree-1/refs/worktree/bad-branch-1: badRefContent: $bad_content_1
-+ EOF
-+ rm $worktree1_refdir_prefix/bad-branch-1 &&
-+ test_cmp expect err &&
-+
-+ printf "%s" $bad_content_2 >$worktree2_refdir_prefix/bad-branch-2 &&
-+ test_must_fail git refs verify 2>err &&
-+ cat >expect <<-EOF &&
-+ error: worktrees/worktree-2/refs/worktree/bad-branch-2: badRefContent: $bad_content_2
-+ EOF
-+ rm $worktree2_refdir_prefix/bad-branch-2 &&
-+ test_cmp expect err &&
-+
-+ printf "%s" $bad_content_3 >$worktree1_refdir_prefix/bad-branch-3 &&
-+ test_must_fail git refs verify 2>err &&
-+ cat >expect <<-EOF &&
-+ error: worktrees/worktree-1/refs/worktree/bad-branch-3: badRefContent: $bad_content_3
-+ EOF
-+ rm $worktree1_refdir_prefix/bad-branch-3 &&
-+ test_cmp expect err
++ for bad_content in "$(git rev-parse HEAD)x" "xfsazqfxcadas" "Xfsazqfxcadas"
++ do
++ printf "%s" $bad_content >$worktree1_refdir_prefix/bad-branch-1 &&
++ test_must_fail git refs verify 2>err &&
++ cat >expect <<-EOF &&
++ error: worktrees/worktree-1/refs/worktree/bad-branch-1: badRefContent: $bad_content
++ EOF
++ rm $worktree1_refdir_prefix/bad-branch-1 &&
++ test_cmp expect err || return 1
++ done &&
++
++ for bad_content in "$(git rev-parse HEAD)x" "xfsazqfxcadas" "Xfsazqfxcadas"
++ do
++ printf "%s" $bad_content >$worktree2_refdir_prefix/bad-branch-2 &&
++ test_must_fail git refs verify 2>err &&
++ cat >expect <<-EOF &&
++ error: worktrees/worktree-2/refs/worktree/bad-branch-2: badRefContent: $bad_content
++ EOF
++ rm $worktree2_refdir_prefix/bad-branch-2 &&
++ test_cmp expect err || return 1
++ done
+'
+
test_done
6: 7eea024182 ! 6: 945322fab7 ref: add more strict checks for regular refs
@@ refs/refs-internal.h: struct ref_store {
## t/t0602-reffiles-fsck.sh ##
@@ t/t0602-reffiles-fsck.sh: test_expect_success 'regular ref content should be checked (individual)' '
- error: refs/heads/a/b/branch-bad: badRefContent: $bad_content
- EOF
- rm $branch_dir_prefix/a/b/branch-bad &&
-+ test_cmp expect err &&
+ EOF
+ rm $branch_dir_prefix/a/b/branch-bad &&
+ test_cmp expect err || return 1
+- done
++ done &&
+
+ printf "%s" "$(git rev-parse main)" >$branch_dir_prefix/branch-no-newline &&
+ git refs verify 2>err &&
@@ t/t0602-reffiles-fsck.sh: test_expect_success 'regular ref content should be che
+ rm $branch_dir_prefix/branch-no-newline &&
+ test_cmp expect err &&
+
-+ printf "%s garbage" "$(git rev-parse main)" >$branch_dir_prefix/branch-garbage &&
-+ git refs verify 2>err &&
-+ cat >expect <<-EOF &&
-+ warning: refs/heads/branch-garbage: trailingRefContent: has trailing garbage: '\'' garbage'\''
-+ EOF
-+ rm $branch_dir_prefix/branch-garbage &&
-+ test_cmp expect err &&
-+
-+ printf "%s\n\n\n" "$(git rev-parse main)" >$tag_dir_prefix/tag-garbage-1 &&
++ for trailing_content in " garbage" " more garbage"
++ do
++ printf "%s" "$(git rev-parse main)$trailing_content" >$branch_dir_prefix/branch-garbage &&
++ git refs verify 2>err &&
++ cat >expect <<-EOF &&
++ warning: refs/heads/branch-garbage: trailingRefContent: has trailing garbage: '\''$trailing_content'\''
++ EOF
++ rm $branch_dir_prefix/branch-garbage &&
++ test_cmp expect err || return 1
++ done &&
++
++ printf "%s\n\n\n" "$(git rev-parse main)" >$branch_dir_prefix/branch-garbage-special &&
+ git refs verify 2>err &&
+ cat >expect <<-EOF &&
-+ warning: refs/tags/tag-garbage-1: trailingRefContent: has trailing garbage: '\''
++ warning: refs/heads/branch-garbage-special: trailingRefContent: has trailing garbage: '\''
+
+
+ '\''
+ EOF
-+ rm $tag_dir_prefix/tag-garbage-1 &&
++ rm $branch_dir_prefix/branch-garbage-special &&
+ test_cmp expect err &&
+
-+ printf "%s\n\n\n garbage" "$(git rev-parse main)" >$tag_dir_prefix/tag-garbage-2 &&
++ printf "%s\n\n\n garbage" "$(git rev-parse main)" >$branch_dir_prefix/branch-garbage-special &&
+ git refs verify 2>err &&
+ cat >expect <<-EOF &&
-+ warning: refs/tags/tag-garbage-2: trailingRefContent: has trailing garbage: '\''
++ warning: refs/heads/branch-garbage-special: trailingRefContent: has trailing garbage: '\''
+
+
+ garbage'\''
+ EOF
-+ rm $tag_dir_prefix/tag-garbage-2 &&
-+ test_cmp expect err &&
-+
-+ printf "%s garbage\na" "$(git rev-parse main)" >$tag_dir_prefix/tag-garbage-3 &&
-+ git refs verify 2>err &&
-+ cat >expect <<-EOF &&
-+ warning: refs/tags/tag-garbage-3: trailingRefContent: has trailing garbage: '\'' garbage
-+ a'\''
-+ EOF
-+ rm $tag_dir_prefix/tag-garbage-3 &&
-+ test_cmp expect err &&
-+
-+ printf "%s garbage" "$(git rev-parse main)" >$tag_dir_prefix/tag-garbage-4 &&
-+ test_must_fail git -c fsck.trailingRefContent=error refs verify 2>err &&
-+ cat >expect <<-EOF &&
-+ error: refs/tags/tag-garbage-4: trailingRefContent: has trailing garbage: '\'' garbage'\''
-+ EOF
-+ rm $tag_dir_prefix/tag-garbage-4 &&
- test_cmp expect err
++ rm $branch_dir_prefix/branch-garbage-special &&
++ test_cmp expect err
'
+ test_expect_success 'regular ref content should be checked (aggregate)' '
@@ t/t0602-reffiles-fsck.sh: test_expect_success 'regular ref content should be checked (aggregate)' '
printf "%s" $bad_content_1 >$tag_dir_prefix/tag-bad-1 &&
printf "%s" $bad_content_2 >$tag_dir_prefix/tag-bad-2 &&
@@ t/t0602-reffiles-fsck.sh: test_expect_success 'regular ref content should be che
sort err >sorted_err &&
test_cmp expect sorted_err
@@ t/t0602-reffiles-fsck.sh: test_expect_success 'ref content checks should work with worktrees' '
- error: worktrees/worktree-1/refs/worktree/bad-branch-3: badRefContent: $bad_content_3
- EOF
- rm $worktree1_refdir_prefix/bad-branch-3 &&
-+ test_cmp expect err &&
+ EOF
+ rm $worktree2_refdir_prefix/bad-branch-2 &&
+ test_cmp expect err || return 1
+- done
++ done &&
+
+ printf "%s" "$(git rev-parse HEAD)" >$worktree1_refdir_prefix/branch-no-newline &&
+ git refs verify 2>err &&
@@ t/t0602-reffiles-fsck.sh: test_expect_success 'ref content checks should work wi
+ warning: worktrees/worktree-1/refs/worktree/branch-no-newline: refMissingNewline: misses LF at the end
+ EOF
+ rm $worktree1_refdir_prefix/branch-no-newline &&
- test_cmp expect err
++ test_cmp expect err
'
+ test_done
7: 1bf36dd644 ! 7: 3006eb9431 ref: add basic symref content check for files backend
@@ fsck.h: enum fsck_msg_type {
## refs/files-backend.c ##
@@ refs/files-backend.c: typedef int (*files_fsck_refs_fn)(struct ref_store *ref_store,
- const char *target_name,
+ const char *refname,
struct dir_iterator *iter);
+static int files_fsck_symref_target(struct fsck_options *o,
@@ t/t0602-reffiles-fsck.sh: test_expect_success 'regular ref content should be che
+ 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" &&
+
-+ printf "ref: refs/heads/branch\n" >$branch_dir_prefix/branch-good &&
-+ git refs verify 2>err &&
-+ rm $branch_dir_prefix/branch-good &&
-+ test_must_be_empty err &&
++ for good_referent in "refs/heads/branch" "HEAD"
++ do
++ printf "ref: %s\n" $good_referent >$branch_dir_prefix/branch-good &&
++ git refs verify 2>err &&
++ rm $branch_dir_prefix/branch-good &&
++ test_must_be_empty err || return 1
++ done &&
+
-+ printf "ref: HEAD\n" >$branch_dir_prefix/branch-head &&
-+ git refs verify 2>err &&
-+ rm $branch_dir_prefix/branch-head &&
-+ test_must_be_empty err &&
++ for bad_referent in "refs/heads/.branch" "refs/heads/~branch" "refs/heads/?branch"
++ do
++ printf "ref: %s\n" $bad_referent >$branch_dir_prefix/branch-bad &&
++ test_must_fail git refs verify 2>err &&
++ cat >expect <<-EOF &&
++ error: refs/heads/branch-bad: badReferentName: points to invalid refname '\''$bad_referent'\''
++ EOF
++ rm $branch_dir_prefix/branch-bad &&
++ test_cmp expect err || return 1
++ done &&
+
-+ printf "ref: refs/heads/branch" >$branch_dir_prefix/branch-no-newline-1 &&
++ printf "ref: refs/heads/branch" >$branch_dir_prefix/branch-no-newline &&
+ git refs verify 2>err &&
+ cat >expect <<-EOF &&
-+ warning: refs/heads/branch-no-newline-1: refMissingNewline: misses LF at the end
++ warning: refs/heads/branch-no-newline: refMissingNewline: misses LF at the end
+ EOF
-+ rm $branch_dir_prefix/branch-no-newline-1 &&
++ rm $branch_dir_prefix/branch-no-newline &&
+ test_cmp expect err &&
+
+ printf "ref: refs/heads/branch " >$branch_dir_prefix/a/b/branch-trailing-1 &&
@@ t/t0602-reffiles-fsck.sh: test_expect_success 'regular ref content should be che
+ warning: refs/heads/a/b/branch-complicated: trailingRefContent: has trailing whitespaces or newlines
+ EOF
+ rm $branch_dir_prefix/a/b/branch-complicated &&
-+ test_cmp expect err &&
-+
-+ printf "ref: refs/heads/.branch\n" >$branch_dir_prefix/branch-bad-1 &&
-+ test_must_fail git refs verify 2>err &&
-+ cat >expect <<-EOF &&
-+ error: refs/heads/branch-bad-1: badReferentName: points to invalid refname '\''refs/heads/.branch'\''
-+ EOF
-+ rm $branch_dir_prefix/branch-bad-1 &&
+ test_cmp expect err
+'
+
8: 1d200f2ade ! 8: c59d003d78 ref: check whether the target of the symref is a ref
@@ t/t0602-reffiles-fsck.sh: test_expect_success 'textual symref content should be
+ test_commit default &&
+ mkdir -p "$branch_dir_prefix/a/b" &&
+
-+ printf "ref: HEAD\n" >$branch_dir_prefix/branch-good &&
-+ git refs verify 2>err &&
-+ rm $branch_dir_prefix/branch-good &&
-+ test_must_be_empty err &&
++ for good_referent in "refs/heads/branch" "HEAD" "refs/tags/tag"
++ do
++ printf "ref: %s\n" $good_referent >$branch_dir_prefix/branch-good &&
++ git refs verify 2>err &&
++ rm $branch_dir_prefix/branch-good &&
++ test_must_be_empty err || return 1
++ done &&
+
-+ printf "ref: refs/foo\n" >$branch_dir_prefix/branch-good &&
-+ git refs verify 2>err &&
-+ rm $branch_dir_prefix/branch-good &&
-+ test_must_be_empty err &&
-+
-+ printf "ref: refs-back/heads/main\n" >$branch_dir_prefix/branch-bad-1 &&
-+ git refs verify 2>err &&
-+ cat >expect <<-EOF &&
-+ warning: refs/heads/branch-bad-1: symrefTargetIsNotARef: points to non-ref target '\''refs-back/heads/main'\''
-+ EOF
-+ rm $branch_dir_prefix/branch-bad-1 &&
-+ test_cmp expect err
++ for nonref_referent in "refs-back/heads/branch" "refs-back/tags/tag" "reflogs/refs/heads/branch"
++ do
++ printf "ref: %s\n" $nonref_referent >$branch_dir_prefix/branch-bad-1 &&
++ git refs verify 2>err &&
++ cat >expect <<-EOF &&
++ warning: refs/heads/branch-bad-1: symrefTargetIsNotARef: points to non-ref target '\''$nonref_referent'\''
++ EOF
++ rm $branch_dir_prefix/branch-bad-1 &&
++ test_cmp expect err || return 1
++ done
+'
+
test_expect_success 'ref content checks should work with worktrees' ' 9: 752f0ad22e ! 9: bb6d7f3323 ref: add symlink ref content check for files backend
@@ refs/files-backend.c: static int files_fsck_refs_content(struct ref_store *ref_s
## t/t0602-reffiles-fsck.sh ##
@@ t/t0602-reffiles-fsck.sh: test_expect_success 'the target of the textual symref should be checked' '
- test_cmp expect err
+ done
'
+test_expect_success SYMLINKS 'symlink symref content should be checked' '
Signature · 1 line
2.47.0
PATCH v7 1/9 ref: initialize "fsck_ref_report" with zero
In "fsck.c::fsck_refs_error_function", we need to tell whether "oid" and "referent" is NULL. So, we need to always initialize these parameters to NULL instead of letting them point to anywhere when creating a new "fsck_ref_report" structure.
The original code explicitly initializes the "path" member in the "struct fsck_ref_report" to NULL (which implicitly 0-initializes other members in the struct). It is more customary to use "{ 0 }" to express that we are 0-initializing everything. In order to align with the codebase, initialize "fsck_ref_report" with zero.
--- refs/files-backend.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
refs/files-backend.c +1 -1
@@ -3520,7 +3520,7 @@ static int files_fsck_refs_name(struct ref_store *ref_store UNUSED, goto cleanup;if (check_refname_format (iter->basename,REFNAME_ALLOW_ONELEVEL )) { -struct fsck_ref_report report = { .path =NULL }; +struct fsck_ref_report report = {0 };strbuf_addf (&sb,"%s/%s" , refs_check_dir, iter->relative_path); report.path = sb.buf;
Signature · 1 line
2.47.0
PATCH v7 2/9 ref: check the full refname instead of basename
In reply to shejialuoIn "files-backend.c::files_fsck_refs_name", we validate the refname format by using "check_refname_format" to check the basename of the iterator with "REFNAME_ALLOW_ONELEVEL" flag.
However, this is a bad implementation. Although we doesn't allow a single "@" in ".git" directory, we do allow "refs/heads/@". So, we will report an error wrongly when there is a "refs/heads/@" ref by using one level refname "@".
Because we just check one level refname, we either cannot check the other parts of the full refname. And we will ignore the following errors:
"refs/heads/ new-feature/test"
"refs/heads/~new-feature/test"
In order to fix the above problem, enhance "files_fsck_refs_name" to use the full name for "check_refname_format". Then, replace the tests which are related to "@" and add tests to exercise the above situations using for loop to avoid repetition.
--- refs/files-backend.c | 7 ++- t/t0602-reffiles-fsck.sh | 92 ++++++++++++++++++++++++---------------- 2 files changed, 60 insertions(+), 39 deletions(-)
refs/files-backend.c +5 -2
@@ -3519,10 +3519,13 @@ static int files_fsck_refs_name(struct ref_store *ref_store UNUSED,if (iter->basename[0 ] !='.' &&ends_with (iter->basename,".lock" )) goto cleanup; -if (check_refname_format (iter->basename,REFNAME_ALLOW_ONELEVEL )) { +/* +* This works right now because we never check the root refs. +*/ +strbuf_addf (&sb,"%s/%s" , refs_check_dir, iter->relative_path); +if (check_refname_format (sb.buf,0 )) {struct fsck_ref_report report = {0 }; -strbuf_addf (&sb,"%s/%s" , refs_check_dir, iter->relative_path); report.path = sb.buf; ret = fsck_report_ref(o, &report, FSCK_MSG_BAD_REF_NAME,
t/t0602-reffiles-fsck.sh +55 -37
125 lines not on this page · raw message
Signature · 1 line
2.47.0
PATCH v7 3/9 ref: initialize ref name outside of check functions
In reply to shejialuoWe passes "refs_check_dir" to the "files_fsck_refs_name" function which allows it to create the checked ref name later. However, when we introduce a new check function, we have to allocate redundant memory and re-calculate the ref name. It's bad for us to allocate redundant memory and duplicate logic. Instead, we should allocate and calculate it only once and pass the ref name to the check functions.
In order not to do repeat calculation, rename "refs_check_dir" to "refname". And in "files_fsck_refs_dir", create a new strbuf "refname", thus whenever we handle a new ref, calculate the name and call the check functions one by one.
--- refs/files-backend.c | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-)
refs/files-backend.c +13 -8
@@ -3501,12 +3501,12 @@ static int files_ref_store_remove_on_disk(struct ref_store *ref_store,*/ typedef int (*files_fsck_refs_fn )(struct ref_store *ref_store,struct fsck_options *o, -const char *refs_check_dir, +const char *refname,struct dir_iterator *iter); static int files_fsck_refs_name(struct ref_store *ref_store UNUSED,struct fsck_options *o, -const char *refs_check_dir, +const char *refname,struct dir_iterator *iter) {struct strbuf sb =STRBUF_INIT ;
@@ -3522,11 +3522,10 @@ static int files_fsck_refs_name(struct ref_store *ref_store UNUSED,/* * This works right now because we never check the root refs. */ -strbuf_addf (&sb,"%s/%s" , refs_check_dir, iter->relative_path); -if (check_refname_format (sb.buf,0 )) { +if (check_refname_format (refname,0 )) {struct fsck_ref_report report = {0 }; - report.path = sb.buf; + report.path = refname; ret =fsck_report_ref (o, &report,FSCK_MSG_BAD_REF_NAME ,"invalid refname format" );
@@ -3542,6 +3541,7 @@ static int files_fsck_refs_dir(struct ref_store *ref_store,const char *refs_check_dir, files_fsck_refs_fn *fsck_refs_fn) { + struct strbuf refname = STRBUF_INIT;struct strbuf sb =STRBUF_INIT ;struct dir_iterator *iter;int iter_status;
@@ -3560,11 +3560,15 @@ static int files_fsck_refs_dir(struct ref_store *ref_store,continue ; }else if (S_ISREG (iter->st.st_mode) ||S_ISLNK (iter->st.st_mode)) { +strbuf_reset (&refname); +strbuf_addf (&refname,"%s/%s" , refs_check_dir, + iter->relative_path); +if (o->verbose) -fprintf_ln (stderr,"Checking %s/%s" , - refs_check_dir, iter->relative_path); +fprintf_ln (stderr,"Checking %s" , refname.buf); +for (size_t i =0 ; fsck_refs_fn[i]; i++) { -if (fsck_refs_fn[i](ref_store, o, refs_check_dir, iter)) +if (fsck_refs_fn[i](ref_store, o, refname.buf, iter)) ret =-1 ; } }else {
@@ -3581,6 +3585,7 @@ static int files_fsck_refs_dir(struct ref_store *ref_store, out:strbuf_release (&sb); +strbuf_release (&refname);return ret; }
Signature · 1 line
2.47.0
PATCH v7 4/9 ref: support multiple worktrees check for refs
In reply to shejialuoWe have already set up the infrastructure to check the consistency for refs, but we do not support multiple worktrees. However, "git-fsck(1)" will check the refs of worktrees. As we decide to get feature parity with "git-fsck(1)", we need to set up support for multiple worktrees.
Because each worktree has its own specific refs, instead of just showing the users "refs/worktree/foo", we need to display the full name such as "worktrees/<id>/refs/worktree/foo". So we should know the id of the worktree to get the full name. Add a new parameter "struct worktree *" for "refs-internal.h::fsck_fn". Then change the related functions to follow this new interface.
The "packed-refs" only exists in the main worktree, so we should only check "packed-refs" in the main worktree. Use "is_main_worktree" method to skip checking "packed-refs" in "packed_fsck" function.
Then, enhance the "files-backend.c::files_fsck_refs_dir" function to add "worktree/<id>/" prefix when we are not in the main worktree.
Last, add a new test to check the refname when there are multiple worktrees to exercise the code.
--- builtin/refs.c | 10 ++++++-- refs.c | 5 ++-- refs.h | 3 ++- refs/debug.c | 5 ++-- refs/files-backend.c | 17 ++++++++++---- refs/packed-backend.c | 8 ++++++- refs/refs-internal.h | 3 ++- refs/reftable-backend.c | 3 ++- t/t0602-reffiles-fsck.sh | 51 ++++++++++++++++++++++++++++++++++++++++ 9 files changed, 90 insertions(+), 15 deletions(-)
builtin/refs.c +8 -2
@@ -5,6 +5,7 @@#include "parse-options.h" #include "refs.h" #include "strbuf.h" +#include "worktree.h" #define REFS_MIGRATE_USAGE \ N_("git refs migrate --ref-format=<format> [--dry-run]")
@@ -66,6 +67,7 @@ static int cmd_refs_migrate(int argc, const char **argv, const char *prefix)static int cmd_refs_verify (int argc,const char **argv,const char *prefix) {struct fsck_options fsck_refs_options =FSCK_REFS_OPTIONS_DEFAULT ; +struct worktree **worktrees;const char *const verify_usage[] = {REFS_VERIFY_USAGE ,NULL ,
@@ -75,7 +77,7 @@ static int cmd_refs_verify(int argc, const char **argv, const char *prefix)OPT_BOOL (0 ,"strict" , &fsck_refs_options.strict,N_ ("enable strict checking" )), OPT_END(), }; -int ret; +int ret =0 ; argc =parse_options (argc, argv, prefix, options, verify_usage,0 );if (argc)
@@ -84,9 +86,13 @@ static int cmd_refs_verify(int argc, const char **argv, const char *prefix)git_config (git_fsck_config, &fsck_refs_options);prepare_repo_settings (the_repository); - ret =refs_fsck (get_main_ref_store (the_repository), &fsck_refs_options); + worktrees =get_worktrees (); +for (size_t i =0 ; worktrees[i]; i++) + ret |=refs_fsck (get_worktree_ref_store (worktrees[i]), + &fsck_refs_options, worktrees[i]);fsck_options_clear (&fsck_refs_options); +free_worktrees (worktrees);return ret; }
refs.c +3 -2
@@ -318,9 +318,10 @@ int check_refname_format(const char *refname, int flags)return check_or_sanitize_refname (refname, flags,NULL ); } -int refs_fsck (struct ref_store *refs,struct fsck_options *o) +int refs_fsck (struct ref_store *refs,struct fsck_options *o, +struct worktree *wt) { -return refs->be->fsck (refs, o); +return refs->be->fsck (refs, o, wt); }void sanitize_refname_component (const char *refname,struct strbuf *out)
refs.h +2 -1
@@ -549,7 +549,8 @@ int check_refname_format(const char *refname, int flags);* reflogs are consistent, and non-zero otherwise. The errors will be * written to stderr. */ -int refs_fsck (struct ref_store *refs,struct fsck_options *o); +int refs_fsck (struct ref_store *refs,struct fsck_options *o, +struct worktree *wt);/* * Apply the rules from check_refname_format, but mutate the result until it
refs/debug.c +3 -2
@@ -420,10 +420,11 @@ static int debug_reflog_expire(struct ref_store *ref_store, const char *refname, }static int debug_fsck (struct ref_store *ref_store, -struct fsck_options *o) +struct fsck_options *o, +struct worktree *wt) {struct debug_ref_store *drefs = (struct debug_ref_store *)ref_store; -int res = drefs->refs->be->fsck (drefs->refs, o); +int res = drefs->refs->be->fsck (drefs->refs, o, wt);trace_printf_key (&trace_refs,"fsck: %d\n" , res);return res; }
refs/files-backend.c +12 -5
@@ -23,6 +23,7 @@#include "../dir.h" #include "../chdir-notify.h" #include "../setup.h" +#include "../worktree.h" #include "../wrapper.h" #include "../write-or-die.h" #include "../revision.h"
@@ -3539,6 +3540,7 @@ static int files_fsck_refs_name(struct ref_store *ref_store UNUSED,static int files_fsck_refs_dir (struct ref_store *ref_store,struct fsck_options *o,const char *refs_check_dir, +struct worktree *wt,files_fsck_refs_fn *fsck_refs_fn) {struct strbuf refname =STRBUF_INIT ;
@@ -3561,6 +3563,9 @@ static int files_fsck_refs_dir(struct ref_store *ref_store, }else if (S_ISREG (iter->st.st_mode) ||S_ISLNK (iter->st.st_mode)) {strbuf_reset (&refname); + +if (!is_main_worktree (wt)) +strbuf_addf (&refname,"worktrees/%s/" , wt->id);strbuf_addf (&refname,"%s/%s" , refs_check_dir, iter->relative_path);
@@ -3590,7 +3595,8 @@ static int files_fsck_refs_dir(struct ref_store *ref_store, }static int files_fsck_refs (struct ref_store *ref_store, -struct fsck_options *o) +struct fsck_options *o, +struct worktree *wt) {files_fsck_refs_fn fsck_refs_fn[]= { files_fsck_refs_name,
@@ -3599,17 +3605,18 @@ static int files_fsck_refs(struct ref_store *ref_store,if (o->verbose)fprintf_ln (stderr,_ ("Checking references consistency" )); -return files_fsck_refs_dir (ref_store, o,"refs" , fsck_refs_fn); +return files_fsck_refs_dir (ref_store, o,"refs" , wt, fsck_refs_fn); }static int files_fsck (struct ref_store *ref_store, -struct fsck_options *o) +struct fsck_options *o, +struct worktree *wt) {struct files_ref_store *refs =files_downcast (ref_store,REF_STORE_READ ,"fsck" ); -return files_fsck_refs (ref_store, o) | - refs->packed_ref_store->be->fsck (refs->packed_ref_store, o); +return files_fsck_refs (ref_store, o, wt) | + refs->packed_ref_store->be->fsck (refs->packed_ref_store, o, wt); }struct ref_storage_be refs_be_files = {
refs/packed-backend.c +7 -1
@@ -13,6 +13,7 @@#include "../lockfile.h" #include "../chdir-notify.h" #include "../statinfo.h" +#include "../worktree.h" #include "../wrapper.h" #include "../write-or-die.h" #include "../trace2.h"
@@ -1754,8 +1755,13 @@ static struct ref_iterator *packed_reflog_iterator_begin(struct ref_store *ref_s } static int packed_fsck(struct ref_store *ref_store UNUSED, - struct fsck_options *o UNUSED) + struct fsck_options *o UNUSED, +struct worktree *wt) { + +if (!is_main_worktree (wt)) +return 0 ; +return 0 ; }
refs/refs-internal.h +2 -1
@@ -653,7 +653,8 @@ typedef int read_symbolic_ref_fn(struct ref_store *ref_store, const char *refnam struct strbuf *referent);typedef int fsck_fn (struct ref_store *ref_store, -struct fsck_options *o); +struct fsck_options *o, +struct worktree *wt);struct ref_storage_be {const char *name;
refs/reftable-backend.c +2 -1
@@ -2443,7 +2443,8 @@ static int reftable_be_reflog_expire(struct ref_store *ref_store, } static int reftable_be_fsck(struct ref_store *ref_store UNUSED, - struct fsck_options *o UNUSED) + struct fsck_options *o UNUSED, + struct worktree *wt UNUSED) {return 0 ; }
t/t0602-reffiles-fsck.sh +51 -0
55 lines not on this page · raw message
Signature · 1 line
2.47.0
PATCH v7 5/9 ref: port git-fsck(1) regular refs check for files backend
In reply to shejialuo"git-fsck(1)" implicitly checks the ref content by passing the callback "fsck_handle_ref" to the "refs.c::refs_for_each_rawref". Then, it will check whether the ref content (eventually "oid") is valid. If not, it will report the following error to the user.
error: refs/heads/main: invalid sha1 pointer 0000...
And it will also report above errors when there are dangling symrefs in the repository wrongly. This does not align with the behavior of the "git symbolic-ref" command which allows users to create dangling symrefs.
As we have already introduced the "git refs verify" command, we'd better check the ref content explicitly in the "git refs verify" command thus later we could remove these checks in "git-fsck(1)" and launch a subprocess to call "git refs verify" in "git-fsck(1)" to make the "git-fsck(1)" more clean.
Following what "git-fsck(1)" does, 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.
--- Documentation/fsck-msgids.txt | 3 + fsck.h | 1 + refs/files-backend.c | 43 ++++++++++++++ t/t0602-reffiles-fsck.sh | 105 ++++++++++++++++++++++++++++++++++ 4 files changed, 152 insertions(+)
Documentation/fsck-msgids.txt +3 -0
@@ -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.
fsck.h +1 -0
@@ -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 ) \
refs/files-backend.c +43 -0
55 lines not on this page · raw message
t/t0602-reffiles-fsck.sh +105 -0
109 lines not on this page · raw message
Signature · 1 line
2.47.0
Re: [PATCH v7 5/9] ref: port git-fsck(1) regular refs check for files backend
Quoted 23 lines · shejialuo
refs/files-backend.c +15
-0@@ -3505,6 +3505,48 @@ typedef int (*files_fsck_refs_fn)(struct ref_store *ref_store,const char *refname, struct dir_iterator *iter); +static int files_fsck_refs_content (struct ref_store *ref_store, +struct fsck_options *o, +const char *target_name, +struct dir_iterator *iter) +{ +struct strbuf ref_content =STRBUF_INIT ; +struct strbuf referent =STRBUF_INIT ; +struct fsck_ref_report report = {0 }; +unsigned int type =0 ; +int failure_errno =0 ; +struct object_id oid; +int ret =0 ; + + report.path = target_name; +
refs/files-backend.c typedef int (*files_fsck_refs_fn)(struct ref_store *ref_store, · line 3532
Quoted 10 lines
refs/files-backend.c +10
-0+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 '%s': %s" , + iter->path.buf,strerror (errno)); + goto cleanup; + }
I didn't catch this in previous rounds, but it's a little dubious whether we should report this as an actual fsck error. I can expect multiple situations:
- The file has weird permissions and thus cannot be read, failing with EPERM, which doesn't match well with BAD_REF_CONTENT.
- The file does not exist anymore because we were racing with a concurrent writer, failing with ENOENT. This is benign and expected to happen in busy repos, so generating an error here feels wrong.
- The file cannot be read at all due to an I/O error. This may be reported with BAD_REF_CONTENT, but conflating this with the case where we have actually bad content may not be the best idea.
So maybe we should ignore ENOENT, report bad permissions and otherwise return an actual error to the caller?
Patrick
Re: [PATCH v7 5/9] ref: port git-fsck(1) regular refs check for files backend
Quoted 46 lines · Patrick Steinhardt
Quoted 23 lines · shejialuo
refs/files-backend.c +15
-0@@ -3505,6 +3505,48 @@ typedef int (*files_fsck_refs_fn)(struct ref_store *ref_store,const char *refname, struct dir_iterator *iter); +static int files_fsck_refs_content (struct ref_store *ref_store, +struct fsck_options *o, +const char *target_name, +struct dir_iterator *iter) +{ +struct strbuf ref_content =STRBUF_INIT ; +struct strbuf referent =STRBUF_INIT ; +struct fsck_ref_report report = {0 }; +unsigned int type =0 ; +int failure_errno =0 ; +struct object_id oid; +int ret =0 ; + + report.path = target_name; +refs/files-backend.c typedef int (*files_fsck_refs_fn)(struct ref_store *ref_store, · line 3532
Quoted 10 lines
refs/files-backend.c +10
-0+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 '%s': %s" , + iter->path.buf,strerror (errno)); + goto cleanup; + }I didn't catch this in previous rounds, but it's a little dubious whether we should report this as an actual fsck error. I can expect multiple situations:
- The file has weird permissions and thus cannot be read, failing with EPERM, which doesn't match well with
BAD_REF_CONTENT.- The file does not exist anymore because we were racing with a concurrent writer, failing with ENOENT. This is benign and expected to happen in busy repos, so generating an error here feels wrong.
Quoted 7 lines
- The file cannot be read at all due to an I/O error. This may be reported with
BAD_REF_CONTENT, but conflating this with the case where we have actually bad content may not be the best idea.So maybe we should ignore ENOENT, report bad permissions and otherwise return an actual error to the caller?
So, I think we should just use "error_errno" method to report the actual error to the caller. And we also need to add some comments.
Thanks for this wonderful suggestion.
Quoted 1 line
Patrick
PATCH v7 6/9 ref: add more strict checks for regular refs
In reply to shejialuoWe have already used "parse_loose_ref_contents" function to check whether the ref content is valid in files backend. However, by using "parse_loose_ref_contents", we allow the ref's content to end with garbage or without a newline.
Even though we never create such loose refs ourselves, we have accepted such loose refs. So, it is entirely possible that some third-party tools may rely on such loose refs being valid. We should not report an error fsck message at current. We should notify the users about such "curiously formatted" loose refs so that adequate care is taken before we decide to tighten the rules in the future.
And it's not suitable either to report a warn fsck message to the user. We don't yet want the "--strict" flag that controls this bit to end up generating errors for such weirdly-formatted reference contents, as we first want to assess whether this retroactive tightening will cause issues for any tools out there. It may cause compatibility issues which may break the repository. So, we add the following two fsck infos to represent the situation where the ref content ends without newline or has trailing garbages:
1. refMissingNewline(INFO): A loose ref that does not end with newline(LF).
2. trailingRefContent(INFO): A loose ref has trailing content.
It might appear that we can't provide the user with any warnings by using FSCK_INFO. However, in "fsck.c::fsck_vreport", we will convert FSCK_INFO to FSCK_WARN and we can still warn the user about these situations when using "git refs verify" without introducing compatibility issues.
--- Documentation/fsck-msgids.txt | 14 +++++++++ fsck.h | 2 ++ refs.c | 2 +- refs/files-backend.c | 26 ++++++++++++++-- refs/refs-internal.h | 2 +- t/t0602-reffiles-fsck.sh | 57 +++++++++++++++++++++++++++++++++-- 6 files changed, 96 insertions(+), 7 deletions(-)
Documentation/fsck-msgids.txt +14 -0
@@ -173,6 +173,20 @@ `nullSha1`:: (WARN) Tree contains entries pointing to a null sha1. +`refMissingNewline`:: + (INFO) A loose ref that does not end with newline(LF). As + valid implementations of Git never created such a loose ref + file, it may become an error in the future. Report to the + [email protected] mailing list if you see this error, as + we need to know what tools created such a file. + +`trailingRefContent`:: + (INFO) A loose ref has trailing content. As valid implementations + of Git never created such a loose ref file, it may become an + error in the future. Report to the [email protected] mailing + list if you see this error, as we need to know what tools + created such a file. + `treeNotSorted`:: (ERROR) A tree is not properly sorted.
fsck.h +2 -0
@@ -85,6 +85,8 @@ enum fsck_msg_type { FUNC(MAILMAP_SYMLINK, INFO) \ FUNC(BAD_TAG_NAME, INFO) \ FUNC(MISSING_TAGGER_ENTRY, INFO) \ + FUNC(REF_MISSING_NEWLINE, INFO) \ +FUNC (TRAILING_REF_CONTENT ,INFO ) \ /* ignored (elevated when requested) */ \FUNC (EXTRA_HEADER_ENTRY ,IGNORE )
refs.c +1 -1
@@ -1789,7 +1789,7 @@ static int refs_read_special_head(struct ref_store *ref_store, } result =parse_loose_ref_contents (ref_store->repo->hash_algo, content.buf, - oid, referent, type, failure_errno); + oid, referent, type,NULL , failure_errno); done:strbuf_release (&full_path);
refs/files-backend.c +23 -3
@@ -569,7 +569,7 @@ static int read_ref_internal(struct ref_store *ref_store, const char *refname, buf = sb_contents.buf; ret =parse_loose_ref_contents (ref_store->repo->hash_algo, buf, - oid, referent, type, &myerr); + oid, referent, type,NULL , &myerr); out:if (ret && !myerr)
@@ -606,7 +606,7 @@ static int files_read_symbolic_ref(struct ref_store *ref_store, const char *refnint parse_loose_ref_contents (const struct git_hash_algo *algop,const char *buf,struct object_id *oid,struct strbuf *referent,unsigned int *type, -int *failure_errno) +const char **trailing,int *failure_errno) {const char *p;if (skip_prefix (buf,"ref:" , &buf)) {
@@ -628,6 +628,10 @@ int parse_loose_ref_contents(const struct git_hash_algo *algop, *failure_errno =EINVAL ;return -1 ; } + +if (trailing) + *trailing = p; +return 0 ; }
@@ -3513,6 +3517,7 @@ static int files_fsck_refs_content(struct ref_store *ref_store,struct strbuf ref_content =STRBUF_INIT ;struct strbuf referent =STRBUF_INIT ;struct fsck_ref_report report = {0 }; +const char *trailing =NULL ;unsigned int type =0 ;int failure_errno =0 ;struct object_id oid;
@@ -3533,7 +3538,7 @@ static int files_fsck_refs_content(struct ref_store *ref_store,if (parse_loose_ref_contents (ref_store->repo->hash_algo, ref_content.buf, &oid, &referent, - &type, &failure_errno)) { + &type, &trailing, &failure_errno)) {strbuf_rtrim (&ref_content); ret = fsck_report_ref(o, &report, FSCK_MSG_BAD_REF_CONTENT,
@@ -3541,6 +3546,21 @@ static int files_fsck_refs_content(struct ref_store *ref_store, goto cleanup; } +if (!(type &REF_ISSYMREF )) { +if (!*trailing) { + ret =fsck_report_ref (o, &report, +FSCK_MSG_REF_MISSING_NEWLINE , +"misses LF at the end" ); + goto cleanup; + } +if (*trailing !='\n' || *(trailing +1 )) { + ret =fsck_report_ref (o, &report, +FSCK_MSG_TRAILING_REF_CONTENT , +"has trailing garbage: '%s'" , trailing); + goto cleanup; + } + } + cleanup:strbuf_release (&ref_content);strbuf_release (&referent);
refs/refs-internal.h +1 -1
@@ -716,7 +716,7 @@ struct ref_store {int parse_loose_ref_contents (const struct git_hash_algo *algop,const char *buf,struct object_id *oid,struct strbuf *referent,unsigned int *type, -int *failure_errno); +const char **trailing,int *failure_errno);/* * Fill in the generic part of refs and add it to our collection of
t/t0602-reffiles-fsck.sh +55 -2
81 lines not on this page · raw message
Signature · 1 line
2.47.0
PATCH v7 7/9 ref: add basic symref content check for files backend
In reply to shejialuoWe have code that checks regular ref contents, but we do not yet check the contents of symbolic refs. By using "parse_loose_ref_content" for symbolic refs, we will get the information of the "referent".
We do not need to check the "referent" by opening the file. This is because if "referent" exists in the file system, we will eventually check its correctness by inspecting every file in the "refs" directory. If the "referent" does not exist in the filesystem, this is OK as it is seen as the dangling symref.
So we just need to check the "referent" string content. A regular ref could be accepted as a textual symref if it begins with "ref:", followed by zero or more whitespaces, followed by the full refname, followed only by whitespace characters. However, we always write a single SP after "ref:" and a single LF after the refname. It may seem that we should report a fsck error message when the "referent" does not apply above rules and we should not be so aggressive because third-party reimplementations of Git may have taken advantage of the looser syntax. Put it more specific, we accept the following contents:
1. "ref: refs/heads/master "
2. "ref: refs/heads/master \n \n"
3. "ref: refs/heads/master\n\n"
When introducing the regular ref content checks, we created two fsck infos "refMissingNewline" and "trailingRefContent" which exactly represents above situations. So we will reuse these two fsck messages to write checks to info the user about these situations.
But we do not allow any other trailing garbage. The followings are bad symref contents which will be reported as fsck error by "git-fsck(1)".
1. "ref: refs/heads/master garbage\n"
2. "ref: refs/heads/master \n\n\n garbage "
And we introduce a new "badReferentName(ERROR)" fsck message to report above errors by using "is_root_ref" and "check_refname_format" to check the "referent". Since both "is_root_ref" and "check_refname_format" don't work with whitespaces, we use the trimmed version of "referent" with these functions.
In order to add checks, we will do the following things:
1. Record the untrimmed length "orig_len" and untrimmed last byte "orig_last_byte".
2. Use "strbuf_rtrim" to trim the whitespaces or newlines to make sure "is_root_ref" and "check_refname_format" won't be failed by them.
3. Use "orig_len" and "orig_last_byte" to check whether the "referent" misses '\n' at the end or it has trailing whitespaces or newlines.
--- Documentation/fsck-msgids.txt | 3 + fsck.h | 1 + refs/files-backend.c | 40 ++++++++++++ t/t0602-reffiles-fsck.sh | 111 ++++++++++++++++++++++++++++++++++ 4 files changed, 155 insertions(+)
Documentation/fsck-msgids.txt +3 -0
@@ -28,6 +28,9 @@ `badRefName`:: (ERROR) A ref has an invalid format. +`badReferentName`:: + (ERROR) The referent name of a symref is invalid. + `badTagName`:: (INFO) A tag has an invalid format.
fsck.h +1 -0
@@ -34,6 +34,7 @@ enum fsck_msg_type { FUNC(BAD_REF_CONTENT, ERROR) \ FUNC(BAD_REF_FILETYPE, ERROR) \ FUNC(BAD_REF_NAME, ERROR) \ + FUNC(BAD_REFERENT_NAME, ERROR) \ FUNC(BAD_TIMEZONE, ERROR) \ FUNC(BAD_TREE, ERROR) \FUNC (BAD_TREE_SHA1 ,ERROR ) \
refs/files-backend.c +40 -0
@@ -3509,6 +3509,43 @@ typedef int (*files_fsck_refs_fn)(struct ref_store *ref_store,const char *refname, struct dir_iterator *iter); +static int files_fsck_symref_target (struct fsck_options *o, +struct fsck_ref_report *report, +struct strbuf *referent) +{ +char orig_last_byte; +size_t orig_len; +int ret =0 ; + + orig_len = referent->len; + orig_last_byte = referent->buf[orig_len -1 ]; +strbuf_rtrim (referent); + +if (!is_root_ref (referent->buf) && +check_refname_format (referent->buf,0 )) { + ret =fsck_report_ref (o, report, +FSCK_MSG_BAD_REFERENT_NAME , +"points to invalid refname '%s'" , referent->buf); + goto out; + } + +if (referent->len == orig_len || + (referent->len < orig_len && orig_last_byte !='\n' )) { + ret =fsck_report_ref (o, report, +FSCK_MSG_REF_MISSING_NEWLINE , +"misses LF at the end" ); + } + +if (referent->len != orig_len && referent->len != orig_len -1 ) { + ret =fsck_report_ref (o, report, +FSCK_MSG_TRAILING_REF_CONTENT , +"has trailing whitespaces or newlines" ); + } + +out: +return ret; +} + static int files_fsck_refs_content(struct ref_store *ref_store, struct fsck_options *o, const char *target_name,
@@ -3559,6 +3596,9 @@ static int files_fsck_refs_content(struct ref_store *ref_store, "has trailing garbage: '%s'", trailing); goto cleanup; } + }else { + ret =files_fsck_symref_target (o, &report, &referent); + goto cleanup; } cleanup:
t/t0602-reffiles-fsck.sh +111 -0
123 lines not on this page · raw message
Signature · 1 line
2.47.0
PATCH v7 8/9 ref: check whether the target of the symref is a ref
In reply to shejialuoIdeally, we want to the users use "git symbolic-ref" to create symrefs instead of writing raw contents into the filesystem. However, "git symbolic-ref" is strict with the refname but not strict with the referent. For example, we can make the "referent" located at the "$(gitdir)/logs/aaa" and manually write the content into this where we can still successfully parse this symref by using "git rev-parse".
$ git init repo && cd repo && git commit --allow-empty -mx $ git symbolic-ref refs/heads/test logs/aaa $ echo $(git rev-parse HEAD) > .git/logs/aaa $ git rev-parse test
We may need to add some restrictions for "referent" parameter when using "git symbolic-ref" to create symrefs because ideally all the nonpseudo-refs should be located under the "refs" directory and we may tighten this in the future.
In order to tell the user we may tighten the above situation, create a new fsck message "symrefTargetIsNotARef" to notify the user that this may become an error in the future.
--- Documentation/fsck-msgids.txt | 9 +++++++++ fsck.h | 1 + refs/files-backend.c | 14 ++++++++++++-- t/t0602-reffiles-fsck.sh | 29 +++++++++++++++++++++++++++++ 4 files changed, 51 insertions(+), 2 deletions(-)
Documentation/fsck-msgids.txt +9 -0
@@ -183,6 +183,15 @@ [email protected] mailing list if you see this error, as we need to know what tools created such a file. +`symrefTargetIsNotARef`:: + (INFO) The target of a symbolic reference points neither to + a root reference nor to a reference starting with "refs/". + Although we allow create a symref pointing to the referent which + is outside the "ref" by using `git symbolic-ref`, we may tighten + the rule in the future. Report to the [email protected] + mailing list if you see this error, as we need to know what tools + created such a file. + `trailingRefContent`:: (INFO) A loose ref has trailing content. As valid implementations of Git never created such a loose ref file, it may become an
fsck.h +1 -0
@@ -87,6 +87,7 @@ enum fsck_msg_type { FUNC(BAD_TAG_NAME, INFO) \ FUNC(MISSING_TAGGER_ENTRY, INFO) \ FUNC(REF_MISSING_NEWLINE, INFO) \ + FUNC(SYMREF_TARGET_IS_NOT_A_REF, INFO) \FUNC (TRAILING_REF_CONTENT ,INFO ) \ /* ignored (elevated when requested) */ \FUNC (EXTRA_HEADER_ENTRY ,IGNORE )
refs/files-backend.c +12 -2
@@ -3513,6 +3513,7 @@ static int files_fsck_symref_target(struct fsck_options *o,struct fsck_ref_report *report, struct strbuf *referent) { + int is_referent_root;char orig_last_byte;size_t orig_len;int ret =0 ;
@@ -3521,8 +3522,17 @@ static int files_fsck_symref_target(struct fsck_options *o, orig_last_byte = referent->buf[orig_len -1 ];strbuf_rtrim (referent); -if (!is_root_ref (referent->buf) && -check_refname_format (referent->buf,0 )) { + is_referent_root =is_root_ref (referent->buf); +if (!is_referent_root && + !starts_with (referent->buf,"refs/" ) && + !starts_with (referent->buf,"worktrees/" )) { + ret =fsck_report_ref (o, report, +FSCK_MSG_SYMREF_TARGET_IS_NOT_A_REF , +"points to non-ref target '%s'" , referent->buf); + + } + +if (!is_referent_root &&check_refname_format (referent->buf,0 )) { ret =fsck_report_ref (o, report,FSCK_MSG_BAD_REFERENT_NAME ,"points to invalid refname '%s'" , referent->buf);
t/t0602-reffiles-fsck.sh +29 -0
@@ -366,6 +366,35 @@ test_expect_success 'textual symref content should be checked (aggregate)' ' test_cmp expect sorted_err ' +test_expect_success 'the target of the textual symref should be checked' ' + 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" && + + for good_referent in "refs/heads/branch" "HEAD" "refs/tags/tag" + do + printf "ref: %s\n" $good_referent >$branch_dir_prefix/branch-good && + git refs verify 2>err && + rm $branch_dir_prefix/branch-good && + test_must_be_empty err || return 1 + done && + + for nonref_referent in "refs-back/heads/branch" "refs-back/tags/tag" "reflogs/refs/heads/branch" + do + printf "ref: %s\n" $nonref_referent >$branch_dir_prefix/branch-bad-1 && + git refs verify 2>err && + cat >expect <<-EOF && + warning: refs/heads/branch-bad-1: symrefTargetIsNotARef: points to non-ref target '\''$nonref_referent'\'' + EOF + rm $branch_dir_prefix/branch-bad-1 && + test_cmp expect err || return 1 + done +' + test_expect_success 'ref content checks should work with worktrees' ' test_when_finished "rm -rf repo" && git init repo &&
Signature · 1 line
2.47.0
PATCH v7 9/9 ref: add symlink ref content check for files backend
In reply to shejialuoBesides the textual symref, we also allow symbolic links as the symref. So, we should also provide the consistency check as what we have done for textual symref. And also we consider deprecating writing the symbolic links. We first need to access whether symbolic links still be used. So, add a new fsck message "symlinkRef(INFO)" to tell the user be aware of this information.
We have already introduced "files_fsck_symref_target". We should reuse this function to handle the symrefs which use legacy symbolic links. We should not check the trailing garbage for symbolic refs. Add a new parameter "symbolic_link" to disable some checks which should only be executed for textual symrefs.
And we need to also generate the "referent" parameter for reusing "files_fsck_symref_target" by the following steps:
1. Use "strbuf_add_real_path" to resolve the symlink and get the absolute path "ref_content" which the symlink ref points to.
2. Generate the absolute path "abs_gitdir" of "gitdir" and combine "ref_content" and "abs_gitdir" to extract the relative path "relative_referent_path".
3. If "ref_content" is outside of "gitdir", we just set "referent" with "ref_content". Instead, we set "referent" with "relative_referent_path".
--- Documentation/fsck-msgids.txt | 6 +++++ fsck.h | 1 + refs/files-backend.c | 38 +++++++++++++++++++++++++---- t/t0602-reffiles-fsck.sh | 45 +++++++++++++++++++++++++++++++++++ 4 files changed, 86 insertions(+), 4 deletions(-)
Documentation/fsck-msgids.txt +6 -0
@@ -183,6 +183,12 @@ [email protected] mailing list if you see this error, as we need to know what tools created such a file. +`symlinkRef`:: + (INFO) A symbolic link is used as a symref. Report to the + [email protected] mailing list if you see this error, as we + are assessing the feasibility of dropping the support to drop + creating symbolic links as symrefs. + `symrefTargetIsNotARef`:: (INFO) The target of a symbolic reference points neither to a root reference nor to a reference starting with "refs/".
fsck.h +1 -0
@@ -86,6 +86,7 @@ enum fsck_msg_type { FUNC(MAILMAP_SYMLINK, INFO) \ FUNC(BAD_TAG_NAME, INFO) \ FUNC(MISSING_TAGGER_ENTRY, INFO) \ + FUNC(SYMLINK_REF, INFO) \ FUNC(REF_MISSING_NEWLINE, INFO) \ FUNC(SYMREF_TARGET_IS_NOT_A_REF, INFO) \FUNC (TRAILING_REF_CONTENT ,INFO ) \
refs/files-backend.c +34 -4
@@ -1,6 +1,7 @@#define USE_THE_REPOSITORY_VARIABLE #include "../git-compat-util.h" +#include "../abspath.h" #include "../config.h" #include "../copy.h" #include "../environment.h"
@@ -3511,7 +3512,8 @@ typedef int (*files_fsck_refs_fn)(struct ref_store *ref_store,static int files_fsck_symref_target (struct fsck_options *o,struct fsck_ref_report *report, -struct strbuf *referent) +struct strbuf *referent, +unsigned int symbolic_link) {int is_referent_root;char orig_last_byte;
@@ -3520,7 +3522,8 @@ static int files_fsck_symref_target(struct fsck_options *o, orig_len = referent->len; orig_last_byte = referent->buf[orig_len -1 ]; -strbuf_rtrim (referent); +if (!symbolic_link) +strbuf_rtrim (referent); is_referent_root =is_root_ref (referent->buf); if (!is_referent_root &&
@@ -3539,6 +3542,9 @@ static int files_fsck_symref_target(struct fsck_options *o, goto out; } +if (symbolic_link) + goto out; +if (referent->len == orig_len || (referent->len < orig_len && orig_last_byte !='\n' )) { ret = fsck_report_ref(o, report,
@@ -3562,6 +3568,7 @@ static int files_fsck_refs_content(struct ref_store *ref_store, struct dir_iterator *iter) {struct strbuf ref_content =STRBUF_INIT ; +struct strbuf abs_gitdir =STRBUF_INIT ;struct strbuf referent =STRBUF_INIT ;struct fsck_ref_report report = {0 };const char *trailing =NULL ;
@@ -3572,8 +3579,30 @@ static int files_fsck_refs_content(struct ref_store *ref_store, report.path = target_name; -if (S_ISLNK (iter->st.st_mode)) +if (S_ISLNK (iter->st.st_mode)) { +const char * relative_referent_path =NULL ; + + ret =fsck_report_ref (o, &report, +FSCK_MSG_SYMLINK_REF , +"use deprecated symbolic link for symref" ); + +strbuf_add_absolute_path (&abs_gitdir, ref_store->gitdir); +strbuf_normalize_path (&abs_gitdir); +if (!is_dir_sep (abs_gitdir.buf[abs_gitdir.len -1 ])) +strbuf_addch (&abs_gitdir,'/' ); + +strbuf_add_real_path (&ref_content, iter->path.buf); +skip_prefix (ref_content.buf, abs_gitdir.buf, + &relative_referent_path); + +if (relative_referent_path) +strbuf_addstr (&referent, relative_referent_path); +else +strbuf_addbuf (&referent, &ref_content); + + ret |=files_fsck_symref_target (o, &report, &referent,1 ); goto cleanup; + }if (strbuf_read_file (&ref_content, iter->path.buf,0 ) <0 ) { ret = fsck_report_ref(o, &report,
@@ -3607,13 +3636,14 @@ static int files_fsck_refs_content(struct ref_store *ref_store, goto cleanup; } }else { - ret =files_fsck_symref_target (o, &report, &referent); + ret =files_fsck_symref_target (o, &report, &referent,0 ); goto cleanup; } cleanup:strbuf_release (&ref_content);strbuf_release (&referent); +strbuf_release (&abs_gitdir);return ret; }
t/t0602-reffiles-fsck.sh +45 -0
51 lines not on this page · raw message
Signature · 1 line
2.47.0
Re: [PATCH v7 9/9] ref: add symlink ref content check for files backend
refs/files-backend.c static int files_fsck_refs_content(struct ref_store *ref_store, · line 3583
Quoted 7 lines · shejialuo
refs/files-backend.c +2
-1@@ -3572,8 +3579,30 @@ static int files_fsck_refs_content(struct ref_store *ref_store, report.path = target_name; -if (S_ISLNK (iter->st.st_mode)) +if (S_ISLNK (iter->st.st_mode)) { +const char * relative_referent_path =NULL ;
Nit: the asterisk should stick with the variable name.
Quoted 11 lines
refs/files-backend.c +11
-0+ ret =fsck_report_ref (o, &report, +FSCK_MSG_SYMLINK_REF , +"use deprecated symbolic link for symref" ); + +strbuf_add_absolute_path (&abs_gitdir, ref_store->gitdir); +strbuf_normalize_path (&abs_gitdir); +if (!is_dir_sep (abs_gitdir.buf[abs_gitdir.len -1 ])) +strbuf_addch (&abs_gitdir,'/' ); + +strbuf_add_real_path (&ref_content, iter->path.buf); + skip_prefix(ref_content.buf, abs_gitdir.buf,
refs/files-backend.c static int files_fsck_refs_content(struct ref_store *ref_store, · line 3605
Quoted 10 lines
refs/files-backend.c +9
-0+ &relative_referent_path); + +if (relative_referent_path) +strbuf_addstr (&referent, relative_referent_path); +else +strbuf_addbuf (&referent, &ref_content); + + ret |=files_fsck_symref_target (o, &report, &referent,1 ); goto cleanup; + }
I wonder whether this logic works as expected with per-worktree symbolic refs which are a symlink. On the other hand I wonder whether those work as expected in the first place. Probably not. *shrug*
In any case, it would be nice to have a test for this.
Patrick
Re: [PATCH v7 9/9] ref: add symlink ref content check for files backend
Quoted 11 lines · Patrick Steinhardt
refs/files-backend.c static int files_fsck_refs_content(struct ref_store *ref_store, · line 3583
Quoted 7 lines · shejialuo
refs/files-backend.c +2
-1@@ -3572,8 +3579,30 @@ static int files_fsck_refs_content(struct ref_store *ref_store, report.path = target_name; -if (S_ISLNK (iter->st.st_mode)) +if (S_ISLNK (iter->st.st_mode)) { +const char * relative_referent_path =NULL ;Nit: the asterisk should stick with the variable name.
I will improve this in the next version.
Quoted 26 lines
Quoted 11 lines
refs/files-backend.c +11
-0+ ret =fsck_report_ref (o, &report, +FSCK_MSG_SYMLINK_REF , +"use deprecated symbolic link for symref" ); + +strbuf_add_absolute_path (&abs_gitdir, ref_store->gitdir); +strbuf_normalize_path (&abs_gitdir); +if (!is_dir_sep (abs_gitdir.buf[abs_gitdir.len -1 ])) +strbuf_addch (&abs_gitdir,'/' ); + +strbuf_add_real_path (&ref_content, iter->path.buf); + skip_prefix(ref_content.buf, abs_gitdir.buf,refs/files-backend.c static int files_fsck_refs_content(struct ref_store *ref_store, · line 3605
Quoted 10 lines
refs/files-backend.c +9
-0+ &relative_referent_path); + +if (relative_referent_path) +strbuf_addstr (&referent, relative_referent_path); +else +strbuf_addbuf (&referent, &ref_content); + + ret |=files_fsck_symref_target (o, &report, &referent,1 ); goto cleanup; + }I wonder whether this logic works as expected with per-worktree symbolic refs which are a symlink. On the other hand I wonder whether those work as expected in the first place. Probably not. *shrug*
Quoted 2 lines
In any case, it would be nice to have a test for this.
Correct, I have ignored because I add worktree support in the later version. Let me add a new test to verify this.
Quoted 1 line
Patrick
Thanks,
Jialuo
Re: [PATCH v7 0/9] add ref content check for files backend
In reply to shejialuoQuoted 11 lines · shejialuo
Hi All:
This new version solves the follow problems:
1. Enhance the commit message suggested by Patrick.
2. Rename "target_name" to "refname".
3. Enhance the shell scripts to usefor into avoid repetition. And this is the main change of this new version.Thanks,
Jialuo
I've got two more comments, but otherwise this series looks close now. Thanks!
Patrick
v8 cover add ref content check for files backend
In reply to shejialuoHi all:
This new version solves the following problem:
1. when reading the content of the ref file, we do not use "fsck_report_ref" function. It's not suitable.
2. Add a new test for symlink worktree test in the last patch. After writing the tets, find a bug. Fix the bug described below.
Because we have introduced the check for worktrees, we should not use "ref_store->gitdir", instead we need to use "ref_store->repo->gitdir" to get the main worktree "gitdir". After fixing this, the test is passed.
Thank Patrick to remind me about this. I forgot to add test thus making mistakes.
Thanks,
Jialuo
shejialuo (9):
ref: initialize "fsck_ref_report" with zero
ref: check the full refname instead of basename
ref: initialize ref name outside of check functions
ref: support multiple worktrees check for refs
ref: port git-fsck(1) regular refs check for files backend
ref: add more strict checks for regular refs
ref: add basic symref content check for files backend
ref: check whether the target of the symref is a ref
ref: add symlink ref content check for files backend
Documentation/fsck-msgids.txt | 35 +++ builtin/refs.c | 10 +- fsck.h | 6 + refs.c | 7 +- refs.h | 3 +- refs/debug.c | 5 +- refs/files-backend.c | 195 +++++++++++- refs/packed-backend.c | 8 +- refs/refs-internal.h | 5 +- refs/reftable-backend.c | 3 +- t/t0602-reffiles-fsck.sh | 576 ++++++++++++++++++++++++++++++++-- 11 files changed, 791 insertions(+), 62 deletions(-)
Range-diff against v7:
1: bfb2a21af4 = 1: bfb2a21af4 ref: initialize "fsck_ref_report" with zero
2: 9efc83f7ea = 2: 9efc83f7ea ref: check the full refname instead of basename
3: 5ea7d18203 = 3: 5ea7d18203 ref: initialize ref name outside of check functions
4: cb4669b64d = 4: cb4669b64d ref: support multiple worktrees check for refs
5: 4e1add6465 ! 5: c6c128c922 ref: port git-fsck(1) regular refs check for files backend
@@ refs/files-backend.c: typedef int (*files_fsck_refs_fn)(struct ref_store *ref_st
+ 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 '%s': %s",
-+ iter->path.buf, strerror(errno));
++ if (strbuf_read_file(&ref_content, iter->path.buf, 0) < 0 ) {
++ /*
++ * Ref file could be removed by another concurrent process. We should
++ * ignore this error and continue to the next ref.
++ */
++ if (errno == ENOENT)
++ goto cleanup;
++
++ ret = error_errno(_("cannot read ref file '%s': %s"),
++ iter->path.buf, strerror(errno));
+ goto cleanup;
+ }
+
6: 945322fab7 = 6: 911fa42717 ref: add more strict checks for regular refs
7: 3006eb9431 = 7: 7aa6a99206 ref: add basic symref content check for files backend
8: c59d003d78 = 8: dbb0787ad1 ref: check whether the target of the symref is a ref
9: bb6d7f3323 ! 9: a6d85b4864 ref: add symlink ref content check for files backend
@@ refs/files-backend.c: static int files_fsck_refs_content(struct ref_store *ref_s
- if (S_ISLNK(iter->st.st_mode))
+ if (S_ISLNK(iter->st.st_mode)) {
-+ const char* relative_referent_path = NULL;
++ const char *relative_referent_path = NULL;
+
+ ret = fsck_report_ref(o, &report,
+ FSCK_MSG_SYMLINK_REF,
+ "use deprecated symbolic link for symref");
+
-+ strbuf_add_absolute_path(&abs_gitdir, ref_store->gitdir);
++ strbuf_add_absolute_path(&abs_gitdir, ref_store->repo->gitdir);
+ strbuf_normalize_path(&abs_gitdir);
+ if (!is_dir_sep(abs_gitdir.buf[abs_gitdir.len - 1]))
+ strbuf_addch(&abs_gitdir, '/');
@@ refs/files-backend.c: static int files_fsck_refs_content(struct ref_store *ref_s
goto cleanup;
+ }
- if (strbuf_read_file(&ref_content, iter->path.buf, 0) < 0) {
- ret = fsck_report_ref(o, &report,
+ if (strbuf_read_file(&ref_content, iter->path.buf, 0) < 0 ) {
+ /*
@@ refs/files-backend.c: static int files_fsck_refs_content(struct ref_store *ref_store,
goto cleanup;
}
@@ t/t0602-reffiles-fsck.sh: test_expect_success 'the target of the textual symref
+ rm $tag_dir_prefix/tag-symbolic-1 &&
+ test_cmp expect err
+'
++
++test_expect_success SYMLINKS 'symlink symref content should be checked (worktree)' '
++ test_when_finished "rm -rf repo" &&
++ git init repo &&
++ cd repo &&
++ test_commit default &&
++ git branch branch-1 &&
++ git branch branch-2 &&
++ git branch branch-3 &&
++ git worktree add ./worktree-1 branch-2 &&
++ git worktree add ./worktree-2 branch-3 &&
++ main_worktree_refdir_prefix=.git/refs/heads &&
++ worktree1_refdir_prefix=.git/worktrees/worktree-1/refs/worktree &&
++ worktree2_refdir_prefix=.git/worktrees/worktree-2/refs/worktree &&
++
++ (
++ cd worktree-1 &&
++ git update-ref refs/worktree/branch-4 refs/heads/branch-1
++ ) &&
++ (
++ cd worktree-2 &&
++ git update-ref refs/worktree/branch-4 refs/heads/branch-1
++ ) &&
++
++ ln -sf ../../../../refs/heads/good-branch $worktree1_refdir_prefix/branch-symbolic-good &&
++ git refs verify 2>err &&
++ cat >expect <<-EOF &&
++ warning: worktrees/worktree-1/refs/worktree/branch-symbolic-good: symlinkRef: use deprecated symbolic link for symref
++ EOF
++ rm $worktree1_refdir_prefix/branch-symbolic-good &&
++ test_cmp expect err &&
++
++ ln -sf ../../../../worktrees/worktree-1/good-branch $worktree2_refdir_prefix/branch-symbolic-good &&
++ git refs verify 2>err &&
++ cat >expect <<-EOF &&
++ warning: worktrees/worktree-2/refs/worktree/branch-symbolic-good: symlinkRef: use deprecated symbolic link for symref
++ EOF
++ rm $worktree2_refdir_prefix/branch-symbolic-good &&
++ test_cmp expect err &&
++
++ ln -sf ../../worktrees/worktree-2/good-branch $main_worktree_refdir_prefix/branch-symbolic-good &&
++ git refs verify 2>err &&
++ cat >expect <<-EOF &&
++ warning: refs/heads/branch-symbolic-good: symlinkRef: use deprecated symbolic link for symref
++ EOF
++ rm $main_worktree_refdir_prefix/branch-symbolic-good &&
++ test_cmp expect err &&
++
++ ln -sf ../../../../logs/branch-escape $worktree1_refdir_prefix/branch-symbolic &&
++ git refs verify 2>err &&
++ cat >expect <<-EOF &&
++ warning: worktrees/worktree-1/refs/worktree/branch-symbolic: symlinkRef: use deprecated symbolic link for symref
++ warning: worktrees/worktree-1/refs/worktree/branch-symbolic: symrefTargetIsNotARef: points to non-ref target '\''logs/branch-escape'\''
++ EOF
++ rm $worktree1_refdir_prefix/branch-symbolic &&
++ test_cmp expect err &&
++
++ for bad_referent_name in ".tag" "branch "
++ do
++ ln -sf ./"$bad_referent_name" $worktree1_refdir_prefix/bad-symbolic &&
++ test_must_fail git refs verify 2>err &&
++ cat >expect <<-EOF &&
++ warning: worktrees/worktree-1/refs/worktree/bad-symbolic: symlinkRef: use deprecated symbolic link for symref
++ error: worktrees/worktree-1/refs/worktree/bad-symbolic: badReferentName: points to invalid refname '\''worktrees/worktree-1/refs/worktree/$bad_referent_name'\''
++ EOF
++ rm $worktree1_refdir_prefix/bad-symbolic &&
++ test_cmp expect err &&
++
++ ln -sf ../../../../refs/heads/"$bad_referent_name" $worktree1_refdir_prefix/bad-symbolic &&
++ test_must_fail git refs verify 2>err &&
++ cat >expect <<-EOF &&
++ warning: worktrees/worktree-1/refs/worktree/bad-symbolic: symlinkRef: use deprecated symbolic link for symref
++ error: worktrees/worktree-1/refs/worktree/bad-symbolic: badReferentName: points to invalid refname '\''refs/heads/$bad_referent_name'\''
++ EOF
++ rm $worktree1_refdir_prefix/bad-symbolic &&
++ test_cmp expect err &&
++
++ ln -sf ./"$bad_referent_name" $worktree2_refdir_prefix/bad-symbolic &&
++ test_must_fail git refs verify 2>err &&
++ cat >expect <<-EOF &&
++ warning: worktrees/worktree-2/refs/worktree/bad-symbolic: symlinkRef: use deprecated symbolic link for symref
++ error: worktrees/worktree-2/refs/worktree/bad-symbolic: badReferentName: points to invalid refname '\''worktrees/worktree-2/refs/worktree/$bad_referent_name'\''
++ EOF
++ rm $worktree2_refdir_prefix/bad-symbolic &&
++ test_cmp expect err &&
++
++ ln -sf ../../../../refs/heads/"$bad_referent_name" $worktree2_refdir_prefix/bad-symbolic &&
++ test_must_fail git refs verify 2>err &&
++ cat >expect <<-EOF &&
++ warning: worktrees/worktree-2/refs/worktree/bad-symbolic: symlinkRef: use deprecated symbolic link for symref
++ error: worktrees/worktree-2/refs/worktree/bad-symbolic: badReferentName: points to invalid refname '\''refs/heads/$bad_referent_name'\''
++ EOF
++ rm $worktree2_refdir_prefix/bad-symbolic &&
++ test_cmp expect err || return 1
++ done
++'
+
test_expect_success 'ref content checks should work with worktrees' '
test_when_finished "rm -rf repo" &&Signature · 1 line
2.47.0
PATCH v8 1/9 ref: initialize "fsck_ref_report" with zero
In "fsck.c::fsck_refs_error_function", we need to tell whether "oid" and "referent" is NULL. So, we need to always initialize these parameters to NULL instead of letting them point to anywhere when creating a new "fsck_ref_report" structure.
The original code explicitly initializes the "path" member in the "struct fsck_ref_report" to NULL (which implicitly 0-initializes other members in the struct). It is more customary to use "{ 0 }" to express that we are 0-initializing everything. In order to align with the codebase, initialize "fsck_ref_report" with zero.
--- refs/files-backend.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
refs/files-backend.c +1 -1
@@ -3520,7 +3520,7 @@ static int files_fsck_refs_name(struct ref_store *ref_store UNUSED, goto cleanup;if (check_refname_format (iter->basename,REFNAME_ALLOW_ONELEVEL )) { -struct fsck_ref_report report = { .path =NULL }; +struct fsck_ref_report report = {0 };strbuf_addf (&sb,"%s/%s" , refs_check_dir, iter->relative_path); report.path = sb.buf;
Signature · 1 line
2.47.0
PATCH v8 2/9 ref: check the full refname instead of basename
In reply to shejialuoIn "files-backend.c::files_fsck_refs_name", we validate the refname format by using "check_refname_format" to check the basename of the iterator with "REFNAME_ALLOW_ONELEVEL" flag.
However, this is a bad implementation. Although we doesn't allow a single "@" in ".git" directory, we do allow "refs/heads/@". So, we will report an error wrongly when there is a "refs/heads/@" ref by using one level refname "@".
Because we just check one level refname, we either cannot check the other parts of the full refname. And we will ignore the following errors:
"refs/heads/ new-feature/test"
"refs/heads/~new-feature/test"
In order to fix the above problem, enhance "files_fsck_refs_name" to use the full name for "check_refname_format". Then, replace the tests which are related to "@" and add tests to exercise the above situations using for loop to avoid repetition.
--- refs/files-backend.c | 7 ++- t/t0602-reffiles-fsck.sh | 92 ++++++++++++++++++++++++---------------- 2 files changed, 60 insertions(+), 39 deletions(-)
refs/files-backend.c +5 -2
@@ -3519,10 +3519,13 @@ static int files_fsck_refs_name(struct ref_store *ref_store UNUSED,if (iter->basename[0 ] !='.' &&ends_with (iter->basename,".lock" )) goto cleanup; -if (check_refname_format (iter->basename,REFNAME_ALLOW_ONELEVEL )) { +/* +* This works right now because we never check the root refs. +*/ +strbuf_addf (&sb,"%s/%s" , refs_check_dir, iter->relative_path); +if (check_refname_format (sb.buf,0 )) {struct fsck_ref_report report = {0 }; -strbuf_addf (&sb,"%s/%s" , refs_check_dir, iter->relative_path); report.path = sb.buf; ret = fsck_report_ref(o, &report, FSCK_MSG_BAD_REF_NAME,
t/t0602-reffiles-fsck.sh +55 -37
125 lines not on this page · raw message
Signature · 1 line
2.47.0
PATCH v8 3/9 ref: initialize ref name outside of check functions
In reply to shejialuoWe passes "refs_check_dir" to the "files_fsck_refs_name" function which allows it to create the checked ref name later. However, when we introduce a new check function, we have to allocate redundant memory and re-calculate the ref name. It's bad for us to allocate redundant memory and duplicate logic. Instead, we should allocate and calculate it only once and pass the ref name to the check functions.
In order not to do repeat calculation, rename "refs_check_dir" to "refname". And in "files_fsck_refs_dir", create a new strbuf "refname", thus whenever we handle a new ref, calculate the name and call the check functions one by one.
--- refs/files-backend.c | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-)
refs/files-backend.c +13 -8
@@ -3501,12 +3501,12 @@ static int files_ref_store_remove_on_disk(struct ref_store *ref_store,*/ typedef int (*files_fsck_refs_fn )(struct ref_store *ref_store,struct fsck_options *o, -const char *refs_check_dir, +const char *refname,struct dir_iterator *iter); static int files_fsck_refs_name(struct ref_store *ref_store UNUSED,struct fsck_options *o, -const char *refs_check_dir, +const char *refname,struct dir_iterator *iter) {struct strbuf sb =STRBUF_INIT ;
@@ -3522,11 +3522,10 @@ static int files_fsck_refs_name(struct ref_store *ref_store UNUSED,/* * This works right now because we never check the root refs. */ -strbuf_addf (&sb,"%s/%s" , refs_check_dir, iter->relative_path); -if (check_refname_format (sb.buf,0 )) { +if (check_refname_format (refname,0 )) {struct fsck_ref_report report = {0 }; - report.path = sb.buf; + report.path = refname; ret =fsck_report_ref (o, &report,FSCK_MSG_BAD_REF_NAME ,"invalid refname format" );
@@ -3542,6 +3541,7 @@ static int files_fsck_refs_dir(struct ref_store *ref_store,const char *refs_check_dir, files_fsck_refs_fn *fsck_refs_fn) { + struct strbuf refname = STRBUF_INIT;struct strbuf sb =STRBUF_INIT ;struct dir_iterator *iter;int iter_status;
@@ -3560,11 +3560,15 @@ static int files_fsck_refs_dir(struct ref_store *ref_store,continue ; }else if (S_ISREG (iter->st.st_mode) ||S_ISLNK (iter->st.st_mode)) { +strbuf_reset (&refname); +strbuf_addf (&refname,"%s/%s" , refs_check_dir, + iter->relative_path); +if (o->verbose) -fprintf_ln (stderr,"Checking %s/%s" , - refs_check_dir, iter->relative_path); +fprintf_ln (stderr,"Checking %s" , refname.buf); +for (size_t i =0 ; fsck_refs_fn[i]; i++) { -if (fsck_refs_fn[i](ref_store, o, refs_check_dir, iter)) +if (fsck_refs_fn[i](ref_store, o, refname.buf, iter)) ret =-1 ; } }else {
@@ -3581,6 +3585,7 @@ static int files_fsck_refs_dir(struct ref_store *ref_store, out:strbuf_release (&sb); +strbuf_release (&refname);return ret; }
Signature · 1 line
2.47.0
PATCH v8 4/9 ref: support multiple worktrees check for refs
In reply to shejialuoWe have already set up the infrastructure to check the consistency for refs, but we do not support multiple worktrees. However, "git-fsck(1)" will check the refs of worktrees. As we decide to get feature parity with "git-fsck(1)", we need to set up support for multiple worktrees.
Because each worktree has its own specific refs, instead of just showing the users "refs/worktree/foo", we need to display the full name such as "worktrees/<id>/refs/worktree/foo". So we should know the id of the worktree to get the full name. Add a new parameter "struct worktree *" for "refs-internal.h::fsck_fn". Then change the related functions to follow this new interface.
The "packed-refs" only exists in the main worktree, so we should only check "packed-refs" in the main worktree. Use "is_main_worktree" method to skip checking "packed-refs" in "packed_fsck" function.
Then, enhance the "files-backend.c::files_fsck_refs_dir" function to add "worktree/<id>/" prefix when we are not in the main worktree.
Last, add a new test to check the refname when there are multiple worktrees to exercise the code.
--- builtin/refs.c | 10 ++++++-- refs.c | 5 ++-- refs.h | 3 ++- refs/debug.c | 5 ++-- refs/files-backend.c | 17 ++++++++++---- refs/packed-backend.c | 8 ++++++- refs/refs-internal.h | 3 ++- refs/reftable-backend.c | 3 ++- t/t0602-reffiles-fsck.sh | 51 ++++++++++++++++++++++++++++++++++++++++ 9 files changed, 90 insertions(+), 15 deletions(-)
builtin/refs.c +8 -2
@@ -5,6 +5,7 @@#include "parse-options.h" #include "refs.h" #include "strbuf.h" +#include "worktree.h" #define REFS_MIGRATE_USAGE \ N_("git refs migrate --ref-format=<format> [--dry-run]")
@@ -66,6 +67,7 @@ static int cmd_refs_migrate(int argc, const char **argv, const char *prefix)static int cmd_refs_verify (int argc,const char **argv,const char *prefix) {struct fsck_options fsck_refs_options =FSCK_REFS_OPTIONS_DEFAULT ; +struct worktree **worktrees;const char *const verify_usage[] = {REFS_VERIFY_USAGE ,NULL ,
@@ -75,7 +77,7 @@ static int cmd_refs_verify(int argc, const char **argv, const char *prefix)OPT_BOOL (0 ,"strict" , &fsck_refs_options.strict,N_ ("enable strict checking" )), OPT_END(), }; -int ret; +int ret =0 ; argc =parse_options (argc, argv, prefix, options, verify_usage,0 );if (argc)
@@ -84,9 +86,13 @@ static int cmd_refs_verify(int argc, const char **argv, const char *prefix)git_config (git_fsck_config, &fsck_refs_options);prepare_repo_settings (the_repository); - ret =refs_fsck (get_main_ref_store (the_repository), &fsck_refs_options); + worktrees =get_worktrees (); +for (size_t i =0 ; worktrees[i]; i++) + ret |=refs_fsck (get_worktree_ref_store (worktrees[i]), + &fsck_refs_options, worktrees[i]);fsck_options_clear (&fsck_refs_options); +free_worktrees (worktrees);return ret; }
refs.c +3 -2
@@ -318,9 +318,10 @@ int check_refname_format(const char *refname, int flags)return check_or_sanitize_refname (refname, flags,NULL ); } -int refs_fsck (struct ref_store *refs,struct fsck_options *o) +int refs_fsck (struct ref_store *refs,struct fsck_options *o, +struct worktree *wt) { -return refs->be->fsck (refs, o); +return refs->be->fsck (refs, o, wt); }void sanitize_refname_component (const char *refname,struct strbuf *out)
refs.h +2 -1
@@ -549,7 +549,8 @@ int check_refname_format(const char *refname, int flags);* reflogs are consistent, and non-zero otherwise. The errors will be * written to stderr. */ -int refs_fsck (struct ref_store *refs,struct fsck_options *o); +int refs_fsck (struct ref_store *refs,struct fsck_options *o, +struct worktree *wt);/* * Apply the rules from check_refname_format, but mutate the result until it
refs/debug.c +3 -2
@@ -420,10 +420,11 @@ static int debug_reflog_expire(struct ref_store *ref_store, const char *refname, }static int debug_fsck (struct ref_store *ref_store, -struct fsck_options *o) +struct fsck_options *o, +struct worktree *wt) {struct debug_ref_store *drefs = (struct debug_ref_store *)ref_store; -int res = drefs->refs->be->fsck (drefs->refs, o); +int res = drefs->refs->be->fsck (drefs->refs, o, wt);trace_printf_key (&trace_refs,"fsck: %d\n" , res);return res; }
refs/files-backend.c +12 -5
@@ -23,6 +23,7 @@#include "../dir.h" #include "../chdir-notify.h" #include "../setup.h" +#include "../worktree.h" #include "../wrapper.h" #include "../write-or-die.h" #include "../revision.h"
@@ -3539,6 +3540,7 @@ static int files_fsck_refs_name(struct ref_store *ref_store UNUSED,static int files_fsck_refs_dir (struct ref_store *ref_store,struct fsck_options *o,const char *refs_check_dir, +struct worktree *wt,files_fsck_refs_fn *fsck_refs_fn) {struct strbuf refname =STRBUF_INIT ;
@@ -3561,6 +3563,9 @@ static int files_fsck_refs_dir(struct ref_store *ref_store, }else if (S_ISREG (iter->st.st_mode) ||S_ISLNK (iter->st.st_mode)) {strbuf_reset (&refname); + +if (!is_main_worktree (wt)) +strbuf_addf (&refname,"worktrees/%s/" , wt->id);strbuf_addf (&refname,"%s/%s" , refs_check_dir, iter->relative_path);
@@ -3590,7 +3595,8 @@ static int files_fsck_refs_dir(struct ref_store *ref_store, }static int files_fsck_refs (struct ref_store *ref_store, -struct fsck_options *o) +struct fsck_options *o, +struct worktree *wt) {files_fsck_refs_fn fsck_refs_fn[]= { files_fsck_refs_name,
@@ -3599,17 +3605,18 @@ static int files_fsck_refs(struct ref_store *ref_store,if (o->verbose)fprintf_ln (stderr,_ ("Checking references consistency" )); -return files_fsck_refs_dir (ref_store, o,"refs" , fsck_refs_fn); +return files_fsck_refs_dir (ref_store, o,"refs" , wt, fsck_refs_fn); }static int files_fsck (struct ref_store *ref_store, -struct fsck_options *o) +struct fsck_options *o, +struct worktree *wt) {struct files_ref_store *refs =files_downcast (ref_store,REF_STORE_READ ,"fsck" ); -return files_fsck_refs (ref_store, o) | - refs->packed_ref_store->be->fsck (refs->packed_ref_store, o); +return files_fsck_refs (ref_store, o, wt) | + refs->packed_ref_store->be->fsck (refs->packed_ref_store, o, wt); }struct ref_storage_be refs_be_files = {
refs/packed-backend.c +7 -1
@@ -13,6 +13,7 @@#include "../lockfile.h" #include "../chdir-notify.h" #include "../statinfo.h" +#include "../worktree.h" #include "../wrapper.h" #include "../write-or-die.h" #include "../trace2.h"
@@ -1754,8 +1755,13 @@ static struct ref_iterator *packed_reflog_iterator_begin(struct ref_store *ref_s } static int packed_fsck(struct ref_store *ref_store UNUSED, - struct fsck_options *o UNUSED) + struct fsck_options *o UNUSED, +struct worktree *wt) { + +if (!is_main_worktree (wt)) +return 0 ; +return 0 ; }
refs/refs-internal.h +2 -1
@@ -653,7 +653,8 @@ typedef int read_symbolic_ref_fn(struct ref_store *ref_store, const char *refnam struct strbuf *referent);typedef int fsck_fn (struct ref_store *ref_store, -struct fsck_options *o); +struct fsck_options *o, +struct worktree *wt);struct ref_storage_be {const char *name;
refs/reftable-backend.c +2 -1
@@ -2443,7 +2443,8 @@ static int reftable_be_reflog_expire(struct ref_store *ref_store, } static int reftable_be_fsck(struct ref_store *ref_store UNUSED, - struct fsck_options *o UNUSED) + struct fsck_options *o UNUSED, + struct worktree *wt UNUSED) {return 0 ; }
t/t0602-reffiles-fsck.sh +51 -0
55 lines not on this page · raw message
Signature · 1 line
2.47.0
PATCH v8 5/9 ref: port git-fsck(1) regular refs check for files backend
In reply to shejialuo"git-fsck(1)" implicitly checks the ref content by passing the callback "fsck_handle_ref" to the "refs.c::refs_for_each_rawref". Then, it will check whether the ref content (eventually "oid") is valid. If not, it will report the following error to the user.
error: refs/heads/main: invalid sha1 pointer 0000...
And it will also report above errors when there are dangling symrefs in the repository wrongly. This does not align with the behavior of the "git symbolic-ref" command which allows users to create dangling symrefs.
As we have already introduced the "git refs verify" command, we'd better check the ref content explicitly in the "git refs verify" command thus later we could remove these checks in "git-fsck(1)" and launch a subprocess to call "git refs verify" in "git-fsck(1)" to make the "git-fsck(1)" more clean.
Following what "git-fsck(1)" does, 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.
--- Documentation/fsck-msgids.txt | 3 + fsck.h | 1 + refs/files-backend.c | 48 ++++++++++++++++ t/t0602-reffiles-fsck.sh | 105 ++++++++++++++++++++++++++++++++++ 4 files changed, 157 insertions(+)
Documentation/fsck-msgids.txt +3 -0
@@ -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.
fsck.h +1 -0
@@ -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 ) \
refs/files-backend.c +48 -0
60 lines not on this page · raw message
t/t0602-reffiles-fsck.sh +105 -0
109 lines not on this page · raw message
Signature · 1 line
2.47.0
Re: [PATCH v8 5/9] ref: port git-fsck(1) regular refs check for files backend
refs/files-backend.c typedef int (*files_fsck_refs_fn)(struct ref_store *ref_store, · line 3526
Quoted 1 line · shejialuo
refs/files-backend.c +1
-0+if (strbuf_read_file (&ref_content, iter->path.buf,0 ) <0 ) {
Nit: there's a space too much here now.
refs/files-backend.c typedef int (*files_fsck_refs_fn)(struct ref_store *ref_store, · line 3537
Quoted 11 lines
refs/files-backend.c +11
-0+/* +* Ref file could be removed by another concurrent process. We should +* ignore this error and continue to the next ref. +*/ +if (errno ==ENOENT ) + goto cleanup; + + ret =error_errno (_ ("cannot read ref file '%s': %s" ), + iter->path.buf,strerror (errno)); + goto cleanup; + }
You report errno twice. This should be:
ret = error_errno(_("cannot read ref file '%s'"), iter->path.buf);Other than that this version looks good to me, thanks!
Patrick
Re: [PATCH v8 5/9] ref: port git-fsck(1) regular refs check for files backend
Quoted 5 lines · Patrick Steinhardt
refs/files-backend.c typedef int (*files_fsck_refs_fn)(struct ref_store *ref_store, · line 3526
Quoted 1 line · shejialuo
refs/files-backend.c +1
-0+if (strbuf_read_file (&ref_content, iter->path.buf,0 ) <0 ) {Nit: there's a space too much here now.
I will improve this in the next version.
Quoted 14 lines
refs/files-backend.c typedef int (*files_fsck_refs_fn)(struct ref_store *ref_store, · line 3537
Quoted 11 lines
refs/files-backend.c +11
-0+/* +* Ref file could be removed by another concurrent process. We should +* ignore this error and continue to the next ref. +*/ +if (errno ==ENOENT ) + goto cleanup; + + ret =error_errno (_ ("cannot read ref file '%s': %s" ), + iter->path.buf,strerror (errno)); + goto cleanup; + }You report
errnotwice. This should be:
Quoted 4 lines
ret = error_errno(_("cannot read ref file '%s'"), iter->path.buf);Other than that this version looks good to me, thanks!
Opps, I didn't think about it, I just copied it. I will fix this in the next version.
Quoted 1 line
Patrick
PATCH v8 6/9 ref: add more strict checks for regular refs
In reply to shejialuoWe have already used "parse_loose_ref_contents" function to check whether the ref content is valid in files backend. However, by using "parse_loose_ref_contents", we allow the ref's content to end with garbage or without a newline.
Even though we never create such loose refs ourselves, we have accepted such loose refs. So, it is entirely possible that some third-party tools may rely on such loose refs being valid. We should not report an error fsck message at current. We should notify the users about such "curiously formatted" loose refs so that adequate care is taken before we decide to tighten the rules in the future.
And it's not suitable either to report a warn fsck message to the user. We don't yet want the "--strict" flag that controls this bit to end up generating errors for such weirdly-formatted reference contents, as we first want to assess whether this retroactive tightening will cause issues for any tools out there. It may cause compatibility issues which may break the repository. So, we add the following two fsck infos to represent the situation where the ref content ends without newline or has trailing garbages:
1. refMissingNewline(INFO): A loose ref that does not end with newline(LF).
2. trailingRefContent(INFO): A loose ref has trailing content.
It might appear that we can't provide the user with any warnings by using FSCK_INFO. However, in "fsck.c::fsck_vreport", we will convert FSCK_INFO to FSCK_WARN and we can still warn the user about these situations when using "git refs verify" without introducing compatibility issues.
--- Documentation/fsck-msgids.txt | 14 +++++++++ fsck.h | 2 ++ refs.c | 2 +- refs/files-backend.c | 26 ++++++++++++++-- refs/refs-internal.h | 2 +- t/t0602-reffiles-fsck.sh | 57 +++++++++++++++++++++++++++++++++-- 6 files changed, 96 insertions(+), 7 deletions(-)
Documentation/fsck-msgids.txt +14 -0
@@ -173,6 +173,20 @@ `nullSha1`:: (WARN) Tree contains entries pointing to a null sha1. +`refMissingNewline`:: + (INFO) A loose ref that does not end with newline(LF). As + valid implementations of Git never created such a loose ref + file, it may become an error in the future. Report to the + [email protected] mailing list if you see this error, as + we need to know what tools created such a file. + +`trailingRefContent`:: + (INFO) A loose ref has trailing content. As valid implementations + of Git never created such a loose ref file, it may become an + error in the future. Report to the [email protected] mailing + list if you see this error, as we need to know what tools + created such a file. + `treeNotSorted`:: (ERROR) A tree is not properly sorted.
fsck.h +2 -0
@@ -85,6 +85,8 @@ enum fsck_msg_type { FUNC(MAILMAP_SYMLINK, INFO) \ FUNC(BAD_TAG_NAME, INFO) \ FUNC(MISSING_TAGGER_ENTRY, INFO) \ + FUNC(REF_MISSING_NEWLINE, INFO) \ +FUNC (TRAILING_REF_CONTENT ,INFO ) \ /* ignored (elevated when requested) */ \FUNC (EXTRA_HEADER_ENTRY ,IGNORE )
refs.c +1 -1
@@ -1789,7 +1789,7 @@ static int refs_read_special_head(struct ref_store *ref_store, } result =parse_loose_ref_contents (ref_store->repo->hash_algo, content.buf, - oid, referent, type, failure_errno); + oid, referent, type,NULL , failure_errno); done:strbuf_release (&full_path);
refs/files-backend.c +23 -3
@@ -569,7 +569,7 @@ static int read_ref_internal(struct ref_store *ref_store, const char *refname, buf = sb_contents.buf; ret =parse_loose_ref_contents (ref_store->repo->hash_algo, buf, - oid, referent, type, &myerr); + oid, referent, type,NULL , &myerr); out:if (ret && !myerr)
@@ -606,7 +606,7 @@ static int files_read_symbolic_ref(struct ref_store *ref_store, const char *refnint parse_loose_ref_contents (const struct git_hash_algo *algop,const char *buf,struct object_id *oid,struct strbuf *referent,unsigned int *type, -int *failure_errno) +const char **trailing,int *failure_errno) {const char *p;if (skip_prefix (buf,"ref:" , &buf)) {
@@ -628,6 +628,10 @@ int parse_loose_ref_contents(const struct git_hash_algo *algop, *failure_errno =EINVAL ;return -1 ; } + +if (trailing) + *trailing = p; +return 0 ; }
@@ -3513,6 +3517,7 @@ static int files_fsck_refs_content(struct ref_store *ref_store,struct strbuf ref_content =STRBUF_INIT ;struct strbuf referent =STRBUF_INIT ;struct fsck_ref_report report = {0 }; +const char *trailing =NULL ;unsigned int type =0 ;int failure_errno =0 ;struct object_id oid;
@@ -3538,7 +3543,7 @@ static int files_fsck_refs_content(struct ref_store *ref_store,if (parse_loose_ref_contents (ref_store->repo->hash_algo, ref_content.buf, &oid, &referent, - &type, &failure_errno)) { + &type, &trailing, &failure_errno)) {strbuf_rtrim (&ref_content); ret = fsck_report_ref(o, &report, FSCK_MSG_BAD_REF_CONTENT,
@@ -3546,6 +3551,21 @@ static int files_fsck_refs_content(struct ref_store *ref_store, goto cleanup; } +if (!(type &REF_ISSYMREF )) { +if (!*trailing) { + ret =fsck_report_ref (o, &report, +FSCK_MSG_REF_MISSING_NEWLINE , +"misses LF at the end" ); + goto cleanup; + } +if (*trailing !='\n' || *(trailing +1 )) { + ret =fsck_report_ref (o, &report, +FSCK_MSG_TRAILING_REF_CONTENT , +"has trailing garbage: '%s'" , trailing); + goto cleanup; + } + } + cleanup:strbuf_release (&ref_content);strbuf_release (&referent);
refs/refs-internal.h +1 -1
@@ -716,7 +716,7 @@ struct ref_store {int parse_loose_ref_contents (const struct git_hash_algo *algop,const char *buf,struct object_id *oid,struct strbuf *referent,unsigned int *type, -int *failure_errno); +const char **trailing,int *failure_errno);/* * Fill in the generic part of refs and add it to our collection of
t/t0602-reffiles-fsck.sh +55 -2
81 lines not on this page · raw message
Signature · 1 line
2.47.0
PATCH v8 7/9 ref: add basic symref content check for files backend
In reply to shejialuoWe have code that checks regular ref contents, but we do not yet check the contents of symbolic refs. By using "parse_loose_ref_content" for symbolic refs, we will get the information of the "referent".
We do not need to check the "referent" by opening the file. This is because if "referent" exists in the file system, we will eventually check its correctness by inspecting every file in the "refs" directory. If the "referent" does not exist in the filesystem, this is OK as it is seen as the dangling symref.
So we just need to check the "referent" string content. A regular ref could be accepted as a textual symref if it begins with "ref:", followed by zero or more whitespaces, followed by the full refname, followed only by whitespace characters. However, we always write a single SP after "ref:" and a single LF after the refname. It may seem that we should report a fsck error message when the "referent" does not apply above rules and we should not be so aggressive because third-party reimplementations of Git may have taken advantage of the looser syntax. Put it more specific, we accept the following contents:
1. "ref: refs/heads/master "
2. "ref: refs/heads/master \n \n"
3. "ref: refs/heads/master\n\n"
When introducing the regular ref content checks, we created two fsck infos "refMissingNewline" and "trailingRefContent" which exactly represents above situations. So we will reuse these two fsck messages to write checks to info the user about these situations.
But we do not allow any other trailing garbage. The followings are bad symref contents which will be reported as fsck error by "git-fsck(1)".
1. "ref: refs/heads/master garbage\n"
2. "ref: refs/heads/master \n\n\n garbage "
And we introduce a new "badReferentName(ERROR)" fsck message to report above errors by using "is_root_ref" and "check_refname_format" to check the "referent". Since both "is_root_ref" and "check_refname_format" don't work with whitespaces, we use the trimmed version of "referent" with these functions.
In order to add checks, we will do the following things:
1. Record the untrimmed length "orig_len" and untrimmed last byte "orig_last_byte".
2. Use "strbuf_rtrim" to trim the whitespaces or newlines to make sure "is_root_ref" and "check_refname_format" won't be failed by them.
3. Use "orig_len" and "orig_last_byte" to check whether the "referent" misses '\n' at the end or it has trailing whitespaces or newlines.
--- Documentation/fsck-msgids.txt | 3 + fsck.h | 1 + refs/files-backend.c | 40 ++++++++++++ t/t0602-reffiles-fsck.sh | 111 ++++++++++++++++++++++++++++++++++ 4 files changed, 155 insertions(+)
Documentation/fsck-msgids.txt +3 -0
@@ -28,6 +28,9 @@ `badRefName`:: (ERROR) A ref has an invalid format. +`badReferentName`:: + (ERROR) The referent name of a symref is invalid. + `badTagName`:: (INFO) A tag has an invalid format.
fsck.h +1 -0
@@ -34,6 +34,7 @@ enum fsck_msg_type { FUNC(BAD_REF_CONTENT, ERROR) \ FUNC(BAD_REF_FILETYPE, ERROR) \ FUNC(BAD_REF_NAME, ERROR) \ + FUNC(BAD_REFERENT_NAME, ERROR) \ FUNC(BAD_TIMEZONE, ERROR) \ FUNC(BAD_TREE, ERROR) \FUNC (BAD_TREE_SHA1 ,ERROR ) \
refs/files-backend.c +40 -0
@@ -3509,6 +3509,43 @@ typedef int (*files_fsck_refs_fn)(struct ref_store *ref_store,const char *refname, struct dir_iterator *iter); +static int files_fsck_symref_target (struct fsck_options *o, +struct fsck_ref_report *report, +struct strbuf *referent) +{ +char orig_last_byte; +size_t orig_len; +int ret =0 ; + + orig_len = referent->len; + orig_last_byte = referent->buf[orig_len -1 ]; +strbuf_rtrim (referent); + +if (!is_root_ref (referent->buf) && +check_refname_format (referent->buf,0 )) { + ret =fsck_report_ref (o, report, +FSCK_MSG_BAD_REFERENT_NAME , +"points to invalid refname '%s'" , referent->buf); + goto out; + } + +if (referent->len == orig_len || + (referent->len < orig_len && orig_last_byte !='\n' )) { + ret =fsck_report_ref (o, report, +FSCK_MSG_REF_MISSING_NEWLINE , +"misses LF at the end" ); + } + +if (referent->len != orig_len && referent->len != orig_len -1 ) { + ret =fsck_report_ref (o, report, +FSCK_MSG_TRAILING_REF_CONTENT , +"has trailing whitespaces or newlines" ); + } + +out: +return ret; +} + static int files_fsck_refs_content(struct ref_store *ref_store, struct fsck_options *o, const char *target_name,
@@ -3564,6 +3601,9 @@ static int files_fsck_refs_content(struct ref_store *ref_store, "has trailing garbage: '%s'", trailing); goto cleanup; } + }else { + ret =files_fsck_symref_target (o, &report, &referent); + goto cleanup; } cleanup:
t/t0602-reffiles-fsck.sh +111 -0
123 lines not on this page · raw message
Signature · 1 line
2.47.0
PATCH v8 8/9 ref: check whether the target of the symref is a ref
In reply to shejialuoIdeally, we want to the users use "git symbolic-ref" to create symrefs instead of writing raw contents into the filesystem. However, "git symbolic-ref" is strict with the refname but not strict with the referent. For example, we can make the "referent" located at the "$(gitdir)/logs/aaa" and manually write the content into this where we can still successfully parse this symref by using "git rev-parse".
$ git init repo && cd repo && git commit --allow-empty -mx $ git symbolic-ref refs/heads/test logs/aaa $ echo $(git rev-parse HEAD) > .git/logs/aaa $ git rev-parse test
We may need to add some restrictions for "referent" parameter when using "git symbolic-ref" to create symrefs because ideally all the nonpseudo-refs should be located under the "refs" directory and we may tighten this in the future.
In order to tell the user we may tighten the above situation, create a new fsck message "symrefTargetIsNotARef" to notify the user that this may become an error in the future.
--- Documentation/fsck-msgids.txt | 9 +++++++++ fsck.h | 1 + refs/files-backend.c | 14 ++++++++++++-- t/t0602-reffiles-fsck.sh | 29 +++++++++++++++++++++++++++++ 4 files changed, 51 insertions(+), 2 deletions(-)
Documentation/fsck-msgids.txt +9 -0
@@ -183,6 +183,15 @@ [email protected] mailing list if you see this error, as we need to know what tools created such a file. +`symrefTargetIsNotARef`:: + (INFO) The target of a symbolic reference points neither to + a root reference nor to a reference starting with "refs/". + Although we allow create a symref pointing to the referent which + is outside the "ref" by using `git symbolic-ref`, we may tighten + the rule in the future. Report to the [email protected] + mailing list if you see this error, as we need to know what tools + created such a file. + `trailingRefContent`:: (INFO) A loose ref has trailing content. As valid implementations of Git never created such a loose ref file, it may become an
fsck.h +1 -0
@@ -87,6 +87,7 @@ enum fsck_msg_type { FUNC(BAD_TAG_NAME, INFO) \ FUNC(MISSING_TAGGER_ENTRY, INFO) \ FUNC(REF_MISSING_NEWLINE, INFO) \ + FUNC(SYMREF_TARGET_IS_NOT_A_REF, INFO) \FUNC (TRAILING_REF_CONTENT ,INFO ) \ /* ignored (elevated when requested) */ \FUNC (EXTRA_HEADER_ENTRY ,IGNORE )
refs/files-backend.c +12 -2
@@ -3513,6 +3513,7 @@ static int files_fsck_symref_target(struct fsck_options *o,struct fsck_ref_report *report, struct strbuf *referent) { + int is_referent_root;char orig_last_byte;size_t orig_len;int ret =0 ;
@@ -3521,8 +3522,17 @@ static int files_fsck_symref_target(struct fsck_options *o, orig_last_byte = referent->buf[orig_len -1 ];strbuf_rtrim (referent); -if (!is_root_ref (referent->buf) && -check_refname_format (referent->buf,0 )) { + is_referent_root =is_root_ref (referent->buf); +if (!is_referent_root && + !starts_with (referent->buf,"refs/" ) && + !starts_with (referent->buf,"worktrees/" )) { + ret =fsck_report_ref (o, report, +FSCK_MSG_SYMREF_TARGET_IS_NOT_A_REF , +"points to non-ref target '%s'" , referent->buf); + + } + +if (!is_referent_root &&check_refname_format (referent->buf,0 )) { ret =fsck_report_ref (o, report,FSCK_MSG_BAD_REFERENT_NAME ,"points to invalid refname '%s'" , referent->buf);
t/t0602-reffiles-fsck.sh +29 -0
@@ -366,6 +366,35 @@ test_expect_success 'textual symref content should be checked (aggregate)' ' test_cmp expect sorted_err ' +test_expect_success 'the target of the textual symref should be checked' ' + 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" && + + for good_referent in "refs/heads/branch" "HEAD" "refs/tags/tag" + do + printf "ref: %s\n" $good_referent >$branch_dir_prefix/branch-good && + git refs verify 2>err && + rm $branch_dir_prefix/branch-good && + test_must_be_empty err || return 1 + done && + + for nonref_referent in "refs-back/heads/branch" "refs-back/tags/tag" "reflogs/refs/heads/branch" + do + printf "ref: %s\n" $nonref_referent >$branch_dir_prefix/branch-bad-1 && + git refs verify 2>err && + cat >expect <<-EOF && + warning: refs/heads/branch-bad-1: symrefTargetIsNotARef: points to non-ref target '\''$nonref_referent'\'' + EOF + rm $branch_dir_prefix/branch-bad-1 && + test_cmp expect err || return 1 + done +' + test_expect_success 'ref content checks should work with worktrees' ' test_when_finished "rm -rf repo" && git init repo &&
Signature · 1 line
2.47.0
PATCH v8 9/9 ref: add symlink ref content check for files backend
In reply to shejialuoBesides the textual symref, we also allow symbolic links as the symref. So, we should also provide the consistency check as what we have done for textual symref. And also we consider deprecating writing the symbolic links. We first need to access whether symbolic links still be used. So, add a new fsck message "symlinkRef(INFO)" to tell the user be aware of this information.
We have already introduced "files_fsck_symref_target". We should reuse this function to handle the symrefs which use legacy symbolic links. We should not check the trailing garbage for symbolic refs. Add a new parameter "symbolic_link" to disable some checks which should only be executed for textual symrefs.
And we need to also generate the "referent" parameter for reusing "files_fsck_symref_target" by the following steps:
1. Use "strbuf_add_real_path" to resolve the symlink and get the absolute path "ref_content" which the symlink ref points to.
2. Generate the absolute path "abs_gitdir" of "gitdir" and combine "ref_content" and "abs_gitdir" to extract the relative path "relative_referent_path".
3. If "ref_content" is outside of "gitdir", we just set "referent" with "ref_content". Instead, we set "referent" with "relative_referent_path".
--- Documentation/fsck-msgids.txt | 6 ++ fsck.h | 1 + refs/files-backend.c | 38 ++++++++- t/t0602-reffiles-fsck.sh | 141 ++++++++++++++++++++++++++++++++++ 4 files changed, 182 insertions(+), 4 deletions(-)
Documentation/fsck-msgids.txt +6 -0
@@ -183,6 +183,12 @@ [email protected] mailing list if you see this error, as we need to know what tools created such a file. +`symlinkRef`:: + (INFO) A symbolic link is used as a symref. Report to the + [email protected] mailing list if you see this error, as we + are assessing the feasibility of dropping the support to drop + creating symbolic links as symrefs. + `symrefTargetIsNotARef`:: (INFO) The target of a symbolic reference points neither to a root reference nor to a reference starting with "refs/".
fsck.h +1 -0
@@ -86,6 +86,7 @@ enum fsck_msg_type { FUNC(MAILMAP_SYMLINK, INFO) \ FUNC(BAD_TAG_NAME, INFO) \ FUNC(MISSING_TAGGER_ENTRY, INFO) \ + FUNC(SYMLINK_REF, INFO) \ FUNC(REF_MISSING_NEWLINE, INFO) \ FUNC(SYMREF_TARGET_IS_NOT_A_REF, INFO) \FUNC (TRAILING_REF_CONTENT ,INFO ) \
refs/files-backend.c +34 -4
@@ -1,6 +1,7 @@#define USE_THE_REPOSITORY_VARIABLE #include "../git-compat-util.h" +#include "../abspath.h" #include "../config.h" #include "../copy.h" #include "../environment.h"
@@ -3511,7 +3512,8 @@ typedef int (*files_fsck_refs_fn)(struct ref_store *ref_store,static int files_fsck_symref_target (struct fsck_options *o,struct fsck_ref_report *report, -struct strbuf *referent) +struct strbuf *referent, +unsigned int symbolic_link) {int is_referent_root;char orig_last_byte;
@@ -3520,7 +3522,8 @@ static int files_fsck_symref_target(struct fsck_options *o, orig_len = referent->len; orig_last_byte = referent->buf[orig_len -1 ]; -strbuf_rtrim (referent); +if (!symbolic_link) +strbuf_rtrim (referent); is_referent_root =is_root_ref (referent->buf); if (!is_referent_root &&
@@ -3539,6 +3542,9 @@ static int files_fsck_symref_target(struct fsck_options *o, goto out; } +if (symbolic_link) + goto out; +if (referent->len == orig_len || (referent->len < orig_len && orig_last_byte !='\n' )) { ret = fsck_report_ref(o, report,
@@ -3562,6 +3568,7 @@ static int files_fsck_refs_content(struct ref_store *ref_store, struct dir_iterator *iter) {struct strbuf ref_content =STRBUF_INIT ; +struct strbuf abs_gitdir =STRBUF_INIT ;struct strbuf referent =STRBUF_INIT ;struct fsck_ref_report report = {0 };const char *trailing =NULL ;
@@ -3572,8 +3579,30 @@ static int files_fsck_refs_content(struct ref_store *ref_store, report.path = target_name; -if (S_ISLNK (iter->st.st_mode)) +if (S_ISLNK (iter->st.st_mode)) { +const char *relative_referent_path =NULL ; + + ret =fsck_report_ref (o, &report, +FSCK_MSG_SYMLINK_REF , +"use deprecated symbolic link for symref" ); + +strbuf_add_absolute_path (&abs_gitdir, ref_store->repo->gitdir); +strbuf_normalize_path (&abs_gitdir); +if (!is_dir_sep (abs_gitdir.buf[abs_gitdir.len -1 ])) +strbuf_addch (&abs_gitdir,'/' ); + +strbuf_add_real_path (&ref_content, iter->path.buf); +skip_prefix (ref_content.buf, abs_gitdir.buf, + &relative_referent_path); + +if (relative_referent_path) +strbuf_addstr (&referent, relative_referent_path); +else +strbuf_addbuf (&referent, &ref_content); + + ret |=files_fsck_symref_target (o, &report, &referent,1 ); goto cleanup; + }if (strbuf_read_file (&ref_content, iter->path.buf,0 ) <0 ) {/*
@@ -3612,13 +3641,14 @@ static int files_fsck_refs_content(struct ref_store *ref_store, goto cleanup; } }else { - ret =files_fsck_symref_target (o, &report, &referent); + ret =files_fsck_symref_target (o, &report, &referent,0 ); goto cleanup; } cleanup:strbuf_release (&ref_content);strbuf_release (&referent); +strbuf_release (&abs_gitdir);return ret; }
t/t0602-reffiles-fsck.sh +141 -0
147 lines not on this page · raw message
Signature · 1 line
2.47.0
Re: [PATCH v8 0/9] add ref content check for files backend
In reply to shejialuoQuoted 13 lines · shejialuo
Hi all:
This new version solves the following problem:
1. when reading the content of the ref file, we do not use "
fsck_report_ref" function. It's not suitable.
2. Add a new test for symlink worktree test in the last patch. After writing the tets, find a bug. Fix the bug described below.Because we have introduced the check for worktrees, we should not use "
ref_store->gitdir", instead we need to use "ref_store->repo->gitdir" to get the main worktree "gitdir". After fixing this, the test is passed.
Quoted 5 lines
Thank Patrick to remind me about this. I forgot to add test thus making mistakes.
Thanks,
Jialuo
I'd like to wait for couple of days for more reviews and comments from Junio and Karthik.
v9 cover add ref content check for files backend
In reply to shejialuoHi All:
This version fixes two problems:
1. Remove unnecessary space.
2. Drop extra "strerror(errno)".
Thanks,
Jialuo
shejialuo (9):
ref: initialize "fsck_ref_report" with zero
ref: check the full refname instead of basename
ref: initialize ref name outside of check functions
ref: support multiple worktrees check for refs
ref: port git-fsck(1) regular refs check for files backend
ref: add more strict checks for regular refs
ref: add basic symref content check for files backend
ref: check whether the target of the symref is a ref
ref: add symlink ref content check for files backend
Documentation/fsck-msgids.txt | 35 +++ builtin/refs.c | 10 +- fsck.h | 6 + refs.c | 7 +- refs.h | 3 +- refs/debug.c | 5 +- refs/files-backend.c | 194 +++++++++++- refs/packed-backend.c | 8 +- refs/refs-internal.h | 5 +- refs/reftable-backend.c | 3 +- t/t0602-reffiles-fsck.sh | 576 ++++++++++++++++++++++++++++++++-- 11 files changed, 790 insertions(+), 62 deletions(-)
Range-diff against v8:
1: bfb2a21af4 = 1: bfb2a21af4 ref: initialize "fsck_ref_report" with zero
2: 9efc83f7ea = 2: 9efc83f7ea ref: check the full refname instead of basename
3: 5ea7d18203 = 3: 5ea7d18203 ref: initialize ref name outside of check functions
4: cb4669b64d = 4: cb4669b64d ref: support multiple worktrees check for refs
5: c6c128c922 ! 5: d6188063d9 ref: port git-fsck(1) regular refs check for files backend
@@ refs/files-backend.c: typedef int (*files_fsck_refs_fn)(struct ref_store *ref_st
+ if (S_ISLNK(iter->st.st_mode))
+ goto cleanup;
+
-+ if (strbuf_read_file(&ref_content, iter->path.buf, 0) < 0 ) {
++ if (strbuf_read_file(&ref_content, iter->path.buf, 0) < 0) {
+ /*
+ * Ref file could be removed by another concurrent process. We should
+ * ignore this error and continue to the next ref.
@@ refs/files-backend.c: typedef int (*files_fsck_refs_fn)(struct ref_store *ref_st
+ if (errno == ENOENT)
+ goto cleanup;
+
-+ ret = error_errno(_("cannot read ref file '%s': %s"),
-+ iter->path.buf, strerror(errno));
++ ret = error_errno(_("cannot read ref file '%s'"), iter->path.buf);
+ goto cleanup;
+ }
+
6: 911fa42717 = 6: e5e97ba3ad ref: add more strict checks for regular refs
7: 7aa6a99206 = 7: 1dec0a56d2 ref: add basic symref content check for files backend
8: dbb0787ad1 = 8: dcc4a02102 ref: check whether the target of the symref is a ref
9: a6d85b4864 ! 9: fc10862f6f ref: add symlink ref content check for files backend @@ refs/files-backend.c: static int files_fsck_refs_content(struct ref_store *ref_s
goto cleanup;
+ }
- if (strbuf_read_file(&ref_content, iter->path.buf, 0) < 0 ) {
+ if (strbuf_read_file(&ref_content, iter->path.buf, 0) < 0) {
/*
@@ refs/files-backend.c: static int files_fsck_refs_content(struct ref_store *ref_store,
goto cleanup;
Signature · 1 line
2.47.0
PATCH v9 1/9 ref: initialize "fsck_ref_report" with zero
In "fsck.c::fsck_refs_error_function", we need to tell whether "oid" and "referent" is NULL. So, we need to always initialize these parameters to NULL instead of letting them point to anywhere when creating a new "fsck_ref_report" structure.
The original code explicitly initializes the "path" member in the "struct fsck_ref_report" to NULL (which implicitly 0-initializes other members in the struct). It is more customary to use "{ 0 }" to express that we are 0-initializing everything. In order to align with the codebase, initialize "fsck_ref_report" with zero.
--- refs/files-backend.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
refs/files-backend.c +1 -1
@@ -3520,7 +3520,7 @@ static int files_fsck_refs_name(struct ref_store *ref_store UNUSED, goto cleanup;if (check_refname_format (iter->basename,REFNAME_ALLOW_ONELEVEL )) { -struct fsck_ref_report report = { .path =NULL }; +struct fsck_ref_report report = {0 };strbuf_addf (&sb,"%s/%s" , refs_check_dir, iter->relative_path); report.path = sb.buf;
Signature · 1 line
2.47.0
PATCH v9 2/9 ref: check the full refname instead of basename
In reply to shejialuoIn "files-backend.c::files_fsck_refs_name", we validate the refname format by using "check_refname_format" to check the basename of the iterator with "REFNAME_ALLOW_ONELEVEL" flag.
However, this is a bad implementation. Although we doesn't allow a single "@" in ".git" directory, we do allow "refs/heads/@". So, we will report an error wrongly when there is a "refs/heads/@" ref by using one level refname "@".
Because we just check one level refname, we either cannot check the other parts of the full refname. And we will ignore the following errors:
"refs/heads/ new-feature/test"
"refs/heads/~new-feature/test"
In order to fix the above problem, enhance "files_fsck_refs_name" to use the full name for "check_refname_format". Then, replace the tests which are related to "@" and add tests to exercise the above situations using for loop to avoid repetition.
--- refs/files-backend.c | 7 ++- t/t0602-reffiles-fsck.sh | 92 ++++++++++++++++++++++++---------------- 2 files changed, 60 insertions(+), 39 deletions(-)
refs/files-backend.c +5 -2
@@ -3519,10 +3519,13 @@ static int files_fsck_refs_name(struct ref_store *ref_store UNUSED,if (iter->basename[0 ] !='.' &&ends_with (iter->basename,".lock" )) goto cleanup; -if (check_refname_format (iter->basename,REFNAME_ALLOW_ONELEVEL )) { +/* +* This works right now because we never check the root refs. +*/ +strbuf_addf (&sb,"%s/%s" , refs_check_dir, iter->relative_path); +if (check_refname_format (sb.buf,0 )) {struct fsck_ref_report report = {0 }; -strbuf_addf (&sb,"%s/%s" , refs_check_dir, iter->relative_path); report.path = sb.buf; ret = fsck_report_ref(o, &report, FSCK_MSG_BAD_REF_NAME,
t/t0602-reffiles-fsck.sh +55 -37
125 lines not on this page · raw message
Signature · 1 line
2.47.0
PATCH v9 3/9 ref: initialize ref name outside of check functions
In reply to shejialuoWe passes "refs_check_dir" to the "files_fsck_refs_name" function which allows it to create the checked ref name later. However, when we introduce a new check function, we have to allocate redundant memory and re-calculate the ref name. It's bad for us to allocate redundant memory and duplicate logic. Instead, we should allocate and calculate it only once and pass the ref name to the check functions.
In order not to do repeat calculation, rename "refs_check_dir" to "refname". And in "files_fsck_refs_dir", create a new strbuf "refname", thus whenever we handle a new ref, calculate the name and call the check functions one by one.
--- refs/files-backend.c | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-)
refs/files-backend.c +13 -8
@@ -3501,12 +3501,12 @@ static int files_ref_store_remove_on_disk(struct ref_store *ref_store,*/ typedef int (*files_fsck_refs_fn )(struct ref_store *ref_store,struct fsck_options *o, -const char *refs_check_dir, +const char *refname,struct dir_iterator *iter); static int files_fsck_refs_name(struct ref_store *ref_store UNUSED,struct fsck_options *o, -const char *refs_check_dir, +const char *refname,struct dir_iterator *iter) {struct strbuf sb =STRBUF_INIT ;
@@ -3522,11 +3522,10 @@ static int files_fsck_refs_name(struct ref_store *ref_store UNUSED,/* * This works right now because we never check the root refs. */ -strbuf_addf (&sb,"%s/%s" , refs_check_dir, iter->relative_path); -if (check_refname_format (sb.buf,0 )) { +if (check_refname_format (refname,0 )) {struct fsck_ref_report report = {0 }; - report.path = sb.buf; + report.path = refname; ret =fsck_report_ref (o, &report,FSCK_MSG_BAD_REF_NAME ,"invalid refname format" );
@@ -3542,6 +3541,7 @@ static int files_fsck_refs_dir(struct ref_store *ref_store,const char *refs_check_dir, files_fsck_refs_fn *fsck_refs_fn) { + struct strbuf refname = STRBUF_INIT;struct strbuf sb =STRBUF_INIT ;struct dir_iterator *iter;int iter_status;
@@ -3560,11 +3560,15 @@ static int files_fsck_refs_dir(struct ref_store *ref_store,continue ; }else if (S_ISREG (iter->st.st_mode) ||S_ISLNK (iter->st.st_mode)) { +strbuf_reset (&refname); +strbuf_addf (&refname,"%s/%s" , refs_check_dir, + iter->relative_path); +if (o->verbose) -fprintf_ln (stderr,"Checking %s/%s" , - refs_check_dir, iter->relative_path); +fprintf_ln (stderr,"Checking %s" , refname.buf); +for (size_t i =0 ; fsck_refs_fn[i]; i++) { -if (fsck_refs_fn[i](ref_store, o, refs_check_dir, iter)) +if (fsck_refs_fn[i](ref_store, o, refname.buf, iter)) ret =-1 ; } }else {
@@ -3581,6 +3585,7 @@ static int files_fsck_refs_dir(struct ref_store *ref_store, out:strbuf_release (&sb); +strbuf_release (&refname);return ret; }
Signature · 1 line
2.47.0
PATCH v9 4/9 ref: support multiple worktrees check for refs
In reply to shejialuoWe have already set up the infrastructure to check the consistency for refs, but we do not support multiple worktrees. However, "git-fsck(1)" will check the refs of worktrees. As we decide to get feature parity with "git-fsck(1)", we need to set up support for multiple worktrees.
Because each worktree has its own specific refs, instead of just showing the users "refs/worktree/foo", we need to display the full name such as "worktrees/<id>/refs/worktree/foo". So we should know the id of the worktree to get the full name. Add a new parameter "struct worktree *" for "refs-internal.h::fsck_fn". Then change the related functions to follow this new interface.
The "packed-refs" only exists in the main worktree, so we should only check "packed-refs" in the main worktree. Use "is_main_worktree" method to skip checking "packed-refs" in "packed_fsck" function.
Then, enhance the "files-backend.c::files_fsck_refs_dir" function to add "worktree/<id>/" prefix when we are not in the main worktree.
Last, add a new test to check the refname when there are multiple worktrees to exercise the code.
--- builtin/refs.c | 10 ++++++-- refs.c | 5 ++-- refs.h | 3 ++- refs/debug.c | 5 ++-- refs/files-backend.c | 17 ++++++++++---- refs/packed-backend.c | 8 ++++++- refs/refs-internal.h | 3 ++- refs/reftable-backend.c | 3 ++- t/t0602-reffiles-fsck.sh | 51 ++++++++++++++++++++++++++++++++++++++++ 9 files changed, 90 insertions(+), 15 deletions(-)
builtin/refs.c +8 -2
@@ -5,6 +5,7 @@#include "parse-options.h" #include "refs.h" #include "strbuf.h" +#include "worktree.h" #define REFS_MIGRATE_USAGE \ N_("git refs migrate --ref-format=<format> [--dry-run]")
@@ -66,6 +67,7 @@ static int cmd_refs_migrate(int argc, const char **argv, const char *prefix)static int cmd_refs_verify (int argc,const char **argv,const char *prefix) {struct fsck_options fsck_refs_options =FSCK_REFS_OPTIONS_DEFAULT ; +struct worktree **worktrees;const char *const verify_usage[] = {REFS_VERIFY_USAGE ,NULL ,
@@ -75,7 +77,7 @@ static int cmd_refs_verify(int argc, const char **argv, const char *prefix)OPT_BOOL (0 ,"strict" , &fsck_refs_options.strict,N_ ("enable strict checking" )), OPT_END(), }; -int ret; +int ret =0 ; argc =parse_options (argc, argv, prefix, options, verify_usage,0 );if (argc)
@@ -84,9 +86,13 @@ static int cmd_refs_verify(int argc, const char **argv, const char *prefix)git_config (git_fsck_config, &fsck_refs_options);prepare_repo_settings (the_repository); - ret =refs_fsck (get_main_ref_store (the_repository), &fsck_refs_options); + worktrees =get_worktrees (); +for (size_t i =0 ; worktrees[i]; i++) + ret |=refs_fsck (get_worktree_ref_store (worktrees[i]), + &fsck_refs_options, worktrees[i]);fsck_options_clear (&fsck_refs_options); +free_worktrees (worktrees);return ret; }
refs.c +3 -2
@@ -318,9 +318,10 @@ int check_refname_format(const char *refname, int flags)return check_or_sanitize_refname (refname, flags,NULL ); } -int refs_fsck (struct ref_store *refs,struct fsck_options *o) +int refs_fsck (struct ref_store *refs,struct fsck_options *o, +struct worktree *wt) { -return refs->be->fsck (refs, o); +return refs->be->fsck (refs, o, wt); }void sanitize_refname_component (const char *refname,struct strbuf *out)
refs.h +2 -1
@@ -549,7 +549,8 @@ int check_refname_format(const char *refname, int flags);* reflogs are consistent, and non-zero otherwise. The errors will be * written to stderr. */ -int refs_fsck (struct ref_store *refs,struct fsck_options *o); +int refs_fsck (struct ref_store *refs,struct fsck_options *o, +struct worktree *wt);/* * Apply the rules from check_refname_format, but mutate the result until it
refs/debug.c +3 -2
@@ -420,10 +420,11 @@ static int debug_reflog_expire(struct ref_store *ref_store, const char *refname, }static int debug_fsck (struct ref_store *ref_store, -struct fsck_options *o) +struct fsck_options *o, +struct worktree *wt) {struct debug_ref_store *drefs = (struct debug_ref_store *)ref_store; -int res = drefs->refs->be->fsck (drefs->refs, o); +int res = drefs->refs->be->fsck (drefs->refs, o, wt);trace_printf_key (&trace_refs,"fsck: %d\n" , res);return res; }
refs/files-backend.c +12 -5
@@ -23,6 +23,7 @@#include "../dir.h" #include "../chdir-notify.h" #include "../setup.h" +#include "../worktree.h" #include "../wrapper.h" #include "../write-or-die.h" #include "../revision.h"
@@ -3539,6 +3540,7 @@ static int files_fsck_refs_name(struct ref_store *ref_store UNUSED,static int files_fsck_refs_dir (struct ref_store *ref_store,struct fsck_options *o,const char *refs_check_dir, +struct worktree *wt,files_fsck_refs_fn *fsck_refs_fn) {struct strbuf refname =STRBUF_INIT ;
@@ -3561,6 +3563,9 @@ static int files_fsck_refs_dir(struct ref_store *ref_store, }else if (S_ISREG (iter->st.st_mode) ||S_ISLNK (iter->st.st_mode)) {strbuf_reset (&refname); + +if (!is_main_worktree (wt)) +strbuf_addf (&refname,"worktrees/%s/" , wt->id);strbuf_addf (&refname,"%s/%s" , refs_check_dir, iter->relative_path);
@@ -3590,7 +3595,8 @@ static int files_fsck_refs_dir(struct ref_store *ref_store, }static int files_fsck_refs (struct ref_store *ref_store, -struct fsck_options *o) +struct fsck_options *o, +struct worktree *wt) {files_fsck_refs_fn fsck_refs_fn[]= { files_fsck_refs_name,
@@ -3599,17 +3605,18 @@ static int files_fsck_refs(struct ref_store *ref_store,if (o->verbose)fprintf_ln (stderr,_ ("Checking references consistency" )); -return files_fsck_refs_dir (ref_store, o,"refs" , fsck_refs_fn); +return files_fsck_refs_dir (ref_store, o,"refs" , wt, fsck_refs_fn); }static int files_fsck (struct ref_store *ref_store, -struct fsck_options *o) +struct fsck_options *o, +struct worktree *wt) {struct files_ref_store *refs =files_downcast (ref_store,REF_STORE_READ ,"fsck" ); -return files_fsck_refs (ref_store, o) | - refs->packed_ref_store->be->fsck (refs->packed_ref_store, o); +return files_fsck_refs (ref_store, o, wt) | + refs->packed_ref_store->be->fsck (refs->packed_ref_store, o, wt); }struct ref_storage_be refs_be_files = {
refs/packed-backend.c +7 -1
@@ -13,6 +13,7 @@#include "../lockfile.h" #include "../chdir-notify.h" #include "../statinfo.h" +#include "../worktree.h" #include "../wrapper.h" #include "../write-or-die.h" #include "../trace2.h"
@@ -1754,8 +1755,13 @@ static struct ref_iterator *packed_reflog_iterator_begin(struct ref_store *ref_s } static int packed_fsck(struct ref_store *ref_store UNUSED, - struct fsck_options *o UNUSED) + struct fsck_options *o UNUSED, +struct worktree *wt) { + +if (!is_main_worktree (wt)) +return 0 ; +return 0 ; }
refs/refs-internal.h +2 -1
@@ -653,7 +653,8 @@ typedef int read_symbolic_ref_fn(struct ref_store *ref_store, const char *refnam struct strbuf *referent);typedef int fsck_fn (struct ref_store *ref_store, -struct fsck_options *o); +struct fsck_options *o, +struct worktree *wt);struct ref_storage_be {const char *name;
refs/reftable-backend.c +2 -1
@@ -2443,7 +2443,8 @@ static int reftable_be_reflog_expire(struct ref_store *ref_store, } static int reftable_be_fsck(struct ref_store *ref_store UNUSED, - struct fsck_options *o UNUSED) + struct fsck_options *o UNUSED, + struct worktree *wt UNUSED) {return 0 ; }
t/t0602-reffiles-fsck.sh +51 -0
55 lines not on this page · raw message
Signature · 1 line
2.47.0
PATCH v9 5/9 ref: port git-fsck(1) regular refs check for files backend
In reply to shejialuo"git-fsck(1)" implicitly checks the ref content by passing the callback "fsck_handle_ref" to the "refs.c::refs_for_each_rawref". Then, it will check whether the ref content (eventually "oid") is valid. If not, it will report the following error to the user.
error: refs/heads/main: invalid sha1 pointer 0000...
And it will also report above errors when there are dangling symrefs in the repository wrongly. This does not align with the behavior of the "git symbolic-ref" command which allows users to create dangling symrefs.
As we have already introduced the "git refs verify" command, we'd better check the ref content explicitly in the "git refs verify" command thus later we could remove these checks in "git-fsck(1)" and launch a subprocess to call "git refs verify" in "git-fsck(1)" to make the "git-fsck(1)" more clean.
Following what "git-fsck(1)" does, 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.
--- Documentation/fsck-msgids.txt | 3 + fsck.h | 1 + refs/files-backend.c | 47 +++++++++++++++ t/t0602-reffiles-fsck.sh | 105 ++++++++++++++++++++++++++++++++++ 4 files changed, 156 insertions(+)
Documentation/fsck-msgids.txt +3 -0
@@ -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.
fsck.h +1 -0
@@ -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 ) \
refs/files-backend.c +47 -0
59 lines not on this page · raw message
t/t0602-reffiles-fsck.sh +105 -0
109 lines not on this page · raw message
Signature · 1 line
2.47.0
PATCH v9 6/9 ref: add more strict checks for regular refs
In reply to shejialuoWe have already used "parse_loose_ref_contents" function to check whether the ref content is valid in files backend. However, by using "parse_loose_ref_contents", we allow the ref's content to end with garbage or without a newline.
Even though we never create such loose refs ourselves, we have accepted such loose refs. So, it is entirely possible that some third-party tools may rely on such loose refs being valid. We should not report an error fsck message at current. We should notify the users about such "curiously formatted" loose refs so that adequate care is taken before we decide to tighten the rules in the future.
And it's not suitable either to report a warn fsck message to the user. We don't yet want the "--strict" flag that controls this bit to end up generating errors for such weirdly-formatted reference contents, as we first want to assess whether this retroactive tightening will cause issues for any tools out there. It may cause compatibility issues which may break the repository. So, we add the following two fsck infos to represent the situation where the ref content ends without newline or has trailing garbages:
1. refMissingNewline(INFO): A loose ref that does not end with newline(LF).
2. trailingRefContent(INFO): A loose ref has trailing content.
It might appear that we can't provide the user with any warnings by using FSCK_INFO. However, in "fsck.c::fsck_vreport", we will convert FSCK_INFO to FSCK_WARN and we can still warn the user about these situations when using "git refs verify" without introducing compatibility issues.
--- Documentation/fsck-msgids.txt | 14 +++++++++ fsck.h | 2 ++ refs.c | 2 +- refs/files-backend.c | 26 ++++++++++++++-- refs/refs-internal.h | 2 +- t/t0602-reffiles-fsck.sh | 57 +++++++++++++++++++++++++++++++++-- 6 files changed, 96 insertions(+), 7 deletions(-)
Documentation/fsck-msgids.txt +14 -0
@@ -173,6 +173,20 @@ `nullSha1`:: (WARN) Tree contains entries pointing to a null sha1. +`refMissingNewline`:: + (INFO) A loose ref that does not end with newline(LF). As + valid implementations of Git never created such a loose ref + file, it may become an error in the future. Report to the + [email protected] mailing list if you see this error, as + we need to know what tools created such a file. + +`trailingRefContent`:: + (INFO) A loose ref has trailing content. As valid implementations + of Git never created such a loose ref file, it may become an + error in the future. Report to the [email protected] mailing + list if you see this error, as we need to know what tools + created such a file. + `treeNotSorted`:: (ERROR) A tree is not properly sorted.
fsck.h +2 -0
@@ -85,6 +85,8 @@ enum fsck_msg_type { FUNC(MAILMAP_SYMLINK, INFO) \ FUNC(BAD_TAG_NAME, INFO) \ FUNC(MISSING_TAGGER_ENTRY, INFO) \ + FUNC(REF_MISSING_NEWLINE, INFO) \ +FUNC (TRAILING_REF_CONTENT ,INFO ) \ /* ignored (elevated when requested) */ \FUNC (EXTRA_HEADER_ENTRY ,IGNORE )
refs.c +1 -1
@@ -1789,7 +1789,7 @@ static int refs_read_special_head(struct ref_store *ref_store, } result =parse_loose_ref_contents (ref_store->repo->hash_algo, content.buf, - oid, referent, type, failure_errno); + oid, referent, type,NULL , failure_errno); done:strbuf_release (&full_path);
refs/files-backend.c +23 -3
@@ -569,7 +569,7 @@ static int read_ref_internal(struct ref_store *ref_store, const char *refname, buf = sb_contents.buf; ret =parse_loose_ref_contents (ref_store->repo->hash_algo, buf, - oid, referent, type, &myerr); + oid, referent, type,NULL , &myerr); out:if (ret && !myerr)
@@ -606,7 +606,7 @@ static int files_read_symbolic_ref(struct ref_store *ref_store, const char *refnint parse_loose_ref_contents (const struct git_hash_algo *algop,const char *buf,struct object_id *oid,struct strbuf *referent,unsigned int *type, -int *failure_errno) +const char **trailing,int *failure_errno) {const char *p;if (skip_prefix (buf,"ref:" , &buf)) {
@@ -628,6 +628,10 @@ int parse_loose_ref_contents(const struct git_hash_algo *algop, *failure_errno =EINVAL ;return -1 ; } + +if (trailing) + *trailing = p; +return 0 ; }
@@ -3513,6 +3517,7 @@ static int files_fsck_refs_content(struct ref_store *ref_store,struct strbuf ref_content =STRBUF_INIT ;struct strbuf referent =STRBUF_INIT ;struct fsck_ref_report report = {0 }; +const char *trailing =NULL ;unsigned int type =0 ;int failure_errno =0 ;struct object_id oid;
@@ -3537,7 +3542,7 @@ static int files_fsck_refs_content(struct ref_store *ref_store,if (parse_loose_ref_contents (ref_store->repo->hash_algo, ref_content.buf, &oid, &referent, - &type, &failure_errno)) { + &type, &trailing, &failure_errno)) {strbuf_rtrim (&ref_content); ret = fsck_report_ref(o, &report, FSCK_MSG_BAD_REF_CONTENT,
@@ -3545,6 +3550,21 @@ static int files_fsck_refs_content(struct ref_store *ref_store, goto cleanup; } +if (!(type &REF_ISSYMREF )) { +if (!*trailing) { + ret =fsck_report_ref (o, &report, +FSCK_MSG_REF_MISSING_NEWLINE , +"misses LF at the end" ); + goto cleanup; + } +if (*trailing !='\n' || *(trailing +1 )) { + ret =fsck_report_ref (o, &report, +FSCK_MSG_TRAILING_REF_CONTENT , +"has trailing garbage: '%s'" , trailing); + goto cleanup; + } + } + cleanup:strbuf_release (&ref_content);strbuf_release (&referent);
refs/refs-internal.h +1 -1
@@ -716,7 +716,7 @@ struct ref_store {int parse_loose_ref_contents (const struct git_hash_algo *algop,const char *buf,struct object_id *oid,struct strbuf *referent,unsigned int *type, -int *failure_errno); +const char **trailing,int *failure_errno);/* * Fill in the generic part of refs and add it to our collection of
t/t0602-reffiles-fsck.sh +55 -2
81 lines not on this page · raw message
Signature · 1 line
2.47.0
PATCH v9 7/9 ref: add basic symref content check for files backend
In reply to shejialuoWe have code that checks regular ref contents, but we do not yet check the contents of symbolic refs. By using "parse_loose_ref_content" for symbolic refs, we will get the information of the "referent".
We do not need to check the "referent" by opening the file. This is because if "referent" exists in the file system, we will eventually check its correctness by inspecting every file in the "refs" directory. If the "referent" does not exist in the filesystem, this is OK as it is seen as the dangling symref.
So we just need to check the "referent" string content. A regular ref could be accepted as a textual symref if it begins with "ref:", followed by zero or more whitespaces, followed by the full refname, followed only by whitespace characters. However, we always write a single SP after "ref:" and a single LF after the refname. It may seem that we should report a fsck error message when the "referent" does not apply above rules and we should not be so aggressive because third-party reimplementations of Git may have taken advantage of the looser syntax. Put it more specific, we accept the following contents:
1. "ref: refs/heads/master "
2. "ref: refs/heads/master \n \n"
3. "ref: refs/heads/master\n\n"
When introducing the regular ref content checks, we created two fsck infos "refMissingNewline" and "trailingRefContent" which exactly represents above situations. So we will reuse these two fsck messages to write checks to info the user about these situations.
But we do not allow any other trailing garbage. The followings are bad symref contents which will be reported as fsck error by "git-fsck(1)".
1. "ref: refs/heads/master garbage\n"
2. "ref: refs/heads/master \n\n\n garbage "
And we introduce a new "badReferentName(ERROR)" fsck message to report above errors by using "is_root_ref" and "check_refname_format" to check the "referent". Since both "is_root_ref" and "check_refname_format" don't work with whitespaces, we use the trimmed version of "referent" with these functions.
In order to add checks, we will do the following things:
1. Record the untrimmed length "orig_len" and untrimmed last byte "orig_last_byte".
2. Use "strbuf_rtrim" to trim the whitespaces or newlines to make sure "is_root_ref" and "check_refname_format" won't be failed by them.
3. Use "orig_len" and "orig_last_byte" to check whether the "referent" misses '\n' at the end or it has trailing whitespaces or newlines.
--- Documentation/fsck-msgids.txt | 3 + fsck.h | 1 + refs/files-backend.c | 40 ++++++++++++ t/t0602-reffiles-fsck.sh | 111 ++++++++++++++++++++++++++++++++++ 4 files changed, 155 insertions(+)
Documentation/fsck-msgids.txt +3 -0
@@ -28,6 +28,9 @@ `badRefName`:: (ERROR) A ref has an invalid format. +`badReferentName`:: + (ERROR) The referent name of a symref is invalid. + `badTagName`:: (INFO) A tag has an invalid format.
fsck.h +1 -0
@@ -34,6 +34,7 @@ enum fsck_msg_type { FUNC(BAD_REF_CONTENT, ERROR) \ FUNC(BAD_REF_FILETYPE, ERROR) \ FUNC(BAD_REF_NAME, ERROR) \ + FUNC(BAD_REFERENT_NAME, ERROR) \ FUNC(BAD_TIMEZONE, ERROR) \ FUNC(BAD_TREE, ERROR) \FUNC (BAD_TREE_SHA1 ,ERROR ) \
refs/files-backend.c +40 -0
@@ -3509,6 +3509,43 @@ typedef int (*files_fsck_refs_fn)(struct ref_store *ref_store,const char *refname, struct dir_iterator *iter); +static int files_fsck_symref_target (struct fsck_options *o, +struct fsck_ref_report *report, +struct strbuf *referent) +{ +char orig_last_byte; +size_t orig_len; +int ret =0 ; + + orig_len = referent->len; + orig_last_byte = referent->buf[orig_len -1 ]; +strbuf_rtrim (referent); + +if (!is_root_ref (referent->buf) && +check_refname_format (referent->buf,0 )) { + ret =fsck_report_ref (o, report, +FSCK_MSG_BAD_REFERENT_NAME , +"points to invalid refname '%s'" , referent->buf); + goto out; + } + +if (referent->len == orig_len || + (referent->len < orig_len && orig_last_byte !='\n' )) { + ret =fsck_report_ref (o, report, +FSCK_MSG_REF_MISSING_NEWLINE , +"misses LF at the end" ); + } + +if (referent->len != orig_len && referent->len != orig_len -1 ) { + ret =fsck_report_ref (o, report, +FSCK_MSG_TRAILING_REF_CONTENT , +"has trailing whitespaces or newlines" ); + } + +out: +return ret; +} + static int files_fsck_refs_content(struct ref_store *ref_store, struct fsck_options *o, const char *target_name,
@@ -3563,6 +3600,9 @@ static int files_fsck_refs_content(struct ref_store *ref_store, "has trailing garbage: '%s'", trailing); goto cleanup; } + }else { + ret =files_fsck_symref_target (o, &report, &referent); + goto cleanup; } cleanup:
t/t0602-reffiles-fsck.sh +111 -0
123 lines not on this page · raw message
Signature · 1 line
2.47.0
PATCH v9 8/9 ref: check whether the target of the symref is a ref
In reply to shejialuoIdeally, we want to the users use "git symbolic-ref" to create symrefs instead of writing raw contents into the filesystem. However, "git symbolic-ref" is strict with the refname but not strict with the referent. For example, we can make the "referent" located at the "$(gitdir)/logs/aaa" and manually write the content into this where we can still successfully parse this symref by using "git rev-parse".
$ git init repo && cd repo && git commit --allow-empty -mx $ git symbolic-ref refs/heads/test logs/aaa $ echo $(git rev-parse HEAD) > .git/logs/aaa $ git rev-parse test
We may need to add some restrictions for "referent" parameter when using "git symbolic-ref" to create symrefs because ideally all the nonpseudo-refs should be located under the "refs" directory and we may tighten this in the future.
In order to tell the user we may tighten the above situation, create a new fsck message "symrefTargetIsNotARef" to notify the user that this may become an error in the future.
--- Documentation/fsck-msgids.txt | 9 +++++++++ fsck.h | 1 + refs/files-backend.c | 14 ++++++++++++-- t/t0602-reffiles-fsck.sh | 29 +++++++++++++++++++++++++++++ 4 files changed, 51 insertions(+), 2 deletions(-)
Documentation/fsck-msgids.txt +9 -0
@@ -183,6 +183,15 @@ [email protected] mailing list if you see this error, as we need to know what tools created such a file. +`symrefTargetIsNotARef`:: + (INFO) The target of a symbolic reference points neither to + a root reference nor to a reference starting with "refs/". + Although we allow create a symref pointing to the referent which + is outside the "ref" by using `git symbolic-ref`, we may tighten + the rule in the future. Report to the [email protected] + mailing list if you see this error, as we need to know what tools + created such a file. + `trailingRefContent`:: (INFO) A loose ref has trailing content. As valid implementations of Git never created such a loose ref file, it may become an
fsck.h +1 -0
@@ -87,6 +87,7 @@ enum fsck_msg_type { FUNC(BAD_TAG_NAME, INFO) \ FUNC(MISSING_TAGGER_ENTRY, INFO) \ FUNC(REF_MISSING_NEWLINE, INFO) \ + FUNC(SYMREF_TARGET_IS_NOT_A_REF, INFO) \FUNC (TRAILING_REF_CONTENT ,INFO ) \ /* ignored (elevated when requested) */ \FUNC (EXTRA_HEADER_ENTRY ,IGNORE )
refs/files-backend.c +12 -2
@@ -3513,6 +3513,7 @@ static int files_fsck_symref_target(struct fsck_options *o,struct fsck_ref_report *report, struct strbuf *referent) { + int is_referent_root;char orig_last_byte;size_t orig_len;int ret =0 ;
@@ -3521,8 +3522,17 @@ static int files_fsck_symref_target(struct fsck_options *o, orig_last_byte = referent->buf[orig_len -1 ];strbuf_rtrim (referent); -if (!is_root_ref (referent->buf) && -check_refname_format (referent->buf,0 )) { + is_referent_root =is_root_ref (referent->buf); +if (!is_referent_root && + !starts_with (referent->buf,"refs/" ) && + !starts_with (referent->buf,"worktrees/" )) { + ret =fsck_report_ref (o, report, +FSCK_MSG_SYMREF_TARGET_IS_NOT_A_REF , +"points to non-ref target '%s'" , referent->buf); + + } + +if (!is_referent_root &&check_refname_format (referent->buf,0 )) { ret =fsck_report_ref (o, report,FSCK_MSG_BAD_REFERENT_NAME ,"points to invalid refname '%s'" , referent->buf);
t/t0602-reffiles-fsck.sh +29 -0
@@ -366,6 +366,35 @@ test_expect_success 'textual symref content should be checked (aggregate)' ' test_cmp expect sorted_err ' +test_expect_success 'the target of the textual symref should be checked' ' + 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" && + + for good_referent in "refs/heads/branch" "HEAD" "refs/tags/tag" + do + printf "ref: %s\n" $good_referent >$branch_dir_prefix/branch-good && + git refs verify 2>err && + rm $branch_dir_prefix/branch-good && + test_must_be_empty err || return 1 + done && + + for nonref_referent in "refs-back/heads/branch" "refs-back/tags/tag" "reflogs/refs/heads/branch" + do + printf "ref: %s\n" $nonref_referent >$branch_dir_prefix/branch-bad-1 && + git refs verify 2>err && + cat >expect <<-EOF && + warning: refs/heads/branch-bad-1: symrefTargetIsNotARef: points to non-ref target '\''$nonref_referent'\'' + EOF + rm $branch_dir_prefix/branch-bad-1 && + test_cmp expect err || return 1 + done +' + test_expect_success 'ref content checks should work with worktrees' ' test_when_finished "rm -rf repo" && git init repo &&
Signature · 1 line
2.47.0
PATCH v9 9/9 ref: add symlink ref content check for files backend
In reply to shejialuoBesides the textual symref, we also allow symbolic links as the symref. So, we should also provide the consistency check as what we have done for textual symref. And also we consider deprecating writing the symbolic links. We first need to access whether symbolic links still be used. So, add a new fsck message "symlinkRef(INFO)" to tell the user be aware of this information.
We have already introduced "files_fsck_symref_target". We should reuse this function to handle the symrefs which use legacy symbolic links. We should not check the trailing garbage for symbolic refs. Add a new parameter "symbolic_link" to disable some checks which should only be executed for textual symrefs.
And we need to also generate the "referent" parameter for reusing "files_fsck_symref_target" by the following steps:
1. Use "strbuf_add_real_path" to resolve the symlink and get the absolute path "ref_content" which the symlink ref points to.
2. Generate the absolute path "abs_gitdir" of "gitdir" and combine "ref_content" and "abs_gitdir" to extract the relative path "relative_referent_path".
3. If "ref_content" is outside of "gitdir", we just set "referent" with "ref_content". Instead, we set "referent" with "relative_referent_path".
--- Documentation/fsck-msgids.txt | 6 ++ fsck.h | 1 + refs/files-backend.c | 38 ++++++++- t/t0602-reffiles-fsck.sh | 141 ++++++++++++++++++++++++++++++++++ 4 files changed, 182 insertions(+), 4 deletions(-)
Documentation/fsck-msgids.txt +6 -0
@@ -183,6 +183,12 @@ [email protected] mailing list if you see this error, as we need to know what tools created such a file. +`symlinkRef`:: + (INFO) A symbolic link is used as a symref. Report to the + [email protected] mailing list if you see this error, as we + are assessing the feasibility of dropping the support to drop + creating symbolic links as symrefs. + `symrefTargetIsNotARef`:: (INFO) The target of a symbolic reference points neither to a root reference nor to a reference starting with "refs/".
fsck.h +1 -0
@@ -86,6 +86,7 @@ enum fsck_msg_type { FUNC(MAILMAP_SYMLINK, INFO) \ FUNC(BAD_TAG_NAME, INFO) \ FUNC(MISSING_TAGGER_ENTRY, INFO) \ + FUNC(SYMLINK_REF, INFO) \ FUNC(REF_MISSING_NEWLINE, INFO) \ FUNC(SYMREF_TARGET_IS_NOT_A_REF, INFO) \FUNC (TRAILING_REF_CONTENT ,INFO ) \
refs/files-backend.c +34 -4
@@ -1,6 +1,7 @@#define USE_THE_REPOSITORY_VARIABLE #include "../git-compat-util.h" +#include "../abspath.h" #include "../config.h" #include "../copy.h" #include "../environment.h"
@@ -3511,7 +3512,8 @@ typedef int (*files_fsck_refs_fn)(struct ref_store *ref_store,static int files_fsck_symref_target (struct fsck_options *o,struct fsck_ref_report *report, -struct strbuf *referent) +struct strbuf *referent, +unsigned int symbolic_link) {int is_referent_root;char orig_last_byte;
@@ -3520,7 +3522,8 @@ static int files_fsck_symref_target(struct fsck_options *o, orig_len = referent->len; orig_last_byte = referent->buf[orig_len -1 ]; -strbuf_rtrim (referent); +if (!symbolic_link) +strbuf_rtrim (referent); is_referent_root =is_root_ref (referent->buf); if (!is_referent_root &&
@@ -3539,6 +3542,9 @@ static int files_fsck_symref_target(struct fsck_options *o, goto out; } +if (symbolic_link) + goto out; +if (referent->len == orig_len || (referent->len < orig_len && orig_last_byte !='\n' )) { ret = fsck_report_ref(o, report,
@@ -3562,6 +3568,7 @@ static int files_fsck_refs_content(struct ref_store *ref_store, struct dir_iterator *iter) {struct strbuf ref_content =STRBUF_INIT ; +struct strbuf abs_gitdir =STRBUF_INIT ;struct strbuf referent =STRBUF_INIT ;struct fsck_ref_report report = {0 };const char *trailing =NULL ;
@@ -3572,8 +3579,30 @@ static int files_fsck_refs_content(struct ref_store *ref_store, report.path = target_name; -if (S_ISLNK (iter->st.st_mode)) +if (S_ISLNK (iter->st.st_mode)) { +const char *relative_referent_path =NULL ; + + ret =fsck_report_ref (o, &report, +FSCK_MSG_SYMLINK_REF , +"use deprecated symbolic link for symref" ); + +strbuf_add_absolute_path (&abs_gitdir, ref_store->repo->gitdir); +strbuf_normalize_path (&abs_gitdir); +if (!is_dir_sep (abs_gitdir.buf[abs_gitdir.len -1 ])) +strbuf_addch (&abs_gitdir,'/' ); + +strbuf_add_real_path (&ref_content, iter->path.buf); +skip_prefix (ref_content.buf, abs_gitdir.buf, + &relative_referent_path); + +if (relative_referent_path) +strbuf_addstr (&referent, relative_referent_path); +else +strbuf_addbuf (&referent, &ref_content); + + ret |=files_fsck_symref_target (o, &report, &referent,1 ); goto cleanup; + }if (strbuf_read_file (&ref_content, iter->path.buf,0 ) <0 ) {/*
@@ -3611,13 +3640,14 @@ static int files_fsck_refs_content(struct ref_store *ref_store, goto cleanup; } }else { - ret =files_fsck_symref_target (o, &report, &referent); + ret =files_fsck_symref_target (o, &report, &referent,0 ); goto cleanup; } cleanup:strbuf_release (&ref_content);strbuf_release (&referent); +strbuf_release (&abs_gitdir);return ret; }
t/t0602-reffiles-fsck.sh +141 -0
147 lines not on this page · raw message
Signature · 1 line
2.47.0
Re: [PATCH v9 0/9] add ref content check for files backend
In reply to shejialuoQuoted 9 lines · shejialuo
Hi All:
This version fixes two problems:
1. Remove unnecessary space.
2. Drop extra "strerror(errno)".Thanks,
Jialuo
The range-diff looks as expected, so this version lokos good to me.
Thanks!
Patrick
Re: [PATCH v9 0/9] add ref content check for files backend
Quoted 5 lines · Patrick Steinhardt
Quoted 4 lines · shejialuo
Hi All:
This version fixes two problems:
Quoted 7 lines
Quoted 5 lines
1. Remove unnecessary space.
2. Drop extra "strerror(errno)".Thanks,
JialuoThe range-diff looks as expected, so this version lokos good to me.
Thanks, both. Looking good.