* [PATCH] read-cache: report lock error when refreshing index
@ 2025-07-01 11:57 Han Young
2025-07-01 16:08 ` Justin Tobler
2025-07-03 7:45 ` [PATCH v2] " Han Young
0 siblings, 2 replies; 6+ messages in thread
From: Han Young @ 2025-07-01 11:57 UTC (permalink / raw)
To: git; +Cc: Han Young
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.
We can let repo_hold_locked_index print the error message if the locking
failed.
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);
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
)
'
@@ -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
)
'
@@ -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
)
'
--
2.50.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] read-cache: report lock error when refreshing index
2025-07-01 11:57 [PATCH] read-cache: report lock error when refreshing index Han Young
@ 2025-07-01 16:08 ` Justin Tobler
2025-07-01 19:21 ` Junio C Hamano
2025-07-03 7:45 ` [PATCH v2] " Han Young
1 sibling, 1 reply; 6+ messages in thread
From: Justin Tobler @ 2025-07-01 16:08 UTC (permalink / raw)
To: Han Young; +Cc: git
On 25/07/01 07:57PM, Han Young wrote:
> 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.
> 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.
> 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?
> )
> '
>
> @@ -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.
> )
> '
>
> @@ -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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] read-cache: report lock error when refreshing index
2025-07-01 16:08 ` Justin Tobler
@ 2025-07-01 19:21 ` Junio C Hamano
0 siblings, 0 replies; 6+ messages in thread
From: Junio C Hamano @ 2025-07-01 19:21 UTC (permalink / raw)
To: Justin Tobler; +Cc: Han Young, git
Justin Tobler <jltobler@gmail•com> writes:
>> 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?
Probably. I was wondering which part of the patch removed the
existing message while reading this update to the test.
Thanks.
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2] read-cache: report lock error when refreshing index
2025-07-01 11:57 [PATCH] read-cache: report lock error when refreshing index Han Young
2025-07-01 16:08 ` Justin Tobler
@ 2025-07-03 7:45 ` Han Young
2025-07-07 18:01 ` Junio C Hamano
1 sibling, 1 reply; 6+ messages in thread
From: Han Young @ 2025-07-03 7:45 UTC (permalink / raw)
To: git; +Cc: Han Young
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.
We can let repo_hold_locked_index print the error message if the locking
failed.
Signed-off-by: Han Young <hanyang.tony@bytedance•com>
---
Changes since v1:
also check the "could not write index" error output
read-cache.c | 2 +-
t/t3903-stash.sh | 18 ++++++------------
2 files changed, 7 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);
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 c58ccb136..0bb4648e3 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -1672,11 +1672,9 @@ 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: could not write index" err &&
+ test_grep "error: Unable to create '.*index.lock'" err
)
'
@@ -1689,11 +1687,9 @@ 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: could not write index" err &&
+ test_grep "error: Unable to create '.*index.lock'" err
)
'
@@ -1707,11 +1703,9 @@ 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: could not write index" err &&
+ test_grep "error: Unable to create '.*index.lock'" err
)
'
--
2.50.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] read-cache: report lock error when refreshing index
2025-07-03 7:45 ` [PATCH v2] " Han Young
@ 2025-07-07 18:01 ` Junio C Hamano
2025-07-08 2:47 ` [External] " Han Young
0 siblings, 1 reply; 6+ messages in thread
From: Junio C Hamano @ 2025-07-07 18:01 UTC (permalink / raw)
To: Han Young; +Cc: git
Han Young <hanyang.tony@bytedance•com> writes:
> 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.
>
> We can let repo_hold_locked_index print the error message if the locking
> failed.
>
> Signed-off-by: Han Young <hanyang.tony@bytedance•com>
> ---
> Changes since v1:
> also check the "could not write index" error output
>
> read-cache.c | 2 +-
> t/t3903-stash.sh | 18 ++++++------------
> 2 files changed, 7 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);
Let's wrap this line to stay under 80-columns, i.e.
fd = repo_hold_locked_index(repo, &lock_file,
gentle ? 0 : LOCK_REPORT_ON_ERROR);
No need to resend, as I've done so locally while applying.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2025-07-08 2:47 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-07-01 11:57 [PATCH] read-cache: report lock error when refreshing index Han Young
2025-07-01 16:08 ` Justin Tobler
2025-07-01 19:21 ` Junio C Hamano
2025-07-03 7:45 ` [PATCH v2] " Han Young
2025-07-07 18:01 ` Junio C Hamano
2025-07-08 2:47 ` [External] " Han Young
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox