* [PATCH 0/1] built-in stash: fix segmentation fault when files were added with intent
@ 2019-01-18 9:50 Johannes Schindelin via GitGitGadget
2019-01-18 9:50 ` [PATCH 1/1] " Matthew Kraai via GitGitGadget
0 siblings, 1 reply; 6+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2019-01-18 9:50 UTC (permalink / raw)
To: git; +Cc: Paul-Sebastian Ungureanu, Junio C Hamano
Git for Windows offered the built-in stash early: in v2.19.0 it was offered
as an experimental option, and in v2.20.0 it was enabled by default.
One corner case was identified
[https://github.com/git-for-windows/git/issues/2006] and fixed
[https://github.com/git-for-windows/git/pull/2008] in the meantime, and this
contribution brings it to the Git mailing list (with a commit message that
was "lightly edited for clarity", as they say).
This patch applies on top of ps/stash-in-c.
Granted, it fixes a regression in that patch series, but as Paul is busy
with University, I would suggest accepting this bug fix on top, just this
time, as if we had stash-in-c already in next.
Matthew Kraai (1):
stash: fix segmentation fault when files were added with intent
builtin/stash.c | 3 ++-
t/t3903-stash.sh | 8 ++++++++
2 files changed, 10 insertions(+), 1 deletion(-)
base-commit: bec65d5b783ef5ce4c655c26ad8f25c04b001dd1
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-110%2Fdscho%2Fstash-ita-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-110/dscho/stash-ita-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/110
--
gitgitgadget
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/1] stash: fix segmentation fault when files were added with intent
2019-01-18 9:50 [PATCH 0/1] built-in stash: fix segmentation fault when files were added with intent Johannes Schindelin via GitGitGadget
@ 2019-01-18 9:50 ` Matthew Kraai via GitGitGadget
2019-01-18 20:42 ` Junio C Hamano
0 siblings, 1 reply; 6+ messages in thread
From: Matthew Kraai via GitGitGadget @ 2019-01-18 9:50 UTC (permalink / raw)
To: git; +Cc: Paul-Sebastian Ungureanu, Junio C Hamano, Matthew Kraai
From: Matthew Kraai <mkraai@its.jnj.com>
After `git add -N <file>`, the index is in a special state. A state for
which the built-in stash was not prepared, as it failed to initialize
the `rev` structure in that case before using `&rev.pending`.
Detailed explanation: If `reset_tree()` returns a non-zero value,
`stash_working_tree()` calls `object_array_clear()` with `&rev.pending`.
If `rev` is not initialized, this causes a segmentation fault.
Prevent this by initializing `rev` before calling `reset_tree()`.
This fixes https://github.com/git-for-windows/git/issues/2006.
[jes: modified the commit message in preparation for sending this patch
to the Git mailing list.]
Signed-off-by: Matthew Kraai <mkraai@its.jnj.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
builtin/stash.c | 3 ++-
t/t3903-stash.sh | 8 ++++++++
2 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/builtin/stash.c b/builtin/stash.c
index 3ee8a41cda..74e6ff62b5 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -1048,6 +1048,8 @@ static int stash_working_tree(struct stash_info *info, struct pathspec ps)
struct strbuf diff_output = STRBUF_INIT;
struct index_state istate = { NULL };
+ init_revisions(&rev, NULL);
+
set_alternate_index_output(stash_index_path.buf);
if (reset_tree(&info->i_tree, 0, 0)) {
ret = -1;
@@ -1055,7 +1057,6 @@ static int stash_working_tree(struct stash_info *info, struct pathspec ps)
}
set_alternate_index_output(NULL);
- init_revisions(&rev, NULL);
rev.prune_data = ps;
rev.diffopt.output_format = DIFF_FORMAT_CALLBACK;
rev.diffopt.format_callback = add_diff_to_buf;
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index b67d7a1120..7dfa3a8038 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -287,6 +287,14 @@ test_expect_success 'stash an added file' '
test new = "$(cat file3)"
'
+test_expect_success 'stash --intent-to-add file' '
+ git reset --hard &&
+ echo new >file4 &&
+ git add --intent-to-add file4 &&
+ test_when_finished "git rm -f file4" &&
+ test_must_fail git stash
+'
+
test_expect_success 'stash rm then recreate' '
git reset --hard &&
git rm file &&
--
gitgitgadget
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] stash: fix segmentation fault when files were added with intent
2019-01-18 9:50 ` [PATCH 1/1] " Matthew Kraai via GitGitGadget
@ 2019-01-18 20:42 ` Junio C Hamano
2019-01-21 15:14 ` Johannes Schindelin
0 siblings, 1 reply; 6+ messages in thread
From: Junio C Hamano @ 2019-01-18 20:42 UTC (permalink / raw)
To: Matthew Kraai via GitGitGadget
Cc: git, Paul-Sebastian Ungureanu, Matthew Kraai
"Matthew Kraai via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Matthew Kraai <mkraai@its.jnj.com>
>
> After `git add -N <file>`, the index is in a special state. A state for
> which the built-in stash was not prepared, as it failed to initialize
> the `rev` structure in that case before using `&rev.pending`.
>
> Detailed explanation: If `reset_tree()` returns a non-zero value,
> `stash_working_tree()` calls `object_array_clear()` with `&rev.pending`.
> If `rev` is not initialized, this causes a segmentation fault.
It is a bit strange that the paragraph for "detailed explanation" is
shorter than the paragraph it attempts to clarify.
Dropping those two words "Detailed explanation:" easily fixes
awkwardness ;-).
> +test_expect_success 'stash --intent-to-add file' '
> + git reset --hard &&
> + echo new >file4 &&
> + git add --intent-to-add file4 &&
> + test_when_finished "git rm -f file4" &&
> + test_must_fail git stash
> +'
This still must fail because an index with an I-T-A cannot be
included in a stash, but test_must_fail will make sure that the
command does not suffer an uncontrolled crash. Good.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] stash: fix segmentation fault when files were added with intent
2019-01-18 20:42 ` Junio C Hamano
@ 2019-01-21 15:14 ` Johannes Schindelin
2019-01-22 18:33 ` Junio C Hamano
0 siblings, 1 reply; 6+ messages in thread
From: Johannes Schindelin @ 2019-01-21 15:14 UTC (permalink / raw)
To: Junio C Hamano
Cc: Matthew Kraai via GitGitGadget, git, Paul-Sebastian Ungureanu,
Matthew Kraai
Hi Junio,
On Fri, 18 Jan 2019, Junio C Hamano wrote:
> "Matthew Kraai via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > From: Matthew Kraai <mkraai@its.jnj.com>
> >
> > After `git add -N <file>`, the index is in a special state. A state for
> > which the built-in stash was not prepared, as it failed to initialize
> > the `rev` structure in that case before using `&rev.pending`.
> >
> > Detailed explanation: If `reset_tree()` returns a non-zero value,
> > `stash_working_tree()` calls `object_array_clear()` with `&rev.pending`.
> > If `rev` is not initialized, this causes a segmentation fault.
>
> It is a bit strange that the paragraph for "detailed explanation" is
> shorter than the paragraph it attempts to clarify.
>
> Dropping those two words "Detailed explanation:" easily fixes
> awkwardness ;-).
If you must.
For me, it is not the quantity, but the quality of the words that makes it
a detailed explanation. You see, as a mathematician, I can give you a very
condensed, super detailed, complicated proof for many a lemma which is
substantially shorter than the understandable, English explanation that
I would want to give to non-mathematicians.
Likewise, if you take a step back, and try to forget for a moment that you
are very familiar with Git's source code, you will without any doubt
*have* to admit that the detailed explanation requires a *lot* of
knowledge already, while the paragraph before that does not.
So if you find that awkward, I respectfully disagree. And if you insist on
removing those "two words" (to make the paragraph *even* shorter, which is
apparently something you took exception with), I have no tools to stop
you.
Just let it be known that it is against the wish of the author of those
lines.
> > +test_expect_success 'stash --intent-to-add file' '
> > + git reset --hard &&
> > + echo new >file4 &&
> > + git add --intent-to-add file4 &&
> > + test_when_finished "git rm -f file4" &&
> > + test_must_fail git stash
> > +'
>
> This still must fail because an index with an I-T-A cannot be
> included in a stash, but test_must_fail will make sure that the
> command does not suffer an uncontrolled crash. Good.
Indeed. And Matthew even adopted your preferred strategy of combining the
demonstration of the bug with the fix. In the meantime, I have found the
totally intuitive (and equally documented) command line that I can use
when I want to see whether a given branch is buggy and when I cannot
simply `git cherry-pick <commit-demonstrating-a-bug>`:
git cherry-pick <commit-fixing-the-bug-and-adding-a-test>
git checkout HEAD^ -- :^/t/
Ciao,
Johannes
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] stash: fix segmentation fault when files were added with intent
2019-01-21 15:14 ` Johannes Schindelin
@ 2019-01-22 18:33 ` Junio C Hamano
2019-01-23 11:38 ` Johannes Schindelin
0 siblings, 1 reply; 6+ messages in thread
From: Junio C Hamano @ 2019-01-22 18:33 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Matthew Kraai via GitGitGadget, git, Paul-Sebastian Ungureanu,
Matthew Kraai
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> when I want to see whether a given branch is buggy and when I cannot
> simply `git cherry-pick <commit-demonstrating-a-bug>`:
>
> git cherry-pick <commit-fixing-the-bug-and-adding-a-test>
> git checkout HEAD^ -- :^/t/
Yup. It is easy to just apply the t/ part to grab the test update
to see breakage (which I already said when I told you to have a fix
and test protecting the future breakage of the fix in a single patch
long time ago).
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] stash: fix segmentation fault when files were added with intent
2019-01-22 18:33 ` Junio C Hamano
@ 2019-01-23 11:38 ` Johannes Schindelin
0 siblings, 0 replies; 6+ messages in thread
From: Johannes Schindelin @ 2019-01-23 11:38 UTC (permalink / raw)
To: Junio C Hamano
Cc: Matthew Kraai via GitGitGadget, git, Paul-Sebastian Ungureanu,
Matthew Kraai
Hi Junio,
On Tue, 22 Jan 2019, Junio C Hamano wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> > when I want to see whether a given branch is buggy and when I cannot
> > simply `git cherry-pick <commit-demonstrating-a-bug>`:
> >
> > git cherry-pick <commit-fixing-the-bug-and-adding-a-test>
> > git checkout HEAD^ -- :^/t/
>
> Yup. It is easy to just apply the t/ part to grab the test update
> to see breakage (which I already said when I told you to have a fix
> and test protecting the future breakage of the fix in a single patch
> long time ago).
Sorry, that was not my point.
My point is that
git cherry-pick <commit-fixing-the-bug-and-adding-a-test>
git checkout HEAD^ -- :^/t/
is *ridiculously* less intuitive than
git cherry-pick <commit-demonstrating-a-bug>
and I would rather you stop promoting the former over the latter. After
all, Git's purpose in life is to make things easier and quicker and less
error-prone, rathern than slower, more complicated and unintuitive.
And I am sure you agree with me on that goal, so I do not understand why
you promote that a bit more.
Ciao,
Dscho
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-01-23 11:38 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-18 9:50 [PATCH 0/1] built-in stash: fix segmentation fault when files were added with intent Johannes Schindelin via GitGitGadget
2019-01-18 9:50 ` [PATCH 1/1] " Matthew Kraai via GitGitGadget
2019-01-18 20:42 ` Junio C Hamano
2019-01-21 15:14 ` Johannes Schindelin
2019-01-22 18:33 ` Junio C Hamano
2019-01-23 11:38 ` Johannes Schindelin
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).