From: Junio C Hamano <hidden> Date: 2016-06-15 22:45:42
"Shawn O. Pearce" [off-list ref] writes:
Johannes Schindelin [off-list ref] wrote:
...
quoted
In other words, unless there is more interest in that feature, enough to
generate a well-understood design before a good implementation, I'd rather
see this patch series dropped.
Ack. I agree with every remark made by Dscho, and also want to cry "wolf".
I haven't had time to read the patch series. Its big and intrusive
and I just don't need the feature.
Well, "me neither". Although I personally think resisting changes until
it becomes absolutely necessary is a good discipline, we also need to
recognise that there is a chicken-and-egg problem. When you have a
potentially useful feature, unless people actually try using it in the
field, you won't discover the drawbacks in either the design nor the
implementation, let alone any improvements.
But I feel like if it were in fact merged I'll fall over some bug
in it sometime soon and be forced to stop and debug it.
Exactly. That is how you make progress.
Having said that, I am willing to carry it over in 'next' outside 'master'
for the 1.6.1 cycle, as three people who are most likely to be able to fix
any potential issues are not using that feature.
Heck at
the least I'll have to go back to JGit's index code and implement
the new file format.
I am sorry to dissapoint you but I am planning to use the first one in the
series, which is the one that adds extended index flag bits, for the fix
to an unrelated feature.
From: Daniel Barkalow <hidden> Date: 2016-06-15 22:45:42
On Fri, 28 Nov 2008, Junio C Hamano wrote:
"Shawn O. Pearce" [off-list ref] writes:
quoted
Johannes Schindelin [off-list ref] wrote:
...
quoted
In other words, unless there is more interest in that feature, enough to
generate a well-understood design before a good implementation, I'd rather
see this patch series dropped.
Ack. I agree with every remark made by Dscho, and also want to cry "wolf".
I haven't had time to read the patch series. Its big and intrusive
and I just don't need the feature.
Well, "me neither". Although I personally think resisting changes until
it becomes absolutely necessary is a good discipline, we also need to
recognise that there is a chicken-and-egg problem. When you have a
potentially useful feature, unless people actually try using it in the
field, you won't discover the drawbacks in either the design nor the
implementation, let alone any improvements.
I just looked over most of it (skipping the generic index extension
portion). It looks to me like it's introducing an extra concept to avoid
actually fixing maybe-bugs in the "assume unchanged" implementation
when used with files that have been changed intentionally (with the user
intending git to overlook this change). Sparse checkout is essentially a
special case of this, where the user has changed the working directory
radically (not populating it at all) and wants git to carry on as if this
was not the case (with a certain amount of porcelain code to cause this to
happen automatically).
If there's any need for this to be distinguished from "assume unchanged",
I think it should be used with, not instead of, the CE_VALID bit; and it
could probably use some bit in the stat info section, since we don't need
stat info if we know by assumption that the entry is valid.
-Daniel
*This .sig left intentionally blank*
On 11/29/08, Daniel Barkalow [off-list ref] wrote:
On Fri, 28 Nov 2008, Junio C Hamano wrote:
> "Shawn O. Pearce" [off-list ref] writes:
>
> > Johannes Schindelin [off-list ref] wrote:
> > ...
> >> In other words, unless there is more interest in that feature, enough to
> >> generate a well-understood design before a good implementation, I'd rather
> >> see this patch series dropped.
> >
> > Ack. I agree with every remark made by Dscho, and also want to cry "wolf".
> >
> > I haven't had time to read the patch series. Its big and intrusive
> > and I just don't need the feature.
>
> Well, "me neither". Although I personally think resisting changes until
> it becomes absolutely necessary is a good discipline, we also need to
> recognise that there is a chicken-and-egg problem. When you have a
> potentially useful feature, unless people actually try using it in the
> field, you won't discover the drawbacks in either the design nor the
> implementation, let alone any improvements.
I just looked over most of it (skipping the generic index extension
portion). It looks to me like it's introducing an extra concept to avoid
actually fixing maybe-bugs in the "assume unchanged" implementation
when used with files that have been changed intentionally (with the user
intending git to overlook this change). Sparse checkout is essentially a
special case of this, where the user has changed the working directory
radically (not populating it at all) and wants git to carry on as if this
was not the case (with a certain amount of porcelain code to cause this to
happen automatically).
I chose to use another bit because I did not want to change the
behaviour of CE_VALID bit: it assumes all files are present.
If there's any need for this to be distinguished from "assume unchanged",
I think it should be used with, not instead of, the CE_VALID bit; and it
could probably use some bit in the stat info section, since we don't need
stat info if we know by assumption that the entry is valid.
Interesting. I'll think more about this.
-Daniel
*This .sig left intentionally blank*
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/29/08, Nguyen Thai Ngoc Duy [off-list ref] wrote:
On 11/29/08, Daniel Barkalow [off-list ref] wrote:
> If there's any need for this to be distinguished from "assume unchanged",
> I think it should be used with, not instead of, the CE_VALID bit; and it
> could probably use some bit in the stat info section, since we don't need
> stat info if we know by assumption that the entry is valid.
Interesting. I'll think more about this.
As I said, CE_VALID implies all files are present. I could make
CE_NO_CHECKOUT to be used with CE_VALID, but I would need to check all
CE_VALID code path to make sure the behaviour remains if
CE_NO_CHECKOUT is absent. It's just more intrusive.
I have nothing against storing CE_NO_CHECKOUT in stat info except that
it seems inappropriate/hidden place to do. ce_flags is more obvious
choice. I haven't looked closely to stat info code in read-cache.c
though.
--
Duy
From: Daniel Barkalow <hidden> Date: 2016-06-15 22:45:42
On Sun, 30 Nov 2008, Nguyen Thai Ngoc Duy wrote:
On 11/29/08, Nguyen Thai Ngoc Duy [off-list ref] wrote:
quoted
On 11/29/08, Daniel Barkalow [off-list ref] wrote:
> If there's any need for this to be distinguished from "assume unchanged",
> I think it should be used with, not instead of, the CE_VALID bit; and it
> could probably use some bit in the stat info section, since we don't need
> stat info if we know by assumption that the entry is valid.
Interesting. I'll think more about this.
As I said, CE_VALID implies all files are present.
My first question is whether this actually should be true. Going back to
the message for 5f73076c1a9b4b8dc94f77eac98eb558d25e33c0, it sounds like
the CE_VALID code is designed to be safe and sort of correct even if the
files are not actually unchanged; I don't think it would be out-of-spec
for CE_VALID to (1) always produce output as if the working tree contained
what the index contains, while (2) refusing to make any changes to working
tree files that do not actually match the index. As it is now, (2) is
explicitly true, but (1) is left vague-- commands may fail entirely or
produce different output if CE_VALID is set in the index for a file that
has changes in the working tree, but not in any particular way.
Now, it might be necessary for CE_NO_CHECKOUT to differ from CE_VALID in
some ways in (2): if a file is CE_NO_CHECKOUT and absent, code which would
modify it could probably just report sucess, while CE_VALID on a file
with changes should probably report failure. On the other hand, that could
just as easily be at the porcelain layer, with the porcelain instructing
the plumbing to change the index without changing the working tree for
those files outside the sparse checkout, and the plumbing would report
errors if the porcelain did not do this.
I could make CE_NO_CHECKOUT to be used with CE_VALID, but I would need
to check all CE_VALID code path to make sure the behaviour remains if
CE_NO_CHECKOUT is absent. It's just more intrusive.
I would expect all code that has a CE_VALID path to do something actually
wrong if it took the non-CE_VALID code path on CE_NO_CHECKOUT and there
was no CE_NO_CHECKOUT code path. So I'd expect that your patch is
insufficient to the extent that CE_NO_CHECKOUT doesn't imply CE_VALID
(since there is very little in the way of CE_NO_CHECKOUT-specific
handling in your patch).
The only case I can think of where NO_CHECKOUT is more like !VALID than
VALID is with respect to whether we can report the content in the index by
looking in the filesystem instead of in the database; I don't think this
is an intentional optimization anywhere, and I think it would be a likely
source of bugs if it were (e.g., it would have to know about files which
are up-to-date with respect to stat info, but which have been "smudged" on
disk and therefore don't match byte-for-byte with the database). Actually,
it might be most accurate to treat --no-checkout as being CE_VALID with a
smudge filter of "rm". If the combination of CE_VALID and on-disk
conversion works (which is likely to be the common pattern for Windows
users, who need autocrlf and have a slow lstat(), and is therefore
maintained), surely this combination would work for CE_NO_CHECKOUT.
I have nothing against storing CE_NO_CHECKOUT in stat info except that
it seems inappropriate/hidden place to do. ce_flags is more obvious
choice. I haven't looked closely to stat info code in read-cache.c
though.
It should be pretty clean to check CE_VALID when reading an entry from
disk and remap bits from it to additional flags in memory. I wouldn't
suggest overlaying them in memory, but there's also no shortage of space
for flags in memory.
-Daniel
*This .sig left intentionally blank*
On Sun, 30 Nov 2008, Nguyen Thai Ngoc Duy wrote:
> On 11/29/08, Nguyen Thai Ngoc Duy [off-list ref] wrote:
> > On 11/29/08, Daniel Barkalow [off-list ref] wrote:
> > > If there's any need for this to be distinguished from "assume unchanged",
> > > I think it should be used with, not instead of, the CE_VALID bit; and it
> > > could probably use some bit in the stat info section, since we don't need
> > > stat info if we know by assumption that the entry is valid.
> >
> >
> > Interesting. I'll think more about this.
> >
>
> As I said, CE_VALID implies all files are present.
My first question is whether this actually should be true. Going back to
the message for 5f73076c1a9b4b8dc94f77eac98eb558d25e33c0, it sounds like
the CE_VALID code is designed to be safe and sort of correct even if the
files are not actually unchanged; I don't think it would be out-of-spec
for CE_VALID to (1) always produce output as if the working tree contained
what the index contains, while (2) refusing to make any changes to working
tree files that do not actually match the index. As it is now, (2) is
explicitly true, but (1) is left vague-- commands may fail entirely or
produce different output if CE_VALID is set in the index for a file that
has changes in the working tree, but not in any particular way.
(1) is not always true. For example diff machinary may examine
worktree files regardless CE_VALID, which is updated for
CE_NO_CHECKOUT in d9f8fca (Prevent diff machinery from examining
worktree outside sparse checkout)
Now, it might be necessary for CE_NO_CHECKOUT to differ from CE_VALID in
some ways in (2): if a file is CE_NO_CHECKOUT and absent, code which would
modify it could probably just report sucess, while CE_VALID on a file
with changes should probably report failure. On the other hand, that could
just as easily be at the porcelain layer, with the porcelain instructing
the plumbing to change the index without changing the working tree for
those files outside the sparse checkout, and the plumbing would report
errors if the porcelain did not do this.
That's right. Much of work in the last half of the series is on
porcelain layer. "git grep" fix is the only porcelain that gets fixed
in this series.
> I could make CE_NO_CHECKOUT to be used with CE_VALID, but I would need
> to check all CE_VALID code path to make sure the behaviour remains if
> CE_NO_CHECKOUT is absent. It's just more intrusive.
I would expect all code that has a CE_VALID path to do something actually
wrong if it took the non-CE_VALID code path on CE_NO_CHECKOUT and there
was no CE_NO_CHECKOUT code path. So I'd expect that your patch is
insufficient to the extent that CE_NO_CHECKOUT doesn't imply CE_VALID
(since there is very little in the way of CE_NO_CHECKOUT-specific
handling in your patch).
I read the code again. CE_NO_CHECKOUT should follow CE_VALID code path
(which was extended to CE_VALID_MASK to have both flags). That means
CE_NO_CHECKOUT is treated as same as CE_VALID. The only difference
here is CE_VALID is set/unset by "git update-index --really-refresh"
and core.ingorestat while CE_NO_CHECKOUT has its own way to set/unset.
There is not much work for CE_NO_CHECKOUT on plumbling level except
some fixes. The last half of the series, for porcelain level, you will
see more.
The only case I can think of where NO_CHECKOUT is more like !VALID than
VALID is with respect to whether we can report the content in the index by
looking in the filesystem instead of in the database; I don't think this
is an intentional optimization anywhere, and I think it would be a likely
source of bugs if it were (e.g., it would have to know about files which
are up-to-date with respect to stat info, but which have been "smudged" on
disk and therefore don't match byte-for-byte with the database).
There is worktree file reuse in diff code somewhere IIRC. Yes, this
should be checked.
Actually,
it might be most accurate to treat --no-checkout as being CE_VALID with a
smudge filter of "rm". If the combination of CE_VALID and on-disk
conversion works (which is likely to be the common pattern for Windows
users, who need autocrlf and have a slow lstat(), and is therefore
maintained), surely this combination would work for CE_NO_CHECKOUT.
Very interesting.
> I have nothing against storing CE_NO_CHECKOUT in stat info except that
> it seems inappropriate/hidden place to do. ce_flags is more obvious
> choice. I haven't looked closely to stat info code in read-cache.c
> though.
It should be pretty clean to check CE_VALID when reading an entry from
disk and remap bits from it to additional flags in memory. I wouldn't
suggest overlaying them in memory, but there's also no shortage of space
for flags in memory.
I see. Still I prefer the current approach, less headache to decide
what bit to take from stat info ;-)
--
Duy
From: Daniel Barkalow <hidden> Date: 2016-06-15 22:45:44
On Sun, 7 Dec 2008, Nguyen Thai Ngoc Duy wrote:
On 12/1/08, Daniel Barkalow [off-list ref] wrote:
quoted
On Sun, 30 Nov 2008, Nguyen Thai Ngoc Duy wrote:
> On 11/29/08, Nguyen Thai Ngoc Duy [off-list ref] wrote:
> > On 11/29/08, Daniel Barkalow [off-list ref] wrote:
> > > If there's any need for this to be distinguished from "assume unchanged",
> > > I think it should be used with, not instead of, the CE_VALID bit; and it
> > > could probably use some bit in the stat info section, since we don't need
> > > stat info if we know by assumption that the entry is valid.
> >
> >
> > Interesting. I'll think more about this.
> >
>
> As I said, CE_VALID implies all files are present.
My first question is whether this actually should be true. Going back to
the message for 5f73076c1a9b4b8dc94f77eac98eb558d25e33c0, it sounds like
the CE_VALID code is designed to be safe and sort of correct even if the
files are not actually unchanged; I don't think it would be out-of-spec
for CE_VALID to (1) always produce output as if the working tree contained
what the index contains, while (2) refusing to make any changes to working
tree files that do not actually match the index. As it is now, (2) is
explicitly true, but (1) is left vague-- commands may fail entirely or
produce different output if CE_VALID is set in the index for a file that
has changes in the working tree, but not in any particular way.
(1) is not always true. For example diff machinary may examine
worktree files regardless CE_VALID, which is updated for
CE_NO_CHECKOUT in d9f8fca (Prevent diff machinery from examining
worktree outside sparse checkout)
I know (1) is not always true in the current implementation. What I'm
getting at is to ask (a) whether our documented behavior ever violates (1)
and (b) whether enforcing (1) would be an improvement.
I suspect that enforcing (1) would be less surprising to users in the
situation where the assumption is false that worktree files match the
index when CE_VALID is set. As it is, the diff machinery does surprising
things in this situation, and I think we just say "Nasal Monkey Territory"
(that is, we tell the user, "if you don't want git to do surprising
things, don't get into this situation"). If that is the case, we should be
fine changing it to match your CE_NO_CHECKOUT behavior, and it wouldn't be
worse for CE_VALID and might be better.
quoted
Now, it might be necessary for CE_NO_CHECKOUT to differ from CE_VALID in
some ways in (2): if a file is CE_NO_CHECKOUT and absent, code which would
modify it could probably just report sucess, while CE_VALID on a file
with changes should probably report failure. On the other hand, that could
just as easily be at the porcelain layer, with the porcelain instructing
the plumbing to change the index without changing the working tree for
those files outside the sparse checkout, and the plumbing would report
errors if the porcelain did not do this.
That's right. Much of work in the last half of the series is on
porcelain layer. "git grep" fix is the only porcelain that gets fixed
in this series.
I haven't gotten to reading the last half yet, so I'll avoid taking a
specific position on how it should work, and just make unsupported
suggestions.
quoted
> I could make CE_NO_CHECKOUT to be used with CE_VALID, but I would need
> to check all CE_VALID code path to make sure the behaviour remains if
> CE_NO_CHECKOUT is absent. It's just more intrusive.
I would expect all code that has a CE_VALID path to do something actually
wrong if it took the non-CE_VALID code path on CE_NO_CHECKOUT and there
was no CE_NO_CHECKOUT code path. So I'd expect that your patch is
insufficient to the extent that CE_NO_CHECKOUT doesn't imply CE_VALID
(since there is very little in the way of CE_NO_CHECKOUT-specific
handling in your patch).
I read the code again. CE_NO_CHECKOUT should follow CE_VALID code path
(which was extended to CE_VALID_MASK to have both flags). That means
CE_NO_CHECKOUT is treated as same as CE_VALID. The only difference
here is CE_VALID is set/unset by "git update-index --really-refresh"
and core.ingorestat while CE_NO_CHECKOUT has its own way to set/unset.
I think, then, that it would be cleaner to have CE_VALID instead of
CE_VALID_MASK, and have the things that care test CE_NO_CHECKOUT or
!CE_NO_CHECKOUT. This wouldn't be all that different, except that it would
mean that distinguishing them appears as a special case, which in turn
makes it easier to question whether they should differ.
There is not much work for CE_NO_CHECKOUT on plumbling level except
some fixes. The last half of the series, for porcelain level, you will
see more.
For the porcelain level, do we need the difference to be in the index? If
the porcelain knows the sparse checkout area and can instruct the plumbing
appropriately, the information shouldn't need to be stored in the index
unless it's ever important to remember whether an entry is CE_VALID due to
having been outside the checkout when the index was written, even though
the checkout area now includes it. I don't have a good intuition as to
what ought to happen if the user manually changes what's specified for
checkout without actually updating the index and working tree.
quoted
The only case I can think of where NO_CHECKOUT is more like !VALID than
VALID is with respect to whether we can report the content in the index by
looking in the filesystem instead of in the database; I don't think this
is an intentional optimization anywhere, and I think it would be a likely
source of bugs if it were (e.g., it would have to know about files which
are up-to-date with respect to stat info, but which have been "smudged" on
disk and therefore don't match byte-for-byte with the database).
There is worktree file reuse in diff code somewhere IIRC. Yes, this
should be checked.
quoted
Actually,
it might be most accurate to treat --no-checkout as being CE_VALID with a
smudge filter of "rm". If the combination of CE_VALID and on-disk
conversion works (which is likely to be the common pattern for Windows
users, who need autocrlf and have a slow lstat(), and is therefore
maintained), surely this combination would work for CE_NO_CHECKOUT.
Very interesting.
quoted
> I have nothing against storing CE_NO_CHECKOUT in stat info except that
> it seems inappropriate/hidden place to do. ce_flags is more obvious
> choice. I haven't looked closely to stat info code in read-cache.c
> though.
It should be pretty clean to check CE_VALID when reading an entry from
disk and remap bits from it to additional flags in memory. I wouldn't
suggest overlaying them in memory, but there's also no shortage of space
for flags in memory.
I see. Still I prefer the current approach, less headache to decide
what bit to take from stat info ;-)
True. But it would be even easier if it didn't have to be marked in the
index at all.
-Daniel
*This .sig left intentionally blank*
> There is not much work for CE_NO_CHECKOUT on plumbling level except
> some fixes. The last half of the series, for porcelain level, you will
> see more.
For the porcelain level, do we need the difference to be in the index? If
the porcelain knows the sparse checkout area and can instruct the plumbing
appropriately, the information shouldn't need to be stored in the index
This was discussed since the beginning of this feature. I recall that
the index reflects worktree, and because we mark CE_NO_CHECKOUT on
file basis, it's best to save the information there, not separately.
We do save high level information to form the checkout area (sparse
patterns) in the last half, but basically you should be able to live
without that.
unless it's ever important to remember whether an entry is CE_VALID due to
having been outside the checkout when the index was written, even though
the checkout area now includes it. I don't have a good intuition as to
what ought to happen if the user manually changes what's specified for
checkout without actually updating the index and working tree.
So if a user changes worktree without updating index, they will have
the same results as they do now: files are shown as modified if they
don't have CE_NO_CHECKOUT set. If those files do, they are considered
'orphaned' or staled and are recommended to be removed/updated to
avoid unexpected consequences (not availble this this first half
series because that belongs to "git status").
--
Duy
From: Daniel Barkalow <hidden> Date: 2016-06-15 22:45:44
On Sun, 7 Dec 2008, Nguyen Thai Ngoc Duy wrote:
On 12/7/08, Daniel Barkalow [off-list ref] wrote:
quoted
> There is not much work for CE_NO_CHECKOUT on plumbling level except
> some fixes. The last half of the series, for porcelain level, you will
> see more.
For the porcelain level, do we need the difference to be in the index? If
the porcelain knows the sparse checkout area and can instruct the plumbing
appropriately, the information shouldn't need to be stored in the index
This was discussed since the beginning of this feature. I recall that
the index reflects worktree, and because we mark CE_NO_CHECKOUT on
file basis, it's best to save the information there, not separately.
We do save high level information to form the checkout area (sparse
patterns) in the last half, but basically you should be able to live
without that.
We need to mark in the index the information that reflects the worktree.
If, however, we take CE_VALID to be the flag for "ignore the worktree
entirely at this path; act as it if contains what the index contains" (and
use this to cause that aspect of no-checkout), and we then entirely ignore
the worktree, including not caring whether there are files there or not
(except, of course, that in the transition from caring to not caring for
no-checkout, we make the worktree empty, while in the case for
"stat-is-expensive", we bring it into agreement with the index), then
there is no additional information that needs to be conveyed in the index.
quoted
unless it's ever important to remember whether an entry is CE_VALID due to
having been outside the checkout when the index was written, even though
the checkout area now includes it. I don't have a good intuition as to
what ought to happen if the user manually changes what's specified for
checkout without actually updating the index and working tree.
So if a user changes worktree without updating index, they will have
the same results as they do now: files are shown as modified if they
don't have CE_NO_CHECKOUT set. If those files do, they are considered
'orphaned' or staled and are recommended to be removed/updated to
avoid unexpected consequences (not availble this this first half
series because that belongs to "git status").
I was actually thinking that there would be a file for "this is what the
user wants to have checked out" (as opposed to the index, which must
contain "this is what is checked out"), and the porcelain would instruct
the plumbing as to what to do with the worktree (that the plumbing with
then ignore, due to the index bit) based on this information.
The index obviously can't contain the user's full instructions for what
should be checked out, because the user will want to say "I don't care
about anything in Documentation/" and have this apply to
Documentation/some-file-not-in-the-index, so that if this file is in the
worktree, the user gets a warning.
I think you're doing this with core.defaultsparse, although you seem to
allow the index to diverge from this easily.
The question, then, is what happens when the index and core.defaultsparse
disagree, either because the porcelain supports causing it or because the
user has simply editting the config file or used plumbing to modify the
index. That is, (1) we have index entries that say that the worktree is
ignored, and the rules don't say they're outside the sparse checkout; do
we care whether we expect the worktree to be empty or match the index?
And, (2) we have index entries that say we do care about them, but the
rules say they're outside the sparse checkout; what happens with these?
Case (1) is where we would need to know, in the index, whether we expect
the worktree to actually match the index (traditional CE_VALID) or whether
we expect the worktree to be empty (CE_NO_CHECKOUT), if our behavior
should actually differ. My vague feeling is that we don't want it to
differ, and these paths are unexpectional "interesting to the user, but
the worktree is ignored" until reading a tree into the index again. (But
note that we will have to check on the worktree when reading into the
index if this changes the index from "blobA, CE_VALID" to
"blobA, !CE_VALID", since the worktree could differ in a way that we don't
want to retain. And I think we want it to be an error to have the worktree
be something other than blobA or nothing before, but "nothing" is fine and
we just write it out. (This means that users of CE_VALID who remove files
behind git's back may lose their removal work; but this is a pretty
trivial danger).
-Daniel
*This .sig left intentionally blank*
> This was discussed since the beginning of this feature. I recall that
> the index reflects worktree, and because we mark CE_NO_CHECKOUT on
> file basis, it's best to save the information there, not separately.
> We do save high level information to form the checkout area (sparse
> patterns) in the last half, but basically you should be able to live
> without that.
We need to mark in the index the information that reflects the worktree.
If, however, we take CE_VALID to be the flag for "ignore the worktree
entirely at this path; act as it if contains what the index contains" (and
use this to cause that aspect of no-checkout), and we then entirely ignore
the worktree, including not caring whether there are files there or not
(except, of course, that in the transition from caring to not caring for
no-checkout, we make the worktree empty, while in the case for
"stat-is-expensive", we bring it into agreement with the index), then
there is no additional information that needs to be conveyed in the index.
That's not enough. CE_VALID is "ignore the worktree files" while
CE_NO_CHECKOUT is stricter: "those files does (or should) not exist".
The difference is
- for "git grep", we ignore path with CE_NO_CHECKOUT (while using
cache version for CE_VALID)
- porcelain-level support to widen/narrow checkout area will need
CE_NO_CHECKOUT, not CE_VALID
> > unless it's ever important to remember whether an entry is CE_VALID due to
> > having been outside the checkout when the index was written, even though
> > the checkout area now includes it. I don't have a good intuition as to
> > what ought to happen if the user manually changes what's specified for
> > checkout without actually updating the index and working tree.
>
> So if a user changes worktree without updating index, they will have
> the same results as they do now: files are shown as modified if they
> don't have CE_NO_CHECKOUT set. If those files do, they are considered
> 'orphaned' or staled and are recommended to be removed/updated to
> avoid unexpected consequences (not availble this this first half
> series because that belongs to "git status").
I was actually thinking that there would be a file for "this is what the
user wants to have checked out" (as opposed to the index, which must
contain "this is what is checked out"), and the porcelain would instruct
the plumbing as to what to do with the worktree (that the plumbing with
then ignore, due to the index bit) based on this information.
The index obviously can't contain the user's full instructions for what
should be checked out, because the user will want to say "I don't care
about anything in Documentation/" and have this apply to
Documentation/some-file-not-in-the-index, so that if this file is in the
worktree, the user gets a warning.
I think you're doing this with core.defaultsparse, although you seem to
allow the index to diverge from this easily.
Yes they can.
The question, then, is what happens when the index and core.defaultsparse
disagree, either because the porcelain supports causing it or because the
user has simply editting the config file or used plumbing to modify the
index. That is, (1) we have index entries that say that the worktree is
ignored, and the rules don't say they're outside the sparse checkout; do
we care whether we expect the worktree to be empty or match the index?
And, (2) we have index entries that say we do care about them, but the
rules say they're outside the sparse checkout; what happens with these?
The rule is CE_NO_CHECKOUT is king. core.defaultsparse only helps
setting CE_NO_CHECKOUT on new entries when they enter the index.
So if core.defaultsparse does not match what is in index, that's fine.
You may get more files with "git merge", "git checkout".. but
already-removed files should stay removed.
If you are not happy with core.defaultsparse, you can replace it with
your own tools by manipulating directly CE_NO_CHECKOUT using "git
update-index" and "git ls-files". BTW the implementation has not had
an easy way to replace core.defaultsparse. You have to modify
apply_narrow_spec(). But if someone really needs it, a hook can be
added.
--
Duy
From: Daniel Barkalow <hidden> Date: 2016-06-15 22:45:44
On Mon, 8 Dec 2008, Nguyen Thai Ngoc Duy wrote:
On 12/8/08, Daniel Barkalow [off-list ref] wrote:
quoted
> This was discussed since the beginning of this feature. I recall that
> the index reflects worktree, and because we mark CE_NO_CHECKOUT on
> file basis, it's best to save the information there, not separately.
> We do save high level information to form the checkout area (sparse
> patterns) in the last half, but basically you should be able to live
> without that.
We need to mark in the index the information that reflects the worktree.
If, however, we take CE_VALID to be the flag for "ignore the worktree
entirely at this path; act as it if contains what the index contains" (and
use this to cause that aspect of no-checkout), and we then entirely ignore
the worktree, including not caring whether there are files there or not
(except, of course, that in the transition from caring to not caring for
no-checkout, we make the worktree empty, while in the case for
"stat-is-expensive", we bring it into agreement with the index), then
there is no additional information that needs to be conveyed in the index.
That's not enough. CE_VALID is "ignore the worktree files" while
CE_NO_CHECKOUT is stricter: "those files does (or should) not exist".
The difference is
- for "git grep", we ignore path with CE_NO_CHECKOUT (while using
cache version for CE_VALID)
Is this sufficient? I'd expect "git grep" to ignore paths that are outside
the checked-out region, even when searching an arbitrary tree, and even
when those files aren't in the index at all (i.e., the current commit
doesn't have them). That is, I'd expect core.defaultsparse or the
equivalent to limit the paths, normally giving this effect.
- porcelain-level support to widen/narrow checkout area will need
CE_NO_CHECKOUT, not CE_VALID
This isn't a meaningful difference between CE_NO_CHECKOUT and CE_VALID if
there aren't any other differences.
quoted
> > unless it's ever important to remember whether an entry is CE_VALID due to
> > having been outside the checkout when the index was written, even though
> > the checkout area now includes it. I don't have a good intuition as to
> > what ought to happen if the user manually changes what's specified for
> > checkout without actually updating the index and working tree.
>
> So if a user changes worktree without updating index, they will have
> the same results as they do now: files are shown as modified if they
> don't have CE_NO_CHECKOUT set. If those files do, they are considered
> 'orphaned' or staled and are recommended to be removed/updated to
> avoid unexpected consequences (not availble this this first half
> series because that belongs to "git status").
I was actually thinking that there would be a file for "this is what the
user wants to have checked out" (as opposed to the index, which must
contain "this is what is checked out"), and the porcelain would instruct
the plumbing as to what to do with the worktree (that the plumbing with
then ignore, due to the index bit) based on this information.
The index obviously can't contain the user's full instructions for what
should be checked out, because the user will want to say "I don't care
about anything in Documentation/" and have this apply to
Documentation/some-file-not-in-the-index, so that if this file is in the
worktree, the user gets a warning.
I think you're doing this with core.defaultsparse, although you seem to
allow the index to diverge from this easily.
Yes they can.
quoted
The question, then, is what happens when the index and core.defaultsparse
disagree, either because the porcelain supports causing it or because the
user has simply editting the config file or used plumbing to modify the
index. That is, (1) we have index entries that say that the worktree is
ignored, and the rules don't say they're outside the sparse checkout; do
we care whether we expect the worktree to be empty or match the index?
And, (2) we have index entries that say we do care about them, but the
rules say they're outside the sparse checkout; what happens with these?
The rule is CE_NO_CHECKOUT is king. core.defaultsparse only helps
setting CE_NO_CHECKOUT on new entries when they enter the index.
This seems like a really bad idea to me. If you ask for a file that's
outside your default area to be checked out, and then you switch branches
and switch back, the file may or may not disappear (depending on whether
the branch you switched to temporarily had it or not). Likewise, if you
remove files, and then switch branches and back, the files may or may not
reappear.
Of course, commands need to look at the index to determine what we've
actually done with the worktree and index, but I think there should be
some other location that is responsible for keeping track of what the user
has asked for.
-Daniel
*This .sig left intentionally blank*
> - for "git grep", we ignore path with CE_NO_CHECKOUT (while using
> cache version for CE_VALID)
Is this sufficient? I'd expect "git grep" to ignore paths that are outside
the checked-out region, even when searching an arbitrary tree, and even
when those files aren't in the index at all (i.e., the current commit
doesn't have them). That is, I'd expect core.defaultsparse or the
equivalent to limit the paths, normally giving this effect.
That's the point. CE_VALID does not define checkout area while
CE_NO_CHECKOUT does. If an entry is CE_VALID, it is still in checkout
area. But if it is CE_NO_CHECKOUT, "git grep" should ignore that path.
core.defaultsparse has nothing to do here.
> > The question, then, is what happens when the index and core.defaultsparse
> > disagree, either because the porcelain supports causing it or because the
> > user has simply editting the config file or used plumbing to modify the
> > index. That is, (1) we have index entries that say that the worktree is
> > ignored, and the rules don't say they're outside the sparse checkout; do
> > we care whether we expect the worktree to be empty or match the index?
> > And, (2) we have index entries that say we do care about them, but the
> > rules say they're outside the sparse checkout; what happens with these?
>
> The rule is CE_NO_CHECKOUT is king. core.defaultsparse only helps
> setting CE_NO_CHECKOUT on new entries when they enter the index.
This seems like a really bad idea to me. If you ask for a file that's
outside your default area to be checked out, and then you switch branches
and switch back, the file may or may not disappear (depending on whether
the branch you switched to temporarily had it or not). Likewise, if you
remove files, and then switch branches and back, the files may or may not
reappear.
Well, if you set core.defaultsparse properly, those files should
appear/disappear as you wish (and as of now if you define your
checkout area with "git checkout --{include-,exclude-,}sparse" then
core.defaultsparse should be updated accordingly). I don't say
core.defaultsparse is perfect.
Anyway how do you suppose the tool to do in your case (checkout,
switch away then switch back)?
--
Duy
From: Daniel Barkalow <hidden> Date: 2016-06-15 22:45:45
On Thu, 11 Dec 2008, Nguyen Thai Ngoc Duy wrote:
On 12/9/08, Daniel Barkalow [off-list ref] wrote:
quoted
> - for "git grep", we ignore path with CE_NO_CHECKOUT (while using
> cache version for CE_VALID)
Is this sufficient? I'd expect "git grep" to ignore paths that are outside
the checked-out region, even when searching an arbitrary tree, and even
when those files aren't in the index at all (i.e., the current commit
doesn't have them). That is, I'd expect core.defaultsparse or the
equivalent to limit the paths, normally giving this effect.
That's the point. CE_VALID does not define checkout area while
CE_NO_CHECKOUT does. If an entry is CE_VALID, it is still in checkout
area. But if it is CE_NO_CHECKOUT, "git grep" should ignore that path.
core.defaultsparse has nothing to do here.
My point is that the index cannot tell git grep whether it should search a
path if the path isn't in the index. If I do a narrow checkout of only
Documentation/, and I do "git grep foo", I won't see files that aren't in
Documentation/; if I do "git grep foo origin/next", I think I shouldn't
see files that aren't in Documentation/, and "new-program.c" isn't in my
index at all, marked as CE_NO_CHECKOUT or otherwise, so git grep can't
find out from the index whether that file is outside my area of interest.
It needs to be able to determine that "only Documentation/ is in the
checkout area" ignoring the details of the list of files in the working
directory currently in or out of the area.
quoted
> > The question, then, is what happens when the index and core.defaultsparse
> > disagree, either because the porcelain supports causing it or because the
> > user has simply editting the config file or used plumbing to modify the
> > index. That is, (1) we have index entries that say that the worktree is
> > ignored, and the rules don't say they're outside the sparse checkout; do
> > we care whether we expect the worktree to be empty or match the index?
> > And, (2) we have index entries that say we do care about them, but the
> > rules say they're outside the sparse checkout; what happens with these?
>
> The rule is CE_NO_CHECKOUT is king. core.defaultsparse only helps
> setting CE_NO_CHECKOUT on new entries when they enter the index.
This seems like a really bad idea to me. If you ask for a file that's
outside your default area to be checked out, and then you switch branches
and switch back, the file may or may not disappear (depending on whether
the branch you switched to temporarily had it or not). Likewise, if you
remove files, and then switch branches and back, the files may or may not
reappear.
Well, if you set core.defaultsparse properly, those files should
appear/disappear as you wish (and as of now if you define your
checkout area with "git checkout --{include-,exclude-,}sparse" then
core.defaultsparse should be updated accordingly). I don't say
core.defaultsparse is perfect.
Right, so in order to get reasonable behavior, the user must use
--{include,exclude}-sparse. I think that this should be the *default*
behavior, and probably the *only porcelain-supported* behavior, because
otherwise it's confusing.
-Daniel
*This .sig left intentionally blank*
On 12/12/08, Daniel Barkalow [off-list ref] wrote:
> Well, if you set core.defaultsparse properly, those files should
> appear/disappear as you wish (and as of now if you define your
> checkout area with "git checkout --{include-,exclude-,}sparse" then
> core.defaultsparse should be updated accordingly). I don't say
> core.defaultsparse is perfect.
Right, so in order to get reasonable behavior, the user must use
--{include,exclude}-sparse. I think that this should be the *default*
behavior, and probably the *only porcelain-supported* behavior, because
otherwise it's confusing.
It's pretty hard (or intrusive) to enforce such behaviour. How about
showing files that does not match core.defaultsparse in "git status"
along with instructions how to add them to core.defaultsparse? That
way people can keep it consistent and less modification to current
code.
--
Duy