Re: [PATCH] fsck: check skiplist for object in fsck_blob()

From: Ramsay Jones <ramsay@ramsayjones.plus.com>
Date: Jul 11, 2018, 19:31 UTC
Message-ID: <2ad1b00c-70ff-c4b2-8cbc-9ef55c174221@ramsayjones.plus.com>
In-Reply-To: <20180707013239.GA4687@sigill.intra.peff.net>

On 07/07/18 02:32, Jeff King wrote: [snip]

Show 10 quoted lines

>> I'm not interested in any savings - it would have to be a pretty
>> wacky repo for there to be much in the way of savings!
>>
>> Simply, I have found (for many different reasons) that, if there
>> is no good reason to execute some code, it is _far_ better to not
>> do so! ;-)
> 
> Heh. I also agree with that as a guiding principle. But I _also_ like
> the principle of "if you do not need to do add this code, do not add
> it". So the two are a little at odds here. :)

I agree with that also! ;-) However, in this case, I can't imagine having to do less, to do nothing - if you see what I mean! So, I think "don't execute code you don't need to" trumps "don't add code you don't need to" here.

[snip]

> What next? I was kind of leaning towards loosening, but it sounded like
> Junio thought the opposite. One thing I didn't like about the patch I
> sent earlier is that it removes the option for the admin to say "no, I
> really do want to enforce this". I don't know if that's of value or not.

Yes, it would be good to let the admin set the policy.

Show 10 quoted lines

> In an ideal world, I think we'd detect the problem and then react
> according to the receiver's receive.fsck.* config. And we could just
> flip the default for receive.fsck.gitmodulesParse to "warning". But
> IIRC, the fsck code in index-pack does not bother distinguishing between
> warnings and errors. I think it ought to, but that's too big a change to
> go on maint.
> 
> It _might_ work to just flip the default to "ignore". That leaves the
> paranoid admin with a way to re-enable it if they want, and I _think_ it
> would be respected by index-pack.

Ah, that would be good, if it works.

Show 6 quoted lines

> [goes and looks at the code]
> 
> Hmm, we seem to have "info" these days, so maybe that would do what I
> want. I.e., I wonder if the patch below does everything we'd want. It's
> late here and I probably won't get back to this until Monday, but you
> may want to play with it in the meantime.

Sorry, I've been busy with other things and have not had the time to try the patch below (still trying to catch up with the mailing-list emails!).

Show 21 quoted lines

> diff --git a/fsck.c b/fsck.c
> index 48e7e36869..0b0003055e 100644
> --- a/fsck.c
> +++ b/fsck.c
> @@ -61,7 +61,6 @@ static struct oidset gitmodules_done = OIDSET_INIT;
>  	FUNC(ZERO_PADDED_DATE, ERROR) \
>  	FUNC(GITMODULES_MISSING, ERROR) \
>  	FUNC(GITMODULES_BLOB, ERROR) \
> -	FUNC(GITMODULES_PARSE, ERROR) \
>  	FUNC(GITMODULES_NAME, ERROR) \
>  	FUNC(GITMODULES_SYMLINK, ERROR) \
>  	/* warnings */ \
> @@ -76,6 +75,7 @@ static struct oidset gitmodules_done = OIDSET_INIT;
>  	FUNC(NUL_IN_COMMIT, WARN) \
>  	/* infos (reported as warnings, but ignored by default) */ \
>  	FUNC(BAD_TAG_NAME, INFO) \
> +	FUNC(GITMODULES_PARSE, INFO) \
>  	FUNC(MISSING_TAGGER_ENTRY, INFO)
>  
>  #define MSG_ID(id, msg_type) FSCK_MSG_##id,
>

So, just squinting at this in my email client, if this allowed a push/fetch to succeed (along with an 'info' message), while providing an admin the means to configure it to loudly deny the push/fetch - then I think we have a winner! ;-)

Sorry for not testing the patch.

ATB, Ramsay Jones