* Re: [PATCH 00/12] Introduce `refStorage` extension
From: Karthik Nayak @ 2023-12-22 13:43 UTC (permalink / raw)
To: Patrick Steinhardt, git
In-Reply-To: <cover.1703067989.git.ps@pks.im>
Hello,
Patrick Steinhardt <ps@pks.im> writes:
> Hi,
>
> this patch series introduces a new `refStorage` extension and related
> tooling. While there is only a single user-visible ref backend for now,
> this extension will eventually allow us to determine which backend shall
> be used for a repository. Furthermore, the series introduces tooling so
> that the ref storage format becomes more accessible.
Apart from small nits, the series looks good to me, thanks!
^ permalink raw reply
* Re: [PATCH 12/12] t9500: write "extensions.refstorage" into config
From: Karthik Nayak @ 2023-12-22 13:43 UTC (permalink / raw)
To: Patrick Steinhardt, git
In-Reply-To: <bbe2fbb15495ad6c8eb0824b4b4aaa7f3e6e2537.1703067989.git.ps@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
> In t9500 we're writing a custom configuration that sets up gitweb. This
> requires us manually ensure that the repository format is configured as
>
Nit: s/requires us/requires us to/
^ permalink raw reply
* Re: [PATCH 04/12] setup: start tracking ref storage format when
From: Karthik Nayak @ 2023-12-22 13:09 UTC (permalink / raw)
To: Patrick Steinhardt, git
In-Reply-To: <6564659d403de098799ddb8101b74c2803a655d4.1703067989.git.ps@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
> In order to discern which ref storage format a repository is supposed to
> use we need to start setting up and/or discovering the format. This
> needs to happen in two separate code paths.
>
> - The first path is when we create a repository via `init_db()`. When
> we are re-initializing a preexisting repository we need to retain
> the previously used ref storage format -- if the user asked for a
> different format then this indicates an erorr and we error out.
Nit: s/erorr/error
> diff --git a/refs.c b/refs.c
> index e87c85897d..d289a5e175 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -2045,12 +2045,12 @@ static struct ref_store *ref_store_init(struct repository *repo,
> const char *gitdir,
> unsigned int flags)
> {
> - int format = REF_STORAGE_FORMAT_FILES;
> - const struct ref_storage_be *be = find_ref_storage_backend(format);
> + const struct ref_storage_be *be;
> struct ref_store *refs;
>
> + be = find_ref_storage_backend(repo->ref_storage_format);
> if (!be)
> - BUG("reference backend %s is unknown", ref_storage_format_to_name(format));
> + BUG("reference backend is unknown");
>
> refs = be->init(repo, gitdir, flags);
> return refs;
> diff --git a/refs.h b/refs.h
> index c6571bcf6c..944a67ac1b 100644
> --- a/refs.h
> +++ b/refs.h
> @@ -11,6 +11,7 @@ struct string_list;
> struct string_list_item;
> struct worktree;
>
> +int default_ref_storage_format(void);
>
Is this used/defined somewhere?
^ permalink raw reply
* Re: [PATCH 03/12] refs: refactor logic to look up storage backends
From: Karthik Nayak @ 2023-12-22 12:38 UTC (permalink / raw)
To: Patrick Steinhardt, git
In-Reply-To: <f712d5ef5bc5bed423cf50a0d0489095ba103df7.1703067989.git.ps@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
> +
> +const char *ref_storage_format_to_name(int ref_storage_format)
> +{
> + const struct ref_storage_be *be = find_ref_storage_backend(ref_storage_format);
> + if (!be)
> + return "unknown";
> + return be->name;
> +}
>
Would it make more sense to return NULL here?
> +
> /*
> * How to handle various characters in refnames:
> * 0: An acceptable character for refs
> @@ -2029,12 +2045,12 @@ static struct ref_store *ref_store_init(struct repository *repo,
> const char *gitdir,
> unsigned int flags)
> {
> - const char *be_name = "files";
> - struct ref_storage_be *be = find_ref_storage_backend(be_name);
> + int format = REF_STORAGE_FORMAT_FILES;
> + const struct ref_storage_be *be = find_ref_storage_backend(format);
> struct ref_store *refs;
>
> if (!be)
> - BUG("reference backend %s is unknown", be_name);
> + BUG("reference backend %s is unknown", ref_storage_format_to_name(format));
>
This doesn't really get us more information, since be == NULL, calling
`ref_storage_format_to_name` will again call `find_ref_storage_backend`,
which will be NULL. I wonder if its better to:
@@ -2029,12 +2045,12 @@ static struct ref_store *ref_store_init(struct
repository *repo,
const char *gitdir,
unsigned int flags)
{
- const char *be_name = "files";
- struct ref_storage_be *be = find_ref_storage_backend(be_name);
+ int format = REF_STORAGE_FORMAT_FILES;
+ const struct ref_storage_be *be = find_ref_storage_backend(format);
struct ref_store *refs;
if (!be)
- BUG("reference backend %s is unknown", be_name);
+ BUG("reference backend is unknown");
^ permalink raw reply
* Re: [PATCH 02/12] worktree: skip reading HEAD when repairing worktrees
From: Karthik Nayak @ 2023-12-22 12:23 UTC (permalink / raw)
To: Patrick Steinhardt, git
In-Reply-To: <e895091025e6786e0a831e4fb9b092eb74bd4379.1703067989.git.ps@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
> When calling `git init --separate-git-dir=<new-path>` on a preexisting
> repository, then we move the Git directory of that repository to the new
> path specified by the user. If there are worktrees present in the Git
> repository, we need to repair the worktrees so that their gitlinks point
> to the new location of the repository.
>
Nit: s/then we/we
^ permalink raw reply
* Re: [PATCH 01/12] t: introduce DEFAULT_REPO_FORMAT prereq
From: Karthik Nayak @ 2023-12-22 11:41 UTC (permalink / raw)
To: Patrick Steinhardt, git
In-Reply-To: <239ca38efdbe3195e6319be4423b17d2b42e0b9f.1703067989.git.ps@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
> A limited number of tests require repositories to have the default
> repository format or otherwise they would fail to run, e.g. because they
> fail to detect the correct hash function. While the hash function is the
> only extension right now that creates problems like this, we are about
> to add a second extensions for the ref format.
>
Nit: s/extensions/extension
> -test_expect_success SHA1 'git branch -m q q2 without config should succeed' '
> +test_expect_success DEFAULT_REPO_FORMAT 'git branch -m q q2 without config should succeed' '
> git branch -m q q2 &&
> git branch -m q2 q
> '
My only concern is whether we'll end up blindly adding
DEFAULT_REPO_FORMAT for tests where only SHA1 is a prereq or only where
the new format extension is a prereq.
^ permalink raw reply
* Re: rebase invoking pre-commit
From: Phillip Wood @ 2023-12-22 10:05 UTC (permalink / raw)
To: Sean Allred, Git List
In-Reply-To: <m0sf3vi86g.fsf@epic96565.epic.com>
Hi Sean
On 21/12/2023 20:58, Sean Allred wrote:
> Is there a current reason why pre-commit shouldn't be invoked during
> rebase, or is this just waiting for a reviewable patch?
The reason that we don't run the pre-commit hook is that the commit
being rebased may have been created with "git commit --no-verify" and so
running the pre-commit hook would stop it from being rebased - see
e637122ef2 (rebase -m: do not trigger pre-commit verification, 2008-03-16).
I think that most of the time it would be valuable to run the pre-commit
hook when committing a conflict resolution but we'd need to add
something like "git rebase --continue --no-verify" as a way to bypass it
when resolving conflicts in commits that were created with "git commit
--no-verify".
Best Wishes
Phillip
> This was brought up before at [1] in 2015, but that thread so old at
> this point that it seemed prudent to double-check before investing time
> in a developing and testing a patch.
>
> [1]: https://lore.kernel.org/git/1m55i3m.1fum4zo1fpnhncM%25lists@haller-berlin.de/
>
> --
> Sean Allred
>
^ permalink raw reply
* Re: [PATCH v2 1/9] CodingGuidelines: move period inside parentheses
From: Dragan Simic @ 2023-12-22 1:30 UTC (permalink / raw)
To: Junio C Hamano
Cc: Josh Soref via GitGitGadget, git, Elijah Newren,
René Scharfe, Phillip Wood, Josh Soref
In-Reply-To: <xmqqedffl13r.fsf@gitster.g>
On 2023-12-21 22:03, Junio C Hamano wrote:
> "Josh Soref via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> From: Josh Soref <jsoref@gmail.com>
>>
>> The contents within parenthesis should be omittable without resulting
>> in broken text.
>>
>> Eliding the parenthesis left a period to end a run without any
>> content.
>
> Nobody seems to have commented on this one in the previous round,
> but quite honestly, for this particular instance, I suspect that it
> may become easier to read if we just lost these parentheses, as the
> next sentence "You do not have to include more than ..." is just a
> bit of extra material to support the first sentence like the one we
> see in the parentheses. Alternatively, moving that "You do not have
> to" also inside the same parentheses might work slightly better.
I agree with simply erasing the parentheses, it makes the paragraph flow
much better.
> It might be even easier to follow if we moved the list of "approved
> headers" (and the "You do not have to ... more than one" note that
> supports the "currently approved list") totally out of line by
> making it a side note.
>
>> Signed-off-by: Josh Soref <jsoref@gmail.com>
>> ---
>> Documentation/CodingGuidelines | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/Documentation/CodingGuidelines
>> b/Documentation/CodingGuidelines
>> index 8ed517a5ca0..af94ed3a75d 100644
>> --- a/Documentation/CodingGuidelines
>> +++ b/Documentation/CodingGuidelines
>> @@ -450,7 +450,7 @@ For C programs:
>> one of the approved headers that includes it first for you. (The
>> approved headers currently include "builtin.h",
>> "t/helper/test-tool.h", "xdiff/xinclude.h", or
>> - "reftable/system.h"). You do not have to include more than one of
>> + "reftable/system.h".) You do not have to include more than one of
>> these.
>>
>> - A C file must directly include the header files that declare the
^ permalink raw reply
* [PATCH] Port helper/test-ctype.c to unit-tests/t-ctype.c
From: Achu Luma @ 2023-12-21 23:15 UTC (permalink / raw)
To: christian.couder; +Cc: git, Achu Luma, Christian Couder
In the recent codebase update (8bf6fbd00d (Merge branch
'js/doc-unit-tests', 2023-12-09)), a new unit testing framework was
merged, providing a standardized approach for testing C code. Prior to
this update, some unit tests relied on the test helper mechanism,
lacking a dedicated unit testing framework. It's more natural to perform
these unit tests using the new unit test framework.
This commit migrates the unit tests for C character classification
functions (isdigit(), isspace(), etc) from the legacy approach
using the test-tool command `test-tool ctype` in t/helper/test-ctype.c
to the new unit testing framework (t/unit-tests/test-lib.h).
The migration involves refactoring the tests to utilize the testing
macros provided by the framework (TEST() and check_*()).
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Achu Luma <ach.lumap@gmail.com>
---
Makefile | 2 +-
t/helper/test-ctype.c | 70 --------------------------------------
t/helper/test-tool.c | 1 -
t/helper/test-tool.h | 1 -
t/t0070-fundamental.sh | 4 ---
t/unit-tests/t-ctype.c | 76 ++++++++++++++++++++++++++++++++++++++++++
6 files changed, 77 insertions(+), 77 deletions(-)
delete mode 100644 t/helper/test-ctype.c
create mode 100644 t/unit-tests/t-ctype.c
diff --git a/Makefile b/Makefile
index 88ba7a3c51..a4df48ba65 100644
--- a/Makefile
+++ b/Makefile
@@ -792,7 +792,6 @@ 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
TEST_BUILTINS_OBJS += test-dir-iterator.o
@@ -1341,6 +1340,7 @@ THIRD_PARTY_SOURCES += sha1dc/%
UNIT_TEST_PROGRAMS += t-basic
UNIT_TEST_PROGRAMS += t-strbuf
+UNIT_TEST_PROGRAMS += t-ctype
UNIT_TEST_PROGS = $(patsubst %,$(UNIT_TEST_BIN)/%$X,$(UNIT_TEST_PROGRAMS))
UNIT_TEST_OBJS = $(patsubst %,$(UNIT_TEST_DIR)/%.o,$(UNIT_TEST_PROGRAMS))
UNIT_TEST_OBJS += $(UNIT_TEST_DIR)/test-lib.o
diff --git a/t/helper/test-ctype.c b/t/helper/test-ctype.c
deleted file mode 100644
index e5659df40b..0000000000
--- a/t/helper/test-ctype.c
+++ /dev/null
@@ -1,70 +0,0 @@
-#include "test-tool.h"
-
-static int rc;
-
-static void report_error(const char *class, int ch)
-{
- printf("%s classifies char %d (0x%02x) wrongly\n", class, ch, ch);
- rc = 1;
-}
-
-static int is_in(const char *s, int ch)
-{
- /*
- * We can't find NUL using strchr. Accept it as the first
- * character in the spec -- there are no empty classes.
- */
- if (ch == '\0')
- return ch == *s;
- if (*s == '\0')
- s++;
- return !!strchr(s, ch);
-}
-
-#define TEST_CLASS(t,s) { \
- int i; \
- for (i = 0; i < 256; i++) { \
- if (is_in(s, i) != t(i)) \
- report_error(#t, i); \
- } \
- if (t(EOF)) \
- report_error(#t, EOF); \
-}
-
-#define DIGIT "0123456789"
-#define LOWER "abcdefghijklmnopqrstuvwxyz"
-#define UPPER "ABCDEFGHIJKLMNOPQRSTUVWXYZ"
-#define PUNCT "!\"#$%&'()*+,-./:;<=>?@[\\]^_`{|}~"
-#define ASCII \
- "\x00\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f" \
- "\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f" \
- "\x20\x21\x22\x23\x24\x25\x26\x27\x28\x29\x2a\x2b\x2c\x2d\x2e\x2f" \
- "\x30\x31\x32\x33\x34\x35\x36\x37\x38\x39\x3a\x3b\x3c\x3d\x3e\x3f" \
- "\x40\x41\x42\x43\x44\x45\x46\x47\x48\x49\x4a\x4b\x4c\x4d\x4e\x4f" \
- "\x50\x51\x52\x53\x54\x55\x56\x57\x58\x59\x5a\x5b\x5c\x5d\x5e\x5f" \
- "\x60\x61\x62\x63\x64\x65\x66\x67\x68\x69\x6a\x6b\x6c\x6d\x6e\x6f" \
- "\x70\x71\x72\x73\x74\x75\x76\x77\x78\x79\x7a\x7b\x7c\x7d\x7e\x7f"
-#define CNTRL \
- "\x00\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f" \
- "\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f" \
- "\x7f"
-
-int cmd__ctype(int argc UNUSED, const char **argv UNUSED)
-{
- TEST_CLASS(isdigit, DIGIT);
- TEST_CLASS(isspace, " \n\r\t");
- TEST_CLASS(isalpha, LOWER UPPER);
- TEST_CLASS(isalnum, LOWER UPPER DIGIT);
- TEST_CLASS(is_glob_special, "*?[\\");
- TEST_CLASS(is_regex_special, "$()*+.?[\\^{|");
- TEST_CLASS(is_pathspec_magic, "!\"#%&',-/:;<=>@_`~");
- TEST_CLASS(isascii, ASCII);
- TEST_CLASS(islower, LOWER);
- TEST_CLASS(isupper, UPPER);
- TEST_CLASS(iscntrl, CNTRL);
- TEST_CLASS(ispunct, PUNCT);
- TEST_CLASS(isxdigit, DIGIT "abcdefABCDEF");
- TEST_CLASS(isprint, LOWER UPPER DIGIT PUNCT " ");
-
- return rc;
-}
diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c
index 37ba996539..33b9501c21 100644
--- a/t/helper/test-tool.c
+++ b/t/helper/test-tool.c
@@ -19,7 +19,6 @@ static struct test_cmd cmds[] = {
{ "config", cmd__config },
{ "crontab", cmd__crontab },
{ "csprng", cmd__csprng },
- { "ctype", cmd__ctype },
{ "date", cmd__date },
{ "delta", cmd__delta },
{ "dir-iterator", cmd__dir_iterator },
diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h
index 8a1a7c63da..b72f07ded9 100644
--- a/t/helper/test-tool.h
+++ b/t/helper/test-tool.h
@@ -12,7 +12,6 @@ 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);
int cmd__dir_iterator(int argc, const char **argv);
diff --git a/t/t0070-fundamental.sh b/t/t0070-fundamental.sh
index 487bc8d905..a4756fbab9 100755
--- a/t/t0070-fundamental.sh
+++ b/t/t0070-fundamental.sh
@@ -9,10 +9,6 @@ Verify wrappers and compatibility functions.
TEST_PASSES_SANITIZE_LEAK=true
. ./test-lib.sh
-test_expect_success 'character classes (isspace, isalpha etc.)' '
- test-tool ctype
-'
-
test_expect_success 'mktemp to nonexistent directory prints filename' '
test_must_fail test-tool mktemp doesnotexist/testXXXXXX 2>err &&
grep "doesnotexist/test" err
diff --git a/t/unit-tests/t-ctype.c b/t/unit-tests/t-ctype.c
new file mode 100644
index 0000000000..41189ba9f9
--- /dev/null
+++ b/t/unit-tests/t-ctype.c
@@ -0,0 +1,76 @@
+#include "test-lib.h"
+
+static int is_in(const char *s, int ch)
+{
+ /*
+ * We can't find NUL using strchr. Accept it as the first
+ * character in the spec -- there are no empty classes.
+ */
+ if (ch == '\0')
+ return ch == *s;
+ if (*s == '\0')
+ s++;
+ return !!strchr(s, ch);
+}
+
+/* Macro to test a character type */
+#define TEST_CTYPE_FUNC(func, string) \
+static void test_ctype_##func(void) \
+{ \
+ int i; \
+ for (i = 0; i < 256; i++) \
+ check_int(func(i), ==, is_in(string, i)); \
+}
+
+#define DIGIT "0123456789"
+#define LOWER "abcdefghijklmnopqrstuvwxyz"
+#define UPPER "ABCDEFGHIJKLMNOPQRSTUVWXYZ"
+#define PUNCT "!\"#$%&'()*+,-./:;<=>?@[\\]^_`{|}~"
+#define ASCII \
+ "\x00\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f" \
+ "\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f" \
+ "\x20\x21\x22\x23\x24\x25\x26\x27\x28\x29\x2a\x2b\x2c\x2d\x2e\x2f" \
+ "\x30\x31\x32\x33\x34\x35\x36\x37\x38\x39\x3a\x3b\x3c\x3d\x3e\x3f" \
+ "\x40\x41\x42\x43\x44\x45\x46\x47\x48\x49\x4a\x4b\x4c\x4d\x4e\x4f" \
+ "\x50\x51\x52\x53\x54\x55\x56\x57\x58\x59\x5a\x5b\x5c\x5d\x5e\x5f" \
+ "\x60\x61\x62\x63\x64\x65\x66\x67\x68\x69\x6a\x6b\x6c\x6d\x6e\x6f" \
+ "\x70\x71\x72\x73\x74\x75\x76\x77\x78\x79\x7a\x7b\x7c\x7d\x7e\x7f"
+#define CNTRL \
+ "\x00\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f" \
+ "\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f" \
+ "\x7f"
+
+TEST_CTYPE_FUNC(isdigit, DIGIT)
+TEST_CTYPE_FUNC(isspace, " \n\r\t")
+TEST_CTYPE_FUNC(isalpha, LOWER UPPER)
+TEST_CTYPE_FUNC(isalnum, LOWER UPPER DIGIT)
+TEST_CTYPE_FUNC(is_glob_special, "*?[\\")
+TEST_CTYPE_FUNC(is_regex_special, "$()*+.?[\\^{|")
+TEST_CTYPE_FUNC(is_pathspec_magic, "!\"#%&',-/:;<=>@_`~")
+TEST_CTYPE_FUNC(isascii, ASCII)
+TEST_CTYPE_FUNC(islower, LOWER)
+TEST_CTYPE_FUNC(isupper, UPPER)
+TEST_CTYPE_FUNC(iscntrl, CNTRL)
+TEST_CTYPE_FUNC(ispunct, PUNCT)
+TEST_CTYPE_FUNC(isxdigit, DIGIT "abcdefABCDEF")
+TEST_CTYPE_FUNC(isprint, LOWER UPPER DIGIT PUNCT " ")
+
+int cmd_main(int argc, const char **argv) {
+ /* Run all character type tests */
+ TEST(test_ctype_isspace(), "isspace() works as we expect");
+ TEST(test_ctype_isdigit(), "isdigit() works as we expect");
+ TEST(test_ctype_isalpha(), "isalpha() works as we expect");
+ TEST(test_ctype_isalnum(), "isalnum() works as we expect");
+ TEST(test_ctype_is_glob_special(), "is_glob_special() works as we expect");
+ TEST(test_ctype_is_regex_special(), "is_regex_special() works as we expect");
+ TEST(test_ctype_is_pathspec_magic(), "is_pathspec_magic() works as we expect");
+ TEST(test_ctype_isascii(), "isascii() works as we expect");
+ TEST(test_ctype_islower(), "islower() works as we expect");
+ TEST(test_ctype_isupper(), "isupper() works as we expect");
+ TEST(test_ctype_iscntrl(), "iscntrl() works as we expect");
+ TEST(test_ctype_ispunct(), "ispunct() works as we expect");
+ TEST(test_ctype_isxdigit(), "isxdigit() works as we expect");
+ TEST(test_ctype_isprint(), "isprint() works as we expect");
+
+ return test_done();
+}
base-commit: 055bb6e9969085777b7fab83e3fee0017654f134
--
2.42.0.windows.2
^ permalink raw reply related
* Re: [PATCH] t1006: add tests for %(objectsize:disk)
From: René Scharfe @ 2023-12-21 23:13 UTC (permalink / raw)
To: Jeff King; +Cc: git, Ondrej Pohorelsky, brian m . carlson, Junio C Hamano
In-Reply-To: <20231221213034.GB1446091@coredump.intra.peff.net>
Am 21.12.23 um 22:30 schrieb Jeff King:
> On Thu, Dec 21, 2023 at 01:19:53PM +0100, René Scharfe wrote:
>
>> I think we can do it even in shell, especially if...
>> [...]
>
> Yeah, your conversion looks accurate. I do wonder if it is worth golfing
> further, though. If it were a process invocation per object, I'd
> definitely say the efficiency gain is worth it. But dropping one process
> from the whole test isn't that exciting either way.
Fair enough.
>
>> (sort -r), then we don't need to carry the oid forward:
>>
>> sort -nr <idx.raw >idx.sorted &&
>> packsz=$(test_file_size "${idx%.idx}.pack") &&
>> end=$((packsz - rawsz)) &&
>> awk -v end="$end" "
>> { print \$2, end - \$1; end = \$1 }
>> " idx.sorted ||
>>
>> And at that point it should be easy to use a shell loop instead of awk:
>>
>> while read start oid rest
>> do
>> size=$((end - start)) &&
>> end=$start &&
>> echo "$oid $size" ||
>> return 1
>> done <idx.sorted
>
> The one thing I do like is that we don't have to escape anything inside
> an awk program that is forced to use double-quotes. ;)
For me it's processing the data in the "correct" order (descending, i.e.
starting at the end, which we have to calculate first anyway based on the
size).
>> Should we deduplicate here, like cat-file does (i.e. use "sort -u")?
>> Having the same object in multiple places for whatever reason would not
>> be a cause for reporting an error in this test, I would think.
>
> No, for the reasons I said in the commit message: if an object exists in
> multiple places the test is already potentially invalid, as Git does not
> promise which version it will use. So it might work racily, or it might
> work for now but be fragile. By not de-duplicating, we make sure the
> test's assumption holds.
Oh, skipped that paragraph. Still I don't see how a duplicate object
would necessarily invalidate t1006. The comment for the test "cat-file
--batch-all-objects shows all objects" a few lines above indicates that
it's picky about the provenance of objects, but it uses a separate
repository. I can't infer the same requirement for the root repo, but
we already established that I can't read.
Anyway, if someone finds a use for git repack without -d or
git unpack-objects or whatever else causes duplicates in the root
repository of t1006 then they can try to reverse your ban with concrete
arguments.
René
^ permalink raw reply
* [PATCH] mem-pool: fix big allocations
From: René Scharfe @ 2023-12-21 23:13 UTC (permalink / raw)
To: Git List; +Cc: Jameson Miller
Memory pool allocations that require a new block and would fill at
least half of it are handled specially. Before 158dfeff3d (mem-pool:
add life cycle management functions, 2018-07-02) they used to be
allocated outside of the pool. This patch made mem_pool_alloc() create
a bespoke block instead, to allow releasing it when the pool gets
discarded.
Unfortunately mem_pool_alloc() returns a pointer to the start of such a
bespoke block, i.e. to the struct mp_block at its top. When the caller
writes to it, the management information gets corrupted. This affects
mem_pool_discard() and -- if there are no other blocks in the pool --
also mem_pool_alloc().
Return the payload pointer of bespoke blocks, just like for smaller
allocations, to protect the management struct.
Also update next_free to mark the block as full. This is only strictly
necessary for the first allocated block, because subsequent ones are
inserted after the current block and never considered for further
allocations, but it's easier to just do it in all cases.
Add a basic unit test to demonstate the issue by using mem_pool_calloc()
with a tiny block size, which forces the creation of a bespoke block.
Without the mem_pool_alloc() fix it reports zeroed pointers:
ok 1 - mem_pool_calloc returns 100 zeroed bytes with big block
# check "((pool->mp_block->next_free) != (((void*)0))) == 1" failed at t/unit-tests/t-mem-pool.c:22
# left: 0
# right: 1
# check "((pool->mp_block->end) != (((void*)0))) == 1" failed at t/unit-tests/t-mem-pool.c:23
# left: 0
# right: 1
not ok 2 - mem_pool_calloc returns 100 zeroed bytes with tiny block
1..2
Signed-off-by: René Scharfe <l.s.r@web.de>
---
Makefile | 1 +
mem-pool.c | 6 +++---
t/unit-tests/t-mem-pool.c | 34 ++++++++++++++++++++++++++++++++++
3 files changed, 38 insertions(+), 3 deletions(-)
create mode 100644 t/unit-tests/t-mem-pool.c
diff --git a/Makefile b/Makefile
index 88ba7a3c51..15990ff312 100644
--- a/Makefile
+++ b/Makefile
@@ -1340,6 +1340,7 @@ THIRD_PARTY_SOURCES += sha1collisiondetection/%
THIRD_PARTY_SOURCES += sha1dc/%
UNIT_TEST_PROGRAMS += t-basic
+UNIT_TEST_PROGRAMS += t-mem-pool
UNIT_TEST_PROGRAMS += t-strbuf
UNIT_TEST_PROGS = $(patsubst %,$(UNIT_TEST_BIN)/%$X,$(UNIT_TEST_PROGRAMS))
UNIT_TEST_OBJS = $(patsubst %,$(UNIT_TEST_DIR)/%.o,$(UNIT_TEST_PROGRAMS))
diff --git a/mem-pool.c b/mem-pool.c
index c34846d176..e8d976c3ee 100644
--- a/mem-pool.c
+++ b/mem-pool.c
@@ -99,9 +99,9 @@ void *mem_pool_alloc(struct mem_pool *pool, size_t len)
if (!p) {
if (len >= (pool->block_alloc / 2))
- return mem_pool_alloc_block(pool, len, pool->mp_block);
-
- p = mem_pool_alloc_block(pool, pool->block_alloc, NULL);
+ p = mem_pool_alloc_block(pool, len, pool->mp_block);
+ else
+ p = mem_pool_alloc_block(pool, pool->block_alloc, NULL);
}
r = p->next_free;
diff --git a/t/unit-tests/t-mem-pool.c b/t/unit-tests/t-mem-pool.c
new file mode 100644
index 0000000000..2295779b0b
--- /dev/null
+++ b/t/unit-tests/t-mem-pool.c
@@ -0,0 +1,34 @@
+#include "test-lib.h"
+#include "mem-pool.h"
+
+#define check_ptr(a, op, b) check_int(((a) op (b)), ==, 1)
+
+static void setup_static(void (*f)(struct mem_pool *), size_t block_alloc)
+{
+ struct mem_pool pool = { .block_alloc = block_alloc };
+ f(&pool);
+ mem_pool_discard(&pool, 0);
+}
+
+static void t_calloc_100(struct mem_pool *pool)
+{
+ size_t size = 100;
+ char *buffer = mem_pool_calloc(pool, 1, size);
+ for (size_t i = 0; i < size; i++)
+ check_int(buffer[i], ==, 0);
+ if (!check_ptr(pool->mp_block, !=, NULL))
+ return;
+ check_ptr(pool->mp_block->next_free, <=, pool->mp_block->end);
+ check_ptr(pool->mp_block->next_free, !=, NULL);
+ check_ptr(pool->mp_block->end, !=, NULL);
+}
+
+int cmd_main(int argc, const char **argv)
+{
+ TEST(setup_static(t_calloc_100, 1024 * 1024),
+ "mem_pool_calloc returns 100 zeroed bytes with big block");
+ TEST(setup_static(t_calloc_100, 1),
+ "mem_pool_calloc returns 100 zeroed bytes with tiny block");
+
+ return test_done();
+}
--
2.43.0
^ permalink raw reply related
* Re: [PATCH/RFC] sparse-checkout: take care of "--end-of-options" in set/add
From: Junio C Hamano @ 2023-12-21 22:04 UTC (permalink / raw)
To: Jeff King; +Cc: Josh Steadmon, git
In-Reply-To: <20231221214550.GD1446091@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> Right, that is the "gotcha" I mentioned in my other email. Though that
> is the way it has behaved historically, my argument is that users are
> unreasonable to expect it to work:
>
> 1. It is not consistent with the rest of Git commands.
>
> 2. It is inconsistent with respect to existing options (and is an
> accident waiting to happen when new options are added).
>
> So I'd consider it a bug-fix.
So the counter-proposal here is just to drop KEEP_UNKNOWN_OPT and
deliberately break them^W^W^Wrealign their expectations?
I do not have much stake in sparse-checkout, so I am fine with that
direction. But I suspect other folks on the list would have users
of their own who would rather loudly complain to them if we broke
them ;-)
I'll see how bad the fallout would be if we go that route, using the
test modification I made for the previous rounds as yardsticks.
Thanks.
^ permalink raw reply
* Re: [PATCH v2 2/9] CodingGuidelines: write punctuation marks
From: Junio C Hamano @ 2023-12-21 21:57 UTC (permalink / raw)
To: Josh Soref; +Cc: git, Elijah Newren, René Scharfe, Phillip Wood
In-Reply-To: <CACZqfqAtT_M8T3AxbgZrkgi3GtQ=JZLu4wvh3O0C9dFvCU3rWg@mail.gmail.com>
Josh Soref <jsoref@gmail.com> writes:
> Junio C Hamano wrote:
>> Nobody seems to have commented on this step in the previous round,
>> but I do not understand what you mean by ...
>>
>> > From: Josh Soref <jsoref@gmail.com>
>> > - Match style in Release Notes
>>
>> ... at all. The patch text is fine, though.
>
> https://github.com/gitgitgadget/git/blob/692be87cbba55e8488f805d236f2ad50483bd7d5/Documentation/RelNotes/2.43.0.txt#L221
You mean just a single mention of the phrase? If so, you should
pinpoint it, e.g. "The release notes for Git 2.43 calls them
'punctuation marks', so let's use that here, too" or something like
that.
Unless you are sure that all the issues of Release Notes have and
will consistently say "punctuation marks" and never say
"punctuations", that is.
^ permalink raw reply
* Re: [PATCH v2 5/9] SubmittingPatches: update extra tags list
From: Josh Soref @ 2023-12-21 21:55 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Elijah Newren, René Scharfe, Phillip Wood
In-Reply-To: <xmqq4jgbl0iz.fsf@gitster.g>
Junio C Hamano wrote:
> "Josh Soref via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > +. `Co-authored-by:` is used to indicate that people exchanged drafts
> > + of a patch before submitting it.
>
> I am afraid this misses the essence of what Co-authoring means.
> You may have shared your draft as FYI to your colleagues, and they
> may have tweaked phrasing and responded with their reviews to help
> you improve _your_ work, but that may not make them your Co-authors.
>
> I think the "intent" counts more. If people closely worked
> together, exchanging drafts and agreeing on the final version among
> themselves before submitting, with the understanding that they share
> the authorship credit/blame, they are Co-authors.
. `Co-authored-by:` is used to indicate that people collaborated on the
contents of a patch before submitting it.
?
^ permalink raw reply
* Re: [PATCH v2 1/9] CodingGuidelines: move period inside parentheses
From: Josh Soref @ 2023-12-21 21:52 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Elijah Newren, René Scharfe, Phillip Wood
In-Reply-To: <xmqqedffl13r.fsf@gitster.g>
Junio C Hamano wrote:
> > From: Josh Soref <jsoref@gmail.com>
> >
> > The contents within parenthesis should be omittable without resulting
> > in broken text.
> >
> > Eliding the parenthesis left a period to end a run without any content.
>
> Nobody seems to have commented on this one in the previous round,
> but quite honestly, for this particular instance, I suspect that it
> may become easier to read if we just lost these parentheses, as the
> next sentence "You do not have to include more than ..." is just a
> bit of extra material to support the first sentence like the one we
> see in the parentheses. Alternatively, moving that "You do not have
> to" also inside the same parentheses might work slightly better.
> It might be even easier to follow if we moved the list of "approved
> headers" (and the "You do not have to ... more than one" note that
> supports the "currently approved list") totally out of line by
> making it a side note.
I think, in principle, it'd be better if it was its own thing, but the
structure of this entire section makes this really hard -- I certainly
can't imagine a way to do it.
The laziest fix is:
- The first #include in C files, except in platform specific compat/
implementations and sha1dc/, must be one of "git-compat-util.h",
"builtin.h", "t/helper/test-tool.h", "xdiff/xinclude.h", or
"reftable/system.h".
Beyond that, I don't understand how to parse:
You do not have to include more than one of these.
(Sorry, I'd like to be slightly ignorant of the C parts of git --
let's ignore that I've been blamed for at least one change to them --
so, I'm not going to look.)
^ permalink raw reply
* Re: [PATCH v2 2/9] CodingGuidelines: write punctuation marks
From: Josh Soref @ 2023-12-21 21:46 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Elijah Newren, René Scharfe, Phillip Wood
In-Reply-To: <xmqqplyzl1e8.fsf@gitster.g>
Junio C Hamano wrote:
> Nobody seems to have commented on this step in the previous round,
> but I do not understand what you mean by ...
>
> > From: Josh Soref <jsoref@gmail.com>
> > - Match style in Release Notes
>
> ... at all. The patch text is fine, though.
https://github.com/gitgitgadget/git/blob/692be87cbba55e8488f805d236f2ad50483bd7d5/Documentation/RelNotes/2.43.0.txt#L221
^ permalink raw reply
* Re: [PATCH/RFC] sparse-checkout: take care of "--end-of-options" in set/add
From: Jeff King @ 2023-12-21 21:45 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Josh Steadmon, git
In-Reply-To: <xmqqsf3vmqug.fsf@gitster.g>
On Thu, Dec 21, 2023 at 09:02:15AM -0800, Junio C Hamano wrote:
> > Yeah. I think it is in the same boat as the other two, in that I believe
> > that the KEEP_UNKNOWN_OPT flag is counter-productive and should just be
> > dropped.
>
> If we dropped KEEP_UNKNOWN_OPT, however, the pattern that is
> currently accepted will stop working, e.g.,
>
> $ git sparse-checkout [add/set] --[no-]cone --foo bar
>
> as we would barf with "--foo: unknown option", and the users who are
> used to this sloppy command line parsing we had for the past few
> years must now write "--end-of-options" before "--foo". After all,
> the reason why the original authors of this code used KEEP_UNKNOWN
> is likely to deal with path patterns that begin with dashes.
Right, that is the "gotcha" I mentioned in my other email. Though that
is the way it has behaved historically, my argument is that users are
unreasonable to expect it to work:
1. It is not consistent with the rest of Git commands.
2. It is inconsistent with respect to existing options (and is an
accident waiting to happen when new options are added).
So I'd consider it a bug-fix.
> The patch in the message that started this thread may not be
> correct, either, I am afraid. For either of these:
>
> $ git sparse-checkout [add/set] --[no-]cone foo --end-of-options bar
> $ git sparse-checkout [add/set] --[no-]cone --foo --end-of-options bar
>
> we would see that "foo" (or "--foo") is not "--end-of-options", and
> we end up using three patterns "foo" (or "--foo"),
> "--end-of-options", and "bar", I suspect. I wonder if we should
> notice the "foo" or "--foo" that were not understood and error out,
> instead.
Yes, I agree that "--foo --end-of-options" should barf. And of course
that happens naturally if you just let parse-options do its job by not
passing the KEEP_UNKNOWN_OPT flag. ;)
I'm not sure about "foo". We do allow out-of-order options/args, so
isn't it correct to keep it as a non-option argument?
> But after all, it is not absolutely necessary to notice and barf.
> The ONLY practical use of the "--end-of-options" mechanism is to
> allow us to write (this applies to any git subcommand):
>
> #!/bin/sh
> git cmd --hard --coded --options --end-of-options "$@"
>
> in scripts to protect the intended operation from mistaking the
> end-user input as options. And with a script written carefully to
> do so, all the args that appear before "--end-of-options" would be
> recognizable by the command line parser.
Yes, but if you misspell "--otpions", it magically becomes a parameter
rather than having the command barf and complain. That is not the end of
the world, but it is unfriendly and inconsistent with the rest of Git.
-Peff
^ permalink raw reply
* Re: [RFC/PATCH] archive: "--list" does not take further options
From: Jeff King @ 2023-12-21 21:35 UTC (permalink / raw)
To: Junio C Hamano; +Cc: René Scharfe, git
In-Reply-To: <xmqqmsu3mnix.fsf@gitster.g>
On Thu, Dec 21, 2023 at 10:13:58AM -0800, Junio C Hamano wrote:
> Thanks, both. Just to tie the loose end, let me queue this and
> merge it to 'next'.
>
> ----- >8 --------- >8 --------- >8 -----
> Subject: [PATCH] archive: "--list" does not take further options
This looks fine to me. It does mean that this sequence of commands no
longer works:
[let's try to make a "foo" archive]
$ git archive --format=foo HEAD
fatal: Unknown archive format 'foo'
[oops, that didn't work. What formats are supported?]
$ !! --list
git archive --format=foo HEAD --list
tar
tgz
tar.gz
zip
I think that's OK in practice. The increased error checking is worth it
(and matches many other commands, which tend to warn about confusing
extra bits).
-Peff
^ permalink raw reply
* Re: [PATCH] t1006: add tests for %(objectsize:disk)
From: Jeff King @ 2023-12-21 21:30 UTC (permalink / raw)
To: René Scharfe
Cc: git, Ondrej Pohorelsky, brian m . carlson, Junio C Hamano
In-Reply-To: <d44cb8e7-ffce-4184-b9b5-6bb56705dcd1@web.de>
On Thu, Dec 21, 2023 at 01:19:53PM +0100, René Scharfe wrote:
> I think we can do it even in shell, especially if...
> [...]
Yeah, your conversion looks accurate. I do wonder if it is worth golfing
further, though. If it were a process invocation per object, I'd
definitely say the efficiency gain is worth it. But dropping one process
from the whole test isn't that exciting either way.
> (sort -r), then we don't need to carry the oid forward:
>
> sort -nr <idx.raw >idx.sorted &&
> packsz=$(test_file_size "${idx%.idx}.pack") &&
> end=$((packsz - rawsz)) &&
> awk -v end="$end" "
> { print \$2, end - \$1; end = \$1 }
> " idx.sorted ||
>
> And at that point it should be easy to use a shell loop instead of awk:
>
> while read start oid rest
> do
> size=$((end - start)) &&
> end=$start &&
> echo "$oid $size" ||
> return 1
> done <idx.sorted
The one thing I do like is that we don't have to escape anything inside
an awk program that is forced to use double-quotes. ;)
> Should we deduplicate here, like cat-file does (i.e. use "sort -u")?
> Having the same object in multiple places for whatever reason would not
> be a cause for reporting an error in this test, I would think.
No, for the reasons I said in the commit message: if an object exists in
multiple places the test is already potentially invalid, as Git does not
promise which version it will use. So it might work racily, or it might
work for now but be fragile. By not de-duplicating, we make sure the
test's assumption holds.
> One more thing: We can do the work of the first awk invocation in the
> already existing loop as well:
> [...]
> ... but the substitutions are a bit awkward:
>
> find .git/objects/?? -type f |
> while read path
> do
> basename=${path##*/} &&
> dirname=${path%/$basename} &&
> oid="${dirname#.git/objects/}${basename}" &&
> size=$(test_file_size "$path") &&
> echo "$oid $size" ||
> return 1
> done &&
>
> The avoided awk invocation might be worth the trouble, though.
Yeah, I briefly considered whether it would be possible in pure shell,
but didn't get very far before assuming it was going to be ugly. Thank
you for confirming. ;)
Again, if we were doing one awk per object, I'd try to avoid it. But
since we can cover all objects in a single pass, I think it's OK.
-Peff
^ permalink raw reply
* Re: [PATCH v2 7/9] SubmittingPatches: clarify GitHub visual
From: Junio C Hamano @ 2023-12-21 21:27 UTC (permalink / raw)
To: Josh Soref via GitGitGadget
Cc: git, Elijah Newren, René Scharfe, Phillip Wood, Josh Soref
In-Reply-To: <cdb5fd0957fee7ce8c19720f588da96898cd3dc9.1703176866.git.gitgitgadget@gmail.com>
"Josh Soref via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Josh Soref <jsoref@gmail.com>
>
> GitHub has two general forms for its states, sometimes they're a simple
> colored object (e.g. green check or red x), and sometimes there's also a
> colored container (e.g. green box or red circle) with containing that
> object (e.g. check or x).
>
> That's a lot of words to try to describe things, but in general, the key
> for a failure is that it's recognized as an `x` and that it's associated
> with the color red -- the color of course is problematic for people who
> are red-green color-blind, but that's why they are paired with distinct
> shapes.
>
> Using the term `cross` doesn't really help.
I am not sure if this is accurate. Using `x` alone does not help,
either.
I think this was raised during the review of the initial round, but
...
> If a branch did not pass all test cases then it is marked with a red
> -cross. In that case you can click on the failing job and navigate to
> ++x+. In that case you can click on the failing job and navigate to
... it would help if we added something like ", instead of a green
checkmark" after "with a red x". It will make the contrast with the
succeeding case stronger. IOW, we can take advantage of the idea to
use "pair with distinct shapes and colors" ourselves.
> "ci/run-build-and-tests.sh" and/or "ci/print-test-failures.sh". You
> can also download "Artifacts" which are tarred (or zipped) archives
> with test data relevant for debugging.
^ permalink raw reply
* Re: [PATCH v2 6/9] SubmittingPatches: improve extra tags advice
From: Junio C Hamano @ 2023-12-21 21:18 UTC (permalink / raw)
To: Josh Soref via GitGitGadget
Cc: git, Elijah Newren, René Scharfe, Phillip Wood, Josh Soref
In-Reply-To: <8f16c7caa7366cab22ad332c402f80823add8224.1703176866.git.gitgitgadget@gmail.com>
"Josh Soref via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Josh Soref <jsoref@gmail.com>
>
> Current statistics show a strong preference to only capitalize the first
> letter in a hyphenated tag, but that some guidance would be helpful:
>
> git log |
> perl -ne 'next unless /^\s+(?:Signed-[oO]ff|Acked)-[bB]y:/;
> s/^\s+//;s/:.*/:/;print'|
> sort|uniq -c|sort -n
> 2 Signed-off-By:
> 4 Signed-Off-by:
> 22 Acked-By:
> 47 Signed-Off-By:
> 2202 Acked-by:
> 95315 Signed-off-by:
>
> Signed-off-by: Josh Soref <jsoref@gmail.com>
> ---
> Documentation/SubmittingPatches | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
> index 31878cb70b7..4476b52a50f 100644
> --- a/Documentation/SubmittingPatches
> +++ b/Documentation/SubmittingPatches
> @@ -368,6 +368,9 @@ While you can also create your own trailer if the situation warrants it, we
> encourage you to instead use one of the common trailers in this project
> highlighted above.
>
> +Extra tags should only capitalize the very first letter, i.e. favor
> +"Signed-off-by" over "Signed-Off-By" and "Acked-by:" over "Acked-By".
Drop "Extra", perhaps? The sentence before already discourages any
extra ones, and what this sentence teaches the contributors is to
avoud spelling variation when to spell the common ones.
> [[git-tools]]
> === Generate your patch using Git tools out of your commits.
^ permalink raw reply
* Re: [PATCH v2 5/9] SubmittingPatches: update extra tags list
From: Junio C Hamano @ 2023-12-21 21:16 UTC (permalink / raw)
To: Josh Soref via GitGitGadget
Cc: git, Elijah Newren, René Scharfe, Phillip Wood, Josh Soref
In-Reply-To: <8848572fe2c7432ede85e042bc2558fd8b3e8b1d.1703176866.git.gitgitgadget@gmail.com>
"Josh Soref via GitGitGadget" <gitgitgadget@gmail.com> writes:
> +. `Co-authored-by:` is used to indicate that people exchanged drafts
> + of a patch before submitting it.
I am afraid this misses the essence of what Co-authoring means.
You may have shared your draft as FYI to your colleagues, and they
may have tweaked phrasing and responded with their reviews to help
you improve _your_ work, but that may not make them your Co-authors.
I think the "intent" counts more. If people closely worked
together, exchanging drafts and agreeing on the final version among
themselves before submitting, with the understanding that they share
the authorship credit/blame, they are Co-authors.
> +. `Helped-by:` is used to credit someone who suggested ideas for
> + changes without providing the precise changes in patch form.
> +. `Mentored-by:` is used to credit someone with helping develop a
> + patch as part of a mentorship program (e.g., GSoC or Outreachy).
Nicely differentiated.
^ permalink raw reply
* Re: [PATCH v2 3/9] SubmittingPatches: drop ref to "What's in git.git"
From: Junio C Hamano @ 2023-12-21 21:09 UTC (permalink / raw)
To: Josh Soref via GitGitGadget
Cc: git, Elijah Newren, René Scharfe, Phillip Wood, Josh Soref
In-Reply-To: <22d66c5b78a6930e195141df848266cac099ca08.1703176866.git.gitgitgadget@gmail.com>
"Josh Soref via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Josh Soref <jsoref@gmail.com>
>
> "What's in git.git" was last seen in 2010:
> https://lore.kernel.org/git/?q=%22what%27s+in+git.git%22
> https://lore.kernel.org/git/7vaavikg72.fsf@alter.siamese.dyndns.org/
>
> Signed-off-by: Josh Soref <jsoref@gmail.com>
> ---
> Documentation/SubmittingPatches | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Thanks. I stopped doing these long time ago, as there were certain
overlaps with "What's cooking" that lists the topics that have
graduated in a separate section.
>
> diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
> index bce7f97815c..32e90238777 100644
> --- a/Documentation/SubmittingPatches
> +++ b/Documentation/SubmittingPatches
> @@ -570,7 +570,7 @@ their trees themselves.
> master).
>
> * Read the Git mailing list, the maintainer regularly posts messages
> - entitled "What's cooking in git.git" and "What's in git.git" giving
> + entitled "What's cooking in git.git" giving
> the status of various proposed changes.
>
> == GitHub CI[[GHCI]]
^ permalink raw reply
* Re: [PATCH v2 1/9] CodingGuidelines: move period inside parentheses
From: Junio C Hamano @ 2023-12-21 21:03 UTC (permalink / raw)
To: Josh Soref via GitGitGadget
Cc: git, Elijah Newren, René Scharfe, Phillip Wood, Josh Soref
In-Reply-To: <b9a8eb6aa4e87cd96fbf2b5d514350508076d756.1703176866.git.gitgitgadget@gmail.com>
"Josh Soref via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Josh Soref <jsoref@gmail.com>
>
> The contents within parenthesis should be omittable without resulting
> in broken text.
>
> Eliding the parenthesis left a period to end a run without any content.
Nobody seems to have commented on this one in the previous round,
but quite honestly, for this particular instance, I suspect that it
may become easier to read if we just lost these parentheses, as the
next sentence "You do not have to include more than ..." is just a
bit of extra material to support the first sentence like the one we
see in the parentheses. Alternatively, moving that "You do not have
to" also inside the same parentheses might work slightly better.
It might be even easier to follow if we moved the list of "approved
headers" (and the "You do not have to ... more than one" note that
supports the "currently approved list") totally out of line by
making it a side note.
> Signed-off-by: Josh Soref <jsoref@gmail.com>
> ---
> Documentation/CodingGuidelines | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
> index 8ed517a5ca0..af94ed3a75d 100644
> --- a/Documentation/CodingGuidelines
> +++ b/Documentation/CodingGuidelines
> @@ -450,7 +450,7 @@ For C programs:
> one of the approved headers that includes it first for you. (The
> approved headers currently include "builtin.h",
> "t/helper/test-tool.h", "xdiff/xinclude.h", or
> - "reftable/system.h"). You do not have to include more than one of
> + "reftable/system.h".) You do not have to include more than one of
> these.
>
> - A C file must directly include the header files that declare the
^ permalink raw reply
* rebase invoking pre-commit
From: Sean Allred @ 2023-12-21 20:58 UTC (permalink / raw)
To: Git List
Is there a current reason why pre-commit shouldn't be invoked during
rebase, or is this just waiting for a reviewable patch?
This was brought up before at [1] in 2015, but that thread so old at
this point that it seemed prudent to double-check before investing time
in a developing and testing a patch.
[1]: https://lore.kernel.org/git/1m55i3m.1fum4zo1fpnhncM%25lists@haller-berlin.de/
--
Sean Allred
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox