From dbb137e0f3ccaef7db4982127da1c7bdfc581e98 Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Mon, 17 Jul 2017 12:25:09 -0400 Subject: [PATCH 1/3] Extract constants for reflog entry message prefixes Document explicitly that these are untranslated to (mostly) match C git. Change-Id: I3abcffb4fd611d053bf4373e5d6a14a66f7b9b6b --- .../storage/file/RefDirectoryUpdate.java | 9 ++--- .../src/org/eclipse/jgit/lib/ReflogEntry.java | 33 +++++++++++++++++++ 2 files changed, 38 insertions(+), 4 deletions(-) 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 3c1916b64..110535252 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 @@ -50,6 +50,7 @@ import java.io.IOException; import org.eclipse.jgit.lib.Ref; import org.eclipse.jgit.lib.RefUpdate; +import org.eclipse.jgit.lib.ReflogEntry; import org.eclipse.jgit.lib.Repository; /** Updates any reference stored by {@link RefDirectory}. */ @@ -127,14 +128,14 @@ class RefDirectoryUpdate extends RefUpdate { return status; } - private String toResultString(final Result status) { + private String toResultString(Result status) { switch (status) { case FORCED: - return "forced-update"; //$NON-NLS-1$ + return ReflogEntry.PREFIX_FORCED_UPDATE; case FAST_FORWARD: - return "fast forward"; //$NON-NLS-1$ + return ReflogEntry.PREFIX_FAST_FORWARD; case NEW: - return "created"; //$NON-NLS-1$ + return ReflogEntry.PREFIX_CREATED; default: return null; } diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/lib/ReflogEntry.java b/org.eclipse.jgit/src/org/eclipse/jgit/lib/ReflogEntry.java index 8bd063378..afa6521d6 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/lib/ReflogEntry.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/lib/ReflogEntry.java @@ -49,6 +49,39 @@ package org.eclipse.jgit.lib; */ public interface ReflogEntry { + /** + * Prefix used in reflog messages when the ref was first created. + *

+ * Does not have a corresponding constant in C git, but is untranslated like + * the other constants. + * + * @since 4.9 + */ + public static final String PREFIX_CREATED = "created"; //$NON-NLS-1$ + + /** + * Prefix used in reflog messages when the ref was updated with a fast + * forward. + *

+ * Untranslated, and exactly matches the + * + * untranslated string in C git. + * + * @since 4.9 + */ + public static final String PREFIX_FAST_FORWARD = "fast-forward"; //$NON-NLS-1$ + + /** + * Prefix used in reflog messages when the ref was force updated. + *

+ * Untranslated, and exactly matches the + * + * untranslated string in C git. + * + * @since 4.9 + */ + public static final String PREFIX_FORCED_UPDATE = "forced-update"; //$NON-NLS-1$ + /** * @return the commit id before the change */ From 22e9106224e1514dbee6d6f8271187de606716a6 Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Mon, 17 Jul 2017 10:01:10 -0400 Subject: [PATCH 2/3] PackedBatchRefUpdate: Write reflogs On-disk reflogs are not stored in the packed-refs file, so we cannot ensure atomic updates. We choose the lesser evil of dropping failed reflog updates on the floor, rather than throwing an exception even though the underlying ref updates succeeded. Add tests for reflogs to BatchRefUpdateTest. Change-Id: Ia456ba9e36af8e01fde81b19af46a72378e614cd --- .../storage/file/BatchRefUpdateTest.java | 316 ++++++++++++++++++ .../storage/file/PackedBatchRefUpdate.java | 78 +++++ 2 files changed, 394 insertions(+) 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 06c47acf2..a51a910fb 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 @@ -55,12 +55,14 @@ import static org.eclipse.jgit.transport.ReceiveCommand.Type.UPDATE; import static org.eclipse.jgit.transport.ReceiveCommand.Type.UPDATE_NONFASTFORWARD; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; import java.io.File; import java.io.IOException; import java.util.Arrays; import java.util.Collection; +import java.util.Collections; import java.util.LinkedHashMap; import java.util.List; import java.util.Map; @@ -71,12 +73,19 @@ import org.eclipse.jgit.junit.StrictWorkMonitor; import org.eclipse.jgit.junit.TestRepository; import org.eclipse.jgit.lib.AnyObjectId; import org.eclipse.jgit.lib.BatchRefUpdate; +import org.eclipse.jgit.lib.CheckoutEntry; +import org.eclipse.jgit.lib.ConfigConstants; import org.eclipse.jgit.lib.Constants; import org.eclipse.jgit.lib.NullProgressMonitor; import org.eclipse.jgit.lib.ObjectId; +import org.eclipse.jgit.lib.PersonIdent; import org.eclipse.jgit.lib.Ref; import org.eclipse.jgit.lib.RefDatabase; +import org.eclipse.jgit.lib.RefUpdate; +import org.eclipse.jgit.lib.ReflogEntry; +import org.eclipse.jgit.lib.ReflogReader; import org.eclipse.jgit.lib.Repository; +import org.eclipse.jgit.lib.StoredConfig; import org.eclipse.jgit.revwalk.RevCommit; import org.eclipse.jgit.revwalk.RevWalk; import org.eclipse.jgit.transport.ReceiveCommand; @@ -109,6 +118,12 @@ public class BatchRefUpdateTest extends LocalDiskRepositoryTestCase { super.setUp(); diskRepo = createBareRepository(); + StoredConfig cfg = diskRepo.getConfig(); + cfg.load(); + cfg.setBoolean(ConfigConstants.CONFIG_CORE_SECTION, null, + ConfigConstants.CONFIG_KEY_LOGALLREFUPDATES, true); + cfg.save(); + refdir = (RefDirectory) diskRepo.getRefDatabase(); repo = new TestRepository<>(diskRepo); @@ -318,10 +333,231 @@ public class BatchRefUpdateTest extends LocalDiskRepositoryTestCase { } } + @Test + public void noRefLog() throws IOException { + writeRef("refs/heads/master", A); + + Map oldLogs = + getLastReflogs("refs/heads/master", "refs/heads/branch"); + assertEquals(Collections.singleton("refs/heads/master"), oldLogs.keySet()); + + List cmds = Arrays.asList( + new ReceiveCommand(A, B, "refs/heads/master", UPDATE), + new ReceiveCommand(zeroId(), B, "refs/heads/branch", CREATE)); + execute(newBatchUpdate(cmds).setAllowNonFastForwards(true)); + + assertResults(cmds, OK, OK); + assertRefs( + "refs/heads/master", B, + "refs/heads/branch", B); + assertReflogUnchanged(oldLogs, "refs/heads/master"); + assertReflogUnchanged(oldLogs, "refs/heads/branch"); + } + + @Test + public void reflogDefaultIdent() throws IOException { + writeRef("refs/heads/master", A); + writeRef("refs/heads/branch2", A); + + Map oldLogs = getLastReflogs( + "refs/heads/master", "refs/heads/branch1", "refs/heads/branch2"); + List cmds = Arrays.asList( + new ReceiveCommand(A, B, "refs/heads/master", UPDATE), + new ReceiveCommand(zeroId(), B, "refs/heads/branch1", CREATE)); + execute( + newBatchUpdate(cmds) + .setAllowNonFastForwards(true) + .setRefLogMessage("a reflog", false)); + + assertResults(cmds, OK, OK); + assertRefs( + "refs/heads/master", B, + "refs/heads/branch1", B, + "refs/heads/branch2", A); + 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/branch1")); + assertReflogUnchanged(oldLogs, "refs/heads/branch2"); + } + + @Test + public void reflogAppendStatusNoMessage() throws IOException { + writeRef("refs/heads/master", A); + writeRef("refs/heads/branch1", B); + + List cmds = Arrays.asList( + new ReceiveCommand(A, B, "refs/heads/master", UPDATE), + new ReceiveCommand(B, A, "refs/heads/branch1", UPDATE_NONFASTFORWARD), + new ReceiveCommand(zeroId(), A, "refs/heads/branch2", CREATE)); + execute( + newBatchUpdate(cmds) + .setAllowNonFastForwards(true) + .setRefLogMessage(null, true)); + + assertResults(cmds, OK, OK, OK); + assertRefs( + "refs/heads/master", B, + "refs/heads/branch1", A, + "refs/heads/branch2", A); + assertReflogEquals( + // Always forced; setAllowNonFastForwards(true) bypasses the check. + reflog(A, B, new PersonIdent(diskRepo), "forced-update"), + getLastReflog("refs/heads/master")); + assertReflogEquals( + reflog(B, A, new PersonIdent(diskRepo), "forced-update"), + getLastReflog("refs/heads/branch1")); + assertReflogEquals( + reflog(zeroId(), A, new PersonIdent(diskRepo), "created"), + getLastReflog("refs/heads/branch2")); + } + + @Test + public void reflogAppendStatusFastForward() throws IOException { + writeRef("refs/heads/master", A); + + List cmds = Arrays.asList( + new ReceiveCommand(A, B, "refs/heads/master", UPDATE)); + execute(newBatchUpdate(cmds).setRefLogMessage(null, true)); + + assertResults(cmds, OK); + assertRefs("refs/heads/master", B); + assertReflogEquals( + reflog(A, B, new PersonIdent(diskRepo), "fast-forward"), + getLastReflog("refs/heads/master")); + } + + @Test + public void reflogAppendStatusWithMessage() throws IOException { + writeRef("refs/heads/master", A); + + List cmds = Arrays.asList( + new ReceiveCommand(A, B, "refs/heads/master", UPDATE), + new ReceiveCommand(zeroId(), A, "refs/heads/branch", CREATE)); + execute(newBatchUpdate(cmds).setRefLogMessage("a reflog", true)); + + assertResults(cmds, OK, OK); + assertRefs( + "refs/heads/master", B, + "refs/heads/branch", A); + assertReflogEquals( + reflog(A, B, new PersonIdent(diskRepo), "a reflog: fast-forward"), + getLastReflog("refs/heads/master")); + assertReflogEquals( + reflog(zeroId(), A, new PersonIdent(diskRepo), "a reflog: created"), + getLastReflog("refs/heads/branch")); + } + + @Test + public void reflogCustomIdent() throws IOException { + writeRef("refs/heads/master", A); + + List cmds = Arrays.asList( + new ReceiveCommand(A, B, "refs/heads/master", UPDATE), + new ReceiveCommand(zeroId(), B, "refs/heads/branch", CREATE)); + PersonIdent ident = new PersonIdent("A Reflog User", "reflog@example.com"); + execute( + newBatchUpdate(cmds) + .setRefLogMessage("a reflog", false) + .setRefLogIdent(ident)); + + assertResults(cmds, OK, OK); + assertRefs( + "refs/heads/master", B, + "refs/heads/branch", B); + assertReflogEquals( + reflog(A, B, ident, "a reflog"), + getLastReflog("refs/heads/master"), + true); + assertReflogEquals( + reflog(zeroId(), B, ident, "a reflog"), + getLastReflog("refs/heads/branch"), + true); + } + + @Test + public void reflogDelete() throws IOException { + writeRef("refs/heads/master", A); + writeRef("refs/heads/branch", A); + assertEquals( + 2, getLastReflogs("refs/heads/master", "refs/heads/branch").size()); + + List cmds = Arrays.asList( + new ReceiveCommand(A, zeroId(), "refs/heads/master", DELETE), + new ReceiveCommand(A, B, "refs/heads/branch", UPDATE)); + execute(newBatchUpdate(cmds).setRefLogMessage("a reflog", false)); + + assertResults(cmds, OK, OK); + assertRefs("refs/heads/branch", B); + assertNull(getLastReflog("refs/heads/master")); + assertReflogEquals( + reflog(A, B, new PersonIdent(diskRepo), "a reflog"), + getLastReflog("refs/heads/branch")); + } + + @Test + public void reflogFileDirectoryConflict() throws IOException { + writeRef("refs/heads/master", A); + + List cmds = Arrays.asList( + new ReceiveCommand(A, zeroId(), "refs/heads/master", DELETE), + new ReceiveCommand(zeroId(), A, "refs/heads/master/x", CREATE)); + execute(newBatchUpdate(cmds).setRefLogMessage("a reflog", false)); + + assertResults(cmds, OK, OK); + assertRefs("refs/heads/master/x", A); + assertNull(getLastReflog("refs/heads/master")); + assertReflogEquals( + reflog(zeroId(), A, new PersonIdent(diskRepo), "a reflog"), + getLastReflog("refs/heads/master/x")); + } + + @Test + public void reflogOnLockFailure() throws IOException { + writeRef("refs/heads/master", A); + + Map oldLogs = + getLastReflogs("refs/heads/master", "refs/heads/branch"); + + List cmds = Arrays.asList( + new ReceiveCommand(A, B, "refs/heads/master", UPDATE), + new ReceiveCommand(A, B, "refs/heads/branch", UPDATE)); + execute(newBatchUpdate(cmds).setRefLogMessage("a reflog", false)); + + if (atomic) { + assertResults(cmds, TRANSACTION_ABORTED, LOCK_FAILURE); + assertReflogUnchanged(oldLogs, "refs/heads/master"); + assertReflogUnchanged(oldLogs, "refs/heads/branch"); + } else { + assertResults(cmds, OK, LOCK_FAILURE); + assertReflogEquals( + reflog(A, B, new PersonIdent(diskRepo), "a reflog"), + getLastReflog("refs/heads/master")); + assertReflogUnchanged(oldLogs, "refs/heads/branch"); + } + } + private void writeLooseRef(String name, AnyObjectId id) throws IOException { write(new File(diskRepo.getDirectory(), name), id.name() + "\n"); } + private void writeRef(String name, AnyObjectId id) throws IOException { + RefUpdate u = diskRepo.updateRef(name); + u.setRefLogMessage(getClass().getSimpleName(), false); + u.setForceUpdate(true); + u.setNewObjectId(id); + RefUpdate.Result r = u.update(); + switch (r) { + case NEW: + case FORCED: + return; + default: + throw new IOException("Got " + r + " while updating " + name); + } + } + private BatchRefUpdate newBatchUpdate(List cmds) { BatchRefUpdate u = refdir.newBatchUpdate(); if (atomic) { @@ -410,4 +646,84 @@ public class BatchRefUpdateTest extends LocalDiskRepositoryTestCase { r.p.test(c)); } } + + private Map getLastReflogs(String... names) + throws IOException { + Map result = new LinkedHashMap<>(); + for (String name : names) { + ReflogEntry e = getLastReflog(name); + if (e != null) { + result.put(name, e); + } + } + return result; + } + + private ReflogEntry getLastReflog(String name) throws IOException { + ReflogReader r = diskRepo.getReflogReader(name); + if (r == null) { + return null; + } + return r.getLastEntry(); + } + + private void assertReflogUnchanged( + Map old, String name) throws IOException { + assertReflogEquals(old.get(name), getLastReflog(name), true); + } + + private static void assertReflogEquals( + ReflogEntry expected, ReflogEntry actual) { + assertReflogEquals(expected, actual, false); + } + + private static void assertReflogEquals( + ReflogEntry expected, ReflogEntry actual, boolean strictTime) { + if (expected == null) { + assertNull(actual); + return; + } + assertNotNull(actual); + assertEquals(expected.getOldId(), actual.getOldId()); + assertEquals(expected.getNewId(), actual.getNewId()); + if (strictTime) { + assertEquals(expected.getWho(), actual.getWho()); + } else { + assertEquals(expected.getWho().getName(), actual.getWho().getName()); + assertEquals( + expected.getWho().getEmailAddress(), + actual.getWho().getEmailAddress()); + } + assertEquals(expected.getComment(), actual.getComment()); + } + + private static ReflogEntry reflog(ObjectId oldId, ObjectId newId, + PersonIdent who, String comment) { + return new ReflogEntry() { + @Override + public ObjectId getOldId() { + return oldId; + } + + @Override + public ObjectId getNewId() { + return newId; + } + + @Override + public PersonIdent getWho() { + return who; + } + + @Override + public String getComment() { + return comment; + } + + @Override + public CheckoutEntry parseCheckout() { + throw new UnsupportedOperationException(); + } + }; + } } 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 65f9ec42a..90155cbac 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 @@ -62,9 +62,11 @@ import org.eclipse.jgit.internal.storage.file.RefDirectory.PackedRefList; import org.eclipse.jgit.lib.BatchRefUpdate; import org.eclipse.jgit.lib.ObjectId; import org.eclipse.jgit.lib.ObjectIdRef; +import org.eclipse.jgit.lib.PersonIdent; import org.eclipse.jgit.lib.ProgressMonitor; import org.eclipse.jgit.lib.Ref; import org.eclipse.jgit.lib.RefDatabase; +import org.eclipse.jgit.lib.ReflogEntry; import org.eclipse.jgit.revwalk.RevObject; import org.eclipse.jgit.revwalk.RevTag; import org.eclipse.jgit.revwalk.RevWalk; @@ -188,6 +190,7 @@ class PackedBatchRefUpdate extends BatchRefUpdate { refdb.fireRefsChanged(); pending.forEach(c -> c.setResult(ReceiveCommand.Result.OK)); + writeReflog(pending); } private boolean checkConflictingNames(List commands) @@ -345,6 +348,81 @@ class PackedBatchRefUpdate extends BatchRefUpdate { return b.toRefList(); } + private void writeReflog(List commands) { + if (isRefLogDisabled()) { + return; + } + + PersonIdent ident = getRefLogIdent(); + 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) { + continue; + } + String name = cmd.getRefName(); + + if (cmd.getType() == ReceiveCommand.Type.DELETE) { + try { + RefDirectory.delete(w.logFor(name), RefDirectory.levelsIn(name)); + } catch (IOException e) { + // Ignore failures, see below. + } + continue; + } + + String msg = getRefLogMessage(); + if (isRefLogIncludingResult()) { + String strResult = toResultString(cmd); + if (strResult != null) { + msg = msg.isEmpty() + ? strResult : msg + ": " + strResult; //$NON-NLS-1$ + } + } + try { + w.log(name, cmd.getOldId(), cmd.getNewId(), ident, msg); + } catch (IOException e) { + // Ignore failures, but continue attempting to write more reflogs. + // + // In this storage format, it is impossible to atomically write the + // reflog with the ref updates, so we have to choose between: + // a. Propagating this exception and claiming failure, even though the + // actual ref updates succeeded. + // b. Ignoring failures writing the reflog, so we claim success if and + // only if the ref updates succeeded. + // We choose (b) in order to surprise callers the least. + // + // Possible future improvements: + // * Log a warning to a logger. + // * Retry a fixed number of times in case the error was transient. + } + } + } + + private String toResultString(ReceiveCommand cmd) { + switch (cmd.getType()) { + case CREATE: + return ReflogEntry.PREFIX_CREATED; + case UPDATE: + // Match the behavior of a single RefUpdate. In that case, setting the + // force bit completely bypasses the potentially expensive isMergedInto + // check, by design, so the reflog message may be inaccurate. + // + // Similarly, this class bypasses the isMergedInto checks when the force + // bit is set, meaning we can't actually distinguish between UPDATE and + // UPDATE_NONFASTFORWARD when isAllowNonFastForwards() returns true. + return isAllowNonFastForwards() + ? ReflogEntry.PREFIX_FORCED_UPDATE : ReflogEntry.PREFIX_FAST_FORWARD; + case UPDATE_NONFASTFORWARD: + return ReflogEntry.PREFIX_FORCED_UPDATE; + default: + return null; + } + } + private static Map byName( List commands) { Map ret = new LinkedHashMap<>(); From a1e11461cc2d2c83c4898bde7ad885baaecd0b14 Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Wed, 19 Jul 2017 13:55:16 -0400 Subject: [PATCH 3/3] BatchRefUpdate: Expand javadocs and add @Nullable Change-Id: I22d739a9677e24f36323dceadf7d375ac2f446e8 --- .../org/eclipse/jgit/lib/BatchRefUpdate.java | 23 ++++++++++++++----- 1 file changed, 17 insertions(+), 6 deletions(-) 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 aa85cc7b2..4f1299a6c 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/lib/BatchRefUpdate.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/lib/BatchRefUpdate.java @@ -177,11 +177,17 @@ public class BatchRefUpdate { * @return message the caller wants to include in the reflog; null if the * update should not be logged. */ + @Nullable public String getRefLogMessage() { return refLogMessage; } - /** @return {@code true} if the ref log message should show the result. */ + /** + * Check whether the reflog message should include the result of the update, + * such as fast-forward or force-update. + * + * @return true if the message should include the result. + */ public boolean isRefLogIncludingResult() { return refLogIncludeResult; } @@ -190,12 +196,11 @@ public class BatchRefUpdate { * Set the message to include in the reflog. * * @param msg - * the message to describe this change. It may be null if - * appendStatus is null in order not to append to the reflog + * the message to describe this change. If null and appendStatus is + * false, the reflog will not be updated. * @param appendStatus * true if the status of the ref change (fast-forward or - * forced-update) should be appended to the user supplied - * message. + * forced-update) should be appended to the user supplied message. * @return {@code this}. */ public BatchRefUpdate setRefLogMessage(String msg, boolean appendStatus) { @@ -213,6 +218,8 @@ public class BatchRefUpdate { /** * Don't record this update in the ref's associated reflog. + *

+ * Equivalent to {@code setRefLogMessage(null, false)}. * * @return {@code this}. */ @@ -222,7 +229,11 @@ public class BatchRefUpdate { return this; } - /** @return true if log has been disabled by {@link #disableRefLog()}. */ + /** + * Check whether log has been disabled by {@link #disableRefLog()}. + * + * @return true if disabled. + */ public boolean isRefLogDisabled() { return refLogMessage == null; }