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

Re: [EGIT PATCH 1/3] Add ref rename support to JGit

From
Shawn O. Pearce <spearce@spearce.org>
Date
May 7, 2009, 15:51 UTC
Message-ID
<20090507155117.GS30527@spearce.org>
In-Reply-To
<1241652781-16873-2-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.
See below.  I'm not going to apply as-is, because its really quite
messy, as you stated in the cover letter...
Show 16 quoted lines
> diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/RefDatabase.java b/org.spearce.jgit/src/org/spearce/jgit/lib/RefDatabase.java
> index 87f26bf..a73467a 100644
> --- a/org.spearce.jgit/src/org/spearce/jgit/lib/RefDatabase.java
> +++ b/org.spearce.jgit/src/org/spearce/jgit/lib/RefDatabase.java
> @@ -148,6 +149,25 @@ synchronized (this) {
>  	}
>  
>  	/**
> +	 * An set of update operations for renaming a ref
> +	 *
> +	 * @param fromRef Old ref name
> +	 * @param toRef New ref name
> +	 * @return a RefUpdate operation to rename a ref
> +	 * @throws IOException
> +	 */
> +	public RenameRefUpdates newRename(String fromRef, String toRef) throws IOException {

This class is not public; none of its methods are public; please don't mark this public.

Show 7 quoted lines
> +		refreshPackedRefs();
> +		Ref f = readRefBasic(fromRef, 0);
> +		Ref t = readRefBasic(toRef, 0);
> +		if (t != null)
> +			throw new IOException("Ref rename target exists: " + t.getName());
> +		RefUpdate refUpdateFrom = new RefUpdate(this, f, fileForRef(f.getName()));
> +		RefUpdate refUpdateTo = db.updateRef(toRef);

db.updateRef is just a redirection to this.newUpdate(), which scans the refs (done above) and then makes a new RefUpdate instance. Why not just reproduce the three lines that matter and avoid this messy indirection?

  if (t == null)
    t = new Ref(Ref.Storage.NEW, name, null)
  RefUpdate refUpdateTo = new RefUpdate(this, t, fileForRef(t.getName()));
Show 9 quoted lines
> @@ -484,8 +512,8 @@ private void lockAndWriteFile(File file, byte[] content) throws IOException {
>  	}
>  
>  	synchronized void removePackedRef(String name) throws IOException {
> -		packedRefs.remove(name);
> -		writePackedRefs();
> +		if (packedRefs.remove(name) != null)
> +			writePackedRefs();
>  	}

How did we get here when Ref.Storage.isPacked() == false? IMHO this shouldn't have been invoked if isPacked is false.

Show 10 quoted lines
> diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/RefLogWriter.java b/org.spearce.jgit/src/org/spearce/jgit/lib/RefLogWriter.java
> index a077051..bbf26eb 100644
> --- a/org.spearce.jgit/src/org/spearce/jgit/lib/RefLogWriter.java
> +++ b/org.spearce.jgit/src/org/spearce/jgit/lib/RefLogWriter.java
> @@ -58,6 +58,25 @@ static void append(final RefUpdate u, final String msg) throws IOException {
>  		appendOneRecord(oldId, newId, ident, msg, db, u.getName());
>  	}
>  
> +	static boolean renameTo(final Repository db, final RefUpdate from,
> +			final RefUpdate to) throws IOException {

Hmm, this method returns only true, or throws an IOException. I wonder, why bother returning boolean? Why not make it void?

Show 13 quoted lines
> +		final File logdir = new File(db.getDirectory(), Constants.LOGS);
> +		final File reflogFrom = new File(logdir, from.getName());
> +		if (!reflogFrom.exists())
> +			return true;
> +		final File reflogTo = new File(logdir, to.getName());
> +		final File refdirTo = reflogTo.getParentFile();
> +		if (!refdirTo.exists() && !refdirTo.mkdirs()) {
> +			throw new IOException("Cannot create directory " + refdirTo);
> +		}
> +		if (!reflogFrom.renameTo(reflogTo)) {
> +			reflogTo.delete(); // try
> +			throw new IOException("Cannot rename " + reflogFrom + " to "
> +					+ reflogTo);

So if the rename fails, we delete the target and then throw an exception anyway? Why delete the target in this case? I think we should just fail here.

Show 12 quoted lines
> diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/RefUpdate.java b/org.spearce.jgit/src/org/spearce/jgit/lib/RefUpdate.java
> index a9ab73b..8ecccfe 100644
> --- a/org.spearce.jgit/src/org/spearce/jgit/lib/RefUpdate.java
> +++ b/org.spearce.jgit/src/org/spearce/jgit/lib/RefUpdate.java
> @@ -50,6 +50,100 @@
>   * Updates any locally stored ref.
>   */
>  public class RefUpdate {
> +	/**
> +	 * A RefUpdate combination for renaming a ref
> +	 */
> +	public static class RenameRefUpdates {

I would prefer this to be called "RefRename", and to make it a top level class in the lib package, rather than a nested class of RefUpdate.

> +		RenameRefUpdates(final RefUpdate toUpdate, final RefUpdate fromUpdate,
> +				final RefUpdate headUpdate) {
> +			newToUpdate = toUpdate;
> +			oldFromDelete = fromUpdate;
What happened to headUpdate?
Show 6 quoted lines
> +		}
> +		private RefUpdate newToUpdate;
> +
> +		private RefUpdate oldFromDelete;
> +
> +		private Result renameResult;
Constructor after instance member field decls please.
Show 39 quoted lines
> +		/**
> +		 * @return the result of the new ref update
> +		 * @throws IOException
> +		 */
> +		public Result rename() throws IOException {
> +			LockFile lockFileFrom = new LockFile(oldFromDelete.looseFile);
> +			boolean lockFrom = lockFileFrom.lock();
> +			if (!lockFrom)
> +				return Result.LOCK_FAILURE;
> +			LockFile lockFileTo = null;
> +			try {
> +				lockFileTo = new LockFile(newToUpdate.looseFile);
> +				boolean lockTo = lockFileTo.lock();
> +				if (!lockTo) {
> +					lockFileFrom.unlock();
> +					return renameResult = Result.LOCK_FAILURE;
> +				}
> +			} catch (IOException e) {
> +				lockFileFrom.unlock();
> +				throw e;
> +			}
> +			LockFile lockFileHEAD = new LockFile(new File(oldFromDelete.db.getRepository().getDirectory(), Constants.HEAD));
> +			boolean renameHEADtoo;
> +			try {
> +				boolean lockHEAD = lockFileHEAD.lock();
> +				renameHEADtoo = oldFromDelete.db.readRef(Constants.HEAD).getName().equals(oldFromDelete.getName());
> +				if (!renameHEADtoo)
> +					lockFileHEAD.unlock();
> +				else {
> +					if (!lockHEAD) {
> +						lockFileFrom.unlock();
> +						lockFileTo.unlock();
> +						return renameResult = Result.LOCK_FAILURE;
> +					}
> +				}
> +			} catch (IOException e) {
> +				lockFileHEAD.unlock();
> +				lockFileFrom.unlock();
> +				lockFileTo.unlock();

I wonder if this code would be easier to follow if we made a Set<LockFile> as an instace member, and made some utility methods like:

  private final Set<LockFile> allLocks = new HashSet<LockFile>();
  private boolean takeLock(final LockFile f) {
    if (f.lock()) {
      allLocks.add(f);
      return true;
    }
    return false;
  }
  private void unlock(final LockFile f) {
    f.unlock();
    allLocks.remove(f);
  }
  private void abort() {
    for (final LockFile f : allLocks)
      f.unlock();
  }
Then a lot of this local variable management code might simplify out.

Another approach might be to make LockFile a member of RefUpdate, and allow the RefUpdate members for oldFromDelete and newToUpdate to keep track of their associated LockFile for you. This may clean up the code related to constructing the store operation.

Show 8 quoted lines
> +				throw e;
> +			}
> +			try {
> +				UpdateStore toStore = newToUpdate.newUpdateStore();
> +				if (RefLogWriter.renameTo(oldFromDelete.getRepository(), oldFromDelete, newToUpdate)) {
> +					newToUpdate.setNewObjectId(oldFromDelete.getOldObjectId());
> +					newToUpdate.setExpectedOldObjectId(oldFromDelete.getOldObjectId());
> +					newToUpdate.setRefLogMessage("jgit branch: renamed " + oldFromDelete.getName() + " to " + oldFromDelete.getName(), false);

I would prefer if the caller can set the prefix part of the reflog message ("jgit branch" portion) so we can set it by command.

Show 7 quoted lines
> +					newToUpdate.result = toStore.store(lockFileTo, Result.RENAMED);
> +					DeleteStore fromStore = oldFromDelete.newDeleteStore();
> +					Result store = fromStore.store(lockFileFrom, Result.RENAMED);
> +					if (renameHEADtoo) {
> +						final byte[] content = Constants.encode("ref: " + newToUpdate.getName() + "\n");
> +						lockFileHEAD.write(content);
> +						synchronized (this) {

What effect could "synchronized (this)" really have here, other than to waste CPU cycles? Who else could have a reference to this RefRenames instance and be able to lock on it?

Show 7 quoted lines
> @@ -478,6 +578,8 @@ else if (status == Result.FAST_FORWARD)
>  				msg += ": fast forward";
>  			else if (status == Result.NEW)
>  				msg += ": created";
> +			else if (status == Result.RENAMED)
> +				msg += ": renamed";
>  		}

I don't think this is useful. We probably shouldn't append status during a rename as the rename method had already done setRefLogMessage("renamed A to B") earlier. Doing status here would yield "renamed A to B: renamed" in the reflog; ugly.

Show 21 quoted lines
> diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/Repository.java b/org.spearce.jgit/src/org/spearce/jgit/lib/Repository.java
> index 5baa7a0..e704aeb 100644
> --- a/org.spearce.jgit/src/org/spearce/jgit/lib/Repository.java
> +++ b/org.spearce.jgit/src/org/spearce/jgit/lib/Repository.java
> @@ -1067,4 +1083,21 @@ public void scanForRepoChanges() throws IOException {
>  		getAllRefs(); // This will look for changes to refs
>  		getIndex(); // This will detect changes in the index
>  	}
> +
> +	/**
> +	 * Writes a symref (e.g. HEAD) to disk
> +	 *
> +	 * @param name
> +	 *            symref name
> +	 * @param target
> +	 *            pointed to ref
> +	 * @throws IOException
> +	 */
> +	public void link(final String name, final String target) throws IOException {
> +		refs.link(name, target);
> +	}
Uhm, isn't this already called writeSymref() ?
> +	void uncacheSymRef(String name) {
> +		refs.uncacheSymRef(name);
> +	}
Why is this in Repository?
-- 
Shawn.
Previous: Robin RosenbergNext: Robin Rosenberg
Message 5 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.