{"thread":{"id":"35591","subject":"[PATCH] gc: notice gc processes run by other users","startedAt":"2013-12-31T12:07:39Z","lastAt":"2014-01-03T00:15:11Z","messageCount":5,"participants":["Kyle J. McKay","Duy Nguyen","Thiago Farina","Kyle","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"232542","messageId":"720d7a5676f8cbfc76c80198f9d3816@74d39fa044aa309eaea14b9f57fe79c","threadId":"35591","inReplyTo":null,"subject":"[PATCH] gc: notice gc processes run by other users","fromName":"Kyle J. McKay","fromEmail":"mackyle@gmail.com","sentAt":"2013-12-31T12:07:39Z","receivedAt":"2013-12-31T12:07:39Z","isPatch":true,"sender":{"key":"mackyle@gmail.com","avatar":"https://avatars.githubusercontent.com/u/813346?v=4"},"body":"Since 64a99eb4 git gc refuses to run without the --force option if\nanother gc process on the same repository is already running.\n\nHowever, if the repository is shared and user A runs git gc on the\nrepository and while that gc is still running user B runs git gc on\nthe same repository the gc process run by user A will not be noticed\nand the gc run by user B will go ahead and run.\n\nThe problem is that the kill(pid, 0) test fails with an EPERM error\nsince user B is not allowed to signal processes owned by user A\n(unless user B is root).\n\nUpdate the test to recognize an EPERM error as meaning the process\nexists and another gc should not be run (unless --force is given).\n---\n\nI suggest this be included in maint as others may also have expected the\nshared repository, different user gc scenario to be caught by the new\ncode when in fact it's not without this patch.\n\n builtin/gc.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex c14190f8..25f2237c 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -222,7 +222,7 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)\n \t\t\ttime(NULL) - st.st_mtime <= 12 * 3600 &&\n \t\t\tfscanf(fp, \"%\"PRIuMAX\" %127c\", &pid, locking_host) == 2 &&\n \t\t\t/* be gentle to concurrent \"gc\" on remote hosts */\n-\t\t\t(strcmp(locking_host, my_host) || !kill(pid, 0));\n+\t\t\t(strcmp(locking_host, my_host) || !kill(pid, 0) || errno == EPERM);\n \t\tif (fp != NULL)\n \t\t\tfclose(fp);\n \t\tif (should_exit) {\n-- \n1.8.5.2\n"},{"id":"232543","messageId":"CACsJy8AABkF8tq5sU+HxtNPuWTTAVwau-vUJq8SjhiDxawQ4jw@mail.gmail.com","threadId":"35591","inReplyTo":"720d7a5676f8cbfc76c80198f9d3816@74d39fa044aa309eaea14b9f57fe79c","subject":"Re: [PATCH] gc: notice gc processes run by other users","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-12-31T13:46:14Z","receivedAt":"2013-12-31T13:46:14Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Dec 31, 2013 at 7:07 PM, Kyle J. McKay <mackyle@gmail.com> wrote:\n> Since 64a99eb4 git gc refuses to run without the --force option if\n> another gc process on the same repository is already running.\n>\n> However, if the repository is shared and user A runs git gc on the\n> repository and while that gc is still running user B runs git gc on\n> the same repository the gc process run by user A will not be noticed\n> and the gc run by user B will go ahead and run.\n>\n> The problem is that the kill(pid, 0) test fails with an EPERM error\n> since user B is not allowed to signal processes owned by user A\n> (unless user B is root).\n>\n> Update the test to recognize an EPERM error as meaning the process\n> exists and another gc should not be run (unless --force is given).\n\nAck. Looking at kill(2) the other errors are EINVAL and ESRCH, which\nare fine to ignore.\n\n> ---\n>\n> I suggest this be included in maint as others may also have expected the\n> shared repository, different user gc scenario to be caught by the new\n> code when in fact it's not without this patch.\n>\n>  builtin/gc.c | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/builtin/gc.c b/builtin/gc.c\n> index c14190f8..25f2237c 100644\n> --- a/builtin/gc.c\n> +++ b/builtin/gc.c\n> @@ -222,7 +222,7 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)\n>                         time(NULL) - st.st_mtime <= 12 * 3600 &&\n>                         fscanf(fp, \"%\"PRIuMAX\" %127c\", &pid, locking_host) == 2 &&\n>                         /* be gentle to concurrent \"gc\" on remote hosts */\n> -                       (strcmp(locking_host, my_host) || !kill(pid, 0));\n> +                       (strcmp(locking_host, my_host) || !kill(pid, 0) || errno == EPERM);\n>                 if (fp != NULL)\n>                         fclose(fp);\n>                 if (should_exit) {\n> --\n> 1.8.5.2\n>\n\n\n\n-- \nDuy\n"},{"id":"232548","messageId":"CACnwZYe91VOX=JYOOek5UsDv4=F5wEDPj_20H5iyZKnurgXgzQ@mail.gmail.com","threadId":"35591","inReplyTo":"720d7a5676f8cbfc76c80198f9d3816@74d39fa044aa309eaea14b9f57fe79c","subject":"Re: [PATCH] gc: notice gc processes run by other users","fromName":"Thiago Farina","fromEmail":"tfransosi@gmail.com","sentAt":"2013-12-31T20:35:47Z","receivedAt":"2013-12-31T20:35:47Z","isPatch":true,"sender":{"key":"tfransosi@gmail.com","avatar":"https://avatars.githubusercontent.com/u/970071?v=4"},"body":"On Tue, Dec 31, 2013 at 10:07 AM, Kyle J. McKay <mackyle@gmail.com> wrote:\n> Since 64a99eb4 git gc refuses to run without the --force option if\n> another gc process on the same repository is already running.\n>\n> However, if the repository is shared and user A runs git gc on the\n> repository and while that gc is still running user B runs git gc on\n> the same repository the gc process run by user A will not be noticed\n> and the gc run by user B will go ahead and run.\n>\n> The problem is that the kill(pid, 0) test fails with an EPERM error\n> since user B is not allowed to signal processes owned by user A\n> (unless user B is root).\n>\n> Update the test to recognize an EPERM error as meaning the process\n> exists and another gc should not be run (unless --force is given).\n\nLooks like you are missing your Signed-off-by: line.\n\n--\nThiago Farina\n"},{"id":"232600","messageId":"02BCE0EB-ADA1-48F9-BD00-369FFDB5E372@gmail.com","threadId":"35591","inReplyTo":"720d7a5676f8cbfc76c80198f9d3816@74d39fa044aa309eaea14b9f57fe79c","subject":"Re: [PATCH] gc: notice gc processes run by other users","fromName":"Kyle","fromEmail":"kiltnaked@gmail.com","sentAt":"2014-01-03T00:11:24Z","receivedAt":"2014-01-03T00:11:24Z","isPatch":true,"sender":{"key":"kiltnaked@gmail.com","avatar":null},"body":"On Dec 31, 2013, at 04:07, Kyle J. McKay wrote:\n> Since 64a99eb4 git gc refuses to run without the --force option if\n> another gc process on the same repository is already running.\n>\n> However, if the repository is shared and user A runs git gc on the\n> repository and while that gc is still running user B runs git gc on\n> the same repository the gc process run by user A will not be noticed\n> and the gc run by user B will go ahead and run.\n>\n> The problem is that the kill(pid, 0) test fails with an EPERM error\n> since user B is not allowed to signal processes owned by user A\n> (unless user B is root).\n>\n> Update the test to recognize an EPERM error as meaning the process\n> exists and another gc should not be run (unless --force is given).\n\nOops, sorry, forgot sign off, here's my sign off:\n\nSigned-off-by: Kyle J. McKay <mackyle@gmail.com>\n\n> ---\n>\n> I suggest this be included in maint as others may also have expected  \n> the\n> shared repository, different user gc scenario to be caught by the new\n> code when in fact it's not without this patch.\n>\n> builtin/gc.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/builtin/gc.c b/builtin/gc.c\n> index c14190f8..25f2237c 100644\n> --- a/builtin/gc.c\n> +++ b/builtin/gc.c\n> @@ -222,7 +222,7 @@ static const char *lock_repo_for_gc(int force,  \n> pid_t* ret_pid)\n> \t\t\ttime(NULL) - st.st_mtime <= 12 * 3600 &&\n> \t\t\tfscanf(fp, \"%\"PRIuMAX\" %127c\", &pid, locking_host) == 2 &&\n> \t\t\t/* be gentle to concurrent \"gc\" on remote hosts */\n> -\t\t\t(strcmp(locking_host, my_host) || !kill(pid, 0));\n> +\t\t\t(strcmp(locking_host, my_host) || !kill(pid, 0) || errno ==  \n> EPERM);\n> \t\tif (fp != NULL)\n> \t\t\tfclose(fp);\n> \t\tif (should_exit) {\n> -- \n> 1.8.5.2\n>\n"},{"id":"232601","messageId":"xmqq61q1or8w.fsf@gitster.dls.corp.google.com","threadId":"35591","inReplyTo":"02BCE0EB-ADA1-48F9-BD00-369FFDB5E372@gmail.com","subject":"Re: [PATCH] gc: notice gc processes run by other users","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-01-03T00:15:11Z","receivedAt":"2014-01-03T00:15:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thanks.\n"}]}