Re: [PATCH v2] setup: don't die if realpath(3) fails on getcwd(3)
- From
Elijah Newren <newren@gmail.com>
- Date
- May 24, 2022, 15:20 UTC
- Message-ID
- <CABPp-BGZTDKorz+CFScfTfx47c+TuJaAD_Zyyo1Jj_tymYkVXQ@mail.gmail.com>
- In-Reply-To
- <YozlZ9DPrRLPBTBP@kevinlocke.name>
On Tue, May 24, 2022 at 7:02 AM Kevin Locke <kevin@kevinlocke.name> wrote:
Show 74 quoted lines
>
> On Mon, 2022-05-23 at 14:57 -0400, Derrick Stolee wrote:
> > On 5/21/22 9:53 AM, Kevin Locke wrote:
> > > + free((char*)tmp_original_cwd);
> >
> > Hm. I'm never a fan of this casting, but it existed before. It's
> > because tmp_original_cwd is exposed globally in cache.h, which
> > is _really widely_. However, there are only two uses: setup.c,
> > which defines it, and common-main.c, which initializes it during
> > process startup.
> >
> > The following diff could apply before your commit, removing this
> > use of "const char *", but maybe it doesn't fit normal Git
> > coding guidelines (putting the extern directly in a *.c file):
> >
> > --- >8 ---
> >
> > diff --git a/cache.h b/cache.h
> > index aaf334e2aa4..ce9cd6fa3f0 100644
> > --- a/cache.h
> > +++ b/cache.h
> > @@ -1797,7 +1797,6 @@ struct startup_info {
> > const char *original_cwd;
> > };
> > extern struct startup_info *startup_info;
> > -extern const char *tmp_original_cwd;
> >
> > /* merge.c */
> > struct commit_list;
> > diff --git a/common-main.c b/common-main.c
> > index 29fb7452f8a..e472258b83b 100644
> > --- a/common-main.c
> > +++ b/common-main.c
> > @@ -23,6 +23,8 @@ static void restore_sigpipe_to_default(void)
> > signal(SIGPIPE, SIG_DFL);
> > }
> >
> > +extern char *tmp_original_cwd;
> > +
> > int main(int argc, const char **argv)
> > {
> > int result;
> > diff --git a/setup.c b/setup.c
> > index 04ce33cdcd4..86986317490 100644
> > --- a/setup.c
> > +++ b/setup.c
> > @@ -12,7 +12,7 @@ static int work_tree_config_is_bogus;
> >
> > static struct startup_info the_startup_info;
> > struct startup_info *startup_info = &the_startup_info;
> > -const char *tmp_original_cwd;
> > +char *tmp_original_cwd;
> >
> > /*
> > * The input parameter must contain an absolute path, and it must already be
> > @@ -459,7 +459,7 @@ static void setup_original_cwd(void)
> >
> > /* Normalize the directory */
> > strbuf_realpath(&tmp, tmp_original_cwd, 1);
> > - free((char*)tmp_original_cwd);
> > + free(tmp_original_cwd);
> > tmp_original_cwd = NULL;
> > startup_info->original_cwd = strbuf_detach(&tmp, NULL);
> >
> > --- >8 ---
>
> This approach seems reasonable to me, as does casting to free(). It's
> not clear to me which is preferable in this case. How to balance the
> trade-offs between exposing const interfaces, limiting (internal)
> interfaces to headers, and avoiding casts might be worth discussing
> and documenting a matter of project coding style. `grep -rF 'free(('`
> lists about 100 casts to free, suggesting the discussion may be
> worthwhile. Introducing a free_const() macro could be another option
> to consider.I'd prefer either a free_const() as you suggest (though as a separate patch from what you are submitting here), or leaving the code as-is. free() could have been written to take a const void* instead of just void*, since it's not going to modify what the pointer points at. The reason we call free() is because the variable isn't needed anymore, and using a non-const value after freeing is just as wrong as using a const one after freeing, so casting away the constness cannot really cause any new problems. So, I think the signature of free() is just wrong: it should have taken a const void* all along. Unfortunately, the wrong type signature sadly makes people feel like they have to choose between (a) dropping the added safety of const that the compiler can enforce for you during the lifetime of the variable, or (b) leaking memory you no longer need. I think it's a bad choice and you should just typecast when free'ing, but clearly others just don't want to see any typecasts and are willing to dispense with const on constant variables.