Re: [PATCH] refs: implement reference transaction hooks

From: Patrick Steinhardt <ps@pks.im>
Date: Jun 3, 2020, 11:26 UTC
Message-ID: <20200603112604.GA25644@tanuki.pks.im>
In-Reply-To: <xmqq4krttl4k.fsf@gitster.c.googlers.com>

On Tue, Jun 02, 2020 at 10:47:55AM -0700, Junio C Hamano wrote:

Show 10 quoted lines

> Patrick Steinhardt <ps@pks.im> writes:
> 
> > The above scenario is the motivation for a set of three new hooks that
> > reach directly into Git's reference transaction. Each of the following
> > new hooks (currently) doesn't accept any parameters and receives the set
> > of queued reference updates via stdin:
> 
> Do we have something (e.g. performance measurement) to convince
> ourselves that this won't incur unacceptable levels of overhead in
> null cases where there is no hook installed in the repository?

Not yet, but I'll try to come up with a benchmark in the next iteration. I guess the best way to test is to directly exercise git-update-refs, as it's nearly a direct wrapper around reference transactions.

Show 33 quoted lines

> > +	proc.in = -1;
> > +	proc.stdout_to_stderr = 1;
> > +	proc.trace2_hook_name = hook_name;
> > +
> > +	code = start_command(&proc);
> > +	if (code)
> > +		return code;
> > +
> > +	sigchain_push(SIGPIPE, SIG_IGN);
> > +
> > +	for (i = 0; i < transaction->nr; i++) {
> > +		struct ref_update *update = transaction->updates[i];
> > +
> > +		strbuf_reset(&buf);
> > +		strbuf_addf(&buf, "%s %s %s\n",
> > +			    oid_to_hex(&update->old_oid),
> > +			    oid_to_hex(&update->new_oid),
> > +			    update->refname);
> > +
> > +		if (write_in_full(proc.in, buf.buf, buf.len) < 0)
> > +			break;
> 
> We leave the loop early when we detect a write failure here...
> 
> > +	}
> > +
> > +	close(proc.in);
> > +	sigchain_pop(SIGPIPE);
> > +
> > +	strbuf_release(&buf);
> > +	return finish_command(&proc);
> 
> ... but the caller does not get notified.  Intended?

This is semi-intended. In case the hook doesn't fully consume stdin and exits early, writing to its stdin would fail as we ignore SIGPIPE. We don't want to force the hook to care about consuming all of stdin, though.

We could improve error handling here by ignoring EPIPE, but making every other write error fatal. If there's any other abnormal error condition then we certainly don't want the hook to act on incomplete data and pretend everything's fine.

Show 22 quoted lines

> > +}
> > +
> >  int ref_transaction_prepare(struct ref_transaction *transaction,
> >  			    struct strbuf *err)
> >  {
> >  	struct ref_store *refs = transaction->ref_store;
> > +	int ret;
> >  
> >  	switch (transaction->state) {
> >  	case REF_TRANSACTION_OPEN:
> > @@ -2012,7 +2060,17 @@ int ref_transaction_prepare(struct ref_transaction *transaction,
> >  		return -1;
> >  	}
> >  
> > -	return refs->be->transaction_prepare(refs, transaction, err);
> > +	ret = refs->be->transaction_prepare(refs, transaction, err);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = run_transaction_hook(transaction, "ref-transaction-prepared");
> 
> This caller does care about it, no?

This caller does as it may abort the transaction, but...

Show 17 quoted lines

> > +	if (ret) {
> > +		ref_transaction_abort(transaction, err);
> > +		die(_("ref updates aborted by hook"));
> > +	}
> > +
> > +	return 0;
> >  }
> >  
> >  int ref_transaction_abort(struct ref_transaction *transaction,
> > @@ -2036,6 +2094,8 @@ int ref_transaction_abort(struct ref_transaction *transaction,
> >  		break;
> >  	}
> >  
> > +	run_transaction_hook(transaction, "ref-transaction-aborted");
> 
> And I presume that the callers of "ref_xn_abort()" would, too, but
> the value returned here does not get folded into 'ret'.

... this one doesn't. The thing is that the reference transaction hook for the "aborted" case can't do anything about an aborted transaction after the fact anyway. That's why I chose to ignore errors here, same for the "committed" case.

Patrick