Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[GSoC][Patch] area: t4202-log.sh, modernizing test script #1220

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

JackMcGu
Copy link

@JackMcGu JackMcGu commented Apr 18, 2022

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 jmcguinness2@ucmerced.edu

cc: Bagas Sanjaya bagasdotme@gmail.com
cc: Jack McGuinness jmcguinness2@ucmerced.edu
cc: Christian Couder christian.couder@gmail.com

Replace test body spaces with tabs where appropiate
Remove blank lines at start and and of test bodies.

Signed-off-by: Jack McGuinness <jmcguinness2@ucmerced.edu>
Remove whitespace after redirect operator.

Signed-off-by: Jack McGuinness <jmcguinness2@ucmerced.edu>
Split up multiple lines on one line to multiple
Fix style of cd & echo in subshell

Signed-off-by: Jack McGuinness <jmcguinness2@ucmerced.edu>
@JackMcGu
Copy link
Author

/preview

@gitgitgadget
Copy link

gitgitgadget bot commented Apr 19, 2022

Preview email sent as pull.1220.git.1650328292.gitgitgadget@gmail.com

@JackMcGu
Copy link
Author

/submit

@gitgitgadget
Copy link

gitgitgadget bot commented Apr 19, 2022

Submitted as pull.1220.git.1650331876.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-1220/JackMcGu/master-v1

To fetch this version to local tag pr-1220/JackMcGu/master-v1:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-1220/JackMcGu/master-v1

@derrickstolee
Copy link

Hi @JackMcGu: You can force-push to your topic and re-submit from the same PR. This allows GitGitGadget to realize you have a new version, then show a range-diff between the versions. It keeps the topic threaded nicely on the mailing list, too. (Oh, and it adds CCs for the reviewers you've had so far.)

@gitgitgadget
Copy link

gitgitgadget bot commented Apr 19, 2022

On the Git mailing list, Bagas Sanjaya wrote (reply to this):

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.

-- 
An old man doll... just what I always wanted! - Clara

@gitgitgadget
Copy link

gitgitgadget bot commented Apr 19, 2022

User Bagas Sanjaya <bagasdotme@gmail.com> has been added to the cc: list.

@gitgitgadget
Copy link

gitgitgadget bot commented Apr 19, 2022

On the Git mailing list, Jack McGuinness wrote (reply to this):

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 <jmcguinness2@ucmerced.edu>

________________________________________
From: Bagas Sanjaya <bagasdotme@gmail.com>
Sent: Monday, April 18, 2022 10:26 PM
To: Jack McGuinness via GitGitGadget; git@vger.kernel.org
Cc: Jack McGuinness
Subject: Re: [PATCH 0/3] [GSoC][Patch] area: t4202-log.sh, modernizing test script

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.

--
An old man doll... just what I always wanted! - Clara

@gitgitgadget
Copy link

gitgitgadget bot commented Apr 19, 2022

User Jack McGuinness <jmcguinness2@ucmerced.edu> has been added to the cc: list.

@gitgitgadget
Copy link

gitgitgadget bot commented Apr 25, 2022

On the Git mailing list, Christian Couder wrote (reply to this):

Hi,

On Tue, Apr 19, 2022 at 8:33 AM Jack McGuinness
<jmcguinness2@ucmerced.edu> 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; git@vger.kernel.org
> 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.

@gitgitgadget
Copy link

gitgitgadget bot commented Apr 25, 2022

User Christian Couder <christian.couder@gmail.com> has been added to the cc: list.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants