diff mbox series

[v2,3/6] merge: fix save_state() to work when there are racy-dirty files

Message ID 89e5e633241e45a0c4b18289ab2fafdaabc8191e.1655621424.git.gitgitgadget@gmail.com (mailing list archive)
State New, archived
Headers show
Series Fix merge restore state | expand

Commit Message

Elijah Newren June 19, 2022, 6:50 a.m. UTC 
From: Elijah Newren <newren@gmail.com>

When there are racy-dirty files, but no files are modified,
`git stash create` exits with unsuccessful status.  This causes merge
to fail.  Refresh the index first to avoid this problem.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 builtin/merge.c | 8 ++++++++
 1 file changed, 8 insertions(+)

Comments

ZheNing Hu July 17, 2022, 4:28 p.m. UTC | #1 
Elijah Newren via GitGitGadget <gitgitgadget@gmail.com> 于2022年6月19日周日 14:50写道：
>
> From: Elijah Newren <newren@gmail.com>
>
> When there are racy-dirty files, but no files are modified,
> `git stash create` exits with unsuccessful status.  This causes merge
> to fail.  Refresh the index first to avoid this problem.
>
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
>  builtin/merge.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/builtin/merge.c b/builtin/merge.c
> index 00de224a2da..8ce4336dd3f 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -313,8 +313,16 @@ static int save_state(struct object_id *stash)
>         int len;
>         struct child_process cp = CHILD_PROCESS_INIT;
>         struct strbuf buffer = STRBUF_INIT;
> +       struct lock_file lock_file = LOCK_INIT;
> +       int fd;
>         int rc = -1;
>
> +       fd = repo_hold_locked_index(the_repository, &lock_file, 0);
> +       refresh_cache(REFRESH_QUIET);
> +       if (0 <= fd)
> +               repo_update_index_if_able(the_repository, &lock_file);
> +       rollback_lock_file(&lock_file);
> +
>         strvec_pushl(&cp.args, "stash", "create", NULL);
>         cp.out = -1;
>         cp.git_cmd = 1;
> --
> gitgitgadget
>

I just want to show what sence will meet this errors:

1. touch file
2. git add file
3. git stash push (user may do it before git merge)
4. touch file (update file but not update its content)
5. git merge (call git stash create and return 1)

So I have knew about what's the meaning of this patch:

If user do the git stash manually and update file timestamp
before git merge, it may make next git stash failed. And
refresh index can make index entry sync with the work tree
file status.

Thanks.

ZheNing Hu
Junio C Hamano July 19, 2022, 10:43 p.m. UTC | #2 
"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Elijah Newren <newren@gmail.com>
>
> When there are racy-dirty files, but no files are modified,
> `git stash create` exits with unsuccessful status.  This causes merge
> to fail.  Refresh the index first to avoid this problem.
>
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
>  builtin/merge.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/builtin/merge.c b/builtin/merge.c
> index 00de224a2da..8ce4336dd3f 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -313,8 +313,16 @@ static int save_state(struct object_id *stash)
>  	int len;
>  	struct child_process cp = CHILD_PROCESS_INIT;
>  	struct strbuf buffer = STRBUF_INIT;
> +	struct lock_file lock_file = LOCK_INIT;
> +	int fd;
>  	int rc = -1;
>  
> +	fd = repo_hold_locked_index(the_repository, &lock_file, 0);
> +	refresh_cache(REFRESH_QUIET);
> +	if (0 <= fd)
> +		repo_update_index_if_able(the_repository, &lock_file);
> +	rollback_lock_file(&lock_file);

I might have added "else" but rolling back a lock file that was
already committed or rolled back is a safe no-op, so this is OK.
The pattern already appears elsewhere twice, anyway.

Is it sufficient to be opportunistic?  IOW, if we fail to refresh
the index or write the refreshed result to disk, can we be silent
here and rely on "stash create" and things that follow to safely
fail as necessary, or should we also be detecting errors?

>  	strvec_pushl(&cp.args, "stash", "create", NULL);
>  	cp.out = -1;
>  	cp.git_cmd = 1;
Junio C Hamano July 19, 2022, 10:49 p.m. UTC | #3 
ZheNing Hu <adlternative@gmail.com> writes:

> Elijah Newren via GitGitGadget <gitgitgadget@gmail.com> 于2022年6月19日周日 14:50写道：
>>
>> From: Elijah Newren <newren@gmail.com>
>>
>> When there are racy-dirty files, but no files are modified,
>> `git stash create` exits with unsuccessful status.  This causes merge
>> to fail.  Refresh the index first to avoid this problem.

Racily dirty?  Or just being stat-dirty is sufficient to cause the
"stash create" to fail?

> I just want to show what sence will meet this errors:
>
> 1. touch file
> 2. git add file
> 3. git stash push (user may do it before git merge)
> 4. touch file (update file but not update its content)
> 5. git merge (call git stash create and return 1)

I think, from the above reproduction recipe, that the breakage does
not depend on racily-clean index entries (i.e. file touched within
the same timestamp as the last write of the index without changing
their size).  So s/racy-dirty/stat-dirty/ (both on the title and the
body) would be a sufficient fix.

Thanks.
Elijah Newren July 21, 2022, 1:09 a.m. UTC | #4 
On Tue, Jul 19, 2022 at 3:49 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> ZheNing Hu <adlternative@gmail.com> writes:
>
> > Elijah Newren via GitGitGadget <gitgitgadget@gmail.com> 于2022年6月19日周日 14:50写道：
> >>
> >> From: Elijah Newren <newren@gmail.com>
> >>
> >> When there are racy-dirty files, but no files are modified,
> >> `git stash create` exits with unsuccessful status.  This causes merge
> >> to fail.  Refresh the index first to avoid this problem.
>
> Racily dirty?  Or just being stat-dirty is sufficient to cause the
> "stash create" to fail?
>
> > I just want to show what sence will meet this errors:
> >
> > 1. touch file
> > 2. git add file
> > 3. git stash push (user may do it before git merge)
> > 4. touch file (update file but not update its content)
> > 5. git merge (call git stash create and return 1)
>
> I think, from the above reproduction recipe, that the breakage does
> not depend on racily-clean index entries (i.e. file touched within
> the same timestamp as the last write of the index without changing
> their size).  So s/racy-dirty/stat-dirty/ (both on the title and the
> body) would be a sufficient fix.

Yep, stat-dirty.  I'll fix up the title and body; thanks.
diff mbox series

Patch

diff --git a/builtin/merge.c b/builtin/merge.c
index 00de224a2da..8ce4336dd3f 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -313,8 +313,16 @@  static int save_state(struct object_id *stash)
 	int len;
 	struct child_process cp = CHILD_PROCESS_INIT;
 	struct strbuf buffer = STRBUF_INIT;
+	struct lock_file lock_file = LOCK_INIT;
+	int fd;
 	int rc = -1;
 
+	fd = repo_hold_locked_index(the_repository, &lock_file, 0);
+	refresh_cache(REFRESH_QUIET);
+	if (0 <= fd)
+		repo_update_index_if_able(the_repository, &lock_file);
+	rollback_lock_file(&lock_file);
+
 	strvec_pushl(&cp.args, "stash", "create", NULL);
 	cp.out = -1;
 	cp.git_cmd = 1;