On Thu, Sep 30, 2021 at 3:17 PM Junio C Hamano <gitster@xxxxxxxxx> wrote:
>
> David Aguilar <davvid@xxxxxxxxx> 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 ;-)
Likewise, even I don't like to take credit for my scrappy patches sometimes ;-)
I'll reword this to avoid mentioning the review context.
> > +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).
Good point, I'll move the conditional checks into the call sites
rather than having it in the helper. It'll read a little more clearly that
way as well.
> 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.
>
> > + 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.
write_file_in_directory() is much clearer. I'll adjust the signature
in the next version.
Thanks for the review. I'll wait to hear back about 3/5 before sending v7.
--
David
