# [PATCH 0/2] don't add duplicate paths to info/alternates 5 messages from 2015-05-31 to 2015-06-01. Participants: Jim Hill, Jeff King. Thread: https://gitlist.dev/t/39468 ## Jim Hill, 2015-05-31 18:15 Subject: [PATCH 0/2] don't add duplicate paths to info/alternates Message-ID: <1433096123-14420-1-git-send-email-gjthill@gmail.com> URL: https://gitlist.dev/e/1433096123-14420-1-git-send-email-gjthill%40gmail.com ``` These patches address http://thread.gmane.org/gmane.comp.version-control.git/269050/focus=269415 linked from the git blame page, avoiding adding duplicates to info/alternates and removing hold_lock_file_for_append which is too heavyweight for logging and too limited for anything else. There's an argument to be made that since a-t-a-f is only used by clone, it shouldn't even bother taking a lock -- but then it should be moved to builtin/clone.c and reduced to a single write of the pre-deduped list, followed by a single read_info_alternates call. One thing at a time. Taking out the locking in the incremental version here doesn't really simplify the code much anyway. ``` ## Jim Hill, 2015-05-31 18:15 Subject: [PATCH 1/2] add_to_alternates_file: don't add duplicate paths Message-ID: <1433096123-14420-2-git-send-email-gjthill@gmail.com> URL: https://gitlist.dev/e/1433096123-14420-2-git-send-email-gjthill%40gmail.com In-Reply-To: <1433096123-14420-1-git-send-email-gjthill@gmail.com> ``` Check for an existing match before appending a path to the alternates file. Beyond making git look smart to anyone checking the alternates file, this removes the last use of hold_lock_file_for_append. Signed-off-by: Jim Hill --- sha1_file.c | 29 +++++++++++++++++++++++++---- t/t5700-clone-reference.sh | 4 ++-- 2 files changed, 27 insertions(+), 6 deletions(-) diff --git a/sha1_file.c b/sha1_file.c index 47f56f2..43d9530 100644 --- a/sha1_file.c +++ b/sha1_file.c @@ -403,14 +403,35 @@ void read_info_alternates(const char * relative_base, int depth) void add_to_alternates_file(const char *reference) { - struct lock_file *lock = xcalloc(1, sizeof(struct lock_file)); - int fd = hold_lock_file_for_append(lock, git_path("objects/info/alternates"), LOCK_DIE_ON_ERROR); + static struct lock_file lock = {0}; char *alt = mkpath("%s\n", reference); + char *alts = git_path("objects/info/alternates"); + int fd = hold_lock_file_for_update(&lock, alts, LOCK_DIE_ON_ERROR); + struct strbuf altdata = STRBUF_INIT; + struct string_list lines = STRING_LIST_INIT_NODUP; + + if (strbuf_read_file(&altdata, alts, 0) < 0) + if (errno != ENOENT) + die("alternates file unreadable"); + strbuf_complete_line(&altdata); + write_or_die(fd, altdata.buf, altdata.len); + + string_list_split_in_place(&lines, altdata.buf, '\n', -1); + lines.cmp = strcmp_icase; + if (unsorted_string_list_has_string(&lines, reference)) { + rollback_lock_file(&lock); + goto cleanup; + } + write_or_die(fd, alt, strlen(alt)); - if (commit_lock_file(lock)) - die("could not close alternates file"); + if (commit_lock_file(&lock)) + die("could not update alternates file"); if (alt_odb_tail) link_alt_odb_entries(alt, strlen(alt), '\n', NULL, 0); + +cleanup: + strbuf_reset(&altdata); + string_list_clear(&lines,0); } int foreach_alt_odb(alt_odb_fn fn, void *cb) diff --git a/t/t5700-clone-reference.sh b/t/t5700-clone-reference.sh index 3e783fc..cd9fa34 100755 --- a/t/t5700-clone-reference.sh +++ b/t/t5700-clone-reference.sh @@ -29,11 +29,11 @@ git prune' cd "$base_dir" test_expect_success 'cloning with reference (-l -s)' \ -'git clone -l -s --reference B A C' +'git clone -l -s --reference B --reference A --reference B A C' cd "$base_dir" -test_expect_success 'existence of info/alternates' \ +test_expect_success 'existence of info/alternates, no duplicates' \ 'test_line_count = 2 C/.git/objects/info/alternates' cd "$base_dir" -- 2.4.1.4.gfc728c2 ``` ## Jim Hill, 2015-05-31 18:15 Subject: [PATCH 2/2] remove hold_lock_file_for_append Message-ID: <1433096123-14420-3-git-send-email-gjthill@gmail.com> URL: https://gitlist.dev/e/1433096123-14420-3-git-send-email-gjthill%40gmail.com In-Reply-To: <1433096123-14420-1-git-send-email-gjthill@gmail.com> ``` No uses of hold_lock_file_for_append remain, so remove it. hold_lock_file_for_append copies its target file internally. This makes it too heavyweight for logging and too limited for anything else. It shouldn't be used. Signed-off-by: Jim Hill --- lockfile.c | 38 -------------------------------------- 1 file changed, 38 deletions(-) diff --git a/lockfile.c b/lockfile.c index 9889277..1467778 100644 --- a/lockfile.c +++ b/lockfile.c @@ -187,44 +187,6 @@ int hold_lock_file_for_update(struct lock_file *lk, const char *path, int flags) return fd; } -int hold_lock_file_for_append(struct lock_file *lk, const char *path, int flags) -{ - int fd, orig_fd; - - fd = lock_file(lk, path, flags); - if (fd < 0) { - if (flags & LOCK_DIE_ON_ERROR) - unable_to_lock_die(path, errno); - return fd; - } - - orig_fd = open(path, O_RDONLY); - if (orig_fd < 0) { - if (errno != ENOENT) { - int save_errno = errno; - - if (flags & LOCK_DIE_ON_ERROR) - die("cannot open '%s' for copying", path); - rollback_lock_file(lk); - error("cannot open '%s' for copying", path); - errno = save_errno; - return -1; - } - } else if (copy_fd(orig_fd, fd)) { - int save_errno = errno; - - if (flags & LOCK_DIE_ON_ERROR) - exit(128); - close(orig_fd); - rollback_lock_file(lk); - errno = save_errno; - return -1; - } else { - close(orig_fd); - } - return fd; -} - FILE *fdopen_lock_file(struct lock_file *lk, const char *mode) { if (!lk->active) -- 2.4.1.4.gfc728c2 ``` ## Jeff King, 2015-06-01 10:51 Subject: Re: [PATCH 1/2] add_to_alternates_file: don't add duplicate paths Message-ID: <20150601105142.GC31792@peff.net> URL: https://gitlist.dev/e/20150601105142.GC31792%40peff.net In-Reply-To: <1433096123-14420-2-git-send-email-gjthill@gmail.com> ``` On Sun, May 31, 2015 at 11:15:22AM -0700, Jim Hill wrote: > Check for an existing match before appending a path to the alternates > file. Beyond making git look smart to anyone checking the alternates > file, this removes the last use of hold_lock_file_for_append. Makes sense. We don't catch _all_ cases here (e.g., we do not bother to see if "foo" is a symlink to "bar"), but we at least catch the obvious ones. > void add_to_alternates_file(const char *reference) > { > - struct lock_file *lock = xcalloc(1, sizeof(struct lock_file)); > [...] > + static struct lock_file lock = {0}; This seems like an unrelated change. I don't mind it in general, but it should probably go in a separate patch. > - int fd = hold_lock_file_for_append(lock, git_path("objects/info/alternates"), LOCK_DIE_ON_ERROR); > char *alt = mkpath("%s\n", reference); > + char *alts = git_path("objects/info/alternates"); A minor nit, but I found "alts" and "alt" to be a little bit similar while reading. I wonder if "our_alts" or "alts_files" would be a little more clear. > + struct strbuf altdata = STRBUF_INIT; > + struct string_list lines = STRING_LIST_INIT_NODUP; > + > + if (strbuf_read_file(&altdata, alts, 0) < 0) > + if (errno != ENOENT) > + die("alternates file unreadable"); Hmm, so we read the whole content in, then split it into a string list of lines. Might it be simpler to just read it line by line and compare as we go? Like (totally untested); FILE *in, *out; ... out = fdopen_lock_file(&lock, "w"); if (!out) die_errno("unable to fdopen alternates lockfile"); in = fopen(alts, "r"); if (in) { struct strbuf line = STRBUF_INIT; int found = 0; while (strbuf_getline(&line, in, '\n') != EOF) { if (!strcmp(reference, line.buf)) { found = 1; break; } fprintf_or_die(out, "%s\n", line.buf); } strbuf_release(&line); fclose(in); if (found) { rollback_lock_file(&lock); return; } } else if (errno != ENOENT) die_errno("unable to read alternates file"); fprintf_or_die(out, "%s\n", reference); /* commit_lock_file, etc; it takes care of the fclose() */ Note that I also fdopen'd the output file, which makes the newline handling a little easier (I think you can even drop the "alt" variable). That's optional, and you could use strbuf_getwholeline to retain the original newlines. But... > + strbuf_complete_line(&altdata); This seems like a nice bugfix that you didn't mention. With the earlier code, if your alternates file was missing a trailing newline, we would produce a bogus output. So it makes sense to do (either this way, or the way I showed above). I think it probably goes in the same commit (it's part of the refactoring), but you may want to mention it in the commit message. > +cleanup: > + strbuf_reset(&altdata); I think you want strbuf_release() here (though it goes away if you follow my suggestion above). > diff --git a/t/t5700-clone-reference.sh b/t/t5700-clone-reference.sh > index 3e783fc..cd9fa34 100755 > --- a/t/t5700-clone-reference.sh > +++ b/t/t5700-clone-reference.sh > @@ -29,11 +29,11 @@ git prune' > cd "$base_dir" > > test_expect_success 'cloning with reference (-l -s)' \ > -'git clone -l -s --reference B A C' > +'git clone -l -s --reference B --reference A --reference B A C' > > cd "$base_dir" > > -test_expect_success 'existence of info/alternates' \ > +test_expect_success 'existence of info/alternates, no duplicates' \ > 'test_line_count = 2 C/.git/objects/info/alternates' Generally we prefer a new separate test rather than trying to take over an existing one (i.e., just add a new one with the clone and line-count test together). -Peff ``` ## Jeff King, 2015-06-01 10:53 Subject: Re: [PATCH 0/2] don't add duplicate paths to info/alternates Message-ID: <20150601105348.GD31792@peff.net> URL: https://gitlist.dev/e/20150601105348.GD31792%40peff.net In-Reply-To: <1433096123-14420-1-git-send-email-gjthill@gmail.com> ``` On Sun, May 31, 2015 at 11:15:21AM -0700, Jim Hill wrote: > These patches address > http://thread.gmane.org/gmane.comp.version-control.git/269050/focus=269415 > linked from the git blame page, avoiding adding duplicates to info/alternates > and removing hold_lock_file_for_append which is too heavyweight for logging and > too limited for anything else. Thanks. Mentioning that I would like something done and then having it unexpectedly arrive in my mailbox is like getting a present. :) I had a few comments on the first patch, and the second looks obviously correct. > There's an argument to be made that since a-t-a-f is only used by clone, it > shouldn't even bother taking a lock -- but then it should be moved to > builtin/clone.c and reduced to a single write of the pre-deduped list, followed > by a single read_info_alternates call. One thing at a time. Taking out the > locking in the incremental version here doesn't really simplify the code much > anyway. Yeah, I agree with all of the reasoning here. -Peff ```