]> source.dussan.org Git - jgit.git/commit
DfsPackDescription: Disallow null PackSource 95/123695/3
authorDave Borowitz <dborowitz@google.com>
Wed, 30 May 2018 00:53:26 +0000 (17:53 -0700)
committerDave Borowitz <dborowitz@google.com>
Fri, 1 Jun 2018 16:40:35 +0000 (12:40 -0400)
commit43ec590d0e7c6b294bcbb1f7e4c2490b6b984c20
tree092a8e41c90aca7f62e63879d1f08526b7cce7d2
parent621880bc9cafe547596bbd3c863a3a08780b13e4
DfsPackDescription: Disallow null PackSource

In normal operation, the source of a pack should never be null; the DFS
implementation should always know where a pack came from. Existing
implementations in InMemoryRepository and at Google always have the
source available at construction time.

The problem with null PackSources in the previous implementation was it
made the DfsPackDescription#compareTo method intransitive. Specifically,
it skips comparing the sources at all if *either* operand is null.
Suppose we have three descriptions A, B, and C, where all fields are
equal except the PackSource, and:
 * A's source is INSERT
 * B's source is null
 * C's source is RECEIVE
In this case, A.compareTo(B) == 0, and B.compareTo(C) == 0, since all
fields are equal except the source, which is skipped. But
A.compareTo(C) != 0, since A and B have different sources.

Avoid this problem in compareTo by enforcing that the source is never
null. We could of course assign an arbitrary category number to a null
source in order to make comparison transitive[1], but it's simpler to
implement and reason about if the field is non-nullable, and there is no
real-world use case to make it null.

Although a non-null source is required at construction time, the field
is currently still mutable: DfsPackDecscription#setPackSource is used by
DfsInserterTest to mark packs as garbage. This could probably be
avoided as well, allowing us to convert packSource to a final field, but
doing so is beyond the scope of this change.

[1] The astute reader will notice this is already done by
    DfsObjDatabase#reftableComparator(). In fact, the reason that
    different comparator implementations non-obviously have different
    semantics for this nullable field is another reason why it's clearer
    to avoid null entirely.

Change-Id: I85a2aaf3fd6d4868f241f7972a0349f087830ffa
org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsObjDatabase.java
org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsPackDescription.java
org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsReader.java
org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/InMemoryRepository.java