Re: [PATCH v2 5/5] fetch: implement fetch.fsck.*
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- May 31, 2018, 07:23 UTC
- Message-ID
- <87efhs9tlw.fsf@evledraar.gmail.com>
- In-Reply-To
- <xmqqin75ere0.fsf@gitster-ct.c.googlers.com>
On Wed, May 30 2018, Junio C Hamano wrote:
Show 37 quoted lines
> Earlier I mumbled "this 4-patch series generally looks good but I > need to re-read the implementation step"; I meant this 5-patch > series and here is my impression after re-reading the implementation > step. > > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >> diff --git a/Documentation/config.txt b/Documentation/config.txt >> index f97f21c022..f69cd31ad0 100644 >> --- a/Documentation/config.txt >> +++ b/Documentation/config.txt >> @@ -1426,6 +1426,16 @@ fetch.fsckObjects:: >> ... > > Nicely written. > >> diff --git a/fetch-pack.c b/fetch-pack.c >> index 490c38f833..9e4282788e 100644 >> --- a/fetch-pack.c >> +++ b/fetch-pack.c >> @@ -19,6 +19,7 @@ >> #include "sha1-array.h" >> #include "oidset.h" >> #include "packfile.h" >> +#include "fsck.h" >> >> static int transfer_unpack_limit = -1; >> static int fetch_unpack_limit = -1; >> @@ -33,6 +34,7 @@ static int agent_supported; >> static int server_supports_filtering; >> static struct lock_file shallow_lock; >> static const char *alternate_shallow_file; >> +static struct strbuf fsck_msg_types = STRBUF_INIT; > > s/msg_types[]/exclude[]/ or something, as this is not just about "we > tolerate known and future instances of 0-padded mode in trees", but > also "we know this and that object is bad so do not complain" as well.
I copied the fsck_msg_types variable as-is from builtin/receive-pack.c which Johannes added in 5d477a334a ("fsck (receive-pack): allow demoting errors to warnings", 2015-06-22).
That's not a justification for doing the same here, but does your critique also extend to that variable name, so I could fix it there while I'm at it?
Show 27 quoted lines
> Other than that, the implementation looks good. > >> diff --git a/t/t5504-fetch-receive-strict.sh b/t/t5504-fetch-receive-strict.sh >> index 49d3621a92..b7f5222c2b 100755 >> --- a/t/t5504-fetch-receive-strict.sh >> +++ b/t/t5504-fetch-receive-strict.sh >> @@ -135,6 +135,20 @@ test_expect_success 'push with receive.fsck.skipList' ' >> git push --porcelain dst bogus >> ' >> >> +test_expect_success 'fetch with fetch.fsck.skipList' ' >> + commit="$(git hash-object -t commit -w --stdin <bogus-commit)" && >> + refspec=refs/heads/bogus:refs/heads/bogus && >> + git push . $commit:refs/heads/bogus && >> + rm -rf dst && >> + git init dst && >> + git --git-dir=dst/.git config fetch.fsckObjects true && >> + test_must_fail git --git-dir=dst/.git fetch "file://$(pwd)" $refspec && >> + git --git-dir=dst/.git config fetch.fsck.skipList dst/.git/SKIP && >> + echo $commit >dst/.git/SKIP && >> + git --git-dir=dst/.git fetch "file://$(pwd)" $refspec >> +' > > If the test does not change meaning when file://$(pwd) is replaced > with "." (or ".." or whatever if other tests below moves cwd > around), I'd think it is preferrable. Seeing $(pwd) always makes me > nervous about Windows.
Thanks. Will fix.