Re: [PATCH 4/4] RFC: refs: reflog entries aren't written based on reflog existence.
- From
Taylor Blau <me@ttaylorr.com>
- Date
- Aug 30, 2021, 21:10 UTC
- Message-ID
- <YS1JTuBY0Lq34yP/@nand.local>
- In-Reply-To
- <2f11fd7718005d1e94b3139f086134896da202f1.1630334929.git.gitgitgadget@gmail.com>
On Mon, Aug 30, 2021 at 02:48:48PM +0000, Han-Wen Nienhuys via GitGitGadget wrote:
Show 10 quoted lines
> 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.
Show 49 quoted lines
> 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.
Show 28 quoted lines
> @@ -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