Re: [BUG?] git checkout $commit -- somedir doesn't drop files
Jeff King p...@peff.net writes: But I think that points to a larger problem, which is that we do not want to just look at the entries that are different between the tree and the index. True. The unpack-trees API knows how to walk the index and trees in parallel, and I tend to agree that it may be a more suitable vehicle for this purpose. -- 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: [BUG?] git checkout $commit -- somedir doesn't drop files
On Tue, Sep 17, 2013 at 03:14:39PM -0700, Junio C Hamano wrote: Yeah, then I agree that git checkout HEAD^ -- subdir should be a one-way go HEAD^ merge limited only to the paths that match subdir/. If implemented in a straight-forward way, I suspect that we may end up not removing subdir/b in Uwe's sample transcript. I am not sure if that is a good thing or not, though. If the index originally tracked and then going to tree does not, I think removing it would match ignore local modifications rule, as subdir/a that is tracked in the index and also in going to tree does get overwritten to match the state recorded in the tree. I had assumed the goal was to subdir/b (by the reasoning above, and the rm -rf tar analogy you gave earlier). I tried a very hacky attempt at shoving unpack-trees into the right spot in checkout_paths. But its pathspec handling from unpack_trees is not quite what we want. In Uwe's case, it did delete subdir/b and overwrite subdir/a, which I'd expect. But if there was an additional file outside of subdir, it got deleted, too. The problem is that I was giving the regular index to unpack_trees as the destination; so it wrote out only the bits of the index under the pathspec subdir/, and omitted the rest entirely. I had hoped instead it would leave those parts untouched from the source. An alternative would be to write the new entries to a temporary index in memory. And then you can throw away the entries in the current index that match the pathspec, and add in the entries from the temporary index. I was just hoping that unpack-trees would do all of that for me. :) At this point, I'm giving up for now. I was hoping a naive application of unpack-trees would just work (and reduce the code size to boot), but it is obviously a bit more complicated than that. I still think it's a good idea for checkout to behave as Uwe described, but I don't think it's that high a priority, and I have other stuff I'd prefer to work on at the moment. -Peff -- 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: [BUG?] git checkout $commit -- somedir doesn't drop files
Jeff King p...@peff.net writes: An alternative would be to write the new entries to a temporary index in memory. And then you can throw away the entries in the current index that match the pathspec, and add in the entries from the temporary index. I was just hoping that unpack-trees would do all of that for me. :) Isn't a go there one-way merge with pathspecs very similar to what happens in reset -- pathspec except for the working tree part? I suspect that it may be sufficient to mimic the read_from_tree() and adapt update_index_from_diff() callback in builtin/reset.c to also update the working tree (and we can do so unconditionally without checking if we have any local modification in this case, which simplifies things a lot), but I may be missing something. -- 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: [BUG?] git checkout $commit -- somedir doesn't drop files
On Thu, Sep 19, 2013 at 11:02:17AM -0700, Junio C Hamano wrote: Jeff King p...@peff.net writes: An alternative would be to write the new entries to a temporary index in memory. And then you can throw away the entries in the current index that match the pathspec, and add in the entries from the temporary index. I was just hoping that unpack-trees would do all of that for me. :) Isn't a go there one-way merge with pathspecs very similar to what happens in reset -- pathspec except for the working tree part? I thought so at first, but now I'm not so sure... suspect that it may be sufficient to mimic the read_from_tree() and adapt update_index_from_diff() callback in builtin/reset.c to also update the working tree (and we can do so unconditionally without checking if we have any local modification in this case, which simplifies things a lot), but I may be missing something. It almost works to simply update the index as reset does via diff_cache, marking each updated entry with CE_UPDATE, and then let the rest of checkout_paths proceed, which handles updating the working tree based on the new index. But I found two problems: 1. The diff never feeds us entries that are unchanged, so we never mark them for update. But that interferes with later code to check whether our pathspecs matched anything (so we complain on git reset --hard; git checkout HEAD foo would barf on the checkout, since we do not need to touch foo). But I think that points to a larger problem, which is that we do not want to just look at the entries that are different between the tree and the index. We also need to care about the entries that are different in the working tree and index, because those need written out, too. 2. The code in checkout_paths cannot handle the deletion for us, because it doesn't even know about the path any more (we removed it during the index diff). I think we could get around this by leaving an entry with the CE_WT_REMOVE flag set. But it looks like there is a bit more magic to removing a path than just remove_or_warn(). unpack-trees has unlink_entry, which queues up leading directories for removal. I think (2) is a matter of refactoring (but again, if we could convince unpack-trees to do this for us, that might be the nicest way to reuse the code). But (1) points to a larger problem in thinking about this as a diff; it is really about re-loading bits of the index, and then checking it out into the working tree. -Peff -- 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: [BUG?] git checkout $commit -- somedir doesn't drop files
Uwe Kleine-König u.kleine-koe...@pengutronix.de writes: after these commands: $ git init $ mkdir subdir $ echo a subdir/a $ git add subdir/a $ git commit -m a $ echo more a subdir/a $ echo b subdir/b $ git add subdir/* $ git commit -m b $ git checkout HEAD^ -- subdir I'd expect that subdir/b is removed from the index as this file didn't exist in HEAD^ but git-status only reports: # On branch master # Changes to be committed: # (use git reset HEAD file... to unstage) # # modified: subdir/a # Is this intended? (I'm using git 1.8.4.rc3 as shipped by Debian (package version 1:1.8.4~rc3-1).) The intended behaviour of git checkout tree-ish -- pathspec has always been grab paths that match pathspec from tree-ish, add them to the index and check them out to the working tree. If you have subdir/b in the index and the working tree, it will be kept when the paths that match subdir pathspec is grabbed out of the tree-ish HEAD^ (namely, subdir/a) is added to the index and to the working tree. I could argue that the above intended behaviour is suboptimal and it should have been the resulting paths in the index and the work tree that match the given pathspec must be identical to that of the tree-ish. In the resulting index or working tree, paths that match subdir pathspec in the above result is subdir/a and subdir/b, and that is different from what exists in the given tree-ish (which has only subdir/a and not subdir/b), and under that modified definition, what the current one does is not correct. I vaguely recall arguing for the updated behaviour described in the above paragraph, and I even might have started working on it, but I do not think we changed this behaviour recently, unfortunately. -- 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: [BUG?] git checkout $commit -- somedir doesn't drop files
On Tue, Sep 17, 2013 at 12:58:07PM -0700, Junio C Hamano wrote: I could argue that the above intended behaviour is suboptimal and it should have been the resulting paths in the index and the work tree that match the given pathspec must be identical to that of the tree-ish. In the resulting index or working tree, paths that match subdir pathspec in the above result is subdir/a and subdir/b, and that is different from what exists in the given tree-ish (which has only subdir/a and not subdir/b), and under that modified definition, what the current one does is not correct. Our emails just crossed, but I basically ended up saying a similar thing. Could we simply replace the update_some in builtin/checkout.c with a two-way merge via unpack-trees? I vaguely recall arguing for the updated behaviour described in the above paragraph, and I even might have started working on it, but I do not think we changed this behaviour recently, unfortunately. Yes, I did some digging and I think it has always been this way, even before git-checkout was a builtin. -Peff -- 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: [BUG?] git checkout $commit -- somedir doesn't drop files
On Tue, Sep 17, 2013 at 09:06:59PM +0200, Uwe Kleine-König wrote: $ git checkout HEAD^ -- subdir I'd expect that subdir/b is removed from the index as this file didn't exist in HEAD^ but git-status only reports: I'm not sure if this is intentional or not. The definition of git checkout $tree $path given in commit 0a1283b says: Checking paths out of a tree is (currently) defined to do: - Grab the paths from the named tree that match the given pathspec, and add them to the index; - Check out the contents from the index for paths that match the pathspec to the working tree; and while at it - If the given pathspec did not match anything, suspect a typo from the command line and error out without updating the index nor the working tree. So we are applying the pathspec to the named tree, and pulling anything that matches into the index. Which by definition cannot involve a deletion, because there is no comparison happening (so we either have it, or we do not). Whereas what you are expecting is to compare the tree and the index, limited by the pathspec, and pull any changes from the tree into the index. I tend to agree that the latter is more like how other git commands behave (e.g., if you tried to use read-tree to emulate what git-checkout was doing, I think you would end up with a two-way merge). But there may be implications I haven't thought of. Note also that git checkout -p does present subdir/b as a deletion. It works by doing a pathspec-limited diff between the two endpoints, which will notice deletions. So there is some inconsistency there with the baseline form. -Peff -- 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: [BUG?] git checkout $commit -- somedir doesn't drop files
Jeff King p...@peff.net writes: On Tue, Sep 17, 2013 at 12:58:07PM -0700, Junio C Hamano wrote: I could argue that the above intended behaviour is suboptimal and it should have been the resulting paths in the index and the work tree that match the given pathspec must be identical to that of the tree-ish. In the resulting index or working tree, paths that match subdir pathspec in the above result is subdir/a and subdir/b, and that is different from what exists in the given tree-ish (which has only subdir/a and not subdir/b), and under that modified definition, what the current one does is not correct. Our emails just crossed, but I basically ended up saying a similar thing. Could we simply replace the update_some in builtin/checkout.c with a two-way merge via unpack-trees? Would it work to resolve a conflicted index by checking out from a known tree? I vaguely recall arguing for the updated behaviour described in the above paragraph, and I even might have started working on it, but I do not think we changed this behaviour recently, unfortunately. Yes, I did some digging and I think it has always been this way, even before git-checkout was a builtin. -Peff -- 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: [BUG?] git checkout $commit -- somedir doesn't drop files
On Tue, Sep 17, 2013 at 01:27:08PM -0700, Junio C Hamano wrote: Jeff King p...@peff.net writes: On Tue, Sep 17, 2013 at 12:58:07PM -0700, Junio C Hamano wrote: I could argue that the above intended behaviour is suboptimal and it should have been the resulting paths in the index and the work tree that match the given pathspec must be identical to that of the tree-ish. In the resulting index or working tree, paths that match subdir pathspec in the above result is subdir/a and subdir/b, and that is different from what exists in the given tree-ish (which has only subdir/a and not subdir/b), and under that modified definition, what the current one does is not correct. Our emails just crossed, but I basically ended up saying a similar thing. Could we simply replace the update_some in builtin/checkout.c with a two-way merge via unpack-trees? Would it work to resolve a conflicted index by checking out from a known tree? Hrm. Probably not. It is almost a one-way merge going to the named tree (but limited by the pathspec), except that I think the current git-checkout code may provide some safety checks related to where we are coming from (e.g., do we unconditionally overwrite entries that are not uptodate?). -Peff -- 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: [BUG?] git checkout $commit -- somedir doesn't drop files
Jeff King p...@peff.net writes: On Tue, Sep 17, 2013 at 01:27:08PM -0700, Junio C Hamano wrote: Jeff King p...@peff.net writes: On Tue, Sep 17, 2013 at 12:58:07PM -0700, Junio C Hamano wrote: I could argue that the above intended behaviour is suboptimal and it should have been the resulting paths in the index and the work tree that match the given pathspec must be identical to that of the tree-ish. In the resulting index or working tree, paths that match subdir pathspec in the above result is subdir/a and subdir/b, and that is different from what exists in the given tree-ish (which has only subdir/a and not subdir/b), and under that modified definition, what the current one does is not correct. Our emails just crossed, but I basically ended up saying a similar thing. Could we simply replace the update_some in builtin/checkout.c with a two-way merge via unpack-trees? Would it work to resolve a conflicted index by checking out from a known tree? Hrm. Probably not. It is almost a one-way merge going to the named tree (but limited by the pathspec), except that I think the current git-checkout code may provide some safety checks related to where we are coming from (e.g., do we unconditionally overwrite entries that are not uptodate?). I think we do unconditionally overwrite and that has been very much on purpose. git checkout tree-ish -- file.txt has always been about replacing whatever cruft is in paths in the worktree that match pathspec, just like cat content-created-elsewhere file.txt is. Oops, you have a local change that do not match index is the last thing we want to say, because getting rid of that local change is the primary reason why checkout tree-ish -- file.txt exists. Taking the state of a subdirectory as a whole as content, the change we are discussing will make it more like rm -fr dir tar xf some-content dir to replace the directory wholesale, which I personally think is a good thing in the longer term. -- 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: [BUG?] git checkout $commit -- somedir doesn't drop files
On Tue, Sep 17, 2013 at 01:40:17PM -0700, Junio C Hamano wrote: Hrm. Probably not. It is almost a one-way merge going to the named tree (but limited by the pathspec), except that I think the current git-checkout code may provide some safety checks related to where we are coming from (e.g., do we unconditionally overwrite entries that are not uptodate?). I think we do unconditionally overwrite and that has been very much on purpose. I thought so, too, but I was thrown off by the code in checkout_paths() that warns/aborts if there are unmerged entries. But it looks like we will have thrown out those entries already during the read_tree_some call, which adds the new entries using OK_TO_REPLACE. git checkout tree-ish -- file.txt has always been about replacing whatever cruft is in paths in the worktree that match pathspec, just like cat content-created-elsewhere file.txt is. Oops, you have a local change that do not match index is the last thing we want to say, because getting rid of that local change is the primary reason why checkout tree-ish -- file.txt exists. Taking the state of a subdirectory as a whole as content, the change we are discussing will make it more like rm -fr dir tar xf some-content dir to replace the directory wholesale, which I personally think is a good thing in the longer term. Yeah, that makes sense. What about untracked files? Right now we overwrite them if the tree-ish has an entry at the same path; that is a bit more dangerous than the rest of git, but does match the ignore local modifications rule. I assume if we handled deletions, though, that we would simply leave them be. So given that, is it fair to say that a one-way go here merge, limited by pathspec, is the closest equivalent? -Peff -- 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: [BUG?] git checkout $commit -- somedir doesn't drop files
Jeff King p...@peff.net writes: On Tue, Sep 17, 2013 at 03:00:41PM -0700, Junio C Hamano wrote: So given that, is it fair to say that a one-way go here merge, limited by pathspec, is the closest equivalent? Sorry, but it is unclear to me what you mean by one-way go here merge. Do you mean oneway_merge() in unpack-trees.c? Yes, that is what I meant. Yeah, then I agree that git checkout HEAD^ -- subdir should be a one-way go HEAD^ merge limited only to the paths that match subdir/. If implemented in a straight-forward way, I suspect that we may end up not removing subdir/b in Uwe's sample transcript. I am not sure if that is a good thing or not, though. If the index originally tracked and then going to tree does not, I think removing it would match ignore local modifications rule, as subdir/a that is tracked in the index and also in going to tree does get overwritten to match the state recorded in the 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
