BUG: No way to set fsck.<msg-id> when cloning

BUG: No way to set fsck.<msg-id> when cloning

[Ævar Arnfjörð Bjarmason]

When I do:

    git -c fetch.fsckObjects=true clone git@github.com:robbyrussell/oh-my-zsh.git

I get:

    error: object 2b7227859263b6aabcc28355b0b994995b7148b6: zeroPaddedFilemode: contains zero-padded file modes
    fatal: Error in object
    fatal: index-pack failed

The docs (https://git-scm.com/docs/git-config#git-config-fsckltmsg-idgt)
say you can override this with -c fsck.zeroPaddedFilemode = ignore, but
I see in builtin/fsck.c that just fsck_config() knows about this, and
indeed this works *after* you clone the repo when you use 'git fsck'.

I don't have time to fix this now, but what's the best approach here?
Make all the relevant commands copy what fsck_config() is doing, or
should fsck_object() be lazily looking up this config by itself?

Re: BUG: No way to set fsck.<msg-id> when cloning

[Kevin Daudt]

On Thu, May 24, 2018 at 05:25:29PM +0200, Ævar Arnfjörð Bjarmason wrote:
> When I do:
> 
>     git -c fetch.fsckObjects=true clone git@github.com:robbyrussell/oh-my-zsh.git
> 
> I get:
> 
>     error: object 2b7227859263b6aabcc28355b0b994995b7148b6: zeroPaddedFilemode: contains zero-padded file modes
>     fatal: Error in object
>     fatal: index-pack failed
> 
> The docs (https://git-scm.com/docs/git-config#git-config-fsckltmsg-idgt)
> say you can override this with -c fsck.zeroPaddedFilemode = ignore, but
> I see in builtin/fsck.c that just fsck_config() knows about this, and
> indeed this works *after* you clone the repo when you use 'git fsck'.
> 
> I don't have time to fix this now, but what's the best approach here?
> Make all the relevant commands copy what fsck_config() is doing, or
> should fsck_object() be lazily looking up this config by itself?

Apparently someone reported this earlier[0]. Johannes replied:

>  Well, you can apparently have your cake and eat it too (see
> https://git-scm.com/docs/git-config#git-config-receivefsckltmsg-idgt):
> 
> receive.fsck.<msg-id>::
>         When `receive.fsckObjects` is set to true, errors can be switched
>         to warnings and vice versa by configuring the `receive.fsck.<msg-id>`
>         setting where the `<msg-id>` is the fsck message ID and the value
>         is one of `error`, `warn` or `ignore`. For convenience, fsck prefixes
>         the error/warning with the message ID, e.g. "missingEmail: invalid
>         author/committer line - missing email" means that setting
>         `receive.fsck.missingEmail = ignore` will hide that issue.
> 
> In your case, use receive.fsck.zeroPaddedFilemode=ignore=warn (or
> =ignore).

[0]https://public-inbox.org/git/alpine.DEB.2.21.1.1801042125430.32@MININT-6BKU6QN.europe.corp.microsoft.com/

Hope this helps, Kevin.

Re: BUG: No way to set fsck.<msg-id> when cloning

[Ævar Arnfjörð Bjarmason]

On Thu, May 24 2018, Kevin Daudt wrote:

> On Thu, May 24, 2018 at 05:25:29PM +0200, Ævar Arnfjörð Bjarmason wrote:
>> When I do:
>>
>>     git -c fetch.fsckObjects=true clone git@github.com:robbyrussell/oh-my-zsh.git
>>
>> I get:
>>
>>     error: object 2b7227859263b6aabcc28355b0b994995b7148b6: zeroPaddedFilemode: contains zero-padded file modes
>>     fatal: Error in object
>>     fatal: index-pack failed
>>
>> The docs (https://git-scm.com/docs/git-config#git-config-fsckltmsg-idgt)
>> say you can override this with -c fsck.zeroPaddedFilemode = ignore, but
>> I see in builtin/fsck.c that just fsck_config() knows about this, and
>> indeed this works *after* you clone the repo when you use 'git fsck'.
>>
>> I don't have time to fix this now, but what's the best approach here?
>> Make all the relevant commands copy what fsck_config() is doing, or
>> should fsck_object() be lazily looking up this config by itself?
>
> Apparently someone reported this earlier[0]. Johannes replied:
>
>>  Well, you can apparently have your cake and eat it too (see
>> https://git-scm.com/docs/git-config#git-config-receivefsckltmsg-idgt):
>>
>> receive.fsck.<msg-id>::
>>         When `receive.fsckObjects` is set to true, errors can be switched
>>         to warnings and vice versa by configuring the `receive.fsck.<msg-id>`
>>         setting where the `<msg-id>` is the fsck message ID and the value
>>         is one of `error`, `warn` or `ignore`. For convenience, fsck prefixes
>>         the error/warning with the message ID, e.g. "missingEmail: invalid
>>         author/committer line - missing email" means that setting
>>         `receive.fsck.missingEmail = ignore` will hide that issue.
>>
>> In your case, use receive.fsck.zeroPaddedFilemode=ignore=warn (or
>> =ignore).
>
> [0]https://public-inbox.org/git/alpine.DEB.2.21.1.1801042125430.32@MININT-6BKU6QN.europe.corp.microsoft.com/
>
> Hope this helps, Kevin.

That doesn't work, because that's for the server-side, but I need the
fetch.fsck.* that doesn't exist. This works (I'll send a better patch
with tests / docs etc. soon):

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;

 /* Remember to update object flag allocation in object.h */
 #define COMPLETE	(1U << 0)
@@ -935,7 +937,8 @@ static int get_pack(struct fetch_pack_args *args,
 			 */
 			argv_array_push(&cmd.args, "--fsck-objects");
 		else
-			argv_array_push(&cmd.args, "--strict");
+			argv_array_pushf(&cmd.args, "--strict%s",
+					 fsck_msg_types.buf);
 	}

 	cmd.in = demux.out;
@@ -1409,6 +1412,31 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
 	return ref;
 }

+static int fetch_pack_config_cb(const char *var, const char *value, void *cb)
+{
+	if (strcmp(var, "fetch.fsck.skiplist") == 0) {
+		const char *path;
+
+		if (git_config_pathname(&path, var, value))
+			return 1;
+		strbuf_addf(&fsck_msg_types, "%cskiplist=%s",
+			fsck_msg_types.len ? ',' : '=', path);
+		free((char *)path);
+		return 0;
+	}
+
+	if (skip_prefix(var, "fetch.fsck.", &var)) {
+		if (is_valid_msg_type(var, value))
+			strbuf_addf(&fsck_msg_types, "%c%s=%s",
+				fsck_msg_types.len ? ',' : '=', var, value);
+		else
+			warning("Skipping unknown msg id '%s'", var);
+		return 0;
+	}
+
+	return git_default_config(var, value, cb);
+}
+
 static void fetch_pack_config(void)
 {
 	git_config_get_int("fetch.unpacklimit", &fetch_unpack_limit);
@@ -1417,7 +1445,7 @@ static void fetch_pack_config(void)
 	git_config_get_bool("fetch.fsckobjects", &fetch_fsck_objects);
 	git_config_get_bool("transfer.fsckobjects", &transfer_fsck_objects);

-	git_config(git_default_config, NULL);
+	git_config(fetch_pack_config_cb, NULL);
 }

 static void fetch_pack_setup(void)

Re: BUG: No way to set fsck.<msg-id> when cloning

[Jeff King]

On Thu, May 24, 2018 at 07:04:04PM +0200, Ævar Arnfjörð Bjarmason wrote:

> That doesn't work, because that's for the server-side, but I need the
> fetch.fsck.* that doesn't exist. This works (I'll send a better patch
> with tests / docs etc. soon):

Yeah, I think this is the right direction.

> +static int fetch_pack_config_cb(const char *var, const char *value, void *cb)
> +{
> +	if (strcmp(var, "fetch.fsck.skiplist") == 0) {
> +		const char *path;
> +
> +		if (git_config_pathname(&path, var, value))
> +			return 1;
> +		strbuf_addf(&fsck_msg_types, "%cskiplist=%s",
> +			fsck_msg_types.len ? ',' : '=', path);
> +		free((char *)path);
> +		return 0;
> +	}
> +
> +	if (skip_prefix(var, "fetch.fsck.", &var)) {
> +		if (is_valid_msg_type(var, value))
> +			strbuf_addf(&fsck_msg_types, "%c%s=%s",
> +				fsck_msg_types.len ? ',' : '=', var, value);
> +		else
> +			warning("Skipping unknown msg id '%s'", var);
> +		return 0;
> +	}

This matches what's in receive-pack, though the way we stuff all of the
options into a flat string is kind of nasty. I also wonder if we'd
eventually run up against command-line limits if somebody had a
complicated fsck config.

I wonder if we should simply be telling index-pack "please read fsck
config from the prefix 'fetch.fsck'" instead.

I dunno, maybe I am just creating work. Certainly I don't think it
should be a blocker for adding fetch.fsck support. But if you want to
think about it while you are here or write a patch, I wouldn't complain. :)

-Peff

[PATCH 0/4] fsck: doc fixes & fetch.fsck.* implementation

[Ævar Arnfjörð Bjarmason]

On Thu, May 24, 2018 at 9:02 PM, Jeff King <peff@peff.net> wrote:
> On Thu, May 24, 2018 at 07:04:04PM +0200, Ævar Arnfjörð Bjarmason wrote:
>
>> That doesn't work, because that's for the server-side, but I need the
>> fetch.fsck.* that doesn't exist. This works (I'll send a better patch
>> with tests / docs etc. soon):
>
> Yeah, I think this is the right direction.
>
>> +static int fetch_pack_config_cb(const char *var, const char *value, void *cb)
>> +{
>> +     if (strcmp(var, "fetch.fsck.skiplist") == 0) {
>> +             const char *path;
>> +
>> +             if (git_config_pathname(&path, var, value))
>> +                     return 1;
>> +             strbuf_addf(&fsck_msg_types, "%cskiplist=%s",
>> +                     fsck_msg_types.len ? ',' : '=', path);
>> +             free((char *)path);
>> +             return 0;
>> +     }
>> +
>> +     if (skip_prefix(var, "fetch.fsck.", &var)) {
>> +             if (is_valid_msg_type(var, value))
>> +                     strbuf_addf(&fsck_msg_types, "%c%s=%s",
>> +                             fsck_msg_types.len ? ',' : '=', var, value);
>> +             else
>> +                     warning("Skipping unknown msg id '%s'", var);
>> +             return 0;
>> +     }
>
> This matches what's in receive-pack, though the way we stuff all of the
> options into a flat string is kind of nasty. I also wonder if we'd
> eventually run up against command-line limits if somebody had a
> complicated fsck config.

Yeah, but I'm leaving this for the future. I doubt that it's going to
happen in practice, although if you have a huge skip-list...

> I wonder if we should simply be telling index-pack "please read fsck
> config from the prefix 'fetch.fsck'" instead.

I think this whole notion of reading the same config from two places
sucks, and now with my patches it's three. But I can't think of a
reasonable way to make it better without even more complexity, and
maybe it's better to split it up anyway, i.e. the stuff you want to
accept is different that fsck and fetch.

> I dunno, maybe I am just creating work. Certainly I don't think it
> should be a blocker for adding fetch.fsck support. But if you want to
> think about it while you are here or write a patch, I wouldn't complain. :)

Sorry, too late. I already wrote all of this :)

Ævar Arnfjörð Bjarmason (4):
  config doc: don't describe *.fetchObjects twice
  config doc: unify the description of fsck.* and receive.fsck.*
  config doc: elaborate on what transfer.fsckObjects does
  fetch: implement fetch.fsck.*

 Documentation/config.txt        | 113 ++++++++++++++++++++------------
 fetch-pack.c                    |  32 ++++++++-
 t/t5504-fetch-receive-strict.sh |  46 +++++++++++++
 3 files changed, 148 insertions(+), 43 deletions(-)

-- 
2.17.0.290.gded63e768a

[PATCH v2 0/5] fsck: doc fixes & fetch.fsck.* implementation

[Ævar Arnfjörð Bjarmason]

This should address all the comments to v1. Inter-diff:
    
    1: a9cd795db5 ! 1: 3d61e44cb8 config doc: don't describe *.fetchObjects twice
        @@ -1,10 +1,6 @@
         Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
         
             config doc: don't describe *.fetchObjects twice
        -    
        -    Change the copy/pasted description of the fetch.fsckObjects and
        -    receive.fsckObjects variables to refer to transfer.fsckObjects
        -    instead.
             
             Let's not duplicate the description of what *.fsckObjects does twice.
             instead let's refer to transfer.fsckObjects from both fetch.* and
    2: 637c2d4241 ! 2: 9683fd2ec6 config doc: unify the description of fsck.* and receive.fsck.*
        @@ -64,21 +64,21 @@
         +unless someone is being deliberately malicious.
          
          fsck.skipList::
        --	The path to a sorted list of object names (i.e. one SHA-1 per
        + 	The path to a sorted list of object names (i.e. one SHA-1 per
         -	line) that are known to be broken in a non-fatal way and should
         -	be ignored. This feature is useful when an established project
         -	should be accepted despite early commits containing errors that
         -	can be safely ignored such as invalid committer email addresses.
         -	Note: corrupt objects cannot be skipped with this setting.
        -+	Like `fsck.<msg-id>` this variable has a corresponding
        -+	`receive.fsck.skipList` variant.
        ++	line) that are known to be broken in a non-fatal way and
        ++	should be ignored. This feature is useful when an established
        ++	project should be accepted despite early commits containing
        ++	errors that can be safely ignored such as invalid committer
        ++	email addresses. Note: corrupt objects cannot be skipped with
        ++	this setting.
         ++
        -+The path to a sorted list of object names (i.e. one SHA-1 per line)
        -+that are known to be broken in a non-fatal way and should be
        -+ignored. This feature is useful when an established project should be
        -+accepted despite early commits containing errors that can be safely
        -+ignored such as invalid committer email addresses. Note: corrupt
        -+objects cannot be skipped with this setting.
        ++Like `fsck.<msg-id>` this variable has a corresponding
        ++`receive.fsck.skipList` variant.
          
          gc.aggressiveDepth::
          	The depth parameter used in the delta compression
    3: 55dc555196 < -:  ------- config doc: elaborate on what transfer.fsckObjects does
    -:  ------- > 3: 8e9646a6ce config doc: elaborate on what transfer.fsckObjects does
    -:  ------- > 4: 2b3aafdfde config doc: mention future aspirations for transfer.fsckObjects
    4: 13f4d994c0 ! 5: be32b19696 fetch: implement fetch.fsck.*
        @@ -67,16 +67,16 @@
          When `fsck.<msg-id>` is set, errors can be switched to warnings and
          vice versa by configuring the `fsck.<msg-id>` setting where the
         @@
        - unless someone is being deliberately malicious.
        - 
        - fsck.skipList::
        --	Like `fsck.<msg-id>` this variable has a corresponding
        --	`receive.fsck.skipList` variant.
        -+	Like `fsck.<msg-id>` this variable has corresponding
        -+	`receive.fsck.skipList` and `fetch.fsck.skipList` variants.
        + 	email addresses. Note: corrupt objects cannot be skipped with
        + 	this setting.
          +
        - The path to a sorted list of object names (i.e. one SHA-1 per line)
        - that are known to be broken in a non-fatal way and should be
        +-Like `fsck.<msg-id>` this variable has a corresponding
        +-`receive.fsck.skipList` variant.
        ++Like `fsck.<msg-id>` this variable has corresponding
        ++`receive.fsck.skipList` and `fetch.fsck.skipList` variants.
        + 
        + gc.aggressiveDepth::
        + 	The depth parameter used in the delta compression
         
         diff --git a/fetch-pack.c b/fetch-pack.c
         --- a/fetch-pack.c

The "mention future aspirations for transfer.fsckObjects" patch is
new. I've split up the "we're probably going to quarantine fetches
too" part of this.

Ævar Arnfjörð Bjarmason (5):
  config doc: don't describe *.fetchObjects twice
  config doc: unify the description of fsck.* and receive.fsck.*
  config doc: elaborate on what transfer.fsckObjects does
  config doc: mention future aspirations for transfer.fsckObjects
  fetch: implement fetch.fsck.*

 Documentation/config.txt        | 112 ++++++++++++++++++++------------
 fetch-pack.c                    |  32 ++++++++-
 t/t5504-fetch-receive-strict.sh |  46 +++++++++++++
 3 files changed, 148 insertions(+), 42 deletions(-)

-- 
2.17.0.290.gded63e768a

[PATCH v3 00/10] fsck: doc fixes & fetch.fsck.* implementation

[Ævar Arnfjörð Bjarmason]

It's been a couple of months since I submitted v2 of this. This takes
into account all the feedback in the previous thread
(https://public-inbox.org/git/20180525192811.25680-1-avarab@gmail.com/).

To recap, this series is mainly about implementing
fetch.fsck.<msg-id>, so you can turn on transfer.fsckObjects=true and
still allow "fetch" to fetch a whitelist of repos.

I wrote this because I turned transfer.fsckObjects=true on internally
while the .gitmodules security issue was still under embargo, but then
found that we couldn't clone various repos with historical issues
anymore.

It's now 10 patches instead of 5. I rolled the spiritual successor of
https://public-inbox.org/git/20180529211950.26896-1-avarab@gmail.com/
into this series (as documentation, not tests), and noticed many more
things along the way that were already broken or not tested for.

Three of those come after the "fetch.fsck.*" implementation, not
because they rely on that new feature, but because now fetch.fsck.* is
one of the three modes (along with fsck and receive) that we'd like to
test.

Ævar Arnfjörð Bjarmason (10):
  receive.fsck.<msg-id> tests: remove dead code
  config doc: don't describe *.fetchObjects twice
  config doc: unify the description of fsck.* and receive.fsck.*
  config doc: elaborate on what transfer.fsckObjects does
  config doc: elaborate on fetch.fsckObjects security
  transfer.fsckObjects tests: untangle confusing setup
  fetch: implement fetch.fsck.*
  fsck: test & document {fetch,receive}.fsck.* config fallback
  fsck: add stress tests for fsck.skipList
  fsck: test and document unknown fsck.<msg-id> values

 Documentation/config.txt        | 138 ++++++++++++++++++++++++--------
 fetch-pack.c                    |  32 +++++++-
 t/t5504-fetch-receive-strict.sh | 126 ++++++++++++++++++++++++++++-
 3 files changed, 255 insertions(+), 41 deletions(-)

-- 
2.18.0.345.g5c9ce644c3

Re: [PATCH v3 00/10] fsck: doc fixes & fetch.fsck.* implementation

[SZEDER Gábor]

't5504-fetch-receive-strict.sh', in particular the test 'push with
transfer.fsckobjects' failed on me while building this branch the
other day, caused by 'test_cmp' failing, because 'git push' didn't
print anything to its standard input.

I was unable to reproduce the failure with the usual means (running
the test repeatedly for a long time while the machine is under heavy
load), and given that this test has a history of raciness[1] with the
same symptom, I'm not sure whether this patch series does something
wrong, or perhaps the fixes[2] are incomplete.  Or the Blood Moon.


Sidenote: I found some of the error messages from the expectedly
failing 'git fetch' and 'git push' commands misleading.  Some of them
output "fatal: object of unexpected type", even though the original
and the corrupted objects are both of the same type (blob).


1 - 8bf4becf0c (add "ok=sigpipe" to test_must_fail and use it to fix
    flaky tests, 2015-11-27)
2 - five commits leading up to c4b27511ab (t5504: drop sigpipe=ok from
    push tests, 2016-04-19)

[PATCH v3 01/10] receive.fsck.<msg-id> tests: remove dead code

[Ævar Arnfjörð Bjarmason]

Remove the setting of a receive.fsck.badDate config variable to
"ignore". This was added in efaba7cc77 ("fsck: optionally ignore
specific fsck issues completely", 2015-06-22) but never did anything,
presumably it was part of some work-in-progress code that never made
it into git.git.

None of these tests will emit the "invalid author/committer line - bad
date" warning. The dates on the commit objects we're setting up are
not invalid.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t5504-fetch-receive-strict.sh | 2 --
 1 file changed, 2 deletions(-)

diff --git a/t/t5504-fetch-receive-strict.sh b/t/t5504-fetch-receive-strict.sh
index 49d3621a92..e1f8768094 100755
--- a/t/t5504-fetch-receive-strict.sh
+++ b/t/t5504-fetch-receive-strict.sh
@@ -149,8 +149,6 @@ test_expect_success 'push with receive.fsck.missingEmail=warn' '
 	git --git-dir=dst/.git branch -D bogus &&
 	git --git-dir=dst/.git config --add \
 		receive.fsck.missingEmail ignore &&
-	git --git-dir=dst/.git config --add \
-		receive.fsck.badDate warn &&
 	git push --porcelain dst bogus >act 2>&1 &&
 	! grep "missingEmail" act
 '
-- 
2.18.0.345.g5c9ce644c3

Re: [PATCH v3 01/10] receive.fsck.<msg-id> tests: remove dead code

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> Remove the setting of a receive.fsck.badDate config variable to
> "ignore". This was added in efaba7cc77 ("fsck: optionally ignore
> specific fsck issues completely", 2015-06-22) but never did anything,
> presumably it was part of some work-in-progress code that never made
> it into git.git.
>
> None of these tests will emit the "invalid author/committer line - bad
> date" warning. The dates on the commit objects we're setting up are
> not invalid.

It is a timestamp somewhere mid February of 2009.  Perhaps the code
is playing defensive against the lack of email address on the
deliberately broken author line, i.e.

    author Bugs Bunny 1234567890 +0000
    committer Bugs Bunny <bugs@bun.ni> 1234567890 +0000

in case the parser punted and failed to parse that timestamp
correctly?  IOW, the above _could_ be a commit written by "Bugs
Bunny 1234567890" with missing e-mail and missing timestamp.

So I dunno.  It won't break the test with today's system if we
removed this config, but with an updated parser from the next year,
it may start to break.

>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  t/t5504-fetch-receive-strict.sh | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/t/t5504-fetch-receive-strict.sh b/t/t5504-fetch-receive-strict.sh
> index 49d3621a92..e1f8768094 100755
> --- a/t/t5504-fetch-receive-strict.sh
> +++ b/t/t5504-fetch-receive-strict.sh
> @@ -149,8 +149,6 @@ test_expect_success 'push with receive.fsck.missingEmail=warn' '
>  	git --git-dir=dst/.git branch -D bogus &&
>  	git --git-dir=dst/.git config --add \
>  		receive.fsck.missingEmail ignore &&
> -	git --git-dir=dst/.git config --add \
> -		receive.fsck.badDate warn &&
>  	git push --porcelain dst bogus >act 2>&1 &&
>  	! grep "missingEmail" act
>  '

Re: [PATCH v3 01/10] receive.fsck.<msg-id> tests: remove dead code

[Ævar Arnfjörð Bjarmason]

On Fri, Jul 27 2018, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> Remove the setting of a receive.fsck.badDate config variable to
>> "ignore". This was added in efaba7cc77 ("fsck: optionally ignore
>> specific fsck issues completely", 2015-06-22) but never did anything,
>> presumably it was part of some work-in-progress code that never made
>> it into git.git.
>>
>> None of these tests will emit the "invalid author/committer line - bad
>> date" warning. The dates on the commit objects we're setting up are
>> not invalid.
>
> It is a timestamp somewhere mid February of 2009.  Perhaps the code
> is playing defensive against the lack of email address on the
> deliberately broken author line, i.e.
>
>     author Bugs Bunny 1234567890 +0000
>     committer Bugs Bunny <bugs@bun.ni> 1234567890 +0000

This is covered by the "missingEmail" part of the test, but there's
nothing wrong with the timestamp itself.

I doubt Johannes remembers why he did this almost a decade ago, but it
looks to me like he was working on some test where the date was also
bad, and never finished it. There's no point in having that "badDate"
now.

> in case the parser punted and failed to parse that timestamp
> correctly?  IOW, the above _could_ be a commit written by "Bugs
> Bunny 1234567890" with missing e-mail and missing timestamp.
>
> So I dunno.  It won't break the test with today's system if we
> removed this config, but with an updated parser from the next year,
> it may start to break.

I still think it makes sense to remove this particular thing. Let's add
exhaustive tests for all this fsck.* stuff in another series, but no
point in testing for arbitrary fsck errors that aren't going to be
triggered in unrelated tests.

>>
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> ---
>>  t/t5504-fetch-receive-strict.sh | 2 --
>>  1 file changed, 2 deletions(-)
>>
>> diff --git a/t/t5504-fetch-receive-strict.sh b/t/t5504-fetch-receive-strict.sh
>> index 49d3621a92..e1f8768094 100755
>> --- a/t/t5504-fetch-receive-strict.sh
>> +++ b/t/t5504-fetch-receive-strict.sh
>> @@ -149,8 +149,6 @@ test_expect_success 'push with receive.fsck.missingEmail=warn' '
>>  	git --git-dir=dst/.git branch -D bogus &&
>>  	git --git-dir=dst/.git config --add \
>>  		receive.fsck.missingEmail ignore &&
>> -	git --git-dir=dst/.git config --add \
>> -		receive.fsck.badDate warn &&
>>  	git push --porcelain dst bogus >act 2>&1 &&
>>  	! grep "missingEmail" act
>>  '

Re: [PATCH v3 01/10] receive.fsck.<msg-id> tests: remove dead code

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

>>
>>     author Bugs Bunny 1234567890 +0000
>>     committer Bugs Bunny <bugs@bun.ni> 1234567890 +0000
>
> This is covered by the "missingEmail" part of the test, but there's
> nothing wrong with the timestamp itself.

I think you didn't get what I meant.  

What makes you think "1234567890 +0000" on the first line is a
timestamp in the first place?  The parser could be updated and split
that malformed string (as it lacks <...> thing we expect, which is
what missing Email is about) differently from what you are
expecting, causing the line to be broken in two ways, i.e. missing
email and badly formatted timestamp.

But we know for certain that the line is wrong not to have email in
either parser (the current one, or a possibly updated one).  So it
is defensive to doubt your preconception that there is nothing wrong
with the timestamp, and demote possible timestamp errors to warning,
as that is not what we are interested in here.

> I still think it makes sense to remove this particular thing. Let's add
> exhaustive tests for all this fsck.* stuff in another series, but no
> point in testing for arbitrary fsck errors that aren't going to be
> triggered in unrelated tests.

That is exactly what I am saying with "being defensive".  Your
change starts testing for arbitrary errors that are not relevant.

[PATCH v3 02/10] config doc: don't describe *.fetchObjects twice

[Ævar Arnfjörð Bjarmason]

Refer readers of fetch.fsckObjects and receive.fsckObjects to
transfer.fsckObjects instead of repeating the description at each
location.

I don't think this description of them makes much sense, but for now
I'm just moving the existing documentation around. Making it better
will be done in a later patch.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/config.txt | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 43b2de7b5f..6b99cf8d71 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1463,10 +1463,9 @@ fetch.recurseSubmodules::
 
 fetch.fsckObjects::
 	If it is set to true, git-fetch-pack will check all fetched
-	objects. It will abort in the case of a malformed object or a
-	broken link. The result of an abort are only dangling objects.
-	Defaults to false. If not set, the value of `transfer.fsckObjects`
-	is used instead.
+	objects. See `transfer.fsckObjects` for what's
+	checked. Defaults to false. If not set, the value of
+	`transfer.fsckObjects` is used instead.
 
 fetch.unpackLimit::
 	If the number of objects fetched over the Git native
@@ -2889,10 +2888,9 @@ receive.certNonceSlop::
 
 receive.fsckObjects::
 	If it is set to true, git-receive-pack will check all received
-	objects. It will abort in the case of a malformed object or a
-	broken link. The result of an abort are only dangling objects.
-	Defaults to false. If not set, the value of `transfer.fsckObjects`
-	is used instead.
+	objects. See `transfer.fsckObjects` for what's checked.
+	Defaults to false. If not set, the value of
+	`transfer.fsckObjects` is used instead.
 
 receive.fsck.<msg-id>::
 	When `receive.fsckObjects` is set to true, errors can be switched
@@ -3389,6 +3387,10 @@ transfer.fsckObjects::
 	When `fetch.fsckObjects` or `receive.fsckObjects` are
 	not set, the value of this variable is used instead.
 	Defaults to false.
++
+When set, the fetch or receive will abort in the case of a malformed
+object or a broken link. The result of an abort are only dangling
+objects.
 
 transfer.hideRefs::
 	String(s) `receive-pack` and `upload-pack` use to decide which
-- 
2.18.0.345.g5c9ce644c3

Re: [PATCH v3 02/10] config doc: don't describe *.fetchObjects twice

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> Refer readers of fetch.fsckObjects and receive.fsckObjects to
> transfer.fsckObjects instead of repeating the description at each
> location.
>
> I don't think this description of them makes much sense, but for now
> I'm just moving the existing documentation around. Making it better
> will be done in a later patch.

Good clean-up to make them better in a later step, which makes sense.

[PATCH v3 03/10] config doc: unify the description of fsck.* and receive.fsck.*

[Ævar Arnfjörð Bjarmason]

The documentation for the fsck.<msg-id> and receive.fsck.<msg-id>
variables was mostly duplicated in two places, with fsck.<msg-id>
making no mention of the corresponding receive.fsck.<msg-id>, and the
same for fsck.skipList.

I spent quite a lot of time today wondering why setting the
fsck.<msg-id> variant wasn't working to clone a legacy repository (not
that that would have worked anyway, but a subsequent patch implements
fetch.fsck.<msg-id>).

Rectify this situation by describing the feature in general terms
under the fsck.* documentation, and make the receive.fsck.*
documentation refer to those variables instead.

This documentation was initially added in 2becf00ff7 ("fsck: support
demoting errors to warnings", 2015-06-22) and 4b55b9b479 ("fsck:
document the new receive.fsck.<msg-id> options", 2015-06-22).

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/config.txt | 62 +++++++++++++++++++++++-----------------
 1 file changed, 35 insertions(+), 27 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 6b99cf8d71..8d08250a5b 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1595,15 +1595,30 @@ filter.<driver>.smudge::
 	linkgit:gitattributes[5] for details.
 
 fsck.<msg-id>::
-	Allows overriding the message type (error, warn or ignore) of a
-	specific message ID such as `missingEmail`.
-+
-For convenience, fsck prefixes the error/warning with the message ID,
-e.g.  "missingEmail: invalid author/committer line - missing email" means
-that setting `fsck.missingEmail = ignore` will hide that issue.
-+
-This feature is intended to support working with legacy repositories
-which cannot be repaired without disruptive changes.
+	During fsck git may find issues with legacy data which
+	wouldn't be generated by current versions of git, and which
+	wouldn't be sent over the wire if `transfer.fsckObjects` was
+	set. This feature is intended to support working with legacy
+	repositories containing such data.
++
+Setting `fsck.<msg-id>` will be picked up by linkgit:git-fsck[1], but
+to accept pushes of such data set `receive.fsck.<msg-id>` instead.
++
+The rest of the documentation discusses `fsck.*` for brevity, but the
+same applies for the corresponding `receive.fsck.*` variables.
++
+When `fsck.<msg-id>` is set, errors can be switched to warnings and
+vice versa by configuring the `fsck.<msg-id>` setting where the
+`<msg-id>` is the fsck message ID and the value is one of `error`,
+`warn` or `ignore`. For convenience, fsck prefixes the error/warning
+with the message ID, e.g. "missingEmail: invalid author/committer line
+- missing email" means that setting `fsck.missingEmail = ignore` will
+hide that issue.
++
+In general, it is better to enumerate existing objects with problems
+with `fsck.skipList`, instead of listing the kind of breakages these
+problematic objects share to be ignored, as doing the latter will
+allow new instances of the same breakages go unnoticed.
 
 fsck.skipList::
 	The path to a sorted list of object names (i.e. one SHA-1 per
@@ -1612,6 +1627,9 @@ fsck.skipList::
 	should be accepted despite early commits containing errors that
 	can be safely ignored such as invalid committer email addresses.
 	Note: corrupt objects cannot be skipped with this setting.
++
+Like `fsck.<msg-id>` this variable has a corresponding
+`receive.fsck.skipList` variant.
 
 gc.aggressiveDepth::
 	The depth parameter used in the delta compression
@@ -2893,26 +2911,16 @@ receive.fsckObjects::
 	`transfer.fsckObjects` is used instead.
 
 receive.fsck.<msg-id>::
-	When `receive.fsckObjects` is set to true, errors can be switched
-	to warnings and vice versa by configuring the `receive.fsck.<msg-id>`
-	setting where the `<msg-id>` is the fsck message ID and the value
-	is one of `error`, `warn` or `ignore`. For convenience, fsck prefixes
-	the error/warning with the message ID, e.g. "missingEmail: invalid
-	author/committer line - missing email" means that setting
-	`receive.fsck.missingEmail = ignore` will hide that issue.
-+
-This feature is intended to support working with legacy repositories
-which would not pass pushing when `receive.fsckObjects = true`, allowing
-the host to accept repositories with certain known issues but still catch
-other issues.
+	Acts like `fsck.<msg-id>`, but is used by
+	linkgit:git-receive-pack[1] instead of
+	linkgit:git-fsck[1]. See the `fsck.<msg-id>` documentation for
+	details.
 
 receive.fsck.skipList::
-	The path to a sorted list of object names (i.e. one SHA-1 per
-	line) that are known to be broken in a non-fatal way and should
-	be ignored. This feature is useful when an established project
-	should be accepted despite early commits containing errors that
-	can be safely ignored such as invalid committer email addresses.
-	Note: corrupt objects cannot be skipped with this setting.
+	Acts like `fsck.skipList`, but is used by
+	linkgit:git-receive-pack[1] instead of
+	linkgit:git-fsck[1]. See the `fsck.skipList` documentation for
+	details.
 
 receive.keepAlive::
 	After receiving the pack from the client, `receive-pack` may
-- 
2.18.0.345.g5c9ce644c3

Re: [PATCH v3 03/10] config doc: unify the description of fsck.* and receive.fsck.*

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> The documentation for the fsck.<msg-id> and receive.fsck.<msg-id>
> variables was mostly duplicated in two places, with fsck.<msg-id>
> making no mention of the corresponding receive.fsck.<msg-id>, and the
> same for fsck.skipList.

Overall the result reads a lot easily.   Especially this part:

> +	During fsck git may find issues with legacy data which
> +	wouldn't be generated by current versions of git, and which
> +	wouldn't be sent over the wire if `transfer.fsckObjects` was
> +	set. This feature is intended to support working with legacy
> +	repositories containing such data.

that adds only a few sentences and clarifies the impact of
"legacy"-ness to this issue rather nicely.

I first found it a bit odd that fsck.* and receive.fsck.* are
discussed with an apparent asymmetry but it actually is correct,
because we haven't introduced fetch.fsck.* at this step in the
series.

Thanks.

[PATCH v3 04/10] config doc: elaborate on what transfer.fsckObjects does

[Ævar Arnfjörð Bjarmason]

The existing documentation led the user to believe that all we were
doing were basic reachability sanity checks, but that hasn't been true
for a very long time. Update the description to match reality, and
note the caveat that there's a quarantine for accepting pushes, but
not for fetching.

Also mention that the fsck checks for security issues, which was my
initial motivation for writing this fetch.fsck.* series.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/config.txt | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 8d08250a5b..291b4f3c57 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -3397,8 +3397,17 @@ transfer.fsckObjects::
 	Defaults to false.
 +
 When set, the fetch or receive will abort in the case of a malformed
-object or a broken link. The result of an abort are only dangling
-objects.
+object or a link to a nonexistent object. In addition, various other
+issues are checked for, including legacy issues (see `fsck.<msg-id>`),
+and potential security issues like the existence of a `.GIT` directory
+or a malicious `.gitmodules` file (see the release notes for v2.2.1
+and v2.17.1 for details). Other sanity and security checks may be
+added in future releases.
++
+On the receiving side, failing fsckObjects will make those objects
+unreachable, see "QUARANTINE ENVIRONMENT" in
+linkgit:git-receive-pack[1]. On the fetch side, malformed objects will
+instead be left unreferenced in the repository.
 
 transfer.hideRefs::
 	String(s) `receive-pack` and `upload-pack` use to decide which
-- 
2.18.0.345.g5c9ce644c3

Re: [PATCH v3 04/10] config doc: elaborate on what transfer.fsckObjects does

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> The existing documentation led the user to believe that all we were
> doing were basic reachability sanity checks, but that hasn't been true
> for a very long time. Update the description to match reality, and
> note the caveat that there's a quarantine for accepting pushes, but
> not for fetching.
>
> Also mention that the fsck checks for security issues, which was my
> initial motivation for writing this fetch.fsck.* series.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  Documentation/config.txt | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 8d08250a5b..291b4f3c57 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -3397,8 +3397,17 @@ transfer.fsckObjects::
>  	Defaults to false.
>  +
>  When set, the fetch or receive will abort in the case of a malformed
> ...
> +On the receiving side, failing fsckObjects will make those objects
> +unreachable, see "QUARANTINE ENVIRONMENT" in
> +linkgit:git-receive-pack[1]. On the fetch side, malformed objects will
> +instead be left unreferenced in the repository.

"On the receiving side" would contrast better if the counterpart
were "On the fetching side", no?  

It may be clear to everybody who updates this document and reviews
such updates that "receive" is what happens on the other side when
you "push", but I think it is helpful to new readers if there were a
hint that indicates the linkage nearby (if merely as a reminder).

	When set, the fetch or receive (i.e. the other side that
	accepts your "push") will abort in the case of ...

[PATCH v3 05/10] config doc: elaborate on fetch.fsckObjects security

[Ævar Arnfjörð Bjarmason]

Change the transfer.fsckObjects documentation to explicitly note the
unique security and/or corruption issues fetch.fsckObjects suffers
from, since it doesn't have a quarantine environment.

This was already alluded to in the existing documentation, but let's
spell it out so there's no confusion here, and give a concrete example
of how to work around this limitation.

Let's also prominently note that this is considered to be a limitation
of the current implementation, rather than something that's intended
and by design, since we might change this in the future.

See
https://public-inbox.org/git/20180531060259.GE17344@sigill.intra.peff.net/
for further details.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/config.txt | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 291b4f3c57..7ff453c53b 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -3408,6 +3408,27 @@ On the receiving side, failing fsckObjects will make those objects
 unreachable, see "QUARANTINE ENVIRONMENT" in
 linkgit:git-receive-pack[1]. On the fetch side, malformed objects will
 instead be left unreferenced in the repository.
++
+Due to the non-quarantine nature of the `fetch.fsckObjects`
+implementation it can not be relied upon to leave the object store
+clean like `receive.fsckObjects` can.
++
+As objects are unpacked they're written to the object store, so there
+can be cases where malicious objects get introduced even though the
+"fetch" failed, only to have a subsequent "fetch" succeed because only
+new incoming objects are checked, not those that have already been
+written to the object store. That difference in behavior should not be
+relied upon. In the future, such objects may be quarantined for
+"fetch" as well.
++
+For now, the paranoid need to find some way to emulate the quarantine
+environment if they'd like the same protection as "push". E.g. in the
+case of an internal mirror do the mirroring in two steps, one to fetch
+the untrusted objects, and then do a second "push" (which will use the
+quarantine) to another internal repo, and have internal clients
+consume this pushed-to repository, or embargo internal fetches and
+only allow them once a full "fsck" has run (and no new fetches have
+happened in the meantime).
 
 transfer.hideRefs::
 	String(s) `receive-pack` and `upload-pack` use to decide which
-- 
2.18.0.345.g5c9ce644c3

Re: [PATCH v3 05/10] config doc: elaborate on fetch.fsckObjects security

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> +For now, the paranoid need to find some way to emulate the quarantine
> +environment if they'd like the same protection as "push". E.g. in the

We probably should mention that you can immediately prune, as these
unwanted crufts are unreferenced.  That would probably be a lot easier
workaround for the intended readers of this document than "find some
way to emulate".

Re: [PATCH v3 05/10] config doc: elaborate on fetch.fsckObjects security

[Ævar Arnfjörð Bjarmason]

On Fri, Jul 27 2018, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> +For now, the paranoid need to find some way to emulate the quarantine
>> +environment if they'd like the same protection as "push". E.g. in the
>
> We probably should mention that you can immediately prune, as these
> unwanted crufts are unreferenced.  That would probably be a lot easier
> workaround for the intended readers of this document than "find some
> way to emulate".

I'll mention that as well in v6 that "git prune" will get rid of these
objects.

For what it's worth I was imagining something like a system where you're
mirroring every push to some unpatched-git-host.com repo in-house, by
doing a local "git fetch" when you see new data, and you're paranoid
that someone's trying to introduce something like the .gitmodules
security issue to your local mirror, even if you have
transfer.fsckObjects set.

In a case like that, relying on "git prune" is much more fragile. You'd
need to implement your mirror as some loop that does (pseudocode):

    while ref = poll_new_refs()
        git fetch upstream
        git prune --expire=now

As opposed to:

    while ref = poll_new_refs()
        (git fetch upstream && git prune --expire=now) &

As you might find in some event-based system. I.e. every time you fetch
you need to stop the world and run a full prune, because the potentially
malicious upstream can craft a series of ref updates where one ref
update (which you'll refuse) contains the bad data, but at that point
you have some of those blobs/trees/commits it in your object store, and
then a second ref update references that already existing data and
causes you to update the ref.

It's also much slower and I/O heavy, on an already-pruned linux.git
running 'git prune --expire=now' takes 40 seconds on my machine, as
opposed to:

    while ref = poll_new_refs()
        (git fetch && git push internal-mirror --mirror) &

Which could take as little time as a second for the whole operation, can
safely be run in parallel, and would be protected because the actually
published internal mirror gets its refs via receive-pack, which uses the
quarantine.

[PATCH v3 06/10] transfer.fsckObjects tests: untangle confusing setup

[Ævar Arnfjörð Bjarmason]

The tests for transfer.fsckObjects have grown organically over time to
not make much sense.

Initially when these were added in b10a53583f ("test: fetch/receive
with fsckobjects", 2011-09-04) they were only testing the "corrupt or
missing object" case, but later on in 70a4ae73d8 ("fsck: add a simple
test for receive.fsck.<msg-id>", 2015-06-22) they were expanded to
check for the fsck.<msg-id> feature.

The problem was that we still kept the same corrupt test repo, making
it harder to add new tests that check the entirety of the repository
between operations via "git fsck" to see whether only known issues
that can be ignored with fsck.<msg-id> have occurred.

The tests only did the right thing because such a full "git fsck" was
never done after a certain point, and instead we were only
manipulating specific refs. This makes it harder to add new tests, and
none of the fsck.<msg-id> tests relied on this.

So let's not confuse the two and repair the corrupt repository before
we run the fsck.<msg-id> tests.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t5504-fetch-receive-strict.sh | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/t/t5504-fetch-receive-strict.sh b/t/t5504-fetch-receive-strict.sh
index e1f8768094..57ff78c201 100755
--- a/t/t5504-fetch-receive-strict.sh
+++ b/t/t5504-fetch-receive-strict.sh
@@ -3,13 +3,16 @@
 test_description='fetch/receive strict mode'
 . ./test-lib.sh
 
-test_expect_success setup '
+test_expect_success 'setup and inject "corrupt or missing" object' '
 	echo hello >greetings &&
 	git add greetings &&
 	git commit -m greetings &&
 
 	S=$(git rev-parse :greetings | sed -e "s|^..|&/|") &&
 	X=$(echo bye | git hash-object -w --stdin | sed -e "s|^..|&/|") &&
+	echo $S >S &&
+	echo $X >X &&
+	cp .git/objects/$S .git/objects/$S.back &&
 	mv -f .git/objects/$X .git/objects/$S &&
 
 	test_must_fail git fsck
@@ -115,6 +118,13 @@ test_expect_success 'push with transfer.fsckobjects' '
 	test_cmp exp act
 '
 
+test_expect_success 'repair the "corrupt or missing" object' '
+	mv -f .git/objects/$(cat S) .git/objects/$(cat X) &&
+	mv .git/objects/$(cat S).back .git/objects/$(cat S) &&
+	rm -rf .git/objects/$(cat X) &&
+	git fsck
+'
+
 cat >bogus-commit <<EOF
 tree $EMPTY_TREE
 author Bugs Bunny 1234567890 +0000
-- 
2.18.0.345.g5c9ce644c3

[PATCH v3 07/10] fetch: implement fetch.fsck.*

[Ævar Arnfjörð Bjarmason]

Implement support for fetch.fsck.* corresponding with the existing
receive.fsck.*. This allows for pedantically cloning repositories with
specific issues without turning off fetch.fsckObjects.

One such repository is https://github.com/robbyrussell/oh-my-zsh.git
which before this change will emit this error when cloned with
fetch.fsckObjects:

    error: object 2b7227859263b6aabcc28355b0b994995b7148b6: zeroPaddedFilemode: contains zero-padded file modes
    fatal: Error in object
    fatal: index-pack failed

Now with fetch.fsck.zeroPaddedFilemode=warn we'll warn about that
issue, but the clone will succeed:

    warning: object 2b7227859263b6aabcc28355b0b994995b7148b6: zeroPaddedFilemode: contains zero-padded file modes
    warning: object a18c4d13c2a5fa2d4ecd5346c50e119b999b807d: zeroPaddedFilemode: contains zero-padded file modes
    warning: object 84df066176c8da3fd59b13731a86d90f4f1e5c9d: zeroPaddedFilemode: contains zero-padded file modes

The motivation for this is to be able to turn on fetch.fsckObjects
globally across a fleet of computers but still be able to manually
clone various legacy repositories by either white-listing specific
issues, or better yet whitelist specific objects.

The use of --git-dir=* instead of -C in the tests could be considered
somewhat archaic, but the tests I'm adding here are duplicating the
corresponding receive.* tests with as few changes as possible.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/config.txt        | 20 +++++++++++---
 fetch-pack.c                    | 32 +++++++++++++++++++++--
 t/t5504-fetch-receive-strict.sh | 46 +++++++++++++++++++++++++++++++++
 3 files changed, 92 insertions(+), 6 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 7ff453c53b..8dace49daa 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1467,6 +1467,16 @@ fetch.fsckObjects::
 	checked. Defaults to false. If not set, the value of
 	`transfer.fsckObjects` is used instead.
 
+fetch.fsck.<msg-id>::
+	Acts like `fsck.<msg-id>`, but is used by
+	linkgit:git-fetch-pack[1] instead of linkgit:git-fsck[1]. See
+	the `fsck.<msg-id>` documentation for details.
+
+fetch.fsck.skipList::
+	Acts like `fsck.skipList`, but is used by
+	linkgit:git-fetch-pack[1] instead of linkgit:git-fsck[1]. See
+	the `fsck.skipList` documentation for details.
+
 fetch.unpackLimit::
 	If the number of objects fetched over the Git native
 	transfer is below this
@@ -1602,10 +1612,12 @@ fsck.<msg-id>::
 	repositories containing such data.
 +
 Setting `fsck.<msg-id>` will be picked up by linkgit:git-fsck[1], but
-to accept pushes of such data set `receive.fsck.<msg-id>` instead.
+to accept pushes of such data set `receive.fsck.<msg-id>` instead, or
+to clone or fetch it set `fetch.fsck.<msg-id>`.
 +
 The rest of the documentation discusses `fsck.*` for brevity, but the
-same applies for the corresponding `receive.fsck.*` variables.
+same applies for the corresponding `receive.fsck.*` and
+`fetch.<msg-id>.*`. variables.
 +
 When `fsck.<msg-id>` is set, errors can be switched to warnings and
 vice versa by configuring the `fsck.<msg-id>` setting where the
@@ -1628,8 +1640,8 @@ fsck.skipList::
 	can be safely ignored such as invalid committer email addresses.
 	Note: corrupt objects cannot be skipped with this setting.
 +
-Like `fsck.<msg-id>` this variable has a corresponding
-`receive.fsck.skipList` variant.
+Like `fsck.<msg-id>` this variable has corresponding
+`receive.fsck.skipList` and `fetch.fsck.skipList` variants.
 
 gc.aggressiveDepth::
 	The depth parameter used in the delta compression
diff --git a/fetch-pack.c b/fetch-pack.c
index 7ccb9c0d45..aea2f6cf26 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -21,6 +21,7 @@
 #include "packfile.h"
 #include "object-store.h"
 #include "connected.h"
+#include "fsck.h"
 
 static int transfer_unpack_limit = -1;
 static int fetch_unpack_limit = -1;
@@ -35,6 +36,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;
 
 /* Remember to update object flag allocation in object.h */
 #define COMPLETE	(1U << 0)
@@ -937,7 +939,8 @@ static int get_pack(struct fetch_pack_args *args,
 			 */
 			argv_array_push(&cmd.args, "--fsck-objects");
 		else
-			argv_array_push(&cmd.args, "--strict");
+			argv_array_pushf(&cmd.args, "--strict%s",
+					 fsck_msg_types.buf);
 	}
 
 	cmd.in = demux.out;
@@ -1458,6 +1461,31 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
 	return ref;
 }
 
+static int fetch_pack_config_cb(const char *var, const char *value, void *cb)
+{
+	if (strcmp(var, "fetch.fsck.skiplist") == 0) {
+		const char *path;
+
+		if (git_config_pathname(&path, var, value))
+			return 1;
+		strbuf_addf(&fsck_msg_types, "%cskiplist=%s",
+			fsck_msg_types.len ? ',' : '=', path);
+		free((char *)path);
+		return 0;
+	}
+
+	if (skip_prefix(var, "fetch.fsck.", &var)) {
+		if (is_valid_msg_type(var, value))
+			strbuf_addf(&fsck_msg_types, "%c%s=%s",
+				fsck_msg_types.len ? ',' : '=', var, value);
+		else
+			warning("Skipping unknown msg id '%s'", var);
+		return 0;
+	}
+
+	return git_default_config(var, value, cb);
+}
+
 static void fetch_pack_config(void)
 {
 	git_config_get_int("fetch.unpacklimit", &fetch_unpack_limit);
@@ -1466,7 +1494,7 @@ static void fetch_pack_config(void)
 	git_config_get_bool("fetch.fsckobjects", &fetch_fsck_objects);
 	git_config_get_bool("transfer.fsckobjects", &transfer_fsck_objects);
 
-	git_config(git_default_config, NULL);
+	git_config(fetch_pack_config_cb, NULL);
 }
 
 static void fetch_pack_setup(void)
diff --git a/t/t5504-fetch-receive-strict.sh b/t/t5504-fetch-receive-strict.sh
index 57ff78c201..004bfebe98 100755
--- a/t/t5504-fetch-receive-strict.sh
+++ b/t/t5504-fetch-receive-strict.sh
@@ -145,6 +145,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
+'
+
+
 test_expect_success 'push with receive.fsck.missingEmail=warn' '
 	commit="$(git hash-object -t commit -w --stdin <bogus-commit)" &&
 	git push . $commit:refs/heads/bogus &&
@@ -163,6 +177,27 @@ test_expect_success 'push with receive.fsck.missingEmail=warn' '
 	! grep "missingEmail" act
 '
 
+test_expect_success 'fetch with fetch.fsck.missingEmail=warn' '
+	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.missingEmail warn &&
+	git --git-dir=dst/.git fetch "file://$(pwd)" $refspec >act 2>&1 &&
+	grep "missingEmail" act &&
+	rm -rf dst &&
+	git init dst &&
+	git --git-dir=dst/.git config fetch.fsckobjects true &&
+	git --git-dir=dst/.git config \
+		fetch.fsck.missingEmail ignore &&
+	git --git-dir=dst/.git fetch "file://$(pwd)" $refspec >act 2>&1 &&
+	! grep "missingEmail" act
+'
+
 test_expect_success \
 	'receive.fsck.unterminatedHeader=warn triggers error' '
 	rm -rf dst &&
@@ -174,4 +209,15 @@ test_expect_success \
 	grep "Cannot demote unterminatedheader" act
 '
 
+test_expect_success \
+	'fetch.fsck.unterminatedHeader=warn triggers error' '
+	rm -rf dst &&
+	git init dst &&
+	git --git-dir=dst/.git config fetch.fsckobjects true &&
+	git --git-dir=dst/.git config \
+		fetch.fsck.unterminatedheader warn &&
+	test_must_fail git --git-dir=dst/.git fetch "file://$(pwd)" HEAD &&
+	grep "Cannot demote unterminatedheader" act
+'
+
 test_done
-- 
2.18.0.345.g5c9ce644c3

Re: [PATCH v3 07/10] fetch: implement fetch.fsck.*

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

>  +
>  Setting `fsck.<msg-id>` will be picked up by linkgit:git-fsck[1], but
> -to accept pushes of such data set `receive.fsck.<msg-id>` instead.
> +to accept pushes of such data set `receive.fsck.<msg-id>` instead, or

Inherited from the original, but I find it a lot more readable to
write "to accept pushes of such data, set X" with a comma, given
especially that "data set" is a noun that has little to do with what
this sentence wants to talk about.

I'd suggest doing s/instead/& to loosen the error checking/ or
something, as you are only talking about "to accept" (iow, the
target audience is not those who want to reject more by promoting
"warn" to "error"---you are talking to those who are only interested
in demoting "error" to "warn" or "ignore").

> +to clone or fetch it set `fetch.fsck.<msg-id>`.

It is unclear to me what "it" in the last sentence refers to.  

The best rewrite I came up with is "... to clone or fetch from a
repository with such data" but that is not an exact counterpart to
"to accept pushes of such data", so I am hesitant to suggest it.

"... to accept such data when fetching or cloning" might be a better
counterpart, but repeated "to accept" in this single sentence bothers
me somewhat.  I dunno.

I'll comment on the implementation part of this patch after lunch.
Thanks for working on this.

Re: [PATCH v3 07/10] fetch: implement fetch.fsck.*

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> -			argv_array_push(&cmd.args, "--strict");
> +			argv_array_pushf(&cmd.args, "--strict%s",
> +					 fsck_msg_types.buf);
> ...
> +		if (git_config_pathname(&path, var, value))
> +			return 1;
> +		strbuf_addf(&fsck_msg_types, "%cskiplist=%s",
> +			fsck_msg_types.len ? ',' : '=', path);
> ...
> +		if (is_valid_msg_type(var, value))
> +			strbuf_addf(&fsck_msg_types, "%c%s=%s",
> +				fsck_msg_types.len ? ',' : '=', var, value);
> +		else
> +			warning("Skipping unknown msg id '%s'", var);

This follows quite familiar pattern found in receive_pack_config();
looking good.

> diff --git a/t/t5504-fetch-receive-strict.sh b/t/t5504-fetch-receive-strict.sh
> index 57ff78c201..004bfebe98 100755
> --- a/t/t5504-fetch-receive-strict.sh
> +++ b/t/t5504-fetch-receive-strict.sh
> @@ -145,6 +145,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 &&

I see this used in the previous test for receive.fsck.skipList, but
it is an interesting implementation of "git update-ref" that could
be affected by potential fsck error in push-to-receive-pack transport.
As we are interested in transport into "dst" and we want this creation
of our 'bogus' branch to succeed no matter what, it probably is not
a good idea to use "git push ." like this in the context of this test.

Perhaps leave a 'leftoverbits' comment to force us remember to update
all these uses of local push from the script in the future?

> +	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 &&

We see that by default fetch.fsckObjects errors out when it notices
the bogus commit object.

> +	git --git-dir=dst/.git config fetch.fsck.skipList dst/.git/SKIP &&
> +	echo $commit >dst/.git/SKIP &&

And then we set up a skip to see ...

> +	git --git-dir=dst/.git fetch "file://$(pwd)" $refspec

... if that is ignored.  Looks great.

Would this second attempt succeed _without_ the SKIP list, I wonder,
though?  

After the initial attempt that transferred the object, inspected it
and then aborted before pointing a ref to make the object reachable,
wouldn't it be possible for the quickfetch codepath to say "ah, we
locally have that object, so let's see it is a descendant of the tip
of one of our refs *and* all the objects it points at (recursively)
are all available in this repository", as we do not quarantine on
the fetch side?

Re: [PATCH v3 07/10] fetch: implement fetch.fsck.*

[Duy Nguyen]

On Fri, Jul 27, 2018 at 02:37:17PM +0000, Ævar Arnfjörð Bjarmason wrote:
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 7ff453c53b..8dace49daa 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -1467,6 +1467,16 @@ fetch.fsckObjects::
>  	checked. Defaults to false. If not set, the value of
>  	`transfer.fsckObjects` is used instead.
>  
> +fetch.fsck.<msg-id>::
> +	Acts like `fsck.<msg-id>`, but is used by
> +	linkgit:git-fetch-pack[1] instead of linkgit:git-fsck[1]. See
> +	the `fsck.<msg-id>` documentation for details.

Could you squash this patch in? It would let us auto-complete "git
config fetch.fsck" in bash/tcsh

-- 8< --
diff --git a/help.c b/help.c
index 9c0b5a8de9..32fe466dbf 100644
--- a/help.c
+++ b/help.c
@@ -427,6 +427,7 @@ void list_config_help(int for_human)
 		{ "color.interactive", "<slot>", list_config_color_interactive_slots },
 		{ "color.status", "<slot>", list_config_color_status_slots },
 		{ "fsck", "<msg-id>", list_config_fsck_msg_ids },
+		{ "fetch.fsck", "<msg-id>", list_config_fsck_msg_ids },
 		{ "receive.fsck", "<msg-id>", list_config_fsck_msg_ids },
 		{ NULL, NULL, NULL }
 	};
-- 8< --
--
Duy

Re: [PATCH v3 07/10] fetch: implement fetch.fsck.*

[Ævar Arnfjörð Bjarmason]

On Mon, Jul 30 2018, Duy Nguyen wrote:

> On Fri, Jul 27, 2018 at 02:37:17PM +0000, Ævar Arnfjörð Bjarmason wrote:
>> diff --git a/Documentation/config.txt b/Documentation/config.txt
>> index 7ff453c53b..8dace49daa 100644
>> --- a/Documentation/config.txt
>> +++ b/Documentation/config.txt
>> @@ -1467,6 +1467,16 @@ fetch.fsckObjects::
>>  	checked. Defaults to false. If not set, the value of
>>  	`transfer.fsckObjects` is used instead.
>>  
>> +fetch.fsck.<msg-id>::
>> +	Acts like `fsck.<msg-id>`, but is used by
>> +	linkgit:git-fetch-pack[1] instead of linkgit:git-fsck[1]. See
>> +	the `fsck.<msg-id>` documentation for details.
>
> Could you squash this patch in? It would let us auto-complete "git
> config fetch.fsck" in bash/tcsh
>
> -- 8< --
> diff --git a/help.c b/help.c
> index 9c0b5a8de9..32fe466dbf 100644
> --- a/help.c
> +++ b/help.c
> @@ -427,6 +427,7 @@ void list_config_help(int for_human)
>  		{ "color.interactive", "<slot>", list_config_color_interactive_slots },
>  		{ "color.status", "<slot>", list_config_color_status_slots },
>  		{ "fsck", "<msg-id>", list_config_fsck_msg_ids },
> +		{ "fetch.fsck", "<msg-id>", list_config_fsck_msg_ids },
>  		{ "receive.fsck", "<msg-id>", list_config_fsck_msg_ids },
>  		{ NULL, NULL, NULL }
>  	};
> -- 8< --

Thanks! I missed that. Willdo.

[PATCH v3 08/10] fsck: test & document {fetch,receive}.fsck.* config fallback

[Ævar Arnfjörð Bjarmason]

Test and document that the {fetch,receive}.fsck.* family of variables
doesn't fall back on the corresponding .fsck.* variables.

This was alluded to in the existing documentation by saying that
"receive" looks at receive.fsck.* and "fsck" looks at fsck.* etc., but
it wasn't explicitly stated that there was no fallback, and if you'd
e.g. like to configure the skipList you need to do that for all three.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/config.txt        | 12 ++++++++++++
 t/t5504-fetch-receive-strict.sh | 26 ++++++++++++++++++++++++--
 2 files changed, 36 insertions(+), 2 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 8dace49daa..57c463c6e2 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1619,6 +1619,12 @@ The rest of the documentation discusses `fsck.*` for brevity, but the
 same applies for the corresponding `receive.fsck.*` and
 `fetch.<msg-id>.*`. variables.
 +
+Unlike variables like `color.ui` and `core.editor` the
+`receive.fsck.<msg-id>` and `fetch.fsck.<msg-id>` variables will not
+fall back on the `fsck.<msg-id>` configuration if they aren't set. To
+uniformly configure the same fsck settings in different circumstances
+all three of them they must all set to the same values.
++
 When `fsck.<msg-id>` is set, errors can be switched to warnings and
 vice versa by configuring the `fsck.<msg-id>` setting where the
 `<msg-id>` is the fsck message ID and the value is one of `error`,
@@ -1642,6 +1648,12 @@ fsck.skipList::
 +
 Like `fsck.<msg-id>` this variable has corresponding
 `receive.fsck.skipList` and `fetch.fsck.skipList` variants.
++
+Unlike variables like `color.ui` and `core.editor` the
+`receive.fsck.skipList` and `fetch.fsck.skipList` variables will not
+fall back on the `fsck.skipList` configuration if they aren't set. To
+uniformly configure the same fsck settings in different circumstances
+all three of them they must all set to the same values.
 
 gc.aggressiveDepth::
 	The depth parameter used in the delta compression
diff --git a/t/t5504-fetch-receive-strict.sh b/t/t5504-fetch-receive-strict.sh
index 004bfebe98..771a94b4b6 100755
--- a/t/t5504-fetch-receive-strict.sh
+++ b/t/t5504-fetch-receive-strict.sh
@@ -140,8 +140,13 @@ test_expect_success 'push with receive.fsck.skipList' '
 	git init dst &&
 	git --git-dir=dst/.git config receive.fsckObjects true &&
 	test_must_fail git push --porcelain dst bogus &&
-	git --git-dir=dst/.git config receive.fsck.skipList SKIP &&
 	echo $commit >dst/.git/SKIP &&
+
+	# receive.fsck.* does not fall back on fsck.*
+	git --git-dir=dst/.git config fsck.skipList SKIP &&
+	test_must_fail git push --porcelain dst bogus &&
+
+	git --git-dir=dst/.git config receive.fsck.skipList SKIP &&
 	git push --porcelain dst bogus
 '
 
@@ -153,8 +158,15 @@ test_expect_success 'fetch with fetch.fsck.skipList' '
 	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 &&
+	git --git-dir=dst/.git config fetch.fsck.skipList /dev/null &&
+	test_must_fail git --git-dir=dst/.git fetch "file://$(pwd)" $refspec &&
 	echo $commit >dst/.git/SKIP &&
+
+	# fetch.fsck.* does not fall back on fsck.*
+	git --git-dir=dst/.git config fsck.skipList dst/.git/SKIP &&
+	test_must_fail git --git-dir=dst/.git fetch "file://$(pwd)" $refspec &&
+
+	git --git-dir=dst/.git config fetch.fsck.skipList dst/.git/SKIP &&
 	git --git-dir=dst/.git fetch "file://$(pwd)" $refspec
 '
 
@@ -166,6 +178,11 @@ test_expect_success 'push with receive.fsck.missingEmail=warn' '
 	git init dst &&
 	git --git-dir=dst/.git config receive.fsckobjects true &&
 	test_must_fail git push --porcelain dst bogus &&
+
+	# receive.fsck.<msg-id> does not fall back on fsck.<msg-id>
+	git --git-dir=dst/.git config fsck.missingEmail warn &&
+	test_must_fail git push --porcelain dst bogus &&
+
 	git --git-dir=dst/.git config \
 		receive.fsck.missingEmail warn &&
 	git push --porcelain dst bogus >act 2>&1 &&
@@ -185,6 +202,11 @@ test_expect_success 'fetch with fetch.fsck.missingEmail=warn' '
 	git init dst &&
 	git --git-dir=dst/.git config fetch.fsckobjects true &&
 	test_must_fail git --git-dir=dst/.git fetch "file://$(pwd)" $refspec &&
+
+	# fetch.fsck.<msg-id> does not fall back on fsck.<msg-id>
+	git --git-dir=dst/.git config fsck.missingEmail warn &&
+	test_must_fail git --git-dir=dst/.git fetch "file://$(pwd)" $refspec &&
+
 	git --git-dir=dst/.git config \
 		fetch.fsck.missingEmail warn &&
 	git --git-dir=dst/.git fetch "file://$(pwd)" $refspec >act 2>&1 &&
-- 
2.18.0.345.g5c9ce644c3

Re: [PATCH v3 08/10] fsck: test & document {fetch,receive}.fsck.* config fallback

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> Test and document that the {fetch,receive}.fsck.* family of variables
> doesn't fall back on the corresponding .fsck.* variables.
>
> This was alluded to in the existing documentation by saying that
> "receive" looks at receive.fsck.* and "fsck" looks at fsck.* etc., but
> it wasn't explicitly stated that there was no fallback, and if you'd
> e.g. like to configure the skipList you need to do that for all three.

Hmph, personally I felt that the updates so far made it clear and
explicit enough.

>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  Documentation/config.txt        | 12 ++++++++++++
>  t/t5504-fetch-receive-strict.sh | 26 ++++++++++++++++++++++++--
>  2 files changed, 36 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 8dace49daa..57c463c6e2 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -1619,6 +1619,12 @@ The rest of the documentation discusses `fsck.*` for brevity, but the
>  same applies for the corresponding `receive.fsck.*` and
>  `fetch.<msg-id>.*`. variables.
>  +
> +Unlike variables like `color.ui` and `core.editor` the
> +`receive.fsck.<msg-id>` and `fetch.fsck.<msg-id>` variables will not
> +fall back on the `fsck.<msg-id>` configuration if they aren't set. To

I do not think "Unlike ..." and mention of two sample variables
(color.ui and core.editor) adds much value to the paragraph,
primarily because it does not mention "color.status" etc. to show
what kind of fallback the sentence wants to discuss.  I actually
find the paragraph even clearer to read if it began with "The
`receive.fsck.<msgid>` and ...".

If you insist keeping that "Unlike..." thing, please have a comma
before "the `receive.fsck.<msg-id>`", but I'd recommend to remove
it.

> +uniformly configure the same fsck settings in different circumstances
> +all three of them they must all set to the same values.

That is not incorrect per-se, but I doubt it is a good idea to give
an impression that most people would want al three to be the same.

You want fsck.<msg-id>.* to be stricter to ensure you do not create
new problems, together with fsck.skipList to pardon mistakes that
have already happened, and with $transfer.fsck.<msg-id>, optionally
loosen certain class of breakage if a particular project has special
needs (e.g. the tool they use is known to produce incorrect
objects).  Which makes me expect that in most cases, fsck.<msg-id>.*
would be empty, fsck.skipList to be populated with known-bad ones,
and $transfer.fsck.<msg-id> may be empty, or may not be empty.

On the other hand, there may be value in allowing skipList to be
shared across three, though.  You may have known-bad ones and you'd
need to give a copy of that known-bad thing to other participants of
the project anyway.

[PATCH v3 09/10] fsck: add stress tests for fsck.skipList

[Ævar Arnfjörð Bjarmason]

Stress test the parsing logic shared by fsck.skipList and
{fetch,receive}.fsck.skipList added in cd94c6f91e ("fsck: git
receive-pack: support excluding objects from fsck'ing",
2015-06-22). There were no tests for the work done by the
init_skiplist() routine, e.g. how it dies on invalid input.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t5504-fetch-receive-strict.sh | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/t/t5504-fetch-receive-strict.sh b/t/t5504-fetch-receive-strict.sh
index 771a94b4b6..7f06b537d3 100755
--- a/t/t5504-fetch-receive-strict.sh
+++ b/t/t5504-fetch-receive-strict.sh
@@ -133,6 +133,14 @@ committer Bugs Bunny <bugs@bun.ni> 1234567890 +0000
 This commit object intentionally broken
 EOF
 
+test_expect_success 'fsck with invalid or bogus skipList input' '
+	git -c fsck.skipList=/dev/null -c fsck.missingEmail=ignore fsck &&
+	test_must_fail git -c fsck.skipList=does-not-exist -c fsck.missingEmail=ignore fsck 2>err &&
+	test_i18ngrep "Could not open skip list: does-not-exist" err &&
+	test_must_fail git -c fsck.skipList=.git/config -c fsck.missingEmail=ignore fsck 2>err &&
+	test_i18ngrep "Invalid SHA-1: \[core\]" err
+'
+
 test_expect_success 'push with receive.fsck.skipList' '
 	commit="$(git hash-object -t commit -w --stdin <bogus-commit)" &&
 	git push . $commit:refs/heads/bogus &&
@@ -146,6 +154,16 @@ test_expect_success 'push with receive.fsck.skipList' '
 	git --git-dir=dst/.git config fsck.skipList SKIP &&
 	test_must_fail git push --porcelain dst bogus &&
 
+	# Invalid and/or bogus skipList input
+	git --git-dir=dst/.git config receive.fsck.skipList /dev/null &&
+	test_must_fail git push --porcelain dst bogus &&
+	git --git-dir=dst/.git config receive.fsck.skipList does-not-exist &&
+	test_must_fail git push --porcelain dst bogus 2>err &&
+	test_i18ngrep "Could not open skip list: does-not-exist" err &&
+	git --git-dir=dst/.git config receive.fsck.skipList config &&
+	test_must_fail git push --porcelain dst bogus 2>err &&
+	test_i18ngrep "Invalid SHA-1: \[core\]" err &&
+
 	git --git-dir=dst/.git config receive.fsck.skipList SKIP &&
 	git push --porcelain dst bogus
 '
@@ -166,6 +184,16 @@ test_expect_success 'fetch with fetch.fsck.skipList' '
 	git --git-dir=dst/.git config fsck.skipList dst/.git/SKIP &&
 	test_must_fail git --git-dir=dst/.git fetch "file://$(pwd)" $refspec &&
 
+	# Invalid and/or bogus skipList input
+	git --git-dir=dst/.git config fetch.fsck.skipList /dev/null &&
+	test_must_fail git --git-dir=dst/.git fetch "file://$(pwd)" $refspec &&
+	git --git-dir=dst/.git config fetch.fsck.skipList does-not-exist &&
+	test_must_fail git --git-dir=dst/.git fetch "file://$(pwd)" $refspec 2>err &&
+	test_i18ngrep "Could not open skip list: does-not-exist" err &&
+	git --git-dir=dst/.git config fetch.fsck.skipList dst/.git/config &&
+	test_must_fail git --git-dir=dst/.git fetch "file://$(pwd)" $refspec 2>err &&
+	test_i18ngrep "Invalid SHA-1: \[core\]" err &&
+
 	git --git-dir=dst/.git config fetch.fsck.skipList dst/.git/SKIP &&
 	git --git-dir=dst/.git fetch "file://$(pwd)" $refspec
 '
-- 
2.18.0.345.g5c9ce644c3

[PATCH v3 10/10] fsck: test and document unknown fsck.<msg-id> values

[Ævar Arnfjörð Bjarmason]

When fsck.<msg-id> is set to an unknown value it'll cause "fsck" to
die, but the same is not rue of the "fetch" and "receive"
variants. Document this and test for it.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/config.txt        |  4 ++++
 t/t5504-fetch-receive-strict.sh | 14 ++++++++++++++
 2 files changed, 18 insertions(+)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 57c463c6e2..4cead6119a 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1637,6 +1637,10 @@ In general, it is better to enumerate existing objects with problems
 with `fsck.skipList`, instead of listing the kind of breakages these
 problematic objects share to be ignored, as doing the latter will
 allow new instances of the same breakages go unnoticed.
++
+Setting an unknown `fsck.<msg-id>` value will cause fsck to die, but
+doing the same for `receive.fsck.<msg-id>` and `fetch.fsck.<msg-id>`
+will only cause git to warn.
 
 fsck.skipList::
 	The path to a sorted list of object names (i.e. one SHA-1 per
diff --git a/t/t5504-fetch-receive-strict.sh b/t/t5504-fetch-receive-strict.sh
index 7f06b537d3..62f3569891 100755
--- a/t/t5504-fetch-receive-strict.sh
+++ b/t/t5504-fetch-receive-strict.sh
@@ -198,6 +198,10 @@ test_expect_success 'fetch with fetch.fsck.skipList' '
 	git --git-dir=dst/.git fetch "file://$(pwd)" $refspec
 '
 
+test_expect_success 'fsck.<unknownmsg-id> dies' '
+	test_must_fail git -c fsck.whatEver=ignore fsck 2>err &&
+	test_i18ngrep "Unhandled message id: whatever" err
+'
 
 test_expect_success 'push with receive.fsck.missingEmail=warn' '
 	commit="$(git hash-object -t commit -w --stdin <bogus-commit)" &&
@@ -211,10 +215,15 @@ test_expect_success 'push with receive.fsck.missingEmail=warn' '
 	git --git-dir=dst/.git config fsck.missingEmail warn &&
 	test_must_fail git push --porcelain dst bogus &&
 
+	# receive.fsck.<unknownmsg-id> warns
+	git --git-dir=dst/.git config \
+		receive.fsck.whatEver error &&
+
 	git --git-dir=dst/.git config \
 		receive.fsck.missingEmail warn &&
 	git push --porcelain dst bogus >act 2>&1 &&
 	grep "missingEmail" act &&
+	test_i18ngrep "Skipping unknown msg id.*whatever" act &&
 	git --git-dir=dst/.git branch -D bogus &&
 	git --git-dir=dst/.git config --add \
 		receive.fsck.missingEmail ignore &&
@@ -235,10 +244,15 @@ test_expect_success 'fetch with fetch.fsck.missingEmail=warn' '
 	git --git-dir=dst/.git config fsck.missingEmail warn &&
 	test_must_fail git --git-dir=dst/.git fetch "file://$(pwd)" $refspec &&
 
+	# receive.fsck.<unknownmsg-id> warns
+	git --git-dir=dst/.git config \
+		fetch.fsck.whatEver error &&
+
 	git --git-dir=dst/.git config \
 		fetch.fsck.missingEmail warn &&
 	git --git-dir=dst/.git fetch "file://$(pwd)" $refspec >act 2>&1 &&
 	grep "missingEmail" act &&
+	test_i18ngrep "Skipping unknown msg id.*whatever" act &&
 	rm -rf dst &&
 	git init dst &&
 	git --git-dir=dst/.git config fetch.fsckobjects true &&
-- 
2.18.0.345.g5c9ce644c3

Re: [PATCH v3 10/10] fsck: test and document unknown fsck.<msg-id> values

[Ævar Arnfjörð Bjarmason]

a
On Fri, Jul 27 2018, Ævar Arnfjörð Bjarmason wrote:

> When fsck.<msg-id> is set to an unknown value it'll cause "fsck" to
> die, but the same is not rue of the "fetch" and "receive"

s/rue/t&/. Looks like we're headed for a v4. I'll fix this typo.

Re: [PATCH v3 10/10] fsck: test and document unknown fsck.<msg-id> values

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> When fsck.<msg-id> is set to an unknown value it'll cause "fsck" to
> die, but the same is not rue of the "fetch" and "receive"
> variants. Document this and test for it.

Interesting.  Before documenting and adding a test to cast the
current behaviour in stone, do we need to see if the discrepancy is
desired and designed one, or something we may want to fix?



>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  Documentation/config.txt        |  4 ++++
>  t/t5504-fetch-receive-strict.sh | 14 ++++++++++++++
>  2 files changed, 18 insertions(+)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 57c463c6e2..4cead6119a 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -1637,6 +1637,10 @@ In general, it is better to enumerate existing objects with problems
>  with `fsck.skipList`, instead of listing the kind of breakages these
>  problematic objects share to be ignored, as doing the latter will
>  allow new instances of the same breakages go unnoticed.
> ++
> +Setting an unknown `fsck.<msg-id>` value will cause fsck to die, but
> +doing the same for `receive.fsck.<msg-id>` and `fetch.fsck.<msg-id>`
> +will only cause git to warn.
>  
>  fsck.skipList::
>  	The path to a sorted list of object names (i.e. one SHA-1 per
> diff --git a/t/t5504-fetch-receive-strict.sh b/t/t5504-fetch-receive-strict.sh
> index 7f06b537d3..62f3569891 100755
> --- a/t/t5504-fetch-receive-strict.sh
> +++ b/t/t5504-fetch-receive-strict.sh
> @@ -198,6 +198,10 @@ test_expect_success 'fetch with fetch.fsck.skipList' '
>  	git --git-dir=dst/.git fetch "file://$(pwd)" $refspec
>  '
>  
> +test_expect_success 'fsck.<unknownmsg-id> dies' '
> +	test_must_fail git -c fsck.whatEver=ignore fsck 2>err &&
> +	test_i18ngrep "Unhandled message id: whatever" err
> +'
>  
>  test_expect_success 'push with receive.fsck.missingEmail=warn' '
>  	commit="$(git hash-object -t commit -w --stdin <bogus-commit)" &&
> @@ -211,10 +215,15 @@ test_expect_success 'push with receive.fsck.missingEmail=warn' '
>  	git --git-dir=dst/.git config fsck.missingEmail warn &&
>  	test_must_fail git push --porcelain dst bogus &&
>  
> +	# receive.fsck.<unknownmsg-id> warns
> +	git --git-dir=dst/.git config \
> +		receive.fsck.whatEver error &&
> +
>  	git --git-dir=dst/.git config \
>  		receive.fsck.missingEmail warn &&
>  	git push --porcelain dst bogus >act 2>&1 &&
>  	grep "missingEmail" act &&
> +	test_i18ngrep "Skipping unknown msg id.*whatever" act &&
>  	git --git-dir=dst/.git branch -D bogus &&
>  	git --git-dir=dst/.git config --add \
>  		receive.fsck.missingEmail ignore &&
> @@ -235,10 +244,15 @@ test_expect_success 'fetch with fetch.fsck.missingEmail=warn' '
>  	git --git-dir=dst/.git config fsck.missingEmail warn &&
>  	test_must_fail git --git-dir=dst/.git fetch "file://$(pwd)" $refspec &&
>  
> +	# receive.fsck.<unknownmsg-id> warns
> +	git --git-dir=dst/.git config \
> +		fetch.fsck.whatEver error &&
> +
>  	git --git-dir=dst/.git config \
>  		fetch.fsck.missingEmail warn &&
>  	git --git-dir=dst/.git fetch "file://$(pwd)" $refspec >act 2>&1 &&
>  	grep "missingEmail" act &&
> +	test_i18ngrep "Skipping unknown msg id.*whatever" act &&
>  	rm -rf dst &&
>  	git init dst &&
>  	git --git-dir=dst/.git config fetch.fsckobjects true &&

Re: [PATCH v3 10/10] fsck: test and document unknown fsck.<msg-id> values

[Ævar Arnfjörð Bjarmason]

On Fri, Jul 27 2018, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> When fsck.<msg-id> is set to an unknown value it'll cause "fsck" to
>> die, but the same is not rue of the "fetch" and "receive"
>> variants. Document this and test for it.
>
> Interesting.  Before documenting and adding a test to cast the
> current behaviour in stone, do we need to see if the discrepancy is
> desired and designed one, or something we may want to fix?

We could change it. This is just something I ran into and figured it
should be tested / documented, and didn't feel any need to change it
myself.

The current behavior is probably more of an organically grown
accident. Maybe we should make all of these warn.

Trying to post-hoc rationalize these, it probably makes sense for
receive.* not to die, since you don't want pushes to fail if you're
tweaking this on a server and typo it, same with fetch (although less
so), whereas "fsck" tends to be more of an offline validation command.

So Yeah, I can change this, or not. What do you/others think?

>
>>
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> ---
>>  Documentation/config.txt        |  4 ++++
>>  t/t5504-fetch-receive-strict.sh | 14 ++++++++++++++
>>  2 files changed, 18 insertions(+)
>>
>> diff --git a/Documentation/config.txt b/Documentation/config.txt
>> index 57c463c6e2..4cead6119a 100644
>> --- a/Documentation/config.txt
>> +++ b/Documentation/config.txt
>> @@ -1637,6 +1637,10 @@ In general, it is better to enumerate existing objects with problems
>>  with `fsck.skipList`, instead of listing the kind of breakages these
>>  problematic objects share to be ignored, as doing the latter will
>>  allow new instances of the same breakages go unnoticed.
>> ++
>> +Setting an unknown `fsck.<msg-id>` value will cause fsck to die, but
>> +doing the same for `receive.fsck.<msg-id>` and `fetch.fsck.<msg-id>`
>> +will only cause git to warn.
>>
>>  fsck.skipList::
>>  	The path to a sorted list of object names (i.e. one SHA-1 per
>> diff --git a/t/t5504-fetch-receive-strict.sh b/t/t5504-fetch-receive-strict.sh
>> index 7f06b537d3..62f3569891 100755
>> --- a/t/t5504-fetch-receive-strict.sh
>> +++ b/t/t5504-fetch-receive-strict.sh
>> @@ -198,6 +198,10 @@ test_expect_success 'fetch with fetch.fsck.skipList' '
>>  	git --git-dir=dst/.git fetch "file://$(pwd)" $refspec
>>  '
>>
>> +test_expect_success 'fsck.<unknownmsg-id> dies' '
>> +	test_must_fail git -c fsck.whatEver=ignore fsck 2>err &&
>> +	test_i18ngrep "Unhandled message id: whatever" err
>> +'
>>
>>  test_expect_success 'push with receive.fsck.missingEmail=warn' '
>>  	commit="$(git hash-object -t commit -w --stdin <bogus-commit)" &&
>> @@ -211,10 +215,15 @@ test_expect_success 'push with receive.fsck.missingEmail=warn' '
>>  	git --git-dir=dst/.git config fsck.missingEmail warn &&
>>  	test_must_fail git push --porcelain dst bogus &&
>>
>> +	# receive.fsck.<unknownmsg-id> warns
>> +	git --git-dir=dst/.git config \
>> +		receive.fsck.whatEver error &&
>> +
>>  	git --git-dir=dst/.git config \
>>  		receive.fsck.missingEmail warn &&
>>  	git push --porcelain dst bogus >act 2>&1 &&
>>  	grep "missingEmail" act &&
>> +	test_i18ngrep "Skipping unknown msg id.*whatever" act &&
>>  	git --git-dir=dst/.git branch -D bogus &&
>>  	git --git-dir=dst/.git config --add \
>>  		receive.fsck.missingEmail ignore &&
>> @@ -235,10 +244,15 @@ test_expect_success 'fetch with fetch.fsck.missingEmail=warn' '
>>  	git --git-dir=dst/.git config fsck.missingEmail warn &&
>>  	test_must_fail git --git-dir=dst/.git fetch "file://$(pwd)" $refspec &&
>>
>> +	# receive.fsck.<unknownmsg-id> warns
>> +	git --git-dir=dst/.git config \
>> +		fetch.fsck.whatEver error &&
>> +
>>  	git --git-dir=dst/.git config \
>>  		fetch.fsck.missingEmail warn &&
>>  	git --git-dir=dst/.git fetch "file://$(pwd)" $refspec >act 2>&1 &&
>>  	grep "missingEmail" act &&
>> +	test_i18ngrep "Skipping unknown msg id.*whatever" act &&
>>  	rm -rf dst &&
>>  	git init dst &&
>>  	git --git-dir=dst/.git config fetch.fsckobjects true &&

Re: [PATCH v3 10/10] fsck: test and document unknown fsck.<msg-id> values

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

>>> When fsck.<msg-id> is set to an unknown value it'll cause "fsck" to
>>> die, but the same is not rue of the "fetch" and "receive"
>>> variants. Document this and test for it.
> ...
> We could change it. This is just something I ran into and figured it
> should be tested / documented, and didn't feel any need to change it
> myself.
>
> The current behavior is probably more of an organically grown
> accident. Maybe we should make all of these warn.

Or die.  I do agree with your assessment that the discrepancy was
not designed, and my inclination for a correctness-centered feature
like fsck is to err on the side of caution, rather than letting a
misconfiguration pass unintended kinds of breakages through.

Thanks.

[PATCH v2 1/5] config doc: don't describe *.fetchObjects twice

[Ævar Arnfjörð Bjarmason]

Let's not duplicate the description of what *.fsckObjects does twice.
instead let's refer to transfer.fsckObjects from both fetch.* and
receive.*.

I don't think this description of it makes much sense, but for now I'm
just moving the existing documentation around. Making it better will
be done in a later patch.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/config.txt | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 84e2891aed..623dffd198 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1422,10 +1422,9 @@ fetch.recurseSubmodules::
 
 fetch.fsckObjects::
 	If it is set to true, git-fetch-pack will check all fetched
-	objects. It will abort in the case of a malformed object or a
-	broken link. The result of an abort are only dangling objects.
-	Defaults to false. If not set, the value of `transfer.fsckObjects`
-	is used instead.
+	objects. See `transfer.fsckObjects` for what's
+	checked. Defaults to false. If not set, the value of
+	`transfer.fsckObjects` is used instead.
 
 fetch.unpackLimit::
 	If the number of objects fetched over the Git native
@@ -2845,10 +2844,9 @@ receive.certNonceSlop::
 
 receive.fsckObjects::
 	If it is set to true, git-receive-pack will check all received
-	objects. It will abort in the case of a malformed object or a
-	broken link. The result of an abort are only dangling objects.
-	Defaults to false. If not set, the value of `transfer.fsckObjects`
-	is used instead.
+	objects. See `transfer.fsckObjects` for what's checked.
+	Defaults to false. If not set, the value of
+	`transfer.fsckObjects` is used instead.
 
 receive.fsck.<msg-id>::
 	When `receive.fsckObjects` is set to true, errors can be switched
@@ -3332,6 +3330,10 @@ transfer.fsckObjects::
 	When `fetch.fsckObjects` or `receive.fsckObjects` are
 	not set, the value of this variable is used instead.
 	Defaults to false.
++
+When set, the fetch or receive will abort in the case of a malformed
+object or a broken link. The result of an abort are only dangling
+objects.
 
 transfer.hideRefs::
 	String(s) `receive-pack` and `upload-pack` use to decide which
-- 
2.17.0.290.gded63e768a

Re: [PATCH v2 1/5] config doc: don't describe *.fetchObjects twice

[Eric Sunshine]

On Fri, May 25, 2018 at 3:28 PM, Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> Let's not duplicate the description of what *.fsckObjects does twice.

Nit: "duplicate" and "twice" smell redundant.

> instead let's refer to transfer.fsckObjects from both fetch.* and
> receive.*.

s/instead/Instead/

Perhaps the above paragraph can be rewritten:

    Refer readers of fetch.fsckObjects and receive.fsckObjects to
    transfer.fsckObjects instead of repeating the description at each
    location.

(Not at all worth a re-roll.)

> I don't think this description of it makes much sense, but for now I'm
> just moving the existing documentation around. Making it better will
> be done in a later patch.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>

[PATCH v2 2/5] config doc: unify the description of fsck.* and receive.fsck.*

[Ævar Arnfjörð Bjarmason]

The documentation for the fsck.<msg-id> and receive.fsck.<msg-id>
variables was mostly duplicated in two places, with fsck.<msg-id>
making no mention of the corresponding receive.fsck.<msg-id>, and the
same for fsck.skipList.

I spent quite a lot of time today wondering why setting the
fsck.<msg-id> variant wasn't working to clone a legacy repository (not
that that would have worked anyway, but a subsequent patch implements
fetch.fsck.<msg-id>).

Rectify this situation by describing the feature in general terms
under the fsck.* documentation, and make the receive.fsck.*
documentation refer to those variables instead.

This documentation was initially added in 2becf00ff7 ("fsck: support
demoting errors to warnings", 2015-06-22) and 4b55b9b479 ("fsck:
document the new receive.fsck.<msg-id> options", 2015-06-22).

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/config.txt | 73 ++++++++++++++++++++++------------------
 1 file changed, 41 insertions(+), 32 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 623dffd198..af7311e73f 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1554,23 +1554,42 @@ filter.<driver>.smudge::
 	linkgit:gitattributes[5] for details.
 
 fsck.<msg-id>::
-	Allows overriding the message type (error, warn or ignore) of a
-	specific message ID such as `missingEmail`.
-+
-For convenience, fsck prefixes the error/warning with the message ID,
-e.g.  "missingEmail: invalid author/committer line - missing email" means
-that setting `fsck.missingEmail = ignore` will hide that issue.
-+
-This feature is intended to support working with legacy repositories
-which cannot be repaired without disruptive changes.
+	During fsck git may find issues with legacy data which
+	wouldn't be generated by current versions of git, and which
+	wouldn't be sent over the wire if `transfer.fsckObjects` was
+	set. This feature is intended to support working with legacy
+	repositories containing such data.
++
+Setting `fsck.<msg-id>` will be picked up by linkgit:git-fsck[1], but
+to accept pushes of such data set `receive.fsck.<msg-id>` instead. The
+rest of the documentation discusses `fsck.*` for brevity, but the same
+applies for the corresponding `receive.fsck.*` variables.
++
+When `fsck.<msg-id>` is set, errors can be switched to warnings and
+vice versa by configuring the `fsck.<msg-id>` setting where the
+`<msg-id>` is the fsck message ID and the value is one of `error`,
+`warn` or `ignore`. For convenience, fsck prefixes the error/warning
+with the message ID, e.g. "missingEmail: invalid author/committer line
+- missing email" means that setting `fsck.missingEmail = ignore` will
+hide that issue.
++
+Depending on the circumstances it might be better to use
+`fsck.skipList` instead to explicitly whitelist those objects that
+have issues, otherwise new occurrences of the same issue will be
+hidden going forward, although that's unlikely to happen in practice
+unless someone is being deliberately malicious.
 
 fsck.skipList::
 	The path to a sorted list of object names (i.e. one SHA-1 per
-	line) that are known to be broken in a non-fatal way and should
-	be ignored. This feature is useful when an established project
-	should be accepted despite early commits containing errors that
-	can be safely ignored such as invalid committer email addresses.
-	Note: corrupt objects cannot be skipped with this setting.
+	line) that are known to be broken in a non-fatal way and
+	should be ignored. This feature is useful when an established
+	project should be accepted despite early commits containing
+	errors that can be safely ignored such as invalid committer
+	email addresses. Note: corrupt objects cannot be skipped with
+	this setting.
++
+Like `fsck.<msg-id>` this variable has a corresponding
+`receive.fsck.skipList` variant.
 
 gc.aggressiveDepth::
 	The depth parameter used in the delta compression
@@ -2849,26 +2868,16 @@ receive.fsckObjects::
 	`transfer.fsckObjects` is used instead.
 
 receive.fsck.<msg-id>::
-	When `receive.fsckObjects` is set to true, errors can be switched
-	to warnings and vice versa by configuring the `receive.fsck.<msg-id>`
-	setting where the `<msg-id>` is the fsck message ID and the value
-	is one of `error`, `warn` or `ignore`. For convenience, fsck prefixes
-	the error/warning with the message ID, e.g. "missingEmail: invalid
-	author/committer line - missing email" means that setting
-	`receive.fsck.missingEmail = ignore` will hide that issue.
-+
-This feature is intended to support working with legacy repositories
-which would not pass pushing when `receive.fsckObjects = true`, allowing
-the host to accept repositories with certain known issues but still catch
-other issues.
+	Acts like `fsck.<msg-id>`, but is used by
+	linkgit:git-receive-pack[1] instead of
+	linkgit:git-fsck[1]. See the `fsck.<msg-id>` documentation for
+	details.
 
 receive.fsck.skipList::
-	The path to a sorted list of object names (i.e. one SHA-1 per
-	line) that are known to be broken in a non-fatal way and should
-	be ignored. This feature is useful when an established project
-	should be accepted despite early commits containing errors that
-	can be safely ignored such as invalid committer email addresses.
-	Note: corrupt objects cannot be skipped with this setting.
+	Acts like `fsck.skipList`, but is used by
+	linkgit:git-receive-pack[1] instead of
+	linkgit:git-fsck[1]. See the `fsck.skipList` documentation for
+	details.
 
 receive.keepAlive::
 	After receiving the pack from the client, `receive-pack` may
-- 
2.17.0.290.gded63e768a

Re: [PATCH v2 2/5] config doc: unify the description of fsck.* and receive.fsck.*

[Eric Sunshine]

On Fri, May 25, 2018 at 3:28 PM, Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> The documentation for the fsck.<msg-id> and receive.fsck.<msg-id>
> variables was mostly duplicated in two places, with fsck.<msg-id>
> making no mention of the corresponding receive.fsck.<msg-id>, and the
> same for fsck.skipList.
> [...]
> Rectify this situation by describing the feature in general terms
> under the fsck.* documentation, and make the receive.fsck.*
> documentation refer to those variables instead.
> [...]
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> @@ -1554,23 +1554,42 @@ filter.<driver>.smudge::
> +Depending on the circumstances it might be better to use
> +`fsck.skipList` instead to explicitly whitelist those objects that
> +have issues, otherwise new occurrences of the same issue will be
> +hidden going forward, although that's unlikely to happen in practice
> +unless someone is being deliberately malicious.

Is it worth mentioning buggy tools and/or other buggy Git
implementations as sources?

>  fsck.skipList::
>         The path to a sorted list of object names (i.e. one SHA-1 per
> -       line) that are known to be broken in a non-fatal way and should
> -       be ignored. This feature is useful when an established project
> -       should be accepted despite early commits containing errors that
> -       can be safely ignored such as invalid committer email addresses.
> -       Note: corrupt objects cannot be skipped with this setting.
> +       line) that are known to be broken in a non-fatal way and
> +       should be ignored. This feature is useful when an established
> +       project should be accepted despite early commits containing
> +       errors that can be safely ignored such as invalid committer
> +       email addresses. Note: corrupt objects cannot be skipped with
> +       this setting.

Not sure why this paragraph got re-wrapped (without, as far as I can
see, any other changes), thus making the patch unnecessarily noisy.

> +Like `fsck.<msg-id>` this variable has a corresponding
> +`receive.fsck.skipList` variant.

Re: [PATCH v2 2/5] config doc: unify the description of fsck.* and receive.fsck.*

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

>> @@ -1554,23 +1554,42 @@ filter.<driver>.smudge::
>> +Depending on the circumstances it might be better to use
>> +`fsck.skipList` instead to explicitly whitelist those objects that
>> +have issues, otherwise new occurrences of the same issue will be

I had to read the "instead to ..." part three times before
convincing myself that this is a good description, and I agree with
the assessment.  Perhaps we would want to make the recommendation a
bit stronger, even.

	In general, it is better to enumerate existing objects with
	problems with skipList, instead of listing the kind of
	breakages these problematic objects share to be ignored, as
	doing the latter will allow new instances of the same
	breakages go unnoticed.

If the project has some tool constraints and have to accept new
"broken" objects on ongoing basis, then fsck.<msg-id> facility may
make sense, but that is probably a very narrow special use case.

>> +hidden going forward, although that's unlikely to happen in practice
>> +unless someone is being deliberately malicious.
>
> Is it worth mentioning buggy tools and/or other buggy Git
> implementations as sources?

Or old Git implementations we ourselves shipped.  I do not think it
is worth mentioning it, but I do agree that "unless somebody is
being deliberatly malicious" is misleading, if that is what you are
getting at.  One use of fsck is about noticing that you are under
attack, so "unless someone is being malicious" there in the sentence
does not make sense.  When somebody is attacking you, you do not
want to use fsck.<msg-id> to ignore it.

But that becomes a moot point, if we were to follow the line of
rewrite I suggested above.

Re: [PATCH v2 2/5] config doc: unify the description of fsck.* and receive.fsck.*

[Ævar Arnfjörð Bjarmason]

On Mon, May 28, 2018 at 11:45 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
>
>>> @@ -1554,23 +1554,42 @@ filter.<driver>.smudge::
>>> +Depending on the circumstances it might be better to use
>>> +`fsck.skipList` instead to explicitly whitelist those objects that
>>> +have issues, otherwise new occurrences of the same issue will be
>
> I had to read the "instead to ..." part three times before
> convincing myself that this is a good description, and I agree with
> the assessment.  Perhaps we would want to make the recommendation a
> bit stronger, even.
>
>         In general, it is better to enumerate existing objects with
>         problems with skipList, instead of listing the kind of
>         breakages these problematic objects share to be ignored, as
>         doing the latter will allow new instances of the same
>         breakages go unnoticed.
>
> If the project has some tool constraints and have to accept new
> "broken" objects on ongoing basis, then fsck.<msg-id> facility may
> make sense, but that is probably a very narrow special use case.

That makes sense. I'll reword this bit.

>>> +hidden going forward, although that's unlikely to happen in practice
>>> +unless someone is being deliberately malicious.
>>
>> Is it worth mentioning buggy tools and/or other buggy Git
>> implementations as sources?
>
> Or old Git implementations we ourselves shipped.  I do not think it
> is worth mentioning it, but I do agree that "unless somebody is
> being deliberatly malicious" is misleading, if that is what you are
> getting at.  One use of fsck is about noticing that you are under
> attack, so "unless someone is being malicious" there in the sentence
> does not make sense.  When somebody is attacking you, you do not
> want to use fsck.<msg-id> to ignore it.
>
> But that becomes a moot point, if we were to follow the line of
> rewrite I suggested above.

I'll try to clarify this, but I think we really should have some bit
there about historical tools. Realistically no new git tools produce
these, so the user needs to be made aware of what the trade-off of
turning these on is.

The reality of that is that these objects are exceedingly rare, and
mostly found in various old repositories. Something like that need to
be mentioned so the user can weigh the trade-off of turning this on.

Re: [PATCH v2 2/5] config doc: unify the description of fsck.* and receive.fsck.*

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> On Mon, May 28, 2018 at 11:45 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> If the project has some tool constraints and have to accept new
>> "broken" objects on ongoing basis, then fsck.<msg-id> facility may
>> make sense, but that is probably a very narrow special use case.
>
> That makes sense. I'll reword this bit.
> ...
> I'll try to clarify this, but I think we really should have some bit
> there about historical tools. Realistically no new git tools produce
> these, so the user needs to be made aware of what the trade-off of
> turning these on is.
>
> The reality of that is that these objects are exceedingly rare, and
> mostly found in various old repositories. Something like that need to
> be mentioned so the user can weigh the trade-off of turning this on.

Rare or not, once we say "avoid fsck.<msg-id> unless you have a good
reason not to", wouldn't that be sufficient?  

Between "fsck.<msg-id> makes sense only when you use these rare and
you-probably-never-heard-of tools ongoing basis" and "when you
already have (slightly)broken objects, naming each of them in
skiplist, rather than covering the class, is better because you want
*new* instances of the same breakage", I'd imagine the latter would be
more helpful.

In any case, let's see if there are more input to this topic and
then wrap it up in v3 ;-)

Thanks.

Re: [PATCH v2 2/5] config doc: unify the description of fsck.* and receive.fsck.*

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Between "fsck.<msg-id> makes sense only when you use these rare and
> you-probably-never-heard-of tools ongoing basis" and "when you
> already have (slightly)broken objects, naming each of them in
> skiplist, rather than covering the class, is better because you want
> *new* instances of the same breakage", I'd imagine the latter would be

s/breakage/& caught/; obviously, otherwise what I typed does not
make much sense.  Sorry about the premature <SEND>.

> more helpful.
>
> In any case, let's see if there are more input to this topic and
> then wrap it up in v3 ;-)
>
> Thanks.

Re: [PATCH v2 2/5] config doc: unify the description of fsck.* and receive.fsck.*

[Ævar Arnfjörð Bjarmason]

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

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> On Mon, May 28, 2018 at 11:45 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>> If the project has some tool constraints and have to accept new
>>> "broken" objects on ongoing basis, then fsck.<msg-id> facility may
>>> make sense, but that is probably a very narrow special use case.
>>
>> That makes sense. I'll reword this bit.
>> ...
>> I'll try to clarify this, but I think we really should have some bit
>> there about historical tools. Realistically no new git tools produce
>> these, so the user needs to be made aware of what the trade-off of
>> turning these on is.
>>
>> The reality of that is that these objects are exceedingly rare, and
>> mostly found in various old repositories. Something like that need to
>> be mentioned so the user can weigh the trade-off of turning this on.
>
> Rare or not, once we say "avoid fsck.<msg-id> unless you have a good
> reason not to", wouldn't that be sufficient?

It's our documentation that should be clearly stating those reasons. If
we're not saying anything about these being historical bugs, then e.g. I
(not knowing the implementation) wouldn't have turned this on globally
on my site knowing that because I have none of these now I'm *very*
unlikely to have them in the future.

That's different from something that just happens rarely, because a rare
non-historical event can be expected to happen in the future.

> Between "fsck.<msg-id> makes sense only when you use these rare and
> you-probably-never-heard-of tools ongoing basis" and "when you
> already have (slightly)broken objects, naming each of them in
> skiplist, rather than covering the class, is better because you want
> *new* instances of the same breakage", I'd imagine the latter would be
> more helpful.
>
> In any case, let's see if there are more input to this topic and
> then wrap it up in v3 ;-)
>
> Thanks.

Re: [PATCH v2 2/5] config doc: unify the description of fsck.* and receive.fsck.*

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> It's our documentation that should be clearly stating those reasons. If
> we're not saying anything about these being historical bugs, then e.g. I
> (not knowing the implementation) wouldn't have turned this on globally
> on my site knowing that because I have none of these now I'm *very*
> unlikely to have them in the future.
>
> That's different from something that just happens rarely, because a rare
> non-historical event can be expected to happen in the future.

Interesting.  If I did not know Git at all, I would decide
completely opposite---because I have none of these now, I'm very
unlikely to have them in the future, so I would leave fsck.<msg-id>
alone to the generally recommended state (i.e. not customizing
without understanding what I am doing).  That way, (1) if that
unlikely thing happens, I would notice and have a chance to deal
with it, and (2) otherwise, I wouldn't have to worry about that
unlikely event at all.

And that decision would not change even if I _knew_ these knobs'
categories were invented to align with bugs and anomalies in older
implementations of Git.

>
>> Between "fsck.<msg-id> makes sense only when you use these rare and
>> you-probably-never-heard-of tools ongoing basis" and "when you
>> already have (slightly)broken objects, naming each of them in
>> skiplist, rather than covering the class, is better because you want
>> *new* instances of the same breakage", I'd imagine the latter would be
>> more helpful.
>>
>> In any case, let's see if there are more input to this topic and
>> then wrap it up in v3 ;-)
>>
>> Thanks.

[PATCH v2 3/5] config doc: elaborate on what transfer.fsckObjects does

[Ævar Arnfjörð Bjarmason]

The existing documentation led the user to believe that all we were
doing were basic reachability sanity checks, but that hasn't been true
for a very long time. Update the description to match reality, and
note the caveat that there's a quarantine for accepting pushes, but
not for fetching.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/config.txt | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index af7311e73f..71b3805b4e 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -3341,8 +3341,16 @@ transfer.fsckObjects::
 	Defaults to false.
 +
 When set, the fetch or receive will abort in the case of a malformed
-object or a broken link. The result of an abort are only dangling
-objects.
+object or a link to a nonexistent object. In addition, various other
+issues are checked for, including legacy issues (see `fsck.<msg-id>`),
+and potential security issues like the existence of a `.GIT` directory
+(see the release notes for v2.2.1 for details). Other sanity and
+security checks may be added in future releases.
++
+On the receiving side, failing fsckObjects will make those objects
+unreachable, see "QUARANTINE ENVIRONMENT" in
+linkgit:git-receive-pack[1]. On the fetch side, malformed objects will
+instead be left unreferenced in the repository.
 
 transfer.hideRefs::
 	String(s) `receive-pack` and `upload-pack` use to decide which
-- 
2.17.0.290.gded63e768a

Re: [PATCH v2 3/5] config doc: elaborate on what transfer.fsckObjects does

[Eric Sunshine]

On Fri, May 25, 2018 at 3:28 PM, Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> The existing documentation led the user to believe that all we were
> doing were basic reachability sanity checks, but that hasn't been true
> for a very long time. Update the description to match reality, and
> note the caveat that there's a quarantine for accepting pushes, but
> not for fetching.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> @@ -3341,8 +3341,16 @@ transfer.fsckObjects::
>  When set, the fetch or receive will abort in the case of a malformed
> +object or a link to a nonexistent object. In addition, various other
> +issues are checked for, including legacy issues (see `fsck.<msg-id>`),
> +and potential security issues like the existence of a `.GIT` directory
> +(see the release notes for v2.2.1 for details). Other sanity and
> +security checks may be added in future releases.
> ++
> +On the receiving side, failing fsckObjects will make those objects
> +unreachable, see "QUARANTINE ENVIRONMENT" in
> +linkgit:git-receive-pack[1]. On the fetch side, malformed objects will
> +instead be left unreferenced in the repository.

This version looks better. Thanks.

[PATCH v2 4/5] config doc: mention future aspirations for transfer.fsckObjects

[Ævar Arnfjörð Bjarmason]

Jeff King has said on at least a couple of occasions (at least one of
which may have been in person over beer) that leaving corrupt objects
in the local object store after a "fetch" that fails
transfer.fsckObjects should be fixed, and we should have something
like the server-side quarantine environment on the client-side.

Let's note that in the documentation so we don't seem to be claiming
that this is by design. A previous version of this change called the
current behavior a "bug", that's probably too strong a claim, but I
don't think anyone would dislike a hypothetical local quarantine
patch, so let's we might change this in the future.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/config.txt | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 71b3805b4e..f97f21c022 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -3350,7 +3350,9 @@ security checks may be added in future releases.
 On the receiving side, failing fsckObjects will make those objects
 unreachable, see "QUARANTINE ENVIRONMENT" in
 linkgit:git-receive-pack[1]. On the fetch side, malformed objects will
-instead be left unreferenced in the repository.
+instead be left unreferenced in the repository. That difference in
+behavior should not be relied upon. In the future, such objects may be
+quarantined for "fetch" as well.
 
 transfer.hideRefs::
 	String(s) `receive-pack` and `upload-pack` use to decide which
-- 
2.17.0.290.gded63e768a

Re: [PATCH v2 4/5] config doc: mention future aspirations for transfer.fsckObjects

[Christian Couder]

On Fri, May 25, 2018 at 9:28 PM, Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
>
> Jeff King has said on at least a couple of occasions (at least one of
> Let's note that in the documentation so we don't seem to be claiming
> that this is by design. A previous version of this change called the
> current behavior a "bug", that's probably too strong a claim, but I
> don't think anyone would dislike a hypothetical local quarantine
> patch, so let's we might change this in the future.

Maybe: s/so let's we might/so let's say we might/

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

[Ævar Arnfjörð Bjarmason]

Implement support for fetch.fsck.* corresponding with the existing
receive.fsck.*. This allows for pedantically cloning repositories with
specific issues without turning off fetch.fsckObjects.

One such repository is https://github.com/robbyrussell/oh-my-zsh.git
which before this change will emit this error when cloned with
fetch.fsckObjects:

    error: object 2b7227859263b6aabcc28355b0b994995b7148b6: zeroPaddedFilemode: contains zero-padded file modes
    fatal: Error in object
    fatal: index-pack failed

Now with fetch.fsck.zeroPaddedFilemode=warn we'll warn about that
issue, but the clone will succeed:

    warning: object 2b7227859263b6aabcc28355b0b994995b7148b6: zeroPaddedFilemode: contains zero-padded file modes
    warning: object a18c4d13c2a5fa2d4ecd5346c50e119b999b807d: zeroPaddedFilemode: contains zero-padded file modes
    warning: object 84df066176c8da3fd59b13731a86d90f4f1e5c9d: zeroPaddedFilemode: contains zero-padded file modes

The motivation for this is to be able to turn on fetch.fsckObjects
globally across a fleet of computers but still be able to manually
clone various legacy repositories by either white-listing specific
issues, or better yet whitelist specific objects.

The use of --git-dir=* instead of -C in the tests could be considered
somewhat archaic, but the tests I'm adding here are duplicating the
corresponding receive.* tests with as few changes as possible.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/config.txt        | 21 +++++++++++----
 fetch-pack.c                    | 32 +++++++++++++++++++++--
 t/t5504-fetch-receive-strict.sh | 46 +++++++++++++++++++++++++++++++++
 3 files changed, 92 insertions(+), 7 deletions(-)

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::
 	checked. Defaults to false. If not set, the value of
 	`transfer.fsckObjects` is used instead.
 
+fetch.fsck.<msg-id>::
+	Acts like `fsck.<msg-id>`, but is used by
+	linkgit:git-fetch-pack[1] instead of linkgit:git-fsck[1]. See
+	the `fsck.<msg-id>` documentation for details.
+
+fetch.fsck.skipList::
+	Acts like `fsck.skipList`, but is used by
+	linkgit:git-fetch-pack[1] instead of linkgit:git-fsck[1]. See
+	the `fsck.skipList` documentation for details.
+
 fetch.unpackLimit::
 	If the number of objects fetched over the Git native
 	transfer is below this
@@ -1561,9 +1571,10 @@ fsck.<msg-id>::
 	repositories containing such data.
 +
 Setting `fsck.<msg-id>` will be picked up by linkgit:git-fsck[1], but
-to accept pushes of such data set `receive.fsck.<msg-id>` instead. The
-rest of the documentation discusses `fsck.*` for brevity, but the same
-applies for the corresponding `receive.fsck.*` variables.
+to accept pushes of such data set `receive.fsck.<msg-id>` instead, or
+to clone or fetch it set `fetch.fsck.<msg-id>`. The rest of the
+documentation discusses `fsck.*` for brevity, but the same applies for
+the corresponding `receive.fsck.*` and `fetch.<msg-id>.*`. variables.
 +
 When `fsck.<msg-id>` is set, errors can be switched to warnings and
 vice versa by configuring the `fsck.<msg-id>` setting where the
@@ -1588,8 +1599,8 @@ fsck.skipList::
 	email addresses. Note: corrupt objects cannot be skipped with
 	this setting.
 +
-Like `fsck.<msg-id>` this variable has a corresponding
-`receive.fsck.skipList` variant.
+Like `fsck.<msg-id>` this variable has corresponding
+`receive.fsck.skipList` and `fetch.fsck.skipList` variants.
 
 gc.aggressiveDepth::
 	The depth parameter used in the delta compression
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;
 
 /* Remember to update object flag allocation in object.h */
 #define COMPLETE	(1U << 0)
@@ -935,7 +937,8 @@ static int get_pack(struct fetch_pack_args *args,
 			 */
 			argv_array_push(&cmd.args, "--fsck-objects");
 		else
-			argv_array_push(&cmd.args, "--strict");
+			argv_array_pushf(&cmd.args, "--strict%s",
+					 fsck_msg_types.buf);
 	}
 
 	cmd.in = demux.out;
@@ -1409,6 +1412,31 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
 	return ref;
 }
 
+static int fetch_pack_config_cb(const char *var, const char *value, void *cb)
+{
+	if (strcmp(var, "fetch.fsck.skiplist") == 0) {
+		const char *path;
+
+		if (git_config_pathname(&path, var, value))
+			return 1;
+		strbuf_addf(&fsck_msg_types, "%cskiplist=%s",
+			fsck_msg_types.len ? ',' : '=', path);
+		free((char *)path);
+		return 0;
+	}
+
+	if (skip_prefix(var, "fetch.fsck.", &var)) {
+		if (is_valid_msg_type(var, value))
+			strbuf_addf(&fsck_msg_types, "%c%s=%s",
+				fsck_msg_types.len ? ',' : '=', var, value);
+		else
+			warning("Skipping unknown msg id '%s'", var);
+		return 0;
+	}
+
+	return git_default_config(var, value, cb);
+}
+
 static void fetch_pack_config(void)
 {
 	git_config_get_int("fetch.unpacklimit", &fetch_unpack_limit);
@@ -1417,7 +1445,7 @@ static void fetch_pack_config(void)
 	git_config_get_bool("fetch.fsckobjects", &fetch_fsck_objects);
 	git_config_get_bool("transfer.fsckobjects", &transfer_fsck_objects);
 
-	git_config(git_default_config, NULL);
+	git_config(fetch_pack_config_cb, NULL);
 }
 
 static void fetch_pack_setup(void)
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
+'
+
+
 test_expect_success 'push with receive.fsck.missingEmail=warn' '
 	commit="$(git hash-object -t commit -w --stdin <bogus-commit)" &&
 	git push . $commit:refs/heads/bogus &&
@@ -155,6 +169,27 @@ test_expect_success 'push with receive.fsck.missingEmail=warn' '
 	! grep "missingEmail" act
 '
 
+test_expect_success 'fetch with fetch.fsck.missingEmail=warn' '
+	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.missingEmail warn &&
+	git --git-dir=dst/.git fetch "file://$(pwd)" $refspec >act 2>&1 &&
+	grep "missingEmail" act &&
+	git --git-dir=dst/.git branch -D bogus &&
+	git --git-dir=dst/.git config --add \
+		receive.fsck.missingEmail ignore &&
+	git --git-dir=dst/.git config --add \
+		receive.fsck.badDate warn &&
+	git --git-dir=dst/.git fetch "file://$(pwd)" $refspec >act 2>&1 &&
+	! grep "missingEmail" act
+'
+
 test_expect_success \
 	'receive.fsck.unterminatedHeader=warn triggers error' '
 	rm -rf dst &&
@@ -166,4 +201,15 @@ test_expect_success \
 	grep "Cannot demote unterminatedheader" act
 '
 
+test_expect_success \
+	'fetch.fsck.unterminatedHeader=warn triggers error' '
+	rm -rf dst &&
+	git init dst &&
+	git --git-dir=dst/.git config fetch.fsckobjects true &&
+	git --git-dir=dst/.git config \
+		fetch.fsck.unterminatedheader warn &&
+	test_must_fail git --git-dir=dst/.git fetch "file://$(pwd)" HEAD &&
+	grep "Cannot demote unterminatedheader" act
+'
+
 test_done
-- 
2.17.0.290.gded63e768a

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

[Junio C Hamano]

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.

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.

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

[Ævar Arnfjörð Bjarmason]

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.

Re: [PATCH 0/4] fsck: doc fixes & fetch.fsck.* implementation

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> On Thu, May 24, 2018 at 9:02 PM, Jeff King <peff@peff.net> wrote:
>> On Thu, May 24, 2018 at 07:04:04PM +0200, Ævar Arnfjörð Bjarmason wrote:
>>
>>> That doesn't work, because that's for the server-side, but I need the
>>> fetch.fsck.* that doesn't exist. This works (I'll send a better patch
>>> with tests / docs etc. soon):
>>
>> Yeah, I think this is the right direction.

It seems that this 4-patch series is sufficiently commented and
earlier parts can be further polished soonish using help made by
others.  I need to re-read the last patch (implementation) but I
think this generally is good.

Thanks.

[PATCH 1/4] config doc: don't describe *.fetchObjects twice

[Ævar Arnfjörð Bjarmason]

Change the copy/pasted description of the fetch.fsckObjects and
receive.fsckObjects variables to refer to transfer.fsckObjects
instead.

Let's not duplicate the description of what *.fsckObjects does twice.
instead let's refer to transfer.fsckObjects from both fetch.* and
receive.*.

I don't think this description of it makes much sense, but for now I'm
just moving the existing documentation around. Making it better will
be done in a later patch.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/config.txt | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 84e2891aed..623dffd198 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1422,10 +1422,9 @@ fetch.recurseSubmodules::
 
 fetch.fsckObjects::
 	If it is set to true, git-fetch-pack will check all fetched
-	objects. It will abort in the case of a malformed object or a
-	broken link. The result of an abort are only dangling objects.
-	Defaults to false. If not set, the value of `transfer.fsckObjects`
-	is used instead.
+	objects. See `transfer.fsckObjects` for what's
+	checked. Defaults to false. If not set, the value of
+	`transfer.fsckObjects` is used instead.
 
 fetch.unpackLimit::
 	If the number of objects fetched over the Git native
@@ -2845,10 +2844,9 @@ receive.certNonceSlop::
 
 receive.fsckObjects::
 	If it is set to true, git-receive-pack will check all received
-	objects. It will abort in the case of a malformed object or a
-	broken link. The result of an abort are only dangling objects.
-	Defaults to false. If not set, the value of `transfer.fsckObjects`
-	is used instead.
+	objects. See `transfer.fsckObjects` for what's checked.
+	Defaults to false. If not set, the value of
+	`transfer.fsckObjects` is used instead.
 
 receive.fsck.<msg-id>::
 	When `receive.fsckObjects` is set to true, errors can be switched
@@ -3332,6 +3330,10 @@ transfer.fsckObjects::
 	When `fetch.fsckObjects` or `receive.fsckObjects` are
 	not set, the value of this variable is used instead.
 	Defaults to false.
++
+When set, the fetch or receive will abort in the case of a malformed
+object or a broken link. The result of an abort are only dangling
+objects.
 
 transfer.hideRefs::
 	String(s) `receive-pack` and `upload-pack` use to decide which
-- 
2.17.0.290.gded63e768a

Re: [PATCH 1/4] config doc: don't describe *.fetchObjects twice

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> Change the copy/pasted description of the fetch.fsckObjects and
> receive.fsckObjects variables to refer to transfer.fsckObjects
> instead.
>
> Let's not duplicate the description of what *.fsckObjects does twice.
> instead let's refer to transfer.fsckObjects from both fetch.* and
> receive.*.

The two paragraphs above are duplicating what each other says,
perhaps meant as a half-joke?  Well played if that is the case ;-).

Thanks for polishing this area.

[PATCH 2/4] config doc: unify the description of fsck.* and receive.fsck.*

[Ævar Arnfjörð Bjarmason]

The documentation for the fsck.<msg-id> and receive.fsck.<msg-id>
variables was mostly duplicated in two places, with fsck.<msg-id>
making no mention of the corresponding receive.fsck.<msg-id>, and the
same for fsck.skipList.

I spent quite a lot of time today wondering why setting the
fsck.<msg-id> variant wasn't working to clone a legacy repository (not
that that would have worked anyway, but a subsequent patch implements
fetch.fsck.<msg-id>).

Rectify this situation by describing the feature in general terms
under the fsck.* documentation, and make the receive.fsck.*
documentation refer to those variables instead.

This documentation was initially added in 2becf00ff7 ("fsck: support
demoting errors to warnings", 2015-06-22) and 4b55b9b479 ("fsck:
document the new receive.fsck.<msg-id> options", 2015-06-22).

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/config.txt | 74 ++++++++++++++++++++++------------------
 1 file changed, 41 insertions(+), 33 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 623dffd198..351c541ab8 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1554,23 +1554,41 @@ filter.<driver>.smudge::
 	linkgit:gitattributes[5] for details.
 
 fsck.<msg-id>::
-	Allows overriding the message type (error, warn or ignore) of a
-	specific message ID such as `missingEmail`.
-+
-For convenience, fsck prefixes the error/warning with the message ID,
-e.g.  "missingEmail: invalid author/committer line - missing email" means
-that setting `fsck.missingEmail = ignore` will hide that issue.
-+
-This feature is intended to support working with legacy repositories
-which cannot be repaired without disruptive changes.
+	During fsck git may find issues with legacy data which
+	wouldn't be generated by current versions of git, and which
+	wouldn't be sent over the wire if `transfer.fsckObjects` was
+	set. This feature is intended to support working with legacy
+	repositories containing such data.
++
+Setting `fsck.<msg-id>` will be picked up by linkgit:git-fsck[1], but
+to accept pushes of such data set `receive.fsck.<msg-id>` instead. The
+rest of the documentation discusses `fsck.*` for brevity, but the same
+applies for the corresponding `receive.fsck.*` variables.
++
+When `fsck.<msg-id>` is set, errors can be switched to warnings and
+vice versa by configuring the `fsck.<msg-id>` setting where the
+`<msg-id>` is the fsck message ID and the value is one of `error`,
+`warn` or `ignore`. For convenience, fsck prefixes the error/warning
+with the message ID, e.g. "missingEmail: invalid author/committer line
+- missing email" means that setting `fsck.missingEmail = ignore` will
+hide that issue.
++
+Depending on the circumstances it might be better to use
+`fsck.skipList` instead to explicitly whitelist those objects that
+have issues, otherwise new occurrences of the same issue will be
+hidden going forward, although that's unlikely to happen in practice
+unless someone is being deliberately malicious.
 
 fsck.skipList::
-	The path to a sorted list of object names (i.e. one SHA-1 per
-	line) that are known to be broken in a non-fatal way and should
-	be ignored. This feature is useful when an established project
-	should be accepted despite early commits containing errors that
-	can be safely ignored such as invalid committer email addresses.
-	Note: corrupt objects cannot be skipped with this setting.
+	Like `fsck.<msg-id>` this variable has a corresponding
+	`receive.fsck.skipList` variant.
++
+The path to a sorted list of object names (i.e. one SHA-1 per line)
+that are known to be broken in a non-fatal way and should be
+ignored. This feature is useful when an established project should be
+accepted despite early commits containing errors that can be safely
+ignored such as invalid committer email addresses. Note: corrupt
+objects cannot be skipped with this setting.
 
 gc.aggressiveDepth::
 	The depth parameter used in the delta compression
@@ -2849,26 +2867,16 @@ receive.fsckObjects::
 	`transfer.fsckObjects` is used instead.
 
 receive.fsck.<msg-id>::
-	When `receive.fsckObjects` is set to true, errors can be switched
-	to warnings and vice versa by configuring the `receive.fsck.<msg-id>`
-	setting where the `<msg-id>` is the fsck message ID and the value
-	is one of `error`, `warn` or `ignore`. For convenience, fsck prefixes
-	the error/warning with the message ID, e.g. "missingEmail: invalid
-	author/committer line - missing email" means that setting
-	`receive.fsck.missingEmail = ignore` will hide that issue.
-+
-This feature is intended to support working with legacy repositories
-which would not pass pushing when `receive.fsckObjects = true`, allowing
-the host to accept repositories with certain known issues but still catch
-other issues.
+	Acts like `fsck.<msg-id>`, but is used by
+	linkgit:git-receive-pack[1] instead of
+	linkgit:git-fsck[1]. See the `fsck.<msg-id>` documentation for
+	details.
 
 receive.fsck.skipList::
-	The path to a sorted list of object names (i.e. one SHA-1 per
-	line) that are known to be broken in a non-fatal way and should
-	be ignored. This feature is useful when an established project
-	should be accepted despite early commits containing errors that
-	can be safely ignored such as invalid committer email addresses.
-	Note: corrupt objects cannot be skipped with this setting.
+	Acts like `fsck.skipList`, but is used by
+	linkgit:git-receive-pack[1] instead of
+	linkgit:git-fsck[1]. See the `fsck.skipList` documentation for
+	details.
 
 receive.keepAlive::
 	After receiving the pack from the client, `receive-pack` may
-- 
2.17.0.290.gded63e768a

Re: [PATCH 2/4] config doc: unify the description of fsck.* and receive.fsck.*

[Eric Sunshine]

On Thu, May 24, 2018 at 3:35 PM, Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> The documentation for the fsck.<msg-id> and receive.fsck.<msg-id>
> variables was mostly duplicated in two places, with fsck.<msg-id>
> making no mention of the corresponding receive.fsck.<msg-id>, and the
> same for fsck.skipList.
> [...]
> Rectify this situation by describing the feature in general terms
> under the fsck.* documentation, and make the receive.fsck.*
> documentation refer to those variables instead.
> [...]
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> @@ -1554,23 +1554,41 @@ filter.<driver>.smudge::
>  fsck.skipList::
> -       The path to a sorted list of object names (i.e. one SHA-1 per
> -       line) that are known to be broken in a non-fatal way and should
> -       be ignored. This feature is useful when an established project
> -       should be accepted despite early commits containing errors that
> -       can be safely ignored such as invalid committer email addresses.
> -       Note: corrupt objects cannot be skipped with this setting.
> +       Like `fsck.<msg-id>` this variable has a corresponding
> +       `receive.fsck.skipList` variant.
> ++
> +The path to a sorted list of object names (i.e. one SHA-1 per line)
> +that are known to be broken in a non-fatal way and should be
> +ignored. This feature is useful when an established project should be
> +accepted despite early commits containing errors that can be safely
> +ignored such as invalid committer email addresses. Note: corrupt
> +objects cannot be skipped with this setting.

Nit: This organization seems backward. Typically, one would describe
what the option is for and then add the incidental note ("Like
fsck.<...>, this variable...") at the end. It's not clear why this
patch demotes the description to a secondary paragraph and considers
the incidental note as primary.

Re: [PATCH 2/4] config doc: unify the description of fsck.* and receive.fsck.*

[Ævar Arnfjörð Bjarmason]

On Thu, May 24 2018, Eric Sunshine wrote:

> On Thu, May 24, 2018 at 3:35 PM, Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
>> The documentation for the fsck.<msg-id> and receive.fsck.<msg-id>
>> variables was mostly duplicated in two places, with fsck.<msg-id>
>> making no mention of the corresponding receive.fsck.<msg-id>, and the
>> same for fsck.skipList.
>> [...]
>> Rectify this situation by describing the feature in general terms
>> under the fsck.* documentation, and make the receive.fsck.*
>> documentation refer to those variables instead.
>> [...]
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> ---
>> diff --git a/Documentation/config.txt b/Documentation/config.txt
>> @@ -1554,23 +1554,41 @@ filter.<driver>.smudge::
>>  fsck.skipList::
>> -       The path to a sorted list of object names (i.e. one SHA-1 per
>> -       line) that are known to be broken in a non-fatal way and should
>> -       be ignored. This feature is useful when an established project
>> -       should be accepted despite early commits containing errors that
>> -       can be safely ignored such as invalid committer email addresses.
>> -       Note: corrupt objects cannot be skipped with this setting.
>> +       Like `fsck.<msg-id>` this variable has a corresponding
>> +       `receive.fsck.skipList` variant.
>> ++
>> +The path to a sorted list of object names (i.e. one SHA-1 per line)
>> +that are known to be broken in a non-fatal way and should be
>> +ignored. This feature is useful when an established project should be
>> +accepted despite early commits containing errors that can be safely
>> +ignored such as invalid committer email addresses. Note: corrupt
>> +objects cannot be skipped with this setting.
>
> Nit: This organization seems backward. Typically, one would describe
> what the option is for and then add the incidental note ("Like
> fsck.<...>, this variable...") at the end. It's not clear why this
> patch demotes the description to a secondary paragraph and considers
> the incidental note as primary.

I could change it like that. I was thinking that later in the series
fetch.fsck.* is going to be first in the file, and then the user is told
to look at this variable, so it made sense to note from the outset that
we're describing several variables here.

What do you think?

Re: [PATCH 2/4] config doc: unify the description of fsck.* and receive.fsck.*

[Eric Sunshine]

On Thu, May 24, 2018 at 4:12 PM, Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> On Thu, May 24 2018, Eric Sunshine wrote:
>> On Thu, May 24, 2018 at 3:35 PM, Ævar Arnfjörð Bjarmason
>> <avarab@gmail.com> wrote:
>>>  fsck.skipList::
>>> +       Like `fsck.<msg-id>` this variable has a corresponding
>>> +       `receive.fsck.skipList` variant.
>>> ++
>>> +The path to a sorted list of object names (i.e. one SHA-1 per line)
>>> +that are known to be broken in a non-fatal way and should be
>>> +ignored. This feature is useful when an established project should be
>>> +accepted despite early commits containing errors that can be safely
>>> +ignored such as invalid committer email addresses. Note: corrupt
>>> +objects cannot be skipped with this setting.
>>
>> Nit: This organization seems backward. Typically, one would describe
>> what the option is for and then add the incidental note ("Like
>> fsck.<...>, this variable...") at the end. It's not clear why this
>> patch demotes the description to a secondary paragraph and considers
>> the incidental note as primary.
>
> I could change it like that. I was thinking that later in the series
> fetch.fsck.* is going to be first in the file, and then the user is told
> to look at this variable, so it made sense to note from the outset that
> we're describing several variables here.
> What do you think?

I see where you're coming from, however, I would think that readers
arriving at this topic (generally) do so as a result of actively
looking for it (as opposed to happening upon it), in which case they
probably are directly seeking information about it; the incidental
information is just a bonus after reading what they came to learn.

Anyhow, I don't care too strongly about it (it was just a "nit", after all).

Re: [PATCH 2/4] config doc: unify the description of fsck.* and receive.fsck.*

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> I see where you're coming from, however, I would think that readers
> arriving at this topic (generally) do so as a result of actively
> looking for it (as opposed to happening upon it), in which case they
> probably are directly seeking information about it; the incidental
> information is just a bonus after reading what they came to learn.

Yup.  That matches my mental model as well.

[PATCH 3/4] config doc: elaborate on what transfer.fsckObjects does

[Ævar Arnfjörð Bjarmason]

The existing documentation led the user to believe that all we were
doing were basic readability sanity checks, but that hasn't been true
for a very long time. Update the description to match reality, and
note the caveat that there's a quarantine for accepting pushes, but
not for fetching.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/config.txt | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 351c541ab8..124f7a187c 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -3339,9 +3339,19 @@ transfer.fsckObjects::
 	not set, the value of this variable is used instead.
 	Defaults to false.
 +
-When set, the fetch or receive will abort in the case of a malformed
-object or a broken link. The result of an abort are only dangling
-objects.
+When set, the fetch receive will abort in the case of a malformed
+object or a link to a nonexisting object. In addition, various other
+issues are checked for, including legacy issues (see `fsck.<msg-id>`),
+and potential security issues like there being a `.GIT` directory (see
+the release notes for v2.2.1 for details). Other sanity and security
+checks may be added in future releases.
++
+On the receiving side failing fsckObjects will make those objects
+unreachable, see "QUARANTINE ENVIRONMENT" in
+linkgit:git-receive-pack[1]. On the fetch side the malformed objects
+will instead be left unreferenced in the repository. That's considered
+a bug, and hopefully future git release will implement a quarantine
+for the "fetch" side as well.
 
 transfer.hideRefs::
 	String(s) `receive-pack` and `upload-pack` use to decide which
-- 
2.17.0.290.gded63e768a

Re: [PATCH 3/4] config doc: elaborate on what transfer.fsckObjects does

[Eric Sunshine]

On Thu, May 24, 2018 at 3:35 PM, Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> The existing documentation led the user to believe that all we were
> doing were basic readability sanity checks, but that hasn't been true
> for a very long time. Update the description to match reality, and
> note the caveat that there's a quarantine for accepting pushes, but
> not for fetching.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> @@ -3339,9 +3339,19 @@ transfer.fsckObjects::
> -When set, the fetch or receive will abort in the case of a malformed
> -object or a broken link. The result of an abort are only dangling
> -objects.
> +When set, the fetch receive will abort in the case of a malformed

"fetch receive"? Did you mean "fetch or receive" (like the original)?

> +object or a link to a nonexisting object. In addition, various other

s/nonexisting/nonexistent/

> +issues are checked for, including legacy issues (see `fsck.<msg-id>`),
> +and potential security issues like there being a `.GIT` directory (see

s/there being/existence of/

> +the release notes for v2.2.1 for details). Other sanity and security
> +checks may be added in future releases.
> ++
> +On the receiving side failing fsckObjects will make those objects

s/side/&,/

> +unreachable, see "QUARANTINE ENVIRONMENT" in
> +linkgit:git-receive-pack[1]. On the fetch side the malformed objects

s/side/&,/

> +will instead be left unreferenced in the repository. That's considered
> +a bug, and hopefully future git release will implement a quarantine
> +for the "fetch" side as well.

If this was a "BUGS" section in a man-page, the above might be less
scary. In this context, however, I wonder if it makes sense to tone it
down a bit:

    On the fetch side, malformed objects will instead be left
    unreferenced in the repository. (However, in the future, such
    objects may be quarantined for "fetch", as well.)

Re: [PATCH 3/4] config doc: elaborate on what transfer.fsckObjects does

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

>> +will instead be left unreferenced in the repository. That's considered
>> +a bug, and hopefully future git release will implement a quarantine
>> +for the "fetch" side as well.
>
> If this was a "BUGS" section in a man-page, the above might be less
> scary. In this context, however, I wonder if it makes sense to tone it
> down a bit:
>
>     On the fetch side, malformed objects will instead be left
>     unreferenced in the repository. (However, in the future, such
>     objects may be quarantined for "fetch", as well.)

I had an impression that nobody else sayd it is considered as a
bug.  Do we need to say it in this series?  I'd rather not--with or
without such a future modification (or lack of plan thereof),
teaching the fetch side to pay attention to the various fsck tweaks
is an improvement.

Re: [PATCH 3/4] config doc: elaborate on what transfer.fsckObjects does

[Ævar Arnfjörð Bjarmason]

On Fri, May 25 2018, Junio C Hamano wrote:

> Eric Sunshine <sunshine@sunshineco.com> writes:
>
>>> +will instead be left unreferenced in the repository. That's considered
>>> +a bug, and hopefully future git release will implement a quarantine
>>> +for the "fetch" side as well.
>>
>> If this was a "BUGS" section in a man-page, the above might be less
>> scary. In this context, however, I wonder if it makes sense to tone it
>> down a bit:
>>
>>     On the fetch side, malformed objects will instead be left
>>     unreferenced in the repository. (However, in the future, such
>>     objects may be quarantined for "fetch", as well.)
>
> I had an impression that nobody else sayd it is considered as a
> bug.  Do we need to say it in this series?  I'd rather not--with or
> without such a future modification (or lack of plan thereof),
> teaching the fetch side to pay attention to the various fsck tweaks
> is an improvement.

I changed this in v2 to tone it down, but given what Jeff's mentioned in
https://public-inbox.org/git/20180531060259.GE17344@sigill.intra.peff.net/
I'm inclined to bring back some stronger wording for it. Something like:

    Due to the non-quarantine nature of the fetch.fsckObjects
    implementation it can not be relied upon like receive.fsckObjects
    can. As objects are unpacked they're written to the object store, so
    there can be cases where malicious objects get introduced even
    though the "fetch" fail, only to have a subsequent "fetch" succeed
    because only new incoming objects are checked, not those that have
    already been written to the store.

    This is considered a bug and will likely be fixed in future versions
    of git. For now the paranoid need to find some way to emulate the
    quarantine environment if they'd like the same protection as
    "push". E.g. in the case of an internal mirror do the mirroring in
    two steps, one to fetch the untrusted objects, and then do a second
    "push" (which will use the quarantine) to another internal repo, and
    have internal clients consume this pushed-to repository, or embargo
    internal fetches and only allow them once a full "fsck" has run (and
    no new fetches have happened in the meantime).

[PATCH 4/4] fetch: implement fetch.fsck.*

[Ævar Arnfjörð Bjarmason]

Implement support for fetch.fsck.* corresponding with the existing
receive.fsck.*. This allows for pedantically cloning repositories with
specific issues without turning off fetch.fsckObjects.

One such repository is https://github.com/robbyrussell/oh-my-zsh.git
which before this change will emit this error when cloned with
fetch.fsckObjects:

    error: object 2b7227859263b6aabcc28355b0b994995b7148b6: zeroPaddedFilemode: contains zero-padded file modes
    fatal: Error in object
    fatal: index-pack failed

Now with fetch.fsck.zeroPaddedFilemode=warn we'll warn about that
issue, but the clone will succeed:

    warning: object 2b7227859263b6aabcc28355b0b994995b7148b6: zeroPaddedFilemode: contains zero-padded file modes
    warning: object a18c4d13c2a5fa2d4ecd5346c50e119b999b807d: zeroPaddedFilemode: contains zero-padded file modes
    warning: object 84df066176c8da3fd59b13731a86d90f4f1e5c9d: zeroPaddedFilemode: contains zero-padded file modes

The motivation for this is to be able to turn on fetch.fsckObjects
globally across a fleet of computers but still be able to manually
clone various legacy repositories by either white-listing specific
issues, or better yet whitelist specific objects.

The use of --git-dir=* instead of -C in the tests could be considered
somewhat archaic, but the tests I'm adding here are duplicating the
corresponding receive.* tests with as few changes as possible.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/config.txt        | 21 +++++++++++----
 fetch-pack.c                    | 32 +++++++++++++++++++++--
 t/t5504-fetch-receive-strict.sh | 46 +++++++++++++++++++++++++++++++++
 3 files changed, 92 insertions(+), 7 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 124f7a187c..af6187f6b5 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1426,6 +1426,16 @@ fetch.fsckObjects::
 	checked. Defaults to false. If not set, the value of
 	`transfer.fsckObjects` is used instead.
 
+fetch.fsck.<msg-id>::
+	Acts like `fsck.<msg-id>`, but is used by
+	linkgit:git-fetch-pack[1] instead of linkgit:git-fsck[1]. See
+	the `fsck.<msg-id>` documentation for details.
+
+fetch.fsck.skipList::
+	Acts like `fsck.skipList`, but is used by
+	linkgit:git-fetch-pack[1] instead of linkgit:git-fsck[1]. See
+	the `fsck.skipList` documentation for details.
+
 fetch.unpackLimit::
 	If the number of objects fetched over the Git native
 	transfer is below this
@@ -1561,9 +1571,10 @@ fsck.<msg-id>::
 	repositories containing such data.
 +
 Setting `fsck.<msg-id>` will be picked up by linkgit:git-fsck[1], but
-to accept pushes of such data set `receive.fsck.<msg-id>` instead. The
-rest of the documentation discusses `fsck.*` for brevity, but the same
-applies for the corresponding `receive.fsck.*` variables.
+to accept pushes of such data set `receive.fsck.<msg-id>` instead, or
+to clone or fetch it set `fetch.fsck.<msg-id>`. The rest of the
+documentation discusses `fsck.*` for brevity, but the same applies for
+the corresponding `receive.fsck.*` and `fetch.<msg-id>.*`. variables.
 +
 When `fsck.<msg-id>` is set, errors can be switched to warnings and
 vice versa by configuring the `fsck.<msg-id>` setting where the
@@ -1580,8 +1591,8 @@ hidden going forward, although that's unlikely to happen in practice
 unless someone is being deliberately malicious.
 
 fsck.skipList::
-	Like `fsck.<msg-id>` this variable has a corresponding
-	`receive.fsck.skipList` variant.
+	Like `fsck.<msg-id>` this variable has corresponding
+	`receive.fsck.skipList` and `fetch.fsck.skipList` variants.
 +
 The path to a sorted list of object names (i.e. one SHA-1 per line)
 that are known to be broken in a non-fatal way and should be
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;
 
 /* Remember to update object flag allocation in object.h */
 #define COMPLETE	(1U << 0)
@@ -935,7 +937,8 @@ static int get_pack(struct fetch_pack_args *args,
 			 */
 			argv_array_push(&cmd.args, "--fsck-objects");
 		else
-			argv_array_push(&cmd.args, "--strict");
+			argv_array_pushf(&cmd.args, "--strict%s",
+					 fsck_msg_types.buf);
 	}
 
 	cmd.in = demux.out;
@@ -1409,6 +1412,31 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
 	return ref;
 }
 
+static int fetch_pack_config_cb(const char *var, const char *value, void *cb)
+{
+	if (strcmp(var, "fetch.fsck.skiplist") == 0) {
+		const char *path;
+
+		if (git_config_pathname(&path, var, value))
+			return 1;
+		strbuf_addf(&fsck_msg_types, "%cskiplist=%s",
+			fsck_msg_types.len ? ',' : '=', path);
+		free((char *)path);
+		return 0;
+	}
+
+	if (skip_prefix(var, "fetch.fsck.", &var)) {
+		if (is_valid_msg_type(var, value))
+			strbuf_addf(&fsck_msg_types, "%c%s=%s",
+				fsck_msg_types.len ? ',' : '=', var, value);
+		else
+			warning("Skipping unknown msg id '%s'", var);
+		return 0;
+	}
+
+	return git_default_config(var, value, cb);
+}
+
 static void fetch_pack_config(void)
 {
 	git_config_get_int("fetch.unpacklimit", &fetch_unpack_limit);
@@ -1417,7 +1445,7 @@ static void fetch_pack_config(void)
 	git_config_get_bool("fetch.fsckobjects", &fetch_fsck_objects);
 	git_config_get_bool("transfer.fsckobjects", &transfer_fsck_objects);
 
-	git_config(git_default_config, NULL);
+	git_config(fetch_pack_config_cb, NULL);
 }
 
 static void fetch_pack_setup(void)
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
+'
+
+
 test_expect_success 'push with receive.fsck.missingEmail=warn' '
 	commit="$(git hash-object -t commit -w --stdin <bogus-commit)" &&
 	git push . $commit:refs/heads/bogus &&
@@ -155,6 +169,27 @@ test_expect_success 'push with receive.fsck.missingEmail=warn' '
 	! grep "missingEmail" act
 '
 
+test_expect_success 'fetch with fetch.fsck.missingEmail=warn' '
+	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.missingEmail warn &&
+	git --git-dir=dst/.git fetch "file://$(pwd)" $refspec >act 2>&1 &&
+	grep "missingEmail" act &&
+	git --git-dir=dst/.git branch -D bogus &&
+	git --git-dir=dst/.git config --add \
+		receive.fsck.missingEmail ignore &&
+	git --git-dir=dst/.git config --add \
+		receive.fsck.badDate warn &&
+	git --git-dir=dst/.git fetch "file://$(pwd)" $refspec >act 2>&1 &&
+	! grep "missingEmail" act
+'
+
 test_expect_success \
 	'receive.fsck.unterminatedHeader=warn triggers error' '
 	rm -rf dst &&
@@ -166,4 +201,15 @@ test_expect_success \
 	grep "Cannot demote unterminatedheader" act
 '
 
+test_expect_success \
+	'fetch.fsck.unterminatedHeader=warn triggers error' '
+	rm -rf dst &&
+	git init dst &&
+	git --git-dir=dst/.git config fetch.fsckobjects true &&
+	git --git-dir=dst/.git config \
+		fetch.fsck.unterminatedheader warn &&
+	test_must_fail git --git-dir=dst/.git fetch "file://$(pwd)" HEAD &&
+	grep "Cannot demote unterminatedheader" act
+'
+
 test_done
-- 
2.17.0.290.gded63e768a

Re: [PATCH 4/4] fetch: implement fetch.fsck.*

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

>  fsck.skipList::
> -	Like `fsck.<msg-id>` this variable has a corresponding
> -	`receive.fsck.skipList` variant.
> +	Like `fsck.<msg-id>` this variable has corresponding
> +	`receive.fsck.skipList` and `fetch.fsck.skipList` variants.
>  +
>  The path to a sorted list of object names (i.e. one SHA-1 per line)
>  that are known to be broken in a non-fatal way and should be

I think I've said this already, but I tend to agree with Eric that
this is the other way around.  Perhaps that is because I consider
fsck.<var> the most basic one people would want to understand first,
and then corresponding <command>.fsck.<var> a mere specialization of
it.  So "Here is what fsck.skipList does" followed by "By the way,
you can configure it only for the (internal) fsck run during the
object transfer with transfer.fsck.skipList" feels more natural
presentation order.

> 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;
>  
>  /* Remember to update object flag allocation in object.h */
>  #define COMPLETE	(1U << 0)
> @@ -935,7 +937,8 @@ static int get_pack(struct fetch_pack_args *args,
>  			 */
>  			argv_array_push(&cmd.args, "--fsck-objects");
>  		else
> -			argv_array_push(&cmd.args, "--strict");
> +			argv_array_pushf(&cmd.args, "--strict%s",
> +					 fsck_msg_types.buf);

This made a reader wonder what fsck_msg_types.buf[] has.

It is empty or a comma separated list of things, prefixed with =,
that is constructed by repeated calls to fetch_pack_config_cb(), so
syntactically what we feed index-pack looks like "--strict",
"--strict=thing", or "--strict=thing1,thing2,....,thingn".  And each
"thing" is either "<msgtype>=<value>" or "skiplist=<objectname>".

The buffer that has both msgtype specification and object name
should not be called fsck_msg_types, though.  It is probably
fsck_exception or something.

> +		strbuf_addf(&fsck_msg_types, "%cskiplist=%s",
> +			fsck_msg_types.len ? ',' : '=', path);
> +		free((char *)path);
> +		return 0;
> +	}
> +
> +	if (skip_prefix(var, "fetch.fsck.", &var)) {
> +		if (is_valid_msg_type(var, value))
> +			strbuf_addf(&fsck_msg_types, "%c%s=%s",
> +				fsck_msg_types.len ? ',' : '=', var, value);
> +		else
> +			warning("Skipping unknown msg id '%s'", var);
> +		return 0;
> +	}
> +
> +	return git_default_config(var, value, cb);
> +}

Re: BUG: No way to set fsck.<msg-id> when cloning

[Jeff King]

On Thu, May 24, 2018 at 05:25:29PM +0200, Ævar Arnfjörð Bjarmason wrote:

> When I do:
> 
>     git -c fetch.fsckObjects=true clone git@github.com:robbyrussell/oh-my-zsh.git
> 
> I get:
> 
>     error: object 2b7227859263b6aabcc28355b0b994995b7148b6: zeroPaddedFilemode: contains zero-padded file modes
>     fatal: Error in object
>     fatal: index-pack failed
> 
> The docs (https://git-scm.com/docs/git-config#git-config-fsckltmsg-idgt)
> say you can override this with -c fsck.zeroPaddedFilemode = ignore, but
> I see in builtin/fsck.c that just fsck_config() knows about this, and
> indeed this works *after* you clone the repo when you use 'git fsck'.
> 
> I don't have time to fix this now, but what's the best approach here?
> Make all the relevant commands copy what fsck_config() is doing, or
> should fsck_object() be lazily looking up this config by itself?

I think the relevant commands need to do it themselves. We already have
receive.fsck.*. I don't think there's an equivalent for fetching, but
there probably should be. But fsck_object() doesn't have the context to
know which set should be used.

-Peff

BUG: No way to set fsck.<msg-id> when cloning

[Thomas Braun]

Am 24.05.2018 um 17:25 schrieb Ævar Arnfjörð Bjarmason:
> When I do:
> 
>     git -c fetch.fsckObjects=true clone git@github.com:robbyrussell/oh-my-zsh.git
> 
> I get:
> 
>     error: object 2b7227859263b6aabcc28355b0b994995b7148b6: zeroPaddedFilemode: contains zero-padded file modes
>     fatal: Error in object
>     fatal: index-pack failed
>
> [...]

Doing

git clone --config transfer.fsckobjects=false --config
receive.fsckobjects=false --config fetch.fsckobjects=false
git@github.com:robbyrussell/oh-my-zsh.git

does the trick here (stolen from [1]).

$ git --version
git version 2.17.0.windows.1

I don't know why though.

[1]:
https://github.com/michaeljones/breathe/issues/340#issuecomment-390775142

Re: BUG: No way to set fsck.<msg-id> when cloning

[Ævar Arnfjörð Bjarmason]

On Thu, May 24 2018, Thomas Braun wrote:

> Am 24.05.2018 um 17:25 schrieb Ævar Arnfjörð Bjarmason:
>> When I do:
>>
>>     git -c fetch.fsckObjects=true clone git@github.com:robbyrussell/oh-my-zsh.git
>>
>> I get:
>>
>>     error: object 2b7227859263b6aabcc28355b0b994995b7148b6: zeroPaddedFilemode: contains zero-padded file modes
>>     fatal: Error in object
>>     fatal: index-pack failed
>>
>> [...]
>
> Doing
>
> git clone --config transfer.fsckobjects=false --config
> receive.fsckobjects=false --config fetch.fsckobjects=false
> git@github.com:robbyrussell/oh-my-zsh.git
>
> does the trick here (stolen from [1]).
>
> $ git --version
> git version 2.17.0.windows.1
>
> I don't know why though.

Yeah, because it entirely turns off fsck on fetch, and we didn't have
any way to selectively disable just some stuff until the patch series I
just sent
(https://public-inbox.org/git/20180524193516.28713-1-avarab@gmail.com/)

> [1]:
> https://github.com/michaeljones/breathe/issues/340#issuecomment-390775142