Re: [PATCH 4/6] repack_without_refs(): make the refnames argument a string_list

2014-11-24 Thread Michael Haggerty 
On 11/22/2014 10:17 PM, Jonathan Nieder wrote:
 Michael Haggerty wrote:
 
 All of the callers have string_lists available already
 
 Technically ref_transaction_commit doesn't, but that doesn't matter.

You're right. I'll correct this.

 Suggested-by: Ronnie Sahlberg sahlb...@google.com
 Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
 ---
  builtin/remote.c | 14 ++
  refs.c   | 38 --
  refs.h   | 11 ++-
  3 files changed, 32 insertions(+), 31 deletions(-)
 
 Reviewed-by: Jonathan Nieder jrnie...@gmail.com
 
 One (optional) nit at the bottom of this message.
 
 [...]
 +++ b/refs.c
 @@ -2639,22 +2639,25 @@ static int curate_packed_ref_fn(struct ref_entry 
 *entry, void *cb_data)
  return 0;
  }
  
 -int repack_without_refs(const char **refnames, int n, struct strbuf *err)
 +int repack_without_refs(struct string_list *refnames, struct strbuf *err)
  {
  struct ref_dir *packed;
  struct string_list refs_to_delete = STRING_LIST_INIT_DUP;
 -struct string_list_item *ref_to_delete;
 -int i, ret, removed = 0;
 +struct string_list_item *refname, *ref_to_delete;
 +int ret, needs_repacking = 0, removed = 0;
  
  assert(err);
  
  /* Look for a packed ref */
 -for (i = 0; i  n; i++)
 -if (get_packed_ref(refnames[i]))
 +for_each_string_list_item(refname, refnames) {
 +if (get_packed_ref(refname-string)) {
 +needs_repacking = 1;
  break;
 +}
 +}
  
  /* Avoid locking if we have nothing to do */
 -if (i == n)
 +if (!needs_repacking)
 
 This makes me wish C supported something like Python's for/else
 construct.  Oh well. :)

Ahhh, Python, where arrays of strings *are* string_lists :-)

 [...]
 +++ b/refs.h
 @@ -163,7 +163,16 @@ extern void rollback_packed_refs(void);
   */
  int pack_refs(unsigned int flags);
  
 -extern int repack_without_refs(const char **refnames, int n,
 +/*
 + * Remove the refs listed in 'refnames' from the packed-refs file.
 + * On error, packed-refs will be unchanged, the return value is
 + * nonzero, and a message about the error is written to the 'err'
 + * strbuf.
 + *
 + * The refs in 'refnames' needn't be sorted. The err buffer must not be
 + * omitted.
 
 (nit)
 
 s/buffer/strbuf/, or s/The err buffer/'err'/
 s/omitted/NULL/

I will fix this too (and improve the docstring a bit in general). Thanks
for your careful review!

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 4/6] repack_without_refs(): make the refnames argument a string_list

2014-11-22 Thread Jonathan Nieder 
Michael Haggerty wrote:

 All of the callers have string_lists available already

Technically ref_transaction_commit doesn't, but that doesn't matter.

 Suggested-by: Ronnie Sahlberg sahlb...@google.com
 Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
 ---
  builtin/remote.c | 14 ++
  refs.c   | 38 --
  refs.h   | 11 ++-
  3 files changed, 32 insertions(+), 31 deletions(-)

Reviewed-by: Jonathan Nieder jrnie...@gmail.com

One (optional) nit at the bottom of this message.

[...]
 +++ b/refs.c
 @@ -2639,22 +2639,25 @@ static int curate_packed_ref_fn(struct ref_entry 
 *entry, void *cb_data)
   return 0;
  }
  
 -int repack_without_refs(const char **refnames, int n, struct strbuf *err)
 +int repack_without_refs(struct string_list *refnames, struct strbuf *err)
  {
   struct ref_dir *packed;
   struct string_list refs_to_delete = STRING_LIST_INIT_DUP;
 - struct string_list_item *ref_to_delete;
 - int i, ret, removed = 0;
 + struct string_list_item *refname, *ref_to_delete;
 + int ret, needs_repacking = 0, removed = 0;
  
   assert(err);
  
   /* Look for a packed ref */
 - for (i = 0; i  n; i++)
 - if (get_packed_ref(refnames[i]))
 + for_each_string_list_item(refname, refnames) {
 + if (get_packed_ref(refname-string)) {
 + needs_repacking = 1;
   break;
 + }
 + }
  
   /* Avoid locking if we have nothing to do */
 - if (i == n)
 + if (!needs_repacking)

This makes me wish C supported something like Python's for/else
construct.  Oh well. :)

[...]
 +++ b/refs.h
 @@ -163,7 +163,16 @@ extern void rollback_packed_refs(void);
   */
  int pack_refs(unsigned int flags);
  
 -extern int repack_without_refs(const char **refnames, int n,
 +/*
 + * Remove the refs listed in 'refnames' from the packed-refs file.
 + * On error, packed-refs will be unchanged, the return value is
 + * nonzero, and a message about the error is written to the 'err'
 + * strbuf.
 + *
 + * The refs in 'refnames' needn't be sorted. The err buffer must not be
 + * omitted.

(nit)

s/buffer/strbuf/, or s/The err buffer/'err'/
s/omitted/NULL/

Thanks,
Jonathan
--
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 4/6] repack_without_refs(): make the refnames argument a string_list

2014-11-21 Thread Michael Haggerty 
All of the callers have string_lists available already, whereas two of
them had to read data out of a string_list into an array of strings
just to call this function. So change repack_without_refs() to take
the list of refnames to omit as a string_list, and change the callers
accordingly.

Suggested-by: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
---
 builtin/remote.c | 14 ++
 refs.c   | 38 --
 refs.h   | 11 ++-
 3 files changed, 32 insertions(+), 31 deletions(-)

diff --git a/builtin/remote.c b/builtin/remote.c
index 7d5c8d2..63a6709 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -750,16 +750,11 @@ static int mv(int argc, const char **argv)
 static int remove_branches(struct string_list *branches)
 {
struct strbuf err = STRBUF_INIT;
-   const char **branch_names;
int i, result = 0;
 
-   branch_names = xmalloc(branches-nr * sizeof(*branch_names));
-   for (i = 0; i  branches-nr; i++)
-   branch_names[i] = branches-items[i].string;
-   if (repack_without_refs(branch_names, branches-nr, err))
+   if (repack_without_refs(branches, err))
result |= error(%s, err.buf);
strbuf_release(err);
-   free(branch_names);
 
for (i = 0; i  branches-nr; i++) {
struct string_list_item *item = branches-items + i;
@@ -1317,7 +1312,6 @@ static int prune_remote(const char *remote, int dry_run)
int result = 0, i;
struct ref_states states;
struct string_list delete_refs_list = STRING_LIST_INIT_NODUP;
-   const char **delete_refs;
const char *dangling_msg = dry_run
? _( %s will become dangling!)
: _( %s has become dangling!);
@@ -1336,19 +1330,16 @@ static int prune_remote(const char *remote, int dry_run)
  ? states.remote-url[0]
  : _((no URL)));
 
-   delete_refs = xmalloc(states.stale.nr * sizeof(*delete_refs));
for (i = 0; i  states.stale.nr; i++) {
const char *refname = states.stale.items[i].util;
 
-   delete_refs[i] = refname;
string_list_append(delete_refs_list, refname);
}
sort_string_list(delete_refs_list);
 
if (!dry_run) {
struct strbuf err = STRBUF_INIT;
-   if (repack_without_refs(delete_refs, states.stale.nr,
-   err))
+   if (repack_without_refs(delete_refs_list, err))
result |= error(%s, err.buf);
strbuf_release(err);
}
@@ -1369,7 +1360,6 @@ static int prune_remote(const char *remote, int dry_run)
 
warn_dangling_symrefs(stdout, dangling_msg, delete_refs_list);
 
-   free(delete_refs);
string_list_clear(delete_refs_list, 0);
free_remote_ref_states(states);
return result;
diff --git a/refs.c b/refs.c
index 5ff457e..b675e01 100644
--- a/refs.c
+++ b/refs.c
@@ -2639,22 +2639,25 @@ static int curate_packed_ref_fn(struct ref_entry 
*entry, void *cb_data)
return 0;
 }
 
-int repack_without_refs(const char **refnames, int n, struct strbuf *err)
+int repack_without_refs(struct string_list *refnames, struct strbuf *err)
 {
struct ref_dir *packed;
struct string_list refs_to_delete = STRING_LIST_INIT_DUP;
-   struct string_list_item *ref_to_delete;
-   int i, ret, removed = 0;
+   struct string_list_item *refname, *ref_to_delete;
+   int ret, needs_repacking = 0, removed = 0;
 
assert(err);
 
/* Look for a packed ref */
-   for (i = 0; i  n; i++)
-   if (get_packed_ref(refnames[i]))
+   for_each_string_list_item(refname, refnames) {
+   if (get_packed_ref(refname-string)) {
+   needs_repacking = 1;
break;
+   }
+   }
 
/* Avoid locking if we have nothing to do */
-   if (i == n)
+   if (!needs_repacking)
return 0; /* no refname exists in packed refs */
 
if (lock_packed_refs(0)) {
@@ -2664,8 +2667,8 @@ int repack_without_refs(const char **refnames, int n, 
struct strbuf *err)
packed = get_packed_refs(ref_cache);
 
/* Remove refnames from the cache */
-   for (i = 0; i  n; i++)
-   if (remove_entry(packed, refnames[i]) != -1)
+   for_each_string_list_item(refname, refnames)
+   if (remove_entry(packed, refname-string) != -1)
removed = 1;
if (!removed) {
/*
@@ -3738,10 +3741,11 @@ static int ref_update_reject_duplicates(struct 
ref_update **updates, int n,
 int ref_transaction_commit(struct ref_transaction *transaction,
   struct strbuf *err)
 {
-   int ret = 0, delnum = 0, i;
-   const char **delnames;
+   int ret = 0, i;
int n = transaction-nr;