Re: [PATCH v2 10/10] receive-pack: convert receive hooks to hook API

[Junio C Hamano <gitster@pobox.com>]

Adrian Ratiu <adrian.ratiu@collabora.com> writes:

> +static int feed_receive_hook(int hook_stdin_fd, struct receive_hook_feed_state *state, int lines_batch_size)

Overly long line and cannot read.  Can you stick to 80-column lines?

In any case, the reason I am responding to this message is not about
coding styles, but it seems to be the one whose leak is holding the
CI job from passing at the tip of 'seen'.

>  {
> +	struct command *cmd = state->cmd;
>  
> +	strbuf_reset(&state->buf);
>  
> +	/* batch lines to avoid going through run-command's ppoll for each line */
> +	for (int i = 0; i < lines_batch_size; i++) {
> +		while (cmd &&
> +		       state->skip_broken && (cmd->error_string || cmd->did_not_exist))
> +			cmd = cmd->next;
>  
> +		if (!cmd)
> +			break;  /* no more commands left */
>  
> +		if (!state->report)
> +			state->report = cmd->report;
>  
> +		if (state->report) {
> +			struct object_id *old_oid;
> +			struct object_id *new_oid;
> +			const char *ref_name;
>  
> +			old_oid = state->report->old_oid ? state->report->old_oid : &cmd->old_oid;
> +			new_oid = state->report->new_oid ? state->report->new_oid : &cmd->new_oid;
> +			ref_name = state->report->ref_name ? state->report->ref_name : cmd->ref_name;
>  
> +			strbuf_addf(&state->buf, "%s %s %s\n",
> +				    oid_to_hex(old_oid), oid_to_hex(new_oid),
> +				    ref_name);
>  
> +			state->report = state->report->next;
> +			if (!state->report)
> +				cmd = cmd->next;
> +		} else {
> +			strbuf_addf(&state->buf, "%s %s %s\n",
> +				    oid_to_hex(&cmd->old_oid), oid_to_hex(&cmd->new_oid),
> +				    cmd->ref_name);
> +			cmd = cmd->next;
> +		}
>  	}
>  
> +	state->cmd = cmd;
>  
> +	if (state->buf.len > 0) {
> +		int ret = write_in_full(hook_stdin_fd, state->buf.buf, state->buf.len);
> +		if (ret < 0) {
> +			if (errno == EPIPE)
> +				return 1; /* child closed pipe */
> +			return ret;
> +		}
> +	}
> +
> +	return state->cmd ? 0 : 1;  /* 0 = more to come, 1 = EOF */
>  }

> +static int feed_receive_hook_cb(int hook_stdin_fd, void *pp_cb, void *pp_task_cb UNUSED)
>  {
> -	struct receive_hook_feed_state *state = state_;
> -	struct command *cmd = state->cmd;
> +	struct hook_cb_data *hook_cb = pp_cb;
> +	struct receive_hook_feed_state *feed_state = hook_cb->options->feed_pipe_cb_data;
> ... 
> +	/* first-time setup */
> +	if (!hook_cb->options->feed_pipe_cb_data) {
> +		struct receive_hook_feed_context *ctx = hook_cb->options->feed_pipe_ctx;
> +		if (!ctx)
> +			BUG("run_hooks_opt.feed_pipe_ctx required for receive hook");
> +
> +		hook_cb->options->feed_pipe_cb_data = xmalloc(sizeof(struct receive_hook_feed_state));

The allocation done here seems to be causing one (smaller) leak.

  https://github.com/git/git/actions/runs/19381820975/job/55461902226#step:10:3994

> +		feed_state = hook_cb->options->feed_pipe_cb_data;
> +		strbuf_init(&feed_state->buf, 0);
> +		feed_state->cmd = ctx->cmd;
> +		feed_state->skip_broken = ctx->skip_broken;
> +		feed_state->report = NULL;
>  	}
> +
> +	/* batch 500 lines at once to avoid going through the run-command ppoll loop too often */
> +	if (feed_receive_hook(hook_stdin_fd, feed_state, 500) == 0)
> +		return 0; /* still have more data to feed */

It appears to me that the larger leak the leak checker finds

  https://github.com/git/git/actions/runs/19381820975/job/55461902226#step:10:4017

is in find_receive_hook() called from here, where state->buf has
accumulates a lot.

builtin/receive-pack.c:847 its stacktrace #5 talks about is probably
pointing at the struf_addf() call there.  The particular test is
about deliberately causing EPIPE by the making other side refuse to
read, so some error handling on this side is missing the necessary
deallocation, perhaps?

> +	strbuf_release(&feed_state->buf);
> +
> +	if (hook_cb->options->feed_pipe_cb_data)
> +		FREE_AND_NULL(hook_cb->options->feed_pipe_cb_data);
> +
> +	return 1; /* done feeding, run-command can close pipe */
> +}