Re: [PATCH] difftool: fix bug when printing usage

[Johannes Schindelin]

Hi Junio,

On Tue, 7 Feb 2017, Junio C Hamano wrote:

> Junio C Hamano  writes:
> 
> > Johannes Schindelin  writes:
> >
> >>> > Likewise, this would become
> >>> >
> >>> > GIT_CEILING_DIRECTORIES="$PWD/not" \
> >>> > test_expect_code 129 git -C not/repo difftool -h >output &&
> >>> > grep ^usage: output
> >>> 
> >>> I agree with the intent, but the execution here is "Not quite".
> >>> test_expect_code being a shell function, it does not take the
> >>> "one-shot environment assignment for this single invocation," like
> >>> external commands do.
> >>
> >> So now that we know what is wrong, can you please enlighten me about
> >> what is right?
> >
> > David's original is just fine, isn't it?
> 
> I've also seen people use "env VAR=VAL git command" as the command to be
> tested in t/ scripts.  You can run that under test_expect_code,
> methinks.

That is exactly what David ended up sending out as follow-up patches.

I did not mean to be critical, I just found it to be more helpful to
accompany "that does not work" comments with "but this does" in the past.

Ciao,
Johannes

Re: [PATCH] difftool: fix bug when printing usage

[Junio C Hamano]

Junio C Hamano  writes:

> Johannes Schindelin  writes:
>
>>> > Likewise, this would become
>>> >
>>> >   GIT_CEILING_DIRECTORIES="$PWD/not" \
>>> >   test_expect_code 129 git -C not/repo difftool -h >output &&
>>> >   grep ^usage: output
>>> 
>>> I agree with the intent, but the execution here is "Not quite".
>>> test_expect_code being a shell function, it does not take the
>>> "one-shot environment assignment for this single invocation," like
>>> external commands do.
>>
>> So now that we know what is wrong, can you please enlighten me about what
>> is right?
>
> David's original is just fine, isn't it?

I've also seen people use "env VAR=VAL git command" as the command
to be tested in t/ scripts.  You can run that under test_expect_code,
methinks.

Re: [PATCH] difftool: fix bug when printing usage

[Junio C Hamano]

Johannes Schindelin  writes:

>> > Likewise, this would become
>> >
>> >GIT_CEILING_DIRECTORIES="$PWD/not" \
>> >test_expect_code 129 git -C not/repo difftool -h >output &&
>> >grep ^usage: output
>> 
>> I agree with the intent, but the execution here is "Not quite".
>> test_expect_code being a shell function, it does not take the
>> "one-shot environment assignment for this single invocation," like
>> external commands do.
>
> So now that we know what is wrong, can you please enlighten me about what
> is right?

David's original is just fine, isn't it?

Re: [PATCH] difftool: fix bug when printing usage

[Johannes Schindelin]

Hi Junio,

On Mon, 6 Feb 2017, Junio C Hamano wrote:

> Johannes Schindelin  writes:
> 
> > [... and he quoted someone ...]
> >
> >> +  # create a ceiling directory to prevent Git from finding a repo
> >> +  mkdir -p not/repo &&
> >> +  ceiling="$PWD/not" &&
> >> +  lines=$(cd not/repo &&
> >> +  GIT_CEILING_DIRECTORIES="$ceiling" git difftool -h |
> >> +  grep ^usage: | wc -l) &&
> >> +  test "$lines" -eq 1 &&
> >
> > Likewise, this would become
> >
> > GIT_CEILING_DIRECTORIES="$PWD/not" \
> > test_expect_code 129 git -C not/repo difftool -h >output &&
> > grep ^usage: output
> 
> I agree with the intent, but the execution here is "Not quite".
> test_expect_code being a shell function, it does not take the
> "one-shot environment assignment for this single invocation," like
> external commands do.

So now that we know what is wrong, can you please enlighten me about what
is right?

Thanks,
Johannes

Re: [PATCH] difftool: fix bug when printing usage

[Junio C Hamano]

Johannes Schindelin  writes:

>> +test_expect_success 'basic usage requires no repo' '
>> +lines=$(git difftool -h | grep ^usage: | wc -l) &&
>> +test "$lines" -eq 1 &&
>
> It may be easier to debug future breakages if you wrote
>
>   git difftool -h | grep ^usage: >output &&
>   test_line_count = 1 output &&
> or even better (changing the semantics now):
>
>   test_expect_code 129 git difftool -h >output &&
>   grep ^usage: output &&

True.

>> +# create a ceiling directory to prevent Git from finding a repo
>> +mkdir -p not/repo &&
>> +ceiling="$PWD/not" &&
>> +lines=$(cd not/repo &&
>> +GIT_CEILING_DIRECTORIES="$ceiling" git difftool -h |
>> +grep ^usage: | wc -l) &&
>> +test "$lines" -eq 1 &&
>
> Likewise, this would become
>
>   GIT_CEILING_DIRECTORIES="$PWD/not" \
>   test_expect_code 129 git -C not/repo difftool -h >output &&
>   grep ^usage: output

I agree with the intent, but the execution here is "Not quite".
test_expect_code being a shell function, it does not take the
"one-shot environment assignment for this single invocation," like
external commands do.

> More importantly: When I read $PWD all kinds of alarm bells go off in my
> head, as I immediately think of all the issues we have on Windows due to
> Git's regression test using POSIX paths all over the place.

And we appreciate that somebody who is more familiar with the issue
is watching ;-).

> Insofar as I am the author of the builtin difftool:
>
> Acked-by: Johannes Schindelin 

OK, thanks.

Re: [PATCH] difftool: fix bug when printing usage

[Johannes Schindelin]

Hi David,

On Sun, 5 Feb 2017, David Aguilar wrote:

> "git difftool -h" reports an error:
> 
>   fatal: BUG: setup_git_env called without repository
> 
> Defer repository setup so that the help option processing happens before
> the repository is initialized.
> 
> Add tests to ensure that the basic usage works inside and outside of a
> repository.
> 
> Signed-off-by: David Aguilar 
> ---
> This bug exists in both "master" and "next".
> This patch has been tested on both branches.

Thanks for noticing and for patching!

> diff --git a/builtin/difftool.c b/builtin/difftool.c
> index b5e85ab079..d13350ce83 100644
> --- a/builtin/difftool.c
> +++ b/builtin/difftool.c
> @@ -647,10 +647,6 @@ int cmd_difftool(int argc, const char **argv, const char 
> *prefix)
>   OPT_END()
>   };
>  
> - /* NEEDSWORK: once we no longer spawn anything, remove this */
> - setenv(GIT_DIR_ENVIRONMENT, absolute_path(get_git_dir()), 1);
> - setenv(GIT_WORK_TREE_ENVIRONMENT, absolute_path(get_git_work_tree()), 
> 1);
> -
>   git_config(difftool_config, NULL);
>   symlinks = has_symlinks;
>  
> @@ -661,6 +657,10 @@ int cmd_difftool(int argc, const char **argv, const char 
> *prefix)
>   if (tool_help)
>   return print_tool_help();
>  
> + /* NEEDSWORK: once we no longer spawn anything, remove this */
> + setenv(GIT_DIR_ENVIRONMENT, absolute_path(get_git_dir()), 1);
> + setenv(GIT_WORK_TREE_ENVIRONMENT, absolute_path(get_git_work_tree()), 
> 1);
> +
>   if (use_gui_tool && diff_gui_tool && *diff_gui_tool)
>   setenv("GIT_DIFF_TOOL", diff_gui_tool, 1);
>   else if (difftool_cmd) {

Aaargh. You know, when you are used to code review systems where you can
easily increase the context of the diff hunks, static patches are...
tedious to review.

For the record: the context between those two hunks is:

   argc = parse_options(argc, argv, prefix, builtin_difftool_options,
 builtin_difftool_usage, PARSE_OPT_KEEP_UNKNOWN |
 PARSE_OPT_KEEP_DASHDASH);

> diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
> index aa0ef02597..21e2ac4ad6 100755
> --- a/t/t7800-difftool.sh
> +++ b/t/t7800-difftool.sh
> @@ -23,6 +23,19 @@ prompt_given ()
>   test "$prompt" = "Launch 'test-tool' [Y/n]? branch"
>  }
>  
> +test_expect_success 'basic usage requires no repo' '
> + lines=$(git difftool -h | grep ^usage: | wc -l) &&
> + test "$lines" -eq 1 &&

It may be easier to debug future breakages if you wrote

git difftool -h | grep ^usage: >output &&
test_line_count = 1 output &&

or even better (changing the semantics now):

test_expect_code 129 git difftool -h >output &&
grep ^usage: output &&

> + # create a ceiling directory to prevent Git from finding a repo
> + mkdir -p not/repo &&
> + ceiling="$PWD/not" &&
> + lines=$(cd not/repo &&
> + GIT_CEILING_DIRECTORIES="$ceiling" git difftool -h |
> + grep ^usage: | wc -l) &&
> + test "$lines" -eq 1 &&

Likewise, this would become

GIT_CEILING_DIRECTORIES="$PWD/not" \
test_expect_code 129 git -C not/repo difftool -h >output &&
grep ^usage: output

> + rmdir -p not/repo

I am not sure how portable `rmdir -p` is. Why not use

test_when_finished rm -r not

instead?

But those are all really minor bike-sheddings, and I promised myself that
I would avoid those (as I find them rather pointless) unless there is at
least one real benefit. In this case, I think the result becomes more
readable:

-- snip --
diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index 21e2ac4ad6d..7d7cb63d61e 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -24,16 +24,14 @@ prompt_given ()
 }
 
 test_expect_success 'basic usage requires no repo' '
-   lines=$(git difftool -h | grep ^usage: | wc -l) &&
-   test "$lines" -eq 1 &&
-   # create a ceiling directory to prevent Git from finding a repo
-   mkdir -p not/repo &&
-   ceiling="$PWD/not" &&
-   lines=$(cd not/repo &&
-   GIT_CEILING_DIRECTORIES="$ceiling" git difftool -h |
-   grep ^usage: | wc -l) &&
-   test "$lines" -eq 1 &&
-   rmdir -p not/repo
+   test_expect_code 129 git difftool -h >output &&
+   grep ^usage: output &&
+   # create a ceiling directory to prevent Git from finding a repo
+   mkdir -p not/repo &&
+   test_when_finished rm -r not &&
+   GIT_CEILING_DIRECTORIES="$PWD/not" \
+   test_expect_code 129 git -C not/repo difftool -h >output &&
+   grep ^usage: output
 '
 
 # Create a file on master and change it on branch
-- snap --

More importantly: When I read $PWD all kinds of alarm bells go off in my
head, as I immediately think of all the issues we have on Windows due to
Git's regression test using POSIX paths all over the place.

In this particular instance, it *does* work, magically, 

[PATCH] difftool: fix bug when printing usage

[David Aguilar]

"git difftool -h" reports an error:

fatal: BUG: setup_git_env called without repository

Defer repository setup so that the help option processing happens before
the repository is initialized.

Add tests to ensure that the basic usage works inside and outside of a
repository.

Signed-off-by: David Aguilar 
---
This bug exists in both "master" and "next".
This patch has been tested on both branches.

 builtin/difftool.c  |  8 
 t/t7800-difftool.sh | 13 +
 2 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/builtin/difftool.c b/builtin/difftool.c
index b5e85ab079..d13350ce83 100644
--- a/builtin/difftool.c
+++ b/builtin/difftool.c
@@ -647,10 +647,6 @@ int cmd_difftool(int argc, const char **argv, const char 
*prefix)
OPT_END()
};
 
-   /* NEEDSWORK: once we no longer spawn anything, remove this */
-   setenv(GIT_DIR_ENVIRONMENT, absolute_path(get_git_dir()), 1);
-   setenv(GIT_WORK_TREE_ENVIRONMENT, absolute_path(get_git_work_tree()), 
1);
-
git_config(difftool_config, NULL);
symlinks = has_symlinks;
 
@@ -661,6 +657,10 @@ int cmd_difftool(int argc, const char **argv, const char 
*prefix)
if (tool_help)
return print_tool_help();
 
+   /* NEEDSWORK: once we no longer spawn anything, remove this */
+   setenv(GIT_DIR_ENVIRONMENT, absolute_path(get_git_dir()), 1);
+   setenv(GIT_WORK_TREE_ENVIRONMENT, absolute_path(get_git_work_tree()), 
1);
+
if (use_gui_tool && diff_gui_tool && *diff_gui_tool)
setenv("GIT_DIFF_TOOL", diff_gui_tool, 1);
else if (difftool_cmd) {
diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index aa0ef02597..21e2ac4ad6 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -23,6 +23,19 @@ prompt_given ()
test "$prompt" = "Launch 'test-tool' [Y/n]? branch"
 }
 
+test_expect_success 'basic usage requires no repo' '
+   lines=$(git difftool -h | grep ^usage: | wc -l) &&
+   test "$lines" -eq 1 &&
+   # create a ceiling directory to prevent Git from finding a repo
+   mkdir -p not/repo &&
+   ceiling="$PWD/not" &&
+   lines=$(cd not/repo &&
+   GIT_CEILING_DIRECTORIES="$ceiling" git difftool -h |
+   grep ^usage: | wc -l) &&
+   test "$lines" -eq 1 &&
+   rmdir -p not/repo
+'
+
 # Create a file on master and change it on branch
 test_expect_success 'setup' '
echo master >file &&
-- 
2.12.0.rc0.209.g3d1e53e462