threads / patch / 44968

patchmingw: allow hooks to be .exe files

Subject: [PATCH] mingw: allow hooks to be .exe files

## tl;dr

13 messages between Jan 25, 2017 and Jan 30, 2017. Diffs are folded; open one to read it.

replies: 12people: 4as markdown or json

Johannes Schindelin· Jan 25, 2017, 16:58 UTC · lore

This change is necessary to allow the files in .git/hooks/ to optionally have the file extension `.exe` on Windows, as the file names are hardcoded otherwise.

Signed-off-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
---
Published-As: https://github.com/dscho/git/releases/tag/exe-as-hook-v1
Fetch-It-Via: git fetch https://github.com/dscho/git exe-as-hook-v1
 run-command.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)
Show changes to run-command.c +7 −1
diff --git a/run-command.c b/run-command.c
index 73bfba7ef9..45229ef052 100644
--- a/run-command.c
+++ b/run-command.c
@@ -871,8 +871,14 @@ const char *find_hook(const char *name)
 
 	strbuf_reset(&path);
 	strbuf_git_path(&path, "hooks/%s", name);
-	if (access(path.buf, X_OK) < 0)
+	if (access(path.buf, X_OK) < 0) {
+#ifdef STRIP_EXTENSION
+		strbuf_addstr(&path, ".exe");
+		if (access(path.buf, X_OK) >= 0)
+			return path.buf;
+#endif
 		return NULL;
+	}
 	return path.buf;
 }
 

base-commit: 4e59582ff70d299f5a88449891e78d15b4b3fabe

-- 
2.11.1.windows.prerelease.2.3.g7f3eb74
Jeff King· Jan 25, 2017, 21:15 UTC · re: Johannes Schindelin · lore

Re: [PATCH] mingw: allow hooks to be .exe files

On Wed, Jan 25, 2017 at 05:58:42PM +0100, Johannes Schindelin wrote:
> This change is necessary to allow the files in .git/hooks/ to optionally
> have the file extension `.exe` on Windows, as the file names are
> hardcoded otherwise.
Make sense as a goal.
> -	if (access(path.buf, X_OK) < 0)
> +	if (access(path.buf, X_OK) < 0) {
> +#ifdef STRIP_EXTENSION
> +		strbuf_addstr(&path, ".exe");
I think STRIP_EXTENSION is a string.  Should this line be:
  strbuf_addstr(&path, STRIP_EXTENSION);
?
-Peff
Junio C Hamano· Jan 25, 2017, 21:39 UTC · re: Jeff King · lore

Re: [PATCH] mingw: allow hooks to be .exe files

Jeff King <peff@peff.net> writes:
Show 18 quoted lines
> On Wed, Jan 25, 2017 at 05:58:42PM +0100, Johannes Schindelin wrote:
>
>> This change is necessary to allow the files in .git/hooks/ to optionally
>> have the file extension `.exe` on Windows, as the file names are
>> hardcoded otherwise.
>
> Make sense as a goal.
>
>> -	if (access(path.buf, X_OK) < 0)
>> +	if (access(path.buf, X_OK) < 0) {
>> +#ifdef STRIP_EXTENSION
>> +		strbuf_addstr(&path, ".exe");
>
> I think STRIP_EXTENSION is a string.  Should this line be:
>
>   strbuf_addstr(&path, STRIP_EXTENSION);
>
> ?

Yup, I think that is more in line with how git.c (the other user of the macro) uses it.

Johannes Schindelin· Jan 26, 2017, 12:21 UTC · re: Jeff King · lore

Re: [PATCH] mingw: allow hooks to be .exe files

Hi Peff,
On Wed, 25 Jan 2017, Jeff King wrote:
Show 10 quoted lines
> On Wed, Jan 25, 2017 at 05:58:42PM +0100, Johannes Schindelin wrote:
> 
> > -	if (access(path.buf, X_OK) < 0)
> > +	if (access(path.buf, X_OK) < 0) {
> > +#ifdef STRIP_EXTENSION
> > +		strbuf_addstr(&path, ".exe");
> 
> I think STRIP_EXTENSION is a string.  Should this line be:
> 
>   strbuf_addstr(&path, STRIP_EXTENSION);
Yep.

v2 coming, Johannes

Junio C Hamano· Jan 26, 2017, 18:44 UTC · re: Johannes Schindelin · lore

Re: [PATCH] mingw: allow hooks to be .exe files

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 19 quoted lines
> Hi Peff,
>
> On Wed, 25 Jan 2017, Jeff King wrote:
>
>> On Wed, Jan 25, 2017 at 05:58:42PM +0100, Johannes Schindelin wrote:
>> 
>> > -	if (access(path.buf, X_OK) < 0)
>> > +	if (access(path.buf, X_OK) < 0) {
>> > +#ifdef STRIP_EXTENSION
>> > +		strbuf_addstr(&path, ".exe");
>> 
>> I think STRIP_EXTENSION is a string.  Should this line be:
>> 
>>   strbuf_addstr(&path, STRIP_EXTENSION);
>
> Yep.
>
> v2 coming,
> Johannes
I think I've already tweaked it out when I queued the original one.
Johannes Schindelin· Jan 27, 2017, 10:29 UTC · re: Junio C Hamano · lore

Re: [PATCH] mingw: allow hooks to be .exe files

Hi Junio,
On Thu, 26 Jan 2017, Junio C Hamano wrote:
Show 21 quoted lines
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > On Wed, 25 Jan 2017, Jeff King wrote:
> >
> >> On Wed, Jan 25, 2017 at 05:58:42PM +0100, Johannes Schindelin wrote:
> >> 
> >> > -	if (access(path.buf, X_OK) < 0)
> >> > +	if (access(path.buf, X_OK) < 0) {
> >> > +#ifdef STRIP_EXTENSION
> >> > +		strbuf_addstr(&path, ".exe");
> >> 
> >> I think STRIP_EXTENSION is a string.  Should this line be:
> >> 
> >>   strbuf_addstr(&path, STRIP_EXTENSION);
> >
> > Yep.
> >
> > v2 coming,
> > Johannes
> 
> I think I've already tweaked it out when I queued the original one.
After digging, I found your SQUASH commit. I had not known about that.

In any case, I much rather prefer to have the final version of any patch or patch series I contribute to be identical between what you commit and what I sent to the mailing list. We do disagree from time to time, and I would like to have the opportunity of reviewing how you tweak my changes.

Ciao, Johannes

Stefan Beller· Jan 27, 2017, 18:24 UTC · re: Johannes Schindelin · lore

Re: [PATCH] mingw: allow hooks to be .exe files

On Fri, Jan 27, 2017 at 2:29 AM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

Show 35 quoted lines
> Hi Junio,
>
> On Thu, 26 Jan 2017, Junio C Hamano wrote:
>
>> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>>
>> > On Wed, 25 Jan 2017, Jeff King wrote:
>> >
>> >> On Wed, Jan 25, 2017 at 05:58:42PM +0100, Johannes Schindelin wrote:
>> >>
>> >> > -        if (access(path.buf, X_OK) < 0)
>> >> > +        if (access(path.buf, X_OK) < 0) {
>> >> > +#ifdef STRIP_EXTENSION
>> >> > +                strbuf_addstr(&path, ".exe");
>> >>
>> >> I think STRIP_EXTENSION is a string.  Should this line be:
>> >>
>> >>   strbuf_addstr(&path, STRIP_EXTENSION);
>> >
>> > Yep.
>> >
>> > v2 coming,
>> > Johannes
>>
>> I think I've already tweaked it out when I queued the original one.
>
> After digging, I found your SQUASH commit. I had not known about that.
>
> In any case, I much rather prefer to have the final version of any patch
> or patch series I contribute to be identical between what you commit and
> what I sent to the mailing list. We do disagree from time to time, and I
> would like to have the opportunity of reviewing how you tweak my changes.
>
> Ciao,
> Johannes

I saw the queued up commit and that lead me to this thread here. The commit message, while technically correct, seems a bit off. This is because the commit message only talks about .exe extensions, but the change in code doesn't even have the string "exe" mentioned once.

With this discussion here, it is easy for me to connect the dots, but it would be nice to have the full picture in the commit message. This is what I would write:

    mingw: allow hooks to be .exe files
    Executable files in Windows need to have the extension '.exe', otherwise
    they do not work. Extend the hooks to not just look at the hard coded names,
    but also at the extended names via the custom STRIP_EXTENSION, which
    is defined as '.exe' in Windows.
Stefan
Johannes Schindelin· Jan 30, 2017, 12:29 UTC · re: Stefan Beller · lore

Re: [PATCH] mingw: allow hooks to be .exe files

Hi Stefan,
On Fri, 27 Jan 2017, Stefan Beller wrote:
Show 18 quoted lines
> On Fri, Jan 27, 2017 at 2:29 AM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> > Hi Junio,
> >
> > On Thu, 26 Jan 2017, Junio C Hamano wrote:
> >
> >> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> >>
> >> > On Wed, 25 Jan 2017, Jeff King wrote:
> >> >
> >> > v2 coming,
> 
> The commit message, while technically correct, seems a bit off. This is
> because the commit message only talks about .exe extensions, but the
> change in code doesn't even have the string "exe" mentioned once.
> 
> With this discussion here, it is easy for me to connect the dots, but it
> would be nice to have the full picture in the commit message.

I just sent out v3, using a slightly tweaked version of the commit message you proposed.

Ciao, Dscho

Junio C Hamano· Jan 25, 2017, 21:39 UTC · re: Johannes Schindelin · lore

Re: [PATCH] mingw: allow hooks to be .exe files

Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> This change is necessary to allow the files in .git/hooks/ to optionally
> have the file extension `.exe` on Windows, as the file names are
> hardcoded otherwise.
Hmph.  There is no longer ".com"?

I briefly wondered if hooks/post-receive.{py,rb,...} would be good things to support, but I do not think we want to go there, primarily because we do not want to deal with "what happens when there are many?"

As Peff pointed out while I was typing this message, ".exe" would be better spelled as STRIP_EXTENSION, I think. The resulting code would read naturally when you read the macro not as "please do strip extensions" boolean, but as "this extension is to be stripped" string, which is how it is used in the other site the macro is used (namely, strip_extension() called by handle_builtin()).

Show 30 quoted lines
> Signed-off-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
> ---
> Published-As: https://github.com/dscho/git/releases/tag/exe-as-hook-v1
> Fetch-It-Via: git fetch https://github.com/dscho/git exe-as-hook-v1
>
>  run-command.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/run-command.c b/run-command.c
> index 73bfba7ef9..45229ef052 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -871,8 +871,14 @@ const char *find_hook(const char *name)
>  
>  	strbuf_reset(&path);
>  	strbuf_git_path(&path, "hooks/%s", name);
> -	if (access(path.buf, X_OK) < 0)
> +	if (access(path.buf, X_OK) < 0) {
> +#ifdef STRIP_EXTENSION
> +		strbuf_addstr(&path, ".exe");
> +		if (access(path.buf, X_OK) >= 0)
> +			return path.buf;
> +#endif
>  		return NULL;
> +	}
>  	return path.buf;
>  }
>  
>
> base-commit: 4e59582ff70d299f5a88449891e78d15b4b3fabe
Johannes Schindelin· Jan 26, 2017, 13:45 UTC · re: Junio C Hamano · lore

Re: [PATCH] mingw: allow hooks to be .exe files

Hi Junio,
On Wed, 25 Jan 2017, Junio C Hamano wrote:
Show 7 quoted lines
> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > This change is necessary to allow the files in .git/hooks/ to optionally
> > have the file extension `.exe` on Windows, as the file names are
> > hardcoded otherwise.
> 
> Hmph.  There is no longer ".com"?

No, no longer .com. You have to jump through hoops in this century to build .com files.

> I briefly wondered if hooks/post-receive.{py,rb,...} would be good
> things to support, but I do not think we want to go there, primarily
> because we do not want to deal with "what happens when there are many?"

The answer is correct, the reasoning not. The reason why .exe is special: it simply won't execute unless it has the .exe file extension. That is not true for .py, .rb, etc

Ciao, Johannes

Johannes Schindelin· Jan 26, 2017, 12:21 UTC · re: Johannes Schindelin · lore

[PATCH v2] mingw: allow hooks to be .exe files

This change is necessary to allow the files in .git/hooks/ to optionally have the file extension `.exe` on Windows, as the file names are hardcoded otherwise.

Signed-off-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
---
Published-As: https://github.com/dscho/git/releases/tag/exe-as-hook-v2
Fetch-It-Via: git fetch https://github.com/dscho/git exe-as-hook-v2
Interdiff vs v1:
 diff --git a/run-command.c b/run-command.c
 index 45229ef052..5227f78aea 100644
 --- a/run-command.c
 +++ b/run-command.c
 @@ -873,7 +873,7 @@ const char *find_hook(const char *name)
  	strbuf_git_path(&path, "hooks/%s", name);
  	if (access(path.buf, X_OK) < 0) {
  #ifdef STRIP_EXTENSION
 -		strbuf_addstr(&path, ".exe");
 +		strbuf_addstr(&path, STRIP_EXTENSION);
  		if (access(path.buf, X_OK) >= 0)
  			return path.buf;
  #endif
 run-command.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)
Show changes to run-command.c +7 −1
diff --git a/run-command.c b/run-command.c
index 73bfba7ef9..5227f78aea 100644
--- a/run-command.c
+++ b/run-command.c
@@ -871,8 +871,14 @@ const char *find_hook(const char *name)
 
 	strbuf_reset(&path);
 	strbuf_git_path(&path, "hooks/%s", name);
-	if (access(path.buf, X_OK) < 0)
+	if (access(path.buf, X_OK) < 0) {
+#ifdef STRIP_EXTENSION
+		strbuf_addstr(&path, STRIP_EXTENSION);
+		if (access(path.buf, X_OK) >= 0)
+			return path.buf;
+#endif
 		return NULL;
+	}
 	return path.buf;
 }
 

base-commit: 4e59582ff70d299f5a88449891e78d15b4b3fabe

-- 
2.11.1.windows.prerelease.2.9.g3014b57
Johannes Schindelin· Jan 30, 2017, 12:28 UTC · re: Johannes Schindelin · lore

[PATCH v3] mingw: allow hooks to be .exe files

Executable files in Windows need to have the extension '.exe', otherwise they do not work. Extend the hooks to not just look at the hard coded names, but also at the names extended by the custom STRIP_EXTENSION, which is defined as '.exe' in Windows.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
Published-As: https://github.com/dscho/git/releases/tag/exe-as-hook-v3
Fetch-It-Via: git fetch https://github.com/dscho/git exe-as-hook-v3
 run-command.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)
Show changes to run-command.c +7 −1
diff --git a/run-command.c b/run-command.c
index 73bfba7ef9..5227f78aea 100644
--- a/run-command.c
+++ b/run-command.c
@@ -871,8 +871,14 @@ const char *find_hook(const char *name)
 
 	strbuf_reset(&path);
 	strbuf_git_path(&path, "hooks/%s", name);
-	if (access(path.buf, X_OK) < 0)
+	if (access(path.buf, X_OK) < 0) {
+#ifdef STRIP_EXTENSION
+		strbuf_addstr(&path, STRIP_EXTENSION);
+		if (access(path.buf, X_OK) >= 0)
+			return path.buf;
+#endif
 		return NULL;
+	}
 	return path.buf;
 }
 

base-commit: 4e59582ff70d299f5a88449891e78d15b4b3fabe

-- 
2.11.1.windows.prerelease.2.9.g3014b57
Junio C Hamano· Jan 30, 2017, 16:51 UTC · re: Johannes Schindelin · lore

Re: [PATCH v3] mingw: allow hooks to be .exe files

Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> Executable files in Windows need to have the extension '.exe', otherwise
> they do not work. Extend the hooks to not just look at the hard coded
> names, but also at the names extended by the custom STRIP_EXTENSION,
> which is defined as '.exe' in Windows.

Will replace, and looks good enough for 'next'. Let's stop iterating and go incremental if/as needed.

Thanks.

← back to recent threads