Re: [PATCH 4/9] hook: treat hookdir hook specially

Hello Emily,

Emily Shaffer wrote:

On Fri, Jul 16, 2021 at 10:58:34AM +0200, Ævar Arnfjörð Bjarmason wrote:

quoted

On Thu, Jul 15 2021, Emily Shaffer wrote:

quoted

Soon, we will allow users to specify hooks using the config. These
config-specified hooks may require different child_process options than
hook executables in the gitdir. So, let's differentiate between hooks
coming from the gitdir and hooks coming from the config.

Signed-off-by: Emily Shaffer <redacted>
---
 hook.c | 3 ++-
 hook.h | 2 ++
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/hook.c b/hook.c
index 19138a8290..3a588cb055 100644
--- a/hook.c
+++ b/hook.c

@@ -117,6 +117,7 @@ struct list_head* hook_list(const char* hookname)
 		struct hook *to_add = xmalloc(sizeof(*to_add));
 		to_add->hook_path = hook_path;
 		to_add->feed_pipe_cb_data = NULL;
+		to_add->from_hookdir = 1;
 		list_add_tail(&to_add->list, hook_head);
 	}
 

@@ -200,7 +201,7 @@ static int pick_next_hook(struct child_process *cp,
 	cp->dir = hook_cb->options->dir;
 
 	/* add command */
-	if (hook_cb->options->absolute_path)
+	if (run_me->from_hookdir && hook_cb->options->absolute_path)
 		strvec_push(&cp->args, absolute_path(run_me->hook_path));
 	else
 		strvec_push(&cp->args, run_me->hook_path);

diff --git a/hook.h b/hook.h
index 586ddf40bb..60389cd8cd 100644
--- a/hook.h
+++ b/hook.h

@@ -22,6 +22,8 @@ struct hook {
 	/* The path to the hook */
 	const char *hook_path;
 
+	unsigned from_hookdir : 1;
+
 	/*
 	 * Use this to keep state for your feed_pipe_fn if you are using
 	 * run_hooks_opt.feed_pipe. Otherwise, do not touch it.

The "from_hookdir" looks like it isn't used until 6/9, and maybe the
absolute_path change too? In any case this seems like a carried-forward
rebase of
https://lore.kernel.org/git/20210311021037.3001235-5-emilyshaffer@google.com/ (local)
or some version thereof.

At this point I tihnk it would be way better to squash this and other
such changes that basically add a field to a struct that isn't used yet
into whatever commit use/need them.

I think at this point we run into you and me having different
patch-storytelling styles - which probably is what led to the big topic
restart in the first place ;)

Yes, but as a reader of the story I prefer not to have to read the
entire thing in order to understand it. I prefer each page to tell a
small story.

I'd prefer to see the "start using config too" patch stay as small as it
can; that's why I ended up with a handful of minor setup commits and
then one "and now here's config" commit.

I'm in favor of small patches too, but not to the point where the patch
is not useful by itself.

This is where where patch storytelling and actual storytelling differ:
we don't need Chekhov's guns.

Even if it's different from how you would tell it - is it wrong? (And if
it is, that's fine, and I'll change it, but I don't think it is - that's
why I structured the series this way.)

It's not wrong.

But one of the strongest advantages of open source is that you can have
many more reviewers than in closed source, and following Linus's law:
"given enough eyeballs, all bugs are shallow", that's a great thing.

But in order to fully take advantage of that you need to write patches
in a way that armchair reviewers can simply take a cursory look at the
patch and figure out that's obviously correct.

Putting my armchair reviewer hat I cannot do that for this particular
patch, I would need to do more work to make sense of it, and while I'm
writing this message to explains that, others will simply skip it, and
that's a lost opportunity.

Cheers.

-- 
Felipe Contreras