{"thread":{"id":"57696","subject":"[PATCH] Makefile: add a prerequisite to the coverage-report target","startedAt":"2022-04-08T10:54:57Z","lastAt":"2022-04-09T03:52:03Z","messageCount":3,"participants":["Elia Pinto","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"453326","messageId":"20220408105443.192217-1-gitter.spiros@gmail.com","threadId":"57696","inReplyTo":null,"subject":"[PATCH] Makefile: add a prerequisite to the coverage-report target","fromName":"Elia Pinto","fromEmail":"gitter.spiros@gmail.com","sentAt":"2022-04-08T10:54:43Z","receivedAt":"2022-04-08T10:54:57Z","isPatch":true,"sender":{"key":"gitter.spiros@gmail.com","avatar":"https://avatars.githubusercontent.com/u/158490?v=4"},"body":"Directly invoking make coverage-report as a target results in an error because\nits prerequisites are missing,\n\nThe patch adds the necessary prerequisite.\n\nSigned-off-by: Elia Pinto <gitter.spiros@gmail.com>\n---\n Makefile | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/Makefile b/Makefile\nindex e8aba291d7..d9e8dd331e 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -3409,7 +3409,7 @@ coverage-prove: coverage-clean-results coverage-compile\n \t\tDEFAULT_TEST_TARGET=prove GIT_PROVE_OPTS=\"$(GIT_PROVE_OPTS) -j1\" \\\n \t\t-j1 test\n \n-coverage-report:\n+coverage-report: coverage-test\n \t$(QUIET_GCOV)for dir in $(object_dirs); do \\\n \t\t$(GCOV) $(GCOVFLAGS) --object-directory=$$dir $$dir*.c || exit; \\\n \tdone\n-- \n2.35.1\n\n"},{"id":"453350","messageId":"xmqqpmlrl4j2.fsf@gitster.g","threadId":"57696","inReplyTo":"20220408105443.192217-1-gitter.spiros@gmail.com","subject":"Re: [PATCH] Makefile: add a prerequisite to the coverage-report target","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-04-08T20:10:57Z","receivedAt":"2022-04-08T20:11:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elia Pinto <gitter.spiros@gmail.com> writes:\n\n> @@ -3409,7 +3409,7 @@ coverage-prove: coverage-clean-results coverage-compile\n>  \t\tDEFAULT_TEST_TARGET=prove GIT_PROVE_OPTS=\"$(GIT_PROVE_OPTS) -j1\" \\\n>  \t\t-j1 test\n>  \n> -coverage-report:\n> +coverage-report: coverage-test\n>  \t$(QUIET_GCOV)for dir in $(object_dirs); do \\\n>  \t\t$(GCOV) $(GCOVFLAGS) --object-directory=$$dir $$dir*.c || exit; \\\n>  \tdone\n\nI am not sure if this alone makes it a good change.\n\nWith the current set-up, you can run coverage-test or coverage-prove\nonce, view coverage-report, shift your attention to elsewhere to\naddress issues you saw in the report, and then decide to view the\nreport again to remind yourself what you saw and what motivated you\nto work on your changes.  Most likely this sequence would be\nfollowed by another run of coverage-test followed by coverage-report\nto compare what you saw before you made these changes with the\ncovernage report after your changes.\n\nIf this were conditional, i.e. \"ah, I see you haven't run any\ncoverage test yet, so let me run it for you before showing the\nresult\" combined with \"ok, I see you did run coverage test, so let\nme just show the result without running tests anew\", then it would\nbe a strict improvement from the status quo.\n\nBut with the patch as posted, wouldn't we unconditonally wipe the\nearlier result out?  If so that may negatively affect established\nworkflow of some people, I am afraid.\n\nThanks.\n"},{"id":"453361","messageId":"20220409035145.1256570-1-gitter.spiros@gmail.com","threadId":"57696","inReplyTo":"xmqqpmlrl4j2.fsf@gitster.g","subject":"[PATCH v2 1/1] Makefile: add a prerequisite to the coverage-report target","fromName":"Elia Pinto","fromEmail":"gitter.spiros@gmail.com","sentAt":"2022-04-09T03:51:45Z","receivedAt":"2022-04-09T03:52:03Z","isPatch":true,"sender":{"key":"gitter.spiros@gmail.com","avatar":"https://avatars.githubusercontent.com/u/158490?v=4"},"body":"Directly invoking make coverage-report as a target results in an error because\nits prerequisites are missing,\n\nThis patch adds the compile-test prerequisite, which is run only once each time\nthe compile-report target is invoked. In practice, the developer may decide to\nreview the coverage-report results without necessarily rerunning for this\ncoverage-test, if it has already been run.\n\nSigned-off-by: Elia Pinto <gitter.spiros@gmail.com>\n---\nThis is the second version of the patch.\n\nWith respect to the first version, we tried to eliminate the inefficient\ncoverage-test invocation if the target is coverage-report, introducing a more\nuseful invocation order\n\n Makefile | 9 ++++++++-\n 1 file changed, 8 insertions(+), 1 deletion(-)\n\ndiff --git a/Makefile b/Makefile\nindex e8aba291d7..eacdffd748 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -3378,6 +3378,8 @@ check-builtins::\n .PHONY: coverage-untested-functions cover_db cover_db_html\n .PHONY: coverage-clean-results\n \n+coverage-test.file?=coverage-test.file\n+\n coverage:\n \t$(MAKE) coverage-test\n \t$(MAKE) coverage-untested-functions\n@@ -3389,6 +3391,7 @@ coverage-clean-results:\n \t$(RM) coverage-untested-functions\n \t$(RM) -r cover_db/\n \t$(RM) -r cover_db_html/\n+\t$(RM) -f $(coverage-test.file)\n \n coverage-clean: coverage-clean-results\n \t$(RM) $(addsuffix *.gcno,$(object_dirs))\n@@ -3404,12 +3407,16 @@ coverage-test: coverage-clean-results coverage-compile\n \t$(MAKE) CFLAGS=\"$(COVERAGE_CFLAGS)\" LDFLAGS=\"$(COVERAGE_LDFLAGS)\" \\\n \t\tDEFAULT_TEST_TARGET=test -j1 test\n \n+$(coverage-test.file):\n+\t@make coverage-test\n+\ttouch $(coverage-test.file)\n+\n coverage-prove: coverage-clean-results coverage-compile\n \t$(MAKE) CFLAGS=\"$(COVERAGE_CFLAGS)\" LDFLAGS=\"$(COVERAGE_LDFLAGS)\" \\\n \t\tDEFAULT_TEST_TARGET=prove GIT_PROVE_OPTS=\"$(GIT_PROVE_OPTS) -j1\" \\\n \t\t-j1 test\n \n-coverage-report:\n+coverage-report: $(coverage-test.file)\n \t$(QUIET_GCOV)for dir in $(object_dirs); do \\\n \t\t$(GCOV) $(GCOVFLAGS) --object-directory=$$dir $$dir*.c || exit; \\\n \tdone\n-- \n2.35.1\n\n"}]}