[Outreachy][Patch v3] t2200,t9832: avoid using 'git' upstream in a pipe #885

lkmandy · 2020-10-17T09:18:30Z

Changes since v2:

Optimized commit message

Signed-off-by: Amanda Shafack shafack.likhene@gmail.com

cc: Emily Shaffer emilyshaffer@google.com, Jonathan Nieder jrnieder@gmail.com
cc: Eric Sunshine sunshine@sunshineco.com
cc: Amanda Shafack shafack.likhene@gmail.com

gitgitgadget-git · 2020-10-17T09:19:15Z

Welcome to GitGitGadget

Hi @lkmandy, and welcome to GitGitGadget, the GitHub App to send patch series to the Git mailing list from GitHub Pull Requests.

Please make sure that your Pull Request has a good description, as it will be used as cover letter.

Also, it is a good idea to review the commit messages one last time, as the Git project expects them in a quite specific form:

the lines should not exceed 76 columns,
the first line should be like a header and typically start with a prefix like "tests:" or "commit:", and
the commit messages' body should be describing the "why?" of the change.
Finally, the commit messages should end in a Signed-off-by: line matching the commits' author.

It is in general a good idea to await the automated test ("Checks") in this Pull Request before contributing the patches, e.g. to avoid trivial issues such as unportable code.

Contributing the patches

Before you can contribute the patches, your GitHub username needs to be added to the list of permitted users. Any already-permitted user can do that, by adding a comment to your PR of the form /allow. A good way to find other contributors is to locate recent pull requests where someone has been /allowed:

Search: is:pr is:open "/allow"

Both the person who commented /allow and the PR author are able to /allow you.

An alternative is the channel #git-devel on the FreeNode IRC network:

<newcontributor> I've just created my first PR, could someone please /allow me? https://github.com/gitgitgadget/git/pull/12345
<veteran> newcontributor: it is done
<newcontributor> thanks!

Once on the list of permitted usernames, you can contribute the patches to the Git mailing list by adding a PR comment /submit.

If you want to see what email(s) would be sent for a /submit request, add a PR comment /preview to have the email(s) sent to you. You must have a public GitHub email address for this.

After you submit, GitGitGadget will respond with another comment that contains the link to the cover letter mail in the Git mailing list archive. Please make sure to monitor the discussion in that thread and to address comments and suggestions (while the comments and suggestions will be mirrored into the PR by GitGitGadget, you will still want to reply via mail).

If you do not want to subscribe to the Git mailing list just to be able to respond to a mail, you can download the mbox from the Git mailing list archive (click the (raw) link), then import it into your mail program. If you use GMail, you can do this via:

curl -g --user "<EMailAddress>:<Password>" \
    --url "imaps://imap.gmail.com/INBOX" -T /path/to/raw.txt

To iterate on your change, i.e. send a revised patch or patch series, you will first want to (force-)push to the same branch. You probably also want to modify your Pull Request description (or title). It is a good idea to summarize the revision by adding something like this to the cover letter (read: by editing the first comment on the PR, i.e. the PR description):

Changes since v1:
- Fixed a typo in the commit message (found by ...)
- Added a code comment to ... as suggested by ...
...

To send a new iteration, just add another PR comment with the contents: /submit.

Need help?

New contributors who want advice are encouraged to join git-mentoring@googlegroups.com, where volunteers who regularly contribute to Git are willing to answer newbie questions, give advice, or otherwise provide mentoring to interested contributors. You must join in order to post or view messages, but anyone can join.

You may also be able to find help in real time in the developer IRC channel, #git-devel on Freenode. Remember that IRC does not support offline messaging, so if you send someone a private message and log out, they cannot respond to you. The scrollback of #git-devel is archived, though.

dscho · 2020-10-17T12:29:24Z

/allow

gitgitgadget-git · 2020-10-17T12:30:43Z

User lkmandy is now allowed to use GitGitGadget.

t/t2200-add-update.sh

gitgitgadget-git · 2020-10-17T14:52:11Z

There are issues in commit d177780:
The first line must be separated from the rest by an empty line
Commit not signed off

lkmandy · 2020-10-17T15:52:21Z

/preview

gitgitgadget-git · 2020-10-17T15:53:23Z

Preview email sent as pull.885.git.git.1602949999.gitgitgadget@gmail.com

lkmandy · 2020-10-17T16:01:32Z

/submit

gitgitgadget-git · 2020-10-17T16:02:45Z

Submitted as pull.885.git.git.1602950552.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git pr-git-885/lkmandy/avoid-pipes-v1

To fetch this version to local tag pr-git-885/lkmandy/avoid-pipes-v1:

git fetch --no-tags https://github.com/gitgitgadget/git tag pr-git-885/lkmandy/avoid-pipes-v1

t/t2200-add-update.sh

gitgitgadget-git · 2020-10-18T05:57:10Z

User Eric Sunshine <sunshine@sunshineco.com> has been added to the cc: list.

t/t2200-add-update.sh

gitgitgadget-git · 2020-10-18T14:00:19Z

User Amanda Shafack <shafack.likhene@gmail.com> has been added to the cc: list.

lkmandy · 2020-10-18T14:26:23Z

/preview

gitgitgadget-git · 2020-10-18T14:27:36Z

Preview email sent as pull.885.v2.git.git.1603031254168.gitgitgadget@gmail.com

lkmandy · 2020-10-18T14:38:19Z

/preview

gitgitgadget-git · 2020-10-18T14:40:02Z

Preview email sent as pull.885.v2.git.git.1603031999350.gitgitgadget@gmail.com

lkmandy · 2020-10-18T14:41:10Z

/submit

gitgitgadget-git · 2020-10-18T14:42:15Z

Submitted as pull.885.v2.git.git.1603032125151.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git pr-git-885/lkmandy/avoid-pipes-v2

To fetch this version to local tag pr-git-885/lkmandy/avoid-pipes-v2:

git fetch --no-tags https://github.com/gitgitgadget/git tag pr-git-885/lkmandy/avoid-pipes-v2

gitgitgadget-git · 2020-10-18T19:36:43Z

On the Git mailing list, Eric Sunshine wrote (reply to this):

On Sun, Oct 18, 2020 at 10:42 AM Amanda Shafack via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> t9832,t2200: avoid using pipes in git commands

The subject is a bit confusing since pipes aren't used in Git
commands; instead, Git commands may be components of pipes. However,
even that is too imprecise. The issue this patch is addressing is that
we want to avoid Git commands _upstream_ in a pipe. It's perfectly
acceptable for the Git command to be the final element of a pipe since
the pipe returns the exit code of the final command. So, to be more
precise, the subject could say:

    t2200,t9832: avoid using `git` upstream in a pipe

Nit: It's subjective, but it feels a bit more natural to list the test
numbers in ascending order rather than descending order, which is why
I swapped them around in the example above.

> When a git command is upstream in a pipe, an unexpected failure of
> the git command will go unnoticed.
>
> Write out the output of the git command to a file, so as to actively
> catch a failure of the git command.

It's easy to see from the patch itself that the output of the Git
command is now written to a file, so it's not necessary to say so in
the commit message. Therefore, the entire body of the commit message
could be written more succinctly, perhaps like this:

    Avoid placing `git` upstream in a pipe since doing so throws away
    its exit code, thus an unexpected failure may go unnoticed.

The actual patch itself looks fine, and these comments about the
commit message are quite minor, thus there probably is no need to
re-roll (though feel free to do so if you think the bit of extra
polishing of the commit message is worthwhile).

Thanks.

Avoid placing `git` upstream in a pipe since doing so throws away its exit code, thus an unexpected failure may go unnoticed. Signed-off-by: Amanda Shafack <shafack.likhene@gmail.com>

gitgitgadget-git · 2020-10-18T20:06:43Z

On the Git mailing list, Junio C Hamano wrote (reply to this):

Eric Sunshine <sunshine@sunshineco.com> writes:

> precise, the subject could say:
>
>     t2200,t9832: avoid using `git` upstream in a pipe
>
> Nit: It's subjective, but it feels a bit more natural to list the test
> numbers in ascending order rather than descending order, which is why
> I swapped them around in the example above.

;-)

>> When a git command is upstream in a pipe, an unexpected failure of
>> the git command will go unnoticed.
>>
>> Write out the output of the git command to a file, so as to actively
>> catch a failure of the git command.
>
> It's easy to see from the patch itself that the output of the Git
> command is now written to a file, so it's not necessary to say so in
> the commit message. Therefore, the entire body of the commit message
> could be written more succinctly, perhaps like this:
>
>     Avoid placing `git` upstream in a pipe since doing so throws away
>     its exit code, thus an unexpected failure may go unnoticed.

Yup.

> The actual patch itself looks fine, and these comments about the
> commit message are quite minor, thus there probably is no need to
> re-roll (though feel free to do so if you think the bit of extra
> polishing of the commit message is worthwhile).

IIUC, the microproject experience aims new contributors to get used
to the style of communication that happens during review cycles of a
typical topic, using a trivial dip-the-toes-in-the-water problem as
an example.  I'd rather not to see contributors get into the habit
of leaving loose ends and have somebody else clean after them.

Thanks.

gitgitgadget-git · 2020-10-18T20:42:47Z

On the Git mailing list, Amanda Shafack wrote (reply to this):

On Sun, Oct 18, 2020 at 8:25 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> On Sun, Oct 18, 2020 at 10:42 AM Amanda Shafack via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> > t9832,t2200: avoid using pipes in git commands
>
> The subject is a bit confusing since pipes aren't used in Git
> commands; instead, Git commands may be components of pipes. However,
> even that is too imprecise. The issue this patch is addressing is that
> we want to avoid Git commands _upstream_ in a pipe. It's perfectly
> acceptable for the Git command to be the final element of a pipe since
> the pipe returns the exit code of the final command. So, to be more
> precise, the subject could say:
>
>     t2200,t9832: avoid using `git` upstream in a pipe
>
> Nit: It's subjective, but it feels a bit more natural to list the test
> numbers in ascending order rather than descending order, which is why
> I swapped them around in the example above.
>

I agree it looks more appropriate.

> > When a git command is upstream in a pipe, an unexpected failure of
> > the git command will go unnoticed.
> >
> > Write out the output of the git command to a file, so as to actively
> > catch a failure of the git command.
>
> It's easy to see from the patch itself that the output of the Git
> command is now written to a file, so it's not necessary to say so in
> the commit message. Therefore, the entire body of the commit message
> could be written more succinctly, perhaps like this:
>
>     Avoid placing `git` upstream in a pipe since doing so throws away
>     its exit code, thus an unexpected failure may go unnoticed.
>
> The actual patch itself looks fine, and these comments about the
> commit message are quite minor, thus there probably is no need to
> re-roll (though feel free to do so if you think the bit of extra
> polishing of the commit message is worthwhile).

I believe it's best practice to optimize one's work as much as
possible, so I have included these changes.
Thanks for the detailed explanation.

> Thanks.



-- 

Cheers!

Amanda  Shafack

gitgitgadget-git · 2020-10-18T20:42:49Z

User Amanda Shafack <shafack.likhene@gmail.com> has been added to the cc: list.

lkmandy · 2020-10-18T20:47:12Z

/submit

gitgitgadget-git · 2020-10-18T22:07:00Z

On the Git mailing list, Amanda Shafack wrote (reply to this):

On Sun, Oct 18, 2020 at 9:04 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Eric Sunshine <sunshine@sunshineco.com> writes:
>
> > precise, the subject could say:
> >
> >     t2200,t9832: avoid using `git` upstream in a pipe
> >
> > Nit: It's subjective, but it feels a bit more natural to list the test
> > numbers in ascending order rather than descending order, which is why
> > I swapped them around in the example above.
>
> ;-)
>
> >> When a git command is upstream in a pipe, an unexpected failure of
> >> the git command will go unnoticed.
> >>
> >> Write out the output of the git command to a file, so as to actively
> >> catch a failure of the git command.
> >
> > It's easy to see from the patch itself that the output of the Git
> > command is now written to a file, so it's not necessary to say so in
> > the commit message. Therefore, the entire body of the commit message
> > could be written more succinctly, perhaps like this:
> >
> >     Avoid placing `git` upstream in a pipe since doing so throws away
> >     its exit code, thus an unexpected failure may go unnoticed.
>
> Yup.
>
> > The actual patch itself looks fine, and these comments about the
> > commit message are quite minor, thus there probably is no need to
> > re-roll (though feel free to do so if you think the bit of extra
> > polishing of the commit message is worthwhile).
>
> IIUC, the microproject experience aims new contributors to get used
> to the style of communication that happens during review cycles of a
> typical topic, using a trivial dip-the-toes-in-the-water problem as
> an example.  I'd rather not to see contributors get into the habit
> of leaving loose ends and have somebody else clean after them.

Taken very seriously

> Thanks.


-- 

Cheers!

Amanda  Shafack

gitgitgadget-git · 2020-10-19T17:53:12Z

This patch series was integrated into seen via 7c8d89d.

gitgitgadget-git · 2020-10-20T22:54:08Z

This patch series was integrated into seen via 5e2a7fe.

gitgitgadget-git · 2020-10-21T23:01:32Z

This patch series was integrated into seen via 807d942.

gitgitgadget-git · 2020-10-22T06:36:00Z

This patch series was integrated into seen via eb80085.

gitgitgadget-git · 2020-10-22T19:24:42Z

This patch series was integrated into seen via 8872725.

gitgitgadget-git · 2020-10-22T20:54:27Z

This patch series was integrated into seen via 92e704d.

gitgitgadget-git · 2020-10-22T22:06:41Z

This patch series was integrated into seen via 5d09d4c.

gitgitgadget-git · 2020-10-23T14:36:24Z

This patch series was integrated into seen via a6e649d.

gitgitgadget-git · 2020-10-23T20:00:28Z

This patch series was integrated into seen via 54c2a42.

gitgitgadget-git · 2020-10-23T21:40:08Z

This patch series was integrated into seen via 67bd1e9.

gitgitgadget-git · 2020-10-26T22:31:17Z

This patch series was integrated into seen via 9ad66b6.

gitgitgadget-git · 2020-10-27T18:46:16Z

This patch series was integrated into seen via d137500.

gitgitgadget-git · 2020-10-27T23:11:02Z

This patch series was integrated into seen via ea7f49e.

gitgitgadget-git · 2020-10-28T21:16:18Z

This patch series was integrated into seen via dd4dd1e.

gitgitgadget-git · 2020-10-28T21:16:19Z

This patch series was integrated into next via fd199be.

gitgitgadget-git · 2020-10-29T20:59:31Z

This patch series was integrated into seen via 818cfef.

gitgitgadget-git · 2020-10-29T22:26:43Z

This patch series was integrated into seen via 13f719d.

gitgitgadget-git · 2020-10-30T21:21:58Z

This patch series was integrated into seen via fbc0727.

gitgitgadget-git · 2020-11-02T22:17:43Z

This patch series was integrated into seen via b8780d1.

gitgitgadget-git · 2020-11-05T00:02:58Z

This patch series was integrated into seen via 982160c.

gitgitgadget-git · 2020-11-09T23:53:37Z

This patch series was integrated into seen via b3ae46a.

gitgitgadget-git · 2020-11-09T23:53:38Z

This patch series was integrated into next via b3ae46a.

gitgitgadget-git · 2020-11-09T23:53:39Z

This patch series was integrated into master via b3ae46a.

gitgitgadget-git · 2020-11-09T23:53:42Z

Closed via b3ae46a.

dscho reviewed Oct 17, 2020

View reviewed changes

t/t2200-add-update.sh Outdated Show resolved Hide resolved

lkmandy force-pushed the avoid-pipes branch from d177780 to 0a1550c Compare October 17, 2020 15:18

gitgitgadget-git bot reviewed Oct 18, 2020

View reviewed changes

t/t2200-add-update.sh Show resolved Hide resolved

gitgitgadget-git bot reviewed Oct 18, 2020

View reviewed changes

t/t2200-add-update.sh Show resolved Hide resolved

lkmandy force-pushed the avoid-pipes branch from 0a1550c to bca393e Compare October 18, 2020 13:49

lkmandy changed the title ~~[Outreachy][Patch v1] t9832,t2200: avoid using pipes in git related commands~~ [Outreachy][Patch v2] t9832,t2200: avoid using pipes in git related commands Oct 18, 2020

t2200,t9832: avoid using 'git' upstream in a pipe

d5f508b

Avoid placing `git` upstream in a pipe since doing so throws away its exit code, thus an unexpected failure may go unnoticed. Signed-off-by: Amanda Shafack <shafack.likhene@gmail.com>

lkmandy force-pushed the avoid-pipes branch from bca393e to d5f508b Compare October 18, 2020 20:03

lkmandy changed the title ~~[Outreachy][Patch v2] t9832,t2200: avoid using pipes in git related commands~~ [Outreachy][Patch v3] t2200,t9832: avoid using 'git' upstream in a pipe Oct 18, 2020

gitgitgadget-git bot added the seen label Oct 18, 2020

This comment has been minimized.

Sign in to view

gitgitgadget-git bot added the next label Oct 28, 2020

gitgitgadget-git bot added the master label Nov 9, 2020

gitgitgadget-git bot closed this Nov 9, 2020

[Outreachy][Patch v3] t2200,t9832: avoid using 'git' upstream in a pipe #885

[Outreachy][Patch v3] t2200,t9832: avoid using 'git' upstream in a pipe #885

Uh oh!

Conversation

lkmandy commented Oct 17, 2020 • edited by gitgitgadget-git bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gitgitgadget-git bot commented Oct 17, 2020

Welcome to GitGitGadget

Contributing the patches

Need help?

Uh oh!

dscho commented Oct 17, 2020

Uh oh!

gitgitgadget-git bot commented Oct 17, 2020

Uh oh!

Uh oh!

gitgitgadget-git bot commented Oct 17, 2020

Uh oh!

lkmandy commented Oct 17, 2020

Uh oh!

gitgitgadget-git bot commented Oct 17, 2020

Uh oh!

lkmandy commented Oct 17, 2020

Uh oh!

gitgitgadget-git bot commented Oct 17, 2020

Uh oh!

Uh oh!

gitgitgadget-git bot commented Oct 18, 2020

Uh oh!

Uh oh!

gitgitgadget-git bot commented Oct 18, 2020

Uh oh!

lkmandy commented Oct 18, 2020

Uh oh!

gitgitgadget-git bot commented Oct 18, 2020

Uh oh!

lkmandy commented Oct 18, 2020

Uh oh!

gitgitgadget-git bot commented Oct 18, 2020

Uh oh!

lkmandy commented Oct 18, 2020

Uh oh!

gitgitgadget-git bot commented Oct 18, 2020

Uh oh!

gitgitgadget-git bot commented Oct 18, 2020

Uh oh!

gitgitgadget-git bot commented Oct 18, 2020

Uh oh!

gitgitgadget-git bot commented Oct 18, 2020

Uh oh!

gitgitgadget-git bot commented Oct 18, 2020

Uh oh!

lkmandy commented Oct 18, 2020

Uh oh!

gitgitgadget-git bot commented Oct 18, 2020

Uh oh!

gitgitgadget-git bot commented Oct 19, 2020

Uh oh!

This comment has been minimized.

gitgitgadget-git bot commented Oct 20, 2020

Uh oh!

gitgitgadget-git bot commented Oct 21, 2020

Uh oh!

gitgitgadget-git bot commented Oct 22, 2020

Uh oh!

gitgitgadget-git bot commented Oct 22, 2020

Uh oh!

gitgitgadget-git bot commented Oct 22, 2020

Uh oh!

gitgitgadget-git bot commented Oct 22, 2020

Uh oh!

gitgitgadget-git bot commented Oct 23, 2020

Uh oh!

gitgitgadget-git bot commented Oct 23, 2020

Uh oh!

gitgitgadget-git bot commented Oct 23, 2020

Uh oh!

gitgitgadget-git bot commented Oct 26, 2020

Uh oh!

lkmandy commented Oct 17, 2020 •

edited by gitgitgadget-git bot

Loading