>>>>> Edmund GRIMLEY EVANS writes:

 EGE> Where is the ChangeLog standard defined?

The GNU Standards: (standards)Change Log Concepts. and
(standards)Style of Change Logs.

In general, you'll find that people working on extremely GNUish
projects (such as the Hurd) are a lot more anal about style than other
projects.  The secret is to remember the purpose of a ChangeLog: if
you encounter a bug, it should be possible to grep the ChangeLog and
find a complete history of the feature that has the bug.

Here's a friendly little (actually, it turned out to be quite long)
critique of your style, so that you can understand why OKUJI made the
changes he did.  Please forgive my being pedantic, but how to write
ChangeLog entries has been a FAQ on this list:

        From Edmund GRIMLEY EVANS <[EMAIL PROTECTED]> adapting
        the Dresden version of GRUB:

Correct headers with the date are important; they allow us to check
out a version from CVS before the bug was introduced, so OKUJI wrote:

1999-09-13  Edmund GRIMLEY EVANS  <[EMAIL PROTECTED]>

        The netboot support in the Dresden version of GRUB is integrated.


        * netboot: New directory for BOOTP/TFTP support.

You don't need entries for directories, because adding a new directory
doesn't introduce bugs.

        * Makefile.am: Added netboot to SUBDIRS.

This is mainly a style issue.  `Makefile.am (SUBDIRS): Added
netboot.' is clearer to scan visually.

        * configure.in: Added --enable-tftp, --enable-nepci, etc.

It's important to list the full name of every feature.  Using `etc'
doesn't help when you're doing a search for `--enable-3c59x'.

OKUJI wrote:

        * configure.in (--enable-tftp): New option.
        (--enable-ne): Likewise.
        (--enable-nepci): Likewise.
        (--enable-wd): Likewise.
        [...]

Using `Likewise.' is an established convention, which reduces
redundancy without losing any information.  Some older ChangeLogs
would write this as:

        * configure.in (--enable-tftp, --enable-ne, --enable-nepci,
          --enable-wd, [...]): New option.

but we dropped that convention because it looked ugly. ;)

Most maintainers don't track changes to configure.in so carefully, but
here OKUJI is setting an example of how careful we should be:

        (NET_CFLAGS): New variable.
        (NET_EXTRAFLAGS): Likewise.
        Do AC_OUTPUT for netboot/Makefile as well.


        * stage1/stage1.S: Set the number of sectors for Stage 2 to 130.
        * stage1/stage1_lba.S: Set the number of sectors for Stage 2 to 130.

Aha!  Caught you both! ;)

The correct solution is to introduce names for these variables;
unnamed asm data is totally useless for ChangeLog entries.

So:

1999-09-14  Gordon Matzigkeit  <[EMAIL PROTECTED]>

        * stage1/stage1.S (blocklist_default_start): New label for default
        blocklist start sector.
        (blocklist_default_len): New label for default blocklist length.
        (blocklist_default_seg): New label for default blocklist segment.
        * stage1/stage1_lba.S (blocklist_default_start): Likewise.
        (blocklist_default_len): Likewise.
        (blocklist_default_seg): Likewise.

so your logs should have been:

        * stage1/stage1.S (blocklist_default_len): Set to 130.
        * stage1/stage1_lba.S (blocklist_default_len): Likewise.


        * stage2/Makefile.am: Added library ../netboot/libdrivers.a.

This entry should define the variable or rule where the library was
added.

        * stage2/asm.S (currticks): New function.
        * stage2/char_io.c (grub_sprintf): New function.
        * (grub_memcmp): New function.

Only use an asterisk when the function is in a different file.

Here, OKUJI demonstrates an advanced ChangeLog syntax that originated
with Emacs:

        * stage2/asm.S [!STAGE1_5] (currticks): New function.
        * stage2/char_io.c [!STAGE1_5] (grub_sprintf): Likewise.
        [!STAGE1_5] (grub_memcmp): Likewise.

The square brackets indicate that the change only applies when the
given C preprocessor macro is set (as usual, `!' stands for `not').
This way, it is possible to clearly see that your changes do not apply
when STAGE1_5 is defined.

        * stage2/disk_io.c (fsys_table): Added "tftp".
        * (sane_partition): Added network drive "nd" with number 0x20.
        * (real_open_partition): Likewise.
        * (set_device): Likewise.

(Again, no asterisks for multiple functions in the same file.)  It is
good to describe the actual change that you make, rather than just the
intent.  `Likewise' is really only good when the implementation
changes are identical, so OKUJI goes into more detail:

        * stage2/disk_io.c (fsys_table) [FSYS_TFTP]: Added an entry for
        tftp.
        (sane_partition) [!STAGE1_5]: If CURRENT_DRIVE is a network
        drive, return 1.
        (real_open_partition) [!STAGE1_5]: Likewise.
        (set_device): If DEVICE contains a network drive, set
        CURRENT_DRIVE to 0x20.


Now you wrote:

        * stage2/filesys.h: Added FSYS_TFTP.

OKUJI wrote:

        * stage2/filesys.h [FSYS_TFTP] (FSYS_TFTP_NUM): Defined as 1.
        [!FSYS_TFTP] (FSYS_TFTP_NUM): Defined as 0.

I think it's important to track declarations as well, so I would have
written:

        * stage2/filesys.h [FSYS_TFTP] (FSYS_TFTP_NUM): Define as 1.
        [FSYS_TFTP] (tftp_mount): Declare.
        [FSYS_TFTP] (tftp_read): Likewise.
        [FSYS_TFTP] (tftp_dir): Likewise.
        [!FSYS_TFTP] (FSYS_TFTP_NUM): Define as 0.


        * stage2/gunzip.c (gunzip_test_header): If filemax >=
        16L*1024*1024 do not try to examine the last 8 bytes of the
        file. This is required for compressed files by TFTP.

By convention, variable names should be capitalized.  This applies to
source code comments as well.  Writing 16MB is probably clearer, too.

        * stage2/shared.h: New functions from asm.S and char_io.c.

Again, we need the names of all the functions that were added.

        * stage2/size_test: Set the maximum size of Stage 2 to 66560.

Whew!  Hope you're still with me. ;)

And on an unrelated note:

 EGE> Should netboot/config.h be renamed, or should the compiler be
 EGE> getting different -I arguments, or should stage2/shared.h
 EGE> include "../config.h" instead of <config.h>?

I think netboot/config.h should be renamed.  In GNU packages, a file
that contains `#include <config.h>' should always get the
autoconf-generated header.

-- 
 Gordon Matzigkeit <[EMAIL PROTECTED]>  //\ I'm a FIG (http://www.fig.org/)
Committed to freedom and diversity \// I use GNU (http://www.gnu.org/)

Reply via email to