| Message ID | d255c7a5f95635c2e7ae36b9689c3efd07b4df5d.1604501894.git.ps@pks.im (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | refs: Always pass old object name to reftx hook | expand |
On Wed, Nov 04, 2020 at 03:58:40PM +0100, Patrick Steinhardt wrote: > Inputs of the reference-transaction hook currently depends on the > command which is being run. For example if the command `git update-ref > $REF $A $B` is executed, it will receive "$B $A $REF" as input, but if > the command `git update-ref $REF $A` is executed without providing the > old value, then it will receive "0*40 $A $REF" as input. This is due to > the fact that we directly write queued transaction updates into the > hook's standard input, which will not contain the old object value in > case it wasn't provided. > > While this behaviour reflects what is happening as part of the > repository, it doesn't feel like it is useful. The main intent of the > reference-transaction hook is to be able to completely audit all > reference updates, no matter where they come from. As such, it makes a > lot more sense to always provide actual values instead of what the user > wanted. Furthermore, it's impossible for the hook to distinguish whether > this is intended to be a branch creation or a branch update without > doing additional digging with the current format. > > Fix the issue by storing the old object value into the queued > transaction update operation if it wasn't provided by the caller. > > Signed-off-by: Patrick Steinhardt <ps@pks.im> Quick ping on this patch. Is there any interest or shall I just drop it? Patrick > Documentation/githooks.txt | 6 ++++++ > refs/files-backend.c | 8 ++++++++ > refs/packed-backend.c | 2 ++ > t/t1416-ref-transaction-hooks.sh | 12 ++++++------ > 4 files changed, 22 insertions(+), 6 deletions(-) > > diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt > index 4e097dc4e9..8f3540e2f6 100644 > --- a/Documentation/githooks.txt > +++ b/Documentation/githooks.txt > @@ -492,6 +492,12 @@ receives on standard input a line of the format: > > <old-value> SP <new-value> SP <ref-name> LF > > +where `<old-value>` is the old object name stored in the ref, > +`<new-value>` is the new object name to be stored in the ref and > +`<ref-name>` is the full name of the ref. > +When creating a new ref, `<old-value>` is 40 `0`. > +When deleting an old ref, `<new-value>` is 40 `0`. > + > The exit status of the hook is ignored for any state except for the > "prepared" state. In the "prepared" state, a non-zero exit status will > cause the transaction to be aborted. The hook will not be called with > diff --git a/refs/files-backend.c b/refs/files-backend.c > index 04e85e7002..5b10d3822b 100644 > --- a/refs/files-backend.c > +++ b/refs/files-backend.c > @@ -2452,6 +2452,9 @@ static int lock_ref_for_update(struct files_ref_store *refs, > ret = TRANSACTION_GENERIC_ERROR; > goto out; > } > + > + if (!(update->flags & REF_HAVE_OLD)) > + oidcpy(&update->old_oid, &lock->old_oid); > } else { > /* > * Create a new update for the reference this > @@ -2474,6 +2477,9 @@ static int lock_ref_for_update(struct files_ref_store *refs, > goto out; > } > > + if (!(update->flags & REF_HAVE_OLD)) > + oidcpy(&update->old_oid, &lock->old_oid); > + > /* > * If this update is happening indirectly because of a > * symref update, record the old OID in the parent > @@ -2484,6 +2490,8 @@ static int lock_ref_for_update(struct files_ref_store *refs, > parent_update = parent_update->parent_update) { > struct ref_lock *parent_lock = parent_update->backend_data; > oidcpy(&parent_lock->old_oid, &lock->old_oid); > + if (!(parent_update->flags & REF_HAVE_OLD)) > + oidcpy(&parent_update->old_oid, &lock->old_oid); > } > } > > diff --git a/refs/packed-backend.c b/refs/packed-backend.c > index b912f2505f..08f0feee3d 100644 > --- a/refs/packed-backend.c > +++ b/refs/packed-backend.c > @@ -1178,6 +1178,8 @@ static int write_with_updates(struct packed_ref_store *refs, > oid_to_hex(&update->old_oid)); > goto error; > } > + } else { > + oidcpy(&update->old_oid, iter->oid); > } > > /* Now figure out what to use for the new value: */ > diff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh > index f6e741c6c0..111533682a 100755 > --- a/t/t1416-ref-transaction-hooks.sh > +++ b/t/t1416-ref-transaction-hooks.sh > @@ -52,12 +52,12 @@ test_expect_success 'hook gets all queued updates in prepared state' ' > fi > EOF > cat >expect <<-EOF && > - $ZERO_OID $POST_OID HEAD > - $ZERO_OID $POST_OID refs/heads/master > + $PRE_OID $POST_OID HEAD > + $PRE_OID $POST_OID refs/heads/master > EOF > git update-ref HEAD POST <<-EOF && > - update HEAD $ZERO_OID $POST_OID > - update refs/heads/master $ZERO_OID $POST_OID > + update HEAD $PRE_OID $POST_OID > + update refs/heads/master $PRE_OID $POST_OID > EOF > test_cmp expect actual > ' > @@ -75,8 +75,8 @@ test_expect_success 'hook gets all queued updates in committed state' ' > fi > EOF > cat >expect <<-EOF && > - $ZERO_OID $POST_OID HEAD > - $ZERO_OID $POST_OID refs/heads/master > + $PRE_OID $POST_OID HEAD > + $PRE_OID $POST_OID refs/heads/master > EOF > git update-ref HEAD POST && > test_cmp expect actual > -- > 2.29.2 >
On Fri, Dec 04, 2020 at 09:37:21AM +0100, Patrick Steinhardt wrote:
> Quick ping on this patch. Is there any interest or shall I just drop it?
Apologies for completely dropping this from inbox. I am interested in
it, and the patch looks good to me. I suspect the reason that this never
got queued was that nobody reviewed it.
Would you consider resubmitting this patch if you are still interested
in pushing it forwards?
Thanks,
Taylor
On Tue, Jan 12, 2021 at 04:07:22PM -0500, Taylor Blau wrote: > On Fri, Dec 04, 2020 at 09:37:21AM +0100, Patrick Steinhardt wrote: > > Quick ping on this patch. Is there any interest or shall I just drop it? > > Apologies for completely dropping this from inbox. I am interested in > it, and the patch looks good to me. I suspect the reason that this never > got queued was that nobody reviewed it. No worries. > Would you consider resubmitting this patch if you are still interested > in pushing it forwards? I can, but does it help to resubmit the same patch? Your response bumped the thread up to the top anyway. Patrick
On Wed, Jan 13, 2021 at 12:22:14PM +0100, Patrick Steinhardt wrote: > > Would you consider resubmitting this patch if you are still interested > > in pushing it forwards? > > I can, but does it help to resubmit the same patch? Your response bumped > the thread up to the top anyway. Let's see if Junio picks it up in the next cycle. If he doesn't, it may help to re-submit the thread. > Patrick Thanks, Taylor
Patrick Steinhardt <ps@pks.im> writes: > On Tue, Jan 12, 2021 at 04:07:22PM -0500, Taylor Blau wrote: >> On Fri, Dec 04, 2020 at 09:37:21AM +0100, Patrick Steinhardt wrote: >> > Quick ping on this patch. Is there any interest or shall I just drop it? >> >> Apologies for completely dropping this from inbox. I am interested in >> it, and the patch looks good to me. I suspect the reason that this never >> got queued was that nobody reviewed it. > > No worries. > >> Would you consider resubmitting this patch if you are still interested >> in pushing it forwards? > > I can, but does it help to resubmit the same patch? Your response bumped > the thread up to the top anyway. Bumping without resending would often not help people to see the patch at all. For example, already-read-and-old messages are not even shown to me unless I ask my newsreader "I told you to show only the latest 200 messages, and I see this recent 'bump' message, but it responds to a way old message so you need to show me the latest 3000 messages to cover that era in order to see the patch message(s) it bumps." Thanks.
On Wed, Jan 13, 2021 at 12:11:28PM -0800, Junio C Hamano wrote: > Patrick Steinhardt <ps@pks.im> writes: > > > On Tue, Jan 12, 2021 at 04:07:22PM -0500, Taylor Blau wrote: > >> On Fri, Dec 04, 2020 at 09:37:21AM +0100, Patrick Steinhardt wrote: > >> > Quick ping on this patch. Is there any interest or shall I just drop it? > >> > >> Apologies for completely dropping this from inbox. I am interested in > >> it, and the patch looks good to me. I suspect the reason that this never > >> got queued was that nobody reviewed it. > > > > No worries. > > > >> Would you consider resubmitting this patch if you are still interested > >> in pushing it forwards? > > > > I can, but does it help to resubmit the same patch? Your response bumped > > the thread up to the top anyway. > > Bumping without resending would often not help people to see the > patch at all. > > For example, already-read-and-old messages are not even shown to me > unless I ask my newsreader "I told you to show only the latest 200 > messages, and I see this recent 'bump' message, but it responds to a > way old message so you need to show me the latest 3000 messages to > cover that era in order to see the patch message(s) it bumps." Fair point, I'll resend the patch. Thanks! Patrick
diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt index 4e097dc4e9..8f3540e2f6 100644 --- a/Documentation/githooks.txt +++ b/Documentation/githooks.txt @@ -492,6 +492,12 @@ receives on standard input a line of the format: <old-value> SP <new-value> SP <ref-name> LF +where `<old-value>` is the old object name stored in the ref, +`<new-value>` is the new object name to be stored in the ref and +`<ref-name>` is the full name of the ref. +When creating a new ref, `<old-value>` is 40 `0`. +When deleting an old ref, `<new-value>` is 40 `0`. + The exit status of the hook is ignored for any state except for the "prepared" state. In the "prepared" state, a non-zero exit status will cause the transaction to be aborted. The hook will not be called with diff --git a/refs/files-backend.c b/refs/files-backend.c index 04e85e7002..5b10d3822b 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -2452,6 +2452,9 @@ static int lock_ref_for_update(struct files_ref_store *refs, ret = TRANSACTION_GENERIC_ERROR; goto out; } + + if (!(update->flags & REF_HAVE_OLD)) + oidcpy(&update->old_oid, &lock->old_oid); } else { /* * Create a new update for the reference this @@ -2474,6 +2477,9 @@ static int lock_ref_for_update(struct files_ref_store *refs, goto out; } + if (!(update->flags & REF_HAVE_OLD)) + oidcpy(&update->old_oid, &lock->old_oid); + /* * If this update is happening indirectly because of a * symref update, record the old OID in the parent @@ -2484,6 +2490,8 @@ static int lock_ref_for_update(struct files_ref_store *refs, parent_update = parent_update->parent_update) { struct ref_lock *parent_lock = parent_update->backend_data; oidcpy(&parent_lock->old_oid, &lock->old_oid); + if (!(parent_update->flags & REF_HAVE_OLD)) + oidcpy(&parent_update->old_oid, &lock->old_oid); } } diff --git a/refs/packed-backend.c b/refs/packed-backend.c index b912f2505f..08f0feee3d 100644 --- a/refs/packed-backend.c +++ b/refs/packed-backend.c @@ -1178,6 +1178,8 @@ static int write_with_updates(struct packed_ref_store *refs, oid_to_hex(&update->old_oid)); goto error; } + } else { + oidcpy(&update->old_oid, iter->oid); } /* Now figure out what to use for the new value: */ diff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh index f6e741c6c0..111533682a 100755 --- a/t/t1416-ref-transaction-hooks.sh +++ b/t/t1416-ref-transaction-hooks.sh @@ -52,12 +52,12 @@ test_expect_success 'hook gets all queued updates in prepared state' ' fi EOF cat >expect <<-EOF && - $ZERO_OID $POST_OID HEAD - $ZERO_OID $POST_OID refs/heads/master + $PRE_OID $POST_OID HEAD + $PRE_OID $POST_OID refs/heads/master EOF git update-ref HEAD POST <<-EOF && - update HEAD $ZERO_OID $POST_OID - update refs/heads/master $ZERO_OID $POST_OID + update HEAD $PRE_OID $POST_OID + update refs/heads/master $PRE_OID $POST_OID EOF test_cmp expect actual ' @@ -75,8 +75,8 @@ test_expect_success 'hook gets all queued updates in committed state' ' fi EOF cat >expect <<-EOF && - $ZERO_OID $POST_OID HEAD - $ZERO_OID $POST_OID refs/heads/master + $PRE_OID $POST_OID HEAD + $PRE_OID $POST_OID refs/heads/master EOF git update-ref HEAD POST && test_cmp expect actual
Inputs of the reference-transaction hook currently depends on the command which is being run. For example if the command `git update-ref $REF $A $B` is executed, it will receive "$B $A $REF" as input, but if the command `git update-ref $REF $A` is executed without providing the old value, then it will receive "0*40 $A $REF" as input. This is due to the fact that we directly write queued transaction updates into the hook's standard input, which will not contain the old object value in case it wasn't provided. While this behaviour reflects what is happening as part of the repository, it doesn't feel like it is useful. The main intent of the reference-transaction hook is to be able to completely audit all reference updates, no matter where they come from. As such, it makes a lot more sense to always provide actual values instead of what the user wanted. Furthermore, it's impossible for the hook to distinguish whether this is intended to be a branch creation or a branch update without doing additional digging with the current format. Fix the issue by storing the old object value into the queued transaction update operation if it wasn't provided by the caller. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- Documentation/githooks.txt | 6 ++++++ refs/files-backend.c | 8 ++++++++ refs/packed-backend.c | 2 ++ t/t1416-ref-transaction-hooks.sh | 12 ++++++------ 4 files changed, 22 insertions(+), 6 deletions(-)