Re: [PATCH v4] bugreport: collect list of populated hooks

[Emily Shaffer <emilyshaffer@google.com>]

On Fri, May 08, 2020 at 08:34:05AM +0700, Đoàn Trần Công Danh wrote:
> 
> On 2020-05-07 17:53:57-0700, Emily Shaffer <emilyshaffer@google.com> wrote:
> > +test_expect_success 'indicates populated hooks' '
> > +	test_when_finished rm git-bugreport-hooks.txt &&
> > +	test_when_finished rm -fr .git/hooks &&
> > +	rm -fr .git/hooks &&
> > +	mkdir .git/hooks &&
> > +	for hook in applypatch-msg prepare-commit-msg.sample
> > +	do
> > +		write_script ".git/hooks/$hook" <<-\EOF || return 1
> > +		echo "hook $hook exists"
> > +		EOF
> > +	done &&
> > +	git bugreport -s hooks &&
> > +	grep applypatch-msg git-bugreport-hooks.txt &&
> > +	! grep prepare-commit-msg git-bugreport-hooks.txt
> 
> Hi Emily,
> 
> I think this is a bit more correct test.
> ---------8<----------
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
> 
> Use an exact match to check for populated hooks
> 
> Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
> ---
>  t/t0091-bugreport.sh | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/t/t0091-bugreport.sh b/t/t0091-bugreport.sh
> index 9450cc02e3..789e8f1ac7 100755
> --- a/t/t0091-bugreport.sh
> +++ b/t/t0091-bugreport.sh
> @@ -67,8 +67,13 @@ test_expect_success 'indicates populated hooks' '
>  		EOF
>  	done &&
>  	git bugreport -s hooks &&
> -	grep applypatch-msg git-bugreport-hooks.txt &&
> -	! grep prepare-commit-msg git-bugreport-hooks.txt
> +	cat <<-\EOF >expected &&
> +	applypatch-msg
> +
> +	EOF
> +	awk -F "]\\n" -v RS="[" "/applypatch-msg/{print \$2}" \
> +		git-bugreport-hooks.txt >actual &&

If I understand correctly, you are saying "look for a line like [...]
which is followed by a line that says 'applypatch-msg'" - that is,
making sure that you don't see some false positive should
"applypatch-msg" exist in the rest of the bugreport.

Could we compromise and grep for "^applypatch-msg$", e.g. "there is a
line calling out applypatch-msg in some way that isn't in the context of
the report template"?

- Even though regex magic is used, ^$ are beginner regex that many
  people can understand easily.
- Using grep for a single line means we are not allergic to header
  format changes later on

It doesn't search for false positives, true, but I found your awk
suggestion hard to understand - my impression is that awk is less
commonly understood, even among Git contributors.

In sed, it's a little bit more readable:

  cat <<-\EOF >expected &&
  [Enabled Hooks]
  applypatch-msg

  EOF

  sed -n '/\[Enabled Hooks\]/, /^$/ p' git-bugreport-hooks.txt >actual
  test_cmp expected actual

But, I don't like this because it relies on the name of the header, and
the newline spacing between sections - and would be nontrivial to change
if we decided to underline headers instead, or add a newline between a
header and its contents. So I think it may be overkill.

Thanks for your suggestion, though.

 - Emily