{"thread":{"id":"44963","subject":"[PATCH] tag: add tag.createReflog option","startedAt":"2017-01-25T00:19:43Z","lastAt":"2017-01-31T22:02:44Z","messageCount":33,"participants":["cornelius.weig@tngtech.com","Pranit Bauva","Jeff King","Junio C Hamano","Cornelius Weig"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"310127","messageId":"20170125001906.13916-1-cornelius.weig@tngtech.com","threadId":"44963","inReplyTo":null,"subject":"[PATCH] tag: add tag.createReflog option","fromName":"","fromEmail":"cornelius.weig@tngtech.com","sentAt":"2017-01-25T00:19:06Z","receivedAt":"2017-01-25T00:19:43Z","isPatch":true,"sender":{"key":"cornelius.weig@tngtech.com","avatar":null},"body":"From: Cornelius Weig \n\nGit does not create a history for tags, in contrast to common\nexpectation to simply version everything. This can be changed by using\nthe `--create-reflog` flag when creating the tag. However, a config\noption to enable this behavior by default is missing.\n\nThis commit adds the configuration variable `tag.createReflog` which\nenables reflogs for new tags by default.\n\nSigned-off-by: Cornelius Weig \n---\n Documentation/config.txt | 5 +++++\n Documentation/git-tag.txt | 8 +++++---\n builtin/tag.c | 6 +++++-\n t/t7004-tag.sh | 14 ++++++++++++++\n 4 files changed, 29 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex af2ae4c..9e5f6f6 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -2945,6 +2945,11 @@ submodule.alternateErrorStrategy\n \tas computed via `submodule.alternateLocation`. Possible values are\n \t`ignore`, `info`, `die`. Default is `die`.\n \n+tag.createReflog::\n+\tA boolean to specify whether newly created tags should have a reflog.\n+\tIf `--[no-]create-reflog` is specified on the command line, it takes\n+\tprecedence. Defaults to `false`.\n+\n tag.forceSignAnnotated::\n \tA boolean to specify whether annotated tags created should be GPG signed.\n \tIf `--annotate` is specified on the command line, it takes\ndiff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt\nindex 5055a96..f2ed370 100644\n--- a/Documentation/git-tag.txt\n+++ b/Documentation/git-tag.txt\n@@ -13,7 +13,7 @@ SYNOPSIS\n \t [ | ]\n 'git tag' -d ...\n 'git tag' [-n[]] -l [--contains ] [--points-at ]\n-\t[--column[=] | --no-column] [--create-reflog] [--sort=]\n+\t[--column[=] | --no-column] [--[no-]create-reflog] [--sort=]\n \t[--format=] [--[no-]merged []] [...]\n 'git tag' -v ...\n \n@@ -149,8 +149,10 @@ This option is only applicable when listing tags without annotation lines.\n \tall, 'whitespace' removes just leading/trailing whitespace lines and\n \t'strip' removes both whitespace and commentary.\n \n---create-reflog::\n-\tCreate a reflog for the tag.\n+--[no-]create-reflog::\n+\tForce to create a reflog for the tag, or no reflog if `--no-create-reflog`\n+\tis used. Unless the `tag.createReflog` config variable is set to true, no\n+\treflog is created by default. See linkgit:git-config[1].\n \n ::\n \tThe name of the tag to create, delete, or describe.\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex 73df728..1f13e4d 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -30,6 +30,7 @@ static const char * const git_tag_usage[] = {\n \n static unsigned int colopts;\n static int force_sign_annotate;\n+static int create_reflog;\n \n static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting, const char *format)\n {\n@@ -165,6 +166,10 @@ static int git_tag_config(const char *var, const char *value, void *cb)\n \t\tforce_sign_annotate = git_config_bool(var, value);\n \t\treturn 0;\n \t}\n+\tif (!strcmp(var, \"tag.createreflog\")) {\n+\t\tcreate_reflog = git_config_bool(var, value);\n+\t\treturn 0;\n+\t}\n \n \tif (starts_with(var, \"column.\"))\n \t\treturn git_column_config(var, value, \"tag\", &colopts);\n@@ -325,7 +330,6 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \tconst char *object_ref, *tag;\n \tstruct create_tag_options opt;\n \tchar *cleanup_arg = NULL;\n-\tint create_reflog = 0;\n \tint annotate = 0, force = 0;\n \tint cmdmode = 0, create_tag_object = 0;\n \tconst char *msgfile = NULL, *keyid = NULL;\ndiff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\nindex 1cfa8a2..67b39ec 100755\n--- a/t/t7004-tag.sh\n+++ b/t/t7004-tag.sh\n@@ -90,6 +90,20 @@ test_expect_success '--create-reflog does not create reflog on failure' '\n \ttest_must_fail git reflog exists refs/tags/mytag\n '\n \n+test_expect_success 'option tag.createreflog creates reflog by default' '\n+\ttest_when_finished \"git tag -d tag_with_reflog\" &&\n+\tgit config tag.createReflog true &&\n+\tgit tag tag_with_reflog &&\n+\tgit reflog exists refs/tags/tag_with_reflog\n+'\n+\n+test_expect_success 'option tag.createreflog overridden by command line' '\n+\ttest_when_finished \"git tag -d tag_without_reflog\" &&\n+\tgit config tag.createReflog true &&\n+\tgit tag --no-create-reflog tag_without_reflog &&\n+\ttest_must_fail git reflog exists refs/tags/tag_without_reflog\n+'\n+\n test_expect_success 'listing all tags if one exists should succeed' '\n \tgit tag -l &&\n \tgit tag\n-- \n2.10.2\n\n"},{"id":"310137","messageId":"CAFZEwPM9MWHQAie8KT+4LKdk-tHVJBCLP46HNcLJm5PHM=AWQg@mail.gmail.com","threadId":"44963","inReplyTo":"20170125001906.13916-1-cornelius.weig@tngtech.com","subject":"Re: [PATCH] tag: add tag.createReflog option","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2017-01-25T05:06:24Z","receivedAt":"2017-01-25T05:06:31Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"Hey Cornelius,\n\nOn Wed, Jan 25, 2017 at 5:49 AM, wrote:\n> From: Cornelius Weig \n>\n> Git does not create a history for tags, in contrast to common\n> expectation to simply version everything. This can be changed by using\n> the `--create-reflog` flag when creating the tag. However, a config\n> option to enable this behavior by default is missing.\n>\n> This commit adds the configuration variable `tag.createReflog` which\n> enables reflogs for new tags by default.\n>\n> Signed-off-by: Cornelius Weig \n\nYou have also added the option --no-create-reflog so would it be worth\nto mention it in the commit message?\n> ---\n> Documentation/config.txt | 5 +++++\n> Documentation/git-tag.txt | 8 +++++---\n> builtin/tag.c | 6 +++++-\n> t/t7004-tag.sh | 14 ++++++++++++++\n> 4 files changed, 29 insertions(+), 4 deletions(-)\n>\n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> index af2ae4c..9e5f6f6 100644\n> --- a/Documentation/config.txt\n> +++ b/Documentation/config.txt\n> @@ -2945,6 +2945,11 @@ submodule.alternateErrorStrategy\n> as computed via `submodule.alternateLocation`. Possible values are\n> `ignore`, `info`, `die`. Default is `die`.\n>\n> +tag.createReflog::\n> + A boolean to specify whether newly created tags should have a reflog.\n> + If `--[no-]create-reflog` is specified on the command line, it takes\n> + precedence. Defaults to `false`.\n\nThis follows the convention, good! :)\n\n> tag.forceSignAnnotated::\n> A boolean to specify whether annotated tags created should be GPG signed.\n> If `--annotate` is specified on the command line, it takes\n> diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt\n> index 5055a96..f2ed370 100644\n> --- a/Documentation/git-tag.txt\n> +++ b/Documentation/git-tag.txt\n> @@ -13,7 +13,7 @@ SYNOPSIS\n> [ | ]\n> 'git tag' -d ...\n> 'git tag' [-n[]] -l [--contains ] [--points-at ]\n> - [--column[=] | --no-column] [--create-reflog] [--sort=]\n> + [--column[=] | --no-column] [--[no-]create-reflog] [--sort=]\n> [--format=] [--[no-]merged []] [...]\n> 'git tag' -v ...\n>\n> @@ -149,8 +149,10 @@ This option is only applicable when listing tags without annotation lines.\n> all, 'whitespace' removes just leading/trailing whitespace lines and\n> 'strip' removes both whitespace and commentary.\n>\n> ---create-reflog::\n> - Create a reflog for the tag.\n> +--[no-]create-reflog::\n> + Force to create a reflog for the tag, or no reflog if `--no-create-reflog`\n> + is used. Unless the `tag.createReflog` config variable is set to true, no\n> + reflog is created by default. See linkgit:git-config[1].\n>\n> ::\n> The name of the tag to create, delete, or describe.\n> diff --git a/builtin/tag.c b/builtin/tag.c\n> index 73df728..1f13e4d 100644\n> --- a/builtin/tag.c\n> +++ b/builtin/tag.c\n> @@ -30,6 +30,7 @@ static const char * const git_tag_usage[] = {\n>\n> static unsigned int colopts;\n> static int force_sign_annotate;\n> +static int create_reflog;\n>\n> static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting, const char *format)\n> {\n> @@ -165,6 +166,10 @@ static int git_tag_config(const char *var, const char *value, void *cb)\n> force_sign_annotate = git_config_bool(var, value);\n> return 0;\n> }\n> + if (!strcmp(var, \"tag.createreflog\")) {\n> + create_reflog = git_config_bool(var, value);\n> + return 0;\n> + }\n>\n> if (starts_with(var, \"column.\"))\n> return git_column_config(var, value, \"tag\", &colopts);\n> @@ -325,7 +330,6 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n> const char *object_ref, *tag;\n> struct create_tag_options opt;\n> char *cleanup_arg = NULL;\n> - int create_reflog = 0;\n> int annotate = 0, force = 0;\n> int cmdmode = 0, create_tag_object = 0;\n> const char *msgfile = NULL, *keyid = NULL;\n> diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\n> index 1cfa8a2..67b39ec 100755\n> --- a/t/t7004-tag.sh\n> +++ b/t/t7004-tag.sh\n> @@ -90,6 +90,20 @@ test_expect_success '--create-reflog does not create reflog on failure' '\n> test_must_fail git reflog exists refs/tags/mytag\n> '\n>\n> +test_expect_success 'option tag.createreflog creates reflog by default' '\n> + test_when_finished \"git tag -d tag_with_reflog\" &&\n> + git config tag.createReflog true &&\n> + git tag tag_with_reflog &&\n> + git reflog exists refs/tags/tag_with_reflog\n> +'\n> +\n> +test_expect_success 'option tag.createreflog overridden by command line' '\n> + test_when_finished \"git tag -d tag_without_reflog\" &&\n> + git config tag.createReflog true &&\n> + git tag --no-create-reflog tag_without_reflog &&\n> + test_must_fail git reflog exists refs/tags/tag_without_reflog\n> +'\n> +\n> test_expect_success 'listing all tags if one exists should succeed' '\n> git tag -l &&\n> git tag\n> --\n> 2.10.2\n>\n"},{"id":"310170","messageId":"20170125180054.7mioop2o6uvqloyt@sigill.intra.peff.net","threadId":"44963","inReplyTo":"20170125001906.13916-1-cornelius.weig@tngtech.com","subject":"Re: [PATCH] tag: add tag.createReflog option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-01-25T18:00:54Z","receivedAt":"2017-01-25T18:01:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 25, 2017 at 01:19:06AM +0100, cornelius.weig@tngtech.com wrote:\n\n> From: Cornelius Weig \n> \n> Git does not create a history for tags, in contrast to common\n> expectation to simply version everything. This can be changed by using\n> the `--create-reflog` flag when creating the tag. However, a config\n> option to enable this behavior by default is missing.\n\nHmm, I didn't even know we had \"tag --create-reflog\". Looks like it was\nadded by 144c76fa39 (update-ref and tag: add --create-reflog arg,\n2015-07-21).\n\nIMHO it is a mistake. The \"update-ref --create-reflog\" variant makes\nsense to me as a plumbing operation. But are there end users who want to\ncreate a reflog for just _one_ tag?\n\nAs your patch shows, the more likely variant is \"I want reflogs for all\ntags\". But that raises two questions with your patch:\n\n - yours isn't \"reflogs for all tags\". It's \"reflogs for tags I created\n with git-tag\". What about other operations that create tags, like\n fetching (or even just a script that uses update-ref under the\n hood).\n\n IOW, instead of tag.createReflog, should this be tweaing\n core.logallrefupdates to have a mode that includes tags?\n\n - Is that the end of it, or is the desire really \"I want reflogs for\n _everything_\"? That seems like a sane thing to want.\n\n If so, then the update to core.logallrefupdates should turn it into\n a tri-state:\n\n - false; no reflogs\n\n - true; reflogs for branches, remotes, notes, as now\n\n - always; reflogs for all refs under \"refs/\"\n\nI made a lot of suppositions about your desires there, so maybe you\nreally do want just tag.createReflog. But \"core.logallrefupdates =\nalways\" sounds a lot more useful to me.\n\n-Peff\n"},{"id":"310171","messageId":"xmqq8tpzc8oq.fsf@gitster.mtv.corp.google.com","threadId":"44963","inReplyTo":"20170125180054.7mioop2o6uvqloyt@sigill.intra.peff.net","subject":"Re: [PATCH] tag: add tag.createReflog option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-01-25T18:10:13Z","receivedAt":"2017-01-25T18:10:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I made a lot of suppositions about your desires there, so maybe you\n> really do want just tag.createReflog. But \"core.logallrefupdates =\n> always\" sounds a lot more useful to me.\n\nThanks for saving me from typing exactly the same thing ;-)\n"},{"id":"310209","messageId":"00712f81-e0ba-52e6-77bc-095a2ed706c4@tngtech.com","threadId":"44963","inReplyTo":"20170125180054.7mioop2o6uvqloyt@sigill.intra.peff.net","subject":"Re: [PATCH] tag: add tag.createReflog option","fromName":"Cornelius Weig","fromEmail":"cornelius.weig@tngtech.com","sentAt":"2017-01-25T21:21:48Z","receivedAt":"2017-01-25T21:21:57Z","isPatch":true,"sender":{"key":"cornelius.weig@tngtech.com","avatar":null},"body":"On 01/25/2017 07:00 PM, Jeff King wrote:\n\n> - Is that the end of it, or is the desire really \"I want reflogs for\n> _everything_\"? That seems like a sane thing to want.\n> \n> If so, then the update to core.logallrefupdates should turn it into\n> a tri-state:\n> \n> - false; no reflogs\n> \n> - true; reflogs for branches, remotes, notes, as now\n> \n> - always; reflogs for all refs under \"refs/\"\n> \n\nI think you nailed it. This is much more useful than what I suggested.\nI'll see if I can code it up.\n"},{"id":"310213","messageId":"20170125213328.meehgxvzuajjgvag@sigill.intra.peff.net","threadId":"44963","inReplyTo":"00712f81-e0ba-52e6-77bc-095a2ed706c4@tngtech.com","subject":"Re: [PATCH] tag: add tag.createReflog option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-01-25T21:33:29Z","receivedAt":"2017-01-25T21:33:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 25, 2017 at 10:21:48PM +0100, Cornelius Weig wrote:\n\n> On 01/25/2017 07:00 PM, Jeff King wrote:\n> \n> > - Is that the end of it, or is the desire really \"I want reflogs for\n> > _everything_\"? That seems like a sane thing to want.\n> > \n> > If so, then the update to core.logallrefupdates should turn it into\n> > a tri-state:\n> > \n> > - false; no reflogs\n> > \n> > - true; reflogs for branches, remotes, notes, as now\n> > \n> > - always; reflogs for all refs under \"refs/\"\n> > \n> \n> I think you nailed it. This is much more useful than what I suggested.\n> I'll see if I can code it up.\n\nI cheated a little. I actually wrote the \"always\" patch 3 years ago as\npart of another thing I was working on. But in the end I didn't need it,\nand never submitted it.\n\nThe patch is below for reference. I have no idea whether it even applies\nnow, let alone runs and does the right thing. But perhaps you can\nsalvage bits of it (but feel free to ignore it if it makes things\nharder).\n\n-- >8 --\nFrom: Jeff King \nDate: Mon, 15 Apr 2013 23:31:05 -0400\nSubject: [PATCH] teach core.logallrefupdates an \"always\" mode\n\nWhen core.logallrefupdates is true, we only create a new\nreflog for refs that are under certain well-known\nhierarchies. The reason is that we know that some\nhierarchies (like refs/tags) do not typically change, and\nthat unknown hierarchies might not want reflogs at all\n(e.g., a hypothetical refs/foo might be meant to change\noften and drop old history immediately).\n\nHowever, sometimes it is useful to override this decision\nand simply log for all refs, because the safety and audit\ntrail is more important than the performance implications of\nkeeping the log around.\n\nThis patch introduces a new \"always\" mode for the\ncore.logallrefupdates option which will log updates to\neverything under refs/, regardless where in the hierarchy it\nis (we still will not log things like ORIG_HEAD and\nFETCH_HEAD, which are known to be transient).\n\nSigned-off-by: Jeff King \n---\n Documentation/config.txt | 8 +++++---\n branch.c | 2 +-\n builtin/checkout.c | 2 +-\n builtin/init-db.c | 2 +-\n cache.h | 9 ++++++++-\n config.c | 7 ++++++-\n environment.c | 2 +-\n refs.c | 23 +++++++++++++++++------\n t/t1400-update-ref.sh | 32 ++++++++++++++++++++++++++++++++\n 9 files changed, 72 insertions(+), 15 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex e37ba94a72..cb72e559ec 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -390,10 +390,12 @@ core.logAllRefUpdates::\n \t\"$GIT_DIR/logs/\", by appending the new and old\n \tSHA1, the date/time and the reason of the update, but\n \tonly when the file exists. If this configuration\n-\tvariable is set to true, missing \"$GIT_DIR/logs/\"\n+\tvariable is set to `true`, a missing \"$GIT_DIR/logs/\"\n \tfile is automatically created for branch heads (i.e. under\n-\trefs/heads/), remote refs (i.e. under refs/remotes/),\n-\tnote refs (i.e. under refs/notes/), and the symbolic ref HEAD.\n+\t`refs/heads/`), remote refs (i.e. under `refs/remotes/`),\n+\tnote refs (i.e. under `refs/notes/`), and the symbolic ref `HEAD`.\n+\tIf it is set to `always`, then a missing reflog is automatically\n+\tcreated for any ref under `refs/`.\n +\n This information can be used to determine what commit\n was the tip of a branch \"2 days ago\".\ndiff --git a/branch.c b/branch.c\nindex 2bef1e7e71..c11880b181 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -259,7 +259,7 @@ void create_branch(const char *head,\n \t}\n \n \tif (reflog)\n-\t\tlog_all_ref_updates = 1;\n+\t\tlog_all_ref_updates = LOG_REFS_NORMAL;\n \n \tif (forcing)\n \t\tsnprintf(msg, sizeof msg, \"branch: Reset to %s\",\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex a9c1b5a95f..00e231d83b 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -564,7 +564,7 @@ static void update_refs_for_switch(const struct checkout_opts *opts,\n \t\t\t\tchar *ref_name = mkpath(\"refs/heads/%s\", opts->new_orphan_branch);\n \n \t\t\t\ttemp = log_all_ref_updates;\n-\t\t\t\tlog_all_ref_updates = 1;\n+\t\t\t\tlog_all_ref_updates = LOG_REFS_NORMAL;\n \t\t\t\tif (log_ref_setup(ref_name, log_file, sizeof(log_file))) {\n \t\t\t\t\tfprintf(stderr, _(\"Can not do reflog for '%s'\\n\"),\n \t\t\t\t\t opts->new_orphan_branch);\ndiff --git a/builtin/init-db.c b/builtin/init-db.c\nindex 78aa3872dd..0ebad0b37d 100644\n--- a/builtin/init-db.c\n+++ b/builtin/init-db.c\n@@ -264,7 +264,7 @@ static int create_default_files(const char *template_path)\n \t\tconst char *work_tree = get_git_work_tree();\n \t\tgit_config_set(\"core.bare\", \"false\");\n \t\t/* allow template config file to override the default */\n-\t\tif (log_all_ref_updates == -1)\n+\t\tif (log_all_ref_updates == LOG_REFS_UNSET)\n \t\t git_config_set(\"core.logallrefupdates\", \"true\");\n \t\tif (prefixcmp(git_dir, work_tree) ||\n \t\t strcmp(git_dir + strlen(work_tree), \"/.git\")) {\ndiff --git a/cache.h b/cache.h\nindex 2b192d24ac..d2bfabc67f 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -536,7 +536,6 @@ extern int minimum_abbrev, default_abbrev;\n extern int ignore_case;\n extern int assume_unchanged;\n extern int prefer_symlink_refs;\n-extern int log_all_ref_updates;\n extern int warn_ambiguous_refs;\n extern int shared_repository;\n extern const char *apply_default_whitespace;\n@@ -556,6 +555,14 @@ extern int core_preload_index;\n extern int core_apply_sparse_checkout;\n extern int precomposed_unicode;\n \n+enum log_refs_config {\n+\tLOG_REFS_UNSET = -1,\n+\tLOG_REFS_NONE = 0,\n+\tLOG_REFS_NORMAL, /* see should_create_reflog for rules */\n+\tLOG_REFS_ALWAYS\n+};\n+extern enum log_refs_config log_all_ref_updates;\n+\n enum branch_track {\n \tBRANCH_TRACK_UNSPECIFIED = -1,\n \tBRANCH_TRACK_NEVER = 0,\ndiff --git a/config.c b/config.c\nindex b5696354fa..ffb892c0a0 100644\n--- a/config.c\n+++ b/config.c\n@@ -601,7 +601,12 @@ static int git_default_core_config(const char *var, const char *value)\n \t}\n \n \tif (!strcmp(var, \"core.logallrefupdates\")) {\n-\t\tlog_all_ref_updates = git_config_bool(var, value);\n+\t\tif (value && !strcasecmp(value, \"always\"))\n+\t\t\tlog_all_ref_updates = LOG_REFS_ALWAYS;\n+\t\telse if (git_config_bool(var, value))\n+\t\t\tlog_all_ref_updates = LOG_REFS_NORMAL;\n+\t\telse\n+\t\t\tlog_all_ref_updates = LOG_REFS_NONE;\n \t\treturn 0;\n \t}\n \ndiff --git a/environment.c b/environment.c\nindex 85edd7f95a..1867d31d75 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -19,7 +19,7 @@ int ignore_case;\n int assume_unchanged;\n int prefer_symlink_refs;\n int is_bare_repository_cfg = -1; /* unspecified */\n-int log_all_ref_updates = -1; /* unspecified */\n+enum log_refs_config log_all_ref_updates = LOG_REFS_UNSET;\n int warn_ambiguous_refs = 1;\n int repository_format_version;\n const char *git_commit_encoding;\ndiff --git a/refs.c b/refs.c\nindex 541fec2065..3cd203ef87 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1896,7 +1896,7 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms\n \n \tlock->force_write = 1;\n \tflag = log_all_ref_updates;\n-\tlog_all_ref_updates = 0;\n+\tlog_all_ref_updates = LOG_REFS_NONE;\n \tif (write_ref_sha1(lock, orig_sha1, NULL))\n \t\terror(\"unable to write current sha1 into %s\", oldrefname);\n \tlog_all_ref_updates = flag;\n@@ -1965,16 +1965,27 @@ static int copy_msg(char *buf, const char *msg)\n \treturn cp - buf;\n }\n \n+int should_create_reflog(const char *refname)\n+{\n+\tswitch (log_all_ref_updates) {\n+\tcase LOG_REFS_ALWAYS:\n+\t\treturn 1;\n+\tcase LOG_REFS_NORMAL:\n+\t\treturn !prefixcmp(refname, \"refs/heads/\") ||\n+\t\t !prefixcmp(refname, \"refs/remotes/\") ||\n+\t\t !prefixcmp(refname, \"refs/notes/\") ||\n+\t\t !strcmp(refname, \"HEAD\");\n+\tdefault:\n+\t\treturn 0;\n+\t}\n+}\n+\n int log_ref_setup(const char *refname, char *logfile, int bufsize)\n {\n \tint logfd, oflags = O_APPEND | O_WRONLY;\n \n \tgit_snpath(logfile, bufsize, \"logs/%s\", refname);\n-\tif (log_all_ref_updates &&\n-\t (!prefixcmp(refname, \"refs/heads/\") ||\n-\t !prefixcmp(refname, \"refs/remotes/\") ||\n-\t !prefixcmp(refname, \"refs/notes/\") ||\n-\t !strcmp(refname, \"HEAD\"))) {\n+\tif (should_create_reflog(refname)) {\n \t\tif (safe_create_leading_directories(logfile) < 0)\n \t\t\treturn error(\"unable to create directory for %s\",\n \t\t\t\t logfile);\ndiff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh\nindex e415ee0bbf..f60196d294 100755\n--- a/t/t1400-update-ref.sh\n+++ b/t/t1400-update-ref.sh\n@@ -302,4 +302,36 @@ test_expect_success \\\n \t'git cat-file blob master@{2005-05-26 23:42}:F (expect OTHER)' \\\n \t'test OTHER = $(git cat-file blob \"master@{2005-05-26 23:42}:F\")'\n \n+test_expect_success 'core.logAllRefUpdates=true does not log refs/foo/' '\n+\ttest_config core.logAllRefUpdates true &&\n+\ttest_commit log-true &&\n+\tgit update-ref -m reflog-message refs/heads/logme HEAD &&\n+\tgit update-ref -m reflog-message refs/foo/logme HEAD &&\n+\t{\n+\t\techo \"refs/heads/logme@{0} reflog-message\"\n+\t} >expect &&\n+\t{\n+\t\tgit log -g -1 --format=\"%gD %gs\" refs/heads/logme &&\n+\t\tgit log -g -1 --format=\"%gD %gs\" refs/foo/logme\n+\t} >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'core.logAllRefUpdates=always logs refs/foo/' '\n+\ttest_config core.logAllRefUpdates always &&\n+\ttest_commit log-always &&\n+\tgit update-ref -m reflog-message refs/heads/logme HEAD &&\n+\tgit update-ref -m reflog-message refs/foo/logme HEAD &&\n+\t{\n+\t\techo \"refs/heads/logme@{0} reflog-message\"\n+\t\techo \"refs/foo/logme@{0} reflog-message\"\n+\t} >expect &&\n+\t{\n+\t\tgit log -g -1 --format=\"%gD %gs\" refs/heads/logme &&\n+\t\tgit log -g -1 --format=\"%gD %gs\" refs/foo/logme\n+\t} >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+\n test_done\n-- \n2.11.0.840.gd37c5973a\n\n"},{"id":"310218","messageId":"xmqqpoja95o5.fsf@gitster.mtv.corp.google.com","threadId":"44963","inReplyTo":"20170125213328.meehgxvzuajjgvag@sigill.intra.peff.net","subject":"Re: [PATCH] tag: add tag.createReflog option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-01-25T21:43:38Z","receivedAt":"2017-01-25T21:55:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> +enum log_refs_config {\n> +\tLOG_REFS_UNSET = -1,\n> +\tLOG_REFS_NONE = 0,\n> +\tLOG_REFS_NORMAL, /* see should_create_reflog for rules */\n> +\tLOG_REFS_ALWAYS\n> +};\n> +extern enum log_refs_config log_all_ref_updates;\n> +...\n> +int should_create_reflog(const char *refname)\n> +{\n> +\tswitch (log_all_ref_updates) {\n> +\tcase LOG_REFS_ALWAYS:\n> +\t\treturn 1;\n> +\tcase LOG_REFS_NORMAL:\n> +\t\treturn !prefixcmp(refname, \"refs/heads/\") ||\n> +\t\t !prefixcmp(refname, \"refs/remotes/\") ||\n> +\t\t !prefixcmp(refname, \"refs/notes/\") ||\n> +\t\t !strcmp(refname, \"HEAD\");\n> +\tdefault:\n> +\t\treturn 0;\n> +\t}\n> +}\n\nYup, this is how I expected for the feature to be done.\n\nJust a hint for Cornelius; prefixcmp() is an old name for what is\ncalled starts_with() these days.\n"},{"id":"310252","messageId":"xmqqy3xy7nq0.fsf@gitster.mtv.corp.google.com","threadId":"44963","inReplyTo":"xmqqpoja95o5.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] tag: add tag.createReflog option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-01-25T22:56:39Z","receivedAt":"2017-01-25T22:56:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Jeff King writes:\n>\n>> +enum log_refs_config {\n>> +\tLOG_REFS_UNSET = -1,\n>> +\tLOG_REFS_NONE = 0,\n>> +\tLOG_REFS_NORMAL, /* see should_create_reflog for rules */\n>> +\tLOG_REFS_ALWAYS\n>> +};\n>> +extern enum log_refs_config log_all_ref_updates;\n>> +...\n>> +int should_create_reflog(const char *refname)\n>> +{\n>> +\tswitch (log_all_ref_updates) {\n>> +\tcase LOG_REFS_ALWAYS:\n>> +\t\treturn 1;\n>> +\tcase LOG_REFS_NORMAL:\n>> +\t\treturn !prefixcmp(refname, \"refs/heads/\") ||\n>> +\t\t !prefixcmp(refname, \"refs/remotes/\") ||\n>> +\t\t !prefixcmp(refname, \"refs/notes/\") ||\n>> +\t\t !strcmp(refname, \"HEAD\");\n>> +\tdefault:\n>> +\t\treturn 0;\n>> +\t}\n>> +}\n>\n> Yup, this is how I expected for the feature to be done.\n>\n> Just a hint for Cornelius; prefixcmp() is an old name for what is\n> called starts_with() these days.\n\nIt may have been obvious, but to be explicit for somebody new,\n!prefixcmp() corresponds to starts_with(). IOW, we changed the\nmeaning of the return value when moving from cmp-lookalike (where 0\nmeans \"equal\") to \"does it start with this string?\" bool (where 1\nmeans \"yes\").\n"},{"id":"310265","messageId":"5553094d-db87-4a04-cf02-5405b92a6224@tngtech.com","threadId":"44963","inReplyTo":"xmqqy3xy7nq0.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] tag: add tag.createReflog option","fromName":"Cornelius Weig","fromEmail":"cornelius.weig@tngtech.com","sentAt":"2017-01-25T23:40:45Z","receivedAt":"2017-01-25T23:40:53Z","isPatch":true,"sender":{"key":"cornelius.weig@tngtech.com","avatar":null},"body":"> \n> It may have been obvious, but to be explicit for somebody new,\n> !prefixcmp() corresponds to starts_with(). IOW, we changed the\n> meaning of the return value when moving from cmp-lookalike (where 0\n> means \"equal\") to \"does it start with this string?\" bool (where 1\n> means \"yes\").\n> \n\nI see. It reads much better that way!\n\nI re-did all the changes from Jeff's patch, but some tests are breaking\nnow. I will have to mend that tomorrow, because it's already too late in\nmy timezone.\n\nThanks a lot for your support m(_ _)m\n\n"},{"id":"310278","messageId":"20170126011654.21729-1-cornelius.weig@tngtech.com","threadId":"44963","inReplyTo":"20170125213328.meehgxvzuajjgvag@sigill.intra.peff.net","subject":"[PATCH] refs: add option core.logAllRefUpdates = always","fromName":"","fromEmail":"cornelius.weig@tngtech.com","sentAt":"2017-01-26T01:16:53Z","receivedAt":"2017-01-26T01:17:32Z","isPatch":true,"sender":{"key":"cornelius.weig@tngtech.com","avatar":null},"body":"Hi peff,\n\n you made it easy for me. Most of your patch still applied, only the tests\ndidn't quite fit. Maybe you can have a look if I've overlooked something, since\nyou know the changes best?\n\nThanks for supporting this with your patch!\n\n"},{"id":"310279","messageId":"20170126011654.21729-2-cornelius.weig@tngtech.com","threadId":"44963","inReplyTo":"20170126011654.21729-1-cornelius.weig@tngtech.com","subject":"[PATCH] refs: add option core.logAllRefUpdates = always","fromName":"","fromEmail":"cornelius.weig@tngtech.com","sentAt":"2017-01-26T01:16:54Z","receivedAt":"2017-01-26T01:17:35Z","isPatch":true,"sender":{"key":"cornelius.weig@tngtech.com","avatar":null},"body":"From: Cornelius Weig \n\nWhen core.logallrefupdates is true, we only create a new reflog for refs\nthat are under certain well-known hierarchies. The reason is that we\nknow that some hierarchies (like refs/tags) do not typically change, and\nthat unknown hierarchies might not want reflogs at all (e.g., a\nhypothetical refs/foo might be meant to change often and drop old\nhistory immediately).\n\nHowever, sometimes it is useful to override this decision and simply log\nfor all refs, because the safety and audit trail is more important than\nthe performance implications of keeping the log around.\n\nThis patch introduces a new \"always\" mode for the core.logallrefupdates\noption which will log updates to everything under refs/, regardless\nwhere in the hierarchy it is (we still will not log things like\nORIG_HEAD and FETCH_HEAD, which are known to be transient).\n\nBased-on-patch-by: Jeff King \nSigned-off-by: Cornelius Weig \n---\n Documentation/config.txt | 7 +++++--\n Documentation/git-tag.txt | 3 ++-\n branch.c | 2 +-\n builtin/init-db.c | 2 +-\n cache.h | 9 +++++++-\n config.c | 7 ++++++-\n environment.c | 2 +-\n refs.c | 15 +++++++++-----\n refs/files-backend.c | 6 +++---\n t/t1400-update-ref.sh | 53 +++++++++++++++++++++++++++++++++++++++++++++++\n 10 files changed, 90 insertions(+), 16 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex af2ae4c..2117616 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -517,10 +517,13 @@ core.logAllRefUpdates::\n \t\"`$GIT_DIR/logs/`\", by appending the new and old\n \tSHA-1, the date/time and the reason of the update, but\n \tonly when the file exists. If this configuration\n-\tvariable is set to true, missing \"`$GIT_DIR/logs/`\"\n+\tvariable is set to `true`, missing \"`$GIT_DIR/logs/`\"\n \tfile is automatically created for branch heads (i.e. under\n \trefs/heads/), remote refs (i.e. under refs/remotes/),\n-\tnote refs (i.e. under refs/notes/), and the symbolic ref HEAD.\n+\t`refs/heads/`), remote refs (i.e. under `refs/remotes/`),\n+\tnote refs (i.e. under `refs/notes/`), and the symbolic ref `HEAD`.\n+\tIf it is set to `always`, then a missing reflog is automatically\n+\tcreated for any ref under `refs/`.\n +\n This information can be used to determine what commit\n was the tip of a branch \"2 days ago\".\ndiff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt\nindex 5055a96..2ac25a9 100644\n--- a/Documentation/git-tag.txt\n+++ b/Documentation/git-tag.txt\n@@ -150,7 +150,8 @@ This option is only applicable when listing tags without annotation lines.\n \t'strip' removes both whitespace and commentary.\n \n --create-reflog::\n-\tCreate a reflog for the tag.\n+\tCreate a reflog for the tag. To globally enable reflogs for tags, see\n+\t`core.logAllRefUpdates` in linkgit:git-config[1].\n \n ::\n \tThe name of the tag to create, delete, or describe.\ndiff --git a/branch.c b/branch.c\nindex c431cbf..b955d4f 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -298,7 +298,7 @@ void create_branch(const char *name, const char *start_name,\n \t\t\t start_name);\n \n \tif (reflog)\n-\t\tlog_all_ref_updates = 1;\n+\t\tlog_all_ref_updates = LOG_REFS_NORMAL;\n \n \tif (!dont_change_ref) {\n \t\tstruct ref_transaction *transaction;\ndiff --git a/builtin/init-db.c b/builtin/init-db.c\nindex 76d68fa..1d4d6a0 100644\n--- a/builtin/init-db.c\n+++ b/builtin/init-db.c\n@@ -262,7 +262,7 @@ static int create_default_files(const char *template_path,\n \t\tconst char *work_tree = get_git_work_tree();\n \t\tgit_config_set(\"core.bare\", \"false\");\n \t\t/* allow template config file to override the default */\n-\t\tif (log_all_ref_updates == -1)\n+\t\tif (log_all_ref_updates == LOG_REFS_UNSET)\n \t\t\tgit_config_set(\"core.logallrefupdates\", \"true\");\n \t\tif (needs_work_tree_config(original_git_dir, work_tree))\n \t\t\tgit_config_set(\"core.worktree\", work_tree);\ndiff --git a/cache.h b/cache.h\nindex 00a029a..96eeaaf 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -660,7 +660,6 @@ extern int minimum_abbrev, default_abbrev;\n extern int ignore_case;\n extern int assume_unchanged;\n extern int prefer_symlink_refs;\n-extern int log_all_ref_updates;\n extern int warn_ambiguous_refs;\n extern int warn_on_object_refname_ambiguity;\n extern const char *apply_default_whitespace;\n@@ -728,6 +727,14 @@ enum hide_dotfiles_type {\n };\n extern enum hide_dotfiles_type hide_dotfiles;\n \n+enum log_refs_config {\n+\tLOG_REFS_UNSET = -1,\n+\tLOG_REFS_NONE = 0,\n+\tLOG_REFS_NORMAL,\n+\tLOG_REFS_ALWAYS\n+};\n+extern enum log_refs_config log_all_ref_updates;\n+\n enum branch_track {\n \tBRANCH_TRACK_UNSPECIFIED = -1,\n \tBRANCH_TRACK_NEVER = 0,\ndiff --git a/config.c b/config.c\nindex b680f79..c6b874a 100644\n--- a/config.c\n+++ b/config.c\n@@ -826,7 +826,12 @@ static int git_default_core_config(const char *var, const char *value)\n \t}\n \n \tif (!strcmp(var, \"core.logallrefupdates\")) {\n-\t\tlog_all_ref_updates = git_config_bool(var, value);\n+\t\tif (value && !strcasecmp(value, \"always\"))\n+\t\t\tlog_all_ref_updates = LOG_REFS_ALWAYS;\n+\t\telse if (git_config_bool(var, value))\n+\t\t\tlog_all_ref_updates = LOG_REFS_NORMAL;\n+\t\telse\n+\t\t\tlog_all_ref_updates = LOG_REFS_NONE;\n \t\treturn 0;\n \t}\n \ndiff --git a/environment.c b/environment.c\nindex 8a83101..c07fb17 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -21,7 +21,6 @@ int ignore_case;\n int assume_unchanged;\n int prefer_symlink_refs;\n int is_bare_repository_cfg = -1; /* unspecified */\n-int log_all_ref_updates = -1; /* unspecified */\n int warn_ambiguous_refs = 1;\n int warn_on_object_refname_ambiguity = 1;\n int ref_paranoia = -1;\n@@ -64,6 +63,7 @@ int merge_log_config = -1;\n int precomposed_unicode = -1; /* see probe_utf8_pathname_composition() */\n unsigned long pack_size_limit_cfg;\n enum hide_dotfiles_type hide_dotfiles = HIDE_DOTFILES_DOTGITONLY;\n+enum log_refs_config log_all_ref_updates = LOG_REFS_UNSET;\n \n #ifndef PROTECT_HFS_DEFAULT\n #define PROTECT_HFS_DEFAULT 0\ndiff --git a/refs.c b/refs.c\nindex 9bd0bc1..cd36b64 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -638,12 +638,17 @@ int copy_reflog_msg(char *buf, const char *msg)\n \n int should_autocreate_reflog(const char *refname)\n {\n-\tif (!log_all_ref_updates)\n+\tswitch (log_all_ref_updates) {\n+\tcase LOG_REFS_ALWAYS:\n+\t\treturn 1;\n+\tcase LOG_REFS_NORMAL:\n+\t\treturn starts_with(refname, \"refs/heads/\") ||\n+\t\t\tstarts_with(refname, \"refs/remotes/\") ||\n+\t\t\tstarts_with(refname, \"refs/notes/\") ||\n+\t\t\t!strcmp(refname, \"HEAD\");\n+\tdefault:\n \t\treturn 0;\n-\treturn starts_with(refname, \"refs/heads/\") ||\n-\t\tstarts_with(refname, \"refs/remotes/\") ||\n-\t\tstarts_with(refname, \"refs/notes/\") ||\n-\t\t!strcmp(refname, \"HEAD\");\n+\t}\n }\n \n int is_branch(const char *refname)\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex f902393..14b17a6 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2682,7 +2682,7 @@ static int files_rename_ref(struct ref_store *ref_store,\n \t}\n \n \tflag = log_all_ref_updates;\n-\tlog_all_ref_updates = 0;\n+\tlog_all_ref_updates = LOG_REFS_NONE;\n \tif (write_ref_to_lockfile(lock, orig_sha1, &err) ||\n \t commit_ref_update(refs, lock, orig_sha1, NULL, &err)) {\n \t\terror(\"unable to write current sha1 into %s: %s\", oldrefname, err.buf);\n@@ -2835,8 +2835,8 @@ static int log_ref_write_1(const char *refname, const unsigned char *old_sha1,\n {\n \tint logfd, result, oflags = O_APPEND | O_WRONLY;\n \n-\tif (log_all_ref_updates < 0)\n-\t\tlog_all_ref_updates = !is_bare_repository();\n+\tif (log_all_ref_updates == LOG_REFS_UNSET)\n+\t\tlog_all_ref_updates = is_bare_repository() ? LOG_REFS_NONE : LOG_REFS_NORMAL;\n \n \tresult = log_ref_setup(refname, logfile, err, flags & REF_FORCE_CREATE_REFLOG);\n \ndiff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh\nindex d4fb977..a920559 100755\n--- a/t/t1400-update-ref.sh\n+++ b/t/t1400-update-ref.sh\n@@ -93,6 +93,36 @@ test_expect_success 'update-ref creates reflogs with --create-reflog' '\n \tgit reflog exists $outside\n '\n \n+test_expect_success 'core.logAllRefUpdates=true does not create reflog by default' '\n+\ttest_config core.logAllRefUpdates true &&\n+\ttest_when_finished \"git update-ref -d $outside\" &&\n+\tgit update-ref $outside $A &&\n+\tgit rev-parse $A >expect &&\n+\tgit rev-parse $outside >actual &&\n+\ttest_cmp expect actual &&\n+\ttest_must_fail git reflog exists $outside\n+'\n+\n+test_expect_success 'core.logAllRefUpdates=always creates reflog by default' '\n+\ttest_config core.logAllRefUpdates always &&\n+\ttest_when_finished \"git update-ref -d $outside\" &&\n+\tgit update-ref $outside $A &&\n+\tgit rev-parse $A >expect &&\n+\tgit rev-parse $outside >actual &&\n+\ttest_cmp expect actual &&\n+\tgit reflog exists $outside\n+'\n+\n+test_expect_success 'update-ref does not create reflog with --no-create-reflog if core.logAllRefUpdates=always' '\n+\ttest_config core.logAllRefUpdates true &&\n+\ttest_when_finished \"git update-ref -d $outside\" &&\n+\tgit update-ref --no-create-reflog $outside $A &&\n+\tgit rev-parse $A >expect &&\n+\tgit rev-parse $outside >actual &&\n+\ttest_cmp expect actual &&\n+\ttest_must_fail git reflog exists $outside\n+'\n+\n test_expect_success \\\n \t\"create $m (by HEAD)\" \\\n \t\"git update-ref HEAD $A &&\n@@ -501,6 +531,7 @@ test_expect_success 'stdin does not create reflogs by default' '\n '\n \n test_expect_success 'stdin creates reflogs with --create-reflog' '\n+\ttest_when_finished \"git update-ref -d $outside\" &&\n \techo \"create $outside $m\" >stdin &&\n \tgit update-ref --create-reflog --stdin expect &&\n@@ -509,6 +540,28 @@ test_expect_success 'stdin creates reflogs with --create-reflog' '\n \tgit reflog exists $outside\n '\n \n+test_expect_success 'stdin does not create reflog when core.logAllRefUpdates=true' '\n+\ttest_config core.logAllRefUpdates true &&\n+\ttest_when_finished \"git update-ref -d $outside\" &&\n+\techo \"create $outside $m\" >stdin &&\n+\tgit update-ref --stdin expect &&\n+\tgit rev-parse $outside >actual &&\n+\ttest_cmp expect actual &&\n+\ttest_must_fail git reflog exists $outside\n+'\n+\n+test_expect_success 'stdin creates reflog when core.logAllRefUpdates=always' '\n+\ttest_config core.logAllRefUpdates always &&\n+\ttest_when_finished \"git update-ref -d $outside\" &&\n+\techo \"create $outside $m\" >stdin &&\n+\tgit update-ref --stdin expect &&\n+\tgit rev-parse $outside >actual &&\n+\ttest_cmp expect actual &&\n+\tgit reflog exists $outside\n+'\n+\n test_expect_success 'stdin succeeds with quoted argument' '\n \tgit update-ref -d $a &&\n \techo \"create $a \\\"$m\\\"\" >stdin &&\n-- \n2.10.2\n\n"},{"id":"310286","messageId":"20170126033547.7bszipvkpi2jb4ad@sigill.intra.peff.net","threadId":"44963","inReplyTo":"20170126011654.21729-2-cornelius.weig@tngtech.com","subject":"Re: [PATCH] refs: add option core.logAllRefUpdates = always","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-01-26T03:35:48Z","receivedAt":"2017-01-26T03:35:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 26, 2017 at 02:16:54AM +0100, cornelius.weig@tngtech.com wrote:\n\n> From: Cornelius Weig \n> \n> When core.logallrefupdates is true, we only create a new reflog for refs\n> that are under certain well-known hierarchies. The reason is that we\n> know that some hierarchies (like refs/tags) do not typically change, and\n> that unknown hierarchies might not want reflogs at all (e.g., a\n> hypothetical refs/foo might be meant to change often and drop old\n> history immediately).\n\nI tried to read this patch with fresh eyes. But given the history, you\nmay take my review with a grain of salt. :)\n\nOverall it looks OK to me. A few comments below.\n\n> This patch introduces a new \"always\" mode for the core.logallrefupdates\n> option which will log updates to everything under refs/, regardless\n> where in the hierarchy it is (we still will not log things like\n> ORIG_HEAD and FETCH_HEAD, which are known to be transient).\n\nI don't think my original had tests for this, but it might be worth\nadding a test for this last bit (i.e., that an update of ORIG_HEAD does\nnot write a reflog when logallrefupdates is set to \"always\").\n\n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> index af2ae4c..2117616 100644\n> --- a/Documentation/config.txt\n> +++ b/Documentation/config.txt\n> @@ -517,10 +517,13 @@ core.logAllRefUpdates::\n> \t\"`$GIT_DIR/logs/`\", by appending the new and old\n> \tSHA-1, the date/time and the reason of the update, but\n> \tonly when the file exists. If this configuration\n> -\tvariable is set to true, missing \"`$GIT_DIR/logs/`\"\n> +\tvariable is set to `true`, missing \"`$GIT_DIR/logs/`\"\n> \tfile is automatically created for branch heads (i.e. under\n> \trefs/heads/), remote refs (i.e. under refs/remotes/),\n> -\tnote refs (i.e. under refs/notes/), and the symbolic ref HEAD.\n> +\t`refs/heads/`), remote refs (i.e. under `refs/remotes/`),\n> +\tnote refs (i.e. under `refs/notes/`), and the symbolic ref `HEAD`.\n> +\tIf it is set to `always`, then a missing reflog is automatically\n> +\tcreated for any ref under `refs/`.\n\nI guess the backtick fixups came from my original. It might be easier to\nsee the change if they were pulled into their own patch, but it's\nprobably not that big a deal.\n\n> --- a/Documentation/git-tag.txt\n> +++ b/Documentation/git-tag.txt\n> @@ -150,7 +150,8 @@ This option is only applicable when listing tags without annotation lines.\n> \t'strip' removes both whitespace and commentary.\n> \n> --create-reflog::\n> -\tCreate a reflog for the tag.\n> +\tCreate a reflog for the tag. To globally enable reflogs for tags, see\n> +\t`core.logAllRefUpdates` in linkgit:git-config[1].\n\nThis documentation tweak makes sense to me.\n\n> diff --git a/builtin/init-db.c b/builtin/init-db.c\n> index 76d68fa..1d4d6a0 100644\n> --- a/builtin/init-db.c\n> +++ b/builtin/init-db.c\n> @@ -262,7 +262,7 @@ static int create_default_files(const char *template_path,\n> \t\tconst char *work_tree = get_git_work_tree();\n> \t\tgit_config_set(\"core.bare\", \"false\");\n> \t\t/* allow template config file to override the default */\n> -\t\tif (log_all_ref_updates == -1)\n> +\t\tif (log_all_ref_updates == LOG_REFS_UNSET)\n> \t\t\tgit_config_set(\"core.logallrefupdates\", \"true\");\n> \t\tif (needs_work_tree_config(original_git_dir, work_tree))\n> \t\t\tgit_config_set(\"core.worktree\", work_tree);\n\nI expected that this hunk would need tweaked due to refactoring around\ninit-db that happened earlier this year. But it seems fine.\n\n> diff --git a/refs.c b/refs.c\n> index 9bd0bc1..cd36b64 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -638,12 +638,17 @@ int copy_reflog_msg(char *buf, const char *msg)\n> \n> int should_autocreate_reflog(const char *refname)\n> {\n> -\tif (!log_all_ref_updates)\n> +\tswitch (log_all_ref_updates) {\n> +\tcase LOG_REFS_ALWAYS:\n> +\t\treturn 1;\n> +\tcase LOG_REFS_NORMAL:\n> +\t\treturn starts_with(refname, \"refs/heads/\") ||\n> +\t\t\tstarts_with(refname, \"refs/remotes/\") ||\n> +\t\t\tstarts_with(refname, \"refs/notes/\") ||\n> +\t\t\t!strcmp(refname, \"HEAD\");\n> +\tdefault:\n> \t\treturn 0;\n> -\treturn starts_with(refname, \"refs/heads/\") ||\n> -\t\tstarts_with(refname, \"refs/remotes/\") ||\n> -\t\tstarts_with(refname, \"refs/notes/\") ||\n> -\t\t!strcmp(refname, \"HEAD\");\n> +\t}\n> }\n\nAnd this function got broken out already by David in an earlier patch.\nLooks good.\n\n> @@ -2835,8 +2835,8 @@ static int log_ref_write_1(const char *refname, const unsigned char *old_sha1,\n> {\n> \tint logfd, result, oflags = O_APPEND | O_WRONLY;\n> \n> -\tif (log_all_ref_updates < 0)\n> -\t\tlog_all_ref_updates = !is_bare_repository();\n> +\tif (log_all_ref_updates == LOG_REFS_UNSET)\n> +\t\tlog_all_ref_updates = is_bare_repository() ? LOG_REFS_NONE : LOG_REFS_NORMAL;\n\nThis hunk is new, I think. The enum values are set in such a way that\nthe original code would have continued to work, but I think using the\nsymbolic names is an improvement.\n\nI assume you grepped for log_all_ref_updates to find this. I see only\none spot that now doesn't use the symbolic names. In builtin/checkout.c,\nupdate_refs_for_switch() checks:\n\n if (opts->new_branch_log && !log_all_ref_updates)\n\nThat looks buggy, as it would treat LOG_REFS_NORMAL and LOG_REFS_UNSET\nthe same, and I do not see us resolving the UNSET case to a true/false\nvalue. But I don't think the bug is new in your patch; the default value\nwas \"-1\" already.\n\nI doubt it can be triggered in practice, because either:\n\n - the config value is set in the config file, and we pick up that\n value, whether it's \"true\" or \"false\"\n\n - it's unset, in which case our default would be to enable reflogs in\n a non-bare repo. And since git-checkout would refuse to run in a\n bare repo, we must be non-bare, and thus enabling reflogs does the\n right thing.\n\nBut it works quite by accident. I wonder if we should this\n\"is_bare_repository\" check into a function that can be called instead of\naccessing log_all_ref_updates() directly.\n\n> --- a/t/t1400-update-ref.sh\n> +++ b/t/t1400-update-ref.sh\n> @@ -93,6 +93,36 @@ test_expect_success 'update-ref creates reflogs with --create-reflog' '\n> \tgit reflog exists $outside\n> '\n> \n> +test_expect_success 'core.logAllRefUpdates=true does not create reflog by default' '\n> +\ttest_config core.logAllRefUpdates true &&\n> +\ttest_when_finished \"git update-ref -d $outside\" &&\n> +\tgit update-ref $outside $A &&\n> +\tgit rev-parse $A >expect &&\n> +\tgit rev-parse $outside >actual &&\n> +\ttest_cmp expect actual &&\n> +\ttest_must_fail git reflog exists $outside\n> +'\n> +\n> +test_expect_success 'core.logAllRefUpdates=always creates reflog by default' '\n> +\ttest_config core.logAllRefUpdates always &&\n> +\ttest_when_finished \"git update-ref -d $outside\" &&\n> +\tgit update-ref $outside $A &&\n> +\tgit rev-parse $A >expect &&\n> +\tgit rev-parse $outside >actual &&\n> +\ttest_cmp expect actual &&\n> +\tgit reflog exists $outside\n> +'\n\nAdding the tests to the existing --create-reflog tests is a good choice.\n\n> +test_expect_success 'update-ref does not create reflog with --no-create-reflog if core.logAllRefUpdates=always' '\n\nThis test title is _really_ long, and will wrap in the output on\nreasonable-sized terminals. Maybe '--no-create-reflog overrides\ncore.logAllRefUpdates=always' would be shorter?\n\n> test_expect_success 'stdin creates reflogs with --create-reflog' '\n> +\ttest_when_finished \"git update-ref -d $outside\" &&\n> \techo \"create $outside $m\" >stdin &&\n> \tgit update-ref --create-reflog --stdin \tgit rev-parse $m >expect &&\n\nAdding missing cleanup. Good.\n\n> +test_expect_success 'stdin does not create reflog when core.logAllRefUpdates=true' '\n\nI don't mind these extra stdin tests, but IMHO they are just redundant.\nThe \"--stdin --create-reflog\" one makes sure the option is propagated\ndown via the --stdin machinery. But we know the config option is handled\nat a low level anyway.\n\nI guess it depends on how black-box we want the testing to be. It just\nseems unlikely for a regression to be found here and not in the tests\nabove.\n\n-Peff\n"},{"id":"310315","messageId":"4faf836a-40b6-da9a-877a-3b2ce7c863df@tngtech.com","threadId":"44963","inReplyTo":"20170126033547.7bszipvkpi2jb4ad@sigill.intra.peff.net","subject":"Re: [PATCH] refs: add option core.logAllRefUpdates = always","fromName":"Cornelius Weig","fromEmail":"cornelius.weig@tngtech.com","sentAt":"2017-01-26T14:06:40Z","receivedAt":"2017-01-26T14:06:52Z","isPatch":true,"sender":{"key":"cornelius.weig@tngtech.com","avatar":null},"body":"Hi Peff,\n\n thanks for your thoughts.\n\n> I tried to read this patch with fresh eyes. But given the history, you\n> may take my review with a grain of salt. :)\n\nDoes it mean another reviewer is needed?\n\n> I don't think my original had tests for this, but it might be worth\n> adding a test for this last bit (i.e., that an update of ORIG_HEAD does\n> not write a reflog when logallrefupdates is set to \"always\").\n\nGood point. I blindly copied your commit message without thinking too\nmuch about it.\n\n> I guess the backtick fixups came from my original. It might be easier to\n> see the change if they were pulled into their own patch, but it's\n> probably not that big a deal.\n\nIf it's best practice to break out such changes, I'll revise it.\n\n>> @@ -2835,8 +2835,8 @@ static int log_ref_write_1(const char *refname, const unsigned char *old_sha1,\n>> {\n>> \tint logfd, result, oflags = O_APPEND | O_WRONLY;\n>> \n>> -\tif (log_all_ref_updates < 0)\n>> -\t\tlog_all_ref_updates = !is_bare_repository();\n>> +\tif (log_all_ref_updates == LOG_REFS_UNSET)\n>> +\t\tlog_all_ref_updates = is_bare_repository() ? LOG_REFS_NONE : LOG_REFS_NORMAL;\n> \n> This hunk is new, I think. The enum values are set in such a way that\n> the original code would have continued to work, but I think using the\n> symbolic names is an improvement.\n\nYes it's new.\n\n> I assume you grepped for log_all_ref_updates to find this. I see only\n> one spot that now doesn't use the symbolic names. In builtin/checkout.c,\n> update_refs_for_switch() checks:\n> \n> if (opts->new_branch_log && !log_all_ref_updates)\n> \n> That looks buggy, as it would treat LOG_REFS_NORMAL and LOG_REFS_UNSET\n> the same, and I do not see us resolving the UNSET case to a true/false\n> value. But I don't think the bug is new in your patch; the default value\n> was \"-1\" already.\n>\n> I doubt it can be triggered in practice, because either:\n> \n> - the config value is set in the config file, and we pick up that\n> value, whether it's \"true\" or \"false\"\n> \n> - it's unset, in which case our default would be to enable reflogs in\n> a non-bare repo. And since git-checkout would refuse to run in a\n> bare repo, we must be non-bare, and thus enabling reflogs does the\n> right thing.\n\nThat far I can follow.\n\n> But it works quite by accident. I wonder if we should this\n> \"is_bare_repository\" check into a function that can be called instead of\n> accessing log_all_ref_updates() directly.\n\nAre you saying that we should move the `!log_all_ref_updates` check into\nits own function where we should also check `is_bare_repository`? I\ndon't see that this would win much, because as you said: checkouts in a\nbare repo are forbidden anyway.\n\nOther than that, I guess it should better read `log_all_ref_update !=\nLOG_REFS_NONE` instead of `!log_all_ref_updates`.\n\n\n>> +test_expect_success 'update-ref does not create reflog with --no-create-reflog if core.logAllRefUpdates=always' '\n> \n> This test title is _really_ long, and will wrap in the output on\n> reasonable-sized terminals. Maybe '--no-create-reflog overrides\n> core.logAllRefUpdates=always' would be shorter?\n\nYes, I agree.\n\n>> +test_expect_success 'stdin does not create reflog when core.logAllRefUpdates=true' '\n> \n> I don't mind these extra stdin tests, but IMHO they are just redundant.\n> The \"--stdin --create-reflog\" one makes sure the option is propagated\n> down via the --stdin machinery. But we know the config option is handled\n> at a low level anyway.\n> \n> I guess it depends on how black-box we want the testing to be. It just\n> seems unlikely for a regression to be found here and not in the tests\n> above.\n\nSince these other stdin tests were around, I added this variant. But\nyou're right: this test breaks along with the other and doesn't add add\nmore safety. I'll remove it.\n\nHowever, I realized that I have not written tests about ref updates in a\nbare repository. Do you think it's worthwile?\n\nCheers,\n Cornelius\n\n"},{"id":"310323","messageId":"20170126144610.7tosfix4v3tah7p2@sigill.intra.peff.net","threadId":"44963","inReplyTo":"4faf836a-40b6-da9a-877a-3b2ce7c863df@tngtech.com","subject":"Re: [PATCH] refs: add option core.logAllRefUpdates = always","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-01-26T14:46:11Z","receivedAt":"2017-01-26T14:46:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 26, 2017 at 03:06:40PM +0100, Cornelius Weig wrote:\n\n> > But it works quite by accident. I wonder if we should this\n> > \"is_bare_repository\" check into a function that can be called instead of\n> > accessing log_all_ref_updates() directly.\n> \n> Are you saying that we should move the `!log_all_ref_updates` check into\n> its own function where we should also check `is_bare_repository`? I\n> don't see that this would win much, because as you said: checkouts in a\n> bare repo are forbidden anyway.\n\nYes, I'm suggesting making something like the should_autocreate_reflog()\nfunction public.\n\nI agree it is working correctly now. It's just that it's rather subtle\nthat it treats LOG_REFS_UNSET implicitly as LOG_REFS_NONE.\n\nIt would also possibly break if more values are added to the enum\n(depending on what those values are).\n\n> However, I realized that I have not written tests about ref updates in a\n> bare repository. Do you think it's worthwile?\n\nThere should already be a test for logAllRefUpdates=true in a bare\nrepository (if there isn't, we should probably add one). Testing the\n\"always\" case individually does not add much over testing it in a\nnon-bare repository. IMHO.\n\n-Peff\n"},{"id":"310367","messageId":"20170126223159.16439-2-cornelius.weig@tngtech.com","threadId":"44963","inReplyTo":"20170126223159.16439-1-cornelius.weig@tngtech.com","subject":"[PATCH v2 2/3] refs: add option core.logAllRefUpdates = always","fromName":"","fromEmail":"cornelius.weig@tngtech.com","sentAt":"2017-01-26T22:31:58Z","receivedAt":"2017-01-26T22:41:17Z","isPatch":true,"sender":{"key":"cornelius.weig@tngtech.com","avatar":null},"body":"From: Cornelius Weig \n\nWhen core.logallrefupdates is true, we only create a new reflog for refs\nthat are under certain well-known hierarchies. The reason is that we\nknow that some hierarchies (like refs/tags) do not typically change, and\nthat unknown hierarchies might not want reflogs at all (e.g., a\nhypothetical refs/foo might be meant to change often and drop old\nhistory immediately).\n\nHowever, sometimes it is useful to override this decision and simply log\nfor all refs, because the safety and audit trail is more important than\nthe performance implications of keeping the log around.\n\nThis patch introduces a new \"always\" mode for the core.logallrefupdates\noption which will log updates to everything under refs/, regardless\nwhere in the hierarchy it is (we still will not log things like\nORIG_HEAD and FETCH_HEAD, which are known to be transient).\n\nBased-on-patch-by: Jeff King \nSigned-off-by: Cornelius Weig \nReviewed-by: Jeff King \n---\n\nNotes:\n Changes with respect to the previous version:\n \n - add test that checks that no reflog is created for ORIG_HEAD if\n core.logAllRefUpdates=always\n - remove redundant tests that check reflog creation when update-ref is called\n with --stdin\n - make test description shorter\n - make the function should_autocreate_reflog() public and use it in\n update_refs_for_switch().\n \n The last item addresses Peff's concern that the previous version only works by\n accident and may break in the future (see\n 20170126033547.7bszipvkpi2jb4ad@sigill.intra.peff.net). In particular, this\n concerns the following change:\n \n - if (opts->new_branch_log && !log_all_ref_updates) {\n + if (opts->new_branch_log && should_autocreate_reflog(\"refs/heads/\")) {\n \n The function call to `should_autocreate_reflog()` answers exactly the question\n that the original test `!log_all_ref_updates` tried to resolve in the original\n version.\n\n Documentation/config.txt | 2 ++\n Documentation/git-tag.txt | 3 ++-\n branch.c | 2 +-\n builtin/checkout.c | 2 +-\n builtin/init-db.c | 2 +-\n cache.h | 9 ++++++++-\n config.c | 7 ++++++-\n environment.c | 2 +-\n refs.c | 15 ++++++++++-----\n refs.h | 2 ++\n refs/files-backend.c | 6 +++---\n refs/refs-internal.h | 2 --\n t/t1400-update-ref.sh | 37 +++++++++++++++++++++++++++++++++++++\n 13 files changed, 74 insertions(+), 17 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 3cd8030..2117616 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -522,6 +522,8 @@ core.logAllRefUpdates::\n \trefs/heads/), remote refs (i.e. under refs/remotes/),\n \t`refs/heads/`), remote refs (i.e. under `refs/remotes/`),\n \tnote refs (i.e. under `refs/notes/`), and the symbolic ref `HEAD`.\n+\tIf it is set to `always`, then a missing reflog is automatically\n+\tcreated for any ref under `refs/`.\n +\n This information can be used to determine what commit\n was the tip of a branch \"2 days ago\".\ndiff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt\nindex 5055a96..2ac25a9 100644\n--- a/Documentation/git-tag.txt\n+++ b/Documentation/git-tag.txt\n@@ -150,7 +150,8 @@ This option is only applicable when listing tags without annotation lines.\n \t'strip' removes both whitespace and commentary.\n \n --create-reflog::\n-\tCreate a reflog for the tag.\n+\tCreate a reflog for the tag. To globally enable reflogs for tags, see\n+\t`core.logAllRefUpdates` in linkgit:git-config[1].\n \n ::\n \tThe name of the tag to create, delete, or describe.\ndiff --git a/branch.c b/branch.c\nindex c431cbf..b955d4f 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -298,7 +298,7 @@ void create_branch(const char *name, const char *start_name,\n \t\t\t start_name);\n \n \tif (reflog)\n-\t\tlog_all_ref_updates = 1;\n+\t\tlog_all_ref_updates = LOG_REFS_NORMAL;\n \n \tif (!dont_change_ref) {\n \t\tstruct ref_transaction *transaction;\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex bfe685c..1db0b44 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -612,7 +612,7 @@ static void update_refs_for_switch(const struct checkout_opts *opts,\n \tconst char *old_desc, *reflog_msg;\n \tif (opts->new_branch) {\n \t\tif (opts->new_orphan_branch) {\n-\t\t\tif (opts->new_branch_log && !log_all_ref_updates) {\n+\t\t\tif (opts->new_branch_log && should_autocreate_reflog(\"refs/heads/\")) {\n \t\t\t\tint ret;\n \t\t\t\tchar *refname;\n \t\t\t\tstruct strbuf err = STRBUF_INIT;\ndiff --git a/builtin/init-db.c b/builtin/init-db.c\nindex 76d68fa..1d4d6a0 100644\n--- a/builtin/init-db.c\n+++ b/builtin/init-db.c\n@@ -262,7 +262,7 @@ static int create_default_files(const char *template_path,\n \t\tconst char *work_tree = get_git_work_tree();\n \t\tgit_config_set(\"core.bare\", \"false\");\n \t\t/* allow template config file to override the default */\n-\t\tif (log_all_ref_updates == -1)\n+\t\tif (log_all_ref_updates == LOG_REFS_UNSET)\n \t\t\tgit_config_set(\"core.logallrefupdates\", \"true\");\n \t\tif (needs_work_tree_config(original_git_dir, work_tree))\n \t\t\tgit_config_set(\"core.worktree\", work_tree);\ndiff --git a/cache.h b/cache.h\nindex 00a029a..96eeaaf 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -660,7 +660,6 @@ extern int minimum_abbrev, default_abbrev;\n extern int ignore_case;\n extern int assume_unchanged;\n extern int prefer_symlink_refs;\n-extern int log_all_ref_updates;\n extern int warn_ambiguous_refs;\n extern int warn_on_object_refname_ambiguity;\n extern const char *apply_default_whitespace;\n@@ -728,6 +727,14 @@ enum hide_dotfiles_type {\n };\n extern enum hide_dotfiles_type hide_dotfiles;\n \n+enum log_refs_config {\n+\tLOG_REFS_UNSET = -1,\n+\tLOG_REFS_NONE = 0,\n+\tLOG_REFS_NORMAL,\n+\tLOG_REFS_ALWAYS\n+};\n+extern enum log_refs_config log_all_ref_updates;\n+\n enum branch_track {\n \tBRANCH_TRACK_UNSPECIFIED = -1,\n \tBRANCH_TRACK_NEVER = 0,\ndiff --git a/config.c b/config.c\nindex b680f79..c6b874a 100644\n--- a/config.c\n+++ b/config.c\n@@ -826,7 +826,12 @@ static int git_default_core_config(const char *var, const char *value)\n \t}\n \n \tif (!strcmp(var, \"core.logallrefupdates\")) {\n-\t\tlog_all_ref_updates = git_config_bool(var, value);\n+\t\tif (value && !strcasecmp(value, \"always\"))\n+\t\t\tlog_all_ref_updates = LOG_REFS_ALWAYS;\n+\t\telse if (git_config_bool(var, value))\n+\t\t\tlog_all_ref_updates = LOG_REFS_NORMAL;\n+\t\telse\n+\t\t\tlog_all_ref_updates = LOG_REFS_NONE;\n \t\treturn 0;\n \t}\n \ndiff --git a/environment.c b/environment.c\nindex 8a83101..c07fb17 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -21,7 +21,6 @@ int ignore_case;\n int assume_unchanged;\n int prefer_symlink_refs;\n int is_bare_repository_cfg = -1; /* unspecified */\n-int log_all_ref_updates = -1; /* unspecified */\n int warn_ambiguous_refs = 1;\n int warn_on_object_refname_ambiguity = 1;\n int ref_paranoia = -1;\n@@ -64,6 +63,7 @@ int merge_log_config = -1;\n int precomposed_unicode = -1; /* see probe_utf8_pathname_composition() */\n unsigned long pack_size_limit_cfg;\n enum hide_dotfiles_type hide_dotfiles = HIDE_DOTFILES_DOTGITONLY;\n+enum log_refs_config log_all_ref_updates = LOG_REFS_UNSET;\n \n #ifndef PROTECT_HFS_DEFAULT\n #define PROTECT_HFS_DEFAULT 0\ndiff --git a/refs.c b/refs.c\nindex 9bd0bc1..cd36b64 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -638,12 +638,17 @@ int copy_reflog_msg(char *buf, const char *msg)\n \n int should_autocreate_reflog(const char *refname)\n {\n-\tif (!log_all_ref_updates)\n+\tswitch (log_all_ref_updates) {\n+\tcase LOG_REFS_ALWAYS:\n+\t\treturn 1;\n+\tcase LOG_REFS_NORMAL:\n+\t\treturn starts_with(refname, \"refs/heads/\") ||\n+\t\t\tstarts_with(refname, \"refs/remotes/\") ||\n+\t\t\tstarts_with(refname, \"refs/notes/\") ||\n+\t\t\t!strcmp(refname, \"HEAD\");\n+\tdefault:\n \t\treturn 0;\n-\treturn starts_with(refname, \"refs/heads/\") ||\n-\t\tstarts_with(refname, \"refs/remotes/\") ||\n-\t\tstarts_with(refname, \"refs/notes/\") ||\n-\t\t!strcmp(refname, \"HEAD\");\n+\t}\n }\n \n int is_branch(const char *refname)\ndiff --git a/refs.h b/refs.h\nindex 6947843..9fbff90 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -64,6 +64,8 @@ int read_ref(const char *refname, unsigned char *sha1);\n \n int ref_exists(const char *refname);\n \n+int should_autocreate_reflog(const char *refname);\n+\n int is_branch(const char *refname);\n \n extern int refs_init_db(struct strbuf *err);\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex f902393..14b17a6 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2682,7 +2682,7 @@ static int files_rename_ref(struct ref_store *ref_store,\n \t}\n \n \tflag = log_all_ref_updates;\n-\tlog_all_ref_updates = 0;\n+\tlog_all_ref_updates = LOG_REFS_NONE;\n \tif (write_ref_to_lockfile(lock, orig_sha1, &err) ||\n \t commit_ref_update(refs, lock, orig_sha1, NULL, &err)) {\n \t\terror(\"unable to write current sha1 into %s: %s\", oldrefname, err.buf);\n@@ -2835,8 +2835,8 @@ static int log_ref_write_1(const char *refname, const unsigned char *old_sha1,\n {\n \tint logfd, result, oflags = O_APPEND | O_WRONLY;\n \n-\tif (log_all_ref_updates < 0)\n-\t\tlog_all_ref_updates = !is_bare_repository();\n+\tif (log_all_ref_updates == LOG_REFS_UNSET)\n+\t\tlog_all_ref_updates = is_bare_repository() ? LOG_REFS_NONE : LOG_REFS_NORMAL;\n \n \tresult = log_ref_setup(refname, logfile, err, flags & REF_FORCE_CREATE_REFLOG);\n \ndiff --git a/refs/refs-internal.h b/refs/refs-internal.h\nindex 708b260..25444cf 100644\n--- a/refs/refs-internal.h\n+++ b/refs/refs-internal.h\n@@ -133,8 +133,6 @@ int verify_refname_available(const char *newname,\n */\n int copy_reflog_msg(char *buf, const char *msg);\n \n-int should_autocreate_reflog(const char *refname);\n-\n /**\n * Information needed for a single ref update. Set new_sha1 to the new\n * value or to null_sha1 to delete the ref. To check the old value\ndiff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh\nindex d4fb977..b9084ca 100755\n--- a/t/t1400-update-ref.sh\n+++ b/t/t1400-update-ref.sh\n@@ -93,6 +93,42 @@ test_expect_success 'update-ref creates reflogs with --create-reflog' '\n \tgit reflog exists $outside\n '\n \n+test_expect_success 'core.logAllRefUpdates=true does not create reflog by default' '\n+\ttest_config core.logAllRefUpdates true &&\n+\ttest_when_finished \"git update-ref -d $outside\" &&\n+\tgit update-ref $outside $A &&\n+\tgit rev-parse $A >expect &&\n+\tgit rev-parse $outside >actual &&\n+\ttest_cmp expect actual &&\n+\ttest_must_fail git reflog exists $outside\n+'\n+\n+test_expect_success 'core.logAllRefUpdates=always creates reflog by default' '\n+\ttest_config core.logAllRefUpdates always &&\n+\ttest_when_finished \"git update-ref -d $outside\" &&\n+\tgit update-ref $outside $A &&\n+\tgit rev-parse $A >expect &&\n+\tgit rev-parse $outside >actual &&\n+\ttest_cmp expect actual &&\n+\tgit reflog exists $outside\n+'\n+\n+test_expect_success 'core.logAllRefUpdates=always creates no reflog for ORIG_HEAD' '\n+\ttest_config core.logAllRefUpdates always &&\n+\tgit update-ref ORIG_HEAD $A &&\n+\ttest_must_fail git reflog exists ORIG_HEAD\n+'\n+\n+test_expect_success '--no-create-reflog overrides core.logAllRefUpdates=always' '\n+\ttest_config core.logAllRefUpdates true &&\n+\ttest_when_finished \"git update-ref -d $outside\" &&\n+\tgit update-ref --no-create-reflog $outside $A &&\n+\tgit rev-parse $A >expect &&\n+\tgit rev-parse $outside >actual &&\n+\ttest_cmp expect actual &&\n+\ttest_must_fail git reflog exists $outside\n+'\n+\n test_expect_success \\\n \t\"create $m (by HEAD)\" \\\n \t\"git update-ref HEAD $A &&\n@@ -501,6 +537,7 @@ test_expect_success 'stdin does not create reflogs by default' '\n '\n \n test_expect_success 'stdin creates reflogs with --create-reflog' '\n+\ttest_when_finished \"git update-ref -d $outside\" &&\n \techo \"create $outside $m\" >stdin &&\n \tgit update-ref --create-reflog --stdin expect &&\n-- \n2.10.2\n\n"},{"id":"310368","messageId":"20170126223159.16439-3-cornelius.weig@tngtech.com","threadId":"44963","inReplyTo":"20170126223159.16439-1-cornelius.weig@tngtech.com","subject":"[PATCH v2 3/3] update-ref: add test cases for bare repository","fromName":"","fromEmail":"cornelius.weig@tngtech.com","sentAt":"2017-01-26T22:31:59Z","receivedAt":"2017-01-26T22:41:20Z","isPatch":true,"sender":{"key":"cornelius.weig@tngtech.com","avatar":null},"body":"From: Cornelius Weig \n\nThe default behavior of update-ref to create reflogs differs in\nrepositories with worktree and bare ones. The existing tests cover only\nthe behavior of repositories with worktree.\n\nThis commit adds tests that assert the correct behavior in bare\nrepositories for update-ref. Two cases are covered:\n\n - If core.logAllRefUpdates is not set, no reflogs should be created\n - If core.logAllRefUpdates is true, reflogs should be created\n\nSigned-off-by: Cornelius Weig \n---\n t/t1400-update-ref.sh | 43 ++++++++++++++++++++++++++++++++++++-------\n 1 file changed, 36 insertions(+), 7 deletions(-)\n\ndiff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh\nindex b9084ca..bad88c8 100755\n--- a/t/t1400-update-ref.sh\n+++ b/t/t1400-update-ref.sh\n@@ -8,23 +8,33 @@ test_description='Test git update-ref and basic ref logging'\n \n Z=$_z40\n \n-test_expect_success setup '\n+m=refs/heads/master\n+n_dir=refs/heads/gu\n+n=$n_dir/fixes\n+outside=refs/foo\n+bare=bare-repo\n \n+create_test_objects ()\n+{\n+\tlocal T, sha1, prfx=\"$1\"\n \tfor name in A B C D E F\n \tdo\n \t\ttest_tick &&\n \t\tT=$(git write-tree) &&\n \t\tsha1=$(echo $name | git commit-tree $T) &&\n-\t\teval $name=$sha1\n+\t\teval $prfx$name=$sha1\n \tdone\n+}\n \n+test_expect_success setup '\n+\tcreate_test_objects \"\" &&\n+\tmkdir $bare &&\n+\tcd $bare &&\n+\tgit init --bare &&\n+\tcreate_test_objects \"bare\" &&\n+\tcd -\n '\n \n-m=refs/heads/master\n-n_dir=refs/heads/gu\n-n=$n_dir/fixes\n-outside=refs/foo\n-\n test_expect_success \\\n \t\"create $m\" \\\n \t\"git update-ref $m $A &&\n@@ -93,6 +103,25 @@ test_expect_success 'update-ref creates reflogs with --create-reflog' '\n \tgit reflog exists $outside\n '\n \n+test_expect_success 'creates no reflog in bare repository' '\n+\tgit -C $bare update-ref $m $bareA &&\n+\tgit -C $bare rev-parse $bareA >expect &&\n+\tgit -C $bare rev-parse $m >actual &&\n+\ttest_cmp expect actual &&\n+\ttest_must_fail git -C $bare reflog exists $m\n+'\n+\n+test_expect_success 'core.logAllRefUpdates=true creates reflog in bare repository' '\n+\ttest_when_finished \"git -C $bare config --unset core.logAllRefUpdates && \\\n+\t\trm $bare/logs/$m\" &&\n+\tgit -C $bare config core.logAllRefUpdates true &&\n+\tgit -C $bare update-ref $m $bareB &&\n+\tgit -C $bare rev-parse $bareB >expect &&\n+\tgit -C $bare rev-parse $m >actual &&\n+\ttest_cmp expect actual &&\n+\tgit -C $bare reflog exists $m\n+'\n+\n test_expect_success 'core.logAllRefUpdates=true does not create reflog by default' '\n \ttest_config core.logAllRefUpdates true &&\n \ttest_when_finished \"git update-ref -d $outside\" &&\n-- \n2.10.2\n\n"},{"id":"310369","messageId":"20170126223159.16439-1-cornelius.weig@tngtech.com","threadId":"44963","inReplyTo":"20170126033547.7bszipvkpi2jb4ad@sigill.intra.peff.net","subject":"[PATCH v2 1/3] config: add markup to core.logAllRefUpdates doc","fromName":"","fromEmail":"cornelius.weig@tngtech.com","sentAt":"2017-01-26T22:31:57Z","receivedAt":"2017-01-26T22:41:21Z","isPatch":true,"sender":{"key":"cornelius.weig@tngtech.com","avatar":null},"body":"From: Cornelius Weig \n\nSigned-off-by: Cornelius Weig \n---\n\nNotes:\n As suggested, I moved the modification of the markup to its own commit.\n\n Documentation/config.txt | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex af2ae4c..3cd8030 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -517,10 +517,11 @@ core.logAllRefUpdates::\n \t\"`$GIT_DIR/logs/`\", by appending the new and old\n \tSHA-1, the date/time and the reason of the update, but\n \tonly when the file exists. If this configuration\n-\tvariable is set to true, missing \"`$GIT_DIR/logs/`\"\n+\tvariable is set to `true`, missing \"`$GIT_DIR/logs/`\"\n \tfile is automatically created for branch heads (i.e. under\n \trefs/heads/), remote refs (i.e. under refs/remotes/),\n-\tnote refs (i.e. under refs/notes/), and the symbolic ref HEAD.\n+\t`refs/heads/`), remote refs (i.e. under `refs/remotes/`),\n+\tnote refs (i.e. under `refs/notes/`), and the symbolic ref `HEAD`.\n +\n This information can be used to determine what commit\n was the tip of a branch \"2 days ago\".\n-- \n2.10.2\n\n"},{"id":"310372","messageId":"xmqqvat11k1i.fsf@gitster.mtv.corp.google.com","threadId":"44963","inReplyTo":"20170126223159.16439-1-cornelius.weig@tngtech.com","subject":"Re: [PATCH v2 1/3] config: add markup to core.logAllRefUpdates doc","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-01-26T23:24:57Z","receivedAt":"2017-01-26T23:25:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"cornelius.weig@tngtech.com writes:\n\n> From: Cornelius Weig \n>\n> Signed-off-by: Cornelius Weig \n> ---\n>\n> Notes:\n> As suggested, I moved the modification of the markup to its own commit.\n>\n> Documentation/config.txt | 5 +++--\n> 1 file changed, 3 insertions(+), 2 deletions(-)\n>\n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> index af2ae4c..3cd8030 100644\n> --- a/Documentation/config.txt\n> +++ b/Documentation/config.txt\n> @@ -517,10 +517,11 @@ core.logAllRefUpdates::\n> \t\"`$GIT_DIR/logs/`\", by appending the new and old\n> \tSHA-1, the date/time and the reason of the update, but\n> \tonly when the file exists. If this configuration\n> -\tvariable is set to true, missing \"`$GIT_DIR/logs/`\"\n> +\tvariable is set to `true`, missing \"`$GIT_DIR/logs/`\"\n> \tfile is automatically created for branch heads (i.e. under\n> \trefs/heads/), remote refs (i.e. under refs/remotes/),\n> -\tnote refs (i.e. under refs/notes/), and the symbolic ref HEAD.\n> +\t`refs/heads/`), remote refs (i.e. under `refs/remotes/`),\n> +\tnote refs (i.e. under `refs/notes/`), and the symbolic ref `HEAD`.\n\nThis is a peculiar patch. \n\nDid you hand edit and lose a leading '-' from one of the lines by\naccident, or something?\n"},{"id":"310375","messageId":"xmqqr33p1jd2.fsf@gitster.mtv.corp.google.com","threadId":"44963","inReplyTo":"20170126223159.16439-2-cornelius.weig@tngtech.com","subject":"Re: [PATCH v2 2/3] refs: add option core.logAllRefUpdates = always","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-01-26T23:39:37Z","receivedAt":"2017-01-26T23:41:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"cornelius.weig@tngtech.com writes:\n\n> From: Cornelius Weig \n>\n> When core.logallrefupdates is true, we only create a new reflog for refs\n> that are under certain well-known hierarchies. The reason is that we\n> know that some hierarchies (like refs/tags) do not typically change, and\n\ns/do not typically/are not meant to/;\n\n> that unknown hierarchies might not want reflogs at all (e.g., a\n> hypothetical refs/foo might be meant to change often and drop old\n> history immediately).\n>\n> However, sometimes it is useful to override this decision and simply log\n> for all refs, because the safety and audit trail is more important than\n> the performance implications of keeping the log around.\n>\n> This patch introduces a new \"always\" mode for the core.logallrefupdates\n> option which will log updates to everything under refs/, regardless\n> where in the hierarchy it is (we still will not log things like\n> ORIG_HEAD and FETCH_HEAD, which are known to be transient).\n\nOK.\n\n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> index 3cd8030..2117616 100644\n> --- a/Documentation/config.txt\n> +++ b/Documentation/config.txt\n> @@ -522,6 +522,8 @@ core.logAllRefUpdates::\n> \trefs/heads/), remote refs (i.e. under refs/remotes/),\n> \t`refs/heads/`), remote refs (i.e. under `refs/remotes/`),\n\nAhh, the answer to my question on 1/3 is \"no, the commit that the\npatch was taken out of was already wrong, still having the old line\nin front of its rewrite\".\n\n> \tnote refs (i.e. under `refs/notes/`), and the symbolic ref `HEAD`.\n> +\tIf it is set to `always`, then a missing reflog is automatically\n> +\tcreated for any ref under `refs/`.\n> +\n\nOK.\n\n> diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt\n> index 5055a96..2ac25a9 100644\n> --- a/Documentation/git-tag.txt\n> +++ b/Documentation/git-tag.txt\n> @@ -150,7 +150,8 @@ This option is only applicable when listing tags without annotation lines.\n> \t'strip' removes both whitespace and commentary.\n> \n> --create-reflog::\n> -\tCreate a reflog for the tag.\n> +\tCreate a reflog for the tag. To globally enable reflogs for tags, see\n> +\t`core.logAllRefUpdates` in linkgit:git-config[1].\n\nOK.\n\n> diff --git a/builtin/checkout.c b/builtin/checkout.c\n> index bfe685c..1db0b44 100644\n> --- a/builtin/checkout.c\n> +++ b/builtin/checkout.c\n> @@ -612,7 +612,7 @@ static void update_refs_for_switch(const struct checkout_opts *opts,\n> \tconst char *old_desc, *reflog_msg;\n> \tif (opts->new_branch) {\n> \t\tif (opts->new_orphan_branch) {\n> -\t\t\tif (opts->new_branch_log && !log_all_ref_updates) {\n> +\t\t\tif (opts->new_branch_log && should_autocreate_reflog(\"refs/heads/\")) {\n\nThis is inviting a maintenance nightmare. The helper function is\ndefined to take the final refname, not a leading directory name.\nThat is why you named the parameter \"refname\" in your patch like\nthis:\n\n --- a/refs.h\n +++ b/refs.h\n @@ -64,6 +64,8 @@ int read_ref(const char *refname, unsigned char *sha1);\n\n int ref_exists(const char *refname);\n\n +int should_autocreate_reflog(const char *refname);\n +\n int is_branch(const char *refname);\n\nThe callers are not supposed to know that its current implementation\nhappens to only use the leading prefix. When the definition of this\nhelper function is changed (e.g. imagine a future where this\n\"log.allrefupdate\" is further enhanced to take glob patterns to\nmatch the refname against), this may break and nobody would notice\nfor a few weeks, and we will get a regression report after a release\nis made.\n\nDon't we have the refname for the branch already in this codepath?\n\n> diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh\n> index d4fb977..b9084ca 100755\n> --- a/t/t1400-update-ref.sh\n> +++ b/t/t1400-update-ref.sh\n> @@ -93,6 +93,42 @@ test_expect_success 'update-ref creates reflogs with --create-reflog' '\n> \tgit reflog exists $outside\n> '\n> \n> +test_expect_success 'core.logAllRefUpdates=true does not create reflog by default' '\n> +\ttest_config core.logAllRefUpdates true &&\n> +\ttest_when_finished \"git update-ref -d $outside\" &&\n> +\tgit update-ref $outside $A &&\n> +\tgit rev-parse $A >expect &&\n> +\tgit rev-parse $outside >actual &&\n> +\ttest_cmp expect actual &&\n> +\ttest_must_fail git reflog exists $outside\n> +'\n> +\n> +test_expect_success 'core.logAllRefUpdates=always creates reflog by default' '\n> +\ttest_config core.logAllRefUpdates always &&\n> +\ttest_when_finished \"git update-ref -d $outside\" &&\n> +\tgit update-ref $outside $A &&\n> +\tgit rev-parse $A >expect &&\n> +\tgit rev-parse $outside >actual &&\n> +\ttest_cmp expect actual &&\n> +\tgit reflog exists $outside\n> +'\n\nYou might want to add two tests for your original motivation, i.e.\n\n\ttest_config core.logAllRefUpdates always &&\n\tgit tag a-tag &&\n\tgit reflog exists refs/tags/a-tag\n\nand the other one that does not give reflog for a tag.\n\nOther than that, looks good to me.\n"},{"id":"310376","messageId":"xmqqmved1j98.fsf@gitster.mtv.corp.google.com","threadId":"44963","inReplyTo":"20170126223159.16439-3-cornelius.weig@tngtech.com","subject":"Re: [PATCH v2 3/3] update-ref: add test cases for bare repository","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-01-26T23:41:55Z","receivedAt":"2017-01-26T23:58:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"cornelius.weig@tngtech.com writes:\n\n> From: Cornelius Weig \n>\n> The default behavior of update-ref to create reflogs differs in\n> repositories with worktree and bare ones. The existing tests cover only\n> the behavior of repositories with worktree.\n>\n> This commit adds tests that assert the correct behavior in bare\n> repositories for update-ref. Two cases are covered:\n>\n> - If core.logAllRefUpdates is not set, no reflogs should be created\n> - If core.logAllRefUpdates is true, reflogs should be created\n>\n> Signed-off-by: Cornelius Weig \n> ---\n> t/t1400-update-ref.sh | 43 ++++++++++++++++++++++++++++++++++++-------\n> 1 file changed, 36 insertions(+), 7 deletions(-)\n>\n> diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh\n> index b9084ca..bad88c8 100755\n> --- a/t/t1400-update-ref.sh\n> +++ b/t/t1400-update-ref.sh\n> @@ -8,23 +8,33 @@ test_description='Test git update-ref and basic ref logging'\n> \n> Z=$_z40\n> \n> -test_expect_success setup '\n> +m=refs/heads/master\n> +n_dir=refs/heads/gu\n> +n=$n_dir/fixes\n> +outside=refs/foo\n> +bare=bare-repo\n> \n> +create_test_objects ()\n> +{\n> +\tlocal T, sha1, prfx=\"$1\"\n\nCodingGuidelines. Do not use bash-ism \"local\" (besides, I do not\nthink you want to have comma here).\n\n> \tfor name in A B C D E F\n> \tdo\n> \t\ttest_tick &&\n> \t\tT=$(git write-tree) &&\n> \t\tsha1=$(echo $name | git commit-tree $T) &&\n> -\t\teval $name=$sha1\n> +\t\teval $prfx$name=$sha1\n> \tdone\n> +}\n"},{"id":"310390","messageId":"20170127100948.29408-3-cornelius.weig@tngtech.com","threadId":"44963","inReplyTo":"20170127100948.29408-1-cornelius.weig@tngtech.com","subject":"[PATCH v3 3/3] update-ref: add test cases for bare repository","fromName":"","fromEmail":"cornelius.weig@tngtech.com","sentAt":"2017-01-27T10:09:48Z","receivedAt":"2017-01-27T10:22:09Z","isPatch":true,"sender":{"key":"cornelius.weig@tngtech.com","avatar":null},"body":"From: Cornelius Weig \n\nThe default behavior of update-ref to create reflogs differs in\nrepositories with worktree and bare ones. The existing tests cover only\nthe behavior of repositories with worktree.\n\nThis commit adds tests that assert the correct behavior in bare\nrepositories for update-ref. Two cases are covered:\n\n - If core.logAllRefUpdates is not set, no reflogs should be created\n - If core.logAllRefUpdates is true, reflogs should be created\n\nSigned-off-by: Cornelius Weig \n---\n\nNotes:\n Changes wrt v2:\n \tRemove bashism 'local' from test function\n\n t/t1400-update-ref.sh | 43 ++++++++++++++++++++++++++++++++++++-------\n 1 file changed, 36 insertions(+), 7 deletions(-)\n\ndiff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh\nindex b9084ca..b0ffc0b 100755\n--- a/t/t1400-update-ref.sh\n+++ b/t/t1400-update-ref.sh\n@@ -8,23 +8,33 @@ test_description='Test git update-ref and basic ref logging'\n \n Z=$_z40\n \n-test_expect_success setup '\n+m=refs/heads/master\n+n_dir=refs/heads/gu\n+n=$n_dir/fixes\n+outside=refs/foo\n+bare=bare-repo\n \n+create_test_commits ()\n+{\n+\tprfx=\"$1\"\n \tfor name in A B C D E F\n \tdo\n \t\ttest_tick &&\n \t\tT=$(git write-tree) &&\n \t\tsha1=$(echo $name | git commit-tree $T) &&\n-\t\teval $name=$sha1\n+\t\teval $prfx$name=$sha1\n \tdone\n+}\n \n+test_expect_success setup '\n+\tcreate_test_commits \"\" &&\n+\tmkdir $bare &&\n+\tcd $bare &&\n+\tgit init --bare &&\n+\tcreate_test_commits \"bare\" &&\n+\tcd -\n '\n \n-m=refs/heads/master\n-n_dir=refs/heads/gu\n-n=$n_dir/fixes\n-outside=refs/foo\n-\n test_expect_success \\\n \t\"create $m\" \\\n \t\"git update-ref $m $A &&\n@@ -93,6 +103,25 @@ test_expect_success 'update-ref creates reflogs with --create-reflog' '\n \tgit reflog exists $outside\n '\n \n+test_expect_success 'creates no reflog in bare repository' '\n+\tgit -C $bare update-ref $m $bareA &&\n+\tgit -C $bare rev-parse $bareA >expect &&\n+\tgit -C $bare rev-parse $m >actual &&\n+\ttest_cmp expect actual &&\n+\ttest_must_fail git -C $bare reflog exists $m\n+'\n+\n+test_expect_success 'core.logAllRefUpdates=true creates reflog in bare repository' '\n+\ttest_when_finished \"git -C $bare config --unset core.logAllRefUpdates && \\\n+\t\trm $bare/logs/$m\" &&\n+\tgit -C $bare config core.logAllRefUpdates true &&\n+\tgit -C $bare update-ref $m $bareB &&\n+\tgit -C $bare rev-parse $bareB >expect &&\n+\tgit -C $bare rev-parse $m >actual &&\n+\ttest_cmp expect actual &&\n+\tgit -C $bare reflog exists $m\n+'\n+\n test_expect_success 'core.logAllRefUpdates=true does not create reflog by default' '\n \ttest_config core.logAllRefUpdates true &&\n \ttest_when_finished \"git update-ref -d $outside\" &&\n-- \n2.10.2\n\n"},{"id":"310391","messageId":"20170127100948.29408-1-cornelius.weig@tngtech.com","threadId":"44963","inReplyTo":"xmqqvat11k1i.fsf@gitster.mtv.corp.google.com","subject":"[PATCH v3 1/3] config: add markup to core.logAllRefUpdates doc","fromName":"","fromEmail":"cornelius.weig@tngtech.com","sentAt":"2017-01-27T10:09:46Z","receivedAt":"2017-01-27T10:22:10Z","isPatch":true,"sender":{"key":"cornelius.weig@tngtech.com","avatar":null},"body":"From: Cornelius Weig \n\nSigned-off-by: Cornelius Weig \n---\n\nNotes:\n Changes wrt v2:\n \tRemove duplicated line.\n\n Documentation/config.txt | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex af2ae4c..c7d8a01 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -517,10 +517,10 @@ core.logAllRefUpdates::\n \t\"`$GIT_DIR/logs/`\", by appending the new and old\n \tSHA-1, the date/time and the reason of the update, but\n \tonly when the file exists. If this configuration\n-\tvariable is set to true, missing \"`$GIT_DIR/logs/`\"\n+\tvariable is set to `true`, missing \"`$GIT_DIR/logs/`\"\n \tfile is automatically created for branch heads (i.e. under\n-\trefs/heads/), remote refs (i.e. under refs/remotes/),\n-\tnote refs (i.e. under refs/notes/), and the symbolic ref HEAD.\n+\t`refs/heads/`), remote refs (i.e. under `refs/remotes/`),\n+\tnote refs (i.e. under `refs/notes/`), and the symbolic ref `HEAD`.\n +\n This information can be used to determine what commit\n was the tip of a branch \"2 days ago\".\n-- \n2.10.2\n\n"},{"id":"310392","messageId":"20170127100948.29408-2-cornelius.weig@tngtech.com","threadId":"44963","inReplyTo":"20170127100948.29408-1-cornelius.weig@tngtech.com","subject":"[PATCH v3 2/3] refs: add option core.logAllRefUpdates = always","fromName":"","fromEmail":"cornelius.weig@tngtech.com","sentAt":"2017-01-27T10:09:47Z","receivedAt":"2017-01-27T10:22:13Z","isPatch":true,"sender":{"key":"cornelius.weig@tngtech.com","avatar":null},"body":"From: Cornelius Weig \n\nWhen core.logallrefupdates is true, we only create a new reflog for refs\nthat are under certain well-known hierarchies. The reason is that we\nknow that some hierarchies (like refs/tags) are not meant to change, and\nthat unknown hierarchies might not want reflogs at all (e.g., a\nhypothetical refs/foo might be meant to change often and drop old\nhistory immediately).\n\nHowever, sometimes it is useful to override this decision and simply log\nfor all refs, because the safety and audit trail is more important than\nthe performance implications of keeping the log around.\n\nThis patch introduces a new \"always\" mode for the core.logallrefupdates\noption which will log updates to everything under refs/, regardless\nwhere in the hierarchy it is (we still will not log things like\nORIG_HEAD and FETCH_HEAD, which are known to be transient).\n\nBased-on-patch-by: Jeff King \nSigned-off-by: Cornelius Weig \nReviewed-by: Jeff King \n---\n\nNotes:\n Changes wrt v2:\n \n - change wording in commit message s/do not typically/are not meant to/;\n - in update_refs_for_switch move refname to the enclosing block, so that\n should_autocreate_reflog has access. Thanks Junio for spotting this\n potential bug early :)\n - add test that asserts reflogs are created for tags if\n logAllRefUpdates=always. The case with logAllRefUpdates=true is IMHO already\n covered by the default case. To make that clearer, I explicitly added\n logAllRefUpdates=true.\n \n When writing the test for git-tag, I realized that the option\n --no-create-reflog to git-tag does not take precedence over\n logAllRefUpdate=always. IOW the setting cannot be overridden on the command\n line. Do you think this is a defect or would it not be desirable to have this\n feature anyway?\n\n Documentation/config.txt | 2 ++\n Documentation/git-tag.txt | 3 ++-\n branch.c | 2 +-\n builtin/checkout.c | 7 +++----\n builtin/init-db.c | 2 +-\n cache.h | 9 ++++++++-\n config.c | 7 ++++++-\n environment.c | 2 +-\n refs.c | 15 ++++++++++-----\n refs.h | 2 ++\n refs/files-backend.c | 6 +++---\n refs/refs-internal.h | 2 --\n t/t1400-update-ref.sh | 37 +++++++++++++++++++++++++++++++++++++\n t/t7004-tag.sh | 8 ++++++++\n 14 files changed, 84 insertions(+), 20 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex c7d8a01..d1fab67 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -521,6 +521,8 @@ core.logAllRefUpdates::\n \tfile is automatically created for branch heads (i.e. under\n \t`refs/heads/`), remote refs (i.e. under `refs/remotes/`),\n \tnote refs (i.e. under `refs/notes/`), and the symbolic ref `HEAD`.\n+\tIf it is set to `always`, then a missing reflog is automatically\n+\tcreated for any ref under `refs/`.\n +\n This information can be used to determine what commit\n was the tip of a branch \"2 days ago\".\ndiff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt\nindex 5055a96..2ac25a9 100644\n--- a/Documentation/git-tag.txt\n+++ b/Documentation/git-tag.txt\n@@ -150,7 +150,8 @@ This option is only applicable when listing tags without annotation lines.\n \t'strip' removes both whitespace and commentary.\n \n --create-reflog::\n-\tCreate a reflog for the tag.\n+\tCreate a reflog for the tag. To globally enable reflogs for tags, see\n+\t`core.logAllRefUpdates` in linkgit:git-config[1].\n \n ::\n \tThe name of the tag to create, delete, or describe.\ndiff --git a/branch.c b/branch.c\nindex c431cbf..b955d4f 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -298,7 +298,7 @@ void create_branch(const char *name, const char *start_name,\n \t\t\t start_name);\n \n \tif (reflog)\n-\t\tlog_all_ref_updates = 1;\n+\t\tlog_all_ref_updates = LOG_REFS_NORMAL;\n \n \tif (!dont_change_ref) {\n \t\tstruct ref_transaction *transaction;\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex bfe685c..81ea2ed 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -612,14 +612,12 @@ static void update_refs_for_switch(const struct checkout_opts *opts,\n \tconst char *old_desc, *reflog_msg;\n \tif (opts->new_branch) {\n \t\tif (opts->new_orphan_branch) {\n-\t\t\tif (opts->new_branch_log && !log_all_ref_updates) {\n+\t\t\tconst char *refname = mkpathdup(\"refs/heads/%s\", opts->new_orphan_branch);\n+\t\t\tif (opts->new_branch_log && should_autocreate_reflog(refname)) {\n \t\t\t\tint ret;\n-\t\t\t\tchar *refname;\n \t\t\t\tstruct strbuf err = STRBUF_INIT;\n \n-\t\t\t\trefname = mkpathdup(\"refs/heads/%s\", opts->new_orphan_branch);\n \t\t\t\tret = safe_create_reflog(refname, 1, &err);\n-\t\t\t\tfree(refname);\n \t\t\t\tif (ret) {\n \t\t\t\t\tfprintf(stderr, _(\"Can not do reflog for '%s': %s\\n\"),\n \t\t\t\t\t\topts->new_orphan_branch, err.buf);\n@@ -628,6 +626,7 @@ static void update_refs_for_switch(const struct checkout_opts *opts,\n \t\t\t\t}\n \t\t\t\tstrbuf_release(&err);\n \t\t\t}\n+\t\t\tfree(refname);\n \t\t}\n \t\telse\n \t\t\tcreate_branch(opts->new_branch, new->name,\ndiff --git a/builtin/init-db.c b/builtin/init-db.c\nindex 76d68fa..1d4d6a0 100644\n--- a/builtin/init-db.c\n+++ b/builtin/init-db.c\n@@ -262,7 +262,7 @@ static int create_default_files(const char *template_path,\n \t\tconst char *work_tree = get_git_work_tree();\n \t\tgit_config_set(\"core.bare\", \"false\");\n \t\t/* allow template config file to override the default */\n-\t\tif (log_all_ref_updates == -1)\n+\t\tif (log_all_ref_updates == LOG_REFS_UNSET)\n \t\t\tgit_config_set(\"core.logallrefupdates\", \"true\");\n \t\tif (needs_work_tree_config(original_git_dir, work_tree))\n \t\t\tgit_config_set(\"core.worktree\", work_tree);\ndiff --git a/cache.h b/cache.h\nindex 00a029a..96eeaaf 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -660,7 +660,6 @@ extern int minimum_abbrev, default_abbrev;\n extern int ignore_case;\n extern int assume_unchanged;\n extern int prefer_symlink_refs;\n-extern int log_all_ref_updates;\n extern int warn_ambiguous_refs;\n extern int warn_on_object_refname_ambiguity;\n extern const char *apply_default_whitespace;\n@@ -728,6 +727,14 @@ enum hide_dotfiles_type {\n };\n extern enum hide_dotfiles_type hide_dotfiles;\n \n+enum log_refs_config {\n+\tLOG_REFS_UNSET = -1,\n+\tLOG_REFS_NONE = 0,\n+\tLOG_REFS_NORMAL,\n+\tLOG_REFS_ALWAYS\n+};\n+extern enum log_refs_config log_all_ref_updates;\n+\n enum branch_track {\n \tBRANCH_TRACK_UNSPECIFIED = -1,\n \tBRANCH_TRACK_NEVER = 0,\ndiff --git a/config.c b/config.c\nindex b680f79..c6b874a 100644\n--- a/config.c\n+++ b/config.c\n@@ -826,7 +826,12 @@ static int git_default_core_config(const char *var, const char *value)\n \t}\n \n \tif (!strcmp(var, \"core.logallrefupdates\")) {\n-\t\tlog_all_ref_updates = git_config_bool(var, value);\n+\t\tif (value && !strcasecmp(value, \"always\"))\n+\t\t\tlog_all_ref_updates = LOG_REFS_ALWAYS;\n+\t\telse if (git_config_bool(var, value))\n+\t\t\tlog_all_ref_updates = LOG_REFS_NORMAL;\n+\t\telse\n+\t\t\tlog_all_ref_updates = LOG_REFS_NONE;\n \t\treturn 0;\n \t}\n \ndiff --git a/environment.c b/environment.c\nindex 8a83101..c07fb17 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -21,7 +21,6 @@ int ignore_case;\n int assume_unchanged;\n int prefer_symlink_refs;\n int is_bare_repository_cfg = -1; /* unspecified */\n-int log_all_ref_updates = -1; /* unspecified */\n int warn_ambiguous_refs = 1;\n int warn_on_object_refname_ambiguity = 1;\n int ref_paranoia = -1;\n@@ -64,6 +63,7 @@ int merge_log_config = -1;\n int precomposed_unicode = -1; /* see probe_utf8_pathname_composition() */\n unsigned long pack_size_limit_cfg;\n enum hide_dotfiles_type hide_dotfiles = HIDE_DOTFILES_DOTGITONLY;\n+enum log_refs_config log_all_ref_updates = LOG_REFS_UNSET;\n \n #ifndef PROTECT_HFS_DEFAULT\n #define PROTECT_HFS_DEFAULT 0\ndiff --git a/refs.c b/refs.c\nindex 9bd0bc1..cd36b64 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -638,12 +638,17 @@ int copy_reflog_msg(char *buf, const char *msg)\n \n int should_autocreate_reflog(const char *refname)\n {\n-\tif (!log_all_ref_updates)\n+\tswitch (log_all_ref_updates) {\n+\tcase LOG_REFS_ALWAYS:\n+\t\treturn 1;\n+\tcase LOG_REFS_NORMAL:\n+\t\treturn starts_with(refname, \"refs/heads/\") ||\n+\t\t\tstarts_with(refname, \"refs/remotes/\") ||\n+\t\t\tstarts_with(refname, \"refs/notes/\") ||\n+\t\t\t!strcmp(refname, \"HEAD\");\n+\tdefault:\n \t\treturn 0;\n-\treturn starts_with(refname, \"refs/heads/\") ||\n-\t\tstarts_with(refname, \"refs/remotes/\") ||\n-\t\tstarts_with(refname, \"refs/notes/\") ||\n-\t\t!strcmp(refname, \"HEAD\");\n+\t}\n }\n \n int is_branch(const char *refname)\ndiff --git a/refs.h b/refs.h\nindex 6947843..9fbff90 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -64,6 +64,8 @@ int read_ref(const char *refname, unsigned char *sha1);\n \n int ref_exists(const char *refname);\n \n+int should_autocreate_reflog(const char *refname);\n+\n int is_branch(const char *refname);\n \n extern int refs_init_db(struct strbuf *err);\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex f902393..14b17a6 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2682,7 +2682,7 @@ static int files_rename_ref(struct ref_store *ref_store,\n \t}\n \n \tflag = log_all_ref_updates;\n-\tlog_all_ref_updates = 0;\n+\tlog_all_ref_updates = LOG_REFS_NONE;\n \tif (write_ref_to_lockfile(lock, orig_sha1, &err) ||\n \t commit_ref_update(refs, lock, orig_sha1, NULL, &err)) {\n \t\terror(\"unable to write current sha1 into %s: %s\", oldrefname, err.buf);\n@@ -2835,8 +2835,8 @@ static int log_ref_write_1(const char *refname, const unsigned char *old_sha1,\n {\n \tint logfd, result, oflags = O_APPEND | O_WRONLY;\n \n-\tif (log_all_ref_updates < 0)\n-\t\tlog_all_ref_updates = !is_bare_repository();\n+\tif (log_all_ref_updates == LOG_REFS_UNSET)\n+\t\tlog_all_ref_updates = is_bare_repository() ? LOG_REFS_NONE : LOG_REFS_NORMAL;\n \n \tresult = log_ref_setup(refname, logfile, err, flags & REF_FORCE_CREATE_REFLOG);\n \ndiff --git a/refs/refs-internal.h b/refs/refs-internal.h\nindex 708b260..25444cf 100644\n--- a/refs/refs-internal.h\n+++ b/refs/refs-internal.h\n@@ -133,8 +133,6 @@ int verify_refname_available(const char *newname,\n */\n int copy_reflog_msg(char *buf, const char *msg);\n \n-int should_autocreate_reflog(const char *refname);\n-\n /**\n * Information needed for a single ref update. Set new_sha1 to the new\n * value or to null_sha1 to delete the ref. To check the old value\ndiff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh\nindex d4fb977..b9084ca 100755\n--- a/t/t1400-update-ref.sh\n+++ b/t/t1400-update-ref.sh\n@@ -93,6 +93,42 @@ test_expect_success 'update-ref creates reflogs with --create-reflog' '\n \tgit reflog exists $outside\n '\n \n+test_expect_success 'core.logAllRefUpdates=true does not create reflog by default' '\n+\ttest_config core.logAllRefUpdates true &&\n+\ttest_when_finished \"git update-ref -d $outside\" &&\n+\tgit update-ref $outside $A &&\n+\tgit rev-parse $A >expect &&\n+\tgit rev-parse $outside >actual &&\n+\ttest_cmp expect actual &&\n+\ttest_must_fail git reflog exists $outside\n+'\n+\n+test_expect_success 'core.logAllRefUpdates=always creates reflog by default' '\n+\ttest_config core.logAllRefUpdates always &&\n+\ttest_when_finished \"git update-ref -d $outside\" &&\n+\tgit update-ref $outside $A &&\n+\tgit rev-parse $A >expect &&\n+\tgit rev-parse $outside >actual &&\n+\ttest_cmp expect actual &&\n+\tgit reflog exists $outside\n+'\n+\n+test_expect_success 'core.logAllRefUpdates=always creates no reflog for ORIG_HEAD' '\n+\ttest_config core.logAllRefUpdates always &&\n+\tgit update-ref ORIG_HEAD $A &&\n+\ttest_must_fail git reflog exists ORIG_HEAD\n+'\n+\n+test_expect_success '--no-create-reflog overrides core.logAllRefUpdates=always' '\n+\ttest_config core.logAllRefUpdates true &&\n+\ttest_when_finished \"git update-ref -d $outside\" &&\n+\tgit update-ref --no-create-reflog $outside $A &&\n+\tgit rev-parse $A >expect &&\n+\tgit rev-parse $outside >actual &&\n+\ttest_cmp expect actual &&\n+\ttest_must_fail git reflog exists $outside\n+'\n+\n test_expect_success \\\n \t\"create $m (by HEAD)\" \\\n \t\"git update-ref HEAD $A &&\n@@ -501,6 +537,7 @@ test_expect_success 'stdin does not create reflogs by default' '\n '\n \n test_expect_success 'stdin creates reflogs with --create-reflog' '\n+\ttest_when_finished \"git update-ref -d $outside\" &&\n \techo \"create $outside $m\" >stdin &&\n \tgit update-ref --create-reflog --stdin expect &&\ndiff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\nindex 1cfa8a2..1bf622d 100755\n--- a/t/t7004-tag.sh\n+++ b/t/t7004-tag.sh\n@@ -71,6 +71,7 @@ test_expect_success 'creating a tag for an unknown revision should fail' '\n \n # commit used in the tests, test_tick is also called here to freeze the date:\n test_expect_success 'creating a tag using default HEAD should succeed' '\n+\ttest_config core.logAllRefUpdates true &&\n \ttest_tick &&\n \techo foo >foo &&\n \tgit add foo &&\n@@ -90,6 +91,13 @@ test_expect_success '--create-reflog does not create reflog on failure' '\n \ttest_must_fail git reflog exists refs/tags/mytag\n '\n \n+test_expect_success 'option core.logAllRefUpdates=always creates reflog' '\n+\ttest_when_finished \"git tag -d tag_with_reflog\" &&\n+\ttest_config core.logAllRefUpdates always &&\n+\tgit tag tag_with_reflog &&\n+\tgit reflog exists refs/tags/tag_with_reflog\n+'\n+\n test_expect_success 'listing all tags if one exists should succeed' '\n \tgit tag -l &&\n \tgit tag\n-- \n2.10.2\n\n"},{"id":"310564","messageId":"xmqq37g0us5p.fsf@gitster.mtv.corp.google.com","threadId":"44963","inReplyTo":"20170127100948.29408-2-cornelius.weig@tngtech.com","subject":"Re: [PATCH v3 2/3] refs: add option core.logAllRefUpdates = always","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-01-30T21:58:10Z","receivedAt":"2017-01-30T21:58:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"cornelius.weig@tngtech.com writes:\n\n> Notes:\n> Changes wrt v2:\n> \n> - change wording in commit message s/do not typically/are not meant to/;\n> - in update_refs_for_switch move refname to the enclosing block, so that\n> should_autocreate_reflog has access. Thanks Junio for spotting this\n> potential bug early :)\n> - add test that asserts reflogs are created for tags if\n> logAllRefUpdates=always. The case with logAllRefUpdates=true is IMHO already\n> covered by the default case. To make that clearer, I explicitly added\n> logAllRefUpdates=true.\n\nThese look all sensible. Especially thanks for reordering the code\nto feed the real refname for the new branch in the \"checkout\"\ncodepath.\n\n> When writing the test for git-tag, I realized that the option\n> --no-create-reflog to git-tag does not take precedence over\n> logAllRefUpdate=always. IOW the setting cannot be overridden on the command\n> line. Do you think this is a defect or would it not be desirable to have this\n> feature anyway?\n\n\"--no-create-reflog\" should override the configuration set to \"true\"\nor \"always\". Also \"--create-reflog\" should override the\nconfiguration set to \"false\".\n\nIf the problem was inherited from the original code before your\nchange (e.g. you set logAllRefUpdates to true and then did\n\"update-ref --no-create-reflog refs/heads/foo\". Does the code\nbefore your change ignore the command lne option and create a reflog\nfor the branch?), then it would be ideal to fix the bug before this\nseries as a preparatory fix. If the problem was introduced by this\npatch set, then we would need a fix not to introduce it ;-)\n\n> diff --git a/builtin/checkout.c b/builtin/checkout.c\n> index bfe685c..81ea2ed 100644\n> --- a/builtin/checkout.c\n> +++ b/builtin/checkout.c\n> @@ -612,14 +612,12 @@ static void update_refs_for_switch(const struct checkout_opts *opts,\n> \tconst char *old_desc, *reflog_msg;\n> \tif (opts->new_branch) {\n> \t\tif (opts->new_orphan_branch) {\n> -\t\t\tif (opts->new_branch_log && !log_all_ref_updates) {\n> +\t\t\tconst char *refname = mkpathdup(\"refs/heads/%s\", opts->new_orphan_branch);\n> +\t\t\tif (opts->new_branch_log && should_autocreate_reflog(refname)) {\n> \t\t\t\tint ret;\n> -\t\t\t\tchar *refname;\n> \t\t\t\tstruct strbuf err = STRBUF_INIT;\n> \n> -\t\t\t\trefname = mkpathdup(\"refs/heads/%s\", opts->new_orphan_branch);\n> \t\t\t\tret = safe_create_reflog(refname, 1, &err);\n> -\t\t\t\tfree(refname);\n> \t\t\t\tif (ret) {\n> \t\t\t\t\tfprintf(stderr, _(\"Can not do reflog for '%s': %s\\n\"),\n> \t\t\t\t\t\topts->new_orphan_branch, err.buf);\n\nHere you need to have another free(), as this block makes an early\nreturn and you end up leaking refname.\n\n> diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\n> index 1cfa8a2..1bf622d 100755\n> --- a/t/t7004-tag.sh\n> +++ b/t/t7004-tag.sh\n> @@ -71,6 +71,7 @@ test_expect_success 'creating a tag for an unknown revision should fail' '\n> \n> # commit used in the tests, test_tick is also called here to freeze the date:\n> test_expect_success 'creating a tag using default HEAD should succeed' '\n> +\ttest_config core.logAllRefUpdates true &&\n> \ttest_tick &&\n> \techo foo >foo &&\n> \tgit add foo &&\n\nThis change is to make sure that 'true' does not affect tags (but\n'always' does as seen in the later new test)? I am just double\nchecking, not objecting.\n\nThanks.\n"},{"id":"310572","messageId":"xmqq8tpstaus.fsf@gitster.mtv.corp.google.com","threadId":"44963","inReplyTo":"xmqq37g0us5p.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3 2/3] refs: add option core.logAllRefUpdates = always","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-01-30T22:57:15Z","receivedAt":"2017-01-30T22:57:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n>> diff --git a/builtin/checkout.c b/builtin/checkout.c\n>> index bfe685c..81ea2ed 100644\n>> --- a/builtin/checkout.c\n>> +++ b/builtin/checkout.c\n>> @@ -612,14 +612,12 @@ static void update_refs_for_switch(const struct checkout_opts *opts,\n>> \tconst char *old_desc, *reflog_msg;\n>> \tif (opts->new_branch) {\n>> \t\tif (opts->new_orphan_branch) {\n>> -\t\t\tif (opts->new_branch_log && !log_all_ref_updates) {\n>> +\t\t\tconst char *refname = mkpathdup(\"refs/heads/%s\", opts->new_orphan_branch);\n>> ...\n>> \t\t\t\tif (ret) {\n>> \t\t\t\t\tfprintf(stderr, _(\"Can not do reflog for '%s': %s\\n\"),\n>> \t\t\t\t\t\topts->new_orphan_branch, err.buf);\n>\n> Here you need to have another free(), as this block makes an early\n> return and you end up leaking refname.\n\nI am building with the attached patch squashed on top. \n\nThe extra free(refname) is to plug the leak I pointed out, and the\ntype of refname is no longer const, because \"const char *\" cannot be\nfree()d without casting, and in this codepath I do not see a reason\nto mark it as const.\n\nWhen queued on top of 4e59582ff7 (\"Seventh batch for 2.12\",\n2017-01-23), however, this fails t2017#9 (orphan with -l makes\nreflog when core.logAllRefUpdates = false).\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 81ea2eda99..e1a60fd8ea 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -612,7 +612,9 @@ static void update_refs_for_switch(const struct checkout_opts *opts,\n \tconst char *old_desc, *reflog_msg;\n \tif (opts->new_branch) {\n \t\tif (opts->new_orphan_branch) {\n-\t\t\tconst char *refname = mkpathdup(\"refs/heads/%s\", opts->new_orphan_branch);\n+\t\t\tchar *refname;\n+\n+\t\t\trefname = mkpathdup(\"refs/heads/%s\", opts->new_orphan_branch);\n \t\t\tif (opts->new_branch_log && should_autocreate_reflog(refname)) {\n \t\t\t\tint ret;\n \t\t\t\tstruct strbuf err = STRBUF_INIT;\n@@ -622,6 +624,7 @@ static void update_refs_for_switch(const struct checkout_opts *opts,\n \t\t\t\t\tfprintf(stderr, _(\"Can not do reflog for '%s': %s\\n\"),\n \t\t\t\t\t\topts->new_orphan_branch, err.buf);\n \t\t\t\t\tstrbuf_release(&err);\n+\t\t\t\t\tfree(refname);\n \t\t\t\t\treturn;\n \t\t\t\t}\n \t\t\t\tstrbuf_release(&err);\n"},{"id":"310577","messageId":"20170130233702.o6naszpz32juf5gt@sigill.intra.peff.net","threadId":"44963","inReplyTo":"xmqq37g0us5p.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3 2/3] refs: add option core.logAllRefUpdates = always","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-01-30T23:37:03Z","receivedAt":"2017-01-30T23:37:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jan 30, 2017 at 01:58:10PM -0800, Junio C Hamano wrote:\n\n> > When writing the test for git-tag, I realized that the option\n> > --no-create-reflog to git-tag does not take precedence over\n> > logAllRefUpdate=always. IOW the setting cannot be overridden on the command\n> > line. Do you think this is a defect or would it not be desirable to have this\n> > feature anyway?\n> \n> \"--no-create-reflog\" should override the configuration set to \"true\"\n> or \"always\". Also \"--create-reflog\" should override the\n> configuration set to \"false\".\n> \n> If the problem was inherited from the original code before your\n> change (e.g. you set logAllRefUpdates to true and then did\n> \"update-ref --no-create-reflog refs/heads/foo\". Does the code\n> before your change ignore the command lne option and create a reflog\n> for the branch?), then it would be ideal to fix the bug before this\n> series as a preparatory fix. If the problem was introduced by this\n> patch set, then we would need a fix not to introduce it ;-)\n\nI hadn't thought about that. I think \"git branch --no-create-reflog\" has\nthe same problem in the existing code.\n\nI suspect nobody cares much in practice. Even if you say \"don't create a\nreflog now\", if you have core.logAllRefUpdates turned on, then it's\nlikely that some _other_ operation will create the reflog later\naccidentally (e.g., as soon as you \"git checkout foo && git commit\",\nyou'll get a reflog). I think you're fighting an uphill battle to turn\nlogAllRefUpdates on and then try to disable some reflogs selectively.\n\nSo I agree the current behavior is quietly broken, which is not good.\nBut I wonder if \"--no-create-reflog\" is really sane in the first place,\nand whether we might be better off to simply disallow it.\n\n-Peff\n"},{"id":"310600","messageId":"68b6ac92-459d-849d-9589-e1fa500e2572@tngtech.com","threadId":"44963","inReplyTo":"xmqq8tpstaus.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3 2/3] refs: add option core.logAllRefUpdates = always","fromName":"Cornelius Weig","fromEmail":"cornelius.weig@tngtech.com","sentAt":"2017-01-31T13:16:25Z","receivedAt":"2017-01-31T13:18:47Z","isPatch":true,"sender":{"key":"cornelius.weig@tngtech.com","avatar":null},"body":"Hi,\n\n> The extra free(refname) is to plug the leak I pointed out, and the\n> type of refname is no longer const, because \"const char *\" cannot be\n> free()d without casting, and in this codepath I do not see a reason\n> to mark it as const.\n\nOoops.. thanks for not yelling at me for that :-/\n\n> When queued on top of 4e59582ff7 (\"Seventh batch for 2.12\",\n> 2017-01-23), however, this fails t2017#9 (orphan with -l makes\n> reflog when core.logAllRefUpdates = false).\n\nAnd again, thanks for not yelling. I overlooked that the\n\"should_autocreate_reflog\" return value should have been negated as\nshown below. Should I resend this patch, or is it easier for you\nto do the change yourself?\n\n\nInterdiff v2..v3:\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 81ea2ed..1e8631a 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -612,8 +612,10 @@ static void update_refs_for_switch(const struct checkout_opts *opts,\n const char *old_desc, *reflog_msg;\n if (opts->new_branch) {\n if (opts->new_orphan_branch) {\n- const char *refname = mkpathdup(\"refs/heads/%s\", opts->new_orphan_branch);\n- if (opts->new_branch_log && should_autocreate_reflog(refname)) {\n+ char *refname;\n+\n+ refname = mkpathdup(\"refs/heads/%s\", opts->new_orphan_branch);\n+ if (opts->new_branch_log && !should_autocreate_reflog(refname)) {\n int ret;\n struct strbuf err = STRBUF_INIT;\n \n@@ -622,6 +624,7 @@ static void update_refs_for_switch(const struct checkout_opts *opts,\n fprintf(stderr, _(\"Can not do reflog for '%s': %s\\n\"),\n opts->new_orphan_branch, err.buf);\n strbuf_release(&err);\n+ free(refname);\n return;\n }\n strbuf_release(&err);\n"},{"id":"310602","messageId":"1e341485-6fb6-243a-0b27-4035789a6f2a@tngtech.com","threadId":"44963","inReplyTo":"20170130233702.o6naszpz32juf5gt@sigill.intra.peff.net","subject":"Re: [PATCH v3 2/3] refs: add option core.logAllRefUpdates = always","fromName":"Cornelius Weig","fromEmail":"cornelius.weig@tngtech.com","sentAt":"2017-01-31T14:00:33Z","receivedAt":"2017-01-31T14:00:42Z","isPatch":true,"sender":{"key":"cornelius.weig@tngtech.com","avatar":null},"body":"On 01/31/2017 12:37 AM, Jeff King wrote:\n> On Mon, Jan 30, 2017 at 01:58:10PM -0800, Junio C Hamano wrote:\n> \n>>> When writing the test for git-tag, I realized that the option\n>>> --no-create-reflog to git-tag does not take precedence over\n>>> logAllRefUpdate=always. IOW the setting cannot be overridden on the command\n>>> line. Do you think this is a defect or would it not be desirable to have this\n>>> feature anyway?\n>>\n>> \"--no-create-reflog\" should override the configuration set to \"true\"\n>> or \"always\". Also \"--create-reflog\" should override the\n>> configuration set to \"false\".\n>>\n>> If the problem was inherited from the original code before your\n>> change (e.g. you set logAllRefUpdates to true and then did\n>> \"update-ref --no-create-reflog refs/heads/foo\".\n\nI was actually not referring to update-ref, for which the\n--no-create-reflog option works fine. I was referring to git-tag which\nalso has the --create-reflog option. For git-tag, the current code does\nnot allow to override logAllRefUpdates=always with --no-create-reflog.\nOn the other hand logAllRefUpdates=false is overridden by \"git tag\n--create-reflog\". The reason is that the file-backend does allow to\nforce reflog creation, but it does not allow to force reflog\nnon-creation. I have a patch that amends this, but it's not pretty and I\ndon't think it will be useful (see last paragraph).\n\n> I hadn't thought about that. I think \"git branch --no-create-reflog\" has\n> the same problem in the existing code.\n\nYou are right, git-branch also ignores --no-create-reflog.\n\n> I suspect nobody cares much in practice. Even if you say \"don't create a\n> reflog now\", if you have core.logAllRefUpdates turned on, then it's\n> likely that some _other_ operation will create the reflog later\n> accidentally (e.g., as soon as you \"git checkout foo && git commit\",\n> you'll get a reflog). I think you're fighting an uphill battle to turn\n> logAllRefUpdates on and then try to disable some reflogs selectively.\n> \n> So I agree the current behavior is quietly broken, which is not good.\n> But I wonder if \"--no-create-reflog\" is really sane in the first place,\n> and whether we might be better off to simply disallow it.\n\nConcerning branches, I fully agree. For git-branch, the\n\"--no-create-reflog\" option does not make sense at all and should\nproduce an error.\n\nOn the other hand, for tags it may make sense to override\nlogAllRefUpdates=always. As tag updates come exclusively from\nforce-creating the same tag on another revision, a reflog will actually\nnot be created by accident.\n\n\nNevertheless, I don't think it is very useful to have the\n\"--no-create-reflog\" argument to any of git-branch or git-tag. It only\ntakes effect if a user has configured logAllRefUpdates=always, and he\nprobably has done that for a reason. Given that the overhead from a\nreflog is minuscule, IMHO no-one will ever bother about\n\"--no-create-reflog\".\n"},{"id":"310604","messageId":"xmqq7f5brw6z.fsf@gitster.mtv.corp.google.com","threadId":"44963","inReplyTo":"68b6ac92-459d-849d-9589-e1fa500e2572@tngtech.com","subject":"Re: [PATCH v3 2/3] refs: add option core.logAllRefUpdates = always","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-01-31T17:11:32Z","receivedAt":"2017-01-31T17:11:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Cornelius Weig writes:\n\n> And again, thanks for not yelling. I overlooked that the\n> \"should_autocreate_reflog\" return value should have been negated as\n> shown below.\n\nHeh---I AM blind. I didn't spot it even though I was staring at the\ncode and even tweaking it (for the constness thing).\n\n> Should I resend this patch, or is it easier for you\n> to do the change yourself?\n\nI can squash it in, now we have and the list saw all the bits\nnecessary.\n\nThanks for working on this.\n\n> Interdiff v2..v3:\n> diff --git a/builtin/checkout.c b/builtin/checkout.c\n> index 81ea2ed..1e8631a 100644\n> --- a/builtin/checkout.c\n> +++ b/builtin/checkout.c\n> @@ -612,8 +612,10 @@ static void update_refs_for_switch(const struct checkout_opts *opts,\n> const char *old_desc, *reflog_msg;\n> if (opts->new_branch) {\n> if (opts->new_orphan_branch) {\n> - const char *refname = mkpathdup(\"refs/heads/%s\", opts->new_orphan_branch);\n> - if (opts->new_branch_log && should_autocreate_reflog(refname)) {\n> + char *refname;\n> +\n> + refname = mkpathdup(\"refs/heads/%s\", opts->new_orphan_branch);\n> + if (opts->new_branch_log && !should_autocreate_reflog(refname)) {\n> int ret;\n> struct strbuf err = STRBUF_INIT;\n> \n> @@ -622,6 +624,7 @@ static void update_refs_for_switch(const struct checkout_opts *opts,\n> fprintf(stderr, _(\"Can not do reflog for '%s': %s\\n\"),\n> opts->new_orphan_branch, err.buf);\n> strbuf_release(&err);\n> + free(refname);\n> return;\n> }\n> strbuf_release(&err);\n"},{"id":"310606","messageId":"xmqqbmunrwbf.fsf@gitster.mtv.corp.google.com","threadId":"44963","inReplyTo":"20170130233702.o6naszpz32juf5gt@sigill.intra.peff.net","subject":"Re: [PATCH v3 2/3] refs: add option core.logAllRefUpdates = always","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-01-31T17:08:52Z","receivedAt":"2017-01-31T17:19:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> So I agree the current behavior is quietly broken, which is not good.\n> But I wonder if \"--no-create-reflog\" is really sane in the first place,\n> and whether we might be better off to simply disallow it.\n\nThanks for a reasoned argument and a reasonable justification. I\nagree with all that. \n\nI think it is probably a good idea to document the behaviour\n(i.e. \"--no-create\" single-shot from the command line is ignored).\nI am not sure we should error out, though, in order to \"disallow\"\nit---a documented silent no-op may be sufficient.\n"},{"id":"310609","messageId":"20170131182110.mothq33nhswlizsa@sigill.intra.peff.net","threadId":"44963","inReplyTo":"1e341485-6fb6-243a-0b27-4035789a6f2a@tngtech.com","subject":"Re: [PATCH v3 2/3] refs: add option core.logAllRefUpdates = always","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-01-31T18:21:11Z","receivedAt":"2017-01-31T18:21:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 31, 2017 at 03:00:33PM +0100, Cornelius Weig wrote:\n\n> Concerning branches, I fully agree. For git-branch, the\n> \"--no-create-reflog\" option does not make sense at all and should\n> produce an error.\n> \n> On the other hand, for tags it may make sense to override\n> logAllRefUpdates=always. As tag updates come exclusively from\n> force-creating the same tag on another revision, a reflog will actually\n> not be created by accident.\n\nHmm. I think you could also see tag creation and update via \"git fetch\",\nthough only with explicit refspecs, I think, not tag-following.\n\nSo I think ultimately you'd need to use \"git -c logallrefupdates=false\"\nif you want to override reflog options for all commands. A saner\ninterface would probably be put teaching the ref code to respect a\nconfigured list of exceptions (\"I do want reflogs for refs/tags/, but\nnot for refs/foo/\"). But I don't think it's sensible for anybody to go\nto the work of doing that, given that I haven't heard a single useful\nreason for --no-create-reflog in the first place.\n\nPersonally, I'd be fine with leaving it in its current state as a known\nbug that somebody may fix later, if they actually care. But if it is not\ntoo hard to fix while we are all thinking about it, we can do that.\n\n-Peff\n"},{"id":"310616","messageId":"ce8f90a6-d719-63c7-95d0-b2538270e263@tngtech.com","threadId":"44963","inReplyTo":"xmqqbmunrwbf.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3 2/3] refs: add option core.logAllRefUpdates = always","fromName":"Cornelius Weig","fromEmail":"cornelius.weig@tngtech.com","sentAt":"2017-01-31T20:28:43Z","receivedAt":"2017-01-31T20:28:53Z","isPatch":true,"sender":{"key":"cornelius.weig@tngtech.com","avatar":null},"body":"On 01/31/2017 06:08 PM, Junio C Hamano wrote:\n> I think it is probably a good idea to document the behaviour\n> (i.e. \"--no-create\" single-shot from the command line is ignored).\n> I am not sure we should error out, though, in order to \"disallow\"\n> it---a documented silent no-op may be sufficient.\n\nYes, maybe abort on seeing \"--no-create-reflog\" is a too drastic\nmeasure. I presume that the best place to have the documentation would\nbe to print a warning when seeing the ignored argument?\n\nOr did you just have man pages and code comment in mind?\n\nCheers,\n Cornelius\n"},{"id":"310622","messageId":"xmqq7f5aripw.fsf@gitster.mtv.corp.google.com","threadId":"44963","inReplyTo":"ce8f90a6-d719-63c7-95d0-b2538270e263@tngtech.com","subject":"Re: [PATCH v3 2/3] refs: add option core.logAllRefUpdates = always","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-01-31T22:02:35Z","receivedAt":"2017-01-31T22:02:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Cornelius Weig writes:\n\n> On 01/31/2017 06:08 PM, Junio C Hamano wrote:\n>> I think it is probably a good idea to document the behaviour\n>> (i.e. \"--no-create\" single-shot from the command line is ignored).\n>> I am not sure we should error out, though, in order to \"disallow\"\n>> it---a documented silent no-op may be sufficient.\n>\n> Yes, maybe abort on seeing \"--no-create-reflog\" is a too drastic\n> measure. I presume that the best place to have the documentation would\n> be to print a warning when seeing the ignored argument?\n>\n> Or did you just have man pages and code comment in mind?\n\nI meant only in the documentation, but \"you gave me a no-op option\"\nwarning would not hurt. I do not care too deeply either way.\n\n"}]}