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

[Jeff King]

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

> > 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Jeff King]

On Wed, Mar 16, 2016 at 08:27:29PM +0600, Alexander Kuleshov 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.
> 
> Signed-off-by: Alexander Kuleshov 
> ---
>  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", );
> -
> +#ifdef STRIP_EXTENSION
> + strip_suffix(ent, STRIP_EXTENSION, );
> +#endif

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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Alexander Kuleshov]

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 
---
 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", );
-
+#ifdef STRIP_EXTENSION
+   strip_suffix(ent, STRIP_EXTENSION, );
+#endif
add_cmdname(cmds, ent, entlen);
}
closedir(dir);
-- 
2.8.0.rc2.216.g1477fb2.dirty

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Duy Nguyen]

On Wed, Mar 16, 2016 at 9:27 PM, Alexander Kuleshov
 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

> Signed-off-by: Alexander Kuleshov 
> ---
>  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", );
> -
> +#ifdef STRIP_EXTENSION
> +   strip_suffix(ent, STRIP_EXTENSION, );
> +#endif
> add_cmdname(cmds, ent, entlen);
> }
> closedir(dir);
> --
> 2.8.0.rc2.216.g1477fb2.dirty
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Alexander Kuleshov]

Hello Jeff,

On Wed, Mar 16, 2016 at 11:31 PM, Jeff King  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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Alexander Kuleshov]

On Wed, Mar 16, 2016 at 8:47 PM, Duy Nguyen  wrote:
> On Wed, Mar 16, 2016 at 9:27 PM, Alexander Kuleshov
>  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

Yes, agree. Will think about this.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html