Re: [PATCH] read-cache: report lock error when refreshing index

From: Justin Tobler <jltobler@gmail.com>
Date: Jul 1, 2025, 16:08 UTC
Message-ID: <t4czubzmfuihxzmefwwhcel5qyss35gmodhfhvkfyiwitb5osw@d33acdbtds63>
In-Reply-To: <20250701115719.85226-1-hanyang.tony@bytedance.com>

On 25/07/01 07:57PM, Han Young wrote:

Show 5 quoted lines

> In the repo_refresh_and_write_index of read-cache.c, we return -1 to
> indicate that writing the index to disk failed.
> However, callers do not use this information. Commands such as stash print
>   "could not write index"
> and then exit, which does not help to discover the exact problem.

Ok, so `repo_refresh_and_write_index()` returns -1 when the "index.lock" cannot be acquired via `repo_hold_locked_index()` or the index write fails via `write_locked_index()`. The function returns 1 when the index refreshed fails via `refresh_index()`.

Callers of `repo_refresh_and_write_index()` currently do not differentiate between any of these failure types though. This patch wants to begin printing an error message if the lock file fails to be created. This would provide more insight into why the failure occurred than simply "error: could not write index". That makes sense.

> We can let repo_hold_locked_index print the error message if the locking
> failed.

It looks like `repo_hold_locked_index()` already has the `LOCK_REPORT_ON_ERROR` flag which will print the message we want.

Show 16 quoted lines

> Signed-off-by: Han Young <hanyang.tony@bytedance.com>
> ---
>  read-cache.c     |  2 +-
>  t/t3903-stash.sh | 15 +++------------
>  2 files changed, 4 insertions(+), 13 deletions(-)
> 
> diff --git a/read-cache.c b/read-cache.c
> index c0bb760ad..50e842bfa 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -1456,7 +1456,7 @@ int repo_refresh_and_write_index(struct repository *repo,
>  	struct lock_file lock_file = LOCK_INIT;
>  	int fd, ret = 0;
>  
> -	fd = repo_hold_locked_index(repo, &lock_file, 0);
> +	fd = repo_hold_locked_index(repo, &lock_file, gentle ? 0 : LOCK_REPORT_ON_ERROR);

Here we begin passing the `LOCK_REPORT_ON_ERROR` flag to print the error message only if `gentle` is not set. Makes sense.

Show 17 quoted lines

>  	if (!gentle && fd < 0)
>  		return -1;
>  	if (refresh_index(repo->index, refresh_flags, pathspec, seen, header_msg))
> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
> index 35b85c790..39098ade4 100755
> --- a/t/t3903-stash.sh
> +++ b/t/t3903-stash.sh
> @@ -1571,11 +1571,8 @@ test_expect_success 'stash create reports a locked index' '
>  		echo change >A.file &&
>  		touch .git/index.lock &&
>  
> -		cat >expect <<-EOF &&
> -		error: could not write index
> -		EOF
>  		test_must_fail git stash create 2>err &&
> -		test_cmp expect err
> +		test_grep "error: Unable to create '.*index.lock'" err

The test now checks for the explicit lock error message. The check for the "error: could not write index" message is also removed even though it should still be present in the output. Should we also continue to grep for that message too?

Show 13 quoted lines

>  	)
>  '
>  
> @@ -1588,11 +1585,8 @@ test_expect_success 'stash push reports a locked index' '
>  		echo change >A.file &&
>  		touch .git/index.lock &&
>  
> -		cat >expect <<-EOF &&
> -		error: could not write index
> -		EOF
>  		test_must_fail git stash push 2>err &&
> -		test_cmp expect err
> +		test_grep "error: Unable to create '.*index.lock'" err

Same question about testing for the previous error message here.

Show 13 quoted lines

>  	)
>  '
>  
> @@ -1606,11 +1600,8 @@ test_expect_success 'stash apply reports a locked index' '
>  		git stash push &&
>  		touch .git/index.lock &&
>  
> -		cat >expect <<-EOF &&
> -		error: could not write index
> -		EOF
>  		test_must_fail git stash apply 2>err &&
> -		test_cmp expect err
> +		test_grep "error: Unable to create '.*index.lock'" err

and here

>  	)
>  '

Thanks, -Justin