From: Junio C Hamano <hidden> Date: 2016-06-15 22:52:28
Jeff King [off-list ref] writes:
On Fri, Nov 11, 2011 at 08:43:34AM -0800, Junio C Hamano wrote:
I agree the naming is awkward (but then, I think the "needs update"
message is awkward ;) ). But my interpretation was: if you want the
index to be in sync with the working tree, you must do this. Hence:
$EDITOR file ;# triggers needs_update
git update-index file ;# and do update in index
rm file ;# triggers needs rm
git rm --cached file ;# and do rm in index
The word "update" at the plumbing level simply means "update the path in
the index". At the Porcelain level you have to use "git add" or "git rm"
depending on different kind of "needs update", which may be argued an
inconsistency, but at the plumbing level everything is done with the same
update-index command, and "needs update" is the word that matches your
interpretation.
I however think the topic was about updating the Porclain-ish output that
is similar to "status -s" output. And at that level, paths marked with M/T
and D are dealt differently by end users ("add" vs "rm").
And the ? : expressions the patch is touching is about mapping the state
of the path to your "you must do this" interpretation. What is being
mapped is the state, so it would be more natural to have the variable
"modified_fmt" hold the result of the mapping "you must do...", instead of
calling the variable in terms of "you must do this if you were working at
the plumbing level", which is where the original needs_update_fmt naming
comes from.
quoted
An obvious solution would be to rename all of them to be based on "what
happened to the path". E.g. "modified_fmt" would be set to either "M" or
"needs update", and "removed_fmt" would be set to either "D" or "needs
update", etc.
I'm happy to make that change. What do you think of the feature overall?
The "feature" is more or less "Meh" to me; I do not mind differentiating M
and D there because the necessary information is already there in the
codepath, but if we were to really do that, then the variables must be
renamed. We shouldn't name them after "you must do this at the plumbing
level" like we have done so far, and instead use "the path is in this
state". This is even more so if we were to further split a single "state"
that plumbing layer considers the same and gives the same "needs X" to
different states that Porcelains would give different messages in the
future.
And should typechanges also be handled here (it's no extra work for git
to detect them; we just have to pass the TYPE_CHANGED flag back up the
stack)?
As long as "pass ... back up the stack" does not result in too much
ugliness in the code, I am OK with it.
From: Jeff King <hidden> Date: 2016-06-15 22:52:28
On Fri, Nov 11, 2011 at 04:11:29PM -0800, Junio C Hamano wrote:
quoted
I'm happy to make that change. What do you think of the feature overall?
The "feature" is more or less "Meh" to me; I do not mind differentiating M
and D there because the necessary information is already there in the
codepath, but if we were to really do that, then the variables must be
renamed. We shouldn't name them after "you must do this at the plumbing
level" like we have done so far, and instead use "the path is in this
state". This is even more so if we were to further split a single "state"
that plumbing layer considers the same and gives the same "needs X" to
different states that Porcelains would give different messages in the
future.
I admit I don't care all that much either, but it's easy to do, and I
think it is less surprising to users. Patches are below.
quoted
And should typechanges also be handled here (it's no extra work for git
to detect them; we just have to pass the TYPE_CHANGED flag back up the
stack)?
As long as "pass ... back up the stack" does not result in too much
ugliness in the code, I am OK with it.
It ended up pretty simple, but I split it out from the deletion case so
you can see it more clearly (and can drop the latter half of the series
if we don't want it).
[1/4]: refresh_index: rename format variables
[2/4]: refresh_index: mark deletions in porcelain output
[3/4]: read-cache: let refresh_cache_ent pass up changed flags
[4/4]: refresh_index: notice typechanges in output
(If I were sure we wanted the typechange output, I think a more
reasonable ordering would be 1, then 3, then squash 2 and 4 into a
single patch. But see my comment in 4/4).
-Peff
From: Jeff King <hidden> Date: 2016-06-15 22:52:28
When refreshing the index, for modified (or unmerged) files
we will print "needs update" (or "needs merge") for plumbing,
or a "diff --name-status"-ish line for porcelain.
The variables holding which type of message to show are
named after the plumbing messages. However, as we begin to
differentiate more cases at the porcelain level (with the
plumbing message stayin the same), that naming scheme will
become awkward.
Instead, name the variables after which case we found
(modified or unmerged), not what we will output.
Signed-off-by: Jeff King <redacted>
---
read-cache.c | 12 ++++++------
1 files changed, 6 insertions(+), 6 deletions(-)
From: Jeff King <hidden> Date: 2016-06-15 22:52:28
If you have a deleted file and a porcelain refreshes the
cache, we print:
Unstaged changes after reset:
M file
This is technically correct, in that the file is modified,
but it's friendlier to the user if we further differentiate
the case of a deleted file (especially because this output
looks a lot like "diff --name-status", which would also make
the distinction).
The plumbing version stays as "needs update" for historical
compatibility.
Signed-off-by: Jeff King <redacted>
---
read-cache.c | 6 +++++-
1 files changed, 5 insertions(+), 1 deletions(-)
From: Jeff King <hidden> Date: 2016-06-15 22:52:28
This will enable refresh_cache to differentiate more cases
of modification (such as typechange) when telling the user
what isn't fresh.
Signed-off-by: Jeff King <redacted>
---
There's only a single layer of call-stack to modify, and there's only
one other caller, so it turned out to be a pretty minor change.
read-cache.c | 9 ++++++---
1 files changed, 6 insertions(+), 3 deletions(-)
From: Jeff King <hidden> Date: 2016-06-15 22:52:28
If a file changes type and a porcelain updates the index, we
will print "M file". Instead, let's be more specific and
print "T file", which matches actual diff and status output.
The plumbing version remains "needs update" for historical
compatibility.
Signed-off-by: Jeff King <redacted>
---
The "changed" flag comes from refresh_cache_ent, which in turn gets it
from ie_modified_stat. The one hesitation I have is that intent-to-add
entries get the TYPE_CHANGED flag set, which means they will get a "T"
output. Whereas I actually think "M" is a little more sensible.
For "git reset", I'm not sure if it matters, since resetting the index
will always drop such an entry anyway. But you can see it with:
git add -N file
git add --refresh -v other
read-cache.c | 6 +++++-
1 files changed, 5 insertions(+), 1 deletions(-)