* Bug with deleted files and stash-push --keep-idex @ 2019-07-11 12:55 Martin Nicolay 2019-07-11 17:48 ` [PATCH] stash: fix handling removed files with --keep-index Thomas Gummerer 0 siblings, 1 reply; 7+ messages in thread From: Martin Nicolay @ 2019-07-11 12:55 UTC (permalink / raw) To: git [-- Attachment #1: Type: text/plain, Size: 2710 bytes --] Hi! I don't know if this is a software or documentation bug. man git-stash says about --keep-index: If the --keep-index option is used, all changes already added to the index are left intact. If a file is deleted and this deletion is in the index a following $ git stash push --keep-index keeps this deletion in the index but not in the working-tree. If a file is changed and this change is in the index a following $ git stash push --keep-index keeps this change in the index and also in the working-tree. This is inconsistent. ------------- deletion ------------- $ git status On branch master Changes to be committed: (use "git reset HEAD <file>..." to unstage) deleted: test-file $ git stash push --keep-index Saved working directory and index state WIP on master: a3171f887b new test-file $ git status On branch master Changes to be committed: (use "git reset HEAD <file>..." to unstage) deleted: test-file Untracked files: (use "git add <file>..." to include in what will be committed) test-file ------------- change ------------- $ git status On branch master Your branch is up to date with 'origin/master'. Changes to be committed: (use "git reset HEAD <file>..." to unstage) modified: test-file $ git stash push --keep-index Saved working directory and index state WIP on master: a3171f887b new test-file $ git status On branch master Your branch is up to date with 'origin/master'. Changes to be committed: (use "git reset HEAD <file>..." to unstage) modified: test-file -------------------------- My expectation was that the deletion would be preserved within the working-tree because this is a change added to the index and should be left intact. Best regards Martin Nicolay -- No MS-Word attachments (http://www.gnu.org/philosophy/no-word-attachments.html) _______________________________________________________________________________ OSM AG | Ruhrallee 191 | 45136 Essen | Fon: 0201-89 555 | Fax: 0201-89 55 400 web: www.osm-ag.de | e-mail: info@osm-ag.de IBAN: DE67 4325 0030 0001 0059 82 | BIC: WELADED1HRN USt-ldNr.: DE163337313 | HRB: 28171 Essen Aufsichtsratsvorsitzende: Dipl.-Kff. Sabine Elsas Vorstand: Johannes Kuhn (Vorsitzender), Christian Damsky, Axel Roland -- Diese E-Mail enthält vertrauliche und/oder rechtlich geschützte Informationen. Wenn Sie nicht der richtige Adressat sind oder diese E-Mail irrtümlich erhalten haben, informieren Sie bitte sofort den Absender und vernichten Sie diese Mail. Das unerlaubte Kopieren sowie die unbefugte Weitergabe dieser Mail ist nicht gestattet. _______________________________________________________________________________ ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH] stash: fix handling removed files with --keep-index 2019-07-11 12:55 Bug with deleted files and stash-push --keep-idex Martin Nicolay @ 2019-07-11 17:48 ` Thomas Gummerer 2019-07-11 21:23 ` Junio C Hamano 2019-07-16 14:23 ` [PATCH v2] " Thomas Gummerer 0 siblings, 2 replies; 7+ messages in thread From: Thomas Gummerer @ 2019-07-11 17:48 UTC (permalink / raw) To: Martin Nicolay; +Cc: git On 07/11, Martin Nicolay wrote: > Hi! > > I don't know if this is a software or documentation bug. > > man git-stash says about --keep-index: > If the --keep-index option is used, all changes already added to > the index are left intact. > > If a file is deleted and this deletion is in the index a following > $ git stash push --keep-index > keeps this deletion in the index but not in the working-tree. > > If a file is changed and this change is in the index a following > $ git stash push --keep-index > keeps this change in the index and also in the working-tree. > > This is inconsistent. Thanks for your report. This has come up before in https://public-inbox.org/git/1555437849815.60450@rasenplanscher.info/, which I first thought was expected behaviour, but that was just me misunderstanding the --keep-index option. So I belive this is indeed a bug. Luckily I had some more time to actually look at this this time around, so below is a potential fix. This comes with a small caveat of overwriting untracked files if they have been removed from the index, and replaced with a file that has not been added yet. I think that's okay as that happens in other places as well in stash, but wanted to point it out anyway. --- >8 --- Subject: [PATCH] stash: fix handling removed files with --keep-index git stash push --keep-index is supposed to keep all changes that have been added to the index, both in the index and on disk. Currently this doesn't behave correctly when a file is removed from the index. Instead of keeping it deleted on disk, --keep-index currently restores the file. Fix that behaviour by using 'git restore' which can faithfully restore the index and working tree. This also simplifies the code. Note that this will overwrite untracked files if the untracked file has the same name as a file that has been deleted in the index. Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com> --- builtin/stash.c | 34 ++++++++++------------------------ t/t3903-stash.sh | 7 +++++++ 2 files changed, 17 insertions(+), 24 deletions(-) diff --git a/builtin/stash.c b/builtin/stash.c index fde6397caa..2a58c007e1 100644 --- a/builtin/stash.c +++ b/builtin/stash.c @@ -1391,30 +1391,16 @@ static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int q } if (keep_index == 1 && !is_null_oid(&info.i_tree)) { - struct child_process cp_ls = CHILD_PROCESS_INIT; - struct child_process cp_checkout = CHILD_PROCESS_INIT; - struct strbuf out = STRBUF_INIT; - - if (reset_tree(&info.i_tree, 0, 1)) { - ret = -1; - goto done; - } - - cp_ls.git_cmd = 1; - argv_array_pushl(&cp_ls.args, "ls-files", "-z", - "--modified", "--", NULL); - - add_pathspecs(&cp_ls.args, ps); - if (pipe_command(&cp_ls, NULL, 0, &out, 0, NULL, 0)) { - ret = -1; - goto done; - } - - cp_checkout.git_cmd = 1; - argv_array_pushl(&cp_checkout.args, "checkout-index", - "-z", "--force", "--stdin", NULL); - if (pipe_command(&cp_checkout, out.buf, out.len, NULL, - 0, NULL, 0)) { + struct child_process cp_restore = CHILD_PROCESS_INIT; + + cp_restore.git_cmd = 1; + argv_array_pushl(&cp_restore.args, "restore", "--source", oid_to_hex(&info.i_tree), + "--staged", "--worktree", NULL); + if (!ps->nr) + argv_array_push(&cp_restore.args, "."); + else + add_pathspecs(&cp_restore.args, ps); + if (run_command(&cp_restore)) { ret = -1; goto done; } diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh index b22e671608..b8e337893f 100755 --- a/t/t3903-stash.sh +++ b/t/t3903-stash.sh @@ -1234,4 +1234,11 @@ test_expect_success 'stash works when user.name and user.email are not set' ' ) ' +test_expect_success 'stash --keep-index with file deleted in index does not resurrect it on disk' ' + test_commit to-remove to-remove && + git rm to-remove && + git stash --keep-index && + test_path_is_missing to-remove +' + test_done -- 2.22.0.599.gf5cf68d754 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] stash: fix handling removed files with --keep-index 2019-07-11 17:48 ` [PATCH] stash: fix handling removed files with --keep-index Thomas Gummerer @ 2019-07-11 21:23 ` Junio C Hamano 2019-07-16 13:35 ` Thomas Gummerer 2019-07-16 14:23 ` [PATCH v2] " Thomas Gummerer 1 sibling, 1 reply; 7+ messages in thread From: Junio C Hamano @ 2019-07-11 21:23 UTC (permalink / raw) To: Thomas Gummerer; +Cc: Martin Nicolay, git Thomas Gummerer <t.gummerer@gmail.com> writes: > Fix that behaviour by using 'git restore' which can faithfully restore > the index and working tree. This also simplifies the code. Hmph. I would have preferred to see we stayed away from 'restore' (and used 'checkout' instead, if you must use a Porcelain command), so that the "fix" can go to maintenance tracks, if distro packagers choose to backport it. Isn't the machinery for "git status" (in wt-status.c) mature enough to allow us to learn what got changed all in-core, without spawning an external process these days, though? > if (keep_index == 1 && !is_null_oid(&info.i_tree)) { > - struct child_process cp_ls = CHILD_PROCESS_INIT; > - struct child_process cp_checkout = CHILD_PROCESS_INIT; > - struct strbuf out = STRBUF_INIT; > - > - if (reset_tree(&info.i_tree, 0, 1)) { > - ret = -1; > - goto done; > - } > - > - cp_ls.git_cmd = 1; > - argv_array_pushl(&cp_ls.args, "ls-files", "-z", > - "--modified", "--", NULL); > - > - add_pathspecs(&cp_ls.args, ps); > - if (pipe_command(&cp_ls, NULL, 0, &out, 0, NULL, 0)) { > - ret = -1; > - goto done; > - } > - > - cp_checkout.git_cmd = 1; > - argv_array_pushl(&cp_checkout.args, "checkout-index", > - "-z", "--force", "--stdin", NULL); > - if (pipe_command(&cp_checkout, out.buf, out.len, NULL, > - 0, NULL, 0)) { > + struct child_process cp_restore = CHILD_PROCESS_INIT; > + > + cp_restore.git_cmd = 1; > + argv_array_pushl(&cp_restore.args, "restore", "--source", oid_to_hex(&info.i_tree), > + "--staged", "--worktree", NULL); > + if (!ps->nr) > + argv_array_push(&cp_restore.args, "."); > + else > + add_pathspecs(&cp_restore.args, ps); > + if (run_command(&cp_restore)) { > ret = -1; > goto done; > } > diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh > index b22e671608..b8e337893f 100755 > --- a/t/t3903-stash.sh > +++ b/t/t3903-stash.sh > @@ -1234,4 +1234,11 @@ test_expect_success 'stash works when user.name and user.email are not set' ' > ) > ' > > +test_expect_success 'stash --keep-index with file deleted in index does not resurrect it on disk' ' > + test_commit to-remove to-remove && > + git rm to-remove && > + git stash --keep-index && > + test_path_is_missing to-remove > +' > + > test_done ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] stash: fix handling removed files with --keep-index 2019-07-11 21:23 ` Junio C Hamano @ 2019-07-16 13:35 ` Thomas Gummerer 2019-07-16 20:08 ` Junio C Hamano 0 siblings, 1 reply; 7+ messages in thread From: Thomas Gummerer @ 2019-07-16 13:35 UTC (permalink / raw) To: Junio C Hamano; +Cc: Martin Nicolay, git On 07/11, Junio C Hamano wrote: > Thomas Gummerer <t.gummerer@gmail.com> writes: > > > Fix that behaviour by using 'git restore' which can faithfully restore > > the index and working tree. This also simplifies the code. > > Hmph. I would have preferred to see we stayed away from 'restore' > (and used 'checkout' instead, if you must use a Porcelain command), > so that the "fix" can go to maintenance tracks, if distro packagers > choose to backport it. Fair enough. I thought this wouldn't even go to 'maint', since the bug exists since a while, so 'git restore' would be fine, but didn't think of distro packagers. I'm happy to use 'checkout' here instead. > Isn't the machinery for "git status" (in wt-status.c) mature enough > to allow us to learn what got changed all in-core, without spawning > an external process these days, though? Maybe, I'm not all that familar with that machinery. My longer term hope was actually to libify the checkout machinery, and to use that here and use that to do all this (and the 'add', 'diff-index' and 'apply' dance above) in core. But maybe it's worth looking at the "git status" machinery for that as well? I probably won't have enough time to do that in the next few weeks though, so my preference would be to just use checkout for this (I'll send an updated patch) to fix the bug in the next release. As we're already spawning two external processes and would replace that with just spawning one it wouldn't make anything worse at least. Then we can try to do this all in-core at some point later, which I think is a bit more work, and probably wouldn't be ready for the next release (at least I won't have time to work on it). > > if (keep_index == 1 && !is_null_oid(&info.i_tree)) { > > - struct child_process cp_ls = CHILD_PROCESS_INIT; > > - struct child_process cp_checkout = CHILD_PROCESS_INIT; > > - struct strbuf out = STRBUF_INIT; > > - > > - if (reset_tree(&info.i_tree, 0, 1)) { > > - ret = -1; > > - goto done; > > - } > > - > > - cp_ls.git_cmd = 1; > > - argv_array_pushl(&cp_ls.args, "ls-files", "-z", > > - "--modified", "--", NULL); > > - > > - add_pathspecs(&cp_ls.args, ps); > > - if (pipe_command(&cp_ls, NULL, 0, &out, 0, NULL, 0)) { > > - ret = -1; > > - goto done; > > - } > > - > > - cp_checkout.git_cmd = 1; > > - argv_array_pushl(&cp_checkout.args, "checkout-index", > > - "-z", "--force", "--stdin", NULL); > > - if (pipe_command(&cp_checkout, out.buf, out.len, NULL, > > - 0, NULL, 0)) { > > + struct child_process cp_restore = CHILD_PROCESS_INIT; > > + > > + cp_restore.git_cmd = 1; > > + argv_array_pushl(&cp_restore.args, "restore", "--source", oid_to_hex(&info.i_tree), > > + "--staged", "--worktree", NULL); > > + if (!ps->nr) > > + argv_array_push(&cp_restore.args, "."); > > + else > > + add_pathspecs(&cp_restore.args, ps); > > + if (run_command(&cp_restore)) { > > ret = -1; > > goto done; > > } > > diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh > > index b22e671608..b8e337893f 100755 > > --- a/t/t3903-stash.sh > > +++ b/t/t3903-stash.sh > > @@ -1234,4 +1234,11 @@ test_expect_success 'stash works when user.name and user.email are not set' ' > > ) > > ' > > > > +test_expect_success 'stash --keep-index with file deleted in index does not resurrect it on disk' ' > > + test_commit to-remove to-remove && > > + git rm to-remove && > > + git stash --keep-index && > > + test_path_is_missing to-remove > > +' > > + > > test_done ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] stash: fix handling removed files with --keep-index 2019-07-16 13:35 ` Thomas Gummerer @ 2019-07-16 20:08 ` Junio C Hamano 0 siblings, 0 replies; 7+ messages in thread From: Junio C Hamano @ 2019-07-16 20:08 UTC (permalink / raw) To: Thomas Gummerer; +Cc: Martin Nicolay, git Thomas Gummerer <t.gummerer@gmail.com> writes: > On 07/11, Junio C Hamano wrote: >> Thomas Gummerer <t.gummerer@gmail.com> writes: >> >> > Fix that behaviour by using 'git restore' which can faithfully restore >> > the index and working tree. This also simplifies the code. >> >> Hmph. I would have preferred to see we stayed away from 'restore' >> (and used 'checkout' instead, if you must use a Porcelain command), >> so that the "fix" can go to maintenance tracks, if distro packagers >> choose to backport it. > > Fair enough. I thought this wouldn't even go to 'maint', since the > bug exists since a while, so 'git restore' would be fine, but didn't > think of distro packagers. I'm happy to use 'checkout' here instead. As long as the "--no-overlay" option is used, it is not much better. Backporting the fix goes only back to 2.22 and no earlier. ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2] stash: fix handling removed files with --keep-index 2019-07-11 17:48 ` [PATCH] stash: fix handling removed files with --keep-index Thomas Gummerer 2019-07-11 21:23 ` Junio C Hamano @ 2019-07-16 14:23 ` Thomas Gummerer 2019-07-16 20:08 ` Junio C Hamano 1 sibling, 1 reply; 7+ messages in thread From: Thomas Gummerer @ 2019-07-16 14:23 UTC (permalink / raw) To: Martin Nicolay; +Cc: git, Junio C Hamano git stash push --keep-index is supposed to keep all changes that have been added to the index, both in the index and on disk. Currently this doesn't behave correctly when a file is removed from the index. Instead of keeping it deleted on disk, --keep-index currently restores the file. Fix that behaviour by using 'git checkout' in no-overlay mode which can faithfully restore the index and working tree. This also simplifies the code. Note that this will overwrite untracked files if the untracked file has the same name as a file that has been deleted in the index. Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com> --- This would be the version using 'git checkout' instead of 'git restore'. Still not doing everything in-core though, as mentioned in the previous email. builtin/stash.c | 32 +++++++++----------------------- t/t3903-stash.sh | 7 +++++++ 2 files changed, 16 insertions(+), 23 deletions(-) diff --git a/builtin/stash.c b/builtin/stash.c index fde6397caa..b5a301f24d 100644 --- a/builtin/stash.c +++ b/builtin/stash.c @@ -1391,30 +1391,16 @@ static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int q } if (keep_index == 1 && !is_null_oid(&info.i_tree)) { - struct child_process cp_ls = CHILD_PROCESS_INIT; - struct child_process cp_checkout = CHILD_PROCESS_INIT; - struct strbuf out = STRBUF_INIT; - - if (reset_tree(&info.i_tree, 0, 1)) { - ret = -1; - goto done; - } - - cp_ls.git_cmd = 1; - argv_array_pushl(&cp_ls.args, "ls-files", "-z", - "--modified", "--", NULL); - - add_pathspecs(&cp_ls.args, ps); - if (pipe_command(&cp_ls, NULL, 0, &out, 0, NULL, 0)) { - ret = -1; - goto done; - } + struct child_process cp = CHILD_PROCESS_INIT; - cp_checkout.git_cmd = 1; - argv_array_pushl(&cp_checkout.args, "checkout-index", - "-z", "--force", "--stdin", NULL); - if (pipe_command(&cp_checkout, out.buf, out.len, NULL, - 0, NULL, 0)) { + cp.git_cmd = 1; + argv_array_pushl(&cp.args, "checkout", "--no-overlay", + oid_to_hex(&info.i_tree), "--", NULL); + if (!ps->nr) + argv_array_push(&cp.args, ":/"); + else + add_pathspecs(&cp.args, ps); + if (run_command(&cp)) { ret = -1; goto done; } diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh index b22e671608..b8e337893f 100755 --- a/t/t3903-stash.sh +++ b/t/t3903-stash.sh @@ -1234,4 +1234,11 @@ test_expect_success 'stash works when user.name and user.email are not set' ' ) ' +test_expect_success 'stash --keep-index with file deleted in index does not resurrect it on disk' ' + test_commit to-remove to-remove && + git rm to-remove && + git stash --keep-index && + test_path_is_missing to-remove +' + test_done -- 2.22.0.599.gf5cf68d754 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2] stash: fix handling removed files with --keep-index 2019-07-16 14:23 ` [PATCH v2] " Thomas Gummerer @ 2019-07-16 20:08 ` Junio C Hamano 0 siblings, 0 replies; 7+ messages in thread From: Junio C Hamano @ 2019-07-16 20:08 UTC (permalink / raw) To: Thomas Gummerer; +Cc: Martin Nicolay, git Thomas Gummerer <t.gummerer@gmail.com> writes: > git stash push --keep-index is supposed to keep all changes that have > been added to the index, both in the index and on disk. Thanks, will queue. ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-07-16 20:09 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-07-11 12:55 Bug with deleted files and stash-push --keep-idex Martin Nicolay 2019-07-11 17:48 ` [PATCH] stash: fix handling removed files with --keep-index Thomas Gummerer 2019-07-11 21:23 ` Junio C Hamano 2019-07-16 13:35 ` Thomas Gummerer 2019-07-16 20:08 ` Junio C Hamano 2019-07-16 14:23 ` [PATCH v2] " Thomas Gummerer 2019-07-16 20:08 ` Junio C Hamano
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.