{"thread":{"id":"38003","subject":"[PATCH] refs.c: use a stringlist for repack_without_refs","startedAt":"2014-11-18T22:43:56Z","lastAt":"2014-12-04T02:03:40Z","messageCount":61,"participants":["Stefan Beller","Junio C Hamano","Jonathan Nieder","Ronnie Sahlberg","Michael Haggerty","Torsten Bögershausen","Matthieu Moy","Eric Wong","Philip Oakley","Marc Branchaud","brian m. carlson","Damien Robert"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"252140","messageId":"1416350636-12934-1-git-send-email-sbeller@google.com","threadId":"38003","inReplyTo":null,"subject":"[PATCH] refs.c: use a stringlist for repack_without_refs","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-11-18T22:43:56Z","receivedAt":"2014-11-18T22:43:56Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This patch was heavily inspired by a part of the ref-transactions-rename\nseries[1], but people tend to dislike large series and this part is\nrelatively easy to take out and unrelated, so I'll send it as a single\npatch.\n\nThis patch doesn't intend any functional changes. It is just a refactoring, \nwhich replaces a char** array by a stringlist in the function \nrepack_without_refs.\n\n[1] https://www.mail-archive.com/git@vger.kernel.org/msg60604.html\n\nIdea-by: Ronnie Sahlberg \nSigned-off-by: Stefan Beller \n---\n builtin/remote.c | 22 +++++++---------------\n refs.c | 41 ++++++++++++++++++++---------------------\n refs.h | 3 +--\n 3 files changed, 28 insertions(+), 38 deletions(-)\n\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex 7f28f92..dca4ebf 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -750,16 +750,11 @@ static int mv(int argc, const char **argv)\n static int remove_branches(struct string_list *branches)\n {\n \tstruct strbuf err = STRBUF_INIT;\n-\tconst char **branch_names;\n \tint i, result = 0;\n \n-\tbranch_names = xmalloc(branches->nr * sizeof(*branch_names));\n-\tfor (i = 0; i < branches->nr; i++)\n-\t\tbranch_names[i] = branches->items[i].string;\n-\tif (repack_without_refs(branch_names, branches->nr, &err))\n+\tif (repack_without_refs(branches, &err))\n \t\tresult |= error(\"%s\", err.buf);\n \tstrbuf_release(&err);\n-\tfree(branch_names);\n \n \tfor (i = 0; i < branches->nr; i++) {\n \t\tstruct string_list_item *item = branches->items + i;\n@@ -1317,7 +1312,6 @@ static int prune_remote(const char *remote, int dry_run)\n \tint result = 0, i;\n \tstruct ref_states states;\n \tstruct string_list delete_refs_list = STRING_LIST_INIT_NODUP;\n-\tconst char **delete_refs;\n \tconst char *dangling_msg = dry_run\n \t\t? _(\" %s will become dangling!\")\n \t\t: _(\" %s has become dangling!\");\n@@ -1325,6 +1319,11 @@ static int prune_remote(const char *remote, int dry_run)\n \tmemset(&states, 0, sizeof(states));\n \tget_remote_ref_states(remote, &states, GET_REF_STATES);\n \n+\tfor (i = 0; i < states.stale.nr; i++)\n+\t\tstring_list_insert(&delete_refs_list,\n+\t\t\t\t states.stale.items[i].util);\n+\n+\n \tif (states.stale.nr) {\n \t\tprintf_ln(_(\"Pruning %s\"), remote);\n \t\tprintf_ln(_(\"URL: %s\"),\n@@ -1332,24 +1331,17 @@ static int prune_remote(const char *remote, int dry_run)\n \t\t ? states.remote->url[0]\n \t\t : _(\"(no URL)\"));\n \n-\t\tdelete_refs = xmalloc(states.stale.nr * sizeof(*delete_refs));\n-\t\tfor (i = 0; i < states.stale.nr; i++)\n-\t\t\tdelete_refs[i] = states.stale.items[i].util;\n \t\tif (!dry_run) {\n \t\t\tstruct strbuf err = STRBUF_INIT;\n-\t\t\tif (repack_without_refs(delete_refs, states.stale.nr,\n-\t\t\t\t\t\t&err))\n+\t\t\tif (repack_without_refs(&delete_refs_list, &err))\n \t\t\t\tresult |= error(\"%s\", err.buf);\n \t\t\tstrbuf_release(&err);\n \t\t}\n-\t\tfree(delete_refs);\n \t}\n \n \tfor (i = 0; i < states.stale.nr; i++) {\n \t\tconst char *refname = states.stale.items[i].util;\n \n-\t\tstring_list_insert(&delete_refs_list, refname);\n-\n \t\tif (!dry_run)\n \t\t\tresult |= delete_ref(refname, NULL, 0);\n \ndiff --git a/refs.c b/refs.c\nindex 5ff457e..2333a9b 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2639,23 +2639,23 @@ static int curate_packed_ref_fn(struct ref_entry *entry, void *cb_data)\n \treturn 0;\n }\n \n-int repack_without_refs(const char **refnames, int n, struct strbuf *err)\n+int repack_without_refs(struct string_list *without, struct strbuf *err)\n {\n \tstruct ref_dir *packed;\n \tstruct string_list refs_to_delete = STRING_LIST_INIT_DUP;\n \tstruct string_list_item *ref_to_delete;\n-\tint i, ret, removed = 0;\n+\tint count, ret, removed = 0;\n \n \tassert(err);\n \n-\t/* Look for a packed ref */\n-\tfor (i = 0; i < n; i++)\n-\t\tif (get_packed_ref(refnames[i]))\n-\t\t\tbreak;\n+\tcount = 0;\n+\tfor_each_string_list_item(ref_to_delete, without)\n+\t\tif (get_packed_ref(ref_to_delete->string))\n+\t\t\tcount++;\n \n-\t/* Avoid locking if we have nothing to do */\n-\tif (i == n)\n-\t\treturn 0; /* no refname exists in packed refs */\n+\t/* No refname exists in packed refs */\n+\tif (!count)\n+\t\treturn 0;\n \n \tif (lock_packed_refs(0)) {\n \t\tunable_to_lock_message(git_path(\"packed-refs\"), errno, err);\n@@ -2664,8 +2664,8 @@ int repack_without_refs(const char **refnames, int n, struct strbuf *err)\n \tpacked = get_packed_refs(&ref_cache);\n \n \t/* Remove refnames from the cache */\n-\tfor (i = 0; i < n; i++)\n-\t\tif (remove_entry(packed, refnames[i]) != -1)\n+\tfor_each_string_list_item(ref_to_delete, without)\n+\t\tif (remove_entry(packed, ref_to_delete->string) != -1)\n \t\t\tremoved = 1;\n \tif (!removed) {\n \t\t/*\n@@ -3738,10 +3738,11 @@ static int ref_update_reject_duplicates(struct ref_update **updates, int n,\n int ref_transaction_commit(struct ref_transaction *transaction,\n \t\t\t struct strbuf *err)\n {\n-\tint ret = 0, delnum = 0, i;\n-\tconst char **delnames;\n+\tint ret = 0, i;\n \tint n = transaction->nr;\n \tstruct ref_update **updates = transaction->updates;\n+\tstruct string_list refs_to_delete = STRING_LIST_INIT_DUP;\n+\tstruct string_list_item *ref_to_delete;\n \n \tassert(err);\n \n@@ -3753,9 +3754,6 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \t\treturn 0;\n \t}\n \n-\t/* Allocate work space */\n-\tdelnames = xmalloc(sizeof(*delnames) * n);\n-\n \t/* Copy, sort, and reject duplicate refs */\n \tqsort(updates, n, sizeof(*updates), ref_update_compare);\n \tif (ref_update_reject_duplicates(updates, n, err)) {\n@@ -3815,16 +3813,17 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \t\t\t}\n \n \t\t\tif (!(update->flags & REF_ISPRUNING))\n-\t\t\t\tdelnames[delnum++] = update->lock->ref_name;\n+\t\t\t\tstring_list_insert(&refs_to_delete,\n+\t\t\t\t\t\t update->lock->ref_name);\n \t\t}\n \t}\n \n-\tif (repack_without_refs(delnames, delnum, err)) {\n+\tif (repack_without_refs(&refs_to_delete, err)) {\n \t\tret = TRANSACTION_GENERIC_ERROR;\n \t\tgoto cleanup;\n \t}\n-\tfor (i = 0; i < delnum; i++)\n-\t\tunlink_or_warn(git_path(\"logs/%s\", delnames[i]));\n+\tfor_each_string_list_item(ref_to_delete, &refs_to_delete)\n+\t\tunlink_or_warn(git_path(\"logs/%s\", ref_to_delete->string));\n \tclear_loose_ref_cache(&ref_cache);\n \n cleanup:\n@@ -3833,7 +3832,7 @@ cleanup:\n \tfor (i = 0; i < n; i++)\n \t\tif (updates[i]->lock)\n \t\t\tunlock_ref(updates[i]->lock);\n-\tfree(delnames);\n+\tstring_list_clear(&refs_to_delete, 0);\n \treturn ret;\n }\n \ndiff --git a/refs.h b/refs.h\nindex 2bc3556..0416e5f 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -163,8 +163,7 @@ extern void rollback_packed_refs(void);\n */\n int pack_refs(unsigned int flags);\n \n-extern int repack_without_refs(const char **refnames, int n,\n-\t\t\t struct strbuf *err);\n+extern int repack_without_refs(struct string_list *without, struct strbuf *err);\n \n extern int ref_exists(const char *);\n \n-- \n2.2.0.rc2.5.gf7b9fb2\n"},{"id":"252142","messageId":"xmqqlhn8t8g2.fsf@gitster.dls.corp.google.com","threadId":"38003","inReplyTo":"1416350636-12934-1-git-send-email-sbeller@google.com","subject":"Re: [PATCH] refs.c: use a stringlist for repack_without_refs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-11-18T23:06:37Z","receivedAt":"2014-11-18T23:06:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> This patch was heavily inspired by a part of the ref-transactions-rename\n> series[1], but people tend to dislike large series and this part is\n> relatively easy to take out and unrelated, so I'll send it as a single\n> patch.\n>\n> This patch doesn't intend any functional changes. It is just a refactoring, \n> which replaces a char** array by a stringlist in the function \n> repack_without_refs.\n>\n> [1] https://www.mail-archive.com/git@vger.kernel.org/msg60604.html\n>\n> Idea-by: Ronnie Sahlberg \n> Signed-off-by: Stefan Beller \n> ---\n> builtin/remote.c | 22 +++++++---------------\n> refs.c | 41 ++++++++++++++++++++---------------------\n> refs.h | 3 +--\n> 3 files changed, 28 insertions(+), 38 deletions(-)\n\nIn one codepath we were already using a string_list delete_refs_list\nanyway, so it makes sense to reuse that by movingan existing call to\nstring_list_insert() a bit higher, instead of maintaining another\narray of pointers delete_refs[] to strings.\n\nOK, it simplifies the code by reducing the line count, which is a\nplus ;-)\n\nSounds good.\n\n>\n> diff --git a/builtin/remote.c b/builtin/remote.c\n> index 7f28f92..dca4ebf 100644\n> --- a/builtin/remote.c\n> +++ b/builtin/remote.c\n> @@ -750,16 +750,11 @@ static int mv(int argc, const char **argv)\n> static int remove_branches(struct string_list *branches)\n> {\n> \tstruct strbuf err = STRBUF_INIT;\n> -\tconst char **branch_names;\n> \tint i, result = 0;\n> \n> -\tbranch_names = xmalloc(branches->nr * sizeof(*branch_names));\n> -\tfor (i = 0; i < branches->nr; i++)\n> -\t\tbranch_names[i] = branches->items[i].string;\n> -\tif (repack_without_refs(branch_names, branches->nr, &err))\n> +\tif (repack_without_refs(branches, &err))\n> \t\tresult |= error(\"%s\", err.buf);\n> \tstrbuf_release(&err);\n> -\tfree(branch_names);\n> \n> \tfor (i = 0; i < branches->nr; i++) {\n> \t\tstruct string_list_item *item = branches->items + i;\n> @@ -1317,7 +1312,6 @@ static int prune_remote(const char *remote, int dry_run)\n> \tint result = 0, i;\n> \tstruct ref_states states;\n> \tstruct string_list delete_refs_list = STRING_LIST_INIT_NODUP;\n> -\tconst char **delete_refs;\n> \tconst char *dangling_msg = dry_run\n> \t\t? _(\" %s will become dangling!\")\n> \t\t: _(\" %s has become dangling!\");\n> @@ -1325,6 +1319,11 @@ static int prune_remote(const char *remote, int dry_run)\n> \tmemset(&states, 0, sizeof(states));\n> \tget_remote_ref_states(remote, &states, GET_REF_STATES);\n> \n> +\tfor (i = 0; i < states.stale.nr; i++)\n> +\t\tstring_list_insert(&delete_refs_list,\n> +\t\t\t\t states.stale.items[i].util);\n> +\n> +\n> \tif (states.stale.nr) {\n> \t\tprintf_ln(_(\"Pruning %s\"), remote);\n> \t\tprintf_ln(_(\"URL: %s\"),\n> @@ -1332,24 +1331,17 @@ static int prune_remote(const char *remote, int dry_run)\n> \t\t ? states.remote->url[0]\n> \t\t : _(\"(no URL)\"));\n> \n> -\t\tdelete_refs = xmalloc(states.stale.nr * sizeof(*delete_refs));\n> -\t\tfor (i = 0; i < states.stale.nr; i++)\n> -\t\t\tdelete_refs[i] = states.stale.items[i].util;\n> \t\tif (!dry_run) {\n> \t\t\tstruct strbuf err = STRBUF_INIT;\n> -\t\t\tif (repack_without_refs(delete_refs, states.stale.nr,\n> -\t\t\t\t\t\t&err))\n> +\t\t\tif (repack_without_refs(&delete_refs_list, &err))\n> \t\t\t\tresult |= error(\"%s\", err.buf);\n> \t\t\tstrbuf_release(&err);\n> \t\t}\n> -\t\tfree(delete_refs);\n> \t}\n> \n> \tfor (i = 0; i < states.stale.nr; i++) {\n> \t\tconst char *refname = states.stale.items[i].util;\n> \n> -\t\tstring_list_insert(&delete_refs_list, refname);\n> -\n> \t\tif (!dry_run)\n> \t\t\tresult |= delete_ref(refname, NULL, 0);\n> \n> diff --git a/refs.c b/refs.c\n> index 5ff457e..2333a9b 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -2639,23 +2639,23 @@ static int curate_packed_ref_fn(struct ref_entry *entry, void *cb_data)\n> \treturn 0;\n> }\n> \n> -int repack_without_refs(const char **refnames, int n, struct strbuf *err)\n> +int repack_without_refs(struct string_list *without, struct strbuf *err)\n> {\n> \tstruct ref_dir *packed;\n> \tstruct string_list refs_to_delete = STRING_LIST_INIT_DUP;\n> \tstruct string_list_item *ref_to_delete;\n> -\tint i, ret, removed = 0;\n> +\tint count, ret, removed = 0;\n> \n> \tassert(err);\n> \n> -\t/* Look for a packed ref */\n> -\tfor (i = 0; i < n; i++)\n> -\t\tif (get_packed_ref(refnames[i]))\n> -\t\t\tbreak;\n> +\tcount = 0;\n> +\tfor_each_string_list_item(ref_to_delete, without)\n> +\t\tif (get_packed_ref(ref_to_delete->string))\n> +\t\t\tcount++;\n> \n> -\t/* Avoid locking if we have nothing to do */\n> -\tif (i == n)\n> -\t\treturn 0; /* no refname exists in packed refs */\n> +\t/* No refname exists in packed refs */\n> +\tif (!count)\n> +\t\treturn 0;\n> \n> \tif (lock_packed_refs(0)) {\n> \t\tunable_to_lock_message(git_path(\"packed-refs\"), errno, err);\n> @@ -2664,8 +2664,8 @@ int repack_without_refs(const char **refnames, int n, struct strbuf *err)\n> \tpacked = get_packed_refs(&ref_cache);\n> \n> \t/* Remove refnames from the cache */\n> -\tfor (i = 0; i < n; i++)\n> -\t\tif (remove_entry(packed, refnames[i]) != -1)\n> +\tfor_each_string_list_item(ref_to_delete, without)\n> +\t\tif (remove_entry(packed, ref_to_delete->string) != -1)\n> \t\t\tremoved = 1;\n> \tif (!removed) {\n> \t\t/*\n> @@ -3738,10 +3738,11 @@ static int ref_update_reject_duplicates(struct ref_update **updates, int n,\n> int ref_transaction_commit(struct ref_transaction *transaction,\n> \t\t\t struct strbuf *err)\n> {\n> -\tint ret = 0, delnum = 0, i;\n> -\tconst char **delnames;\n> +\tint ret = 0, i;\n> \tint n = transaction->nr;\n> \tstruct ref_update **updates = transaction->updates;\n> +\tstruct string_list refs_to_delete = STRING_LIST_INIT_DUP;\n> +\tstruct string_list_item *ref_to_delete;\n> \n> \tassert(err);\n> \n> @@ -3753,9 +3754,6 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n> \t\treturn 0;\n> \t}\n> \n> -\t/* Allocate work space */\n> -\tdelnames = xmalloc(sizeof(*delnames) * n);\n> -\n> \t/* Copy, sort, and reject duplicate refs */\n> \tqsort(updates, n, sizeof(*updates), ref_update_compare);\n> \tif (ref_update_reject_duplicates(updates, n, err)) {\n> @@ -3815,16 +3813,17 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n> \t\t\t}\n> \n> \t\t\tif (!(update->flags & REF_ISPRUNING))\n> -\t\t\t\tdelnames[delnum++] = update->lock->ref_name;\n> +\t\t\t\tstring_list_insert(&refs_to_delete,\n> +\t\t\t\t\t\t update->lock->ref_name);\n> \t\t}\n> \t}\n> \n> -\tif (repack_without_refs(delnames, delnum, err)) {\n> +\tif (repack_without_refs(&refs_to_delete, err)) {\n> \t\tret = TRANSACTION_GENERIC_ERROR;\n> \t\tgoto cleanup;\n> \t}\n> -\tfor (i = 0; i < delnum; i++)\n> -\t\tunlink_or_warn(git_path(\"logs/%s\", delnames[i]));\n> +\tfor_each_string_list_item(ref_to_delete, &refs_to_delete)\n> +\t\tunlink_or_warn(git_path(\"logs/%s\", ref_to_delete->string));\n> \tclear_loose_ref_cache(&ref_cache);\n> \n> cleanup:\n> @@ -3833,7 +3832,7 @@ cleanup:\n> \tfor (i = 0; i < n; i++)\n> \t\tif (updates[i]->lock)\n> \t\t\tunlock_ref(updates[i]->lock);\n> -\tfree(delnames);\n> +\tstring_list_clear(&refs_to_delete, 0);\n> \treturn ret;\n> }\n> \n> diff --git a/refs.h b/refs.h\n> index 2bc3556..0416e5f 100644\n> --- a/refs.h\n> +++ b/refs.h\n> @@ -163,8 +163,7 @@ extern void rollback_packed_refs(void);\n> */\n> int pack_refs(unsigned int flags);\n> \n> -extern int repack_without_refs(const char **refnames, int n,\n> -\t\t\t struct strbuf *err);\n> +extern int repack_without_refs(struct string_list *without, struct strbuf *err);\n> \n> extern int ref_exists(const char *);\n"},{"id":"252148","messageId":"xmqqh9xwt6wy.fsf@gitster.dls.corp.google.com","threadId":"38003","inReplyTo":"xmqqlhn8t8g2.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] refs.c: use a stringlist for repack_without_refs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-11-18T23:39:41Z","receivedAt":"2014-11-18T23:39:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Stefan Beller writes:\n>\n>> This patch was heavily inspired by a part of the ref-transactions-rename\n>> series[1], but people tend to dislike large series and this part is\n>> relatively easy to take out and unrelated, so I'll send it as a single\n>> patch.\n>>\n>> This patch doesn't intend any functional changes. It is just a refactoring, \n>> which replaces a char** array by a stringlist in the function \n>> repack_without_refs.\n>>\n>> [1] https://www.mail-archive.com/git@vger.kernel.org/msg60604.html\n>>\n>> Idea-by: Ronnie Sahlberg \n>> Signed-off-by: Stefan Beller \n>> ---\n>> builtin/remote.c | 22 +++++++---------------\n>> refs.c | 41 ++++++++++++++++++++---------------------\n>> refs.h | 3 +--\n>> 3 files changed, 28 insertions(+), 38 deletions(-)\n>\n> In one codepath we were already using a string_list delete_refs_list\n> anyway, so it makes sense to reuse that by movingan existing call to\n> string_list_insert() a bit higher, instead of maintaining another\n> array of pointers delete_refs[] to strings.\n>\n> OK, it simplifies the code by reducing the line count, which is a\n> plus ;-)\n>\n> Sounds good.\n\nI queued this but as I suspected yesterday had to drop all the other\nrs/ref-transaction-* topics that are not in 'next' yet. I am\nguessing that your plan is to make them come back one piece at a\ntime in many easier-to-digest bite sized series.\n"},{"id":"252149","messageId":"20141118234500.GO6527@google.com","threadId":"38003","inReplyTo":"1416350636-12934-1-git-send-email-sbeller@google.com","subject":"Re: [PATCH] refs.c: use a stringlist for repack_without_refs","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-11-18T23:45:00Z","receivedAt":"2014-11-18T23:45:00Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Stefan Beller wrote:\n\n> This patch was heavily inspired by a part of the ref-transactions-rename\n> series[1], but people tend to dislike large series and this part is\n> relatively easy to take out and unrelated, so I'll send it as a single\n> patch.\n>\n> [1] https://www.mail-archive.com/git@vger.kernel.org/msg60604.html\n\nThe above is a useful kind of comment to put below the three-dashes. It\ndoesn't explain what the intent behind the patch is, why I should want\nthis patch when considering whether to upgrade git, or what is going to\nbreak when I consider reverting it as part of fixing something else, so\nit doesn't belong in the commit message.\n\n> This patch doesn't intend any functional changes. It is just a refactoring, \n> which replaces a char** array by a stringlist in the function \n> repack_without_refs.\n\nThanks. Why, though? Is it about having something simpler to pass\nfrom builtin/remote.c::remove_branches(), or something else?\n\n> Idea-by: Ronnie Sahlberg \n> Signed-off-by: Stefan Beller \n\nIsn't the patch by Ronnie?\n\nSometimes I send a patch by someone else and make some change that I\ndon't want them to be blamed for. Then I keep their sign-off and put\na note in the commit message about the change I made. See output from\n\n git log origin/pu --grep='jc:'\n\nfor more examples of that.\n\nSome nits below.\n\n> --- a/builtin/remote.c\n> +++ b/builtin/remote.c\n[...]\n> @@ -1325,6 +1319,11 @@ static int prune_remote(const char *remote, int dry_run)\n[...]\n> \tmemset(&states, 0, sizeof(states));\n> \tget_remote_ref_states(remote, &states, GET_REF_STATES);\n> \n> +\tfor (i = 0; i < states.stale.nr; i++)\n> +\t\tstring_list_insert(&delete_refs_list,\n> +\t\t\t\t states.stale.items[i].util);\n> +\n> +\n> \tif (states.stale.nr) {\n\n(style) The double blank line looks odd here.\n\n> \t\tprintf_ln(_(\"Pruning %s\"), remote);\n> \t\tprintf_ln(_(\"URL: %s\"),\n> @@ -1332,24 +1331,17 @@ static int prune_remote(const char *remote, int dry_run)\n> \t\t ? states.remote->url[0]\n> \t\t : _(\"(no URL)\"));\n> \n> -\t\tdelete_refs = xmalloc(states.stale.nr * sizeof(*delete_refs));\n\nNow that there's no delete_refs array duplicating the string list,\nwould it make sense to rename delete_refs_list to delete_refs?\n\nAs a nice side-effect, that would make the definition of\ndelete_refs_list and other places it is used appear in the patch.\n\n> \tfor (i = 0; i < states.stale.nr; i++) {\n> \t\tconst char *refname = states.stale.items[i].util;\n\n(optional) this could be\n\n\tfor_each_string_list_item(ref, &delete_refs_list) {\n\t\tconst char *refname = ref->string;\n\t\t...\n\nwhich saves the reader from having to remember what states.stale.items\nmeans.\n\n[...]\n> +++ b/refs.c\n[...]\n> @@ -2639,23 +2639,23 @@ int repack_without_refs(struct string_list *without, struct strbuf *err)\n[...]\n> -\tint i, ret, removed = 0;\n> +\tint count, ret, removed = 0;\n> \n> \tassert(err);\n> \n> -\t/* Look for a packed ref */\n\nThe old code has comments marking sections of the function:\n\n\t/* Look for a packed ref */\n\t/* Avoid processing if we have nothing to do */\n\t/* Remove refnames from the cache */\n\t/* Remove any other accumulated cruft */\n\t/* Write what remains */\n\nIs dropping this comment intended?\n\n> -\tfor (i = 0; i < n; i++)\n> -\t\tif (get_packed_ref(refnames[i]))\n> -\t\t\tbreak;\n> +\tcount = 0;\n> +\tfor_each_string_list_item(ref_to_delete, without)\n> +\t\tif (get_packed_ref(ref_to_delete->string))\n> +\t\t\tcount++;\n\nThe old code breaks out early as soon as it finds a ref to delete.\nCan we do similar?\n\nE.g.\n\n\tfor (i = 0; i < without->nr; i++)\n\t\tif (get_packed_ref(without->items[i].string))\n\t\t\tbreak;\n\n(not about this patch) Is refs_to_delete leaked?\n\n[...]\n> @@ -3738,10 +3738,11 @@ static int ref_update_reject_duplicates(struct ref_update **updates, int n,\n> int ref_transaction_commit(struct ref_transaction *transaction,\n> \t\t\t struct strbuf *err)\n> {\n> -\tint ret = 0, delnum = 0, i;\n> -\tconst char **delnames;\n> +\tint ret = 0, i;\n> \tint n = transaction->nr;\n> \tstruct ref_update **updates = transaction->updates;\n> +\tstruct string_list refs_to_delete = STRING_LIST_INIT_DUP;\n\nThe old code doesn't xstrdup the list items, so _NODUP should work\nfine (and be slightly more efficient).\n\n[...]\n> @@ -3815,16 +3813,17 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n> \t\t\t}\n> \n> \t\t\tif (!(update->flags & REF_ISPRUNING))\n> -\t\t\t\tdelnames[delnum++] = update->lock->ref_name;\n> +\t\t\t\tstring_list_insert(&refs_to_delete,\n> +\t\t\t\t\t\t update->lock->ref_name);\n\nstring_list_append would be analagous to the old code.\n\n[....]\n> --- a/refs.h\n> +++ b/refs.h\n> @@ -163,8 +163,7 @@ extern void rollback_packed_refs(void);\n> */\n> int pack_refs(unsigned int flags);\n> \n> -extern int repack_without_refs(const char **refnames, int n,\n> -\t\t\t struct strbuf *err);\n> +extern int repack_without_refs(struct string_list *without, struct strbuf *err);\n\nA comment could mention whether the ref list needs to be sorted. (It\ndoesn't, right?)\n\nThanks and hope that helps,\nJonathan\n"},{"id":"252150","messageId":"CAGZ79kZgqRsiBFGZjmZtX3v37_DqNVqwwbhTmdHTYVGOv4=mbA@mail.gmail.com","threadId":"38003","inReplyTo":"20141118234500.GO6527@google.com","subject":"Re: [PATCH] refs.c: use a stringlist for repack_without_refs","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-11-19T00:28:04Z","receivedAt":"2014-11-19T00:28:04Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Nov 18, 2014 at 3:45 PM, Jonathan Nieder wrote:\n>\n> The above is a useful kind of comment to put below the three-dashes. It\n> doesn't explain what the intent behind the patch is, why I should want\n> this patch when considering whether to upgrade git, or what is going to\n> break when I consider reverting it as part of fixing something else, so\n> it doesn't belong in the commit message.\n\nYes, I'll do a resend, removing this paragraph.\n\n>\n>> This patch doesn't intend any functional changes. It is just a refactoring,\n>> which replaces a char** array by a stringlist in the function\n>> repack_without_refs.\n>\n> Thanks. Why, though? Is it about having something simpler to pass\n> from builtin/remote.c::remove_branches(), or something else?\n\nEssentially it's simpler to read and maintain as we're having less\nlines of code.\nI'll add that to the commit message instead.\n\n>\n>> Idea-by: Ronnie Sahlberg \n>> Signed-off-by: Stefan Beller \n>\n> Isn't the patch by Ronnie?\n\nAs it was part of the ref-transaction-rename series, it was authored by Ronnie.\nPorting it back to the master branch brought up so many conflicts,\nthat I decided\nto rewrite it from scratch while having an occasional look at the\noriginal patch.\n\nIf you want we can retain Ronnies authorship, however I may have messed up\nthe rewriting, so I put my name as author and Ronnie as giving the idea.\n\n>\n> Sometimes I send a patch by someone else and make some change that I\n> don't want them to be blamed for. Then I keep their sign-off and put\n> a note in the commit message about the change I made. See output from\n\nSounds reasonable, I can do something similar.\n\n>\n> git log origin/pu --grep='jc:'\n>\n> for more examples of that.\n>\n> Some nits below.\n\nBecause of the nits, I'd rather be blamed. :)\n\n>\n>> --- a/builtin/remote.c\n>> +++ b/builtin/remote.c\n> [...]\n>> @@ -1325,6 +1319,11 @@ static int prune_remote(const char *remote, int dry_run)\n> [...]\n>> memset(&states, 0, sizeof(states));\n>> get_remote_ref_states(remote, &states, GET_REF_STATES);\n>>\n>> + for (i = 0; i < states.stale.nr; i++)\n>> + string_list_insert(&delete_refs_list,\n>> + states.stale.items[i].util);\n>> +\n>> +\n>> if (states.stale.nr) {\n>\n> (style) The double blank line looks odd here.\n\nwill fix\n\n>\n>> printf_ln(_(\"Pruning %s\"), remote);\n>> printf_ln(_(\"URL: %s\"),\n>> @@ -1332,24 +1331,17 @@ static int prune_remote(const char *remote, int dry_run)\n>> ? states.remote->url[0]\n>> : _(\"(no URL)\"));\n>>\n>> - delete_refs = xmalloc(states.stale.nr * sizeof(*delete_refs));\n>\n> Now that there's no delete_refs array duplicating the string list,\n> would it make sense to rename delete_refs_list to delete_refs?\n>\n> As a nice side-effect, that would make the definition of\n> delete_refs_list and other places it is used appear in the patch.\n>\n>> for (i = 0; i < states.stale.nr; i++) {\n>> const char *refname = states.stale.items[i].util;\n>\n> (optional) this could be\n>\n> for_each_string_list_item(ref, &delete_refs_list) {\n> const char *refname = ref->string;\n> ...\n>\n> which saves the reader from having to remember what states.stale.items\n> means.\n\ndone\n\n>\n> [...]\n>> +++ b/refs.c\n> [...]\n>> @@ -2639,23 +2639,23 @@ int repack_without_refs(struct string_list *without, struct strbuf *err)\n> [...]\n>> - int i, ret, removed = 0;\n>> + int count, ret, removed = 0;\n>>\n>> assert(err);\n>>\n>> - /* Look for a packed ref */\n>\n> The old code has comments marking sections of the function:\n>\n> /* Look for a packed ref */\n> /* Avoid processing if we have nothing to do */\n> /* Remove refnames from the cache */\n> /* Remove any other accumulated cruft */\n> /* Write what remains */\n>\n> Is dropping this comment intended?\n\nno, dropped the dropping in the reroll.\n\n>\n>> - for (i = 0; i < n; i++)\n>> - if (get_packed_ref(refnames[i]))\n>> - break;\n>> + count = 0;\n>> + for_each_string_list_item(ref_to_delete, without)\n>> + if (get_packed_ref(ref_to_delete->string))\n>> + count++;\n>\n> The old code breaks out early as soon as it finds a ref to delete.\n> Can we do similar?\n\ndone\n\n>\n> E.g.\n>\n> for (i = 0; i < without->nr; i++)\n> if (get_packed_ref(without->items[i].string))\n> break;\n>\n> (not about this patch) Is refs_to_delete leaked?\n>\n> [...]\n>> @@ -3738,10 +3738,11 @@ static int ref_update_reject_duplicates(struct ref_update **updates, int n,\n>> int ref_transaction_commit(struct ref_transaction *transaction,\n>> struct strbuf *err)\n>> {\n>> - int ret = 0, delnum = 0, i;\n>> - const char **delnames;\n>> + int ret = 0, i;\n>> int n = transaction->nr;\n>> struct ref_update **updates = transaction->updates;\n>> + struct string_list refs_to_delete = STRING_LIST_INIT_DUP;\n>\n> The old code doesn't xstrdup the list items, so _NODUP should work\n> fine (and be slightly more efficient).\n\nok\n\n>\n> [...]\n>> @@ -3815,16 +3813,17 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n>> }\n>>\n>> if (!(update->flags & REF_ISPRUNING))\n>> - delnames[delnum++] = update->lock->ref_name;\n>> + string_list_insert(&refs_to_delete,\n>> + update->lock->ref_name);\n>\n> string_list_append would be analagous to the old code.\n\nok\n\n>\n> [....]\n>> --- a/refs.h\n>> +++ b/refs.h\n>> @@ -163,8 +163,7 @@ extern void rollback_packed_refs(void);\n>> */\n>> int pack_refs(unsigned int flags);\n>>\n>> -extern int repack_without_refs(const char **refnames, int n,\n>> - struct strbuf *err);\n>> +extern int repack_without_refs(struct string_list *without, struct strbuf *err);\n>\n> A comment could mention whether the ref list needs to be sorted. (It\n> doesn't, right?)\n\nok, I tried adding comments.\n\n>\n> Thanks and hope that helps,\n> Jonathan\n"},{"id":"252155","messageId":"1416359308-14831-1-git-send-email-sbeller@google.com","threadId":"38003","inReplyTo":"20141118234500.GO6527@google.com","subject":"[PATCH] refs.c: use a stringlist for repack_without_refs","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-11-19T01:08:28Z","receivedAt":"2014-11-19T01:08:28Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This patch doesn't intend any functional changes. It is just\na refactoring, which replaces a char** array by a stringlist\nin the function repack_without_refs.\nThis is easier to read and maintain as it delivers the same\nfunctionality with less lines of code less pointers.\n\nSigned-off-by: Ronnie Sahlberg \nSigned-off-by: Stefan Beller \n\n-\n\nThis patch was heavily inspired by a part of the ref-transactions-rename\nseries[1], but people tend to dislike large series and this part is\nrelatively easy to take out and unrelated, so I'll send it as a single\npatch.\n\n[1] https://www.mail-archive.com/git@vger.kernel.org/msg60604.html\n\n---\n builtin/remote.c | 31 +++++++++++--------------------\n refs.c | 40 +++++++++++++++++++++-------------------\n refs.h | 10 ++++++++--\n 3 files changed, 40 insertions(+), 41 deletions(-)\n\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex 7f28f92..5f5fa4c 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -750,16 +750,11 @@ static int mv(int argc, const char **argv)\n static int remove_branches(struct string_list *branches)\n {\n \tstruct strbuf err = STRBUF_INIT;\n-\tconst char **branch_names;\n \tint i, result = 0;\n \n-\tbranch_names = xmalloc(branches->nr * sizeof(*branch_names));\n-\tfor (i = 0; i < branches->nr; i++)\n-\t\tbranch_names[i] = branches->items[i].string;\n-\tif (repack_without_refs(branch_names, branches->nr, &err))\n+\tif (repack_without_refs(branches, &err))\n \t\tresult |= error(\"%s\", err.buf);\n \tstrbuf_release(&err);\n-\tfree(branch_names);\n \n \tfor (i = 0; i < branches->nr; i++) {\n \t\tstruct string_list_item *item = branches->items + i;\n@@ -1316,8 +1311,8 @@ static int prune_remote(const char *remote, int dry_run)\n {\n \tint result = 0, i;\n \tstruct ref_states states;\n-\tstruct string_list delete_refs_list = STRING_LIST_INIT_NODUP;\n-\tconst char **delete_refs;\n+\tstruct string_list delete_refs = STRING_LIST_INIT_NODUP;\n+\tstruct string_list_item *ref;\n \tconst char *dangling_msg = dry_run\n \t\t? _(\" %s will become dangling!\")\n \t\t: _(\" %s has become dangling!\");\n@@ -1325,6 +1320,9 @@ static int prune_remote(const char *remote, int dry_run)\n \tmemset(&states, 0, sizeof(states));\n \tget_remote_ref_states(remote, &states, GET_REF_STATES);\n \n+\tfor_each_string_list_item(ref, &delete_refs)\n+\t\tstring_list_append(&delete_refs, ref->string);\n+\n \tif (states.stale.nr) {\n \t\tprintf_ln(_(\"Pruning %s\"), remote);\n \t\tprintf_ln(_(\"URL: %s\"),\n@@ -1332,23 +1330,16 @@ static int prune_remote(const char *remote, int dry_run)\n \t\t ? states.remote->url[0]\n \t\t : _(\"(no URL)\"));\n \n-\t\tdelete_refs = xmalloc(states.stale.nr * sizeof(*delete_refs));\n-\t\tfor (i = 0; i < states.stale.nr; i++)\n-\t\t\tdelete_refs[i] = states.stale.items[i].util;\n \t\tif (!dry_run) {\n \t\t\tstruct strbuf err = STRBUF_INIT;\n-\t\t\tif (repack_without_refs(delete_refs, states.stale.nr,\n-\t\t\t\t\t\t&err))\n+\t\t\tif (repack_without_refs(&delete_refs, &err))\n \t\t\t\tresult |= error(\"%s\", err.buf);\n \t\t\tstrbuf_release(&err);\n \t\t}\n-\t\tfree(delete_refs);\n \t}\n \n-\tfor (i = 0; i < states.stale.nr; i++) {\n-\t\tconst char *refname = states.stale.items[i].util;\n-\n-\t\tstring_list_insert(&delete_refs_list, refname);\n+\tfor_each_string_list_item(ref, &delete_refs) {\n+\t\tconst char *refname = ref->string;\n \n \t\tif (!dry_run)\n \t\t\tresult |= delete_ref(refname, NULL, 0);\n@@ -1361,8 +1352,8 @@ static int prune_remote(const char *remote, int dry_run)\n \t\t\t abbrev_ref(refname, \"refs/remotes/\"));\n \t}\n \n-\twarn_dangling_symrefs(stdout, dangling_msg, &delete_refs_list);\n-\tstring_list_clear(&delete_refs_list, 0);\n+\twarn_dangling_symrefs(stdout, dangling_msg, &delete_refs);\n+\tstring_list_clear(&delete_refs, 0);\n \n \tfree_remote_ref_states(&states);\n \treturn result;\ndiff --git a/refs.c b/refs.c\nindex 5ff457e..2f6e08b 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2639,23 +2639,26 @@ static int curate_packed_ref_fn(struct ref_entry *entry, void *cb_data)\n \treturn 0;\n }\n \n-int repack_without_refs(const char **refnames, int n, struct strbuf *err)\n+int repack_without_refs(struct string_list *without, struct strbuf *err)\n {\n \tstruct ref_dir *packed;\n \tstruct string_list refs_to_delete = STRING_LIST_INIT_DUP;\n \tstruct string_list_item *ref_to_delete;\n-\tint i, ret, removed = 0;\n+\tint ret, needs_repacking = 0, removed = 0;\n \n \tassert(err);\n \n \t/* Look for a packed ref */\n-\tfor (i = 0; i < n; i++)\n-\t\tif (get_packed_ref(refnames[i]))\n+\tfor_each_string_list_item(ref_to_delete, without) {\n+\t\tif (get_packed_ref(ref_to_delete->string)) {\n+\t\t\tneeds_repacking = 1;\n \t\t\tbreak;\n+\t\t}\n+\t}\n \n-\t/* Avoid locking if we have nothing to do */\n-\tif (i == n)\n-\t\treturn 0; /* no refname exists in packed refs */\n+\t/* No refname exists in packed refs */\n+\tif (!needs_repacking)\n+\t\treturn 0;\n \n \tif (lock_packed_refs(0)) {\n \t\tunable_to_lock_message(git_path(\"packed-refs\"), errno, err);\n@@ -2664,8 +2667,8 @@ int repack_without_refs(const char **refnames, int n, struct strbuf *err)\n \tpacked = get_packed_refs(&ref_cache);\n \n \t/* Remove refnames from the cache */\n-\tfor (i = 0; i < n; i++)\n-\t\tif (remove_entry(packed, refnames[i]) != -1)\n+\tfor_each_string_list_item(ref_to_delete, without)\n+\t\tif (remove_entry(packed, ref_to_delete->string) != -1)\n \t\t\tremoved = 1;\n \tif (!removed) {\n \t\t/*\n@@ -3738,10 +3741,11 @@ static int ref_update_reject_duplicates(struct ref_update **updates, int n,\n int ref_transaction_commit(struct ref_transaction *transaction,\n \t\t\t struct strbuf *err)\n {\n-\tint ret = 0, delnum = 0, i;\n-\tconst char **delnames;\n+\tint ret = 0, i;\n \tint n = transaction->nr;\n \tstruct ref_update **updates = transaction->updates;\n+\tstruct string_list refs_to_delete = STRING_LIST_INIT_NODUP;\n+\tstruct string_list_item *ref_to_delete;\n \n \tassert(err);\n \n@@ -3753,9 +3757,6 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \t\treturn 0;\n \t}\n \n-\t/* Allocate work space */\n-\tdelnames = xmalloc(sizeof(*delnames) * n);\n-\n \t/* Copy, sort, and reject duplicate refs */\n \tqsort(updates, n, sizeof(*updates), ref_update_compare);\n \tif (ref_update_reject_duplicates(updates, n, err)) {\n@@ -3815,16 +3816,17 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \t\t\t}\n \n \t\t\tif (!(update->flags & REF_ISPRUNING))\n-\t\t\t\tdelnames[delnum++] = update->lock->ref_name;\n+\t\t\t\tstring_list_append(&refs_to_delete,\n+\t\t\t\t\t\t update->lock->ref_name);\n \t\t}\n \t}\n \n-\tif (repack_without_refs(delnames, delnum, err)) {\n+\tif (repack_without_refs(&refs_to_delete, err)) {\n \t\tret = TRANSACTION_GENERIC_ERROR;\n \t\tgoto cleanup;\n \t}\n-\tfor (i = 0; i < delnum; i++)\n-\t\tunlink_or_warn(git_path(\"logs/%s\", delnames[i]));\n+\tfor_each_string_list_item(ref_to_delete, &refs_to_delete)\n+\t\tunlink_or_warn(git_path(\"logs/%s\", ref_to_delete->string));\n \tclear_loose_ref_cache(&ref_cache);\n \n cleanup:\n@@ -3833,7 +3835,7 @@ cleanup:\n \tfor (i = 0; i < n; i++)\n \t\tif (updates[i]->lock)\n \t\t\tunlock_ref(updates[i]->lock);\n-\tfree(delnames);\n+\tstring_list_clear(&refs_to_delete, 0);\n \treturn ret;\n }\n \ndiff --git a/refs.h b/refs.h\nindex 2bc3556..69f88ef 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -163,8 +163,14 @@ extern void rollback_packed_refs(void);\n */\n int pack_refs(unsigned int flags);\n \n-extern int repack_without_refs(const char **refnames, int n,\n-\t\t\t struct strbuf *err);\n+/*\n+ * Repacks the refs pack file excluding the refs given\n+ * without: The refs to be excluded from the new refs pack file,\n+ * May be unsorted\n+ * err: String buffer, which will be used for reporting errors,\n+ * Must not be NULL\n+ */\n+extern int repack_without_refs(struct string_list *without, struct strbuf *err);\n \n extern int ref_exists(const char *);\n \n-- \n2.2.0.rc2.5.gf7b9fb2\n"},{"id":"252186","messageId":"xmqq4mtvt6jj.fsf@gitster.dls.corp.google.com","threadId":"38003","inReplyTo":"1416359308-14831-1-git-send-email-sbeller@google.com","subject":"Re: [PATCH] refs.c: use a stringlist for repack_without_refs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-11-19T18:00:00Z","receivedAt":"2014-11-19T18:00:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> This patch doesn't intend any functional changes. It is just\n> a refactoring, which replaces a char** array by a stringlist\n> in the function repack_without_refs.\n> This is easier to read and maintain as it delivers the same\n> functionality with less lines of code less pointers.\n>\n> Signed-off-by: Ronnie Sahlberg \n> Signed-off-by: Stefan Beller \n>\n> -\n\nHave three of them, not just one, here. (no need to resend to fix\nonly this). Or...\n\n>\n> This patch was heavily inspired by a part of the ref-transactions-rename\n> series[1], but people tend to dislike large series and this part is\n> relatively easy to take out and unrelated, so I'll send it as a single\n> patch.\n>\n> [1] https://www.mail-archive.com/git@vger.kernel.org/msg60604.html\n>\n> ---\n\n... next time, write the comments here, where Git already gives you\nthree dashes.\n\nAlso mention what you updated and why relative to your earlier round\nhere, if not covered in the log message already.\n\nFor example, renaming of delete_refs_list (in v1) to delete_refs\n(this version) is a sensible change because readers know it is a\nlist from its type being string_list already, but that change is new\nrelative to the codebase, so it could go to the log message (\"Having\narray delete_refs[] and string_list delete_refs_list is redundant;\ndrop the array and give the string_list variable the shorter name\",\nor something like that) if you wanted to.\n\n> @@ -1316,8 +1311,8 @@ static int prune_remote(const char *remote, int dry_run)\n> {\n> \tint result = 0, i;\n> \tstruct ref_states states;\n> -\tstruct string_list delete_refs_list = STRING_LIST_INIT_NODUP;\n> -\tconst char **delete_refs;\n> +\tstruct string_list delete_refs = STRING_LIST_INIT_NODUP;\n> +\tstruct string_list_item *ref;\n> \tconst char *dangling_msg = dry_run\n> \t\t? _(\" %s will become dangling!\")\n> \t\t: _(\" %s has become dangling!\");\n> @@ -1325,6 +1320,9 @@ static int prune_remote(const char *remote, int dry_run)\n> \tmemset(&states, 0, sizeof(states));\n> \tget_remote_ref_states(remote, &states, GET_REF_STATES);\n> \n> +\tfor_each_string_list_item(ref, &delete_refs)\n> +\t\tstring_list_append(&delete_refs, ref->string);\n\nWhat are you trying to do here?\n\nInitialise delete_refs to an empty string list, and then iterate\nover its elements and append them into the same string list???\n\nIt looks like a \"currently noop, waiting for somebody to throw an\nitem to the list before this code, at which time it turns into an\ninfinite memory eater\".\n\nCurious...\n"},{"id":"252192","messageId":"1416423000-4323-1-git-send-email-sbeller@google.com","threadId":"38003","inReplyTo":"xmqq4mtvt6jj.fsf@gitster.dls.corp.google.com","subject":"[PATCH] refs.c: use a stringlist for repack_without_refs","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-11-19T18:50:00Z","receivedAt":"2014-11-19T18:50:00Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"From: Ronnie Sahlberg \n\nThis patch doesn't intend any functional changes. It is just\na refactoring, which replaces a char** array by a stringlist\nin the function repack_without_refs.\nThis is easier to read and maintain as it delivers the same\nfunctionality with less lines of code and less pointers.\n\n[sb: ported this patch from a larger patch series to the master branch,\nadded documentary comments in refs.h]\n\nSigned-off-by: Ronnie Sahlberg \nSigned-off-by: Stefan Beller \n---\n\nOn Wed, Nov 19, 2014 at 10:00 AM, Junio C Hamano wrote:\n> + for_each_string_list_item(ref, &delete_refs)\n> + string_list_append(&delete_refs, ref->string);\n> What are you trying to do here?\n\nI messed up this patch completely yesterday in the evening.\nEssentially the inter-patch diff are all the nits by Jonathan.\nSo here is my attempt on sending a more maintainer friendly patch.\n\nChanges to version 1:\n * removed the double blank line\n * rename delete_refs_list to delete_refs\n * add back comments dropped by accident\n * use STRING_LIST_INIT_NODUP instead of the _DUP version in ref_transaction_commit\n * add documentary comments on the repack_without_refs function\n * user string_list_append instead of string_list_insert as it follows the previous\n behavior more closely.\n * put back the early exit of the loop in repack_without_refs\n \nChanges to version 2:\n * fixed commit message (comments after the three dashes)\n * fixed the curiosity Junio pointed out as it was just wrong code.\n Now it actually builds a list of all states.stale.items[i].util items.\n\n\n builtin/remote.c | 31 +++++++++++--------------------\n refs.c | 40 +++++++++++++++++++++-------------------\n refs.h | 10 ++++++++--\n 3 files changed, 40 insertions(+), 41 deletions(-)\n\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex 7f28f92..0d89aba 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -750,16 +750,11 @@ static int mv(int argc, const char **argv)\n static int remove_branches(struct string_list *branches)\n {\n \tstruct strbuf err = STRBUF_INIT;\n-\tconst char **branch_names;\n \tint i, result = 0;\n \n-\tbranch_names = xmalloc(branches->nr * sizeof(*branch_names));\n-\tfor (i = 0; i < branches->nr; i++)\n-\t\tbranch_names[i] = branches->items[i].string;\n-\tif (repack_without_refs(branch_names, branches->nr, &err))\n+\tif (repack_without_refs(branches, &err))\n \t\tresult |= error(\"%s\", err.buf);\n \tstrbuf_release(&err);\n-\tfree(branch_names);\n \n \tfor (i = 0; i < branches->nr; i++) {\n \t\tstruct string_list_item *item = branches->items + i;\n@@ -1316,8 +1311,8 @@ static int prune_remote(const char *remote, int dry_run)\n {\n \tint result = 0, i;\n \tstruct ref_states states;\n-\tstruct string_list delete_refs_list = STRING_LIST_INIT_NODUP;\n-\tconst char **delete_refs;\n+\tstruct string_list delete_refs = STRING_LIST_INIT_NODUP;\n+\tstruct string_list_item *ref;\n \tconst char *dangling_msg = dry_run\n \t\t? _(\" %s will become dangling!\")\n \t\t: _(\" %s has become dangling!\");\n@@ -1325,6 +1320,9 @@ static int prune_remote(const char *remote, int dry_run)\n \tmemset(&states, 0, sizeof(states));\n \tget_remote_ref_states(remote, &states, GET_REF_STATES);\n \n+\tfor (i = 0; i < states.stale.nr; i++)\n+\t\tstring_list_append(&delete_refs, states.stale.items[i].util);\n+\n \tif (states.stale.nr) {\n \t\tprintf_ln(_(\"Pruning %s\"), remote);\n \t\tprintf_ln(_(\"URL: %s\"),\n@@ -1332,23 +1330,16 @@ static int prune_remote(const char *remote, int dry_run)\n \t\t ? states.remote->url[0]\n \t\t : _(\"(no URL)\"));\n \n-\t\tdelete_refs = xmalloc(states.stale.nr * sizeof(*delete_refs));\n-\t\tfor (i = 0; i < states.stale.nr; i++)\n-\t\t\tdelete_refs[i] = states.stale.items[i].util;\n \t\tif (!dry_run) {\n \t\t\tstruct strbuf err = STRBUF_INIT;\n-\t\t\tif (repack_without_refs(delete_refs, states.stale.nr,\n-\t\t\t\t\t\t&err))\n+\t\t\tif (repack_without_refs(&delete_refs, &err))\n \t\t\t\tresult |= error(\"%s\", err.buf);\n \t\t\tstrbuf_release(&err);\n \t\t}\n-\t\tfree(delete_refs);\n \t}\n \n-\tfor (i = 0; i < states.stale.nr; i++) {\n-\t\tconst char *refname = states.stale.items[i].util;\n-\n-\t\tstring_list_insert(&delete_refs_list, refname);\n+\tfor_each_string_list_item(ref, &delete_refs) {\n+\t\tconst char *refname = ref->string;\n \n \t\tif (!dry_run)\n \t\t\tresult |= delete_ref(refname, NULL, 0);\n@@ -1361,8 +1352,8 @@ static int prune_remote(const char *remote, int dry_run)\n \t\t\t abbrev_ref(refname, \"refs/remotes/\"));\n \t}\n \n-\twarn_dangling_symrefs(stdout, dangling_msg, &delete_refs_list);\n-\tstring_list_clear(&delete_refs_list, 0);\n+\twarn_dangling_symrefs(stdout, dangling_msg, &delete_refs);\n+\tstring_list_clear(&delete_refs, 0);\n \n \tfree_remote_ref_states(&states);\n \treturn result;\ndiff --git a/refs.c b/refs.c\nindex 5ff457e..2f6e08b 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2639,23 +2639,26 @@ static int curate_packed_ref_fn(struct ref_entry *entry, void *cb_data)\n \treturn 0;\n }\n \n-int repack_without_refs(const char **refnames, int n, struct strbuf *err)\n+int repack_without_refs(struct string_list *without, struct strbuf *err)\n {\n \tstruct ref_dir *packed;\n \tstruct string_list refs_to_delete = STRING_LIST_INIT_DUP;\n \tstruct string_list_item *ref_to_delete;\n-\tint i, ret, removed = 0;\n+\tint ret, needs_repacking = 0, removed = 0;\n \n \tassert(err);\n \n \t/* Look for a packed ref */\n-\tfor (i = 0; i < n; i++)\n-\t\tif (get_packed_ref(refnames[i]))\n+\tfor_each_string_list_item(ref_to_delete, without) {\n+\t\tif (get_packed_ref(ref_to_delete->string)) {\n+\t\t\tneeds_repacking = 1;\n \t\t\tbreak;\n+\t\t}\n+\t}\n \n-\t/* Avoid locking if we have nothing to do */\n-\tif (i == n)\n-\t\treturn 0; /* no refname exists in packed refs */\n+\t/* No refname exists in packed refs */\n+\tif (!needs_repacking)\n+\t\treturn 0;\n \n \tif (lock_packed_refs(0)) {\n \t\tunable_to_lock_message(git_path(\"packed-refs\"), errno, err);\n@@ -2664,8 +2667,8 @@ int repack_without_refs(const char **refnames, int n, struct strbuf *err)\n \tpacked = get_packed_refs(&ref_cache);\n \n \t/* Remove refnames from the cache */\n-\tfor (i = 0; i < n; i++)\n-\t\tif (remove_entry(packed, refnames[i]) != -1)\n+\tfor_each_string_list_item(ref_to_delete, without)\n+\t\tif (remove_entry(packed, ref_to_delete->string) != -1)\n \t\t\tremoved = 1;\n \tif (!removed) {\n \t\t/*\n@@ -3738,10 +3741,11 @@ static int ref_update_reject_duplicates(struct ref_update **updates, int n,\n int ref_transaction_commit(struct ref_transaction *transaction,\n \t\t\t struct strbuf *err)\n {\n-\tint ret = 0, delnum = 0, i;\n-\tconst char **delnames;\n+\tint ret = 0, i;\n \tint n = transaction->nr;\n \tstruct ref_update **updates = transaction->updates;\n+\tstruct string_list refs_to_delete = STRING_LIST_INIT_NODUP;\n+\tstruct string_list_item *ref_to_delete;\n \n \tassert(err);\n \n@@ -3753,9 +3757,6 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \t\treturn 0;\n \t}\n \n-\t/* Allocate work space */\n-\tdelnames = xmalloc(sizeof(*delnames) * n);\n-\n \t/* Copy, sort, and reject duplicate refs */\n \tqsort(updates, n, sizeof(*updates), ref_update_compare);\n \tif (ref_update_reject_duplicates(updates, n, err)) {\n@@ -3815,16 +3816,17 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \t\t\t}\n \n \t\t\tif (!(update->flags & REF_ISPRUNING))\n-\t\t\t\tdelnames[delnum++] = update->lock->ref_name;\n+\t\t\t\tstring_list_append(&refs_to_delete,\n+\t\t\t\t\t\t update->lock->ref_name);\n \t\t}\n \t}\n \n-\tif (repack_without_refs(delnames, delnum, err)) {\n+\tif (repack_without_refs(&refs_to_delete, err)) {\n \t\tret = TRANSACTION_GENERIC_ERROR;\n \t\tgoto cleanup;\n \t}\n-\tfor (i = 0; i < delnum; i++)\n-\t\tunlink_or_warn(git_path(\"logs/%s\", delnames[i]));\n+\tfor_each_string_list_item(ref_to_delete, &refs_to_delete)\n+\t\tunlink_or_warn(git_path(\"logs/%s\", ref_to_delete->string));\n \tclear_loose_ref_cache(&ref_cache);\n \n cleanup:\n@@ -3833,7 +3835,7 @@ cleanup:\n \tfor (i = 0; i < n; i++)\n \t\tif (updates[i]->lock)\n \t\t\tunlock_ref(updates[i]->lock);\n-\tfree(delnames);\n+\tstring_list_clear(&refs_to_delete, 0);\n \treturn ret;\n }\n \ndiff --git a/refs.h b/refs.h\nindex 2bc3556..69f88ef 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -163,8 +163,14 @@ extern void rollback_packed_refs(void);\n */\n int pack_refs(unsigned int flags);\n \n-extern int repack_without_refs(const char **refnames, int n,\n-\t\t\t struct strbuf *err);\n+/*\n+ * Repacks the refs pack file excluding the refs given\n+ * without: The refs to be excluded from the new refs pack file,\n+ * May be unsorted\n+ * err: String buffer, which will be used for reporting errors,\n+ * Must not be NULL\n+ */\n+extern int repack_without_refs(struct string_list *without, struct strbuf *err);\n \n extern int ref_exists(const char *);\n \n-- \n2.2.0.rc2.13.g0786cdb\n"},{"id":"252203","messageId":"20141119204450.GX6527@google.com","threadId":"38003","inReplyTo":"1416423000-4323-1-git-send-email-sbeller@google.com","subject":"Re: [PATCH] refs.c: use a stringlist for repack_without_refs","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-11-19T20:44:50Z","receivedAt":"2014-11-19T20:44:50Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Stefan Beller wrote:\n\n> This patch doesn't intend any functional changes.\n\nYay. :)\n\n> a refactoring, which replaces a char** array by a stringlist\n> in the function repack_without_refs.\n> This is easier to read and maintain as it delivers the same\n> functionality with less lines of code and less pointers.\n\nPlease wrap to a consistent width and add a blank line between\nparagraphs. So, either:\n\n\t... repack_without_refs. This is easier to read and ...\n\nor:\n\n\t... repack_without_refs.\n\n\tThis is easier to read and ...\n\n[...]\n> +++ b/builtin/remote.c\n> @@ -750,16 +750,11 @@ static int mv(int argc, const char **argv)\n[...]\n> @@ -1325,6 +1320,9 @@ static int prune_remote(const char *remote, int dry_run)\n> \tmemset(&states, 0, sizeof(states));\n> \tget_remote_ref_states(remote, &states, GET_REF_STATES);\n> \n> +\tfor (i = 0; i < states.stale.nr; i++)\n> +\t\tstring_list_append(&delete_refs, states.stale.items[i].util);\n\nwarn_dangling_symref requires a sorted list. Possible fixes:\n\n (a) switch to string_list_insert, or\n (b) [nicer] call sort_string_list before the warn_dangling_symrefs\n call.\n\n[...]\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -2639,23 +2639,26 @@ static int curate_packed_ref_fn(struct ref_entry *entry, void *cb_data)\n> \treturn 0;\n> }\n> \n> -int repack_without_refs(const char **refnames, int n, struct strbuf *err)\n> +int repack_without_refs(struct string_list *without, struct strbuf *err)\n> {\n> \tstruct ref_dir *packed;\n> \tstruct string_list refs_to_delete = STRING_LIST_INIT_DUP;\n> \tstruct string_list_item *ref_to_delete;\n> -\tint i, ret, removed = 0;\n> +\tint ret, needs_repacking = 0, removed = 0;\n> \n> \tassert(err);\n> \n> \t/* Look for a packed ref */\n> -\tfor (i = 0; i < n; i++)\n> -\t\tif (get_packed_ref(refnames[i]))\n> +\tfor_each_string_list_item(ref_to_delete, without) {\n> +\t\tif (get_packed_ref(ref_to_delete->string)) {\n> +\t\t\tneeds_repacking = 1;\n> \t\t\tbreak;\n> +\t\t}\n> +\t}\n> \n> -\t/* Avoid locking if we have nothing to do */\n\nThis comment was helpful --- it's sad to lose it (but if you feel\nstrongly about it then I don't mind).\n\n> -\tif (i == n)\n> -\t\treturn 0; /* no refname exists in packed refs */\n> +\t/* No refname exists in packed refs */\n> +\tif (!needs_repacking)\n> +\t\treturn 0;\n\nI kind of liked the 'i == n' test that avoided needing a new auxiliary\nvariable. This is fine and probably a little clearer, though.\n\n[...]\n> --- a/refs.h\n> +++ b/refs.h\n> @@ -163,8 +163,14 @@ extern void rollback_packed_refs(void);\n> */\n> int pack_refs(unsigned int flags);\n> \n> -extern int repack_without_refs(const char **refnames, int n,\n> -\t\t\t struct strbuf *err);\n> +/*\n> + * Repacks the refs pack file excluding the refs given\n> + * without: The refs to be excluded from the new refs pack file,\n> + * May be unsorted\n> + * err: String buffer, which will be used for reporting errors,\n> + * Must not be NULL\n> + */\n> +extern int repack_without_refs(struct string_list *without, struct strbuf *err);\n\n(nit) Other comments in this file use the imperative mood to describe\nwhat a function does, so it would be a little clearer to do that here,\ntoo (\"Repack the ...\" instead of \"Repacks the ...\").\n\nIt might be just me, but I find this formatted comment with everything\njammed together hard to read. I'd prefer a simple paragraph, like:\n\n\t/*\n\t * Remove the refs listed in 'without' from the packed-refs file.\n\t * On error, packed-refs will be unchanged, the return value is\n\t * nonzero, and a message about the error is written to the 'err'\n\t * strbuf.\n\t */\n\nThanks,\nJonathan\n"},{"id":"252217","messageId":"1416434088-1472-1-git-send-email-sbeller@google.com","threadId":"38003","inReplyTo":"20141119204450.GX6527@google.com","subject":"[PATCH] refs.c: use a stringlist for repack_without_refs","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-11-19T21:54:48Z","receivedAt":"2014-11-19T21:54:48Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"From: Ronnie Sahlberg \n\nThis patch doesn't intend any functional changes. It is just\na refactoring, which replaces a char** array by a stringlist\nin the function repack_without_refs.\nThis is easier to read and maintain as it delivers the same\nfunctionality with less lines of code and less pointers.\n\n[sb: ported this patch from a larger patch series to the master branch,\nadded documentary comments in refs.h]\n\nSigned-off-by: Ronnie Sahlberg \nSigned-off-by: Stefan Beller \n---\n\nversion 3 includes all nits by Jonathan.\n\nChanges to version 1:\n * removed the double blank line\n * rename delete_refs_list to delete_refs\n * add back comments dropped by accident\n * use STRING_LIST_INIT_NODUP instead of the _DUP version in ref_transaction_commit\n * add documentary comments on the repack_without_refs function\n * user string_list_append instead of string_list_insert as it follows the previous\n behavior more closely.\n * put back the early exit of the loop in repack_without_refs\n \nChanges to version 2:\n * fixed commit message (comments after the three dashes)\n * fixed the curiosity Junio pointed out as it was just wrong code.\n Now it actually builds a list of all states.stale.items[i].util items.\n \nChanges in version 3:\n\n * reword commit message\n * sort delete_refs before passing it to warn_dangling_symrefs\n * change the comments (get back the one jrn complained about) \n in repack_without_refs\n * use the suggestion of jonathan for documenting repack_without_refs in the\n header. Add a note about the arguments.\n\n---\n builtin/remote.c | 32 ++++++++++++--------------------\n refs.c | 38 ++++++++++++++++++++------------------\n refs.h | 10 ++++++++--\n 3 files changed, 40 insertions(+), 40 deletions(-)\n\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex 7f28f92..b37ed3d 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -750,16 +750,11 @@ static int mv(int argc, const char **argv)\n static int remove_branches(struct string_list *branches)\n {\n \tstruct strbuf err = STRBUF_INIT;\n-\tconst char **branch_names;\n \tint i, result = 0;\n \n-\tbranch_names = xmalloc(branches->nr * sizeof(*branch_names));\n-\tfor (i = 0; i < branches->nr; i++)\n-\t\tbranch_names[i] = branches->items[i].string;\n-\tif (repack_without_refs(branch_names, branches->nr, &err))\n+\tif (repack_without_refs(branches, &err))\n \t\tresult |= error(\"%s\", err.buf);\n \tstrbuf_release(&err);\n-\tfree(branch_names);\n \n \tfor (i = 0; i < branches->nr; i++) {\n \t\tstruct string_list_item *item = branches->items + i;\n@@ -1316,8 +1311,8 @@ static int prune_remote(const char *remote, int dry_run)\n {\n \tint result = 0, i;\n \tstruct ref_states states;\n-\tstruct string_list delete_refs_list = STRING_LIST_INIT_NODUP;\n-\tconst char **delete_refs;\n+\tstruct string_list delete_refs = STRING_LIST_INIT_NODUP;\n+\tstruct string_list_item *ref;\n \tconst char *dangling_msg = dry_run\n \t\t? _(\" %s will become dangling!\")\n \t\t: _(\" %s has become dangling!\");\n@@ -1325,6 +1320,9 @@ static int prune_remote(const char *remote, int dry_run)\n \tmemset(&states, 0, sizeof(states));\n \tget_remote_ref_states(remote, &states, GET_REF_STATES);\n \n+\tfor (i = 0; i < states.stale.nr; i++)\n+\t\tstring_list_append(&delete_refs, states.stale.items[i].util);\n+\n \tif (states.stale.nr) {\n \t\tprintf_ln(_(\"Pruning %s\"), remote);\n \t\tprintf_ln(_(\"URL: %s\"),\n@@ -1332,23 +1330,16 @@ static int prune_remote(const char *remote, int dry_run)\n \t\t ? states.remote->url[0]\n \t\t : _(\"(no URL)\"));\n \n-\t\tdelete_refs = xmalloc(states.stale.nr * sizeof(*delete_refs));\n-\t\tfor (i = 0; i < states.stale.nr; i++)\n-\t\t\tdelete_refs[i] = states.stale.items[i].util;\n \t\tif (!dry_run) {\n \t\t\tstruct strbuf err = STRBUF_INIT;\n-\t\t\tif (repack_without_refs(delete_refs, states.stale.nr,\n-\t\t\t\t\t\t&err))\n+\t\t\tif (repack_without_refs(&delete_refs, &err))\n \t\t\t\tresult |= error(\"%s\", err.buf);\n \t\t\tstrbuf_release(&err);\n \t\t}\n-\t\tfree(delete_refs);\n \t}\n \n-\tfor (i = 0; i < states.stale.nr; i++) {\n-\t\tconst char *refname = states.stale.items[i].util;\n-\n-\t\tstring_list_insert(&delete_refs_list, refname);\n+\tfor_each_string_list_item(ref, &delete_refs) {\n+\t\tconst char *refname = ref->string;\n \n \t\tif (!dry_run)\n \t\t\tresult |= delete_ref(refname, NULL, 0);\n@@ -1361,8 +1352,9 @@ static int prune_remote(const char *remote, int dry_run)\n \t\t\t abbrev_ref(refname, \"refs/remotes/\"));\n \t}\n \n-\twarn_dangling_symrefs(stdout, dangling_msg, &delete_refs_list);\n-\tstring_list_clear(&delete_refs_list, 0);\n+\tsort_string_list(&delete_refs);\n+\twarn_dangling_symrefs(stdout, dangling_msg, &delete_refs);\n+\tstring_list_clear(&delete_refs, 0);\n \n \tfree_remote_ref_states(&states);\n \treturn result;\ndiff --git a/refs.c b/refs.c\nindex 5ff457e..ebcd90f 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2639,23 +2639,26 @@ static int curate_packed_ref_fn(struct ref_entry *entry, void *cb_data)\n \treturn 0;\n }\n \n-int repack_without_refs(const char **refnames, int n, struct strbuf *err)\n+int repack_without_refs(struct string_list *without, struct strbuf *err)\n {\n \tstruct ref_dir *packed;\n \tstruct string_list refs_to_delete = STRING_LIST_INIT_DUP;\n \tstruct string_list_item *ref_to_delete;\n-\tint i, ret, removed = 0;\n+\tint ret, needs_repacking = 0, removed = 0;\n \n \tassert(err);\n \n \t/* Look for a packed ref */\n-\tfor (i = 0; i < n; i++)\n-\t\tif (get_packed_ref(refnames[i]))\n+\tfor_each_string_list_item(ref_to_delete, without) {\n+\t\tif (get_packed_ref(ref_to_delete->string)) {\n+\t\t\tneeds_repacking = 1;\n \t\t\tbreak;\n+\t\t}\n+\t}\n \n \t/* Avoid locking if we have nothing to do */\n-\tif (i == n)\n-\t\treturn 0; /* no refname exists in packed refs */\n+\tif (!needs_repacking)\n+\t\treturn 0;\n \n \tif (lock_packed_refs(0)) {\n \t\tunable_to_lock_message(git_path(\"packed-refs\"), errno, err);\n@@ -2664,8 +2667,8 @@ int repack_without_refs(const char **refnames, int n, struct strbuf *err)\n \tpacked = get_packed_refs(&ref_cache);\n \n \t/* Remove refnames from the cache */\n-\tfor (i = 0; i < n; i++)\n-\t\tif (remove_entry(packed, refnames[i]) != -1)\n+\tfor_each_string_list_item(ref_to_delete, without)\n+\t\tif (remove_entry(packed, ref_to_delete->string) != -1)\n \t\t\tremoved = 1;\n \tif (!removed) {\n \t\t/*\n@@ -3738,10 +3741,11 @@ static int ref_update_reject_duplicates(struct ref_update **updates, int n,\n int ref_transaction_commit(struct ref_transaction *transaction,\n \t\t\t struct strbuf *err)\n {\n-\tint ret = 0, delnum = 0, i;\n-\tconst char **delnames;\n+\tint ret = 0, i;\n \tint n = transaction->nr;\n \tstruct ref_update **updates = transaction->updates;\n+\tstruct string_list refs_to_delete = STRING_LIST_INIT_NODUP;\n+\tstruct string_list_item *ref_to_delete;\n \n \tassert(err);\n \n@@ -3753,9 +3757,6 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \t\treturn 0;\n \t}\n \n-\t/* Allocate work space */\n-\tdelnames = xmalloc(sizeof(*delnames) * n);\n-\n \t/* Copy, sort, and reject duplicate refs */\n \tqsort(updates, n, sizeof(*updates), ref_update_compare);\n \tif (ref_update_reject_duplicates(updates, n, err)) {\n@@ -3815,16 +3816,17 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \t\t\t}\n \n \t\t\tif (!(update->flags & REF_ISPRUNING))\n-\t\t\t\tdelnames[delnum++] = update->lock->ref_name;\n+\t\t\t\tstring_list_append(&refs_to_delete,\n+\t\t\t\t\t\t update->lock->ref_name);\n \t\t}\n \t}\n \n-\tif (repack_without_refs(delnames, delnum, err)) {\n+\tif (repack_without_refs(&refs_to_delete, err)) {\n \t\tret = TRANSACTION_GENERIC_ERROR;\n \t\tgoto cleanup;\n \t}\n-\tfor (i = 0; i < delnum; i++)\n-\t\tunlink_or_warn(git_path(\"logs/%s\", delnames[i]));\n+\tfor_each_string_list_item(ref_to_delete, &refs_to_delete)\n+\t\tunlink_or_warn(git_path(\"logs/%s\", ref_to_delete->string));\n \tclear_loose_ref_cache(&ref_cache);\n \n cleanup:\n@@ -3833,7 +3835,7 @@ cleanup:\n \tfor (i = 0; i < n; i++)\n \t\tif (updates[i]->lock)\n \t\t\tunlock_ref(updates[i]->lock);\n-\tfree(delnames);\n+\tstring_list_clear(&refs_to_delete, 0);\n \treturn ret;\n }\n \ndiff --git a/refs.h b/refs.h\nindex 2bc3556..69f88ef 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -163,8 +163,14 @@ extern void rollback_packed_refs(void);\n */\n int pack_refs(unsigned int flags);\n \n-extern int repack_without_refs(const char **refnames, int n,\n-\t\t\t struct strbuf *err);\n+/*\n+ * Repacks the refs pack file excluding the refs given\n+ * without: The refs to be excluded from the new refs pack file,\n+ * May be unsorted\n+ * err: String buffer, which will be used for reporting errors,\n+ * Must not be NULL\n+ */\n+extern int repack_without_refs(struct string_list *without, struct strbuf *err);\n \n extern int ref_exists(const char *);\n \n-- \n2.2.0.rc2.13.g0786cdb\n"},{"id":"252219","messageId":"1416434399-2303-1-git-send-email-sbeller@google.com","threadId":"38003","inReplyTo":"1416434088-1472-1-git-send-email-sbeller@google.com","subject":"[PATCH v4] refs.c: use a stringlist for repack_without_refs","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-11-19T21:59:59Z","receivedAt":"2014-11-19T21:59:59Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"From: Ronnie Sahlberg \n\nThis patch doesn't intend any functional changes. It is just\na refactoring, which replaces a char** array by a stringlist\nin the function repack_without_refs.\nThis is easier to read and maintain as it delivers the same\nfunctionality with less lines of code and less pointers.\n\n[sb: ported this patch from a larger patch series to the master branch,\nadded documentary comments in refs.h]\n\nSigned-off-by: Ronnie Sahlberg \nSigned-off-by: Stefan Beller \n---\n\nChanges to version 1:\n * removed the double blank line\n * rename delete_refs_list to delete_refs\n * add back comments dropped by accident\n * use STRING_LIST_INIT_NODUP instead of the _DUP version in ref_transaction_commit\n * add documentary comments on the repack_without_refs function\n * user string_list_append instead of string_list_insert as it follows the previous\n behavior more closely.\n * put back the early exit of the loop in repack_without_refs\n \nChanges to version 2:\n * fixed commit message (comments after the three dashes)\n * fixed the curiosity Junio pointed out as it was just wrong code.\n Now it actually builds a list of all states.stale.items[i].util items.\n \nChanges in version 3:\n\n * reword commit message\n * sort delete_refs before passing it to warn_dangling_symrefs\n * change the comments (get back the one jrn complained about) \n in repack_without_refs\n * use the suggestion of jonathan for documenting repack_without_refs in the\n header. Add a note about the arguments.\n\nChanges in version 4:\n * I lied, when saying I had all the nits from Jonathan. \n I messed up the documentation in the header.\n This includes the documentary comment in the header.\n \n---\n builtin/remote.c | 32 ++++++++++++--------------------\n refs.c | 38 ++++++++++++++++++++------------------\n refs.h | 11 +++++++++--\n 3 files changed, 41 insertions(+), 40 deletions(-)\n\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex 7f28f92..b37ed3d 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -750,16 +750,11 @@ static int mv(int argc, const char **argv)\n static int remove_branches(struct string_list *branches)\n {\n \tstruct strbuf err = STRBUF_INIT;\n-\tconst char **branch_names;\n \tint i, result = 0;\n \n-\tbranch_names = xmalloc(branches->nr * sizeof(*branch_names));\n-\tfor (i = 0; i < branches->nr; i++)\n-\t\tbranch_names[i] = branches->items[i].string;\n-\tif (repack_without_refs(branch_names, branches->nr, &err))\n+\tif (repack_without_refs(branches, &err))\n \t\tresult |= error(\"%s\", err.buf);\n \tstrbuf_release(&err);\n-\tfree(branch_names);\n \n \tfor (i = 0; i < branches->nr; i++) {\n \t\tstruct string_list_item *item = branches->items + i;\n@@ -1316,8 +1311,8 @@ static int prune_remote(const char *remote, int dry_run)\n {\n \tint result = 0, i;\n \tstruct ref_states states;\n-\tstruct string_list delete_refs_list = STRING_LIST_INIT_NODUP;\n-\tconst char **delete_refs;\n+\tstruct string_list delete_refs = STRING_LIST_INIT_NODUP;\n+\tstruct string_list_item *ref;\n \tconst char *dangling_msg = dry_run\n \t\t? _(\" %s will become dangling!\")\n \t\t: _(\" %s has become dangling!\");\n@@ -1325,6 +1320,9 @@ static int prune_remote(const char *remote, int dry_run)\n \tmemset(&states, 0, sizeof(states));\n \tget_remote_ref_states(remote, &states, GET_REF_STATES);\n \n+\tfor (i = 0; i < states.stale.nr; i++)\n+\t\tstring_list_append(&delete_refs, states.stale.items[i].util);\n+\n \tif (states.stale.nr) {\n \t\tprintf_ln(_(\"Pruning %s\"), remote);\n \t\tprintf_ln(_(\"URL: %s\"),\n@@ -1332,23 +1330,16 @@ static int prune_remote(const char *remote, int dry_run)\n \t\t ? states.remote->url[0]\n \t\t : _(\"(no URL)\"));\n \n-\t\tdelete_refs = xmalloc(states.stale.nr * sizeof(*delete_refs));\n-\t\tfor (i = 0; i < states.stale.nr; i++)\n-\t\t\tdelete_refs[i] = states.stale.items[i].util;\n \t\tif (!dry_run) {\n \t\t\tstruct strbuf err = STRBUF_INIT;\n-\t\t\tif (repack_without_refs(delete_refs, states.stale.nr,\n-\t\t\t\t\t\t&err))\n+\t\t\tif (repack_without_refs(&delete_refs, &err))\n \t\t\t\tresult |= error(\"%s\", err.buf);\n \t\t\tstrbuf_release(&err);\n \t\t}\n-\t\tfree(delete_refs);\n \t}\n \n-\tfor (i = 0; i < states.stale.nr; i++) {\n-\t\tconst char *refname = states.stale.items[i].util;\n-\n-\t\tstring_list_insert(&delete_refs_list, refname);\n+\tfor_each_string_list_item(ref, &delete_refs) {\n+\t\tconst char *refname = ref->string;\n \n \t\tif (!dry_run)\n \t\t\tresult |= delete_ref(refname, NULL, 0);\n@@ -1361,8 +1352,9 @@ static int prune_remote(const char *remote, int dry_run)\n \t\t\t abbrev_ref(refname, \"refs/remotes/\"));\n \t}\n \n-\twarn_dangling_symrefs(stdout, dangling_msg, &delete_refs_list);\n-\tstring_list_clear(&delete_refs_list, 0);\n+\tsort_string_list(&delete_refs);\n+\twarn_dangling_symrefs(stdout, dangling_msg, &delete_refs);\n+\tstring_list_clear(&delete_refs, 0);\n \n \tfree_remote_ref_states(&states);\n \treturn result;\ndiff --git a/refs.c b/refs.c\nindex 5ff457e..ebcd90f 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2639,23 +2639,26 @@ static int curate_packed_ref_fn(struct ref_entry *entry, void *cb_data)\n \treturn 0;\n }\n \n-int repack_without_refs(const char **refnames, int n, struct strbuf *err)\n+int repack_without_refs(struct string_list *without, struct strbuf *err)\n {\n \tstruct ref_dir *packed;\n \tstruct string_list refs_to_delete = STRING_LIST_INIT_DUP;\n \tstruct string_list_item *ref_to_delete;\n-\tint i, ret, removed = 0;\n+\tint ret, needs_repacking = 0, removed = 0;\n \n \tassert(err);\n \n \t/* Look for a packed ref */\n-\tfor (i = 0; i < n; i++)\n-\t\tif (get_packed_ref(refnames[i]))\n+\tfor_each_string_list_item(ref_to_delete, without) {\n+\t\tif (get_packed_ref(ref_to_delete->string)) {\n+\t\t\tneeds_repacking = 1;\n \t\t\tbreak;\n+\t\t}\n+\t}\n \n \t/* Avoid locking if we have nothing to do */\n-\tif (i == n)\n-\t\treturn 0; /* no refname exists in packed refs */\n+\tif (!needs_repacking)\n+\t\treturn 0;\n \n \tif (lock_packed_refs(0)) {\n \t\tunable_to_lock_message(git_path(\"packed-refs\"), errno, err);\n@@ -2664,8 +2667,8 @@ int repack_without_refs(const char **refnames, int n, struct strbuf *err)\n \tpacked = get_packed_refs(&ref_cache);\n \n \t/* Remove refnames from the cache */\n-\tfor (i = 0; i < n; i++)\n-\t\tif (remove_entry(packed, refnames[i]) != -1)\n+\tfor_each_string_list_item(ref_to_delete, without)\n+\t\tif (remove_entry(packed, ref_to_delete->string) != -1)\n \t\t\tremoved = 1;\n \tif (!removed) {\n \t\t/*\n@@ -3738,10 +3741,11 @@ static int ref_update_reject_duplicates(struct ref_update **updates, int n,\n int ref_transaction_commit(struct ref_transaction *transaction,\n \t\t\t struct strbuf *err)\n {\n-\tint ret = 0, delnum = 0, i;\n-\tconst char **delnames;\n+\tint ret = 0, i;\n \tint n = transaction->nr;\n \tstruct ref_update **updates = transaction->updates;\n+\tstruct string_list refs_to_delete = STRING_LIST_INIT_NODUP;\n+\tstruct string_list_item *ref_to_delete;\n \n \tassert(err);\n \n@@ -3753,9 +3757,6 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \t\treturn 0;\n \t}\n \n-\t/* Allocate work space */\n-\tdelnames = xmalloc(sizeof(*delnames) * n);\n-\n \t/* Copy, sort, and reject duplicate refs */\n \tqsort(updates, n, sizeof(*updates), ref_update_compare);\n \tif (ref_update_reject_duplicates(updates, n, err)) {\n@@ -3815,16 +3816,17 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \t\t\t}\n \n \t\t\tif (!(update->flags & REF_ISPRUNING))\n-\t\t\t\tdelnames[delnum++] = update->lock->ref_name;\n+\t\t\t\tstring_list_append(&refs_to_delete,\n+\t\t\t\t\t\t update->lock->ref_name);\n \t\t}\n \t}\n \n-\tif (repack_without_refs(delnames, delnum, err)) {\n+\tif (repack_without_refs(&refs_to_delete, err)) {\n \t\tret = TRANSACTION_GENERIC_ERROR;\n \t\tgoto cleanup;\n \t}\n-\tfor (i = 0; i < delnum; i++)\n-\t\tunlink_or_warn(git_path(\"logs/%s\", delnames[i]));\n+\tfor_each_string_list_item(ref_to_delete, &refs_to_delete)\n+\t\tunlink_or_warn(git_path(\"logs/%s\", ref_to_delete->string));\n \tclear_loose_ref_cache(&ref_cache);\n \n cleanup:\n@@ -3833,7 +3835,7 @@ cleanup:\n \tfor (i = 0; i < n; i++)\n \t\tif (updates[i]->lock)\n \t\t\tunlock_ref(updates[i]->lock);\n-\tfree(delnames);\n+\tstring_list_clear(&refs_to_delete, 0);\n \treturn ret;\n }\n \ndiff --git a/refs.h b/refs.h\nindex 2bc3556..5a0cd21 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -163,8 +163,15 @@ extern void rollback_packed_refs(void);\n */\n int pack_refs(unsigned int flags);\n \n-extern int repack_without_refs(const char **refnames, int n,\n-\t\t\t struct strbuf *err);\n+/*\n+ * Remove the refs listed in 'without' from the packed-refs file.\n+ * On error, packed-refs will be unchanged, the return value is\n+ * nonzero, and a message about the error is written to the 'err'\n+ * strbuf.\n+ *\n+ * The refs in 'without' may have any order, the err buffer must not be ommited.\n+ */\n+extern int repack_without_refs(struct string_list *without, struct strbuf *err);\n \n extern int ref_exists(const char *);\n \n-- \n2.2.0.rc2.13.g0786cdb\n"},{"id":"252238","messageId":"20141120021540.GF6527@google.com","threadId":"38003","inReplyTo":"1416434399-2303-1-git-send-email-sbeller@google.com","subject":"Re: [PATCH v4] refs.c: use a stringlist for repack_without_refs","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-11-20T02:15:40Z","receivedAt":"2014-11-20T02:15:40Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Stefan Beller wrote:\n\n> From: Ronnie Sahlberg \n>\n> This patch doesn't intend any functional changes. It is just\n> a refactoring, which replaces a char** array by a stringlist\n> in the function repack_without_refs.\n> This is easier to read and maintain as it delivers the same\n> functionality with less lines of code and less pointers.\n\nThanks for the quick turnaround.\n\nNit: please wrap to a consistent width and put a blank line between\nparagraphs.\n\nThat is, the above should either say\n\n\tThis patch doesn't intend any functional changes. It is just\n\ta refactoring to replace a char** array with a string_list\n\tin the function repack_without_refs. This is easier to read\n\tand maintain as it delivers the same functionality with less\n\tcode and fewer pointers.\n\nor\n\n\tThis patch doesn't intend any functional changes. It is just\n\ta refactoring to replace a char** array with a string_list\n\tin the function repack_without_refs.\n\n\tThis is easier to read and maintain as it delivers the same\n\tfunctionality with less code and fewer pointers.\n\nAlthough I'm not sure the main benefit is having fewer asterisks. ;-)\n\n[...]\n> +++ b/builtin/remote.c\n[...]\n> @@ -1361,8 +1352,9 @@ static int prune_remote(const char *remote, int dry_run)\n> \t\t\t abbrev_ref(refname, \"refs/remotes/\"));\n> \t}\n> \n> -\twarn_dangling_symrefs(stdout, dangling_msg, &delete_refs_list);\n> -\tstring_list_clear(&delete_refs_list, 0);\n> +\tsort_string_list(&delete_refs);\n> +\twarn_dangling_symrefs(stdout, dangling_msg, &delete_refs);\n> +\tstring_list_clear(&delete_refs, 0);\n> \n> \tfree_remote_ref_states(&states);\n> \treturn result;\n\nMicronit: it would be clearer (and easier to remember to free the list\nin other code paths if this function gains more 'return' statements)\nwith the string_list_clear in the same block as other code that frees\nresources (i.e., if the blank line moved one line up).\n\n[...]\n> --- a/refs.h\n> +++ b/refs.h\n> @@ -163,8 +163,15 @@ extern void rollback_packed_refs(void);\n> */\n> int pack_refs(unsigned int flags);\n> \n> -extern int repack_without_refs(const char **refnames, int n,\n> -\t\t\t struct strbuf *err);\n> +/*\n> + * Remove the refs listed in 'without' from the packed-refs file.\n> + * On error, packed-refs will be unchanged, the return value is\n> + * nonzero, and a message about the error is written to the 'err'\n> + * strbuf.\n> + *\n> + * The refs in 'without' may have any order, the err buffer must not be ommited.\n\nNits:\n\ns/ommited/omitted/\n\nComma splice. Long line.\n\nThe function has to be able to write to 'err' on error, so I think the\ncomment doesn't have to mention that err must be non-NULL. Any caller\nthat tries to pass NULL will get an assertion error quickly.\n\nWith or without the changes suggested above,\nReviewed-by: Jonathan Nieder \n"},{"id":"252263","messageId":"xmqqr3wx4y5x.fsf@gitster.dls.corp.google.com","threadId":"38003","inReplyTo":"20141120021540.GF6527@google.com","subject":"Re: [PATCH v4] refs.c: use a stringlist for repack_without_refs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-11-20T16:47:06Z","receivedAt":"2014-11-20T16:47:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> [...]\n>> +++ b/builtin/remote.c\n> [...]\n>> @@ -1361,8 +1352,9 @@ static int prune_remote(const char *remote, int dry_run)\n>> \t\t\t abbrev_ref(refname, \"refs/remotes/\"));\n>> \t}\n>> \n>> -\twarn_dangling_symrefs(stdout, dangling_msg, &delete_refs_list);\n>> -\tstring_list_clear(&delete_refs_list, 0);\n>> +\tsort_string_list(&delete_refs);\n>> +\twarn_dangling_symrefs(stdout, dangling_msg, &delete_refs);\n>> +\tstring_list_clear(&delete_refs, 0);\n>> \n>> \tfree_remote_ref_states(&states);\n>> \treturn result;\n>\n> Micronit: it would be clearer (and easier to remember to free the list\n> in other code paths if this function gains more 'return' statements)\n> with the string_list_clear in the same block as other code that frees\n> resources (i.e., if the blank line moved one line up).\n\nThanks for a careful reading. This kind of attention to detail\nhelps the longer term health of the codebase.\n\n> The function has to be able to write to 'err' on error, so I think the\n> comment doesn't have to mention that err must be non-NULL. Any caller\n> that tries to pass NULL will get an assertion error quickly.\n\nThat invites a bit of question, though.\n\nAn equally plausible alternative definition for set of API functions\nthat take strbuf *err is to pass it only when you care about the\nexplanation of the error (i.e. it is valid for \"git cmd --quiet\" to\npass NULL there) [*1*] (do we already have such a function?). And\nthe comment may help clarifying which is which. I however think we\nshouldn't have mixtures (formatting into \"strbuf *err\" may be costly\nwhen we know we are asked to fail silently, but an error path is not\nusually performance sensitive).\n\n\n[Footnote]\n\n*1* With yet another one, a function may call error() on its own\nwhen a NULL is passed to strbuf *err, but let's not go there.\n"},{"id":"252272","messageId":"1416506666-5989-1-git-send-email-sbeller@google.com","threadId":"38003","inReplyTo":"20141120021540.GF6527@google.com","subject":"[PATCH v5 1/1] refs.c: use a stringlist for repack_without_refs","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-11-20T18:04:26Z","receivedAt":"2014-11-20T18:04:26Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"From: Ronnie Sahlberg \n\nThis patch doesn't intend any functional changes. It is just\na refactoring, which replaces a char** array by a stringlist\nin the function repack_without_refs.\n\nThis is easier to read and maintain as it delivers the same\nfunctionality with less lines of code and more lines of\ndocumentation.\n\n[sb: ported this patch from a larger patch series to the\nmaster branch, added documentary comments in refs.h]\n\nChange-Id: Id7eaa821331f2ab89df063e1e76c8485dbcc3aed\nSigned-off-by: Ronnie Sahlberg \nSigned-off-by: Stefan Beller \nReviewed-by: Jonathan Nieder \n---\n\nChanges to version 1:\n * removed the double blank line\n * rename delete_refs_list to delete_refs\n * add back comments dropped by accident\n * use STRING_LIST_INIT_NODUP instead of the _DUP version in ref_transaction_commit\n * add documentary comments on the repack_without_refs function\n * user string_list_append instead of string_list_insert as it follows the previous\n behavior more closely.\n * put back the early exit of the loop in repack_without_refs\n \nChanges to version 2:\n * fixed commit message (comments after the three dashes)\n * fixed the curiosity Junio pointed out as it was just wrong code.\n Now it actually builds a list of all states.stale.items[i].util items.\n \nChanges in version 3:\n\n * reword commit message\n * sort delete_refs before passing it to warn_dangling_symrefs\n * change the comments (get back the one jrn complained about) \n in repack_without_refs\n * use the suggestion of jonathan for documenting repack_without_refs in the\n header. Add a note about the arguments.\n\nChanges in version 4:\n * I lied, when saying I had all the nits from Jonathan. \n I messed up the documentation in the header.\n This includes the documentary comment in the header.\n\nChanges in version 5:\n * Break lines as suggested by Jonathan, slightly rewording the commit message\n * have an empty line at another place in builtin/remote.c remove_branches to \n tell cleanup parts apart from actual work.\n * fix typo, improve documentary comment in refs.c\n * add Jonathans reviewed by\n \n Junio, I'll address your proposed changes in a different patch. \n If err is passed in as NULL, we'll just skip all the error string \n formatting and return silent and fast.\n \n builtin/remote.c | 32 ++++++++++++--------------------\n refs.c | 38 ++++++++++++++++++++------------------\n refs.h | 12 ++++++++++--\n 3 files changed, 42 insertions(+), 40 deletions(-)\n\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex 7f28f92..364350a 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -750,16 +750,11 @@ static int mv(int argc, const char **argv)\n static int remove_branches(struct string_list *branches)\n {\n \tstruct strbuf err = STRBUF_INIT;\n-\tconst char **branch_names;\n \tint i, result = 0;\n \n-\tbranch_names = xmalloc(branches->nr * sizeof(*branch_names));\n-\tfor (i = 0; i < branches->nr; i++)\n-\t\tbranch_names[i] = branches->items[i].string;\n-\tif (repack_without_refs(branch_names, branches->nr, &err))\n+\tif (repack_without_refs(branches, &err))\n \t\tresult |= error(\"%s\", err.buf);\n \tstrbuf_release(&err);\n-\tfree(branch_names);\n \n \tfor (i = 0; i < branches->nr; i++) {\n \t\tstruct string_list_item *item = branches->items + i;\n@@ -1316,8 +1311,8 @@ static int prune_remote(const char *remote, int dry_run)\n {\n \tint result = 0, i;\n \tstruct ref_states states;\n-\tstruct string_list delete_refs_list = STRING_LIST_INIT_NODUP;\n-\tconst char **delete_refs;\n+\tstruct string_list delete_refs = STRING_LIST_INIT_NODUP;\n+\tstruct string_list_item *ref;\n \tconst char *dangling_msg = dry_run\n \t\t? _(\" %s will become dangling!\")\n \t\t: _(\" %s has become dangling!\");\n@@ -1325,6 +1320,9 @@ static int prune_remote(const char *remote, int dry_run)\n \tmemset(&states, 0, sizeof(states));\n \tget_remote_ref_states(remote, &states, GET_REF_STATES);\n \n+\tfor (i = 0; i < states.stale.nr; i++)\n+\t\tstring_list_append(&delete_refs, states.stale.items[i].util);\n+\n \tif (states.stale.nr) {\n \t\tprintf_ln(_(\"Pruning %s\"), remote);\n \t\tprintf_ln(_(\"URL: %s\"),\n@@ -1332,23 +1330,16 @@ static int prune_remote(const char *remote, int dry_run)\n \t\t ? states.remote->url[0]\n \t\t : _(\"(no URL)\"));\n \n-\t\tdelete_refs = xmalloc(states.stale.nr * sizeof(*delete_refs));\n-\t\tfor (i = 0; i < states.stale.nr; i++)\n-\t\t\tdelete_refs[i] = states.stale.items[i].util;\n \t\tif (!dry_run) {\n \t\t\tstruct strbuf err = STRBUF_INIT;\n-\t\t\tif (repack_without_refs(delete_refs, states.stale.nr,\n-\t\t\t\t\t\t&err))\n+\t\t\tif (repack_without_refs(&delete_refs, &err))\n \t\t\t\tresult |= error(\"%s\", err.buf);\n \t\t\tstrbuf_release(&err);\n \t\t}\n-\t\tfree(delete_refs);\n \t}\n \n-\tfor (i = 0; i < states.stale.nr; i++) {\n-\t\tconst char *refname = states.stale.items[i].util;\n-\n-\t\tstring_list_insert(&delete_refs_list, refname);\n+\tfor_each_string_list_item(ref, &delete_refs) {\n+\t\tconst char *refname = ref->string;\n \n \t\tif (!dry_run)\n \t\t\tresult |= delete_ref(refname, NULL, 0);\n@@ -1361,9 +1352,10 @@ static int prune_remote(const char *remote, int dry_run)\n \t\t\t abbrev_ref(refname, \"refs/remotes/\"));\n \t}\n \n-\twarn_dangling_symrefs(stdout, dangling_msg, &delete_refs_list);\n-\tstring_list_clear(&delete_refs_list, 0);\n+\tsort_string_list(&delete_refs);\n+\twarn_dangling_symrefs(stdout, dangling_msg, &delete_refs);\n \n+\tstring_list_clear(&delete_refs, 0);\n \tfree_remote_ref_states(&states);\n \treturn result;\n }\ndiff --git a/refs.c b/refs.c\nindex 5ff457e..ebcd90f 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2639,23 +2639,26 @@ static int curate_packed_ref_fn(struct ref_entry *entry, void *cb_data)\n \treturn 0;\n }\n \n-int repack_without_refs(const char **refnames, int n, struct strbuf *err)\n+int repack_without_refs(struct string_list *without, struct strbuf *err)\n {\n \tstruct ref_dir *packed;\n \tstruct string_list refs_to_delete = STRING_LIST_INIT_DUP;\n \tstruct string_list_item *ref_to_delete;\n-\tint i, ret, removed = 0;\n+\tint ret, needs_repacking = 0, removed = 0;\n \n \tassert(err);\n \n \t/* Look for a packed ref */\n-\tfor (i = 0; i < n; i++)\n-\t\tif (get_packed_ref(refnames[i]))\n+\tfor_each_string_list_item(ref_to_delete, without) {\n+\t\tif (get_packed_ref(ref_to_delete->string)) {\n+\t\t\tneeds_repacking = 1;\n \t\t\tbreak;\n+\t\t}\n+\t}\n \n \t/* Avoid locking if we have nothing to do */\n-\tif (i == n)\n-\t\treturn 0; /* no refname exists in packed refs */\n+\tif (!needs_repacking)\n+\t\treturn 0;\n \n \tif (lock_packed_refs(0)) {\n \t\tunable_to_lock_message(git_path(\"packed-refs\"), errno, err);\n@@ -2664,8 +2667,8 @@ int repack_without_refs(const char **refnames, int n, struct strbuf *err)\n \tpacked = get_packed_refs(&ref_cache);\n \n \t/* Remove refnames from the cache */\n-\tfor (i = 0; i < n; i++)\n-\t\tif (remove_entry(packed, refnames[i]) != -1)\n+\tfor_each_string_list_item(ref_to_delete, without)\n+\t\tif (remove_entry(packed, ref_to_delete->string) != -1)\n \t\t\tremoved = 1;\n \tif (!removed) {\n \t\t/*\n@@ -3738,10 +3741,11 @@ static int ref_update_reject_duplicates(struct ref_update **updates, int n,\n int ref_transaction_commit(struct ref_transaction *transaction,\n \t\t\t struct strbuf *err)\n {\n-\tint ret = 0, delnum = 0, i;\n-\tconst char **delnames;\n+\tint ret = 0, i;\n \tint n = transaction->nr;\n \tstruct ref_update **updates = transaction->updates;\n+\tstruct string_list refs_to_delete = STRING_LIST_INIT_NODUP;\n+\tstruct string_list_item *ref_to_delete;\n \n \tassert(err);\n \n@@ -3753,9 +3757,6 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \t\treturn 0;\n \t}\n \n-\t/* Allocate work space */\n-\tdelnames = xmalloc(sizeof(*delnames) * n);\n-\n \t/* Copy, sort, and reject duplicate refs */\n \tqsort(updates, n, sizeof(*updates), ref_update_compare);\n \tif (ref_update_reject_duplicates(updates, n, err)) {\n@@ -3815,16 +3816,17 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \t\t\t}\n \n \t\t\tif (!(update->flags & REF_ISPRUNING))\n-\t\t\t\tdelnames[delnum++] = update->lock->ref_name;\n+\t\t\t\tstring_list_append(&refs_to_delete,\n+\t\t\t\t\t\t update->lock->ref_name);\n \t\t}\n \t}\n \n-\tif (repack_without_refs(delnames, delnum, err)) {\n+\tif (repack_without_refs(&refs_to_delete, err)) {\n \t\tret = TRANSACTION_GENERIC_ERROR;\n \t\tgoto cleanup;\n \t}\n-\tfor (i = 0; i < delnum; i++)\n-\t\tunlink_or_warn(git_path(\"logs/%s\", delnames[i]));\n+\tfor_each_string_list_item(ref_to_delete, &refs_to_delete)\n+\t\tunlink_or_warn(git_path(\"logs/%s\", ref_to_delete->string));\n \tclear_loose_ref_cache(&ref_cache);\n \n cleanup:\n@@ -3833,7 +3835,7 @@ cleanup:\n \tfor (i = 0; i < n; i++)\n \t\tif (updates[i]->lock)\n \t\t\tunlock_ref(updates[i]->lock);\n-\tfree(delnames);\n+\tstring_list_clear(&refs_to_delete, 0);\n \treturn ret;\n }\n \ndiff --git a/refs.h b/refs.h\nindex 2bc3556..c7323ff 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -163,8 +163,16 @@ extern void rollback_packed_refs(void);\n */\n int pack_refs(unsigned int flags);\n \n-extern int repack_without_refs(const char **refnames, int n,\n-\t\t\t struct strbuf *err);\n+/*\n+ * Remove the refs listed in 'without' from the packed-refs file.\n+ * On error, packed-refs will be unchanged, the return value is\n+ * nonzero, and a message about the error is written to the 'err'\n+ * strbuf.\n+ *\n+ * The refs in 'without' may have any order.\n+ * The err buffer must not be omitted.\n+ */\n+extern int repack_without_refs(struct string_list *without, struct strbuf *err);\n \n extern int ref_exists(const char *);\n \n-- \n2.2.0.rc2.23.gca0107e\n"},{"id":"252274","messageId":"1416507040-6576-1-git-send-email-sbeller@google.com","threadId":"38003","inReplyTo":"1416506666-5989-1-git-send-email-sbeller@google.com","subject":"[PATCH] refs.c: repack_without_refs may be called without error string buffer","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-11-20T18:10:40Z","receivedAt":"2014-11-20T18:10:40Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"If we don't pass in the error string buffer, we skip over all\nparts dealing with preparing error messages.\n\nSigned-off-by: Stefan Beller \n---\n\nThis goes ontop of [PATCH v5] refs.c: use a stringlist for repack_without_refs\nif that makes sense.\n\n refs.c | 8 ++++----\n refs.h | 1 -\n 2 files changed, 4 insertions(+), 5 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex ebcd90f..3c85ea6 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2646,8 +2646,6 @@ int repack_without_refs(struct string_list *without, struct strbuf *err)\n \tstruct string_list_item *ref_to_delete;\n \tint ret, needs_repacking = 0, removed = 0;\n \n-\tassert(err);\n-\n \t/* Look for a packed ref */\n \tfor_each_string_list_item(ref_to_delete, without) {\n \t\tif (get_packed_ref(ref_to_delete->string)) {\n@@ -2661,7 +2659,9 @@ int repack_without_refs(struct string_list *without, struct strbuf *err)\n \t\treturn 0;\n \n \tif (lock_packed_refs(0)) {\n-\t\tunable_to_lock_message(git_path(\"packed-refs\"), errno, err);\n+\t\tif (err)\n+\t\t\tunable_to_lock_message(git_path(\"packed-refs\"),\n+\t\t\t\t\t errno, err);\n \t\treturn -1;\n \t}\n \tpacked = get_packed_refs(&ref_cache);\n@@ -2688,7 +2688,7 @@ int repack_without_refs(struct string_list *without, struct strbuf *err)\n \n \t/* Write what remains */\n \tret = commit_packed_refs();\n-\tif (ret)\n+\tif (ret && err)\n \t\tstrbuf_addf(err, \"unable to overwrite old ref-pack file: %s\",\n \t\t\t strerror(errno));\n \treturn ret;\ndiff --git a/refs.h b/refs.h\nindex c7323ff..b71fb79 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -170,7 +170,6 @@ int pack_refs(unsigned int flags);\n * strbuf.\n *\n * The refs in 'without' may have any order.\n- * The err buffer must not be omitted.\n */\n extern int repack_without_refs(struct string_list *without, struct strbuf *err);\n \n-- \n2.2.0.rc2.23.gca0107e\n"},{"id":"252275","messageId":"CAL=YDWkOa4wmQdq5wOUZVQUUhnm49AVpf6gGM=5RO9Qi5dCzew@mail.gmail.com","threadId":"38003","inReplyTo":"1416507040-6576-1-git-send-email-sbeller@google.com","subject":"Re: [PATCH] refs.c: repack_without_refs may be called without error string buffer","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-11-20T18:15:00Z","receivedAt":"2014-11-20T18:15:00Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"On Thu, Nov 20, 2014 at 10:10 AM, Stefan Beller wrote:\n> If we don't pass in the error string buffer, we skip over all\n> parts dealing with preparing error messages.\n>\n> Signed-off-by: Stefan Beller \n> ---\n>\n> This goes ontop of [PATCH v5] refs.c: use a stringlist for repack_without_refs\n> if that makes sense.\n>\n> refs.c | 8 ++++----\n> refs.h | 1 -\n> 2 files changed, 4 insertions(+), 5 deletions(-)\n>\n> diff --git a/refs.c b/refs.c\n> index ebcd90f..3c85ea6 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -2646,8 +2646,6 @@ int repack_without_refs(struct string_list *without, struct strbuf *err)\n> struct string_list_item *ref_to_delete;\n> int ret, needs_repacking = 0, removed = 0;\n>\n> - assert(err);\n> -\n> /* Look for a packed ref */\n> for_each_string_list_item(ref_to_delete, without) {\n> if (get_packed_ref(ref_to_delete->string)) {\n> @@ -2661,7 +2659,9 @@ int repack_without_refs(struct string_list *without, struct strbuf *err)\n> return 0;\n>\n> if (lock_packed_refs(0)) {\n> - unable_to_lock_message(git_path(\"packed-refs\"), errno, err);\n> + if (err)\n> + unable_to_lock_message(git_path(\"packed-refs\"),\n> + errno, err);\n> return -1;\n> }\n> packed = get_packed_refs(&ref_cache);\n> @@ -2688,7 +2688,7 @@ int repack_without_refs(struct string_list *without, struct strbuf *err)\n>\n> /* Write what remains */\n> ret = commit_packed_refs();\n> - if (ret)\n> + if (ret && err)\n> strbuf_addf(err, \"unable to overwrite old ref-pack file: %s\",\n> strerror(errno));\n> return ret;\n> diff --git a/refs.h b/refs.h\n> index c7323ff..b71fb79 100644\n> --- a/refs.h\n> +++ b/refs.h\n> @@ -170,7 +170,6 @@ int pack_refs(unsigned int flags);\n> * strbuf.\n> *\n> * The refs in 'without' may have any order.\n> - * The err buffer must not be omitted.\n> */\n> extern int repack_without_refs(struct string_list *without, struct strbuf *err);\n>\n> --\n> 2.2.0.rc2.23.gca0107e\n>\n\nLGTM\nReviewed-by: Ronnie Sahlberg \n\nNit:\nWhile it does not hurt to allow passing NULL, at some stage later\nthis function will become\nprivate to refs.c and ONLY be called from within transaction_commit()\nwhich will always\npass a non-NULL err argument.\nAt that stage we will not strictly need to allow err==NULL since all\ncallers are guaranteed to\nalways pass err!=NULL.\n\nThat said, having err being optional is probably a better API. Maybe\nerr should be made optional for all other functions that take\nan err strbuf too so that the calling conventions become more consistent?\n"},{"id":"252279","messageId":"20141120182901.GC15945@google.com","threadId":"38003","inReplyTo":"1416506666-5989-1-git-send-email-sbeller@google.com","subject":"Re: [PATCH v5 1/1] refs.c: use a stringlist for repack_without_refs","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-11-20T18:29:01Z","receivedAt":"2014-11-20T18:29:01Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Stefan Beller wrote:\n\n> Change-Id: Id7eaa821331f2ab89df063e1e76c8485dbcc3aed\n\nChange-id snuck in.\n\n[...]\n> --- a/refs.h\n> +++ b/refs.h\n> @@ -163,8 +163,16 @@ extern void rollback_packed_refs(void);\n> */\n> int pack_refs(unsigned int flags);\n> \n> -extern int repack_without_refs(const char **refnames, int n,\n> -\t\t\t struct strbuf *err);\n> +/*\n> + * Remove the refs listed in 'without' from the packed-refs file.\n> + * On error, packed-refs will be unchanged, the return value is\n> + * nonzero, and a message about the error is written to the 'err'\n> + * strbuf.\n> + *\n> + * The refs in 'without' may have any order.\n\nTiny nit: this makes me wonder what the order represents --- how do\nI pick which order for the refs in without to have?\n\nI think the idea is just that 'without' doesn't have to be sorted (it's\na shame we don't have separate sorted string list and unsorted string\nlist types or a string_list_sorted() helper to catch bad callers early\nto functions that care). One way to say that would be\n\n\tRemove the refs listed in the unsorted string list 'without' from the\n\tpacked-refs file. On error, [...]\n\n> + * The err buffer must not be omitted.\n\ns/buffer/strbuf/, or s/The err buffer/'err'/\ns/omitted/NULL/\n\nWith the Change-Id dropped, and with or without the above comment nits\naddressed,\n\nReviewed-by: Jonathan Nieder \n"},{"id":"252280","messageId":"20141120183523.GD15945@google.com","threadId":"38003","inReplyTo":"1416507040-6576-1-git-send-email-sbeller@google.com","subject":"Re: [PATCH] refs.c: repack_without_refs may be called without error string buffer","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-11-20T18:35:23Z","receivedAt":"2014-11-20T18:35:23Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Stefan Beller wrote:\n\n> If we don't pass in the error string buffer, we skip over all\n> parts dealing with preparing error messages.\n\nPlease no.\n\nWe tried this with the ref transaction code. When someone wants\nto silence the message, it is cheap enough to do\n\n\tstruct strbuf ignore = STRBUF_INIT;\n\n\tif (thing_that_can_fail_in_an_ignorable_way(..., &ignore)) {\n\t\t... handle the failure ...\n\t}\n\nThe extra lines of code make it obvious that the error message is\nbeing dropped, which is a very good thing. The extra work to format a\nmessage in the error case is not so bad and can be mitigated if the\nerror is a common normal case by passing a flag to not consider it an\nerror.\n\nSilently losing good diagnostic messages when err == NULL would have\nthe opposite effect: when there isn't a spare strbuf to put errors in\naround, it would be tempting for people coding in a hurry to just pass\nNULL, and to readers it would look at first glance like \"oh, an\noptional paramter was not passed and we are getting the good default\nbehavior\".\n\nThis is not a theoretical concern --- it actually happened.\n\nMy two cents,\nJonathan\n"},{"id":"252281","messageId":"CAL=YDWkZcoXBreoPyf1EnSmYcaUPFCPW8tGkCNaD+is2hOiR_g@mail.gmail.com","threadId":"38003","inReplyTo":"20141120183523.GD15945@google.com","subject":"Re: [PATCH] refs.c: repack_without_refs may be called without error string buffer","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-11-20T18:36:52Z","receivedAt":"2014-11-20T18:36:52Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"On Thu, Nov 20, 2014 at 10:35 AM, Jonathan Nieder wrote:\n> Stefan Beller wrote:\n>\n>> If we don't pass in the error string buffer, we skip over all\n>> parts dealing with preparing error messages.\n>\n> Please no.\n>\n> We tried this with the ref transaction code. When someone wants\n> to silence the message, it is cheap enough to do\n>\n> struct strbuf ignore = STRBUF_INIT;\n>\n> if (thing_that_can_fail_in_an_ignorable_way(..., &ignore)) {\n> ... handle the failure ...\n> }\n>\n> The extra lines of code make it obvious that the error message is\n> being dropped, which is a very good thing. The extra work to format a\n> message in the error case is not so bad and can be mitigated if the\n> error is a common normal case by passing a flag to not consider it an\n> error.\n>\n> Silently losing good diagnostic messages when err == NULL would have\n> the opposite effect: when there isn't a spare strbuf to put errors in\n> around, it would be tempting for people coding in a hurry to just pass\n> NULL, and to readers it would look at first glance like \"oh, an\n> optional paramter was not passed and we are getting the good default\n> behavior\".\n>\n> This is not a theoretical concern --- it actually happened.\n>\n\nFair enough.\nUn-LGTM my message above.\n\n\n\n> My two cents,\n> Jonathan\n"},{"id":"252283","messageId":"20141120183756.GE15945@google.com","threadId":"38003","inReplyTo":"1416506666-5989-1-git-send-email-sbeller@google.com","subject":"Re: [PATCH v5 1/1] refs.c: use a stringlist for repack_without_refs","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-11-20T18:37:57Z","receivedAt":"2014-11-20T18:37:57Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"On Thu, Nov 20, 2014 at 10:04:26AM -0800, Stefan Beller wrote:\n\n> [Subject: refs.c: use a stringlist for repack_without_refs]\n\nOne more nitpick. :)\n\ns/stringlist/string_list/\n\nThanks,\nJonathan\n"},{"id":"252285","messageId":"CAGZ79kZZfjqhRyFsyPmgBv5bz70TxmStXnHnAV8aMHessEfeWg@mail.gmail.com","threadId":"38003","inReplyTo":"CAL=YDWkZcoXBreoPyf1EnSmYcaUPFCPW8tGkCNaD+is2hOiR_g@mail.gmail.com","subject":"Re: [PATCH] refs.c: repack_without_refs may be called without error string buffer","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-11-20T18:56:02Z","receivedAt":"2014-11-20T18:56:02Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"ok, will drop the patch due to bad design.\n\nOn Thu, Nov 20, 2014 at 10:36 AM, Ronnie Sahlberg wrote:\n> On Thu, Nov 20, 2014 at 10:35 AM, Jonathan Nieder wrote:\n>> Stefan Beller wrote:\n>>\n>>> If we don't pass in the error string buffer, we skip over all\n>>> parts dealing with preparing error messages.\n>>\n>> Please no.\n>>\n>> We tried this with the ref transaction code. When someone wants\n>> to silence the message, it is cheap enough to do\n>>\n>> struct strbuf ignore = STRBUF_INIT;\n>>\n>> if (thing_that_can_fail_in_an_ignorable_way(..., &ignore)) {\n>> ... handle the failure ...\n>> }\n>>\n>> The extra lines of code make it obvious that the error message is\n>> being dropped, which is a very good thing. The extra work to format a\n>> message in the error case is not so bad and can be mitigated if the\n>> error is a common normal case by passing a flag to not consider it an\n>> error.\n>>\n>> Silently losing good diagnostic messages when err == NULL would have\n>> the opposite effect: when there isn't a spare strbuf to put errors in\n>> around, it would be tempting for people coding in a hurry to just pass\n>> NULL, and to readers it would look at first glance like \"oh, an\n>> optional paramter was not passed and we are getting the good default\n>> behavior\".\n>>\n>> This is not a theoretical concern --- it actually happened.\n>>\n>\n> Fair enough.\n> Un-LGTM my message above.\n>\n>\n>\n>> My two cents,\n>> Jonathan\n"},{"id":"252287","messageId":"xmqqppch3dde.fsf@gitster.dls.corp.google.com","threadId":"38003","inReplyTo":"1416506666-5989-1-git-send-email-sbeller@google.com","subject":"Re: [PATCH v5 1/1] refs.c: use a stringlist for repack_without_refs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-11-20T19:01:33Z","receivedAt":"2014-11-20T19:01:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> Junio, I'll address your proposed changes in a different patch. \n> If err is passed in as NULL, we'll just skip all the error string \n> formatting and return silent and fast.\n\nHuh, I lost track, but I never meant to say \"the functions should\nreturn silently with error code when err == NULL\". I said that it\nis another plausible expectation, hence justifies the comment to\nclarify, but wished that there were no need to clarify in the first\nplace.\n\nIf everybody required err != NULL, there would be no need to clarify\nwhich functions require err != NULL. If everybody accepted err ==\nNULL as a more efficient way to do \"--quiet\", that is another way to\nremove the need to clarify.\n\nEither way is fine and I did not \"propose\" anything ;-).\n\nI think this matches more-or-less what I've locally tweaked after\nfollowing the discussion between you and Jonathan. Thanks.\n"},{"id":"252288","messageId":"CAGZ79kYogEQuynukSkb9La+7DZxOQonAyuYD=kWB7KRdsXLHOA@mail.gmail.com","threadId":"38003","inReplyTo":"xmqqppch3dde.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v5 1/1] refs.c: use a stringlist for repack_without_refs","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-11-20T19:05:40Z","receivedAt":"2014-11-20T19:05:40Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Nov 20, 2014 at 11:01 AM, Junio C Hamano wrote:\n>\n> I think this matches more-or-less what I've locally tweaked after\n> following the discussion between you and Jonathan. Thanks.\n\nDo you want me to resend the patch with Jonathans nits fixed?\n\nJonathan wrote:\n>> Change-Id: Id7eaa821331f2ab89df063e1e76c8485dbcc3aed\n> Change-id snuck in.\n\nI need to get the format patch hook running, I was talking about\nearlier this week.\n"},{"id":"252292","messageId":"1416514066-17049-1-git-send-email-sbeller@google.com","threadId":"38003","inReplyTo":"CAGZ79kYogEQuynukSkb9La+7DZxOQonAyuYD=kWB7KRdsXLHOA@mail.gmail.com","subject":"[PATCH v6] refs.c: use a string_list for repack_without_refs","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-11-20T20:07:46Z","receivedAt":"2014-11-20T20:07:46Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"From: Ronnie Sahlberg \n\nThis patch doesn't intend any functional changes. It is just\na refactoring, which replaces a char** array by a stringlist\nin the function repack_without_refs.\n\nThis is easier to read and maintain as it delivers the same\nfunctionality with less lines of code and more lines of\ndocumentation.\n\n[sb: ported this patch from a larger patch series to the\nmaster branch, added documentary comments in refs.h]\n\nSigned-off-by: Ronnie Sahlberg \nSigned-off-by: Stefan Beller \nReviewed-by: Jonathan Nieder \n---\n\nChanges to version 1:\n * removed the double blank line\n * rename delete_refs_list to delete_refs\n * add back comments dropped by accident\n * use STRING_LIST_INIT_NODUP instead of the _DUP version in ref_transaction_commit\n * add documentary comments on the repack_without_refs function\n * user string_list_append instead of string_list_insert as it follows the previous\n behavior more closely.\n * put back the early exit of the loop in repack_without_refs\n \nChanges to version 2:\n * fixed commit message (comments after the three dashes)\n * fixed the curiosity Junio pointed out as it was just wrong code.\n Now it actually builds a list of all states.stale.items[i].util items.\n \nChanges in version 3:\n\n * reword commit message\n * sort delete_refs before passing it to warn_dangling_symrefs\n * change the comments (get back the one jrn complained about) \n in repack_without_refs\n * use the suggestion of jonathan for documenting repack_without_refs in the\n header. Add a note about the arguments.\n\nChanges in version 4:\n * I lied, when saying I had all the nits from Jonathan.\n I messed up the documentation in the header.\n This includes the documentary comment in the header.\n\nChanges in version 5:\n * Break lines as suggested by Jonathan, slightly rewording the commit message\n * have an empty line at another place in builtin/remote.c remove_branches to\n tell cleanup parts apart from actual work.\n * fix typo, improve documentary comment in refs.h\n * add Jonathans reviewed by\n\nChanges in version 6:\n * remove change id snuck in v5\n * s/stringlist/string_list/ in commit message title\n * reworded the documentary comment in refs.h once again\n\n\n builtin/remote.c | 32 ++++++++++++--------------------\n refs.c | 38 ++++++++++++++++++++------------------\n refs.h | 12 ++++++++++--\n 3 files changed, 42 insertions(+), 40 deletions(-)\n\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex 7f28f92..364350a 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -750,16 +750,11 @@ static int mv(int argc, const char **argv)\n static int remove_branches(struct string_list *branches)\n {\n \tstruct strbuf err = STRBUF_INIT;\n-\tconst char **branch_names;\n \tint i, result = 0;\n \n-\tbranch_names = xmalloc(branches->nr * sizeof(*branch_names));\n-\tfor (i = 0; i < branches->nr; i++)\n-\t\tbranch_names[i] = branches->items[i].string;\n-\tif (repack_without_refs(branch_names, branches->nr, &err))\n+\tif (repack_without_refs(branches, &err))\n \t\tresult |= error(\"%s\", err.buf);\n \tstrbuf_release(&err);\n-\tfree(branch_names);\n \n \tfor (i = 0; i < branches->nr; i++) {\n \t\tstruct string_list_item *item = branches->items + i;\n@@ -1316,8 +1311,8 @@ static int prune_remote(const char *remote, int dry_run)\n {\n \tint result = 0, i;\n \tstruct ref_states states;\n-\tstruct string_list delete_refs_list = STRING_LIST_INIT_NODUP;\n-\tconst char **delete_refs;\n+\tstruct string_list delete_refs = STRING_LIST_INIT_NODUP;\n+\tstruct string_list_item *ref;\n \tconst char *dangling_msg = dry_run\n \t\t? _(\" %s will become dangling!\")\n \t\t: _(\" %s has become dangling!\");\n@@ -1325,6 +1320,9 @@ static int prune_remote(const char *remote, int dry_run)\n \tmemset(&states, 0, sizeof(states));\n \tget_remote_ref_states(remote, &states, GET_REF_STATES);\n \n+\tfor (i = 0; i < states.stale.nr; i++)\n+\t\tstring_list_append(&delete_refs, states.stale.items[i].util);\n+\n \tif (states.stale.nr) {\n \t\tprintf_ln(_(\"Pruning %s\"), remote);\n \t\tprintf_ln(_(\"URL: %s\"),\n@@ -1332,23 +1330,16 @@ static int prune_remote(const char *remote, int dry_run)\n \t\t ? states.remote->url[0]\n \t\t : _(\"(no URL)\"));\n \n-\t\tdelete_refs = xmalloc(states.stale.nr * sizeof(*delete_refs));\n-\t\tfor (i = 0; i < states.stale.nr; i++)\n-\t\t\tdelete_refs[i] = states.stale.items[i].util;\n \t\tif (!dry_run) {\n \t\t\tstruct strbuf err = STRBUF_INIT;\n-\t\t\tif (repack_without_refs(delete_refs, states.stale.nr,\n-\t\t\t\t\t\t&err))\n+\t\t\tif (repack_without_refs(&delete_refs, &err))\n \t\t\t\tresult |= error(\"%s\", err.buf);\n \t\t\tstrbuf_release(&err);\n \t\t}\n-\t\tfree(delete_refs);\n \t}\n \n-\tfor (i = 0; i < states.stale.nr; i++) {\n-\t\tconst char *refname = states.stale.items[i].util;\n-\n-\t\tstring_list_insert(&delete_refs_list, refname);\n+\tfor_each_string_list_item(ref, &delete_refs) {\n+\t\tconst char *refname = ref->string;\n \n \t\tif (!dry_run)\n \t\t\tresult |= delete_ref(refname, NULL, 0);\n@@ -1361,9 +1352,10 @@ static int prune_remote(const char *remote, int dry_run)\n \t\t\t abbrev_ref(refname, \"refs/remotes/\"));\n \t}\n \n-\twarn_dangling_symrefs(stdout, dangling_msg, &delete_refs_list);\n-\tstring_list_clear(&delete_refs_list, 0);\n+\tsort_string_list(&delete_refs);\n+\twarn_dangling_symrefs(stdout, dangling_msg, &delete_refs);\n \n+\tstring_list_clear(&delete_refs, 0);\n \tfree_remote_ref_states(&states);\n \treturn result;\n }\ndiff --git a/refs.c b/refs.c\nindex 5ff457e..ebcd90f 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2639,23 +2639,26 @@ static int curate_packed_ref_fn(struct ref_entry *entry, void *cb_data)\n \treturn 0;\n }\n \n-int repack_without_refs(const char **refnames, int n, struct strbuf *err)\n+int repack_without_refs(struct string_list *without, struct strbuf *err)\n {\n \tstruct ref_dir *packed;\n \tstruct string_list refs_to_delete = STRING_LIST_INIT_DUP;\n \tstruct string_list_item *ref_to_delete;\n-\tint i, ret, removed = 0;\n+\tint ret, needs_repacking = 0, removed = 0;\n \n \tassert(err);\n \n \t/* Look for a packed ref */\n-\tfor (i = 0; i < n; i++)\n-\t\tif (get_packed_ref(refnames[i]))\n+\tfor_each_string_list_item(ref_to_delete, without) {\n+\t\tif (get_packed_ref(ref_to_delete->string)) {\n+\t\t\tneeds_repacking = 1;\n \t\t\tbreak;\n+\t\t}\n+\t}\n \n \t/* Avoid locking if we have nothing to do */\n-\tif (i == n)\n-\t\treturn 0; /* no refname exists in packed refs */\n+\tif (!needs_repacking)\n+\t\treturn 0;\n \n \tif (lock_packed_refs(0)) {\n \t\tunable_to_lock_message(git_path(\"packed-refs\"), errno, err);\n@@ -2664,8 +2667,8 @@ int repack_without_refs(const char **refnames, int n, struct strbuf *err)\n \tpacked = get_packed_refs(&ref_cache);\n \n \t/* Remove refnames from the cache */\n-\tfor (i = 0; i < n; i++)\n-\t\tif (remove_entry(packed, refnames[i]) != -1)\n+\tfor_each_string_list_item(ref_to_delete, without)\n+\t\tif (remove_entry(packed, ref_to_delete->string) != -1)\n \t\t\tremoved = 1;\n \tif (!removed) {\n \t\t/*\n@@ -3738,10 +3741,11 @@ static int ref_update_reject_duplicates(struct ref_update **updates, int n,\n int ref_transaction_commit(struct ref_transaction *transaction,\n \t\t\t struct strbuf *err)\n {\n-\tint ret = 0, delnum = 0, i;\n-\tconst char **delnames;\n+\tint ret = 0, i;\n \tint n = transaction->nr;\n \tstruct ref_update **updates = transaction->updates;\n+\tstruct string_list refs_to_delete = STRING_LIST_INIT_NODUP;\n+\tstruct string_list_item *ref_to_delete;\n \n \tassert(err);\n \n@@ -3753,9 +3757,6 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \t\treturn 0;\n \t}\n \n-\t/* Allocate work space */\n-\tdelnames = xmalloc(sizeof(*delnames) * n);\n-\n \t/* Copy, sort, and reject duplicate refs */\n \tqsort(updates, n, sizeof(*updates), ref_update_compare);\n \tif (ref_update_reject_duplicates(updates, n, err)) {\n@@ -3815,16 +3816,17 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \t\t\t}\n \n \t\t\tif (!(update->flags & REF_ISPRUNING))\n-\t\t\t\tdelnames[delnum++] = update->lock->ref_name;\n+\t\t\t\tstring_list_append(&refs_to_delete,\n+\t\t\t\t\t\t update->lock->ref_name);\n \t\t}\n \t}\n \n-\tif (repack_without_refs(delnames, delnum, err)) {\n+\tif (repack_without_refs(&refs_to_delete, err)) {\n \t\tret = TRANSACTION_GENERIC_ERROR;\n \t\tgoto cleanup;\n \t}\n-\tfor (i = 0; i < delnum; i++)\n-\t\tunlink_or_warn(git_path(\"logs/%s\", delnames[i]));\n+\tfor_each_string_list_item(ref_to_delete, &refs_to_delete)\n+\t\tunlink_or_warn(git_path(\"logs/%s\", ref_to_delete->string));\n \tclear_loose_ref_cache(&ref_cache);\n \n cleanup:\n@@ -3833,7 +3835,7 @@ cleanup:\n \tfor (i = 0; i < n; i++)\n \t\tif (updates[i]->lock)\n \t\t\tunlock_ref(updates[i]->lock);\n-\tfree(delnames);\n+\tstring_list_clear(&refs_to_delete, 0);\n \treturn ret;\n }\n \ndiff --git a/refs.h b/refs.h\nindex 2bc3556..0bc6a1a 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -163,8 +163,16 @@ extern void rollback_packed_refs(void);\n */\n int pack_refs(unsigned int flags);\n \n-extern int repack_without_refs(const char **refnames, int n,\n-\t\t\t struct strbuf *err);\n+/*\n+ * Remove the refs listed in the unsorted string list 'without' from\n+ * the packed-refs file. On error, packed-refs will be unchanged, the\n+ * return value is nonzero, and a message about the error is written\n+ * to the 'err' strbuf.\n+ *\n+ * The refs in 'without' may be unsorted.\n+ * 'err' must not be NULL.\n+ */\n+extern int repack_without_refs(struct string_list *without, struct strbuf *err);\n \n extern int ref_exists(const char *);\n \n-- \n2.2.0.rc2.23.gca0107e\n"},{"id":"252295","messageId":"20141120203648.GI6527@google.com","threadId":"38003","inReplyTo":"1416514066-17049-1-git-send-email-sbeller@google.com","subject":"Re: [PATCH v6] refs.c: use a string_list for repack_without_refs","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-11-20T20:36:48Z","receivedAt":"2014-11-20T20:36:48Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Stefan Beller wrote:\n\n> Signed-off-by: Ronnie Sahlberg \n> Signed-off-by: Stefan Beller \n> Reviewed-by: Jonathan Nieder \n> ---\n\nYep, looks good now. Thanks for bearing with me.\n\n[...]\n> +++ b/refs.h\n> @@ -163,8 +163,16 @@ extern void rollback_packed_refs(void);\n[...]\n> +/*\n> + * Remove the refs listed in the unsorted string list 'without' from\n> + * the packed-refs file. On error, packed-refs will be unchanged, the\n> + * return value is nonzero, and a message about the error is written\n> + * to the 'err' strbuf.\n> + *\n> + * The refs in 'without' may be unsorted.\n> + * 'err' must not be NULL.\n\nI think we've gone back and forth enough on this text and it's not\nworth the transactional cost to tweak it further, so I'm not\nsuggesting a change --- just explaining how I read it for future\nreference.\n\n\"may be unsorted\" is confusing to me. It sounds like the reader of\nthis comment (someone calling repack_without_refs) has to be prepared\nfor that possibility. But we are saying the opposite --- not \"be\nprepared\", but \"don't worry about sorting 'without', since\nrepack_without_refs can handle it\".\n\nIt's also redundant, since the paragraph above already says that\n'without' is an unsorted string list.\n\nThe way I see it, there are four types that for various reasons (lack\nof language-level support for subclassing, etc) are conflated into a\nsingle struct in the string-list API:\n\n * sorted string list that owns its items (i.e., created with DUP)\n * sorted string list that does not own its items (i.e., created with NODUP)\n * unsorted string list that owns its items\n * unsorted string list that does not own its items\n\nDifferent functions are valid to call on each type, as documented in\nthe comments in string-list.h.\n\nrepack_without_refs accepts all 4 types of string-list. That's what\nit means when the documentation says its argument is unsorted.\n\nThanks,\nJonathan\n"},{"id":"252337","messageId":"1416578950-23210-1-git-send-email-mhagger@alum.mit.edu","threadId":"38003","inReplyTo":"1416423000-4323-1-git-send-email-sbeller@google.com","subject":"[PATCH 0/6] repack_without_refs(): convert to string_list","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-11-21T14:09:04Z","receivedAt":"2014-11-21T14:09:04Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"This is basically an atomized version of Ronnie/Jonathan/Stefan's\npatch [1] \"refs.c: use a stringlist for repack_without_refs\". But I've\nactually rewritten most of it from scratch, using the original patch\nas a reference.\n\nI was reviewing the original patch and it looked mostly OK [2], but I\nfound it hard to read because it did several steps at once. So I tried\nto make the same basic change, but one baby step at a time. This is\nthe result.\n\nI'm a known fanatic about making the smallest possible changes in each\ncommit. The goal is to make the patch series as readable as possible,\nbecause reviewers' time is in shorter supply than coders' time.\n\n* Tiny little patches are IMO usually much easier to read than big\n ones, because there is less to keep in mind at a time.\n\n* Often tiny changes (e.g., renaming variables or functions) are so\n blindingly obvious that one only has to skim them, or even trust\n that the author, with the help of the compiler, could hardly have\n made a mistake [3].\n\n* Using baby steps keeps the author from introducing unnecessary\n changes (\"code churn\"), by forcing him/her to justify each change on\n its own merits.\n\n* Using baby steps makes it harder for substantive changes to get\n overlooked or to sneak in without discussion [4].\n\n* If there is a problem, baby commits can be bisected, usually making\n it obvious why the bug arose.\n\n* If the mailing list doesn't like part of the series, it is usually\n easier to omit a patch from the next reroll than to extract one\n change out of a patch that contains multiple logical changes.\n\n* It is often possible to arrange the order of the patches to give the\n patch series a good \"narrative\".\n\nSome members of the community probably disagree with me. Using baby\nstep patches means that there is more mailing list traffic and more\ncommits that accumulate in the project's history. There is sometimes a\nbit of extra to-and-fro as code is mutated incrementally. Or maybe\nother people can just keep more complicated changes in their heads at\none time than I can.\n\nNevertheless, I submit this version of the patch series for your\namusement. Feel free to ignore it.\n\n[1] http://mid.gmane.org/1416434399-2303-1-git-send-email-sbeller@google.com\n[2] Problems that I noticed:\n\n * The commit message refers to \"stringlist\" where it should be\n \"string_list\".\n\n * One of the loops in prune_remote() iterates using indexes, while\n another loop (over the same string_list) uses\n for_each_string_list_item().\n\n * The change from using string_list_insert() to string_list_append()\n in the same function, followed by sort_string_list(), doesn't remove\n duplicates as the old version did. The commit message should\n justify that this is OK.\n\n[3] I love the quote from C. A. R. Hoare:\n\n There are two ways of constructing a software design: One way\n is to make it so simple that there are obviously no\n deficiencies, and the other way is to make it so complicated\n that there are no obvious deficiencies.\n\n I think the same thing applies to patches.\n\n[4] Case in point: when I was writing the commit message for patch\n 3/6, I realized that string_list_insert() omits duplicates whereas\n string_list_append() obviously doesn't. This aspect of the change\n wasn't justified. Do we have to add a call to\n string_list_remove_duplicates()? It turns out that the list cannot\n contain duplicates, but it took some digging to verify this.\n\nMichael Haggerty (6):\n prune_remote(): exit early if there are no stale references\n prune_remote(): initialize both delete_refs lists in a single loop\n prune_remote(): sort delete_refs_list references en masse\n repack_without_refs(): make the refnames argument a string_list\n prune_remote(): rename local variable\n prune_remote(): iterate using for_each_string_list_item()\n\n builtin/remote.c | 59 ++++++++++++++++++++++++++------------------------------\n refs.c | 38 +++++++++++++++++++-----------------\n refs.h | 11 ++++++++++-\n 3 files changed, 57 insertions(+), 51 deletions(-)\n\n-- \n2.1.3\n"},{"id":"252334","messageId":"1416578950-23210-2-git-send-email-mhagger@alum.mit.edu","threadId":"38003","inReplyTo":"1416578950-23210-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 1/6] prune_remote(): exit early if there are no stale references","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-11-21T14:09:05Z","receivedAt":"2014-11-21T14:09:05Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Aside from making the logic clearer, this avoids a call to\nwarn_dangling_symrefs(), which always does a for_each_rawref()\niteration.\n\nSigned-off-by: Michael Haggerty \n---\n builtin/remote.c | 39 +++++++++++++++++++++------------------\n 1 file changed, 21 insertions(+), 18 deletions(-)\n\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex 7f28f92..d2b684c 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -1325,25 +1325,28 @@ static int prune_remote(const char *remote, int dry_run)\n \tmemset(&states, 0, sizeof(states));\n \tget_remote_ref_states(remote, &states, GET_REF_STATES);\n \n-\tif (states.stale.nr) {\n-\t\tprintf_ln(_(\"Pruning %s\"), remote);\n-\t\tprintf_ln(_(\"URL: %s\"),\n-\t\t states.remote->url_nr\n-\t\t ? states.remote->url[0]\n-\t\t : _(\"(no URL)\"));\n-\n-\t\tdelete_refs = xmalloc(states.stale.nr * sizeof(*delete_refs));\n-\t\tfor (i = 0; i < states.stale.nr; i++)\n-\t\t\tdelete_refs[i] = states.stale.items[i].util;\n-\t\tif (!dry_run) {\n-\t\t\tstruct strbuf err = STRBUF_INIT;\n-\t\t\tif (repack_without_refs(delete_refs, states.stale.nr,\n-\t\t\t\t\t\t&err))\n-\t\t\t\tresult |= error(\"%s\", err.buf);\n-\t\t\tstrbuf_release(&err);\n-\t\t}\n-\t\tfree(delete_refs);\n+\tif (!states.stale.nr) {\n+\t\tfree_remote_ref_states(&states);\n+\t\treturn 0;\n+\t}\n+\n+\tprintf_ln(_(\"Pruning %s\"), remote);\n+\tprintf_ln(_(\"URL: %s\"),\n+\t\t states.remote->url_nr\n+\t\t ? states.remote->url[0]\n+\t\t : _(\"(no URL)\"));\n+\n+\tdelete_refs = xmalloc(states.stale.nr * sizeof(*delete_refs));\n+\tfor (i = 0; i < states.stale.nr; i++)\n+\t\tdelete_refs[i] = states.stale.items[i].util;\n+\tif (!dry_run) {\n+\t\tstruct strbuf err = STRBUF_INIT;\n+\t\tif (repack_without_refs(delete_refs, states.stale.nr,\n+\t\t\t\t\t&err))\n+\t\t\tresult |= error(\"%s\", err.buf);\n+\t\tstrbuf_release(&err);\n \t}\n+\tfree(delete_refs);\n \n \tfor (i = 0; i < states.stale.nr; i++) {\n \t\tconst char *refname = states.stale.items[i].util;\n-- \n2.1.3\n"},{"id":"252338","messageId":"1416578950-23210-3-git-send-email-mhagger@alum.mit.edu","threadId":"38003","inReplyTo":"1416578950-23210-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 2/6] prune_remote(): initialize both delete_refs lists in a single loop","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-11-21T14:09:06Z","receivedAt":"2014-11-21T14:09:06Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Also free them together at the end of the function.\n\nIn a moment, the array version will become redundant. Managing them\ntogether makes later steps more obvious.\n\nSigned-off-by: Michael Haggerty \n---\n builtin/remote.c | 15 +++++++++------\n 1 file changed, 9 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex d2b684c..d5a5a16 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -1337,8 +1337,13 @@ static int prune_remote(const char *remote, int dry_run)\n \t\t : _(\"(no URL)\"));\n \n \tdelete_refs = xmalloc(states.stale.nr * sizeof(*delete_refs));\n-\tfor (i = 0; i < states.stale.nr; i++)\n-\t\tdelete_refs[i] = states.stale.items[i].util;\n+\tfor (i = 0; i < states.stale.nr; i++) {\n+\t\tconst char *refname = states.stale.items[i].util;\n+\n+\t\tdelete_refs[i] = refname;\n+\t\tstring_list_insert(&delete_refs_list, refname);\n+\t}\n+\n \tif (!dry_run) {\n \t\tstruct strbuf err = STRBUF_INIT;\n \t\tif (repack_without_refs(delete_refs, states.stale.nr,\n@@ -1346,13 +1351,10 @@ static int prune_remote(const char *remote, int dry_run)\n \t\t\tresult |= error(\"%s\", err.buf);\n \t\tstrbuf_release(&err);\n \t}\n-\tfree(delete_refs);\n \n \tfor (i = 0; i < states.stale.nr; i++) {\n \t\tconst char *refname = states.stale.items[i].util;\n \n-\t\tstring_list_insert(&delete_refs_list, refname);\n-\n \t\tif (!dry_run)\n \t\t\tresult |= delete_ref(refname, NULL, 0);\n \n@@ -1365,8 +1367,9 @@ static int prune_remote(const char *remote, int dry_run)\n \t}\n \n \twarn_dangling_symrefs(stdout, dangling_msg, &delete_refs_list);\n-\tstring_list_clear(&delete_refs_list, 0);\n \n+\tfree(delete_refs);\n+\tstring_list_clear(&delete_refs_list, 0);\n \tfree_remote_ref_states(&states);\n \treturn result;\n }\n-- \n2.1.3\n"},{"id":"252340","messageId":"1416578950-23210-4-git-send-email-mhagger@alum.mit.edu","threadId":"38003","inReplyTo":"1416578950-23210-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 3/6] prune_remote(): sort delete_refs_list references en masse","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-11-21T14:09:07Z","receivedAt":"2014-11-21T14:09:07Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Inserting items into a list in sorted order is O(N^2) whereas\nappending them unsorted and then sorting the list all at once is\nO(N lg N).\n\nstring_list_insert() also removes duplicates, and this change loses\nthat functionality. But the strings in this list, which ultimately\ncome from a for_each_ref() iteration, cannot contain duplicates.\n\nSigned-off-by: Michael Haggerty \n---\n builtin/remote.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex d5a5a16..7d5c8d2 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -1341,8 +1341,9 @@ static int prune_remote(const char *remote, int dry_run)\n \t\tconst char *refname = states.stale.items[i].util;\n \n \t\tdelete_refs[i] = refname;\n-\t\tstring_list_insert(&delete_refs_list, refname);\n+\t\tstring_list_append(&delete_refs_list, refname);\n \t}\n+\tsort_string_list(&delete_refs_list);\n \n \tif (!dry_run) {\n \t\tstruct strbuf err = STRBUF_INIT;\n-- \n2.1.3\n"},{"id":"252336","messageId":"1416578950-23210-5-git-send-email-mhagger@alum.mit.edu","threadId":"38003","inReplyTo":"1416578950-23210-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 4/6] repack_without_refs(): make the refnames argument a string_list","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-11-21T14:09:08Z","receivedAt":"2014-11-21T14:09:08Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"All of the callers have string_lists available already, whereas two of\nthem had to read data out of a string_list into an array of strings\njust to call this function. So change repack_without_refs() to take\nthe list of refnames to omit as a string_list, and change the callers\naccordingly.\n\nSuggested-by: Ronnie Sahlberg \nSigned-off-by: Michael Haggerty \n---\n builtin/remote.c | 14 ++------------\n refs.c | 38 ++++++++++++++++++++------------------\n refs.h | 11 ++++++++++-\n 3 files changed, 32 insertions(+), 31 deletions(-)\n\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex 7d5c8d2..63a6709 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -750,16 +750,11 @@ static int mv(int argc, const char **argv)\n static int remove_branches(struct string_list *branches)\n {\n \tstruct strbuf err = STRBUF_INIT;\n-\tconst char **branch_names;\n \tint i, result = 0;\n \n-\tbranch_names = xmalloc(branches->nr * sizeof(*branch_names));\n-\tfor (i = 0; i < branches->nr; i++)\n-\t\tbranch_names[i] = branches->items[i].string;\n-\tif (repack_without_refs(branch_names, branches->nr, &err))\n+\tif (repack_without_refs(branches, &err))\n \t\tresult |= error(\"%s\", err.buf);\n \tstrbuf_release(&err);\n-\tfree(branch_names);\n \n \tfor (i = 0; i < branches->nr; i++) {\n \t\tstruct string_list_item *item = branches->items + i;\n@@ -1317,7 +1312,6 @@ static int prune_remote(const char *remote, int dry_run)\n \tint result = 0, i;\n \tstruct ref_states states;\n \tstruct string_list delete_refs_list = STRING_LIST_INIT_NODUP;\n-\tconst char **delete_refs;\n \tconst char *dangling_msg = dry_run\n \t\t? _(\" %s will become dangling!\")\n \t\t: _(\" %s has become dangling!\");\n@@ -1336,19 +1330,16 @@ static int prune_remote(const char *remote, int dry_run)\n \t\t ? states.remote->url[0]\n \t\t : _(\"(no URL)\"));\n \n-\tdelete_refs = xmalloc(states.stale.nr * sizeof(*delete_refs));\n \tfor (i = 0; i < states.stale.nr; i++) {\n \t\tconst char *refname = states.stale.items[i].util;\n \n-\t\tdelete_refs[i] = refname;\n \t\tstring_list_append(&delete_refs_list, refname);\n \t}\n \tsort_string_list(&delete_refs_list);\n \n \tif (!dry_run) {\n \t\tstruct strbuf err = STRBUF_INIT;\n-\t\tif (repack_without_refs(delete_refs, states.stale.nr,\n-\t\t\t\t\t&err))\n+\t\tif (repack_without_refs(&delete_refs_list, &err))\n \t\t\tresult |= error(\"%s\", err.buf);\n \t\tstrbuf_release(&err);\n \t}\n@@ -1369,7 +1360,6 @@ static int prune_remote(const char *remote, int dry_run)\n \n \twarn_dangling_symrefs(stdout, dangling_msg, &delete_refs_list);\n \n-\tfree(delete_refs);\n \tstring_list_clear(&delete_refs_list, 0);\n \tfree_remote_ref_states(&states);\n \treturn result;\ndiff --git a/refs.c b/refs.c\nindex 5ff457e..b675e01 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2639,22 +2639,25 @@ static int curate_packed_ref_fn(struct ref_entry *entry, void *cb_data)\n \treturn 0;\n }\n \n-int repack_without_refs(const char **refnames, int n, struct strbuf *err)\n+int repack_without_refs(struct string_list *refnames, struct strbuf *err)\n {\n \tstruct ref_dir *packed;\n \tstruct string_list refs_to_delete = STRING_LIST_INIT_DUP;\n-\tstruct string_list_item *ref_to_delete;\n-\tint i, ret, removed = 0;\n+\tstruct string_list_item *refname, *ref_to_delete;\n+\tint ret, needs_repacking = 0, removed = 0;\n \n \tassert(err);\n \n \t/* Look for a packed ref */\n-\tfor (i = 0; i < n; i++)\n-\t\tif (get_packed_ref(refnames[i]))\n+\tfor_each_string_list_item(refname, refnames) {\n+\t\tif (get_packed_ref(refname->string)) {\n+\t\t\tneeds_repacking = 1;\n \t\t\tbreak;\n+\t\t}\n+\t}\n \n \t/* Avoid locking if we have nothing to do */\n-\tif (i == n)\n+\tif (!needs_repacking)\n \t\treturn 0; /* no refname exists in packed refs */\n \n \tif (lock_packed_refs(0)) {\n@@ -2664,8 +2667,8 @@ int repack_without_refs(const char **refnames, int n, struct strbuf *err)\n \tpacked = get_packed_refs(&ref_cache);\n \n \t/* Remove refnames from the cache */\n-\tfor (i = 0; i < n; i++)\n-\t\tif (remove_entry(packed, refnames[i]) != -1)\n+\tfor_each_string_list_item(refname, refnames)\n+\t\tif (remove_entry(packed, refname->string) != -1)\n \t\t\tremoved = 1;\n \tif (!removed) {\n \t\t/*\n@@ -3738,10 +3741,11 @@ static int ref_update_reject_duplicates(struct ref_update **updates, int n,\n int ref_transaction_commit(struct ref_transaction *transaction,\n \t\t\t struct strbuf *err)\n {\n-\tint ret = 0, delnum = 0, i;\n-\tconst char **delnames;\n+\tint ret = 0, i;\n \tint n = transaction->nr;\n \tstruct ref_update **updates = transaction->updates;\n+\tstruct string_list refs_to_delete = STRING_LIST_INIT_NODUP;\n+\tstruct string_list_item *ref_to_delete;\n \n \tassert(err);\n \n@@ -3753,9 +3757,6 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \t\treturn 0;\n \t}\n \n-\t/* Allocate work space */\n-\tdelnames = xmalloc(sizeof(*delnames) * n);\n-\n \t/* Copy, sort, and reject duplicate refs */\n \tqsort(updates, n, sizeof(*updates), ref_update_compare);\n \tif (ref_update_reject_duplicates(updates, n, err)) {\n@@ -3815,16 +3816,17 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \t\t\t}\n \n \t\t\tif (!(update->flags & REF_ISPRUNING))\n-\t\t\t\tdelnames[delnum++] = update->lock->ref_name;\n+\t\t\t\tstring_list_append(&refs_to_delete,\n+\t\t\t\t\t\t update->lock->ref_name);\n \t\t}\n \t}\n \n-\tif (repack_without_refs(delnames, delnum, err)) {\n+\tif (repack_without_refs(&refs_to_delete, err)) {\n \t\tret = TRANSACTION_GENERIC_ERROR;\n \t\tgoto cleanup;\n \t}\n-\tfor (i = 0; i < delnum; i++)\n-\t\tunlink_or_warn(git_path(\"logs/%s\", delnames[i]));\n+\tfor_each_string_list_item(ref_to_delete, &refs_to_delete)\n+\t\tunlink_or_warn(git_path(\"logs/%s\", ref_to_delete->string));\n \tclear_loose_ref_cache(&ref_cache);\n \n cleanup:\n@@ -3833,7 +3835,7 @@ cleanup:\n \tfor (i = 0; i < n; i++)\n \t\tif (updates[i]->lock)\n \t\t\tunlock_ref(updates[i]->lock);\n-\tfree(delnames);\n+\tstring_list_clear(&refs_to_delete, 0);\n \treturn ret;\n }\n \ndiff --git a/refs.h b/refs.h\nindex 2bc3556..90a4a40 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -163,7 +163,16 @@ extern void rollback_packed_refs(void);\n */\n int pack_refs(unsigned int flags);\n \n-extern int repack_without_refs(const char **refnames, int n,\n+/*\n+ * Remove the refs listed in 'refnames' from the packed-refs file.\n+ * On error, packed-refs will be unchanged, the return value is\n+ * nonzero, and a message about the error is written to the 'err'\n+ * strbuf.\n+ *\n+ * The refs in 'refnames' needn't be sorted. The err buffer must not be\n+ * omitted.\n+ */\n+extern int repack_without_refs(struct string_list *refnames,\n \t\t\t struct strbuf *err);\n \n extern int ref_exists(const char *);\n-- \n2.1.3\n"},{"id":"252335","messageId":"1416578950-23210-6-git-send-email-mhagger@alum.mit.edu","threadId":"38003","inReplyTo":"1416578950-23210-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 5/6] prune_remote(): rename local variable","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-11-21T14:09:09Z","receivedAt":"2014-11-21T14:09:09Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Rename \"delete_refs_list\" to \"refs_to_prune\". The new name is more\nself-explanatory.\n\nSigned-off-by: Michael Haggerty \n---\n builtin/remote.c | 12 ++++++------\n 1 file changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex 63a6709..efbf5fb 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -1311,7 +1311,7 @@ static int prune_remote(const char *remote, int dry_run)\n {\n \tint result = 0, i;\n \tstruct ref_states states;\n-\tstruct string_list delete_refs_list = STRING_LIST_INIT_NODUP;\n+\tstruct string_list refs_to_prune = STRING_LIST_INIT_NODUP;\n \tconst char *dangling_msg = dry_run\n \t\t? _(\" %s will become dangling!\")\n \t\t: _(\" %s has become dangling!\");\n@@ -1333,13 +1333,13 @@ static int prune_remote(const char *remote, int dry_run)\n \tfor (i = 0; i < states.stale.nr; i++) {\n \t\tconst char *refname = states.stale.items[i].util;\n \n-\t\tstring_list_append(&delete_refs_list, refname);\n+\t\tstring_list_append(&refs_to_prune, refname);\n \t}\n-\tsort_string_list(&delete_refs_list);\n+\tsort_string_list(&refs_to_prune);\n \n \tif (!dry_run) {\n \t\tstruct strbuf err = STRBUF_INIT;\n-\t\tif (repack_without_refs(&delete_refs_list, &err))\n+\t\tif (repack_without_refs(&refs_to_prune, &err))\n \t\t\tresult |= error(\"%s\", err.buf);\n \t\tstrbuf_release(&err);\n \t}\n@@ -1358,9 +1358,9 @@ static int prune_remote(const char *remote, int dry_run)\n \t\t\t abbrev_ref(refname, \"refs/remotes/\"));\n \t}\n \n-\twarn_dangling_symrefs(stdout, dangling_msg, &delete_refs_list);\n+\twarn_dangling_symrefs(stdout, dangling_msg, &refs_to_prune);\n \n-\tstring_list_clear(&delete_refs_list, 0);\n+\tstring_list_clear(&refs_to_prune, 0);\n \tfree_remote_ref_states(&states);\n \treturn result;\n }\n-- \n2.1.3\n"},{"id":"252339","messageId":"1416578950-23210-7-git-send-email-mhagger@alum.mit.edu","threadId":"38003","inReplyTo":"1416578950-23210-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 6/6] prune_remote(): iterate using for_each_string_list_item()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-11-21T14:09:10Z","receivedAt":"2014-11-21T14:09:10Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Iterate over refs_to_prune using for_each_string_list_item() rather\nthan writing out the loop in longhand.\n\nSigned-off-by: Michael Haggerty \n---\n builtin/remote.c | 14 ++++++--------\n 1 file changed, 6 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex efbf5fb..7fec170 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -1309,9 +1309,10 @@ static int set_head(int argc, const char **argv)\n \n static int prune_remote(const char *remote, int dry_run)\n {\n-\tint result = 0, i;\n+\tint result = 0;\n \tstruct ref_states states;\n \tstruct string_list refs_to_prune = STRING_LIST_INIT_NODUP;\n+\tstruct string_list_item *item;\n \tconst char *dangling_msg = dry_run\n \t\t? _(\" %s will become dangling!\")\n \t\t: _(\" %s has become dangling!\");\n@@ -1330,11 +1331,8 @@ static int prune_remote(const char *remote, int dry_run)\n \t\t ? states.remote->url[0]\n \t\t : _(\"(no URL)\"));\n \n-\tfor (i = 0; i < states.stale.nr; i++) {\n-\t\tconst char *refname = states.stale.items[i].util;\n-\n-\t\tstring_list_append(&refs_to_prune, refname);\n-\t}\n+\tfor_each_string_list_item(item, &states.stale)\n+\t\tstring_list_append(&refs_to_prune, item->util);\n \tsort_string_list(&refs_to_prune);\n \n \tif (!dry_run) {\n@@ -1344,8 +1342,8 @@ static int prune_remote(const char *remote, int dry_run)\n \t\tstrbuf_release(&err);\n \t}\n \n-\tfor (i = 0; i < states.stale.nr; i++) {\n-\t\tconst char *refname = states.stale.items[i].util;\n+\tfor_each_string_list_item(item, &states.stale) {\n+\t\tconst char *refname = item->util;\n \n \t\tif (!dry_run)\n \t\t\tresult |= delete_ref(refname, NULL, 0);\n-- \n2.1.3\n"},{"id":"252341","messageId":"546F4B5B.2060508@alum.mit.edu","threadId":"38003","inReplyTo":"1416578950-23210-1-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 0/6] repack_without_refs(): convert to string_list","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-11-21T14:25:31Z","receivedAt":"2014-11-21T14:25:31Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 11/21/2014 03:09 PM, Michael Haggerty wrote:\n> This is basically an atomized version of Ronnie/Jonathan/Stefan's\n> patch [1] \"refs.c: use a stringlist for repack_without_refs\". But I've\n> actually rewritten most of it from scratch, using the original patch\n> as a reference.\n\nNaturally, right after I emailed this series I realized that there have\nbeen two more iterations on the original patch, which I overlooked\nbecause I was not CCed on them. (I'm not complaining, just explaining.)\n\nI don't think that those iterations changed anything substantial that\noverlaps with my version, but TBH it's such a pain in the ass working\nwith patches in email that I don't think I'll go to the effort of\nchecking for sure unless somebody shows interest in actually using my\nversion.\n\nSorry for being grumpy today :-(\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"252347","messageId":"CAPc5daWubo+CSD-C+AH6Y-PKQ7h2MoUU=DbW+nYKO9uceogsAg@mail.gmail.com","threadId":"38003","inReplyTo":"1416578950-23210-4-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 3/6] prune_remote(): sort delete_refs_list references en masse","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-11-21T16:44:40Z","receivedAt":"2014-11-21T16:44:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"On Fri, Nov 21, 2014 at 6:09 AM, Michael Haggerty wrote:\n> Inserting items into a list in sorted order is O(N^2) whereas\n> appending them unsorted and then sorting the list all at once is\n> O(N lg N).\n>\n> string_list_insert() also removes duplicates, and this change loses\n> that functionality. But the strings in this list, which ultimately\n> come from a for_each_ref() iteration, cannot contain duplicates.\n>\n\nA similar conversion in other places we may do in the future\nmight find a need for an equivalent to \"-u\" option of \"sort\" in the\nstring_list_sort() function, but the above nicely explains why\nit is not necessary for this one. Good.\n\nEh, why is that called sort_string_list()? Perhaps it is a good\nopening to introduce string_list_sort(list, flag) where flag would\nbe a bitmask that represents ignore-case, uniquify, etc., and\nthen either deprecate the current one or make it a thin wrapper\nof the one that is more consistently named.\n\n\n> Signed-off-by: Michael Haggerty \n> ---\n> builtin/remote.c | 3 ++-\n> 1 file changed, 2 insertions(+), 1 deletion(-)\n>\n> diff --git a/builtin/remote.c b/builtin/remote.c\n> index d5a5a16..7d5c8d2 100644\n> --- a/builtin/remote.c\n> +++ b/builtin/remote.c\n> @@ -1341,8 +1341,9 @@ static int prune_remote(const char *remote, int dry_run)\n> const char *refname = states.stale.items[i].util;\n>\n> delete_refs[i] = refname;\n> - string_list_insert(&delete_refs_list, refname);\n> + string_list_append(&delete_refs_list, refname);\n> }\n> + sort_string_list(&delete_refs_list);\n>\n> if (!dry_run) {\n> struct strbuf err = STRBUF_INIT;\n> --\n> 2.1.3\n>\n"},{"id":"252353","messageId":"xmqq61e81ljq.fsf@gitster.dls.corp.google.com","threadId":"38003","inReplyTo":"546F4B5B.2060508@alum.mit.edu","subject":"Re: [PATCH 0/6] repack_without_refs(): convert to string_list","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-11-21T18:00:09Z","receivedAt":"2014-11-21T18:00:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> I don't think that those iterations changed anything substantial that\n> overlaps with my version, but TBH it's such a pain in the ass working\n> with patches in email that I don't think I'll go to the effort of\n> checking for sure unless somebody shows interest in actually using my\n> version.\n>\n> Sorry for being grumpy today :-(\n\nIs the above meant as a grumpy rant to be ignored, or as a\ndiscussion starter to improve the colaboration to allow people to\nwork better together instead of stepping on each other's patches?\n\nFWIW, I liked your rationale for \"many smaller steps\".\n\nOne small uncomfort in that approach is that it often is not very\nobvious by reading \"log -p master..\" alone how well the end result\nfits together. Each individual step may make sense, or at least it\nmay not make it any worse than the original, but until you apply the\nwhole series and read \"diff master...\" in a sitting, it is somewhat\nhard to tell where you are going. But this is not \"risk\" or \"bad\nthing\"; just something that may make readers feel uncomfortable---we\nare not losing anything by splitting a series into small logical\nchunks.\n\nThanks.\n"},{"id":"252362","messageId":"CAGZ79kaGuMNO7_ynRMO_8T2shRn=S-gctos6WJL=gMOsDitM+w@mail.gmail.com","threadId":"38003","inReplyTo":"xmqq61e81ljq.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 0/6] repack_without_refs(): convert to string_list","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-11-21T19:57:22Z","receivedAt":"2014-11-21T19:57:22Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Nov 21, 2014 at 10:00 AM, Junio C Hamano wrote:\n> Michael Haggerty writes:\n>\n>> I don't think that those iterations changed anything substantial that\n>> overlaps with my version, but TBH it's such a pain in the ass working\n>> with patches in email that I don't think I'll go to the effort of\n>> checking for sure unless somebody shows interest in actually using my\n>> version.\n>>\n>> Sorry for being grumpy today :-(\n\nSorry for causing the grumpyness.\nI have compared the versions, and they do look pretty similar.\nIn refs.{c,h} we're just talking about variable names and comments,\nthat are different.\n\nIn remote.c prune_remote however we did have slight differences,\n* early exit vs a large body below an if\n* your approach seems more elegant to me as you seem to know what you're doing:\n for_each_string_list_item(item, &states.stale)\n string_list_append(&refs_to_prune, item->util);\n instead of\n for (i = 0; i < states.stale.nr; i++)\n string_list_append(&delete_refs, states.stale.items[i].util);\n* You do not have a sort_string_list at the end before warn_dangling_symrefs,\n but you explained that it is not necessary.\n\nOn my continued journey on this mailing list I'll try to follow your\nexample and write lots of\nsmall easy to review patches, as they are indeed way easier to follow.\n\nHowever as Junio mentioned, we get other problems having too small changes.\nIn the review for the [PATCH v3 00/14] ref-transactions-reflog series you said:\n\n> I was reviewing this patch series (I left some comments in Gerrit about\n> the first few patches) when I realized that I'm having trouble\n> understanding the big picture of where you want to go with this.\n\nMaybe that was just my fault, not having stated the intentions in\nthe cover letter explicit enough. But having many patches will also not help\non presenting the big picture easily.\n\nThanks for bearing with me,\nStefan\n\n>\n> Is the above meant as a grumpy rant to be ignored, or as a\n> discussion starter to improve the colaboration to allow people to\n> work better together instead of stepping on each other's patches?\n>\n> FWIW, I liked your rationale for \"many smaller steps\".\n>\n> One small uncomfort in that approach is that it often is not very\n> obvious by reading \"log -p master..\" alone how well the end result\n> fits together. Each individual step may make sense, or at least it\n> may not make it any worse than the original, but until you apply the\n> whole series and read \"diff master...\" in a sitting, it is somewhat\n> hard to tell where you are going. But this is not \"risk\" or \"bad\n> thing\"; just something that may make readers feel uncomfortable---we\n> are not losing anything by splitting a series into small logical\n> chunks.\n>\n> Thanks.\n>\n>\n"},{"id":"252382","messageId":"20141122210725.GB15320@google.com","threadId":"38003","inReplyTo":"1416578950-23210-2-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 1/6] prune_remote(): exit early if there are no stale references","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-11-22T21:07:25Z","receivedAt":"2014-11-22T21:07:25Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Michael Haggerty wrote:\n\n> Aside from making the logic clearer, this avoids a call to\n> warn_dangling_symrefs(), which always does a for_each_rawref()\n> iteration.\n>\n> Signed-off-by: Michael Haggerty \n> ---\n> builtin/remote.c | 39 +++++++++++++++++++++------------------\n> 1 file changed, 21 insertions(+), 18 deletions(-)\n\nI had been wondering about this but didn't chase it down far enough.\nThanks for noticing and cleaning it up.\n\nReviewed-by: Jonathan Nieder \n"},{"id":"252383","messageId":"20141122210824.GC15320@google.com","threadId":"38003","inReplyTo":"1416578950-23210-4-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 3/6] prune_remote(): sort delete_refs_list references en masse","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-11-22T21:08:24Z","receivedAt":"2014-11-22T21:08:24Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Michael Haggerty wrote:\n\n> Signed-off-by: Michael Haggerty \n> ---\n> builtin/remote.c | 3 ++-\n> 1 file changed, 2 insertions(+), 1 deletion(-)\n\nThis and 2/6 are also\n\nReviewed-by: Jonathan Nieder \n"},{"id":"252384","messageId":"20141122211722.GD15320@google.com","threadId":"38003","inReplyTo":"1416578950-23210-5-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 4/6] repack_without_refs(): make the refnames argument a string_list","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-11-22T21:17:22Z","receivedAt":"2014-11-22T21:17:22Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Michael Haggerty wrote:\n\n> All of the callers have string_lists available already\n\nTechnically ref_transaction_commit doesn't, but that doesn't matter.\n\n> Suggested-by: Ronnie Sahlberg \n> Signed-off-by: Michael Haggerty \n> ---\n> builtin/remote.c | 14 ++------------\n> refs.c | 38 ++++++++++++++++++++------------------\n> refs.h | 11 ++++++++++-\n> 3 files changed, 32 insertions(+), 31 deletions(-)\n\nReviewed-by: Jonathan Nieder \n\nOne (optional) nit at the bottom of this message.\n\n[...]\n> +++ b/refs.c\n> @@ -2639,22 +2639,25 @@ static int curate_packed_ref_fn(struct ref_entry *entry, void *cb_data)\n> \treturn 0;\n> }\n> \n> -int repack_without_refs(const char **refnames, int n, struct strbuf *err)\n> +int repack_without_refs(struct string_list *refnames, struct strbuf *err)\n> {\n> \tstruct ref_dir *packed;\n> \tstruct string_list refs_to_delete = STRING_LIST_INIT_DUP;\n> -\tstruct string_list_item *ref_to_delete;\n> -\tint i, ret, removed = 0;\n> +\tstruct string_list_item *refname, *ref_to_delete;\n> +\tint ret, needs_repacking = 0, removed = 0;\n> \n> \tassert(err);\n> \n> \t/* Look for a packed ref */\n> -\tfor (i = 0; i < n; i++)\n> -\t\tif (get_packed_ref(refnames[i]))\n> +\tfor_each_string_list_item(refname, refnames) {\n> +\t\tif (get_packed_ref(refname->string)) {\n> +\t\t\tneeds_repacking = 1;\n> \t\t\tbreak;\n> +\t\t}\n> +\t}\n> \n> \t/* Avoid locking if we have nothing to do */\n> -\tif (i == n)\n> +\tif (!needs_repacking)\n\nThis makes me wish C supported something like Python's for/else\nconstruct. Oh well. :)\n\n[...]\n> +++ b/refs.h\n> @@ -163,7 +163,16 @@ extern void rollback_packed_refs(void);\n> */\n> int pack_refs(unsigned int flags);\n> \n> -extern int repack_without_refs(const char **refnames, int n,\n> +/*\n> + * Remove the refs listed in 'refnames' from the packed-refs file.\n> + * On error, packed-refs will be unchanged, the return value is\n> + * nonzero, and a message about the error is written to the 'err'\n> + * strbuf.\n> + *\n> + * The refs in 'refnames' needn't be sorted. The err buffer must not be\n> + * omitted.\n\n(nit)\n\ns/buffer/strbuf/, or s/The err buffer/'err'/\ns/omitted/NULL/\n\nThanks,\nJonathan\n"},{"id":"252385","messageId":"20141122211805.GE15320@google.com","threadId":"38003","inReplyTo":"1416578950-23210-6-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 5/6] prune_remote(): rename local variable","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-11-22T21:18:05Z","receivedAt":"2014-11-22T21:18:05Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Michael Haggerty wrote:\n\n> Signed-off-by: Michael Haggerty \n> ---\n> builtin/remote.c | 12 ++++++------\n> 1 file changed, 6 insertions(+), 6 deletions(-)\n\nReviewed-by: Jonathan Nieder \n"},{"id":"252386","messageId":"20141122211910.GF15320@google.com","threadId":"38003","inReplyTo":"1416578950-23210-7-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 6/6] prune_remote(): iterate using for_each_string_list_item()","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-11-22T21:19:10Z","receivedAt":"2014-11-22T21:19:10Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Michael Haggerty wrote:\n\n> Signed-off-by: Michael Haggerty \n> ---\n> builtin/remote.c | 14 ++++++--------\n> 1 file changed, 6 insertions(+), 8 deletions(-)\n\nReviewed-by: Jonathan Nieder \n\n(That makes 6/6. :))\n\nThanks for your thoughtfulness in putting these together. They were\npleasant to read.\n"},{"id":"252476","messageId":"5473CD28.5020405@alum.mit.edu","threadId":"38003","inReplyTo":"xmqq61e81ljq.fsf@gitster.dls.corp.google.com","subject":"Our cumbersome mailing list workflow (was: Re: [PATCH 0/6] repack_without_refs(): convert to string_list)","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-11-25T00:28:24Z","receivedAt":"2014-11-25T00:28:24Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 11/21/2014 07:00 PM, Junio C Hamano wrote:\n> Michael Haggerty writes:\n> \n>> I don't think that those iterations changed anything substantial that\n>> overlaps with my version, but TBH it's such a pain in the ass working\n>> with patches in email that I don't think I'll go to the effort of\n>> checking for sure unless somebody shows interest in actually using my\n>> version.\n>>\n>> Sorry for being grumpy today :-(\n> \n> Is the above meant as a grumpy rant to be ignored, or as a\n> discussion starter to improve the colaboration to allow people to\n> work better together instead of stepping on each other's patches?\n\nI think I know the sentiments of the mailing list regulars well enough\nthat it didn't seem worthwhile to open this topic again, so I was just\nletting off steam without any hope of changing anything. But since you\nasked...\n\nLet me list the aspects of our mailing list workflow that I find\ncumbersome as a contributor and reviewer:\n\n* Submitting patches to the mailing list is an ordeal of configuring\nformat-patch and send-email and getting everything just right, using\ninstructions that depend on the local environment. We saw that hardly\nany GSoC applicants were able to get it right on their first attempt.\nSubmitting a patch series should be as simple as \"git push\".\n\n* Once patches are submitted, there is no assurance that you (Junio)\nwill apply them to your tree at the same point that the submitter\ndeveloped and tested them.\n\n* The branch name that you choose for a patch series is not easily\nderivable from the patches as they appeared in the mailing list. Trying\nto figure out whether/where the patches exist in your tree is a largely\nmanual task. The reverse mapping, from in-tree commit to the email where\nit was proposed, is even more difficult to infer.\n\n* Your tree has no indication of which version of a patch series (v1,\nv2, etc) is currently applied.\n\nThe previous three points combine to make it awkward to get patches into\nmy local repository to review or test. There are two alternatives, both\ncumbersome and imprecise:\n\n * I do \"git fetch gitster\", then try to figure out whether the branch\nI'm interested in is present, what its name is, and whether the version\nin your tree is the latest version, then \"git checkout xy/foobar\".\n\n * Or I save the emails to a temporary directory (awkward because, Oh\nHorror, I use Thunderbird and not mutt as email client), hope that I've\nguessed the right place to apply them, run \"git am\", and later try to\nremember to clean up the temporary directory.\n\n* Once I've done that, the \"supplemental\" comments from the emails (the\ncover letter and the text under the \"---\") are nowhere available in the\nGit repository. So if I want to see the changes in context plus the\nsupplemental comments, I have to jump back and forth between email\nclient and Git repo. Plus I have to jump around the rest of the email\nthread to see what comments other reviewers have already made about the\nseries.\n\n* Following patch series across iterations is also awkward. To compare\ntwo versions, I have to first get both patch series into my repo, which\ninvolves digging through the ML history to find older versions, followed\nby the \"git am\" steps. Often submitters are nice enough to put links to\nprevious versions of their patch series in their cover letters, but the\nlinks are to a web-based email archive, from which it is even more\nawkward to grab and apply patches. So in practice I then go back to my\nemail client and search my local archive for my copy of the same email\nthat was referenced in the archive, and apply the patch from there.\nFinding comments about old versions of a patch series is nearly as much\nwork.\n\n* Because of the indeterminate application point, accumulating\nSigned-off-by lines, changed committer metadata, and maintainer tweaks,\nthe commits that make it to the official tree have different SHA-1s than\nthe commits in the submitter's tree, and both are different than the\ncommits in the tree of any reviewer who got the patches using \"git am\".\nThis makes it hard to be sure that everybody is on the same page. It\nalso makes it awkward for people to exchange ideas for further changes\nvia Git protocols in the form of patches.\n\n* Because of the crude serialization of patches through email, it is\nonly possible to submit linear patch series, not merge commits.\n\nHmmm, I think that covers most of the problems of handling patches and\nreview via a mailing list.\n\n\nWhat are some alternatives?\n\nI did enjoy the variety of reviewing some patch series using Gerrit. It\nis nice that it tracks the evolution of a patch from version to version,\nand that the comments made on all versions of a patch are summarized in\na single place. This makes it easier to avoid commenting on issues that\nother reviewers have already noted and easier to check that your own\ncomments have been addressed by later versions of the patch. On the\nother hand, Gerrit seems strongly focused on individual patches rather\nthan on patch series (which might not match our workflow so well), the\nUI is overwhelming (though I think one could get quite productive with\nit if one used it every day), and the notification emails come in blizzards.\n\nGitHub is another obvious alternative [1], free for open-source projects\nalbeit not open-source itself. It is very easy to use and easy to\ninteract with from a Git client, and also has a good API. It is super\neasy to submit patches to a project using GitHub. But the GitHub user\ninterface (ISTM) is optimized for getting the net result of an entire\nfeature branch perfect, as opposed to iterating a series of patches\nuntil each one is individually perfect (e.g., it works best when adding\npatches on top of a feature branch as opposed to rebasing existing\npatches). Since Git development is oriented towards perfecting each\npatch, GitHub would be a bit of an impedance mismatch.\n\nI don't think either of those systems is ideally matched to the Git\nproject's workflow, but in my opinion either one of them would be more\nconvenient for contributors and reviewers than serializing everything\nthrough the mailing list.\n\nOf course what is most convenient for the maintainer is of huge\nimportance, but I can't say much about that.\n\n> FWIW, I liked your rationale for \"many smaller steps\".\n\nThanks.\n\n> One small uncomfort in that approach is that it often is not very\n> obvious by reading \"log -p master..\" alone how well the end result\n> fits together. Each individual step may make sense, or at least it\n> may not make it any worse than the original, but until you apply the\n> whole series and read \"diff master...\" in a sitting, it is somewhat\n> hard to tell where you are going. But this is not \"risk\" or \"bad\n> thing\"; just something that may make readers feel uncomfortable---we\n> are not losing anything by splitting a series into small logical\n> chunks.\n\nIdeally, the cover letter should provide the \"big picture\" rationale for\na patch series, and the individual commit messages should provide clues\nabout why that step is useful.\n\nIt might be a nice convention to ask people to write the \"big picture\"\nrationale in their cover letter, separated by a \"---\" from non-permanent\ndiscussion. Then the part above the \"---\" could be copied into the\ncommit message for the *merge commit* that brings the feature branch\ninto master. That would preserve it for posterity in a place where it is\nrelatively easy to find. But I am reluctant to make the process of\nsubmitting patches even more difficult :-)\n\nMichael\n\n[1] Disclaimer: I work for GitHub.\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"252497","messageId":"54742DEE.7090905@alum.mit.edu","threadId":"38003","inReplyTo":"CAPc5daWubo+CSD-C+AH6Y-PKQ7h2MoUU=DbW+nYKO9uceogsAg@mail.gmail.com","subject":"Re: [PATCH 3/6] prune_remote(): sort delete_refs_list references en masse","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-11-25T07:21:18Z","receivedAt":"2014-11-25T07:21:18Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 11/21/2014 05:44 PM, Junio C Hamano wrote:\n> On Fri, Nov 21, 2014 at 6:09 AM, Michael Haggerty wrote:\n>> Inserting items into a list in sorted order is O(N^2) whereas\n>> appending them unsorted and then sorting the list all at once is\n>> O(N lg N).\n>>\n>> string_list_insert() also removes duplicates, and this change loses\n>> that functionality. But the strings in this list, which ultimately\n>> come from a for_each_ref() iteration, cannot contain duplicates.\n>>\n> \n> A similar conversion in other places we may do in the future\n> might find a need for an equivalent to \"-u\" option of \"sort\" in the\n> string_list_sort() function, but the above nicely explains why\n> it is not necessary for this one. Good.\n\nThe only reason to integrate \"-u\" functionality into the sort would be\nif one expects a significant fraction of entries to be duplicates, in\nwhich case the sort could be structured to discard duplicates as it\nworks, thereby reducing the work needed for the sort. I can't think of\nsuch a case in our code. Otherwise, calling sort_string_list() followed\nby string_list_remove_duplicates() should be just as clear and\napproximately as efficient.\n\nA couple of times I've also felt that an all-purpose *stable* sort would\nbe convenient (though I can't remember the context offhand). I don't\nthink we have such a thing.\n\n> Eh, why is that called sort_string_list()? Perhaps it is a good\n> opening to introduce string_list_sort(list, flag) where flag would\n> be a bitmask that represents ignore-case, uniquify, etc., and\n> then either deprecate the current one or make it a thin wrapper\n> of the one that is more consistently named.\n\nI agree. Indeed, I typed that function's name wrong once when\nconstructing this patch. It would be better to name it consistently with\nthe other string_list_*() functions.\n\nI put it on my todo list (but don't let that dissuade somebody else from\ndoing it).\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"252498","messageId":"547432D5.8070802@alum.mit.edu","threadId":"38003","inReplyTo":"20141122211722.GD15320@google.com","subject":"Re: [PATCH 4/6] repack_without_refs(): make the refnames argument a string_list","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-11-25T07:42:13Z","receivedAt":"2014-11-25T07:42:13Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 11/22/2014 10:17 PM, Jonathan Nieder wrote:\n> Michael Haggerty wrote:\n> \n>> All of the callers have string_lists available already\n> \n> Technically ref_transaction_commit doesn't, but that doesn't matter.\n\nYou're right. I'll correct this.\n\n>> Suggested-by: Ronnie Sahlberg \n>> Signed-off-by: Michael Haggerty \n>> ---\n>> builtin/remote.c | 14 ++------------\n>> refs.c | 38 ++++++++++++++++++++------------------\n>> refs.h | 11 ++++++++++-\n>> 3 files changed, 32 insertions(+), 31 deletions(-)\n> \n> Reviewed-by: Jonathan Nieder \n> \n> One (optional) nit at the bottom of this message.\n> \n> [...]\n>> +++ b/refs.c\n>> @@ -2639,22 +2639,25 @@ static int curate_packed_ref_fn(struct ref_entry *entry, void *cb_data)\n>> \treturn 0;\n>> }\n>> \n>> -int repack_without_refs(const char **refnames, int n, struct strbuf *err)\n>> +int repack_without_refs(struct string_list *refnames, struct strbuf *err)\n>> {\n>> \tstruct ref_dir *packed;\n>> \tstruct string_list refs_to_delete = STRING_LIST_INIT_DUP;\n>> -\tstruct string_list_item *ref_to_delete;\n>> -\tint i, ret, removed = 0;\n>> +\tstruct string_list_item *refname, *ref_to_delete;\n>> +\tint ret, needs_repacking = 0, removed = 0;\n>> \n>> \tassert(err);\n>> \n>> \t/* Look for a packed ref */\n>> -\tfor (i = 0; i < n; i++)\n>> -\t\tif (get_packed_ref(refnames[i]))\n>> +\tfor_each_string_list_item(refname, refnames) {\n>> +\t\tif (get_packed_ref(refname->string)) {\n>> +\t\t\tneeds_repacking = 1;\n>> \t\t\tbreak;\n>> +\t\t}\n>> +\t}\n>> \n>> \t/* Avoid locking if we have nothing to do */\n>> -\tif (i == n)\n>> +\tif (!needs_repacking)\n> \n> This makes me wish C supported something like Python's for/else\n> construct. Oh well. :)\n\nAhhh, Python, where arrays of strings *are* string_lists :-)\n\n> [...]\n>> +++ b/refs.h\n>> @@ -163,7 +163,16 @@ extern void rollback_packed_refs(void);\n>> */\n>> int pack_refs(unsigned int flags);\n>> \n>> -extern int repack_without_refs(const char **refnames, int n,\n>> +/*\n>> + * Remove the refs listed in 'refnames' from the packed-refs file.\n>> + * On error, packed-refs will be unchanged, the return value is\n>> + * nonzero, and a message about the error is written to the 'err'\n>> + * strbuf.\n>> + *\n>> + * The refs in 'refnames' needn't be sorted. The err buffer must not be\n>> + * omitted.\n> \n> (nit)\n> \n> s/buffer/strbuf/, or s/The err buffer/'err'/\n> s/omitted/NULL/\n\nI will fix this too (and improve the docstring a bit in general). Thanks\nfor your careful review!\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"252505","messageId":"54743805.20601@alum.mit.edu","threadId":"38003","inReplyTo":"54742DEE.7090905@alum.mit.edu","subject":"Re: [PATCH 3/6] prune_remote(): sort delete_refs_list references en masse","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-11-25T08:04:21Z","receivedAt":"2014-11-25T08:04:21Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 11/25/2014 08:21 AM, Michael Haggerty wrote:\n> On 11/21/2014 05:44 PM, Junio C Hamano wrote:\n>> [...]\n>> Eh, why is that called sort_string_list()? Perhaps it is a good\n>> opening to introduce string_list_sort(list, flag) where flag would\n>> be a bitmask that represents ignore-case, uniquify, etc., and\n>> then either deprecate the current one or make it a thin wrapper\n>> of the one that is more consistently named.\n> \n> I agree. Indeed, I typed that function's name wrong once when\n> constructing this patch. It would be better to name it consistently with\n> the other string_list_*() functions.\n> \n> I put it on my todo list (but don't let that dissuade somebody else from\n> doing it).\n\nSince I was re-rolling the patch series anyway, I tacked this renaming\nchange onto the end of it. Feel free to omit it if you think it belongs\nseparately.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"252645","messageId":"54776367.1010104@web.de","threadId":"38003","inReplyTo":"5473CD28.5020405@alum.mit.edu","subject":"Re: Our cumbersome mailing list workflow","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2014-11-27T17:46:15Z","receivedAt":"2014-11-27T17:46:15Z","isPatch":false,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 2014-11-25 01.28, Michael Haggerty wrote:\n[]\n> Let me list the aspects of our mailing list workflow that I find\n> cumbersome as a contributor and reviewer:\n> \n> * Submitting patches to the mailing list is an ordeal of configuring\n> format-patch and send-email and getting everything just right, using\n> instructions that depend on the local environment.\nTypically everything fits into ~/.gitconfig,\nwhich can be carried around on a USB-Stick.\nIs there any details which I miss, or howtows we can improve ?\n> We saw that hardly\n> any GSoC applicants were able to get it right on their first attempt.\n> Submitting a patch series should be as simple as \"git push\".\n> \n> * Once patches are submitted, there is no assurance that you (Junio)\n> will apply them to your tree at the same point that the submitter\n> developed and tested them.\n> \n> * The branch name that you choose for a patch series is not easily\n> derivable from the patches as they appeared in the mailing list. Trying\n> to figure out whether/where the patches exist in your tree is a largely\n> manual task. The reverse mapping, from in-tree commit to the email where\n> it was proposed, is even more difficult to infer.\n> \n> * Your tree has no indication of which version of a patch series (v1,\n> v2, etc) is currently applied.\n\n> \n> The previous three points combine to make it awkward to get patches into\n> my local repository to review or test. There are two alternatives, both\n> cumbersome and imprecise:\n> \n> * I do \"git fetch gitster\", then try to figure out whether the branch\n> I'm interested in is present, what its name is, and whether the version\n> in your tree is the latest version, then \"git checkout xy/foobar\".\nThere are 12 branches from mh/, so it should be possible to find the name,\nund run git log gitster/xy/fix_this_bug or so.\nEven more important, this branch is the \"single point of truth\", because\nthis branch may be merged eventually, and nothing else.\n> \n> * Or I save the emails to a temporary directory (awkward because, Oh\n> Horror, I use Thunderbird and not mutt as email client), hope that I've\n> guessed the right place to apply them, run \"git am\", and later try to\n> remember to clean up the temporary directory.\nIs there a \"mutt howto\" somewhere?\n> \n> * Once I've done that, the \"supplemental\" comments from the emails (the\n> cover letter and the text under the \"---\") are nowhere available in the\n> Git repository. So if I want to see the changes in context plus the\n> supplemental comments, I have to jump back and forth between email\n> client and Git repo. Plus I have to jump around the rest of the email\n> thread to see what comments other reviewers have already made about the\n> series.\n> \n> * Following patch series across iterations is also awkward. To compare\n> two versions, I have to first get both patch series into my repo, which\n> involves digging through the ML history to find older versions, followed\n> by the \"git am\" steps. Often submitters are nice enough to put links to\n> previous versions of their patch series in their cover letters, but the\n> links are to a web-based email archive, from which it is even more\n> awkward to grab and apply patches. So in practice I then go back to my\n> email client and search my local archive for my copy of the same email\n> that was referenced in the archive, and apply the patch from there.\n> Finding comments about old versions of a patch series is nearly as much\n> work.\nIn short:\nWe can ask every contributor, if the patch send to the mailing list\nis available on a public Git-repo, and what the branch name is,\nlike _V2.. Does this makes sense ?\n\nAs an alternative, you can save the branches locally, after running\ngit-am once, just keep the branch.\n[]\n\n> \n> I did enjoy the variety of reviewing some patch series using Gerrit. It\n> is nice that it tracks the evolution of a patch from version to version,\n> and that the comments made on all versions of a patch are summarized in\n> a single place. This makes it easier to avoid commenting on issues that\n> other reviewers have already noted and easier to check that your own\n> comments have been addressed by later versions of the patch. On the\n> other hand, Gerrit seems strongly focused on individual patches rather\n> than on patch series (which might not match our workflow so well), the\n> UI is overwhelming (though I think one could get quite productive with\n> it if one used it every day), and the notification emails come in blizzards.\n> \n> Michael\n> \n> [1] Disclaimer: I work for GitHub.\n> \nI like Gerrit as well.\nBut it is less efficient to use, a WEB browser is slower (often), and\nyou need to use the mouse...\nHowever, if you put your patches on Gerrit, and add the link in your cover-letter,\nit may be worth a trial.\n\nBut there is another thing:\nOnce a patch is send out, I would ask the sender to wait and collect comments\nat least 24 hours before sending a V2.\nWe all living in different time zones, so please let the world spin once.\n\nMy feeling is that a patch > 5 commits should have\na waiting time > 5 days, otherwise I start reviewing V1, then V2 comes,\nthen V3 before I am finished with V1. That is not ideal.\n\nWhat does it cost to push your branch to a public repo and\ninclude that information in the email ?\n\nAnd how feasable/nice/useful is it to ask contributers for a wait\ntime between re-rolling ?\n \n"},{"id":"252646","messageId":"vpqoars7b8z.fsf@anie.imag.fr","threadId":"38003","inReplyTo":"54776367.1010104@web.de","subject":"Re: Our cumbersome mailing list workflow","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-11-27T18:24:12Z","receivedAt":"2014-11-27T18:24:12Z","isPatch":false,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Torsten Bögershausen writes:\n\n> On 2014-11-25 01.28, Michael Haggerty wrote:\n> []\n>> Let me list the aspects of our mailing list workflow that I find\n>> cumbersome as a contributor and reviewer:\n>> \n>> * Submitting patches to the mailing list is an ordeal of configuring\n>> format-patch and send-email and getting everything just right, using\n>> instructions that depend on the local environment.\n> Typically everything fits into ~/.gitconfig,\n> which can be carried around on a USB-Stick.\n\nI personnally submit all my Git patches from a machine whose\n/usr/sbin/sendmail knows how to send emails, so for me configuration is\nsuper simple. But I can imagine the pain of someone working on various\nmachines with various network configuration and normally using a webmail\nto send emails. Sharing ~/.gitconfig does not always work because on\nmachine A you only can use one SMTP server, and on machine B only\nanother ...\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"252651","messageId":"20141127225334.GA29203@dcvr.yhbt.net","threadId":"38003","inReplyTo":"54776367.1010104@web.de","subject":"Re: Our cumbersome mailing list workflow","fromName":"Eric Wong","fromEmail":"normalperson@yhbt.net","sentAt":"2014-11-27T22:53:34Z","receivedAt":"2014-11-27T22:53:34Z","isPatch":false,"sender":{"key":"e@80x24.org","avatar":null},"body":"Torsten Bögershausen wrote:\n> On 2014-11-25 01.28, Michael Haggerty wrote:\n> > * Or I save the emails to a temporary directory (awkward because, Oh\n> > Horror, I use Thunderbird and not mutt as email client), hope that I've\n> > guessed the right place to apply them, run \"git am\", and later try to\n> > remember to clean up the temporary directory.\n> \n> Is there a \"mutt howto\" somewhere?\n\nNot that I'm aware of, but Documentation/email-clients.txt in\nthe Linux kernel has some short notes...\n\nMy muttrc has had the following since my early days as a git user:\n\n macro index A \":unset pipe_decode\\n|git am -3\\n:set pipe_decode\\n\"\n macro pager A \":unset pipe_decode\\n|git am -3\\n:set pipe_decode\\n\"\n\n(Hit Shift-A while viewing/selecting a message to apply a patch,\n it requires you run mutt in your project working directory, though).\n\nPerhaps there can be a similar document or reference to it in our\nDocumentation/\n\n> In short:\n> We can ask every contributor, if the patch send to the mailing list\n> is available on a public Git-repo, and what the branch name is,\n> like _V2.. Does this makes sense ?\n\nNot unreasonable. I hope that won't give folks an excuse to refuse\nto mail patches, though. Some folks read email offline and can't\nfetch repos until they're online again.\n\n> I like Gerrit as well.\n> But it is less efficient to use, a WEB browser is slower (often), and\n> you need to use the mouse...\n\nIMNSHO, development of non-graphical software should never depend on\ngraphical software. Also, I guess there is no way to comment on Gerrit\nvia email (without registration/logins?).\n\nLately, I've been trying to think of ways to make collaboration less\ncentralized. Moving to more centralized collaboration tools is a step\nback for decentralized VCS.\n\n> But there is another thing:\n> Once a patch is send out, I would ask the sender to wait and collect comments\n> at least 24 hours before sending a V2.\n> We all living in different time zones, so please let the world spin once.\n> \n> My feeling is that a patch > 5 commits should have\n> a waiting time > 5 days, otherwise I start reviewing V1, then V2 comes,\n> then V3 before I am finished with V1. That is not ideal.\n> \n> What does it cost to push your branch to a public repo and\n> include that information in the email ?\n> \n> And how feasable/nice/useful is it to ask contributers for a wait\n> time between re-rolling ?\n\nAll that sounds good.\n"},{"id":"252657","messageId":"A2A8D58134DA4E0B8D9CCE3D29E5319E@PhilipOakley","threadId":"38003","inReplyTo":"vpqoars7b8z.fsf@anie.imag.fr","subject":"Re: Our cumbersome mailing list workflow","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.org","sentAt":null,"receivedAt":"2014-11-28T11:13:24Z","isPatch":false,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"From: \"Matthieu Moy\" \n> Torsten Bögershausen writes:\n>\n>> On 2014-11-25 01.28, Michael Haggerty wrote:\n>> []\n>>> Let me list the aspects of our mailing list workflow that I find\n>>> cumbersome as a contributor and reviewer:\n>>>\n>>> * Submitting patches to the mailing list is an ordeal of configuring\n>>> format-patch and send-email and getting everything just right, using\n>>> instructions that depend on the local environment.\n>> Typically everything fits into ~/.gitconfig,\n>> which can be carried around on a USB-Stick.\n>\n> I personnally submit all my Git patches from a machine whose\n> /usr/sbin/sendmail knows how to send emails, so for me configuration \n> is\n> super simple. But I can imagine the pain of someone working on various\n> machines with various network configuration and normally using a \n> webmail\n> to send emails. Sharing ~/.gitconfig does not always work because on\n> machine A you only can use one SMTP server, and on machine B only\n> another ...\n\nThe bit I find awkward for the send-email step is the creation of the \n\"to\" and \"cc\" lists. I tend to create the command line in a separate \nfile so that I can re-use it for V2 etc. and even then I end up with all \npatches going to the full to/cc list.\n\nMichael's original discussion email did feel to summarise the isses [1] \nwell.\n\n--\nPhilip\n[1] System Problems are Wicked problems :\nhttp://en.wikipedia.org/wiki/Wicked_problem\nwww.poppendieck.com/wicked.htm\n"},{"id":"252660","messageId":"54788743.5090703@alum.mit.edu","threadId":"38003","inReplyTo":"54776367.1010104@web.de","subject":"Re: Our cumbersome mailing list workflow","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-11-28T14:31:31Z","receivedAt":"2014-11-28T14:31:31Z","isPatch":false,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 11/27/2014 06:46 PM, Torsten Bögershausen wrote:\n> On 2014-11-25 01.28, Michael Haggerty wrote:\n> []\n>> Let me list the aspects of our mailing list workflow that I find\n>> cumbersome as a contributor and reviewer:\n>>\n>> * Submitting patches to the mailing list is an ordeal of configuring\n>> format-patch and send-email and getting everything just right, using\n>> instructions that depend on the local environment.\n> Typically everything fits into ~/.gitconfig,\n> which can be carried around on a USB-Stick.\n> Is there any details which I miss, or howtows we can improve ?\n\nI used to need one setup at work and a different one at home (because of\nhow my email was configured), and sometimes had to switch back and forth\nas I carried my notebook around.\n\n>> [...]\n>> * I do \"git fetch gitster\", then try to figure out whether the branch\n>> I'm interested in is present, what its name is, and whether the version\n>> in your tree is the latest version, then \"git checkout xy/foobar\".\n> There are 12 branches from mh/, so it should be possible to find the name,\n> und run git log gitster/xy/fix_this_bug or so.\n> Even more important, this branch is the \"single point of truth\", because\n> this branch may be merged eventually, and nothing else.\n\nI know it's *possible*. The question is whether it could be made easier.\n\n>> * Following patch series across iterations is also awkward. To compare\n>> two versions, I have to first get both patch series into my repo, which\n>> involves digging through the ML history to find older versions, followed\n>> by the \"git am\" steps. Often submitters are nice enough to put links to\n>> previous versions of their patch series in their cover letters, but the\n>> links are to a web-based email archive, from which it is even more\n>> awkward to grab and apply patches. So in practice I then go back to my\n>> email client and search my local archive for my copy of the same email\n>> that was referenced in the archive, and apply the patch from there.\n>> Finding comments about old versions of a patch series is nearly as much\n>> work.\n> In short:\n> We can ask every contributor, if the patch send to the mailing list\n> is available on a public Git-repo, and what the branch name is,\n> like _V2.. Does this makes sense ?\n\nThat would be helpful, but it would put yet *another* requirement on the\nsubmitter (to send patch emails *and* push the branch to some accessible\nrepository). We regulars could script this pretty easily, but people who\nonly contribute occasionally or who are trying to get started will be\neven more overwhelmed.\n\n> As an alternative, you can save the branches locally, after running\n> git-am once, just keep the branch.\n> []\n\nYes, but it is even more unnecessary manual bookkeeping.\n\n> [...]\n> But there is another thing:\n> Once a patch is send out, I would ask the sender to wait and collect comments\n> at least 24 hours before sending a V2.\n> We all living in different time zones, so please let the world spin once.\n\nYes, good idea.\n\n> My feeling is that a patch > 5 commits should have\n> a waiting time > 5 days, otherwise I start reviewing V1, then V2 comes,\n> then V3 before I am finished with V1. That is not ideal.\n\nOne day per patch might be exaggerated, but I agree that long series\nshould be iterated more slowly than short ones.\n\n> What does it cost to push your branch to a public repo and\n> include that information in the email ?\n\nOne has to run an additional command and add some information to the\ncover letter, every time a patch series is submitted. If it's scripted\nthen it's relatively painless. But for a newcomer these will be manual\nsteps that are easy to forget or to do incorrectly, making it more\nlikely that the newcomer's first contribution to Git will end in mild\nembarrassment rather than success.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"252662","messageId":"547895F1.1010307@alum.mit.edu","threadId":"38003","inReplyTo":"20141127225334.GA29203@dcvr.yhbt.net","subject":"Re: Our cumbersome mailing list workflow","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-11-28T15:34:09Z","receivedAt":"2014-11-28T15:34:09Z","isPatch":false,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 11/27/2014 11:53 PM, Eric Wong wrote:\n> Torsten Bögershausen wrote:\n>> On 2014-11-25 01.28, Michael Haggerty wrote:\n>>> [...]\n>> In short:\n>> We can ask every contributor, if the patch send to the mailing list\n>> is available on a public Git-repo, and what the branch name is,\n>> like _V2.. Does this makes sense ?\n> \n> Not unreasonable. I hope that won't give folks an excuse to refuse\n> to mail patches, though. Some folks read email offline and can't\n> fetch repos until they're online again.\n\nMy ideal would be to invert the procedure. Let the patches in a public\nGit repository somewhere be the primary artifact, and let the review\nprocess be focused there. Let email be an alternative interface to the\ncentral review site:\n\n* Generate patch emails (similar to the current format) when pull\nrequests are submitted.\n\n* Generate notification emails when people comment on the patches.\n\n* Allow people to respond to the patch and notification emails via\nemail. The central review site should associate those comments with the\npatches that they apply to, and present them along with other review\ncomments received via other interfaces.\n\n>> I like Gerrit as well.\n>> But it is less efficient to use, a WEB browser is slower (often), and\n>> you need to use the mouse...\n> \n> IMNSHO, development of non-graphical software should never depend on\n> graphical software. Also, I guess there is no way to comment on Gerrit\n> via email (without registration/logins?).\n\nThe days of the vt52 are over. I'm an old neckbeard myself and have used\n*real* vt52s. But these days even my *cellphone* is able to handle the\nGitHub website [1]. Rejecting modern technology is not intrinsically\nvirtuous; it only makes sense if the old technology is really superior.\nAnd it is not enough for it to be superior only for neckbeards; it\nshould be superior when averaged over all of the people whose\nparticipation we would like to have in the Git project.\n\nAnd by the way, there are text-only clients for interacting with GitHub [1].\n\n> Lately, I've been trying to think of ways to make collaboration less\n> centralized. Moving to more centralized collaboration tools is a step\n> back for decentralized VCS.\n\nIf an efficient decentralized collaboration system existed, then I'd\nlove to give it a chance. But as far as I know, the existing systems are\nall embryonic.\n\nDon't forget that even our current system is centralized to some extent.\nThere is a single mailing list through which all emails pass. There are\na few email archives that we de facto rely on (and it is a brittle\ndependency--if Gmane were to disappear, we would have an awful lot of\nbroken URLs in our emails that would be impossible to fix).\n\nIt seems like a few desirable features are being talked about here, and\nsummarizing the discussion as \"centralized\" vs \"decentralized\" is too\nsimplistic. What is really important?\n\n1. Convenient and efficient, including for newcomers\n2. Usable while offline\n3. Usable in pure-text mode\n4. Decentralized\n\nSomething else?\n\nIn my opinion, a central system with good Git integration (helps with 1)\nand both a straightforward web UI (also helps 1) and a good email\ninterface (which gives both 2 and 3) and the ability to export the\nreview history (which avoids lockin, the most important aspect of 4)\nwould be perfect. Is there such a thing?\n\nMichael\n\n[1] ...probably other websites too. I'm really not trying to flog GitHub\nhere; it's just the one I have the most experience with. In fact, I\nkindof assume that the Git project would choose a service that is itself\nbased on open-source software.\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"252663","messageId":"547897F4.5000305@xiplink.com","threadId":"38003","inReplyTo":"54788743.5090703@alum.mit.edu","subject":"Re: Our cumbersome mailing list workflow","fromName":"Marc Branchaud","fromEmail":"marcnarc@xiplink.com","sentAt":"2014-11-28T15:42:44Z","receivedAt":"2014-11-28T15:42:44Z","isPatch":false,"sender":{"key":"marcnarc@xiplink.com","avatar":"https://avatars.githubusercontent.com/u/14980203?v=4"},"body":"On 14-11-28 09:31 AM, Michael Haggerty wrote:\n> On 11/27/2014 06:46 PM, Torsten Bögershausen wrote:\n>> On 2014-11-25 01.28, Michael Haggerty wrote:\n>> []\n>>> Let me list the aspects of our mailing list workflow that I find\n>>> cumbersome as a contributor and reviewer:\n>>>\n>>> * Submitting patches to the mailing list is an ordeal of configuring\n>>> format-patch and send-email and getting everything just right, using\n>>> instructions that depend on the local environment.\n>> Typically everything fits into ~/.gitconfig,\n>> which can be carried around on a USB-Stick.\n>> Is there any details which I miss, or howtows we can improve ?\n> \n> I used to need one setup at work and a different one at home (because of\n> how my email was configured), and sometimes had to switch back and forth\n> as I carried my notebook around.\n> \n>>> [...]\n>>> * I do \"git fetch gitster\", then try to figure out whether the branch\n>>> I'm interested in is present, what its name is, and whether the version\n>>> in your tree is the latest version, then \"git checkout xy/foobar\".\n>> There are 12 branches from mh/, so it should be possible to find the name,\n>> und run git log gitster/xy/fix_this_bug or so.\n>> Even more important, this branch is the \"single point of truth\", because\n>> this branch may be merged eventually, and nothing else.\n> \n> I know it's *possible*. The question is whether it could be made easier.\n> \n>>> * Following patch series across iterations is also awkward. To compare\n>>> two versions, I have to first get both patch series into my repo, which\n>>> involves digging through the ML history to find older versions, followed\n>>> by the \"git am\" steps. Often submitters are nice enough to put links to\n>>> previous versions of their patch series in their cover letters, but the\n>>> links are to a web-based email archive, from which it is even more\n>>> awkward to grab and apply patches. So in practice I then go back to my\n>>> email client and search my local archive for my copy of the same email\n>>> that was referenced in the archive, and apply the patch from there.\n>>> Finding comments about old versions of a patch series is nearly as much\n>>> work.\n>> In short:\n>> We can ask every contributor, if the patch send to the mailing list\n>> is available on a public Git-repo, and what the branch name is,\n>> like _V2.. Does this makes sense ?\n> \n> That would be helpful, but it would put yet *another* requirement on the\n> submitter (to send patch emails *and* push the branch to some accessible\n> repository). We regulars could script this pretty easily, but people who\n> only contribute occasionally or who are trying to get started will be\n> even more overwhelmed.\n\nA bot could subscribe to the list and create branches in a public repo.\n(This idea feels familiar -- didn't somebody attempt this already?)\n\nIntegrate the bot into the list manager, and every PATCH email sent through\nthe list could have the patch's URL (maybe in the footer, or as an X- header).\n\nCould this make a decent GSoC project?\n\n\t\tM.\n"},{"id":"252665","messageId":"20141128162425.GE4744@vauxhall.crustytoothpaste.net","threadId":"38003","inReplyTo":"547895F1.1010307@alum.mit.edu","subject":"Re: Our cumbersome mailing list workflow","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2014-11-28T16:24:26Z","receivedAt":"2014-11-28T16:24:26Z","isPatch":false,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Fri, Nov 28, 2014 at 04:34:09PM +0100, Michael Haggerty wrote:\n> My ideal would be to invert the procedure. Let the patches in a public\n> Git repository somewhere be the primary artifact, and let the review\n> process be focused there. Let email be an alternative interface to the\n> central review site:\n> \n> * Generate patch emails (similar to the current format) when pull\n> requests are submitted.\n> \n> * Generate notification emails when people comment on the patches.\n> \n> * Allow people to respond to the patch and notification emails via\n> email. The central review site should associate those comments with the\n> patches that they apply to, and present them along with other review\n> comments received via other interfaces.\n\nI think these are good goals. Even as a semi-regular contributor, I\nprefer to push branches around using Git rather than formatting patches\nand mailing them.\n\nAlso, I think that being able to comment on a patch or report a bug\nwithout a login (via email) is desirable. I'm not a fan of having to\nhave an account on every Bugzilla on the planet. That's why I like\ndebbugs.\n\n> It seems like a few desirable features are being talked about here, and\n> summarizing the discussion as \"centralized\" vs \"decentralized\" is too\n> simplistic. What is really important?\n> \n> 1. Convenient and efficient, including for newcomers\n> 2. Usable while offline\n> 3. Usable in pure-text mode\n> 4. Decentralized\n\nI think 1 is definitely important. For me personally, 2 isn't very\nimportant, as all my email is via IMAP (so I have to be online). I\nthink 3 is important for accessibility reasons. There are a lot of\nblind or low-sighted people for whom a GUI is infeasible or burdensome.\n\n> Something else?\n\nIt might be useful to have a system that has a bug or issue tracker. We\noften have posts to the mailing list that don't get a response, even\nthough those may represent legitimate bugs (code or documentation).\n-- \nbrian m. carlson / brian with sandals: Houston, Texas, US\n+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only\nOpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187\n"},{"id":"252673","messageId":"m5aq2s$njj$1@ger.gmane.org","threadId":"38003","inReplyTo":"547897F4.5000305@xiplink.com","subject":"Re: Our cumbersome mailing list workflow","fromName":"Damien Robert","fromEmail":"damien.olivier.robert+gmane@gmail.com","sentAt":"2014-11-28T21:39:40Z","receivedAt":"2014-11-28T21:39:40Z","isPatch":false,"sender":{"key":"damien.olivier.robert+gmane@gmail.com","avatar":null},"body":"> A bot could subscribe to the list and create branches in a public repo.\n> (This idea feels familiar -- didn't somebody attempt this already?)\n\nThomas Rast maintains git notes that link git commits to their gmane\ndiscussion, you can get them with\n\n[remote \"mailnotes\"]\n url = git://github.com/trast/git.git\n fetch = refs/heads/notes/*:refs/notes/*\n\nThere is gmane branch and a message-id branch, its pretty usefull.\n"},{"id":"252758","messageId":"xmqqh9xgrssc.fsf@gitster.dls.corp.google.com","threadId":"38003","inReplyTo":"547895F1.1010307@alum.mit.edu","subject":"Re: Our cumbersome mailing list workflow","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-12-01T02:46:27Z","receivedAt":"2014-12-01T02:46:27Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> It seems like a few desirable features are being talked about here, and\n> summarizing the discussion as \"centralized\" vs \"decentralized\" is too\n> simplistic. What is really important?\n>\n> 1. Convenient and efficient, including for newcomers\n> 2. Usable while offline\n> 3. Usable in pure-text mode\n> 4. Decentralized\n>\n> Something else?\n\nAs a reviewer / contributor (not speaking as the top maintainer), I\nwould say that everything in one place, and for that one place\nmailbox is preferrable.\n\n\"Somebody commented on (this instance of | the central) Gerrit, come\nlook at it\" is not usable; sending that comment out to those who\nwork in their MUA, and allowing them to respond via their MUA\nprobably adding their response as a new comment to Gerrit) would be\nusable.\n\nWhen I had to view a large-ish series by Ronnie on Gerrit, it was\nfairly painful. The interaction on an individual patch might be\nmore convenient and efficient using a system like Gerrit than via\ne-mailed patch with reply messages, but as a vehicle to review a\nlarge series and see how the whole thing fits together, I did not\nfind pages that made it usable (I am avoiding to say \"I found it\nunusable\", as that impression may be purely from that I couldn't\nfind a more suitable pages that showed the same information in more\nusable form, i.e. user inexperience).\n\nSpeaking of the \"whole picture\", I am hesitant to see us pushed into\nthe \"here is a central system (or here are federated systems) to\nhandle only the patch reviews\" direction; our changes result after\ndiscussing unrelated features, wishes, or bugs that happen outside\nof any specific patches with enough frequency, and that is why I\nprefer \"everything in one place\" aspect of the development based on\nthe mailing list. That is not to say that the \"one place\" has\nforever to be the mailing list, though. But the tooling around an\ne-mail based workflow (e.g. marking threads as \"worth revisiting\"\nfor later inspection, saving chosen messages into a mailbox and\nrunning \"git am\" on it) is already something I am used to. Whatever\nsystem we might end up migrating to, the convenience it offers has\nto beat the convenience of existing workflow to be worth switching\nto, at least to me as a reviewer/contributor.\n\nAs the maintainer, I am not worried too much. As long as the\nmechanism can (1) reach \"here is a series that is accepted by\nreviewers whose opinions are trusted\" efficiently, and (2) allow\nme to queue the result without mistakes, I can go along with\nanything reasonable.\n"},{"id":"252910","messageId":"CAGZ79kagELCSkZ0CA1A7gc7CifjToYmb4kiBYQCse3Q7Hwca5Q@mail.gmail.com","threadId":"38003","inReplyTo":"xmqqh9xgrssc.fsf@gitster.dls.corp.google.com","subject":"Re: Our cumbersome mailing list workflow","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-12-03T02:20:14Z","receivedAt":"2014-12-03T02:20:14Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Sun, Nov 30, 2014 at 6:46 PM, Junio C Hamano wrote:\n> Michael Haggerty writes:\n>\n>> It seems like a few desirable features are being talked about here, and\n>> summarizing the discussion as \"centralized\" vs \"decentralized\" is too\n>> simplistic. What is really important?\n>>\n>> 1. Convenient and efficient, including for newcomers\n>> 2. Usable while offline\n>> 3. Usable in pure-text mode\n>> 4. Decentralized\n>>\n>> Something else?\n>\n\nSo when I started overtaking the ref log series by Ronnie,\nRonnies main concern was missing reviewers time. So my idea was to\nmake it as accessible as possible, so the reviewing party can use their\ntime best. However here are a few points, I want to mention:\n\n * Having send emails as well as uploaded it to Gerrit, I either needed\n a ChangeId (Gerrit strictly requires them to track inter-patch\ndiffs), and the\n mailing list here strictly avoids them, so I was told.\n Ok, that's my problem as I wasn't following the actual procedure of the\n Git development model (mailing list only).\n * That's why I stopped uploads to Gerrit, so I do not need to care about the\n ChangeIds any more. I am not sure if that improved the quality of my patches\n though.\n * I seem to not have found the right workflow with the mailing list yet, as I\n personally find copying around the inter-patch changelog very inconvenient.\n Most of the regulars here just need fewer iterations, so I can understand,\n that you find it less annoying. Hopefully I'll also get used to the\nnit-picky things\n and will require less review iterations in the future.\n How are non-regulars/newcomers, who supposingly need more iterations on\n a patch, supposed to handle the inter patch change log conveniently?\n I tried to keep the inter patch changelog be part of the commit message and\n then just before sending the email, I'd move it the non-permanent section of\n the email.\n * Editing patches as text files is hard/annoying. I have setup git send-email,\n and that works awesome, as I'd only need one command to send off a series.\n Having a step in between makes it more error-prone. So I do git format-patch\n and then inject the inter patch change log, check to remove ChangeId and then\n use git send-email. And at that final manual step I realized I am\nfar from being\n perfect, so sometimes patches arrive on the mailing list, which are\nsub quality\n in the sense, that there are leftovers, i.e. a ChangeId\n * A possible feature, which just comes to my mind:\n Would it make sense for format-patch to not just show the diff\nstats, but also\n include, on which branch it applies? In git.git this is usually the\norigin/master\n branch, but dealing with patch series, building on top of each other that may\n be a good feature to have.\n\n>\n> When I had to view a large-ish series by Ronnie on Gerrit, it was\n> fairly painful. The interaction on an individual patch might be\n> more convenient and efficient using a system like Gerrit than via\n> e-mailed patch with reply messages, but as a vehicle to review a\n> large series and see how the whole thing fits together, I did not\n> find pages that made it usable (I am avoiding to say \"I found it\n> unusable\", as that impression may be purely from that I couldn't\n> find a more suitable pages that showed the same information in more\n> usable form, i.e. user inexperience).\n\nSo you're liking the email workflow more. How do you do the final\nformatting of an email, such as including the inter patch diff?\n\n\n>\n> Speaking of the \"whole picture\", I am hesitant to see us pushed into\n> the \"here is a central system (or here are federated systems) to\n> handle only the patch reviews\" direction; our changes result after\n> discussing unrelated features, wishes, or bugs that happen outside\n> of any specific patches with enough frequency, and that is why I\n> prefer \"everything in one place\" aspect of the development based on\n> the mailing list. That is not to say that the \"one place\" has\n> forever to be the mailing list, though. But the tooling around an\n> e-mail based workflow (e.g. marking threads as \"worth revisiting\"\n> for later inspection, saving chosen messages into a mailbox and\n> running \"git am\" on it) is already something I am used to. Whatever\n> system we might end up migrating to, the convenience it offers has\n> to beat the convenience of existing workflow to be worth switching\n> to, at least to me as a reviewer/contributor.\n\nI do like the way as well to just mark emails unread when I need\nto work on them later.\n\n>\n> As the maintainer, I am not worried too much. As long as the\n> mechanism can (1) reach \"here is a series that is accepted by\n> reviewers whose opinions are trusted\" efficiently, and (2) allow\n> me to queue the result without mistakes, I can go along with\n> anything reasonable.\n>\n"},{"id":"252918","messageId":"20141203035347.GH6527@google.com","threadId":"38003","inReplyTo":"CAGZ79kagELCSkZ0CA1A7gc7CifjToYmb4kiBYQCse3Q7Hwca5Q@mail.gmail.com","subject":"Re: Our cumbersome mailing list workflow","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-12-03T03:53:47Z","receivedAt":"2014-12-03T03:53:47Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Stefan Beller wrote:\n\n> How are non-regulars/newcomers, who supposingly need more iterations on\n> a patch, supposed to handle the inter patch change log conveniently?\n\nI think this is one of the more important issues.\n\nI don't think there's any reason that newcomers should need more\niterations than regulars to finish a patch. Regulars are actually\nheld to a higher standard, so they are likely to need more iterations.\n\nA common mistake for newcomers, that I haven't learned yet how to warn\nproperly against, is to keep re-sending minor iterations on a patch\ntoo quickly. Some ways to avoid that:\n\n * feel free to respond to review comments with something like \"how\n about this?\" and a copy/pasted block of code that just addresses\n that one comment. That way, you can clear up ambiguity and avoid\n the work of applying that change to the entire patch if it ends\n up seeming like a bad idea. This also avoids having to re-send a\n larger patch or series multiple times to clear up a small ambiguity\n from a review.\n\n * be proactive. Look for other examples of the same issue that a\n reviewer pointed out once so they don't have to find it again\n elsewhere in the next iteration. Run the testsuite. Build with\n the flags from\n https://kernel.googlesource.com/pub/scm/git/git/+/todo/Make#106\n in CFLAGS in config.mak. Proofread and try to read as though you\n knew nothing about the patch to anticipate what reviewers will\n find.\n\n * feel free to get more review out-of-band, too. If you're still\n playing with ideas and want someone to take a quick glance before\n the patches are in reviewable form, you can do that and say so\n (e.g., with 'RFC/' before 'PATCH' in the subject line).\n\nJonathan\n"},{"id":"252960","messageId":"xmqq7fy8k5yv.fsf@gitster.dls.corp.google.com","threadId":"38003","inReplyTo":"20141203035347.GH6527@google.com","subject":"Re: Our cumbersome mailing list workflow","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-12-03T17:18:00Z","receivedAt":"2014-12-03T17:18:00Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> I don't think there's any reason that newcomers should need more\n> iterations than regulars to finish a patch. Regulars are actually\n> held to a higher standard, so they are likely to need more iterations.\n>\n> A common mistake for newcomers, that I haven't learned yet how to warn\n> properly against, is to keep re-sending minor iterations on a patch\n> too quickly. Some ways to avoid that:\n>\n> * feel free to respond to review comments with something like \"how\n> about this?\" and a copy/pasted block of code that just addresses\n> that one comment. That way, you can clear up ambiguity and avoid\n> the work of applying that change to the entire patch if it ends\n> up seeming like a bad idea. This also avoids having to re-send a\n> larger patch or series multiple times to clear up a small ambiguity\n> from a review.\n\nThis can go both ways. A trivial improvement can be suggested that\nway by the reviewer.\n\n> * be proactive. Look for other examples of the same issue that a\n> reviewer pointed out once so they don't have to find it again\n> elsewhere in the next iteration....\n> * feel free to get more review out-of-band, too. If you're still\n> playing with ideas and want someone to take a quick glance before\n> the patches are in reviewable form, you can do that and say so\n> (e.g., with 'RFC/' before 'PATCH' in the subject line).\n\nOverall, good suggestions.\n\nThanks.\n"},{"id":"252962","messageId":"547F4828.3000801@web.de","threadId":"38003","inReplyTo":"CAGZ79kagELCSkZ0CA1A7gc7CifjToYmb4kiBYQCse3Q7Hwca5Q@mail.gmail.com","subject":"Re: Our cumbersome mailing list workflow","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2014-12-03T17:28:08Z","receivedAt":"2014-12-03T17:28:08Z","isPatch":false,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 2014-12-03 03.20, Stefan Beller wrote:\n> On Sun, Nov 30, 2014 at 6:46 PM, Junio C Hamano wrote:\n>> Michael Haggerty writes:\n>>\n>>> It seems like a few desirable features are being talked about here, and\n>>> summarizing the discussion as \"centralized\" vs \"decentralized\" is too\n>>> simplistic. What is really important?\n>>>\n>>> 1. Convenient and efficient, including for newcomers\n>>> 2. Usable while offline\n>>> 3. Usable in pure-text mode\n>>> 4. Decentralized\n>>>\n>>> Something else?\n> So when I started overtaking the ref log series by Ronnie,\n> Ronnies main concern was missing reviewers time. So my idea was to\n> make it as accessible as possible, so the reviewing party can use their\n> time best. However here are a few points, I want to mention:\n>\n> * Having send emails as well as uploaded it to Gerrit, I either needed\n> a ChangeId (Gerrit strictly requires them to track inter-patch\n> diffs), and the\n> mailing list here strictly avoids them, so I was told.\n> Ok, that's my problem as I wasn't following the actual procedure of the\n> Git development model (mailing list only).\n> * That's why I stopped uploads to Gerrit, so I do not need to care about the\n> ChangeIds any more. I am not sure if that improved the quality of my patches\n> though.\n> * I seem to not have found the right workflow with the mailing list yet, as I\n> personally find copying around the inter-patch changelog very inconvenient.\n> Most of the regulars here just need fewer iterations, so I can understand,\n> that you find it less annoying. Hopefully I'll also get used to the\n> nit-picky things\n> and will require less review iterations in the future.\n> How are non-regulars/newcomers, who supposingly need more iterations on\n> a patch, supposed to handle the inter patch change log conveniently?\n> I tried to keep the inter patch changelog be part of the commit message and\n> then just before sending the email, I'd move it the non-permanent section of\n> the email.\n> * Editing patches as text files is hard/annoying.\nNot sure if I understand. Editing text files isn't that hard, we do it all the time.\n> I have setup git send-email,\n> and that works awesome, as I'd only need one command to send off a series.\n> Having a step in between makes it more error-prone. So I do git format-patch\n> and then inject the inter patch change log, check to remove ChangeId and then\n> use git send-email.\nHow do you \"inject the inter patch change log\" ? Is that manually, or is it a script ?\n> And at that final manual step I realized I am\n> far from being\n> perfect, so sometimes patches arrive on the mailing list, which are\n> sub quality\n> in the sense, that there are leftovers, i.e. a ChangeId\n> * A possible feature, which just comes to my mind:\n> Would it make sense for format-patch to not just show the diff\n> stats, but also\n> include, on which branch it applies? In git.git this is usually the\n> origin/master\n> branch, but dealing with patch series, building on top of each other that may\n> be a good feature to have.\n>\n\nThanks for the description (and everybody for the discussion)\nIn the hope that it may help, I can try to describe my work flow:\n- Run a script to send the patch (this is a real example)\n#################\n\nSRCCOMMIT=119efe90bffee688a3c37d4358667\nDSTCOMMIT=$(git log --oneline -n1 | awk '{print $1}')\nVERSION=\"-v 1\"\n\nPATCHFILE=$( echo $0 | sed -e 's/\\.sh$/.patch/')\nGIT_TEST_LONG=t\nexport GIT_TEST_LONG\ngit am --abort || :\n( test -s $PATCHFILE || \n\tgit format-patch $VERSION -s --to=git@vger.kernel.org --cc=tboegi@web.de --cc=mhagger@alum.mit.edu --stdout $SRCCOMMIT..$DSTCOMMIT >$PATCHFILE ) &&\ngit checkout $SRCCOMMIT &&\ngit am <$PATCHFILE &&\ncd t && cd .. && make &&\n(cd t && ./t0001*.sh) &&\ngit imap-send <$PATCHFILE\n\n#####################\nThe script formats a patch file (if that does not exist),\napplies the patch on the source commit,\nruns make and then the test cases to verify that the patch works.\n(For bigger patches more tests or the whole test suite should be run,\nfor this very isolated work it OK to run a singe test)\n\nOnce everything is OK, the patch is stored both on disc and in the Drafts folder of the \"email program\".\n(In your case you can use grep to remove the ChangedId or to check that it had been removed)\n\nNow it is time to \"tweak\" the patch file with an editor:\nAdd what has been changed since V1....\nSave the patch file, run the script again to verify that the patch still applies and works and\nput it into the Drafts folder of the mail program.\n\n(That's why I abort the \"git imap-send\" in the first round\nand press ^C when the password is asked)\n\nStart the favorite email program\n(Kmail works, or Thunderbird or \n every other program that can send email in \"plain text\")\n\nHave a final look at the patch in the email prgram\n(remove the V1 from the header, change PATCH into PATCH/RFC).\n\nLet the spell checker look at it, re-read once more.\nIf everything is OK, press the \"send\" button.\n\nIf I send out a V2 version, make a copy of the script, and call it doit2.sh,\nchange what needs to be changed.\nWe can enhance the script to push to a global repo, create a new branch just to\nbe sure we re-find our work...\n\nI store all these scripts under a folder in my home directory,\neach script has it's own directory, this for example is under\n141119_check_file_mode_for_SAMBA/.\nAnd if I am afraid that I don't know where it ended,\nI can make a comment file here and notice that Junio picked it up here:\njunio/tb/config-core-filemode-check-on-broken-fs\n(And the remote junio is \"git://github.com/gitster/git.git\")\n\nThe good thing is that both the script and the patch file can be put\nunder version control.\n\n\nI realized that re-checking the email which is rally send out to the list\nis worth the time and effort.\nSometimes I keep it in the Drafts folder over night, and have\na new look with fresh eyes the next day.\n"},{"id":"253011","messageId":"221286D608764D5EA342E08097333279@PhilipOakley","threadId":"38003","inReplyTo":"5473CD28.5020405@alum.mit.edu","subject":"Re: Our cumbersome mailing list workflow","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.org","sentAt":null,"receivedAt":"2014-12-03T23:57:42Z","isPatch":false,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"From: \"Michael Haggerty\" \nSent: Tuesday, November 25, 2014 12:28 AM\n> On 11/21/2014 07:00 PM, Junio C Hamano wrote:\n>> Michael Haggerty writes:\n>>\n>>> I don't think that those iterations changed anything substantial \n>>> that\n>>> overlaps with my version, but TBH it's such a pain in the ass \n>>> working\n>>> with patches in email that I don't think I'll go to the effort of\n>>> checking for sure unless somebody shows interest in actually using \n>>> my\n>>> version.\n>>>\n>>> Sorry for being grumpy today :-(\n[..]\n> Let me list the aspects of our mailing list workflow that I find\n> cumbersome as a contributor and reviewer:\n>\n> * Submitting patches to the mailing list is an ordeal of configuring\n> format-patch and send-email and getting everything just right, using\n> instructions that depend on the local environment. We saw that hardly\n> any GSoC applicants were able to get it right on their first attempt.\n> Submitting a patch series should be as simple as \"git push\".\n>\n> * Once patches are submitted, there is no assurance that you (Junio)\n> will apply them to your tree at the same point that the submitter\n> developed and tested them.\n>\n> * The branch name that you choose for a patch series is not easily\n> derivable from the patches as they appeared in the mailing list. \n> Trying\n> to figure out whether/where the patches exist in your tree is a \n> largely\n> manual task. The reverse mapping, from in-tree commit to the email \n> where\n> it was proposed, is even more difficult to infer.\n>\n> * Your tree has no indication of which version of a patch series (v1,\n> v2, etc) is currently applied.\n>\n> The previous three points combine to make it awkward to get patches \n> into\n> my local repository to review or test. There are two alternatives, \n> both\n> cumbersome and imprecise:\n>\n> * I do \"git fetch gitster\", then try to figure out whether the branch\n> I'm interested in is present, what its name is, and whether the \n> version\n> in your tree is the latest version, then \"git checkout xy/foobar\".\n>\n\nI had a thought about the issue of version labeling and of keeping the \nold patch series hanging about during development that I felt was worth \nrecording.\n\nMy thought was that while the cover letter and series version number are \ncurrently stripped out from the start of the series, they could be added \nback as a supplemental commit at the end of the series (an --allow-empty \ncommit). This could contain all of the patch subject lines and their \npost '---' notes as appropriate.\n\nThus the series branch would appear to have an extra commit (compared to \nthe current process) after the original tip's possible merge into say \npu.\n\nWhen subsequent series are sent to the list, the new supplemental commit \nwould be a 'merge', with its second parent being the old series, thus \nthe old series is not lost until the branch is deleted, and the existing \nmerge pattern is retained.\n\nClearly if this would need some additional coding as it's not suitable \nas a manual process, but it could be just as automatic as the current \nprocess while providing that little bit of additional visibility.\n\nBelow, I've tried to set out how the commit graph might look (oldest to \nthe left). Hopefully my MUA won't ruin it.\nThe first patch series branches at A, and is merged at D, with the \nsupplemental commit labeled with v1z.\n\nWhen the new series arrives, and pu is rewound, we have the new series \napplied from G (which in reality may not be linked directly from A), and \nmerged back at K. However the new v2z supplemental commit is now the \npo/patches\nbranch head, and is also a merge back to v1z.\n\npatch series 1 (cover letter z)\n- A - B - C - D - E - F <- pu\n \\ /\n v1a-v1b--v1z <-po/patches\n\npatch series 2\n- A - G - H - I - J - K <- pu (note re-wound)\n | \\ / (merge D lost)\n \\ v2a-v2b-v2c--v2z <-po/patches\n \\ /\n v1a-v1b--v1z - - -.\n\nThe key idea here is to use the existing branching model, but then to \nadd the cover letter and other details at the end, rather than the \nbeginning as might have been expected from the email transmit sequence.\n\nPhilip\n"},{"id":"253015","messageId":"CAGZ79kbV8hjnE1tyPdmwZTcrd4WfOu6gSOYvAqnF_bykL2j8Sw@mail.gmail.com","threadId":"38003","inReplyTo":"221286D608764D5EA342E08097333279@PhilipOakley","subject":"Re: Our cumbersome mailing list workflow","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-12-04T02:03:40Z","receivedAt":"2014-12-04T02:03:40Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"> Editing text files isn't that hard, we do it all the time.\n\nIt is not indeed. But doing it all over again and again is hard and error prone.\nI did re-read the man page on git format-patch and found the --notes\noption, which I am going to try\nto use in my workflow. That way I only need to update the notes\ninstead of redoing them all the time.\nBy redoing it I mean copying the changelog from the last time I sent\nthe patch and adding new entries.\n\n> My thought was that while the cover letter and series version number are currently stripped out from the start of the series, they could be added back as a supplemental commit at the end of the series (an --allow-empty commit). This could contain all of the patch subject lines and their post '---' notes as appropriate.\n\nThis sounds interesting. The only changes I can see here are the\nreferenced message ids, so it would be worthwhile to have the last\npatch sent out first and all other patches 1..n-1 referencing the last\nempty commit.\nIf additionally the numbering is corrected, the reader of the mailing\nlist would not notice any difference to the status quo, just the\nsender would have the convenience to be able to track the cover letter\nas an empty commit on top of a series.\n"}]}