Re: [PATCH v6 4/5] difftool: refactor dir-diff to write files using a helper function

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 30, 2021, 22:17 UTC
Message-ID: <xmqq35plaetp.fsf@gitster.g>
In-Reply-To: <20210930170146.61489-4-davvid@gmail.com>

David Aguilar <davvid@gmail.com> writes:

> This is cleanup refactoring that Junio suggested when
> 5bafb3576a (difftool: fix symlink-file writing in dir-diff mode, 2021-09-22)
> touched this area of the code.

Not really what I would want to take credit for  ;-)

Show 5 quoted lines

> +static void write_entry(const char *path, const char *content,
> +			struct strbuf *buf, size_t len)
> +{
> +	if (!*content)
> +		return;

I am not sure "this function is unable to write an empty file" is a limitation we want to give to an otherwise more or less generic helper function that may be useful in this file (it probably is not very useful outside difftool, as what add_path() does seems quite specific to it).

Also, is "write entry" a good name for this helper? The fact that the contents came from entry->$side is lost inside this callee. It looks more like "create a file for <path> and write <content> to it", i.e. a variant of write_file() but inside the tree specified by the extra <buf, len> pair. So perhaps

	write_file_in_directory(buf, len, path, content);

to match how the write_file() takes its parameters? While write_file() takes a single pathname with the payload, this thing takes three parameters <buf, len, path> to specify to which "file-in-directory" the payload is written.

Show 5 quoted lines

> +	add_path(buf, len, path);
> +	ensure_leading_directories(buf->buf);
> +	unlink(buf->buf);
> +	write_file(buf->buf, "%s", content);
> +}

Other than these two minor points, this looks good to me.

Thanks.