Re: [PATCH v2 2/4] git-p4: create new method gitRunHook

From: Ben Keene <seraphire@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: Ben Keene via GitGitGadget <gitgitgadget@gmail.com>, git@vger.kernel.org
Subject: Re: [PATCH v2 2/4] git-p4: create new method gitRunHook
Date: Thu, 6 Feb 2020 09:00:17 -0500	[thread overview]
Message-ID: <9cf8790e-6d13-4653-cddc-f5328f22b0df@gmail.com> (raw)
In-Reply-To: <xmqq36bohfsi.fsf@gitster-ct.c.googlers.com>

On 2/5/2020 4:42 PM, Junio C Hamano wrote:
> Ben Keene <seraphire@gmail.com> writes:
>
>>>> +        hooks_path = os.path.join(os.environ.get("GIT_DIR", ".git"), "hooks")
>>> This assumes that the process when his function is called (by the
>>> way, even though the title of the patch uses the word "method", this
>>> is not a method but a function, no?), it is always at the top level
>>> of the working tree.  Is that a good assumption?  I don't know the
>>> code well, so "yes it is good because a very early thing we do is to
>>> go up to the top" is a good answer.
>> I'm not sure what you mean by top level of the tree unless you mean
>> that it is not part of a class, but a "Free standing" function?
> No.  The discussion about function vs method was over immediately
> after we left the parentheses ;-)
>
> The "top level of the working tree" is the directory where the files
> you see in "git ls-tree $commit^{tree}" would appear in.  In our
> project, that is where the primary Makefile, COPYING, Documentation/,
> etc. hangs from.
>
> The code in your patch (quoted above) says that "When $GIT_DIR is
> not set, '.git/hooks/' is the directory the hooks live in".  That is
> true only when your process is at the top level of the working tree.
> If you chdir'ed to a subdirectory (e.g. Documentation/ in our
> project) and then let the quoted code run, hooks_path is set to
> ".git/hooks/", but from the point of view of the process running
> inside "Documentation/" subdirectory, it should actually be
> "../.git/hooks/", right?

Okay, NOW I get what you meant.  Thank you for the explanation!
The hook directory resolution is what was originally found in the
git-p4.py script.  I just tried to describe what it is doing
because I moved the code around.

As best as I understand the mechanics, and I haven't examined the
source code of git, this is just experimental, the GIT_DIR environment
value is set (at least in Git for Windows) when the program is
executed, so this always has GIT_DIR set, which may be why this
wasn't an issue in the past?

Does this prompt the need to search the hierarchy if we don't find
the directory?

>> And
>> yes, it returns a value so it should be called a function. I'll
>> correct that.
> This is an irrelevant tangent ;-) but yeah, it is a function, as
> opposed to a method, because it is not attached to any class.  I did
> not think Python differentiated functions that return a value and
> ones that do not (e.g. Pascal call the latter "procedure").
>
>> I chose to not put the function within a class so
>> that if other hooks should be added, it would not require a refactoring
>> of the code to use the function in other classes.
> I think that is a sensible design decision to have a free-standing
> function to execute hooks.
>
> Thanks.