Taylor Blau <me@xxxxxxxxxxxx> 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.
> 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.
> 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.
> > -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?
> 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.
