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@xxxxxxxxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxx
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html