ref: add more strict checks for regular refs
We 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. Mentored-by: Patrick Steinhardt <ps@pks.im> Mentored-by: Karthik Nayak <karthik.188@gmail.com> Signed-off-by: shejialuo <shejialuo@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
shejialuo committed
Nov 20, 2024 at 19:51 UTC
1c0e2a0019c29c6e046634b4b8324df4570ef9d1
6 files changed
+96
-7
Documentation/fsck-msgids.txt
+14
@@ -173,6 +173,20 @@
173
`nullSha1`::
174
(WARN) Tree contains entries pointing to a null sha1.
175
176
+`refMissingNewline`::
177
+ (INFO) A loose ref that does not end with newline(LF). As
178
+ valid implementations of Git never created such a loose ref
179
+ file, it may become an error in the future. Report to the
180
+ git@vger.kernel.org mailing list if you see this error, as
181
+ we need to know what tools created such a file.
182
+
183
+`trailingRefContent`::
184
+ (INFO) A loose ref has trailing content. As valid implementations
185
+ of Git never created such a loose ref file, it may become an
186
+ error in the future. Report to the git@vger.kernel.org mailing
187
+ list if you see this error, as we need to know what tools
188
+ created such a file.
189
+
190
`treeNotSorted`::
191
(ERROR) A tree is not properly sorted.
192
fsck.h
+2
@@ -85,6 +85,8 @@ enum fsck_msg_type {
85
FUNC(MAILMAP_SYMLINK, INFO) \
86
FUNC(BAD_TAG_NAME, INFO) \
87
FUNC(MISSING_TAGGER_ENTRY, INFO) \
88
+ FUNC(REF_MISSING_NEWLINE, INFO) \
89
+ FUNC(TRAILING_REF_CONTENT, INFO) \
90
/* ignored (elevated when requested) */ \
91
FUNC(EXTRA_HEADER_ENTRY, IGNORE)
92
refs.c
+1
-1
@@ -1789,7 +1789,7 @@ static int refs_read_special_head(struct ref_store *ref_store,
1789
}
1790
1791
result = parse_loose_ref_contents(ref_store->repo->hash_algo, content.buf,
1792
- oid, referent, type, failure_errno);
1792
+ oid, referent, type, NULL, failure_errno);
1793
1794
done:
1795
strbuf_release(&full_path);
refs/files-backend.c
+23
-3
@@ -569,7 +569,7 @@ stat_ref:
569
buf = sb_contents.buf;
570
571
ret = parse_loose_ref_contents(ref_store->repo->hash_algo, buf,
572
- oid, referent, type, &myerr);
572
+ oid, referent, type, NULL, &myerr);
573
574
out:
575
if (ret && !myerr)
@@ -606,7 +606,7 @@ static int files_read_symbolic_ref(struct ref_store *ref_store, const char *refn
606
int parse_loose_ref_contents(const struct git_hash_algo *algop,
607
const char *buf, struct object_id *oid,
608
struct strbuf *referent, unsigned int *type,
609
- int *failure_errno)
609
+ const char **trailing, int *failure_errno)
610
{
611
const char *p;
612
if (skip_prefix(buf, "ref:", &buf)) {
@@ -628,6 +628,10 @@ int parse_loose_ref_contents(const struct git_hash_algo *algop,
628
*failure_errno = EINVAL;
629
return -1;
630
}
631
+
632
+ if (trailing)
633
+ *trailing = p;
634
+
635
return 0;
636
}
637
@@ -3513,6 +3517,7 @@ static int files_fsck_refs_content(struct ref_store *ref_store,
3517
struct strbuf ref_content = STRBUF_INIT;
3518
struct strbuf referent = STRBUF_INIT;
3519
struct fsck_ref_report report = { 0 };
3520
+ const char *trailing = NULL;
3521
unsigned int type = 0;
3522
int failure_errno = 0;
3523
struct object_id oid;
@@ -3537,7 +3542,7 @@ static int files_fsck_refs_content(struct ref_store *ref_store,
3542
3543
if (parse_loose_ref_contents(ref_store->repo->hash_algo,
3544
ref_content.buf, &oid, &referent,
3540
- &type, &failure_errno)) {
3545
+ &type, &trailing, &failure_errno)) {
3546
strbuf_rtrim(&ref_content);
3547
ret = fsck_report_ref(o, &report,
3548
FSCK_MSG_BAD_REF_CONTENT,
@@ -3545,6 +3550,21 @@ static int files_fsck_refs_content(struct ref_store *ref_store,
3550
goto cleanup;
3551
}
3552
3553
+ if (!(type & REF_ISSYMREF)) {
3554
+ if (!*trailing) {
3555
+ ret = fsck_report_ref(o, &report,
3556
+ FSCK_MSG_REF_MISSING_NEWLINE,
3557
+ "misses LF at the end");
3558
+ goto cleanup;
3559
+ }
3560
+ if (*trailing != '\n' || *(trailing + 1)) {
3561
+ ret = fsck_report_ref(o, &report,
3562
+ FSCK_MSG_TRAILING_REF_CONTENT,
3563
+ "has trailing garbage: '%s'", trailing);
3564
+ goto cleanup;
3565
+ }
3566
+ }
3567
+
3568
cleanup:
3569
strbuf_release(&ref_content);
3570
strbuf_release(&referent);
refs/refs-internal.h
+1
-1
@@ -716,7 +716,7 @@ struct ref_store {
716
int parse_loose_ref_contents(const struct git_hash_algo *algop,
717
const char *buf, struct object_id *oid,
718
struct strbuf *referent, unsigned int *type,
719
- int *failure_errno);
719
+ const char **trailing, int *failure_errno);
720
721
/*
722
* Fill in the generic part of refs and add it to our collection of
t/t0602-reffiles-fsck.sh
+55
-2
@@ -189,7 +189,48 @@ test_expect_success 'regular ref content should be checked (individual)' '
189
EOF
190
rm $branch_dir_prefix/a/b/branch-bad &&
191
test_cmp expect err || return 1
192
- done
192
+ done &&
193
+
194
+ printf "%s" "$(git rev-parse main)" >$branch_dir_prefix/branch-no-newline &&
195
+ git refs verify 2>err &&
196
+ cat >expect <<-EOF &&
197
+ warning: refs/heads/branch-no-newline: refMissingNewline: misses LF at the end
198
+ EOF
199
+ rm $branch_dir_prefix/branch-no-newline &&
200
+ test_cmp expect err &&
201
+
202
+ for trailing_content in " garbage" " more garbage"
203
+ do
204
+ printf "%s" "$(git rev-parse main)$trailing_content" >$branch_dir_prefix/branch-garbage &&
205
+ git refs verify 2>err &&
206
+ cat >expect <<-EOF &&
207
+ warning: refs/heads/branch-garbage: trailingRefContent: has trailing garbage: '\''$trailing_content'\''
208
+ EOF
209
+ rm $branch_dir_prefix/branch-garbage &&
210
+ test_cmp expect err || return 1
211
+ done &&
212
+
213
+ printf "%s\n\n\n" "$(git rev-parse main)" >$branch_dir_prefix/branch-garbage-special &&
214
+ git refs verify 2>err &&
215
+ cat >expect <<-EOF &&
216
+ warning: refs/heads/branch-garbage-special: trailingRefContent: has trailing garbage: '\''
217
+
218
+
219
+ '\''
220
+ EOF
221
+ rm $branch_dir_prefix/branch-garbage-special &&
222
+ test_cmp expect err &&
223
+
224
+ printf "%s\n\n\n garbage" "$(git rev-parse main)" >$branch_dir_prefix/branch-garbage-special &&
225
+ git refs verify 2>err &&
226
+ cat >expect <<-EOF &&
227
+ warning: refs/heads/branch-garbage-special: trailingRefContent: has trailing garbage: '\''
228
+
229
+
230
+ garbage'\''
231
+ EOF
232
+ rm $branch_dir_prefix/branch-garbage-special &&
233
+ test_cmp expect err
234
'
235
236
test_expect_success 'regular ref content should be checked (aggregate)' '
@@ -207,12 +248,16 @@ test_expect_success 'regular ref content should be checked (aggregate)' '
248
printf "%s" $bad_content_1 >$tag_dir_prefix/tag-bad-1 &&
249
printf "%s" $bad_content_2 >$tag_dir_prefix/tag-bad-2 &&
250
printf "%s" $bad_content_3 >$branch_dir_prefix/a/b/branch-bad &&
251
+ printf "%s" "$(git rev-parse main)" >$branch_dir_prefix/branch-no-newline &&
252
+ printf "%s garbage" "$(git rev-parse main)" >$branch_dir_prefix/branch-garbage &&
253
254
test_must_fail git refs verify 2>err &&
255
cat >expect <<-EOF &&
256
error: refs/heads/a/b/branch-bad: badRefContent: $bad_content_3
257
error: refs/tags/tag-bad-1: badRefContent: $bad_content_1
258
error: refs/tags/tag-bad-2: badRefContent: $bad_content_2
259
+ warning: refs/heads/branch-garbage: trailingRefContent: has trailing garbage: '\'' garbage'\''
260
+ warning: refs/heads/branch-no-newline: refMissingNewline: misses LF at the end
261
EOF
262
sort err >sorted_err &&
263
test_cmp expect sorted_err
@@ -260,7 +305,15 @@ test_expect_success 'ref content checks should work with worktrees' '
305
EOF
306
rm $worktree2_refdir_prefix/bad-branch-2 &&
307
test_cmp expect err || return 1
263
- done
308
+ done &&
309
+
310
+ printf "%s" "$(git rev-parse HEAD)" >$worktree1_refdir_prefix/branch-no-newline &&
311
+ git refs verify 2>err &&
312
+ cat >expect <<-EOF &&
313
+ warning: worktrees/worktree-1/refs/worktree/branch-no-newline: refMissingNewline: misses LF at the end
314
+ EOF
315
+ rm $worktree1_refdir_prefix/branch-no-newline &&
316
+ test_cmp expect err
317
'
318
319
test_done