Git Client Plugin: Advice on expanding MergeCommand

[t...@praqma.net]

Hello,

First of all, I'd like to apologize in advance for the rant I'm about to post.
This all started as a discussion with my colleagues, which lasted way too long, on a simple method name.
I'd appreciate it if you'd just tell us we're crazy and should just make a pull request.


We use the Git Client Plugin in one of our plugins and are lacking a couple of options, which we're adding at the moment.
I'm currently working on adding the --no-commit option for the MergeCommand and wanted some input on naming/default behaviour.

I added a 'setCommit' method to MergeCommand, accepting a boolean which defaults to true.

For JGit, I call JGit's own 'setCommit', passing in the boolean.
Looking at JGit's implementation, I realized that this changes the default behaviour of the Git Client Plugin (as did the implementations of the squash/message options, by the way).
If JGit's 'setCommit' isn't called, it will look at the merge configuration of the repository.
So simply by calling the JGit methods in our implementation, we're already enforcing our own defaults.
Not to make a fuss about it, just wanted to bring that up.

For the CLI implementation, I only add '--no-commit' when setCommit is set to false.
If setCommit is said to true, I do nothing. This is in line with the implementations of the other options.
I didn't want to explicitly add '--commit' when it was set to true, because that would change the default behaviour of the plugin.
That's the way I like it, but it's not in line way the JGit implementation, where we always set either --commit or --no-commit.

I wouldn't mind hearing your opinions on the matter.

Kind regards,
Thierry

[Mark Waite]

On Wed, Sep 16, 2015 at 3:18 AM <t...@praqma.net> wrote:

I'm currently working on adding the --no-commit option for the MergeCommand and wanted some input on naming/default behaviour.

I added a 'setCommit' method to MergeCommand, accepting a boolean which defaults to true.

For JGit, I call JGit's own 'setCommit', passing in the boolean.
Looking at JGit's implementation, I realized that this changes the default behaviour of the Git Client Plugin (as did the implementations of the squash/message options, by the way).
If JGit's 'setCommit' isn't called, it will look at the merge configuration of the repository.
So simply by calling the JGit methods in our implementation, we're already enforcing our own defaults.
Not to make a fuss about it, just wanted to bring that up.

I think that is a bug.  It seems like it should be fixed before the MergeCommand.setMessage() call becomes part of a release of the git client plugin.

Any chance you have a test which shows the problem?

 

For the CLI implementation, I only add '--no-commit' when setCommit is set to false.
If setCommit is said to true, I do nothing. This is in line with the implementations of the other options.
I didn't want to explicitly add '--commit' when it was set to true, because that would change the default behaviour of the plugin.
That's the way I like it, but it's not in line way the JGit implementation, where we always set either --commit or --no-commit.

I wouldn't mind hearing your opinions on the matter.

I've tried where ever I could to keep the command line git and JGit implementations similar.  This seems like a case where you've detected an unnecessary and possibly undesirable behavioral difference between command line git and JGit.

I admit that JGit and command line git are different, but it is a great help (for me) to use those differences to assure that I understand the behavior of the plugin and its implementations.

 

Kind regards,
Thierry

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to jenkinsci-de...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/477f41cf-235a-4e8e-beeb-15edaaa4821e%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[t...@praqma.net]

Hey Mark,

I added two tests to a branch on my fork, which also contains my changes for adding --no-comit.
One fails for the JGit implementation, while the other fails for the CLI implementaion.
Yesterday's post was way too big, by the way, the whole thing just boils down to:

The current JGit implementation always overrides repository defaults, even when options weren't explicitly set.
The current CLI implementation sometimes doesn't override repository defaults, even when options were explicitly set.

So everything works fine, but things get a bit confusing once you start changing the repo defaults.
We'd have to keep track of which options the user actually set in the command builder and only add those.

To imagine this all started with "Should I call this method setCommit or setNoCommit?".. :)

Kind regards,
Thierry