Re: [jgit-dev] creating a new branch fails with LOCK_FAILURE if branch a

[Shawn Pearce <spearce@xxxxxxxxxxx>]

On Wed, Jan 4, 2012 at 06:30, Kempin, Edwin <edwin.kempin@xxxxxxx> wrote:
> this is how we create a new branch:
>
> final RefUpdate u = repo.updateRef(refname);
> u.setExpectedOldObjectId(ObjectId.zeroId());
> u.setNewObjectId(object.copy());
> u.setRefLogIdent(refLogIdent);
> u.setRefLogMessage("some message", false);
> final RefUpdate.Result result = u.update(rw);
> switch (result) {
>  case FAST_FORWARD:
>  case NEW:
>  case NO_CHANGE:
>    // branch creation was successful
>    ...
>    break;
>  default: {
>    throw new IOException(result.name());
>  }
> }
>
> When the branch creation fails because a branch with the name already exists LOCK_FAILURE is returned as result.

Yes. Because LOCK_FAILURE is returned when the reference's current
value does not match the expected old ObjectId. Using
ObjectId.zeroId() is only special in terms of looking for the
reference to not exist at all, otherwise it is treated the same as if
any other ObjectId was used and the reference no longer points at that
ObjectId.

> Is this the intended behavior?

Apparently. Changing the enum to add a new result code for
ALREADY_EXISTS is going to be a major API breakage. Any application
that uses RefUpdate will need to make sure it handles the new enum
case. JGit alone has a lot of these places to audit.

I regret organizing this RefUpdate with an enum result. It has proven
much more difficult to work with than I had expected.

> Is LOCK_FAILURE only returned if the branch already exists or can it have other causes?

There may be other reasons.

If "refs/heads/foo" exists as a reference and you try to create
"refs/heads/foo/bar" (file within a file not supported), we return
LOCK_FAILURE.

If LockFile.lock() cannot create the new lock file atomically, we
return LOCK_FAILURE.

If LockFile.commit() fails, we return LOCK_FAILURE and not IO_FAILURE.

None of the above really mean the reference already exists. They just
mean we couldn't carry about the update because of something.  :-(

> If there are also other possible causes, wouldn't it be better to rather have an explicit result for this case, e.g. ALREADY_EXISTS?
> Then it would be easier to handle this case (e.g. to print out an appropriate error message).

Probably. But I regret having this huge enum with different states. It
may have been cleaner to have just returned true or false, with other
reporting methods like:

  boolean wasForceUpdate();
  boolean wasExisting();
  boolean wasWrongObjectId();
  boolean wasNoChange();
  boolean wasCreatedReference();
  boolean Exception getLastException();
  ...

for additional detail.