Re: [PATCH/RFC/GSoC 3/3] t0301: test credential-cache support of XDG_RUNTIME_DIR

[谭俊浩]

On 17/03/2016 01:24, Junio C Hamano wrote:

惠轶群  writes:


Is it because the wish is to always use /tmp/git-$uid/ as a fallback
for $XDG_RUNTIME_DIR (as opposed to ~/.git-credential-cache/, which
is specific to the credential-cache and would look strange if we
used it for other "runtime" things)?


Yes, I mean to use it as a general fallback for git.


If that is the case the code probably needs to be a bit more careful
before deciding to use /tmp/git-$uid/ directory (is it really $uid's?
can anybody else write to it to race with the real user? etc.)

On the other hand, I think, falling back to 
$HOME/.git-credential-cache/socket

doesn't make any sense for back-compability cannot be ensured.


What do you mean by that?

Using ~/.git-credential-cache/credential-cache.sock would not help
at all for existing users, but ~/.git-credential-cache/socket would
interoperate well with users with existing versions of Git, no?


Just being curious, and wanting to see the reasoning behind the
design decision the patch series makes in the log message of one of
these patches.


I guess it is better to use /tmp or such instead of $HOME/.* so that
the users home directory won't be flooded by sockets.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/RFC/GSoC 3/3] t0301: test credential-cache support of XDG_RUNTIME_DIR

[惠轶群]

2016-03-17 1:24 GMT+08:00 Junio C Hamano :
> 惠轶群  writes:
>
>>> Is it because the wish is to always use /tmp/git-$uid/ as a fallback
>>> for $XDG_RUNTIME_DIR (as opposed to ~/.git-credential-cache/, which
>>> is specific to the credential-cache and would look strange if we
>>> used it for other "runtime" things)?
>>
>> Yes, I mean to use it as a general fallback for git.
>
> If that is the case the code probably needs to be a bit more careful
> before deciding to use /tmp/git-$uid/ directory (is it really $uid's?
> can anybody else write to it to race with the real user? etc.)
>
>> On the other hand, I think, falling back to 
>> $HOME/.git-credential-cache/socket
>> doesn't make any sense for back-compability cannot be ensured.
>
> What do you mean by that?
>
> Using ~/.git-credential-cache/credential-cache.sock would not help
> at all for existing users, but ~/.git-credential-cache/socket would
> interoperate well with users with existing versions of Git, no?
>

I meant that FALLBACK instead of DEFAULT is useless for back-compatibility.

Most users with existing versions of Git will be broken even if there is a
fallback.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/RFC/GSoC 3/3] t0301: test credential-cache support of XDG_RUNTIME_DIR

[惠轶群]

2016-03-17 1:15 GMT+08:00 Jeff King :
> On Wed, Mar 16, 2016 at 06:07:45PM +0800, Hui Yiqun wrote:
>
>> diff --git a/t/t0301-credential-cache.sh b/t/t0301-credential-cache.sh
>> index 82c8411..0718bb0 100755
>> --- a/t/t0301-credential-cache.sh
>> +++ b/t/t0301-credential-cache.sh
>> @@ -12,7 +12,32 @@ test -z "$NO_UNIX_SOCKETS" || {
>>  # don't leave a stale daemon running
>>  trap 'code=$?; git credential-cache exit; (exit $code); die' EXIT
>>
>> +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.

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

>> +test_expect_success 'force git-credential-cache to exit so that socket 
>> disappear' '
>> + git credential-cache exit &&
>> + test_path_is_missing "$XDG_RUNTIME_DIR/git/credential-cache.sock" &&
>> + unset XDG_RUNTIME_DIR
>> +'
>
> 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?

What is not designed? cleanup being done, my tests passing or the
synchronization?

>
> -Pef
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/RFC/GSoC 3/3] t0301: test credential-cache support of XDG_RUNTIME_DIR

[Jeff King]

On Thu, Mar 17, 2016 at 12:40:59AM +0800, 惠轶群 wrote:

> > Is it better to have the fallback in /tmp, and not in
> > ~/.git-credential-cache/, and why?
> >
> > Is it because the wish is to always use /tmp/git-$uid/ as a fallback
> > for $XDG_RUNTIME_DIR (as opposed to ~/.git-credential-cache/, which
> > is specific to the credential-cache and would look strange if we
> > used it for other "runtime" things)?
> 
> Yes, I mean to use it as a general fallback for git.
> 
> xdg base dir spec does not specify where to fallback when
> $XDG_RUNTIME_DIR is not defined. It just says:
> 
> If $XDG_RUNTIME_DIR is not set applications should fall back to
> a replacement directory with similar capabilities and print a warning
> message. Applications should use this directory for communication
> and synchronization purposes and should not place larger files in it,
> since it might reside in runtime memory and cannot necessarily be
> swapped out to disk.
> 
> tmpfs is just like what it describes. And many other applications
> put socket under which, such as tmux.
> 
> On the other hand, I think, falling back to $HOME/.git-credential-cache/socket
> doesn't make any sense for back-compability cannot be ensured.

If we are going to use a publicly accessible directory like /tmp, I
think we need to start worrying about tmp-races with malicious users.

Right now we make sure that an existing socket directory is mode 0700.
That's just a courtesy check that the user didn't create it themselves
with a permissive mode. But we don't check the owner of the directory,
and our check is racy with accessing the directory.

So if we blindly use an existing /tmp/git-$uid, I think an attacker can
race with:

dir=/tmp/git-$victimuid
mkdir $dir
while true; do
chmod 0700 $dir
chmod 0777 $dir
done

If the victim does their mode check while the 0700 is in effect, but
then creates the socket during the 0777 moment, they won't notice
anything amiss. And the attacker will have access to their credential
socket.

This is a classic /tmp race.  I imagine it's less of an issue in this
day and age when people mostly have their own machines and their own
/tmp, but we still should not recreate the mistakes of the past.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/RFC/GSoC 3/3] t0301: test credential-cache support of XDG_RUNTIME_DIR

[惠轶群]

2016-03-17 0:17 GMT+08:00 Junio C Hamano :
> Hui Yiqun  writes:
>
>> t0301 now tests git-credential-cache support for XDG user-specific
>> runtime file $XDG_RUNTIME_DIR/git/credential.sock. Specifically:
>>
>> * if $XDG_RUNTIME_DIR exists, use socket at
>>   `$XDG_RUNTIME_DIR/git/credential-cache.sock`.
>>
>> * otherwise, `/tmp/git-$uid/credential-cache.sock` is taken.
>
> Is it better to have the fallback in /tmp, and not in
> ~/.git-credential-cache/, and why?
>
> Is it because the wish is to always use /tmp/git-$uid/ as a fallback
> for $XDG_RUNTIME_DIR (as opposed to ~/.git-credential-cache/, which
> is specific to the credential-cache and would look strange if we
> used it for other "runtime" things)?

Yes, I mean to use it as a general fallback for git.

xdg base dir spec does not specify where to fallback when
$XDG_RUNTIME_DIR is not defined. It just says:

If $XDG_RUNTIME_DIR is not set applications should fall back to
a replacement directory with similar capabilities and print a warning
message. Applications should use this directory for communication
and synchronization purposes and should not place larger files in it,
since it might reside in runtime memory and cannot necessarily be
swapped out to disk.

tmpfs is just like what it describes. And many other applications
put socket under which, such as tmux.

On the other hand, I think, falling back to $HOME/.git-credential-cache/socket
doesn't make any sense for back-compability cannot be ensured.

>
> Just being curious, and wanting to see the reasoning behind the
> design decision the patch series makes in the log message of one of
> these patches.
>
> Thanks.
>
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/RFC/GSoC 3/3] t0301: test credential-cache support of XDG_RUNTIME_DIR

[惠轶群]

2016-03-18 13:00 GMT+08:00 Jeff King :
> 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.

>
> -Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/RFC/GSoC 3/3] t0301: test credential-cache support of XDG_RUNTIME_DIR

[Junio C Hamano]

惠轶群  writes:

>> Is it because the wish is to always use /tmp/git-$uid/ as a fallback
>> for $XDG_RUNTIME_DIR (as opposed to ~/.git-credential-cache/, which
>> is specific to the credential-cache and would look strange if we
>> used it for other "runtime" things)?
>
> Yes, I mean to use it as a general fallback for git.

If that is the case the code probably needs to be a bit more careful
before deciding to use /tmp/git-$uid/ directory (is it really $uid's?
can anybody else write to it to race with the real user? etc.)

> On the other hand, I think, falling back to $HOME/.git-credential-cache/socket
> doesn't make any sense for back-compability cannot be ensured.

What do you mean by that?

Using ~/.git-credential-cache/credential-cache.sock would not help
at all for existing users, but ~/.git-credential-cache/socket would
interoperate well with users with existing versions of Git, no?

>> Just being curious, and wanting to see the reasoning behind the
>> design decision the patch series makes in the log message of one of
>> these patches.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/RFC/GSoC 3/3] t0301: test credential-cache support of XDG_RUNTIME_DIR

[Junio C Hamano]

Hui Yiqun  writes:

> t0301 now tests git-credential-cache support for XDG user-specific
> runtime file $XDG_RUNTIME_DIR/git/credential.sock. Specifically:
>
> * if $XDG_RUNTIME_DIR exists, use socket at
>   `$XDG_RUNTIME_DIR/git/credential-cache.sock`.
>
> * otherwise, `/tmp/git-$uid/credential-cache.sock` is taken.

Is it better to have the fallback in /tmp, and not in
~/.git-credential-cache/, and why?

Is it because the wish is to always use /tmp/git-$uid/ as a fallback
for $XDG_RUNTIME_DIR (as opposed to ~/.git-credential-cache/, which
is specific to the credential-cache and would look strange if we
used it for other "runtime" things)?

Just being curious, and wanting to see the reasoning behind the
design decision the patch series makes in the log message of one of
these patches.

Thanks.

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/RFC/GSoC 3/3] t0301: test credential-cache support of XDG_RUNTIME_DIR

[惠轶群]

2016-03-17 16:12 GMT+08:00 Junio C Hamano :
> 谭俊浩  writes:
>
>> On 17/03/2016 01:24, Junio C Hamano wrote:
>>
>>> Using ~/.git-credential-cache/credential-cache.sock would not help
>>> at all for existing users, but ~/.git-credential-cache/socket would
>>> interoperate well with users with existing versions of Git, no?
>>>
> Just being curious, and wanting to see the reasoning behind the
> design decision the patch series makes in the log message of one of
> these patches.
>>
>> I guess it is better to use /tmp or such instead of $HOME/.* so that
>> the users home directory won't be flooded by sockets.
>
> The "fallback" being discussed is to see if $XDG can be used (and
> use it if so), otherwise see if ~/.git-credential-cache/socket can
> be used (and use it if so), otherwise die with a message (see
> credential-cache.c).  The order of the falling back may want to be
> the other way around, but in either case, the definition of "can be
> used" includes "is there already a directory in which we can create
> a socket?".
>
> The existing versions have used ~/.git-credential-cache/socket as
> the default socket path, so it is reasonable to expect that users
> that are already using the feature already have the directory there.
>
> So I do not think there is any "flooded" involved; if the directory
> is already there, we can use it to create and use a single socket.
> It's not like we'd be creating many random new directories in ~/.

As mentioned above, The purpose I try "$XDG_RUNTIME_DIR" and then
"/tmp/git-$uid" is that I'd like to build an identical mechanism for searching
for path to store runtime files such as socket and so on.

I guess you would not second an additional configuration to let git
store runtime
files under XDG-compatible path.

Then is there any better solution to keep compatibility than checking the
existence of ~/.git-credential-cache? I'm not sure.

I think we could left following message in documentation:

From version xxx, we put the socket under a XDG-compatible path instead of
a directory under $HOME. If that incompatibility disturbs you,
please consider
adding `--socket $HOME/.git-credential-cache/socket` to your configuration.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/RFC/GSoC 3/3] t0301: test credential-cache support of XDG_RUNTIME_DIR

[惠轶群]

2016-03-18 13:11 GMT+08:00 惠轶群 :
> 2016-03-18 13:00 GMT+08:00 Jeff King :
>> 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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/RFC/GSoC 3/3] t0301: test credential-cache support of XDG_RUNTIME_DIR

[Jeff King]

On Wed, Mar 16, 2016 at 06:07:45PM +0800, Hui Yiqun wrote:

> diff --git a/t/t0301-credential-cache.sh b/t/t0301-credential-cache.sh
> index 82c8411..0718bb0 100755
> --- a/t/t0301-credential-cache.sh
> +++ b/t/t0301-credential-cache.sh
> @@ -12,7 +12,32 @@ test -z "$NO_UNIX_SOCKETS" || {
>  # don't leave a stale daemon running
>  trap 'code=$?; git credential-cache exit; (exit $code); die' EXIT
>  
> +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?

> +
> +helper_test cache
> +

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)?

> +test_expect_success 'force git-credential-cache to exit so that socket 
> disappear' '
> + git credential-cache exit &&
> + test_path_is_missing "$XDG_RUNTIME_DIR/git/credential-cache.sock" &&
> + unset XDG_RUNTIME_DIR
> +'

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

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/RFC/GSoC 3/3] t0301: test credential-cache support of XDG_RUNTIME_DIR

[Jeff King]

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.

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.

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.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/RFC/GSoC 3/3] t0301: test credential-cache support of XDG_RUNTIME_DIR

[Junio C Hamano]

谭俊浩  writes:

> On 17/03/2016 01:24, Junio C Hamano wrote:
>
>> Using ~/.git-credential-cache/credential-cache.sock would not help
>> at all for existing users, but ~/.git-credential-cache/socket would
>> interoperate well with users with existing versions of Git, no?
>>
 Just being curious, and wanting to see the reasoning behind the
 design decision the patch series makes in the log message of one of
 these patches.
>
> I guess it is better to use /tmp or such instead of $HOME/.* so that
> the users home directory won't be flooded by sockets.

The "fallback" being discussed is to see if $XDG can be used (and
use it if so), otherwise see if ~/.git-credential-cache/socket can
be used (and use it if so), otherwise die with a message (see
credential-cache.c).  The order of the falling back may want to be
the other way around, but in either case, the definition of "can be
used" includes "is there already a directory in which we can create
a socket?".

The existing versions have used ~/.git-credential-cache/socket as
the default socket path, so it is reasonable to expect that users
that are already using the feature already have the directory there.

So I do not think there is any "flooded" involved; if the directory
is already there, we can use it to create and use a single socket.
It's not like we'd be creating many random new directories in ~/.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html