Re: [PATCH v2 5/5] fetch: implement fetch.fsck.*

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Wed, May 30 2018, Junio C Hamano wrote:

> Earlier I mumbled "this 4-patch series generally looks good but I
> need to re-read the implementation step"; I meant this 5-patch
> series and here is my impression after re-reading the implementation
> step.
>
> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> diff --git a/Documentation/config.txt b/Documentation/config.txt
>> index f97f21c022..f69cd31ad0 100644
>> --- a/Documentation/config.txt
>> +++ b/Documentation/config.txt
>> @@ -1426,6 +1426,16 @@ fetch.fsckObjects::
>> ...
>
> Nicely written.
>
>> diff --git a/fetch-pack.c b/fetch-pack.c
>> index 490c38f833..9e4282788e 100644
>> --- a/fetch-pack.c
>> +++ b/fetch-pack.c
>> @@ -19,6 +19,7 @@
>>  #include "sha1-array.h"
>>  #include "oidset.h"
>>  #include "packfile.h"
>> +#include "fsck.h"
>>
>>  static int transfer_unpack_limit = -1;
>>  static int fetch_unpack_limit = -1;
>> @@ -33,6 +34,7 @@ static int agent_supported;
>>  static int server_supports_filtering;
>>  static struct lock_file shallow_lock;
>>  static const char *alternate_shallow_file;
>> +static struct strbuf fsck_msg_types = STRBUF_INIT;
>
> s/msg_types[]/exclude[]/ or something, as this is not just about "we
> tolerate known and future instances of 0-padded mode in trees", but
> also "we know this and that object is bad so do not complain" as well.

I copied the fsck_msg_types variable as-is from builtin/receive-pack.c
which Johannes added in 5d477a334a ("fsck (receive-pack): allow demoting
errors to warnings", 2015-06-22).

That's not a justification for doing the same here, but does your
critique also extend to that variable name, so I could fix it there
while I'm at it?

> Other than that, the implementation looks good.
>
>> diff --git a/t/t5504-fetch-receive-strict.sh b/t/t5504-fetch-receive-strict.sh
>> index 49d3621a92..b7f5222c2b 100755
>> --- a/t/t5504-fetch-receive-strict.sh
>> +++ b/t/t5504-fetch-receive-strict.sh
>> @@ -135,6 +135,20 @@ test_expect_success 'push with receive.fsck.skipList' '
>>  	git push --porcelain dst bogus
>>  '
>>
>> +test_expect_success 'fetch with fetch.fsck.skipList' '
>> +	commit="$(git hash-object -t commit -w --stdin <bogus-commit)" &&
>> +	refspec=refs/heads/bogus:refs/heads/bogus &&
>> +	git push . $commit:refs/heads/bogus &&
>> +	rm -rf dst &&
>> +	git init dst &&
>> +	git --git-dir=dst/.git config fetch.fsckObjects true &&
>> +	test_must_fail git --git-dir=dst/.git fetch "file://$(pwd)" $refspec &&
>> +	git --git-dir=dst/.git config fetch.fsck.skipList dst/.git/SKIP &&
>> +	echo $commit >dst/.git/SKIP &&
>> +	git --git-dir=dst/.git fetch "file://$(pwd)" $refspec
>> +'
>
> If the test does not change meaning when file://$(pwd) is replaced
> with "." (or ".." or whatever if other tests below moves cwd
> around), I'd think it is preferrable.  Seeing $(pwd) always makes me
> nervous about Windows.

Thanks. Will fix.