{"thread":{"id":"13969","subject":"[PATCH] Make git reflog expire honour core.sharedRepository.","startedAt":"2008-06-15T21:37:42Z","lastAt":"2008-06-16T15:19:03Z","messageCount":5,"participants":["Pierre Habouzit","Junio C Hamano","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"79964","messageId":"1213565862-23247-1-git-send-email-madcoder@debian.org","threadId":"13969","inReplyTo":null,"subject":"[PATCH] Make git reflog expire honour core.sharedRepository.","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2008-06-15T21:37:42Z","receivedAt":"2008-06-15T21:37:42Z","isPatch":true,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"Signed-off-by: Pierre Habouzit \n---\n builtin-reflog.c | 1 +\n t/t1301-shared-repo.sh | 15 +++++++++++++++\n 2 files changed, 16 insertions(+), 0 deletions(-)\n\n Some people like to have logAllRefUpdates even for bare repositories, and if\n shared, git-gc breaks them.\n\ndiff --git a/builtin-reflog.c b/builtin-reflog.c\nindex 897d1dc..430929f 100644\n--- a/builtin-reflog.c\n+++ b/builtin-reflog.c\n@@ -308,6 +308,7 @@ static int expire_reflog(const char *ref, const unsigned char *sha1, int unused,\n \t\t} else if (cmd->updateref && commit_ref(lock)) {\n \t\t\tstatus |= error(\"Couldn't set %s\", lock->ref_name);\n \t\t}\n+\t\tadjust_shared_perm(log_file);\n \t}\n \tfree(newlog_path);\n \tfree(log_file);\ndiff --git a/t/t1301-shared-repo.sh b/t/t1301-shared-repo.sh\nindex 5e4252a..6c78c8b 100755\n--- a/t/t1301-shared-repo.sh\n+++ b/t/t1301-shared-repo.sh\n@@ -83,4 +83,19 @@ do\n \n done\n \n+test_expect_success 'git reflog expire honors core.sharedRepository' '\n+\tgit config core.sharedRepository group &&\n+\tgit reflog expire --all &&\n+\tactual=\"$(ls -l .git/logs/refs/heads/master)\" &&\n+\tcase \"$actual\" in\n+\t-rw-rw-*)\n+\t\t: happy\n+\t\t;;\n+\t*)\n+\t\techo Ooops, .git/logs/refs/heads/master is not 0662 [$actual]\n+\t\tfalse\n+\t\t;;\n+\tesac\n+'\n+\n test_done\n-- \n1.5.6.rc3.149.gaacb0.dirty\n"},{"id":"79988","messageId":"7vhcbuco2w.fsf@gitster.siamese.dyndns.org","threadId":"13969","inReplyTo":"1213565862-23247-1-git-send-email-madcoder@debian.org","subject":"Re: [PATCH] Make git reflog expire honour core.sharedRepository.","fromName":"Junio C Hamano","fromEmail":"junio@pobox.com","sentAt":"2008-06-15T22:47:51Z","receivedAt":"2008-06-15T22:47:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Pierre Habouzit writes:\n\n> Signed-off-by: Pierre Habouzit \n> ---\n> builtin-reflog.c | 1 +\n> t/t1301-shared-repo.sh | 15 +++++++++++++++\n> 2 files changed, 16 insertions(+), 0 deletions(-)\n>\n> Some people like to have logAllRefUpdates even for bare repositories, and if\n> shared, git-gc breaks them.\n>\n> diff --git a/builtin-reflog.c b/builtin-reflog.c\n> index 897d1dc..430929f 100644\n> --- a/builtin-reflog.c\n> +++ b/builtin-reflog.c\n> @@ -308,6 +308,7 @@ static int expire_reflog(const char *ref, const unsigned char *sha1, int unused,\n> \t\t} else if (cmd->updateref && commit_ref(lock)) {\n> \t\t\tstatus |= error(\"Couldn't set %s\", lock->ref_name);\n> \t\t}\n> +\t\tadjust_shared_perm(log_file);\n\nWhy is it sane to do this unconditionally, instead of putting in another\nelse or something?\n"},{"id":"79990","messageId":"20080615225459.GA13472@artemis.madism.org","threadId":"13969","inReplyTo":"7vhcbuco2w.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Make git reflog expire honour core.sharedRepository.","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2008-06-15T22:55:00Z","receivedAt":"2008-06-15T22:55:00Z","isPatch":true,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"On Sun, Jun 15, 2008 at 10:47:51PM +0000, Junio C Hamano wrote:\n> Pierre Habouzit writes:\n> \n> > Signed-off-by: Pierre Habouzit \n> > ---\n> > builtin-reflog.c | 1 +\n> > t/t1301-shared-repo.sh | 15 +++++++++++++++\n> > 2 files changed, 16 insertions(+), 0 deletions(-)\n> >\n> > Some people like to have logAllRefUpdates even for bare repositories, and if\n> > shared, git-gc breaks them.\n> >\n> > diff --git a/builtin-reflog.c b/builtin-reflog.c\n> > index 897d1dc..430929f 100644\n> > --- a/builtin-reflog.c\n> > +++ b/builtin-reflog.c\n> > @@ -308,6 +308,7 @@ static int expire_reflog(const char *ref, const unsigned char *sha1, int unused,\n> > \t\t} else if (cmd->updateref && commit_ref(lock)) {\n> > \t\t\tstatus |= error(\"Couldn't set %s\", lock->ref_name);\n> > \t\t}\n> > +\t\tadjust_shared_perm(log_file);\n> \n> Why is it sane to do this unconditionally, instead of putting in another\n> else or something?\n\n Huh indeed, the proper patch should have been:\n\n-\t\t}\n+\t\t} else {\n+\t\t\tadjust_shared_perm(log_file);\n+\t\t}\n\n It was inattention on my end, I focused more on writing a test case\nthan on the one liner fix. Sorry.\n-- \n·O· Pierre Habouzit\n··O madcoder@debian.org\nOOO http://www.madism.org\n"},{"id":"80032","messageId":"alpine.DEB.1.00.0806161713520.3302@eeepc-johanness","threadId":"13969","inReplyTo":"7vhcbuco2w.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Make git reflog expire honour core.sharedRepository.","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-06-16T15:15:12Z","receivedAt":"2008-06-16T15:15:12Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sun, 15 Jun 2008, Junio C Hamano wrote:\n\n> Pierre Habouzit writes:\n> \n> > Signed-off-by: Pierre Habouzit \n> > ---\n> > builtin-reflog.c | 1 +\n> > t/t1301-shared-repo.sh | 15 +++++++++++++++\n> > 2 files changed, 16 insertions(+), 0 deletions(-)\n> >\n> > Some people like to have logAllRefUpdates even for bare repositories, and if\n> > shared, git-gc breaks them.\n> >\n> > diff --git a/builtin-reflog.c b/builtin-reflog.c\n> > index 897d1dc..430929f 100644\n> > --- a/builtin-reflog.c\n> > +++ b/builtin-reflog.c\n> > @@ -308,6 +308,7 @@ static int expire_reflog(const char *ref, const unsigned char *sha1, int unused,\n> > \t\t} else if (cmd->updateref && commit_ref(lock)) {\n> > \t\t\tstatus |= error(\"Couldn't set %s\", lock->ref_name);\n> > \t\t}\n> > +\t\tadjust_shared_perm(log_file);\n> \n> Why is it sane to do this unconditionally, instead of putting in another\n> else or something?\n\nBecause adjust_shared_perm() checks for shared_repository, and returns \nwhen there is nothing to do.\n\nIMO this is a sane convention, since you _never_ want to do this \nunconditionally anyway, and you avoid a lot of identical \"if () adjust()\" \nconstructs.\n\nCiao,\nDscho\n"},{"id":"80033","messageId":"alpine.DEB.1.00.0806161718050.3302@eeepc-johanness","threadId":"13969","inReplyTo":"alpine.DEB.1.00.0806161713520.3302@eeepc-johanness","subject":"Re: [PATCH] Make git reflog expire honour core.sharedRepository.","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-06-16T15:19:03Z","receivedAt":"2008-06-16T15:19:03Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 16 Jun 2008, Johannes Schindelin wrote:\n\n> On Sun, 15 Jun 2008, Junio C Hamano wrote:\n> \n> > Pierre Habouzit writes:\n> > \n> > > Signed-off-by: Pierre Habouzit \n> > > ---\n> > > builtin-reflog.c | 1 +\n> > > t/t1301-shared-repo.sh | 15 +++++++++++++++\n> > > 2 files changed, 16 insertions(+), 0 deletions(-)\n> > >\n> > > Some people like to have logAllRefUpdates even for bare repositories, and if\n> > > shared, git-gc breaks them.\n> > >\n> > > diff --git a/builtin-reflog.c b/builtin-reflog.c\n> > > index 897d1dc..430929f 100644\n> > > --- a/builtin-reflog.c\n> > > +++ b/builtin-reflog.c\n> > > @@ -308,6 +308,7 @@ static int expire_reflog(const char *ref, const unsigned char *sha1, int unused,\n> > > \t\t} else if (cmd->updateref && commit_ref(lock)) {\n> > > \t\t\tstatus |= error(\"Couldn't set %s\", lock->ref_name);\n> > > \t\t}\n> > > +\t\tadjust_shared_perm(log_file);\n> > \n> > Why is it sane to do this unconditionally, instead of putting in another\n> > else or something?\n> \n> Because adjust_shared_perm() checks for shared_repository, and returns \n> when there is nothing to do.\n\nOoops. I think I missed your point, namely that this should only be done \nwhen no error occurred. I just blame it on the short snippet.\n\nSorry,\nDscho\n"}]}