Re: [PATCH v2] setup: don't die if realpath(3) fails on getcwd(3)
From: Kevin Locke <hidden>
Date: 2022-05-24 14:02:33
On Mon, 2022-05-23 at 14:57 -0400, Derrick Stolee wrote:
quoted hunk ↗ jump to hunk
On 5/21/22 9:53 AM, Kevin Locke wrote:quoted
+ 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.
quoted
+ tmp_original_cwd = NULL; + startup_info->original_cwd = strbuf_detach(&tmp, NULL); + } else { + trace_printf("realpath(cwd) failed: %s\n", strerror(errno));It could also be helpful to include a trace2 output, since most modern tracing uses that mechanism: trace2_data_string("setup", the_repository, "realpath-path", tmp_original_cwd); trace2_data_string("setup", the_repository, "realpath-failure", strerror(errno)); But this is also unlikely to be necessary. We can instruct a user to rerun their command with GIT_TRACE=1 if we see this error in the wild.
That's a great idea. I hadn't realized the difference between the trace_* and trace2_* APIs until investigating as a result of your suggestion. Trace2 seems preferable for many reasons. I'll send an updated patch shortly. Thanks for the review and feedback Stolee! Cheers, Kevin