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

From: Christian Couder <christian.couder@gmail.com>
Date: Nov 10, 2020, 07:55 UTC
Message-ID: <CAP8UFD0SkCHpSeZ0aWO21mj7+DJJ0GRJhkSCeKzbUsgQkwVLRw@mail.gmail.com>
In-Reply-To: <87y2jap4ch.fsf@0x63.nu>

On Mon, Nov 9, 2020 at 11:12 PM Anders Waldenborg <anders@0x63.nu> wrote:

Show 13 quoted lines

>
>
> 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.

Show 11 quoted lines

> >> 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.