google / git-appraise

Distributed code review system for Git repos
Apache License 2.0
5.13k stars 146 forks source link

Accepting -F flag on git-appraise comments #57

Closed hazbo closed 8 years ago

hazbo commented 8 years ago

I've gathered a few thoughts in regards to #56 in the form of this commit. This currently doesn't take in to account stdin (-), just a file name.

Basically, the user has 3 choices as this point:

Given that 'message' is the file name that contains the message. As for the stdin, keeping that similar to git would be a good idea. They present the user with the following output and then read:

if (isatty(0))
    fprintf(stderr, _("(reading log message from standard input)\n"));
read_from_stdin(&log);

This would be consistent and imo seems like it could work well. I agree with @ojarjur in that this should be supported with accept, comment, reject and request. This commit only takes into account, comment. Once that's nailed down, applying it to the rest would be trivial.

ojarjur commented 8 years ago

Thanks for putting this together. I like where this is headed so far.

I also like the proposal to be consistent with git's stderr message before trying to read from stdin. I think that will make things a lot more clear to the end user.

Did you want to get this submitted with just the current scope, or did you want to add the other functionality too? I'm happy with either approach, so just let me know.

Thanks.

hazbo commented 8 years ago

After receiving feedback on this first commit I was planning on working on the rest and then just have it all merged as one. But I think it might be better if we submit this one now, figure out the stdin related stuff in a separate request, then once we're happy with that, it'd just be a case of applying this work to the other subcommands.

How does this sound to you?

ojarjur commented 8 years ago

That sounds good to me. The only caveat for that approach is that we'll need to remove the bit in the help text about using - for reading from stdin.

hazbo commented 8 years ago

Feel free to assign #56 to me if you'd like. I'm more than happy to hack away at this tonight, and probably the rest tomorrow :)

hazbo commented 8 years ago

I branched off from hazbo/unix-filter to create #58