2016-03-18 13:11 GMT+08:00 惠轶群 <huiyiqun@xxxxxxxxx>:
> 2016-03-18 13:00 GMT+08:00 Jeff King <peff@xxxxxxxx>:
>> On Fri, Mar 18, 2016 at 12:34:04PM +0800, 惠轶群 wrote:
>>
>>> >> +test_expect_success 'set $XDG_RUNTIME_DIR' '
>>> >> + XDG_RUNTIME_DIR=$HOME/xdg_runtime/
>>> >> +'
>>> >
>>> > Doesn't this need to export the variable so that credential-cache can
>>> > see it?
>>>
>>> I'm not sure, but it seems that a little clean up code added before
>>> send-email
>>> make the test fail. At that time, I run test without building. I've send
>>> PATCH v2
>>> which runs well on my computer. However, $XDG_RUNTIME_DIR is still not
>>> exported, but that just works.
>>>
>>> I will try to dig deeper into the bash script to see why.
>>
>> I suspect it is because you have $XDG_RUNTIME_DIR defined in your
>> environment, which causes the shell to automatically export it. I don't,
>> so an explicit "export" is required to for the variable to make it to
>> its children.
>
> Yes, that's the problem. the explicit "export" is new knowledge for me, thanks.
>
>> I think we should actually be unsetting it in test-lib.sh for all tests,
>> as we do for XDG_CONFIG_HOME. That makes sure the tests are running with
>> a known state.
>
> Well, I seems a good choice.
>
>> For the non-XDG_RUNTIME_DIR tests, does this mean we are creating the
>> socket in /tmp? I'm not entirely happy with that, as we usually try to
>> have the test suite avoid touching anything outside of its trash
>> directories.
>>
>>> > This runs the full suite of tests twice (once here, and once for the
>>> > original helper_test invocation you left below). Shouldn't we just do it
>>> > once (making sure that $XDG_RUNTIME_DIR is respected)?
>>>
>>> I'd like to test the behavior of git-credential-cache when $XDG_RUNTIME_DIR
>>> is unset.
>>>
>>> In `t/t0302-credential-store.sh`, helper_test is also run multiple times.
>>> That's why I do so.
>>
>> OK. My main concern was just that the tests would take too long, but the
>> slow one is the cache test at the end, which is not repeated. So I think
>> this is fine.
>>
>>> > I wondered if this might be racy. credential-cache tells the daemon
>>> > "exit", then waits for a response or EOF. The daemon sees "exit" and
>>> > calls exit(0) immediately. We clean up the socket in an atexit()
>>> > handler. So I think we are OK (the pipe will get closed when the process
>>> > exits, and the atexit handler must have run by then).
>>> >
>>> > But that definitely was not designed, and is just how it happens to
>>> > work. I'm not sure if it's worth commenting on that (here, or perhaps in
>>> > the daemon code).
>>>
>>> I'm still confused.
>>>
>>> What do you mean by "pipe"? should it be "socket" instead?
>>
>> Sorry, yes, I used "pipe" and "socket" interchangeably there.
>>
>>> What is not designed? cleanup being done, my tests passing or the
>>> synchronization?
>>
>> The synchronization. If the daemon were implemented as:
>>
>> if (!strcmp(action.buf, "exit")) {
>> /* acknowledge that we got command */
>> fclose(out);
>> exit(0);
>> }
>>
>> for example, then the client would exit at the same that the daemon is
>> cleaning up the socket, and we may or may not call test_path_is_missing
>> before the cleanup is done.
>>
>> I think it's OK to rely on that, but we may want to put a comment to
>> that effect in the daemon code so that it doesn't get changed.
>
> The current implementation is natural for me. But having additional comment
> is better.
I believe git-credential--daemon is a better place to comment on, but
I'm not sure
whether the comment should be included in this patch set. Above all, they are
not quite related.
>
>>
>> -Peff
--
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