| Message ID | da9f52a82406ffc909e9c5f2b6b5e77818d972c0.1652210824.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | scalar: implement the subcommand "diagnose" | expand |
On Tue, May 10 2022, Johannes Schindelin via GitGitGadget wrote: > From: Johannes Schindelin <johannes.schindelin@gmx.de> > > The `scalar` command needs a Scalar enlistment for many subcommands, and > looks in the current directory for such an enlistment (traversing the > parent directories until it finds one). > > These is subcommands can also be called with an optional argument > specifying the enlistment. Here, too, we traverse parent directories as > needed, until we find an enlistment. > > However, if the specified directory does not even exist, or is not a > directory, we should stop right there, with an error message. > > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> > --- > contrib/scalar/scalar.c | 6 ++++-- > contrib/scalar/t/t9099-scalar.sh | 5 +++++ > 2 files changed, 9 insertions(+), 2 deletions(-) > > diff --git a/contrib/scalar/scalar.c b/contrib/scalar/scalar.c > index 1ce9c2b00e8..00dcd4b50ef 100644 > --- a/contrib/scalar/scalar.c > +++ b/contrib/scalar/scalar.c > @@ -43,9 +43,11 @@ static void setup_enlistment_directory(int argc, const char **argv, > usage_with_options(usagestr, options); > > /* find the worktree, determine its corresponding root */ > - if (argc == 1) > + if (argc == 1) { > strbuf_add_absolute_path(&path, argv[0]); > - else if (strbuf_getcwd(&path) < 0) > + if (!is_directory(path.buf)) > + die(_("'%s' does not exist"), path.buf); > + } else if (strbuf_getcwd(&path) < 0) > die(_("need a working directory")); > > strbuf_trim_trailing_dir_sep(&path); > diff --git a/contrib/scalar/t/t9099-scalar.sh b/contrib/scalar/t/t9099-scalar.sh > index 2e1502ad45e..9d83fdf25e8 100755 > --- a/contrib/scalar/t/t9099-scalar.sh > +++ b/contrib/scalar/t/t9099-scalar.sh > @@ -85,4 +85,9 @@ test_expect_success 'scalar delete with enlistment' ' > test_path_is_missing cloned > ' > > +test_expect_success '`scalar [...] <dir>` errors out when dir is missing' ' > + ! scalar run config cloned 2>err && Needs to use test_must_fail, not !
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: >> +test_expect_success '`scalar [...] <dir>` errors out when dir is missing' ' >> + ! scalar run config cloned 2>err && > > Needs to use test_must_fail, not ! Good eyes and careful reading are very much appreciated, but in this case, doesn't such an improvement depend on an update to teach test_must_fail_acceptable about scalar being whitelisted?
On Wed, May 18 2022, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >>> +test_expect_success '`scalar [...] <dir>` errors out when dir is missing' ' >>> + ! scalar run config cloned 2>err && >> >> Needs to use test_must_fail, not ! > > Good eyes and careful reading are very much appreciated, but in this > case, doesn't such an improvement depend on an update to teach > test_must_fail_acceptable about scalar being whitelisted? Yes, I think so (but haven't tested it just now), but it's a relatively small change to t/test-lib-functions.sh. I was just noting the potential hidden segfault etc., the issue remains in v5.
Hi Ævar, On Fri, 20 May 2022, Ævar Arnfjörð Bjarmason wrote: > > On Wed, May 18 2022, Junio C Hamano wrote: > > > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > > > >>> +test_expect_success '`scalar [...] <dir>` errors out when dir is missing' ' > >>> + ! scalar run config cloned 2>err && > >> > >> Needs to use test_must_fail, not ! > > > > Good eyes and careful reading are very much appreciated, but in this > > case, doesn't such an improvement depend on an update to teach > > test_must_fail_acceptable about scalar being whitelisted? > > Yes, I think so (but haven't tested it just now), but it's a relatively > small change to t/test-lib-functions.sh. Let it be noted that I fully agree with Junio that good eyes and careful reading are very much appreciated. And that in this case, that would have implied noticing that `test_must_fail` is reserved for Git commands. Scalar is not (yet?) a Git command. Ciao, Johannes
On Fri, May 20 2022, Johannes Schindelin wrote: > Hi Ævar, > > On Fri, 20 May 2022, Ævar Arnfjörð Bjarmason wrote: > >> >> On Wed, May 18 2022, Junio C Hamano wrote: >> >> > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: >> > >> >>> +test_expect_success '`scalar [...] <dir>` errors out when dir is missing' ' >> >>> + ! scalar run config cloned 2>err && >> >> >> >> Needs to use test_must_fail, not ! >> > >> > Good eyes and careful reading are very much appreciated, but in this >> > case, doesn't such an improvement depend on an update to teach >> > test_must_fail_acceptable about scalar being whitelisted? >> >> Yes, I think so (but haven't tested it just now), but it's a relatively >> small change to t/test-lib-functions.sh. > > Let it be noted that I fully agree with Junio that good eyes and careful > reading are very much appreciated. And that in this case, that would have > implied noticing that `test_must_fail` is reserved for Git commands. > > Scalar is not (yet?) a Git command. "test-tool" isn't "git" either, so I think this argument is a non-starter. As the documentation for "test_must_fail" notes the distinction is whether something is "system-supplied". I.e. we're not going to test whether "grep" segfaults, but we should test our own code to see if it segfaults. The scalar code is code we ship and test, so we should use the helper that doesn't hide a segfault. I don't understand why you wouldn't think that's the obvious fix here, adding "scalar" to that whitelist is a one-line fix, and clearly yields a more useful end result than a test silently hiding segfaults.
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: >> Scalar is not (yet?) a Git command. > > "test-tool" isn't "git" either, so I think this argument is a > non-starter. > > As the documentation for "test_must_fail" notes the distinction is > whether something is "system-supplied". I.e. we're not going to test > whether "grep" segfaults, but we should test our own code to see if it > segfaults. > > The scalar code is code we ship and test, so we should use the helper > that doesn't hide a segfault. > > I don't understand why you wouldn't think that's the obvious fix here, > adding "scalar" to that whitelist is a one-line fix, and clearly yields > a more useful end result than a test silently hiding segfaults. FWIW, I don't, either.
Hi Junio, On Sat, 21 May 2022, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > > >> Scalar is not (yet?) a Git command. > > > > "test-tool" isn't "git" either, so I think this argument is a > > non-starter. > > > > As the documentation for "test_must_fail" notes the distinction is > > whether something is "system-supplied". I.e. we're not going to test > > whether "grep" segfaults, but we should test our own code to see if it > > segfaults. > > > > The scalar code is code we ship and test, so we should use the helper > > that doesn't hide a segfault. > > > > I don't understand why you wouldn't think that's the obvious fix here, > > adding "scalar" to that whitelist is a one-line fix, and clearly yields > > a more useful end result than a test silently hiding segfaults. > > FWIW, I don't, either. Because we are still talking about code that lives as much encapsulated inside `contrib/scalar/` as possible. The `! scalar` call is in `contrib/scalar/t/t9099-scalar.sh`. To make it work with Git's test suite, you would have to bleed an implementation detail of something inside `contrib/` into `t/test-lib-functions.sh`. Not what we want, at this stage. Ciao, Dscho
On Tue, May 24 2022, Johannes Schindelin wrote: > Hi Junio, > > On Sat, 21 May 2022, Junio C Hamano wrote: > >> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: >> >> >> Scalar is not (yet?) a Git command. >> > >> > "test-tool" isn't "git" either, so I think this argument is a >> > non-starter. >> > >> > As the documentation for "test_must_fail" notes the distinction is >> > whether something is "system-supplied". I.e. we're not going to test >> > whether "grep" segfaults, but we should test our own code to see if it >> > segfaults. >> > >> > The scalar code is code we ship and test, so we should use the helper >> > that doesn't hide a segfault. >> > >> > I don't understand why you wouldn't think that's the obvious fix here, >> > adding "scalar" to that whitelist is a one-line fix, and clearly yields >> > a more useful end result than a test silently hiding segfaults. >> >> FWIW, I don't, either. > > Because we are still talking about code that lives as much encapsulated > inside `contrib/scalar/` as possible. > > The `! scalar` call is in `contrib/scalar/t/t9099-scalar.sh`. > > To make it work with Git's test suite, you would have to bleed an > implementation detail of something inside `contrib/` into > `t/test-lib-functions.sh`. The "scalar" command is already built by the top-level Makefile, so I don't think the distinction you're trying to maintain here even exists in practice. I.e. if we ran with this strict reasoning then surely "scalar" belongs on there just as much as "test-tool" does. Both are built by our main build process, and thus should have corresponding adjustments in our main test code, just as is already the case for both "git" and "test-tool". But even if that wasn't the case I'd still be of the view that we should add "scalar" to that list. It's just a matter of potential time sinks in the future. If we introduce a hidden segfault in the scalar code and don't notice for some time because we're using that test pattern that's going to suck, and likely to waste a lot of time. We might even ship a broken command to users. Whereas having "scalar" on that list is going to be a relatively easy matter of grepping and doing some boilerplate changes if and when we ever "git rm" it entirely, or "promote it" from contrib or whatever. I also think that just getting rid of that whitelist entirely is an acceptable solution. Perhaps it's just being overzealous in forbidding everything except "git", we should still not use it for the likes of "grep", but we could just leave that to the documentation. But I suspect Junio would disagree with that, so in lieu of that ...
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > Both are built by our main build process, and thus should have > corresponding adjustments in our main test code, just as is already the > case for both "git" and "test-tool". > > But even if that wasn't the case I'd still be of the view that we should > add "scalar" to that list. > > It's just a matter of potential time sinks in the future. If we > introduce a hidden segfault in the scalar code and don't notice for some > time because we're using that test pattern that's going to suck, and > likely to waste a lot of time. We might even ship a broken command to > users. > > Whereas having "scalar" on that list is going to be a relatively easy > matter of grepping and doing some boilerplate changes if and when we > ever "git rm" it entirely, or "promote it" from contrib or whatever. In addition, it already is an actual time sink that causes us send a lot more bytes back and forth than the number of bytes necessary to send a reroll that adds one liner to the same step. > I also think that just getting rid of that whitelist entirely is an > acceptable solution. Perhaps it's just being overzealous in forbidding > everything except "git", we should still not use it for the likes of > "grep", but we could just leave that to the documentation. It indeed is tempting entry into a slippery slope, and I'd see it as a change bigger than we could comfortably make as a "while at it" change. We can stop arguing and instead send in a reroll that squashes in something like this, which shouldn't be controversial, I would say. t/test-lib-functions.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git i/t/test-lib-functions.sh w/t/test-lib-functions.sh index 93c03380d4..8899eaabed 100644 --- i/t/test-lib-functions.sh +++ w/t/test-lib-functions.sh @@ -1106,7 +1106,7 @@ test_must_fail_acceptable () { fi case "$1" in - git|__git*|test-tool|test_terminal) + git|__git*|scalar|test-tool|test_terminal) return 0 ;; *)
Hi Junio, On Tue, 24 May 2022, Junio C Hamano wrote: > We can stop arguing and instead send in a reroll that squashes in > something like this, which shouldn't be controversial, I would say. > > t/test-lib-functions.sh | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git i/t/test-lib-functions.sh w/t/test-lib-functions.sh > index 93c03380d4..8899eaabed 100644 > --- i/t/test-lib-functions.sh > +++ w/t/test-lib-functions.sh > @@ -1106,7 +1106,7 @@ test_must_fail_acceptable () { > fi > > case "$1" in > - git|__git*|test-tool|test_terminal) > + git|__git*|scalar|test-tool|test_terminal) > return 0 > ;; > *) > > > > It is still wrong to adjust Git's test suite for a user that is not part of Git proper. But if your pragmatism says that this is the only way we can venture on to more productive venues, I won't argue against that :-) Ciao, Dscho
diff --git a/contrib/scalar/scalar.c b/contrib/scalar/scalar.c index 1ce9c2b00e8..00dcd4b50ef 100644 --- a/contrib/scalar/scalar.c +++ b/contrib/scalar/scalar.c @@ -43,9 +43,11 @@ static void setup_enlistment_directory(int argc, const char **argv, usage_with_options(usagestr, options); /* find the worktree, determine its corresponding root */ - if (argc == 1) + if (argc == 1) { strbuf_add_absolute_path(&path, argv[0]); - else if (strbuf_getcwd(&path) < 0) + if (!is_directory(path.buf)) + die(_("'%s' does not exist"), path.buf); + } else if (strbuf_getcwd(&path) < 0) die(_("need a working directory")); strbuf_trim_trailing_dir_sep(&path); diff --git a/contrib/scalar/t/t9099-scalar.sh b/contrib/scalar/t/t9099-scalar.sh index 2e1502ad45e..9d83fdf25e8 100755 --- a/contrib/scalar/t/t9099-scalar.sh +++ b/contrib/scalar/t/t9099-scalar.sh @@ -85,4 +85,9 @@ test_expect_success 'scalar delete with enlistment' ' test_path_is_missing cloned ' +test_expect_success '`scalar [...] <dir>` errors out when dir is missing' ' + ! scalar run config cloned 2>err && + grep "cloned. does not exist" err +' + test_done