Re: mutt_buffer_concat_path is broken [PATCH: 1/1] Version 2

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.