Re: [PATCH v6 6/7] git-reflog: add create and exists functions

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jun 30, 2015, 07:34 UTC
Message-ID: <CAPig+cROJJNTcZnZfMP0meA8ZWGcSHcQCMTCkuC+kn_+OQZ-zA@mail.gmail.com>
In-Reply-To: <1435609076-8592-7-git-send-email-dturner@twopensource.com>

On Mon, Jun 29, 2015 at 4:17 PM, David Turner <dturner@twopensource.com> wrote:

Show 46 quoted lines

> These are necessary because alternate ref backends might store reflogs
> somewhere other than .git/logs.  Code that now directly manipulates
> .git/logs should instead go through git-reflog.
>
> In a moment, we will use these functions to make git stash work with
> alternate ref backends.
>
> Signed-off-by: David Turner <dturner@twopensource.com>
> ---
> diff --git a/builtin/reflog.c b/builtin/reflog.c
> index c2eb8ff..3080865 100644
> --- a/builtin/reflog.c
> +++ b/builtin/reflog.c
> @@ -13,6 +13,10 @@ static const char reflog_expire_usage[] =
> +static int cmd_reflog_create(int argc, const char **argv, const char *prefix)
> +{
> +       int i, status = 0, start = 0;
> +       struct strbuf err = STRBUF_INIT;
> +
> +       for (i = 1; i < argc; i++) {
> +               const char *arg = argv[i];
> +               if (!strcmp(arg, "--")) {
> +                       i++;
> +                       break;
> +               }
> +               else if (arg[0] == '-')
> +                       usage(reflog_create_usage);
> +               else
> +                       break;
> +       }
> +
> +       start = i;
> +
> +       if (argc - start < 1)
> +               return error("Nothing to create?");
> +
> +       for (i = start; i < argc; i++) {
> +               if (check_refname_format(argv[i], REFNAME_ALLOW_ONELEVEL))
> +                       die("invalid ref format: %s", argv[i]);
> +       }
> +       for (i = start; i < argc; i++) {
> +               if (safe_create_reflog(argv[i], &err, 1)) {
> +                       error("could not create reflog %s: %s", argv[i],
> +                             err.buf);
> +                       status = 1;
> +                       strbuf_release(&err);

This feels a bit dirty. While it's true that the current implementation of strbuf_release() re-initializes the strbuf (so you're not technically wrong by re-using it), that's an implementation detail; and indeed, the strbuf_release() documentation says:

    Release a string buffer and the memory it used. You should not
    use the string buffer after using this function, unless you
    initialize it again.

Alternatives would be strbuf_reset() or declaring and releasing the strbuf within the for-loop scope.

> +               }
> +       }
> +       return status;
> +}