Subject: Re: bug#6602: [PATCH] make dist target for Windows

From: Juanma Barranquero <[email-masked]>
Date: Sat, 24 Jul 2010 15:28:41 +0200

On Sat, Jul 24, 2010 at 05:18, Christoph <[email-masked]> wrote:

> Find attached the new patch for the `make dist' target.
>
> I added a Changelog and NEWS entry and removed README.W32 and makedist.bat
> files from the nt/ directory.

OK, thaks a lot.

A few comments (nitpicks, mostly):

 - You've added the ChangeLog entry to the root directory's ChangeLog,
instead of nt/ChangeLog. Also, it lacks an entry in admin/ChangeLog
about the changes in that directory (you've documented most of them in
the other ChangeLog, though). In some cases, as when moving files, it
is customary to put a "Moved to X/" entry in the old directory's
ChangeLog, and ad "Moved from Y/" in the new's.

- Changes to documentation files, like etc/NEWS and nt/INSTALL, must
also be documented in ChangeLog entries.

- If you're going to move a file, like README.W32, instead of

    cd admin/nt
    copy README.W32 ../../nt
    del README.W32

  or

    move admin/nt/README.W32 nt/

  please do

    bzr mv admin/nt/README.W32 nt/

  so Bazaar knows that it is the same file.

- It's best if you do these changes in a branch, and then submit a
bundle. For example, applying your patch creates a file
nt/zipdist.bat, but it has to be "bzr add"ed manually.

- Try not to introduce trailing whitespace in files you modify. Not
earth-shaking, but ugly if you have `show-trailing-whitespace' set to
t (which I recommend).

That said, I'm committing your change right now. Great work. Thanks!

    Juanma