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

[Jim Segrave]

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):

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.

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).

   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.

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.

   Of course doing something wrong, as I did recently and putting /*
   $Id $ (note the extra space causes CVS not to put Id's in at all,
   but in general, I think this is helpful. See the new version of
   your diffs below - the first change tells what version of gnubg.c
   the diff is agains.

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."

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
   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.

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.

Attached is an updated version of the diffs, I have committed this,
along with a fix to set.c (unrelated problem - under FreeBSD,
including sys/resource.h requires including sys/time.h, and changes to
Makefile.am - makebearoff1 needs to explicitly link with getopt and
getopt1, makebearoff.c had an extraneous single quote, causing a core dump)


-- 
Jim Segrave           address@hidden

diffs.out.gz
Description: application/gunzip