Re: [Bug-gnubg] Patch: File associations under Windows and spaces in filenames

[Holger]

> On Mon 25 Nov 2002 (01:37 +0100), Holger wrote:
> > Hi,
> > 
> > The attached patch makes file associations under Windows
> (double-clicking
> > on a match) work and provides the ability to load matches with
> whitespace
> > characters in the path or filename.
> > 
> > Since it's my first source code contribution and I certainly don't have
> the
> > programming experience and insight to the GNUbg source like you
> developers
> > I'd like to ask you to check and test the patch thoroughly. Of course
> I've
> > tried not to break anything, but you never know.
> > 
> > The patch is against the 021121 snapshot.
> 
> A few suggestions, just from looking at the diffs (please don't take
> these as criticisms, there are things here that everyone should keep
> in mind and, at least in some cases, I've failed to follow my own
> advice):

I am really grateful and appreciate your advices. I just didn't know those
things.

> Using diff:
> 
> 1) make a copy of the original (before editing) file - Unix users
>    would usually use gnubg.c.orig, I don't know if Windows allows
>    multiple full stops in file names.

Yes, Windows allows this. I will do this.

> 2) Diff output should be the result of (for example):
> 
>    diff -b -w -u gnubg.c.orig gnubg.c > gnubg.c.diff
> 
>    the -b option tells diff not to generate output for changes which
>    are only blank lines (usually these are irrelevant and simply
>    clutter up the patch).
> 
>    the -w option tells diff not to generate output for changes which
>    are only changes in white space (if your editor changes spaces to
>    tabs or vice-versa for example). Again, usually these are
>    irrelevant).
> 
>    the -u option is the most important - unified diffs are generally
>    easier to look at to see what's changed than the old context style
>    diffs (my opinion, but I think it's widely shared).

I'm not sure whether I've missed the option or I don't have it in my diff
(old Cygwin version). I'll have another look.

>    In the case of the diffs you submitted, running them against the
>    current CVS put the new local variables szQuoted et al into the
>    middle of the usage text, as there was no context from diff.

I was hoping that it would be enough to state the snapshot date (from
alpha.gnu.org). I will use the IDs from now on.

> 3) this may or may not meet with agreement from others, and I've
>    screwed up slightly before in doing this. In order to make it very
>    clear which version of a routine you have edited (021121 snapshot
>    isn't necessarily specific enough), you can do the following:
> 
>    In your edited version, find the CVS $ID line and remove everything
>    except $Id$. Now the diff output will contain the contents of the
>    $Id$ line from the original as part of the changes, so there's no
>    question of where it came from. When it's checked into CVS, the ID
>    line will be regenerated with the new version no.

Nifty.

> Other comments:
> 
> 1) When you added the --datadir option to the getopt_long call, you
>    missed the comments above, which should be changed to reflect the
>    change to the code. It's a mistake every programmer makes, but it
>    leads to a support nightmare. There's a saying somewhere
>    (Programming Pearls II perhaps) to the effect:
> 
>    "If the comments and the code disagree, it is likely that both are
>    wrong."

Indeed, sorry. Like I said before, my programming experience is not that
huge and zero in regard to public projects. So, please be patient with me.
However, I'm really willing to improve and learn from you.

> 2) I don't have a Windows environment, so I don't know if it supplies
>    PATH_MAX, but Unix environments don't supply _MAX_PATH. I added a

I had thought about this earlier. But then I've seen it in MinGW, too, (and
not only in the Visual Studio help) and supposed that it would exist under
Unix as well. Unfortunately, a wrong guess.

If you or somebody else could recommend a comprehensive, downloadable
manual/reference on what is portably usable I'd be very grateful.

>    conditional directive to work around this:
> 
>   #ifdef WIN32
>   #define BIG_PATH _MAX_PATH
>   #else
>   #define BIG_PATH PATH_MAX
>   #endif
> 
>   and changed the declarations to use BIG_PATH. If the Windows
>   environment supplies PATH_MAX, then we should use that instead of this
>   workaround.

I'll have a look at home whether there is also PATH_MAX.

> 3) Again, this is my opinion only: 
>    I would strongly recommend against using C++ style comments ('//
>    text') and only use /*..*/ ones. We're much better off staying with
>    ANSI C. The option is going to C++, but I don't think that that's
>    desireable/possible. I notice other developers who are also doing
>    this, but this really limits any build to being done with gcc or a
>    similarly tolerant compiler. The person trying to build a text only
>    version on an obscure platform (or with a compiler optimised for
>    his hardware) will thank us all for not expecting the compiler to
>    accept this.

I've never learnt pure ANSI C. That's why. C++ style comments are much more
convenient. But of course I agree with you.

> Attached is an updated version of the diffs, I have committed this,
> along with a fix to set.c (unrelated problem - under FreeBSD,

Thanks. I hope my patch is worth your trust.

Regards,

        Holger

-- 
+++ GMX - Mail, Messaging & more  http://www.gmx.net +++
NEU: Mit GMX ins Internet. Rund um die Uhr für 1 ct/ Min. surfen!