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

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 13, 2017, 19:02 UTC
Message-ID: <xmqqh92xrodr.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20170313175640.14106-1-davvid@gmail.com>

David Aguilar <davvid@gmail.com> writes:

Show 12 quoted lines

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