{"thread":{"id":"49310","subject":"[PATCH] t3701-add-interactive: tighten the check of trace output","startedAt":"2018-09-10T14:07:33Z","lastAt":"2018-09-18T17:43:15Z","messageCount":7,"participants":["SZEDER Gábor","Taylor Blau","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"357785","messageId":"20180910140714.19617-1-szeder.dev@gmail.com","threadId":"49310","inReplyTo":null,"subject":"[PATCH] t3701-add-interactive: tighten the check of trace output","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-09-10T14:07:14Z","receivedAt":"2018-09-10T14:07:33Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"The test 'add -p does not expand argument lists' in\n't3701-add-interactive.sh', added in 7288e12cce (add--interactive: do\nnot expand pathspecs with ls-files, 2017-03-14), checks the GIT_TRACE\nof 'git add -p' to ensure that the name of a tracked file wasn't\npassed around as argument to any of the commands executed as a result\nof undesired pathspec expansion. This check is done with 'grep' using\nthe filename on its own as the pattern, which is too loose a pattern,\nand would match any occurrences of the filename in the trace output,\nnot just those as command arguments. E.g. if a developer were to\nlitter the index handling code with trace_printf()s printing, among\nother things, the name of the just processed cache entry, then that\npattern would mistakenly match these as well, and would fail the test.\n\nTighten this 'grep' pattern to only match trace lines that show the\nexecuted commands.\n\nSigned-off-by: SZEDER Gábor \n---\n t/t3701-add-interactive.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex 609fbfdc31..65dfbc033a 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -540,7 +540,7 @@ test_expect_success 'add -p does not expand argument lists' '\n \t# update it, but we want to be sure that our \".\" pathspec\n \t# was not expanded into the argument list of any command.\n \t# So look only for \"not-changed\".\n-\t! grep not-changed trace.out\n+\t! grep -E \"^trace: (built-in|exec|run_command): .*not-changed\" trace.out\n '\n \n test_expect_success 'hunk-editing handles custom comment char' '\n-- \n2.19.0.rc2.140.g09cf9e37c9\n\n"},{"id":"357786","messageId":"20180910141834.GJ86006@syl","threadId":"49310","inReplyTo":"20180910140714.19617-1-szeder.dev@gmail.com","subject":"Re: [PATCH] t3701-add-interactive: tighten the check of trace output","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2018-09-10T14:18:34Z","receivedAt":"2018-09-10T14:18:40Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"> Tighten this 'grep' pattern to only match trace lines that show the\n> executed commands.\n\nLooks good, and I think that this is the only such occurrence in t3701\nthat needs this treatment.\n\nSigned-off-by: Taylor Blau \n\n"},{"id":"357787","messageId":"20180910154453.GA15270@sigill.intra.peff.net","threadId":"49310","inReplyTo":"20180910140714.19617-1-szeder.dev@gmail.com","subject":"Re: [PATCH] t3701-add-interactive: tighten the check of trace output","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-09-10T15:44:54Z","receivedAt":"2018-09-10T15:44:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 10, 2018 at 04:07:14PM +0200, SZEDER Gábor wrote:\n\n> The test 'add -p does not expand argument lists' in\n> 't3701-add-interactive.sh', added in 7288e12cce (add--interactive: do\n> not expand pathspecs with ls-files, 2017-03-14), checks the GIT_TRACE\n> of 'git add -p' to ensure that the name of a tracked file wasn't\n> passed around as argument to any of the commands executed as a result\n> of undesired pathspec expansion. This check is done with 'grep' using\n> the filename on its own as the pattern, which is too loose a pattern,\n> and would match any occurrences of the filename in the trace output,\n> not just those as command arguments. E.g. if a developer were to\n> litter the index handling code with trace_printf()s printing, among\n> other things, the name of the just processed cache entry, then that\n> pattern would mistakenly match these as well, and would fail the test.\n\nIs this a real thing we're running into? I'd have thought that anybody\nadding index-specific tracing would do it as GIT_TRACE_INDEX. It's\nunfortunate that \"trace commands and processes\" is just GIT_TRACE, and not\nGIT_TRACE_RUN or similar. But that's mostly historical. I wouldn't\nexpect people to add other subsystems to it.\n\nNot that I'm totally opposed to your patch, but it's a little sad that\nwe have to match the specific text used in GIT_TRACE now (and if they\never changed we won't even notice, but rather the test will just become\na silent noop).\n\nI think it would be nice if we could move towards something like:\n\n - move current GIT_TRACE messages to use GIT_TRACE_COMMAND or similar\n\n - abolish trace_printf() without a specific subsystem key\n\n - do one of:\n\n - keep GIT_TRACE as a historical synonym for GIT_TRACE_COMMAND; that\n keeps things working as they are now\n\n - have GIT_TRACE enable _all_ tracing; that's a change in behavior,\n but arguably a more useful thing to have going forward (e.g., when\n you're not sure which traces are even available)\n\nAnd then a test like this would just use GIT_TRACE_COMMAND.\n\n> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\n> index 609fbfdc31..65dfbc033a 100755\n> --- a/t/t3701-add-interactive.sh\n> +++ b/t/t3701-add-interactive.sh\n> @@ -540,7 +540,7 @@ test_expect_success 'add -p does not expand argument lists' '\n> \t# update it, but we want to be sure that our \".\" pathspec\n> \t# was not expanded into the argument list of any command.\n> \t# So look only for \"not-changed\".\n> -\t! grep not-changed trace.out\n> +\t! grep -E \"^trace: (built-in|exec|run_command): .*not-changed\" trace.out\n\nI had a vague recollection that we preferred \"egrep\" to \"grep -E\" due to\nportability. But digging in the history, I could only find \"fgrep\" over\n\"grep -F\". And we seem to have plenty of \"grep -E\" invocations already.\n\n-Peff\n"},{"id":"357806","messageId":"xmqqin3dtgcy.fsf@gitster-ct.c.googlers.com","threadId":"49310","inReplyTo":"20180910154453.GA15270@sigill.intra.peff.net","subject":"Re: [PATCH] t3701-add-interactive: tighten the check of trace output","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-09-10T17:25:01Z","receivedAt":"2018-09-10T17:25:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Not that I'm totally opposed to your patch, but it's a little sad that\n> we have to match the specific text used in GIT_TRACE now (and if they\n> ever changed we won't even notice, but rather the test will just become\n> a silent noop).\n>\n> I think it would be nice if we could move towards something like:\n>\n> - move current GIT_TRACE messages to use GIT_TRACE_COMMAND or similar\n>\n> - abolish trace_printf() without a specific subsystem key\n>\n> - do one of:\n>\n> - keep GIT_TRACE as a historical synonym for GIT_TRACE_COMMAND; that\n> keeps things working as they are now\n>\n> - have GIT_TRACE enable _all_ tracing; that's a change in behavior,\n> but arguably a more useful thing to have going forward (e.g., when\n> you're not sure which traces are even available)\n>\n> And then a test like this would just use GIT_TRACE_COMMAND.\n\nYup. Sounds like a better world.\n\n>\n>> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\n>> index 609fbfdc31..65dfbc033a 100755\n>> --- a/t/t3701-add-interactive.sh\n>> +++ b/t/t3701-add-interactive.sh\n>> @@ -540,7 +540,7 @@ test_expect_success 'add -p does not expand argument lists' '\n>> \t# update it, but we want to be sure that our \".\" pathspec\n>> \t# was not expanded into the argument list of any command.\n>> \t# So look only for \"not-changed\".\n>> -\t! grep not-changed trace.out\n>> +\t! grep -E \"^trace: (built-in|exec|run_command): .*not-changed\" trace.out\n>\n> I had a vague recollection that we preferred \"egrep\" to \"grep -E\" due to\n> portability. But digging in the history, I could only find \"fgrep\" over\n> \"grep -F\". And we seem to have plenty of \"grep -E\" invocations already.\n\nI had the same reaction and came to the same conclusion. FWIW, the\n\"favor fgrep over -F\" was done by you with 87539416 (\"tests: grep\nportability fixes\", 2008-09-30).\n\n"},{"id":"357818","messageId":"20180910191932.GB17224@localhost","threadId":"49310","inReplyTo":"20180910154453.GA15270@sigill.intra.peff.net","subject":"Re: [PATCH] t3701-add-interactive: tighten the check of trace output","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-09-10T19:19:32Z","receivedAt":"2018-09-10T19:19:40Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Mon, Sep 10, 2018 at 11:44:54AM -0400, Jeff King wrote:\n> On Mon, Sep 10, 2018 at 04:07:14PM +0200, SZEDER Gábor wrote:\n> \n> > The test 'add -p does not expand argument lists' in\n> > 't3701-add-interactive.sh', added in 7288e12cce (add--interactive: do\n> > not expand pathspecs with ls-files, 2017-03-14), checks the GIT_TRACE\n> > of 'git add -p' to ensure that the name of a tracked file wasn't\n> > passed around as argument to any of the commands executed as a result\n> > of undesired pathspec expansion. This check is done with 'grep' using\n> > the filename on its own as the pattern, which is too loose a pattern,\n> > and would match any occurrences of the filename in the trace output,\n> > not just those as command arguments. E.g. if a developer were to\n> > litter the index handling code with trace_printf()s printing, among\n> > other things, the name of the just processed cache entry, then that\n> > pattern would mistakenly match these as well, and would fail the test.\n> \n> Is this a real thing we're running into?\n\nWell, we, in general, don't, but that example mentioned in the commit\nmessage does contain autobiographical elements :)\n\n> I'd have thought that anybody\n> adding index-specific tracing would do it as GIT_TRACE_INDEX.\n\nDepends on the purpose, I guess. For tracing that is aimed to become\npart of in git, definitely. However, for my own ad-hoc tracing used\nto try to make sense of some split-index corner cases, trace_printf()\nis perfect.\n\n> It's\n> unfortunate that \"trace commands and processes\" is just GIT_TRACE, and not\n> GIT_TRACE_RUN or similar. But that's mostly historical. I wouldn't\n> expect people to add other subsystems to it.\n> \n> Not that I'm totally opposed to your patch, but it's a little sad that\n> we have to match the specific text used in GIT_TRACE now (and if they\n> ever changed we won't even notice, but rather the test will just become\n> a silent noop).\n> \n> I think it would be nice if we could move towards something like:\n> \n> - move current GIT_TRACE messages to use GIT_TRACE_COMMAND or similar\n> \n> - abolish trace_printf() without a specific subsystem key\n\nNah, please leave trace_printf() alone.\n\n> - do one of:\n> \n> - keep GIT_TRACE as a historical synonym for GIT_TRACE_COMMAND; that\n> keeps things working as they are now\n> \n> - have GIT_TRACE enable _all_ tracing; that's a change in behavior,\n> but arguably a more useful thing to have going forward (e.g., when\n> you're not sure which traces are even available)\n> \n> And then a test like this would just use GIT_TRACE_COMMAND.\n\nExcept for removing keyless trace_printf(), I agree.\n\n"},{"id":"357821","messageId":"20180910193333.GA24850@sigill.intra.peff.net","threadId":"49310","inReplyTo":"20180910191932.GB17224@localhost","subject":"Re: [PATCH] t3701-add-interactive: tighten the check of trace output","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-09-10T19:33:34Z","receivedAt":"2018-09-10T19:33:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 10, 2018 at 09:19:32PM +0200, SZEDER Gábor wrote:\n\n> > I'd have thought that anybody\n> > adding index-specific tracing would do it as GIT_TRACE_INDEX.\n> \n> Depends on the purpose, I guess. For tracing that is aimed to become\n> part of in git, definitely. However, for my own ad-hoc tracing used\n> to try to make sense of some split-index corner cases, trace_printf()\n> is perfect.\n\nI don't mind it as a temporary debug thing, but it would be nice if it\nwasn't a temptation for people to include in their actual patches.\n\n-Peff\n"},{"id":"358407","messageId":"20180918174307.GA87396@syl","threadId":"49310","inReplyTo":"20180910141834.GJ86006@syl","subject":"Re: [PATCH] t3701-add-interactive: tighten the check of trace output","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2018-09-18T17:43:07Z","receivedAt":"2018-09-18T17:43:15Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Sep 10, 2018 at 10:18:34AM -0400, Taylor Blau wrote:\n> Signed-off-by: Taylor Blau \n\nOops, this should be:\n\n Reviewed-by: Taylor Blau \n\nThanks,\nTaylor\n"}]}