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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Dec 8, 2022, 12:52 UTC
Message-ID: <221208.86k0326nse.gmgdl@evledraar.gmail.com>
In-Reply-To: <98ee4b5b-9d72-31fc-d5b1-1a978450973f@github.com>

On Wed, Dec 07 2022, Derrick Stolee wrote:

Show 12 quoted lines

> On 12/6/22 6:37 AM, Ævar Arnfjörð Bjarmason wrote:
>
>> 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
>
> I don't like this approach and think it distracts from the goal
> of the series. If you want to update it afterwards, then by all
> means go for it.

Yes, I think it shouldn't be part of this series at all.

I.e. the part in [1] that you're replying to is just something I poked at because you nerd-sniped me into poking at it.

I think it's probably not worth it, and I don't think I like the API[2]. What I do think is worth including in this series one way or another is [3].

You're proposing a new addition to the strbuf API. I think it's relevant feedback that you seem to be re-inventing close relative (literally a 1-byte difference!) of a function that's there already.

Or I'm just wrong, but then what input does your strbuf_strip_file_from_path() handle differently than the strbuf_trim_trailing_not_dir_sep() in [3]?

Making them sibling functions would make the API more discoverable. The comment you're adding would also improve the existing code. I.e. we could have this end-state in strbuf.h:

	/**
	 * Strip trailing directory separators, or not-directory separators.
	 *
	 * The "dir_sep" variant portably trims redundant slash(es) from the
	 * end, while the "not_dir_sep" gets you to the base directory, should
	 * the path refer to a file:
	 *
	 * |---------------+---------------+-------------------|
	 * | In            | out (dir_sep) | out (not_dir_sep) |
	 * |---------------+---------------+-------------------|
	 * | /path/to/file | /path/to/file | /path/to/         |
	 * | /path/to/dir/ | /path/to/dir  | /path/to/dir/     |
	 * |---------------+---------------+-------------------|
	 */
	void strbuf_trim_trailing_dir_sep(struct strbuf *sb);
	void strbuf_trim_trailing_not_dir_sep(struct strbuf *sb);

Or maybe you still think it's not worth it, I also think that's fine. I'd really appreciate knowing if it's a "yeah maybe they're the same, but I haven't checked", or if it's "I think you missed a case, but I haven't explained it to you".

Otherwise if I do follow-up I'd probably have to start by brute-force testing the two to satisfy my own paranoia :)

Thanks.

1. https://lore.kernel.org/git/221206.86wn74bw35.gmgdl@evledraar.gmail.com/
2. Although once I started poking at it I found a lot of cases in-tree
   where we hardcoded that exact loop (or the equivalent strbuf_setlen()
   variant), which could be replaced by "trim all cases of this
   character from the end of a strbuf", or "trim all cases of this
   character match ..." function.
3. https://lore.kernel.org/git/221206.86a640dda3.gmgdl@evledraar.gmail.com/