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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
