Re: [PATCH] concat_dir_and_file() needs fixes

[Jindrich Novy <jnovy redhat com>]

Hello Roland,

On Fri, 2005-12-02 at 15:58 +0100, Roland Illig wrote: 
> Jindrich Novy wrote:
> > --- mc-4.6.1a/src/util.c.jn	2005-12-02 11:08:26.000000000 +0100
> > +++ mc-4.6.1a/src/util.c	2005-12-02 13:11:19.000000000 +0100
> > @@ -1515,9 +1515,16 @@
> >  
> >  /* If filename is NULL, then we just append PATH_SEP to the dir */
> >  char *
> > -concat_dir_and_file (const char *dir, const char *file)
> > +concat_dir_and_file (const char *dir, const char *filename)
> >  {
> >      int i = strlen (dir);
> > +    const char *file = filename;
> > +    
> 
> It's obvious that you used mcedit here. :) (Hint: trailing white-space.)

Yes ;)

> > +    /* Return filename when dir is empty */
> > +    if (!i) return g_strdup (filename);
> 
> This looks almost good, except that "i" is not a boolean variable. You 
> should use "if (i == 0)" instead.

It's a question of style, !i and i==0 are equivalent, but feel free to
change it.

> > +    
> > +    if (file != NULL && *file == PATH_SEP)
> > +    	file++;
> 
> Maybe we should rather make sure that this function is never called with 
> non-empty "dir" and "file" starting with a slash. Otherwise we might 
> hide bugs. How often do you want to concatenate two absolute paths?

The problem isn't to concatenate two absolute paths actually. The patch
enhances the functionality of concat_dir_and_file() in the way that it
will correctly add say dir="/home/jnovy" and file="/.mc/bindings" to
"/home/jnovy/.mc/bindings" as the unfixed concat_dir_and_file checks
only whether the dir ends with PATH_SEP and adds it even if file begins
with PATH_SEP already -> the result is "/home/jnovy//.mc/bindings"
what's not too good. Fortunately the path works even if it's so ugly.

I thought that it would be less PITA to make concat_dir_and_file less
stupid than fixing the strings in edit.h:

/* File names */
#define EDIT_DIR           PATH_SEP_STR ".mc" PATH_SEP_STR "cedit"
#define SYNTAX_FILE        EDIT_DIR PATH_SEP_STR "Syntax"
#define CLIP_FILE          EDIT_DIR PATH_SEP_STR "cooledit.clip"
#define MACRO_FILE         EDIT_DIR PATH_SEP_STR "cooledit.macros"
#define BLOCK_FILE         EDIT_DIR PATH_SEP_STR "cooledit.block"
#define TEMP_FILE          EDIT_DIR PATH_SEP_STR "cooledit.temp"

used for concatenation, all of them beginning with PATH_SEP. Please note
that if you remove the leading PATH_SEP from these strings, you need to
rewrite almost all calls to catstrs(), edit_save_block() and friends by
at least adding the PATH_SEP_STR there.

Regards,
Jindrich
-- 
Jindrich Novy <jnovy redhat com>, http://people.redhat.com/jnovy/
(o_                                                           _o)
//\      The worst evil in the world is refusal to think.     //\
V_/_                                                         _\_V