| Message ID | 20240330224623.579457-8-knayak@gitlab.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | update-ref: add support for update-symref option | expand |
Karthik Nayak <karthik.188@gmail.com> writes: > diff --git a/Documentation/git-update-ref.txt b/Documentation/git-update-ref.txt > index 0561808cca..2ea8bc8167 100644 > --- a/Documentation/git-update-ref.txt > +++ b/Documentation/git-update-ref.txt > @@ -65,6 +65,7 @@ performs all modifications together. Specify commands of the form: In pre-context, there is this entry. update SP <ref> SP <newvalue> [SP <oldvalue>] LF Unrelated to this patch, we probably should say <new-value> and <old-value> to follow the (recent) documentation guideline to write multiple words concatenated with hyphen. If we are updating these newvalue and oldvalue anyway, we may want to update them to <new-oid> and <old-oid>, as the existing commands on refs are about object names, and what we are adding here are not. I would prefer to see such an update of existing documentation as a separate preliminary clean-up patch, so that we can cleanly add the "update-symref" that takes <new-symref-target> on top of a cleaned-up base. The <newvalue> ... > create SP <ref> SP <newvalue> LF > delete SP <ref> [SP <oldvalue>] LF > verify SP <ref> [SP <oldvalue>] LF > + update-symref SP <ref> SP <newvalue> LF ... we are giving to "update-symref" is not an object name, but a refname (i.e. the target of the symref at <ref> points at), so "newvalue" -> "new-ref" or something is needed. The semantics of this new command is not quite in line with the existing commands. Existing "update" and "delete" allow you to optionally specify <old-oid> to make sure you do not overwrite somebody else's update in the meantime. Your "update-symref" lacks such safety completely, which I doubt is a good idea in the context of adding it to an existing set that has such safety features. We should at least offer the same "optional" safety. That makes the syntax more like update-symref SP <ref> SP <new-ref> [SP <old-ref>] LF probably. Existing "update" command can be used to create (by having "zero" <old-oid>) and to delete (by having "zero" <new-oid>), but we still have "create" and "delete" separately. Given that we are teaching the command to also work with symrefs by adding command(s) for symrefs to an existing vocabulary, we should offer "create-symref" and "delete-symref" for consistency between refs and symrefs. It probably is a good idea to allow "update-symref" to also create and delete a symref in similar ways with the existing "update" allows creation and deletion of a ref. An "zero" <old-ref> may be an update that can happen only when the <ref> does not exist, i.e., creation, and an "zero" <new-ref> may be an update that makes <ref> disappear. "zero" value in the context of refs is a 0{40} object name (side note: we have an explicit mention of 40 in the doc, which needs to be updated eventually, probably outside this series). But in the new context of symrefs, a safe "zero" value needs to be picked carefully. An empty string may not work well syntactically especially in the `-z` format, because you cannot tell if update-symref NUL HEAD NUL refs/heads/main NUL NUL wants to say that <old-ref> is an empty string, or if it is missing. As a refname cannot have a path component that begins with a dot, a usable "zero" value for <new-ref> and <old-ref> may be a string like ".missing", ".detached", and perhaps ".detached-f78d7...f2d4". To summarize: update-symref SP HEAD SP refs/heads/main LF forcibly sets HEAD to point at refs/heads/main without regard to the current state of HEAD. update-symref SP HEAD SP .missing LF forcibly removes HEAD symref without regard to the current state of HEAD. update-symref SP HEAD SP refs/heads/main SP .missing LF creates HEAD symref to point at the 'main' branch but fails if HEAD already exists. update-symref SP HEAD SP refs/heads/main SP .detached LF update-symref SP HEAD SP refs/heads/main SP .detached-f78d7...f2d4 LF creates HEAD symref to point at the 'main' branch but fails if HEAD is not detached (or detached at the specified commit). update-symref SP HEAD SP refs/heads/main SP refs/heads/next LF creates HEAD symref to point at the 'main' branch but fails if HEAD is not pointing at the 'next' branch. Hmm?
On Sun, Mar 31, 2024 at 3:09 PM Junio C Hamano <gitster@pobox.com> wrote: > ... we are giving to "update-symref" is not an object name, but a > refname (i.e. the target of the symref at <ref> points at), so > "newvalue" -> "new-ref" or something is needed. I kept this line just to say, I agree on all of the above ... > Existing "update" command can be used to create (by having "zero" > <old-oid>) and to delete (by having "zero" <new-oid>), but we still > have "create" and "delete" separately. Given that we are teaching > the command to also work with symrefs by adding command(s) for > symrefs to an existing vocabulary, we should offer "create-symref" > and "delete-symref" for consistency between refs and symrefs. .. and on this, but: > It probably is a good idea to allow "update-symref" to also create > and delete a symref in similar ways with the existing "update" > allows creation and deletion of a ref. An "zero" <old-ref> may be > an update that can happen only when the <ref> does not exist, i.e., > creation, and an "zero" <new-ref> may be an update that makes <ref> > disappear. > > "zero" value in the context of refs is a 0{40} object name (side > note: we have an explicit mention of 40 in the doc, which needs to > be updated eventually, probably outside this series). But in the > new context of symrefs, a safe "zero" value needs to be picked > carefully. An empty string may not work well syntactically > especially in the `-z` format, because you cannot tell if > > update-symref NUL HEAD NUL refs/heads/main NUL NUL > > wants to say that <old-ref> is an empty string, or if it is missing. For these reasons, I'd suggest that the all-zero hash be officially deprecated in favor of create/delete and of course create-symref and delete-symref. Of course, compatibility requires some sort of support for the old system for some time. As to whether that means something like the suggestion of ".missing" etc, I have no particular opinion -- but since the symref options are new, they would not *need* symmetric options, if we just say that "update-symref" cannot create or delete a symref. Meanwhile, for testing purposes I was curious as to what happens if you ask `git update-ref` to delete an existing symref, so after creating a test repository: $ git branch * main symref -> main $ git update-ref --stdin delete refs/heads/symref $ git branch Whoops, this doesn't look good... Restoring the branch name (I had saved the hash ID Just In Case): $ echo d88ee82e6a5c29c95f712030f5efc9d43116ae79 > .git/refs/heads/main brings things back, after which this works properly: $ git branch -d symref Deleted branch symref (was refs/heads/main). $ git branch * main If this (deleting the target of the symref when using "delete") is a bug, and I think it is, that's a separate topic of course... Chris
Chris Torek <chris.torek@gmail.com> writes: > For these reasons, I'd suggest that the all-zero hash be officially > deprecated in favor of create/delete and of course create-symref > and delete-symref. Of course, compatibility requires some sort > of support for the old system for some time. As to whether that > means something like the suggestion of ".missing" etc, I have no > particular opinion -- but since the symref options are new, they > would not *need* symmetric options, if we just say that "update-symref" > cannot create or delete a symref. I love that simplicity. > If this (deleting the target of the symref when using "delete") > is a bug, and I think it is, that's a separate topic of course... "git blame" may find those who can give us answers, but my gut feeling is that they weren't thinking too deeply about what should happen, and the code does what it happens to do. When you update, you'd want the update go through a symref (i.e., update-ref HEAD would update your current branch unless you are detached), so they must have been aware of the fact that they need to deal with symrefs. But when you delete a symref, what do you want that deletion affect? I do not have a good answer offhand, and they probably didn't even think of the need to deal with symrefs, as the most common symref you see, HEAD, is not something you'd ever want to delete anyway.
Junio C Hamano <gitster@pobox.com> writes: > Chris Torek <chris.torek@gmail.com> writes: > >> For these reasons, I'd suggest that the all-zero hash be officially >> deprecated in favor of create/delete and of course create-symref >> and delete-symref. Of course, compatibility requires some sort >> of support for the old system for some time. As to whether that >> means something like the suggestion of ".missing" etc, I have no >> particular opinion -- but since the symref options are new, they >> would not *need* symmetric options, if we just say that "update-symref" >> cannot create or delete a symref. > > I love that simplicity. Having said that, the loose "update that can create or delete" may actually be used by applications that do not care about overwriting competing operation, so I am not sure if we can easily deprecate that mode of operation. Saying "update refs/heads/next to point at this object" and have it created if it does not exist may be handy for some loosely written applications. So perhaps we can say "update with a concrete <old-oid> will ensure that the <ref> poitns at <old-oid> before proceeding, but update with 0{40} as <old-oid> to ensure creation is deprecated. update with 0{40} as <new-oid> as deletion is also deprecated. Use create and delete for these two deprecated operation modes". This assumes that create and delete currently ensures that what is asked to be created does not exist, and what is asked to be deleted does exist, before the operation. If we are loosely doing these two operations, then we cannot easily deprecate the checking-update, without breaking existing users. As {create,update,delete,verify}-symref do not exist yet, we can start with the right semantics from day one. "update-symref" will accept a <old-ref> only to ensure that the symref is pointing to that ref and there is no "zero" value based creation/deletion validation offered via "update-symref". "create-symref" will error out if the ref asked to be created already exists, "delete-symref" will error out if the ref asked to be deleted does not exist.
Chris Torek <chris.torek@gmail.com> writes: > Meanwhile, for testing purposes I was curious as to what happens > if you ask `git update-ref` to delete an existing symref, so after > creating a test repository: > > $ git branch > * main > symref -> main > $ git update-ref --stdin > delete refs/heads/symref > $ git branch > > Whoops, this doesn't look good... > This is expected though. Remember that `git-update-ref(1)` by default does dereferencing of symlinks. So in your case, 'refs/heads/symref' is dereferenced to 'refs/heads/main' and when a delete command is issued, 'main' is deleted. Try the same with the `git update-ref --no-deref --stdin` instead. > Restoring the branch name (I had saved the hash ID Just In Case): > > $ echo d88ee82e6a5c29c95f712030f5efc9d43116ae79 > .git/refs/heads/main > > brings things back, after which this works properly: > > $ git branch -d symref > Deleted branch symref (was refs/heads/main). > $ git branch > * main > Here you switch to using 'git-branch(1)' and that doesn't dereference by default. So there is a difference in the two attempts.
Junio C Hamano <gitster@pobox.com> writes: > Karthik Nayak <karthik.188@gmail.com> writes: > >> diff --git a/Documentation/git-update-ref.txt b/Documentation/git-update-ref.txt >> index 0561808cca..2ea8bc8167 100644 >> --- a/Documentation/git-update-ref.txt >> +++ b/Documentation/git-update-ref.txt >> @@ -65,6 +65,7 @@ performs all modifications together. Specify commands of the form: > > In pre-context, there is this entry. > > update SP <ref> SP <newvalue> [SP <oldvalue>] LF > > Unrelated to this patch, we probably should say <new-value> and > <old-value> to follow the (recent) documentation guideline to write > multiple words concatenated with hyphen. > > If we are updating these newvalue and oldvalue anyway, we may want > to update them to <new-oid> and <old-oid>, as the existing commands > on refs are about object names, and what we are adding here are not. > > I would prefer to see such an update of existing documentation as a > separate preliminary clean-up patch, so that we can cleanly add the > "update-symref" that takes <new-symref-target> on top of a > cleaned-up base. > > The <newvalue> ... > I agree with your synopsis here, I'll send in a patch for this, independent of these changes as a precursor, while we discuss the final design of this series. >> Chris Torek <chris.torek@gmail.com> writes: >> >>> For these reasons, I'd suggest that the all-zero hash be officially >>> deprecated in favor of create/delete and of course create-symref >>> and delete-symref. Of course, compatibility requires some sort >>> of support for the old system for some time. As to whether that >>> means something like the suggestion of ".missing" etc, I have no >>> particular opinion -- but since the symref options are new, they >>> would not *need* symmetric options, if we just say that "update-symref" >>> cannot create or delete a symref. >> >> I love that simplicity. > > Having said that, the loose "update that can create or delete" may > actually be used by applications that do not care about overwriting > competing operation, so I am not sure if we can easily deprecate > that mode of operation. Saying "update refs/heads/next to point at > this object" and have it created if it does not exist may be handy > for some loosely written applications. > > So perhaps we can say "update with a concrete <old-oid> will ensure > that the <ref> poitns at <old-oid> before proceeding, but update > with 0{40} as <old-oid> to ensure creation is deprecated. update > with 0{40} as <new-oid> as deletion is also deprecated. Use create > and delete for these two deprecated operation modes". > > This assumes that create and delete currently ensures that what is > asked to be created does not exist, and what is asked to be deleted > does exist, before the operation. If we are loosely doing these two > operations, then we cannot easily deprecate the checking-update, > without breaking existing users. > > As {create,update,delete,verify}-symref do not exist yet, we can > start with the right semantics from day one. "update-symref" will > accept a <old-ref> only to ensure that the symref is pointing to > that ref and there is no "zero" value based creation/deletion > validation offered via "update-symref". "create-symref" will error > out if the ref asked to be created already exists, "delete-symref" > will error out if the ref asked to be deleted does not exist. This seems very reasonable to me. This ensures that each of the commands do not spill over to the other and most importantly we don't have to deal with "zero" values. But this still means we need to think of the best output for the reference transaction hook (following commit). My current implementation of: <symref-target> SP <ref-name> LF Should be changed to: <old-ref> SP <new-ref> LF But this means, for creation of symrefs <old-ref> needs to be "zero" value. Also there is no way for clients to differentiate between regular refs and symrefs here. I wonder if it makes sense to do something like: symref SP <old-ref> SP <new-ref> LF Where symref is a fixed string at the start, used as a differentiator. This however still would leave us to deal with "zero" values for creation and deletion. Perhaps the best way here to actually be a lot more verbose and have the hook output the following: symref-create SP <new-ref> LF symref-delete SP <old-ref> LF symref-update SP <old-ref> SP <new-ref> LF symref-update-forced <new-ref> LF I lean towards the latter, because its also much easier to extend in the future and we don't have to deal with the "zero" values. I'm against the "zero" values mostly cause there is no nice way to describe a zero value for a ref unlike OIDs, which is inherently baked into the type of data.
Karthik Nayak <karthik.188@gmail.com> writes: >> So perhaps we can say "update with a concrete <old-oid> will ensure >> that the <ref> poitns at <old-oid> before proceeding, but update >> with 0{40} as <old-oid> to ensure creation is deprecated. update >> with 0{40} as <new-oid> as deletion is also deprecated. Use create >> and delete for these two deprecated operation modes". >> >> This assumes that create and delete currently ensures that what is >> asked to be created does not exist, and what is asked to be deleted >> does exist, before the operation. If we are loosely doing these two >> operations, then we cannot easily deprecate the checking-update, >> without breaking existing users. Note that I did not (and do not) know if "create" and "delete" have such checks; I expected somebody (other than me) to check before going forward. > But this still means we need to think of the best output for the > reference transaction hook (following commit). > > My current implementation of: > <symref-target> SP <ref-name> LF > Should be changed to: > <old-ref> SP <new-ref> LF > > But this means, for creation of symrefs <old-ref> needs to be "zero" > value. Also there is no way for clients to differentiate between regular > refs and symrefs here. I wonder if it makes sense to do something like: > > symref SP <old-ref> SP <new-ref> LF What do clients see for a regular ref? "<old-oid> SP <new-oid> LF"? That would be OK, as "symref" cannot be an object name, I guess? > Where symref is a fixed string at the start, used as a differentiator. > This however still would leave us to deal with "zero" values for > creation and deletion. Are these two <old-ref> and <new-ref> values optional in the context of your discussion? The review comment was on input from the end-user that made it optional to validate the precondition, but this is what you produce as a result---if these are not optional, then an empty string can be a reasonable "zero" value. I am confused... > Perhaps the best way here to actually be a lot more verbose and have the > hook output the following: > > symref-create SP <new-ref> LF > symref-delete SP <old-ref> LF > symref-update SP <old-ref> SP <new-ref> LF > symref-update-forced <new-ref> LF It is unfortunate that the input to the hook for a normal reference update uses syntax different from the "--stdin" input format, i.e., <old-oid> SP <new-oid> SP <ref> LF but it is way too late to change it now. So to be consistent, symref-create SP <new-ref> SP <ref> LF symref-delete SP <old-ref> SP <ref> LF symref-update SP <old-ref> SP <new-ref> SP <ref> LF would be the three operations. But this is not an end-user input that tells Git "I do not care about precondition, I did not even bother to learn the current state to give you as <old-something>, just force it". The input to hook is what we tell the hook what we are planning to do (so that it can decline), and we do not need the ability to say "I do not know what the current state is". So I do not think you need any "zero" value in the input to the reference-transaction hook. And I do not see a need for the "symref-update-forced" variant, either. By the way, if we were to use symref-{create,delete,update} here, wouldn't it make more sense to name the command on the "--stdin" side the same, i.e., not "update-symref" but "symref-update"? What I suspect that needs more thought is what should happen when you request via "--stdin" to create, update, or delete a symref, but <ref> is a regular ref, e.g., "symref-delete <ref>". For "symref-create <ref> <new-ref>", we would fail if <ref> exists, whether it is a symref or a normal ref, so that is OK. For "symref-delete <ref> <old-ref>", we would fail if <ref> is *not* a symref to <old-ref>, so the case where <ref> is a normal ref is already covered. Should we support "symref-update <ref> <new-ref> <old-oid>" that makes <ref> a symref that points at <new-ref>, but ensures that <ref> before the operation is a normal ref that points at <old-oid>? Or should "symref-update" work only on <ref> that is an existing symref? I think I am OK if the answer was "You can only give a precondition in the form of <old-ref>, which means you can only turn an existing symref to point at a different ref with precondition protection. If you want to turn a normal ref into a symref, you have to force it by not having a precondition, or delete the ref and then (re)create it as a symref". But we need to decide the semantics and document it. Thanks.
Junio C Hamano <gitster@pobox.com> writes: >> But this still means we need to think of the best output for the >> reference transaction hook (following commit). >> ... One thing I missed. We are not currently reporting symref updates to these hooks. Are they prepared to take such extra input? If not, are they going to barf when they see "symref-update" while expecting to see <old-oid>? We may need to make it possible for Git to tell which variant of the hook script it was given somehow (the easiest cop-out is to introduce ref-transaction-hook-v2 as a separate hook, and use it if exists, or fall back to the reference-transaction hook, and report symref updates only when we are using v2, but there may be better approaches). > But this is not an end-user input that tells Git "I do not care > about precondition, I did not even bother to learn the current state > to give you as <old-something>, just force it". The input to hook > is what we tell the hook what we are planning to do (so that it can > decline), and we do not need the ability to say "I do not know what > the current state is". So I do not think you need any "zero" value > in the input to the reference-transaction hook. And I do not see a > need for the "symref-update-forced" variant, either. I misspoke here. We do need "zero" value to indicate that "this update is a creation event" and "this update is a deletion event". What I meant to say is that there is no need to make the "zero" value distinguishable from a "missing optional" value, which was a problem on the "--stdin" side with "-z" format, where each command is in a format with fixed number of parameters, unlike the textual format, where a missing optional argument can be expressed by omitting SP before the value and the value itself and it can be differentiated from an empty string as an optional value that is not missing. Thanks.
Junio C Hamano <gitster@pobox.com> writes: > Karthik Nayak <karthik.188@gmail.com> writes: > >>> So perhaps we can say "update with a concrete <old-oid> will ensure >>> that the <ref> poitns at <old-oid> before proceeding, but update >>> with 0{40} as <old-oid> to ensure creation is deprecated. update >>> with 0{40} as <new-oid> as deletion is also deprecated. Use create >>> and delete for these two deprecated operation modes". >>> >>> This assumes that create and delete currently ensures that what is >>> asked to be created does not exist, and what is asked to be deleted >>> does exist, before the operation. If we are loosely doing these two >>> operations, then we cannot easily deprecate the checking-update, >>> without breaking existing users. > > Note that I did not (and do not) know if "create" and "delete" have > such checks; I expected somebody (other than me) to check before > going forward. > create does: $ git update-ref --stdin create refs/heads/a 53eb33454ce4f3db4d8c79e9c15640c2dffc4a37 fatal: cannot lock ref 'refs/heads/a': reference already exists delete doesn't: $ git update-ref --stdin delete refs/heads/b % echo $? 0 $ ls .git/refs/heads/ a master >> But this still means we need to think of the best output for the >> reference transaction hook (following commit). >> >> My current implementation of: >> <symref-target> SP <ref-name> LF >> Should be changed to: >> <old-ref> SP <new-ref> LF >> >> But this means, for creation of symrefs <old-ref> needs to be "zero" >> value. Also there is no way for clients to differentiate between regular >> refs and symrefs here. I wonder if it makes sense to do something like: >> >> symref SP <old-ref> SP <new-ref> LF > > What do clients see for a regular ref? "<old-oid> SP <new-oid> LF"? > That would be OK, as "symref" cannot be an object name, I guess? > <old-value> SP <new-value> SP <ref-name> LF Yeah, 'symref' should work. >> Where symref is a fixed string at the start, used as a differentiator. >> This however still would leave us to deal with "zero" values for >> creation and deletion. > > Are these two <old-ref> and <new-ref> values optional in the context > of your discussion? The review comment was on input from the end-user > that made it optional to validate the precondition, but this is what > you produce as a result---if these are not optional, then an empty > string can be a reasonable "zero" value. I am confused... >> Perhaps the best way here to actually be a lot more verbose and have the >> hook output the following: >> >> symref-create SP <new-ref> LF >> symref-delete SP <old-ref> LF >> symref-update SP <old-ref> SP <new-ref> LF >> symref-update-forced <new-ref> LF > > It is unfortunate that the input to the hook for a normal reference > update uses syntax different from the "--stdin" input format, i.e., > > <old-oid> SP <new-oid> SP <ref> LF > > but it is way too late to change it now. So to be consistent, > > symref-create SP <new-ref> SP <ref> LF > symref-delete SP <old-ref> SP <ref> LF > symref-update SP <old-ref> SP <new-ref> SP <ref> LF > > would be the three operations. > > But this is not an end-user input that tells Git "I do not care > about precondition, I did not even bother to learn the current state > to give you as <old-something>, just force it". The input to hook > is what we tell the hook what we are planning to do (so that it can > decline), and we do not need the ability to say "I do not know what > the current state is". So I do not think you need any "zero" value > in the input to the reference-transaction hook. And I do not see a > need for the "symref-update-forced" variant, either. ... also from your latest email ... >> But this is not an end-user input that tells Git "I do not care >> about precondition, I did not even bother to learn the current state >> to give you as <old-something>, just force it". The input to hook >> is what we tell the hook what we are planning to do (so that it can >> decline), and we do not need the ability to say "I do not know what >> the current state is". So I do not think you need any "zero" value >> in the input to the reference-transaction hook. And I do not see a >> need for the "symref-update-forced" variant, either. > > I misspoke here. We do need "zero" value to indicate that "this > update is a creation event" and "this update is a deletion event". > What I meant to say is that there is no need to make the "zero" > value distinguishable from a "missing optional" value, which was a > problem on the "--stdin" side with "-z" format, where each command > is in a format with fixed number of parameters, unlike the textual > format, where a missing optional argument can be expressed by > omitting SP before the value and the value itself and it can be > differentiated from an empty string as an optional value that is not > missing. > > Thanks. Yup. There is a slight subtlety here though, currently with the reference-transaction hook: When force updating the reference regardless of its current value or when the reference is to be created anew, <old-value> is the all-zeroes object name. To distinguish these cases, you can inspect the current value of <ref-name> via git rev-parse. I'll keep the same behavior with the symref updates. > By the way, if we were to use symref-{create,delete,update} here, > wouldn't it make more sense to name the command on the "--stdin" > side the same, i.e., not "update-symref" but "symref-update"? If we were to use symref-*, definitely. > What I suspect that needs more thought is what should happen when > you request via "--stdin" to create, update, or delete a symref, > but <ref> is a regular ref, e.g., "symref-delete <ref>". For > "symref-create <ref> <new-ref>", we would fail if <ref> exists, > whether it is a symref or a normal ref, so that is OK. For > "symref-delete <ref> <old-ref>", we would fail if <ref> is *not* > a symref to <old-ref>, so the case where <ref> is a normal ref > is already covered. > > Should we support "symref-update <ref> <new-ref> <old-oid>" that > makes <ref> a symref that points at <new-ref>, but ensures that > <ref> before the operation is a normal ref that points at <old-oid>? > > Or should "symref-update" work only on <ref> that is an existing > symref? > > I think I am OK if the answer was "You can only give a precondition > in the form of <old-ref>, which means you can only turn an existing > symref to point at a different ref with precondition protection. If > you want to turn a normal ref into a symref, you have to force it by > not having a precondition, or delete the ref and then (re)create it > as a symref". But we need to decide the semantics and document it. I would think that doing what you mentioned in the last para to be the way to go, unless someone thinks otherwise. This allows the ugly mess of parsing an value as a ref and then as a oid and provides some structure to what the input cases are. One more case I'd add is that the <ref> argument for "symref-delete" should be optional, similar to having forced update, we'd also want to support forced deletion. > Junio C Hamano <gitster@pobox.com> writes: > >>> But this still means we need to think of the best output for the >>> reference transaction hook (following commit). >>> ... > > One thing I missed. We are not currently reporting symref updates > to these hooks. Are they prepared to take such extra input? If not, > are they going to barf when they see "symref-update" while expecting > to see <old-oid>? > > We may need to make it possible for Git to tell which variant of the > hook script it was given somehow (the easiest cop-out is to introduce > ref-transaction-hook-v2 as a separate hook, and use it if exists, or > fall back to the reference-transaction hook, and report symref updates > only when we are using v2, but there may be better approaches). > Well I was hoping this is okay since reference-transaction and update-ref never supported symrefs. So after this change: 1. If a user never updates the hook to support symrefs, but doesn't use symref feature of update-ref, they would be fine. 2. If a user starts using symref features of update-ref, they would see that reference transaction needs to be updated too. This especially since the hook's documentation always claimed that symref support might be added in the future. The hook does not cover symbolic references (but that may change in the future). --- In summary the plan going forward from my side would be to: Implement the following in update-ref: symref-create SP <ref> SP <new-ref> LF symref-update SP <ref> SP <new-ref> [SP <old-ref>] LF symref-delete SP <ref> [SP <old-ref>] LF symref-verify SP <ref> [SP <old-ref>] LF Also on the reference transaction hook, we'll be adding the following new inputs to the hook: symref-create SP <new-ref> SP <ref> LF symref-delete SP <old-ref> SP <ref> LF symref-update SP <old-ref> SP <new-ref> SP <ref> LF This either will be added to the existing hook or we would support a new hook (v2).
On Sun, Mar 31, 2024 at 06:31:14PM -0700, Junio C Hamano wrote: > Junio C Hamano <gitster@pobox.com> writes: > > > Chris Torek <chris.torek@gmail.com> writes: > > > >> For these reasons, I'd suggest that the all-zero hash be officially > >> deprecated in favor of create/delete and of course create-symref > >> and delete-symref. Of course, compatibility requires some sort > >> of support for the old system for some time. As to whether that > >> means something like the suggestion of ".missing" etc, I have no > >> particular opinion -- but since the symref options are new, they > >> would not *need* symmetric options, if we just say that "update-symref" > >> cannot create or delete a symref. > > > > I love that simplicity. > > Having said that, the loose "update that can create or delete" may > actually be used by applications that do not care about overwriting > competing operation, so I am not sure if we can easily deprecate > that mode of operation. Saying "update refs/heads/next to point at > this object" and have it created if it does not exist may be handy > for some loosely written applications. I wouldn't say "loosely written here". I certainly know that we do use these implicit modes in Gitaly, and we have conciously chosen them because they have been supported by Git all along. It simply makes our lifes easier when we don't have to special-case creations and deletions in any way. So I'd really not want those to go away or become deprecated. Patrick > So perhaps we can say "update with a concrete <old-oid> will ensure > that the <ref> poitns at <old-oid> before proceeding, but update > with 0{40} as <old-oid> to ensure creation is deprecated. update > with 0{40} as <new-oid> as deletion is also deprecated. Use create > and delete for these two deprecated operation modes". > > This assumes that create and delete currently ensures that what is > asked to be created does not exist, and what is asked to be deleted > does exist, before the operation. If we are loosely doing these two > operations, then we cannot easily deprecate the checking-update, > without breaking existing users. > > As {create,update,delete,verify}-symref do not exist yet, we can > start with the right semantics from day one. "update-symref" will > accept a <old-ref> only to ensure that the symref is pointing to > that ref and there is no "zero" value based creation/deletion > validation offered via "update-symref". "create-symref" will error > out if the ref asked to be created already exists, "delete-symref" > will error out if the ref asked to be deleted does not exist. >
Patrick Steinhardt <ps@pks.im> writes: > because they have been supported by Git all along. It simply makes our > lifes easier when we don't have to special-case creations and deletions > in any way. > > So I'd really not want those to go away or become deprecated. That is a good input. Do you have anything to add as a counter-proposal? The "I do not care what was there before" update mode does make it necessary to have a "zero" value for symrefs that can be distinguishable from not having a value at all. Thanks.
On Tue, Apr 02, 2024 at 09:40:41AM -0700, Junio C Hamano wrote: > Patrick Steinhardt <ps@pks.im> writes: > > > because they have been supported by Git all along. It simply makes our > > lifes easier when we don't have to special-case creations and deletions > > in any way. > > > > So I'd really not want those to go away or become deprecated. > > That is a good input. > > Do you have anything to add as a counter-proposal? The "I do not > care what was there before" update mode does make it necessary to > have a "zero" value for symrefs that can be distinguishable from > not having a value at all. > > Thanks. Sorry for taking this long to answer your question. I might have missed it while scanning through this thread, but why exactly is the zero OID not a good enough placeholder here to say that the ref must not exist? A symref cannot point to a ref named after the zero OID anyway. In my opinion, "update-symref" with an old-value must be able to accept both object IDs and symrefs as old value. Like this it would be possible to update a proper ref to a symref in a race-free way. So you can say: git update-ref SYMREF refs/heads/main 19981daefd7c147444462739375462b49412ce33 To update "SYRMEF" to "refs/heads/main", but only in case it currently is a proper ref that points to 19981daefd7c147444462739375462b49412ce33. Similarly... git update-ref SYMREF refs/heads/main refs/heads/master would update "SYMREF" to "refs/heads/main", but only if it currently points to the symref "refs/heads/master". And by extension I think that the zero OID should retain its established meaning of "This ref must not exist": git update-ref SYMREF refs/heads/main 0000000000000000000000000000000000000000 This would only update "SYMREF" to "refs/heads/main" if it does not yet exist. Patrick
Patrick Steinhardt <ps@pks.im> writes: > On Tue, Apr 02, 2024 at 09:40:41AM -0700, Junio C Hamano wrote: >> Patrick Steinhardt <ps@pks.im> writes: >> >> > because they have been supported by Git all along. It simply makes our >> > lifes easier when we don't have to special-case creations and deletions >> > in any way. >> > >> > So I'd really not want those to go away or become deprecated. >> >> That is a good input. >> >> Do you have anything to add as a counter-proposal? The "I do not >> care what was there before" update mode does make it necessary to >> have a "zero" value for symrefs that can be distinguishable from >> not having a value at all. >> >> Thanks. > > Sorry for taking this long to answer your question. > > I might have missed it while scanning through this thread, but why > exactly is the zero OID not a good enough placeholder here to say that > the ref must not exist? A symref cannot point to a ref named after the > zero OID anyway. > > In my opinion, "update-symref" with an old-value must be able to accept > both object IDs and symrefs as old value. Like this it would be possible > to update a proper ref to a symref in a race-free way. So you can say: > > git update-ref SYMREF refs/heads/main 19981daefd7c147444462739375462b49412ce33 > > To update "SYRMEF" to "refs/heads/main", but only in case it currently > is a proper ref that points to 19981daefd7c147444462739375462b49412ce33. > Similarly... > > git update-ref SYMREF refs/heads/main refs/heads/master > > would update "SYMREF" to "refs/heads/main", but only if it currently > points to the symref "refs/heads/master". And by extension I think that > the zero OID should retain its established meaning of "This ref must not > exist": > > git update-ref SYMREF refs/heads/main 0000000000000000000000000000000000000000 > > This would only update "SYMREF" to "refs/heads/main" if it does not yet > exist. > This is definitely nicer experience for the user. From looking at the other commands, {verify, create, delete} I can only see this applying to `symref-update`. Making the syntax for update something like: symref-update SP <ref> SP <new-ref> [SP (<old-oid> | <old-rev>)] LF I think this makes sense to me, will incorporate this and send the next version in the next few days. On a side-note: This would also mean that we should somehow support moving from symrefs to a regular ref via a transaction, so that means we should allow update SP <ref> SP <new-oid> [SP (<old-oid> | <old-rev>)] LF too, but I'm not going to tackle that in my patches.
On Tue, Apr 09, 2024 at 09:15:59AM -0700, Karthik Nayak wrote: > Patrick Steinhardt <ps@pks.im> writes: > > > On Tue, Apr 02, 2024 at 09:40:41AM -0700, Junio C Hamano wrote: > >> Patrick Steinhardt <ps@pks.im> writes: > >> > >> > because they have been supported by Git all along. It simply makes our > >> > lifes easier when we don't have to special-case creations and deletions > >> > in any way. > >> > > >> > So I'd really not want those to go away or become deprecated. > >> > >> That is a good input. > >> > >> Do you have anything to add as a counter-proposal? The "I do not > >> care what was there before" update mode does make it necessary to > >> have a "zero" value for symrefs that can be distinguishable from > >> not having a value at all. > >> > >> Thanks. > > > > Sorry for taking this long to answer your question. > > > > I might have missed it while scanning through this thread, but why > > exactly is the zero OID not a good enough placeholder here to say that > > the ref must not exist? A symref cannot point to a ref named after the > > zero OID anyway. > > > > In my opinion, "update-symref" with an old-value must be able to accept > > both object IDs and symrefs as old value. Like this it would be possible > > to update a proper ref to a symref in a race-free way. So you can say: > > > > git update-ref SYMREF refs/heads/main 19981daefd7c147444462739375462b49412ce33 > > > > To update "SYRMEF" to "refs/heads/main", but only in case it currently > > is a proper ref that points to 19981daefd7c147444462739375462b49412ce33. > > Similarly... > > > > git update-ref SYMREF refs/heads/main refs/heads/master > > > > would update "SYMREF" to "refs/heads/main", but only if it currently > > points to the symref "refs/heads/master". And by extension I think that > > the zero OID should retain its established meaning of "This ref must not > > exist": > > > > git update-ref SYMREF refs/heads/main 0000000000000000000000000000000000000000 > > > > This would only update "SYMREF" to "refs/heads/main" if it does not yet > > exist. > > > > This is definitely nicer experience for the user. From looking at the > other commands, {verify, create, delete} I can only see this applying to > `symref-update`. Making the syntax for update something like: > > symref-update SP <ref> SP <new-ref> [SP (<old-oid> | <old-rev>)] LF > > I think this makes sense to me, will incorporate this and send the next > version in the next few days. > > On a side-note: This would also mean that we should somehow support > moving from symrefs to a regular ref via a transaction, so that means we > should allow > > update SP <ref> SP <new-oid> [SP (<old-oid> | <old-rev>)] LF > > too, but I'm not going to tackle that in my patches. Yes, I think that would be a sensible idea, even though we have to be careful with backwards compatibility here. In any case, I think it makes sense to not extend the scope of your patch series and leave this for the future. Patrick
Patrick Steinhardt <ps@pks.im> writes: >> > I might have missed it while scanning through this thread, but why >> > exactly is the zero OID not a good enough placeholder here to say that >> > the ref must not exist? A symref cannot point to a ref named after the >> > zero OID anyway. > >> > In my opinion, "update-symref" with an old-value must be able to accept >> > both object IDs and symrefs as old value. Like this it would be possible >> > to update a proper ref to a symref in a race-free way. So you can say: >> > >> > git update-ref SYMREF refs/heads/main 19981daefd7c147444462739375462b49412ce33 > >> > To update "SYRMEF" to "refs/heads/main", but only in case it currently >> > is a proper ref that points to 19981daefd7c147444462739375462b49412ce33. >> > Similarly... >> > >> > git update-ref SYMREF refs/heads/main refs/heads/master > >> > would update "SYMREF" to "refs/heads/main", but only if it currently >> > points to the symref "refs/heads/master". I think that would work well. We need to explicitly forbid a file $GIT_DIR/[0-9a-f]{40} to be used as a pseudoref, which I think that is an improvement. I do not know how the transition to move to a world with a stricter rule would look like, though. >> > And by extension I think that >> > the zero OID should retain its established meaning of "This ref must not >> > exist": >> > >> > git update-ref SYMREF refs/heads/main 0000000000000000000000000000000000000000 >> > >> > This would only update "SYMREF" to "refs/heads/main" if it does not yet >> > exist. We express "SYMREF must currently be a symref" by naming an old-ref, and we say "SYMREF can currently be either a ref or symref, but it must point at this object" by naming an old-oid. The looseness of the latter (i.e. we cannot express "Before making HEAD point at ref X, ensure that it is detached at OID") looks a bit disturbing, but otherwise looks good. I offhand do not know how expressive we would want the "old" requirement to be. >> On a side-note: This would also mean that we should somehow support >> moving from symrefs to a regular ref via a transaction, so that means we >> should allow >> >> update SP <ref> SP <new-oid> [SP (<old-oid> | <old-rev>)] LF >> >> too, but I'm not going to tackle that in my patches. > > Yes, I think that would be a sensible idea, even though we have to be > careful with backwards compatibility here. In any case, I think it makes > sense to not extend the scope of your patch series and leave this for > the future. Likewise, we may want to be able to express "Before making HEAD detached at commit X, ensure that HEAD points at ref Y that points at commit Z". IOW, the "old" part might have to be not [SP (<old-oid> | <old-ref>)] but [SP <old-oid> SP <old-ref>] to specify both, perhaps?
On Wed, Apr 10, 2024 at 09:06:45AM -0700, Junio C Hamano wrote: > Patrick Steinhardt <ps@pks.im> writes: > > >> > I might have missed it while scanning through this thread, but why > >> > exactly is the zero OID not a good enough placeholder here to say that > >> > the ref must not exist? A symref cannot point to a ref named after the > >> > zero OID anyway. > > > >> > In my opinion, "update-symref" with an old-value must be able to accept > >> > both object IDs and symrefs as old value. Like this it would be possible > >> > to update a proper ref to a symref in a race-free way. So you can say: > >> > > >> > git update-ref SYMREF refs/heads/main 19981daefd7c147444462739375462b49412ce33 > > > >> > To update "SYRMEF" to "refs/heads/main", but only in case it currently > >> > is a proper ref that points to 19981daefd7c147444462739375462b49412ce33. > >> > Similarly... > >> > > >> > git update-ref SYMREF refs/heads/main refs/heads/master > > > >> > would update "SYMREF" to "refs/heads/main", but only if it currently > >> > points to the symref "refs/heads/master". > > I think that would work well. We need to explicitly forbid a file > $GIT_DIR/[0-9a-f]{40} to be used as a pseudoref, which I think that > is an improvement. I do not know how the transition to move to a > world with a stricter rule would look like, though. I thought that Git already refuses such refnames anyway. Otherwise it would be impossible to distinguish a ref called [0-9a-f]{40} from the actual object hash in much of our tooling. I certainly know that GitLab does refuse such refnames, and thought that GitHub does, too. But turns out that at least git-update-ref(1) is happy to write such refs: ``` $ git update-ref 1111111111111111111111111111111111111111 HEAD $ cat .git/1111111111111111111111111111111111111111 cf6ba211cd2fce88f5d22d9f036029d502565509 ``` What does it resolve to? ``` $ git rev-parse --verify 1111111111111111111111111111111111111111 warning: refname '1111111111111111111111111111111111111111' is ambiguous. Git normally never creates a ref that ends with 40 hex characters because it will be ignored when you just specify 40-hex. These refs may be created by mistake. For example, git switch -c $br $(git rev-parse ...) where "$br" is somehow empty and a 40-hex ref is created. Please examine these refs and maybe delete them. Turn this message off by running "git config advice.objectNameWarning false" 1111111111111111111111111111111111111111 ``` It resolves to 1111111111111111111111111111111111111111 because it's a valid object ID already. And what if we try to peel it? ``` $ git rev-parse --verify 1111111111111111111111111111111111111111^{commit} warning: refname '1111111111111111111111111111111111111111' is ambiguous. Git normally never creates a ref that ends with 40 hex characters because it will be ignored when you just specify 40-hex. These refs may be created by mistake. For example, git switch -c $br $(git rev-parse ...) where "$br" is somehow empty and a 40-hex ref is created. Please examine these refs and maybe delete them. Turn this message off by running "git config advice.objectNameWarning false" fatal: Needed a single revision ``` Doesn't work. So these refs essentially cannot be accessed anyway. Restricting git-update-ref(1) such that it cannot write them in the first place thus shouldn't be breaking much, I'd claim, and feels like a strict improvement overall. Patrick
diff --git a/Documentation/git-update-ref.txt b/Documentation/git-update-ref.txt index 0561808cca..2ea8bc8167 100644 --- a/Documentation/git-update-ref.txt +++ b/Documentation/git-update-ref.txt @@ -65,6 +65,7 @@ performs all modifications together. Specify commands of the form: create SP <ref> SP <newvalue> LF delete SP <ref> [SP <oldvalue>] LF verify SP <ref> [SP <oldvalue>] LF + update-symref SP <ref> SP <newvalue> LF option SP <opt> LF start LF prepare LF @@ -86,6 +87,7 @@ quoting: create SP <ref> NUL <newvalue> NUL delete SP <ref> NUL [<oldvalue>] NUL verify SP <ref> NUL [<oldvalue>] NUL + update-symref NUL <ref> NUL <newvalue> NUL option SP <opt> NUL start NUL prepare NUL @@ -111,12 +113,19 @@ create:: delete:: Delete <ref> after verifying it exists with <oldvalue>, if - given. If given, <oldvalue> may not be zero. + given. If given, <oldvalue> may not be zero. Can also delete + symrefs. verify:: Verify <ref> against <oldvalue> but do not change it. If <oldvalue> is zero or missing, the ref must not exist. +update-symref:: + Update <ref> as a symbolic reference to point to the given + reference <newvalue>. By default, <ref> will be recursively + de-referenced, unless the `no-deref` option is used. Can also + be used to create new symrefs. + option:: Modify the behavior of the next command naming a <ref>. The only valid option is `no-deref` to avoid dereferencing diff --git a/builtin/update-ref.c b/builtin/update-ref.c index 3807cf4106..357daf31b8 100644 --- a/builtin/update-ref.c +++ b/builtin/update-ref.c @@ -213,6 +213,48 @@ static void parse_cmd_update(struct ref_transaction *transaction, strbuf_release(&err); } +static void parse_cmd_update_symref(struct ref_transaction *transaction, + const char *next, const char *end) +{ + struct strbuf err = STRBUF_INIT; + char *refname, *symref; + + refname = parse_refname(&next); + if (!refname) + die("update-symref: missing <ref>"); + + if (line_termination) { + /* Without -z, consume SP and use next argument */ + if (!*next || *next == line_termination) + die("update-symref %s: missing <newvalue>", refname); + if (*next != ' ') + die("update-symref %s: expected SP but got: %s", + refname, next); + } else { + /* With -z, read the next NUL-terminated line */ + if (*next) + die("update-symref %s: missing <newvalue>", refname); + } + next++; + + symref = parse_refname(&next); + if (!symref) + die("update-symref %s: missing <newvalue>", refname); + + if (*next != line_termination) + die("update-symref %s: extra input: %s", refname, next); + + if (ref_transaction_update(transaction, refname, NULL, NULL, + update_flags | create_reflog_flag | REF_UPDATE_SYMREF, + msg, symref, &err)) + die("%s", err.buf); + + update_flags = default_flags; + free(symref); + free(refname); + strbuf_release(&err); +} + static void parse_cmd_create(struct ref_transaction *transaction, const char *next, const char *end) { @@ -379,15 +421,16 @@ static const struct parse_cmd { unsigned args; enum update_refs_state state; } command[] = { - { "update", parse_cmd_update, 3, UPDATE_REFS_OPEN }, - { "create", parse_cmd_create, 2, UPDATE_REFS_OPEN }, - { "delete", parse_cmd_delete, 2, UPDATE_REFS_OPEN }, - { "verify", parse_cmd_verify, 2, UPDATE_REFS_OPEN }, - { "option", parse_cmd_option, 1, UPDATE_REFS_OPEN }, - { "start", parse_cmd_start, 0, UPDATE_REFS_STARTED }, - { "prepare", parse_cmd_prepare, 0, UPDATE_REFS_PREPARED }, - { "abort", parse_cmd_abort, 0, UPDATE_REFS_CLOSED }, - { "commit", parse_cmd_commit, 0, UPDATE_REFS_CLOSED }, + { "update", parse_cmd_update, 3, UPDATE_REFS_OPEN }, + { "update-symref", parse_cmd_update_symref, 2, UPDATE_REFS_OPEN }, + { "create", parse_cmd_create, 2, UPDATE_REFS_OPEN }, + { "delete", parse_cmd_delete, 2, UPDATE_REFS_OPEN }, + { "verify", parse_cmd_verify, 2, UPDATE_REFS_OPEN }, + { "option", parse_cmd_option, 1, UPDATE_REFS_OPEN }, + { "start", parse_cmd_start, 0, UPDATE_REFS_STARTED }, + { "prepare", parse_cmd_prepare, 0, UPDATE_REFS_PREPARED }, + { "abort", parse_cmd_abort, 0, UPDATE_REFS_CLOSED }, + { "commit", parse_cmd_commit, 0, UPDATE_REFS_CLOSED }, }; static void update_refs_stdin(void) diff --git a/refs.c b/refs.c index 69b89a1aa2..706dcd6deb 100644 --- a/refs.c +++ b/refs.c @@ -1216,6 +1216,7 @@ void ref_transaction_free(struct ref_transaction *transaction) } for (i = 0; i < transaction->nr; i++) { + free(transaction->updates[i]->symref_target); free(transaction->updates[i]->msg); free(transaction->updates[i]); } @@ -1235,6 +1236,9 @@ struct ref_update *ref_transaction_add_update( if (transaction->state != REF_TRANSACTION_OPEN) BUG("update called for transaction that is not open"); + if ((flags & (REF_HAVE_NEW | REF_UPDATE_SYMREF)) == (REF_HAVE_NEW | REF_UPDATE_SYMREF)) + BUG("cannot create regular ref and symref at once"); + FLEX_ALLOC_STR(update, refname, refname); ALLOC_GROW(transaction->updates, transaction->nr + 1, transaction->alloc); transaction->updates[transaction->nr++] = update; @@ -1245,6 +1249,8 @@ struct ref_update *ref_transaction_add_update( oidcpy(&update->new_oid, new_oid); if (flags & REF_HAVE_OLD) oidcpy(&update->old_oid, old_oid); + if (flags & REF_UPDATE_SYMREF) + update->symref_target = xstrdup(symref); update->msg = normalize_reflog_message(msg); return update; } @@ -2337,6 +2343,10 @@ static int run_transaction_hook(struct ref_transaction *transaction, for (i = 0; i < transaction->nr; i++) { struct ref_update *update = transaction->updates[i]; + // Reference transaction does not support symbolic updates. + if (update->flags & REF_UPDATE_SYMREF) + continue; + strbuf_reset(&buf); strbuf_addf(&buf, "%s %s %s\n", oid_to_hex(&update->old_oid), diff --git a/t/t0600-reffiles-backend.sh b/t/t0600-reffiles-backend.sh index 64214340e7..6d334cb477 100755 --- a/t/t0600-reffiles-backend.sh +++ b/t/t0600-reffiles-backend.sh @@ -472,4 +472,34 @@ test_expect_success POSIXPERM 'git reflog expire honors core.sharedRepository' ' esac ' +test_expect_success SYMLINKS 'symref transaction supports symlinks' ' + git update-ref refs/heads/new @ && + test_config core.prefersymlinkrefs true && + cat >stdin <<-EOF && + start + update-symref TESTSYMREFONE refs/heads/new + prepare + commit + EOF + git update-ref --no-deref --stdin <stdin && + test_path_is_symlink .git/TESTSYMREFONE && + test "$(test_readlink .git/TESTSYMREFONE)" = refs/heads/new +' + +test_expect_success 'symref transaction supports false symlink config' ' + git update-ref refs/heads/new @ && + test_config core.prefersymlinkrefs false && + cat >stdin <<-EOF && + start + update-symref TESTSYMREFONE refs/heads/new + prepare + commit + EOF + git update-ref --no-deref --stdin <stdin && + test_path_is_file .git/TESTSYMREFONE && + git symbolic-ref TESTSYMREFONE >actual && + echo refs/heads/new >expect && + test_cmp expect actual +' + test_done diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh index 6ebc3ef945..2a6036471b 100755 --- a/t/t1400-update-ref.sh +++ b/t/t1400-update-ref.sh @@ -868,6 +868,105 @@ test_expect_success 'stdin delete symref works flag --no-deref' ' test_cmp expect actual ' +test_expect_success 'stdin update-symref creates symref with --no-deref' ' + # ensure that the symref does not already exist + test_must_fail git symbolic-ref --no-recurse refs/heads/symref && + cat >stdin <<-EOF && + update-symref refs/heads/symref $b + EOF + git update-ref --no-deref --stdin <stdin && + echo $b >expect && + git symbolic-ref --no-recurse refs/heads/symref >actual && + test_cmp expect actual && + test-tool ref-store main for-each-reflog-ent refs/heads/symref >actual && + grep "$Z $(git rev-parse $b)" actual +' + +test_expect_success 'stdin update-symref updates symref with --no-deref' ' + # ensure that the symref already exists + git symbolic-ref --no-recurse refs/heads/symref && + cat >stdin <<-EOF && + update-symref refs/heads/symref $a + EOF + git update-ref --no-deref --stdin <stdin && + echo $a >expect && + git symbolic-ref --no-recurse refs/heads/symref >actual && + test_cmp expect actual && + test-tool ref-store main for-each-reflog-ent refs/heads/symref >actual && + grep "$(git rev-parse $b) $(git rev-parse $a)" actual +' + +test_expect_success 'stdin update-symref noop update with --no-deref' ' + git symbolic-ref --no-recurse refs/heads/symref >actual && + echo $a >expect && + test_cmp expect actual && + cat >stdin <<-EOF && + update-symref refs/heads/symref $a + EOF + git update-ref --no-deref --stdin <stdin && + git symbolic-ref --no-recurse refs/heads/symref >actual && + test_cmp expect actual && + test-tool ref-store main for-each-reflog-ent refs/heads/symref >actual && + grep "$(git rev-parse $a) $(git rev-parse $a)" actual +' + +test_expect_success 'stdin update-symref regular ref with --no-deref' ' + git update-ref refs/heads/regularref $a && + cat >stdin <<-EOF && + update-symref refs/heads/regularref $a + EOF + git update-ref --no-deref --stdin <stdin && + echo $a >expect && + git symbolic-ref --no-recurse refs/heads/regularref >actual && + test_cmp expect actual && + test-tool ref-store main for-each-reflog-ent refs/heads/regularref >actual && + grep "$(git rev-parse $a) $(git rev-parse $a)" actual +' + +test_expect_success 'stdin update-symref creates symref' ' + # delete the ref since it already exists from previous tests + git update-ref --no-deref -d refs/heads/symref && + cat >stdin <<-EOF && + update-symref refs/heads/symref $b + EOF + git update-ref --stdin <stdin && + echo $b >expect && + git symbolic-ref --no-recurse refs/heads/symref >actual && + test_cmp expect actual && + test-tool ref-store main for-each-reflog-ent refs/heads/symref >actual && + grep "$Z $(git rev-parse $b)" actual +' + +test_expect_success 'stdin update-symref updates symref' ' + git update-ref refs/heads/symref2 $b && + git symbolic-ref --no-recurse refs/heads/symref refs/heads/symref2 && + cat >stdin <<-EOF && + update-symref refs/heads/symref $a + EOF + git update-ref --stdin <stdin && + echo $a >expect && + git symbolic-ref --no-recurse refs/heads/symref2 >actual && + test_cmp expect actual && + echo refs/heads/symref2 >expect && + git symbolic-ref --no-recurse refs/heads/symref >actual && + test_cmp expect actual && + test-tool ref-store main for-each-reflog-ent refs/heads/symref >actual && + grep "$(git rev-parse $b) $(git rev-parse $b)" actual +' + +test_expect_success 'stdin update-symref regular ref' ' + git update-ref --no-deref refs/heads/regularref $a && + cat >stdin <<-EOF && + update-symref refs/heads/regularref $a + EOF + git update-ref --stdin <stdin && + echo $a >expect && + git symbolic-ref --no-recurse refs/heads/regularref >actual && + test_cmp expect actual && + test-tool ref-store main for-each-reflog-ent refs/heads/regularref >actual && + grep "$(git rev-parse $a) $(git rev-parse $a)" actual +' + test_expect_success 'stdin delete ref works with right old value' ' echo "delete $b $m~1" >stdin && git update-ref --stdin <stdin && @@ -1641,4 +1740,32 @@ test_expect_success PIPE 'transaction flushes status updates' ' test_cmp expected actual ' +test_expect_success 'transaction can commit symref update' ' + git symbolic-ref TESTSYMREFONE $a && + cat >stdin <<-EOF && + start + update-symref TESTSYMREFONE refs/heads/branch + prepare + commit + EOF + git update-ref --no-deref --stdin <stdin && + echo refs/heads/branch >expect && + git symbolic-ref TESTSYMREFONE >actual && + test_cmp expect actual +' + +test_expect_success 'transaction can abort symref update' ' + git symbolic-ref TESTSYMREFONE $a && + cat >stdin <<-EOF && + start + update-symref TESTSYMREFONE refs/heads/branch + prepare + abort + EOF + git update-ref --no-deref --stdin <stdin && + echo $a >expect && + git symbolic-ref TESTSYMREFONE >actual && + test_cmp expect actual +' + test_done