[PATCH 0/3] small quote.[ch] cleanup

[PATCH 0/3] small quote.[ch] cleanup

[Jeff King]

I noticed some unused code while looking at an unrelated topic. So
here's a small cleanup.

  [1/3]: quote.h: bump strvec forward declaration to the top
  [2/3]: quote: drop sq_dequote_to_argv()
  [3/3]: quote: simplify internals of dequoting

 quote.c | 21 ++-------------------
 quote.h | 14 ++++----------
 2 files changed, 6 insertions(+), 29 deletions(-)

-Peff

[PATCH 1/3] quote.h: bump strvec forward declaration to the top

[Jeff King]

We usually put forward declarations at the top of header files, rather
than next to the functions that need them. In theory placing it next to
the function has some explanatory value, but it's also just as likely to
become stale if other uses are added.

Signed-off-by: Jeff King <peff@peff.net>
---
 quote.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/quote.h b/quote.h
index 0300c29104..400397b11a 100644
--- a/quote.h
+++ b/quote.h
@@ -2,6 +2,7 @@
 #define QUOTE_H
 
 struct strbuf;
+struct strvec;
 
 extern int quote_path_fully;
 
@@ -77,7 +78,6 @@ int sq_dequote_to_argv(char *arg, const char ***argv, int *nr, int *alloc);
  * still modify arg in place, but unlike sq_dequote_to_argv, the strvec
  * will duplicate and take ownership of the strings.
  */
-struct strvec;
 int sq_dequote_to_strvec(char *arg, struct strvec *);
 
 int unquote_c_style(struct strbuf *, const char *quoted, const char **endp);
-- 
2.54.0.524.g198262df96

[PATCH 2/3] quote: drop sq_dequote_to_argv()

[Jeff King]

The last caller went away in f9dbb64fad (config: parse more robust
format in GIT_CONFIG_PARAMETERS, 2021-01-12), when we switched to using
sq_dequote_step().

The "to_argv()" form is not a great interface. If you care about raw
speed, then sq_dequote_step() lets you work incrementally without extra
allocations. If you care about simplicity, then sq_dequote_to_strvec()
puts the result in an encapsulated data structure. With sq_dequote_to_argv(),
you have a data dependency on the original string but still have to
remember to manually free the argv array itself (but not its elements).

So it's sort of a worst-of-both-worlds middle ground. Let's get rid of
it.

Signed-off-by: Jeff King <peff@peff.net>
---
 quote.c |  5 -----
 quote.h | 12 +++---------
 2 files changed, 3 insertions(+), 14 deletions(-)

diff --git a/quote.c b/quote.c
index b9f6bdc775..cff78af3a4 100644
--- a/quote.c
+++ b/quote.c
@@ -202,11 +202,6 @@ static int sq_dequote_to_argv_internal(char *arg,
 	return 0;
 }
 
-int sq_dequote_to_argv(char *arg, const char ***argv, int *nr, int *alloc)
-{
-	return sq_dequote_to_argv_internal(arg, argv, nr, alloc, NULL);
-}
-
 int sq_dequote_to_strvec(char *arg, struct strvec *array)
 {
 	return sq_dequote_to_argv_internal(arg, NULL, NULL, NULL, array);
diff --git a/quote.h b/quote.h
index 400397b11a..989f2388c0 100644
--- a/quote.h
+++ b/quote.h
@@ -68,15 +68,9 @@ char *sq_dequote_step(char *src, char **next);
 
 /*
  * Same as the above, but can be used to unwrap many arguments in the
- * same string separated by space. Like sq_quote, it works in place,
- * modifying arg and appending pointers into it to argv.
- */
-int sq_dequote_to_argv(char *arg, const char ***argv, int *nr, int *alloc);
-
-/*
- * Same as above, but store the unquoted strings in a strvec. We will
- * still modify arg in place, but unlike sq_dequote_to_argv, the strvec
- * will duplicate and take ownership of the strings.
+ * same string separated by space. The strvec will duplicate and take
+ * ownership of the strings, but note that "arg" is still modified in-place
+ * during parsing.
  */
 int sq_dequote_to_strvec(char *arg, struct strvec *);
 
-- 
2.54.0.524.g198262df96

[PATCH 3/3] quote: simplify internals of dequoting

[Jeff King]

Our sq_dequote_to_argv_internal() helper was wrapped by the to_argv()
and to_strvec() forms. Now that we have only the latter, we can stop
wrapping it and drop the argv-only bits.

Note that in theory sq_dequote_to_strvec() could take a const input
string, which would be friendlier to its callers. We couldn't do that
with the to_argv() form because it reused the input string to hold the
output elements. But since we're built on sq_dequote_step(), which
munges the input, we'd have to rework the parser. Since no callers care
about it currently, we'll leave that for another day.

Signed-off-by: Jeff King <peff@peff.net>
---
 quote.c | 16 ++--------------
 1 file changed, 2 insertions(+), 14 deletions(-)

diff --git a/quote.c b/quote.c
index cff78af3a4..235fac8e47 100644
--- a/quote.c
+++ b/quote.c
@@ -171,9 +171,7 @@ char *sq_dequote(char *arg)
 	return sq_dequote_step(arg, NULL);
 }
 
-static int sq_dequote_to_argv_internal(char *arg,
-				       const char ***argv, int *nr, int *alloc,
-				       struct strvec *array)
+int sq_dequote_to_strvec(char *arg, struct strvec *array)
 {
 	char *next = arg;
 
@@ -191,22 +189,12 @@ static int sq_dequote_to_argv_internal(char *arg,
 				c = *++next;
 			} while (isspace(c));
 		}
-		if (argv) {
-			ALLOC_GROW(*argv, *nr + 1, *alloc);
-			(*argv)[(*nr)++] = dequoted;
-		}
-		if (array)
-			strvec_push(array, dequoted);
+		strvec_push(array, dequoted);
 	} while (next);
 
 	return 0;
 }
 
-int sq_dequote_to_strvec(char *arg, struct strvec *array)
-{
-	return sq_dequote_to_argv_internal(arg, NULL, NULL, NULL, array);
-}
-
 /* 1 means: quote as octal
  * 0 means: quote as octal if (quote_path_fully)
  * -1 means: never quote
-- 
2.54.0.524.g198262df96

Re: [PATCH 0/3] small quote.[ch] cleanup

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I noticed some unused code while looking at an unrelated topic. So
> here's a small cleanup.
>
>   [1/3]: quote.h: bump strvec forward declaration to the top
>   [2/3]: quote: drop sq_dequote_to_argv()
>   [3/3]: quote: simplify internals of dequoting
>
>  quote.c | 21 ++-------------------
>  quote.h | 14 ++++----------
>  2 files changed, 6 insertions(+), 29 deletions(-)
>
> -Peff

These were very straight-forward and pleasant to read.  Queued.
Thanks.