threads / patch / 34479

patch, 3 partstreewalk.c: Rename variable ret to cb_bits and remove some dead lines.

Subject: [PATCH 1/3] treewalk.c: Rename variable ret to cb_bits and remove some dead lines.

## tl;dr

8 messages between Jul 18, 2013 and Jul 19, 2013. Diffs are folded; open one to read it.

replies: 7people: 2as markdown or json

Stefan Beller· Jul 18, 2013, 21:35 UTC · lore

The variable name ret sounds like the variable to be returned, but since e6c111b4 we return error. Hence the variable name is miss leading. As this variable is used only to extract the bits from the callback of a tree object, I named it cb_bits for callback bits.

Also the assignment to 0 was removed at the start of the function as well after the if(interesting) block. Those were unneeded as that variable is set to the callback return value any time we enter the if(interesting) block, so we'd overwrite old values anyway.

Signed-off-by: Stefan Beller <stefanbeller@googlemail.com>
---
 tree-walk.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

Show changes to tree-walk.c +5 −6

diff --git a/tree-walk.c b/tree-walk.c
index c366852..3c323d1 100644
--- a/tree-walk.c
+++ b/tree-walk.c
@@ -324,7 +324,6 @@ static inline int prune_traversal(struct name_entry *e,
 
 int traverse_trees(int n, struct tree_desc *t, struct traverse_info *info)
 {
-	int ret = 0;
 	int error = 0;
 	struct name_entry *entry = xmalloc(n*sizeof(*entry));
 	int i;
@@ -342,6 +341,7 @@ int traverse_trees(int n, struct tree_desc *t, struct traverse_info *info)
 		strbuf_setlen(&base, info->pathlen);
 	}
 	for (;;) {
+		int cb_bits;
 		unsigned long mask, dirmask;
 		const char *first = NULL;
 		int first_len = 0;
@@ -405,15 +405,14 @@ int traverse_trees(int n, struct tree_desc *t, struct traverse_info *info)
 		if (interesting < 0)
 			break;
 		if (interesting) {
-			ret = info->fn(n, mask, dirmask, entry, info);
-			if (ret < 0) {
-				error = ret;
+			cb_bits = info->fn(n, mask, dirmask, entry, info);
+			if (cb_bits < 0) {
+				error = cb_bits;
 				if (!info->show_all_errors)
 					break;
 			}
-			mask &= ret;
+			mask &= cb_bits;
 		}
-		ret = 0;
 		for (i = 0; i < n; i++)
 			if (mask & (1ul << i))
 				update_extended_entry(tx + i, entry + i);

-- 
1.8.3.3.754.g9c3c367.dirty

Stefan Beller· Jul 18, 2013, 21:35 UTC · re: Stefan Beller · lore

[PATCH 2/3] http-push.c, add_send_request: Do not initialize transfer_request.

That pointer will be assigned to new memory via request = xmalloc(sizeof(*request)); 20 lines later unconditionally anyway, so it's safe to not assign it to an arbitrary variable.

As this patch is a micro-optimization (for readability?) it would be nice to have as little work with this patch, so maybe I need a feature to send a patch without a symmetric number of lines around change-diff. For this patch I'd be only interested in showing the next 20 lines, but nothing really before the function declaration.

Signed-off-by: Stefan Beller <stefanbeller@googlemail.com>
---
 http-push.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Show changes to http-push.c +1 −1

diff --git a/http-push.c b/http-push.c
index 395a8cf..6dad188 100644
--- a/http-push.c
+++ b/http-push.c
@@ -663,7 +663,7 @@ static void add_fetch_request(struct object *obj)
 
 static int add_send_request(struct object *obj, struct remote_lock *lock)
 {
-	struct transfer_request *request = request_queue_head;
+	struct transfer_request *request;
 	struct packed_git *target;
 
 	/* Keep locks active */

-- 
1.8.3.3.754.g9c3c367.dirty

Junio C Hamano· Jul 19, 2013, 18:30 UTC · re: Stefan Beller · lore

Re: [PATCH 2/3] http-push.c, add_send_request: Do not initialize transfer_request.

Patch 2 and 3 look fairly straight-forward. Will queue them directly on 'maint'.

Thanks.

Stefan Beller· Jul 18, 2013, 21:35 UTC · re: Stefan Beller · lore

[PATCH 3/3] apply, find_name_traditional: Do not initialize len to the lines length.

The variable len is set to len = strchrnul(line, '\n') - line; unconditionally 9 lines later, hence we can remove the call to strlen.

Signed-off-by: Stefan Beller <stefanbeller@googlemail.com>
---
 builtin/apply.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Show changes to builtin/apply.c +1 −1

diff --git a/builtin/apply.c b/builtin/apply.c
index 1218688..681a40b 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -722,7 +722,7 @@ static char *find_name(const char *line, char *def, int p_value, int terminate)
 
 static char *find_name_traditional(const char *line, char *def, int p_value)
 {
-	size_t len = strlen(line);
+	size_t len;
 	size_t date_len;
 
 	if (*line == '"') {

-- 
1.8.3.3.754.g9c3c367.dirty

Junio C Hamano· Jul 19, 2013, 18:13 UTC · re: Stefan Beller · lore

Re: [PATCH 1/3] treewalk.c: Rename variable ret to cb_bits and remove some dead lines.

The "ret" was meant to mean "the return value we got from the callback function", not "the return value we would give our caller".

This rename is a bit misleading in that "cb_bits == -1" does not mean "full bits set", and it does not tell us much what these "bits" signify.

They are used to answer this question: which one of the trees in t[0..n] did the callback function consumed (hence needs their pointers updated).

So perhaps call it "trees_used" or something?

By the way, our log message usually do not Capitalize the subject after the "<area>:", i.e. do something like this instead:

    Subject: [PATCH 1/3] traverse_trees(): clarify return value of the callback

Thanks.

Stefan Beller· Jul 19, 2013, 20:25 UTC · re: Junio C Hamano · lore

Re: [PATCH 1/3] treewalk.c: Rename variable ret to cb_bits and remove some dead lines.

On 07/19/2013 08:13 PM, Junio C Hamano wrote:

> The "ret" was meant to mean "the return value we got from the
> callback function", not "the return value we would give our caller".

Thanks for clarifying. I assumed the "ret" was meant as the return value of that function as it was the case before e6c111b4c. In other projects I am using ret as "the return value we would give our caller" as it's such a convenient name for that if you cannot come up with a better name.

Show 10 quoted lines

> 
> This rename is a bit misleading in that "cb_bits == -1" does not
> mean "full bits set", and it does not tell us much what these "bits"
> signify.
> 
> They are used to answer this question: which one of the trees in
> t[0..n] did the callback function consumed (hence needs their
> pointers updated).
> 
> So perhaps call it "trees_used" or something?

Sounds indeed way better. I'll rename it.

Show 8 quoted lines

> 
> By the way, our log message usually do not Capitalize the subject
> after the "<area>:", i.e. do something like this instead:
> 
>     Subject: [PATCH 1/3] traverse_trees(): clarify return value of the callback
> 
> Thanks.
>

Thanks for pointing out.

As a general question: I was mostly doing micro-optimisations or the mailmap file, which are rather small fixups, which I think are ok for beginners. Is there a tasklist for beginners, other than that? Such as porting shell commands to C or other larger tasks? I used git://github.com/gitster/git.git as remote/origin. There the todo branch has the last commit as of 2012/04, so I also found git://git.kernel.org/pub/scm/git/git.git, where the todo branch seems more up-to-date, but the TODO file there also seems a little dated to me. So is there any up-to-date task list for beginning contributors?

Stefan

Stefan Beller· Jul 19, 2013, 20:26 UTC · re: Stefan Beller · lore

[PATCH] traverse_trees(): clarify return value of the callback.

Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Stefan Beller <stefanbeller@googlemail.com>
---
 tree-walk.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

Show changes to tree-walk.c +5 −6

diff --git a/tree-walk.c b/tree-walk.c
index c366852..5ece8c3 100644
--- a/tree-walk.c
+++ b/tree-walk.c
@@ -324,7 +324,6 @@ static inline int prune_traversal(struct name_entry *e,
 
 int traverse_trees(int n, struct tree_desc *t, struct traverse_info *info)
 {
-	int ret = 0;
 	int error = 0;
 	struct name_entry *entry = xmalloc(n*sizeof(*entry));
 	int i;
@@ -342,6 +341,7 @@ int traverse_trees(int n, struct tree_desc *t, struct traverse_info *info)
 		strbuf_setlen(&base, info->pathlen);
 	}
 	for (;;) {
+		int trees_used;
 		unsigned long mask, dirmask;
 		const char *first = NULL;
 		int first_len = 0;
@@ -405,15 +405,14 @@ int traverse_trees(int n, struct tree_desc *t, struct traverse_info *info)
 		if (interesting < 0)
 			break;
 		if (interesting) {
-			ret = info->fn(n, mask, dirmask, entry, info);
-			if (ret < 0) {
-				error = ret;
+			trees_used = info->fn(n, mask, dirmask, entry, info);
+			if (trees_used < 0) {
+				error = trees_used;
 				if (!info->show_all_errors)
 					break;
 			}
-			mask &= ret;
+			mask &= trees_used;
 		}
-		ret = 0;
 		for (i = 0; i < n; i++)
 			if (mask & (1ul << i))
 				update_extended_entry(tx + i, entry + i);

-- 
1.8.3.3.754.g9c3c367.dirty

Junio C Hamano· Jul 19, 2013, 22:28 UTC · re: Stefan Beller · lore

Re: [PATCH 1/3] treewalk.c: Rename variable ret to cb_bits and remove some dead lines.

Stefan Beller <stefanbeller@googlemail.com> writes:

> So is there any up-to-date task list for beginning contributors?

I am fairly bad at keeping track of small things incrementally, as it is often quicker to do them myself if/when I were so inclined, but there are too many of them and a day does not have enough number of seconds X-<.

These days, I tend to scan the mailing list backlog in batch every once in a while.

  http://git-blame.blogspot.com/search?q=leftover+bits

may show some of them.

Thanks for asking and volunteering.