From: Junio C Hamano <hidden> Date: 2016-06-15 23:02:59
Stefan Beller [off-list ref] writes:
This patch doesn't intend any functional changes. It is just
a refactoring, which replaces a char** array by a stringlist
in the function repack_without_refs.
This is easier to read and maintain as it delivers the same
functionality with less lines of code less pointers.
Signed-off-by: Ronnie Sahlberg <redacted>
Signed-off-by: Stefan Beller <redacted>
-
Have three of them, not just one, here. (no need to resend to fix
only this). Or...
This patch was heavily inspired by a part of the ref-transactions-rename
series[1], but people tend to dislike large series and this part is
relatively easy to take out and unrelated, so I'll send it as a single
patch.
[1] https://www.mail-archive.com/git@vger.kernel.org/msg60604.html
---
... next time, write the comments here, where Git already gives you
three dashes.
Also mention what you updated and why relative to your earlier round
here, if not covered in the log message already.
For example, renaming of delete_refs_list (in v1) to delete_refs
(this version) is a sensible change because readers know it is a
list from its type being string_list already, but that change is new
relative to the codebase, so it could go to the log message ("Having
array delete_refs[] and string_list delete_refs_list is redundant;
drop the array and give the string_list variable the shorter name",
or something like that) if you wanted to.
quoted hunk
@@ -1316,8 +1311,8 @@ 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;+ struct string_list delete_refs = STRING_LIST_INIT_NODUP;+ struct string_list_item *ref; const char *dangling_msg = dry_run ? _(" %s will become dangling!") : _(" %s has become dangling!");
What are you trying to do here?
Initialise delete_refs to an empty string list, and then iterate
over its elements and append them into the same string list???
It looks like a "currently noop, waiting for somebody to throw an
item to the list before this code, at which time it turns into an
infinite memory eater".
Curious...
From: Stefan Beller <hidden> Date: 2016-06-15 23:02:59
From: Ronnie Sahlberg <redacted>
This patch doesn't intend any functional changes. It is just
a refactoring, which replaces a char** array by a stringlist
in the function repack_without_refs.
This is easier to read and maintain as it delivers the same
functionality with less lines of code and less pointers.
[sb: ported this patch from a larger patch series to the master branch,
added documentary comments in refs.h]
Signed-off-by: Ronnie Sahlberg <redacted>
Signed-off-by: Stefan Beller <redacted>
---
On Wed, Nov 19, 2014 at 10:00 AM, Junio C Hamano [off-list ref] wrote:
+ for_each_string_list_item(ref, &delete_refs)
+ string_list_append(&delete_refs, ref->string);
What are you trying to do here?
I messed up this patch completely yesterday in the evening.
Essentially the inter-patch diff are all the nits by Jonathan.
So here is my attempt on sending a more maintainer friendly patch.
Changes to version 1:
* removed the double blank line
* rename delete_refs_list to delete_refs
* add back comments dropped by accident
* use STRING_LIST_INIT_NODUP instead of the _DUP version in ref_transaction_commit
* add documentary comments on the repack_without_refs function
* user string_list_append instead of string_list_insert as it follows the previous
behavior more closely.
* put back the early exit of the loop in repack_without_refs
Changes to version 2:
* fixed commit message (comments after the three dashes)
* fixed the curiosity Junio pointed out as it was just wrong code.
Now it actually builds a list of all states.stale.items[i].util items.
builtin/remote.c | 31 +++++++++++--------------------
refs.c | 40 +++++++++++++++++++++-------------------
refs.h | 10 ++++++++--
3 files changed, 40 insertions(+), 41 deletions(-)
@@ -1316,8 +1311,8 @@ static int prune_remote(const char *remote, int dry_run){intresult=0,i;structref_statesstates;-structstring_listdelete_refs_list=STRING_LIST_INIT_NODUP;-constchar**delete_refs;+structstring_listdelete_refs=STRING_LIST_INIT_NODUP;+structstring_list_item*ref;constchar*dangling_msg=dry_run?_(" %s will become dangling!"):_(" %s has become dangling!");
@@ -1325,6 +1320,9 @@ static int prune_remote(const char *remote, int dry_run)memset(&states,0,sizeof(states));get_remote_ref_states(remote,&states,GET_REF_STATES);+for(i=0;i<states.stale.nr;i++)+string_list_append(&delete_refs,states.stale.items[i].util);+if(states.stale.nr){printf_ln(_("Pruning %s"),remote);printf_ln(_("URL: %s"),
@@ -1332,23 +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++)-delete_refs[i]=states.stale.items[i].util;if(!dry_run){structstrbuferr=STRBUF_INIT;-if(repack_without_refs(delete_refs,states.stale.nr,-&err))+if(repack_without_refs(&delete_refs,&err))result|=error("%s",err.buf);strbuf_release(&err);}-free(delete_refs);}-for(i=0;i<states.stale.nr;i++){-constchar*refname=states.stale.items[i].util;--string_list_insert(&delete_refs_list,refname);+for_each_string_list_item(ref,&delete_refs){+constchar*refname=ref->string;if(!dry_run)result|=delete_ref(refname,NULL,0);
@@ -1361,8 +1352,8 @@ static int prune_remote(const char *remote, int dry_run)abbrev_ref(refname,"refs/remotes/"));}-warn_dangling_symrefs(stdout,dangling_msg,&delete_refs_list);-string_list_clear(&delete_refs_list,0);+warn_dangling_symrefs(stdout,dangling_msg,&delete_refs);+string_list_clear(&delete_refs,0);free_remote_ref_states(&states);returnresult;
@@ -2639,23 +2639,26 @@ static int curate_packed_ref_fn(struct ref_entry *entry, void *cb_data)return0;}-intrepack_without_refs(constchar**refnames,intn,structstrbuf*err)+intrepack_without_refs(structstring_list*without,structstrbuf*err){structref_dir*packed;structstring_listrefs_to_delete=STRING_LIST_INIT_DUP;structstring_list_item*ref_to_delete;-inti,ret,removed=0;+intret,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(ref_to_delete,without){+if(get_packed_ref(ref_to_delete->string)){+needs_repacking=1;break;+}+}-/* Avoid locking if we have nothing to do */-if(i==n)-return0;/* no refname exists in packed refs */+/* No refname exists in packed refs */+if(!needs_repacking)+return0;if(lock_packed_refs(0)){unable_to_lock_message(git_path("packed-refs"),errno,err);
@@ -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(ref_to_delete,without)+if(remove_entry(packed,ref_to_delete->string)!=-1)removed=1;if(!removed){/*
@@ -3738,10 +3741,11 @@ static int ref_update_reject_duplicates(struct ref_update **updates, int n,intref_transaction_commit(structref_transaction*transaction,structstrbuf*err){-intret=0,delnum=0,i;-constchar**delnames;+intret=0,i;intn=transaction->nr;structref_update**updates=transaction->updates;+structstring_listrefs_to_delete=STRING_LIST_INIT_NODUP;+structstring_list_item*ref_to_delete;assert(err);
@@ -3753,9 +3757,6 @@ int ref_transaction_commit(struct ref_transaction *transaction,return0;}-/* Allocate work space */-delnames=xmalloc(sizeof(*delnames)*n);-/* Copy, sort, and reject duplicate refs */qsort(updates,n,sizeof(*updates),ref_update_compare);if(ref_update_reject_duplicates(updates,n,err)){
@@ -3815,16 +3816,17 @@ int ref_transaction_commit(struct ref_transaction *transaction,}if(!(update->flags&REF_ISPRUNING))-delnames[delnum++]=update->lock->ref_name;+string_list_append(&refs_to_delete,+update->lock->ref_name);}}-if(repack_without_refs(delnames,delnum,err)){+if(repack_without_refs(&refs_to_delete,err)){ret=TRANSACTION_GENERIC_ERROR;gotocleanup;}-for(i=0;i<delnum;i++)-unlink_or_warn(git_path("logs/%s",delnames[i]));+for_each_string_list_item(ref_to_delete,&refs_to_delete)+unlink_or_warn(git_path("logs/%s",ref_to_delete->string));clear_loose_ref_cache(&ref_cache);cleanup:
From: Jonathan Nieder <hidden> Date: 2016-06-15 23:02:59
Stefan Beller wrote:
This patch doesn't intend any functional changes.
Yay. :)
a refactoring, which replaces a char** array by a stringlist
in the function repack_without_refs.
This is easier to read and maintain as it delivers the same
functionality with less lines of code and less pointers.
Please wrap to a consistent width and add a blank line between
paragraphs. So, either:
... repack_without_refs. This is easier to read and ...
or:
... repack_without_refs.
This is easier to read and ...
[...]
@@ -1325,6 +1320,9 @@ static int prune_remote(const char *remote, int dry_run) memset(&states, 0, sizeof(states)); get_remote_ref_states(remote, &states, GET_REF_STATES);+ for (i = 0; i < states.stale.nr; i++)+ string_list_append(&delete_refs, states.stale.items[i].util);
warn_dangling_symref requires a sorted list. Possible fixes:
(a) switch to string_list_insert, or
(b) [nicer] call sort_string_list before the warn_dangling_symrefs
call.
[...]
quoted hunk
--- a/refs.c+++ b/refs.c
@@ -2639,23 +2639,26 @@ static int curate_packed_ref_fn(struct ref_entry *entry, void *cb_data)return0;}-intrepack_without_refs(constchar**refnames,intn,structstrbuf*err)+intrepack_without_refs(structstring_list*without,structstrbuf*err){structref_dir*packed;structstring_listrefs_to_delete=STRING_LIST_INIT_DUP;structstring_list_item*ref_to_delete;-inti,ret,removed=0;+intret,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(ref_to_delete,without){+if(get_packed_ref(ref_to_delete->string)){+needs_repacking=1;break;+}+}-/* Avoid locking if we have nothing to do */
This comment was helpful --- it's sad to lose it (but if you feel
strongly about it then I don't mind).
- if (i == n)
- return 0; /* no refname exists in packed refs */
+ /* No refname exists in packed refs */
+ if (!needs_repacking)
+ return 0;
I kind of liked the 'i == n' test that avoided needing a new auxiliary
variable. This is fine and probably a little clearer, though.
[...]
(nit) Other comments in this file use the imperative mood to describe
what a function does, so it would be a little clearer to do that here,
too ("Repack the ..." instead of "Repacks the ...").
It might be just me, but I find this formatted comment with everything
jammed together hard to read. I'd prefer a simple paragraph, like:
/*
* Remove the refs listed in 'without' 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.
*/
Thanks,
Jonathan
From: Stefan Beller <hidden> Date: 2016-06-15 23:02:59
From: Ronnie Sahlberg <redacted>
This patch doesn't intend any functional changes. It is just
a refactoring, which replaces a char** array by a stringlist
in the function repack_without_refs.
This is easier to read and maintain as it delivers the same
functionality with less lines of code and less pointers.
[sb: ported this patch from a larger patch series to the master branch,
added documentary comments in refs.h]
Signed-off-by: Ronnie Sahlberg <redacted>
Signed-off-by: Stefan Beller <redacted>
---
version 3 includes all nits by Jonathan.
Changes to version 1:
* removed the double blank line
* rename delete_refs_list to delete_refs
* add back comments dropped by accident
* use STRING_LIST_INIT_NODUP instead of the _DUP version in ref_transaction_commit
* add documentary comments on the repack_without_refs function
* user string_list_append instead of string_list_insert as it follows the previous
behavior more closely.
* put back the early exit of the loop in repack_without_refs
Changes to version 2:
* fixed commit message (comments after the three dashes)
* fixed the curiosity Junio pointed out as it was just wrong code.
Now it actually builds a list of all states.stale.items[i].util items.
Changes in version 3:
* reword commit message
* sort delete_refs before passing it to warn_dangling_symrefs
* change the comments (get back the one jrn complained about)
in repack_without_refs
* use the suggestion of jonathan for documenting repack_without_refs in the
header. Add a note about the arguments.
---
builtin/remote.c | 32 ++++++++++++--------------------
refs.c | 38 ++++++++++++++++++++------------------
refs.h | 10 ++++++++--
3 files changed, 40 insertions(+), 40 deletions(-)
@@ -1316,8 +1311,8 @@ static int prune_remote(const char *remote, int dry_run){intresult=0,i;structref_statesstates;-structstring_listdelete_refs_list=STRING_LIST_INIT_NODUP;-constchar**delete_refs;+structstring_listdelete_refs=STRING_LIST_INIT_NODUP;+structstring_list_item*ref;constchar*dangling_msg=dry_run?_(" %s will become dangling!"):_(" %s has become dangling!");
@@ -1325,6 +1320,9 @@ static int prune_remote(const char *remote, int dry_run)memset(&states,0,sizeof(states));get_remote_ref_states(remote,&states,GET_REF_STATES);+for(i=0;i<states.stale.nr;i++)+string_list_append(&delete_refs,states.stale.items[i].util);+if(states.stale.nr){printf_ln(_("Pruning %s"),remote);printf_ln(_("URL: %s"),
@@ -1332,23 +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++)-delete_refs[i]=states.stale.items[i].util;if(!dry_run){structstrbuferr=STRBUF_INIT;-if(repack_without_refs(delete_refs,states.stale.nr,-&err))+if(repack_without_refs(&delete_refs,&err))result|=error("%s",err.buf);strbuf_release(&err);}-free(delete_refs);}-for(i=0;i<states.stale.nr;i++){-constchar*refname=states.stale.items[i].util;--string_list_insert(&delete_refs_list,refname);+for_each_string_list_item(ref,&delete_refs){+constchar*refname=ref->string;if(!dry_run)result|=delete_ref(refname,NULL,0);
@@ -1361,8 +1352,9 @@ static int prune_remote(const char *remote, int dry_run)abbrev_ref(refname,"refs/remotes/"));}-warn_dangling_symrefs(stdout,dangling_msg,&delete_refs_list);-string_list_clear(&delete_refs_list,0);+sort_string_list(&delete_refs);+warn_dangling_symrefs(stdout,dangling_msg,&delete_refs);+string_list_clear(&delete_refs,0);free_remote_ref_states(&states);returnresult;
@@ -2639,23 +2639,26 @@ static int curate_packed_ref_fn(struct ref_entry *entry, void *cb_data)return0;}-intrepack_without_refs(constchar**refnames,intn,structstrbuf*err)+intrepack_without_refs(structstring_list*without,structstrbuf*err){structref_dir*packed;structstring_listrefs_to_delete=STRING_LIST_INIT_DUP;structstring_list_item*ref_to_delete;-inti,ret,removed=0;+intret,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(ref_to_delete,without){+if(get_packed_ref(ref_to_delete->string)){+needs_repacking=1;break;+}+}/* Avoid locking if we have nothing to do */-if(i==n)-return0;/* no refname exists in packed refs */+if(!needs_repacking)+return0;if(lock_packed_refs(0)){unable_to_lock_message(git_path("packed-refs"),errno,err);
@@ -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(ref_to_delete,without)+if(remove_entry(packed,ref_to_delete->string)!=-1)removed=1;if(!removed){/*
@@ -3738,10 +3741,11 @@ static int ref_update_reject_duplicates(struct ref_update **updates, int n,intref_transaction_commit(structref_transaction*transaction,structstrbuf*err){-intret=0,delnum=0,i;-constchar**delnames;+intret=0,i;intn=transaction->nr;structref_update**updates=transaction->updates;+structstring_listrefs_to_delete=STRING_LIST_INIT_NODUP;+structstring_list_item*ref_to_delete;assert(err);
@@ -3753,9 +3757,6 @@ int ref_transaction_commit(struct ref_transaction *transaction,return0;}-/* Allocate work space */-delnames=xmalloc(sizeof(*delnames)*n);-/* Copy, sort, and reject duplicate refs */qsort(updates,n,sizeof(*updates),ref_update_compare);if(ref_update_reject_duplicates(updates,n,err)){
@@ -3815,16 +3816,17 @@ int ref_transaction_commit(struct ref_transaction *transaction,}if(!(update->flags&REF_ISPRUNING))-delnames[delnum++]=update->lock->ref_name;+string_list_append(&refs_to_delete,+update->lock->ref_name);}}-if(repack_without_refs(delnames,delnum,err)){+if(repack_without_refs(&refs_to_delete,err)){ret=TRANSACTION_GENERIC_ERROR;gotocleanup;}-for(i=0;i<delnum;i++)-unlink_or_warn(git_path("logs/%s",delnames[i]));+for_each_string_list_item(ref_to_delete,&refs_to_delete)+unlink_or_warn(git_path("logs/%s",ref_to_delete->string));clear_loose_ref_cache(&ref_cache);cleanup:
From: Stefan Beller <hidden> Date: 2016-06-15 23:02:59
From: Ronnie Sahlberg <redacted>
This patch doesn't intend any functional changes. It is just
a refactoring, which replaces a char** array by a stringlist
in the function repack_without_refs.
This is easier to read and maintain as it delivers the same
functionality with less lines of code and less pointers.
[sb: ported this patch from a larger patch series to the master branch,
added documentary comments in refs.h]
Signed-off-by: Ronnie Sahlberg <redacted>
Signed-off-by: Stefan Beller <redacted>
---
Changes to version 1:
* removed the double blank line
* rename delete_refs_list to delete_refs
* add back comments dropped by accident
* use STRING_LIST_INIT_NODUP instead of the _DUP version in ref_transaction_commit
* add documentary comments on the repack_without_refs function
* user string_list_append instead of string_list_insert as it follows the previous
behavior more closely.
* put back the early exit of the loop in repack_without_refs
Changes to version 2:
* fixed commit message (comments after the three dashes)
* fixed the curiosity Junio pointed out as it was just wrong code.
Now it actually builds a list of all states.stale.items[i].util items.
Changes in version 3:
* reword commit message
* sort delete_refs before passing it to warn_dangling_symrefs
* change the comments (get back the one jrn complained about)
in repack_without_refs
* use the suggestion of jonathan for documenting repack_without_refs in the
header. Add a note about the arguments.
Changes in version 4:
* I lied, when saying I had all the nits from Jonathan.
I messed up the documentation in the header.
This includes the documentary comment in the header.
---
builtin/remote.c | 32 ++++++++++++--------------------
refs.c | 38 ++++++++++++++++++++------------------
refs.h | 11 +++++++++--
3 files changed, 41 insertions(+), 40 deletions(-)
@@ -1316,8 +1311,8 @@ static int prune_remote(const char *remote, int dry_run){intresult=0,i;structref_statesstates;-structstring_listdelete_refs_list=STRING_LIST_INIT_NODUP;-constchar**delete_refs;+structstring_listdelete_refs=STRING_LIST_INIT_NODUP;+structstring_list_item*ref;constchar*dangling_msg=dry_run?_(" %s will become dangling!"):_(" %s has become dangling!");
@@ -1325,6 +1320,9 @@ static int prune_remote(const char *remote, int dry_run)memset(&states,0,sizeof(states));get_remote_ref_states(remote,&states,GET_REF_STATES);+for(i=0;i<states.stale.nr;i++)+string_list_append(&delete_refs,states.stale.items[i].util);+if(states.stale.nr){printf_ln(_("Pruning %s"),remote);printf_ln(_("URL: %s"),
@@ -1332,23 +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++)-delete_refs[i]=states.stale.items[i].util;if(!dry_run){structstrbuferr=STRBUF_INIT;-if(repack_without_refs(delete_refs,states.stale.nr,-&err))+if(repack_without_refs(&delete_refs,&err))result|=error("%s",err.buf);strbuf_release(&err);}-free(delete_refs);}-for(i=0;i<states.stale.nr;i++){-constchar*refname=states.stale.items[i].util;--string_list_insert(&delete_refs_list,refname);+for_each_string_list_item(ref,&delete_refs){+constchar*refname=ref->string;if(!dry_run)result|=delete_ref(refname,NULL,0);
@@ -1361,8 +1352,9 @@ static int prune_remote(const char *remote, int dry_run)abbrev_ref(refname,"refs/remotes/"));}-warn_dangling_symrefs(stdout,dangling_msg,&delete_refs_list);-string_list_clear(&delete_refs_list,0);+sort_string_list(&delete_refs);+warn_dangling_symrefs(stdout,dangling_msg,&delete_refs);+string_list_clear(&delete_refs,0);free_remote_ref_states(&states);returnresult;
@@ -2639,23 +2639,26 @@ static int curate_packed_ref_fn(struct ref_entry *entry, void *cb_data)return0;}-intrepack_without_refs(constchar**refnames,intn,structstrbuf*err)+intrepack_without_refs(structstring_list*without,structstrbuf*err){structref_dir*packed;structstring_listrefs_to_delete=STRING_LIST_INIT_DUP;structstring_list_item*ref_to_delete;-inti,ret,removed=0;+intret,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(ref_to_delete,without){+if(get_packed_ref(ref_to_delete->string)){+needs_repacking=1;break;+}+}/* Avoid locking if we have nothing to do */-if(i==n)-return0;/* no refname exists in packed refs */+if(!needs_repacking)+return0;if(lock_packed_refs(0)){unable_to_lock_message(git_path("packed-refs"),errno,err);
@@ -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(ref_to_delete,without)+if(remove_entry(packed,ref_to_delete->string)!=-1)removed=1;if(!removed){/*
@@ -3738,10 +3741,11 @@ static int ref_update_reject_duplicates(struct ref_update **updates, int n,intref_transaction_commit(structref_transaction*transaction,structstrbuf*err){-intret=0,delnum=0,i;-constchar**delnames;+intret=0,i;intn=transaction->nr;structref_update**updates=transaction->updates;+structstring_listrefs_to_delete=STRING_LIST_INIT_NODUP;+structstring_list_item*ref_to_delete;assert(err);
@@ -3753,9 +3757,6 @@ int ref_transaction_commit(struct ref_transaction *transaction,return0;}-/* Allocate work space */-delnames=xmalloc(sizeof(*delnames)*n);-/* Copy, sort, and reject duplicate refs */qsort(updates,n,sizeof(*updates),ref_update_compare);if(ref_update_reject_duplicates(updates,n,err)){
@@ -3815,16 +3816,17 @@ int ref_transaction_commit(struct ref_transaction *transaction,}if(!(update->flags&REF_ISPRUNING))-delnames[delnum++]=update->lock->ref_name;+string_list_append(&refs_to_delete,+update->lock->ref_name);}}-if(repack_without_refs(delnames,delnum,err)){+if(repack_without_refs(&refs_to_delete,err)){ret=TRANSACTION_GENERIC_ERROR;gotocleanup;}-for(i=0;i<delnum;i++)-unlink_or_warn(git_path("logs/%s",delnames[i]));+for_each_string_list_item(ref_to_delete,&refs_to_delete)+unlink_or_warn(git_path("logs/%s",ref_to_delete->string));clear_loose_ref_cache(&ref_cache);cleanup:
From: Jonathan Nieder <hidden> Date: 2016-06-15 23:03:00
Stefan Beller wrote:
From: Ronnie Sahlberg <redacted>
This patch doesn't intend any functional changes. It is just
a refactoring, which replaces a char** array by a stringlist
in the function repack_without_refs.
This is easier to read and maintain as it delivers the same
functionality with less lines of code and less pointers.
Thanks for the quick turnaround.
Nit: please wrap to a consistent width and put a blank line between
paragraphs.
That is, the above should either say
This patch doesn't intend any functional changes. It is just
a refactoring to replace a char** array with a string_list
in the function repack_without_refs. This is easier to read
and maintain as it delivers the same functionality with less
code and fewer pointers.
or
This patch doesn't intend any functional changes. It is just
a refactoring to replace a char** array with a string_list
in the function repack_without_refs.
This is easier to read and maintain as it delivers the same
functionality with less code and fewer pointers.
Although I'm not sure the main benefit is having fewer asterisks. ;-)
[...]
Micronit: it would be clearer (and easier to remember to free the list
in other code paths if this function gains more 'return' statements)
with the string_list_clear in the same block as other code that frees
resources (i.e., if the blank line moved one line up).
[...]
Nits:
s/ommited/omitted/
Comma splice. Long line.
The function has to be able to write to 'err' on error, so I think the
comment doesn't have to mention that err must be non-NULL. Any caller
that tries to pass NULL will get an assertion error quickly.
With or without the changes suggested above,
Reviewed-by: Jonathan Nieder <redacted>
From: Stefan Beller <hidden> Date: 2016-06-15 23:03:00
From: Ronnie Sahlberg <redacted>
This patch doesn't intend any functional changes. It is just
a refactoring, which replaces a char** array by a stringlist
in the function repack_without_refs.
This is easier to read and maintain as it delivers the same
functionality with less lines of code and more lines of
documentation.
[sb: ported this patch from a larger patch series to the
master branch, added documentary comments in refs.h]
Change-Id: Id7eaa821331f2ab89df063e1e76c8485dbcc3aed
Signed-off-by: Ronnie Sahlberg <redacted>
Signed-off-by: Stefan Beller <redacted>
Reviewed-by: Jonathan Nieder <redacted>
---
Changes to version 1:
* removed the double blank line
* rename delete_refs_list to delete_refs
* add back comments dropped by accident
* use STRING_LIST_INIT_NODUP instead of the _DUP version in ref_transaction_commit
* add documentary comments on the repack_without_refs function
* user string_list_append instead of string_list_insert as it follows the previous
behavior more closely.
* put back the early exit of the loop in repack_without_refs
Changes to version 2:
* fixed commit message (comments after the three dashes)
* fixed the curiosity Junio pointed out as it was just wrong code.
Now it actually builds a list of all states.stale.items[i].util items.
Changes in version 3:
* reword commit message
* sort delete_refs before passing it to warn_dangling_symrefs
* change the comments (get back the one jrn complained about)
in repack_without_refs
* use the suggestion of jonathan for documenting repack_without_refs in the
header. Add a note about the arguments.
Changes in version 4:
* I lied, when saying I had all the nits from Jonathan.
I messed up the documentation in the header.
This includes the documentary comment in the header.
Changes in version 5:
* Break lines as suggested by Jonathan, slightly rewording the commit message
* have an empty line at another place in builtin/remote.c remove_branches to
tell cleanup parts apart from actual work.
* fix typo, improve documentary comment in refs.c
* add Jonathans reviewed by
Junio, I'll address your proposed changes in a different patch.
If err is passed in as NULL, we'll just skip all the error string
formatting and return silent and fast.
builtin/remote.c | 32 ++++++++++++--------------------
refs.c | 38 ++++++++++++++++++++------------------
refs.h | 12 ++++++++++--
3 files changed, 42 insertions(+), 40 deletions(-)
@@ -1316,8 +1311,8 @@ static int prune_remote(const char *remote, int dry_run){intresult=0,i;structref_statesstates;-structstring_listdelete_refs_list=STRING_LIST_INIT_NODUP;-constchar**delete_refs;+structstring_listdelete_refs=STRING_LIST_INIT_NODUP;+structstring_list_item*ref;constchar*dangling_msg=dry_run?_(" %s will become dangling!"):_(" %s has become dangling!");
@@ -1325,6 +1320,9 @@ static int prune_remote(const char *remote, int dry_run)memset(&states,0,sizeof(states));get_remote_ref_states(remote,&states,GET_REF_STATES);+for(i=0;i<states.stale.nr;i++)+string_list_append(&delete_refs,states.stale.items[i].util);+if(states.stale.nr){printf_ln(_("Pruning %s"),remote);printf_ln(_("URL: %s"),
@@ -1332,23 +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++)-delete_refs[i]=states.stale.items[i].util;if(!dry_run){structstrbuferr=STRBUF_INIT;-if(repack_without_refs(delete_refs,states.stale.nr,-&err))+if(repack_without_refs(&delete_refs,&err))result|=error("%s",err.buf);strbuf_release(&err);}-free(delete_refs);}-for(i=0;i<states.stale.nr;i++){-constchar*refname=states.stale.items[i].util;--string_list_insert(&delete_refs_list,refname);+for_each_string_list_item(ref,&delete_refs){+constchar*refname=ref->string;if(!dry_run)result|=delete_ref(refname,NULL,0);
@@ -1361,9 +1352,10 @@ static int prune_remote(const char *remote, int dry_run)abbrev_ref(refname,"refs/remotes/"));}-warn_dangling_symrefs(stdout,dangling_msg,&delete_refs_list);-string_list_clear(&delete_refs_list,0);+sort_string_list(&delete_refs);+warn_dangling_symrefs(stdout,dangling_msg,&delete_refs);+string_list_clear(&delete_refs,0);free_remote_ref_states(&states);returnresult;}
@@ -2639,23 +2639,26 @@ static int curate_packed_ref_fn(struct ref_entry *entry, void *cb_data)return0;}-intrepack_without_refs(constchar**refnames,intn,structstrbuf*err)+intrepack_without_refs(structstring_list*without,structstrbuf*err){structref_dir*packed;structstring_listrefs_to_delete=STRING_LIST_INIT_DUP;structstring_list_item*ref_to_delete;-inti,ret,removed=0;+intret,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(ref_to_delete,without){+if(get_packed_ref(ref_to_delete->string)){+needs_repacking=1;break;+}+}/* Avoid locking if we have nothing to do */-if(i==n)-return0;/* no refname exists in packed refs */+if(!needs_repacking)+return0;if(lock_packed_refs(0)){unable_to_lock_message(git_path("packed-refs"),errno,err);
@@ -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(ref_to_delete,without)+if(remove_entry(packed,ref_to_delete->string)!=-1)removed=1;if(!removed){/*
@@ -3738,10 +3741,11 @@ static int ref_update_reject_duplicates(struct ref_update **updates, int n,intref_transaction_commit(structref_transaction*transaction,structstrbuf*err){-intret=0,delnum=0,i;-constchar**delnames;+intret=0,i;intn=transaction->nr;structref_update**updates=transaction->updates;+structstring_listrefs_to_delete=STRING_LIST_INIT_NODUP;+structstring_list_item*ref_to_delete;assert(err);
@@ -3753,9 +3757,6 @@ int ref_transaction_commit(struct ref_transaction *transaction,return0;}-/* Allocate work space */-delnames=xmalloc(sizeof(*delnames)*n);-/* Copy, sort, and reject duplicate refs */qsort(updates,n,sizeof(*updates),ref_update_compare);if(ref_update_reject_duplicates(updates,n,err)){
@@ -3815,16 +3816,17 @@ int ref_transaction_commit(struct ref_transaction *transaction,}if(!(update->flags&REF_ISPRUNING))-delnames[delnum++]=update->lock->ref_name;+string_list_append(&refs_to_delete,+update->lock->ref_name);}}-if(repack_without_refs(delnames,delnum,err)){+if(repack_without_refs(&refs_to_delete,err)){ret=TRANSACTION_GENERIC_ERROR;gotocleanup;}-for(i=0;i<delnum;i++)-unlink_or_warn(git_path("logs/%s",delnames[i]));+for_each_string_list_item(ref_to_delete,&refs_to_delete)+unlink_or_warn(git_path("logs/%s",ref_to_delete->string));clear_loose_ref_cache(&ref_cache);cleanup:
From: Stefan Beller <hidden> Date: 2016-06-15 23:03:00
If we don't pass in the error string buffer, we skip over all
parts dealing with preparing error messages.
Signed-off-by: Stefan Beller <redacted>
---
This goes ontop of [PATCH v5] refs.c: use a stringlist for repack_without_refs
if that makes sense.
refs.c | 8 ++++----
refs.h | 1 -
2 files changed, 4 insertions(+), 5 deletions(-)
@@ -170,7 +170,6 @@ int pack_refs(unsigned int flags);*strbuf.**Therefsin'without'mayhaveanyorder.-*Theerrbuffermustnotbeomitted.*/externintrepack_without_refs(structstring_list*without,structstrbuf*err);
From: Ronnie Sahlberg <hidden> Date: 2016-06-15 23:03:00
On Thu, Nov 20, 2014 at 10:10 AM, Stefan Beller [off-list ref] wrote:
quoted hunk
If we don't pass in the error string buffer, we skip over all
parts dealing with preparing error messages.
Signed-off-by: Stefan Beller <redacted>
---
This goes ontop of [PATCH v5] refs.c: use a stringlist for repack_without_refs
if that makes sense.
refs.c | 8 ++++----
refs.h | 1 -
2 files changed, 4 insertions(+), 5 deletions(-)
@@ -170,7 +170,6 @@ int pack_refs(unsigned int flags);*strbuf.**Therefsin'without'mayhaveanyorder.-*Theerrbuffermustnotbeomitted.*/externintrepack_without_refs(structstring_list*without,structstrbuf*err);--
2.2.0.rc2.23.gca0107e
LGTM
Reviewed-by: Ronnie Sahlberg <redacted>
Nit:
While it does not hurt to allow passing NULL, at some stage later
this function will become
private to refs.c and ONLY be called from within transaction_commit()
which will always
pass a non-NULL err argument.
At that stage we will not strictly need to allow err==NULL since all
callers are guaranteed to
always pass err!=NULL.
That said, having err being optional is probably a better API. Maybe
err should be made optional for all other functions that take
an err strbuf too so that the calling conventions become more consistent?
Tiny nit: this makes me wonder what the order represents --- how do
I pick which order for the refs in without to have?
I think the idea is just that 'without' doesn't have to be sorted (it's
a shame we don't have separate sorted string list and unsorted string
list types or a string_list_sorted() helper to catch bad callers early
to functions that care). One way to say that would be
Remove the refs listed in the unsorted string list 'without' from the
packed-refs file. On error, [...]
+ * The err buffer must not be omitted.
s/buffer/strbuf/, or s/The err buffer/'err'/
s/omitted/NULL/
With the Change-Id dropped, and with or without the above comment nits
addressed,
Reviewed-by: Jonathan Nieder <redacted>
From: Jonathan Nieder <hidden> Date: 2016-06-15 23:03:00
Stefan Beller wrote:
If we don't pass in the error string buffer, we skip over all
parts dealing with preparing error messages.
Please no.
We tried this with the ref transaction code. When someone wants
to silence the message, it is cheap enough to do
struct strbuf ignore = STRBUF_INIT;
if (thing_that_can_fail_in_an_ignorable_way(..., &ignore)) {
... handle the failure ...
}
The extra lines of code make it obvious that the error message is
being dropped, which is a very good thing. The extra work to format a
message in the error case is not so bad and can be mitigated if the
error is a common normal case by passing a flag to not consider it an
error.
Silently losing good diagnostic messages when err == NULL would have
the opposite effect: when there isn't a spare strbuf to put errors in
around, it would be tempting for people coding in a hurry to just pass
NULL, and to readers it would look at first glance like "oh, an
optional paramter was not passed and we are getting the good default
behavior".
This is not a theoretical concern --- it actually happened.
My two cents,
Jonathan
From: Ronnie Sahlberg <hidden> Date: 2016-06-15 23:03:00
On Thu, Nov 20, 2014 at 10:35 AM, Jonathan Nieder [off-list ref] wrote:
Stefan Beller wrote:
quoted
If we don't pass in the error string buffer, we skip over all
parts dealing with preparing error messages.
Please no.
We tried this with the ref transaction code. When someone wants
to silence the message, it is cheap enough to do
struct strbuf ignore = STRBUF_INIT;
if (thing_that_can_fail_in_an_ignorable_way(..., &ignore)) {
... handle the failure ...
}
The extra lines of code make it obvious that the error message is
being dropped, which is a very good thing. The extra work to format a
message in the error case is not so bad and can be mitigated if the
error is a common normal case by passing a flag to not consider it an
error.
Silently losing good diagnostic messages when err == NULL would have
the opposite effect: when there isn't a spare strbuf to put errors in
around, it would be tempting for people coding in a hurry to just pass
NULL, and to readers it would look at first glance like "oh, an
optional paramter was not passed and we are getting the good default
behavior".
This is not a theoretical concern --- it actually happened.
From: Stefan Beller <hidden> Date: 2016-06-15 23:03:00
ok, will drop the patch due to bad design.
On Thu, Nov 20, 2014 at 10:36 AM, Ronnie Sahlberg [off-list ref] wrote:
On Thu, Nov 20, 2014 at 10:35 AM, Jonathan Nieder [off-list ref] wrote:
quoted
Stefan Beller wrote:
quoted
If we don't pass in the error string buffer, we skip over all
parts dealing with preparing error messages.
Please no.
We tried this with the ref transaction code. When someone wants
to silence the message, it is cheap enough to do
struct strbuf ignore = STRBUF_INIT;
if (thing_that_can_fail_in_an_ignorable_way(..., &ignore)) {
... handle the failure ...
}
The extra lines of code make it obvious that the error message is
being dropped, which is a very good thing. The extra work to format a
message in the error case is not so bad and can be mitigated if the
error is a common normal case by passing a flag to not consider it an
error.
Silently losing good diagnostic messages when err == NULL would have
the opposite effect: when there isn't a spare strbuf to put errors in
around, it would be tempting for people coding in a hurry to just pass
NULL, and to readers it would look at first glance like "oh, an
optional paramter was not passed and we are getting the good default
behavior".
This is not a theoretical concern --- it actually happened.
From: Michael Haggerty <hidden> Date: 2016-06-15 23:03:05
Aside from making the logic clearer, this avoids a call to
warn_dangling_symrefs(), which always does a for_each_rawref()
iteration.
Signed-off-by: Michael Haggerty <redacted>
---
builtin/remote.c | 39 +++++++++++++++++++++------------------
1 file changed, 21 insertions(+), 18 deletions(-)
From: Michael Haggerty <hidden> Date: 2016-06-15 23:03:05
Rename "delete_refs_list" to "refs_to_prune". The new name is more
self-explanatory.
Signed-off-by: Michael Haggerty <redacted>
---
builtin/remote.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
@@ -1311,7 +1311,7 @@ static int prune_remote(const char *remote, int dry_run){intresult=0,i;structref_statesstates;-structstring_listdelete_refs_list=STRING_LIST_INIT_NODUP;+structstring_listrefs_to_prune=STRING_LIST_INIT_NODUP;constchar*dangling_msg=dry_run?_(" %s will become dangling!"):_(" %s has become dangling!");
@@ -1333,13 +1333,13 @@ static int prune_remote(const char *remote, int dry_run)for(i=0;i<states.stale.nr;i++){constchar*refname=states.stale.items[i].util;-string_list_append(&delete_refs_list,refname);+string_list_append(&refs_to_prune,refname);}-sort_string_list(&delete_refs_list);+sort_string_list(&refs_to_prune);if(!dry_run){structstrbuferr=STRBUF_INIT;-if(repack_without_refs(&delete_refs_list,&err))+if(repack_without_refs(&refs_to_prune,&err))result|=error("%s",err.buf);strbuf_release(&err);}
@@ -1358,9 +1358,9 @@ static int prune_remote(const char *remote, int dry_run)abbrev_ref(refname,"refs/remotes/"));}-warn_dangling_symrefs(stdout,dangling_msg,&delete_refs_list);+warn_dangling_symrefs(stdout,dangling_msg,&refs_to_prune);-string_list_clear(&delete_refs_list,0);+string_list_clear(&refs_to_prune,0);free_remote_ref_states(&states);returnresult;}
From: Michael Haggerty <hidden> Date: 2016-06-15 23:03:05
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 <redacted>
Signed-off-by: Michael Haggerty <redacted>
---
builtin/remote.c | 14 ++------------
refs.c | 38 ++++++++++++++++++++------------------
refs.h | 11 ++++++++++-
3 files changed, 32 insertions(+), 31 deletions(-)
@@ -1317,7 +1312,6 @@ static int prune_remote(const char *remote, int dry_run)intresult=0,i;structref_statesstates;structstring_listdelete_refs_list=STRING_LIST_INIT_NODUP;-constchar**delete_refs;constchar*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++){constchar*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){structstrbuferr=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);returnresult;
@@ -2639,22 +2639,25 @@ static int curate_packed_ref_fn(struct ref_entry *entry, void *cb_data)return0;}-intrepack_without_refs(constchar**refnames,intn,structstrbuf*err)+intrepack_without_refs(structstring_list*refnames,structstrbuf*err){structref_dir*packed;structstring_listrefs_to_delete=STRING_LIST_INIT_DUP;-structstring_list_item*ref_to_delete;-inti,ret,removed=0;+structstring_list_item*refname,*ref_to_delete;+intret,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)return0;/* 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,intref_transaction_commit(structref_transaction*transaction,structstrbuf*err){-intret=0,delnum=0,i;-constchar**delnames;+intret=0,i;intn=transaction->nr;structref_update**updates=transaction->updates;+structstring_listrefs_to_delete=STRING_LIST_INIT_NODUP;+structstring_list_item*ref_to_delete;assert(err);
@@ -3753,9 +3757,6 @@ int ref_transaction_commit(struct ref_transaction *transaction,return0;}-/* Allocate work space */-delnames=xmalloc(sizeof(*delnames)*n);-/* Copy, sort, and reject duplicate refs */qsort(updates,n,sizeof(*updates),ref_update_compare);if(ref_update_reject_duplicates(updates,n,err)){
@@ -3815,16 +3816,17 @@ int ref_transaction_commit(struct ref_transaction *transaction,}if(!(update->flags&REF_ISPRUNING))-delnames[delnum++]=update->lock->ref_name;+string_list_append(&refs_to_delete,+update->lock->ref_name);}}-if(repack_without_refs(delnames,delnum,err)){+if(repack_without_refs(&refs_to_delete,err)){ret=TRANSACTION_GENERIC_ERROR;gotocleanup;}-for(i=0;i<delnum;i++)-unlink_or_warn(git_path("logs/%s",delnames[i]));+for_each_string_list_item(ref_to_delete,&refs_to_delete)+unlink_or_warn(git_path("logs/%s",ref_to_delete->string));clear_loose_ref_cache(&ref_cache);cleanup:
From: Michael Haggerty <hidden> Date: 2016-06-15 23:03:05
This is basically an atomized version of Ronnie/Jonathan/Stefan's
patch [1] "refs.c: use a stringlist for repack_without_refs". But I've
actually rewritten most of it from scratch, using the original patch
as a reference.
I was reviewing the original patch and it looked mostly OK [2], but I
found it hard to read because it did several steps at once. So I tried
to make the same basic change, but one baby step at a time. This is
the result.
I'm a known fanatic about making the smallest possible changes in each
commit. The goal is to make the patch series as readable as possible,
because reviewers' time is in shorter supply than coders' time.
* Tiny little patches are IMO usually much easier to read than big
ones, because there is less to keep in mind at a time.
* Often tiny changes (e.g., renaming variables or functions) are so
blindingly obvious that one only has to skim them, or even trust
that the author, with the help of the compiler, could hardly have
made a mistake [3].
* Using baby steps keeps the author from introducing unnecessary
changes ("code churn"), by forcing him/her to justify each change on
its own merits.
* Using baby steps makes it harder for substantive changes to get
overlooked or to sneak in without discussion [4].
* If there is a problem, baby commits can be bisected, usually making
it obvious why the bug arose.
* If the mailing list doesn't like part of the series, it is usually
easier to omit a patch from the next reroll than to extract one
change out of a patch that contains multiple logical changes.
* It is often possible to arrange the order of the patches to give the
patch series a good "narrative".
Some members of the community probably disagree with me. Using baby
step patches means that there is more mailing list traffic and more
commits that accumulate in the project's history. There is sometimes a
bit of extra to-and-fro as code is mutated incrementally. Or maybe
other people can just keep more complicated changes in their heads at
one time than I can.
Nevertheless, I submit this version of the patch series for your
amusement. Feel free to ignore it.
[1] http://mid.gmane.org/1416434399-2303-1-git-send-email-sbeller@google.com
[2] Problems that I noticed:
* The commit message refers to "stringlist" where it should be
"string_list".
* One of the loops in prune_remote() iterates using indexes, while
another loop (over the same string_list) uses
for_each_string_list_item().
* The change from using string_list_insert() to string_list_append()
in the same function, followed by sort_string_list(), doesn't remove
duplicates as the old version did. The commit message should
justify that this is OK.
[3] I love the quote from C. A. R. Hoare:
There are two ways of constructing a software design: One way
is to make it so simple that there are obviously no
deficiencies, and the other way is to make it so complicated
that there are no obvious deficiencies.
I think the same thing applies to patches.
[4] Case in point: when I was writing the commit message for patch
3/6, I realized that string_list_insert() omits duplicates whereas
string_list_append() obviously doesn't. This aspect of the change
wasn't justified. Do we have to add a call to
string_list_remove_duplicates()? It turns out that the list cannot
contain duplicates, but it took some digging to verify this.
Michael Haggerty (6):
prune_remote(): exit early if there are no stale references
prune_remote(): initialize both delete_refs lists in a single loop
prune_remote(): sort delete_refs_list references en masse
repack_without_refs(): make the refnames argument a string_list
prune_remote(): rename local variable
prune_remote(): iterate using for_each_string_list_item()
builtin/remote.c | 59 ++++++++++++++++++++++++++------------------------------
refs.c | 38 +++++++++++++++++++-----------------
refs.h | 11 ++++++++++-
3 files changed, 57 insertions(+), 51 deletions(-)
--
2.1.3
From: Michael Haggerty <hidden> Date: 2016-06-15 23:03:05
Also free them together at the end of the function.
In a moment, the array version will become redundant. Managing them
together makes later steps more obvious.
Signed-off-by: Michael Haggerty <redacted>
---
builtin/remote.c | 15 +++++++++------
1 file changed, 9 insertions(+), 6 deletions(-)
From: Michael Haggerty <hidden> Date: 2016-06-15 23:03:05
Iterate over refs_to_prune using for_each_string_list_item() rather
than writing out the loop in longhand.
Signed-off-by: Michael Haggerty <redacted>
---
builtin/remote.c | 14 ++++++--------
1 file changed, 6 insertions(+), 8 deletions(-)
@@ -1309,9 +1309,10 @@ static int set_head(int argc, const char **argv)staticintprune_remote(constchar*remote,intdry_run){-intresult=0,i;+intresult=0;structref_statesstates;structstring_listrefs_to_prune=STRING_LIST_INIT_NODUP;+structstring_list_item*item;constchar*dangling_msg=dry_run?_(" %s will become dangling!"):_(" %s has become dangling!");
@@ -1330,11 +1331,8 @@ static int prune_remote(const char *remote, int dry_run)?states.remote->url[0]:_("(no URL)"));-for(i=0;i<states.stale.nr;i++){-constchar*refname=states.stale.items[i].util;--string_list_append(&refs_to_prune,refname);-}+for_each_string_list_item(item,&states.stale)+string_list_append(&refs_to_prune,item->util);sort_string_list(&refs_to_prune);if(!dry_run){
@@ -1344,8 +1342,8 @@ static int prune_remote(const char *remote, int dry_run)strbuf_release(&err);}-for(i=0;i<states.stale.nr;i++){-constchar*refname=states.stale.items[i].util;+for_each_string_list_item(item,&states.stale){+constchar*refname=item->util;if(!dry_run)result|=delete_ref(refname,NULL,0);
From: Michael Haggerty <hidden> Date: 2016-06-15 23:03:05
Inserting items into a list in sorted order is O(N^2) whereas
appending them unsorted and then sorting the list all at once is
O(N lg N).
string_list_insert() also removes duplicates, and this change loses
that functionality. But the strings in this list, which ultimately
come from a for_each_ref() iteration, cannot contain duplicates.
Signed-off-by: Michael Haggerty <redacted>
---
builtin/remote.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
From: Michael Haggerty <hidden> Date: 2016-06-15 23:03:05
On 11/21/2014 03:09 PM, Michael Haggerty wrote:
This is basically an atomized version of Ronnie/Jonathan/Stefan's
patch [1] "refs.c: use a stringlist for repack_without_refs". But I've
actually rewritten most of it from scratch, using the original patch
as a reference.
Naturally, right after I emailed this series I realized that there have
been two more iterations on the original patch, which I overlooked
because I was not CCed on them. (I'm not complaining, just explaining.)
I don't think that those iterations changed anything substantial that
overlaps with my version, but TBH it's such a pain in the ass working
with patches in email that I don't think I'll go to the effort of
checking for sure unless somebody shows interest in actually using my
version.
Sorry for being grumpy today :-(
Michael
--
Michael Haggerty
mhagger@alum.mit.edu
From: Junio C Hamano <hidden> Date: 2016-06-15 23:03:05
On Fri, Nov 21, 2014 at 6:09 AM, Michael Haggerty [off-list ref] wrote:
Inserting items into a list in sorted order is O(N^2) whereas
appending them unsorted and then sorting the list all at once is
O(N lg N).
string_list_insert() also removes duplicates, and this change loses
that functionality. But the strings in this list, which ultimately
come from a for_each_ref() iteration, cannot contain duplicates.
A similar conversion in other places we may do in the future
might find a need for an equivalent to "-u" option of "sort" in the
string_list_sort() function, but the above nicely explains why
it is not necessary for this one. Good.
Eh, why is that called sort_string_list()? Perhaps it is a good
opening to introduce string_list_sort(list, flag) where flag would
be a bitmask that represents ignore-case, uniquify, etc., and
then either deprecate the current one or make it a thin wrapper
of the one that is more consistently named.
From: Jonathan Nieder <hidden> Date: 2016-06-15 23:03:05
Michael Haggerty wrote:
Aside from making the logic clearer, this avoids a call to
warn_dangling_symrefs(), which always does a for_each_rawref()
iteration.
Signed-off-by: Michael Haggerty <redacted>
---
builtin/remote.c | 39 +++++++++++++++++++++------------------
1 file changed, 21 insertions(+), 18 deletions(-)
I had been wondering about this but didn't chase it down far enough.
Thanks for noticing and cleaning it up.
Reviewed-by: Jonathan Nieder <redacted>
Reviewed-by: Jonathan Nieder <redacted>
One (optional) nit at the bottom of this message.
[...]
quoted hunk
+++ b/refs.c
@@ -2639,22 +2639,25 @@ static int curate_packed_ref_fn(struct ref_entry *entry, void *cb_data)return0;}-intrepack_without_refs(constchar**refnames,intn,structstrbuf*err)+intrepack_without_refs(structstring_list*refnames,structstrbuf*err){structref_dir*packed;structstring_listrefs_to_delete=STRING_LIST_INIT_DUP;-structstring_list_item*ref_to_delete;-inti,ret,removed=0;+structstring_list_item*refname,*ref_to_delete;+intret,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. :)
[...]
From: Michael Haggerty <hidden> Date: 2016-06-15 23:03:06
On 11/21/2014 05:44 PM, Junio C Hamano wrote:
On Fri, Nov 21, 2014 at 6:09 AM, Michael Haggerty [off-list ref] wrote:
quoted
Inserting items into a list in sorted order is O(N^2) whereas
appending them unsorted and then sorting the list all at once is
O(N lg N).
string_list_insert() also removes duplicates, and this change loses
that functionality. But the strings in this list, which ultimately
come from a for_each_ref() iteration, cannot contain duplicates.
A similar conversion in other places we may do in the future
might find a need for an equivalent to "-u" option of "sort" in the
string_list_sort() function, but the above nicely explains why
it is not necessary for this one. Good.
The only reason to integrate "-u" functionality into the sort would be
if one expects a significant fraction of entries to be duplicates, in
which case the sort could be structured to discard duplicates as it
works, thereby reducing the work needed for the sort. I can't think of
such a case in our code. Otherwise, calling sort_string_list() followed
by string_list_remove_duplicates() should be just as clear and
approximately as efficient.
A couple of times I've also felt that an all-purpose *stable* sort would
be convenient (though I can't remember the context offhand). I don't
think we have such a thing.
Eh, why is that called sort_string_list()? Perhaps it is a good
opening to introduce string_list_sort(list, flag) where flag would
be a bitmask that represents ignore-case, uniquify, etc., and
then either deprecate the current one or make it a thin wrapper
of the one that is more consistently named.
I agree. Indeed, I typed that function's name wrong once when
constructing this patch. It would be better to name it consistently with
the other string_list_*() functions.
I put it on my todo list (but don't let that dissuade somebody else from
doing it).
Michael
--
Michael Haggerty
mhagger@alum.mit.edu
Reviewed-by: Jonathan Nieder <redacted>
One (optional) nit at the bottom of this message.
[...]
quoted
+++ b/refs.c
@@ -2639,22 +2639,25 @@ static int curate_packed_ref_fn(struct ref_entry *entry, void *cb_data)return0;}-intrepack_without_refs(constchar**refnames,intn,structstrbuf*err)+intrepack_without_refs(structstring_list*refnames,structstrbuf*err){structref_dir*packed;structstring_listrefs_to_delete=STRING_LIST_INIT_DUP;-structstring_list_item*ref_to_delete;-inti,ret,removed=0;+structstring_list_item*refname,*ref_to_delete;+intret,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 :-)
From: Michael Haggerty <hidden> Date: 2016-06-15 23:03:06
On 11/25/2014 08:21 AM, Michael Haggerty wrote:
On 11/21/2014 05:44 PM, Junio C Hamano wrote:
quoted
[...]
Eh, why is that called sort_string_list()? Perhaps it is a good
opening to introduce string_list_sort(list, flag) where flag would
be a bitmask that represents ignore-case, uniquify, etc., and
then either deprecate the current one or make it a thin wrapper
of the one that is more consistently named.
I agree. Indeed, I typed that function's name wrong once when
constructing this patch. It would be better to name it consistently with
the other string_list_*() functions.
I put it on my todo list (but don't let that dissuade somebody else from
doing it).
Since I was re-rolling the patch series anyway, I tacked this renaming
change onto the end of it. Feel free to omit it if you think it belongs
separately.
Michael
--
Michael Haggerty
mhagger@alum.mit.edu