# [PATCH] help.c: strip suffix only if the STRIP_EXTENSION defined 6 messages from 2016-03-16 to 2016-03-16. Participants: Alexander Kuleshov, Duy Nguyen, Jeff King. Thread: https://gitlist.dev/t/41706 ## Alexander Kuleshov, 2016-03-16 14:27 Subject: [PATCH] help.c: strip suffix only if the STRIP_EXTENSION defined Message-ID: <1458138449-26690-1-git-send-email-kuleshovmail@gmail.com> URL: https://gitlist.dev/e/1458138449-26690-1-git-send-email-kuleshovmail%40gmail.com ``` 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", &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 ``` ## Duy Nguyen, 2016-03-16 14:47 Subject: Re: [PATCH] help.c: strip suffix only if the STRIP_EXTENSION defined Message-ID: URL: https://gitlist.dev/e/CACsJy8ARrpu%3D7JJ0%3DviuNN0pN7wae8FWRN96EqtiH9bOF_vypw%40mail.gmail.com In-Reply-To: <1458138449-26690-1-git-send-email-kuleshovmail@gmail.com> ``` 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", &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 > > -- > To unsubscribe from this list: send the line "unsubscribe git" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- Duy ``` ## Alexander Kuleshov, 2016-03-16 16:20 Subject: Re: [PATCH] help.c: strip suffix only if the STRIP_EXTENSION defined Message-ID: URL: https://gitlist.dev/e/CANCZXo5m-SF5qHxEgEUSPMaNEkcrZD07qbyfcmMurLVmBDRw%2Bg%40mail.gmail.com In-Reply-To: ``` 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. ``` ## Jeff King, 2016-03-16 17:31 Subject: Re: [PATCH] help.c: strip suffix only if the STRIP_EXTENSION defined Message-ID: <20160316173127.GD4039@sigill.intra.peff.net> URL: https://gitlist.dev/e/20160316173127.GD4039%40sigill.intra.peff.net In-Reply-To: <1458138449-26690-1-git-send-email-kuleshovmail@gmail.com> ``` 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", &entlen); > - > +#ifdef STRIP_EXTENSION > + strip_suffix(ent, STRIP_EXTENSION, &entlen); > +#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 ``` ## Alexander Kuleshov, 2016-03-16 17:36 Subject: Re: [PATCH] help.c: strip suffix only if the STRIP_EXTENSION defined Message-ID: URL: https://gitlist.dev/e/CANCZXo6KGtuuiZ3aha%3DTqkwj3d8qytmA_rvw7fPHfevzz9Qyfg%40mail.gmail.com In-Reply-To: <20160316173127.GD4039@sigill.intra.peff.net> ``` 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. ``` ## Jeff King, 2016-03-16 17:39 Subject: Re: [PATCH] help.c: strip suffix only if the STRIP_EXTENSION defined Message-ID: <20160316173941.GA9142@sigill.intra.peff.net> URL: https://gitlist.dev/e/20160316173941.GA9142%40sigill.intra.peff.net In-Reply-To: ``` 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 ```