| Message ID | f6a7c5ad56efceef9c11226beb854b806ef54687.1630947142.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, Sep 06 2021, Han-Wen Nienhuys via GitGitGadget wrote: > In CAFQ2z_Ps3YxycA+NJ9VKt_PEXb+m83JdNB7ujzWw1fTPKyZ=fg@mail.gmail.com, Nit: use <message-id> for quoting, not message-id. > 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. Maybe some summary of the flexibily either Jeff King or Junio mentioned we were losing (i.e. we can't selectively enable per-ref now), but that we think it's OK because... For the implementation: > + *logfd = -1; Weird, more on this later... > + if (!force_create && !should_autocreate_reflog(refname)) > + return 0; OK, so we can early abort. > files_reflog_path(refs, &logfile_sb, refname); > logfile = strbuf_detach(&logfile_sb, NULL); > > - if (force_create || should_autocreate_reflog(refname)) { > - if (raceproof_create_file(logfile, open_or_create_logfile, logfd)) { > - if (errno == ENOENT) > - strbuf_addf(err, "unable to create directory for '%s': " > - "%s", logfile, strerror(errno)); Here we use one indent/wrapping style... > - else if (errno == EISDIR) > - strbuf_addf(err, "there are still logs under '%s'", > - logfile); > - 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 (raceproof_create_file(logfile, open_or_create_logfile, logfd)) { > + if (errno == ENOENT) > + strbuf_addf(err, > + "unable to create directory for '%s': " > + "%s", > + logfile, strerror(errno)); ...but here it's changed while we're at it, this patch would be easier to follow IMO if we just left the formatting alone (or did it as another step). I'm aware that it ends us at over 79 columns, but that was the case before... > + else if (errno == EISDIR) > + strbuf_addf(err, "there are still logs under '%s'", > + logfile); > + else > + strbuf_addf(err, "unable to append to '%s': %s", > + logfile, strerror(errno)); > } > > if (*logfd >= 0) > adjust_shared_perm(logfile); > > free(logfile); > - return 0; > - > -error: > - free(logfile); > - return -1; > + return (*logfd < 0) ? -1 : 0; On "more on this later": Since we just return -1, 0 or a valid fd now, can't we just return the "fd" here and let the callers sort out -1, 0 and >0?
diff --git a/refs/files-backend.c b/refs/files-backend.c index b710d43be16..5ba68584aba 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -1551,51 +1551,32 @@ static int log_ref_setup(struct files_ref_store *refs, struct strbuf logfile_sb = STRBUF_INIT; char *logfile; + *logfd = -1; + if (!force_create && !should_autocreate_reflog(refname)) + return 0; + files_reflog_path(refs, &logfile_sb, refname); logfile = strbuf_detach(&logfile_sb, NULL); - if (force_create || should_autocreate_reflog(refname)) { - if (raceproof_create_file(logfile, open_or_create_logfile, logfd)) { - if (errno == ENOENT) - strbuf_addf(err, "unable to create directory for '%s': " - "%s", logfile, strerror(errno)); - else if (errno == EISDIR) - strbuf_addf(err, "there are still logs under '%s'", - logfile); - 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 (raceproof_create_file(logfile, open_or_create_logfile, logfd)) { + if (errno == ENOENT) + strbuf_addf(err, + "unable to create directory for '%s': " + "%s", + logfile, strerror(errno)); + else if (errno == EISDIR) + strbuf_addf(err, "there are still logs under '%s'", + logfile); + else + strbuf_addf(err, "unable to append to '%s': %s", + logfile, strerror(errno)); } if (*logfd >= 0) adjust_shared_perm(logfile); free(logfile); - return 0; - -error: - free(logfile); - return -1; + return (*logfd < 0) ? -1 : 0; } static int files_create_reflog(struct ref_store *ref_store, const char *refname, diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh index 8ced98e0fe8..446b568cef3 100755 --- a/t/t1400-update-ref.sh +++ b/t/t1400-update-ref.sh @@ -270,7 +270,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" && @@ -318,9 +318,8 @@ test_expect_success 'symref empty directory removal' ' 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 &&