Merge branch 'jk/repack-tempfile-cleanup'

The way "git repack" creared temporary files when it received a
signal was prone to deadlocking, which has been corrected.

* jk/repack-tempfile-cleanup:
  t7700: annotate cruft-pack failure with ok=sigpipe
  repack: drop remove_temporary_files()
  repack: use tempfiles for signal cleanup
  repack: expand error message for missing pack files
  repack: populate extension bits incrementally
  repack: convert "names" util bitfield to array
maint
Taylor Blau 2022-10-30 21:04:42 -04:00
commit c88895e67b
2 changed files with 45 additions and 61 deletions

View File

@ -91,44 +91,6 @@ static int repack_config(const char *var, const char *value, void *cb)
return git_default_config(var, value, cb); return git_default_config(var, value, cb);
} }


/*
* Remove temporary $GIT_OBJECT_DIRECTORY/pack/.tmp-$$-pack-* files.
*/
static void remove_temporary_files(void)
{
struct strbuf buf = STRBUF_INIT;
size_t dirlen, prefixlen;
DIR *dir;
struct dirent *e;

dir = opendir(packdir);
if (!dir)
return;

/* Point at the slash at the end of ".../objects/pack/" */
dirlen = strlen(packdir) + 1;
strbuf_addstr(&buf, packtmp);
/* Hold the length of ".tmp-%d-pack-" */
prefixlen = buf.len - dirlen;

while ((e = readdir(dir))) {
if (strncmp(e->d_name, buf.buf + dirlen, prefixlen))
continue;
strbuf_setlen(&buf, dirlen);
strbuf_addstr(&buf, e->d_name);
unlink(buf.buf);
}
closedir(dir);
strbuf_release(&buf);
}

static void remove_pack_on_signal(int signo)
{
remove_temporary_files();
sigchain_pop(signo);
raise(signo);
}

/* /*
* Adds all packs hex strings to either fname_nonkept_list or * Adds all packs hex strings to either fname_nonkept_list or
* fname_kept_list based on whether each pack has a corresponding * fname_kept_list based on whether each pack has a corresponding
@ -247,11 +209,15 @@ static struct {
{".idx"}, {".idx"},
}; };


static unsigned populate_pack_exts(char *name) struct generated_pack_data {
struct tempfile *tempfiles[ARRAY_SIZE(exts)];
};

static struct generated_pack_data *populate_pack_exts(const char *name)
{ {
struct stat statbuf; struct stat statbuf;
struct strbuf path = STRBUF_INIT; struct strbuf path = STRBUF_INIT;
unsigned ret = 0; struct generated_pack_data *data = xcalloc(1, sizeof(*data));
int i; int i;


for (i = 0; i < ARRAY_SIZE(exts); i++) { for (i = 0; i < ARRAY_SIZE(exts); i++) {
@ -261,11 +227,11 @@ static unsigned populate_pack_exts(char *name)
if (stat(path.buf, &statbuf)) if (stat(path.buf, &statbuf))
continue; continue;


ret |= (1 << i); data->tempfiles[i] = register_tempfile(path.buf);
} }


strbuf_release(&path); strbuf_release(&path);
return ret; return data;
} }


static void repack_promisor_objects(const struct pack_objects_args *args, static void repack_promisor_objects(const struct pack_objects_args *args,
@ -320,7 +286,7 @@ static void repack_promisor_objects(const struct pack_objects_args *args,
line.buf); line.buf);
write_promisor_file(promisor_name, NULL, 0); write_promisor_file(promisor_name, NULL, 0);


item->util = (void *)(uintptr_t)populate_pack_exts(item->string); item->util = populate_pack_exts(item->string);


free(promisor_name); free(promisor_name);
} }
@ -739,10 +705,14 @@ static int write_cruft_pack(const struct pack_objects_args *args,


out = xfdopen(cmd.out, "r"); out = xfdopen(cmd.out, "r");
while (strbuf_getline_lf(&line, out) != EOF) { while (strbuf_getline_lf(&line, out) != EOF) {
struct string_list_item *item;

if (line.len != the_hash_algo->hexsz) if (line.len != the_hash_algo->hexsz)
die(_("repack: Expecting full hex object ID lines only " die(_("repack: Expecting full hex object ID lines only "
"from pack-objects.")); "from pack-objects."));
string_list_append(names, line.buf);
item = string_list_append(names, line.buf);
item->util = populate_pack_exts(line.buf);
} }
fclose(out); fclose(out);


@ -888,8 +858,6 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
split_pack_geometry(geometry, geometric_factor); split_pack_geometry(geometry, geometric_factor);
} }


sigchain_push_common(remove_pack_on_signal);

prepare_pack_objects(&cmd, &po_args); prepare_pack_objects(&cmd, &po_args);


show_progress = !po_args.quiet && isatty(2); show_progress = !po_args.quiet && isatty(2);
@ -981,9 +949,12 @@ int cmd_repack(int argc, const char **argv, const char *prefix)


out = xfdopen(cmd.out, "r"); out = xfdopen(cmd.out, "r");
while (strbuf_getline_lf(&line, out) != EOF) { while (strbuf_getline_lf(&line, out) != EOF) {
struct string_list_item *item;

if (line.len != the_hash_algo->hexsz) if (line.len != the_hash_algo->hexsz)
die(_("repack: Expecting full hex object ID lines only from pack-objects.")); die(_("repack: Expecting full hex object ID lines only from pack-objects."));
string_list_append(&names, line.buf); item = string_list_append(&names, line.buf);
item->util = populate_pack_exts(item->string);
} }
fclose(out); fclose(out);
ret = finish_command(&cmd); ret = finish_command(&cmd);
@ -1022,40 +993,38 @@ int cmd_repack(int argc, const char **argv, const char *prefix)


string_list_sort(&names); string_list_sort(&names);


for_each_string_list_item(item, &names) {
item->util = (void *)(uintptr_t)populate_pack_exts(item->string);
}

close_object_store(the_repository->objects); close_object_store(the_repository->objects);


/* /*
* Ok we have prepared all new packfiles. * Ok we have prepared all new packfiles.
*/ */
for_each_string_list_item(item, &names) { for_each_string_list_item(item, &names) {
struct generated_pack_data *data = item->util;

for (ext = 0; ext < ARRAY_SIZE(exts); ext++) { for (ext = 0; ext < ARRAY_SIZE(exts); ext++) {
char *fname, *fname_old; char *fname;


fname = mkpathdup("%s/pack-%s%s", fname = mkpathdup("%s/pack-%s%s",
packdir, item->string, exts[ext].name); packdir, item->string, exts[ext].name);
fname_old = mkpathdup("%s-%s%s",
packtmp, item->string, exts[ext].name);


if (((uintptr_t)item->util) & ((uintptr_t)1 << ext)) { if (data->tempfiles[ext]) {
const char *fname_old = get_tempfile_path(data->tempfiles[ext]);
struct stat statbuffer; struct stat statbuffer;

if (!stat(fname_old, &statbuffer)) { if (!stat(fname_old, &statbuffer)) {
statbuffer.st_mode &= ~(S_IWUSR | S_IWGRP | S_IWOTH); statbuffer.st_mode &= ~(S_IWUSR | S_IWGRP | S_IWOTH);
chmod(fname_old, statbuffer.st_mode); chmod(fname_old, statbuffer.st_mode);
} }


if (rename(fname_old, fname)) if (rename_tempfile(&data->tempfiles[ext], fname))
die_errno(_("renaming '%s' failed"), fname_old); die_errno(_("renaming pack to '%s' failed"), fname);
} else if (!exts[ext].optional) } else if (!exts[ext].optional)
die(_("missing required file: %s"), fname_old); die(_("pack-objects did not write a '%s' file for pack %s-%s"),
exts[ext].name, packtmp, item->string);
else if (unlink(fname) < 0 && errno != ENOENT) else if (unlink(fname) < 0 && errno != ENOENT)
die_errno(_("could not unlink: %s"), fname); die_errno(_("could not unlink: %s"), fname);


free(fname); free(fname);
free(fname_old);
} }
} }
/* End of pack replacement. */ /* End of pack replacement. */
@ -1143,7 +1112,6 @@ int cmd_repack(int argc, const char **argv, const char *prefix)


if (run_update_server_info) if (run_update_server_info)
update_server_info(0); update_server_info(0);
remove_temporary_files();


if (git_env_bool(GIT_TEST_MULTI_PACK_INDEX, 0)) { if (git_env_bool(GIT_TEST_MULTI_PACK_INDEX, 0)) {
unsigned flags = 0; unsigned flags = 0;
@ -1152,7 +1120,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
write_midx_file(get_object_directory(), NULL, NULL, flags); write_midx_file(get_object_directory(), NULL, NULL, flags);
} }


string_list_clear(&names, 0); string_list_clear(&names, 1);
string_list_clear(&existing_nonkept_packs, 0); string_list_clear(&existing_nonkept_packs, 0);
string_list_clear(&existing_kept_packs, 0); string_list_clear(&existing_kept_packs, 0);
clear_pack_geometry(geometry); clear_pack_geometry(geometry);

View File

@ -477,6 +477,22 @@ test_expect_success TTY '--quiet disables progress' '
test_must_be_empty stderr test_must_be_empty stderr
' '


test_expect_success 'clean up .tmp-* packs on error' '
test_must_fail ok=sigpipe git \
-c repack.cruftwindow=bogus \
repack -ad --cruft &&
find $objdir/pack -name '.tmp-*' >tmpfiles &&
test_must_be_empty tmpfiles
'

test_expect_success 'repack -ad cleans up old .tmp-* packs' '
git rev-parse HEAD >input &&
git pack-objects $objdir/pack/.tmp-1234 <input &&
git repack -ad &&
find $objdir/pack -name '.tmp-*' >tmpfiles &&
test_must_be_empty tmpfiles
'

test_expect_success 'setup for update-server-info' ' test_expect_success 'setup for update-server-info' '
git init update-server-info && git init update-server-info &&
test_commit -C update-server-info message test_commit -C update-server-info message