Re: [RTC/PATCH] Add 'update-branch' hook

[Felipe Contreras <felipe.contreras@xxxxxxxxx>]

Ilya Bobyr wrote:
> On Mon, Apr 21, 2014 at 2:35 PM, Felipe Contreras <
> felipe.contreras@xxxxxxxxx> wrote:
> > Ilya Bobyr wrote:
> > > test_expect_success 'setup' "
> > >       mkdir -p .git/hooks &&
> > >       cat > .git/hooks/update-branch <<-\\EOF &&
> > >       #!/bin/sh
> > >       echo \$@ > .git/update-branch.args
> > >       EOF
> > >       chmod +x .git/hooks/update-branch &&
> > >       echo one > content &&
> > >       git add content &&
> > >       git commit -a -m one
> > > "
> >
> > That is not maintainable at all.
> 
> Maybe you could explain how is this less maintainable, compared to a separate
> function?

Do I really have to explain that manually escaping a shell script is not
maintainable?

> This is how it is suggested by t/README and how it is done in the other
> test suites.
> I can not see how your case is different, but I might be missing something.

Let's take a cursoy look at `git grep -l "'EOF'" t`.

== t/t0009-prio-queue.sh ==

  cat >expect <<'EOF'
  1
  2
  3
  4
  5
  5
  6
  7
  8
  9
  10
  EOF
  test_expect_success 'basic ordering' '
	  test-prio-queue 2 6 3 10 9 5 7 4 5 8 1 dump >actual &&
	  test_cmp expect actual
  '

Look at that, code outside the cage, not once, but in every test.

== t/t0040-parse-options.sh ==

  cat >>expect <<'EOF'
  list: foo
  list: bar
  list: baz
  EOF
  test_expect_success '--list keeps list of strings' '
	  test-parse-options --list foo --list=bar --list=baz >output &&
	  test_cmp expect output
  '

Once again.

== t/t1411-reflog-show.sh ==
== t/t2020-checkout-detach.sh ==
== t/t3203-branch-output.sh ==
== t/t3412-rebase-root.sh ==
== t/t4014-format-patch.sh ==
== t/t4030-diff-textconv.sh ==

All these do something similar, not once, but many many times.

== t/t4031-diff-rewrite-binary.sh ==

  {
	  echo "#!$SHELL_PATH"
	  cat <<'EOF'
  "$PERL_PATH" -e '$/ = undef; $_ = <>; s/./ord($&)/ge; print $_' < "$1"
  EOF
  } >dump
  chmod +x dump

More code outside.

== t/t4042-diff-textconv-caching.sh ==

  cat >helper <<'EOF'
  #!/bin/sh
  sed 's/^/converted: /' "$@" >helper.out
  cat helper.out
  EOF
  chmod +x helper

== t/t5401-update-hooks.sh ==

  cat >victim.git/hooks/pre-receive <<'EOF'
  #!/bin/sh
  printf %s "$@" >>$GIT_DIR/pre-receive.args
  cat - >$GIT_DIR/pre-receive.stdin
  echo STDOUT pre-receive
  echo STDERR pre-receive >&2
  EOF
  chmod u+x victim.git/hooks/pre-receive

Would you look at that? This is actually a hook test that is changing the hook
*outside* the cage.

== t/t5402-post-merge-hook.sh ==

  for clone in 1 2; do
      cat >clone${clone}/.git/hooks/post-merge <<'EOF'
  #!/bin/sh
  echo $@ >> $GIT_DIR/post-merge.args
  EOF
      chmod u+x clone${clone}/.git/hooks/post-merge
  done

Another hook test with code outside.

== t/t5403-post-checkout-hook.sh ==

Doing the same.

== t/t5516-fetch-push.sh ==

  mk_test_with_hooks() {
	  repo_name=$1
	  mk_test "$@" &&
	  (
		  cd "$repo_name" &&
		  mkdir .git/hooks &&
		  cd .git/hooks &&

		  cat >pre-receive <<-'EOF' &&
		  #!/bin/sh
		  cat - >>pre-receive.actual
		  EOF

		  cat >update <<-'EOF' &&
		  #!/bin/sh
		  printf "%s %s %s\n" "$@" >>update.actual
		  EOF

		  cat >post-receive <<-'EOF' &&
		  #!/bin/sh
		  cat - >>post-receive.actual
		  EOF

		  cat >post-update <<-'EOF' &&
		  #!/bin/sh
		  for ref in "$@"
		  do
			  printf "%s\n" "$ref" >>post-update.actual
		  done
		  EOF

		  chmod +x pre-receive update post-receive post-update
	  )
  }

This one is using a function, just like I am. It's not run outside, but we can
do the same.

== t/t5571-pre-push-hook.sh ==

  write_script "$HOOK" <<'EOF'
  echo "$1" >actual
  echo "$2" >>actual
  cat >>actual
  EOF

Anhoter hook test with code outside.

== t/t7004-tag.sh ==

  cat >fakeeditor <<'EOF'
  #!/bin/sh
  test -n "$1" && exec >"$1"
  echo A signed tag message
  echo from a fake editor.
  EOF
  chmod +x fakeeditor

== t/t7008-grep-binary.sh ==

  cat >nul_to_q_textconv <<'EOF'
  #!/bin/sh
  "$PERL_PATH" -pe 'y/\000/Q/' < "$1"
  EOF
  chmod +x nul_to_q_textconv

== t/t7504-commit-msg-hook.sh ==
== t/t8006-blame-textconv.sh ==
== t/t8007-cat-file-textconv.sh ==
== t/t9138-git-svn-authors-prog.sh ==

Very similar: scripts outside the cage.


In fact my version is actually cleaner than these, because the code that is run
outside the cage is clearly delimited by a function.

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html