On Thu, May 07, 2026 at 12:02:08PM -0400, Derek Martin wrote: > Updated patch attached. I think I addressed everything you mentioned. > Tests still pass.
A little nitpicking below. > $ ./mutt > Path 1 Path 2 Expected Result Derek's Version Status Old > Version Status > ---------------------------------------------------------------------------------------- > "foo/bar" "baz" foo/bar/baz "foo/bar/baz" PASS > "foo/bar/baz" PASS > "foo/bar/" "baz" foo/bar/baz "foo/bar/baz" PASS > "foo/bar/baz" PASS > "foo/bar/" "/baz" foo/bar/baz "foo/bar/baz" PASS > "foo/bar//baz" FAIL > "" "/baz" /baz "/baz" PASS > "//baz" FAIL > "/" "foo" /foo "/foo" PASS "/foo" > PASS > "/" "/foo" /foo "/foo" PASS > "//foo" FAIL > "foo/bar" "" foo/bar "foo/bar" PASS > "foo/bar" PASS > > > -- > Derek D. Martin http://www.pizzashack.org/ GPG Key ID: 0xDFBEAD02 > -=-=-=-=- > This message is posted from an invalid address. Replying to it will result in > undeliverable mail due to spam prevention. Sorry for the inconvenience. > > diff --git a/muttlib.c b/muttlib.c > index 59a48378..d6636a3f 100644 > --- a/muttlib.c > +++ b/muttlib.c > @@ -1389,12 +1389,37 @@ void mutt_safe_path(BUFFER *dest, ADDRESS *a) > > void mutt_buffer_concat_path(BUFFER *d, const char *dir, const char *fname) > { > - const char *fmt = "%s/%s"; > - > - if (!*fname || (*dir && dir[strlen(dir)-1] == '/')) > - fmt = "%s%s"; > + mutt_buffer_clear(d); > + if (!*dir) > + { > + /* dir is empty, just append the file name */ This comment ... > + mutt_buffer_addstr(d, fname); > + } > + else > + { > + /* dir is not empty, append it */ ... and this ... > + mutt_buffer_addstr(d, dir); > > - mutt_buffer_printf(d, fmt, dir, fname); > + /* Next, sort out appending the file name with or without a slash */ > + bool dir_ends_with_slash = dir[strlen(dir) - 1] == '/'; > + bool fname_starts_with_slash = *fname && fname[0] == '/'; > + /* if fname is nonempty, dir doesn't end with a slash, and fname doesn't > + * start with a slash, append one */ ... and this ... > + if (!dir_ends_with_slash && *fname && !(fname_starts_with_slash)) ^ Why the parens around fname_starts_with_slash? > + { > + mutt_buffer_addch(d, '/'); > + } > + else if (dir_ends_with_slash && fname_starts_with_slash) > + { > + /* both have slashes; skip leading slashes of fname */ ... and this ... > + while (*fname && *fname == '/') fname++; > + } > + /* if fname is not empty, append it */ ... and also this comment just describe what the code says. For me this just clutters the code and adds no value. > + if (*fname) > + { > + mutt_buffer_addstr(d, fname); Do we really need this check? Adding an empty string should do no harm and would IMHO improve readability. If this is a common pattern we might better return early from mutt_buffer_addstr if we get an empty string. If we don't care about adding empty strings we could just do (untested): #v+ void mutt_buffer_concat_path(BUFFER *d, const char *dir, const char *fname) { mutt_buffer_clear(d); mutt_buffer_addstr(d, dir); if (*dir && dir[strlen(dir)-1 != '/') mutt_buffer_addch(d, '/'); while (*fname && *fname == '/') fname++; mutt_buffer_addstr(d, fname); } #v- As I said, just nitpicking/bikeshedding while drinking coffee, going back to do real work. ;-) Feel free to ignore me.
