# [PATCH] Make git reflog expire honour core.sharedRepository. 5 messages from 2008-06-15 to 2008-06-16. Participants: Pierre Habouzit, Junio C Hamano, Johannes Schindelin. Thread: https://gitlist.dev/t/13969 ## Pierre Habouzit, 2008-06-15 21:37 Subject: [PATCH] Make git reflog expire honour core.sharedRepository. Message-ID: <1213565862-23247-1-git-send-email-madcoder@debian.org> URL: https://gitlist.dev/e/1213565862-23247-1-git-send-email-madcoder%40debian.org ``` Signed-off-by: Pierre Habouzit --- builtin-reflog.c | 1 + t/t1301-shared-repo.sh | 15 +++++++++++++++ 2 files changed, 16 insertions(+), 0 deletions(-) Some people like to have logAllRefUpdates even for bare repositories, and if shared, git-gc breaks them. diff --git a/builtin-reflog.c b/builtin-reflog.c index 897d1dc..430929f 100644 --- a/builtin-reflog.c +++ b/builtin-reflog.c @@ -308,6 +308,7 @@ static int expire_reflog(const char *ref, const unsigned char *sha1, int unused, } else if (cmd->updateref && commit_ref(lock)) { status |= error("Couldn't set %s", lock->ref_name); } + adjust_shared_perm(log_file); } free(newlog_path); free(log_file); diff --git a/t/t1301-shared-repo.sh b/t/t1301-shared-repo.sh index 5e4252a..6c78c8b 100755 --- a/t/t1301-shared-repo.sh +++ b/t/t1301-shared-repo.sh @@ -83,4 +83,19 @@ do done +test_expect_success 'git reflog expire honors core.sharedRepository' ' + git config core.sharedRepository group && + git reflog expire --all && + actual="$(ls -l .git/logs/refs/heads/master)" && + case "$actual" in + -rw-rw-*) + : happy + ;; + *) + echo Ooops, .git/logs/refs/heads/master is not 0662 [$actual] + false + ;; + esac +' + test_done -- 1.5.6.rc3.149.gaacb0.dirty ``` ## Junio C Hamano, 2008-06-15 22:47 Subject: Re: [PATCH] Make git reflog expire honour core.sharedRepository. Message-ID: <7vhcbuco2w.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vhcbuco2w.fsf%40gitster.siamese.dyndns.org In-Reply-To: <1213565862-23247-1-git-send-email-madcoder@debian.org> ``` Pierre Habouzit writes: > Signed-off-by: Pierre Habouzit > --- > builtin-reflog.c | 1 + > t/t1301-shared-repo.sh | 15 +++++++++++++++ > 2 files changed, 16 insertions(+), 0 deletions(-) > > Some people like to have logAllRefUpdates even for bare repositories, and if > shared, git-gc breaks them. > > diff --git a/builtin-reflog.c b/builtin-reflog.c > index 897d1dc..430929f 100644 > --- a/builtin-reflog.c > +++ b/builtin-reflog.c > @@ -308,6 +308,7 @@ static int expire_reflog(const char *ref, const unsigned char *sha1, int unused, > } else if (cmd->updateref && commit_ref(lock)) { > status |= error("Couldn't set %s", lock->ref_name); > } > + adjust_shared_perm(log_file); Why is it sane to do this unconditionally, instead of putting in another else or something? ``` ## Pierre Habouzit, 2008-06-15 22:55 Subject: Re: [PATCH] Make git reflog expire honour core.sharedRepository. Message-ID: <20080615225459.GA13472@artemis.madism.org> URL: https://gitlist.dev/e/20080615225459.GA13472%40artemis.madism.org In-Reply-To: <7vhcbuco2w.fsf@gitster.siamese.dyndns.org> ``` On Sun, Jun 15, 2008 at 10:47:51PM +0000, Junio C Hamano wrote: > Pierre Habouzit writes: > > > Signed-off-by: Pierre Habouzit > > --- > > builtin-reflog.c | 1 + > > t/t1301-shared-repo.sh | 15 +++++++++++++++ > > 2 files changed, 16 insertions(+), 0 deletions(-) > > > > Some people like to have logAllRefUpdates even for bare repositories, and if > > shared, git-gc breaks them. > > > > diff --git a/builtin-reflog.c b/builtin-reflog.c > > index 897d1dc..430929f 100644 > > --- a/builtin-reflog.c > > +++ b/builtin-reflog.c > > @@ -308,6 +308,7 @@ static int expire_reflog(const char *ref, const unsigned char *sha1, int unused, > > } else if (cmd->updateref && commit_ref(lock)) { > > status |= error("Couldn't set %s", lock->ref_name); > > } > > + adjust_shared_perm(log_file); > > Why is it sane to do this unconditionally, instead of putting in another > else or something? Huh indeed, the proper patch should have been: - } + } else { + adjust_shared_perm(log_file); + } It was inattention on my end, I focused more on writing a test case than on the one liner fix. Sorry. -- ·O· Pierre Habouzit ··O madcoder@debian.org OOO http://www.madism.org ``` ## Johannes Schindelin, 2008-06-16 15:15 Subject: Re: [PATCH] Make git reflog expire honour core.sharedRepository. Message-ID: URL: https://gitlist.dev/e/alpine.DEB.1.00.0806161713520.3302%40eeepc-johanness In-Reply-To: <7vhcbuco2w.fsf@gitster.siamese.dyndns.org> ``` Hi, On Sun, 15 Jun 2008, Junio C Hamano wrote: > Pierre Habouzit writes: > > > Signed-off-by: Pierre Habouzit > > --- > > builtin-reflog.c | 1 + > > t/t1301-shared-repo.sh | 15 +++++++++++++++ > > 2 files changed, 16 insertions(+), 0 deletions(-) > > > > Some people like to have logAllRefUpdates even for bare repositories, and if > > shared, git-gc breaks them. > > > > diff --git a/builtin-reflog.c b/builtin-reflog.c > > index 897d1dc..430929f 100644 > > --- a/builtin-reflog.c > > +++ b/builtin-reflog.c > > @@ -308,6 +308,7 @@ static int expire_reflog(const char *ref, const unsigned char *sha1, int unused, > > } else if (cmd->updateref && commit_ref(lock)) { > > status |= error("Couldn't set %s", lock->ref_name); > > } > > + adjust_shared_perm(log_file); > > Why is it sane to do this unconditionally, instead of putting in another > else or something? Because adjust_shared_perm() checks for shared_repository, and returns when there is nothing to do. IMO this is a sane convention, since you _never_ want to do this unconditionally anyway, and you avoid a lot of identical "if () adjust()" constructs. Ciao, Dscho ``` ## Johannes Schindelin, 2008-06-16 15:19 Subject: Re: [PATCH] Make git reflog expire honour core.sharedRepository. Message-ID: URL: https://gitlist.dev/e/alpine.DEB.1.00.0806161718050.3302%40eeepc-johanness In-Reply-To: ``` Hi, On Mon, 16 Jun 2008, Johannes Schindelin wrote: > On Sun, 15 Jun 2008, Junio C Hamano wrote: > > > Pierre Habouzit writes: > > > > > Signed-off-by: Pierre Habouzit > > > --- > > > builtin-reflog.c | 1 + > > > t/t1301-shared-repo.sh | 15 +++++++++++++++ > > > 2 files changed, 16 insertions(+), 0 deletions(-) > > > > > > Some people like to have logAllRefUpdates even for bare repositories, and if > > > shared, git-gc breaks them. > > > > > > diff --git a/builtin-reflog.c b/builtin-reflog.c > > > index 897d1dc..430929f 100644 > > > --- a/builtin-reflog.c > > > +++ b/builtin-reflog.c > > > @@ -308,6 +308,7 @@ static int expire_reflog(const char *ref, const unsigned char *sha1, int unused, > > > } else if (cmd->updateref && commit_ref(lock)) { > > > status |= error("Couldn't set %s", lock->ref_name); > > > } > > > + adjust_shared_perm(log_file); > > > > Why is it sane to do this unconditionally, instead of putting in another > > else or something? > > Because adjust_shared_perm() checks for shared_repository, and returns > when there is nothing to do. Ooops. I think I missed your point, namely that this should only be done when no error occurred. I just blame it on the short snippet. Sorry, Dscho ```