From: Duy Nguyen <pclouds@gmail.com>
Date: Fri, 07 Mar 2014 05:03:13 GMT
Subject: Re: [PATCH v4 02/27] Convert git_snpath() to strbuf_git_path()
Message-ID: <CACsJy8BwUa63yG0WownJQNedNE8JC+5_oG8PBs6sy7_tiHOh-A@mail.gmail.com>
In-Reply-To: <CACsJy8DeGCdnWHOxTeGUV4CsHyV-u_DgR-03iyOrEKznsQsqGA@mail.gmail.com>

On Mon, Mar 3, 2014 at 7:15 AM, Duy Nguyen <pclouds@gmail.com> wrote:
> On Mon, Mar 3, 2014 at 7:02 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> On Sat, Mar 1, 2014 at 7:12 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
>>> In the previous patch, git_snpath() is modified to allocate a new
>>> strbuf buffer because vsnpath() needs that. But that makes it awkward
>>> because git_snpath() receives a pre-allocated buffer from outside and
>>> has to copy data back. Rename it to strbuf_git_path() and make it
>>> receive strbuf directly.
>>>
>>> The conversion from git_snpath() to git_path() in
>>> update_refs_for_switch() is safe because that function does not keep
>>> any pointer to the round-robin buffer pool allocated by
>>> get_pathname().
>>>
>>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>>> ---
>>> diff --git a/refs.c b/refs.c
>>> index 89228e2..434bd5e 100644
>>> --- a/refs.c
>>> +++ b/refs.c
>>> @@ -2717,17 +2729,19 @@ static int copy_msg(char *buf, const char *msg)
>>>         return cp - buf;
>>>  }
>>>
>>> -int log_ref_setup(const char *refname, char *logfile, int bufsize)
>>> +int log_ref_setup(const char *refname, struct strbuf *sb_logfile)
>>>  {
>>>         int logfd, oflags = O_APPEND | O_WRONLY;
>>> +       const char *logfile;
>>>
>>> -       git_snpath(logfile, bufsize, "logs/%s", refname);
>>> +       strbuf_git_path(sb_logfile, "logs/%s", refname);
>>> +       logfile = sb_logfile->buf;
>>>         if (log_all_ref_updates &&
>>>             (starts_with(refname, "refs/heads/") ||
>>>              starts_with(refname, "refs/remotes/") ||
>>>              starts_with(refname, "refs/notes/") ||
>>>              !strcmp(refname, "HEAD"))) {
>>> -               if (safe_create_leading_directories(logfile) < 0)
>>> +               if (safe_create_leading_directories(sb_logfile->buf) < 0)
>>
>> At this point, 'logfile' is still 'sb_logfile->buf', so do you really
>> need this change?
>
> Junio made the same comment last time and I missed it. Will update.

No I will not :-) safe_create_leading_directories takes an editable
string, but logfile is now a const string. We could use
s_c_l_d_const() but that one will make a copy of the string
unncessarily. Will make a note in the commit message though.
-- 
Duy