You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Daniel Näslund <da...@longitudo.com> on 2010/02/05 20:03:46 UTC
Re: [PATCH] v2 Test context within fuzzy hunk for 'svn patch'
On Thu, Feb 04, 2010 at 10:53:26PM +0100, Stefan Sperling wrote:
> On Thu, Feb 04, 2010 at 08:11:00PM +0100, Daniel N�slund wrote:
> Can you explain why?
We have changed a part of the hunk parsing logic to only check for fuzz
at start and end of a hunk. It should not take intermediate context into
account.
> + "@@ -7,7 +8,9 @@\n",
> " and 50,000,000 individual email addresses from all over the world.\n",
> " \n",
> " Your email address drew and have won the sum of 750,000 Euros\n",
> "+Another new line\n",
> " ( Seven Hundred and Fifty Thousand Euros) in cash credited to\n",
> + "+A third new line\n",
> " file with\n",
> " REFERENCE NUMBER: ESP/WIN/008/05/10/MA;\n",
> + " WINNING NUMBER : 14-17-24-34-37-45-16\n",
> Is the indentation of the above line off on purpose?
The extra space was a typo when I created the test. Fixed it at the same
time.
New log message which is ridiciously long for such a small change.
Feedback on it is welcome:
[[[
For determining fuzz we have an algorithm that should only check for
trailing and leading context. Add a small regression test to existing
testcase. At the same time fix small typo.
* subversion/tests/cmdline/patch_tests.py
(patch_with_fuzz): Add context in the middle of one of the fuzzy
hunks.
Patch by: Daniel N�slund <daniel{_AT_}longitudo.com>
]]]
Re: [PATCH] v2 Test context within fuzzy hunk for 'svn patch'
Posted by Julian Foad <ju...@wandisco.com>.
Daniel Näslund wrote:
> Stefan Sperling wrote:
[...]
> > * subversion/tests/cmdline/patch_tests.py
> > (patch_with_fuzz): For determining fuzz we have an algorithm that
> > should only check for trailing and leading context. Make this test
> > verify that this property of the algorithm holds by adding context
> > in the middle of one of the hunks. Also fix an indentation error.
> >
> > What do you think?
>
> Looks good.
It's *beautiful*.
:-)
> This sounds silly but I often feel a bit of rush when I'm
> finished with a patch. It's like if I was thinking to myself: 'If I don't
> get it in immediately, the bug may disappear by itself'. I'm trying
> to slow down and give more thought to things. The most valueable lesson
> so far from contributing to subversion is the importance of beeing
> concious of all the choises I make when programming. In the end, writing
> a log message should be pretty easy if I really understand what I've
> been doing. :-)
I recognize those thoughts.
- Julian
Re: [PATCH] v2 Test context within fuzzy hunk for 'svn patch'
Posted by Daniel Näslund <da...@longitudo.com>.
On Fri, Feb 05, 2010 at 08:41:04PM +0100, Stefan Sperling wrote:
> On Fri, Feb 05, 2010 at 08:03:46PM +0100, Daniel Näslund wrote:
> > New log message which is ridiciously long for such a small change.
>
> No, your new log message is much better! It's not ridiculous.
>
> The basic algorithm for writing good log messages is:
>
> 1) Write the log message.
> 2) Think about *why* you are making the change.
> 3) Look for the answer to this question in the log message
> you've just written. If it is not there, go to step 1.
> 4) You're done.
A good algorithm. Perhaps it should be added to 'HACKING'. Although
there is *a lot* about writing log messages there. I admit to not having
studied it as detailed as I should have. Read it last summer and thought
I had grasped all of it but at a second look, that was not the case.
>
> The rest is partly intuition and partly practice.
>
> You're almost right saying that really good log messages are short.
> But it's not shortness that makes a log message good, but conciseness
> (i.e. high information density).
>
> > Feedback on it is welcome:
> >
> > [[[
> > For determining fuzz we have an algorithm that should only check for
> > trailing and leading context. Add a small regression test to existing
> > testcase. At the same time fix small typo.
>
> So this is better than what was there before (there was nothing before :)
>
> If you make the second sentence refer to things mentioned in the first
> one, it's even clearer what the regression test is for.
> You can also easily say what kind of typo you've fixed:
>
> For determining fuzz we have an algorithm that should only check for
> trailing and leading context. Make an existing existing test case verify
> that this property of the algorithm holds. At the same time fix an
> indentation error.
>
> And you can even merge the above text into this part of your
> log message:
>
> >
> > * subversion/tests/cmdline/patch_tests.py
> > (patch_with_fuzz): Add context in the middle of one of the fuzzy
> > hunks.
>
> because that function is the only scope of all your changes.
> Which also shortens the log message a bit because now you can use the
> pronoun 'this' to refer to the existing test case.
> I'm also doing s/At the same time/Also/ because it provides the same
> information but is shorter:
>
> * subversion/tests/cmdline/patch_tests.py
> (patch_with_fuzz): For determining fuzz we have an algorithm that
> should only check for trailing and leading context. Make this test
> verify that this property of the algorithm holds by adding context
> in the middle of one of the hunks. Also fix an indentation error.
>
> What do you think?
Looks good. This sounds silly but I often feel a bit of rush when I'm
finished with a patch. It's like if I was thinking to myself: 'If I don't
get it in immediately, the bug may disappear by itself'. I'm trying
to slow down and give more thought to things. The most valueable lesson
so far from contributing to subversion is the importance of beeing
concious of all the choises I make when programming. In the end, writing
a log message should be pretty easy if I really understand what I've
been doing. :-)
Daniel
Re: [PATCH] v2 Test context within fuzzy hunk for 'svn patch'
Posted by Daniel Näslund <da...@longitudo.com>.
On Fri, Feb 05, 2010 at 08:41:04PM +0100, Stefan Sperling wrote:
> On Fri, Feb 05, 2010 at 08:03:46PM +0100, Daniel Näslund wrote:
> > New log message which is ridiciously long for such a small change.
>
> No, your new log message is much better! It's not ridiculous.
>
> The basic algorithm for writing good log messages is:
>
> 1) Write the log message.
> 2) Think about *why* you are making the change.
> 3) Look for the answer to this question in the log message
> you've just written. If it is not there, go to step 1.
> 4) You're done.
A good algorithm. Perhaps it should be added to 'HACKING'. Although
there is *a lot* about writing log messages there. I admit to not having
studied it as detailed as I should have. Read it last summer and thought
I had grasped all of it but at a second look, that was not the case.
>
> The rest is partly intuition and partly practice.
>
> You're almost right saying that really good log messages are short.
> But it's not shortness that makes a log message good, but conciseness
> (i.e. high information density).
>
> > Feedback on it is welcome:
> >
> > [[[
> > For determining fuzz we have an algorithm that should only check for
> > trailing and leading context. Add a small regression test to existing
> > testcase. At the same time fix small typo.
>
> So this is better than what was there before (there was nothing before :)
>
> If you make the second sentence refer to things mentioned in the first
> one, it's even clearer what the regression test is for.
> You can also easily say what kind of typo you've fixed:
>
> For determining fuzz we have an algorithm that should only check for
> trailing and leading context. Make an existing existing test case verify
> that this property of the algorithm holds. At the same time fix an
> indentation error.
>
> And you can even merge the above text into this part of your
> log message:
>
> >
> > * subversion/tests/cmdline/patch_tests.py
> > (patch_with_fuzz): Add context in the middle of one of the fuzzy
> > hunks.
>
> because that function is the only scope of all your changes.
> Which also shortens the log message a bit because now you can use the
> pronoun 'this' to refer to the existing test case.
> I'm also doing s/At the same time/Also/ because it provides the same
> information but is shorter:
>
> * subversion/tests/cmdline/patch_tests.py
> (patch_with_fuzz): For determining fuzz we have an algorithm that
> should only check for trailing and leading context. Make this test
> verify that this property of the algorithm holds by adding context
> in the middle of one of the hunks. Also fix an indentation error.
>
> What do you think?
Looks good. This sounds silly but I often feel a bit of rush when I'm
finished with a patch. It's like if I was thinking to myself: 'If I don't
get it in immediately, the bug may disappear by itself'. I'm trying
to slow down and give more thought to things. The most valueable lesson
so far from contributing to subversion is the importance of beeing
concious of all the choises I make when programming. In the end, writing
a log message should be pretty easy if I really understand what I've
been doing. :-)
Daniel
Re: [PATCH] v2 Test context within fuzzy hunk for 'svn patch'
Posted by Stefan Sperling <st...@elego.de>.
On Fri, Feb 05, 2010 at 08:03:46PM +0100, Daniel Näslund wrote:
> New log message which is ridiciously long for such a small change.
No, your new log message is much better! It's not ridiculous.
The basic algorithm for writing good log messages is:
1) Write the log message.
2) Think about *why* you are making the change.
3) Look for the answer to this question in the log message
you've just written. If it is not there, go to step 1.
4) You're done.
The rest is partly intuition and partly practice.
You're almost right saying that really good log messages are short.
But it's not shortness that makes a log message good, but conciseness
(i.e. high information density).
> Feedback on it is welcome:
>
> [[[
> For determining fuzz we have an algorithm that should only check for
> trailing and leading context. Add a small regression test to existing
> testcase. At the same time fix small typo.
So this is better than what was there before (there was nothing before :)
If you make the second sentence refer to things mentioned in the first
one, it's even clearer what the regression test is for.
You can also easily say what kind of typo you've fixed:
For determining fuzz we have an algorithm that should only check for
trailing and leading context. Make an existing existing test case verify
that this property of the algorithm holds. At the same time fix an
indentation error.
And you can even merge the above text into this part of your
log message:
>
> * subversion/tests/cmdline/patch_tests.py
> (patch_with_fuzz): Add context in the middle of one of the fuzzy
> hunks.
because that function is the only scope of all your changes.
Which also shortens the log message a bit because now you can use the
pronoun 'this' to refer to the existing test case.
I'm also doing s/At the same time/Also/ because it provides the same
information but is shorter:
* subversion/tests/cmdline/patch_tests.py
(patch_with_fuzz): For determining fuzz we have an algorithm that
should only check for trailing and leading context. Make this test
verify that this property of the algorithm holds by adding context
in the middle of one of the hunks. Also fix an indentation error.
What do you think?
Stefan
Re: [PATCH] v2 Test context within fuzzy hunk for 'svn patch'
Posted by Stefan Sperling <st...@elego.de>.
On Fri, Feb 05, 2010 at 08:03:46PM +0100, Daniel Näslund wrote:
> New log message which is ridiciously long for such a small change.
No, your new log message is much better! It's not ridiculous.
The basic algorithm for writing good log messages is:
1) Write the log message.
2) Think about *why* you are making the change.
3) Look for the answer to this question in the log message
you've just written. If it is not there, go to step 1.
4) You're done.
The rest is partly intuition and partly practice.
You're almost right saying that really good log messages are short.
But it's not shortness that makes a log message good, but conciseness
(i.e. high information density).
> Feedback on it is welcome:
>
> [[[
> For determining fuzz we have an algorithm that should only check for
> trailing and leading context. Add a small regression test to existing
> testcase. At the same time fix small typo.
So this is better than what was there before (there was nothing before :)
If you make the second sentence refer to things mentioned in the first
one, it's even clearer what the regression test is for.
You can also easily say what kind of typo you've fixed:
For determining fuzz we have an algorithm that should only check for
trailing and leading context. Make an existing existing test case verify
that this property of the algorithm holds. At the same time fix an
indentation error.
And you can even merge the above text into this part of your
log message:
>
> * subversion/tests/cmdline/patch_tests.py
> (patch_with_fuzz): Add context in the middle of one of the fuzzy
> hunks.
because that function is the only scope of all your changes.
Which also shortens the log message a bit because now you can use the
pronoun 'this' to refer to the existing test case.
I'm also doing s/At the same time/Also/ because it provides the same
information but is shorter:
* subversion/tests/cmdline/patch_tests.py
(patch_with_fuzz): For determining fuzz we have an algorithm that
should only check for trailing and leading context. Make this test
verify that this property of the algorithm holds by adding context
in the middle of one of the hunks. Also fix an indentation error.
What do you think?
Stefan