fatal error when diffing changed symlinks

> with the commands below, you will get :
>
>> fatal: bad object 0000000000000000000000000000000000000000
>> show 0000000000000000000000000000000000000000: 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

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

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

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

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

> Johannes Schindelin <Johannes.Schindelin@gmx.de> 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.

> Detect the null object ID for symlinks in dir-diff so that difftool can
> prepare temporary files that matches how git handles symlinks.
>
> Previously, a null object ID would crash difftool.  We now detect null
> object IDs and write the symlink's content into the temporary symlink
> stand-in file.
>
> Original-patch-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> Signed-off-by: David Aguilar <davvid@gmail.com>
> ---

> +	 * into the checkout state's path.
> +	 */
> +	struct strbuf path = STRBUF_INIT;
> +	struct strbuf link = STRBUF_INIT;
> +
> +	int ok = 0;
> +
> +	if (strbuf_readlink(&link, ce->name, ce_namelen(ce)) == 0) {
> +		strbuf_add(&path, state->base_dir, state->base_dir_len);
> +		strbuf_add(&path, ce->name, ce_namelen(ce));
> +
> +		write_file_buf(path.buf, link.buf, link.len);

> David Aguilar <davvid@gmail.com> writes:
> 
> > Detect the null object ID for symlinks in dir-diff so that difftool
> > can prepare temporary files that matches how git handles symlinks.
> >
> > Previously, a null object ID would crash difftool.  We now detect null
> > object IDs and write the symlink's content into the temporary symlink
> > stand-in file.
> >
> > Original-patch-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > Signed-off-by: David Aguilar <davvid@gmail.com>
> > ---
> 
> I would have appreciated (and I suspect other reviewers would, too)
> a bit of back-story wrt how "Original-patch-by" resulted in this
> patch after the three-dashes line.  It is perfectly fine if you two
> coordinated privately; I mostly wanted to hear something like "Dscho
> has been working on this and I asked him if it is OK to take over
> his WIP to produce a quick-fix we can ship on the maint branch, here
> is the result of that collaboration."  IOW, the person who is named
> as the original author is fine to be named like so (I care only
> because I do not think we saw the "original" here on the list).

> David Aguilar <davvid@gmail.com> writes:
> 
> > +static int create_symlink_file(struct cache_entry* ce, struct checkout* state)
> 
> Asterisk sticks to variable, not type.

> > +	 * into the checkout state's path.
> > +	 */
> > +	struct strbuf path = STRBUF_INIT;
> > +	struct strbuf link = STRBUF_INIT;
> > +
> > +	int ok = 0;
> > +
> > +	if (strbuf_readlink(&link, ce->name, ce_namelen(ce)) == 0) {
> > +		strbuf_add(&path, state->base_dir, state->base_dir_len);
> > +		strbuf_add(&path, ce->name, ce_namelen(ce));
> > +
> > +		write_file_buf(path.buf, link.buf, link.len);
> 
> This does "write content into symlink stand-in file", but why?

> Also, I am not sure if strbuf_readlink() can unconditionally used
> here.  On a filesystem without symbolic link, the working tree
> entity that corresponds to the ce that represents a symlink is a
> stand-in regular file, so shouldn't we be opening it as a regular
> file and reading its contents in that case?

>> > +	if (strbuf_readlink(&link, ce->name, ce_namelen(ce)) == 0) {
>> > +		strbuf_add(&path, state->base_dir, state->base_dir_len);
>> > +		strbuf_add(&path, ce->name, ce_namelen(ce));
>> > +
>> > +		write_file_buf(path.buf, link.buf, link.len);
>> 
>> This does "write content into symlink stand-in file", but why?
>
> From the commit message:
>
> 	> Detect the null object ID for symlinks in dir-diff so that
> 	> difftool can prepare temporary files that matches how git
> 	> handles symlinks.

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> >> > +	if (strbuf_readlink(&link, ce->name, ce_namelen(ce)) == 0) {
> >> > +		strbuf_add(&path, state->base_dir, state->base_dir_len);
> >> > +		strbuf_add(&path, ce->name, ce_namelen(ce));
> >> > +
> >> > +		write_file_buf(path.buf, link.buf, link.len);
> >> 
> >> This does "write content into symlink stand-in file", but why?
> >
> > From the commit message:
> >
> > 	> Detect the null object ID for symlinks in dir-diff so that
> > 	> difftool can prepare temporary files that matches how git
> > 	> handles symlinks.
> 
> Yes I read what the proposed log message said, and noticed that
> symbolic link is _always_ written as a regular file.
> 
> I was questioning why.  I know Git falls back to such behaviour when
> the filesystem does not support symbolic links.  "Why do so
> unconditionally, even when the filesystem does?" was the question.
> 
> > The obvious connection: when core.symlinks = false, Git already falls back
> > to writing plain files with the link target as contents. This function
> > does the same, for the same motivation: it is the best we can do in this
> > case.
> 
> And that "obvious connection" does not answer the question.

> I think you are right, we cannot simply call strbuf_readlink(), we would
> have to check the core_symlinks variable to maybe read the file contents
> instead.
>
> But then, it may not be appropriate to read the worktree to begin
> with...
> see my reply to the patch that I will send out in a couple of minutes.

> > > Detect the null object ID for symlinks in dir-diff so that
> > > difftool can prepare temporary files that matches how git
> > > handles symlinks.
>
> Yes I read what the proposed log message said, and noticed that
> symbolic link is _always_ written as a regular file.
>
> I was questioning why.  I know Git falls back to such behaviour when
> the filesystem does not support symbolic links.  "Why do so
> unconditionally, even when the filesystem does?" was the question.

> On this part I didn't comment in my previous message, but what is
> the implication of omitting add-left-or-right and not registering
> this symbolic link modified in the working tree to the symlinks2
> table?
>
> I am wondering if these should be more like
>
>         if (S_ISLNK(lmode) {
>                 char *content = get_symlink(src_path, &loid);
>                 add_left_or_right(&symlinks2, src_path, content, 0);
>                 free(content);
>         }
>
> with get_symlink() helper that does
>
>  - if the object name is not 0{40}, read from the object store
>
>  - if the object name is 0{40}, that means we need to read the real
>    contents from the working tree file, so do the "readlink(2) if
>    symbolic link is supported, otherwise open/read/close the stub
>    file sitting there" thing.
>
> Similary to the right hand side tree.

> Reading the code again, the point of add_left_or_right()
> is to populate the worktree (done later in the loop) with
> the stuff we read from Git.  Thus, if we changed just this
> section to call get_symlink() then we should not even try
> to checkout any symlink entries in !use_wt_file(...)
> block where checkout_entry() / create_symlink_file() are called.
> 
> Since the symlinks2 hashmap already populates the worktree
> then that code should instead simply skip symlinks.

> One minor thing I noticed is that I had to use "echo -n"
> for the stuff coming out of strbuf_readlink(), and
> plain "echo" for entries that come in via read_sha1_file()
> content passed to add_left_or_right().
>
> That suggests that maybe I should append a newline to the
> output from strbuf_readlink() so that it matches Git.
> Does that sound right?  Does Git store symlink entries
> with a terminating newline?

>> +	struct strbuf path = STRBUF_INIT;
>> +	struct strbuf link = STRBUF_INIT;
>> +
>> +	int ok = 0;
>> +
>> +	if (strbuf_readlink(&link, ce->name, ce_namelen(ce)) == 0) {
>> +		strbuf_add(&path, state->base_dir, state->base_dir_len);
>> +		strbuf_add(&path, ce->name, ce_namelen(ce));
>> +
>> +		write_file_buf(path.buf, link.buf, link.len);
>
> This does "write content into symlink stand-in file", but why?  A
> symbolic link that is not changed on the right-hand side of the
> comparison (i.e. S_ISLNK(rmode) && !is_null_oid(&roid)) would go to
> checkout_entry() which
>
>  - creates a symbolic link, on a filesystem that supports symlink; or
>  - writes the stand-in file on a filesystem that does not.
>
> which is the right thing.  It seems that the other side of "if (!use_wt_file())"
> if/elseif/... cascade also does the right thing manually.
>
> Shouldn't this helper also do the same (i.e. create symlink and fall
> back to stand-in)?
>
> Also, I am not sure if strbuf_readlink() can unconditionally used
> here.  On a filesystem without symbolic link, the working tree
> entity that corresponds to the ce that represents a symlink is a
> stand-in regular file, so shouldn't we be opening it as a regular
> file and reading its contents in that case?

> -		if (S_ISLNK(lmode)) {
> +		if (S_ISLNK(lmode) && !is_null_oid(&loid)) {
>  			char *content = read_sha1_file(loid.hash, &type, &size);
>  			add_left_or_right(&symlinks2, src_path, content, 0);
>  			free(content);
>  		}
>  
> -		if (S_ISLNK(rmode)) {
> +		if (S_ISLNK(rmode) && !is_null_oid(&roid)) {
>  			char *content = read_sha1_file(roid.hash, &type, &size);
>  			add_left_or_right(&symlinks2, dst_path, content, 1);
>  			free(content);

> Previously, a null object ID would crash difftool.  We now detect null
> object IDs and write the symlink's content into the temporary symlink
> stand-in file.
> 
> Original-patch-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> Signed-off-by: David Aguilar <davvid@gmail.com>
> ---
> diff --git a/builtin/difftool.c b/builtin/difftool.c
> index d13350ce83..6c20e20b45 100644
> --- a/builtin/difftool.c
> +++ b/builtin/difftool.c
> @@ -254,6 +254,31 @@ static int ensure_leading_directories(char *path)
>  	}
>  }
>  
> +static int create_symlink_file(struct cache_entry* ce, struct checkout* state)
> +{
> +	/*
> +	 * Dereference a worktree symlink and writes its contents
> +	 * into the checkout state's path.
> +	 */
> +	struct strbuf path = STRBUF_INIT;
> +	struct strbuf link = STRBUF_INIT;
> +
> +	int ok = 0;

>  				oidcpy(&ce->oid, &roid);
>  				strcpy(ce->name, dst_path);
>  				ce->ce_namelen = dst_path_len;
> -				if (checkout_entry(ce, &rstate, NULL))
> +
> +				if (S_ISLNK(rmode) && is_null_oid(&roid)) {
> +					if (!create_symlink_file(ce, &rstate))
> +						return error("unable to create symlink file %s",
> +							     dst_path);
> +				} else if (checkout_entry(ce, &rstate, NULL))
>  					return error("could not write '%s'",
>  						     dst_path);
>  			} else if (!is_null_oid(&roid)) {