Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refs: exit early from the loop if it is not a main worktree #1848

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

AZero13
Copy link

@AZero13 AZero13 commented Dec 18, 2024

The is_main_worktree function just checks for !wt->id, but the compiler doesn't know this as it is in a different file, so just exit out early.

cc: shejialuo shejialuo@gmail.com
cc: Eric Sunshine sunshine@sunshineco.com

The is_main_worktree function just checks for !wt->id,
but the compiler doesn't know this as it is in a different
file, so just exit out early.

Signed-off-by: Seija Kijin <doremylover123@gmail.com>
Copy link

Submitted as pull.1848.git.git.1734488445457.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-git-1848/AreaZR/exit-early-v1

To fetch this version to local tag pr-git-1848/AreaZR/exit-early-v1:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-git-1848/AreaZR/exit-early-v1

Copy link

On the Git mailing list, shejialuo wrote (reply to this):

On Wed, Dec 18, 2024 at 02:20:45AM +0000, AreaZR via GitGitGadget wrote:
> From: Seija Kijin <doremylover123@gmail.com>
> 
> The is_main_worktree function just checks for !wt->id,
> but the compiler doesn't know this as it is in a different
> file, so just exit out early.
> 

I think maybe we should exit out the loop early. However, the above
statement is confusing. As you have said, `is_main_worktree` checks
whether the `wt->id` is NULL. Why compiler doesn't know this? And why we
need to exit out the loop due to above reason?

> Signed-off-by: Seija Kijin <doremylover123@gmail.com>
> ---
>     refs: exit early from the loop if it is not a main worktree
>     
>     The is_main_worktree function just checks for !wt->id, but the compiler
>     doesn't know this as it is in a different file, so just exit out early.
> 
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1848%2FAreaZR%2Fexit-early-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1848/AreaZR/exit-early-v1
> Pull-Request: https://github.com/git/git/pull/1848
> 
>  refs.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/refs.c b/refs.c
> index 8b713692359..cce63a618d7 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -2791,6 +2791,7 @@ static int has_worktrees(void)
>  		if (is_main_worktree(worktrees[i]))
>  			continue;
>  		ret = 1;
> +		break;

So, when we find a linked worktree, we just return the value. From my
perspective, if we decide to optimize like this way, we could drop the
loop because the first element of the result of `get_worktrees` is the
main worktree. And we could just check whether the "worktrees[1]" is
NULL to do above.

However, I don't know whether it's a good idea to exit the loop early
in the first place. CC Patrick to help.

Thanks,
Jialuo

Copy link

User shejialuo <shejialuo@gmail.com> has been added to the cc: list.

Copy link

On the Git mailing list, Eric Sunshine wrote (reply to this):

On Wed, Dec 18, 2024 at 8:30 AM shejialuo <shejialuo@gmail.com> wrote:
> On Wed, Dec 18, 2024 at 02:20:45AM +0000, AreaZR via GitGitGadget wrote:
> >               if (is_main_worktree(worktrees[i]))
> >                       continue;
> >               ret = 1;
> > +             break;
>
> So, when we find a linked worktree, we just return the value. From my
> perspective, if we decide to optimize like this way, we could drop the
> loop because the first element of the result of `get_worktrees` is the
> main worktree. And we could just check whether the "worktrees[1]" is
> NULL to do above.

You're correct. get_worktrees() guarantees that the main worktree (or
bare repository) is the first item in the list, so merely checking
whether `worktrees[1]` is non-NULL would be sufficient to answer
whether linked worktrees are present; no looping is required.

> However, I don't know whether it's a good idea to exit the loop early
> in the first place. CC Patrick to help.

If the loop is retained for some reason (though it really isn't
needed), then exiting early is indeed desirable. I suspect that the
missing `break` was just a silly oversight on Patrick's part.

Copy link

User Eric Sunshine <sunshine@sunshineco.com> has been added to the cc: list.

Copy link

On the Git mailing list, Junio C Hamano wrote (reply to this):

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Wed, Dec 18, 2024 at 8:30 AM shejialuo <shejialuo@gmail.com> wrote:
>> On Wed, Dec 18, 2024 at 02:20:45AM +0000, AreaZR via GitGitGadget wrote:
>> >               if (is_main_worktree(worktrees[i]))
>> >                       continue;
>> >               ret = 1;
>> > +             break;
>>
>> So, when we find a linked worktree, we just return the value. From my
>> perspective, if we decide to optimize like this way, we could drop the
>> loop because the first element of the result of `get_worktrees` is the
>> main worktree. And we could just check whether the "worktrees[1]" is
>> NULL to do above.
>
> You're correct. get_worktrees() guarantees that the main worktree (or
> bare repository) is the first item in the list, so merely checking
> whether `worktrees[1]` is non-NULL would be sufficient to answer
> whether linked worktrees are present; no looping is required.

Thanks for a well-reasoned write-up.

Would many other callers potentially want to know if the repository
has more than one worktree?  It looks to me that the has_worktrees()
helper function in refs.c is a sign that the worktree API is missing
a function.  Calling get_worktrees() to prepare a list of worktrees
and then counting the result, only to see if there are more than
one, sounds a bit wasteful if we need to do so too often.

Copy link

On the Git mailing list, Eric Sunshine wrote (reply to this):

On Wed, Dec 18, 2024 at 8:10 PM Junio C Hamano <gitster@pobox.com> wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
> > On Wed, Dec 18, 2024 at 8:30 AM shejialuo <shejialuo@gmail.com> wrote:
> >> On Wed, Dec 18, 2024 at 02:20:45AM +0000, AreaZR via GitGitGadget wrote:
> >> >               if (is_main_worktree(worktrees[i]))
> >> >                       continue;
> >> >               ret = 1;
> >> > +             break;
> >>
> >> So, when we find a linked worktree, we just return the value. From my
> >> perspective, if we decide to optimize like this way, we could drop the
> >> loop because the first element of the result of `get_worktrees` is the
> >> main worktree. And we could just check whether the "worktrees[1]" is
> >> NULL to do above.
> >
> > You're correct. get_worktrees() guarantees that the main worktree (or
> > bare repository) is the first item in the list, so merely checking
> > whether `worktrees[1]` is non-NULL would be sufficient to answer
> > whether linked worktrees are present; no looping is required.
>
> Would many other callers potentially want to know if the repository
> has more than one worktree?  It looks to me that the has_worktrees()
> helper function in refs.c is a sign that the worktree API is missing
> a function.  Calling get_worktrees() to prepare a list of worktrees
> and then counting the result, only to see if there are more than
> one, sounds a bit wasteful if we need to do so too often.

If the need to answer this question does become common, then I can
imagine a function being added to the worktree API which tries to be
smart about it by only calling readdir() -- and validating a
.git/worktrees/<id>/ metainformation -- enough times to be able to
answer the question.

However, although I haven't audited the code, I suspect the question
"are there any linked worktrees" is rare, possibly only asked by
`refs.c`. And in that case, it is asked only at the start of a
refs-migration operation. Moreover, it appears that even that case of
asking the question is probably temporary, existing only until someone
extends the migration logic to work correctly in the presence of
worktrees. (I'm sure Patrick can shed more light on this, though.)

Copy link

On the Git mailing list, Junio C Hamano wrote (reply to this):

Eric Sunshine <sunshine@sunshineco.com> writes:

> However, although I haven't audited the code, I suspect the question
> "are there any linked worktrees" is rare, possibly only asked by
> `refs.c`. And in that case, it is asked only at the start of a
> refs-migration operation.

That matches my gut feeling.  Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants