[PATCH] refs.c: use a stringlist for repack_without_refs

[PATCH] refs.c: use a stringlist for repack_without_refs

[Stefan Beller]

This patch was heavily inspired by a part of the ref-transactions-rename
series[1], but people tend to dislike large series and this part is
relatively easy to take out and unrelated, so I'll send it as a single
patch.

This patch doesn't intend any functional changes. It is just a refactoring, 
which replaces a char** array by a stringlist in the function 
repack_without_refs.

[1] https://www.mail-archive.com/git@vger.kernel.org/msg60604.html

Idea-by: Ronnie Sahlberg <sahlberg@google.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
 builtin/remote.c | 22 +++++++---------------
 refs.c           | 41 ++++++++++++++++++++---------------------
 refs.h           |  3 +--
 3 files changed, 28 insertions(+), 38 deletions(-)

diff --git a/builtin/remote.c b/builtin/remote.c
index 7f28f92..dca4ebf 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -750,16 +750,11 @@ static int mv(int argc, const char **argv)
 static int remove_branches(struct string_list *branches)
 {
 	struct strbuf err = STRBUF_INIT;
-	const char **branch_names;
 	int i, result = 0;
 
-	branch_names = xmalloc(branches->nr * sizeof(*branch_names));
-	for (i = 0; i < branches->nr; i++)
-		branch_names[i] = branches->items[i].string;
-	if (repack_without_refs(branch_names, branches->nr, &err))
+	if (repack_without_refs(branches, &err))
 		result |= error("%s", err.buf);
 	strbuf_release(&err);
-	free(branch_names);
 
 	for (i = 0; i < branches->nr; i++) {
 		struct string_list_item *item = branches->items + i;
@@ -1317,7 +1312,6 @@ static int prune_remote(const char *remote, int dry_run)
 	int result = 0, i;
 	struct ref_states states;
 	struct string_list delete_refs_list = STRING_LIST_INIT_NODUP;
-	const char **delete_refs;
 	const char *dangling_msg = dry_run
 		? _(" %s will become dangling!")
 		: _(" %s has become dangling!");
@@ -1325,6 +1319,11 @@ static int prune_remote(const char *remote, int dry_run)
 	memset(&states, 0, sizeof(states));
 	get_remote_ref_states(remote, &states, GET_REF_STATES);
 
+	for (i = 0; i < states.stale.nr; i++)
+		string_list_insert(&delete_refs_list,
+				   states.stale.items[i].util);
+
+
 	if (states.stale.nr) {
 		printf_ln(_("Pruning %s"), remote);
 		printf_ln(_("URL: %s"),
@@ -1332,24 +1331,17 @@ static int prune_remote(const char *remote, int dry_run)
 		       ? states.remote->url[0]
 		       : _("(no URL)"));
 
-		delete_refs = xmalloc(states.stale.nr * sizeof(*delete_refs));
-		for (i = 0; i < states.stale.nr; i++)
-			delete_refs[i] = states.stale.items[i].util;
 		if (!dry_run) {
 			struct strbuf err = STRBUF_INIT;
-			if (repack_without_refs(delete_refs, states.stale.nr,
-						&err))
+			if (repack_without_refs(&delete_refs_list, &err))
 				result |= error("%s", err.buf);
 			strbuf_release(&err);
 		}
-		free(delete_refs);
 	}
 
 	for (i = 0; i < states.stale.nr; i++) {
 		const char *refname = states.stale.items[i].util;
 
-		string_list_insert(&delete_refs_list, refname);
-
 		if (!dry_run)
 			result |= delete_ref(refname, NULL, 0);
 
diff --git a/refs.c b/refs.c
index 5ff457e..2333a9b 100644
--- a/refs.c
+++ b/refs.c
@@ -2639,23 +2639,23 @@ static int curate_packed_ref_fn(struct ref_entry *entry, void *cb_data)
 	return 0;
 }
 
-int repack_without_refs(const char **refnames, int n, struct strbuf *err)
+int repack_without_refs(struct string_list *without, struct strbuf *err)
 {
 	struct ref_dir *packed;
 	struct string_list refs_to_delete = STRING_LIST_INIT_DUP;
 	struct string_list_item *ref_to_delete;
-	int i, ret, removed = 0;
+	int count, ret, removed = 0;
 
 	assert(err);
 
-	/* Look for a packed ref */
-	for (i = 0; i < n; i++)
-		if (get_packed_ref(refnames[i]))
-			break;
+	count = 0;
+	for_each_string_list_item(ref_to_delete, without)
+		if (get_packed_ref(ref_to_delete->string))
+			count++;
 
-	/* Avoid locking if we have nothing to do */
-	if (i == n)
-		return 0; /* no refname exists in packed refs */
+	/* No refname exists in packed refs */
+	if (!count)
+		return 0;
 
 	if (lock_packed_refs(0)) {
 		unable_to_lock_message(git_path("packed-refs"), errno, err);
@@ -2664,8 +2664,8 @@ int repack_without_refs(const char **refnames, int n, struct strbuf *err)
 	packed = get_packed_refs(&ref_cache);
 
 	/* Remove refnames from the cache */
-	for (i = 0; i < n; i++)
-		if (remove_entry(packed, refnames[i]) != -1)
+	for_each_string_list_item(ref_to_delete, without)
+		if (remove_entry(packed, ref_to_delete->string) != -1)
 			removed = 1;
 	if (!removed) {
 		/*
@@ -3738,10 +3738,11 @@ static int ref_update_reject_duplicates(struct ref_update **updates, int n,
 int ref_transaction_commit(struct ref_transaction *transaction,
 			   struct strbuf *err)
 {
-	int ret = 0, delnum = 0, i;
-	const char **delnames;
+	int ret = 0, i;
 	int n = transaction->nr;
 	struct ref_update **updates = transaction->updates;
+	struct string_list refs_to_delete = STRING_LIST_INIT_DUP;
+	struct string_list_item *ref_to_delete;
 
 	assert(err);
 
@@ -3753,9 +3754,6 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 		return 0;
 	}
 
-	/* Allocate work space */
-	delnames = xmalloc(sizeof(*delnames) * n);
-
 	/* Copy, sort, and reject duplicate refs */
 	qsort(updates, n, sizeof(*updates), ref_update_compare);
 	if (ref_update_reject_duplicates(updates, n, err)) {
@@ -3815,16 +3813,17 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 			}
 
 			if (!(update->flags & REF_ISPRUNING))
-				delnames[delnum++] = update->lock->ref_name;
+				string_list_insert(&refs_to_delete,
+						   update->lock->ref_name);
 		}
 	}
 
-	if (repack_without_refs(delnames, delnum, err)) {
+	if (repack_without_refs(&refs_to_delete, err)) {
 		ret = TRANSACTION_GENERIC_ERROR;
 		goto cleanup;
 	}
-	for (i = 0; i < delnum; i++)
-		unlink_or_warn(git_path("logs/%s", delnames[i]));
+	for_each_string_list_item(ref_to_delete, &refs_to_delete)
+		unlink_or_warn(git_path("logs/%s", ref_to_delete->string));
 	clear_loose_ref_cache(&ref_cache);
 
 cleanup:
@@ -3833,7 +3832,7 @@ cleanup:
 	for (i = 0; i < n; i++)
 		if (updates[i]->lock)
 			unlock_ref(updates[i]->lock);
-	free(delnames);
+	string_list_clear(&refs_to_delete, 0);
 	return ret;
 }
 
diff --git a/refs.h b/refs.h
index 2bc3556..0416e5f 100644
--- a/refs.h
+++ b/refs.h
@@ -163,8 +163,7 @@ extern void rollback_packed_refs(void);
  */
 int pack_refs(unsigned int flags);
 
-extern int repack_without_refs(const char **refnames, int n,
-			       struct strbuf *err);
+extern int repack_without_refs(struct string_list *without, struct strbuf *err);
 
 extern int ref_exists(const char *);
 
-- 
2.2.0.rc2.5.gf7b9fb2

Re: [PATCH] refs.c: use a stringlist for repack_without_refs

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> This patch was heavily inspired by a part of the ref-transactions-rename
> series[1], but people tend to dislike large series and this part is
> relatively easy to take out and unrelated, so I'll send it as a single
> patch.
>
> This patch doesn't intend any functional changes. It is just a refactoring, 
> which replaces a char** array by a stringlist in the function 
> repack_without_refs.
>
> [1] https://www.mail-archive.com/git@vger.kernel.org/msg60604.html
>
> Idea-by: Ronnie Sahlberg <sahlberg@google.com>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  builtin/remote.c | 22 +++++++---------------
>  refs.c           | 41 ++++++++++++++++++++---------------------
>  refs.h           |  3 +--
>  3 files changed, 28 insertions(+), 38 deletions(-)

In one codepath we were already using a string_list delete_refs_list
anyway, so it makes sense to reuse that by movingan existing call to
string_list_insert() a bit higher, instead of maintaining another
array of pointers delete_refs[] to strings.

OK, it simplifies the code by reducing the line count, which is a
plus ;-)

Sounds good.

>
> diff --git a/builtin/remote.c b/builtin/remote.c
> index 7f28f92..dca4ebf 100644
> --- a/builtin/remote.c
> +++ b/builtin/remote.c
> @@ -750,16 +750,11 @@ static int mv(int argc, const char **argv)
>  static int remove_branches(struct string_list *branches)
>  {
>  	struct strbuf err = STRBUF_INIT;
> -	const char **branch_names;
>  	int i, result = 0;
>  
> -	branch_names = xmalloc(branches->nr * sizeof(*branch_names));
> -	for (i = 0; i < branches->nr; i++)
> -		branch_names[i] = branches->items[i].string;
> -	if (repack_without_refs(branch_names, branches->nr, &err))
> +	if (repack_without_refs(branches, &err))
>  		result |= error("%s", err.buf);
>  	strbuf_release(&err);
> -	free(branch_names);
>  
>  	for (i = 0; i < branches->nr; i++) {
>  		struct string_list_item *item = branches->items + i;
> @@ -1317,7 +1312,6 @@ static int prune_remote(const char *remote, int dry_run)
>  	int result = 0, i;
>  	struct ref_states states;
>  	struct string_list delete_refs_list = STRING_LIST_INIT_NODUP;
> -	const char **delete_refs;
>  	const char *dangling_msg = dry_run
>  		? _(" %s will become dangling!")
>  		: _(" %s has become dangling!");
> @@ -1325,6 +1319,11 @@ static int prune_remote(const char *remote, int dry_run)
>  	memset(&states, 0, sizeof(states));
>  	get_remote_ref_states(remote, &states, GET_REF_STATES);
>  
> +	for (i = 0; i < states.stale.nr; i++)
> +		string_list_insert(&delete_refs_list,
> +				   states.stale.items[i].util);
> +
> +
>  	if (states.stale.nr) {
>  		printf_ln(_("Pruning %s"), remote);
>  		printf_ln(_("URL: %s"),
> @@ -1332,24 +1331,17 @@ static int prune_remote(const char *remote, int dry_run)
>  		       ? states.remote->url[0]
>  		       : _("(no URL)"));
>  
> -		delete_refs = xmalloc(states.stale.nr * sizeof(*delete_refs));
> -		for (i = 0; i < states.stale.nr; i++)
> -			delete_refs[i] = states.stale.items[i].util;
>  		if (!dry_run) {
>  			struct strbuf err = STRBUF_INIT;
> -			if (repack_without_refs(delete_refs, states.stale.nr,
> -						&err))
> +			if (repack_without_refs(&delete_refs_list, &err))
>  				result |= error("%s", err.buf);
>  			strbuf_release(&err);
>  		}
> -		free(delete_refs);
>  	}
>  
>  	for (i = 0; i < states.stale.nr; i++) {
>  		const char *refname = states.stale.items[i].util;
>  
> -		string_list_insert(&delete_refs_list, refname);
> -
>  		if (!dry_run)
>  			result |= delete_ref(refname, NULL, 0);
>  
> diff --git a/refs.c b/refs.c
> index 5ff457e..2333a9b 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -2639,23 +2639,23 @@ static int curate_packed_ref_fn(struct ref_entry *entry, void *cb_data)
>  	return 0;
>  }
>  
> -int repack_without_refs(const char **refnames, int n, struct strbuf *err)
> +int repack_without_refs(struct string_list *without, struct strbuf *err)
>  {
>  	struct ref_dir *packed;
>  	struct string_list refs_to_delete = STRING_LIST_INIT_DUP;
>  	struct string_list_item *ref_to_delete;
> -	int i, ret, removed = 0;
> +	int count, ret, removed = 0;
>  
>  	assert(err);
>  
> -	/* Look for a packed ref */
> -	for (i = 0; i < n; i++)
> -		if (get_packed_ref(refnames[i]))
> -			break;
> +	count = 0;
> +	for_each_string_list_item(ref_to_delete, without)
> +		if (get_packed_ref(ref_to_delete->string))
> +			count++;
>  
> -	/* Avoid locking if we have nothing to do */
> -	if (i == n)
> -		return 0; /* no refname exists in packed refs */
> +	/* No refname exists in packed refs */
> +	if (!count)
> +		return 0;
>  
>  	if (lock_packed_refs(0)) {
>  		unable_to_lock_message(git_path("packed-refs"), errno, err);
> @@ -2664,8 +2664,8 @@ int repack_without_refs(const char **refnames, int n, struct strbuf *err)
>  	packed = get_packed_refs(&ref_cache);
>  
>  	/* Remove refnames from the cache */
> -	for (i = 0; i < n; i++)
> -		if (remove_entry(packed, refnames[i]) != -1)
> +	for_each_string_list_item(ref_to_delete, without)
> +		if (remove_entry(packed, ref_to_delete->string) != -1)
>  			removed = 1;
>  	if (!removed) {
>  		/*
> @@ -3738,10 +3738,11 @@ static int ref_update_reject_duplicates(struct ref_update **updates, int n,
>  int ref_transaction_commit(struct ref_transaction *transaction,
>  			   struct strbuf *err)
>  {
> -	int ret = 0, delnum = 0, i;
> -	const char **delnames;
> +	int ret = 0, i;
>  	int n = transaction->nr;
>  	struct ref_update **updates = transaction->updates;
> +	struct string_list refs_to_delete = STRING_LIST_INIT_DUP;
> +	struct string_list_item *ref_to_delete;
>  
>  	assert(err);
>  
> @@ -3753,9 +3754,6 @@ int ref_transaction_commit(struct ref_transaction *transaction,
>  		return 0;
>  	}
>  
> -	/* Allocate work space */
> -	delnames = xmalloc(sizeof(*delnames) * n);
> -
>  	/* Copy, sort, and reject duplicate refs */
>  	qsort(updates, n, sizeof(*updates), ref_update_compare);
>  	if (ref_update_reject_duplicates(updates, n, err)) {
> @@ -3815,16 +3813,17 @@ int ref_transaction_commit(struct ref_transaction *transaction,
>  			}
>  
>  			if (!(update->flags & REF_ISPRUNING))
> -				delnames[delnum++] = update->lock->ref_name;
> +				string_list_insert(&refs_to_delete,
> +						   update->lock->ref_name);
>  		}
>  	}
>  
> -	if (repack_without_refs(delnames, delnum, err)) {
> +	if (repack_without_refs(&refs_to_delete, err)) {
>  		ret = TRANSACTION_GENERIC_ERROR;
>  		goto cleanup;
>  	}
> -	for (i = 0; i < delnum; i++)
> -		unlink_or_warn(git_path("logs/%s", delnames[i]));
> +	for_each_string_list_item(ref_to_delete, &refs_to_delete)
> +		unlink_or_warn(git_path("logs/%s", ref_to_delete->string));
>  	clear_loose_ref_cache(&ref_cache);
>  
>  cleanup:
> @@ -3833,7 +3832,7 @@ cleanup:
>  	for (i = 0; i < n; i++)
>  		if (updates[i]->lock)
>  			unlock_ref(updates[i]->lock);
> -	free(delnames);
> +	string_list_clear(&refs_to_delete, 0);
>  	return ret;
>  }
>  
> diff --git a/refs.h b/refs.h
> index 2bc3556..0416e5f 100644
> --- a/refs.h
> +++ b/refs.h
> @@ -163,8 +163,7 @@ extern void rollback_packed_refs(void);
>   */
>  int pack_refs(unsigned int flags);
>  
> -extern int repack_without_refs(const char **refnames, int n,
> -			       struct strbuf *err);
> +extern int repack_without_refs(struct string_list *without, struct strbuf *err);
>  
>  extern int ref_exists(const char *);

Re: [PATCH] refs.c: use a stringlist for repack_without_refs

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Stefan Beller <sbeller@google.com> writes:
>
>> This patch was heavily inspired by a part of the ref-transactions-rename
>> series[1], but people tend to dislike large series and this part is
>> relatively easy to take out and unrelated, so I'll send it as a single
>> patch.
>>
>> This patch doesn't intend any functional changes. It is just a refactoring, 
>> which replaces a char** array by a stringlist in the function 
>> repack_without_refs.
>>
>> [1] https://www.mail-archive.com/git@vger.kernel.org/msg60604.html
>>
>> Idea-by: Ronnie Sahlberg <sahlberg@google.com>
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>> ---
>>  builtin/remote.c | 22 +++++++---------------
>>  refs.c           | 41 ++++++++++++++++++++---------------------
>>  refs.h           |  3 +--
>>  3 files changed, 28 insertions(+), 38 deletions(-)
>
> In one codepath we were already using a string_list delete_refs_list
> anyway, so it makes sense to reuse that by movingan existing call to
> string_list_insert() a bit higher, instead of maintaining another
> array of pointers delete_refs[] to strings.
>
> OK, it simplifies the code by reducing the line count, which is a
> plus ;-)
>
> Sounds good.

I queued this but as I suspected yesterday had to drop all the other
rs/ref-transaction-* topics that are not in 'next' yet.  I am
guessing that your plan is to make them come back one piece at a
time in many easier-to-digest bite sized series.

Re: [PATCH] refs.c: use a stringlist for repack_without_refs

[Jonathan Nieder]

Stefan Beller wrote:

> This patch was heavily inspired by a part of the ref-transactions-rename
> series[1], but people tend to dislike large series and this part is
> relatively easy to take out and unrelated, so I'll send it as a single
> patch.
>
> [1] https://www.mail-archive.com/git@vger.kernel.org/msg60604.html

The above is a useful kind of comment to put below the three-dashes.  It
doesn't explain what the intent behind the patch is, why I should want
this patch when considering whether to upgrade git, or what is going to
break when I consider reverting it as part of fixing something else, so
it doesn't belong in the commit message.

> This patch doesn't intend any functional changes. It is just a refactoring, 
> which replaces a char** array by a stringlist in the function 
> repack_without_refs.

Thanks.  Why, though?  Is it about having something simpler to pass
from builtin/remote.c::remove_branches(), or something else?

> Idea-by: Ronnie Sahlberg <sahlberg@google.com>
> Signed-off-by: Stefan Beller <sbeller@google.com>

Isn't the patch by Ronnie?

Sometimes I send a patch by someone else and make some change that I
don't want them to be blamed for.  Then I keep their sign-off and put
a note in the commit message about the change I made.  See output from

  git log origin/pu --grep='jc:'

for more examples of that.

Some nits below.

> --- a/builtin/remote.c
> +++ b/builtin/remote.c
[...]
> @@ -1325,6 +1319,11 @@ static int prune_remote(const char *remote, int dry_run)
[...]
>  	memset(&states, 0, sizeof(states));
>  	get_remote_ref_states(remote, &states, GET_REF_STATES);
>  
> +	for (i = 0; i < states.stale.nr; i++)
> +		string_list_insert(&delete_refs_list,
> +				   states.stale.items[i].util);
> +
> +
>  	if (states.stale.nr) {

(style) The double blank line looks odd here.

>  		printf_ln(_("Pruning %s"), remote);
>  		printf_ln(_("URL: %s"),
> @@ -1332,24 +1331,17 @@ static int prune_remote(const char *remote, int dry_run)
>  		       ? states.remote->url[0]
>  		       : _("(no URL)"));
>  
> -		delete_refs = xmalloc(states.stale.nr * sizeof(*delete_refs));

Now that there's no delete_refs array duplicating the string list,
would it make sense to rename delete_refs_list to delete_refs?

As a nice side-effect, that would make the definition of
delete_refs_list and other places it is used appear in the patch.

>  	for (i = 0; i < states.stale.nr; i++) {
>  		const char *refname = states.stale.items[i].util;

(optional) this could be

	for_each_string_list_item(ref, &delete_refs_list) {
		const char *refname = ref->string;
		...

which saves the reader from having to remember what states.stale.items
means.

[...]
> +++ b/refs.c
[...]
> @@ -2639,23 +2639,23 @@ int repack_without_refs(struct string_list *without, struct strbuf *err)
[...]
> -	int i, ret, removed = 0;
> +	int count, ret, removed = 0;
>  
>  	assert(err);
>  
> -	/* Look for a packed ref */

The old code has comments marking sections of the function:

	/* Look for a packed ref */
	/* Avoid processing if we have nothing to do */
	/* Remove refnames from the cache */
	/* Remove any other accumulated cruft */
	/* Write what remains */

Is dropping this comment intended?

> -	for (i = 0; i < n; i++)
> -		if (get_packed_ref(refnames[i]))
> -			break;
> +	count = 0;
> +	for_each_string_list_item(ref_to_delete, without)
> +		if (get_packed_ref(ref_to_delete->string))
> +			count++;

The old code breaks out early as soon as it finds a ref to delete.
Can we do similar?

E.g.

	for (i = 0; i < without->nr; i++)
		if (get_packed_ref(without->items[i].string))
			break;

(not about this patch) Is refs_to_delete leaked?

[...]
> @@ -3738,10 +3738,11 @@ static int ref_update_reject_duplicates(struct ref_update **updates, int n,
>  int ref_transaction_commit(struct ref_transaction *transaction,
>  			   struct strbuf *err)
>  {
> -	int ret = 0, delnum = 0, i;
> -	const char **delnames;
> +	int ret = 0, i;
>  	int n = transaction->nr;
>  	struct ref_update **updates = transaction->updates;
> +	struct string_list refs_to_delete = STRING_LIST_INIT_DUP;

The old code doesn't xstrdup the list items, so _NODUP should work
fine (and be slightly more efficient).

[...]
> @@ -3815,16 +3813,17 @@ int ref_transaction_commit(struct ref_transaction *transaction,
>  			}
>  
>  			if (!(update->flags & REF_ISPRUNING))
> -				delnames[delnum++] = update->lock->ref_name;
> +				string_list_insert(&refs_to_delete,
> +						   update->lock->ref_name);

string_list_append would be analagous to the old code.

[....]
> --- a/refs.h
> +++ b/refs.h
> @@ -163,8 +163,7 @@ extern void rollback_packed_refs(void);
>   */
>  int pack_refs(unsigned int flags);
>  
> -extern int repack_without_refs(const char **refnames, int n,
> -			       struct strbuf *err);
> +extern int repack_without_refs(struct string_list *without, struct strbuf *err);

A comment could mention whether the ref list needs to be sorted.  (It
doesn't, right?)

Thanks and hope that helps,
Jonathan

Re: [PATCH] refs.c: use a stringlist for repack_without_refs

[Stefan Beller]

On Tue, Nov 18, 2014 at 3:45 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
>
> The above is a useful kind of comment to put below the three-dashes.  It
> doesn't explain what the intent behind the patch is, why I should want
> this patch when considering whether to upgrade git, or what is going to
> break when I consider reverting it as part of fixing something else, so
> it doesn't belong in the commit message.

Yes, I'll do a resend, removing this paragraph.

>
>> This patch doesn't intend any functional changes. It is just a refactoring,
>> which replaces a char** array by a stringlist in the function
>> repack_without_refs.
>
> Thanks.  Why, though?  Is it about having something simpler to pass
> from builtin/remote.c::remove_branches(), or something else?

Essentially it's simpler to read and maintain as we're having less
lines of code.
I'll add that to the commit message instead.

>
>> Idea-by: Ronnie Sahlberg <sahlberg@google.com>
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>
> Isn't the patch by Ronnie?

As it was part of the ref-transaction-rename series, it was authored by Ronnie.
Porting it back to the master branch brought up so many conflicts,
that I decided
to rewrite it from scratch while having an occasional look at the
original patch.

If you want we can retain Ronnies authorship, however I may have messed up
the rewriting, so I put my name as author and Ronnie as giving the idea.

>
> Sometimes I send a patch by someone else and make some change that I
> don't want them to be blamed for.  Then I keep their sign-off and put
> a note in the commit message about the change I made.  See output from

Sounds reasonable, I can do something similar.

>
>   git log origin/pu --grep='jc:'
>
> for more examples of that.
>
> Some nits below.

Because of the nits, I'd rather be blamed. :)

>
>> --- a/builtin/remote.c
>> +++ b/builtin/remote.c
> [...]
>> @@ -1325,6 +1319,11 @@ static int prune_remote(const char *remote, int dry_run)
> [...]
>>       memset(&states, 0, sizeof(states));
>>       get_remote_ref_states(remote, &states, GET_REF_STATES);
>>
>> +     for (i = 0; i < states.stale.nr; i++)
>> +             string_list_insert(&delete_refs_list,
>> +                                states.stale.items[i].util);
>> +
>> +
>>       if (states.stale.nr) {
>
> (style) The double blank line looks odd here.

will fix

>
>>               printf_ln(_("Pruning %s"), remote);
>>               printf_ln(_("URL: %s"),
>> @@ -1332,24 +1331,17 @@ static int prune_remote(const char *remote, int dry_run)
>>                      ? states.remote->url[0]
>>                      : _("(no URL)"));
>>
>> -             delete_refs = xmalloc(states.stale.nr * sizeof(*delete_refs));
>
> Now that there's no delete_refs array duplicating the string list,
> would it make sense to rename delete_refs_list to delete_refs?
>
> As a nice side-effect, that would make the definition of
> delete_refs_list and other places it is used appear in the patch.
>
>>       for (i = 0; i < states.stale.nr; i++) {
>>               const char *refname = states.stale.items[i].util;
>
> (optional) this could be
>
>         for_each_string_list_item(ref, &delete_refs_list) {
>                 const char *refname = ref->string;
>                 ...
>
> which saves the reader from having to remember what states.stale.items
> means.

done

>
> [...]
>> +++ b/refs.c
> [...]
>> @@ -2639,23 +2639,23 @@ int repack_without_refs(struct string_list *without, struct strbuf *err)
> [...]
>> -     int i, ret, removed = 0;
>> +     int count, ret, removed = 0;
>>
>>       assert(err);
>>
>> -     /* Look for a packed ref */
>
> The old code has comments marking sections of the function:
>
>         /* Look for a packed ref */
>         /* Avoid processing if we have nothing to do */
>         /* Remove refnames from the cache */
>         /* Remove any other accumulated cruft */
>         /* Write what remains */
>
> Is dropping this comment intended?

no, dropped the dropping in the reroll.

>
>> -     for (i = 0; i < n; i++)
>> -             if (get_packed_ref(refnames[i]))
>> -                     break;
>> +     count = 0;
>> +     for_each_string_list_item(ref_to_delete, without)
>> +             if (get_packed_ref(ref_to_delete->string))
>> +                     count++;
>
> The old code breaks out early as soon as it finds a ref to delete.
> Can we do similar?

done

>
> E.g.
>
>         for (i = 0; i < without->nr; i++)
>                 if (get_packed_ref(without->items[i].string))
>                         break;
>
> (not about this patch) Is refs_to_delete leaked?
>
> [...]
>> @@ -3738,10 +3738,11 @@ static int ref_update_reject_duplicates(struct ref_update **updates, int n,
>>  int ref_transaction_commit(struct ref_transaction *transaction,
>>                          struct strbuf *err)
>>  {
>> -     int ret = 0, delnum = 0, i;
>> -     const char **delnames;
>> +     int ret = 0, i;
>>       int n = transaction->nr;
>>       struct ref_update **updates = transaction->updates;
>> +     struct string_list refs_to_delete = STRING_LIST_INIT_DUP;
>
> The old code doesn't xstrdup the list items, so _NODUP should work
> fine (and be slightly more efficient).

ok

>
> [...]
>> @@ -3815,16 +3813,17 @@ int ref_transaction_commit(struct ref_transaction *transaction,
>>                       }
>>
>>                       if (!(update->flags & REF_ISPRUNING))
>> -                             delnames[delnum++] = update->lock->ref_name;
>> +                             string_list_insert(&refs_to_delete,
>> +                                                update->lock->ref_name);
>
> string_list_append would be analagous to the old code.

ok

>
> [....]
>> --- a/refs.h
>> +++ b/refs.h
>> @@ -163,8 +163,7 @@ extern void rollback_packed_refs(void);
>>   */
>>  int pack_refs(unsigned int flags);
>>
>> -extern int repack_without_refs(const char **refnames, int n,
>> -                            struct strbuf *err);
>> +extern int repack_without_refs(struct string_list *without, struct strbuf *err);
>
> A comment could mention whether the ref list needs to be sorted.  (It
> doesn't, right?)

ok, I tried adding comments.

>
> Thanks and hope that helps,
> Jonathan

[PATCH] refs.c: use a stringlist for repack_without_refs

[Stefan Beller]

This patch doesn't intend any functional changes. It is just
a refactoring, which replaces a char** array by a stringlist
in the function repack_without_refs.
This is easier to read and maintain as it delivers the same
functionality with less lines of code less pointers.

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
Signed-off-by: Stefan Beller <sbeller@google.com>

-

This patch was heavily inspired by a part of the ref-transactions-rename
series[1], but people tend to dislike large series and this part is
relatively easy to take out and unrelated, so I'll send it as a single
patch.

[1] https://www.mail-archive.com/git@vger.kernel.org/msg60604.html

---
 builtin/remote.c | 31 +++++++++++--------------------
 refs.c           | 40 +++++++++++++++++++++-------------------
 refs.h           | 10 ++++++++--
 3 files changed, 40 insertions(+), 41 deletions(-)

diff --git a/builtin/remote.c b/builtin/remote.c
index 7f28f92..5f5fa4c 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -750,16 +750,11 @@ static int mv(int argc, const char **argv)
 static int remove_branches(struct string_list *branches)
 {
 	struct strbuf err = STRBUF_INIT;
-	const char **branch_names;
 	int i, result = 0;
 
-	branch_names = xmalloc(branches->nr * sizeof(*branch_names));
-	for (i = 0; i < branches->nr; i++)
-		branch_names[i] = branches->items[i].string;
-	if (repack_without_refs(branch_names, branches->nr, &err))
+	if (repack_without_refs(branches, &err))
 		result |= error("%s", err.buf);
 	strbuf_release(&err);
-	free(branch_names);
 
 	for (i = 0; i < branches->nr; i++) {
 		struct string_list_item *item = branches->items + i;
@@ -1316,8 +1311,8 @@ static int prune_remote(const char *remote, int dry_run)
 {
 	int result = 0, i;
 	struct ref_states states;
-	struct string_list delete_refs_list = STRING_LIST_INIT_NODUP;
-	const char **delete_refs;
+	struct string_list delete_refs = STRING_LIST_INIT_NODUP;
+	struct string_list_item *ref;
 	const char *dangling_msg = dry_run
 		? _(" %s will become dangling!")
 		: _(" %s has become dangling!");
@@ -1325,6 +1320,9 @@ static int prune_remote(const char *remote, int dry_run)
 	memset(&states, 0, sizeof(states));
 	get_remote_ref_states(remote, &states, GET_REF_STATES);
 
+	for_each_string_list_item(ref, &delete_refs)
+		string_list_append(&delete_refs, ref->string);
+
 	if (states.stale.nr) {
 		printf_ln(_("Pruning %s"), remote);
 		printf_ln(_("URL: %s"),
@@ -1332,23 +1330,16 @@ static int prune_remote(const char *remote, int dry_run)
 		       ? states.remote->url[0]
 		       : _("(no URL)"));
 
-		delete_refs = xmalloc(states.stale.nr * sizeof(*delete_refs));
-		for (i = 0; i < states.stale.nr; i++)
-			delete_refs[i] = states.stale.items[i].util;
 		if (!dry_run) {
 			struct strbuf err = STRBUF_INIT;
-			if (repack_without_refs(delete_refs, states.stale.nr,
-						&err))
+			if (repack_without_refs(&delete_refs, &err))
 				result |= error("%s", err.buf);
 			strbuf_release(&err);
 		}
-		free(delete_refs);
 	}
 
-	for (i = 0; i < states.stale.nr; i++) {
-		const char *refname = states.stale.items[i].util;
-
-		string_list_insert(&delete_refs_list, refname);
+	for_each_string_list_item(ref, &delete_refs) {
+		const char *refname = ref->string;
 
 		if (!dry_run)
 			result |= delete_ref(refname, NULL, 0);
@@ -1361,8 +1352,8 @@ static int prune_remote(const char *remote, int dry_run)
 			       abbrev_ref(refname, "refs/remotes/"));
 	}
 
-	warn_dangling_symrefs(stdout, dangling_msg, &delete_refs_list);
-	string_list_clear(&delete_refs_list, 0);
+	warn_dangling_symrefs(stdout, dangling_msg, &delete_refs);
+	string_list_clear(&delete_refs, 0);
 
 	free_remote_ref_states(&states);
 	return result;
diff --git a/refs.c b/refs.c
index 5ff457e..2f6e08b 100644
--- a/refs.c
+++ b/refs.c
@@ -2639,23 +2639,26 @@ static int curate_packed_ref_fn(struct ref_entry *entry, void *cb_data)
 	return 0;
 }
 
-int repack_without_refs(const char **refnames, int n, struct strbuf *err)
+int repack_without_refs(struct string_list *without, struct strbuf *err)
 {
 	struct ref_dir *packed;
 	struct string_list refs_to_delete = STRING_LIST_INIT_DUP;
 	struct string_list_item *ref_to_delete;
-	int i, ret, removed = 0;
+	int ret, needs_repacking = 0, removed = 0;
 
 	assert(err);
 
 	/* Look for a packed ref */
-	for (i = 0; i < n; i++)
-		if (get_packed_ref(refnames[i]))
+	for_each_string_list_item(ref_to_delete, without) {
+		if (get_packed_ref(ref_to_delete->string)) {
+			needs_repacking = 1;
 			break;
+		}
+	}
 
-	/* Avoid locking if we have nothing to do */
-	if (i == n)
-		return 0; /* no refname exists in packed refs */
+	/* No refname exists in packed refs */
+	if (!needs_repacking)
+		return 0;
 
 	if (lock_packed_refs(0)) {
 		unable_to_lock_message(git_path("packed-refs"), errno, err);
@@ -2664,8 +2667,8 @@ int repack_without_refs(const char **refnames, int n, struct strbuf *err)
 	packed = get_packed_refs(&ref_cache);
 
 	/* Remove refnames from the cache */
-	for (i = 0; i < n; i++)
-		if (remove_entry(packed, refnames[i]) != -1)
+	for_each_string_list_item(ref_to_delete, without)
+		if (remove_entry(packed, ref_to_delete->string) != -1)
 			removed = 1;
 	if (!removed) {
 		/*
@@ -3738,10 +3741,11 @@ static int ref_update_reject_duplicates(struct ref_update **updates, int n,
 int ref_transaction_commit(struct ref_transaction *transaction,
 			   struct strbuf *err)
 {
-	int ret = 0, delnum = 0, i;
-	const char **delnames;
+	int ret = 0, i;
 	int n = transaction->nr;
 	struct ref_update **updates = transaction->updates;
+	struct string_list refs_to_delete = STRING_LIST_INIT_NODUP;
+	struct string_list_item *ref_to_delete;
 
 	assert(err);
 
@@ -3753,9 +3757,6 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 		return 0;
 	}
 
-	/* Allocate work space */
-	delnames = xmalloc(sizeof(*delnames) * n);
-
 	/* Copy, sort, and reject duplicate refs */
 	qsort(updates, n, sizeof(*updates), ref_update_compare);
 	if (ref_update_reject_duplicates(updates, n, err)) {
@@ -3815,16 +3816,17 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 			}
 
 			if (!(update->flags & REF_ISPRUNING))
-				delnames[delnum++] = update->lock->ref_name;
+				string_list_append(&refs_to_delete,
+						   update->lock->ref_name);
 		}
 	}
 
-	if (repack_without_refs(delnames, delnum, err)) {
+	if (repack_without_refs(&refs_to_delete, err)) {
 		ret = TRANSACTION_GENERIC_ERROR;
 		goto cleanup;
 	}
-	for (i = 0; i < delnum; i++)
-		unlink_or_warn(git_path("logs/%s", delnames[i]));
+	for_each_string_list_item(ref_to_delete, &refs_to_delete)
+		unlink_or_warn(git_path("logs/%s", ref_to_delete->string));
 	clear_loose_ref_cache(&ref_cache);
 
 cleanup:
@@ -3833,7 +3835,7 @@ cleanup:
 	for (i = 0; i < n; i++)
 		if (updates[i]->lock)
 			unlock_ref(updates[i]->lock);
-	free(delnames);
+	string_list_clear(&refs_to_delete, 0);
 	return ret;
 }
 
diff --git a/refs.h b/refs.h
index 2bc3556..69f88ef 100644
--- a/refs.h
+++ b/refs.h
@@ -163,8 +163,14 @@ extern void rollback_packed_refs(void);
  */
 int pack_refs(unsigned int flags);
 
-extern int repack_without_refs(const char **refnames, int n,
-			       struct strbuf *err);
+/*
+ * Repacks the refs pack file excluding the refs given
+ * without: The refs to be excluded from the new refs pack file,
+ *          May be unsorted
+ * err: String buffer, which will be used for reporting errors,
+ *      Must not be NULL
+ */
+extern int repack_without_refs(struct string_list *without, struct strbuf *err);
 
 extern int ref_exists(const char *);
 
-- 
2.2.0.rc2.5.gf7b9fb2

Re: [PATCH] refs.c: use a stringlist for repack_without_refs

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> This patch doesn't intend any functional changes. It is just
> a refactoring, which replaces a char** array by a stringlist
> in the function repack_without_refs.
> This is easier to read and maintain as it delivers the same
> functionality with less lines of code less pointers.
>
> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
> Signed-off-by: Stefan Beller <sbeller@google.com>
>
> -

Have three of them, not just one, here. (no need to resend to fix
only this).  Or...

>
> This patch was heavily inspired by a part of the ref-transactions-rename
> series[1], but people tend to dislike large series and this part is
> relatively easy to take out and unrelated, so I'll send it as a single
> patch.
>
> [1] https://www.mail-archive.com/git@vger.kernel.org/msg60604.html
>
> ---

... next time, write the comments here, where Git already gives you
three dashes.

Also mention what you updated and why relative to your earlier round
here, if not covered in the log message already.

For example, renaming of delete_refs_list (in v1) to delete_refs
(this version) is a sensible change because readers know it is a
list from its type being string_list already, but that change is new
relative to the codebase, so it could go to the log message ("Having
array delete_refs[] and string_list delete_refs_list is redundant;
drop the array and give the string_list variable the shorter name",
or something like that) if you wanted to.

> @@ -1316,8 +1311,8 @@ static int prune_remote(const char *remote, int dry_run)
>  {
>  	int result = 0, i;
>  	struct ref_states states;
> -	struct string_list delete_refs_list = STRING_LIST_INIT_NODUP;
> -	const char **delete_refs;
> +	struct string_list delete_refs = STRING_LIST_INIT_NODUP;
> +	struct string_list_item *ref;
>  	const char *dangling_msg = dry_run
>  		? _(" %s will become dangling!")
>  		: _(" %s has become dangling!");
> @@ -1325,6 +1320,9 @@ static int prune_remote(const char *remote, int dry_run)
>  	memset(&states, 0, sizeof(states));
>  	get_remote_ref_states(remote, &states, GET_REF_STATES);
>  
> +	for_each_string_list_item(ref, &delete_refs)
> +		string_list_append(&delete_refs, ref->string);

What are you trying to do here?

Initialise delete_refs to an empty string list, and then iterate
over its elements and append them into the same string list???

It looks like a "currently noop, waiting for somebody to throw an
item to the list before this code, at which time it turns into an
infinite memory eater".

Curious...

[PATCH] refs.c: use a stringlist for repack_without_refs

[Stefan Beller]

From: Ronnie Sahlberg <sahlberg@google.com>

This patch doesn't intend any functional changes. It is just
a refactoring, which replaces a char** array by a stringlist
in the function repack_without_refs.
This is easier to read and maintain as it delivers the same
functionality with less lines of code and less pointers.

[sb: ported this patch from a larger patch series to the master branch,
added documentary comments in refs.h]

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---

On Wed, Nov 19, 2014 at 10:00 AM, Junio C Hamano <gitster@pobox.com> wrote:
> +     for_each_string_list_item(ref, &delete_refs)
> +             string_list_append(&delete_refs, ref->string);
> What are you trying to do here?

I messed up this patch completely yesterday in the evening.
Essentially the inter-patch diff are all the nits by Jonathan.
So here is my attempt on sending a more maintainer friendly patch.

Changes to version 1:
 * removed the double blank line
 * rename delete_refs_list to delete_refs
 * add back comments dropped by accident
 * use STRING_LIST_INIT_NODUP instead of the _DUP version in ref_transaction_commit
 * add documentary comments on the repack_without_refs function
 * user string_list_append instead of string_list_insert as it follows the previous
   behavior more closely.
 * put back the early exit of the loop in repack_without_refs
   
Changes to version 2:
 * fixed commit message (comments after the three dashes)
 * fixed the curiosity Junio pointed out as it was just wrong code.
   Now it actually builds a list of all states.stale.items[i].util items.


 builtin/remote.c | 31 +++++++++++--------------------
 refs.c           | 40 +++++++++++++++++++++-------------------
 refs.h           | 10 ++++++++--
 3 files changed, 40 insertions(+), 41 deletions(-)

diff --git a/builtin/remote.c b/builtin/remote.c
index 7f28f92..0d89aba 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -750,16 +750,11 @@ static int mv(int argc, const char **argv)
 static int remove_branches(struct string_list *branches)
 {
 	struct strbuf err = STRBUF_INIT;
-	const char **branch_names;
 	int i, result = 0;
 
-	branch_names = xmalloc(branches->nr * sizeof(*branch_names));
-	for (i = 0; i < branches->nr; i++)
-		branch_names[i] = branches->items[i].string;
-	if (repack_without_refs(branch_names, branches->nr, &err))
+	if (repack_without_refs(branches, &err))
 		result |= error("%s", err.buf);
 	strbuf_release(&err);
-	free(branch_names);
 
 	for (i = 0; i < branches->nr; i++) {
 		struct string_list_item *item = branches->items + i;
@@ -1316,8 +1311,8 @@ static int prune_remote(const char *remote, int dry_run)
 {
 	int result = 0, i;
 	struct ref_states states;
-	struct string_list delete_refs_list = STRING_LIST_INIT_NODUP;
-	const char **delete_refs;
+	struct string_list delete_refs = STRING_LIST_INIT_NODUP;
+	struct string_list_item *ref;
 	const char *dangling_msg = dry_run
 		? _(" %s will become dangling!")
 		: _(" %s has become dangling!");
@@ -1325,6 +1320,9 @@ static int prune_remote(const char *remote, int dry_run)
 	memset(&states, 0, sizeof(states));
 	get_remote_ref_states(remote, &states, GET_REF_STATES);
 
+	for (i = 0; i < states.stale.nr; i++)
+		string_list_append(&delete_refs, states.stale.items[i].util);
+
 	if (states.stale.nr) {
 		printf_ln(_("Pruning %s"), remote);
 		printf_ln(_("URL: %s"),
@@ -1332,23 +1330,16 @@ static int prune_remote(const char *remote, int dry_run)
 		       ? states.remote->url[0]
 		       : _("(no URL)"));
 
-		delete_refs = xmalloc(states.stale.nr * sizeof(*delete_refs));
-		for (i = 0; i < states.stale.nr; i++)
-			delete_refs[i] = states.stale.items[i].util;
 		if (!dry_run) {
 			struct strbuf err = STRBUF_INIT;
-			if (repack_without_refs(delete_refs, states.stale.nr,
-						&err))
+			if (repack_without_refs(&delete_refs, &err))
 				result |= error("%s", err.buf);
 			strbuf_release(&err);
 		}
-		free(delete_refs);
 	}
 
-	for (i = 0; i < states.stale.nr; i++) {
-		const char *refname = states.stale.items[i].util;
-
-		string_list_insert(&delete_refs_list, refname);
+	for_each_string_list_item(ref, &delete_refs) {
+		const char *refname = ref->string;
 
 		if (!dry_run)
 			result |= delete_ref(refname, NULL, 0);
@@ -1361,8 +1352,8 @@ static int prune_remote(const char *remote, int dry_run)
 			       abbrev_ref(refname, "refs/remotes/"));
 	}
 
-	warn_dangling_symrefs(stdout, dangling_msg, &delete_refs_list);
-	string_list_clear(&delete_refs_list, 0);
+	warn_dangling_symrefs(stdout, dangling_msg, &delete_refs);
+	string_list_clear(&delete_refs, 0);
 
 	free_remote_ref_states(&states);
 	return result;
diff --git a/refs.c b/refs.c
index 5ff457e..2f6e08b 100644
--- a/refs.c
+++ b/refs.c
@@ -2639,23 +2639,26 @@ static int curate_packed_ref_fn(struct ref_entry *entry, void *cb_data)
 	return 0;
 }
 
-int repack_without_refs(const char **refnames, int n, struct strbuf *err)
+int repack_without_refs(struct string_list *without, struct strbuf *err)
 {
 	struct ref_dir *packed;
 	struct string_list refs_to_delete = STRING_LIST_INIT_DUP;
 	struct string_list_item *ref_to_delete;
-	int i, ret, removed = 0;
+	int ret, needs_repacking = 0, removed = 0;
 
 	assert(err);
 
 	/* Look for a packed ref */
-	for (i = 0; i < n; i++)
-		if (get_packed_ref(refnames[i]))
+	for_each_string_list_item(ref_to_delete, without) {
+		if (get_packed_ref(ref_to_delete->string)) {
+			needs_repacking = 1;
 			break;
+		}
+	}
 
-	/* Avoid locking if we have nothing to do */
-	if (i == n)
-		return 0; /* no refname exists in packed refs */
+	/* No refname exists in packed refs */
+	if (!needs_repacking)
+		return 0;
 
 	if (lock_packed_refs(0)) {
 		unable_to_lock_message(git_path("packed-refs"), errno, err);
@@ -2664,8 +2667,8 @@ int repack_without_refs(const char **refnames, int n, struct strbuf *err)
 	packed = get_packed_refs(&ref_cache);
 
 	/* Remove refnames from the cache */
-	for (i = 0; i < n; i++)
-		if (remove_entry(packed, refnames[i]) != -1)
+	for_each_string_list_item(ref_to_delete, without)
+		if (remove_entry(packed, ref_to_delete->string) != -1)
 			removed = 1;
 	if (!removed) {
 		/*
@@ -3738,10 +3741,11 @@ static int ref_update_reject_duplicates(struct ref_update **updates, int n,
 int ref_transaction_commit(struct ref_transaction *transaction,
 			   struct strbuf *err)
 {
-	int ret = 0, delnum = 0, i;
-	const char **delnames;
+	int ret = 0, i;
 	int n = transaction->nr;
 	struct ref_update **updates = transaction->updates;
+	struct string_list refs_to_delete = STRING_LIST_INIT_NODUP;
+	struct string_list_item *ref_to_delete;
 
 	assert(err);
 
@@ -3753,9 +3757,6 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 		return 0;
 	}
 
-	/* Allocate work space */
-	delnames = xmalloc(sizeof(*delnames) * n);
-
 	/* Copy, sort, and reject duplicate refs */
 	qsort(updates, n, sizeof(*updates), ref_update_compare);
 	if (ref_update_reject_duplicates(updates, n, err)) {
@@ -3815,16 +3816,17 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 			}
 
 			if (!(update->flags & REF_ISPRUNING))
-				delnames[delnum++] = update->lock->ref_name;
+				string_list_append(&refs_to_delete,
+						   update->lock->ref_name);
 		}
 	}
 
-	if (repack_without_refs(delnames, delnum, err)) {
+	if (repack_without_refs(&refs_to_delete, err)) {
 		ret = TRANSACTION_GENERIC_ERROR;
 		goto cleanup;
 	}
-	for (i = 0; i < delnum; i++)
-		unlink_or_warn(git_path("logs/%s", delnames[i]));
+	for_each_string_list_item(ref_to_delete, &refs_to_delete)
+		unlink_or_warn(git_path("logs/%s", ref_to_delete->string));
 	clear_loose_ref_cache(&ref_cache);
 
 cleanup:
@@ -3833,7 +3835,7 @@ cleanup:
 	for (i = 0; i < n; i++)
 		if (updates[i]->lock)
 			unlock_ref(updates[i]->lock);
-	free(delnames);
+	string_list_clear(&refs_to_delete, 0);
 	return ret;
 }
 
diff --git a/refs.h b/refs.h
index 2bc3556..69f88ef 100644
--- a/refs.h
+++ b/refs.h
@@ -163,8 +163,14 @@ extern void rollback_packed_refs(void);
  */
 int pack_refs(unsigned int flags);
 
-extern int repack_without_refs(const char **refnames, int n,
-			       struct strbuf *err);
+/*
+ * Repacks the refs pack file excluding the refs given
+ * without: The refs to be excluded from the new refs pack file,
+ *          May be unsorted
+ * err: String buffer, which will be used for reporting errors,
+ *      Must not be NULL
+ */
+extern int repack_without_refs(struct string_list *without, struct strbuf *err);
 
 extern int ref_exists(const char *);
 
-- 
2.2.0.rc2.13.g0786cdb

Re: [PATCH] refs.c: use a stringlist for repack_without_refs

[Jonathan Nieder]

Stefan Beller wrote:

> This patch doesn't intend any functional changes.

Yay. :)

> a refactoring, which replaces a char** array by a stringlist
> in the function repack_without_refs.
> This is easier to read and maintain as it delivers the same
> functionality with less lines of code and less pointers.

Please wrap to a consistent width and add a blank line between
paragraphs.  So, either:

	... repack_without_refs.  This is easier to read and ...

or:

	... repack_without_refs.

	This is easier to read and ...

[...]
> +++ b/builtin/remote.c
> @@ -750,16 +750,11 @@ static int mv(int argc, const char **argv)
[...]
> @@ -1325,6 +1320,9 @@ static int prune_remote(const char *remote, int dry_run)
>  	memset(&states, 0, sizeof(states));
>  	get_remote_ref_states(remote, &states, GET_REF_STATES);
>  
> +	for (i = 0; i < states.stale.nr; i++)
> +		string_list_append(&delete_refs, states.stale.items[i].util);

warn_dangling_symref requires a sorted list.  Possible fixes:

 (a) switch to string_list_insert, or
 (b) [nicer] call sort_string_list before the warn_dangling_symrefs
     call.

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

This comment was helpful --- it's sad to lose it (but if you feel
strongly about it then I don't mind).

> -	if (i == n)
> -		return 0; /* no refname exists in packed refs */
> +	/* No refname exists in packed refs */
> +	if (!needs_repacking)
> +		return 0;

I kind of liked the 'i == n' test that avoided needing a new auxiliary
variable.  This is fine and probably a little clearer, though.

[...]
> --- a/refs.h
> +++ b/refs.h
> @@ -163,8 +163,14 @@ extern void rollback_packed_refs(void);
>   */
>  int pack_refs(unsigned int flags);
>  
> -extern int repack_without_refs(const char **refnames, int n,
> -			       struct strbuf *err);
> +/*
> + * Repacks the refs pack file excluding the refs given
> + * without: The refs to be excluded from the new refs pack file,
> + *          May be unsorted
> + * err: String buffer, which will be used for reporting errors,
> + *      Must not be NULL
> + */
> +extern int repack_without_refs(struct string_list *without, struct strbuf *err);

(nit) Other comments in this file use the imperative mood to describe
what a function does, so it would be a little clearer to do that here,
too ("Repack the ..." instead of "Repacks the ...").

It might be just me, but I find this formatted comment with everything
jammed together hard to read.  I'd prefer a simple paragraph, like:

	/*
	 * Remove the refs listed in 'without' from the packed-refs file.
	 * On error, packed-refs will be unchanged, the return value is
	 * nonzero, and a message about the error is written to the 'err'
	 * strbuf.
	 */

Thanks,
Jonathan

[PATCH] refs.c: use a stringlist for repack_without_refs

[Stefan Beller]

From: Ronnie Sahlberg <sahlberg@google.com>

This patch doesn't intend any functional changes. It is just
a refactoring, which replaces a char** array by a stringlist
in the function repack_without_refs.
This is easier to read and maintain as it delivers the same
functionality with less lines of code and less pointers.

[sb: ported this patch from a larger patch series to the master branch,
added documentary comments in refs.h]

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---

version 3 includes all nits by Jonathan.

Changes to version 1:
 * removed the double blank line
 * rename delete_refs_list to delete_refs
 * add back comments dropped by accident
 * use STRING_LIST_INIT_NODUP instead of the _DUP version in ref_transaction_commit
 * add documentary comments on the repack_without_refs function
 * user string_list_append instead of string_list_insert as it follows the previous
   behavior more closely.
 * put back the early exit of the loop in repack_without_refs
   
Changes to version 2:
 * fixed commit message (comments after the three dashes)
 * fixed the curiosity Junio pointed out as it was just wrong code.
   Now it actually builds a list of all states.stale.items[i].util items.
   
Changes in version 3:

 * reword commit message
 * sort delete_refs before passing it to warn_dangling_symrefs
 * change the comments (get back the one jrn complained about) 
   in repack_without_refs
 * use the suggestion of jonathan for documenting repack_without_refs in the
   header. Add a note about the arguments.

---
 builtin/remote.c | 32 ++++++++++++--------------------
 refs.c           | 38 ++++++++++++++++++++------------------
 refs.h           | 10 ++++++++--
 3 files changed, 40 insertions(+), 40 deletions(-)

diff --git a/builtin/remote.c b/builtin/remote.c
index 7f28f92..b37ed3d 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -750,16 +750,11 @@ static int mv(int argc, const char **argv)
 static int remove_branches(struct string_list *branches)
 {
 	struct strbuf err = STRBUF_INIT;
-	const char **branch_names;
 	int i, result = 0;
 
-	branch_names = xmalloc(branches->nr * sizeof(*branch_names));
-	for (i = 0; i < branches->nr; i++)
-		branch_names[i] = branches->items[i].string;
-	if (repack_without_refs(branch_names, branches->nr, &err))
+	if (repack_without_refs(branches, &err))
 		result |= error("%s", err.buf);
 	strbuf_release(&err);
-	free(branch_names);
 
 	for (i = 0; i < branches->nr; i++) {
 		struct string_list_item *item = branches->items + i;
@@ -1316,8 +1311,8 @@ static int prune_remote(const char *remote, int dry_run)
 {
 	int result = 0, i;
 	struct ref_states states;
-	struct string_list delete_refs_list = STRING_LIST_INIT_NODUP;
-	const char **delete_refs;
+	struct string_list delete_refs = STRING_LIST_INIT_NODUP;
+	struct string_list_item *ref;
 	const char *dangling_msg = dry_run
 		? _(" %s will become dangling!")
 		: _(" %s has become dangling!");
@@ -1325,6 +1320,9 @@ static int prune_remote(const char *remote, int dry_run)
 	memset(&states, 0, sizeof(states));
 	get_remote_ref_states(remote, &states, GET_REF_STATES);
 
+	for (i = 0; i < states.stale.nr; i++)
+		string_list_append(&delete_refs, states.stale.items[i].util);
+
 	if (states.stale.nr) {
 		printf_ln(_("Pruning %s"), remote);
 		printf_ln(_("URL: %s"),
@@ -1332,23 +1330,16 @@ static int prune_remote(const char *remote, int dry_run)
 		       ? states.remote->url[0]
 		       : _("(no URL)"));
 
-		delete_refs = xmalloc(states.stale.nr * sizeof(*delete_refs));
-		for (i = 0; i < states.stale.nr; i++)
-			delete_refs[i] = states.stale.items[i].util;
 		if (!dry_run) {
 			struct strbuf err = STRBUF_INIT;
-			if (repack_without_refs(delete_refs, states.stale.nr,
-						&err))
+			if (repack_without_refs(&delete_refs, &err))
 				result |= error("%s", err.buf);
 			strbuf_release(&err);
 		}
-		free(delete_refs);
 	}
 
-	for (i = 0; i < states.stale.nr; i++) {
-		const char *refname = states.stale.items[i].util;
-
-		string_list_insert(&delete_refs_list, refname);
+	for_each_string_list_item(ref, &delete_refs) {
+		const char *refname = ref->string;
 
 		if (!dry_run)
 			result |= delete_ref(refname, NULL, 0);
@@ -1361,8 +1352,9 @@ static int prune_remote(const char *remote, int dry_run)
 			       abbrev_ref(refname, "refs/remotes/"));
 	}
 
-	warn_dangling_symrefs(stdout, dangling_msg, &delete_refs_list);
-	string_list_clear(&delete_refs_list, 0);
+	sort_string_list(&delete_refs);
+	warn_dangling_symrefs(stdout, dangling_msg, &delete_refs);
+	string_list_clear(&delete_refs, 0);
 
 	free_remote_ref_states(&states);
 	return result;
diff --git a/refs.c b/refs.c
index 5ff457e..ebcd90f 100644
--- a/refs.c
+++ b/refs.c
@@ -2639,23 +2639,26 @@ static int curate_packed_ref_fn(struct ref_entry *entry, void *cb_data)
 	return 0;
 }
 
-int repack_without_refs(const char **refnames, int n, struct strbuf *err)
+int repack_without_refs(struct string_list *without, struct strbuf *err)
 {
 	struct ref_dir *packed;
 	struct string_list refs_to_delete = STRING_LIST_INIT_DUP;
 	struct string_list_item *ref_to_delete;
-	int i, ret, removed = 0;
+	int ret, needs_repacking = 0, removed = 0;
 
 	assert(err);
 
 	/* Look for a packed ref */
-	for (i = 0; i < n; i++)
-		if (get_packed_ref(refnames[i]))
+	for_each_string_list_item(ref_to_delete, without) {
+		if (get_packed_ref(ref_to_delete->string)) {
+			needs_repacking = 1;
 			break;
+		}
+	}
 
 	/* Avoid locking if we have nothing to do */
-	if (i == n)
-		return 0; /* no refname exists in packed refs */
+	if (!needs_repacking)
+		return 0;
 
 	if (lock_packed_refs(0)) {
 		unable_to_lock_message(git_path("packed-refs"), errno, err);
@@ -2664,8 +2667,8 @@ int repack_without_refs(const char **refnames, int n, struct strbuf *err)
 	packed = get_packed_refs(&ref_cache);
 
 	/* Remove refnames from the cache */
-	for (i = 0; i < n; i++)
-		if (remove_entry(packed, refnames[i]) != -1)
+	for_each_string_list_item(ref_to_delete, without)
+		if (remove_entry(packed, ref_to_delete->string) != -1)
 			removed = 1;
 	if (!removed) {
 		/*
@@ -3738,10 +3741,11 @@ static int ref_update_reject_duplicates(struct ref_update **updates, int n,
 int ref_transaction_commit(struct ref_transaction *transaction,
 			   struct strbuf *err)
 {
-	int ret = 0, delnum = 0, i;
-	const char **delnames;
+	int ret = 0, i;
 	int n = transaction->nr;
 	struct ref_update **updates = transaction->updates;
+	struct string_list refs_to_delete = STRING_LIST_INIT_NODUP;
+	struct string_list_item *ref_to_delete;
 
 	assert(err);
 
@@ -3753,9 +3757,6 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 		return 0;
 	}
 
-	/* Allocate work space */
-	delnames = xmalloc(sizeof(*delnames) * n);
-
 	/* Copy, sort, and reject duplicate refs */
 	qsort(updates, n, sizeof(*updates), ref_update_compare);
 	if (ref_update_reject_duplicates(updates, n, err)) {
@@ -3815,16 +3816,17 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 			}
 
 			if (!(update->flags & REF_ISPRUNING))
-				delnames[delnum++] = update->lock->ref_name;
+				string_list_append(&refs_to_delete,
+						   update->lock->ref_name);
 		}
 	}
 
-	if (repack_without_refs(delnames, delnum, err)) {
+	if (repack_without_refs(&refs_to_delete, err)) {
 		ret = TRANSACTION_GENERIC_ERROR;
 		goto cleanup;
 	}
-	for (i = 0; i < delnum; i++)
-		unlink_or_warn(git_path("logs/%s", delnames[i]));
+	for_each_string_list_item(ref_to_delete, &refs_to_delete)
+		unlink_or_warn(git_path("logs/%s", ref_to_delete->string));
 	clear_loose_ref_cache(&ref_cache);
 
 cleanup:
@@ -3833,7 +3835,7 @@ cleanup:
 	for (i = 0; i < n; i++)
 		if (updates[i]->lock)
 			unlock_ref(updates[i]->lock);
-	free(delnames);
+	string_list_clear(&refs_to_delete, 0);
 	return ret;
 }
 
diff --git a/refs.h b/refs.h
index 2bc3556..69f88ef 100644
--- a/refs.h
+++ b/refs.h
@@ -163,8 +163,14 @@ extern void rollback_packed_refs(void);
  */
 int pack_refs(unsigned int flags);
 
-extern int repack_without_refs(const char **refnames, int n,
-			       struct strbuf *err);
+/*
+ * Repacks the refs pack file excluding the refs given
+ * without: The refs to be excluded from the new refs pack file,
+ *          May be unsorted
+ * err: String buffer, which will be used for reporting errors,
+ *      Must not be NULL
+ */
+extern int repack_without_refs(struct string_list *without, struct strbuf *err);
 
 extern int ref_exists(const char *);
 
-- 
2.2.0.rc2.13.g0786cdb

[PATCH v4] refs.c: use a stringlist for repack_without_refs

[Stefan Beller]

From: Ronnie Sahlberg <sahlberg@google.com>

This patch doesn't intend any functional changes. It is just
a refactoring, which replaces a char** array by a stringlist
in the function repack_without_refs.
This is easier to read and maintain as it delivers the same
functionality with less lines of code and less pointers.

[sb: ported this patch from a larger patch series to the master branch,
added documentary comments in refs.h]

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---

Changes to version 1:
 * removed the double blank line
 * rename delete_refs_list to delete_refs
 * add back comments dropped by accident
 * use STRING_LIST_INIT_NODUP instead of the _DUP version in ref_transaction_commit
 * add documentary comments on the repack_without_refs function
 * user string_list_append instead of string_list_insert as it follows the previous
   behavior more closely.
 * put back the early exit of the loop in repack_without_refs
   
Changes to version 2:
 * fixed commit message (comments after the three dashes)
 * fixed the curiosity Junio pointed out as it was just wrong code.
   Now it actually builds a list of all states.stale.items[i].util items.
   
Changes in version 3:

 * reword commit message
 * sort delete_refs before passing it to warn_dangling_symrefs
 * change the comments (get back the one jrn complained about) 
   in repack_without_refs
 * use the suggestion of jonathan for documenting repack_without_refs in the
   header. Add a note about the arguments.

Changes in version 4:
 * I lied, when saying I had all the nits from Jonathan. 
   I messed up the documentation in the header.
   This includes the documentary comment in the header.
   
---
 builtin/remote.c | 32 ++++++++++++--------------------
 refs.c           | 38 ++++++++++++++++++++------------------
 refs.h           | 11 +++++++++--
 3 files changed, 41 insertions(+), 40 deletions(-)

diff --git a/builtin/remote.c b/builtin/remote.c
index 7f28f92..b37ed3d 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -750,16 +750,11 @@ static int mv(int argc, const char **argv)
 static int remove_branches(struct string_list *branches)
 {
 	struct strbuf err = STRBUF_INIT;
-	const char **branch_names;
 	int i, result = 0;
 
-	branch_names = xmalloc(branches->nr * sizeof(*branch_names));
-	for (i = 0; i < branches->nr; i++)
-		branch_names[i] = branches->items[i].string;
-	if (repack_without_refs(branch_names, branches->nr, &err))
+	if (repack_without_refs(branches, &err))
 		result |= error("%s", err.buf);
 	strbuf_release(&err);
-	free(branch_names);
 
 	for (i = 0; i < branches->nr; i++) {
 		struct string_list_item *item = branches->items + i;
@@ -1316,8 +1311,8 @@ static int prune_remote(const char *remote, int dry_run)
 {
 	int result = 0, i;
 	struct ref_states states;
-	struct string_list delete_refs_list = STRING_LIST_INIT_NODUP;
-	const char **delete_refs;
+	struct string_list delete_refs = STRING_LIST_INIT_NODUP;
+	struct string_list_item *ref;
 	const char *dangling_msg = dry_run
 		? _(" %s will become dangling!")
 		: _(" %s has become dangling!");
@@ -1325,6 +1320,9 @@ static int prune_remote(const char *remote, int dry_run)
 	memset(&states, 0, sizeof(states));
 	get_remote_ref_states(remote, &states, GET_REF_STATES);
 
+	for (i = 0; i < states.stale.nr; i++)
+		string_list_append(&delete_refs, states.stale.items[i].util);
+
 	if (states.stale.nr) {
 		printf_ln(_("Pruning %s"), remote);
 		printf_ln(_("URL: %s"),
@@ -1332,23 +1330,16 @@ static int prune_remote(const char *remote, int dry_run)
 		       ? states.remote->url[0]
 		       : _("(no URL)"));
 
-		delete_refs = xmalloc(states.stale.nr * sizeof(*delete_refs));
-		for (i = 0; i < states.stale.nr; i++)
-			delete_refs[i] = states.stale.items[i].util;
 		if (!dry_run) {
 			struct strbuf err = STRBUF_INIT;
-			if (repack_without_refs(delete_refs, states.stale.nr,
-						&err))
+			if (repack_without_refs(&delete_refs, &err))
 				result |= error("%s", err.buf);
 			strbuf_release(&err);
 		}
-		free(delete_refs);
 	}
 
-	for (i = 0; i < states.stale.nr; i++) {
-		const char *refname = states.stale.items[i].util;
-
-		string_list_insert(&delete_refs_list, refname);
+	for_each_string_list_item(ref, &delete_refs) {
+		const char *refname = ref->string;
 
 		if (!dry_run)
 			result |= delete_ref(refname, NULL, 0);
@@ -1361,8 +1352,9 @@ static int prune_remote(const char *remote, int dry_run)
 			       abbrev_ref(refname, "refs/remotes/"));
 	}
 
-	warn_dangling_symrefs(stdout, dangling_msg, &delete_refs_list);
-	string_list_clear(&delete_refs_list, 0);
+	sort_string_list(&delete_refs);
+	warn_dangling_symrefs(stdout, dangling_msg, &delete_refs);
+	string_list_clear(&delete_refs, 0);
 
 	free_remote_ref_states(&states);
 	return result;
diff --git a/refs.c b/refs.c
index 5ff457e..ebcd90f 100644
--- a/refs.c
+++ b/refs.c
@@ -2639,23 +2639,26 @@ static int curate_packed_ref_fn(struct ref_entry *entry, void *cb_data)
 	return 0;
 }
 
-int repack_without_refs(const char **refnames, int n, struct strbuf *err)
+int repack_without_refs(struct string_list *without, struct strbuf *err)
 {
 	struct ref_dir *packed;
 	struct string_list refs_to_delete = STRING_LIST_INIT_DUP;
 	struct string_list_item *ref_to_delete;
-	int i, ret, removed = 0;
+	int ret, needs_repacking = 0, removed = 0;
 
 	assert(err);
 
 	/* Look for a packed ref */
-	for (i = 0; i < n; i++)
-		if (get_packed_ref(refnames[i]))
+	for_each_string_list_item(ref_to_delete, without) {
+		if (get_packed_ref(ref_to_delete->string)) {
+			needs_repacking = 1;
 			break;
+		}
+	}
 
 	/* Avoid locking if we have nothing to do */
-	if (i == n)
-		return 0; /* no refname exists in packed refs */
+	if (!needs_repacking)
+		return 0;
 
 	if (lock_packed_refs(0)) {
 		unable_to_lock_message(git_path("packed-refs"), errno, err);
@@ -2664,8 +2667,8 @@ int repack_without_refs(const char **refnames, int n, struct strbuf *err)
 	packed = get_packed_refs(&ref_cache);
 
 	/* Remove refnames from the cache */
-	for (i = 0; i < n; i++)
-		if (remove_entry(packed, refnames[i]) != -1)
+	for_each_string_list_item(ref_to_delete, without)
+		if (remove_entry(packed, ref_to_delete->string) != -1)
 			removed = 1;
 	if (!removed) {
 		/*
@@ -3738,10 +3741,11 @@ static int ref_update_reject_duplicates(struct ref_update **updates, int n,
 int ref_transaction_commit(struct ref_transaction *transaction,
 			   struct strbuf *err)
 {
-	int ret = 0, delnum = 0, i;
-	const char **delnames;
+	int ret = 0, i;
 	int n = transaction->nr;
 	struct ref_update **updates = transaction->updates;
+	struct string_list refs_to_delete = STRING_LIST_INIT_NODUP;
+	struct string_list_item *ref_to_delete;
 
 	assert(err);
 
@@ -3753,9 +3757,6 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 		return 0;
 	}
 
-	/* Allocate work space */
-	delnames = xmalloc(sizeof(*delnames) * n);
-
 	/* Copy, sort, and reject duplicate refs */
 	qsort(updates, n, sizeof(*updates), ref_update_compare);
 	if (ref_update_reject_duplicates(updates, n, err)) {
@@ -3815,16 +3816,17 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 			}
 
 			if (!(update->flags & REF_ISPRUNING))
-				delnames[delnum++] = update->lock->ref_name;
+				string_list_append(&refs_to_delete,
+						   update->lock->ref_name);
 		}
 	}
 
-	if (repack_without_refs(delnames, delnum, err)) {
+	if (repack_without_refs(&refs_to_delete, err)) {
 		ret = TRANSACTION_GENERIC_ERROR;
 		goto cleanup;
 	}
-	for (i = 0; i < delnum; i++)
-		unlink_or_warn(git_path("logs/%s", delnames[i]));
+	for_each_string_list_item(ref_to_delete, &refs_to_delete)
+		unlink_or_warn(git_path("logs/%s", ref_to_delete->string));
 	clear_loose_ref_cache(&ref_cache);
 
 cleanup:
@@ -3833,7 +3835,7 @@ cleanup:
 	for (i = 0; i < n; i++)
 		if (updates[i]->lock)
 			unlock_ref(updates[i]->lock);
-	free(delnames);
+	string_list_clear(&refs_to_delete, 0);
 	return ret;
 }
 
diff --git a/refs.h b/refs.h
index 2bc3556..5a0cd21 100644
--- a/refs.h
+++ b/refs.h
@@ -163,8 +163,15 @@ extern void rollback_packed_refs(void);
  */
 int pack_refs(unsigned int flags);
 
-extern int repack_without_refs(const char **refnames, int n,
-			       struct strbuf *err);
+/*
+ * Remove the refs listed in 'without' from the packed-refs file.
+ * On error, packed-refs will be unchanged, the return value is
+ * nonzero, and a message about the error is written to the 'err'
+ * strbuf.
+ *
+ * The refs in 'without' may have any order, the err buffer must not be ommited.
+ */
+extern int repack_without_refs(struct string_list *without, struct strbuf *err);
 
 extern int ref_exists(const char *);
 
-- 
2.2.0.rc2.13.g0786cdb

Re: [PATCH v4] refs.c: use a stringlist for repack_without_refs

[Jonathan Nieder]

Stefan Beller wrote:

> From: Ronnie Sahlberg <sahlberg@google.com>
>
> This patch doesn't intend any functional changes. It is just
> a refactoring, which replaces a char** array by a stringlist
> in the function repack_without_refs.
> This is easier to read and maintain as it delivers the same
> functionality with less lines of code and less pointers.

Thanks for the quick turnaround.

Nit: please wrap to a consistent width and put a blank line between
paragraphs.

That is, the above should either say

	This patch doesn't intend any functional changes.  It is just
	a refactoring to replace a char** array with a string_list
	in the function repack_without_refs.  This is easier to read
	and maintain as it delivers the same functionality with less
	code and fewer pointers.

or

	This patch doesn't intend any functional changes.  It is just
	a refactoring to replace a char** array with a string_list
	in the function repack_without_refs.

	This is easier to read and maintain as it delivers the same
	functionality with less code and fewer pointers.

Although I'm not sure the main benefit is having fewer asterisks. ;-)

[...]
> +++ b/builtin/remote.c
[...]
> @@ -1361,8 +1352,9 @@ static int prune_remote(const char *remote, int dry_run)
>  			       abbrev_ref(refname, "refs/remotes/"));
>  	}
>  
> -	warn_dangling_symrefs(stdout, dangling_msg, &delete_refs_list);
> -	string_list_clear(&delete_refs_list, 0);
> +	sort_string_list(&delete_refs);
> +	warn_dangling_symrefs(stdout, dangling_msg, &delete_refs);
> +	string_list_clear(&delete_refs, 0);
>  
>  	free_remote_ref_states(&states);
>  	return result;

Micronit: it would be clearer (and easier to remember to free the list
in other code paths if this function gains more 'return' statements)
with the string_list_clear in the same block as other code that frees
resources (i.e., if the blank line moved one line up).

[...]
> --- a/refs.h
> +++ b/refs.h
> @@ -163,8 +163,15 @@ extern void rollback_packed_refs(void);
>   */
>  int pack_refs(unsigned int flags);
>  
> -extern int repack_without_refs(const char **refnames, int n,
> -			       struct strbuf *err);
> +/*
> + * Remove the refs listed in 'without' from the packed-refs file.
> + * On error, packed-refs will be unchanged, the return value is
> + * nonzero, and a message about the error is written to the 'err'
> + * strbuf.
> + *
> + * The refs in 'without' may have any order, the err buffer must not be ommited.

Nits:

s/ommited/omitted/

Comma splice.  Long line.

The function has to be able to write to 'err' on error, so I think the
comment doesn't have to mention that err must be non-NULL.  Any caller
that tries to pass NULL will get an assertion error quickly.

With or without the changes suggested above,
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Re: [PATCH v4] refs.c: use a stringlist for repack_without_refs

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> [...]
>> +++ b/builtin/remote.c
> [...]
>> @@ -1361,8 +1352,9 @@ static int prune_remote(const char *remote, int dry_run)
>>  			       abbrev_ref(refname, "refs/remotes/"));
>>  	}
>>  
>> -	warn_dangling_symrefs(stdout, dangling_msg, &delete_refs_list);
>> -	string_list_clear(&delete_refs_list, 0);
>> +	sort_string_list(&delete_refs);
>> +	warn_dangling_symrefs(stdout, dangling_msg, &delete_refs);
>> +	string_list_clear(&delete_refs, 0);
>>  
>>  	free_remote_ref_states(&states);
>>  	return result;
>
> Micronit: it would be clearer (and easier to remember to free the list
> in other code paths if this function gains more 'return' statements)
> with the string_list_clear in the same block as other code that frees
> resources (i.e., if the blank line moved one line up).

Thanks for a careful reading.  This kind of attention to detail
helps the longer term health of the codebase.

> The function has to be able to write to 'err' on error, so I think the
> comment doesn't have to mention that err must be non-NULL.  Any caller
> that tries to pass NULL will get an assertion error quickly.

That invites a bit of question, though.

An equally plausible alternative definition for set of API functions
that take strbuf *err is to pass it only when you care about the
explanation of the error (i.e. it is valid for "git cmd --quiet" to
pass NULL there) [*1*] (do we already have such a function?).  And
the comment may help clarifying which is which.  I however think we
shouldn't have mixtures (formatting into "strbuf *err" may be costly
when we know we are asked to fail silently, but an error path is not
usually performance sensitive).


[Footnote]

*1* With yet another one, a function may call error() on its own
when a NULL is passed to strbuf *err, but let's not go there.

[PATCH v5 1/1] refs.c: use a stringlist for repack_without_refs

[Stefan Beller]

From: Ronnie Sahlberg <sahlberg@google.com>

This patch doesn't intend any functional changes. It is just
a refactoring, which replaces a char** array by a stringlist
in the function repack_without_refs.

This is easier to read and maintain as it delivers the same
functionality with less lines of code and more lines of
documentation.

[sb: ported this patch from a larger patch series to the
master branch, added documentary comments in refs.h]

Change-Id: Id7eaa821331f2ab89df063e1e76c8485dbcc3aed
Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
---

Changes to version 1:
 * removed the double blank line
 * rename delete_refs_list to delete_refs
 * add back comments dropped by accident
 * use STRING_LIST_INIT_NODUP instead of the _DUP version in ref_transaction_commit
 * add documentary comments on the repack_without_refs function
 * user string_list_append instead of string_list_insert as it follows the previous
   behavior more closely.
 * put back the early exit of the loop in repack_without_refs
   
Changes to version 2:
 * fixed commit message (comments after the three dashes)
 * fixed the curiosity Junio pointed out as it was just wrong code.
   Now it actually builds a list of all states.stale.items[i].util items.
   
Changes in version 3:

 * reword commit message
 * sort delete_refs before passing it to warn_dangling_symrefs
 * change the comments (get back the one jrn complained about) 
   in repack_without_refs
 * use the suggestion of jonathan for documenting repack_without_refs in the
   header. Add a note about the arguments.

Changes in version 4:
 * I lied, when saying I had all the nits from Jonathan. 
   I messed up the documentation in the header.
   This includes the documentary comment in the header.

Changes in version 5:
 * Break lines as suggested by Jonathan, slightly rewording the commit message
 * have an empty line at another place in builtin/remote.c remove_branches to 
   tell cleanup parts apart from actual work.
 * fix typo, improve documentary comment in refs.c
 * add Jonathans reviewed by
 
 Junio, I'll address your proposed changes in a different patch. 
 If err is passed in as NULL, we'll just skip all the error string 
 formatting and return silent and fast.
   
 builtin/remote.c | 32 ++++++++++++--------------------
 refs.c           | 38 ++++++++++++++++++++------------------
 refs.h           | 12 ++++++++++--
 3 files changed, 42 insertions(+), 40 deletions(-)

diff --git a/builtin/remote.c b/builtin/remote.c
index 7f28f92..364350a 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -750,16 +750,11 @@ static int mv(int argc, const char **argv)
 static int remove_branches(struct string_list *branches)
 {
 	struct strbuf err = STRBUF_INIT;
-	const char **branch_names;
 	int i, result = 0;
 
-	branch_names = xmalloc(branches->nr * sizeof(*branch_names));
-	for (i = 0; i < branches->nr; i++)
-		branch_names[i] = branches->items[i].string;
-	if (repack_without_refs(branch_names, branches->nr, &err))
+	if (repack_without_refs(branches, &err))
 		result |= error("%s", err.buf);
 	strbuf_release(&err);
-	free(branch_names);
 
 	for (i = 0; i < branches->nr; i++) {
 		struct string_list_item *item = branches->items + i;
@@ -1316,8 +1311,8 @@ static int prune_remote(const char *remote, int dry_run)
 {
 	int result = 0, i;
 	struct ref_states states;
-	struct string_list delete_refs_list = STRING_LIST_INIT_NODUP;
-	const char **delete_refs;
+	struct string_list delete_refs = STRING_LIST_INIT_NODUP;
+	struct string_list_item *ref;
 	const char *dangling_msg = dry_run
 		? _(" %s will become dangling!")
 		: _(" %s has become dangling!");
@@ -1325,6 +1320,9 @@ static int prune_remote(const char *remote, int dry_run)
 	memset(&states, 0, sizeof(states));
 	get_remote_ref_states(remote, &states, GET_REF_STATES);
 
+	for (i = 0; i < states.stale.nr; i++)
+		string_list_append(&delete_refs, states.stale.items[i].util);
+
 	if (states.stale.nr) {
 		printf_ln(_("Pruning %s"), remote);
 		printf_ln(_("URL: %s"),
@@ -1332,23 +1330,16 @@ static int prune_remote(const char *remote, int dry_run)
 		       ? states.remote->url[0]
 		       : _("(no URL)"));
 
-		delete_refs = xmalloc(states.stale.nr * sizeof(*delete_refs));
-		for (i = 0; i < states.stale.nr; i++)
-			delete_refs[i] = states.stale.items[i].util;
 		if (!dry_run) {
 			struct strbuf err = STRBUF_INIT;
-			if (repack_without_refs(delete_refs, states.stale.nr,
-						&err))
+			if (repack_without_refs(&delete_refs, &err))
 				result |= error("%s", err.buf);
 			strbuf_release(&err);
 		}
-		free(delete_refs);
 	}
 
-	for (i = 0; i < states.stale.nr; i++) {
-		const char *refname = states.stale.items[i].util;
-
-		string_list_insert(&delete_refs_list, refname);
+	for_each_string_list_item(ref, &delete_refs) {
+		const char *refname = ref->string;
 
 		if (!dry_run)
 			result |= delete_ref(refname, NULL, 0);
@@ -1361,9 +1352,10 @@ static int prune_remote(const char *remote, int dry_run)
 			       abbrev_ref(refname, "refs/remotes/"));
 	}
 
-	warn_dangling_symrefs(stdout, dangling_msg, &delete_refs_list);
-	string_list_clear(&delete_refs_list, 0);
+	sort_string_list(&delete_refs);
+	warn_dangling_symrefs(stdout, dangling_msg, &delete_refs);
 
+	string_list_clear(&delete_refs, 0);
 	free_remote_ref_states(&states);
 	return result;
 }
diff --git a/refs.c b/refs.c
index 5ff457e..ebcd90f 100644
--- a/refs.c
+++ b/refs.c
@@ -2639,23 +2639,26 @@ static int curate_packed_ref_fn(struct ref_entry *entry, void *cb_data)
 	return 0;
 }
 
-int repack_without_refs(const char **refnames, int n, struct strbuf *err)
+int repack_without_refs(struct string_list *without, struct strbuf *err)
 {
 	struct ref_dir *packed;
 	struct string_list refs_to_delete = STRING_LIST_INIT_DUP;
 	struct string_list_item *ref_to_delete;
-	int i, ret, removed = 0;
+	int ret, needs_repacking = 0, removed = 0;
 
 	assert(err);
 
 	/* Look for a packed ref */
-	for (i = 0; i < n; i++)
-		if (get_packed_ref(refnames[i]))
+	for_each_string_list_item(ref_to_delete, without) {
+		if (get_packed_ref(ref_to_delete->string)) {
+			needs_repacking = 1;
 			break;
+		}
+	}
 
 	/* Avoid locking if we have nothing to do */
-	if (i == n)
-		return 0; /* no refname exists in packed refs */
+	if (!needs_repacking)
+		return 0;
 
 	if (lock_packed_refs(0)) {
 		unable_to_lock_message(git_path("packed-refs"), errno, err);
@@ -2664,8 +2667,8 @@ int repack_without_refs(const char **refnames, int n, struct strbuf *err)
 	packed = get_packed_refs(&ref_cache);
 
 	/* Remove refnames from the cache */
-	for (i = 0; i < n; i++)
-		if (remove_entry(packed, refnames[i]) != -1)
+	for_each_string_list_item(ref_to_delete, without)
+		if (remove_entry(packed, ref_to_delete->string) != -1)
 			removed = 1;
 	if (!removed) {
 		/*
@@ -3738,10 +3741,11 @@ static int ref_update_reject_duplicates(struct ref_update **updates, int n,
 int ref_transaction_commit(struct ref_transaction *transaction,
 			   struct strbuf *err)
 {
-	int ret = 0, delnum = 0, i;
-	const char **delnames;
+	int ret = 0, i;
 	int n = transaction->nr;
 	struct ref_update **updates = transaction->updates;
+	struct string_list refs_to_delete = STRING_LIST_INIT_NODUP;
+	struct string_list_item *ref_to_delete;
 
 	assert(err);
 
@@ -3753,9 +3757,6 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 		return 0;
 	}
 
-	/* Allocate work space */
-	delnames = xmalloc(sizeof(*delnames) * n);
-
 	/* Copy, sort, and reject duplicate refs */
 	qsort(updates, n, sizeof(*updates), ref_update_compare);
 	if (ref_update_reject_duplicates(updates, n, err)) {
@@ -3815,16 +3816,17 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 			}
 
 			if (!(update->flags & REF_ISPRUNING))
-				delnames[delnum++] = update->lock->ref_name;
+				string_list_append(&refs_to_delete,
+						   update->lock->ref_name);
 		}
 	}
 
-	if (repack_without_refs(delnames, delnum, err)) {
+	if (repack_without_refs(&refs_to_delete, err)) {
 		ret = TRANSACTION_GENERIC_ERROR;
 		goto cleanup;
 	}
-	for (i = 0; i < delnum; i++)
-		unlink_or_warn(git_path("logs/%s", delnames[i]));
+	for_each_string_list_item(ref_to_delete, &refs_to_delete)
+		unlink_or_warn(git_path("logs/%s", ref_to_delete->string));
 	clear_loose_ref_cache(&ref_cache);
 
 cleanup:
@@ -3833,7 +3835,7 @@ cleanup:
 	for (i = 0; i < n; i++)
 		if (updates[i]->lock)
 			unlock_ref(updates[i]->lock);
-	free(delnames);
+	string_list_clear(&refs_to_delete, 0);
 	return ret;
 }
 
diff --git a/refs.h b/refs.h
index 2bc3556..c7323ff 100644
--- a/refs.h
+++ b/refs.h
@@ -163,8 +163,16 @@ extern void rollback_packed_refs(void);
  */
 int pack_refs(unsigned int flags);
 
-extern int repack_without_refs(const char **refnames, int n,
-			       struct strbuf *err);
+/*
+ * Remove the refs listed in 'without' from the packed-refs file.
+ * On error, packed-refs will be unchanged, the return value is
+ * nonzero, and a message about the error is written to the 'err'
+ * strbuf.
+ *
+ * The refs in 'without' may have any order.
+ * The err buffer must not be omitted.
+ */
+extern int repack_without_refs(struct string_list *without, struct strbuf *err);
 
 extern int ref_exists(const char *);
 
-- 
2.2.0.rc2.23.gca0107e

[PATCH] refs.c: repack_without_refs may be called without error string buffer

[Stefan Beller]

If we don't pass in the error string buffer, we skip over all
parts dealing with preparing error messages.

Signed-off-by: Stefan Beller <sbeller@google.com>
---

This goes ontop of [PATCH v5] refs.c: use a stringlist for repack_without_refs
if that makes sense.

 refs.c | 8 ++++----
 refs.h | 1 -
 2 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/refs.c b/refs.c
index ebcd90f..3c85ea6 100644
--- a/refs.c
+++ b/refs.c
@@ -2646,8 +2646,6 @@ int repack_without_refs(struct string_list *without, struct strbuf *err)
 	struct string_list_item *ref_to_delete;
 	int ret, needs_repacking = 0, removed = 0;
 
-	assert(err);
-
 	/* Look for a packed ref */
 	for_each_string_list_item(ref_to_delete, without) {
 		if (get_packed_ref(ref_to_delete->string)) {
@@ -2661,7 +2659,9 @@ int repack_without_refs(struct string_list *without, struct strbuf *err)
 		return 0;
 
 	if (lock_packed_refs(0)) {
-		unable_to_lock_message(git_path("packed-refs"), errno, err);
+		if (err)
+			unable_to_lock_message(git_path("packed-refs"),
+					       errno, err);
 		return -1;
 	}
 	packed = get_packed_refs(&ref_cache);
@@ -2688,7 +2688,7 @@ int repack_without_refs(struct string_list *without, struct strbuf *err)
 
 	/* Write what remains */
 	ret = commit_packed_refs();
-	if (ret)
+	if (ret && err)
 		strbuf_addf(err, "unable to overwrite old ref-pack file: %s",
 			    strerror(errno));
 	return ret;
diff --git a/refs.h b/refs.h
index c7323ff..b71fb79 100644
--- a/refs.h
+++ b/refs.h
@@ -170,7 +170,6 @@ int pack_refs(unsigned int flags);
  * strbuf.
  *
  * The refs in 'without' may have any order.
- * The err buffer must not be omitted.
  */
 extern int repack_without_refs(struct string_list *without, struct strbuf *err);
 
-- 
2.2.0.rc2.23.gca0107e

Re: [PATCH] refs.c: repack_without_refs may be called without error string buffer

[Ronnie Sahlberg]

On Thu, Nov 20, 2014 at 10:10 AM, Stefan Beller <sbeller@google.com> wrote:
> If we don't pass in the error string buffer, we skip over all
> parts dealing with preparing error messages.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>
> This goes ontop of [PATCH v5] refs.c: use a stringlist for repack_without_refs
> if that makes sense.
>
>  refs.c | 8 ++++----
>  refs.h | 1 -
>  2 files changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/refs.c b/refs.c
> index ebcd90f..3c85ea6 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -2646,8 +2646,6 @@ int repack_without_refs(struct string_list *without, struct strbuf *err)
>         struct string_list_item *ref_to_delete;
>         int ret, needs_repacking = 0, removed = 0;
>
> -       assert(err);
> -
>         /* Look for a packed ref */
>         for_each_string_list_item(ref_to_delete, without) {
>                 if (get_packed_ref(ref_to_delete->string)) {
> @@ -2661,7 +2659,9 @@ int repack_without_refs(struct string_list *without, struct strbuf *err)
>                 return 0;
>
>         if (lock_packed_refs(0)) {
> -               unable_to_lock_message(git_path("packed-refs"), errno, err);
> +               if (err)
> +                       unable_to_lock_message(git_path("packed-refs"),
> +                                              errno, err);
>                 return -1;
>         }
>         packed = get_packed_refs(&ref_cache);
> @@ -2688,7 +2688,7 @@ int repack_without_refs(struct string_list *without, struct strbuf *err)
>
>         /* Write what remains */
>         ret = commit_packed_refs();
> -       if (ret)
> +       if (ret && err)
>                 strbuf_addf(err, "unable to overwrite old ref-pack file: %s",
>                             strerror(errno));
>         return ret;
> diff --git a/refs.h b/refs.h
> index c7323ff..b71fb79 100644
> --- a/refs.h
> +++ b/refs.h
> @@ -170,7 +170,6 @@ int pack_refs(unsigned int flags);
>   * strbuf.
>   *
>   * The refs in 'without' may have any order.
> - * The err buffer must not be omitted.
>   */
>  extern int repack_without_refs(struct string_list *without, struct strbuf *err);
>
> --
> 2.2.0.rc2.23.gca0107e
>

LGTM
Reviewed-by: Ronnie Sahlberg <sahlberg@google.com>

Nit:
While it does not hurt to allow passing NULL,  at some stage later
this function will become
private to refs.c and ONLY be called from within transaction_commit()
which will always
pass a non-NULL err argument.
At that stage we will not strictly need to allow err==NULL since all
callers are guaranteed to
always pass err!=NULL.

That said, having err being optional is probably a better API. Maybe
err should be made optional for all other functions that take
an err strbuf too so that the calling conventions become more consistent?

Re: [PATCH v5 1/1] refs.c: use a stringlist for repack_without_refs

[Jonathan Nieder]

Stefan Beller wrote:

> Change-Id: Id7eaa821331f2ab89df063e1e76c8485dbcc3aed

Change-id snuck in.

[...]
> --- a/refs.h
> +++ b/refs.h
> @@ -163,8 +163,16 @@ extern void rollback_packed_refs(void);
>   */
>  int pack_refs(unsigned int flags);
>  
> -extern int repack_without_refs(const char **refnames, int n,
> -			       struct strbuf *err);
> +/*
> + * Remove the refs listed in 'without' from the packed-refs file.
> + * On error, packed-refs will be unchanged, the return value is
> + * nonzero, and a message about the error is written to the 'err'
> + * strbuf.
> + *
> + * The refs in 'without' may have any order.

Tiny nit: this makes me wonder what the order represents --- how do
I pick which order for the refs in without to have?

I think the idea is just that 'without' doesn't have to be sorted (it's
a shame we don't have separate sorted string list and unsorted string
list types or a string_list_sorted() helper to catch bad callers early
to functions that care).  One way to say that would be

	Remove the refs listed in the unsorted string list 'without' from the
	packed-refs file.  On error, [...]

> + * The err buffer must not be omitted.

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

With the Change-Id dropped, and with or without the above comment nits
addressed,

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Re: [PATCH] refs.c: repack_without_refs may be called without error string buffer

[Jonathan Nieder]

Stefan Beller wrote:

> If we don't pass in the error string buffer, we skip over all
> parts dealing with preparing error messages.

Please no.

We tried this with the ref transaction code.  When someone wants
to silence the message, it is cheap enough to do

	struct strbuf ignore = STRBUF_INIT;

	if (thing_that_can_fail_in_an_ignorable_way(..., &ignore)) {
		... handle the failure ...
	}

The extra lines of code make it obvious that the error message is
being dropped, which is a very good thing.  The extra work to format a
message in the error case is not so bad and can be mitigated if the
error is a common normal case by passing a flag to not consider it an
error.

Silently losing good diagnostic messages when err == NULL would have
the opposite effect: when there isn't a spare strbuf to put errors in
around, it would be tempting for people coding in a hurry to just pass
NULL, and to readers it would look at first glance like "oh, an
optional paramter was not passed and we are getting the good default
behavior".

This is not a theoretical concern --- it actually happened.

My two cents,
Jonathan

Re: [PATCH] refs.c: repack_without_refs may be called without error string buffer

[Ronnie Sahlberg]

On Thu, Nov 20, 2014 at 10:35 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Stefan Beller wrote:
>
>> If we don't pass in the error string buffer, we skip over all
>> parts dealing with preparing error messages.
>
> Please no.
>
> We tried this with the ref transaction code.  When someone wants
> to silence the message, it is cheap enough to do
>
>         struct strbuf ignore = STRBUF_INIT;
>
>         if (thing_that_can_fail_in_an_ignorable_way(..., &ignore)) {
>                 ... handle the failure ...
>         }
>
> The extra lines of code make it obvious that the error message is
> being dropped, which is a very good thing.  The extra work to format a
> message in the error case is not so bad and can be mitigated if the
> error is a common normal case by passing a flag to not consider it an
> error.
>
> Silently losing good diagnostic messages when err == NULL would have
> the opposite effect: when there isn't a spare strbuf to put errors in
> around, it would be tempting for people coding in a hurry to just pass
> NULL, and to readers it would look at first glance like "oh, an
> optional paramter was not passed and we are getting the good default
> behavior".
>
> This is not a theoretical concern --- it actually happened.
>

Fair enough.
Un-LGTM my message above.



> My two cents,
> Jonathan

Re: [PATCH v5 1/1] refs.c: use a stringlist for repack_without_refs

[Jonathan Nieder]

On Thu, Nov 20, 2014 at 10:04:26AM -0800, Stefan Beller wrote:

> [Subject: refs.c: use a stringlist for repack_without_refs]

One more nitpick. :)

s/stringlist/string_list/

Thanks,
Jonathan

Re: [PATCH] refs.c: repack_without_refs may be called without error string buffer

[Stefan Beller]

ok, will drop the patch due to bad design.

On Thu, Nov 20, 2014 at 10:36 AM, Ronnie Sahlberg <sahlberg@google.com> wrote:
> On Thu, Nov 20, 2014 at 10:35 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
>> Stefan Beller wrote:
>>
>>> If we don't pass in the error string buffer, we skip over all
>>> parts dealing with preparing error messages.
>>
>> Please no.
>>
>> We tried this with the ref transaction code.  When someone wants
>> to silence the message, it is cheap enough to do
>>
>>         struct strbuf ignore = STRBUF_INIT;
>>
>>         if (thing_that_can_fail_in_an_ignorable_way(..., &ignore)) {
>>                 ... handle the failure ...
>>         }
>>
>> The extra lines of code make it obvious that the error message is
>> being dropped, which is a very good thing.  The extra work to format a
>> message in the error case is not so bad and can be mitigated if the
>> error is a common normal case by passing a flag to not consider it an
>> error.
>>
>> Silently losing good diagnostic messages when err == NULL would have
>> the opposite effect: when there isn't a spare strbuf to put errors in
>> around, it would be tempting for people coding in a hurry to just pass
>> NULL, and to readers it would look at first glance like "oh, an
>> optional paramter was not passed and we are getting the good default
>> behavior".
>>
>> This is not a theoretical concern --- it actually happened.
>>
>
> Fair enough.
> Un-LGTM my message above.
>
>
>
>> My two cents,
>> Jonathan

Re: [PATCH v5 1/1] refs.c: use a stringlist for repack_without_refs

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

>  Junio, I'll address your proposed changes in a different patch. 
>  If err is passed in as NULL, we'll just skip all the error string 
>  formatting and return silent and fast.

Huh, I lost track, but I never meant to say "the functions should
return silently with error code when err == NULL".  I said that it
is another plausible expectation, hence justifies the comment to
clarify, but wished that there were no need to clarify in the first
place.

If everybody required err != NULL, there would be no need to clarify
which functions require err != NULL.  If everybody accepted err ==
NULL as a more efficient way to do "--quiet", that is another way to
remove the need to clarify.

Either way is fine and I did not "propose" anything ;-).

I think this matches more-or-less what I've locally tweaked after
following the discussion between you and Jonathan.  Thanks.

Re: [PATCH v5 1/1] refs.c: use a stringlist for repack_without_refs

[Stefan Beller]

On Thu, Nov 20, 2014 at 11:01 AM, Junio C Hamano <gitster@pobox.com> wrote:
>
> I think this matches more-or-less what I've locally tweaked after
> following the discussion between you and Jonathan.  Thanks.

Do you want me to resend the patch with Jonathans nits fixed?

Jonathan wrote:
>> Change-Id: Id7eaa821331f2ab89df063e1e76c8485dbcc3aed
> Change-id snuck in.

I need to get the format patch hook running, I was talking about
earlier this week.

[PATCH v6] refs.c: use a string_list for repack_without_refs

[Stefan Beller]

From: Ronnie Sahlberg <sahlberg@google.com>

This patch doesn't intend any functional changes. It is just
a refactoring, which replaces a char** array by a stringlist
in the function repack_without_refs.

This is easier to read and maintain as it delivers the same
functionality with less lines of code and more lines of
documentation.

[sb: ported this patch from a larger patch series to the
master branch, added documentary comments in refs.h]

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
---

Changes to version 1:
 * removed the double blank line
 * rename delete_refs_list to delete_refs
 * add back comments dropped by accident
 * use STRING_LIST_INIT_NODUP instead of the _DUP version in ref_transaction_commit
 * add documentary comments on the repack_without_refs function
 * user string_list_append instead of string_list_insert as it follows the previous
   behavior more closely.
 * put back the early exit of the loop in repack_without_refs
   
Changes to version 2:
 * fixed commit message (comments after the three dashes)
 * fixed the curiosity Junio pointed out as it was just wrong code.
   Now it actually builds a list of all states.stale.items[i].util items.
   
Changes in version 3:

 * reword commit message
 * sort delete_refs before passing it to warn_dangling_symrefs
 * change the comments (get back the one jrn complained about) 
   in repack_without_refs
 * use the suggestion of jonathan for documenting repack_without_refs in the
   header. Add a note about the arguments.

Changes in version 4:
 * I lied, when saying I had all the nits from Jonathan.
   I messed up the documentation in the header.
   This includes the documentary comment in the header.

Changes in version 5:
 * Break lines as suggested by Jonathan, slightly rewording the commit message
 * have an empty line at another place in builtin/remote.c remove_branches to
   tell cleanup parts apart from actual work.
 * fix typo, improve documentary comment in refs.h
 * add Jonathans reviewed by

Changes in version 6:
 * remove change id snuck in v5
 * s/stringlist/string_list/ in commit message title
 * reworded the documentary comment in refs.h once again


 builtin/remote.c | 32 ++++++++++++--------------------
 refs.c           | 38 ++++++++++++++++++++------------------
 refs.h           | 12 ++++++++++--
 3 files changed, 42 insertions(+), 40 deletions(-)

diff --git a/builtin/remote.c b/builtin/remote.c
index 7f28f92..364350a 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -750,16 +750,11 @@ static int mv(int argc, const char **argv)
 static int remove_branches(struct string_list *branches)
 {
 	struct strbuf err = STRBUF_INIT;
-	const char **branch_names;
 	int i, result = 0;
 
-	branch_names = xmalloc(branches->nr * sizeof(*branch_names));
-	for (i = 0; i < branches->nr; i++)
-		branch_names[i] = branches->items[i].string;
-	if (repack_without_refs(branch_names, branches->nr, &err))
+	if (repack_without_refs(branches, &err))
 		result |= error("%s", err.buf);
 	strbuf_release(&err);
-	free(branch_names);
 
 	for (i = 0; i < branches->nr; i++) {
 		struct string_list_item *item = branches->items + i;
@@ -1316,8 +1311,8 @@ static int prune_remote(const char *remote, int dry_run)
 {
 	int result = 0, i;
 	struct ref_states states;
-	struct string_list delete_refs_list = STRING_LIST_INIT_NODUP;
-	const char **delete_refs;
+	struct string_list delete_refs = STRING_LIST_INIT_NODUP;
+	struct string_list_item *ref;
 	const char *dangling_msg = dry_run
 		? _(" %s will become dangling!")
 		: _(" %s has become dangling!");
@@ -1325,6 +1320,9 @@ static int prune_remote(const char *remote, int dry_run)
 	memset(&states, 0, sizeof(states));
 	get_remote_ref_states(remote, &states, GET_REF_STATES);
 
+	for (i = 0; i < states.stale.nr; i++)
+		string_list_append(&delete_refs, states.stale.items[i].util);
+
 	if (states.stale.nr) {
 		printf_ln(_("Pruning %s"), remote);
 		printf_ln(_("URL: %s"),
@@ -1332,23 +1330,16 @@ static int prune_remote(const char *remote, int dry_run)
 		       ? states.remote->url[0]
 		       : _("(no URL)"));
 
-		delete_refs = xmalloc(states.stale.nr * sizeof(*delete_refs));
-		for (i = 0; i < states.stale.nr; i++)
-			delete_refs[i] = states.stale.items[i].util;
 		if (!dry_run) {
 			struct strbuf err = STRBUF_INIT;
-			if (repack_without_refs(delete_refs, states.stale.nr,
-						&err))
+			if (repack_without_refs(&delete_refs, &err))
 				result |= error("%s", err.buf);
 			strbuf_release(&err);
 		}
-		free(delete_refs);
 	}
 
-	for (i = 0; i < states.stale.nr; i++) {
-		const char *refname = states.stale.items[i].util;
-
-		string_list_insert(&delete_refs_list, refname);
+	for_each_string_list_item(ref, &delete_refs) {
+		const char *refname = ref->string;
 
 		if (!dry_run)
 			result |= delete_ref(refname, NULL, 0);
@@ -1361,9 +1352,10 @@ static int prune_remote(const char *remote, int dry_run)
 			       abbrev_ref(refname, "refs/remotes/"));
 	}
 
-	warn_dangling_symrefs(stdout, dangling_msg, &delete_refs_list);
-	string_list_clear(&delete_refs_list, 0);
+	sort_string_list(&delete_refs);
+	warn_dangling_symrefs(stdout, dangling_msg, &delete_refs);
 
+	string_list_clear(&delete_refs, 0);
 	free_remote_ref_states(&states);
 	return result;
 }
diff --git a/refs.c b/refs.c
index 5ff457e..ebcd90f 100644
--- a/refs.c
+++ b/refs.c
@@ -2639,23 +2639,26 @@ static int curate_packed_ref_fn(struct ref_entry *entry, void *cb_data)
 	return 0;
 }
 
-int repack_without_refs(const char **refnames, int n, struct strbuf *err)
+int repack_without_refs(struct string_list *without, struct strbuf *err)
 {
 	struct ref_dir *packed;
 	struct string_list refs_to_delete = STRING_LIST_INIT_DUP;
 	struct string_list_item *ref_to_delete;
-	int i, ret, removed = 0;
+	int ret, needs_repacking = 0, removed = 0;
 
 	assert(err);
 
 	/* Look for a packed ref */
-	for (i = 0; i < n; i++)
-		if (get_packed_ref(refnames[i]))
+	for_each_string_list_item(ref_to_delete, without) {
+		if (get_packed_ref(ref_to_delete->string)) {
+			needs_repacking = 1;
 			break;
+		}
+	}
 
 	/* Avoid locking if we have nothing to do */
-	if (i == n)
-		return 0; /* no refname exists in packed refs */
+	if (!needs_repacking)
+		return 0;
 
 	if (lock_packed_refs(0)) {
 		unable_to_lock_message(git_path("packed-refs"), errno, err);
@@ -2664,8 +2667,8 @@ int repack_without_refs(const char **refnames, int n, struct strbuf *err)
 	packed = get_packed_refs(&ref_cache);
 
 	/* Remove refnames from the cache */
-	for (i = 0; i < n; i++)
-		if (remove_entry(packed, refnames[i]) != -1)
+	for_each_string_list_item(ref_to_delete, without)
+		if (remove_entry(packed, ref_to_delete->string) != -1)
 			removed = 1;
 	if (!removed) {
 		/*
@@ -3738,10 +3741,11 @@ static int ref_update_reject_duplicates(struct ref_update **updates, int n,
 int ref_transaction_commit(struct ref_transaction *transaction,
 			   struct strbuf *err)
 {
-	int ret = 0, delnum = 0, i;
-	const char **delnames;
+	int ret = 0, i;
 	int n = transaction->nr;
 	struct ref_update **updates = transaction->updates;
+	struct string_list refs_to_delete = STRING_LIST_INIT_NODUP;
+	struct string_list_item *ref_to_delete;
 
 	assert(err);
 
@@ -3753,9 +3757,6 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 		return 0;
 	}
 
-	/* Allocate work space */
-	delnames = xmalloc(sizeof(*delnames) * n);
-
 	/* Copy, sort, and reject duplicate refs */
 	qsort(updates, n, sizeof(*updates), ref_update_compare);
 	if (ref_update_reject_duplicates(updates, n, err)) {
@@ -3815,16 +3816,17 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 			}
 
 			if (!(update->flags & REF_ISPRUNING))
-				delnames[delnum++] = update->lock->ref_name;
+				string_list_append(&refs_to_delete,
+						   update->lock->ref_name);
 		}
 	}
 
-	if (repack_without_refs(delnames, delnum, err)) {
+	if (repack_without_refs(&refs_to_delete, err)) {
 		ret = TRANSACTION_GENERIC_ERROR;
 		goto cleanup;
 	}
-	for (i = 0; i < delnum; i++)
-		unlink_or_warn(git_path("logs/%s", delnames[i]));
+	for_each_string_list_item(ref_to_delete, &refs_to_delete)
+		unlink_or_warn(git_path("logs/%s", ref_to_delete->string));
 	clear_loose_ref_cache(&ref_cache);
 
 cleanup:
@@ -3833,7 +3835,7 @@ cleanup:
 	for (i = 0; i < n; i++)
 		if (updates[i]->lock)
 			unlock_ref(updates[i]->lock);
-	free(delnames);
+	string_list_clear(&refs_to_delete, 0);
 	return ret;
 }
 
diff --git a/refs.h b/refs.h
index 2bc3556..0bc6a1a 100644
--- a/refs.h
+++ b/refs.h
@@ -163,8 +163,16 @@ extern void rollback_packed_refs(void);
  */
 int pack_refs(unsigned int flags);
 
-extern int repack_without_refs(const char **refnames, int n,
-			       struct strbuf *err);
+/*
+ * Remove the refs listed in the unsorted string list 'without' from
+ * the packed-refs file. On error, packed-refs will be unchanged, the
+ * return value is nonzero, and a message about the error is written
+ * to the 'err' strbuf.
+ *
+ * The refs in 'without' may be unsorted.
+ * 'err' must not be NULL.
+ */
+extern int repack_without_refs(struct string_list *without, struct strbuf *err);
 
 extern int ref_exists(const char *);
 
-- 
2.2.0.rc2.23.gca0107e

Re: [PATCH v6] refs.c: use a string_list for repack_without_refs

[Jonathan Nieder]

Stefan Beller wrote:

> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
> ---

Yep, looks good now.  Thanks for bearing with me.

[...]
> +++ b/refs.h
> @@ -163,8 +163,16 @@ extern void rollback_packed_refs(void);
[...]
> +/*
> + * Remove the refs listed in the unsorted string list 'without' from
> + * the packed-refs file. On error, packed-refs will be unchanged, the
> + * return value is nonzero, and a message about the error is written
> + * to the 'err' strbuf.
> + *
> + * The refs in 'without' may be unsorted.
> + * 'err' must not be NULL.

I think we've gone back and forth enough on this text and it's not
worth the transactional cost to tweak it further, so I'm not
suggesting a change --- just explaining how I read it for future
reference.

"may be unsorted" is confusing to me.  It sounds like the reader of
this comment (someone calling repack_without_refs) has to be prepared
for that possibility.  But we are saying the opposite --- not "be
prepared", but "don't worry about sorting 'without', since
repack_without_refs can handle it".

It's also redundant, since the paragraph above already says that
'without' is an unsorted string list.

The way I see it, there are four types that for various reasons (lack
of language-level support for subclassing, etc) are conflated into a
single struct in the string-list API:

 * sorted string list that owns its items (i.e., created with DUP)
 * sorted string list that does not own its items (i.e., created with NODUP)
 * unsorted string list that owns its items
 * unsorted string list that does not own its items

Different functions are valid to call on each type, as documented in
the comments in string-list.h.

repack_without_refs accepts all 4 types of string-list.  That's what
it means when the documentation says its argument is unsorted.

Thanks,
Jonathan

[PATCH 0/6] repack_without_refs(): convert to string_list

[Michael Haggerty]

This is basically an atomized version of Ronnie/Jonathan/Stefan's
patch [1] "refs.c: use a stringlist for repack_without_refs". But I've
actually rewritten most of it from scratch, using the original patch
as a reference.

I was reviewing the original patch and it looked mostly OK [2], but I
found it hard to read because it did several steps at once. So I tried
to make the same basic change, but one baby step at a time. This is
the result.

I'm a known fanatic about making the smallest possible changes in each
commit. The goal is to make the patch series as readable as possible,
because reviewers' time is in shorter supply than coders' time.

* Tiny little patches are IMO usually much easier to read than big
  ones, because there is less to keep in mind at a time.

* Often tiny changes (e.g., renaming variables or functions) are so
  blindingly obvious that one only has to skim them, or even trust
  that the author, with the help of the compiler, could hardly have
  made a mistake [3].

* Using baby steps keeps the author from introducing unnecessary
  changes ("code churn"), by forcing him/her to justify each change on
  its own merits.

* Using baby steps makes it harder for substantive changes to get
  overlooked or to sneak in without discussion [4].

* If there is a problem, baby commits can be bisected, usually making
  it obvious why the bug arose.

* If the mailing list doesn't like part of the series, it is usually
  easier to omit a patch from the next reroll than to extract one
  change out of a patch that contains multiple logical changes.

* It is often possible to arrange the order of the patches to give the
  patch series a good "narrative".

Some members of the community probably disagree with me. Using baby
step patches means that there is more mailing list traffic and more
commits that accumulate in the project's history. There is sometimes a
bit of extra to-and-fro as code is mutated incrementally. Or maybe
other people can just keep more complicated changes in their heads at
one time than I can.

Nevertheless, I submit this version of the patch series for your
amusement. Feel free to ignore it.

[1] http://mid.gmane.org/1416434399-2303-1-git-send-email-sbeller@google.com
[2] Problems that I noticed:

    * The commit message refers to "stringlist" where it should be
      "string_list".

    * One of the loops in prune_remote() iterates using indexes, while
      another loop (over the same string_list) uses
      for_each_string_list_item().

    * The change from using string_list_insert() to string_list_append()
      in the same function, followed by sort_string_list(), doesn't remove
      duplicates as the old version did. The commit message should
      justify that this is OK.

[3] I love the quote from C. A. R. Hoare:

        There are two ways of constructing a software design: One way
        is to make it so simple that there are obviously no
        deficiencies, and the other way is to make it so complicated
        that there are no obvious deficiencies.

    I think the same thing applies to patches.

[4] Case in point: when I was writing the commit message for patch
    3/6, I realized that string_list_insert() omits duplicates whereas
    string_list_append() obviously doesn't. This aspect of the change
    wasn't justified. Do we have to add a call to
    string_list_remove_duplicates()? It turns out that the list cannot
    contain duplicates, but it took some digging to verify this.

Michael Haggerty (6):
  prune_remote(): exit early if there are no stale references
  prune_remote(): initialize both delete_refs lists in a single loop
  prune_remote(): sort delete_refs_list references en masse
  repack_without_refs(): make the refnames argument a string_list
  prune_remote(): rename local variable
  prune_remote(): iterate using for_each_string_list_item()

 builtin/remote.c | 59 ++++++++++++++++++++++++++------------------------------
 refs.c           | 38 +++++++++++++++++++-----------------
 refs.h           | 11 ++++++++++-
 3 files changed, 57 insertions(+), 51 deletions(-)

-- 
2.1.3

[PATCH 1/6] prune_remote(): exit early if there are no stale references

[Michael Haggerty]

Aside from making the logic clearer, this avoids a call to
warn_dangling_symrefs(), which always does a for_each_rawref()
iteration.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 builtin/remote.c | 39 +++++++++++++++++++++------------------
 1 file changed, 21 insertions(+), 18 deletions(-)

diff --git a/builtin/remote.c b/builtin/remote.c
index 7f28f92..d2b684c 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -1325,25 +1325,28 @@ static int prune_remote(const char *remote, int dry_run)
 	memset(&states, 0, sizeof(states));
 	get_remote_ref_states(remote, &states, GET_REF_STATES);
 
-	if (states.stale.nr) {
-		printf_ln(_("Pruning %s"), remote);
-		printf_ln(_("URL: %s"),
-		       states.remote->url_nr
-		       ? states.remote->url[0]
-		       : _("(no URL)"));
-
-		delete_refs = xmalloc(states.stale.nr * sizeof(*delete_refs));
-		for (i = 0; i < states.stale.nr; i++)
-			delete_refs[i] = states.stale.items[i].util;
-		if (!dry_run) {
-			struct strbuf err = STRBUF_INIT;
-			if (repack_without_refs(delete_refs, states.stale.nr,
-						&err))
-				result |= error("%s", err.buf);
-			strbuf_release(&err);
-		}
-		free(delete_refs);
+	if (!states.stale.nr) {
+		free_remote_ref_states(&states);
+		return 0;
+	}
+
+	printf_ln(_("Pruning %s"), remote);
+	printf_ln(_("URL: %s"),
+		  states.remote->url_nr
+		  ? states.remote->url[0]
+		  : _("(no URL)"));
+
+	delete_refs = xmalloc(states.stale.nr * sizeof(*delete_refs));
+	for (i = 0; i < states.stale.nr; i++)
+		delete_refs[i] = states.stale.items[i].util;
+	if (!dry_run) {
+		struct strbuf err = STRBUF_INIT;
+		if (repack_without_refs(delete_refs, states.stale.nr,
+					&err))
+			result |= error("%s", err.buf);
+		strbuf_release(&err);
 	}
+	free(delete_refs);
 
 	for (i = 0; i < states.stale.nr; i++) {
 		const char *refname = states.stale.items[i].util;
-- 
2.1.3

[PATCH 2/6] prune_remote(): initialize both delete_refs lists in a single loop

[Michael Haggerty]

Also free them together at the end of the function.

In a moment, the array version will become redundant. Managing them
together makes later steps more obvious.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 builtin/remote.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/builtin/remote.c b/builtin/remote.c
index d2b684c..d5a5a16 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -1337,8 +1337,13 @@ static int prune_remote(const char *remote, int dry_run)
 		  : _("(no URL)"));
 
 	delete_refs = xmalloc(states.stale.nr * sizeof(*delete_refs));
-	for (i = 0; i < states.stale.nr; i++)
-		delete_refs[i] = states.stale.items[i].util;
+	for (i = 0; i < states.stale.nr; i++) {
+		const char *refname = states.stale.items[i].util;
+
+		delete_refs[i] = refname;
+		string_list_insert(&delete_refs_list, refname);
+	}
+
 	if (!dry_run) {
 		struct strbuf err = STRBUF_INIT;
 		if (repack_without_refs(delete_refs, states.stale.nr,
@@ -1346,13 +1351,10 @@ static int prune_remote(const char *remote, int dry_run)
 			result |= error("%s", err.buf);
 		strbuf_release(&err);
 	}
-	free(delete_refs);
 
 	for (i = 0; i < states.stale.nr; i++) {
 		const char *refname = states.stale.items[i].util;
 
-		string_list_insert(&delete_refs_list, refname);
-
 		if (!dry_run)
 			result |= delete_ref(refname, NULL, 0);
 
@@ -1365,8 +1367,9 @@ static int prune_remote(const char *remote, int dry_run)
 	}
 
 	warn_dangling_symrefs(stdout, dangling_msg, &delete_refs_list);
-	string_list_clear(&delete_refs_list, 0);
 
+	free(delete_refs);
+	string_list_clear(&delete_refs_list, 0);
 	free_remote_ref_states(&states);
 	return result;
 }
-- 
2.1.3

[PATCH 3/6] prune_remote(): sort delete_refs_list references en masse

[Michael Haggerty]

Inserting items into a list in sorted order is O(N^2) whereas
appending them unsorted and then sorting the list all at once is
O(N lg N).

string_list_insert() also removes duplicates, and this change loses
that functionality. But the strings in this list, which ultimately
come from a for_each_ref() iteration, cannot contain duplicates.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 builtin/remote.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/builtin/remote.c b/builtin/remote.c
index d5a5a16..7d5c8d2 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -1341,8 +1341,9 @@ static int prune_remote(const char *remote, int dry_run)
 		const char *refname = states.stale.items[i].util;
 
 		delete_refs[i] = refname;
-		string_list_insert(&delete_refs_list, refname);
+		string_list_append(&delete_refs_list, refname);
 	}
+	sort_string_list(&delete_refs_list);
 
 	if (!dry_run) {
 		struct strbuf err = STRBUF_INIT;
-- 
2.1.3

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

[Michael Haggerty]

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

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

diff --git a/builtin/remote.c b/builtin/remote.c
index 7d5c8d2..63a6709 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -750,16 +750,11 @@ static int mv(int argc, const char **argv)
 static int remove_branches(struct string_list *branches)
 {
 	struct strbuf err = STRBUF_INIT;
-	const char **branch_names;
 	int i, result = 0;
 
-	branch_names = xmalloc(branches->nr * sizeof(*branch_names));
-	for (i = 0; i < branches->nr; i++)
-		branch_names[i] = branches->items[i].string;
-	if (repack_without_refs(branch_names, branches->nr, &err))
+	if (repack_without_refs(branches, &err))
 		result |= error("%s", err.buf);
 	strbuf_release(&err);
-	free(branch_names);
 
 	for (i = 0; i < branches->nr; i++) {
 		struct string_list_item *item = branches->items + i;
@@ -1317,7 +1312,6 @@ static int prune_remote(const char *remote, int dry_run)
 	int result = 0, i;
 	struct ref_states states;
 	struct string_list delete_refs_list = STRING_LIST_INIT_NODUP;
-	const char **delete_refs;
 	const char *dangling_msg = dry_run
 		? _(" %s will become dangling!")
 		: _(" %s has become dangling!");
@@ -1336,19 +1330,16 @@ static int prune_remote(const char *remote, int dry_run)
 		  ? states.remote->url[0]
 		  : _("(no URL)"));
 
-	delete_refs = xmalloc(states.stale.nr * sizeof(*delete_refs));
 	for (i = 0; i < states.stale.nr; i++) {
 		const char *refname = states.stale.items[i].util;
 
-		delete_refs[i] = refname;
 		string_list_append(&delete_refs_list, refname);
 	}
 	sort_string_list(&delete_refs_list);
 
 	if (!dry_run) {
 		struct strbuf err = STRBUF_INIT;
-		if (repack_without_refs(delete_refs, states.stale.nr,
-					&err))
+		if (repack_without_refs(&delete_refs_list, &err))
 			result |= error("%s", err.buf);
 		strbuf_release(&err);
 	}
@@ -1369,7 +1360,6 @@ static int prune_remote(const char *remote, int dry_run)
 
 	warn_dangling_symrefs(stdout, dangling_msg, &delete_refs_list);
 
-	free(delete_refs);
 	string_list_clear(&delete_refs_list, 0);
 	free_remote_ref_states(&states);
 	return result;
diff --git a/refs.c b/refs.c
index 5ff457e..b675e01 100644
--- a/refs.c
+++ b/refs.c
@@ -2639,22 +2639,25 @@ static int curate_packed_ref_fn(struct ref_entry *entry, void *cb_data)
 	return 0;
 }
 
-int repack_without_refs(const char **refnames, int n, struct strbuf *err)
+int repack_without_refs(struct string_list *refnames, struct strbuf *err)
 {
 	struct ref_dir *packed;
 	struct string_list refs_to_delete = STRING_LIST_INIT_DUP;
-	struct string_list_item *ref_to_delete;
-	int i, ret, removed = 0;
+	struct string_list_item *refname, *ref_to_delete;
+	int ret, needs_repacking = 0, removed = 0;
 
 	assert(err);
 
 	/* Look for a packed ref */
-	for (i = 0; i < n; i++)
-		if (get_packed_ref(refnames[i]))
+	for_each_string_list_item(refname, refnames) {
+		if (get_packed_ref(refname->string)) {
+			needs_repacking = 1;
 			break;
+		}
+	}
 
 	/* Avoid locking if we have nothing to do */
-	if (i == n)
+	if (!needs_repacking)
 		return 0; /* no refname exists in packed refs */
 
 	if (lock_packed_refs(0)) {
@@ -2664,8 +2667,8 @@ int repack_without_refs(const char **refnames, int n, struct strbuf *err)
 	packed = get_packed_refs(&ref_cache);
 
 	/* Remove refnames from the cache */
-	for (i = 0; i < n; i++)
-		if (remove_entry(packed, refnames[i]) != -1)
+	for_each_string_list_item(refname, refnames)
+		if (remove_entry(packed, refname->string) != -1)
 			removed = 1;
 	if (!removed) {
 		/*
@@ -3738,10 +3741,11 @@ static int ref_update_reject_duplicates(struct ref_update **updates, int n,
 int ref_transaction_commit(struct ref_transaction *transaction,
 			   struct strbuf *err)
 {
-	int ret = 0, delnum = 0, i;
-	const char **delnames;
+	int ret = 0, i;
 	int n = transaction->nr;
 	struct ref_update **updates = transaction->updates;
+	struct string_list refs_to_delete = STRING_LIST_INIT_NODUP;
+	struct string_list_item *ref_to_delete;
 
 	assert(err);
 
@@ -3753,9 +3757,6 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 		return 0;
 	}
 
-	/* Allocate work space */
-	delnames = xmalloc(sizeof(*delnames) * n);
-
 	/* Copy, sort, and reject duplicate refs */
 	qsort(updates, n, sizeof(*updates), ref_update_compare);
 	if (ref_update_reject_duplicates(updates, n, err)) {
@@ -3815,16 +3816,17 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 			}
 
 			if (!(update->flags & REF_ISPRUNING))
-				delnames[delnum++] = update->lock->ref_name;
+				string_list_append(&refs_to_delete,
+						   update->lock->ref_name);
 		}
 	}
 
-	if (repack_without_refs(delnames, delnum, err)) {
+	if (repack_without_refs(&refs_to_delete, err)) {
 		ret = TRANSACTION_GENERIC_ERROR;
 		goto cleanup;
 	}
-	for (i = 0; i < delnum; i++)
-		unlink_or_warn(git_path("logs/%s", delnames[i]));
+	for_each_string_list_item(ref_to_delete, &refs_to_delete)
+		unlink_or_warn(git_path("logs/%s", ref_to_delete->string));
 	clear_loose_ref_cache(&ref_cache);
 
 cleanup:
@@ -3833,7 +3835,7 @@ cleanup:
 	for (i = 0; i < n; i++)
 		if (updates[i]->lock)
 			unlock_ref(updates[i]->lock);
-	free(delnames);
+	string_list_clear(&refs_to_delete, 0);
 	return ret;
 }
 
diff --git a/refs.h b/refs.h
index 2bc3556..90a4a40 100644
--- a/refs.h
+++ b/refs.h
@@ -163,7 +163,16 @@ extern void rollback_packed_refs(void);
  */
 int pack_refs(unsigned int flags);
 
-extern int repack_without_refs(const char **refnames, int n,
+/*
+ * Remove the refs listed in 'refnames' from the packed-refs file.
+ * On error, packed-refs will be unchanged, the return value is
+ * nonzero, and a message about the error is written to the 'err'
+ * strbuf.
+ *
+ * The refs in 'refnames' needn't be sorted. The err buffer must not be
+ * omitted.
+ */
+extern int repack_without_refs(struct string_list *refnames,
 			       struct strbuf *err);
 
 extern int ref_exists(const char *);
-- 
2.1.3

[PATCH 5/6] prune_remote(): rename local variable

[Michael Haggerty]

Rename "delete_refs_list" to "refs_to_prune". The new name is more
self-explanatory.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 builtin/remote.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/builtin/remote.c b/builtin/remote.c
index 63a6709..efbf5fb 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -1311,7 +1311,7 @@ static int prune_remote(const char *remote, int dry_run)
 {
 	int result = 0, i;
 	struct ref_states states;
-	struct string_list delete_refs_list = STRING_LIST_INIT_NODUP;
+	struct string_list refs_to_prune = STRING_LIST_INIT_NODUP;
 	const char *dangling_msg = dry_run
 		? _(" %s will become dangling!")
 		: _(" %s has become dangling!");
@@ -1333,13 +1333,13 @@ static int prune_remote(const char *remote, int dry_run)
 	for (i = 0; i < states.stale.nr; i++) {
 		const char *refname = states.stale.items[i].util;
 
-		string_list_append(&delete_refs_list, refname);
+		string_list_append(&refs_to_prune, refname);
 	}
-	sort_string_list(&delete_refs_list);
+	sort_string_list(&refs_to_prune);
 
 	if (!dry_run) {
 		struct strbuf err = STRBUF_INIT;
-		if (repack_without_refs(&delete_refs_list, &err))
+		if (repack_without_refs(&refs_to_prune, &err))
 			result |= error("%s", err.buf);
 		strbuf_release(&err);
 	}
@@ -1358,9 +1358,9 @@ static int prune_remote(const char *remote, int dry_run)
 			       abbrev_ref(refname, "refs/remotes/"));
 	}
 
-	warn_dangling_symrefs(stdout, dangling_msg, &delete_refs_list);
+	warn_dangling_symrefs(stdout, dangling_msg, &refs_to_prune);
 
-	string_list_clear(&delete_refs_list, 0);
+	string_list_clear(&refs_to_prune, 0);
 	free_remote_ref_states(&states);
 	return result;
 }
-- 
2.1.3

[PATCH 6/6] prune_remote(): iterate using for_each_string_list_item()

[Michael Haggerty]

Iterate over refs_to_prune using for_each_string_list_item() rather
than writing out the loop in longhand.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 builtin/remote.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/builtin/remote.c b/builtin/remote.c
index efbf5fb..7fec170 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -1309,9 +1309,10 @@ static int set_head(int argc, const char **argv)
 
 static int prune_remote(const char *remote, int dry_run)
 {
-	int result = 0, i;
+	int result = 0;
 	struct ref_states states;
 	struct string_list refs_to_prune = STRING_LIST_INIT_NODUP;
+	struct string_list_item *item;
 	const char *dangling_msg = dry_run
 		? _(" %s will become dangling!")
 		: _(" %s has become dangling!");
@@ -1330,11 +1331,8 @@ static int prune_remote(const char *remote, int dry_run)
 		  ? states.remote->url[0]
 		  : _("(no URL)"));
 
-	for (i = 0; i < states.stale.nr; i++) {
-		const char *refname = states.stale.items[i].util;
-
-		string_list_append(&refs_to_prune, refname);
-	}
+	for_each_string_list_item(item, &states.stale)
+		string_list_append(&refs_to_prune, item->util);
 	sort_string_list(&refs_to_prune);
 
 	if (!dry_run) {
@@ -1344,8 +1342,8 @@ static int prune_remote(const char *remote, int dry_run)
 		strbuf_release(&err);
 	}
 
-	for (i = 0; i < states.stale.nr; i++) {
-		const char *refname = states.stale.items[i].util;
+	for_each_string_list_item(item, &states.stale) {
+		const char *refname = item->util;
 
 		if (!dry_run)
 			result |= delete_ref(refname, NULL, 0);
-- 
2.1.3

Re: [PATCH 0/6] repack_without_refs(): convert to string_list

[Michael Haggerty]

On 11/21/2014 03:09 PM, Michael Haggerty wrote:
> This is basically an atomized version of Ronnie/Jonathan/Stefan's
> patch [1] "refs.c: use a stringlist for repack_without_refs". But I've
> actually rewritten most of it from scratch, using the original patch
> as a reference.

Naturally, right after I emailed this series I realized that there have
been two more iterations on the original patch, which I overlooked
because I was not CCed on them. (I'm not complaining, just explaining.)

I don't think that those iterations changed anything substantial that
overlaps with my version, but TBH it's such a pain in the ass working
with patches in email that I don't think I'll go to the effort of
checking for sure unless somebody shows interest in actually using my
version.

Sorry for being grumpy today :-(

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu

Re: [PATCH 3/6] prune_remote(): sort delete_refs_list references en masse

[Junio C Hamano]

On Fri, Nov 21, 2014 at 6:09 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> Inserting items into a list in sorted order is O(N^2) whereas
> appending them unsorted and then sorting the list all at once is
> O(N lg N).
>
> string_list_insert() also removes duplicates, and this change loses
> that functionality. But the strings in this list, which ultimately
> come from a for_each_ref() iteration, cannot contain duplicates.
>

A similar conversion in other places we may do in the future
might find a need for an equivalent to "-u" option of "sort" in the
string_list_sort() function, but the above nicely explains why
it is not necessary for this one.  Good.

Eh, why is that called sort_string_list()?  Perhaps it is a good
opening to introduce string_list_sort(list, flag) where flag would
be a bitmask that represents ignore-case, uniquify, etc., and
then either deprecate the current one or make it a thin wrapper
of the one that is more consistently named.


> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
>  builtin/remote.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/remote.c b/builtin/remote.c
> index d5a5a16..7d5c8d2 100644
> --- a/builtin/remote.c
> +++ b/builtin/remote.c
> @@ -1341,8 +1341,9 @@ static int prune_remote(const char *remote, int dry_run)
>                 const char *refname = states.stale.items[i].util;
>
>                 delete_refs[i] = refname;
> -               string_list_insert(&delete_refs_list, refname);
> +               string_list_append(&delete_refs_list, refname);
>         }
> +       sort_string_list(&delete_refs_list);
>
>         if (!dry_run) {
>                 struct strbuf err = STRBUF_INIT;
> --
> 2.1.3
>

Re: [PATCH 0/6] repack_without_refs(): convert to string_list

[Junio C Hamano]

Michael Haggerty <mhagger@alum.mit.edu> writes:

> I don't think that those iterations changed anything substantial that
> overlaps with my version, but TBH it's such a pain in the ass working
> with patches in email that I don't think I'll go to the effort of
> checking for sure unless somebody shows interest in actually using my
> version.
>
> Sorry for being grumpy today :-(

Is the above meant as a grumpy rant to be ignored, or as a
discussion starter to improve the colaboration to allow people to
work better together instead of stepping on each other's patches?

FWIW, I liked your rationale for "many smaller steps".

One small uncomfort in that approach is that it often is not very
obvious by reading "log -p master.." alone how well the end result
fits together.  Each individual step may make sense, or at least it
may not make it any worse than the original, but until you apply the
whole series and read "diff master..." in a sitting, it is somewhat
hard to tell where you are going.  But this is not "risk" or "bad
thing"; just something that may make readers feel uncomfortable---we
are not losing anything by splitting a series into small logical
chunks.

Thanks.

Re: [PATCH 0/6] repack_without_refs(): convert to string_list

[Stefan Beller]

On Fri, Nov 21, 2014 at 10:00 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
>
>> I don't think that those iterations changed anything substantial that
>> overlaps with my version, but TBH it's such a pain in the ass working
>> with patches in email that I don't think I'll go to the effort of
>> checking for sure unless somebody shows interest in actually using my
>> version.
>>
>> Sorry for being grumpy today :-(

Sorry for causing the grumpyness.
I have compared the versions, and they do look pretty similar.
In refs.{c,h} we're just talking about variable names and comments,
that are different.

In remote.c prune_remote however we did have slight differences,
* early exit vs a large body below an if
* your approach seems more elegant to me as you seem to know what you're doing:
       for_each_string_list_item(item, &states.stale)
               string_list_append(&refs_to_prune, item->util);
 instead of
       for (i = 0; i < states.stale.nr; i++)
               string_list_append(&delete_refs, states.stale.items[i].util);
* You do not have a sort_string_list at the end before warn_dangling_symrefs,
   but you explained that it is not necessary.

On my continued journey on this mailing list I'll try to follow your
example and write lots of
small easy to review patches, as they are indeed way easier to follow.

However as Junio mentioned, we get other problems having too small changes.
In the review for the [PATCH v3 00/14] ref-transactions-reflog series you said:

> I was reviewing this patch series (I left some comments in Gerrit about
> the first few patches) when I realized that I'm having trouble
> understanding the big picture of where you want to go with this.

Maybe that was just my fault, not having stated the intentions in
the cover letter explicit enough. But having many patches will also not help
on presenting the big picture easily.

Thanks for bearing with me,
Stefan

>
> Is the above meant as a grumpy rant to be ignored, or as a
> discussion starter to improve the colaboration to allow people to
> work better together instead of stepping on each other's patches?
>
> FWIW, I liked your rationale for "many smaller steps".
>
> One small uncomfort in that approach is that it often is not very
> obvious by reading "log -p master.." alone how well the end result
> fits together.  Each individual step may make sense, or at least it
> may not make it any worse than the original, but until you apply the
> whole series and read "diff master..." in a sitting, it is somewhat
> hard to tell where you are going.  But this is not "risk" or "bad
> thing"; just something that may make readers feel uncomfortable---we
> are not losing anything by splitting a series into small logical
> chunks.
>
> Thanks.
>
>

Re: [PATCH 1/6] prune_remote(): exit early if there are no stale references

[Jonathan Nieder]

Michael Haggerty wrote:

> Aside from making the logic clearer, this avoids a call to
> warn_dangling_symrefs(), which always does a for_each_rawref()
> iteration.
>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
>  builtin/remote.c | 39 +++++++++++++++++++++------------------
>  1 file changed, 21 insertions(+), 18 deletions(-)

I had been wondering about this but didn't chase it down far enough.
Thanks for noticing and cleaning it up.

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Re: [PATCH 3/6] prune_remote(): sort delete_refs_list references en masse

[Jonathan Nieder]

Michael Haggerty wrote:

> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
>  builtin/remote.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

This and 2/6 are also

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

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

[Jonathan Nieder]

Michael Haggerty wrote:

> All of the callers have string_lists available already

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

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

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

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

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

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

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

(nit)

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

Thanks,
Jonathan

Re: [PATCH 5/6] prune_remote(): rename local variable

[Jonathan Nieder]

Michael Haggerty wrote:

> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
>  builtin/remote.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Re: [PATCH 6/6] prune_remote(): iterate using for_each_string_list_item()

[Jonathan Nieder]

Michael Haggerty wrote:

> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
>  builtin/remote.c | 14 ++++++--------
>  1 file changed, 6 insertions(+), 8 deletions(-)

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

(That makes 6/6. :))

Thanks for your thoughtfulness in putting these together.  They were
pleasant to read.

Our cumbersome mailing list workflow (was: Re: [PATCH 0/6] repack_without_refs(): convert to string_list)

[Michael Haggerty]

On 11/21/2014 07:00 PM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
>> I don't think that those iterations changed anything substantial that
>> overlaps with my version, but TBH it's such a pain in the ass working
>> with patches in email that I don't think I'll go to the effort of
>> checking for sure unless somebody shows interest in actually using my
>> version.
>>
>> Sorry for being grumpy today :-(
> 
> Is the above meant as a grumpy rant to be ignored, or as a
> discussion starter to improve the colaboration to allow people to
> work better together instead of stepping on each other's patches?

I think I know the sentiments of the mailing list regulars well enough
that it didn't seem worthwhile to open this topic again, so I was just
letting off steam without any hope of changing anything. But since you
asked...

Let me list the aspects of our mailing list workflow that I find
cumbersome as a contributor and reviewer:

* Submitting patches to the mailing list is an ordeal of configuring
format-patch and send-email and getting everything just right, using
instructions that depend on the local environment. We saw that hardly
any GSoC applicants were able to get it right on their first attempt.
Submitting a patch series should be as simple as "git push".

* Once patches are submitted, there is no assurance that you (Junio)
will apply them to your tree at the same point that the submitter
developed and tested them.

* The branch name that you choose for a patch series is not easily
derivable from the patches as they appeared in the mailing list. Trying
to figure out whether/where the patches exist in your tree is a largely
manual task. The reverse mapping, from in-tree commit to the email where
it was proposed, is even more difficult to infer.

* Your tree has no indication of which version of a patch series (v1,
v2, etc) is currently applied.

The previous three points combine to make it awkward to get patches into
my local repository to review or test. There are two alternatives, both
cumbersome and imprecise:

  * I do "git fetch gitster", then try to figure out whether the branch
I'm interested in is present, what its name is, and whether the version
in your tree is the latest version, then "git checkout xy/foobar".

  * Or I save the emails to a temporary directory (awkward because, Oh
Horror, I use Thunderbird and not mutt as email client), hope that I've
guessed the right place to apply them, run "git am", and later try to
remember to clean up the temporary directory.

* Once I've done that, the "supplemental" comments from the emails (the
cover letter and the text under the "---") are nowhere available in the
Git repository. So if I want to see the changes in context plus the
supplemental comments, I have to jump back and forth between email
client and Git repo. Plus I have to jump around the rest of the email
thread to see what comments other reviewers have already made about the
series.

* Following patch series across iterations is also awkward. To compare
two versions, I have to first get both patch series into my repo, which
involves digging through the ML history to find older versions, followed
by the "git am" steps. Often submitters are nice enough to put links to
previous versions of their patch series in their cover letters, but the
links are to a web-based email archive, from which it is even more
awkward to grab and apply patches. So in practice I then go back to my
email client and search my local archive for my copy of the same email
that was referenced in the archive, and apply the patch from there.
Finding comments about old versions of a patch series is nearly as much
work.

* Because of the indeterminate application point, accumulating
Signed-off-by lines, changed committer metadata, and maintainer tweaks,
the commits that make it to the official tree have different SHA-1s than
the commits in the submitter's tree, and both are different than the
commits in the tree of any reviewer who got the patches using "git am".
This makes it hard to be sure that everybody is on the same page. It
also makes it awkward for people to exchange ideas for further changes
via Git protocols in the form of patches.

* Because of the crude serialization of patches through email, it is
only possible to submit linear patch series, not merge commits.

Hmmm, I think that covers most of the problems of handling patches and
review via a mailing list.


What are some alternatives?

I did enjoy the variety of reviewing some patch series using Gerrit. It
is nice that it tracks the evolution of a patch from version to version,
and that the comments made on all versions of a patch are summarized in
a single place. This makes it easier to avoid commenting on issues that
other reviewers have already noted and easier to check that your own
comments have been addressed by later versions of the patch. On the
other hand, Gerrit seems strongly focused on individual patches rather
than on patch series (which might not match our workflow so well), the
UI is overwhelming (though I think one could get quite productive with
it if one used it every day), and the notification emails come in blizzards.

GitHub is another obvious alternative [1], free for open-source projects
albeit not open-source itself. It is very easy to use and easy to
interact with from a Git client, and also has a good API. It is super
easy to submit patches to a project using GitHub. But the GitHub user
interface (ISTM) is optimized for getting the net result of an entire
feature branch perfect, as opposed to iterating a series of patches
until each one is individually perfect (e.g., it works best when adding
patches on top of a feature branch as opposed to rebasing existing
patches). Since Git development is oriented towards perfecting each
patch, GitHub would be a bit of an impedance mismatch.

I don't think either of those systems is ideally matched to the Git
project's workflow, but in my opinion either one of them would be more
convenient for contributors and reviewers than serializing everything
through the mailing list.

Of course what is most convenient for the maintainer is of huge
importance, but I can't say much about that.

> FWIW, I liked your rationale for "many smaller steps".

Thanks.

> One small uncomfort in that approach is that it often is not very
> obvious by reading "log -p master.." alone how well the end result
> fits together.  Each individual step may make sense, or at least it
> may not make it any worse than the original, but until you apply the
> whole series and read "diff master..." in a sitting, it is somewhat
> hard to tell where you are going.  But this is not "risk" or "bad
> thing"; just something that may make readers feel uncomfortable---we
> are not losing anything by splitting a series into small logical
> chunks.

Ideally, the cover letter should provide the "big picture" rationale for
a patch series, and the individual commit messages should provide clues
about why that step is useful.

It might be a nice convention to ask people to write the "big picture"
rationale in their cover letter, separated by a "---" from non-permanent
discussion. Then the part above the "---" could be copied into the
commit message for the *merge commit* that brings the feature branch
into master. That would preserve it for posterity in a place where it is
relatively easy to find. But I am reluctant to make the process of
submitting patches even more difficult :-)

Michael

[1] Disclaimer: I work for GitHub.

-- 
Michael Haggerty
mhagger@alum.mit.edu

Re: [PATCH 3/6] prune_remote(): sort delete_refs_list references en masse

[Michael Haggerty]

On 11/21/2014 05:44 PM, Junio C Hamano wrote:
> On Fri, Nov 21, 2014 at 6:09 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>> Inserting items into a list in sorted order is O(N^2) whereas
>> appending them unsorted and then sorting the list all at once is
>> O(N lg N).
>>
>> string_list_insert() also removes duplicates, and this change loses
>> that functionality. But the strings in this list, which ultimately
>> come from a for_each_ref() iteration, cannot contain duplicates.
>>
> 
> A similar conversion in other places we may do in the future
> might find a need for an equivalent to "-u" option of "sort" in the
> string_list_sort() function, but the above nicely explains why
> it is not necessary for this one.  Good.

The only reason to integrate "-u" functionality into the sort would be
if one expects a significant fraction of entries to be duplicates, in
which case the sort could be structured to discard duplicates as it
works, thereby reducing the work needed for the sort. I can't think of
such a case in our code. Otherwise, calling sort_string_list() followed
by string_list_remove_duplicates() should be just as clear and
approximately as efficient.

A couple of times I've also felt that an all-purpose *stable* sort would
be convenient (though I can't remember the context offhand). I don't
think we have such a thing.

> Eh, why is that called sort_string_list()?  Perhaps it is a good
> opening to introduce string_list_sort(list, flag) where flag would
> be a bitmask that represents ignore-case, uniquify, etc., and
> then either deprecate the current one or make it a thin wrapper
> of the one that is more consistently named.

I agree. Indeed, I typed that function's name wrong once when
constructing this patch. It would be better to name it consistently with
the other string_list_*() functions.

I put it on my todo list (but don't let that dissuade somebody else from
doing it).

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu

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

[Michael Haggerty]

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

You're right. I'll correct this.

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

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

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

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

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu

Re: [PATCH 3/6] prune_remote(): sort delete_refs_list references en masse

[Michael Haggerty]

On 11/25/2014 08:21 AM, Michael Haggerty wrote:
> On 11/21/2014 05:44 PM, Junio C Hamano wrote:
>> [...]
>> Eh, why is that called sort_string_list()?  Perhaps it is a good
>> opening to introduce string_list_sort(list, flag) where flag would
>> be a bitmask that represents ignore-case, uniquify, etc., and
>> then either deprecate the current one or make it a thin wrapper
>> of the one that is more consistently named.
> 
> I agree. Indeed, I typed that function's name wrong once when
> constructing this patch. It would be better to name it consistently with
> the other string_list_*() functions.
> 
> I put it on my todo list (but don't let that dissuade somebody else from
> doing it).

Since I was re-rolling the patch series anyway, I tacked this renaming
change onto the end of it. Feel free to omit it if you think it belongs
separately.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu

Re: Our cumbersome mailing list workflow

[Torsten Bögershausen]

On 2014-11-25 01.28, Michael Haggerty wrote:
[]
> Let me list the aspects of our mailing list workflow that I find
> cumbersome as a contributor and reviewer:
> 
> * Submitting patches to the mailing list is an ordeal of configuring
> format-patch and send-email and getting everything just right, using
> instructions that depend on the local environment.
Typically everything fits into ~/.gitconfig,
which can be carried around on a USB-Stick.
Is there any details which I miss, or howtows we can improve ?
> We saw that hardly
> any GSoC applicants were able to get it right on their first attempt.
> Submitting a patch series should be as simple as "git push".
> 
> * Once patches are submitted, there is no assurance that you (Junio)
> will apply them to your tree at the same point that the submitter
> developed and tested them.
> 
> * The branch name that you choose for a patch series is not easily
> derivable from the patches as they appeared in the mailing list. Trying
> to figure out whether/where the patches exist in your tree is a largely
> manual task. The reverse mapping, from in-tree commit to the email where
> it was proposed, is even more difficult to infer.
> 
> * Your tree has no indication of which version of a patch series (v1,
> v2, etc) is currently applied.

> 
> The previous three points combine to make it awkward to get patches into
> my local repository to review or test. There are two alternatives, both
> cumbersome and imprecise:
> 
>   * I do "git fetch gitster", then try to figure out whether the branch
> I'm interested in is present, what its name is, and whether the version
> in your tree is the latest version, then "git checkout xy/foobar".
There are 12 branches from mh/, so it should be possible to find the name,
und run git log gitster/xy/fix_this_bug or so.
Even more important, this branch is the "single point of truth", because
this branch may be merged eventually, and nothing else.
> 
>   * Or I save the emails to a temporary directory (awkward because, Oh
> Horror, I use Thunderbird and not mutt as email client), hope that I've
> guessed the right place to apply them, run "git am", and later try to
> remember to clean up the temporary directory.
Is there a "mutt howto" somewhere?
> 
> * Once I've done that, the "supplemental" comments from the emails (the
> cover letter and the text under the "---") are nowhere available in the
> Git repository. So if I want to see the changes in context plus the
> supplemental comments, I have to jump back and forth between email
> client and Git repo. Plus I have to jump around the rest of the email
> thread to see what comments other reviewers have already made about the
> series.
> 
> * Following patch series across iterations is also awkward. To compare
> two versions, I have to first get both patch series into my repo, which
> involves digging through the ML history to find older versions, followed
> by the "git am" steps. Often submitters are nice enough to put links to
> previous versions of their patch series in their cover letters, but the
> links are to a web-based email archive, from which it is even more
> awkward to grab and apply patches. So in practice I then go back to my
> email client and search my local archive for my copy of the same email
> that was referenced in the archive, and apply the patch from there.
> Finding comments about old versions of a patch series is nearly as much
> work.
In short:
We can ask every contributor, if the patch send to the mailing list
is available on a public Git-repo, and what the branch name is,
like _V2.. Does this makes sense ?

As an alternative, you can save the branches locally, after running
git-am once, just keep the branch.
[]

> 
> I did enjoy the variety of reviewing some patch series using Gerrit. It
> is nice that it tracks the evolution of a patch from version to version,
> and that the comments made on all versions of a patch are summarized in
> a single place. This makes it easier to avoid commenting on issues that
> other reviewers have already noted and easier to check that your own
> comments have been addressed by later versions of the patch. On the
> other hand, Gerrit seems strongly focused on individual patches rather
> than on patch series (which might not match our workflow so well), the
> UI is overwhelming (though I think one could get quite productive with
> it if one used it every day), and the notification emails come in blizzards.
> 
> Michael
> 
> [1] Disclaimer: I work for GitHub.
> 
I like Gerrit as well.
But it is less efficient to use, a WEB browser is slower (often), and
you need to use the mouse...
However, if you put your patches on Gerrit, and add the link in your cover-letter,
it may be worth a trial.

But there is another thing:
Once a patch is send out, I would ask the sender to wait and collect comments
at least 24 hours before sending a V2.
We all living in different time zones, so please let the world spin once.

My feeling is that a patch > 5 commits should have
a waiting time > 5 days, otherwise I start reviewing V1, then V2 comes,
then V3 before I am finished with V1. That is not ideal.

What does it cost to push your branch to a public repo and
include that information in the email ?

And how feasable/nice/useful is it to ask contributers for a wait
time between re-rolling ?
 

Re: Our cumbersome mailing list workflow

[Matthieu Moy]

Torsten Bögershausen <tboegi@web.de> writes:

> On 2014-11-25 01.28, Michael Haggerty wrote:
> []
>> Let me list the aspects of our mailing list workflow that I find
>> cumbersome as a contributor and reviewer:
>> 
>> * Submitting patches to the mailing list is an ordeal of configuring
>> format-patch and send-email and getting everything just right, using
>> instructions that depend on the local environment.
> Typically everything fits into ~/.gitconfig,
> which can be carried around on a USB-Stick.

I personnally submit all my Git patches from a machine whose
/usr/sbin/sendmail knows how to send emails, so for me configuration is
super simple. But I can imagine the pain of someone working on various
machines with various network configuration and normally using a webmail
to send emails. Sharing ~/.gitconfig does not always work because on
machine A you only can use one SMTP server, and on machine B only
another ...

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: Our cumbersome mailing list workflow

[Eric Wong]

Torsten Bögershausen <tboegi@web.de> wrote:
> On 2014-11-25 01.28, Michael Haggerty wrote:
> >   * Or I save the emails to a temporary directory (awkward because, Oh
> > Horror, I use Thunderbird and not mutt as email client), hope that I've
> > guessed the right place to apply them, run "git am", and later try to
> > remember to clean up the temporary directory.
> 
> Is there a "mutt howto" somewhere?

Not that I'm aware of, but Documentation/email-clients.txt in
the Linux kernel has some short notes...

My muttrc has had the following since my early days as a git user:

  macro index A ":unset pipe_decode\n|git am -3\n:set pipe_decode\n"
  macro pager A ":unset pipe_decode\n|git am -3\n:set pipe_decode\n"

(Hit Shift-A while viewing/selecting a message to apply a patch,
 it requires you run mutt in your project working directory, though).

Perhaps there can be a similar document or reference to it in our
Documentation/

> In short:
> We can ask every contributor, if the patch send to the mailing list
> is available on a public Git-repo, and what the branch name is,
> like _V2.. Does this makes sense ?

Not unreasonable.  I hope that won't give folks an excuse to refuse
to mail patches, though.  Some folks read email offline and can't
fetch repos until they're online again.

> I like Gerrit as well.
> But it is less efficient to use, a WEB browser is slower (often), and
> you need to use the mouse...

IMNSHO, development of non-graphical software should never depend on
graphical software.  Also, I guess there is no way to comment on Gerrit
via email (without registration/logins?).

Lately, I've been trying to think of ways to make collaboration less
centralized.  Moving to more centralized collaboration tools is a step
back for decentralized VCS.

> But there is another thing:
> Once a patch is send out, I would ask the sender to wait and collect comments
> at least 24 hours before sending a V2.
> We all living in different time zones, so please let the world spin once.
> 
> My feeling is that a patch > 5 commits should have
> a waiting time > 5 days, otherwise I start reviewing V1, then V2 comes,
> then V3 before I am finished with V1. That is not ideal.
> 
> What does it cost to push your branch to a public repo and
> include that information in the email ?
> 
> And how feasable/nice/useful is it to ask contributers for a wait
> time between re-rolling ?

All that sounds good.

Re: Our cumbersome mailing list workflow

[Philip Oakley]

From: "Matthieu Moy" <Matthieu.Moy@grenoble-inp.fr>
> Torsten Bögershausen <tboegi@web.de> writes:
>
>> On 2014-11-25 01.28, Michael Haggerty wrote:
>> []
>>> Let me list the aspects of our mailing list workflow that I find
>>> cumbersome as a contributor and reviewer:
>>>
>>> * Submitting patches to the mailing list is an ordeal of configuring
>>> format-patch and send-email and getting everything just right, using
>>> instructions that depend on the local environment.
>> Typically everything fits into ~/.gitconfig,
>> which can be carried around on a USB-Stick.
>
> I personnally submit all my Git patches from a machine whose
> /usr/sbin/sendmail knows how to send emails, so for me configuration 
> is
> super simple. But I can imagine the pain of someone working on various
> machines with various network configuration and normally using a 
> webmail
> to send emails. Sharing ~/.gitconfig does not always work because on
> machine A you only can use one SMTP server, and on machine B only
> another ...

The bit I find awkward for the send-email step is the creation of the 
"to" and "cc" lists. I tend to create the command line in a separate 
file so that I can re-use it for V2 etc. and even then I end up with all 
patches going to the full to/cc list.

Michael's original discussion email did feel to summarise the isses [1] 
well.

--
Philip
[1] System Problems are Wicked problems :
http://en.wikipedia.org/wiki/Wicked_problem
www.poppendieck.com/wicked.htm

Re: Our cumbersome mailing list workflow

[Michael Haggerty]

On 11/27/2014 06:46 PM, Torsten Bögershausen wrote:
> On 2014-11-25 01.28, Michael Haggerty wrote:
> []
>> Let me list the aspects of our mailing list workflow that I find
>> cumbersome as a contributor and reviewer:
>>
>> * Submitting patches to the mailing list is an ordeal of configuring
>> format-patch and send-email and getting everything just right, using
>> instructions that depend on the local environment.
> Typically everything fits into ~/.gitconfig,
> which can be carried around on a USB-Stick.
> Is there any details which I miss, or howtows we can improve ?

I used to need one setup at work and a different one at home (because of
how my email was configured), and sometimes had to switch back and forth
as I carried my notebook around.

>> [...]
>>   * I do "git fetch gitster", then try to figure out whether the branch
>> I'm interested in is present, what its name is, and whether the version
>> in your tree is the latest version, then "git checkout xy/foobar".
> There are 12 branches from mh/, so it should be possible to find the name,
> und run git log gitster/xy/fix_this_bug or so.
> Even more important, this branch is the "single point of truth", because
> this branch may be merged eventually, and nothing else.

I know it's *possible*. The question is whether it could be made easier.

>> * Following patch series across iterations is also awkward. To compare
>> two versions, I have to first get both patch series into my repo, which
>> involves digging through the ML history to find older versions, followed
>> by the "git am" steps. Often submitters are nice enough to put links to
>> previous versions of their patch series in their cover letters, but the
>> links are to a web-based email archive, from which it is even more
>> awkward to grab and apply patches. So in practice I then go back to my
>> email client and search my local archive for my copy of the same email
>> that was referenced in the archive, and apply the patch from there.
>> Finding comments about old versions of a patch series is nearly as much
>> work.
> In short:
> We can ask every contributor, if the patch send to the mailing list
> is available on a public Git-repo, and what the branch name is,
> like _V2.. Does this makes sense ?

That would be helpful, but it would put yet *another* requirement on the
submitter (to send patch emails *and* push the branch to some accessible
repository). We regulars could script this pretty easily, but people who
only contribute occasionally or who are trying to get started will be
even more overwhelmed.

> As an alternative, you can save the branches locally, after running
> git-am once, just keep the branch.
> []

Yes, but it is even more unnecessary manual bookkeeping.

> [...]
> But there is another thing:
> Once a patch is send out, I would ask the sender to wait and collect comments
> at least 24 hours before sending a V2.
> We all living in different time zones, so please let the world spin once.

Yes, good idea.

> My feeling is that a patch > 5 commits should have
> a waiting time > 5 days, otherwise I start reviewing V1, then V2 comes,
> then V3 before I am finished with V1. That is not ideal.

One day per patch might be exaggerated, but I agree that long series
should be iterated more slowly than short ones.

> What does it cost to push your branch to a public repo and
> include that information in the email ?

One has to run an additional command and add some information to the
cover letter, every time a patch series is submitted. If it's scripted
then it's relatively painless. But for a newcomer these will be manual
steps that are easy to forget or to do incorrectly, making it more
likely that the newcomer's first contribution to Git will end in mild
embarrassment rather than success.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu

Re: Our cumbersome mailing list workflow

[Michael Haggerty]

On 11/27/2014 11:53 PM, Eric Wong wrote:
> Torsten Bögershausen <tboegi@web.de> wrote:
>> On 2014-11-25 01.28, Michael Haggerty wrote:
>>> [...]
>> In short:
>> We can ask every contributor, if the patch send to the mailing list
>> is available on a public Git-repo, and what the branch name is,
>> like _V2.. Does this makes sense ?
> 
> Not unreasonable.  I hope that won't give folks an excuse to refuse
> to mail patches, though.  Some folks read email offline and can't
> fetch repos until they're online again.

My ideal would be to invert the procedure. Let the patches in a public
Git repository somewhere be the primary artifact, and let the review
process be focused there. Let email be an alternative interface to the
central review site:

* Generate patch emails (similar to the current format) when pull
requests are submitted.

* Generate notification emails when people comment on the patches.

* Allow people to respond to the patch and notification emails via
email. The central review site should associate those comments with the
patches that they apply to, and present them along with other review
comments received via other interfaces.

>> I like Gerrit as well.
>> But it is less efficient to use, a WEB browser is slower (often), and
>> you need to use the mouse...
> 
> IMNSHO, development of non-graphical software should never depend on
> graphical software.  Also, I guess there is no way to comment on Gerrit
> via email (without registration/logins?).

The days of the vt52 are over. I'm an old neckbeard myself and have used
*real* vt52s. But these days even my *cellphone* is able to handle the
GitHub website [1]. Rejecting modern technology is not intrinsically
virtuous; it only makes sense if the old technology is really superior.
And it is not enough for it to be superior only for neckbeards; it
should be superior when averaged over all of the people whose
participation we would like to have in the Git project.

And by the way, there are text-only clients for interacting with GitHub [1].

> Lately, I've been trying to think of ways to make collaboration less
> centralized.  Moving to more centralized collaboration tools is a step
> back for decentralized VCS.

If an efficient decentralized collaboration system existed, then I'd
love to give it a chance. But as far as I know, the existing systems are
all embryonic.

Don't forget that even our current system is centralized to some extent.
There is a single mailing list through which all emails pass. There are
a few email archives that we de facto rely on (and it is a brittle
dependency--if Gmane were to disappear, we would have an awful lot of
broken URLs in our emails that would be impossible to fix).

It seems like a few desirable features are being talked about here, and
summarizing the discussion as "centralized" vs "decentralized" is too
simplistic. What is really important?

1. Convenient and efficient, including for newcomers
2. Usable while offline
3. Usable in pure-text mode
4. Decentralized

Something else?

In my opinion, a central system with good Git integration (helps with 1)
and both a straightforward web UI (also helps 1) and a good email
interface (which gives both 2 and 3) and the ability to export the
review history (which avoids lockin, the most important aspect of 4)
would be perfect. Is there such a thing?

Michael

[1] ...probably other websites too. I'm really not trying to flog GitHub
here; it's just the one I have the most experience with. In fact, I
kindof assume that the Git project would choose a service that is itself
based on open-source software.

-- 
Michael Haggerty
mhagger@alum.mit.edu

Re: Our cumbersome mailing list workflow

[Marc Branchaud]

On 14-11-28 09:31 AM, Michael Haggerty wrote:
> On 11/27/2014 06:46 PM, Torsten Bögershausen wrote:
>> On 2014-11-25 01.28, Michael Haggerty wrote:
>> []
>>> Let me list the aspects of our mailing list workflow that I find
>>> cumbersome as a contributor and reviewer:
>>>
>>> * Submitting patches to the mailing list is an ordeal of configuring
>>> format-patch and send-email and getting everything just right, using
>>> instructions that depend on the local environment.
>> Typically everything fits into ~/.gitconfig,
>> which can be carried around on a USB-Stick.
>> Is there any details which I miss, or howtows we can improve ?
> 
> I used to need one setup at work and a different one at home (because of
> how my email was configured), and sometimes had to switch back and forth
> as I carried my notebook around.
> 
>>> [...]
>>>   * I do "git fetch gitster", then try to figure out whether the branch
>>> I'm interested in is present, what its name is, and whether the version
>>> in your tree is the latest version, then "git checkout xy/foobar".
>> There are 12 branches from mh/, so it should be possible to find the name,
>> und run git log gitster/xy/fix_this_bug or so.
>> Even more important, this branch is the "single point of truth", because
>> this branch may be merged eventually, and nothing else.
> 
> I know it's *possible*. The question is whether it could be made easier.
> 
>>> * Following patch series across iterations is also awkward. To compare
>>> two versions, I have to first get both patch series into my repo, which
>>> involves digging through the ML history to find older versions, followed
>>> by the "git am" steps. Often submitters are nice enough to put links to
>>> previous versions of their patch series in their cover letters, but the
>>> links are to a web-based email archive, from which it is even more
>>> awkward to grab and apply patches. So in practice I then go back to my
>>> email client and search my local archive for my copy of the same email
>>> that was referenced in the archive, and apply the patch from there.
>>> Finding comments about old versions of a patch series is nearly as much
>>> work.
>> In short:
>> We can ask every contributor, if the patch send to the mailing list
>> is available on a public Git-repo, and what the branch name is,
>> like _V2.. Does this makes sense ?
> 
> That would be helpful, but it would put yet *another* requirement on the
> submitter (to send patch emails *and* push the branch to some accessible
> repository). We regulars could script this pretty easily, but people who
> only contribute occasionally or who are trying to get started will be
> even more overwhelmed.

A bot could subscribe to the list and create branches in a public repo.
(This idea feels familiar -- didn't somebody attempt this already?)

Integrate the bot into the list manager, and every PATCH email sent through
the list could have the patch's URL (maybe in the footer, or as an X- header).

Could this make a decent GSoC project?

		M.

Re: Our cumbersome mailing list workflow

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 2193 bytes --]

On Fri, Nov 28, 2014 at 04:34:09PM +0100, Michael Haggerty wrote:
> My ideal would be to invert the procedure. Let the patches in a public
> Git repository somewhere be the primary artifact, and let the review
> process be focused there. Let email be an alternative interface to the
> central review site:
> 
> * Generate patch emails (similar to the current format) when pull
> requests are submitted.
> 
> * Generate notification emails when people comment on the patches.
> 
> * Allow people to respond to the patch and notification emails via
> email. The central review site should associate those comments with the
> patches that they apply to, and present them along with other review
> comments received via other interfaces.

I think these are good goals.  Even as a semi-regular contributor, I
prefer to push branches around using Git rather than formatting patches
and mailing them.

Also, I think that being able to comment on a patch or report a bug
without a login (via email) is desirable.  I'm not a fan of having to
have an account on every Bugzilla on the planet.  That's why I like
debbugs.

> It seems like a few desirable features are being talked about here, and
> summarizing the discussion as "centralized" vs "decentralized" is too
> simplistic. What is really important?
> 
> 1. Convenient and efficient, including for newcomers
> 2. Usable while offline
> 3. Usable in pure-text mode
> 4. Decentralized

I think 1 is definitely important.  For me personally, 2 isn't very
important, as all my email is via IMAP (so I have to be online).  I
think 3 is important for accessibility reasons.  There are a lot of
blind or low-sighted people for whom a GUI is infeasible or burdensome.

> Something else?

It might be useful to have a system that has a bug or issue tracker.  We
often have posts to the mailing list that don't get a response, even
though those may represent legitimate bugs (code or documentation).
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: Our cumbersome mailing list workflow

[Damien Robert]

> A bot could subscribe to the list and create branches in a public repo.
> (This idea feels familiar -- didn't somebody attempt this already?)

Thomas Rast maintains git notes that link git commits to their gmane
discussion, you can get them with

[remote "mailnotes"]
  url = git://github.com/trast/git.git
  fetch = refs/heads/notes/*:refs/notes/*

There is gmane branch and a message-id branch, its pretty usefull.

Re: Our cumbersome mailing list workflow

[Junio C Hamano]

Michael Haggerty <mhagger@alum.mit.edu> writes:

> It seems like a few desirable features are being talked about here, and
> summarizing the discussion as "centralized" vs "decentralized" is too
> simplistic. What is really important?
>
> 1. Convenient and efficient, including for newcomers
> 2. Usable while offline
> 3. Usable in pure-text mode
> 4. Decentralized
>
> Something else?

As a reviewer / contributor (not speaking as the top maintainer), I
would say that everything in one place, and for that one place
mailbox is preferrable.

"Somebody commented on (this instance of | the central) Gerrit, come
look at it" is not usable; sending that comment out to those who
work in their MUA, and allowing them to respond via their MUA
probably adding their response as a new comment to Gerrit) would be
usable.

When I had to view a large-ish series by Ronnie on Gerrit, it was
fairly painful.  The interaction on an individual patch might be
more convenient and efficient using a system like Gerrit than via
e-mailed patch with reply messages, but as a vehicle to review a
large series and see how the whole thing fits together, I did not
find pages that made it usable (I am avoiding to say "I found it
unusable", as that impression may be purely from that I couldn't
find a more suitable pages that showed the same information in more
usable form, i.e. user inexperience).

Speaking of the "whole picture", I am hesitant to see us pushed into
the "here is a central system (or here are federated systems) to
handle only the patch reviews" direction; our changes result after
discussing unrelated features, wishes, or bugs that happen outside
of any specific patches with enough frequency, and that is why I
prefer "everything in one place" aspect of the development based on
the mailing list.  That is not to say that the "one place" has
forever to be the mailing list, though.  But the tooling around an
e-mail based workflow (e.g. marking threads as "worth revisiting"
for later inspection, saving chosen messages into a mailbox and
running "git am" on it) is already something I am used to.  Whatever
system we might end up migrating to, the convenience it offers has
to beat the convenience of existing workflow to be worth switching
to, at least to me as a reviewer/contributor.

As the maintainer, I am not worried too much.  As long as the
mechanism can (1) reach "here is a series that is accepted by
reviewers whose opinions are trusted" efficiently, and (2) allow
me to queue the result without mistakes, I can go along with
anything reasonable.

Re: Our cumbersome mailing list workflow

[Stefan Beller]

On Sun, Nov 30, 2014 at 6:46 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
>
>> It seems like a few desirable features are being talked about here, and
>> summarizing the discussion as "centralized" vs "decentralized" is too
>> simplistic. What is really important?
>>
>> 1. Convenient and efficient, including for newcomers
>> 2. Usable while offline
>> 3. Usable in pure-text mode
>> 4. Decentralized
>>
>> Something else?
>

So when I started overtaking the ref log series by Ronnie,
Ronnies main concern was missing reviewers time. So my idea was to
make it as accessible as possible, so the reviewing party can use their
time best. However here are a few points, I want to mention:

 * Having send emails as well as uploaded it to Gerrit, I either needed
   a ChangeId (Gerrit strictly requires them to track inter-patch
diffs), and the
   mailing list here strictly avoids them, so I was told.
   Ok, that's my problem as I wasn't following the actual procedure of the
   Git development model (mailing list only).
 * That's why I stopped uploads to Gerrit, so I do not need to care about the
   ChangeIds any more. I am not sure if that improved the quality of my patches
   though.
 * I seem to not have found the right workflow with the mailing list yet, as I
   personally find copying around the inter-patch changelog very inconvenient.
   Most of the regulars here just need fewer iterations, so I can understand,
   that you find it less annoying. Hopefully I'll also get used to the
nit-picky things
   and will require less review iterations in the future.
   How are non-regulars/newcomers, who supposingly need more iterations on
   a patch,  supposed to handle the inter patch change log conveniently?
   I tried to keep the inter patch changelog be part of the commit message and
   then just before sending the email, I'd move it the non-permanent section of
   the email.
 * Editing patches as text files is hard/annoying. I have setup git send-email,
   and that works awesome, as I'd only need one command to send off a series.
   Having a step in between makes it more error-prone. So I do git format-patch
   and then inject the inter patch change log, check to remove ChangeId and then
   use git send-email. And at that final manual step I realized I am
far from being
   perfect, so sometimes patches arrive on the mailing list, which are
sub quality
   in the sense, that there are leftovers, i.e. a ChangeId
 * A possible feature, which just comes to my mind:
   Would it make sense for format-patch to not just show the diff
stats, but also
   include, on which branch it applies? In git.git this is usually the
origin/master
   branch, but dealing with patch series, building on top of each other that may
   be a good feature to have.

>
> When I had to view a large-ish series by Ronnie on Gerrit, it was
> fairly painful.  The interaction on an individual patch might be
> more convenient and efficient using a system like Gerrit than via
> e-mailed patch with reply messages, but as a vehicle to review a
> large series and see how the whole thing fits together, I did not
> find pages that made it usable (I am avoiding to say "I found it
> unusable", as that impression may be purely from that I couldn't
> find a more suitable pages that showed the same information in more
> usable form, i.e. user inexperience).

So you're liking the email workflow more. How do you do the final
formatting of an email, such as including the inter patch diff?


>
> Speaking of the "whole picture", I am hesitant to see us pushed into
> the "here is a central system (or here are federated systems) to
> handle only the patch reviews" direction; our changes result after
> discussing unrelated features, wishes, or bugs that happen outside
> of any specific patches with enough frequency, and that is why I
> prefer "everything in one place" aspect of the development based on
> the mailing list.  That is not to say that the "one place" has
> forever to be the mailing list, though.  But the tooling around an
> e-mail based workflow (e.g. marking threads as "worth revisiting"
> for later inspection, saving chosen messages into a mailbox and
> running "git am" on it) is already something I am used to.  Whatever
> system we might end up migrating to, the convenience it offers has
> to beat the convenience of existing workflow to be worth switching
> to, at least to me as a reviewer/contributor.

I do like the way as well to just mark emails unread when I need
to work on them later.

>
> As the maintainer, I am not worried too much.  As long as the
> mechanism can (1) reach "here is a series that is accepted by
> reviewers whose opinions are trusted" efficiently, and (2) allow
> me to queue the result without mistakes, I can go along with
> anything reasonable.
>

Re: Our cumbersome mailing list workflow

[Jonathan Nieder]

Stefan Beller wrote:

> How are non-regulars/newcomers, who supposingly need more iterations on
> a patch, supposed to handle the inter patch change log conveniently?

I think this is one of the more important issues.

I don't think there's any reason that newcomers should need more
iterations than regulars to finish a patch.  Regulars are actually
held to a higher standard, so they are likely to need more iterations.

A common mistake for newcomers, that I haven't learned yet how to warn
properly against, is to keep re-sending minor iterations on a patch
too quickly.  Some ways to avoid that:

 * feel free to respond to review comments with something like "how
   about this?" and a copy/pasted block of code that just addresses
   that one comment.  That way, you can clear up ambiguity and avoid
   the work of applying that change to the entire patch if it ends
   up seeming like a bad idea.  This also avoids having to re-send a
   larger patch or series multiple times to clear up a small ambiguity
   from a review.

 * be proactive.  Look for other examples of the same issue that a
   reviewer pointed out once so they don't have to find it again
   elsewhere in the next iteration.  Run the testsuite.  Build with
   the flags from
   https://kernel.googlesource.com/pub/scm/git/git/+/todo/Make#106
   in CFLAGS in config.mak.  Proofread and try to read as though you
   knew nothing about the patch to anticipate what reviewers will
   find.

 * feel free to get more review out-of-band, too.  If you're still
   playing with ideas and want someone to take a quick glance before
   the patches are in reviewable form, you can do that and say so
   (e.g., with 'RFC/' before 'PATCH' in the subject line).

Jonathan

Re: Our cumbersome mailing list workflow

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> I don't think there's any reason that newcomers should need more
> iterations than regulars to finish a patch.  Regulars are actually
> held to a higher standard, so they are likely to need more iterations.
>
> A common mistake for newcomers, that I haven't learned yet how to warn
> properly against, is to keep re-sending minor iterations on a patch
> too quickly.  Some ways to avoid that:
>
>  * feel free to respond to review comments with something like "how
>    about this?" and a copy/pasted block of code that just addresses
>    that one comment.  That way, you can clear up ambiguity and avoid
>    the work of applying that change to the entire patch if it ends
>    up seeming like a bad idea.  This also avoids having to re-send a
>    larger patch or series multiple times to clear up a small ambiguity
>    from a review.

This can go both ways.  A trivial improvement can be suggested that
way by the reviewer.

>  * be proactive.  Look for other examples of the same issue that a
>    reviewer pointed out once so they don't have to find it again
>    elsewhere in the next iteration....
>  * feel free to get more review out-of-band, too.  If you're still
>    playing with ideas and want someone to take a quick glance before
>    the patches are in reviewable form, you can do that and say so
>    (e.g., with 'RFC/' before 'PATCH' in the subject line).

Overall, good suggestions.

Thanks.

Re: Our cumbersome mailing list workflow

[Torsten Bögershausen]

On 2014-12-03 03.20, Stefan Beller wrote:
> On Sun, Nov 30, 2014 at 6:46 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Michael Haggerty <mhagger@alum.mit.edu> writes:
>>
>>> It seems like a few desirable features are being talked about here, and
>>> summarizing the discussion as "centralized" vs "decentralized" is too
>>> simplistic. What is really important?
>>>
>>> 1. Convenient and efficient, including for newcomers
>>> 2. Usable while offline
>>> 3. Usable in pure-text mode
>>> 4. Decentralized
>>>
>>> Something else?
> So when I started overtaking the ref log series by Ronnie,
> Ronnies main concern was missing reviewers time. So my idea was to
> make it as accessible as possible, so the reviewing party can use their
> time best. However here are a few points, I want to mention:
>
>  * Having send emails as well as uploaded it to Gerrit, I either needed
>    a ChangeId (Gerrit strictly requires them to track inter-patch
> diffs), and the
>    mailing list here strictly avoids them, so I was told.
>    Ok, that's my problem as I wasn't following the actual procedure of the
>    Git development model (mailing list only).
>  * That's why I stopped uploads to Gerrit, so I do not need to care about the
>    ChangeIds any more. I am not sure if that improved the quality of my patches
>    though.
>  * I seem to not have found the right workflow with the mailing list yet, as I
>    personally find copying around the inter-patch changelog very inconvenient.
>    Most of the regulars here just need fewer iterations, so I can understand,
>    that you find it less annoying. Hopefully I'll also get used to the
> nit-picky things
>    and will require less review iterations in the future.
>    How are non-regulars/newcomers, who supposingly need more iterations on
>    a patch,  supposed to handle the inter patch change log conveniently?
>    I tried to keep the inter patch changelog be part of the commit message and
>    then just before sending the email, I'd move it the non-permanent section of
>    the email.
>  * Editing patches as text files is hard/annoying.
Not sure if I understand. Editing text files isn't that hard, we do it all the time.
>  I have setup git send-email,
>    and that works awesome, as I'd only need one command to send off a series.
>    Having a step in between makes it more error-prone. So I do git format-patch
>    and then inject the inter patch change log, check to remove ChangeId and then
>    use git send-email.
How do you "inject the inter patch change log" ? Is that manually, or is it a script ?
>  And at that final manual step I realized I am
> far from being
>    perfect, so sometimes patches arrive on the mailing list, which are
> sub quality
>    in the sense, that there are leftovers, i.e. a ChangeId
>  * A possible feature, which just comes to my mind:
>    Would it make sense for format-patch to not just show the diff
> stats, but also
>    include, on which branch it applies? In git.git this is usually the
> origin/master
>    branch, but dealing with patch series, building on top of each other that may
>    be a good feature to have.
>

Thanks for the description (and everybody for the discussion)
In the hope that it may help, I can try to describe my work flow:
- Run a script to send the patch (this is a real example)
#################

SRCCOMMIT=119efe90bffee688a3c37d4358667
DSTCOMMIT=$(git log --oneline -n1 | awk '{print $1}')
VERSION="-v 1"

PATCHFILE=$( echo $0 | sed -e 's/\.sh$/.patch/')
GIT_TEST_LONG=t
export GIT_TEST_LONG
git am --abort || :
(  test -s $PATCHFILE || 
	git format-patch $VERSION -s --to=git@vger.kernel.org  --cc=tboegi@web.de  --cc=mhagger@alum.mit.edu --stdout $SRCCOMMIT..$DSTCOMMIT >$PATCHFILE ) &&
git checkout $SRCCOMMIT &&
git am <$PATCHFILE &&
cd t && cd .. && make &&
(cd t && ./t0001*.sh) &&
git imap-send <$PATCHFILE

#####################
The script formats a patch file (if that does not exist),
applies the patch on the source commit,
runs make and then the test cases to verify that the patch works.
(For bigger patches more tests or the whole test suite should be run,
for this very isolated work it OK to run a singe test)

Once everything is OK, the patch is stored both on disc and in the Drafts folder of the "email program".
(In your case you can use grep to remove the ChangedId or to check that it had been removed)

Now it is time to "tweak" the patch file with an editor:
Add what has been changed  since V1....
Save the patch file, run the script again to verify that the patch still applies and works and
put it into the Drafts folder of the mail program.

(That's why I abort the "git imap-send" in the first round
and press ^C when the password is asked)

Start the favorite email program
(Kmail works, or Thunderbird or 
 every other program that can send email in "plain text")

Have a final look at the patch in the email prgram
(remove the V1 from the header, change PATCH into PATCH/RFC).

Let the spell checker look at it, re-read once more.
If everything is OK, press the "send" button.

If I send out a V2 version, make a copy of the script, and call it doit2.sh,
change what needs to be changed.
We can enhance the script to push to a global repo, create a new branch just to
be sure we re-find our work...

I store all these scripts under a folder in my home directory,
each script has it's own directory, this for example is under
141119_check_file_mode_for_SAMBA/.
And if I am afraid that I don't know where it ended,
I can make a comment file here and notice that Junio picked it up here:
junio/tb/config-core-filemode-check-on-broken-fs
(And the remote junio is "git://github.com/gitster/git.git")

The good thing is that both the script and the patch file can be put
under version control.


I realized that re-checking the email which is rally send out to the list
is worth the time and effort.
Sometimes I keep it in the Drafts folder over night, and have
a new look with fresh eyes the next day.

Re: Our cumbersome mailing list workflow

[Philip Oakley]

From: "Michael Haggerty" <mhagger@alum.mit.edu>
Sent: Tuesday, November 25, 2014 12:28 AM
> On 11/21/2014 07:00 PM, Junio C Hamano wrote:
>> Michael Haggerty <mhagger@alum.mit.edu> writes:
>>
>>> I don't think that those iterations changed anything substantial 
>>> that
>>> overlaps with my version, but TBH it's such a pain in the ass 
>>> working
>>> with patches in email that I don't think I'll go to the effort of
>>> checking for sure unless somebody shows interest in actually using 
>>> my
>>> version.
>>>
>>> Sorry for being grumpy today :-(
[..]
> Let me list the aspects of our mailing list workflow that I find
> cumbersome as a contributor and reviewer:
>
> * Submitting patches to the mailing list is an ordeal of configuring
> format-patch and send-email and getting everything just right, using
> instructions that depend on the local environment. We saw that hardly
> any GSoC applicants were able to get it right on their first attempt.
> Submitting a patch series should be as simple as "git push".
>
> * Once patches are submitted, there is no assurance that you (Junio)
> will apply them to your tree at the same point that the submitter
> developed and tested them.
>
> * The branch name that you choose for a patch series is not easily
> derivable from the patches as they appeared in the mailing list. 
> Trying
> to figure out whether/where the patches exist in your tree is a 
> largely
> manual task. The reverse mapping, from in-tree commit to the email 
> where
> it was proposed, is even more difficult to infer.
>
> * Your tree has no indication of which version of a patch series (v1,
> v2, etc) is currently applied.
>
> The previous three points combine to make it awkward to get patches 
> into
> my local repository to review or test. There are two alternatives, 
> both
> cumbersome and imprecise:
>
>  * I do "git fetch gitster", then try to figure out whether the branch
> I'm interested in is present, what its name is, and whether the 
> version
> in your tree is the latest version, then "git checkout xy/foobar".
>

I had a thought about the issue of version labeling and of keeping the 
old patch series hanging about during development that I felt was worth 
recording.

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.

Thus the series branch would appear to have an extra commit (compared to 
the current process) after the original tip's possible merge into say 
pu.

When subsequent series are sent to the list, the new supplemental commit 
would be a 'merge', with its second parent being the old series, thus 
the old series is not lost until the branch is deleted, and the existing 
merge pattern is retained.

Clearly if this would need some additional coding as it's not suitable 
as a manual process, but it could be just as automatic as the current 
process while providing that little bit of additional visibility.

Below, I've tried to set out how the commit graph might look (oldest to 
the left). Hopefully my MUA won't ruin it.
The first patch series branches at A, and is merged at D, with the 
supplemental commit labeled with v1z.

When the new series arrives, and pu is rewound, we have the new series 
applied from G (which in reality may not be linked directly from A), and 
merged back at K. However the new v2z supplemental commit is now the 
po/patches
branch head, and is also a merge back to v1z.

patch series 1 (cover letter z)
- A - B - C - D - E - F   <- pu
   \        /
    v1a-v1b--v1z     <-po/patches

patch series 2
- A - G - H - I - J - K     <- pu (note re-wound)
  |        \         /            (merge D lost)
   \       v2a-v2b-v2c--v2z    <-po/patches
    \                  /
    v1a-v1b--v1z - - -.

The key idea here is to use the existing branching model, but then to 
add the cover letter and other details at the end, rather than the 
beginning as might have been expected from the email transmit sequence.

Philip

Re: Our cumbersome mailing list workflow

[Stefan Beller]

>  Editing text files isn't that hard, we do it all the time.

It is not indeed. But doing it all over again and again is hard and error prone.
I did re-read the man page on git format-patch and found the --notes
option, which I am going to try
to use in my workflow. That way I only need to update the notes
instead of redoing them all the time.
By redoing it I mean copying the changelog from the last time I sent
the patch and adding new entries.

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

This sounds interesting. The only changes I can see here are the
referenced message ids, so it would be worthwhile to have the last
patch sent out first and all other patches 1..n-1 referencing the last
empty commit.
If additionally the numbering is corrected, the reader of the mailing
list would not notice any difference to the status quo, just the
sender would have the convenience to be able to track the cover letter
as an empty commit on top of a series.