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

From: Ben Keene <seraphire@gmail.com>
Date: Feb 6, 2020, 14:00 UTC
Message-ID: <9cf8790e-6d13-4653-cddc-f5328f22b0df@gmail.com>
In-Reply-To: <xmqq36bohfsi.fsf@gitster-ct.c.googlers.com>

On 2/5/2020 4:42 PM, Junio C Hamano wrote:

Show 27 quoted lines

> 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?

Show 15 quoted lines

>> 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.