Re: [PATCH v3 2/3] refs: add option core.logAllRefUpdates = always

4 messages, 3 authors, 2017-01-31 · open the first message on its own page

Re: [PATCH v3 2/3] refs: add option core.logAllRefUpdates = always

From: Junio C Hamano <hidden>
Date: 2017-01-30 21:58:54

cornelius.weig@tngtech.com writes:

Notes:
    Changes wrt v2:
    
     - change wording in commit message s/do not typically/are not meant to/;
     - in update_refs_for_switch move refname to the enclosing block, so that
       should_autocreate_reflog has access. Thanks Junio for spotting this
       potential bug early :)
     - add test that asserts reflogs are created for tags if
       logAllRefUpdates=always. The case with logAllRefUpdates=true is IMHO already
       covered by the default case. To make that clearer, I explicitly added
       logAllRefUpdates=true.

These look all sensible.  Especially thanks for reordering the code
to feed the real refname for the new branch in the "checkout"
codepath.

    When writing the test for git-tag, I realized that the option
    --no-create-reflog to git-tag does not take precedence over
    logAllRefUpdate=always. IOW the setting cannot be overridden on the command
    line. Do you think this is a defect or would it not be desirable to have this
    feature anyway?

"--no-create-reflog" should override the configuration set to "true"
or "always".  Also "--create-reflog" should override the
configuration set to "false".

If the problem was inherited from the original code before your
change (e.g. you set logAllRefUpdates to true and then did
"update-ref --no-create-reflog refs/heads/foo".  Does the code
before your change ignore the command lne option and create a reflog
for the branch?), then it would be ideal to fix the bug before this
series as a preparatory fix.  If the problem was introduced by this
patch set, then we would need a fix not to introduce it ;-)

quoted hunk

diff --git a/builtin/checkout.c b/builtin/checkout.c
index bfe685c..81ea2ed 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c

@@ -612,14 +612,12 @@ static void update_refs_for_switch(const struct checkout_opts *opts,
 	const char *old_desc, *reflog_msg;
 	if (opts->new_branch) {
 		if (opts->new_orphan_branch) {
-			if (opts->new_branch_log && !log_all_ref_updates) {
+			const char *refname = mkpathdup("refs/heads/%s", opts->new_orphan_branch);
+			if (opts->new_branch_log && should_autocreate_reflog(refname)) {
 				int ret;
-				char *refname;
 				struct strbuf err = STRBUF_INIT;
 
-				refname = mkpathdup("refs/heads/%s", opts->new_orphan_branch);
 				ret = safe_create_reflog(refname, 1, &err);
-				free(refname);
 				if (ret) {
 					fprintf(stderr, _("Can not do reflog for '%s': %s\n"),
 						opts->new_orphan_branch, err.buf);

Here you need to have another free(), as this block makes an early
return and you end up leaking refname.

quoted hunk

diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index 1cfa8a2..1bf622d 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh

@@ -71,6 +71,7 @@ test_expect_success 'creating a tag for an unknown revision should fail' '
 
 # commit used in the tests, test_tick is also called here to freeze the date:
 test_expect_success 'creating a tag using default HEAD should succeed' '
+	test_config core.logAllRefUpdates true &&
 	test_tick &&
 	echo foo >foo &&
 	git add foo &&

This change is to make sure that 'true' does not affect tags (but
'always' does as seen in the later new test)?  I am just double
checking, not objecting.

Thanks.

Re: [PATCH v3 2/3] refs: add option core.logAllRefUpdates = always

From: Jeff King <hidden>
Date: 2017-01-30 23:37:21

On Mon, Jan 30, 2017 at 01:58:10PM -0800, Junio C Hamano wrote:

quoted

    When writing the test for git-tag, I realized that the option
    --no-create-reflog to git-tag does not take precedence over
    logAllRefUpdate=always. IOW the setting cannot be overridden on the command
    line. Do you think this is a defect or would it not be desirable to have this
    feature anyway?

"--no-create-reflog" should override the configuration set to "true"
or "always".  Also "--create-reflog" should override the
configuration set to "false".

If the problem was inherited from the original code before your
change (e.g. you set logAllRefUpdates to true and then did
"update-ref --no-create-reflog refs/heads/foo".  Does the code
before your change ignore the command lne option and create a reflog
for the branch?), then it would be ideal to fix the bug before this
series as a preparatory fix.  If the problem was introduced by this
patch set, then we would need a fix not to introduce it ;-)

I hadn't thought about that. I think "git branch --no-create-reflog" has
the same problem in the existing code.

I suspect nobody cares much in practice. Even if you say "don't create a
reflog now", if you have core.logAllRefUpdates turned on, then it's
likely that some _other_ operation will create the reflog later
accidentally (e.g., as soon as you "git checkout foo && git commit",
you'll get a reflog). I think you're fighting an uphill battle to turn
logAllRefUpdates on and then try to disable some reflogs selectively.

So I agree the current behavior is quietly broken, which is not good.
But I wonder if "--no-create-reflog" is really sane in the first place,
and whether we might be better off to simply disallow it.

-Peff

Re: [PATCH v3 2/3] refs: add option core.logAllRefUpdates = always

From: Cornelius Weig <hidden>
Date: 2017-01-31 14:00:42

On 01/31/2017 12:37 AM, Jeff King wrote:

On Mon, Jan 30, 2017 at 01:58:10PM -0800, Junio C Hamano wrote:

quoted

    When writing the test for git-tag, I realized that the option
    --no-create-reflog to git-tag does not take precedence over
    logAllRefUpdate=always. IOW the setting cannot be overridden on the command
    line. Do you think this is a defect or would it not be desirable to have this
    feature anyway?

"--no-create-reflog" should override the configuration set to "true"
or "always".  Also "--create-reflog" should override the
configuration set to "false".

If the problem was inherited from the original code before your
change (e.g. you set logAllRefUpdates to true and then did
"update-ref --no-create-reflog refs/heads/foo".

I was actually not referring to update-ref, for which the
--no-create-reflog option works fine. I was referring to git-tag which
also has the --create-reflog option. For git-tag, the current code does
not allow to override logAllRefUpdates=always with --no-create-reflog.
On the other hand logAllRefUpdates=false is overridden by "git tag
--create-reflog". The reason is that the file-backend does allow to
force reflog creation, but it does not allow to force reflog
non-creation. I have a patch that amends this, but it's not pretty and I
don't think it will be useful (see last paragraph).

I hadn't thought about that. I think "git branch --no-create-reflog" has
the same problem in the existing code.

You are right, git-branch also ignores --no-create-reflog.

I suspect nobody cares much in practice. Even if you say "don't create a
reflog now", if you have core.logAllRefUpdates turned on, then it's
likely that some _other_ operation will create the reflog later
accidentally (e.g., as soon as you "git checkout foo && git commit",
you'll get a reflog). I think you're fighting an uphill battle to turn
logAllRefUpdates on and then try to disable some reflogs selectively.

So I agree the current behavior is quietly broken, which is not good.
But I wonder if "--no-create-reflog" is really sane in the first place,
and whether we might be better off to simply disallow it.

Concerning branches, I fully agree. For git-branch, the
"--no-create-reflog" option does not make sense at all and should
produce an error.

On the other hand, for tags it may make sense to override
logAllRefUpdates=always. As tag updates come exclusively from
force-creating the same tag on another revision, a reflog will actually
not be created by accident.


Nevertheless, I don't think it is very useful to have the
"--no-create-reflog" argument to any of git-branch or git-tag. It only
takes effect if a user has configured logAllRefUpdates=always, and he
probably has done that for a reason. Given that the overhead from a
reflog is minuscule, IMHO no-one will ever bother about
"--no-create-reflog".

Re: [PATCH v3 2/3] refs: add option core.logAllRefUpdates = always

From: Jeff King <hidden>
Date: 2017-01-31 18:21:46

On Tue, Jan 31, 2017 at 03:00:33PM +0100, Cornelius Weig wrote:

Concerning branches, I fully agree. For git-branch, the
"--no-create-reflog" option does not make sense at all and should
produce an error.

On the other hand, for tags it may make sense to override
logAllRefUpdates=always. As tag updates come exclusively from
force-creating the same tag on another revision, a reflog will actually
not be created by accident.

Hmm. I think you could also see tag creation and update via "git fetch",
though only with explicit refspecs, I think, not tag-following.

So I think ultimately you'd need to use "git -c logallrefupdates=false"
if you want to override reflog options for all commands. A saner
interface would probably be put teaching the ref code to respect a
configured list of exceptions ("I do want reflogs for refs/tags/, but
not for refs/foo/"). But I don't think it's sensible for anybody to go
to the work of doing that, given that I haven't heard a single useful
reason for --no-create-reflog in the first place.

Personally, I'd be fine with leaving it in its current state as a known
bug that somebody may fix later, if they actually care. But if it is not
too hard to fix while we are all thinking about it, we can do that.

-Peff

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help