[PATCH] pretend_sha1_file(): Change return type from int to void

prented_sha1_file() always returns 0 and its only callsite in
builtin/blame.c doesn't use the return value, so change the return type
to void.

Signed-off-by: Tobias Klauser <tklauser@distanz.ch>
---
 cache.h     | 2 +-
 sha1_file.c | 5 ++---
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/cache.h b/cache.h
index 752031e..445853b 100644
--- a/cache.h
+++ b/cache.h

@@ -970,7 +970,7 @@ extern int sha1_object_info(const unsigned char *, unsigned long *);
 extern int hash_sha1_file(const void *buf, unsigned long len, const char *type, unsigned char *sha1);
 extern int write_sha1_file(const void *buf, unsigned long len, const char *type, unsigned char *return_sha1);
 extern int hash_sha1_file_literally(const void *buf, unsigned long len, const char *type, unsigned char *sha1, unsigned flags);
-extern int pretend_sha1_file(void *, unsigned long, enum object_type, unsigned char *);
+extern void pretend_sha1_file(void *, unsigned long, enum object_type, unsigned char *);
 extern int force_object_loose(const unsigned char *sha1, time_t mtime);
 extern int git_open_noatime(const char *name);
 extern void *map_sha1_file(const unsigned char *sha1, unsigned long *size);

diff --git a/sha1_file.c b/sha1_file.c
index d295a32..d76b723 100644
--- a/sha1_file.c
+++ b/sha1_file.c

@@ -2789,14 +2789,14 @@ static void *read_packed_sha1(const unsigned char *sha1,
 	return data;
 }
 
-int pretend_sha1_file(void *buf, unsigned long len, enum object_type type,
+void pretend_sha1_file(void *buf, unsigned long len, enum object_type type,
 		      unsigned char *sha1)
 {
 	struct cached_object *co;
 
 	hash_sha1_file(buf, len, typename(type), sha1);
 	if (has_sha1_file(sha1) || find_cached_object(sha1))
-		return 0;
+		return;
 	ALLOC_GROW(cached_objects, cached_object_nr + 1, cached_object_alloc);
 	co = &cached_objects[cached_object_nr++];
 	co->size = len;

@@ -2804,7 +2804,6 @@ int pretend_sha1_file(void *buf, unsigned long len, enum object_type type,
 	co->buf = xmalloc(len);
 	memcpy(co->buf, buf, len);
 	hashcpy(co->sha1, sha1);
-	return 0;
 }
 
 static void *read_object(const unsigned char *sha1, enum object_type *type,

-- 
2.6.0

Hi Tobias,

On 2015-10-06 14:15, Tobias Klauser wrote:

prented_sha1_file() always returns 0 and its only callsite in
builtin/blame.c doesn't use the return value, so change the return type
to void.

While this commit message is technically correct, it would appear that there are some things left unsaid.

Is there a problem with the current code that is solved by not returning 0? If so, could you add it to the commit message? And in particular, change the oneline appropriately?

Ciao,
Johannes

Hi Johannes

Thanks for your feedback.

On 2015-10-06 at 15:16:12 +0200, Johannes Schindelin [off-list ref] wrote:

Hi Tobias,

On 2015-10-06 14:15, Tobias Klauser wrote:

quoted

prented_sha1_file() always returns 0 and its only callsite in
builtin/blame.c doesn't use the return value, so change the return type
to void.

While this commit message is technically correct, it would appear that there are some things left unsaid.

Is there a problem with the current code that is solved by not returning 0? If so, could you add it to the commit message? And in particular, change the oneline appropriately?

There's no problem with the current code other than that the return
value is unused and thus unnecessary for correct funcionality. So it's
certainly not a functional problem but rather a cosmetic change.

Does such a change even make sense (it's one of my first patch to git,
so I'm not really sure what your criteria in this respect are)?

If yes, would something like the following bring across the intention
more clearly?

  pretend_sha1_file() always returns 0 and its only user in
  builtin/blame.c doesn't use the returned value. Thus, the return value
  is unnecessary and the return type of pretend_sha1_file() can be
  changed to void.

Cheers
Tobias

Hi Tobias,

On 2015-10-06 15:51, Tobias Klauser wrote:

On 2015-10-06 at 15:16:12 +0200, Johannes Schindelin
[off-list ref] wrote:

quoted

On 2015-10-06 14:15, Tobias Klauser wrote:

quoted

prented_sha1_file() always returns 0 and its only callsite in
builtin/blame.c doesn't use the return value, so change the return type
to void.

While this commit message is technically correct, it would appear that there are some things left unsaid.

Is there a problem with the current code that is solved by not returning 0? If so, could you add it to the commit message? And in particular, change the oneline appropriately?

There's no problem with the current code other than that the return
value is unused and thus unnecessary for correct funcionality. So it's
certainly not a functional problem but rather a cosmetic change.

Okay.

Does such a change even make sense (it's one of my first patch to git,
so I'm not really sure what your criteria in this respect are)?

Welcome!

As to the patch, I cannot speak for Junio, of course, but my preference would be to keep the return type. Traditionally, functions that can fail either die() or return an int; non-zero indicates an error. In this case, it seems that we do not have any condition (yet...) under which an error could occur. It does not seem very unlikely that we may eventually have such conditions, though, hence my preference.

Ciao,
Johannes

Hi Johannes

On 2015-10-06 at 16:30:36 +0200, Johannes Schindelin [off-list ref] wrote:

On 2015-10-06 15:51, Tobias Klauser wrote:

quoted

On 2015-10-06 at 15:16:12 +0200, Johannes Schindelin
[off-list ref] wrote:

quoted

On 2015-10-06 14:15, Tobias Klauser wrote:

quoted

prented_sha1_file() always returns 0 and its only callsite in
builtin/blame.c doesn't use the return value, so change the return type
to void.

While this commit message is technically correct, it would appear that there are some things left unsaid.

Is there a problem with the current code that is solved by not returning 0? If so, could you add it to the commit message? And in particular, change the oneline appropriately?

There's no problem with the current code other than that the return
value is unused and thus unnecessary for correct funcionality. So it's
certainly not a functional problem but rather a cosmetic change.

Okay.

quoted

Does such a change even make sense (it's one of my first patch to git,
so I'm not really sure what your criteria in this respect are)?

Welcome!

As to the patch, I cannot speak for Junio, of course, but my preference would be to keep the return type. Traditionally, functions that can fail either die() or return an int; non-zero indicates an error. In this case, it seems that we do not have any condition (yet...) under which an error could occur. It does not seem very unlikely that we may eventually have such conditions, though, hence my preference.

Ok, I see. Thank you for your explanation. I'll wait for Junio's decision
then :)

Cheers
Tobias