Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Fix the check-in generated commits to include only recent messages #346

Open
wants to merge 1 commit into
from

Conversation

Projects
None yet
4 participants

vitalybe commented Apr 9, 2013

Right now, if the common ancestor is on a different branch, than the one you check-in, the generated check-in message includes irrelevant messages.

This happens since this is the code that generates commit messages:

            foreach (LibGit2Sharp.Commit comm in
                _repository.Commits.QueryBy(new LibGit2Sharp.Filter { Since = head, Until = parentCommitish }))

When the until code is on a different branch, there is, in effect, no stopping condition.

I fixed the issue by limiting the commit with the message to always have a newer date than the ancestor commit.

Filter out irrelevant commit messages.
This is needed if the parentCommit is not on the same branch as the head

@sc68cal sc68cal commented on the diff Apr 9, 2013

GitTfs/Core/GitRepository.cs
@@ -331,9 +331,19 @@ private TfsChangesetInfo TryParseChangesetInfo(string gitTfsMetaInfo, string com
public string GetCommitMessage(string head, string parentCommitish)
{
var message = new System.Text.StringBuilder();
+
+ var parentCommit = (Commit)_repository.Lookup(parentCommitish, GitObjectType.Commit);
@sc68cal

sc68cal Apr 9, 2013

Contributor

I think there is an extension method _repository.Lookup() - I would prefer that to a typecast, if it exists.

@sc68cal

sc68cal Apr 9, 2013

Contributor

_repository.Lookup<Commit>()

@vitalybe

vitalybe Apr 10, 2013

Sure, that's a good idea. Will change.

Owner

spraints commented Apr 9, 2013

I like the general idea of this, but I don't like filtering by date... it seems like there are a lot of ways for it to filter out too much. In my mind, the ideal would be to provide a commit range, and git-tfs takes changes just from that range of commits. The default is tfs/default...HEAD, but allow both of those to be overridden. Right now, only HEAD can be changed with an option.

That said, it would be easier just to drop in an option that enables or disables the loop break that you've got here. Could you do that? A command-line option or a .git/config flag (or both) would work.

I like the general idea of this, but I don't like filtering by date... it seems like there are a lot of ways for it to filter out too much.

I agree it isn't the optimal way. The direction I originally was going to, was getting all the direct children of the ancestor commit (if he has more than 1), and using a list of ancestor+children as the filter's "Until". However, it proved rather difficult to get the children commits and moreover, I wasn't sure that it would always be a good idea.

That said, it would be easier just to drop in an option that enables or disables the loop break that you've got here. Could you do that? A command-line option or a .git/config flag (or both) would work.

Sure, I'll look into that. What would the default be, however?

Owner

spraints commented Apr 11, 2013

I'm fine with either as a default. I would guess that the old behavior is going to be more common. But, if you don't want the default you probably never want the old behavior? So a config option would be good for that. Have you seen the config reader?

I played with it a bit more. My problem seems to be is that in my company
we're using a gated check-in. This causes all the check-ins to fail, always.
Without a gate, git-tfs simulates a merge into the remote "tfs/default"
branch, which causes the comments to be correctly calculated, however, not
so in my case.

I do think my default behavior would be more correct for most - It would
work well both for gated and non-gated check-ins. Do you see a common
scenario in which my code would generate an incorrect message?

And I haven't seen the config reader, I'll try to find it, but it will be
helpful if you can point me to the right direction :)

On Thu, Apr 11, 2013 at 3:24 AM, Matt Burke notifications@github.comwrote:

I'm fine with either as a default. I would guess that the old behavior is
going to be more common. But, if you don't want the default you probably
never want the old behavior? So a config option would be good for that.
Have you seen the config reader?


Reply to this email directly or view it on GitHubhttps://github.com/git-tfs/git-tfs/pull/346#issuecomment-16210619
.

  • Vitaly
Owner

spraints commented Apr 11, 2013

Looking at this again, I think a rebase (e.g. you make changes, git-tfs fetch and rebase onto tfs/default) is the most likely thing to cause a problem. I was thinking that normal parallel work (e.g. you are making git commits while someone else is making tfs changesets) would, too, but if the right parent commit is used, that shouldn't be a problem. To fix the rebase thing, you might be able to switch from Committer to Author when comparing the date. There's also the problem of someone's machine having a bad local clock, or someone manually setting the author/committer date, but I don't feel the need to cover those cases.

Oops.. it's not a reader, it's RemoteConfigConverter. It should be pretty easy to update, and to add tests to.

Out of curiosity, have you tried using git tfs checkintool to do the gated checkin? I haven't used git-tfs with gated checkins, but I think, looking back at #46 and #78, git-tfs can work with them. It probably doesn't solve the comment problem, but it may make your checkins not fail?

Owner

pmiossec commented Apr 11, 2013

Don't know if that's what your are looking for but you could have a look to the class GitRepository.
Especially the methods :

  • ReadTfsRemotes() used to read remotes configs
  • CreateTfsRemote() where we set and unset config

To access config through _repository.Config

Out of curiosity, have you tried using git tfs checkintool to do the gated checkin? I haven't used git-tfs with gated checkins, but I think, looking back at #46 and #78, git-tfs can work with them. It probably doesn't solve the comment problem, but it may make your checkins not fail?

I did use "checkintool". In case of a gated checkin, in TFS, the failure is actually expected.

The standard flow in TFS (without GitTfs) is:

  1. Check-In your changes
  2. The check-in "fails" and is preserved as a "shelve"
  3. You have the option to revert your changes or continue working as usual.
  4. Once the gate accepts your change, it runs an "undo checked-in"-like procedure

With GitTfs it is the same basic principle - Even though you "check-in", as far as Git concerned, you never merged to to the remote tfs branch. When the gate succeeds, you have to manually run "git tfs fetch" and merge the changes back to your development branch.

I've been using my fix for a while, and in some cases it doesn't work as expected. I guess I'll have to rethink the whole thing (unless you have an idea).

For example, consider the following graph:

-a---b [tfs/default]
      \
-q-----c [tfs] 
        \
-w---t---d---e---f [development]

tfs being a local branch, tfs/default is the remote tfs branch and development is where I developed my feature.

Next I perform a merge:

-a---b [tfs/default]
      \
-q-----c------------g [tfs] 
        \          /
-w---t---d---e---f [development]

At this point, I checkout tfs and I run git tfs ct.

The commit messages I'd like to get, in this case are: g and c - The commit messages of the commits from the ancestor, to the current head.

However, without my fix, I get the commit messages of all the commits. With my fix, I get the following: g, f, e, d, c, which also doesn't make a whole lot of sense.

I've been looking on the code that uses git_revwalk_next to walk from the head (g) to the common ancestor (b) and I don't really understand its logic. It seems to be going like so: g-->f-->e-->....
Basically, it chooses the development branch, even though g has two parents.

I tried to read the docs of the walker but haven't progressed too far. Anyone knows what's going on? How does the revwalk choose the branch to walk in case of a merge?

It seems to me, that if I could just use the logic that goes on the right path from the HEAD to the common ancestor, I could generate the correct message.

@sc68cal sc68cal referenced this pull request Sep 30, 2013

Closed

New release? #451

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment