Duplicate entries with the same repo nickname ends up cloning only one of them, without notifying so

[130s]

Problem

When there are multiple entries with the same nickname (?) I see only the last entry is cloned, which might be fine. But I do see it a problem that the tool doesn't let the user know there was a duplicate entry.

I just had an issue where a repo defined earlier than the last duplicated entry didn't get cloned, which I didn't notice for awhile.

Demo

foo.repos.

- git: {local-name: dirk-thomas/vcstool, uri: 'https://github.com/facebook/robolectric'}
- git: {local-name: dirk-thomas/vcstool, uri: 'https://github.com/dirk-thomas/vcstool'}

I do not see any mention about robolectric in the result below.

$ mkdir src && vcs import src < ./foo.repos 
=== src/dirk-thomas/vcstool (git) ===
Cloning into '.'...

$ cd src/dirk-thomas/vcstool/ && git remote -vv
origin  git@github.com:dirk-thomas/vcstool (fetch)
origin  git@github.com:dirk-thomas/vcstool (push)

$ apt-cache policy python3-vcstool
python3-vcstool:
  Installed: 0.2.15-1

[christophebedard]

But I do see it a problem that the tool doesn't let the user know there was a duplicate entry.

do you think it should simply print a warning or should it error out?

I think being able to concatenate two files (especially two rosinstall files) and knowing that the last instance takes precedence could be useful (similar to #148), assuming that the user is aware of it.

Example:

$ vcs import src < foo.repos 
Repository 'dirk-thomas/vcstool' is defined twice: using https://github.com/dirk-thomas/vcstool instead of https://github.com/facebook/robolectric
=== src/dirk-thomas/vcstool (git) ===
Cloning into '.'...

with just

diff --git a/vcstool/commands/import_.py b/vcstool/commands/import_.py
index 55b3e18..5093fe0 100644
--- a/vcstool/commands/import_.py
+++ b/vcstool/commands/import_.py
@@ -114,6 +114,14 @@ def get_repos_in_vcstool_format(repositories):
                     'information: %s' % (path, e)) + ansi('reset'),
                 file=sys.stderr)
             continue
+        if path in repos:
+            print(
+                ansi('yellowf') + (
+                    "Repository '%s' is defined twice: "
+                    'using %s instead of %s'
+                    % (path, repo['url'], repos[path]['url'])
+                ) + ansi('reset'),
+                file=sys.stderr)
         repos[path] = repo
     return repos

@@ -145,6 +153,14 @@ def get_repos_in_rosinstall_format(root):
                     'information: %s' % (path, e)) + ansi('reset'),
                 file=sys.stderr)
             continue
+        if path in repos:
+            print(
+                ansi('yellowf') + (
+                    "Repository '%s' is defined twice: "
+                    'using %s instead of %s'
+                    % (path, repo['url'], repos[path]['url'])
+                ) + ansi('reset'),
+                file=sys.stderr)
         repos[path] = repo
     return repos

[130s]

But I do see it a problem that the tool doesn't let the user know there was a duplicate entry.

do you think it should simply print a warning or should it error out?

• Error out? User can more easily be notified about the problem, e.g. CI where you don't open the log unless it fails.
• Print warning? Sounds like there are good usecases where failing the command would deminish the tool's usefulness.

Maybe hybrid? By default only print warning but with an option the tool can fail?

[christophebedard]

• Error out? User can more easily be notified about the problem, e.g. CI where you don't open the log unless it fails.
• Print warning? Sounds like there are good usecases where failing the command would deminish the tool's usefulness.

Maybe hybrid? By default only print warning but with an option the tool can fail?

I might be wrong, but with the current state of the tool, I can't really think of a situation where having duplicate entries is not an error, so I'd make it fail by default and have an option to just print a warning (or even just keep going without any warning).

This could change, for example with #148 or something else, but there isn't anything like that currently.

[130s]

I might be wrong, but with the current state of the tool, I can't really think of a situation where having duplicate entries is not an error

I think with my OP a user didn't have a way to get notified of non-standard condition.

[christophebedard]

I think with my OP a user didn't have a way to get notified of non-standard condition.

Yeah, sorry! By "error" I meant "bad situation/bad configuration/user error" so the fact that it doesn't currently throw an error in that case would be a bug.

[dirk-thomas]

The file format of the file foo.repos doesn't use the format fully supported by vcstool. Instead it uses the rosinstall_file_format which is only supported for backward compatibility during vcs import. The fact that this file allows duplicate entries like this is the reason why vcstool didn't adopt it in the first place.

I won't be able to spend any time on enhance the user experience for this compatibility format. If anyone would like to work on a patch, the logic for checking for duplicate entries should added within the get_repos_in_rosinstall_format function which reads this file format.