[PATCH] t3701-add-interactive: tighten the check of trace output

[PATCH] t3701-add-interactive: tighten the check of trace output

[SZEDER Gábor]

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(-)

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' '
-- 
2.19.0.rc2.140.g09cf9e37c9

Re: [PATCH] t3701-add-interactive: tighten the check of trace output

[Taylor Blau]

> 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>

Re: [PATCH] t3701-add-interactive: tighten the check of trace output

[Taylor Blau]

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

Re: [PATCH] t3701-add-interactive: tighten the check of trace output

[Jeff King]

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

Re: [PATCH] t3701-add-interactive: tighten the check of trace output

[Junio C Hamano]

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).

Re: [PATCH] t3701-add-interactive: tighten the check of trace output

[SZEDER Gábor]

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.

Re: [PATCH] t3701-add-interactive: tighten the check of trace output

[Jeff King]

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