Issue 2311 - Track remote Git branches when required

ensure_dependencies.py

 #!/usr/bin/env python
 # coding: utf-8
 
 # This Source Code Form is subject to the terms of the Mozilla Public
 # License, v. 2.0. If a copy of the MPL was not distributed with this
 # file, You can obtain one at http://mozilla.org/MPL/2.0/.
 
 import sys
 import os
 import posixpath
@@ skipping 35 matching lines @@
   def get_revision_id(self, repo, rev=None):
     command = ["hg", "id", "--repository", repo, "--id"]
     if rev:
       command.extend(["--rev", rev])
 
     # Ignore stderr output and return code here: if revision lookup failed we
     # should simply return an empty string.
     result = subprocess.Popen(command, stdout=subprocess.PIPE, stderr=subprocess.PIPE).communicate()[0]
     return result.strip()
 
-  def pull(self, repo, rev):
+  def pull(self, repo):
     subprocess.check_call(["hg", "pull", "--repository", repo, "--quiet"])
 
   def update(self, repo, rev):
     subprocess.check_call(["hg", "update", "--repository", repo, "--quiet", "--check", "--rev", rev])
 
   def ignore(self, target, repo):
 
     if not self.istype(target):
 
       config_path = os.path.join(repo, ".hg", "hgrc")
@@ skipping 22 matching lines @@
   def clone(self, source, target):
     source = source.rstrip("/")
     if not source.endswith(".git"):
       source += ".git"
     subprocess.check_call(["git", "clone", "--quiet", source, target])
 
   def get_revision_id(self, repo, rev="HEAD"):
     command = ["git", "rev-parse", "--revs-only", rev + '^{commit}']
     return subprocess.check_output(command, cwd=repo).strip()
 
-  def pull(self, repo, rev):
+  def pull(self, repo):
+    # Fetch tracked branches, new tags and the list of available remote branches
     subprocess.check_call(["git", "fetch", "--quiet", "--all", "--tags"], cwd=repo)
-    subprocess.check_call(["git", "checkout", rev], cwd=repo)

[Sebastian Noack, 2015/04/30 17:54:34]

Wasn't the problem you try to address here, that branches other than master that
haven't been manually marked as tracking, aren't even fetched?

[kzar, 2015/04/30 19:31:46]

So it turns out the remote branches are fetched, the problem is that local
tracking branches aren't created for them. Checking out the revision here causes
a tracking branch of the same name to be created where relevant.

It seems to work pretty well, only the required remote branch is tracked and it
is only done when required (when the revision isn't resolved already).

[Sebastian Noack, 2015/04/30 20:53:57]

Isn't this call redundant with the one in Git.update()? I don't get how this
should change anything. Are you sure that this is the right thing to do here?

[kzar, 2015/05/01 09:43:55]

As of Git 1.6.6 [1] checking out a local branch when there is no local branch by
that name but there is a remote branch of that name creates a local tracking
branch before checking it out. We need to make sure the tracking branch is
created here, before the later checkout, because without it the revision_id
can't be resolved.

This approach seems to work fine, only difference really is that should the
revision not be found by this checkout a `subprocess.CalledProcessError`
exception will be thrown instead of the later "Failed to resolve revision"
exception. (If that's an issue I can catch the `subprocess.CalledProcessError`
here.)

The other difference is that the branch is checked out now, but that doesn't
really matter IMHO. If it works here the later one would have worked too.
(Actually the later check out uses the resolved revision ID instead of the
branch name, so they both do something. My point is that they either both work
or both fail.)

[1]
https://raw.githubusercontent.com/git/git/master/Documentation/RelNotes/1.6.6...

[Sebastian Noack, 2015/05/04 11:44:31]

If I understand the issue correctly, this change is rather pointless. As it
merely makes sure the Git.pull() fails if Git.update() would fail, to generate a
different output in an error scenario. But in the end of the day, either
ensure_dependency succeeds or fails regardless of this change. But on the other
hand this change would mess up the abstraction, and duplicates code that is even
executed redundantly.

[kzar, 2015/05/05 13:33:55]

Then you do not understand this change properly. Without this change if I set a
branch name as repository revision ensure_dependencies will fail.

The reason it fails is because we do not tell Git to checkout the revision until
we ensure it exists and that we can resolve it to a commit hash. The problem is
that the checkout in this case _causes_ the revision (branch name) to exist and
to be able to resolve to a commit hash.

We therefore need to attempt the checkout before we ensure the revision
resolves, hence my change.

If you think this change is redundant try it for yourself, create a Git
repository on Github or somewhere that contains a branch. Put the Git URL as the
dependency name and the branch name as the revision in your dependencies file
and see what the script does. Try it with and without my change, you will see
what I mean.

[Sebastian Noack, 2015/05/05 15:08:54]

Ok, got it. But this still seems like a silly hack to me. Why don't you just
change the git fetch command line to "git fetch --quiet origin <rev>" as I
initially suggested? Then we will explicitly fetch the revision we checkout in
the next step, without breaking the abstraction here.

[kzar, 2015/05/05 15:26:52]

The remote branch has already been fetched, we just need to create a tracking
branch for it. `git fetch origin branch-name` does not create a tracking branch
for the remote branch - `git checkout branch-name` does.

(Another way to create the tracking branch would be `git branch --track
branch-name origin/branch-name` but obviously that would fail if the specified
revision was a commit hash instead of a branch name, or if the tracking branch
had already been created. We could do it that way however and just ignore the
exceptions thrown I suppose.)

[Sebastian Noack, 2015/05/05 16:00:54]

I think I'd prefer that one. If something went wrong we still get an exception
indicating the error on "git fetch" or "git checkout". So It should be safe to
ignore errors here.

That way we don't need to mess with the abstraction at least. And .update()
won't behave incosistent for Mercurial and Git, unexpectly checking out a
revision on Git.

[Sebastian Noack, 2015/05/05 16:59:02]

But either way, this will only work if you specify a branch name. If you specify
a commit that isn't in any tracked branch or tag, it won't work as "git fetch
--all --tags" only fetches tracking branches and tags.

[Sebastian Noack, 2015/05/05 17:35:05]

After thinking a little more about the problem and discussing it with Wladimir,
we came to the conclusion that we have to implement Git.pull() like below, to
make it work like "hg pull" and to eliminate all limitations with git branches:

  # Make sure the local list of remote branches is up-to-date
  git fetch

  # Mark all remote branches as tracking
  for remote in `git branch --remotes`; do
    do git branch --track $remote
  done

  # Fetch all tracking branches and tags
  git fetch --all --tags

[kzar, 2015/05/06 11:04:44]

Done.

+    # Next we need to ensure all remote branches are tracked
+    newly_tracked = False
+    remotes = subprocess.check_output(["git", "branch", "--remotes"], cwd=repo)
+    for match in re.finditer(r"^\s*(origin/(\S+))$", remotes, re.M):
+      remote, local = match.groups()
+      with open(os.devnull, "wb") as devnull:
+        if subprocess.call(["git", "branch", "--track", local, remote],
+                           cwd=repo, stdout=devnull, stderr=devnull) == 0:
+          newly_tracked = True
+    # Finally fetch any newly tracked remote branches
+    if newly_tracked:
+      subprocess.check_call(["git", "fetch", "--quiet", "origin"], cwd=repo)
 
   def update(self, repo, rev):
     subprocess.check_call(["git", "checkout", "--quiet", rev], cwd=repo)
 
   def ignore(self, target, repo):
     module = os.path.relpath(target, repo)
     exclude_file = os.path.join(repo, ".git", "info", "exclude")
     _ensure_line_exists(exclude_file, module)
 
   def postprocess_url(self, url):
@@ skipping 49 matching lines @@
         if spec:
           result[key] = spec
     return result
   except IOError, e:
     if e.errno != errno.ENOENT:
       raise
     return None
 
 def safe_join(path, subpath):
   # This has been inspired by Flask's safe_join() function
-  forbidden = set([os.sep, os.altsep]) - set([posixpath.sep, None])
+  forbidden = {os.sep, os.altsep} - {posixpath.sep, None}
   if any(sep in subpath for sep in forbidden):
     raise Exception("Illegal directory separator in dependency path %s" % subpath)
 
   normpath = posixpath.normpath(subpath)
   if posixpath.isabs(normpath):
     raise Exception("Dependency path %s cannot be absolute" % subpath)
   if normpath == posixpath.pardir or normpath.startswith(posixpath.pardir + posixpath.sep):
     raise Exception("Dependency path %s has to be inside the repository" % subpath)
   return os.path.join(path, *normpath.split(posixpath.sep))
 
@@ skipping 41 matching lines @@
     revision = revisions[type]
   elif "*" in revisions:
     revision = revisions["*"]
   else:
     logging.warning("No revision specified for repository %s (type %s), skipping update" % (target, type))
     return
 
   resolved_revision = repo_types[type].get_revision_id(target, revision)
   if not resolved_revision:
     logging.info("Revision %s is unknown, downloading remote changes" % revision)
-    repo_types[type].pull(target, revision)
+    repo_types[type].pull(target)
     resolved_revision = repo_types[type].get_revision_id(target, revision)
     if not resolved_revision:
       raise Exception("Failed to resolve revision %s" % revision)
 
   current_revision = repo_types[type].get_revision_id(target)
   if resolved_revision != current_revision:
     logging.info("Updating repository %s to revision %s" % (target, resolved_revision))
     repo_types[type].update(target, resolved_revision)
 
 def resolve_deps(repodir, level=0, self_update=True, overrideroots=None, skipdependencies=set()):
   config = read_deps(repodir)
   if config is None:
     if level == 0:
       logging.warning("No dependencies file in directory %s, nothing to do...\n%s" % (repodir, USAGE))
     return
   if level >= 10:
     logging.warning("Too much subrepository nesting, ignoring %s" % repo)
+    return
 
   if overrideroots is not None:
     config["_root"] = overrideroots
 
   for dir, revisions in config.iteritems():
     if dir.startswith("_") or revisions["_source"] in skipdependencies:
       continue
     target = safe_join(repodir, dir)
     ensure_repo(repodir, target, config.get("_root", {}), revisions["_source"])
     update_repo(target, revisions)
@@ skipping 43 matching lines @@
   args = parser.parse_args()
 
   if args.quiet:
     logging.disable(logging.INFO)
 
   repos = args.repos
   if not len(repos):
     repos = [os.path.dirname(__file__)]
   for repo in repos:
     resolve_deps(repo)