--exclude-from works but "exclude from" in rsyncd.conf doesn't ?

[jw schultz]

On Thu, Jun 26, 2003 at 12:51:03PM -0700, Wayne Davison wrote:
> I'm finally getting back to finishing up the testing for my patch of the
> "exclude from" handling.  The main thing I had on my list was checking
> that --delete-excluded worked right, prompted by your comments:
[snip]
> Sounds good.  I've also improved the args to make_file().  Here's the
> latest patch:
> 
>     http://www.blorf.net/rsync-daemon-exclude.patch
> 
> I think this one is ready to be committed.  If anyone disagrees, let me
> know.

It's been almost two months so i'm even rustier on you patch
that you could be.  I'll jot these down as they occur to me.
Some may be just niggling.  

I notice some code formatting changes, that does tend to
obscure things a tiny bit.

| -       add_exclude_file(p, 1, 1);
| +       add_exclude_file(&server_exclude_list, p, 1, 1);

I know it isn't your patch adding it but these literals are
really more opaque than the code deserves.  I'm thinking
we/i should replace them with #defines that indicate
meaning.  As i glance through your patch i haven't a clue what
either of those 1s mean and i know this is only the tip of
the iceberg.  There are flag arguments of 1, 0 and -1 all
through the code.

|         if (verbose >= 2)
|                 rprintf(FINFO, "%s %s %s because of pattern %s%s\n",
|                         ent->include ? "including" : "excluding",
| -                       S_ISDIR(st->st_mode) ? "directory" : "file",
| +                       name_is_dir ? "directory" : "file",
|                         name, ent->pattern,
|                         ent->directory ? "/" : "");

We probably shouldn't be reporting to the client that we
have matched a server-side pattern.  I'd say a TODO style
comment would be OK for now while this is is testing.

| -       if (check_exclude_file(f, fname, &st))
| +       if (check_exclude_file(fname, S_ISDIR(st.st_mode) != 0, exclude_level))

Is != 0 necessary here?  I can see using S_ISDIR(),
!S_ISDIR(),  or as a unlikely possibliltiy !!S_ISDIR() if
you absolutely need the results of a logical operator.
Maybe it is just me but i see no sense to != 0 unless you
are inverting a test for 0 in an arithmatic just as you
might test for != 6.  I'm sure the compiler will optimize it
away but it is still ugly.

| + int check_exclude(struct exclude_struct **list, char *name, int name_is_dir)
| +{
| +       return 0 && list && name && name_is_dir;
|  }

Maybe i'm having a brain-fart but how does this ever return
anything but 0?

The patch looks like it contains some unrelated refactoring
work in util.c.  Can we isolate that out please?

I haven't given the patch quite the attention it probably
deserves but i trust my earlier assesment.



-- 
________________________________________________________________
	J.W. Schultz            Pegasystems Technologies
	email address:		jw at pegasys.ws

		Remember Cernan and Schmitt