1377 - Blacklist dependencies from SCM tracking

ensure_dependencies.py

Index: ensure_dependencies.py
===================================================================
--- a/ensure_dependencies.py
+++ b/ensure_dependencies.py
@@ -67,6 +67,10 @@
   def update(self, repo, rev):
     subprocess.check_call(["hg", "update", "--repository", repo, "--quiet", "--check", "--rev", rev])
 
+  def ignore(self, target):
+    if not self.istype(target):
+      logging.warning("Mercurial won't ignore dependency %s by default. You should do it manually." % (target))

[Wladimir Palant, 2014/09/23 15:04:13]

Quite the opposite actually - Mercurial will automatically ignore other
repositories. So nothing needs to be done here, this method should simply say
"pass."

Btw, just for reference as this isn't necessary:
http://mercurial.selenic.com/wiki/TipsAndTricks#Ignore_files_in_local_working...

[mathias, 2014/09/23 16:20:05]

Mercurial does not ignore Git submodules (just verified that again). And it's
quite possible to have an HG root repo but a Git submodule, especially when
using both HG and Git together in the root repo (which is, again, quite common)
or just synchronizing repos, debugging dependencies or whatever.

We are aware that Mercurial ignores Mercurial repositories in the working tree
though, which is why the "if not self.istype(target)" condition is in there. But
I assume that the use of the word "dependency" is a bit misleading, and the "You
should..." part is overhead.

[Wladimir Palant, 2014/09/25 19:52:20]

You are right, so a local hgignore file should be the way to go here as well...

[mathias, 2014/09/26 05:06:39]

Thank you for the "local" keyword! I've been searching for something similar to
.git/info/exclude in HG, but couldn't find anything before trying it again with
that one in addition.. sometimes it may be too simple I guess :)

[mathias, 2014/09/28 00:24:25]

Done.

+
 class Git():
   def istype(self, repodir):
     return os.path.exists(os.path.join(repodir, ".git"))
@@ -87,6 +91,18 @@
   def update(self, repo, rev):
     subprocess.check_call(["git", "checkout", "--quiet", rev], cwd=repo)
 
+  def ignore(self, target):
+    module = str(target).replace(os.getcwd(),'')[1:]

[Wladimir Palant, 2014/09/23 15:04:13]

There is a reason why this script never relies on the current work directory -
we don't know what the current work directory is. Even if you run
ensure_dependencies.py without parameters - it applies to the repository in
current work directory initially but it will recurse to other repositories after
that, located in other directories.

This function should have both repo and target as parameters. And a relative
path is being determined by calling os.path.relpath(), not by doing some
adventurous string replacements.

[mathias, 2014/09/23 16:20:05]

Agreed, will be addressed in the next patch-set.

[mathias, 2014/09/28 00:24:25]

Done.

+    exclude_file = '.git/info/exclude'
+    if os.path.exists(exclude_file):

[Wladimir Palant, 2014/09/23 15:04:13]

1) Please don't use relative file paths (see above).
2) Please don't assume Unix-style paths.

This should be:

  exclude_file = os.path.join(repo, ".git", "info", "exclude")

[mathias, 2014/09/23 16:20:05]

Agreed, will be addressed in the next patch-set.

[mathias, 2014/09/28 00:24:25]

Done.

+      with open(exclude_file, 'r') as f:
+        exclude_file_content = f.read().splitlines()

[Wladimir Palant, 2014/09/23 15:04:13]

I'd rather do [l.strip() for l in f.readlines()] here - this will remove other
whitespace as well, not just newlines.

[mathias, 2014/09/23 16:20:05]

Agreed, will be addressed in the next patch-set.

[mathias, 2014/09/28 00:24:25]

Done.

+        if not module in exclude_file_content:
+          logging.warning("Adding dependency %s to %s" % (module, exclude_file) )

[Wladimir Palant, 2014/09/23 15:04:13]

1) Why is this a warning? Nothing is wrong, a completely normal operation is
being performed - we call logging.info() for those.

2) This message is very misleading, what's being added here definitely isn't a
dependency.

3) The repository classes merely abstract away repository specifics, any
informational messages should be produced by the code using them.

Luckily, this message should no longer be necessary if this is called by
ensure_repo().

[mathias, 2014/09/23 16:20:05]

Agreed, we just remove the line entirely.

[mathias, 2014/09/28 00:24:25]

Done.

+          exclude_file_content = open(exclude_file, 'a')

[Wladimir Palant, 2014/09/23 15:04:13]

This will fail on Windows - you are opening a file for writing that is still
open for reading, file locking won't allow it. You should really only open this
file once, file mode "r+" - this will allow you overwriting the file contents
after reading it.

Bonus obfuscation points for overwriting a string variable with a file handle.

[mathias, 2014/09/23 16:20:05]

Agreed, will be addressed in the next patch-set.

[mathias, 2014/09/28 00:24:25]

Done.

+          exclude_file_content.write("\n"+module)

[Wladimir Palant, 2014/09/23 15:04:13]

What if there is already a line break at the end of the file? It's better to
overwrite the file completely - and make sure there is always a line break at
the end of file while at it.

  exclude_file.seek(0)
  exclude_file.truncate(0)
  exclude_file.write("\n".join(exclude_file_content) + "\n")

[mathias, 2014/09/23 16:20:05]

Agreed, will be addressed in the next patch-set.

[mathias, 2014/09/28 00:24:25]

Done.

+          exclude_file_content.close()

[Wladimir Palant, 2014/09/23 15:04:13]

If your code throws an exception that file will never be closed - that's why we
use the with statement.

[mathias, 2014/09/23 16:20:05]

Agreed, will be addressed as well.

[mathias, 2014/09/28 00:24:25]

Done.

+
 repo_types = {
   "hg": Mercurial(),
   "git": Git(),
@@ -220,6 +236,9 @@
     ensure_repo(repodir, target, config.get("_root", {}), revisions["_source"])
     update_repo(target, revisions)
     resolve_deps(target, level + 1, self_update=False, overrideroots=overrideroots, skipdependencies=skipdependencies)
+    for repo in repo_types.itervalues():
+      if repo.istype(repodir):
+        repo.ignore(target)

[Wladimir Palant, 2014/09/23 15:04:13]

IMHO, this should be part of ensure_repo():

1) The repository type is known there, no need to call get_repo_type() again.

2) If we didn't create the repository, we shouldn't touch the configuration for
it.

3) The recursive call is being performed once we are finished processing a
repository - it should stay that way.

[mathias, 2014/09/23 16:20:05]

Valid point, we move it to `ensure_repo()`. Note, however, that
`get_repo_type()` is not called in this part, because it considers all repo
types (the parallel use thingy, again).

[mathias, 2014/09/28 00:24:25]

Done.

 
   if self_update and "_self" in config and "*" in config["_self"]:
     source = safe_join(repodir, config["_self"]["*"])