git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [EGIT PATCH 08/10] Add ref rename support to JGit

From
Shawn O. Pearce <spearce@spearce.org>
Date
Jun 3, 2009, 16:43 UTC
Message-ID
<20090603164330.GI3355@spearce.org>
In-Reply-To
<1243462137-24133-9-git-send-email-robin.rosenberg@dewire.com>
Robin Rosenberg <robin.rosenberg@dewire.com> wrote:
> Now refs can be renamed. The intent is that should be safe. Only the named
> refs and associated logs are updated. Any symbolic refs referring to the renames
> branches are unaffected, except HEAD, which is updated if the branch it refers
> to is being renamed.
...
Show 5 quoted lines
> +//	public void testRenameBranchCannotLockAFileHEADisToLockHead() throws IOException {
> +//		an example following the rename failure pattern, but this one is ok!
> +//		tryRenameWhenLocked("HEAD", "refs/heads/b", "refs/heads/new/name",
> +//				"refs/heads/new/name");
> +//	}
Commented out test case?  Eh?
Show 5 quoted lines
> +//	public void testRenameBranchCannotLockAFileHEADisOtherLockHead() throws IOException {
> +//		an example following the rename failure pattern, but this one is ok!
> +//		tryRenameWhenLocked("HEAD", "refs/heads/b", "refs/heads/new/name",
> +//				"refs/heads/a");
> +//	}
Ditto.
> diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/RefDatabase.java b/org.spearce.jgit/src/org/spearce/jgit/lib/RefDatabase.java
...
Show 6 quoted lines
> +	RefRename newRename(String fromRef, String toRef) throws IOException {
> +		refreshPackedRefs();
> +		Ref f = readRefBasic(fromRef, 0);
> +		Ref t = readRefBasic(toRef, 0);
> +		if (t != null)
> +			throw new IOException("Ref rename target exists: " + t.getName());

NAK, I'd prefer to have the RefRename return successfully, but when we try to execute it, it has a result that fails with RefUpdate.REJECTED.

> diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/RefLogWriter.java b/org.spearce.jgit/src/org/spearce/jgit/lib/RefLogWriter.java
...
Show 10 quoted lines
> +	static void renameTo(final Repository db, final RefUpdate from,
> +			final RefUpdate to) throws IOException {
> +		final File logdir = new File(db.getDirectory(), Constants.LOGS);
> +		final File reflogFrom = new File(logdir, from.getName());
> +		if (!reflogFrom.exists())
> +			return;
> +		final File reflogTo = new File(logdir, to.getName());
> +		final File reflogToDir = reflogTo.getParentFile();
> +		File tmp = new File(logdir,"tmp-renamed-log");
> +		tmp.delete(); // if any exists

That's not thread safe. Most of the rest of JGit is actually thread safe. I think we should be here too. We should use a temporary file name inside logdir, one that isn't a legal ref name at all. Maybe:

  File tmp = File.createTempFile("tmp..renamed", "-log", logdir);
  if (!reflogFrom.renameTo(tmp)) {
    tmp.delete();
    if (!reflogFrom.renameTo(tmp))
      throw new IOException....
  }
Show 7 quoted lines
> +		if (!reflogToDir.exists() && !reflogToDir.mkdirs()) {
> +			throw new IOException("Cannot create directory " + reflogToDir);
> +		}
> +		if (!tmp.renameTo(reflogTo)) {
> +			throw new IOException("Cannot rename (" + tmp + ")" + reflogFrom
> +					+ " to " + reflogTo);
> +		}

If we fail here, should we attempt to put the tmp log file back into the old name? Or leave it orphaned under the tmp file?

> diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/RefRename.java b/org.spearce.jgit/src/org/spearce/jgit/lib/RefRename.java
...
Show 6 quoted lines
> +	public Result rename() throws IOException {
> +		Ref oldRef = oldFromDelete.db.readRef(Constants.HEAD);
> +		boolean renameHEADtoo = oldRef != null
> +				&& oldRef.getName().equals(oldFromDelete.getName());
> +		try {
> +			Repository db = oldFromDelete.getRepository();
Style nit: Hoist to top of method, so oldRef init can use it.
> +			RefLogWriter.renameTo(db, oldFromDelete,
> +					newToUpdate);
Style nit: This fits on one line.
> +			newToUpdate.setRefLogMessage(null, false);
> +			RefUpdate tmpUpdateRef = oldFromDelete.db.getRepository().updateRef(
> +					"RENAMED-REF");
Like the log, this isn't thread safe.
Show 10 quoted lines
> +			if (renameHEADtoo) {
> +				try {
> +					oldFromDelete.db.link(Constants.HEAD, "RENAMED-REF");
> +				} catch (IOException e) {
> +					RefLogWriter.renameTo(db,
> +							newToUpdate, oldFromDelete);
> +					return renameResult = Result.LOCK_FAILURE;
> +				}
> +			}
> +			tmpUpdateRef.setNewObjectId(oldFromDelete.getOldObjectId());
We should also do:
  oldFromDelete.setExpectedOldObjectId(oldFromDelete.getOldObjectId())

so that if the old ref was modified between the time we read it and the time we delete it, we fail on the delete, and don't whack a ref that was modified by another thread.

> +			RefLogWriter.append(this, "jgit branch: renamed "
I think this should just be "renamed: $old to $new".
Show 5 quoted lines
> +					+ db.shortenRefName(oldFromDelete.getName()) + " to "
> +					+ db.shortenRefName(newToUpdate.getName()));
> +			return renameResult = Result.RENAMED;
> +		} catch (RuntimeException e) {
> +			throw e;
What's the point of this catch block?
> +		} catch (IOException e) {
> +			System.err.println(e);
Please don't print to System.err from such a low level method.
> diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/RefUpdate.java b/org.spearce.jgit/src/org/spearce/jgit/lib/RefUpdate.java
...
Show 5 quoted lines
> +		/**
> +		 * The ref was renamed from another name
> +		 * <p>
> +		 */
> +		RENAMED

Is a new state really necessary? Can't we use NEW on success? After all, the dst is now created.

Show 5 quoted lines
>  	}
>  
>  	/** Repository the ref is stored in. */
> -	private final RefDatabase db;
> +	final RefDatabase db;
I wonder if this shouldn't just be a public accessor method; we expose
getRepository() on other application visible objects like RevWalk.
  
>  	/** Location of the loose file holding the value of this ref. */
> -	private final File looseFile;
> +	final File looseFile;
I don't see this used outside of the class, is it still necessary?
>  	/** Result of the update operation. */
> -	private Result result = Result.NOT_ATTEMPTED;
> +	Result result = Result.NOT_ATTEMPTED;
Ditto.
  
> +	static void deleteEmptyDir(File dir, int depth) {
> +		for (; depth > 0 && dir != null; depth--) {
> +			if (dir.exists() && !dir.delete())
Why bother with a stat() call, just to do an rmdir()?
> -	private static int count(final String s, final char c) {
> +	static int count(final String s, final char c) {

Since all callers pass '/', maybe we should just change this to use '/' as a constant in the loop and remove the char c parameter? I think that would fix the nasty line wrapping in RefLogWriter too.

-- 
Shawn.
Previous: Robin RosenbergNext: Shawn O. Pearce
Message 25 of 43 in “Rename refs”
  1. 0/3 Rename refsRobin Rosenberg, May 6, 2009
  2. 1/3 Add ref rename support to JGitRobin Rosenberg, May 6, 2009
  3. 2/3 Use Constants.R_* in Branch dialogRobin Rosenberg, May 6, 2009
  4. 3/3 Add ref rename support to the branch dialogRobin Rosenberg, May 6, 2009
  5. Shawn O. PearceMay 7, 2009
  6. 0/6 Ref renameRobin Rosenberg, May 19, 2009
  7. 1/6 Make sure we get the right storage for loose/pack/loose and packed refsRobin Rosenberg, May 19, 2009
  8. 2/6 Add ref rename support to JGitRobin Rosenberg, May 19, 2009
  9. 3/6 Add ref rename support to the branch dialogRobin Rosenberg, May 19, 2009
  10. 4/6 Allow non-ASCII ref names when writing the packed-refs fileRobin Rosenberg, May 19, 2009
  11. 5/6 Use Constants.PACKED_REFS in RefWriterRobin Rosenberg, May 19, 2009
  12. 6/6 Improve error reporting in the branch dialogRobin Rosenberg, May 19, 2009
  13. Shawn O. PearceMay 20, 2009
  14. 00/10 Rename supportRobin Rosenberg, May 27, 2009
  15. 01/10 Make sure we get the right storage for loose/pack/loose and packed refsRobin Rosenberg, May 27, 2009
  16. 02/10 Add a toString for debugging to RemoteRefUpdateRobin Rosenberg, May 27, 2009
  17. 03/10 Add a toString to LockFileRobin Rosenberg, May 27, 2009
  18. 04/10 Add more tests for RefUpdateRobin Rosenberg, May 27, 2009
  19. 05/10 Do not write to the reflog unless the refupdate logmessage is setRobin Rosenberg, May 27, 2009
  20. 06/10 Add a utility method for shortening long ref names to short ones.Robin Rosenberg, May 27, 2009
  21. 07/10 Set a nice reflog message in the branch commandRobin Rosenberg, May 27, 2009
  22. 08/10 Add ref rename support to JGitRobin Rosenberg, May 27, 2009
  23. 09/10 Add ref rename support to the branch dialogRobin Rosenberg, May 27, 2009
  24. 10/10 Improve error reporting in the branch dialogRobin Rosenberg, May 27, 2009
  25. Shawn O. PearceJun 3, 2009
  26. Shawn O. PearceJun 3, 2009
  27. Robin RosenbergJun 7, 2009
  28. Shawn O. PearceJun 7, 2009
  29. Shawn O. PearceMay 20, 2009
  30. Robin RosenbergMay 21, 2009
  31. Shawn O. PearceMay 21, 2009
  32. 0/7 Ref rename support againRobin Rosenberg, Jun 10, 2009
  33. 1/7 Add methods to RawParseUtils for scanning backwards.Robin Rosenberg, Jun 10, 2009
  34. 2/7 Add a ref log reader classRobin Rosenberg, Jun 10, 2009
  35. 3/7 Do not write to the reflog unless the refupdate logmessage is setRobin Rosenberg, Jun 10, 2009
  36. 4/7 Add ref rename support to JGitRobin Rosenberg, Jun 10, 2009
  37. 5/7 Add ref rename support to the branch dialogRobin Rosenberg, Jun 10, 2009
  38. 6/7 Improve error reporting in the branch dialogRobin Rosenberg, Jun 10, 2009
  39. 7/7 Remove a TAB from the message Egit generates into the reflog on commitRobin Rosenberg, Jun 10, 2009
  40. Shawn O. PearceJun 12, 2009
  41. Warn for unlocalized string literals in ui plugin.Robin Rosenberg, Jun 14, 2009
  42. Add ref rename support to the branch dialogRobin Rosenberg, Jun 14, 2009
  43. 6/7 Improve error reporting in the branch dialogRobin Rosenberg, Jun 14, 2009

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.