Re: [PATCH] refs: implement reference transaction hooks

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 3738 bytes --]

On Tue, Jun 02, 2020 at 10:47:55AM -0700, Junio C Hamano wrote:
> 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.

> > +	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.

> > +}
> > +
> >  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...

> > +	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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]