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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Dec 6, 2022, 10:06 UTC
Message-ID: <221206.86a640dda3.gmgdl@evledraar.gmail.com>
In-Reply-To: <1eec3426aee12bbd674ebd646075f0d4c0b1f5af.1670262639.git.gitgitgadget@gmail.com>

On Mon, Dec 05 2022, Derrick Stolee via GitGitGadget wrote:

Show 55 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.
>
> 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?