Re: [PATCH v2 6/9] strbuf: introduce strbuf_strip_file_from_path()
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- Dec 2, 2022, 18:32 UTC
- Message-ID
- <221202.86bkolfwzv.gmgdl@evledraar.gmail.com>
- In-Reply-To
- <7d86852c01513ee131bf993302416f4c881a0bc6.1668628303.git.gitgitgadget@gmail.com>
On Wed, Nov 16 2022, Derrick Stolee via GitGitGadget wrote:
Show 30 quoted lines
> 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.