Re: [PATCH v7 3/9] packed-backend: check whether the "packed-refs" is regular file
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Feb 26, 2025, 18:36 UTC
- Message-ID
- <xmqq5xkwd042.fsf@gitster.g>
- In-Reply-To
- <Z78cAU69IUSDgpuD@ArchLinux>
shejialuo <shejialuo@gmail.com> writes:
Show 12 quoted lines
> +static int packed_fsck(struct ref_store *ref_store,
> + struct fsck_options *o,
> struct worktree *wt)
> {
> + struct packed_ref_store *refs = packed_downcast(ref_store,
> + REF_STORE_READ, "fsck");
> + struct stat st;
> + int ret = 0;
> + int fd;
>
> if (!is_main_worktree(wt))
> return 0;I do not think it is worth a reroll only to improve this one, but for future reference, initializing "fd = -1" and jumping to cleanup here instead of "return 0" would future-proof the code better. This is especially so, given that in a few patches later, we would add a strbuf that is initialized before this "we do not do anything outside the primary worktree" short-cut, and many "goto cleanup"s we see in this patch below would jump to cleanup to strbuf_release() on that initialized but unused strbuf. Jumping there with negative fd to cleanup that already avoids close(fd) for negative fd would be like jumping there with initialized but unused strbuf. Having a single exit point ("cleanup:" label) would help future evolution of the code, by making it easier to add more resource-acquriing code to this function in the future.
Show 41 quoted lines
> - return 0;
> + if (o->verbose)
> + fprintf_ln(stderr, "Checking packed-refs file %s", refs->path);
> +
> + fd = open_nofollow(refs->path, O_RDONLY);
> + if (fd < 0) {
> + /*
> + * If the packed-refs file doesn't exist, there's nothing
> + * to check.
> + */
> + if (errno == ENOENT)
> + goto cleanup;
> +
> + if (errno == ELOOP) {
> + struct fsck_ref_report report = { 0 };
> + report.path = "packed-refs";
> + ret = fsck_report_ref(o, &report,
> + FSCK_MSG_BAD_REF_FILETYPE,
> + "not a regular file but a symlink");
> + goto cleanup;
> + }
> +
> + ret = error_errno(_("unable to open '%s'"), refs->path);
> + goto cleanup;
> + } else if (fstat(fd, &st) < 0) {
> + ret = error_errno(_("unable to stat '%s'"), refs->path);
> + goto cleanup;
> + } else if (!S_ISREG(st.st_mode)) {
> + struct fsck_ref_report report = { 0 };
> + report.path = "packed-refs";
> + ret = fsck_report_ref(o, &report,
> + FSCK_MSG_BAD_REF_FILETYPE,
> + "not a regular file");
> + goto cleanup;
> + }
> +
> +cleanup:
> + if (fd >= 0)
> + close(fd);
> + return ret;
> }