Re: [PATCH v5 07/24] files-backend: add and use files_refname_path()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 02/22/2017 03:04 PM, Nguyễn Thái Ngọc Duy wrote:
> Keep repo-related path handling in one place. This will make it easier
> to add submodule/multiworktree support later.
> 
> This automatically adds the "if submodule then use the submodule version
> of git_path" to other call sites too. But it does not mean those
> operations are sumodule-ready. Not yet.

s/sumodule/submodule/

> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@xxxxxxxxx>
> ---
>  refs/files-backend.c | 45 +++++++++++++++++++++++++--------------------
>  1 file changed, 25 insertions(+), 20 deletions(-)
> 
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 7b4ea4c56..72f4e1746 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -1180,6 +1180,18 @@ static void files_reflog_path(struct files_ref_store *refs,
>  	strbuf_git_path(sb, "logs/%s", refname);
>  }
>  
> +static void files_refname_path(struct files_ref_store *refs,
> +			       struct strbuf *sb,
> +			       const char *refname)
> +{
> +	if (refs->submodule) {
> +		strbuf_git_path_submodule(sb, refs->submodule, "%s", refname);
> +		return;
> +	}
> +
> +	strbuf_git_path(sb, "%s", refname);
> +}

Maybe it's just me, but I find it odd to exit early here when the first
exit isn't due to an error. For me, structuring this like `if ()
call1(); else call2();` would make it clearer that the two code paths
are equally-valid alternatives, and either one or the other will be
executed.

I had the same feeling when I read `files_reflog_path()`

>  /*
>   * Get the packed_ref_cache for the specified files_ref_store,
>   * creating it if necessary.
> @@ -1251,10 +1263,7 @@ static void read_loose_refs(const char *dirname, struct ref_dir *dir)
>  	size_t path_baselen;
>  	int err = 0;
>  
> -	if (refs->submodule)
> -		err = strbuf_git_path_submodule(&path, refs->submodule, "%s", dirname);
> -	else
> -		strbuf_git_path(&path, "%s", dirname);
> +	files_refname_path(refs, &path, dirname);

It's so nice to see these ugly `if (refs->submodule)` code blocks
disappearing!

> [...]

Michael




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]