From: René Scharfe <l.s.r@web.de>
Date: Tue, 03 Jan 2017 17:24:39 GMT
Subject: Re: [PATCH] archive-zip: load userdiff config
Message-ID: <7710c564-6b53-1908-7205-210d80eda59b@web.de>
In-Reply-To: <20170102222509.ho7motscnffrtnfh@sigill.intra.peff.net>

Am 02.01.2017 um 23:25 schrieb Jeff King:
> Since 4aff646d17 (archive-zip: mark text files in archives,
> 2015-03-05), the zip archiver will look at the userdiff
> driver to decide whether a file is text or binary. This
> usually doesn't need to look any further than the attributes
> themselves (e.g., "-diff", etc). But if the user defines a
> custom driver like "diff=foo", we need to look at
> "diff.foo.binary" in the config. Prior to this patch, we
> didn't actually load it.

Ah, didn't think of that, obviously.

Would it make sense for userdiff_find_by_path() to die if 
userdiff_config() hasn't been called, yet?

> I also happened to notice that zipfiles are created using the local
> timezone (because they have no notion of the timezone, so we have to
> pick _something_). That's probably the least-terrible option, but it was
> certainly surprising to me when I tried to bit-for-bit reproduce a
> zipfile from GitHub on my local machine.

That reminds me of an old request to allow users better control over the 
meta-data written into archives.  Being able to specify a time zone 
offset could be a start.

>  archive-zip.c          |  7 +++++++
>  t/t5003-archive-zip.sh | 22 ++++++++++++++++++----
>  2 files changed, 25 insertions(+), 4 deletions(-)
>
> diff --git a/archive-zip.c b/archive-zip.c
> index 9db47357b0..b429a8d974 100644
> --- a/archive-zip.c
> +++ b/archive-zip.c
> @@ -554,11 +554,18 @@ static void dos_time(time_t *time, int *dos_date, int *dos_time)
>  	*dos_time = t->tm_sec / 2 + t->tm_min * 32 + t->tm_hour * 2048;
>  }
>
> +static int archive_zip_config(const char *var, const char *value, void *data)
> +{
> +	return userdiff_config(var, value);
> +}
> +
>  static int write_zip_archive(const struct archiver *ar,
>  			     struct archiver_args *args)
>  {
>  	int err;
>
> +	git_config(archive_zip_config, NULL);
> +

I briefly thought about moving this call to archive.c with the rest of 
the config-related stuff, but I agree it's better kept here.

>  	dos_time(&args->time, &zip_date, &zip_time);
>
>  	zip_dir = xmalloc(ZIP_DIRECTORY_MIN_SIZE);
> diff --git a/t/t5003-archive-zip.sh b/t/t5003-archive-zip.sh
> index 14744b2a4b..55c7870997 100755
> --- a/t/t5003-archive-zip.sh
> +++ b/t/t5003-archive-zip.sh
> @@ -64,6 +64,12 @@ check_zip() {
>  		test_cmp_bin $original/nodiff.crlf $extracted/nodiff.crlf &&
>  		test_cmp_bin $original/nodiff.lf   $extracted/nodiff.lf
>  	"
> +
> +	test_expect_success UNZIP " validate that custom diff is unchanged " "
> +		test_cmp_bin $original/custom.cr   $extracted/custom.cr &&
> +		test_cmp_bin $original/custom.crlf $extracted/custom.crlf &&
> +		test_cmp_bin $original/custom.lf   $extracted/custom.lf
> +	"
>  }
>
>  test_expect_success \
> @@ -78,6 +84,9 @@ test_expect_success \
>       printf "text\r"	>a/nodiff.cr &&
>       printf "text\r\n"	>a/nodiff.crlf &&
>       printf "text\n"	>a/nodiff.lf &&
> +     printf "text\r"	>a/custom.cr &&
> +     printf "text\r\n"	>a/custom.crlf &&
> +     printf "text\n"	>a/custom.lf &&
>       printf "\0\r"	>a/binary.cr &&
>       printf "\0\r\n"	>a/binary.crlf &&
>       printf "\0\n"	>a/binary.lf &&
> @@ -112,15 +121,20 @@ test_expect_success 'add files to repository' '
>  test_expect_success 'setup export-subst and diff attributes' '
>  	echo "a/nodiff.* -diff" >>.git/info/attributes &&
>  	echo "a/diff.* diff" >>.git/info/attributes &&
> +	echo "a/custom.* diff=custom" >>.git/info/attributes &&
> +	git config diff.custom.binary true &&
>  	echo "substfile?" export-subst >>.git/info/attributes &&
>  	git log --max-count=1 "--pretty=format:A${SUBSTFORMAT}O" HEAD \
>  		>a/substfile1
>  '
>
> -test_expect_success \
> -    'create bare clone' \
> -    'git clone --bare . bare.git &&
> -     cp .git/info/attributes bare.git/info/attributes'
> +test_expect_success 'create bare clone' '
> +	git clone --bare . bare.git &&
> +	cp .git/info/attributes bare.git/info/attributes &&
> +	# Recreate our changes to .git/config rather than just copying it, as
> +	# we do not want to clobber core.bare or other settings.
> +	git -C bare.git config diff.custom.binary true
> +'
>
>  test_expect_success \
>      'remove ignored file' \
>

Looks good, thanks!

René