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

[Junio C Hamano <gitster@pobox.com>]

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);
>  		}