* [PATCH v3 2/3] pkt-line: memorize sideband fragment in reader
From: Jiang Xin @ 2023-10-04 13:18 UTC (permalink / raw)
To: Git List, Junio C Hamano, Jonathan Tan, Oswald Buddenhagen; +Cc: Jiang Xin
In-Reply-To: <cover.1696425168.git.zhiyou.jx@alibaba-inc.com>
From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
When we turn on the "use_sideband" field of the packet_reader,
"packet_reader_read()" will call the function "demultiplex_sideband()"
to parse and consume sideband messages. Sideband fragment which does not
end with "\r" or "\n" will be saved in the sixth parameter "scratch"
and it can be reused and be concatenated when parsing another sideband
message.
In "packet_reader_read()" function, the local variable "scratch" can
only be reused by subsequent sideband messages. But if there is a
payload message between two sideband fragments, the first fragment
which is saved in the local variable "scratch" will be lost.
To solve this problem, we can add a new field "scratch" in
packet_reader to memorize the sideband fragment across different calls
of "packet_reader_read()".
Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
---
pkt-line.c | 5 ++---
pkt-line.h | 3 +++
t/t0070-fundamental.sh | 2 +-
3 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/pkt-line.c b/pkt-line.c
index af83a19f4d..5943777a17 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -592,12 +592,11 @@ void packet_reader_init(struct packet_reader *reader, int fd,
reader->options = options;
reader->me = "git";
reader->hash_algo = &hash_algos[GIT_HASH_SHA1];
+ strbuf_init(&reader->scratch, 0);
}
enum packet_read_status packet_reader_read(struct packet_reader *reader)
{
- struct strbuf scratch = STRBUF_INIT;
-
if (reader->line_peeked) {
reader->line_peeked = 0;
return reader->status;
@@ -620,7 +619,7 @@ enum packet_read_status packet_reader_read(struct packet_reader *reader)
break;
if (demultiplex_sideband(reader->me, reader->status,
reader->buffer, reader->pktlen, 1,
- &scratch, &sideband_type))
+ &reader->scratch, &sideband_type))
break;
}
diff --git a/pkt-line.h b/pkt-line.h
index 954eec8719..be1010d34e 100644
--- a/pkt-line.h
+++ b/pkt-line.h
@@ -194,6 +194,9 @@ struct packet_reader {
/* hash algorithm in use */
const struct git_hash_algo *hash_algo;
+
+ /* hold temporary sideband message */
+ struct strbuf scratch;
};
/*
diff --git a/t/t0070-fundamental.sh b/t/t0070-fundamental.sh
index 1053913d2d..a927c665d6 100755
--- a/t/t0070-fundamental.sh
+++ b/t/t0070-fundamental.sh
@@ -81,7 +81,7 @@ test_expect_success 'unpack-sideband: --chomp-newline (default)' '
test_cmp expect-err err
'
-test_expect_failure 'unpack-sideband with demultiplex_sideband(), no chomp newline' '
+test_expect_success 'unpack-sideband with demultiplex_sideband(), no chomp newline' '
test_when_finished "rm -f expect-out expect-err" &&
test-tool pkt-line send-split-sideband >split-sideband &&
test-tool pkt-line unpack-sideband \
--
2.40.1.50.gf560bcc116.dirty
^ permalink raw reply related
* [PATCH v3 1/3] test-pkt-line: add option parser for unpack-sideband
From: Jiang Xin @ 2023-10-04 13:18 UTC (permalink / raw)
To: Git List, Junio C Hamano, Jonathan Tan, Oswald Buddenhagen; +Cc: Jiang Xin
In-Reply-To: <cover.1696425168.git.zhiyou.jx@alibaba-inc.com>
From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
We can use the test helper program "test-tool pkt-line" to test pkt-line
related functions. E.g.:
* Use "test-tool pkt-line send-split-sideband" to generate sideband
messages.
* We can pipe these generated sideband messages to command "test-tool
pkt-line unpack-sideband" to test packet_reader_read() function.
In order to make a complete test of the packet_reader_read() function,
add option parser for command "test-tool pkt-line unpack-sideband".
To remove newlines in sideband messages, we can use:
$ test-tool pkt-line unpack-sideband --chomp-newline
To preserve newlines in sideband messages, we can use:
$ test-tool pkt-line unpack-sideband --no-chomp-newline
To parse sideband messages using "demultiplex_sideband()" inside the
function "packet_reader_read()", we can use:
$ test-tool pkt-line unpack-sideband --reader-use-sideband
Add several new test cases in t0070. Among these test cases, we pipe
output of the "send-split-sideband" subcommand to the "unpack-sideband"
subcommand. We found two issues:
1. The two splitted sideband messages "Hello," and " world!\n" should
be concatenated together. But when we enabled the function
"demultiplex_sideband()" to parse sideband messages, the first part
of the splitted message ("Hello,") is lost.
2. The newline characters in sideband 2 (progress info) and sideband 3
(error message) should be preserved, but they are also trimmed.
Will fix the above two issues in subsequent commits.
Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
---
t/helper/test-pkt-line.c | 58 ++++++++++++++++++++++++++++++++++++----
t/t0070-fundamental.sh | 58 ++++++++++++++++++++++++++++++++++++++++
2 files changed, 111 insertions(+), 5 deletions(-)
diff --git a/t/helper/test-pkt-line.c b/t/helper/test-pkt-line.c
index f4d134a145..9aa35f7861 100644
--- a/t/helper/test-pkt-line.c
+++ b/t/helper/test-pkt-line.c
@@ -2,6 +2,7 @@
#include "test-tool.h"
#include "pkt-line.h"
#include "write-or-die.h"
+#include "parse-options.h"
static void pack_line(const char *line)
{
@@ -64,12 +65,33 @@ static void unpack(void)
}
}
-static void unpack_sideband(void)
+static void unpack_sideband(int argc, const char **argv)
{
struct packet_reader reader;
- packet_reader_init(&reader, 0, NULL, 0,
- PACKET_READ_GENTLE_ON_EOF |
- PACKET_READ_CHOMP_NEWLINE);
+ int options = PACKET_READ_GENTLE_ON_EOF;
+ int chomp_newline = 1;
+ int reader_use_sideband = 0;
+ const char *const unpack_sideband_usage[] = {
+ "test_tool unpack_sideband [options...]", NULL
+ };
+ struct option cmd_options[] = {
+ OPT_BOOL(0, "reader-use-sideband", &reader_use_sideband,
+ "set use_sideband bit for packet reader (Default: off)"),
+ OPT_BOOL(0, "chomp-newline", &chomp_newline,
+ "chomp newline in packet (Default: on)"),
+ OPT_END()
+ };
+
+ argc = parse_options(argc, argv, "", cmd_options, unpack_sideband_usage,
+ 0);
+ if (argc > 0)
+ usage_msg_opt(_("too many arguments"), unpack_sideband_usage,
+ cmd_options);
+
+ if (chomp_newline)
+ options |= PACKET_READ_CHOMP_NEWLINE;
+ packet_reader_init(&reader, 0, NULL, 0, options);
+ reader.use_sideband = reader_use_sideband;
while (packet_reader_read(&reader) != PACKET_READ_EOF) {
int band;
@@ -79,6 +101,16 @@ static void unpack_sideband(void)
case PACKET_READ_EOF:
break;
case PACKET_READ_NORMAL:
+ /*
+ * When the "use_sideband" field of the reader is turned
+ * on, sideband packets other than the payload have been
+ * parsed and consumed.
+ */
+ if (reader.use_sideband) {
+ write_or_die(1, reader.line, reader.pktlen - 1);
+ break;
+ }
+
band = reader.line[0] & 0xff;
if (band < 1 || band > 2)
continue; /* skip non-sideband packets */
@@ -97,15 +129,31 @@ static void unpack_sideband(void)
static int send_split_sideband(void)
{
+ const char *foo = "Foo.\n";
+ const char *bar = "Bar.\n";
const char *part1 = "Hello,";
const char *primary = "\001primary: regular output\n";
const char *part2 = " world!\n";
+ /* Each sideband message has a trailing newline character. */
+ send_sideband(1, 2, foo, strlen(foo), LARGE_PACKET_MAX);
+ send_sideband(1, 2, bar, strlen(bar), LARGE_PACKET_MAX);
+
+ /*
+ * One sideband message is divided into part1 and part2
+ * by the primary message.
+ */
send_sideband(1, 2, part1, strlen(part1), LARGE_PACKET_MAX);
packet_write(1, primary, strlen(primary));
send_sideband(1, 2, part2, strlen(part2), LARGE_PACKET_MAX);
packet_response_end(1);
+ /*
+ * The unpack_sideband() function above requires a flush
+ * packet to end parsing.
+ */
+ packet_flush(1);
+
return 0;
}
@@ -126,7 +174,7 @@ int cmd__pkt_line(int argc, const char **argv)
else if (!strcmp(argv[1], "unpack"))
unpack();
else if (!strcmp(argv[1], "unpack-sideband"))
- unpack_sideband();
+ unpack_sideband(argc - 1, argv + 1);
else if (!strcmp(argv[1], "send-split-sideband"))
send_split_sideband();
else if (!strcmp(argv[1], "receive-sideband"))
diff --git a/t/t0070-fundamental.sh b/t/t0070-fundamental.sh
index 574de34198..1053913d2d 100755
--- a/t/t0070-fundamental.sh
+++ b/t/t0070-fundamental.sh
@@ -53,4 +53,62 @@ test_expect_success 'missing sideband designator is reported' '
test_i18ngrep "missing sideband" err
'
+test_expect_success 'unpack-sideband: --no-chomp-newline' '
+ test_when_finished "rm -f expect-out expect-err" &&
+ test-tool pkt-line send-split-sideband >split-sideband &&
+ test-tool pkt-line unpack-sideband \
+ --no-chomp-newline <split-sideband >out 2>err &&
+ cat >expect-out <<-EOF &&
+ primary: regular output
+ EOF
+ cat >expect-err <<-EOF &&
+ Foo.
+ Bar.
+ Hello, world!
+ EOF
+ test_cmp expect-out out &&
+ test_cmp expect-err err
+'
+
+test_expect_success 'unpack-sideband: --chomp-newline (default)' '
+ test_when_finished "rm -f expect-out expect-err" &&
+ test-tool pkt-line send-split-sideband >split-sideband &&
+ test-tool pkt-line unpack-sideband \
+ --chomp-newline <split-sideband >out 2>err &&
+ printf "primary: regular output" >expect-out &&
+ printf "Foo.Bar.Hello, world!" >expect-err &&
+ test_cmp expect-out out &&
+ test_cmp expect-err err
+'
+
+test_expect_failure 'unpack-sideband with demultiplex_sideband(), no chomp newline' '
+ test_when_finished "rm -f expect-out expect-err" &&
+ test-tool pkt-line send-split-sideband >split-sideband &&
+ test-tool pkt-line unpack-sideband \
+ --reader-use-sideband \
+ --no-chomp-newline <split-sideband >out 2>err &&
+ cat >expect-out <<-EOF &&
+ primary: regular output
+ EOF
+ printf "remote: Foo. \n" >expect-err &&
+ printf "remote: Bar. \n" >>expect-err &&
+ printf "remote: Hello, world! \n" >>expect-err &&
+ test_cmp expect-out out &&
+ test_cmp expect-err err
+'
+
+test_expect_failure 'unpack-sideband with demultiplex_sideband(), chomp newline' '
+ test_when_finished "rm -f expect-out expect-err" &&
+ test-tool pkt-line send-split-sideband >split-sideband &&
+ test-tool pkt-line unpack-sideband \
+ --reader-use-sideband \
+ --chomp-newline <split-sideband >out 2>err &&
+ printf "primary: regular output" >expect-out &&
+ printf "remote: Foo. \n" >expect-err &&
+ printf "remote: Bar. \n" >>expect-err &&
+ printf "remote: Hello, world! \n" >>expect-err &&
+ test_cmp expect-out out &&
+ test_cmp expect-err err
+'
+
test_done
--
2.40.1.50.gf560bcc116.dirty
^ permalink raw reply related
* [PATCH v3 3/3] pkt-line: do not chomp newlines for sideband messages
From: Jiang Xin @ 2023-10-04 13:18 UTC (permalink / raw)
To: Git List, Junio C Hamano, Jonathan Tan, Oswald Buddenhagen; +Cc: Jiang Xin
In-Reply-To: <cover.1696425168.git.zhiyou.jx@alibaba-inc.com>
From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
When calling "packet_read_with_status()" to parse pkt-line encoded
packets, we can turn on the flag "PACKET_READ_CHOMP_NEWLINE" to chomp
newline character for each packet for better line matching. But when
receiving data and progress information using sideband, we should turn
off the flag "PACKET_READ_CHOMP_NEWLINE" to prevent mangling newline
characters from data and progress information.
When both the server and the client support "sideband-all" capability,
we have a dilemma that newline characters in negotiation packets should
be removed, but the newline characters in the progress information
should be left intact.
Add new flag "PACKET_READ_USE_SIDEBAND" for "packet_read_with_status()"
to prevent mangling newline characters in sideband messages.
Helped-by: Jonathan Tan <jonathantanmy@google.com>
Helped-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
---
pkt-line.c | 31 +++++++++++++++++++++++++++++--
pkt-line.h | 1 +
t/t0070-fundamental.sh | 2 +-
3 files changed, 31 insertions(+), 3 deletions(-)
diff --git a/pkt-line.c b/pkt-line.c
index 5943777a17..e9061e61a4 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -462,8 +462,32 @@ enum packet_read_status packet_read_with_status(int fd, char **src_buffer,
}
if ((options & PACKET_READ_CHOMP_NEWLINE) &&
- len && buffer[len-1] == '\n')
- len--;
+ len && buffer[len-1] == '\n') {
+ if (options & PACKET_READ_USE_SIDEBAND) {
+ int band = *buffer & 0xff;
+ switch (band) {
+ case 1:
+ /* Chomp newline for payload */
+ len--;
+ break;
+ case 2:
+ case 3:
+ /*
+ * Do not chomp newline for progress and error
+ * message.
+ */
+ break;
+ default:
+ /*
+ * Bad sideband, let's leave it to
+ * demultiplex_sideband() to catch this error.
+ */
+ break;
+ }
+ } else {
+ len--;
+ }
+ }
buffer[len] = 0;
if (options & PACKET_READ_REDACT_URI_PATH &&
@@ -602,6 +626,9 @@ enum packet_read_status packet_reader_read(struct packet_reader *reader)
return reader->status;
}
+ if (reader->use_sideband)
+ reader->options |= PACKET_READ_USE_SIDEBAND;
+
/*
* Consume all progress packets until a primary payload packet is
* received
diff --git a/pkt-line.h b/pkt-line.h
index be1010d34e..a7ff2e2f18 100644
--- a/pkt-line.h
+++ b/pkt-line.h
@@ -85,6 +85,7 @@ void packet_fflush(FILE *f);
#define PACKET_READ_DIE_ON_ERR_PACKET (1u<<2)
#define PACKET_READ_GENTLE_ON_READ_ERROR (1u<<3)
#define PACKET_READ_REDACT_URI_PATH (1u<<4)
+#define PACKET_READ_USE_SIDEBAND (1u<<5)
int packet_read(int fd, char *buffer, unsigned size, int options);
/*
diff --git a/t/t0070-fundamental.sh b/t/t0070-fundamental.sh
index a927c665d6..138c2becc1 100755
--- a/t/t0070-fundamental.sh
+++ b/t/t0070-fundamental.sh
@@ -97,7 +97,7 @@ test_expect_success 'unpack-sideband with demultiplex_sideband(), no chomp newli
test_cmp expect-err err
'
-test_expect_failure 'unpack-sideband with demultiplex_sideband(), chomp newline' '
+test_expect_success 'unpack-sideband with demultiplex_sideband(), chomp newline' '
test_when_finished "rm -f expect-out expect-err" &&
test-tool pkt-line send-split-sideband >split-sideband &&
test-tool pkt-line unpack-sideband \
--
2.40.1.50.gf560bcc116.dirty
^ permalink raw reply related
* Re: Is SANITIZE=leak make test unreliable for anyone else?
From: Jeff King @ 2023-10-04 13:21 UTC (permalink / raw)
To: Eric W. Biederman; +Cc: git
In-Reply-To: <878r8j2mu1.fsf@email.froward.int.ebiederm.org>
On Tue, Oct 03, 2023 at 08:33:26PM -0500, Eric W. Biederman wrote:
> My big question is:
>
> Do other people see random test failures when SANITIZE=leak is enabled?
>
> Is it just me?
Yes, I've seen this. You mentioned that you were testing with v2.42,
which lacks 370ef7e40d (test-lib: ignore uninteresting LSan output,
2023-08-28). Try using the current version of 'master', or just
cherry-picking that commit onto v2.42.
A few other tips to avoid confusing results (though they at least do not
vary from run to run):
- use the LEAK_LOG option, since you otherwise miss some cases (it
looks like you already are from what you posted above)
- gcc and clang sometimes produce different results. Right now I get
no leak from gcc on t9004, but clang reports one (I think clang is
right here)
- turn off compiler optimizations; we've had cases where code
reordering/removal creates false positives. Oh, hmm, I forgot we do
this by default since d3775de074 (Makefile: force -O0 when compiling
with SANITIZE=leak, 2022-10-18), so your v2.42 should be covered.
-Peff
^ permalink raw reply
* Re: [PATCH v2 2/3] transport-helper: run do_take_over in connect_helper
From: Jiang Xin @ 2023-10-04 14:00 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git List, Brandon Williams, Ilari Liusvaara, Jiang Xin
In-Reply-To: <xmqqil7yq6ms.fsf@gitster.g>
On Tue, Sep 26, 2023 at 5:34 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Jiang Xin <worldhello.net@gmail.com> writes:
>
> > 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.
>
> 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().
>
> And the only other caller of process_connect_service() is
> connect_helper(), so in that sense, making a call to do_take_over()
> when process_connect_service() succeeds in the helper does make
> things consistent. The connect_helper() function being static, the
> helper transport is the only transport that gets affected, but how
> has it been working without having this do_take_over() step? An
> obvious related question is if it has been working so far, would it
> break if we have do_take_over() added here?
The connect_helper() function is used as the connect method of the
vtable in "transport-helper.c", and we use the function
"transport_connect()" in "transport.c" to call this connect method of
vtable. The only place that we call transport_connect() to setup a
connection is in "builtin/archive.c". So it won't break others if we
add do_take_over() in connect_helper().
In fact, it was not "git archive" that made me discover this issue.
When I implemented a fetch proxy and added a new caller for
transport_connect(), I found that the HTTP protocol didn't work, so I
dug it out.
^ permalink raw reply
* Re: Is SANITIZE=leak make test unreliable for anyone else?
From: Eric W. Biederman @ 2023-10-04 14:19 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20231004132132.GC607079@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Tue, Oct 03, 2023 at 08:33:26PM -0500, Eric W. Biederman wrote:
>
>> My big question is:
>>
>> Do other people see random test failures when SANITIZE=leak is enabled?
>>
>> Is it just me?
>
> Yes, I've seen this. You mentioned that you were testing with v2.42,
> which lacks 370ef7e40d (test-lib: ignore uninteresting LSan output,
> 2023-08-28). Try using the current version of 'master', or just
> cherry-picking that commit onto v2.42.
>
> A few other tips to avoid confusing results (though they at least do not
> vary from run to run):
>
> - use the LEAK_LOG option, since you otherwise miss some cases (it
> looks like you already are from what you posted above)
>
> - gcc and clang sometimes produce different results. Right now I get
> no leak from gcc on t9004, but clang reports one (I think clang is
> right here)
>
> - turn off compiler optimizations; we've had cases where code
> reordering/removal creates false positives. Oh, hmm, I forgot we do
> this by default since d3775de074 (Makefile: force -O0 when compiling
> with SANITIZE=leak, 2022-10-18), so your v2.42 should be covered.
I just tried master, aka commit d0e8084c65cb ("The fourteenth batch").
What I see on a random failure looks like:
> make -C t/ all
> make[1]: Entering directory '/home/user/projects/git/git/t'
> rm -f -r 'test-results'
> GIT_TEST_EXT_CHAIN_LINT=0 && export GIT_TEST_EXT_CHAIN_LINT && make aggregate-results-and-cleanup
> make[2]: Entering directory '/home/user/projects/git/git/t'
> *** t0000-basic.sh ***
> Segmentation fault
> error: test_bool_env requires bool values both for $GIT_TEST_PASSING_SANITIZE_LEAK and for the default fallback
Which doesn't sound like anything you have described so I am guessing it
is something with my environment I need to track down.
Eric
^ permalink raw reply
* Re: Is SANITIZE=leak make test unreliable for anyone else?
From: Jeff King @ 2023-10-04 14:47 UTC (permalink / raw)
To: Eric W. Biederman; +Cc: git
In-Reply-To: <871qea31xf.fsf@email.froward.int.ebiederm.org>
On Wed, Oct 04, 2023 at 09:19:40AM -0500, Eric W. Biederman wrote:
> What I see on a random failure looks like:
>
> > make -C t/ all
> > make[1]: Entering directory '/home/user/projects/git/git/t'
> > rm -f -r 'test-results'
> > GIT_TEST_EXT_CHAIN_LINT=0 && export GIT_TEST_EXT_CHAIN_LINT && make aggregate-results-and-cleanup
> > make[2]: Entering directory '/home/user/projects/git/git/t'
> > *** t0000-basic.sh ***
> > Segmentation fault
> > error: test_bool_env requires bool values both for $GIT_TEST_PASSING_SANITIZE_LEAK and for the default fallback
>
> Which doesn't sound like anything you have described so I am guessing it
> is something with my environment I need to track down.
No, that seems different entirely. You'll have to figure out which
program is segfaulting and why (if you can see it in a script besides
t0000 you're probably better off, as that one is a maze of
tests-within-tests, since it is testing the test-harness itself).
Although the "error" you see maybe implies that it is failing early on
in test-lib.sh, when we are calling "test-tool env-helper". If that is
segfaulting there is probably something very wrong with your build.
-Peff
^ permalink raw reply
* [PATCH v3 0/4] support remote archive from stateless transport
From: Jiang Xin @ 2023-10-04 15:21 UTC (permalink / raw)
To: Git List, Junio C Hamano, Eric Sunshine; +Cc: Jiang Xin
In-Reply-To: <xmqqil7yq6ms.fsf@gitster.g>
From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
"git archive --remote=<remote>" learned to talk over the smart
http (aka stateless) transport.
range-diff v2...v3
1: 4497404900 = 1: e660fc79b6 transport-helper: no connection restriction in connect_helper
-: ---------- > 2: e3dc18caa9 transport-helper: call do_take_over() in process_connect
2: 9bfaa1a904 ! 3: 01699822c3 transport-helper: run do_take_over in connect_helper
@@ Metadata
Author: Jiang Xin <zhiyou.jx@alibaba-inc.com>
## Commit message ##
- transport-helper: run do_take_over in connect_helper
+ transport-helper: call do_take_over() in connect_helper
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.
+ 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.
- The subsequent commit introduces remote archive for a stateless-rpc
- connection. But 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 or a
- service like "git-upload-pack-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.
+ 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 or a service like "git-upload-pack-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.
+
+ The subsequent commit will introduce remote archive over a stateless-rpc
+ connection.
Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
3: 1e305394ee ! 4: a38ac182d6 archive: support remote archive from stateless transport
@@ Commit message
capabilities when connecting to remote-helper, so do not send them
in "remote-curl.c" for the "git-upload-archive" service.
+ Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
## http-backend.c ##
@@ http-backend.c: static void check_content_type(struct strbuf *hdr, const char *a
static void service_rpc(struct strbuf *hdr, char *service_name)
{
- const char *argv[] = {NULL, "--stateless-rpc", ".", NULL};
-+ const char *argv[4];
++ struct strvec argv = STRVEC_INIT;
struct rpc_service *svc = select_service(hdr, service_name);
struct strbuf buf = STRBUF_INIT;
-+ if (!strcmp(service_name, "git-upload-archive")) {
-+ argv[1] = ".";
-+ argv[2] = NULL;
-+ } else {
-+ argv[1] = "--stateless-rpc";
-+ argv[2] = ".";
-+ argv[3] = NULL;
-+ }
++ 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);
+@@ http-backend.c: 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;
@@ http-backend.c: static struct service_cmd {
{"GET", "/objects/pack/pack-[0-9a-f]{64}\\.idx$", get_idx_file},
---
Jiang Xin (4):
transport-helper: no connection restriction in connect_helper
transport-helper: call do_take_over() in process_connect
transport-helper: call do_take_over() in connect_helper
archive: support remote archive from stateless transport
http-backend.c | 15 +++++++++++----
remote-curl.c | 14 +++++++++++---
t/t5003-archive-zip.sh | 30 ++++++++++++++++++++++++++++++
transport-helper.c | 29 +++++++++++++----------------
4 files changed, 65 insertions(+), 23 deletions(-)
--
2.40.1.50.gf560bcc116.dirty
^ permalink raw reply
* [PATCH v3 1/4] transport-helper: no connection restriction in connect_helper
From: Jiang Xin @ 2023-10-04 15:21 UTC (permalink / raw)
To: Git List, Junio C Hamano, Eric Sunshine; +Cc: Jiang Xin
In-Reply-To: <cover.1696432593.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 did not work with such a transport.
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, which
process_connect_service() was taught to handle the "stateless"
connection, making the old safety valve in its caller that insisted
that ".connect" method must be defined too strict, and forgot to loosen
it.
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.
Helped-by: Junio C Hamano <gitster@pobox.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.40.1.50.gf560bcc116.dirty
^ permalink raw reply related
* [PATCH v3 3/4] transport-helper: call do_take_over() in connect_helper
From: Jiang Xin @ 2023-10-04 15:21 UTC (permalink / raw)
To: Git List, Junio C Hamano, Eric Sunshine; +Cc: Jiang Xin
In-Reply-To: <cover.1696432593.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.
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 or a service like "git-upload-pack-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.
The subsequent commit will introduce remote archive over a stateless-rpc
connection.
Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
---
transport-helper.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/transport-helper.c b/transport-helper.c
index 51088cc03a..3b036ae1ca 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -672,6 +672,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.40.1.50.gf560bcc116.dirty
^ permalink raw reply related
* [PATCH v3 4/4] archive: support remote archive from stateless transport
From: Jiang Xin @ 2023-10-04 15:21 UTC (permalink / raw)
To: Git List, Junio C Hamano, Eric Sunshine; +Cc: Jiang Xin
In-Reply-To: <cover.1696432593.git.zhiyou.jx@alibaba-inc.com>
From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
Even though we can establish a stateless connection, we still cannot
archive the remote repository using a stateless HTTP protocol. Try the
following steps to make it work.
1. Add support for "git-upload-archive" service in "http-backend".
2. Use the URL ".../info/refs?service=git-upload-pack" to detect the
protocol version, instead of use the "git-upload-archive" service.
3. "git-archive" does not expect to see protocol version and
capabilities when connecting to remote-helper, so do not send them
in "remote-curl.c" for the "git-upload-archive" service.
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
---
http-backend.c | 15 +++++++++++----
remote-curl.c | 14 +++++++++++---
t/t5003-archive-zip.sh | 30 ++++++++++++++++++++++++++++++
transport-helper.c | 3 ++-
4 files changed, 54 insertions(+), 8 deletions(-)
diff --git a/http-backend.c b/http-backend.c
index ff07b87e64..6a2c919839 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,7 +729,8 @@ static struct service_cmd {
{"GET", "/objects/pack/pack-[0-9a-f]{64}\\.idx$", get_idx_file},
{"POST", "/git-upload-pack$", service_rpc},
- {"POST", "/git-receive-pack$", service_rpc}
+ {"POST", "/git-receive-pack$", service_rpc},
+ {"POST", "/git-upload-archive$", service_rpc}
};
static int bad_request(struct strbuf *hdr, const struct service_cmd *c)
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) {
diff --git a/t/t5003-archive-zip.sh b/t/t5003-archive-zip.sh
index fc499cdff0..80123c1e06 100755
--- a/t/t5003-archive-zip.sh
+++ b/t/t5003-archive-zip.sh
@@ -239,4 +239,34 @@ check_zip with_untracked2
check_added with_untracked2 untracked one/untracked
check_added with_untracked2 untracked two/untracked
+. "$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_when_finished "rm -f d5.zip" &&
+ test_must_fail git -c protocol.version=1 archive \
+ --remote="$HTTPD_URL/auth/smart/bare.git" \
+ --output=d5.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_when_finished "rm -f d5.zip" &&
+ git archive --remote="$HTTPD_URL/auth/smart/bare.git" \
+ --output=d5.zip HEAD &&
+ test_cmp_bin d.zip d5.zip
+'
+
test_done
diff --git a/transport-helper.c b/transport-helper.c
index 3b036ae1ca..566f7473df 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.40.1.50.gf560bcc116.dirty
^ permalink raw reply related
* [PATCH v3 2/4] transport-helper: call do_take_over() in process_connect
From: Jiang Xin @ 2023-10-04 15:21 UTC (permalink / raw)
To: Git List, Junio C Hamano, Eric Sunshine; +Cc: Jiang Xin
In-Reply-To: <cover.1696432593.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().
It is safe to make a refactor by moving the call of do_take_over()
into the function process_connect().
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 2e127d24a5..51088cc03a 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -645,6 +645,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)
@@ -652,7 +653,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,
@@ -682,10 +686,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
@@ -1142,10 +1144,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,
@@ -1186,11 +1186,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);
}
@@ -1274,10 +1272,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.40.1.50.gf560bcc116.dirty
^ permalink raw reply related
* Re: Is SANITIZE=leak make test unreliable for anyone else?
From: Eric W. Biederman @ 2023-10-04 15:38 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20231004144734.GA1143669@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Wed, Oct 04, 2023 at 09:19:40AM -0500, Eric W. Biederman wrote:
>
>> What I see on a random failure looks like:
>>
>> > make -C t/ all
>> > make[1]: Entering directory '/home/user/projects/git/git/t'
>> > rm -f -r 'test-results'
>> > GIT_TEST_EXT_CHAIN_LINT=0 && export GIT_TEST_EXT_CHAIN_LINT && make aggregate-results-and-cleanup
>> > make[2]: Entering directory '/home/user/projects/git/git/t'
>> > *** t0000-basic.sh ***
>> > Segmentation fault
>> > error: test_bool_env requires bool values both for $GIT_TEST_PASSING_SANITIZE_LEAK and for the default fallback
>>
>> Which doesn't sound like anything you have described so I am guessing it
>> is something with my environment I need to track down.
>
> No, that seems different entirely. You'll have to figure out which
> program is segfaulting and why (if you can see it in a script besides
> t0000 you're probably better off, as that one is a maze of
> tests-within-tests, since it is testing the test-harness itself).
>
> Although the "error" you see maybe implies that it is failing early on
> in test-lib.sh, when we are calling "test-tool env-helper". If that is
> segfaulting there is probably something very wrong with your build.
Just to document what I am seeing it appears to be some odd interaction
with address space randomization.
If I run my make as: "setarch --addr-no-randomize make test"
I don't see coredumps any more.
Now to dig a deeper and see if I can figure out what about address space
randomization is making things break.
Eric
^ permalink raw reply
* Re: [PATCH] test-lib: make sure TEST_DIRECTORY has no trailing slash
From: Junio C Hamano @ 2023-10-04 16:19 UTC (permalink / raw)
To: Štěpán Němec; +Cc: git
In-Reply-To: <20231004113413+0200.161419-stepnem@smrk.net>
Štěpán Němec <stepnem@smrk.net> writes:
> Yes, actually, AFAICT just $(cd . && pwd) fixes things (and saves a few
> syscalls), and I agree this is a much better approach than my naive fix.
Actually I was still being silly. We sometimes do
val=$(cd there && do something there)
so that we can get the output from a command in a different
directory _without_ having to move our current directory. But the
point of this current topic is that we _need_ to convince the shell
that the path to our current directory is a canonical one without
trailing slash, so my silly 'cd "$(pwd)/."' (or your "cd .") should
be done outside the command expansion, or the canonicalized $PWD will
only appear inside the $() and the next reference to $(pwd) or $PWD
in the test script will still give the path with the trailing slash,
that is textually different from $TEST_DIRECTORY.
I wonder if this works better for you. We would be sure that $PWD
and $TEST_DIRECTORY (when the latter is not imported from the
environment) are the same, so "your cwd that does not end with /t
and has a trailing slash after it" would be gone. Any $PWD or $(pwd)
the tests refer to later in the step will also lack the unwanted
trailing slash. As long as "cd ." is sufficient to cause the shell
reexamine and canonicalize the $PWD, that is.
Thanks.
t/test-lib.sh | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git c/t/test-lib.sh w/t/test-lib.sh
index 1656c9eed0..a7045e028c 100644
--- c/t/test-lib.sh
+++ w/t/test-lib.sh
@@ -19,6 +19,11 @@
# t/ subdirectory and are run in 'trash directory' subdirectory.
if test -z "$TEST_DIRECTORY"
then
+ # It is reported that some shells spawned in tricky ways can
+ # give $PWD with a trailing slash. An explicit chdir hopefully
+ # would wake them out of their hallucination.
+ cd .
+
# ensure that TEST_DIRECTORY is an absolute path so that it
# is valid even if the current working directory is changed
TEST_DIRECTORY=$(pwd)
@@ -626,7 +631,6 @@ fi
# Protect ourselves from common misconfiguration to export
# CDPATH into the environment
unset CDPATH
-
unset GREP_OPTIONS
unset UNZIP
^ permalink raw reply related
* Re: What's cooking in git.git (Oct 2023, #01; Mon, 2)
From: Junio C Hamano @ 2023-10-04 16:28 UTC (permalink / raw)
To: Eric W. Biederman; +Cc: git, Eric W. Biederman
In-Reply-To: <875y3n4207.fsf@email.froward.int.ebiederm.org>
"Eric W. Biederman" <ebiederm@xmission.com> writes:
> I pushed v2 out precisely because it contains fixes that should have
> fixed all of the CI breakages.
>
> I am not really familiar with github but looking at the recent CI runs
> it appears since v2 landed the seen branch has been building cleanly.
https://github.com/git/git/actions/runs/6398848314 shows the CI run
with seen at 7052c9b and it seems it does have the
eb/hash-transition topic in it.
> I just don't want people to avoid reviewing it because it is that huge
> patchset that causes problems in seen.
Certainly.
I have seen people review patches that do not even compile, though,
so "problems in seen" may not be that much of an issue, compared to
how intimidating the large series looks like.
Thanks.
^ permalink raw reply
* Re: [PATCH v3] git-status.txt: fix minor asciidoc format issue
From: Junio C Hamano @ 2023-10-04 16:56 UTC (permalink / raw)
To: cousteau via GitGitGadget; +Cc: git, Javier Mora
In-Reply-To: <pull.1591.v3.git.1696386165616.gitgitgadget@gmail.com>
"cousteau via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Javier Mora <cousteaulecommandant@gmail.com>
>
> The list of additional XY values for submodules in short format
> isn't formatted consistently with the rest of the document.
> Format as list for consistency.
>
> Signed-off-by: Javier Mora <cousteaulecommandant@gmail.com>
> ---
> ...
> diff --git a/Documentation/git-status.txt b/Documentation/git-status.txt
> index b27d127b5e2..48f46eb2047 100644
> --- a/Documentation/git-status.txt
> +++ b/Documentation/git-status.txt
> @@ -246,10 +246,9 @@ U U unmerged, both modified
>
> Submodules have more state and instead report
>
> - M the submodule has a different HEAD than
> - recorded in the index
> - m the submodule has modified content
> - ? the submodule has untracked files
> +* 'M' = the submodule has a different HEAD than recorded in the index
> +* 'm' = the submodule has modified content
> +* '?' = the submodule has untracked files
>
> since modified content or untracked files in a submodule cannot be added
> via `git add` in the superproject to prepare a commit.
>
> base-commit: d0e8084c65cbf949038ae4cc344ac2c2efd77415
Perfect. Thanks.
^ permalink raw reply
* Re: [PATCH] test-lib: make sure TEST_DIRECTORY has no trailing slash
From: Štěpán Němec @ 2023-10-04 17:01 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqbkdes6lf.fsf@gitster.g>
On Wed, 04 Oct 2023 09:19:40 -0700
Junio C. Hamano wrote:
> Štěpán Němec <stepnem@smrk.net> writes:
>
>> Yes, actually, AFAICT just $(cd . && pwd) fixes things (and saves a few
>> syscalls), and I agree this is a much better approach than my naive fix.
>
> Actually I was still being silly. We sometimes do
>
> val=$(cd there && do something there)
>
> so that we can get the output from a command in a different
> directory _without_ having to move our current directory. But the
> point of this current topic is that we _need_ to convince the shell
> that the path to our current directory is a canonical one without
> trailing slash, so my silly 'cd "$(pwd)/."' (or your "cd .") should
> be done outside the command expansion, or the canonicalized $PWD will
> only appear inside the $() and the next reference to $(pwd) or $PWD
> in the test script will still give the path with the trailing slash,
> that is textually different from $TEST_DIRECTORY.
>
> I wonder if this works better for you. We would be sure that $PWD
> and $TEST_DIRECTORY (when the latter is not imported from the
> environment) are the same, so "your cwd that does not end with /t
> and has a trailing slash after it" would be gone. Any $PWD or $(pwd)
> the tests refer to later in the step will also lack the unwanted
> trailing slash. As long as "cd ." is sufficient to cause the shell
> reexamine and canonicalize the $PWD, that is.
>
> Thanks.
>
> t/test-lib.sh | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git c/t/test-lib.sh w/t/test-lib.sh
> index 1656c9eed0..a7045e028c 100644
> --- c/t/test-lib.sh
> +++ w/t/test-lib.sh
> @@ -19,6 +19,11 @@
> # t/ subdirectory and are run in 'trash directory' subdirectory.
> if test -z "$TEST_DIRECTORY"
> then
> + # It is reported that some shells spawned in tricky ways can
> + # give $PWD with a trailing slash. An explicit chdir hopefully
> + # would wake them out of their hallucination.
> + cd .
> +
> # ensure that TEST_DIRECTORY is an absolute path so that it
> # is valid even if the current working directory is changed
> TEST_DIRECTORY=$(pwd)
> @@ -626,7 +631,6 @@ fi
> # Protect ourselves from common misconfiguration to export
> # CDPATH into the environment
> unset CDPATH
> -
> unset GREP_OPTIONS
> unset UNZIP
Yes, this is even simpler and more obvious, although for some reason,
the subshell version works just as well, i.e., with just
TEST_DIRECTORY=$(cd . && pwd) (no cd in the parent) in test-lib.sh and
cat <<EOF >./tslash.sh
#!/bin/sh
test_description='yada yada'
. ./test-lib.sh
test_expect_success 'check TEST_DIRECTORY for trailing slash' '
echo "$TEST_DIRECTORY" &&
test "$TEST_DIRECTORY" = "${TEST_DIRECTORY%/}"
'
test_done
EOF
I get
; echo ${TEST_DIRECTORY-unset}
unset
; pwd
/home/stepnem/src/git/t/
; echo $PWD
/home/stepnem/src/git/t/
; sh ./tslash.sh -v
Initialized empty Git repository in /home/stepnem/src/git/t/trash directory.tslash/.git/
expecting success of slash.1 'check TEST_DIRECTORY for trailing slash':
echo "$TEST_DIRECTORY" &&
test "$TEST_DIRECTORY" = "${TEST_DIRECTORY%/}"
/home/stepnem/src/git/t
ok 1 - check TEST_DIRECTORY for trailing slash
# passed all 1 test(s)
1..1
; pwd
/home/stepnem/src/git/t/
; echo $PWD
/home/stepnem/src/git/t/
[and, needless to say, reverting the changes still triggers the panic:
; sh ./tslash.sh
PANIC: Running in a /home/stepnem/src/git/t/ that doesn't end in '/t'?
]
Go figure...
In any case, for what it's worth, any of your versions is
Tested-by: Štěpán Němec <stepnem@smrk.net>
Thanks,
Štěpán
^ permalink raw reply
* Re: [PATCH] test-lib: make sure TEST_DIRECTORY has no trailing slash
From: Junio C Hamano @ 2023-10-04 17:15 UTC (permalink / raw)
To: Štěpán Němec; +Cc: git
In-Reply-To: <20231004190140+0200.740775-stepnem@smrk.net>
Štěpán Němec <stepnem@smrk.net> writes:
> Yes, this is even simpler and more obvious, although for some reason,
> the subshell version works just as well, i.e., with just
> TEST_DIRECTORY=$(cd . && pwd) (no cd in the parent) in test-lib.sh and
>
> cat <<EOF >./tslash.sh
> #!/bin/sh
> test_description='yada yada'
>
> . ./test-lib.sh
>
> test_expect_success 'check TEST_DIRECTORY for trailing slash' '
> echo "$TEST_DIRECTORY" &&
> test "$TEST_DIRECTORY" = "${TEST_DIRECTORY%/}"
> '
>
> test_done
> EOF
If the only thing you checked was if TEST_DIRECTORY has or does not
have a trailing slash, then it is totally understandable how and why
the chdir inside subshell works. The value $(pwd) in the subshell
returns, that is assigned to TEST_DIRECTORY, is canonicalized. But
the outer shell still thinks $(pwd) is with trailing slash, and the
above does not test it.
test_expect_success 'check $PWD for trailing slash' '
echo "$PWD" &&
test "$PWD" = "${PWD%/}"
'
The primary reason why I said I was silly doing it in a subshell was
because I wanted to make sure that any test that refers to $PWD or
$(pwd) later in the code would not get upset the same way with
trailing slash after $PWD or $(pwd). As long as it is not overdone,
it is a good practice to consider possibilities of similar problems
triggered by the same root cause, which we may not have got bitten
by yet, and preventing it from happening with the same fix.
Thanks.
^ permalink raw reply
* Re: [PATCH] test-lib: make sure TEST_DIRECTORY has no trailing slash
From: Štěpán Němec @ 2023-10-04 17:40 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqttr6pavh.fsf@gitster.g>
On Wed, 04 Oct 2023 10:15:30 -0700
Junio C. Hamano wrote:
> Štěpán Němec <stepnem@smrk.net> writes:
>
>> Yes, this is even simpler and more obvious, although for some reason,
>> the subshell version works just as well, i.e., with just
>> TEST_DIRECTORY=$(cd . && pwd) (no cd in the parent) in test-lib.sh and
>>
>> cat <<EOF >./tslash.sh
>> #!/bin/sh
>> test_description='yada yada'
>>
>> . ./test-lib.sh
>>
>> test_expect_success 'check TEST_DIRECTORY for trailing slash' '
>> echo "$TEST_DIRECTORY" &&
>> test "$TEST_DIRECTORY" = "${TEST_DIRECTORY%/}"
>> '
>>
>> test_done
>> EOF
>
> If the only thing you checked was if TEST_DIRECTORY has or does not
> have a trailing slash, then it is totally understandable how and why
> the chdir inside subshell works. The value $(pwd) in the subshell
> returns, that is assigned to TEST_DIRECTORY, is canonicalized. But
> the outer shell still thinks $(pwd) is with trailing slash, and the
> above does not test it.
Oh my, of course... thanks.
>
> test_expect_success 'check $PWD for trailing slash' '
> echo "$PWD" &&
> test "$PWD" = "${PWD%/}"
> '
> The primary reason why I said I was silly doing it in a subshell was
> because I wanted to make sure that any test that refers to $PWD or
> $(pwd) later in the code would not get upset the same way with
> trailing slash after $PWD or $(pwd).
The $PWD inside the test is the trash directory, though, so that's not
the problematic $PWD with a trailing slash any more, so I guess this
problem can't really happen in a test that follows the '. ./test-lib.sh'
(which does cd -P "$TRASH_DIRECTORY") convention?
--
Štěpán
^ permalink raw reply
* [PATCH v2 0/2] attr: add attr.tree and attr.allowInvalidSource configs
From: John Cai via GitGitGadget @ 2023-10-04 18:18 UTC (permalink / raw)
To: git; +Cc: Jeff King, John Cai
In-Reply-To: <pull.1577.git.git.1695218431033.gitgitgadget@gmail.com>
44451a2e5e (attr: teach "--attr-source=" global option to "git", 2023-05-06)
provided the ability to pass in a treeish as the attr source. When a
revision does not resolve to a valid tree is passed, Git will die. At
GitLab, we server repositories as bare repos and would like to always read
attributes from the default branch, so we'd like to pass in HEAD as the
treeish to read gitattributes from on every command. In this context we
would not want Git to die if HEAD is unborn, like in the case of empty
repositories.
Instead of modifying the default behavior of --attr-source, create a pair of
configs attr.tree and attr.allowInvalidSource whereby an admin can configure
a ref for all commands to read gitattributes from, and another config that
relaxes the requirement that this treeish resolve to a valid tree.
Changes since v1:
* Added a commit to add attr.tree config
John Cai (2):
attr: add attr.tree for setting the treeish to read attributes from
attr: add attr.allowInvalidSource config to allow invalid revision
Documentation/config.txt | 2 ++
Documentation/config/attr.txt | 12 +++++++++
attr.c | 21 +++++++++++++--
t/t0003-attributes.sh | 49 +++++++++++++++++++++++++++++++++++
4 files changed, 82 insertions(+), 2 deletions(-)
create mode 100644 Documentation/config/attr.txt
base-commit: 1fc548b2d6a3596f3e1c1f8b1930d8dbd1e30bf3
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1577%2Fjohn-cai%2Fjc%2Fconfig-attr-invalid-source-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1577/john-cai/jc/config-attr-invalid-source-v2
Pull-Request: https://github.com/git/git/pull/1577
Range-diff vs v1:
-: ----------- > 1: 446bce03a96 attr: add attr.tree for setting the treeish to read attributes from
1: 06b9acf24ca ! 2: 52d9e180352 attr: attr.allowInvalidSource config to allow invalid revision
@@ Metadata
Author: John Cai <johncai86@gmail.com>
## Commit message ##
- attr: attr.allowInvalidSource config to allow invalid revision
+ attr: add attr.allowInvalidSource config to allow invalid revision
- 44451a2e5e (attr: teach "--attr-source=<tree>" global option to "git",
- 2023-05-06) provided the ability to pass in a treeish as the attr
- source. When a revision does not resolve to a valid tree is passed, Git
- will die. GitLab keeps bare repositories and always reads attributes
- from the default branch, so we pass in HEAD to --attr-source.
+ The previous commit provided the ability to pass in a treeish as the
+ attr source via the attr.tree config. The default behavior is that when
+ a revision does not resolve to a valid tree is passed, Git will die.
- With empty repositories however, HEAD does not point to a valid treeish,
- causing Git to die. This means we would need to check for a valid
- treeish each time. To avoid this, let's add a configuration that allows
- Git to simply ignore --attr-source if it does not resolve to a valid
- tree.
+ When HEAD is unborn however, it does not point to a valid treeish,
+ causing Git to die. In the context of serving repositories through bare
+ repositories, we'd like to be able to set attr.tree to HEAD and allow
+ cases where HEAD does not resolve to a valid tree to be treated as if
+ there were no treeish provided.
+
+ Add attr.allowInvalidSource that allows this.
Signed-off-by: John Cai <johncai86@gmail.com>
- ## Documentation/config.txt ##
-@@ Documentation/config.txt: other popular tools, and describe them in your documentation.
-
- include::config/advice.txt[]
-
-+include::config/attr.txt[]
+ ## Documentation/config/attr.txt ##
+@@ Documentation/config/attr.txt: attr.tree:
+ linkgit:gitattributes[5]. This is equivalent to setting the
+ `GIT_ATTR_SOURCE` environment variable, or passing in --attr-source to
+ the Git command.
+
- include::config/core.txt[]
-
- include::config/add.txt[]
-
- ## Documentation/config/attr.txt (new) ##
-@@
+attr.allowInvalidSource::
-+ If `--attr-source` cannot resolve to a valid tree object, ignore
-+ `--attr-source` instead of erroring out, and fall back to looking for
++ If `attr.tree` cannot resolve to a valid tree object, ignore
++ `attr.tree` instead of erroring out, and fall back to looking for
+ attributes in the default locations. Useful when passing `HEAD` into
+ `attr-source` since it allows `HEAD` to point to an unborn branch in
+ cases like an empty repository.
## attr.c ##
-@@ attr.c: static void compute_default_attr_source(struct object_id *attr_source)
+@@ attr.c: void set_git_attr_source(const char *tree_object_name)
+
+ static void compute_default_attr_source(struct object_id *attr_source)
+ {
++ int attr_source_from_config = 0;
++
+ if (!default_attr_source_tree_object_name)
+ default_attr_source_tree_object_name = getenv(GIT_ATTR_SOURCE_ENVIRONMENT);
+
+ if (!default_attr_source_tree_object_name) {
+ char *attr_tree;
+
+- if (!git_config_get_string("attr.tree", &attr_tree))
++ if (!git_config_get_string("attr.tree", &attr_tree)) {
++ attr_source_from_config = 1;
+ default_attr_source_tree_object_name = attr_tree;
++ }
+ }
+
if (!default_attr_source_tree_object_name || !is_null_oid(attr_source))
return;
- if (repo_get_oid_treeish(the_repository, default_attr_source_tree_object_name, attr_source))
- die(_("bad --attr-source or GIT_ATTR_SOURCE"));
-+
+ if (repo_get_oid_treeish(the_repository, default_attr_source_tree_object_name, attr_source)) {
+ int allow_invalid_attr_source = 0;
+
+ git_config_get_bool("attr.allowinvalidsource", &allow_invalid_attr_source);
+
-+ if (!allow_invalid_attr_source)
++ if (!(allow_invalid_attr_source && attr_source_from_config))
+ die(_("bad --attr-source or GIT_ATTR_SOURCE"));
+ }
}
@@ t/t0003-attributes.sh: test_expect_success 'bare repository: check that .gitattr
+ git init empty &&
+ test_must_fail git -C empty --attr-source=HEAD check-attr test -- f/path 2>err &&
+ test_cmp expect_err err &&
-+ git -C empty -c attr.allowInvalidSource=true --attr-source=HEAD check-attr test -- f/path >actual 2>err &&
++ git -C empty -c attr.tree=HEAD -c attr.allowInvalidSource=true check-attr test -- f/path >actual 2>err &&
+ test_must_be_empty err &&
+ test_cmp expect actual
+'
@@ t/t0003-attributes.sh: test_expect_success 'bare repository: check that .gitattr
+ git init empty &&
+ test_must_fail git -C empty --attr-source=refs/does/not/exist check-attr test -- f/path 2>err &&
+ test_cmp expect_err err &&
-+ git -C empty -c attr.allowInvalidSource=true --attr-source=refs/does/not/exist check-attr test -- f/path >actual 2>err &&
++ git -C empty -c attr.tree=refs/does/not/exist -c attr.allowInvalidSource=true check-attr test -- f/path >actual 2>err &&
+ test_must_be_empty err &&
+ test_cmp expect actual
+'
@@ t/t0003-attributes.sh: test_expect_success 'bare repository: check that .gitattr
+ git init empty &&
+ echo "f/path test=val" >empty/.gitattributes &&
+ echo "f/path: test: val" >expect &&
-+ git -C empty -c attr.allowInvalidSource=true --attr-source=HEAD check-attr test -- f/path >actual 2>err &&
++ git -C empty -c attr.tree=HEAD -c attr.allowInvalidSource=true check-attr test -- f/path >actual 2>err &&
+ test_must_be_empty err &&
+ test_cmp expect actual
+'
++
++test_expect_success 'attr.allowInvalidSource has no effect on --attr-source' '
++ test_when_finished rm -rf empty &&
++ echo $bad_attr_source_err >expect_err &&
++ echo "f/path: test: unspecified" >expect &&
++ git init empty &&
++ test_must_fail git -C empty -c attr.allowInvalidSource=true --attr-source=HEAD check-attr test -- f/path 2>err &&
++ test_cmp expect_err err
++'
+
test_expect_success 'bare repository: with --source' '
(
--
gitgitgadget
^ permalink raw reply
* [PATCH v2 1/2] attr: add attr.tree for setting the treeish to read attributes from
From: John Cai via GitGitGadget @ 2023-10-04 18:18 UTC (permalink / raw)
To: git; +Cc: Jeff King, John Cai, John Cai
In-Reply-To: <pull.1577.v2.git.git.1696443502.gitgitgadget@gmail.com>
From: John Cai <johncai86@gmail.com>
44451a2e5e (attr: teach "--attr-source=<tree>" global option to "git",
2023-05-06) provided the ability to pass in a treeish as the attr
source. In the context of serving Git repositories as bare repos like we
do at GitLab however, it would be easier to point --attr-source to HEAD
for all commands by setting it once.
Add a new config attr.tree that allows this.
Signed-off-by: John Cai <johncai86@gmail.com>
---
Documentation/config.txt | 2 ++
Documentation/config/attr.txt | 5 +++++
attr.c | 7 +++++++
t/t0003-attributes.sh | 4 ++++
4 files changed, 18 insertions(+)
create mode 100644 Documentation/config/attr.txt
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 229b63a454c..b1891c2b5af 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -371,6 +371,8 @@ other popular tools, and describe them in your documentation.
include::config/advice.txt[]
+include::config/attr.txt[]
+
include::config/core.txt[]
include::config/add.txt[]
diff --git a/Documentation/config/attr.txt b/Documentation/config/attr.txt
new file mode 100644
index 00000000000..e4f2122b7ab
--- /dev/null
+++ b/Documentation/config/attr.txt
@@ -0,0 +1,5 @@
+attr.tree:
+ A <tree-ish> to read gitattributes from instead of the worktree. See
+ linkgit:gitattributes[5]. This is equivalent to setting the
+ `GIT_ATTR_SOURCE` environment variable, or passing in --attr-source to
+ the Git command.
diff --git a/attr.c b/attr.c
index 71c84fbcf86..bb0d54eb967 100644
--- a/attr.c
+++ b/attr.c
@@ -1205,6 +1205,13 @@ static void compute_default_attr_source(struct object_id *attr_source)
if (!default_attr_source_tree_object_name)
default_attr_source_tree_object_name = getenv(GIT_ATTR_SOURCE_ENVIRONMENT);
+ if (!default_attr_source_tree_object_name) {
+ char *attr_tree;
+
+ if (!git_config_get_string("attr.tree", &attr_tree))
+ default_attr_source_tree_object_name = attr_tree;
+ }
+
if (!default_attr_source_tree_object_name || !is_null_oid(attr_source))
return;
diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh
index 26e082f05b4..6342187c751 100755
--- a/t/t0003-attributes.sh
+++ b/t/t0003-attributes.sh
@@ -40,6 +40,10 @@ attr_check_source () {
test_cmp expect actual &&
test_must_be_empty err
+ git $git_opts -c "attr.tree=$source" check-attr test -- "$path" >actual 2>err &&
+ test_cmp expect actual &&
+ test_must_be_empty err
+
GIT_ATTR_SOURCE="$source" git $git_opts check-attr test -- "$path" >actual 2>err &&
test_cmp expect actual &&
test_must_be_empty err
--
gitgitgadget
^ permalink raw reply related
* [PATCH v2 2/2] attr: add attr.allowInvalidSource config to allow invalid revision
From: John Cai via GitGitGadget @ 2023-10-04 18:18 UTC (permalink / raw)
To: git; +Cc: Jeff King, John Cai, John Cai
In-Reply-To: <pull.1577.v2.git.git.1696443502.gitgitgadget@gmail.com>
From: John Cai <johncai86@gmail.com>
The previous commit provided the ability to pass in a treeish as the
attr source via the attr.tree config. The default behavior is that when
a revision does not resolve to a valid tree is passed, Git will die.
When HEAD is unborn however, it does not point to a valid treeish,
causing Git to die. In the context of serving repositories through bare
repositories, we'd like to be able to set attr.tree to HEAD and allow
cases where HEAD does not resolve to a valid tree to be treated as if
there were no treeish provided.
Add attr.allowInvalidSource that allows this.
Signed-off-by: John Cai <johncai86@gmail.com>
---
Documentation/config/attr.txt | 7 ++++++
attr.c | 16 ++++++++++---
t/t0003-attributes.sh | 45 +++++++++++++++++++++++++++++++++++
3 files changed, 65 insertions(+), 3 deletions(-)
diff --git a/Documentation/config/attr.txt b/Documentation/config/attr.txt
index e4f2122b7ab..00113a4950e 100644
--- a/Documentation/config/attr.txt
+++ b/Documentation/config/attr.txt
@@ -3,3 +3,10 @@ attr.tree:
linkgit:gitattributes[5]. This is equivalent to setting the
`GIT_ATTR_SOURCE` environment variable, or passing in --attr-source to
the Git command.
+
+attr.allowInvalidSource::
+ If `attr.tree` cannot resolve to a valid tree object, ignore
+ `attr.tree` instead of erroring out, and fall back to looking for
+ attributes in the default locations. Useful when passing `HEAD` into
+ `attr-source` since it allows `HEAD` to point to an unborn branch in
+ cases like an empty repository.
diff --git a/attr.c b/attr.c
index bb0d54eb967..1a7ac39b9d1 100644
--- a/attr.c
+++ b/attr.c
@@ -1202,21 +1202,31 @@ void set_git_attr_source(const char *tree_object_name)
static void compute_default_attr_source(struct object_id *attr_source)
{
+ int attr_source_from_config = 0;
+
if (!default_attr_source_tree_object_name)
default_attr_source_tree_object_name = getenv(GIT_ATTR_SOURCE_ENVIRONMENT);
if (!default_attr_source_tree_object_name) {
char *attr_tree;
- if (!git_config_get_string("attr.tree", &attr_tree))
+ if (!git_config_get_string("attr.tree", &attr_tree)) {
+ attr_source_from_config = 1;
default_attr_source_tree_object_name = attr_tree;
+ }
}
if (!default_attr_source_tree_object_name || !is_null_oid(attr_source))
return;
- if (repo_get_oid_treeish(the_repository, default_attr_source_tree_object_name, attr_source))
- die(_("bad --attr-source or GIT_ATTR_SOURCE"));
+ if (repo_get_oid_treeish(the_repository, default_attr_source_tree_object_name, attr_source)) {
+ int allow_invalid_attr_source = 0;
+
+ git_config_get_bool("attr.allowinvalidsource", &allow_invalid_attr_source);
+
+ if (!(allow_invalid_attr_source && attr_source_from_config))
+ die(_("bad --attr-source or GIT_ATTR_SOURCE"));
+ }
}
static struct object_id *default_attr_source(void)
diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh
index 6342187c751..972b64496e7 100755
--- a/t/t0003-attributes.sh
+++ b/t/t0003-attributes.sh
@@ -346,6 +346,51 @@ test_expect_success 'bare repository: check that .gitattribute is ignored' '
)
'
+bad_attr_source_err="fatal: bad --attr-source or GIT_ATTR_SOURCE"
+
+test_expect_success 'attr.allowInvalidSource when HEAD is unborn' '
+ test_when_finished rm -rf empty &&
+ echo $bad_attr_source_err >expect_err &&
+ echo "f/path: test: unspecified" >expect &&
+ git init empty &&
+ test_must_fail git -C empty --attr-source=HEAD check-attr test -- f/path 2>err &&
+ test_cmp expect_err err &&
+ git -C empty -c attr.tree=HEAD -c attr.allowInvalidSource=true check-attr test -- f/path >actual 2>err &&
+ test_must_be_empty err &&
+ test_cmp expect actual
+'
+
+test_expect_success 'attr.allowInvalidSource when --attr-source points to non-existing ref' '
+ test_when_finished rm -rf empty &&
+ echo $bad_attr_source_err >expect_err &&
+ echo "f/path: test: unspecified" >expect &&
+ git init empty &&
+ test_must_fail git -C empty --attr-source=refs/does/not/exist check-attr test -- f/path 2>err &&
+ test_cmp expect_err err &&
+ git -C empty -c attr.tree=refs/does/not/exist -c attr.allowInvalidSource=true check-attr test -- f/path >actual 2>err &&
+ test_must_be_empty err &&
+ test_cmp expect actual
+'
+
+test_expect_success 'bad attr source defaults to reading .gitattributes file' '
+ test_when_finished rm -rf empty &&
+ git init empty &&
+ echo "f/path test=val" >empty/.gitattributes &&
+ echo "f/path: test: val" >expect &&
+ git -C empty -c attr.tree=HEAD -c attr.allowInvalidSource=true check-attr test -- f/path >actual 2>err &&
+ test_must_be_empty err &&
+ test_cmp expect actual
+'
+
+test_expect_success 'attr.allowInvalidSource has no effect on --attr-source' '
+ test_when_finished rm -rf empty &&
+ echo $bad_attr_source_err >expect_err &&
+ echo "f/path: test: unspecified" >expect &&
+ git init empty &&
+ test_must_fail git -C empty -c attr.allowInvalidSource=true --attr-source=HEAD check-attr test -- f/path 2>err &&
+ test_cmp expect_err err
+'
+
test_expect_success 'bare repository: with --source' '
(
cd bare.git &&
--
gitgitgadget
^ permalink raw reply related
* Re: [PATCH] test-lib: make sure TEST_DIRECTORY has no trailing slash
From: Junio C Hamano @ 2023-10-04 18:24 UTC (permalink / raw)
To: Štěpán Němec; +Cc: git
In-Reply-To: <20231004194046+0200.353534-stepnem@smrk.net>
Štěpán Němec <stepnem@smrk.net> writes:
> The $PWD inside the test is the trash directory, though, so that's not
> the problematic $PWD with a trailing slash any more, so I guess this
> problem can't really happen in a test that follows the '. ./test-lib.sh'
> (which does cd -P "$TRASH_DIRECTORY") convention?
Yeah, after the dot-include of test-lib.sh returns, everything
should be safe. What happens inside test-lib.sh (and worse yet,
inside future versions of that file) before that "cd -P" is what
the extra-carefulness protects against.
^ permalink raw reply
* Re: [PATCH v3 2/4] transport-helper: call do_take_over() in process_connect
From: Junio C Hamano @ 2023-10-04 18:29 UTC (permalink / raw)
To: Jiang Xin; +Cc: Git List, Eric Sunshine, Jiang Xin
In-Reply-To: <e3dc18caa91bd16d95dc7c2bbd0e6eceedefe636.1696432594.git.zhiyou.jx@alibaba-inc.com>
Jiang Xin <worldhello.net@gmail.com> writes:
> It is safe to make a refactor by moving the call of do_take_over()
> into the function process_connect().
"It is safe" only explains why it does not hurt, and does not
explain why it is a good idea to do so, though.
^ permalink raw reply
* Re: [PATCH v2 1/2] attr: add attr.tree for setting the treeish to read attributes from
From: Junio C Hamano @ 2023-10-04 19:58 UTC (permalink / raw)
To: John Cai via GitGitGadget; +Cc: git, Jeff King, John Cai
In-Reply-To: <446bce03a96836f35f94e9ef8548cf4a2b041ba8.1696443502.git.gitgitgadget@gmail.com>
"John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: John Cai <johncai86@gmail.com>
>
> 44451a2e5e (attr: teach "--attr-source=<tree>" global option to "git",
> 2023-05-06) provided the ability to pass in a treeish as the attr
> source. In the context of serving Git repositories as bare repos like we
> do at GitLab however, it would be easier to point --attr-source to HEAD
> for all commands by setting it once.
>
> Add a new config attr.tree that allows this.
Hmph, I wonder if we want to go all the way to emulate how the
mailmap.blob was done, including
- Default the value of attr.tree to HEAD in a bare repository;
- Notice but ignore errors if the attr.tree does not point at a
tree object, and pretend as if attr.tree specified an empty tree;
which does not seem to be in this patch. With such a change,
probably we do not even need [2/2] of the series, perhaps?
^ 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