| Message ID | 20250301105838.1481-2-danimahendra0904@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | t1403: prefer test_path_exists helper function | expand |
On Sat, Mar 01, 2025 at 04:28:38PM +0530, Mahendra Dani wrote: > test -e does not provide a nice error message when > we hit test failures, so use test_path_exists instead. > > Signed-off-by: Mahendra Dani <danimahendra0904@gmail.com> > --- > t/t1403-show-ref.sh | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/t/t1403-show-ref.sh b/t/t1403-show-ref.sh > index 9d698b3cc3..12f7b60024 100755 > --- a/t/t1403-show-ref.sh > +++ b/t/t1403-show-ref.sh > @@ -196,7 +196,7 @@ test_expect_success 'show-ref --verify with dangling ref' ' > > remove_object() { > file=$(sha1_file "$*") && > - test -e "$file" && > + test_path_exists "$file" && > rm -f "$file" > } && The refactoring is true to the original spirit of the preimage indeed. But we could also improve it even further if we verified that the path not only exists, but exists and is a file via `test_path_is_file()`. If we decide to do that we should also explain the change in the commit message. Thanks! Patrick
On Mon, Mar 3, 2025 at 3:56 PM Patrick Steinhardt <ps@pks.im> wrote: > > On Sat, Mar 01, 2025 at 04:28:38PM +0530, Mahendra Dani wrote: > > test -e does not provide a nice error message when > > we hit test failures, so use test_path_exists instead. > > > > Signed-off-by: Mahendra Dani <danimahendra0904@gmail.com> > > --- > > t/t1403-show-ref.sh | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/t/t1403-show-ref.sh b/t/t1403-show-ref.sh > > index 9d698b3cc3..12f7b60024 100755 > > --- a/t/t1403-show-ref.sh > > +++ b/t/t1403-show-ref.sh > > @@ -196,7 +196,7 @@ test_expect_success 'show-ref --verify with dangling ref' ' > > > > remove_object() { > > file=$(sha1_file "$*") && > > - test -e "$file" && > > + test_path_exists "$file" && > > rm -f "$file" > > } && > > The refactoring is true to the original spirit of the preimage indeed. > But we could also improve it even further if we verified that the path > not only exists, but exists and is a file via `test_path_is_file()`. If > we decide to do that we should also explain the change in the commit > message. > > Thanks! > > Patrick Yes, sure. I will improve it further using the `test_path_is_file()` helper function and change the commit message in v2 patch. Thanks, Mahendra.
Mahendra Dani <danimahendra0904@gmail.com> writes: >> > remove_object() { >> > file=$(sha1_file "$*") && >> > - test -e "$file" && >> > + test_path_exists "$file" && >> > rm -f "$file" >> > } && >> >> The refactoring is true to the original spirit of the preimage indeed. >> But we could also improve it even further if we verified that the path >> not only exists, but exists and is a file via `test_path_is_file()`. If >> we decide to do that we should also explain the change in the commit >> message. > > Yes, sure. > I will improve it further using the `test_path_is_file()` helper > function and change the commit message in v2 patch. You may want to think about why there is "-f" there. If we remove it, do we still need to have any check there?
On Tue, Mar 4, 2025 at 5:35 PM Junio C Hamano <gitster@pobox.com> wrote: > > Mahendra Dani <danimahendra0904@gmail.com> writes: > > >> > remove_object() { > >> > file=$(sha1_file "$*") && > >> > - test -e "$file" && > >> > + test_path_exists "$file" && > >> > rm -f "$file" > >> > } && > >> > >> The refactoring is true to the original spirit of the preimage indeed. > >> But we could also improve it even further if we verified that the path > >> not only exists, but exists and is a file via `test_path_is_file()`. If > >> we decide to do that we should also explain the change in the commit > >> message. > > > > Yes, sure. > > I will improve it further using the `test_path_is_file()` helper > > function and change the commit message in v2 patch. > > You may want to think about why there is "-f" there. If we remove > it, do we still need to have any check there? Here, the "-f" flag in `rm -f "$file"` does not produce an error message even if the file does not exist [1], thus the `test -e "$file"` check was redundant, as pointed out by Patrick in [2]. However, switching to `test_path_is_file()` would provide additional safety by ensuring that we only attempt to remove a regular file and not a directory. [References] 1. https://man7.org/linux/man-pages/man1/rm.1.html 2. https://lore.kernel.org/git/Z8bd3iHrhXb4WH6A@pks.im/
Mahendra Dani <danimahendra0904@gmail.com> writes: > On Tue, Mar 4, 2025 at 5:35 PM Junio C Hamano <gitster@pobox.com> wrote: >> >> Mahendra Dani <danimahendra0904@gmail.com> writes: >> >> >> > remove_object() { >> >> > file=$(sha1_file "$*") && >> >> > - test -e "$file" && >> >> > + test_path_exists "$file" && >> >> > rm -f "$file" >> >> > } && >> >> You may want to think about why there is "-f" there. If we remove >> it, do we still need to have any check there? > > Here, the "-f" flag in `rm -f "$file"` does not produce an error message even > if the file does not exist [1], thus the `test -e "$file"` check was redundant, > as pointed out by Patrick in [2]. So what happens if you dropped "-f" as I hinted? We'll notice the lack of file and the command exits with non-zero status. So "test -e" was not necessary in the first place, was it?
On Tue, Mar 4, 2025 at 10:57 PM Junio C Hamano <gitster@pobox.com> wrote: > > Mahendra Dani <danimahendra0904@gmail.com> writes: > > > On Tue, Mar 4, 2025 at 5:35 PM Junio C Hamano <gitster@pobox.com> wrote: > >> > >> Mahendra Dani <danimahendra0904@gmail.com> writes: > >> > >> >> > remove_object() { > >> >> > file=$(sha1_file "$*") && > >> >> > - test -e "$file" && > >> >> > + test_path_exists "$file" && > >> >> > rm -f "$file" > >> >> > } && > >> > >> You may want to think about why there is "-f" there. If we remove > >> it, do we still need to have any check there? > > > > Here, the "-f" flag in `rm -f "$file"` does not produce an error message even > > if the file does not exist [1], thus the `test -e "$file"` check was redundant, > > as pointed out by Patrick in [2]. > > So what happens if you dropped "-f" as I hinted? We'll notice the > lack of file and the command exits with non-zero status. So "test -e" > was not necessary in the first place, was it? > Yes, due to the use of the "-f" flag, it's not necessary to explicitly check the lack of file using `test -e`. But if we drop the "-f" flag, we will have to check the lack of file using `test -e` or `test_path_is_file()`. Thanks, Mahendra
On Tue, Mar 4, 2025 at 7:05 AM Junio C Hamano <gitster@pobox.com> wrote: > Mahendra Dani <danimahendra0904@gmail.com> writes: > >> > remove_object() { > >> > file=$(sha1_file "$*") && > >> > - test -e "$file" && > >> > + test_path_exists "$file" && > >> > rm -f "$file" > >> > } && > >> > >> The refactoring is true to the original spirit of the preimage indeed. > >> But we could also improve it even further if we verified that the path > >> not only exists, but exists and is a file via `test_path_is_file()`. If > >> we decide to do that we should also explain the change in the commit > >> message. > > > > I will improve it further using the `test_path_is_file()` helper > > function and change the commit message in v2 patch. > > You may want to think about why there is "-f" there. If we remove > it, do we still need to have any check there? That's a good question to ask, but isn't the implied suggestion of dropping "-f" going in the wrong direction? If I'm reading remove_object() correctly, `test -e` is being used as control flow, *not* as an assertion that the file exists. That is, the expectation of the caller is that the file will not exist once the call completes and that remove_object() will return a success code whether the file was present before the call or not. By control flow, I mean that the function, as written, is the same as this more explicit version: remove_object() { file=$(sha1_file "$*") && if test -e "$file" then rm -f "$file" fi } && Given this understanding, then it becomes apparent that this GSoC microproject shouldn't be applying *any* test_path_foo() to this function. As an alternative, given that `rm -f` returns a success code whether or not the file exists, the microproject could instead *remove* the `test -e "$file" &&` line entirely (and the commit message should explain why doing so is a reasonable thing to do).
Mahendra Dani <danimahendra0904@gmail.com> writes: > Yes, due to the use of the "-f" flag, it's not necessary to explicitly > check the lack of file using `test -e`. > But if we drop the "-f" flag, we will have to check the lack of file > using `test -e` or > `test_path_is_file()`. Isn't it the other way around? $ rm -f nosuch ; echo $? 0 $ rm nosuch ; echo $? rm: cannot remove 'nosuch': No such file or directory 1
Eric Sunshine <sunshine@sunshineco.com> writes: > On Tue, Mar 4, 2025 at 7:05 AM Junio C Hamano <gitster@pobox.com> wrote: >> Mahendra Dani <danimahendra0904@gmail.com> writes: >> >> > remove_object() { >> >> > file=$(sha1_file "$*") && >> >> > - test -e "$file" && >> >> > + test_path_exists "$file" && >> >> > rm -f "$file" >> >> > } && > That's a good question to ask, but isn't the implied suggestion of > dropping "-f" going in the wrong direction? If I'm reading > remove_object() correctly, `test -e` is being used as control flow, > *not* as an assertion that the file exists. If sha1_file says the loose object must be at path $file, and the call to test -e "$file" returns false, two things happen in this function: (1) control stops and "rm -f" does not trigger (2) the function returns non-zero status to the caller If you omit the check and say rm "$file" instead, under the same scenario, (1) "rm" is attempted, but there is nothing to remove so the command returns non-zero status, and (2) the function returns that non-zero status to the caller If the file does exist, both will remove the file, and give the caller zero status return. So?
On Tue, Mar 4, 2025 at 11:14 PM Junio C Hamano <gitster@pobox.com> wrote: > > Mahendra Dani <danimahendra0904@gmail.com> writes: > > > Yes, due to the use of the "-f" flag, it's not necessary to explicitly > > check the lack of file using `test -e`. > > But if we drop the "-f" flag, we will have to check the lack of file > > using `test -e` or > > `test_path_is_file()`. > > Isn't it the other way around? > > $ rm -f nosuch ; echo $? > 0 > $ rm nosuch ; echo $? > rm: cannot remove 'nosuch': No such file or directory > 1 > Yes, you are right. With the "-f" flag, `rm` returns exit code 0 irrespective of whether the file is present or not. Thus, the `test -e` check is _required_ if we drop the "-f" flag to return the correct exit code. I apologize for the mistake. Thanks, Mahendra
On Tue, Mar 4, 2025 at 12:49 PM Junio C Hamano <gitster@pobox.com> wrote: > Eric Sunshine <sunshine@sunshineco.com> writes: > > On Tue, Mar 4, 2025 at 7:05 AM Junio C Hamano <gitster@pobox.com> wrote: > >> Mahendra Dani <danimahendra0904@gmail.com> writes: > >> >> > remove_object() { > >> >> > file=$(sha1_file "$*") && > >> >> > - test -e "$file" && > >> >> > + test_path_exists "$file" && > >> >> > rm -f "$file" > >> >> > } && > > That's a good question to ask, but isn't the implied suggestion of > > dropping "-f" going in the wrong direction? If I'm reading > > remove_object() correctly, `test -e` is being used as control flow, > > *not* as an assertion that the file exists. > > If sha1_file says the loose object must be at path $file, and the > call to test -e "$file" returns false, two things happen in this > function: > > (1) control stops and "rm -f" does not trigger > (2) the function returns non-zero status to the caller True enough. I was misreading `test -e "$file"` as _only_ control flow. However, it's still not clear to me why this function is making the `test -e "$file"` assertion in the first place or why the enclosing test should care, especially since that assertion is only checking that `git commit` worked correctly, but that's not the intent of this particular test[1]. So, `test -e "$file"` seems pointless or at least misleading. > If you omit the check and say rm "$file" instead, under the same > scenario, (1) "rm" is attempted, but there is nothing to remove so > the command returns non-zero status, and (2) the function returns > that non-zero status to the caller Yes, I understood the implication of your suggestion, but as mentioned above, it's not clear (at least to me) why `test -e "$file"` is there at all since this test is not about checking functionality of `git commit`. [1]: d01b8203ec (show-ref: detect dangling refs under --verify as well, 2017-01-23) doesn't explain why `test -e "$file"` was used.
Mahendra Dani <danimahendra0904@gmail.com> writes: > On Tue, Mar 4, 2025 at 11:14 PM Junio C Hamano <gitster@pobox.com> wrote: >> >> Mahendra Dani <danimahendra0904@gmail.com> writes: >> >> > Yes, due to the use of the "-f" flag, it's not necessary to explicitly >> > check the lack of file using `test -e`. >> > But if we drop the "-f" flag, we will have to check the lack of file >> > using `test -e` or >> > `test_path_is_file()`. >> >> Isn't it the other way around? >> >> $ rm -f nosuch ; echo $? >> 0 >> $ rm nosuch ; echo $? >> rm: cannot remove 'nosuch': No such file or directory >> 1 >> > > Yes, you are right. > With the "-f" flag, `rm` returns exit code 0 irrespective of whether > the file is present or not. > Thus, the `test -e` check is _required_ if we drop the "-f" flag to > return the correct exit code. > > I apologize for the mistake. No need for an apology when correcting technical mistakes, which we all make.
On Tue, Mar 4, 2025 at 1:07 PM Eric Sunshine <sunshine@sunshineco.com> wrote: > > >> >> > remove_object() { > > >> >> > file=$(sha1_file "$*") && > > >> >> > - test -e "$file" && > > >> >> > + test_path_exists "$file" && > > >> >> > rm -f "$file" > > >> >> > } && > > However, it's still not clear to me why this function is making the > `test -e "$file"` assertion in the first place or why the enclosing > test should care, especially since that assertion is only checking > that `git commit` worked correctly, but that's not the intent of this > particular test[1]. So, `test -e "$file"` seems pointless or at least > misleading. Perhaps, I'm falling into the trap of assuming that a lone `git commit` in a new repository will unconditionally create a loose object, and that that will always be the case? If, down the road, `git commit` no longer works that way, then the assumption about the loose object becomes invalid, in which case I can see how the `test -e "$file"` assertion is protecting the test against that future.
Eric Sunshine <sunshine@sunshineco.com> writes: > On Tue, Mar 4, 2025 at 12:49 PM Junio C Hamano <gitster@pobox.com> wrote: >> Eric Sunshine <sunshine@sunshineco.com> writes: >> > On Tue, Mar 4, 2025 at 7:05 AM Junio C Hamano <gitster@pobox.com> wrote: >> >> Mahendra Dani <danimahendra0904@gmail.com> writes: >> >> >> > remove_object() { >> >> >> > file=$(sha1_file "$*") && >> >> >> > - test -e "$file" && >> >> >> > + test_path_exists "$file" && >> >> >> > rm -f "$file" >> >> >> > } && > ... > Yes, I understood the implication of your suggestion, but as mentioned > above, it's not clear (at least to me) why `test -e "$file"` is there > at all since this test is not about checking functionality of `git > commit`. Yup, I do not see much point in "test -e" there in the original, and it does not change even if it were "test -f". I would understand if the author wanted to have a "slightly more intelligent 'rm -f' that knows where a loose object is located, and removes the named object no matter what", but if the objective were to ensure the object is missing, I wouldn't have written it to return non-zero when the object were missing in the first place. And if the purpose of the function is to catch unexpected cases, such as "the loose object file should be there but isn't" and "we located the file but we failed to remove it", then it shouldn't have the 'test -e' guard and 'rm' shouldn't have been used with '-f'. So, I agree with you that the original is already iffy. Thanks.
diff --git a/t/t1403-show-ref.sh b/t/t1403-show-ref.sh index 9d698b3cc3..12f7b60024 100755 --- a/t/t1403-show-ref.sh +++ b/t/t1403-show-ref.sh @@ -196,7 +196,7 @@ test_expect_success 'show-ref --verify with dangling ref' ' remove_object() { file=$(sha1_file "$*") && - test -e "$file" && + test_path_exists "$file" && rm -f "$file" } &&
test -e does not provide a nice error message when we hit test failures, so use test_path_exists instead. Signed-off-by: Mahendra Dani <danimahendra0904@gmail.com> --- t/t1403-show-ref.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)