[GSoC microproject] Add XDG support to the credential-store helper

[GSoC microproject] Add XDG support to the credential-store helper

[Luis Ressel]

I'm contributing this patch in preparation for a GSoC15 application with
the git project. In particular, I'm interested in the two bisect
improvements listed on the Ideas page. Does anyone have other
suggestions for potential improvements of git bisect that would be
suitable for such a GSoC project?

Oh, and should I add a testcase for the new functionality introduced by
this patch? And if yes, what exactly should I test?

Also, apologies if these mails arrive twice. I'm resending them because
Majordomo seems to have ignored my initial attempt 6 hours ago.

Regards,
Luis Ressel

[PATCH] Add XDG support to the credential-store helper

[Luis Ressel]

credential-store will use ${XDG_CONFIG_HOME}/credentials to store
credentials if this file already exists and no --file option is given.
Otherwise it'll fall back to ~/.git-credentials (status quo).

Signed-off-by: Luis Ressel <aranea@aixah.de>
---
 Documentation/git-credential-store.txt | 4 +++-
 credential-store.c                     | 2 ++
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-credential-store.txt
b/Documentation/git-credential-store.txt index bc97071..cab8f72 100644
--- a/Documentation/git-credential-store.txt
+++ b/Documentation/git-credential-store.txt
@@ -34,7 +34,9 @@ OPTIONS
 	Use `<path>` to store credentials. The file will have its
 	filesystem permissions set to prevent other users on the system
 	from reading it, but will not be encrypted or otherwise
-	protected. Defaults to `~/.git-credentials`.
+	protected. Defaults to `~/.git-credentials` or
+	`$XDG_CONFIG_HOME/git/credentials` if the latter exists
+	($XDG_CONFIG_HOME defaults to ~/.config).
 
 EXAMPLES
 --------
diff --git a/credential-store.c b/credential-store.c
index 925d3f4..9720b42 100644
--- a/credential-store.c
+++ b/credential-store.c
@@ -138,6 +138,8 @@ int main(int argc, char **argv)
 	op = argv[0];
 
 	if (!file)
+		home_config_paths(NULL, &file, "credentials");
+	if (!file || access(file, R_OK|W_OK))
 		file = expand_user_path("~/.git-credentials");
 	if (!file)
 		die("unable to set up default path; use --file");
-- 
2.3.1

Re: [GSoC microproject] Add XDG support to the credential-store helper

[Junio C Hamano]

Luis Ressel <aranea@aixah.de> writes:

> Oh, and should I add a testcase for the new functionality introduced by
> this patch? And if yes, what exactly should I test?

Absolutely.

Thinking things through to design what needs to be tested is part of
the exercise ;-)  You need to demonstrate that the new feature kicks
in when it should, and also you need to make sure the new feature
does not get used when it shouldn't.

Re: [GSoC microproject] Add XDG support to the credential-store helper

[Christian Couder]

Hi,

On Thu, Mar 5, 2015 at 9:51 PM, Luis Ressel <aranea@aixah.de> wrote:
> I'm contributing this patch in preparation for a GSoC15 application with
> the git project. In particular, I'm interested in the two bisect
> improvements listed on the Ideas page. Does anyone have other
> suggestions for potential improvements of git bisect that would be
> suitable for such a GSoC project?

Last year, on the ideas page:

http://git.github.io/SoC-2014-Ideas.html

we also had the following improvement:

"in some cases, git bisect may test too many merge bases, thus slowing
down the bisection (making it closer to linear than logarithmic)."

so you could work on that too.

And if you really want and have time you can also rewrite
git-bisect.sh into a builtin command. There is already
builtin/bisect--helper.c where you can put some migrated code step by
step.

> Oh, and should I add a testcase for the new functionality introduced by
> this patch? And if yes, what exactly should I test?

First, it looks like another student started working on the same
microproject. Did you have a look at what he did? If you didn't, you
probably should, so that reviewers don't need to tell you what they
already told the other student. And if you did, you should tell it,
and maybe explain what you did differently and why.

If it looks like the other student is more advanced on this subject,
you might want to try another microproject. And if all the
microprojects are already taken, you might want to ask the list for
other microprojects, or you may want to have a look at the many merge
bases improvement above. (You could start by having a look at
check_merge_bases() in "bisect.c" and by creating a script or better a
test case that reproduces the problem, see
t/t6030-bisect-porcelain.sh.)

Welcome to the Git community!

Best,
Christian.

Re: [GSoC microproject] Add XDG support to the credential-store helper

[Luis Ressel]

[-- Attachment #1: Type: text/plain, Size: 1701 bytes --]

On Thu, 5 Mar 2015 23:38:19 +0100
Christian Couder <christian.couder@gmail.com> wrote:

> Hi,
> 
> Last year, on the ideas page:
> 
> http://git.github.io/SoC-2014-Ideas.html
> 
> we also had the following improvement:
> 
> "in some cases, git bisect may test too many merge bases, thus slowing
> down the bisection (making it closer to linear than logarithmic)."
> 
> so you could work on that too.
> 
> And if you really want and have time you can also rewrite
> git-bisect.sh into a builtin command. There is already
> builtin/bisect--helper.c where you can put some migrated code step by
> step.
> 

Thanks for those suggestions!

> First, it looks like another student started working on the same
> microproject. Did you have a look at what he did? If you didn't, you
> probably should, so that reviewers don't need to tell you what they
> already told the other student. And if you did, you should tell it,
> and maybe explain what you did differently and why.
> 
> If it looks like the other student is more advanced on this subject,
> you might want to try another microproject. And if all the
> microprojects are already taken, you might want to ask the list for
> other microprojects, or you may want to have a look at the many merge
> bases improvement above. (You could start by having a look at
> check_merge_bases() in "bisect.c" and by creating a script or better a
> test case that reproduces the problem, see
> t/t6030-bisect-porcelain.sh.)
> 

Oops. I had a quick glance at the list archives, but apparently I
missed this. I'll have a look.

> Welcome to the Git community!
> 
> Best,
> Christian.

Thanks!


Regards,
Luis Ressel

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 949 bytes --]

Re: [GSoC microproject] Add XDG support to the credential-store helper

[Luis Ressel]

[-- Attachment #1: Type: text/plain, Size: 764 bytes --]

Okay, I've had a look at Paul's patch for this. Sorry again for the
dupe, I must've missed it before. I guess I'll attempt another
microproject.

However, I feel like my patch is really all that's neccessary; I don't
think we should try to use both files if they exist -- I consider
Paul's approach to be a bit overcomplicated.

My patch still uses ~/.git-credentials as a default location, which
should be sufficient for compability. A user that wants to use
~/.config/git/credentials instead has to manually create that file
first, which he just shouldn't do if he has to stay compatible to
previous git versions.

Of course, my patch is definitly lacking a test case, and the
documentation could be a tad better, too.


Regards,
Luis Ressel

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 949 bytes --]

Re: [GSoC microproject] Add XDG support to the credential-store helper

[Paul Tan]

Hi Luis,

On Fri, Mar 6, 2015 at 7:41 AM, Luis Ressel <aranea@aixah.de> wrote:
> Okay, I've had a look at Paul's patch for this. Sorry again for the
> dupe, I must've missed it before. I guess I'll attempt another
> microproject.
>
> However, I feel like my patch is really all that's neccessary; I don't
> think we should try to use both files if they exist -- I consider
> Paul's approach to be a bit overcomplicated.
>
> My patch still uses ~/.git-credentials as a default location, which
> should be sufficient for compability.

Indeed, that was my initial plan for implementation as well[1]. Matthieu,
however, wanted the behavior to follow that of git-config[2].

[1] http://article.gmane.org/gmane.comp.version-control.git/264666
[2] http://article.gmane.org/gmane.comp.version-control.git/264669

> A user that wants to use
> ~/.config/git/credentials instead has to manually create that file
> first, which he just shouldn't do if he has to stay compatible to
> previous git versions.

Yes, I totally agree. I wonder if the current behavior to read the home
config file in addition to the xdg config file is actually
useful to the end-user. (However, that behavior has been in git since 2012, so
for consistency purposes it may not be desirable to make credential-store act
differently from git-config.)

What I do believe, however, is that there *should* be a way for
credential-store to lookup, erase and (maybe) store credentials to multiple
files. This opens the door for git to be fully xdg base dir spec compliant
by supporing both $XDG_CONFIG_HOME and $XDG_CONFIG_DIRS.
I quote from the spec[3]:

    The order of base directories denotes their importance; the first directory
    listed is the most important. When the same information is defined in
    multiple places the information defined relative to the more important base
    directory takes precedent. The base directory defined by $XDG_DATA_HOME is
    considered more important than any of the base directories defined by
    $XDG_DATA_DIRS. The base directory defined by $XDG_CONFIG_HOME is
    considered more important than any of the base directories defined by
    $XDG_CONFIG_DIRS.

[3] http://standards.freedesktop.org/basedir-spec/basedir-spec-latest.html

Reading from multiple credential stores could be useful, for example, if the
system administrator wanted to setup credentials for all users on the system.

Thus, if we are implementing the machinery for the XDG dir spec, it would make
sense to just add the home git-credentials file to the search path as well.

Regards,
Paul

Re: [GSoC microproject] Add XDG support to the credential-store helper

[Matthieu Moy]

Paul Tan <pyokagan@gmail.com> writes:

> Hi Luis,
>
> On Fri, Mar 6, 2015 at 7:41 AM, Luis Ressel <aranea@aixah.de> wrote:
>> Okay, I've had a look at Paul's patch for this. Sorry again for the
>> dupe, I must've missed it before. I guess I'll attempt another
>> microproject.
>>
>> However, I feel like my patch is really all that's neccessary; I don't
>> think we should try to use both files if they exist -- I consider
>> Paul's approach to be a bit overcomplicated.
>>
>> My patch still uses ~/.git-credentials as a default location, which
>> should be sufficient for compability.
>
> Indeed, that was my initial plan for implementation as well[1]. Matthieu,
> however, wanted the behavior to follow that of git-config[2].

The fact that I suggested doing it this way does not mean it _has_ to be
done this way. Decisions are taken by trying to reach a consensus with
discussion, so everyone is welcome to argue.

I don't remember all the discussions we had about the ~/.gitconfig, but
one issue with considering only one file is if you create
~/.git/config/foo and initially make sure you don't have ~/.gitfoo, and
then one tool creates ~/.gitfoo (either an old Git, or another tool
trying to edit the config file), then you totally break your
configuration.

I argued for not taking backward compatibility too much into account in
another thread, but that was about precedence of one file over the other
which is far less important. Here, any tool creating even an empty home
file would break your configuration.

That also breaks the least surprise principle if you have a ~/.gitfoo
file that you forgot about: edit ~/.config/git/foo, nothing is taken
into account, at all (or the other way around, depending on the
precedence you choose). I remember loosing some time with two vlc
configuration files like this.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [GSoC microproject] Add XDG support to the credential-store helper

[Paul Tan]

Hi,

Reading my previous message again, I apologize if it sounded
conflicting. Truth to be told, I see merits in both proposed
behaviors, but it all depends on whether we want git-credentials-store
to support an arbitrary number of config files (now or in the future)
or just two. I'm not sitting on the fence though, personally I think
that we should go with supporting an arbitrary number of config files
(and the behavior it entails for xdg file vs home file), because it
will open up more possibilities in the future with regards to
supporting multiple config sources.

On Sat, Mar 7, 2015 at 1:28 AM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> The fact that I suggested doing it this way does not mean it _has_ to be
> done this way. Decisions are taken by trying to reach a consensus with
> discussion, so everyone is welcome to argue.

Well, I think we need to decide if git is going to implement support
for XDG_CONFIG_DIRS as well, as support for reading/writing an
arbitrary number of config files will affect my views on the behavior.
Personally, I think git-credentials-store should implement support for
XDG_CONFIG_DIRS because, as I mentioned in the previous message,
administrators may wish to provide users with default saved
credentials.

If machinery is being added to support reading/writing to an arbitrary
number of config files, it would lead to simpler behavior (and simpler
code) if the old ~/.git-credentials is just treated as just another
config file to load from. (So yes, I agree with implementing your
proposed behavior)

However, if we are just going to support 2 configuration files (the
xdg file and the home file), then I think Luis' proposed behavior has
some merit. See below.

(Just mentioning for completeness) The third option would be to
implement a hybrid of the above two approaches (support arbitrary
number of config files, but only choose 1 between the xdg file and
home file), but this behavior is unnecessarily complex.

> I don't remember all the discussions we had about the ~/.gitconfig, but
> one issue with considering only one file is if you create
> ~/.git/config/foo and initially make sure you don't have ~/.gitfoo, and
> then one tool creates ~/.gitfoo (either an old Git, or another tool
> trying to edit the config file), then you totally break your
> configuration.
> I argued for not taking backward compatibility too much into account in
> another thread, but that was about precedence of one file over the other
> which is far less important. Here, any tool creating even an empty home
> file would break your configuration.

Luis mentioned that if the user expects to use an old version of git,
the user would (or should) not create the xdg file in the first place.
I think that automated tools (and users) should call git-config to
edit the config files anyway and not roll their own. In fact, I think
that this issue will not occur at all if git prioritized
~/.config/git/foo over ~/.gitfoo instead of the other way around. When
the user creates the xdg file, the user is signaling that old versions
of git will not be used. Thus, if a tool creates/updates the old home
file (and it should not if it calls git-config), then configuration
changes won't take effect at all, and it _shouldn't_ because the tool
is _broken_.

But yes, the above strategy does not scale at all for multiple
configuration sources, which there will be if support for
XDG_CONFIG_DIRS is implemented.

(As an aside, I find it weird that git-config allows values in
~/.gitconfig to override ~/.config/git/config, given that the xdg file
is opt-in and introduced after ~/.gitconfig. Furthermore, it conflicts
with its writing behavior -- it writes to ~/.config/git/config and not
~/.gitconfig if it exists.)

> That also breaks the least surprise principle if you have a ~/.gitfoo
> file that you forgot about: edit ~/.config/git/foo, nothing is taken
> into account, at all (or the other way around, depending on the
> precedence you choose). I remember loosing some time with two vlc
> configuration files like this.

Hmm, I don't know the exact specifics of what happened with VLC, so I
can't judge. As mentioned above, if the user wants compatibility with
old tools, the user will not create the xdg file. If the user has an
updated toolset, the user will create the xdg file and delete the old
home file. The old home file will not be created at all because all
tools would have been updated to support the xdg file, and hence the
user will not be confused.

Of course, in the context of git-config, it has to read the files in
/etc/gitconfig, $GIT_DIR/config etc, and thus as mentioned above,
reading from the home file as well would lead to simpler behavior and
code.

Regards,
Paul