Re: [PATCH v3 14/25] setup.c: convert is_git_directory() to use strbuf

2014-02-20 Thread Duy Nguyen 
On Thu, Feb 20, 2014 at 3:17 AM, Junio C Hamano gits...@pobox.com wrote:
 + strbuf_setlen(sb, len);
 + strbuf_add(sb, s, strlen(s));

 I am not sure addstr_at() gives us a good abstraction, or at least
 the name conveys what it does well not to confuse readers.

 At first after only seeing its name, I would have expected that it
 would splice the given string into an existing strbuf at the
 location, not chopping the existing strbuf at the location and
 appending.

I think I invented a few new strbuf_* in this series and this is one
of them. We have about ~14 other places in current code that do
similar pattern: set length back, then add something on top. Not sure
if it's worth a convenient wrapper. I don't know, maybe it's not worth
reducing one line and causing more confusion.


 +}
  static inline void strbuf_addbuf(struct strbuf *sb, const struct strbuf 
 *sb2) {
   strbuf_grow(sb, sb2-len);
   strbuf_add(sb, sb2-buf, sb2-len);



-- 
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 v3 14/25] setup.c: convert is_git_directory() to use strbuf

2014-02-20 Thread Junio C Hamano 
Duy Nguyen pclo...@gmail.com writes:

 On Thu, Feb 20, 2014 at 3:17 AM, Junio C Hamano gits...@pobox.com wrote:
 + strbuf_setlen(sb, len);
 + strbuf_add(sb, s, strlen(s));

 I am not sure addstr_at() gives us a good abstraction, or at least
 the name conveys what it does well not to confuse readers.

 At first after only seeing its name, I would have expected that it
 would splice the given string into an existing strbuf at the
 location, not chopping the existing strbuf at the location and
 appending.

 I think I invented a few new strbuf_* in this series and this is one
 of them. We have about ~14 other places in current code that do
 similar pattern: set length back, then add something on top.

Yes, and you can count getline/getwholeline as a special case to
chomp to empty at the beginning.  I am not opposed to a helper to
give us an easy access to this common pattern.

It was just the name addstr-at did not sound, at least to me, what
it does, i.e. replace with s from the pos to the end, which I
think is the same thing as a single-liner:

strbuf_splice(sb, pos, sb-len - pos, s, strlen(s))

--
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 v3 14/25] setup.c: convert is_git_directory() to use strbuf

2014-02-20 Thread Junio C Hamano 
Junio C Hamano gits...@pobox.com writes:

 Duy Nguyen pclo...@gmail.com writes:

 On Thu, Feb 20, 2014 at 3:17 AM, Junio C Hamano gits...@pobox.com wrote:
 + strbuf_setlen(sb, len);
 + strbuf_add(sb, s, strlen(s));

 I am not sure addstr_at() gives us a good abstraction, or at least
 the name conveys what it does well not to confuse readers.

 At first after only seeing its name, I would have expected that it
 would splice the given string into an existing strbuf at the
 location, not chopping the existing strbuf at the location and
 appending.

 I think I invented a few new strbuf_* in this series and this is one
 of them. We have about ~14 other places in current code that do
 similar pattern: set length back, then add something on top.

 Yes, and you can count getline/getwholeline as a special case to
 chomp to empty at the beginning.  I am not opposed to a helper to
 give us an easy access to this common pattern.

 It was just the name addstr-at did not sound, at least to me, what
 it does, i.e. replace with s from the pos to the end, which I
 think is the same thing as a single-liner:

 strbuf_splice(sb, pos, sb-len - pos, s, strlen(s))

Oh, and as to other strbuf_* helpers, I am finding myself getting
very fond of strbuf_git_path() as I read the series along; it gives
us the same convenience as git_path() [*1*] while giving us tighter
control on the lifetime rules of the path buffer.

[Footnote]

*1* And the new git_path() updated in this series has to be a lot
 more than catenate($GIT_DIR, /, $path) but needs the smart of
 adjust_git_path(), the convenience matters.
--
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 v3 14/25] setup.c: convert is_git_directory() to use strbuf

2014-02-20 Thread Eric Sunshine 
On Tue, Feb 18, 2014 at 8:40 AM, Nguyễn Thái Ngọc Duy pclo...@gmail.com wrote:
 Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
 ---
 index 73e80ce..aec9fdb 100644
 --- a/strbuf.h
 +++ b/strbuf.h
 @@ -116,6 +116,10 @@ extern void strbuf_add(struct strbuf *, const void *, 
 size_t);
  static inline void strbuf_addstr(struct strbuf *sb, const char *s) {
 strbuf_add(sb, s, strlen(s));
  }
 +static inline void strbuf_addstr_at(struct strbuf *sb, size_t len, const 
 char *s) {
 +   strbuf_setlen(sb, len);
 +   strbuf_add(sb, s, strlen(s));
 +}

Update Documentation/technical/api-strbuf.txt?

  static inline void strbuf_addbuf(struct strbuf *sb, const struct strbuf 
 *sb2) {
 strbuf_grow(sb, sb2-len);
 strbuf_add(sb, sb2-buf, sb2-len);
 --
 1.8.5.2.240.g8478abd
--
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 v3 14/25] setup.c: convert is_git_directory() to use strbuf

2014-02-19 Thread Junio C Hamano 
Nguyễn Thái Ngọc Duy  pclo...@gmail.com writes:

 Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
 ---

(Only minor nits first during this round of review)

 diff --git a/strbuf.h b/strbuf.h
 index 73e80ce..aec9fdb 100644
 --- a/strbuf.h
 +++ b/strbuf.h
 @@ -116,6 +116,10 @@ extern void strbuf_add(struct strbuf *, const void *, 
 size_t);
  static inline void strbuf_addstr(struct strbuf *sb, const char *s) {
   strbuf_add(sb, s, strlen(s));
  }
 +static inline void strbuf_addstr_at(struct strbuf *sb, size_t len, const 
 char *s) {

Please have the opening { on its own line.

Surrounding existing functions are all offenders, but that is not an
excuse to make it worse (cleaning them up will need to be done in a
separate patch).

 + strbuf_setlen(sb, len);
 + strbuf_add(sb, s, strlen(s));

I am not sure addstr_at() gives us a good abstraction, or at least
the name conveys what it does well not to confuse readers.

At first after only seeing its name, I would have expected that it
would splice the given string into an existing strbuf at the
location, not chopping the existing strbuf at the location and
appending.

 +}
  static inline void strbuf_addbuf(struct strbuf *sb, const struct strbuf 
 *sb2) {
   strbuf_grow(sb, sb2-len);
   strbuf_add(sb, sb2-buf, sb2-len);
--
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 v3 14/25] setup.c: convert is_git_directory() to use strbuf

2014-02-18 Thread Nguyễn Thái Ngọc Duy 
Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
 setup.c  | 35 +++
 strbuf.h |  4 
 2 files changed, 23 insertions(+), 16 deletions(-)

diff --git a/setup.c b/setup.c
index 6c3f85f..999225b 100644
--- a/setup.c
+++ b/setup.c
@@ -184,31 +184,34 @@ void verify_non_filename(const char *prefix, const char 
*arg)
  */
 int is_git_directory(const char *suspect)
 {
-   char path[PATH_MAX];
-   size_t len = strlen(suspect);
+   struct strbuf path = STRBUF_INIT;
+   int ret = 0;
+   size_t len;
 
-   if (PATH_MAX = len + strlen(/objects))
-   die(Too long path: %.*s, 60, suspect);
-   strcpy(path, suspect);
+   strbuf_addstr(path, suspect);
+   len = path.len;
if (getenv(DB_ENVIRONMENT)) {
if (access(getenv(DB_ENVIRONMENT), X_OK))
-   return 0;
+   goto done;
}
else {
-   strcpy(path + len, /objects);
-   if (access(path, X_OK))
-   return 0;
+   strbuf_addstr(path, /objects);
+   if (access(path.buf, X_OK))
+   goto done;
}
 
-   strcpy(path + len, /refs);
-   if (access(path, X_OK))
-   return 0;
+   strbuf_addstr_at(path, len, /refs);
+   if (access(path.buf, X_OK))
+   goto done;
 
-   strcpy(path + len, /HEAD);
-   if (validate_headref(path))
-   return 0;
+   strbuf_addstr_at(path, len, /HEAD);
+   if (validate_headref(path.buf))
+   goto done;
 
-   return 1;
+   ret = 1;
+done:
+   strbuf_release(path);
+   return ret;
 }
 
 int is_inside_git_dir(void)
diff --git a/strbuf.h b/strbuf.h
index 73e80ce..aec9fdb 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -116,6 +116,10 @@ extern void strbuf_add(struct strbuf *, const void *, 
size_t);
 static inline void strbuf_addstr(struct strbuf *sb, const char *s) {
strbuf_add(sb, s, strlen(s));
 }
+static inline void strbuf_addstr_at(struct strbuf *sb, size_t len, const char 
*s) {
+   strbuf_setlen(sb, len);
+   strbuf_add(sb, s, strlen(s));
+}
 static inline void strbuf_addbuf(struct strbuf *sb, const struct strbuf *sb2) {
strbuf_grow(sb, sb2-len);
strbuf_add(sb, sb2-buf, sb2-len);
-- 
1.8.5.2.240.g8478abd

--
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