* [PATCH v5] tests: move t0009-prio-queue.sh to the new unit testing framework
From: Chandra Pratap via GitGitGadget @ 2024-01-25 20:02 UTC (permalink / raw)
To: git; +Cc: Chandra Pratap, Jeff King, Chandra Pratap, Chandra Pratap
In-Reply-To: <pull.1642.v4.git.1705865326185.gitgitgadget@gmail.com>
From: Chandra Pratap <chandrapratap3519@gmail.com>
t/t0009-prio-queue.sh along with t/helper/test-prio-queue.c unit
tests Git's implementation of a priority queue. Migrate the
test over to the new unit testing framework to simplify debugging
and reduce test run-time. Refactor the required logic and add
a new test case in addition to porting over the original ones in
shell.
Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
---
tests: move t0009-prio-queue.sh to the new unit testing framework
Thanks for the feedbacks! I have updated the patch so that the resulting
tests show something like:
> check "result[j++] == show(get)" failed at
> unit-tests/t-prio-queue.c:60
> left: 20 right: 2
> input: push 1, push 2, get, get, get, push 1, push 2, get, get, get,
> failed at input index 8
in the case of a failing test.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1642%2FChand-ra%2Fprio-queue-v5
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1642/Chand-ra/prio-queue-v5
Pull-Request: https://github.com/gitgitgadget/git/pull/1642
Range-diff vs v4:
1: e9a0dba6f47 ! 1: 88e70127ad3 tests: move t0009-prio-queue.sh to the new unit testing framework
@@ t/unit-tests/t-prio-queue.c (new)
+ return v ? *v : MISSING;
+}
+
++static void print_input(int *input, size_t input_size)
++{
++ char buf[128];
++ int len = 0;
++ for(size_t i = 0; i < input_size; i++) {
++ switch (input[i]) {
++ case GET:
++ len += xsnprintf(buf+len, sizeof(buf), "get, ");
++ break;
++ case DUMP:
++ len += xsnprintf(buf+len, sizeof(buf), "dump");
++ break;
++ case STACK:
++ len += xsnprintf(buf+len, sizeof(buf), "stack, ");
++ break;
++ case REVERSE:
++ len += xsnprintf(buf+len, sizeof(buf), "reverse, ");
++ break;
++ default:
++ len += xsnprintf(buf+len, sizeof(buf), "push %d, ", input[i]);
++ break;
++ }
++ }
++ test_msg("input: %s", buf);
++}
++
+static void test_prio_queue(int *input, int *result, size_t input_size)
+{
+ struct prio_queue pq = { intcmp };
+
+ for (int i = 0, j = 0; i < input_size; i++) {
+ void *peek, *get;
-+ switch(input[i]) {
++ switch (input[i]) {
+ case GET:
+ peek = prio_queue_peek(&pq);
+ get = prio_queue_get(&pq);
+ if (!check(peek == get))
+ return;
-+ if(!check_int(result[j++], ==, show(get)))
-+ test_msg("failed at result[] index %d", j-1);
++ if (!check_int(result[j++], ==, show(get))) {
++ print_input(input, input_size);
++ test_msg("failed at input index %d", i);
++ }
+ break;
+ case DUMP:
+ while ((peek = prio_queue_peek(&pq))) {
+ get = prio_queue_get(&pq);
+ if (!check(peek == get))
+ return;
-+ if(!check_int(result[j++], ==, show(get)))
-+ test_msg("failed at result[] index %d", j-1);
++ if (!check_int(result[j++], ==, show(get))) {
++ print_input(input, input_size);
++ test_msg("failed at input index %d", i);
++ }
+ }
+ break;
+ case STACK:
@@ t/unit-tests/t-prio-queue.c (new)
+ clear_prio_queue(&pq);
+}
+
++
+#define BASIC_INPUT 2, 6, 3, 10, 9, 5, 7, 4, 5, 8, 1, DUMP
+#define BASIC_RESULT 1, 2, 3, 4, 5, 5, 6, 7, 8, 9, 10
+
Makefile | 2 +-
t/helper/test-prio-queue.c | 51 --------------
t/helper/test-tool.c | 1 -
t/helper/test-tool.h | 1 -
t/t0009-prio-queue.sh | 66 ------------------
t/unit-tests/t-prio-queue.c | 129 ++++++++++++++++++++++++++++++++++++
6 files changed, 130 insertions(+), 120 deletions(-)
delete mode 100644 t/helper/test-prio-queue.c
delete mode 100755 t/t0009-prio-queue.sh
create mode 100644 t/unit-tests/t-prio-queue.c
diff --git a/Makefile b/Makefile
index 312d95084c1..d5e48e656b3 100644
--- a/Makefile
+++ b/Makefile
@@ -828,7 +828,6 @@ TEST_BUILTINS_OBJS += test-partial-clone.o
TEST_BUILTINS_OBJS += test-path-utils.o
TEST_BUILTINS_OBJS += test-pcre2-config.o
TEST_BUILTINS_OBJS += test-pkt-line.o
-TEST_BUILTINS_OBJS += test-prio-queue.o
TEST_BUILTINS_OBJS += test-proc-receive.o
TEST_BUILTINS_OBJS += test-progress.o
TEST_BUILTINS_OBJS += test-reach.o
@@ -1340,6 +1339,7 @@ THIRD_PARTY_SOURCES += sha1dc/%
UNIT_TEST_PROGRAMS += t-basic
UNIT_TEST_PROGRAMS += t-strbuf
+UNIT_TEST_PROGRAMS += t-prio-queue
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-prio-queue.c b/t/helper/test-prio-queue.c
deleted file mode 100644
index f0bf255f5f0..00000000000
--- a/t/helper/test-prio-queue.c
+++ /dev/null
@@ -1,51 +0,0 @@
-#include "test-tool.h"
-#include "prio-queue.h"
-
-static int intcmp(const void *va, const void *vb, void *data UNUSED)
-{
- const int *a = va, *b = vb;
- return *a - *b;
-}
-
-static void show(int *v)
-{
- if (!v)
- printf("NULL\n");
- else
- printf("%d\n", *v);
- free(v);
-}
-
-int cmd__prio_queue(int argc UNUSED, const char **argv)
-{
- struct prio_queue pq = { intcmp };
-
- while (*++argv) {
- if (!strcmp(*argv, "get")) {
- void *peek = prio_queue_peek(&pq);
- void *get = prio_queue_get(&pq);
- if (peek != get)
- BUG("peek and get results do not match");
- show(get);
- } else if (!strcmp(*argv, "dump")) {
- void *peek;
- void *get;
- while ((peek = prio_queue_peek(&pq))) {
- get = prio_queue_get(&pq);
- if (peek != get)
- BUG("peek and get results do not match");
- show(get);
- }
- } else if (!strcmp(*argv, "stack")) {
- pq.compare = NULL;
- } else {
- int *v = xmalloc(sizeof(*v));
- *v = atoi(*argv);
- prio_queue_put(&pq, v);
- }
- }
-
- clear_prio_queue(&pq);
-
- return 0;
-}
diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c
index 876cd2dc313..5f591b9718f 100644
--- a/t/helper/test-tool.c
+++ b/t/helper/test-tool.c
@@ -57,7 +57,6 @@ static struct test_cmd cmds[] = {
{ "path-utils", cmd__path_utils },
{ "pcre2-config", cmd__pcre2_config },
{ "pkt-line", cmd__pkt_line },
- { "prio-queue", cmd__prio_queue },
{ "proc-receive", cmd__proc_receive },
{ "progress", cmd__progress },
{ "reach", cmd__reach },
diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h
index 70dd4eba119..b921138d8ec 100644
--- a/t/helper/test-tool.h
+++ b/t/helper/test-tool.h
@@ -50,7 +50,6 @@ int cmd__partial_clone(int argc, const char **argv);
int cmd__path_utils(int argc, const char **argv);
int cmd__pcre2_config(int argc, const char **argv);
int cmd__pkt_line(int argc, const char **argv);
-int cmd__prio_queue(int argc, const char **argv);
int cmd__proc_receive(int argc, const char **argv);
int cmd__progress(int argc, const char **argv);
int cmd__reach(int argc, const char **argv);
diff --git a/t/t0009-prio-queue.sh b/t/t0009-prio-queue.sh
deleted file mode 100755
index eea99107a48..00000000000
--- a/t/t0009-prio-queue.sh
+++ /dev/null
@@ -1,66 +0,0 @@
-#!/bin/sh
-
-test_description='basic tests for priority queue implementation'
-
-TEST_PASSES_SANITIZE_LEAK=true
-. ./test-lib.sh
-
-cat >expect <<'EOF'
-1
-2
-3
-4
-5
-5
-6
-7
-8
-9
-10
-EOF
-test_expect_success 'basic ordering' '
- test-tool prio-queue 2 6 3 10 9 5 7 4 5 8 1 dump >actual &&
- test_cmp expect actual
-'
-
-cat >expect <<'EOF'
-2
-3
-4
-1
-5
-6
-EOF
-test_expect_success 'mixed put and get' '
- test-tool prio-queue 6 2 4 get 5 3 get get 1 dump >actual &&
- test_cmp expect actual
-'
-
-cat >expect <<'EOF'
-1
-2
-NULL
-1
-2
-NULL
-EOF
-test_expect_success 'notice empty queue' '
- test-tool prio-queue 1 2 get get get 1 2 get get get >actual &&
- test_cmp expect actual
-'
-
-cat >expect <<'EOF'
-3
-2
-6
-4
-5
-1
-8
-EOF
-test_expect_success 'stack order' '
- test-tool prio-queue stack 8 1 5 4 6 2 3 dump >actual &&
- test_cmp expect actual
-'
-
-test_done
diff --git a/t/unit-tests/t-prio-queue.c b/t/unit-tests/t-prio-queue.c
new file mode 100644
index 00000000000..8456b23255c
--- /dev/null
+++ b/t/unit-tests/t-prio-queue.c
@@ -0,0 +1,129 @@
+#include "test-lib.h"
+#include "prio-queue.h"
+
+static int intcmp(const void *va, const void *vb, void *data UNUSED)
+{
+ const int *a = va, *b = vb;
+ return *a - *b;
+}
+
+
+#define MISSING -1
+#define DUMP -2
+#define STACK -3
+#define GET -4
+#define REVERSE -5
+
+static int show(int *v)
+{
+ return v ? *v : MISSING;
+}
+
+static void print_input(int *input, size_t input_size)
+{
+ char buf[128];
+ int len = 0;
+ for(size_t i = 0; i < input_size; i++) {
+ switch (input[i]) {
+ case GET:
+ len += xsnprintf(buf+len, sizeof(buf), "get, ");
+ break;
+ case DUMP:
+ len += xsnprintf(buf+len, sizeof(buf), "dump");
+ break;
+ case STACK:
+ len += xsnprintf(buf+len, sizeof(buf), "stack, ");
+ break;
+ case REVERSE:
+ len += xsnprintf(buf+len, sizeof(buf), "reverse, ");
+ break;
+ default:
+ len += xsnprintf(buf+len, sizeof(buf), "push %d, ", input[i]);
+ break;
+ }
+ }
+ test_msg("input: %s", buf);
+}
+
+static void test_prio_queue(int *input, int *result, size_t input_size)
+{
+ struct prio_queue pq = { intcmp };
+
+ for (int i = 0, j = 0; i < input_size; i++) {
+ void *peek, *get;
+ switch (input[i]) {
+ case GET:
+ peek = prio_queue_peek(&pq);
+ get = prio_queue_get(&pq);
+ if (!check(peek == get))
+ return;
+ if (!check_int(result[j++], ==, show(get))) {
+ print_input(input, input_size);
+ test_msg("failed at input index %d", i);
+ }
+ break;
+ case DUMP:
+ while ((peek = prio_queue_peek(&pq))) {
+ get = prio_queue_get(&pq);
+ if (!check(peek == get))
+ return;
+ if (!check_int(result[j++], ==, show(get))) {
+ print_input(input, input_size);
+ test_msg("failed at input index %d", i);
+ }
+ }
+ break;
+ case STACK:
+ pq.compare = NULL;
+ break;
+ case REVERSE:
+ prio_queue_reverse(&pq);
+ break;
+ default:
+ prio_queue_put(&pq, &input[i]);
+ break;
+ }
+ }
+ clear_prio_queue(&pq);
+}
+
+
+#define BASIC_INPUT 2, 6, 3, 10, 9, 5, 7, 4, 5, 8, 1, DUMP
+#define BASIC_RESULT 1, 2, 3, 4, 5, 5, 6, 7, 8, 9, 10
+
+#define MIXED_PUT_GET_INPUT 6, 2, 4, GET, 5, 3, GET, GET, 1, DUMP
+#define MIXED_PUT_GET_RESULT 2, 3, 4, 1, 5, 6
+
+#define EMPTY_QUEUE_INPUT 1, 2, GET, GET, GET, 1, 2, GET, GET, GET
+#define EMPTY_QUEUE_RESULT 1, 2, MISSING, 1, 2, MISSING
+
+#define STACK_INPUT STACK, 8, 1, 5, 4, 6, 2, 3, DUMP
+#define STACK_RESULT 3, 2, 6, 4, 5, 1, 8
+
+#define REVERSE_STACK_INPUT STACK, 1, 2, 3, 4, 5, 6, REVERSE, DUMP
+#define REVERSE_STACK_RESULT 1, 2, 3, 4, 5, 6
+
+#define TEST_INPUT(INPUT, RESULT, name) \
+ static void test_##name(void) \
+{ \
+ int input[] = {INPUT}; \
+ int result[] = {RESULT}; \
+ test_prio_queue(input, result, ARRAY_SIZE(input)); \
+}
+
+TEST_INPUT(BASIC_INPUT, BASIC_RESULT, basic)
+TEST_INPUT(MIXED_PUT_GET_INPUT, MIXED_PUT_GET_RESULT, mixed)
+TEST_INPUT(EMPTY_QUEUE_INPUT, EMPTY_QUEUE_RESULT, empty)
+TEST_INPUT(STACK_INPUT, STACK_RESULT, stack)
+TEST_INPUT(REVERSE_STACK_INPUT, REVERSE_STACK_RESULT, reverse)
+
+int cmd_main(int argc, const char **argv)
+{
+ TEST(test_basic(), "prio-queue works for basic input");
+ TEST(test_mixed(), "prio-queue works for mixed put & get commands");
+ TEST(test_empty(), "prio-queue works when queue is empty");
+ TEST(test_stack(), "prio-queue works when used as a LIFO stack");
+ TEST(test_reverse(), "prio-queue works when LIFO stack is reversed");
+
+ return test_done();
+}
base-commit: 1a87c842ece327d03d08096395969aca5e0a6996
--
gitgitgadget
^ permalink raw reply related
* Re: [PATCH 06/10] trailer: make trailer_info struct private
From: Josh Steadmon @ 2024-01-25 19:35 UTC (permalink / raw)
To: Linus Arver via GitGitGadget
Cc: git, Emily Shaffer, Junio C Hamano, Christian Couder, Linus Arver
In-Reply-To: <0cbe96421c7bf573e8ddc97b2a0aecc894095399.1704869487.git.gitgitgadget@gmail.com>
On 2024.01.10 06:51, Linus Arver via GitGitGadget wrote:
> From: Linus Arver <linusa@google.com>
>
> In 13211ae23f (trailer: separate public from internal portion of
> trailer_iterator, 2023-09-09) we moved trailer_info behind an anonymous
> struct to discourage use by trailer.h API users. However it still left
> open the possibility of external use of trailer_info itself. Now that
> there are no external users of trailer_info, we can make this struct
> private.
>
> Make this struct private by putting its definition inside trailer.c.
> This has two benefits:
>
> (1) it makes the surface area of the public facing interface (trailer.h)
> smaller, and
>
> (2) external API users are unable to peer inside this struct (because it
> is only ever exposed as an opaque pointer).
>
> This change exposes some deficiencies in the API, mainly with regard to
> information about the location of the trailer block that was parsed.
> Expose new API functions to access this information (needed by
> builtin/interpret-trailers.c).
>
> The idea in this patch to hide implementation details behind an "opaque
> pointer" is also known as the "pimpl" (pointer to implementation) idiom
> in C++ and is a common pattern in that language (where, for example,
> abstract classes only have pointers to concrete classes).
>
> However, the original inspiration to use this idiom does not come from
> C++, but instead the book "C Interfaces and Implementations: Techniques
> for Creating Reusable Software" [1]. This book recommends opaque
> pointers as a good design principle for designing C libraries, using the
> term "interface" as the functions defined in *.h (header) files and
> "implementation" as the corresponding *.c file which define the
> interfaces.
>
> The book says this about opaque pointers:
>
> ... clients can manipulate such pointers freely, but they can’t
> dereference them; that is, they can’t look at the innards of the
> structure pointed to by them. Only the implementation has that
> privilege. Opaque pointers hide representation details and help
> catch errors.
>
> In our case, "struct trailer_info" is now hidden from clients, and the
> ways in which this opaque pointer can be used is limited to the richness
> of the trailer.h file. In other words, trailer.h exclusively controls
> exactly how "trailer_info" pointers are to be used.
>
> [1] Hanson, David R. "C Interfaces and Implementations: Techniques for
> Creating Reusable Software". Addison Wesley, 1997. p. 22
>
> Signed-off-by: Linus Arver <linusa@google.com>
> ---
> builtin/interpret-trailers.c | 13 +--
> trailer.c | 154 +++++++++++++++++++++++------------
> trailer.h | 37 ++-------
> 3 files changed, 117 insertions(+), 87 deletions(-)
Looks like a pretty straightforward change. I think the only point worth
discussing is using the "pimpl" idiom. I think it's harder to see the
value because the trailer_info struct is fairly simple, but I can
definitely see this pattern being useful as we libify more complex parts
of Git where the struct internals have more complicated logic involved.
The pimpl pattern also seems like it will force us to think harder about
providing a useful interface, so I am in favor of using it here as
practice for future libification.
^ permalink raw reply
* Re: [PATCH v6] subtree: fix split processing with multiple subtrees present
From: Junio C Hamano @ 2024-01-25 18:56 UTC (permalink / raw)
To: Christian Couder
Cc: Zach FettersMoore, Zach FettersMoore via GitGitGadget, git
In-Reply-To: <CAP8UFD2Oo8v8Qn0JPYURZA_s7ynZmk6v30b9zR==MxWBTXk9Ng@mail.gmail.com>
Christian Couder <christian.couder@gmail.com> writes:
> On Thu, Jan 25, 2024 at 5:38 PM Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Christian Couder <christian.couder@gmail.com> writes:
>>
>> > It seems that this topic has fallen into the cracks or something,
>> > while the associated pch looks good to me.
>>
>> Yeah, it wasn't clear to me that your message you are responding to
>> was your Reviewed-by:. If I recall my impression correctly from the
>> time I skimmed its proposed log message the last time, it focused on
>> describing a single failure case the author encountered in the real
>> world and said that the patch changed the behaviour to correct that
>> single case, and was not very clear if it was meant as a general
>> fix. Is the patch text, including its proposed patch description,
>> satisfactory to you? In other words, is the above your Reviewed-by:?
>
> Yes, it's satisfactory for me, and I am Ok to give my Reviewed-by:, thanks!
Thanks. Will queue.
^ permalink raw reply
* Re: [PATCH] ls-files: avoid the verb "deprecate" for individual options
From: Junio C Hamano @ 2024-01-25 18:55 UTC (permalink / raw)
To: Jeff King
Cc: git, Elijah Newren, Raúl Núñez de Arenas Coronado
In-Reply-To: <20240125180625.GA54488@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Wed, Jan 24, 2024 at 01:10:31PM -0800, Junio C Hamano wrote:
>
>> --exclude-per-directory=<file>::
>> Read additional exclude patterns that apply only to the
>> - directory and its subdirectories in <file>. Deprecated; use
>> - --exclude-standard instead.
>> + directory and its subdirectories in <file>. If you are
>> + trying to emulate the way Porcelain commands work, using
>> + the `--exclude-standard` instead is easier and more
>> + thorough.
>
> Sorry I missed this gramm-o before, but should be "the --exclude-standard
> option" or just "--exclude-standard".
Oops, certainly.
^ permalink raw reply
* Re: partial commit of file-to-directory change, was Re: Bugreport
From: Junio C Hamano @ 2024-01-25 18:54 UTC (permalink / raw)
To: Jeff King; +Cc: brian m. carlson, Frank Schwidom, git
In-Reply-To: <20240120004628.GA117170@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Fri, Jan 19, 2024 at 11:14:47PM +0000, brian m. carlson wrote:
>
>> > $ git commit . -m +
>> > error: 'd' does not have a commit checked out
>> > fatal: updating files failed
>>
>> I can confirm this behaviour[0], and it's definitely wrong that it
>> thinks `d` is a submodule. It does, however, work if you do `git commit
>> -m +` (that is, without the .), which makes sense since the relevant
>> change is already staged in the index.
>>
>> I'm not going to get to a patch or more thorough investigation, but
>> perhaps someone else will.
>
> I peeked at this a bit earlier; I didn't come up with a solution, but
> here are a few more details.
I didn't either X-<, and the thing is somewhat nasty, as there are two
states (HEAD and the index) involved.
* For paths that do not match the pathspec, we want to take the
version from the HEAD. For paths that do match the pathspec, we
want to take the version from the working tree. And after making
the partial commit with such changes, we want to make the same
change to the index to prepare for the next commit.
* With the way the code is currently structured, we find paths that
appear either in the index or in the HEAD to match against the
pathspec. This is so that we can honor an earlier "git rm" since
we read HEAD.
* Then we check each of these paths that are either in the index or
in HEAD that matched the pathspec. If it is missing from the
working tree, we would remove it from both the temporary ndex
used for the partial commit, and the real index that we'll
continue to use after the partial commit. If it exists in the
working tree, we would take the contents of it to update.
For the purpose of this operation, a path D that was a blob in
the index should be _removed_ when it is a directory in the
working tree (i.e. made room to create D/F). And we should not
add D/F when running "git commit D" or "git commit D/F", because
the partial commit does not "git add" (it only does "git add -u")
In the example in the discussion, we had D that was a blob and got
replaced with a directory. If we did "git add -u D", we would just
have removed D from the index, so the partial commit should do the
same. Which means we need to know the type of the thing we expect.
But list_paths() only returns a list of path names, and does not
give us the type of the thing we expect. We use the same list
twice, once to update the temporary index (which we create by
reading HEAD) and update the paths listed there from the working
tree. And the same list is used again to update the real index
(which is what the user had before starting the command) and update
the paths listed there from the working tree. The tricky part is
that the type of (or even existence of) D may have changed between
the HEAD and the index, but we want to perform the same operation to
the real index with respect to the paths we touched to create the
partial commit.
Naively, add_remove_files() looks like an obvious place to see if
the path is in the working tree (which we already do) *and* compare
it with "the type of the thing we expect", but the thing is that
this function is called twice with different index (once with the
temporary index that started from HEAD, i.e. oblivious to what the
user did to the index since HEAD; then again with the real index
with the changes the user made since HEAD), so we cannot just say
"let's see what this path D is in the index".
I _think_ we would need to update list_paths() so that it records a
bit more than just pathnames that match the pathspec, namely, what
should be done to the path, by doing lstat() on the working tree
paths.
^ permalink raw reply
* Re: [PATCH v6] subtree: fix split processing with multiple subtrees present
From: Christian Couder @ 2024-01-25 18:52 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Zach FettersMoore, Zach FettersMoore via GitGitGadget, git
In-Reply-To: <xmqq5xzhxta3.fsf@gitster.g>
On Thu, Jan 25, 2024 at 5:38 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Christian Couder <christian.couder@gmail.com> writes:
>
> > It seems that this topic has fallen into the cracks or something,
> > while the associated pch looks good to me.
>
> Yeah, it wasn't clear to me that your message you are responding to
> was your Reviewed-by:. If I recall my impression correctly from the
> time I skimmed its proposed log message the last time, it focused on
> describing a single failure case the author encountered in the real
> world and said that the patch changed the behaviour to correct that
> single case, and was not very clear if it was meant as a general
> fix. Is the patch text, including its proposed patch description,
> satisfactory to you? In other words, is the above your Reviewed-by:?
Yes, it's satisfactory for me, and I am Ok to give my Reviewed-by:, thanks!
^ permalink raw reply
* Re: Slightly embarassing bug in error message when RSA key doesn't match.
From: Jeff King @ 2024-01-25 18:12 UTC (permalink / raw)
To: Peter da Silva; +Cc: git
In-Reply-To: <CAMbH6s90tNo1Kz3xuv741LoUkN=Y3Bcw2=7yGQhEe=xrgBgQcw@mail.gmail.com>
On Thu, Jan 25, 2024 at 10:37:15AM -0600, Peter da Silva wrote:
> Warning: the ECDSA host key for 'github.com' differs from the key for
> the IP address '140.82.113.3'
> Offending key for IP in /[...]/.ssh/known_hosts:8
> Matching host key in /[...]/.ssh/known_hosts:18
>
> Of course it was the RSA key that had changed, and known_hosts:8 was
> an RSA key, but the error said the ECDSA key was wrong. After removing
> the bogus RSA key from the file it worked fine with the matching ECDSA
> key. This error should probably report the correct type for the key
> that is wrong rather than the key it’s trying to use. :)
This comes from your ssh client, not Git itself (of course Git is
calling the ssh client under the hood, but we can't fix their messages).
-Peff
^ permalink raw reply
* Re: [PATCH] ls-files: avoid the verb "deprecate" for individual options
From: Jeff King @ 2024-01-25 18:06 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Elijah Newren, Raúl Núñez de Arenas Coronado
In-Reply-To: <xmqqjznybfp4.fsf@gitster.g>
On Wed, Jan 24, 2024 at 01:10:31PM -0800, Junio C Hamano wrote:
> --exclude-per-directory=<file>::
> Read additional exclude patterns that apply only to the
> - directory and its subdirectories in <file>. Deprecated; use
> - --exclude-standard instead.
> + directory and its subdirectories in <file>. If you are
> + trying to emulate the way Porcelain commands work, using
> + the `--exclude-standard` instead is easier and more
> + thorough.
Sorry I missed this gramm-o before, but should be "the --exclude-standard
option" or just "--exclude-standard".
-Peff
^ permalink raw reply
* Seanpeytonnyc@gmail.com
From: John Appleseed @ 2024-01-25 18:03 UTC (permalink / raw)
To: git
Sent from my iPhone
^ permalink raw reply
* Re: what should "git clean -n -f [-d] [-x] <pattern>" do?
From: Junio C Hamano @ 2024-01-25 17:46 UTC (permalink / raw)
To: Sergey Organov; +Cc: Elijah Newren, git
In-Reply-To: <87il3h72ym.fsf@osv.gnss.ru>
Sergey Organov <sorganov@gmail.com> writes:
> Now we are going to introduce "dry run" option "-n". Most simple and
> obvious way to do it is to set internal flag "dry_run" and then at every
> invocation of "remove(file_name)" put an if(dry_run) that will just
> print(file_name) instead or removing it. Let's suppose we did just that.
> We get this behavior:
>
> $ git clean -n
> fatal: clean.requireForce defaults to true and neither -i nor -f given; refusing to clean
> $ git clean -f -n
> would remove "a"
> would remove "b"
> $ git clean -f -f -n
> would remove "a"
> would remove "b"
> would remove "sub/a"
> $
>
> I see this as logical, clean, and straightforward behavior, meeting user
> expectations for "dry run" option, so I suggest to do just that.
I think we are saying the same thing. If the original semantics
were "you must force with -f to do anything useful", instead of "you
must choose either forcing with -f or not doing with -n", then it
would have led to the above behaviour.
The thing is, it is way too late to change it that way without
breaking too many folks, and that is the problem.
^ permalink raw reply
* Re: what should "git clean -n -f [-d] [-x] <pattern>" do?
From: Sergey Organov @ 2024-01-25 17:11 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Elijah Newren, git
In-Reply-To: <xmqqa5ouhckj.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> Sergey Organov <sorganov@gmail.com> writes:
>
>> Whereas obsoleting second -f in favor of new --nested-repo might be a
>> good idea indeed, I believe it's still a mistake for "dry run" to
>> somehow interfere with -f, sorry.
>
> No need to be sorry ;-)
>
> I actually think the true culprit of making this an odd-man-out is
> that the use of "-f" in "git clean", especially with its use of the
> configuration variable clean.requireForce that defaults to true, is
> utterly non-standard.
>
> The usual pattern of defining what "-f" does is that the "git foo"
> command without any options does its common thing but refuses to
> perform undesirable operations (e.g. "git add ." adds everything
> but refrains from adding ignored paths). And "git foo -f" is a way
> to also perform what it commonly skips.
>
> In contrast, with clean.requireForce that defaults to true, "git
> clean" does not do anything useful by default. Without such a
> safety, "git clean" would be a way to clean expendable paths, and
> "git clean -f" might be to also clean precious paths. But it does
> not work that way. It always requires "-f" to do anything. Worse
> yet, it is not even "by default it acts as if -n is given and -f is
> a way to countermand that implicit -n". It is "you must give me
> either -f (i.e. please do work) or -n (i.e. please show what you
> would do) before I do anything".
>
> $ git clean
> fatal: clean.requireForce defaults to true and neither -i, -n, nor -f given; refusing to clean
>
> Given that, it is hard to argue that it would be a natural end-user
> expectation that the command does something useful (i.e. show what
> would be done) when it is given "-f" and "-n" at the same time.
> What makes this a rather nonsense UI is the fact that "-f" does not
> work the way we would expect for this command.
I think we all agree that current UI is a kind of nonsense, but have
different views of the optimal target interface. My points are as
following:
1. The fact that bare "git clean" only produces error by default is
probably a good thing, as removal of untracked files is unrecoverable
operation in Git domain, so requiring -f by default is probably a good
thing as well, provided the *only* operation that "git clean" performs
is dangerous enough.
2. The "-n" behavior is pure nonsense.
So, how do we fix (2)? Let's try mental experiment. Suppose there is no
"-n" option for "git clean" and we are going to implement it. We start
from:
$ git clean
fatal: clean.requireForce defaults to true and neither -i nor -f given; refusing to clean
$ git clean -f
removing "a"
removing "b"
$
Please notice that there is no "-n" in the error message as there is no
such option yet in our experiment.
Now we are going to introduce "dry run" option "-n". Most simple and
obvious way to do it is to set internal flag "dry_run" and then at every
invocation of "remove(file_name)" put an if(dry_run) that will just
print(file_name) instead or removing it. Let's suppose we did just that.
We get this behavior:
$ git clean -n
fatal: clean.requireForce defaults to true and neither -i nor -f given; refusing to clean
$ git clean -f -n
would remove "a"
would remove "b"
$ git clean -f -f -n
would remove "a"
would remove "b"
would remove "sub/a"
$
I see this as logical, clean, and straightforward behavior, meeting user
expectations for "dry run" option, so I suggest to do just that.
Thanks,
-- Sergey Organov.
^ permalink raw reply
* Re: [PATCH v6] subtree: fix split processing with multiple subtrees present
From: Junio C Hamano @ 2024-01-25 16:38 UTC (permalink / raw)
To: Christian Couder
Cc: Zach FettersMoore, Zach FettersMoore via GitGitGadget, git
In-Reply-To: <CAP8UFD0M_KeUTHthQ6n_a1KbEvuA1gAsE2jKkAqd-4twjbpNWw@mail.gmail.com>
Christian Couder <christian.couder@gmail.com> writes:
> It seems that this topic has fallen into the cracks or something,
> while the associated pch looks good to me.
Yeah, it wasn't clear to me that your message you are responding to
was your Reviewed-by:. If I recall my impression correctly from the
time I skimmed its proposed log message the last time, it focused on
describing a single failure case the author encountered in the real
world and said that the patch changed the behaviour to correct that
single case, and was not very clear if it was meant as a general
fix. Is the patch text, including its proposed patch description,
satisfactory to you? In other words, is the above your Reviewed-by:?
Thanks for pinging the thread.
^ permalink raw reply
* Slightly embarassing bug in error message when RSA key doesn't match.
From: Peter da Silva @ 2024-01-25 16:37 UTC (permalink / raw)
To: git
git version 2.20.1
I just went to clone a repo on github on a dev server that I hadn't
used since March 1 2023.
Github updated their RSA key on March 24th.
So I got this message.
Warning: the ECDSA host key for 'github.com' differs from the key for
the IP address '140.82.113.3'
Offending key for IP in /[...]/.ssh/known_hosts:8
Matching host key in /[...]/.ssh/known_hosts:18
Of course it was the RSA key that had changed, and known_hosts:8 was
an RSA key, but the error said the ECDSA key was wrong. After removing
the bogus RSA key from the file it worked fine with the matching ECDSA
key. This error should probably report the correct type for the key
that is wrong rather than the key it’s trying to use. :)
^ permalink raw reply
* Re: [PATCH v2 1/4] refs: introduce `is_pseudoref()` and `is_headref()`
From: Junio C Hamano @ 2024-01-25 16:28 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git, ps, phillip.wood123
In-Reply-To: <CAOLa=ZQxMZo2Y6x6GmVw=df_xS4tkF4D1_tZOeLb7jL5d5bKXA@mail.gmail.com>
Karthik Nayak <karthik.188@gmail.com> writes:
>>> +int is_headref(struct ref_store *refs, const char *refname)
>>> +{
>>> + if (!strcmp(refname, "HEAD"))
>>> + return refs_ref_exists(refs, refname);
>>
>> Given that "git for-each-ref refs/remotes" does not show
>> "refs/remotes/origin/HEAD" in the output when we do not have the
>> remote-tracking branch that symref points at, we probably do want
>> to omit "HEAD" from the output when the HEAD symref points at an
>> unborn branch. If refs_ref_exists() says "no, it does not exist"
>> in such a case, we are perfectly fine with this code.
>>
>> We do not have to worry about the unborn state for pseudorefs
>> because they would never be symbolic. But that in turn makes me
>> suspect that the check done with refs_ref_exists() in the
>> is_pseudoref() helper is a bit too lenient by allowing it to be a
>> symbolic ref. Shouldn't we be using a check based on
>> read_ref_full(), like we did in another topic recently [*]?
>>
>>
>> [Reference]
>>
>> * https://lore.kernel.org/git/xmqqzfxa9usx.fsf@gitster.g/
>>
>
> Thanks, this makes sense and the link is helpful. I'll do something
> similar, but since HEAD can be a symref, I'll drop the
> `RESOLVE_REF_NO_RECURSE` flag and only use `RESOLVE_REF_READING`.
Just to make sure there is no misunderstanding, I think how
is_headref() does what it does in the patch is perfectly fine,
including its use of refs_ref_exists(). The side I was referring to
with "in turn makes me suspect" is the other helper function that
will never have to deal with a symref. Use of refs_ref_exists() in
that function is too loose.
^ permalink raw reply
* Re: Virus detected
From: Konstantin Khomoutov @ 2024-01-25 16:21 UTC (permalink / raw)
To: git
In-Reply-To: <a96e6744-c0bb-4f6e-9066-646ca15a353d@GMX.De>
Hi!
On Thu, Jan 25, 2024 at 04:23:25PM +0100, Hr. Rüge wrote:
> trying to use windows version 2.43.0 from https://git-scm.com/downloads
> my antivirus software detects it as a virus.
I assume you're talking about Windows.
This problem is not new and has been reported and discussed numerous times
on the 'net. It has even been reported to the Git-for-Windows bug tracker -
see [1, 2, 3, 4].
The problem with A\V software is that it does not operate solely by scanning
the executable image files for having byte sequences identifying particular
infections - this approach worked in 90s but not for too long. These days,
pieces of A/V software try to _guess_ whether a particular executable is a
malware, and to do that, they "score" what they inspect; for instance, lack of
signing of some binaries lowers the score, not being presented in some
"whitelist" lowers the score and so on.
What you can do is to navigate directly to the GfW's project downloads [5]
(you might have noticed that the link to download the Windows binaries over
there at git-scm.com actually already leads there, just to some more
user-friendly web page), pick the version and make (32-bit vs 64-bit) you
need, download it and then verify its checksum [6] matches that one listed at
the download page. If the checksum is OK, you can be sure no software running
on your machine has tampered with the downloaded installer, and then the
question of whether or not the installer contains any malware is the trust in
the Git-for-Windows maintainers and the integrity of the GfW's Github project.
> In parallel something wanted to change my hosts-entries.
>
> Ist that normal?
I would say it's not normal but as usually, coinsidence of this activity with
an attempt to install GfW might not indicate any connection between the two -
other than temporal. Of course, this only stands if you have no malware
running on your host which patches the downloaded files on-the-fly.
Still, if you're not sure about this one, please ask on the dedicated GfW
mailing list [7].
1. https://github.com/git-for-windows/git/issues?q=is%3Aissue+in%3Atitle+virus
2. https://github.com/git-for-windows/git/issues?q=is%3Aissue+in%3Atitle+malware
3. https://groups.google.com/g/git-for-windows/search?q=malware
4. https://groups.google.com/g/git-for-windows/search?q=virus
5. https://github.com/git-for-windows/git/releases/
6. https://www.google.com/search?q=windows+calculate+hash+of+file
7. https://groups.google.com/g/git-for-windows/
^ permalink raw reply
* Re: [PATCH v2 1/4] refs: introduce `is_pseudoref()` and `is_headref()`
From: Karthik Nayak @ 2024-01-25 16:20 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, ps, phillip.wood123
In-Reply-To: <xmqqfrymeega.fsf@gitster.g>
[-- Attachment #1: Type: text/plain, Size: 3761 bytes --]
Hello Junio,
Junio C Hamano <gitster@pobox.com> writes:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> We cannot directly add the new syntax checks to `is_pseudoref_syntax()`
>> because the function is also used by `is_current_worktree_ref()` and
>> making it stricter to match only known pseudorefs might have unintended
>> consequences due to files like 'BISECT_START' which isn't a pseudoref
>> but sometimes contains object ID.
>
> Well described.
>
>> diff --git a/refs.c b/refs.c
>> index 20e8f1ff1f..4b6bfc66fb 100644
>> --- a/refs.c
>> +++ b/refs.c
>> @@ -859,6 +859,38 @@ static int is_pseudoref_syntax(const char *refname)
>> return 1;
>> }
>>
>> +int is_pseudoref(struct ref_store *refs, const char *refname)
>> +{
>> + static const char *const irregular_pseudorefs[] = {
>> + "AUTO_MERGE",
>> + "BISECT_EXPECTED_REV",
>> + "NOTES_MERGE_PARTIAL",
>> + "NOTES_MERGE_REF",
>> + "MERGE_AUTOSTASH"
>
> Let's end an array's initializer with a trailing comma, to help
> future patches to add entries to this array without unnecessary
> patch noise.
Sure, will add!
>> + };
>> + size_t i;
>> +
>> + if (!is_pseudoref_syntax(refname))
>> + return 0;
>> +
>> + if (ends_with(refname, "_HEAD"))
>> + return refs_ref_exists(refs, refname);
>> +
>> + for (i = 0; i < ARRAY_SIZE(irregular_pseudorefs); i++)
>> + if (!strcmp(refname, irregular_pseudorefs[i]))
>> + return refs_ref_exists(refs, refname);
>> +
>> + return 0;
>> +}
>
> The above uses refs_ref_exists() because we want these to
> successfully resolve for reading.
>
>> +int is_headref(struct ref_store *refs, const char *refname)
>> +{
>> + if (!strcmp(refname, "HEAD"))
>> + return refs_ref_exists(refs, refname);
>
> Given that "git for-each-ref refs/remotes" does not show
> "refs/remotes/origin/HEAD" in the output when we do not have the
> remote-tracking branch that symref points at, we probably do want
> to omit "HEAD" from the output when the HEAD symref points at an
> unborn branch. If refs_ref_exists() says "no, it does not exist"
> in such a case, we are perfectly fine with this code.
>
> We do not have to worry about the unborn state for pseudorefs
> because they would never be symbolic. But that in turn makes me
> suspect that the check done with refs_ref_exists() in the
> is_pseudoref() helper is a bit too lenient by allowing it to be a
> symbolic ref. Shouldn't we be using a check based on
> read_ref_full(), like we did in another topic recently [*]?
>
>
> [Reference]
>
> * https://lore.kernel.org/git/xmqqzfxa9usx.fsf@gitster.g/
>
Thanks, this makes sense and the link is helpful. I'll do something
similar, but since HEAD can be a symref, I'll drop the
`RESOLVE_REF_NO_RECURSE` flag and only use `RESOLVE_REF_READING`.
I'll wait a day or two, before sending in the new version with the
fixes. The current diff is
diff --git a/refs.c b/refs.c
index b5e63f133a..4a1fd30ef2 100644
--- a/refs.c
+++ b/refs.c
@@ -866,7 +866,7 @@ int is_pseudoref(struct ref_store *refs, const
char *refname)
"BISECT_EXPECTED_REV",
"NOTES_MERGE_PARTIAL",
"NOTES_MERGE_REF",
- "MERGE_AUTOSTASH"
+ "MERGE_AUTOSTASH",
};
size_t i;
@@ -885,10 +885,23 @@ int is_pseudoref(struct ref_store *refs, const
char *refname)
int is_headref(struct ref_store *refs, const char *refname)
{
- if (!strcmp(refname, "HEAD"))
- return refs_ref_exists(refs, refname);
+ struct object_id oid;
+ int flag;
- return 0;
+ if (strcmp(refname, "HEAD"))
+ return 0;
+
+ /*
+ * If HEAD doesn't exist, we don't have to die, but rather,
+ * we simply return 0.
+ */
+ if (read_ref_full("HEAD", RESOLVE_REF_READING, &oid, &flag))
+ return 0;
+
+ if (is_null_oid(&oid))
+ return 0;
+
+ return 1;
}
static int is_current_worktree_ref(const char *ref) {
- Karthik
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]
^ permalink raw reply related
* Re: [PATCH v2] reftable/stack: adjust permissions of compacted tables
From: Justin Tobler @ 2024-01-25 16:02 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git
In-Reply-To: <b2fb6f5ad0c558527341bd8040544d6b0ae5d8a2.1706100744.git.ps@pks.im>
On Wed, Jan 24, 2024 at 7:21 AM Patrick Steinhardt <ps@pks.im> wrote:
>
> diff --git a/reftable/stack_test.c b/reftable/stack_test.c
> index 289e902146..2e7d1768b7 100644
> --- a/reftable/stack_test.c
> +++ b/reftable/stack_test.c
> @@ -443,15 +443,16 @@ static void test_reftable_stack_add(void)
> int err = 0;
> struct reftable_write_options cfg = {
> .exact_log_message = 1,
> + .default_permissions = 0660,
> };
> struct reftable_stack *st = NULL;
> char *dir = get_tmp_dir(__LINE__);
> -
> struct reftable_ref_record refs[2] = { { NULL } };
> struct reftable_log_record logs[2] = { { NULL } };
> + struct strbuf scratch = STRBUF_INIT;
The variable name `scratch` seems rather vague to me as opposed to something
like `path`. After a quick search though, `scratch` appears to be a fairly
common name used in similar scenarios. So probably not a big deal, but
something
I thought I'd mention.
> + struct stat stat_result;
> int N = ARRAY_SIZE(refs);
>
> -
> err = reftable_new_stack(&st, dir, cfg);
> EXPECT_ERR(err);
> st->disable_auto_compact = 1;
> @@ -509,12 +510,32 @@ static void test_reftable_stack_add(void)
> reftable_log_record_release(&dest);
> }
>
> +#ifndef GIT_WINDOWS_NATIVE
> + strbuf_addstr(&scratch, dir);
> + strbuf_addstr(&scratch, "/tables.list");
> + err = stat(scratch.buf, &stat_result);
> + EXPECT(!err);
> + EXPECT((stat_result.st_mode & 0777) == cfg.default_permissions);
> +
> + strbuf_reset(&scratch);
> + strbuf_addstr(&scratch, dir);
> + strbuf_addstr(&scratch, "/");
> + /* do not try at home; not an external API for reftable. */
> + strbuf_addstr(&scratch, st->readers[0]->name);
> + err = stat(scratch.buf, &stat_result);
> + EXPECT(!err);
> + EXPECT((stat_result.st_mode & 0777) == cfg.default_permissions);
> +#else
> + (void) stat_result;
> +#endif
Why do we ignore Windows here? And would it warrant explaining in the commit
message?
-Justin
^ permalink raw reply
* P. S. Virus detected
From: Hr. Rüge @ 2024-01-25 15:46 UTC (permalink / raw)
To: git
In-Reply-To: <a96e6744-c0bb-4f6e-9066-646ca15a353d@GMX.De>
Tried a few versions:
First version WITHOUT detected virus (avira.com) is version 2.41.0.3.
All newer versions lead to virus detections.
Best regards,
Martin
Am 25.01.2024 um 16:23 schrieb Hr. Rüge:
> Hello,
>
> trying to use windows version 2.43.0 from https://git-scm.com/downloads
> my antivirus software detects it as a virus.
> In parallel something wanted to change my hosts-entries.
>
> Ist that normal?
>
> Best regards,
> Martin
^ permalink raw reply
* Virus detected
From: Hr. Rüge @ 2024-01-25 15:23 UTC (permalink / raw)
To: git
Hello,
trying to use windows version 2.43.0 from https://git-scm.com/downloads
my antivirus software detects it as a virus.
In parallel something wanted to change my hosts-entries.
Ist that normal?
Best regards,
Martin
^ permalink raw reply
* Re: [PATCH v2] reftable/stack: adjust permissions of compacted tables
From: Karthik Nayak @ 2024-01-25 13:05 UTC (permalink / raw)
To: Patrick Steinhardt, git
In-Reply-To: <b2fb6f5ad0c558527341bd8040544d6b0ae5d8a2.1706100744.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 665 bytes --]
Patrick Steinhardt <ps@pks.im> writes:
> When creating a new compacted table from a range of preexisting ones we
> don't set the default permissions on the resulting table when specified
> by the user. This has the effect that the "core.sharedRepository" config
> will not be honored correctly.
>
> Fix this bug and add a test to catch this issue.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>
> Ugh, I totally forgot about the fact that fchmod(3P) isn't actually
> available on Windows. I've thus evicted the first patch and changed the
> second patch to use chmod(3P) instead. Too bad.
>
Good catch. This version looks good to me! Nothing to add :)
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]
^ permalink raw reply
* Re: [PATCH v6] subtree: fix split processing with multiple subtrees present
From: Christian Couder @ 2024-01-25 10:09 UTC (permalink / raw)
To: Zach FettersMoore, Junio C Hamano; +Cc: Zach FettersMoore via GitGitGadget, git
In-Reply-To: <CAP8UFD3b2y+55j3NMDm89hpVRNxX2TA-AdQS=zsboD30pZ1c4Q@mail.gmail.com>
It seems that this topic has fallen into the cracks or something,
while the associated pch looks good to me.
On Wed, Dec 20, 2023 at 4:25 PM Christian Couder
<christian.couder@gmail.com> wrote:
>
> On Tue, Dec 12, 2023 at 5:06 PM Christian Couder
> <christian.couder@gmail.com> wrote:
> >
> > On Mon, Dec 11, 2023 at 4:39 PM Zach FettersMoore
> > <zach.fetters@apollographql.com> wrote:
> > >
> > > >>
> > > >> From: Zach FettersMoore <zach.fetters@apollographql.com>
> >
> > > >> To see this in practice you can use the open source GitHub repo
> > > >> 'apollo-ios-dev' and do the following in order:
> > > >>
> > > >> -Make a changes to a file in 'apollo-ios' and 'apollo-ios-codegen'
> > > >> directories
> > > >> -Create a commit containing these changes
> > > >> -Do a split on apollo-ios-codegen
> > > >> - Do a fetch on the subtree repo
> > > >> - git fetch git@github.com:apollographql/apollo-ios-codegen.git
> > > >> - git subtree split --prefix=apollo-ios-codegen --squash --rejoin
> > >
> > > > Now I get the following without your patch at this step:
> > > >
> > > > $ git subtree split --prefix=apollo-ios-codegen --squash --rejoin
> > > > [...]/libexec/git-core/git-subtree: 318: Maximum function recursion
> > > > depth (1000) reached
> > > >
> > > > Line 318 in git-subtree.sh contains the following:
> > > >
> > > > missed=$(cache_miss "$@") || exit $?
> > > >
> > > > With your patch it seems to work:
> > > >
> > > > $ git subtree split --prefix=apollo-ios-codegen --squash --rejoin
> > > > Merge made by the 'ort' strategy.
> > > > e274aed3ba6d0659fb4cc014587cf31c1e8df7f4
> > >
> > > Looking into this some it looks like it could be a bash config
> > > difference? My machine always runs it all the way through vs
> > > failing for recursion depth. Although that would also be an issue
> > > which is solved by this fix.
> >
> > I use Ubuntu where /bin/sh is dash so my current guess is that dash
> > might have a smaller recursion limit than bash.
> >
> > I just found https://stackoverflow.com/questions/69493528/git-subtree-maximum-function-recursion-depth
> > which seems to agree.
> >
> > I will try to test using bash soon.
>
> Sorry, to not have tried earlier before with bash.
>
> Now I have tried it and yeah it works fine with you patch, while
> without it the last step of the reproduction recipe takes a lot of
> time and results in a core dump:
>
> /home/christian/libexec/git-core/git-subtree: line 924: 857920 Done
> eval "$grl"
> 857921 Segmentation fault (core dumped) | while read rev parents; do
> process_split_commit "$rev" "$parents";
> done
>
> So overall I think your patch is great! Thanks!
^ permalink raw reply
* Re: [PATCH 1/2] FIX: use utf8_strnwidth for line_prefix in diff.c
From: Md Isfarul Haque @ 2024-01-25 5:52 UTC (permalink / raw)
To: Junio C Hamano, Md Isfarul Haque via GitGitGadget; +Cc: git
In-Reply-To: <xmqqplxqcx5p.fsf@gitster.g>
On 1/25/24 01:38, Junio C Hamano wrote:
> "Md Isfarul Haque via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> From: Md Isfarul Haque <isfarul.876@gmail.com>
>>
>> This patch adresses diff.c:2721 and proposes the fix using a new function.
>
> Once the issue has fully been addressed, it is expected that the
> NEEDSWORK comment there would be removed, making this proposed log
> message useless. Make it a habit to write a log message that is
> self-contained enough to help readers (including yourself in the
> future when you have forgotten the details of what you did in this
> commit).
>
I understand. Sorry for the mess-up. I will keep it in mind the next time.
>> +const struct strbuf *diff_line_prefix_buf(struct diff_options *opt)
>> +{
>
> Given that there is only one caller of this function in the same
> file, I do not see a reason why this needs to be extern and exported
> in diff.h (actually I do not see a need for this helper at all).
>
> When dealing with a string buffer, it is much more common in this
> codebase for the caller to prepare a strbuf (often on its stack) and
> pass a pointer to it to helper functions. I.e.
>
> static void prepare_diff_line_prefix_buf(struct strbuf *buf,
> struct diff_options *opt)
> {
> ... stuff whatever you need into the string buffer ...
> strbuf_add(buf, ...);
> }
>
> /* in show_stats() */
> struct strbuf line_prefix = STRBUF_INIT;
> ...
> prepare_diff_line_prefix_buf(&line_prefix, options);
> ... use line_prefix and ...
> ... release the resource before returning ...
> strbuf_release(&line_prefix);
>
> is more common and less prone to resource leak over time.
>
Ah, this is indeed very neat. Didn't strike me. I'm not extremely familiar
with the codebase and was unaware of this practice. I will follow this
pattern in the future.
>> @@ -2635,7 +2649,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
>> int width, name_width, graph_width, number_width = 0, bin_width = 0;
>> const char *reset, *add_c, *del_c;
>> int extra_shown = 0;
>> - const char *line_prefix = diff_line_prefix(options);
>> + const struct strbuf *line_prefix = diff_line_prefix_buf(options);
>> struct strbuf out = STRBUF_INIT;
>>
>> if (data->nr == 0)
>> @@ -2718,7 +2732,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
>> * used to correctly count the display width instead of strlen().
>> */
>> if (options->stat_width == -1)
>> - width = term_columns() - strlen(line_prefix);
>> + width = term_columns() - utf8_strnwidth(line_prefix->buf, line_prefix->len, 1);
>
> I do not see the need for any of the diff_line_prefix_buf() related
> changes, only to do this change. You have a const char *line_prefix
> at this point, and utf8_strnwidth() wants to know its length, so
> what you need is to massage the parameter to match what it wants.
> Perhaps even something simple and dumb like
>
> utf8_strnwidth(line_prefix, strlen(line_prefix), 1);
>
> might be sufficient to replace strlen(line_prefix) in the original?
It was more of a force of habit on my end, since I usually do not use
functions that do not have a limit on the length they are reading.
However, considering that the string is generated by another function
and is most likely safe as it was used earlier, I will implement
this suggestion.
>
> This patch hopefully will change the behaviour of the command. A
> patch needs to also protect the change from future breakages by
> adding a test or two to demonstrate the desired behaviour. Such a
> test should pass with the code change in the patch, and should fail
> when the code change in the patch gets reverted.
>
Alright. Where should I add the test? A new/existing test in t/t4013
or t4124-log-graph-octopus.sh?
--
Thanks and regards,
Md Isfarul Haque
^ permalink raw reply
* Re: [PATCH 1/2] FIX: use utf8_strnwidth for line_prefix in diff.c
From: Md Isfarul Haque @ 2024-01-25 5:42 UTC (permalink / raw)
To: Christian Couder, Md Isfarul Haque via GitGitGadget; +Cc: git
In-Reply-To: <CAP8UFD2oxpefhGtH61wZvcHEvd24jtu_ZBVsnNZMjD2aM_NDcg@mail.gmail.com>
On 1/25/24 01:31, Christian Couder wrote:
> On Wed, Jan 24, 2024 at 3:04 PM Md Isfarul Haque via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>>
>> From: Md Isfarul Haque <isfarul.876@gmail.com>
>>
>> This patch adresses diff.c:2721 and proposes the fix using a new function.
>
> Please give more details here about what is currently at diff.c:2721
> and what the patch is fixing, as lines at diff.c:2721 could move to a
> different location if some changes are made to diff.c before your
> patches get merged or after they get merged.
>
> Also if the patch is addressing an issue in a code comment I would
> expect the corresponding code comment to be removed by the patch.
I understand and apologize for the mess-up. I will keep it in mind next time.
> About the subject, maybe "diff: use utf8_strnwidth() for line_prefix"
> is already better.
>
>> Signed-off-by: Md Isfarul Haque <isfarul.876@gmail.com>
>> ---
>> diff.c | 18 ++++++++++++++++--
>> diff.h | 1 +
>> 2 files changed, 17 insertions(+), 2 deletions(-)
>>
>> diff --git a/diff.c b/diff.c
>> index a89a6a6128a..e3223b8ce5b 100644
>> --- a/diff.c
>> +++ b/diff.c
>> @@ -2300,6 +2300,20 @@ const char *diff_line_prefix(struct diff_options *opt)
>> return msgbuf->buf;
>> }
>>
>> +const struct strbuf *diff_line_prefix_buf(struct diff_options *opt)
>
> This function seems to be used only in diff.c, so it could be static.
As Junio pointed out, I will probably use the existing function with slight
modifications and use it. Besides, having unnecessary allocations and frees
will probably only add overhead.
--
Thanks and regards
Md. Isfarul Haque
^ permalink raw reply
* Re: [PATCH v3 2/2] revision: Implement `git log --merge` also for rebase/cherry_pick/revert
From: Junio C Hamano @ 2024-01-24 22:13 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Elijah Newren, Michael Lohmann, git, phillip.wood123
In-Reply-To: <0a061bee-9b78-419f-9f1a-ced69ca96002@kdbg.org>
Johannes Sixt <j6t@kdbg.org> writes:
> Am 24.01.24 um 20:46 schrieb Junio C Hamano:
>> Johannes Sixt <j6t@kdbg.org> writes:
>>
>>> Good point! IMO, REBASE_HEAD should have lower precedence than all the
>>> other *_HEADs. It would mean to reorder the entries:
>>>
>>> static const char *const other_head[] = {
>>> "MERGE_HEAD", "CHERRY_PICK_HEAD", "REVERT_HEAD", "REBASE_HEAD"
>>> };
>>>
>>> (and perhaps adjust the error message, too).
>>
>> And probably give a warning saying that we noticed you are rebasing
>> and cherry-picking and we chose to show the --merge based on the
>> relationship between cherry-pick-head and head, ignoring your rebase
>> status, or something.
>
> I don't think that's necessary. When rebase stopped with a merge
> conflict, neither of the other commands can begin their work until the
> conflicted state is removed. That should be a concious act, just like
> when thereafter one of these other commands is used and leads to a
> conflict. At least I would certainly not need a reminder.
OK.
^ permalink raw reply
* Re: [PATCH v3 2/2] revision: Implement `git log --merge` also for rebase/cherry_pick/revert
From: Johannes Sixt @ 2024-01-24 22:06 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Elijah Newren, Michael Lohmann, git, phillip.wood123
In-Reply-To: <xmqq1qa6ecqr.fsf@gitster.g>
Am 24.01.24 um 20:46 schrieb Junio C Hamano:
> Johannes Sixt <j6t@kdbg.org> writes:
>
>> Good point! IMO, REBASE_HEAD should have lower precedence than all the
>> other *_HEADs. It would mean to reorder the entries:
>>
>> static const char *const other_head[] = {
>> "MERGE_HEAD", "CHERRY_PICK_HEAD", "REVERT_HEAD", "REBASE_HEAD"
>> };
>>
>> (and perhaps adjust the error message, too).
>
> And probably give a warning saying that we noticed you are rebasing
> and cherry-picking and we chose to show the --merge based on the
> relationship between cherry-pick-head and head, ignoring your rebase
> status, or something.
I don't think that's necessary. When rebase stopped with a merge
conflict, neither of the other commands can begin their work until the
conflicted state is removed. That should be a concious act, just like
when thereafter one of these other commands is used and leads to a
conflict. At least I would certainly not need a reminder.
-- Hannes
^ 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