Thread (18 messages) 18 messages, 4 authors, 2022-05-28

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
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help