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

On Fri, Jun 29, 2018 at 02:10:59AM +0100, Ramsay Jones wrote:

On 28/06/18 23:03, Jeff King wrote:

quoted

On Thu, Jun 28, 2018 at 07:53:27PM +0100, Ramsay Jones wrote:

[snip]

quoted

Yes, it can go in quickly. But I'd prefer not to keep it in the long
term if it's literally doing nothing.

Hmm, I don't think you can say its doing nothing!

    "Yeah, this solution seems sensible. Given that we would
     never report any error for that blob, there is no point
     in even looking at it."

... is no less true, with or without additional patches! ;-)

True that we don't even bother doing the parsing with your patch. But I
think I talked myself out of that part being a significant savings
elsewhere.

I guess it would be OK to leave it in. It just feels like it would be
vestigial after the rest of the patches.

quoted

I have some patches which I think solve your problem. They apply on
v2.18.0, but not on v2.17.1 (because they rely on Dscho's increased
passing of config_options in v2.18). Is that good enough?

Heh, I was also writing patches to address this tonight (but
I was also watching the football, so I was somewhat behind you).
My patches were not too dissimilar to yours, except I was aiming
to allow even do_config_from_file() etc., to suppress errors.

I think this should work via do_config_from_file(). The thing it really
misses is that git_config_with_options() will not respect it, but the
handling of options there is already a bug (well, I don't think there's
anything triggerable either before or after my patches, but it feels
like a bug waiting to happen).

Your patches were cleaner and more focused than mine. (Instead of
turning die_on_error into an enum, I added an additional 'quiet'
flag. When pushing the stack (eg. for include files), I had to
copy the quiet flag from the parent struct, etc, ... ;-) ).

Yes, I think that's what you have to do pre-v2.18, where we don't pass
the options struct around.

quoted

Yes, it would include any syntax error. I also have a slight worry about
that, but nobody seems to have screamed _yet_. :)

Hmm, I don't think we can ignore this. :(

I'm not sure. This has been running on every push to GitHub for the past
6 weeks, and this is the first report. It's hard to say what that means,
and technically speaking of course this _is_ a regression.

There's a nearby thread of interest, too, which I cc'd you on:

  https://public-inbox.org/git/20180703070650.b3drk5a6kb4k4tnp@glandium.org/

quoted

Here are the patches I came up with.

Yes, I applied these locally and tested them. All OK here.

So, FWIW, Ack!

[I still think my original patch, with the 'to_be_skipped'
function name changed to 'object_on_skiplist', should be
the first patch of the series!]

Thanks. If we're going to do any loosening, I think we may want to
address that _first_, so it can go directly on top of the patches in
v2.17.1 (because it's a bigger issue than the stray message, IMHO).

-Peff