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

From: Junio C Hamano <gitster@pobox.com>
To: Ben Keene <seraphire@gmail.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: Wed, 05 Feb 2020 13:42:37 -0800	[thread overview]
Message-ID: <xmqq36bohfsi.fsf@gitster-ct.c.googlers.com> (raw)
In-Reply-To: <ac44531e-b02d-5a98-3e25-a305b1250cf6@gmail.com> (Ben Keene's message of "Wed, 5 Feb 2020 14:56:19 -0500")

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?

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