Skip to content

tests: rewrite rebase-submodule .gitmodule file#4275

Merged
ethomson merged 2 commits into
libgit2:masterfrom
tiennou:fix-rebase-submodule-test
Jul 27, 2017
Merged

tests: rewrite rebase-submodule .gitmodule file#4275
ethomson merged 2 commits into
libgit2:masterfrom
tiennou:fix-rebase-submodule-test

Conversation

@tiennou

@tiennou tiennou commented Jun 20, 2017

Copy link
Copy Markdown
Contributor

Fixes #4274

@tiennou

tiennou commented Jun 20, 2017

Copy link
Copy Markdown
Contributor Author

Crossing fingers (a.k.a thanks Apple, for .DS_Store files) 🔥.

@tiennou tiennou force-pushed the fix-rebase-submodule-test branch 2 times, most recently from 383d593 to 1fc265e Compare June 20, 2017 20:57
@pks-t

pks-t commented Jun 21, 2017

Copy link
Copy Markdown
Member

The change looks good to me, thanks. There are more tests with a ".gitmodules" file, though, any particular reason why you didn't adjust them, as well?

./tests/resources/submodule_simple/.gitmodules
./tests/resources/submodule_with_path/.gitmodules
./tests/resources/submodules-worktree-parent/.gitmodules

@tiennou

tiennou commented Jun 21, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

The rationale is that rebase-submodule is the only one with a bogus url, which triggered the git CLI failure in the first place.

I'm still in a bit of a conundrum here, as even with that change, on a freshly cloned repository, I get

$ git submodule update --init
fatal: No url found for submodule path 'tests/resources/rebase-submodule/my-submodule' in .gitmodules

Note, this is not using --recursive (same with git submodule init), and there's no config section in the .git/config file. It seems to find the submodule using magic™ now 😞 .

@tiennou tiennou force-pushed the fix-rebase-submodule-test branch from 1fc265e to 7e71668 Compare June 21, 2017 12:58
@pks-t

pks-t commented Jun 27, 2017

Copy link
Copy Markdown
Member

Unfortunately, this breaks rebase::submodule::init_untracked. I guess we need to create the submodule during the test.

@tiennou tiennou force-pushed the fix-rebase-submodule-test branch from 7e71668 to 5cd7d42 Compare July 20, 2017 21:00
@tiennou

tiennou commented Jul 20, 2017

Copy link
Copy Markdown
Contributor Author

Gah, that gave me a hard time. Tests should be A-OK, but I'm unsure of the cleanliness of my fix. I had to reset after doing the commit, least the index would be weird.

@pks-t pks-t mentioned this pull request Jul 21, 2017
Closed
pks-t
pks-t requested changes Jul 21, 2017
View reviewed changes

@pks-t pks-t left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You forgot to free some variables in the initialization. You could also merge your two commits together and add some reasoning why this commit is required inside the commit message. Looks fine otherwise and passes ony my local machine, will merge when my comments are addressed. Thanks!

Comment thread tests/rebase/submodule.c Outdated

/* And a final reset, for good measure */
git_object_lookup(&obj, repo, &commit_id, GIT_OBJ_COMMIT);
cl_git_pass(git_reset(repo, obj, GIT_RESET_HARD, &opts));

@pks-t pks-t Jul 21, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This misses cleanup of some variables, namely parent, obj and master_ref.

@tiennou tiennou force-pushed the fix-rebase-submodule-test branch from 5cd7d42 to 35cb7b8 Compare July 25, 2017 18:38
@tiennou

tiennou commented Jul 25, 2017

Copy link
Copy Markdown
Contributor Author

@pks-t Fixed the missing frees, and the space-indent. Would it be possible to have that back-ported to 0.26 as well (this is against master), as it's intended to help downstream users/wrappers ?

@tiennou

tiennou commented Jul 25, 2017

Copy link
Copy Markdown
Contributor Author

Good to go, Travis choked on proxies again.

@ethomson ethomson merged commit 4f4bc57 into libgit2:master Jul 27, 2017
@tiennou tiennou deleted the fix-rebase-submodule-test branch August 9, 2017 19:08
@tiennou

tiennou commented Jan 12, 2018

Copy link
Copy Markdown
Contributor Author

@pks-t Can this be backported to 0.26.1 too ? Right now Objective-Git tracks "sometimes after 0.26" because of #4274.

@tiennou tiennou mentioned this pull request Jan 17, 2018
Merged
@pks-t pks-t added the backport label Jan 18, 2018
@carlosmn carlosmn mentioned this pull request Jan 4, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pks-t pks-t pks-t requested changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@tiennou @pks-t @ethomson