[PATCH] help.c: strip suffix only if the STRIP_EXTENSION defined

We stripping extension in the list_commands_in_dir() to get
commands without '.exe' suffix. Let's do it only if STRIP_EXTENSION
is defined to not spend time for unnecessary strip_suffix() call in
this case.

Signed-off-by: Alexander Kuleshov <redacted>
---
 help.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/help.c b/help.c
index 19328ea..c865991 100644
--- a/help.c
+++ b/help.c

@@ -153,8 +153,9 @@ static void list_commands_in_dir(struct cmdnames *cmds,
 			continue;
 
 		entlen = strlen(ent);
-		strip_suffix(ent, ".exe", &entlen);
-
+#ifdef STRIP_EXTENSION
+		strip_suffix(ent, STRIP_EXTENSION, &entlen);
+#endif
 		add_cmdname(cmds, ent, entlen);
 	}
 	closedir(dir);

-- 
2.8.0.rc2.216.g1477fb2.dirty

On Wed, Mar 16, 2016 at 9:27 PM, Alexander Kuleshov
[off-list ref] wrote:

We stripping extension in the list_commands_in_dir() to get
commands without '.exe' suffix. Let's do it only if STRIP_EXTENSION
is defined to not spend time for unnecessary strip_suffix() call in
this case.

Unless the time saving is significant, I'm against this change. It
makes it harder to spot compile bugs in #ifdef'd code that's only
active on Windows (imagine somebody renames "ent" or change its type).
If you can do something like strip_extension() in git.c, it's better.
Or maybe just refactor that function a bit and share it with help.c

-- 
Duy

On Wed, Mar 16, 2016 at 8:47 PM, Duy Nguyen [off-list ref] wrote:

On Wed, Mar 16, 2016 at 9:27 PM, Alexander Kuleshov
[off-list ref] wrote:

quoted

We stripping extension in the list_commands_in_dir() to get
commands without '.exe' suffix. Let's do it only if STRIP_EXTENSION
is defined to not spend time for unnecessary strip_suffix() call in
this case.

Unless the time saving is significant, I'm against this change. It
makes it harder to spot compile bugs in #ifdef'd code that's only
active on Windows (imagine somebody renames "ent" or change its type).
If you can do something like strip_extension() in git.c, it's better.
Or maybe just refactor that function a bit and share it with help.c

Yes, agree. Will think about this.

On Wed, Mar 16, 2016 at 08:27:29PM +0600, Alexander Kuleshov wrote:

This is billed as an optimization in the commit message, but these two
pieces of code are not the same. The original always strips ".exe",
whether or not STRIP_EXTENSION is defined, and whether or not it is
".exe".

In practice it works out because people on Unix systems do not have
"git-foo.exe", and nobody sets STRIP_EXTENSION to other things.  But I
tend to think this is an improvement in robustness.

I also wonder if this should be sharing the strip_extension() helper
added in your 63ca1c0.

-Peff

Hello Jeff,

On Wed, Mar 16, 2016 at 11:31 PM, Jeff King [off-list ref] wrote:

This is billed as an optimization in the commit message, but these two
pieces of code are not the same. The original always strips ".exe",
whether or not STRIP_EXTENSION is defined, and whether or not it is
".exe".

In practice it works out because people on Unix systems do not have
"git-foo.exe", and nobody sets STRIP_EXTENSION to other things.  But I
tend to think this is an improvement in robustness.

I also wonder if this should be sharing the strip_extension() helper
added in your 63ca1c0.

Yes, I want to move strip_extension() (from 63ca1c0) to the git-compat-util.h
and adapt/reuse it in the help.c. What do you think about this?

Thank you.

On Wed, Mar 16, 2016 at 11:36:49PM +0600, Alexander Kuleshov wrote:

quoted

I also wonder if this should be sharing the strip_extension() helper
added in your 63ca1c0.

Yes, I want to move strip_extension() (from 63ca1c0) to the git-compat-util.h
and adapt/reuse it in the help.c. What do you think about this?

Naively, it sounds like a good idea to me, but I haven't looked too
hard. There may be complications in the interface (it looks like the
helper wants to make a new string, but one in help.c makes its own copy
into the flex-array struct).

-Peff