| Message ID | 20210715232603.3415111-5-emilyshaffer@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | config-based hooks restarted | expand |
On Thu, Jul 15 2021, Emily Shaffer wrote: > 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 <emilyshaffer@google.com> > --- > 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/ 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.
On Fri, Jul 16, 2021 at 10:58:34AM +0200, Ævar Arnfjörð Bjarmason wrote: > > > On Thu, Jul 15 2021, Emily Shaffer wrote: > > > 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 <emilyshaffer@google.com> > > --- > > 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/ > 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 ;) 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. 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.) - Emily
On Thu, Jul 22 2021, Emily Shaffer wrote: > On Fri, Jul 16, 2021 at 10:58:34AM +0200, Ævar Arnfjörð Bjarmason wrote: >> >> >> On Thu, Jul 15 2021, Emily Shaffer wrote: >> >> > 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 <emilyshaffer@google.com> >> > --- >> > 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/ >> 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 ;) > > 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. > > 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, if you'd like it that way sure. The only reason I nudged you about it was that I assumed you'd perhaps mostly rebased these one-at-a-time on top of the base topic, before the base topic the patches were much larger. There is some arbitrary cut-off-limit where it makes sense to split the addition of code to be used later with the actual use, and that limit is a matter of taste. I thought perhaps given the base topic some of these patches should be squshed given their new size, but if you've looked at them and think it's fine as-is let's leave it at that.
Hello Emily, Emily Shaffer wrote: > On Fri, Jul 16, 2021 at 10:58:34AM +0200, Ævar Arnfjörð Bjarmason wrote: > > On Thu, Jul 15 2021, Emily Shaffer wrote: > > > > > 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 <emilyshaffer@google.com> > > > --- > > > 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/ > > 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.
On Fri, Jul 23, 2021 at 1:34 PM Felipe Contreras <felipe.contreras@gmail.com> wrote: > Emily Shaffer wrote: > > On Fri, Jul 16, 2021 at 10:58:34AM +0200, Ævar Arnfjörð Bjarmason wrote: > > > 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. > > 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. Implicit in what Felipe and Ævar are saying is that a well-structured patch series asks the reviewer to keep only one or two details in mind after reading a patch in order to understand the next patch in the series, and that the reviewer shouldn't be expected to keep a large set of details in mind over several patches. Unlike the author of the patches who can keep all the details in mind at once and understands the series in its entirety, reviewers (usually) don't have such luxury[1]. So, it's important to hand-hold reviewers as much as possible by not asking them to remember a lot of details between patches and by ensuring that the details which they must remember only need to be remembered for a very short time. This is why it is helpful, for instance, to bundle documentation and test updates in the patch with changes to code, so the reviewer can see at a glance that the changes to documentation and tests match the changes to the code, rather than delaying documentation and test updates until later in the series. [1]: If you've ever read a novel in which the author has multiple story lines running and switches between story lines infrequently, such that when the author switches back to story-line "A", which you last saw 100 pages ago, and you can't remember what was going in "A" or even who the minor characters are anymore, so that you have to go back and reread 10 or 20 pages from the previous time you saw "A", then that's representative of the difficulty reviewers can experience when reading a patch series, except with a patch series, the cognitive load is already quite high. (Very nice run-on sentence I just wrote.)
Eric Sunshine wrote: > On Fri, Jul 23, 2021 at 1:34 PM Felipe Contreras > <felipe.contreras@gmail.com> wrote: > > Emily Shaffer wrote: > > > On Fri, Jul 16, 2021 at 10:58:34AM +0200, Ævar Arnfjörð Bjarmason wrote: > > > > 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. > > > > 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. > > Implicit in what Felipe and Ævar are saying is that a well-structured > patch series asks the reviewer to keep only one or two details in mind > after reading a patch in order to understand the next patch in the > series, and that the reviewer shouldn't be expected to keep a large > set of details in mind over several patches. Unlike the author of the > patches who can keep all the details in mind at once and understands > the series in its entirety, reviewers (usually) don't have such > luxury[1]. So, it's important to hand-hold reviewers as much as > possible by not asking them to remember a lot of details between > patches and by ensuring that the details which they must remember only > need to be remembered for a very short time. This is why it is > helpful, for instance, to bundle documentation and test updates in the > patch with changes to code, so the reviewer can see at a glance that > the changes to documentation and tests match the changes to the code, > rather than delaying documentation and test updates until later in the > series. Another important point is that while as a patch author it's natural to push back against possibly unnecessary changes because it would require considerably more work, there's a reason why writers sometimes chose to rewrite entire chapters, and it's because every effort to improve the text as an author would be translated into less effort for the potentially millions of readers. It's a multiplicative effect. Sure, programmers don't have millions of readers, but I think it makes sense to do 2x the effort as a patch author to receive 4x the review (at least). Cheers.
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.
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 <emilyshaffer@google.com> --- hook.c | 3 ++- hook.h | 2 ++ 2 files changed, 4 insertions(+), 1 deletion(-)