[0/3,GSoC] area: t4202-log.sh, modernizing test script

Message

Linus Arver via GitGitGadget April 19, 2022, 1:31 a.m. UTC

I am using this thread to redo my previous attempt to modernize, where I
mistakenly replaced spaces with tabs incorrectly, and to complete the rest
of the modernization of t4202. I am really new to gitgitgadget, and
contributing to open source in general, so I expect I will mess up again,
but hopefully I can get manage to get through it when i do.

Thank you Derrick Stolee for reviewing my last patch, it's clear I still
have a lot of work to do on understanding style and bash scripting. Thanks
for checking this, -Jack McGuinness [e-mail]

Jack McGuinness (3):
  [GSoC][Patch] area: t4202-log.sh, modernizing test script
  [GSoC][Patch] area: t4202-log.sh, modernizing test script p2
  [GSoC][Patch] area: t4202-log.sh, modernizing test script p3

 t/t4202-log.sh | 156 +++++++++++++++++++++++++------------------------
 1 file changed, 80 insertions(+), 76 deletions(-)


base-commit: 6cd33dceed60949e2dbc32e3f0f5e67c4c882e1e
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1220%2FJackMcGu%2Fmaster-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1220/JackMcGu/master-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1220

Comments

Bagas Sanjaya April 19, 2022, 5:26 a.m. UTC | #1

On 4/19/22 08:31, Jack McGuinness via GitGitGadget wrote:
> Jack McGuinness (3):
>   [GSoC][Patch] area: t4202-log.sh, modernizing test script
>   [GSoC][Patch] area: t4202-log.sh, modernizing test script p2
>   [GSoC][Patch] area: t4202-log.sh, modernizing test script p3
> 
>  t/t4202-log.sh | 156 +++++++++++++++++++++++++------------------------
>  1 file changed, 80 insertions(+), 76 deletions(-)
> 

I think the subject prefix of this patch series can be just
[GSOC] [PATCH] instead.

Jack McGuinness April 19, 2022, 5:34 a.m. UTC | #2

Hi, thank you for the advice. I wanted it to be that way myself, however I was using gitgitgadget to email it, and my PR was composed of three different commits, which caused it to automatically be formatted that way. I tried finding a way to remove it, but I had no luck, If you know how I would love to know!

Thanks for your time,
-Jack McGuinness <[e-mail]>

Christian Couder April 25, 2022, 9:45 a.m. UTC | #3

Hi,

On Tue, Apr 19, 2022 at 8:33 AM Jack McGuinness
<[e-mail]> wrote:
>
> Hi, thank you for the advice. I wanted it to be that way myself, however I was using gitgitgadget to email it, and my PR was composed of three different commits, which caused it to automatically be formatted that way. I tried finding a way to remove it, but I had no luck, If you know how I would love to know!

I don't use gitgitgadget, so I cannot help you much on this. The
approach I would take if I had to use it would be to find patches on
the mailing list that were sent using gitgitgadget by experienced
developers using it, then find and see how the corresponding PRs look
like on GitHub, and try to imitate those PRs.

Anyway, it would be nice if you could try again taking into account
Junio's suggestions in:

https://lore.kernel.org/git/xmqqmtggs2nv.fsf@gitster.g/

I also have some suggestions below.

> To: Jack McGuinness via GitGitGadget; [e-mail]
> Cc: Jack McGuinness
> Subject: Re: [PATCH 0/3] [GSoC][Patch] area: t4202-log.sh, modernizing test script

This is your second attempt so it would be nice if it had a "v2"
marker in it like "[PATCH v2 0/3] [GSoC]" instead of "[PATCH 0/3]
[GSoC][Patch]". (Your next attempt should use "v3".)

> On 4/19/22 08:31, Jack McGuinness via GitGitGadget wrote:
> > Jack McGuinness (3):
> >   [GSoC][Patch] area: t4202-log.sh, modernizing test script
> >   [GSoC][Patch] area: t4202-log.sh, modernizing test script p2
> >   [GSoC][Patch] area: t4202-log.sh, modernizing test script p3

Junio already commented on the "area: t4202-log.sh" part of the
subject, so I won't repeat him.

About the "modernizing test script ..." part, it would be nice if the
different patches would be a bit more specific about what each one is
doing and "p2" or "p3" is redundant with the "2/3" or "3/3" added by
GitGitGadget. For example it could be replaced with "remove whitespace
after redirect" for the second patch.