Merge analysis support for bare repos by rcoup · Pull Request #5022 · libgit2/libgit2

rcoup · 2019-03-19T10:53:40Z

Fix for #5017

~~For testing, I copied tests/merge/workdir/analysis.c into tests/merge/trees/, and (manually) converted tests/resources/merge-resolve/ repo into a bare tests/resources/merge-resolve.git/ repo.~~

For testing, I refactored tests/merge/workdir/analysis.c into two matching test suites with a common implementation, one for working dirs and one for bare trees.

8/9 new tests fail without the change. The remaining one checks for unborn, which happens before the bare-repo check.
9/9 new tests pass with the change
no change to existing merge tests

tiennou

Hello @rcoup, thanks for the PR ! Your fix is fine with me, but I'm not fond of the duplicate tests, hence some remarks :

A "this has been duplicated as" comment at the top of both files would be a 👍, just so changes can be propagated.
It might be possible to bare-clone from the other repo when initializing the testcase, but I'm not sure about runtime speed.
Test case duplication : this has been bothering me as well (surprisingly, on the topic of bare repositories), but I don't think it can be done cleanly without changes to clar (something like RSpec's behaves). The only alternative I can think of is preprocessor tomfoolery (having a "shared" analysis.template, and include this from both .c files, switching the prefixes from the template, but meh).

I'm OK with merging it as is, I just want to fly those clar questions through maintainers 😉.

rcoup · 2019-03-19T22:30:01Z

I'm not fond of the duplicate tests, hence some remarks :

Yeah, I wasn't too excited about it either.

A "this has been duplicated as" comment at the top of both files would be a 👍, just so changes can be propagated.

Can do this easily.

It might be possible to bare-clone from the other repo when initializing the testcase, but I'm not sure about runtime speed.

Any examples in the test suite for doing this? I guess it's just git_clone(..., {bare=1}) but I'll need some pointers to figure out the path/tempdir handling in the tests.

Test case duplication ... I don't think it can be done cleanly without changes to clar

backs away slowly 🐉🐍

Alternatively as a simple starting point before clar supports parameterisation I could probably move all the analysis test bodies into merge/analysis.c and call them from the workdir/analysis.c & trees/analysis.c versions with the right setup?

tiennou · 2019-03-20T12:59:55Z

Any examples in the test suite for doing this? I guess it's just git_clone(..., {bare=1}) but I'll need some pointers to figure out the path/tempdir handling in the tests.

initialize

#define ANALYZED_REPO "merge-analysis-bare.git"

/* cl_sandbox setup, pretty standard, using the non-bare version */

git_clone_options opts = GIT_CLONE_OPTIONS_INIT;
opts.bare = 1;

git_clone(ANALYZED_REPO, &opts);

cleanup

git_futils_rmdir_r(ANALYZED_REPO);

Code mildly brain-compiled. Helpful "low-level" utilities can be found in futils.c/h, path.c/h, good-ol' buffer.c/h, as well as the clar headers.

Alternatively as a simple starting point before clar supports parameterisation […]

Yeah, I'd much prefer having a clean implementation if that's something we ever want. For now, a documentation comment is sufficient in my eyes. Again, thanks for doing this !

pks-t · 2019-03-29T13:33:13Z

You could just use git_repository_open_ext(&repo, "/path/to/sandbox/.git", GIT_REPOSITORY_OPEN_BARE, NULL); instead of doing a copy.

rcoup · 2019-06-06T15:36:31Z

@tiennou sorry, been distracted by some other work. I've refactored the analysis test implementation into one file and removed the duplicate test data (I went with @pks-t's suggestion).

pks-t

Some nits with regards to style, but otherwise I'm fine with this PR. Thanks for amending!

rcoup · 2019-06-07T15:40:12Z

I addressed all those.

I can't quite spot my error which is triggering valgrind though. Any obvious insight?

ethomson · 2019-06-07T18:12:25Z

+	git_merge_analysis_t merge_analysis;
+	git_merge_preference_t merge_pref;
+
+	git_repository_config(&config, repo);


You're not freeing this...

ethomson · 2019-06-07T18:13:32Z

I can't quite spot my error which is triggering valgrind though. Any obvious insight?

Not sure offhand. But something is probably incrementing the refcount on the repository, so your call to repository_free isn't actually disposing it (just reducing the refcount).

My guess is that it's that git_repository_config but I'm not 100%.

pks-t

I can't quite spot my error which is triggering valgrind though. Any obvious insight?

Not sure offhand. But something is probably incrementing the refcount on the repository, so your call to repository_free isn't actually disposing it (just reducing the refcount).
My guess is that it's that git_repository_config but I'm not 100%.

It is. All leaks point to the repository configuration snapshots, which are leaked in merge::analysis::no_fastforward_with_config_ffonly and merge::analysis::fastforward_with_config_noff. Sorry for not spotting these in my previous review.

rcoup · 2019-06-10T09:57:48Z

Aha, it had passed CI before my test refactoring which had confused me, I assumed it was related to the init/reorg. <looking in the wrong place> Thanks 😄

OT: Valgrind on OSX (3.14.0/Homebrew) reports a pile of leaks, even when using tests/valgrind-supp-mac.txt & libgit2_clar.supp, looks like mostly from dyld/ImageLoader. And doesn't report these test leaks, with or without the suppressions. Built with -DVALGRIND=ON. There some other spells I need to cast to make it work?

dupe of workdir/analysis.c against a bare repo.

- move duplication between merge/trees/ and merge/workdir/ into merge/analysis{.c,.h} - remove merge-resolve.git resource, open the existing merge-resolve as a bare repo instead.

- whitespace -> tabs - comment style - improve repo naming in merge/trees/analysis tests.

Wrap some missed setup api calls in asserts.

rcoup · 2019-06-10T10:22:39Z

Rebased onto newer master

ethomson · 2019-06-10T10:41:37Z

OT: Valgrind on OSX (3.14.0/Homebrew) reports a pile of leaks, even when using tests/valgrind-supp-mac.txt & libgit2_clar.supp, looks like mostly from dyld/ImageLoader. And doesn't report these test leaks, with or without the suppressions. Built with -DVALGRIND=ON. There some other spells I need to cast to make it work?

Although we checked in valgrind-supp-mac.txt, I haven't made valgrind work correctly on macOS in years. (valgrind itself says that the latest supported version is macOS 10.12.)

We use leaks on macOS if you have Xcode installed (which I presume that you do). You can take a look at the CI test script to see how we invoke it:

MallocStackLogging=1 MallocScribble=1 MallocLogFile=/dev/null CLAR_AT_EXIT=\"leaks -quiet \$PPID\" libgit2_clar

rcoup · 2019-06-10T10:47:59Z

/rebuild

libgit2-azure-pipelines · 2019-06-10T10:48:06Z

Okay, @rcoup, I started to rebuild this pull request as build #2028.

rcoup · 2019-06-10T10:56:35Z

We use leaks on macOS if you have Xcode installed (which I presume that you do). You can take a look at the CI test script to see how we invoke it:

@ethomson aha, victory! that works (I've never used leaks before, only valgrind). Is it worth me adding that to docs somewhere? tests/README?

ethomson · 2019-06-10T11:00:42Z

Is it worth me adding that to docs somewhere? tests/README?

Whoa, that would be great. 🙇

rcoup · 2019-06-13T09:38:12Z

@pks-t you happy for this to be merged now?

pks-t · 2019-06-13T09:42:29Z

Looks good to me now, thanks for your work @rcoup! I'll do a follow-up to make use of our new test variants so that we can avoid the duplication, but seeing as that only got merged a few days ago I don't want to shift that burden on you.

tiennou approved these changes Mar 19, 2019

View reviewed changes

pks-t requested changes Jun 7, 2019

View reviewed changes

Comment thread tests/merge/analysis.c Outdated

Comment thread tests/merge/analysis.c Outdated

Comment thread tests/merge/workdir/analysis.c Outdated

Comment thread tests/merge/analysis.h

ethomson reviewed Jun 7, 2019

View reviewed changes

pks-t requested changes Jun 7, 2019

View reviewed changes

Comment thread tests/merge/analysis.c Outdated

Comment thread tests/merge/analysis.c Outdated

rcoup added 6 commits June 10, 2019 11:22

merge: analysis support for bare repositories

6d2ab2c

merge: tests for bare repo merge analysis

1d04f47

dupe of workdir/analysis.c against a bare repo.

merge: add doc header to analysis tests

5427461

Refactor testing:

7b27b6c

- move duplication between merge/trees/ and merge/workdir/ into merge/analysis{.c,.h} - remove merge-resolve.git resource, open the existing merge-resolve as a bare repo instead.

Review fixes:

21ddeab

- whitespace -> tabs - comment style - improve repo naming in merge/trees/analysis tests.

Fix memleaks in analysis tests.

438c995

Wrap some missed setup api calls in asserts.

rcoup force-pushed the merge-analysis-bare-repo-5017 branch from e406132 to 438c995 Compare June 10, 2019 10:22

rcoup mentioned this pull request Jun 10, 2019

Add memleak check docs #5104

Merged

pks-t approved these changes Jun 13, 2019

View reviewed changes

pks-t merged commit 0c1029b into libgit2:master Jun 13, 2019

rcoup deleted the merge-analysis-bare-repo-5017 branch June 13, 2019 09:42

snyk-bot mentioned this pull request Feb 23, 2020

[Snyk] Upgrade nodegit from 0.4.1 to 0.26.4 saurabharch/Breezeblocks#1

Open

ethomson mentioned this pull request Apr 1, 2020

support merge analysis for bare repos #5017

Closed

snyk-bot mentioned this pull request Apr 22, 2020

[Snyk] Upgrade nodegit from 0.24.3 to 0.26.5 aminatakonate000/Graviton-App#4

Open

snyk-bot mentioned this pull request May 5, 2020

[Snyk] Upgrade nodegit from 0.24.3 to 0.26.5 Barnstorm-Online/ngp-openapi-generator#1

Open

Uh oh!

Conversation

rcoup commented Mar 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tiennou left a comment

Choose a reason for hiding this comment

Uh oh!

rcoup commented Mar 19, 2019

Uh oh!

tiennou commented Mar 20, 2019

Uh oh!

pks-t commented Mar 29, 2019

Uh oh!

rcoup commented Jun 6, 2019

Uh oh!

pks-t left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rcoup commented Jun 7, 2019

Uh oh!

ethomson Jun 7, 2019

Choose a reason for hiding this comment

Uh oh!

ethomson commented Jun 7, 2019

Uh oh!

pks-t left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

rcoup commented Jun 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rcoup commented Jun 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ethomson commented Jun 10, 2019

Uh oh!

rcoup commented Jun 10, 2019

Uh oh!

libgit2-azure-pipelines Bot commented Jun 10, 2019

Uh oh!

rcoup commented Jun 10, 2019

Uh oh!

ethomson commented Jun 10, 2019

Uh oh!

rcoup commented Jun 13, 2019

Uh oh!

pks-t commented Jun 13, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

rcoup commented Mar 19, 2019 •

edited

Loading

pks-t left a comment •

edited

Loading

rcoup commented Jun 10, 2019 •

edited

Loading

rcoup commented Jun 10, 2019 •

edited

Loading