2016-03-18 13:00 GMT+08:00 Jeff King [off-list ref]:

On Fri, Mar 18, 2016 at 12:34:04PM +0800, 惠轶群 wrote:

quoted

quoted

quoted

+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.

quoted

quoted

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.

quoted

quoted

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.

quoted

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.

-Peff