You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Edmund Wong <ed...@kdtc.net> on 2009/08/29 04:39:07 UTC
[PATCH] Renaming svn_eol_* functions to svn_eol__*.
Since the svn_eol_* functions are in the private
headers, Arfrever pointed out that they should be
renamed to s/svn_eol_/&_/g.
Log:
[[[
* subversion/libsvn_subr/eol.c,
subversion/libsvn_diff/diff_file.c,
subversion/tests/libsvn_subr/eol-test.c,
subversion/libsvn_client/patch.c,
subversion/include/private/svn_eol_private.h:
(svn_eol_find_eol_start, svn_eol_detect_eol,
svn_eol_detect_file_eol): Renamed from ...
* subversion/libsvn_subr/eol.c,
subversion/libsvn_diff/diff_file.c,
subversion/tests/libsvn_subr/eol-test.c,
subversion/libsvn_client/patch.c,
subversion/include/private/svn_eol_private.h:
(svn_eol__find_eol_start, svn_eol__detect_eol,
svn_eol__detect_file_eol): ... to.
Patch by: Edmund Wong ed <at> kdtc.net
Suggested by: arfrever
]]]
]]]
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2388443
Re: [PATCH] Renaming svn_eol_* functions to svn_eol__*.
Posted by Julian Foad <ju...@btopenworld.com>.
On Tue, 2009-09-01 at 09:58 +0800, Edmund Wong wrote:
> Stefan Sperling wrote:
> > On Sat, Aug 29, 2009 at 12:39:07PM +0800, Edmund Wong wrote:
> >> Since the svn_eol_* functions are in the private
> >> headers, Arfrever pointed out that they should be
> >> renamed to s/svn_eol_/&_/g.
> >
> > Some small remarks on the log message:
>
> Even after all these submitted patches, I still can't seem
> to get the log right. Quite a disappointment I must say. Sorry about
> that.
Pah. You're doing fine. Better than fine. We're not disappointed, but
rather extremely appreciative of your patches and log messages.
It's really really helpful for every log message to start with a brief
statement of the purpose of the change. That's important. It can make
the difference between other people being able to understand the change
and not being able to understand it.
It's also good to be tidy and consistent with spaces and colons and
commas and whatnot, but that's on a much, much lower level, and the
space taken up talking about in emails it is far greater than its
importance. It makes a small difference to readability but doesn't
usually affect the reader's understanding.
In cases like this patch of yours, experienced committers sometimes
judge that the purpose is so obvious that it's not worth mentioning, but
to avoid uncertainty it's better to mention it anyway.
- Julian
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2389719
Re: [PATCH] Renaming svn_eol_* functions to svn_eol__*.
Posted by Stefan Sperling <st...@elego.de>.
On Tue, Sep 01, 2009 at 09:58:56AM +0800, Edmund Wong wrote:
> Stefan Sperling wrote:
> > On Sat, Aug 29, 2009 at 12:39:07PM +0800, Edmund Wong wrote:
> >> Since the svn_eol_* functions are in the private
> >> headers, Arfrever pointed out that they should be
> >> renamed to s/svn_eol_/&_/g.
> >
> > Some small remarks on the log message:
>
> Even after all these submitted patches, I still can't seem
> to get the log right.
Don't worry.
Stefan
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2389723
Re: [PATCH] Renaming svn_eol_* functions to svn_eol__*.
Posted by Edmund Wong <ed...@kdtc.net>.
Stefan Sperling wrote:
> On Sat, Aug 29, 2009 at 12:39:07PM +0800, Edmund Wong wrote:
>> Since the svn_eol_* functions are in the private
>> headers, Arfrever pointed out that they should be
>> renamed to s/svn_eol_/&_/g.
>
> Some small remarks on the log message:
Even after all these submitted patches, I still can't seem
to get the log right. Quite a disappointment I must say. Sorry about
that.
> You might want to put a short note describing the overall change
> here, unless the change is really trivial. In this case, we could
> say:
>
> "Follow-up to r38983: The new eol functions are supposed to be private,
> not public, so rename them accordingly."
>> * subversion/libsvn_subr/eol.c,
>> subversion/libsvn_diff/diff_file.c,
>
> The asterisk alone is a good enough visual hint for the grouping.
> So the indentation isn't needed, and these can all go on the same
> line, like this:
>
> * subversion/libsvn_subr/eol.c,
> subversion/libsvn_diff/diff_file.c,
I'll keep these in mind.
Edmund
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2389610
Re: [PATCH] Renaming svn_eol_* functions to svn_eol__*.
Posted by "Hyrum K. Wright" <hy...@hyrumwright.org>.
On Aug 31, 2009, at 6:26 AM, Stefan Sperling wrote:
> On Mon, Aug 31, 2009 at 06:13:23AM -0500, Hyrum K. Wright wrote:
>> On Aug 31, 2009, at 6:06 AM, Stefan Sperling wrote:
>>
>>> On Mon, Aug 31, 2009 at 05:55:55AM -0500, Hyrum K. Wright wrote:
>>>>
>>>> On Aug 31, 2009, at 5:18 AM, Stefan Sperling wrote:
>>>>
>>>>> On Sat, Aug 29, 2009 at 12:39:07PM +0800, Edmund Wong wrote:
>>>>>> * subversion/libsvn_subr/eol.c,
>>>>>> subversion/libsvn_diff/diff_file.c,
>>>>>
>>>>> The asterisk alone is a good enough visual hint for the grouping.
>>>>> So the indentation isn't needed, and these can all go on the same
>>>>> line, like this:
>>>>>
>>>>> * subversion/libsvn_subr/eol.c,
>>>>> subversion/libsvn_diff/diff_file.c,
>>>>
>>>> Not to be pedantic (well, actually, I am being pedantic), but the
>>>> examples in HACKING include the indentation for each file in a
>>>> condensed file list. See
>>>> http://subversion.tigris.org/hacking.html#log-messages
>>>
>>> The indentation you are quoting me for there does not match what I
>>> sent.
>>> Maybe tigris is mangling my mail again?
>>>
>>> Let's try again.
>>>
>>> Good:
>>> * subversion/libsvn_subr/eol.c,
>>> subversion/libsvn_diff/diff_file.c,
>>>
>>> Bad:
>>> * subversion/libsvn_subr/eol.c,
>>> subversion/libsvn_diff/diff_file.c,
>>
>> And the "Bad" version is what is shown in HACKING as Good. :)
>
> That's because it came out wrong, again.
>
> What I sent was this:
>
> * blah
> <2 spaces>blah2
>
> * blah
> <3 spaces>blah2
>
> What you are quoting above looks like this:
>
> * blah
> <1 space>blah2
>
> * blah
> <2 spaces>blah2
Okay, then we're on the same page. Sorry for the confusion.
(And it could have just as easily been my mailer as tigris which
mangled you mail. Apologies if it was.)
-Hyrum
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2388882
Re: [PATCH] Renaming svn_eol_* functions to svn_eol__*.
Posted by Stefan Sperling <st...@elego.de>.
On Mon, Aug 31, 2009 at 06:13:23AM -0500, Hyrum K. Wright wrote:
> On Aug 31, 2009, at 6:06 AM, Stefan Sperling wrote:
>
> > On Mon, Aug 31, 2009 at 05:55:55AM -0500, Hyrum K. Wright wrote:
> >>
> >> On Aug 31, 2009, at 5:18 AM, Stefan Sperling wrote:
> >>
> >>> On Sat, Aug 29, 2009 at 12:39:07PM +0800, Edmund Wong wrote:
> >>>> * subversion/libsvn_subr/eol.c,
> >>>> subversion/libsvn_diff/diff_file.c,
> >>>
> >>> The asterisk alone is a good enough visual hint for the grouping.
> >>> So the indentation isn't needed, and these can all go on the same
> >>> line, like this:
> >>>
> >>> * subversion/libsvn_subr/eol.c,
> >>> subversion/libsvn_diff/diff_file.c,
> >>
> >> Not to be pedantic (well, actually, I am being pedantic), but the
> >> examples in HACKING include the indentation for each file in a
> >> condensed file list. See
> >> http://subversion.tigris.org/hacking.html#log-messages
> >
> > The indentation you are quoting me for there does not match what I
> > sent.
> > Maybe tigris is mangling my mail again?
> >
> > Let's try again.
> >
> > Good:
> > * subversion/libsvn_subr/eol.c,
> > subversion/libsvn_diff/diff_file.c,
> >
> > Bad:
> > * subversion/libsvn_subr/eol.c,
> > subversion/libsvn_diff/diff_file.c,
>
> And the "Bad" version is what is shown in HACKING as Good. :)
That's because it came out wrong, again.
What I sent was this:
* blah
<2 spaces>blah2
* blah
<3 spaces>blah2
What you are quoting above looks like this:
* blah
<1 space>blah2
* blah
<2 spaces>blah2
Stefan
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2388854
Re: [PATCH] Renaming svn_eol_* functions to svn_eol__*.
Posted by "Hyrum K. Wright" <hy...@hyrumwright.org>.
On Aug 31, 2009, at 6:06 AM, Stefan Sperling wrote:
> On Mon, Aug 31, 2009 at 05:55:55AM -0500, Hyrum K. Wright wrote:
>>
>> On Aug 31, 2009, at 5:18 AM, Stefan Sperling wrote:
>>
>>> On Sat, Aug 29, 2009 at 12:39:07PM +0800, Edmund Wong wrote:
>>>> * subversion/libsvn_subr/eol.c,
>>>> subversion/libsvn_diff/diff_file.c,
>>>
>>> The asterisk alone is a good enough visual hint for the grouping.
>>> So the indentation isn't needed, and these can all go on the same
>>> line, like this:
>>>
>>> * subversion/libsvn_subr/eol.c,
>>> subversion/libsvn_diff/diff_file.c,
>>
>> Not to be pedantic (well, actually, I am being pedantic), but the
>> examples in HACKING include the indentation for each file in a
>> condensed file list. See
>> http://subversion.tigris.org/hacking.html#log-messages
>
> The indentation you are quoting me for there does not match what I
> sent.
> Maybe tigris is mangling my mail again?
>
> Let's try again.
>
> Good:
> * subversion/libsvn_subr/eol.c,
> subversion/libsvn_diff/diff_file.c,
>
> Bad:
> * subversion/libsvn_subr/eol.c,
> subversion/libsvn_diff/diff_file.c,
And the "Bad" version is what is shown in HACKING as Good. :)
-Hyrum
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2388846
Re: [PATCH] Renaming svn_eol_* functions to svn_eol__*.
Posted by Stefan Sperling <st...@elego.de>.
On Mon, Aug 31, 2009 at 05:55:55AM -0500, Hyrum K. Wright wrote:
>
> On Aug 31, 2009, at 5:18 AM, Stefan Sperling wrote:
>
> >On Sat, Aug 29, 2009 at 12:39:07PM +0800, Edmund Wong wrote:
> >>* subversion/libsvn_subr/eol.c,
> >> subversion/libsvn_diff/diff_file.c,
> >
> >The asterisk alone is a good enough visual hint for the grouping.
> >So the indentation isn't needed, and these can all go on the same
> >line, like this:
> >
> >* subversion/libsvn_subr/eol.c,
> > subversion/libsvn_diff/diff_file.c,
>
> Not to be pedantic (well, actually, I am being pedantic), but the
> examples in HACKING include the indentation for each file in a
> condensed file list. See
> http://subversion.tigris.org/hacking.html#log-messages
The indentation you are quoting me for there does not match what I sent.
Maybe tigris is mangling my mail again?
Let's try again.
Good:
* subversion/libsvn_subr/eol.c,
subversion/libsvn_diff/diff_file.c,
Bad:
* subversion/libsvn_subr/eol.c,
subversion/libsvn_diff/diff_file.c,
Stefan
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2388843
Re: [PATCH] Renaming svn_eol_* functions to svn_eol__*.
Posted by "Hyrum K. Wright" <hy...@hyrumwright.org>.
On Aug 31, 2009, at 5:18 AM, Stefan Sperling wrote:
> On Sat, Aug 29, 2009 at 12:39:07PM +0800, Edmund Wong wrote:
>> Since the svn_eol_* functions are in the private
>> headers, Arfrever pointed out that they should be
>> renamed to s/svn_eol_/&_/g.
>
> Some small remarks on the log message:
>
>> Log:
>>
>> [[[
>
> You might want to put a short note describing the overall change
> here, unless the change is really trivial. In this case, we could
> say:
>
> "Follow-up to r38983: The new eol functions are supposed to be
> private,
> not public, so rename them accordingly."
>>
>> * subversion/libsvn_subr/eol.c,
>> subversion/libsvn_diff/diff_file.c,
>
> The asterisk alone is a good enough visual hint for the grouping.
> So the indentation isn't needed, and these can all go on the same
> line, like this:
>
> * subversion/libsvn_subr/eol.c,
> subversion/libsvn_diff/diff_file.c,
Not to be pedantic (well, actually, I am being pedantic), but the
examples in HACKING include the indentation for each file in a
condensed file list. See http://subversion.tigris.org/hacking.html#log-messages
-Hyrum
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2388838
Re: [PATCH] Renaming svn_eol_* functions to svn_eol__*.
Posted by Stefan Sperling <st...@elego.de>.
On Sat, Aug 29, 2009 at 12:39:07PM +0800, Edmund Wong wrote:
> Since the svn_eol_* functions are in the private
> headers, Arfrever pointed out that they should be
> renamed to s/svn_eol_/&_/g.
Some small remarks on the log message:
> Log:
>
> [[[
You might want to put a short note describing the overall change
here, unless the change is really trivial. In this case, we could
say:
"Follow-up to r38983: The new eol functions are supposed to be private,
not public, so rename them accordingly."
>
> * subversion/libsvn_subr/eol.c,
> subversion/libsvn_diff/diff_file.c,
The asterisk alone is a good enough visual hint for the grouping.
So the indentation isn't needed, and these can all go on the same
line, like this:
* subversion/libsvn_subr/eol.c,
subversion/libsvn_diff/diff_file.c,
> subversion/tests/libsvn_subr/eol-test.c,
> subversion/libsvn_client/patch.c,
> subversion/include/private/svn_eol_private.h:
The colon above is not necessary. I usually put the colon just between
listing all the affected files/items and describing the change.
This aligns with the examples in HACKING.
> (svn_eol_find_eol_start, svn_eol_detect_eol,
> svn_eol_detect_file_eol): Renamed from ...
I.e. the colon here is fine.
I've also compacted the entries a little so that each item is
only listed once, like this:
* subversion/libsvn_subr/eol.c,
subversion/include/private/svn_eol_private.h
(svn_eol_find_eol_start, svn_eol_detect_eol,
svn_eol_detect_file_eol): Renamed from these ...
(svn_eol__find_eol_start, svn_eol__detect_eol,
svn_eol__detect_file_eol): ... to these.
* subversion/libsvn_diff/diff_file.c,
subversion/tests/libsvn_subr/eol-test.c,
subversion/libsvn_client/patch.c: Track renames.
Committed in r38990.
Thanks,
Stefan
> * subversion/libsvn_subr/eol.c,
> subversion/libsvn_diff/diff_file.c,
> subversion/tests/libsvn_subr/eol-test.c,
> subversion/libsvn_client/patch.c,
> subversion/include/private/svn_eol_private.h:
> (svn_eol__find_eol_start, svn_eol__detect_eol,
> svn_eol__detect_file_eol): ... to.
>
> Patch by: Edmund Wong ed <at> kdtc.net
> Suggested by: arfrever
>
> ]]]
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2388820