| Message ID | 20201025212652.3003036-7-anders@0x63.nu (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | trailer fixes | expand |
On Sun, Oct 25, 2020 at 10:27 PM Anders Waldenborg <anders@0x63.nu> wrote: > > ); SAEximRunCond expanded to false > > Signed-off-by: Anders Waldenborg <anders@0x63.nu> Why is this new test important?
Christian Couder writes: > On Sun, Oct 25, 2020 at 10:27 PM Anders Waldenborg <anders@0x63.nu> wrote: >> >> ); SAEximRunCond expanded to false Please disregard this line. It is an unfortunate and most embarrassing artifact of messed up git send-email and stmp forwarding over ssh. Which hopefully have been sorted so it doesn't happen next time. It obviously shouldn't be part of the commit massage in any of the patches in the series. >> Signed-off-by: Anders Waldenborg <anders@0x63.nu> > > Why is this new test important? The test that checks that 'git log --pretty=format:%(trailers)' shows the output in the form "Closes: 1234" even if input was "Closes #1234" is interesting both because it checks that this behavior is kept intact in the patches later in the series which modifies handling of separator and because it is a behavior that can be surprising and not well defined in documentation and those tend to be the ones that are easiest to accidentally break. Maybe the addition of the test should come later in the series where the changes that potentially could break it happen. It seems like you stopped reviewing my patch series at patch 06/21. That is IMHO just before it starts to get interesting :) Now I don't know if rest of it was rubbish or uninteresting or just there was no time to look at it. I've updated according to the suggestions, but not sure if I should repost the series with just such small adjustments.
On Mon, Nov 9, 2020 at 11:12 PM Anders Waldenborg <anders@0x63.nu> wrote: > > > Christian Couder writes: > > > On Sun, Oct 25, 2020 at 10:27 PM Anders Waldenborg <anders@0x63.nu> wrote: > >> > >> ); SAEximRunCond expanded to false > > Please disregard this line. It is an unfortunate and most embarrassing > artifact of messed up git send-email and stmp forwarding over ssh. Which > hopefully have been sorted so it doesn't happen next time. It obviously > shouldn't be part of the commit massage in any of the patches in the > series. Ok. > >> Signed-off-by: Anders Waldenborg <anders@0x63.nu> > > > > Why is this new test important? > > The test that checks that 'git log --pretty=format:%(trailers)' shows > the output in the form "Closes: 1234" even if input was "Closes #1234" > is interesting both because it checks that this behavior is kept intact > in the patches later in the series which modifies handling of separator > and because it is a behavior that can be surprising and not well defined > in documentation and those tend to be the ones that are easiest to > accidentally break. Ok, I would suggest adding some of the above in the commit message of the next version of the patch. > Maybe the addition of the test should come later in > the series where the changes that potentially could break it happen. Maybe. I found the series a bit confusing because it seemed to me that the cover letter wasn't explaining very well what it does. I just commented on the cover letter. Hopefully in the next version it will be better, and it will then be easier to see if patches should be moved around. > It seems like you stopped reviewing my patch series at patch 06/21. That > is IMHO just before it starts to get interesting :) Now I don't know if > rest of it was rubbish or uninteresting or just there was no time to > look at it. It was a combination of not much time and the cover letter not making it easy to understand the whole series. I was hoping that the next version would have more explanations in the cover letter and also in some commit messages. > I've updated according to the suggestions, but not sure if I should > repost the series with just such small adjustments. I think it's worth reposting with an improved cover letter and other small adjustments. Thanks, Christian.
On Mon, Nov 09, 2020 at 11:12:14PM +0100, Anders Waldenborg wrote: > > Why is this new test important? > > The test that checks that 'git log --pretty=format:%(trailers)' shows > the output in the form "Closes: 1234" even if input was "Closes #1234" > is interesting both because it checks that this behavior is kept intact > in the patches later in the series which modifies handling of separator > and because it is a behavior that can be surprising and not well defined > in documentation and those tend to be the ones that are easiest to > accidentally break. Maybe the addition of the test should come later in > the series where the changes that potentially could break it happen. That makes sense, but should be in the commit message. I also found the expected output confusing. I thought at first we were mis-parsing to include part of the subject in the trailer, but it is just that we put "%s" into the format argument. > It seems like you stopped reviewing my patch series at patch 06/21. That > is IMHO just before it starts to get interesting :) Now I don't know if > rest of it was rubbish or uninteresting or just there was no time to > look at it. > > I've updated according to the suggestions, but not sure if I should > repost the series with just such small adjustments. Reviewing this has been on my todo list, but I'd just as soon do it from your latest version. Since it has been a while, it may make sense to just repost with the fixes, and note in the cover letter that it didn't get a lot of review yet. -Peff
diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh index 757575d3f6..42544fb07a 100755 --- a/t/t4205-log-pretty-formats.sh +++ b/t/t4205-log-pretty-formats.sh @@ -757,6 +757,18 @@ test_expect_success 'pretty format %(trailers) combining separator/key/valueonly test_cmp expect actual ' +test_expect_success 'pretty format %(trailers) with nonstandard separator' ' + git commit --allow-empty -F - <<-\EOF && + Some fix + + Closes #1234 + EOF + + git -c "trailer.separators=:#" log --no-walk --pretty="format:%s% (trailers:key=Closes)" >actual && + echo "Some fix Closes: 1234" >expect && + test_cmp expect actual +' + test_expect_success 'trailer parsing not fooled by --- line' ' git commit --allow-empty -F - <<-\EOF && this is the subject
); SAEximRunCond expanded to false Signed-off-by: Anders Waldenborg <anders@0x63.nu> --- t/t4205-log-pretty-formats.sh | 12 ++++++++++++ 1 file changed, 12 insertions(+)