{"thread":{"id":"36591","subject":"[PATCH v2 0/2] add a reflog_exists and delete_reflog abstraction","startedAt":"2014-05-06T22:45:51Z","lastAt":"2014-05-07T20:17:55Z","messageCount":5,"participants":["Ronnie Sahlberg","Michael Haggerty","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":2},"messages":[{"id":"240851","messageId":"1399416353-31817-1-git-send-email-sahlberg@google.com","threadId":"36591","inReplyTo":null,"subject":"[PATCH v2 0/2] add a reflog_exists and delete_reflog abstraction","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-05-06T22:45:51Z","receivedAt":"2014-05-06T22:45:51Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"This is a series adds two new functions to try to hide the reflog\nimplementation details from the callers in checkout.c and reflog.c.\nIt adds new functions to test if a reflog exists and to delete it, thus\nallowing checkout.c to perform this if-test-then-delete operation without\nhaving to know the internal implementation of reflogs (i.e. that they are files\nthat live under .git/logs)\n\nAdditionally we change checkout.c to use ref_exists instead of file_exists\nwhen checking for ref existence. This fixes a bug when checkout could delete\na valid reflog file if the branch was a packed ref. The tests have been updated\nto test for this bug.\n\n\nVersion 2:\n - Typos and fixes suggested by mhagger.\n - Break the checkout-deletes reflog bugfix out into a separate patch.\n\n\nRonnie Sahlberg (2):\n refs.c: add new functions reflog_exists and delete_reflog\n checkout.c: use ref_exists instead of file_exist\n\n builtin/checkout.c | 8 ++------\n builtin/reflog.c | 2 +-\n refs.c | 21 +++++++++++++++------\n refs.h | 6 ++++++\n t/t1410-reflog.sh | 8 ++++++++\n 5 files changed, 32 insertions(+), 13 deletions(-)\n\n-- \n2.0.0.rc1.354.g7561c2b.dirty\n"},{"id":"240853","messageId":"1399416353-31817-2-git-send-email-sahlberg@google.com","threadId":"36591","inReplyTo":"1399416353-31817-1-git-send-email-sahlberg@google.com","subject":"[PATCH v2 1/2] refs.c: add new functions reflog_exists and delete_reflog","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-05-06T22:45:52Z","receivedAt":"2014-05-06T22:45:52Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Add two new functions, reflog_exists and delete_reflog, to hide the internal\nreflog implementation (that they are files under .git/logs/...) from callers.\nUpdate checkout.c to use these functions in update_refs_for_switch instead of\nbuilding pathnames and calling out to file access functions. Update reflog.c\nto use these to check if the reflog exists. Now there are still many places\nin reflog.c where we are still leaking the reflog storage implementation but\nthis at least reduces the number of such dependencies by one. Finally\nchange two places in refs.c itself to use the new function to check if a ref\nexists or not isntead of build-path-and-stat(). Now, this is strictly not all\nthat important since these are in parts of refs that are implementing the\nactual file storage backend but on the other hand it will not hurt either.\n\nSigned-off-by: Ronnie Sahlberg \n---\n builtin/checkout.c | 7 +++----\n builtin/reflog.c | 2 +-\n refs.c | 21 +++++++++++++++------\n refs.h | 6 ++++++\n 4 files changed, 25 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex ff44921..929f5bd 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -651,12 +651,11 @@ static void update_refs_for_switch(const struct checkout_opts *opts,\n \t\t\t}\n \t\t}\n \t\tif (old->path && old->name) {\n-\t\t\tchar log_file[PATH_MAX], ref_file[PATH_MAX];\n+\t\t\tchar ref_file[PATH_MAX];\n \n-\t\t\tgit_snpath(log_file, sizeof(log_file), \"logs/%s\", old->path);\n \t\t\tgit_snpath(ref_file, sizeof(ref_file), \"%s\", old->path);\n-\t\t\tif (!file_exists(ref_file) && file_exists(log_file))\n-\t\t\t\tremove_path(log_file);\n+\t\t\tif (!file_exists(ref_file) && reflog_exists(old->path))\n+\t\t\t\tdelete_reflog(old->path);\n \t\t}\n \t}\n \tremove_branch_state();\ndiff --git a/builtin/reflog.c b/builtin/reflog.c\nindex c12a9784..e8a8fb1 100644\n--- a/builtin/reflog.c\n+++ b/builtin/reflog.c\n@@ -369,7 +369,7 @@ static int expire_reflog(const char *ref, const unsigned char *sha1, int unused,\n \tif (!lock)\n \t\treturn error(\"cannot lock ref '%s'\", ref);\n \tlog_file = git_pathdup(\"logs/%s\", ref);\n-\tif (!file_exists(log_file))\n+\tif (!reflog_exists(ref))\n \t\tgoto finish;\n \tif (!cmd->dry_run) {\n \t\tnewlog_path = git_pathdup(\"logs/%s.lock\", ref);\ndiff --git a/refs.c b/refs.c\nindex e59bc18..302a2b3 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2013,7 +2013,6 @@ int dwim_log(const char *str, int len, unsigned char *sha1, char **log)\n \n \t*log = NULL;\n \tfor (p = ref_rev_parse_rules; *p; p++) {\n-\t\tstruct stat st;\n \t\tunsigned char hash[20];\n \t\tchar path[PATH_MAX];\n \t\tconst char *ref, *it;\n@@ -2022,12 +2021,9 @@ int dwim_log(const char *str, int len, unsigned char *sha1, char **log)\n \t\tref = resolve_ref_unsafe(path, hash, 1, NULL);\n \t\tif (!ref)\n \t\t\tcontinue;\n-\t\tif (!stat(git_path(\"logs/%s\", path), &st) &&\n-\t\t S_ISREG(st.st_mode))\n+\t\tif (reflog_exists(path))\n \t\t\tit = path;\n-\t\telse if (strcmp(ref, path) &&\n-\t\t\t !stat(git_path(\"logs/%s\", ref), &st) &&\n-\t\t\t S_ISREG(st.st_mode))\n+\t\telse if (strcmp(ref, path) && reflog_exists(ref))\n \t\t\tit = ref;\n \t\telse\n \t\t\tcontinue;\n@@ -3030,6 +3026,19 @@ int read_ref_at(const char *refname, unsigned long at_time, int cnt,\n \treturn 1;\n }\n \n+int reflog_exists(const char *refname)\n+{\n+\tstruct stat st;\n+\n+\treturn !lstat(git_path(\"logs/%s\", refname), &st) &&\n+\t\tS_ISREG(st.st_mode);\n+}\n+\n+int delete_reflog(const char *refname)\n+{\n+\treturn remove_path(git_path(\"logs/%s\", refname));\n+}\n+\n static int show_one_reflog_ent(struct strbuf *sb, each_reflog_ent_fn fn, void *cb_data)\n {\n \tunsigned char osha1[20], nsha1[20];\ndiff --git a/refs.h b/refs.h\nindex c1cb4b4..8bd815d 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -159,6 +159,12 @@ extern int read_ref_at(const char *refname, unsigned long at_time, int cnt,\n \t\t unsigned char *sha1, char **msg,\n \t\t unsigned long *cutoff_time, int *cutoff_tz, int *cutoff_cnt);\n \n+/** Check if a particular reflog exists */\n+extern int reflog_exists(const char *refname);\n+\n+/** Delete a reflog */\n+extern int delete_reflog(const char *refname);\n+\n /* iterate over reflog entries */\n typedef int each_reflog_ent_fn(unsigned char *osha1, unsigned char *nsha1, const char *, unsigned long, int, const char *, void *);\n int for_each_reflog_ent(const char *refname, each_reflog_ent_fn fn, void *cb_data);\n-- \n2.0.0.rc1.354.g7561c2b.dirty\n"},{"id":"240852","messageId":"1399416353-31817-3-git-send-email-sahlberg@google.com","threadId":"36591","inReplyTo":"1399416353-31817-1-git-send-email-sahlberg@google.com","subject":"[PATCH v2 2/2] checkout.c: use ref_exists instead of file_exist","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-05-06T22:45:53Z","receivedAt":"2014-05-06T22:45:53Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Change checkout.c to check if a ref exists instead of checking if a loose ref\nfile exists when deciding if to delete an orphaned log file. Otherwise, if a\nref only exists as a packed ref without a corresponding loose ref for the\ncurrently checked out branch, we risk that the reflog will be deleted when we\nswitch to a different branch.\n\nUpdate the reflog tests to check for this bug.\n\nThe following reproduces the bug:\n$ git init-db\n$ git config core.logallrefupdates true\n$ git commit -m Initial --allow-empty\n [master (root-commit) bb11abe] Initial\n$ git reflog master\n [8561dcb master@{0}: commit (initial): Initial]\n$ find .git/{refs,logs} -type f | grep master\n [.git/refs/heads/master]\n [.git/logs/refs/heads/master]\n$ git branch foo\n$ git pack-refs --all\n$ find .git/{refs,logs} -type f | grep master\n [.git/logs/refs/heads/master]\n$ git checkout foo\n$ find .git/{refs,logs} -type f | grep master\n ... reflog file is missing ...\n$ git reflog master\n ... nothing ...\n\nSigned-off-by: Ronnie Sahlberg \n---\n builtin/checkout.c | 5 +----\n t/t1410-reflog.sh | 8 ++++++++\n 2 files changed, 9 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 929f5bd..f1dc56e 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -651,10 +651,7 @@ static void update_refs_for_switch(const struct checkout_opts *opts,\n \t\t\t}\n \t\t}\n \t\tif (old->path && old->name) {\n-\t\t\tchar ref_file[PATH_MAX];\n-\n-\t\t\tgit_snpath(ref_file, sizeof(ref_file), \"%s\", old->path);\n-\t\t\tif (!file_exists(ref_file) && reflog_exists(old->path))\n+\t\t\tif (!ref_exists(old->path) && reflog_exists(old->path))\n \t\t\t\tdelete_reflog(old->path);\n \t\t}\n \t}\ndiff --git a/t/t1410-reflog.sh b/t/t1410-reflog.sh\nindex 236b13a..8cab06f 100755\n--- a/t/t1410-reflog.sh\n+++ b/t/t1410-reflog.sh\n@@ -245,4 +245,12 @@ test_expect_success 'gc.reflogexpire=false' '\n \n '\n \n+test_expect_success 'checkout should not delete log for packed ref' '\n+\ttest $(git reflog master | wc -l) = 4 &&\n+\tgit branch foo &&\n+\tgit pack-refs --all &&\n+\tgit checkout foo &&\n+\ttest $(git reflog master | wc -l) = 4\n+'\n+\n test_done\n-- \n2.0.0.rc1.354.g7561c2b.dirty\n"},{"id":"240885","messageId":"536A1F84.8020902@alum.mit.edu","threadId":"36591","inReplyTo":"1399416353-31817-1-git-send-email-sahlberg@google.com","subject":"Re: [PATCH v2 0/2] add a reflog_exists and delete_reflog abstraction","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-05-07T11:56:52Z","receivedAt":"2014-05-07T11:56:52Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 05/07/2014 12:45 AM, Ronnie Sahlberg wrote:\n> This is a series adds two new functions to try to hide the reflog\n> implementation details from the callers in checkout.c and reflog.c.\n> It adds new functions to test if a reflog exists and to delete it, thus\n> allowing checkout.c to perform this if-test-then-delete operation without\n> having to know the internal implementation of reflogs (i.e. that they are files\n> that live under .git/logs)\n> \n> Additionally we change checkout.c to use ref_exists instead of file_exists\n> when checking for ref existence. This fixes a bug when checkout could delete\n> a valid reflog file if the branch was a packed ref. The tests have been updated\n> to test for this bug.\n> \n> \n> Version 2:\n> - Typos and fixes suggested by mhagger.\n> - Break the checkout-deletes reflog bugfix out into a separate patch.\n> \n> \n> Ronnie Sahlberg (2):\n> refs.c: add new functions reflog_exists and delete_reflog\n> checkout.c: use ref_exists instead of file_exist\n> \n> builtin/checkout.c | 8 ++------\n> builtin/reflog.c | 2 +-\n> refs.c | 21 +++++++++++++++------\n> refs.h | 6 ++++++\n> t/t1410-reflog.sh | 8 ++++++++\n> 5 files changed, 32 insertions(+), 13 deletions(-)\n\n+1 Looks good to me. Thanks!\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"240944","messageId":"xmqqzjit8hsc.fsf@gitster.dls.corp.google.com","threadId":"36591","inReplyTo":"536A1F84.8020902@alum.mit.edu","subject":"Re: [PATCH v2 0/2] add a reflog_exists and delete_reflog abstraction","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-05-07T20:17:55Z","receivedAt":"2014-05-07T20:17:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> +1 Looks good to me. Thanks!\n\nWill queue with your Ack; thanks.\n"}]}