Re: [PATCH 3/4] refs.c: add a transaction function to append a reflog entry

[Jonathan Nieder <jrnieder@gmail.com>]

Stefan Beller wrote:
> On Tue, Dec 02, 2014 at 07:15:19PM -0800, Jonathan Nieder wrote:
>> Stefan Beller wrote:

>>> +int transaction_update_reflog(struct transaction *transaction,
>>> +			      const char *refname,
>>> +			      const unsigned char *new_sha1,
>>> +			      const unsigned char *old_sha1,
>>> +			      const char *email,
>>> +			      unsigned long timestamp, int tz,
>>> +			      const char *msg, int flags,
>>> +			      struct strbuf *err)
>>
>> This is an intimidating list of arguments.  Would it make sense to
>> pack them into a struct, or to make the list less intimidating
>> some other way (e.g. combining email + timestamp + tz into an
>> ident string)?
>
> It's true, that is's a huge list.
[...]
> If we want to change the function signature of transaction_update_reflog,
> I'd do it in a follow up?

The important change here is that it is moving from a static to a public
function.  In static functions, having a signature that is hard to work
with is okay because the possibility of confusion is restricted to
callers coordinating between each other in the same file.  Now that the
function is becoming public, it is time to think about the API.

[...]
>> Maybe I am misunderstanding the API.  If I use
>> transaction_update_reflog() and have not updated the reflog
>> previously, isn't this supposed to just append a new entry to the
>> reflog?
>
> He, that's indeed a good catch. I was investigating the API again myself and
> the REFLOG_TRUNCATE flag is only set on the very first call to transaction_update_reflog
> and the subsequent calls are rebuilding the reflog by adding the unpruned lines
> again line by line.

Taking a step back, there are two ways this API can be used:

 * commands like "git reflog expire" that filter the reflog want to
   create an entirely new reflog and copy entries one by one into it

 * commands like "git update-ref" that append to the reflog want to
   have the reflog as-is, plus some new entries added one-by-one

For that second use, the only operation needed is

	transaction_update_reflog

which would add a line to the reflog.  The first use can have that plus

	transaction_truncate_reflog

which would clear the reflog.

If I understood the API proposed by Ronnie, it combined those two
operations into a single function, transaction_truncate_reflog(flag).
The flag could be REFLOG_TRUNCATE to mean "truncate the reflog first",
analagous to the O_TRUNC flag in open(2).

Either API (the two-function version with no flag or one-function
version with flag) is basically equivalent --- they can be implemented
in terms of each other.  I slightly prefer the two-function API since
it makes the sequence of operations more obvious ("first truncate,
then append this, then append that, then...").

[...]
>>> +failure:
>>> +	strbuf_release(&buf);
>>> +	/*
>>> +	 * As we are using the lock file API, any stale files left behind will
>>> +	 * be taken care of, no need to do anything here.
>>> +	 */
>>
>> That's only true if the caller is going to exit instead of proceeding
>> to other work.
>>
>> With current callers, I assume that's true.  So should this comment
>> say something like "No need to roll back stale lock files because
>> the caller will exit soon"?  Or should this roll back the lockfile
>> anyway, in case the caller wants to try again?
>
> I am not sure if we ever want transactions be tried again without the user
> explicitely rerunning the command?
>
> As we're operating on a lockfile, which was just created by us, and now in
> the failure command, we're likely to die? Maybe I remove the comment altogether,
> as the wondering reader will look into the API for lock files, when looking
> for problems here.

It is tempting to roll back the lock file and keeping the transaction in OPEN
state would be best.  That way, the caller can keep going if they want
to --- it is as if the failing call never happened.

... except that rolling back the lockfile would mean rolling back previous
updates to the same reflog that had succeeded, too.

So I think closing the transaction like you do is the right thing to
do.  Rolling back the lockfile wouldn't be useful since there are
still other lockfiles to take care of from previous updates to other
reflogs that haven't been rolled back.  The comment should say that
the caller is going to exit.

Thanks,
Jonathan