Re: [BUG?] git checkout $commit -- somedir doesn't drop files
Jeff King 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 Thu, Sep 19, 2013 at 11:02:17AM -0700, Junio C Hamano wrote: > Jeff King 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
Jeff King 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 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 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
Re: [BUG?] git checkout $commit -- somedir doesn't drop files
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. In my mind, oneway_merge is "go here unconditionally", twoway_merge is "go from A to B, respecting changes already made from A", and threeway_merge is a "real" merge. But I am not sure if that is accurate or just my confused mental model of unpack-trees. :) -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 writes: > On Tue, Sep 17, 2013 at 01:40:17PM -0700, Junio C Hamano wrote: > >> 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? Obviously we cannot literally do "rm -fr dir && tar x", but I agree that if tree-ish has a path that is not tracked in the current index, the path should be overwritten and made identical to what is in the tree-ish. > 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? 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? -- 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 writes: > On Tue, Sep 17, 2013 at 01:27:08PM -0700, Junio C Hamano wrote: > >> Jeff King 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:27:08PM -0700, Junio C Hamano wrote: > Jeff King 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 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 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
Uwe Kleine-König 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 ..." 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
