Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Maintenance locks and interactivity #598

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions Documentation/config/credential.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
74 changes: 46 additions & 28 deletions builtin/gc.c
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*
Expand All @@ -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;
}
Expand Down Expand Up @@ -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;
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a nit. As an alternative to defining a static string here, you could #define a macro and use that in each of the fprintf() format strings below (like we do for the PRIuMAX or PRItime strings).

/*
* get_schedule_cmd` reads the GIT_TEST_MAINT_SCHEDULER environment variable
* to mock the schedulers that `git maintenance start` rely on.
Expand Down Expand Up @@ -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"
Expand All @@ -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:
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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");
Expand Down Expand Up @@ -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"
Expand All @@ -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;
Expand Down
30 changes: 27 additions & 3 deletions credential.c
Original file line number Diff line number Diff line change
Expand Up @@ -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)
{
Expand Down Expand Up @@ -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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to trace the fact that the interactive credential prompt was skipped?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting idea. I was thinking that the lack of the other region would be enough.

Do you propose using a trace2_printf()? or what kind of indicator? I'm not familiar with an example of this kind of tracing.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you could do something like a trace2_data_intmax(... "credential", "interactive/skipped", 1)

only log the true case.

}
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)
Expand Down Expand Up @@ -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");
}

Expand Down
5 changes: 4 additions & 1 deletion scalar.c
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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] == '-') {
Expand Down
22 changes: 22 additions & 0 deletions t/t5551-http-fetch-smart.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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 &&
derrickstolee marked this conversation as resolved.
Show resolved Hide resolved

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 &&
Expand Down
20 changes: 3 additions & 17 deletions t/t7900-maintenance.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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 &&
Expand Down Expand Up @@ -771,6 +754,9 @@ test_expect_success 'start and stop Linux/systemd maintenance' '
test_systemd_analyze_verify "systemd/user/git-maintenance@daily.service" &&
test_systemd_analyze_verify "systemd/user/git-maintenance@weekly.service" &&

grep "core.askPass=true" "systemd/user/git-maintenance@.service" &&
grep "credential.interactive=false" "systemd/user/git-maintenance@.service" &&

printf -- "--user enable --now git-maintenance@%s.timer\n" hourly daily weekly >expect &&
test_cmp expect args &&

Expand Down
13 changes: 10 additions & 3 deletions t/t9210-scalar.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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
derrickstolee marked this conversation as resolved.
Show resolved Hide resolved
'

test_expect_success '`reconfigure -a` removes stale config entries' '
Expand Down Expand Up @@ -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/ |
Expand Down
Loading