Re: [PATCH 1/1] add hook pre-p4-submit

[Eric Sunshine <sunshine@sunshineco.com>]

On Wed, Jul 25, 2018 at 9:45 AM Chen Bin <chenbin.sh@gmail.com> wrote:
> Hook pre-p4-submit is executed before git-p4 actually submits code.
> If the hook exits with non-zero value, submit process will abort.

Bikeshedding: Should this be named 'p4-pre-submit'? That way, if
git-p4 ever grows additional hooks, they can all be grouped under the
"p4-" prefix.

More below...

> Signed-off-by: Chen Bin <chenbin.sh@gmail.com>
> ---
> diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt
> @@ -374,6 +374,13 @@ These options can be used to modify 'git p4 submit' behavior.
> +Hook for submit
> +~~~~~~~~~~~~~~~
> +Hook `pre-p4-submit` is executed if it exists and is executable.
> +Exiting with non-zero status from this script cause `git-p4 submit` to abort.
> +By default the hooks directory is `$GIT_DIR/hooks`, but that can be changed.

Does the hook receive any arguments? Does it receive anything on its
standard input? Those questions ought to be answered by the
documentation.

Also, how is the hook supposed to determine whether the "submit"
should proceed? How does it know what is being submitted? Perhaps the
documentation could provide some explanation of how the hook should
glean such information (especially if the hook does not itself receive
any input).

> diff --git a/git-p4.py b/git-p4.py
> +        # locate hook
> +        hooks_path = gitConfig("core.hooksPath")
> +        if len(hooks_path) > 0:
> +            hook_file = os.path.join(hooks_path, "pre-p4-submit")
> +        else:
> +            hook_file = os.path.join(read_pipe("git rev-parse --git-dir").strip(), "hooks", "pre-p4-submit")
> +
> +        # Execute hook. If it exits with non-zero value, do NOT continue.
> +        if os.path.isfile(hook_file) and os.access(hook_file, os.X_OK) and subprocess.call([hook_file]) != 0:
> +            sys.exit(1)

Nit: The two #comments merely repeat what the code itself already says
clearly enough, thus don't add value (so can be dropped).

Should this be emitting some sort of message letting the user know
that the operation was aborted due to the hook's "rejection"? That
could be especially important if the hook itself is silent (or buggy).

> diff --git a/t/t9800-git-p4-basic.sh b/t/t9800-git-p4-basic.sh
> @@ -261,6 +261,28 @@ test_expect_success 'unresolvable host in P4PORT should display error' '
> +# Test following scenarios:
> +#   - Without hook ".git/hooks/pre-p4-submit", submit should continue
> +#   - With hook returning 0, submit should continue
> +#   - With hook returning 1, submit should abort

Testing three separate cases would normally be done with three
separate tests, making it easier to figure out which case failed. But,
perhaps it doesn't matter here.

> +test_expect_success 'run hook pre-p4-submit before submit' '
> +       test_when_finished cleanup_git &&
> +       git p4 clone --dest="$git" //depot &&
> +       (
> +               cd "$git" &&
> +               echo "hello world" >hello.txt &&
> +               git add hello.txt &&
> +               git commit -m "add hello.txt" &&
> +               git config git-p4.skipSubmitEdit true &&

The wholesale removal of the $git directory by cleanup_git() when the
test finishes is undoing this config setting. Ditto regarding the
hooks created below. Okay.

> +               git p4 submit --dry-run | grep "Would apply" &&

We normally don't use a git command upstream in a pipe since the exit
code of that command gets lost (so, if it crashes, we don't know about
it). Instead:

    git p4 submit --dry-run >out &&
    grep "Would apply" out &&

Also, if "Would apply" is localized, we'd use test_i18ngrep (but you
don't need it in this case).

> +               mkdir -p .git/hooks &&
> +               : >.git/hooks/pre-p4-submit && chmod +x .git/hooks/pre-p4-submit &&

It might be clearer to readers of this test to use an explicit "exit
0" here (complementing the "exit 1" below). The current asymmetry
required extra cognitive effort to realize that this is the "returning
0" case mentioned in the comment above the test.

> +               git p4 submit --dry-run | grep "Would apply" &&
> +               echo #!/bin/sh && echo exit 1 >.git/hooks/pre-p4-submit &&

Use write_script() for this, which takes care of the #!/bin/sh line
and doing chmod:

    write_script >.git/hooks/pre-p4-submit <<-\EOF &&
    exit 1
    EOF

> +               git p4 submit --dry-run | grep "Would apply" || echo "Abort submit"

More idiomatic in this test suite:

    git p4 submit --dry-run >out &&
    ! grep "Would apply" out

> +       )
> +'