Re: [PATCH v3 1/4] git-credential-store: support multiple credential files
Hi all, thanks for providing your feedback. On Sun, Mar 15, 2015 at 6:14 AM, Junio C Hamano gits...@pobox.com wrote: I am not sure if this is not a premature over-engineering---I am not convinced that such a future need will be fulfilled by passing just a single default_fn this version already passes, or it needs even more parameters that this version does not pass yet, and the interface to the function needs to be updated at that point when you need it _anyways_. One thing that we all agree is that we don't need the extra parameter within the context of what the current code does. After considering everyone's responses, I've decided to remove the argument in the v4 patch. As Junio says, when there is a policy change the code can be modified anyway. Regards, Paul -- 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 v3 1/4] git-credential-store: support multiple credential files
Junio C Hamano gits...@pobox.com writes: Paul Tan pyoka...@gmail.com writes: I think you could even get away without passing default_fn here, and just use the rule the first file in the list is the default. Unless you are anticipating ever passing something else, but I couldn't think of a case where that would be useful. Even though in this case the store_credential() function is not used anywhere else, from my personal API design experience I think that cementing the rule of the first file in the list is the default in the behavior of the function is not a good thing. For example, in the future, we may wish to keep the precedence ordering the same, but if none of the credential files exist, we create the XDG file by default instead. I am not sure if this is not a premature over-engineering I would say so if having this default_fn made the code more complex, but here the code is basically + if (default_fn) + store_credential_file(default_fn, c); and - store_credential(file, c); + store_credential(fns, c, fns.items[0].string); Taking the first element in the list wouldn't change much. I'm personally fine with both versions. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- 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 v3 1/4] git-credential-store: support multiple credential files
Matthieu Moy matthieu@grenoble-inp.fr writes: Junio C Hamano gits...@pobox.com writes: Paul Tan pyoka...@gmail.com writes: I think you could even get away without passing default_fn here, and just use the rule the first file in the list is the default. Unless you are anticipating ever passing something else, but I couldn't think of a case where that would be useful. Even though in this case the store_credential() function is not used anywhere else, from my personal API design experience I think that cementing the rule of the first file in the list is the default in the behavior of the function is not a good thing. For example, in the future, we may wish to keep the precedence ordering the same, but if none of the credential files exist, we create the XDG file by default instead. I am not sure if this is not a premature over-engineering I would say so if having this default_fn made the code more complex, True, or if it made caller(s) be redundant or repeat themselves without a good reason. Otherwise we would end up having to drop the redundant and/or unnecessary arguments in future clean-up patches; I had to de-conflict a series with 7ce7c760 (convert: drop arguments other than 'path' from would_convert_to_git(), 2014-08-21) recently, which reminded me of this point ;-). but here the code is basically + if (default_fn) + store_credential_file(default_fn, c); and - store_credential(file, c); + store_credential(fns, c, fns.items[0].string); Taking the first element in the list wouldn't change much. I'm personally fine with both versions. Turning the current code to drop the extra parameter and to use the first element instead wouldn't be a big change, but these things tend to add up, so unless this discussion immediately lead to a future enhancement plan to make use of the flexibility the parameter gives us, I'd rather see things kept simpler. I do not terribly mind either way, either, though. -- 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 v3 1/4] git-credential-store: support multiple credential files
Paul Tan pyoka...@gmail.com writes: I think you could even get away without passing default_fn here, and just use the rule the first file in the list is the default. Unless you are anticipating ever passing something else, but I couldn't think of a case where that would be useful. Even though in this case the store_credential() function is not used anywhere else, from my personal API design experience I think that cementing the rule of the first file in the list is the default in the behavior of the function is not a good thing. For example, in the future, we may wish to keep the precedence ordering the same, but if none of the credential files exist, we create the XDG file by default instead. I am not sure if this is not a premature over-engineering---I am not convinced that such a future need will be fulfilled by passing just a single default_fn this version already passes, or it needs even more parameters that this version does not pass yet, and the interface to the function needs to be updated at that point when you need it _anyways_. One thing that we all agree is that we don't need the extra parameter within the context of what the current code does. So, it smells like a case of YAGNI a bit, at least to me. -- 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 v3 1/4] git-credential-store: support multiple credential files
On Fri, Mar 13, 2015 at 2:15 PM, Jeff King p...@peff.net wrote: +static void store_credential(const struct string_list *fns, struct credential *c, + const char *default_fn) I think you could even get away without passing default_fn here, and just use the rule the first file in the list is the default. Unless you are anticipating ever passing something else, but I couldn't think of a case where that would be useful. Even though in this case the store_credential() function is not used anywhere else, from my personal API design experience I think that cementing the rule of the first file in the list is the default in the behavior of the function is not a good thing. For example, in the future, we may wish to keep the precedence ordering the same, but if none of the credential files exist, we create the XDG file by default instead. It's a balance of flexibility, but in this case I think putting the default filename in a separate argument is a good thing. + struct string_list fns = STRING_LIST_INIT_NODUP; This is declared NODUP... - if (!file) + if (file) + string_list_append_nodup(fns, file); So you can just call the regular string_list_append here (the idea of declaring the list as DUP or NODUP is that the callers do not have to care; string_list_append does the right thing). Actually, thinking about it again from a memory management perspective, STRING_LIST_INIT_DUP should be used instead as the string_list `fns` should own the memory of the strings inside it. Thus, in this case since `file` is pulled from the argv array, string_list_append() should be used to duplicate the memory, and for expand_user_path(), string_list_append_nodup() should be used because the returned path is newly-allocated memory. Finally, string_list_clear() should be called at the end to release memory. Thanks for taking the time to review the patches, I will work on v4 now. (Really keen on getting this to pu) Regards, Paul -- 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 v3 1/4] git-credential-store: support multiple credential files
On Sat, Mar 14, 2015 at 04:15:53PM +0800, Paul Tan wrote: Even though in this case the store_credential() function is not used anywhere else, from my personal API design experience I think that cementing the rule of the first file in the list is the default in the behavior of the function is not a good thing. For example, in the future, we may wish to keep the precedence ordering the same, but if none of the credential files exist, we create the XDG file by default instead. It's a balance of flexibility, but in this case I think putting the default filename in a separate argument is a good thing. Yeah, I see your line of reasoning. I think this is probably a case of YAGNI, but it is really a matter of personal preference. It's not a big deal either way. So you can just call the regular string_list_append here (the idea of declaring the list as DUP or NODUP is that the callers do not have to care; string_list_append does the right thing). Actually, thinking about it again from a memory management perspective, STRING_LIST_INIT_DUP should be used instead as the string_list `fns` should own the memory of the strings inside it. Thus, in this case since `file` is pulled from the argv array, string_list_append() should be used to duplicate the memory, and for expand_user_path(), string_list_append_nodup() should be used because the returned path is newly-allocated memory. Finally, string_list_clear() should be called at the end to release memory. Yeah, I had the same thought, but I noticed that you don't call string_list_clear. Nor is it really necessary to do so. Since git programs are generally short-lived, we often let the OS take care of deallocating variables like this (it's not appropriate for all variables, of course, but quite frequently there are variables that are effectively allocated at program startup and used until program end). Again, I think this is a matter of taste. I don't mind if you want to string_list_clear at the end, and I agree that using nodup() is the right thing in that case. -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 v3 1/4] git-credential-store: support multiple credential files
On Sat, Mar 14, 2015 at 01:33:28PM -0400, Jeff King wrote: On Sat, Mar 14, 2015 at 04:15:53PM +0800, Paul Tan wrote: Even though in this case the store_credential() function is not used anywhere else, from my personal API design experience I think that cementing the rule of the first file in the list is the default in the behavior of the function is not a good thing. For example, in the future, we may wish to keep the precedence ordering the same, but if none of the credential files exist, we create the XDG file by default instead. It's a balance of flexibility, but in this case I think putting the default filename in a separate argument is a good thing. Yeah, I see your line of reasoning. I think this is probably a case of YAGNI, but it is really a matter of personal preference. It's not a big deal either way. By the way, I hope this (and the other comment) do not come off as you are wrong, but I do not feel like arguing with you. I really do think these are a matter of taste, and while we often express issues of taste in reviews, it is ultimately up to the patch submitter (who is, after all, doing most of the work) to have the final say on minor issues like this. Sometimes the response to taste issue is oh, I didn't think to do that, thanks for the suggestion and sometimes it is nah, I like it better my way. And both are OK. Of course there are also taste issues where we insist (e.g., consistent whitespace), but I do not think this is one of them. :) Maybe that was all obvious, but since you are new to the list, I wanted to make sure I was clear. -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 v3 1/4] git-credential-store: support multiple credential files
On Wed, Mar 11, 2015 at 02:49:10PM +0800, Paul Tan wrote: Previously, git-credential-store only supported storing credentials in a single file: ~/.git-credentials. In order to support the XDG base directory specification[1], git-credential-store needs to be able to lookup and erase credentials from multiple files, as well as to pick the appropriate file to write to so that the credentials can be found on subsequent lookups. I looked over the code here and in the second patch, and I didn't see any real problems. Thanks for iterating on this. Two minor nits, that might not even be worth addressing: +static void store_credential(const struct string_list *fns, struct credential *c, + const char *default_fn) I think you could even get away without passing default_fn here, and just use the rule the first file in the list is the default. Unless you are anticipating ever passing something else, but I couldn't think of a case where that would be useful. + struct string_list fns = STRING_LIST_INIT_NODUP; This is declared NODUP... - if (!file) + if (file) + string_list_append_nodup(fns, file); So you can just call the regular string_list_append here (the idea of declaring the list as DUP or NODUP is that the callers do not have to care; string_list_append does the right thing). -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
[PATCH v3 1/4] git-credential-store: support multiple credential files
Previously, git-credential-store only supported storing credentials in a
single file: ~/.git-credentials. In order to support the XDG base
directory specification[1], git-credential-store needs to be able to
lookup and erase credentials from multiple files, as well as to pick the
appropriate file to write to so that the credentials can be found on
subsequent lookups.
[1] http://standards.freedesktop.org/basedir-spec/basedir-spec-0.7.html
Note that some credential storage files may not be owned, readable or
writable by the user, as they may be system-wide files that are meant to
apply to every user.
Instead of a single file path, lookup_credential(), remove_credential()
and store_credential() now take a precedence-ordered string_list of
file paths. lookup_credential() expects both user-specific and
system-wide credential files to be provided to support the use case of
system administrators setting default credentials for users.
remove_credential() and store_credential() expect only the user-specific
credential files to be provided as usually the only config files that
users are allowed to edit are their own user-specific ones.
lookup_credential() will read these (user-specific and system-wide) file
paths in order until it finds the 1st matching credential and print it.
As some files may be private and thus unreadable, any file which cannot
be read will be ignored silently.
remove_credential() will erase credentials from all (user-specific)
files in the list. This is because if credentials are only erased from
the file with the highest precedence, a matching credential may still be
found in a file further down the list. (Note that due to the lockfile
code, this requires the directory to be writable, which should be so for
user-specific config files)
store_credential() will write the credentials to the first existing
(user-specific) file in the list. If none of the files in the list
exist, store_credential() will write to the filename specified by
default_fn, thus creating it. For backwards compatibility, default_fn
should be ~/.git-credentials.
Helped-by: Matthieu Moy matthieu@grenoble-inp.fr
Helped-by: Junio C Hamano gits...@pobox.com
Helped-by: Jeff King p...@peff.net
Signed-off-by: Paul Tan pyoka...@gmail.com
---
Previous version can be found at [1].
[1] http://thread.gmane.org/gmane.comp.version-control.git/265042/focus=265038
The changes are as follows:
* store_credential(), instead of taking an index to the string_list for the
default filename, takes a filename string instead as it leads to a more
flexible API.
credential-store.c | 78 +-
1 file changed, 53 insertions(+), 25 deletions(-)
diff --git a/credential-store.c b/credential-store.c
index 925d3f4..803bed2 100644
--- a/credential-store.c
+++ b/credential-store.c
@@ -6,7 +6,7 @@
static struct lock_file credential_lock;
-static void parse_credential_file(const char *fn,
+static int parse_credential_file(const char *fn,
struct credential *c,
void (*match_cb)(struct credential *),
void (*other_cb)(struct strbuf *))
@@ -14,18 +14,20 @@ static void parse_credential_file(const char *fn,
FILE *fh;
struct strbuf line = STRBUF_INIT;
struct credential entry = CREDENTIAL_INIT;
+ int found_credential = 0;
fh = fopen(fn, r);
if (!fh) {
- if (errno != ENOENT)
+ if (errno != ENOENT errno != EACCES)
die_errno(unable to open %s, fn);
- return;
+ return found_credential;
}
while (strbuf_getline(line, fh, '\n') != EOF) {
credential_from_url(entry, line.buf);
if (entry.username entry.password
credential_match(c, entry)) {
+ found_credential = 1;
if (match_cb) {
match_cb(entry);
break;
@@ -38,6 +40,7 @@ static void parse_credential_file(const char *fn,
credential_clear(entry);
strbuf_release(line);
fclose(fh);
+ return found_credential;
}
static void print_entry(struct credential *c)
@@ -64,21 +67,10 @@ static void rewrite_credential_file(const char *fn, struct
credential *c,
die_errno(unable to commit credential store);
}
-static void store_credential(const char *fn, struct credential *c)
+static void store_credential_file(const char *fn, struct credential *c)
{
struct strbuf buf = STRBUF_INIT;
- /*
-* Sanity check that what we are storing is actually sensible.
-* In particular, we can't make a URL without a protocol field.
-* Without either a host or pathname (depending on the scheme),
-* we have no primary key. And without a username and password,
-* we are not actually
