| Message ID | 2f11fd7718005d1e94b3139f086134896da202f1.1630334929.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Gets rid of "if reflog exists, append to it regardless of config settings" | expand |
On Mon, Aug 30, 2021 at 02:48:48PM +0000, Han-Wen Nienhuys via GitGitGadget wrote: > From: Han-Wen Nienhuys <hanwen@google.com> > > Before, if we aren't supposed to update reflogs (eg. > core.logallrefupdates=NONE), we would still write reflog entries if the > reflog file (.git/logs/REFNAME) existed. > > The reftable storage backend cannot distinguish between a non-existing > reflog, and an empty one. Therefore it cannot mimick this functionality. > > In CAFQ2z_Ps3YxycA+NJ9VKt_PEXb+m83JdNB7ujzWw1fTPKyZ=fg@mail.gmail.com, This looks like a faithful implementation of that discussion, so I don't see any problems with the change. > we came to the conclusion that this feature is probably a remnant from > the time that reflogs weren't enabled by default, and it does not need > to be kept. > > Signed-off-by: Han-Wen Nienhuys <hanwen@google.com> > --- > refs/files-backend.c | 20 +------------------- > t/t1400-update-ref.sh | 7 +++---- > 2 files changed, 4 insertions(+), 23 deletions(-) > > diff --git a/refs/files-backend.c b/refs/files-backend.c > index e05ada9286d..25f5a4ce777 100644 > --- a/refs/files-backend.c > +++ b/refs/files-backend.c > @@ -1551,6 +1551,7 @@ static int log_ref_setup(struct files_ref_store *refs, > struct strbuf logfile_sb = STRBUF_INIT; > char *logfile; > > + *logfd = -1; > files_reflog_path(refs, &logfile_sb, refname); > logfile = strbuf_detach(&logfile_sb, NULL); > > @@ -1565,26 +1566,8 @@ static int log_ref_setup(struct files_ref_store *refs, > else > strbuf_addf(err, "unable to append to '%s': %s", > logfile, strerror(errno)); > - > goto error; > } > - } else { > - *logfd = open(logfile, O_APPEND | O_WRONLY, 0666); > - if (*logfd < 0) { > - if (errno == ENOENT || errno == EISDIR) { > - /* > - * The logfile doesn't already exist, > - * but that is not an error; it only > - * means that we won't write log > - * entries to it. > - */ > - ; > - } else { > - strbuf_addf(err, "unable to append to '%s': %s", > - logfile, strerror(errno)); > - goto error; > - } > - } > } > > if (*logfd >= 0) I'm nit-picking, but I think that this conditional could probably be moved into the if-statement above, since we no longer set logfd anywhere else. It might read more clearly, and if you're going to resubmit this series anyway, it would be helpful to pick that up. > @@ -1592,7 +1575,6 @@ static int log_ref_setup(struct files_ref_store *refs, > > free(logfile); > return 0; > - > error: > free(logfile); > return -1; > diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh > index de0cf678f8e..52433f6d0d2 100755 > --- a/t/t1400-update-ref.sh > +++ b/t/t1400-update-ref.sh > @@ -269,7 +269,7 @@ test_expect_success "(not) changed .git/$m" ' > ' > > rm -f .git/logs/refs/heads/main > -test_expect_success "create $m (logged by touch)" ' > +test_expect_success "create $m" ' > test_config core.logAllRefUpdates false && > GIT_COMMITTER_DATE="2005-05-26 23:30" \ > git update-ref --create-reflog HEAD $A -m "Initial Creation" && > @@ -315,12 +315,10 @@ test_expect_success 'symref empty directory removal' ' > test_path_is_file .git/logs/HEAD > ' > > -TAB=' ' > cat >expect <<EOF > $Z $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150200 +0000 Initial Creation OK, so "Initial Creation" happened in a state where "core.logAllRefUpdates=false", but we passed "--create-reflog" to force the behavior anyway. Then during "Switch", we had a reflog, but again "core.logAllRefUpdates" is false, and we didn't pass "--create-reflog", so no more updates are written to the reflog. That's exactly the test that I would have expected to exist for this change, and so this LGTM. Thanks, Taylor
"Han-Wen Nienhuys via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Han-Wen Nienhuys <hanwen@google.com> > > Before, if we aren't supposed to update reflogs (eg. > core.logallrefupdates=NONE), we would still write reflog entries if the > reflog file (.git/logs/REFNAME) existed. > > The reftable storage backend cannot distinguish between a non-existing > reflog, and an empty one. Therefore it cannot mimick this functionality. > > In CAFQ2z_Ps3YxycA+NJ9VKt_PEXb+m83JdNB7ujzWw1fTPKyZ=fg@mail.gmail.com, > we came to the conclusion that this feature is probably a remnant from > the time that reflogs weren't enabled by default, and it does not need > to be kept. > > Signed-off-by: Han-Wen Nienhuys <hanwen@google.com> > --- > refs/files-backend.c | 20 +------------------- > t/t1400-update-ref.sh | 7 +++---- > 2 files changed, 4 insertions(+), 23 deletions(-) While I like the loss of lines and reduced complexity... One potential harm this change will bring to us is what happens to people who disable core.logAllRefUpdates manually after using the repository for a while. Their @{4} will point at the same commit no matter how many operations are done on the current branch after they do so. I wouldn't mind if "git reflog disable" command is given to the users prominently and core.logAllRefUpdates becomes a mere implementation detail nobody has to care about---in such a world, we could set the configuration and drop the existing reflog records at the same time and nobody will be hurt. If we keep the current behaviour, what are we harming instead? Growth of diskspace usage is an obvious one, but disks are cheaper compared to human brainwave cycles cost. > diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh > index de0cf678f8e..52433f6d0d2 100755 > --- a/t/t1400-update-ref.sh > +++ b/t/t1400-update-ref.sh > @@ -269,7 +269,7 @@ test_expect_success "(not) changed .git/$m" ' > ' > > rm -f .git/logs/refs/heads/main > -test_expect_success "create $m (logged by touch)" ' > +test_expect_success "create $m" ' > test_config core.logAllRefUpdates false && > GIT_COMMITTER_DATE="2005-05-26 23:30" \ > git update-ref --create-reflog HEAD $A -m "Initial Creation" && > @@ -315,12 +315,10 @@ test_expect_success 'symref empty directory removal' ' > test_path_is_file .git/logs/HEAD > ' > > -TAB=' ' > cat >expect <<EOF > $Z $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150200 +0000 Initial Creation > -$A $B $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150260 +0000 Switch > -$B $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150860 +0000$TAB > EOF > + > test_expect_success "verifying $m's log (logged by touch)" ' > test_when_finished "git update-ref -d $m && rm -rf .git/logs actual expect" && > test-tool ref-store main for-each-reflog-ent $m > actual && > @@ -346,6 +344,7 @@ test_expect_success "set $m (logged by config)" ' > test $A = $(git show-ref -s --verify $m) > ' > > +TAB=' ' > cat >expect <<EOF > $Z $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150320 +0000 Initial Creation > $A $B $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150380 +0000 Switch Moving the definition of TAB= around in the test is not all that welcome. If anything, I'd suggest doing so nearer to the beginning of the test sequence, before the first test that uses the ref-store test-tool, because it is just like $GIT_COMMITTER_EMAIL and $RUN that define constants used throughout the remainder of the test script. THanks.
diff --git a/refs/files-backend.c b/refs/files-backend.c index e05ada9286d..25f5a4ce777 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -1551,6 +1551,7 @@ static int log_ref_setup(struct files_ref_store *refs, struct strbuf logfile_sb = STRBUF_INIT; char *logfile; + *logfd = -1; files_reflog_path(refs, &logfile_sb, refname); logfile = strbuf_detach(&logfile_sb, NULL); @@ -1565,26 +1566,8 @@ static int log_ref_setup(struct files_ref_store *refs, else strbuf_addf(err, "unable to append to '%s': %s", logfile, strerror(errno)); - goto error; } - } else { - *logfd = open(logfile, O_APPEND | O_WRONLY, 0666); - if (*logfd < 0) { - if (errno == ENOENT || errno == EISDIR) { - /* - * The logfile doesn't already exist, - * but that is not an error; it only - * means that we won't write log - * entries to it. - */ - ; - } else { - strbuf_addf(err, "unable to append to '%s': %s", - logfile, strerror(errno)); - goto error; - } - } } if (*logfd >= 0) @@ -1592,7 +1575,6 @@ static int log_ref_setup(struct files_ref_store *refs, free(logfile); return 0; - error: free(logfile); return -1; diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh index de0cf678f8e..52433f6d0d2 100755 --- a/t/t1400-update-ref.sh +++ b/t/t1400-update-ref.sh @@ -269,7 +269,7 @@ test_expect_success "(not) changed .git/$m" ' ' rm -f .git/logs/refs/heads/main -test_expect_success "create $m (logged by touch)" ' +test_expect_success "create $m" ' test_config core.logAllRefUpdates false && GIT_COMMITTER_DATE="2005-05-26 23:30" \ git update-ref --create-reflog HEAD $A -m "Initial Creation" && @@ -315,12 +315,10 @@ test_expect_success 'symref empty directory removal' ' test_path_is_file .git/logs/HEAD ' -TAB=' ' cat >expect <<EOF $Z $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150200 +0000 Initial Creation -$A $B $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150260 +0000 Switch -$B $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150860 +0000$TAB EOF + test_expect_success "verifying $m's log (logged by touch)" ' test_when_finished "git update-ref -d $m && rm -rf .git/logs actual expect" && test-tool ref-store main for-each-reflog-ent $m > actual && @@ -346,6 +344,7 @@ test_expect_success "set $m (logged by config)" ' test $A = $(git show-ref -s --verify $m) ' +TAB=' ' cat >expect <<EOF $Z $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150320 +0000 Initial Creation $A $B $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150380 +0000 Switch