[PATCH 0/2] don't add duplicate paths to info/alternates

[PATCH 0/2] don't add duplicate paths to info/alternates

[Jim Hill]

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.

[PATCH 1/2] add_to_alternates_file: don't add duplicate paths

[Jim Hill]

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 <gjthill@gmail.com>
---
 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

[PATCH 2/2] remove hold_lock_file_for_append

[Jim Hill]

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 <gjthill@gmail.com>
---
 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

Re: [PATCH 1/2] add_to_alternates_file: don't add duplicate paths

[Jeff King]

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

Re: [PATCH 0/2] don't add duplicate paths to info/alternates

[Jeff King]

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