Today, whenever git tf checkin is executed a lock is acquired on the root of directory that is mapped in git-tf. Some users have complained that this is blocking them pretty often when files they did not touch is locked by their peers.

To better handle this I think we can do the following when git-tf checkin is executed:

Shallow Mode

1) Determine the next changeset number (CS#)

2) Do not lock the directory

3) Perform the checkin

4) If the changeset number checked in is equal to CS# determined in step 1 then the repo is in good state

5) if the changeset number is NOT equal to the CS# this means that there have been a change that was checked in before our change and our repo might not be in a good state so we need to perform further checks

- Look at the CS that have been checked in, if any of the files are in the scope of the directory mapped by git-tf in the repo then indeed our repo is not in sync with TFS. So mark the repo as in bad state (in the config) if the user tries to perform any operation ask the user to execute "git tf fetch --force" to fix the repo first

6) Provide the user a way to fix this using git tf fetch --force OR git tf pull --force ... these commands will perform a forced get in the repo to fix the state.

Deep Mode

I think we still need to keep the lock in deep mode because deep checkins can take a while depending on the number of commits. However, I think using the same technique used in shallow mode can also work.

I wrote up some background to this conversation at http://blogs.msdn.com/b/ethomson/archive/2012/08/28/locking-and-git-tf.aspx so that anybody who wants to participate can have some background on why we take a lock in the first place.

Youhana, let me think about this a little bit...  I had some idea in my head that we could avoid updating the HWM during checkin and wait until a subsequent pull happened, but that's not going to work.

What if, in shallow mode, we perform steps 1-4 as you suggest, but go ahead and fix the problem.

Imagine my HEAD is some git commit whose parent corresponds to changeset 3.  You checkin changeset 4 while I'm in the middle of git-tf checking in (without a lock.)  Now we get your changeset 4 as a commit parented off the commit that corresponds to changeset 3.  Then we merge those two commits (there should be no conflicts) and create a new commit, update HEAD to that and mark that as changeset (whatever changeset number I checked in.)

Ie:

42afdc (CS 3)  <-  93ef1a (my HEAD)

becomes:

                acf3a4 (Your CS 4)
              /                      \
42afdc (CS 3)                          b49a8 (My CS 5)
              \                      /
                93ef1a (my old HEAD) 

I suppose there could be some issues if you had more commits that had 93ef1a as a parent...?  You'd have to merge in b49a8...  Does that seems reasonable, though?

Thanks Ed for the blog post and for the reply. I think there are two different questions here:

1) If we detected this issue during check in should we fix it as part of the check in operation or should we notify the user of the error and have the user run another command to fix it ?

I think we need to provide fetch --force in both cases, just in case something goes wrong while we are fixing the repository we should provide the user of a way to re-run the "fixing" operation if needed. I am open to either trying to fix it if we detect the case during checkin or later.

2) How to fix this issue ?

I do not think we need to perform a merge between (CS 4) and (93ef1a) above, Since your and my checkin were submitted successfully, then the content in TFS should rule. Thus all we really need to do is fetch CS 4 parent it to CS3 and fetch CS5 and parent it to both CS4 and your Changeset (93ef1a) ... There is no need to do any merging here as we know the content we want to end up with.

What do you think ?

Oh, right, I see what you mean.  Yeah, sorry, I guess I was trying to say "represent changeset 5 as the result of a merge" - or more accurately, "represent changeset 5 as the product of two parents", not actually perform a merge.

I think we should only warn the user when someone else successfully sneaks in one (or more?) changesets before we checking in, and automatically do whatever operations we need in order to checkin our changes.

I agree with Youhana with going with fetching again the changes between HWM and latest changeset from TFS. The warning is just to inform the user that their checked in changes are going to be effectively "rebased" on top of the changeset(s) the other user checked in, instead of the common case where our checked in commit just becomes the new HWM.

Yes, certainly we'll only warn in the unlikely event that somebody snuck in a changeset between our query history and our checkin.

We haven't yet talked about conflicts.  Do we downloading the offending changeset and try to merge?  I'm inclined to say "yes" - try to automerge, if it fails, make the user fix and commit, and then git-tf checkin again.

So what about deep mode?  Youhana, I see why you're saying we should keep lock on deep.  If there are no conflicts, we can behave the same way we were before, it will just add a happy little branch to the git graph.  However, if there are conflicts, things are a bit trickier.  I think maybe in the case of a conflict in one of the intermediate changesets, we could rebase the rest off the merged commit.  That seems sort of meh in the best case scenario.  Do we let the user fix it?  Seems also meh.

I think we can and should improve shallow in the near-term.  I think we should play around with deep to see how bad it is if we turn off locking.

I think that in case that there is a conflict, check in will fail and we will abort the operation, the user will then need to fetch, merge and rerun git tf checkin. I do not think we should automerge and retry to submit the checkin automatically.

I am on the fence with deep, I guess I need to think more about it. However, I am inclined to say that for deep I would prefer us to continue to lock. The reason is, if the user wants to checkin deep then the user cares to keep his history intact, for example if in git the user has commits "ae56" <- "b5ac" <- "de3a" he would expect to see them in that order in TFS. Having another user sneak another changeset in between defeats the purpose of "deep" in my opinion. (Specially because there is no way to fix this in tfs once it is checked in). I think we can make it work technically, but I think we will lose the value of deep in this situation functionally. I think users operating in deep mode would expect "git-tf" to protect them from having different history in git and tfs.

I definitely don't want to try to resubmit the checkin.  (Sorry if that was unclear above, that sentence was sort of madness.)  I am suggesting that we behave like TFS 2012 wherein we just go ahead and do the merge and then stop.  (The user can then git-tf checkin again on their own.)

To do the automerge, we need to either run fetch or pull on behalf of the user. I think we can do it. I am not sure if we should do it though. I need to think about this for a bit. In the case where the user does not have latest and tries to checkin we block the user with an error message saying you need to fetch latest first, we do not try to fix it for the user the same way TFS does. I think we should be consistent here to keep git-tf commands consistent with each other. Fail, give the user guidance and abort.

What do you think ?

Sounds reasonable.  Sounds easier, too, which is always an added bonus.