Re: [PATCH v3 2/3] refs: add option...

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

From: Junio C Hamano <hidden>
Date: 2017-01-30 22:57:27

Junio C Hamano [off-list ref] writes:

quoted

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 (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.

I am building with the attached patch squashed on top.  

The extra free(refname) is to plug the leak I pointed out, and the
type of refname is no longer const, because "const char *" cannot be
free()d without casting, and in this codepath I do not see a reason
to mark it as const.

When queued on top of 4e59582ff7 ("Seventh batch for 2.12",
2017-01-23), however, this fails t2017#9 (orphan with -l makes
reflog when core.logAllRefUpdates = false).

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 81ea2eda99..e1a60fd8ea 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c

@@ -612,7 +612,9 @@ 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) {
-			const char *refname = mkpathdup("refs/heads/%s", opts->new_orphan_branch);
+			char *refname;
+
+			refname = mkpathdup("refs/heads/%s", opts->new_orphan_branch);
 			if (opts->new_branch_log && should_autocreate_reflog(refname)) {
 				int ret;
 				struct strbuf err = STRBUF_INIT;

@@ -622,6 +624,7 @@ static void update_refs_for_switch(const struct checkout_opts *opts,
 					fprintf(stderr, _("Can not do reflog for '%s': %s\n"),
 						opts->new_orphan_branch, err.buf);
 					strbuf_release(&err);
+					free(refname);
 					return;
 				}
 				strbuf_release(&err);

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

From: Cornelius Weig <hidden>
Date: 2017-01-31 13:18:47

Hi,

The extra free(refname) is to plug the leak I pointed out, and the
type of refname is no longer const, because "const char *" cannot be
free()d without casting, and in this codepath I do not see a reason
to mark it as const.

Ooops.. thanks for not yelling at me for that :-/

When queued on top of 4e59582ff7 ("Seventh batch for 2.12",
2017-01-23), however, this fails t2017#9 (orphan with -l makes
reflog when core.logAllRefUpdates = false).

And again, thanks for not yelling. I overlooked that the
"should_autocreate_reflog" return value should have been negated as
shown below. Should I resend this patch, or is it easier for you
to do the change yourself?


Interdiff v2..v3:

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

@@ -612,8 +612,10 @@ 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) {
-                       const char *refname = mkpathdup("refs/heads/%s", opts->new_orphan_branch);
-                       if (opts->new_branch_log && should_autocreate_reflog(refname)) {
+                       char *refname;
+
+                       refname = mkpathdup("refs/heads/%s", opts->new_orphan_branch);
+                       if (opts->new_branch_log && !should_autocreate_reflog(refname)) {
                                int ret;
                                struct strbuf err = STRBUF_INIT;

@@ -622,6 +624,7 @@ static void update_refs_for_switch(const struct checkout_opts *opts,
                                        fprintf(stderr, _("Can not do reflog for '%s': %s\n"),
                                                opts->new_orphan_branch, err.buf);
                                        strbuf_release(&err);
+                                       free(refname);
                                        return;
                                }
                                strbuf_release(&err);

`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