Re: [PATCH v16 Part II 2/8] bisect--helper: `bisect_write` shell function in C

[Johannes Schindelin]

Hi Martin,

On Fri, 23 Nov 2018, Martin Ågren wrote:

> On Fri, 23 Nov 2018 at 11:13, Johannes Schindelin
>  wrote:
> > On Mon, 30 Oct 2017, Pranit Bauva wrote:
> >
> > > On Fri, Oct 27, 2017 at 10:58 PM, Martin Ågren  
> > > wrote:
> > > > On 27 October 2017 at 17:06, Pranit Bauva  
> > > > wrote:
> > > >> +static void free_terms(struct bisect_terms *terms)
> > > >> +{
> > > >> +   if (!terms->term_good)
> > > >> +   free((void *) terms->term_good);
> > > >> +   if (!terms->term_bad)
> > > >> +   free((void *) terms->term_bad);
> > > >> +}
> 
> > > > You leave the pointers dangling, but you're ok for now since this is the
> > > > last thing that happens in `cmd_bisect__helper()`. Your later patches
> > > > add more users, but they're also ok, since they immediately assign new
> > > > values.
> > > >
> > > > In case you (and others) find it useful, the below is a patch I've been
> > > > sitting on for a while as part of a series to plug various memory-leaks.
> > > > `FREE_AND_NULL_CONST()` would be useful in precisely these situations.
> > >
> > > Honestly, I wouldn't be the best person to judge this.
> >
> > Git's source code implicitly assumes that any `const` pointer refers to
> > memory owned by another code path. It is therefore probably not a good
> > idea to encourage `free()`ing a `const` pointer.
> 
> Yeah, I never submitted that patch as part of a real series. I remember
> having a funky feeling about it, and whatever use-case I had for this
> macro, I managed to solve the memory leak in some other way in a
> rewrite.
> 
> Thanks for a sanity check.

I am glad you agree, and it's just fair that I contribute a sanity check
on this here list when I have benefitted so many times from the same.

Ciao,
Johannes

Re: [PATCH v16 Part II 2/8] bisect--helper: `bisect_write` shell function in C

[Martin Ågren]

On Fri, 23 Nov 2018 at 11:13, Johannes Schindelin
 wrote:
> On Mon, 30 Oct 2017, Pranit Bauva wrote:
>
> > On Fri, Oct 27, 2017 at 10:58 PM, Martin Ågren  
> > wrote:
> > > On 27 October 2017 at 17:06, Pranit Bauva  wrote:
> > >> +static void free_terms(struct bisect_terms *terms)
> > >> +{
> > >> +   if (!terms->term_good)
> > >> +   free((void *) terms->term_good);
> > >> +   if (!terms->term_bad)
> > >> +   free((void *) terms->term_bad);
> > >> +}

> > > You leave the pointers dangling, but you're ok for now since this is the
> > > last thing that happens in `cmd_bisect__helper()`. Your later patches
> > > add more users, but they're also ok, since they immediately assign new
> > > values.
> > >
> > > In case you (and others) find it useful, the below is a patch I've been
> > > sitting on for a while as part of a series to plug various memory-leaks.
> > > `FREE_AND_NULL_CONST()` would be useful in precisely these situations.
> >
> > Honestly, I wouldn't be the best person to judge this.
>
> Git's source code implicitly assumes that any `const` pointer refers to
> memory owned by another code path. It is therefore probably not a good
> idea to encourage `free()`ing a `const` pointer.

Yeah, I never submitted that patch as part of a real series. I remember
having a funky feeling about it, and whatever use-case I had for this
macro, I managed to solve the memory leak in some other way in a
rewrite.

Thanks for a sanity check.

Martin

Re: [PATCH v16 Part II 2/8] bisect--helper: `bisect_write` shell function in C

[Johannes Schindelin]

Hi Pranit,

(Cc:ing Tanushree because they will try to pick up this patch series as
part of the Outreachy program.)

On Mon, 30 Oct 2017, Pranit Bauva wrote:

> On Fri, Oct 27, 2017 at 10:58 PM, Martin Ågren  wrote:
> > On 27 October 2017 at 17:06, Pranit Bauva  wrote:
> >> +static void free_terms(struct bisect_terms *terms)
> >> +{
> >> +   if (!terms->term_good)
> >> +   free((void *) terms->term_good);
> >> +   if (!terms->term_bad)
> >> +   free((void *) terms->term_bad);
> >> +}
> >
> > These look like no-ops. Remove `!` for correctness, or `if (...)` for
> > simplicity, since `free()` can handle NULL.
> 
> I probably forgot to do this here. I will make the change.
> 
> > You leave the pointers dangling, but you're ok for now since this is the
> > last thing that happens in `cmd_bisect__helper()`. Your later patches
> > add more users, but they're also ok, since they immediately assign new
> > values.
> >
> > In case you (and others) find it useful, the below is a patch I've been
> > sitting on for a while as part of a series to plug various memory-leaks.
> > `FREE_AND_NULL_CONST()` would be useful in precisely these situations.
> 
> Honestly, I wouldn't be the best person to judge this.

Git's source code implicitly assumes that any `const` pointer refers to
memory owned by another code path. It is therefore probably not a good
idea to encourage `free()`ing a `const` pointer.

Which brings me back to the question: who really owns that allocated
memory to which `term_good` and `term_bad` refer?

Ciao,
Johannes

Re: [PATCH v16 Part II 2/8] bisect--helper: `bisect_write` shell function in C

[Ramsay Jones]

On 27/10/17 16:06, Pranit Bauva wrote:
> Reimplement the `bisect_write` shell function in C and add a
> `bisect-write` subcommand to `git bisect--helper` to call it from
> git-bisect.sh
> 
> Using `--bisect-write` subcommand is a temporary measure to port shell
> function in C so as to use the existing test suite. As more functions
> are ported, this subcommand will be retired but its implementation will
> be called by some other methods.
> 
> Note: bisect_write() uses two variables namely TERM_GOOD and TERM_BAD
> from the global shell script thus we need to pass it to the subcommand
> using the arguments. We then store them in a struct bisect_terms and
> pass the memory address around functions.
> 
> Add a log_commit() helper function to write the contents of the commit message
> header to a file which will be re-used in future parts of the code as
> well.
> 
> Also introduce a function free_terms() to free the memory of `struct
> bisect_terms` and set_terms() to set the values of members in `struct
> bisect_terms`.
> 
> Helped-by: Ramsay Jones 
> Mentored-by: Lars Schneider 
> Mentored-by: Christian Couder 
> Signed-off-by: Pranit Bauva 
> ---
>  builtin/bisect--helper.c | 107 
> +--
>  git-bisect.sh|  25 ++-
>  2 files changed, 108 insertions(+), 24 deletions(-)
> 
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index 12754448f7b6a..6295f53c850a8 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> @@ -12,15 +12,37 @@ static GIT_PATH_FUNC(git_path_bisect_expected_rev, 
> "BISECT_EXPECTED_REV")
>  static GIT_PATH_FUNC(git_path_bisect_ancestors_ok, "BISECT_ANCESTORS_OK")
>  static GIT_PATH_FUNC(git_path_bisect_start, "BISECT_START")
>  static GIT_PATH_FUNC(git_path_bisect_head, "BISECT_HEAD")
> +static GIT_PATH_FUNC(git_path_bisect_log, "BISECT_LOG")
>  
>  static const char * const git_bisect_helper_usage[] = {
>   N_("git bisect--helper --next-all [--no-checkout]"),
>   N_("git bisect--helper --write-terms  "),
>   N_("git bisect--helper --bisect-clean-state"),
>   N_("git bisect--helper --bisect-reset []"),
> + N_("git bisect--helper --bisect-write
>  []"),
>   NULL
>  };
>  
> +struct bisect_terms {
> + const char *term_good;
> + const char *term_bad;
> +};
> +
> +static void free_terms(struct bisect_terms *terms)
> +{
> + if (!terms->term_good)
> + free((void *) terms->term_good);> + if (!terms->term_bad)
> + free((void *) terms->term_bad);
You can pass a NULL pointer to free(), so the conditionals here
are not required.

> +}
> +
> +static void set_terms(struct bisect_terms *terms, const char *bad,
> +   const char *good)
> +{
> + terms->term_good = xstrdup(good);
> + terms->term_bad = xstrdup(bad);
> +}
> +
>  /*
>   * Check whether the string `term` belongs to the set of strings
>   * included in the variable arguments.
> @@ -146,6 +168,72 @@ static int bisect_reset(const char *commit)
>   return bisect_clean_state();
>  }
>  
> +static void log_commit(FILE *fp, char *fmt, const char *state,
> +struct commit *commit)
> +{
> + struct pretty_print_context pp = {0};
> + struct strbuf commit_msg = STRBUF_INIT;
> + char *label = xstrfmt(fmt, state);
> +
> + format_commit_message(commit, "%s", _msg, );
> +
> + fprintf(fp, "# %s: [%s] %s\n", label, oid_to_hex(>object.oid),
> + commit_msg.buf);
> +
> + strbuf_release(_msg);
> + free(label);
> +}
> +
> +static int bisect_write(const char *state, const char *rev,
> + const struct bisect_terms *terms, int nolog)
> +{
> + struct strbuf tag = STRBUF_INIT;
> + struct object_id oid;
> + struct commit *commit;
> + FILE *fp = NULL;
> + int retval = 0;
> +
> + if (!strcmp(state, terms->term_bad)) {
> + strbuf_addf(, "refs/bisect/%s", state);
> + } else if (one_of(state, terms->term_good, "skip", NULL)) {
> + strbuf_addf(, "refs/bisect/%s-%s", state, rev);
> + } else {
> + error(_("Bad bisect_write argument: %s"), state);
> + goto fail;
> + }
> +
> + if (get_oid(rev, )) {
> + error(_("couldn't get the oid of the rev '%s'"), rev);
> + goto fail;
> + }
> +
> + if (update_ref(NULL, tag.buf, , NULL, 0,
> +UPDATE_REFS_MSG_ON_ERR))
> + goto fail;
> +
> + fp = fopen(git_path_bisect_log(), "a");
> + if (!fp) {
> + error_errno(_("couldn't open the file '%s'"), 
> git_path_bisect_log());
> + goto fail;
> + }
> +
> + commit = lookup_commit_reference();
> + log_commit(fp, "%s", state, commit);
> +
> + if (!nolog)
> + fprintf(fp, "git bisect %s %s\n", state, rev);
> +
> + goto finish;

Re: [PATCH v16 Part II 2/8] bisect--helper: `bisect_write` shell function in C

[Pranit Bauva]

Hey Junio,

On Fri, Oct 27, 2017 at 11:49 PM, Junio C Hamano  wrote:
> Pranit Bauva  writes:
>
>> - bisect_write "$state" "$rev"
>> + git bisect--helper --bisect-write "$state" "$rev" "$TERM_GOOD" 
>> "$TERM_BAD" || exit
>
> I can see why two extra "terms" parameters need to be passed to this
> helper at this step; looking at patches around 4/8 and 6/8 where C
> code can directly find out what words are used for GOOD and BAD, we
> should be able to lose these two extra parameters from this helper
> by internally making a call to get_terms() from bisect_write() ;-)

Yes quite true, but then after converting bisect_skip() we can
completely get rid of this line and then it won't be needed in the
ported C code.

PS: I have already ported that function but those patches are local as of now.

Regards,
Pranit Bauva

Re: [PATCH v16 Part II 2/8] bisect--helper: `bisect_write` shell function in C

[Pranit Bauva]

Hey Martin,

On Fri, Oct 27, 2017 at 10:58 PM, Martin Ågren  wrote:
> On 27 October 2017 at 17:06, Pranit Bauva  wrote:
>> +static void free_terms(struct bisect_terms *terms)
>> +{
>> +   if (!terms->term_good)
>> +   free((void *) terms->term_good);
>> +   if (!terms->term_bad)
>> +   free((void *) terms->term_bad);
>> +}
>
> These look like no-ops. Remove `!` for correctness, or `if (...)` for
> simplicity, since `free()` can handle NULL.

I probably forgot to do this here. I will make the change.

> You leave the pointers dangling, but you're ok for now since this is the
> last thing that happens in `cmd_bisect__helper()`. Your later patches
> add more users, but they're also ok, since they immediately assign new
> values.
>
> In case you (and others) find it useful, the below is a patch I've been
> sitting on for a while as part of a series to plug various memory-leaks.
> `FREE_AND_NULL_CONST()` would be useful in precisely these situations.

Honestly, I wouldn't be the best person to judge this.

Regards,
Pranit Bauva

Re: [PATCH v16 Part II 2/8] bisect--helper: `bisect_write` shell function in C

[Stephan Beyer]

On 10/30/2017 02:38 PM, Stephan Beyer wrote:
> This last change is not necessary. You never use "res".

Whoops, ignore this!
I compared old and new diff and mixed something up, it seems.

Sorry,
Stephan

Re: [PATCH v16 Part II 2/8] bisect--helper: `bisect_write` shell function in C

[Stephan Beyer]

Also just a minor in addition to the others:

> @@ -153,9 +241,10 @@ int cmd_bisect__helper(int argc, const char **argv, 
> const char *prefix)
>   WRITE_TERMS,
>   BISECT_CLEAN_STATE,
>   CHECK_EXPECTED_REVS,
> - BISECT_RESET
> + BISECT_RESET,
> + BISECT_WRITE
>   } cmdmode = 0;
> - int no_checkout = 0;
> + int no_checkout = 0, res = 0;

This last change is not necessary. You never use "res".

Stephan

Re: [PATCH v16 Part II 2/8] bisect--helper: `bisect_write` shell function in C

[Junio C Hamano]

Pranit Bauva  writes:

> - bisect_write "$state" "$rev"
> + git bisect--helper --bisect-write "$state" "$rev" "$TERM_GOOD" 
> "$TERM_BAD" || exit

I can see why two extra "terms" parameters need to be passed to this
helper at this step; looking at patches around 4/8 and 6/8 where C
code can directly find out what words are used for GOOD and BAD, we
should be able to lose these two extra parameters from this helper
by internally making a call to get_terms() from bisect_write() ;-)

Re: [PATCH v16 Part II 2/8] bisect--helper: `bisect_write` shell function in C

[Martin Ågren]

On 27 October 2017 at 17:06, Pranit Bauva  wrote:
> +static void free_terms(struct bisect_terms *terms)
> +{
> +   if (!terms->term_good)
> +   free((void *) terms->term_good);
> +   if (!terms->term_bad)
> +   free((void *) terms->term_bad);
> +}

These look like no-ops. Remove `!` for correctness, or `if (...)` for
simplicity, since `free()` can handle NULL.

You leave the pointers dangling, but you're ok for now since this is the
last thing that happens in `cmd_bisect__helper()`. Your later patches
add more users, but they're also ok, since they immediately assign new
values.

In case you (and others) find it useful, the below is a patch I've been
sitting on for a while as part of a series to plug various memory-leaks.
`FREE_AND_NULL_CONST()` would be useful in precisely these situations.

-- >8 --
Subject: git-compat-util: add FREE_AND_NULL_CONST() wrapper

Commit 481df65 ("git-compat-util: add a FREE_AND_NULL() wrapper around
free(ptr); ptr = NULL", 2017-06-15) added `FREE_AND_NULL()`. One
advantage of this macro is that it reduces risk of copy-paste errors and
reviewer-fatigue, especially when cleaning up more than just a single
pointer.

However, `FREE_AND_NULL()` can not be used with a const-pointer:

  struct foo { const char *bar; }
  ...
  FREE_AND_NULL(baz.bar);

This causes the compiler to barf as `free()` is called: "error: passing
argument 1 of ‘free’ discards ‘const’ qualifier from pointer target
type". A naive attempt to remedy this might look like this:

  FREE_AND_NULL((void *)baz.bar);

Now the assignment is problematic: "error: lvalue required as left
operand of assignment".

Add a `FREE_AND_NULL_CONST()` wrapper macro which acts as
`FREE_AND_NULL()`, except it casts to `(void *)` when freeing. As a
demonstration, use this in two existing code paths that were identified
by some grepping. Future patches will use it to clean up struct-fields:

  FREE_AND_NULL_CONST(baz.bar);

This macro is a slightly bigger hammer than `FREE_AND_NULL` and has a
slightly larger potential for misuse. Document that `FREE_AND_NULL`
should be preferred.

Signed-off-by: Martin Ågren 
---
 argv-array.c  | 3 +--
 git-compat-util.h | 8 
 submodule.c   | 3 +--
 3 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/argv-array.c b/argv-array.c
index 5d370fa33..433a5997a 100644
--- a/argv-array.c
+++ b/argv-array.c
@@ -59,8 +59,7 @@ void argv_array_pop(struct argv_array *array)
 {
if (!array->argc)
return;
-   free((char *)array->argv[array->argc - 1]);
-   array->argv[array->argc - 1] = NULL;
+   FREE_AND_NULL_CONST(array->argv[array->argc - 1]);
array->argc--;
 }
 
diff --git a/git-compat-util.h b/git-compat-util.h
index cedad4d58..ca3dcbc58 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -815,6 +815,14 @@ extern FILE *fopen_or_warn(const char *path, const char 
*mode);
  */
 #define FREE_AND_NULL(p) do { free(p); (p) = NULL; } while (0)
 
+/*
+ * FREE_AND_NULL_CONST(ptr) is like FREE_AND_NULL(ptr) except it casts
+ * to (void *) when calling free. This is useful when handling, e.g., a
+ * `const char *`, but it can also be misused. Prefer FREE_AND_NULL, and
+ * use this only when you need to and it is safe to do so.
+ */
+#define FREE_AND_NULL_CONST(p) do { free((void *)(p)); (p) = NULL; } while (0)
+
 #define ALLOC_ARRAY(x, alloc) (x) = xmalloc(st_mult(sizeof(*(x)), (alloc)))
 #define REALLOC_ARRAY(x, alloc) (x) = xrealloc((x), st_mult(sizeof(*(x)), 
(alloc)))
 
diff --git a/submodule.c b/submodule.c
index 63e7094e1..7759f9050 100644
--- a/submodule.c
+++ b/submodule.c
@@ -364,8 +364,7 @@ int parse_submodule_update_strategy(const char *value,
 {
enum submodule_update_type type;
 
-   free((void*)dst->command);
-   dst->command = NULL;
+   FREE_AND_NULL_CONST(dst->command);
 
type = parse_submodule_update_type(value);
if (type == SM_UPDATE_UNSPECIFIED)
-- 
2.15.0.rc2.5.g97dd00bb7

[PATCH v16 Part II 2/8] bisect--helper: `bisect_write` shell function in C

[Pranit Bauva]

Reimplement the `bisect_write` shell function in C and add a
`bisect-write` subcommand to `git bisect--helper` to call it from
git-bisect.sh

Using `--bisect-write` subcommand is a temporary measure to port shell
function in C so as to use the existing test suite. As more functions
are ported, this subcommand will be retired but its implementation will
be called by some other methods.

Note: bisect_write() uses two variables namely TERM_GOOD and TERM_BAD
from the global shell script thus we need to pass it to the subcommand
using the arguments. We then store them in a struct bisect_terms and
pass the memory address around functions.

Add a log_commit() helper function to write the contents of the commit message
header to a file which will be re-used in future parts of the code as
well.

Also introduce a function free_terms() to free the memory of `struct
bisect_terms` and set_terms() to set the values of members in `struct
bisect_terms`.

Helped-by: Ramsay Jones 
Mentored-by: Lars Schneider 
Mentored-by: Christian Couder 
Signed-off-by: Pranit Bauva 
---
 builtin/bisect--helper.c | 107 +--
 git-bisect.sh|  25 ++-
 2 files changed, 108 insertions(+), 24 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 12754448f7b6a..6295f53c850a8 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -12,15 +12,37 @@ static GIT_PATH_FUNC(git_path_bisect_expected_rev, 
"BISECT_EXPECTED_REV")
 static GIT_PATH_FUNC(git_path_bisect_ancestors_ok, "BISECT_ANCESTORS_OK")
 static GIT_PATH_FUNC(git_path_bisect_start, "BISECT_START")
 static GIT_PATH_FUNC(git_path_bisect_head, "BISECT_HEAD")
+static GIT_PATH_FUNC(git_path_bisect_log, "BISECT_LOG")
 
 static const char * const git_bisect_helper_usage[] = {
N_("git bisect--helper --next-all [--no-checkout]"),
N_("git bisect--helper --write-terms  "),
N_("git bisect--helper --bisect-clean-state"),
N_("git bisect--helper --bisect-reset []"),
+   N_("git bisect--helper --bisect-write
 []"),
NULL
 };
 
+struct bisect_terms {
+   const char *term_good;
+   const char *term_bad;
+};
+
+static void free_terms(struct bisect_terms *terms)
+{
+   if (!terms->term_good)
+   free((void *) terms->term_good);
+   if (!terms->term_bad)
+   free((void *) terms->term_bad);
+}
+
+static void set_terms(struct bisect_terms *terms, const char *bad,
+ const char *good)
+{
+   terms->term_good = xstrdup(good);
+   terms->term_bad = xstrdup(bad);
+}
+
 /*
  * Check whether the string `term` belongs to the set of strings
  * included in the variable arguments.
@@ -146,6 +168,72 @@ static int bisect_reset(const char *commit)
return bisect_clean_state();
 }
 
+static void log_commit(FILE *fp, char *fmt, const char *state,
+  struct commit *commit)
+{
+   struct pretty_print_context pp = {0};
+   struct strbuf commit_msg = STRBUF_INIT;
+   char *label = xstrfmt(fmt, state);
+
+   format_commit_message(commit, "%s", _msg, );
+
+   fprintf(fp, "# %s: [%s] %s\n", label, oid_to_hex(>object.oid),
+   commit_msg.buf);
+
+   strbuf_release(_msg);
+   free(label);
+}
+
+static int bisect_write(const char *state, const char *rev,
+   const struct bisect_terms *terms, int nolog)
+{
+   struct strbuf tag = STRBUF_INIT;
+   struct object_id oid;
+   struct commit *commit;
+   FILE *fp = NULL;
+   int retval = 0;
+
+   if (!strcmp(state, terms->term_bad)) {
+   strbuf_addf(, "refs/bisect/%s", state);
+   } else if (one_of(state, terms->term_good, "skip", NULL)) {
+   strbuf_addf(, "refs/bisect/%s-%s", state, rev);
+   } else {
+   error(_("Bad bisect_write argument: %s"), state);
+   goto fail;
+   }
+
+   if (get_oid(rev, )) {
+   error(_("couldn't get the oid of the rev '%s'"), rev);
+   goto fail;
+   }
+
+   if (update_ref(NULL, tag.buf, , NULL, 0,
+  UPDATE_REFS_MSG_ON_ERR))
+   goto fail;
+
+   fp = fopen(git_path_bisect_log(), "a");
+   if (!fp) {
+   error_errno(_("couldn't open the file '%s'"), 
git_path_bisect_log());
+   goto fail;
+   }
+
+   commit = lookup_commit_reference();
+   log_commit(fp, "%s", state, commit);
+
+   if (!nolog)
+   fprintf(fp, "git bisect %s %s\n", state, rev);
+
+   goto finish;
+
+fail:
+   retval = -1;
+finish:
+   if (fp)
+   fclose(fp);
+   strbuf_release();
+   return retval;
+}
+
 int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 {
enum {
@@ -153,9 +241,10 @@ int cmd_bisect__helper(int argc,