Re: [PATCH 1/3] Resurrect "diff-lib.c: adjust position of i-t-a entries in diff"
Duy Nguyen writes: > Off topic. This reminds me of an old patch about apply and ita [1] but > that one is not the same here ... Yeah, and re-reading that one, I think that sort-of makes sense. I am hesitant to take it out of context, though. I wonder how it would interact with that broken mode of "git apply" where it can take patches to the same path number of times... > [1] > https://public-inbox.org/git/1451181092-26054-4-git-send-email-pclo...@gmail.com/ >> ita entry in the index for symmetry, wouldn't it? That by itself >> can be seen as an improvement (we no longer would have to say that >> "git apply patchfile && git commit -a" that is run in a clean state >> will forget new files the patchfile creates), but it also means ... Another downside is that "git reset --hard" after "git apply" will suddenly start removing them. Perhaps that can be seen a feature? I dunno.
Re: [PATCH 1/3] Resurrect "diff-lib.c: adjust position of i-t-a entries in diff"
On Fri, Oct 7, 2016 at 2:15 AM, Junio C Hamano wrote: > Duy Nguyen writes: > >> On Tue, Oct 4, 2016 at 11:15 PM, Junio C Hamano wrote: >>> Duy Nguyen writes: >>> We don't use it internally _yet_. I need to go through all the external diff code and see --shift-ita should be there. The end goal is still changing the default behavior and getting rid of --shift-ita, >>> >>> I do not agree with that endgame, and quite honestly I do not want >>> to waste time reviewing such a series. > > I definitely shouldn't have said that, especially "waste". Many > issues around i-t-a and diff make my head hurt when I think about > them [*1*], but not wanting to spend time that gets my > head hurt and not wanting to waste time are totally different > things. My apologies. No problem. I do appreciate a straight shoot down though. Many of my topics have been going on for months (ones not in 'pu') and seeing it rejected near the end is worse than stopping working on them early. > I missed something curious in your statement above, i.e. "external > diff". I thought we have pretty much got rid of all the invocation > of "git diff" via the run_command() interface and we do not need the > command line option (we only need the options->shift_ita so that > callers like "git status" can seletively ask for it when making > internal calls), and that is why I didn't want to see it. I don't know if we have had any external diff calls in our shell-based commands. I don't read them often. Regardless, people do use "git diff" and it should show the "right thing" (I know it's subjective). Or at least be consistent with both git-commit and git-status. > [Footnote] > > Here is one of the things around i-t-a and diff. If you make "git > diff" (between the index and the working tree) report "new" file, it > would imply that "git apply" run without "--index" should create an Off topic. This reminds me of an old patch about apply and ita [1] but that one is not the same here > ita entry in the index for symmetry, wouldn't it? That by itself > can be seen as an improvement (we no longer would have to say that > "git apply patchfile && git commit -a" that is run in a clean state > will forget new files the patchfile creates), but it also means we > now need a repository in order to run "git apply" (without "--index"), > which is a problem, as "git apply" is often used as a better "patch". We could detect "no repo available" and ignore the index, I guess. > "git apply --cached" may also become "interesting". A patch that > would apply cleanly to HEAD should apply cleanly if you did this: > > $ git read-tree HEAD > $ git apply --cached > no matter what the working tree state is. Should a patch that > creates a "new" file add contents to the index, or just an i-t-a > entry? I could argue it both ways, but either is quite satisfactory > and makes my head hurt. --cached tells you to put new contents in the index. I-ta entries, being a reminder to add stuff, don't really fit in here because you want to add contents _now_, i think. After a successful "git apply --cached", a "git commit" should contain exactly what the applied patch has. If new files are i-t-a entries instead, then the new commit would not be the same as the patch. [1] https://public-inbox.org/git/1451181092-26054-4-git-send-email-pclo...@gmail.com/ -- Duy
Re: [PATCH 1/3] Resurrect "diff-lib.c: adjust position of i-t-a entries in diff"
Duy Nguyen writes: > On Tue, Oct 4, 2016 at 11:15 PM, Junio C Hamano wrote: >> Duy Nguyen writes: >> >>> We don't use it internally _yet_. I need to go through all the >>> external diff code and see --shift-ita should be there. The end goal >>> is still changing the default behavior and getting rid of --shift-ita, >> >> I do not agree with that endgame, and quite honestly I do not want >> to waste time reviewing such a series. I definitely shouldn't have said that, especially "waste". Many issues around i-t-a and diff make my head hurt when I think about them [*1*], but not wanting to spend time that gets my head hurt and not wanting to waste time are totally different things. My apologies. I missed something curious in your statement above, i.e. "external diff". I thought we have pretty much got rid of all the invocation of "git diff" via the run_command() interface and we do not need the command line option (we only need the options->shift_ita so that callers like "git status" can seletively ask for it when making internal calls), and that is why I didn't want to see it. > I do not believe current "diff" behavior wrt. i-t-a entries is right > either. There's no point in pursuing this series then. Feel free to > revert 3f6d56d (commit: ignore intent-to-add entries instead of > refusing - 2012-02-07) and bring back the old i-t-a behavior. All the > problems would go away. It is way too late to revert 3f6d56d. Even though I think it was overall a mistake to treat i-t-a as "not exist" while committing, people are now used to the behaviour with that change, and we need to make our progress within the constraint of the real world. [Footnote] Here is one of the things around i-t-a and diff. If you make "git diff" (between the index and the working tree) report "new" file, it would imply that "git apply" run without "--index" should create an ita entry in the index for symmetry, wouldn't it? That by itself can be seen as an improvement (we no longer would have to say that "git apply patchfile && git commit -a" that is run in a clean state will forget new files the patchfile creates), but it also means we now need a repository in order to run "git apply" (without "--index"), which is a problem, as "git apply" is often used as a better "patch". "git apply --cached" may also become "interesting". A patch that would apply cleanly to HEAD should apply cleanly if you did this: $ git read-tree HEAD $ git apply --cached
Re: [PATCH 1/3] Resurrect "diff-lib.c: adjust position of i-t-a entries in diff"
On Tue, Oct 4, 2016 at 11:15 PM, Junio C Hamano wrote: > Duy Nguyen writes: > >> We don't use it internally _yet_. I need to go through all the >> external diff code and see --shift-ita should be there. The end goal >> is still changing the default behavior and getting rid of --shift-ita, > > I do not agree with that endgame, and quite honestly I do not want > to waste time reviewing such a series. I do not believe current "diff" behavior wrt. i-t-a entries is right either. There's no point in pursuing this series then. Feel free to revert 3f6d56d (commit: ignore intent-to-add entries instead of refusing - 2012-02-07) and bring back the old i-t-a behavior. All the problems would go away. -- Duy
Re: [PATCH 1/3] Resurrect "diff-lib.c: adjust position of i-t-a entries in diff"
Duy Nguyen writes: > We don't use it internally _yet_. I need to go through all the > external diff code and see --shift-ita should be there. The end goal > is still changing the default behavior and getting rid of --shift-ita, I do not agree with that endgame, and quite honestly I do not want to waste time reviewing such a series.
Re: [PATCH 1/3] Resurrect "diff-lib.c: adjust position of i-t-a entries in diff"
On Thu, Sep 29, 2016 at 2:28 AM, Junio C Hamano wrote: > After reading the three patches through, however, I do not think we > use the command line option anywhere. I'm inclined to say that we > shouldn't add it at all. Or at least do so in a separate follow-up > patch "now we have an internal mechanism, let's expose it anyway" at > the end. Which means that the last sentence in my attempted rewrite > should go. We don't use it internally _yet_. I need to go through all the external diff code and see --shift-ita should be there. The end goal is still changing the default behavior and getting rid of --shift-ita, after making sure we don't break stuff. I do use it though because "git diff" is more often run in my workflow than "git status". > As I already said, --shift-ita is not quite descriptive and I think > it should be renamed to something else, but I kept that in the > following attempt to rewrite: It's meant to be a temporary thing (which could last a year or three, depending on how fast I scan through the code base) so I didn't give much thought on naming. Umm... after a couple of minutes, I still couldn't think of any better. The one-line summary of this change is "correct the position of intent-to-add entries in diff", or as you put it more precisely (with a bit paraphrasing), "make ita entries not exist in index". I don't see any good way to shorten that to one or two words. --ita-not-in-index good enough? Or maybe --[no-]ita-visible-in-index. -- Duy
Re: [PATCH 1/3] Resurrect "diff-lib.c: adjust position of i-t-a entries in diff"
Junio C Hamano writes: > As I already said, --shift-ita is not quite descriptive and I think > it should be renamed to something else, but I kept that in the > following attempt to rewrite: > ... Please do not use that verbatim; it was full of typo and grammo. > After reading the three patches through, however, I do not think we > use the command line option anywhere. I'm inclined to say that we > shouldn't add it at all. Or at least do so in a separate follow-up > patch "now we have an internal mechanism, let's expose it anyway" at > the end. Which means that the last sentence in my attempted rewrite > should go. > > The patch to diff-lib.c machinery looks good. > > Thanks.
Re: [PATCH 1/3] Resurrect "diff-lib.c: adjust position of i-t-a entries in diff"
Nguyễn Thái Ngọc Duy writes:
> The original commit d95d728aba06a34394d15466045cbdabdada58a2 was
> reverted in commit 78cc1a540ba127b13f2f3fd531777b57f3a9cd46 because we
> were (and still are) not ready for a new world order. A lot more
> investigation must be done to see what is impacted. See the 78cc1a5 for
> details.
>
> This patch takes a smaller and safer step. The new behavior is
> controlled by shift_ita flag. We can gradually move more diff users to
> the new behavior after we are sure it's safe to do so. This flag is
> exposed to outside temporarily as "--shift-ita" for people who prefer
> "git diff [--cached] --stat" to "git status"
Let's stop advertising this as a resurrection of something else.
The original that was unconditional was simply broken.
It is very good to refer to it (and its reversion), when justifying
why this version takes the particular approach to introduce a new
optional behaviour that can be toggled on selectively, by explaining
why doing this unconditionally was a broken idea that needed to be
reverted later.
But you would need to explain what problem this patch attempts to
solve and how before even going into that. The above two paragraphs
are backwards.
As I already said, --shift-ita is not quite descriptive and I think
it should be renamed to something else, but I kept that in the
following attempt to rewrite:
Subject: diff-lib: allow ita entries treated as "not yet exist in index"
When comparing the index and the working tree to show which
paths are new, and comparing the tree recorded in the HEAD and
the index to see if committing the contents recorded in the
index would result in an empty commit, we would want the former
comparison to say "these are new paths" and the latter to say
"there is no change" for paths that are marked as intent-to-add.
We made a similar attempt at d95d728a ("diff-lib.c: adjust
position of i-t-a entries in diff", 2015-03-16), which redefined
the semantics of these two comparison modes globally, which was
a disastor and had to be reverted at 78cc1a54 ("Revert
"diff-lib.c: adjust position of i-t-a entries in diff"",
2015-06-23). To make sure we do not repeat the same mistake,
introduce a new internal diffopt option so that this different
semantics can be asked for only by callers that ask it, while
making sure other unaudited callers will get the same comparison
result. This internal option is also exposed temporarily as
"--shift-ita" to help experiment.
After reading the three patches through, however, I do not think we
use the command line option anywhere. I'm inclined to say that we
shouldn't add it at all. Or at least do so in a separate follow-up
patch "now we have an internal mechanism, let's expose it anyway" at
the end. Which means that the last sentence in my attempted rewrite
should go.
The patch to diff-lib.c machinery looks good.
Thanks.
[PATCH 1/3] Resurrect "diff-lib.c: adjust position of i-t-a entries in diff"
The original commit d95d728aba06a34394d15466045cbdabdada58a2 was
reverted in commit 78cc1a540ba127b13f2f3fd531777b57f3a9cd46 because we
were (and still are) not ready for a new world order. A lot more
investigation must be done to see what is impacted. See the 78cc1a5 for
details.
This patch takes a smaller and safer step. The new behavior is
controlled by shift_ita flag. We can gradually move more diff users to
the new behavior after we are sure it's safe to do so. This flag is
exposed to outside temporarily as "--shift-ita" for people who prefer
"git diff [--cached] --stat" to "git status"
Signed-off-by: Nguyễn Thái Ngọc Duy
---
Documentation/diff-options.txt | 7 +++
diff-lib.c | 12
diff.c | 2 ++
diff.h | 1 +
t/t2203-add-intent.sh | 20 ++--
t/t7064-wtstatus-pv2.sh| 4 ++--
wt-status.c| 7 ++-
7 files changed, 48 insertions(+), 5 deletions(-)
diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index 7805a0c..e63285c 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -575,5 +575,12 @@ endif::git-format-patch[]
--line-prefix=::
Prepend an additional prefix to every line of output.
+--shift-ita::
+ By default entries added by "git add -N" appear as an existing
+ empty file in "git diff" and a new file in "git diff --cached".
+ This option makes the entry appear as a new file in "git diff"
+ and non-existent in "git diff --cached". Experimental option,
+ could be removed in future.
+
For more detailed explanation on these common options, see also
linkgit:gitdiffcore[7].
diff --git a/diff-lib.c b/diff-lib.c
index 3007c85..62d67c8 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -214,6 +214,11 @@ int run_diff_files(struct rev_info *revs, unsigned int
option)
!is_null_oid(&ce->oid),
ce->name, 0);
continue;
+ } else if (revs->diffopt.shift_ita &&
ce_intent_to_add(ce)) {
+ diff_addremove(&revs->diffopt, '+', ce->ce_mode,
+ EMPTY_BLOB_SHA1_BIN, 0,
+ ce->name, 0);
+ continue;
}
changed = match_stat_with_submodule(&revs->diffopt, ce,
&st,
@@ -379,6 +384,13 @@ static void do_oneway_diff(struct unpack_trees_options *o,
struct rev_info *revs = o->unpack_data;
int match_missing, cached;
+ /* i-t-a entries do not actually exist in the index */
+ if (revs->diffopt.shift_ita && idx && ce_intent_to_add(idx)) {
+ idx = NULL;
+ if (!tree)
+ return; /* nothing to diff.. */
+ }
+
/* if the entry is not checked out, don't examine work tree */
cached = o->index_only ||
(idx && ((idx->ce_flags & CE_VALID) || ce_skip_worktree(idx)));
diff --git a/diff.c b/diff.c
index c6da383..4178689 100644
--- a/diff.c
+++ b/diff.c
@@ -3923,6 +3923,8 @@ int diff_opt_parse(struct diff_options *options,
return parse_submodule_opt(options, arg);
else if (skip_prefix(arg, "--ws-error-highlight=", &arg))
return parse_ws_error_highlight(options, arg);
+ else if (!strcmp(arg, "--shift-ita"))
+ options->shift_ita = 1;
/* misc options */
else if (!strcmp(arg, "-z"))
diff --git a/diff.h b/diff.h
index ec76a90..5dd4f9c 100644
--- a/diff.h
+++ b/diff.h
@@ -146,6 +146,7 @@ struct diff_options {
int dirstat_permille;
int setup;
int abbrev;
+ int shift_ita;
/* white-space error highlighting */
#define WSEH_NEW 1
#define WSEH_CONTEXT 2
diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
index 8f22c43..c6a4648 100755
--- a/t/t2203-add-intent.sh
+++ b/t/t2203-add-intent.sh
@@ -5,10 +5,24 @@ test_description='Intent to add'
. ./test-lib.sh
test_expect_success 'intent to add' '
+ test_commit 1 &&
+ git rm 1.t &&
+ echo hello >1.t &&
echo hello >file &&
echo hello >elif &&
git add -N file &&
- git add elif
+ git add elif &&
+ git add -N 1.t
+'
+
+test_expect_success 'git status' '
+ git status --porcelain | grep -v actual >actual &&
+ cat >expect <<-\EOF &&
+ DA 1.t
+ A elif
+A file
+ EOF
+ test_cmp expect actual
'
test_expect_success 'check result of "add -N"' '
@@ -43,7 +57,9 @@ test_expect_success 'i-t-a entry is simply ignored' '
git add -N nitfol &&
git commit -m second &&
test $(git ls-tree HEAD -- nitfol | wc -l) = 0 &&
- test $(git diff --name-only HEAD -- nitfol | wc -l) = 1
+ test $(git d
