From: Michael Haggerty <hidden> Date: 2016-06-15 23:03:12
I propose this patch series as an alternative to Ronnie's "reflog
transactions" series.
Both alternatives have been discussed exhaustively on the list [1,2].
My opinion is that there is no need to allow arbitrary reflog changes
via the ref_transaction API, because just about the only things we
want to do are
* Append an entry to a reflog when a reference is updated. This should
(and is already) done as a side effect of the reference update.
* Expire or delete old reflog entries based on relatively complicated
logic, possibly repairing the remaining entries so as to preserve
the continuity of the reflog chain.
This patch series shows that the latter can be done with a single,
fairly simple, purpose-made function, expire_reflog(), in the
references API. The policy for what reflog entries should be expired
is specified by the caller via three callback functions that don't
have to know anything about how reflogs are stored. The locking,
iteration, repair, and writing is implemented within the references
module, in a function that can easily be swapped out when pluggable
reference backends are implemented.
The remaining reflog operations (enabling/disabling reflogs for a
reference, renaming the reflog when a reference is renamed) are not
especially difficult but will be brought into the same framework in a
future patch series.
The first few patches and the last few are taken from Ronnie's and
Stefan's work. I chose *not* to rename the ref_transaction functions
for obvious reasons. A couple of the later patches from their series
would make sense but are not duplicated here.
This branch is also available on GitHub:
https://github.com/mhagger/git.git, branch reflog-expire-api-v1
Michael
[1] http://thread.gmane.org/gmane.comp.version-control.git/259712/focus=259770
[2] http://thread.gmane.org/gmane.comp.version-control.git/260731/focus=260767
Michael Haggerty (17):
expire_reflog(): remove unused parameter
expire_reflog(): rename "ref" parameter to "refname"
expire_reflog(): exit early if the reference has no reflog
expire_reflog(): use a lock_file for rewriting the reflog file
Extract function should_expire_reflog_ent()
expire_reflog(): extract two policy-related functions
expire_reflog(): add a "flags" argument
expire_reflog(): move dry_run to flags argument
expire_reflog(): move updateref to flags argument
Rename expire_reflog_cb to expire_reflog_policy_cb
struct expire_reflog_cb: a new callback data type
expire_reflog(): pass flags through to expire_reflog_ent()
expire_reflog(): move verbose to flags argument
expire_reflog(): move rewrite to flags argument
Move newlog and last_kept_sha1 to "struct expire_reflog_cb"
expire_reflog(): treat the policy callback data as opaque
reflog_expire(): new function in the reference API
Ronnie Sahlberg (5):
refs.c: make ref_transaction_create a wrapper for
ref_transaction_update
refs.c: make ref_transaction_delete a wrapper for
ref_transaction_update
refs.c: add a function to append a reflog entry to a fd
refs.c: remove unlock_ref/close_ref/commit_ref from the refs api
lock_any_ref_for_update(): inline function
Stefan Beller (1):
refs.c: don't expose the internal struct ref_lock in the header file
builtin/reflog.c | 259 ++++++++++++++++++++++---------------------------------
refs.c | 251 +++++++++++++++++++++++++++++++++++------------------
refs.h | 74 ++++++++++------
3 files changed, 319 insertions(+), 265 deletions(-)
--
2.1.3
From: Michael Haggerty <hidden> Date: 2016-06-15 23:03:12
From: Ronnie Sahlberg <redacted>
The ref_transaction_update function can already be used to create refs by
passing null_sha1 as the old_sha1 parameter. Simplify by replacing
transaction_create with a thin wrapper.
Signed-off-by: Ronnie Sahlberg <redacted>
Signed-off-by: Stefan Beller <redacted>
Reviewed-by: Michael Haggerty <redacted>
Reviewed-by: Jonathan Nieder <redacted>
Signed-off-by: Michael Haggerty <redacted>
---
refs.c | 27 ++-------------------------
1 file changed, 2 insertions(+), 25 deletions(-)
@@ -3623,31 +3623,8 @@ int ref_transaction_create(struct ref_transaction *transaction,intflags,constchar*msg,structstrbuf*err){-structref_update*update;--assert(err);--if(transaction->state!=REF_TRANSACTION_OPEN)-die("BUG: create called for transaction that is not open");--if(!new_sha1||is_null_sha1(new_sha1))-die("BUG: create ref with null new_sha1");--if(check_refname_format(refname,REFNAME_ALLOW_ONELEVEL)){-strbuf_addf(err,"refusing to create ref with bad name %s",-refname);-return-1;-}--update=add_update(transaction,refname);--hashcpy(update->new_sha1,new_sha1);-hashclr(update->old_sha1);-update->flags=flags;-update->have_old=1;-if(msg)-update->msg=xstrdup(msg);-return0;+returnref_transaction_update(transaction,refname,new_sha1,+null_sha1,flags,1,msg,err);}intref_transaction_delete(structref_transaction*transaction,
From: Michael Haggerty <hidden> Date: 2016-06-15 23:03:12
It was called "unused", so at least it was self-consistent.
Signed-off-by: Michael Haggerty <redacted>
---
builtin/reflog.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
@@ -3633,26 +3633,8 @@ int ref_transaction_delete(struct ref_transaction *transaction,intflags,inthave_old,constchar*msg,structstrbuf*err){-structref_update*update;--assert(err);--if(transaction->state!=REF_TRANSACTION_OPEN)-die("BUG: delete called for transaction that is not open");--if(have_old&&!old_sha1)-die("BUG: have_old is true but old_sha1 is NULL");--update=add_update(transaction,refname);-update->flags=flags;-update->have_old=have_old;-if(have_old){-assert(!is_null_sha1(old_sha1));-hashcpy(update->old_sha1,old_sha1);-}-if(msg)-update->msg=xstrdup(msg);-return0;+returnref_transaction_update(transaction,refname,null_sha1,+old_sha1,flags,have_old,msg,err);}intupdate_ref(constchar*action,constchar*refname,
From: Michael Haggerty <hidden> Date: 2016-06-15 23:03:12
We don't actually need the locking functionality, because we already
hold the lock on the reference itself, which is how the reflog file is
locked. But the lock_file code still does some of the bookkeeping for
us and is more careful than the old code here was.
For example:
* Correctly handle the case that the reflog lock file already exists
for some reason or cannot be opened.
* Correctly clean up the lockfile if the program dies.
Signed-off-by: Michael Haggerty <redacted>
---
builtin/reflog.c | 37 ++++++++++++++++++++++---------------
1 file changed, 22 insertions(+), 15 deletions(-)
From: Michael Haggerty <hidden> Date: 2016-06-15 23:03:12
We want to separate the options relevant to the expiry machinery from
the options affecting the expiration policy. So add a "flags" argument
to expire_reflog() to hold the former.
The argument doesn't yet do anything.
Signed-off-by: Michael Haggerty <redacted>
---
builtin/reflog.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
From: Michael Haggerty <hidden> Date: 2016-06-15 23:03:12
The policy objects don't care about "--updateref". So move it to
expire_reflog()'s flags parameter.
Signed-off-by: Michael Haggerty <redacted>
---
builtin/reflog.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
From: Michael Haggerty <hidden> Date: 2016-06-15 23:03:12
Extract two functions, reflog_expiry_prepare() and
reflog_expiry_cleanup(), from expire_reflog(). This is a further step
towards separating the code for deciding on expiration policy from the
code that manages the physical expiration.
This change requires a couple of local variables from expire_reflog()
to be turned into fields of "struct expire_reflog_cb". More
reorganization of the callback data will follow in later commits.
Signed-off-by: Michael Haggerty <redacted>
---
In fact, the work done in reflog_expire_cleanup() doesn't really need
to be done via a callback, because it doesn't need to be done while
the reference lock is held. But the symmetry between prepare and
cleanup is kindof nice. Perhaps some future policy decision will want
to do some final work under the reference lock?
But it would be easy to get rid of this third callback function and
have the callers do the work themselves after calling expire_reflog().
I don't have a string feeling either way.
builtin/reflog.c | 94 +++++++++++++++++++++++++++++++-------------------------
1 file changed, 52 insertions(+), 42 deletions(-)
From: Michael Haggerty <hidden> Date: 2016-06-15 23:03:12
The policy objects don't care about "--rewrite". So move it to
expire_reflog()'s flags parameter.
Signed-off-by: Michael Haggerty <redacted>
---
builtin/reflog.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
From: Michael Haggerty <hidden> Date: 2016-06-15 23:03:12
This is the first step towards separating the data needed by the
policy code from the data needed by the reflog expiration machinery.
Signed-off-by: Michael Haggerty <redacted>
---
builtin/reflog.c | 19 ++++++++++---------
1 file changed, 10 insertions(+), 9 deletions(-)
From: Michael Haggerty <hidden> Date: 2016-06-15 23:03:12
Now that expire_reflog() doesn't actually look in the
expire_reflog_policy_cb data structure, we can make it opaque:
* Change its callers to pass it a pointer to an entire "struct
expire_reflog_policy_cb".
* Change it to pass the pointer through as a "void *".
* Change the policy functions, reflog_expiry_prepare(),
reflog_expiry_cleanup(), and should_expire_reflog_ent(), to accept
"void *cb_data" arguments and cast them to "struct
expire_reflog_policy_cb" internally.
Signed-off-by: Michael Haggerty <redacted>
---
builtin/reflog.c | 73 ++++++++++++++++++++++++++++----------------------------
1 file changed, 36 insertions(+), 37 deletions(-)
@@ -653,25 +652,25 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)do_all=status=0;memset(&cb,0,sizeof(cb));-cb.expire_total=default_reflog_expire;-cb.expire_unreachable=default_reflog_expire_unreachable;+cb.cmd.expire_total=default_reflog_expire;+cb.cmd.expire_unreachable=default_reflog_expire_unreachable;for(i=1;i<argc;i++){constchar*arg=argv[i];if(!strcmp(arg,"--dry-run")||!strcmp(arg,"-n"))flags|=EXPIRE_REFLOGS_DRY_RUN;elseif(starts_with(arg,"--expire=")){-if(parse_expiry_date(arg+9,&cb.expire_total))+if(parse_expiry_date(arg+9,&cb.cmd.expire_total))die(_("'%s' is not a valid timestamp"),arg);explicit_expiry|=EXPIRE_TOTAL;}elseif(starts_with(arg,"--expire-unreachable=")){-if(parse_expiry_date(arg+21,&cb.expire_unreachable))+if(parse_expiry_date(arg+21,&cb.cmd.expire_unreachable))die(_("'%s' is not a valid timestamp"),arg);explicit_expiry|=EXPIRE_UNREACH;}elseif(!strcmp(arg,"--stale-fix"))-cb.stalefix=1;+cb.cmd.stalefix=1;elseif(!strcmp(arg,"--rewrite"))flags|=EXPIRE_REFLOGS_REWRITE;elseif(!strcmp(arg,"--updateref"))
From: Michael Haggerty <hidden> Date: 2016-06-15 23:03:12
Move expire_reflog() into refs.c and rename it to reflog_expire().
Turn the three policy functions into function pointers that are passed
into reflog_expire(). Add function prototypes and documentation to
refs.h.
Signed-off-by: Michael Haggerty <redacted>
---
builtin/reflog.c | 133 +++++++------------------------------------------------
refs.c | 114 +++++++++++++++++++++++++++++++++++++++++++++++
refs.h | 45 +++++++++++++++++++
3 files changed, 174 insertions(+), 118 deletions(-)
From: Michael Haggerty <hidden> Date: 2016-06-15 23:03:12
From: Ronnie Sahlberg <redacted>
Inline the function at its one remaining caller (which is within
refs.c) and remove it.
Signed-off-by: Michael Haggerty <redacted>
---
refs.c | 9 +--------
refs.h | 9 +--------
2 files changed, 2 insertions(+), 16 deletions(-)
From: Michael Haggerty <hidden> Date: 2016-06-15 23:03:12
From: Stefan Beller <redacted>
Now the struct ref_lock is used completely internally, so let's
remove it from the header file.
Signed-off-by: Stefan Beller <redacted>
Signed-off-by: Michael Haggerty <redacted>
---
refs.c | 9 +++++++++
refs.h | 9 ---------
2 files changed, 9 insertions(+), 9 deletions(-)
From: Michael Haggerty <hidden> Date: 2016-06-15 23:03:12
Add a new data type, "struct expire_reflog_cb", for holding the data
that expire_reflog() passes to expire_reflog_ent() via
for_each_reflog_ent(). For now it only holds a pointer to "struct
expire_reflog_policy_cb". In future commits we will move some data
from the latter to the former.
Signed-off-by: Michael Haggerty <redacted>
---
builtin/reflog.c | 43 ++++++++++++++++++++++++++-----------------
1 file changed, 26 insertions(+), 17 deletions(-)
From: Michael Haggerty <hidden> Date: 2016-06-15 23:03:12
The policy objects don't care about "--verbose". So move it to
expire_reflog()'s flags parameter.
Signed-off-by: Michael Haggerty <redacted>
---
builtin/reflog.c | 24 ++++++++++++------------
1 file changed, 12 insertions(+), 12 deletions(-)
From: Michael Haggerty <hidden> Date: 2016-06-15 23:03:12
These members are not needed by the policy functions.
Signed-off-by: Michael Haggerty <redacted>
---
builtin/reflog.c | 20 ++++++++++----------
1 file changed, 10 insertions(+), 10 deletions(-)
From: Michael Haggerty <hidden> Date: 2016-06-15 23:03:12
Add a flags field to "struct expire_reflog_cb", and pass the flags
argument through to expire_reflog_ent(). In a moment we will start
using it to pass through flags that expire_reflog_ent() needs.
Signed-off-by: Michael Haggerty <redacted>
---
builtin/reflog.c | 2 ++
1 file changed, 2 insertions(+)
From: Michael Haggerty <hidden> Date: 2016-06-15 23:03:12
There is very little cleanup needed if the reference has no reflog. If
we move the initialization of log_file down a bit, there's even less.
So instead of jumping to the cleanup code at the end of the function,
just do the cleanup and return inline.
Signed-off-by: Michael Haggerty <redacted>
---
builtin/reflog.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
From: Michael Haggerty <hidden> Date: 2016-06-15 23:03:12
Extracted from expire_reflog_ent() a function that is solely
responsible for deciding whether a reflog entry should be expired. By
separating this "business logic" from the mechanics of actually
expiring entries, we are working towards the goal of encapsulating
reflog expiry within the refs API, with policy decided by a callback
function passed to it by its caller.
Signed-off-by: Michael Haggerty <redacted>
---
builtin/reflog.c | 70 +++++++++++++++++++++++++++++++++-----------------------
1 file changed, 42 insertions(+), 28 deletions(-)
From: Michael Haggerty <hidden> Date: 2016-06-15 23:03:12
From: Ronnie Sahlberg <redacted>
Break out the code to create the string and writing it to the file
descriptor from log_ref_write and add it into a dedicated function
log_ref_write_fd. For now this is only used from log_ref_write,
but later on we will call this function from reflog transactions too,
which means that we will end up with only a single place,
where we write a reflog entry to a file instead of the current two
places (log_ref_write and builtin/reflog.c).
Signed-off-by: Ronnie Sahlberg <redacted>
Signed-off-by: Stefan Beller <redacted>
Reviewed-by: Jonathan Nieder <redacted>
Signed-off-by: Michael Haggerty <redacted>
---
refs.c | 48 ++++++++++++++++++++++++++++++------------------
1 file changed, 30 insertions(+), 18 deletions(-)
From: Michael Haggerty <hidden> Date: 2016-06-15 23:03:12
The policy objects don't care about "--dry-run". So move it to
expire_reflog()'s flags parameter.
Signed-off-by: Michael Haggerty <redacted>
---
builtin/reflog.c | 13 ++++++++-----
1 file changed, 8 insertions(+), 5 deletions(-)
@@ -644,7 +647,7 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)for(i=1;i<argc;i++){constchar*arg=argv[i];if(!strcmp(arg,"--dry-run")||!strcmp(arg,"-n"))-cb.dry_run=1;+flags|=EXPIRE_REFLOGS_DRY_RUN;elseif(starts_with(arg,"--expire=")){if(parse_expiry_date(arg+9,&cb.expire_total))die(_("'%s' is not a valid timestamp"),arg);
From: Michael Haggerty <hidden> Date: 2016-06-15 23:03:12
From: Ronnie Sahlberg <redacted>
unlock|close|commit_ref can be made static since there are no more external
callers.
Signed-off-by: Ronnie Sahlberg <redacted>
Signed-off-by: Stefan Beller <redacted>
Signed-off-by: Michael Haggerty <redacted>
---
refs.c | 24 ++++++++++++------------
refs.h | 9 ---------
2 files changed, 12 insertions(+), 21 deletions(-)
@@ -2090,6 +2090,16 @@ int refname_match(const char *abbrev_name, const char *full_name)return0;}+staticvoidunlock_ref(structref_lock*lock)+{+/* Do not free lock->lk -- atexit() still looks at them */+if(lock->lk)+rollback_lock_file(lock->lk);+free(lock->ref_name);+free(lock->orig_ref_name);+free(lock);+}+/* This function should make sure errno is meaningful on error */staticstructref_lock*verify_lock(structref_lock*lock,constunsignedchar*old_sha1,intmustexist)
@@ -2896,7 +2906,7 @@ int close_ref(struct ref_lock *lock)return0;}-intcommit_ref(structref_lock*lock)+staticintcommit_ref(structref_lock*lock){if(commit_lock_file(lock->lk))return-1;
@@ -2904,16 +2914,6 @@ int commit_ref(struct ref_lock *lock)return0;}-voidunlock_ref(structref_lock*lock)-{-/* Do not free lock->lk -- atexit() still looks at them */-if(lock->lk)-rollback_lock_file(lock->lk);-free(lock->ref_name);-free(lock->orig_ref_name);-free(lock);-}-/**copythereflogmessagemsgtobuf,whichhasbeenallocatedsufficiently*large,whilecleaningupthewhitespaces.Especially,convertLFtospace,
@@ -198,15 +198,6 @@ extern struct ref_lock *lock_any_ref_for_update(const char *refname,constunsignedchar*old_sha1,intflags,int*type_p);-/** Close the file descriptor owned by a lock and return the status */-externintclose_ref(structref_lock*lock);--/** Close and commit the ref locked by the lock */-externintcommit_ref(structref_lock*lock);--/** Release any lock taken but not written. **/-externvoidunlock_ref(structref_lock*lock);-/**Setupreflogbeforeusing.Seterrnotosomethingmeaningfulonfailure.*/
From: Jonathan Nieder <hidden> Date: 2016-06-15 23:03:12
Michael Haggerty wrote:
It was called "unused", so at least it was self-consistent.
The missing context is that this was a callback function that had to
match the each_ref_fn signature (where that parameter is 'flags')
until v1.5.4~14 (reflog-expire: avoid creating new files in a
directory inside readdir(3) loop, 2008-01-25). v1.5.4~14 forgot to
clean up.
With or without a note in the commit message explaining that,
Reviewed-by: Jonathan Nieder <redacted>
On second thought: why not update the last parameter to be a 'struct
cmd_reflog_expire_cb *' instead of 'void *' while at it, like this?
builtin/reflog.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
From: Jonathan Nieder <hidden> Date: 2016-06-15 23:03:12
Michael Haggerty wrote:
[Subject: expire_reflog(): exit early if the reference has no reflog]
The caller moves on to expire other reflogs, so it's not exiting.
"return early", maybe?
Except the function already returned early. I think the purpose of
this patch is to simplify the no-reflog case by handling it separately.
Anyway, that's just nitpicking about the subject line. With
s/exit/return/ it should be clear that this is a refactoring change,
which for someone looking at the shortlog is the important thing.
Thanks,
Jonathan
From: Jonathan Nieder <hidden> Date: 2016-06-15 23:03:12
Michael Haggerty wrote:
We don't actually need the locking functionality, because we already
hold the lock on the reference itself, which is how the reflog file is
locked. But the lock_file code still does some of the bookkeeping for
us and is more careful than the old code here was.
As you say, the ref lock takes care of mutual exclusion, so we do not
have to be too careful about compatibility with other tools that might
not know to lock the reflog. And this is not tying our hands for a
future when I might want to lock logs/refs/heads/topic/1 while
logs/refs/heads/topic still exists as part of the implementation of
"git mv topic/1 topic".
Stefan and I had forgotten about that guarantee when looking at that
kind of operation --- thanks for the reminder.
Should updates to the HEAD reflog acquire HEAD.lock? (They don't
currently.)
[...]
If this lockfile is only used in that one function, it can be declared
inside the function.
If it is meant to be used throughout the 'git reflog' command, then it
can go near the top of the file.
hold_lock_file_for_update doesn't print a message. Code to print one
looks like
if (hold_lock_file_for_update(&reflog_lock, log_file, 0) < 0) {
unable_to_lock_message(log_file, errno, &err);
error("%s", err.buf);
goto failure;
}
(A patch in flight changes that to
if (hold_lock_file_for_update(&reflog_lock, log_file, 0, &err) < 0) {
error("%s", err.buf);
goto failure;
}
)
Hm. lockfile.c::fdopen_lock_file ought to use xfdopen to make this
case impossible. And xfdopen should use try_to_free_routine() and
try again on failure.
[...]
quoted hunk
@@ -423,10 +429,9 @@ static int expire_reflog(const char *refname, const unsigned char *sha1, void *c } if (cb.newlog) {- if (fclose(cb.newlog)) {- status |= error("%s: %s", strerror(errno),- newlog_path);- unlink(newlog_path);+ if (close_lock_file(&reflog_lock)) {+ status |= error("Couldn't write %s: %s", log_file,+ strerror(errno));
Style nit: error messages usually start with a lowercase letter
(though I realize nearby examples are already inconsistent).
commit_lock_file() can take care of the close_lock_file automatically.
[...]
quoted hunk
@@ -434,21 +439,23 @@ static int expire_reflog(const char *refname, const unsigned char *sha1, void *c close_ref(lock) < 0)) { status |= error("Couldn't write %s", lock->lk->filename.buf);- unlink(newlog_path);- } else if (rename(newlog_path, log_file)) {- status |= error("cannot rename %s to %s",- newlog_path, log_file);- unlink(newlog_path);+ rollback_lock_file(&reflog_lock);+ } else if (commit_lock_file(&reflog_lock)) {+ status |= error("cannot rename %s.lock to %s",+ log_file, log_file);
Most callers say "unable to commit reflog '%s'", log_file to hedge their
bets in case the close failed (which may be what you were avoiding
above.
errno is meaningful when commit_lock_file fails, making a more
detailed diagnosis from strerror(errno) possible.
Thanks,
Jonathan
From: Stefan Beller <hidden> Date: 2016-06-15 23:03:12
On Thu, Dec 04, 2014 at 04:23:31PM -0800, Jonathan Nieder wrote:
Michael Haggerty wrote:
quoted
We don't actually need the locking functionality, because we already
hold the lock on the reference itself, which is how the reflog file is
locked. But the lock_file code still does some of the bookkeeping for
us and is more careful than the old code here was.
As you say, the ref lock takes care of mutual exclusion, so we do not
have to be too careful about compatibility with other tools that might
not know to lock the reflog. And this is not tying our hands for a
future when I might want to lock logs/refs/heads/topic/1 while
logs/refs/heads/topic still exists as part of the implementation of
"git mv topic/1 topic".
Stefan and I had forgotten about that guarantee when looking at that
kind of operation --- thanks for the reminder.
I did not forget about it, I did not know about that in the first hand.
We don't seem to have documentation on it?
So sorry for heading in a direction, which would have been avoidable.
Thanks,
Stefan
From: ronnie sahlberg <ronniesahlberg@gmail.com> Date: 2016-06-15 23:03:12
On Thu, Dec 4, 2014 at 3:08 PM, Michael Haggerty [off-list ref] wrote:
We don't actually need the locking functionality, because we already
hold the lock on the reference itself,
No. You do need the lock.
The ref is locked only during transaction_commit()
If you don't want to lock the reflog file and instead rely on the lock
on the ref itself you will need to
rework your patches so that the lock on the ref is taken already
during, for example, transaction_update_ref() instead.
But without doing those changes and moving the ref locking from
_commit() to _update_ref() you will risk reflog corruption/surprises
if two operations collide and both rewrite the reflog without any lock held.
quoted hunk
which is how the reflog file is
locked. But the lock_file code still does some of the bookkeeping for
us and is more careful than the old code here was.
For example:
* Correctly handle the case that the reflog lock file already exists
for some reason or cannot be opened.
* Correctly clean up the lockfile if the program dies.
Signed-off-by: Michael Haggerty <redacted>
---
builtin/reflog.c | 37 ++++++++++++++++++++++---------------
1 file changed, 22 insertions(+), 15 deletions(-)
From: Michael Haggerty <hidden> Date: 2016-06-15 23:03:12
On 12/05/2014 12:28 AM, Jonathan Nieder wrote:> Michael Haggerty wrote:
quoted
It was called "unused", so at least it was self-consistent.
The missing context is that this was a callback function that had to
match the each_ref_fn signature [...]
With or without a note in the commit message explaining that,
Reviewed-by: Jonathan Nieder <redacted>
[...]
quoted
--- a/builtin/reflog.c+++ b/builtin/reflog.c
@@ -349,7 +349,7 @@ static int push_tip_to_list(const char *refname,
+static int expire_reflog(const char *ref, const unsigned char *sha1,
void *cb_data)
quoted
{
struct cmd_reflog_expire_cb *cmd = cb_data;
On second thought: why not update the last parameter to be a 'struct
cmd_reflog_expire_cb *' instead of 'void *' while at it, like this?
[...]
Thanks for the explanation, the review, and the suggestion. I will
expand the commit to be "don't implement each_ref_fn anymore" and
incorporate all of your suggestions.
Michael
--
Michael Haggerty
mhagger@alum.mit.edu
From: Michael Haggerty <hidden> Date: 2016-06-15 23:03:12
On 12/05/2014 12:53 AM, Jonathan Nieder wrote:
Michael Haggerty wrote:
quoted
[Subject: expire_reflog(): exit early if the reference has no reflog]
The caller moves on to expire other reflogs, so it's not exiting.
"return early", maybe?
Except the function already returned early. I think the purpose of
this patch is to simplify the no-reflog case by handling it separately.
Anyway, that's just nitpicking about the subject line. With
s/exit/return/ it should be clear that this is a refactoring change,
which for someone looking at the shortlog is the important thing.
Good suggestion. I will change the commit message.
Michael
--
Michael Haggerty
mhagger@alum.mit.edu
From: Stefan Beller <hidden> Date: 2016-06-15 23:03:12
On Fri, Dec 05, 2014 at 12:08:21AM +0100, Michael Haggerty wrote:
Extract two functions, reflog_expiry_prepare() and
reflog_expiry_cleanup(), from expire_reflog(). This is a further step
towards separating the code for deciding on expiration policy from the
code that manages the physical expiration.
This change requires a couple of local variables from expire_reflog()
to be turned into fields of "struct expire_reflog_cb". More
reorganization of the callback data will follow in later commits.
Signed-off-by: Michael Haggerty <redacted>
Reviewed-by: Stefan Beller <redacted>
quoted hunk
---
In fact, the work done in reflog_expire_cleanup() doesn't really need
to be done via a callback, because it doesn't need to be done while
the reference lock is held. But the symmetry between prepare and
cleanup is kindof nice. Perhaps some future policy decision will want
to do some final work under the reference lock?
But it would be easy to get rid of this third callback function and
have the callers do the work themselves after calling expire_reflog().
I don't have a string feeling either way.
builtin/reflog.c | 94 +++++++++++++++++++++++++++++++-------------------------
1 file changed, 52 insertions(+), 42 deletions(-)
From: Stefan Beller <hidden> Date: 2016-06-15 23:03:12
On Thu, Dec 04, 2014 at 04:23:31PM -0800, Jonathan Nieder wrote:
Michael Haggerty wrote:
quoted
We don't actually need the locking functionality, because we already
hold the lock on the reference itself, which is how the reflog file is
locked. But the lock_file code still does some of the bookkeeping for
us and is more careful than the old code here was.
As you say, the ref lock takes care of mutual exclusion, so we do not
have to be too careful about compatibility with other tools that might
not know to lock the reflog. And this is not tying our hands for a
future when I might want to lock logs/refs/heads/topic/1 while
logs/refs/heads/topic still exists as part of the implementation of
"git mv topic/1 topic".
Stefan and I had forgotten about that guarantee when looking at that
kind of operation --- thanks for the reminder.
Should updates to the HEAD reflog acquire HEAD.lock? (They don't
currently.)
[...]
If this lockfile is only used in that one function, it can be declared
inside the function.
If it is meant to be used throughout the 'git reflog' command, then it
can go near the top of the file.
After the series completes, this lock is only used in reflog_expire.
So I'd rather move it inside the function? Then we could run the reflog_expire
function in parallel for different locks in theory?
hold_lock_file_for_update doesn't print a message. Code to print one
looks like
if (hold_lock_file_for_update(&reflog_lock, log_file, 0) < 0) {
unable_to_lock_message(log_file, errno, &err);
error("%s", err.buf);
goto failure;
}
(A patch in flight changes that to
if (hold_lock_file_for_update(&reflog_lock, log_file, 0, &err) < 0) {
error("%s", err.buf);
goto failure;
}
)
Hm. lockfile.c::fdopen_lock_file ought to use xfdopen to make this
case impossible. And xfdopen should use try_to_free_routine() and
try again on failure.
[...]
quoted
@@ -423,10 +429,9 @@ static int expire_reflog(const char *refname, const unsigned char *sha1, void *c } if (cb.newlog) {- if (fclose(cb.newlog)) {- status |= error("%s: %s", strerror(errno),- newlog_path);- unlink(newlog_path);+ if (close_lock_file(&reflog_lock)) {+ status |= error("Couldn't write %s: %s", log_file,+ strerror(errno));
Style nit: error messages usually start with a lowercase letter
(though I realize nearby examples are already inconsistent).
commit_lock_file() can take care of the close_lock_file automatically.
[...]
quoted
@@ -434,21 +439,23 @@ static int expire_reflog(const char *refname, const unsigned char *sha1, void *c close_ref(lock) < 0)) { status |= error("Couldn't write %s", lock->lk->filename.buf);- unlink(newlog_path);- } else if (rename(newlog_path, log_file)) {- status |= error("cannot rename %s to %s",- newlog_path, log_file);- unlink(newlog_path);+ rollback_lock_file(&reflog_lock);+ } else if (commit_lock_file(&reflog_lock)) {+ status |= error("cannot rename %s.lock to %s",+ log_file, log_file);
Most callers say "unable to commit reflog '%s'", log_file to hedge their
bets in case the close failed (which may be what you were avoiding
above.
errno is meaningful when commit_lock_file fails, making a more
detailed diagnosis from strerror(errno) possible.
Thanks,
Jonathan
From: Michael Haggerty <hidden> Date: 2016-06-15 23:03:14
On 12/05/2014 03:19 AM, Stefan Beller wrote:
On Thu, Dec 04, 2014 at 04:23:31PM -0800, Jonathan Nieder wrote:
quoted
Michael Haggerty wrote:
quoted
We don't actually need the locking functionality, because we already
hold the lock on the reference itself, which is how the reflog file is
locked. But the lock_file code still does some of the bookkeeping for
us and is more careful than the old code here was.
As you say, the ref lock takes care of mutual exclusion, so we do not
have to be too careful about compatibility with other tools that might
not know to lock the reflog. And this is not tying our hands for a
future when I might want to lock logs/refs/heads/topic/1 while
logs/refs/heads/topic still exists as part of the implementation of
"git mv topic/1 topic".
Stefan and I had forgotten about that guarantee when looking at that
kind of operation --- thanks for the reminder.
I did not forget about it, I did not know about that in the first hand.
We don't seem to have documentation on it?
So sorry for heading in a direction, which would have been avoidable.
This isn't documented very well. I thought I saw a comment somewhere in
the code that stated it explicitly, but I can't find it now. In any
case, my understanding of the locking protocol for reflogs is:
The reflog for "$refname", which is stored at
"$GIT_DIR/logs/$refname", is locked by holding
"$GIT_DIR/refs/$refname.lock", *even if the corresponding
reference is packed*.
This implies that readers, who don't pay attention to locks, have to be
prepared for the possibility that the reflog is in the middle of an
update and that the last line is incomplete. This is handled by
show_one_reflog_ent(), which discards incomplete lines.
This protocol avoids the need to rewrite the reflog from scratch for
each reference update.
Given how poorly-documented this point is, I wonder whether other
implementations of Git (e.g., libgit2, JGit, Dulwich, ...) got it right.
Michael
--
Michael Haggerty
mhagger@alum.mit.edu
From: Michael Haggerty <hidden> Date: 2016-06-15 23:03:14
On 12/05/2014 03:59 AM, ronnie sahlberg wrote:
On Thu, Dec 4, 2014 at 3:08 PM, Michael Haggerty [off-list ref] wrote:
quoted
We don't actually need the locking functionality, because we already
hold the lock on the reference itself,
No. You do need the lock.
The ref is locked only during transaction_commit()
If you don't want to lock the reflog file and instead rely on the lock
on the ref itself you will need to
rework your patches so that the lock on the ref is taken already
during, for example, transaction_update_ref() instead.
But without doing those changes and moving the ref locking from
_commit() to _update_ref() you will risk reflog corruption/surprises
if two operations collide and both rewrite the reflog without any lock held.
Ronnie, I don't understand your comments.
It is a statement of fact (to the best of my knowledge) that reflogs are
supposed to be modified only under a lock on the corresponding
reference, namely "$GIT_DIR/refs/$refname.lock". We do not require
reflog writers to hold "$GIT_DIR/logs/$refname.lock".
In this function, "$GIT_DIR/logs/$refname.lock" happens to be the name
of the temporary file being used to stage the new contents of the
reflog. But that is more or less a coincidence; we could call the
temporary file whatever we want because it has no locking implications.
However, what we want to do with the file in this code path (write a new
version then rename the new version on top of the old version, deleting
the temporary file if the program is interrupted) is the same as what we
do with lockfiles, so we use the lockfile code because it is convenient.
This patch series has nothing to do with ref_transaction_commit() or any
of the transaction machinery. It has to do with expire_reflog(), which
is invoked outside of any transaction.
Michael
--
Michael Haggerty
mhagger@alum.mit.edu
From: Michael Haggerty <hidden> Date: 2016-06-15 23:03:14
On 12/05/2014 01:23 AM, Jonathan Nieder wrote:
Michael Haggerty wrote:
quoted
We don't actually need the locking functionality, because we already
hold the lock on the reference itself, which is how the reflog file is
locked. But the lock_file code still does some of the bookkeeping for
us and is more careful than the old code here was.
As you say, the ref lock takes care of mutual exclusion, so we do not
have to be too careful about compatibility with other tools that might
not know to lock the reflog. And this is not tying our hands for a
future when I might want to lock logs/refs/heads/topic/1 while
logs/refs/heads/topic still exists as part of the implementation of
"git mv topic/1 topic".
Stefan and I had forgotten about that guarantee when looking at that
kind of operation --- thanks for the reminder.
This reminder is important (and forgettable) enough that I will add a
comment within the function explaining it.
Should updates to the HEAD reflog acquire HEAD.lock? (They don't
currently.)
Yes, they should; good catch. I assume that you are referring to the
code at the bottom of write_ref_sha1()? Or did you find a problem in
this patch series?
If the former, then I propose that we address this bug in a separate
patch series.
If this lockfile is only used in that one function, it can be declared
inside the function.
If it is meant to be used throughout the 'git reflog' command, then it
can go near the top of the file.
For now it is only used within this function, so I will move it into the
function as you suggest. (As you know, it does need to remain static,
because of the way the lock_file module takes over ownership of these
objects.)
Hm. lockfile.c::fdopen_lock_file ought to use xfdopen to make this
case impossible. And xfdopen should use try_to_free_routine() and
try again on failure.
That sounds reasonable, but it is not manifestly obvious given that at
least one caller of fdopen_lock_file() (in fast-import.c) tries to
recover if fdopen_lock_file() fails. Let's address this in a separate
patch series if that is OK with you. For now I will add explicit
error-reporting code here before "goto failure".
[...]
quoted
@@ -423,10 +429,9 @@ static int expire_reflog(const char *refname, const unsigned char *sha1, void *c } if (cb.newlog) {- if (fclose(cb.newlog)) {- status |= error("%s: %s", strerror(errno),- newlog_path);- unlink(newlog_path);+ if (close_lock_file(&reflog_lock)) {+ status |= error("Couldn't write %s: %s", log_file,+ strerror(errno));
Style nit: error messages usually start with a lowercase letter
(though I realize nearby examples are already inconsistent).
Thanks; will fix.
commit_lock_file() can take care of the close_lock_file automatically.
The existing code is a tiny bit safer: first make sure both files can be
written, *then* rename each of them into place. If either write fails,
then both files will get rolled back. But if we switch to using
commit_lock_file(), then a failure when writing the reference would
leave the reflog updated but the reference rolled back.
[...]
quoted
@@ -434,21 +439,23 @@ static int expire_reflog(const char *refname, const unsigned char *sha1, void *c close_ref(lock) < 0)) { status |= error("Couldn't write %s", lock->lk->filename.buf);- unlink(newlog_path);- } else if (rename(newlog_path, log_file)) {- status |= error("cannot rename %s to %s",- newlog_path, log_file);- unlink(newlog_path);+ rollback_lock_file(&reflog_lock);+ } else if (commit_lock_file(&reflog_lock)) {+ status |= error("cannot rename %s.lock to %s",+ log_file, log_file);
Most callers say "unable to commit reflog '%s'", log_file to hedge their
bets in case the close failed (which may be what you were avoiding
above.
errno is meaningful when commit_lock_file fails, making a more
detailed diagnosis from strerror(errno) possible.
I will improve the error message.
Thanks for your detailed review!
Michael
--
Michael Haggerty
mhagger@alum.mit.edu
From: Stefan Beller <hidden> Date: 2016-06-15 23:03:14
On Fri, Dec 05, 2014 at 12:08:20AM +0100, Michael Haggerty wrote:
Extracted from expire_reflog_ent() a function that is solely
responsible for deciding whether a reflog entry should be expired. By
separating this "business logic" from the mechanics of actually
expiring entries, we are working towards the goal of encapsulating
reflog expiry within the refs API, with policy decided by a callback
function passed to it by its caller.
Signed-off-by: Michael Haggerty <redacted>
Reviewed-by: Stefan Beller <redacted>
The comments below are just thoughts, which don't need to be
included into this commit.
+ if (should_expire_reflog_ent(osha1, nsha1, email, timestamp, tz,
+ message, cb_data)) {
+ if (!cb->newlog)
+ printf("would prune %s", message);
+ else if (cb->cmd->verbose)
+ printf("prune %s", message);
While this commit is just shoveling code around, we don't want to introduce
changes here. So a question for a possible later follow up:
"git reflog" is listed as an ancillary manipulator, which still is porcelain.
So we maybe want to translate "[would] prune"?
This is fine for just moving code around and reviewing.
I send a patch on top of this one to remove the manual calculation of the
sign and zone and let the fprintf function figure it out.
From: Stefan Beller <hidden> Date: 2016-06-15 23:03:14
On Fri, Dec 05, 2014 at 12:08:22AM +0100, Michael Haggerty wrote:
We want to separate the options relevant to the expiry machinery from
the options affecting the expiration policy. So add a "flags" argument
to expire_reflog() to hold the former.
The argument doesn't yet do anything.
Signed-off-by: Michael Haggerty <redacted>
@@ -644,7 +647,7 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)for(i=1;i<argc;i++){constchar*arg=argv[i];if(!strcmp(arg,"--dry-run")||!strcmp(arg,"-n"))-cb.dry_run=1;+flags|=EXPIRE_REFLOGS_DRY_RUN;elseif(starts_with(arg,"--expire=")){if(parse_expiry_date(arg+9,&cb.expire_total))die(_("'%s' is not a valid timestamp"),arg);
From: Stefan Beller <hidden> Date: 2016-06-15 23:03:14
On Fri, Dec 05, 2014 at 12:08:25AM +0100, Michael Haggerty wrote:
This is the first step towards separating the data needed by the
policy code from the data needed by the reflog expiration machinery.
Signed-off-by: Michael Haggerty <redacted>
From: Stefan Beller <hidden> Date: 2016-06-15 23:03:14
On Fri, Dec 05, 2014 at 12:08:26AM +0100, Michael Haggerty wrote:
Add a new data type, "struct expire_reflog_cb", for holding the data
that expire_reflog() passes to expire_reflog_ent() via
for_each_reflog_ent(). For now it only holds a pointer to "struct
expire_reflog_policy_cb". In future commits we will move some data
from the latter to the former.
Signed-off-by: Michael Haggerty <redacted>
From: Stefan Beller <hidden> Date: 2016-06-15 23:03:14
On Fri, Dec 05, 2014 at 12:08:27AM +0100, Michael Haggerty wrote:
Add a flags field to "struct expire_reflog_cb", and pass the flags
argument through to expire_reflog_ent(). In a moment we will start
using it to pass through flags that expire_reflog_ent() needs.
Signed-off-by: Michael Haggerty <redacted>
From: Stefan Beller <hidden> Date: 2016-06-15 23:03:14
On Fri, Dec 05, 2014 at 12:08:31AM +0100, Michael Haggerty wrote:
Now that expire_reflog() doesn't actually look in the
expire_reflog_policy_cb data structure, we can make it opaque:
* Change its callers to pass it a pointer to an entire "struct
expire_reflog_policy_cb".
* Change it to pass the pointer through as a "void *".
* Change the policy functions, reflog_expiry_prepare(),
reflog_expiry_cleanup(), and should_expire_reflog_ent(), to accept
"void *cb_data" arguments and cast them to "struct
expire_reflog_policy_cb" internally.
Signed-off-by: Michael Haggerty <redacted>
@@ -653,25 +652,25 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)do_all=status=0;memset(&cb,0,sizeof(cb));-cb.expire_total=default_reflog_expire;-cb.expire_unreachable=default_reflog_expire_unreachable;+cb.cmd.expire_total=default_reflog_expire;+cb.cmd.expire_unreachable=default_reflog_expire_unreachable;for(i=1;i<argc;i++){constchar*arg=argv[i];if(!strcmp(arg,"--dry-run")||!strcmp(arg,"-n"))flags|=EXPIRE_REFLOGS_DRY_RUN;elseif(starts_with(arg,"--expire=")){-if(parse_expiry_date(arg+9,&cb.expire_total))+if(parse_expiry_date(arg+9,&cb.cmd.expire_total))die(_("'%s' is not a valid timestamp"),arg);explicit_expiry|=EXPIRE_TOTAL;}elseif(starts_with(arg,"--expire-unreachable=")){-if(parse_expiry_date(arg+21,&cb.expire_unreachable))+if(parse_expiry_date(arg+21,&cb.cmd.expire_unreachable))die(_("'%s' is not a valid timestamp"),arg);explicit_expiry|=EXPIRE_UNREACH;}elseif(!strcmp(arg,"--stale-fix"))-cb.stalefix=1;+cb.cmd.stalefix=1;elseif(!strcmp(arg,"--rewrite"))flags|=EXPIRE_REFLOGS_REWRITE;elseif(!strcmp(arg,"--updateref"))
From: Stefan Beller <hidden> Date: 2016-06-15 23:03:14
On Fri, Dec 05, 2014 at 12:08:32AM +0100, Michael Haggerty wrote:
Move expire_reflog() into refs.c and rename it to reflog_expire().
Turn the three policy functions into function pointers that are passed
into reflog_expire(). Add function prototypes and documentation to
refs.h.
Signed-off-by: Michael Haggerty <redacted>
With or without the nits fixed
Reviewed-by: Stefan Beller <redacted>
as the nits are not degrading functionality.
+static int expire_reflog_ent(unsigned char *osha1, unsigned char *nsha1,
+ const char *email, unsigned long timestamp, int tz,
+ const char *message, void *cb_data)
Nit: According to our Codingguidelines we want to indent it further, so it aligns with
the arguments from the first line.
+static int expire_reflog_ent(unsigned char *osha1, unsigned char *nsha1,
+ const char *email, unsigned long timestamp, int tz,
+ const char *message, void *cb_data)
+ }
+ return 0;
Why do we need the return value for expire_reflog_ent?
The "return 0:" at the very end of the function is the only return I see here.
Sometimes we align the assigned numbers and sometimes we don't in git, so an alternative would be
enum expire_reflog_flags {
EXPIRE_REFLOGS_DRY_RUN = 1 << 0,
EXPIRE_REFLOGS_UPDATE_REF = 1 << 1,
EXPIRE_REFLOGS_VERBOSE = 1 << 2,
EXPIRE_REFLOGS_REWRITE = 1 << 3
}
Do we have a preference in the coding style on this one?
+ *
+ * reflog_expiry_select_fn -- Called once for each entry in the
+ * existing reflog. It should return true iff that entry should be
+ * pruned.
Also I know how we got here, I wonder if we should inverse the logic here
(in a later patch). "select" sounds to me as if the line is selected to keep it.
However the opposite is true. To actually select (keep) the line we need to return
0. Would it make sense to rename this to reflog_expiry_should_prune_fn ?
From: Michael Haggerty <hidden> Date: 2016-06-15 23:03:16
On 12/09/2014 12:34 AM, Stefan Beller wrote:
On Fri, Dec 05, 2014 at 12:08:34AM +0100, Michael Haggerty wrote:
quoted
From: Ronnie Sahlberg <redacted>
Inline the function at its one remaining caller (which is within
refs.c) and remove it.
quoted
Signed-off-by: Michael Haggerty <redacted>
It's originally from Ronnie, but his sign off is missing?
If that sign off is found again,
Reviewed-by: Stefan Beller <redacted>
Sorry for the confusion.
This patch ultimately descends from a patch by Ronnie that completely
deleted lock_any_ref_for_update() because (in the context that he wrote
it) there were no remaining callers of that function.
In the context of this patch, there *is* a remaining caller. So this
patch is different--it deletes the obvious stuff as in Ronnie's patch,
but it also inlines the function at its last caller. And the commit
message is completely different.
Given those differences and the fact that the only overlap with Ronnie's
original patch is the *deletion* of content, I thought it most
appropriate to change the authorship of the patch. So I removed Ronnie's
Signed-off-by line but I forgot to remove his authorship.
If anybody thinks it would be more appropriate to leave Ronnie as author
of the new patch or leave his Signed-off-by line, I am totally OK with
doing so. I'm just trying to figure out what's right.
Otherwise, I will change the authorship of the patch to myself in the
upcoming reroll and include only my own Signed-off-by line.
Michael
--
Michael Haggerty
mhagger@alum.mit.edu
From: Michael Haggerty <hidden> Date: 2016-06-15 23:03:16
On 12/09/2014 12:32 AM, Stefan Beller wrote:
On Fri, Dec 05, 2014 at 12:08:32AM +0100, Michael Haggerty wrote:
quoted
Move expire_reflog() into refs.c and rename it to reflog_expire().
Turn the three policy functions into function pointers that are passed
into reflog_expire(). Add function prototypes and documentation to
refs.h.
Signed-off-by: Michael Haggerty <redacted>
With or without the nits fixed
Reviewed-by: Stefan Beller <redacted>
as the nits are not degrading functionality.
+static int expire_reflog_ent(unsigned char *osha1, unsigned char *nsha1,
+ const char *email, unsigned long timestamp, int tz,
+ const char *message, void *cb_data)
Nit: According to our Codingguidelines we want to indent it further, so it aligns with
the arguments from the first line.
Will fix.
+static int expire_reflog_ent(unsigned char *osha1, unsigned char *nsha1,
+ const char *email, unsigned long timestamp, int tz,
+ const char *message, void *cb_data)
quoted
+ }
+ return 0;
Why do we need the return value for expire_reflog_ent?
The "return 0:" at the very end of the function is the only return I see here.
expire_reflog_ent() is passed to for_each_reflog_ent() and therefore
must be an each_reflog_ent_fn. If it returns a nonzero value, the
iteration is ended prematurely and the value is returned to the caller
of for_each_reflog_ent(). We don't ever want to end the iteration
prematurely here, so we always return 0.
Sometimes we align the assigned numbers and sometimes we don't in git, so an alternative would be
enum expire_reflog_flags {
EXPIRE_REFLOGS_DRY_RUN = 1 << 0,
EXPIRE_REFLOGS_UPDATE_REF = 1 << 1,
EXPIRE_REFLOGS_VERBOSE = 1 << 2,
EXPIRE_REFLOGS_REWRITE = 1 << 3
}
Do we have a preference in the coding style on this one?
Both styles are used in our codebase, and I don't think the style guide
says anything about it. My practice in such cases is:
* If I'm modifying existing code, preserve the existing style (to avoid
unnecessary churn)
* If most of our code uses one style, then use that style
* If our code uses both styles frequently, just use whatever style looks
better to me
If and when somebody cares enough to build a consensus for one policy or
the other and to submit a patch to the CodingGuidelines I will be happy
to follow it.
quoted
+ *
+ * reflog_expiry_select_fn -- Called once for each entry in the
+ * existing reflog. It should return true iff that entry should be
+ * pruned.
Also I know how we got here, I wonder if we should inverse the logic here
(in a later patch). "select" sounds to me as if the line is selected to keep it.
However the opposite is true. To actually select (keep) the line we need to return
0. Would it make sense to rename this to reflog_expiry_should_prune_fn ?
Yes, that would be clearer. I will make the change.
Michael
--
Michael Haggerty
mhagger@alum.mit.edu
Sometimes we align the assigned numbers and sometimes we don't in git, so an alternative would be
enum expire_reflog_flags {
EXPIRE_REFLOGS_DRY_RUN = 1 << 0,
EXPIRE_REFLOGS_UPDATE_REF = 1 << 1,
EXPIRE_REFLOGS_VERBOSE = 1 << 2,
EXPIRE_REFLOGS_REWRITE = 1 << 3
}
Do we have a preference in the coding style on this one?
I think vertically aligned lists look really nice. But they often wreak
havoc with diffs, because introducing one longer line means re-aligning
the whole thing. IMHO, it's not worth it (but if you're going to do it,
leave lots of extra room for expansion).
Just my two cents, of course. I don't recall this particular style point
coming up before.
Both styles are used in our codebase, and I don't think the style guide
says anything about it. My practice in such cases is:
* If I'm modifying existing code, preserve the existing style (to avoid
unnecessary churn)
* If most of our code uses one style, then use that style
* If our code uses both styles frequently, just use whatever style looks
better to me
I think that is a very good philosophy in general.
-Peff