Re: [PATCH v7 2/8] cherry-pick: treat CHERRY_PICK_HEAD and REVERT_HEAD as refs
On Wed, 2015-07-15 at 09:24 -0700, Junio C Hamano wrote: On reflection, I think that this would also be a reasonable approach to take for stash, which does not fall into any of the listed categories. It's not a pseudoref because it's not all-caps and it starts with refs/. Unlike other pseudorefs, it needs a reflog. But like HEAD and unlike other refs, it (and its reflog) wants to be per-worktree. I think we want stash, unlike HEAD, to be shared across worktrees, and contrib/workdir gets this right. So there is nothing that makes refs/stash like CHERRY_PICK_HEAD at all. I just did a brief survey here and folks here agree with you. So, I'll adjust my patches accordingly. Are there other ref-like-things in this category (which I'll call per-worktree refs)? I guess one set of these is submodules' HEADs. I've been assuming that a secondary worktree of superproject will get its own corresponding secondary worktree of a submodule, which would automatically give per-worktree submodule's HEADs and everything else that has to be per-worktree. Am I missing something or are there any more need for underlying machinery than what we currently have for secondary worktrees for a single project tree? That sounds good to me, but judging by the docs, that's not yet implemented. So I'm going to worry about that when it is implemented. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 2/8] cherry-pick: treat CHERRY_PICK_HEAD and REVERT_HEAD as refs
David Turner dtur...@twopensource.com writes: So I am thinking instead that backends should be required to manage updates to HEAD themselves, and that some functions from refs-be-files would be made generic to help with this. ... For the LMDB backend, I could put most of that code at the LMDB access ... backend, it should still be possible to handle this cleanly. Sounds sensible. On reflection, I think that this would also be a reasonable approach to take for stash, which does not fall into any of the listed categories. It's not a pseudoref because it's not all-caps and it starts with refs/. Unlike other pseudorefs, it needs a reflog. But like HEAD and unlike other refs, it (and its reflog) wants to be per-worktree. I think we want stash, unlike HEAD, to be shared across worktrees, and contrib/workdir gets this right. So there is nothing that makes refs/stash like CHERRY_PICK_HEAD at all. Are there other ref-like-things in this category (which I'll call per-worktree refs)? I guess one set of these is submodules' HEADs. I've been assuming that a secondary worktree of superproject will get its own corresponding secondary worktree of a submodule, which would automatically give per-worktree submodule's HEADs and everything else that has to be per-worktree. Am I missing something or are there any more need for underlying machinery than what we currently have for secondary worktrees for a single project tree? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 2/8] cherry-pick: treat CHERRY_PICK_HEAD and REVERT_HEAD as refs
[j6t to bcc as it looks like his concerns have been addressed] On Fri, 2015-07-10 at 06:30 +0200, Michael Haggerty wrote: On 07/10/2015 12:06 AM, Junio C Hamano wrote: David Turner dtur...@twopensource.com writes: OK, here's my current best idea: 1. A pseudoref is an all-caps file in $GIT_DIR/ that always contains at least a SHA1. CHERRY_PICK_HEAD and REVERT_HEAD are examples. Because HEAD might be a symbolic ref, it is not a pseudoref. Refs backends do not manage pseudorefs. Instead, when a pseudoref (an all-caps ref containing no slashes) is requested (e.g. git rev-parse FETCH_HEAD) the generic refs code checks for the existence of that file and if it exists, returns immediately without hitting the backend. The generic code will refuse to allow updates to pseudorefs. 2. The pluggable refs backend manages all refs other than HEAD. 3. The files backend always manages HEAD. This allows for a reflog and for HEAD to be a symbolic ref. The major complication here is ref transactions -- what if there's a transaction that wants to update e.g. both HEAD and refs/heads/master? An update to the current branch (e.g. git commit) does involve at least update to the reflog of HEAD, the current branch somewhere in refs/heads/ and its log, so it is not what if but is a norm [*1*]. The updating of symlink reflogs in general, and particularly that of HEAD, is not done very cleanly. You can see the code in `commit_ref_update()` (some of it helpfully commented to be a Special hack): * If a reference is modified through a symlink, the symlink is locked rather than the reference itself. * If a reference is modified directly, and HEAD points at it, then the HEAD reflog is amended without locking HEAD. Aside from the lack of proper locking, which could result in races with other processes, we also have the problem that the same reference that is being changed via one of these implicit updates could *also* be being changed directly in the same transaction. Such an update would evade the `ref_update_reject_duplicates()` check. On reflection, I'm not sure my approach makes sense. The problem is that refs backends internally manage recursive updates to symbolic refs, so it is not easy to send update for HEAD to one backend while sending the corresponding refs/heads/master updates to a different one. So I am thinking instead that backends should be required to manage updates to HEAD themselves, and that some functions from refs-be-files would be made generic to help with this. For the LMDB backend, I could put most of that code at the LMDB access layer (which presently simply converts all LMDB errors other than NOT_FOUND to calls to die()). I would intercept reads and writes to HEAD and logs/HEAD and replace them with calls to the appropriate functions. So, for instance, the LMDB backend would still decide whether to write HEAD, but it would delegate to the files backend code to actually write it. Reflog updates are a bit more complicated, because we might end up using a different reflog format for LMDB vs for the files backend, but since all reflog updates are controlled by the backend, it should still be possible to handle this cleanly. On reflection, I think that this would also be a reasonable approach to take for stash, which does not fall into any of the listed categories. It's not a pseudoref because it's not all-caps and it starts with refs/. Unlike other pseudorefs, it needs a reflog. But like HEAD and unlike other refs, it (and its reflog) wants to be per-worktree. Are there other ref-like-things in this category (which I'll call per-worktree refs)? I guess one set of these is submodules' HEADs. I have been planning on punting on these because it looks to me like that's what the files backend does. So, let's leave those aside. I don't think of any others but maybe I'm missing something. I'm also not sure how worktrees handle HEAD's reflog -- there doesn't seem to be anything worktree-specific in refs.c. But perhaps I'm just not understanding that bit of the code. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 2/8] cherry-pick: treat CHERRY_PICK_HEAD and REVERT_HEAD as refs
On Fri, 2015-07-10 at 06:30 +0200, Michael Haggerty wrote: On 07/10/2015 12:06 AM, Junio C Hamano wrote: David Turner dtur...@twopensource.com writes: OK, here's my current best idea: 1. A pseudoref is an all-caps file in $GIT_DIR/ that always contains at least a SHA1. CHERRY_PICK_HEAD and REVERT_HEAD are examples. Because HEAD might be a symbolic ref, it is not a pseudoref. Refs backends do not manage pseudorefs. Instead, when a pseudoref (an all-caps ref containing no slashes) is requested (e.g. git rev-parse FETCH_HEAD) the generic refs code checks for the existence of that file and if it exists, returns immediately without hitting the backend. The generic code will refuse to allow updates to pseudorefs. 2. The pluggable refs backend manages all refs other than HEAD. 3. The files backend always manages HEAD. This allows for a reflog and for HEAD to be a symbolic ref. The major complication here is ref transactions -- what if there's a transaction that wants to update e.g. both HEAD and refs/heads/master? An update to the current branch (e.g. git commit) does involve at least update to the reflog of HEAD, the current branch somewhere in refs/heads/ and its log, so it is not what if but is a norm [*1*]. The updating of symlink reflogs in general, and particularly that of HEAD, is not done very cleanly. You can see the code in `commit_ref_update()` (some of it helpfully commented to be a Special hack): * If a reference is modified through a symlink, the symlink is locked rather than the reference itself. * If a reference is modified directly, and HEAD points at it, then the HEAD reflog is amended without locking HEAD. Aside from the lack of proper locking, which could result in races with other processes, we also have the problem that the same reference that is being changed via one of these implicit updates could *also* be being changed directly in the same transaction. Such an update would evade the `ref_update_reject_duplicates()` check. Previously my thinking was that the locking should be done differently: when the transaction is being processed, extra ref_update records could be created for the extra reference(s) that have to be modified, then these could be handled more straightforwardly. So supposing that HEAD points at refs/heads/master, * An update of HEAD would be turned into a reflog update and also add a synthetic update to refs/heads/master. * An update of refs/heads/master would add a synthetic update to the HEAD reflog The first point would obviously apply to any updates via symbolic refs. The second one should too, thought this is a case that we currently punt on to avoid the need to do reverse symbolic ref lookups. All of this is worth fixing, but I don't know that it needs to be fixed before ref backends hit. What do you think? It may be the case that this never happens; I have not actually audited the code to figure it out. If someone knows for sure that it does not happen, please say so. But assuming it does happen, here's my idea: If the refs backend is the files backend, we can simply treat HEAD like any other ref. If the refs backend is different, then the refs code needs to hold a files-backend transaction for HEAD, which it will commit immediately after the other transaction succeeds. We can stick a pointer to the extra transaction in the generic struct ref_transaction, which (as Michael Haggerty suggests) specific backends will extend. A failure to commit either transaction will be reported as a failure, and we'll give an additional inconsistent state warning if the main transaction succeeds but the HEAD transaction fails. Yeah, I was thinking along those lines, too. Thanks for clearly writing it down. What do other folks think? Me too ;-) I don't have an answer right now, and I have to get on an airplane in a few hours so I can't think hard about it at the moment. But let me also braindump another vague plan that I have had for a long time: overlayable reference storage schemes. Think of the way that loose refs are currently overlaid on top of packed refs. I think it might be useful to support overlaying more generally. In this particular case there could be a workspace-local reference storage that only handles HEAD and perhaps some of the other pseudoreferences. That could be overlaid onto loose reference storage (which would then only concern itself with references under refs/), which would in turn be overlaid onto packed refs. The workspace-local reference storage layer would have evil special-cased code for dealing with the references that live outside of refs/. A `ref_transaction_commit()` would be broken into phases: first each of the stacked backends would be asked to verify that the transaction is possible and acquire any necessary locks, then each backend would get the final commit command. This
Re: [PATCH v7 2/8] cherry-pick: treat CHERRY_PICK_HEAD and REVERT_HEAD as refs
On 07/10/2015 12:06 AM, Junio C Hamano wrote: David Turner dtur...@twopensource.com writes: OK, here's my current best idea: 1. A pseudoref is an all-caps file in $GIT_DIR/ that always contains at least a SHA1. CHERRY_PICK_HEAD and REVERT_HEAD are examples. Because HEAD might be a symbolic ref, it is not a pseudoref. Refs backends do not manage pseudorefs. Instead, when a pseudoref (an all-caps ref containing no slashes) is requested (e.g. git rev-parse FETCH_HEAD) the generic refs code checks for the existence of that file and if it exists, returns immediately without hitting the backend. The generic code will refuse to allow updates to pseudorefs. 2. The pluggable refs backend manages all refs other than HEAD. 3. The files backend always manages HEAD. This allows for a reflog and for HEAD to be a symbolic ref. The major complication here is ref transactions -- what if there's a transaction that wants to update e.g. both HEAD and refs/heads/master? An update to the current branch (e.g. git commit) does involve at least update to the reflog of HEAD, the current branch somewhere in refs/heads/ and its log, so it is not what if but is a norm [*1*]. The updating of symlink reflogs in general, and particularly that of HEAD, is not done very cleanly. You can see the code in `commit_ref_update()` (some of it helpfully commented to be a Special hack): * If a reference is modified through a symlink, the symlink is locked rather than the reference itself. * If a reference is modified directly, and HEAD points at it, then the HEAD reflog is amended without locking HEAD. Aside from the lack of proper locking, which could result in races with other processes, we also have the problem that the same reference that is being changed via one of these implicit updates could *also* be being changed directly in the same transaction. Such an update would evade the `ref_update_reject_duplicates()` check. Previously my thinking was that the locking should be done differently: when the transaction is being processed, extra ref_update records could be created for the extra reference(s) that have to be modified, then these could be handled more straightforwardly. So supposing that HEAD points at refs/heads/master, * An update of HEAD would be turned into a reflog update and also add a synthetic update to refs/heads/master. * An update of refs/heads/master would add a synthetic update to the HEAD reflog The first point would obviously apply to any updates via symbolic refs. The second one should too, thought this is a case that we currently punt on to avoid the need to do reverse symbolic ref lookups. It may be the case that this never happens; I have not actually audited the code to figure it out. If someone knows for sure that it does not happen, please say so. But assuming it does happen, here's my idea: If the refs backend is the files backend, we can simply treat HEAD like any other ref. If the refs backend is different, then the refs code needs to hold a files-backend transaction for HEAD, which it will commit immediately after the other transaction succeeds. We can stick a pointer to the extra transaction in the generic struct ref_transaction, which (as Michael Haggerty suggests) specific backends will extend. A failure to commit either transaction will be reported as a failure, and we'll give an additional inconsistent state warning if the main transaction succeeds but the HEAD transaction fails. Yeah, I was thinking along those lines, too. Thanks for clearly writing it down. What do other folks think? Me too ;-) I don't have an answer right now, and I have to get on an airplane in a few hours so I can't think hard about it at the moment. But let me also braindump another vague plan that I have had for a long time: overlayable reference storage schemes. Think of the way that loose refs are currently overlaid on top of packed refs. I think it might be useful to support overlaying more generally. In this particular case there could be a workspace-local reference storage that only handles HEAD and perhaps some of the other pseudoreferences. That could be overlaid onto loose reference storage (which would then only concern itself with references under refs/), which would in turn be overlaid onto packed refs. The workspace-local reference storage layer would have evil special-cased code for dealing with the references that live outside of refs/. A `ref_transaction_commit()` would be broken into phases: first each of the stacked backends would be asked to verify that the transaction is possible and acquire any necessary locks, then each backend would get the final commit command. This construct would make it easy for different backends to share the same implementation for HEAD (and potentially other workspace-local) references, by simply layering that one storage mechanism on top of their own. That would probably be overengineering if it were only used to
Re: [PATCH v7 2/8] cherry-pick: treat CHERRY_PICK_HEAD and REVERT_HEAD as refs
David Turner dtur...@twopensource.com writes: OK, here's my current best idea: 1. A pseudoref is an all-caps file in $GIT_DIR/ that always contains at least a SHA1. CHERRY_PICK_HEAD and REVERT_HEAD are examples. Because HEAD might be a symbolic ref, it is not a pseudoref. Refs backends do not manage pseudorefs. Instead, when a pseudoref (an all-caps ref containing no slashes) is requested (e.g. git rev-parse FETCH_HEAD) the generic refs code checks for the existence of that file and if it exists, returns immediately without hitting the backend. The generic code will refuse to allow updates to pseudorefs. 2. The pluggable refs backend manages all refs other than HEAD. 3. The files backend always manages HEAD. This allows for a reflog and for HEAD to be a symbolic ref. The major complication here is ref transactions -- what if there's a transaction that wants to update e.g. both HEAD and refs/heads/master? An update to the current branch (e.g. git commit) does involve at least update to the reflog of HEAD, the current branch somewhere in refs/heads/ and its log, so it is not what if but is a norm [*1*]. It may be the case that this never happens; I have not actually audited the code to figure it out. If someone knows for sure that it does not happen, please say so. But assuming it does happen, here's my idea: If the refs backend is the files backend, we can simply treat HEAD like any other ref. If the refs backend is different, then the refs code needs to hold a files-backend transaction for HEAD, which it will commit immediately after the other transaction succeeds. We can stick a pointer to the extra transaction in the generic struct ref_transaction, which (as Michael Haggerty suggests) specific backends will extend. A failure to commit either transaction will be reported as a failure, and we'll give an additional inconsistent state warning if the main transaction succeeds but the HEAD transaction fails. Yeah, I was thinking along those lines, too. Thanks for clearly writing it down. What do other folks think? Me too ;-) [Footnote] *1* But that is not a complaint; I do not have a better idea myself either. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 2/8] cherry-pick: treat CHERRY_PICK_HEAD and REVERT_HEAD as refs
On Wed, 2015-07-08 at 22:55 -0700, Junio C Hamano wrote: David Turner dtur...@twopensource.com writes: I didn't see this until after I had sent my previous message. I think the multiple working trees argument is strong enough that I will change the code (and tests). Not just code, but we probably should step back a bit and clearly define what we are trying to achieve. I _think_ what we want are: - Everything under refs/* and their associated logs would be handed off to the pluggable ref backend when one is in use. - All ref-like things with one-level ALL_CAPS names are per working tree. - They do not participate in prunable? reachability computation. - They (typically) do not want logs. Except HEAD definitely does. - Each may have extra information specific to it. - You can however read an object name off of them. One possible and straight-forward implementation to achieve ref-like things with one-level ALL_CAPS names are per working tree is to declare that they will not be handed off to the backend, but will always be implemented as files immediately under $GIT_DIR/. But note that there is no fundamental reason we have to do it that way; an alternative would be to allow backends to store these things per working tree, but then the interface to drive backends need to tell them which working tree we are working from. Unlike branches, HEAD must be per working tree; the pluggable ref backend needs to be able handle HEAD when you introduce it. I actually punted on this in my implementation, because at the time, git-new-workdir was only in contrib. But since the new worktree stuff, multiple worktrees have first-class support, so I'll have to update the code to handle it. So from that point of view, multiple working tree is *not* a valid argument why they *have* to be implemented as files under $GIT_DIR/. If you plan to let the pluggable ref backend to handle HEAD, you must have a solution for per working tree ref-like things anyway. OK, here's my current best idea: 1. A pseudoref is an all-caps file in $GIT_DIR/ that always contains at least a SHA1. CHERRY_PICK_HEAD and REVERT_HEAD are examples. Because HEAD might be a symbolic ref, it is not a pseudoref. Refs backends do not manage pseudorefs. Instead, when a pseudoref (an all-caps ref containing no slashes) is requested (e.g. git rev-parse FETCH_HEAD) the generic refs code checks for the existence of that file and if it exists, returns immediately without hitting the backend. The generic code will refuse to allow updates to pseudorefs. 2. The pluggable refs backend manages all refs other than HEAD. 3. The files backend always manages HEAD. This allows for a reflog and for HEAD to be a symbolic ref. The major complication here is ref transactions -- what if there's a transaction that wants to update e.g. both HEAD and refs/heads/master? It may be the case that this never happens; I have not actually audited the code to figure it out. If someone knows for sure that it does not happen, please say so. But assuming it does happen, here's my idea: If the refs backend is the files backend, we can simply treat HEAD like any other ref. If the refs backend is different, then the refs code needs to hold a files-backend transaction for HEAD, which it will commit immediately after the other transaction succeeds. We can stick a pointer to the extra transaction in the generic struct ref_transaction, which (as Michael Haggerty suggests) specific backends will extend. A failure to commit either transaction will be reported as a failure, and we'll give an additional inconsistent state warning if the main transaction succeeds but the HEAD transaction fails. I don't love this idea -- it seems like kind of a hack. But it's the best I can come up with. What do other folks think? As to J6t's no excessive plumbing invocations, I think the right approach is to have a single git prompt--helper command that does what the current script does to compute $r. we want to keep peeking into files under $GIT_DIR/ alone is not a valid enough reason to constrain us (I am fine if the solution we find appropriate for the 'multiple working trees' and other issues ends up being we solve it by having them as files in $GIT_DIR for other reasons, though). Agree. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 2/8] cherry-pick: treat CHERRY_PICK_HEAD and REVERT_HEAD as refs
Am 08.07.2015 um 02:55 schrieb David Turner: Instead of directly writing to and reading from files in $GIT_DIR, use ref API to interact with CHERRY_PICK_HEAD and REVERT_HEAD. Signed-off-by: David Turner dtur...@twopensource.com --- ... diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh index 366f0bc..e2c5583 100644 --- a/contrib/completion/git-prompt.sh +++ b/contrib/completion/git-prompt.sh @@ -415,9 +415,9 @@ __git_ps1 () fi elif [ -f $g/MERGE_HEAD ]; then r=|MERGING - elif [ -f $g/CHERRY_PICK_HEAD ]; then + elif git rev-parse --quiet --verify CHERRY_PICK_HEAD /dev/null; then r=|CHERRY-PICKING - elif [ -f $g/REVERT_HEAD ]; then + elif git rev-parse --quiet --verify REVERT_HEAD /dev/null; then r=|REVERTING elif [ -f $g/BISECT_LOG ]; then r=|BISECTING We are trying very hard not to spawn any new processes in __git_ps1(). So, I raise a moderate veto against this hunk. -- Hannes -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 2/8] cherry-pick: treat CHERRY_PICK_HEAD and REVERT_HEAD as refs
Am 08.07.2015 um 21:16 schrieb David Turner: On Wed, 2015-07-08 at 19:46 +0200, Johannes Sixt wrote: Am 08.07.2015 um 02:55 schrieb David Turner: Instead of directly writing to and reading from files in $GIT_DIR, use ref API to interact with CHERRY_PICK_HEAD and REVERT_HEAD. Signed-off-by: David Turner dtur...@twopensource.com --- ... diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh index 366f0bc..e2c5583 100644 --- a/contrib/completion/git-prompt.sh +++ b/contrib/completion/git-prompt.sh @@ -415,9 +415,9 @@ __git_ps1 () fi elif [ -f $g/MERGE_HEAD ]; then r=|MERGING - elif [ -f $g/CHERRY_PICK_HEAD ]; then + elif git rev-parse --quiet --verify CHERRY_PICK_HEAD /dev/null; then r=|CHERRY-PICKING - elif [ -f $g/REVERT_HEAD ]; then + elif git rev-parse --quiet --verify REVERT_HEAD /dev/null; then r=|REVERTING elif [ -f $g/BISECT_LOG ]; then r=|BISECTING We are trying very hard not to spawn any new processes in __git_ps1(). So, I raise a moderate veto against this hunk. Do you have an alternate suggestion about how to accomplish the same thing? Here are my ideas: We could special-case CHERRY_PICK_HEAD and REVERT_HEAD to be files independent of the ref backend, but that tends to complicate the backends. I think this is a mistake. We could reduce the number from two to one by providing a new git-am-status command which outputs one of CHERRY-PICKING, REVERTING, or (or maybe it would also handle rebase and am). We could also generalize it to git-prompt-helper or something by moving that entire bunch of if statements inside. This would replace calls to git describe. But you probably have a better idea. Isn't it mere coincidence that the content of these two files looks like a non-packed ref? Wouldn't it be better to consider the two akin to MERGE_HEAD (which is not a ref because it records more than just a commit name)? -- Hannes -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 2/8] cherry-pick: treat CHERRY_PICK_HEAD and REVERT_HEAD as refs
Johannes Sixt j...@kdbg.org writes: We could reduce the number from two to one by providing a new git-am-status command which outputs one of CHERRY-PICKING, REVERTING, or (or maybe it would also handle rebase and am). We could also generalize it to git-prompt-helper or something by moving that entire bunch of if statements inside. This would replace calls to git describe. But you probably have a better idea. Isn't it mere coincidence that the content of these two files looks like a non-packed ref? Wouldn't it be better to consider the two akin to MERGE_HEAD (which is not a ref because it records more than just a commit name)? That is an excellent thought that seems to have escaped everybody involved in the review. These things do not behave like refs. They do not want reflogs (and even if they had, the log would not mean much), and if we want to add more information on the cherry-pick and revert in progress, they are the most natural place to do so. Another thing that makes me vote for treating them just like FETCH_HEAD, MERGE_HEAD and other ALL_CAPS files like COMMIT_MSG is what should happen in a repository with more than one working trees. You do not want to share what CHERRY_PICK_HEAD means across them only because they happen to record an object name. Thanks for a bit of sanity. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 2/8] cherry-pick: treat CHERRY_PICK_HEAD and REVERT_HEAD as refs
On Wed, 2015-07-08 at 23:14 +0200, Johannes Sixt wrote: Am 08.07.2015 um 21:16 schrieb David Turner: On Wed, 2015-07-08 at 19:46 +0200, Johannes Sixt wrote: Am 08.07.2015 um 02:55 schrieb David Turner: Instead of directly writing to and reading from files in $GIT_DIR, use ref API to interact with CHERRY_PICK_HEAD and REVERT_HEAD. Signed-off-by: David Turner dtur...@twopensource.com --- ... diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh index 366f0bc..e2c5583 100644 --- a/contrib/completion/git-prompt.sh +++ b/contrib/completion/git-prompt.sh @@ -415,9 +415,9 @@ __git_ps1 () fi elif [ -f $g/MERGE_HEAD ]; then r=|MERGING - elif [ -f $g/CHERRY_PICK_HEAD ]; then + elif git rev-parse --quiet --verify CHERRY_PICK_HEAD /dev/null; then r=|CHERRY-PICKING - elif [ -f $g/REVERT_HEAD ]; then + elif git rev-parse --quiet --verify REVERT_HEAD /dev/null; then r=|REVERTING elif [ -f $g/BISECT_LOG ]; then r=|BISECTING We are trying very hard not to spawn any new processes in __git_ps1(). So, I raise a moderate veto against this hunk. Do you have an alternate suggestion about how to accomplish the same thing? Here are my ideas: We could special-case CHERRY_PICK_HEAD and REVERT_HEAD to be files independent of the ref backend, but that tends to complicate the backends. I think this is a mistake. We could reduce the number from two to one by providing a new git-am-status command which outputs one of CHERRY-PICKING, REVERTING, or (or maybe it would also handle rebase and am). We could also generalize it to git-prompt-helper or something by moving that entire bunch of if statements inside. This would replace calls to git describe. But you probably have a better idea. Isn't it mere coincidence that the content of these two files looks like a non-packed ref? Wouldn't it be better to consider the two akin to MERGE_HEAD (which is not a ref because it records more than just a commit name)? There appears to be a bit of code that assumes they can be read as refs, and the tests test this functionality. Indeed, I wrote most of this code by replacing the backend and fixing failing tests. At least some people seem to use the rev-ness of CHERRY_PICK_HEAD: https://mnaoumov.wordpress.com/2014/12/03/bulk-cherry-picking-process-and-magic-ref-cherry_pick_head/ So I think it's best to keep the feature. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 2/8] cherry-pick: treat CHERRY_PICK_HEAD and REVERT_HEAD as refs
On Wed, 2015-07-08 at 16:23 -0700, Junio C Hamano wrote: Johannes Sixt j...@kdbg.org writes: We could reduce the number from two to one by providing a new git-am-status command which outputs one of CHERRY-PICKING, REVERTING, or (or maybe it would also handle rebase and am). We could also generalize it to git-prompt-helper or something by moving that entire bunch of if statements inside. This would replace calls to git describe. But you probably have a better idea. Isn't it mere coincidence that the content of these two files looks like a non-packed ref? Wouldn't it be better to consider the two akin to MERGE_HEAD (which is not a ref because it records more than just a commit name)? That is an excellent thought that seems to have escaped everybody involved in the review. These things do not behave like refs. They do not want reflogs (and even if they had, the log would not mean much), and if we want to add more information on the cherry-pick and revert in progress, they are the most natural place to do so. Another thing that makes me vote for treating them just like FETCH_HEAD, MERGE_HEAD and other ALL_CAPS files like COMMIT_MSG is what should happen in a repository with more than one working trees. You do not want to share what CHERRY_PICK_HEAD means across them only because they happen to record an object name. I didn't see this until after I had sent my previous message. I think the multiple working trees argument is strong enough that I will change the code (and tests). -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 2/8] cherry-pick: treat CHERRY_PICK_HEAD and REVERT_HEAD as refs
David Turner dtur...@twopensource.com writes: I didn't see this until after I had sent my previous message. I think the multiple working trees argument is strong enough that I will change the code (and tests). Not just code, but we probably should step back a bit and clearly define what we are trying to achieve. I _think_ what we want are: - Everything under refs/* and their associated logs would be handed off to the pluggable ref backend when one is in use. - All ref-like things with one-level ALL_CAPS names are per working tree. - They do not participate in prunable? reachability computation. - They (typically) do not want logs. - Each may have extra information specific to it. - You can however read an object name off of them. One possible and straight-forward implementation to achieve ref-like things with one-level ALL_CAPS names are per working tree is to declare that they will not be handed off to the backend, but will always be implemented as files immediately under $GIT_DIR/. But note that there is no fundamental reason we have to do it that way; an alternative would be to allow backends to store these things per working tree, but then the interface to drive backends need to tell them which working tree we are working from. Unlike branches, HEAD must be per working tree; the pluggable ref backend needs to be able handle HEAD when you introduce it. So from that point of view, multiple working tree is *not* a valid argument why they *have* to be implemented as files under $GIT_DIR/. If you plan to let the pluggable ref backend to handle HEAD, you must have a solution for per working tree ref-like things anyway. As to J6t's no excessive plumbing invocations, I think the right approach is to have a single git prompt--helper command that does what the current script does to compute $r. we want to keep peeking into files under $GIT_DIR/ alone is not a valid enough reason to constrain us (I am fine if the solution we find appropriate for the 'multiple working trees' and other issues ends up being we solve it by having them as files in $GIT_DIR for other reasons, though). -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 2/8] cherry-pick: treat CHERRY_PICK_HEAD and REVERT_HEAD as refs
On Wed, 2015-07-08 at 19:46 +0200, Johannes Sixt wrote: Am 08.07.2015 um 02:55 schrieb David Turner: Instead of directly writing to and reading from files in $GIT_DIR, use ref API to interact with CHERRY_PICK_HEAD and REVERT_HEAD. Signed-off-by: David Turner dtur...@twopensource.com --- ... diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh index 366f0bc..e2c5583 100644 --- a/contrib/completion/git-prompt.sh +++ b/contrib/completion/git-prompt.sh @@ -415,9 +415,9 @@ __git_ps1 () fi elif [ -f $g/MERGE_HEAD ]; then r=|MERGING - elif [ -f $g/CHERRY_PICK_HEAD ]; then + elif git rev-parse --quiet --verify CHERRY_PICK_HEAD /dev/null; then r=|CHERRY-PICKING - elif [ -f $g/REVERT_HEAD ]; then + elif git rev-parse --quiet --verify REVERT_HEAD /dev/null; then r=|REVERTING elif [ -f $g/BISECT_LOG ]; then r=|BISECTING We are trying very hard not to spawn any new processes in __git_ps1(). So, I raise a moderate veto against this hunk. Do you have an alternate suggestion about how to accomplish the same thing? Here are my ideas: We could special-case CHERRY_PICK_HEAD and REVERT_HEAD to be files independent of the ref backend, but that tends to complicate the backends. I think this is a mistake. We could reduce the number from two to one by providing a new git-am-status command which outputs one of CHERRY-PICKING, REVERTING, or (or maybe it would also handle rebase and am). We could also generalize it to git-prompt-helper or something by moving that entire bunch of if statements inside. This would replace calls to git describe. But you probably have a better idea. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
