Repositories

<hazbo>
hazbo wants to merge refs/pull/12/head into refs/heads/master
2015-12-17 22:10:35

Force default git editor when omitting -m

For review comments, the absense of the -m flag will now attempt to load the user's default git editor.

i.e. git appraise comment c0a643ff39dd

An initial draft as discussed in #8

I'm still not sure whether or not the file that is saved is in the most appropriate place or not. I like the idea of it being relative to the project although it could have gone in /tmp I suppose.

<hazbo>
hazbo commented on 2015-12-17 15:07:09 with status ℹ️

I initially thought about abstracting the exec stuff out a little bit as it could easily be used elsewhere in the codebase, to use an editor for writing a message, but it's really not that much code as of now I just left it in the comment file.

<ojarjur@google.com>
[email protected] commented on 2015-12-17 18:13:39 with status ℹ️
Location { commit: Some("09aecba64027d190f6211a75b2e05a6940013d86"), path: Some("repository/git.go"), range: Some(Range { start_line: Some(100) }) }

The command "git config core.editor" only works if the user has explicitly set that config value, and not if they are instead relying on an environment variable.

Instead, we should use "git var GIT_EDITOR", which is what the built-in git commands use.

<ojarjur@google.com>
[email protected] commented on 2015-12-17 18:49:35 with status 👍
Location { commit: Some("09aecba64027d190f6211a75b2e05a6940013d86"), path: None, range: None }

Thanks for doing this. This looks good, and I think you chose the right approach since it matches what the built in git commands do. Just one potential improvement over the way we pick the editor, but overal LGTM.

<ojarjur>
ojarjur commented on 2015-12-17 18:50:54 with status ℹ️

Thanks for doing this. Overall this LGTM, but I've left a (minor) comment using git-appraise, so please take a look and see what you think.

<hazbo>
hazbo commented on 2015-12-17 19:40:28 with status ℹ️

You're right, this is a better approach for getting the editor. Is this kind of approach worth thinking about in regards to adding a message to an initial request?

<hazbo>
hazbo commented on 2015-12-17 19:41:34 with status ℹ️

You're right, this is a better approach for getting the editor. Is this kind of code in general worth thinking about in regards to adding a message to an initial request?

<ojarjur>
ojarjur commented on 2015-12-17 20:49:20 with status ℹ️

Is this kind of code in general worth thinking about in regards to adding a message to an initial request?

I don't know. There is a reasonable default for request messages (the message from the corresponding commit), but it could be useful to have the editor pop up with that default pre-populated.

At the very least, I would not want to pop up the editor by default for the request command (i.e. there could be a flag to turn that on).

<hazbo>
hazbo commented on 2015-12-17 22:10:35 with status ℹ️

Yes that makes sense, given that the user would / could of most likely just used that editor to write the commit message in the first place. I think a flag would be nice but I wouldn't consider it massively important at this stage - the default message (commit) is reasonable I think.