[parted-devel] [PATCH] next branch

[Joel Granados]

On Tue, May 26, 2009 at 12:19:50PM +0200, Jim Meyering wrote:
> Joel Granados Moreno wrote:
> > Hello list
> >
> > Moved the length and start maximum checks to the next branch.
> > Created an additional patch that puts all the logic in one place:
> > pt-tools.c
> >
> > As always, comments a greatly appreciated.
> 
> Thank you.
> I've applied those two to "next" with minor changes, rebased all of next
> and pushed the result.
> 
> I adjusted their commit log comments as follows,
> and corrected some comments not to be off-by-one.  i.e.,
> 
>     We have to ensure that the values are *smaller* than 2^32.
>     The old wording implied that only larger was a problem,
>     while the value 2^32 is not valid.

True, thx for that..

> 
> Regarding the logs,
> when you add a new function, just saying "New function."
> is enough, because there has to be a comment in the
> code already saying what the function does.

This is true. But I have to insist on a small description, instead of
just saying "New function" as the diff that accompanies the commit might
not be as readable or as obvious.  With that said, I'm OK with that
change.


> It's useful to write more when describing code-movement style
> changes, as you did for other parts.
> 
>     commit 51955589159dab48f1574eaab66a4107edd0bf78
>     Author: Joel Granados Moreno <jgranado at redhat.com>
>     Date:   Mon May 18 17:37:44 2009 +0200
> 
>         put the maximum sector checks in pt-tools.c
> 
>         * libparted/labels/dos.c (msdos_partition_check): Use new
>         ptt_partition_max_start_len function to test for len and start maxs.
>         * libparted/labels/dvh.c (msdos_partition_check): Likewise.
>         * libparted/labels/pt-tools.c (ptt_partition_max_start_len):
>         New function.
>         * libparted/labels/pt-tools.h: Likewise.
>         * po/POTFILES.in: Add pt-tools.c to the translation list.
> 
>     commit 5c9a033a1cabb95bffa7e3f33356c7bd349bebea
>     Author: Joel Granados Moreno <jgranado at redhat.com>
>     Date:   Mon May 18 17:37:43 2009 +0200
> 
>         put partition-table-specific logic in the corresponding files
> 
>         * include/parted/disk.h (struct _PedDiskOps) [partition_check]:
>         New member function.
>         * libparted/disk.c (_check_partition): Replace open-coded tests with
>         use of the new ->partition_check().
>         (_partition_max_start, _partition_max_len): Remove functions.
> 
>         * libparted/labels/dvh.c (dvh_partition_check): New function.
>         * libparted/labels/dos.c (dos_partition_check): Likewise.
>         * libparted/labels/aix.c (aix_partition_check): New stub.
>         * libparted/labels/bsd.c (bsd_partition_check): Likewise.
>         * libparted/labels/gpt.c (gpt_partition_check): Likewise.
>         * libparted/labels/loop.c (loop_partition_check): Likewise.
>         * libparted/labels/mac.c (mac_partition_check): Likewise.
>         * libparted/labels/pc98.c (pc98_partition_check): Likewise.
>         * libparted/labels/rdb.c (rdb_partition_check): Likewise.
>         * libparted/labels/sun.c (sun_partition_check): Likewise.
> 
> Regarding code changes, I tweaked cpp indentation to use
> one space, not two, and added curly braces around a
> single-statement-but-multi-line loop body.

-- 
Joel Andres Granados
Brno, Czech Republic, Red Hat.