Re: [PATCH 1/4] hooks: Add function to check if a hook exists
Aaron Schrab writes: > Since I'm going to be changing the interface for this hook in v2 of > the series so that it will be more complicated than can be readily > addressed with the run_hook() API (and will have use a fixed number of > arguments anyway) I'll be dropping the run_hook_argv() function. Just to make sure there is no misunderstanding (sorry for sending the message without finishing it with this clarification at the end in the first place). I didn't mean that converting all of the existing callers must come earlier than introducing a new hook invoker. I just wanted to make sure that we are aware that we are adding to our technical debt, if we are adding another that is also specialized; as the proposed interface looked sufficiently generic, it would be the ideal one to make _other_ ones thin wrappers around it to unify the various codepaths. Thanks. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/4] hooks: Add function to check if a hook exists
At 18:08 -0800 28 Dec 2012, Junio C Hamano wrote: Aaron Schrab writes: Create find_hook() function to determine if a given hook exists and is executable. If it is the path to the script will be returned, otherwise NULL is returned. Sounds like a sensible thing to do. To make sure the API is also sensible, all the existing hooks should be updated to use this API, no? I'd been trying to keep the changes limited. I'll see about modifying the existing places that run hooks in v2 of the series. This is in support for an upcoming run_hook_argv() function which will expect the full path to the hook script as the first element in the argv_array. There is currently a public function called run_hook() that squats on the good name with a kludgy API that is too specific to using separate index file. Back when it was a private helper in the implementation of "git commit", it was perfectly fine, but it was exported without giving much thought on the API. If you are introducing a new run_hook_* function, give it a generic enough API that lets all the existing hook callers to use it. I would imagine that the API requirement may be modelled after run_command() API so that we can pass argv[] and tweak the hook's environ[], as well as feeding its stdin and possibly reading from its stdout. That would be very useful. I think the attraction of the run_hook() API is its simplicity. It's currently a fairly thin wrapper around the run_command() API. I suspect that if the run_hook() API were made generic enough to support all of the existing hook callers it would greatly complicate the existing calls to run_hook() while not providing much benefit to hook callers which can't currently use it beyond what run_command() offers. Since I'm going to be changing the interface for this hook in v2 of the series so that it will be more complicated than can be readily addressed with the run_hook() API (and will have use a fixed number of arguments anyway) I'll be dropping the run_hook_argv() function. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/4] hooks: Add function to check if a hook exists
Aaron Schrab writes: > Create find_hook() function to determine if a given hook exists and is > executable. If it is the path to the script will be returned, otherwise > NULL is returned. Sounds like a sensible thing to do. To make sure the API is also sensible, all the existing hooks should be updated to use this API, no? > This is in support for an upcoming run_hook_argv() function which will > expect the full path to the hook script as the first element in the > argv_array. There is currently a public function called run_hook() that squats on the good name with a kludgy API that is too specific to using separate index file. Back when it was a private helper in the implementation of "git commit", it was perfectly fine, but it was exported without giving much thought on the API. If you are introducing a new run_hook_* function, give it a generic enough API that lets all the existing hook callers to use it. I would imagine that the API requirement may be modelled after run_command() API so that we can pass argv[] and tweak the hook's environ[], as well as feeding its stdin and possibly reading from its stdout. That would be very useful. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/4] hooks: Add function to check if a hook exists
Create find_hook() function to determine if a given hook exists and is
executable. If it is the path to the script will be returned, otherwise
NULL is returned.
This is in support for an upcoming run_hook_argv() function which will
expect the full path to the hook script as the first element in the
argv_array. This also makes it simple for places that can use a hook to
check if a hook exists before doing, possibly lengthy, setup work which
would be pointless if no such hook is present.
The returned value is left as a static value from get_pathname() rather
than a duplicate because it is anticipated that the return value will
either be used as a boolean, or immediately added to an argv_array list
which would result in it being duplicated at that point.
Signed-off-by: Aaron Schrab
---
run-command.c | 15 +--
run-command.h |1 +
2 files changed, 14 insertions(+), 2 deletions(-)
diff --git a/run-command.c b/run-command.c
index 3b982e4..49c8fa0 100644
--- a/run-command.c
+++ b/run-command.c
@@ -735,6 +735,15 @@ int finish_async(struct async *async)
#endif
}
+char *find_hook(const char *name)
+{
+ char *path = git_path("hooks/%s", name);
+ if (access(path, X_OK) < 0)
+ path = NULL;
+
+ return path;
+}
+
int run_hook(const char *index_file, const char *name, ...)
{
struct child_process hook;
@@ -744,11 +753,13 @@ int run_hook(const char *index_file, const char *name,
...)
va_list args;
int ret;
- if (access(git_path("hooks/%s", name), X_OK) < 0)
+ p = find_hook(name);
+ if (!p)
return 0;
+ argv_array_push(&argv, p);
+
va_start(args, name);
- argv_array_push(&argv, git_path("hooks/%s", name));
while ((p = va_arg(args, const char *)))
argv_array_push(&argv, p);
va_end(args);
diff --git a/run-command.h b/run-command.h
index 850c638..221ce33 100644
--- a/run-command.h
+++ b/run-command.h
@@ -45,6 +45,7 @@ int start_command(struct child_process *);
int finish_command(struct child_process *);
int run_command(struct child_process *);
+extern char *find_hook(const char *name);
extern int run_hook(const char *index_file, const char *name, ...);
#define RUN_COMMAND_NO_STDIN 1
--
1.7.10.4
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
