Re: [PATCH v6 5/7] refs: new public ref function: safe_create_reflog

[Michael Haggerty <mhagger@alum.mit.edu>]

On 06/29/2015 10:17 PM, David Turner wrote:
> The safe_create_reflog function creates a reflog, if it does not
> already exist.
> 
> The log_ref_setup function becomes private and gains a force_create
> parameter to force the creation of a reflog even if log_all_ref_updates
> is false or the refname is not one of the special refnames.
> 
> The new parameter also reduces the need to store, modify, and restore
> the log_all_ref_updates global before reflog creation.
> 
> In a moment, we will use this to add reflog creation commands to
> git-reflog.
> 
> Signed-off-by: David Turner <dturner@twopensource.com>
> ---
>  builtin/checkout.c | 10 +---------
>  refs.c             | 25 +++++++++++++++++++++----
>  refs.h             |  2 +-
>  3 files changed, 23 insertions(+), 14 deletions(-)
> 
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index 93f63d3..9f68399 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -620,19 +620,11 @@ static void update_refs_for_switch(const struct checkout_opts *opts,
>  	if (opts->new_branch) {
>  		if (opts->new_orphan_branch) {
>  			if (opts->new_branch_log && !log_all_ref_updates) {
> -				int temp;
> -				struct strbuf log_file = STRBUF_INIT;
> -				int ret;
>  				const char *ref_name;
>  				struct strbuf err = STRBUF_INIT;
>  
>  				ref_name = mkpath("refs/heads/%s", opts->new_orphan_branch);
> -				temp = log_all_ref_updates;
> -				log_all_ref_updates = 1;
> -				ret = log_ref_setup(ref_name, &log_file, &err);
> -				log_all_ref_updates = temp;
> -				strbuf_release(&log_file);
> -				if (ret) {
> +				if (safe_create_reflog(ref_name, &err, 1)) {
>  					fprintf(stderr, _("Can not do reflog for '%s'. %s\n"),
>  						opts->new_orphan_branch, err.buf);
>  					strbuf_release(&err);

This was probably already dangerous before your change, but...

mkpath() returns a pointer to a static buffer. It is subject to being
overwritten if any of a number of path-related functions is called. So
passing it into a function is dangerous.

Instead, you should store it into memory that you control, for example
by using a strbuf and strbuf_addf().

Also, we usually call variables holding reference names "refname", not
"ref_name". Maybe rename the variable while you are in the area.

> diff --git a/refs.c b/refs.c
> index 30e81ba..1e53ef0 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -3128,8 +3128,14 @@ static int should_autocreate_reflog(const char *refname)
>  		!strcmp(refname, "HEAD");
>  }
>  
> -/* This function will fill in *err and return -1 on failure */
> -int log_ref_setup(const char *refname, struct strbuf *sb_logfile, struct strbuf *err)
> +/*
> + * This function creates a reflog for a ref.  If force_create = 0, the
> + * reflog will only be created for certain refs (those for which
> + * should_autocreate_reflog returns non-zero.  Otherwise, it will be
> + * created regardless of the ref name.  This function will fill in *err
> + * and return -1 on failure
> + */

It is preferable to write function docstrings in the imperative voice:

    Create a reflog for a ref. If force_create == 0, only create
    the reflog for certain refs...

> +static int log_ref_setup(const char *refname, struct strbuf *sb_logfile, struct strbuf *err, int force_create)
>  {
>  	int logfd, oflags = O_APPEND | O_WRONLY;
>  	char *logfile;
> @@ -3138,7 +3144,7 @@ int log_ref_setup(const char *refname, struct strbuf *sb_logfile, struct strbuf
>  	logfile = sb_logfile->buf;
>  	/* make sure the rest of the function can't change "logfile" */
>  	sb_logfile = NULL;
> -	if (should_autocreate_reflog(refname)) {
> +	if (force_create || should_autocreate_reflog(refname)) {
>  		if (safe_create_leading_directories(logfile) < 0) {
>  			strbuf_addf(err, "unable to create directory for %s. "
>  				    "%s", logfile, strerror(errno));
> @@ -3173,6 +3179,17 @@ int log_ref_setup(const char *refname, struct strbuf *sb_logfile, struct strbuf
>  	return 0;
>  }
>  
> +
> +int safe_create_reflog(const char *refname, struct strbuf *err, int force_create)
> +{
> +	int ret;
> +	struct strbuf sb = STRBUF_INIT;
> +
> +	ret = log_ref_setup(refname, &sb, err, force_create);
> +	strbuf_release(&sb);
> +	return ret;
> +}
> +

Is it really necessary to have two functions, safe_create_reflog() and
log_ref_setup()? I don't see any of the callers doing anything special
with the sb_logfile argument from the latter, so maybe it could be
inlined into safe_create_reflog()? Maybe I'm overlooking something.

> [...]

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu