Re: [PATCH] refs.c: use a stringlist for repack_without_refs
Stefan Beller sbel...@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 sahlb...@google.com
Signed-off-by: Stefan Beller sbel...@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...
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] refs.c: use a stringlist for repack_without_refs
From: Ronnie Sahlberg sahlb...@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 sahlb...@google.com
Signed-off-by: Stefan Beller sbel...@google.com
---
On Wed, Nov 19, 2014 at 10:00 AM, Junio C Hamano gits...@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/));
}
-
[PATCH] refs.c: use a stringlist for repack_without_refs
From: Ronnie Sahlberg sahlb...@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 sahlb...@google.com
Signed-off-by: Stefan Beller sbel...@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,
[PATCH] refs.c: use a stringlist for repack_without_refs
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 sahlb...@google.com
Signed-off-by: Stefan Beller sbel...@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
Re: [PATCH] refs.c: use a stringlist for repack_without_refs
Stefan Beller sbel...@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 sahlb...@google.com
Signed-off-by: Stefan Beller sbel...@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
Re: [PATCH] refs.c: use a stringlist for repack_without_refs
Junio C Hamano gits...@pobox.com writes: Stefan Beller sbel...@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 sahlb...@google.com Signed-off-by: Stefan Beller sbel...@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. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] refs.c: use a stringlist for repack_without_refs
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 sahlb...@google.com
Signed-off-by: Stefan Beller sbel...@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 @@
Re: [PATCH] refs.c: use a stringlist for repack_without_refs
On Tue, Nov 18, 2014 at 3:45 PM, Jonathan Nieder jrnie...@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 sahlb...@google.com
Signed-off-by: Stefan Beller sbel...@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
[PATCH] refs.c: use a stringlist for repack_without_refs
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 sahlb...@google.com
Signed-off-by: Stefan Beller sbel...@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
