| Message ID | 5a20a97658fa8e6c874c9c9cafb2cf49e39f94d6.1593536481.git.me@ttaylorr.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | commit-graph: introduce 'core.useBloomFilters' | expand |
On Tue, Jun 30, 2020 at 1:17 PM Taylor Blau <me@ttaylorr.com> wrote: > In a759bfa9ee (t4216: add end to end tests for git log with Bloom > filters, 2020-04-06), a 'rm' invocation was added without a > corresponding '&&' chain. > > This ends up working fine when the file already exists, in which case > 'rm' exits cleanly and the rest of the function executes normally. When > the file does _not_ exist, however, 'rm' returns an unclean exit code, > causing the function to terminate. This explanation makes no sense. Since this command was not part of the &&-chain, its failure would not cause the function to terminate prematurely nor would it affect the return value of the function. This explanation would make sense, however, if you're talking about the behavior _after_ fixing the broken &&-chain. > Fix this by making the test use an '&&'-chain, and passing '-f' to > ignore missing files (as can be the case when specifying which tests are > '--run'). The entire commit message is talking about implementation details and merely repeats what the subject and patch itself already say quite clearly; anyone familiar with 'rm' understands implicitly that '-f' must be added to incorporate it into the &&-chain if the file's presence is not guaranteed. Thus, you could drop the entire body of the commit message without losing clarity... With one minor exception: What is much more interesting for the reader to know is whether the file being removed is guaranteed to exist (in which case '-f' is unnecessary) or may be missing (requiring '-f'), and under what conditions it might be missing. The very last part of the last sentence of the current commit message gives a good hint about the latter, thus would be a good bit to retain. > diff --git a/t/t4216-log-bloom.sh b/t/t4216-log-bloom.sh > @@ -53,7 +53,7 @@ sane_unset GIT_TRACE2_PERF_BRIEF > sane_unset GIT_TRACE2_CONFIG_PARAMS Not related to this patch, but 'sane_unset' is pointless outside of a test since there is no &&-chain to maintain. Plain 'unset' would work just as well and be less misleading. > setup () { > - rm "$TRASH_DIRECTORY/trace.perf" > + rm -f "$TRASH_DIRECTORY/trace.perf" &&
On Tue, Jun 30, 2020 at 01:50:22PM -0400, Eric Sunshine wrote: > On Tue, Jun 30, 2020 at 1:17 PM Taylor Blau <me@ttaylorr.com> wrote: > > In a759bfa9ee (t4216: add end to end tests for git log with Bloom > > filters, 2020-04-06), a 'rm' invocation was added without a > > corresponding '&&' chain. > > > > This ends up working fine when the file already exists, in which case > > 'rm' exits cleanly and the rest of the function executes normally. When > > the file does _not_ exist, however, 'rm' returns an unclean exit code, > > causing the function to terminate. > > This explanation makes no sense. Since this command was not part of > the &&-chain, its failure would not cause the function to terminate > prematurely nor would it affect the return value of the function. This > explanation would make sense, however, if you're talking about the > behavior _after_ fixing the broken &&-chain. Fair enough. For what it's worth, this explanation *does* make sense if you 'set -e' beforehand, which I am accustomed to (and had incorrectly assumed that tests in 't' also have 'set -e', when they do not). I've corrected the patch and shortened it to account for your suggestions. Mind taking a look at the updated version and telling me what you think? --- >8 --- Subject: [PATCH] t4216: fix broken '&&'-chain The 'rm' added in a759bfa9ee (t4216: add end to end tests for git log with Bloom filters, 2020-04-06) should be placed within the function's '&&'-chain. The file being removed may not exist (for eg., in the case of '--run', in which case it may not be generated beforehand by a skipped test), and so add '-f' to account for the file's optional existence. Helped-by: Eric Sunshine <sunshine@sunshineco.com> Signed-off-by: Taylor Blau <me@ttaylorr.com> --- t/t4216-log-bloom.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/t/t4216-log-bloom.sh b/t/t4216-log-bloom.sh index c855bcd3e7..0b4cc4f8d1 100755 --- a/t/t4216-log-bloom.sh +++ b/t/t4216-log-bloom.sh @@ -53,7 +53,7 @@ sane_unset GIT_TRACE2_PERF_BRIEF sane_unset GIT_TRACE2_CONFIG_PARAMS setup () { - rm "$TRASH_DIRECTORY/trace.perf" + rm -f "$TRASH_DIRECTORY/trace.perf" && git -c core.commitGraph=false log --pretty="format:%s" $1 >log_wo_bloom && GIT_TRACE2_PERF="$TRASH_DIRECTORY/trace.perf" git -c core.commitGraph=true log --pretty="format:%s" $1 >log_w_bloom } -- 2.27.0.224.g4cfa086e50
On Tue, Jun 30, 2020 at 01:50:22PM -0400, Eric Sunshine wrote: > > diff --git a/t/t4216-log-bloom.sh b/t/t4216-log-bloom.sh > > @@ -53,7 +53,7 @@ sane_unset GIT_TRACE2_PERF_BRIEF > > sane_unset GIT_TRACE2_CONFIG_PARAMS > > Not related to this patch, but 'sane_unset' is pointless outside of a > test since there is no &&-chain to maintain. Plain 'unset' would work > just as well and be less misleading. We do this in lots of other places, too. Try: git grep sane_unset | grep -v '&&' Though seeing the variables they cover, I think many of them may come from the same few authors. :) I wonder if it is worth keeping these as sane_unset, though. Your comment is based on the knowledge that the "sane" part of the function is ignoring the return value. But we could conceivably have other portability fixes (it's not impossible that a shell wants one "unset" per variable, for example), in which case we'd want it in more places. That's hypothetical, of course, but saying "just use our portable unset wrapper that behaves sanely" seems like exactly what these call sites want, now and in a hypothetical future. -Peff
On Tue, Jun 30, 2020 at 02:39:28PM -0400, Taylor Blau wrote: > > > This ends up working fine when the file already exists, in which case > > > 'rm' exits cleanly and the rest of the function executes normally. When > > > the file does _not_ exist, however, 'rm' returns an unclean exit code, > > > causing the function to terminate. > > > > This explanation makes no sense. Since this command was not part of > > the &&-chain, its failure would not cause the function to terminate > > prematurely nor would it affect the return value of the function. This > > explanation would make sense, however, if you're talking about the > > behavior _after_ fixing the broken &&-chain. > > Fair enough. For what it's worth, this explanation *does* make sense if > you 'set -e' beforehand, which I am accustomed to (and had incorrectly > assumed that tests in 't' also have 'set -e', when they do not). If we _really_ want to nitpick, it probably wouldn't terminate under "set -e" because the call to "setup" is itself part of an &&-chain, which suppresses "-e" handling (which is one of the many confusing "set -e" behaviors that led us to avoid it in the first place). But definitely your revised commit message below is more accurate. However... > --- >8 --- > > Subject: [PATCH] t4216: fix broken '&&'-chain > > The 'rm' added in a759bfa9ee (t4216: add end to end tests for git log > with Bloom filters, 2020-04-06) should be placed within the function's > '&&'-chain. > > The file being removed may not exist (for eg., in the case of '--run', > in which case it may not be generated beforehand by a skipped test), and > so add '-f' to account for the file's optional existence. Is the &&-chain really broken, or is the first command simply not part of that chain? Perhaps a question for philosophers, but the more applied question here is: what are we improving, and why? The original code handled the fact that the file might not exist by not including its exit code in the &&-chain which leads to the function's return value. Your new code does so by putting it in the &&-chain but asking "rm" to ignore errors. Is one better than the other? I think so, but my argument would be more along the lines of: - without "-f", "rm" will complain about a missing file, which is distracting noise in the test log - once "-f" is added in to suppress that, we might as well add the command to the &&-chain. That's our normal style, so readers don't have to wonder if it's important or not. Plus it would help avoid a broken chain if more commands are added at the beginning of the function. -Peff
On Tue, Jun 30, 2020 at 03:03:25PM -0400, Jeff King wrote: > On Tue, Jun 30, 2020 at 02:39:28PM -0400, Taylor Blau wrote: > > > > > This ends up working fine when the file already exists, in which case > > > > 'rm' exits cleanly and the rest of the function executes normally. When > > > > the file does _not_ exist, however, 'rm' returns an unclean exit code, > > > > causing the function to terminate. > > > > > > This explanation makes no sense. Since this command was not part of > > > the &&-chain, its failure would not cause the function to terminate > > > prematurely nor would it affect the return value of the function. This > > > explanation would make sense, however, if you're talking about the > > > behavior _after_ fixing the broken &&-chain. > > > > Fair enough. For what it's worth, this explanation *does* make sense if > > you 'set -e' beforehand, which I am accustomed to (and had incorrectly > > assumed that tests in 't' also have 'set -e', when they do not). > > If we _really_ want to nitpick, it probably wouldn't terminate under > "set -e" because the call to "setup" is itself part of an &&-chain, > which suppresses "-e" handling (which is one of the many confusing "set > -e" behaviors that led us to avoid it in the first place). I learned something new about 'set -e'! I don't mind nitpicking at all, it's useful information to know... > But definitely your revised commit message below is more accurate. > > However... > > > --- >8 --- > > > > Subject: [PATCH] t4216: fix broken '&&'-chain > > > > The 'rm' added in a759bfa9ee (t4216: add end to end tests for git log > > with Bloom filters, 2020-04-06) should be placed within the function's > > '&&'-chain. > > > > The file being removed may not exist (for eg., in the case of '--run', > > in which case it may not be generated beforehand by a skipped test), and > > so add '-f' to account for the file's optional existence. > > Is the &&-chain really broken, or is the first command simply not part > of that chain? Perhaps a question for philosophers, but the more applied > question here is: what are we improving, and why? > > The original code handled the fact that the file might not exist by not > including its exit code in the &&-chain which leads to the function's > return value. Your new code does so by putting it in the &&-chain but > asking "rm" to ignore errors. Is one better than the other? > > I think so, but my argument would be more along the lines of: > > - without "-f", "rm" will complain about a missing file, which is > distracting noise in the test log > > - once "-f" is added in to suppress that, we might as well add the > command to the &&-chain. That's our normal style, so readers don't > have to wonder if it's important or not. Plus it would help avoid a > broken chain if more commands are added at the beginning of the > function. I made the change for basically these reasons, but mostly to bring this function into good style as with the rest of our test suite (there are a handful of other minor nits that we could look at, such as some odd spacing, etc.). Whether or not all of this needs to go into the commit message... I don't know. On the one hand, I think that your explanation here is clearer than what I wrote in the commit message, but on the other hand, I think that amending it again may be belaboring an otherwise simple change. If you feel strongly, though, I'm happy to send a revised patch. > -Peff Thanks, Taylor
On Tue, Jun 30, 2020 at 03:12:31PM -0400, Taylor Blau wrote: > > I think so, but my argument would be more along the lines of: > > > > - without "-f", "rm" will complain about a missing file, which is > > distracting noise in the test log > > > > - once "-f" is added in to suppress that, we might as well add the > > command to the &&-chain. That's our normal style, so readers don't > > have to wonder if it's important or not. Plus it would help avoid a > > broken chain if more commands are added at the beginning of the > > function. > > I made the change for basically these reasons, but mostly to bring this > function into good style as with the rest of our test suite (there are a > handful of other minor nits that we could look at, such as some odd > spacing, etc.). > > Whether or not all of this needs to go into the commit message... I > don't know. On the one hand, I think that your explanation here is > clearer than what I wrote in the commit message, but on the other hand, > I think that amending it again may be belaboring an otherwise simple > change. > > If you feel strongly, though, I'm happy to send a revised patch. I agree it's a pretty trivial patch, but I think if it's worth applying at all, then it's worth justifying it appropriately. -Peff
On Tue, Jun 30, 2020 at 3:03 PM Jeff King <peff@peff.net> wrote: > [...] what are we improving, and why? > > The original code handled the fact that the file might not exist by not > including its exit code in the &&-chain which leads to the function's > return value. Your new code does so by putting it in the &&-chain but > asking "rm" to ignore errors. Is one better than the other? > > I think so, but my argument would be more along the lines of: > > - without "-f", "rm" will complain about a missing file, which is > distracting noise in the test log Indeed, a nice detail when reading verbose test output; one less thing to distract the attention from the real/important problems. > - once "-f" is added in to suppress that, we might as well add the > command to the &&-chain. That's our normal style, so readers don't > have to wonder if it's important or not. Plus it would help avoid a > broken chain if more commands are added at the beginning of the > function. The bit about commands possibly being added at the beginning of the function probably deserves its own bullet point. I often (relatively speaking) cite that reason when asking people to &&-chain variable assignments at the beginning of a function. func () { a=1 && b=2 && ... }
diff --git a/t/t4216-log-bloom.sh b/t/t4216-log-bloom.sh index c855bcd3e7..0b4cc4f8d1 100755 --- a/t/t4216-log-bloom.sh +++ b/t/t4216-log-bloom.sh @@ -53,7 +53,7 @@ sane_unset GIT_TRACE2_PERF_BRIEF sane_unset GIT_TRACE2_CONFIG_PARAMS setup () { - rm "$TRASH_DIRECTORY/trace.perf" + rm -f "$TRASH_DIRECTORY/trace.perf" && git -c core.commitGraph=false log --pretty="format:%s" $1 >log_wo_bloom && GIT_TRACE2_PERF="$TRASH_DIRECTORY/trace.perf" git -c core.commitGraph=true log --pretty="format:%s" $1 >log_w_bloom }
In a759bfa9ee (t4216: add end to end tests for git log with Bloom filters, 2020-04-06), a 'rm' invocation was added without a corresponding '&&' chain. This ends up working fine when the file already exists, in which case 'rm' exits cleanly and the rest of the function executes normally. When the file does _not_ exist, however, 'rm' returns an unclean exit code, causing the function to terminate. Fix this by making the test use an '&&'-chain, and passing '-f' to ignore missing files (as can be the case when specifying which tests are '--run'). Signed-off-by: Taylor Blau <me@ttaylorr.com> --- t/t4216-log-bloom.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)