[v4,0/2] Fix difftool problem with intent-to-add files

Message

Linus Arver via GitGitGadget June 25, 2020, 5:53 p.m. UTC

This problem was reported in 
https://github.com/git-for-windows/git/issues/2677, but the problem actually
lies with git diff --raw, and it seems that the bug has been with us ever
since --intent-to-add was introduced.

Changes since v3:

 * Instead of showing the same OID in git diff-files --raw as in git
   diff-files -p for intent-to-add files, we now imitate the logic for
   modified (non-intent-to-add) files: we show the "null" OID (i.e.
   all-zero).
   
   
 * As a consequence, we no longer just undo the inadvertent change where
   intent-to-add files were reported with the empty tree instead of the
   empty blob, but we fix the bug that has been with us since 
   run_diff_files() was taught about intent-to-add files, i.e. we fix the
   original bug of 425a28e0a4e (diff-lib: allow ita entries treated as "not
   yet exist in index", 2016-10-24), at long last.
   
   

Changes since v2:

 * Now we also drop the no-longer-used definition of hash_t in t2203.

Changes since v1:

 * Rebased onto sk/diff-files-show-i-t-a-as-new.
 * Verified that sk/diff-files-show-i-t-a-as-new does not completely resolve
   the issue (the --raw output still claims the empty blob as the
   post-image, although the difftool symptom "went away").
 * Amended the central patch of this PR to include a fix for the regression
   test that was introduced in sk/diff-files-show-i-t-a-as-new: it expected
   the raw diff to contain the hash of the empty tree object (which is
   incorrect no matter how you turn it: any hash in any raw diff should
   refer to blob objects).
 * Reordered the patches so that the central patch comes first (otherwise,
   the "empty tree" fix would cause a test failure in t2203).

Johannes Schindelin (2):
  diff-files --raw: show correct post-image of intent-to-add files
  difftool -d: ensure that intent-to-add files are handled correctly

 diff-lib.c            | 3 +--
 t/t2203-add-intent.sh | 5 ++---
 t/t7800-difftool.sh   | 8 ++++++++
 3 files changed, 11 insertions(+), 5 deletions(-)


base-commit: feea6946a5b746ff4ebf8ccdf959e303203a6011
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-654%2Fdscho%2Fdifftool-ita-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-654/dscho/difftool-ita-v4
Pull-Request: https://github.com/gitgitgadget/git/pull/654

Range-diff vs v3:

 1:  8c27c78831 ! 1:  69256ab910 diff-files --raw: handle intent-to-add files correctly
     @@ Metadata
      Author: Johannes Schindelin <[e-mail]>
      
       ## Commit message ##
     -    diff-files --raw: handle intent-to-add files correctly
     +    diff-files --raw: show correct post-image of intent-to-add files
      
     -    In `run_diff_files()`, files that have been staged with the intention to
     -    add are queued without a valid OID in the `diff_filepair`.
     +    The documented behavior of `git diff-files --raw` is to display
      
     -    When the output mode is, say, `DIFF_FORMAT_PATCH`, the
     -    `diff_fill_oid_info()` function, called from `run_diff()`, will remedy
     -    that situation by reading the file contents from disk.
     +            [...] 0{40} if creation, unmerged or "look at work tree".
      
     -    However, when the output mode is `DIFF_FORMAT_RAW`, that does not hold
     -    true, and the output will contain a bogus OID (and the flag `M` for
     -    "modified" instead of the correct `A` for "added").
     +    This happens for example when showing modified, unstaged files.
      
     -    As a consequence, `git difftool -d` (which relies on `git diff-files
     -    --raw`'s output) does not work correctly.
     +    For intent-to-add files, we used to show the empty blob's hash instead.
     +    In c26022ea8f5 (diff: convert diff_addremove to struct object_id,
     +    2017-05-30), we made that worse by inadvertently changing that to the
     +    hash of the empty tree.
      
     -    Let's fix this specifically by imitating `diff_fill_oid_info()`.
     +    Let's make the behavior consistent with modified files by showing
     +    all-zero values also for intent-to-add files.
      
     -    Note: we can only do that for diff formats that do not actually need the
     -    file contents, such as `DIFF_FORMAT_PATCH`: `run_diff()` would try to
     -    read the blob contents, but that blob might not have been written to
     -    Git's object database.
     -
     -    This fixes https://github.com/git-for-windows/git/issues/2677
     -
     -    This patch _also_ fixes the expectations set by the regression test
     -    introduced in feea6946a5b (diff-files: treat "i-t-a" files as
     +    Accordingly, this patch adjusts the expectations set by the regression
     +    test introduced in feea6946a5b (diff-files: treat "i-t-a" files as
          "not-in-index", 2020-06-20).
      
          Signed-off-by: Johannes Schindelin <[e-mail]>
      
       ## diff-lib.c ##
      @@ diff-lib.c: int run_diff_files(struct rev_info *revs, unsigned int option)
     - 					       !is_null_oid(&ce->oid),
     - 					       ce->name, 0);
     - 				continue;
     -+			} else if (ce_intent_to_add(ce) &&
     -+				   !(revs->diffopt.output_format &
     -+				     ~(DIFF_FORMAT_RAW | DIFF_FORMAT_NAME_STATUS))) {
     -+				struct object_id oid;
     -+				int ret = lstat(ce->name, &st);
     -+
     -+				if (ret < 0)
     -+					oidclr(&oid);
     -+				else
     -+					ret = index_path(istate, &oid,
     -+						 ce->name, &st, 0);
     -+				diff_addremove(&revs->diffopt, '+', ce->ce_mode,
     -+					       &oid, ret >= 0, ce->name, 0);
     -+				continue;
       			} else if (revs->diffopt.ita_invisible_in_index &&
       				   ce_intent_to_add(ce)) {
       				diff_addremove(&revs->diffopt, '+', ce->ce_mode,
     +-					       the_hash_algo->empty_tree, 0,
     +-					       ce->name, 0);
     ++					       &null_oid, 0, ce->name, 0);
     + 				continue;
     + 			}
     + 
      
       ## t/t2203-add-intent.sh ##
      @@ t/t2203-add-intent.sh: test_expect_success 'i-t-a files shown as new for "diff", "diff-files"; not-new
     @@ t/t2203-add-intent.sh: test_expect_success 'i-t-a files shown as new for "diff",
       	cat >expect.diff_a <<-EOF &&
      -	:000000 100644 0000000 $(git rev-parse --short $hash_t) A$(printf "\t")empty
      -	:000000 100644 0000000 $(git rev-parse --short $hash_t) A$(printf "\t")not-empty
     -+	:000000 100644 0000000 $(git rev-parse --short $hash_e) A$(printf "\t")empty
     -+	:000000 100644 0000000 $(git rev-parse --short $hash_n) A$(printf "\t")not-empty
     ++	:000000 100644 0000000 0000000 A$(printf "\t")empty
     ++	:000000 100644 0000000 0000000 A$(printf "\t")not-empty
       	EOF
       
       	git add -N empty not-empty &&
     -
     - ## t/t4000-diff-format.sh ##
     -@@ t/t4000-diff-format.sh: test_expect_success 'git diff-files --patch --no-patch does not show the patch'
     - 	test_must_be_empty err
     - '
     - 
     -+test_expect_success 'git diff-files --raw handles intent-to-add files correctly' '
     -+	echo 123 >ita &&
     -+	git add -N ita &&
     -+	printf ":000000 100644 %s %s A\\tita\n" \
     -+		$ZERO_OID $(git hash-object --stdin <ita) >expect &&
     -+	git diff-files --raw ita >actual &&
     -+	test_cmp expect actual
     -+'
     -+
     -+
     - test_done
 2:  a9e06427ec < -:  ---------- diff-files: fix incorrect usage of an empty tree
 3:  f51cbedd3f = 2:  9bb8d84ea9 difftool -d: ensure that intent-to-add files are handled correctly