Test that the Sage git repo is clean.

[jhpalmieri]

Test that the Sage git repo is clean

Add a doctest which will check whether the Sage git repo is clean. The test is based on the top answer at https://unix.stackexchange.com/questions/155046/determine-if-git-working-directory-is-clean-from-a-script.

📝 Checklist

• The title is concise and informative.
• The description explains in detail what this PR is about.
• I have linked a relevant issue or discussion.
• I have created tests covering the changes.
• I have updated the documentation and checked the documentation preview.

⌛ Dependencies

[github-actions]

Documentation preview for this PR (built with commit d8dc279; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

[jhpalmieri]

I'm tempted to make this a blocker.

[dimpase]

I'm not sure this is an appropriate test to have among "normal" Sage tests. While it might be useful for CI, in development one often runs tests with uncommitted changes.

[dimpase]

By the way, did you see #39363 ?

[jhpalmieri]

I'm not sure this is an appropriate test to have among "normal" Sage tests. While it might be useful for CI, in development one often runs tests with uncommitted changes.

Maybe it's okay to be reminded in such situations that there are uncommitted changes. We have recently had at least two cases with beta releases where the git repo was left in a non-clean state. It is apparently not on the release manager's radar to check this, and some developers don't seem to check it either.

How else might we implement such a test?

[vbraun]

+1 to adding a test.

I'm fine with it being part of the doctests, though that means it runs somewhere in the middle of all tests

Alternative would be to use a make target like

.PHONY: test-git-no-uncommitted-changes
test-git-no-uncommitted-changes:
	@UNCOMMITTED=$$(git status --porcelain --untracked-files=no); \
	if [ -n "$$UNCOMMITTED" ]; then \
	    echo "git repo has uncommitted changes:"; \
	    echo "$$UNCOMMITTED"; \
	    echo; \
	    git diff; \
	    exit 1; \
	fi

and run it at the end of the test target ilke

test: all
        @echo '### make $(TEST_TARGET): Running $(TEST)' >> $(TEST_LOG)
        $(TEST)
        $(MAKE) test-git-no-uncommitted-changes

[dimpase]

Alternative would be to use a make target like

yes, this sounds like a better idea. As well, it would be great to actually list such files, as this may happen at CI's runs.

[jhpalmieri]

Yes, that looks good, and it's a great point about making sure this runs after all of the other tests. Two comments:

• running git diff opens up a pager for me, maybe because of my ~/.gitconfig, so in case others have similar settings, I would lean away from using that. Even without that, git diff could produce a lot of output, burying the key error message that the git repo is not clean.
• I would prefer that we run git status --porcelain without --untracked-files=no: we want to test whether someone has accidentally added files without tracking them, in addition to other errors. So I propose this:

diff --git a/Makefile b/Makefile
index 9990af924c..58d139a341 100644
--- a/Makefile
+++ b/Makefile
@@ -253,9 +253,20 @@ TEST_TARGET = $@
 
 TEST = ./sage -t --logfile=$(TEST_LOG) $(TEST_FLAGS) --optional=$(TEST_OPTIONAL) $(TEST_FILES)
 
+.PHONY: test-git-no-uncommitted-changes
+test-git-no-uncommitted-changes:
+       @UNCOMMITTED=$$(git status --porcelain); \
+       if [ -n "$$UNCOMMITTED" ]; then \
+           echo "git repo has uncommitted changes"; \
+           echo "$$UNCOMMITTED"; \
+           echo; \
+           exit 1; \
+       fi
+
 test: all
        @echo '### make $(TEST_TARGET): Running $(TEST)' >> $(TEST_LOG)
        $(TEST)
+       $(MAKE) test-git-no-uncommitted-changes
 
 check:
        @$(MAKE) test

[jhpalmieri]

Oh, and I think I need to change the lines

        $(TEST)
       $(MAKE) test-git-no-uncommitted-changes

to

       $(TEST); \
       $(MAKE) test-git-no-uncommitted-changes

to make sure both lines get executed; otherwise, if there are doctest failures, it bails out before running the last line. (And I regularly get doctest failures on OS X, and I want this change to be helpful even when that happens.)

[jhpalmieri]

Okay, here's a branch with the changes to Makefile.