From: Kevin Locke <kevin@kevinlocke.name>
To: Derrick Stolee <derrickstolee@github.com>
Cc: git@vger.kernel.org, Elijah Newren <newren@gmail.com>,
Junio C Hamano <gitster@pobox.com>
Subject: Re: [PATCH v2] setup: don't die if realpath(3) fails on getcwd(3)
Date: Tue, 24 May 2022 08:02:15 -0600 [thread overview]
Message-ID: <YozlZ9DPrRLPBTBP@kevinlocke.name> (raw)
In-Reply-To: <1580ad10-43f6-bc73-901a-b65b1aea73ff@github.com>
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.
>> + 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
next prev parent reply other threads:[~2022-05-24 14:02 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-05-19 23:39 [PATCH] setup: don't die if realpath(3) fails on getcwd(3) Kevin Locke
2022-05-20 18:38 ` Junio C Hamano
2022-05-21 0:14 ` Elijah Newren
2022-05-21 13:02 ` Kevin Locke
2022-05-23 18:44 ` Derrick Stolee
2022-05-21 13:53 ` [PATCH v2] " Kevin Locke
2022-05-23 18:57 ` Derrick Stolee
2022-05-24 14:02 ` Kevin Locke [this message]
2022-05-24 15:20 ` Elijah Newren
2022-05-24 17:38 ` Derrick Stolee
2022-05-25 3:47 ` Elijah Newren
2022-05-27 7:48 ` Ævar Arnfjörð Bjarmason
2022-05-28 1:27 ` Elijah Newren
2022-05-24 14:51 ` [PATCH v3] " Kevin Locke
2022-05-24 15:21 ` Elijah Newren
2022-05-24 17:41 ` Derrick Stolee
2022-05-24 18:00 ` Kevin Locke
2022-05-24 19:20 ` [PATCH v4] " Kevin Locke
2022-05-24 20:40 ` Derrick Stolee
2022-05-24 21:25 ` Junio C Hamano
2022-05-25 3:51 ` Elijah Newren
2022-05-25 5:11 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YozlZ9DPrRLPBTBP@kevinlocke.name \
--to=kevin@kevinlocke.name \
--cc=derrickstolee@github.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=newren@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).