[v13,11/27] stash: convert apply to builtin
diff mbox series

Message ID 20190225231631.30507-12-t.gummerer@gmail.com
State New
Headers show
Series
  • Convert "git stash" to C builtin
Related show

Commit Message

Thomas Gummerer Feb. 25, 2019, 11:16 p.m. UTC
From: Joel Teichroeb <joel@teichroeb.net>

Add a builtin helper for performing stash commands. Converting
all at once proved hard to review, so starting with just apply
lets conversion get started without the other commands being
finished.

The helper is being implemented as a drop in replacement for
stash so that when it is complete it can simply be renamed and
the shell script deleted.

Delete the contents of the apply_stash shell function and replace
it with a call to stash--helper apply until pop is also
converted.

Signed-off-by: Joel Teichroeb <joel@teichroeb.net>
Signed-off-by: Paul-Sebastian Ungureanu <ungureanupaulsebastian@gmail.com>
Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
 .gitignore              |   1 +
 Makefile                |   1 +
 builtin.h               |   1 +
 builtin/stash--helper.c | 451 ++++++++++++++++++++++++++++++++++++++++
 git-stash.sh            |  78 +------
 git.c                   |   1 +
 6 files changed, 462 insertions(+), 71 deletions(-)
 create mode 100644 builtin/stash--helper.c

Comments

Ævar Arnfjörð Bjarmason March 14, 2019, 1:19 p.m. UTC | #1
On Tue, Feb 26 2019, Thomas Gummerer wrote:

> From: Joel Teichroeb <joel@teichroeb.net>
>
> Add a builtin helper for performing stash commands. Converting
> all at once proved hard to review, so starting with just apply
> lets conversion get started without the other commands being
> finished.
>
> The helper is being implemented as a drop in replacement for
> stash so that when it is complete it can simply be renamed and
> the shell script deleted.
>
> Delete the contents of the apply_stash shell function and replace
> it with a call to stash--helper apply until pop is also
> converted.

This

    GIT_TEST_FSMONITOR=$PWD/t7519/fsmonitor-all ./t3420-rebase-autostash.sh

Now fails, which bisects to 8a0fc8d19d ("stash: convert apply to
builtin", 2019-02-25).

Tested on both a CentOS 6 & modern Debian testing machine:

    + git rebase -i --autostash HEAD^
    Created autostash: 5cd734b
    HEAD is now at 0c4d2f1 third commit
    hint: Waiting for your editor to close the file...
    error: There was a problem with the editor '"$FAKE_EDITOR"'.
    Applied autostash.
    + exit_code=1
    + test 1 -eq 0
    + test_match_signal 13 1
    + test 1 = 141
    + test 1 = 269
    + return 1
    + test 1 -gt 129
    + test 1 -eq 127
    + test 1 -eq 126
    + return 0
    + rm -f abort-editor.sh
    + echo conflicting-content
    + test_cmp expected file0
    + diff -u expected file0
    --- expected    2019-03-14 13:19:08.212215263 +0000
    +++ file0       2019-03-14 13:19:08.196215250 +0000
    @@ -1 +1 @@
    -conflicting-content
    +uncommitted-content
    error: last command exited with $?=1
    not ok 36 - autostash is saved on editor failure with conflict

Are you able to reproduce this? And if so I suggest running the test
suite with some of the other GIT_TEST_* modes documented in
t/README. Maybe it'll turn up something else...
Johannes Schindelin March 14, 2019, 3:20 p.m. UTC | #2
Hi Ævar,

On Thu, 14 Mar 2019, Ævar Arnfjörð Bjarmason wrote:

> On Tue, Feb 26 2019, Thomas Gummerer wrote:
> 
> > From: Joel Teichroeb <joel@teichroeb.net>
> >
> > Add a builtin helper for performing stash commands. Converting
> > all at once proved hard to review, so starting with just apply
> > lets conversion get started without the other commands being
> > finished.
> >
> > The helper is being implemented as a drop in replacement for
> > stash so that when it is complete it can simply be renamed and
> > the shell script deleted.
> >
> > Delete the contents of the apply_stash shell function and replace
> > it with a call to stash--helper apply until pop is also
> > converted.
> 
> This
> 
>     GIT_TEST_FSMONITOR=$PWD/t7519/fsmonitor-all ./t3420-rebase-autostash.sh
> 
> Now fails, which bisects to 8a0fc8d19d ("stash: convert apply to
> builtin", 2019-02-25).
> 
> Tested on both a CentOS 6 & modern Debian testing machine:
> 
>     + git rebase -i --autostash HEAD^
>     Created autostash: 5cd734b
>     HEAD is now at 0c4d2f1 third commit
>     hint: Waiting for your editor to close the file...
>     error: There was a problem with the editor '"$FAKE_EDITOR"'.
>     Applied autostash.
>     + exit_code=1
>     + test 1 -eq 0
>     + test_match_signal 13 1
>     + test 1 = 141
>     + test 1 = 269
>     + return 1
>     + test 1 -gt 129
>     + test 1 -eq 127
>     + test 1 -eq 126
>     + return 0
>     + rm -f abort-editor.sh
>     + echo conflicting-content
>     + test_cmp expected file0
>     + diff -u expected file0
>     --- expected    2019-03-14 13:19:08.212215263 +0000
>     +++ file0       2019-03-14 13:19:08.196215250 +0000
>     @@ -1 +1 @@
>     -conflicting-content
>     +uncommitted-content
>     error: last command exited with $?=1
>     not ok 36 - autostash is saved on editor failure with conflict
> 
> Are you able to reproduce this? And if so I suggest running the test
> suite with some of the other GIT_TEST_* modes documented in
> t/README. Maybe it'll turn up something else...

Yep, totally can reproduce it :-(

I'll try to investigate a bit,
Dscho
Ævar Arnfjörð Bjarmason March 14, 2019, 3:40 p.m. UTC | #3
On Thu, Mar 14 2019, Johannes Schindelin wrote:

> Hi Ævar,
>
> On Thu, 14 Mar 2019, Ævar Arnfjörð Bjarmason wrote:
>
>> On Tue, Feb 26 2019, Thomas Gummerer wrote:
>>
>> > From: Joel Teichroeb <joel@teichroeb.net>
>> >
>> > Add a builtin helper for performing stash commands. Converting
>> > all at once proved hard to review, so starting with just apply
>> > lets conversion get started without the other commands being
>> > finished.
>> >
>> > The helper is being implemented as a drop in replacement for
>> > stash so that when it is complete it can simply be renamed and
>> > the shell script deleted.
>> >
>> > Delete the contents of the apply_stash shell function and replace
>> > it with a call to stash--helper apply until pop is also
>> > converted.
>>
>> This
>>
>>     GIT_TEST_FSMONITOR=$PWD/t7519/fsmonitor-all ./t3420-rebase-autostash.sh
>>
>> Now fails, which bisects to 8a0fc8d19d ("stash: convert apply to
>> builtin", 2019-02-25).
>>
>> Tested on both a CentOS 6 & modern Debian testing machine:
>>
>>     + git rebase -i --autostash HEAD^
>>     Created autostash: 5cd734b
>>     HEAD is now at 0c4d2f1 third commit
>>     hint: Waiting for your editor to close the file...
>>     error: There was a problem with the editor '"$FAKE_EDITOR"'.
>>     Applied autostash.
>>     + exit_code=1
>>     + test 1 -eq 0
>>     + test_match_signal 13 1
>>     + test 1 = 141
>>     + test 1 = 269
>>     + return 1
>>     + test 1 -gt 129
>>     + test 1 -eq 127
>>     + test 1 -eq 126
>>     + return 0
>>     + rm -f abort-editor.sh
>>     + echo conflicting-content
>>     + test_cmp expected file0
>>     + diff -u expected file0
>>     --- expected    2019-03-14 13:19:08.212215263 +0000
>>     +++ file0       2019-03-14 13:19:08.196215250 +0000
>>     @@ -1 +1 @@
>>     -conflicting-content
>>     +uncommitted-content
>>     error: last command exited with $?=1
>>     not ok 36 - autostash is saved on editor failure with conflict
>>
>> Are you able to reproduce this? And if so I suggest running the test
>> suite with some of the other GIT_TEST_* modes documented in
>> t/README. Maybe it'll turn up something else...
>
> Yep, totally can reproduce it :-(

In the meantime I did a build with "next" (so stash-in-C) using the
standard test mode and these:

    (cd t && GIT_TEST_GETTEXT_POISON=true /usr/bin/prove $BKNG_GIT_HARNESS_OPTIONS t[0-9]*.sh)
    (cd t && GIT_TEST_FSMONITOR=$PWD/t7519/fsmonitor-all GIT_SKIP_TESTS="t3404.8 t3420.36" /usr/bin/prove $BKNG_GIT_HARNESS_OPTIONS t[0-9]*.sh)
    (cd t && GIT_TEST_SPLIT_INDEX=true /usr/bin/prove $BKNG_GIT_HARNESS_OPTIONS t[0-9]*.sh)
    (cd t && GIT_TEST_FULL_IN_PACK_ARRAY=true GIT_TEST_OE_SIZE=10 /usr/bin/prove $BKNG_GIT_HARNESS_OPTIONS t[0-9]*.sh)
    (cd t && GIT_TEST_COMMIT_GRAPH=true /usr/bin/prove $BKNG_GIT_HARNESS_OPTIONS t[0-9]*.sh)
    (cd t && GIT_TEST_MULTI_PACK_INDEX=true /usr/bin/prove $BKNG_GIT_HARNESS_OPTIONS t[0-9]*.sh)
    (cd t && GIT_TEST_STASH_USE_BUILTIN=false /usr/bin/prove $BKNG_GIT_HARNESS_OPTIONS t[0-9]*.sh)
    (cd t && GIT_TEST_CHECK_COLLISIONS=false /usr/bin/prove $BKNG_GIT_HARNESS_OPTIONS t[0-9]*.sh)

Only this specific test failed.
Johannes Schindelin March 14, 2019, 10:45 p.m. UTC | #4
Hi Ævar,

On Thu, 14 Mar 2019, Ævar Arnfjörð Bjarmason wrote:

> On Thu, Mar 14 2019, Johannes Schindelin wrote:
> 
> > On Thu, 14 Mar 2019, Ævar Arnfjörð Bjarmason wrote:
> >
> >> On Tue, Feb 26 2019, Thomas Gummerer wrote:
> >>
> >> > From: Joel Teichroeb <joel@teichroeb.net>
> >> >
> >> > Add a builtin helper for performing stash commands. Converting
> >> > all at once proved hard to review, so starting with just apply
> >> > lets conversion get started without the other commands being
> >> > finished.
> >> >
> >> > The helper is being implemented as a drop in replacement for
> >> > stash so that when it is complete it can simply be renamed and
> >> > the shell script deleted.
> >> >
> >> > Delete the contents of the apply_stash shell function and replace
> >> > it with a call to stash--helper apply until pop is also
> >> > converted.
> >>
> >> This
> >>
> >>     GIT_TEST_FSMONITOR=$PWD/t7519/fsmonitor-all ./t3420-rebase-autostash.sh
> >>
> >> Now fails, which bisects to 8a0fc8d19d ("stash: convert apply to
> >> builtin", 2019-02-25).
> >>
> >> Tested on both a CentOS 6 & modern Debian testing machine:
> >>
> >>     + git rebase -i --autostash HEAD^
> >>     Created autostash: 5cd734b
> >>     HEAD is now at 0c4d2f1 third commit
> >>     hint: Waiting for your editor to close the file...
> >>     error: There was a problem with the editor '"$FAKE_EDITOR"'.
> >>     Applied autostash.
> >>     + exit_code=1
> >>     + test 1 -eq 0
> >>     + test_match_signal 13 1
> >>     + test 1 = 141
> >>     + test 1 = 269
> >>     + return 1
> >>     + test 1 -gt 129
> >>     + test 1 -eq 127
> >>     + test 1 -eq 126
> >>     + return 0
> >>     + rm -f abort-editor.sh
> >>     + echo conflicting-content
> >>     + test_cmp expected file0
> >>     + diff -u expected file0
> >>     --- expected    2019-03-14 13:19:08.212215263 +0000
> >>     +++ file0       2019-03-14 13:19:08.196215250 +0000
> >>     @@ -1 +1 @@
> >>     -conflicting-content
> >>     +uncommitted-content
> >>     error: last command exited with $?=1
> >>     not ok 36 - autostash is saved on editor failure with conflict
> >>
> >> Are you able to reproduce this? And if so I suggest running the test
> >> suite with some of the other GIT_TEST_* modes documented in
> >> t/README. Maybe it'll turn up something else...
> >
> > Yep, totally can reproduce it :-(

Well, isn't this exciting: we found not a bug in the built-in stash (even
if Junio probably expected yet another one), but an fsmonitor one! Even
better, I think this might be the bug that Alex Vandiver was chasing and
that he talked about at the Contributors' Summit last year in Barcelona.

The symptom is that cache entries are sometimes considered up to date,
when they really are not.

And the reason is that the fsmonitor has this honking global flag
`has_run_once` (it is not really global, it is `static` to
`refresh_fsmonitor()`, but that's the same for all practical purposes, as
it is *not* specific to one `struct index_state`), which was kind of okay
as long as `the_index` was used implicitly by everything.

Except it was not okay when `discard_index()` (or `discard_cache()`) was
called: in that case, the flag was not re-set. And re-set it needs to be,
in that case, otherwise the fsmonitor is not asked which entries need to
be updated.

I saw this pretty early on in my investigation and marked it up for a
follow-up task, wasting hours of investigation by not believing that this
could be the culprit of the bug you described. I did not believe it
because `git stash apply` is *spawned*, so there is not even an index that
needs to be discarded (I thought; more on that one later).

It is quite curious that this is the only occasion that our test suite
covers that particular part of the fsmonitor...

I do not really want to rely on implementation details of the rebase to verify
that the fsmonitor is queried again (and, crucially, resets the
FSMONITOR_VALID flag of the file(s) indicated as out of date) after the index
is discarded and re-read.

I guess the best bet is to extend `t/helper/test-read-cache.c` to optionally
output information about a specific cache entry, then refresh it, and then run
that test helper with fsmonitor-all (which should mark anything as modified,
all the time). That should verify that the fix works.

I did exactly that, and pushed the result to https://github.com/dscho/git
as `fix-fsmonitor` branch.

Could I ask you to test that one (it is based off of Git for Windows'
`master`, but that should compile cleanly for you)?

For now, I am a bit spent, so I'll leave the rest for tomorrow.

> In the meantime I did a build with "next" (so stash-in-C) using the
> standard test mode and these:
> 
>     (cd t && GIT_TEST_GETTEXT_POISON=true /usr/bin/prove $BKNG_GIT_HARNESS_OPTIONS t[0-9]*.sh)
>     (cd t && GIT_TEST_FSMONITOR=$PWD/t7519/fsmonitor-all GIT_SKIP_TESTS="t3404.8 t3420.36" /usr/bin/prove $BKNG_GIT_HARNESS_OPTIONS t[0-9]*.sh)
>     (cd t && GIT_TEST_SPLIT_INDEX=true /usr/bin/prove $BKNG_GIT_HARNESS_OPTIONS t[0-9]*.sh)
>     (cd t && GIT_TEST_FULL_IN_PACK_ARRAY=true GIT_TEST_OE_SIZE=10 /usr/bin/prove $BKNG_GIT_HARNESS_OPTIONS t[0-9]*.sh)
>     (cd t && GIT_TEST_COMMIT_GRAPH=true /usr/bin/prove $BKNG_GIT_HARNESS_OPTIONS t[0-9]*.sh)
>     (cd t && GIT_TEST_MULTI_PACK_INDEX=true /usr/bin/prove $BKNG_GIT_HARNESS_OPTIONS t[0-9]*.sh)
>     (cd t && GIT_TEST_STASH_USE_BUILTIN=false /usr/bin/prove $BKNG_GIT_HARNESS_OPTIONS t[0-9]*.sh)
>     (cd t && GIT_TEST_CHECK_COLLISIONS=false /usr/bin/prove $BKNG_GIT_HARNESS_OPTIONS t[0-9]*.sh)
> 
> Only this specific test failed.

Well, good!

Thank you for getting the ball rolling!
Dscho
Ævar Arnfjörð Bjarmason March 14, 2019, 11:39 p.m. UTC | #5
On Thu, Mar 14 2019, Johannes Schindelin wrote:

> Hi Ævar,
>
> On Thu, 14 Mar 2019, Ævar Arnfjörð Bjarmason wrote:
>
>> On Thu, Mar 14 2019, Johannes Schindelin wrote:
>>
>> > On Thu, 14 Mar 2019, Ævar Arnfjörð Bjarmason wrote:
>> >
>> >> On Tue, Feb 26 2019, Thomas Gummerer wrote:
>> >>
>> >> > From: Joel Teichroeb <joel@teichroeb.net>
>> >> >
>> >> > Add a builtin helper for performing stash commands. Converting
>> >> > all at once proved hard to review, so starting with just apply
>> >> > lets conversion get started without the other commands being
>> >> > finished.
>> >> >
>> >> > The helper is being implemented as a drop in replacement for
>> >> > stash so that when it is complete it can simply be renamed and
>> >> > the shell script deleted.
>> >> >
>> >> > Delete the contents of the apply_stash shell function and replace
>> >> > it with a call to stash--helper apply until pop is also
>> >> > converted.
>> >>
>> >> This
>> >>
>> >>     GIT_TEST_FSMONITOR=$PWD/t7519/fsmonitor-all ./t3420-rebase-autostash.sh
>> >>
>> >> Now fails, which bisects to 8a0fc8d19d ("stash: convert apply to
>> >> builtin", 2019-02-25).
>> >>
>> >> Tested on both a CentOS 6 & modern Debian testing machine:
>> >>
>> >>     + git rebase -i --autostash HEAD^
>> >>     Created autostash: 5cd734b
>> >>     HEAD is now at 0c4d2f1 third commit
>> >>     hint: Waiting for your editor to close the file...
>> >>     error: There was a problem with the editor '"$FAKE_EDITOR"'.
>> >>     Applied autostash.
>> >>     + exit_code=1
>> >>     + test 1 -eq 0
>> >>     + test_match_signal 13 1
>> >>     + test 1 = 141
>> >>     + test 1 = 269
>> >>     + return 1
>> >>     + test 1 -gt 129
>> >>     + test 1 -eq 127
>> >>     + test 1 -eq 126
>> >>     + return 0
>> >>     + rm -f abort-editor.sh
>> >>     + echo conflicting-content
>> >>     + test_cmp expected file0
>> >>     + diff -u expected file0
>> >>     --- expected    2019-03-14 13:19:08.212215263 +0000
>> >>     +++ file0       2019-03-14 13:19:08.196215250 +0000
>> >>     @@ -1 +1 @@
>> >>     -conflicting-content
>> >>     +uncommitted-content
>> >>     error: last command exited with $?=1
>> >>     not ok 36 - autostash is saved on editor failure with conflict
>> >>
>> >> Are you able to reproduce this? And if so I suggest running the test
>> >> suite with some of the other GIT_TEST_* modes documented in
>> >> t/README. Maybe it'll turn up something else...
>> >
>> > Yep, totally can reproduce it :-(
>
> Well, isn't this exciting: we found not a bug in the built-in stash (even
> if Junio probably expected yet another one), but an fsmonitor one! Even
> better, I think this might be the bug that Alex Vandiver was chasing and
> that he talked about at the Contributors' Summit last year in Barcelona.
>
> The symptom is that cache entries are sometimes considered up to date,
> when they really are not.
>
> And the reason is that the fsmonitor has this honking global flag
> `has_run_once` (it is not really global, it is `static` to
> `refresh_fsmonitor()`, but that's the same for all practical purposes, as
> it is *not* specific to one `struct index_state`), which was kind of okay
> as long as `the_index` was used implicitly by everything.
>
> Except it was not okay when `discard_index()` (or `discard_cache()`) was
> called: in that case, the flag was not re-set. And re-set it needs to be,
> in that case, otherwise the fsmonitor is not asked which entries need to
> be updated.
>
> I saw this pretty early on in my investigation and marked it up for a
> follow-up task, wasting hours of investigation by not believing that this
> could be the culprit of the bug you described. I did not believe it
> because `git stash apply` is *spawned*, so there is not even an index that
> needs to be discarded (I thought; more on that one later).
>
> It is quite curious that this is the only occasion that our test suite
> covers that particular part of the fsmonitor...
>
> I do not really want to rely on implementation details of the rebase to verify
> that the fsmonitor is queried again (and, crucially, resets the
> FSMONITOR_VALID flag of the file(s) indicated as out of date) after the index
> is discarded and re-read.
>
> I guess the best bet is to extend `t/helper/test-read-cache.c` to optionally
> output information about a specific cache entry, then refresh it, and then run
> that test helper with fsmonitor-all (which should mark anything as modified,
> all the time). That should verify that the fix works.
>
> I did exactly that, and pushed the result to https://github.com/dscho/git
> as `fix-fsmonitor` branch.
>
> Could I ask you to test that one (it is based off of Git for Windows'
> `master`, but that should compile cleanly for you)?
>
> For now, I am a bit spent, so I'll leave the rest for tomorrow.

It fixes not just this issue, but now the whole test suite passes with
GIT_TEST_FSMONITOR, i.e. this test that's been failing for ~2 years also
works now:
https://public-inbox.org/git/87k1vwn9qe.fsf@evledraar.gmail.com/

>> In the meantime I did a build with "next" (so stash-in-C) using the
>> standard test mode and these:
>>
>>     (cd t && GIT_TEST_GETTEXT_POISON=true /usr/bin/prove $BKNG_GIT_HARNESS_OPTIONS t[0-9]*.sh)
>>     (cd t && GIT_TEST_FSMONITOR=$PWD/t7519/fsmonitor-all GIT_SKIP_TESTS="t3404.8 t3420.36" /usr/bin/prove $BKNG_GIT_HARNESS_OPTIONS t[0-9]*.sh)
>>     (cd t && GIT_TEST_SPLIT_INDEX=true /usr/bin/prove $BKNG_GIT_HARNESS_OPTIONS t[0-9]*.sh)
>>     (cd t && GIT_TEST_FULL_IN_PACK_ARRAY=true GIT_TEST_OE_SIZE=10 /usr/bin/prove $BKNG_GIT_HARNESS_OPTIONS t[0-9]*.sh)
>>     (cd t && GIT_TEST_COMMIT_GRAPH=true /usr/bin/prove $BKNG_GIT_HARNESS_OPTIONS t[0-9]*.sh)
>>     (cd t && GIT_TEST_MULTI_PACK_INDEX=true /usr/bin/prove $BKNG_GIT_HARNESS_OPTIONS t[0-9]*.sh)
>>     (cd t && GIT_TEST_STASH_USE_BUILTIN=false /usr/bin/prove $BKNG_GIT_HARNESS_OPTIONS t[0-9]*.sh)
>>     (cd t && GIT_TEST_CHECK_COLLISIONS=false /usr/bin/prove $BKNG_GIT_HARNESS_OPTIONS t[0-9]*.sh)
>>
>> Only this specific test failed.
>
> Well, good!
>
> Thank you for getting the ball rolling!
> Dscho
Ben Peart March 15, 2019, 2:23 a.m. UTC | #6
> It fixes not just this issue, but now the whole test suite passes with
> GIT_TEST_FSMONITOR, i.e. this test that's been failing for ~2 years also
> works now:
> https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpubli
> c-
> inbox.org%2Fgit%2F87k1vwn9qe.fsf%40evledraar.gmail.com%2F&amp;data=
> 02%7C01%7CBen.Peart%40microsoft.com%7C758f6272cb9741291c0208d6a8
> d650b4%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C6368820357
> 67290383&amp;sdata=H4J7xguBJVxanMfn4y0I0HsXBNYcrMS9IrwHn3aWwO
> U%3D&amp;reserved=0
> 
> >> In the meantime I did a build with "next" (so stash-in-C) using the
> >> standard test mode and these:
> >>
> >>     (cd t && GIT_TEST_GETTEXT_POISON=true /usr/bin/prove
> $BKNG_GIT_HARNESS_OPTIONS t[0-9]*.sh)
> >>     (cd t && GIT_TEST_FSMONITOR=$PWD/t7519/fsmonitor-all
> GIT_SKIP_TESTS="t3404.8 t3420.36" /usr/bin/prove
> $BKNG_GIT_HARNESS_OPTIONS t[0-9]*.sh)
> >>     (cd t && GIT_TEST_SPLIT_INDEX=true /usr/bin/prove
> $BKNG_GIT_HARNESS_OPTIONS t[0-9]*.sh)
> >>     (cd t && GIT_TEST_FULL_IN_PACK_ARRAY=true GIT_TEST_OE_SIZE=10
> /usr/bin/prove $BKNG_GIT_HARNESS_OPTIONS t[0-9]*.sh)
> >>     (cd t && GIT_TEST_COMMIT_GRAPH=true /usr/bin/prove
> $BKNG_GIT_HARNESS_OPTIONS t[0-9]*.sh)
> >>     (cd t && GIT_TEST_MULTI_PACK_INDEX=true /usr/bin/prove
> $BKNG_GIT_HARNESS_OPTIONS t[0-9]*.sh)
> >>     (cd t && GIT_TEST_STASH_USE_BUILTIN=false /usr/bin/prove
> $BKNG_GIT_HARNESS_OPTIONS t[0-9]*.sh)
> >>     (cd t && GIT_TEST_CHECK_COLLISIONS=false /usr/bin/prove
> >> $BKNG_GIT_HARNESS_OPTIONS t[0-9]*.sh)
> >>
> >> Only this specific test failed.
> >
> > Well, good!
> >
> > Thank you for getting the ball rolling!

Awesome find and fix.  Thanks for chasing this down and CC'ing me so I am aware.

> > Dscho

Patch
diff mbox series

diff --git a/.gitignore b/.gitignore
index 0d77ea5894..6ecab90ab2 100644
--- a/.gitignore
+++ b/.gitignore
@@ -162,6 +162,7 @@ 
 /git-show-ref
 /git-stage
 /git-stash
+/git-stash--helper
 /git-status
 /git-stripspace
 /git-submodule
diff --git a/Makefile b/Makefile
index 1a44c811aa..c246fc7078 100644
--- a/Makefile
+++ b/Makefile
@@ -1117,6 +1117,7 @@  BUILTIN_OBJS += builtin/shortlog.o
 BUILTIN_OBJS += builtin/show-branch.o
 BUILTIN_OBJS += builtin/show-index.o
 BUILTIN_OBJS += builtin/show-ref.o
+BUILTIN_OBJS += builtin/stash--helper.o
 BUILTIN_OBJS += builtin/stripspace.o
 BUILTIN_OBJS += builtin/submodule--helper.o
 BUILTIN_OBJS += builtin/symbolic-ref.o
diff --git a/builtin.h b/builtin.h
index 6538932e99..ff4460aff7 100644
--- a/builtin.h
+++ b/builtin.h
@@ -225,6 +225,7 @@  extern int cmd_show(int argc, const char **argv, const char *prefix);
 extern int cmd_show_branch(int argc, const char **argv, const char *prefix);
 extern int cmd_show_index(int argc, const char **argv, const char *prefix);
 extern int cmd_status(int argc, const char **argv, const char *prefix);
+extern int cmd_stash__helper(int argc, const char **argv, const char *prefix);
 extern int cmd_stripspace(int argc, const char **argv, const char *prefix);
 extern int cmd_submodule__helper(int argc, const char **argv, const char *prefix);
 extern int cmd_symbolic_ref(int argc, const char **argv, const char *prefix);
diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
new file mode 100644
index 0000000000..03511f466b
--- /dev/null
+++ b/builtin/stash--helper.c
@@ -0,0 +1,451 @@ 
+#include "builtin.h"
+#include "config.h"
+#include "parse-options.h"
+#include "refs.h"
+#include "lockfile.h"
+#include "cache-tree.h"
+#include "unpack-trees.h"
+#include "merge-recursive.h"
+#include "argv-array.h"
+#include "run-command.h"
+#include "dir.h"
+#include "rerere.h"
+
+static const char * const git_stash_helper_usage[] = {
+	N_("git stash--helper apply [--index] [-q|--quiet] [<stash>]"),
+	NULL
+};
+
+static const char * const git_stash_helper_apply_usage[] = {
+	N_("git stash--helper apply [--index] [-q|--quiet] [<stash>]"),
+	NULL
+};
+
+static const char *ref_stash = "refs/stash";
+static struct strbuf stash_index_path = STRBUF_INIT;
+
+/*
+ * w_commit is set to the commit containing the working tree
+ * b_commit is set to the base commit
+ * i_commit is set to the commit containing the index tree
+ * u_commit is set to the commit containing the untracked files tree
+ * w_tree is set to the working tree
+ * b_tree is set to the base tree
+ * i_tree is set to the index tree
+ * u_tree is set to the untracked files tree
+ */
+struct stash_info {
+	struct object_id w_commit;
+	struct object_id b_commit;
+	struct object_id i_commit;
+	struct object_id u_commit;
+	struct object_id w_tree;
+	struct object_id b_tree;
+	struct object_id i_tree;
+	struct object_id u_tree;
+	struct strbuf revision;
+	int is_stash_ref;
+	int has_u;
+};
+
+static void free_stash_info(struct stash_info *info)
+{
+	strbuf_release(&info->revision);
+}
+
+static void assert_stash_like(struct stash_info *info, const char *revision)
+{
+	if (get_oidf(&info->b_commit, "%s^1", revision) ||
+	    get_oidf(&info->w_tree, "%s:", revision) ||
+	    get_oidf(&info->b_tree, "%s^1:", revision) ||
+	    get_oidf(&info->i_tree, "%s^2:", revision))
+		die(_("'%s' is not a stash-like commit"), revision);
+}
+
+static int get_stash_info(struct stash_info *info, int argc, const char **argv)
+{
+	int ret;
+	char *end_of_rev;
+	char *expanded_ref;
+	const char *revision;
+	const char *commit = NULL;
+	struct object_id dummy;
+	struct strbuf symbolic = STRBUF_INIT;
+
+	if (argc > 1) {
+		int i;
+		struct strbuf refs_msg = STRBUF_INIT;
+
+		for (i = 0; i < argc; i++)
+			strbuf_addf(&refs_msg, " '%s'", argv[i]);
+
+		fprintf_ln(stderr, _("Too many revisions specified:%s"),
+			   refs_msg.buf);
+		strbuf_release(&refs_msg);
+
+		return -1;
+	}
+
+	if (argc == 1)
+		commit = argv[0];
+
+	strbuf_init(&info->revision, 0);
+	if (!commit) {
+		if (!ref_exists(ref_stash)) {
+			free_stash_info(info);
+			fprintf_ln(stderr, _("No stash entries found."));
+			return -1;
+		}
+
+		strbuf_addf(&info->revision, "%s@{0}", ref_stash);
+	} else if (strspn(commit, "0123456789") == strlen(commit)) {
+		strbuf_addf(&info->revision, "%s@{%s}", ref_stash, commit);
+	} else {
+		strbuf_addstr(&info->revision, commit);
+	}
+
+	revision = info->revision.buf;
+
+	if (get_oid(revision, &info->w_commit)) {
+		error(_("%s is not a valid reference"), revision);
+		free_stash_info(info);
+		return -1;
+	}
+
+	assert_stash_like(info, revision);
+
+	info->has_u = !get_oidf(&info->u_tree, "%s^3:", revision);
+
+	end_of_rev = strchrnul(revision, '@');
+	strbuf_add(&symbolic, revision, end_of_rev - revision);
+
+	ret = dwim_ref(symbolic.buf, symbolic.len, &dummy, &expanded_ref);
+	strbuf_release(&symbolic);
+	switch (ret) {
+	case 0: /* Not found, but valid ref */
+		info->is_stash_ref = 0;
+		break;
+	case 1:
+		info->is_stash_ref = !strcmp(expanded_ref, ref_stash);
+		break;
+	default: /* Invalid or ambiguous */
+		free_stash_info(info);
+	}
+
+	free(expanded_ref);
+	return !(ret == 0 || ret == 1);
+}
+
+static int reset_tree(struct object_id *i_tree, int update, int reset)
+{
+	int nr_trees = 1;
+	struct unpack_trees_options opts;
+	struct tree_desc t[MAX_UNPACK_TREES];
+	struct tree *tree;
+	struct lock_file lock_file = LOCK_INIT;
+
+	read_cache_preload(NULL);
+	if (refresh_cache(REFRESH_QUIET))
+		return -1;
+
+	hold_locked_index(&lock_file, LOCK_DIE_ON_ERROR);
+
+	memset(&opts, 0, sizeof(opts));
+
+	tree = parse_tree_indirect(i_tree);
+	if (parse_tree(tree))
+		return -1;
+
+	init_tree_desc(t, tree->buffer, tree->size);
+
+	opts.head_idx = 1;
+	opts.src_index = &the_index;
+	opts.dst_index = &the_index;
+	opts.merge = 1;
+	opts.reset = reset;
+	opts.update = update;
+	opts.fn = oneway_merge;
+
+	if (unpack_trees(nr_trees, t, &opts))
+		return -1;
+
+	if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))
+		return error(_("unable to write new index file"));
+
+	return 0;
+}
+
+static int diff_tree_binary(struct strbuf *out, struct object_id *w_commit)
+{
+	struct child_process cp = CHILD_PROCESS_INIT;
+	const char *w_commit_hex = oid_to_hex(w_commit);
+
+	/*
+	 * Diff-tree would not be very hard to replace with a native function,
+	 * however it should be done together with apply_cached.
+	 */
+	cp.git_cmd = 1;
+	argv_array_pushl(&cp.args, "diff-tree", "--binary", NULL);
+	argv_array_pushf(&cp.args, "%s^2^..%s^2", w_commit_hex, w_commit_hex);
+
+	return pipe_command(&cp, NULL, 0, out, 0, NULL, 0);
+}
+
+static int apply_cached(struct strbuf *out)
+{
+	struct child_process cp = CHILD_PROCESS_INIT;
+
+	/*
+	 * Apply currently only reads either from stdin or a file, thus
+	 * apply_all_patches would have to be updated to optionally take a
+	 * buffer.
+	 */
+	cp.git_cmd = 1;
+	argv_array_pushl(&cp.args, "apply", "--cached", NULL);
+	return pipe_command(&cp, out->buf, out->len, NULL, 0, NULL, 0);
+}
+
+static int reset_head(void)
+{
+	struct child_process cp = CHILD_PROCESS_INIT;
+
+	/*
+	 * Reset is overall quite simple, however there is no current public
+	 * API for resetting.
+	 */
+	cp.git_cmd = 1;
+	argv_array_push(&cp.args, "reset");
+
+	return run_command(&cp);
+}
+
+static int get_newly_staged(struct strbuf *out, struct object_id *c_tree)
+{
+	struct child_process cp = CHILD_PROCESS_INIT;
+	const char *c_tree_hex = oid_to_hex(c_tree);
+
+	/*
+	 * diff-index is very similar to diff-tree above, and should be
+	 * converted together with update_index.
+	 */
+	cp.git_cmd = 1;
+	argv_array_pushl(&cp.args, "diff-index", "--cached", "--name-only",
+			 "--diff-filter=A", NULL);
+	argv_array_push(&cp.args, c_tree_hex);
+	return pipe_command(&cp, NULL, 0, out, 0, NULL, 0);
+}
+
+static int update_index(struct strbuf *out)
+{
+	struct child_process cp = CHILD_PROCESS_INIT;
+
+	/*
+	 * Update-index is very complicated and may need to have a public
+	 * function exposed in order to remove this forking.
+	 */
+	cp.git_cmd = 1;
+	argv_array_pushl(&cp.args, "update-index", "--add", "--stdin", NULL);
+	return pipe_command(&cp, out->buf, out->len, NULL, 0, NULL, 0);
+}
+
+static int restore_untracked(struct object_id *u_tree)
+{
+	int res;
+	struct child_process cp = CHILD_PROCESS_INIT;
+
+	/*
+	 * We need to run restore files from a given index, but without
+	 * affecting the current index, so we use GIT_INDEX_FILE with
+	 * run_command to fork processes that will not interfere.
+	 */
+	cp.git_cmd = 1;
+	argv_array_push(&cp.args, "read-tree");
+	argv_array_push(&cp.args, oid_to_hex(u_tree));
+	argv_array_pushf(&cp.env_array, "GIT_INDEX_FILE=%s",
+			 stash_index_path.buf);
+	if (run_command(&cp)) {
+		remove_path(stash_index_path.buf);
+		return -1;
+	}
+
+	child_process_init(&cp);
+	cp.git_cmd = 1;
+	argv_array_pushl(&cp.args, "checkout-index", "--all", NULL);
+	argv_array_pushf(&cp.env_array, "GIT_INDEX_FILE=%s",
+			 stash_index_path.buf);
+
+	res = run_command(&cp);
+	remove_path(stash_index_path.buf);
+	return res;
+}
+
+static int do_apply_stash(const char *prefix, struct stash_info *info,
+			  int index, int quiet)
+{
+	int ret;
+	int has_index = index;
+	struct merge_options o;
+	struct object_id c_tree;
+	struct object_id index_tree;
+	struct commit *result;
+	const struct object_id *bases[1];
+
+	read_cache_preload(NULL);
+	if (refresh_cache(REFRESH_QUIET))
+		return -1;
+
+	if (write_cache_as_tree(&c_tree, 0, NULL))
+		return error(_("cannot apply a stash in the middle of a merge"));
+
+	if (index) {
+		if (oideq(&info->b_tree, &info->i_tree) ||
+		    oideq(&c_tree, &info->i_tree)) {
+			has_index = 0;
+		} else {
+			struct strbuf out = STRBUF_INIT;
+
+			if (diff_tree_binary(&out, &info->w_commit)) {
+				strbuf_release(&out);
+				return error(_("could not generate diff %s^!."),
+					     oid_to_hex(&info->w_commit));
+			}
+
+			ret = apply_cached(&out);
+			strbuf_release(&out);
+			if (ret)
+				return error(_("conflicts in index."
+					       "Try without --index."));
+
+			discard_cache();
+			read_cache();
+			if (write_cache_as_tree(&index_tree, 0, NULL))
+				return error(_("could not save index tree"));
+
+			reset_head();
+		}
+	}
+
+	if (info->has_u && restore_untracked(&info->u_tree))
+		return error(_("could not restore untracked files from stash"));
+
+	init_merge_options(&o);
+
+	o.branch1 = "Updated upstream";
+	o.branch2 = "Stashed changes";
+
+	if (oideq(&info->b_tree, &c_tree))
+		o.branch1 = "Version stash was based on";
+
+	if (quiet)
+		o.verbosity = 0;
+
+	if (o.verbosity >= 3)
+		printf_ln(_("Merging %s with %s"), o.branch1, o.branch2);
+
+	bases[0] = &info->b_tree;
+
+	ret = merge_recursive_generic(&o, &c_tree, &info->w_tree, 1, bases,
+				      &result);
+	if (ret) {
+		rerere(0);
+
+		if (index)
+			fprintf_ln(stderr, _("Index was not unstashed."));
+
+		return ret;
+	}
+
+	if (has_index) {
+		if (reset_tree(&index_tree, 0, 0))
+			return -1;
+	} else {
+		struct strbuf out = STRBUF_INIT;
+
+		if (get_newly_staged(&out, &c_tree)) {
+			strbuf_release(&out);
+			return -1;
+		}
+
+		if (reset_tree(&c_tree, 0, 1)) {
+			strbuf_release(&out);
+			return -1;
+		}
+
+		ret = update_index(&out);
+		strbuf_release(&out);
+		if (ret)
+			return -1;
+
+		discard_cache();
+	}
+
+	if (quiet) {
+		if (refresh_cache(REFRESH_QUIET))
+			warning("could not refresh index");
+	} else {
+		struct child_process cp = CHILD_PROCESS_INIT;
+
+		/*
+		 * Status is quite simple and could be replaced with calls to
+		 * wt_status in the future, but it adds complexities which may
+		 * require more tests.
+		 */
+		cp.git_cmd = 1;
+		cp.dir = prefix;
+		argv_array_push(&cp.args, "status");
+		run_command(&cp);
+	}
+
+	return 0;
+}
+
+static int apply_stash(int argc, const char **argv, const char *prefix)
+{
+	int ret;
+	int quiet = 0;
+	int index = 0;
+	struct stash_info info;
+	struct option options[] = {
+		OPT__QUIET(&quiet, N_("be quiet, only report errors")),
+		OPT_BOOL(0, "index", &index,
+			 N_("attempt to recreate the index")),
+		OPT_END()
+	};
+
+	argc = parse_options(argc, argv, prefix, options,
+			     git_stash_helper_apply_usage, 0);
+
+	if (get_stash_info(&info, argc, argv))
+		return -1;
+
+	ret = do_apply_stash(prefix, &info, index, quiet);
+	free_stash_info(&info);
+	return ret;
+}
+
+int cmd_stash__helper(int argc, const char **argv, const char *prefix)
+{
+	pid_t pid = getpid();
+	const char *index_file;
+
+	struct option options[] = {
+		OPT_END()
+	};
+
+	git_config(git_default_config, NULL);
+
+	argc = parse_options(argc, argv, prefix, options, git_stash_helper_usage,
+			     PARSE_OPT_KEEP_UNKNOWN | PARSE_OPT_KEEP_DASHDASH);
+
+	index_file = get_index_file();
+	strbuf_addf(&stash_index_path, "%s.stash.%" PRIuMAX, index_file,
+		    (uintmax_t)pid);
+
+	if (argc < 1)
+		usage_with_options(git_stash_helper_usage, options);
+	if (!strcmp(argv[0], "apply"))
+		return !!apply_stash(argc, argv, prefix);
+
+	usage_msg_opt(xstrfmt(_("unknown subcommand: %s"), argv[0]),
+		      git_stash_helper_usage, options);
+}
diff --git a/git-stash.sh b/git-stash.sh
index 789ce2f41d..366a082853 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -583,76 +583,11 @@  assert_stash_ref() {
 }
 
 apply_stash () {
-
-	assert_stash_like "$@"
-
-	git update-index -q --refresh || die "$(gettext "unable to refresh index")"
-
-	# current index state
-	c_tree=$(git write-tree) ||
-		die "$(gettext "Cannot apply a stash in the middle of a merge")"
-
-	unstashed_index_tree=
-	if test -n "$INDEX_OPTION" && test "$b_tree" != "$i_tree" &&
-			test "$c_tree" != "$i_tree"
-	then
-		git diff-tree --binary $s^2^..$s^2 | git apply --cached
-		test $? -ne 0 &&
-			die "$(gettext "Conflicts in index. Try without --index.")"
-		unstashed_index_tree=$(git write-tree) ||
-			die "$(gettext "Could not save index tree")"
-		git reset
-	fi
-
-	if test -n "$u_tree"
-	then
-		GIT_INDEX_FILE="$TMPindex" git read-tree "$u_tree" &&
-		GIT_INDEX_FILE="$TMPindex" git checkout-index --all &&
-		rm -f "$TMPindex" ||
-		die "$(gettext "Could not restore untracked files from stash entry")"
-	fi
-
-	eval "
-		GITHEAD_$w_tree='Stashed changes' &&
-		GITHEAD_$c_tree='Updated upstream' &&
-		GITHEAD_$b_tree='Version stash was based on' &&
-		export GITHEAD_$w_tree GITHEAD_$c_tree GITHEAD_$b_tree
-	"
-
-	if test -n "$GIT_QUIET"
-	then
-		GIT_MERGE_VERBOSITY=0 && export GIT_MERGE_VERBOSITY
-	fi
-	if git merge-recursive $b_tree -- $c_tree $w_tree
-	then
-		# No conflict
-		if test -n "$unstashed_index_tree"
-		then
-			git read-tree "$unstashed_index_tree"
-		else
-			a="$TMP-added" &&
-			git diff-index --cached --name-only --diff-filter=A $c_tree >"$a" &&
-			git read-tree --reset $c_tree &&
-			git update-index --add --stdin <"$a" ||
-				die "$(gettext "Cannot unstage modified files")"
-			rm -f "$a"
-		fi
-		squelch=
-		if test -n "$GIT_QUIET"
-		then
-			squelch='>/dev/null 2>&1'
-		fi
-		(cd "$START_DIR" && eval "git status $squelch") || :
-	else
-		# Merge conflict; keep the exit status from merge-recursive
-		status=$?
-		git rerere
-		if test -n "$INDEX_OPTION"
-		then
-			gettextln "Index was not unstashed." >&2
-		fi
-		exit $status
-	fi
+	cd "$START_DIR"
+	git stash--helper apply "$@"
+	res=$?
+	cd_to_toplevel
+	return $res
 }
 
 pop_stash() {
@@ -730,7 +665,8 @@  push)
 	;;
 apply)
 	shift
-	apply_stash "$@"
+	cd "$START_DIR"
+	git stash--helper apply "$@"
 	;;
 clear)
 	shift
diff --git a/git.c b/git.c
index 2f604a41ea..76ee02802e 100644
--- a/git.c
+++ b/git.c
@@ -554,6 +554,7 @@  static struct cmd_struct commands[] = {
 	{ "show-index", cmd_show_index },
 	{ "show-ref", cmd_show_ref, RUN_SETUP },
 	{ "stage", cmd_add, RUN_SETUP | NEED_WORK_TREE },
+	{ "stash--helper", cmd_stash__helper, RUN_SETUP | NEED_WORK_TREE },
 	{ "status", cmd_status, RUN_SETUP | NEED_WORK_TREE },
 	{ "stripspace", cmd_stripspace },
 	{ "submodule--helper", cmd_submodule__helper, RUN_SETUP | SUPPORT_SUPER_PREFIX | NO_PARSEOPT },