* Re: [REGRESSION] uninitialized value $address in git send-email
From: Michael Strawbridge @ 2023-09-19 14:04 UTC (permalink / raw)
To: Bagas Sanjaya, Junio C Hamano, Luben Tuikov,
Ævar Arnfjörð Bjarmason, Emily Shaffer,
Doug Anderson
Cc: Git Mailing List
In-Reply-To: <ZQknHjKdGZV3vJpV@debian.me>
On 2023-09-19 00:44, Bagas Sanjaya wrote:
> On Mon, Sep 18, 2023 at 04:26:44PM -0400, Michael Strawbridge wrote:
>> Hi,
>>
>> Author of a8022c5f7b67 (send-email: expose header information to
>> git-send-email's sendemail-validate hook, 2023-04-19) here.
>>
>> On 2023-09-18 08:56, Bagas Sanjaya wrote:
>>> I triggered this issue on patch series with cover letter. To reproduce:
>>>
>>> 1. Clone git.git repo, then branch off:
>>>
>>> ```
>>> $ git clone https://github.com/git/git.git && cd git
>>> $ git checkout -b test
>>> ```
>>>
>>> 2. Make two dummy signed-off commits:
>>>
>>> ```
>>> $ echo test > test && git add test && git commit -s -m "test"
>>> $ echo "test test" >> test && git commit -a -s -m "test test"
>>> ```
>>>
>>> 3. Generate patch series:
>>>
>>> ```
>>> $ mkdir /tmp/test
>>> $ git format-patch -o /tmp/test --cover-letter main
>>> ```
>>>
>>> 4. Send the series to dummy address:
>>>
>>> ```
>>> $ git send-email --to="pi <pi@pi>" /tmp/test/*.patch
>>> ```
>> I tried to repro this today on my side. I can repro the error when
>> using the address "pi <pi@pi>" but that's not a valid email address and
>> so one would expect it to fail in the extract_valid_address_or_die
>> function with the error that you mention. As soon as I make the address
>> valid like "pi <pi@pi.com>", git send-email no longer complains.
>>
>> In your original case, are you trying to send email to an invalid email
>> address? Is it an alias by chance?
> I triggered this regression when I passed multiple addresses separated by comma
> (like `--to="foo <foo@acme.com>,bar <bar@acme.com>"`, but somehow I managed to
> reduce the trigger to one address only (in this case, "pi <pi@pi.com>"). As for
> multiple addresses part, let me know if I should post another regression
> report.
>
Hm. I'm not sure what to say. I have used the below docker container
as a test environment and don't seem to find issues with 'git send-email
--to="pi <pi@pi.com>" /email/test/*.patch' nor with 'git send-email
--to="foo <foo@acme.com>,bar <bar@acme.com>" /email/test/*.patch'.
Maybe if you could try the following test environment too and see if you
can reproduce it inside the docker container:
NOTE: I assume you install docker on your system
Step 1) Create folder with the below files inside
Dockerfile:
...
FROM debian:trixie
RUN apt-get update && \
apt-get install -y git git-email vim
WORKDIR /
RUN git clone https://github.com/git/git.git && \
cd git && \
git checkout -b test
#COPY git-send-email /usr/lib/git-core/git-send-email
RUN git config --global user.email "you@example.com"
RUN git config --global user.name "Your Name"
#specific error case
RUN cd git && echo '#!/bin/sh \n\
patatt sign --hook "${1}"' > .git/hooks/sendemail-validate
RUN cd git && echo test > test && git add test && git commit -s -m "test"
RUN cd git && echo "test test" >> test && git commit -a -s -m "test test"
RUN mkdir -p /email/test
RUN cd git && git format-patch -o /email/test --cover-letter master
RUN sed -i 's/\*\*\* SUBJECT HERE \*\*\*/test/'
/email/test/0000-cover-letter.patch
...
run.sh:
...
#!/bin/sh
sudo docker stop git-send-email-debug
sudo docker rm git-send-email-debug
sudo docker build -t git-send-email-debug:latest .
sudo docker run -it --name git-send-email-debug git-send-email-debug:latest
...
Step 2) Make run.sh executable and start run.sh to create docker
container shell. Inside the container's shell (will pop up
automatically) please try this:
git send-email --to="foo <foo@acme.com>,bar <bar@acme.com>"
/email/test/*.patch
Please let me know the results of the test case above and any other
things you try that have interesting results.
Thank you!
^ permalink raw reply
* Re: [PATCH 0/8] fsmonitor unused parameter cleanups
From: Jeff Hostetler @ 2023-09-19 13:34 UTC (permalink / raw)
To: Jeff King, git; +Cc: Jeff Hostetler, Eric DeCosta
In-Reply-To: <20230918222908.GA2659096@coredump.intra.peff.net>
On 9/18/23 6:29 PM, Jeff King wrote:
> Here are a few cleanups of the fsmonitor code to remove or annotate
> unused parameters (working towards my goal of making us compile clean
> with -Wunused-parameter). I think they should all be pretty
> non-controversial, but I'm cc-ing folks active in the area in case patch
> 2 steps on the toes of any unpublished works in progress.
>
> [1/8]: fsmonitor: prefer repo_git_path() to git_pathdup()
> [2/8]: fsmonitor/win32: drop unused parameters
> [3/8]: fsmonitor: mark some maybe-unused parameters
> [4/8]: fsmonitor/win32: mark unused parameter in fsm_os__incompatible()
> [5/8]: fsmonitor: mark unused parameters in stub functions
> [6/8]: fsmonitor/darwin: mark unused parameters in system callback
> [7/8]: fsmonitor: mark unused hashmap callback parameters
> [8/8]: run-command: mark unused parameters in start_bg_wait callbacks
>
> builtin/fsmonitor--daemon.c | 10 ++++++----
> compat/fsmonitor/fsm-health-darwin.c | 8 ++++----
> compat/fsmonitor/fsm-ipc-win32.c | 2 +-
> compat/fsmonitor/fsm-listen-darwin.c | 4 ++--
> compat/fsmonitor/fsm-listen-win32.c | 24 ++++++++++--------------
> compat/fsmonitor/fsm-path-utils-win32.c | 7 ++++---
> compat/fsmonitor/fsm-settings-win32.c | 2 +-
> fsmonitor-ipc.c | 10 +++++-----
> fsmonitor-settings.c | 3 ++-
> t/helper/test-simple-ipc.c | 3 ++-
> 10 files changed, 37 insertions(+), 36 deletions(-)
>
> -Peff
LGTM
Thanks,
Jeff
^ permalink raw reply
* [PATCH] fix: check parameters in json-write.c
From: mark via GitGitGadget @ 2023-09-19 11:54 UTC (permalink / raw)
To: git; +Cc: mark, wangsirun
From: wangsirun <wangsirun@zhidaoauto.com>
When I used the json-writer.c file as a lib, I found that
it often caused coredump errors, so I submitted this patch
Signed-off-by: sirun Wang <870355373@qq.com>
---
fix: check parameters in json-write.c
When I used the json-writer.c file as a lib, I found that it often
caused coredump errors, so I submitted this patch
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1576%2Fwangsirun%2Fjson-write-fix-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1576/wangsirun/json-write-fix-v1
Pull-Request: https://github.com/git/git/pull/1576
json-writer.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/json-writer.c b/json-writer.c
index 005c820aa42..23ba7046e5d 100644
--- a/json-writer.c
+++ b/json-writer.c
@@ -20,6 +20,11 @@ static void append_quoted_string(struct strbuf *out, const char *in)
{
unsigned char c;
+ if (!in || !*in) {
+ strbuf_addstr(out, "\"\"");
+ return;
+ }
+
strbuf_addch(out, '"');
while ((c = *in++) != '\0') {
if (c == '"')
base-commit: d4a83d07b8cc66d4afac2f33a8af729f2ba93bba
--
gitgitgadget
^ permalink raw reply related
* Re: [PATCH 2/2] parse-options: use and require int pointer for OPT_CMDMODE
From: Oswald Buddenhagen @ 2023-09-19 9:40 UTC (permalink / raw)
To: René Scharfe; +Cc: Git List, Jeff King, Junio C Hamano
In-Reply-To: <e6d8a291-03de-cfd3-3813-747fc2cad145@web.de>
On Sat, Sep 09, 2023 at 11:14:20PM +0200, René Scharfe wrote:
>Some uses of OPT_CMDMODE provide a pointer to an enum. It is
>dereferenced as an int pointer in parse-options.c::get_value(). These
>two types are incompatible, though
>
s/are/may be/ - c.f. https://en.cppreference.com/w/c/language/enum
>-- the storage size of an enum can vary between platforms.
>
here's a completely different perspective:
this is merely a theoretical problem, right? gcc for example won't
actually use non-ints unless -fshort-enums is supplied. so how about
simply adding a (configure) test to ensure that there is actually no
problem, and calling it a day?
regards
^ permalink raw reply
* Re: [PATCH v3 1/1] range-diff: treat notes like `log`
From: Kristoffer Haugsbakk @ 2023-09-19 9:12 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Schindelin, git, Denton Liu, Jeff King
In-Reply-To: <xmqqled3hsip.fsf@gitster.g>
On Tue, Sep 19, 2023, at 03:16, Junio C Hamano wrote:
> Will one of you tie the loose ends?
>
> Thanks.
I will this afternoon (CEST).
Cheers
--
Kristoffer Haugsbakk
^ permalink raw reply
* Re: [PATCH v3] revision: add `--ignore-missing-links` user option
From: Karthik Nayak @ 2023-09-19 8:45 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, me
In-Reply-To: <xmqqv8c7mq4o.fsf@gitster.g>
On Mon, Sep 18, 2023 at 5:56 PM Junio C Hamano <gitster@pobox.com> wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
> > This is to be expected, in my opinion. In terms of revision.c and
> > setting the `revs->ignore_missing_links` bit, the traversal will
> > go throw all objects (commits and otherwise) and call
> > `show_commit` or `show_object` on them.
>
> Yes. And the user can choose how to handle such an object here by
> telling finish_object__ma() with the --missing=<how> option, so
> letting them do so, instead of robbing the choice from them, would
> be a more flexible design here, right?
>
> > if a commit is
> > missing, git-rev-list(1) will still barf an error, but this error
>
> OK, yeah, I do see the need for setting the ignore-missing-links bit
> for what you are doing, and --missing and --ignore-missing-links are
> orthogonal options. Getting rid of the hardcoded skipping of
> finish_object__ma() would make sense from this angle, too.
Well. The only problem is that setting `ignore_missing_links` bit never calls
`show_commit` for missing commits (since commits are pre-parsed in revision.c).
So to keep that behavior consistent for non-commit objects, I hardcoded the
skipping of `finish_object__ma()` in `show_object`.
If I remove the hardcoding, it would mean that `--ignore-missing-links` would
skip missing commits but for non-commits objects, the user would have to pass
`--missing=allow-any` else rev-list would still error out with a
missing object error.
Don't you think this would be confusing for the user?
I'm happy to send a revised version removing this hardcoding if you still think
otherwise :)
^ permalink raw reply
* Re: [PATCH 2/2] parse-options: use and require int pointer for OPT_CMDMODE
From: René Scharfe @ 2023-09-19 7:47 UTC (permalink / raw)
To: phillip.wood, Junio C Hamano; +Cc: Jeff King, Oswald Buddenhagen, Git List
In-Reply-To: <0bf56c65-e59f-4290-8160-cce141f692d5@gmail.com>
Am 18.09.23 um 15:33 schrieb Phillip Wood:
> Hi René
>
> On 18/09/2023 10:28, René Scharfe wrote:
>> Here's a version that preserves the enums by using additional int
>> variables just for the parsing phase. No tricks. The diff is long, but
>> most changes aren't particularly complicated and the resulting code is
>> not that ugly. Except for builtin/am.c perhaps, which changes the
>> command mode value using a callback as well.
>>
>
>> diff --git a/builtin/am.c b/builtin/am.c
>> index 202040b62e..00930e2152 100644
>> --- a/builtin/am.c
>> +++ b/builtin/am.c
>> @@ -2270,13 +2270,14 @@ enum resume_type {
>>
>> struct resume_mode {
>> enum resume_type mode;
>> + int mode_int;
>> enum show_patch_type sub_mode;
>> };
>>
>> static int parse_opt_show_current_patch(const struct option *opt, const char *arg, int unset)
>> {
>> int *opt_value = opt->value;
>> - struct resume_mode *resume = container_of(opt_value, struct resume_mode, mode);
>> + struct resume_mode *resume = container_of(opt_value, struct resume_mode, mode_int);
>>
>> /*
>> * Please update $__git_showcurrentpatch in git-completion.bash
>> @@ -2300,12 +2301,12 @@ static int parse_opt_show_current_patch(const struct option *opt, const char *ar
>> "--show-current-patch", arg);
>> }
>>
>> - if (resume->mode == RESUME_SHOW_PATCH && new_value != resume->sub_mode)
>> + if (resume->mode_int == RESUME_SHOW_PATCH && new_value != resume->sub_mode)
>> return error(_("options '%s=%s' and '%s=%s' "
>> "cannot be used together"),
>> "--show-current-patch", "--show-current-patch", arg, valid_modes[resume->sub_mode]);
>>
>> - resume->mode = RESUME_SHOW_PATCH;
>> + resume->mode_int = RESUME_SHOW_PATCH;
>> resume->sub_mode = new_value;
>> return 0;
>> }
>
> Having "mode" and "mode_int" feels a bit fragile as only "mode_int"
> is valid while parsing the options but then we want to use "mode".
True. It feels a bit worse here than for the others because there the
variable is on the stack and here it's in a struct that is passed
around.
> I wonder if we could get Oswald's idea of using callbacks working in
> a reasonably ergonomic way with a couple of macros. We could add an
> new OPTION_SET_ENUM member to "enum parse_opt_type" that would take
> a setter function as well as the usual void *value. To set the value
> it would pass the value pointer and an integer value to the setter
> function. We could change OPT_CMDMODE to use OPTION_SET_ENUM and
> take the name of the enum as well as the integer value we want to set
> for that option. The name of the enum would be used to generate the
> name of the setter callback which would be defined with another
> macro. The macro to generate the setter would look like
>
> #define MAKE_CMDMODE_SETTER(name) \
> static void parse_cmdmode_ ## name (void * var, int value) {
> enum name *p = var;
> *p = value;
> }
>
> then OPT_CMDMODE would look like
>
> #define OPT_CMDMODE_F(s, l, v, h, n, i, f) { \
> .type = OPTION_SET_ENUM, \
> .short_name = (s), \
> .long_name = (l), \
> .value = (v), \
> .help = (h), \
> .flags = PARSE_OPT_CMDMODE|PARSE_OPT_NOARG|PARSE_OPT_NONEG | (f), \
> .defval = (i), \
> .enum_setter = parse_cmdmode_ ## n,
> }
> #define OPT_CMDMODE(s, l, v, h, n, i) OPT_CMDMODE_F(s, l, v, h, n, i, 0)
>
>
> Then in builtin/am.c at the top level we'd add
>
> MAKE_CMDMODE_SETTER(resume_type)
>
> and change the option definitions to look like
>
> OPT_CMDMODE(0, "continue", resume_type, &resume.mode, ...)
About half of the OPT_CMDMODE users use int, not enum. They'd have to
be converted. On the flipside the direct and indirect uses of
OPT_SET_INT_F with enums could be converted to an OPT_SET_ENUM_F based
on the above to avoid their use of incompatible pointers as well (e.g.
OPT_IPVERSION).
For uses of OPT_BIT and OPT_NEGBIT with enums a getter would be needed
as well, though (e.g. git ls-tree -d/-r/-t).
At this point I wonder if the existing callback mechanism would suffice.
Which brings me full circle to the topic of typed callbacks. Perhaps I
should introduce them first and come back to solving the int/enum issue
at the end of that journey.
René
^ permalink raw reply
* Re: [PATCH v2 06/10] parse-options: mark unused "opt" parameter in callbacks
From: René Scharfe @ 2023-09-19 7:42 UTC (permalink / raw)
To: Jeff King; +Cc: git, Junio C Hamano
In-Reply-To: <20230905070512.GC199565@coredump.intra.peff.net>
Am 05.09.23 um 09:05 schrieb Jeff King:
> On Sat, Sep 02, 2023 at 12:12:56PM +0200, René Scharfe wrote:
>
>> And I don't understand why the callback returns 1 (PARSE_OPT_NON_OPTION)
>> on error, but that's a different matter.
>
> Yeah, that doesn't make sense at all.
Actually it does: any non-zero return from a callback is interpreted as an
error. "return error(...);" would be shorter, but "error(...); return 1;"
is not wrong. Sorry for the noise!
René
^ permalink raw reply
* Re: [PATCH 2/2] parse-options: use and require int pointer for OPT_CMDMODE
From: René Scharfe @ 2023-09-19 7:41 UTC (permalink / raw)
To: Oswald Buddenhagen; +Cc: Junio C Hamano, Jeff King, Git List
In-Reply-To: <ZQgiD0ivfYRpSbnJ@ugly>
Am 18.09.23 um 12:10 schrieb Oswald Buddenhagen:
> On Mon, Sep 18, 2023 at 11:28:31AM +0200, René Scharfe wrote:
>>
>> return error(_("options '%s=%s' and '%s=%s' "
>> "cannot be used together"),
>
>> "--show-current-patch", "--show-current-patch", arg, valid_modes[resume->sub_mode]);
>>
> totally on a tangent: the argument order is bogus here.
> and the line wrapping is also funny.
Hah, good catch! Care to send a patch?
René
^ permalink raw reply
* [PATCH] pkt-line: do not chomp EOL for sideband progress info
From: Jiang Xin @ 2023-09-19 7:19 UTC (permalink / raw)
To: Git List, Junio C Hamano, Jonathan Tan; +Cc: Jiang Xin
From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
In the protocol negotiation stage, we need to turn on the flag
"PACKET_READ_CHOMP_NEWLINE" to chomp EOL for each packet line from
client or server. But when receiving data and progress information
using sideband, we will turn off the flag "PACKET_READ_CHOMP_NEWLINE"
to prevent mangling EOLs from data and progress information.
When both the server and the client support "sideband-all" capability,
we have a dilemma that EOLs in negotiation packets should be trimmed,
but EOLs in progress infomation should be leaved as is.
Move the logic of chomping EOLs from "packet_read_with_status()" to
"packet_reader_read()" can resolve this dilemma.
Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
---
pkt-line.c | 19 ++++++++++++++++---
1 file changed, 16 insertions(+), 3 deletions(-)
diff --git a/pkt-line.c b/pkt-line.c
index af83a19f4d..d6d08b6aa6 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -597,12 +597,18 @@ void packet_reader_init(struct packet_reader *reader, int fd,
enum packet_read_status packet_reader_read(struct packet_reader *reader)
{
struct strbuf scratch = STRBUF_INIT;
+ int options = reader->options;
if (reader->line_peeked) {
reader->line_peeked = 0;
return reader->status;
}
+ /* Do not chomp newlines for sideband progress and error messages */
+ if (reader->use_sideband && options & PACKET_READ_CHOMP_NEWLINE) {
+ options &= ~PACKET_READ_CHOMP_NEWLINE;
+ }
+
/*
* Consume all progress packets until a primary payload packet is
* received
@@ -615,7 +621,7 @@ enum packet_read_status packet_reader_read(struct packet_reader *reader)
reader->buffer,
reader->buffer_size,
&reader->pktlen,
- reader->options);
+ options);
if (!reader->use_sideband)
break;
if (demultiplex_sideband(reader->me, reader->status,
@@ -624,12 +630,19 @@ enum packet_read_status packet_reader_read(struct packet_reader *reader)
break;
}
- if (reader->status == PACKET_READ_NORMAL)
+ if (reader->status == PACKET_READ_NORMAL) {
/* Skip the sideband designator if sideband is used */
reader->line = reader->use_sideband ?
reader->buffer + 1 : reader->buffer;
- else
+
+ if ((reader->options & PACKET_READ_CHOMP_NEWLINE) &&
+ reader->buffer[reader->pktlen - 1] == '\n') {
+ reader->buffer[reader->pktlen - 1] = 0;
+ reader->pktlen--;
+ }
+ } else {
reader->line = NULL;
+ }
return reader->status;
}
--
2.40.1.49.g40e13c3520.dirty
^ permalink raw reply related
* [PATCH 2/2] archive: support remote archive from stateless transport
From: Jiang Xin @ 2023-09-19 6:41 UTC (permalink / raw)
To: Git List, Junio C Hamano, Ilari Liusvaara; +Cc: Jiang Xin
In-Reply-To: <20230919064156.13892-1-worldhello.net@gmail.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. Unable to access the URL ".../info/refs?service=git-upload-archive"
to detect the protocol version, use the "git-upload-pack" service
instead.
3. "git-archive" does not resolve the protocol version and capabilities
when connecting to remote-helper, so the remote-helper should not
send them.
4. "git-archive" may not be able to disconnect the stateless
connection. Run "do_take_over()" to take_over the transfer for
a graceful disconnect function.
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 | 5 ++++-
4 files changed, 58 insertions(+), 6 deletions(-)
diff --git a/http-backend.c b/http-backend.c
index ff07b87e64..ed3bed965a 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,19 @@ 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};
+ const char *argv[4];
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;
+ }
+
strbuf_reset(&buf);
strbuf_addf(&buf, "application/x-git-%s-request", svc->name);
check_content_type(hdr, buf.buf);
@@ -723,7 +733,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 2e127d24a5..91381be622 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)
@@ -668,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.40.1.49.g40e13c3520.dirty
^ permalink raw reply related
* [PATCH 1/2] transport-helper: no connection restriction in connect_helper
From: Jiang Xin @ 2023-09-19 6:41 UTC (permalink / raw)
To: Git List, Junio C Hamano, Ilari Liusvaara; +Cc: Jiang Xin
From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
For protocol-v2, "stateless-connection" can be used to establish a
stateless connection between the client and the server, but trying to
establish http connection by calling "transport->vtable->connect" will
fail. This restriction was first introduced in commit b236752a87
(Support remote archive from all smart transports, 2009-12-09) by
adding a limitation in the "connect_helper()" function.
Remove the restriction in the "connect_helper()" function and use the
logic in the "process_connect_service()" function to check the protocol
version and service name. By this way, we can make a connection and do
something useful. E.g., in a later commit, implements remote archive
for a repository over HTTP protocol.
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.49.g40e13c3520.dirty
^ permalink raw reply related
* Re: [REGRESSION] uninitialized value $address in git send-email
From: Bagas Sanjaya @ 2023-09-19 4:44 UTC (permalink / raw)
To: Michael Strawbridge, Junio C Hamano, Luben Tuikov,
Ævar Arnfjörð Bjarmason, Emily Shaffer,
Doug Anderson
Cc: Git Mailing List
In-Reply-To: <8fcd6d2a-2c16-adf9-da1f-6b9d6fdcc87a@amd.com>
[-- Attachment #1: Type: text/plain, Size: 1888 bytes --]
On Mon, Sep 18, 2023 at 04:26:44PM -0400, Michael Strawbridge wrote:
> Hi,
>
> Author of a8022c5f7b67 (send-email: expose header information to
> git-send-email's sendemail-validate hook, 2023-04-19) here.
>
> On 2023-09-18 08:56, Bagas Sanjaya wrote:
> > I triggered this issue on patch series with cover letter. To reproduce:
> >
> > 1. Clone git.git repo, then branch off:
> >
> > ```
> > $ git clone https://github.com/git/git.git && cd git
> > $ git checkout -b test
> > ```
> >
> > 2. Make two dummy signed-off commits:
> >
> > ```
> > $ echo test > test && git add test && git commit -s -m "test"
> > $ echo "test test" >> test && git commit -a -s -m "test test"
> > ```
> >
> > 3. Generate patch series:
> >
> > ```
> > $ mkdir /tmp/test
> > $ git format-patch -o /tmp/test --cover-letter main
> > ```
> >
> > 4. Send the series to dummy address:
> >
> > ```
> > $ git send-email --to="pi <pi@pi>" /tmp/test/*.patch
> > ```
>
> I tried to repro this today on my side. I can repro the error when
> using the address "pi <pi@pi>" but that's not a valid email address and
> so one would expect it to fail in the extract_valid_address_or_die
> function with the error that you mention. As soon as I make the address
> valid like "pi <pi@pi.com>", git send-email no longer complains.
>
> In your original case, are you trying to send email to an invalid email
> address? Is it an alias by chance?
I triggered this regression when I passed multiple addresses separated by comma
(like `--to="foo <foo@acme.com>,bar <bar@acme.com>"`, but somehow I managed to
reduce the trigger to one address only (in this case, "pi <pi@pi.com>"). As for
multiple addresses part, let me know if I should post another regression
report.
--
An old man doll... just what I always wanted! - Clara
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH] diff --stat: add config option to limit filename width
From: Dragan Simic @ 2023-09-19 1:27 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqttrrl9m2.fsf@gitster.g>
On 2023-09-18 18:38, Junio C Hamano wrote:
> Dragan Simic <dsimic@manjaro.org> writes:
>
>> Just checking, do you want me to perform any improvements to this
>> patch, so you can have it pulled into one of your trees?
>
> I do not think of any outstanding issues I spotted on this change.
> Thanks for pinging.
Great, thanks.
^ permalink raw reply
* Re: [PATCH v3 1/1] range-diff: treat notes like `log`
From: Junio C Hamano @ 2023-09-19 1:16 UTC (permalink / raw)
To: Kristoffer Haugsbakk; +Cc: Johannes Schindelin, git, Denton Liu, Jeff King
In-Reply-To: <c7d1e196-9521-45a7-b41c-80499f19f546@app.fastmail.com>
"Kristoffer Haugsbakk" <code@khaugsbakk.name> writes:
> On Thu, Sep 14, 2023, at 10:29, Johannes Schindelin wrote:
>>> [...]
>>
>> Right, `-G --notes` is a good argument to rethink this.
>>
>> A much more surgical way to address the issue at hand might be this
>> (Kristoffer, what do you think? It's missing documentation for the
>> newly-introduced `--show-notes-by-default` option, but you get the idea):
>
> Looks good to me. It seems like an explicit argument is the only way to
> make this work.
Will one of you tie the loose ends?
Thanks.
^ permalink raw reply
* Re: [PATCH] subtree: fix split processing with multiple subtrees present
From: Junio C Hamano @ 2023-09-19 1:04 UTC (permalink / raw)
To: Zach FettersMoore via GitGitGadget; +Cc: git, Zach FettersMoore
In-Reply-To: <pull.1587.git.1695067516192.gitgitgadget@gmail.com>
"Zach FettersMoore via GitGitGadget" <gitgitgadget@gmail.com>
writes:
> In the diagram below, 'M' represents the mainline repo branch, 'A'
> represents one subtree, and 'B' represents another. M3 and B1 represent
> a split commit for subtree B that was created from commit M4. M2 and A1
> represent a split commit made from subtree A that was also created
> based on changes back to and including M4. M1 represents new changes to
> the repo, in this scenario if you try to run a 'git subtree split
> --rejoin' for subtree B, commits M1, M2, and A1, will be included in
> the processing of changes for the new split commit since the last
> split/rejoin for subtree B was at M3. The issue is that by having A1
> included in this processing the command ends up needing to processing
> every commit down tree A even though none of that is needed or relevant
> to the current command and result.
>
> M1
> | \ \
> M2 | |
> | A1 |
> M3 | |
> | | B1
> M4 | |
The above paragraph explains which different things you drew in the
diagram are representing, but it is not clear how they relate to
each other. Do they for example depict parent-child commit
relationship? What are the wide gaps between these three tracks and
what are the short angled lines leaning to the left near the tip?
Is the time/topology flowing from bottom to top?
> diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
> index e0c5d3b0de6..e9250dfb019 100755
> --- a/contrib/subtree/git-subtree.sh
> +++ b/contrib/subtree/git-subtree.sh
> @@ -778,12 +778,29 @@ ensure_valid_ref_format () {
> die "fatal: '$1' does not look like a ref"
> }
>
> +# Usage: check if a commit from another subtree should be ignored from processing for splits
Way overlong line. Please split them accordingly. I won't comment
on what CodingGuidelines tells us already, in this review, but have
a few comments here:
> +should_ignore_subtree_commit () {
> + if [ "$(git log -1 --grep="git-subtree-dir:" $1)" ]
> + then
> + if [[ -z "$(git log -1 --grep="git-subtree-mainline:" $1)" && -z "$(git log -1 --grep="git-subtree-dir: $dir$" $1)" ]]
Here $dir is a free variable that comes from outside. The caller
does not supply it as a parameter to this function (and the caller
does not receive it as its parameter from its caller). Yet the file
as a whole seems to liberally make assignments to it ("git grep dir="
on the file counts 7 assignments). Are we sure we are looking for
the right $dir in this particular grep?
Side note: I am not familiar with this part of the code at
all, so do not take it as "here is a bug", but more as "this
smells error prone."
Also can $dir have regular expressions special characters? "The
existing code and new code alike, git-subtree is not prepared to
handle directory names with RE special characters well at all, so
do not use them if you do not want your history broken" is an
acceptable answer.
The caller of this function process_split_commit is cmd_split and
process_split_commit (hence this function) is called repeatedly
inside a loop. This function makes a traversal over the entire
history for each and every iteration in "good" cases where there is
no 'mainline' or 'subtree-dir' commits for the given $dir.
I wonder if it is more efficient to enumerate all commits that hits
these grep criteria in the cmd_split before it starts to call
process_split_commit repeatedly. If it knows which commit can be
ignored beforehand, it can skip and not call process_split_commit,
no?
> + then
> + return 0
> + fi
> + fi
> + return 1
> +}
> +
> # Usage: process_split_commit REV PARENTS
> process_split_commit () {
> assert test $# = 2
> local rev="$1"
> local parents="$2"
These seem to assume that $1 and $2 can have $IFS in them, so
shouldn't ...
> + if should_ignore_subtree_commit $rev
... this call too enclose $rev inside a pair of double-quotes for
consistency? We know the loop in the cmd_split that calls this
function is reading from "rev-list --parents" and $rev is a 40-hex
commit object name (and $parents can have more than one 40-hex
commit object names separated with SP), so it is safe to leave $rev
unquoted, but it pays to be consistent to help make the code more
readable.
> + then
> + return
> + fi
> +
> if test $indent -eq 0
> then
> revcount=$(($revcount + 1))
>
> base-commit: bda494f4043963b9ec9a1ecd4b19b7d1cd9a0518
^ permalink raw reply
* Re: [PATCH] subtree: fix split processing with multiple subtrees present
From: Junio C Hamano @ 2023-09-18 23:31 UTC (permalink / raw)
To: Zach FettersMoore via GitGitGadget; +Cc: git, Zach FettersMoore
In-Reply-To: <pull.1587.git.1695067516192.gitgitgadget@gmail.com>
"Zach FettersMoore via GitGitGadget" <gitgitgadget@gmail.com>
writes:
> contrib/subtree/git-subtree.sh | 17 +++++++++++++++++
> 1 file changed, 17 insertions(+)
>
> diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
> index e0c5d3b0de6..e9250dfb019 100755
> --- a/contrib/subtree/git-subtree.sh
> +++ b/contrib/subtree/git-subtree.sh
> @@ -778,12 +778,29 @@ ensure_valid_ref_format () {
> die "fatal: '$1' does not look like a ref"
> }
>
> +# Usage: check if a commit from another subtree should be ignored from processing for splits
> +should_ignore_subtree_commit () {
> + if [ "$(git log -1 --grep="git-subtree-dir:" $1)" ]
> + then
> + if [[ -z "$(git log -1 --grep="git-subtree-mainline:" $1)" && -z "$(git log -1 --grep="git-subtree-dir: $dir$" $1)" ]]
> + then
> + return 0
> + fi
> + fi
> + return 1
> +}
>
> # Usage: process_split_commit REV PARENTS
> process_split_commit () {
> assert test $# = 2
> local rev="$1"
> local parents="$2"
>
> + if should_ignore_subtree_commit $rev
> + then
> + return
> + fi
> +
Please do not violate Documentation/CodingGuidelines for our shell
scripted Porcelain, even if it is a script in contrib/ and also
please avoid bash-isms.
Also doesn't "subtree" have its own test? If this change is a fix
for some problem(s), can we have a test or two that demonstrate how
the current code without the patch is broken?
Thanks.
^ permalink raw reply
* Re: [PATCH 1/3] refs: push lock management into packed backend
From: Junio C Hamano @ 2023-09-18 22:46 UTC (permalink / raw)
To: Han-Wen Nienhuys via GitGitGadget; +Cc: git, Han-Wen Nienhuys, Han-Wen Nienhuys
In-Reply-To: <dea0fbb139a82fe449a7edab6a8f445ce763d0c0.1695059978.git.gitgitgadget@gmail.com>
"Han-Wen Nienhuys via GitGitGadget" <gitgitgadget@gmail.com> writes:
> struct ref_transaction *tr;
> + int ret = 0;
> assert(err);
>
> CALLOC_ARRAY(tr, 1);
> tr->ref_store = refs;
> +
> + if (refs->be->transaction_begin)
> + ret = refs->be->transaction_begin(refs, tr, err);
> + if (ret) {
> + free(tr);
> + return NULL;
> + }
> return tr;
> }
This looks a bit more convoluted than necessary. Is it the same as
if (refs->be->transaction_begin &&
refs->be->transaction_begin(refs, tr, err))
FREE_AND_NULL(tr);
> + if (backend_data->packed_transaction) {
> + if (backend_data->packed_transaction_needed) {
> + ret = ref_transaction_commit(packed_transaction, err);
> + if (ret)
> + goto cleanup;
> + /* TODO: leaks on error path. */
> + ref_transaction_free(packed_transaction);
> + packed_transaction = NULL;
> + backend_data->packed_transaction = NULL;
> + } else {
If it were just a matter of flipping the early return and freeing of
the transaction before going to clean-up, then that would have been
less effort than leaving the TODO: comment. What other things are
needed to plug this leak?
^ permalink raw reply
* Re: [PATCH 0/3] Simple reftable backend
From: Junio C Hamano @ 2023-09-18 22:43 UTC (permalink / raw)
To: Han-Wen Nienhuys via GitGitGadget; +Cc: git, Han-Wen Nienhuys
In-Reply-To: <pull.1574.git.git.1695059978.gitgitgadget@gmail.com>
"Han-Wen Nienhuys via GitGitGadget" <gitgitgadget@gmail.com> writes:
> I think the first part ("refs: push lock management into packed backend") is
> a good improvement overall, and could be landed separately without much
> tweaking.
It looked quite nice to remove more knowledge specific to the
.git/packed-refs backend from the files backend, and instead have
the files backend rely more on the backend-provided methods to make
it work together with whatever backend happens to be in use. There
were a few minor things I found puzzling, but other than that, the
two patches looked mostly sensible.
^ permalink raw reply
* [PATCH 8/8] run-command: mark unused parameters in start_bg_wait callbacks
From: Jeff King @ 2023-09-18 22:33 UTC (permalink / raw)
To: git; +Cc: Jeff Hostetler, Eric DeCosta
In-Reply-To: <20230918222908.GA2659096@coredump.intra.peff.net>
The start_bg_command() function takes a callback to tell when the
background-ed process is "ready". The callback receives the
child_process struct as well as an extra void pointer. But curiously,
neither of the two users of this interface look at either parameter!
This makes some sense. The only non-test user of the API is fsmonitor,
which uses fsmonitor_ipc__get_state() to connect to a single global
fsmonitor daemon (i.e., the one we just started!).
So we could just drop these parameters entirely. But it seems like a
pretty reasonable interface for the "wait" callback to have access to
the details of the spawned process, and to have room for passing extra
data through a void pointer. So let's leave these in place but mark the
unused ones so that -Wunused-parameter does not complain.
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/fsmonitor--daemon.c | 3 ++-
t/helper/test-simple-ipc.c | 3 ++-
2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/builtin/fsmonitor--daemon.c b/builtin/fsmonitor--daemon.c
index ce511c3ed6..5d01db5c02 100644
--- a/builtin/fsmonitor--daemon.c
+++ b/builtin/fsmonitor--daemon.c
@@ -1443,7 +1443,8 @@ static int try_to_run_foreground_daemon(int detach_console MAYBE_UNUSED)
static start_bg_wait_cb bg_wait_cb;
-static int bg_wait_cb(const struct child_process *cp, void *cb_data)
+static int bg_wait_cb(const struct child_process *cp UNUSED,
+ void *cb_data UNUSED)
{
enum ipc_active_state s = fsmonitor_ipc__get_state();
diff --git a/t/helper/test-simple-ipc.c b/t/helper/test-simple-ipc.c
index 3d1436da59..941ae7e3bc 100644
--- a/t/helper/test-simple-ipc.c
+++ b/t/helper/test-simple-ipc.c
@@ -278,7 +278,8 @@ static int daemon__run_server(void)
static start_bg_wait_cb bg_wait_cb;
-static int bg_wait_cb(const struct child_process *cp, void *cb_data)
+static int bg_wait_cb(const struct child_process *cp UNUSED,
+ void *cb_data UNUSED)
{
int s = ipc_get_active_state(cl_args.path);
--
2.42.0.671.g43fbf3903a
^ permalink raw reply related
* [PATCH 7/8] fsmonitor: mark unused hashmap callback parameters
From: Jeff King @ 2023-09-18 22:33 UTC (permalink / raw)
To: git; +Cc: Jeff Hostetler, Eric DeCosta
In-Reply-To: <20230918222908.GA2659096@coredump.intra.peff.net>
Like many hashmap comparison functions, our cookies_cmp() does not look
at its extra void data parameter. This should have been annotated in
02c3c59e62 (hashmap: mark unused callback parameters, 2022-08-19), but
this new case was added around the same time (plus fsmonitor is not
built at all on Linux, so it is easy to miss there).
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/fsmonitor--daemon.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/builtin/fsmonitor--daemon.c b/builtin/fsmonitor--daemon.c
index 7c4130c981..ce511c3ed6 100644
--- a/builtin/fsmonitor--daemon.c
+++ b/builtin/fsmonitor--daemon.c
@@ -129,8 +129,9 @@ struct fsmonitor_cookie_item {
enum fsmonitor_cookie_item_result result;
};
-static int cookies_cmp(const void *data, const struct hashmap_entry *he1,
- const struct hashmap_entry *he2, const void *keydata)
+static int cookies_cmp(const void *data UNUSED,
+ const struct hashmap_entry *he1,
+ const struct hashmap_entry *he2, const void *keydata)
{
const struct fsmonitor_cookie_item *a =
container_of(he1, const struct fsmonitor_cookie_item, entry);
--
2.42.0.671.g43fbf3903a
^ permalink raw reply related
* [PATCH 6/8] fsmonitor/darwin: mark unused parameters in system callback
From: Jeff King @ 2023-09-18 22:32 UTC (permalink / raw)
To: git; +Cc: Jeff Hostetler, Eric DeCosta
In-Reply-To: <20230918222908.GA2659096@coredump.intra.peff.net>
We pass fsevent_callback() to the system FSEventStreamCreate() function
as a callback. So we must match the expected function signature, even
though we don't care about all of the parameters. Mark the unused ones
to satisfy -Wunused-parameter.
Signed-off-by: Jeff King <peff@peff.net>
---
compat/fsmonitor/fsm-listen-darwin.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/compat/fsmonitor/fsm-listen-darwin.c b/compat/fsmonitor/fsm-listen-darwin.c
index 36c7e13281..11b56d3ef1 100644
--- a/compat/fsmonitor/fsm-listen-darwin.c
+++ b/compat/fsmonitor/fsm-listen-darwin.c
@@ -191,12 +191,12 @@ static void my_add_path(struct fsmonitor_batch *batch, const char *path)
}
-static void fsevent_callback(ConstFSEventStreamRef streamRef,
+static void fsevent_callback(ConstFSEventStreamRef streamRef UNUSED,
void *ctx,
size_t num_of_events,
void *event_paths,
const FSEventStreamEventFlags event_flags[],
- const FSEventStreamEventId event_ids[])
+ const FSEventStreamEventId event_ids[] UNUSED)
{
struct fsmonitor_daemon_state *state = ctx;
struct fsm_listen_data *data = state->listen_data;
--
2.42.0.671.g43fbf3903a
^ permalink raw reply related
* [PATCH 5/8] fsmonitor: mark unused parameters in stub functions
From: Jeff King @ 2023-09-18 22:32 UTC (permalink / raw)
To: git; +Cc: Jeff Hostetler, Eric DeCosta
In-Reply-To: <20230918222908.GA2659096@coredump.intra.peff.net>
The fsmonitor code has some platform-specific functions for which one or
more platforms implement noop or stub functions. We can't get rid of
these functions nor change their interface, since the point is to match
their equivalents in other platforms. But let's annotate their
parameters to quiet the compiler's -Wunused-parameter warning.
Signed-off-by: Jeff King <peff@peff.net>
---
compat/fsmonitor/fsm-health-darwin.c | 8 ++++----
compat/fsmonitor/fsm-path-utils-win32.c | 7 ++++---
fsmonitor-ipc.c | 10 +++++-----
3 files changed, 13 insertions(+), 12 deletions(-)
diff --git a/compat/fsmonitor/fsm-health-darwin.c b/compat/fsmonitor/fsm-health-darwin.c
index 5b1709d63f..c2afcbe6c8 100644
--- a/compat/fsmonitor/fsm-health-darwin.c
+++ b/compat/fsmonitor/fsm-health-darwin.c
@@ -4,21 +4,21 @@
#include "fsm-health.h"
#include "fsmonitor--daemon.h"
-int fsm_health__ctor(struct fsmonitor_daemon_state *state)
+int fsm_health__ctor(struct fsmonitor_daemon_state *state UNUSED)
{
return 0;
}
-void fsm_health__dtor(struct fsmonitor_daemon_state *state)
+void fsm_health__dtor(struct fsmonitor_daemon_state *state UNUSED)
{
return;
}
-void fsm_health__loop(struct fsmonitor_daemon_state *state)
+void fsm_health__loop(struct fsmonitor_daemon_state *state UNUSED)
{
return;
}
-void fsm_health__stop_async(struct fsmonitor_daemon_state *state)
+void fsm_health__stop_async(struct fsmonitor_daemon_state *state UNUSED)
{
}
diff --git a/compat/fsmonitor/fsm-path-utils-win32.c b/compat/fsmonitor/fsm-path-utils-win32.c
index c8a3e9dcdb..f4f9cc1f33 100644
--- a/compat/fsmonitor/fsm-path-utils-win32.c
+++ b/compat/fsmonitor/fsm-path-utils-win32.c
@@ -132,16 +132,17 @@ int fsmonitor__is_fs_remote(const char *path)
/*
* No-op for now.
*/
-int fsmonitor__get_alias(const char *path, struct alias_info *info)
+int fsmonitor__get_alias(const char *path UNUSED,
+ struct alias_info *info UNUSED)
{
return 0;
}
/*
* No-op for now.
*/
-char *fsmonitor__resolve_alias(const char *path,
- const struct alias_info *info)
+char *fsmonitor__resolve_alias(const char *path UNUSED,
+ const struct alias_info *info UNUSED)
{
return NULL;
}
diff --git a/fsmonitor-ipc.c b/fsmonitor-ipc.c
index 88575aa54c..153918cf76 100644
--- a/fsmonitor-ipc.c
+++ b/fsmonitor-ipc.c
@@ -20,7 +20,7 @@ int fsmonitor_ipc__is_supported(void)
return 0;
}
-const char *fsmonitor_ipc__get_path(struct repository *r)
+const char *fsmonitor_ipc__get_path(struct repository *r UNUSED)
{
return NULL;
}
@@ -30,14 +30,14 @@ enum ipc_active_state fsmonitor_ipc__get_state(void)
return IPC_STATE__OTHER_ERROR;
}
-int fsmonitor_ipc__send_query(const char *since_token,
- struct strbuf *answer)
+int fsmonitor_ipc__send_query(const char *since_token UNUSED,
+ struct strbuf *answer UNUSED)
{
return -1;
}
-int fsmonitor_ipc__send_command(const char *command,
- struct strbuf *answer)
+int fsmonitor_ipc__send_command(const char *command UNUSED,
+ struct strbuf *answer UNUSED)
{
return -1;
}
--
2.42.0.671.g43fbf3903a
^ permalink raw reply related
* [PATCH 4/8] fsmonitor/win32: mark unused parameter in fsm_os__incompatible()
From: Jeff King @ 2023-09-18 22:32 UTC (permalink / raw)
To: git; +Cc: Jeff Hostetler, Eric DeCosta
In-Reply-To: <20230918222908.GA2659096@coredump.intra.peff.net>
We never look at the "ipc" argument we receive. It was added in
8f44976882 (fsmonitor: avoid socket location check if using hook,
2022-10-04) to support the darwin fsmonitor code. The win32 code has to
match the same interface, but we should use an annotation to silence
-Wunused-parameter.
Signed-off-by: Jeff King <peff@peff.net>
---
compat/fsmonitor/fsm-settings-win32.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/compat/fsmonitor/fsm-settings-win32.c b/compat/fsmonitor/fsm-settings-win32.c
index b6f6744494..0f2aa321f6 100644
--- a/compat/fsmonitor/fsm-settings-win32.c
+++ b/compat/fsmonitor/fsm-settings-win32.c
@@ -25,7 +25,7 @@ static enum fsmonitor_reason check_vfs4git(struct repository *r)
return FSMONITOR_REASON_OK;
}
-enum fsmonitor_reason fsm_os__incompatible(struct repository *r, int ipc)
+enum fsmonitor_reason fsm_os__incompatible(struct repository *r, int ipc UNUSED)
{
enum fsmonitor_reason reason;
--
2.42.0.671.g43fbf3903a
^ permalink raw reply related
* [PATCH 3/8] fsmonitor: mark some maybe-unused parameters
From: Jeff King @ 2023-09-18 22:31 UTC (permalink / raw)
To: git; +Cc: Jeff Hostetler, Eric DeCosta
In-Reply-To: <20230918222908.GA2659096@coredump.intra.peff.net>
There's a bit of conditionally-compiled code in fsmonitor, so some
function parameters may be unused depending on the build options:
- in fsmonitor--daemon.c's try_to_run_foreground_daemon(), we take a
detach_console argument, but it's only used on Windows. This seems
intentional (and not mistakenly missing other platforms) based on
the discussion in c284e27ba7 (fsmonitor--daemon: implement 'start'
command, 2022-03-25), which introduced it.
- in fsmonitor-setting.c's check_for_incompatible(), we pass the "ipc"
flag down to the system-specific fsm_os__incompatible() helper. But
we can only do so if our platform has such a helper.
In both cases we can mark the argument as MAYBE_UNUSED. That annotates
it enough to suppress the compiler's -Wunused-parameter warning, but
without making it impossible to use the variable, as a regular UNUSED
annotation would.
Signed-off-by: Jeff King <peff@peff.net>
---
For a similar case in 2c3c3d88fc (imap-send: mark unused parameters with
NO_OPENSSL, 2023-08-29), I used the old:
(void)some_parameter_that_might_not_be_used;
trick. But I realized while writing this one that MAYBE_UNUSED fits the
bill a little more nicely, and I don't see any reason we would have
portability problems with it.
builtin/fsmonitor--daemon.c | 2 +-
fsmonitor-settings.c | 3 ++-
2 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/builtin/fsmonitor--daemon.c b/builtin/fsmonitor--daemon.c
index 7e99c4d61b..7c4130c981 100644
--- a/builtin/fsmonitor--daemon.c
+++ b/builtin/fsmonitor--daemon.c
@@ -1412,7 +1412,7 @@ static int fsmonitor_run_daemon(void)
return err;
}
-static int try_to_run_foreground_daemon(int detach_console)
+static int try_to_run_foreground_daemon(int detach_console MAYBE_UNUSED)
{
/*
* Technically, we don't need to probe for an existing daemon
diff --git a/fsmonitor-settings.c b/fsmonitor-settings.c
index b62acf44ae..a6a9e6bc19 100644
--- a/fsmonitor-settings.c
+++ b/fsmonitor-settings.c
@@ -62,7 +62,8 @@ static enum fsmonitor_reason check_remote(struct repository *r)
}
#endif
-static enum fsmonitor_reason check_for_incompatible(struct repository *r, int ipc)
+static enum fsmonitor_reason check_for_incompatible(struct repository *r,
+ int ipc MAYBE_UNUSED)
{
if (!r->worktree) {
/*
--
2.42.0.671.g43fbf3903a
^ 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