On Wed, May 13, 2009 at 10:21:47AM +0200, Jean Delvare wrote:
> On Wed, 13 May 2009 10:09:43 +0200, Andre Prendel wrote:
> > On Tue, May 12, 2009 at 09:43:20PM +0200, Jean Delvare wrote:
> > > On Mon, 11 May 2009 18:04:11 +0200, Andre Prendel wrote:
> > > > This patch does some refactoring of the loadConfig()
> > > > function.
> > > >
> > > > * Simplifying the conditions makes code flow clearer and eliminates
> > > > long lines (> 80 chars).
> > > > * Removed useless stat() call.
> > > > * Return -1 in error case, instead of several positive values (never defined).
> > > >
> > > > Changes in v2:
> > > >
> > > > Blank line before the first #include.
> > > > Fix typo.
> > > > Bring back logging (reload configuration).
> > > > Consistent error messages.
> > > > Drop reading configuration from stdin.
> > > > Fix logging (if fopen() fails).
> > > > Cleanup sensors before reloading configuration.
> > > > Fix compile warning.
> > > > Don't print error value if reloadLib() fails.
> > >
> > > Looks almost OK, just one minor issue:
> > >
> > > > ---
> > > >
> > > > lib.c | 74 +++++++++++++++++++++++++++++++-------------------------------
> > > > sensord.c | 4 +--
> > > > 2 files changed, 40 insertions(+), 38 deletions(-)
> > > >
> > > > Index: quilt-sensors/prog/sensord/lib.c
> > > > ===================================================================
> > > > --- quilt-sensors.orig/prog/sensord/lib.c 2009-04-26 22:11:03.000000000 +0200
> > > > +++ quilt-sensors/prog/sensord/lib.c 2009-04-26 22:13:09.000000000 +0200
> > > > (...)
> > > > + /* Load default configuration. */
> > > > + if (!cfgPath) {
> > > > + if (reload) {
> > > > sensorLog(LOG_INFO, "configuration reloading");
> > > > - sensors_cleanup();
> > > > + sensors_cleanup();
> > >
> > > You're adding leading white spaces!
> >
> > Hi Jean,
> >
> > I really cannot see any whitespaces. What do you mean?
>
> I mean:
>
> <minus><tab><tab><tab>sensors_cleanup();
> <plus><space><space><tab><tab><tab>sensors_cleanup();
>
> The two spaces shouldn't be there. As a matter of fact, the effective
> content of this line doesn't change, so it shouldn't show up in the
> diff... unless there are invisible whitespace changes.
>
Ah, now I got it, thanks. I didn't see the two spaces (have trusted the
Emacs indentation). I'll fix that before committing.
BTW, how do you review patches? In your mailclient or in external
tool? Sometimes diffs are very difficult to read.
Thank,
Andre
> --
> Jean Delvare
