On Fri, Mar 13, 2020 at 11:00:00PM +0530, Abhishek Kumar wrote:
> > @@ -874,24 +874,31 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
> > continue;
> > }
> > if (!strcmp(arg, "--is-inside-git-dir")) {
> > - printf("%s\n", is_inside_git_dir() ? "true"
> > - : "false");
> > + int is_set = is_inside_git_dir();
>
> Avoid declaration after statement. Move is_set to the beginning of
> cmd_rev_parse().
This one is OK, because we're starting a new block (and the printf there
is going away). And it's nicer to keep the variable local to this block
if we can.
> > + if (quiet)
> > + return is_set ? 0 : 1;
>
> Returning prematurely might be a bad idea. If there are more options after
> "--is-inside-git-dir", they will be not executed. You can maybe rewrite this as:
>
> ```
> if (!strcmp(arg, "--is-inside-git-dir")) {
> is_set = is_inside_git_dir();
> if (!quiet)
> printf("%s\n", is_set ? "true"
> : "false");
> continue;
> }
> ```
> And then return the value at the end of function, replacing '0' with !is_set.
Yeah, the inability of this new mode to handle multiple options is a
drawback. We could perhaps live with it as a restriction (it is up to
the caller to decide if they want to make multiple calls with --quiet,
or do it all as one and accept the hassle of parsing), but it's rather
unfortunate if:
git rev-parse --quiet --is-inside-git-dir --is-bare-repository
quietly ignores the third argument. That seems like a recipe for errors.
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.
-Peff
