# [PATCH] scalar: make enlistment delete to work on all POSIX platforms 5 messages from 2024-05-17 to 2024-05-30. Participants: Marcel Telka, Junio C Hamano, Johannes Schindelin, Josh Steadmon. Thread: https://gitlist.dev/t/61501 ## Marcel Telka, 2024-05-17 14:42 Subject: [PATCH] scalar: make enlistment delete to work on all POSIX platforms Message-ID: URL: https://gitlist.dev/e/Zkds81OB7C5bTCl_%40telcontar ``` The ability to remove the current working directory is not guaranteed by POSIX so it is better to go out of the directory we want to delete on all platforms unconditionally. Signed-off-by: Marcel Telka --- scalar.c | 4 ---- 1 file changed, 4 deletions(-) diff --git a/scalar.c b/scalar.c index 7234049a1b..331b91dbdb 100644 --- a/scalar.c +++ b/scalar.c @@ -361,16 +361,13 @@ static char *remote_default_branch(const char *url) static int delete_enlistment(struct strbuf *enlistment) { -#ifdef WIN32 struct strbuf parent = STRBUF_INIT; size_t offset; char *path_sep; -#endif if (unregister_dir()) return error(_("failed to unregister repository")); -#ifdef WIN32 /* * Change the current directory to one outside of the enlistment so * that we may delete everything underneath it. @@ -385,7 +382,6 @@ static int delete_enlistment(struct strbuf *enlistment) return res; } strbuf_release(&parent); -#endif if (have_fsmonitor_support() && stop_fsmonitor_daemon()) return error(_("failed to stop the FSMonitor daemon")); ``` ## Junio C Hamano, 2024-05-17 20:49 Subject: Re: [PATCH] scalar: make enlistment delete to work on all POSIX platforms Message-ID: URL: https://gitlist.dev/e/xmqqbk54go51.fsf%40gitster.g In-Reply-To: ``` Marcel Telka writes: > The ability to remove the current working directory is not guaranteed by > POSIX so it is better to go out of the directory we want to delete on > all platforms unconditionally. > > Signed-off-by: Marcel Telka > --- > scalar.c | 4 ---- > 1 file changed, 4 deletions(-) Let's CC a few folks that had their hands in the delete_enlistment() function over the years for their opinions on this change. > diff --git a/scalar.c b/scalar.c > index 7234049a1b..331b91dbdb 100644 > --- a/scalar.c > +++ b/scalar.c > @@ -361,16 +361,13 @@ static char *remote_default_branch(const char *url) > > static int delete_enlistment(struct strbuf *enlistment) > { > -#ifdef WIN32 > struct strbuf parent = STRBUF_INIT; > size_t offset; > char *path_sep; > -#endif > > if (unregister_dir()) > return error(_("failed to unregister repository")); > > -#ifdef WIN32 > /* > * Change the current directory to one outside of the enlistment so > * that we may delete everything underneath it. > @@ -385,7 +382,6 @@ static int delete_enlistment(struct strbuf *enlistment) > return res; > } > strbuf_release(&parent); > -#endif > > if (have_fsmonitor_support() && stop_fsmonitor_daemon()) > return error(_("failed to stop the FSMonitor daemon")); ``` ## Johannes Schindelin, 2024-05-18 19:45 Subject: Re: [PATCH] scalar: make enlistment delete to work on all POSIX platforms Message-ID: URL: https://gitlist.dev/e/fb20ebe4-e643-211e-29c8-2cbfb4b49abf%40gmx.de In-Reply-To: ``` Hi, On Fri, 17 May 2024, Junio C Hamano wrote: > Marcel Telka writes: > > > The ability to remove the current working directory is not guaranteed by > > POSIX so it is better to go out of the directory we want to delete on > > all platforms unconditionally. > > > > Signed-off-by: Marcel Telka > > --- > > scalar.c | 4 ---- > > 1 file changed, 4 deletions(-) > > Let's CC a few folks that had their hands in the delete_enlistment() > function over the years for their opinions on this change. > > > diff --git a/scalar.c b/scalar.c > > index 7234049a1b..331b91dbdb 100644 > > --- a/scalar.c > > +++ b/scalar.c > > @@ -361,16 +361,13 @@ static char *remote_default_branch(const char *url) > > > > static int delete_enlistment(struct strbuf *enlistment) > > { > > -#ifdef WIN32 > > struct strbuf parent = STRBUF_INIT; > > size_t offset; > > char *path_sep; > > -#endif > > > > if (unregister_dir()) > > return error(_("failed to unregister repository")); > > > > -#ifdef WIN32 > > /* > > * Change the current directory to one outside of the enlistment so > > * that we may delete everything underneath it. > > @@ -385,7 +382,6 @@ static int delete_enlistment(struct strbuf *enlistment) > > return res; > > } > > strbuf_release(&parent); > > -#endif Basically, this turns the previously Windows-only logic to `chdir("..")` into the now-universal logic. I like it! Thank you, Johannes > > > > if (have_fsmonitor_support() && stop_fsmonitor_daemon()) > > return error(_("failed to stop the FSMonitor daemon")); > ``` ## Josh Steadmon, 2024-05-30 20:57 Subject: Re: [PATCH] scalar: make enlistment delete to work on all POSIX platforms Message-ID: URL: https://gitlist.dev/e/v6q5pahyzcoa7c6tp2x63awvtc6xvjjpi4kl5okgk5qi4god7f%40yg55zwwmcuzt In-Reply-To: ``` On 2024.05.17 16:42, Marcel Telka wrote: > The ability to remove the current working directory is not guaranteed by > POSIX so it is better to go out of the directory we want to delete on > all platforms unconditionally. > > Signed-off-by: Marcel Telka > --- > scalar.c | 4 ---- > 1 file changed, 4 deletions(-) > > diff --git a/scalar.c b/scalar.c > index 7234049a1b..331b91dbdb 100644 > --- a/scalar.c > +++ b/scalar.c > @@ -361,16 +361,13 @@ static char *remote_default_branch(const char *url) > > static int delete_enlistment(struct strbuf *enlistment) > { > -#ifdef WIN32 > struct strbuf parent = STRBUF_INIT; > size_t offset; > char *path_sep; > -#endif > > if (unregister_dir()) > return error(_("failed to unregister repository")); > > -#ifdef WIN32 > /* > * Change the current directory to one outside of the enlistment so > * that we may delete everything underneath it. > @@ -385,7 +382,6 @@ static int delete_enlistment(struct strbuf *enlistment) > return res; > } > strbuf_release(&parent); > -#endif > > if (have_fsmonitor_support() && stop_fsmonitor_daemon()) > return error(_("failed to stop the FSMonitor daemon")); > This looks like a straightforward change; none of the formerly protected logic uses anything specific to Windows, and tests still pass on Linux, so this looks good to me. Reviewed-by: Josh Steadmon ``` ## Junio C Hamano, 2024-05-30 21:17 Subject: Re: [PATCH] scalar: make enlistment delete to work on all POSIX platforms Message-ID: URL: https://gitlist.dev/e/xmqqwmnbngni.fsf%40gitster.g In-Reply-To: ``` Josh Steadmon writes: > This looks like a straightforward change; none of the formerly protected > logic uses anything specific to Windows, and tests still pass on Linux, > so this looks good to me. > > Reviewed-by: Josh Steadmon Thanks. With Dscho's Ack we saw earlier, I should have marked the topic for 'next' already, but I was placing a lot of things on back burner. A gentle nudge like this is greatly appreciated. Will merge to 'next'. Thanks. ```