Re: [PATCH 1/2] add_to_alternates_file: don't add duplicate paths
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.
Show 5 quoted lines
> 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.
Show 6 quoted lines
> + 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).
Show 16 quoted lines
> 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
Jim Hill