On Wed, Nov 16 2022, Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee <derrickstolee@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> ---
> 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.
