Bug 225986 – [git-webkit] Add support for native git commands (log/blame/show/etc.)

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Dean Johnson]

git-webkit is our primary way for working with identifiers. We should support using identifiers with native git commands and annotating identifiers where hashes/revisions exist in output of git commands like `git log`, `git blame`, etc.

[Dean Johnson]

Created attachment 429099 [details]
v1

Couple notes on this patch:
- This introduces a robust caching layer for conversions between identifiers, hashes and revisions. We may want to introduce / review that code separately.
- There are still a couple failing tests that seems related to the cache.
- New tests should be written for git_fallback both in making sure the original commands work, and for annotation.
- We need to support showing the correct error message when a git command fails (currently it shows nothing).

Since the mocking code needs additional updates to support the caching layer, uploading the patch as-is so Jonathan and I can more easily iterate on it together.

[Jonathan Bedard]

Comment on attachment 429099 [details]
v1

View in context: https://bugs.webkit.org/attachment.cgi?id=429099&action=review

Wanted to call out a few things that jumped out to me on a first-pass.

Big picture, we want to split this change up into at least 2 parts: the cache and adding the new commands. As a side note, it should be pretty straight forward to test the filter `git log` given that we have already done the heavy-lifting to mock the output of `git log`

> Tools/Scripts/libraries/webkitscmpy/webkitscmpy/local/git.py:182
> +                        print('[{}] Processed 1000 entries for branch {}'.format(round(time.time(), 2), branch))

This shows up way too many times in unit test output. Haven't thought too much about if we want it at all, but it certainly shouldn't be a print statement, given we do configure logging for this module

> Tools/Scripts/libraries/webkitscmpy/webkitscmpy/mocks/local/git.py:203
> +                ) if self.branch else mocks.ProcessCompletion(returncode=128),

self.branch can never be be none under the current implementation. I think this should be this:

mocks.Subprocess.Route(
    self.executable, 'branch', '--show-current',
    cwd=self.path,
    generator=lambda *args, **kwargs: mocks.ProcessCompletion(
        returncode=0,
        stdout='{}\n'.format('' if self.detached else self.branch),
    )

Also pretty certain the fact that this will return a branch even when the head is detached is the cause of at least one of your test failures.

> Tools/Scripts/libraries/webkitscmpy/webkitscmpy/mocks/local/git.py:206
> +                self.executable, 'branch',

We should just remove the `-a` from the existing `self.executable, 'branch', '-a'`, I think

> Tools/Scripts/libraries/webkitscmpy/webkitscmpy/program/git_fallback.py:133
> +        with tempfile.TemporaryFile(suffix=f'git-{cls.name}') as f:

We still need Python 2 compatibility :(

> Tools/Scripts/libraries/webkitscmpy/webkitscmpy/test/canonicalize_unittest.py:39
> +            shutil.rmtree(self.path)

Should all of our SCM test classes share a base class so we aren't duplicating this?

Also, if I recall correctly, the mock git repo is basing the repository name off of the last folder. So, if we make this:

def setUp(self):
    self.container = tempfile.mkdtemp()
    self.path = os.join(self.container, 'repository')
    os.mkdir(self.path)

def tearDown(self):
    if os.path.exists(self.container) and self.container.startswith('/tmp'):
        shut.rmtree(self.container)

we wouldn't need to change tests.

> Tools/Scripts/webkitpy/common/checkout/scm/svn.py:103
> +            return False

This bit is causing a failure in webkitpy.common.checkout.scm.detection_unittest.SCMDetectorTest.test_detect_scm_system, I think. It seems likely it's the test that's wrong, not your code. But in either case, this change (and the accompanying test change) should probably be separated from the rest of the cache work.

[Dean Johnson]

(In reply to Jonathan Bedard from comment #2)
> Comment on attachment 429099 [details]
> v1
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=429099&action=review
> 
> Wanted to call out a few things that jumped out to me on a first-pass.
> 
> Big picture, we want to split this change up into at least 2 parts: the
> cache and adding the new commands. As a side note, it should be pretty
> straight forward to test the filter `git log` given that we have already
> done the heavy-lifting to mock the output of `git log`
> 
> > Tools/Scripts/libraries/webkitscmpy/webkitscmpy/local/git.py:182
> > +                        print('[{}] Processed 1000 entries for branch {}'.format(round(time.time(), 2), branch))
> 
> This shows up way too many times in unit test output. Haven't thought too
> much about if we want it at all, but it certainly shouldn't be a print
> statement, given we do configure logging for this module
Yeah, looks like I missed removing that.
> 
> > Tools/Scripts/libraries/webkitscmpy/webkitscmpy/mocks/local/git.py:203
> > +                ) if self.branch else mocks.ProcessCompletion(returncode=128),
> 
> self.branch can never be be none under the current implementation. I think
> this should be this:
> 
> mocks.Subprocess.Route(
>     self.executable, 'branch', '--show-current',
>     cwd=self.path,
>     generator=lambda *args, **kwargs: mocks.ProcessCompletion(
>         returncode=0,
>         stdout='{}\n'.format('' if self.detached else self.branch),
>     )
Technically this output would not match `git branch --show-current` -- if there is no branch there is no output. But I can update it with that in mind.
> 
> Also pretty certain the fact that this will return a branch even when the
> head is detached is the cause of at least one of your test failures.
> 
> > Tools/Scripts/libraries/webkitscmpy/webkitscmpy/mocks/local/git.py:206
> > +                self.executable, 'branch',
> 
> We should just remove the `-a` from the existing `self.executable, 'branch',
> '-a'`, I think
That results in different output -- adding `-a` causes the output to include remotes.
> 
> > Tools/Scripts/libraries/webkitscmpy/webkitscmpy/program/git_fallback.py:133
> > +        with tempfile.TemporaryFile(suffix=f'git-{cls.name}') as f:
> 
> We still need Python 2 compatibility :(
Will update.
> 
> > Tools/Scripts/libraries/webkitscmpy/webkitscmpy/test/canonicalize_unittest.py:39
> > +            shutil.rmtree(self.path)
> 
> Should all of our SCM test classes share a base class so we aren't
> duplicating this?
> 
> Also, if I recall correctly, the mock git repo is basing the repository name
> off of the last folder. So, if we make this:
> 
> def setUp(self):
>     self.container = tempfile.mkdtemp()
>     self.path = os.join(self.container, 'repository')
>     os.mkdir(self.path)
> 
> def tearDown(self):
>     if os.path.exists(self.container) and self.container.startswith('/tmp'):
>         shut.rmtree(self.container)
> 
> we wouldn't need to change tests.
I'll try this approach.
> 
> > Tools/Scripts/webkitpy/common/checkout/scm/svn.py:103
> > +            return False
> 
> This bit is causing a failure in
> webkitpy.common.checkout.scm.detection_unittest.SCMDetectorTest.
> test_detect_scm_system, I think. It seems likely it's the test that's wrong,
> not your code. But in either case, this change (and the accompanying test
> change) should probably be separated from the rest of the cache work.

I'll create a new bug w/ a patch for the mapping cache, but probably not by EOD.

[Radar WebKit Bug Importer]

<rdar://problem/78535096>

[Brent Fulgham]

Jonathan (4/4/2022):
Addressed this in https://commits.webkit.org/240074@main, marking this bug as resolved.