| Message ID | 20220223115347.3083-2-shivam828787@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | avoid pipes with Git on LHS | expand |
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/
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/
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
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 > ) > '
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 > > ) > > '
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 ) '
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(-)