Re: [EGIT PATCH 3/3] Add a ref log reader class

["Shawn O. Pearce" <spearce@spearce.org>]

Robin Rosenberg <robin.rosenberg@dewire.com> wrote:
> diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/ReflogReader.java b/org.spearce.jgit/src/org/spearce/jgit/lib/ReflogReader.java
> new file mode 100644
> index 0000000..15591af
> --- /dev/null
> +++ b/org.spearce.jgit/src/org/spearce/jgit/lib/ReflogReader.java
> @@ -0,0 +1,186 @@
...
> +public class ReflogReader {
> +	/**
> +	 * Parsed reflog entry
> +	 */
> +	static public class Entry {
> +		Entry(byte[] raw, int pos) {
> +			oldId = ObjectId.fromString(raw, pos);
> +			if (raw[pos + Constants.OBJECT_ID_LENGTH * 2] != ' ')
> +				throw new IllegalArgumentException(
> +						"Raw log message does not parse as log entry");

Please, for the sake of everyone's sanity, increment pos after you
read oldId.  Then its just raw[pos++] != ' ' here, and the next
line for newId is even shorter.

> +			int p1 = RawParseUtils.next(raw, p0 + 1, ':');
> +			if (p1 == -1)
> +				throw new IllegalArgumentException(
> +						"Raw log message does not parse as log entry");

Technically, missing a ':' is legal.  Everything after the '\t'
is the comment.  It may be splittable into an action/comment,
it might not be.

> +			action = RawParseUtils.decode(raw, p0, p1-1).trim();
> +			int p2 = RawParseUtils.nextLF(raw, p1 + 1);
> +			if (p2 == -1)
> +				throw new IllegalArgumentException(
> +						"Raw log message does not parse as log entry");
> +			comment = RawParseUtils.decode(raw, p1, p2).trim();

trim() on these shouldn't be necessary.  If you parse the line
right, you can avoid handing the '\t' and the '\n' to the decode,
and then whatever is left is what was given to the logger, and we
should faithfully return that to the caller.  Its unlikely to have
unncessary whitespace, so the trim is just wasting CPU looking
for it.

> +		private ObjectId oldId;
> +
> +		private ObjectId newId;
> +
> +		private PersonIdent who;
> +
> +		private String action;
> +
> +		private String comment;

Style nit: I much prefer fields to appear before the constructor.

> +	}
...
> +	public List<Entry> getReverseEntries(int max) throws IOException {
> +		FileInputStream fileInputStream;
> +		try {
> +			fileInputStream = new FileInputStream(logName);
> +		} catch (FileNotFoundException e) {
> +			return Collections.emptyList();
> +		}

Please ensure fileInputStream doesn't leak and is closed before
this method block returns.

> +		if (logName.length() > Integer.MAX_VALUE)
> +			// implementation limit, will suck with smaller files too
> +			throw new IOException("Cannot handle reflog larger than "
> +					+ Integer.MAX_VALUE + " bytes");

Style nit: Please wrap this huge block in {}, its easier to read
when there are more than one line dangling below.

> +		byte[] log = new byte[(int) logName.length()];

Please use fileInputStream.getChannel().size() as it does an fstat()
rather than a stat() on the path.

But I'd rather just read the file backwards, with a RandomAccessFile
and what amounts to a reverse version of BufferedInputStream.

> +		NB.readFully(fileInputStream, log, 0, log.length);
> +		int rs = log.length - 2; // skip terminating \n

If the file is currently being appended to, it might not end in
'\n'.  Something to keep in mind.

-- 
Shawn.