Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

checkin and checkintool overwrite upstream changes on modified files #199

Closed
jewer opened this Issue · 11 comments

3 participants

@jewer

Steps to reproduce:

  1. User 1 changes a file, commits in git, then pushes to TFS.
  2. User 2 changes same file, commits in git, fetches upstream changes, rebases, then pushes to TFS.
  3. User 1 changes same file, commits in git, then pushes to TFS WITHOUT rebasing on upstream changes.

Result:
User 1's change completely replaces User 2's change, no opportunity for merge is given and data is potentially lost.

rcheckin prevents this by short-circuiting the commit if there are upstream changes that should be fetched first. checkin and checkintool should probably do the same thing to prevent the possible loss of data.

Caveat: I see an issue with checkintool because it pops a dialog and the dialog itself does the actual commit. There's a possible window that, while the dialog is open, a TFS commit could have been made so there is always going to be a window for this to occur.

@jewer

Note that this issue is really only a problem if other users are pushing from other repositories or directly into TFS. If everyone is working from the same git repository, and there is a single repository being pushed to TFS you wouldn't have this problem.

@sc68cal

Currently, I believe only rcheckin does a check to see if there are any newer changesets in TFS, prior to checking in. You may want to use that instead.

@jewer

rcheckin does make that check. however, there is a window, albeit small, between the points when

a) rcheckin does the initial call to TFS to find upstream changes, and
b) the bridge does the actual push

that someone else could have made another upstream change. On a large repository with a correspondingly large index, that window can be measured in seconds. The issue described above can be experienced in that window as well.

@sc68cal

I agree completely with your diagnosis. I do not know how much we can mitigate that risk on our side, since TFS relies on clients setting the read-only flag so that files can be "locked" by specific users, to prevent these types of problems. Great design, right?

@sc68cal

We could make checkin and checkintool behave like rcheckin does, querying the latest changeset in the TFS system, and stopping if our latest does not match the latest on the server. We'd have to get the word out about a large change like that - I learned my lesson after removing the --build-default-comment flag that broke some people's scripts and gave them confusing error messages.

@jewer

After walking through the code, (which was really easy, BTW. kudos!) the impl for that feature is simple enough.

My larger concern is that, with the gap even technically possible, it makes using the bridge on a larger project almost nonviable. I haven't grok'd the workspace areas in the TFS api, but it looks like workspaces are the only way to construct pending changes. Just curious if anyone else has worked around the issue or just not noticed it for one reason or another ...

@spraints
Owner

Checkin (and checkintool) were implemented with the mindset of "try it and fail if it doesn't work." To be more specific, they take the changes from git, apply them to a TFS workspace (at the most recent TFS version that's an ancestor of the git commit), and checkin. If TFS has a newer version of one of the files, it'll make the whole checkin fail. It's similar to you creating a workspace (in place of git-tfs-clone), doing no tfs get, then trying to check in. Or, that was the intent. If the checkin succeeds, it fetches all the other TFS checkins, and marks the one you just created as a merge commit in git.

Does that make any sense? I can try to explain it a different way, if it doesn't.

@jewer

@spraints Got time for a skype or gtalk? The concept makes total sense, but I don't know if the bridge is actually following that logic or my issue wouldn't be reproducible.

@sc68cal

We are on IRC, freenode #git-tfs if you'd like to chat

@jewer

I can verify that this issue does occur as stated on TFS 2010, but is not reproducible in TFS 2012. Anedoctal conversation from others say it's not reproducible on 2005 but is reproducible on 2008.

Given the fact it took me hours to get everything set up for TFS, not sure it's worth going back to those much older versions to verify. I'm tempted to say, if everyone else can also reproduce this that the 2010 version should at least have the changes to checkin/checkintool, but even then there's a definite possibility of losing data on checkin.

Thoughts?

@jewer

Though, my gut tells me that there's gotta be some checkbox thingy somewhere that's preventing the merge commit window from popping (or just bypassing with some default "take local" return code)...

@spraints spraints closed this in #507
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.