[prev in list] [next in list] [prev in thread] [next in thread] 

List:       git
Subject:    Re: [PATCH v6 4/5] difftool: refactor dir-diff to write files using a helper function
From:       David Aguilar <davvid () gmail ! com>
Date:       2021-09-30 23:34:50
Message-ID: CAJDDKr78WcoWjeZqz3c_bdt3s0RRg8Hx9-wC1VFmpy2yPbpbqA () mail ! gmail ! com
[Download RAW message or body]

On Thu, Sep 30, 2021 at 3:17 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> 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  ;-)

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
[prev in list] [next in list] [prev in thread] [next in thread]