Re: [PATCH] difftool: handle changing symlinks in dir-diff mode

[Junio C Hamano <gitster@pobox.com>]

David Aguilar <davvid@gmail.com> writes:

> -		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);

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.

Discarding "content" after reading feels wasteful, as that is the
information we would be using when populating the rstate and lstaten
working trees later in the loop, but that would probably need a
larger surgery to the code to optimize, I would imagine.