{"thread":{"id":"51689","subject":"[PATCH] t0021: make sure clean filter runs","startedAt":"2019-08-20T06:56:33Z","lastAt":"2019-08-23T08:34:53Z","messageCount":14,"participants":["Thomas Gummerer","Junio C Hamano","Johannes Sixt","SZEDER Gábor"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"380773","messageId":"20190820065625.128130-1-t.gummerer@gmail.com","threadId":"51689","inReplyTo":null,"subject":"[PATCH] t0021: make sure clean filter runs","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2019-08-20T06:56:25Z","receivedAt":"2019-08-20T06:56:33Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"In t0021.15 one of the things we are checking is that the clean filter\nis run when checking out empty-branch.  The clean filter needs to be\nrun to make sure there are no modifications on the file system for the\ntest.r file, and thus it isn't dangerous to overwrite it.\n\nHowever in the current test setup it is not always necessary to run\nthe clean filter, and thus the test sometimes fails, as debug.log\nisn't written.\n\nThis happens when test.r has an older mtime than the index itself.\nThat mtime is also recorded as stat data for test.r in the index, and\nbased on the heuristic we're using for index entries, git correctly\nassumes this file is up-to-date.\n\nUsually this test succeeds because the mtime of test.r is the same as\nthe mtime of the index.  In this case test.r is racily clean, so git\nactually checks the contents, for which the clean filter is run.\n\nFix the test by updating the mtime of test.r, so git is forced to\ncheck the contents of the file, and the clean filter is run as the\ntest expects.\n\nSigned-off-by: Thomas Gummerer <t.gummerer@gmail.com>\n---\n t/t0021-conversion.sh | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh\nindex e10f5f787f..66f75005d5 100755\n--- a/t/t0021-conversion.sh\n+++ b/t/t0021-conversion.sh\n@@ -390,6 +390,7 @@ test_expect_success PERL 'required process filter should filter data' '\n \t\tEOF\n \t\ttest_cmp_exclude_clean expected.log debug.log &&\n \n+\t\ttouch test.r &&\n \t\tfilter_git checkout --quiet --no-progress empty-branch &&\n \t\tcat >expected.log <<-EOF &&\n \t\t\tSTART\n-- \n2.23.0.rc2.194.ge5444969c9\n\n"},{"id":"380823","messageId":"xmqqlfvnvhpl.fsf@gitster-ct.c.googlers.com","threadId":"51689","inReplyTo":"20190820065625.128130-1-t.gummerer@gmail.com","subject":"Re: [PATCH] t0021: make sure clean filter runs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-08-20T18:01:58Z","receivedAt":"2019-08-20T18:02:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Gummerer <t.gummerer@gmail.com> writes:\n\n> Fix the test by updating the mtime of test.r, so git is forced to\n> check the contents of the file, and the clean filter is run as the\n> test expects.\n\nHmph, depending on the timestamp granularity, with this patch,\ntest.r would have mtime that is the same or a bit later than that of\nthe index file.  Is it sufficient to really \"force\" Git to check the\ncontents, or does it just make the likelyhood that it would choose\nto check a bit bigger (in other words, are we solving the race, or\nmerely making the race window smaller)?\n\nThanks.\n\n>\n> Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>\n> ---\n>  t/t0021-conversion.sh | 1 +\n>  1 file changed, 1 insertion(+)\n>\n> diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh\n> index e10f5f787f..66f75005d5 100755\n> --- a/t/t0021-conversion.sh\n> +++ b/t/t0021-conversion.sh\n> @@ -390,6 +390,7 @@ test_expect_success PERL 'required process filter should filter data' '\n>  \t\tEOF\n>  \t\ttest_cmp_exclude_clean expected.log debug.log &&\n>  \n> +\t\ttouch test.r &&\n>  \t\tfilter_git checkout --quiet --no-progress empty-branch &&\n>  \t\tcat >expected.log <<-EOF &&\n>  \t\t\tSTART\n"},{"id":"380842","messageId":"aea64308-fcba-77a1-1196-182b35ad405c@kdbg.org","threadId":"51689","inReplyTo":"20190820065625.128130-1-t.gummerer@gmail.com","subject":"Re: [PATCH] t0021: make sure clean filter runs","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2019-08-20T19:11:32Z","receivedAt":"2019-08-20T19:11:36Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 20.08.19 um 08:56 schrieb Thomas Gummerer:\n> Fix the test by updating the mtime of test.r, ...\n\n> diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh\n> index e10f5f787f..66f75005d5 100755\n> --- a/t/t0021-conversion.sh\n> +++ b/t/t0021-conversion.sh\n> @@ -390,6 +390,7 @@ test_expect_success PERL 'required process filter should filter data' '\n>  \t\tEOF\n>  \t\ttest_cmp_exclude_clean expected.log debug.log &&\n>  \n> +\t\ttouch test.r &&\n\n\t\ttest-tool chmtime +10 test.r\n\nwould be more reliable.\n\n>  \t\tfilter_git checkout --quiet --no-progress empty-branch &&\n>  \t\tcat >expected.log <<-EOF &&\n>  \t\t\tSTART\n> \n\n-- Hannes\n"},{"id":"380887","messageId":"20190821145215.GA2679@cat","threadId":"51689","inReplyTo":"xmqqlfvnvhpl.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] t0021: make sure clean filter runs","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2019-08-21T14:52:15Z","receivedAt":"2019-08-21T14:52:20Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"On 08/20, Junio C Hamano wrote:\n> Thomas Gummerer <t.gummerer@gmail.com> writes:\n> \n> > Fix the test by updating the mtime of test.r, so git is forced to\n> > check the contents of the file, and the clean filter is run as the\n> > test expects.\n> \n> Hmph, depending on the timestamp granularity, with this patch,\n> test.r would have mtime that is the same or a bit later than that of\n> the index file.  Is it sufficient to really \"force\" Git to check the\n> contents, or does it just make the likelyhood that it would choose\n> to check a bit bigger (in other words, are we solving the race, or\n> merely making the race window smaller)?\n\nThis test only worked until now because git checks the contents if the\nmtime of the file and the index are the same.  This is because of\nracy-git.  I tried to describe this in the commit message, but looks\nlike it wasn't clear enough.  Do you have any suggestions on how to\nmake it clearer?\n\nIt will also check the contents if the mtime is greater than the\ntimestamp of the index, so the 'touch' here would also cover that.\n\nSo the changes here do solve the race completely.\n\n> Thanks.\n> \n> >\n> > Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>\n> > ---\n> >  t/t0021-conversion.sh | 1 +\n> >  1 file changed, 1 insertion(+)\n> >\n> > diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh\n> > index e10f5f787f..66f75005d5 100755\n> > --- a/t/t0021-conversion.sh\n> > +++ b/t/t0021-conversion.sh\n> > @@ -390,6 +390,7 @@ test_expect_success PERL 'required process filter should filter data' '\n> >  \t\tEOF\n> >  \t\ttest_cmp_exclude_clean expected.log debug.log &&\n> >  \n> > +\t\ttouch test.r &&\n> >  \t\tfilter_git checkout --quiet --no-progress empty-branch &&\n> >  \t\tcat >expected.log <<-EOF &&\n> >  \t\t\tSTART\n"},{"id":"380888","messageId":"20190821145616.GB2679@cat","threadId":"51689","inReplyTo":"aea64308-fcba-77a1-1196-182b35ad405c@kdbg.org","subject":"Re: [PATCH] t0021: make sure clean filter runs","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2019-08-21T14:56:16Z","receivedAt":"2019-08-21T14:56:21Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"On 08/20, Johannes Sixt wrote:\n> Am 20.08.19 um 08:56 schrieb Thomas Gummerer:\n> > Fix the test by updating the mtime of test.r, ...\n> \n> > diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh\n> > index e10f5f787f..66f75005d5 100755\n> > --- a/t/t0021-conversion.sh\n> > +++ b/t/t0021-conversion.sh\n> > @@ -390,6 +390,7 @@ test_expect_success PERL 'required process filter should filter data' '\n> >  \t\tEOF\n> >  \t\ttest_cmp_exclude_clean expected.log debug.log &&\n> >  \n> > +\t\ttouch test.r &&\n> \n> \t\ttest-tool chmtime +10 test.r\n> \n> would be more reliable.\n\nHmm, is touch unreliable on some platforms?  I didn't think of\n'test-tool chmtime', but I'm also not sure it's better than touch in\nthis case.\n\nTo me te 'touch' signifies that the timestamp must be updated after\nthe previous checkout, so git thinks it could possibly have been\nchanged, which I think is clearer in this case than setting the mtime\nto a future time.\n\nBut I'm happy to change it if there's something I'm missing why\n'test-tool chmtime' is better in this case.\n\n> >  \t\tfilter_git checkout --quiet --no-progress empty-branch &&\n> >  \t\tcat >expected.log <<-EOF &&\n> >  \t\t\tSTART\n> > \n> \n> -- Hannes\n"},{"id":"380890","messageId":"xmqq36huttku.fsf@gitster-ct.c.googlers.com","threadId":"51689","inReplyTo":"20190821145215.GA2679@cat","subject":"Re: [PATCH] t0021: make sure clean filter runs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-08-21T15:40:49Z","receivedAt":"2019-08-21T15:40:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Gummerer <t.gummerer@gmail.com> writes:\n\n> It will also check the contents if the mtime is greater than the\n> timestamp of the index, so the 'touch' here would also cover that.\n>\n> So the changes here do solve the race completely.\n\nOK, the explanation makes sense.\n\nEither test.r has been correctly checked out and has an older\ntimestamp or a more recent timestamp. In the former case, the index\nknows that we did not touch it, so the next \"checkout\" knows it does\nnot have to ask the clean filter to work on it.  In the latter case,\nthe index is unsure if we touched it (or, suspects that it has\nupdated contents in it), so the clean filter needs to read from the\nworking tree to see if we did change it (and we find it is not\nmodified).  The outcome at the higher level, the answer to the\nquestion \"checkout\" wanted to ask, is the same: test.r has no local\nmodificaiton and we can switch branches safely.\n\nAnd that is already validated by seeing what exit status \"checkout\"\ngives us, so it sort-of feels to be testing a bit too low level\nimplementation detail to see on which paths the filters are or are\nnot called, but that is not a problem with this fix.  If we want to\ncheck at that level, we should do so correctly, and making sure that\nthe test.r file has recent timestamp to convince \"checkout\" that it\nneeds to verify contents is the right thing to do.\n\nThanks.\n"},{"id":"380898","messageId":"a8de9661-7f6a-f953-93a0-8ef88e9a490a@kdbg.org","threadId":"51689","inReplyTo":"20190821145616.GB2679@cat","subject":"Re: [PATCH] t0021: make sure clean filter runs","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2019-08-21T18:23:23Z","receivedAt":"2019-08-21T18:23:27Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 21.08.19 um 16:56 schrieb Thomas Gummerer:\n> On 08/20, Johannes Sixt wrote:\n>> Am 20.08.19 um 08:56 schrieb Thomas Gummerer:\n>>> Fix the test by updating the mtime of test.r, ...\n>>\n>>> diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh\n>>> index e10f5f787f..66f75005d5 100755\n>>> --- a/t/t0021-conversion.sh\n>>> +++ b/t/t0021-conversion.sh\n>>> @@ -390,6 +390,7 @@ test_expect_success PERL 'required process filter should filter data' '\n>>>  \t\tEOF\n>>>  \t\ttest_cmp_exclude_clean expected.log debug.log &&\n>>>  \n>>> +\t\ttouch test.r &&\n>>\n>> \t\ttest-tool chmtime +10 test.r\n>>\n>> would be more reliable.\n> \n> Hmm, is touch unreliable on some platforms?  I didn't think of\n> 'test-tool chmtime', but I'm also not sure it's better than touch in\n> this case.\n> \n> To me te 'touch' signifies that the timestamp must be updated after\n> the previous checkout, so git thinks it could possibly have been\n> changed, which I think is clearer in this case than setting the mtime\n> to a future time.\n\ntouch does not guarantee that the current time is different from the\ntimestamp that the file already carries, particularly not when the\nfilesystem stores just a resolution of 1 second, and commands are\nexecuted quickly.\n\nBut when we use test-tool chmtime +10, then the timestamp is definitely\ndifferent. If you don't like a timestamp in the future, use -10, or\nanything else that is different from zero.\n\n-- Hannes\n"},{"id":"380910","messageId":"20190821220355.GZ20404@szeder.dev","threadId":"51689","inReplyTo":"a8de9661-7f6a-f953-93a0-8ef88e9a490a@kdbg.org","subject":"Re: [PATCH] t0021: make sure clean filter runs","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-08-21T22:03:55Z","receivedAt":"2019-08-21T22:04:02Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Wed, Aug 21, 2019 at 08:23:23PM +0200, Johannes Sixt wrote:\n> Am 21.08.19 um 16:56 schrieb Thomas Gummerer:\n> > On 08/20, Johannes Sixt wrote:\n> >> Am 20.08.19 um 08:56 schrieb Thomas Gummerer:\n> >>> Fix the test by updating the mtime of test.r, ...\n> >>\n> >>> diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh\n> >>> index e10f5f787f..66f75005d5 100755\n> >>> --- a/t/t0021-conversion.sh\n> >>> +++ b/t/t0021-conversion.sh\n> >>> @@ -390,6 +390,7 @@ test_expect_success PERL 'required process filter should filter data' '\n> >>>  \t\tEOF\n> >>>  \t\ttest_cmp_exclude_clean expected.log debug.log &&\n> >>>  \n> >>> +\t\ttouch test.r &&\n> >>\n> >> \t\ttest-tool chmtime +10 test.r\n> >>\n> >> would be more reliable.\n> > \n> > Hmm, is touch unreliable on some platforms?  I didn't think of\n> > 'test-tool chmtime', but I'm also not sure it's better than touch in\n> > this case.\n> > \n> > To me te 'touch' signifies that the timestamp must be updated after\n> > the previous checkout, so git thinks it could possibly have been\n> > changed, which I think is clearer in this case than setting the mtime\n> > to a future time.\n> \n> touch does not guarantee that the current time is different from the\n> timestamp that the file already carries, particularly not when the\n> filesystem stores just a resolution of 1 second, and commands are\n> executed quickly.\n\nThis 'touch' must ensure that the timestamp of the file is not older\nthan the timestamp of the index, and to achive that it doesn't\nnecessarily have to modify the timestamp.\n\nThe file is modified first, then the index is updated, and finally\ncomes this 'touch'.  Consequently, if 'touch' doesn't modify the\ntimestamp of the file, then it must have the same timestamp as the\nindex, IOW it's racily clean, and the subsequent 'git checkout' has to\nlook at the file content and has to run the filter, and that's what we\nwant to see here.\n\nHowever, I'm not sure what would happens if the system clock were to\njump back in between, but since it's only a test I don't think it's\nworth caring about.\n\n> But when we use test-tool chmtime +10, then the timestamp is definitely\n> different.\n\n'test-tool chmtime +10' adjusts the timestamp of the file relative to\nits current timestamp.  So yeah, the file's timestamp definitely\nchanges, but that's not enough, because it doesn't ensure that the new\ntimestamp is not older than the timestamp of the index.  Just imagine\nthe arguably pathological situation that right after the file was last\nmodified the system miraculously comes to a complete stall, and only\nmanages to resume after 15 seconds to continue with updating the\nindex.  This means that the timestamp of the file will be 15s older\nthan the index, and after that 'chmtime +10' it will still be 5s\nolder.  Consequently, 'git checkout' will think that the file is\nclean, it won't run the filter that we expect, and the test will fail.\nSo instead of '+10' it should be '=+10' to set the new timestamp\nrelative to the current time, but I'm not too keen about the timestamp\nin the future either (though the file is about to be deleted anyway).\n\nI think it would be best to explicitly set the timestamp of the file\nand the index sort-of relative to each other and add an in-code\ncomment as well, e.g.:\n\n  # Make sure that the file appears dirty, so checkout below has to\n  # run the configured filter.\n  test-tool chmtime =-10 .git/index &&\n  test-tool chmtime =+0 test.r &&\n\n"},{"id":"380960","messageId":"20190822174901.GA71239@cat","threadId":"51689","inReplyTo":"20190821220355.GZ20404@szeder.dev","subject":"Re: [PATCH] t0021: make sure clean filter runs","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2019-08-22T17:49:01Z","receivedAt":"2019-08-22T17:49:08Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"On 08/22, SZEDER Gábor wrote:\n> On Wed, Aug 21, 2019 at 08:23:23PM +0200, Johannes Sixt wrote:\n> > Am 21.08.19 um 16:56 schrieb Thomas Gummerer:\n> > > On 08/20, Johannes Sixt wrote:\n> > >> Am 20.08.19 um 08:56 schrieb Thomas Gummerer:\n> > >>> Fix the test by updating the mtime of test.r, ...\n> > >>\n> > >>> diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh\n> > >>> index e10f5f787f..66f75005d5 100755\n> > >>> --- a/t/t0021-conversion.sh\n> > >>> +++ b/t/t0021-conversion.sh\n> > >>> @@ -390,6 +390,7 @@ test_expect_success PERL 'required process filter should filter data' '\n> > >>>  \t\tEOF\n> > >>>  \t\ttest_cmp_exclude_clean expected.log debug.log &&\n> > >>>  \n> > >>> +\t\ttouch test.r &&\n> > >>\n> > >> \t\ttest-tool chmtime +10 test.r\n> > >>\n> > >> would be more reliable.\n> > > \n> > > Hmm, is touch unreliable on some platforms?  I didn't think of\n> > > 'test-tool chmtime', but I'm also not sure it's better than touch in\n> > > this case.\n> > > \n> > > To me te 'touch' signifies that the timestamp must be updated after\n> > > the previous checkout, so git thinks it could possibly have been\n> > > changed, which I think is clearer in this case than setting the mtime\n> > > to a future time.\n> > \n> > touch does not guarantee that the current time is different from the\n> > timestamp that the file already carries, particularly not when the\n> > filesystem stores just a resolution of 1 second, and commands are\n> > executed quickly.\n> \n> This 'touch' must ensure that the timestamp of the file is not older\n> than the timestamp of the index, and to achive that it doesn't\n> necessarily have to modify the timestamp.\n> \n> The file is modified first, then the index is updated, and finally\n> comes this 'touch'.  Consequently, if 'touch' doesn't modify the\n> timestamp of the file, then it must have the same timestamp as the\n> index, IOW it's racily clean, and the subsequent 'git checkout' has to\n> look at the file content and has to run the filter, and that's what we\n> want to see here.\n\nRight.\n\n> However, I'm not sure what would happens if the system clock were to\n> jump back in between, but since it's only a test I don't think it's\n> worth caring about.\n\nYeah, I think much of git wouldn't work correctly if that happens, so\nI think it's fairly safe to ignore in the test suite.\n\n> > But when we use test-tool chmtime +10, then the timestamp is definitely\n> > different.\n> \n> 'test-tool chmtime +10' adjusts the timestamp of the file relative to\n> its current timestamp.  So yeah, the file's timestamp definitely\n> changes, but that's not enough, because it doesn't ensure that the new\n> timestamp is not older than the timestamp of the index.  Just imagine\n> the arguably pathological situation that right after the file was last\n> modified the system miraculously comes to a complete stall, and only\n> manages to resume after 15 seconds to continue with updating the\n> index.  This means that the timestamp of the file will be 15s older\n> than the index, and after that 'chmtime +10' it will still be 5s\n> older.  Consequently, 'git checkout' will think that the file is\n> clean, it won't run the filter that we expect, and the test will fail.\n> So instead of '+10' it should be '=+10' to set the new timestamp\n> relative to the current time, but I'm not too keen about the timestamp\n> in the future either (though the file is about to be deleted anyway).\n\nRight, the above is why I think 'touch' is a good idea here.  Short of\nsystem clocks jumping around, which will most likely break more than\nthis test anyway it guarantees that the timestamp is equal or greater\nthan the timestamp of the index, which is what we need here.\n\n> I think it would be best to explicitly set the timestamp of the file\n> and the index sort-of relative to each other and add an in-code\n> comment as well, e.g.:\n> \n>   # Make sure that the file appears dirty, so checkout below has to\n>   # run the configured filter.\n>   test-tool chmtime =-10 .git/index &&\n>   test-tool chmtime =+0 test.r &&\n\nI think the comment is a good idea.  I personally still prefer just\nusing 'touch' though, as I find it slightly easier to read (I had to\ngo look up what the =-/=+ in 'test-tool chmtime' does, while I knew\nwhat touch would be doing :)\n\nThat said that's a minor preference for me, if people have a strong\nopinion that test-tool chmtime is really better here I'm fine with\nchanging it.\n"},{"id":"380962","messageId":"xmqqpnkxoz4l.fsf@gitster-ct.c.googlers.com","threadId":"51689","inReplyTo":"20190822174901.GA71239@cat","subject":"Re: [PATCH] t0021: make sure clean filter runs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-08-22T18:04:26Z","receivedAt":"2019-08-22T18:04:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Gummerer <t.gummerer@gmail.com> writes:\n\n>>   # Make sure that the file appears dirty, so checkout below has to\n>>   # run the configured filter.\n>>   test-tool chmtime =-10 .git/index &&\n>>   test-tool chmtime =+0 test.r &&\n>\n> I think the comment is a good idea.  I personally still prefer just\n> using 'touch' though, as I find it slightly easier to read (I had to\n> go look up what the =-/=+ in 'test-tool chmtime' does, while I knew\n> what touch would be doing :)\n\nYup, I do not quite get why people feel 'touch' is a bad idea here.\n\nI also think we should discourage \"test-tool chmtime =<anything>\"\n(i.e. set to relative to the timestamp read from time(), as opposed\nto relative to the timestamp read from the filesystem), when we can\navoid it, to allow tests on remote filesystem where the filesystem\nclock and the system clock may not always be in sync.\n\n> That said that's a minor preference for me, if people have a strong\n> opinion that test-tool chmtime is really better here I'm fine with\n> changing it.\n"},{"id":"380968","messageId":"79284459-d338-be91-5d13-8f06890f438f@kdbg.org","threadId":"51689","inReplyTo":"20190822174901.GA71239@cat","subject":"Re: [PATCH] t0021: make sure clean filter runs","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2019-08-22T18:52:53Z","receivedAt":"2019-08-22T18:53:01Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 22.08.19 um 19:49 schrieb Thomas Gummerer:\n> Right, the above is why I think 'touch' is a good idea here.  Short of\n> system clocks jumping around, which will most likely break more than\n> this test anyway it guarantees that the timestamp is equal or greater\n> than the timestamp of the index, which is what we need here.\n\nOk, thanks for the clarification. I didn't see the context. It looks\nlike touch is good enough.\n\n-- Hannes\n"},{"id":"380970","messageId":"20190822192240.GA4077@cat","threadId":"51689","inReplyTo":"20190820065625.128130-1-t.gummerer@gmail.com","subject":"[PATCH v2] t0021: make sure clean filter runs","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2019-08-22T19:22:40Z","receivedAt":"2019-08-22T19:22:47Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"In t0021.15 one of the things we are checking is that the clean filter\nis run when checking out empty-branch.  The clean filter needs to be\nrun to make sure there are no modifications on the file system for the\ntest.r file, and thus it isn't dangerous to overwrite it.\n\nHowever in the current test setup it is not always necessary to run\nthe clean filter, and thus the test sometimes fails, as debug.log\nisn't written.\n\nThis happens when test.r has an older mtime than the index itself.\nThat mtime is also recorded as stat data for test.r in the index, and\nbased on the heuristic we're using for index entries, git correctly\nassumes this file is up-to-date.\n\nUsually this test succeeds because the mtime of test.r is the same as\nthe mtime of the index.  In this case test.r is racily clean, so git\nactually checks the contents, for which the clean filter is run.\n\nFix the test by updating the mtime of test.r, so git is forced to\ncheck the contents of the file, and the clean filter is run as the\ntest expects.\n\nSigned-off-by: Thomas Gummerer <t.gummerer@gmail.com>\n---\n\nv2 adds the comment as suggested by Szeder.\n\nJunio: I saw this is marked as \"merged to 'next'\" in the What's\ncooking, so if it got merged already I'm fine with just keeping v1,\nbut otherwise I think adding the comment would be nice.\n\n t/t0021-conversion.sh | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh\nindex e10f5f787f..c954c709ad 100755\n--- a/t/t0021-conversion.sh\n+++ b/t/t0021-conversion.sh\n@@ -390,6 +390,9 @@ test_expect_success PERL 'required process filter should filter data' '\n \t\tEOF\n \t\ttest_cmp_exclude_clean expected.log debug.log &&\n \n+\t\t# Make sure that the file appears dirty, so checkout below has to\n+\t\t# run the configured filter.\n+\t\ttouch test.r &&\n \t\tfilter_git checkout --quiet --no-progress empty-branch &&\n \t\tcat >expected.log <<-EOF &&\n \t\t\tSTART\n-- \n2.23.0.rc2.194.ge5444969c9\n\n"},{"id":"380972","messageId":"xmqqzhk1nf4m.fsf@gitster-ct.c.googlers.com","threadId":"51689","inReplyTo":"20190822192240.GA4077@cat","subject":"Re: [PATCH v2] t0021: make sure clean filter runs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-08-22T20:01:45Z","receivedAt":"2019-08-22T20:01:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Gummerer <t.gummerer@gmail.com> writes:\n\n> Fix the test by updating the mtime of test.r, so git is forced to\n> check the contents of the file, and the clean filter is run as the\n> test expects.\n>\n> Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>\n> ---\n>\n> v2 adds the comment as suggested by Szeder.\n>\n> Junio: I saw this is marked as \"merged to 'next'\" in the What's\n> cooking, so if it got merged already I'm fine with just keeping v1,\n> but otherwise I think adding the comment would be nice.\n\nI think it was marked with \"Will merge to ...\".  Replaced.\n\nThanks.\n"},{"id":"381004","messageId":"20190823083447.GH20404@szeder.dev","threadId":"51689","inReplyTo":"20190822192240.GA4077@cat","subject":"Re: [PATCH v2] t0021: make sure clean filter runs","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-08-23T08:34:47Z","receivedAt":"2019-08-23T08:34:53Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Thu, Aug 22, 2019 at 08:22:40PM +0100, Thomas Gummerer wrote:\n> v2 adds the comment as suggested by Szeder.\n\n> +\t\t# Make sure that the file appears dirty, so checkout below has to\n> +\t\t# run the configured filter.\n\nYeah, but that comment only really applied when setting both\ntimestamps.  With a simple 'touch' it's more like\n\n  # Make sure that the file appears dirty or is at least racily clean,\n  # so ...\n\nSo the next reader will know that right away, that the author didn't\noverlook racyness issue.\n\n> +\t\ttouch test.r &&\n>  \t\tfilter_git checkout --quiet --no-progress empty-branch &&\n>  \t\tcat >expected.log <<-EOF &&\n>  \t\t\tSTART\n> -- \n> 2.23.0.rc2.194.ge5444969c9\n> \n"}]}