Re: [PATCH v2 6/9] strbuf: introduce strbuf_strip_file_from_path()

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Wed, Nov 16 2022, Derrick Stolee via GitGitGadget wrote:

> From: Derrick Stolee <derrickstolee@github.com>
>
> The strbuf_parent_directory() method was added as a static method in
> contrib/scalar by d0feac4e8c0 (scalar: 'register' sets recommended
> config and starts maintenance, 2021-12-03) and then removed in
> 65f6a9eb0b9 (scalar: constrain enlistment search, 2022-08-18), but now
> there is a need for a similar method in the bundle URI feature.
>
> Re-add the method, this time in strbuf.c, but with a new name:
> strbuf_strip_file_from_path(). The method requirements are slightly
> modified to allow a trailing slash, in which case nothing is done, which
> makes the name change valuable. The return value is the number of bytes
> removed.
>
> Signed-off-by: Derrick Stolee <derrickstolee@github.com>
> ---
>  strbuf.c |  9 +++++++++
>  strbuf.h | 12 ++++++++++++
>  2 files changed, 21 insertions(+)
>
> diff --git a/strbuf.c b/strbuf.c
> index 0890b1405c5..8d1e2e8bb61 100644
> --- a/strbuf.c
> +++ b/strbuf.c
> @@ -1200,3 +1200,12 @@ int strbuf_edit_interactively(struct strbuf *buffer, const char *path,
>  	free(path2);
>  	return res;
>  }
> +
> +size_t strbuf_strip_file_from_path(struct strbuf *buf)

Nit: Almost every function in this API calls its argument "sb", let's do
that for new functions.

> +{
> +	size_t len = buf->len;
> +	size_t offset = offset_1st_component(buf->buf);

Mm, isn't the return value of offset_1st_component() a boolean? it's
just an "is_dir_sep(buf->buf[0])".

So this works to....

> +	char *path_sep = find_last_dir_sep(buf->buf + offset);

...find the last dir separator starting at either 0 or 1.

But anyway, what sort of string is this expecting to handle where the
last dir separator isn't >=1 offset into the string anyway? Shouldn't we
just exclude the string "/" here? Maybe I'm missing something....


> +	strbuf_setlen(buf, path_sep ? path_sep - buf->buf + 1 : offset);
> +	return len - buf->len;
> +}

Urm, so isn't this literally one-byte away from being equivalent to a
function that's already in the API?:
strbuf_trim_trailing_dir_sep. I.e. this seems to me to do the same as
this new function.

Context manually adjusted so we can see the only difference is the
"is_dir_sep" v.s. "!is_dir_sep".

There's a few strbuf functions like that, and we should probably
generalize the ctype-like test they share into some callback mechanism,
but in the meantime keeping with the pattern & naming of existing
functions seems better.

But again, I may be missing something.

I removed the comment because if it's the same then the new function is
self-documenting. It doesn't matter if the URI ends in a "/" or not, all
we need to get across is that we're stripping non-dirsep characters from
the URL, whether it ends in one or not.

In terms of correctness: The use of is_dir_sep() seems incorrect to me
here. On Windows won't that end up using is_xplatform_dir_sep(), so
bundle-uri's behavior will differ there, and we'll support \\-paths as
well as /-paths, but elsewhere only /-paths.

Shouldn't this just test "/", not "is_dir_sep()"?

At which point (if the above is correct) we could also call this
strbuf_rtrim_notchr(), and just call strbuf_rtrim_notchr(sb, '/') (but
even better would be a ctype-like callback).

diff --git a/bundle-uri.c b/bundle-uri.c
index 5914d220c43..c3ed04eae0f 100644
--- a/bundle-uri.c
+++ b/bundle-uri.c
@@ -192,20 +192,15 @@ int bundle_uri_parse_config_format(const char *uri,
 		.error_action = CONFIG_ERROR_ERROR,
 	};
 
 	if (!list->baseURI) {
 		struct strbuf baseURI = STRBUF_INIT;
 		strbuf_addstr(&baseURI, uri);
 
-		/*
-		 * If the URI does not end with a trailing slash, then
-		 * remove the filename portion of the path. This is
-		 * important for relative URIs.
-		 */
-		strbuf_strip_file_from_path(&baseURI);
+		strbuf_trim_trailing_not_dir_sep(&baseURI);
 		list->baseURI = strbuf_detach(&baseURI, NULL);
 	}
 	result = git_config_from_file_with_options(config_to_bundle_list,
 						   filename, list,
 						   &opts);
 
 	if (!result && list->mode == BUNDLE_MODE_NONE) {
diff --git a/strbuf.c b/strbuf.c
index 8d1e2e8bb61..3466552b854 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -117,14 +117,21 @@ void strbuf_rtrim(struct strbuf *sb)
 void strbuf_trim_trailing_dir_sep(struct strbuf *sb)
 {
 	while (sb->len > 0 && is_dir_sep((unsigned char)sb->buf[sb->len - 1]))
 		sb->len--;
 	sb->buf[sb->len] = '\0';
 }
 
+void strbuf_trim_trailing_not_dir_sep(struct strbuf *sb)
+{
+	while (sb->len > 0 && !is_dir_sep((unsigned char)sb->buf[sb->len - 1]))
+		sb->len--;
+	sb->buf[sb->len] = '\0';
+}
+
 void strbuf_trim_trailing_newline(struct strbuf *sb)
 {
 	if (sb->len > 0 && sb->buf[sb->len - 1] == '\n') {
 		if (--sb->len > 0 && sb->buf[sb->len - 1] == '\r')
 			--sb->len;
 		sb->buf[sb->len] = '\0';
 	}
@@ -1196,16 +1203,7 @@ int strbuf_edit_interactively(struct strbuf *buffer, const char *path,
 			res = error_errno(_("could not edit '%s'"), path);
 		unlink(path);
 	}
 
 	free(path2);
 	return res;
 }
-
-size_t strbuf_strip_file_from_path(struct strbuf *buf)
-{
-	size_t len = buf->len;
-	size_t offset = offset_1st_component(buf->buf);
-	char *path_sep = find_last_dir_sep(buf->buf + offset);
-	strbuf_setlen(buf, path_sep ? path_sep - buf->buf + 1 : offset);
-	return len - buf->len;
-}
diff --git a/strbuf.h b/strbuf.h
index 4822b713786..b5929ecc8dd 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -185,14 +185,16 @@ static inline void strbuf_setlen(struct strbuf *sb, size_t len)
  */
 void strbuf_trim(struct strbuf *sb);
 void strbuf_rtrim(struct strbuf *sb);
 void strbuf_ltrim(struct strbuf *sb);
 
 /* Strip trailing directory separators */
 void strbuf_trim_trailing_dir_sep(struct strbuf *sb);
+/* Strip trailing non-directory separators */
+void strbuf_trim_trailing_not_dir_sep(struct strbuf *sb);
 
 /* Strip trailing LF or CR/LF */
 void strbuf_trim_trailing_newline(struct strbuf *sb);
 
 /**
  * Replace the contents of the strbuf with a reencoded form.  Returns -1
  * on error, 0 on success.