Junio C Hamano [off-list ref] writes:
Jeff King [off-list ref] writes:
quoted
What I was trying to get at is that naming it "status --no-lock-index"
would not be the same thing (even though with the current implementation
it would behave the same). IOW, can we improve the documentation of
"status" to point to make it easier to discover this use case.
Yeah, the name is unfortunate.
What the end user really wants to see, I suspect, is a "--read-only"
option that applies to any filesystem entity and to any command, in
the context of this thread, and also in the original discussion that
led to the introduction of that option.
While I think the variable losing "index" from its name was a vast
improvement relative to "--no-lock-index", simply because it
expresses what we do a bit closer to "do not just do things without
modifying anything my repository", it did not go far enough.
Yuck, the last sentence was garbled. What I meant as the ideal
"read-only" was "do things without modifying anything in my
repository".
And to avoid any misunderstanding, what I mean by "it did not go far
enough" is *NOT* this:
We added a narrow feature and gave it a narrow name. Instead we
should have added a "--read-only" feature, which this change may
be a small part of, and waited releasing the whole thing until
it is reasonably complete.
By going far enough, I was wondering if we should have done
something that we historically did not do. An "aspirational"
feature that is incrementally released with a known bug and that
will give users what they want in the larger picture when completed.
IOW, we could have made this "git --read-only <cmd>", that is
explained initially as "tell Git not to modify repository when it
does not have to (e.g. avoid opportunistic update)" and perhaps
later as "tell Git not to modify anything in the repository--if it
absolutely has to (e.g. "git --read-only commit" is impossible to
complete without modifying anything in the repository), error out
instead". And with a known-bug section to clearly state that this
feature is not something we vetted every codepath to ensure the
read-only operation, but is still a work in progress.
After all, "status" does not have to stay to be the only command
with opportunistic modification (in the current implementation, it
does "update-index --refresh" to update the index). And the index
does not have to stay to be the only thing that is opportunistically
modified (e.g. "git diff --cached" could not just opportunistically
update the index, but also it could be taught to write out tree
objects for intermediate directories when it does its cache-tree
refresh, which would help the diff-index algorithm quite a bit in
the performance department).
Having a large picture option like "--read-only" instead of ending
up with dozens of "we implemented a knob to tweak only this little
piece, and here is an option to trigger it" would help users in the
long run, but we traditionally did not do so because we tend to
avoid shipping "incomplete" features, but being perfectionist with
such a large undertaking can stall topics with feature bloat. In a
case like this, however, I suspect that an aspirational feature that
starts small, promises little and can be extended over time may be a
good way to move forward.
On Sun, Nov 26, 2017 at 06:35:56PM +0900, Junio C Hamano wrote:
Having a large picture option like "--read-only" instead of ending
up with dozens of "we implemented a knob to tweak only this little
piece, and here is an option to trigger it" would help users in the
long run, but we traditionally did not do so because we tend to
avoid shipping "incomplete" features, but being perfectionist with
such a large undertaking can stall topics with feature bloat. In a
case like this, however, I suspect that an aspirational feature that
starts small, promises little and can be extended over time may be a
good way to move forward.
I actually consider "--no-optional-locks" to be such an aspirational
feature. I didn't go digging for other cases (though I'm fairly certain
that "diff" has one), but hoped to leave it for further bug reports ("I
used the option, ran command X, and saw lock contention").
I would be fine with having a further aspirational "read only" mode. As
I said before, that's not quite the same thing as no-optional-locks, but
I think they're close enough that I'd be fine having only one of them.
But now that we've shipped a version with the locking one, we're stuck
with at least for the duration of a deprecation cycle.
-Peff
Hi,
Jeff King wrote:
On Sun, Nov 26, 2017 at 06:35:56PM +0900, Junio C Hamano wrote:
quoted
Having a large picture option like "--read-only" instead of ending
up with dozens of "we implemented a knob to tweak only this little
piece, and here is an option to trigger it" would help users in the
long run, but we traditionally did not do so because we tend to
avoid shipping "incomplete" features, but being perfectionist with
such a large undertaking can stall topics with feature bloat. In a
case like this, however, I suspect that an aspirational feature that
starts small, promises little and can be extended over time may be a
good way to move forward.
I actually consider "--no-optional-locks" to be such an aspirational
feature. I didn't go digging for other cases (though I'm fairly certain
that "diff" has one), but hoped to leave it for further bug reports ("I
used the option, ran command X, and saw lock contention").
I am worried that the project is not learning from what happened here.
My main issue with the --no-optional-locks name is that it does not
connect to the underlying user need. Your main argument for it is
that it exactly describes the underlying user need. One of us has to
be wrong.
So let me describe my naive reading:
As a user, I want to inspect the state of the repository without
disrupting it in any way. That means not breaking concurrent
processes and not upsetting permissions. --read-only seems to
describe this use case to me perfectly.
If I understood correctly, your objection is that --read-only is not
specific enough. What I really want, you might say, is not to break
concurrent processes. Any other aspects of being read-only are not
relevant. E.g. if I can refresh the on-disk index using O_APPEND
without disrupting concurrent processes then I should be satisfied
with that.
Fair enough, though that feels like overengineering. But I *still*
don't see what that has to do with the name "no-optional-locks". When
is a lock *optional*? And how am I supposed to discover this option?
This also came up during review, and I am worried that this review
feedback is being ignored. In other words, I have no reason to
believe it won't happen again.
I would be fine with having a further aspirational "read only" mode.
Excellent, we seem to agree on this much. If I can find time for it
today then I'll write a patch.
Thanks,
Jonathan
On Mon, Nov 27, 2017 at 12:57:31PM -0800, Jonathan Nieder wrote:
quoted
I actually consider "--no-optional-locks" to be such an aspirational
feature. I didn't go digging for other cases (though I'm fairly certain
that "diff" has one), but hoped to leave it for further bug reports ("I
used the option, ran command X, and saw lock contention").
I am worried that the project is not learning from what happened here.
My main issue with the --no-optional-locks name is that it does not
connect to the underlying user need. Your main argument for it is
that it exactly describes the underlying user need. One of us has to
be wrong.
Or there's a false dichotomy. ;) We could be talking about two different
users.
So let me describe my naive reading:
As a user, I want to inspect the state of the repository without
disrupting it in any way. That means not breaking concurrent
processes and not upsetting permissions. --read-only seems to
describe this use case to me perfectly.
That does not match the request that I got from real script writers who
were having a problem. They wanted to avoid lock contention with
background tasks. They don't care if the repository is modified as long
as it is done in a safe and non-conflicting way.
I agree (as I think I've said already in this thread) that --read-only
would be a superset of that. And that it would probably be OK to have
just gone there in the first place, sacrificing a small amount of
specificity in the name of having fewer knobs for the user to turn.
If I understood correctly, your objection is that --read-only is not
specific enough. What I really want, you might say, is not to break
concurrent processes. Any other aspects of being read-only are not
relevant. E.g. if I can refresh the on-disk index using O_APPEND
without disrupting concurrent processes then I should be satisfied
with that.
Do I have an objection? It's not clear to me that anybody is actually
proposing anything concrete for me to object to.
Are we adding "--read-only"? Are we going back to "status
--no-lock-index"? In either case, are we deprecating
"--no-optional-locks"?
It sounds like you are arguing for the first, and it sounds like Dscho
is arguing for the second. Frankly, I don't really care that much. I've
said all that I can on why I chose the direction I did, and I remain
unconvinced that we have evidence that the current option is somehow
impossible to find. If somebody wants to take us down one of the other
roads, that's fine by me.
Fair enough, though that feels like overengineering. But I *still*
don't see what that has to do with the name "no-optional-locks". When
is a lock *optional*? And how am I supposed to discover this option?
I kind of feel like any answer I give to these questions is just going
to be waved aside. But here are my earnest answers:
1. You are bit by lock contention, where running operation X ends up
with some error like "unable to create index.lock: file exists".
"X" is probably something like "commit".
2. You search the documentation for options related to locks. You're
not likely to find it in the manpage for X, since the root of the
problem actually has nothing to do with X in the first place. It's
a background task running "status" that is the problem.
3. You might find it in git(1) while searching for information on
locks, since "lock" is in the name of the option (and is in fact
the only hit in that page). The index is also mentioned there
(though searching for "index" yields a lot more hits).
4. You might find it in git-status(1) if you suspect that "status" is
at work. Searching for "index" or "lock" turns up the addition I
just proposed yesterday.
There are obviously a lot of places where that sequence might fail to
find a hit. But the same is true of just about any option, including
putting "--read-only" into git(1).
This also came up during review, and I am worried that this review
feedback is being ignored. In other words, I have no reason to
believe it won't happen again.
I'm having a hard time figuring out what you mean here. Do you mean that
I ignored feedback on this topic during the initial review?
Looking at the original thread, I just don't see it. There was some
question about the name. I tried to lay out my thinking here:
https://public-inbox.org/git/20170921050835.mrbgx2zryy3jusdk@sigill.intra.peff.net/
and ended with:
I am open to a better name, but I could not come up with one.
There was no meaningful response on the topic. When I reposted v2, I
tried to bring attention to that with:
- there was some discussion over the name. I didn't see other
suggestions, and I didn't come up with anything better.
So...am I missing something? Am I misunderstanding your point?
quoted
I would be fine with having a further aspirational "read only" mode.
Excellent, we seem to agree on this much. If I can find time for it
today then I'll write a patch.
Great.
-Peff