Jeff King [off-list ref] writes:
It works, and it detects truncated output both ways properly (I know
because I had to update every test, since the old output was missing the
end-of-credential marker).
It makes me a little sad, because the original format (relying on EOF)
was so Unix-y.
It saddens me, too. A reasonable middle ground would be to stop treating
an empty input as "no restriction" but "never matches".
I suspect that it is far more likely for a helper to fail (due to
configuration errors, for example) before it produces any output than
after it gives some but not all output lines.
On Fri, Dec 09, 2011 at 10:00:44AM -0800, Junio C Hamano wrote:
Jeff King [off-list ref] writes:
quoted
It works, and it detects truncated output both ways properly (I know
because I had to update every test, since the old output was missing the
end-of-credential marker).
It makes me a little sad, because the original format (relying on EOF)
was so Unix-y.
It saddens me, too. A reasonable middle ground would be to stop treating
an empty input as "no restriction" but "never matches".
I suspect that it is far more likely for a helper to fail (due to
configuration errors, for example) before it produces any output than
after it gives some but not all output lines.
Yeah, I think that's a reasonable compromise. Instead of the patch I
posted earlier, how about this:
diff --git a/credential-store.c b/credential-store.c
index a2c2cd0..26f7589 100644
--- a/credential-store.c
+++ b/credential-store.c
@@ -96,7 +96,16 @@ static void store_credential(const char *fn, struct credential *c)
static void remove_credential(const char *fn, struct credential *c)
{
- rewrite_credential_file(fn, c, NULL);
+ /*
+ * Sanity check that we actually have something to match
+ * against. The input we get is a restrictive pattern,
+ * so technically a blank credential means "erase everything".
+ * But it is too easy to accidentally send this, since it is equivalent
+ * to empty input. So explicitly disallow it, and require that the
+ * pattern have some actual content to match.
+ */
+ if (c->protocol || c->host || c->path || c->username)
+ rewrite_credential_file(fn, c, NULL);
}
static int lookup_credential(const char *fn, struct credential *c)
We _could_ modify credential_match() to automatically reject such a
pattern at that level, but it does actually have a use on the lookup
side. In config, a context like "https://example.com/foo.git" would
match each of:
[credential "https://example.com/foo.git"]
helper = ...
[credential "https://example.com"]
helper = ...
[credential "https://"]
helper = ...
[credential]
helper = ...
The final one is an just an extension of the others to the empty pattern
(you could also spell it [credential ""], and it would have the same
effect).
So the "empty pattern" does actually have a use, from the end-users's
point of view. It's just that with removal, it's a little more dangerous
and a little less likely to be useful (as compared to lookup).
-Peff