Implemented a way to configure submissions

[hazbo]

I've made a start on what you suggested in #35. It seems to work as expected. Adding a --merge flag when git is configured to rebase for example will overwrite the rebase configuration.

This will fallback to 'merge' by default as I believe it did before.

Is this the kind of thing you were after?

[jishi9]

I noticed that the submit command will fail silently (exit status 1, but no error message) if the config key appraise.submit doesn't exist or was set to a bad value (other than "merge" or "rebase"). I arrived at this state by running these commands on a branch:

git appraise request
git appraise comment -lgtm
git appraise submit 

I think the reason no error message is being displayed is because git config doesn't display an error message for a missing config key.

We will need to check the exit code of git config; according to the docs, an exit code of 1 for git config means "the section or key is invalid".

We also need to check that that the retrieved config value is one of 'merge' or 'rebase'

[jishi9]

That's interesting.. mixed opinions on whether you can get the exit code :( http://stackoverflow.com/questions/10385551/get-exit-code-go

[hazbo]

I came up with something like this, so instead of:

// GetMergeStrategy returns the way in which a submitted review is merged
func (repo *GitRepo) GetMergeStrategy() (string, error) {
    return repo.runGitCommand("config", "appraise.submit")
}

would this (although it may seem a little strange as we're essentially avoiding the error if the message is blank in this particular instance) work:

// GetMergeStrategy returns the way in which a submitted review is merged
func (repo *GitRepo) GetMergeStrategy() (string, error) {
    mergeStrategy, err := repo.runGitCommand("config", "appraise.submit")
    if err != nil && mergeStrategy == "" {
        return "", nil
    } else if err != nil {
        return mergeStrategy, err
    }
    return mergeStrategy, nil
}

Thoughts?

[hazbo]

The reason I'd suggest ignoring the error (providing it comes with an empty string in this case) is that I'd assume we'd still want to to just fall back to the default (merge in this case) if the user has not specified via git config what this will be.

[jishi9]

would this (although it may seem a little strange as we're essentially avoiding the error if the message is blank in this particular instance) work

I think that would work, though I'm concerned that we're treating all errors arising from the git config command as meaning "the key doesn't exist". man git-config lists a surprising number of possible exit codes :)

    This command will fail with non-zero status upon error. Some exit codes are:
     1. The config file is invalid (ret=3),
     2. can not write to the config file (ret=4),
     3. no section or name was provided (ret=2),
     4. the section or key is invalid (ret=1),
     5. you try to unset an option which does not exist (ret=5),
     6. you try to unset/set an option for which multiple lines match (ret=5), or
     7. you try to use an invalid regexp (ret=6).
    On success, the command returns the exit code 0.

I'd assume we'd still want to to just fall back to the default (merge in this case) if the user has not specified via git config what this will be

That makes sense. I'm thinking of writing a getGitConfig function that allows for an 'optional' return type, or maybe a default value.

See also my pull request #37 , if you patch that change in your branch and try the submit command then a clearer error message is displayed.

[hazbo]

I agree, I think a getGitConfig abstraction is a good idea, as we currently use this to get the email, text editor and (hopefully soon) the merge strategy (which may actually be better named, submit strategy).

This way we're in a safer position for all counts of git config being ran. I'll merge your patch into my branch and give this a quick test.

[googlebot]

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

[ojarjur]

First off, thanks for putting this together, @hazbo. Also, thanks @jishi9 for the input.

My thoughts on the questions brought up so far:

1. Having a getGitConfig method might be valuable, but there are also arguments against it, as every time we call "git config" we want to handle the absence of a config value differently (throwing an error, falling back to an environment variable, or falling back to a default). As is, I don't feel strongly about it.
2. "GetSubmitStrategy" is a better name than "GetMergeStrategy", but I wouldn't block this review based on it.

3. If an error is returned from "git config", then ignoring the actual contents of the error seems acceptable in this case.

   This is because, in this context, none of the error codes should be possible except for 1 (unknown section) and 3 (invalid file). The 1 status code is basically what we expect, so ignoring it is what we would do anyway. The 3 status code means that the user's setup is broken in some way. Ideally we would report that to them, but it's a major type of breakage so hopefully the user will be made aware of that via other means. Further, it isn't something that we can do cleanly. These two things add up to make me think that the additional complexity (and, thus, brittleness) that would add to the code are not worth the additional user benefit that we would get from checking the error.

   As such, my advice is to just check if there is an error of any sort, and if so fall back to the default.

BTW, the default isn't "merge", it's fast-forward. Before closing #36, we'll also need to add a "submitFastForward" flag to be able to selectively choose that default even if a different strategy has been set in the config, but that just blocks the feature request, not this pull request.

Also, I've added an inline comment; take a look using git appraise show.

[googlebot]

CLAs look good, thanks!

[hazbo]

@ojarjur thanks for the feedback on this. As you can see I have made some amendments in my latest commit. However I was unable to find your inline comment using git-appraise this time. I did a pull and then a list, but didn't see the review there.

[jishi9]

@hazbo It's hidden in git appraise show 708d7c2846d8, took me a while to find!

Look for a35b23b2737594d793d42b7ede04b8f068145384 in the output, where it says "This should only be done if the user has not set one of the command line flags"

[hazbo]

@jishi9 thanks a lot! :)

[ojarjur]

@hazbo @jishi9 OK, I see what happened with the comment disappearing; it was a bug in the script to mirror pull requests into git-appraise.

Apparently when the base moved forward, that tool thought it should create a new review, instead of just detecting the existing review.

I've turned off the script for now, will fix the bug, and then turn it back on.

Sorry for the confusion.