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

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

[Stefan Beller]

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(-)

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

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

[Stefan Beller]

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(-)

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

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

[Junio C Hamano]

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

Thanks.

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

[Stefan Beller]

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(-)

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

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

[Junio C Hamano]

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.

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

[Stefan Beller]

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.

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

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

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

[Stefan Beller]

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, trees_used is a better name.

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.

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(-)

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

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

[Junio C Hamano]

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.