| Message ID | eceb2d835be7168081d6eeffbce57bba89b5f423.1727185364.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Remove the_repository global for am, annotate, apply, archive builtins | expand |
On Tue, Sep 24, 2024 at 01:42:41PM +0000, John Cai via GitGitGadget wrote: [snip] > diff --git a/git.c b/git.c > index 2fbea24ec92..e31b52dcc50 100644 > --- a/git.c > +++ b/git.c > @@ -480,7 +480,10 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv, struct > trace2_cmd_name(p->cmd); This line is a little long, we may clean this in this patch. > > validate_cache_entries(repo->index); > - status = p->fn(argc, argv, prefix, (p->option & RUN_SETUP)? repo : NULL); > + status = p->fn(argc, > + argv, > + prefix, > + ((p->option & RUN_SETUP) || (p->option & RUN_SETUP_GENTLY))? repo : NULL); This reads so strange, could we create a new variable here? Small problems, don't worth a reroll. Thanks, Jialuo
shejialuo <shejialuo@gmail.com> writes: > On Tue, Sep 24, 2024 at 01:42:41PM +0000, John Cai via GitGitGadget wrote: > > [snip] > >> diff --git a/git.c b/git.c >> index 2fbea24ec92..e31b52dcc50 100644 >> --- a/git.c >> +++ b/git.c >> @@ -480,7 +480,10 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv, struct >> trace2_cmd_name(p->cmd); > > This line is a little long, we may clean this in this patch. > >> >> validate_cache_entries(repo->index); >> - status = p->fn(argc, argv, prefix, (p->option & RUN_SETUP)? repo : NULL); >> + status = p->fn(argc, >> + argv, >> + prefix, >> + ((p->option & RUN_SETUP) || (p->option & RUN_SETUP_GENTLY))? repo : NULL); > > This reads so strange, could we create a new variable here? > > Small problems, don't worth a reroll. If it is so annoying to make your reading hiccup, it is not small at all. One argument per line is so strange, and one plausible fix would be status = p->fn(argc, argv, prefix, (p->option & (RUN_SETUP|RUN_SETUP_GENTLY)) ? repo : NULL); Thanks.
"John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: John Cai <johncai86@gmail.com> > > commands that have RUN_SETUP_GENTLY potentially need a repository. > Modify the logic in run_builtin() to pass the repository to the builtin > if a builtin has the RUN_SETUP_GENTLY property. Ah, I remember mentioning this as a potiential future direction while reviewing the other series that added the repository argument to the cmd_foo() interface. Nice to see the idea getting followed up. Let's see how [Patches 2-4/4] can make effective use of this. > Signed-off-by: John Cai <johncai86@gmail.com> > --- > git.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/git.c b/git.c > index 2fbea24ec92..e31b52dcc50 100644 > --- a/git.c > +++ b/git.c > @@ -480,7 +480,10 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv, struct > trace2_cmd_name(p->cmd); > > validate_cache_entries(repo->index); > - status = p->fn(argc, argv, prefix, (p->option & RUN_SETUP)? repo : NULL); > + status = p->fn(argc, > + argv, > + prefix, > + ((p->option & RUN_SETUP) || (p->option & RUN_SETUP_GENTLY))? repo : NULL); > validate_cache_entries(repo->index); > > if (status)
On Tue, Sep 24, 2024 at 01:42:41PM +0000, John Cai via GitGitGadget wrote: > From: John Cai <johncai86@gmail.com> > > commands that have RUN_SETUP_GENTLY potentially need a repository. > Modify the logic in run_builtin() to pass the repository to the builtin > if a builtin has the RUN_SETUP_GENTLY property. > > Signed-off-by: John Cai <johncai86@gmail.com> > --- > git.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/git.c b/git.c > index 2fbea24ec92..e31b52dcc50 100644 > --- a/git.c > +++ b/git.c > @@ -480,7 +480,10 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv, struct > trace2_cmd_name(p->cmd); > > validate_cache_entries(repo->index); > - status = p->fn(argc, argv, prefix, (p->option & RUN_SETUP)? repo : NULL); > + status = p->fn(argc, > + argv, > + prefix, > + ((p->option & RUN_SETUP) || (p->option & RUN_SETUP_GENTLY))? repo : NULL); > validate_cache_entries(repo->index); Should we really pass `repo` unconditionally when `RUN_SETUP_GENTLY` was requested? I'd think that we should rather pass `NULL` if we didn't find a repository in that case. So this condition should likely be made conditional, shouldn't it? There's also a missing space between the closing brace and the ternary questionmark. Patrick
Patrick Steinhardt <ps@pks.im> writes: >> - status = p->fn(argc, argv, prefix, (p->option & RUN_SETUP)? repo : NULL); >> + status = p->fn(argc, >> + argv, >> + prefix, >> + ((p->option & RUN_SETUP) || (p->option & RUN_SETUP_GENTLY))? repo : NULL); >> validate_cache_entries(repo->index); > > Should we really pass `repo` unconditionally when `RUN_SETUP_GENTLY` was > requested? I'd think that we should rather pass `NULL` if we didn't find > a repository in that case. So this condition should likely be made > conditional, shouldn't it? Yeah, that is much much more preferrable than my earlier suggestion to pass yet another Boolean parameter to p->fn(). > There's also a missing space between the closing brace and the ternary > questionmark. True, too.
diff --git a/git.c b/git.c index 2fbea24ec92..e31b52dcc50 100644 --- a/git.c +++ b/git.c @@ -480,7 +480,10 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv, struct trace2_cmd_name(p->cmd); validate_cache_entries(repo->index); - status = p->fn(argc, argv, prefix, (p->option & RUN_SETUP)? repo : NULL); + status = p->fn(argc, + argv, + prefix, + ((p->option & RUN_SETUP) || (p->option & RUN_SETUP_GENTLY))? repo : NULL); validate_cache_entries(repo->index); if (status)