Re: [PATCH v4 02/27] Convert git_snpath() to strbuf_git_path()
- From
Eric Sunshine <sunshine@sunshineco.com>
- Date
- Mar 7, 2014, 05:26 UTC
- Message-ID
- <CAPig+cR_JaaW18ng8Z8_hsXy3DNT216o7ogNLxmcW2+tp2Ax9Q@mail.gmail.com>
- In-Reply-To
- <CACsJy8BwUa63yG0WownJQNedNE8JC+5_oG8PBs6sy7_tiHOh-A@mail.gmail.com>
On Fri, Mar 7, 2014 at 12:03 AM, Duy Nguyen <pclouds@gmail.com> wrote:
Show 50 quoted lines
> 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.Rather than explaining it in the commit message, it might be better eliminate the source of confusion by taking one of these approaches:
1. Drop the 'const char *logfile' variable altogether; rename the strbuf argument to 'logfile'; and just use logfile->buf everywhere 'logfile' is used in the current code. This makes the diff a bit more noisy, but eliminates confusion of reviewers reading the patch.
2. Keep the 'const char *logfile' but assign it just before its first (real) use in 'logfd = open(logfile...)'. (There's one earlier use in a diagnostic, but sb_logfile->buf could suffice there.)
3. Just declare it 'char *logfile' and use it everywhere in the function. This is a bit ugly since it's not obvious to the reviewer that it is non-const only for the sake of safe_create_leading_directories().
I fiind myself favoring #1 in this particular case.