diff options
author | David Pursehouse <david.pursehouse@gmail.com> | 2017-07-28 05:40:45 -0400 |
---|---|---|
committer | Gerrit Code Review @ Eclipse.org <gerrit@eclipse.org> | 2017-07-28 05:40:45 -0400 |
commit | 7e4946626e462534852b32216c7c345e38b11081 (patch) | |
tree | 14c5e410de20041682edef4b635af763d2babff9 | |
parent | b0695e5b7b639797280916201e49076784377bd5 (diff) | |
parent | a1e11461cc2d2c83c4898bde7ad885baaecd0b14 (diff) | |
download | jgit-7e4946626e462534852b32216c7c345e38b11081.tar.gz jgit-7e4946626e462534852b32216c7c345e38b11081.zip |
Merge changes from topic 'batch-ref-update-reflog'
* changes:
BatchRefUpdate: Expand javadocs and add @Nullable
PackedBatchRefUpdate: Write reflogs
Extract constants for reflog entry message prefixes
5 files changed, 449 insertions, 10 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 06c47acf21..a51a910fb2 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<String, ReflogEntry> oldLogs = + getLastReflogs("refs/heads/master", "refs/heads/branch"); + assertEquals(Collections.singleton("refs/heads/master"), oldLogs.keySet()); + + List<ReceiveCommand> 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<String, ReflogEntry> oldLogs = getLastReflogs( + "refs/heads/master", "refs/heads/branch1", "refs/heads/branch2"); + List<ReceiveCommand> 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<ReceiveCommand> 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<ReceiveCommand> 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<ReceiveCommand> 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<ReceiveCommand> 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<ReceiveCommand> 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<ReceiveCommand> 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<String, ReflogEntry> oldLogs = + getLastReflogs("refs/heads/master", "refs/heads/branch"); + + List<ReceiveCommand> 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<ReceiveCommand> cmds) { BatchRefUpdate u = refdir.newBatchUpdate(); if (atomic) { @@ -410,4 +646,84 @@ public class BatchRefUpdateTest extends LocalDiskRepositoryTestCase { r.p.test(c)); } } + + private Map<String, ReflogEntry> getLastReflogs(String... names) + throws IOException { + Map<String, ReflogEntry> 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<String, ReflogEntry> 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 65f9ec42a6..90155cbacf 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<ReceiveCommand> commands) @@ -345,6 +348,81 @@ class PackedBatchRefUpdate extends BatchRefUpdate { return b.toRefList(); } + private void writeReflog(List<ReceiveCommand> 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<String, ReceiveCommand> byName( List<ReceiveCommand> commands) { Map<String, ReceiveCommand> ret = new LinkedHashMap<>(); 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 3c1916b642..1105352524 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/BatchRefUpdate.java b/org.eclipse.jgit/src/org/eclipse/jgit/lib/BatchRefUpdate.java index aa85cc7b29..4f1299a6cb 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. + * <p> + * 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; } 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 8bd0633789..afa6521d67 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/lib/ReflogEntry.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/lib/ReflogEntry.java @@ -50,6 +50,39 @@ package org.eclipse.jgit.lib; public interface ReflogEntry { /** + * Prefix used in reflog messages when the ref was first created. + * <p> + * 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. + * <p> + * Untranslated, and exactly matches the + * <a href="https://git.kernel.org/pub/scm/git/git.git/tree/builtin/fetch.c?id=f3da2b79be9565779e4f76dc5812c68e156afdf0#n680"> + * untranslated string in C git</a>. + * + * @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. + * <p> + * Untranslated, and exactly matches the + * <a href="https://git.kernel.org/pub/scm/git/git.git/tree/builtin/fetch.c?id=f3da2b79be9565779e4f76dc5812c68e156afdf0#n695"> + * untranslated string in C git</a>. + * + * @since 4.9 + */ + public static final String PREFIX_FORCED_UPDATE = "forced-update"; //$NON-NLS-1$ + + /** * @return the commit id before the change */ public abstract ObjectId getOldId(); |