[PATCH 00/15] Rearrange xcalloc arguments

xcalloc takes two arguments: the number of elements and their size.
The vast majority of the Git codebase passes these arguments in the
correct order, but there are some exceptions. This patch series
corrects those exceptions.

Brian Gesiak (15):
  builtin/add.c: rearrange xcalloc arguments
  builtin/ls-remote.c: rearrange xcalloc arguments
  builtin/remote.c: rearrange xcalloc arguments
  commit.c: rearrange xcalloc arguments
  config.c: rearrange xcalloc arguments
  diff.c: rearrange xcalloc arguments
  hash.c: rearrange xcalloc arguments
  hash.h: rearrange xcalloc arguments
  http-push.c: rearrange xcalloc arguments
  imap-send.c: rearrange xcalloc arguments
  notes.c: rearrange xcalloc arguments
  pack-revindex.c: rearrange xcalloc arguments
  reflog-walk.c: rearrange xcalloc arguments
  remote.c: rearrange xcalloc arguments
  transport-helper.c: rearrange xcalloc arguments

builtin/add.c       | 2 +-
builtin/ls-remote.c | 2 +-
builtin/remote.c    | 8 ++++----
commit.c            | 2 +-
config.c            | 4 ++--
diff.c              | 2 +-
hash.c              | 2 +-
hash.h              | 2 +-
http-push.c         | 2 +-
imap-send.c         | 2 +-
notes.c             | 6 +++---
pack-revindex.c     | 2 +-
reflog-walk.c       | 8 ++++----
remote.c            | 2 +-
transport-helper.c  | 2 +-
15 files changed, 24 insertions(+), 24 deletions(-)

-- 
2.0.0.rc1.543.gc8042da

xcalloc takes two arguments: the number of elements and their size.
cmd_ls_remote passes the arguments in reverse order, passing the
size of a char*, followed by the number of char* to be allocated.
Rearrgange them so they are in the correct order.

Signed-off-by: Brian Gesiak <redacted>
---
 builtin/ls-remote.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c
index 39e5144..aec1c0c 100644
--- a/builtin/ls-remote.c
+++ b/builtin/ls-remote.c

@@ -92,7 +92,7 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 
 	if (argv[i]) {
 		int j;
-		pattern = xcalloc(sizeof(const char *), argc - i + 1);
+		pattern = xcalloc(argc - i + 1, sizeof(const char *));
 		for (j = i; j < argc; j++) {
 			int len = strlen(argv[j]);
 			char *p = xmalloc(len + 3);

-- 
2.0.0.rc1.543.gc8042da

xcalloc takes two arguments: the number of elements and their size.
run_add_interactive passes the arguments in reverse order, passing the
size of a char*, followed by the number of char* to be allocated.
Rearrgange them so they are in the correct order.

Signed-off-by: Brian Gesiak <redacted>
---
 builtin/add.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/add.c b/builtin/add.c
index 672adc0..488acf4 100644
--- a/builtin/add.c
+++ b/builtin/add.c

@@ -248,7 +248,7 @@ int run_add_interactive(const char *revision, const char *patch_mode,
 	int status, ac, i;
 	const char **args;
 
-	args = xcalloc(sizeof(const char *), (pathspec->nr + 6));
+	args = xcalloc((pathspec->nr + 6), sizeof(const char *));
 	ac = 0;
 	args[ac++] = "add--interactive";
 	if (patch_mode)

-- 
2.0.0.rc1.543.gc8042da

xcalloc takes two arguments: the number of elements and their size.
builtin/remote.c includes several calls to xcalloc that pass the
arguments in reverse order. Rearrgange them so they are in the
correct order.

Signed-off-by: Brian Gesiak <redacted>
---
 builtin/remote.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/builtin/remote.c b/builtin/remote.c
index b3ab4cf..9f62021 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c

@@ -282,7 +282,7 @@ static int config_read_branches(const char *key, const char *value, void *cb)
 		item = string_list_insert(&branch_list, name);
 
 		if (!item->util)
-			item->util = xcalloc(sizeof(struct branch_info), 1);
+			item->util = xcalloc(1, sizeof(struct branch_info));
 		info = item->util;
 		if (type == REMOTE) {
 			if (info->remote_name)

@@ -398,7 +398,7 @@ static int get_push_ref_states(const struct ref *remote_refs,
 
 		item = string_list_append(&states->push,
 					  abbrev_branch(ref->peer_ref->name));
-		item->util = xcalloc(sizeof(struct push_info), 1);
+		item->util = xcalloc(1, sizeof(struct push_info));
 		info = item->util;
 		info->forced = ref->force;
 		info->dest = xstrdup(abbrev_branch(ref->name));

@@ -433,7 +433,7 @@ static int get_push_ref_states_noquery(struct ref_states *states)
 	states->push.strdup_strings = 1;
 	if (!remote->push_refspec_nr) {
 		item = string_list_append(&states->push, _("(matching)"));
-		info = item->util = xcalloc(sizeof(struct push_info), 1);
+		info = item->util = xcalloc(1, sizeof(struct push_info));
 		info->status = PUSH_STATUS_NOTQUERIED;
 		info->dest = xstrdup(item->string);
 	}

@@ -446,7 +446,7 @@ static int get_push_ref_states_noquery(struct ref_states *states)
 		else
 			item = string_list_append(&states->push, _("(delete)"));
 
-		info = item->util = xcalloc(sizeof(struct push_info), 1);
+		info = item->util = xcalloc(1, sizeof(struct push_info));
 		info->forced = spec->force;
 		info->status = PUSH_STATUS_NOTQUERIED;
 		info->dest = xstrdup(spec->dst ? spec->dst : item->string);

-- 
2.0.0.rc1.543.gc8042da

xcalloc takes two arguments: the number of elements and their size.
grow_hash_table passes the arguments in reverse order, passing the
size of a hash table entry, followed by the number of entries.
Rearrgange them so they are in the correct order.

Signed-off-by: Brian Gesiak <redacted>
---
 hash.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hash.c b/hash.c
index 749ecfe..2067be9 100644
--- a/hash.c
+++ b/hash.c

@@ -53,7 +53,7 @@ static void grow_hash_table(struct hash_table *table)
 	struct hash_table_entry *old_array = table->array, *new_array;
 
 	new_size = alloc_nr(old_size);
-	new_array = xcalloc(sizeof(struct hash_table_entry), new_size);
+	new_array = xcalloc(new_size, sizeof(struct hash_table_entry));
 	table->size = new_size;
 	table->array = new_array;
 	table->nr = 0;

-- 
2.0.0.rc1.543.gc8042da

xcalloc takes two arguments: the number of elements and their size.
diffstat_add passes the arguments in reverse order, passing the
size of a diffstat_file*, followed by the number of diffstat_file* to
be allocated. Rearrgange them so they are in the correct order.

Signed-off-by: Brian Gesiak <redacted>
---
 diff.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/diff.c b/diff.c
index 635dee2..a71dfde 100644
--- a/diff.c
+++ b/diff.c

@@ -1360,7 +1360,7 @@ static struct diffstat_file *diffstat_add(struct diffstat_t *diffstat,
 					  const char *name_b)
 {
 	struct diffstat_file *x;
-	x = xcalloc(sizeof (*x), 1);
+	x = xcalloc(1, sizeof(*x));
 	if (diffstat->nr == diffstat->alloc) {
 		diffstat->alloc = alloc_nr(diffstat->alloc);
 		diffstat->files = xrealloc(diffstat->files,

-- 
2.0.0.rc1.543.gc8042da

xcalloc takes two arguments: the number of elements and their size.
imap_open_store passes the arguments in reverse order, passing the
size of an imap_store*, followed by the number to allocate.
Rearrgange them so they are in the correct order.

Signed-off-by: Brian Gesiak <redacted>
---
 imap-send.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/imap-send.c b/imap-send.c
index 0bc6f7f..45230e1 100644
--- a/imap-send.c
+++ b/imap-send.c

@@ -951,7 +951,7 @@ static struct imap_store *imap_open_store(struct imap_server_conf *srvc)
 	char *arg, *rsp;
 	int s = -1, preauth;
 
-	ctx = xcalloc(sizeof(*ctx), 1);
+	ctx = xcalloc(1, sizeof(*ctx));
 
 	ctx->imap = imap = xcalloc(sizeof(*imap), 1);
 	imap->buf.sock.fd[0] = imap->buf.sock.fd[1] = -1;

-- 
2.0.0.rc1.543.gc8042da

xcalloc takes two arguments: the number of elements and their size.
http-push passes the arguments in reverse order, passing the size
of a repo, followed by the number to allocate. Rearrgange them so
they are in the correct order.

Signed-off-by: Brian Gesiak <redacted>
---
 http-push.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/http-push.c b/http-push.c
index d4b40c9..1c722e5 100644
--- a/http-push.c
+++ b/http-push.c

@@ -1733,7 +1733,7 @@ int main(int argc, char **argv)
 
 	git_extract_argv0_path(argv[0]);
 
-	repo = xcalloc(sizeof(*repo), 1);
+	repo = xcalloc(1, sizeof(*repo));
 
 	argv++;
 	for (i = 1; i < argc; i++, argv++) {

-- 
2.0.0.rc1.543.gc8042da

xcalloc takes two arguments: the number of elements and their size.
reflog-walk.c includes several calls to xcalloc that pass the arguments
in reverse order. Rearrgange them so they are in the correct order.

Signed-off-by: Brian Gesiak <redacted>
---
 reflog-walk.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/reflog-walk.c b/reflog-walk.c
index b2fbdb2..f8d8f13 100644
--- a/reflog-walk.c
+++ b/reflog-walk.c

@@ -45,7 +45,7 @@ static int read_one_reflog(unsigned char *osha1, unsigned char *nsha1,
 static struct complete_reflogs *read_complete_reflog(const char *ref)
 {
 	struct complete_reflogs *reflogs =
-		xcalloc(sizeof(struct complete_reflogs), 1);
+		xcalloc(1, sizeof(struct complete_reflogs));
 	reflogs->ref = xstrdup(ref);
 	for_each_reflog_ent(ref, read_one_reflog, reflogs);
 	if (reflogs->nr == 0) {

@@ -143,7 +143,7 @@ struct reflog_walk_info {
 
 void init_reflog_walk(struct reflog_walk_info** info)
 {
-	*info = xcalloc(sizeof(struct reflog_walk_info), 1);
+	*info = xcalloc(1, sizeof(struct reflog_walk_info));
 }
 
 int add_reflog_for_walk(struct reflog_walk_info *info,

@@ -207,7 +207,7 @@ int add_reflog_for_walk(struct reflog_walk_info *info,
 			= reflogs;
 	}
 
-	commit_reflog = xcalloc(sizeof(struct commit_reflog), 1);
+	commit_reflog = xcalloc(1, sizeof(struct commit_reflog));
 	if (recno < 0) {
 		commit_reflog->recno = get_reflog_recno_by_time(reflogs, timestamp);
 		if (commit_reflog->recno < 0) {

@@ -250,7 +250,7 @@ void fake_reflog_parent(struct reflog_walk_info *info, struct commit *commit)
 		return;
 	}
 
-	commit->parents = xcalloc(sizeof(struct commit_list), 1);
+	commit->parents = xcalloc(1, sizeof(struct commit_list));
 	commit->parents->item = commit_info->commit;
 }
 

-- 
2.0.0.rc1.543.gc8042da

xcalloc takes two arguments: the number of elements and their size.
notes.c includes several calls to xcalloc that pass the arguments in
reverse order. Rearrgange them so they are in the correct order.

Signed-off-by: Brian Gesiak <redacted>
---
 notes.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/notes.c b/notes.c
index 5f07c0b..5fe691d 100644
--- a/notes.c
+++ b/notes.c

@@ -303,7 +303,7 @@ static int note_tree_insert(struct notes_tree *t, struct int_node *tree,
 		free(entry);
 		return 0;
 	}
-	new_node = (struct int_node *) xcalloc(sizeof(struct int_node), 1);
+	new_node = (struct int_node *) xcalloc(1, sizeof(struct int_node));
 	ret = note_tree_insert(t, new_node, n + 1, l, GET_PTR_TYPE(*p),
 			       combine_notes);
 	if (ret)

@@ -443,7 +443,7 @@ static void load_subtree(struct notes_tree *t, struct leaf_node *subtree,
 		if (len <= 20) {
 			type = PTR_TYPE_NOTE;
 			l = (struct leaf_node *)
-				xcalloc(sizeof(struct leaf_node), 1);
+				xcalloc(1, sizeof(struct leaf_node));
 			hashcpy(l->key_sha1, object_sha1);
 			hashcpy(l->val_sha1, entry.sha1);
 			if (len < 20) {

@@ -1003,7 +1003,7 @@ void init_notes(struct notes_tree *t, const char *notes_ref,
 	if (!combine_notes)
 		combine_notes = combine_notes_concatenate;
 
-	t->root = (struct int_node *) xcalloc(sizeof(struct int_node), 1);
+	t->root = (struct int_node *) xcalloc(1, sizeof(struct int_node));
 	t->first_non_note = NULL;
 	t->prev_non_note = NULL;
 	t->ref = notes_ref ? xstrdup(notes_ref) : NULL;

-- 
2.0.0.rc1.543.gc8042da

xcalloc takes two arguments: the number of elements and their size.
transport_helper_init passes the arguments in reverse order, passing the
size of a helper_data*, followed by the number to allocate. Rearrgange
them so they are in the correct order.

Signed-off-by: Brian Gesiak <redacted>
---
 transport-helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/transport-helper.c b/transport-helper.c
index ad72fbd..cf48913 100644
--- a/transport-helper.c
+++ b/transport-helper.c

@@ -1002,7 +1002,7 @@ static struct ref *get_refs_list(struct transport *transport, int for_push)
 
 int transport_helper_init(struct transport *transport, const char *name)
 {
-	struct helper_data *data = xcalloc(sizeof(*data), 1);
+	struct helper_data *data = xcalloc(1, sizeof(*data));
 	data->name = name;
 
 	if (getenv("GIT_TRANSPORT_HELPER_DEBUG"))

-- 
2.0.0.rc1.543.gc8042da

xcalloc takes two arguments: the number of elements and their size.
parse_refspec_internal passes the arguments in reverse order, passing the
size of a refspec, followed by the number to allocate. Rearrgange them
so they are in the correct order.

Signed-off-by: Brian Gesiak <redacted>
---
 remote.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/remote.c b/remote.c
index ebed40d..df3267b 100644
--- a/remote.c
+++ b/remote.c

@@ -523,7 +523,7 @@ static void free_refspecs(struct refspec *refspec, int nr_refspec)
 static struct refspec *parse_refspec_internal(int nr_refspec, const char **refspec, int fetch, int verify)
 {
 	int i;
-	struct refspec *rs = xcalloc(sizeof(*rs), nr_refspec);
+	struct refspec *rs = xcalloc(nr_refspec, sizeof(*rs));
 
 	for (i = 0; i < nr_refspec; i++) {
 		size_t llen;

-- 
2.0.0.rc1.543.gc8042da

xcalloc takes two arguments: the number of elements and their size.
init_pack_revindex passes the arguments in reverse order, passing the
size of a pack_revindex, followed by the number to allocate.
Rearrgange them so they are in the correct order.

Signed-off-by: Brian Gesiak <redacted>
---
 pack-revindex.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/pack-revindex.c b/pack-revindex.c
index b4d2b35..f84979b 100644
--- a/pack-revindex.c
+++ b/pack-revindex.c

@@ -50,7 +50,7 @@ static void init_pack_revindex(void)
 	if (!num)
 		return;
 	pack_revindex_hashsz = num * 11;
-	pack_revindex = xcalloc(sizeof(*pack_revindex), pack_revindex_hashsz);
+	pack_revindex = xcalloc(pack_revindex_hashsz, sizeof(*pack_revindex));
 	for (p = packed_git; p; p = p->next) {
 		num = pack_revindex_ix(p);
 		num = - 1 - num;

-- 
2.0.0.rc1.543.gc8042da

xcalloc takes two arguments: the number of elements and their size.
prellocate_hash passes the arguments in reverse order, passing the
size of a hash table entry, followed by the number of entries.
Rearrgange them so they are in the correct order.

Signed-off-by: Brian Gesiak <redacted>
---
 hash.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hash.h b/hash.h
index 1d43ac0..3b5d9e7 100644
--- a/hash.h
+++ b/hash.h

@@ -44,7 +44,7 @@ static inline void preallocate_hash(struct hash_table *table, unsigned int elts)
 {
 	assert(table->size == 0 && table->nr == 0 && table->array == NULL);
 	table->size = elts * 2;
-	table->array = xcalloc(sizeof(struct hash_table_entry), table->size);
+	table->array = xcalloc(table->size, sizeof(struct hash_table_entry));
 }
 
 #endif

-- 
2.0.0.rc1.543.gc8042da

xcalloc takes two arguments: the number of elements and their size.
reduce_heads passes the arguments in reverse order, passing the
size of a commit*, followed by the number of commit* to be allocated.
Rearrgange them so they are in the correct order.

Signed-off-by: Brian Gesiak <redacted>
---
 commit.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/commit.c b/commit.c
index 6bf4fe0..0fe685f 100644
--- a/commit.c
+++ b/commit.c

@@ -1041,7 +1041,7 @@ struct commit_list *reduce_heads(struct commit_list *heads)
 		p->item->object.flags |= STALE;
 		num_head++;
 	}
-	array = xcalloc(sizeof(*array), num_head);
+	array = xcalloc(num_head, sizeof(*array));
 	for (p = heads, i = 0; p; p = p->next) {
 		if (p->item->object.flags & STALE) {
 			array[i++] = p->item;

-- 
2.0.0.rc1.543.gc8042da

xcalloc takes two arguments: the number of elements and their size.
config.c includes several calls to xcalloc that pass the arguments
in reverse order: the size of a struct lock_file*, followed by the
number to allocate. Rearrgange them so they are in the correct order.

Signed-off-by: Brian Gesiak <redacted>
---
 config.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/config.c b/config.c
index 314d8ee..c3612cc 100644
--- a/config.c
+++ b/config.c

@@ -1536,7 +1536,7 @@ int git_config_set_multivar_in_file(const char *config_filename,
 	 * The lock serves a purpose in addition to locking: the new
 	 * contents of .git/config will be written into it.
 	 */
-	lock = xcalloc(sizeof(struct lock_file), 1);
+	lock = xcalloc(1, sizeof(struct lock_file));
 	fd = hold_lock_file_for_update(lock, config_filename, 0);
 	if (fd < 0) {
 		error("could not lock config file %s: %s", config_filename, strerror(errno));

@@ -1791,7 +1791,7 @@ int git_config_rename_section_in_file(const char *config_filename,
 	if (!config_filename)
 		config_filename = filename_buf = git_pathdup("config");
 
-	lock = xcalloc(sizeof(struct lock_file), 1);
+	lock = xcalloc(1, sizeof(struct lock_file));
 	out_fd = hold_lock_file_for_update(lock, config_filename, 0);
 	if (out_fd < 0) {
 		ret = error("could not lock config file %s", config_filename);

-- 
2.0.0.rc1.543.gc8042da

Brian,

On Tue, May 27, 2014 at 12:33:42AM +0900, Brian Gesiak wrote:

This patch doesn't apply to any of the branches I have available
(master, pu, next).  And there is no line containing "pathspec->nr + 6"
anywhere in my builtin/add.c.  Which branch is your work based off?

-- 
Jeremiah Mahler
jmmahler@gmail.com
http://github.com/jmahler

Brian,

On Tue, May 27, 2014 at 12:33:41AM +0900, Brian Gesiak wrote:

xcalloc takes two arguments: the number of elements and their size.
The vast majority of the Git codebase passes these arguments in the
correct order, but there are some exceptions. This patch series
corrects those exceptions.

Let me see if I understand the issue underlying this patch set.

xcalloc works like calloc and takes two arguments, the number of
elements and the size of each element.  However, many calls specified
these arguments in the reverse order.  It didn't produce a compile
error because both arguments are the same type.  And it didn't produce
a run time error because A*B is the same as B*A.

If this behaved like dd, performance would be different depending on the
order.

  dd if=in of=out bs=1    count=1024
  dd if=in of=out bs=1024 count=1

Nonetheless, it appears to be a good fix.  Nice job!

-- 
Jeremiah Mahler
jmmahler@gmail.com
http://github.com/jmahler

My apologies! I based my work off of maint, branching off of eea591.

My reasoning was that Documentation/SubmittingPatches states that "a
bugfix should be based on 'maint'". [1] Now that I think about it,
this is probably not the kind of "bug" that statement had in mind.

Should I reroll the patch based on master?

- Brian Gesiak

[1] https://github.com/git/git/blob/4a28f169ad29ba452e0e7bea2583914c10c58322/Documentation/SubmittingPatches#L9

On Tue, May 27, 2014 at 8:11 AM, Jeremiah Mahler [off-list ref] wrote:

Brian,

On Tue, May 27, 2014 at 12:33:42AM +0900, Brian Gesiak wrote:

quoted

xcalloc takes two arguments: the number of elements and their size.
run_add_interactive passes the arguments in reverse order, passing the
size of a char*, followed by the number of char* to be allocated.
Rearrgange them so they are in the correct order.

Signed-off-by: Brian Gesiak <redacted>
---
 builtin/add.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/add.c b/builtin/add.c
index 672adc0..488acf4 100644
--- a/builtin/add.c
+++ b/builtin/add.c

@@ -248,7 +248,7 @@ int run_add_interactive(const char *revision, const char *patch_mode,
      int status, ac, i;
      const char **args;

-     args = xcalloc(sizeof(const char *), (pathspec->nr + 6));
+     args = xcalloc((pathspec->nr + 6), sizeof(const char *));
      ac = 0;
      args[ac++] = "add--interactive";
      if (patch_mode)

This patch doesn't apply to any of the branches I have available
(master, pu, next).  And there is no line containing "pathspec->nr + 6"
anywhere in my builtin/add.c.  Which branch is your work based off?

--
Jeremiah Mahler
jmmahler@gmail.com
http://github.com/jmahler

On Tue, May 27, 2014 at 11:22:00AM +0900, Brian Gesiak wrote:

My apologies! I based my work off of maint, branching off of eea591.

My reasoning was that Documentation/SubmittingPatches states that "a
bugfix should be based on 'maint'". [1] Now that I think about it,
this is probably not the kind of "bug" that statement had in mind.

Should I reroll the patch based on master?

- Brian Gesiak

[1] https://github.com/git/git/blob/4a28f169ad29ba452e0e7bea2583914c10c58322/Documentation/SubmittingPatches#L9

OK, got it.  I should have read Documentation/SubmittingPatches more
closely like you did :-)  No need to reroll I can just use the maint
branch to test it out.  Thanks!

-- 
Jeremiah Mahler
jmmahler@gmail.com
http://github.com/jmahler

On Mon, May 26, 2014 at 11:33 AM, Brian Gesiak [off-list ref] wrote:

xcalloc takes two arguments: the number of elements and their size.
run_add_interactive passes the arguments in reverse order, passing the
size of a char*, followed by the number of char* to be allocated.
Rearrgange them so they are in the correct order.

s/Rearrgange/Rearrange/

Same misspelling afflicts the entire patch series.

Oomph, how embarrassing. Thanks for pointing that out!

Would it be better if I rerolled the patches?

- Brian Gesiak

On Tue, May 27, 2014 at 12:25 PM, Eric Sunshine [off-list ref] wrote:

On Mon, May 26, 2014 at 11:33 AM, Brian Gesiak [off-list ref] wrote:

quoted

xcalloc takes two arguments: the number of elements and their size.
run_add_interactive passes the arguments in reverse order, passing the
size of a char*, followed by the number of char* to be allocated.
Rearrgange them so they are in the correct order.

s/Rearrgange/Rearrange/

Same misspelling afflicts the entire patch series.

quoted

Signed-off-by: Brian Gesiak <redacted>
---
 builtin/add.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/add.c b/builtin/add.c
index 672adc0..488acf4 100644
--- a/builtin/add.c
+++ b/builtin/add.c

@@ -248,7 +248,7 @@ int run_add_interactive(const char *revision, const char *patch_mode,
        int status, ac, i;
        const char **args;

-       args = xcalloc(sizeof(const char *), (pathspec->nr + 6));
+       args = xcalloc((pathspec->nr + 6), sizeof(const char *));
        ac = 0;
        args[ac++] = "add--interactive";
        if (patch_mode)
--

2.0.0.rc1.543.gc8042da

On Tue, May 27, 2014 at 7:32 AM, Brian Gesiak [off-list ref] wrote:

Oomph, how embarrassing. Thanks for pointing that out!

Etiquette on this list is to avoid top-posting [1].

[1]: https://lkml.org/lkml/2005/1/11/111

Would it be better if I rerolled the patches?

Junio may or may not make small fixes himself when he picks up a patch
series. If you don't hear from him and your patches don't appear in
his 'pu' branch with that fix, re-rolling might be advisable.

- Brian Gesiak

On Tue, May 27, 2014 at 12:25 PM, Eric Sunshine [off-list ref] wrote:

quoted

On Mon, May 26, 2014 at 11:33 AM, Brian Gesiak [off-list ref] wrote:

quoted

xcalloc takes two arguments: the number of elements and their size.
run_add_interactive passes the arguments in reverse order, passing the
size of a char*, followed by the number of char* to be allocated.
Rearrgange them so they are in the correct order.

If you do re-roll, perhaps consider simplifying the commit messages.
The patch itself states concisely and precisely what is being changed;
the lengthy prose description doesn't really add anything (and makes
more work for you and the reader of the message). It might be
sufficient to use a single-line (Subject:) commit message, like this:

    builtin/add.c: fix order of xcalloc arguments

quoted

s/Rearrgange/Rearrange/

Same misspelling afflicts the entire patch series.

quoted

Signed-off-by: Brian Gesiak <redacted>
---
 builtin/add.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/add.c b/builtin/add.c
index 672adc0..488acf4 100644
--- a/builtin/add.c
+++ b/builtin/add.c

@@ -248,7 +248,7 @@ int run_add_interactive(const char *revision, const char *patch_mode,
        int status, ac, i;
        const char **args;

-       args = xcalloc(sizeof(const char *), (pathspec->nr + 6));
+       args = xcalloc((pathspec->nr + 6), sizeof(const char *));
        ac = 0;
        args[ac++] = "add--interactive";
        if (patch_mode)
--

2.0.0.rc1.543.gc8042da

On Mon, May 26, 2014 at 04:37:41PM -0700, Jeremiah Mahler wrote:

quoted

xcalloc takes two arguments: the number of elements and their size.
The vast majority of the Git codebase passes these arguments in the
correct order, but there are some exceptions. This patch series
corrects those exceptions.

Let me see if I understand the issue underlying this patch set.

xcalloc works like calloc and takes two arguments, the number of
elements and the size of each element.  However, many calls specified
these arguments in the reverse order.  It didn't produce a compile
error because both arguments are the same type.  And it didn't produce
a run time error because A*B is the same as B*A.

Yes, I think that is a good summary.

It may be theoretically possible that an implementation of calloc()
can use the distinction between the two arguments to adjust the padding
or alignment of the result. However, I don't know if any implementation
actually does this, or if it is even true in theory. You can find
some discussions[1,2] online, but nothing conclusive. The most plausible
theory I saw is that early K&R C may have done something clever here,
but ANSI C alignment requirements effectively remove any wiggle room for
the implementation.

But it certainly does not hurt to follow the spec and be consistent.

-Peff

[1] http://stackoverflow.com/questions/501839/is-calloc4-6-the-same-as-calloc6-4

[2] https://groups.google.com/d/msg/comp.lang.c/jZbiyuYqjB4/NIAmeNd11IoJ

On Tue, May 27, 2014 at 05:35:29PM -0400, Eric Sunshine wrote:

On Tue, May 27, 2014 at 7:32 AM, Brian Gesiak [off-list ref] wrote:

quoted

Oomph, how embarrassing. Thanks for pointing that out!

Etiquette on this list is to avoid top-posting [1].

[1]: https://lkml.org/lkml/2005/1/11/111

A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?

That is the funniest post I have ever seen by Kroah.

-- 
Jeremiah Mahler
jmmahler@gmail.com
http://github.com/jmahler

On Wed, May 28, 2014 at 7:41 AM, Junio C Hamano [off-list ref] wrote:

I do not think it is worth doing this change starting from maint, so
I've dropped this one and a few others that did not apply to master
and queued the remainder to 'pu'.

Thank you! I'll keep this in mind when choosing what to branch off of
in the future.

On Wed, May 28, 2014 at 6:35 AM, Eric Sunshine [off-list ref] wrote:

Etiquette on this list is to avoid top-posting [1].
...
If you do re-roll, perhaps consider simplifying the commit messages.

Thank you for the tips; very much appreciated.

- Brian Gesiak

Brian,

On Tue, May 27, 2014 at 12:33:51AM +0900, Brian Gesiak wrote:

                       ^^^^^^^^^^^^^^^^^^^^^^^^

Why wasn't the second instance swapped?  It is still this way in the
latest 'pu' branch.

-- 
Jeremiah Mahler
jmmahler@gmail.com
http://github.com/jmahler