Re: [PATCH] refs/files: skip lock files during consistency checks

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

Karthik Nayak <karthik.188@gmail.com> writes:

> Running consistency checks on the files reference backend involves
> iterating over all the existing files in the 'refs/' directory and
> running all `files_fsck_ref()` on each of them.
>
> Unfortunately this also includes the '*.lock' files created by the files
> backend. While the `files_fsck_refs_name()` check was skipping over such
> lock files, all other checks still continue to validate them.
>
> Avoid this situation by moving the check for lock files to a layer above
> and skipping all checks when encountering such a file.

Saying "all other checks" when there is only one other check is
highly confusing, even though it may not be telling any lies.
files_fsck_ref() calls files_fsck_refs_name() and
files_fsck_refs_content(), so this change moves the test from the
former to a higher level caller so that files_fsck_ref() itself
won't be called, the net result is that files_fsck_refs_content)( is
not called on these *.lock files.

And this does not explain why only one of the callers of
files_fsck_ref() is the best place to add this "*.lock files are
exempt" knowledge to, compared to (presumably at the beginning of)
files_fsck_ref() itself.  If we do not anticipate that we will ever
gain new caller to the function and the only meaningful caller that
needs this protection is files_fsck_refs_dir(), then the choice may
be justifiable, but if we check at the beginning of the callee, we
do not have to rely on such an assuption, do we?

> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
>  refs/files-backend.c     | 22 +++++++++++-----------
>  t/t0602-reffiles-fsck.sh | 21 +++++++++++++++++++++
>  2 files changed, 32 insertions(+), 11 deletions(-)

> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index b3b0c25f84..f1bdfbe88e 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -3864,22 +3864,12 @@ static int files_fsck_refs_content(struct ref_store *ref_store,
>  static int files_fsck_refs_name(struct ref_store *ref_store UNUSED,
>  				struct fsck_options *o,
>  				const char *refname,
> -				const char *path,
> +				const char *path UNUSED,
>  				int mode UNUSED)
>  {
>  	struct strbuf sb = STRBUF_INIT;
> -	const char *filename;
>  	int ret = 0;
>  
> -	filename = basename((char *) path);
> -
> -	/*
> -	 * Ignore the files ending with ".lock" as they may be lock files
> -	 * However, do not allow bare ".lock" files.
> -	 */
> -	if (filename[0] != '.' && ends_with(filename, ".lock"))
> -		goto cleanup;
> -
>  	if (is_root_ref(refname))
>  		goto cleanup;
>  
> @@ -3939,6 +3929,7 @@ static int files_fsck_refs_dir(struct ref_store *ref_store,
>  	struct strbuf refname = STRBUF_INIT;
>  	struct strbuf sb = STRBUF_INIT;
>  	struct dir_iterator *iter;
> +	const char *filename;
>  	int iter_status;
>  	int ret = 0;
>  
> @@ -3962,6 +3953,15 @@ static int files_fsck_refs_dir(struct ref_store *ref_store,
>  			strbuf_addf(&refname, "worktrees/%s/", wt->id);
>  		strbuf_addf(&refname, "refs/%s", iter->relative_path);
>  
> +		filename = basename((char *) iter->path.buf);
> +
> +		/*
> +		 * Ignore the files ending with ".lock" as they may be lock files
> +		 * However, do not allow bare ".lock" files.
> +		 */
> +		if (filename[0] != '.' && ends_with(filename, ".lock"))
> +			continue;
> +
>  		if (files_fsck_ref(ref_store, o, refname.buf,
>  				   iter->path.buf, iter->st.st_mode) < 0)
>  			ret = -1;
> diff --git a/t/t0602-reffiles-fsck.sh b/t/t0602-reffiles-fsck.sh
> index 3c1f553b81..fc67bee161 100755
> --- a/t/t0602-reffiles-fsck.sh
> +++ b/t/t0602-reffiles-fsck.sh
> @@ -87,6 +87,27 @@ test_expect_success 'ref name should be checked' '
>  	)
>  '
>  
> +test_expect_success 'lock files should be ignored' '
> +	test_when_finished "rm -rf repo" &&
> +	git init repo &&
> +	(
> +		cd repo &&
> +		git commit --allow-empty -m initial &&
> +		git checkout -b branch-1 &&
> +
> +		touch .git/refs/heads/branch-1.lock &&
> +		git refs verify 2>err &&
> +		test_must_be_empty err &&
> +
> +		echo "foobar" >.git/refs/heads/branch-2 &&
> +		test_must_fail git refs verify 2>err &&
> +		cat >expect <<-EOF &&
> +		error: refs/heads/branch-2: badRefContent: foobar
> +		EOF
> +		test_cmp expect err
> +	)
> +'
> +
>  test_expect_success 'ref name check should be adapted into fsck messages' '
>  	test_when_finished "rm -rf repo" &&
>  	git init repo &&