ci: include a Visual Studio build & test in our Azure Pipeline

Git's Continuous Integration (CI) includes an Azure Pipeline that builds Git on Linux, macOS and Windows, in the former two cases even in multiple configurations (using GCC vs clang, 32-bit vs 64-bit, etc). On Windows, we only build using GCC, using (a subset of) Git for Windows' SDK.

Recently, a patch series made it into Git that re-instates the ability to generate project files for use with Visual Studio. The idea there being:
contributors can check out a special branch that has those generated files in one generated commit on top of e.g. Git for Windows' master, allowing the contributors to build Git in Visual Studio, without the need for downloading Git for Windows' SDK (which weighs quite a bit: ~600MB download, ~2GB disk footprint). The tests can then be run from a regular Git for Windows Bash.

This patch series adds that axis to Git's Azure Pipeline: the project files are generated, MSBuild (which is kind of the command-line equivalent of Visual Studio's "Build" operation) is used to build Git, and then a parallelized test job runs the test suite in a Portable Git.

These patches are based on js/visual-studio.

Johannes Schindelin (13):
push: do not pretend to return int from die_push_simple()
msvc: avoid using minus operator on unsigned types
winansi: use FLEX_ARRAY to avoid compiler warning compat/win32/path-utils.h: add #include guards
msvc: ignore some libraries when linking
msvc: handle DEVELOPER=1
msvc: work around a bug in GetEnvironmentVariable()
vcxproj: only copy git-remote-http.exe once it was built
vcxproj: include more generated files
test-tool run-command: learn to run (parts of) the testsuite
tests: let --immediate and --write-junit-xml play well together
ci: really use shallow clones on Azure Pipelines
ci: also build and test with MS Visual Studio on Azure Pipelines

 Makefile                                   |   4 +
 azure-pipelines.yml                        | 164 ++++++++++++++++++++-
 builtin/push.c                             |   4 +-
 compat/mingw.c                             |   2 +
 compat/vcbuild/scripts/clink.pl            |  48 +++++-
 compat/win32/path-utils.h                  |   5 +
 compat/winansi.c                           |   2 +-
 config.mak.uname                           |  19 ++-
 contrib/buildsystems/Generators/Vcxproj.pm |   3 +
 read-cache.c                               |   4 +-
 sha1-lookup.c                              |   2 +-
 t/helper/test-run-command.c                | 153 +++++++++++++++++++
 t/test-lib.sh                              |  37 +++--
 13 files changed, 415 insertions(+), 32 deletions(-)

base-commit: aac6ff7b5beeea9bca66ecda6eec60fc1dd447ec
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-288%2Fdscho%2Fazure-pipelines-msvc-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-288/dscho/azure-pipelines-msvc-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/288

gitgitgadget

From: Johannes Schindelin <[email protected]>

This function is marked as NORETURN, and it indeed does not want to return anything. So let's not declare it with the return type int. This fixes the following warning when building with MSVC:

C4646: function declared with 'noreturn' has non-void return type

---
 builtin/push.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index 021dd3b1e4..d216270d5f 100644
@@ -143,8 +143,8 @@ static int push_url_of_remote(struct remote *remote, const char ***url_p)
 	return remote->url_nr;
 }
 
-static NORETURN int die_push_simple(struct branch *branch,
-				    struct remote *remote)
+static NORETURN void die_push_simple(struct branch *branch,
+				     struct remote *remote)
 {
 	/*
 	 * There's no point in using shorten_unambiguous_ref here,

gitgitgadget

From: Johannes Schindelin <[email protected]>

MSVC complains about this with -Wall, which can be taken as a sign that this is indeed a real bug. The symptom is:

C4146: unary minus operator applied to unsigned type, result still unsigned

Let's avoid this warning in the minimal way, e.g. writing -1 - <unsigned value> instead of -<unsigned value> - 1.

Note that the change in the estimate_cache_size() function is needed because MSVC considers the "return type" of the sizeof() operator to be size_t, i.e. unsigned, and therefore it cannot be negated using the unary minus operator.

Even worse, that arithmetic is doing extra work, in vain. We want to calculate the entry extra cache size as the difference between the size of the cache_entry structure minus the size of the ondisk_cache_entry structure, padded to the appropriate alignment boundary.

To that end, we start by assigning that difference to the per_entry variable, and then abuse the len parameter of the align_padding_size() macro to take the negative size of the ondisk entry size. Essentially, we try to avoid passing the already calculated difference to that macro by passing the operands of that difference instead, when the macro expects operands of an addition:

	#define align_padding_size(size, len) \
		((size + (len) + 8) & ~7) - (size + len)

Currently, we pass A and -B to that macro instead of passing A - B and 0, where A - B is already stored in the per_entry variable, ready to be used.

This is neither necessary, nor intuitive. Let's fix this, and have code that is both easier to read and that also does not trigger MSVC's warning.

---
 read-cache.c  | 4 ++--
 sha1-lookup.c | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

index c701f7f8b8..11f3357216 100644
@@ -1276,7 +1276,7 @@ static int add_index_entry_with_check(struct index_state *istate, struct cache_e
 	 */
 	if (istate->cache_nr > 0 &&
 		strcmp(ce->name, istate->cache[istate->cache_nr - 1]->name) > 0)
-		pos = -istate->cache_nr - 1;
+		pos = -1 - istate->cache_nr;
 	else
 		pos = index_name_stage_pos(istate, ce->name, ce_namelen(ce), ce_stage(ce));
 

@@ -1894,7 +1894,7 @@ static size_t estimate_cache_size(size_t ondisk_size, unsigned int entries)
 	/*
 	 * Account for potential alignment differences.
 	 */
-	per_entry += align_padding_size(sizeof(struct cache_entry), -sizeof(struct ondisk_cache_entry));
+	per_entry += align_padding_size(per_entry, 0);
 	return ondisk_size + entries * per_entry;
 }
 

index 796ab68da8..c819687730 100644
@@ -97,7 +97,7 @@ int sha1_pos(const unsigned char *sha1, void *table, size_t nr,
 			lo = mi + 1;
 		mi = lo + (hi - lo) / 2;
 	} while (lo < hi);
-	return -lo-1;
+	return -1 - lo;
 }
 
 int bsearch_hash(const unsigned char *sha1, const uint32_t *fanout_nbo,

gitgitgadget

From: Johannes Schindelin <[email protected]>

MSVC would complain thusly:

C4200: nonstandard extension used: zero-sized array in struct/union

Let's just use the FLEX_ARRAY constant that we introduced for exactly this type of scenario.

---
 compat/winansi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index cacd82c833..54fd701cbf 100644
@@ -546,7 +546,7 @@ static HANDLE swap_osfhnd(int fd, HANDLE new_handle)
 typedef struct _OBJECT_NAME_INFORMATION
 {
 	UNICODE_STRING Name;
-	WCHAR NameBuffer[0];
+	WCHAR NameBuffer[FLEX_ARRAY];
 } OBJECT_NAME_INFORMATION, *POBJECT_NAME_INFORMATION;
 
 #define ObjectNameInformation 1

gitgitgadget

From: Johannes Schindelin <[email protected]>

This adds the common guards that allow headers to be #include'd multiple times.

---
 compat/win32/path-utils.h | 5 +++++
 1 file changed, 5 insertions(+)

index 0f70d43920..8ed062a6b7 100644
@@ -1,3 +1,6 @@
+#ifndef WIN32_PATH_UTILS_H
+#define WIN32_PATH_UTILS_H
+
 #define has_dos_drive_prefix(path) \
 	(isalpha(*(path)) && (path)[1] == ':' ? 2 : 0)
 int win32_skip_dos_drive_prefix(char **path);

@@ -18,3 +21,5 @@ static inline char *win32_find_last_dir_sep(const char *path)
 #define find_last_dir_sep win32_find_last_dir_sep
 int win32_offset_1st_component(const char *path);
 #define offset_1st_component win32_offset_1st_component
+
+#endif

gitgitgadget

From: Johannes Schindelin <[email protected]>

To build with MSVC, we "translate" GCC options to MSVC options, and part of those options refer to the libraries to link into the final executable. Currently, this part looks somewhat like this on Windows:

	-lcurl -lnghttp2 -lidn2 -lssl -lcrypto -lssl -lcrypto -lgdi32
	-lcrypt32 -lwldap32 -lz -lws2_32 -lexpat

Some of those are direct dependencies (such as curl and ssl) and others are indirect (nghttp2 and idn2, for example, are dependencies of curl, but need to be linked in for reasons).

We already handle the direct dependencies, e.g. -liconv is already handled as adding libiconv.lib to the list of libraries to link against.

Let's just ignore the remaining -l* options so that MSVC does not have to warn us that it ignored e.g. the /lnghttp2 option. We do that by extending the clause that already "eats" the -R* options to also eat the -l* options.

---
 compat/vcbuild/scripts/clink.pl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index c7b021bfac..00fc339cba 100755
@@ -68,7 +68,7 @@
 	} elsif ("$arg" =~ /^-L/ && "$arg" ne "-LTCG") {
 		$arg =~ s/^-L/-LIBPATH:/;
 		push(@lflags, $arg);
-	} elsif ("$arg" =~ /^-R/) {
+	} elsif ("$arg" =~ /^-[Rl]/) {
 		# eat
 	} else {
 		push(@args, $arg);

gitgitgadget

From: Johannes Schindelin <[email protected]>

We frequently build Git using the DEVELOPER=1 make setting as a shortcut to enable all kinds of more stringent compiler warnings.

Those compiler warnings are relatively specific to GCC, though, so let's try our best to translate them to the equivalent options to pass to MS Visual C++'s cl.exe.

---
 compat/vcbuild/scripts/clink.pl | 46 +++++++++++++++++++++++++++++++++
 1 file changed, 46 insertions(+)

index 00fc339cba..ec95a3b2d0 100755
@@ -70,6 +70,52 @@
 		push(@lflags, $arg);
 	} elsif ("$arg" =~ /^-[Rl]/) {
 		# eat
+	} elsif ("$arg" eq "-Werror") {
+		push(@cflags, "-WX");
+	} elsif ("$arg" eq "-Wall") {
+		# cl.exe understands -Wall, but it is really overzealous
+		push(@cflags, "-W4");
+		# disable the "signed/unsigned mismatch" warnings; our source code violates that
+		push(@cflags, "-wd4018");
+		push(@cflags, "-wd4245");
+		push(@cflags, "-wd4389");
+		# disable the "unreferenced formal parameter" warning; our source code violates that
+		push(@cflags, "-wd4100");
+		# disable the "conditional expression is constant" warning; our source code violates that
+		push(@cflags, "-wd4127");
+		# disable the "const object should be initialized" warning; these warnings affect only objects that are `static`
+		push(@cflags, "-wd4132");
+		# disable the "function/data pointer conversion in expression" warning; our source code violates that
+		push(@cflags, "-wd4152");
+		# disable the "non-constant aggregate initializer" warning; our source code violates that
+		push(@cflags, "-wd4204");
+		# disable the "cannot be initialized using address of automatic variable" warning; our source code violates that
+		push(@cflags, "-wd4221");
+		# disable the "possible loss of data" warnings; our source code violates that
+		push(@cflags, "-wd4244");
+		push(@cflags, "-wd4267");
+		# disable the "array is too small to include a terminating null character" warning; we ab-use strings to initialize OIDs
+		push(@cflags, "-wd4295");
+		# disable the "'<<': result of 32-bit shift implicitly converted to 64 bits" warning; our source code violates that
+		push(@cflags, "-wd4334");
+		# disable the "declaration hides previous local declaration" warning; our source code violates that
+		push(@cflags, "-wd4456");
+		# disable the "declaration hides function parameter" warning; our source code violates that
+		push(@cflags, "-wd4457");
+		# disable the "declaration hides global declaration" warning; our source code violates that
+		push(@cflags, "-wd4459");
+		# disable the "potentially uninitialized local variable '<name>' used" warning; our source code violates that
+		push(@cflags, "-wd4701");
+		# disable the "unreachable code" warning; our source code violates that
+		push(@cflags, "-wd4702");
+		# disable the "potentially uninitialized local pointer variable used" warning; our source code violates that
+		push(@cflags, "-wd4703");
+		# disable the "assignment within conditional expression" warning; our source code violates that
+		push(@cflags, "-wd4706");
+		# disable the "'inet_ntoa': Use inet_ntop() or InetNtop() instead" warning; our source code violates that
+		push(@cflags, "-wd4996");
+	} elsif ("$arg" =~ /^-W[a-z]/) {
+		# let's ignore those
 	} else {
 		push(@args, $arg);
 	}

gitgitgadget

From: Johannes Schindelin <[email protected]>

The return value of that function is 0 both for variables that are unset, as well as for variables whose values are empty. To discern those two cases, one has to call GetLastError(), whose return value is ERROR_ENVVAR_NOT_FOUND and ERROR_SUCCESS, respectively.

Except that it is not actually set to ERROR_SUCCESS in the latter case, apparently, but the last error value seems to be simply unchanged.

To work around this, let's just re-set the last error value just before inspecting the environment variable.

This fixes a problem that triggers failures in t3301-notes.sh (where we try to override config settings by passing empty values for certain environment variables).

This problem is hidden in the MINGW build by the fact that older MSVC runtimes (such as the one used by MINGW builds) have a calloc() that re-sets the last error value in case of success, while newer runtimes set the error value only if NULL is returned by that function.

---
 compat/mingw.c | 2 ++
 1 file changed, 2 insertions(+)

index 4891789771..7d8cb814ba 100644
@@ -1661,6 +1661,8 @@ char *mingw_getenv(const char *name)
 	if (!w_key)
 		die("Out of memory, (tried to allocate %u wchar_t's)", len_key);
 	xutftowcs(w_key, name, len_key);
+	/* GetEnvironmentVariableW() only sets the last error upon failure */
+	SetLastError(ERROR_SUCCESS);
 	len_value = GetEnvironmentVariableW(w_key, w_value, ARRAY_SIZE(w_value));
 	if (!len_value && GetLastError() == ERROR_ENVVAR_NOT_FOUND) {
 		free(w_key);

gitgitgadget

From: Johannes Schindelin <[email protected]>

In b18ae14a8f6 (vcxproj: also link-or-copy builtins, 2019-07-29), we started to copy or hard-link the built-ins as a post-build step of the git project.

At the same time, we tried to copy or hard-link git-remote-http.exe, but it is quite possible that it was not built at that time.

Let's move that latter task into a post-install step of the git-remote-http project instead.

---
 config.mak.uname                           | 10 +++++++---
 contrib/buildsystems/Generators/Vcxproj.pm |  3 +++
 2 files changed, 10 insertions(+), 3 deletions(-)

index db7f06b95f..701aad62b1 100644
@@ -703,20 +703,24 @@ vcxproj:
 	perl contrib/buildsystems/generate -g Vcxproj
 	git add -f git.sln {*,*/lib,t/helper/*}/*.vcxproj
 
-	# Generate the LinkOrCopyBuiltins.targets file
+	# Generate the LinkOrCopyBuiltins.targets and LinkOrCopyRemoteHttp.targets file
 	(echo '<Project xmlns="http://schemas.microsoft.com/developer/msbuild/2003">' && \
 	 echo '  <Target Name="CopyBuiltins_AfterBuild" AfterTargets="AfterBuild">' && \
 	 for name in $(BUILT_INS);\
 	 do \
 	   echo '    <Copy SourceFiles="$$(OutDir)\git.exe" DestinationFiles="$$(OutDir)\'"$$name"'" SkipUnchangedFiles="true" UseHardlinksIfPossible="true" />'; \
 	 done && \
+	 echo '  </Target>' && \
+	 echo '</Project>') >git/LinkOrCopyBuiltins.targets
+	(echo '<Project xmlns="http://schemas.microsoft.com/developer/msbuild/2003">' && \
+	 echo '  <Target Name="CopyBuiltins_AfterBuild" AfterTargets="AfterBuild">' && \
 	 for name in $(REMOTE_CURL_ALIASES); \
 	 do \
 	   echo '    <Copy SourceFiles="$$(OutDir)\'"$(REMOTE_CURL_PRIMARY)"'" DestinationFiles="$$(OutDir)\'"$$name"'" SkipUnchangedFiles="true" UseHardlinksIfPossible="true" />'; \
 	 done && \
 	 echo '  </Target>' && \
-	 echo '</Project>') >git/LinkOrCopyBuiltins.targets
-	git add -f git/LinkOrCopyBuiltins.targets
+	 echo '</Project>') >git-remote-http/LinkOrCopyRemoteHttp.targets
+	git add -f git/LinkOrCopyBuiltins.targets git-remote-http/LinkOrCopyRemoteHttp.targets
 
 	# Add command-list.h
 	$(MAKE) MSVC=1 SKIP_VCPKG=1 prefix=/mingw64 command-list.h

index 576ccabe1d..868b787855 100644
@@ -277,6 +277,9 @@ sub createProject {
     if ($target eq 'git') {
       print F "  <Import Project=\"LinkOrCopyBuiltins.targets\" />\n";
     }
+    if ($target eq 'git-remote-http') {
+      print F "  <Import Project=\"LinkOrCopyRemoteHttp.targets\" />\n";
+    }
     print F << "EOM";
 </Project>
 EOM

gitgitgadget

From: Johannes Schindelin <[email protected]>

Git for Windows jumps through hoops to provide a development environment that allows to build Git and to run its test suite. To that end, an entire MSYS2 system, including GNU make and GCC is offered as "the Git for Windows SDK". It does come at a price: an initial download of said SDK weighs in with several hundreds of megabytes, and the unpacked SDK occupies ~2GB of disk space.

A much more native development environment on Windows is Visual Studio. To help contributors use that environment, we already have a Makefile target vcxproj that generates a commit with project files (and other generated files), and Git for Windows' vs/master branch is continuously re-generated using that target.

The idea is to allow building Git in Visual Studio, and to run individual tests using a Portable Git.

The one missing thing is a way to run the entire test suite: neither make nor prove are required to run Git, therefore Git for Windows does not support those commands in the Portable Git.

To help with that, add a simple test helper that exercises the run_processes_parallel() function to allow for running test scripts in parallel (which is really necessary, especially on Windows, as Git's test suite takes such a long time to run).

This will also come in handy for the upcoming change to our Azure
Pipeline: we will use this helper in a Portable Git to test the Visual Studio build of Git.

---
 t/helper/test-run-command.c | 153 ++++++++++++++++++++++++++++++++++++
 1 file changed, 153 insertions(+)

index 2cc93bb69c..ead6dc611a 100644
@@ -10,9 +10,14 @@
 
 #include "test-tool.h"
 #include "git-compat-util.h"
+#include "cache.h"
 #include "run-command.h"
 #include "argv-array.h"
 #include "strbuf.h"
+#include "parse-options.h"
+#include "string-list.h"
+#include "thread-utils.h"
+#include "wildmatch.h"
 #include <string.h>
 #include <errno.h>
 

@@ -50,11 +55,159 @@ static int task_finished(int result,
 	return 1;
 }
 
+struct testsuite {
+	struct string_list tests, failed;
+	int next;
+	int quiet, immediate, verbose, verbose_log, trace, write_junit_xml;
+};
+#define TESTSUITE_INIT \
+	{ STRING_LIST_INIT_DUP, STRING_LIST_INIT_DUP, -1, 0, 0, 0, 0, 0, 0 }
+
+static int next_test(struct child_process *cp, struct strbuf *err, void *cb,
+		     void **task_cb)
+{
+	struct testsuite *suite = cb;
+	const char *test;
+	if (suite->next >= suite->tests.nr)
+		return 0;
+
+	test = suite->tests.items[suite->next++].string;
+	argv_array_pushl(&cp->args, "sh", test, NULL);
+	if (suite->quiet)
+		argv_array_push(&cp->args, "--quiet");
+	if (suite->immediate)
+		argv_array_push(&cp->args, "-i");
+	if (suite->verbose)
+		argv_array_push(&cp->args, "-v");
+	if (suite->verbose_log)
+		argv_array_push(&cp->args, "-V");
+	if (suite->trace)
+		argv_array_push(&cp->args, "-x");
+	if (suite->write_junit_xml)
+		argv_array_push(&cp->args, "--write-junit-xml");
+
+	strbuf_addf(err, "Output of '%s':\n", test);
+	*task_cb = (void *)test;
+
+	return 1;
+}
+
+static int test_finished(int result, struct strbuf *err, void *cb,
+			 void *task_cb)
+{
+	struct testsuite *suite = cb;
+	const char *name = (const char *)task_cb;
+
+	if (result)
+		string_list_append(&suite->failed, name);
+
+	strbuf_addf(err, "%s: '%s'\n", result ? "FAIL" : "SUCCESS", name);
+
+	return 0;
+}
+
+static int test_failed(struct strbuf *out, void *cb, void *task_cb)
+{
+	struct testsuite *suite = cb;
+	const char *name = (const char *)task_cb;
+
+	string_list_append(&suite->failed, name);
+	strbuf_addf(out, "FAILED TO START: '%s'\n", name);
+
+	return 0;
+}
+
+static const char * const testsuite_usage[] = {
+	"test-run-command testsuite [<options>] [<pattern>...]",
+	NULL
+};
+
+static int testsuite(int argc, const char **argv)
+{
+	struct testsuite suite = TESTSUITE_INIT;
+	int max_jobs = 1, i, ret;
+	DIR *dir;
+	struct dirent *d;
+	struct option options[] = {
+		OPT_BOOL('i', "immediate", &suite.immediate,
+			 "stop at first failed test case(s)"),
+		OPT_INTEGER('j', "jobs", &max_jobs, "run <N> jobs in parallel"),
+		OPT_BOOL('q', "quiet", &suite.quiet, "be terse"),
+		OPT_BOOL('v', "verbose", &suite.verbose, "be verbose"),
+		OPT_BOOL('V', "verbose-log", &suite.verbose_log,
+			 "be verbose, redirected to a file"),
+		OPT_BOOL('x', "trace", &suite.trace, "trace shell commands"),
+		OPT_BOOL(0, "write-junit-xml", &suite.write_junit_xml,
+			 "write JUnit-style XML files"),
+		OPT_END()
+	};
+
+	memset(&suite, 0, sizeof(suite));
+	suite.tests.strdup_strings = suite.failed.strdup_strings = 1;
+
+	argc = parse_options(argc, argv, NULL, options,
+			testsuite_usage, PARSE_OPT_STOP_AT_NON_OPTION);
+
+	if (max_jobs <= 0)
+		max_jobs = online_cpus();
+
+	dir = opendir(".");
+	if (!dir)
+		die("Could not open the current directory");
+	while ((d = readdir(dir))) {
+		const char *p = d->d_name;
+
+		if (*p != 't' || !isdigit(p[1]) || !isdigit(p[2]) ||
+		    !isdigit(p[3]) || !isdigit(p[4]) || p[5] != '-' ||
+		    !ends_with(p, ".sh"))
+			continue;
+
+		/* No pattern: match all */
+		if (!argc) {
+			string_list_append(&suite.tests, p);
+			continue;
+		}
+
+		for (i = 0; i < argc; i++)
+			if (!wildmatch(argv[i], p, 0)) {
+				string_list_append(&suite.tests, p);
+				break;
+			}
+	}
+	closedir(dir);
+
+	if (!suite.tests.nr)
+		die("No tests match!");
+	if (max_jobs > suite.tests.nr)
+		max_jobs = suite.tests.nr;
+
+	fprintf(stderr, "Running %d tests (%d at a time)\n",
+		suite.tests.nr, max_jobs);
+
+	ret = run_processes_parallel(max_jobs, next_test, test_failed,
+				     test_finished, &suite);
+
+	if (suite.failed.nr > 0) {
+		ret = 1;
+		fprintf(stderr, "%d tests failed:\n\n", suite.failed.nr);
+		for (i = 0; i < suite.failed.nr; i++)
+			fprintf(stderr, "\t%s\n", suite.failed.items[i].string);
+	}
+
+	string_list_clear(&suite.tests, 0);
+	string_list_clear(&suite.failed, 0);
+
+	return !!ret;
+}
+
 int cmd__run_command(int argc, const char **argv)
 {
 	struct child_process proc = CHILD_PROCESS_INIT;
 	int jobs;
 
+	if (argc > 1 && !strcmp(argv[1], "testsuite"))
+		exit(testsuite(argc - 1, argv + 1));
+
 	if (argc < 3)
 		return 1;
 	while (!strcmp(argv[1], "env")) {

gitgitgadget

From: Johannes Schindelin <[email protected]>

In the CI builds, we bundle all generated files into a so-called artifacts .tar file, so that the test phase can fan out into multiple parallel builds.

This patch makes sure that all files are included in the vcxproj target which are needed for that artifacts .tar file.

---
 config.mak.uname | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

index 701aad62b1..cc8efd95b1 100644
@@ -728,11 +728,10 @@ vcxproj:
 
 	# Add scripts
 	rm -f perl/perl.mak
-	$(MAKE) MSVC=1 SKIP_VCPKG=1 prefix=/mingw64 \
-		$(SCRIPT_LIB) $(SCRIPT_SH_GEN) $(SCRIPT_PERL_GEN)
+	$(MAKE) MSVC=1 SKIP_VCPKG=1 prefix=/mingw64 $(SCRIPT_LIB) $(SCRIPTS)
 	# Strip out the sane tool path, needed only for building
 	sed -i '/^git_broken_path_fix ".*/d' git-sh-setup
-	git add -f $(SCRIPT_LIB) $(SCRIPT_SH_GEN) $(SCRIPT_PERL_GEN)
+	git add -f $(SCRIPT_LIB) $(SCRIPTS)
 
 	# Add Perl module
 	$(MAKE) $(LIB_PERL_GEN)

@@ -762,6 +761,10 @@ vcxproj:
 	$(MAKE) -C templates
 	git add -f templates/boilerplates.made templates/blt/
 
+	# Add the translated messages
+	make MSVC=1 SKIP_VCPKG=1 prefix=/mingw64 $(MOFILES)
+	git add -f $(MOFILES)
+
 	# Add build options
 	$(MAKE) MSVC=1 SKIP_VCPKG=1 prefix=/mingw64 GIT-BUILD-OPTIONS
 	git add -f GIT-BUILD-OPTIONS

gitgitgadget

From: Johannes Schindelin <[email protected]>

When the --immediate option is in effect, any test failure will immediately exit the test script. Together with --write-junit-xml, we will want the JUnit-style .xml file to be finalized (and not leave the XML incomplete). Let's make it so.

This comes in particularly handy when trying to debug via Azure Pipelines, where the JUnit-style XML is consumed to present the test results in an informative and helpful way.

---
 t/test-lib.sh | 37 +++++++++++++++++++++----------------
 1 file changed, 21 insertions(+), 16 deletions(-)

index d1ba33745a..f21c781e68 100644
@@ -695,7 +695,7 @@ test_failure_ () {
 	say_color error "not ok $test_count - $1"
 	shift
 	printf '%s\n' "$*" | sed -e 's/^/#	/'
-	test "$immediate" = "" || { GIT_EXIT_OK=t; exit 1; }
+	test "$immediate" = "" || { finalize_junit_xml; GIT_EXIT_OK=t; exit 1; }
 }
 
 test_known_broken_ok_ () {

@@ -1063,6 +1063,25 @@ write_junit_xml_testcase () {
 	junit_have_testcase=t
 }
 
+finalize_junit_xml () {
+	if test -n "$write_junit_xml" && test -n "$junit_xml_path"
+	then
+		test -n "$junit_have_testcase" || {
+			junit_start=$(test-tool date getnanos)
+			write_junit_xml_testcase "all tests skipped"
+		}
+
+		# adjust the overall time
+		junit_time=$(test-tool date getnanos $junit_suite_start)
+		sed "s/<testsuite [^>]*/& time=\"$junit_time\"/" \
+			<"$junit_xml_path" >"$junit_xml_path.new"
+		mv "$junit_xml_path.new" "$junit_xml_path"
+
+		write_junit_xml "  </testsuite>" "</testsuites>"
+		write_junit_xml=
+	fi
+}
+
 test_atexit_cleanup=:
 test_atexit_handler () {
 	# In a succeeding test script 'test_atexit_handler' is invoked

@@ -1085,21 +1104,7 @@ test_done () {
 	# removed, so the commands can access pidfiles and socket files.
 	test_atexit_handler
 
-	if test -n "$write_junit_xml" && test -n "$junit_xml_path"
-	then
-		test -n "$junit_have_testcase" || {
-			junit_start=$(test-tool date getnanos)
-			write_junit_xml_testcase "all tests skipped"
-		}
-
-		# adjust the overall time
-		junit_time=$(test-tool date getnanos $junit_suite_start)
-		sed "s/<testsuite [^>]*/& time=\"$junit_time\"/" \
-			<"$junit_xml_path" >"$junit_xml_path.new"
-		mv "$junit_xml_path.new" "$junit_xml_path"
-
-		write_junit_xml "  </testsuite>" "</testsuites>"
-	fi
+	finalize_junit_xml
 
 	if test -z "$HARNESS_ACTIVE"
 	then

gitgitgadget

From: Johannes Schindelin <[email protected]>

This was a left-over from the previous YAML schema, and it no longer works. The problem was noticed while editing azure-pipelines.yml in VS Code with the very helpful "Azure Pipelines" extension (syntax highlighting and intellisense for azure-pipelines.yml...).

---
 azure-pipelines.yml | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

index c329b7218b..55ee23ad0f 100644
@@ -1,6 +1,5 @@
-resources:
-- repo: self
-  fetchDepth: 1
+variables:
+  Agent.Source.Git.ShallowFetchDepth: 1
 
 jobs:
 - job: windows_build

gitgitgadget

From: Johannes Schindelin <[email protected]>

... because we can, now. Technically, we actually build using MSBuild, which is however pretty close to building interactively in Visual Studio.

As there is no convenient way to run Git's test suite in Visual Studio, we unpack a Portable Git to run it, using the just-added test helper to allow running test scripts in parallel.

---
 Makefile            |   4 ++
 azure-pipelines.yml | 159 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 163 insertions(+)

index 3716dadc08..3f6dcec54b 100644
@@ -3025,6 +3025,10 @@ rpm::
 	@false
 .PHONY: rpm
 
+ifneq ($(INCLUDE_DLLS_IN_ARTIFACTS),)
+OTHER_PROGRAMS += $(shell echo *.dll t/helper/*.dll)
+endif
+
 artifacts-tar:: $(ALL_PROGRAMS) $(SCRIPT_LIB) $(BUILT_INS) $(OTHER_PROGRAMS) \
 		GIT-BUILD-OPTIONS $(TEST_PROGRAMS) $(test_bindir_programs) \
 		$(MOFILES)

gitgitgadget

Hi Dscho,

[...]

	pos = -1 - (int) istate->cache_nr;

Hi Denton,

I guess we should use signed_add_overflows() to make extra certain that it does what we want it to do, kind of like st_add(). Or just do the check explicitly, like so:

	if (istate->cache_nr > INT_MAX)
		die("overflow: -1 - %u", istate->cache_nr);
	pos = -1 - istate->cache_nr;
}

This is even more critical, as lo has the type size_t:

	if (lo > INT_MAX)
		die("overflow: -1 - %"PRIuMAX, (uintmax_t)lo);
	return -1 - lo;

What do you think?
Dscho

Hi Dscho,

Could we change this to

pos = -1 - (int) istate->cache_nr;

so that we alleviate the problem I was talking about above?

Other than that, it looks good. Well, it might break on one's complement systems but I don't think we support UNIVACs anyway. ;)

Also, could we change this to

return -1 - (int) lo;

Thanks,

Denton

"Johannes Schindelin via GitGitGadget" <[email protected]>

        error () {
                say_color error "error: $*"
                GIT_EXIT_OK=t
                exit 1
        }

Hi Denton,

Thank you, I had missed that.

Let's hope that nobody tries to run Git on such a system, indeed.

Thanks,
Dscho

Hi Junio,

Good point.

This adds an additional message. I am not sure how many scripts/CI integrations are there that rely on the current behavior, so I would like to exclude this change from this here patch series: it is about including a Visual Studio build in our Azure Pipeline, nothing more, nothing less...

Ciao,
Dscho

From: Johannes Schindelin <[email protected]>

This function is marked as NORETURN, and it indeed does not want to return anything. So let's not declare it with the return type int. This fixes the following warning when building with MSVC:

C4646: function declared with 'noreturn' has non-void return type

---
 builtin/push.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index 021dd3b1e4..d216270d5f 100644
@@ -143,8 +143,8 @@ static int push_url_of_remote(struct remote *remote, const char ***url_p)
 	return remote->url_nr;
 }
 
-static NORETURN int die_push_simple(struct branch *branch,
-				    struct remote *remote)
+static NORETURN void die_push_simple(struct branch *branch,
+				     struct remote *remote)
 {
 	/*
 	 * There's no point in using shorten_unambiguous_ref here,

gitgitgadget

From: Johannes Schindelin <[email protected]>

MSVC would complain thusly:

C4200: nonstandard extension used: zero-sized array in struct/union

Let's just use the FLEX_ARRAY constant that we introduced for exactly this type of scenario.

---
 compat/winansi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index cacd82c833..54fd701cbf 100644
@@ -546,7 +546,7 @@ static HANDLE swap_osfhnd(int fd, HANDLE new_handle)
 typedef struct _OBJECT_NAME_INFORMATION
 {
 	UNICODE_STRING Name;
-	WCHAR NameBuffer[0];
+	WCHAR NameBuffer[FLEX_ARRAY];
 } OBJECT_NAME_INFORMATION, *POBJECT_NAME_INFORMATION;
 
 #define ObjectNameInformation 1

gitgitgadget

From: Johannes Schindelin <[email protected]>

MSVC complains about this with -Wall, which can be taken as a sign that this is indeed a real bug. The symptom is:

C4146: unary minus operator applied to unsigned type, result still unsigned

Let's avoid this warning in the minimal way, e.g. writing -1 - <unsigned value> instead of -<unsigned value> - 1.

Note that the change in the estimate_cache_size() function is needed because MSVC considers the "return type" of the sizeof() operator to be size_t, i.e. unsigned, and therefore it cannot be negated using the unary minus operator.

Even worse, that arithmetic is doing extra work, in vain. We want to calculate the entry extra cache size as the difference between the size of the cache_entry structure minus the size of the ondisk_cache_entry structure, padded to the appropriate alignment boundary.

To that end, we start by assigning that difference to the per_entry variable, and then abuse the len parameter of the align_padding_size() macro to take the negative size of the ondisk entry size. Essentially, we try to avoid passing the already calculated difference to that macro by passing the operands of that difference instead, when the macro expects operands of an addition:

	#define align_padding_size(size, len) \
		((size + (len) + 8) & ~7) - (size + len)

Currently, we pass A and -B to that macro instead of passing A - B and 0, where A - B is already stored in the per_entry variable, ready to be used.

This is neither necessary, nor intuitive. Let's fix this, and have code that is both easier to read and that also does not trigger MSVC's warning.

While at it, we take care of reporting overflows (which are unlikely, but hey, defensive programming is good!).

We _also_ take pains of casting the unsigned value to signed: otherwise, the signed operand (i.e. the -1) would be cast to unsigned before doing the arithmetic.

---
 read-cache.c  |  9 ++++++---
 sha1-lookup.c | 12 +++++++++---
 2 files changed, 15 insertions(+), 6 deletions(-)

index c701f7f8b8..97745c2a31 100644
@@ -1275,8 +1275,11 @@ static int add_index_entry_with_check(struct index_state *istate, struct cache_e
 	 * we can avoid searching for it.
 	 */
 	if (istate->cache_nr > 0 &&
-		strcmp(ce->name, istate->cache[istate->cache_nr - 1]->name) > 0)
-		pos = -istate->cache_nr - 1;
+		strcmp(ce->name, istate->cache[istate->cache_nr - 1]->name) > 0) {
+		if (istate->cache_nr > INT_MAX)
+			die("overflow: -1 - %u", istate->cache_nr);
+		pos = -1 - (int)istate->cache_nr;
+	}
 	else
 		pos = index_name_stage_pos(istate, ce->name, ce_namelen(ce), ce_stage(ce));
 

@@ -1894,7 +1897,7 @@ static size_t estimate_cache_size(size_t ondisk_size, unsigned int entries)
 	/*
 	 * Account for potential alignment differences.
 	 */
-	per_entry += align_padding_size(sizeof(struct cache_entry), -sizeof(struct ondisk_cache_entry));
+	per_entry += align_padding_size(per_entry, 0);
 	return ondisk_size + entries * per_entry;
 }
 

index 796ab68da8..bb786b5420 100644
@@ -69,8 +69,12 @@ int sha1_pos(const unsigned char *sha1, void *table, size_t nr,
 			miv = take2(sha1 + ofs);
 			if (miv < lov)
 				return -1;
-			if (hiv < miv)
-				return -1 - nr;
+			if (hiv < miv) {
+				if (nr > INT_MAX)
+					die("overflow: -1 - %"PRIuMAX,
+					    (uintmax_t)nr);
+				return -1 - (int)nr;
+			}
 			if (lov != hiv) {
 				/*
 				 * At this point miv could be equal

@@ -97,7 +101,9 @@ int sha1_pos(const unsigned char *sha1, void *table, size_t nr,
 			lo = mi + 1;
 		mi = lo + (hi - lo) / 2;
 	} while (lo < hi);
-	return -lo-1;
+	if (nr > INT_MAX)
+		die("overflow: -1 - %"PRIuMAX, (uintmax_t)lo);
+	return -1 - (int)lo;
 }
 
 int bsearch_hash(const unsigned char *sha1, const uint32_t *fanout_nbo,

gitgitgadget

From: Johannes Schindelin <[email protected]>

To build with MSVC, we "translate" GCC options to MSVC options, and part of those options refer to the libraries to link into the final executable. Currently, this part looks somewhat like this on Windows:

	-lcurl -lnghttp2 -lidn2 -lssl -lcrypto -lssl -lcrypto -lgdi32
	-lcrypt32 -lwldap32 -lz -lws2_32 -lexpat

Some of those are direct dependencies (such as curl and ssl) and others are indirect (nghttp2 and idn2, for example, are dependencies of curl, but need to be linked in for reasons).

We already handle the direct dependencies, e.g. -liconv is already handled as adding libiconv.lib to the list of libraries to link against.

Let's just ignore the remaining -l* options so that MSVC does not have to warn us that it ignored e.g. the /lnghttp2 option. We do that by extending the clause that already "eats" the -R* options to also eat the -l* options.

---
 compat/vcbuild/scripts/clink.pl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index c7b021bfac..00fc339cba 100755
@@ -68,7 +68,7 @@
 	} elsif ("$arg" =~ /^-L/ && "$arg" ne "-LTCG") {
 		$arg =~ s/^-L/-LIBPATH:/;
 		push(@lflags, $arg);
-	} elsif ("$arg" =~ /^-R/) {
+	} elsif ("$arg" =~ /^-[Rl]/) {
 		# eat
 	} else {
 		push(@args, $arg);

gitgitgadget

From: Johannes Schindelin <[email protected]>

We frequently build Git using the DEVELOPER=1 make setting as a shortcut to enable all kinds of more stringent compiler warnings.

Those compiler warnings are relatively specific to GCC, though, so let's try our best to translate them to the equivalent options to pass to MS Visual C++'s cl.exe.

---
 compat/vcbuild/scripts/clink.pl | 46 +++++++++++++++++++++++++++++++++
 1 file changed, 46 insertions(+)

index 00fc339cba..ec95a3b2d0 100755
@@ -70,6 +70,52 @@
 		push(@lflags, $arg);
 	} elsif ("$arg" =~ /^-[Rl]/) {
 		# eat
+	} elsif ("$arg" eq "-Werror") {
+		push(@cflags, "-WX");
+	} elsif ("$arg" eq "-Wall") {
+		# cl.exe understands -Wall, but it is really overzealous
+		push(@cflags, "-W4");
+		# disable the "signed/unsigned mismatch" warnings; our source code violates that
+		push(@cflags, "-wd4018");
+		push(@cflags, "-wd4245");
+		push(@cflags, "-wd4389");
+		# disable the "unreferenced formal parameter" warning; our source code violates that
+		push(@cflags, "-wd4100");
+		# disable the "conditional expression is constant" warning; our source code violates that
+		push(@cflags, "-wd4127");
+		# disable the "const object should be initialized" warning; these warnings affect only objects that are `static`
+		push(@cflags, "-wd4132");
+		# disable the "function/data pointer conversion in expression" warning; our source code violates that
+		push(@cflags, "-wd4152");
+		# disable the "non-constant aggregate initializer" warning; our source code violates that
+		push(@cflags, "-wd4204");
+		# disable the "cannot be initialized using address of automatic variable" warning; our source code violates that
+		push(@cflags, "-wd4221");
+		# disable the "possible loss of data" warnings; our source code violates that
+		push(@cflags, "-wd4244");
+		push(@cflags, "-wd4267");
+		# disable the "array is too small to include a terminating null character" warning; we ab-use strings to initialize OIDs
+		push(@cflags, "-wd4295");
+		# disable the "'<<': result of 32-bit shift implicitly converted to 64 bits" warning; our source code violates that
+		push(@cflags, "-wd4334");
+		# disable the "declaration hides previous local declaration" warning; our source code violates that
+		push(@cflags, "-wd4456");
+		# disable the "declaration hides function parameter" warning; our source code violates that
+		push(@cflags, "-wd4457");
+		# disable the "declaration hides global declaration" warning; our source code violates that
+		push(@cflags, "-wd4459");
+		# disable the "potentially uninitialized local variable '<name>' used" warning; our source code violates that
+		push(@cflags, "-wd4701");
+		# disable the "unreachable code" warning; our source code violates that
+		push(@cflags, "-wd4702");
+		# disable the "potentially uninitialized local pointer variable used" warning; our source code violates that
+		push(@cflags, "-wd4703");
+		# disable the "assignment within conditional expression" warning; our source code violates that
+		push(@cflags, "-wd4706");
+		# disable the "'inet_ntoa': Use inet_ntop() or InetNtop() instead" warning; our source code violates that
+		push(@cflags, "-wd4996");
+	} elsif ("$arg" =~ /^-W[a-z]/) {
+		# let's ignore those
 	} else {
 		push(@args, $arg);
 	}

gitgitgadget

From: Johannes Schindelin <[email protected]>

This adds the common guards that allow headers to be #include'd multiple times.

---
 compat/win32/path-utils.h | 5 +++++
 1 file changed, 5 insertions(+)

index 0f70d43920..8ed062a6b7 100644
@@ -1,3 +1,6 @@
+#ifndef WIN32_PATH_UTILS_H
+#define WIN32_PATH_UTILS_H
+
 #define has_dos_drive_prefix(path) \
 	(isalpha(*(path)) && (path)[1] == ':' ? 2 : 0)
 int win32_skip_dos_drive_prefix(char **path);

@@ -18,3 +21,5 @@ static inline char *win32_find_last_dir_sep(const char *path)
 #define find_last_dir_sep win32_find_last_dir_sep
 int win32_offset_1st_component(const char *path);
 #define offset_1st_component win32_offset_1st_component
+
+#endif

gitgitgadget

Git's Continuous Integration (CI) includes an Azure Pipeline that builds Git on Linux, macOS and Windows, in the former two cases even in multiple configurations (using GCC vs clang, 32-bit vs 64-bit, etc). On Windows, we only build using GCC, using (a subset of) Git for Windows' SDK.

Recently, a patch series made it into Git that re-instates the ability to generate project files for use with Visual Studio. The idea there being:
contributors can check out a special branch that has those generated files in one generated commit on top of e.g. Git for Windows' master, allowing the contributors to build Git in Visual Studio, without the need for downloading Git for Windows' SDK (which weighs quite a bit: ~600MB download, ~2GB disk footprint). The tests can then be run from a regular Git for Windows Bash.

This patch series adds that axis to Git's Azure Pipeline: the project files are generated, MSBuild (which is kind of the command-line equivalent of Visual Studio's "Build" operation) is used to build Git, and then a parallelized test job runs the test suite in a Portable Git.

These patches are based on js/visual-studio.

Changes since v1:

* "While at it", we now also check for overflows when doing that -1 - <unsigned> arithmetic.
* The JUnit-style XML is finalized also in case that the script aborts e.g. due to an incorrect number of arguments in a test_expect_success call.

Johannes Schindelin (13):
push: do not pretend to return int from die_push_simple()
msvc: avoid using minus operator on unsigned types
winansi: use FLEX_ARRAY to avoid compiler warning compat/win32/path-utils.h: add #include guards
msvc: ignore some libraries when linking
msvc: handle DEVELOPER=1
msvc: work around a bug in GetEnvironmentVariable()
vcxproj: only copy git-remote-http.exe once it was built
vcxproj: include more generated files
test-tool run-command: learn to run (parts of) the testsuite
tests: let --immediate and --write-junit-xml play well together
ci: really use shallow clones on Azure Pipelines
ci: also build and test with MS Visual Studio on Azure Pipelines

 Makefile                                   |   4 +
 azure-pipelines.yml                        | 164 ++++++++++++++++++++-
 builtin/push.c                             |   4 +-
 compat/mingw.c                             |   2 +
 compat/vcbuild/scripts/clink.pl            |  48 +++++-
 compat/win32/path-utils.h                  |   5 +
 compat/winansi.c                           |   2 +-
 config.mak.uname                           |  19 ++-
 contrib/buildsystems/Generators/Vcxproj.pm |   3 +
 read-cache.c                               |   9 +-
 sha1-lookup.c                              |  12 +-
 t/helper/test-run-command.c                | 153 +++++++++++++++++++
 t/test-lib.sh                              |  38 +++--
 13 files changed, 428 insertions(+), 35 deletions(-)

base-commit: aac6ff7b5beeea9bca66ecda6eec60fc1dd447ec
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-288%2Fdscho%2Fazure-pipelines-msvc-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-288/dscho/azure-pipelines-msvc-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/288

Range-diff vs v1:

  1:  4d0b38125a =  1:  4d0b38125a push: do not pretend to return `int` from `die_push_simple()`
  2:  2abe1e1fb0 !  2:  8800320590 msvc: avoid using minus operator on unsigned types
     @@ -40,17 +40,30 @@
          that is both easier to read and that also does not trigger MSVC's
          warning.
      
     +    While at it, we take care of reporting overflows (which are unlikely,
     +    but hey, defensive programming is good!).
     +
     +    We _also_ take pains of casting the unsigned value to signed: otherwise,
     +    the signed operand (i.e. the `-1`) would be cast to unsigned before
     +    doing the arithmetic.
     +
     +    Helped-by: Denton Liu <[email protected]>
          Signed-off-by: Johannes Schindelin <[email protected]>
      
       diff --git a/read-cache.c b/read-cache.c
       --- a/read-cache.c
       +++ b/read-cache.c
      @@
     + 	 * we can avoid searching for it.
       	 */
       	if (istate->cache_nr > 0 &&
     - 		strcmp(ce->name, istate->cache[istate->cache_nr - 1]->name) > 0)
     +-		strcmp(ce->name, istate->cache[istate->cache_nr - 1]->name) > 0)
      -		pos = -istate->cache_nr - 1;
     -+		pos = -1 - istate->cache_nr;
     ++		strcmp(ce->name, istate->cache[istate->cache_nr - 1]->name) > 0) {
     ++		if (istate->cache_nr > INT_MAX)
     ++			die("overflow: -1 - %u", istate->cache_nr);
     ++		pos = -1 - (int)istate->cache_nr;
     ++	}
       	else
       		pos = index_name_stage_pos(istate, ce->name, ce_namelen(ce), ce_stage(ce));
       
     @@ -67,12 +80,29 @@
       diff --git a/sha1-lookup.c b/sha1-lookup.c
       --- a/sha1-lookup.c
       +++ b/sha1-lookup.c
     +@@
     + 			miv = take2(sha1 + ofs);
     + 			if (miv < lov)
     + 				return -1;
     +-			if (hiv < miv)
     +-				return -1 - nr;
     ++			if (hiv < miv) {
     ++				if (nr > INT_MAX)
     ++					die("overflow: -1 - %"PRIuMAX,
     ++					    (uintmax_t)nr);
     ++				return -1 - (int)nr;
     ++			}
     + 			if (lov != hiv) {
     + 				/*
     + 				 * At this point miv could be equal
      @@
       			lo = mi + 1;
       		mi = lo + (hi - lo) / 2;
       	} while (lo < hi);
      -	return -lo-1;
     -+	return -1 - lo;
     ++	if (nr > INT_MAX)
     ++		die("overflow: -1 - %"PRIuMAX, (uintmax_t)lo);
     ++	return -1 - (int)lo;
       }
       
       int bsearch_hash(const unsigned char *sha1, const uint32_t *fanout_nbo,
  3:  dbd9022ad5 =  3:  8512a3e96d winansi: use FLEX_ARRAY to avoid compiler warning
  4:  6417d6f689 =  4:  0345b08f54 compat/win32/path-utils.h: add #include guards
  5:  d1dee56fcc =  5:  5add01f8ff msvc: ignore some libraries when linking
  6:  c978c76599 =  6:  5c880f923e msvc: handle DEVELOPER=1
  7:  0590514e43 =  7:  1f2245a228 msvc: work around a bug in GetEnvironmentVariable()
  8:  f7d5d1a1bc =  8:  582b299634 vcxproj: only copy `git-remote-http.exe` once it was built
  9:  6adfc63e98 =  9:  38ccf999e7 vcxproj: include more generated files
 10:  ad9ab10ce0 = 10:  24b1c7bff3 test-tool run-command: learn to run (parts of) the testsuite
 11:  99724f6a1e ! 11:  7be13d19e1 tests: let --immediate and --write-junit-xml play well together

@@ -11,12 +11,34 @@
Pipelines, where the JUnit-style XML is consumed to present the test results in an informative and helpful way.

+ While at it, also handle the error() code path.
+
+ The only remaining code path that sets GIT_EXIT_OK happens whenever
+ the trash directory could not be set up, i.e. long before the JUnit XML
+ was written, therefore we should _not_ try to finalize that XML in that
+ case.
+
+ It is tempting to change the immediate code path to just hand off to
+ error, simplifying the code in the process. That would, however,
+ result in a change of behavior (an additional error message) in the test
+ suite, which is outside of the purview of the current patch series: its
+ goal is to allow building Git with Visual Studio and testing it with a
+ portable version of Git for Windows.
+

diff --git a/t/test-lib.sh b/t/test-lib.sh
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@
+
+ error () {
+ say_color error "error: $*"
++ finalize_junit_xml
+ GIT_EXIT_OK=t
+ exit 1
+ }
+@@
say_color error "not ok $test_count - $1"
shift
printf '%s\n' "$*" | sed -e 's/^/# /'

 12:  29dceaae5e = 12:  bde1e8ef65 ci: really use shallow clones on Azure Pipelines
 13:  ece296922b = 13:  7af1c01a08 ci: also build and test with MS Visual Studio on Azure Pipelines

gitgitgadget

From: Johannes Schindelin <[email protected]>

The return value of that function is 0 both for variables that are unset, as well as for variables whose values are empty. To discern those two cases, one has to call GetLastError(), whose return value is ERROR_ENVVAR_NOT_FOUND and ERROR_SUCCESS, respectively.

Except that it is not actually set to ERROR_SUCCESS in the latter case, apparently, but the last error value seems to be simply unchanged.

To work around this, let's just re-set the last error value just before inspecting the environment variable.

This fixes a problem that triggers failures in t3301-notes.sh (where we try to override config settings by passing empty values for certain environment variables).

This problem is hidden in the MINGW build by the fact that older MSVC runtimes (such as the one used by MINGW builds) have a calloc() that re-sets the last error value in case of success, while newer runtimes set the error value only if NULL is returned by that function.

---
 compat/mingw.c | 2 ++
 1 file changed, 2 insertions(+)

index 4891789771..7d8cb814ba 100644
@@ -1661,6 +1661,8 @@ char *mingw_getenv(const char *name)
 	if (!w_key)
 		die("Out of memory, (tried to allocate %u wchar_t's)", len_key);
 	xutftowcs(w_key, name, len_key);
+	/* GetEnvironmentVariableW() only sets the last error upon failure */
+	SetLastError(ERROR_SUCCESS);
 	len_value = GetEnvironmentVariableW(w_key, w_value, ARRAY_SIZE(w_value));
 	if (!len_value && GetLastError() == ERROR_ENVVAR_NOT_FOUND) {
 		free(w_key);

gitgitgadget

From: Johannes Schindelin <[email protected]>

When the --immediate option is in effect, any test failure will immediately exit the test script. Together with --write-junit-xml, we will want the JUnit-style .xml file to be finalized (and not leave the XML incomplete). Let's make it so.

This comes in particularly handy when trying to debug via Azure Pipelines, where the JUnit-style XML is consumed to present the test results in an informative and helpful way.

While at it, also handle the error() code path.

The only remaining code path that sets GIT_EXIT_OK happens whenever the trash directory could not be set up, i.e. long before the JUnit XML was written, therefore we should _not_ try to finalize that XML in that case.

It is tempting to change the immediate code path to just hand off to error, simplifying the code in the process. That would, however, result in a change of behavior (an additional error message) in the test suite, which is outside of the purview of the current patch series: its goal is to allow building Git with Visual Studio and testing it with a portable version of Git for Windows.

---
 t/test-lib.sh | 38 ++++++++++++++++++++++----------------
 1 file changed, 22 insertions(+), 16 deletions(-)

index d1ba33745a..86b5e6133b 100644
@@ -567,6 +567,7 @@ export TERM
 
 error () {
 	say_color error "error: $*"
+	finalize_junit_xml
 	GIT_EXIT_OK=t
 	exit 1
 }

@@ -695,7 +696,7 @@ test_failure_ () {
 	say_color error "not ok $test_count - $1"
 	shift
 	printf '%s\n' "$*" | sed -e 's/^/#	/'
-	test "$immediate" = "" || { GIT_EXIT_OK=t; exit 1; }
+	test "$immediate" = "" || { finalize_junit_xml; GIT_EXIT_OK=t; exit 1; }
 }
 
 test_known_broken_ok_ () {

@@ -1063,6 +1064,25 @@ write_junit_xml_testcase () {
 	junit_have_testcase=t
 }
 
+finalize_junit_xml () {
+	if test -n "$write_junit_xml" && test -n "$junit_xml_path"
+	then
+		test -n "$junit_have_testcase" || {
+			junit_start=$(test-tool date getnanos)
+			write_junit_xml_testcase "all tests skipped"
+		}
+
+		# adjust the overall time
+		junit_time=$(test-tool date getnanos $junit_suite_start)
+		sed "s/<testsuite [^>]*/& time=\"$junit_time\"/" \
+			<"$junit_xml_path" >"$junit_xml_path.new"
+		mv "$junit_xml_path.new" "$junit_xml_path"
+
+		write_junit_xml "  </testsuite>" "</testsuites>"
+		write_junit_xml=
+	fi
+}
+
 test_atexit_cleanup=:
 test_atexit_handler () {
 	# In a succeeding test script 'test_atexit_handler' is invoked

@@ -1085,21 +1105,7 @@ test_done () {
 	# removed, so the commands can access pidfiles and socket files.
 	test_atexit_handler
 
-	if test -n "$write_junit_xml" && test -n "$junit_xml_path"
-	then
-		test -n "$junit_have_testcase" || {
-			junit_start=$(test-tool date getnanos)
-			write_junit_xml_testcase "all tests skipped"
-		}
-
-		# adjust the overall time
-		junit_time=$(test-tool date getnanos $junit_suite_start)
-		sed "s/<testsuite [^>]*/& time=\"$junit_time\"/" \
-			<"$junit_xml_path" >"$junit_xml_path.new"
-		mv "$junit_xml_path.new" "$junit_xml_path"
-
-		write_junit_xml "  </testsuite>" "</testsuites>"
-	fi
+	finalize_junit_xml
 
 	if test -z "$HARNESS_ACTIVE"
 	then

gitgitgadget

From: Johannes Schindelin <[email protected]>

... because we can, now. Technically, we actually build using MSBuild, which is however pretty close to building interactively in Visual Studio.

As there is no convenient way to run Git's test suite in Visual Studio, we unpack a Portable Git to run it, using the just-added test helper to allow running test scripts in parallel.

---
 Makefile            |   4 ++
 azure-pipelines.yml | 159 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 163 insertions(+)

index 3716dadc08..3f6dcec54b 100644
@@ -3025,6 +3025,10 @@ rpm::
 	@false
 .PHONY: rpm
 
+ifneq ($(INCLUDE_DLLS_IN_ARTIFACTS),)
+OTHER_PROGRAMS += $(shell echo *.dll t/helper/*.dll)
+endif
+
 artifacts-tar:: $(ALL_PROGRAMS) $(SCRIPT_LIB) $(BUILT_INS) $(OTHER_PROGRAMS) \
 		GIT-BUILD-OPTIONS $(TEST_PROGRAMS) $(test_bindir_programs) \
 		$(MOFILES)

gitgitgadget

From: Johannes Schindelin <[email protected]>

Git for Windows jumps through hoops to provide a development environment that allows to build Git and to run its test suite. To that end, an entire MSYS2 system, including GNU make and GCC is offered as "the Git for Windows SDK". It does come at a price: an initial download of said SDK weighs in with several hundreds of megabytes, and the unpacked SDK occupies ~2GB of disk space.

A much more native development environment on Windows is Visual Studio. To help contributors use that environment, we already have a Makefile target vcxproj that generates a commit with project files (and other generated files), and Git for Windows' vs/master branch is continuously re-generated using that target.

The idea is to allow building Git in Visual Studio, and to run individual tests using a Portable Git.

The one missing thing is a way to run the entire test suite: neither make nor prove are required to run Git, therefore Git for Windows does not support those commands in the Portable Git.

To help with that, add a simple test helper that exercises the run_processes_parallel() function to allow for running test scripts in parallel (which is really necessary, especially on Windows, as Git's test suite takes such a long time to run).

This will also come in handy for the upcoming change to our Azure
Pipeline: we will use this helper in a Portable Git to test the Visual Studio build of Git.

---
 t/helper/test-run-command.c | 153 ++++++++++++++++++++++++++++++++++++
 1 file changed, 153 insertions(+)

index 2cc93bb69c..ead6dc611a 100644
@@ -10,9 +10,14 @@
 
 #include "test-tool.h"
 #include "git-compat-util.h"
+#include "cache.h"
 #include "run-command.h"
 #include "argv-array.h"
 #include "strbuf.h"
+#include "parse-options.h"
+#include "string-list.h"
+#include "thread-utils.h"
+#include "wildmatch.h"
 #include <string.h>
 #include <errno.h>
 

@@ -50,11 +55,159 @@ static int task_finished(int result,
 	return 1;
 }
 
+struct testsuite {
+	struct string_list tests, failed;
+	int next;
+	int quiet, immediate, verbose, verbose_log, trace, write_junit_xml;
+};
+#define TESTSUITE_INIT \
+	{ STRING_LIST_INIT_DUP, STRING_LIST_INIT_DUP, -1, 0, 0, 0, 0, 0, 0 }
+
+static int next_test(struct child_process *cp, struct strbuf *err, void *cb,
+		     void **task_cb)
+{
+	struct testsuite *suite = cb;
+	const char *test;
+	if (suite->next >= suite->tests.nr)
+		return 0;
+
+	test = suite->tests.items[suite->next++].string;
+	argv_array_pushl(&cp->args, "sh", test, NULL);
+	if (suite->quiet)
+		argv_array_push(&cp->args, "--quiet");
+	if (suite->immediate)
+		argv_array_push(&cp->args, "-i");
+	if (suite->verbose)
+		argv_array_push(&cp->args, "-v");
+	if (suite->verbose_log)
+		argv_array_push(&cp->args, "-V");
+	if (suite->trace)
+		argv_array_push(&cp->args, "-x");
+	if (suite->write_junit_xml)
+		argv_array_push(&cp->args, "--write-junit-xml");
+
+	strbuf_addf(err, "Output of '%s':\n", test);
+	*task_cb = (void *)test;
+
+	return 1;
+}
+
+static int test_finished(int result, struct strbuf *err, void *cb,
+			 void *task_cb)
+{
+	struct testsuite *suite = cb;
+	const char *name = (const char *)task_cb;
+
+	if (result)
+		string_list_append(&suite->failed, name);
+
+	strbuf_addf(err, "%s: '%s'\n", result ? "FAIL" : "SUCCESS", name);
+
+	return 0;
+}
+
+static int test_failed(struct strbuf *out, void *cb, void *task_cb)
+{
+	struct testsuite *suite = cb;
+	const char *name = (const char *)task_cb;
+
+	string_list_append(&suite->failed, name);
+	strbuf_addf(out, "FAILED TO START: '%s'\n", name);
+
+	return 0;
+}
+
+static const char * const testsuite_usage[] = {
+	"test-run-command testsuite [<options>] [<pattern>...]",
+	NULL
+};
+
+static int testsuite(int argc, const char **argv)
+{
+	struct testsuite suite = TESTSUITE_INIT;
+	int max_jobs = 1, i, ret;
+	DIR *dir;
+	struct dirent *d;
+	struct option options[] = {
+		OPT_BOOL('i', "immediate", &suite.immediate,
+			 "stop at first failed test case(s)"),
+		OPT_INTEGER('j', "jobs", &max_jobs, "run <N> jobs in parallel"),
+		OPT_BOOL('q', "quiet", &suite.quiet, "be terse"),
+		OPT_BOOL('v', "verbose", &suite.verbose, "be verbose"),
+		OPT_BOOL('V', "verbose-log", &suite.verbose_log,
+			 "be verbose, redirected to a file"),
+		OPT_BOOL('x', "trace", &suite.trace, "trace shell commands"),
+		OPT_BOOL(0, "write-junit-xml", &suite.write_junit_xml,
+			 "write JUnit-style XML files"),
+		OPT_END()
+	};
+
+	memset(&suite, 0, sizeof(suite));
+	suite.tests.strdup_strings = suite.failed.strdup_strings = 1;
+
+	argc = parse_options(argc, argv, NULL, options,
+			testsuite_usage, PARSE_OPT_STOP_AT_NON_OPTION);
+
+	if (max_jobs <= 0)
+		max_jobs = online_cpus();
+
+	dir = opendir(".");
+	if (!dir)
+		die("Could not open the current directory");
+	while ((d = readdir(dir))) {
+		const char *p = d->d_name;
+
+		if (*p != 't' || !isdigit(p[1]) || !isdigit(p[2]) ||
+		    !isdigit(p[3]) || !isdigit(p[4]) || p[5] != '-' ||
+		    !ends_with(p, ".sh"))
+			continue;
+
+		/* No pattern: match all */
+		if (!argc) {
+			string_list_append(&suite.tests, p);
+			continue;
+		}
+
+		for (i = 0; i < argc; i++)
+			if (!wildmatch(argv[i], p, 0)) {
+				string_list_append(&suite.tests, p);
+				break;
+			}
+	}
+	closedir(dir);
+
+	if (!suite.tests.nr)
+		die("No tests match!");
+	if (max_jobs > suite.tests.nr)
+		max_jobs = suite.tests.nr;
+
+	fprintf(stderr, "Running %d tests (%d at a time)\n",
+		suite.tests.nr, max_jobs);
+
+	ret = run_processes_parallel(max_jobs, next_test, test_failed,
+				     test_finished, &suite);
+
+	if (suite.failed.nr > 0) {
+		ret = 1;
+		fprintf(stderr, "%d tests failed:\n\n", suite.failed.nr);
+		for (i = 0; i < suite.failed.nr; i++)
+			fprintf(stderr, "\t%s\n", suite.failed.items[i].string);
+	}
+
+	string_list_clear(&suite.tests, 0);
+	string_list_clear(&suite.failed, 0);
+
+	return !!ret;
+}
+
 int cmd__run_command(int argc, const char **argv)
 {
 	struct child_process proc = CHILD_PROCESS_INIT;
 	int jobs;
 
+	if (argc > 1 && !strcmp(argv[1], "testsuite"))
+		exit(testsuite(argc - 1, argv + 1));
+
 	if (argc < 3)
 		return 1;
 	while (!strcmp(argv[1], "env")) {

gitgitgadget

From: Johannes Schindelin <[email protected]>

In b18ae14a8f6 (vcxproj: also link-or-copy builtins, 2019-07-29), we started to copy or hard-link the built-ins as a post-build step of the git project.

At the same time, we tried to copy or hard-link git-remote-http.exe, but it is quite possible that it was not built at that time.

Let's move that latter task into a post-install step of the git-remote-http project instead.

---
 config.mak.uname                           | 10 +++++++---
 contrib/buildsystems/Generators/Vcxproj.pm |  3 +++
 2 files changed, 10 insertions(+), 3 deletions(-)

index db7f06b95f..701aad62b1 100644
@@ -703,20 +703,24 @@ vcxproj:
 	perl contrib/buildsystems/generate -g Vcxproj
 	git add -f git.sln {*,*/lib,t/helper/*}/*.vcxproj
 
-	# Generate the LinkOrCopyBuiltins.targets file
+	# Generate the LinkOrCopyBuiltins.targets and LinkOrCopyRemoteHttp.targets file
 	(echo '<Project xmlns="http://schemas.microsoft.com/developer/msbuild/2003">' && \
 	 echo '  <Target Name="CopyBuiltins_AfterBuild" AfterTargets="AfterBuild">' && \
 	 for name in $(BUILT_INS);\
 	 do \
 	   echo '    <Copy SourceFiles="$$(OutDir)\git.exe" DestinationFiles="$$(OutDir)\'"$$name"'" SkipUnchangedFiles="true" UseHardlinksIfPossible="true" />'; \
 	 done && \
+	 echo '  </Target>' && \
+	 echo '</Project>') >git/LinkOrCopyBuiltins.targets
+	(echo '<Project xmlns="http://schemas.microsoft.com/developer/msbuild/2003">' && \
+	 echo '  <Target Name="CopyBuiltins_AfterBuild" AfterTargets="AfterBuild">' && \
 	 for name in $(REMOTE_CURL_ALIASES); \
 	 do \
 	   echo '    <Copy SourceFiles="$$(OutDir)\'"$(REMOTE_CURL_PRIMARY)"'" DestinationFiles="$$(OutDir)\'"$$name"'" SkipUnchangedFiles="true" UseHardlinksIfPossible="true" />'; \
 	 done && \
 	 echo '  </Target>' && \
-	 echo '</Project>') >git/LinkOrCopyBuiltins.targets
-	git add -f git/LinkOrCopyBuiltins.targets
+	 echo '</Project>') >git-remote-http/LinkOrCopyRemoteHttp.targets
+	git add -f git/LinkOrCopyBuiltins.targets git-remote-http/LinkOrCopyRemoteHttp.targets
 
 	# Add command-list.h
 	$(MAKE) MSVC=1 SKIP_VCPKG=1 prefix=/mingw64 command-list.h

index 576ccabe1d..868b787855 100644
@@ -277,6 +277,9 @@ sub createProject {
     if ($target eq 'git') {
       print F "  <Import Project=\"LinkOrCopyBuiltins.targets\" />\n";
     }
+    if ($target eq 'git-remote-http') {
+      print F "  <Import Project=\"LinkOrCopyRemoteHttp.targets\" />\n";
+    }
     print F << "EOM";
 </Project>
 EOM

gitgitgadget

From: Johannes Schindelin <[email protected]>

In the CI builds, we bundle all generated files into a so-called artifacts .tar file, so that the test phase can fan out into multiple parallel builds.

This patch makes sure that all files are included in the vcxproj target which are needed for that artifacts .tar file.

---
 config.mak.uname | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

index 701aad62b1..cc8efd95b1 100644
@@ -728,11 +728,10 @@ vcxproj:
 
 	# Add scripts
 	rm -f perl/perl.mak
-	$(MAKE) MSVC=1 SKIP_VCPKG=1 prefix=/mingw64 \
-		$(SCRIPT_LIB) $(SCRIPT_SH_GEN) $(SCRIPT_PERL_GEN)
+	$(MAKE) MSVC=1 SKIP_VCPKG=1 prefix=/mingw64 $(SCRIPT_LIB) $(SCRIPTS)
 	# Strip out the sane tool path, needed only for building
 	sed -i '/^git_broken_path_fix ".*/d' git-sh-setup
-	git add -f $(SCRIPT_LIB) $(SCRIPT_SH_GEN) $(SCRIPT_PERL_GEN)
+	git add -f $(SCRIPT_LIB) $(SCRIPTS)
 
 	# Add Perl module
 	$(MAKE) $(LIB_PERL_GEN)

@@ -762,6 +761,10 @@ vcxproj:
 	$(MAKE) -C templates
 	git add -f templates/boilerplates.made templates/blt/
 
+	# Add the translated messages
+	make MSVC=1 SKIP_VCPKG=1 prefix=/mingw64 $(MOFILES)
+	git add -f $(MOFILES)
+
 	# Add build options
 	$(MAKE) MSVC=1 SKIP_VCPKG=1 prefix=/mingw64 GIT-BUILD-OPTIONS
 	git add -f GIT-BUILD-OPTIONS

gitgitgadget

From: Johannes Schindelin <[email protected]>

This was a left-over from the previous YAML schema, and it no longer works. The problem was noticed while editing azure-pipelines.yml in VS Code with the very helpful "Azure Pipelines" extension (syntax highlighting and intellisense for azure-pipelines.yml...).

---
 azure-pipelines.yml | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

index c329b7218b..55ee23ad0f 100644
@@ -1,6 +1,5 @@
-resources:
-- repo: self
-  fetchDepth: 1
+variables:
+  Agent.Source.Git.ShallowFetchDepth: 1
 
 jobs:
 - job: windows_build

gitgitgadget

"Johannes Schindelin via GitGitGadget" <[email protected]>

"Johannes Schindelin via GitGitGadget" <[email protected]>

These three look good and too similar to each other, which makes me wonder if we want to allow them simply write

	return insert_pos_as_negative_offset(nr);

with something like

	static int insert_pos_as_negative_offset(uintmax_t nr)
	{
		if (INT_MAX < nr)
			die("overflow: -1 - %"PRIuMAX, nr);
		return -1 - (int)nr;
	}

to avoid repetition.

Hi Junio,

Funny, right? ;-)

I was a bit concerned that GCC did not catch it, nor Clang, but hey, that's the benefit of compiling with more than just one compiler.

Ciao,
Dscho

Hi Junio,

I tried not to do that because there are two different data types in
play: unsigned int and size_t. But I guess by making this an inline function, compilers can optimize for the common case and avoid casting _twice_.

Will be fixed in v2,
Dscho

Git's Continuous Integration (CI) includes an Azure Pipeline that builds Git on Linux, macOS and Windows, in the former two cases even in multiple configurations (using GCC vs clang, 32-bit vs 64-bit, etc). On Windows, we only build using GCC, using (a subset of) Git for Windows' SDK.

Recently, a patch series made it into Git that re-instates the ability to generate project files for use with Visual Studio. The idea there being:
contributors can check out a special branch that has those generated files in one generated commit on top of e.g. Git for Windows' master, allowing the contributors to build Git in Visual Studio, without the need for downloading Git for Windows' SDK (which weighs quite a bit: ~600MB download, ~2GB disk footprint). The tests can then be run from a regular Git for Windows Bash.

This patch series adds that axis to Git's Azure Pipeline: the project files are generated, MSBuild (which is kind of the command-line equivalent of Visual Studio's "Build" operation) is used to build Git, and then a parallelized test job runs the test suite in a Portable Git.

These patches are based on js/visual-studio.

Changes since v2:

* The overflow check introduced in v1 was consolidated into a single helper.

Changes since v1:

* "While at it", we now also check for overflows when doing that -1 - <unsigned> arithmetic.
* The JUnit-style XML is finalized also in case that the script aborts e.g. due to an incorrect number of arguments in a test_expect_success call.

Johannes Schindelin (13):
push: do not pretend to return int from die_push_simple()
msvc: avoid using minus operator on unsigned types
winansi: use FLEX_ARRAY to avoid compiler warning compat/win32/path-utils.h: add #include guards
msvc: ignore some libraries when linking
msvc: handle DEVELOPER=1
msvc: work around a bug in GetEnvironmentVariable()
vcxproj: only copy git-remote-http.exe once it was built
vcxproj: include more generated files
test-tool run-command: learn to run (parts of) the testsuite
tests: let --immediate and --write-junit-xml play well together
ci: really use shallow clones on Azure Pipelines
ci: also build and test with MS Visual Studio on Azure Pipelines

 Makefile                                   |   4 +
 azure-pipelines.yml                        | 164 ++++++++++++++++++++-
 builtin/push.c                             |   4 +-
 cache.h                                    |  13 ++
 compat/mingw.c                             |   2 +
 compat/vcbuild/scripts/clink.pl            |  48 +++++-
 compat/win32/path-utils.h                  |   5 +
 compat/winansi.c                           |   2 +-
 config.mak.uname                           |  19 ++-
 contrib/buildsystems/Generators/Vcxproj.pm |   3 +
 read-cache.c                               |   4 +-
 sha1-lookup.c                              |   4 +-
 t/helper/test-run-command.c                | 153 +++++++++++++++++++
 t/test-lib.sh                              |  38 +++--
 14 files changed, 430 insertions(+), 33 deletions(-)

base-commit: aac6ff7b5beeea9bca66ecda6eec60fc1dd447ec
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-288%2Fdscho%2Fazure-pipelines-msvc-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-288/dscho/azure-pipelines-msvc-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/288

Range-diff vs v2:

  1:  4d0b38125a =  1:  4d0b38125a push: do not pretend to return `int` from `die_push_simple()`
  2:  8800320590 <  -:  ---------- msvc: avoid using minus operator on unsigned types
  -:  ---------- >  2:  7fe2a85506 msvc: avoid using minus operator on unsigned types
  3:  8512a3e96d =  3:  e632a4eef4 winansi: use FLEX_ARRAY to avoid compiler warning
  4:  0345b08f54 =  4:  cd96d7ff70 compat/win32/path-utils.h: add #include guards
  5:  5add01f8ff =  5:  bf09257f65 msvc: ignore some libraries when linking
  6:  5c880f923e =  6:  39c707464c msvc: handle DEVELOPER=1
  7:  1f2245a228 =  7:  91b09cfdd8 msvc: work around a bug in GetEnvironmentVariable()
  8:  582b299634 =  8:  cca891450d vcxproj: only copy `git-remote-http.exe` once it was built
  9:  38ccf999e7 =  9:  e6e60b3c2b vcxproj: include more generated files
 10:  24b1c7bff3 = 10:  d2c4973431 test-tool run-command: learn to run (parts of) the testsuite
 11:  7be13d19e1 = 11:  0644a2f8da tests: let --immediate and --write-junit-xml play well together
 12:  bde1e8ef65 = 12:  4495c392fd ci: really use shallow clones on Azure Pipelines
 13:  7af1c01a08 = 13:  b1ff8eff4d ci: also build and test with MS Visual Studio on Azure Pipelines

gitgitgadget

From: Johannes Schindelin <[email protected]>

This function is marked as NORETURN, and it indeed does not want to return anything. So let's not declare it with the return type int. This fixes the following warning when building with MSVC:

C4646: function declared with 'noreturn' has non-void return type

---
 builtin/push.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index 021dd3b1e4..d216270d5f 100644
@@ -143,8 +143,8 @@ static int push_url_of_remote(struct remote *remote, const char ***url_p)
 	return remote->url_nr;
 }
 
-static NORETURN int die_push_simple(struct branch *branch,
-				    struct remote *remote)
+static NORETURN void die_push_simple(struct branch *branch,
+				     struct remote *remote)
 {
 	/*
 	 * There's no point in using shorten_unambiguous_ref here,

gitgitgadget

From: Johannes Schindelin <[email protected]>

MSVC would complain thusly:

C4200: nonstandard extension used: zero-sized array in struct/union

Let's just use the FLEX_ARRAY constant that we introduced for exactly this type of scenario.

---
 compat/winansi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index cacd82c833..54fd701cbf 100644
@@ -546,7 +546,7 @@ static HANDLE swap_osfhnd(int fd, HANDLE new_handle)
 typedef struct _OBJECT_NAME_INFORMATION
 {
 	UNICODE_STRING Name;
-	WCHAR NameBuffer[0];
+	WCHAR NameBuffer[FLEX_ARRAY];
 } OBJECT_NAME_INFORMATION, *POBJECT_NAME_INFORMATION;
 
 #define ObjectNameInformation 1

gitgitgadget

From: Johannes Schindelin <[email protected]>

To build with MSVC, we "translate" GCC options to MSVC options, and part of those options refer to the libraries to link into the final executable. Currently, this part looks somewhat like this on Windows:

	-lcurl -lnghttp2 -lidn2 -lssl -lcrypto -lssl -lcrypto -lgdi32
	-lcrypt32 -lwldap32 -lz -lws2_32 -lexpat

Some of those are direct dependencies (such as curl and ssl) and others are indirect (nghttp2 and idn2, for example, are dependencies of curl, but need to be linked in for reasons).

We already handle the direct dependencies, e.g. -liconv is already handled as adding libiconv.lib to the list of libraries to link against.

Let's just ignore the remaining -l* options so that MSVC does not have to warn us that it ignored e.g. the /lnghttp2 option. We do that by extending the clause that already "eats" the -R* options to also eat the -l* options.

---
 compat/vcbuild/scripts/clink.pl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index c7b021bfac..00fc339cba 100755
@@ -68,7 +68,7 @@
 	} elsif ("$arg" =~ /^-L/ && "$arg" ne "-LTCG") {
 		$arg =~ s/^-L/-LIBPATH:/;
 		push(@lflags, $arg);
-	} elsif ("$arg" =~ /^-R/) {
+	} elsif ("$arg" =~ /^-[Rl]/) {
 		# eat
 	} else {
 		push(@args, $arg);

gitgitgadget

From: Johannes Schindelin <[email protected]>

MSVC complains about this with -Wall, which can be taken as a sign that this is indeed a real bug. The symptom is:

C4146: unary minus operator applied to unsigned type, result still unsigned

Let's avoid this warning in the minimal way, e.g. writing -1 - <unsigned value> instead of -<unsigned value> - 1.

Note that the change in the estimate_cache_size() function is needed because MSVC considers the "return type" of the sizeof() operator to be size_t, i.e. unsigned, and therefore it cannot be negated using the unary minus operator.

Even worse, that arithmetic is doing extra work, in vain. We want to calculate the entry extra cache size as the difference between the size of the cache_entry structure minus the size of the ondisk_cache_entry structure, padded to the appropriate alignment boundary.

To that end, we start by assigning that difference to the per_entry variable, and then abuse the len parameter of the align_padding_size() macro to take the negative size of the ondisk entry size. Essentially, we try to avoid passing the already calculated difference to that macro by passing the operands of that difference instead, when the macro expects operands of an addition:

	#define align_padding_size(size, len) \
		((size + (len) + 8) & ~7) - (size + len)

Currently, we pass A and -B to that macro instead of passing A - B and 0, where A - B is already stored in the per_entry variable, ready to be used.

This is neither necessary, nor intuitive. Let's fix this, and have code that is both easier to read and that also does not trigger MSVC's warning.

While at it, we take care of reporting overflows (which are unlikely, but hey, defensive programming is good!).

We _also_ take pains of casting the unsigned value to signed: otherwise, the signed operand (i.e. the -1) would be cast to unsigned before doing the arithmetic.

---
 cache.h       | 13 +++++++++++++
 read-cache.c  |  4 ++--
 sha1-lookup.c |  4 ++--
 3 files changed, 17 insertions(+), 4 deletions(-)

index 3167585cab..850e7b945a 100644
@@ -725,6 +725,19 @@ struct cache_entry *index_file_exists(struct index_state *istate, const char *na
  */
 int index_name_pos(const struct index_state *, const char *name, int namelen);
 
+/*
+ * Some functions return the negative complement of an insert position when a
+ * precise match was not found but a position was found where the entry would
+ * need to be inserted. This helper protects that logic from any integer
+ * underflow.
+ */
+static inline int index_pos_to_insert_pos(uintmax_t pos)
+{
+	if (pos > INT_MAX)
+		die("overflow: -1 - %"PRIuMAX, pos);
+	return -1 - (int)pos;
+}
+
 #define ADD_CACHE_OK_TO_ADD 1		/* Ok to add */
 #define ADD_CACHE_OK_TO_REPLACE 2	/* Ok to replace file/directory */
 #define ADD_CACHE_SKIP_DFCHECK 4	/* Ok to skip DF conflict checks */

index c701f7f8b8..ec13953a21 100644
@@ -1276,7 +1276,7 @@ static int add_index_entry_with_check(struct index_state *istate, struct cache_e
 	 */
 	if (istate->cache_nr > 0 &&
 		strcmp(ce->name, istate->cache[istate->cache_nr - 1]->name) > 0)
-		pos = -istate->cache_nr - 1;
+		pos = index_pos_to_insert_pos(istate->cache_nr);
 	else
 		pos = index_name_stage_pos(istate, ce->name, ce_namelen(ce), ce_stage(ce));
 

@@ -1894,7 +1894,7 @@ static size_t estimate_cache_size(size_t ondisk_size, unsigned int entries)
 	/*
 	 * Account for potential alignment differences.
 	 */
-	per_entry += align_padding_size(sizeof(struct cache_entry), -sizeof(struct ondisk_cache_entry));
+	per_entry += align_padding_size(per_entry, 0);
 	return ondisk_size + entries * per_entry;
 }
 

index 796ab68da8..8590aac254 100644
@@ -70,7 +70,7 @@ int sha1_pos(const unsigned char *sha1, void *table, size_t nr,
 			if (miv < lov)
 				return -1;
 			if (hiv < miv)
-				return -1 - nr;
+				return index_pos_to_insert_pos(nr);
 			if (lov != hiv) {
 				/*
 				 * At this point miv could be equal

@@ -97,7 +97,7 @@ int sha1_pos(const unsigned char *sha1, void *table, size_t nr,
 			lo = mi + 1;
 		mi = lo + (hi - lo) / 2;
 	} while (lo < hi);
-	return -lo-1;
+	return index_pos_to_insert_pos(lo);
 }
 
 int bsearch_hash(const unsigned char *sha1, const uint32_t *fanout_nbo,

gitgitgadget

From: Johannes Schindelin <[email protected]>

The return value of that function is 0 both for variables that are unset, as well as for variables whose values are empty. To discern those two cases, one has to call GetLastError(), whose return value is ERROR_ENVVAR_NOT_FOUND and ERROR_SUCCESS, respectively.

Except that it is not actually set to ERROR_SUCCESS in the latter case, apparently, but the last error value seems to be simply unchanged.

To work around this, let's just re-set the last error value just before inspecting the environment variable.

This fixes a problem that triggers failures in t3301-notes.sh (where we try to override config settings by passing empty values for certain environment variables).

This problem is hidden in the MINGW build by the fact that older MSVC runtimes (such as the one used by MINGW builds) have a calloc() that re-sets the last error value in case of success, while newer runtimes set the error value only if NULL is returned by that function.

---
 compat/mingw.c | 2 ++
 1 file changed, 2 insertions(+)

index 4891789771..7d8cb814ba 100644
@@ -1661,6 +1661,8 @@ char *mingw_getenv(const char *name)
 	if (!w_key)
 		die("Out of memory, (tried to allocate %u wchar_t's)", len_key);
 	xutftowcs(w_key, name, len_key);
+	/* GetEnvironmentVariableW() only sets the last error upon failure */
+	SetLastError(ERROR_SUCCESS);
 	len_value = GetEnvironmentVariableW(w_key, w_value, ARRAY_SIZE(w_value));
 	if (!len_value && GetLastError() == ERROR_ENVVAR_NOT_FOUND) {
 		free(w_key);

gitgitgadget

From: Johannes Schindelin <[email protected]>

In b18ae14a8f6 (vcxproj: also link-or-copy builtins, 2019-07-29), we started to copy or hard-link the built-ins as a post-build step of the git project.

At the same time, we tried to copy or hard-link git-remote-http.exe, but it is quite possible that it was not built at that time.

Let's move that latter task into a post-install step of the git-remote-http project instead.

---
 config.mak.uname                           | 10 +++++++---
 contrib/buildsystems/Generators/Vcxproj.pm |  3 +++
 2 files changed, 10 insertions(+), 3 deletions(-)

index db7f06b95f..701aad62b1 100644
@@ -703,20 +703,24 @@ vcxproj:
 	perl contrib/buildsystems/generate -g Vcxproj
 	git add -f git.sln {*,*/lib,t/helper/*}/*.vcxproj
 
-	# Generate the LinkOrCopyBuiltins.targets file
+	# Generate the LinkOrCopyBuiltins.targets and LinkOrCopyRemoteHttp.targets file
 	(echo '<Project xmlns="http://schemas.microsoft.com/developer/msbuild/2003">' && \
 	 echo '  <Target Name="CopyBuiltins_AfterBuild" AfterTargets="AfterBuild">' && \
 	 for name in $(BUILT_INS);\
 	 do \
 	   echo '    <Copy SourceFiles="$$(OutDir)\git.exe" DestinationFiles="$$(OutDir)\'"$$name"'" SkipUnchangedFiles="true" UseHardlinksIfPossible="true" />'; \
 	 done && \
+	 echo '  </Target>' && \
+	 echo '</Project>') >git/LinkOrCopyBuiltins.targets
+	(echo '<Project xmlns="http://schemas.microsoft.com/developer/msbuild/2003">' && \
+	 echo '  <Target Name="CopyBuiltins_AfterBuild" AfterTargets="AfterBuild">' && \
 	 for name in $(REMOTE_CURL_ALIASES); \
 	 do \
 	   echo '    <Copy SourceFiles="$$(OutDir)\'"$(REMOTE_CURL_PRIMARY)"'" DestinationFiles="$$(OutDir)\'"$$name"'" SkipUnchangedFiles="true" UseHardlinksIfPossible="true" />'; \
 	 done && \
 	 echo '  </Target>' && \
-	 echo '</Project>') >git/LinkOrCopyBuiltins.targets
-	git add -f git/LinkOrCopyBuiltins.targets
+	 echo '</Project>') >git-remote-http/LinkOrCopyRemoteHttp.targets
+	git add -f git/LinkOrCopyBuiltins.targets git-remote-http/LinkOrCopyRemoteHttp.targets
 
 	# Add command-list.h
 	$(MAKE) MSVC=1 SKIP_VCPKG=1 prefix=/mingw64 command-list.h

index 576ccabe1d..868b787855 100644
@@ -277,6 +277,9 @@ sub createProject {
     if ($target eq 'git') {
       print F "  <Import Project=\"LinkOrCopyBuiltins.targets\" />\n";
     }
+    if ($target eq 'git-remote-http') {
+      print F "  <Import Project=\"LinkOrCopyRemoteHttp.targets\" />\n";
+    }
     print F << "EOM";
 </Project>
 EOM

gitgitgadget

From: Johannes Schindelin <[email protected]>

In the CI builds, we bundle all generated files into a so-called artifacts .tar file, so that the test phase can fan out into multiple parallel builds.

This patch makes sure that all files are included in the vcxproj target which are needed for that artifacts .tar file.

---
 config.mak.uname | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

index 701aad62b1..cc8efd95b1 100644
@@ -728,11 +728,10 @@ vcxproj:
 
 	# Add scripts
 	rm -f perl/perl.mak
-	$(MAKE) MSVC=1 SKIP_VCPKG=1 prefix=/mingw64 \
-		$(SCRIPT_LIB) $(SCRIPT_SH_GEN) $(SCRIPT_PERL_GEN)
+	$(MAKE) MSVC=1 SKIP_VCPKG=1 prefix=/mingw64 $(SCRIPT_LIB) $(SCRIPTS)
 	# Strip out the sane tool path, needed only for building
 	sed -i '/^git_broken_path_fix ".*/d' git-sh-setup
-	git add -f $(SCRIPT_LIB) $(SCRIPT_SH_GEN) $(SCRIPT_PERL_GEN)
+	git add -f $(SCRIPT_LIB) $(SCRIPTS)
 
 	# Add Perl module
 	$(MAKE) $(LIB_PERL_GEN)

@@ -762,6 +761,10 @@ vcxproj:
 	$(MAKE) -C templates
 	git add -f templates/boilerplates.made templates/blt/
 
+	# Add the translated messages
+	make MSVC=1 SKIP_VCPKG=1 prefix=/mingw64 $(MOFILES)
+	git add -f $(MOFILES)
+
 	# Add build options
 	$(MAKE) MSVC=1 SKIP_VCPKG=1 prefix=/mingw64 GIT-BUILD-OPTIONS
 	git add -f GIT-BUILD-OPTIONS

gitgitgadget

From: Johannes Schindelin <[email protected]>

This adds the common guards that allow headers to be #include'd multiple times.

---
 compat/win32/path-utils.h | 5 +++++
 1 file changed, 5 insertions(+)

index 0f70d43920..8ed062a6b7 100644
@@ -1,3 +1,6 @@
+#ifndef WIN32_PATH_UTILS_H
+#define WIN32_PATH_UTILS_H
+
 #define has_dos_drive_prefix(path) \
 	(isalpha(*(path)) && (path)[1] == ':' ? 2 : 0)
 int win32_skip_dos_drive_prefix(char **path);

@@ -18,3 +21,5 @@ static inline char *win32_find_last_dir_sep(const char *path)
 #define find_last_dir_sep win32_find_last_dir_sep
 int win32_offset_1st_component(const char *path);
 #define offset_1st_component win32_offset_1st_component
+
+#endif

gitgitgadget

From: Johannes Schindelin <[email protected]>

This was a left-over from the previous YAML schema, and it no longer works. The problem was noticed while editing azure-pipelines.yml in VS Code with the very helpful "Azure Pipelines" extension (syntax highlighting and intellisense for azure-pipelines.yml...).

---
 azure-pipelines.yml | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

index c329b7218b..55ee23ad0f 100644
@@ -1,6 +1,5 @@
-resources:
-- repo: self
-  fetchDepth: 1
+variables:
+  Agent.Source.Git.ShallowFetchDepth: 1
 
 jobs:
 - job: windows_build

gitgitgadget

From: Johannes Schindelin <[email protected]>

We frequently build Git using the DEVELOPER=1 make setting as a shortcut to enable all kinds of more stringent compiler warnings.

Those compiler warnings are relatively specific to GCC, though, so let's try our best to translate them to the equivalent options to pass to MS Visual C++'s cl.exe.

---
 compat/vcbuild/scripts/clink.pl | 46 +++++++++++++++++++++++++++++++++
 1 file changed, 46 insertions(+)

index 00fc339cba..ec95a3b2d0 100755
@@ -70,6 +70,52 @@
 		push(@lflags, $arg);
 	} elsif ("$arg" =~ /^-[Rl]/) {
 		# eat
+	} elsif ("$arg" eq "-Werror") {
+		push(@cflags, "-WX");
+	} elsif ("$arg" eq "-Wall") {
+		# cl.exe understands -Wall, but it is really overzealous
+		push(@cflags, "-W4");
+		# disable the "signed/unsigned mismatch" warnings; our source code violates that
+		push(@cflags, "-wd4018");
+		push(@cflags, "-wd4245");
+		push(@cflags, "-wd4389");
+		# disable the "unreferenced formal parameter" warning; our source code violates that
+		push(@cflags, "-wd4100");
+		# disable the "conditional expression is constant" warning; our source code violates that
+		push(@cflags, "-wd4127");
+		# disable the "const object should be initialized" warning; these warnings affect only objects that are `static`
+		push(@cflags, "-wd4132");
+		# disable the "function/data pointer conversion in expression" warning; our source code violates that
+		push(@cflags, "-wd4152");
+		# disable the "non-constant aggregate initializer" warning; our source code violates that
+		push(@cflags, "-wd4204");
+		# disable the "cannot be initialized using address of automatic variable" warning; our source code violates that
+		push(@cflags, "-wd4221");
+		# disable the "possible loss of data" warnings; our source code violates that
+		push(@cflags, "-wd4244");
+		push(@cflags, "-wd4267");
+		# disable the "array is too small to include a terminating null character" warning; we ab-use strings to initialize OIDs
+		push(@cflags, "-wd4295");
+		# disable the "'<<': result of 32-bit shift implicitly converted to 64 bits" warning; our source code violates that
+		push(@cflags, "-wd4334");
+		# disable the "declaration hides previous local declaration" warning; our source code violates that
+		push(@cflags, "-wd4456");
+		# disable the "declaration hides function parameter" warning; our source code violates that
+		push(@cflags, "-wd4457");
+		# disable the "declaration hides global declaration" warning; our source code violates that
+		push(@cflags, "-wd4459");
+		# disable the "potentially uninitialized local variable '<name>' used" warning; our source code violates that
+		push(@cflags, "-wd4701");
+		# disable the "unreachable code" warning; our source code violates that
+		push(@cflags, "-wd4702");
+		# disable the "potentially uninitialized local pointer variable used" warning; our source code violates that
+		push(@cflags, "-wd4703");
+		# disable the "assignment within conditional expression" warning; our source code violates that
+		push(@cflags, "-wd4706");
+		# disable the "'inet_ntoa': Use inet_ntop() or InetNtop() instead" warning; our source code violates that
+		push(@cflags, "-wd4996");
+	} elsif ("$arg" =~ /^-W[a-z]/) {
+		# let's ignore those
 	} else {
 		push(@args, $arg);
 	}

gitgitgadget

From: Johannes Schindelin <[email protected]>

When the --immediate option is in effect, any test failure will immediately exit the test script. Together with --write-junit-xml, we will want the JUnit-style .xml file to be finalized (and not leave the XML incomplete). Let's make it so.

This comes in particularly handy when trying to debug via Azure Pipelines, where the JUnit-style XML is consumed to present the test results in an informative and helpful way.

While at it, also handle the error() code path.

The only remaining code path that sets GIT_EXIT_OK happens whenever the trash directory could not be set up, i.e. long before the JUnit XML was written, therefore we should _not_ try to finalize that XML in that case.

It is tempting to change the immediate code path to just hand off to error, simplifying the code in the process. That would, however, result in a change of behavior (an additional error message) in the test suite, which is outside of the purview of the current patch series: its goal is to allow building Git with Visual Studio and testing it with a portable version of Git for Windows.

---
 t/test-lib.sh | 38 ++++++++++++++++++++++----------------
 1 file changed, 22 insertions(+), 16 deletions(-)

index d1ba33745a..86b5e6133b 100644
@@ -567,6 +567,7 @@ export TERM
 
 error () {
 	say_color error "error: $*"
+	finalize_junit_xml
 	GIT_EXIT_OK=t
 	exit 1
 }

@@ -695,7 +696,7 @@ test_failure_ () {
 	say_color error "not ok $test_count - $1"
 	shift
 	printf '%s\n' "$*" | sed -e 's/^/#	/'
-	test "$immediate" = "" || { GIT_EXIT_OK=t; exit 1; }
+	test "$immediate" = "" || { finalize_junit_xml; GIT_EXIT_OK=t; exit 1; }
 }
 
 test_known_broken_ok_ () {

@@ -1063,6 +1064,25 @@ write_junit_xml_testcase () {
 	junit_have_testcase=t
 }
 
+finalize_junit_xml () {
+	if test -n "$write_junit_xml" && test -n "$junit_xml_path"
+	then
+		test -n "$junit_have_testcase" || {
+			junit_start=$(test-tool date getnanos)
+			write_junit_xml_testcase "all tests skipped"
+		}
+
+		# adjust the overall time
+		junit_time=$(test-tool date getnanos $junit_suite_start)
+		sed "s/<testsuite [^>]*/& time=\"$junit_time\"/" \
+			<"$junit_xml_path" >"$junit_xml_path.new"
+		mv "$junit_xml_path.new" "$junit_xml_path"
+
+		write_junit_xml "  </testsuite>" "</testsuites>"
+		write_junit_xml=
+	fi
+}
+
 test_atexit_cleanup=:
 test_atexit_handler () {
 	# In a succeeding test script 'test_atexit_handler' is invoked

@@ -1085,21 +1105,7 @@ test_done () {
 	# removed, so the commands can access pidfiles and socket files.
 	test_atexit_handler
 
-	if test -n "$write_junit_xml" && test -n "$junit_xml_path"
-	then
-		test -n "$junit_have_testcase" || {
-			junit_start=$(test-tool date getnanos)
-			write_junit_xml_testcase "all tests skipped"
-		}
-
-		# adjust the overall time
-		junit_time=$(test-tool date getnanos $junit_suite_start)
-		sed "s/<testsuite [^>]*/& time=\"$junit_time\"/" \
-			<"$junit_xml_path" >"$junit_xml_path.new"
-		mv "$junit_xml_path.new" "$junit_xml_path"
-
-		write_junit_xml "  </testsuite>" "</testsuites>"
-	fi
+	finalize_junit_xml
 
 	if test -z "$HARNESS_ACTIVE"
 	then

gitgitgadget

From: Johannes Schindelin <[email protected]>

Git for Windows jumps through hoops to provide a development environment that allows to build Git and to run its test suite. To that end, an entire MSYS2 system, including GNU make and GCC is offered as "the Git for Windows SDK". It does come at a price: an initial download of said SDK weighs in with several hundreds of megabytes, and the unpacked SDK occupies ~2GB of disk space.

A much more native development environment on Windows is Visual Studio. To help contributors use that environment, we already have a Makefile target vcxproj that generates a commit with project files (and other generated files), and Git for Windows' vs/master branch is continuously re-generated using that target.

The idea is to allow building Git in Visual Studio, and to run individual tests using a Portable Git.

The one missing thing is a way to run the entire test suite: neither make nor prove are required to run Git, therefore Git for Windows does not support those commands in the Portable Git.

To help with that, add a simple test helper that exercises the run_processes_parallel() function to allow for running test scripts in parallel (which is really necessary, especially on Windows, as Git's test suite takes such a long time to run).

This will also come in handy for the upcoming change to our Azure
Pipeline: we will use this helper in a Portable Git to test the Visual Studio build of Git.

---
 t/helper/test-run-command.c | 153 ++++++++++++++++++++++++++++++++++++
 1 file changed, 153 insertions(+)

index 2cc93bb69c..ead6dc611a 100644
@@ -10,9 +10,14 @@
 
 #include "test-tool.h"
 #include "git-compat-util.h"
+#include "cache.h"
 #include "run-command.h"
 #include "argv-array.h"
 #include "strbuf.h"
+#include "parse-options.h"
+#include "string-list.h"
+#include "thread-utils.h"
+#include "wildmatch.h"
 #include <string.h>
 #include <errno.h>
 

@@ -50,11 +55,159 @@ static int task_finished(int result,
 	return 1;
 }
 
+struct testsuite {
+	struct string_list tests, failed;
+	int next;
+	int quiet, immediate, verbose, verbose_log, trace, write_junit_xml;
+};
+#define TESTSUITE_INIT \
+	{ STRING_LIST_INIT_DUP, STRING_LIST_INIT_DUP, -1, 0, 0, 0, 0, 0, 0 }
+
+static int next_test(struct child_process *cp, struct strbuf *err, void *cb,
+		     void **task_cb)
+{
+	struct testsuite *suite = cb;
+	const char *test;
+	if (suite->next >= suite->tests.nr)
+		return 0;
+
+	test = suite->tests.items[suite->next++].string;
+	argv_array_pushl(&cp->args, "sh", test, NULL);
+	if (suite->quiet)
+		argv_array_push(&cp->args, "--quiet");
+	if (suite->immediate)
+		argv_array_push(&cp->args, "-i");
+	if (suite->verbose)
+		argv_array_push(&cp->args, "-v");
+	if (suite->verbose_log)
+		argv_array_push(&cp->args, "-V");
+	if (suite->trace)
+		argv_array_push(&cp->args, "-x");
+	if (suite->write_junit_xml)
+		argv_array_push(&cp->args, "--write-junit-xml");
+
+	strbuf_addf(err, "Output of '%s':\n", test);
+	*task_cb = (void *)test;
+
+	return 1;
+}
+
+static int test_finished(int result, struct strbuf *err, void *cb,
+			 void *task_cb)
+{
+	struct testsuite *suite = cb;
+	const char *name = (const char *)task_cb;
+
+	if (result)
+		string_list_append(&suite->failed, name);
+
+	strbuf_addf(err, "%s: '%s'\n", result ? "FAIL" : "SUCCESS", name);
+
+	return 0;
+}
+
+static int test_failed(struct strbuf *out, void *cb, void *task_cb)
+{
+	struct testsuite *suite = cb;
+	const char *name = (const char *)task_cb;
+
+	string_list_append(&suite->failed, name);
+	strbuf_addf(out, "FAILED TO START: '%s'\n", name);
+
+	return 0;
+}
+
+static const char * const testsuite_usage[] = {
+	"test-run-command testsuite [<options>] [<pattern>...]",
+	NULL
+};
+
+static int testsuite(int argc, const char **argv)
+{
+	struct testsuite suite = TESTSUITE_INIT;
+	int max_jobs = 1, i, ret;
+	DIR *dir;
+	struct dirent *d;
+	struct option options[] = {
+		OPT_BOOL('i', "immediate", &suite.immediate,
+			 "stop at first failed test case(s)"),
+		OPT_INTEGER('j', "jobs", &max_jobs, "run <N> jobs in parallel"),
+		OPT_BOOL('q', "quiet", &suite.quiet, "be terse"),
+		OPT_BOOL('v', "verbose", &suite.verbose, "be verbose"),
+		OPT_BOOL('V', "verbose-log", &suite.verbose_log,
+			 "be verbose, redirected to a file"),
+		OPT_BOOL('x', "trace", &suite.trace, "trace shell commands"),
+		OPT_BOOL(0, "write-junit-xml", &suite.write_junit_xml,
+			 "write JUnit-style XML files"),
+		OPT_END()
+	};
+
+	memset(&suite, 0, sizeof(suite));
+	suite.tests.strdup_strings = suite.failed.strdup_strings = 1;
+
+	argc = parse_options(argc, argv, NULL, options,
+			testsuite_usage, PARSE_OPT_STOP_AT_NON_OPTION);
+
+	if (max_jobs <= 0)
+		max_jobs = online_cpus();
+
+	dir = opendir(".");
+	if (!dir)
+		die("Could not open the current directory");
+	while ((d = readdir(dir))) {
+		const char *p = d->d_name;
+
+		if (*p != 't' || !isdigit(p[1]) || !isdigit(p[2]) ||
+		    !isdigit(p[3]) || !isdigit(p[4]) || p[5] != '-' ||
+		    !ends_with(p, ".sh"))
+			continue;
+
+		/* No pattern: match all */
+		if (!argc) {
+			string_list_append(&suite.tests, p);
+			continue;
+		}
+
+		for (i = 0; i < argc; i++)
+			if (!wildmatch(argv[i], p, 0)) {
+				string_list_append(&suite.tests, p);
+				break;
+			}
+	}
+	closedir(dir);
+
+	if (!suite.tests.nr)
+		die("No tests match!");
+	if (max_jobs > suite.tests.nr)
+		max_jobs = suite.tests.nr;
+
+	fprintf(stderr, "Running %d tests (%d at a time)\n",
+		suite.tests.nr, max_jobs);
+
+	ret = run_processes_parallel(max_jobs, next_test, test_failed,
+				     test_finished, &suite);
+
+	if (suite.failed.nr > 0) {
+		ret = 1;
+		fprintf(stderr, "%d tests failed:\n\n", suite.failed.nr);
+		for (i = 0; i < suite.failed.nr; i++)
+			fprintf(stderr, "\t%s\n", suite.failed.items[i].string);
+	}
+
+	string_list_clear(&suite.tests, 0);
+	string_list_clear(&suite.failed, 0);
+
+	return !!ret;
+}
+
 int cmd__run_command(int argc, const char **argv)
 {
 	struct child_process proc = CHILD_PROCESS_INIT;
 	int jobs;
 
+	if (argc > 1 && !strcmp(argv[1], "testsuite"))
+		exit(testsuite(argc - 1, argv + 1));
+
 	if (argc < 3)
 		return 1;
 	while (!strcmp(argv[1], "env")) {

gitgitgadget

From: Johannes Schindelin <[email protected]>

... because we can, now. Technically, we actually build using MSBuild, which is however pretty close to building interactively in Visual Studio.

As there is no convenient way to run Git's test suite in Visual Studio, we unpack a Portable Git to run it, using the just-added test helper to allow running test scripts in parallel.

---
 Makefile            |   4 ++
 azure-pipelines.yml | 159 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 163 insertions(+)

index 3716dadc08..3f6dcec54b 100644
@@ -3025,6 +3025,10 @@ rpm::
 	@false
 .PHONY: rpm
 
+ifneq ($(INCLUDE_DLLS_IN_ARTIFACTS),)
+OTHER_PROGRAMS += $(shell echo *.dll t/helper/*.dll)
+endif
+
 artifacts-tar:: $(ALL_PROGRAMS) $(SCRIPT_LIB) $(BUILT_INS) $(OTHER_PROGRAMS) \
 		GIT-BUILD-OPTIONS $(TEST_PROGRAMS) $(test_bindir_programs) \
 		$(MOFILES)

gitgitgadget

Am 04.10.19 um 11:55 schrieb Johannes Schindelin:

IMHO, if you don't accompany insert_pos_as_negative_offset() with a corresponding extract_pos_and_found_condition() and use it everywhere, it is more obfuscating than necessary.

The *real* problem to solve is to ensure that the index/cache does not grow so large that 32-bit indexes would be needed. Then the calculation that you want to hide here cannot overflow.

-- Hannes

Hi Hannes,

I do disagree here. No overflow checking needs to be performed for -1 - <int-value>. And that's what the opposite of this function really boils down to.

Well, that may be the real problem of another patch series. This patch series' problem is to add a job to our Azure Pipeline that builds Git with Visual Studio, and it patches the code minimally so that it builds even in DEVELOPER=1 mode, for good measure.

So I'd like not to dilute the purpose of this patch series.

Thanks,
Dscho

I do not think j6t is referring to the over/underflow issues at all.

The suggestion is that, because insert-pos-as-negative-offset abstracts away (in addition to the overflow checks) the fact that "does not exist but here is the location it would be inserted" is encoded in a certain way (i.e. not just the array index negated, but also is offset by -1, because we wouldn't be able to say "at the very beginning at index 0" without the -1 offset), the side that consumes the encoded "pos" (i.e. "we got a negative, so we know the element does not exist, and the index into the array we would insert a new element is computed this way") should be abstracted away, as it must know that the extra negative offset used when encoding is "-1".

I think that is a reasonable thing to consider; it is not necessary for correctness, but contributes to the conceptual clarity (iow, it can be left as a separate clean-up step done after the series is done).

"Johannes Schindelin via GitGitGadget" <[email protected]>

Looks good to me.

This is less useful than it could be.

With a larger creation-factor (and we can afford using a larger one, simply because the user of GGG _knows_ that the two series being compared are closely related), what is output is entirely readable (attached at the end).

Oh, while I am suggesting possible improvements on GGG, can we please tweak the sender date like git-send-email does so that two messages in the same series do not share the same timestamp? When multi-patch series are displayed in MUA or public-inbox News feed out of order, it almost always is from GGG that gave the same timestamp to adjacent messages in a series, and it prevents me from applying them in one go (or saving in one action to a mbox).

What send-email does is, at the beginning for N patch series, to take the current wallclock time and subtract N seconds from it, and then give that timestamp to the first message it sends out, and after that, it increments the timestamp by 1 seconds.

Note that there is no need for any "sleep"---the timestamps are given by explicitly generating the "Date: " header. The last time we looked into this issue, I think the code was trying to do almost the right thing but it was giving a malformatted timezone and forcing the sending MTA to override it with the wallclock time or something.

Thanks.

1: 9629f3c751 ! 1: c097b95a26 msvc: avoid using minus operator on unsigned types

    @@ Commit message
         Signed-off-by: Johannes Schindelin <[email protected]>
         Signed-off-by: Junio C Hamano <[email protected]>
     
    + ## cache.h ##
    +@@ cache.h: struct cache_entry *index_file_exists(struct index_state *istate, const char *na
    +  */
    + int index_name_pos(const struct index_state *, const char *name, int namelen);
    + 
    ++/*
    ++ * Some functions return the negative complement of an insert position when a
    ++ * precise match was not found but a position was found where the entry would
    ++ * need to be inserted. This helper protects that logic from any integer
    ++ * underflow.
    ++ */
    ++static inline int index_pos_to_insert_pos(uintmax_t pos)
    ++{
    ++	if (pos > INT_MAX)
    ++		die("overflow: -1 - %"PRIuMAX, pos);
    ++	return -1 - (int)pos;
    ++}
    ++
    + #define ADD_CACHE_OK_TO_ADD 1		/* Ok to add */
    + #define ADD_CACHE_OK_TO_REPLACE 2	/* Ok to replace file/directory */
    + #define ADD_CACHE_SKIP_DFCHECK 4	/* Ok to skip DF conflict checks */
    +
      ## read-cache.c ##
     @@ read-cache.c: static int add_index_entry_with_check(struct index_state *istate, struct cache_e
    - 	 * we can avoid searching for it.
      	 */
      	if (istate->cache_nr > 0 &&
    --		strcmp(ce->name, istate->cache[istate->cache_nr - 1]->name) > 0)
    + 		strcmp(ce->name, istate->cache[istate->cache_nr - 1]->name) > 0)
     -		pos = -istate->cache_nr - 1;
    -+		strcmp(ce->name, istate->cache[istate->cache_nr - 1]->name) > 0) {
    -+		if (istate->cache_nr > INT_MAX)
    -+			die("overflow: -1 - %u", istate->cache_nr);
    -+		pos = -1 - (int)istate->cache_nr;
    -+	}
    ++		pos = index_pos_to_insert_pos(istate->cache_nr);
      	else
      		pos = index_name_stage_pos(istate, ce->name, ce_namelen(ce), ce_stage(ce));
      
    @@ read-cache.c: static size_t estimate_cache_size(size_t ondisk_size, unsigned int
     
      ## sha1-lookup.c ##
     @@ sha1-lookup.c: int sha1_pos(const unsigned char *sha1, void *table, size_t nr,
    - 			miv = take2(sha1 + ofs);
      			if (miv < lov)
      				return -1;
    --			if (hiv < miv)
    + 			if (hiv < miv)
     -				return -1 - nr;
    -+			if (hiv < miv) {
    -+				if (nr > INT_MAX)
    -+					die("overflow: -1 - %"PRIuMAX,
    -+					    (uintmax_t)nr);
    -+				return -1 - (int)nr;
    -+			}
    ++				return index_pos_to_insert_pos(nr);
      			if (lov != hiv) {
      				/*
      				 * At this point miv could be equal
    @@ sha1-lookup.c: int sha1_pos(const unsigned char *sha1, void *table, size_t nr,
      		mi = lo + (hi - lo) / 2;
      	} while (lo < hi);
     -	return -lo-1;
    -+	if (nr > INT_MAX)
    -+		die("overflow: -1 - %"PRIuMAX, (uintmax_t)lo);
    -+	return -1 - (int)lo;
    ++	return index_pos_to_insert_pos(lo);
      }
      
      int bsearch_hash(const unsigned char *sha1, const uint32_t *fanout_nbo,

Hi Junio,

I just implemented this here:
https://github.com/gitgitgadget/gitgitgadget/pull/128 (it still needs to be reviewed and merged before it takes effect).

You mentioned this before, and I implemented it. But GMail ignores the Date: header sent by GitGitGadget, and I don't know why. See e.g.
https://public-inbox.org/git/4d0b38125a13d85963be5e524becf48271893e2b.1570201763.git.gitgitgadget@gmail.com/raw

	[...]
	Date:   Fri, 04 Oct 2019 08:09:25 -0700 (PDT)
	[...]
	X-Google-Original-Date: Fri, 04 Oct 2019 15:09:10 GMT
	[...]

I am fairly certain that the latter is the actual Date: line sent to GMail, and GMail just decides that it will not respect it.

Once https://github.com/gitgitgadget/gitgitgadget/pull/125 makes it into GitGitGadget (adding the /preview command that allows to send patch series to the PR owner as a test), it should be easier to start investigating further.

Unless anybody here knows why GMail rejects the header. Maybe it is the GMT?

Ciao,
Dscho

Am 06.10.19 um 02:02 schrieb Junio C Hamano:

Thank you, that summarizes my thoughts very well.

Thanks, but I disagree with the course of action: I think we should do both sides at the same time. (And if we aren't ready to do the decoding side now, then we should delay the encoding side).

Consider someone is looking at the call site without knowing the detailed meaning of the return value ("What the heck is this -1 business?"), it is a matter to look at the function being called to know what it is. With another function that does the encoding, it is one additional hop. That is my reason for saying "more obfuscating than necessary".

That the helper function would reduce some small code duplication does not outweigh the obfuscation in my book. That's just MHO, of course.

-- Hannes

Hi Junio,

FWIW this is now merged.

Ciao,
Dscho

If the submitting program said "Fri, 04 Oct 2019 15:09:10 +0000 (GMT)" instead of "Fri, 04 Oct 2019 15:09:10 GMT", that would match the format the MTA produced itself, I guess. I am kind-of surprised if the problem is the use of the obs-zone format (RFC 2822 page 31), but anything is possible with GMail X-<.

How does send-email write that date header? Matching that would be probably the most appropriate, if possible, given that GGG was written for send-email refugees, I guess ;-)

Here is what its format_2822_time sub does, so +0000 without any textual zone name, it is.

	return sprintf("%s, %2d %s %d %02d:%02d:%02d %s%02d%02d",
		       qw(Sun Mon Tue Wed Thu Fri Sat)[$localtm[6]],
		       $localtm[3],
		       qw(Jan Feb Mar Apr May Jun
			  Jul Aug Sep Oct Nov Dec)[$localtm[4]],
		       $localtm[5]+1900,
		       $localtm[2],
		       $localtm[1],
		       $localtm[0],
		       ($offset >= 0) ? '+' : '-',
		       abs($offhour),
		       $offmin,
		       );

Nice.

I didn't quite understand this part, though.

The default creation factor is 60 (roughly speaking, it wants 60% of the lines to match between two patches, otherwise it considers the patches to be unrelated).

Would the updated creation factor used which is 95 (roughly speaking) want 95% of the lines to match between two patches?

That would make the matching logic even pickier and reject more paring, so I must be reading the statement wrong X-<.

Yeah, the obs-zone format did seem to be the problem. I just dug up the previous thread we had about this, where I confirmed that +0000 as the timezone worked just fine in my setup through GMail [*1*]. Note sure if the (GMT) would cause any problems, but I'd agree with avoiding it as you mention below to make sure GMail doesn't do anything funny with it.

*1*: https://public-inbox.org/git/[email protected]/

Hi Johannes,

Le 04/10/2019 à 17:09, Johannes Schindelin via GitGitGadget a écrit :

Perhaps this is a good candidate for a semantic patch?

Cheers,
Alban

Hi Junio,

No, I must have written the opposite of what I tried to say, is all. Sorry about the confusion.

Ciao,
Dscho

So, cfactor of 60 means at most 60% is allowed to differ and the two patches are still considered to be related, while 95 means only 5% needs to be common? That would make more sense to me.

Thanks.

Hi Hannes,

So you got what you wished for:
https://public-inbox.org/git/[email protected]

Care to review?

Thanks,
Dscho

Hi Junio,

Okay, I not only wrote the opposite of what I wanted to say, I also misremembered.

When range-diff tries to determine matching pairs of patches, it builds an (m+n)x(m+n) cost matrix, where m is the number of patches in the first commit range and n is the number of patches in the second one.

Why not m x n? Well, that's the obvious matrix, and that's what it starts with, essentially assigning the number of lines of the diff between the diffs as "cost".

But then git range-diff extends the cost matrix to allow for _all_ of the m patches to be considered deleted, and _all_ of the n patches to be added. As cost, it cannot use a "diff of diffs" because there is no second diff. So it uses the number of lines of the one diff it has, multiplied by the creation factor interpreted as a percentage.

The naive creation factor would be 100%, which is (almost) as if we assumed an empty diff for the missing diff. But that would make the range-diff too eager to dismiss rewrites, as experience obviously showed (not my experience, but Thomas Rast's, who came up with tbdiff after all): the diff of diffs includes a diff header, for example.

The interpretation I offered (although I inverted what I wanted to say) is similar in spirit to that metric (which is not actually a metric, I believe, because I expect it to violate the triangle inequality) is obviously inaccurate: the number of lines of the diff of diffs does not say anything about the number of matching lines, quite to the contrary, it correlates somewhat to the number of non-matching lines.

So a better interpretation would have been:

The default creation factor is 60 (roughly speaking, it wants at most 60% of the diffs' lines to differ, otherwise it considers them not to be a match.

This is still inaccurate, but at least it gets the idea of the range-diff across.

Of course, I will never be able to amend the commit message in GitGitGadget anyway, as I have merged that PR already.

Ciao,
Dscho

Am 08.10.19 um 14:04 schrieb Johannes Schindelin:

After having seen the result I do not wish for it anymore. (Not that I had "wished" for it in the first place...) It does not make the result any more readable than the original.

I do wish you had rejected Junio's suggestion to introduce index_pos_to_insert_pos(). It doesn't make the code a lot more readable, either, in my eyes.

-- Hannes

Hi Dscho,

Medium term, is this something that could go in the algorithms section of the range-diff man page, especially if the upstream commit message is already in place.

#leftoverdocs ?

Philip

Hi Philip,

Sure. How about giving it a try while our memory is still fresh? You would help me immensely if you could take that task off of my plate...

Thanks,
Dscho

Hi Dscho,

My sneak think was to introduce the new hash tag for useful explanations from the list that could be copied into the man/guide pages, especially for the project pages.

At the moment I'm trying to get the >4Gb on Window' series sorted, which though working has been a load of bear traps for testing and configs and VS compiling, etc. But yes, it is on the back-list.

Philip

Hi Thomas & Junio,

Urg. Sorry for dropping the ball on that one.

I changed the GitGitGadget code accordingly (here is the commit:
https://github.com/gitgitgadget/gitgitgadget/commit/6117c61be5c2d13b46229ed12b2a9f63ef80f3c9).

Thanks,
Dscho