Maintenance locks and interactivity (#598)

* [x] This is an early version of work ~~already under review
upstream~~.
* [x] This change only applies to interactions with Azure DevOps and the
      GVFS Protocol.
* [ ] This change only applies to the virtualization hook and VFS for
Git.

This set of patches has a little of both:

1. We need to revert a custom change around `maintenance.lock` files
that was only in `microsoft/git`. The "fix" actually caused a worse
situation where many background maintenance processes would pile up.
2. Create the `credential.interactive` config as an official Git config
and use it in Git to prevent any chance of a foreground
username/password request. (This is borrowed from GCM, which already
respects this.)
3. To help avoid problems, especially when blocked by credentials, add
configuration to the background schedule to avoid interactive prompts.
4. To make sure that the new config options are set up in the background
schedule, update `scalar reconfigure` to execute `git maintenance
start`.

Points 2-4 could maybe to upstream, but we shouldn't wait for that.

I've tested these features locally on Linux and Windows and
double-checked that they will prevent the backup of maintenance
processes when the credentials become invalid.

Author: derrickstolee

Documentation/config/credential.txt

@@ -9,6 +9,14 @@ credential.helper::
 Note that multiple helpers may be defined. See linkgit:gitcredentials[7]
 for details and examples.
 
+credential.interactive::
+	By default, Git and any configured credential helpers will ask for
+	user input when new credentials are required. Many of these helpers
+	will succeed based on stored credentials if those credentials are
+	still valid. To avoid the possibility of user interactivity from
+	Git, set `credential.interactive=false`. Some credential helpers
+	respect this option as well.
+
 credential.useHttpPath::
 	When acquiring credentials, consider the "path" component of an http
 	or https URL to be important. Defaults to false. See

builtin/gc.c

@@ -1309,8 +1309,6 @@ static int maintenance_run_tasks(struct maintenance_run_opts *opts)
 	char *lock_path = xstrfmt("%s/maintenance", r->objects->odb->path);
 
 	if (hold_lock_file_for_update(&lk, lock_path, LOCK_NO_DEREF) < 0) {
-		struct stat st;
-		struct strbuf lock_dot_lock = STRBUF_INIT;
 		/*
 		 * Another maintenance command is running.
 		 *
@@ -1321,25 +1319,6 @@ static int maintenance_run_tasks(struct maintenance_run_opts *opts)
 		if (!opts->auto_flag && !opts->quiet)
 			warning(_("lock file '%s' exists, skipping maintenance"),
 				lock_path);
-
-		/*
-		 * Check timestamp on .lock file to see if we should
-		 * delete it to recover from a fail state.
-		 */
-		strbuf_addstr(&lock_dot_lock, lock_path);
-		strbuf_addstr(&lock_dot_lock, ".lock");
-		if (lstat(lock_dot_lock.buf, &st))
-			warning_errno(_("unable to stat '%s'"), lock_dot_lock.buf);
-		else {
-			if (st.st_mtime < time(NULL) - (6 * 60 * 60)) {
-				if (unlink(lock_dot_lock.buf))
-					warning_errno(_("unable to delete stale lock file"));
-				else
-					warning(_("deleted stale lock file"));
-			}
-		}
-
-		strbuf_release(&lock_dot_lock);
 		free(lock_path);
 		return 0;
 	}
@@ -1678,6 +1657,42 @@ static const char *get_frequency(enum schedule_priority schedule)
 	}
 }
 
+static const char *extraconfig[] = {
+	"credential.interactive=false",
+	"core.askPass=true", /* 'true' returns success, but no output. */
+	NULL
+};
+
+static const char *get_extra_config_parameters(void) {
+	static const char *result = NULL;
+	struct strbuf builder = STRBUF_INIT;
+
+	if (result)
+		return result;
+
+	for (const char **s = extraconfig; s && *s; s++)
+		strbuf_addf(&builder, "-c %s ", *s);
+
+	result = strbuf_detach(&builder, NULL);
+	return result;
+}
+
+static const char *get_extra_launchctl_strings(void) {
+	static const char *result = NULL;
+	struct strbuf builder = STRBUF_INIT;
+
+	if (result)
+		return result;
+
+	for (const char **s = extraconfig; s && *s; s++) {
+		strbuf_addstr(&builder, "<string>-c</string>\n");
+		strbuf_addf(&builder, "<string>%s</string>\n", *s);
+	}
+
+	result = strbuf_detach(&builder, NULL);
+	return result;
+}
+
 /*
  * get_schedule_cmd` reads the GIT_TEST_MAINT_SCHEDULER environment variable
  * to mock the schedulers that `git maintenance start` rely on.
@@ -1884,6 +1899,7 @@ static int launchctl_schedule_plist(const char *exec_path, enum schedule_priorit
 		   "<array>\n"
 		   "<string>%s/git</string>\n"
 		   "<string>--exec-path=%s</string>\n"
+		   "%s" /* For extra config parameters. */
 		   "<string>for-each-repo</string>\n"
 		   "<string>--config=maintenance.repo</string>\n"
 		   "<string>maintenance</string>\n"
@@ -1892,7 +1908,8 @@ static int launchctl_schedule_plist(const char *exec_path, enum schedule_priorit
 		   "</array>\n"
 		   "<key>StartCalendarInterval</key>\n"
 		   "<array>\n";
-	strbuf_addf(&plist, preamble, name, exec_path, exec_path, frequency);
+	strbuf_addf(&plist, preamble, name, exec_path, exec_path,
+		    get_extra_launchctl_strings(), frequency);
 
 	switch (schedule) {
 	case SCHEDULE_HOURLY:
@@ -2127,11 +2144,12 @@ static int schtasks_schedule_task(const char *exec_path, enum schedule_priority
 	      "<Actions Context=\"Author\">\n"
 	      "<Exec>\n"
 	      "<Command>\"%s\\headless-git.exe\"</Command>\n"
-	      "<Arguments>--exec-path=\"%s\" for-each-repo --config=maintenance.repo maintenance run --schedule=%s</Arguments>\n"
+	      "<Arguments>--exec-path=\"%s\" %s for-each-repo --config=maintenance.repo maintenance run --schedule=%s</Arguments>\n"
 	      "</Exec>\n"
 	      "</Actions>\n"
 	      "</Task>\n";
-	fprintf(tfile->fp, xml, exec_path, exec_path, frequency);
+	fprintf(tfile->fp, xml, exec_path, exec_path,
+		get_extra_config_parameters(), frequency);
 	strvec_split(&child.args, cmd);
 	strvec_pushl(&child.args, "/create", "/tn", name, "/f", "/xml",
 				  get_tempfile_path(tfile), NULL);
@@ -2272,8 +2290,8 @@ static int crontab_update_schedule(int run_maintenance, int fd)
 			"# replaced in the future by a Git command.\n\n");
 
 		strbuf_addf(&line_format,
-			    "%%d %%s * * %%s \"%s/git\" --exec-path=\"%s\" for-each-repo --config=maintenance.repo maintenance run --schedule=%%s\n",
-			    exec_path, exec_path);
+			    "%%d %%s * * %%s \"%s/git\" --exec-path=\"%s\" %s for-each-repo --config=maintenance.repo maintenance run --schedule=%%s\n",
+			    exec_path, exec_path, get_extra_config_parameters());
 		fprintf(cron_in, line_format.buf, minute, "1-23", "*", "hourly");
 		fprintf(cron_in, line_format.buf, minute, "0", "1-6", "daily");
 		fprintf(cron_in, line_format.buf, minute, "0", "0", "weekly");
@@ -2473,7 +2491,7 @@ static int systemd_timer_write_service_template(const char *exec_path)
 	       "\n"
 	       "[Service]\n"
 	       "Type=oneshot\n"
-	       "ExecStart=\"%s/git\" --exec-path=\"%s\" for-each-repo --config=maintenance.repo maintenance run --schedule=%%i\n"
+	       "ExecStart=\"%s/git\" --exec-path=\"%s\" %s for-each-repo --config=maintenance.repo maintenance run --schedule=%%i\n"
 	       "LockPersonality=yes\n"
 	       "MemoryDenyWriteExecute=yes\n"
 	       "NoNewPrivileges=yes\n"
@@ -2483,7 +2501,7 @@ static int systemd_timer_write_service_template(const char *exec_path)
 	       "RestrictSUIDSGID=yes\n"
 	       "SystemCallArchitectures=native\n"
 	       "SystemCallFilter=@system-service\n";
-	if (fprintf(file, unit, exec_path, exec_path) < 0) {
+	if (fprintf(file, unit, exec_path, exec_path, get_extra_config_parameters()) < 0) {
 		error(_("failed to write to '%s'"), filename);
 		fclose(file);
 		goto error;

credential.c

@@ -11,6 +11,8 @@
 #include "strbuf.h"
 #include "urlmatch.h"
 #include "git-compat-util.h"
+#include "trace2.h"
+#include "repository.h"
 
 void credential_init(struct credential *c)
 {
@@ -196,14 +198,36 @@ static char *credential_ask_one(const char *what, struct credential *c,
 	return xstrdup(r);
 }
 
-static void credential_getpass(struct credential *c)
+static int credential_getpass(struct credential *c)
 {
+	int interactive;
+	char *value;
+	if (!git_config_get_maybe_bool("credential.interactive", &interactive) &&
+	    !interactive) {
+		trace2_data_intmax("credential", the_repository,
+				   "interactive/skipped", 1);
+		return -1;
+	}
+	if (!git_config_get_string("credential.interactive", &value)) {
+		int same = !strcmp(value, "never");
+		free(value);
+		if (same) {
+			trace2_data_intmax("credential", the_repository,
+					   "interactive/skipped", 1);
+			return -1;
+		}
+	}
+
+	trace2_region_enter("credential", "interactive", the_repository);
 	if (!c->username)
 		c->username = credential_ask_one("Username", c,
 						 PROMPT_ASKPASS|PROMPT_ECHO);
 	if (!c->password)
 		c->password = credential_ask_one("Password", c,
 						 PROMPT_ASKPASS);
+	trace2_region_leave("credential", "interactive", the_repository);
+
+	return 0;
 }
 
 int credential_read(struct credential *c, FILE *fp)
@@ -381,8 +405,8 @@ void credential_fill(struct credential *c)
 			    c->helpers.items[i].string);
 	}
 
-	credential_getpass(c);
-	if (!c->username && !c->password)
+	if (credential_getpass(c) ||
+	    (!c->username && !c->password))
 		die("unable to get password from user");
 }
 

scalar.c

@@ -1098,6 +1098,9 @@ static int cmd_reconfigure(int argc, const char **argv)
 		if (set_recommended_config(1) < 0)
 			failed = -1;
 
+		if (toggle_maintenance(1) < 0)
+			failed = -1;
+
 loop_end:
 		if (failed) {
 			res = failed;
@@ -1400,7 +1403,7 @@ int cmd_main(int argc, const char **argv)
 	if (is_unattended()) {
 		setenv("GIT_ASKPASS", "", 0);
 		setenv("GIT_TERMINAL_PROMPT", "false", 0);
-		git_config_push_parameter("credential.interactive=never");
+		git_config_push_parameter("credential.interactive=false");
 	}
 
 	while (argc > 1 && *argv[1] == '-') {

t/t5551-http-fetch-smart.sh

@@ -186,6 +186,28 @@ test_expect_success 'clone from password-protected repository' '
 	test_cmp expect actual
 '
 
+test_expect_success 'credential.interactive=false skips askpass' '
+	set_askpass bogus nonsense &&
+	(
+		GIT_TRACE2_EVENT="$(pwd)/interactive-true" &&
+		export GIT_TRACE2_EVENT &&
+		test_must_fail git clone --bare "$HTTPD_URL/auth/smart/repo.git" interactive-true-dir &&
+		test_region credential interactive interactive-true &&
+
+		GIT_TRACE2_EVENT="$(pwd)/interactive-false" &&
+		export GIT_TRACE2_EVENT &&
+		test_must_fail git -c credential.interactive=false \
+			clone --bare "$HTTPD_URL/auth/smart/repo.git" interactive-false-dir &&
+		test_region ! credential interactive interactive-false &&
+
+		GIT_TRACE2_EVENT="$(pwd)/interactive-never" &&
+		export GIT_TRACE2_EVENT &&
+		test_must_fail git -c credential.interactive=never \
+			clone --bare "$HTTPD_URL/auth/smart/repo.git" interactive-never-dir &&
+		test_region ! credential interactive interactive-never
+	)
+'
+
 test_expect_success 'clone from auth-only-for-push repository' '
 	echo two >expect &&
 	set_askpass wrong &&

t/t7900-maintenance.sh

@@ -54,23 +54,6 @@ test_expect_success 'run [--auto|--quiet]' '
 	test_subcommand git gc --no-quiet <run-no-quiet.txt
 '
 
-test_expect_success 'lock file behavior' '
-	test_when_finished git config --unset maintenance.commit-graph.schedule &&
-	git config maintenance.commit-graph.schedule hourly &&
-
-	touch .git/objects/maintenance.lock &&
-	git maintenance run --schedule=hourly --no-quiet 2>err &&
-	grep "lock file .* exists, skipping maintenance" err &&
-
-	test-tool chmtime =-22000 .git/objects/maintenance.lock &&
-	git maintenance run --schedule=hourly --no-quiet 2>err &&
-	grep "deleted stale lock file" err &&
-	test_path_is_missing .git/objects/maintenance.lock &&
-
-	git maintenance run --schedule=hourly 2>err &&
-	test_must_be_empty err
-'
-
 test_expect_success 'maintenance.auto config option' '
 	GIT_TRACE2_EVENT="$(pwd)/default" git commit --quiet --allow-empty -m 1 &&
 	test_subcommand git maintenance run --auto --quiet <default &&
@@ -771,6 +754,9 @@ test_expect_success 'start and stop Linux/systemd maintenance' '
 	test_systemd_analyze_verify "systemd/user/[email protected]" &&
 	test_systemd_analyze_verify "systemd/user/[email protected]" &&
 
+	grep "core.askPass=true" "systemd/user/[email protected]" &&
+	grep "credential.interactive=false" "systemd/user/[email protected]" &&
+
 	printf -- "--user enable --now git-maintenance@%s.timer\n" hourly daily weekly >expect &&
 	test_cmp expect args &&
 

t/t9210-scalar.sh

@@ -176,8 +176,11 @@ test_expect_success 'scalar reconfigure' '
 	scalar reconfigure one &&
 	test true = "$(git -C one/src config core.preloadIndex)" &&
 	git -C one/src config core.preloadIndex false &&
-	scalar reconfigure -a &&
-	test true = "$(git -C one/src config core.preloadIndex)"
+	rm one/src/cron.txt &&
+	GIT_TRACE2_EVENT="$(pwd)/reconfigure" scalar reconfigure -a &&
+	test_path_is_file one/src/cron.txt &&
+	test true = "$(git -C one/src config core.preloadIndex)" &&
+	test_subcommand git maintenance start <reconfigure
 '
 
 test_expect_success '`reconfigure -a` removes stale config entries' '
@@ -300,7 +303,11 @@ test_expect_success 'start GVFS-enabled server' '
 test_expect_success '`scalar clone` with GVFS-enabled server' '
 	: the fake cache server requires fake authentication &&
 	git config --global core.askPass true &&
-	scalar clone --single-branch -- http://$HOST_PORT/ using-gvfs &&
+
+	# We must set credential.interactive=true to bypass a setting
+	# in "scalar clone" that disables interactive credentials during
+	# an unattended command.
+	scalar -c credential.interactive=true clone --single-branch -- http://$HOST_PORT/ using-gvfs &&
 
 	: verify that the shared cache has been configured &&
 	cache_key="url_$(printf "%s" http://$HOST_PORT/ |