From: Philip Oakley <philipoakley@iee.org>
Date: Fri, 06 May 2016 07:23:23 GMT
Subject: Re: [PATCH] exec_cmd.c, sideband.c, Makefile: avoid multiple PREFIX definitions
Message-ID: <1C7D96C1B53D45CCAA8658C9DBB1AAB5@PhilipOakley>
In-Reply-To: <xmqqk2j89n9y.fsf@gitster.mtv.corp.google.com>

From: "Junio C Hamano" <gitster@pobox.com>
> Philip Oakley <philipoakley@iee.org> writes:
>
>> The short and sweet PREFIX can be confused when used in many places.
>>
>> Rename both usages to better describe their purpose.
>>
>> Noticed when compiling Git for Windows using MSVC/Visual Studio which
>> reports the conflict beteeen the command line definition and the
>> definition in sideband.c
>
> Good eyes.
>
>>
>> Signed-off-by: Philip Oakley <philipoakley@iee.org>
>> ---
>>  Makefile   | 2 +-
>>  exec_cmd.c | 4 ++--
>>  sideband.c | 6 +++---
>>  3 files changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/Makefile b/Makefile
>> index 33b0f76..bcdd3ec 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -1973,7 +1973,7 @@ exec_cmd.sp exec_cmd.s exec_cmd.o: GIT-PREFIX
>>  exec_cmd.sp exec_cmd.s exec_cmd.o: EXTRA_CPPFLAGS = \
>>  '-DGIT_EXEC_PATH="$(gitexecdir_SQ)"' \
>>  '-DBINDIR="$(bindir_relative_SQ)"' \
>> - '-DPREFIX="$(prefix_SQ)"'
>> + '-DEXEC_PREFIX="$(prefix_SQ)"'
>
> I am not entirely happy with this name as the name can be easily
> confused with GIT_EXEC_PATH.
>
> This is a fallback used under RUNTIME_PREFIX option, if I am reading
> the code correctly, so the name should hint the linkage between the
> "runtime prefix" mechanism and this variable.
>
> Perhaps RUNTIME_PREFIX_FALLBACK?  Used at only two places, that
> should not be an overlong name.

Perhaps EXEC_CMD_PREFIX, for that is what it is? And it's got a 2-way 
difference from the GIT_EXEC_PATH. I wasn't seeing it as a fallback, rather 
just a different way the OS uses to get to the path. If anything 
RUNTIME_PREFIX feels a bit long for a flag.

>
> DISPLAY_PREFIX is OK, as it is an entirely a local thing to
> sideband.c, but with the externally visible PREFIX fixed to be a
> more appropriate name that hints its relation with the "runtime
> prefix" mechanism, it may be better not to even touch it.
>
>>  builtin/init-db.sp builtin/init-db.s builtin/init-db.o: GIT-PREFIX
>>  builtin/init-db.sp builtin/init-db.s builtin/init-db.o: EXTRA_CPPFLAGS = 
>> \
>> diff --git a/exec_cmd.c b/exec_cmd.c
>> index 9d5703a..2a79781 100644
>> --- a/exec_cmd.c
>> +++ b/exec_cmd.c
>> @@ -12,7 +12,7 @@ char *system_path(const char *path)
>>  #ifdef RUNTIME_PREFIX
>>  static const char *prefix;
>>  #else
>> - static const char *prefix = PREFIX;
>> + static const char *prefix = EXEC_PREFIX;
>>  #endif
>>  struct strbuf d = STRBUF_INIT;
>>
>> @@ -27,7 +27,7 @@ char *system_path(const char *path)
>>      !(prefix = strip_path_suffix(argv0_path, GIT_EXEC_PATH)) &&
>>      !(prefix = strip_path_suffix(argv0_path, BINDIR)) &&
>>      !(prefix = strip_path_suffix(argv0_path, "git"))) {
>> - prefix = PREFIX;
>> + prefix = EXEC_PREFIX;
>>  trace_printf("RUNTIME_PREFIX requested, "
>>  "but prefix computation failed.  "
>>  "Using static fallback '%s'.\n", prefix);
>