{"thread":{"id":"44808","subject":"[PATCH] archive-zip: load userdiff config","startedAt":"2017-01-02T22:25:56Z","lastAt":"2017-01-04T00:40:41Z","messageCount":3,"participants":["Jeff King","René Scharfe"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"308692","messageId":"20170102222509.ho7motscnffrtnfh@sigill.intra.peff.net","threadId":"44808","inReplyTo":null,"subject":"[PATCH] archive-zip: load userdiff config","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-01-02T22:25:09Z","receivedAt":"2017-01-02T22:25:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Since 4aff646d17 (archive-zip: mark text files in archives,\n2015-03-05), the zip archiver will look at the userdiff\ndriver to decide whether a file is text or binary. This\nusually doesn't need to look any further than the attributes\nthemselves (e.g., \"-diff\", etc). But if the user defines a\ncustom driver like \"diff=foo\", we need to look at\n\"diff.foo.binary\" in the config. Prior to this patch, we\ndidn't actually load it.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI'd be surprised if anybody actually triggered this in practice. I don't\nthink any of the custom-driver fields except \"binary\" matter, and using\ndirect attributes is almost always easier than setting up a custom\ndriver. Though you could also do trickery with:\n\n  git -c diff.default.binary=true archive ...\n\nif you wanted to be really clever.\n\nI ran across this while investigating a case where somebody's zipfile\nwas all marked as binary (it turned out not to be related; the issue was\njust that their Git was pre-4aff646d17).\n\nI also happened to notice that zipfiles are created using the local\ntimezone (because they have no notion of the timezone, so we have to\npick _something_). That's probably the least-terrible option, but it was\ncertainly surprising to me when I tried to bit-for-bit reproduce a\nzipfile from GitHub on my local machine.\n\n archive-zip.c          |  7 +++++++\n t/t5003-archive-zip.sh | 22 ++++++++++++++++++----\n 2 files changed, 25 insertions(+), 4 deletions(-)\n\ndiff --git a/archive-zip.c b/archive-zip.c\nindex 9db47357b0..b429a8d974 100644\n--- a/archive-zip.c\n+++ b/archive-zip.c\n@@ -554,11 +554,18 @@ static void dos_time(time_t *time, int *dos_date, int *dos_time)\n \t*dos_time = t->tm_sec / 2 + t->tm_min * 32 + t->tm_hour * 2048;\n }\n \n+static int archive_zip_config(const char *var, const char *value, void *data)\n+{\n+\treturn userdiff_config(var, value);\n+}\n+\n static int write_zip_archive(const struct archiver *ar,\n \t\t\t     struct archiver_args *args)\n {\n \tint err;\n \n+\tgit_config(archive_zip_config, NULL);\n+\n \tdos_time(&args->time, &zip_date, &zip_time);\n \n \tzip_dir = xmalloc(ZIP_DIRECTORY_MIN_SIZE);\ndiff --git a/t/t5003-archive-zip.sh b/t/t5003-archive-zip.sh\nindex 14744b2a4b..55c7870997 100755\n--- a/t/t5003-archive-zip.sh\n+++ b/t/t5003-archive-zip.sh\n@@ -64,6 +64,12 @@ check_zip() {\n \t\ttest_cmp_bin $original/nodiff.crlf $extracted/nodiff.crlf &&\n \t\ttest_cmp_bin $original/nodiff.lf   $extracted/nodiff.lf\n \t\"\n+\n+\ttest_expect_success UNZIP \" validate that custom diff is unchanged \" \"\n+\t\ttest_cmp_bin $original/custom.cr   $extracted/custom.cr &&\n+\t\ttest_cmp_bin $original/custom.crlf $extracted/custom.crlf &&\n+\t\ttest_cmp_bin $original/custom.lf   $extracted/custom.lf\n+\t\"\n }\n \n test_expect_success \\\n@@ -78,6 +84,9 @@ test_expect_success \\\n      printf \"text\\r\"\t>a/nodiff.cr &&\n      printf \"text\\r\\n\"\t>a/nodiff.crlf &&\n      printf \"text\\n\"\t>a/nodiff.lf &&\n+     printf \"text\\r\"\t>a/custom.cr &&\n+     printf \"text\\r\\n\"\t>a/custom.crlf &&\n+     printf \"text\\n\"\t>a/custom.lf &&\n      printf \"\\0\\r\"\t>a/binary.cr &&\n      printf \"\\0\\r\\n\"\t>a/binary.crlf &&\n      printf \"\\0\\n\"\t>a/binary.lf &&\n@@ -112,15 +121,20 @@ test_expect_success 'add files to repository' '\n test_expect_success 'setup export-subst and diff attributes' '\n \techo \"a/nodiff.* -diff\" >>.git/info/attributes &&\n \techo \"a/diff.* diff\" >>.git/info/attributes &&\n+\techo \"a/custom.* diff=custom\" >>.git/info/attributes &&\n+\tgit config diff.custom.binary true &&\n \techo \"substfile?\" export-subst >>.git/info/attributes &&\n \tgit log --max-count=1 \"--pretty=format:A${SUBSTFORMAT}O\" HEAD \\\n \t\t>a/substfile1\n '\n \n-test_expect_success \\\n-    'create bare clone' \\\n-    'git clone --bare . bare.git &&\n-     cp .git/info/attributes bare.git/info/attributes'\n+test_expect_success 'create bare clone' '\n+\tgit clone --bare . bare.git &&\n+\tcp .git/info/attributes bare.git/info/attributes &&\n+\t# Recreate our changes to .git/config rather than just copying it, as\n+\t# we do not want to clobber core.bare or other settings.\n+\tgit -C bare.git config diff.custom.binary true\n+'\n \n test_expect_success \\\n     'remove ignored file' \\\n-- \n2.11.0.519.g31435224cf\n"},{"id":"308705","messageId":"7710c564-6b53-1908-7205-210d80eda59b@web.de","threadId":"44808","inReplyTo":"20170102222509.ho7motscnffrtnfh@sigill.intra.peff.net","subject":"Re: [PATCH] archive-zip: load userdiff config","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-01-03T17:24:39Z","receivedAt":"2017-01-03T17:26:36Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 02.01.2017 um 23:25 schrieb Jeff King:\n> Since 4aff646d17 (archive-zip: mark text files in archives,\n> 2015-03-05), the zip archiver will look at the userdiff\n> driver to decide whether a file is text or binary. This\n> usually doesn't need to look any further than the attributes\n> themselves (e.g., \"-diff\", etc). But if the user defines a\n> custom driver like \"diff=foo\", we need to look at\n> \"diff.foo.binary\" in the config. Prior to this patch, we\n> didn't actually load it.\n\nAh, didn't think of that, obviously.\n\nWould it make sense for userdiff_find_by_path() to die if \nuserdiff_config() hasn't been called, yet?\n\n> I also happened to notice that zipfiles are created using the local\n> timezone (because they have no notion of the timezone, so we have to\n> pick _something_). That's probably the least-terrible option, but it was\n> certainly surprising to me when I tried to bit-for-bit reproduce a\n> zipfile from GitHub on my local machine.\n\nThat reminds me of an old request to allow users better control over the \nmeta-data written into archives.  Being able to specify a time zone \noffset could be a start.\n\n>  archive-zip.c          |  7 +++++++\n>  t/t5003-archive-zip.sh | 22 ++++++++++++++++++----\n>  2 files changed, 25 insertions(+), 4 deletions(-)\n>\n> diff --git a/archive-zip.c b/archive-zip.c\n> index 9db47357b0..b429a8d974 100644\n> --- a/archive-zip.c\n> +++ b/archive-zip.c\n> @@ -554,11 +554,18 @@ static void dos_time(time_t *time, int *dos_date, int *dos_time)\n>  \t*dos_time = t->tm_sec / 2 + t->tm_min * 32 + t->tm_hour * 2048;\n>  }\n>\n> +static int archive_zip_config(const char *var, const char *value, void *data)\n> +{\n> +\treturn userdiff_config(var, value);\n> +}\n> +\n>  static int write_zip_archive(const struct archiver *ar,\n>  \t\t\t     struct archiver_args *args)\n>  {\n>  \tint err;\n>\n> +\tgit_config(archive_zip_config, NULL);\n> +\n\nI briefly thought about moving this call to archive.c with the rest of \nthe config-related stuff, but I agree it's better kept here.\n\n>  \tdos_time(&args->time, &zip_date, &zip_time);\n>\n>  \tzip_dir = xmalloc(ZIP_DIRECTORY_MIN_SIZE);\n> diff --git a/t/t5003-archive-zip.sh b/t/t5003-archive-zip.sh\n> index 14744b2a4b..55c7870997 100755\n> --- a/t/t5003-archive-zip.sh\n> +++ b/t/t5003-archive-zip.sh\n> @@ -64,6 +64,12 @@ check_zip() {\n>  \t\ttest_cmp_bin $original/nodiff.crlf $extracted/nodiff.crlf &&\n>  \t\ttest_cmp_bin $original/nodiff.lf   $extracted/nodiff.lf\n>  \t\"\n> +\n> +\ttest_expect_success UNZIP \" validate that custom diff is unchanged \" \"\n> +\t\ttest_cmp_bin $original/custom.cr   $extracted/custom.cr &&\n> +\t\ttest_cmp_bin $original/custom.crlf $extracted/custom.crlf &&\n> +\t\ttest_cmp_bin $original/custom.lf   $extracted/custom.lf\n> +\t\"\n>  }\n>\n>  test_expect_success \\\n> @@ -78,6 +84,9 @@ test_expect_success \\\n>       printf \"text\\r\"\t>a/nodiff.cr &&\n>       printf \"text\\r\\n\"\t>a/nodiff.crlf &&\n>       printf \"text\\n\"\t>a/nodiff.lf &&\n> +     printf \"text\\r\"\t>a/custom.cr &&\n> +     printf \"text\\r\\n\"\t>a/custom.crlf &&\n> +     printf \"text\\n\"\t>a/custom.lf &&\n>       printf \"\\0\\r\"\t>a/binary.cr &&\n>       printf \"\\0\\r\\n\"\t>a/binary.crlf &&\n>       printf \"\\0\\n\"\t>a/binary.lf &&\n> @@ -112,15 +121,20 @@ test_expect_success 'add files to repository' '\n>  test_expect_success 'setup export-subst and diff attributes' '\n>  \techo \"a/nodiff.* -diff\" >>.git/info/attributes &&\n>  \techo \"a/diff.* diff\" >>.git/info/attributes &&\n> +\techo \"a/custom.* diff=custom\" >>.git/info/attributes &&\n> +\tgit config diff.custom.binary true &&\n>  \techo \"substfile?\" export-subst >>.git/info/attributes &&\n>  \tgit log --max-count=1 \"--pretty=format:A${SUBSTFORMAT}O\" HEAD \\\n>  \t\t>a/substfile1\n>  '\n>\n> -test_expect_success \\\n> -    'create bare clone' \\\n> -    'git clone --bare . bare.git &&\n> -     cp .git/info/attributes bare.git/info/attributes'\n> +test_expect_success 'create bare clone' '\n> +\tgit clone --bare . bare.git &&\n> +\tcp .git/info/attributes bare.git/info/attributes &&\n> +\t# Recreate our changes to .git/config rather than just copying it, as\n> +\t# we do not want to clobber core.bare or other settings.\n> +\tgit -C bare.git config diff.custom.binary true\n> +'\n>\n>  test_expect_success \\\n>      'remove ignored file' \\\n>\n\nLooks good, thanks!\n\nRené\n"},{"id":"308745","messageId":"20170104004031.ccm6skvfe3u4kkbw@sigill.intra.peff.net","threadId":"44808","inReplyTo":"7710c564-6b53-1908-7205-210d80eda59b@web.de","subject":"Re: [PATCH] archive-zip: load userdiff config","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-01-04T00:40:31Z","receivedAt":"2017-01-04T00:40:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 03, 2017 at 06:24:39PM +0100, René Scharfe wrote:\n\n> Am 02.01.2017 um 23:25 schrieb Jeff King:\n> > Since 4aff646d17 (archive-zip: mark text files in archives,\n> > 2015-03-05), the zip archiver will look at the userdiff\n> > driver to decide whether a file is text or binary. This\n> > usually doesn't need to look any further than the attributes\n> > themselves (e.g., \"-diff\", etc). But if the user defines a\n> > custom driver like \"diff=foo\", we need to look at\n> > \"diff.foo.binary\" in the config. Prior to this patch, we\n> > didn't actually load it.\n> \n> Ah, didn't think of that, obviously.\n> \n> Would it make sense for userdiff_find_by_path() to die if userdiff_config()\n> hasn't been called, yet?\n\nYeah, perhaps. That makes it impossible for a program to intentionally\nignore the config. But it looks like even plumbing diff commands load\nuserdiff (which makes sense; they control its behavior through things\nlike ALLOW_TEXTCONV). So it's probably fine to have it everywhere.\n\nOther options include:\n\n  1. Just loading it always as part of git_default_config.\n\n  2. Lazy-loading it on the first call. This seems elegant, though it\n     does open up hidden cache-invalidation issues. E.g., somebody asks\n     for userdiff_find_by_path(), we load the values, then they\n     setup_git_repository(), and we would need to reload. That's\n     far-fetched for userdiff, but it makes lazy-loading as a general\n     pattern a bit of a potential maintenance trap.\n\n     We could also introduce some infrastructure to deal with that\n     (e.g., if callers could ask the config machinery \"have you been\n     invalidated\"). That would help here and in other places (e.g., I\n     considered this when dealing with get_shared_repository()).\n\n> > I also happened to notice that zipfiles are created using the local\n> > timezone (because they have no notion of the timezone, so we have to\n> > pick _something_). That's probably the least-terrible option, but it was\n> > certainly surprising to me when I tried to bit-for-bit reproduce a\n> > zipfile from GitHub on my local machine.\n> \n> That reminds me of an old request to allow users better control over the\n> meta-data written into archives.  Being able to specify a time zone offset\n> could be a start.\n\nI did it with:\n\n  TZ=PST8PDT git archive ...\n\nwhich let me get a bit-for-bit match with what GitHub generates. The\nreal problem was just knowing that I needed to do that. OTOH, we're\nconsidering having GitHub generate all archives in UTC for sanity's\nsake, and it would be nice to do that by setting zip.timezone instead of\nhacking $TZ for each invocation.\n\n> > +static int archive_zip_config(const char *var, const char *value, void *data)\n> > +{\n> > +\treturn userdiff_config(var, value);\n> > +}\n> > +\n> >  static int write_zip_archive(const struct archiver *ar,\n> >  \t\t\t     struct archiver_args *args)\n> >  {\n> >  \tint err;\n> > \n> > +\tgit_config(archive_zip_config, NULL);\n> > +\n> \n> I briefly thought about moving this call to archive.c with the rest of the\n> config-related stuff, but I agree it's better kept here.\n\nThat was my first thought, but there are already two config calls:\nwrite_archive() loads default config, but then archive-tar loads\ntar-specific config. Since only zip cares about userdiff, I patterned it\nafter the latter. But arguably everybody _could_ end up calling into\nuserdiff. If we take that philosophy, though, I'd be more inclined to\npush it into git_default_config(). That covers archive writers _and_ any\nother programs which might happen to call into the diff code.\n\n> Looks good, thanks!\n\nThanks for reviewing.\n\n-Peff\n"}]}