On Wed, Dec 07 2022, Derrick Stolee wrote:
> 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@xxxxxxxxxxxxxxxxxxx/
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@xxxxxxxxxxxxxxxxxxx/
