Re: [PATCH 06/21] convert manual allocations to argv_array

[Eric Sunshine]

On Sat, Feb 20, 2016 at 3:57 AM, Jeff King  wrote:
> On Sat, Feb 20, 2016 at 03:39:36AM -0500, Eric Sunshine wrote:
>> I also had made the strbuf_detach() analogy in my response but deleted
>> it before sending; I do think it's a reasonable API template to mirror
>> via new argv_array_detach().
>
> That would look like this, which I think is not too bad (on top of my
> series for now; I'd do the API function as a separate patch at the
> beginning and then use it immediately).

Looks reasonable.

> diff --git a/argv-array.c b/argv-array.c
> index eaed477..5d370fa 100644
> --- a/argv-array.c
> +++ b/argv-array.c
> @@ -74,3 +74,14 @@ void argv_array_clear(struct argv_array *array)
> }
> argv_array_init(array);
>  }
> +
> +const char **argv_array_detach(struct argv_array *array)
> +{
> +   if (array->argv == empty_argv)
> +   return xcalloc(1, sizeof(const char *));
> +   else {
> +   const char **ret = array->argv;
> +   argv_array_init(array);
> +   return ret;
> +   }
> +}
> diff --git a/argv-array.h b/argv-array.h
> index a2fa0aa..29056e4 100644
> --- a/argv-array.h
> +++ b/argv-array.h
> @@ -20,5 +20,6 @@ void argv_array_pushl(struct argv_array *, ...);
>  void argv_array_pushv(struct argv_array *, const char **);
>  void argv_array_pop(struct argv_array *);
>  void argv_array_clear(struct argv_array *);
> +const char **argv_array_detach(struct argv_array *);
>
>  #endif /* ARGV_ARRAY_H */
> diff --git a/line-log.c b/line-log.c
> index fa095b9..bbe31ed 100644
> --- a/line-log.c
> +++ b/line-log.c
> @@ -748,15 +748,17 @@ void line_log_init(struct rev_info *rev, const char 
> *prefix, struct string_list
>
> if (!rev->diffopt.detect_rename) {
> struct line_log_data *r;
> -   struct argv_array paths = ARGV_ARRAY_INIT;
> +   struct argv_array array = ARGV_ARRAY_INIT;
> +   const char **paths;
>
> for (r = range; r; r = r->next)
> -   argv_array_push(&paths, r->path);
> +   argv_array_push(&array, r->path);
> +   paths = argv_array_detach(&array);
> +
> parse_pathspec(&rev->diffopt.pathspec, 0,
> -  PATHSPEC_PREFER_FULL, "", paths.argv);
> -   /* argv strings are now owned by pathspec */
> -   paths.argc = 0;
> -   argv_array_clear(&paths);
> +  PATHSPEC_PREFER_FULL, "", paths);
> +   /* strings are now owned by pathspec */
> +   free(paths);
> }
>  }
--
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 06/21] convert manual allocations to argv_array

[Jeff King]

On Sat, Feb 20, 2016 at 03:39:36AM -0500, Eric Sunshine wrote:

> I also had made the strbuf_detach() analogy in my response but deleted
> it before sending; I do think it's a reasonable API template to mirror
> via new argv_array_detach().

That would look like this, which I think is not too bad (on top of my
series for now; I'd do the API function as a separate patch at the
beginning and then use it immediately).

diff --git a/argv-array.c b/argv-array.c
index eaed477..5d370fa 100644
--- a/argv-array.c
+++ b/argv-array.c
@@ -74,3 +74,14 @@ void argv_array_clear(struct argv_array *array)
}
argv_array_init(array);
 }
+
+const char **argv_array_detach(struct argv_array *array)
+{
+   if (array->argv == empty_argv)
+   return xcalloc(1, sizeof(const char *));
+   else {
+   const char **ret = array->argv;
+   argv_array_init(array);
+   return ret;
+   }
+}
diff --git a/argv-array.h b/argv-array.h
index a2fa0aa..29056e4 100644
--- a/argv-array.h
+++ b/argv-array.h
@@ -20,5 +20,6 @@ void argv_array_pushl(struct argv_array *, ...);
 void argv_array_pushv(struct argv_array *, const char **);
 void argv_array_pop(struct argv_array *);
 void argv_array_clear(struct argv_array *);
+const char **argv_array_detach(struct argv_array *);
 
 #endif /* ARGV_ARRAY_H */
diff --git a/line-log.c b/line-log.c
index fa095b9..bbe31ed 100644
--- a/line-log.c
+++ b/line-log.c
@@ -748,15 +748,17 @@ void line_log_init(struct rev_info *rev, const char 
*prefix, struct string_list
 
if (!rev->diffopt.detect_rename) {
struct line_log_data *r;
-   struct argv_array paths = ARGV_ARRAY_INIT;
+   struct argv_array array = ARGV_ARRAY_INIT;
+   const char **paths;
 
for (r = range; r; r = r->next)
-   argv_array_push(&paths, r->path);
+   argv_array_push(&array, r->path);
+   paths = argv_array_detach(&array);
+
parse_pathspec(&rev->diffopt.pathspec, 0,
-  PATHSPEC_PREFER_FULL, "", paths.argv);
-   /* argv strings are now owned by pathspec */
-   paths.argc = 0;
-   argv_array_clear(&paths);
+  PATHSPEC_PREFER_FULL, "", paths);
+   /* strings are now owned by pathspec */
+   free(paths);
}
 }
 
--
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 06/21] convert manual allocations to argv_array

[Eric Sunshine]

On Sat, Feb 20, 2016 at 3:34 AM, Jeff King  wrote:
> On Sat, Feb 20, 2016 at 03:29:29AM -0500, Eric Sunshine wrote:
>> On Sat, Feb 20, 2016 at 3:10 AM, Jeff King  wrote:
>> > On Sat, Feb 20, 2016 at 03:07:00AM -0500, Eric Sunshine wrote:
>> >> > +   /* argv strings are now owned by pathspec */
>> >> > +   paths.argc = 0;
>> >> > +   argv_array_clear(&paths);
>> >>
>> >> This overly intimate knowledge of the internal implementation of
>> >> argv_array_clear() is rather ugly.
>> >
>> > Yep, I agree. Suggestions?
>> > [...]
>> >
>> > I guess we can make an argv_array_detach_strings() function. Or maybe
>> > even just argv_array_detach() would be less gross, and then this
>> > function could manually free the array but not the strings themselves.
>>
>> [...]
>> I wonder if a simple "dup'ing" string_list would be more suitable for
>> this case. You'd have to append the NULL item manually with
>> string_list_append_nodup(), and string_list_clear() would then be the
>> correct way to dispose of the list without intimate knowledge of its
>> implementation and no need for an API extension.
>
> A string_list doesn't just store pointers; it's a struct with a util
> field. So you can't pass it to things expecting a "const char **".

Yep, I knew that but wasn't thinking straight.

> I think argv_array_detach() is the least-bad thing here. It matches
> strbuf_detach() to say "you now own the storage" (as opposed to just
> peeking at argv.argv, which we should do only in a read-only way).

I also had made the strbuf_detach() analogy in my response but deleted
it before sending; I do think it's a reasonable API template to mirror
via new argv_array_detach().
--
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 06/21] convert manual allocations to argv_array

[Jeff King]

On Sat, Feb 20, 2016 at 03:29:29AM -0500, Eric Sunshine wrote:

> On Sat, Feb 20, 2016 at 3:10 AM, Jeff King  wrote:
> > On Sat, Feb 20, 2016 at 03:07:00AM -0500, Eric Sunshine wrote:
> >> > +   /* argv strings are now owned by pathspec */
> >> > +   paths.argc = 0;
> >> > +   argv_array_clear(&paths);
> >>
> >> This overly intimate knowledge of the internal implementation of
> >> argv_array_clear() is rather ugly.
> >
> > Yep, I agree. Suggestions?
> >
> > We can just leak the array of "char *". This function is called only
> > once per program invocation, and that's unlikely to change.
> >
> > I guess we can make an argv_array_detach_strings() function. Or maybe
> > even just argv_array_detach() would be less gross, and then this
> > function could manually free the array but not the strings themselves.
> 
> The latter is what I was thinking, and I agree that
> argv_array_detach() would be less gross than
> argv_array_detach_strings(), however, it also feels a bit wrong since
> this sort of ownership transfer is kind of out of scope for
> argv_array.
> 
> I wonder if a simple "dup'ing" string_list would be more suitable for
> this case. You'd have to append the NULL item manually with
> string_list_append_nodup(), and string_list_clear() would then be the
> correct way to dispose of the list without intimate knowledge of its
> implementation and no need for an API extension.

A string_list doesn't just store pointers; it's a struct with a util
field. So you can't pass it to things expecting a "const char **".

I think argv_array_detach() is the least-bad thing here. It matches
strbuf_detach() to say "you now own the storage" (as opposed to just
peeking at argv.argv, which we should do only in a read-only way).

-Peff
--
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 06/21] convert manual allocations to argv_array

[Eric Sunshine]

On Sat, Feb 20, 2016 at 3:10 AM, Jeff King  wrote:
> On Sat, Feb 20, 2016 at 03:07:00AM -0500, Eric Sunshine wrote:
>> > +   /* argv strings are now owned by pathspec */
>> > +   paths.argc = 0;
>> > +   argv_array_clear(&paths);
>>
>> This overly intimate knowledge of the internal implementation of
>> argv_array_clear() is rather ugly.
>
> Yep, I agree. Suggestions?
>
> We can just leak the array of "char *". This function is called only
> once per program invocation, and that's unlikely to change.
>
> I guess we can make an argv_array_detach_strings() function. Or maybe
> even just argv_array_detach() would be less gross, and then this
> function could manually free the array but not the strings themselves.

The latter is what I was thinking, and I agree that
argv_array_detach() would be less gross than
argv_array_detach_strings(), however, it also feels a bit wrong since
this sort of ownership transfer is kind of out of scope for
argv_array.

I wonder if a simple "dup'ing" string_list would be more suitable for
this case. You'd have to append the NULL item manually with
string_list_append_nodup(), and string_list_clear() would then be the
correct way to dispose of the list without intimate knowledge of its
implementation and no need for an API extension.
--
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 06/21] convert manual allocations to argv_array

[Jeff King]

On Sat, Feb 20, 2016 at 03:07:00AM -0500, Eric Sunshine wrote:

> > diff --git a/line-log.c b/line-log.c
> > @@ -746,23 +747,16 @@ void line_log_init(struct rev_info *rev, const char 
> > *prefix, struct string_list
> > if (!rev->diffopt.detect_rename) {
> > -   int i, count = 0;
> > -   struct line_log_data *r = range;
> > -   const char **paths;
> > -   while (r) {
> > -   count++;
> > -   r = r->next;
> > -   }
> > -   paths = xmalloc((count+1)*sizeof(char *));
> > -   r = range;
> > -   for (i = 0; i < count; i++) {
> > -   paths[i] = xstrdup(r->path);
> > -   r = r->next;
> > -   }
> > -   paths[count] = NULL;
> > +   struct line_log_data *r;
> > +   struct argv_array paths = ARGV_ARRAY_INIT;
> > +
> > +   for (r = range; r; r = r->next)
> > +   argv_array_push(&paths, r->path);
> > parse_pathspec(&rev->diffopt.pathspec, 0,
> > -  PATHSPEC_PREFER_FULL, "", paths);
> > -   free(paths);
> > +  PATHSPEC_PREFER_FULL, "", paths.argv);
> > +   /* argv strings are now owned by pathspec */
> > +   paths.argc = 0;
> > +   argv_array_clear(&paths);
> 
> This overly intimate knowledge of the internal implementation of
> argv_array_clear() is rather ugly.

Yep, I agree. Suggestions?

We can just leak the array of "char *". This function is called only
once per program invocation, and that's unlikely to change.

I guess we can make an argv_array_detach_strings() function. Or maybe
even just argv_array_detach() would be less gross, and then this
function could manually free the array but not the strings themselves.

-Peff
--
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 06/21] convert manual allocations to argv_array

[Eric Sunshine]

On Fri, Feb 19, 2016 at 6:23 AM, Jeff King  wrote:
> There are many manual argv allocations that predate the
> argv_array API. Switching to that API brings a few
> advantages:
>
>   1. We no longer have to manually compute the correct final
>  array size (so it's one less thing we can screw up).
>
>   2. In many cases we had to make a separate pass to count,
>  then allocate, then fill in the array. Now we can do it
>  in one pass, making the code shorter and easier to
>  follow.
>
>   3. argv_array handles memory ownership for us, making it
>  more obvious when things should be free()d and and when
>  not.
>
> Most of these cases are pretty straightforward. In some, we
> switch from "run_command_v" to "run_command" which lets us
> directly use the argv_array embedded in "struct
> child_process".
>
> Signed-off-by: Jeff King 
> ---
> diff --git a/line-log.c b/line-log.c
> @@ -746,23 +747,16 @@ void line_log_init(struct rev_info *rev, const char 
> *prefix, struct string_list
> if (!rev->diffopt.detect_rename) {
> -   int i, count = 0;
> -   struct line_log_data *r = range;
> -   const char **paths;
> -   while (r) {
> -   count++;
> -   r = r->next;
> -   }
> -   paths = xmalloc((count+1)*sizeof(char *));
> -   r = range;
> -   for (i = 0; i < count; i++) {
> -   paths[i] = xstrdup(r->path);
> -   r = r->next;
> -   }
> -   paths[count] = NULL;
> +   struct line_log_data *r;
> +   struct argv_array paths = ARGV_ARRAY_INIT;
> +
> +   for (r = range; r; r = r->next)
> +   argv_array_push(&paths, r->path);
> parse_pathspec(&rev->diffopt.pathspec, 0,
> -  PATHSPEC_PREFER_FULL, "", paths);
> -   free(paths);
> +  PATHSPEC_PREFER_FULL, "", paths.argv);
> +   /* argv strings are now owned by pathspec */
> +   paths.argc = 0;
> +   argv_array_clear(&paths);

This overly intimate knowledge of the internal implementation of
argv_array_clear() is rather ugly.

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