[RFC PATCH v10 2/1] credential-store: warn also for store and erase

["Carlo Marcelo Arenas Belón" <carenas@gmail.com>]

done in a hacky way and only as a POC, but at least works, if we
are still thinking that warning should be at least comprehensive
for the first iteration.

didn't change the callback as you suggested but I think this
doesn't look that bad either.

squash for final v10 or ..?
---
 Documentation/git-credential-store.txt |  7 -------
 credential-store.c                     | 19 +++++++++----------
 t/t0302-credential-store.sh            | 20 +++++++++++++++++---
 3 files changed, 26 insertions(+), 20 deletions(-)

diff --git a/Documentation/git-credential-store.txt b/Documentation/git-credential-store.txt
index 18ba8b6984..d5841cffad 100644
--- a/Documentation/git-credential-store.txt
+++ b/Documentation/git-credential-store.txt
@@ -105,13 +105,6 @@ like spaces, line wrapping or text encoding.
 An unparseable or otherwise invalid line is ignored, and a warning
 message points out the problematic file and line number it appears in.
 
-Note that because of deficiencies of the current implementation, these
-warnings will be only generated for the entries that were processed
-while reading the credential files during a get operation, and so they
-are not comprehensive, and will require you to use an editor (or
-better a sed/awk/perl one liner) to edit the credential file and remove
-them.
-
 When Git needs authentication for a particular URL context,
 credential-store will consider that context a pattern to match against
 each entry in the credentials file.  If the protocol, hostname, and
diff --git a/credential-store.c b/credential-store.c
index c2778eff8a..236df8e7bf 100644
--- a/credential-store.c
+++ b/credential-store.c
@@ -4,8 +4,6 @@
 #include "string-list.h"
 #include "parse-options.h"
 
-#define PARSE_VERBOSE 0x01
-
 static struct lock_file credential_lock;
 
 static int valid_credential(struct credential *entry)
@@ -17,7 +15,7 @@ static int valid_credential(struct credential *entry)
 
 static int parse_credential_file(const char *fn,
 				  struct credential *c,
-				  int flags,
+				  int initial_line,
 				  void (*match_cb)(struct credential *),
 				  void (*other_cb)(struct strbuf *))
 {
@@ -25,7 +23,7 @@ static int parse_credential_file(const char *fn,
 	struct strbuf line = STRBUF_INIT;
 	struct credential entry = CREDENTIAL_INIT;
 	int found_credential = 0;
-	int lineno = 0;
+	int lineno = initial_line;
 
 	fh = fopen(fn, "r");
 	if (!fh) {
@@ -40,8 +38,7 @@ static int parse_credential_file(const char *fn,
 		if (strchr(line.buf, '\r') ||
 			credential_from_url_gently(&entry, line.buf, 1) ||
 			!valid_credential(&entry)) {
-			if (flags & PARSE_VERBOSE)
-				warning(_("%s:%d: ignoring invalid credential"),
+			warning(_("%s:%d: ignoring invalid credential"),
 					fn, lineno);
 		} else if (credential_match(c, &entry)) {
 			found_credential = 1;
@@ -77,11 +74,14 @@ static void print_line(struct strbuf *buf)
 static void rewrite_credential_file(const char *fn, struct credential *c,
 				    struct strbuf *extra)
 {
+	int initial_line = 0;
 	if (hold_lock_file_for_update(&credential_lock, fn, 0) < 0)
 		die_errno("unable to get credential storage lock");
-	if (extra)
+	if (extra) {
 		print_line(extra);
-	parse_credential_file(fn, c, 0, NULL, print_line);
+		initial_line++;
+	}
+	parse_credential_file(fn, c, initial_line, NULL, print_line);
 	if (commit_lock_file(&credential_lock) < 0)
 		die_errno("unable to write credential store");
 }
@@ -158,8 +158,7 @@ static void lookup_credential(const struct string_list *fns, struct credential *
 	struct string_list_item *fn;
 
 	for_each_string_list_item(fn, fns)
-		if (parse_credential_file(fn->string, c, PARSE_VERBOSE,
-						 print_entry, NULL))
+		if (parse_credential_file(fn->string, c, 0, print_entry, NULL))
 			return; /* Found credential */
 }
 
diff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh
index 00608e365a..78010590f4 100755
--- a/t/t0302-credential-store.sh
+++ b/t/t0302-credential-store.sh
@@ -218,13 +218,27 @@ test_expect_success 'get: store file can contain empty/bogus lines' '
 	test_line_count = 3 stderr
 '
 
-test_expect_success 'store: store succeeds silently in corrupted file' '
+test_expect_success 'store: succeeds in corrupted file but correctly warn' '
 	echo "#comment" >"$HOME/.git-credentials" &&
-	check approve store <<-\EOF &&
+	test_config credential.helper store &&
+	git credential approve <<-\EOF &&
 	url=https://user:pass@example.com
 	EOF
 	grep "https://user:pass@example.com" "$HOME/.git-credentials" &&
-	test_must_be_empty stderr
+	test_i18ngrep "git-credentials:2: ignoring invalid credential" stderr
+'
+
+test_expect_success 'erase: succeeds in corrupted file but correctly warn' '
+	cat <<-\CONFIG >"$HOME/.git-credentials" &&
+	#comment
+	https://user:pass@example.com
+	CONFIG
+	test_config credential.helper store &&
+	git credential reject <<-\EOF &&
+	url=https://example.com
+	EOF
+	! grep "https://user:pass@example.com" "$HOME/.git-credentials" &&
+	test_i18ngrep "git-credentials:1: ignoring invalid credential" stderr
 '
 
 test_done
-- 
2.26.2.3.g90bd2f7136