Re: [PATCH v5 6/9] ref: add escape check for the referent of symref
- From
Patrick Steinhardt <ps@pks.im>
- Date
- Oct 7, 2024, 06:58 UTC
- Message-ID
- <ZwOGr4Tv8K_wemtD@pks.im>
- In-Reply-To
- <Zvj-xaa_j26Auig7@ArchLinux>
On Sun, Sep 29, 2024 at 03:16:21PM +0800, shejialuo wrote:
Show 11 quoted lines
> Ideally, 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
Oh, curious. This should definitely be tightened in git-symbolic-ref(1) itself. The target should either be a root ref or something starting with "refs/". Anyway, that is of course outside of the scope of this patch series.
> We may need to add some restrictions for "referent" parameter when using > "git symbolic-ref" to create symrefs because ideally all the > nonpeudo-refs should be located under the "refs" directory and we may > tighten this in the future.
Agreed.
Show 24 quoted lines
> In order to tell the user we may tighten the "escape" situation, create > a new fsck message "escapeReferent" to notify the user that this may > become an error in the future. > > Mentored-by: Patrick Steinhardt <ps@pks.im> > Mentored-by: Karthik Nayak <karthik.188@gmail.com> > Signed-off-by: shejialuo <shejialuo@gmail.com> > --- > Documentation/fsck-msgids.txt | 8 ++++++++ > fsck.h | 1 + > refs/files-backend.c | 7 +++++++ > t/t0602-reffiles-fsck.sh | 18 ++++++++++++++++++ > 4 files changed, 34 insertions(+) > > diff --git a/Documentation/fsck-msgids.txt b/Documentation/fsck-msgids.txt > index e0e4519334..223974057d 100644 > --- a/Documentation/fsck-msgids.txt > +++ b/Documentation/fsck-msgids.txt > @@ -52,6 +52,14 @@ > `emptyName`:: > (WARN) A path contains an empty name. > > +`escapeReferent`:: > + (INFO) The referent of a symref is outside the "ref" directory.
Proposal: 'The referent of a symbolic reference points neither to a root reference nor to a reference starting with "refs/".'
I'd also rename this to e.g. "symrefTargetIsNotAReference" or something like that, because it's not really about whether or not the referent is "escaping". It's a bit of a mouthful, but I don't really have a better name. So feel free to pick something different that describes the error better.
Show 18 quoted lines
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 57ac466b64..bd215c8d08 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -3520,6 +3520,13 @@ static int files_fsck_symref_target(struct fsck_options *o,
> orig_last_byte = referent->buf[orig_len - 1];
> strbuf_rtrim(referent);
>
> + if (!starts_with(referent->buf, "refs/")) {
> + ret = fsck_report_ref(o, report,
> + FSCK_MSG_ESCAPE_REFERENT,
> + "referent '%s' is outside of refs/",
> + referent->buf);
> + }
> +
> if (check_refname_format(referent->buf, 0)) {
> ret = fsck_report_ref(o, report,
> FSCK_MSG_BAD_REFERENT,This check is invalid, because referents can also point to root refs. So you should probably also add a call to `is_root_ref()` here.
We also have `is_pseudo_ref()`, and one might be tempted to also allow that. But pseudo refs aren't proper refs, so I'd argue that a symref pointing to a pseudo ref is invalid, too.
Patrick