diff mbox series

[06/21] t4205: add test for trailer in log with nonstandard separator

Message ID 20201025212652.3003036-7-anders@0x63.nu
State New
Headers show
Series trailer fixes | expand

Commit Message

Anders Waldenborg Oct. 25, 2020, 9:26 p.m. UTC
); SAEximRunCond expanded to false

Signed-off-by: Anders Waldenborg <anders@0x63.nu>
---
 t/t4205-log-pretty-formats.sh | 12 ++++++++++++
 1 file changed, 12 insertions(+)

Comments

Christian Couder Oct. 26, 2020, 12:43 p.m. UTC | #1
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?
Anders Waldenborg Nov. 9, 2020, 10:12 p.m. UTC | #2
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.
Christian Couder Nov. 10, 2020, 7:55 a.m. UTC | #3
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.
Jeff King Nov. 10, 2020, 7:54 p.m. UTC | #4
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 mbox series

Patch

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