Re: [PATCH v2 1/1] pack-bitmap.c: avoid exposing absolute paths

From: Teng Long <dyroneteng@gmail.com>
Date: Nov 3, 2022, 09:35 UTC
Message-ID: <20221103093532.40511-1-tenglong.tl@alibaba-inc.com>
In-Reply-To: <Y2MWeE2f/P1iXPCY@nand.local>

Taylor Blau <me@ttaylorr.com> writes:

> OK... here we're getting rid of the user-visible warning, which makes
> sense and is the point of this patch.

Yes, my point is to avoid exposing some sensitive informations to end- users.

Show 5 quoted lines

> But we replace it with a trace2_data_string() that only includes the
> basename? I'd think that the point of moving this warning into
> trace2-land is that we could emit the full path without worrying about
> who sees it, since trace2 data is typically not plumbed through to
> end-users.

I'm not sure if trace2 data will be given to end-users, at least as I understand it as you, it's not. If so, your opinion is very reasonable.

So... maybe we could add a new configuration like "core.hideSensitive", and there are some supported values , "loose", "normal " and "strict":

    loose: plumbing full information in trace2, even warning.
    normal: plumbing full information in trace2, but not warning.
    strict: plumbing part of information in trace2, but not warning

But it looks like heavy, maybe not worthy... So, currently I will remove basename and print the pack with path if there are no new inputs here.

Thanks.

Show 10 quoted lines

> If we get later bitmap-related information in the trace2 stream, we know
> that we opened a bitmap. And at the moment we read a bitmap, there is
> only one such bitmap in the repository.
>
> I suppose that this is race-proof in the sense that if the bitmaps
> change after reading, we can still usefully debug the trace2 stream
> after the fact.
>
> So even though my first reaction was that this was unnecessary, on
> balance I do find it useful.

Yes... I think it's useful as least for me to do some bitmap tests and I think print a bit more related information in trace2 data might be OK.

Show 19 quoted lines

> > -test_expect_success 'complains about multiple pack bitmaps' '
> > +test_expect_success 'complains about multiple pack bitmaps when debugging by trace2' '
> >  	rm -fr repo &&
> >  	git init repo &&
> >  	test_when_finished "rm -fr repo" &&
> > @@ -420,8 +420,13 @@ test_expect_success 'complains about multiple pack bitmaps' '
> >  		test_line_count = 2 packs &&
> >  		test_line_count = 2 bitmaps &&
> >
> > -		git rev-list --use-bitmap-index HEAD 2>err &&
> > -		grep "ignoring extra bitmap file" err
> > +		ls -tr .git/objects/pack | grep -e "^pack-.*\.pack$" > sorted-packs &&
> > +		ignored_pack="$(cat sorted-packs | awk 'NR==1{print}')" &&
> > +		open_pack="$(cat sorted-packs | awk 'NR==2{print}')" &&
>
> Hmmph. This test only passes if 'ls -tr' gives us the packs in order
> that they are read by readdir(), which doesn't seem all that portable to
> me. At the very least, it is tightly coupled to the implementation of
> open_pack_bitmap() and friends.

Yes, because the _order_ matters here I think originally. May you explain a little more about _portable_ problem please?

Show 9 quoted lines

> Do we necessarily care about which .bitmap is read and which isn't? The
> existing test doesn't look too closely, which I think is fine (though as
> the author of that test, I might be biased ;-)).
>
> I would be equally happy to write:
>
> > +		GIT_TRACE2_PERF=$(pwd)/trace2.txt git rev-list --use-bitmap-index HEAD &&
> > +		grep "opened bitmap" trace2.txt &&
> > +		grep "ignoring extra bitmap" trace2.txt

Orz. Actually, I wrote it on the same way, but I think it maybe not so sufficient for my patch. So...

But I think you are right afterI look other test cases, will fix.

Thanks.