{"thread":{"id":"39468","subject":"[PATCH 0/2] don't add duplicate paths to info/alternates","startedAt":"2015-05-31T18:15:21Z","lastAt":"2015-06-01T10:53:48Z","messageCount":5,"participants":["Jim Hill","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"262522","messageId":"1433096123-14420-1-git-send-email-gjthill@gmail.com","threadId":"39468","inReplyTo":null,"subject":"[PATCH 0/2] don't add duplicate paths to info/alternates","fromName":"Jim Hill","fromEmail":"gjthill@gmail.com","sentAt":"2015-05-31T18:15:21Z","receivedAt":"2015-05-31T18:15:21Z","isPatch":true,"sender":{"key":"gjthill@gmail.com","avatar":"https://avatars.githubusercontent.com/u/80352?v=4"},"body":"These patches address\nhttp://thread.gmane.org/gmane.comp.version-control.git/269050/focus=269415\nlinked from the git blame page, avoiding adding duplicates to info/alternates\nand removing hold_lock_file_for_append which is too heavyweight for logging and\ntoo limited for anything else.\n\nThere's an argument to be made that since a-t-a-f is only used by clone, it\nshouldn't even bother taking a lock -- but then it should be moved to\nbuiltin/clone.c and reduced to a single write of the pre-deduped list, followed\nby a single read_info_alternates call. One thing at a time.  Taking out the\nlocking in the incremental version here doesn't really simplify the code much\nanyway.\n"},{"id":"262524","messageId":"1433096123-14420-2-git-send-email-gjthill@gmail.com","threadId":"39468","inReplyTo":"1433096123-14420-1-git-send-email-gjthill@gmail.com","subject":"[PATCH 1/2] add_to_alternates_file: don't add duplicate paths","fromName":"Jim Hill","fromEmail":"gjthill@gmail.com","sentAt":"2015-05-31T18:15:22Z","receivedAt":"2015-05-31T18:15:22Z","isPatch":true,"sender":{"key":"gjthill@gmail.com","avatar":"https://avatars.githubusercontent.com/u/80352?v=4"},"body":"Check for an existing match before appending a path to the alternates\nfile.  Beyond making git look smart to anyone checking the alternates\nfile, this removes the last use of hold_lock_file_for_append.\n\nSigned-off-by: Jim Hill <gjthill@gmail.com>\n---\n sha1_file.c                | 29 +++++++++++++++++++++++++----\n t/t5700-clone-reference.sh |  4 ++--\n 2 files changed, 27 insertions(+), 6 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 47f56f2..43d9530 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -403,14 +403,35 @@ void read_info_alternates(const char * relative_base, int depth)\n \n void add_to_alternates_file(const char *reference)\n {\n-\tstruct lock_file *lock = xcalloc(1, sizeof(struct lock_file));\n-\tint fd = hold_lock_file_for_append(lock, git_path(\"objects/info/alternates\"), LOCK_DIE_ON_ERROR);\n+\tstatic struct lock_file lock = {0};\n \tchar *alt = mkpath(\"%s\\n\", reference);\n+\tchar *alts = git_path(\"objects/info/alternates\");\n+\tint fd = hold_lock_file_for_update(&lock, alts, LOCK_DIE_ON_ERROR);\n+\tstruct strbuf altdata = STRBUF_INIT;\n+\tstruct string_list lines = STRING_LIST_INIT_NODUP;\n+\n+\tif (strbuf_read_file(&altdata, alts, 0) < 0)\n+\t\tif (errno != ENOENT)\n+\t\t\tdie(\"alternates file unreadable\");\n+\tstrbuf_complete_line(&altdata);\n+\twrite_or_die(fd, altdata.buf, altdata.len);\n+\n+\tstring_list_split_in_place(&lines, altdata.buf, '\\n', -1);\n+\tlines.cmp = strcmp_icase;\n+\tif (unsorted_string_list_has_string(&lines, reference)) {\n+\t\trollback_lock_file(&lock);\n+\t\tgoto cleanup;\n+\t}\n+\n \twrite_or_die(fd, alt, strlen(alt));\n-\tif (commit_lock_file(lock))\n-\t\tdie(\"could not close alternates file\");\n+\tif (commit_lock_file(&lock))\n+\t\tdie(\"could not update alternates file\");\n \tif (alt_odb_tail)\n \t\tlink_alt_odb_entries(alt, strlen(alt), '\\n', NULL, 0);\n+\n+cleanup:\n+\tstrbuf_reset(&altdata);\n+\tstring_list_clear(&lines,0);\n }\n \n int foreach_alt_odb(alt_odb_fn fn, void *cb)\ndiff --git a/t/t5700-clone-reference.sh b/t/t5700-clone-reference.sh\nindex 3e783fc..cd9fa34 100755\n--- a/t/t5700-clone-reference.sh\n+++ b/t/t5700-clone-reference.sh\n@@ -29,11 +29,11 @@ git prune'\n cd \"$base_dir\"\n \n test_expect_success 'cloning with reference (-l -s)' \\\n-'git clone -l -s --reference B A C'\n+'git clone -l -s --reference B --reference A --reference B A C'\n \n cd \"$base_dir\"\n \n-test_expect_success 'existence of info/alternates' \\\n+test_expect_success 'existence of info/alternates, no duplicates' \\\n 'test_line_count = 2 C/.git/objects/info/alternates'\n \n cd \"$base_dir\"\n-- \n2.4.1.4.gfc728c2\n"},{"id":"262523","messageId":"1433096123-14420-3-git-send-email-gjthill@gmail.com","threadId":"39468","inReplyTo":"1433096123-14420-1-git-send-email-gjthill@gmail.com","subject":"[PATCH 2/2] remove hold_lock_file_for_append","fromName":"Jim Hill","fromEmail":"gjthill@gmail.com","sentAt":"2015-05-31T18:15:23Z","receivedAt":"2015-05-31T18:15:23Z","isPatch":true,"sender":{"key":"gjthill@gmail.com","avatar":"https://avatars.githubusercontent.com/u/80352?v=4"},"body":"No uses of hold_lock_file_for_append remain, so remove it.\n\nhold_lock_file_for_append copies its target file internally.  This makes\nit too heavyweight for logging and too limited for anything else. It\nshouldn't be used.\n\nSigned-off-by: Jim Hill <gjthill@gmail.com>\n---\n lockfile.c | 38 --------------------------------------\n 1 file changed, 38 deletions(-)\n\ndiff --git a/lockfile.c b/lockfile.c\nindex 9889277..1467778 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -187,44 +187,6 @@ int hold_lock_file_for_update(struct lock_file *lk, const char *path, int flags)\n \treturn fd;\n }\n \n-int hold_lock_file_for_append(struct lock_file *lk, const char *path, int flags)\n-{\n-\tint fd, orig_fd;\n-\n-\tfd = lock_file(lk, path, flags);\n-\tif (fd < 0) {\n-\t\tif (flags & LOCK_DIE_ON_ERROR)\n-\t\t\tunable_to_lock_die(path, errno);\n-\t\treturn fd;\n-\t}\n-\n-\torig_fd = open(path, O_RDONLY);\n-\tif (orig_fd < 0) {\n-\t\tif (errno != ENOENT) {\n-\t\t\tint save_errno = errno;\n-\n-\t\t\tif (flags & LOCK_DIE_ON_ERROR)\n-\t\t\t\tdie(\"cannot open '%s' for copying\", path);\n-\t\t\trollback_lock_file(lk);\n-\t\t\terror(\"cannot open '%s' for copying\", path);\n-\t\t\terrno = save_errno;\n-\t\t\treturn -1;\n-\t\t}\n-\t} else if (copy_fd(orig_fd, fd)) {\n-\t\tint save_errno = errno;\n-\n-\t\tif (flags & LOCK_DIE_ON_ERROR)\n-\t\t\texit(128);\n-\t\tclose(orig_fd);\n-\t\trollback_lock_file(lk);\n-\t\terrno = save_errno;\n-\t\treturn -1;\n-\t} else {\n-\t\tclose(orig_fd);\n-\t}\n-\treturn fd;\n-}\n-\n FILE *fdopen_lock_file(struct lock_file *lk, const char *mode)\n {\n \tif (!lk->active)\n-- \n2.4.1.4.gfc728c2\n"},{"id":"262578","messageId":"20150601105142.GC31792@peff.net","threadId":"39468","inReplyTo":"1433096123-14420-2-git-send-email-gjthill@gmail.com","subject":"Re: [PATCH 1/2] add_to_alternates_file: don't add duplicate paths","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-06-01T10:51:42Z","receivedAt":"2015-06-01T10:51:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, May 31, 2015 at 11:15:22AM -0700, Jim Hill wrote:\n\n> Check for an existing match before appending a path to the alternates\n> file.  Beyond making git look smart to anyone checking the alternates\n> file, this removes the last use of hold_lock_file_for_append.\n\nMakes sense. We don't catch _all_ cases here (e.g., we do not bother to\nsee if \"foo\" is a symlink to \"bar\"), but we at least catch the obvious\nones.\n\n>  void add_to_alternates_file(const char *reference)\n>  {\n> -\tstruct lock_file *lock = xcalloc(1, sizeof(struct lock_file));\n> [...]\n> +\tstatic struct lock_file lock = {0};\n\nThis seems like an unrelated change. I don't mind it in general, but it\nshould probably go in a separate patch.\n\n> -\tint fd = hold_lock_file_for_append(lock, git_path(\"objects/info/alternates\"), LOCK_DIE_ON_ERROR);\n>  \tchar *alt = mkpath(\"%s\\n\", reference);\n> +\tchar *alts = git_path(\"objects/info/alternates\");\n\nA minor nit, but I found \"alts\" and \"alt\" to be a little bit similar\nwhile reading. I wonder if \"our_alts\" or \"alts_files\" would be a little\nmore clear.\n\n> +\tstruct strbuf altdata = STRBUF_INIT;\n> +\tstruct string_list lines = STRING_LIST_INIT_NODUP;\n> +\n> +\tif (strbuf_read_file(&altdata, alts, 0) < 0)\n> +\t\tif (errno != ENOENT)\n> +\t\t\tdie(\"alternates file unreadable\");\n\nHmm, so we read the whole content in, then split it into a string list\nof lines.  Might it be simpler to just read it line by line and compare\nas we go? Like (totally untested);\n\n  FILE *in, *out;\n  ...\n\n  out = fdopen_lock_file(&lock, \"w\");\n  if (!out)\n\tdie_errno(\"unable to fdopen alternates lockfile\");\n\n  in = fopen(alts, \"r\");\n  if (in) {\n\tstruct strbuf line = STRBUF_INIT;\n\tint found = 0;\n\n\twhile (strbuf_getline(&line, in, '\\n') != EOF) {\n\t\tif (!strcmp(reference, line.buf)) {\n\t\t\tfound = 1;\n\t\t\tbreak;\n\t\t}\n\t\tfprintf_or_die(out, \"%s\\n\", line.buf);\n\t}\n\n\tstrbuf_release(&line);\n\tfclose(in);\n\n\tif (found) {\n\t\trollback_lock_file(&lock);\n\t\treturn;\n\t}\n\n  }\n  else if (errno != ENOENT)\n\t  die_errno(\"unable to read alternates file\");\n\n  fprintf_or_die(out, \"%s\\n\", reference);\n  /* commit_lock_file, etc; it takes care of the fclose() */\n\n\nNote that I also fdopen'd the output file, which makes the newline\nhandling a little easier (I think you can even drop the \"alt\" variable).\nThat's optional, and you could use strbuf_getwholeline to retain the\noriginal newlines. But...\n\n> +\tstrbuf_complete_line(&altdata);\n\nThis seems like a nice bugfix that you didn't mention. With the earlier\ncode, if your alternates file was missing a trailing newline, we would\nproduce a bogus output. So it makes sense to do (either this way, or the\nway I showed above). I think it probably goes in the same commit (it's\npart of the refactoring), but you may want to mention it in the commit\nmessage.\n\n> +cleanup:\n> +\tstrbuf_reset(&altdata);\n\nI think you want strbuf_release() here (though it goes away if you\nfollow my suggestion above).\n\n> diff --git a/t/t5700-clone-reference.sh b/t/t5700-clone-reference.sh\n> index 3e783fc..cd9fa34 100755\n> --- a/t/t5700-clone-reference.sh\n> +++ b/t/t5700-clone-reference.sh\n> @@ -29,11 +29,11 @@ git prune'\n>  cd \"$base_dir\"\n>  \n>  test_expect_success 'cloning with reference (-l -s)' \\\n> -'git clone -l -s --reference B A C'\n> +'git clone -l -s --reference B --reference A --reference B A C'\n>  \n>  cd \"$base_dir\"\n>  \n> -test_expect_success 'existence of info/alternates' \\\n> +test_expect_success 'existence of info/alternates, no duplicates' \\\n>  'test_line_count = 2 C/.git/objects/info/alternates'\n\nGenerally we prefer a new separate test rather than trying to take over\nan existing one (i.e., just add a new one with the clone and line-count\ntest together).\n\n-Peff\n"},{"id":"262579","messageId":"20150601105348.GD31792@peff.net","threadId":"39468","inReplyTo":"1433096123-14420-1-git-send-email-gjthill@gmail.com","subject":"Re: [PATCH 0/2] don't add duplicate paths to info/alternates","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-06-01T10:53:48Z","receivedAt":"2015-06-01T10:53:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, May 31, 2015 at 11:15:21AM -0700, Jim Hill wrote:\n\n> These patches address\n> http://thread.gmane.org/gmane.comp.version-control.git/269050/focus=269415\n> linked from the git blame page, avoiding adding duplicates to info/alternates\n> and removing hold_lock_file_for_append which is too heavyweight for logging and\n> too limited for anything else.\n\nThanks. Mentioning that I would like something done and then having it\nunexpectedly arrive in my mailbox is like getting a present. :)\n\nI had a few comments on the first patch, and the second looks obviously\ncorrect.\n\n> There's an argument to be made that since a-t-a-f is only used by clone, it\n> shouldn't even bother taking a lock -- but then it should be moved to\n> builtin/clone.c and reduced to a single write of the pre-deduped list, followed\n> by a single read_info_alternates call. One thing at a time.  Taking out the\n> locking in the incremental version here doesn't really simplify the code much\n> anyway.\n\nYeah, I agree with all of the reasoning here.\n\n-Peff\n"}]}