Re: [RFC PATCH] unpack-trees.c: handle empty deleted ita files

2019-08-19 Thread Junio C Hamano
Varun Naik  writes:

>> Either is fine as the implementation of the same semantics; I
>> however am not sure if two I-T-A entries should compare "same" or
>> "not same", or if we are better off catching the caller that feeds
>> two I-T-A entries to same() with a BUG().
>
> I'd argue that two ita cache entries should be a BUG. Since we believe
> that a cache entry in the tree can never have the intent-to-add bit set,
> it suffices to show that no call to same() ever passes two cache entries
> from the index.
> ...
> The same argument probably extends to the conflicted bit, but changing
> that is probably out of scope of this patch.

Yup.  I think the patch as-posted is fine.  I also agree that
tightening the validity check of parameters to same() is better done
as a separate topic.


Re: [RFC PATCH] unpack-trees.c: handle empty deleted ita files

2019-08-19 Thread Varun Naik
On Thu, Aug 15, 2019 at 9:34 AM Junio C Hamano  wrote:
>
> Varun Naik  writes:
>
> > On Tue, Aug 13, 2019 at 1:33 PM Junio C Hamano  wrote:
> >>
> >> So perhaps
> >>
> >> +   !ce_intent_to_add(a) && !ce_intent_to_add(b) &&
> >>
> >> i.e. "a cache entry is eligible to be same with something else only
> >> when its I-T-A bit is unset".
> >>
> >
> > I decided to follow René's suggestion in response to this. Patch coming 
> > soon.
>
> Either is fine as the implementation of the same semantics; I
> however am not sure if two I-T-A entries should compare "same" or
> "not same", or if we are better off catching the caller that feeds
> two I-T-A entries to same() with a BUG().

I'd argue that two ita cache entries should be a BUG. Since we believe
that a cache entry in the tree can never have the intent-to-add bit set,
it suffices to show that no call to same() ever passes two cache entries
from the index.

The call in unpack-trees.c:merged_entry() compares the "old" entry
(which comes from the index in all cases) to a newly created "merge"
entry (which is a duplicate of an entry from a tree in all cases). All
other calls compare either entries from two trees, or an entry from a
tree and an entry from the index. I also can't think of any case where
someone would want to check if two index entries are the "same" in the
future.

The same argument probably extends to the conflicted bit, but changing
that is probably out of scope of this patch.

Varun


Re: [RFC PATCH] unpack-trees.c: handle empty deleted ita files

2019-08-15 Thread Junio C Hamano
Varun Naik  writes:

> On Tue, Aug 13, 2019 at 1:33 PM Junio C Hamano  wrote:
>>
>> The original code considered that two entries with the same mode and
>> the same "contents" are the same.  As nobody sane tracks an empty
>> file for an extended span of history, that meant that most of the
>> time, intent-to-add entries, which has the normal mode bits for the
>> blobs (with or without the executable bit) and object name for a
>> zero length blob, would have been judged "different".
>>
>
> I agree, this edge case is really arcane. The rabbit hole was deep :)

In retrospect, perhaps I shouldn't have used the empty-blob SHA-1
for I-T-A entries (instead, perhaps 0{40} or something impossible)
when I invented this feature at 39425819 ("git-add --intent-to-add
(-N)", 2008-08-21).  It made the code to "diff" easier than having
to special case comparison between a real blob and an I-T-A entry,
but we are paying the price for that laziness with a discussion and
a patch like this one.

>> So perhaps
>>
>> +   !ce_intent_to_add(a) && !ce_intent_to_add(b) &&
>>
>> i.e. "a cache entry is eligible to be same with something else only
>> when its I-T-A bit is unset".
>>
>
> I decided to follow René's suggestion in response to this. Patch coming soon.

Either is fine as the implementation of the same semantics; I
however am not sure if two I-T-A entries should compare "same" or
"not same", or if we are better off catching the caller that feeds
two I-T-A entries to same() with a BUG().

Thanks.


Re: [RFC PATCH] unpack-trees.c: handle empty deleted ita files

2019-08-15 Thread Varun Naik
On Tue, Aug 13, 2019 at 1:33 PM Junio C Hamano  wrote:
>
> The original code considered that two entries with the same mode and
> the same "contents" are the same.  As nobody sane tracks an empty
> file for an extended span of history, that meant that most of the
> time, intent-to-add entries, which has the normal mode bits for the
> blobs (with or without the executable bit) and object name for a
> zero length blob, would have been judged "different".
>

I agree, this edge case is really arcane. The rabbit hole was deep :)

> So perhaps
>
> +   !ce_intent_to_add(a) && !ce_intent_to_add(b) &&
>
> i.e. "a cache entry is eligible to be same with something else only
> when its I-T-A bit is unset".
>

I decided to follow René's suggestion in response to this. Patch coming soon.

Varun


Re: [RFC PATCH] unpack-trees.c: handle empty deleted ita files

2019-08-13 Thread René Scharfe
Am 13.08.19 um 22:32 schrieb Junio C Hamano:
> So perhaps
>
> + !ce_intent_to_add(a) && !ce_intent_to_add(b) &&
>
> i.e. "a cache entry is eligible to be same with something else only
> when its I-T-A bit is unset".

FWIW, here's another way to express that:

diff --git a/unpack-trees.c b/unpack-trees.c
index 62276d4fef..a62d67d131 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -1658,7 +1658,7 @@ static int same(const struct cache_entry *a, const struct 
cache_entry *b)
return 0;
if (!a && !b)
return 1;
-   if ((a->ce_flags | b->ce_flags) & CE_CONFLICTED)
+   if ((a->ce_flags | b->ce_flags) & (CE_CONFLICTED | CE_INTENT_TO_ADD))
return 0;
return a->ce_mode == b->ce_mode &&
   oideq(&a->oid, &b->oid);



Re: [RFC PATCH] unpack-trees.c: handle empty deleted ita files

2019-08-13 Thread Junio C Hamano
René Scharfe  writes:

> Am 13.08.19 um 18:03 schrieb Varun Naik:
>> It is possible to delete a committed file from the index and then add it
>> as intent-to-add. Several variations of "reset" and "checkout" should
>> resurrect the file in the index from HEAD. "merge", "cherry-pick", and
>> "revert" should all fail with an error message. This patch provides the
>> desired behavior even when the file is empty in HEAD.
>>
>> The affected commands all compare two cache entries by calling
>> unpack-trees.c:same(). A cache entry for an ita file and a cache entry
>> for an empty file have the same oid. So, the cache entry for an empty
>> deleted ita file was previously considered the "same" as the cache entry
>> for an empty file. This fix adds a comparison of the intent-to-add bits
>> so that the two cache entries are no longer considered the "same".
>>
>> Signed-off-by: Varun Naik 
>> ---
>> I am marking this patch as RFC because it is changing code deep in
>> unpack-trees.c, and I'm sure it will generate some controversy :)
>
> Lacking experience with intent-to-add I don't see why this would be
> controversial.

The "same()" function here is used to compare two cache entries
(either came from the in-core index, or fabricated out of one of the
tree objects as a potential merge result to replace the one in the
index), and is expected to tell if they are the same or the
different, which is used for example in a logic like "if the one in
the index and the one came from HEAD are not the same, then it is
not safe to continue the merge", "if the one came from HEAD and the
one from the other branch are the same, just keep the one from the
index (which may or may not be the same as HEAD)".

The original code considered that two entries with the same mode and
the same "contents" are the same.  As nobody sane tracks an empty
file for an extended span of history, that meant that most of the
time, intent-to-add entries, which has the normal mode bits for the
blobs (with or without the executable bit) and object name for a
zero length blob, would have been judged "different".

This change extends the logic to the case in which the contents
recorded in the tree-ishes is also a zero length blob.  We used to
say the I-T-A entry was the same as the recorded blob, but whether
the recorded one in the tree-ish is 0-length or 1-byte blob, this
patch does not want it to be declared the "same" as any I-T-A entry.

So in that sense, it makes the behaviour for I-T-A entries
consistent.  But it is a separate matter if the consistency is good;
we do not want our code to be consistently wrong ;-)

>> diff --git a/unpack-trees.c b/unpack-trees.c
>> index 50189909b8..9b7e6b01c4 100644
>> --- a/unpack-trees.c
>> +++ b/unpack-trees.c
>> @@ -1661,6 +1661,7 @@ static int same(const struct cache_entry *a, const 
>> struct cache_entry *b)
>>  if ((a->ce_flags | b->ce_flags) & CE_CONFLICTED)
>>  return 0;
>>  return a->ce_mode == b->ce_mode &&
>> +   !ce_intent_to_add(a) == !ce_intent_to_add(b) &&
>
> Why the bangs?  This would work just as well and be slightly easier to
> read without negating both sides, wouldn't it?

Looking at the implementation of that macro, I agree.  If it were
"returns non-zero for true, zero for false, we do not guarantee that
we return the same non-zero value for true all the time", then these
bangs do make sense, but that is not the case here.

But more importantly, can both sides of the comparison be I-T-A
entries?

I offhand do not think such a situation can arise (a cache entry
with I-T-A bit on can only come from the in-core index, IIUC, and
never from a tree-ish in this codepath), but if we encouter such a
case, I would imagine that we do not want to treat an I-T-A entry to
be the same with anything else, including another I-T-A entry.

So perhaps 

+   !ce_intent_to_add(a) && !ce_intent_to_add(b) &&

i.e. "a cache entry is eligible to be same with something else only
when its I-T-A bit is unset".

I dunno.

>> oideq(&a->oid, &b->oid);
>>  }
>>
>>


Re: [RFC PATCH] unpack-trees.c: handle empty deleted ita files

2019-08-13 Thread René Scharfe
Am 13.08.19 um 18:03 schrieb Varun Naik:
> It is possible to delete a committed file from the index and then add it
> as intent-to-add. Several variations of "reset" and "checkout" should
> resurrect the file in the index from HEAD. "merge", "cherry-pick", and
> "revert" should all fail with an error message. This patch provides the
> desired behavior even when the file is empty in HEAD.
>
> The affected commands all compare two cache entries by calling
> unpack-trees.c:same(). A cache entry for an ita file and a cache entry
> for an empty file have the same oid. So, the cache entry for an empty
> deleted ita file was previously considered the "same" as the cache entry
> for an empty file. This fix adds a comparison of the intent-to-add bits
> so that the two cache entries are no longer considered the "same".
>
> Signed-off-by: Varun Naik 
> ---
> I am marking this patch as RFC because it is changing code deep in
> unpack-trees.c, and I'm sure it will generate some controversy :)

Lacking experience with intent-to-add I don't see why this would be
controversial.  Copying Duy and quoting in full, as he might have more
to say on that topic.

I have just one silly question below.

>
> The affected "reset" and "checkout" commands call
> unpack-trees.c:oneway_merge(), which calls same(). The affected "merge",
> "cherry-pick", and "revert" commands call
> unpack-trees.c:threeway_merge(), which calls same(). "stash" also calls
> oneway_merge(), and "rebase" also calls threeway_merge(), but they are
> not included in the test cases because their behaviors have not changed.
>
> The new tests are not comprehensive. In particular, they don't call
> plumbing commands, such as "read-tree". But hopefully they provide
> enough coverage to prevent most regressions.
>
> The new test cases for "cherry-pick" and "revert" grep for the single
> word "overwritten" rather than a more precise error message because the
> error message for an empty deleted ita file changes slightly if the
> patch in [0] is also applied. In retrospect, the commands affected by
> [0], [1], and this patch were all intertwined, and it would have been
> better to create a single large patch instead of three smaller patches.
>
> [0]: https://public-inbox.org/git/20190801161558.12838-1-vcnai...@gmail.com/
> [1]: https://public-inbox.org/git/20190802162852.14498-1-vcnai...@gmail.com/
>
>  t/t3030-merge-recursive.sh| 25 +++---
>  t/t3501-revert-cherry-pick.sh | 49 ++-
>  t/t7104-reset-hard.sh | 11 
>  t/t7110-reset-merge.sh| 31 ++
>  t/t7201-co.sh | 12 +
>  unpack-trees.c|  1 +
>  6 files changed, 125 insertions(+), 4 deletions(-)
>
> diff --git a/t/t3030-merge-recursive.sh b/t/t3030-merge-recursive.sh
> index ff641b348a..8aebb829a6 100755
> --- a/t/t3030-merge-recursive.sh
> +++ b/t/t3030-merge-recursive.sh
> @@ -303,13 +303,32 @@ test_expect_success 'fail if the index has unresolved 
> entries' '
>   git checkout -f "$c1" &&
>
>   test_must_fail git merge "$c5" &&
> - test_must_fail git merge "$c5" 2> out &&
> + test_must_fail git merge "$c5" 2>out &&
>   test_i18ngrep "not possible because you have unmerged files" out &&
>   git add -u &&
> - test_must_fail git merge "$c5" 2> out &&
> + test_must_fail git merge "$c5" 2>out &&
>   test_i18ngrep "You have not concluded your merge" out &&
>   rm -f .git/MERGE_HEAD &&
> - test_must_fail git merge "$c5" 2> out &&
> + test_must_fail git merge "$c5" 2>out &&
> + test_i18ngrep "Your local changes to the following files would be 
> overwritten by merge:" out
> +'
> +
> +test_expect_success 'fail if a deleted intent-to-add file exists in the 
> index' '
> + git checkout -f "$c1" &&
> + echo "nonempty" >nonempty &&
> + git add nonempty &&
> + git commit -m "create file to be deleted" &&
> + git rm --cached nonempty &&
> + git add -N nonempty &&
> + test_must_fail git merge "$c5" 2>out &&
> + test_i18ngrep "Your local changes to the following files would be 
> overwritten by merge:" out &&
> + git checkout -f "$c1" &&
> + >empty &&
> + git add empty &&
> + git commit -m "create file to be deleted" &&
> + git rm --cached empty &&
> + git add -N empty &&
> + test_must_fail git merge "$c5" 2>out &&
>   test_i18ngrep "Your local changes to the following files would be 
> overwritten by merge:" out
>  '
>
> diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh
> index d1c68af8c5..45d816fc0c 100755
> --- a/t/t3501-revert-cherry-pick.sh
> +++ b/t/t3501-revert-cherry-pick.sh
> @@ -91,16 +91,63 @@ test_expect_success 'cherry-pick on stat-dirty working 
> tree' '
>   )
>  '
>
> -test_expect_success 'revert forbidden on dirty working tree' '
> +test_expect_success 'cherry-pick forbidden on dirty working tree' '
>
> + git checkout -b temp &&
>   echo c