Thank you all for your thorough reviews and constructive comments. Many of you comment on the same flaws in my patch; I’ll address all of them in the event of a second version of this patch, if you all this this feature is worth it. Issue 1) Naming I’ll rename the is_set variables according to Abhishek’s proposal (is_inline, is_shallow, etc.) Issue 2) Premature return Yes, I chose the easiest way out, since this was a RFC :) One possibility could be to AND or OR the —is-* options together, but I think going with Junio’s proposal is the most sane thing to do: On 13 Mar 2020, at 19:18, Junio C Hamano <gitster@xxxxxxxxx> wrote: > - when --quiet is in use, make --is-* mutually exclusive and die > when more than one of them is given. I think any of the --is-*, > when used with --quiet, should also be an error if there are revs > on the command line (e.g. "git rev-parse --is-inside-git-dir > HEAD" is OK, but not with "--quiet"). Issue 3) Exit code I think going with Junio’s proposal (0 => true, 1 => false, 2 => error) is ok. I’ll have another go at it, if you all think it’s worth it. If so, I’ll also add docs and unit tests. Erlend > On 13 Mar 2020, at 19:18, Junio C Hamano <gitster@xxxxxxxxx> wrote: > > Jeff King <peff@xxxxxxxx> writes: > >> I'm not sure if returning a single is_set makes sense, though. It >> effectively becomes an OR, and you wouldn't know which flag triggered >> it. It would make more sense to me for the invocation above to simply be >> an error, reminding the caller that they need to handle it more >> carefully. >> >> We _could_ encode each value into the exit code (e.g., set bit 1 if the >> first condition was true, and so on). But checking that becomes as much >> hassle as reading stdout, so there's little value. > > None of the above excites me. I do not think it makes much sense to > combine -q with --is-* for the reasons you stated already (i.e. the > caller cannot tell between "failure" and "false") in the first > place, but if we must do this: > > - reserve 0 (true) and 1 (false) for successful exit and use 2 (or > above) for other failures; > > - when --quiet is in use, make --is-* mutually exclusive and die > when more than one of them is given. I think any of the --is-*, > when used with --quiet, should also be an error if there are revs > on the command line (e.g. "git rev-parse --is-inside-git-dir > HEAD" is OK, but not with "--quiet"). > > is the minimum that makes me feel that we have semi-reasonable > behaviour that can be explained to end users.
