On Fri, Dec 09, 2011 at 10:00:44AM -0800, Junio C Hamano wrote:
> Jeff King <peff@xxxxxxxx> 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.
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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html