| Message ID | 20180910140714.19617-1-szeder.dev@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | t3701-add-interactive: tighten the check of trace output | expand |
> Tighten this 'grep' pattern to only match trace lines that show the > executed commands. Looks good, and I think that this is the only such occurrence in t3701 that needs this treatment. Signed-off-by: Taylor Blau <me@ttaylorr.com>
On Mon, Sep 10, 2018 at 04:07:14PM +0200, SZEDER Gábor wrote: > The test 'add -p does not expand argument lists' in > 't3701-add-interactive.sh', added in 7288e12cce (add--interactive: do > not expand pathspecs with ls-files, 2017-03-14), checks the GIT_TRACE > of 'git add -p' to ensure that the name of a tracked file wasn't > passed around as argument to any of the commands executed as a result > of undesired pathspec expansion. This check is done with 'grep' using > the filename on its own as the pattern, which is too loose a pattern, > and would match any occurrences of the filename in the trace output, > not just those as command arguments. E.g. if a developer were to > litter the index handling code with trace_printf()s printing, among > other things, the name of the just processed cache entry, then that > pattern would mistakenly match these as well, and would fail the test. Is this a real thing we're running into? I'd have thought that anybody adding index-specific tracing would do it as GIT_TRACE_INDEX. It's unfortunate that "trace commands and processes" is just GIT_TRACE, and not GIT_TRACE_RUN or similar. But that's mostly historical. I wouldn't expect people to add other subsystems to it. Not that I'm totally opposed to your patch, but it's a little sad that we have to match the specific text used in GIT_TRACE now (and if they ever changed we won't even notice, but rather the test will just become a silent noop). I think it would be nice if we could move towards something like: - move current GIT_TRACE messages to use GIT_TRACE_COMMAND or similar - abolish trace_printf() without a specific subsystem key - do one of: - keep GIT_TRACE as a historical synonym for GIT_TRACE_COMMAND; that keeps things working as they are now - have GIT_TRACE enable _all_ tracing; that's a change in behavior, but arguably a more useful thing to have going forward (e.g., when you're not sure which traces are even available) And then a test like this would just use GIT_TRACE_COMMAND. > diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh > index 609fbfdc31..65dfbc033a 100755 > --- a/t/t3701-add-interactive.sh > +++ b/t/t3701-add-interactive.sh > @@ -540,7 +540,7 @@ test_expect_success 'add -p does not expand argument lists' ' > # update it, but we want to be sure that our "." pathspec > # was not expanded into the argument list of any command. > # So look only for "not-changed". > - ! grep not-changed trace.out > + ! grep -E "^trace: (built-in|exec|run_command): .*not-changed" trace.out I had a vague recollection that we preferred "egrep" to "grep -E" due to portability. But digging in the history, I could only find "fgrep" over "grep -F". And we seem to have plenty of "grep -E" invocations already. -Peff
Jeff King <peff@peff.net> writes: > Not that I'm totally opposed to your patch, but it's a little sad that > we have to match the specific text used in GIT_TRACE now (and if they > ever changed we won't even notice, but rather the test will just become > a silent noop). > > I think it would be nice if we could move towards something like: > > - move current GIT_TRACE messages to use GIT_TRACE_COMMAND or similar > > - abolish trace_printf() without a specific subsystem key > > - do one of: > > - keep GIT_TRACE as a historical synonym for GIT_TRACE_COMMAND; that > keeps things working as they are now > > - have GIT_TRACE enable _all_ tracing; that's a change in behavior, > but arguably a more useful thing to have going forward (e.g., when > you're not sure which traces are even available) > > And then a test like this would just use GIT_TRACE_COMMAND. Yup. Sounds like a better world. > >> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh >> index 609fbfdc31..65dfbc033a 100755 >> --- a/t/t3701-add-interactive.sh >> +++ b/t/t3701-add-interactive.sh >> @@ -540,7 +540,7 @@ test_expect_success 'add -p does not expand argument lists' ' >> # update it, but we want to be sure that our "." pathspec >> # was not expanded into the argument list of any command. >> # So look only for "not-changed". >> - ! grep not-changed trace.out >> + ! grep -E "^trace: (built-in|exec|run_command): .*not-changed" trace.out > > I had a vague recollection that we preferred "egrep" to "grep -E" due to > portability. But digging in the history, I could only find "fgrep" over > "grep -F". And we seem to have plenty of "grep -E" invocations already. I had the same reaction and came to the same conclusion. FWIW, the "favor fgrep over -F" was done by you with 87539416 ("tests: grep portability fixes", 2008-09-30).
On Mon, Sep 10, 2018 at 11:44:54AM -0400, Jeff King wrote: > On Mon, Sep 10, 2018 at 04:07:14PM +0200, SZEDER Gábor wrote: > > > The test 'add -p does not expand argument lists' in > > 't3701-add-interactive.sh', added in 7288e12cce (add--interactive: do > > not expand pathspecs with ls-files, 2017-03-14), checks the GIT_TRACE > > of 'git add -p' to ensure that the name of a tracked file wasn't > > passed around as argument to any of the commands executed as a result > > of undesired pathspec expansion. This check is done with 'grep' using > > the filename on its own as the pattern, which is too loose a pattern, > > and would match any occurrences of the filename in the trace output, > > not just those as command arguments. E.g. if a developer were to > > litter the index handling code with trace_printf()s printing, among > > other things, the name of the just processed cache entry, then that > > pattern would mistakenly match these as well, and would fail the test. > > Is this a real thing we're running into? Well, we, in general, don't, but that example mentioned in the commit message does contain autobiographical elements :) > I'd have thought that anybody > adding index-specific tracing would do it as GIT_TRACE_INDEX. Depends on the purpose, I guess. For tracing that is aimed to become part of in git, definitely. However, for my own ad-hoc tracing used to try to make sense of some split-index corner cases, trace_printf() is perfect. > It's > unfortunate that "trace commands and processes" is just GIT_TRACE, and not > GIT_TRACE_RUN or similar. But that's mostly historical. I wouldn't > expect people to add other subsystems to it. > > Not that I'm totally opposed to your patch, but it's a little sad that > we have to match the specific text used in GIT_TRACE now (and if they > ever changed we won't even notice, but rather the test will just become > a silent noop). > > I think it would be nice if we could move towards something like: > > - move current GIT_TRACE messages to use GIT_TRACE_COMMAND or similar > > - abolish trace_printf() without a specific subsystem key Nah, please leave trace_printf() alone. > - do one of: > > - keep GIT_TRACE as a historical synonym for GIT_TRACE_COMMAND; that > keeps things working as they are now > > - have GIT_TRACE enable _all_ tracing; that's a change in behavior, > but arguably a more useful thing to have going forward (e.g., when > you're not sure which traces are even available) > > And then a test like this would just use GIT_TRACE_COMMAND. Except for removing keyless trace_printf(), I agree.
On Mon, Sep 10, 2018 at 09:19:32PM +0200, SZEDER Gábor wrote: > > I'd have thought that anybody > > adding index-specific tracing would do it as GIT_TRACE_INDEX. > > Depends on the purpose, I guess. For tracing that is aimed to become > part of in git, definitely. However, for my own ad-hoc tracing used > to try to make sense of some split-index corner cases, trace_printf() > is perfect. I don't mind it as a temporary debug thing, but it would be nice if it wasn't a temptation for people to include in their actual patches. -Peff
On Mon, Sep 10, 2018 at 10:18:34AM -0400, Taylor Blau wrote: > Signed-off-by: Taylor Blau <me@ttaylorr.com> Oops, this should be: Reviewed-by: Taylor Blau <me@ttaylorr> Thanks, Taylor
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh index 609fbfdc31..65dfbc033a 100755 --- a/t/t3701-add-interactive.sh +++ b/t/t3701-add-interactive.sh @@ -540,7 +540,7 @@ test_expect_success 'add -p does not expand argument lists' ' # update it, but we want to be sure that our "." pathspec # was not expanded into the argument list of any command. # So look only for "not-changed". - ! grep not-changed trace.out + ! grep -E "^trace: (built-in|exec|run_command): .*not-changed" trace.out ' test_expect_success 'hunk-editing handles custom comment char' '
The test 'add -p does not expand argument lists' in 't3701-add-interactive.sh', added in 7288e12cce (add--interactive: do not expand pathspecs with ls-files, 2017-03-14), checks the GIT_TRACE of 'git add -p' to ensure that the name of a tracked file wasn't passed around as argument to any of the commands executed as a result of undesired pathspec expansion. This check is done with 'grep' using the filename on its own as the pattern, which is too loose a pattern, and would match any occurrences of the filename in the trace output, not just those as command arguments. E.g. if a developer were to litter the index handling code with trace_printf()s printing, among other things, the name of the just processed cache entry, then that pattern would mistakenly match these as well, and would fail the test. Tighten this 'grep' pattern to only match trace lines that show the executed commands. Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com> --- t/t3701-add-interactive.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)