Correct author attribution for RCheckin Quick

[l3m]

I extended the code for correct author attribution (#336) and refactored some duplicate existing code.

• Implemented correct author attribution for RCheckin Quick Mode.
• Split up the large PerformRCheckin function into 3 parts:
  • PerformRCheckin
  • _PerformRCheckinQuick
  • _PerformRCheckin
• Created RCheckinCommit class to encapsulate identical code between the two _RCheckinCommit functions
  • Before, that code was simply duplicated in the function.

Open Questions

RCheckinCommit is in the RCheckin.cs file. I am not sure where to put it; can it stay, or in Utils? What would you suggest?

There's still some ugliness in Util/CommitSpecificCheckinOptionsFactory. I directly create and load the authors file. It's the only 'new AuthorsFile' outside of unit tests, I assume this is because of your DI framework. How can I solve this better?

[pmiossec]

I'm must say that, from a personal point of view, even if it's very good to refactor to delete duplicate code, I'm not very pleased by this rcheckin refactor :(

I have 2 consequent development that do big modifications in the PerformRCheckin() method : #353 and #355
If this refactoring is merge with master, I think it will be a hard way for me :(
I don't even know if what you factor with this pull request could still be with my modifications...

There is another thing that bother me. It's that we have 2 ways to perform rcheckin.
Since #317, the result of the 2 ways should be the same but rcheckin quick do it quicker (when you have lots of commits because it doesn't rebase all the commits)! I and my team always use rcheckin -quick for quite some time now without problems...
And rcheckin quick is/will be used in #353 (where rcheckin can't!)
So, a good decision should be to remove rcheckin to not have to maintain 2 similar way of do something.

But that's a team decision...

All that to say that I'm not very pleased to see that refactor happens NOW :(

[l3m]

Feel free to reject this pull request, if the refactoring doesn't fit into your plans. I do not need the quick version for my project, I only implemented this because of your request. It's just that I got confused by the existing code and did this to minimize the possibility of errors creeping in trying to keep the duplicate code consistent.

Also, if you look at the code, not a lot of things have changed; it should be easy to apply whatever change you did to this updated version - or not.

[pmiossec]

OK. We will see how it turns (which merge first and which must be adapted. If that's not difficult, I will try to do it...).

Because the objective is to have the 2 merged ;)

Because your fix really make sense when we use a bare repository (and rcheckin in bare repository use quick version. The normal version was unadaptable!)

[spraints]

@pmiossec - I'll let you make the call on which order to merge these.

[pmiossec]

@spraints After a careful review of what have been done in this pull request and some tries at merging things, there should be no big problems. So, I'm 👍 to merge that first.

@l3m
I do not need the quick version for my project, I only implemented this because of your request.
If I remember well your workflow, you rcheckin a lot of commits in tfs each time you do a release. So, you should REALLY consider using the quick version because it does exactly the same thing but with a really time gains when you rcheckin multiple commits. The more you have commits to rcheckin, the more quicker it will be (because there isn't rebase of remaining commits after the rcheckin of one commit). The quicker version was made for this use...

[pmiossec]

@spraints @sc68cal Could you review this PR? I'm interested in merging it. I did it and it should be OK...

[spraints]

I'll try to make some time to review it this week. (I keep trying to look at it at the end of the day, or when I only have a couple of minutes, and that's not going to work.)

[pmiossec]

To help you for the review, this is far easier in this case to review commit by commit, because :

• there is (nearly) nothing in the first commit, just tabs diff introduced by extracting 2 methods
• (nearly) nothing in the 2nd commit
• just a trivial refactoring in the 3rd commit.

Nothing scary...

PS : And One day, I think we should discuss about removing one of the 2 rcheckin methods (because there are doing the same thing, now). My heart goes to the quick version, that is the quicker ;) and the only one useable in a bare repository (next to come...)

[spraints]

I think I like the quick one better, but honestly I don't know that code very well, so it's just a gut feeling at this point.

[spraints]

Thanks for your patience. 😄