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

David Turner <dturner@twopensource.com> writes:

Because?

	Because with core.logAllRefUpdates set to false, creating or
        updating a ref would not log what is done to it, unless a
        reflog already exists for the ref.  There are cases where
        we always want to have a reflog for a ref (e.g. refs/stash)
        regardless of the value of core.logAllRefUpdates, and we
        need a way to ensure that a reflog for a ref exists.
        "reflog create" is the way to do so.

	Also we need to be able to tell if a reflog for a ref
	exists, and "reflog exists" is the way to do so.

Now, going back to 4/6, I think create_reflog() function as an external API has a few problems.

 * Its name does not tell us what should happen when a reflog
   already exists for the refname the caller asked to "create"
   reflog for.  I understand that this only makes sure it exists and
   does not destroy existing one.  Its old name, log_ref_setup(),
   did not have this problem, but now it does.

 * Its second parameter that is a strbuf is used only internally
   from within log_ref_write_1(); all the external callers do not
   look at it and they are forced to strbuf_init() it before calling
   the function and to strbuf_release() after the function returns.

Oh, also 4/6 incorrectly says that log_ref_write() is renamed to create_reflog(), but it was log_ref_setup() that was renamed.

I think what 4/6 should have done in order to make the guts of what log_ref_setup() does available as a more useful external API is to

 * keep log_ref_setup() as-is, make it static to refs.c, and have
   the internal caller that cares about the strbuf (i.e. the path
   to the log file) call that; and

 * Add a thin-wrapper for callers that do not care about the path to
   the log file, e.g.

	int vivify_reflog(const char *refname, struct strbuf *err)
	{
		int ret;
		struct strbuf sb = STRBUF_INIT;

                ret = log_ref_setup(refname, &sb, err);
                strbuf_release(&sb);
                return ret;
	}

Hmm?