{"thread":{"id":"44796","subject":"[PATCH v3 01/23] files_rename_ref(): tidy up whitespace","startedAt":"2016-12-31T03:13:21Z","lastAt":"2017-01-02T18:55:55Z","messageCount":55,"participants":["Michael Haggerty","Jeff King","Junio C Hamano","Jacob Keller","Philip Oakley"],"isPatch":true,"patchVersion":3,"patchTotal":23},"messages":[{"id":"308514","messageId":"129f09835a10512ad532ff9521a1b279994324ff.1483153436.git.mhagger@alum.mit.edu","threadId":"44796","inReplyTo":"cover.1483153436.git.mhagger@alum.mit.edu","subject":"[PATCH v3 01/23] files_rename_ref(): tidy up whitespace","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-12-31T03:12:41Z","receivedAt":"2016-12-31T03:13:21Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Signed-off-by: Michael Haggerty \n---\n refs/files-backend.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex f902393..be4ffdc 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2631,7 +2631,7 @@ static int files_rename_ref(struct ref_store *ref_store,\n \tif (!read_ref_full(newrefname, RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,\n \t\t\t sha1, NULL) &&\n \t delete_ref(newrefname, NULL, REF_NODEREF)) {\n-\t\tif (errno==EISDIR) {\n+\t\tif (errno == EISDIR) {\n \t\t\tstruct strbuf path = STRBUF_INIT;\n \t\t\tint result;\n \n-- \n2.9.3\n\n"},{"id":"308515","messageId":"ee090187391ea0c7b39514698be8a7ed35a0c885.1483153436.git.mhagger@alum.mit.edu","threadId":"44796","inReplyTo":"cover.1483153436.git.mhagger@alum.mit.edu","subject":"[PATCH v3 02/23] t5505: use \"for-each-ref\" to test for the non-existence of references","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-12-31T03:12:42Z","receivedAt":"2016-12-31T03:13:26Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Instead of looking on the filesystem inside \".git/refs/remotes/origin\",\nuse \"git for-each-ref\" to check for leftover references under the\nremote's old name.\n\nSigned-off-by: Michael Haggerty \n---\n t/t5505-remote.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t5505-remote.sh b/t/t5505-remote.sh\nindex 8198d8e..65030fb 100755\n--- a/t/t5505-remote.sh\n+++ b/t/t5505-remote.sh\n@@ -725,7 +725,7 @@ test_expect_success 'rename a remote' '\n \t(\n \t\tcd four &&\n \t\tgit remote rename origin upstream &&\n-\t\trmdir .git/refs/remotes/origin &&\n+\t\ttest -z \"$(git for-each-ref refs/remotes/origin)\" &&\n \t\ttest \"$(git symbolic-ref refs/remotes/upstream/HEAD)\" = \"refs/remotes/upstream/master\" &&\n \t\ttest \"$(git rev-parse upstream/master)\" = \"$(git rev-parse master)\" &&\n \t\ttest \"$(git config remote.upstream.fetch)\" = \"+refs/heads/*:refs/remotes/upstream/*\" &&\n-- \n2.9.3\n\n"},{"id":"308516","messageId":"cover.1483153436.git.mhagger@alum.mit.edu","threadId":"44796","inReplyTo":null,"subject":"[PATCH v3 00/23] Delete directories left empty after ref deletion","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-12-31T03:12:40Z","receivedAt":"2016-12-31T03:13:28Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"This is a re-roll of an old patch series. v1 [1] got some feedback,\nwhich I think was all addressed in v2 [2]. But it seems that v2 fell\non the floor, and I didn't bother following up because it was in the\nsame area of code that was undergoing heavy changes due to the\npluggable reference backend work. Sorry for the long delay before\ngetting back to it.\n\nIt turns out that this patch series is still relevant and didn't even\nneed all that must adjustment. While rebasing onto the current\n`master`, I tidied up a bit, tightened up some code, and improved some\ncommit messages. But the spirit of the patch series and most of its\ncode are unchanged.\n\nThis patch series is also available from my GitHub account [3] as\nbranch delete-empty-refs-dirs.\n\nBrief summary (see v1 [1] for more details):\n\nPreviously, we were pretty sloppy about leaving empty directories\nbehind (under both $GIT_DIR/refs and $GIT_DIR/logs) when deleting\nreferences. Such directories could accumulate essentially forever.\nIt's true that `pack-refs` deletes directories that it empties, but if\na directory is *already* empty, then `pack-refs` doesn't remove it. It\nis also true that if an empty directory gets in the way of the\ncreation of a *new* reference, then it is deleted. But otherwise there\nis no systematic cleanup of empty directories.\n\nA reason for the old behavior was that the code paths *creating* new\nfiles in these hierarchies were not always robust against a race with\nanother process that was cleaning up empty directories. So most of\nthis patch series is dedicated to hardening up the creation code paths\n(via a new function, `raceproof_create_file()`). The last several\npatches teach `files_transaction_commit()` to delete empty directories\nwhen deleting references and reflogs.\n\nMichael\n\n[1] http://public-inbox.org/git/cover.1455626201.git.mhagger@alum.mit.edu/T/#u\n[2] http://public-inbox.org/git/cover.1456405698.git.mhagger@alum.mit.edu/T/#u\n[3] http://github.com/mhagger/git\n\nMichael Haggerty (23):\n files_rename_ref(): tidy up whitespace\n t5505: use \"for-each-ref\" to test for the non-existence of references\n safe_create_leading_directories_const(): preserve errno\n safe_create_leading_directories(): set errno on SCLD_EXISTS\n raceproof_create_file(): new function\n lock_ref_sha1_basic(): inline constant\n lock_ref_sha1_basic(): use raceproof_create_file()\n rename_tmp_log(): use raceproof_create_file()\n rename_tmp_log(): improve error reporting\n log_ref_write(): inline function\n log_ref_setup(): separate code for create vs non-create\n log_ref_setup(): improve robustness against races\n log_ref_setup(): pass the open file descriptor back to the caller\n log_ref_write_1(): don't depend on logfile argument\n log_ref_setup(): manage the name of the reflog file internally\n log_ref_write_1(): inline function\n delete_ref_loose(): derive loose reference path from lock\n delete_ref_loose(): inline function\n try_remove_empty_parents(): rename parameter \"name\" -> \"refname\"\n try_remove_empty_parents(): don't trash argument contents\n try_remove_empty_parents(): don't accommodate consecutive slashes\n try_remove_empty_parents(): teach to remove parents of reflogs, too\n files_transaction_commit(): clean up empty directories\n\n cache.h | 48 ++++++-\n refs/files-backend.c | 375 +++++++++++++++++++++++++-------------------------\n refs/refs-internal.h | 11 +-\n sha1_file.c | 76 +++++++++-\n t/t1400-update-ref.sh | 27 ++++\n t/t5505-remote.sh | 2 +-\n 6 files changed, 346 insertions(+), 193 deletions(-)\n\n-- \n2.9.3\n\n"},{"id":"308517","messageId":"2d685f4c5cbe17bf1abf2ef3fb2f03fd20a0c23d.1483153436.git.mhagger@alum.mit.edu","threadId":"44796","inReplyTo":"cover.1483153436.git.mhagger@alum.mit.edu","subject":"[PATCH v3 03/23] safe_create_leading_directories_const(): preserve errno","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-12-31T03:12:43Z","receivedAt":"2016-12-31T03:13:29Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Some implementations of free() change errno (even thought they\nshouldn't):\n\n https://sourceware.org/bugzilla/show_bug.cgi?id=17924\n\nSo preserve the errno from safe_create_leading_directories() across the\ncall to free().\n\nSigned-off-by: Michael Haggerty \n---\n sha1_file.c | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 1173071..10395e7 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -166,10 +166,14 @@ enum scld_error safe_create_leading_directories(char *path)\n \n enum scld_error safe_create_leading_directories_const(const char *path)\n {\n+\tint save_errno;\n \t/* path points to cache entries, so xstrdup before messing with it */\n \tchar *buf = xstrdup(path);\n \tenum scld_error result = safe_create_leading_directories(buf);\n+\n+\tsave_errno = errno;\n \tfree(buf);\n+\terrno = save_errno;\n \treturn result;\n }\n \n-- \n2.9.3\n\n"},{"id":"308518","messageId":"f933f9d3c4c53b42ecc75b7a743ed4bfd390b4c5.1483153436.git.mhagger@alum.mit.edu","threadId":"44796","inReplyTo":"cover.1483153436.git.mhagger@alum.mit.edu","subject":"[PATCH v3 05/23] raceproof_create_file(): new function","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-12-31T03:12:45Z","receivedAt":"2016-12-31T03:13:31Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Add a function that tries to create a file and any containing\ndirectories in a way that is robust against races with other processes\nthat might be cleaning up empty directories at the same time.\n\nThe actual file creation is done by a callback function, which, if it\nfails, should set errno to EISDIR or ENOENT according to the convention\nof open(). raceproof_create_file() detects such failures, and\nrespectively either tries to delete empty directories that might be in\nthe way of the file or tries to create the containing directories. Then\nit retries the callback function.\n\nThis function is not yet used.\n\nSigned-off-by: Michael Haggerty \n---\n cache.h | 43 ++++++++++++++++++++++++++++++++++++++\n sha1_file.c | 68 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 111 insertions(+)\n\ndiff --git a/cache.h b/cache.h\nindex 8177c3a..7bc4ea6 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1057,6 +1057,49 @@ enum scld_error {\n enum scld_error safe_create_leading_directories(char *path);\n enum scld_error safe_create_leading_directories_const(const char *path);\n \n+/*\n+ * Callback function for raceproof_create_file(). This function is\n+ * expected to do something that makes dirname(path) permanent despite\n+ * the fact that other processes might be cleaning up empty\n+ * directories at the same time. Usually it will create a file named\n+ * path, but alternatively it could create another file in that\n+ * directory, or even chdir() into that directory. The function should\n+ * return 0 if the action was completed successfully. On error, it\n+ * should return a nonzero result and set errno.\n+ * raceproof_create_file() treats two errno values specially:\n+ *\n+ * - ENOENT -- dirname(path) does not exist. In this case,\n+ * raceproof_create_file() tries creating dirname(path)\n+ * (and any parent directories, if necessary) and calls\n+ * the function again.\n+ *\n+ * - EISDIR -- the file already exists and is a directory. In this\n+ * case, raceproof_create_file() deletes the directory\n+ * (recursively) if it is empty and calls the function\n+ * again.\n+ *\n+ * Any other errno causes raceproof_create_file() to fail with the\n+ * callback's return value and errno.\n+ *\n+ * Obviously, this function should be OK with being called again if it\n+ * fails with ENOENT or EISDIR. In other scenarios it will not be\n+ * called again.\n+ */\n+typedef int create_file_fn(const char *path, void *cb);\n+\n+/*\n+ * Create a file in dirname(path) by calling fn, creating leading\n+ * directories if necessary. Retry a few times in case we are racing\n+ * with another process that is trying to clean up the directory that\n+ * contains path. See the documentation for create_file_fn for more\n+ * details.\n+ *\n+ * Return the value and set the errno that resulted from the most\n+ * recent call of fn. fn is always called at least once, and will be\n+ * called more than once if it returns ENOENT or EISDIR.\n+ */\n+int raceproof_create_file(const char *path, create_file_fn fn, void *cb);\n+\n int mkdir_in_gitdir(const char *path);\n extern char *expand_user_path(const char *path);\n const char *enter_repo(const char *path, int strict);\ndiff --git a/sha1_file.c b/sha1_file.c\nindex ae8f0b4..b08f54c 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -179,6 +179,74 @@ enum scld_error safe_create_leading_directories_const(const char *path)\n \treturn result;\n }\n \n+int raceproof_create_file(const char *path, create_file_fn fn, void *cb)\n+{\n+\t/*\n+\t * The number of times we will try to remove empty directories\n+\t * in the way of path. This is only 1 because if another\n+\t * process is racily creating directories that conflict with\n+\t * us, we don't want to fight against them.\n+\t */\n+\tint remove_directories_remaining = 1;\n+\n+\t/*\n+\t * The number of times that we will try to create the\n+\t * directories containing path. We are willing to attempt this\n+\t * more than once, because another process could be trying to\n+\t * clean up empty directories at the same time as we are\n+\t * trying to create them.\n+\t */\n+\tint create_directories_remaining = 3;\n+\n+\t/* A scratch copy of path, filled lazily if we need it: */\n+\tstruct strbuf path_copy = STRBUF_INIT;\n+\n+\tint ret, save_errno;\n+\n+\t/* Sanity check: */\n+\tassert(*path);\n+\n+retry_fn:\n+\tret = fn(path, cb);\n+\tsave_errno = errno;\n+\tif (!ret)\n+\t\tgoto out;\n+\n+\tif (errno == EISDIR && remove_directories_remaining-- > 0) {\n+\t\t/*\n+\t\t * A directory is in the way. Maybe it is empty; try\n+\t\t * to remove it:\n+\t\t */\n+\t\tif (!path_copy.len)\n+\t\t\tstrbuf_addstr(&path_copy, path);\n+\n+\t\tif (!remove_dir_recursively(&path_copy, REMOVE_DIR_EMPTY_ONLY))\n+\t\t\tgoto retry_fn;\n+\t} else if (errno == ENOENT && create_directories_remaining-- > 0) {\n+\t\t/*\n+\t\t * Maybe the containing directory didn't exist, or\n+\t\t * maybe it was just deleted by a process that is\n+\t\t * racing with us to clean up empty directories. Try\n+\t\t * to create it:\n+\t\t */\n+\t\tenum scld_error scld_result;\n+\n+\t\tif (!path_copy.len)\n+\t\t\tstrbuf_addstr(&path_copy, path);\n+\n+\t\tdo {\n+\t\t\tscld_result = safe_create_leading_directories(path_copy.buf);\n+\t\t\tif (scld_result == SCLD_OK)\n+\t\t\t\tgoto retry_fn;\n+\t\t} while (scld_result == SCLD_VANISHED && create_directories_remaining-- > 0);\n+\t}\n+\n+out:\n+\tstrbuf_release(&path_copy);\n+\terrno = save_errno;\n+\treturn ret;\n+}\n+\n static void fill_sha1_path(struct strbuf *buf, const unsigned char *sha1)\n {\n \tint i;\n-- \n2.9.3\n\n"},{"id":"308519","messageId":"811abb3cd507ed7bc0a288bebbf7f0e4d9a2cd37.1483153436.git.mhagger@alum.mit.edu","threadId":"44796","inReplyTo":"cover.1483153436.git.mhagger@alum.mit.edu","subject":"[PATCH v3 06/23] lock_ref_sha1_basic(): inline constant","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-12-31T03:12:46Z","receivedAt":"2016-12-31T03:13:35Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"`lflags` is set a single time then never changed, so just inline it.\n\nSigned-off-by: Michael Haggerty \n---\n refs/files-backend.c | 3 +--\n 1 file changed, 1 insertion(+), 2 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex be4ffdc..7d4658a 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2000,7 +2000,6 @@ static struct ref_lock *lock_ref_sha1_basic(struct files_ref_store *refs,\n \tstruct strbuf ref_file = STRBUF_INIT;\n \tstruct ref_lock *lock;\n \tint last_errno = 0;\n-\tint lflags = LOCK_NO_DEREF;\n \tint mustexist = (old_sha1 && !is_null_sha1(old_sha1));\n \tint resolve_flags = RESOLVE_REF_NO_RECURSE;\n \tint attempts_remaining = 3;\n@@ -2083,7 +2082,7 @@ static struct ref_lock *lock_ref_sha1_basic(struct files_ref_store *refs,\n \t\tgoto error_return;\n \t}\n \n-\tif (hold_lock_file_for_update(lock->lk, ref_file.buf, lflags) < 0) {\n+\tif (hold_lock_file_for_update(lock->lk, ref_file.buf, LOCK_NO_DEREF) < 0) {\n \t\tlast_errno = errno;\n \t\tif (errno == ENOENT && --attempts_remaining > 0)\n \t\t\t/*\n-- \n2.9.3\n\n"},{"id":"308520","messageId":"0145c9444cf7eb9e9897e420c9b38ddbe76d4562.1483153436.git.mhagger@alum.mit.edu","threadId":"44796","inReplyTo":"cover.1483153436.git.mhagger@alum.mit.edu","subject":"[PATCH v3 07/23] lock_ref_sha1_basic(): use raceproof_create_file()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-12-31T03:12:47Z","receivedAt":"2016-12-31T03:13:38Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Instead of coding the retry loop inline, use raceproof_create_file() to\nmake lock acquisition safe against directory creation/deletion races.\n\nSigned-off-by: Michael Haggerty \n---\n refs/files-backend.c | 35 +++++++++--------------------------\n 1 file changed, 9 insertions(+), 26 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 7d4658a..74de289 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -1985,6 +1985,13 @@ static int remove_empty_directories(struct strbuf *path)\n \treturn remove_dir_recursively(path, REMOVE_DIR_EMPTY_ONLY);\n }\n \n+static int create_reflock(const char *path, void *cb)\n+{\n+\tstruct lock_file *lk = cb;\n+\n+\treturn hold_lock_file_for_update(lk, path, LOCK_NO_DEREF) < 0 ? -1 : 0;\n+}\n+\n /*\n * Locks a ref returning the lock on success and NULL on failure.\n * On failure errno is set to something meaningful.\n@@ -2002,7 +2009,6 @@ static struct ref_lock *lock_ref_sha1_basic(struct files_ref_store *refs,\n \tint last_errno = 0;\n \tint mustexist = (old_sha1 && !is_null_sha1(old_sha1));\n \tint resolve_flags = RESOLVE_REF_NO_RECURSE;\n-\tint attempts_remaining = 3;\n \tint resolved;\n \n \tassert_main_repository(&refs->base, \"lock_ref_sha1_basic\");\n@@ -2067,35 +2073,12 @@ static struct ref_lock *lock_ref_sha1_basic(struct files_ref_store *refs,\n \n \tlock->ref_name = xstrdup(refname);\n \n- retry:\n-\tswitch (safe_create_leading_directories_const(ref_file.buf)) {\n-\tcase SCLD_OK:\n-\t\tbreak; /* success */\n-\tcase SCLD_VANISHED:\n-\t\tif (--attempts_remaining > 0)\n-\t\t\tgoto retry;\n-\t\t/* fall through */\n-\tdefault:\n+\tif (raceproof_create_file(ref_file.buf, create_reflock, lock->lk)) {\n \t\tlast_errno = errno;\n-\t\tstrbuf_addf(err, \"unable to create directory for '%s'\",\n-\t\t\t ref_file.buf);\n+\t\tunable_to_lock_message(ref_file.buf, errno, err);\n \t\tgoto error_return;\n \t}\n \n-\tif (hold_lock_file_for_update(lock->lk, ref_file.buf, LOCK_NO_DEREF) < 0) {\n-\t\tlast_errno = errno;\n-\t\tif (errno == ENOENT && --attempts_remaining > 0)\n-\t\t\t/*\n-\t\t\t * Maybe somebody just deleted one of the\n-\t\t\t * directories leading to ref_file. Try\n-\t\t\t * again:\n-\t\t\t */\n-\t\t\tgoto retry;\n-\t\telse {\n-\t\t\tunable_to_lock_message(ref_file.buf, errno, err);\n-\t\t\tgoto error_return;\n-\t\t}\n-\t}\n \tif (verify_lock(lock, old_sha1, mustexist, err)) {\n \t\tlast_errno = errno;\n \t\tgoto error_return;\n-- \n2.9.3\n\n"},{"id":"308521","messageId":"ef2355e9d5ccaa53928c821530bae59f2b118013.1483153436.git.mhagger@alum.mit.edu","threadId":"44796","inReplyTo":"cover.1483153436.git.mhagger@alum.mit.edu","subject":"[PATCH v3 13/23] log_ref_setup(): pass the open file descriptor back to the caller","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-12-31T03:12:53Z","receivedAt":"2016-12-31T03:13:39Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"This function will most often be called by log_ref_write_1(), which\nwants to append to the reflog file. In that case, it is silly to close\nthe file only for the caller to reopen it immediately. So, in the case\nthat the file was opened, pass the open file descriptor back to the\ncaller.\n\nSigned-off-by: Michael Haggerty \n---\n refs/files-backend.c | 39 ++++++++++++++++++++++-----------------\n 1 file changed, 22 insertions(+), 17 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 345f7c3..7f26cf8 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2719,19 +2719,23 @@ static int open_or_create_logfile(const char *path, void *cb)\n }\n \n /*\n- * Create a reflog for a ref. If force_create = 0, the reflog will\n- * only be created for certain refs (those for which\n- * should_autocreate_reflog returns non-zero. Otherwise, create it\n- * regardless of the ref name. Fill in *err and return -1 on failure.\n+ * Create a reflog for a ref. Store its path to *logfile. If\n+ * force_create = 0, only create the reflog for certain refs (those\n+ * for which should_autocreate_reflog returns non-zero). Otherwise,\n+ * create it regardless of the reference name. If the logfile already\n+ * existed or was created, return 0 and set *logfd to the file\n+ * descriptor opened for appending to the file. If no logfile exists\n+ * and we decided not to create one, return 0 and set *logfd to -1. On\n+ * failure, fill in *err, set *logfd to -1, and return -1.\n */\n-static int log_ref_setup(const char *refname, struct strbuf *logfile, struct strbuf *err, int force_create)\n+static int log_ref_setup(const char *refname,\n+\t\t\t struct strbuf *logfile, int *logfd,\n+\t\t\t struct strbuf *err, int force_create)\n {\n-\tint logfd;\n-\n \tstrbuf_git_path(logfile, \"logs/%s\", refname);\n \n \tif (force_create || should_autocreate_reflog(refname)) {\n-\t\tif (raceproof_create_file(logfile->buf, open_or_create_logfile, &logfd)) {\n+\t\tif (raceproof_create_file(logfile->buf, open_or_create_logfile, logfd)) {\n \t\t\tif (errno == ENOENT)\n \t\t\t\tstrbuf_addf(err, \"unable to create directory for '%s': \"\n \t\t\t\t\t \"%s\", logfile->buf, strerror(errno));\n@@ -2745,8 +2749,8 @@ static int log_ref_setup(const char *refname, struct strbuf *logfile, struct str\n \t\t\treturn -1;\n \t\t}\n \t} else {\n-\t\tlogfd = open(logfile->buf, O_APPEND | O_WRONLY, 0666);\n-\t\tif (logfd < 0) {\n+\t\t*logfd = open(logfile->buf, O_APPEND | O_WRONLY, 0666);\n+\t\tif (*logfd < 0) {\n \t\t\tif (errno == ENOENT || errno == EISDIR) {\n \t\t\t\t/*\n \t\t\t\t * The logfile doesn't already exist,\n@@ -2762,10 +2766,8 @@ static int log_ref_setup(const char *refname, struct strbuf *logfile, struct str\n \t\t}\n \t}\n \n-\tif (logfd >= 0) {\n+\tif (*logfd >= 0)\n \t\tadjust_shared_perm(logfile->buf);\n-\t\tclose(logfd);\n-\t}\n \n \treturn 0;\n }\n@@ -2776,11 +2778,14 @@ static int files_create_reflog(struct ref_store *ref_store,\n {\n \tint ret;\n \tstruct strbuf sb = STRBUF_INIT;\n+\tint fd;\n \n \t/* Check validity (but we don't need the result): */\n \tfiles_downcast(ref_store, 0, \"create_reflog\");\n \n-\tret = log_ref_setup(refname, &sb, err, force_create);\n+\tret = log_ref_setup(refname, &sb, &fd, err, force_create);\n+\tif (fd >= 0)\n+\t\tclose(fd);\n \tstrbuf_release(&sb);\n \treturn ret;\n }\n@@ -2816,17 +2821,17 @@ static int log_ref_write_1(const char *refname, const unsigned char *old_sha1,\n \t\t\t struct strbuf *logfile, int flags,\n \t\t\t struct strbuf *err)\n {\n-\tint logfd, result, oflags = O_APPEND | O_WRONLY;\n+\tint logfd, result;\n \n \tif (log_all_ref_updates < 0)\n \t\tlog_all_ref_updates = !is_bare_repository();\n \n-\tresult = log_ref_setup(refname, logfile, err, flags & REF_FORCE_CREATE_REFLOG);\n+\tresult = log_ref_setup(refname, logfile, &logfd, err,\n+\t\t\t flags & REF_FORCE_CREATE_REFLOG);\n \n \tif (result)\n \t\treturn result;\n \n-\tlogfd = open(logfile->buf, oflags);\n \tif (logfd < 0)\n \t\treturn 0;\n \tresult = log_ref_write_fd(logfd, old_sha1, new_sha1,\n-- \n2.9.3\n\n"},{"id":"308522","messageId":"9ab4c213af5e342e0bfd6bc7a120c6fbd42d9538.1483153436.git.mhagger@alum.mit.edu","threadId":"44796","inReplyTo":"cover.1483153436.git.mhagger@alum.mit.edu","subject":"[PATCH v3 12/23] log_ref_setup(): improve robustness against races","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-12-31T03:12:52Z","receivedAt":"2016-12-31T03:13:44Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Change log_ref_setup() to use raceproof_create_file() to create the new\nlogfile. This makes it more robust against a race against another\nprocess that might be trying to clean up empty directories while we are\ntrying to create a new logfile.\n\nThis also means that it will only call create_leading_directories() if\nopen() fails, which should be a net win. Even in the cases where we are\nwilling to create a new logfile, it will usually be the case that the\nlogfile already exists, or if not then that the directory containing the\nlogfile already exists. In such cases, we will save some work that was\npreviously done unconditionally.\n\nSigned-off-by: Michael Haggerty \n---\n refs/files-backend.c | 41 ++++++++++++++++++-----------------------\n 1 file changed, 18 insertions(+), 23 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 47c7829..345f7c3 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2710,6 +2710,14 @@ static int commit_ref(struct ref_lock *lock)\n \treturn 0;\n }\n \n+static int open_or_create_logfile(const char *path, void *cb)\n+{\n+\tint *fd = cb;\n+\n+\t*fd = open(path, O_APPEND | O_WRONLY | O_CREAT, 0666);\n+\treturn (*fd < 0) ? -1 : 0;\n+}\n+\n /*\n * Create a reflog for a ref. If force_create = 0, the reflog will\n * only be created for certain refs (those for which\n@@ -2723,31 +2731,18 @@ static int log_ref_setup(const char *refname, struct strbuf *logfile, struct str\n \tstrbuf_git_path(logfile, \"logs/%s\", refname);\n \n \tif (force_create || should_autocreate_reflog(refname)) {\n-\t\tif (safe_create_leading_directories(logfile->buf) < 0) {\n-\t\t\tstrbuf_addf(err, \"unable to create directory for '%s': \"\n-\t\t\t\t \"%s\", logfile->buf, strerror(errno));\n-\t\t\treturn -1;\n-\t\t}\n-\t\tlogfd = open(logfile->buf, O_APPEND | O_WRONLY | O_CREAT, 0666);\n-\t\tif (logfd < 0) {\n-\t\t\tif (errno == EISDIR) {\n-\t\t\t\t/*\n-\t\t\t\t * The directory that is in the way might be\n-\t\t\t\t * empty. Try to remove it.\n-\t\t\t\t */\n-\t\t\t\tif (remove_empty_directories(logfile)) {\n-\t\t\t\t\tstrbuf_addf(err, \"there are still logs under \"\n-\t\t\t\t\t\t \"'%s'\", logfile->buf);\n-\t\t\t\t\treturn -1;\n-\t\t\t\t}\n-\t\t\t\tlogfd = open(logfile->buf, O_APPEND | O_WRONLY | O_CREAT, 0666);\n-\t\t\t}\n-\n-\t\t\tif (logfd < 0) {\n+\t\tif (raceproof_create_file(logfile->buf, open_or_create_logfile, &logfd)) {\n+\t\t\tif (errno == ENOENT)\n+\t\t\t\tstrbuf_addf(err, \"unable to create directory for '%s': \"\n+\t\t\t\t\t \"%s\", logfile->buf, strerror(errno));\n+\t\t\telse if (errno == EISDIR)\n+\t\t\t\tstrbuf_addf(err, \"there are still logs under '%s'\",\n+\t\t\t\t\t logfile->buf);\n+\t\t\telse\n \t\t\t\tstrbuf_addf(err, \"unable to append to '%s': %s\",\n \t\t\t\t\t logfile->buf, strerror(errno));\n-\t\t\t\treturn -1;\n-\t\t\t}\n+\n+\t\t\treturn -1;\n \t\t}\n \t} else {\n \t\tlogfd = open(logfile->buf, O_APPEND | O_WRONLY, 0666);\n-- \n2.9.3\n\n"},{"id":"308523","messageId":"d9f96df1bb2d5b9a95388da04b770ea9f317c491.1483153436.git.mhagger@alum.mit.edu","threadId":"44796","inReplyTo":"cover.1483153436.git.mhagger@alum.mit.edu","subject":"[PATCH v3 11/23] log_ref_setup(): separate code for create vs non-create","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-12-31T03:12:51Z","receivedAt":"2016-12-31T03:13:47Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"The behavior of this function (especially how it handles errors) is\nquite different depending on whether we are willing to create the reflog\nvs. whether we are only trying to open an existing reflog. So separate\nthe code paths.\n\nThis also simplifies the next steps.\n\nSigned-off-by: Michael Haggerty \n---\n refs/files-backend.c | 58 ++++++++++++++++++++++++++++++++++------------------\n 1 file changed, 38 insertions(+), 20 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex fd8a751..47c7829 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2718,45 +2718,63 @@ static int commit_ref(struct ref_lock *lock)\n */\n static int log_ref_setup(const char *refname, struct strbuf *logfile, struct strbuf *err, int force_create)\n {\n-\tint logfd, oflags = O_APPEND | O_WRONLY;\n+\tint logfd;\n \n \tstrbuf_git_path(logfile, \"logs/%s\", refname);\n+\n \tif (force_create || should_autocreate_reflog(refname)) {\n \t\tif (safe_create_leading_directories(logfile->buf) < 0) {\n \t\t\tstrbuf_addf(err, \"unable to create directory for '%s': \"\n \t\t\t\t \"%s\", logfile->buf, strerror(errno));\n \t\t\treturn -1;\n \t\t}\n-\t\toflags |= O_CREAT;\n-\t}\n-\n-\tlogfd = open(logfile->buf, oflags, 0666);\n-\tif (logfd < 0) {\n-\t\tif (!(oflags & O_CREAT) && (errno == ENOENT || errno == EISDIR))\n-\t\t\treturn 0;\n+\t\tlogfd = open(logfile->buf, O_APPEND | O_WRONLY | O_CREAT, 0666);\n+\t\tif (logfd < 0) {\n+\t\t\tif (errno == EISDIR) {\n+\t\t\t\t/*\n+\t\t\t\t * The directory that is in the way might be\n+\t\t\t\t * empty. Try to remove it.\n+\t\t\t\t */\n+\t\t\t\tif (remove_empty_directories(logfile)) {\n+\t\t\t\t\tstrbuf_addf(err, \"there are still logs under \"\n+\t\t\t\t\t\t \"'%s'\", logfile->buf);\n+\t\t\t\t\treturn -1;\n+\t\t\t\t}\n+\t\t\t\tlogfd = open(logfile->buf, O_APPEND | O_WRONLY | O_CREAT, 0666);\n+\t\t\t}\n \n-\t\tif (errno == EISDIR) {\n-\t\t\tif (remove_empty_directories(logfile)) {\n-\t\t\t\tstrbuf_addf(err, \"there are still logs under \"\n-\t\t\t\t\t \"'%s'\", logfile->buf);\n+\t\t\tif (logfd < 0) {\n+\t\t\t\tstrbuf_addf(err, \"unable to append to '%s': %s\",\n+\t\t\t\t\t logfile->buf, strerror(errno));\n \t\t\t\treturn -1;\n \t\t\t}\n-\t\t\tlogfd = open(logfile->buf, oflags, 0666);\n \t\t}\n-\n+\t} else {\n+\t\tlogfd = open(logfile->buf, O_APPEND | O_WRONLY, 0666);\n \t\tif (logfd < 0) {\n-\t\t\tstrbuf_addf(err, \"unable to append to '%s': %s\",\n-\t\t\t\t logfile->buf, strerror(errno));\n-\t\t\treturn -1;\n+\t\t\tif (errno == ENOENT || errno == EISDIR) {\n+\t\t\t\t/*\n+\t\t\t\t * The logfile doesn't already exist,\n+\t\t\t\t * but that is not an error; it only\n+\t\t\t\t * means that we won't write log\n+\t\t\t\t * entries to it.\n+\t\t\t\t */\n+\t\t\t} else {\n+\t\t\t\tstrbuf_addf(err, \"unable to append to '%s': %s\",\n+\t\t\t\t\t logfile->buf, strerror(errno));\n+\t\t\t\treturn -1;\n+\t\t\t}\n \t\t}\n \t}\n \n-\tadjust_shared_perm(logfile->buf);\n-\tclose(logfd);\n+\tif (logfd >= 0) {\n+\t\tadjust_shared_perm(logfile->buf);\n+\t\tclose(logfd);\n+\t}\n+\n \treturn 0;\n }\n \n-\n static int files_create_reflog(struct ref_store *ref_store,\n \t\t\t const char *refname, int force_create,\n \t\t\t struct strbuf *err)\n-- \n2.9.3\n\n"},{"id":"308524","messageId":"18e824188fd9254d4649cd692aabb11ce6cd1c03.1483153436.git.mhagger@alum.mit.edu","threadId":"44796","inReplyTo":"cover.1483153436.git.mhagger@alum.mit.edu","subject":"[PATCH v3 15/23] log_ref_setup(): manage the name of the reflog file internally","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-12-31T03:12:55Z","receivedAt":"2016-12-31T03:13:51Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Instead of writing the name of the reflog file into a strbuf that is\nsupplied by the caller but not needed there, write it into a local\ntemporary buffer and remove the strbuf parameter entirely.\n\nAnd while we're adjusting the function signature, reorder the arguments\nto move the input parameters before the output parameters.\n\nSigned-off-by: Michael Haggerty \n---\n refs/files-backend.c | 69 ++++++++++++++++++++++++++--------------------------\n 1 file changed, 34 insertions(+), 35 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 5a96424..455652c 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2719,37 +2719,36 @@ static int open_or_create_logfile(const char *path, void *cb)\n }\n \n /*\n- * Create a reflog for a ref. Store its path to *logfile. If\n- * force_create = 0, only create the reflog for certain refs (those\n- * for which should_autocreate_reflog returns non-zero). Otherwise,\n- * create it regardless of the reference name. If the logfile already\n- * existed or was created, return 0 and set *logfd to the file\n- * descriptor opened for appending to the file. If no logfile exists\n- * and we decided not to create one, return 0 and set *logfd to -1. On\n- * failure, fill in *err, set *logfd to -1, and return -1.\n+ * Create a reflog for a ref. If force_create = 0, only create the\n+ * reflog for certain refs (those for which should_autocreate_reflog\n+ * returns non-zero). Otherwise, create it regardless of the reference\n+ * name. If the logfile already existed or was created, return 0 and\n+ * set *logfd to the file descriptor opened for appending to the file.\n+ * If no logfile exists and we decided not to create one, return 0 and\n+ * set *logfd to -1. On failure, fill in *err, set *logfd to -1, and\n+ * return -1.\n */\n-static int log_ref_setup(const char *refname,\n-\t\t\t struct strbuf *logfile, int *logfd,\n-\t\t\t struct strbuf *err, int force_create)\n+static int log_ref_setup(const char *refname, int force_create,\n+\t\t\t int *logfd, struct strbuf *err)\n {\n-\tstrbuf_git_path(logfile, \"logs/%s\", refname);\n+\tchar *logfile = git_pathdup(\"logs/%s\", refname);\n \n \tif (force_create || should_autocreate_reflog(refname)) {\n-\t\tif (raceproof_create_file(logfile->buf, open_or_create_logfile, logfd)) {\n+\t\tif (raceproof_create_file(logfile, open_or_create_logfile, logfd)) {\n \t\t\tif (errno == ENOENT)\n \t\t\t\tstrbuf_addf(err, \"unable to create directory for '%s': \"\n-\t\t\t\t\t \"%s\", logfile->buf, strerror(errno));\n+\t\t\t\t\t \"%s\", logfile, strerror(errno));\n \t\t\telse if (errno == EISDIR)\n \t\t\t\tstrbuf_addf(err, \"there are still logs under '%s'\",\n-\t\t\t\t\t logfile->buf);\n+\t\t\t\t\t logfile);\n \t\t\telse\n \t\t\t\tstrbuf_addf(err, \"unable to append to '%s': %s\",\n-\t\t\t\t\t logfile->buf, strerror(errno));\n+\t\t\t\t\t logfile, strerror(errno));\n \n-\t\t\treturn -1;\n+\t\t\tgoto error;\n \t\t}\n \t} else {\n-\t\t*logfd = open(logfile->buf, O_APPEND | O_WRONLY, 0666);\n+\t\t*logfd = open(logfile, O_APPEND | O_WRONLY, 0666);\n \t\tif (*logfd < 0) {\n \t\t\tif (errno == ENOENT || errno == EISDIR) {\n \t\t\t\t/*\n@@ -2760,34 +2759,39 @@ static int log_ref_setup(const char *refname,\n \t\t\t\t */\n \t\t\t} else {\n \t\t\t\tstrbuf_addf(err, \"unable to append to '%s': %s\",\n-\t\t\t\t\t logfile->buf, strerror(errno));\n-\t\t\t\treturn -1;\n+\t\t\t\t\t logfile, strerror(errno));\n+\t\t\t\tgoto error;\n \t\t\t}\n \t\t}\n \t}\n \n \tif (*logfd >= 0)\n-\t\tadjust_shared_perm(logfile->buf);\n+\t\tadjust_shared_perm(logfile);\n \n+\tfree(logfile);\n \treturn 0;\n+\n+error:\n+\tfree(logfile);\n+\treturn -1;\n }\n \n static int files_create_reflog(struct ref_store *ref_store,\n \t\t\t const char *refname, int force_create,\n \t\t\t struct strbuf *err)\n {\n-\tint ret;\n-\tstruct strbuf sb = STRBUF_INIT;\n \tint fd;\n \n \t/* Check validity (but we don't need the result): */\n \tfiles_downcast(ref_store, 0, \"create_reflog\");\n \n-\tret = log_ref_setup(refname, &sb, &fd, err, force_create);\n+\tif (log_ref_setup(refname, force_create, &fd, err))\n+\t\treturn -1;\n+\n \tif (fd >= 0)\n \t\tclose(fd);\n-\tstrbuf_release(&sb);\n-\treturn ret;\n+\n+\treturn 0;\n }\n \n static int log_ref_write_fd(int fd, const unsigned char *old_sha1,\n@@ -2818,16 +2822,15 @@ static int log_ref_write_fd(int fd, const unsigned char *old_sha1,\n \n static int log_ref_write_1(const char *refname, const unsigned char *old_sha1,\n \t\t\t const unsigned char *new_sha1, const char *msg,\n-\t\t\t struct strbuf *logfile, int flags,\n-\t\t\t struct strbuf *err)\n+\t\t\t int flags, struct strbuf *err)\n {\n \tint logfd, result;\n \n \tif (log_all_ref_updates < 0)\n \t\tlog_all_ref_updates = !is_bare_repository();\n \n-\tresult = log_ref_setup(refname, logfile, &logfd, err,\n-\t\t\t flags & REF_FORCE_CREATE_REFLOG);\n+\tresult = log_ref_setup(refname, flags & REF_FORCE_CREATE_REFLOG,\n+\t\t\t &logfd, err);\n \n \tif (result)\n \t\treturn result;\n@@ -2858,11 +2861,7 @@ int files_log_ref_write(const char *refname, const unsigned char *old_sha1,\n \t\t\tconst unsigned char *new_sha1, const char *msg,\n \t\t\tint flags, struct strbuf *err)\n {\n-\tstruct strbuf sb = STRBUF_INIT;\n-\tint ret = log_ref_write_1(refname, old_sha1, new_sha1, msg, &sb, flags,\n-\t\t\t\t err);\n-\tstrbuf_release(&sb);\n-\treturn ret;\n+\treturn log_ref_write_1(refname, old_sha1, new_sha1, msg, flags, err);\n }\n \n /*\n-- \n2.9.3\n\n"},{"id":"308525","messageId":"f794e74e9aad39dc411f860c4834d4ba367ab30a.1483153436.git.mhagger@alum.mit.edu","threadId":"44796","inReplyTo":"cover.1483153436.git.mhagger@alum.mit.edu","subject":"[PATCH v3 16/23] log_ref_write_1(): inline function","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-12-31T03:12:56Z","receivedAt":"2016-12-31T03:13:52Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Now files_log_ref_write() doesn't do anything beyond call\nlog_ref_write_1(), so inline the latter into the former.\n\nSigned-off-by: Michael Haggerty \n---\n refs/files-backend.c | 13 +++----------\n 1 file changed, 3 insertions(+), 10 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 455652c..a4e0de5 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2820,9 +2820,9 @@ static int log_ref_write_fd(int fd, const unsigned char *old_sha1,\n \treturn 0;\n }\n \n-static int log_ref_write_1(const char *refname, const unsigned char *old_sha1,\n-\t\t\t const unsigned char *new_sha1, const char *msg,\n-\t\t\t int flags, struct strbuf *err)\n+int files_log_ref_write(const char *refname, const unsigned char *old_sha1,\n+\t\t\tconst unsigned char *new_sha1, const char *msg,\n+\t\t\tint flags, struct strbuf *err)\n {\n \tint logfd, result;\n \n@@ -2857,13 +2857,6 @@ static int log_ref_write_1(const char *refname, const unsigned char *old_sha1,\n \treturn 0;\n }\n \n-int files_log_ref_write(const char *refname, const unsigned char *old_sha1,\n-\t\t\tconst unsigned char *new_sha1, const char *msg,\n-\t\t\tint flags, struct strbuf *err)\n-{\n-\treturn log_ref_write_1(refname, old_sha1, new_sha1, msg, flags, err);\n-}\n-\n /*\n * Write sha1 into the open lockfile, then close the lockfile. On\n * errors, rollback the lockfile, fill in *err and\n-- \n2.9.3\n\n"},{"id":"308526","messageId":"1397dd5bbe2a9cc5bf2dda5c0bc3eda369edac9a.1483153436.git.mhagger@alum.mit.edu","threadId":"44796","inReplyTo":"cover.1483153436.git.mhagger@alum.mit.edu","subject":"[PATCH v3 19/23] try_remove_empty_parents(): rename parameter \"name\" -> \"refname\"","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-12-31T03:12:59Z","receivedAt":"2016-12-31T03:13:54Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"This is the standard nomenclature.\n\nSigned-off-by: Michael Haggerty \n---\n refs/files-backend.c | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex e81c8a9..0275c8d 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2282,13 +2282,13 @@ static int pack_if_possible_fn(struct ref_entry *entry, void *cb_data)\n \n /*\n * Remove empty parents, but spare refs/ and immediate subdirs.\n- * Note: munges *name.\n+ * Note: munges *refname.\n */\n-static void try_remove_empty_parents(char *name)\n+static void try_remove_empty_parents(char *refname)\n {\n \tchar *p, *q;\n \tint i;\n-\tp = name;\n+\tp = refname;\n \tfor (i = 0; i < 2; i++) { /* refs/{heads,tags,...}/ */\n \t\twhile (*p && *p != '/')\n \t\t\tp++;\n@@ -2306,7 +2306,7 @@ static void try_remove_empty_parents(char *name)\n \t\tif (q == p)\n \t\t\tbreak;\n \t\t*q = '\\0';\n-\t\tif (rmdir(git_path(\"%s\", name)))\n+\t\tif (rmdir(git_path(\"%s\", refname)))\n \t\t\tbreak;\n \t}\n }\n-- \n2.9.3\n\n"},{"id":"308527","messageId":"6164af2d1f9eeb5bd339b3913e8046c1ea0b02be.1483153436.git.mhagger@alum.mit.edu","threadId":"44796","inReplyTo":"cover.1483153436.git.mhagger@alum.mit.edu","subject":"[PATCH v3 20/23] try_remove_empty_parents(): don't trash argument contents","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-12-31T03:13:00Z","receivedAt":"2016-12-31T03:13:58Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"It's bad manners and surprising and therefore error-prone.\n\nSigned-off-by: Michael Haggerty \n---\n refs/files-backend.c | 16 +++++++++-------\n 1 file changed, 9 insertions(+), 7 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 0275c8d..af5a0e2 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2282,13 +2282,15 @@ static int pack_if_possible_fn(struct ref_entry *entry, void *cb_data)\n \n /*\n * Remove empty parents, but spare refs/ and immediate subdirs.\n- * Note: munges *refname.\n */\n-static void try_remove_empty_parents(char *refname)\n+static void try_remove_empty_parents(const char *refname)\n {\n+\tstruct strbuf buf = STRBUF_INIT;\n \tchar *p, *q;\n \tint i;\n-\tp = refname;\n+\n+\tstrbuf_addstr(&buf, refname);\n+\tp = buf.buf;\n \tfor (i = 0; i < 2; i++) { /* refs/{heads,tags,...}/ */\n \t\twhile (*p && *p != '/')\n \t\t\tp++;\n@@ -2296,8 +2298,7 @@ static void try_remove_empty_parents(char *refname)\n \t\twhile (*p == '/')\n \t\t\tp++;\n \t}\n-\tfor (q = p; *q; q++)\n-\t\t;\n+\tq = buf.buf + buf.len;\n \twhile (1) {\n \t\twhile (q > p && *q != '/')\n \t\t\tq--;\n@@ -2305,10 +2306,11 @@ static void try_remove_empty_parents(char *refname)\n \t\t\tq--;\n \t\tif (q == p)\n \t\t\tbreak;\n-\t\t*q = '\\0';\n-\t\tif (rmdir(git_path(\"%s\", refname)))\n+\t\tstrbuf_setlen(&buf, q - buf.buf);\n+\t\tif (rmdir(git_path(\"%s\", buf.buf)))\n \t\t\tbreak;\n \t}\n+\tstrbuf_release(&buf);\n }\n \n /* make sure nobody touched the ref, and unlink */\n-- \n2.9.3\n\n"},{"id":"308528","messageId":"8a114f536883002b359306194a9bbcdb6401468d.1483153436.git.mhagger@alum.mit.edu","threadId":"44796","inReplyTo":"cover.1483153436.git.mhagger@alum.mit.edu","subject":"[PATCH v3 22/23] try_remove_empty_parents(): teach to remove parents of reflogs, too","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-12-31T03:13:02Z","receivedAt":"2016-12-31T03:14:00Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Add a new \"flags\" parameter that tells the function whether to remove\nempty parent directories of the loose reference file, of the reflog\nfile, or both. The new functionality is not yet used.\n\nSigned-off-by: Michael Haggerty \n---\n refs/files-backend.c | 24 ++++++++++++++++++------\n 1 file changed, 18 insertions(+), 6 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 397488e..2155acf 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2280,10 +2280,18 @@ static int pack_if_possible_fn(struct ref_entry *entry, void *cb_data)\n \treturn 0;\n }\n \n+enum {\n+\tREMOVE_EMPTY_PARENTS_REF = 0x01,\n+\tREMOVE_EMPTY_PARENTS_REFLOG = 0x02\n+};\n+\n /*\n- * Remove empty parents, but spare refs/ and immediate subdirs.\n+ * Remove empty parent directories associated with the specified\n+ * reference and/or its reflog, but spare [logs/]refs/ and immediate\n+ * subdirs. flags is a combination of REMOVE_EMPTY_PARENTS_REF and/or\n+ * REMOVE_EMPTY_PARENTS_REFLOG.\n */\n-static void try_remove_empty_parents(const char *refname)\n+static void try_remove_empty_parents(const char *refname, unsigned int flags)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n \tchar *p, *q;\n@@ -2298,7 +2306,7 @@ static void try_remove_empty_parents(const char *refname)\n \t\t\tp++;\n \t}\n \tq = buf.buf + buf.len;\n-\twhile (1) {\n+\twhile (flags & (REMOVE_EMPTY_PARENTS_REF | REMOVE_EMPTY_PARENTS_REFLOG)) {\n \t\twhile (q > p && *q != '/')\n \t\t\tq--;\n \t\tif (q > p && *(q-1) == '/')\n@@ -2306,8 +2314,12 @@ static void try_remove_empty_parents(const char *refname)\n \t\tif (q == p)\n \t\t\tbreak;\n \t\tstrbuf_setlen(&buf, q - buf.buf);\n-\t\tif (rmdir(git_path(\"%s\", buf.buf)))\n-\t\t\tbreak;\n+\t\tif ((flags & REMOVE_EMPTY_PARENTS_REF) &&\n+\t\t rmdir(git_path(\"%s\", buf.buf)))\n+\t\t\tflags &= ~REMOVE_EMPTY_PARENTS_REF;\n+\t\tif ((flags & REMOVE_EMPTY_PARENTS_REFLOG) &&\n+\t\t rmdir(git_path(\"logs/%s\", buf.buf)))\n+\t\t\tflags &= ~REMOVE_EMPTY_PARENTS_REFLOG;\n \t}\n \tstrbuf_release(&buf);\n }\n@@ -2333,7 +2345,7 @@ static void prune_ref(struct ref_to_prune *r)\n \t}\n \tref_transaction_free(transaction);\n \tstrbuf_release(&err);\n-\ttry_remove_empty_parents(r->name);\n+\ttry_remove_empty_parents(r->name, REMOVE_EMPTY_PARENTS_REF);\n }\n \n static void prune_refs(struct ref_to_prune *r)\n-- \n2.9.3\n\n"},{"id":"308529","messageId":"c7a89febcbf7bdffb44f8fdf63a43f11339a0289.1483153436.git.mhagger@alum.mit.edu","threadId":"44796","inReplyTo":"cover.1483153436.git.mhagger@alum.mit.edu","subject":"[PATCH v3 21/23] try_remove_empty_parents(): don't accommodate consecutive slashes","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-12-31T03:13:01Z","receivedAt":"2016-12-31T03:14:03Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"\"refname\" has already been checked by check_refname_format(), so it\ncannot have consecutive slashes.\n\nSigned-off-by: Michael Haggerty \n---\n refs/files-backend.c | 5 ++---\n 1 file changed, 2 insertions(+), 3 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex af5a0e2..397488e 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2294,15 +2294,14 @@ static void try_remove_empty_parents(const char *refname)\n \tfor (i = 0; i < 2; i++) { /* refs/{heads,tags,...}/ */\n \t\twhile (*p && *p != '/')\n \t\t\tp++;\n-\t\t/* tolerate duplicate slashes; see check_refname_format() */\n-\t\twhile (*p == '/')\n+\t\tif (*p == '/')\n \t\t\tp++;\n \t}\n \tq = buf.buf + buf.len;\n \twhile (1) {\n \t\twhile (q > p && *q != '/')\n \t\t\tq--;\n-\t\twhile (q > p && *(q-1) == '/')\n+\t\tif (q > p && *(q-1) == '/')\n \t\t\tq--;\n \t\tif (q == p)\n \t\t\tbreak;\n-- \n2.9.3\n\n"},{"id":"308530","messageId":"732e8a06acd424f718add206214515919b107ebc.1483153436.git.mhagger@alum.mit.edu","threadId":"44796","inReplyTo":"cover.1483153436.git.mhagger@alum.mit.edu","subject":"[PATCH v3 23/23] files_transaction_commit(): clean up empty directories","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-12-31T03:13:03Z","receivedAt":"2016-12-31T03:14:08Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"When deleting/pruning references, remove any directories that are made\nempty by the deletion of loose references or of reflogs. Otherwise such\nempty directories can survive forever and accumulate over time. (Even\n'pack-refs', which is smart enough to remove the parent directories of\nloose references that it prunes, leaves directories that were already\nempty.)\n\nAnd now that files_transaction_commit() takes care of deleting the\nparent directories of loose references that it prunes, we don't have to\ndo that in prune_ref() anymore.\n\nThis change would be unwise if the *creation* of these directories could\nrace with our deletion of them. But the earlier changes in this patch\nseries made the creation paths robust against races, so now it is safe\nto tidy them up more aggressively.\n\nSigned-off-by: Michael Haggerty \n---\n refs/files-backend.c | 34 ++++++++++++++++++++++++++++------\n refs/refs-internal.h | 11 +++++++++--\n t/t1400-update-ref.sh | 27 +++++++++++++++++++++++++++\n 3 files changed, 64 insertions(+), 8 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 2155acf..26cfea3 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2345,7 +2345,6 @@ static void prune_ref(struct ref_to_prune *r)\n \t}\n \tref_transaction_free(transaction);\n \tstrbuf_release(&err);\n-\ttry_remove_empty_parents(r->name, REMOVE_EMPTY_PARENTS_REF);\n }\n \n static void prune_refs(struct ref_to_prune *r)\n@@ -3792,6 +3791,7 @@ static int files_transaction_commit(struct ref_store *ref_store,\n \t\t\t\t\tret = TRANSACTION_GENERIC_ERROR;\n \t\t\t\t\tgoto cleanup;\n \t\t\t\t}\n+\t\t\t\tupdate->flags |= REF_DELETED_LOOSE;\n \t\t\t}\n \n \t\t\tif (!(update->flags & REF_ISPRUNING))\n@@ -3804,16 +3804,38 @@ static int files_transaction_commit(struct ref_store *ref_store,\n \t\tret = TRANSACTION_GENERIC_ERROR;\n \t\tgoto cleanup;\n \t}\n-\tfor_each_string_list_item(ref_to_delete, &refs_to_delete)\n-\t\tunlink_or_warn(git_path(\"logs/%s\", ref_to_delete->string));\n+\n+\t/* Delete the reflogs of any references that were deleted: */\n+\tfor_each_string_list_item(ref_to_delete, &refs_to_delete) {\n+\t\tif (!unlink_or_warn(git_path(\"logs/%s\", ref_to_delete->string)))\n+\t\t\ttry_remove_empty_parents(ref_to_delete->string,\n+\t\t\t\t\t\t REMOVE_EMPTY_PARENTS_REFLOG);\n+\t}\n+\n \tclear_loose_ref_cache(refs);\n \n cleanup:\n \ttransaction->state = REF_TRANSACTION_CLOSED;\n \n-\tfor (i = 0; i < transaction->nr; i++)\n-\t\tif (transaction->updates[i]->backend_data)\n-\t\t\tunlock_ref(transaction->updates[i]->backend_data);\n+\tfor (i = 0; i < transaction->nr; i++) {\n+\t\tstruct ref_update *update = transaction->updates[i];\n+\t\tstruct ref_lock *lock = update->backend_data;\n+\n+\t\tif (lock)\n+\t\t\tunlock_ref(lock);\n+\n+\t\tif (update->flags & REF_DELETED_LOOSE) {\n+\t\t\t/*\n+\t\t\t * The loose reference was deleted. Delete any\n+\t\t\t * empty parent directories. (Note that this\n+\t\t\t * can only work because we have already\n+\t\t\t * removed the lockfile.)\n+\t\t\t */\n+\t\t\ttry_remove_empty_parents(update->refname,\n+\t\t\t\t\t\t REMOVE_EMPTY_PARENTS_REF);\n+\t\t}\n+\t}\n+\n \tstring_list_clear(&refs_to_delete, 0);\n \tfree(head_ref);\n \tstring_list_clear(&affected_refnames, 0);\ndiff --git a/refs/refs-internal.h b/refs/refs-internal.h\nindex 708b260..de49362 100644\n--- a/refs/refs-internal.h\n+++ b/refs/refs-internal.h\n@@ -56,6 +56,12 @@\n #define REF_UPDATE_VIA_HEAD 0x100\n \n /*\n+ * Used as a flag in ref_update::flags when the loose reference has\n+ * been deleted.\n+ */\n+#define REF_DELETED_LOOSE 0x200\n+\n+/*\n * Return true iff refname is minimally safe. \"Safe\" here means that\n * deleting a loose reference by this name will not do any damage, for\n * example by causing a file that is not a reference to be deleted.\n@@ -157,8 +163,9 @@ struct ref_update {\n \n \t/*\n \t * One or more of REF_HAVE_NEW, REF_HAVE_OLD, REF_NODEREF,\n-\t * REF_DELETING, REF_ISPRUNING, REF_LOG_ONLY, and\n-\t * REF_UPDATE_VIA_HEAD:\n+\t * REF_DELETING, REF_ISPRUNING, REF_LOG_ONLY,\n+\t * REF_UPDATE_VIA_HEAD, REF_NEEDS_COMMIT, and\n+\t * REF_DELETED_LOOSE:\n \t */\n \tunsigned int flags;\n \ndiff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh\nindex d4fb977..97d8793 100755\n--- a/t/t1400-update-ref.sh\n+++ b/t/t1400-update-ref.sh\n@@ -191,6 +191,33 @@ test_expect_success \\\n \t git update-ref HEAD'\" $A &&\n \t test $A\"' = $(cat .git/'\"$m\"')'\n \n+test_expect_success \"empty directory removal\" '\n+\tgit branch d1/d2/r1 HEAD &&\n+\tgit branch d1/r2 HEAD &&\n+\ttest -f .git/refs/heads/d1/d2/r1 &&\n+\ttest -f .git/logs/refs/heads/d1/d2/r1 &&\n+\tgit branch -d d1/d2/r1 &&\n+\t! test -e .git/refs/heads/d1/d2 &&\n+\t! test -e .git/logs/refs/heads/d1/d2 &&\n+\ttest -f .git/refs/heads/d1/r2 &&\n+\ttest -f .git/logs/refs/heads/d1/r2\n+'\n+\n+test_expect_success \"symref empty directory removal\" '\n+\tgit branch e1/e2/r1 HEAD &&\n+\tgit branch e1/r2 HEAD &&\n+\tgit checkout e1/e2/r1 &&\n+\ttest_when_finished \"git checkout master\" &&\n+\ttest -f .git/refs/heads/e1/e2/r1 &&\n+\ttest -f .git/logs/refs/heads/e1/e2/r1 &&\n+\tgit update-ref -d HEAD &&\n+\t! test -e .git/refs/heads/e1/e2 &&\n+\t! test -e .git/logs/refs/heads/e1/e2 &&\n+\ttest -f .git/refs/heads/e1/r2 &&\n+\ttest -f .git/logs/refs/heads/e1/r2 &&\n+\ttest -f .git/logs/HEAD\n+'\n+\n cat >expect < 1117150200 +0000\tInitial Creation\n $A $B $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150260 +0000\tSwitch\n-- \n2.9.3\n\n"},{"id":"308531","messageId":"ea26c628a22d169063b781d87ea961f4b787a8f4.1483153436.git.mhagger@alum.mit.edu","threadId":"44796","inReplyTo":"cover.1483153436.git.mhagger@alum.mit.edu","subject":"[PATCH v3 04/23] safe_create_leading_directories(): set errno on SCLD_EXISTS","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-12-31T03:12:44Z","receivedAt":"2016-12-31T03:14:11Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"The exit path for SCLD_EXISTS wasn't setting errno, which some callers\nuse to generate error messages for the user. Fix the problem and\ndocument that the function sets errno correctly to help avoid similar\nregressions in the future.\n\nSigned-off-by: Michael Haggerty \n---\n cache.h | 5 +++--\n sha1_file.c | 4 +++-\n 2 files changed, 6 insertions(+), 3 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex a50a61a..8177c3a 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1031,8 +1031,9 @@ int adjust_shared_perm(const char *path);\n \n /*\n * Create the directory containing the named path, using care to be\n- * somewhat safe against races. Return one of the scld_error values\n- * to indicate success/failure.\n+ * somewhat safe against races. Return one of the scld_error values to\n+ * indicate success/failure. On error, set errno to describe the\n+ * problem.\n *\n * SCLD_VANISHED indicates that one of the ancestor directories of the\n * path existed at one point during the function call and then\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 10395e7..ae8f0b4 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -137,8 +137,10 @@ enum scld_error safe_create_leading_directories(char *path)\n \t\t*slash = '\\0';\n \t\tif (!stat(path, &st)) {\n \t\t\t/* path exists */\n-\t\t\tif (!S_ISDIR(st.st_mode))\n+\t\t\tif (!S_ISDIR(st.st_mode)) {\n+\t\t\t\terrno = ENOTDIR;\n \t\t\t\tret = SCLD_EXISTS;\n+\t\t\t}\n \t\t} else if (mkdir(path, 0777)) {\n \t\t\tif (errno == EEXIST &&\n \t\t\t !stat(path, &st) && S_ISDIR(st.st_mode))\n-- \n2.9.3\n\n"},{"id":"308532","messageId":"75f51a671ed56bff55906c6f9400d53dfe8199c9.1483153436.git.mhagger@alum.mit.edu","threadId":"44796","inReplyTo":"cover.1483153436.git.mhagger@alum.mit.edu","subject":"[PATCH v3 08/23] rename_tmp_log(): use raceproof_create_file()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-12-31T03:12:48Z","receivedAt":"2016-12-31T03:14:13Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Besides shortening the code, this saves an unnecessary call to\nsafe_create_leading_directories_const() in almost all cases.\n\nSigned-off-by: Michael Haggerty \n---\n refs/files-backend.c | 73 +++++++++++++++++++++-------------------------------\n 1 file changed, 30 insertions(+), 43 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 74de289..3f18a01 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2489,55 +2489,42 @@ static int files_delete_refs(struct ref_store *ref_store,\n */\n #define TMP_RENAMED_LOG \"logs/refs/.tmp-renamed-log\"\n \n-static int rename_tmp_log(const char *newrefname)\n+static int rename_tmp_log_callback(const char *path, void *cb)\n {\n-\tint attempts_remaining = 4;\n-\tstruct strbuf path = STRBUF_INIT;\n-\tint ret = -1;\n+\tint *true_errno = cb;\n \n- retry:\n-\tstrbuf_reset(&path);\n-\tstrbuf_git_path(&path, \"logs/%s\", newrefname);\n-\tswitch (safe_create_leading_directories_const(path.buf)) {\n-\tcase SCLD_OK:\n-\t\tbreak; /* success */\n-\tcase SCLD_VANISHED:\n-\t\tif (--attempts_remaining > 0)\n-\t\t\tgoto retry;\n-\t\t/* fall through */\n-\tdefault:\n-\t\terror(\"unable to create directory for %s\", newrefname);\n-\t\tgoto out;\n+\tif (rename(git_path(TMP_RENAMED_LOG), path)) {\n+\t\t/*\n+\t\t * rename(a, b) when b is an existing directory ought\n+\t\t * to result in ISDIR, but Solaris 5.8 gives ENOTDIR.\n+\t\t * Sheesh. Record the true errno for error reporting,\n+\t\t * but report EISDIR to raceproof_create_file() so\n+\t\t * that it knows to retry.\n+\t\t */\n+\t\t*true_errno = errno;\n+\t\tif (errno == ENOTDIR)\n+\t\t\terrno = EISDIR;\n+\t\treturn -1;\n+\t} else {\n+\t\treturn 0;\n \t}\n+}\n \n-\tif (rename(git_path(TMP_RENAMED_LOG), path.buf)) {\n-\t\tif ((errno==EISDIR || errno==ENOTDIR) && --attempts_remaining > 0) {\n-\t\t\t/*\n-\t\t\t * rename(a, b) when b is an existing\n-\t\t\t * directory ought to result in ISDIR, but\n-\t\t\t * Solaris 5.8 gives ENOTDIR. Sheesh.\n-\t\t\t */\n-\t\t\tif (remove_empty_directories(&path)) {\n-\t\t\t\terror(\"Directory not empty: logs/%s\", newrefname);\n-\t\t\t\tgoto out;\n-\t\t\t}\n-\t\t\tgoto retry;\n-\t\t} else if (errno == ENOENT && --attempts_remaining > 0) {\n-\t\t\t/*\n-\t\t\t * Maybe another process just deleted one of\n-\t\t\t * the directories in the path to newrefname.\n-\t\t\t * Try again from the beginning.\n-\t\t\t */\n-\t\t\tgoto retry;\n-\t\t} else {\n+static int rename_tmp_log(const char *newrefname)\n+{\n+\tchar *path = git_pathdup(\"logs/%s\", newrefname);\n+\tint ret, true_errno;\n+\n+\tret = raceproof_create_file(path, rename_tmp_log_callback, &true_errno);\n+\tif (ret) {\n+\t\tif (errno == EISDIR)\n+\t\t\terror(\"Directory not empty: %s\", path);\n+\t\telse\n \t\t\terror(\"unable to move logfile \"TMP_RENAMED_LOG\" to logs/%s: %s\",\n-\t\t\t\tnewrefname, strerror(errno));\n-\t\t\tgoto out;\n-\t\t}\n+\t\t\t\tnewrefname, strerror(true_errno));\n \t}\n-\tret = 0;\n-out:\n-\tstrbuf_release(&path);\n+\n+\tfree(path);\n \treturn ret;\n }\n \n-- \n2.9.3\n\n"},{"id":"308533","messageId":"1e1295aff09039fc49188b085bda6ee5166d313e.1483153436.git.mhagger@alum.mit.edu","threadId":"44796","inReplyTo":"cover.1483153436.git.mhagger@alum.mit.edu","subject":"[PATCH v3 14/23] log_ref_write_1(): don't depend on logfile argument","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-12-31T03:12:54Z","receivedAt":"2016-12-31T03:14:15Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"It's unnecessary to pass a strbuf holding the reflog path up and down\nthe call stack now that it is hardly needed by the callers. Remove the\nplaces where log_ref_write_1() uses it, in preparation for making it\ninternal to log_ref_setup().\n\nSigned-off-by: Michael Haggerty \n---\n refs/files-backend.c | 12 ++++++++----\n 1 file changed, 8 insertions(+), 4 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 7f26cf8..5a96424 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2837,14 +2837,18 @@ static int log_ref_write_1(const char *refname, const unsigned char *old_sha1,\n \tresult = log_ref_write_fd(logfd, old_sha1, new_sha1,\n \t\t\t\t git_committer_info(0), msg);\n \tif (result) {\n-\t\tstrbuf_addf(err, \"unable to append to '%s': %s\", logfile->buf,\n-\t\t\t strerror(errno));\n+\t\tint save_errno = errno;\n+\n+\t\tstrbuf_addf(err, \"unable to append to '%s': %s\",\n+\t\t\t git_path(\"logs/%s\", refname), strerror(save_errno));\n \t\tclose(logfd);\n \t\treturn -1;\n \t}\n \tif (close(logfd)) {\n-\t\tstrbuf_addf(err, \"unable to append to '%s': %s\", logfile->buf,\n-\t\t\t strerror(errno));\n+\t\tint save_errno = errno;\n+\n+\t\tstrbuf_addf(err, \"unable to append to '%s': %s\",\n+\t\t\t git_path(\"logs/%s\", refname), strerror(save_errno));\n \t\treturn -1;\n \t}\n \treturn 0;\n-- \n2.9.3\n\n"},{"id":"308534","messageId":"c9627b22c46572c8a76c6a3d9ca81f221c2d2b59.1483153436.git.mhagger@alum.mit.edu","threadId":"44796","inReplyTo":"cover.1483153436.git.mhagger@alum.mit.edu","subject":"[PATCH v3 17/23] delete_ref_loose(): derive loose reference path from lock","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-12-31T03:12:57Z","receivedAt":"2016-12-31T03:14:17Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"It is simpler to derive the path to the file that must be deleted from\n\"lock->ref_name\" than from the lock_file object.\n\nSigned-off-by: Michael Haggerty \n---\n refs/files-backend.c | 5 +----\n 1 file changed, 1 insertion(+), 4 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex a4e0de5..847af81 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2430,10 +2430,7 @@ static int delete_ref_loose(struct ref_lock *lock, int flag, struct strbuf *err)\n \t\t * loose. The loose file name is the same as the\n \t\t * lockfile name, minus \".lock\":\n \t\t */\n-\t\tchar *loose_filename = get_locked_file_path(lock->lk);\n-\t\tint res = unlink_or_msg(loose_filename, err);\n-\t\tfree(loose_filename);\n-\t\tif (res)\n+\t\tif (unlink_or_msg(git_path(\"%s\", lock->ref_name), err))\n \t\t\treturn 1;\n \t}\n \treturn 0;\n-- \n2.9.3\n\n"},{"id":"308535","messageId":"428b760cc6110ea89a47056b88bf36a7e7517fd4.1483153436.git.mhagger@alum.mit.edu","threadId":"44796","inReplyTo":"cover.1483153436.git.mhagger@alum.mit.edu","subject":"[PATCH v3 18/23] delete_ref_loose(): inline function","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-12-31T03:12:58Z","receivedAt":"2016-12-31T03:14:18Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"It was hardly doing anything anymore, and had only one caller.\n\nSigned-off-by: Michael Haggerty \n---\n refs/files-backend.c | 25 +++++++------------------\n 1 file changed, 7 insertions(+), 18 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 847af81..e81c8a9 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2421,21 +2421,6 @@ static int repack_without_refs(struct files_ref_store *refs,\n \treturn ret;\n }\n \n-static int delete_ref_loose(struct ref_lock *lock, int flag, struct strbuf *err)\n-{\n-\tassert(err);\n-\n-\tif (!(flag & REF_ISPACKED) || flag & REF_ISSYMREF) {\n-\t\t/*\n-\t\t * loose. The loose file name is the same as the\n-\t\t * lockfile name, minus \".lock\":\n-\t\t */\n-\t\tif (unlink_or_msg(git_path(\"%s\", lock->ref_name), err))\n-\t\t\treturn 1;\n-\t}\n-\treturn 0;\n-}\n-\n static int files_delete_refs(struct ref_store *ref_store,\n \t\t\t struct string_list *refnames, unsigned int flags)\n {\n@@ -3787,9 +3772,13 @@ static int files_transaction_commit(struct ref_store *ref_store,\n \n \t\tif (update->flags & REF_DELETING &&\n \t\t !(update->flags & REF_LOG_ONLY)) {\n-\t\t\tif (delete_ref_loose(lock, update->type, err)) {\n-\t\t\t\tret = TRANSACTION_GENERIC_ERROR;\n-\t\t\t\tgoto cleanup;\n+\t\t\tif (!(update->type & REF_ISPACKED) ||\n+\t\t\t update->type & REF_ISSYMREF) {\n+\t\t\t\t/* It is a loose reference. */\n+\t\t\t\tif (unlink_or_msg(git_path(\"%s\", lock->ref_name), err)) {\n+\t\t\t\t\tret = TRANSACTION_GENERIC_ERROR;\n+\t\t\t\t\tgoto cleanup;\n+\t\t\t\t}\n \t\t\t}\n \n \t\t\tif (!(update->flags & REF_ISPRUNING))\n-- \n2.9.3\n\n"},{"id":"308536","messageId":"8fc817feaf8d76980ea33b19a4529a2ed440095e.1483153436.git.mhagger@alum.mit.edu","threadId":"44796","inReplyTo":"cover.1483153436.git.mhagger@alum.mit.edu","subject":"[PATCH v3 09/23] rename_tmp_log(): improve error reporting","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-12-31T03:12:49Z","receivedAt":"2016-12-31T03:14:22Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"* Don't capitalize error strings\n* Report true paths of affected files\n\nSigned-off-by: Michael Haggerty \n---\n refs/files-backend.c | 7 ++++---\n 1 file changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 3f18a01..49a119c 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2518,10 +2518,11 @@ static int rename_tmp_log(const char *newrefname)\n \tret = raceproof_create_file(path, rename_tmp_log_callback, &true_errno);\n \tif (ret) {\n \t\tif (errno == EISDIR)\n-\t\t\terror(\"Directory not empty: %s\", path);\n+\t\t\terror(\"directory not empty: %s\", path);\n \t\telse\n-\t\t\terror(\"unable to move logfile \"TMP_RENAMED_LOG\" to logs/%s: %s\",\n-\t\t\t\tnewrefname, strerror(true_errno));\n+\t\t\terror(\"unable to move logfile %s to %s: %s\",\n+\t\t\t git_path(TMP_RENAMED_LOG), path,\n+\t\t\t strerror(true_errno));\n \t}\n \n \tfree(path);\n-- \n2.9.3\n\n"},{"id":"308537","messageId":"dba3d081c32854593d8113f9cd604a9891748bcd.1483153436.git.mhagger@alum.mit.edu","threadId":"44796","inReplyTo":"cover.1483153436.git.mhagger@alum.mit.edu","subject":"[PATCH v3 10/23] log_ref_write(): inline function","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-12-31T03:12:50Z","receivedAt":"2016-12-31T03:14:23Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"This function doesn't do anything beyond call files_log_ref_write(), so\nreplace it with the latter at its call sites.\n\nSigned-off-by: Michael Haggerty \n---\n refs/files-backend.c | 24 ++++++++++--------------\n 1 file changed, 10 insertions(+), 14 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 49a119c..fd8a751 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2832,14 +2832,6 @@ static int log_ref_write_1(const char *refname, const unsigned char *old_sha1,\n \treturn 0;\n }\n \n-static int log_ref_write(const char *refname, const unsigned char *old_sha1,\n-\t\t\t const unsigned char *new_sha1, const char *msg,\n-\t\t\t int flags, struct strbuf *err)\n-{\n-\treturn files_log_ref_write(refname, old_sha1, new_sha1, msg, flags,\n-\t\t\t\t err);\n-}\n-\n int files_log_ref_write(const char *refname, const unsigned char *old_sha1,\n \t\t\tconst unsigned char *new_sha1, const char *msg,\n \t\t\tint flags, struct strbuf *err)\n@@ -2903,7 +2895,8 @@ static int commit_ref_update(struct files_ref_store *refs,\n \tassert_main_repository(&refs->base, \"commit_ref_update\");\n \n \tclear_loose_ref_cache(refs);\n-\tif (log_ref_write(lock->ref_name, lock->old_oid.hash, sha1, logmsg, 0, err)) {\n+\tif (files_log_ref_write(lock->ref_name, lock->old_oid.hash, sha1,\n+\t\t\t\tlogmsg, 0, err)) {\n \t\tchar *old_msg = strbuf_detach(err, NULL);\n \t\tstrbuf_addf(err, \"cannot update the ref '%s': %s\",\n \t\t\t lock->ref_name, old_msg);\n@@ -2934,7 +2927,7 @@ static int commit_ref_update(struct files_ref_store *refs,\n \t\tif (head_ref && (head_flag & REF_ISSYMREF) &&\n \t\t !strcmp(head_ref, lock->ref_name)) {\n \t\t\tstruct strbuf log_err = STRBUF_INIT;\n-\t\t\tif (log_ref_write(\"HEAD\", lock->old_oid.hash, sha1,\n+\t\t\tif (files_log_ref_write(\"HEAD\", lock->old_oid.hash, sha1,\n \t\t\t\t\t logmsg, 0, &log_err)) {\n \t\t\t\terror(\"%s\", log_err.buf);\n \t\t\t\tstrbuf_release(&log_err);\n@@ -2973,7 +2966,8 @@ static void update_symref_reflog(struct ref_lock *lock, const char *refname,\n \tstruct strbuf err = STRBUF_INIT;\n \tunsigned char new_sha1[20];\n \tif (logmsg && !read_ref(target, new_sha1) &&\n-\t log_ref_write(refname, lock->old_oid.hash, new_sha1, logmsg, 0, &err)) {\n+\t files_log_ref_write(refname, lock->old_oid.hash, new_sha1,\n+\t\t\t\tlogmsg, 0, &err)) {\n \t\terror(\"%s\", err.buf);\n \t\tstrbuf_release(&err);\n \t}\n@@ -3748,9 +3742,11 @@ static int files_transaction_commit(struct ref_store *ref_store,\n \n \t\tif (update->flags & REF_NEEDS_COMMIT ||\n \t\t update->flags & REF_LOG_ONLY) {\n-\t\t\tif (log_ref_write(lock->ref_name, lock->old_oid.hash,\n-\t\t\t\t\t update->new_sha1,\n-\t\t\t\t\t update->msg, update->flags, err)) {\n+\t\t\tif (files_log_ref_write(lock->ref_name,\n+\t\t\t\t\t\tlock->old_oid.hash,\n+\t\t\t\t\t\tupdate->new_sha1,\n+\t\t\t\t\t\tupdate->msg, update->flags,\n+\t\t\t\t\t\terr)) {\n \t\t\t\tchar *old_msg = strbuf_detach(err, NULL);\n \n \t\t\t\tstrbuf_addf(err, \"cannot update the ref '%s': %s\",\n-- \n2.9.3\n\n"},{"id":"308538","messageId":"20161231061146.gxlbma6w7odho4c7@sigill.intra.peff.net","threadId":"44796","inReplyTo":"f933f9d3c4c53b42ecc75b7a743ed4bfd390b4c5.1483153436.git.mhagger@alum.mit.edu","subject":"Re: [PATCH v3 05/23] raceproof_create_file(): new function","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-12-31T06:11:46Z","receivedAt":"2016-12-31T06:11:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Dec 31, 2016 at 04:12:45AM +0100, Michael Haggerty wrote:\n\n> Add a function that tries to create a file and any containing\n> directories in a way that is robust against races with other processes\n> that might be cleaning up empty directories at the same time.\n> \n> The actual file creation is done by a callback function, which, if it\n> fails, should set errno to EISDIR or ENOENT according to the convention\n> of open(). raceproof_create_file() detects such failures, and\n> respectively either tries to delete empty directories that might be in\n> the way of the file or tries to create the containing directories. Then\n> it retries the callback function.\n\nThis seems like a nice primitive, and the resulting change in patch 7 is\nvery pleasant.\n\nAt first I was surprised that the callback did not take the more usual\nopen(2) flags, which might make it easy to reuse a few basic callbacks.\nBut I see that in most cases the actual opening is deep inside a higher\nlevel construct like the lockfile code, and anything beyond the \"void *\"\ncallback parameter that you have would make that really awkward.\n\n> +/*\n> + * Callback function for raceproof_create_file(). This function is\n> + * expected to do something that makes dirname(path) permanent despite\n> + * the fact that other processes might be cleaning up empty\n> + * directories at the same time. Usually it will create a file named\n> + * path, but alternatively it could create another file in that\n> + * directory, or even chdir() into that directory. The function should\n> + * return 0 if the action was completed successfully. On error, it\n> + * should return a nonzero result and set errno.\n> + * raceproof_create_file() treats two errno values specially:\n> + *\n> + * - ENOENT -- dirname(path) does not exist. In this case,\n> + * raceproof_create_file() tries creating dirname(path)\n> + * (and any parent directories, if necessary) and calls\n> + * the function again.\n> + *\n> + * - EISDIR -- the file already exists and is a directory. In this\n> + * case, raceproof_create_file() deletes the directory\n> + * (recursively) if it is empty and calls the function\n> + * again.\n\nIt took me a minute to figure out why EISDIR is recursive.\n\nIf we are trying to create \"foo/bar/baz\", we can only get EISDIR when\n\"baz\" exists and is a directory. I at first took your recursive to me\nthat we delete it and \"foo/bar\" and \"foo\". Which is just silly and\ncounterproductive.\n\nBut presumably you mean that we delete \"foo/bar/baz/xyzzy\", etc, up to\n\"foo/bar/baz\", provided they are all empty directories. I think your\ncomment is probably OK and I was just being thick, but maybe stating it\nlike:\n\n ...removes the directory if it is empty (and recursively any empty\n directories it contains) and calls the function again.\n\nwould be more clear. That is still leaving the definition of \"empty\"\nimplied, but it's hopefully obvious from the context.\n\n> +int raceproof_create_file(const char *path, create_file_fn fn, void *cb)\n> +{\n> +\t/*\n> +\t * The number of times we will try to remove empty directories\n> +\t * in the way of path. This is only 1 because if another\n> +\t * process is racily creating directories that conflict with\n> +\t * us, we don't want to fight against them.\n> +\t */\n> +\tint remove_directories_remaining = 1;\n> +\n> +\t/*\n> +\t * The number of times that we will try to create the\n> +\t * directories containing path. We are willing to attempt this\n> +\t * more than once, because another process could be trying to\n> +\t * clean up empty directories at the same time as we are\n> +\t * trying to create them.\n> +\t */\n> +\tint create_directories_remaining = 3;\n\nWe know that 3 is higher than 1, so we would not fight forever between\nwriting \"foo\" and \"foo/bar\". That made me wonder if we could fight with\nother code. The obvious one would be try_remove_empty_parents() in\nfiles-backend.c, but it makes only a single attempt at each directory.\nSo we should \"win\" against it short of weird cases (like somebody\nrunning \"git pack-refs --prune\" in a tight loop).\n\n> [...the actual function logic...]\n\nNice. This looks very straightforward.\n\n-Peff\n"},{"id":"308539","messageId":"20161231062607.uxftwujophv37ymb@sigill.intra.peff.net","threadId":"44796","inReplyTo":"d9f96df1bb2d5b9a95388da04b770ea9f317c491.1483153436.git.mhagger@alum.mit.edu","subject":"Re: [PATCH v3 11/23] log_ref_setup(): separate code for create vs non-create","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-12-31T06:26:07Z","receivedAt":"2016-12-31T06:26:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Dec 31, 2016 at 04:12:51AM +0100, Michael Haggerty wrote:\n\n> +\t} else {\n> +\t\tlogfd = open(logfile->buf, O_APPEND | O_WRONLY, 0666);\n> \t\tif (logfd < 0) {\n> -\t\t\tstrbuf_addf(err, \"unable to append to '%s': %s\",\n> -\t\t\t\t logfile->buf, strerror(errno));\n> -\t\t\treturn -1;\n> +\t\t\tif (errno == ENOENT || errno == EISDIR) {\n> +\t\t\t\t/*\n> +\t\t\t\t * The logfile doesn't already exist,\n> +\t\t\t\t * but that is not an error; it only\n> +\t\t\t\t * means that we won't write log\n> +\t\t\t\t * entries to it.\n> +\t\t\t\t */\n> +\t\t\t} else {\n> +\t\t\t\tstrbuf_addf(err, \"unable to append to '%s': %s\",\n> +\t\t\t\t\t logfile->buf, strerror(errno));\n> +\t\t\t\treturn -1;\n> +\t\t\t}\n> \t\t}\n> \t}\n> \n> -\tadjust_shared_perm(logfile->buf);\n> -\tclose(logfd);\n> +\tif (logfd >= 0) {\n> +\t\tadjust_shared_perm(logfile->buf);\n> +\t\tclose(logfd);\n> +\t}\n> +\n\nHmm. I would have thought in the existing-logfile case that we would not\nneed to adjust_shared_perm(). But maybe we just do it anyway to pick up\npotentially-changed config.\n\nI also had to double-take at this close(). Aren't we calling this\nfunction so we can actually write to the log? But I skipped ahead in\nyour series and see you address that confusion. :)\n\n-Peff\n"},{"id":"308540","messageId":"20161231063211.tqsiafg3iahcuotz@sigill.intra.peff.net","threadId":"44796","inReplyTo":"ef2355e9d5ccaa53928c821530bae59f2b118013.1483153436.git.mhagger@alum.mit.edu","subject":"Re: [PATCH v3 13/23] log_ref_setup(): pass the open file descriptor back to the caller","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-12-31T06:32:12Z","receivedAt":"2016-12-31T06:32:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Dec 31, 2016 at 04:12:53AM +0100, Michael Haggerty wrote:\n\n> This function will most often be called by log_ref_write_1(), which\n> wants to append to the reflog file. In that case, it is silly to close\n> the file only for the caller to reopen it immediately. So, in the case\n> that the file was opened, pass the open file descriptor back to the\n> caller.\n\nSounds like a much saner interface.\n\n> /*\n> - * Create a reflog for a ref. If force_create = 0, the reflog will\n> - * only be created for certain refs (those for which\n> - * should_autocreate_reflog returns non-zero. Otherwise, create it\n> - * regardless of the ref name. Fill in *err and return -1 on failure.\n> + * Create a reflog for a ref. Store its path to *logfile. If\n> + * force_create = 0, only create the reflog for certain refs (those\n> + * for which should_autocreate_reflog returns non-zero). Otherwise,\n> + * create it regardless of the reference name. If the logfile already\n> + * existed or was created, return 0 and set *logfd to the file\n> + * descriptor opened for appending to the file. If no logfile exists\n> + * and we decided not to create one, return 0 and set *logfd to -1. On\n> + * failure, fill in *err, set *logfd to -1, and return -1.\n> */\n> -static int log_ref_setup(const char *refname, struct strbuf *logfile, struct strbuf *err, int force_create)\n> +static int log_ref_setup(const char *refname,\n> +\t\t\t struct strbuf *logfile, int *logfd,\n> +\t\t\t struct strbuf *err, int force_create)\n\nThe return value is always \"0\" or \"-1\". It seems like it would be\nsimpler to just return the descriptor instead of 0.\n\nI guess that makes it hard to identify the case when we chose not to\ncreate a descriptor. I wonder if more \"normal\" semantics would be:\n\n 1. ret >= 0: file existed or was created, and ret is the descriptor\n\n 2. ret < 0, err is empty: we chose not to create\n\n 3. ret < 0, err is non-empty: a real error\n\nI dunno. This may just be bikeshedding, and I can live with it either\nway (especially because you documented it!).\n\n-Peff\n"},{"id":"308541","messageId":"20161231063523.fncqqpr3m42jjvbs@sigill.intra.peff.net","threadId":"44796","inReplyTo":"1e1295aff09039fc49188b085bda6ee5166d313e.1483153436.git.mhagger@alum.mit.edu","subject":"Re: [PATCH v3 14/23] log_ref_write_1(): don't depend on logfile argument","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-12-31T06:35:23Z","receivedAt":"2016-12-31T06:35:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Dec 31, 2016 at 04:12:54AM +0100, Michael Haggerty wrote:\n\n> It's unnecessary to pass a strbuf holding the reflog path up and down\n> the call stack now that it is hardly needed by the callers. Remove the\n> places where log_ref_write_1() uses it, in preparation for making it\n> internal to log_ref_setup().\n> \n> Signed-off-by: Michael Haggerty \n> ---\n> refs/files-backend.c | 12 ++++++++----\n> 1 file changed, 8 insertions(+), 4 deletions(-)\n> \n> diff --git a/refs/files-backend.c b/refs/files-backend.c\n> index 7f26cf8..5a96424 100644\n> --- a/refs/files-backend.c\n> +++ b/refs/files-backend.c\n> @@ -2837,14 +2837,18 @@ static int log_ref_write_1(const char *refname, const unsigned char *old_sha1,\n> \tresult = log_ref_write_fd(logfd, old_sha1, new_sha1,\n> \t\t\t\t git_committer_info(0), msg);\n> \tif (result) {\n> -\t\tstrbuf_addf(err, \"unable to append to '%s': %s\", logfile->buf,\n> -\t\t\t strerror(errno));\n> +\t\tint save_errno = errno;\n> +\n> +\t\tstrbuf_addf(err, \"unable to append to '%s': %s\",\n> +\t\t\t git_path(\"logs/%s\", refname), strerror(save_errno));\n\nHmm. This means the logic of \"the path for a reflog is\ngit_path(logs/%s)\" is now replicated in several places. Which feels kind\nof like a backwards step. But I guess it is pretty well cemented in the\nconcept of files-backend.c, and I do like the later cleanups that this\nallows.\n\n-Peff\n"},{"id":"308542","messageId":"20161231064053.prvlw6x6qprzkmw7@sigill.intra.peff.net","threadId":"44796","inReplyTo":"6164af2d1f9eeb5bd339b3913e8046c1ea0b02be.1483153436.git.mhagger@alum.mit.edu","subject":"Re: [PATCH v3 20/23] try_remove_empty_parents(): don't trash argument contents","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-12-31T06:40:53Z","receivedAt":"2016-12-31T06:41:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Dec 31, 2016 at 04:13:00AM +0100, Michael Haggerty wrote:\n\n> It's bad manners and surprising and therefore error-prone.\n\nAgreed.\n\nI wondered while reading your earlier patch whether the\nsafe_create_leading_directories() function had the same problem, but it\ncarefully replaces the NUL it inserts.\n\n> -static void try_remove_empty_parents(char *refname)\n> +static void try_remove_empty_parents(const char *refname)\n> {\n> +\tstruct strbuf buf = STRBUF_INIT;\n> \tchar *p, *q;\n> \tint i;\n> -\tp = refname;\n> +\n> +\tstrbuf_addstr(&buf, refname);\n\nI see here you just make a copy. I think it would be enough to do:\n\n> @@ -2305,10 +2306,11 @@ static void try_remove_empty_parents(char *refname)\n> \t\t\tq--;\n> \t\tif (q == p)\n> \t\t\tbreak;\n> -\t\t*q = '\\0';\n> -\t\tif (rmdir(git_path(\"%s\", refname)))\n> +\t\tstrbuf_setlen(&buf, q - buf.buf);\n> +\t\tif (rmdir(git_path(\"%s\", buf.buf)))\n> \t\t\tbreak;\n\n *q = '\\0';\n r = rmdir(git_path(\"%s\", refname));\n *q = '/';\n\n if (r)\n break;\n\nor something. But I doubt the single allocation is breaking the bank,\nand it has the nice side effect that callers can pass in a const string\n(I didn't check yet whether that enables further cleanups).\n\n-Peff\n"},{"id":"308543","messageId":"20161231064746.6bvis76p5x5ubc2b@sigill.intra.peff.net","threadId":"44796","inReplyTo":"cover.1483153436.git.mhagger@alum.mit.edu","subject":"Re: [PATCH v3 00/23] Delete directories left empty after ref deletion","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-12-31T06:47:46Z","receivedAt":"2016-12-31T06:47:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Dec 31, 2016 at 04:12:40AM +0100, Michael Haggerty wrote:\n\n> This is a re-roll of an old patch series. v1 [1] got some feedback,\n> which I think was all addressed in v2 [2]. But it seems that v2 fell\n> on the floor, and I didn't bother following up because it was in the\n> same area of code that was undergoing heavy changes due to the\n> pluggable reference backend work. Sorry for the long delay before\n> getting back to it.\n\nI've read through the whole thing, and aside from a few very minor nits\n(that I am not even sure are worth a re-roll), I didn't see anything\nwrong. And the overall goal and approach seem obviously sound.\n\n> Michael Haggerty (23):\n\nI'll admit to being daunted by the number of patches, but it was quite a\npleasant and easy read. Thanks.\n\n-Peff\n"},{"id":"308544","messageId":"241de54c-63ee-d13c-c9fe-8b420871ac51@alum.mit.edu","threadId":"44796","inReplyTo":"20161231061146.gxlbma6w7odho4c7@sigill.intra.peff.net","subject":"Re: [PATCH v3 05/23] raceproof_create_file(): new function","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-12-31T07:42:23Z","receivedAt":"2016-12-31T07:42:36Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 12/31/2016 07:11 AM, Jeff King wrote:\n> On Sat, Dec 31, 2016 at 04:12:45AM +0100, Michael Haggerty wrote:\n>> [...]\n>> +/*\n>> + * Callback function for raceproof_create_file(). This function is\n>> + * expected to do something that makes dirname(path) permanent despite\n>> + * the fact that other processes might be cleaning up empty\n>> + * directories at the same time. Usually it will create a file named\n>> + * path, but alternatively it could create another file in that\n>> + * directory, or even chdir() into that directory. The function should\n>> + * return 0 if the action was completed successfully. On error, it\n>> + * should return a nonzero result and set errno.\n>> + * raceproof_create_file() treats two errno values specially:\n>> + *\n>> + * - ENOENT -- dirname(path) does not exist. In this case,\n>> + * raceproof_create_file() tries creating dirname(path)\n>> + * (and any parent directories, if necessary) and calls\n>> + * the function again.\n>> + *\n>> + * - EISDIR -- the file already exists and is a directory. In this\n>> + * case, raceproof_create_file() deletes the directory\n>> + * (recursively) if it is empty and calls the function\n>> + * again.\n> \n> It took me a minute to figure out why EISDIR is recursive.\n> \n> If we are trying to create \"foo/bar/baz\", we can only get EISDIR when\n> \"baz\" exists and is a directory. I at first took your recursive to me\n> that we delete it and \"foo/bar\" and \"foo\". Which is just silly and\n> counterproductive.\n> \n> But presumably you mean that we delete \"foo/bar/baz/xyzzy\", etc, up to\n> \"foo/bar/baz\", provided they are all empty directories. I think your\n> comment is probably OK and I was just being thick, but maybe stating it\n> like:\n> \n> ...removes the directory if it is empty (and recursively any empty\n> directories it contains) and calls the function again.\n> \n> would be more clear. That is still leaving the definition of \"empty\"\n> implied, but it's hopefully obvious from the context.\n\nYes, that's clearer. I'll change it. Thanks!\n\n> [...]\n\nMichael\n\n"},{"id":"308545","messageId":"2bd120b1-377a-0416-1591-1da4ab69d655@alum.mit.edu","threadId":"44796","inReplyTo":"20161231062607.uxftwujophv37ymb@sigill.intra.peff.net","subject":"Re: [PATCH v3 11/23] log_ref_setup(): separate code for create vs non-create","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-12-31T07:52:30Z","receivedAt":"2016-12-31T07:52:42Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 12/31/2016 07:26 AM, Jeff King wrote:\n> On Sat, Dec 31, 2016 at 04:12:51AM +0100, Michael Haggerty wrote:\n>> [...]\n>> -\tadjust_shared_perm(logfile->buf);\n>> -\tclose(logfd);\n>> +\tif (logfd >= 0) {\n>> +\t\tadjust_shared_perm(logfile->buf);\n>> +\t\tclose(logfd);\n>> +\t}\n>> +\n> \n> Hmm. I would have thought in the existing-logfile case that we would not\n> need to adjust_shared_perm(). But maybe we just do it anyway to pick up\n> potentially-changed config.\n\nI didn't change this aspect of the code's behavior (though I also found\nit a bit surprising).\n\nAnother thing I considered was changing adjust_shared_perm() to adjust\nthe file's permissions through the open file descriptor using fchmod().\nBut that function has a bunch of callers, and I didn't want to have to\nduplicate the code, nor did I have the energy to change all of its\ncallers (if that would even make sense for all callers, which I doubt).\n\n> [...]\n\nMichael\n\n"},{"id":"308546","messageId":"f5ced16d-61dc-ba14-7f29-88f20d4a65d2@alum.mit.edu","threadId":"44796","inReplyTo":"20161231063211.tqsiafg3iahcuotz@sigill.intra.peff.net","subject":"Re: [PATCH v3 13/23] log_ref_setup(): pass the open file descriptor back to the caller","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-12-31T07:58:43Z","receivedAt":"2016-12-31T07:58:54Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 12/31/2016 07:32 AM, Jeff King wrote:\n> On Sat, Dec 31, 2016 at 04:12:53AM +0100, Michael Haggerty wrote:\n> \n>> This function will most often be called by log_ref_write_1(), which\n>> wants to append to the reflog file. In that case, it is silly to close\n>> the file only for the caller to reopen it immediately. So, in the case\n>> that the file was opened, pass the open file descriptor back to the\n>> caller.\n> \n> Sounds like a much saner interface.\n> \n>> /*\n>> - * Create a reflog for a ref. If force_create = 0, the reflog will\n>> - * only be created for certain refs (those for which\n>> - * should_autocreate_reflog returns non-zero. Otherwise, create it\n>> - * regardless of the ref name. Fill in *err and return -1 on failure.\n>> + * Create a reflog for a ref. Store its path to *logfile. If\n>> + * force_create = 0, only create the reflog for certain refs (those\n>> + * for which should_autocreate_reflog returns non-zero). Otherwise,\n>> + * create it regardless of the reference name. If the logfile already\n>> + * existed or was created, return 0 and set *logfd to the file\n>> + * descriptor opened for appending to the file. If no logfile exists\n>> + * and we decided not to create one, return 0 and set *logfd to -1. On\n>> + * failure, fill in *err, set *logfd to -1, and return -1.\n>> */\n>> -static int log_ref_setup(const char *refname, struct strbuf *logfile, struct strbuf *err, int force_create)\n>> +static int log_ref_setup(const char *refname,\n>> +\t\t\t struct strbuf *logfile, int *logfd,\n>> +\t\t\t struct strbuf *err, int force_create)\n> \n> The return value is always \"0\" or \"-1\". It seems like it would be\n> simpler to just return the descriptor instead of 0.\n> \n> I guess that makes it hard to identify the case when we chose not to\n> create a descriptor. I wonder if more \"normal\" semantics would be:\n> \n> 1. ret >= 0: file existed or was created, and ret is the descriptor\n> \n> 2. ret < 0, err is empty: we chose not to create\n> \n> 3. ret < 0, err is non-empty: a real error\n\nI don't like making callers read err to find out whether the call was\nsuccessful, and I think we've been able to avoid that pattern so far.\n\nAnother alternative would be to make ret == -1 mean a real error and ret\n== -2 mean that we chose not to create the file. But that would be\nstraying from the usual \"-1 means error\" interface of open(), so I\nwasn't fond of that idea either.\n\n> I dunno. This may just be bikeshedding, and I can live with it either\n> way (especially because you documented it!).\n\nLet's see if anybody has a strong opinion about it; otherwise I'd rather\nleave it as is.\n\nMichael\n\n"},{"id":"308547","messageId":"de31c8f1-28b6-9c21-2e1b-1fa114add5ed@alum.mit.edu","threadId":"44796","inReplyTo":"20161231063523.fncqqpr3m42jjvbs@sigill.intra.peff.net","subject":"Re: [PATCH v3 14/23] log_ref_write_1(): don't depend on logfile argument","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2016-12-31T08:01:26Z","receivedAt":"2016-12-31T08:01:49Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 12/31/2016 07:35 AM, Jeff King wrote:\n> On Sat, Dec 31, 2016 at 04:12:54AM +0100, Michael Haggerty wrote:\n> \n>> It's unnecessary to pass a strbuf holding the reflog path up and down\n>> the call stack now that it is hardly needed by the callers. Remove the\n>> places where log_ref_write_1() uses it, in preparation for making it\n>> internal to log_ref_setup().\n>>\n>> Signed-off-by: Michael Haggerty \n>> ---\n>> refs/files-backend.c | 12 ++++++++----\n>> 1 file changed, 8 insertions(+), 4 deletions(-)\n>>\n>> diff --git a/refs/files-backend.c b/refs/files-backend.c\n>> index 7f26cf8..5a96424 100644\n>> --- a/refs/files-backend.c\n>> +++ b/refs/files-backend.c\n>> @@ -2837,14 +2837,18 @@ static int log_ref_write_1(const char *refname, const unsigned char *old_sha1,\n>> \tresult = log_ref_write_fd(logfd, old_sha1, new_sha1,\n>> \t\t\t\t git_committer_info(0), msg);\n>> \tif (result) {\n>> -\t\tstrbuf_addf(err, \"unable to append to '%s': %s\", logfile->buf,\n>> -\t\t\t strerror(errno));\n>> +\t\tint save_errno = errno;\n>> +\n>> +\t\tstrbuf_addf(err, \"unable to append to '%s': %s\",\n>> +\t\t\t git_path(\"logs/%s\", refname), strerror(save_errno));\n> \n> Hmm. This means the logic of \"the path for a reflog is\n> git_path(logs/%s)\" is now replicated in several places. Which feels kind\n> of like a backwards step. But I guess it is pretty well cemented in the\n> concept of files-backend.c, and I do like the later cleanups that this\n> allows.\n\nThis might end up in a helper function in the not-too-distant future for\nother reasons, but given that such code already appears multiple times,\nI didn't feel too guilty about it.\n\nMichael\n\n\n"},{"id":"308554","messageId":"20161231175808.cvm54nmk3x7zoipo@sigill.intra.peff.net","threadId":"44796","inReplyTo":"f5ced16d-61dc-ba14-7f29-88f20d4a65d2@alum.mit.edu","subject":"Re: [PATCH v3 13/23] log_ref_setup(): pass the open file descriptor back to the caller","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-12-31T17:58:08Z","receivedAt":"2016-12-31T17:58:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Dec 31, 2016 at 08:58:43AM +0100, Michael Haggerty wrote:\n\n> > The return value is always \"0\" or \"-1\". It seems like it would be\n> > simpler to just return the descriptor instead of 0.\n> > \n> > I guess that makes it hard to identify the case when we chose not to\n> > create a descriptor. I wonder if more \"normal\" semantics would be:\n> > \n> > 1. ret >= 0: file existed or was created, and ret is the descriptor\n> > \n> > 2. ret < 0, err is empty: we chose not to create\n> > \n> > 3. ret < 0, err is non-empty: a real error\n> \n> I don't like making callers read err to find out whether the call was\n> successful, and I think we've been able to avoid that pattern so far.\n\nI guess my mental model is that case 2 _is_ a failure, because we didn't\nopen the reflog. It's just one that callers may want to distinguish from\ncase 3, because it's probably a silent failure, not one we want to\ncomplain to the user about.\n\nBut whether that's accurate would depend on the callers. Looking at the\ncallers, I think the immediate callers would be happier with this, but\nyou probably would want to end up converting case 3 back to \"return 0\"\nout of files_log_ref_write().\n\n> > I dunno. This may just be bikeshedding, and I can live with it either\n> > way (especially because you documented it!).\n> \n> Let's see if anybody has a strong opinion about it; otherwise I'd rather\n> leave it as is.\n\nSounds good.\n\n-Peff\n"},{"id":"308561","messageId":"xmqq4m1j5y8y.fsf@gitster.mtv.corp.google.com","threadId":"44796","inReplyTo":"241de54c-63ee-d13c-c9fe-8b420871ac51@alum.mit.edu","subject":"Re: [PATCH v3 05/23] raceproof_create_file(): new function","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-01-01T02:07:25Z","receivedAt":"2017-01-01T02:07:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n>> But presumably you mean that we delete \"foo/bar/baz/xyzzy\", etc, up to\n>> \"foo/bar/baz\", provided they are all empty directories. I think your\n>> comment is probably OK and I was just being thick, but maybe stating it\n>> like:\n>> \n>> ...removes the directory if it is empty (and recursively any empty\n>> directories it contains) and calls the function again.\n>> \n>> would be more clear. That is still leaving the definition of \"empty\"\n>> implied, but it's hopefully obvious from the context.\n>\n> Yes, that's clearer. I'll change it. Thanks!\n\nThanks. Will tweak it in while queuing.\n"},{"id":"308562","messageId":"xmqqzijb4jky.fsf@gitster.mtv.corp.google.com","threadId":"44796","inReplyTo":"dba3d081c32854593d8113f9cd604a9891748bcd.1483153436.git.mhagger@alum.mit.edu","subject":"Re: [PATCH v3 10/23] log_ref_write(): inline function","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-01-01T02:09:33Z","receivedAt":"2017-01-01T02:09:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> This function doesn't do anything beyond call files_log_ref_write(), so\n\ns/call/&ing/; I think.\n\n> replace it with the latter at its call sites.\n>\n> Signed-off-by: Michael Haggerty \n> ---\n> refs/files-backend.c | 24 ++++++++++--------------\n> 1 file changed, 10 insertions(+), 14 deletions(-)\n>\n> diff --git a/refs/files-backend.c b/refs/files-backend.c\n> index 49a119c..fd8a751 100644\n> --- a/refs/files-backend.c\n> +++ b/refs/files-backend.c\n> @@ -2832,14 +2832,6 @@ static int log_ref_write_1(const char *refname, const unsigned char *old_sha1,\n> \treturn 0;\n> }\n> \n> -static int log_ref_write(const char *refname, const unsigned char *old_sha1,\n> -\t\t\t const unsigned char *new_sha1, const char *msg,\n> -\t\t\t int flags, struct strbuf *err)\n> -{\n> -\treturn files_log_ref_write(refname, old_sha1, new_sha1, msg, flags,\n> -\t\t\t\t err);\n> -}\n> -\n> int files_log_ref_write(const char *refname, const unsigned char *old_sha1,\n> \t\t\tconst unsigned char *new_sha1, const char *msg,\n> \t\t\tint flags, struct strbuf *err)\n> @@ -2903,7 +2895,8 @@ static int commit_ref_update(struct files_ref_store *refs,\n> \tassert_main_repository(&refs->base, \"commit_ref_update\");\n> \n> \tclear_loose_ref_cache(refs);\n> -\tif (log_ref_write(lock->ref_name, lock->old_oid.hash, sha1, logmsg, 0, err)) {\n> +\tif (files_log_ref_write(lock->ref_name, lock->old_oid.hash, sha1,\n> +\t\t\t\tlogmsg, 0, err)) {\n> \t\tchar *old_msg = strbuf_detach(err, NULL);\n> \t\tstrbuf_addf(err, \"cannot update the ref '%s': %s\",\n> \t\t\t lock->ref_name, old_msg);\n> @@ -2934,7 +2927,7 @@ static int commit_ref_update(struct files_ref_store *refs,\n> \t\tif (head_ref && (head_flag & REF_ISSYMREF) &&\n> \t\t !strcmp(head_ref, lock->ref_name)) {\n> \t\t\tstruct strbuf log_err = STRBUF_INIT;\n> -\t\t\tif (log_ref_write(\"HEAD\", lock->old_oid.hash, sha1,\n> +\t\t\tif (files_log_ref_write(\"HEAD\", lock->old_oid.hash, sha1,\n> \t\t\t\t\t logmsg, 0, &log_err)) {\n> \t\t\t\terror(\"%s\", log_err.buf);\n> \t\t\t\tstrbuf_release(&log_err);\n> @@ -2973,7 +2966,8 @@ static void update_symref_reflog(struct ref_lock *lock, const char *refname,\n> \tstruct strbuf err = STRBUF_INIT;\n> \tunsigned char new_sha1[20];\n> \tif (logmsg && !read_ref(target, new_sha1) &&\n> -\t log_ref_write(refname, lock->old_oid.hash, new_sha1, logmsg, 0, &err)) {\n> +\t files_log_ref_write(refname, lock->old_oid.hash, new_sha1,\n> +\t\t\t\tlogmsg, 0, &err)) {\n> \t\terror(\"%s\", err.buf);\n> \t\tstrbuf_release(&err);\n> \t}\n> @@ -3748,9 +3742,11 @@ static int files_transaction_commit(struct ref_store *ref_store,\n> \n> \t\tif (update->flags & REF_NEEDS_COMMIT ||\n> \t\t update->flags & REF_LOG_ONLY) {\n> -\t\t\tif (log_ref_write(lock->ref_name, lock->old_oid.hash,\n> -\t\t\t\t\t update->new_sha1,\n> -\t\t\t\t\t update->msg, update->flags, err)) {\n> +\t\t\tif (files_log_ref_write(lock->ref_name,\n> +\t\t\t\t\t\tlock->old_oid.hash,\n> +\t\t\t\t\t\tupdate->new_sha1,\n> +\t\t\t\t\t\tupdate->msg, update->flags,\n> +\t\t\t\t\t\terr)) {\n> \t\t\t\tchar *old_msg = strbuf_detach(err, NULL);\n> \n> \t\t\t\tstrbuf_addf(err, \"cannot update the ref '%s': %s\",\n"},{"id":"308563","messageId":"xmqqvatz4imu.fsf@gitster.mtv.corp.google.com","threadId":"44796","inReplyTo":"c7a89febcbf7bdffb44f8fdf63a43f11339a0289.1483153436.git.mhagger@alum.mit.edu","subject":"Re: [PATCH v3 21/23] try_remove_empty_parents(): don't accommodate consecutive slashes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-01-01T02:30:01Z","receivedAt":"2017-01-01T02:32:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> \"refname\" has already been checked by check_refname_format(), so it\n> cannot have consecutive slashes.\n\nIn the endgame state, this has two callers. Both use what came in\nthe transaction->updates[] array. Presumably \"has already been\nchecked by check_refname_format()\" says that whoever created entries\nin that array must have called the function, but it would be helpful\nto be more explicit here.\n\n> Signed-off-by: Michael Haggerty \n> ---\n> refs/files-backend.c | 5 ++---\n> 1 file changed, 2 insertions(+), 3 deletions(-)\n>\n> diff --git a/refs/files-backend.c b/refs/files-backend.c\n> index af5a0e2..397488e 100644\n> --- a/refs/files-backend.c\n> +++ b/refs/files-backend.c\n> @@ -2294,15 +2294,14 @@ static void try_remove_empty_parents(const char *refname)\n> \tfor (i = 0; i < 2; i++) { /* refs/{heads,tags,...}/ */\n> \t\twhile (*p && *p != '/')\n> \t\t\tp++;\n> -\t\t/* tolerate duplicate slashes; see check_refname_format() */\n> -\t\twhile (*p == '/')\n> +\t\tif (*p == '/')\n> \t\t\tp++;\n> \t}\n> \tq = buf.buf + buf.len;\n> \twhile (1) {\n> \t\twhile (q > p && *q != '/')\n> \t\t\tq--;\n> -\t\twhile (q > p && *(q-1) == '/')\n> +\t\tif (q > p && *(q-1) == '/')\n> \t\t\tq--;\n> \t\tif (q == p)\n> \t\t\tbreak;\n"},{"id":"308564","messageId":"xmqqr34n4ii8.fsf@gitster.mtv.corp.google.com","threadId":"44796","inReplyTo":"20161231064746.6bvis76p5x5ubc2b@sigill.intra.peff.net","subject":"Re: [PATCH v3 00/23] Delete directories left empty after ref deletion","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-01-01T02:32:47Z","receivedAt":"2017-01-01T02:32:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Sat, Dec 31, 2016 at 04:12:40AM +0100, Michael Haggerty wrote:\n>\n>> This is a re-roll of an old patch series. v1 [1] got some feedback,\n>> which I think was all addressed in v2 [2]. But it seems that v2 fell\n>> on the floor, and I didn't bother following up because it was in the\n>> same area of code that was undergoing heavy changes due to the\n>> pluggable reference backend work. Sorry for the long delay before\n>> getting back to it.\n>\n> I've read through the whole thing, and aside from a few very minor nits\n> (that I am not even sure are worth a re-roll), I didn't see anything\n> wrong. And the overall goal and approach seem obviously sound.\n>\n>> Michael Haggerty (23):\n>\n> I'll admit to being daunted by the number of patches, but it was quite a\n> pleasant and easy read. Thanks.\n>\n> -Peff\n\nThanks, both. These patches indeed were pleasant.\n"},{"id":"308566","messageId":"xmqqful34fww.fsf@gitster.mtv.corp.google.com","threadId":"44796","inReplyTo":"d9f96df1bb2d5b9a95388da04b770ea9f317c491.1483153436.git.mhagger@alum.mit.edu","subject":"Re: [PATCH v3 11/23] log_ref_setup(): separate code for create vs non-create","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-01-01T03:28:47Z","receivedAt":"2017-01-01T03:28:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> +\t\t\tif (errno == ENOENT || errno == EISDIR) {\n> +\t\t\t\t/*\n> +\t\t\t\t * The logfile doesn't already exist,\n> +\t\t\t\t * but that is not an error; it only\n> +\t\t\t\t * means that we won't write log\n> +\t\t\t\t * entries to it.\n> +\t\t\t\t */\n> +\t\t\t} else {\n\nIt may be valid C, but an\n\n\t{\n\t\t/*\n\t\t * an empty block without any statement,\n\t\t * not even a null statement.\n\t\t */\n\t}\n\nalways makes me a bit nervous. Have a line with a semicolon without\nanything else (other than the indent) at the end and it will read\nnicer, at least to me.\n"},{"id":"308568","messageId":"20170101055947.7b5jxih3wlprqcil@sigill.intra.peff.net","threadId":"44796","inReplyTo":"xmqqvatz4imu.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3 21/23] try_remove_empty_parents(): don't accommodate consecutive slashes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-01-01T05:59:48Z","receivedAt":"2017-01-01T05:59:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Dec 31, 2016 at 06:30:01PM -0800, Junio C Hamano wrote:\n\n> Michael Haggerty writes:\n> \n> > \"refname\" has already been checked by check_refname_format(), so it\n> > cannot have consecutive slashes.\n> \n> In the endgame state, this has two callers. Both use what came in\n> the transaction->updates[] array. Presumably \"has already been\n> checked by check_refname_format()\" says that whoever created entries\n> in that array must have called the function, but it would be helpful\n> to be more explicit here.\n\nHmm, yeah. This is called when we are deleting a ref, and I thought we\nexplicitly _didn't_ do check_refname_format() when deleting, so that\nfunny-named refs could be deleted. It's only is_refname_safe() that we\nmust pass.\n\nSo I have no idea if that's a problem in the code or not, but it is at\nleast not immediately obvious who is responsible for calling\ncheck_refname_format() here.\n\n-Peff\n"},{"id":"308569","messageId":"0b62220b-141f-2872-65f0-c20d6ec10ed7@alum.mit.edu","threadId":"44796","inReplyTo":"xmqqzijb4jky.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3 10/23] log_ref_write(): inline function","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-01-01T08:41:20Z","receivedAt":"2017-01-01T08:41:32Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 01/01/2017 03:09 AM, Junio C Hamano wrote:\n> Michael Haggerty writes:\n> \n>> This function doesn't do anything beyond call files_log_ref_write(), so\n> \n> s/call/&ing/; I think.\n\nThanks; will fix.\n\nMichael\n\n"},{"id":"308571","messageId":"544457a2-5226-feea-1a32-74dd58233df8@alum.mit.edu","threadId":"44796","inReplyTo":"xmqqful34fww.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3 11/23] log_ref_setup(): separate code for create vs non-create","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-01-01T08:45:42Z","receivedAt":"2017-01-01T08:45:51Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 01/01/2017 04:28 AM, Junio C Hamano wrote:\n> Michael Haggerty writes:\n> \n>> +\t\t\tif (errno == ENOENT || errno == EISDIR) {\n>> +\t\t\t\t/*\n>> +\t\t\t\t * The logfile doesn't already exist,\n>> +\t\t\t\t * but that is not an error; it only\n>> +\t\t\t\t * means that we won't write log\n>> +\t\t\t\t * entries to it.\n>> +\t\t\t\t */\n>> +\t\t\t} else {\n> \n> It may be valid C, but an\n> \n> \t{\n> \t\t/*\n> \t\t * an empty block without any statement,\n> \t\t * not even a null statement.\n> \t\t */\n> \t}\n> \n> always makes me a bit nervous. Have a line with a semicolon without\n> anything else (other than the indent) at the end and it will read\n> nicer, at least to me.\n\nThat's a form of superstition that I haven't encountered before ;-) but\nI'll change it.\n\nMichael\n\n"},{"id":"308572","messageId":"CA+P7+xqqVFvDKSCTrGVVdpZB_VHwGdZ3gFQzo+RQqCu0FpOsAQ@mail.gmail.com","threadId":"44796","inReplyTo":"xmqqr34n4ii8.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3 00/23] Delete directories left empty after ref deletion","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2017-01-01T09:24:57Z","receivedAt":"2017-01-01T09:25:26Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Sat, Dec 31, 2016 at 6:32 PM, Junio C Hamano wrote:\n> Jeff King writes:\n>\n>> On Sat, Dec 31, 2016 at 04:12:40AM +0100, Michael Haggerty wrote:\n>>\n>>> This is a re-roll of an old patch series. v1 [1] got some feedback,\n>>> which I think was all addressed in v2 [2]. But it seems that v2 fell\n>>> on the floor, and I didn't bother following up because it was in the\n>>> same area of code that was undergoing heavy changes due to the\n>>> pluggable reference backend work. Sorry for the long delay before\n>>> getting back to it.\n>>\n>> I've read through the whole thing, and aside from a few very minor nits\n>> (that I am not even sure are worth a re-roll), I didn't see anything\n>> wrong. And the overall goal and approach seem obviously sound.\n>>\n>>> Michael Haggerty (23):\n>>\n>> I'll admit to being daunted by the number of patches, but it was quite a\n>> pleasant and easy read. Thanks.\n>>\n>> -Peff\n>\n> Thanks, both. These patches indeed were pleasant.\n\nI do have one comment regarding this series. Is it ever possible for\nan older version of git to be running a process while a new version of\ngit which cleans up dirs runs? Is this expected? I just want to make\nsure we don't need to worry about that scenario since otherwise it\nmakes it much more challenge.\n\nMy thought as far as I understand it is that it is possible, because a\nuser COULD choose to run both this and an older version, but that it\nis unlikely in practice outside of a few developer boxes who\nperiodically switch between versions of git, and are unlikely to\nactually run multiple versions at exactly the same time.\n\nThanks,\nJake\n"},{"id":"308573","messageId":"CA+P7+xq9W_MZaSTw_QaHWEpiQU2u8+6=rr67CUkD4WBqtD=c+A@mail.gmail.com","threadId":"44796","inReplyTo":"CA+P7+xqqVFvDKSCTrGVVdpZB_VHwGdZ3gFQzo+RQqCu0FpOsAQ@mail.gmail.com","subject":"Re: [PATCH v3 00/23] Delete directories left empty after ref deletion","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2017-01-01T09:26:08Z","receivedAt":"2017-01-01T09:26:35Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Sun, Jan 1, 2017 at 1:24 AM, Jacob Keller wrote:\n> On Sat, Dec 31, 2016 at 6:32 PM, Junio C Hamano wrote:\n>> Jeff King writes:\n>>\n>>> On Sat, Dec 31, 2016 at 04:12:40AM +0100, Michael Haggerty wrote:\n>>>\n>>>> This is a re-roll of an old patch series. v1 [1] got some feedback,\n>>>> which I think was all addressed in v2 [2]. But it seems that v2 fell\n>>>> on the floor, and I didn't bother following up because it was in the\n>>>> same area of code that was undergoing heavy changes due to the\n>>>> pluggable reference backend work. Sorry for the long delay before\n>>>> getting back to it.\n>>>\n>>> I've read through the whole thing, and aside from a few very minor nits\n>>> (that I am not even sure are worth a re-roll), I didn't see anything\n>>> wrong. And the overall goal and approach seem obviously sound.\n>>>\n>>>> Michael Haggerty (23):\n>>>\n>>> I'll admit to being daunted by the number of patches, but it was quite a\n>>> pleasant and easy read. Thanks.\n>>>\n>>> -Peff\n>>\n>> Thanks, both. These patches indeed were pleasant.\n>\n> I do have one comment regarding this series. Is it ever possible for\n> an older version of git to be running a process while a new version of\n> git which cleans up dirs runs? Is this expected? I just want to make\n> sure we don't need to worry about that scenario since otherwise it\n> makes it much more challenge.\n>\n> My thought as far as I understand it is that it is possible, because a\n> user COULD choose to run both this and an older version, but that it\n> is unlikely in practice outside of a few developer boxes who\n> periodically switch between versions of git, and are unlikely to\n> actually run multiple versions at exactly the same time.\n>\n> Thanks,\n> Jake\n\nTo add to this, if it is possible, it might be worth merging the \"make\nourselves safer against a race\" first, and then waiting some time\nbefore merging the \"we are now safe to delete directories\". I am not\nyet convinced that it is necessary, but wanted to point it out so that\nsomeone more knowledgeable could explain why it is safe to do so.\n\nRegards,\nJake\n"},{"id":"308575","messageId":"xmqqd1g73w3i.fsf@gitster.mtv.corp.google.com","threadId":"44796","inReplyTo":"20161231175808.cvm54nmk3x7zoipo@sigill.intra.peff.net","subject":"Re: [PATCH v3 13/23] log_ref_setup(): pass the open file descriptor back to the caller","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-01-01T10:36:49Z","receivedAt":"2017-01-01T10:36:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Sat, Dec 31, 2016 at 08:58:43AM +0100, Michael Haggerty wrote:\n>\n>> > The return value is always \"0\" or \"-1\". It seems like it would be\n>> > simpler to just return the descriptor instead of 0.\n>> > \n>> > I guess that makes it hard to identify the case when we chose not to\n>> > create a descriptor. I wonder if more \"normal\" semantics would be:\n>> > \n>> > 1. ret >= 0: file existed or was created, and ret is the descriptor\n>> > \n>> > 2. ret < 0, err is empty: we chose not to create\n>> > \n>> > 3. ret < 0, err is non-empty: a real error\n>> \n>> I don't like making callers read err to find out whether the call was\n>> successful, and I think we've been able to avoid that pattern so far.\n>\n> I guess my mental model is that case 2 _is_ a failure, because we didn't\n> open the reflog. It's just one that callers may want to distinguish from\n> case 3, because it's probably a silent failure, not one we want to\n> complain to the user about.\n>\n> But whether that's accurate would depend on the callers. Looking at the\n> callers, I think the immediate callers would be happier with this, but\n> you probably would want to end up converting case 3 back to \"return 0\"\n> out of files_log_ref_write().\n>\n>> > I dunno. This may just be bikeshedding, and I can live with it either\n>> > way (especially because you documented it!).\n>> \n>> Let's see if anybody has a strong opinion about it; otherwise I'd rather\n>> leave it as is.\n>\n> Sounds good.\n\nFWIW, in my mental model, 2. is not a failure (\"we returned without\ncreating new log because that is what was asked by the user\"). A\ntrue failure case is \"we wanted to open but couldn't\".\n\nThe caller needs to be able to differentiate between these two cases\nbecause we get no usable fd out of the function in either case.\n\nI agree that we could cram the error status and the file descriptor\ninto a single return value as you two discussed, but I think what\nthe patch chose to do is easier to use from the callers' point of\nview. The caller can switch between the codepath to give an error\nmessage and the non-error codepath based on the return value, and in\nthe non-error codepath can choose what to do based on the value of\nlogfd.\n\nI do not mind \"all negative values mean there is no fd\" plus \"some\nnegative values are more special than others\" convention, and if a\npatch did that from the beginning, I certainly would not suggest to\nrewrite it to use the \"error status comes as a return value, file\ndescriptor is an out parameter\" convention; i.e. I personally do not\nsee much difference either way, so...\n"},{"id":"308576","messageId":"994A5650F89F4B0080530BF7FD1879FD@PhilipOakley","threadId":"44796","inReplyTo":"CA+P7+xqqVFvDKSCTrGVVdpZB_VHwGdZ3gFQzo+RQqCu0FpOsAQ@mail.gmail.com","subject":"Re: [PATCH v3 00/23] Delete directories left empty after ref deletion","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.org","sentAt":null,"receivedAt":"2017-01-01T12:43:46Z","isPatch":true,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"From: \"Jacob Keller\" \nSent: Sunday, January 01, 2017 9:24 AM\n> On Sat, Dec 31, 2016 at 6:32 PM, Junio C Hamano wrote:\n>> Jeff King writes:\n>>\n>>> On Sat, Dec 31, 2016 at 04:12:40AM +0100, Michael Haggerty wrote:\n>>>\n>>>> This is a re-roll of an old patch series. v1 [1] got some feedback,\n>>>> which I think was all addressed in v2 [2]. But it seems that v2 fell\n>>>> on the floor, and I didn't bother following up because it was in the\n>>>> same area of code that was undergoing heavy changes due to the\n>>>> pluggable reference backend work. Sorry for the long delay before\n>>>> getting back to it.\n>>>\n>>> I've read through the whole thing, and aside from a few very minor nits\n>>> (that I am not even sure are worth a re-roll), I didn't see anything\n>>> wrong. And the overall goal and approach seem obviously sound.\n>>>\n>>>> Michael Haggerty (23):\n>>>\n>>> I'll admit to being daunted by the number of patches, but it was quite a\n>>> pleasant and easy read. Thanks.\n>>>\n>>> -Peff\n>>\n>> Thanks, both. These patches indeed were pleasant.\n>\n> I do have one comment regarding this series. Is it ever possible for\n> an older version of git to be running a process while a new version of\n> git which cleans up dirs runs? Is this expected? I just want to make\n> sure we don't need to worry about that scenario since otherwise it\n> makes it much more challenge.\n\nIt is easily possible in the Windows environment where the install philosphy \nis different, and some external vendor tools may even bring in their own \ncopy of Git as well. There is also the Portable Git version, so the \npossibility of multiple versions running concurrently is there, though it is \non Windows...\n\nI certainly have a Git-for-Windows published version, and a recent SDK \nversion on my home machines.\n\n>\n> My thought as far as I understand it is that it is possible, because a\n> user COULD choose to run both this and an older version, but that it\n> is unlikely in practice outside of a few developer boxes who\n> periodically switch between versions of git, and are unlikely to\n> actually run multiple versions at exactly the same time.\n>\n> Thanks,\n> Jake\n>\nPhilip \n\n"},{"id":"308602","messageId":"CA+P7+xqxSpV4yOjE+Lv0kw19Kq6UAbcN_-7O3U_EeBoT7AOtfw@mail.gmail.com","threadId":"44796","inReplyTo":"994A5650F89F4B0080530BF7FD1879FD@PhilipOakley","subject":"Re: [PATCH v3 00/23] Delete directories left empty after ref deletion","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2017-01-01T20:36:11Z","receivedAt":"2017-01-01T20:36:39Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Sun, Jan 1, 2017 at 4:43 AM, Philip Oakley wrote:\n> From: \"Jacob Keller\" \n>> I do have one comment regarding this series. Is it ever possible for\n>> an older version of git to be running a process while a new version of\n>> git which cleans up dirs runs? Is this expected? I just want to make\n>> sure we don't need to worry about that scenario since otherwise it\n>> makes it much more challenge.\n>\n>\n> It is easily possible in the Windows environment where the install philosphy\n> is different, and some external vendor tools may even bring in their own\n> copy of Git as well. There is also the Portable Git version, so the\n> possibility of multiple versions running concurrently is there, though it is\n> on Windows...\n>\n> I certainly have a Git-for-Windows published version, and a recent SDK\n> version on my home machines.\n>\n>\n\nOk.\n\n>>\n>> My thought as far as I understand it is that it is possible, because a\n>> user COULD choose to run both this and an older version, but that it\n>> is unlikely in practice outside of a few developer boxes who\n>> periodically switch between versions of git, and are unlikely to\n>> actually run multiple versions at exactly the same time.\n>>\n>> Thanks,\n>> Jake\n>>\n> Philip\n\nBut how likely is it to end up with differing binaries running on the\nexact same repository concurrently? Basically, I am trying to see\nwhether or not we could accidentally end up causing problems by trying\nto race with other git processes that haven't yet been made safe\nagainst race? Is the worst case only that some git operation would\nfail and you would have to retry?\n\nThanks,\nJake\n"},{"id":"308603","messageId":"20170102041947.5jzx6og5fcpv7oso@sigill.intra.peff.net","threadId":"44796","inReplyTo":"CA+P7+xqxSpV4yOjE+Lv0kw19Kq6UAbcN_-7O3U_EeBoT7AOtfw@mail.gmail.com","subject":"Re: [PATCH v3 00/23] Delete directories left empty after ref deletion","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-01-02T04:19:47Z","receivedAt":"2017-01-02T04:19:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jan 01, 2017 at 12:36:11PM -0800, Jacob Keller wrote:\n\n> But how likely is it to end up with differing binaries running on the\n> exact same repository concurrently? Basically, I am trying to see\n> whether or not we could accidentally end up causing problems by trying\n> to race with other git processes that haven't yet been made safe\n> against race? Is the worst case only that some git operation would\n> fail and you would have to retry?\n\nYes, I think that is the worst case.\n\nA more likely scenario might be something like a server accepting pushes\nor other ref updates from both JGit and regular git (or maybe libgit2\nand regular git).\n\nIMHO it's not really worth worrying about too much. Certain esoteric\nsetups might have a slightly higher chance of a pretty obscure race\ncondition happening on a very busy repository. I hate to say \"eh, ship\nit, we'll see if anybody complains\". But I'd be surprised to get a\nsingle report about this.\n\n-Peff\n"},{"id":"308680","messageId":"9215973c-8db1-8f5a-2dc7-3a0137dd5c62@alum.mit.edu","threadId":"44796","inReplyTo":"20161231064053.prvlw6x6qprzkmw7@sigill.intra.peff.net","subject":"Re: [PATCH v3 20/23] try_remove_empty_parents(): don't trash argument contents","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-01-02T16:27:59Z","receivedAt":"2017-01-02T16:28:09Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 12/31/2016 07:40 AM, Jeff King wrote:\n> On Sat, Dec 31, 2016 at 04:13:00AM +0100, Michael Haggerty wrote:\n> \n>> It's bad manners and surprising and therefore error-prone.\n> \n> Agreed.\n> \n> I wondered while reading your earlier patch whether the\n> safe_create_leading_directories() function had the same problem, but it\n> carefully replaces the NUL it inserts.\n> \n>> -static void try_remove_empty_parents(char *refname)\n>> +static void try_remove_empty_parents(const char *refname)\n>> {\n>> +\tstruct strbuf buf = STRBUF_INIT;\n>> \tchar *p, *q;\n>> \tint i;\n>> -\tp = refname;\n>> +\n>> +\tstrbuf_addstr(&buf, refname);\n> \n> I see here you just make a copy. I think it would be enough to do:\n> \n>> @@ -2305,10 +2306,11 @@ static void try_remove_empty_parents(char *refname)\n>> \t\t\tq--;\n>> \t\tif (q == p)\n>> \t\t\tbreak;\n>> -\t\t*q = '\\0';\n>> -\t\tif (rmdir(git_path(\"%s\", refname)))\n>> +\t\tstrbuf_setlen(&buf, q - buf.buf);\n>> +\t\tif (rmdir(git_path(\"%s\", buf.buf)))\n>> \t\t\tbreak;\n> \n> *q = '\\0';\n> r = rmdir(git_path(\"%s\", refname));\n> *q = '/';\n> \n> if (r)\n> break;\n> \n> or something. But I doubt the single allocation is breaking the bank,\n> and it has the nice side effect that callers can pass in a const string\n> (I didn't check yet whether that enables further cleanups).\n\nThe last patch in the series passes ref_update::refname to this\nfunction, which is `const char *`. With your suggested change, either\nthat member would have to be made non-const, or it would have to be cast\nto const at the `try_remove_empty_parents()` callsite.\n\nMaking the member non-const would be easy, though it loses a tiny bit of\ndocumentation and safety against misuse. Also, scribbling even\ntemporarily over that member would mean that this code is not\nthread-safe (though it seems unlikely that we would ever bother making\nit multithreaded).\n\nI think I prefer the current version because it loosens the coupling\nbetween this function and its callers. But I don't mind either way if\nsomebody feels strongly about it.\n\nAs an aside, I wonder whether we would be discussing this at all if we\nhad stack-allocated strbufs that could be used without allocating heap\nmemory in the usual case.\n\nMichael\n\n"},{"id":"308682","messageId":"20170102171043.ebfeaaylf4waq6co@sigill.intra.peff.net","threadId":"44796","inReplyTo":"9215973c-8db1-8f5a-2dc7-3a0137dd5c62@alum.mit.edu","subject":"Re: [PATCH v3 20/23] try_remove_empty_parents(): don't trash argument contents","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-01-02T17:10:44Z","receivedAt":"2017-01-02T17:13:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jan 02, 2017 at 05:27:59PM +0100, Michael Haggerty wrote:\n\n> > or something. But I doubt the single allocation is breaking the bank,\n> > and it has the nice side effect that callers can pass in a const string\n> > (I didn't check yet whether that enables further cleanups).\n> \n> The last patch in the series passes ref_update::refname to this\n> function, which is `const char *`. With your suggested change, either\n> that member would have to be made non-const, or it would have to be cast\n> to const at the `try_remove_empty_parents()` callsite.\n> \n> Making the member non-const would be easy, though it loses a tiny bit of\n> documentation and safety against misuse. Also, scribbling even\n> temporarily over that member would mean that this code is not\n> thread-safe (though it seems unlikely that we would ever bother making\n> it multithreaded).\n> \n> I think I prefer the current version because it loosens the coupling\n> between this function and its callers. But I don't mind either way if\n> somebody feels strongly about it.\n\nOK, let's take what you have here, then.\n\n> As an aside, I wonder whether we would be discussing this at all if we\n> had stack-allocated strbufs that could be used without allocating heap\n> memory in the usual case.\n\nI'm not sure. We still pay the memcpy(), though I don't know how\nsubstantial that is compared to an allocation. For these small strings,\nprobably not very.\n\n-Peff\n"},{"id":"308683","messageId":"5051c78e-51f9-becd-e1a6-9c0b781d6912@alum.mit.edu","threadId":"44796","inReplyTo":"20170101055947.7b5jxih3wlprqcil@sigill.intra.peff.net","subject":"Re: [PATCH v3 21/23] try_remove_empty_parents(): don't accommodate consecutive slashes","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-01-02T18:06:32Z","receivedAt":"2017-01-02T18:07:12Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 01/01/2017 06:59 AM, Jeff King wrote:\n> On Sat, Dec 31, 2016 at 06:30:01PM -0800, Junio C Hamano wrote:\n> \n>> Michael Haggerty writes:\n>>\n>>> \"refname\" has already been checked by check_refname_format(), so it\n>>> cannot have consecutive slashes.\n>>\n>> In the endgame state, this has two callers. Both use what came in\n>> the transaction->updates[] array. Presumably \"has already been\n>> checked by check_refname_format()\" says that whoever created entries\n>> in that array must have called the function, but it would be helpful\n>> to be more explicit here.\n> \n> Hmm, yeah. This is called when we are deleting a ref, and I thought we\n> explicitly _didn't_ do check_refname_format() when deleting, so that\n> funny-named refs could be deleted. It's only is_refname_safe() that we\n> must pass.\n> \n> So I have no idea if that's a problem in the code or not, but it is at\n> least not immediately obvious who is responsible for calling\n> check_refname_format() here.\n\nMy assumption was that only valid reference names should ever be allowed\nto be inserted into a `ref_transaction` entry. But Peff is right that\nsometimes the reference name is checked by `refname_is_safe()` rather\nthan `check_refname_format()`. Let's audit this more carefully...\n\n* `ref_transaction_add_update()` relies on its caller doing the check\n (this fact is documented). Its callers are:\n * `ref_transaction_update()` (the usual codepath), which checks the\n reference itself using either check_refname_format() or\n refname_is_safe() depending on what kind of update it is.\n * `split_head_update()` passes the literal string \"HEAD\".\n * `split_symref_update()` picks apart reference updates that go\n through existing symbolic references. As such I don't think they\n are an attack surface. It doesn't do any checking itself (here\n we're talking about its `referent` argument). It has only one\n caller:\n * `lock_ref_for_update()`, which gets `referent` from:\n * `files_read_raw_ref()`, which gets the value either:\n * by reading a filesystem-level symlink's contents and\n checking it with `check_refname_format()`, or\n * reading a symref from the filesystem. In this case, *the\n value is not checked*.\n\nObviously this chain of custody is disconcertingly long and complicated.\nAnd the gap for symrefs should probably be fixed, even though they are\nhopefully trustworthy.\n\n`refname_is_safe()` checks that its argument is either UPPER_CASE or\nlooks like a plausible filename under \"refs/\". Contrary to its\ndocstring, it *does not* accept strings that contain successive slashes\nor \".\" or \"..\" components. It was made stricter in\n\n e40f355 \"refname_is_safe(): insist that the refname already be\nnormalized\", 2016-04-27\n\n, but the docstring wasn't updated at that time. I'll fix it.\n\nI think the best thing to do is to drop this patch from the patch\nseries, and address these checks in a separate series. (There are more\nknown problems in this area, for example that the checks in\n`check_refname_format()` are not a strict superset of the checks in\n`refname_is_safe()`.)\n\nMichael\n\n"},{"id":"308684","messageId":"ab92dc60-c623-0f6f-868b-b74b1d6dbd2e@alum.mit.edu","threadId":"44796","inReplyTo":"20170102041947.5jzx6og5fcpv7oso@sigill.intra.peff.net","subject":"Re: [PATCH v3 00/23] Delete directories left empty after ref deletion","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-01-02T18:14:03Z","receivedAt":"2017-01-02T18:15:12Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 01/02/2017 05:19 AM, Jeff King wrote:\n> On Sun, Jan 01, 2017 at 12:36:11PM -0800, Jacob Keller wrote:\n> \n>> But how likely is it to end up with differing binaries running on the\n>> exact same repository concurrently? Basically, I am trying to see\n>> whether or not we could accidentally end up causing problems by trying\n>> to race with other git processes that haven't yet been made safe\n>> against race? Is the worst case only that some git operation would\n>> fail and you would have to retry?\n> \n> Yes, I think that is the worst case.\n> \n> A more likely scenario might be something like a server accepting pushes\n> or other ref updates from both JGit and regular git (or maybe libgit2\n> and regular git).\n> \n> IMHO it's not really worth worrying about too much. Certain esoteric\n> setups might have a slightly higher chance of a pretty obscure race\n> condition happening on a very busy repository. I hate to say \"eh, ship\n> it, we'll see if anybody complains\". But I'd be surprised to get a\n> single report about this.\n\nI agree. I think these races would mostly affect busy hosting sites and\nresult in failed updates rather than corruption. And the races can\nalready occur whenever the user runs `git pack-refs`. By contrast, the\nfailure to delete empty directories is more likely to affect normal users.\n\nThat being said, if Junio wants to merge all but the last two patches in\none release, then merge the last two a release or two later, I have no\nobjections.\n\nMichael\n\n"},{"id":"308687","messageId":"20170102182639.c53flvqdkb4kjuuh@sigill.intra.peff.net","threadId":"44796","inReplyTo":"5051c78e-51f9-becd-e1a6-9c0b781d6912@alum.mit.edu","subject":"Re: [PATCH v3 21/23] try_remove_empty_parents(): don't accommodate consecutive slashes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-01-02T18:26:39Z","receivedAt":"2017-01-02T18:28:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jan 02, 2017 at 07:06:32PM +0100, Michael Haggerty wrote:\n\n> My assumption was that only valid reference names should ever be allowed\n> to be inserted into a `ref_transaction` entry. But Peff is right that\n> sometimes the reference name is checked by `refname_is_safe()` rather\n> than `check_refname_format()`. Let's audit this more carefully...\n> \n> * `ref_transaction_add_update()` relies on its caller doing the check\n> (this fact is documented). Its callers are:\n> * `ref_transaction_update()` (the usual codepath), which checks the\n> reference itself using either check_refname_format() or\n> refname_is_safe() depending on what kind of update it is.\n> * `split_head_update()` passes the literal string \"HEAD\".\n> * `split_symref_update()` picks apart reference updates that go\n> through existing symbolic references. As such I don't think they\n> are an attack surface. It doesn't do any checking itself (here\n> we're talking about its `referent` argument). It has only one\n> caller:\n> * `lock_ref_for_update()`, which gets `referent` from:\n> * `files_read_raw_ref()`, which gets the value either:\n> * by reading a filesystem-level symlink's contents and\n> checking it with `check_refname_format()`, or\n> * reading a symref from the filesystem. In this case, *the\n> value is not checked*.\n> \n> Obviously this chain of custody is disconcertingly long and complicated.\n> And the gap for symrefs should probably be fixed, even though they are\n> hopefully trustworthy.\n\nThanks as always for a careful analysis. I agree it seems like a bug\nthat symlinks are checked but symrefs are not.\n\n> I think the best thing to do is to drop this patch from the patch\n> series, and address these checks in a separate series. (There are more\n> known problems in this area, for example that the checks in\n> `check_refname_format()` are not a strict superset of the checks in\n> `refname_is_safe()`.)\n\nSounds like a good plan. I'd be very happy if the \"superset\" mismatch is\nfixed. It seems like it has come up in our discussions more than once.\n\n-Peff\n"},{"id":"308690","messageId":"CA+P7+xrL6TCQGs3b0s_z97hHN_FbSzLKgUtd7tDT_uACECm=tw@mail.gmail.com","threadId":"44796","inReplyTo":"ab92dc60-c623-0f6f-868b-b74b1d6dbd2e@alum.mit.edu","subject":"Re: [PATCH v3 00/23] Delete directories left empty after ref deletion","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2017-01-02T18:54:33Z","receivedAt":"2017-01-02T18:55:55Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Mon, Jan 2, 2017 at 10:14 AM, Michael Haggerty wrote:\n> On 01/02/2017 05:19 AM, Jeff King wrote:\n>> On Sun, Jan 01, 2017 at 12:36:11PM -0800, Jacob Keller wrote:\n>>\n>>> But how likely is it to end up with differing binaries running on the\n>>> exact same repository concurrently? Basically, I am trying to see\n>>> whether or not we could accidentally end up causing problems by trying\n>>> to race with other git processes that haven't yet been made safe\n>>> against race? Is the worst case only that some git operation would\n>>> fail and you would have to retry?\n>>\n>> Yes, I think that is the worst case.\n>>\n>> A more likely scenario might be something like a server accepting pushes\n>> or other ref updates from both JGit and regular git (or maybe libgit2\n>> and regular git).\n>>\n>> IMHO it's not really worth worrying about too much. Certain esoteric\n>> setups might have a slightly higher chance of a pretty obscure race\n>> condition happening on a very busy repository. I hate to say \"eh, ship\n>> it, we'll see if anybody complains\". But I'd be surprised to get a\n>> single report about this.\n>\n> I agree. I think these races would mostly affect busy hosting sites and\n> result in failed updates rather than corruption. And the races can\n> already occur whenever the user runs `git pack-refs`. By contrast, the\n> failure to delete empty directories is more likely to affect normal users.\n>\n> That being said, if Junio wants to merge all but the last two patches in\n> one release, then merge the last two a release or two later, I have no\n> objections.\n>\n> Michael\n>\n\nI only wanted to make sure that the failure mode would not result in\ncorruption. I believe that both you and Peff have alleviated my fears.\n\nThanks,\nJake\n"}]}