Use default branch for git and git_subdir resources with no revision #2663
Conversation
| download_(Dir, {git, Url, ""}, State) -> | ||
| ?WARN("WARNING: It is recommended to use {branch, Name}, {tag, Tag} or {ref, Ref}, otherwise updating the dep may not work as expected.", []), |
There was a problem hiding this comment.
I'd keep the warnings. It is probably a good idea to make dep updates readable/predictable for people looking at the source config, regardless of the lock files.
There was a problem hiding this comment.
Oh yea, definitely should keep that warning.
There was a problem hiding this comment.
But in that case, what shall be the "clean" (no-warning) way to depend on the default branch for a Git repo? To specify {ref, "origin/HEAD"} or "origin/HEAD" (old-rebar-compatible way) explicitly? The latter also generates a warning. In addition, both ways definitely do not upgrade as expected: needs_update() always returns true for any symbolic ref (HEAD, origin/HEAD, etc), because it doesn't bother to resolve the symbolic ref to the actual commit hash that it has in the lock file, and ends up comparing apples to oranges. So rebar3 upgrade may fail or rewrite local uncommitted changes even when there are no changes upstream. (Shall I fix that in a separate commit?) On the contrary, omitting the ref altogether now works as expected (more or less) wrt rebar3 upgrade: it will only try to check out when there are some changes upstream. So IMO the warning (the way it was originally phrased) is no longer relevant.
There was a problem hiding this comment.
The clean way would be to check what the default branch is. There may be a few different used most often, I know I've seen at least develop, master, main, but actually being changed is almost never?
There was a problem hiding this comment.
But I've seen repositories where the default branch has changed (old branch was no longer present), which broke our builds (example at the top of the referenced issue: worker_pool). The main intent of this PR is to cleanly avoid breakage in such cases, and that is to point a Git dependency to the default upstream branch, whatever it is at the moment. I understand that deleting or renaming default branches is largely the fault/bad practice on that part of the repository owner (if you change your default branch, you should at least keep the old branch for compatibility) -- but that is, of course, outside of our control.
There was a problem hiding this comment.
And I spoke too soon -- in the new default branch case, there still remained 2 scenarios not handled consistently with the usual {branch, Name} case, namely:
- When there is no
rebar.lockand there are locally committed changes which are not present upstream, I believe the expectation is thatrebar3 upgradeshould not reset the HEAD to upstream (albeit there are neither test cases nor documentation for this, so I cannot be sure what the expectation is). - When the
originURL has changed, upgrade should always be performed.
This I fixed, so I think now the default branch case would be handled consistently with {branch, Name}.
There was a problem hiding this comment.
I don't necessarily worry about 1, since we can't realistically support people changing deps in the _build directory with no other indications and have never planned to (_checkout deps are there for that use case).
As for 2, yeah that should more or less always happen, and I believe the update comparison steps do compare URLs for that to happen.
That being said, I'd still want the warning to be in place. It is a reasonable one to keep around mostly because we also can't provide any guarantees that the rebar3 copy you're using is as up-to-date as the copy other users may be using and for the foreseeable future. We have to account for the fact that various teams or groups of contributors are going to be working with mixed versions where the usage of a default branch will not be consistent across everyone.
As such, even if the next rebar3 release is safely using origin/HEAD, you may still end up with contributors who don't and for whom the default upgrade mechanism will default to master regardless of having switched to main with both branch available, and will yield surprising upgrade or fetching behaviours. In a wider ecosystem, it remains true that {branch, Name}, {tag, Tag} or {ref, Ref} is safer than specifying nothing.
There was a problem hiding this comment.
Ah -- that makes sense. Reinstated the warning with appropriate phrasing.
Where git revision is not specified for a dependency, check out and upgrade from the default branch (origin/HEAD) instead of the hardcoded `master`, consistently with rebar 2. Mostly to support GitHub repos where the default branch has been (re)named `main` or something else.
Where default git branch is used, commits added locally to the branch
should not prompt `rebar3 upgrade` to reset the local branch to remote
when no remote changes are present (consistently with normal
`{branch, Name}` handling)
New features: - [Add --offline option and REBAR_OFFLINE environment variable](#2643) - [Add support for project-local plugins](#2697) - [Add eunit --test flag](#2684) Experimental features for which we promise no backwards compatibility in the near future: - [Experimental vendoring provider](#2689) - [Support plugins in experimental vendor provider](#2702) Other changes: - [Support OTP 23..25 inclusively](#2706) - [Bump Relx to 4.7.0](#2718) - [Use `erlexec` directly in relx helper functions](erlware/relx#902) - [Make rlx_util:parse_vsn parse integer versions](erlware/relx#913) - [fix awk script check_name() in extended_bin](erlware/relx#915) - [avoid crash when overlay is malformed](erlware/relx#916) - [keep attributes when stripping beams](erlware/relx#906) - [Fix {include_erts,true} in Windows releases](erlware/relx#914) - [ensure the erl file is writable before copying dyn_erl to it](erlware/relx#903) - Various tests added - [Properly carry overlay_vars settings for files in relx](#2711) - [Track mib compilation artifacts](#2709) - [Attempt to find apps in git subdirs (sparse checkouts)](#2687) - [Do not discard parameters --system_libs and --include-erts when duplicate values exist](#2695) - [Use default `depth` parameter for SSL](#2690) - [Fix global cache config overriding](#2683) - [Error out on unknown templates in 'new' command](#2676) - [Fix a typo](#2674) - [Bump certifi to 2.9.0](#2673) - [add a debug message in internal dependency fetching code](#2672) - [Use SPDX id for license in template and test](#2668) - [Use default branch for git and git_subdir resources with no revision](#2663) Signed-off-by: Fred Hebert <mononcqc@ferd.ca>
Where git revision is not specified for a dependency, check out and upgrade from the default branch (origin/HEAD) instead of the hardcoded
master, consistently with rebar 2. Mostly to support GitHub repos where the default branch has been (re)namedmainor something else.Resolves #2651.