[PATCH] Fallback on _NSGetExecutablePath to get the executable path if using argv[0] fails

[PATCH] Fallback on _NSGetExecutablePath to get the executable path if using argv[0] fails

[Jeremy Huddleston]

Signed-off-by: Jeremy Huddleston <jeremyhu@apple.com>
Reviewed-by: Matt Wright <mww@apple.com>
---
 exec_cmd.c |   17 +++++++++++++++++
 1 files changed, 17 insertions(+), 0 deletions(-)

diff --git a/exec_cmd.c b/exec_cmd.c
index bf22570..1e24a8f 100644
--- a/exec_cmd.c
+++ b/exec_cmd.c
@@ -3,6 +3,10 @@
 #include "quote.h"
 #define MAX_ARGS	32
 
+#if defined(__APPLE__) && defined(RUNTIME_PREFIX)
+#include <mach-o/dyld.h>
+#endif
+
 extern char **environ;
 static const char *argv_exec_path;
 static const char *argv0_path;
@@ -53,6 +57,19 @@ const char *git_extract_argv0_path(const char *argv0)
 	if (slash >= argv0) {
 		argv0_path = xstrndup(argv0, slash - argv0);
 		return slash + 1;
+#ifdef __APPLE__
+	} else {
+		char new_argv0[PATH_MAX];
+		uint32_t new_argv0_s = PATH_MAX;
+		if(_NSGetExecutablePath(new_argv0, &new_argv0_s) == 0) {
+			slash = new_argv0 + new_argv0_s;
+			while (new_argv0 <= slash && !is_dir_sep(*slash))
+		                slash--;
+
+			if (slash >= new_argv0)
+				argv0_path = xstrndup(new_argv0, slash - new_argv0);
+		}
+#endif
 	}
 
 	return argv0;
-- 
1.7.3.2

Re: [PATCH] Fallback on _NSGetExecutablePath to get the executable path if using argv[0] fails

[Thiago Farina]

On Mon, Nov 29, 2010 at 2:57 PM, Jeremy Huddleston <jeremyhu@apple.com> wrote:
>
> Signed-off-by: Jeremy Huddleston <jeremyhu@apple.com>
> Reviewed-by: Matt Wright <mww@apple.com>
> ---
>  exec_cmd.c |   17 +++++++++++++++++
>  1 files changed, 17 insertions(+), 0 deletions(-)
>
> diff --git a/exec_cmd.c b/exec_cmd.c
> index bf22570..1e24a8f 100644
> --- a/exec_cmd.c
> +++ b/exec_cmd.c
> @@ -3,6 +3,10 @@
>  #include "quote.h"
>  #define MAX_ARGS       32
>
> +#if defined(__APPLE__) && defined(RUNTIME_PREFIX)
> +#include <mach-o/dyld.h>
> +#endif
> +
>  extern char **environ;
>  static const char *argv_exec_path;
>  static const char *argv0_path;
> @@ -53,6 +57,19 @@ const char *git_extract_argv0_path(const char *argv0)
>        if (slash >= argv0) {
>                argv0_path = xstrndup(argv0, slash - argv0);
>                return slash + 1;
> +#ifdef __APPLE__

Why not #if defined(__APPLE__), like above?

Re: [PATCH] Fallback on _NSGetExecutablePath to get the executable path if using argv[0] fails

[Jonathan Nieder]

Thiago Farina wrote:
> On Mon, Nov 29, 2010 at 2:57 PM, Jeremy Huddleston <jeremyhu@apple.com> wrote:

>> Signed-off-by: Jeremy Huddleston <jeremyhu@apple.com>
>> Reviewed-by: Matt Wright <mww@apple.com>

I like the idea, but could you add a short commit message
explaining the existing behavior and what improvement this makes?

> Why not #if defined(__APPLE__), like above?

More importantly, please search for #ifdef in existing code to get
some examples of how we like to do platform-specific things.

The section "2) #ifdefs are ugly" of
linux-2.6/Documentation/SubmittingPatches explains the rationale.

Regards,
Jonathan

[PATCH updated] Fallback on _NSGetExecutablePath to get the executable path if using argv[0] fails

[Jeremy Huddleston]

This adds better support for RUNTIME_PREFIX on Mac OS X.  The previous codepath
would only work if argv[0] contained the full path to the executable or $PATH
already contained /path/to/libexec/git-core.  We use _NSGetExecutablePath here
to find the full path (and thus prepend the correct libexec/git-core to $PATH)
in the case where argv[0] does not contain the full path to the executable.

Signed-off-by: Jeremy Huddleston <jeremyhu@apple.com>
Reviewed-by: Matt Wright <mww@apple.com>
---
 exec_cmd.c |   17 +++++++++++++++++
 1 files changed, 17 insertions(+), 0 deletions(-)

diff --git a/exec_cmd.c b/exec_cmd.c
index bf22570..182fd3a 100644
--- a/exec_cmd.c
+++ b/exec_cmd.c
@@ -3,6 +3,10 @@
 #include "quote.h"
 #define MAX_ARGS	32
 
+#if defined(__APPLE__) && defined(RUNTIME_PREFIX)
+#include <mach-o/dyld.h>
+#endif
+
 extern char **environ;
 static const char *argv_exec_path;
 static const char *argv0_path;
@@ -53,6 +57,19 @@ const char *git_extract_argv0_path(const char *argv0)
 	if (slash >= argv0) {
 		argv0_path = xstrndup(argv0, slash - argv0);
 		return slash + 1;
+#if defined(__APPLE__)
+	} else {
+		char new_argv0[PATH_MAX];
+		uint32_t new_argv0_s = PATH_MAX;
+		if(_NSGetExecutablePath(new_argv0, &new_argv0_s) == 0) {
+			slash = new_argv0 + strlen(new_argv0);
+			while (new_argv0 <= slash && !is_dir_sep(*slash))
+		                slash--;
+
+			if (slash >= new_argv0)
+				argv0_path = xstrndup(new_argv0, slash - new_argv0);
+		}
+#endif
 	}
 
 	return argv0;
-- 
1.7.3.2

Re: [PATCH] Fallback on _NSGetExecutablePath to get the executable path if using argv[0] fails

[Jeremy Huddleston]

On Nov 29, 2010, at 12:12, Jonathan Nieder wrote:

> Thiago Farina wrote:
>> On Mon, Nov 29, 2010 at 2:57 PM, Jeremy Huddleston <jeremyhu@apple.com> wrote:
> 
>>> Signed-off-by: Jeremy Huddleston <jeremyhu@apple.com>
>>> Reviewed-by: Matt Wright <mww@apple.com>
> 
> I like the idea, but could you add a short commit message
> explaining the existing behavior and what improvement this makes?

Hopefully what I resent is sufficient for explaining the changes.

>> Why not #if defined(__APPLE__), like above?

Originally for style.  I like #ifdef better than #if defined(), but I changed it for you in the resend.

> More importantly, please search for #ifdef in existing code to get
> some examples of how we like to do platform-specific things.

Yeah, I see some:
#ifdef _WIN32

which is why I used __APPLE__.  Do you have a better suggestion?

> The section "2) #ifdefs are ugly" of
> linux-2.6/Documentation/SubmittingPatches explains the rationale.

I agree, but I don't really see a way around it here since this API is specific to OS X.

Re: [PATCH updated] Fallback on _NSGetExecutablePath to get the executable path if using argv[0] fails

[Jonathan Nieder]

Jeremy Huddleston wrote:

> This adds better support for RUNTIME_PREFIX on Mac OS X.  The previous codepath
> would only work if argv[0] contained the full path to the executable or $PATH
> already contained /path/to/libexec/git-core.  We use _NSGetExecutablePath here
> to find the full path (and thus prepend the correct libexec/git-core to $PATH)
> in the case where argv[0] does not contain the full path to the executable.

Closer.  But that is perhaps too much at the level of code rather than
the user:

	Subject: MacOSX: Use _NSGetExecutablePath to get full argv[0] path

	When RUNTIME_PREFIX support is enabled (which is common on Mac OS X)
	the exec-path is derived from the program invocation path.
	Unfortunately, usual Unix semantics are for argv[0] to contain
	the path used to invoke a program rather than the path to the
	executable.  So usual invocations of git would not result in
	helpers from exec-path being found correctly:

		$ git fast-import
		... example output here ...

	So in the spirit of v1.6.0-rc1~21 (Windows: make sure argv[0]
	has a path, 2008-07-21), use _NSGetExecutablePath to find the full
	path to the git binary, avoiding such trouble.

> --- a/exec_cmd.c
> +++ b/exec_cmd.c
[...]
> @@ -53,6 +57,19 @@ const char *git_extract_argv0_path(const char *argv0)
>  	if (slash >= argv0) {
>  		argv0_path = xstrndup(argv0, slash - argv0);
>  		return slash + 1;
> +#if defined(__APPLE__)
> +	} else {
> +		char new_argv0[PATH_MAX];
> +		uint32_t new_argv0_s = PATH_MAX;
> +		if(_NSGetExecutablePath(new_argv0, &new_argv0_s) == 0) {
> +			slash = new_argv0 + strlen(new_argv0);
> +			while (new_argv0 <= slash && !is_dir_sep(*slash))
> +		                slash--;
> +
> +			if (slash >= new_argv0)
> +				argv0_path = xstrndup(new_argv0, slash - new_argv0);
> +		}
> +#endif

Can't this ifdef be avoided?  The ideal is for such code to be
abstracted away into helper functions in git-compat-util.h and compat/*.c.

Jonathan

Re: [PATCH] Fallback on _NSGetExecutablePath to get the executable path if using argv[0] fails

[Jonathan Nieder]

Jeremy Huddleston wrote:
> On Nov 29, 2010, at 12:12, Jonathan Nieder wrote:

>> The section "2) #ifdefs are ugly" of
>> linux-2.6/Documentation/SubmittingPatches explains the rationale.
>
> I agree, but I don't really see a way around it here since this API is specific to OS X.

Did you actually read that section? :)

Re: [PATCH] Fallback on _NSGetExecutablePath to get the executable path if using argv[0] fails

[Jeremy Huddleston]

On Nov 29, 2010, at 13:50, Jonathan Nieder wrote:

> Jeremy Huddleston wrote:
>> On Nov 29, 2010, at 12:12, Jonathan Nieder wrote:
> 
>>> The section "2) #ifdefs are ugly" of
>>> linux-2.6/Documentation/SubmittingPatches explains the rationale.
>> 
>> I agree, but I don't really see a way around it here since this API is specific to OS X.
> 
> Did you actually read that section? :)

Yes, but I don't have the time to "do it right" right now ... I'm contributing the patch that we are using back to the community in the spirit of OSS development, but I don't have the time resources currently to "do it right" at present.  I'll come back to it once time allows if nobody else picks it up.

Thanks,
Jeremy

Re: [PATCH] Fallback on _NSGetExecutablePath to get the executable path if using argv[0] fails

[Jonathan Nieder]

Jeremy Huddleston wrote:
> On Nov 29, 2010, at 13:50, Jonathan Nieder wrote:
>> Jeremy Huddleston wrote:
>>> On Nov 29, 2010, at 12:12, Jonathan Nieder wrote:

>>>> The section "2) #ifdefs are ugly" of
>>>> linux-2.6/Documentation/SubmittingPatches explains the rationale.
>>> 
>>> I agree, but I don't really see a way around it here since this API is specific to OS X.
>> 
>> Did you actually read that section? :)
>
> Yes, but I don't have the time to "do it right" right now ... I'm
> contributing the patch that we are using back to the community in
> the spirit of OSS development, but I don't have the time resources
> currently to "do it right" at present.  I'll come back to it once
> time allows if nobody else picks it up.

Okay.  Thanks for reporting.

My guess is that the Windows version could be simplified, too, if
we introduce a function to get the path to the binary.  On Linux
it should use "readlink /proc/$$/exe", on Darwin the function you
pointed to, on Win32 _pgmptr, as a fallback look for argv[0] in
$PATH if someone on another platform is interested.

Jonathan

Re: [PATCH updated] Fallback on _NSGetExecutablePath to get the executable path if using argv[0] fails

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

>> --- a/exec_cmd.c
>> +++ b/exec_cmd.c
> [...]
>> @@ -53,6 +57,19 @@ const char *git_extract_argv0_path(const char *argv0)
>>  	if (slash >= argv0) {
>>  		argv0_path = xstrndup(argv0, slash - argv0);
>>  		return slash + 1;
>> +#if defined(__APPLE__)
>> +	} else {
>> +		char new_argv0[PATH_MAX];
>> +		uint32_t new_argv0_s = PATH_MAX;
>> +		if(_NSGetExecutablePath(new_argv0, &new_argv0_s) == 0) {
>> +			slash = new_argv0 + strlen(new_argv0);
>> +			while (new_argv0 <= slash && !is_dir_sep(*slash))
>> +		                slash--;
>> +
>> +			if (slash >= new_argv0)
>> +				argv0_path = xstrndup(new_argv0, slash - new_argv0);
>> +		}
>> +#endif
>
> Can't this ifdef be avoided?  The ideal is for such code to be
> abstracted away into helper functions in git-compat-util.h and compat/*.c.

I had exactly the same reaction.  Also doesn't the above need to be
protected by defined(RUNTIME_PREFIX), too?

Re: [PATCH updated] Fallback on _NSGetExecutablePath to get the executable path if using argv[0] fails

[Jeremy Huddleston]

On Nov 29, 2010, at 15:24, Junio C Hamano wrote:

> Jonathan Nieder <jrnieder@gmail.com> writes:
> 
>>> --- a/exec_cmd.c
>>> +++ b/exec_cmd.c
>> [...]
>>> @@ -53,6 +57,19 @@ const char *git_extract_argv0_path(const char *argv0)
>>> 	if (slash >= argv0) {
>>> 		argv0_path = xstrndup(argv0, slash - argv0);
>>> 		return slash + 1;
>>> +#if defined(__APPLE__)
>>> +	} else {
>>> +		char new_argv0[PATH_MAX];
>>> +		uint32_t new_argv0_s = PATH_MAX;
>>> +		if(_NSGetExecutablePath(new_argv0, &new_argv0_s) == 0) {
>>> +			slash = new_argv0 + strlen(new_argv0);
>>> +			while (new_argv0 <= slash && !is_dir_sep(*slash))
>>> +		                slash--;
>>> +
>>> +			if (slash >= new_argv0)
>>> +				argv0_path = xstrndup(new_argv0, slash - new_argv0);
>>> +		}
>>> +#endif
>> 
>> Can't this ifdef be avoided?  The ideal is for such code to be
>> abstracted away into helper functions in git-compat-util.h and compat/*.c.
> 
> I had exactly the same reaction.  Also doesn't the above need to be
> protected by defined(RUNTIME_PREFIX), too?

It already is inside of an #ifdef RUNTIME_PREFIX block.

Re: [PATCH] Fallback on _NSGetExecutablePath to get the executable path if using argv[0] fails

[Kevin Ballard]

On Nov 29, 2010, at 9:12 AM, Jonathan Nieder wrote:

> The section "2) #ifdefs are ugly" of
> linux-2.6/Documentation/SubmittingPatches explains the rationale.

Might this be worth pulling into git.git/Documentation/CodingGuidelines?

-Kevin Ballard

Re: [PATCH] Fallback on _NSGetExecutablePath to get the executable path if using argv[0] fails

[Jonathan Nieder]

Kevin Ballard wrote:
> On Nov 29, 2010, at 9:12 AM, Jonathan Nieder wrote:

>> The section "2) #ifdefs are ugly" of
>> linux-2.6/Documentation/SubmittingPatches explains the rationale.
>
> Might this be worth pulling into git.git/Documentation/CodingGuidelines?

Yes, the example there includes good advice I wish I had received
sooner, and Documentation/CodingGuidelines seems like a good place to
help people find it.  Do you have some wording in mind?

Re: [PATCH] Fallback on _NSGetExecutablePath to get the executable path if using argv[0] fails

[Kevin Ballard]

On Dec 2, 2010, at 11:42 PM, Jonathan Nieder wrote:

> Kevin Ballard wrote:
>> On Nov 29, 2010, at 9:12 AM, Jonathan Nieder wrote:
> 
>>> The section "2) #ifdefs are ugly" of
>>> linux-2.6/Documentation/SubmittingPatches explains the rationale.
>> 
>> Might this be worth pulling into git.git/Documentation/CodingGuidelines?
> 
> Yes, the example there includes good advice I wish I had received
> sooner, and Documentation/CodingGuidelines seems like a good place to
> help people find it.  Do you have some wording in mind?

Not particularly. It just seems that if we're going to point people at the
linux-2.6 documentation, then what we're referencing should be pulled into
our own docs. It's not reasonable to expect people to read the documentation
from another project to find out what they should do in this one.

-Kevin Ballard