* [PATCH 1/2] git-compat-util: add strtol_i2
From: Mohit Marathe via GitGitGadget @ 2024-01-22 8:51 UTC (permalink / raw)
To: git; +Cc: Mohit Marathe, Mohit Marathe
In-Reply-To: <pull.1646.git.1705913519.gitgitgadget@gmail.com>
From: Mohit Marathe <mohitmarathe23@gmail.com>
This function is an updated version of strtol_i function. It will
give more control to handle parsing of the characters after the
integer and better error handling while parsing numbers.
Signed-off-by: Mohit Marathe <mohitmarathe@proton.me>
---
git-compat-util.h | 23 +++++++++++++++++++++++
1 file changed, 23 insertions(+)
diff --git a/git-compat-util.h b/git-compat-util.h
index 7c2a6538e5a..0a014c837d8 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -1309,6 +1309,29 @@ static inline int strtol_i(char const *s, int base, int *result)
return 0;
}
+#define strtol_i(s,b,r) strtol_i2((s), (b), (r), NULL)
+static inline int strtol_i2(char const *s, int base, int *result, char **endp)
+{
+ long ul;
+ char *dummy = NULL;
+
+ if (!endp)
+ endp = &dummy;
+ errno = 0;
+ ul = strtol(s, endp, base);
+ if (errno ||
+ /*
+ * if we are told to parse to the end of the string by
+ * passing NULL to endp, it is an error to have any
+ * remaining character after the digits.
+ */
+ (dummy && *dummy) ||
+ *endp == s || (int) ul != ul)
+ return -1;
+ *result = ul;
+ return 0;
+}
+
void git_stable_qsort(void *base, size_t nmemb, size_t size,
int(*compar)(const void *, const void *));
#ifdef INTERNAL_QSORT
--
gitgitgadget
^ permalink raw reply related
* [PATCH 0/2] Replace atoi() with strtol_i2()
From: Mohit Marathe via GitGitGadget @ 2024-01-22 8:51 UTC (permalink / raw)
To: git; +Cc: Mohit Marathe
Hello,
This patch series replaces atoi() with an updated version of strtol_i()
called strtol_i2 (Credits: Junio C Hamano). The reasoning behind this is to
improve error handling by not allowing non-numerical characters in the hunk
header (which might happen in case of a corrupt patch, although rarely).
There is still a change to be made, as Junio says: "A corrupt patch may be
getting a nonsense patch-ID with the current code and hopefully is not
matching other patches that are not corrupt, but with such a change, a
corrupt patch may not be getting any patch-ID and a loop that computes
patch-ID for many files and try to match them up might need to be rewritten
to take the new failure case into account." I'm not sure where this change
needs to me made (maybe get_one_patchid()?). It would be great if anyone
could point me to the correct place.
Thanks, Mohit Marathe
Mohit Marathe (2):
git-compat-util: add strtol_i2
patch-id: replace `atoi()` with `strtol_i2()`
builtin/patch-id.c | 14 +++++++-------
git-compat-util.h | 23 +++++++++++++++++++++++
2 files changed, 30 insertions(+), 7 deletions(-)
base-commit: e02ecfcc534e2021aae29077a958dd11c3897e4c
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1646%2Fmohit-marathe%2Fupdate-strtol_i-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1646/mohit-marathe/update-strtol_i-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1646
--
gitgitgadget
^ permalink raw reply
* Re: [PATCH v2 0/5] ci: add support for macOS to GitLab CI
From: Patrick Steinhardt @ 2024-01-22 6:14 UTC (permalink / raw)
To: phillip.wood; +Cc: git, Matthias Aßhauer
In-Reply-To: <6e190a32-ee45-451b-b841-25cc6eb2c5ab@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1299 bytes --]
On Sun, Jan 21, 2024 at 02:50:05PM +0000, Phillip Wood wrote:
> Hi Patrick
>
> On 18/01/2024 10:22, Patrick Steinhardt wrote:
> > Hi,
> >
> > this is the second version of my patch series that adds a macOS job to
> > GitLab CI. Changes compared to v1:
> >
> > - Added a fix for a flaky test in t7527 that caused the pipeline to
> > fail in ~50% of all runs.
> >
> > - Improved some commit messages.
> >
> > - Tests now write test data into a RAMDisk. This speeds up tests and
> > fixes some hung pipelines I was seeing.
> >
> > Thanks for your reviews so far!
>
> I've read though all the patches and they seem sensible to me though I'm
> hardly a macOS expert. I did wonder about the use of pushd/popd in the
> fourth patch as they are bashisms but that matches what we're doing on
> Ubuntu already. It's nice to see the GitLab CI running on macOS as well as
> Linux now.
Yeah, that part is a bit weird, agreed. As you say, I basically copied
the code that we use on Ubuntu, and that is intentional because another
follow-up patch series will rip out that part and move the shared code
into a common "install-p4.sh" script. Like that, we can also easily use
this script on the Docker-based Ubuntu jobs.
Thanks for your review!
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 4/4] cherry-pick: Add `--empty` for more robust redundant commit handling
From: Junio C Hamano @ 2024-01-21 22:41 UTC (permalink / raw)
To: Brian Lyles; +Cc: Kristoffer Haugsbakk, Taylor Blau, Elijah Newren, git
In-Reply-To: <CAHPHrSddqTEjtfhgVJ=vmRhbtuwXcGEiE1KFZqR1QhKw-HDtSg@mail.gmail.com>
Brian Lyles <brianmlyles@gmail.com> writes:
>> Trailing whitespace on this line.
>
> Thank you -- This will be corrected with v2.
>
> Is the sample pre-commit hook the ideal way to prevent this in the
> future?
The example we ship in templates/hooks--pre-commit.sample in our
source has such a check at the end of it.
^ permalink raw reply
* Re: [PATCH 4/4] cherry-pick: Add `--empty` for more robust redundant commit handling
From: Kristoffer Haugsbakk @ 2024-01-21 22:05 UTC (permalink / raw)
To: brianmlyles; +Cc: Taylor Blau, Elijah Newren, git
In-Reply-To: <CAHPHrSddqTEjtfhgVJ=vmRhbtuwXcGEiE1KFZqR1QhKw-HDtSg@mail.gmail.com>
On Sun, Jan 21, 2024, at 19:28, Brian Lyles wrote:
> Is the sample pre-commit hook the ideal way to prevent this in the
> future? Or is there some config I could set globally to enforce this
> across repositories? I was having a little trouble finding a good way to
> accomplish this globally.
I don’t know of any global config. So a pre-commit hook is probably the
safest bet. Personally I set all my editors to remove trailing space and
they very seldom mess it up. :)
--
Kristoffer Haugsbakk
^ permalink raw reply
* [PATCH v4] tests: move t0009-prio-queue.sh to the new unit testing framework
From: Chandra Pratap via GitGitGadget @ 2024-01-21 19:28 UTC (permalink / raw)
To: git; +Cc: Chandra Pratap, Jeff King, Phillip Wood, Chandra Pratap,
Chandra Pratap
In-Reply-To: <pull.1642.v3.git.1705502304219.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 to Jeff, Philip and Junio for your valuable feedbacks! I have
updated the patch so that the resulting tests are more verbose when they
fail and lesser prone to memory related errors.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1642%2FChand-ra%2Fprio-queue-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1642/Chand-ra/prio-queue-v4
Pull-Request: https://github.com/gitgitgadget/git/pull/1642
Range-diff vs v3:
1: 6b10028177a ! 1: e9a0dba6f47 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 int test_prio_queue(int *input, int *result)
++static void test_prio_queue(int *input, int *result, size_t input_size)
+{
+ struct prio_queue pq = { intcmp };
-+ int i = 0;
+
-+ while (*input) {
-+ int *val = input++;
++ for (int i = 0, j = 0; i < input_size; i++) {
+ void *peek, *get;
-+ switch(*val) {
-+ case GET:
-+ peek = prio_queue_peek(&pq);
++ 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);
++ break;
++ case DUMP:
++ while ((peek = prio_queue_peek(&pq))) {
+ get = prio_queue_get(&pq);
-+ if (peek != get)
-+ BUG("peek and get results don't match");
-+ result[i++] = show(get);
-+ break;
-+ case DUMP:
-+ while ((peek = prio_queue_peek(&pq))) {
-+ get = prio_queue_get(&pq);
-+ if (peek != get)
-+ BUG("peek and get results don't match");
-+ result[i++] = show(get);
-+ }
-+ break;
-+ case STACK:
-+ pq.compare = NULL;
-+ break;
-+ case REVERSE:
-+ prio_queue_reverse(&pq);
-+ break;
-+ default:
-+ prio_queue_put(&pq, val);
-+ break;
++ if (!check(peek == get))
++ return;
++ if(!check_int(result[j++], ==, show(get)))
++ test_msg("failed at result[] index %d", j-1);
++ }
++ 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);
-+ return 0;
+}
+
-+#define BASIC_INPUT 1, 2, 3, 4, 5, 5, DUMP
-+#define BASIC_EXPECTED 1, 2, 3, 4, 5, 5
++#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_EXPECTED 2, 3, 4, 1, 5, 6
++#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_EXPECTED 1, 2, MISSING, 1, 2, MISSING
++#define EMPTY_QUEUE_RESULT 1, 2, MISSING, 1, 2, MISSING
+
-+#define STACK_INPUT STACK, 1, 5, 4, 6, 2, 3, DUMP
-+#define STACK_EXPECTED 3, 2, 6, 4, 5, 1
++#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_EXPECTED 1, 2, 3, 4, 5, 6
++#define REVERSE_STACK_RESULT 1, 2, 3, 4, 5, 6
+
-+#define TEST_INPUT(INPUT, EXPECTED, name) \
++#define TEST_INPUT(INPUT, RESULT, name) \
+ static void test_##name(void) \
+{ \
+ int input[] = {INPUT}; \
-+ int expected[] = {EXPECTED}; \
-+ int result[ARRAY_SIZE(expected)]; \
-+ test_prio_queue(input, result); \
-+ check(!memcmp(expected, result, sizeof(expected))); \
++ int result[] = {RESULT}; \
++ test_prio_queue(input, result, ARRAY_SIZE(input)); \
+}
+
-+TEST_INPUT(BASIC_INPUT, BASIC_EXPECTED, basic)
-+TEST_INPUT(MIXED_PUT_GET_INPUT, MIXED_PUT_GET_EXPECTED, mixed)
-+TEST_INPUT(EMPTY_QUEUE_INPUT, EMPTY_QUEUE_EXPECTED, empty)
-+TEST_INPUT(STACK_INPUT, STACK_EXPECTED, stack)
-+TEST_INPUT(REVERSE_STACK_INPUT, REVERSE_STACK_EXPECTED, reverse)
++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)
+{
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 | 98 +++++++++++++++++++++++++++++++++++++
6 files changed, 99 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..d78b002f9ea
--- /dev/null
+++ b/t/unit-tests/t-prio-queue.c
@@ -0,0 +1,98 @@
+#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 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)))
+ test_msg("failed at result[] index %d", j-1);
+ 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);
+ }
+ 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 4/4] cherry-pick: Add `--empty` for more robust redundant commit handling
From: Brian Lyles @ 2024-01-21 18:28 UTC (permalink / raw)
To: Kristoffer Haugsbakk; +Cc: Taylor Blau, Elijah Newren, git
In-Reply-To: <06eb93d7-7113-4583-9860-28a6a6d528a2@app.fastmail.com>
On Sat, Jan 20, 2024 at 2:24 PM Kristoffer Haugsbakk
<code@khaugsbakk.name> wrote:
> On Fri, Jan 19, 2024, at 06:59, brianmlyles@gmail.com wrote:
> > From: Brian Lyles <brianmlyles@gmail.com>
> > ---keep-redundant-commits::
> > - If a commit being cherry picked duplicates a commit already in the
> > - current history, it will become empty. By default these
> > - redundant commits cause `cherry-pick` to stop so the user can
> > - examine the commit. This option overrides that behavior and
> > - creates an empty commit object. Note that use of this option only
> > +--empty=(stop|drop|keep)::
> > + How to handle commits being cherry-picked that are redundant with
> > + changes already in the current history.
> > ++
> > +--
>
> Trailing whitespace on this line.
Thank you -- This will be corrected with v2.
Is the sample pre-commit hook the ideal way to prevent this in the
future? Or is there some config I could set globally to enforce this
across repositories? I was having a little trouble finding a good way to
accomplish this globally.
Thanks,
Brian Lyles
^ permalink raw reply
* Re: [PATCH 1/4] sequencer: Do not require `allow_empty` for redundant commit options
From: Brian Lyles @ 2024-01-21 18:19 UTC (permalink / raw)
To: Kristoffer Haugsbakk; +Cc: Taylor Blau, Elijah Newren, git
In-Reply-To: <d6361ecf-82bc-46c6-adfe-3c6ab25d39b2@app.fastmail.com>
On Sat, Jan 20, 2024 at 3:38 PM Kristoffer Haugsbakk
<code@khaugsbakk.name> wrote:
Hi Kristoffer,
Thank you for taking some time to review my patch.
> Initial discussion (proposal): https://lore.kernel.org/git/CAHPHrSevBdQF0BisR8VK=jM=wj1dTUYEVrv31gLerAzL9=Cd8Q@mail.gmail.com/
I ought to have included this in a cover letter -- thanks for linking it
here, and I will include this with v2.
> On Fri, Jan 19, 2024, at 06:59, brianmlyles@gmail.com wrote:
> > From: Brian Lyles <brianmlyles@gmail.com>
> >
> > Previously, a consumer of the sequencer that wishes to take advantage of
> > either the `keep_redundant_commits` or `drop_redundant_commits` feature
> > must also specify `allow_empty`.
>
> Previously to this change? It is preferred to describe what the code
> currently does without this change in the present tense.[1] The change
> itself uses the imperative mood.[2]
>
> † 1: SubmittingPatches, “The problem statement that describes the status
> quo …”
> † 2: SubmittingPatches, “Describe your changes in imperative mood […] as
> if you are giving orders to the codebase to change its behavior.”
I appreciate the stylistic feedback here. I will tweak this for v2 of
the patch.
> > In a future commit, an `--empty` option will be added to
> > `git-cherry-pick`, meaning that `drop_redundant_commits` will be
> > available in that command. For that to be possible with the current
> > implementation of the sequencer's `allow_empty()`, `git-cherry-pick`
> > would need to specify `allow_empty` with `drop_redundant_commits` as
> > well, which is an even less intuitive implication of `--allow-empty`: in
> > order to prevent redundant commits automatically, initially-empty
> > commits would need to be kept automatically.
> >
> > Instead, this commit rewrites the `allow_empty()` logic to remove the
> > over-arching requirement that `allow_empty` be specified in order to
> > reach any of the keep/drop behaviors. Only if the commit was originally
> > empty will `allow_empty` have an effect.
>
> In general, phrases like “this commit <verb>” or “this patch <verb>” can
> be rewritten to the “commanding” style (see [2]).[3] But here you’re
> starting a new paragraph after having talked about a future commit, so
> using the commanding style might be stylistically difficult to pull off
> without breaking the flow of the text.
>
> And “this [commit][patch] <verb>” seems to be used with some regularity in
> any case.
>
> 🔗 3: https://lore.kernel.org/git/xmqqedeqienh.fsf@gitster.g/
I think I should be able to adjust this to a more commanding style
without breaking the flow. I'll give that a shot in v2 as well.
> > Disclaimer: This is my first contribution to the git project, and thus
> > my first attempt at submitting a patch via `git-send-email`. It is also
> > the first time I've touched worked in C in over a decade, and I really
> > didn't work with it much before that either. I welcome any and all
> > feedback on what I may have gotten wrong regarding the patch submission
> > process, the code changes, or my commit messages.
>
> This part (after the commit message) looks like the cover letter for the
> series (the four patches). `SubmittingPatches` recommends submitting that
> in a dedicated email message (for series that have more than one
> patch). Maybe this cover letter style is just an alternative that is
> equally accepted. But most series use a separate cover letter message for
> what it’s worth.
That makes sense -- when I send v2, I can pull this part out into a
separate cover letter email.
I will hold off on sending out v2 for a few days to give others a chance
to weigh in on this series.
Thanks again,
Brian Lyles
^ permalink raw reply
* Re: Git pre-received hook not failing with exit code 1 correcly
From: brian m. carlson @ 2024-01-21 18:05 UTC (permalink / raw)
To: Marc C; +Cc: git
In-Reply-To: <CYK0FIQPVI22.2RHKOY4L00FZS@mccd.space>
[-- Attachment #1: Type: text/plain, Size: 1976 bytes --]
On 2024-01-21 at 01:57:28, Marc C wrote:
> Heya!
Hey,
> I am trying to understand why my pre-publish hook does not exit with the
> correct status code.
>
> I have a pre-publish script
>
> ```
> #!/bin/sh
> set -euo pipefail
One note about this is that `-o pipefail` is a bash- and zsh-specific
option. (It's also present in some ksh variants.) It isn't specified
by POSIX, so you'd probably want to use `#!/bin/bash` as your shebang
instead.
Even OSes such as Debian which specify certain extensions required for
`/bin/sh` (notably `local`) don't require this of it.
> However, when running the script as a pre-receive hook, it is not
> running the commands correctly and returns the wrong exit code. I get
> the following:
>
> ```
> remote: Testing nixos config
> remote: building the system configuration...
> remote: Success <-- ????
> remote: error:
> remote: … while calling the 'seq' builtin
> ...
> To myserver:/myrepo
> bffa94e..a14b3f6 main -> main
> ```
>
> Any clue what I am missing? When running it as a pre-receive hook, the
> failing command returns exit code 0. Running it in the CLI, it returns
> exit code 1. It is Schrodinger's exit code.
I know nothing about nixOS, but I'm wondering if maybe `nixos-rebuild
dry-build` forks in some cases, specifically when it's detached from the
terminal. If that happened, then you'd see the above, where the child
process continued but the parent process exited, and then the error
message would get printed at the end.
You'd also see this if you had an `&` at the end of `nixos-rebuild
dry-build`, but you don't seem to have that here.
You could try something like `(cat | ./.git/hooks/pre-receive 2>&1 | cat)`
to see if the problem is the lack of terminal.
Hopefully that gives you a helpful head start on the problem. I
apologize for not being able to help more.
--
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 262 bytes --]
^ permalink raw reply
* Re: [Bug?] "git diff --no-rename A B"
From: René Scharfe @ 2024-01-21 17:56 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git
In-Reply-To: <xmqqmsszga9m.fsf@gitster.g>
Am 20.01.24 um 18:55 schrieb Junio C Hamano:
>
> I agree with you that the structure of that loop and the processing
> in it does look confusing.
Here's a small simplification.
--- >8 ---
Subject: [PATCH 2/1] parse-options: simplify positivation handling
We accept the positive version of options whose long name starts with
"no-" and are defined without the flag PARSE_OPT_NONEG. E.g. git clone
has an explicitly defined --no-checkout option and also implicitly
accepts --checkout to override it.
parse_long_opt() handles that by restarting the option matching with the
positive version when it finds that only the current option definition
starts with "no-", but not the user-supplied argument. This code is
located almost at the end of the matching logic.
Avoid the need for a restart by moving the code up. We don't have to
check the positive arg against the negative long_name at all -- the
"no-" prefix of the latter makes a match impossible. Skip it and toggle
OPT_UNSET right away to simplify the control flow.
Signed-off-by: René Scharfe <l.s.r@web.de>
---
Patch formatted with --function-context for easier review.
parse-options.c | 13 ++++++-------
1 file changed, 6 insertions(+), 7 deletions(-)
diff --git a/parse-options.c b/parse-options.c
index 92e96ca6cd..63a99dea6e 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -353,95 +353,94 @@ static int is_alias(struct parse_opt_ctx_t *ctx,
static enum parse_opt_result parse_long_opt(
struct parse_opt_ctx_t *p, const char *arg,
const struct option *options)
{
const char *arg_end = strchrnul(arg, '=');
const struct option *abbrev_option = NULL, *ambiguous_option = NULL;
enum opt_parsed abbrev_flags = OPT_LONG, ambiguous_flags = OPT_LONG;
int allow_abbrev = !(p->flags & PARSE_OPT_KEEP_UNKNOWN_OPT);
for (; options->type != OPTION_END; options++) {
const char *rest, *long_name = options->long_name;
enum opt_parsed flags = OPT_LONG, opt_flags = OPT_LONG;
if (options->type == OPTION_SUBCOMMAND)
continue;
if (!long_name)
continue;
-again:
+ if (!starts_with(arg, "no-") &&
+ !(options->flags & PARSE_OPT_NONEG) &&
+ skip_prefix(long_name, "no-", &long_name))
+ opt_flags |= OPT_UNSET;
+
if (!skip_prefix(arg, long_name, &rest))
rest = NULL;
if (!rest) {
/* abbreviated? */
if (allow_abbrev &&
!strncmp(long_name, arg, arg_end - arg)) {
is_abbreviated:
if (abbrev_option &&
!is_alias(p, abbrev_option, options)) {
/*
* If this is abbreviated, it is
* ambiguous. So when there is no
* exact match later, we need to
* error out.
*/
ambiguous_option = abbrev_option;
ambiguous_flags = abbrev_flags;
}
if (!(flags & OPT_UNSET) && *arg_end)
p->opt = arg_end + 1;
abbrev_option = options;
abbrev_flags = flags ^ opt_flags;
continue;
}
/* negation allowed? */
if (options->flags & PARSE_OPT_NONEG)
continue;
/* negated and abbreviated very much? */
if (allow_abbrev && starts_with("no-", arg)) {
flags |= OPT_UNSET;
goto is_abbreviated;
}
/* negated? */
- if (!starts_with(arg, "no-")) {
- if (skip_prefix(long_name, "no-", &long_name)) {
- opt_flags |= OPT_UNSET;
- goto again;
- }
+ if (!starts_with(arg, "no-"))
continue;
- }
flags |= OPT_UNSET;
if (!skip_prefix(arg + 3, long_name, &rest)) {
/* abbreviated and negated? */
if (allow_abbrev &&
starts_with(long_name, arg + 3))
goto is_abbreviated;
else
continue;
}
}
if (*rest) {
if (*rest != '=')
continue;
p->opt = rest + 1;
}
return get_value(p, options, flags ^ opt_flags);
}
if (disallow_abbreviated_options && (ambiguous_option || abbrev_option))
die("disallowed abbreviated or ambiguous option '%.*s'",
(int)(arg_end - arg), arg);
if (ambiguous_option) {
error(_("ambiguous option: %s "
"(could be --%s%s or --%s%s)"),
arg,
(ambiguous_flags & OPT_UNSET) ? "no-" : "",
ambiguous_option->long_name,
(abbrev_flags & OPT_UNSET) ? "no-" : "",
abbrev_option->long_name);
return PARSE_OPT_HELP;
}
if (abbrev_option)
return get_value(p, abbrev_option, abbrev_flags);
return PARSE_OPT_UNKNOWN;
}
--
2.43.0
^ permalink raw reply related
* Re: [PATCH v6 0/6] support remote archive via stateless transport
From: Linus Arver @ 2024-01-21 16:57 UTC (permalink / raw)
To: Jiang Xin, Git List, Junio C Hamano; +Cc: Jiang Xin
In-Reply-To: <cover.1705841443.git.zhiyou.jx@alibaba-inc.com>
This v6 version LGTM. Thanks!
^ permalink raw reply
* Re: [PATCH v2 0/5] ci: add support for macOS to GitLab CI
From: Phillip Wood @ 2024-01-21 14:50 UTC (permalink / raw)
To: Patrick Steinhardt, git; +Cc: Matthias Aßhauer
In-Reply-To: <cover.1705573336.git.ps@pks.im>
Hi Patrick
On 18/01/2024 10:22, Patrick Steinhardt wrote:
> Hi,
>
> this is the second version of my patch series that adds a macOS job to
> GitLab CI. Changes compared to v1:
>
> - Added a fix for a flaky test in t7527 that caused the pipeline to
> fail in ~50% of all runs.
>
> - Improved some commit messages.
>
> - Tests now write test data into a RAMDisk. This speeds up tests and
> fixes some hung pipelines I was seeing.
>
> Thanks for your reviews so far!
I've read though all the patches and they seem sensible to me though I'm
hardly a macOS expert. I did wonder about the use of pushd/popd in the
fourth patch as they are bashisms but that matches what we're doing on
Ubuntu already. It's nice to see the GitLab CI running on macOS as well
as Linux now.
Best Wishes
Phillip
> Patrick
>
> Patrick Steinhardt (5):
> t7527: decrease likelihood of racing with fsmonitor daemon
> Makefile: detect new Homebrew location for ARM-based Macs
> ci: handle TEST_OUTPUT_DIRECTORY when printing test failures
> ci: make p4 setup on macOS more robust
> ci: add macOS jobs to GitLab CI
>
> .gitlab-ci.yml | 34 +++++++++++++++++++++++++++++++++-
> ci/install-dependencies.sh | 10 ++++------
> ci/lib.sh | 12 +++++++++++-
> ci/print-test-failures.sh | 2 +-
> config.mak.uname | 13 +++++++++++++
> t/t7527-builtin-fsmonitor.sh | 2 +-
> 6 files changed, 63 insertions(+), 10 deletions(-)
>
> Range-diff against v1:
> -: ---------- > 1: 554b1c8546 t7527: decrease likelihood of racing with fsmonitor daemon
> 2: 3adb0b7ae8 = 2: 32d8bd1d78 Makefile: detect new Homebrew location for ARM-based Macs
> -: ---------- > 3: d55da77747 ci: handle TEST_OUTPUT_DIRECTORY when printing test failures
> 1: a5d725bea7 ! 4: 1ed6e68650 ci: make p4 setup on macOS more robust
> @@ Commit message
> into a separate directory which we then manually append to our PATH.
> This matches what we do on Linux-based jobs.
>
> + Note that it may seem like we already did append "$HOME/bin" to PATH
> + because we're actually removing the lines that adapt PATH. But we only
> + ever adapted the PATH variable in "ci/install-dependencies.sh", and
> + didn't adapt it when running "ci/run-build-and-test.sh". Consequently,
> + the required binaries wouldn't be found during the test run unless the
> + CI platform already had the "$HOME/bin" in PATH right from the start.
> +
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
>
> ## ci/install-dependencies.sh ##
> 3: d196cfd9d0 ! 5: c5ed38f0a6 ci: add macOS jobs to GitLab CI
> @@ Metadata
> ## Commit message ##
> ci: add macOS jobs to GitLab CI
>
> - Add two macOS-based jobs to GitLab CI, one for Clang and one for GCC.
> - This matches equivalent jobs we have for GitHub Workflows, except that
> - we use macOS 14 instead of macOS 13.
> + Add a job to GitLab CI which runs tests on macOS, which matches the
> + equivalent "osx-clang" job that we have for GitHub Workflows. One
> + significant difference though is that this new job runs on Apple M1
> + machines and thus uses the "arm64" architecture. As GCC does not yet
> + support this comparatively new architecture we cannot easily include an
> + equivalent for the "osx-gcc" job that exists in GitHub Workflows.
>
> Note that one test marked as `test_must_fail` is surprisingly passing:
>
> t7815-grep-binary.sh (Wstat: 0 Tests: 22 Failed: 0)
> TODO passed: 12
>
> - This seems to boil down to an unexpected difference in how regcomp(1)
> + This seems to boil down to an unexpected difference in how regcomp(3P)
> works when matching NUL bytes. Cross-checking with the respective GitHub
> - job shows though that this is not an issue unique to the GitLab CI job
> - as it passes in the same way there.
> -
> - Further note that we do not include the equivalent for the "osx-gcc" job
> - that we use with GitHub Workflows. This is because the runner for macOS
> - on GitLab is running on Apple M1 machines and thus uses the "arm64"
> - architecture. GCC does not support this platform yet.
> + job shows that this is not an issue unique to the GitLab CI job as it
> + passes in the same way there.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
>
> @@ .gitlab-ci.yml: test:
> + image: $image
> + tags:
> + - saas-macos-medium-m1
> ++ variables:
> ++ TEST_OUTPUT_DIRECTORY: "/Volumes/RAMDisk"
> + before_script:
> ++ # Create a 4GB RAM disk that we use to store test output on. This small hack
> ++ # significantly speeds up tests by more than a factor of 2 because the
> ++ # macOS runners use network-attached storage as disks, which is _really_
> ++ # slow with the many small writes that our tests do.
> ++ - sudo diskutil apfs create $(hdiutil attach -nomount ram://8192000) RAMDisk
> + - ./ci/install-dependencies.sh
> + script:
> + - ./ci/run-build-and-tests.sh
> @@ .gitlab-ci.yml: test:
> + if test "$CI_JOB_STATUS" != 'success'
> + then
> + ./ci/print-test-failures.sh
> ++ mv "$TEST_OUTPUT_DIRECTORY"/failed-test-artifacts t/
> + fi
> + parallel:
> + matrix:
>
> base-commit: cd69c635a1a62b0c8bfdbf221778be8a512ad048
^ permalink raw reply
* [PATCH v6 6/6] transport-helper: call do_take_over() in process_connect
From: Jiang Xin @ 2024-01-21 13:15 UTC (permalink / raw)
To: Git List, Junio C Hamano, Linus Arver; +Cc: Jiang Xin
In-Reply-To: <cover.1705841443.git.zhiyou.jx@alibaba-inc.com>
From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
The existing pattern among all callers of process_connect() seems to be
if (process_connect(...)) {
do_take_over();
... dispatch to the underlying method ...
}
... otherwise implement the fallback ...
where the return value from process_connect() is the return value of the
call it makes to process_connect_service().
Move the call of do_take_over() inside process_connect(), so that
calling the process_connect() function is more concise and will not
miss do_take_over().
Suggested-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
---
transport-helper.c | 22 +++++++++-------------
1 file changed, 9 insertions(+), 13 deletions(-)
diff --git a/transport-helper.c b/transport-helper.c
index 91381be622..566f7473df 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -646,6 +646,7 @@ static int process_connect(struct transport *transport,
struct helper_data *data = transport->data;
const char *name;
const char *exec;
+ int ret;
name = for_push ? "git-receive-pack" : "git-upload-pack";
if (for_push)
@@ -653,7 +654,10 @@ static int process_connect(struct transport *transport,
else
exec = data->transport_options.uploadpack;
- return process_connect_service(transport, name, exec);
+ ret = process_connect_service(transport, name, exec);
+ if (ret)
+ do_take_over(transport);
+ return ret;
}
static int connect_helper(struct transport *transport, const char *name,
@@ -685,10 +689,8 @@ static int fetch_refs(struct transport *transport,
get_helper(transport);
- if (process_connect(transport, 0)) {
- do_take_over(transport);
+ if (process_connect(transport, 0))
return transport->vtable->fetch_refs(transport, nr_heads, to_fetch);
- }
/*
* If we reach here, then the server, the client, and/or the transport
@@ -1145,10 +1147,8 @@ static int push_refs(struct transport *transport,
{
struct helper_data *data = transport->data;
- if (process_connect(transport, 1)) {
- do_take_over(transport);
+ if (process_connect(transport, 1))
return transport->vtable->push_refs(transport, remote_refs, flags);
- }
if (!remote_refs) {
fprintf(stderr,
@@ -1189,11 +1189,9 @@ static struct ref *get_refs_list(struct transport *transport, int for_push,
{
get_helper(transport);
- if (process_connect(transport, for_push)) {
- do_take_over(transport);
+ if (process_connect(transport, for_push))
return transport->vtable->get_refs_list(transport, for_push,
transport_options);
- }
return get_refs_list_using_list(transport, for_push);
}
@@ -1277,10 +1275,8 @@ static int get_bundle_uri(struct transport *transport)
{
get_helper(transport);
- if (process_connect(transport, 0)) {
- do_take_over(transport);
+ if (process_connect(transport, 0))
return transport->vtable->get_bundle_uri(transport);
- }
return -1;
}
--
2.43.0
^ permalink raw reply related
* [PATCH v6 5/6] transport-helper: call do_take_over() in connect_helper
From: Jiang Xin @ 2024-01-21 13:15 UTC (permalink / raw)
To: Git List, Junio C Hamano, Linus Arver; +Cc: Jiang Xin
In-Reply-To: <cover.1705841443.git.zhiyou.jx@alibaba-inc.com>
From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
After successfully connecting to the smart transport by calling
process_connect_service() in connect_helper(), run do_take_over() to
replace the old vtable with a new one which has methods ready for the
smart transport connection. This fixes the exit code of git-archive
in test case "archive remote http repository" of t5003.
The connect_helper() function is used as the connect method of the
vtable in "transport-helper.c", and it is called by transport_connect()
in "transport.c" to setup a connection. The only place that we call
transport_connect() so far is in "builtin/archive.c". Without running
do_take_over(), it may fail to call transport_disconnect() in
run_remote_archiver() of "builtin/archive.c". This is because for a
stateless connection and a service like "git-upload-archive", the
remote helper may receive a SIGPIPE signal and exit early. Call
do_take_over() to have a graceful disconnect method, so that we still
call transport_disconnect() even if the remote helper exits early.
Helped-by: Linus Arver <linusa@google.com>
Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
---
t/t5003-archive-zip.sh | 2 +-
transport-helper.c | 2 ++
2 files changed, 3 insertions(+), 1 deletion(-)
diff --git a/t/t5003-archive-zip.sh b/t/t5003-archive-zip.sh
index 6f85bd3463..961c6aac25 100755
--- a/t/t5003-archive-zip.sh
+++ b/t/t5003-archive-zip.sh
@@ -268,7 +268,7 @@ test_expect_success 'remote archive does not work with protocol v1' '
'
test_expect_success 'archive remote http repository' '
- test_must_fail git archive --remote="$HTTPD_URL/auth/smart/bare.git" \
+ git archive --remote="$HTTPD_URL/auth/smart/bare.git" \
--output=remote-http.zip HEAD &&
test_cmp_bin d.zip remote-http.zip
'
diff --git a/transport-helper.c b/transport-helper.c
index 6fe9f4f208..91381be622 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -669,6 +669,8 @@ static int connect_helper(struct transport *transport, const char *name,
fd[0] = data->helper->out;
fd[1] = data->helper->in;
+
+ do_take_over(transport);
return 0;
}
--
2.43.0
^ permalink raw reply related
* [PATCH v6 4/6] http-backend: new rpc-service for git-upload-archive
From: Jiang Xin @ 2024-01-21 13:15 UTC (permalink / raw)
To: Git List, Junio C Hamano, Linus Arver; +Cc: Jiang Xin, Eric Sunshine
In-Reply-To: <cover.1705841443.git.zhiyou.jx@alibaba-inc.com>
From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
Add new rpc-service "upload-archive" in http-backend to add server side
support for remote archive over HTTP/HTTPS protocols.
Also add new test cases in t5003. In the test case "archive remote http
repository", git-archive exits with a non-0 exit code even though we
create the archive correctly. It will be fixed in a later commit.
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
---
http-backend.c | 13 ++++++++++---
t/t5003-archive-zip.sh | 34 ++++++++++++++++++++++++++++++++++
2 files changed, 44 insertions(+), 3 deletions(-)
diff --git a/http-backend.c b/http-backend.c
index ff07b87e64..1ed1e29d07 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -38,6 +38,7 @@ struct rpc_service {
static struct rpc_service rpc_service[] = {
{ "upload-pack", "uploadpack", 1, 1 },
{ "receive-pack", "receivepack", 0, -1 },
+ { "upload-archive", "uploadarchive", 0, -1 },
};
static struct string_list *get_parameters(void)
@@ -639,10 +640,15 @@ static void check_content_type(struct strbuf *hdr, const char *accepted_type)
static void service_rpc(struct strbuf *hdr, char *service_name)
{
- const char *argv[] = {NULL, "--stateless-rpc", ".", NULL};
+ struct strvec argv = STRVEC_INIT;
struct rpc_service *svc = select_service(hdr, service_name);
struct strbuf buf = STRBUF_INIT;
+ strvec_push(&argv, svc->name);
+ if (strcmp(service_name, "git-upload-archive"))
+ strvec_push(&argv, "--stateless-rpc");
+ strvec_push(&argv, ".");
+
strbuf_reset(&buf);
strbuf_addf(&buf, "application/x-git-%s-request", svc->name);
check_content_type(hdr, buf.buf);
@@ -655,9 +661,9 @@ static void service_rpc(struct strbuf *hdr, char *service_name)
end_headers(hdr);
- argv[0] = svc->name;
- run_service(argv, svc->buffer_input);
+ run_service(argv.v, svc->buffer_input);
strbuf_release(&buf);
+ strvec_clear(&argv);
}
static int dead;
@@ -723,6 +729,7 @@ static struct service_cmd {
{"GET", "/objects/pack/pack-[0-9a-f]{64}\\.idx$", get_idx_file},
{"POST", "/git-upload-pack$", service_rpc},
+ {"POST", "/git-upload-archive$", service_rpc},
{"POST", "/git-receive-pack$", service_rpc}
};
diff --git a/t/t5003-archive-zip.sh b/t/t5003-archive-zip.sh
index fc499cdff0..6f85bd3463 100755
--- a/t/t5003-archive-zip.sh
+++ b/t/t5003-archive-zip.sh
@@ -239,4 +239,38 @@ check_zip with_untracked2
check_added with_untracked2 untracked one/untracked
check_added with_untracked2 untracked two/untracked
+# Test remote archive over HTTP protocol.
+#
+# Note: this should be the last part of this test suite, because
+# by including lib-httpd.sh, the test may end early if httpd tests
+# should not be run.
+#
+. "$TEST_DIRECTORY"/lib-httpd.sh
+start_httpd
+
+test_expect_success "setup for HTTP protocol" '
+ cp -R bare.git "$HTTPD_DOCUMENT_ROOT_PATH/bare.git" &&
+ git -C "$HTTPD_DOCUMENT_ROOT_PATH/bare.git" \
+ config http.uploadpack true &&
+ set_askpass user@host pass@host
+'
+
+setup_askpass_helper
+
+test_expect_success 'remote archive does not work with protocol v1' '
+ test_must_fail git -c protocol.version=1 archive \
+ --remote="$HTTPD_URL/auth/smart/bare.git" \
+ --output=remote-http.zip HEAD >actual 2>&1 &&
+ cat >expect <<-EOF &&
+ fatal: can${SQ}t connect to subservice git-upload-archive
+ EOF
+ test_cmp expect actual
+'
+
+test_expect_success 'archive remote http repository' '
+ test_must_fail git archive --remote="$HTTPD_URL/auth/smart/bare.git" \
+ --output=remote-http.zip HEAD &&
+ test_cmp_bin d.zip remote-http.zip
+'
+
test_done
--
2.43.0
^ permalink raw reply related
* [PATCH v6 3/6] transport-helper: protocol v2 supports upload-archive
From: Jiang Xin @ 2024-01-21 13:15 UTC (permalink / raw)
To: Git List, Junio C Hamano, Linus Arver; +Cc: Jiang Xin
In-Reply-To: <cover.1705841443.git.zhiyou.jx@alibaba-inc.com>
From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
We used to support only git-upload-pack service for protocol v2. In
order to support remote archive over HTTP/HTTPS protocols, add new
service support for git-upload-archive in protocol v2.
Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
---
transport-helper.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/transport-helper.c b/transport-helper.c
index 2e127d24a5..6fe9f4f208 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -628,7 +628,8 @@ static int process_connect_service(struct transport *transport,
ret = run_connect(transport, &cmdbuf);
} else if (data->stateless_connect &&
(get_protocol_version_config() == protocol_v2) &&
- !strcmp("git-upload-pack", name)) {
+ (!strcmp("git-upload-pack", name) ||
+ !strcmp("git-upload-archive", name))) {
strbuf_addf(&cmdbuf, "stateless-connect %s\n", name);
ret = run_connect(transport, &cmdbuf);
if (ret)
--
2.43.0
^ permalink raw reply related
* [PATCH v6 2/6] remote-curl: supports git-upload-archive service
From: Jiang Xin @ 2024-01-21 13:15 UTC (permalink / raw)
To: Git List, Junio C Hamano, Linus Arver; +Cc: Jiang Xin
In-Reply-To: <cover.1705841443.git.zhiyou.jx@alibaba-inc.com>
From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
Add new service (git-upload-archive) support in remote-curl, so we can
support remote archive over HTTP/HTTPS protocols. Differences between
git-upload-archive and other services:
1. The git-archive program does not expect to see protocol version and
capabilities when connecting to remote-helper, so do not send them
in remote-curl for the git-upload-archive service.
2. We need to detect protocol version by calling discover_refs().
Fallback to use the git-upload-pack service (which, like
git-upload-archive, is a read-only operation) to discover protocol
version.
Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
---
remote-curl.c | 14 +++++++++++---
1 file changed, 11 insertions(+), 3 deletions(-)
diff --git a/remote-curl.c b/remote-curl.c
index ef05752ca5..ce6cb8ac05 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -1447,8 +1447,14 @@ static int stateless_connect(const char *service_name)
* establish a stateless connection, otherwise we need to tell the
* client to fallback to using other transport helper functions to
* complete their request.
+ *
+ * The "git-upload-archive" service is a read-only operation. Fallback
+ * to use "git-upload-pack" service to discover protocol version.
*/
- discover = discover_refs(service_name, 0);
+ if (!strcmp(service_name, "git-upload-archive"))
+ discover = discover_refs("git-upload-pack", 0);
+ else
+ discover = discover_refs(service_name, 0);
if (discover->version != protocol_v2) {
printf("fallback\n");
fflush(stdout);
@@ -1486,9 +1492,11 @@ static int stateless_connect(const char *service_name)
/*
* Dump the capability listing that we got from the server earlier
- * during the info/refs request.
+ * during the info/refs request. This does not work with the
+ * "git-upload-archive" service.
*/
- write_or_die(rpc.in, discover->buf, discover->len);
+ if (strcmp(service_name, "git-upload-archive"))
+ write_or_die(rpc.in, discover->buf, discover->len);
/* Until we see EOF keep sending POSTs */
while (1) {
--
2.43.0
^ permalink raw reply related
* [PATCH v6 1/6] transport-helper: no connection restriction in connect_helper
From: Jiang Xin @ 2024-01-21 13:15 UTC (permalink / raw)
To: Git List, Junio C Hamano, Linus Arver; +Cc: Jiang Xin
In-Reply-To: <cover.1705841443.git.zhiyou.jx@alibaba-inc.com>
From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
When commit b236752a (Support remote archive from all smart transports,
2009-12-09) added "remote archive" support for "smart transports", it
was for transport that supports the ".connect" method. The
"connect_helper()" function protected itself from getting called for a
transport without the method before calling process_connect_service(),
which only worked with the ".connect" method.
Later, commit edc9caf7 (transport-helper: introduce stateless-connect,
2018-03-15) added a way for a transport without the ".connect" method
to establish a "stateless" connection in protocol v2, where
process_connect_service() was taught to handle the ".stateless_connect"
method, making the old protection too strict. But commit edc9caf7 forgot
to adjust this protection accordingly. Even at the time of commit
b236752a, this protection seemed redundant, since
process_connect_service() would return 0 if the connection could not be
established, and connect_helper() would still die() early.
Remove the restriction in connect_helper() and give the function
process_connect_service() the opportunity to establish a connection
using ".connect" or ".stateless_connect" for protocol v2. So we can
connect with a stateless-rpc and do something useful. E.g., in a later
commit, implements remote archive for a repository over HTTP protocol.
Helped-by: Junio C Hamano <gitster@pobox.com>
Helped-by: Linus Arver <linusa@google.com>
Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
---
transport-helper.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/transport-helper.c b/transport-helper.c
index 49811ef176..2e127d24a5 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -662,8 +662,6 @@ static int connect_helper(struct transport *transport, const char *name,
/* Get_helper so connect is inited. */
get_helper(transport);
- if (!data->connect)
- die(_("operation not supported by protocol"));
if (!process_connect_service(transport, name, exec))
die(_("can't connect to subservice %s"), name);
--
2.43.0
^ permalink raw reply related
* [PATCH v6 0/6] support remote archive via stateless transport
From: Jiang Xin @ 2024-01-21 13:15 UTC (permalink / raw)
To: Git List, Junio C Hamano, Linus Arver; +Cc: Jiang Xin
In-Reply-To: <cover.1705411391.git.zhiyou.jx@alibaba-inc.com>
From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
"git archive --remote=<remote>" learned to talk over the smart
http (aka stateless) transport.
# Changes since v5
Change commit messages.
# range-diff v5...v6
1: f3fef46c05 ! 1: d75e6d27ac transport-helper: no connection restriction in connect_helper
@@ Commit message
Later, commit edc9caf7 (transport-helper: introduce stateless-connect,
2018-03-15) added a way for a transport without the ".connect" method
- to establish a "stateless" connection in protocol-v2, where
+ to establish a "stateless" connection in protocol v2, where
process_connect_service() was taught to handle the ".stateless_connect"
method, making the old protection too strict. But commit edc9caf7 forgot
- to adjust this protection accordingly.
+ to adjust this protection accordingly. Even at the time of commit
+ b236752a, this protection seemed redundant, since
+ process_connect_service() would return 0 if the connection could not be
+ established, and connect_helper() would still die() early.
- Remove the restriction in the "connect_helper()" function and give the
- function "process_connect_service()" the opportunity to establish a
- connection using ".connect" or ".stateless_connect" for protocol v2. So
- we can connect with a stateless-rpc and do something useful. E.g., in a
- later commit, implements remote archive for a repository over HTTP
- protocol.
+ Remove the restriction in connect_helper() and give the function
+ process_connect_service() the opportunity to establish a connection
+ using ".connect" or ".stateless_connect" for protocol v2. So we can
+ connect with a stateless-rpc and do something useful. E.g., in a later
+ commit, implements remote archive for a repository over HTTP protocol.
Helped-by: Junio C Hamano <gitster@pobox.com>
Helped-by: Linus Arver <linusa@google.com>
2: 6be331b22d ! 2: 320526dc56 remote-curl: supports git-upload-archive service
@@ Commit message
Add new service (git-upload-archive) support in remote-curl, so we can
support remote archive over HTTP/HTTPS protocols. Differences between
- git-upload-archive and other serices:
+ git-upload-archive and other services:
1. The git-archive program does not expect to see protocol version and
capabilities when connecting to remote-helper, so do not send them
in remote-curl for the git-upload-archive service.
- 2. We need to detect protocol version by calling discover_refs(),
+ 2. We need to detect protocol version by calling discover_refs().
Fallback to use the git-upload-pack service (which, like
git-upload-archive, is a read-only operation) to discover protocol
version.
+ Helped-by: Linus Arver <linusa@google.com>
Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
## remote-curl.c ##
3: aabc8e1a2a ! 3: 72e575d28a transport-helper: protocol-v2 supports upload-archive
@@ Metadata
Author: Jiang Xin <zhiyou.jx@alibaba-inc.com>
## Commit message ##
- transport-helper: protocol-v2 supports upload-archive
+ transport-helper: protocol v2 supports upload-archive
- We used to support only git-upload-pack service for protocol-v2. In
+ We used to support only git-upload-pack service for protocol v2. In
order to support remote archive over HTTP/HTTPS protocols, add new
- service support for git-upload-archive in protocol-v2.
+ service support for git-upload-archive in protocol v2.
+ Helped-by: Linus Arver <linusa@google.com>
Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
## transport-helper.c ##
4: fdab4abb43 = 4: 390d13c074 http-backend: new rpc-service for git-upload-archive
5: 6ac0c8e105 ! 5: 1c9f7755d3 transport-helper: call do_take_over() in connect_helper
@@ Commit message
After successfully connecting to the smart transport by calling
process_connect_service() in connect_helper(), run do_take_over() to
replace the old vtable with a new one which has methods ready for the
- smart transport connection. This will fix the exit code of git-archive
+ smart transport connection. This fixes the exit code of git-archive
in test case "archive remote http repository" of t5003.
The connect_helper() function is used as the connect method of the
@@ Commit message
do_take_over(), it may fail to call transport_disconnect() in
run_remote_archiver() of "builtin/archive.c". This is because for a
stateless connection and a service like "git-upload-archive", the
- remote helper may receive a SIGPIPE signal and exit early. To have a
- graceful disconnect method by calling do_take_over() will solve this
- issue.
+ remote helper may receive a SIGPIPE signal and exit early. Call
+ do_take_over() to have a graceful disconnect method, so that we still
+ call transport_disconnect() even if the remote helper exits early.
Helped-by: Linus Arver <linusa@google.com>
Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
6: 423a89c593 = 6: 18bc8753df transport-helper: call do_take_over() in process_connect
---
Jiang Xin (6):
transport-helper: no connection restriction in connect_helper
remote-curl: supports git-upload-archive service
transport-helper: protocol v2 supports upload-archive
http-backend: new rpc-service for git-upload-archive
transport-helper: call do_take_over() in connect_helper
transport-helper: call do_take_over() in process_connect
http-backend.c | 13 ++++++++++---
remote-curl.c | 14 +++++++++++---
t/t5003-archive-zip.sh | 34 ++++++++++++++++++++++++++++++++++
transport-helper.c | 29 +++++++++++++----------------
4 files changed, 68 insertions(+), 22 deletions(-)
--
2.43.0
^ permalink raw reply
* Strange and broken behaviour when GIT_WORK_TREE is direct ancestor of GIT_DIR
From: LunarLambda @ 2024-01-21 7:33 UTC (permalink / raw)
To: git
Hello all,
I have run into very odd behaviour with git's --work-tree option/the
GIT_WORK_TREE environment variable.
I was experimenting with a workflow that involves targeted checkouts
of subdirectories in a repository,
commands of the form `git --work-tree=<dir> checkout @:<subdir> -- .`.
This worked fine, until I tried passing the parent directory
or any directory directly up the hierarchy (../, ../../, etc.) to
--work-tree, either as a relative or absolute path.
Almost all git commands behaved strangely or ceased to work at all.
I could not find any information in the git documentation or online
about this being a restriction in git, and given the
observed behaviour I believe it to be a bug, though I'm not sure if
it's something that should work or is something git should reject.
`git ls-tree` works as usual.
`git --work-tree=../empty-dir ls-tree` also works.
`git --work-tree=../nonexistent` also works.
`git --work-tree=../ ls-tree` prints nothing at all.
`git --work-tree=.. status` seems to behave mostly as it should
(reporting all files as having been deleted since the directory is
mostly empty),
but strangely reports the current directory (./) as an untracked file.
It correctly reports any other files in the parent directory as
untracked,
though it excludes the git repository itself from the list. It also
reported the paths of the files strangely,
prefixing them with ../ (or multiple ../ segments if the passed
--work-tree was several levels up).
This never happened with any other paths.
`git --work-tree=.. checkout <branch>` seems to write out the index
and working directory correctly, but while testing it would
sometimes update HEAD and sometimes not.
`git --work-tree=.. checkout [branch/treeish] -- <files>` completely
refused to work, always saying "pathspec did not match any files",
even for dot (.) and catch-all patterns.
This happens both with regular and bare repositories, and in all
repositories that I tested with.
I am using git 2.43.0 through the Arch Linux distribution.
My git configuration doesn't change any settings related to paths and
directories.
I apologize if this email is formatted suboptimally.
Thank you for your time.
^ permalink raw reply
* Re: [PATCH v5 0/6] support remote archive via stateless transport
From: Jiang Xin @ 2024-01-21 4:09 UTC (permalink / raw)
To: Linus Arver; +Cc: Git List, Junio C Hamano, Jiang Xin
In-Reply-To: <owly1qabhh19.fsf@fine.c.googlers.com>
On Sun, Jan 21, 2024 at 4:43 AM Linus Arver <linusa@google.com> wrote:
>
> Jiang Xin <worldhello.net@gmail.com> writes:
>
> This v5 looks good code-wise. I've made small suggestions to make the
> commit messages better, but they are just nits and you are free to
> ignore them.
Thanks for helping me refine commit messages. I will update them based
on your suggestions in next reroll.
> If you choose to reroll one more time, one additional thing you could do
> is to use the word "protocol_v2" in all commit messages because that's
> how that term looks like in the code (unless the "protocol-v2" string is
> already the standard term used elsewhere).
Will s/protocol_v2/protocol v2/
^ permalink raw reply
* [PATCH 4/4] completion: complete missing 'git log' options
From: Philippe Blain via GitGitGadget @ 2024-01-21 4:07 UTC (permalink / raw)
To: git; +Cc: Philippe Blain, Philippe Blain
In-Reply-To: <pull.1650.git.git.1705810071.gitgitgadget@gmail.com>
From: Philippe Blain <levraiphilippeblain@gmail.com>
Some options specific to 'git log' are missing from the Bash completion
script. Add them to _git_log.
Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
---
contrib/completion/git-completion.bash | 2 ++
1 file changed, 2 insertions(+)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index a7ae2cbe55b..2f1412d85ea 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2180,6 +2180,8 @@ _git_log ()
--no-walk --no-walk= --do-walk
--parents --children
--expand-tabs --expand-tabs= --no-expand-tabs
+ --clear-decorations --decorate-refs=
+ --decorate-refs-exclude=
$merge
$__git_diff_common_options
"
--
gitgitgadget
^ permalink raw reply related
* [PATCH 3/4] completion: complete --encoding
From: Philippe Blain via GitGitGadget @ 2024-01-21 4:07 UTC (permalink / raw)
To: git; +Cc: Philippe Blain, Philippe Blain
In-Reply-To: <pull.1650.git.git.1705810071.gitgitgadget@gmail.com>
From: Philippe Blain <levraiphilippeblain@gmail.com>
The option --encoding is supported by 'git log' and 'git show', so add
it to __git_log_show_options.
Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
---
contrib/completion/git-completion.bash | 1 +
1 file changed, 1 insertion(+)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index ccb17f4ad7b..a7ae2cbe55b 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2096,6 +2096,7 @@ __git_log_shortlog_options="
# Options accepted by log and show
__git_log_show_options="
--diff-merges --diff-merges= --no-diff-merges --dd --remerge-diff
+ --encoding=
"
__git_diff_merges_opts="off none on first-parent 1 separate m combined c dense-combined cc remerge r"
--
gitgitgadget
^ permalink raw reply related
* [PATCH 2/4] completion: complete --patch-with-raw
From: Philippe Blain via GitGitGadget @ 2024-01-21 4:07 UTC (permalink / raw)
To: git; +Cc: Philippe Blain, Philippe Blain
In-Reply-To: <pull.1650.git.git.1705810071.gitgitgadget@gmail.com>
From: Philippe Blain <levraiphilippeblain@gmail.com>
Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
---
contrib/completion/git-completion.bash | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 6108d523a11..ccb17f4ad7b 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1807,7 +1807,7 @@ __git_diff_common_options="--stat --numstat --shortstat --summary
--output= --output-indicator-context=
--output-indicator-new= --output-indicator-old=
--ws-error-highlight=
- --pickaxe-all --pickaxe-regex
+ --pickaxe-all --pickaxe-regex --patch-with-raw
"
# Options for diff/difftool
--
gitgitgadget
^ permalink raw reply related
* [PATCH 1/4] completion: complete missing rev-list options
From: Philippe Blain via GitGitGadget @ 2024-01-21 4:07 UTC (permalink / raw)
To: git; +Cc: Philippe Blain, Philippe Blain
In-Reply-To: <pull.1650.git.git.1705810071.gitgitgadget@gmail.com>
From: Philippe Blain <levraiphilippeblain@gmail.com>
Some options listed in rev-list-options.txt, and thus accepted by 'git
log' and friends, are missing from the Bash completion script.
Add them to __git_log_common_options.
Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
---
contrib/completion/git-completion.bash | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 8c40ade4941..6108d523a11 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2071,6 +2071,16 @@ __git_log_common_options="
--min-age= --until= --before=
--min-parents= --max-parents=
--no-min-parents --no-max-parents
+ --alternate-refs --ancestry-path
+ --author-date-order --basic-regexp
+ --bisect --boundary --exclude-first-parent-only
+ --exclude-hidden --extended-regexp
+ --fixed-strings --grep-reflog
+ --ignore-missing --left-only --perl-regexp
+ --reflog --regexp-ignore-case --remove-empty
+ --right-only --show-linear-break
+ --show-notes-by-default --show-pulls
+ --since-as-filter --single-worktree
"
# Options that go well for log and gitk (not shortlog)
__git_log_gitk_options="
--
gitgitgadget
^ permalink raw reply related
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