Issue 5934936779390976: 1377 - Blacklist dependencies from SCM tracking (Closed)
|
Created:
Sept. 23, 2014, 9:13 a.m. by mathias Modified:
Oct. 23, 2014, 9:36 a.m. CC:
paco, aalvz Visibility:
Public. |
Descriptionhttps://issues.adblockplus.org/ticket/1377
Patch Set 1 #
Total comments: 29
Patch Set 2 : 1377 - Blacklist dependencies from Mercurial and Git tracking #
Total comments: 17
Patch Set 3 : 1377 - Blacklist dependencies from SCM tracking #
Total comments: 2
Patch Set 4 : 1377 - Blacklist dependencies from SCM tracking #MessagesTotal messages: 17
http://codereview.adblockplus.org/5934936779390976/diff/5629499534213120/ensu... File ensure_dependencies.py (right): http://codereview.adblockplus.org/5934936779390976/diff/5629499534213120/ensu... ensure_dependencies.py:72: logging.warning("Mercurial won't ignore dependency %s by default. You should do it manually." % (target)) 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... http://codereview.adblockplus.org/5934936779390976/diff/5629499534213120/ensu... ensure_dependencies.py:95: module = str(target).replace(os.getcwd(),'')[1:] 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. http://codereview.adblockplus.org/5934936779390976/diff/5629499534213120/ensu... ensure_dependencies.py:97: if os.path.exists(exclude_file): 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") http://codereview.adblockplus.org/5934936779390976/diff/5629499534213120/ensu... ensure_dependencies.py:99: exclude_file_content = f.read().splitlines() I'd rather do [l.strip() for l in f.readlines()] here - this will remove other whitespace as well, not just newlines. http://codereview.adblockplus.org/5934936779390976/diff/5629499534213120/ensu... ensure_dependencies.py:101: logging.warning("Adding dependency %s to %s" % (module, exclude_file) ) 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(). http://codereview.adblockplus.org/5934936779390976/diff/5629499534213120/ensu... ensure_dependencies.py:102: exclude_file_content = open(exclude_file, 'a') 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. http://codereview.adblockplus.org/5934936779390976/diff/5629499534213120/ensu... ensure_dependencies.py:103: exclude_file_content.write("\n"+module) 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") http://codereview.adblockplus.org/5934936779390976/diff/5629499534213120/ensu... ensure_dependencies.py:104: exclude_file_content.close() If your code throws an exception that file will never be closed - that's why we use the with statement. http://codereview.adblockplus.org/5934936779390976/diff/5629499534213120/ensu... ensure_dependencies.py:241: repo.ignore(target) 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.
Instead of another patch-set, now just a set of comments because there is one issue mentioned in the review (the first) which is not quite clear yet. http://codereview.adblockplus.org/5934936779390976/diff/5629499534213120/ensu... File ensure_dependencies.py (right): http://codereview.adblockplus.org/5934936779390976/diff/5629499534213120/ensu... ensure_dependencies.py:72: logging.warning("Mercurial won't ignore dependency %s by default. You should do it manually." % (target)) On 2014/09/23 15:04:13, Wladimir Palant wrote: > 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... 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. http://codereview.adblockplus.org/5934936779390976/diff/5629499534213120/ensu... ensure_dependencies.py:95: module = str(target).replace(os.getcwd(),'')[1:] On 2014/09/23 15:04:13, Wladimir Palant wrote: > 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. Agreed, will be addressed in the next patch-set. http://codereview.adblockplus.org/5934936779390976/diff/5629499534213120/ensu... ensure_dependencies.py:97: if os.path.exists(exclude_file): On 2014/09/23 15:04:13, Wladimir Palant wrote: > 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") Agreed, will be addressed in the next patch-set. http://codereview.adblockplus.org/5934936779390976/diff/5629499534213120/ensu... ensure_dependencies.py:99: exclude_file_content = f.read().splitlines() On 2014/09/23 15:04:13, Wladimir Palant wrote: > I'd rather do [l.strip() for l in f.readlines()] here - this will remove other > whitespace as well, not just newlines. Agreed, will be addressed in the next patch-set. http://codereview.adblockplus.org/5934936779390976/diff/5629499534213120/ensu... ensure_dependencies.py:101: logging.warning("Adding dependency %s to %s" % (module, exclude_file) ) On 2014/09/23 15:04:13, Wladimir Palant wrote: > 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(). Agreed, we just remove the line entirely. http://codereview.adblockplus.org/5934936779390976/diff/5629499534213120/ensu... ensure_dependencies.py:102: exclude_file_content = open(exclude_file, 'a') On 2014/09/23 15:04:13, Wladimir Palant wrote: > 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. Agreed, will be addressed in the next patch-set. http://codereview.adblockplus.org/5934936779390976/diff/5629499534213120/ensu... ensure_dependencies.py:103: exclude_file_content.write("\n"+module) On 2014/09/23 15:04:13, Wladimir Palant wrote: > 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") Agreed, will be addressed in the next patch-set. http://codereview.adblockplus.org/5934936779390976/diff/5629499534213120/ensu... ensure_dependencies.py:104: exclude_file_content.close() On 2014/09/23 15:04:13, Wladimir Palant wrote: > If your code throws an exception that file will never be closed - that's why we > use the with statement. Agreed, will be addressed as well. http://codereview.adblockplus.org/5934936779390976/diff/5629499534213120/ensu... ensure_dependencies.py:241: repo.ignore(target) On 2014/09/23 15:04:13, Wladimir Palant wrote: > 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. 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).
http://codereview.adblockplus.org/5934936779390976/diff/5629499534213120/ensu... File ensure_dependencies.py (right): http://codereview.adblockplus.org/5934936779390976/diff/5629499534213120/ensu... ensure_dependencies.py:72: logging.warning("Mercurial won't ignore dependency %s by default. You should do it manually." % (target)) On 2014/09/23 16:20:05, matze wrote: > 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. You are right, so a local hgignore file should be the way to go here as well...
http://codereview.adblockplus.org/5934936779390976/diff/5629499534213120/ensu... File ensure_dependencies.py (right): http://codereview.adblockplus.org/5934936779390976/diff/5629499534213120/ensu... ensure_dependencies.py:72: logging.warning("Mercurial won't ignore dependency %s by default. You should do it manually." % (target)) On 2014/09/25 19:52:20, Wladimir Palant wrote: > On 2014/09/23 16:20:05, matze wrote: > > 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. > > You are right, so a local hgignore file should be the way to go here as well... 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 :)
http://codereview.adblockplus.org/5934936779390976/diff/5629499534213120/ensu... File ensure_dependencies.py (right): http://codereview.adblockplus.org/5934936779390976/diff/5629499534213120/ensu... ensure_dependencies.py:72: logging.warning("Mercurial won't ignore dependency %s by default. You should do it manually." % (target)) On 2014/09/26 05:06:39, matze wrote: > On 2014/09/25 19:52:20, Wladimir Palant wrote: > > On 2014/09/23 16:20:05, matze wrote: > > > 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. > > > > You are right, so a local hgignore file should be the way to go here as > well... > > 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 :) Done. http://codereview.adblockplus.org/5934936779390976/diff/5629499534213120/ensu... ensure_dependencies.py:95: module = str(target).replace(os.getcwd(),'')[1:] On 2014/09/23 15:04:13, Wladimir Palant wrote: > 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. Done. http://codereview.adblockplus.org/5934936779390976/diff/5629499534213120/ensu... ensure_dependencies.py:97: if os.path.exists(exclude_file): On 2014/09/23 15:04:13, Wladimir Palant wrote: > 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") Done. http://codereview.adblockplus.org/5934936779390976/diff/5629499534213120/ensu... ensure_dependencies.py:99: exclude_file_content = f.read().splitlines() On 2014/09/23 16:20:05, matze wrote: > On 2014/09/23 15:04:13, Wladimir Palant wrote: > > I'd rather do [l.strip() for l in f.readlines()] here - this will remove other > > whitespace as well, not just newlines. > > Agreed, will be addressed in the next patch-set. Done. http://codereview.adblockplus.org/5934936779390976/diff/5629499534213120/ensu... ensure_dependencies.py:101: logging.warning("Adding dependency %s to %s" % (module, exclude_file) ) On 2014/09/23 16:20:05, matze wrote: > On 2014/09/23 15:04:13, Wladimir Palant wrote: > > 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(). > > Agreed, we just remove the line entirely. Done. http://codereview.adblockplus.org/5934936779390976/diff/5629499534213120/ensu... ensure_dependencies.py:102: exclude_file_content = open(exclude_file, 'a') On 2014/09/23 15:04:13, Wladimir Palant wrote: > 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. Done. http://codereview.adblockplus.org/5934936779390976/diff/5629499534213120/ensu... ensure_dependencies.py:103: exclude_file_content.write("\n"+module) On 2014/09/23 16:20:05, matze wrote: > On 2014/09/23 15:04:13, Wladimir Palant wrote: > > 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") > > Agreed, will be addressed in the next patch-set. Done. http://codereview.adblockplus.org/5934936779390976/diff/5629499534213120/ensu... ensure_dependencies.py:104: exclude_file_content.close() On 2014/09/23 16:20:05, matze wrote: > On 2014/09/23 15:04:13, Wladimir Palant wrote: > > If your code throws an exception that file will never be closed - that's why > we > > use the with statement. > > Agreed, will be addressed as well. Done. http://codereview.adblockplus.org/5934936779390976/diff/5629499534213120/ensu... ensure_dependencies.py:241: repo.ignore(target) On 2014/09/23 16:20:05, matze wrote: > On 2014/09/23 15:04:13, Wladimir Palant wrote: > > 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. > > 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). Done.
http://codereview.adblockplus.org/5934936779390976/diff/5639274879778816/ensu... File ensure_dependencies.py (right): http://codereview.adblockplus.org/5934936779390976/diff/5639274879778816/ensu... ensure_dependencies.py:48: def _ignore(self, target, file_name): That should simply be a helper function _ensure_line_exists, no object inheritance necessary. http://codereview.adblockplus.org/5934936779390976/diff/5639274879778816/ensu... ensure_dependencies.py:55: if not last_character in "\r\n": No need to overcomplicate this: file_content.append(line) f.seek(0, os.SEEK_SET) for l in file_content: print >>f, l f.truncate() Yes, this will overwrite the entire file contents - but the performance difference shouldn't matter here. http://codereview.adblockplus.org/5934936779390976/diff/5639274879778816/ensu... ensure_dependencies.py:90: ignore_path = os.path.join(".", ".hg", "dependencies") Once again, no relative paths please - these are relative to the current work directory which can be anything. The ignore setting in hgrc has to be an absolute path, meaning: ignore_path = os.path.abspath(os.path.join(repo, ".hg", "dependencies")) http://codereview.adblockplus.org/5934936779390976/diff/5639274879778816/ensu... ensure_dependencies.py:103: ignore_path = config.get("ui", "ignore.dependencies") Please set that option regardless of whether it is already there - since it has to be an absolute path, it might be wrong if the repository moved. So you should always update it in order to have it point to the right location.
comment http://codereview.adblockplus.org/5934936779390976/diff/5639274879778816/ensu... File ensure_dependencies.py (right): http://codereview.adblockplus.org/5934936779390976/diff/5639274879778816/ensu... ensure_dependencies.py:48: def _ignore(self, target, file_name): On 2014/10/07 22:30:14, Wladimir Palant wrote: > That should simply be a helper function _ensure_line_exists, no object > inheritance necessary. Done. http://codereview.adblockplus.org/5934936779390976/diff/5639274879778816/ensu... ensure_dependencies.py:55: if not last_character in "\r\n": I think the solution you are suggesting and the current one are not that different and is not more natural in the first solution instead of writing and overwriting it all? :) http://codereview.adblockplus.org/5934936779390976/diff/5639274879778816/ensu... ensure_dependencies.py:90: ignore_path = os.path.join(".", ".hg", "dependencies") On 2014/10/07 22:30:14, Wladimir Palant wrote: > Once again, no relative paths please - these are relative to the current work > directory which can be anything. The ignore setting in hgrc has to be an > absolute path, meaning: > > ignore_path = os.path.abspath(os.path.join(repo, ".hg", "dependencies")) Done. http://codereview.adblockplus.org/5934936779390976/diff/5639274879778816/ensu... ensure_dependencies.py:103: ignore_path = config.get("ui", "ignore.dependencies") On 2014/10/07 22:30:14, Wladimir Palant wrote: > Please set that option regardless of whether it is already there - since it has > to be an absolute path, it might be wrong if the repository moved. So you should > always update it in order to have it point to the right location. Done.
@aalvz Please do not comment "Done" on review tickets others have created. You cannot publish another patch set for those anyway.
Hi Wladimir, please provide us with a bit more information for the first two issues you've raised for the last patch set (despite Aalvz' rash comments). Thank you! Matze http://codereview.adblockplus.org/5934936779390976/diff/5639274879778816/ensu... File ensure_dependencies.py (right): http://codereview.adblockplus.org/5934936779390976/diff/5639274879778816/ensu... ensure_dependencies.py:48: def _ignore(self, target, file_name): On 2014/10/07 22:30:14, Wladimir Palant wrote: > That should simply be a helper function _ensure_line_exists, no object > inheritance necessary. Please provide a rationale for your statement. I do know that inheritance is not the only possible way to go here, but I do not see why a global function "should" be preferred in this case. http://codereview.adblockplus.org/5934936779390976/diff/5639274879778816/ensu... ensure_dependencies.py:55: if not last_character in "\r\n": On 2014/10/07 22:30:14, Wladimir Palant wrote: > No need to overcomplicate this: > > file_content.append(line) > f.seek(0, os.SEEK_SET) > for l in file_content: > print >>f, l > f.truncate() > > Yes, this will overwrite the entire file contents - but the performance > difference shouldn't matter here. Please provide more information here as well. While I see that your suggested code is a bit easier to understand, I haven't figured why one would consider the current version "overcomplicated". By the way, the motivation is of course to avoid re-writing the entire file - not due to performance reasons (which I do not consider a priority for this script anyway), but in order to avoid any side-effects that are not necessary. http://codereview.adblockplus.org/5934936779390976/diff/5639274879778816/ensu... ensure_dependencies.py:90: ignore_path = os.path.join(".", ".hg", "dependencies") On 2014/10/07 22:30:14, Wladimir Palant wrote: > Once again, no relative paths please - these are relative to the current work > directory which can be anything. The ignore setting in hgrc has to be an > absolute path, meaning: > > ignore_path = os.path.abspath(os.path.join(repo, ".hg", "dependencies")) Agreed, this will be fixed in the next patch set. I've tested that one assuming the path would be relative to the repo dir (as with `.git/info/exclude`), but haven't realized that it only worked because I've been in the "correct" working directory. http://codereview.adblockplus.org/5934936779390976/diff/5639274879778816/ensu... ensure_dependencies.py:103: ignore_path = config.get("ui", "ignore.dependencies") On 2014/10/07 22:30:14, Wladimir Palant wrote: > Please set that option regardless of whether it is already there - since it has > to be an absolute path, it might be wrong if the repository moved. So you should > always update it in order to have it point to the right location. Agreed, this is necessary due to the issue above, and will be fixed in the next patch set. Still a bit awkward though..
http://codereview.adblockplus.org/5934936779390976/diff/5639274879778816/ensu... File ensure_dependencies.py (right): http://codereview.adblockplus.org/5934936779390976/diff/5639274879778816/ensu... ensure_dependencies.py:48: def _ignore(self, target, file_name): On 2014/10/08 16:32:11, matze wrote: > Please provide a rationale for your statement. I do know that inheritance is not > the only possible way to go here, but I do not see why a global function > "should" be preferred in this case. There is no inherited functionality here - there is merely a helper function currently used by both implementations (which is merely a coincidence). Inheritance is misleading here, and the current name of the function tells absolutely nothing about its functionality. http://codereview.adblockplus.org/5934936779390976/diff/5639274879778816/ensu... ensure_dependencies.py:55: if not last_character in "\r\n": On 2014/10/08 16:32:11, matze wrote: > Please provide more information here as well. While I see that your suggested > code is a bit easier to understand, I haven't figured why one would consider the > current version "overcomplicated". The current code makes lots of unnecessary assumptions: * File size is at least 1. * \r and \n always indicate line breaks and no other characters can. * Writing os.linesep to a file explicitly is a good idea (hint: it isn't, "Do not use os.linesep as a line terminator when writing files opened in text mode"). My suggested code leaves all the decisions to the Python interpreter. And - yes, overwriting the file contents will unify line breaks as a side-effect. It's a desired side-effect however.
Hello Wladimir, thank you for the information. I'll publish another patch-set soon, according to your feedback and my comments below. Cheers! Matze http://codereview.adblockplus.org/5934936779390976/diff/5639274879778816/ensu... File ensure_dependencies.py (right): http://codereview.adblockplus.org/5934936779390976/diff/5639274879778816/ensu... ensure_dependencies.py:48: def _ignore(self, target, file_name): On 2014/10/08 16:46:49, Wladimir Palant wrote: > On 2014/10/08 16:32:11, matze wrote: > > Please provide a rationale for your statement. I do know that inheritance is > not > > the only possible way to go here, but I do not see why a global function > > "should" be preferred in this case. > > There is no inherited functionality here - there is merely a helper function > currently used by both implementations (which is merely a coincidence). > Inheritance is misleading here, and the current name of the function tells > absolutely nothing about its functionality. Fair enough, I understand your arguments, and especially your point of the name not being verbose enough is something I can agree on. For the alleged lack of inherited functionality resp. inheritance being misleading I do not really agree: It's common to SCM to feature ignore files in *similar* but not *equal* fashion. This requires for individual abstraction, so it cannot be recommended to perform changes from the outside of the SCM context. Therefore, the functionality should be placed somewhere it is - by convention - not accessed directly. The leading underscore makes it quite clear that the functionality is not to be used elsewhere, and since this is not a module (where one could easily introduce a context for internal utilities that is not considered overhead) but a script I do not see a better context to place it. Consider the following method declaration: def _ensure_line_exits(self, ignore_file, pattern): Since argument names do matter in Python, this should make the purpose perfectly clear. Well, I do realize that discussing these points could be considered to over-complicate the matter. But the implementation in inheritance fashion was not something I thought would ever require being justified, since I do consider it a best practice when working with classes anyway. The next patch set (unless I receive an update of yours before) will feature this version. If you still insist on a global function that'll be a minor change afterwards, however. http://codereview.adblockplus.org/5934936779390976/diff/5639274879778816/ensu... ensure_dependencies.py:55: if not last_character in "\r\n": On 2014/10/08 16:46:49, Wladimir Palant wrote: > On 2014/10/08 16:32:11, matze wrote: > > Please provide more information here as well. While I see that your suggested > > code is a bit easier to understand, I haven't figured why one would consider > the > > current version "overcomplicated". > > The current code makes lots of unnecessary assumptions: > > * File size is at least 1. > * \r and \n always indicate line breaks and no other characters can. > * Writing os.linesep to a file explicitly is a good idea (hint: it isn't, "Do > not use os.linesep as a line terminator when writing files opened in text > mode"). > > My suggested code leaves all the decisions to the Python interpreter. And - yes, > overwriting the file contents will unify line breaks as a side-effect. It's a > desired side-effect however. While the first point is not really true (though it's not obvious that the outer `if` block ensures the file having a size of 1 or more), the second being arguable and the third easy to fix,.. I do see that leaving these caveats for the Python interpreter to handle is indeed a better choice, especially when unifying the line-breaks is classified a "desired" side-effect. Will become adjusted in the next patch set.
Modifications since last patch-set: 54f4ca0 1377 - Use relpath to ignore dependencies for mercurial c74f192 1377 - Write ui config section for ignore file in mercurial everytime no matter what 92a75e0 1377 - Rewrite SCM._ignore() as SCM._ensure_line_exists() 6e36624 1377 - Use Python builtins to write HG ignore file 808f878 1377 - Move *.ignore() invocations from ensure_repo() to resolve_deps() 72cfd98 1377 - Undo whitespace modification without context
http://codereview.adblockplus.org/5934936779390976/diff/5639274879778816/ensu... File ensure_dependencies.py (right): http://codereview.adblockplus.org/5934936779390976/diff/5639274879778816/ensu... ensure_dependencies.py:48: def _ignore(self, target, file_name): On 2014/10/08 17:37:55, matze wrote: > For the alleged lack of inherited functionality resp. inheritance being > misleading I do not really agree: It's common to SCM to feature ignore files in > *similar* but not *equal* fashion. And the same file format? That's merely a coincidence that you can reuse the same code here. This is a helper function with no relation to revision control systems. Once again, using inheritance here implies similarities that don't exist in reality. http://codereview.adblockplus.org/5934936779390976/diff/5738600293466112/ensu... File ensure_dependencies.py (right): http://codereview.adblockplus.org/5934936779390976/diff/5738600293466112/ensu... ensure_dependencies.py:48: def _ensure_line_exists(self, ignore_file, pattern): The parameter should be called path, not ignore_file - this is a generic helper function, without any real relation to ignore files. http://codereview.adblockplus.org/5934936779390976/diff/5738600293466112/ensu... ensure_dependencies.py:261: repo.ignore(target, repodir) Why was this code moved again? As noted before, it should only run when we create the repository.
Changes since patch-set #2: 54f4ca0 1377 - Use relpath to ignore dependencies for mercurial c74f192 1377 - Write ui config section for ignore file in mercurial everytime no matter what 92a75e0 1377 - Rewrite SCM._ignore() as SCM._ensure_line_exists() 6e36624 1377 - Use Python builtins to write HG ignore file 808f878 1377 - Move *.ignore() invocations from ensure_repo() to resolve_deps() 72cfd98 1377 - Undo whitespace modification without context 1cb9e26 1377 - Ignore files call moved to ensure_repo be7a5ac 1377 - Renamed variable ignore_file to a most accurate one (path) 5affa21 1377 - Fix function. Truncate before write. To avoid seek lose problem. d789654 1377 - Drop SCM class in favor of global function
LGTM 
|
