Re: [PATCH 0/2] prevent `repack` to unpack and delete promisor objects

[Rafael Silva <rafaeloliveira.cs@gmail.com>]

Jeff King <peff@peff.net> writes:

> On Wed, Apr 14, 2021 at 09:14:01PM +0200, Rafael Silva wrote:
>
>> It took me a bit to come up with the test because it seems `repack`
>> doesn't offer an option to skip the "deletion of unpacked objects",
>> so this series adds a new option to `repack` for skip the
>> `git prune-packed` execution thus allowing us to easily inspect the
>> unpacked objects before they are removed and simplification of our
>> test suite. Furthermore, The test will now test the `repack` code
>> path instead of performing the operations by calling
>> `pack-objects`.
>
> Thanks for working on this. Overall the patches seem sane, though I
> think Jonathan's comments (especially about the confusion in the commit
> message of 2/2) are worth addressing.
>

Thanks for the review. Indeed, I will address Jonathan's comments on
the next revision to remove the confused and misleading commit message.

Sorry for the confusion.

>
> I have mixed feelings on the "--no-prune-packed" option, just because
> it's user-visible and I don't think it's something a normal user would
> ever really want.
>
> In the new test (and I think in the old ones you modified, though I
> didn't look carefully) the main thing we care about is whether we write
> out loose objects. So another solution would be to improve the debug
> logging inside pack-objects to tell us more about what it's doing.
>

Honestly, I'm also not happy adding an end user-visible variable just
for the sake of testing it, specially because it's unclear whether the
user will actually use this option.

Initially, what convinced myself for proposing the changes was the
additional cleanup in our test suite, but giving a second thought I'm
not sure now whether this is a strong argument either.

> The fork of Git we use at GitHub has something similar; when we discard
> objects or force them loose, we write their sha1 values to a log file.
> This has come in handy for a lot of after-the-fact debugging ("oops,
> this repo is corrupted; did we intentionally delete object X?").
>
> I wonder if we could do something similar with the trace2 facility. I
> know it can be turned on via config, but I don't know how good the
> support is for enabling just one segment of data (and this may generate
> a lot of entries, so people using trace2 for telemetry probably wouldn't
> want it on).
>
> For the purposes of the tests, though just a normal GIT_TRACE_PACK_DEBUG
> would be plenty. I dunno. I don't want to open up a can of worms on
> logging that would hold up getting this quite-substantial fix in place.
> But once we add --no-prune-packed, it will be hard to take away.
>
> -Peff

After reading this comment and investigating a bit more, I believe
increasing the debug logging of `pack-objects` will help drop the first
patch, at least for now, and allowing the fix to progress without the
"controversial" user option.  Later, we can revisit adding the
`--no-prune-packed` (or a better named) option in case we think this
will be useful for the users.

I'll be addressing this in v2.

-- 
Thanks
Rafael