Re: [RFC][PATCH 1/2] worktree: represent the primary worktree with '/' instead of NULL

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

Shreyansh Paliwal <shreyanshpaliwalcmsmn@gmail.com> writes:

> diff --git a/worktree.c b/worktree.c
> index 9308389cb6..b29934407f 100644
> --- a/worktree.c
> +++ b/worktree.c
> @@ -101,6 +101,7 @@ static struct worktree *get_main_worktree(int skip_reading_head)
>  
>  	CALLOC_ARRAY(worktree, 1);
>  	worktree->repo = the_repository;
> +	worktree->id = xstrdup("/");
>  	worktree->path = strbuf_detach(&worktree_path, NULL);
>  	worktree->is_current = is_current_worktree(worktree);
>  	worktree->is_bare = (is_bare_repository_cfg == 1) ||

Presumably we left .id = NULL from CALLOC_ARRAY(), so this looks
sensible.  When releasing resources from an instance of worktree,
we'd blindly free(worktree->id) and in the old world, free(NULL)
turned into no-op, and this xstrdup()'d copy will be freed in the
new world, so there is nothing funny here, I hope?  This one, and
the change to is_main_worktree() go together.

> @@ -127,6 +128,8 @@ struct worktree *get_linked_worktree(const char *id,
>  
>  	if (!id)
>  		die("Missing linked worktree name");
> +	if (!strcmp(id, "/"))
> +		die("'/' is reserved for primary worktree");

Makes me wonder if this is a BUG not die; where does id come from?

	... goes and looks ...

The only caller is worktree.c:get_worktrees_internal() and it is
feeding d->d_name that came from readdir_skip_dot_and_dotdot(), so
it cannot be "/".

By the way, I suspect that get_linked_worktree() should become
file-scope static, as there is no other caller.

> @@ -206,9 +209,7 @@ struct worktree **get_worktrees_without_reading_head(void)
>  
>  char *get_worktree_git_dir(const struct worktree *wt)
>  {
> -	if (!wt)
> -		return xstrdup(repo_get_git_dir(the_repository));
> -	else if (!wt->id)
> +	if (is_main_worktree(wt))
>  		return xstrdup(repo_get_common_dir(the_repository));
>  	else
>  		return repo_common_path(the_repository, "worktrees/%s", wt->id);

Good spotting.  This series needs to spot any and all places that
use these other conventions (i.e. wt->id == NULL) to identify the
primary worktree and rewrite them to call is_main_worktree(), which
may be a chore, but once it is done, it would become a lot easier to
follow the resulting code.

> @@ -277,7 +278,7 @@ struct worktree *find_worktree_by_path(struct worktree **list, const char *p)
>  
>  int is_main_worktree(const struct worktree *wt)
>  {
> -	return !wt->id;
> +	return !strcmp(wt->id, "/");
>  }

OK.

> @@ -566,7 +567,7 @@ void strbuf_worktree_ref(const struct worktree *wt,
>  {
>  	if (parse_worktree_ref(refname, NULL, NULL, NULL) ==
>  		    REF_WORKTREE_CURRENT &&
> -	    wt && !wt->is_current) {
> +	    !wt->is_current) {

OK.

> @@ -629,6 +630,9 @@ static void repair_gitfile(struct worktree *wt,
>  	char *path = NULL;
>  	int err;
>  
> +	if (is_main_worktree(wt))
> +		goto done;

This is a bit new.

The original did not say 

	if (!wt || !wt->id || !strcmp(wt->id, "/"))
		goto done;

The only caller is iterating over the resulting list of worktrees
returned from get_worktrees_internal(1) *BUT* it already skips the
primary worktree (the function MUST return the primary one as the
first one, and the callers MUST be aware of the convention).

So I am not sure if the new check is even needed.  Or rather, this ...

	if (!wt || !wt->id || !strcmp(wt->id, "/"))
		BUG("why are you feeding me the primary worktree???");
	
... might be more appropriate, perhaps?  I dunno.


>  	/* missing worktree can't be repaired */
>  	if (!file_exists(wt->path))
>  		goto done;