| Message ID | 20240426152449.228860-2-knayak@gitlab.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | add symref-* commands to 'git-update-ref --stdin' | expand |
Karthik Nayak <karthik.188@gmail.com> writes: > From: Karthik Nayak <karthik.188@gmail.com> > Subject: Re: [PATCH v4 1/7] refs: accept symref values in `ref_transaction[_add]_update` > > The `ref_transaction[_add]_update` functions obtain ref information and > flags to create a `ref_update` and add it to the transaction at hand. Just a very minor irritation, but ref_transaction_add_update() is a function used internally in the ref subsystem and is exported only because its visibility needs to cross file boundaries between refs.c and refs/*backend.c files. It would be better to only mention ref_transaction_update() in the title, and talk about the need to make matching adjustment to ref_transaction_add_update(), which is an internal function, in the body of the log message. This is an unrelated #leftoverbits tangent, but while trying to find out the reason why "[_add]" in the title looked irritating to me, I noticed that builtin/show-ref.c includes <refs/refs-internal.h>. I do not know what it uses from the "internal" implementation detail, but the API may have to be cleaned up so that a random "client" caller do not have to do so. The patch itself looked good. Thanks.
On Fri, Apr 26, 2024 at 12:31:36PM -0700, Junio C Hamano wrote: > This is an unrelated #leftoverbits tangent, but while trying to find > out the reason why "[_add]" in the title looked irritating to me, I > noticed that builtin/show-ref.c includes <refs/refs-internal.h>. I > do not know what it uses from the "internal" implementation detail, > but the API may have to be cleaned up so that a random "client" > caller do not have to do so. There are two issues. One is the use of refs_read_raw_ref(), added by Patrick's 9080a7f178 (builtin/show-ref: add new mode to check for reference existence, 2023-10-31). And it argues there why the regular API is unsufficient (mostly because it does not protect errno). But the more interesting one is a call to refname_is_safe(), added recently by Phillip's 1dbe401563 (show-ref --verify: accept pseudorefs, 2024-02-07). Looking at that commit, the intent was to allow pseudo-refs by loosening the conditional that checked "HEAD" to allow "FOO_BAR" but not "foobar" outside of "refs/". We enforce the all-caps pseudoref syntax in is_refname_safe(). The proper API there is I think check_ref_format() with ALLOW_ONELEVEL. But you shouldn't need to do that, because the refs code should be checking the names itself (using check_ref_format() usually, but falling back to refname_is_safe() if the ALLOW_BAD_NAME flag is passed). And I think there _is_ a bug there. The idea of those two functions is that check_ref_format() would allow a subset of what refname_is_safe() does. We'd fall back to the latter when deleting, but not otherwise allow creation or updates. However, it looks like check_ref_format() doesn't enforce the pseudo-ref syntax. It will happily resolve this: git rev-parse HEAD >.git/foo git rev-parse foo and even update it: git update-ref foo HEAD though curiously we will refuse to delete it: $ git update-ref -d foo error: refusing to update ref with bad name 'foo' since that sets the ALLOW_BAD_NAME flag! IMHO these should _all_ be forbidden, because we only want to allow the more limited pseudoref names everywhere (and never mischievous ones like "config" or whatever). And once we are doing that, then show-ref has no need to check the format. It can just call read_ref() and it either gets an answer or doesn't. I don't know if that is a #leftoverbit though. It perhaps more complicated than that. -Peff
Junio C Hamano <gitster@pobox.com> writes: > Karthik Nayak <karthik.188@gmail.com> writes: > >> From: Karthik Nayak <karthik.188@gmail.com> >> Subject: Re: [PATCH v4 1/7] refs: accept symref values in `ref_transaction[_add]_update` >> >> The `ref_transaction[_add]_update` functions obtain ref information and >> flags to create a `ref_update` and add it to the transaction at hand. > > Just a very minor irritation, but ref_transaction_add_update() is a > function used internally in the ref subsystem and is exported only > because its visibility needs to cross file boundaries between refs.c > and refs/*backend.c files. Yes that is true. I'll amend this for the next version. Thanks
On Fri, Apr 26, 2024 at 05:15:29PM -0400, Jeff King wrote: > On Fri, Apr 26, 2024 at 12:31:36PM -0700, Junio C Hamano wrote: > > > This is an unrelated #leftoverbits tangent, but while trying to find > > out the reason why "[_add]" in the title looked irritating to me, I > > noticed that builtin/show-ref.c includes <refs/refs-internal.h>. I > > do not know what it uses from the "internal" implementation detail, > > but the API may have to be cleaned up so that a random "client" > > caller do not have to do so. > > There are two issues. One is the use of refs_read_raw_ref(), added by > Patrick's 9080a7f178 (builtin/show-ref: add new mode to check for > reference existence, 2023-10-31). And it argues there why the regular > API is unsufficient (mostly because it does not protect errno). > > But the more interesting one is a call to refname_is_safe(), added > recently by Phillip's 1dbe401563 (show-ref --verify: accept pseudorefs, > 2024-02-07). Looking at that commit, the intent was to allow pseudo-refs > by loosening the conditional that checked "HEAD" to allow "FOO_BAR" but > not "foobar" outside of "refs/". We enforce the all-caps pseudoref > syntax in is_refname_safe(). > > The proper API there is I think check_ref_format() with ALLOW_ONELEVEL. > But you shouldn't need to do that, because the refs code should be > checking the names itself (using check_ref_format() usually, but falling > back to refname_is_safe() if the ALLOW_BAD_NAME flag is passed). > > And I think there _is_ a bug there. The idea of those two functions is > that check_ref_format() would allow a subset of what refname_is_safe() > does. We'd fall back to the latter when deleting, but not otherwise > allow creation or updates. > > However, it looks like check_ref_format() doesn't enforce the pseudo-ref > syntax. It will happily resolve this: > > git rev-parse HEAD >.git/foo > git rev-parse foo > > and even update it: > > git update-ref foo HEAD > > though curiously we will refuse to delete it: > > $ git update-ref -d foo > error: refusing to update ref with bad name 'foo' > > since that sets the ALLOW_BAD_NAME flag! > > IMHO these should _all_ be forbidden, because we only want to allow the > more limited pseudoref names everywhere (and never mischievous ones like > "config" or whatever). And once we are doing that, then show-ref has no > need to check the format. It can just call read_ref() and it either gets > an answer or doesn't. > > I don't know if that is a #leftoverbit though. It perhaps more > complicated than that. Yeah, this is something that I've repeatedly stumbled over myself. If I remember correctly, the plan was to clean up and consolidate all these different functions we have for checking ref names such that they become easier to use and hopefully lead to more consistent behaviour. In any case, I very much agree that git-update-ref(1) should refuse to write refs with names that are known-bad. There should probably be an escape hatch though that at least allows you to _delete_ those, or otherwise there is no way to remove such a ref in the reftable repo. Well, except for meddling with the binary format, but I doubt that anybody would really want to do that. Patrick
On Mon, Apr 29, 2024 at 09:02:56AM +0200, Patrick Steinhardt wrote: > > IMHO these should _all_ be forbidden, because we only want to allow the > > more limited pseudoref names everywhere (and never mischievous ones like > > "config" or whatever). And once we are doing that, then show-ref has no > > need to check the format. It can just call read_ref() and it either gets > > an answer or doesn't. > > > > I don't know if that is a #leftoverbit though. It perhaps more > > complicated than that. > > Yeah, this is something that I've repeatedly stumbled over myself. If I > remember correctly, the plan was to clean up and consolidate all these > different functions we have for checking ref names such that they become > easier to use and hopefully lead to more consistent behaviour. I think the code changes here aren't too bad (modulo some head-scratching I had to do around per-worktree refs). I'll post a series in a moment (split off from here since we're far off topic from the original thread). > In any case, I very much agree that git-update-ref(1) should refuse to > write refs with names that are known-bad. There should probably be an > escape hatch though that at least allows you to _delete_ those, or > otherwise there is no way to remove such a ref in the reftable repo. > Well, except for meddling with the binary format, but I doubt that > anybody would really want to do that. Yeah, ironically deleting is the one thing you cannot do with them even right now. ;) That is because the supposedly-looser refname_is_safe() is actually tighter. It would be tough to loosen it safely, since you don't want to allow deleting arbitrary files in .git. Or worse, escaping .git via clever paths, symlinks, etc. So I think at most you'd want some kind of "trust me, for this command don't bother with ref format checks" flag. Or alternatively, refname_is_safe() should become a per-backend thing. And then reftables can say "everything is safe", because the ref names are never used as paths (I think, anyway; I didn't follow all of the discussion around per-worktree refs, pseudorefs, etc). -Peff
On 29/04/2024 08:55, Jeff King wrote: > On Mon, Apr 29, 2024 at 09:02:56AM +0200, Patrick Steinhardt wrote: > >> In any case, I very much agree that git-update-ref(1) should refuse to >> write refs with names that are known-bad. There should probably be an >> escape hatch though that at least allows you to _delete_ those, or >> otherwise there is no way to remove such a ref in the reftable repo. >> Well, except for meddling with the binary format, but I doubt that >> anybody would really want to do that. > > Yeah, ironically deleting is the one thing you cannot do with them even > right now. ;) That is because the supposedly-looser refname_is_safe() is > actually tighter. > > It would be tough to loosen it safely, since you don't want to allow > deleting arbitrary files in .git. Or worse, escaping .git via clever > paths, symlinks, etc. So I think at most you'd want some kind of "trust > me, for this command don't bother with ref format checks" flag. Currently "git update-ref config HEAD" fails because the config file does not look like a ref - could we do similar check when deleting references at least for the files backend for arguments outside refs/? Best Wishes Phillip
Hi Peff On 26/04/2024 22:15, Jeff King wrote: > On Fri, Apr 26, 2024 at 12:31:36PM -0700, Junio C Hamano wrote: > > But the more interesting one is a call to refname_is_safe(), added > recently by Phillip's 1dbe401563 (show-ref --verify: accept pseudorefs, > 2024-02-07). Looking at that commit, the intent was to allow pseudo-refs > by loosening the conditional that checked "HEAD" to allow "FOO_BAR" but > not "foobar" outside of "refs/". We enforce the all-caps pseudoref > syntax in is_refname_safe(). > > The proper API there is I think check_ref_format() with ALLOW_ONELEVEL. ALLOW_ONELEVEL just disables the check that the refname contains a '/' and I think it is aimed at checking branch and tag names without a refs/{heads,tags} prefix. If we want to move away from using refname_is_safe() perhaps we could add an ALLOW_PSEUDOREF flag that only allows the name to contain '[A-Z_]' if there is no '/'. Best Wishes Phillip
Hi Karthik On 26/04/2024 16:24, Karthik Nayak wrote: > From: Karthik Nayak <karthik.188@gmail.com> > > The `ref_transaction[_add]_update` functions obtain ref information and > flags to create a `ref_update` and add it to the transaction at hand. > > To extend symref support in transactions, we need to also accept the > old and new ref targets and process it. s/it/them/ > In this commit, let's add the This commit adds? > required parameters to the function and modify all call sites. > > The two parameters added are `new_target` and `old_target`. The > `new_target` is used to denote what the reference should point to when > the transaction is applied. > > The `old_target` denotes the value the reference must have before the > update. Some functions allow this parameter to be NULL, meaning that the > old value of the reference is not checked. > > The handling logic of these parameters will be added in consequent > commits as we add symref commands to the '--stdin' mode of > 'git-update-ref'. > > Signed-off-by: Karthik Nayak <karthik.188@gmail.com> Thanks for updating the documentation, I've left a couple of comments below > diff --git a/refs.h b/refs.h > index d278775e08..c792e13a64 100644 > --- a/refs.h > +++ b/refs.h > @@ -648,6 +648,15 @@ struct ref_transaction *ref_transaction_begin(struct strbuf *err); > * before the update. A copy of this value is made in the > * transaction. > * > + * new_target -- the target reference that the reference will be > + * update to point to. s/update/updated/ > This takes precedence over new_oid when set. I thought it was a bug to set both new_oid and new_target. > If the reference is a regular reference, it will be > + * converted to a symbolic reference. > + * > + * old_target -- the reference that the reference must be pointing to. > + * Will only be taken into account when the reference is a symbolic > + * reference. Does this last sentence mean it is not possible to assert that it is currently a symbolic reference? I thought the point of being able to specify the old value of a ref when updating was to ensure it hadn't changed since it was read. This contradicts the documentation in the next hunk and the description in the commit message. > * flags -- flags affecting the update, passed to > * update_ref_lock(). Possible flags: REF_NO_DEREF, > * REF_FORCE_CREATE_REFLOG. See those constants for more > @@ -713,7 +722,11 @@ struct ref_transaction *ref_transaction_begin(struct strbuf *err); > * beforehand. The old value is checked after the lock is taken to > * prevent races. If the old value doesn't agree with old_oid, the > * whole transaction fails. If old_oid is NULL, then the previous > - * value is not checked. > + * value is not checked. If `old_target` is not NULL, treat the reference > + * as a symbolic ref and validate that its target before the update is > + * `old_target`. If the `new_target` is not NULL, then the reference > + * will be updated to a symbolic ref which targets `new_target`. This looks good and describes the behavior I'd expected to see. Best Wishes Phillip
Phillip Wood <phillip.wood123@gmail.com> writes: > Hi Karthik > > On 26/04/2024 16:24, Karthik Nayak wrote: >> From: Karthik Nayak <karthik.188@gmail.com> >> >> The `ref_transaction[_add]_update` functions obtain ref information and >> flags to create a `ref_update` and add it to the transaction at hand. >> >> To extend symref support in transactions, we need to also accept the >> old and new ref targets and process it. > > s/it/them/ > >> In this commit, let's add the > > This commit adds? > Thanks, will add both the above. >> required parameters to the function and modify all call sites. >> >> The two parameters added are `new_target` and `old_target`. The >> `new_target` is used to denote what the reference should point to when >> the transaction is applied. >> >> The `old_target` denotes the value the reference must have before the >> update. Some functions allow this parameter to be NULL, meaning that the >> old value of the reference is not checked. >> >> The handling logic of these parameters will be added in consequent >> commits as we add symref commands to the '--stdin' mode of >> 'git-update-ref'. >> >> Signed-off-by: Karthik Nayak <karthik.188@gmail.com> > > Thanks for updating the documentation, I've left a couple of comments below > Thanks for the review. >> diff --git a/refs.h b/refs.h >> index d278775e08..c792e13a64 100644 >> --- a/refs.h >> +++ b/refs.h >> @@ -648,6 +648,15 @@ struct ref_transaction *ref_transaction_begin(struct strbuf *err); >> * before the update. A copy of this value is made in the >> * transaction. >> * >> + * new_target -- the target reference that the reference will be >> + * update to point to. > > s/update/updated/ > >> This takes precedence over new_oid when set. > > I thought it was a bug to set both new_oid and new_target. > Yup. fixed both of these. > >> If the reference is a regular reference, it will be >> + * converted to a symbolic reference. > > + * >> + * old_target -- the reference that the reference must be pointing to. >> + * Will only be taken into account when the reference is a symbolic >> + * reference. > > Does this last sentence mean it is not possible to assert that it is > currently a symbolic reference? I thought the point of being able to > specify the old value of a ref when updating was to ensure it hadn't > changed since it was read. This contradicts the documentation in the > next hunk and the description in the commit message. > I see how this is vague, I was trying to imply that the old_target is used to check a symref's old_value. But actually, if this is set and the ref is a regular ref, we do fail the check. So this is wrong. Let me just strip the last time.
phillip.wood123@gmail.com writes: > ALLOW_ONELEVEL just disables the check that the refname contains a '/' > and I think it is aimed at checking branch and tag names without a > refs/{heads,tags} prefix. If we want to move away from using > refname_is_safe() perhaps we could add an ALLOW_PSEUDOREF flag that > only allows the name to contain '[A-Z_]' if there is no '/'. Makes sense. I wonder if we eventually can get rid of ALLOW_ONELEVEL, though. If all callers that use ALLOW_ONELEVEL know under which prefix they plan to hang the refname they are checking (if the refname passed the check), we can force the check to be performed always on the full refname, and it will become easier to make the check more consistent---as the check will have full context information. For example, with ALLOW_ONELEVEL the check may say "HEAD" is OK, but if we get rid of ALLOW_ONELEVEL and force the callers to always test the full refname, we may say "refs/heads/HEAD" is not acceptable, neither is "refs/tags/HEAD", but "refs/remotes/origin/HEAD" is good.
On Mon, Apr 29, 2024 at 10:32:58AM +0100, phillip.wood123@gmail.com wrote: > On 26/04/2024 22:15, Jeff King wrote: > > On Fri, Apr 26, 2024 at 12:31:36PM -0700, Junio C Hamano wrote: > > > > But the more interesting one is a call to refname_is_safe(), added > > recently by Phillip's 1dbe401563 (show-ref --verify: accept pseudorefs, > > 2024-02-07). Looking at that commit, the intent was to allow pseudo-refs > > by loosening the conditional that checked "HEAD" to allow "FOO_BAR" but > > not "foobar" outside of "refs/". We enforce the all-caps pseudoref > > syntax in is_refname_safe(). > > > > The proper API there is I think check_ref_format() with ALLOW_ONELEVEL. > > ALLOW_ONELEVEL just disables the check that the refname contains a '/' and I > think it is aimed at checking branch and tag names without a > refs/{heads,tags} prefix. If we want to move away from using > refname_is_safe() perhaps we could add an ALLOW_PSEUDOREF flag that only > allows the name to contain '[A-Z_]' if there is no '/'. Right, I think that was the original reason for ALLOW_ONELEVEL. But then lots of callers ended up using it because it was the only way to allow "HEAD" or "FETCH_HEAD" to work alongside regular refs. See the series I posted elsewhere which adds a new flag to cover that case. -Peff
On Mon, Apr 29, 2024 at 09:18:47AM -0700, Junio C Hamano wrote: > phillip.wood123@gmail.com writes: > > > ALLOW_ONELEVEL just disables the check that the refname contains a '/' > > and I think it is aimed at checking branch and tag names without a > > refs/{heads,tags} prefix. If we want to move away from using > > refname_is_safe() perhaps we could add an ALLOW_PSEUDOREF flag that > > only allows the name to contain '[A-Z_]' if there is no '/'. > > Makes sense. > > I wonder if we eventually can get rid of ALLOW_ONELEVEL, though. If > all callers that use ALLOW_ONELEVEL know under which prefix they > plan to hang the refname they are checking (if the refname passed > the check), we can force the check to be performed always on the > full refname, and it will become easier to make the check more > consistent---as the check will have full context information. > > For example, with ALLOW_ONELEVEL the check may say "HEAD" is OK, but > if we get rid of ALLOW_ONELEVEL and force the callers to always test > the full refname, we may say "refs/heads/HEAD" is not acceptable, > neither is "refs/tags/HEAD", but "refs/remotes/origin/HEAD" is good. One case I ran into while working on my series is refspec parsing, where we feed the left and right halves to check_refname_format(), albeit with a special flag that allows the "*" wildcard character. And there we are OK with one-level names because they will be passed through the dwim_ref() lookup. I don't know if there are other spots that do something similar. Most of them would, I imagine, just take any input and leave it to the ref code to enforce syntax after the dwim-ref has happened. -Peff
diff --git a/branch.c b/branch.c index e4a738fc7b..48af4c3ceb 100644 --- a/branch.c +++ b/branch.c @@ -627,7 +627,7 @@ void create_branch(struct repository *r, if (!transaction || ref_transaction_update(transaction, ref.buf, &oid, forcing ? NULL : null_oid(), - 0, msg, &err) || + NULL, NULL, 0, msg, &err) || ref_transaction_commit(transaction, &err)) die("%s", err.buf); ref_transaction_free(transaction); diff --git a/builtin/fast-import.c b/builtin/fast-import.c index dc5a9d32dd..297dfb91a1 100644 --- a/builtin/fast-import.c +++ b/builtin/fast-import.c @@ -1634,7 +1634,7 @@ static int update_branch(struct branch *b) transaction = ref_transaction_begin(&err); if (!transaction || ref_transaction_update(transaction, b->name, &b->oid, &old_oid, - 0, msg, &err) || + NULL, NULL, 0, msg, &err) || ref_transaction_commit(transaction, &err)) { ref_transaction_free(transaction); error("%s", err.buf); @@ -1675,7 +1675,8 @@ static void dump_tags(void) strbuf_addf(&ref_name, "refs/tags/%s", t->name); if (ref_transaction_update(transaction, ref_name.buf, - &t->oid, NULL, 0, msg, &err)) { + &t->oid, NULL, NULL, NULL, + 0, msg, &err)) { failure |= error("%s", err.buf); goto cleanup; } diff --git a/builtin/fetch.c b/builtin/fetch.c index 5857d860db..66840b7c5b 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -668,7 +668,7 @@ static int s_update_ref(const char *action, ret = ref_transaction_update(transaction, ref->name, &ref->new_oid, check_old ? &ref->old_oid : NULL, - 0, msg, &err); + NULL, NULL, 0, msg, &err); if (ret) { ret = STORE_REF_ERROR_OTHER; goto out; diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c index e8d7df14b6..b150ef39a8 100644 --- a/builtin/receive-pack.c +++ b/builtin/receive-pack.c @@ -1595,6 +1595,7 @@ static const char *update(struct command *cmd, struct shallow_info *si) if (ref_transaction_update(transaction, namespaced_name, new_oid, old_oid, + NULL, NULL, 0, "push", &err)) { rp_error("%s", err.buf); diff --git a/builtin/replace.c b/builtin/replace.c index da59600ad2..7690687b0e 100644 --- a/builtin/replace.c +++ b/builtin/replace.c @@ -201,7 +201,7 @@ static int replace_object_oid(const char *object_ref, transaction = ref_transaction_begin(&err); if (!transaction || ref_transaction_update(transaction, ref.buf, repl, &prev, - 0, NULL, &err) || + NULL, NULL, 0, NULL, &err) || ref_transaction_commit(transaction, &err)) res = error("%s", err.buf); diff --git a/builtin/tag.c b/builtin/tag.c index 9a33cb50b4..40a65fdebc 100644 --- a/builtin/tag.c +++ b/builtin/tag.c @@ -660,6 +660,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix) transaction = ref_transaction_begin(&err); if (!transaction || ref_transaction_update(transaction, ref.buf, &object, &prev, + NULL, NULL, create_reflog ? REF_FORCE_CREATE_REFLOG : 0, reflog_msg.buf, &err) || ref_transaction_commit(transaction, &err)) { diff --git a/builtin/update-ref.c b/builtin/update-ref.c index e46afbc46d..21fdbf6ac8 100644 --- a/builtin/update-ref.c +++ b/builtin/update-ref.c @@ -204,6 +204,7 @@ static void parse_cmd_update(struct ref_transaction *transaction, if (ref_transaction_update(transaction, refname, &new_oid, have_old ? &old_oid : NULL, + NULL, NULL, update_flags | create_reflog_flag, msg, &err)) die("%s", err.buf); diff --git a/refs.c b/refs.c index 55d2e0b2cb..060a31616d 100644 --- a/refs.c +++ b/refs.c @@ -1228,6 +1228,7 @@ struct ref_update *ref_transaction_add_update( const char *refname, unsigned int flags, const struct object_id *new_oid, const struct object_id *old_oid, + const char *new_target, const char *old_target, const char *msg) { struct ref_update *update; @@ -1235,6 +1236,11 @@ struct ref_update *ref_transaction_add_update( if (transaction->state != REF_TRANSACTION_OPEN) BUG("update called for transaction that is not open"); + if (old_oid && !is_null_oid(old_oid) && old_target) + BUG("Only one of old_oid and old_target should be non NULL"); + if (new_oid && !is_null_oid(new_oid) && new_target) + BUG("Only one of new_oid and new_target should be non NULL"); + FLEX_ALLOC_STR(update, refname, refname); ALLOC_GROW(transaction->updates, transaction->nr + 1, transaction->alloc); transaction->updates[transaction->nr++] = update; @@ -1253,6 +1259,8 @@ int ref_transaction_update(struct ref_transaction *transaction, const char *refname, const struct object_id *new_oid, const struct object_id *old_oid, + const char *new_target, + const char *old_target, unsigned int flags, const char *msg, struct strbuf *err) { @@ -1280,7 +1288,8 @@ int ref_transaction_update(struct ref_transaction *transaction, flags |= (new_oid ? REF_HAVE_NEW : 0) | (old_oid ? REF_HAVE_OLD : 0); ref_transaction_add_update(transaction, refname, flags, - new_oid, old_oid, msg); + new_oid, old_oid, new_target, + old_target, msg); return 0; } @@ -1295,7 +1304,8 @@ int ref_transaction_create(struct ref_transaction *transaction, return 1; } return ref_transaction_update(transaction, refname, new_oid, - null_oid(), flags, msg, err); + null_oid(), NULL, NULL, flags, + msg, err); } int ref_transaction_delete(struct ref_transaction *transaction, @@ -1308,7 +1318,8 @@ int ref_transaction_delete(struct ref_transaction *transaction, BUG("delete called with old_oid set to zeros"); return ref_transaction_update(transaction, refname, null_oid(), old_oid, - flags, msg, err); + NULL, NULL, flags, + msg, err); } int ref_transaction_verify(struct ref_transaction *transaction, @@ -1321,6 +1332,7 @@ int ref_transaction_verify(struct ref_transaction *transaction, BUG("verify called with old_oid set to NULL"); return ref_transaction_update(transaction, refname, NULL, old_oid, + NULL, NULL, flags, NULL, err); } @@ -1335,8 +1347,8 @@ int refs_update_ref(struct ref_store *refs, const char *msg, t = ref_store_transaction_begin(refs, &err); if (!t || - ref_transaction_update(t, refname, new_oid, old_oid, flags, msg, - &err) || + ref_transaction_update(t, refname, new_oid, old_oid, NULL, NULL, + flags, msg, &err) || ref_transaction_commit(t, &err)) { ret = 1; ref_transaction_free(t); diff --git a/refs.h b/refs.h index d278775e08..c792e13a64 100644 --- a/refs.h +++ b/refs.h @@ -648,6 +648,15 @@ struct ref_transaction *ref_transaction_begin(struct strbuf *err); * before the update. A copy of this value is made in the * transaction. * + * new_target -- the target reference that the reference will be + * update to point to. This takes precedence over new_oid when + * set. If the reference is a regular reference, it will be + * converted to a symbolic reference. + * + * old_target -- the reference that the reference must be pointing to. + * Will only be taken into account when the reference is a symbolic + * reference. + * * flags -- flags affecting the update, passed to * update_ref_lock(). Possible flags: REF_NO_DEREF, * REF_FORCE_CREATE_REFLOG. See those constants for more @@ -713,7 +722,11 @@ struct ref_transaction *ref_transaction_begin(struct strbuf *err); * beforehand. The old value is checked after the lock is taken to * prevent races. If the old value doesn't agree with old_oid, the * whole transaction fails. If old_oid is NULL, then the previous - * value is not checked. + * value is not checked. If `old_target` is not NULL, treat the reference + * as a symbolic ref and validate that its target before the update is + * `old_target`. If the `new_target` is not NULL, then the reference + * will be updated to a symbolic ref which targets `new_target`. + * Together, these allow us to update between regular refs and symrefs. * * See the above comment "Reference transaction updates" for more * information. @@ -722,6 +735,8 @@ int ref_transaction_update(struct ref_transaction *transaction, const char *refname, const struct object_id *new_oid, const struct object_id *old_oid, + const char *new_target, + const char *old_target, unsigned int flags, const char *msg, struct strbuf *err); diff --git a/refs/files-backend.c b/refs/files-backend.c index a098d14ea0..e4d0aa3d41 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -1198,7 +1198,7 @@ static void prune_ref(struct files_ref_store *refs, struct ref_to_prune *r) ref_transaction_add_update( transaction, r->name, REF_NO_DEREF | REF_HAVE_NEW | REF_HAVE_OLD | REF_IS_PRUNING, - null_oid(), &r->oid, NULL); + null_oid(), &r->oid, NULL, NULL, NULL); if (ref_transaction_commit(transaction, &err)) goto cleanup; @@ -1292,7 +1292,7 @@ static int files_pack_refs(struct ref_store *ref_store, * packed-refs transaction: */ if (ref_transaction_update(transaction, iter->refname, - iter->oid, NULL, + iter->oid, NULL, NULL, NULL, REF_NO_DEREF, NULL, &err)) die("failure preparing to create packed reference %s: %s", iter->refname, err.buf); @@ -2309,7 +2309,7 @@ static int split_head_update(struct ref_update *update, transaction, "HEAD", update->flags | REF_LOG_ONLY | REF_NO_DEREF, &update->new_oid, &update->old_oid, - update->msg); + NULL, NULL, update->msg); /* * Add "HEAD". This insertion is O(N) in the transaction @@ -2372,7 +2372,7 @@ static int split_symref_update(struct ref_update *update, new_update = ref_transaction_add_update( transaction, referent, new_flags, &update->new_oid, &update->old_oid, - update->msg); + NULL, NULL, update->msg); new_update->parent_update = update; @@ -2763,7 +2763,7 @@ static int files_transaction_prepare(struct ref_store *ref_store, packed_transaction, update->refname, REF_HAVE_NEW | REF_NO_DEREF, &update->new_oid, NULL, - NULL); + NULL, NULL, NULL); } } @@ -3048,7 +3048,7 @@ static int files_initial_transaction_commit(struct ref_store *ref_store, ref_transaction_add_update(packed_transaction, update->refname, update->flags & ~REF_HAVE_OLD, &update->new_oid, &update->old_oid, - NULL); + NULL, NULL, NULL); } if (packed_refs_lock(refs->packed_ref_store, 0, err)) { diff --git a/refs/refs-internal.h b/refs/refs-internal.h index 56641aa57a..3040d4797c 100644 --- a/refs/refs-internal.h +++ b/refs/refs-internal.h @@ -124,6 +124,18 @@ struct ref_update { */ struct object_id old_oid; + /* + * If set, point the reference to this value. This can also be + * used to convert regular references to become symbolic refs. + */ + const char *new_target; + + /* + * If set and the reference is a symbolic ref, check that the + * reference previously pointed to this value. + */ + const char *old_target; + /* * One or more of REF_NO_DEREF, REF_FORCE_CREATE_REFLOG, * REF_HAVE_NEW, REF_HAVE_OLD, or backend-specific flags. @@ -173,6 +185,7 @@ struct ref_update *ref_transaction_add_update( const char *refname, unsigned int flags, const struct object_id *new_oid, const struct object_id *old_oid, + const char *new_target, const char *old_target, const char *msg); /* diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c index 1cda48c504..6104471199 100644 --- a/refs/reftable-backend.c +++ b/refs/reftable-backend.c @@ -829,7 +829,7 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store, new_update = ref_transaction_add_update( transaction, "HEAD", u->flags | REF_LOG_ONLY | REF_NO_DEREF, - &u->new_oid, &u->old_oid, u->msg); + &u->new_oid, &u->old_oid, NULL, NULL, u->msg); string_list_insert(&affected_refnames, new_update->refname); } @@ -908,7 +908,7 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store, */ new_update = ref_transaction_add_update( transaction, referent.buf, new_flags, - &u->new_oid, &u->old_oid, u->msg); + &u->new_oid, &u->old_oid, NULL, NULL, u->msg); new_update->parent_update = u; /* diff --git a/sequencer.c b/sequencer.c index 2c19846385..af1b25692b 100644 --- a/sequencer.c +++ b/sequencer.c @@ -616,7 +616,7 @@ static int fast_forward_to(struct repository *r, if (!transaction || ref_transaction_update(transaction, "HEAD", to, unborn && !is_rebase_i(opts) ? - null_oid() : from, + null_oid() : from, NULL, NULL, 0, sb.buf, &err) || ref_transaction_commit(transaction, &err)) { ref_transaction_free(transaction); @@ -1248,7 +1248,7 @@ int update_head_with_reflog(const struct commit *old_head, if (!transaction || ref_transaction_update(transaction, "HEAD", new_head, old_head ? &old_head->object.oid : null_oid(), - 0, sb.buf, err) || + NULL, NULL, 0, sb.buf, err) || ref_transaction_commit(transaction, err)) { ret = -1; } @@ -3764,8 +3764,9 @@ static int do_label(struct repository *r, const char *name, int len) } else if (repo_get_oid(r, "HEAD", &head_oid)) { error(_("could not read HEAD")); ret = -1; - } else if (ref_transaction_update(transaction, ref_name.buf, &head_oid, - NULL, 0, msg.buf, &err) < 0 || + } else if (ref_transaction_update(transaction, ref_name.buf, + &head_oid, NULL, NULL, NULL, + 0, msg.buf, &err) < 0 || ref_transaction_commit(transaction, &err)) { error("%s", err.buf); ret = -1; diff --git a/walker.c b/walker.c index c0fd632d92..1b3df43906 100644 --- a/walker.c +++ b/walker.c @@ -324,7 +324,7 @@ int walker_fetch(struct walker *walker, int targets, char **target, strbuf_reset(&refname); strbuf_addf(&refname, "refs/%s", write_ref[i]); if (ref_transaction_update(transaction, refname.buf, - oids + i, NULL, 0, + oids + i, NULL, NULL, NULL, 0, msg ? msg : "fetch (unknown)", &err)) { error("%s", err.buf);