BUG: No way to set fsck.<msg-id> when cloning
When I do:
git -c fetch.fsckObjects=true clone [email protected]: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?
Quoted 10 lines · Ævar Arnfjörð Bjarmason
When I do:
git -c fetch.fsckObjects=true clone [email protected]:robbyrussell/oh-my-zsh.git
I get:
error: object
2b7227859263b6aabcc28355b0b994995b7148b6: zeroPaddedFilemode: contains zero-padded file modes
fatal: Error in object
fatal: index-pack failed
Quoted 8 lines
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 inbuiltin/fsck.cthat justfsck_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 shouldfsck_object()be lazily looking up this config by itself?
Apparently someone reported this earlier[0]. Johannes replied:
Quoted 12 lines
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>::
Whenreceive.fsckObjectsis set to true, errors can be switched to warnings and vice versa by configuring thereceive.fsck.<msg-id>setting where the<msg-id>is the fsck message ID and the value is one oferror,warnorignore. For convenience, fsck prefixes the error/warning with the message ID, e.g. "missingEmail: invalid author/committer line - missing email" means that settingreceive.fsck.missingEmail = ignorewill hide that issue.
Quoted 2 lines
In your case, use receive.fsck.zeroPaddedFilemode=ignore=warn (or =ignore).
Hope this helps, Kevin.
Quoted 34 lines · Kevin Daudt
Quoted 10 lines · Ævar Arnfjörð Bjarmason
When I do:
git -c fetch.fsckObjects=true clone [email protected]:robbyrussell/oh-my-zsh.git
I get:
error: object
2b7227859263b6aabcc28355b0b994995b7148b6: zeroPaddedFilemode: contains zero-padded file modes
fatal: Error in object
fatal: index-pack failedQuoted 8 lines
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 inbuiltin/fsck.cthat justfsck_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 shouldfsck_object()be lazily looking up this config by itself?Apparently someone reported this earlier[0]. Johannes replied:
Quoted 12 lines
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>::
Whenreceive.fsckObjectsis set to true, errors can be switched to warnings and vice versa by configuring thereceive.fsck.<msg-id>setting where the<msg-id>is the fsck message ID and the value is one oferror,warnorignore. For convenience, fsck prefixes the error/warning with the message ID, e.g. "missingEmail: invalid author/committer line - missing email" means that settingreceive.fsck.missingEmail = ignorewill hide that issue.
Quoted 6 lines
Quoted 2 lines
In your case, use receive.fsck.zeroPaddedFilemode=ignore=warn (or =ignore).
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):
fetch-pack.c +30 -2
@@ -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 )
Quoted 3 lines · Ævar Arnfjörð Bjarmason
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.
Quoted 11 lines
fetch-pack.c +11
-0+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 ;
fetch-pack.c static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args, · line 1435
Quoted 10 lines
fetch-pack.c +10
-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
cover fsck: doc fixes & fetch.fsck.* implementation
Quoted 26 lines · Jeff King
Quoted 3 lines · Ævar Arnfjörð Bjarmason
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.
Quoted 18 lines
fetch-pack.c +18
-0+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
Quoted 8 lines
Quoted 3 lines
+
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...
Quoted 2 lines
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.
Quoted 3 lines
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(-)
Signature · 1 line
2.17.0.290.gded63e768a
PATCH 1/4 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 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.
--- Documentation/config.txt | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-)
Documentation/config.txt +10 -8
@@ -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
Signature · 1 line
2.17.0.290.gded63e768a
Re: [PATCH 1/4] config doc: don't describe *.fetchObjects twice
Quoted 7 lines · Æ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.*.
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.*
In reply to Ævar Arnfjörð BjarmasonThe 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).
--- Documentation/config.txt | 74 ++++++++++++++++++++++------------------ 1 file changed, 41 insertions(+), 33 deletions(-)
Documentation/config.txt +41 -33
@@ -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
Signature · 1 line
2.17.0.290.gded63e768a
Re: [PATCH 2/4] config doc: unify the description of fsck.* and receive.fsck.*
Quoted 19 lines · Æ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.
[...]
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 <[email protected]>---
Documentation/config.txt +0
-5@@ -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.
Documentation/config.txt filter.<driver>.smudge:: · line 1563
Quoted 10 lines
Documentation/config.txt +9
-1- 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.*
Quoted 29 lines · Eric Sunshine
Quoted 27 lines · Æ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.
[...]
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 <[email protected]>---
Documentation/config.txt +7
-6@@ -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
Quoted 8 lines
Documentation/config.txt filter.<driver>.smudge:: · line 1591
Quoted 2 lines
Documentation/config.txt +2
-0+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.*
Quoted 20 lines · Ævar Arnfjörð Bjarmason
Quoted 18 lines · Eric Sunshine
Documentation/config.txt filter.<driver>.smudge:: · line 1591
Quoted 10 lines · Ævar Arnfjörð Bjarmason
fsck.skipList::
Documentation/config.txt +9
-0+ 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.
Quoted 5 lines
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.*
Quoted 5 lines · Eric Sunshine
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
In reply to Ævar Arnfjörð BjarmasonThe 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.
--- Documentation/config.txt | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-)
Documentation/config.txt +13 -3
@@ -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
Signature · 1 line
2.17.0.290.gded63e768a
Re: [PATCH 3/4] config doc: elaborate on what transfer.fsckObjects does
Quoted 10 lines · Æ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 <[email protected]>---
Documentation/config.txt transfer.fsckObjects:: · line 3339
Quoted 4 lines
Documentation/config.txt +1
-3@@ -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)?
Documentation/config.txt transfer.fsckObjects:: · line 3343
Quoted 1 line
Documentation/config.txt +1
-0+object or a link to a nonexisting object. In addition, various other
s/nonexisting/nonexistent/
Documentation/config.txt transfer.fsckObjects:: · line 3345
Quoted 2 lines
Documentation/config.txt +2
-0+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/
Documentation/config.txt transfer.fsckObjects:: · line 3349
Quoted 4 lines
Documentation/config.txt +4
-0+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/&,/
Documentation/config.txt transfer.fsckObjects:: · line 3351
Quoted 2 lines
Documentation/config.txt +2
-0+unreachable, see "QUARANTINE ENVIRONMENT" in +linkgit:git-receive-pack[1]. On the fetch side the malformed objects
s/side/&,/
Documentation/config.txt transfer.fsckObjects:: · line 3354
Quoted 3 lines
Documentation/config.txt +3
-0+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
Quoted 11 lines · Eric Sunshine
Documentation/config.txt transfer.fsckObjects:: · line 3354
Quoted 3 lines
Documentation/config.txt +3
-0+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
Quoted 14 lines · Junio C Hamano
Quoted 11 lines · Eric Sunshine
Documentation/config.txt transfer.fsckObjects:: · line 3354
Quoted 3 lines
Documentation/config.txt +3
-0+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.)
Quoted 5 lines
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/[email protected]/
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.*
In reply to Ævar Arnfjörð BjarmasonImplement 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 modesThe 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.
--- Documentation/config.txt | 21 +++++++++++---- fetch-pack.c | 32 +++++++++++++++++++++-- t/t5504-fetch-receive-strict.sh | 46 +++++++++++++++++++++++++++++++++ 3 files changed, 92 insertions(+), 7 deletions(-)
Documentation/config.txt +16 -5
@@ -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
fetch-pack.c +30 -2
@@ -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 )
t/t5504-fetch-receive-strict.sh +46 -0
62 lines not on this page · raw message
Signature · 1 line
2.17.0.290.gded63e768a
Re: [PATCH 4/4] fetch: implement fetch.fsck.*
Documentation/config.txt hidden going forward, although that's unlikely to happen in practice · line 1598
Quoted 8 lines · Ævar Arnfjörð Bjarmason
fsck.skipList::
Documentation/config.txt +2
-2- 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.
Quoted 21 lines
fetch-pack.c +2
-0@@ -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)
fetch-pack.c static int get_pack(struct fetch_pack_args *args, · line 941
Quoted 6 lines
fetch-pack.c +2
-1@@ -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.
Quoted 17 lines
fetch-pack.c +17
-0+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); +}
v2 cover fsck: doc fixes & fetch.fsck.* implementation
In reply to Ævar Arnfjörð BjarmasonThis 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 <[email protected]>
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(-)
Signature · 1 line
2.17.0.290.gded63e768a
PATCH v2 1/5 config doc: don't describe *.fetchObjects twice
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.
--- Documentation/config.txt | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-)
Documentation/config.txt +10 -8
@@ -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
Signature · 1 line
2.17.0.290.gded63e768a
Re: [PATCH v2 1/5] config doc: don't describe *.fetchObjects twice
Quoted 1 line · Ævar Arnfjörð Bjarmason
Let's not duplicate the description of what *.fsckObjects does twice.
Nit: "duplicate" and "twice" smell redundant.
Quoted 2 lines
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.)
Quoted 5 lines
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 <[email protected]>
PATCH v2 2/5 config doc: unify the description of fsck.* and receive.fsck.*
In reply to Ævar Arnfjörð BjarmasonThe 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).
--- Documentation/config.txt | 73 ++++++++++++++++++++++------------------ 1 file changed, 41 insertions(+), 32 deletions(-)
Documentation/config.txt +41 -32
@@ -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
Signature · 1 line
2.17.0.290.gded63e768a
Re: [PATCH v2 2/5] config doc: unify the description of fsck.* and receive.fsck.*
Quoted 13 lines · Æ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.
[...]
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 <[email protected]>---
Documentation/config.txt filter.<driver>.smudge:: · line 1558
Quoted 5 lines
Documentation/config.txt +5
-0@@ -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?
Documentation/config.txt filter.<driver>.smudge:: · line 1589
Quoted 13 lines
Documentation/config.txt +6
-5fsck.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.
Quoted 2 lines
Documentation/config.txt +2
-0+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.*
Quoted 4 lines · Eric Sunshine
Documentation/config.txt filter.<driver>.smudge:: · line 1556
Quoted 4 lines
Documentation/config.txt +3
-0@@ -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.
Quoted 5 lines
Documentation/config.txt filter.<driver>.smudge:: · line 1580
Quoted 2 lines
Documentation/config.txt +2
-0+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.*
Quoted 18 lines · Junio C Hamano
Quoted 4 lines · Eric Sunshine
Documentation/config.txt filter.<driver>.smudge:: · line 1556
Quoted 4 lines
Documentation/config.txt +3
-0@@ -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 beI 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.
Quoted 3 lines
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.
Quoted 14 lines
Quoted 5 lines
Documentation/config.txt filter.<driver>.smudge:: · line 1580
Quoted 2 lines
Documentation/config.txt +2
-0+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.
Quoted 2 lines
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.*
Quoted 12 lines · Ævar Arnfjörð Bjarmason
Quoted 3 lines · Junio C Hamano
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.
Quoted 3 lines
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.*
Quoted 5 lines · Junio C Hamano
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>.
Quoted 6 lines
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.*
In reply to Junio C HamanoQuoted 14 lines · Junio C Hamano
Quoted 12 lines · Ævar Arnfjörð Bjarmason
Quoted 3 lines · Junio C Hamano
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.
Quoted 6 lines
Quoted 3 lines
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.
Quoted 11 lines
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.*
Quoted 8 lines · Ævar Arnfjörð Bjarmason
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.
Quoted 12 lines
Quoted 12 lines
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
In reply to Ævar Arnfjörð BjarmasonThe 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.
--- Documentation/config.txt | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)
Documentation/config.txt +10 -2
@@ -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
Signature · 1 line
2.17.0.290.gded63e768a
Re: [PATCH v2 3/5] config doc: elaborate on what transfer.fsckObjects does
Quoted 11 lines · Æ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 <[email protected]>---
Documentation/config.txt +0
-0@@ -3341,8 +3341,16 @@ transfer.fsckObjects:: When set, the fetch or receive will abort in the case of a malformed
Documentation/config.txt transfer.fsckObjects:: · line 3351
Quoted 10 lines
Documentation/config.txt +10
-0+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
In reply to Ævar Arnfjörð BjarmasonJeff 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.
--- Documentation/config.txt | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
Documentation/config.txt +3 -1
@@ -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
Signature · 1 line
2.17.0.290.gded63e768a
Re: [PATCH v2 4/5] config doc: mention future aspirations for transfer.fsckObjects
Quoted 7 lines · Ævar Arnfjörð Bjarmason
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.*
In reply to Ævar Arnfjörð BjarmasonImplement 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 modesThe 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.
--- Documentation/config.txt | 21 +++++++++++---- fetch-pack.c | 32 +++++++++++++++++++++-- t/t5504-fetch-receive-strict.sh | 46 +++++++++++++++++++++++++++++++++ 3 files changed, 92 insertions(+), 7 deletions(-)
Documentation/config.txt +16 -5
@@ -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
fetch-pack.c +30 -2
@@ -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 )
t/t5504-fetch-receive-strict.sh +46 -0
62 lines not on this page · raw message
Signature · 1 line
2.17.0.290.gded63e768a
Re: [PATCH v2 5/5] fetch: implement fetch.fsck.*
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.
Documentation/config.txt fetch.fsckObjects::
Quoted 6 lines · Ævar Arnfjörð Bjarmason
Documentation/config.txt +0
-0@@ -1426,6 +1426,16 @@ fetch.fsckObjects::
Nicely written.
Quoted 13 lines
fetch-pack.c +1
-0@@ -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 ;
fetch-pack.c static int agent_supported; · line 37
Quoted 4 lines
fetch-pack.c +1
-0@@ -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.
Quoted 10 lines
t/t5504-fetch-receive-strict.sh +2
-0@@ -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)" &&
t/t5504-fetch-receive-strict.sh test_expect_success 'push with receive.fsck.skipList' ' · line 149
Quoted 10 lines
t/t5504-fetch-receive-strict.sh +10
-0+ 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.*
Quoted 26 lines · 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.
Documentation/config.txt fetch.fsckObjects::
Quoted 6 lines · Ævar Arnfjörð Bjarmason
Documentation/config.txt +0
-0@@ -1426,6 +1426,16 @@ fetch.fsckObjects::Nicely written.
Quoted 10 lines
fetch-pack.c +1
-0@@ -19,6 +19,7 @@#include "sha1-array.h" #include "oidset.h" #include "packfile.h" +#include "fsck.h"
Quoted 11 lines
fetch-pack.c static int agent_supported; · line 37
Quoted 7 lines
fetch-pack.c +1
-0static 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?
Quoted 19 lines
Other than that, the implementation looks good.
Quoted 17 lines
t/t5504-fetch-receive-strict.sh +9
-0@@ -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 &&
Quoted 8 lines
t/t5504-fetch-receive-strict.sh test_expect_success 'push with receive.fsck.skipList' ' · line 149
Quoted 3 lines
t/t5504-fetch-receive-strict.sh +3
-0+ 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.
v3 cover fsck: doc fixes & fetch.fsck.* implementation
In reply to Ævar Arnfjörð BjarmasonIt'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/[email protected]/).
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/[email protected]/
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(-)
Signature · 1 line
2.18.0.345.g5c9ce644c3
Re: [PATCH v3 00/10] fsck: doc fixes & fetch.fsck.* implementation
'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 1/10 receive.fsck.<msg-id> tests: remove dead code
In reply to Ævar Arnfjörð BjarmasonRemove 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.
--- t/t5504-fetch-receive-strict.sh | 2 -- 1 file changed, 2 deletions(-)
t/t5504-fetch-receive-strict.sh +0 -2
@@ -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 '
Signature · 1 line
2.18.0.345.g5c9ce644c3
Re: [PATCH v3 01/10] receive.fsck.<msg-id> tests: remove dead code
Quoted 9 lines · Æ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.
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 <[email protected]> 1234567890 +0000in 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.
Quoted 19 lines
Signed-off-by Ævar Arnfjörð Bjarmason <[email protected]>--- t/t5504-fetch-receive-strict.sh | 2 -- 1 file changed, 2 deletions(-)t/t5504-fetch-receive-strict.sh +0
-2@@ -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
Quoted 12 lines · Junio C Hamano
Quoted 9 lines · Æ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.
Quoted 6 lines
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 <[email protected]> 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.
Quoted 7 lines
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.
Quoted 19 lines
Quoted 19 lines
Signed-off-by Ævar Arnfjörð Bjarmason <[email protected]>--- t/t5504-fetch-receive-strict.sh | 2 -- 1 file changed, 2 deletions(-)t/t5504-fetch-receive-strict.sh +0
-2@@ -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
Quoted 6 lines · Ævar Arnfjörð Bjarmason
Quoted 3 lines
author Bugs Bunny 1234567890 +0000 committer Bugs Bunny <[email protected]> 1234567890 +0000This 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.
Quoted 4 lines
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 2/10 config doc: don't describe *.fetchObjects twice
In reply to Ævar Arnfjörð BjarmasonRefer 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.
--- Documentation/config.txt | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-)
Documentation/config.txt +10 -8
@@ -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
Signature · 1 line
2.18.0.345.g5c9ce644c3
Re: [PATCH v3 02/10] config doc: don't describe *.fetchObjects twice
Quoted 7 lines · Æ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.
Good clean-up to make them better in a later step, which makes sense.
PATCH v3 3/10 config doc: unify the description of fsck.* and receive.fsck.*
In reply to Ævar Arnfjörð BjarmasonThe 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).
--- Documentation/config.txt | 62 +++++++++++++++++++++++----------------- 1 file changed, 35 insertions(+), 27 deletions(-)
Documentation/config.txt +35 -27
@@ -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
Signature · 1 line
2.18.0.345.g5c9ce644c3
Re: [PATCH v3 03/10] config doc: unify the description of fsck.* and receive.fsck.*
Quoted 4 lines · Æ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.
Overall the result reads a lot easily. Especially this part:
Documentation/config.txt filter.<driver>.smudge:: · line 1602
Quoted 5 lines
Documentation/config.txt +5
-0+ 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 4/10 config doc: elaborate on what transfer.fsckObjects does
In reply to Ævar Arnfjörð BjarmasonThe 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.
--- Documentation/config.txt | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)
Documentation/config.txt +11 -2
@@ -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
Signature · 1 line
2.18.0.345.g5c9ce644c3
Re: [PATCH v3 04/10] config doc: elaborate on what transfer.fsckObjects does
Quoted 19 lines · Æ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 <[email protected]>--- Documentation/config.txt | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)Documentation/config.txt +0
-0
Documentation/config.txt transfer.fsckObjects:: · line 3410
Quoted 8 lines
Documentation/config.txt +4
-0@@ -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 5/10 config doc: elaborate on fetch.fsckObjects security
In reply to Ævar Arnfjörð BjarmasonChange 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/[email protected]/
for further details.
--- Documentation/config.txt | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+)
Documentation/config.txt +21 -0
@@ -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
Signature · 1 line
2.18.0.345.g5c9ce644c3
Re: [PATCH v3 05/10] config doc: elaborate on fetch.fsckObjects security
Documentation/config.txt On the receiving side, failing fsckObjects will make those objects · line 3425
Quoted 2 lines · Ævar Arnfjörð Bjarmason
Documentation/config.txt +2
-0+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
Quoted 9 lines · Junio C Hamano
Documentation/config.txt On the receiving side, failing fsckObjects will make those objects · line 3425
Quoted 2 lines · Ævar Arnfjörð Bjarmason
Documentation/config.txt +2
-0+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 theWe 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 6/10 transfer.fsckObjects tests: untangle confusing setup
In reply to Ævar Arnfjörð BjarmasonThe 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.
--- t/t5504-fetch-receive-strict.sh | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)
t/t5504-fetch-receive-strict.sh +11 -1
@@ -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
Signature · 1 line
2.18.0.345.g5c9ce644c3
PATCH v3 7/10 fetch: implement fetch.fsck.*
In reply to Ævar Arnfjörð BjarmasonImplement 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 modesThe 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.
--- Documentation/config.txt | 20 +++++++++++--- fetch-pack.c | 32 +++++++++++++++++++++-- t/t5504-fetch-receive-strict.sh | 46 +++++++++++++++++++++++++++++++++ 3 files changed, 92 insertions(+), 6 deletions(-)
Documentation/config.txt +16 -4
@@ -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
fetch-pack.c +30 -2
@@ -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 )
t/t5504-fetch-receive-strict.sh +46 -0
62 lines not on this page · raw message
Signature · 1 line
2.18.0.345.g5c9ce644c3
Re: [PATCH v3 07/10] fetch: implement fetch.fsck.*
Documentation/config.txt fsck.<msg-id>:: · line 1615
Quoted 4 lines · Ævar Arnfjörð Bjarmason
+
Settingfsck.<msg-id>will be picked up by linkgit:git-fsck[1], butDocumentation/config.txt +1
-1-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").
Quoted 1 line
+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.*
In reply to Ævar Arnfjörð BjarmasonQuoted 4 lines · Ævar Arnfjörð Bjarmason
fetch-pack.c +2
-1-argv_array_push (&cmd.args,"--strict" ); +argv_array_pushf (&cmd.args,"--strict%s" , + fsck_msg_types.buf);
fetch-pack.c static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args, · line 1482
Quoted 10 lines
fetch-pack.c +9
-0+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.
t/t5504-fetch-receive-strict.sh test_expect_success 'push with receive.fsck.skipList' ' · line 151
Quoted 12 lines
t/t5504-fetch-receive-strict.sh +4
-0@@ -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?
t/t5504-fetch-receive-strict.sh test_expect_success 'push with receive.fsck.skipList' ' · line 155
Quoted 4 lines
t/t5504-fetch-receive-strict.sh +4
-0+ 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.
t/t5504-fetch-receive-strict.sh test_expect_success 'push with receive.fsck.skipList' ' · line 157
Quoted 2 lines
t/t5504-fetch-receive-strict.sh +2
-0+ 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 ...
t/t5504-fetch-receive-strict.sh test_expect_success 'push with receive.fsck.skipList' ' · line 158
Quoted 1 line
t/t5504-fetch-receive-strict.sh +1
-0+ 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.*
In reply to Ævar Arnfjörð BjarmasonDocumentation/config.txt fetch.fsckObjects:: · line 1473
Quoted 12 lines · Ævar Arnfjörð Bjarmason
Documentation/config.txt +4
-0@@ -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< --
help.c +1 -0
@@ -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.*
Quoted 23 lines · Duy Nguyen
Documentation/config.txt fetch.fsckObjects:: · line 1473
Quoted 12 lines · Ævar Arnfjörð Bjarmason
Documentation/config.txt +4
-0@@ -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< --
help.c +0
-0
help.c void list_config_help(int for_human)
Quoted 8 lines
help.c +1
-1@@ -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 8/10 fsck: test & document {fetch,receive}.fsck.* config fallback
In reply to Ævar Arnfjörð BjarmasonTest 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.
--- Documentation/config.txt | 12 ++++++++++++ t/t5504-fetch-receive-strict.sh | 26 ++++++++++++++++++++++++-- 2 files changed, 36 insertions(+), 2 deletions(-)
Documentation/config.txt +12 -0
@@ -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
t/t5504-fetch-receive-strict.sh +24 -2
@@ -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 &&
Signature · 1 line
2.18.0.345.g5c9ce644c3
Re: [PATCH v3 08/10] fsck: test & document {fetch,receive}.fsck.* config fallback
Quoted 7 lines · Æ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.
Hmph, personally I felt that the updates so far made it clear and explicit enough.
Quoted 12 lines
Signed-off-by Ævar Arnfjörð Bjarmason <[email protected]>--- Documentation/config.txt | 12 ++++++++++++ t/t5504-fetch-receive-strict.sh | 26 ++++++++++++++++++++++++-- 2 files changed, 36 insertions(+), 2 deletions(-)Documentation/config.txt +0
-0
Documentation/config.txt The rest of the documentation discusses `fsck.*` for brevity, but the · line 1624
Quoted 6 lines
Documentation/config.txt +3
-0@@ -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.
Documentation/config.txt The rest of the documentation discusses `fsck.*` for brevity, but the · line 1626
Quoted 2 lines
Documentation/config.txt +2
-0+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 9/10 fsck: add stress tests for fsck.skipList
In reply to Ævar Arnfjörð BjarmasonStress 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.
--- t/t5504-fetch-receive-strict.sh | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+)
t/t5504-fetch-receive-strict.sh +28 -0
@@ -133,6 +133,14 @@ committer Bugs Bunny <[email protected]> 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 '
Signature · 1 line
2.18.0.345.g5c9ce644c3
PATCH v3 10/10 fsck: test and document unknown fsck.<msg-id> values
In reply to Ævar Arnfjörð BjarmasonWhen 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.
--- Documentation/config.txt | 4 ++++ t/t5504-fetch-receive-strict.sh | 14 ++++++++++++++ 2 files changed, 18 insertions(+)
Documentation/config.txt +4 -0
@@ -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
t/t5504-fetch-receive-strict.sh +14 -0
@@ -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 &&
Signature · 1 line
2.18.0.345.g5c9ce644c3
Re: [PATCH v3 10/10] fsck: test and document unknown fsck.<msg-id> values
a
Quoted 2 lines · Æ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"
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
In reply to Ævar Arnfjörð BjarmasonQuoted 3 lines · Æ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.
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?
Quoted 69 lines
Signed-off-by Ævar Arnfjörð Bjarmason <[email protected]>--- Documentation/config.txt | 4 ++++ t/t5504-fetch-receive-strict.sh | 14 ++++++++++++++ 2 files changed, 18 insertions(+)Documentation/config.txt +4
-0@@ -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 pert/t5504-fetch-receive-strict.sh +14
-0@@ -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
Quoted 9 lines · Junio C Hamano
Quoted 3 lines · Æ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.
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?
Quoted 70 lines
Quoted 70 lines
Signed-off-by Ævar Arnfjörð Bjarmason <[email protected]>--- Documentation/config.txt | 4 ++++ t/t5504-fetch-receive-strict.sh | 14 ++++++++++++++ 2 files changed, 18 insertions(+)Documentation/config.txt +4
-0@@ -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 pert/t5504-fetch-receive-strict.sh +14
-0@@ -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
Quoted 10 lines · Ævar Arnfjörð Bjarmason
Quoted 3 lines
Quoted 3 lines
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.
Re: [PATCH 0/4] fsck: doc fixes & fetch.fsck.* implementation
In reply to Ævar Arnfjörð BjarmasonQuoted 8 lines · Ævar Arnfjörð Bjarmason
Quoted 7 lines · Jeff King
Quoted 3 lines · Ævar Arnfjörð Bjarmason
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.
Quoted 10 lines · Ævar Arnfjörð Bjarmason
When I do:
git -c fetch.fsckObjects=true clone [email protected]:robbyrussell/oh-my-zsh.git
I get:
error: object
2b7227859263b6aabcc28355b0b994995b7148b6: zeroPaddedFilemode: contains zero-padded file modes
fatal: Error in object
fatal: index-pack failed
Quoted 8 lines
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 inbuiltin/fsck.cthat justfsck_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 shouldfsck_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
Am 24.05.2018 um 17:25 schrieb Ævar Arnfjörð Bjarmason:
Quoted 11 lines
When I do:
git -c fetch.fsckObjects=true clone [email protected]: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 [email protected]: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
Quoted 19 lines · Thomas Braun
Am 24.05.2018 um 17:25 schrieb Ævar Arnfjörð Bjarmason:
Quoted 11 lines
When I do:
git -c fetch.fsckObjects=true clone [email protected]: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 [email protected]:robbyrussell/oh-my-zsh.git
Quoted 6 lines
does the trick here (stolen from [1]).
$ git --version git version 2.17.0.windows.1I 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/[email protected]/)