From: karthik nayak <karthik.188@gmail.com>
Date: Fri, 06 Mar 2015 10:53:05 GMT
Subject: Re: [PATCH v3] git: make "git -C '' <cmd>" not to barf
Message-ID: <54F98711.6010401@gmail.com>
In-Reply-To: <CAPig+cSeTA=SfQDouP-WCaSjsMBkUg=9Qm8xxfp0jcq=+GSksg@mail.gmail.com>


On 03/06/2015 02:23 PM, Eric Sunshine wrote:
> On Fri, Mar 6, 2015 at 2:05 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
> > It now acts like "cd ''" and does not barf and treats
> > it as a no-op.
>
> What does "barf" mean in this context? Does the program crash? Spit
> out nonsensical messages? Misbehave in some fashion? A good commit
> message should explain the problem with sufficient detail so readers
> don't need to guess what the "bad" behavior is.
>
> > This is useful if a caller function
> > does not want to change directory and hence gives no
> > path value, which would have generally caused git to
> > output an undesired error message.
>
> This is an odd justification. A caller not wanting to change the
> directory wouldn't pass -C in the first place. A better justification
> might be that die()ing is unnecessarily harsh behavior for what
> otherwise could be considered a no-op, citing "cd ''" as an example.
>
> Also, write in imperative mood, as if you're instructing the code to
> change itself.
>
> Taking the above observations into consideration, you might say:
>
>      git: treat `-C <path>' as a no-op when <path> is empty
>
>      `git -C ""' unhelpfully dies with error "Cannot change to ''",
>      whereas the shell treats `cd ""' as a no-op. Taking the shell's
>      behavior as a precedent, teach git to treat `-C ""' as a no-op, as
>      well.
>
Hey Eric,
I see what you mean.
> > Included a simple test to check the same, as suggested
> > by Junio.
>
> It is a bit weak to say that Junio "suggested" the test, considering
> that he actually wrote it[1].
> > Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> > ---
> > diff --git a/git.c b/git.c
> > index 8c7ee9c..d734afa 100644
> > --- a/git.c
> > +++ b/git.c
> > @@ -204,10 +204,14 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
> >                                  fprintf(stderr, "No directory given for -C.\n" );
> >                                  usage(git_usage_string);
> >                          }
> > -                       if (chdir((*argv)[1]))
> > -                               die_errno("Cannot change to '%s'", (*argv)[1]);
> > -                       if (envchanged)
> > -                               *envchanged = 1;
> > +                       if (*((*argv)[1]) == 0)
>
> Saying '\0' rather than 0 would make the intent clearer.
>
> > +                               ; /* DO not change directory if no directory is given*/
> > +                       else {
> > +                               if (chdir((*argv)[1]))
> > +                                       die_errno("Cannot change to '%s'", (*argv)[1]);
> > +                               if (envchanged)
> > +                                       *envchanged = 1;
> > +                       }
>
> The 'if/else' statement you've composed (with an empty 'if' branch) is
> unnecessarily complicated when a simple 'if' suffices:
Yes, will change this.
>
>      if (*(*argv)[1]) {
>          if (chdir((*argv)[1]))
>              die_errno("Cannot change to '%s'", (*argv)[1]);
>          if (envchanged)
>              *envchanged = 1;
>      }
>
> >                          (*argv)++;
> >                          (*argc)--;
> >                  } else {
> > diff --git a/t/t0056-git-C.sh b/t/t0056-git-C.sh
> > index 99c0377..a6b52f1 100755
> > --- a/t/t0056-git-C.sh
> > +++ b/t/t0056-git-C.sh
> > @@ -14,6 +14,14 @@ test_expect_success '"git -C <path>" runs git from the directory <path>' '
> >          test_cmp expected actual
> >   '
> >
> > +test_expect_success '"git -C <path>" with an empty <path> is a no-op' '
> > +       mkdir -p dir1/subdir &&
> > +       cd dir1/subdir &&
>
> When Junio composed this test[1], he intentionally wrapped it in a
> subshell via '(' and ')'. The problem with dropping the subshell, as
> you did here, is that the 'cd' in this test will still be in effect
> when tests following this one are run, which typically will break
> them. Wrapping the test in a subshell side-steps the problem because
> the parent shell is not affected by 'cd' within the subshell. To
> summarize: Don't remove the subshell from Junio's example.
>
> (You lucked out in this case, by accident, since the following tests
> are not impacted by such ill-behavior.)
I did not know that. I got lucky, definitely.
>
> > +       git -C "" rev-parse --show-prefix >actual &&
> > +       echo subdir/ >expect
>
> Broken &&-chain.
>
> > +       test_cmp expect actual
> > +'
> > +
> >   test_expect_success 'Multiple -C options: "-C dir1 -C dir2" is equivalent to "-C dir1/dir2"' '
> >          test_create_repo dir1/dir2 &&
> >          echo 1 >dir1/dir2/b.txt &&
> > --
> > 2.3.1.167.g7f4ba4b.dirty
>
> [1]: http://article.gmane.org/gmane.comp.version-control.git/264871
>
Thanks for the code review, will be back with the next patch :D