Skip to content

Fixed git-subrepo and tests if set ff=only #640

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

spog
Copy link
Contributor

@spog spog commented Nov 16, 2024

Some tests failed, when ff_only is set in users git configuration; like:
...
test/branch-rev-list-one-path.t .... Died at line 23 in main of test/branch-rev-list-one-path.t
test/branch-rev-list-one-path.t .... No subtests run
test/branch-rev-list.t ............. Died at line 26 in main of test/branch-rev-list.t
test/branch-rev-list.t ............. No subtests run ...

and git configuration is set like:
$ git config --global pull.ff only
$ git config --global merge.ff only

There may be a similar cause of failure described in issue 550.

@spog spog force-pushed the ff_only branch 2 times, most recently from 6add0dc to 05034b1 Compare November 23, 2024 17:56
spog added 4 commits December 1, 2024 20:09
Some tests failed, when ff_only is set in users git configuration;
like:
...
test/branch-rev-list-one-path.t .... Died at line 23 in main of test/branch-rev-list-one-path.t
test/branch-rev-list-one-path.t .... No subtests run
test/branch-rev-list.t ............. Died at line 26 in main of test/branch-rev-list.t
test/branch-rev-list.t ............. No subtests run
...

and git configuration is set like:
    $ git config --global pull.ff only
    $ git config --global merge.ff only
Copy link
Collaborator

@admorgan admorgan left a comment

Choose a reason for hiding this comment

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

I don't feel this is the best option for testing this feature. I believe that it would be sufficient to have a specific test that incorporates the fast forward only instead of setting it for all tests.

@@ -46,6 +46,7 @@ help:

.PHONY: test
test:
@echo uname: '$(shell uname)'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why did you feel this would beneficial?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi and thanks for the question. Namely, while i was wrestling with github pipelines for Linux and Macos, i found this additional pipeline output very useful, especially in PR #639. So i thought i should leave it in this PR as well. There is no other value to it otherwise.

else

# Real GIT configuration for tests is set here:
rm -f "${PROJ_DIR}/.gitconfig"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am not ok with removing the .gitconfig from a person's working directory.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi again,
i thought i made sure, that HOME is temporarily set to $PWD in 'test/setup' for the duration of running tests and only when git-subrepo's clone directory (PROJ_DIR) and HOME match, then rewrite tests-defined git configuration in PROJ_DIR.
regards, Samo

@spog
Copy link
Contributor Author

spog commented Feb 19, 2025

I don't feel this is the best option for testing this feature. I believe that it would be sufficient to have a specific test that incorporates the fast forward only instead of setting it for all tests.

That may certainly be true, however i wanted to provide a way to define/set default tests-specific git configuration and which is independent of current user's git config,

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants