| Message ID | 20200117204426.9347-3-shouryashukla.oo@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | t6025: amended changes after suggestions from the community | expand |
On Fri, Jan 17, 2020 at 3:45 PM Shourya Shukla <shouryashukla.oo@gmail.com> wrote: > The exit code of pipes(|) are always ignored, which will create > errors in subsequent statements. Let's handle it by redirecting > its output to a file and capturing return values. Replace pipe > with redirect(>) operator. This is not an accurate description. The proper way to explain this is that exit code of a command upstream of a pipe is lost; the exit code of a command downstream is not lost. We don't want to lose the exit code of a git command, so a git command should not be upstream. (We don't care about non-git commands being upstream when that command's exit code is not relevant.) > Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com> > --- > diff --git a/t/t6025-merge-symlinks.sh b/t/t6025-merge-symlinks.sh > @@ -17,14 +17,13 @@ test_expect_success 'setup' ' > git commit -m initial && > git branch b-symlink && > git branch b-file && > - l=$(printf file | git hash-object -t blob -w --stdin) && This command is fine as-is. We are interested in the exit code of the git command (which is correctly downstream), and we don't care about the exit code of 'printf' (which is upstream), so there is no reason to rewrite this to use temporary files instead. > + printf file >file && > + l=$(git hash-object -t blob -w --stdin) && Sorry, but this just doesn't make sense. You're telling "git hash-object" to take its input from the standard input stream, yet you don't feed anything to it on that stream. If anything, it should have been written like this: l=$(git hash-object -t blob -w --stdin <file) && however, as noted above, there is no reason to avoid pipes in this case, so this rewrite is unnecessary. By the way, it's hard to imagine that this test passed once this change was made (and, if it did pass, then that would likely indicate that the test is somehow flawed.)
Greetings everyone! This is my first ever contribution in the Open-Source Community and I chose Git for this purpose as I have been using this important tool to maintain my projects regularly. In this patch, I have: - modernized these tests so that they meet current Git CodingGuidlines[1] - replaced the pipe operator with the redirection operator so that one can detect the errors easily and precisely - used helper function 'test_path_is_file()' to replace 'test -f' checks in in the program as it improves the readability of the code and provides better error messages Also, I have some questions to better my understanding of the code: - In the statement, > git hash-object -t blob -w --stdin is it necessary to explicitly specify the type 'blob' of the hash-object? I have this question because it is the default type of hash-object. - In the statement, > l=$(printf file | git hash-object -t blob -w --stdin) I have not used the redirection operator as this sub-shell will be executed separately, hence its error cannot be captured therefore the presence of '>' will not matter. Will using '>' improve the code? Thanks, Shourya Shukla Shourya Shukla (3): t6025: modernize style t6025: replace pipe with redirection operator t6025: use helpers to replace test -f <path> t/t6025-merge-symlinks.sh | 97 ++++++++++++++++++++------------------- 1 file changed, 50 insertions(+), 47 deletions(-)
diff --git a/t/t6025-merge-symlinks.sh b/t/t6025-merge-symlinks.sh index 7a19ba8520..5136bf1e13 100755 --- a/t/t6025-merge-symlinks.sh +++ b/t/t6025-merge-symlinks.sh @@ -17,14 +17,13 @@ test_expect_success 'setup' ' git commit -m initial && git branch b-symlink && git branch b-file && - l=$(printf file | git hash-object -t blob -w --stdin) && - echo "120000 $l symlink" | - git update-index --index-info && + printf file >file && + l=$(git hash-object -t blob -w --stdin) && + echo "120000 $l symlink" | git update-index --index-info && git commit -m master && git checkout b-symlink && l=$(printf file-different | git hash-object -t blob -w --stdin) && - echo "120000 $l symlink" | - git update-index --index-info && + echo "120000 $l symlink" | git update-index --index-info && git commit -m b-symlink && git checkout b-file && echo plain-file >symlink &&
The exit code of pipes(|) are always ignored, which will create errors in subsequent statements. Let's handle it by redirecting its output to a file and capturing return values. Replace pipe with redirect(>) operator. Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com> --- t/t6025-merge-symlinks.sh | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)