[PATCH 0/2] microproject: avoid using pipes in test

[PATCH 0/2] microproject: avoid using pipes in test

[Shubham Mishra]

pipes doesn't care about error codes and ignore them thus we should not use them in tests.
As an easy alternative, I am using a tmp file to write from git command so we can test the exit code.

This is my first contribution that's why I am keeping diff short with an intention to understand the process instead of making impactful change in first attempt. Later, I will send more patches to fix the issue for other files.
Shubham Mishra (2):
  t0001: remove pipes
  t0003: remove pipes

 t/t0001-init.sh       | 4 ++--
 t/t0003-attributes.sh | 6 +++---
 2 files changed, 5 insertions(+), 5 deletions(-)


base-commit: e6ebfd0e8cbbd10878070c8a356b5ad1b3ca464e
-- 
2.25.1

[PATCH 1/2] t0001: remove pipes

[Shubham Mishra]

pipes doesn't care about error codes and ignore them thus we should not use them in tests.
As an easy alternative, I am using a tmp file to write from git command so we can test the exit code.

Signed-off-by: Shubham Mishra <shivam828787@gmail.com>
---
 t/t0001-init.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t0001-init.sh b/t/t0001-init.sh
index 3235ab4d53..9a8f209648 100755
--- a/t/t0001-init.sh
+++ b/t/t0001-init.sh
@@ -489,11 +489,11 @@ test_expect_success 're-init from a linked worktree' '
 		git worktree add ../linked-worktree &&
 		mv .git/info/exclude expected-exclude &&
 		cp .git/config expected-config &&
-		find .git/worktrees -print | sort >expected &&
+		find .git/worktrees -print >tmp && sort tmp >expected &&
 		git -C ../linked-worktree init &&
 		test_cmp expected-exclude .git/info/exclude &&
 		test_cmp expected-config .git/config &&
-		find .git/worktrees -print | sort >actual &&
+		find .git/worktrees -print >tmp && sort tmp >actual &&
 		test_cmp expected actual
 	)
 '
-- 
2.25.1

[PATCH 2/2] t0003: remove pipes

[Shubham Mishra]

pipes doesn't care about error codes and ignore them thus we should not use them in tests.
As an easy alternative, I am using a tmp file to write from git command so we can test the exit code.

Signed-off-by: Shubham Mishra <shivam828787@gmail.com>
---
 t/t0003-attributes.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh
index b9ed612af1..51f9f6503c 100755
--- a/t/t0003-attributes.sh
+++ b/t/t0003-attributes.sh
@@ -205,15 +205,15 @@ test_expect_success 'attribute test: read paths from stdin' '
 test_expect_success 'attribute test: --all option' '
 	grep -v unspecified <expect-all | sort >specified-all &&
 	sed -e "s/:.*//" <expect-all | uniq >stdin-all &&
-	git check-attr --stdin --all <stdin-all | sort >actual &&
+	git check-attr --stdin --all <stdin-all >tmp && sort tmp >actual &&
 	test_cmp specified-all actual
 '
 
 test_expect_success 'attribute test: --cached option' '
-	git check-attr --cached --stdin --all <stdin-all | sort >actual &&
+	git check-attr --cached --stdin --all <stdin-all >tmp && sort tmp >actual &&
 	test_must_be_empty actual &&
 	git add .gitattributes a/.gitattributes a/b/.gitattributes &&
-	git check-attr --cached --stdin --all <stdin-all | sort >actual &&
+	git check-attr --cached --stdin --all <stdin-all >tmp && sort tmp >actual &&
 	test_cmp specified-all actual
 '
 
-- 
2.25.1

Re: [PATCH 0/2] microproject: avoid using pipes in test

[Derrick Stolee]

On 2/22/2022 6:43 AM, Shubham Mishra wrote:

Welcome, Subham!

> pipes doesn't care about error codes and ignore them thus we should not use them in tests.
> As an easy alternative, I am using a tmp file to write from git command so we can test the exit code.

This is the correct way to convert the pipes into something that
properly notices Git failures. The only issue I have with your
patches is that you should insert a newline after your &&.

(I'll include an example on patch 1.)
 
> This is my first contribution that's why I am keeping diff short with an intention to understand the process instead of making impactful change in first attempt. Later, I will send more patches to fix the issue for other files.

I think this is a great start! After fixing the formatting that
I saw, you should be good to start making more changes in a
single series. These patches are a good size.

Thanks,
-Stolee

Re: [PATCH 1/2] t0001: remove pipes

[Derrick Stolee]

On 2/22/2022 6:43 AM, Shubham Mishra wrote:
> pipes doesn't care about error codes and ignore them thus we should not use them in tests.
> As an easy alternative, I am using a tmp file to write from git command so we can test the exit code.

Please be careful about the length of your lines in your
commit message. vim should auto-wrap as you write.

There are also some grammar issues, so here is an update
to what you wrote:

  Pipes ignore error codes and thus we should not use them
  in tests. As an alternative, use a 'tmp' file to write the
  Git output so we can test the exit code.

(My wrapping is probably too short here.)

> Signed-off-by: Shubham Mishra <shivam828787@gmail.com>
> ---
>  t/t0001-init.sh | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/t/t0001-init.sh b/t/t0001-init.sh
> index 3235ab4d53..9a8f209648 100755
> --- a/t/t0001-init.sh
> +++ b/t/t0001-init.sh
> @@ -489,11 +489,11 @@ test_expect_success 're-init from a linked worktree' '
>  		git worktree add ../linked-worktree &&
>  		mv .git/info/exclude expected-exclude &&
>  		cp .git/config expected-config &&
> -		find .git/worktrees -print | sort >expected &&
> +		find .git/worktrees -print >tmp && sort tmp >expected &&

Split each part of the &&-chain across lines, like this:

+		find .git/worktrees -print >tmp &&
+		sort tmp >expected &&

>  		git -C ../linked-worktree init &&
>  		test_cmp expected-exclude .git/info/exclude &&
>  		test_cmp expected-config .git/config &&
> -		find .git/worktrees -print | sort >actual &&
> +		find .git/worktrees -print >tmp && sort tmp >actual &&

Here, too.

I could make the same comments on patch 2, but I think you
have enough info to go on.

Thanks,
-Stolee

Re: [PATCH 0/2] microproject: avoid using pipes in test

[Ævar Arnfjörð Bjarmason]

On Tue, Feb 22 2022, Shubham Mishra wrote:

> pipes doesn't care about error codes and ignore them thus we should not use them in tests.

Aside from what Derrick Stolee mentioned in his feedback, all of which I
agree with.

I think these changes are good, but it's not the case that we try to
avoid using pipes at all in our tests.

It's often a hassle, and just not worth it, e.g.:

    oid=$(echo foo | git hash-object --stdin -w) &&

Sure, we can make that:

    echo foo >in &&
    oid=$(git hash-object --stdin -w <in) &&

But in the general case it's not worth worrying about.

What we *do* try to avoid, and what's actually important is to never
invoke "git" or other programs we invoke on the LHS of a pipe, or to
otherwise do so in a way that hides potential errors.

That's not isolated to just pipes, but e.g. calling it within helper
functions that don't &&-chain, but pipes are probably the most common
offender.

The reason we do that is because in hacking git we may make it error,
segfault etc. If it's on the LHS of a pipe that failure becomes
indistinguishable from success.

And if the test is really checking e.g. "when I run git like this, it
produces no output" printing nothing with an exit of 0 will become the
same as a segafault for the purposes of test.

And that's bad.

But just invoking things on the LHS of a pipe? Sometimes it's a good
thing not do do that, because we'll be able to see a failure more
incrementally, and with intermediate files.

But it's generally not a problem, our test suite assumes that the OS is
basically sane. We're not going to call every invocation of "sed",
"grep" etc. with a wrapper like "test_must_fail grep" instead of "!
grep".

The same goes for our own helper utility functions such as "q_to_nul"
etc, as long as (and this is critical) that they're implemented by
shelling out to "sed", "grep", "perl" or whatever, and not to "git" or
"test-tool" etc. Then we need to start being as paranoid about them as
"git" on the LHS of pipes.

Re: [PATCH 1/2] t0001: remove pipes

[Shubham Mishra]

> > pipes doesn't care about error codes and ignore them thus we should not use them in tests.
> > As an easy alternative, I am using a tmp file to write from git command so we can test the exit code.
>
> Please be careful about the length of your lines in your
> commit message. vim should auto-wrap as you write.

Oops, I mostly use Nano, I think it's time to use vim. I will take care of it.

> There are also some grammar issues, so here is an update
> to what you wrote:
>
>   Pipes ignore error codes and thus we should not use them
>   in tests. As an alternative, use a 'tmp' file to write the
>   Git output so we can test the exit code.

Thanks for pointing this out. I will update it. I am a non-native
English speaker, and looking to improve my command in language with
time :)

> Split each part of the &&-chain across lines, like this:
>
> +               find .git/worktrees -print >tmp &&
> +               sort tmp >expected &&
>
> >               git -C ../linked-worktree init &&
> >               test_cmp expected-exclude .git/info/exclude &&
> >               test_cmp expected-config .git/config &&
> > -             find .git/worktrees -print | sort >actual &&
> > +             find .git/worktrees -print >tmp && sort tmp >actual &&
>
> Here, too.
>
> I could make the same comments on patch 2, but I think you
> have enough info to go on.

Sure, I get it! I will consider all these suggestions in v2.

Thanks,
Shubham

Re: [PATCH 0/2] microproject: avoid using pipes in test

[Shubham Mishra]

> I think these changes are good, but it's not the case that we try to
> avoid using pipes at all in our tests.
>
> It's often a hassle, and just not worth it, e.g.:
>
>     oid=$(echo foo | git hash-object --stdin -w) &&
>
> Sure, we can make that:
>
>     echo foo >in &&
>     oid=$(git hash-object --stdin -w <in) &&
>
> But in the general case it's not worth worrying about.
>
> What we *do* try to avoid, and what's actually important is to never
> invoke "git" or other programs we invoke on the LHS of a pipe, or to
> otherwise do so in a way that hides potential errors.

Sorry, I have not mentioned it properly in the message but my
intention is not to remove all pipes but to remove only those, which
have "git" command in LHS.

> That's not isolated to just pipes, but e.g. calling it within helper
> functions that don't &&-chain, but pipes are probably the most common
> offender.
>
> The reason we do that is because in hacking git we may make it error,
> segfault etc. If it's on the LHS of a pipe that failure becomes
> indistinguishable from success.
>
> And if the test is really checking e.g. "when I run git like this, it
> produces no output" printing nothing with an exit of 0 will become the
> same as a segafault for the purposes of test.
>
> And that's bad.
>
> But just invoking things on the LHS of a pipe? Sometimes it's a good
> thing not do do that, because we'll be able to see a failure more
> incrementally, and with intermediate files.
>
> But it's generally not a problem, our test suite assumes that the OS is
> basically sane. We're not going to call every invocation of "sed",
> "grep" etc. with a wrapper like "test_must_fail grep" instead of "!
> grep".
>
> The same goes for our own helper utility functions such as "q_to_nul"
> etc, as long as (and this is critical) that they're implemented by
> shelling out to "sed", "grep", "perl" or whatever, and not to "git" or
> "test-tool" etc. Then we need to start being as paranoid about them as
> "git" on the LHS of pipes.

Thanks here for providing me with a broader context of the problem.
What I understand,
It's not just about "git" on LHS of pipes but more broader to anything
custom where
We can miss exit codes but I think as a low hanging fruit I can start
with "git" on LHS of pipe
and as I will understand the codebase more I can work on other custom
helpers too.

Thanks,
Shubham

Re: [PATCH 0/2] microproject: avoid using pipes in test

[Taylor Blau]

On Tue, Feb 22, 2022 at 03:24:31PM +0100, Ævar Arnfjörð Bjarmason wrote:
>
> On Tue, Feb 22 2022, Shubham Mishra wrote:
>
> > pipes doesn't care about error codes and ignore them thus we should not use them in tests.
>
> Aside from what Derrick Stolee mentioned in his feedback, all of which I
> agree with.
>
> I think these changes are good, but it's not the case that we try to
> avoid using pipes at all in our tests.
>
> It's often a hassle, and just not worth it, e.g.:
>
>     oid=$(echo foo | git hash-object --stdin -w) &&
>
> Sure, we can make that:
>
>     echo foo >in &&
>     oid=$(git hash-object --stdin -w <in) &&
>
> But in the general case it's not worth worrying about.

Agreed, and I would add that we don't necessarily need to worry about
non-Git commands on the left-hand side of a pipe. So something like:

    find ... | sort >actual

isn't a problem for us, because our test suite assumes that something
like find will not fail. So leaving instances of those alone is OK,
but...

> What we *do* try to avoid, and what's actually important is to never
> invoke "git" or other programs we invoke on the LHS of a pipe, or to
> otherwise do so in a way that hides potential errors.
>
> That's not isolated to just pipes, but e.g. calling it within helper
> functions that don't &&-chain, but pipes are probably the most common
> offender.
>
> The reason we do that is because in hacking git we may make it error,
> segfault etc. If it's on the LHS of a pipe that failure becomes
> indistinguishable from success.
>
> And if the test is really checking e.g. "when I run git like this, it
> produces no output" printing nothing with an exit of 0 will become the
> same as a segafault for the purposes of test.

...yes, we do care about Git failures. So something like:

    git ls-files | grep "want"

would be no-good, since any failures running 'git ls-files' would be
quashed by the pipe.

Thanks,
Taylor

Re: [PATCH 1/2] t0001: remove pipes

[Christian Couder]

On Tue, Feb 22, 2022 at 3:08 PM Shubham Mishra <shivam828787@gmail.com> wrote:
>
> pipes doesn't care about error codes and ignore them thus we should not use them in tests.

Only the exit code of the command before the pipe is ignored.

Also it's ok to use pipes if the command before the pipe is not `git`.
We trust regular commands to just work and we test only `git`.

> As an easy alternative, I am using a tmp file to write from git command so we can test the exit code.

In general, when improving the code in a way that has already been
used by others, it's a good idea to take a look at previous commits
doing the same thing. See for example 66c0c44df6 (t0000: avoid masking
git exit value through pipes, 2021-09-16). I am not saying that you
should copy paste the commit message though.

> Signed-off-by: Shubham Mishra <shivam828787@gmail.com>
> ---
>  t/t0001-init.sh | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/t/t0001-init.sh b/t/t0001-init.sh
> index 3235ab4d53..9a8f209648 100755
> --- a/t/t0001-init.sh
> +++ b/t/t0001-init.sh
> @@ -489,11 +489,11 @@ test_expect_success 're-init from a linked worktree' '
>                 git worktree add ../linked-worktree &&
>                 mv .git/info/exclude expected-exclude &&
>                 cp .git/config expected-config &&
> -               find .git/worktrees -print | sort >expected &&
> +               find .git/worktrees -print >tmp && sort tmp >expected &&

Please put the `find` and `sort` commands on 2 different lines when
they are separated with &&.

>                 git -C ../linked-worktree init &&
>                 test_cmp expected-exclude .git/info/exclude &&
>                 test_cmp expected-config .git/config &&
> -               find .git/worktrees -print | sort >actual &&
> +               find .git/worktrees -print >tmp && sort tmp >actual &&

Idem.

>                 test_cmp expected actual
>         )
>  '
> --
> 2.25.1
>

[PATCH v2 0/2] avoid pipes with Git on LHS

[Shubham Mishra]

fixed commit messages and endline after '&&' formatting from previous
version.

Shubham Mishra (2):
  t0001: avoid pipes with Git on LHS
  t0003: avoid pipes with Git on LHS

 t/t0001-init.sh       | 6 ++++--
 t/t0003-attributes.sh | 9 ++++++---
 2 files changed, 10 insertions(+), 5 deletions(-)

Range-diff against v1:
1:  620a5b991f = 1:  620a5b991f t0001: avoid pipes with Git on LHS
2:  c17d49d802 = 2:  c17d49d802 t0003: avoid pipes with Git on LHS
-- 
2.25.1

[PATCH v2 1/2] t0001: avoid pipes with Git on LHS

[Shubham Mishra]

Pipes ignore error codes of LHS command and thu`s we should not use
them with Git in tests. As an alternative, use a 'tmp' file to write
the Git output so we can test the exit code.

Signed-off-by: Shubham Mishra <shivam828787@gmail.com>
---
 t/t0001-init.sh | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/t/t0001-init.sh b/t/t0001-init.sh
index 3235ab4d53..c7dd91cb80 100755
--- a/t/t0001-init.sh
+++ b/t/t0001-init.sh
@@ -489,11 +489,13 @@ test_expect_success 're-init from a linked worktree' '
 		git worktree add ../linked-worktree &&
 		mv .git/info/exclude expected-exclude &&
 		cp .git/config expected-config &&
-		find .git/worktrees -print | sort >expected &&
+		find .git/worktrees -print >tmp &&
+		sort tmp >expected &&
 		git -C ../linked-worktree init &&
 		test_cmp expected-exclude .git/info/exclude &&
 		test_cmp expected-config .git/config &&
-		find .git/worktrees -print | sort >actual &&
+		find .git/worktrees -print >tmp &&
+		sort tmp >actual &&
 		test_cmp expected actual
 	)
 '
-- 
2.25.1

[PATCH v2 2/2] t0003: avoid pipes with Git on LHS

[Shubham Mishra]

Pipes ignore error codes of LHS command and thus we should not use them
with Git in tests. As an alternative, use a 'tmp' file to write the Git
output so we can test the exit code.

Signed-off-by: Shubham Mishra <shivam828787@gmail.com>
---
 t/t0003-attributes.sh | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh
index b9ed612af1..143f100517 100755
--- a/t/t0003-attributes.sh
+++ b/t/t0003-attributes.sh
@@ -205,15 +205,18 @@ test_expect_success 'attribute test: read paths from stdin' '
 test_expect_success 'attribute test: --all option' '
 	grep -v unspecified <expect-all | sort >specified-all &&
 	sed -e "s/:.*//" <expect-all | uniq >stdin-all &&
-	git check-attr --stdin --all <stdin-all | sort >actual &&
+	git check-attr --stdin --all <stdin-all >tmp &&
+	sort tmp >actual &&
 	test_cmp specified-all actual
 '
 
 test_expect_success 'attribute test: --cached option' '
-	git check-attr --cached --stdin --all <stdin-all | sort >actual &&
+	git check-attr --cached --stdin --all <stdin-all >tmp &&
+	sort tmp >actual &&
 	test_must_be_empty actual &&
 	git add .gitattributes a/.gitattributes a/b/.gitattributes &&
-	git check-attr --cached --stdin --all <stdin-all | sort >actual &&
+	git check-attr --cached --stdin --all <stdin-all >tmp &&
+	sort tmp >actual &&
 	test_cmp specified-all actual
 '
 
-- 
2.25.1

Re: [PATCH v2 0/2] avoid pipes with Git on LHS

[Shubham Mishra]

Hi,
I am using mails for code review for the first time, I have some
doubts, Can someone please clarify them? -
1. It looks like the cover letter (Including "Range-diff" section) is
only for context sharing with reviewers, nothing from it gets merged
to the "seek" or any other branch.
2. I wanted to know how the merging process takes place. Once the
patch is accepted, do we merge all previous versions of it one after
another or every patch is independent so we have to just merge the
last accepted patch?
3. How does a particular patch set is linked to its previous version?
I added the flag "--in-reply-to" while sending this mail but I can't
find any linking here on the mail. Not sure If I am missing something
here.

Re: [PATCH v2 0/2] avoid pipes with Git on LHS

[Shaoxuan Yuan]

On Wed, Feb 23, 2022 at 8:42 PM Shubham Mishra <shivam828787@gmail.com> wrote:
>
> Hi,

Hi,

> I am using mails for code review for the first time, I have some
> doubts, Can someone please clarify them? -
> 1. It looks like the cover letter (Including "Range-diff" section) is
> only for context sharing with reviewers, nothing from it gets merged
> to the "seek" or any other branch.

The cover letter stands for an introduction/summary to your patches.
You can also put helpful context in it for better understanding. According
to my knowledge, it will not be in the commit messages.

> 2. I wanted to know how the merging process takes place. Once the
> patch is accepted, do we merge all previous versions of it one after
> another or every patch is independent so we have to just merge the
> last accepted patch?

Not so sure about this question. My two cents: generally the most agreed-upon
patch will be merged, but the exact merging process could vary based
on the circumstances. Probably Junio can have a better answer to this.

> 3. How does a particular patch set is linked to its previous version?

"--in-reply-to" will do.

> I added the flag "--in-reply-to" while sending this mail but I can't
> find any linking here on the mail. Not sure If I am missing something
> here.

Just by looking at the current thread, I think you are doing it right.

Adding "--in-reply-to" to the Message-ID of your previous patch can
give you a threaded view of the patch history on the public-inbox. For
instance, by always "--in-reply-to" the first patch version ("v1"), you can
line up your patches as they are threaded on the same level. You also
can choose to always reply to the previous patch's cover letter (just an
example), so that your patch history cascades. I think you always go
the way that best illustrates the history and helps reviewers.

I'm also new to the community, that's the best I can answer.
Inaccuracies may be found above.

-- 
Thanks & Regards,
Shaoxuan

Re: [PATCH v2 1/2] t0001: avoid pipes with Git on LHS

[Taylor Blau]

On Wed, Feb 23, 2022 at 05:23:46PM +0530, Shubham Mishra wrote:
> Pipes ignore error codes of LHS command and thu`s we should not use
> them with Git in tests. As an alternative, use a 'tmp' file to write
> the Git output so we can test the exit code.

This patch does preserve the existing behavior. But I'm hesitant to
recommend that we apply this patch, since our test suite assumes that
commands like find will work, and so we aren't concerned about squashing
any potential error codes when it's on the left-hand side of the pipe,
since we assume that it won't fail in general.

(That's notably different from the second patch in this series, where
the thing on the left-hand side of the pipe is a git invocation. In that
case, we really _do_ want to avoid having it on the left-hand side of
the pipe, because we don't have the same error-free expectation there,
and want to know when it fails).

I think that Ævar gave a nice summary of the above in [1]

Thanks,
Taylor

[1]: https://lore.kernel.org/git/220222.86pmnf6ket.gmgdl@evledraar.gmail.com/

Re: [PATCH v2 2/2] t0003: avoid pipes with Git on LHS

[Taylor Blau]

On Wed, Feb 23, 2022 at 05:23:47PM +0530, Shubham Mishra wrote:
>  t/t0003-attributes.sh | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)

This patch looks great to me, thanks!

    Reviewed-by: Taylor Blau <me@ttaylorr.com>

Thanks,
Taylor

Re: [PATCH v2 0/2] avoid pipes with Git on LHS

[Taylor Blau]

On Wed, Feb 23, 2022 at 09:19:23PM +0800, Shaoxuan Yuan wrote:
> On Wed, Feb 23, 2022 at 8:42 PM Shubham Mishra <shivam828787@gmail.com> wrote:
> >
> > Hi,
>
> Hi,
>
> > I am using mails for code review for the first time, I have some
> > doubts, Can someone please clarify them? -
> > 1. It looks like the cover letter (Including "Range-diff" section) is
> > only for context sharing with reviewers, nothing from it gets merged
> > to the "seek" or any other branch.
>
> The cover letter stands for an introduction/summary to your patches.
> You can also put helpful context in it for better understanding. According
> to my knowledge, it will not be in the commit messages.

Right; the cover letter (along with any notes below the '---' in your
patches do not make it into the commit history).

The range-diff you posted is empty and doesn't look quite right to me...
when I applied both versions of your patches locally and generated a
range-diff myself, I got the expected (non-empty) results.

I'm not sure exactly how you're generating the range-diff locally, but
you may want to make sure that you're picking the previous version
correctly (and not doing something like `git range-diff master HEAD
HEAD`, which is what I suspect may have happened).

> > 2. I wanted to know how the merging process takes place. Once the
> > patch is accepted, do we merge all previous versions of it one after
> > another or every patch is independent so we have to just merge the
> > last accepted patch?
>
> Not so sure about this question. My two cents: generally the most agreed-upon
> patch will be merged, but the exact merging process could vary based
> on the circumstances. Probably Junio can have a better answer to this.

Emily Shaffer did some great work a couple of years ago on a "My First
Contribution" tutorial, which you can find in
Documentation/MyFirstContribution.txt.

The section "My Patch Got Emailed - Now What?" provides a good overview
of the review and queuing process.

Thanks,
Taylor

Re: [PATCH v2 1/2] t0001: avoid pipes with Git on LHS

[Shubham Mishra]

Ah, It's dumb, I'm sorry. This was unintended. The regex I used gave
me all Git on LHS of pipe including this.
I understand it's not a Git command so we don't need to fix this. I
would say We shouldn't merge it.
I will take care to have a final eye on my patch before sending.
Sorry Again.

Thanks,
Shubham

On Wed, Feb 23, 2022 at 11:23 PM Taylor Blau <me@ttaylorr.com> wrote:
>
> On Wed, Feb 23, 2022 at 05:23:46PM +0530, Shubham Mishra wrote:
> > Pipes ignore error codes of LHS command and thu`s we should not use
> > them with Git in tests. As an alternative, use a 'tmp' file to write
> > the Git output so we can test the exit code.
>
> This patch does preserve the existing behavior. But I'm hesitant to
> recommend that we apply this patch, since our test suite assumes that
> commands like find will work, and so we aren't concerned about squashing
> any potential error codes when it's on the left-hand side of the pipe,
> since we assume that it won't fail in general.
>
> (That's notably different from the second patch in this series, where
> the thing on the left-hand side of the pipe is a git invocation. In that
> case, we really _do_ want to avoid having it on the left-hand side of
> the pipe, because we don't have the same error-free expectation there,
> and want to know when it fails).
>
> I think that Ævar gave a nice summary of the above in [1]
>
> Thanks,
> Taylor
>
> [1]: https://lore.kernel.org/git/220222.86pmnf6ket.gmgdl@evledraar.gmail.com/

Re: [PATCH v2 1/2] t0001: avoid pipes with Git on LHS

[Taylor Blau]

On Wed, Feb 23, 2022 at 11:31:41PM +0530, Shubham Mishra wrote:
> Ah, It's dumb, I'm sorry. This was unintended. The regex I used gave
> me all Git on LHS of pipe including this.

It's certainly not dumb :-). You are doing a great job on your first
contribution!

Thanks,
Taylor

Re: [PATCH v2 2/2] t0003: avoid pipes with Git on LHS

[Shubham Mishra]

Thanks for reviewing it :)

On Wed, Feb 23, 2022 at 11:24 PM Taylor Blau <me@ttaylorr.com> wrote:
>
> On Wed, Feb 23, 2022 at 05:23:47PM +0530, Shubham Mishra wrote:
> >  t/t0003-attributes.sh | 9 ++++++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
>
> This patch looks great to me, thanks!
>
>     Reviewed-by: Taylor Blau <me@ttaylorr.com>
>
> Thanks,
> Taylor

Re: [PATCH v2 0/2] avoid pipes with Git on LHS

[Shubham Mishra]

Thanks to both of you, your responded were really insightful.

> The range-diff you posted is empty and doesn't look quite right to me...
> when I applied both versions of your patches locally and generated a
> range-diff myself, I got the expected (non-empty) results.

This is a new command for me, I am looking on the web about range-diff
to know what I did wrong :)

Thanks,
Shubham

Re: [PATCH 1/2] t0001: remove pipes

[Junio C Hamano]

Christian Couder <christian.couder@gmail.com> writes:

> Only the exit code of the command before the pipe is ignored.
>
> Also it's ok to use pipes if the command before the pipe is not `git`.
> We trust regular commands to just work and we test only `git`.

Both very good points.  So ...

>> -               find .git/worktrees -print | sort >expected &&
>> +               find .git/worktrees -print >tmp && sort tmp >expected &&
>
> Please put the `find` and `sort` commands on 2 different lines when
> they are separated with &&.

We do not want to split this pipeline into two commands.

Re: [PATCH v2 1/2] t0001: avoid pipes with Git on LHS

[Junio C Hamano]

Shubham Mishra <shivam828787@gmail.com> writes:

> Pipes ignore error codes of LHS command and thu`s we should not use
> them with Git in tests. As an alternative, use a 'tmp' file to write
> the Git output so we can test the exit code.

I do not know what thu`s is, but the above describes a sensible
criterion to decide which pipe to touch and which pipe to leave
alone.

> -		find .git/worktrees -print | sort >expected &&
> +		find .git/worktrees -print >tmp &&
> +		sort tmp >expected &&

And this change or ...

>  		git -C ../linked-worktree init &&
>  		test_cmp expected-exclude .git/info/exclude &&
>  		test_cmp expected-config .git/config &&
> -		find .git/worktrees -print | sort >actual &&
> +		find .git/worktrees -print >tmp &&
> +		sort tmp >actual &&

... this change squarely contradict the reasoning written in the
proposed log message.  These pipelines place "find" on the upstream
of their pipe, and "find" is not something we are worried about
introducing new bug into.

>  		test_cmp expected actual
>  	)
>  '

Re: [PATCH 1/2] t0001: remove pipes

[Christian Couder]

On Wed, Feb 23, 2022 at 10:07 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Christian Couder <christian.couder@gmail.com> writes:
>
> > Only the exit code of the command before the pipe is ignored.
> >
> > Also it's ok to use pipes if the command before the pipe is not `git`.
> > We trust regular commands to just work and we test only `git`.
>
> Both very good points.  So ...
>
> >> -               find .git/worktrees -print | sort >expected &&
> >> +               find .git/worktrees -print >tmp && sort tmp >expected &&
> >
> > Please put the `find` and `sort` commands on 2 different lines when
> > they are separated with &&.
>
> We do not want to split this pipeline into two commands.

Yeah, right.

Re: [PATCH 0/2] microproject: avoid using pipes in test

[Christian Couder]

On Tue, Feb 22, 2022 at 6:01 PM Shubham Mishra <shivam828787@gmail.com> wrote:
>
> pipes doesn't care about error codes and ignore them thus we should not use them in tests.
> As an easy alternative, I am using a tmp file to write from git command so we can test the exit code.

(Others have already rightly commented on many things, so I am only
focusing on microproject related things.)

As the subject of this cover letter starts with "microproject:" I
would suggest taking a look (or maybe reading again) our page about
microprojects: https://git.github.io/General-Microproject-Information/

Please note that we ask that all related emails start with “[GSoC]” or
“[Outreachy]” or something similar, not "microproject:".

> This is my first contribution that's why I am keeping diff short with an intention to understand the process instead of making impactful change in first attempt. Later, I will send more patches to fix the issue for other files.

Please read the "Only ONE quality focused microproject per student"
section of the above mentioned page.

Thanks for your first contribution!

Re: [PATCH v2 1/2] t0001: avoid pipes with Git on LHS

[Shubham Mishra]

HI Junio,
 I acknowledge, this patch is here by mistake, we don't need to merge it.

Thanks,
Shubham

On Thu, Feb 24, 2022 at 2:40 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Shubham Mishra <shivam828787@gmail.com> writes:
>
> > Pipes ignore error codes of LHS command and thu`s we should not use
> > them with Git in tests. As an alternative, use a 'tmp' file to write
> > the Git output so we can test the exit code.
>
> I do not know what thu`s is, but the above describes a sensible
> criterion to decide which pipe to touch and which pipe to leave
> alone.
>
> > -             find .git/worktrees -print | sort >expected &&
> > +             find .git/worktrees -print >tmp &&
> > +             sort tmp >expected &&
>
> And this change or ...
>
> >               git -C ../linked-worktree init &&
> >               test_cmp expected-exclude .git/info/exclude &&
> >               test_cmp expected-config .git/config &&
> > -             find .git/worktrees -print | sort >actual &&
> > +             find .git/worktrees -print >tmp &&
> > +             sort tmp >actual &&
>
> ... this change squarely contradict the reasoning written in the
> proposed log message.  These pipelines place "find" on the upstream
> of their pipe, and "find" is not something we are worried about
> introducing new bug into.
>
> >               test_cmp expected actual
> >       )
> >  '

Re: [PATCH 0/2] microproject: avoid using pipes in test

[Shubham Mishra]

> (Others have already rightly commented on many things, so I am only
> focusing on microproject related things.)
>
> As the subject of this cover letter starts with "microproject:" I
> would suggest taking a look (or maybe reading again) our page about
> microprojects: https://git.github.io/General-Microproject-Information/
>
> Please note that we ask that all related emails start with “[GSoC]” or
> “[Outreachy]” or something similar, not "microproject:".

Ack.

> > This is my first contribution that's why I am keeping diff short with an intention to understand the process instead of making impactful change in first attempt. Later, I will send more patches to fix the issue for other files.
>
> Please read the "Only ONE quality focused microproject per student"
> section of the above mentioned page.

here I mean in next patches, I will be fixing the same "Git on LHS of
pipe" for other tests. I think that will be considered as the same
microproject and I am still eligible to fix the same thing for other
files?

Thanks,
Shubham

Re: [PATCH 0/2] microproject: avoid using pipes in test

[Christian Couder]

On Thu, Feb 24, 2022 at 6:22 AM Shubham Mishra <shivam828787@gmail.com> wrote:

> > > This is my first contribution that's why I am keeping diff short with an intention to understand the process instead of making impactful change in first attempt. Later, I will send more patches to fix the issue for other files.
> >
> > Please read the "Only ONE quality focused microproject per student"
> > section of the above mentioned page.
>
> here I mean in next patches, I will be fixing the same "Git on LHS of
> pipe" for other tests. I think that will be considered as the same
> microproject and I am still eligible to fix the same thing for other
> files?

In the "Only ONE quality focused microproject per student" section, there is:

=> This means that for a microproject that consist in refactoring or
rewriting a small amount of code, your patch should change only ONE
file, or perhaps 2 files if they are closely related, like “foo.c” and
“foo.h”.

So, no, we really prefer you to focus on 1 file (or 2 files when they
are strongly related) and then move on to your application or regular
patches, reviews, discussions, etc.

Re: [PATCH 0/2] microproject: avoid using pipes in test

[Shubham Mishra]

> So, no, we really prefer you to focus on 1 file (or 2 files when they
> are strongly related) and then move on to your application or regular
> patches, reviews, discussions, etc.

I misunderstood this :(. I will not submit any further patches related
to microprojects but I already sentone patch at Morning -
https://lore.kernel.org/git/20220224054720.23996-1-shivam828787@gmail.com.
Can I finish this?.
Thank you for clarifying.

Thanks,
Shubham.

Re: [PATCH 0/2] microproject: avoid using pipes in test

[Christian Couder]

On Thu, Feb 24, 2022 at 11:13 AM Shubham Mishra <shivam828787@gmail.com> wrote:
>
> > So, no, we really prefer you to focus on 1 file (or 2 files when they
> > are strongly related) and then move on to your application or regular
> > patches, reviews, discussions, etc.
>
> I misunderstood this :(. I will not submit any further patches related
> to microprojects but I already sentone patch at Morning -
> https://lore.kernel.org/git/20220224054720.23996-1-shivam828787@gmail.com.
> Can I finish this?.

Yeah, you can finish it. As I said to another applicant, we are even
likely to help you finish it.

Best,
Christian.