Merge analysis support for bare repos #5022

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!

tests/merge/analysis.c

tests/merge/workdir/analysis.c

tests/merge/analysis.h

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

tests/merge/analysis.c

+	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.

tests/merge/analysis.c

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

tests/merge/analysis.c Outdated Show resolved Hide resolved

tests/merge/analysis.c Outdated Show resolved Hide resolved

tests/merge/workdir/analysis.c Outdated Show resolved Hide resolved

tests/merge/analysis.h Show resolved Hide resolved

ethomson reviewed Jun 7, 2019

View reviewed changes

pks-t requested changes Jun 7, 2019

View reviewed changes

tests/merge/analysis.c Outdated Show resolved Hide resolved

tests/merge/analysis.c Outdated Show resolved Hide resolved

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merge analysis support for bare repos #5022

Merge analysis support for bare repos #5022

rcoup commented Mar 19, 2019 •

edited

Loading

tiennou left a comment

rcoup commented Mar 19, 2019

tiennou commented Mar 20, 2019

pks-t commented Mar 29, 2019

rcoup commented Jun 6, 2019

pks-t left a comment

rcoup commented Jun 7, 2019

ethomson Jun 7, 2019

ethomson commented Jun 7, 2019

pks-t left a comment •

edited

Loading

rcoup commented Jun 10, 2019 •

edited

Loading

rcoup commented Jun 10, 2019 •

edited

Loading

ethomson commented Jun 10, 2019

rcoup commented Jun 10, 2019

libgit2-azure-pipelines bot commented Jun 10, 2019

rcoup commented Jun 10, 2019

ethomson commented Jun 10, 2019

rcoup commented Jun 13, 2019

pks-t commented Jun 13, 2019

Merge analysis support for bare repos #5022

Merge analysis support for bare repos #5022

Conversation

rcoup commented Mar 19, 2019 • edited Loading

tiennou left a comment

Choose a reason for hiding this comment

rcoup commented Mar 19, 2019

tiennou commented Mar 20, 2019

pks-t commented Mar 29, 2019

rcoup commented Jun 6, 2019

pks-t left a comment

Choose a reason for hiding this comment

rcoup commented Jun 7, 2019

ethomson Jun 7, 2019

Choose a reason for hiding this comment

ethomson commented Jun 7, 2019

pks-t left a comment • edited Loading

Choose a reason for hiding this comment

rcoup commented Jun 10, 2019 • edited Loading

rcoup commented Jun 10, 2019 • edited Loading

ethomson commented Jun 10, 2019

rcoup commented Jun 10, 2019

libgit2-azure-pipelines bot commented Jun 10, 2019

rcoup commented Jun 10, 2019

ethomson commented Jun 10, 2019

rcoup commented Jun 13, 2019

pks-t commented Jun 13, 2019

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