{"thread":{"id":"33279","subject":"[PATCH 2/2] optimize set_shared_perm()","startedAt":"2013-03-25T15:57:56Z","lastAt":"2013-03-30T09:53:16Z","messageCount":3,"participants":["Torsten Bögershausen","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"212173","messageId":"201303251657.57222.tboegi@web.de","threadId":"33279","inReplyTo":null,"subject":"[PATCH 2/2] optimize set_shared_perm()","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2013-03-25T15:57:56Z","receivedAt":"2013-03-25T15:57:56Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"  optimize set_shared_perm() in path.c:\n\n  - sometimes the chown() function is called even when not needed.\n    (This can be provoced by running t1301, and adding some debug code)\n\n    Save a chmod from 400 to 400, or from 600->600 on these files:\n    .git/info/refs+\n    .git/objects/info/packs+\n\n    Save chmod on directories from 2770 to 2770:\n    .git/refs\n    .git/refs/heads\n    .git/refs/tags\n\n  - as all callers use mode == 0 when caling set_shared_perm(),\n    the function can be simplified.\n  - all callers use the macro adjust_shared_perm(path) from cache.h\n    Convert adjust_shared_perm() from a macro into a function prototype\n\nSigned-off-by: Torsten Bögershausen <tboegi@web.de>\n---\n cache.h |  3 +--\n path.c  | 71 +++++++++++++++++++++++++++++++++++------------------------------\n 2 files changed, 39 insertions(+), 35 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 59e5b53..65a9db7 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -727,8 +727,7 @@ enum sharedrepo {\n \tPERM_EVERYBODY      = 0664\n };\n int git_config_perm(const char *var, const char *value);\n-int set_shared_perm(const char *path, int mode);\n-#define adjust_shared_perm(path) set_shared_perm((path), 0)\n+int adjust_shared_perm(const char *path);\n int safe_create_leading_directories(char *path);\n int safe_create_leading_directories_const(const char *path);\n int mkdir_in_gitdir(const char *path);\ndiff --git a/path.c b/path.c\nindex 2fdccc2..4bc918a 100644\n--- a/path.c\n+++ b/path.c\n@@ -1,14 +1,5 @@\n /*\n- * I'm tired of doing \"vsnprintf()\" etc just to open a\n- * file, so here's a \"return static buffer with printf\"\n- * interface for paths.\n- *\n- * It's obviously not thread-safe. Sue me. But it's quite\n- * useful for doing things like\n- *\n- *   f = open(mkpath(\"%s/%s.git\", base, name), O_RDONLY);\n- *\n- * which is what it's designed for.\n+ * Different utilitiy functions for path and path names\n  */\n #include \"cache.h\"\n #include \"strbuf.h\"\n@@ -105,6 +96,13 @@ char *git_pathdup(const char *fmt, ...)\n \treturn xstrdup(ret);\n }\n \n+/*\n+ * I'm tired of doing \"vsnprintf()\" etc just to open a\n+ * file, so here's an interface for paths.\n+ *\n+ * f = open(mkpath(\"%s/%s.git\", base, name), O_RDONLY);\n+ *\n+ */\n char *mkpathdup(const char *fmt, ...)\n {\n \tchar *path;\n@@ -405,22 +403,13 @@ const char *enter_repo(const char *path, int strict)\n \treturn NULL;\n }\n \n-int set_shared_perm(const char *path, int mode)\n+static int calc_shared_perm(int mode)\n {\n-\tint tweak, shared, orig_mode;\n+\tint tweak, shared;\n \n-\tif (!shared_repository) {\n-\t\tif (mode)\n-\t\t\treturn chmod(path, mode & ~S_IFMT);\n-\t\treturn 0;\n-\t}\n-\tif (!mode) {\n-\t\tif (get_st_mode_bits(path, &mode) < 0)\n-\t\t\treturn -1;\n-\t\torig_mode = mode;\n-\t} else\n-\t\torig_mode = 0;\n-\tif (shared_repository < 0)\n+\tif (!shared_repository)\n+\t\treturn mode;\n+\telse if (shared_repository < 0)\n \t\tshared = -shared_repository;\n \telse\n \t\tshared = shared_repository;\n@@ -436,16 +425,32 @@ int set_shared_perm(const char *path, int mode)\n \telse\n \t\tmode |= tweak;\n \n-\tif (S_ISDIR(mode)) {\n-\t\t/* Copy read bits to execute bits */\n-\t\tmode |= (shared & 0444) >> 2;\n-\t\tmode |= FORCE_DIR_SET_GID;\n-\t}\n+\treturn mode;\n+}\n+\n+static int calc_shared_perm_dir(int mode)\n+{\n+\tmode = calc_shared_perm(mode);\n+\n+\t/* Copy read bits to execute bits */\n+\tmode |= (mode & 0444) >> 2;\n+\tmode |= FORCE_DIR_SET_GID;\n+\treturn mode;\n+}\n+\n+int adjust_shared_perm(const char *path)\n+{\n+\tint old_mode, new_mode;\n+\tif (get_st_mode_bits(path, &old_mode) < 0)\n+\t\treturn -1;\n+\n+\tif (S_ISDIR(old_mode))\n+\t\tnew_mode = calc_shared_perm_dir(old_mode);\n+\telse\n+\t\tnew_mode = calc_shared_perm(old_mode);\n \n-\tif (((shared_repository < 0\n-\t      ? (orig_mode & (FORCE_DIR_SET_GID | 0777))\n-\t      : (orig_mode & mode)) != mode) &&\n-\t    chmod(path, (mode & ~S_IFMT)) < 0)\n+\tif (((old_mode ^ new_mode) & ~S_IFMT) &&\n+\t\t\tchmod(path, (new_mode & ~S_IFMT)) < 0)\n \t\treturn -2;\n \treturn 0;\n }\n-- \n1.8.2.341.g543621f\n"},{"id":"212651","messageId":"7vli95nbs4.fsf@alter.siamese.dyndns.org","threadId":"33279","inReplyTo":"201303251657.57222.tboegi@web.de","subject":"Re: [PATCH 2/2] optimize set_shared_perm()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-29T21:20:27Z","receivedAt":"2013-03-29T21:20:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Torsten Bögershausen <tboegi@web.de> writes:\n\n>   optimize set_shared_perm() in path.c:\n>\n>   - sometimes the chown() function is called even when not needed.\n>     (This can be provoced by running t1301, and adding some debug code)\n>\n>     Save a chmod from 400 to 400, or from 600->600 on these files:\n>     .git/info/refs+\n>     .git/objects/info/packs+\n>\n>     Save chmod on directories from 2770 to 2770:\n>     .git/refs\n>     .git/refs/heads\n>     .git/refs/tags\n>\n>   - as all callers use mode == 0 when caling set_shared_perm(),\n>     the function can be simplified.\n>   - all callers use the macro adjust_shared_perm(path) from cache.h\n>     Convert adjust_shared_perm() from a macro into a function prototype\n\nThe last two points can become a separate \"preparation\" step.  The\nresult would be easier to read.\n\nYour updated adjust_shared_perm() does not begin with:\n\n\tif (!shared_repository)\n        \treturn 0;\n\nas the original, but it always first calls to get_st_mode_bits()\nwhich makes a call to lstat(2).\n\nThat smells like a huge regression for !shared_repository case,\nunless you have updated the existing callers of adjust_shared_perm()\nnot to call it when !shared_repository.\n\n> diff --git a/path.c b/path.c\n> index 2fdccc2..4bc918a 100644\n> --- a/path.c\n> +++ b/path.c\n> @@ -1,14 +1,5 @@\n>  /*\n> - * I'm tired of doing \"vsnprintf()\" etc just to open a\n> - * file, so here's a \"return static buffer with printf\"\n> - * interface for paths.\n> - *\n> - * It's obviously not thread-safe. Sue me. But it's quite\n> - * useful for doing things like\n> - *\n> - *   f = open(mkpath(\"%s/%s.git\", base, name), O_RDONLY);\n> - *\n> - * which is what it's designed for.\n> + * Different utilitiy functions for path and path names\n>   */\n\nRemoving the stale \"I'm tired of\" is good; you do not have to move\nit anywhere.  A single-liner\n\n\t/* Utilities for paths and pathnames */\n\nshould suffice.\n\nWill discard this step (but will keep PATCH 1/2).\n"},{"id":"212695","messageId":"5156B60C.6070707@web.de","threadId":"33279","inReplyTo":"7vli95nbs4.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/2] optimize set_shared_perm()","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2013-03-30T09:53:16Z","receivedAt":"2013-03-30T09:53:16Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 29.03.13 22:20, Junio C Hamano wrote:\n[snip]\n> The last two points can become a separate \"preparation\" step.  The\n> result would be easier to read.\n> \n> Your updated adjust_shared_perm() does not begin with:\n> \n> \tif (!shared_repository)\n>         \treturn 0;\n> \n> as the original, but it always first calls to get_st_mode_bits()\n> which makes a call to lstat(2).\n> \n> That smells like a huge regression for !shared_repository case,\n> unless you have updated the existing callers of adjust_shared_perm()\n> not to call it when !shared_repository.\n> \nThanks for carefull review:\nThe achieved effect of the code is the same,\nbut the developer is to blame.\n\nI send a new patch of 2/2 in a minute, splitted into 2 commits.\n\nHighlights of part 2:\na) move \"if (!shared_repository)\" to the right place\nb) Simplify calc_shared_perm() even more: Remove the variable \"int shared\"\nc) Remove calc_shared_perm_dir(), the functionality is baked into\n   adjust_shared_perm()\n"}]}