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

From: Jeff King <peff@peff.net>
Date: Jul 13, 2018, 19:38 UTC
Message-ID: <20180713193759.GB10354@sigill.intra.peff.net>
In-Reply-To: <2ad1b00c-70ff-c4b2-8cbc-9ef55c174221@ramsayjones.plus.com>

On Wed, Jul 11, 2018 at 08:31:34PM +0100, Ramsay Jones wrote:

Show 12 quoted lines

> >> 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.

Fair enough. I'm OK with it either way, then.

Show 16 quoted lines

> > @@ -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.

No problem. I didn't get back to it until today. And indeed, the patch works as advertised, but there's one additional bit needed (in the preparatory patch below).

So here's what I came up with, which I think is pretty reasonable. The commit message for the second one is quite long, but I tried to lay out the pros and cons from our discussion. And I think what we discussed here may end up being the blueprint for how we consider similar cases in the future, so I tried to be exhaustive.

I built these two on top of my earlier four patches (which Junio has queued as jk/fsck-gitmodules-gently). The code change itself is orthogonal to silencing the config code, but I built on top of the test added earlier. If we want to back-port this to v2.17 or earlier, I can build it the other way around.

If this is what we decide to do upstream, then I'll lobby to flip GitHub's defaults to match. Other hosters (especially ones using other implementations) may consider doing the same.

  [1/2]: fsck: split ".gitmodules too large" error from parse failure
  [2/2]: fsck: downgrade gitmodulesParse default to "info"

 fsck.c                     | 5 +++--
 t/t7415-submodule-names.sh | 2 +-
 2 files changed, 4 insertions(+), 3 deletions(-)

-Peff