Re: [PATCH] git-p4: Add hook p4-pre-pedit-changelist

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Thanks, but it wasn't very helpful to see an Ack (i.e. "an expert
>> says this is good") without seeing any of my "why is this good?"
>> answered by either the original author or the expert X-<.
> More specifically, to summarize the issues I raised:

>
>   * Is the proposed name of the hook a reasonable one?  If so, the
>     log message should explain why it is a reasonable one.  If not,
>     it should be given a more reasonable name and the log message
>     should justify the new name.

>   * Given that "git commit" has a pair of hooks for log message, is
>     adding one new hook a reasonable thing?  If so, the log mesasge
>     should explain why (e.g. perhaps the other one already is there,
>     or perhaps the other one is not applicable in the context of
>     interacting with P4 with such and such reasons).)

>
> Thanks.
>
>>>> "Ben Keene via GitGitGadget" <gitgitgadget@gmail.com> writes:
>>>>
>>>>> From: Ben Keene <seraphire@gmail.com>
>>>>> Subject: Re: [PATCH] git-p4: Add hook p4-pre-pedit-changelist
>>>> "git shortlog --no-merges" would show that the convention is to
>>>> downcase "Add".
>>>>
>>>> With two consecutive non-words (i.e. 'pre' and "pedit'), it really
>>>> feels an unpronounceable mouthful to a non-perforce person like me.
>>>>
>>>> On the core Git side, "git commit", which is the primary command
>>>> that is used to create a new commit, has two hooks that helps to
>>>> enforce consistency to the commit log messages:
>>>>
>>>>   - The "prepare-commit-msg" hook prepares the message to be further
>>>>     edited by the end-user in the editor
>>>>
>>>>   - The "commit-msg" hook takes what the end-user edited in the
>>>>     editor, and can audit and/or tweaks it.
>>>>
>>>> Having a matching pair of hooks and making sure the new hooks have
>>>> similar names to these existing ones may help experienced Git users
>>>> adopt the new hooks "git p4" learns here.
>>>>
>>>> What makes "p4-pre-pedit-changelist" a good name for this hook?  "In
>>>> pure Perforce without Git, there is 'pre-pedit-changelist' hook that
>>>> Perforce users are already familiar with" would be a good answer but
>>>> not being P4 user myself, I do not know if that is true.
>>>>
>>>> Also, "git commit" has a mechanism (i.e. "--no-verify") to suppress
>>>> the "auditing" hook, and it serves as an escape hatch.  The new hook
>>>> "git p4" learns may want to have a similar mechanism, to keep its
>>>> users productive even when they have broken/stale/bogus hook rejects
>>>> their legitimate log message, by allowing them to bypass the
>>>> offending hook(s).
>>>>
>>>>
>>>>> Add an additional hook to the git-p4 command to allow a hook to modify
>>>>> the text of the changelist prior to displaying the p4editor command.
>>>>>
>>>>> This hook will be called prior to checking for the flag
>>>>> "--prepare-p4-only".
>>>>>
>>>>> The hook is optional, if it does not exist, it will be skipped.
>>>>>
>>>>> The hook takes a single parameter, the filename of the temporary file
>>>>> that contains the P4 submit text.
>>>>>
>>>>> The hook should return a zero exit code on success or a non-zero exit
>>>>> code on failure.  If the hook returns a non-zero exit code, git-p4
>>>>> will revert the P4 edits by calling p4_revert(f) on each file that was
>>>>> flagged as edited and then it will return False so the calling method
>>>>> may continue as it does in existing failure cases.
>>>> The githooks(5) page should talk about some of these, I would think.
>>>>
>>>>>   git-p4.py | 11 +++++++++++
>>>>>   1 file changed, 11 insertions(+)
>>>>>
>>>>> diff --git a/git-p4.py b/git-p4.py
>>>>> index 40d9e7c594..1f8c7383df 100755
>>>>> --- a/git-p4.py
>>>>> +++ b/git-p4.py
>>>>> @@ -2026,6 +2026,17 @@ def applyCommit(self, id):
>>>>>           tmpFile.write(submitTemplate)
>>>>>           tmpFile.close()
>>>>>
>>>>> +        # Run the pre-edit hook to allow programmatic update to the changelist
>>>>> +        hooks_path = gitConfig("core.hooksPath")
>>>>> +        if len(hooks_path) <= 0:
>>>>> +            hooks_path = os.path.join(os.environ.get("GIT_DIR", ".git"), "hooks")
>>>>> +
>>>>> +        hook_file = os.path.join(hooks_path, "p4-pre-edit-changelist")
>>>>> +        if os.path.isfile(hook_file) and os.access(hook_file, os.X_OK) and subprocess.call([hook_file, fileName]) != 0:
>>>>> +            for f in editedFiles:
>>>>> +                p4_revert(f)
>>>>> +            return False
>>>>> +
>>>>>           if self.prepare_p4_only:
>>>>>               #
>>>>>               # Leave the p4 tree prepared, and the submit template around
>>>>>
>>>>> base-commit: 232378479ee6c66206d47a9be175e3a39682aea6