On 03/06/2015 02:23 PM, Eric Sunshine wrote:
On Fri, Mar 6, 2015 at 2:05 AM, Karthik Nayak <karthik.188@xxxxxxxxx> 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@xxxxxxxxx>
> ---
> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html