[PATCH v2] builtin/mktree: remove USE_THE_REPOSITORY_VARIABLE

[PATCH v2] builtin/mktree: remove USE_THE_REPOSITORY_VARIABLE

[Tian Yuchen]

The 'cmd_mktree()' function already receives a 'struct repository *repo'
pointer, but it was previously marked as UNUSED.

Pass the 'repo' pointer down to 'mktree_line()' and 'write_tree()'.
Consequently, remove the 'USE_THE_REPOSITORY_VARIABLE' macro, replace
usages of 'the_repository', and swap 'parse_oid_hex()' with its context-aware
version 'parse_oid_hex_algop()'.

This refactoring is safe because 'cmd_mktree()' is registered with the
'RUN_SETUP' flag in 'git.c', which guarantees that the command is
executed within a initialized repository, ensuring that the passed 'repo'
pointer is never 'NULL'.

Signed-off-by: Tian Yuchen <cat@malon.dev>
---
 builtin/mktree.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/builtin/mktree.c b/builtin/mktree.c
index 12772303f5..4084e32476 100644
--- a/builtin/mktree.c
+++ b/builtin/mktree.c
@@ -3,7 +3,6 @@
  *
  * Copyright (c) Junio C Hamano, 2006, 2009
  */
-#define USE_THE_REPOSITORY_VARIABLE
 #include "builtin.h"
 #include "gettext.h"
 #include "hex.h"
@@ -46,7 +45,7 @@ static int ent_compare(const void *a_, const void *b_)
 				 b->name, b->len, b->mode);
 }
 
-static void write_tree(struct object_id *oid)
+static void write_tree(struct repository *repo, struct object_id *oid)
 {
 	struct strbuf buf;
 	size_t size;
@@ -60,10 +59,10 @@ static void write_tree(struct object_id *oid)
 	for (i = 0; i < used; i++) {
 		struct treeent *ent = entries[i];
 		strbuf_addf(&buf, "%o %s%c", ent->mode, ent->name, '\0');
-		strbuf_add(&buf, ent->oid.hash, the_hash_algo->rawsz);
+		strbuf_add(&buf, ent->oid.hash, repo->hash_algo->rawsz);
 	}
 
-	odb_write_object(the_repository->objects, buf.buf, buf.len, OBJ_TREE, oid);
+	odb_write_object(repo->objects, buf.buf, buf.len, OBJ_TREE, oid);
 	strbuf_release(&buf);
 }
 
@@ -72,7 +71,7 @@ static const char *const mktree_usage[] = {
 	NULL
 };
 
-static void mktree_line(char *buf, int nul_term_line, int allow_missing)
+static void mktree_line(struct repository *repo, char *buf, int nul_term_line, int allow_missing)
 {
 	char *ptr, *ntr;
 	const char *p;
@@ -93,7 +92,7 @@ static void mktree_line(char *buf, int nul_term_line, int allow_missing)
 		die("input format error: %s", buf);
 	ptr = ntr + 1; /* type */
 	ntr = strchr(ptr, ' ');
-	if (!ntr || parse_oid_hex(ntr + 1, &oid, &p) ||
+	if (!ntr || parse_oid_hex_algop(ntr + 1, &oid, &p, repo->hash_algo) ||
 	    *p != '\t')
 		die("input format error: %s", buf);
 
@@ -124,7 +123,7 @@ static void mktree_line(char *buf, int nul_term_line, int allow_missing)
 
 	/* Check the type of object identified by oid without fetching objects */
 	oi.typep = &obj_type;
-	if (odb_read_object_info_extended(the_repository->objects, &oid, &oi,
+	if (odb_read_object_info_extended(repo->objects, &oid, &oi,
 					  OBJECT_INFO_LOOKUP_REPLACE |
 					  OBJECT_INFO_QUICK |
 					  OBJECT_INFO_SKIP_FETCH_OBJECT) < 0)
@@ -155,7 +154,7 @@ static void mktree_line(char *buf, int nul_term_line, int allow_missing)
 int cmd_mktree(int ac,
 	       const char **av,
 	       const char *prefix,
-	       struct repository *repo UNUSED)
+	       struct repository *repo)
 {
 	struct strbuf sb = STRBUF_INIT;
 	struct object_id oid;
@@ -187,7 +186,7 @@ int cmd_mktree(int ac,
 					break;
 				die("input format error: (blank line only valid in batch mode)");
 			}
-			mktree_line(sb.buf, nul_term_line, allow_missing);
+			mktree_line(repo, sb.buf, nul_term_line, allow_missing);
 		}
 		if (is_batch_mode && got_eof && used < 1) {
 			/*
@@ -197,7 +196,7 @@ int cmd_mktree(int ac,
 			 */
 			; /* skip creating an empty tree */
 		} else {
-			write_tree(&oid);
+			write_tree(repo, &oid);
 			puts(oid_to_hex(&oid));
 			fflush(stdout);
 		}
-- 
2.43.0

Re: [PATCH v2] builtin/mktree: remove USE_THE_REPOSITORY_VARIABLE

[Junio C Hamano]

Tian Yuchen <cat@malon.dev> writes:

> The 'cmd_mktree()' function already receives a 'struct repository *repo'
> pointer, but it was previously marked as UNUSED.
>
> Pass the 'repo' pointer down to 'mktree_line()' and 'write_tree()'.
> Consequently, remove the 'USE_THE_REPOSITORY_VARIABLE' macro, replace
> usages of 'the_repository', and swap 'parse_oid_hex()' with its context-aware
> version 'parse_oid_hex_algop()'.
>
> This refactoring is safe because 'cmd_mktree()' is registered with the
> 'RUN_SETUP' flag in 'git.c', which guarantees that the command is
> executed within a initialized repository, ensuring that the passed 'repo'
> pointer is never 'NULL'.

RUN_SETUP also guarantees that the repo points at the_repository.

The patch is not wrong per-se, but at the same time, it is not a
very interesting change exactly for this reason.

Where did you read that dropping USE_THE_REPOSITORY_VARIABLE is a
good idea?

As somebody (Phillip?) said earlier, we probably should update
document and clearly say that removing USE_THE_REPOSITORY_VARIABLE
is not a high-value target when done in the builtin/ directory, even
though it is very desirable thing to do for more library-ish part of
the codebase.

> Signed-off-by: Tian Yuchen <cat@malon.dev>
> ---
>  builtin/mktree.c | 19 +++++++++----------
>  1 file changed, 9 insertions(+), 10 deletions(-)
>
> diff --git a/builtin/mktree.c b/builtin/mktree.c
> index 12772303f5..4084e32476 100644
> --- a/builtin/mktree.c
> +++ b/builtin/mktree.c
> @@ -3,7 +3,6 @@
>   *
>   * Copyright (c) Junio C Hamano, 2006, 2009
>   */
> -#define USE_THE_REPOSITORY_VARIABLE
>  #include "builtin.h"
>  #include "gettext.h"
>  #include "hex.h"
> @@ -46,7 +45,7 @@ static int ent_compare(const void *a_, const void *b_)
>  				 b->name, b->len, b->mode);
>  }
>  
> -static void write_tree(struct object_id *oid)
> +static void write_tree(struct repository *repo, struct object_id *oid)
>  {
>  	struct strbuf buf;
>  	size_t size;
> @@ -60,10 +59,10 @@ static void write_tree(struct object_id *oid)
>  	for (i = 0; i < used; i++) {
>  		struct treeent *ent = entries[i];
>  		strbuf_addf(&buf, "%o %s%c", ent->mode, ent->name, '\0');
> -		strbuf_add(&buf, ent->oid.hash, the_hash_algo->rawsz);
> +		strbuf_add(&buf, ent->oid.hash, repo->hash_algo->rawsz);
>  	}
>  
> -	odb_write_object(the_repository->objects, buf.buf, buf.len, OBJ_TREE, oid);
> +	odb_write_object(repo->objects, buf.buf, buf.len, OBJ_TREE, oid);
>  	strbuf_release(&buf);
>  }
>  
> @@ -72,7 +71,7 @@ static const char *const mktree_usage[] = {
>  	NULL
>  };
>  
> -static void mktree_line(char *buf, int nul_term_line, int allow_missing)
> +static void mktree_line(struct repository *repo, char *buf, int nul_term_line, int allow_missing)
>  {
>  	char *ptr, *ntr;
>  	const char *p;
> @@ -93,7 +92,7 @@ static void mktree_line(char *buf, int nul_term_line, int allow_missing)
>  		die("input format error: %s", buf);
>  	ptr = ntr + 1; /* type */
>  	ntr = strchr(ptr, ' ');
> -	if (!ntr || parse_oid_hex(ntr + 1, &oid, &p) ||
> +	if (!ntr || parse_oid_hex_algop(ntr + 1, &oid, &p, repo->hash_algo) ||
>  	    *p != '\t')
>  		die("input format error: %s", buf);
>  
> @@ -124,7 +123,7 @@ static void mktree_line(char *buf, int nul_term_line, int allow_missing)
>  
>  	/* Check the type of object identified by oid without fetching objects */
>  	oi.typep = &obj_type;
> -	if (odb_read_object_info_extended(the_repository->objects, &oid, &oi,
> +	if (odb_read_object_info_extended(repo->objects, &oid, &oi,
>  					  OBJECT_INFO_LOOKUP_REPLACE |
>  					  OBJECT_INFO_QUICK |
>  					  OBJECT_INFO_SKIP_FETCH_OBJECT) < 0)
> @@ -155,7 +154,7 @@ static void mktree_line(char *buf, int nul_term_line, int allow_missing)
>  int cmd_mktree(int ac,
>  	       const char **av,
>  	       const char *prefix,
> -	       struct repository *repo UNUSED)
> +	       struct repository *repo)
>  {
>  	struct strbuf sb = STRBUF_INIT;
>  	struct object_id oid;
> @@ -187,7 +186,7 @@ int cmd_mktree(int ac,
>  					break;
>  				die("input format error: (blank line only valid in batch mode)");
>  			}
> -			mktree_line(sb.buf, nul_term_line, allow_missing);
> +			mktree_line(repo, sb.buf, nul_term_line, allow_missing);
>  		}
>  		if (is_batch_mode && got_eof && used < 1) {
>  			/*
> @@ -197,7 +196,7 @@ int cmd_mktree(int ac,
>  			 */
>  			; /* skip creating an empty tree */
>  		} else {
> -			write_tree(&oid);
> +			write_tree(repo, &oid);
>  			puts(oid_to_hex(&oid));
>  			fflush(stdout);
>  		}

Re: [PATCH v2] builtin/mktree: remove USE_THE_REPOSITORY_VARIABLE

[Tian Yuchen]

Hi Junio,

> RUN_SETUP also guarantees that the repo points at the_repository.
> 
> The patch is not wrong per-se, but at the same time, it is not a
> very interesting change exactly for this reason.
> 
> Where did you read that dropping USE_THE_REPOSITORY_VARIABLE is a
> good idea?

I mentioned this at the bottom of v1:

> I originally intended to attempt the #FIXME in t1006-cat-file.sh.
> I followed the clues all the way here, only to discover that the
> FIXME required a level of expertise far beyond my capabilities,
> so I gave up. However, I spot the global variable here, so I went
> ahead and fixed it 😉

In other words, I just happened to see this thing. I didn't go looking 
for it ;)

> As somebody (Phillip?) said earlier, we probably should update
> document and clearly say that removing USE_THE_REPOSITORY_VARIABLE
> is not a high-value target when done in the builtin/ directory, even
> though it is very desirable thing to do for more library-ish part of
> the codebase.

I am fully aware of this, and I did not specifically modify 
the_repository in builtin/ during previous patches. It's just that this 
macro makes me particularly uncomfortable, and I believe it would be 
better to remove it.

On the other hand, this patch is indeed boring and useless. Feel free to 
ignore it.

Regards,

Yuchen

Re: [PATCH v2] builtin/mktree: remove USE_THE_REPOSITORY_VARIABLE

[Junio C Hamano]

Tian Yuchen <cat@malon.dev> writes:

>> As somebody (Phillip?) said earlier, we probably should update
>> document and clearly say that removing USE_THE_REPOSITORY_VARIABLE
>> is not a high-value target when done in the builtin/ directory, even
>> though it is very desirable thing to do for more library-ish part of
>> the codebase.
>
> I am fully aware of this, and I did not specifically modify 
> the_repository in builtin/ during previous patches. It's just that this 
> macro makes me particularly uncomfortable, and I believe it would be 
> better to remove it.
>
> On the other hand, this patch is indeed boring and useless. Feel free to 
> ignore it.

Nah, I think we do want to keep it; once it is written, it is a
waste to discard it, especially given that the change is not wrong
per-se.  If anything else, having it will save somebody else time
and effort to do the same thing again ;-).

Thanks.