Junio C Hamano [off-list ref] writes:
quoted
cd /tmp
mkdir a
cd a
git init
touch b
ln -s b c
git add .
git commit -m 'first'
touch d
rm c
ln -s d c
git difftool --dir-diff
A slightly worse is that the upcoming Git will ship with a rewritten
"difftool" that makes the above sequence segfault.
The culprit seems to be these lines in run_dir_diff():
if (S_ISLNK(lmode)) {
char *content = read_sha1_file(loid.hash, &type, &size);
add_left_or_right(&symlinks2, src_path, content, 0);
free(content);
}
if (S_ISLNK(rmode)) {
char *content = read_sha1_file(roid.hash, &type, &size);
add_left_or_right(&symlinks2, dst_path, content, 1);
free(content);
}
When viewing a working tree file, oid.hash could be 0{40} and
read_sha1_file() is not the right function to use to obtain the
contents.
Both of these two need to pay attention to 0{40}, I think, as the
user may be running "difftool -R --dir-diff" in which case the
working tree would appear in the left hand side instead.
I didn't follow the codepath for regular files closely, but the code
that follows the above excerpt does quite different things to lstate
and rstate, which makes me suspect that the code is not prepared to
see "-R"(everse) diff (and I further suspect that these issues were
inherited from the original scripted Porcelain).
On Fri, Feb 24, 2017 at 11:51:22AM -0800, Junio C Hamano wrote:
quoted
A slightly worse is that the upcoming Git will ship with a rewritten
"difftool" that makes the above sequence segfault.
The culprit seems to be these lines in run_dir_diff():
if (S_ISLNK(lmode)) {
char *content = read_sha1_file(loid.hash, &type, &size);
add_left_or_right(&symlinks2, src_path, content, 0);
free(content);
}
if (S_ISLNK(rmode)) {
char *content = read_sha1_file(roid.hash, &type, &size);
add_left_or_right(&symlinks2, dst_path, content, 1);
free(content);
}
When viewing a working tree file, oid.hash could be 0{40} and
read_sha1_file() is not the right function to use to obtain the
contents.
Both of these two need to pay attention to 0{40}, I think, as the
user may be running "difftool -R --dir-diff" in which case the
working tree would appear in the left hand side instead.
As a side note, I think even outside of 0{40}, this should be checking
the return value of read_sha1_file(). A corrupted repo should die(), not
segfault.
-Peff
Hi Peff & Junio,
On Fri, 24 Feb 2017, Jeff King wrote:
On Fri, Feb 24, 2017 at 11:51:22AM -0800, Junio C Hamano wrote:
quoted
quoted
A slightly worse is that the upcoming Git will ship with a rewritten
"difftool" that makes the above sequence segfault.
The culprit seems to be these lines in run_dir_diff():
if (S_ISLNK(lmode)) {
char *content = read_sha1_file(loid.hash, &type, &size);
add_left_or_right(&symlinks2, src_path, content, 0);
free(content);
}
if (S_ISLNK(rmode)) {
char *content = read_sha1_file(roid.hash, &type, &size);
add_left_or_right(&symlinks2, dst_path, content, 1);
free(content);
}
When viewing a working tree file, oid.hash could be 0{40} and
read_sha1_file() is not the right function to use to obtain the
contents.
Both of these two need to pay attention to 0{40}, I think, as the
user may be running "difftool -R --dir-diff" in which case the
working tree would appear in the left hand side instead.
As a side note, I think even outside of 0{40}, this should be checking
the return value of read_sha1_file(). A corrupted repo should die(), not
segfault.
I agree. I am on it.
Ciao,
Dscho