| Message ID | Zkds81OB7C5bTCl_@telcontar (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 72b8c934f236711921014c26c9dc64c1611cff82 |
| Headers | show |
| Series | scalar: make enlistment delete to work on all POSIX platforms | expand |
Marcel Telka <marcel@telka.sk> 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 <marcel@telka.sk> > --- > 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"));
Hi, On Fri, 17 May 2024, Junio C Hamano wrote: > Marcel Telka <marcel@telka.sk> 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 <marcel@telka.sk> > > --- > > 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")); >
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 <marcel@telka.sk> > --- > 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 <steadmon@google.com>
Josh Steadmon <steadmon@google.com> 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 <steadmon@google.com> 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.
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"));
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 <marcel@telka.sk> --- scalar.c | 4 ---- 1 file changed, 4 deletions(-)