Re: [PATCH v4 02/27] Convert git_snpath() to strbuf_git_path()

2014-03-06 Thread Eric Sunshine 
On Fri, Mar 7, 2014 at 12:03 AM, Duy Nguyen  wrote:
> On Mon, Mar 3, 2014 at 7:15 AM, Duy Nguyen  wrote:
>> On Mon, Mar 3, 2014 at 7:02 AM, Eric Sunshine  
>> wrote:
>>> On Sat, Mar 1, 2014 at 7:12 AM, Nguyễn Thái Ngọc Duy  
>>> 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 
 ---
 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 02/27] Convert git_snpath() to strbuf_git_path()

2014-03-06 Thread Duy Nguyen 
On Mon, Mar 3, 2014 at 7:15 AM, Duy Nguyen  wrote:
> On Mon, Mar 3, 2014 at 7:02 AM, Eric Sunshine  wrote:
>> On Sat, Mar 1, 2014 at 7:12 AM, Nguyễn Thái Ngọc Duy  
>> 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 
>>> ---
>>> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 02/27] Convert git_snpath() to strbuf_git_path()

2014-03-02 Thread Duy Nguyen 
On Mon, Mar 3, 2014 at 7:02 AM, Eric Sunshine  wrote:
> On Sat, Mar 1, 2014 at 7:12 AM, Nguyễn Thái Ngọc Duy  
> 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 
>> ---
>> 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.
-- 
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 02/27] Convert git_snpath() to strbuf_git_path()

2014-03-02 Thread Eric Sunshine 
On Sat, Mar 1, 2014 at 7:12 AM, Nguyễn Thái Ngọc Duy  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 
> ---
> 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?

> return error("unable to create directory for %s",
>  logfile);
> oflags |= O_CREAT;
> @@ -2762,20 +2776,22 @@ static int log_ref_write(const char *refname, const 
> unsigned char *old_sha1,
> int logfd, result, written, oflags = O_APPEND | O_WRONLY;
> unsigned maxlen, len;
> int msglen;
> -   char log_file[PATH_MAX];
> +   struct strbuf sb_log_file = STRBUF_INIT;
> +   const char *log_file;
> char *logrec;
> const char *committer;
>
> if (log_all_ref_updates < 0)
> log_all_ref_updates = !is_bare_repository();
>
> -   result = log_ref_setup(refname, log_file, sizeof(log_file));
> +   result = log_ref_setup(refname, &sb_log_file);
> if (result)
> -   return result;
> +   goto done;
> +   log_file = sb_log_file.buf;
>
> logfd = open(log_file, oflags);
> if (logfd < 0)
> -   return 0;
> +   goto done;
> msglen = msg ? strlen(msg) : 0;
> committer = git_committer_info(0);
> maxlen = strlen(committer) + msglen + 100;
> --
> 1.9.0.40.gaa8c3ea
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 02/27] Convert git_snpath() to strbuf_git_path()

2014-03-01 Thread Nguyễn Thái Ngọc Duy 
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 
---
 builtin/checkout.c | 22 +-
 cache.h|  4 ++--
 path.c | 11 ++---
 refs.c | 66 +++---
 refs.h |  2 +-
 5 files changed, 58 insertions(+), 47 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 5df3837..0570e41 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -585,18 +585,20 @@ static void update_refs_for_switch(const struct 
checkout_opts *opts,
if (opts->new_orphan_branch) {
if (opts->new_branch_log && !log_all_ref_updates) {
int temp;
-   char log_file[PATH_MAX];
+   struct strbuf log_file = STRBUF_INIT;
char *ref_name = mkpath("refs/heads/%s", 
opts->new_orphan_branch);
+   int ret;
 
temp = log_all_ref_updates;
log_all_ref_updates = 1;
-   if (log_ref_setup(ref_name, log_file, 
sizeof(log_file))) {
+   ret = log_ref_setup(ref_name, &log_file);
+   log_all_ref_updates = temp;
+   strbuf_release(&log_file);
+   if (ret) {
fprintf(stderr, _("Can not do reflog 
for '%s'\n"),
opts->new_orphan_branch);
-   log_all_ref_updates = temp;
return;
}
-   log_all_ref_updates = temp;
}
}
else
@@ -651,14 +653,10 @@ static void update_refs_for_switch(const struct 
checkout_opts *opts,
new->name);
}
}
-   if (old->path && old->name) {
-   char log_file[PATH_MAX], ref_file[PATH_MAX];
-
-   git_snpath(log_file, sizeof(log_file), "logs/%s", 
old->path);
-   git_snpath(ref_file, sizeof(ref_file), "%s", old->path);
-   if (!file_exists(ref_file) && file_exists(log_file))
-   remove_path(log_file);
-   }
+   if (old->path && old->name &&
+   !file_exists(git_path("%s", old->path)) &&
+file_exists(git_path("logs/%s", old->path)))
+   remove_path(git_path("logs/%s", old->path));
}
remove_branch_state();
strbuf_release(&msg);
diff --git a/cache.h b/cache.h
index dc040fb..8d3697e 100644
--- a/cache.h
+++ b/cache.h
@@ -646,8 +646,8 @@ extern int check_repository_format(void);
 
 extern char *mksnpath(char *buf, size_t n, const char *fmt, ...)
__attribute__((format (printf, 3, 4)));
-extern char *git_snpath(char *buf, size_t n, const char *fmt, ...)
-   __attribute__((format (printf, 3, 4)));
+extern void strbuf_git_path(struct strbuf *sb, const char *fmt, ...)
+   __attribute__((format (printf, 2, 3)));
 extern char *git_pathdup(const char *fmt, ...)
__attribute__((format (printf, 1, 2)));
 extern char *mkpathdup(const char *fmt, ...)
diff --git a/path.c b/path.c
index 5346700..b52a16f 100644
--- a/path.c
+++ b/path.c
@@ -70,19 +70,12 @@ static void vsnpath(struct strbuf *buf, const char *fmt, 
va_list args)
strbuf_cleanup_path(buf);
 }
 
-char *git_snpath(char *buf, size_t n, const char *fmt, ...)
+void strbuf_git_path(struct strbuf *sb, const char *fmt, ...)
 {
-   struct strbuf sb = STRBUF_INIT;
va_list args;
va_start(args, fmt);
-   vsnpath(&sb, fmt, args);
+   vsnpath(sb, fmt, args);
va_end(args);
-   if (sb.len >= n)
-   strlcpy(buf, bad_path, n);
-   else
-   memcpy(buf, sb.buf, sb.len + 1);
-   strbuf_release(&sb);
-   return buf;
 }
 
 char *git_pathdup(const char *fmt, ...)
diff --git a/refs.c b/refs.c
index 89228e2..434bd5e 100644
--- a/refs.c
+++ b/refs.c
@@ -1325,10 +1325,12 @@ static const char *handle_missing_loose_ref(const char 
*refname,
 
 const char *resolve_ref_unsafe(const char *refname, unsigned char *sha1, int 
reading, int *flag)
 {
+   struct strbuf sb_path = STRBUF_INIT;