From 77a28e0d5805da2880ff79a5b54250e7e0b7c9c6 Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Tue, 1 Aug 2017 08:53:33 -0400 Subject: [PATCH] Support force writing reflog on a per-update basis Even if a repository has core.logAllRefUpdates=true, ReflogWriter does not create reflog files unless the refs are under a hard-coded list of prefixes, or unless the forceWrite bit is set. Expose the forceWrite bit on a per-update basis in RefUpdate/BatchRefUpdate/ReceiveCommand, creating RefLogWriters as necessary. Change-Id: Ifc851fba00f76bf56d4134f821d0576b37810f80 --- .../storage/file/BatchRefUpdateTest.java | 79 +++++++++++++++++-- .../internal/storage/file/RefUpdateTest.java | 2 +- .../storage/file/PackedBatchRefUpdate.java | 4 +- .../internal/storage/file/RefDirectory.java | 13 ++- .../storage/file/RefDirectoryUpdate.java | 4 +- .../org/eclipse/jgit/lib/BatchRefUpdate.java | 50 ++++++++++++ .../src/org/eclipse/jgit/lib/RefUpdate.java | 32 ++++++++ .../jgit/transport/ReceiveCommand.java | 32 +++++++- 8 files changed, 196 insertions(+), 20 deletions(-) diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/BatchRefUpdateTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/BatchRefUpdateTest.java index 8097cd6ae..3c4b8cf4b 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/BatchRefUpdateTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/BatchRefUpdateTest.java @@ -142,11 +142,7 @@ public void setUp() throws Exception { super.setUp(); diskRepo = createBareRepository(); - StoredConfig cfg = diskRepo.getConfig(); - cfg.load(); - cfg.setBoolean(ConfigConstants.CONFIG_CORE_SECTION, null, - ConfigConstants.CONFIG_KEY_LOGALLREFUPDATES, true); - cfg.save(); + setLogAllRefUpdates(true); refdir = (RefDirectory) diskRepo.getRefDatabase(); refdir.setRetrySleepMs(Arrays.asList(0, 0)); @@ -655,6 +651,71 @@ public void overrideDisableRefLog() throws Exception { getLastReflog("refs/heads/branch")); } + @Test + public void refLogNotWrittenWithoutConfigOption() throws Exception { + setLogAllRefUpdates(false); + writeRef("refs/heads/master", A); + + Map oldLogs = + getLastReflogs("refs/heads/master", "refs/heads/branch"); + assertTrue(oldLogs.isEmpty()); + + List cmds = Arrays.asList( + new ReceiveCommand(A, B, "refs/heads/master", UPDATE), + new ReceiveCommand(zeroId(), B, "refs/heads/branch", CREATE)); + execute(newBatchUpdate(cmds).setRefLogMessage("a reflog", false)); + + assertResults(cmds, OK, OK); + assertReflogUnchanged(oldLogs, "refs/heads/master"); + assertReflogUnchanged(oldLogs, "refs/heads/branch"); + } + + @Test + public void forceRefLogInUpdate() throws Exception { + setLogAllRefUpdates(false); + writeRef("refs/heads/master", A); + assertTrue( + getLastReflogs("refs/heads/master", "refs/heads/branch").isEmpty()); + + List cmds = Arrays.asList( + new ReceiveCommand(A, B, "refs/heads/master", UPDATE), + new ReceiveCommand(zeroId(), B, "refs/heads/branch", CREATE)); + execute( + newBatchUpdate(cmds) + .setRefLogMessage("a reflog", false) + .setForceRefLog(true)); + + assertResults(cmds, OK, OK); + assertReflogEquals( + reflog(A, B, new PersonIdent(diskRepo), "a reflog"), + getLastReflog("refs/heads/master")); + assertReflogEquals( + reflog(zeroId(), B, new PersonIdent(diskRepo), "a reflog"), + getLastReflog("refs/heads/branch")); + } + + @Test + public void forceRefLogInCommand() throws Exception { + setLogAllRefUpdates(false); + writeRef("refs/heads/master", A); + + Map oldLogs = + getLastReflogs("refs/heads/master", "refs/heads/branch"); + assertTrue(oldLogs.isEmpty()); + + List cmds = Arrays.asList( + new ReceiveCommand(A, B, "refs/heads/master", UPDATE), + new ReceiveCommand(zeroId(), B, "refs/heads/branch", CREATE)); + cmds.get(1).setForceRefLog(true); + execute(newBatchUpdate(cmds).setRefLogMessage("a reflog", false)); + + assertResults(cmds, OK, OK); + assertReflogUnchanged(oldLogs, "refs/heads/master"); + assertReflogEquals( + reflog(zeroId(), B, new PersonIdent(diskRepo), "a reflog"), + getLastReflog("refs/heads/branch")); + } + @Test public void packedRefsLockFailure() throws Exception { writeLooseRef("refs/heads/master", A); @@ -791,6 +852,14 @@ public void atomicUpdateRespectsInProcessLock() throws Exception { "refs/heads/branch", B); } + private void setLogAllRefUpdates(boolean enable) throws Exception { + StoredConfig cfg = diskRepo.getConfig(); + cfg.load(); + cfg.setBoolean(ConfigConstants.CONFIG_CORE_SECTION, null, + ConfigConstants.CONFIG_KEY_LOGALLREFUPDATES, enable); + cfg.save(); + } + private void writeLooseRef(String name, AnyObjectId id) throws IOException { write(new File(diskRepo.getDirectory(), name), id.name() + "\n"); } diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/RefUpdateTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/RefUpdateTest.java index 34f9eb95d..52861ecd5 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/RefUpdateTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/RefUpdateTest.java @@ -1018,7 +1018,7 @@ private static void writeReflog(Repository db, ObjectId newId, String msg, RefDirectory refs = (RefDirectory) db.getRefDatabase(); RefDirectoryUpdate update = refs.newUpdate(refName, true); update.setNewObjectId(newId); - refs.log(update, msg, true); + refs.log(false, update, msg, true); } private static class SubclassedId extends ObjectId { diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackedBatchRefUpdate.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackedBatchRefUpdate.java index 0bb502dfb..b328eb83e 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackedBatchRefUpdate.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/PackedBatchRefUpdate.java @@ -406,7 +406,6 @@ private void writeReflog(List commands) { if (ident == null) { ident = new PersonIdent(refdb.getRepository()); } - ReflogWriter w = refdb.getLogWriter(); for (ReceiveCommand cmd : commands) { // Assume any pending commands have already been executed atomically. if (cmd.getResult() != ReceiveCommand.Result.OK) { @@ -436,7 +435,8 @@ private void writeReflog(List commands) { } } try { - w.log(name, cmd.getOldId(), cmd.getNewId(), ident, msg); + new ReflogWriter(refdb, isForceRefLog(cmd)) + .log(name, cmd.getOldId(), cmd.getNewId(), ident, msg); } catch (IOException e) { // Ignore failures, but continue attempting to write more reflogs. // diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/RefDirectory.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/RefDirectory.java index 105efe728..bb1dc91cd 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/RefDirectory.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/RefDirectory.java @@ -151,8 +151,6 @@ public class RefDirectory extends RefDatabase { final File refsDir; - private final ReflogWriter logWriter; - final File packedRefsFile; final File logsDir; @@ -210,7 +208,6 @@ public class RefDirectory extends RefDatabase { final FS fs = db.getFS(); parent = db; gitDir = db.getDirectory(); - logWriter = new ReflogWriter(this); refsDir = fs.resolve(gitDir, R_REFS); logsDir = fs.resolve(gitDir, LOGS); logsRefsDir = fs.resolve(gitDir, LOGS + '/' + R_REFS); @@ -224,8 +221,8 @@ Repository getRepository() { return parent; } - ReflogWriter getLogWriter() { - return logWriter; + ReflogWriter newLogWriter(boolean force) { + return new ReflogWriter(this, force); } /** @@ -249,7 +246,7 @@ public void create() throws IOException { FileUtils.mkdir(refsDir); FileUtils.mkdir(new File(refsDir, R_HEADS.substring(R_REFS.length()))); FileUtils.mkdir(new File(refsDir, R_TAGS.substring(R_REFS.length()))); - logWriter.create(); + newLogWriter(false).create(); } @Override @@ -866,9 +863,9 @@ private Ref peeledPackedRef(Ref f) } } - void log(final RefUpdate update, final String msg, final boolean deref) + void log(boolean force, RefUpdate update, String msg, boolean deref) throws IOException { - logWriter.log(update, msg, deref); + newLogWriter(force).log(update, msg, deref); } private Ref resolve(final Ref ref, int depth, String prefix, diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/RefDirectoryUpdate.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/RefDirectoryUpdate.java index 110535252..7ab30faf1 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/RefDirectoryUpdate.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/RefDirectoryUpdate.java @@ -120,7 +120,7 @@ protected Result doUpdate(final Result status) throws IOException { msg = strResult; } } - database.log(this, msg, shouldDeref); + database.log(isForceRefLog(), this, msg, shouldDeref); } if (!lock.commit()) return Result.LOCK_FAILURE; @@ -159,7 +159,7 @@ protected Result doLink(final String target) throws IOException { String msg = getRefLogMessage(); if (msg != null) - database.log(this, msg, false); + database.log(isForceRefLog(), this, msg, false); if (!lock.commit()) return Result.LOCK_FAILURE; database.storedSymbolicRef(this, lock.getCommitSnapshot(), target); diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/lib/BatchRefUpdate.java b/org.eclipse.jgit/src/org/eclipse/jgit/lib/BatchRefUpdate.java index 956607c9a..bcf9065dd 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/lib/BatchRefUpdate.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/lib/BatchRefUpdate.java @@ -104,6 +104,12 @@ public class BatchRefUpdate { /** Should the result value be appended to {@link #refLogMessage}. */ private boolean refLogIncludeResult; + /** + * Should reflogs be written even if the configured default for this ref is + * not to write it. + */ + private boolean forceRefLog; + /** Push certificate associated with this update. */ private PushCertificate pushCert; @@ -198,6 +204,12 @@ public boolean isRefLogIncludingResult() { /** * Set the message to include in the reflog. *

+ * Repository implementations may limit which reflogs are written by default, + * based on the project configuration. If a repo is not configured to write + * logs for this ref by default, setting the message alone may have no effect. + * To indicate that the repo should write logs for this update in spite of + * configured defaults, use {@link #setForceRefLog(boolean)}. + *

* Describes the default for commands in this batch that do not override it * with {@link ReceiveCommand#setRefLogMessage(String, boolean)}. * @@ -235,6 +247,18 @@ public BatchRefUpdate disableRefLog() { return this; } + /** + * Force writing a reflog for the updated ref. + * + * @param force whether to force. + * @return {@code this} + * @since 4.9 + */ + public BatchRefUpdate setForceRefLog(boolean force) { + forceRefLog = force; + return this; + } + /** * Check whether log has been disabled by {@link #disableRefLog()}. * @@ -244,6 +268,16 @@ public boolean isRefLogDisabled() { return refLogMessage == null; } + /** + * Check whether the reflog should be written regardless of repo defaults. + * + * @return whether force writing is enabled. + * @since 4.9 + */ + protected boolean isForceRefLog() { + return forceRefLog; + } + /** * Request that all updates in this batch be performed atomically. *

@@ -635,6 +669,7 @@ protected RefUpdate newUpdate(ReceiveCommand cmd) throws IOException { } else { ru.setRefLogIdent(refLogIdent); ru.setRefLogMessage(getRefLogMessage(cmd), isRefLogIncludingResult(cmd)); + ru.setForceRefLog(isForceRefLog(cmd)); } ru.setPushCertificate(pushCert); switch (cmd.getType()) { @@ -696,6 +731,21 @@ protected boolean isRefLogIncludingResult(ReceiveCommand cmd) { ? cmd.isRefLogIncludingResult() : isRefLogIncludingResult(); } + /** + * Check whether the reflog for a command should be written regardless of repo + * defaults. + * + * @param cmd + * specific command. + * @return whether force writing is enabled. + * @since 4.9 + */ + protected boolean isForceRefLog(ReceiveCommand cmd) { + Boolean isForceRefLog = cmd.isForceRefLog(); + return isForceRefLog != null ? isForceRefLog.booleanValue() + : isForceRefLog(); + } + @Override public String toString() { StringBuilder r = new StringBuilder(); diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/lib/RefUpdate.java b/org.eclipse.jgit/src/org/eclipse/jgit/lib/RefUpdate.java index 07786450a..766b21da0 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/lib/RefUpdate.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/lib/RefUpdate.java @@ -185,6 +185,12 @@ public static enum Result { /** Should the Result value be appended to {@link #refLogMessage}. */ private boolean refLogIncludeResult; + /** + * Should reflogs be written even if the configured default for this ref is + * not to write it. + */ + private boolean forceRefLog; + /** Old value of the ref, obtained after we lock it. */ private ObjectId oldValue; @@ -403,6 +409,12 @@ protected boolean isRefLogIncludingResult() { /** * Set the message to include in the reflog. + *

+ * Repository implementations may limit which reflogs are written by default, + * based on the project configuration. If a repo is not configured to write + * logs for this ref by default, setting the message alone may have no effect. + * To indicate that the repo should write logs for this update in spite of + * configured defaults, use {@link #setForceRefLog(boolean)}. * * @param msg * the message to describe this change. It may be null if @@ -430,6 +442,26 @@ public void disableRefLog() { refLogIncludeResult = false; } + /** + * Force writing a reflog for the updated ref. + * + * @param force whether to force. + * @since 4.9 + */ + public void setForceRefLog(boolean force) { + forceRefLog = force; + } + + /** + * Check whether the reflog should be written regardless of repo defaults. + * + * @return whether force writing is enabled. + * @since 4.9 + */ + protected boolean isForceRefLog() { + return forceRefLog; + } + /** * The old value of the ref, prior to the update being attempted. *

diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/ReceiveCommand.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/ReceiveCommand.java index 14b35c9bf..e9681b34c 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/ReceiveCommand.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/ReceiveCommand.java @@ -225,6 +225,8 @@ public static boolean isTransactionAborted(ReceiveCommand cmd) { private boolean refLogIncludeResult; + private Boolean forceRefLog; + private boolean typeIsCorrect; /** @@ -390,8 +392,22 @@ public void disableRefLog() { } /** - * Check whether this command has a custom reflog setting that should override - * defaults in any containing {@link org.eclipse.jgit.lib.BatchRefUpdate}. + * Force writing a reflog for the updated ref. + * + * @param force whether to force. + * @since 4.9 + */ + public void setForceRefLog(boolean force) { + forceRefLog = Boolean.valueOf(force); + } + + /** + * Check whether this command has a custom reflog message setting that should + * override defaults in any containing + * {@link org.eclipse.jgit.lib.BatchRefUpdate}. + *

+ * Does not take into account whether {@code #setForceRefLog(boolean)} has + * been called. * * @return whether a custom reflog is set. * @since 4.9 @@ -433,6 +449,18 @@ public boolean isRefLogIncludingResult() { return refLogIncludeResult; } + /** + * Check whether the reflog should be written regardless of repo defaults. + * + * @return whether force writing is enabled; null if {@code + * #setForceRefLog(boolean)} was never called. + * @since 4.9 + */ + @Nullable + public Boolean isForceRefLog() { + return forceRefLog; + } + /** * Set the status of this command. *