add_to_alternates_file: don't add duplicate entries

The add_to_alternates_file function blindly uses hold_lock_file_for_append to copy the existing contents, and then adds the new line to it. This has two minor problems: 1. We might add duplicate entries, which are ugly and inefficient. 2. We do not check that the file ends with a newline, in which case we would bogusly append to the final line. This is quite unlikely in practice, though, as we call this function only from git-clone, so presumably we are the only writers of the file (and we always add a newline). Instead of using hold_lock_file_for_append, let's copy the file line by line, which ensures all records are properly terminated. If we see an extra line, we can simply abort the update (there is no point in even copying the rest, as we know that it would be identical to the original). As a bonus, we also get rid of some calls to the static-buffer mkpath and git_path functions. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Jeff King committed Aug 10, 2015 at 05:34 UTC 77b9b1d13ac9e6b78ba676d4edb221b7d2273c62
2 files changed +45 -7
sha1_file.c
+40 -7
@@ -404,13 +404,46 @@ void read_info_alternates(const char * relative_base, int depth)
404 void add_to_alternates_file(const char *reference)
405 {
406 struct lock_file *lock = xcalloc(1, sizeof(struct lock_file));
407 - int fd = hold_lock_file_for_append(lock, git_path("objects/info/alternates"), LOCK_DIE_ON_ERROR);
408 - const char *alt = mkpath("%s\n", reference);
409 - write_or_die(fd, alt, strlen(alt));
410 - if (commit_lock_file(lock))
411 - die("could not close alternates file");
412 - if (alt_odb_tail)
413 - link_alt_odb_entries(alt, strlen(alt), '\n', NULL, 0);
407 + char *alts = git_pathdup("objects/info/alternates");
408 + FILE *in, *out;
409 +
410 + hold_lock_file_for_update(lock, alts, LOCK_DIE_ON_ERROR);
411 + out = fdopen_lock_file(lock, "w");
412 + if (!out)
413 + die_errno("unable to fdopen alternates lockfile");
414 +
415 + in = fopen(alts, "r");
416 + if (in) {
417 + struct strbuf line = STRBUF_INIT;
418 + int found = 0;
419 +
420 + while (strbuf_getline(&line, in, '\n') != EOF) {
421 + if (!strcmp(reference, line.buf)) {
422 + found = 1;
423 + break;
424 + }
425 + fprintf_or_die(out, "%s\n", line.buf);
426 + }
427 +
428 + strbuf_release(&line);
429 + fclose(in);
430 +
431 + if (found) {
432 + rollback_lock_file(lock);
433 + lock = NULL;
434 + }
435 + }
436 + else if (errno != ENOENT)
437 + die_errno("unable to read alternates file");
438 +
439 + if (lock) {
440 + fprintf_or_die(out, "%s\n", reference);
441 + if (commit_lock_file(lock))
442 + die_errno("unable to move new alternates file into place");
443 + if (alt_odb_tail)
444 + link_alt_odb_entries(reference, strlen(reference), '\n', NULL, 0);
445 + }
446 + free(alts);
447 }
448
449 int foreach_alt_odb(alt_odb_fn fn, void *cb)
t/t5700-clone-reference.sh
+5
@@ -120,6 +120,11 @@ test_expect_success 'cloning with reference being subset of source (-l -s)' '
120 git clone -l -s --reference A B E
121 '
122
123 +test_expect_success 'cloning with multiple references drops duplicates' '
124 + git clone -s --reference B --reference A --reference B A dups &&
125 + test_line_count = 2 dups/.git/objects/info/alternates
126 +'
127 +
128 test_expect_success 'clone with reference from a tagged repository' '
129 (
130 cd A && git tag -a -m tagged HEAD