shejialuo <shejialuo@xxxxxxxxx> writes:
> +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.
> - 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;
> }
