Re: [PATCH 20/23] reflog_expire(): new function in the reference API

2014-12-12 Thread Michael Haggerty 
On 12/09/2014 12:32 AM, Stefan Beller wrote:
 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 mhag...@alum.mit.edu
 
 With or without the nits fixed
 Reviewed-by: Stefan Beller sbel...@google.com
 as the nits are not degrading functionality.
 
 ---
  builtin/reflog.c | 133 
 +++
  refs.c   | 114 +++
  refs.h   |  45 +++
  3 files changed, 174 insertions(+), 118 deletions(-)

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

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

 + *
 + * 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
mhag...@alum.mit.edu

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 20/23] reflog_expire(): new function in the reference API

2014-12-12 Thread Jeff King 
On Fri, Dec 12, 2014 at 09:23:05AM +0100, Michael Haggerty wrote:

 On 12/09/2014 12:32 AM, Stefan Beller wrote:
  +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
  +};
  
  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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 20/23] reflog_expire(): new function in the reference API

2014-12-12 Thread Junio C Hamano 
Jeff King p...@peff.net writes:

  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.

Thanks.

About the indentation on the second and subsequent lines of a
logical line that is split into multiple lines, we explicitly say
Both ar valid, and we use both.  Following the above three-bullet
list would be a good practice for this one, too.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 20/23] reflog_expire(): new function in the reference API

2014-12-08 Thread Stefan Beller 
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 mhag...@alum.mit.edu

With or without the nits fixed
Reviewed-by: Stefan Beller sbel...@google.com
as the nits are not degrading functionality.

 ---
  builtin/reflog.c | 133 
 +++
  refs.c   | 114 +++
  refs.h   |  45 +++
  3 files changed, 174 insertions(+), 118 deletions(-)
 



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

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

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 ?

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 20/23] reflog_expire(): new function in the reference API

2014-12-04 Thread Michael Haggerty 
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 mhag...@alum.mit.edu
---
 builtin/reflog.c | 133 +++
 refs.c   | 114 +++
 refs.h   |  45 +++
 3 files changed, 174 insertions(+), 118 deletions(-)

diff --git a/builtin/reflog.c b/builtin/reflog.c
index c30936bb..49c64f9 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -20,13 +20,6 @@ static const char reflog_delete_usage[] =
 static unsigned long default_reflog_expire;
 static unsigned long default_reflog_expire_unreachable;
 
-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
-};
-
 struct cmd_reflog_expire_cb {
struct rev_info revs;
int stalefix;
@@ -48,13 +41,6 @@ struct expire_reflog_policy_cb {
struct commit_list *tips;
 };
 
-struct expire_reflog_cb {
-   unsigned int flags;
-   void *policy_cb;
-   FILE *newlog;
-   unsigned char last_kept_sha1[20];
-};
-
 struct collected_reflog {
unsigned char sha1[20];
char reflog[FLEX_ARRAY];
@@ -330,38 +316,6 @@ static int should_expire_reflog_ent(unsigned char *osha1, 
unsigned char *nsha1,
return 0;
 }
 
-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)
-{
-   struct expire_reflog_cb *cb = cb_data;
-   struct expire_reflog_policy_cb *policy_cb = cb-policy_cb;
-
-   if (cb-flags  EXPIRE_REFLOGS_REWRITE)
-   osha1 = cb-last_kept_sha1;
-
-   if (should_expire_reflog_ent(osha1, nsha1, email, timestamp, tz,
-message, policy_cb)) {
-   if (!cb-newlog)
-   printf(would prune %s, message);
-   else if (cb-flags  EXPIRE_REFLOGS_VERBOSE)
-   printf(prune %s, message);
-   } else {
-   if (cb-newlog) {
-   char sign = (tz  0) ? '-' : '+';
-   int zone = (tz  0) ? (-tz) : tz;
-   fprintf(cb-newlog, %s %s %s %lu %c%04d\t%s,
-   sha1_to_hex(osha1), sha1_to_hex(nsha1),
-   email, timestamp, sign, zone,
-   message);
-   hashcpy(cb-last_kept_sha1, nsha1);
-   }
-   if (cb-flags  EXPIRE_REFLOGS_VERBOSE)
-   printf(keep %s, message);
-   }
-   return 0;
-}
-
 static int push_tip_to_list(const char *refname, const unsigned char *sha1,
int flags, void *cb_data)
 {
@@ -428,75 +382,6 @@ static void reflog_expiry_cleanup(void *cb_data)
}
 }
 
-static struct lock_file reflog_lock;
-
-static int expire_reflog(const char *refname, const unsigned char *sha1,
-unsigned int flags, void *policy_cb_data)
-{
-   struct expire_reflog_cb cb;
-   struct ref_lock *lock;
-   char *log_file;
-   int status = 0;
-
-   memset(cb, 0, sizeof(cb));
-   cb.flags = flags;
-   cb.policy_cb = policy_cb_data;
-
-   /*
-* we take the lock for the ref itself to prevent it from
-* getting updated.
-*/
-   lock = lock_any_ref_for_update(refname, sha1, 0, NULL);
-   if (!lock)
-   return error(cannot lock ref '%s', refname);
-   if (!reflog_exists(refname)) {
-   unlock_ref(lock);
-   return 0;
-   }
-
-   log_file = git_pathdup(logs/%s, refname);
-   if (!(flags  EXPIRE_REFLOGS_DRY_RUN)) {
-   if (hold_lock_file_for_update(reflog_lock, log_file, 0)  0)
-   goto failure;
-   cb.newlog = fdopen_lock_file(reflog_lock, w);
-   if (!cb.newlog)
-   goto failure;
-   }
-
-   reflog_expiry_prepare(refname, sha1, cb.policy_cb);
-   for_each_reflog_ent(refname, expire_reflog_ent, cb);
-   reflog_expiry_cleanup(cb.policy_cb);
-
-   if (!(flags  EXPIRE_REFLOGS_DRY_RUN)) {
-   if (close_lock_file(reflog_lock)) {
-   status |= error(Couldn't write %s: %s, log_file,
-   strerror(errno));
-   } else if ((flags  EXPIRE_REFLOGS_UPDATE_REF) 
-   (write_in_full(lock-lock_fd,
-   sha1_to_hex(cb.last_kept_sha1), 40) != 40 ||
-write_str_in_full(lock-lock_fd, \n) != 1 ||
-close_ref(lock)  0)) {
-   status |=