You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by rb...@covalent.net on 2000/07/03 17:43:04 UTC
Re: cvs commit: apache-2.0/src/lib/apr/file_io/win32 filestat.c
On 3 Jul 2000 gstein@locus.apache.org wrote:
> gstein 00/07/03 05:06:40
>
> Modified: src/lib/apr/include apr_file_io.h
> src/lib/apr/file_io/os2 filestat.c
> src/lib/apr/file_io/unix dir.c fileacc.c fileio.h filestat.c
> open.c pipe.c
> src/lib/apr/file_io/win32 filestat.c
> Log:
> add ap_finfo_t.device
> add ap_setfileperms() for setting file permissions (chmod cover).
> - OS/2 and Win32 currently return APR_ENOTIMPL
> fix the file perm handling in APR: some conversion between ap_fileperms_t
> and mode_t was not occurring; adding new conversion function; renamed
> old conversion func.
>
Why were these done in one commit? These are three VERY distinct
patches. I would also like to understand why the function was
renamed. This looks like a personal preference as far as the name goes,
which IMHO is not a great reason for the change. It isn't worth it to
rename it back, but this sure looks like a change for the sake of change.
Ryan
_______________________________________________________________________________
Ryan Bloom rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------
Re: cvs commit: apache-2.0/src/lib/apr/file_io/win32 filestat.c
Posted by Greg Stein <gs...@lyra.org>.
On Wed, Jul 05, 2000 at 04:37:53PM +0100, Tony Finch wrote:
> Greg Stein <gs...@lyra.org> wrote:
> >On Mon, Jul 03, 2000 at 08:43:04AM -0700, rbb@covalent.net wrote:
> >>
> >> Why were these done in one commit? These are three VERY distinct
> >> patches.
> >
> >It was small and easy work, easy to review, and it was all the same set of
> >files. Also, I've seen much larger and disparate changes checked in as a
> >batch before; this didn't even seem close to those.
>
> Just because people have done the wrong thing in the past doesn't mean
> the mistake should be repeated.
Why is this such a big deal? We're talking a few dozen lines.
Next time, I'll check it as "APR changes needed for DAV." There. It is now a
"group of related changes" (per the dev guidelines).
Honestly guys... I break up my changes in many cases (e.g. in this case, I
checked in the APR changes as a unit, then the DAV changes as a unit). Also
for this case, the set of files was the same and it all went in together,
after testing the set with the DAV changes.
I really don't see the big deal here, other than simply getting a feeling of
being singled out and harshed on.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
Re: cvs commit: apache-2.0/src/lib/apr/file_io/win32 filestat.c
Posted by Tony Finch <do...@dotat.at>.
Greg Stein <gs...@lyra.org> wrote:
>On Mon, Jul 03, 2000 at 08:43:04AM -0700, rbb@covalent.net wrote:
>>
>> Why were these done in one commit? These are three VERY distinct
>> patches.
>
>It was small and easy work, easy to review, and it was all the same set of
>files. Also, I've seen much larger and disparate changes checked in as a
>batch before; this didn't even seem close to those.
Just because people have done the wrong thing in the past doesn't mean
the mistake should be repeated.
Tony.
--
f.a.n.finch fanf@covalent.net dot@dotat.at
329 itch-fighting cortex ointment
Re: cvs commit: apache-2.0/src/lib/apr/file_io/win32 filestat.c
Posted by Greg Stein <gs...@lyra.org>.
On Mon, Jul 03, 2000 at 08:43:04AM -0700, rbb@covalent.net wrote:
> On 3 Jul 2000 gstein@locus.apache.org wrote:
> > gstein 00/07/03 05:06:40
> >
> > Modified: src/lib/apr/include apr_file_io.h
> > src/lib/apr/file_io/os2 filestat.c
> > src/lib/apr/file_io/unix dir.c fileacc.c fileio.h filestat.c
> > open.c pipe.c
> > src/lib/apr/file_io/win32 filestat.c
> > Log:
> > add ap_finfo_t.device
> > add ap_setfileperms() for setting file permissions (chmod cover).
> > - OS/2 and Win32 currently return APR_ENOTIMPL
> > fix the file perm handling in APR: some conversion between ap_fileperms_t
> > and mode_t was not occurring; adding new conversion function; renamed
> > old conversion func.
> >
>
> Why were these done in one commit? These are three VERY distinct
> patches.
It was small and easy work, easy to review, and it was all the same set of
files. Also, I've seen much larger and disparate changes checked in as a
batch before; this didn't even seem close to those.
> I would also like to understand why the function was
> renamed. This looks like a personal preference as far as the name goes,
> which IMHO is not a great reason for the change. It isn't worth it to
> rename it back, but this sure looks like a change for the sake of change.
I introduced a second function with the opposite semantics. The rename was
done to create a matched pair of function (ap_unix_perms2mode and
ap_unix_mode2perms). The name also made it clearer what the function did
(ap_unix_get_fileperms() returned a mode_t given an ap_fileperms_t; the new
name makes this a bit more obvious)
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
Re: cvs commit: apache-2.0/src/lib/apr/file_io/win32 filestat.c
Posted by Greg Stein <gs...@lyra.org>.
On Mon, Jul 03, 2000 at 08:43:04AM -0700, rbb@covalent.net wrote:
> On 3 Jul 2000 gstein@locus.apache.org wrote:
> > gstein 00/07/03 05:06:40
> >
> > Modified: src/lib/apr/include apr_file_io.h
> > src/lib/apr/file_io/os2 filestat.c
> > src/lib/apr/file_io/unix dir.c fileacc.c fileio.h filestat.c
> > open.c pipe.c
> > src/lib/apr/file_io/win32 filestat.c
> > Log:
> > add ap_finfo_t.device
> > add ap_setfileperms() for setting file permissions (chmod cover).
> > - OS/2 and Win32 currently return APR_ENOTIMPL
> > fix the file perm handling in APR: some conversion between ap_fileperms_t
> > and mode_t was not occurring; adding new conversion function; renamed
> > old conversion func.
> >
>
> Why were these done in one commit? These are three VERY distinct
> patches.
It was small and easy work, easy to review, and it was all the same set of
files. Also, I've seen much larger and disparate changes checked in as a
batch before; this didn't even seem close to those.
> I would also like to understand why the function was
> renamed. This looks like a personal preference as far as the name goes,
> which IMHO is not a great reason for the change. It isn't worth it to
> rename it back, but this sure looks like a change for the sake of change.
I introduced a second function with the opposite semantics. The rename was
done to create a matched pair of function (ap_unix_perms2mode and
ap_unix_mode2perms). The name also made it clearer what the function did
(ap_unix_get_fileperms() returned a mode_t given an ap_fileperms_t; the new
name makes this a bit more obvious)
Cheers,
-g
--
Greg Stein, http://www.lyra.org/