1313446 - |mach vendor rust| fails when inside a git worktree

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Kartikaya Gupta (email:[e-mail])]

If you're in a git worktree (which is a working directory tied to a repository) there is no .git directory. |mach vendor rust| relies on the existence of .git to detect that it's under git version control, and so this fails. Example output produced:

kats@kgupta-air mozilla-wr$ mach vendor rust
Error running mach:

    ['vendor', 'rust']

The error occurred in code that was called by the mach command. This is either
a bug in the called code itself or in the way that mach is calling it.

You should consider filing a bug for this issue.

If filing a bug, please include the full output of mach, including this error
message.

The details of the failure are as follows:

Exception: Unknown VCS, or not a source checkout: /Users/kats/zspace/mozilla-wr

  File "/Users/kats/zspace/mozilla-wr/python/mozbuild/mozbuild/mach_commands.py", line 1603, in vendor_rust
    vendor_command.vendor(**kwargs)
  File "/Users/kats/zspace/mozilla-wr/python/mozbuild/mozbuild/vendor_rust.py", line 61, in vendor
    self.check_modified_files()
  File "/Users/kats/zspace/mozilla-wr/python/mozbuild/mozbuild/vendor_rust.py", line 46, in check_modified_files
    modified = [f for f in self.repository.get_modified_files() if os.path.basename(f) not in ('Cargo.toml', 'Cargo.lock')]
  File "/Users/kats/zspace/mozilla-wr/python/mozbuild/mozbuild/util.py", line 943, in __get__
    setattr(instance, name, self.func(instance))
  File "/Users/kats/zspace/mozilla-wr/python/mozbuild/mozbuild/base.py", line 289, in repository
    return get_repository_object(self.topsrcdir)
  File "/Users/kats/zspace/mozilla-wr/python/mozversioncontrol/mozversioncontrol/__init__.py", line 105, in get_repository_object
    raise Exception('Unknown VCS, or not a source checkout: %s' % path)

Comment 1

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1313446 - Detect a git repository when inside a git worktree.

Review commit: https://reviewboard.mozilla.org/r/89010/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/89010/

Comment 2

[Gregory Szorc [:gps]]

Comment on attachment 8805236 [details]
Bug 1313446 - Detect a git repository when inside a git worktree.

https://reviewboard.mozilla.org/r/89010/#review88168

::: python/mozversioncontrol/mozversioncontrol/__init__.py:104
(Diff revision 1)
>      '''
>      if os.path.isdir(os.path.join(path, '.hg')):
>          return HgRepository(path)
>      elif os.path.isdir(os.path.join(path, '.git')):
>          return GitRepository(path)
> +    elif os.system('git rev-parse') == 0:

`os.system` should not be used because it executes in a shell. Executing sub-processes in a shell is considered a bad practice unless you need the shell.

::: python/mozversioncontrol/mozversioncontrol/__init__.py:104
(Diff revision 1)
> +    elif os.system('git rev-parse') == 0:
> +        return GitRepository(path)

We should refactor this along with the above 2 lines to be in a shared block that gets executed when the `.git` directory exists.

That being said, as long as we're not dealing with the internals of the .git directory, it is probably sufficient to just change the check to "is a .git path present".

Comment 3

[Mike Hommey [:glandium]]

Yeah, just replace isdir with exists.

Comment 4

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8805236 [details]
Bug 1313446 - Detect a git repository when inside a git worktree.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/89010/diff/1-2/

Comment 5

[Mike Hommey [:glandium]]

Comment on attachment 8805236 [details]
Bug 1313446 - Detect a git repository when inside a git worktree.

https://reviewboard.mozilla.org/r/89010/#review98004

::: python/mozversioncontrol/mozversioncontrol/__init__.py:102
(Diff revision 2)
>      '''Get a repository object for the repository at `path`.
>      If `path` is not a known VCS repository, raise an exception.
>      '''
>      if os.path.isdir(os.path.join(path, '.hg')):
>          return HgRepository(path)
> -    elif os.path.isdir(os.path.join(path, '.git')):
> +    elif os.path.exists(os.path.join(path, '.git')):

Mmmmmm I'm pretty sure I reviewed something similar a few weeks ago. This tells me we really need some abstraction around repositories instead of having to rediscover the same bugs again and again.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/febbbdc0273e
Detect a git repository when inside a git worktree. r=glandium

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/febbbdc0273e

Comment 8

[(not currently active) Ted Mielczarek]

(In reply to Mike Hommey [:glandium] from comment #5)
> Mmmmmm I'm pretty sure I reviewed something similar a few weeks ago. This
> tells me we really need some abstraction around repositories instead of
> having to rediscover the same bugs again and again.

When I wrote the `mach vendor rust` command I rewrote this mozversioncontrol code to try to be a central module for working with VCS. (There wasn't anything using it at the time.) If there are other places touching VCS we should switch them to using mozversioncontrol now.