{"thread":{"id":"52866","subject":"[GSoC][PATCH 1/1] t1050: clean up checks for file existence","startedAt":"2020-02-22T07:20:54Z","lastAt":"2020-02-24T06:03:52Z","messageCount":7,"participants":["Rasmus Jonsson","Junio C Hamano","brian m. carlson","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"392295","messageId":"20200222071335.27292-2-wasmus@zom.bi","threadId":"52866","inReplyTo":"20200222071335.27292-1-wasmus@zom.bi","subject":"[GSoC][PATCH 1/1] t1050: clean up checks for file existence","fromName":"Rasmus Jonsson","fromEmail":"wasmus@zom.bi","sentAt":"2020-02-22T07:13:35Z","receivedAt":"2020-02-22T07:20:54Z","isPatch":true,"sender":{"key":"wasmus@zom.bi","avatar":null},"body":"Replace \"test -f\" with test_path_is_file, which gives more verbose\nand accurate output.\n\nSigned-off-by: Rasmus Jonsson <wasmus@zom.bi>\n---\n t/t1050-large.sh | 10 ++++++----\n 1 file changed, 6 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t1050-large.sh b/t/t1050-large.sh\nindex d3b2adb28b..667fc2a745 100755\n--- a/t/t1050-large.sh\n+++ b/t/t1050-large.sh\n@@ -53,7 +53,8 @@ test_expect_success 'add a large file or two' '\n \tfor p in .git/objects/pack/pack-*.pack\n \tdo\n \t\tcount=$(( $count + 1 ))\n-\t\tif test -f \"$p\" && idx=${p%.pack}.idx && test -f \"$idx\"\n+\t\tif test_path_is_file \"$p\" && idx=${p%.pack}.idx &&\n+\t\t   test_path_is_file \"$idx\"\n \t\tthen\n \t\t\tcontinue\n \t\tfi\n@@ -65,7 +66,7 @@ test_expect_success 'add a large file or two' '\n \ttest $cnt = 2 &&\n \tfor l in .git/objects/??/??????????????????????????????????????\n \tdo\n-\t\ttest -f \"$l\" || continue\n+\t\ttest_path_is_file \"$l\" || continue\n \t\tbad=t\n \tdone &&\n \ttest -z \"$bad\" &&\n@@ -76,7 +77,8 @@ test_expect_success 'add a large file or two' '\n \tfor p in .git/objects/pack/pack-*.pack\n \tdo\n \t\tcount=$(( $count + 1 ))\n-\t\tif test -f \"$p\" && idx=${p%.pack}.idx && test -f \"$idx\"\n+\t\tif test_path_is_file \"$p\" && idx=${p%.pack}.idx &&\n+\t\t   test_path_is_file \"$idx\"\n \t\tthen\n \t\t\tcontinue\n \t\tfi\n@@ -111,7 +113,7 @@ test_expect_success 'packsize limit' '\n \t\tcount=0 &&\n \t\tfor pi in .git/objects/pack/pack-*.idx\n \t\tdo\n-\t\t\ttest -f \"$pi\" && count=$(( $count + 1 ))\n+\t\t\ttest_path_is_file \"$pi\" && count=$(( $count + 1 ))\n \t\tdone &&\n \t\ttest $count = 2 &&\n \n-- \n2.20.1\n\n"},{"id":"392296","messageId":"20200222071335.27292-1-wasmus@zom.bi","threadId":"52866","inReplyTo":null,"subject":"[GSoC][PATCH 0/1] Introduction & microproject","fromName":"Rasmus Jonsson","fromEmail":"wasmus@zom.bi","sentAt":"2020-02-22T07:13:34Z","receivedAt":"2020-02-22T07:20:54Z","isPatch":true,"sender":{"key":"wasmus@zom.bi","avatar":null},"body":"Good morning, \n\nI intend to apply to git for this year's Google Summer of Code. I am a\nComputer Science student at Gothenburg University working toward a \nmaster's degree. Last year I completed GSoC with LibreOffice [1].\n\nWorking on git appeals to me due to its widespread usage and the number\nof people who will benefit from my contributions.\n\nI read the introductory material, picked a microproject, looked at\nprevious commits and produced the patch below.\n\nIt appears that some of the listed project ideas[2] are \"taken\" or at\nleast being looked at, so I'm also looking at issues on GitGitGadget with\nthe label possible-gsoc-project.\n\nRegards, \nRasmus\n\nRasmus Jonsson (1):\n  t1050: clean up checks for file existence\n\n t/t1050-large.sh | 10 ++++++----\n 1 file changed, 6 insertions(+), 4 deletions(-)\n\n-- \n2.20.1\n\n"},{"id":"392303","messageId":"xmqqtv3i5xhx.fsf@gitster-ct.c.googlers.com","threadId":"52866","inReplyTo":"20200222071335.27292-2-wasmus@zom.bi","subject":"Re: [GSoC][PATCH 1/1] t1050: clean up checks for file existence","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-02-22T17:43:06Z","receivedAt":"2020-02-22T17:43:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rasmus Jonsson <wasmus@zom.bi> writes:\n\n> Replace \"test -f\" with test_path_is_file, which gives more verbose\n> and accurate output.\n>\n> Signed-off-by: Rasmus Jonsson <wasmus@zom.bi>\n> ---\n>  t/t1050-large.sh | 10 ++++++----\n>  1 file changed, 6 insertions(+), 4 deletions(-)\n>\n> diff --git a/t/t1050-large.sh b/t/t1050-large.sh\n> index d3b2adb28b..667fc2a745 100755\n> --- a/t/t1050-large.sh\n> +++ b/t/t1050-large.sh\n> @@ -53,7 +53,8 @@ test_expect_success 'add a large file or two' '\n>  \tfor p in .git/objects/pack/pack-*.pack\n>  \tdo\n>  \t\tcount=$(( $count + 1 ))\n> -\t\tif test -f \"$p\" && idx=${p%.pack}.idx && test -f \"$idx\"\n> +\t\tif test_path_is_file \"$p\" && idx=${p%.pack}.idx &&\n> +\t\t   test_path_is_file \"$idx\"\n\nThis is not wrong per-se, but assignment to idx logically is tied\nstronger to its use (i.e. the first test_path_is_file is about the\n.pack half of the packfile, the assignment to idx *and* the second\ntest_path_is_file is about the corresponding .idx half), so either\nwrite it like so:\n\n        if test_path_is_file \"$p\" &&\n           idx=${p%.pack}.idx && test_path_is_file \"$idx\"\n\nor each piece on its own line, if the result becomes overly long:\n\n        if test_path_is_file \"$p\" &&\n           idx=${p%.pack}.idx &&\n\t   test_path_is_file \"$idx\"\n\nAll the changes in this patch make sense, otherwise.  Thanks.\n\n> @@ -65,7 +66,7 @@ test_expect_success 'add a large file or two' '\n>  \ttest $cnt = 2 &&\n>  \tfor l in .git/objects/??/??????????????????????????????????????\n\nIt is totally an unrelated tangent, but brian, are the lines of this\nkind on your radar?  The object names in SHA-256 world would not be\ncaught with the pattern right?  The fix probably belongs to next to\nwhere OID_REGEX is defined in test-lib.sh (this is a glob and not a\nregex, though).  Perhaps the original should have been written like\n\n\t# somewhere in test-lib.sh\n\tHEXGLOB='[0-9a-f]'\n\tHEXGLOB38=$HEXGLOB$HEXGLOB$HEXGLOB$HEXGLOB$HEXGLOB$HEXGLOB ;# 6\n\tHEXGLOB38=$HEXGLOB38$HEXGLOB38$HEXGLOB38$HEXGLOB38$HEXGLOB38$HEXGLOB38 ;# 36\n\tHEXGLOB38=$HEXGLOB$HEXGLOB$HEXGLOB38\n\n\tOBJFANOUTGLOB=$HEXGLOB$HEXGLOB\n\tOBJFILEGLOB=$HEXGLOB38\n\n\t...\n\n\tfor l in .git/objects/$OBJFANOUTGLOB/$OBJFILEGLOB\n\nand then SHA-256 series would just update OBJFANOUTGLOB and\nOBJFILEGLOB patterns, or something like that?\n\n>  \tdo\n> -\t\ttest -f \"$l\" || continue\n> +\t\ttest_path_is_file \"$l\" || continue\n>  \t\tbad=t\n>  \tdone &&\n>  \ttest -z \"$bad\" &&\n> @@ -76,7 +77,8 @@ test_expect_success 'add a large file or two' '\n>  \tfor p in .git/objects/pack/pack-*.pack\n>  \tdo\n>  \t\tcount=$(( $count + 1 ))\n> -\t\tif test -f \"$p\" && idx=${p%.pack}.idx && test -f \"$idx\"\n> +\t\tif test_path_is_file \"$p\" && idx=${p%.pack}.idx &&\n> +\t\t   test_path_is_file \"$idx\"\n>  \t\tthen\n>  \t\t\tcontinue\n>  \t\tfi\n> @@ -111,7 +113,7 @@ test_expect_success 'packsize limit' '\n>  \t\tcount=0 &&\n>  \t\tfor pi in .git/objects/pack/pack-*.idx\n>  \t\tdo\n> -\t\t\ttest -f \"$pi\" && count=$(( $count + 1 ))\n> +\t\t\ttest_path_is_file \"$pi\" && count=$(( $count + 1 ))\n>  \t\tdone &&\n>  \t\ttest $count = 2 &&\n\n\n"},{"id":"392304","messageId":"xmqqo8tq5x5t.fsf@gitster-ct.c.googlers.com","threadId":"52866","inReplyTo":"20200222071335.27292-2-wasmus@zom.bi","subject":"Re: [GSoC][PATCH 1/1] t1050: clean up checks for file existence","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-02-22T17:50:22Z","receivedAt":"2020-02-22T17:50:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rasmus Jonsson <wasmus@zom.bi> writes:\n\n> Replace \"test -f\" with test_path_is_file, which gives more verbose\n> and accurate output.\n\nDoes it?  \"test -f\" is designed to be silent, while the helper\nfunction is designed to be verbose to help debugging the tests,\nbut the accuracy of the latter simply rests on the former.\n\nAlso\n\n> Subject: Re: [GSoC][PATCH 1/1] t1050: clean up checks for file existence\n\n\"test -f <path>\" is not unclean.  It just is that it is less helpful\nthan using test_path_is_file.\n\n\tSubject: [PATCH] t1050: use test_path_is_file instead of \"test -f\"\n\n\tUse test_path_is_file instead of \"test -f\"; this will help\n\twhen running the test with the \"-v\" option for debugging.\n\n> Signed-off-by: Rasmus Jonsson <wasmus@zom.bi>\n"},{"id":"392306","messageId":"20200222180317.GF6462@camp.crustytoothpaste.net","threadId":"52866","inReplyTo":"xmqqtv3i5xhx.fsf@gitster-ct.c.googlers.com","subject":"Re: [GSoC][PATCH 1/1] t1050: clean up checks for file existence","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2020-02-22T18:03:17Z","receivedAt":"2020-02-22T18:03:28Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2020-02-22 at 17:43:06, Junio C Hamano wrote:\n> > @@ -65,7 +66,7 @@ test_expect_success 'add a large file or two' '\n> >  \ttest $cnt = 2 &&\n> >  \tfor l in .git/objects/??/??????????????????????????????????????\n> \n> It is totally an unrelated tangent, but brian, are the lines of this\n> kind on your radar?  The object names in SHA-256 world would not be\n> caught with the pattern right?  The fix probably belongs to next to\n> where OID_REGEX is defined in test-lib.sh (this is a glob and not a\n> regex, though).  Perhaps the original should have been written like\n> \n> \t# somewhere in test-lib.sh\n> \tHEXGLOB='[0-9a-f]'\n> \tHEXGLOB38=$HEXGLOB$HEXGLOB$HEXGLOB$HEXGLOB$HEXGLOB$HEXGLOB ;# 6\n> \tHEXGLOB38=$HEXGLOB38$HEXGLOB38$HEXGLOB38$HEXGLOB38$HEXGLOB38$HEXGLOB38 ;# 36\n> \tHEXGLOB38=$HEXGLOB$HEXGLOB$HEXGLOB38\n> \n> \tOBJFANOUTGLOB=$HEXGLOB$HEXGLOB\n> \tOBJFILEGLOB=$HEXGLOB38\n> \n> \t...\n> \n> \tfor l in .git/objects/$OBJFANOUTGLOB/$OBJFILEGLOB\n> \n> and then SHA-256 series would just update OBJFANOUTGLOB and\n> OBJFILEGLOB patterns, or something like that?\n\nI actually just saw that particular file the other day, but had not yet\nwritten a patch.  I think we could write a pattern for that which would\nbe useful.\n\nWe do have test_oid_to_path, which takes an object ID and inserts a\nslash after the first two characters, so we could do something like\nthis:\n\n  for l in .git/objects/$(test_oid_to_path $ZERO_OID | sed -e 's/0/[0-9a-f]/g')\n\nThat's not very readable, though.  I'll try to find something a little\nbetter, or hide it somewhere behind a variable in the test setup code\nwith a comment.\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"392341","messageId":"20200223005049.2840-1-wasmus@zom.bi","threadId":"52866","inReplyTo":"20200222071335.27292-1-wasmus@zom.bi","subject":"[GSoC][PATCH v2] t1050: replace test -f with test_path_is_file","fromName":"Rasmus Jonsson","fromEmail":"wasmus@zom.bi","sentAt":"2020-02-23T00:50:49Z","receivedAt":"2020-02-23T00:51:00Z","isPatch":true,"sender":{"key":"wasmus@zom.bi","avatar":null},"body":"Use test_path_is_file() instead of 'test -f' for better debugging\ninformation.\n\nSigned-off-by: Rasmus Jonsson <wasmus@zom.bi>\n---\n\nImproved formatting of the code as suggested and corrected the commit\nmessage.\n\n t/t1050-large.sh | 10 ++++++----\n 1 file changed, 6 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t1050-large.sh b/t/t1050-large.sh\nindex d3b2adb28b..184b479a21 100755\n--- a/t/t1050-large.sh\n+++ b/t/t1050-large.sh\n@@ -53,7 +53,8 @@ test_expect_success 'add a large file or two' '\n \tfor p in .git/objects/pack/pack-*.pack\n \tdo\n \t\tcount=$(( $count + 1 ))\n-\t\tif test -f \"$p\" && idx=${p%.pack}.idx && test -f \"$idx\"\n+\t\tif test_path_is_file \"$p\" &&\n+\t\t   idx=${p%.pack}.idx && test_path_is_file \"$idx\"\n \t\tthen\n \t\t\tcontinue\n \t\tfi\n@@ -65,7 +66,7 @@ test_expect_success 'add a large file or two' '\n \ttest $cnt = 2 &&\n \tfor l in .git/objects/??/??????????????????????????????????????\n \tdo\n-\t\ttest -f \"$l\" || continue\n+\t\ttest_path_is_file \"$l\" || continue\n \t\tbad=t\n \tdone &&\n \ttest -z \"$bad\" &&\n@@ -76,7 +77,8 @@ test_expect_success 'add a large file or two' '\n \tfor p in .git/objects/pack/pack-*.pack\n \tdo\n \t\tcount=$(( $count + 1 ))\n-\t\tif test -f \"$p\" && idx=${p%.pack}.idx && test -f \"$idx\"\n+\t\tif test_path_is_file \"$p\" &&\n+\t\t   idx=${p%.pack}.idx && test_path_is_file \"$idx\"\n \t\tthen\n \t\t\tcontinue\n \t\tfi\n@@ -111,7 +113,7 @@ test_expect_success 'packsize limit' '\n \t\tcount=0 &&\n \t\tfor pi in .git/objects/pack/pack-*.idx\n \t\tdo\n-\t\t\ttest -f \"$pi\" && count=$(( $count + 1 ))\n+\t\t\ttest_path_is_file \"$pi\" && count=$(( $count + 1 ))\n \t\tdone &&\n \t\ttest $count = 2 &&\n \n-- \n2.20.1\n\n"},{"id":"392391","messageId":"20200224060350.GA1015967@coredump.intra.peff.net","threadId":"52866","inReplyTo":"20200222071335.27292-2-wasmus@zom.bi","subject":"Re: [GSoC][PATCH 1/1] t1050: clean up checks for file existence","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-02-24T06:03:50Z","receivedAt":"2020-02-24T06:03:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Feb 22, 2020 at 08:13:35AM +0100, Rasmus Jonsson wrote:\n\n> diff --git a/t/t1050-large.sh b/t/t1050-large.sh\n> index d3b2adb28b..667fc2a745 100755\n> --- a/t/t1050-large.sh\n> +++ b/t/t1050-large.sh\n> @@ -53,7 +53,8 @@ test_expect_success 'add a large file or two' '\n>  \tfor p in .git/objects/pack/pack-*.pack\n>  \tdo\n>  \t\tcount=$(( $count + 1 ))\n> -\t\tif test -f \"$p\" && idx=${p%.pack}.idx && test -f \"$idx\"\n> +\t\tif test_path_is_file \"$p\" && idx=${p%.pack}.idx &&\n> +\t\t   test_path_is_file \"$idx\"\n>  \t\tthen\n>  \t\t\tcontinue\n>  \t\tfi\n\nI was confused at first why these tests use \"continue\", since it seems\nlike these conditions would be errors that could cause a test failure\n(and if they're not, we probably wouldn't want to use test_path_is_file,\nsince it's purpose is to complain noisily).\n\nBut the part that didn't quite make it into the diff context is\nsomething like this:\n\n  for p in ...\n    if test -f ...\n    then\n      continue\n    fi\n    bad=t\n  done &&\n  test -z \"$bad\"\n\nI think this could be written more clearly as:\n\n  for p in ...\n    test -f ... || return 1\n  done\n\nWe explicitly run the test snippets in a shell function to allow this\nkind of early return.\n\nThat's orthogonal to your patch, but it might be worth doing on top, or\nas a preparatory patch.\n\nBut there's one more interesting bit. The loose-object loop from the\nnext hunk does this:\n\n  for l in ...\n    test -f \"$l\" || continue\n    bad=t\n  done &&\n  test -z \"$bad\"\n\nIn other words, it's checking the opposite case: the test fails if the\nfile _does_ exist. And so it seems like using test_path_is_file would be\nthe wrong thing there (it would complain noisily in the success case,\nand not at all in the failure case).\n\nI suspect this could be written more clearly by looking at the output of\n`git count-objects`, or perhaps just:\n\n  {\n    # ignore exit code; will fail when the glob matches nothing\n    find objects/??/ -type f >loose-objects\n    test_must_be_empty loose-objects\n  }\n\neither of which would solve the \"match a loose object with any length\"\nproblem that Junio brought up.\n\n-Peff\n"}]}