diff mbox series

[3/7] bundle-uri: create "key=value" line parsing

Message ID 49c4f88b6fd804f0bd5c62d523b45431846f4cee.1661181174.git.gitgitgadget@gmail.com (mailing list archive)
State New, archived
Headers show
Series Bundle URIs III: Parse and download from bundle lists | expand

Commit Message

Ævar Arnfjörð Bjarmason Aug. 22, 2022, 3:12 p.m. UTC 
From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?=
 <avarab@gmail.com>

When advertising a bundle list over Git's protocol v2, we will use
packet lines. Each line will be of the form "key=value" representing a
bundle list. Connect the API necessary for Git's transport to the
key-value pair parsing created in the previous change.

Co-authored-by: Derrick Stolee <derrickstolee@github.com>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Derrick Stolee <derrickstolee@github.com>
---
 bundle-uri.c | 27 ++++++++++++++++++++++++++-
 bundle-uri.h | 14 +++++++++++++-
 2 files changed, 39 insertions(+), 2 deletions(-)

Comments

Junio C Hamano Aug. 22, 2022, 7:17 p.m. UTC | #1 
"Ævar Arnfjörð Bjarmason via GitGitGadget"  <gitgitgadget@gmail.com>
writes:

> +/**
> + * General API for {transport,connect}.c etc.
> + */
> +int bundle_uri_parse_line(struct bundle_list *list, const char *line)
> +{
> +	int result;
> +	const char *equals;
> +	struct strbuf key = STRBUF_INIT;
> +
> +	if (!strlen(line))
> +		return error(_("bundle-uri: got an empty line"));
> +
> +	equals = strchr(line, '=');
> +
> +	if (!equals)
> +		return error(_("bundle-uri: line is not of the form 'key=value'"));
> +	if (line == equals || !*(equals + 1))
> +		return error(_("bundle-uri: line has empty key or value"));

The suggestions implied by my asking fall strictly into the "it does
not have to exist here at this step and we can later extend it", but
for something whose equivalent can be stored in our configuration
file, it is curious why we _insist_ to refuse an empty string as the
value.

I do not miss the "key alone without even '=' means 'true'"
convention, personally, so insisting to have '=' is OK, but the
inability to have an empty string as a value looks a bit disturbing.

This depends on how the helper gets called, but most likely the
caller has a single line of pkt-line that it GAVE us to process, so
it sounds a bit wasteful to insist that "line" to be const to us and
force us to use a separate strbuf, instead of just stuffing NUL at
where we found '=' and pass the two halves to bundle_list_update().

Not a huge deal, it is just something I found funny in the "back in
the days we coded together, Linus would never have written like
this" way.

Other than that small detail, the code looks OK to me.

> +	strbuf_add(&key, line, equals - line);
> +	result = bundle_list_update(key.buf, equals + 1, list);
> +	strbuf_release(&key);
> +
> +	return result;
> +}
> diff --git a/bundle-uri.h b/bundle-uri.h
> index 6692aa4b170..f725c9796f7 100644
> --- a/bundle-uri.h
> +++ b/bundle-uri.h
> @@ -76,4 +76,16 @@ int for_all_bundles_in_list(struct bundle_list *list,
>   */
>  int fetch_bundle_uri(struct repository *r, const char *uri);
>  
> -#endif
> +/**
> + * General API for {transport,connect}.c etc.
> + */
> +
> +/**
> + * Parse a "key=value" packet line from the bundle-uri verb.
> + *
> + * Returns 0 on success and non-zero on error.
> + */
> +int bundle_uri_parse_line(struct bundle_list *list,
> +			  const char *line);
> +
> +#endif /* BUNDLE_URI_H */
Derrick Stolee Aug. 23, 2022, 4:31 p.m. UTC | #2 
On 8/22/2022 3:17 PM, Junio C Hamano wrote:
> "Ævar Arnfjörð Bjarmason via GitGitGadget"  <gitgitgadget@gmail.com>
> writes:
> 
>> +/**
>> + * General API for {transport,connect}.c etc.
>> + */
>> +int bundle_uri_parse_line(struct bundle_list *list, const char *line)
>> +{
>> +	int result;
>> +	const char *equals;
>> +	struct strbuf key = STRBUF_INIT;
>> +
>> +	if (!strlen(line))
>> +		return error(_("bundle-uri: got an empty line"));
>> +
>> +	equals = strchr(line, '=');
>> +
>> +	if (!equals)
>> +		return error(_("bundle-uri: line is not of the form 'key=value'"));
>> +	if (line == equals || !*(equals + 1))
>> +		return error(_("bundle-uri: line has empty key or value"));
> 
> The suggestions implied by my asking fall strictly into the "it does
> not have to exist here at this step and we can later extend it", but
> for something whose equivalent can be stored in our configuration
> file, it is curious why we _insist_ to refuse an empty string as the
> value.
> 
> I do not miss the "key alone without even '=' means 'true'"
> convention, personally, so insisting to have '=' is OK, but the
> inability to have an empty string as a value looks a bit disturbing.

I'd be happy to switch this to allow an empty value.
 
> This depends on how the helper gets called, but most likely the
> caller has a single line of pkt-line that it GAVE us to process, so
> it sounds a bit wasteful to insist that "line" to be const to us and
> force us to use a separate strbuf, instead of just stuffing NUL at
> where we found '=' and pass the two halves to bundle_list_update().

I can look into using a non-const buffer.

Thanks,
-Stolee
Josh Steadmon Sept. 2, 2022, 11:41 p.m. UTC | #3 
On 2022.08.22 15:12, Ævar Arnfjörð Bjarmason via GitGitGadget wrote:
> From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?=
>  <avarab@gmail.com>
> 
> When advertising a bundle list over Git's protocol v2, we will use
> packet lines. Each line will be of the form "key=value" representing a
> bundle list. Connect the API necessary for Git's transport to the
> key-value pair parsing created in the previous change.

Since we're not actually implementing advertisement via proto v2 in this
series, could we add an additional paragraph noting that this is useful
now for implementing the test helper in the next patch?
diff mbox series

Patch

diff --git a/bundle-uri.c b/bundle-uri.c
index ade7eccce39..9a7d09349fe 100644
--- a/bundle-uri.c
+++ b/bundle-uri.c
@@ -71,7 +71,6 @@  int for_all_bundles_in_list(struct bundle_list *list,
  * Returns 0 if the key-value pair is understood. Returns 1 if the key
  * is not understood or the value is malformed.
  */
-MAYBE_UNUSED
 static int bundle_list_update(const char *key, const char *value,
 			      struct bundle_list *list)
 {
@@ -301,3 +300,29 @@  cleanup:
 	strbuf_release(&filename);
 	return result;
 }
+
+/**
+ * General API for {transport,connect}.c etc.
+ */
+int bundle_uri_parse_line(struct bundle_list *list, const char *line)
+{
+	int result;
+	const char *equals;
+	struct strbuf key = STRBUF_INIT;
+
+	if (!strlen(line))
+		return error(_("bundle-uri: got an empty line"));
+
+	equals = strchr(line, '=');
+
+	if (!equals)
+		return error(_("bundle-uri: line is not of the form 'key=value'"));
+	if (line == equals || !*(equals + 1))
+		return error(_("bundle-uri: line has empty key or value"));
+
+	strbuf_add(&key, line, equals - line);
+	result = bundle_list_update(key.buf, equals + 1, list);
+	strbuf_release(&key);
+
+	return result;
+}
diff --git a/bundle-uri.h b/bundle-uri.h
index 6692aa4b170..f725c9796f7 100644
--- a/bundle-uri.h
+++ b/bundle-uri.h
@@ -76,4 +76,16 @@  int for_all_bundles_in_list(struct bundle_list *list,
  */
 int fetch_bundle_uri(struct repository *r, const char *uri);
 
-#endif
+/**
+ * General API for {transport,connect}.c etc.
+ */
+
+/**
+ * Parse a "key=value" packet line from the bundle-uri verb.
+ *
+ * Returns 0 on success and non-zero on error.
+ */
+int bundle_uri_parse_line(struct bundle_list *list,
+			  const char *line);
+
+#endif /* BUNDLE_URI_H */