Re: [PATCH v3 08/11] strbuf: introduce strbuf_strip_file_from_path()

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

On Tue, Dec 06 2022, Ævar Arnfjörð Bjarmason wrote:

> On Mon, Dec 05 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.
>>
>> Signed-off-by: Derrick Stolee <derrickstolee@github.com>
>> ---
>>  strbuf.c |  6 ++++++
>>  strbuf.h | 11 +++++++++++
>>  2 files changed, 17 insertions(+)
>>
>> diff --git a/strbuf.c b/strbuf.c
>> index 0890b1405c5..c383f41a3c5 100644
>> --- a/strbuf.c
>> +++ b/strbuf.c
>> @@ -1200,3 +1200,9 @@ int strbuf_edit_interactively(struct strbuf *buffer, const char *path,
>>  	free(path2);
>>  	return res;
>>  }
>> +
>> +void strbuf_strip_file_from_path(struct strbuf *sb)
>> +{
>> +	char *path_sep = find_last_dir_sep(sb->buf);
>> +	strbuf_setlen(sb, path_sep ? path_sep - sb->buf + 1 : 0);
>> +}
>> diff --git a/strbuf.h b/strbuf.h
>> index 76965a17d44..f6dbb9681ee 100644
>> --- a/strbuf.h
>> +++ b/strbuf.h
>> @@ -664,6 +664,17 @@ int launch_sequence_editor(const char *path, struct strbuf *buffer,
>>  int strbuf_edit_interactively(struct strbuf *buffer, const char *path,
>>  			      const char *const *env);
>>  
>> +/*
>> + * Remove the filename from the provided path string. If the path
>> + * contains a trailing separator, then the path is considered a directory
>> + * and nothing is modified.
>> + *
>> + * Examples:
>> + * - "/path/to/file" -> "/path/to/"
>> + * - "/path/to/dir/" -> "/path/to/dir/"
>> + */
>> +void strbuf_strip_file_from_path(struct strbuf *sb);
>> +
>>  void strbuf_add_lines(struct strbuf *sb,
>>  		      const char *prefix,
>>  		      const char *buf,
>
> Re your reply in
> https://lore.kernel.org/git/0980dcd4-30eb-4ef4-9369-279abe5ca5a2@github.com/
> I still don't get how this is different from a 1-byte change to
> strbuf_trim_trailing_dir_sep(), and if it isn't I think it's confusing
> API design to have two very different ways to return the same data.
>
> There you said "The difference is all about whether or not we start with
> a slash _and_ no other slash appears in the path.".
>
> But I can't find a case where there's any difference between the two. I
> tried this ad-hoc test on top:
> 	
> 	diff --git a/help.c b/help.c
> 	index f1e090a4428..b0866b01439 100644
> 	--- a/help.c
> 	+++ b/help.c
> 	@@ -765,6 +765,16 @@ int cmd_version(int argc, const char **argv, const char *prefix)
> 	 			 "also print build options"),
> 	 		OPT_END()
> 	 	};
> 	+	struct strbuf sb1 = STRBUF_INIT;
> 	+	struct strbuf sb2 = STRBUF_INIT;
> 	+
> 	+	if (getenv("STR")) {
> 	+		strbuf_addstr(&sb1, getenv("STR"));
> 	+		strbuf_addstr(&sb2, getenv("STR"));
> 	+		strbuf_strip_file_from_path(&sb1);
> 	+		strbuf_trim_trailing_not_dir_sep(&sb2);
> 	+		fprintf(stderr, "%s: %s | %s\n", strcmp(sb1.buf, sb2.buf) ? "NEQ" : "EQ", sb1.buf, sb2.buf);
> 	+	}
> 	 
> 	 	argc = parse_options(argc, argv, prefix, options, usage, 0);
> 	 
> 	diff --git a/strbuf.c b/strbuf.c
> 	index c383f41a3c5..f75d94556fc 100644
> 	--- a/strbuf.c
> 	+++ b/strbuf.c
> 	@@ -114,13 +114,23 @@ void strbuf_rtrim(struct strbuf *sb)
> 	 	sb->buf[sb->len] = '\0';
> 	 }
> 	 
> 	-void strbuf_trim_trailing_dir_sep(struct strbuf *sb)
> 	+static void strbuf_trim_trailing_dir_sep_1(struct strbuf *sb, int flip)
> 	 {
> 	-	while (sb->len > 0 && is_dir_sep((unsigned char)sb->buf[sb->len - 1]))
> 	+	while (sb->len > 0 && is_dir_sep((unsigned char)sb->buf[sb->len - 1]) - flip)
> 	 		sb->len--;
> 	 	sb->buf[sb->len] = '\0';
> 	 }
> 	 
> 	+void strbuf_trim_trailing_dir_sep(struct strbuf *sb)
> 	+{
> 	+	strbuf_trim_trailing_dir_sep_1(sb, 1);
> 	+}
> 	+
> 	+void strbuf_trim_trailing_not_dir_sep(struct strbuf *sb)
> 	+{
> 	+	strbuf_trim_trailing_dir_sep_1(sb, 1);
> 	+}
> 	+
> 	 void strbuf_trim_trailing_newline(struct strbuf *sb)
> 	 {
> 	 	if (sb->len > 0 && sb->buf[sb->len - 1] == '\n') {
> 	diff --git a/strbuf.h b/strbuf.h
> 	index f6dbb9681ee..b936f45ffad 100644
> 	--- a/strbuf.h
> 	+++ b/strbuf.h
> 	@@ -189,6 +189,8 @@ void strbuf_ltrim(struct strbuf *sb);
> 	 
> 	 /* Strip trailing directory separators */
> 	 void strbuf_trim_trailing_dir_sep(struct strbuf *sb);
> 	+/* Strip trailing not-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);
>
> Then:
> 	
> 	$ for str in a / b/ /c /d/ /e/ /f/g /h/i/ j/k l//m n/o/p //q/r/s/t; do STR=$str ./git version; done 2>&1 | grep :
> 	EQ:  | 
> 	EQ: / | /
> 	EQ: b/ | b/
> 	EQ: / | /
> 	EQ: /d/ | /d/
> 	EQ: /e/ | /e/
> 	EQ: /f/ | /f/
> 	EQ: /h/i/ | /h/i/
> 	EQ: j/ | j/
> 	EQ: l// | l//
> 	EQ: n/o/ | n/o/
> 	EQ: //q/r/s/ | //q/r/s/
>
> I.e. for those inputs it's the same as the existing
> strbuf_trim_trailing_dir_sep() with an inverted test. Is there some edge
> case that I'm missing?

FWIW the "overkill" change on top to do this via callbacks is the
below. Which I tested just to see how easy it was, and whether it would
fail your tests (it doesn't).

-- >8 --
Subject: [PATCH] strbuf: generalize "{,r,l}trim" to a callback interface

We've had all three variants of "trim" for isspace(), then since
c64a8d200f4 (worktree move: accept destination as directory,
2018-02-12) we've had a "is_dir_sep" variant.

A preceding change then added a "!is_dir_sep" variant. Let's
generalize this, and have all these functions that want to trim
characters matching some criteria be driven by the same logic.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 bundle-uri.c      |  7 +------
 git-compat-util.h |  5 +++++
 strbuf.c          | 44 ++++++++++++++++++++++++++++----------------
 strbuf.h          | 41 +++++++++++++++++++++++++++--------------
 4 files changed, 61 insertions(+), 36 deletions(-)

diff --git a/bundle-uri.c b/bundle-uri.c
index c411b871bdd..7240dedcaee 100644
--- a/bundle-uri.c
+++ b/bundle-uri.c
@@ -195,13 +195,8 @@ int bundle_uri_parse_config_format(const char *uri,
 	if (!list->baseURI) {
 		struct strbuf baseURI = STRBUF_INIT;
 		strbuf_addstr(&baseURI, uri);
+		strbuf_trim_trailing_not_dir_sep(&baseURI);
 
-		/*
-		 * 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);
 		list->baseURI = strbuf_detach(&baseURI, NULL);
 	}
 	result = git_config_from_file_with_options(config_to_bundle_list,
diff --git a/git-compat-util.h b/git-compat-util.h
index a76d0526f79..5bce9fa768c 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -451,6 +451,11 @@ static inline int git_is_dir_sep(int c)
 #define is_dir_sep git_is_dir_sep
 #endif
 
+static inline int is_not_dir_sep(int c)
+{
+	return !is_dir_sep(c);
+}
+
 #ifndef offset_1st_component
 static inline int git_offset_1st_component(const char *path)
 {
diff --git a/strbuf.c b/strbuf.c
index c383f41a3c5..a5a1c01d539 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -101,24 +101,37 @@ void strbuf_grow(struct strbuf *sb, size_t extra)
 		sb->buf[0] = '\0';
 }
 
-void strbuf_trim(struct strbuf *sb)
+void strbuf_trim_fn(struct strbuf *sb, strbuf_ctype_fn_t fn)
 {
-	strbuf_rtrim(sb);
-	strbuf_ltrim(sb);
+	strbuf_rtrim_fn(sb, fn);
+	strbuf_ltrim_fn(sb, fn);
 }
 
-void strbuf_rtrim(struct strbuf *sb)
+void strbuf_rtrim_fn(struct strbuf *sb, strbuf_ctype_fn_t fn)
 {
-	while (sb->len > 0 && isspace((unsigned char)sb->buf[sb->len - 1]))
+	while (sb->len > 0 && fn((unsigned char)sb->buf[sb->len - 1]))
 		sb->len--;
 	sb->buf[sb->len] = '\0';
 }
 
+void strbuf_trim(struct strbuf *sb)
+{
+	strbuf_trim_fn(sb, strbuf_ctype_isspace);
+}
+
+void strbuf_rtrim(struct strbuf *sb)
+{
+	strbuf_rtrim_fn(sb, strbuf_ctype_isspace);
+}
+
 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';
+	strbuf_rtrim_fn(sb, is_dir_sep);
+}
+
+void strbuf_trim_trailing_not_dir_sep(struct strbuf *sb)
+{
+	strbuf_rtrim_fn(sb, is_not_dir_sep);
 }
 
 void strbuf_trim_trailing_newline(struct strbuf *sb)
@@ -130,10 +143,10 @@ void strbuf_trim_trailing_newline(struct strbuf *sb)
 	}
 }
 
-void strbuf_ltrim(struct strbuf *sb)
+void strbuf_ltrim_fn(struct strbuf *sb, strbuf_ctype_fn_t fn)
 {
 	char *b = sb->buf;
-	while (sb->len > 0 && isspace(*b)) {
+	while (sb->len > 0 && fn(*b)) {
 		b++;
 		sb->len--;
 	}
@@ -141,6 +154,11 @@ void strbuf_ltrim(struct strbuf *sb)
 	sb->buf[sb->len] = '\0';
 }
 
+void strbuf_ltrim(struct strbuf *sb)
+{
+	strbuf_ltrim_fn(sb, strbuf_ctype_isspace);
+}
+
 int strbuf_reencode(struct strbuf *sb, const char *from, const char *to)
 {
 	char *out;
@@ -1200,9 +1218,3 @@ int strbuf_edit_interactively(struct strbuf *buffer, const char *path,
 	free(path2);
 	return res;
 }
-
-void strbuf_strip_file_from_path(struct strbuf *sb)
-{
-	char *path_sep = find_last_dir_sep(sb->buf);
-	strbuf_setlen(sb, path_sep ? path_sep - sb->buf + 1 : 0);
-}
diff --git a/strbuf.h b/strbuf.h
index f6dbb9681ee..bb7aa38816f 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -180,15 +180,39 @@ static inline void strbuf_setlen(struct strbuf *sb, size_t len)
  */
 
 /**
- * Strip whitespace from the beginning (`ltrim`), end (`rtrim`), or both side
- * (`trim`) of a string.
+ * A callback function that acts like the macros defined in
+ * <ctype.h>. To be given to strbuf_{,r,l}trim() below.
+ */
+typedef int (*strbuf_ctype_fn_t)(int c);
+static inline int strbuf_ctype_isspace(int c) { return isspace(c); }
+
+/**
+ * Strip characters matching the 'strbuf_ctype_fn_t' from the
+ * beginning (`ltrim`), end (`rtrim`) or both sides (`trim`) of a
+ * string.
+ */
+void strbuf_trim_fn(struct strbuf *sb, strbuf_ctype_fn_t fn);
+void strbuf_rtrim_fn(struct strbuf *sb, strbuf_ctype_fn_t fn);
+void strbuf_ltrim_fn(struct strbuf *sb, strbuf_ctype_fn_t fn);
+
+/**
+ * The common-case wrapper for strbuf_{,r,l}trim_fn() uses the
+ * strbuf_ctype_isspace() callback function.
  */
 void strbuf_trim(struct strbuf *sb);
 void strbuf_rtrim(struct strbuf *sb);
 void strbuf_ltrim(struct strbuf *sb);
 
-/* Strip trailing directory separators */
+/**
+ * Strip trailing directory separators. This is strbuf_rtrim_fn() with
+ * is_dir_sep() as the callback..
+ */
 void strbuf_trim_trailing_dir_sep(struct strbuf *sb);
+/**
+ * Strip trailing not-directory separators. This is strbuf_rtrim_fn()
+ * with is_not_dir_sep() as the callback.
+ */
+void strbuf_trim_trailing_not_dir_sep(struct strbuf *sb);
 
 /* Strip trailing LF or CR/LF */
 void strbuf_trim_trailing_newline(struct strbuf *sb);
@@ -664,17 +688,6 @@ int launch_sequence_editor(const char *path, struct strbuf *buffer,
 int strbuf_edit_interactively(struct strbuf *buffer, const char *path,
 			      const char *const *env);
 
-/*
- * Remove the filename from the provided path string. If the path
- * contains a trailing separator, then the path is considered a directory
- * and nothing is modified.
- *
- * Examples:
- * - "/path/to/file" -> "/path/to/"
- * - "/path/to/dir/" -> "/path/to/dir/"
- */
-void strbuf_strip_file_from_path(struct strbuf *sb);
-
 void strbuf_add_lines(struct strbuf *sb,
 		      const char *prefix,
 		      const char *buf,
-- 
2.39.0.rc1.1014.gc37e9814e18