Re: [PATCH v3] git: make "git -C '' <cmd>" not to barf
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.> 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].
Show 15 quoted lines
> 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.
Show 7 quoted lines
> + ; /* 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:
if (*(*argv)[1]) {
if (chdir((*argv)[1]))
die_errno("Cannot change to '%s'", (*argv)[1]);
if (envchanged)
*envchanged = 1;
}Show 14 quoted lines
> (*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.)
> + git -C "" rev-parse --show-prefix >actual &&
> + echo subdir/ >expect
Show 8 quoted lines
> + 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
Karthik Nayak
