[depot_tools] auto correct url always

[1] was originally added by an [ancient CL] in 2013.
It runs `git config ...` and checks if if the output is not "False".

It has two problems.
1) `git config` returns False only if the value was set as a text value,
   AND the value is read as a text value. If "False" is set with --bool,
   the value is "false", even if it's read as a text value.

   from https://git-scm.com/docs/git-config
   bool: canonicalize values as either "true" or "false".

   In other words, the existing check would work only if someone sets
   it as a text value with "False".

2) the inline comment, describing the intention, is confusing.
   "Skip url auto-correction if gclient-auto-fix-url is set"

   It's not clear whether it's set to any value? false? true?
   Nevertheless, what the code actually does is to auto-correct the URL,
   unless the config is set to False, which would unlikely be true, due
   to (1). To make it work, someone needs to set it with "False" as
   a text value, but "False" is not even a valid string value for
   the corresponding legit boolean value that git config --bool defines.

Therefore, depot_tools had auto-corrected the URL, if the config is
- set to "False" as a boolean value
- set to true, false, bla, foo, bar, or any value
  other than "False" as a text value.
OR
- unset

That's how the code had behaved from 2013 to 2024.

In Feb 2024, https://crrev.com/c/5280779 changed the behaviour.
It replaces the dubious check with GetConfigBool(), which may sound
correct by itself.
i.e., auto-correct if "remote.origin.gclient-auto-fix-url" is set to
"true".

However, it doesn't match the existing behaviour, which
auto-correct the URL regardless of the config. Depot tools had the
behaviour for 11 years. Fixing it is probably not the best decision.

This CL doesn't exactly recover the existing behaviour. Instead,
it removes the check. As a result, even if someone sets the config
with "False" as a text value, depot_tools will auto-correct the URL.

[1]: 3b9212b7ee:gclient_scm.py;l=764-768
[ancient CL]: https://chromiumcodereview.appspot.com/15325003

Bug: 342445995
Change-Id: Idc62f803f2ef1beb11a93a58432867ce16226da7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/5572721
Reviewed-by: Josip Sokcevic <sokcevic@chromium.org>
Reviewed-by: Yiwei Zhang <yiwzhang@google.com>
Commit-Queue: Scott Lee <ddoman@chromium.org>
changes/21/5572721/12
Scott Lee 1 year ago committed by LUCI CQ
parent 0a4852e19d
commit 8ff5d0caee

@ -835,16 +835,11 @@ class GitWrapper(SCMWrapper):
return_early = False
# TODO(maruel): Delete url != 'git://foo' since it's just to make the
# unit test pass. (and update the comment above)
# Skip url auto-correction if remote.origin.gclient-auto-fix-url is set.
# This allows devs to use experimental repos which have a different url
# but whose branch(s) are the same as official repos.
strp_url = url[:-4] if url.endswith('.git') else url
strp_current_url = current_url[:-4] if current_url.endswith(
'.git') else current_url
if (strp_current_url.rstrip('/') != strp_url.rstrip('/')
and url != 'git://foo' and scm.GIT.GetConfigBool(
self.checkout_path,
f'remote.{self.remote}.gclient-auto-fix-url')):
and url != 'git://foo'):
self.Print('_____ switching %s from %s to new upstream %s' %
(self.relpath, current_url, url))
if not (options.force or options.reset):

Loading…
Cancel
Save