{"thread":{"id":"11017","subject":"[PATCH] Use is_absolute_path() in diff-lib.c, lockfile.c, setup.c, trace.c","startedAt":"2007-11-25T22:29:03Z","lastAt":"2007-11-26T07:45:21Z","messageCount":4,"participants":["Steffen Prohaska","Junio C Hamano","Johannes Sixt"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"60881","messageId":"11960297431954-git-send-email-prohaska@zib.de","threadId":"11017","inReplyTo":null,"subject":"[PATCH] Use is_absolute_path() in diff-lib.c, lockfile.c, setup.c, trace.c","fromName":"Steffen Prohaska","fromEmail":"prohaska@zib.de","sentAt":"2007-11-25T22:29:03Z","receivedAt":"2007-11-25T22:29:03Z","isPatch":true,"sender":{"key":"prohaska@zib.de","avatar":"https://avatars.githubusercontent.com/u/217580?v=4"},"body":"Using the helper function to test for absolute paths makes porting easier.\n\nSigned-off-by: Steffen Prohaska \n---\n diff-lib.c | 2 +-\n lockfile.c | 2 +-\n setup.c | 2 +-\n trace.c | 2 +-\n 4 files changed, 4 insertions(+), 4 deletions(-)\n\nBTW, what happend to the msysgit related patches:\n\n[PATCH 1/3] sha1_file.c: Fix size_t related printf format warnings\n[PATCH 2/3] builtin-init-db: use get_git_dir() instead of getenv()\n\nI never received comments about them, nor do I find them on pu.\n\n Steffen\n\ndiff --git a/diff-lib.c b/diff-lib.c\nindex f8e936a..d85d8f3 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -231,7 +231,7 @@ static int handle_diff_files_args(struct rev_info *revs,\n static int is_outside_repo(const char *path, int nongit, const char *prefix)\n {\n \tint i;\n-\tif (nongit || !strcmp(path, \"-\") || path[0] == '/')\n+\tif (nongit || !strcmp(path, \"-\") || is_absolute_path(path))\n \t\treturn 1;\n \tif (prefixcmp(path, \"../\"))\n \t\treturn 0;\ndiff --git a/lockfile.c b/lockfile.c\nindex 258fb3f..f45d3ed 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -94,7 +94,7 @@ static char *resolve_symlink(char *p, size_t s)\n \t\t\treturn p;\n \t\t}\n \n-\t\tif (link[0] == '/') {\n+\t\tif (is_absolute_path(link)) {\n \t\t\t/* absolute path simply replaces p */\n \t\t\tif (link_len < s)\n \t\t\t\tstrcpy(p, link);\ndiff --git a/setup.c b/setup.c\nindex 43cd3f9..2c7b5cb 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -59,7 +59,7 @@ const char *prefix_path(const char *prefix, int len, const char *path)\n const char *prefix_filename(const char *pfx, int pfx_len, const char *arg)\n {\n \tstatic char path[PATH_MAX];\n-\tif (!pfx || !*pfx || arg[0] == '/')\n+\tif (!pfx || !*pfx || is_absolute_path(arg))\n \t\treturn arg;\n \tmemcpy(path, pfx, pfx_len);\n \tstrcpy(path + pfx_len, arg);\ndiff --git a/trace.c b/trace.c\nindex 0d89dbe..d3d1b6d 100644\n--- a/trace.c\n+++ b/trace.c\n@@ -37,7 +37,7 @@ static int get_trace_fd(int *need_close)\n \t\treturn STDERR_FILENO;\n \tif (strlen(trace) == 1 && isdigit(*trace))\n \t\treturn atoi(trace);\n-\tif (*trace == '/') {\n+\tif (is_absolute_path(trace)) {\n \t\tint fd = open(trace, O_WRONLY | O_APPEND | O_CREAT, 0666);\n \t\tif (fd == -1) {\n \t\t\tfprintf(stderr,\n-- \n1.5.3.6.878.g8c9e2\n"},{"id":"60900","messageId":"7vy7clhd9z.fsf@gitster.siamese.dyndns.org","threadId":"11017","inReplyTo":"11960297431954-git-send-email-prohaska@zib.de","subject":"Re: [PATCH] Use is_absolute_path() in diff-lib.c, lockfile.c, setup.c, trace.c","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-11-26T03:54:48Z","receivedAt":"2007-11-26T03:54:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Steffen Prohaska writes:\n\n> Using the helper function to test for absolute paths makes porting easier.\n\nThese probably make sense. I obviously do not see any downside from the\nPOSIX side, and can imagine that treating \"C:\\\" prefix as \"absolute\npaths\" at these four places will not have any ill effect on the Windows\nside (IOW, the codepaths that follow these four places seem to do a\nsensible thing even if the \"absolute path\" prefix is not a single '/',\nbut would work fine as-is).\n\nI am a bit surprised that there are only four places you needed to\ntouch, though.\n\n> BTW, what happend to the msysgit related patches:\n>\n> [PATCH 1/3] sha1_file.c: Fix size_t related printf format warnings\n> [PATCH 2/3] builtin-init-db: use get_git_dir() instead of getenv()\n>\n> I never received comments about them, nor do I find them on pu.\n\nLack of comments was probably due to mixture of bad timing and general\nlack of interests. Many people are busy working on their turkeys than\nhacking this time of the year ;-)\n\nI am reluctant to queue msysgit/gitwin related patches without seeing\npositive comments from other people involved on the Windows side, unless\nthey are trivial and obvious improvements.\n\n * [1/3] seems without harm but on the other hand does not seem so\n urgent either.\n\n * [2/3] may introduce chicken-and-egg problem (use of get_git_dir()\n inside git-init feels quite iffy, as it calls setup_git_env(), which\n does repository discovery), without an obvious and clear advantage.\n\nFor these reasons, both of them disqualify from being trivial and\nobvious improvements, so I did not pick them up unilaterally before\nseeing positive comments from other people.\n"},{"id":"60911","messageId":"F8F93DBB-F0F5-440A-BA0C-434422C8122A@zib.de","threadId":"11017","inReplyTo":"7vy7clhd9z.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Use is_absolute_path() in diff-lib.c, lockfile.c, setup.c, trace.c","fromName":"Steffen Prohaska","fromEmail":"prohaska@zib.de","sentAt":"2007-11-26T06:45:55Z","receivedAt":"2007-11-26T06:45:55Z","isPatch":true,"sender":{"key":"prohaska@zib.de","avatar":"https://avatars.githubusercontent.com/u/217580?v=4"},"body":"\nOn Nov 26, 2007, at 4:54 AM, Junio C Hamano wrote:\n\n> Steffen Prohaska writes:\n>\n>> Using the helper function to test for absolute paths makes porting \n>> easier.\n>\n> These probably make sense. I obviously do not see any downside \n> from the\n> POSIX side, and can imagine that treating \"C:\\\" prefix as \"absolute\n> paths\" at these four places will not have any ill effect on the \n> Windows\n> side (IOW, the codepaths that follow these four places seem to do a\n> sensible thing even if the \"absolute path\" prefix is not a single '/',\n> but would work fine as-is).\n>\n> I am a bit surprised that there are only four places you needed to\n> touch, though.\n\nYes, I was a bit surprised, too. I used grep to find these places.\nMaybe my regular expression was not good enough. On the other side,\nis_absolute_path() is already used at 11 places before this patch.\nI also cross checked with the msysgit code base. It does not use\nis_absolute_path() at more places.\n\n\n>> BTW, what happend to the msysgit related patches:\n>>\n>> [PATCH 1/3] sha1_file.c: Fix size_t related printf format warnings\n>> [PATCH 2/3] builtin-init-db: use get_git_dir() instead of getenv()\n>>\n>> I never received comments about them, nor do I find them on pu.\n>\n> Lack of comments was probably due to mixture of bad timing and general\n> lack of interests. Many people are busy working on their turkeys than\n> hacking this time of the year ;-)\n\nYeah, list traffic was quiet low.\n\n\n> I am reluctant to queue msysgit/gitwin related patches without seeing\n> positive comments from other people involved on the Windows side, \n> unless\n> they are trivial and obvious improvements.\n>\n> * [1/3] seems without harm but on the other hand does not seem so\n> urgent either.\n\nI did not find a simpler way to achieve a compile free warning on\nmingw, without introducing more complex ifdefs. I'm currently trying\nto reduce the differences between git.git, mingw, and msysgit.\n\n\n> * [2/3] may introduce chicken-and-egg problem (use of get_git_dir()\n> inside git-init feels quite iffy, as it calls setup_git_env(), \n> which\n> does repository discovery), without an obvious and clear advantage.\n\nI see. I'll rethink 2/3 and 3/3. Either I come up with more\nconvincing arguments or I'll try if the changes can be reverted\nin the msysgit code base.\n\n\n> For these reasons, both of them disqualify from being trivial and\n> obvious improvements, so I did not pick them up unilaterally before\n> seeing positive comments from other people.\n\n\tSteffen\n"},{"id":"60912","messageId":"474A7991.5010202@viscovery.net","threadId":"11017","inReplyTo":"11960297431954-git-send-email-prohaska@zib.de","subject":"Re: [PATCH] Use is_absolute_path() in diff-lib.c, lockfile.c, setup.c, trace.c","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2007-11-26T07:45:21Z","receivedAt":"2007-11-26T07:45:21Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Steffen Prohaska schrieb:\n> Using the helper function to test for absolute paths makes porting easier.\n\nThe patch looks good.\n\n> --- a/setup.c\n> +++ b/setup.c\n> @@ -59,7 +59,7 @@ const char *prefix_path(const char *prefix, int len, const char *path)\n> const char *prefix_filename(const char *pfx, int pfx_len, const char *arg)\n> {\n> \tstatic char path[PATH_MAX];\n> -\tif (!pfx || !*pfx || arg[0] == '/')\n> +\tif (!pfx || !*pfx || is_absolute_path(arg))\n> \t\treturn arg;\n> \tmemcpy(path, pfx, pfx_len);\n> \tstrcpy(path + pfx_len, arg);\n\nThis instance, however, will be reworked for Windows anyway because we must \ndo '\\\\' to '/' conversion even if the path is absolute (and, hence, we \ncannot just return the input).\n\nBut disregarding this note, the change makes the code more readable, IMO, so \nI'm all for it.\n\n-- Hannes\n"}]}