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

From: Seija Kijin <[email protected]>

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 <[email protected]>

---
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(+)

refs.c +0 -0

index 8b713692359..cce63a618d7 100644

Quoted 4 lines · AreaZR via GitGitGadget

              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.

Quoted 11 lines · shejialuo

Quoted 4 lines · AreaZR via GitGitGadget

              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.

Quoted 13 lines · Eric Sunshine

Quoted 11 lines · shejialuo

Quoted 4 lines · AreaZR via GitGitGadget

              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.

Quoted 4 lines

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.

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.

Quoted 19 lines · Junio C Hamano

Quoted 13 lines · Eric Sunshine

Quoted 11 lines · shejialuo

Quoted 4 lines · AreaZR via GitGitGadget

              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.

Quoted 4 lines

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.

Quoted 6 lines

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.)

Quoted 11 lines · shejialuo

Quoted 4 lines · AreaZR via GitGitGadget

              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.

Quoted 2 lines

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.