v6, 7 partsrefs.c: add err arguments to reflog functions

> Add an err argument to log_ref_setup that can explain the reason
> for a failure. This then eliminates the need to manage errno through
> this function since we can just add strerror(errno) to the err string
> when meaningful. No callers relied on errno from this function for
> anything else than the error message.
> 
> Also add err arguments to private functions write_ref_to_lockfile,
> log_ref_write_1, commit_ref_update. This again eliminates the need to
> manage errno in these functions.
> 
> Update of a patch by Ronnie Sahlberg.

> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
> Signed-off-by: David Turner <dturner@twopensource.com>
> ---
>  builtin/checkout.c |   8 ++--
>  refs.c             | 111 ++++++++++++++++++++++++++++-------------------------
>  refs.h             |   4 +-
>  3 files changed, 66 insertions(+), 57 deletions(-)
> 
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> [...]
> diff --git a/refs.c b/refs.c
> index fb568d7..c97ca02 100644
> --- a/refs.c
> +++ b/refs.c
> [...]
> @@ -3216,26 +3217,25 @@ static int log_ref_write_1(const char *refname, const unsigned char *old_sha1,
>  	result = log_ref_write_fd(logfd, old_sha1, new_sha1,
>  				  git_committer_info(0), msg);
>  	if (result) {
> -		int save_errno = errno;
>  		close(logfd);
> -		error("Unable to append to %s", log_file);
> -		errno = save_errno;
> +		strbuf_addf(err, "Unable to append to %s. %s", log_file,
> +			    strerror(errno));
>  		return -1;

>  	}
>  	if (close(logfd)) {
> -		int save_errno = errno;
> -		error("Unable to append to %s", log_file);
> -		errno = save_errno;
> +		strbuf_addf(err, "Unable to append to %s. %s", log_file,
> +			    strerror(errno));
>  		return -1;
>  	}
>  	return 0;
>  }
>  
>  static int log_ref_write(const char *refname, const unsigned char *old_sha1,
> -			 const unsigned char *new_sha1, const char *msg)
> +			 const unsigned char *new_sha1, const char *msg,
> +			 struct strbuf *err)
>  {
>  	struct strbuf sb = STRBUF_INIT;
> -	int ret = log_ref_write_1(refname, old_sha1, new_sha1, msg, &sb);
> +	int ret = log_ref_write_1(refname, old_sha1, new_sha1, msg, &sb, err);
>  	strbuf_release(&sb);
>  	return ret;
>  }
> [...]
> @@ -3288,12 +3290,15 @@ static int write_ref_to_lockfile(struct ref_lock *lock,
>   * necessary, using the specified lockmsg (which can be NULL).
>   */
>  static int commit_ref_update(struct ref_lock *lock,
> -			     const unsigned char *sha1, const char *logmsg)
> +			     const unsigned char *sha1, const char *logmsg,
> +			     struct strbuf *err)
>  {
>  	clear_loose_ref_cache(&ref_cache);
> -	if (log_ref_write(lock->ref_name, lock->old_oid.hash, sha1, logmsg) < 0 ||
> +	if (log_ref_write(lock->ref_name, lock->old_oid.hash, sha1, logmsg, err) < 0 ||
>  	    (strcmp(lock->ref_name, lock->orig_ref_name) &&
> -	     log_ref_write(lock->orig_ref_name, lock->old_oid.hash, sha1, logmsg) < 0)) {
> +	     log_ref_write(lock->orig_ref_name, lock->old_oid.hash, sha1, logmsg, err) < 0)) {
> +		strbuf_addf(err, "Cannot update the ref '%s'.",
> +			    lock->ref_name);
>  		unlock_ref(lock);
>  		return -1;
>  	}

> @@ -3317,7 +3322,8 @@ static int commit_ref_update(struct ref_lock *lock,
>  					      head_sha1, &head_flag);
>  		if (head_ref && (head_flag & REF_ISSYMREF) &&
>  		    !strcmp(head_ref, lock->ref_name))
> -			log_ref_write("HEAD", lock->old_oid.hash, sha1, logmsg);
> +			log_ref_write("HEAD", lock->old_oid.hash, sha1, logmsg,
> +				      err);

>  	}
>  	if (commit_ref(lock)) {
>  		error("Couldn't set %s", lock->ref_name);
> @@ -3336,6 +3342,7 @@ int create_symref(const char *ref_target, const char *refs_heads_master,
>  	int fd, len, written;
>  	char *git_HEAD = git_pathdup("%s", ref_target);
>  	unsigned char old_sha1[20], new_sha1[20];
> +	struct strbuf err = STRBUF_INIT;
>  
>  	if (logmsg && read_ref(ref_target, old_sha1))
>  		hashclr(old_sha1);
> @@ -3384,8 +3391,11 @@ int create_symref(const char *ref_target, const char *refs_heads_master,
>  #ifndef NO_SYMLINK_HEAD
>  	done:
>  #endif
> -	if (logmsg && !read_ref(refs_heads_master, new_sha1))
> -		log_ref_write(ref_target, old_sha1, new_sha1, logmsg);
> +	if (logmsg && !read_ref(refs_heads_master, new_sha1) &&
> +		log_ref_write(ref_target, old_sha1, new_sha1, logmsg, &err)) {
> +		error("%s", err.buf);
> +		strbuf_release(&err);
> +	}
>  
>  	free(git_HEAD);
>  	return 0;
> @@ -4021,14 +4031,13 @@ int ref_transaction_commit(struct ref_transaction *transaction,
>  				 * value, so we don't need to write it.
>  				 */
>  			} else if (write_ref_to_lockfile(update->lock,
> -							 update->new_sha1)) {
> +							 update->new_sha1,
> +							 err)) {
>  				/*
>  				 * The lock was freed upon failure of
>  				 * write_ref_to_lockfile():
>  				 */
>  				update->lock = NULL;
> -				strbuf_addf(err, "cannot update the ref '%s'.",
> -					    update->refname);

>  				ret = TRANSACTION_GENERIC_ERROR;
>  				goto cleanup;
>  			} else {
> @@ -4054,11 +4063,9 @@ int ref_transaction_commit(struct ref_transaction *transaction,
>  
>  		if (update->flags & REF_NEEDS_COMMIT) {
>  			if (commit_ref_update(update->lock,
> -					      update->new_sha1, update->msg)) {
> +					      update->new_sha1, update->msg, err)) {
>  				/* freed by commit_ref_update(): */
>  				update->lock = NULL;
> -				strbuf_addf(err, "Cannot update the ref '%s'.",
> -					    update->refname);

>  				ret = TRANSACTION_GENERIC_ERROR;
>  				goto cleanup;
>  			} else {
> diff --git a/refs.h b/refs.h
> index e82fca5..debdefc 100644
> --- a/refs.h
> +++ b/refs.h
> @@ -226,9 +226,9 @@ int pack_refs(unsigned int flags);
>  #define REF_NODEREF	0x01
>  
>  /*
> - * Setup reflog before using. Set errno to something meaningful on failure.
> + * Setup reflog before using. Fill in err and return -1 on failure.
>   */
> -int log_ref_setup(const char *refname, struct strbuf *logfile);
> +int log_ref_setup(const char *refname, struct strbuf *logfile, struct strbuf *err);
>  
>  /** Reads log for the value of ref during at_time. **/
>  extern int read_ref_at(const char *refname, unsigned int flags,
> 

> On 06/29/2015 10:17 PM, David Turner wrote:
> > Add an err argument to log_ref_setup that can explain the reason
> > for a failure. This then eliminates the need to manage errno through
> > this function since we can just add strerror(errno) to the err string
> > when meaningful. No callers relied on errno from this function for
> > anything else than the error message.
> > 
> > Also add err arguments to private functions write_ref_to_lockfile,
> > log_ref_write_1, commit_ref_update. This again eliminates the need to
> > manage errno in these functions.
> > 
> > Update of a patch by Ronnie Sahlberg.
> 
> First a general comment: we have a convention, not yet very well adhered
> to, that error messages should start with lower-case letters. I know
> that in many cases you are just carrying along pre-existing error
> messages that are capitalized. But at a minimum, please avoid adding new
> error messages that are capitalized. And if you are feeling ambitious,
> feel free to lower-case some existing ones :-)

> Since you are passing err into log_ref_write(), I assume that it would
> already have an error message written to it by the time you enter into
> this block. Yet in the block you append more text to it.
> 
> It seems to me that you either want to clear err and replace it with
> your own message, or create a new message that looks like
> 
>     Cannot update the ref '%s': %s
> 
> where the second "%s" is replaced with the error from log_ref_write().

> > @@ -3317,7 +3322,8 @@ static int commit_ref_update(struct ref_lock *lock,
> >  					      head_sha1, &head_flag);
> >  		if (head_ref && (head_flag & REF_ISSYMREF) &&
> >  		    !strcmp(head_ref, lock->ref_name))
> > -			log_ref_write("HEAD", lock->old_oid.hash, sha1, logmsg);
> > +			log_ref_write("HEAD", lock->old_oid.hash, sha1, logmsg,
> > +				      err);
> 
> Here you don't check for errors from log_ref_write(). So it might or
> might not have written something to err. If it has, is that error
> handled correctly?

> Previously, the error generated here was "cannot update the ref '%s'."
> But now that you are letting write_ref_to_lockfile() fill err, the error
> message will be something from that function. This might be OK or it
> might not. If you think it is, it would be worth a word or two of
> justification in the commit message.

> On Mon, 2015-07-06 at 17:53 +0200, Michael Haggerty wrote:
>> On 06/29/2015 10:17 PM, David Turner wrote:
>>> [...]
>>> @@ -3317,7 +3322,8 @@ static int commit_ref_update(struct ref_lock *lock,
>>>  					      head_sha1, &head_flag);
>>>  		if (head_ref && (head_flag & REF_ISSYMREF) &&
>>>  		    !strcmp(head_ref, lock->ref_name))
>>> -			log_ref_write("HEAD", lock->old_oid.hash, sha1, logmsg);
>>> +			log_ref_write("HEAD", lock->old_oid.hash, sha1, logmsg,
>>> +				      err);
>>
>> Here you don't check for errors from log_ref_write(). So it might or
>> might not have written something to err. If it has, is that error
>> handled correctly?
> 
> That was the case before this patch too. But I guess I might as well
> check.

> I think your v7 of this patch goes too far, by turning a failure to
> write to the reflog into a failure of the whole transaction. The problem
> is that this failure comes too late, in the commit phase of the
> transaction. Aborting at this late stage can leave some references
> changed and others rolled back, violating the promise of atomicity.

> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
>> I think your v7 of this patch goes too far, by turning a failure to
>> write to the reflog into a failure of the whole transaction. The problem
>> is that this failure comes too late, in the commit phase of the
>> transaction. Aborting at this late stage can leave some references
>> changed and others rolled back, violating the promise of atomicity.
> 
> Yeah, that sounds problematic.
> 
>> The old code reported a failure to write the reflog to stderr but didn't
>> fail the transaction. I think that behavior is more appropriate. The
>> reflog is of lower importance than the references themselves. Junio, do
>> you agree?
> 
> That is actually a loaded question.
> 
> Do I agree that the current (i.e. before this change) behaviour is
> more appropriate given the current choice of representation of refs
> and reflogs on the filesystem, treating a failure to update reflog
> as lower importance event and accept it as a limitation that it
> cannot abort the whole transaction atomically?  Compared to leaving
> the repository in a half-updated state where some refs and their
> logs are updated already, other remaining proposed updates are
> ignored, and the transaction claims to have failed even though some
> things have already changed and we cannot rollback, I would say that
> is a better compromise to treat reflog update as a lower importance.
> 
> Do I agree that reflog writing should stay to be best-effort in the
> longer term?  Not really.  If we are moving the ref API in the
> direction where we can plug a backend that is different from the
> traditional filesystem based one for storing refs, I think the
> backend should also be responsible for storing logs for the refs it
> stores, and if the backend wants to promise atomicity, then we
> should be able to fail the whole transaction when updates to refs
> could proceed but updates to the log of one of these updated refs
> cannot.  So I do not agree to cast in stone "the reflog is of lower
> importance" as a general rule.

> Instead of directly writing to and reading from files in
> $GIT_DIR, use ref API to interact with CHERRY_PICK_HEAD
> and REVERT_HEAD.
> 
> Signed-off-by: David Turner <dturner@twopensource.com>
> ---
>  branch.c                         |  4 ++--
>  builtin/commit.c                 |  6 +++---
>  builtin/merge.c                  |  2 +-
>  contrib/completion/git-prompt.sh |  4 ++--
>  git-gui/lib/commit.tcl           |  2 +-
>  sequencer.c                      | 39 ++++++++++++++++++++-------------------
>  t/t7509-commit.sh                |  4 ++--
>  wt-status.c                      |  6 ++----
>  8 files changed, 33 insertions(+), 34 deletions(-)
> 
> [...]
> diff --git a/sequencer.c b/sequencer.c
> index f8421a8..44c43e5 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -158,21 +158,22 @@ static void free_message(struct commit *commit, struct commit_message *msg)
>  	unuse_commit_buffer(commit, msg->message);
>  }
>  
> -static void write_cherry_pick_head(struct commit *commit, const char *pseudoref)
> +static void write_cherry_pick_head(struct commit *commit, const char *ref)
>  {
> -	const char *filename;
> -	int fd;
> -	struct strbuf buf = STRBUF_INIT;
> +	struct strbuf err = STRBUF_INIT;
> +	void *transaction;
>  
> -	strbuf_addf(&buf, "%s\n", sha1_to_hex(commit->object.sha1));
> +	transaction = ref_transaction_begin(&err);
> +	if (!transaction)
> +		die(_("Could not create transaction: %s"), err.buf);
>  
> -	filename = git_path("%s", pseudoref);
> -	fd = open(filename, O_WRONLY | O_CREAT, 0666);
> -	if (fd < 0)
> -		die_errno(_("Could not open '%s' for writing"), filename);
> -	if (write_in_full(fd, buf.buf, buf.len) != buf.len || close(fd))
> -		die_errno(_("Could not write to '%s'"), filename);
> -	strbuf_release(&buf);
> +	if (ref_transaction_update(transaction, ref, commit->object.sha1,
> +				   NULL, REF_NODEREF, NULL,
> +				   &err))
> +		die(_("Could not write ref %s: %s"), ref, err.buf);
> +
> +	if (ref_transaction_commit(transaction, &err))
> +		die(_("Could not commit ref write %s: %s"), ref, err.buf);
>  }

> 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);

> 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
> + */

> +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;
> +}
> +

> > +
> > +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.

> On Mon, 2015-07-06 at 18:21 +0200, Michael Haggerty wrote:
> 
> <snip> changes applied; will re-roll.
> 
>>> +
>>> +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.
> 
> log_ref_write_1 does use the sb_logfile argument.

> 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);

>> +       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.

> Eric Sunshine <sunshine@sunshineco.com> writes:
>
>>> +       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.
>
> Hmm, I do not share that feeling.  I wouldn't be surprised if you
> found a lot of existing codepaths that run _init() a strbuf once,
> work on it and then _release() once a section of code is done with
> it, reuse it for further (unrelated) processing, knowing that
> _release() implicitly did _init() and the strbuf is ready to use
> after that.  I thought that was a very well established pattern.

>> 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.
>
> Hmph. Perhaps the doc is wrong? ;-)

>> Alternatives would be strbuf_reset() or declaring and releasing the
>> strbuf within the for-loop scope.
>
> Because _reset() just rewinds the .len pointer without deallocating,
> you would need an extra _release() before it goes out of scope. If
> it is expected that the strbuf will be reused for a number of times,
> the length of the string each iteration uses is similar, and you
> will iterate the loop many times, "_reset() each time and _release()
> to clean-up" pattern would save many calls to realloc/free.

>>> Alternatives would be strbuf_reset() or declaring and releasing the
>>> strbuf within the for-loop scope.
>>
>> Because _reset() just rewinds the .len pointer without deallocating,
>> you would need an extra _release() before it goes out of scope. If
>> it is expected that the strbuf will be reused for a number of times,
>> the length of the string each iteration uses is similar, and you
>> will iterate the loop many times, "_reset() each time and _release()
>> to clean-up" pattern would save many calls to realloc/free.
>
> Yep, that's why I suggested strbuf_reset() as an alternative (and
> likely would have chosen it myself).

> Eric Sunshine <sunshine@sunshineco.com> writes:
> 
> >>> Alternatives would be strbuf_reset() or declaring and releasing the
> >>> strbuf within the for-loop scope.
> >>
> >> Because _reset() just rewinds the .len pointer without deallocating,
> >> you would need an extra _release() before it goes out of scope. If
> >> it is expected that the strbuf will be reused for a number of times,
> >> the length of the string each iteration uses is similar, and you
> >> will iterate the loop many times, "_reset() each time and _release()
> >> to clean-up" pattern would save many calls to realloc/free.
> >
> > Yep, that's why I suggested strbuf_reset() as an alternative (and
> > likely would have chosen it myself).
> 
> OK, then let's do that by squashing this in.
> 
>  builtin/reflog.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

> On Tue, 2015-06-30 at 12:48 -0700, Junio C Hamano wrote:
>> Eric Sunshine <sunshine@sunshineco.com> writes:
>> 
>> >>> Alternatives would be strbuf_reset() or declaring and releasing the
>> >>> strbuf within the for-loop scope.
>> >>
>> >> Because _reset() just rewinds the .len pointer without deallocating,
>> >> you would need an extra _release() before it goes out of scope. If
>> >> it is expected that the strbuf will be reused for a number of times,
>> >> the length of the string each iteration uses is similar, and you
>> >> will iterate the loop many times, "_reset() each time and _release()
>> >> to clean-up" pattern would save many calls to realloc/free.
>> >
>> > Yep, that's why I suggested strbuf_reset() as an alternative (and
>> > likely would have chosen it myself).
>> 
>> OK, then let's do that by squashing this in.
>> 
>>  builtin/reflog.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> I squashed that into my repo on github:

> 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>
> ---
>  Documentation/git-reflog.txt | 10 ++++++
>  builtin/reflog.c             | 75 +++++++++++++++++++++++++++++++++++++++++++-
>  t/t1411-reflog-show.sh       | 12 +++++++
>  3 files changed, 96 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/git-reflog.txt b/Documentation/git-reflog.txt
> index 5e7908e..2bf8aa6 100644
> --- a/Documentation/git-reflog.txt
> +++ b/Documentation/git-reflog.txt
> @@ -23,6 +23,8 @@ depending on the subcommand:
>  	[--dry-run] [--verbose] [--all | <refs>...]
>  'git reflog delete' [--rewrite] [--updateref]
>  	[--dry-run] [--verbose] ref@\{specifier\}...
> +'git reflog create' <refs>...
> +'git reflog exists' <ref>
>  
>  Reference logs, or "reflogs", record when the tips of branches and
>  other references were updated in the local repository. Reflogs are
> @@ -52,6 +54,14 @@ argument must be an _exact_ entry (e.g. "`git reflog delete
>  master@{2}`"). This subcommand is also typically not used directly by
>  end users.
>  
> +The "create" subcommand creates a reflog for one or more refs. Most
> +refs (those under refs/heads, refs/remotes, and refs/tags) will
> +automatically have reflogs created. Other refs will not. This command
> +allows manual ref creation.
> +
> +The "exists" subcommand checks whether a ref has a reflog.  It exists
> +with zero status if the reflog exists, and non-zero status if it does
> +not.
>  
>  OPTIONS
>  -------
> 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[] =
>  "git reflog expire [--expire=<time>] [--expire-unreachable=<time>] [--rewrite] [--updateref] [--stale-fix] [--dry-run | -n] [--verbose] [--all] <refs>...";
>  static const char reflog_delete_usage[] =
>  "git reflog delete [--rewrite] [--updateref] [--dry-run | -n] [--verbose] <refs>...";
> +static const char reflog_create_usage[] =
> +"git reflog create <refs>...";
> +static const char reflog_exists_usage[] =
> +"git reflog exists <ref>";
>  
>  static unsigned long default_reflog_expire;
>  static unsigned long default_reflog_expire_unreachable;
> @@ -699,12 +703,75 @@ static int cmd_reflog_delete(int argc, const char **argv, const char *prefix)
>  	return status;
>  }
>  
> +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);
> +		}
> +	}
> +	return status;
> +}
> +

> +static int cmd_reflog_exists(int argc, const char **argv, const char *prefix)
> +{
> +	int i, start = 0;
> +
> +	for (i = 1; i < argc; i++) {
> +		const char *arg = argv[i];
> +		if (!strcmp(arg, "--")) {
> +			i++;
> +			break;
> +		}
> +		else if (arg[0] == '-')
> +			usage(reflog_exists_usage);
> +		else
> +			break;
> +	}
> +
> +	start = i;
> +
> +	if (argc - start != 1)
> +		usage(reflog_exists_usage);
> +
> +	if (check_refname_format(argv[start], REFNAME_ALLOW_ONELEVEL))
> +		die("invalid ref format: %s", argv[start]);
> +	return !reflog_exists(argv[start]);
> +}
> +
>  /*
>   * main "reflog"
>   */
>  
>  static const char reflog_usage[] =
> -"git reflog [ show | expire | delete ]";
> +"git reflog [ show | expire | delete | create | exists ]";
>  
>  int cmd_reflog(int argc, const char **argv, const char *prefix)
>  {
> @@ -724,5 +791,11 @@ int cmd_reflog(int argc, const char **argv, const char *prefix)
>  	if (!strcmp(argv[1], "delete"))
>  		return cmd_reflog_delete(argc - 1, argv + 1, prefix);
>  
> +	if (!strcmp(argv[1], "create"))
> +		return cmd_reflog_create(argc - 1, argv + 1, prefix);
> +
> +	if (!strcmp(argv[1], "exists"))
> +		return cmd_reflog_exists(argc - 1, argv + 1, prefix);
> +
>  	return cmd_log_reflog(argc, argv, prefix);
>  }
> diff --git a/t/t1411-reflog-show.sh b/t/t1411-reflog-show.sh
> index 6f47c0d..6e1abe7 100755
> --- a/t/t1411-reflog-show.sh
> +++ b/t/t1411-reflog-show.sh
> @@ -166,4 +166,16 @@ test_expect_success 'git log -g -p shows diffs vs. parents' '
>  	test_cmp expect actual
>  '
>  
> +test_expect_success 'reflog exists works' '
> +	git reflog exists refs/heads/master &&
> +	! git reflog exists refs/heads/nonexistent
> +'
> +
> +test_expect_success 'reflog create works' '
> +	git update-ref non-refs-dir HEAD &&
> +	! git reflog exists non-refs-dir &&
> +	git reflog create non-refs-dir &&
> +	git reflog exists non-refs-dir
> +'
> +
>  test_done
> 

> > +{
> > +	int i, status = 0, start = 0;
> 
> It looks like start is initialized unconditionally after the first loop,
> so the initialization here is a red herring.

> So, I have a philosophical question here with a practical side...
> 
> It appears that this command allows you to create a reflog for a
> reference that doesn't yet exist. At first blush, this seems to make
> sense. Suppose you want the creation of a reference to be logged. Then
> you can first turn on its reflog, then create it.
> 
> But I think this is going to create problems. Reflogs are rather
> intimately connected to their references. For example, writes to a
> reflog are guarded by locking the corresponding reference. And currently
> a reflog file is deleted when the corresponding reference is deleted.
> Also, there are constraints about which references can coexist; for
> example, references "refs/heads/foo" and "refs/heads/foo/bar" cannot
> exist at the same time, because (at least when using the filesystem
> backend), "refs/heads/foo" would have to be both a file and a directory
> at the same time. So if one of these references already exists, it would
> be an error to create a reflog for the other one.
> 
> Similarly, if there is a reflog file for one of these references that
> was created by this command, but there is not yet a corresponding
> reference, then any command that later tries to create the other
> reference will not be able to create the reflog for *that* reference
> because it will conflict with the existing reflog. I doubt that the
> reference creation is smart enough to deal with this situation.
> 
> So all in all, I think it is unwise to allow a reflog to be created
> without its corresponding reference.
> 
> This, in turn, suggests one or both of the following alternatives:
> 
> 1. Allow "git reflog create", but only for references that already exist.

> 2. If we want to allow a reflog to be created at the same time as the
> corresponding reference, the reference-creation commands ("git branch",
> "git tag", "git update-ref", and maybe some others) probably need a new
> option like "--create-reflog" (and, for symmetry, probably
> "--no-create-reflog").

> On Mon, 2015-07-06 at 18:51 +0200, Michael Haggerty wrote:
>> [...]
>> So all in all, I think it is unwise to allow a reflog to be created
>> without its corresponding reference.
>>
>> This, in turn, suggests one or both of the following alternatives:
>>
>> 1. Allow "git reflog create", but only for references that already exist.
> 
> This turns out not to work for git stash, which wants to create a reflog
> for stash creation.
> 
>> 2. If we want to allow a reflog to be created at the same time as the
>> corresponding reference, the reference-creation commands ("git branch",
>> "git tag", "git update-ref", and maybe some others) probably need a new
>> option like "--create-reflog" (and, for symmetry, probably
>> "--no-create-reflog").
> 
> git branch should already autocreate reflogs, since the refs it creates
> are under refs/heads.

>> At the API level, it might make sense for the ref-transaction functions
>> to get a new "REF_FORCE_CREATE_REFLOG" flag or something.
> 
> Junio was opposed to the converse flag, so I'm going to just add
> manually add code to create reflogs.

> On 07/08/2015 02:49 AM, David Turner wrote:
> > On Mon, 2015-07-06 at 18:51 +0200, Michael Haggerty wrote:
> >> [...]
> >> So all in all, I think it is unwise to allow a reflog to be created
> >> without its corresponding reference.
> >>
> >> This, in turn, suggests one or both of the following alternatives:
> >>
> >> 1. Allow "git reflog create", but only for references that already exist.
> > 
> > This turns out not to work for git stash, which wants to create a reflog
> > for stash creation.
> > 
> >> 2. If we want to allow a reflog to be created at the same time as the
> >> corresponding reference, the reference-creation commands ("git branch",
> >> "git tag", "git update-ref", and maybe some others) probably need a new
> >> option like "--create-reflog" (and, for symmetry, probably
> >> "--no-create-reflog").
> > 
> > git branch should already autocreate reflogs, since the refs it creates
> > are under refs/heads.
> 
> `git branch` only autocreates reflogs if core.logAllRefUpdates is on.
> That setting happens to be on by default in a non-bare repository but
> the user might turn it off. And it is off by default in a bare repository.

> In my opinion it would be nice for the user to be able to ask for a
> reflog to be created for a branch regardless of how
> core.logAllRefUpdates is set. Though I'm not saying that you have to be
> the one to implement that functionality :-)
> 
> >> At the API level, it might make sense for the ref-transaction functions
> >> to get a new "REF_FORCE_CREATE_REFLOG" flag or something.
> > 
> > Junio was opposed to the converse flag, so I'm going to just add
> > manually add code to create reflogs.
> 
> Unfortunately I wasn't keeping up with earlier versions of this patch
> series and now I can't find the email from Junio that you are referring
> to. If the earlier flag had the opposite ("converse"?) sense, like
> REF_INHIBIT_CREATE_REFLOG, then I agree that it wouldn't be an improvement.
> 
> But I think this functionality *has to* be implemented within ref
> transactions for references that are just being created, because
> 
> 1. The reflog must *not* be created if the reference creation fails for
> some reason. For example, the reflog shouldn't be created if the
> reference name has a D/F conflict with an existing one in the "refs/foo"
> vs. "refs/foo/bar" sense. (This conflict might not be obvious when
> creating the reflog file because the other reference might not have its
> reflog turned on.) There are other reasons that a reference creation
> might fail, and code outside of the refs API can't be expected to know
> all possibilities.
> 
> 2. On the other hand, the reflog for a newly-created reference *should*
> reflect the creation of the reference. So it would be awkward to require
> the calling code to create the reference and *then* turn on the reflog.

> For references that already exist, I see no problem with a command that
> turns on the reflog without adding any entries to it. Though if you
> implement this, it would be prudent to check that existing
> reflog-handling code doesn't fail when confronted with an empty file; I
> think empty reflog files are rare now and might not be well-tested.

> David Turner <dturner@twopensource.com> writes:
> 
> > Minor formatting fixes from Junio Hamano.
> 
> There is another.