| Message ID | 49104273b8b801fc61811347120c5f4c42a3700b.1624974969.git.congdanhqx@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | t640{0,2}: preserve ls-files exit status code | expand |
On Tue, Jun 29, 2021 at 9:57 AM Đoàn Trần Công Danh <congdanhqx@gmail.com> wrote: > In t6400, we're checking number of files in the index and the working > tree by piping the output of "git ls-files" to "wc -l", thus losing the > exit status code of git. > > Let's write the output of "git ls-files" to a temporary file, in order > to check exit status code of "git ls-files" properly. Thanks, the simplicity of this version over the previous attempts is appealing. Just a few extremely minor style nits below; don't know if any of them are worth a re-roll. > Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com> > --- > diff --git a/t/t6400-merge-df.sh b/t/t6400-merge-df.sh > @@ -9,6 +9,20 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME > > . ./test-lib.sh > > +check_ls_files_count() { style: funcname () { > + local ops val > + if test "$#" -le 2 I also &&-chain the `local` declaration: local ops val && if test "$#" -le 2 By making it easy to see the `&&` upfront, when new code is inserted, there is a better chance that the &&-chain will be kept intact: local ops val && my new code && if test "$#" -le 2 > + then > + BUG "Expect 2 or more arguments" > + fi && A quick grep of the tests indicates that they are consistent about using lowercase for the first word in a BUG(): BUG "expect 2 or more arguments" > + ops="$1" && > + val="$2" && > + shift 2 && > + mkdir -p .git/trash && > + git ls-files "$@" >.git/trash/output && > + test_line_count "$ops" "$val" .git/trash/output > +}
Eric Sunshine <sunshine@sunshineco.com> writes: >> +check_ls_files_count() { > > style: funcname () { > ... > I also &&-chain the `local` declaration: > > local ops val && > if test "$#" -le 2 > ... > A quick grep of the tests indicates that they are consistent about > using lowercase for the first word in a BUG(): Thanks for a pair of sharp eyes, Eric, in your review. > - test 5 -eq $(git ls-files -s | wc -l) && > - test 4 -eq $(git ls-files -u | wc -l) && > + check_ls_files_count = 5 -s && > + check_ls_files_count = 4 -u && I have one more comment on the main part of the patch. It is easy to see that this conversion is correctly done in this particular patch from the way 5/4 and -s/u are reproduced from the preimage to the postimage, but I doubt that readers in the future, who long have forgotten that the "-s" came from "ls-files -s", would find the new form easy to read and understand. Do we have the same helper duplicated across two test scripts? I wonder if it is worth adding a single copy that forces the callers to spell out the command name in test-lib.sh and make the above into something like test_output_wc_l = 5 ls-files -s or even test_output_wc_l = 5 git ls-files -s That way, it is easier to see what command is being run (yes, I know you have _ls_files_ in the middle of the name of the custom helper, but the thing is that "-s" and "_ls_files_" in the middle of the helper are so far apart that it is not immediately obvious what the argument "-s" is about), and by not having two identical copies, we have less risk of them drifting apart. Hmm?
On Tue, Jun 29, 2021 at 6:49 PM Junio C Hamano <gitster@pobox.com> wrote: > I wonder if it is worth adding a single copy that forces the callers > to spell out the command name in test-lib.sh and make the above into > something like > > test_output_wc_l = 5 ls-files -s > > or even > > test_output_wc_l = 5 git ls-files -s > > That way, it is easier to see what command is being run (yes, I know > you have _ls_files_ in the middle of the name of the custom helper, > but the thing is that "-s" and "_ls_files_" in the middle of the > helper are so far apart that it is not immediately obvious what the > argument "-s" is about), and by not having two identical copies, we > have less risk of them drifting apart. > > Hmm? I may be misunderstanding your suggestion, but isn't the proposed test_output_wc_l() function the same as what Danh had originally implemented several re-rolls back (though he named it test_line_count_cmd())?
Eric Sunshine <sunshine@sunshineco.com> writes: > I may be misunderstanding your suggestion, but isn't the proposed > test_output_wc_l() function the same as what Danh had originally > implemented several re-rolls back (though he named it > test_line_count_cmd())? Could be, except that I recall we saw extra noises like --out/--err that weren't used and contaminating the current working directory, which are gone from the latest iteration. The simplification compared to that iteration is quite welcome---it made the resulting code that uses the helper easier to follow compared to the earlier attempts. But this round simplifies it too much and the results got harder to follow by burying the command name in the helper and made it less obvious that the last part of the helper's parameters are arguments given to ls-files, I would think.
On 2021-06-29 20:36:36-0700, Junio C Hamano <gitster@pobox.com> wrote: > Eric Sunshine <sunshine@sunshineco.com> writes: > > > I may be misunderstanding your suggestion, but isn't the proposed > > test_output_wc_l() function the same as what Danh had originally > > implemented several re-rolls back (though he named it > > test_line_count_cmd())? > > Could be, except that I recall we saw extra noises like --out/--err > that weren't used and contaminating the current working directory, > which are gone from the latest iteration. Yes, in v{1,2}, there's the extra noises of --out and --err, but it's gone in v3. I guess you're thinking about the contamination of $PWD iff it's not a git worktree. That could be simplified by BUG-ging if $PWD is not a git worktree. Maybe, you're thinking about the extra noises to handle the failure run of command under check? That could be dropped, too. Would you mind looking at v3 1/4 again to see what is your opinion there? I don't mind re-roll a same or simplified version of v3, with s/test_line_count_cmd/test_output_wc_l/ if you see fit. > The simplification > compared to that iteration is quite welcome---it made the resulting > code that uses the helper easier to follow compared to the earlier > attempts. But this round simplifies it too much and the results got > harder to follow by burying the command name in the helper and made > it less obvious that the last part of the helper's parameters are > arguments given to ls-files, I would think.
Đoàn Trần Công Danh <congdanhqx@gmail.com> writes: >> Could be, except that I recall we saw extra noises like --out/--err >> that weren't used and contaminating the current working directory, >> which are gone from the latest iteration. > > Yes, in v{1,2}, there's the extra noises of --out and --err, > but it's gone in v3. > > I guess you're thinking about the contamination of $PWD iff it's not > a git worktree. That could be simplified by BUG-ging if $PWD is not > a git worktree. No. I am not thinking about that. I do not think it is a big loss if the helper cannot be used in non-repository. > Maybe, you're thinking about the extra noises to handle the failure run > of command under check? That could be dropped, too. No. I am not thinking about that, either. > Would you mind looking at v3 1/4 again to see what is your opinion > there? I don't mind re-roll a same or simplified version of v3, > with s/test_line_count_cmd/test_output_wc_l/ if you see fit. Let's not go back that far. This is taken from v4 (t/t6400) ... local ops val && if test "$#" -le 2 then BUG "Expect 2 or more arguments" fi && ops="$1" && val="$2" && shift 2 && mkdir -p .git/trash && "$@" >.git/trash/output && test_line_count "$ops" "$val" .git/trash/output ... except that it runs '"$@"' instead of 'git ls-files "$@"', so that we could try running things other than ls-files, and that would be mostly good enough. We may want to be prepared for a caller who wants to use the helper from within a subdirectory by not hardcoding ".git/trash", though. Something along the lines of ... local ops val && + local trashdir=$(git rev-parse --git-dir)/trash && if test ... ... - mkdir -p .git/trash && - "$@" >".git/trash/output" && - test_line_count "$ops" "$val" .git/trash/output + mkdir -p "$trashdir" && + "$@" >"$trashdir/output" && + test_line_count "$ops" "$val" "$trashdir/output"
diff --git a/t/t6400-merge-df.sh b/t/t6400-merge-df.sh index 38700d29b5..c2888323c1 100755 --- a/t/t6400-merge-df.sh +++ b/t/t6400-merge-df.sh @@ -9,6 +9,20 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME . ./test-lib.sh +check_ls_files_count() { + local ops val + if test "$#" -le 2 + then + BUG "Expect 2 or more arguments" + fi && + ops="$1" && + val="$2" && + shift 2 && + mkdir -p .git/trash && + git ls-files "$@" >.git/trash/output && + test_line_count "$ops" "$val" .git/trash/output +} + test_expect_success 'prepare repository' ' echo Hello >init && git add init && @@ -82,13 +96,13 @@ test_expect_success 'modify/delete + directory/file conflict' ' git checkout delete^0 && test_must_fail git merge modify && - test 5 -eq $(git ls-files -s | wc -l) && - test 4 -eq $(git ls-files -u | wc -l) && + check_ls_files_count = 5 -s && + check_ls_files_count = 4 -u && if test "$GIT_TEST_MERGE_ALGORITHM" = ort then - test 0 -eq $(git ls-files -o | wc -l) + check_ls_files_count = 0 -o else - test 1 -eq $(git ls-files -o | wc -l) + check_ls_files_count = 1 -o fi && test_path_is_file letters/file && @@ -103,13 +117,13 @@ test_expect_success 'modify/delete + directory/file conflict; other way' ' test_must_fail git merge delete && - test 5 -eq $(git ls-files -s | wc -l) && - test 4 -eq $(git ls-files -u | wc -l) && + check_ls_files_count = 5 -s && + check_ls_files_count = 4 -u && if test "$GIT_TEST_MERGE_ALGORITHM" = ort then - test 0 -eq $(git ls-files -o | wc -l) + check_ls_files_count = 0 -o else - test 1 -eq $(git ls-files -o | wc -l) + check_ls_files_count = 1 -o fi && test_path_is_file letters/file &&
In t6400, we're checking number of files in the index and the working tree by piping the output of "git ls-files" to "wc -l", thus losing the exit status code of git. Let's write the output of "git ls-files" to a temporary file, in order to check exit status code of "git ls-files" properly. Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com> --- t/t6400-merge-df.sh | 30 ++++++++++++++++++++++-------- 1 file changed, 22 insertions(+), 8 deletions(-)