On Tue, Sep 03, 2024 at 08:21:03PM +0800, shejialuo wrote:
> 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.
>
> We firstly use the "strbuf_add_real_path" to resolve the symlink and
> get the absolute path "referent_path" which the symlink ref points
> to. Then we can get the absolute path "abs_gitdir" of the "gitdir".
> By combining "referent_path" and "abs_gitdir", we can extract the
> "referent". Thus, we can reuse "files_fsck_symref_target" function to
> seamlessly check the symlink refs.
>
> Because we are going to drop support for "core.prefersymlinkrefs", add a
> new fsck message "symlinkRef" to let the user be aware of this
> information.
I don't we fully decided to drop support for symrefs via symbolic links
yet, so this is a tad too strong of a statement. I'd rather say that we
consider deprecating it in the future, but first need to asses whether
they may still be used.
Also, didn't we say that we'd want to remove support for _writing_
symbolic links, but not for reading them? Not a 100% sure though.
> @@ -1961,13 +1965,12 @@ static int create_ref_symlink(struct ref_lock *lock, const char *target)
>
> if (ret)
> fprintf(stderr, "no symlink - falling back to symbolic ref\n");
> -#endif
> return ret;
> }
> +#endif
>
> -static int create_symref_lock(struct files_ref_store *refs,
> - struct ref_lock *lock, const char *refname,
> - const char *target, struct strbuf *err)
> +static int create_symref_lock(struct ref_lock *lock, const char *target,
> + struct strbuf *err)
> {
> if (!fdopen_lock_file(&lock->lk, "w")) {
> strbuf_addf(err, "unable to fdopen %s: %s",
> @@ -2583,8 +2586,7 @@ static int lock_ref_for_update(struct files_ref_store *refs,
> }
>
> if (update->new_target && !(update->flags & REF_LOG_ONLY)) {
> - if (create_symref_lock(refs, lock, update->refname,
> - update->new_target, err)) {
> + if (create_symref_lock(lock, update->new_target, err)) {
> ret = TRANSACTION_GENERIC_ERROR;
> goto out;
> }
Why does the writing side need to change?
> @@ -3509,9 +3516,11 @@ static int files_fsck_refs_content(struct ref_store *ref_store,
> {
> struct strbuf referent_path = STRBUF_INIT;
> struct strbuf ref_content = STRBUF_INIT;
> + struct strbuf abs_gitdir = STRBUF_INIT;
> struct strbuf referent = STRBUF_INIT;
> struct strbuf refname = STRBUF_INIT;
> struct fsck_ref_report report = {0};
> + unsigned int symbolic_link = 0;
This variable isn't used, as both code paths that end up using it could
just statically set it to `1` or `0`.
> const char *trailing = NULL;
> unsigned int type = 0;
> int failure_errno = 0;
> @@ -3521,8 +3530,37 @@ static int files_fsck_refs_content(struct ref_store *ref_store,
> strbuf_addf(&refname, "%s/%s", refs_check_dir, iter->relative_path);
> report.path = refname.buf;
>
> - if (S_ISLNK(iter->st.st_mode))
> + if (S_ISLNK(iter->st.st_mode)) {
> + const char* relative_referent_path;
> +
> + symbolic_link = 1;
> + 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(&referent_path, iter->path.buf);
> +
> + if (!skip_prefix(referent_path.buf,
> + abs_gitdir.buf,
> + &relative_referent_path)) {
> + ret = fsck_report_ref(o, &report,
> + FSCK_MSG_BAD_SYMREF_TARGET,
> + "point to target outside gitdir");
> + goto cleanup;
> + }
> +
> + strbuf_addstr(&referent, relative_referent_path);
> + ret = files_fsck_symref_target(o, &report,
> + &referent, &referent_path,
> + symbolic_link);
> +
> goto cleanup;
> + }
>
> if (strbuf_read_file(&ref_content, iter->path.buf, 0) < 0) {
> ret = error_errno(_("%s/%s: unable to read the ref"),
Patrick
