On Tue, Apr 13, 2021 at 09:27:33PM +0200, Ævar Arnfjörð Bjarmason wrote:
>
>
> On Tue, Apr 13 2021, Martin Ågren wrote:
>
> > On Mon, 12 Apr 2021 at 19:17, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> >>
> >> Martin Ågren <martin.agren@xxxxxxxxx> writes:
> >>
> >> > In the first test in this script, 'creates a report with content in the
> >> > right places', we generate a report and pipe it into our helper
> >> > `check_all_headers_populated()`. The idea of the helper is to find all
> >> > lines that look like headers ("[Some Header Here]") and to check that
> >> > the next line is non-empty. This is supposed to catch erroneous outputs
> >> > such as the following:
> > ...
> >> > Let's instead grep for some contents that we expect to find in a bug
> >> > report. We won't verify that they appear in the right order, but at
> >> > least we end up verifying the contents more than before this commit.
> >>
> >> Nicely described. I agree that the original intent (let alone the
> >> implementation) is misguided and we should allow an empty section as
> >> a perfectly normal thing.
> >
> >> > +test_expect_success 'creates a report with content' '
> >> > test_when_finished rm git-bugreport-check-headers.txt &&
> >> > git bugreport -s check-headers &&
> >> > - check_all_headers_populated <git-bugreport-check-headers.txt
> >> > + grep "^Please answer " git-bugreport-check-headers.txt &&
> >> > + grep "^\[System Info\]$" git-bugreport-check-headers.txt &&
> >> > + grep "^git version:$" git-bugreport-check-headers.txt &&
> >> > + grep "^\[Enabled Hooks\]$" git-bugreport-check-headers.txt
> >> > '
> >>
> >> It is a different matter if it is sufficient to ensure only certain
> >> selected lines appear in the report, though. As all the lines lost
> >> by this fix comes from 238b439d (bugreport: add tool to generate
> >> debugging info, 2020-04-16), it would be nice to hear from Emily.
> >
> > Maybe something like
> >
> > awk '\''BEGIN { sect="" }
> > /^\[.*]$/ { sect=$0 }
> > /./ { print sect, $0 }'\'' \
> > git-bugreport-check-headers.txt >prefixed &&
> > grep "^ Thank you for filling out a Git bug report" prefixed &&
> > grep "^ Please review the rest of the bug report below" prefixed &&
> > grep "^ You can delete any lines you don.t wish to share" prefixed &&
> > grep "\[System Info\] git version ..." prefixed
> >
> > Something like that could be used to verify that a line goes into the
> > right section, as opposed to just seeing that it appears *somewhere*. Or
> > maybe
> >
> > grep -e Thank.you -e Please.review -e You.can.delete -e "^\[" \
> > -e git.version git-bugreport-check-headers.txt >actual
> >
> > then setting up an "expect" and comparing. That would help us verify the
> > order, including which section things appear in. Slightly less friendly
> > for comparing loosely, compared to the awk-then-grep above.
> >
> > Let's see what Emily thinks about the various alternatives. Maybe she can
> > think of something else.
My apologies for the slow reply :)
> I think a straight-up test_cmp is preferrable, both for correctness and
> also as self-documentation, you can see from the test what the full
> expected output is like.
Yeah, I like the sound of this.
>
> Obviously in this case we can't do a test_cmp on the raw output, as it
> contains various things from uname.
>
> But it looks like we could do that if we do some light awk/perl/sed
> munging of the "[System Info]" and "[Enabled Hooks]" section(s).
>
> Or, since we also control the generator we could pass a --no-system-info
> and/or --no-hooks-info, which may be something some people want for
> privacy/reporting reasons anyway (e.g. I've often used perlbug and
> deleted that whole info, because info there has no relevance to the
> specific issue I'm reporting).
This approach sounds more appealing to me than awk munging. I think
you're right that folks may not want to share it in some cases.
Thanks for noticing.
- Emily
