[PATCH] scalar: make enlistment delete to work on all POSIX platforms

[PATCH] scalar: make enlistment delete to work on all POSIX platforms

[Marcel Telka]

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"));

Re: [PATCH] scalar: make enlistment delete to work on all POSIX platforms

[Junio C Hamano]

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"));

Re: [PATCH] scalar: make enlistment delete to work on all POSIX platforms

[Johannes Schindelin]

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"));
>

Re: [PATCH] scalar: make enlistment delete to work on all POSIX platforms

[Josh Steadmon]

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>

Re: [PATCH] scalar: make enlistment delete to work on all POSIX platforms

[Junio C Hamano]

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.