[PATCH] Use is_absolute_path() in diff-lib.c, lockfile.c, setup.c, trace.c

[PATCH] Use is_absolute_path() in diff-lib.c, lockfile.c, setup.c, trace.c

[Steffen Prohaska]

Using the helper function to test for absolute paths makes porting easier.

Signed-off-by: Steffen Prohaska <prohaska@zib.de>
---
 diff-lib.c |    2 +-
 lockfile.c |    2 +-
 setup.c    |    2 +-
 trace.c    |    2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

BTW, what happend to the msysgit related patches:

[PATCH 1/3] sha1_file.c: Fix size_t related printf format warnings
[PATCH 2/3] builtin-init-db: use get_git_dir() instead of getenv()

I never received comments about them, nor do I find them on pu.

    Steffen

diff --git a/diff-lib.c b/diff-lib.c
index f8e936a..d85d8f3 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -231,7 +231,7 @@ static int handle_diff_files_args(struct rev_info *revs,
 static int is_outside_repo(const char *path, int nongit, const char *prefix)
 {
 	int i;
-	if (nongit || !strcmp(path, "-") || path[0] == '/')
+	if (nongit || !strcmp(path, "-") || is_absolute_path(path))
 		return 1;
 	if (prefixcmp(path, "../"))
 		return 0;
diff --git a/lockfile.c b/lockfile.c
index 258fb3f..f45d3ed 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -94,7 +94,7 @@ static char *resolve_symlink(char *p, size_t s)
 			return p;
 		}
 
-		if (link[0] == '/') {
+		if (is_absolute_path(link)) {
 			/* absolute path simply replaces p */
 			if (link_len < s)
 				strcpy(p, link);
diff --git a/setup.c b/setup.c
index 43cd3f9..2c7b5cb 100644
--- a/setup.c
+++ b/setup.c
@@ -59,7 +59,7 @@ const char *prefix_path(const char *prefix, int len, const char *path)
 const char *prefix_filename(const char *pfx, int pfx_len, const char *arg)
 {
 	static char path[PATH_MAX];
-	if (!pfx || !*pfx || arg[0] == '/')
+	if (!pfx || !*pfx || is_absolute_path(arg))
 		return arg;
 	memcpy(path, pfx, pfx_len);
 	strcpy(path + pfx_len, arg);
diff --git a/trace.c b/trace.c
index 0d89dbe..d3d1b6d 100644
--- a/trace.c
+++ b/trace.c
@@ -37,7 +37,7 @@ static int get_trace_fd(int *need_close)
 		return STDERR_FILENO;
 	if (strlen(trace) == 1 && isdigit(*trace))
 		return atoi(trace);
-	if (*trace == '/') {
+	if (is_absolute_path(trace)) {
 		int fd = open(trace, O_WRONLY | O_APPEND | O_CREAT, 0666);
 		if (fd == -1) {
 			fprintf(stderr,
-- 
1.5.3.6.878.g8c9e2

Re: [PATCH] Use is_absolute_path() in diff-lib.c, lockfile.c, setup.c, trace.c

[Junio C Hamano]

Steffen Prohaska <prohaska@zib.de> writes:

> Using the helper function to test for absolute paths makes porting easier.

These probably make sense.  I obviously do not see any downside from the
POSIX side, and can imagine that treating "C:\" prefix as "absolute
paths" at these four places will not have any ill effect on the Windows
side (IOW, the codepaths that follow these four places seem to do a
sensible thing even if the "absolute path" prefix is not a single '/',
but would work fine as-is).

I am a bit surprised that there are only four places you needed to
touch, though.

> BTW, what happend to the msysgit related patches:
>
> [PATCH 1/3] sha1_file.c: Fix size_t related printf format warnings
> [PATCH 2/3] builtin-init-db: use get_git_dir() instead of getenv()
>
> I never received comments about them, nor do I find them on pu.

Lack of comments was probably due to mixture of bad timing and general
lack of interests.  Many people are busy working on their turkeys than
hacking this time of the year ;-)

I am reluctant to queue msysgit/gitwin related patches without seeing
positive comments from other people involved on the Windows side, unless
they are trivial and obvious improvements.

 * [1/3] seems without harm but on the other hand does not seem so
   urgent either.

 * [2/3] may introduce chicken-and-egg problem (use of get_git_dir()
   inside git-init feels quite iffy, as it calls setup_git_env(), which
   does repository discovery), without an obvious and clear advantage.

For these reasons, both of them disqualify from being trivial and
obvious improvements, so I did not pick them up unilaterally before
seeing positive comments from other people.

Re: [PATCH] Use is_absolute_path() in diff-lib.c, lockfile.c, setup.c, trace.c

[Steffen Prohaska]

On Nov 26, 2007, at 4:54 AM, Junio C Hamano wrote:

> Steffen Prohaska <prohaska@zib.de> writes:
>
>> Using the helper function to test for absolute paths makes porting  
>> easier.
>
> These probably make sense.  I obviously do not see any downside  
> from the
> POSIX side, and can imagine that treating "C:\" prefix as "absolute
> paths" at these four places will not have any ill effect on the  
> Windows
> side (IOW, the codepaths that follow these four places seem to do a
> sensible thing even if the "absolute path" prefix is not a single '/',
> but would work fine as-is).
>
> I am a bit surprised that there are only four places you needed to
> touch, though.

Yes, I was a bit surprised, too.  I used grep to find these places.
Maybe my regular expression was not good enough.  On the other side,
is_absolute_path() is already used at 11 places before this patch.
I also cross checked with the msysgit code base.  It does not use
is_absolute_path() at more places.


>> BTW, what happend to the msysgit related patches:
>>
>> [PATCH 1/3] sha1_file.c: Fix size_t related printf format warnings
>> [PATCH 2/3] builtin-init-db: use get_git_dir() instead of getenv()
>>
>> I never received comments about them, nor do I find them on pu.
>
> Lack of comments was probably due to mixture of bad timing and general
> lack of interests.  Many people are busy working on their turkeys than
> hacking this time of the year ;-)

Yeah, list traffic was quiet low.


> I am reluctant to queue msysgit/gitwin related patches without seeing
> positive comments from other people involved on the Windows side,  
> unless
> they are trivial and obvious improvements.
>
>  * [1/3] seems without harm but on the other hand does not seem so
>    urgent either.

I did not find a simpler way to achieve a compile free warning on
mingw, without introducing more complex ifdefs.  I'm currently trying
to reduce the differences between git.git, mingw, and msysgit.


>  * [2/3] may introduce chicken-and-egg problem (use of get_git_dir()
>    inside git-init feels quite iffy, as it calls setup_git_env(),  
> which
>    does repository discovery), without an obvious and clear advantage.

I see.  I'll rethink 2/3 and 3/3.  Either I come up with more
convincing arguments or I'll try if the changes can be reverted
in the msysgit code base.


> For these reasons, both of them disqualify from being trivial and
> obvious improvements, so I did not pick them up unilaterally before
> seeing positive comments from other people.

	Steffen

Re: [PATCH] Use is_absolute_path() in diff-lib.c, lockfile.c, setup.c, trace.c

[Johannes Sixt]

Steffen Prohaska schrieb:
> Using the helper function to test for absolute paths makes porting easier.

The patch looks good.

> --- a/setup.c
> +++ b/setup.c
> @@ -59,7 +59,7 @@ const char *prefix_path(const char *prefix, int len, const char *path)
>  const char *prefix_filename(const char *pfx, int pfx_len, const char *arg)
>  {
>  	static char path[PATH_MAX];
> -	if (!pfx || !*pfx || arg[0] == '/')
> +	if (!pfx || !*pfx || is_absolute_path(arg))
>  		return arg;
>  	memcpy(path, pfx, pfx_len);
>  	strcpy(path + pfx_len, arg);

This instance, however, will be reworked for Windows anyway because we must 
do '\\' to '/' conversion even if the path is absolute (and, hence, we 
cannot just return the input).

But disregarding this note, the change makes the code more readable, IMO, so 
I'm all for it.

-- Hannes