Re: fatal error when diffing changed symlinks

[Johannes Schindelin]

Hi Junio,

On Tue, 7 Mar 2017, Junio C Hamano wrote:

> Johannes Schindelin  writes:
> 
> >> > 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.
> 
> Friendly ping, if only to make sure that we can keep a piece of this
> thread in the more "recent" pile.
> 
> If you have other topics you need to perfect, I think it is OK to
> postpone the fix on this topic a bit longer, but I'd hate to ship
> two releases with a known breakage without an attempt to fix it, so
> if you are otherwise occupied, I may encourage others (including me)
> to take a look at this.  The new "difftool" also has a reported
> regression somebody else expressed willingness to work on, which is
> sort of blocked by everybody else not knowing the timeline on this
> one.  cf. <20170303212836.GB13790@arch-attack.localdomain>
> 
> A patch series would be very welcome, but "Please go ahead if
> somebody else has time, and I'll help reviewing" would be also
> good.

Unfortunately, the obvious fix was not enough. My current work in progress
is in the `difftool-null-sha1` branch on https://gihub.com/dscho/git. I
still have a few other things to tend to, but hope to come back to the
difftool before the end of the week.

Ciao,
Johannes

Re: fatal error when diffing changed symlinks

[Junio C Hamano]

Johannes Schindelin  writes:

>> > 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.

Friendly ping, if only to make sure that we can keep a piece of this
thread in the more "recent" pile.

If you have other topics you need to perfect, I think it is OK to
postpone the fix on this topic a bit longer, but I'd hate to ship
two releases with a known breakage without an attempt to fix it, so
if you are otherwise occupied, I may encourage others (including me)
to take a look at this.  The new "difftool" also has a reported
regression somebody else expressed willingness to work on, which is
sort of blocked by everybody else not knowing the timeline on this
one.  cf. <20170303212836.GB13790@arch-attack.localdomain>

A patch series would be very welcome, but "Please go ahead if
somebody else has time, and I'll help reviewing" would be also
good.

Thanks.

Re: fatal error when diffing changed symlinks

[Johannes Schindelin]

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:
> 
> > > 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, , );
> > add_left_or_right(, src_path, content, 0);
> > free(content);
> > }
> > 
> > if (S_ISLNK(rmode)) {
> > char *content = read_sha1_file(roid.hash, , );
> > add_left_or_right(, 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

Re: fatal error when diffing changed symlinks

[Jeff King]

On Fri, Feb 24, 2017 at 11:51:22AM -0800, Junio C Hamano wrote:

> > 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, , );
>   add_left_or_right(, src_path, content, 0);
>   free(content);
>   }
> 
>   if (S_ISLNK(rmode)) {
>   char *content = read_sha1_file(roid.hash, , );
>   add_left_or_right(, 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

Re: fatal error when diffing changed symlinks

[Junio C Hamano]

Junio C Hamano  writes:

>> 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, , );
add_left_or_right(, src_path, content, 0);
free(content);
}

if (S_ISLNK(rmode)) {
char *content = read_sha1_file(roid.hash, , );
add_left_or_right(, 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).

Re: fatal error when diffing changed symlinks

[Junio C Hamano]

Christophe Macabiau  writes:

> with the commands below, you will get :
>
>> fatal: bad object 
>> show : command returned error: 128
>>
>
> I am using version 2.5.5 fedora 23
>
> 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.  As either way is
bad, I do not particularly think the rewritten one needs to be
reverted (it would give "fatal: bad object" then, and would not fix
your problem), but it needs to be looked at.

Thanks for a report.