Re: [PATCH v6 5/7] refs: new public ref function: safe_create_reflog
From: Michael Haggerty <hidden>
Date: 2016-06-15 23:05:39
On 06/29/2015 10:17 PM, David Turner wrote:
quoted hunk ↗ jump to hunk
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 <redacted> --- 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.
quoted hunk ↗ jump to hunk
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...
quoted hunk ↗ jump to hunk
+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