From ab961513c4a6bb1d65636829aa962593cfd934e9 Mon Sep 17 00:00:00 2001 From: "brian m. carlson" Date: Tue, 11 Jan 2022 02:15:06 +0000 Subject: [PATCH 01/17] t0027: add tests for eol without text in .gitattributes Right now, it isn't clear what the behavior is when the eol attribute is set in .gitattributes but the text attribute is not. Let's add some tests to document this behavior in our code, which happens to be that the behavior is as if we set the text attribute implicitly. This will make sure we don't accidentally change the behavior, which somebody is probably relying on, and serve as documentation to developers. Signed-off-by: brian m. carlson Signed-off-by: Junio C Hamano --- t/t0027-auto-crlf.sh | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/t/t0027-auto-crlf.sh b/t/t0027-auto-crlf.sh index 4a5c5c602cfa8f..c5f7ac63b0ab98 100755 --- a/t/t0027-auto-crlf.sh +++ b/t/t0027-auto-crlf.sh @@ -597,6 +597,12 @@ do # auto: core.autocrlf=false and core.eol unset(or native) uses native eol checkout_files auto "$id" "" false "" $NL CRLF CRLF_mix_LF LF_mix_CR LF_nul checkout_files auto "$id" "" false native $NL CRLF CRLF_mix_LF LF_mix_CR LF_nul + # core.autocrlf false, .gitattributes sets eol + checkout_files "" "$id" "lf" false "" LF CRLF CRLF_mix_LF LF_mix_CR LF_nul + checkout_files "" "$id" "crlf" false "" CRLF CRLF CRLF CRLF_mix_CR CRLF_nul + # core.autocrlf true, .gitattributes sets eol + checkout_files "" "$id" "lf" true "" LF CRLF CRLF_mix_LF LF_mix_CR LF_nul + checkout_files "" "$id" "crlf" true "" CRLF CRLF CRLF CRLF_mix_CR CRLF_nul done # The rest of the tests are unique; do the usual linting. From 8c591dbfcef9b4d40cfae7f675a2c99a0e925157 Mon Sep 17 00:00:00 2001 From: "brian m. carlson" Date: Tue, 11 Jan 2022 02:15:07 +0000 Subject: [PATCH 02/17] docs: correct documentation about eol attribute The documentation for the eol attribute states that it is "effectively setting the text attribute". However, this implies that it forces the text attribute to always be set, which has not been the case since 6523728499 ("convert: unify the "auto" handling of CRLF", 2016-06-28). Let's avoid confusing users (and the present author when trying to describe Git's behavior to others) by clearly documenting in which cases the "eol" attribute has effect. Specifically, the attribute always has an effect unless the file is explicitly set as -text, or the file is set as text=auto and the file is detected as binary. Signed-off-by: brian m. carlson Signed-off-by: Junio C Hamano --- Documentation/gitattributes.txt | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt index 83fd4e19a410ae..60984a46824525 100644 --- a/Documentation/gitattributes.txt +++ b/Documentation/gitattributes.txt @@ -160,11 +160,12 @@ unspecified. ^^^^^ This attribute sets a specific line-ending style to be used in the -working directory. It enables end-of-line conversion without any -content checks, effectively setting the `text` attribute. Note that -setting this attribute on paths which are in the index with CRLF line -endings may make the paths to be considered dirty. Adding the path to -the index again will normalize the line endings in the index. +working directory. This attribute has effect only if the `text` +attribute is set or unspecified, or if it is set to `auto` and the file +is detected as text. Note that setting this attribute on paths which +are in the index with CRLF line endings may make the paths to be +considered dirty. Adding the path to the index again will normalize the +line endings in the index. Set to string value "crlf":: From 05cd988dce58ba4cd46d4b3e79deacd86b34dc52 Mon Sep 17 00:00:00 2001 From: "brian m. carlson" Date: Mon, 17 Jan 2022 21:56:16 +0000 Subject: [PATCH 03/17] wrapper: add a helper to generate numbers from a CSPRNG There are many situations in which having access to a cryptographically secure pseudorandom number generator (CSPRNG) is helpful. In the future, we'll encounter one of these when dealing with temporary files. To make this possible, let's add a function which reads from a system CSPRNG and returns some bytes. We know that all systems will have such an interface. A CSPRNG is required for a secure TLS or SSH implementation and a Git implementation which provided neither would be of little practical use. In addition, POSIX is set to standardize getentropy(2) in the next version, so in the (potentially distant) future we can rely on that. For systems which lack one of the other interfaces, we provide the ability to use OpenSSL's CSPRNG. OpenSSL is highly portable and functions on practically every known OS, and we know it will have access to some source of cryptographically secure randomness. We also provide support for the arc4random in libbsd for folks who would prefer to use that. Because this is a security sensitive interface, we take some precautions. We either succeed by filling the buffer completely as we requested, or we fail. We don't return partial data because the caller will almost never find that to be a useful behavior. Specify a makefile knob which users can use to specify one or more suitable CSPRNGs, and turn the multiple string options into a set of defines, since we cannot match on strings in the preprocessor. We allow multiple options to make the job of handling this in autoconf easier. The order of options is important here. On systems with arc4random, which is most of the BSDs, we use that, since, except on MirBSD and macOS, it uses ChaCha20, which is extremely fast, and sits entirely in userspace, avoiding a system call. We then prefer getrandom over getentropy, because the former has been available longer on Linux, and then OpenSSL. Finally, if none of those are available, we use /dev/urandom, because most Unix-like operating systems provide that API. We prefer options that don't involve device files when possible because those work in some restricted environments where device files may not be available. Set the configuration variables appropriately for Linux and the BSDs, including macOS, as well as Windows and NonStop. We specifically only consider versions which receive publicly available security support here. For the same reason, we don't specify getrandom(2) on Linux, because CentOS 7 doesn't support it in glibc (although its kernel does) and we don't want to resort to making syscalls. Finally, add a test helper to allow this to be tested by hand and in tests. We don't add any tests, since invoking the CSPRNG is not likely to produce interesting, reproducible results. Signed-off-by: brian m. carlson Signed-off-by: Junio C Hamano --- Makefile | 34 +++++++++++++++ compat/winansi.c | 6 +++ config.mak.uname | 8 ++++ contrib/buildsystems/CMakeLists.txt | 2 +- git-compat-util.h | 19 +++++++++ t/helper/test-csprng.c | 29 +++++++++++++ t/helper/test-tool.c | 1 + t/helper/test-tool.h | 1 + wrapper.c | 66 +++++++++++++++++++++++++++++ 9 files changed, 165 insertions(+), 1 deletion(-) create mode 100644 t/helper/test-csprng.c diff --git a/Makefile b/Makefile index 75ed168adbc703..198b759e760f64 100644 --- a/Makefile +++ b/Makefile @@ -234,6 +234,14 @@ all:: # Define NO_TRUSTABLE_FILEMODE if your filesystem may claim to support # the executable mode bit, but doesn't really do so. # +# Define CSPRNG_METHOD to "arc4random" if your system has arc4random and +# arc4random_buf, "libbsd" if your system has those functions from libbsd, +# "getrandom" if your system has getrandom, "getentropy" if your system has +# getentropy, "rtlgenrandom" for RtlGenRandom (Windows only), or "openssl" if +# you'd want to use the OpenSSL CSPRNG. You may set multiple options with +# spaces, in which case a suitable option will be chosen. If unset or set to +# anything else, defaults to using "/dev/urandom". +# # Define NEEDS_MODE_TRANSLATION if your OS strays from the typical file type # bits in mode values (e.g. z/OS defines I_SFMT to 0xFF000000 as opposed to the # usual 0xF000). @@ -693,6 +701,7 @@ TEST_BUILTINS_OBJS += test-bloom.o TEST_BUILTINS_OBJS += test-chmtime.o TEST_BUILTINS_OBJS += test-config.o TEST_BUILTINS_OBJS += test-crontab.o +TEST_BUILTINS_OBJS += test-csprng.o TEST_BUILTINS_OBJS += test-ctype.o TEST_BUILTINS_OBJS += test-date.o TEST_BUILTINS_OBJS += test-delta.o @@ -1908,6 +1917,31 @@ ifdef HAVE_GETDELIM BASIC_CFLAGS += -DHAVE_GETDELIM endif +ifneq ($(findstring arc4random,$(CSPRNG_METHOD)),) + BASIC_CFLAGS += -DHAVE_ARC4RANDOM +endif + +ifneq ($(findstring libbsd,$(CSPRNG_METHOD)),) + BASIC_CFLAGS += -DHAVE_ARC4RANDOM_LIBBSD + EXTLIBS += -lbsd +endif + +ifneq ($(findstring getrandom,$(CSPRNG_METHOD)),) + BASIC_CFLAGS += -DHAVE_GETRANDOM +endif + +ifneq ($(findstring getentropy,$(CSPRNG_METHOD)),) + BASIC_CFLAGS += -DHAVE_GETENTROPY +endif + +ifneq ($(findstring rtlgenrandom,$(CSPRNG_METHOD)),) + BASIC_CFLAGS += -DHAVE_RTLGENRANDOM +endif + +ifneq ($(findstring openssl,$(CSPRNG_METHOD)),) + BASIC_CFLAGS += -DHAVE_OPENSSL_CSPRNG +endif + ifneq ($(PROCFS_EXECUTABLE_PATH),) procfs_executable_path_SQ = $(subst ','\'',$(PROCFS_EXECUTABLE_PATH)) BASIC_CFLAGS += '-DPROCFS_EXECUTABLE_PATH="$(procfs_executable_path_SQ)"' diff --git a/compat/winansi.c b/compat/winansi.c index c27b20a79d91cf..0e5a9cc82e5f31 100644 --- a/compat/winansi.c +++ b/compat/winansi.c @@ -3,6 +3,12 @@ */ #undef NOGDI + +/* + * Including the appropriate header file for RtlGenRandom causes MSVC to see a + * redefinition of types in an incompatible way when including headers below. + */ +#undef HAVE_RTLGENRANDOM #include "../git-compat-util.h" #include #include diff --git a/config.mak.uname b/config.mak.uname index a3a779327f8d5f..ff0710a612eb48 100644 --- a/config.mak.uname +++ b/config.mak.uname @@ -141,6 +141,7 @@ ifeq ($(uname_S),Darwin) HAVE_BSD_SYSCTL = YesPlease FREAD_READS_DIRECTORIES = UnfortunatelyYes HAVE_NS_GET_EXECUTABLE_PATH = YesPlease + CSPRNG_METHOD = arc4random # Workaround for `gettext` being keg-only and not even being linked via # `brew link --force gettext`, should be obsolete as of @@ -256,6 +257,7 @@ ifeq ($(uname_S),FreeBSD) HAVE_PATHS_H = YesPlease HAVE_BSD_SYSCTL = YesPlease HAVE_BSD_KERN_PROC_SYSCTL = YesPlease + CSPRNG_METHOD = arc4random PAGER_ENV = LESS=FRX LV=-c MORE=FRX FREAD_READS_DIRECTORIES = UnfortunatelyYes FILENO_IS_A_MACRO = UnfortunatelyYes @@ -274,6 +276,7 @@ ifeq ($(uname_S),OpenBSD) HAVE_PATHS_H = YesPlease HAVE_BSD_SYSCTL = YesPlease HAVE_BSD_KERN_PROC_SYSCTL = YesPlease + CSPRNG_METHOD = arc4random PROCFS_EXECUTABLE_PATH = /proc/curproc/file FREAD_READS_DIRECTORIES = UnfortunatelyYes FILENO_IS_A_MACRO = UnfortunatelyYes @@ -285,6 +288,7 @@ ifeq ($(uname_S),MirBSD) NEEDS_LIBICONV = YesPlease HAVE_PATHS_H = YesPlease HAVE_BSD_SYSCTL = YesPlease + CSPRNG_METHOD = arc4random endif ifeq ($(uname_S),NetBSD) ifeq ($(shell expr "$(uname_R)" : '[01]\.'),2) @@ -296,6 +300,7 @@ ifeq ($(uname_S),NetBSD) HAVE_PATHS_H = YesPlease HAVE_BSD_SYSCTL = YesPlease HAVE_BSD_KERN_PROC_SYSCTL = YesPlease + CSPRNG_METHOD = arc4random PROCFS_EXECUTABLE_PATH = /proc/curproc/exe endif ifeq ($(uname_S),AIX) @@ -425,6 +430,7 @@ ifeq ($(uname_S),Windows) NO_STRTOUMAX = YesPlease NO_MKDTEMP = YesPlease NO_INTTYPES_H = YesPlease + CSPRNG_METHOD = rtlgenrandom # VS2015 with UCRT claims that snprintf and friends are C99 compliant, # so we don't need this: # @@ -593,6 +599,7 @@ ifeq ($(uname_S),NONSTOP_KERNEL) NO_MMAP = YesPlease NO_POLL = YesPlease NO_INTPTR_T = UnfortunatelyYes + CSPRNG_METHOD = openssl SANE_TOOL_PATH = /usr/coreutils/bin:/usr/local/bin SHELL_PATH = /usr/coreutils/bin/bash endif @@ -628,6 +635,7 @@ ifeq ($(uname_S),MINGW) NO_POSIX_GOODIES = UnfortunatelyYes DEFAULT_HELP_FORMAT = html HAVE_PLATFORM_PROCINFO = YesPlease + CSPRNG_METHOD = rtlgenrandom BASIC_LDFLAGS += -municode COMPAT_CFLAGS += -DNOGDI -Icompat -Icompat/win32 COMPAT_CFLAGS += -DSTRIP_EXTENSION=\".exe\" diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt index 5100f56bb37a41..e44232f85d3660 100644 --- a/contrib/buildsystems/CMakeLists.txt +++ b/contrib/buildsystems/CMakeLists.txt @@ -260,7 +260,7 @@ if(CMAKE_SYSTEM_NAME STREQUAL "Windows") _CONSOLE DETECT_MSYS_TTY STRIP_EXTENSION=".exe" NO_SYMLINK_HEAD UNRELIABLE_FSTAT NOGDI OBJECT_CREATION_MODE=1 __USE_MINGW_ANSI_STDIO=0 USE_NED_ALLOCATOR OVERRIDE_STRDUP MMAP_PREVENTS_DELETE USE_WIN32_MMAP - UNICODE _UNICODE HAVE_WPGMPTR ENSURE_MSYSTEM_IS_SET) + UNICODE _UNICODE HAVE_WPGMPTR ENSURE_MSYSTEM_IS_SET HAVE_RTLGENRANDOM) list(APPEND compat_SOURCES compat/mingw.c compat/winansi.c compat/win32/path-utils.c compat/win32/pthread.c compat/win32mmap.c compat/win32/syslog.c compat/win32/trace2_win32_process_info.c compat/win32/dirent.c diff --git a/git-compat-util.h b/git-compat-util.h index 5fa54a7afe4bf9..50597c76be2d49 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -188,6 +188,12 @@ #endif #include #define GIT_WINDOWS_NATIVE +#ifdef HAVE_RTLGENRANDOM +/* This is required to get access to RtlGenRandom. */ +#define SystemFunction036 NTAPI SystemFunction036 +#include +#undef SystemFunction036 +#endif #endif #include @@ -258,6 +264,12 @@ #else #include #endif +#ifdef HAVE_ARC4RANDOM_LIBBSD +#include +#endif +#ifdef HAVE_GETRANDOM +#include +#endif #ifdef NO_INTPTR_T /* * On I16LP32, ILP32 and LP64 "long" is the safe bet, however @@ -1421,4 +1433,11 @@ static inline void *container_of_or_null_offset(void *ptr, size_t offset) void sleep_millisec(int millisec); +/* + * Generate len bytes from the system cryptographically secure PRNG. + * Returns 0 on success and -1 on error, setting errno. The inability to + * satisfy the full request is an error. + */ +int csprng_bytes(void *buf, size_t len); + #endif diff --git a/t/helper/test-csprng.c b/t/helper/test-csprng.c new file mode 100644 index 00000000000000..65d14973c520ea --- /dev/null +++ b/t/helper/test-csprng.c @@ -0,0 +1,29 @@ +#include "test-tool.h" +#include "git-compat-util.h" + + +int cmd__csprng(int argc, const char **argv) +{ + unsigned long count; + unsigned char buf[1024]; + + if (argc > 2) { + fprintf(stderr, "usage: %s []\n", argv[0]); + return 2; + } + + count = (argc == 2) ? strtoul(argv[1], NULL, 0) : -1L; + + while (count) { + unsigned long chunk = count < sizeof(buf) ? count : sizeof(buf); + if (csprng_bytes(buf, chunk) < 0) { + perror("failed to read"); + return 5; + } + if (fwrite(buf, chunk, 1, stdout) != chunk) + return 1; + count -= chunk; + } + + return 0; +} diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c index 338a57b104d689..e6ec69cf326a3b 100644 --- a/t/helper/test-tool.c +++ b/t/helper/test-tool.c @@ -20,6 +20,7 @@ static struct test_cmd cmds[] = { { "chmtime", cmd__chmtime }, { "config", cmd__config }, { "crontab", cmd__crontab }, + { "csprng", cmd__csprng }, { "ctype", cmd__ctype }, { "date", cmd__date }, { "delta", cmd__delta }, diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h index 48cee1f4a2d985..20756eefddac83 100644 --- a/t/helper/test-tool.h +++ b/t/helper/test-tool.h @@ -10,6 +10,7 @@ int cmd__bloom(int argc, const char **argv); int cmd__chmtime(int argc, const char **argv); int cmd__config(int argc, const char **argv); int cmd__crontab(int argc, const char **argv); +int cmd__csprng(int argc, const char **argv); int cmd__ctype(int argc, const char **argv); int cmd__date(int argc, const char **argv); int cmd__delta(int argc, const char **argv); diff --git a/wrapper.c b/wrapper.c index 36e12119d76556..105235670328b3 100644 --- a/wrapper.c +++ b/wrapper.c @@ -702,3 +702,69 @@ int open_nofollow(const char *path, int flags) return open(path, flags); #endif } + +int csprng_bytes(void *buf, size_t len) +{ +#if defined(HAVE_ARC4RANDOM) || defined(HAVE_ARC4RANDOM_LIBBSD) + /* This function never returns an error. */ + arc4random_buf(buf, len); + return 0; +#elif defined(HAVE_GETRANDOM) + ssize_t res; + char *p = buf; + while (len) { + res = getrandom(p, len, 0); + if (res < 0) + return -1; + len -= res; + p += res; + } + return 0; +#elif defined(HAVE_GETENTROPY) + int res; + char *p = buf; + while (len) { + /* getentropy has a maximum size of 256 bytes. */ + size_t chunk = len < 256 ? len : 256; + res = getentropy(p, chunk); + if (res < 0) + return -1; + len -= chunk; + p += chunk; + } + return 0; +#elif defined(HAVE_RTLGENRANDOM) + if (!RtlGenRandom(buf, len)) + return -1; + return 0; +#elif defined(HAVE_OPENSSL_CSPRNG) + int res = RAND_bytes(buf, len); + if (res == 1) + return 0; + if (res == -1) + errno = ENOTSUP; + else + errno = EIO; + return -1; +#else + ssize_t res; + char *p = buf; + int fd, err; + fd = open("/dev/urandom", O_RDONLY); + if (fd < 0) + return -1; + while (len) { + res = xread(fd, p, len); + if (res < 0) { + err = errno; + close(fd); + errno = err; + return -1; + } + len -= res; + p += res; + } + close(fd); + return 0; +#endif +} From 47efda967cfd4ef9d39de149e1e3654b051e5d19 Mon Sep 17 00:00:00 2001 From: "brian m. carlson" Date: Mon, 17 Jan 2022 21:56:17 +0000 Subject: [PATCH 04/17] wrapper: use a CSPRNG to generate random file names The current way we generate random file names is by taking the seconds and microseconds, plus the PID, and mixing them together, then encoding them. If this fails, we increment the value by 7777, and try again up to TMP_MAX times. Unfortunately, this is not the best idea from a security perspective. If we're writing into TMPDIR, an attacker can guess these values easily and prevent us from creating any temporary files at all by creating them all first. Even though we set TMP_MAX to 16384, this may be achievable in some contexts, even if unlikely to occur in practice. Fortunately, we can simply solve this by using the system cryptographically secure pseudorandom number generator (CSPRNG) to generate a random 64-bit value, and use that as before. Note that there is still a small bias here, but because a six-character sequence chosen out of 62 characters provides about 36 bits of entropy, the bias here is less than 2^-28, which is acceptable, especially considering we'll retry several times. Note that the use of a CSPRNG in generating temporary file names is also used in many libcs. glibc recently changed from an approach similar to ours to using a CSPRNG, and FreeBSD and OpenBSD also use a CSPRNG in this case. Even if the likelihood of an attack is low, we should still be at least as responsible in creating temporary files as libc is. Signed-off-by: brian m. carlson Signed-off-by: Junio C Hamano --- wrapper.c | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-) diff --git a/wrapper.c b/wrapper.c index 105235670328b3..3258cdb171f518 100644 --- a/wrapper.c +++ b/wrapper.c @@ -463,8 +463,6 @@ int git_mkstemps_mode(char *pattern, int suffix_len, int mode) static const int num_letters = ARRAY_SIZE(letters) - 1; static const char x_pattern[] = "XXXXXX"; static const int num_x = ARRAY_SIZE(x_pattern) - 1; - uint64_t value; - struct timeval tv; char *filename_template; size_t len; int fd, count; @@ -485,12 +483,13 @@ int git_mkstemps_mode(char *pattern, int suffix_len, int mode) * Replace pattern's XXXXXX characters with randomness. * Try TMP_MAX different filenames. */ - gettimeofday(&tv, NULL); - value = ((uint64_t)tv.tv_usec << 16) ^ tv.tv_sec ^ getpid(); filename_template = &pattern[len - num_x - suffix_len]; for (count = 0; count < TMP_MAX; ++count) { - uint64_t v = value; int i; + uint64_t v; + if (csprng_bytes(&v, sizeof(v)) < 0) + return error_errno("unable to get random bytes for temporary file"); + /* Fill in the random bits. */ for (i = 0; i < num_x; i++) { filename_template[i] = letters[v % num_letters]; @@ -506,12 +505,6 @@ int git_mkstemps_mode(char *pattern, int suffix_len, int mode) */ if (errno != EEXIST) break; - /* - * This is a random value. It is only necessary that - * the next TMP_MAX values generated by adding 7777 to - * VALUE are different with (module 2^32). - */ - value += 7777; } /* We return the null string if we can't find a unique file name. */ pattern[0] = '\0'; From 518e15db742ee58e73d486251c67218c6a0fa4f5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Scharfe?= Date: Thu, 20 Jan 2022 11:30:15 +0100 Subject: [PATCH 05/17] parse-options: document bracketing of argh MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: René Scharfe Signed-off-by: Junio C Hamano --- parse-options.h | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/parse-options.h b/parse-options.h index 275fb440818d53..eebab6c7d71f55 100644 --- a/parse-options.h +++ b/parse-options.h @@ -85,6 +85,11 @@ typedef enum parse_opt_result parse_opt_ll_cb(struct parse_opt_ctx_t *ctx, * token to explain the kind of argument this option wants. Does not * begin in capital letter, and does not end with a full stop. * Should be wrapped by N_() for translation. + * Is automatically enclosed in brackets when printed, unless it + * contains any of the following characters: ()<>[]| + * E.g. "name" is shown as "" to indicate that a name value + * needs to be supplied, not the literal string "name", but + * "," and "(this|that)" are printed verbatim. * * `help`:: * the short help associated to what the option does. From 09444e74e3acf4e726db60a5595eb7d3ebca8450 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Wed, 26 Jan 2022 15:37:00 +0100 Subject: [PATCH 06/17] sequencer: don't use die_errno() on refs_resolve_ref_unsafe() failure MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Change code that was faithfully migrated to the new "resolve_errno" API in ed90f04155d (refs API: make resolve_ref_unsafe() not set errno, 2021-10-16) to stop caring about the errno at all. When we fail to resolve "HEAD" after the sequencer runs it doesn't really help to say what the "errno" value is, since the fake backend errno may or may not reflect anything real about the state of the ".git/HEAD". With the upcoming reftable backend this fakery will become even more pronounced. So let's just die() instead of die_errno() here. This will also help simplify the refs_resolve_ref_unsafe() API. This was the only user of it that wasn't ignoring the "failure_errno" output parameter. Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- sequencer.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/sequencer.c b/sequencer.c index 6abd72160ccd46..03cdf548d728aa 100644 --- a/sequencer.c +++ b/sequencer.c @@ -1281,7 +1281,7 @@ void print_commit_summary(struct repository *r, struct strbuf author_ident = STRBUF_INIT; struct strbuf committer_ident = STRBUF_INIT; struct ref_store *refs; - int resolve_errno; + int ignore_errno; commit = lookup_commit(r, oid); if (!commit) @@ -1333,11 +1333,9 @@ void print_commit_summary(struct repository *r, refs = get_main_ref_store(the_repository); head = refs_resolve_ref_unsafe(refs, "HEAD", 0, NULL, NULL, - &resolve_errno); - if (!head) { - errno = resolve_errno; - die_errno(_("unable to resolve HEAD after creating commit")); - } + &ignore_errno); + if (!head) + die(_("unable to resolve HEAD after creating commit")); if (!strcmp(head, "HEAD")) head = _("detached HEAD"); else From ce14de03db6e1a29ad92c747542f1eb4357f25d6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Wed, 26 Jan 2022 15:37:01 +0100 Subject: [PATCH 07/17] refs API: remove "failure_errno" from refs_resolve_ref_unsafe() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Remove the now-unused "failure_errno" parameter from the refs_resolve_ref_unsafe() signature. In my recent 96f6623ada0 (Merge branch 'ab/refs-errno-cleanup', 2021-11-29) series we made all of its callers explicitly request the errno via an output parameter. As that series shows all but one caller ended up passing in a boilerplate "ignore_errno", since they only cared about whether the return value was NULL or not, i.e. if the ref could be resolved. There was one small issue with that series fixed with a follow-up in 31e39123695 (Merge branch 'ab/refs-errno-cleanup', 2022-01-14) a small bug in that series was fixed. After those two there was one caller left in sequencer.c that used the "failure_errno', but as of the preceding commit it uses a boilerplate "ignore_errno" instead. This leaves the public refs API without any use of "failure_errno" at all. We could still do with a bit of cleanup and generalization between refs.c and refs/files-backend.c before the "reftable" integration lands, but that's all internal to the reference code itself. So let's remove this output parameter. Not only isn't it used now, but it's unlikely that we'll want it again in the future. We'd like to slowly move the refs API to a more file-backend independent way of communicating error codes, having it use a "failure_errno" was only the first step in that direction. If this or any other function needs to communicate what specifically is wrong with the requested "refname" it'll be better to have the function set some output enum of well-defined error states than piggy-backend on "errno". Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- refs.c | 51 +++++++++++++-------------------------- refs.h | 7 +----- refs/files-backend.c | 31 +++++++----------------- remote.c | 3 +-- sequencer.c | 4 +-- t/helper/test-ref-store.c | 3 +-- worktree.c | 11 +++------ 7 files changed, 33 insertions(+), 77 deletions(-) diff --git a/refs.c b/refs.c index addb26293b4ffc..7017ae598041bb 100644 --- a/refs.c +++ b/refs.c @@ -269,10 +269,9 @@ char *refs_resolve_refdup(struct ref_store *refs, struct object_id *oid, int *flags) { const char *result; - int ignore_errno; result = refs_resolve_ref_unsafe(refs, refname, resolve_flags, - oid, flags, &ignore_errno); + oid, flags); return xstrdup_or_null(result); } @@ -294,11 +293,10 @@ struct ref_filter { int read_ref_full(const char *refname, int resolve_flags, struct object_id *oid, int *flags) { - int ignore_errno; struct ref_store *refs = get_main_ref_store(the_repository); if (refs_resolve_ref_unsafe(refs, refname, resolve_flags, - oid, flags, &ignore_errno)) + oid, flags)) return 0; return -1; } @@ -310,9 +308,8 @@ int read_ref(const char *refname, struct object_id *oid) int refs_ref_exists(struct ref_store *refs, const char *refname) { - int ignore_errno; return !!refs_resolve_ref_unsafe(refs, refname, RESOLVE_REF_READING, - NULL, NULL, &ignore_errno); + NULL, NULL); } int ref_exists(const char *refname) @@ -656,15 +653,13 @@ int expand_ref(struct repository *repo, const char *str, int len, struct object_id *this_result; int flag; struct ref_store *refs = get_main_ref_store(repo); - int ignore_errno; this_result = refs_found ? &oid_from_ref : oid; strbuf_reset(&fullref); strbuf_addf(&fullref, *p, len, str); r = refs_resolve_ref_unsafe(refs, fullref.buf, RESOLVE_REF_READING, - this_result, &flag, - &ignore_errno); + this_result, &flag); if (r) { if (!refs_found++) *ref = xstrdup(r); @@ -693,14 +688,12 @@ int repo_dwim_log(struct repository *r, const char *str, int len, for (p = ref_rev_parse_rules; *p; p++) { struct object_id hash; const char *ref, *it; - int ignore_errno; strbuf_reset(&path); strbuf_addf(&path, *p, len, str); ref = refs_resolve_ref_unsafe(refs, path.buf, RESOLVE_REF_READING, - oid ? &hash : NULL, NULL, - &ignore_errno); + oid ? &hash : NULL, NULL); if (!ref) continue; if (refs_reflog_exists(refs, path.buf)) @@ -1390,10 +1383,9 @@ int refs_head_ref(struct ref_store *refs, each_ref_fn fn, void *cb_data) { struct object_id oid; int flag; - int ignore_errno; if (refs_resolve_ref_unsafe(refs, "HEAD", RESOLVE_REF_READING, - &oid, &flag, &ignore_errno)) + &oid, &flag)) return fn("HEAD", &oid, flag, cb_data); return 0; @@ -1682,15 +1674,13 @@ const char *refs_resolve_ref_unsafe(struct ref_store *refs, const char *refname, int resolve_flags, struct object_id *oid, - int *flags, int *failure_errno) + int *flags) { static struct strbuf sb_refname = STRBUF_INIT; struct object_id unused_oid; int unused_flags; int symref_count; - assert(failure_errno); - if (!oid) oid = &unused_oid; if (!flags) @@ -1700,10 +1690,8 @@ const char *refs_resolve_ref_unsafe(struct ref_store *refs, if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) { if (!(resolve_flags & RESOLVE_REF_ALLOW_BAD_NAME) || - !refname_is_safe(refname)) { - *failure_errno = EINVAL; + !refname_is_safe(refname)) return NULL; - } /* * dwim_ref() uses REF_ISBROKEN to distinguish between @@ -1718,9 +1706,10 @@ const char *refs_resolve_ref_unsafe(struct ref_store *refs, for (symref_count = 0; symref_count < SYMREF_MAXDEPTH; symref_count++) { unsigned int read_flags = 0; + int failure_errno; if (refs_read_raw_ref(refs, refname, oid, &sb_refname, - &read_flags, failure_errno)) { + &read_flags, &failure_errno)) { *flags |= read_flags; /* In reading mode, refs must eventually resolve */ @@ -1732,9 +1721,9 @@ const char *refs_resolve_ref_unsafe(struct ref_store *refs, * may show errors besides ENOENT if there are * similarly-named refs. */ - if (*failure_errno != ENOENT && - *failure_errno != EISDIR && - *failure_errno != ENOTDIR) + if (failure_errno != ENOENT && + failure_errno != EISDIR && + failure_errno != ENOTDIR) return NULL; oidclr(oid); @@ -1760,16 +1749,13 @@ const char *refs_resolve_ref_unsafe(struct ref_store *refs, } if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) { if (!(resolve_flags & RESOLVE_REF_ALLOW_BAD_NAME) || - !refname_is_safe(refname)) { - *failure_errno = EINVAL; + !refname_is_safe(refname)) return NULL; - } *flags |= REF_ISBROKEN | REF_BAD_NAME; } } - *failure_errno = ELOOP; return NULL; } @@ -1784,10 +1770,8 @@ int refs_init_db(struct strbuf *err) const char *resolve_ref_unsafe(const char *refname, int resolve_flags, struct object_id *oid, int *flags) { - int ignore_errno; - return refs_resolve_ref_unsafe(get_main_ref_store(the_repository), refname, - resolve_flags, oid, flags, &ignore_errno); + resolve_flags, oid, flags); } int resolve_gitlink_ref(const char *submodule, const char *refname, @@ -1795,15 +1779,14 @@ int resolve_gitlink_ref(const char *submodule, const char *refname, { struct ref_store *refs; int flags; - int ignore_errno; refs = get_submodule_ref_store(submodule); if (!refs) return -1; - if (!refs_resolve_ref_unsafe(refs, refname, 0, oid, &flags, - &ignore_errno) || is_null_oid(oid)) + if (!refs_resolve_ref_unsafe(refs, refname, 0, oid, &flags) || + is_null_oid(oid)) return -1; return 0; } diff --git a/refs.h b/refs.h index 8f91a7f9ff27ec..cd2d0c1ac09001 100644 --- a/refs.h +++ b/refs.h @@ -58,11 +58,6 @@ struct worktree; * resolved. The function returns NULL for such ref names. * Caps and underscores refers to the special refs, such as HEAD, * FETCH_HEAD and friends, that all live outside of the refs/ directory. - * - * Callers should not inspect "errno" on failure, but rather pass in a - * "failure_errno" parameter, on failure the "errno" will indicate the - * type of failure encountered, but not necessarily one that came from - * a syscall. We might have faked it up. */ #define RESOLVE_REF_READING 0x01 #define RESOLVE_REF_NO_RECURSE 0x02 @@ -72,7 +67,7 @@ const char *refs_resolve_ref_unsafe(struct ref_store *refs, const char *refname, int resolve_flags, struct object_id *oid, - int *flags, int *failure_errno); + int *flags); const char *resolve_ref_unsafe(const char *refname, int resolve_flags, struct object_id *oid, int *flags); diff --git a/refs/files-backend.c b/refs/files-backend.c index 43a3b882d7c50c..a40267b3ae9c35 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -277,11 +277,10 @@ static void loose_fill_ref_dir(struct ref_store *ref_store, create_dir_entry(dir->cache, refname.buf, refname.len)); } else { - int ignore_errno; if (!refs_resolve_ref_unsafe(&refs->base, refname.buf, RESOLVE_REF_READING, - &oid, &flag, &ignore_errno)) { + &oid, &flag)) { oidclr(&oid); flag |= REF_ISBROKEN; } else if (is_null_oid(&oid)) { @@ -1006,7 +1005,6 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs, { struct strbuf ref_file = STRBUF_INIT; struct ref_lock *lock; - int ignore_errno; files_assert_main_repository(refs, "lock_ref_oid_basic"); assert(err); @@ -1034,7 +1032,7 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs, } if (!refs_resolve_ref_unsafe(&refs->base, lock->ref_name, 0, - &lock->old_oid, NULL, &ignore_errno)) + &lock->old_oid, NULL)) oidclr(&lock->old_oid); goto out; @@ -1399,7 +1397,6 @@ static int files_copy_or_rename_ref(struct ref_store *ref_store, struct strbuf tmp_renamed_log = STRBUF_INIT; int log, ret; struct strbuf err = STRBUF_INIT; - int ignore_errno; files_reflog_path(refs, &sb_oldref, oldrefname); files_reflog_path(refs, &sb_newref, newrefname); @@ -1413,7 +1410,7 @@ static int files_copy_or_rename_ref(struct ref_store *ref_store, if (!refs_resolve_ref_unsafe(&refs->base, oldrefname, RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE, - &orig_oid, &flag, &ignore_errno)) { + &orig_oid, &flag)) { ret = error("refname %s not found", oldrefname); goto out; } @@ -1459,7 +1456,7 @@ static int files_copy_or_rename_ref(struct ref_store *ref_store, */ if (!copy && refs_resolve_ref_unsafe(&refs->base, newrefname, RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE, - NULL, NULL, &ignore_errno) && + NULL, NULL) && refs_delete_ref(&refs->base, NULL, newrefname, NULL, REF_NO_DEREF)) { if (errno == EISDIR) { @@ -1828,12 +1825,10 @@ static int commit_ref_update(struct files_ref_store *refs, */ int head_flag; const char *head_ref; - int ignore_errno; head_ref = refs_resolve_ref_unsafe(&refs->base, "HEAD", RESOLVE_REF_READING, - NULL, &head_flag, - &ignore_errno); + NULL, &head_flag); if (head_ref && (head_flag & REF_ISSYMREF) && !strcmp(head_ref, lock->ref_name)) { struct strbuf log_err = STRBUF_INIT; @@ -1877,12 +1872,10 @@ static void update_symref_reflog(struct files_ref_store *refs, { struct strbuf err = STRBUF_INIT; struct object_id new_oid; - int ignore_errno; if (logmsg && refs_resolve_ref_unsafe(&refs->base, target, - RESOLVE_REF_READING, &new_oid, NULL, - &ignore_errno) && + RESOLVE_REF_READING, &new_oid, NULL) && files_log_ref_write(refs, refname, &lock->old_oid, &new_oid, logmsg, 0, &err)) { error("%s", err.buf); @@ -2156,7 +2149,6 @@ static int files_reflog_iterator_advance(struct ref_iterator *ref_iterator) (struct files_reflog_iterator *)ref_iterator; struct dir_iterator *diter = iter->dir_iterator; int ok; - int ignore_errno; while ((ok = dir_iterator_advance(diter)) == ITER_OK) { int flags; @@ -2170,8 +2162,7 @@ static int files_reflog_iterator_advance(struct ref_iterator *ref_iterator) if (!refs_resolve_ref_unsafe(iter->ref_store, diter->relative_path, 0, - &iter->oid, &flags, - &ignore_errno)) { + &iter->oid, &flags)) { error("bad ref for %s", diter->path.buf); continue; } @@ -2515,11 +2506,9 @@ static int lock_ref_for_update(struct files_ref_store *refs, * the transaction, so we have to read it here * to record and possibly check old_oid: */ - int ignore_errno; if (!refs_resolve_ref_unsafe(&refs->base, referent.buf, 0, - &lock->old_oid, NULL, - &ignore_errno)) { + &lock->old_oid, NULL)) { if (update->flags & REF_HAVE_OLD) { strbuf_addf(err, "cannot lock ref '%s': " "error reading reference", @@ -3208,14 +3197,12 @@ static int files_reflog_expire(struct ref_store *ref_store, if ((expire_flags & EXPIRE_REFLOGS_UPDATE_REF) && !is_null_oid(&cb.last_kept_oid)) { - int ignore_errno; int type; const char *ref; ref = refs_resolve_ref_unsafe(&refs->base, refname, RESOLVE_REF_NO_RECURSE, - NULL, &type, - &ignore_errno); + NULL, &type); update = !!(ref && !(type & REF_ISSYMREF)); } diff --git a/remote.c b/remote.c index a6d8ec6c1ac72f..c97c626eaa8390 100644 --- a/remote.c +++ b/remote.c @@ -508,9 +508,8 @@ static void read_config(struct repository *repo) repo->remote_state->current_branch = NULL; if (startup_info->have_repository) { - int ignore_errno; const char *head_ref = refs_resolve_ref_unsafe( - get_main_ref_store(repo), "HEAD", 0, NULL, &flag, &ignore_errno); + get_main_ref_store(repo), "HEAD", 0, NULL, &flag); if (head_ref && (flag & REF_ISSYMREF) && skip_prefix(head_ref, "refs/heads/", &head_ref)) { repo->remote_state->current_branch = make_branch( diff --git a/sequencer.c b/sequencer.c index 03cdf548d728aa..d57b51ed555335 100644 --- a/sequencer.c +++ b/sequencer.c @@ -1281,7 +1281,6 @@ void print_commit_summary(struct repository *r, struct strbuf author_ident = STRBUF_INIT; struct strbuf committer_ident = STRBUF_INIT; struct ref_store *refs; - int ignore_errno; commit = lookup_commit(r, oid); if (!commit) @@ -1332,8 +1331,7 @@ void print_commit_summary(struct repository *r, diff_setup_done(&rev.diffopt); refs = get_main_ref_store(the_repository); - head = refs_resolve_ref_unsafe(refs, "HEAD", 0, NULL, NULL, - &ignore_errno); + head = refs_resolve_ref_unsafe(refs, "HEAD", 0, NULL, NULL); if (!head) die(_("unable to resolve HEAD after creating commit")); if (!strcmp(head, "HEAD")) diff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c index 3e4ddaee705576..9646d85fc84a9e 100644 --- a/t/helper/test-ref-store.c +++ b/t/helper/test-ref-store.c @@ -180,10 +180,9 @@ static int cmd_resolve_ref(struct ref_store *refs, const char **argv) int resolve_flags = arg_flags(*argv++, "resolve-flags", empty_flags); int flags; const char *ref; - int ignore_errno; ref = refs_resolve_ref_unsafe(refs, refname, resolve_flags, - &oid, &flags, &ignore_errno); + &oid, &flags); printf("%s %s 0x%x\n", oid_to_hex(&oid), ref ? ref : "(null)", flags); return ref ? 0 : 1; } diff --git a/worktree.c b/worktree.c index 6f598dcfcdf96f..e8f6f6ae6f46ec 100644 --- a/worktree.c +++ b/worktree.c @@ -28,13 +28,11 @@ static void add_head_info(struct worktree *wt) { int flags; const char *target; - int ignore_errno; target = refs_resolve_ref_unsafe(get_worktree_ref_store(wt), "HEAD", 0, - &wt->head_oid, &flags, - &ignore_errno); + &wt->head_oid, &flags); if (!target) return; @@ -416,7 +414,6 @@ const struct worktree *find_shared_symref(struct worktree **worktrees, const char *symref_target; struct ref_store *refs; int flags; - int ignore_errno; if (wt->is_bare) continue; @@ -434,8 +431,7 @@ const struct worktree *find_shared_symref(struct worktree **worktrees, refs = get_worktree_ref_store(wt); symref_target = refs_resolve_ref_unsafe(refs, symref, 0, - NULL, &flags, - &ignore_errno); + NULL, &flags); if ((flags & REF_ISSYMREF) && symref_target && !strcmp(symref_target, target)) { existing = wt; @@ -563,7 +559,6 @@ int other_head_refs(each_ref_fn fn, void *cb_data) struct worktree *wt = *p; struct object_id oid; int flag; - int ignore_errno; if (wt->is_current) continue; @@ -573,7 +568,7 @@ int other_head_refs(each_ref_fn fn, void *cb_data) if (refs_resolve_ref_unsafe(get_main_ref_store(the_repository), refname.buf, RESOLVE_REF_READING, - &oid, &flag, &ignore_errno)) + &oid, &flag)) ret = fn(refname.buf, &oid, flag, cb_data); if (ret) break; From 7838d9c2a9c246b6e59bcb4d48f70b919fe1f2c0 Mon Sep 17 00:00:00 2001 From: Greg Hurrell Date: Wed, 26 Jan 2022 13:14:25 +0100 Subject: [PATCH 08/17] Documentation/config/pgp.txt: replace stray character with Specifically, replace the tab between "the" and "first" with a space. Signed-off-by: Greg Hurrell Signed-off-by: Junio C Hamano --- Documentation/config/gpg.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Documentation/config/gpg.txt b/Documentation/config/gpg.txt index 0cb189a077ca54..abfabd6d62f6c0 100644 --- a/Documentation/config/gpg.txt +++ b/Documentation/config/gpg.txt @@ -37,7 +37,7 @@ gpg.minTrustLevel:: gpg.ssh.defaultKeyCommand:: This command that will be run when user.signingkey is not set and a ssh signature is requested. On successful exit a valid ssh public key is - expected in the first line of its output. To automatically use the first + expected in the first line of its output. To automatically use the first available key from your ssh-agent set this to "ssh-add -L". gpg.ssh.allowedSignersFile:: From cbac0076ef03892dbd76b6039710487d4f4322df Mon Sep 17 00:00:00 2001 From: Greg Hurrell Date: Wed, 26 Jan 2022 13:14:26 +0100 Subject: [PATCH 09/17] Documentation/config/pgp.txt: add missing apostrophe Add an apostrophe to "signatures" to indicate the possessive relationship in "the signature's creation". Signed-off-by: Greg Hurrell Signed-off-by: Junio C Hamano --- Documentation/config/gpg.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Documentation/config/gpg.txt b/Documentation/config/gpg.txt index abfabd6d62f6c0..86892ada771e80 100644 --- a/Documentation/config/gpg.txt +++ b/Documentation/config/gpg.txt @@ -66,7 +66,7 @@ This way only committers with an already valid key can add or change keys in the + Since OpensSSH 8.8 this file allows specifying a key lifetime using valid-after & valid-before options. Git will mark signatures as valid if the signing key was -valid at the time of the signatures creation. This allows users to change a +valid at the time of the signature's creation. This allows users to change a signing key without invalidating all previously made signatures. + Using a SSH CA key with the cert-authority option From fa1101afb66dfb3ad325ad135b7bed59e4067dd5 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Thu, 27 Jan 2022 11:02:57 -0800 Subject: [PATCH 10/17] SubmittingPatches: write problem statement in the log in the present tense We give a guidance for proposed log message to write problem statement first, followed by the reasoning behind, and recipe for, the solution. Clarify that we describe the situation _before_ the proposed patch is applied in the present tense (not in the past tense e.g. "we used to do X, but thanks to this commit we now do Y") for consistency. Signed-off-by: Junio C Hamano --- Documentation/SubmittingPatches | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches index 92b80d94d488c2..7225a0fb522d3b 100644 --- a/Documentation/SubmittingPatches +++ b/Documentation/SubmittingPatches @@ -142,6 +142,13 @@ The body should provide a meaningful commit message, which: . alternate solutions considered but discarded, if any. +[[present-tense]] +The problem statement that describes the status quo is written in the +present tense. Write "The code does X when it is given input Y", +instead of "The code used to do Y when given input X". You do not +have to say "Currently"---the status quo in the problem statement is +about the code _without_ your change, by project convention. + [[imperative-mood]] Describe your changes in imperative mood, e.g. "make xyzzy do frotz" instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy From 607817a3c8d2992f69e53c42dfa604b59d1570ba Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Thu, 27 Jan 2022 11:02:58 -0800 Subject: [PATCH 11/17] CodingGuidelines: hint why we value clearly written log messages Signed-off-by: Junio C Hamano --- Documentation/CodingGuidelines | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines index 0e27b5395d8b66..c37c43186ea80e 100644 --- a/Documentation/CodingGuidelines +++ b/Documentation/CodingGuidelines @@ -26,6 +26,13 @@ code. For Git in general, a few rough rules are: go and fix it up." Cf. http://lkml.iu.edu/hypermail/linux/kernel/1001.3/01069.html + - Log messages to explain your changes are as important as the + changes themselves. Clearly written code and in-code comments + explain how the code works and what is assumed from the surrounding + context. The log messages explain what the changes wanted to + achieve and why the changes were necessary (more on this in the + accompanying SubmittingPatches document). + Make your code readable and sensible, and don't try to be clever. As for more concrete guidelines, just imitate the existing code From cdba0295b0a70cf7a8113dd74be90d11ceddd5f7 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Thu, 27 Jan 2022 11:02:59 -0800 Subject: [PATCH 12/17] SubmittingPatches: explain why we care about log messages Extend the "describe your changes well" section to cover whom we are trying to help by doing so in the first place. Signed-off-by: Junio C Hamano --- Documentation/SubmittingPatches | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches index 7225a0fb522d3b..a6121d1d428024 100644 --- a/Documentation/SubmittingPatches +++ b/Documentation/SubmittingPatches @@ -110,6 +110,35 @@ run `git diff --check` on your changes before you commit. [[describe-changes]] === Describe your changes well. +The log message that explains your changes is just as important as the +changes themselves. Your code may be clearly written with in-code +comment to sufficiently explain how it works with the surrounding +code, but those who need to fix or enhance your code in the future +will need to know _why_ your code does what it does, for a few +reasons: + +. Your code may be doing something differently from what you wanted it + to do. Writing down what you actually wanted to achieve will help + them fix your code and make it do what it should have been doing + (also, you often discover your own bugs yourself, while writing the + log message to summarize the thought behind it). + +. Your code may be doing things that were only necessary for your + immediate needs (e.g. "do X to directories" without implementing or + even designing what is to be done on files). Writing down why you + excluded what the code does not do will help guide future developers. + Writing down "we do X to directories, because directories have + characteristic Y" would help them infer "oh, files also have the same + characteristic Y, so perhaps doing X to them would also make sense?". + Saying "we don't do the same X to files, because ..." will help them + decide if the reasoning is sound (in which case they do not waste + time extending your code to cover files), or reason differently (in + which case, they can explain why they extend your code to cover + files, too). + +The goal of your log message is to convey the _why_ behind your +change to help future developers. + The first line of the commit message should be a short description (50 characters is the soft limit, see DISCUSSION in linkgit:git-commit[1]), and should skip the full stop. It is also conventional in most cases to From 0f03f04c5c24239bafbb4ce1a9dd73d602d052cb Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Fri, 28 Jan 2022 01:58:18 +0000 Subject: [PATCH 13/17] sparse-checkout: fix a couple minor memory leaks These were introduced in commit 55dfcf9591 ("sparse-checkout: clear tracked sparse dirs", 2021-09-08) and missed in my review at the time. Plug the leaks. Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- builtin/sparse-checkout.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c index d0f5c4702be69d..23482d77c9832f 100644 --- a/builtin/sparse-checkout.c +++ b/builtin/sparse-checkout.c @@ -182,6 +182,8 @@ static void clean_tracked_sparse_directories(struct repository *r) item->string); } + strvec_clear(&s); + clear_pathspec(&p); dir_clear(&dir); } From 2826ffad8c34b639fd41f62fac1236e36e9d83db Mon Sep 17 00:00:00 2001 From: Robert Coup Date: Fri, 28 Jan 2022 14:36:02 +0000 Subject: [PATCH 14/17] fetch: fix negotiate-only error message The error message when invoking a negotiate-only fetch without providing any tips incorrectly refers to a --negotiate-tip=* argument. Fix this to use the actual argument, --negotiation-tip=*. Signed-off-by: Robert Coup Reviewed-by: Jonathan Tan Signed-off-by: Junio C Hamano --- builtin/fetch.c | 2 +- t/t5702-protocol-v2.sh | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/builtin/fetch.c b/builtin/fetch.c index 3f3fa0859fa7cc..c480db32ce31e2 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -1991,7 +1991,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix) } if (negotiate_only && !negotiation_tip.nr) - die(_("--negotiate-only needs one or more --negotiate-tip=*")); + die(_("--negotiate-only needs one or more --negotiation-tip=*")); if (deepen_relative) { if (deepen_relative < 0) diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh index 78de1ff2ad5900..66a16d4ab71510 100755 --- a/t/t5702-protocol-v2.sh +++ b/t/t5702-protocol-v2.sh @@ -606,7 +606,7 @@ test_expect_success 'usage: --negotiate-only without --negotiation-tip' ' setup_negotiate_only "$SERVER" "$URI" && cat >err.expect <<-\EOF && - fatal: --negotiate-only needs one or more --negotiate-tip=* + fatal: --negotiate-only needs one or more --negotiation-tip=* EOF test_must_fail git -c protocol.version=2 -C client fetch \ From c9e04d905edb5487c43b03304704e8d1248f9ac0 Mon Sep 17 00:00:00 2001 From: Thomas Gummerer Date: Mon, 31 Jan 2022 13:30:47 +0000 Subject: [PATCH 15/17] fetch --prune: exit with error if pruning fails When pruning refs fails, we print an error to stderr, but still exit 0 from 'git fetch'. Since this is a genuine error, fetch should be exiting with some non-zero exit code. Make it so. The --prune option was introduced in f360d844de ("builtin-fetch: add --prune option", 2009-11-10). Unfortunately it's unclear from that commit whether ignoring the exit code was an oversight or intentional, but it feels like an oversight. Helped-by: Johannes Schindelin Signed-off-by: Thomas Gummerer Signed-off-by: Junio C Hamano --- builtin/fetch.c | 10 ++++++---- t/t5510-fetch.sh | 11 +++++++++++ 2 files changed, 17 insertions(+), 4 deletions(-) diff --git a/builtin/fetch.c b/builtin/fetch.c index 5f06b21f8e97c5..648f0694f270e4 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -1609,12 +1609,14 @@ static int do_fetch(struct transport *transport, * don't care whether --tags was specified. */ if (rs->nr) { - prune_refs(rs, ref_map, transport->url); + retcode = prune_refs(rs, ref_map, transport->url); } else { - prune_refs(&transport->remote->fetch, - ref_map, - transport->url); + retcode = prune_refs(&transport->remote->fetch, + ref_map, + transport->url); } + if (retcode != 0) + retcode = 1; } if (fetch_and_consume_refs(transport, ref_map, worktrees)) { free_refs(ref_map); diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh index 20f7110ec108fd..ef0da0a63b5c2a 100755 --- a/t/t5510-fetch.sh +++ b/t/t5510-fetch.sh @@ -164,6 +164,17 @@ test_expect_success 'fetch --prune --tags with refspec prunes based on refspec' git rev-parse sometag ' +test_expect_success REFFILES 'fetch --prune fails to delete branches' ' + cd "$D" && + git clone . prune-fail && + cd prune-fail && + git update-ref refs/remotes/origin/extrabranch main && + : this will prevent --prune from locking packed-refs for deleting refs, but adding loose refs still succeeds && + >.git/packed-refs.new && + + test_must_fail git fetch --prune origin +' + test_expect_success 'fetch --atomic works with a single branch' ' test_when_finished "rm -rf \"$D\"/atomic" && From 74f3390dde73bccbcea43ffb15c91b76a6fa06bf Mon Sep 17 00:00:00 2001 From: Shaoxuan Yuan Date: Wed, 2 Feb 2022 15:28:44 +0800 Subject: [PATCH 16/17] builtin/diff.c: fix "git-diff" usage string typo Remove mistaken right square brackets from "git-diff" usage string. Make the usage string conform to "git-diff" documentation (Documentation/git-diff.txt). Signed-off-by: Shaoxuan Yuan Signed-off-by: Junio C Hamano --- builtin/diff.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/builtin/diff.c b/builtin/diff.c index fa4683377ebbe5..bb7fafca618154 100644 --- a/builtin/diff.c +++ b/builtin/diff.c @@ -28,9 +28,9 @@ static const char builtin_diff_usage[] = "git diff [] [] [--] [...]\n" " or: git diff [] --cached [--merge-base] [] [--] [...]\n" " or: git diff [] [--merge-base] [...] [--] [...]\n" -" or: git diff [] ...] [--] [...]\n" -" or: git diff [] ]\n" -" or: git diff [] --no-index [--] ]\n" +" or: git diff [] ... [--] [...]\n" +" or: git diff [] \n" +" or: git diff [] --no-index [--] \n" COMMON_DIFF_OPTIONS_HELP; static const char *blob_path(struct object_array_entry *entry) From b80121027d1247a0754b3cc46897fee75c050b44 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Fri, 11 Feb 2022 16:49:31 -0800 Subject: [PATCH 17/17] The third batch Signed-off-by: Junio C Hamano --- Documentation/RelNotes/2.36.0.txt | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/Documentation/RelNotes/2.36.0.txt b/Documentation/RelNotes/2.36.0.txt index 94db2458c70014..bac3efd715f2b2 100644 --- a/Documentation/RelNotes/2.36.0.txt +++ b/Documentation/RelNotes/2.36.0.txt @@ -103,6 +103,18 @@ Fixes since v2.35 * Update the logic to compute alignment requirement for our mem-pool. (merge e38bcc66d8 jc/mem-pool-alignment later to maint). + * Pick a better random number generator and use it when we prepare + temporary filenames. + (merge 47efda967c bc/csprng-mktemps later to maint). + + * Update the contributor-facing documents on proposed log messages. + (merge cdba0295b0 jc/doc-log-messages later to maint). + + * When "git fetch --prune" failed to prune the refs it wanted to + prune, the command issued error messages but exited with exit + status 0, which has been corrected. + (merge c9e04d905e tg/fetch-prune-exit-code-fix later to maint). + * Other code cleanup, docfix, build fix, etc. (merge cfc5cf428b jc/find-header later to maint). (merge 40e7cfdd46 jh/p4-fix-use-of-process-error-exception later to maint). @@ -110,3 +122,10 @@ Fixes since v2.35 (merge 0a6adc26e2 rs/grep-expr-cleanup later to maint). (merge 4ed7dfa713 po/readme-mention-contributor-hints later to maint). (merge 6046f7a91c en/plug-leaks-in-merge later to maint). + (merge 8c591dbfce bc/clarify-eol-attr later to maint). + (merge 518e15db74 rs/parse-options-lithelp-help later to maint). + (merge cbac0076ef gh/doc-typos later to maint). + (merge ce14de03db ab/no-errno-from-resolve-ref-unsafe later to maint). + (merge 2826ffad8c rc/negotiate-only-typofix later to maint). + (merge 0f03f04c5c en/sparse-checkout-leakfix later to maint). + (merge 74f3390dde sy/diff-usage-typofix later to maint).