On Thu, Feb 2, 2023 at 11:09 PM Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote: > > On Thu, Feb 2, 2023 at 5:37 PM Junio C Hamano <gitster@xxxxxxxxx> wrote: > > Kostya Farber <kostya.farber@xxxxxxxxx> writes: > > > Add the helper function test_file_path_exists to the > > > interpret pax header test. This change makes it clearer > > > as to what the test is trying to check, in this case whether > > > a file path exists. > > > > > > - if test -h $data || test -e $data > > > + if test -h $data || test_file_path_exists $data > > > > Nothing seems to be adding a new helper whose name is > > test_file_path_exists; the patch expects such a helper already > > exists and uses it in place for existing "test -e". > > > > Perhaps you meant to say "use test_path_exists" not "add helper" on > > the title, and use that function in the patch instead? > > A couple comments... > > The test framework does not define a function named > "test_file_path_exists". Probably "test_path_exists" was intended. > > Delving more deeply, though, this change seems undesirable from a > clarity viewpoint. The function "test_path_exists" is an assertion; > its purpose is to make the test fail if the path is expected to exist > but doesn't. However, in the original code from t5000: > > if test -h $data || test -e $data > then > path=$(get_pax_header $header path) && > if test -n "$path" > then > mv "$data" "$path" || exit 1 > fi > fi > > it is perfectly fine if the path is neither a symbolic link nor an > actual file; that's not considered an error. Therefore, using an > assertion function -- which suggests test failure -- muddles the > intent of the code rather than clarifying it. Okay, that makes sense. Thanks for explaining why this function isn't desirable in this instance, and why it actually reduces the clarity of the test. > Additionally, t/test-lib-functions.sh also defines the function > "test_path_is_symlink" which would seem to be the obvious complement > to "test_path_exists", thus one might have expected the patch to > change the code to: > > if test_path_is_symlink $data || test_path_exists $data > then > ... > > however, "test_path_is_symlink" is also an assertion, thus not really > suitable for this case in which it is acceptable (not an error) if > neither condition holds true. > > So, t5000 seems to be one of those relatively rare cases in which the > raw "test" command is more correct than the higher-level helper > functions. Yes on first glance it would look like test_path_is_symlink would be the obvious complement, but as you say I guess it doesn't make sense in this case for the same reasons as using test_path_exists. It's a good learning experience for me to understand when, and in which contexts, to use the helper functions. Thanks
