harden unexpected object types checks

t/t6102-rev-list-unexpected-objects.sh new file +26 -0

new file mode 100755
index 0000000000..472b08528a
@@ -0,0 +1,123 @@
+#!/bin/sh
+
+test_description='git rev-list should handle unexpected object types'
+
+. ./test-lib.sh
+
+test_expect_success 'setup well-formed objects' '
+	blob="$(printf "foo" | git hash-object -w --stdin)" &&
+	tree="$(printf "100644 blob $blob\tfoo" | git mktree)" &&
+	commit="$(git commit-tree $tree -m "first commit")"
+'
+
+test_expect_success 'setup unexpected non-blob entry' '
+	printf "100644 foo\0$(echo $tree | hex2oct)" >broken-tree &&
+	broken_tree="$(git hash-object -w --literally -t tree broken-tree)"
+'
+
+test_expect_failure 'traverse unexpected non-blob entry (lone)' '
+	test_must_fail git rev-list --objects $broken_tree
+'
+
+test_expect_failure 'traverse unexpected non-blob entry (seen)' '
+	test_must_fail git rev-list --objects $tree $broken_tree
+'
+
+test_expect_success 'setup unexpected non-tree entry' '

t/t6102-rev-list-unexpected-objects.sh new file +74 -0

+'
+
+test_expect_success 'traverse unexpected non-commit parent (seen)' '
+	test_must_fail git rev-list --objects $commit $broken_commit \
+		>output 2>&1 &&
+	test_i18ngrep "not a commit" output
+'
+
+test_expect_success 'setup unexpected non-tree root' '
+	git cat-file commit $commit |
+	sed -e "s/$tree/$blob/" >broken-commit &&
+	broken_commit="$(git hash-object -w --literally -t commit \
+		broken-commit)"
+'
+
+test_expect_failure 'traverse unexpected non-tree root (lone)' '
+	test_must_fail git rev-list --objects $broken_commit
+'
+
+test_expect_failure 'traverse unexpected non-tree root (seen)' '
+	test_must_fail git rev-list --objects $blob $broken_commit
+'
+
+test_expect_success 'setup unexpected non-commit tag' '
+	git tag -a -m "tagged commit" tag $commit &&
+	test_when_finished "git tag -d tag" &&
+	git cat-file -p tag | sed -e "s/$commit/$blob/" >broken-tag &&
+	tag=$(git hash-object -w --literally -t tag broken-tag)
+'
+
+test_expect_success 'traverse unexpected non-commit tag (lone)' '
+	test_must_fail git rev-list --objects $tag
+'
+
+test_expect_success 'traverse unexpected non-commit tag (seen)' '
+	test_must_fail git rev-list --objects $blob $tag >output 2>&1 &&
+	test_i18ngrep "not a commit" output
+'
+
+test_expect_success 'setup unexpected non-tree tag' '
+	git tag -a -m "tagged tree" tag $tree &&
+	test_when_finished "git tag -d tag" &&
+	git cat-file -p tag |
+	sed -e "s/$tree/$blob/" >broken-tag &&
+	tag=$(git hash-object -w --literally -t tag broken-tag)
+'
+
+test_expect_success 'traverse unexpected non-tree tag (lone)' '
+	test_must_fail git rev-list --objects $tag
+'
+
+test_expect_success 'traverse unexpected non-tree tag (seen)' '
+	test_must_fail git rev-list --objects $blob $tag >output 2>&1 &&
+	test_i18ngrep "not a tree" output
+'
+
+test_expect_success 'setup unexpected non-blob tag' '
+	git tag -a -m "tagged blob" tag $blob &&
+	test_when_finished "git tag -d tag" &&
+	git cat-file -p tag |
+	sed -e "s/$blob/$commit/" >broken-tag &&
+	tag=$(git hash-object -w --literally -t tag broken-tag)
+'
+
+test_expect_failure 'traverse unexpected non-blob tag (lone)' '
+	test_must_fail git rev-list --objects $tag
+'
+
+test_expect_success 'traverse unexpected non-blob tag (seen)' '
+	test_must_fail git rev-list --objects $commit $tag >output 2>&1 &&
+	test_i18ngrep "not a blob" output
+'
+
+test_done

Signature · 1 line

2.21.0.203.g358da99528

t/t6102-rev-list-unexpected-objects.sh line 36

Quoted 2 lines

t/t6102-rev-list-unexpected-objects.sh new file +2 -0

+test_expect_failure 'traverse unexpected non-tree entry (seen)' '
+	test_must_fail git rev-list --objects $blob $broken_tree >output 2>&1

This test saves standard output and error, but doesn't look at them.

t/t6102-rev-list-unexpected-objects.sh line 42

Quoted 4 lines

t/t6102-rev-list-unexpected-objects.sh new file +4 -0

+test_expect_success 'setup unexpected non-commit parent' '
+	git cat-file commit $commit |
+		perl -lpe "/^author/ && print q(parent $blob)" \
+		>broken-commit &&

Don't run git commands upstream of a pipe, because the pipe hides their exit code. This applies to several other tests below as well.

Wouldn't a 'sed' one-liner suffice, so we won't have yet another perl dependency?

Let A be the object referenced with an unexpected type, and B be the object doing the referencing. Do the following:

- test 'git rev-list --objects A B'. This causes A to be "cached", and presents the above scenario.

Likewise, if we have a tree entry that claims to be a tree (for example) but points to another object type (say, a blob), there are two ways we might find out:

- when we call lookup_tree(), we might find that we've already seen the object referenced as another type, in which case we'd get NULL

- we call lookup_tree() successfully, but when we try to read the object, we find out it's something else.

We should check that we behave sensibly in both cases (especially because it is easy for a malicious actor to provoke one case or the other).

3. have the traversal machinery communicate the failure to the caller, so that it can decide how to proceed without re-evaluting the object itself.

Of those, I think (3) is probably the best path forward. However, this patch does none of them. In the name of expediently fixing the regression to a normal "rev-list --objects" that we use for connectivity checks, this simply restores the pre-7c0fe330d5 behavior of having the traversal die as soon as it fails to load a tree (when --missing is set to MA_ERROR, which is the default).

Quoted 4 lines · SZEDER Gábor

t/t6102-rev-list-unexpected-objects.sh line 36

Quoted 2 lines

t/t6102-rev-list-unexpected-objects.sh new file +2 -0

+test_expect_failure 'traverse unexpected non-tree entry (seen)' '
+	test_must_fail git rev-list --objects $blob $broken_tree >output 2>&1

This test saves standard output and error, but doesn't look at them.

I think we want to be checking for "not a tree" here, which is later added with the fix. But either we should have the test_i18ngrep here initially, or we should add both the redirect and the grep with the fix.

Quoted 7 lines

t/t6102-rev-list-unexpected-objects.sh line 42

Quoted 4 lines

t/t6102-rev-list-unexpected-objects.sh new file +4 -0

+test_expect_success 'setup unexpected non-commit parent' '
+	git cat-file commit $commit |
+		perl -lpe "/^author/ && print q(parent $blob)" \
+		>broken-commit &&

Don't run git commands upstream of a pipe, because the pipe hides their exit code. This applies to several other tests below as well.

I disagree with that rule here. We're not testing "cat-file" in any meaningful way, but just getting some stock output from a known-good commit.

Quoted 2 lines

Wouldn't a 'sed' one-liner suffice, so we won't have yet another perl dependency?

Heh, this was actually the subject of much discussion before the patches hit the list. If you can write such a one-liner that is both readable and portable, please share it. I got disgusted with sed and suggested this perl.

Quoted 6 lines

Quoted 2 lines

Don't run git commands upstream of a pipe, because the pipe hides their exit code. This applies to several other tests below as well.

I disagree with that rule here. We're not testing "cat-file" in any meaningful way, but just getting some stock output from a known-good commit.

It makes auditing harder and sets bad example.

Quoted 7 lines

Quoted 2 lines

Wouldn't a 'sed' one-liner suffice, so we won't have yet another perl dependency?

Heh, this was actually the subject of much discussion before the patches hit the list. If you can write such a one-liner that is both readable and portable, please share it. I got disgusted with sed and suggested this perl.

Hm, so the trivial s/// with '\n' in the replacement part is not portable, then? Oh, well.

Quoted 3 lines · SZEDER Gábor

Quoted 3 lines

t/t6102-rev-list-unexpected-objects.sh new file +3 -0

+   git cat-file commit $commit |
+           perl -lpe "/^author/ && print q(parent $blob)" \
+           >broken-commit &&

Quoted 2 lines

Wouldn't a 'sed' one-liner suffice, so we won't have yet another perl dependency?

Heh, this was actually the subject of much discussion before the patches hit the list. If you can write such a one-liner that is both readable and portable, please share it. I got disgusted with sed and suggested this perl.

Quoted 8 lines · Jeff King

Quoted 6 lines · SZEDER Gábor

t/t6102-rev-list-unexpected-objects.sh line 42

Quoted 3 lines

t/t6102-rev-list-unexpected-objects.sh new file +3 -0

+   git cat-file commit $commit |
+           perl -lpe "/^author/ && print q(parent $blob)" \
+           >broken-commit &&

Wouldn't a 'sed' one-liner suffice, so we won't have yet another perl dependency?

Quoted 4 lines

Heh, this was actually the subject of much discussion before the patches hit the list. If you can write such a one-liner that is both readable and portable, please share it. I got disgusted with sed and suggested this perl.

Trivial and portable 'sed' equivalent:

git cat-file commit $commit | sed "/^author/ { h; s/.*/parent $blob/; G; }"

Quoted 16 lines · Taylor Blau

Let A be the object referenced with an unexpected type, and B be the object doing the referencing. Do the following:

- test 'git rev-list --objects A B'. This causes A to be "cached", and presents the above scenario.

Likewise, if we have a tree entry that claims to be a tree (for example) but points to another object type (say, a blob), there are two ways we might find out:

- when we call lookup_tree(), we might find that we've already seen the object referenced as another type, in which case we'd get NULL

- we call lookup_tree() successfully, but when we try to read the object, we find out it's something else.

Quoted 3 lines

We should check that we behave sensibly in both cases (especially because it is easy for a malicious actor to provoke one case or the other).

I think our pasting together of multiple commits adding the lone/seen cases ended up in some redundancy in the description. In particular, I'm not sure what the first paragraph/bullet quoted above is trying to say, as it corresponds to the second bullet in the later list.

Maybe collapse them together like:

We might hit an unexpected type in two different ways (imagine we have a tree entry that claims to be a tree but actually points to a blob):

- when we call lookup_tree(), we might find that we've already seen the object referenced as a blob, in which case we'd get NULL. We can exercise this with "git rev-list --objects $blob $tree", which guarantees that the blob will have been parsed before we look in the tree. These tests are marked as "seen" in the test script.

- we call lookup_tree() successfully, but when we try to read the object, we find out it's something else. We construct our tests such that $blob is not otherwise mentioned in $tree. These tests are marked as "lone" in the script.

-Peff

Quoted 4 lines · SZEDER Gábor

t/t6102-rev-list-unexpected-objects.sh line 36

Quoted 2 lines

t/t6102-rev-list-unexpected-objects.sh new file +2 -0

+test_expect_failure 'traverse unexpected non-tree entry (seen)' '
+	test_must_fail git rev-list --objects $blob $broken_tree >output 2>&1

This test saves standard output and error, but doesn't look at them.

I think we want to be checking for "not a tree" here, which is later added with the fix. But either we should have the test_i18ngrep here initially, or we should add both the redirect and the grep with the fix.

Quoted 7 lines

t/t6102-rev-list-unexpected-objects.sh line 42

Quoted 4 lines

t/t6102-rev-list-unexpected-objects.sh new file +4 -0

+test_expect_success 'setup unexpected non-commit parent' '
+	git cat-file commit $commit |
+		perl -lpe "/^author/ && print q(parent $blob)" \
+		>broken-commit &&

Don't run git commands upstream of a pipe, because the pipe hides their exit code. This applies to several other tests below as well.

I disagree with that rule here. We're not testing "cat-file" in any meaningful way, but just getting some stock output from a known-good commit.

Quoted 2 lines

Wouldn't a 'sed' one-liner suffice, so we won't have yet another perl dependency?

Heh, this was actually the subject of much discussion before the patches hit the list. If you can write such a one-liner that is both readable and portable, please share it. I got disgusted with sed and suggested this perl.

-Peff

Quoted 14 lines · Eric Sunshine

Quoted 12 lines · Jeff King

Quoted 6 lines · SZEDER Gábor

t/t6102-rev-list-unexpected-objects.sh line 42

Quoted 3 lines

t/t6102-rev-list-unexpected-objects.sh new file +3 -0

+   git cat-file commit $commit |
+           perl -lpe "/^author/ && print q(parent $blob)" \
+           >broken-commit &&

Wouldn't a 'sed' one-liner suffice, so we won't have yet another perl dependency?

Heh, this was actually the subject of much discussion before the patches hit the list. If you can write such a one-liner that is both readable and portable, please share it. I got disgusted with sed and suggested this perl.

Quoted 3 lines

Trivial and portable 'sed' equivalent:

git cat-file commit $commit | sed "/^author/ { h; s/.*/parent $blob/; G; }"

I always forget about the hold space. That's pretty readable (though being sed, it's terse enough that I actually think the perl is more readable; that may be personal taste, though).

Quoted 10 lines · Taylor Blau

3. have the traversal machinery communicate the failure to the caller, so that it can decide how to proceed without re-evaluting the object itself.

Of those, I think (3) is probably the best path forward. However, this patch does none of them. In the name of expediently fixing the regression to a normal "rev-list --objects" that we use for connectivity checks, this simply restores the pre-7c0fe330d5 behavior of having the traversal die as soon as it fails to load a tree (when --missing is set to MA_ERROR, which is the default).

I think this is worth doing, as it restores the earlier behavior. But a few general thoughts (which I've shared already with you, but for the benefit of the list):

- actually doing the "communicate failure to the caller" would probably not be too bad as a single-bit PARSE_FAILED flag in obj->flags. But it does require the caller understanding which objects the traversal would try to parse (i.e., rev-list would have to understand that it is on its own to check blobs, even if they don't have a PARSE_FAILED flag).

- speaking of blobs, this series does not help rev-list find a mis-typed or bit-rotted blob at all, because it never opens the blobs. Does that mean my expectations for rev-list are simply too high, and that we should be expecting fsck-like checks to catch these? I dunno.

It would not be too expensive to convert the existing "do we have the blob" check in rev-list to "do we have it, and is its type correct?". But obviously finding bitrot would be super-expensive. Which leads me to...

- there actually _is_ a --verify-objects option, which would check even blobs for bitrot. It was added long ago in 5a48d24012 (rev-list --verify-object, 2011-09-01) for use with check_connected(). But it was deemed too slow for normal use, and ripped out in d21c463d55 (fetch/receive: remove over-pessimistic connectivity check, 2012-03-15).

That last one implies that we're OK relying on the incoming index-pack to catch these cases (which is going to do a sha1 over each object).

It does seem like we should bother to notice failures when it's _free_ to do so, which is the case with these tree-loading failures. Which is basically what this patch is doing.

-Peff

Quoted 10 lines

Quoted 6 lines

Of those, I think (3) is probably the best path forward. However, this patch does none of them. In the name of expediently fixing the regression to a normal "rev-list --objects" that we use for connectivity checks, this simply restores the pre-7c0fe330d5 behavior of having the traversal die as soon as it fails to load a tree (when --missing is set to MA_ERROR, which is the default).

I think this is worth doing, as it restores the earlier behavior. But a few general thoughts (which I've shared already with you, but for the benefit of the list):

I agree that it's worth doing. One question that I have is _when_ you feel it's good to do. I'm happy to write it and include the change in v2, but if others would be happy not to grow the series too much between re-rolls, I'd be just as pleased to send it in a new series after this one.

Quoted 7 lines · SZEDER Gábor

Quoted 6 lines

Quoted 2 lines

Don't run git commands upstream of a pipe, because the pipe hides their exit code. This applies to several other tests below as well.

I disagree with that rule here. We're not testing "cat-file" in any meaningful way, but just getting some stock output from a known-good commit.

Quoted 1 line

It makes auditing harder and sets bad example.

I disagree that it's a bad example. Just because a reader might not realize that it is sometimes OK and sometimes not, does not make it bad to do so in the OK case. It means the reader needs to understand the rule in order to correctly apply it.

I am sympathetic to the auditing issue, though.

I dunno. In this case it is not too bad to do:

git cat-file commit $commit >good-commit &&
perl ... <good-commit >broken-commit

but I am not sure I am on board with a blanket rule of "git must never be on the left-hand side of a pipe".

Quoted 10 lines

Quoted 7 lines

Quoted 2 lines

Wouldn't a 'sed' one-liner suffice, so we won't have yet another perl dependency?

Heh, this was actually the subject of much discussion before the patches hit the list. If you can write such a one-liner that is both readable and portable, please share it. I got disgusted with sed and suggested this perl.

Hm, so the trivial s/// with '\n' in the replacement part is not portable, then? Oh, well.

I don't think it is, but I could be wrong. POSIX does say that "\n" matches a newline in the pattern space, but nothing about it on the RHS of a substitution. I have a vague feeling of running into problems in the past, but I could just be misremembering.

We also tried matching /^tree/ and using "a\" to append a line, but it was both hard to read and hit portability issues with bsd sed.

-Peff

I don't think it is, but I could be wrong. POSIX does say that "\n" matches a newline in the pattern space, but nothing about it on the RHS of a substitution. I have a vague feeling of running into problems in the past, but I could just be misremembering.

Quoted 8 lines · Jeff King

Quoted 6 lines · SZEDER Gábor

t/t6102-rev-list-unexpected-objects.sh line 42

Quoted 3 lines

t/t6102-rev-list-unexpected-objects.sh new file +3 -0

+   git cat-file commit $commit |
+           perl -lpe "/^author/ && print q(parent $blob)" \
+           >broken-commit &&

Wouldn't a 'sed' one-liner suffice, so we won't have yet another perl dependency?

Quoted 4 lines

Heh, this was actually the subject of much discussion before the patches hit the list. If you can write such a one-liner that is both readable and portable, please share it. I got disgusted with sed and suggested this perl.

Trivial and portable 'sed' equivalent:

git cat-file commit $commit | sed "/^author/ { h; s/.*/parent $blob/; G; }"

Quoted 11 lines · Taylor Blau

Quoted 10 lines

Quoted 6 lines

Of those, I think (3) is probably the best path forward. However, this patch does none of them. In the name of expediently fixing the regression to a normal "rev-list --objects" that we use for connectivity checks, this simply restores the pre-7c0fe330d5 behavior of having the traversal die as soon as it fails to load a tree (when --missing is set to MA_ERROR, which is the default).

I think this is worth doing, as it restores the earlier behavior. But a few general thoughts (which I've shared already with you, but for the benefit of the list):

Quoted 5 lines

I agree that it's worth doing. One question that I have is _when_ you feel it's good to do. I'm happy to write it and include the change in v2, but if others would be happy not to grow the series too much between re-rolls, I'd be just as pleased to send it in a new series after this one.

I'm not sure what "it" is here.

My earlier message was admittedly rambling, but I think I'm arguing that it's OK to continue to include this patch that you already have, and punt further changes to make "rev-list --objects" detect blob problems down the road. I.e., leave the two expect_failures in place that your v1 series ends with.

-Peff

Quoted 9 lines · Jeff King

Quoted 7 lines · SZEDER Gábor

Quoted 6 lines

Quoted 2 lines

Don't run git commands upstream of a pipe, because the pipe hides their exit code. This applies to several other tests below as well.

I disagree with that rule here. We're not testing "cat-file" in any meaningful way, but just getting some stock output from a known-good commit.

Quoted 6 lines

Quoted 1 line

It makes auditing harder and sets bad example.

I disagree that it's a bad example. Just because a reader might not realize that it is sometimes OK and sometimes not, does not make it bad to do so in the OK case. It means the reader needs to understand the rule in order to correctly apply it.

FWIW I thought the rule was something to the effect of "we're hacking on git, any change might segfault in some unexpected test, let's make sure that fails right away", hence the blanket rule in t/README against "! git <cmd>" in favor of "test_must_fail git <cmd>", and "don't feed the output of a git command to a pipe" documented right after that.

Quoted 1 line

I am sympathetic to the auditing issue, though.

Quoted 7 lines

I dunno. In this case it is not too bad to do:

git cat-file commit $commit >good-commit &&
perl ... <good-commit >broken-commit

but I am not sure I am on board with a blanket rule of "git must never be on the left-hand side of a pipe".

Quoted 11 lines

Quoted 10 lines

Quoted 7 lines

Quoted 2 lines

Wouldn't a 'sed' one-liner suffice, so we won't have yet another perl dependency?

Heh, this was actually the subject of much discussion before the patches hit the list. If you can write such a one-liner that is both readable and portable, please share it. I got disgusted with sed and suggested this perl.

Hm, so the trivial s/// with '\n' in the replacement part is not portable, then? Oh, well.

Quoted 7 lines

I don't think it is, but I could be wrong. POSIX does say that "\n" matches a newline in the pattern space, but nothing about it on the RHS of a substitution. I have a vague feeling of running into problems in the past, but I could just be misremembering.

We also tried matching /^tree/ and using "a\" to append a line, but it was both hard to read and hit portability issues with bsd sed.

Quoted 1 line

-Peff

Quoted 14 lines · Eric Sunshine

Quoted 12 lines · Jeff King

Quoted 6 lines · SZEDER Gábor

t/t6102-rev-list-unexpected-objects.sh line 42

Quoted 3 lines

t/t6102-rev-list-unexpected-objects.sh new file +3 -0

+   git cat-file commit $commit |
+           perl -lpe "/^author/ && print q(parent $blob)" \
+           >broken-commit &&

Wouldn't a 'sed' one-liner suffice, so we won't have yet another perl dependency?

Heh, this was actually the subject of much discussion before the patches hit the list. If you can write such a one-liner that is both readable and portable, please share it. I got disgusted with sed and suggested this perl.

Quoted 3 lines

Trivial and portable 'sed' equivalent:

git cat-file commit $commit | sed "/^author/ { h; s/.*/parent $blob/; G; }"

Looks good. I had a bit of head scratching moment when I saw that "perl -lpe" one-liner; this sed expression may not be crystal clear to those who are not used to, but it is not so bad, either.

Quoted 7 lines · Junio C Hamano

Quoted 6 lines · Eric Sunshine

Quoted 3 lines

Quoted 3 lines

t/t6102-rev-list-unexpected-objects.sh line 42

Quoted 3 lines

t/t6102-rev-list-unexpected-objects.sh new file +3 -0

+   git cat-file commit $commit |
+           perl -lpe "/^author/ && print q(parent $blob)" \
+           >broken-commit &&

Trivial and portable 'sed' equivalent:

Quoted 5 lines

Quoted 1 line

git cat-file commit $commit | sed "/^author/ { h; s/.*/parent $blob/; G; }"

Looks good. I had a bit of head scratching moment when I saw that "perl -lpe" one-liner; this sed expression may not be crystal clear to those who are not used to, but it is not so bad, either.

Should I take this as your endorsement of putting 'git' on the left-hand side of a pipe? ;-).

Quoted 10 lines · Taylor Blau

Quoted 9 lines · Junio C Hamano

Quoted 7 lines · Eric Sunshine

Quoted 3 lines

Quoted 3 lines

Review

Quoted 3 lines

(quoted diff) +3 -0

+   git cat-file commit $commit |
+           perl -lpe "/^author/ && print q(parent $blob)" \
+           >broken-commit &&

Trivial and portable 'sed' equivalent:

git cat-file commit $commit | sed "/^author/ { h; s/.*/parent $blob/; G; }"

Quoted 6 lines

Quoted 3 lines

Looks good. I had a bit of head scratching moment when I saw that "perl -lpe" one-liner; this sed expression may not be crystal clear to those who are not used to, but it is not so bad, either.

Should I take this as your endorsement of putting 'git' on the left-hand side of a pipe? ;-).

I suspect that Junio's "Looks good" was referring to the 'sed expression.

With all the recent work of moving away from having Git upstream of a pipe, let's not intentionally introduce a new instance. I wrote the example 'sed' expression that way merely to mirror how the original 'perl' version was written to make it easier to see the equivalence (not because it was intended as an endorsement of having Git upstream of a pipe).

Quoted 1 line · Eric Sunshine

I suspect that Junio's "Looks good" was referring to the 'sed expression.

I think that you are right -- and I'll happily _not_ introduce more Git on the left-hand-side of a pipe instances.

I noticed a few more instances in t6102 [...]

I wrote the following patch, which I've folded into my local copy (and will send with v2):

Quoted 6 lines

With all the recent work of moving away from having Git upstream of a pipe, let's not intentionally introduce a new instance. I wrote the example 'sed' expression that way merely to mirror how the original 'perl' version was written to make it easier to see the equivalence (not because it was intended as an endorsement of having Git upstream of a pipe).

I see, and thank you for the clarification. Let me know if you like the patch above.

Hi Ævar,

Quoted 19 lines · Ævar Arnfjörð Bjarmason

Quoted 9 lines · Jeff King

Quoted 7 lines · SZEDER Gábor

Quoted 6 lines

Quoted 2 lines

Don't run git commands upstream of a pipe, because the pipe hides their exit code. This applies to several other tests below as well.

I disagree with that rule here. We're not testing "cat-file" in any meaningful way, but just getting some stock output from a known-good commit.

Quoted 6 lines

Quoted 1 line

It makes auditing harder and sets bad example.

I disagree that it's a bad example. Just because a reader might not realize that it is sometimes OK and sometimes not, does not make it bad to do so in the OK case. It means the reader needs to understand the rule in order to correctly apply it.

Quoted 5 lines

FWIW I thought the rule was something to the effect of "we're hacking on git, any change might segfault in some unexpected test, let's make sure that fails right away", hence the blanket rule in t/README against "! git <cmd>" in favor of "test_must_fail git <cmd>", and "don't feed the output of a git command to a pipe" documented right after that.

I have some loosely-held opinions on this. Certainly knowing if a change to git caused a segfault in some test is something that we would want to know about, though I'm not sure we're loosing anything by putting 'git' on the left-hand side of a pipe here.

- If someone wrote a change to git that introduced a segfault in 'git cat-file', I'm sure that this would not be the only place that in the suite that would break because of it. Presumably, at least one of those places uses 'test_must_fail git ...' instead

- At the very least, 'broken-commit' doesn't contain what it needs to, the test breaks in another way (albeit further from the actual defect), and the developer finds out about their bug that way.

In any case, these above two might be moot anyways, because I'm almost certain that it will be a rarity for someone to _only_ run t6102, unless it is included in a blanket 'make' from within 't'.

Quoted 1 line

Quoted 1 line

I am sympathetic to the auditing issue, though.

Just to satisfy my curiosity, I put git on the left-hand side of a pipe to see how many such examples exist today:

~/g/git (master) $ git grep 'git cat-file .* |' -- t/t*.sh | wc -l

      179

I'm not going to claim that changing 179 -> 180 is neutral or bad, but it's interesting nonetheless.

Quoted 5 lines

Quoted 5 lines

I dunno. In this case it is not too bad to do:

git cat-file commit $commit >good-commit &&
perl ... <good-commit >broken-commit

Quoted 2 lines

Quoted 2 lines

but I am not sure I am on board with a blanket rule of "git must never be on the left-hand side of a pipe".

I think that I mostly agree with Peff here for the reasons above.

That all said, I don't really want to hold up the series for this alone. Since there don't seem to be many other comments or issues, I would be quite happy to do whatever people are most in favor of.

I basically don't really feel strongly about writing:

git cat-file commit $commit | sed -e ... >broken-commit

as opposed to:

git cat-file commit $commit >good-commit &&
sed -e '' <commit >bad-commit

Quoted 11 lines

Quoted 11 lines

Quoted 10 lines

Quoted 7 lines

Quoted 2 lines

Wouldn't a 'sed' one-liner suffice, so we won't have yet another perl dependency?

Heh, this was actually the subject of much discussion before the patches hit the list. If you can write such a one-liner that is both readable and portable, please share it. I got disgusted with sed and suggested this perl.

Hm, so the trivial s/// with '\n' in the replacement part is not portable, then? Oh, well.

Quoted 7 lines

Quoted 7 lines

I don't think it is, but I could be wrong. POSIX does say that "\n" matches a newline in the pattern space, but nothing about it on the RHS of a substitution. I have a vague feeling of running into problems in the past, but I could just be misremembering.

We also tried matching /^tree/ and using "a\" to append a line, but it was both hard to read and hit portability issues with bsd sed.

I think that all of this discussion is addressed elsewhere in thread, but the gist is that Eric provided a suitable sed invocation that I am going to use instead of Peff's Perl.

Quoted 1 line

Quoted 1 line

-Peff

Thanks,
Taylor

Quoted 7 lines · Taylor Blau

Quoted 1 line · Eric Sunshine

I suspect that Junio's "Looks good" was referring to the 'sed expression.

I think that you are right -- and I'll happily _not_ introduce more Git on the left-hand-side of a pipe instances.

I noticed a few more instances in t6102 [...]

Indeed, SZEDER mentioned those in [1]:

Don't run git commands upstream of a pipe, because the pipe hides their exit code. This applies to several other tests below as well.

[1]: http://public-inbox.org/git/[email protected]/

Quoted 10 lines

I wrote the following patch, which I've folded into my local copy (and will send with v2):

Quoted 6 lines

With all the recent work of moving away from having Git upstream of a pipe, let's not intentionally introduce a new instance. I wrote the example 'sed' expression that way merely to mirror how the original 'perl' version was written to make it easier to see the equivalence (not because it was intended as an endorsement of having Git upstream of a pipe).

Quoted 2 lines

I see, and thank you for the clarification. Let me know if you like the patch above.

Looks fine. Thanks.

Quoted 42 lines · Taylor Blau

Hi Ævar,

Quoted 19 lines · Ævar Arnfjörð Bjarmason

Quoted 9 lines · Jeff King

Quoted 7 lines · SZEDER Gábor

Quoted 6 lines

Quoted 2 lines

Don't run git commands upstream of a pipe, because the pipe hides their exit code. This applies to several other tests below as well.

I disagree with that rule here. We're not testing "cat-file" in any meaningful way, but just getting some stock output from a known-good commit.

Quoted 6 lines

Quoted 1 line

It makes auditing harder and sets bad example.

I disagree that it's a bad example. Just because a reader might not realize that it is sometimes OK and sometimes not, does not make it bad to do so in the OK case. It means the reader needs to understand the rule in order to correctly apply it.

Quoted 5 lines

FWIW I thought the rule was something to the effect of "we're hacking on git, any change might segfault in some unexpected test, let's make sure that fails right away", hence the blanket rule in t/README against "! git <cmd>" in favor of "test_must_fail git <cmd>", and "don't feed the output of a git command to a pipe" documented right after that.

I have some loosely-held opinions on this. Certainly knowing if a change to git caused a segfault in some test is something that we would want to know about, though I'm not sure we're loosing anything by putting 'git' on the left-hand side of a pipe here.

- If someone wrote a change to git that introduced a segfault in 'git cat-file', I'm sure that this would not be the only place that in the suite that would break because of it. Presumably, at least one of those places uses 'test_must_fail git ...' instead

- At the very least, 'broken-commit' doesn't contain what it needs to, the test breaks in another way (albeit further from the actual defect), and the developer finds out about their bug that way.

Quoted 3 lines

In any case, these above two might be moot anyways, because I'm almost certain that it will be a rarity for someone to _only_ run t6102, unless it is included in a blanket 'make' from within 't'.

First. I realize we're talking about the light fixtures in the bike shed at this point, but with that in mind...

I just think it's useful as a general rule, particularly since with the "special setups" in the test mode we've found that all sorts of odd tests we didn't expect to stress test some features turned out to cover edge cases of them, e.g. when "GIT_TEST_COMMIT_GRAPH" was added we found that a bunch of random stuff segfaulted all over the place.

So it's hard to say with any confidence in advance that something isn't going to stress git in some unusual way and isn't useful to guard for segfaults.

Of course if and when it segfaults it's likely to be seen by subsequent tests. In this case I'll note that if git fails we'll happily run not only perl/sed, but then hash-object the subsequent empty file, and then (presumably) fail in the next test.

Quoted 10 lines

Quoted 1 line

Quoted 1 line

I am sympathetic to the auditing issue, though.

Just to satisfy my curiosity, I put git on the left-hand side of a pipe to see how many such examples exist today:

~/g/git (master) $ git grep 'git cat-file .* |' -- t/t*.sh | wc -l

      179

I'm not going to claim that changing 179 -> 180 is neutral or bad, but it's interesting nonetheless.

Separate from the question of if we generally agree that some value of "Y" makes for good coding style or not, we're always going to be in some flux state where a bunch of examples in our existing codebase contradict that, particularly in the test suite.

I think that's a bit unfortunate in some ways. It's the result of the default "policy" that refactoring for refactoring's sakes is generally frowned upon, so we don't tend to go back and mass-fix "X" to "Y" once we agree "Y" is better than "X" for new code, just do it as we go when new code is written, or existing tests are updated for other reasons "while we're at it".

Quoted 10 lines

Quoted 7 lines

Quoted 7 lines

I dunno. In this case it is not too bad to do:

git cat-file commit $commit >good-commit &&
perl ... <good-commit >broken-commit

but I am not sure I am on board with a blanket rule of "git must never be on the left-hand side of a pipe".

I think that I mostly agree with Peff here for the reasons above.

Quoted 3 lines

That all said, I don't really want to hold up the series for this alone. Since there don't seem to be many other comments or issues, I would be quite happy to do whatever people are most in favor of.

FWIW this series LGTM as a whole. I think it says something about the general quality of it that we're all in some giant nitpick thread about perl v.s. sed and git on the LHS of a pipe or not :) I'm happy to have it queued as-is. These test issues are minor...

Quoted 28 lines

I basically don't really feel strongly about writing:

git cat-file commit $commit | sed -e ... >broken-commit

as opposed to:

git cat-file commit $commit >good-commit &&
sed -e '' <commit >bad-commit

Quoted 18 lines

Quoted 18 lines

Quoted 10 lines

Quoted 7 lines

Quoted 2 lines

Wouldn't a 'sed' one-liner suffice, so we won't have yet another perl dependency?

Heh, this was actually the subject of much discussion before the patches hit the list. If you can write such a one-liner that is both readable and portable, please share it. I got disgusted with sed and suggested this perl.

Hm, so the trivial s/// with '\n' in the replacement part is not portable, then? Oh, well.

I don't think it is, but I could be wrong. POSIX does say that "\n" matches a newline in the pattern space, but nothing about it on the RHS of a substitution. I have a vague feeling of running into problems in the past, but I could just be misremembering.

We also tried matching /^tree/ and using "a\" to append a line, but it was both hard to read and hit portability issues with bsd sed.

Quoted 8 lines

I think that all of this discussion is addressed elsewhere in thread, but the gist is that Eric provided a suitable sed invocation that I am going to use instead of Peff's Perl.

Quoted 1 line

Quoted 1 line

-Peff

Thanks,
Taylor