| Message ID | 20250320014646.2899791-1-jltobler@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | clone: suppress unexpected advice message during clone | expand |
Hi Justin On 20/03/2025 01:46, Justin Tobler wrote: > > A side-effect of this change is that the location of the first > `git_default_branch_name()` gets deferred to a later point of execution. > This matters because `git_default_branch_name()` only computes the > default branch name once and returns a cached value for subsequent > invocations. After this change, the `git_default_branch_name()` call > site that actually computes the value becomes `guess_remote_head()` and > is configured to always show the advice message. Isn't the fundamental cause of this bug that advise() ignores GIT_ADVICE? I'm not really clear why "git --no-advice" only applies to advice that is guarded by advice_enabled() when it is documented as disabling all advice hints. Best Wishes Phillip
On 25/03/20 11:10AM, Phillip Wood wrote: > Hi Justin > > On 20/03/2025 01:46, Justin Tobler wrote: > > > > A side-effect of this change is that the location of the first > > `git_default_branch_name()` gets deferred to a later point of execution. > > This matters because `git_default_branch_name()` only computes the > > default branch name once and returns a cached value for subsequent > > invocations. After this change, the `git_default_branch_name()` call > > site that actually computes the value becomes `guess_remote_head()` and > > is configured to always show the advice message. > > Isn't the fundamental cause of this bug that advise() ignores GIT_ADVICE? > I'm not really clear why "git --no-advice" only applies to advice that is > guarded by advice_enabled() when it is documented as disabling all advice > hints. From my point of view, this advice message should never be presented in this particular scenario regardless of the configuration as it doesn't make much sense here. This also happens to be the original behavior so I think we should probably return to that state. I was also a bit suprised to see that not all advice messages respect the `--no-advice` option. I'm not sure if there is a reason for this, or if `advise_if_enabled()` just came later and not everything was converted. In general though, other users of the default branch name advice message should probably follow the `--no-advice` option. I'll add another patch that does this in the next version. Thanks, -Justin
On 20/03/2025 23:48, Justin Tobler wrote: > On 25/03/20 11:10AM, Phillip Wood wrote: >> Hi Justin >> >> On 20/03/2025 01:46, Justin Tobler wrote: >>> >>> A side-effect of this change is that the location of the first >>> `git_default_branch_name()` gets deferred to a later point of execution. >>> This matters because `git_default_branch_name()` only computes the >>> default branch name once and returns a cached value for subsequent >>> invocations. After this change, the `git_default_branch_name()` call >>> site that actually computes the value becomes `guess_remote_head()` and >>> is configured to always show the advice message. >> >> Isn't the fundamental cause of this bug that advise() ignores GIT_ADVICE? >> I'm not really clear why "git --no-advice" only applies to advice that is >> guarded by advice_enabled() when it is documented as disabling all advice >> hints. > > From my point of view, this advice message should never be presented in > this particular scenario regardless of the configuration as it doesn't > make much sense here. This also happens to be the original behavior so I > think we should probably return to that state. Oh, good point. So the original report actually highlights two bugs - the regression you're fixing here and the fact that "--no-advice" does not suppress the advice. > I was also a bit suprised to see that not all advice messages respect > the `--no-advice` option. I'm not sure if there is a reason for this, or if > `advise_if_enabled()` just came later and not everything was converted. > > In general though, other users of the default branch name advice message > should probably follow the `--no-advice` option. I'll add another patch > that does this in the next version. That would be great, Thanks Phillip > Thanks, > -Justin >
It has been reported[1] that starting in Git v2.45.0, cloning from a bundle results in the default branch name advice message always being displayed when it was previously not. It can be reproduced by the following: git init bundle-repo && git -C bundle-repo --allow-empty -m init && git -C bundle-repo bundle create ../repo.bundle --all && git clone bundle.repo bundle-clone This issue bisects to 199f44cb2ead (builtin/clone: allow remote helpers to detect repo, 2024-02-27) where the refdb is partially initialized during clones before remote helpers are executed. This partial initilization creates the HEAD file and "refs/" directory. A side-effect of this change is that the location of the first `git_default_branch_name()` gets deferred to a later point of execution. This matters because `git_default_branch_name()` only computes the default branch name once and returns a cached value for subsequent invocations. After this change, the `git_default_branch_name()` call site that actually computes the value becomes `guess_remote_head()` and is configured to always show the advice message. Also, `guess_remote_head()` only invokes `git_default_branch_name()` in cases where the transport is unable to figure out the remote HEAD and must guess. This explains why the advice message gets printed for bundle clones, but not all clones. This series addresses the issue by adapting `guess_remote_head()` to support configuring the underlying `git_default_branch_name()`, which has since been renamed to `repo_default_branch_name()`, to be quiet and suppress the advice message. Thanks, -Justin [1]: <7EC98E2F-144D-4974-94F6-FC24B443651D@norbauer.com> Justin Tobler (2): remote: allow `guess_remote_head()` to suppress advice clone: suppress unexpected default branch advice builtin/clone.c | 4 ++-- builtin/fetch.c | 2 +- builtin/remote.c | 2 +- remote.c | 4 ++-- remote.h | 2 +- 5 files changed, 7 insertions(+), 7 deletions(-) base-commit: 683c54c999c301c2cd6f715c411407c413b1d84e