Re: [PATCH v7 3/9] packed-backend: check whether the "packed-refs" is regular file

[Junio C Hamano <gitster@pobox.com>]

shejialuo <shejialuo@gmail.com> 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;
>  }