Skip to content
  • Elijah Newren's avatar
    merge-recursive: fix check for skipability of working tree updates · 1de70dbd
    Elijah Newren authored and Junio C Hamano's avatar Junio C Hamano committed
    The can-working-tree-updates-be-skipped check has had a long and blemished
    history.  The update can be skipped iff:
      a) The merge is clean
      b) The merge matches what was in HEAD (content, mode, pathname)
      c) The target path is usable (i.e. not involved in D/F conflict)
    
    Traditionally, we split b into parts:
      b1) The merged result matches the content and mode found in HEAD
      b2) The merged target path existed in HEAD
    
    Steps a & b1 are easy to check; we have always gotten those right.  While
    it is easy to overlook step c, this was fixed seven years ago with commit
    4ab9a157 ("merge_content(): Check whether D/F conflicts are still
    present", 2010-09-20).  merge-recursive didn't have a readily available
    way to directly check step b2, so various approximations were used:
    
      * In commit b2c8c0a7 ("merge-recursive: When we detect we can skip
        an update, actually skip it", 2011-02-28), it was noted that although
        the code claimed it was skipping the update, it did not actually skip
        the update.  The code was made to skip it, but used lstat(path, ...)
        as an approximation to path-was-tracked-in-index-before-merge.
    
      * In commit 5b448b85 ("merge-recursive: When we detect we can skip
        an update, actually skip it", 2011-08-11), the problem with using
        lstat was noted.  It was changed to the approximation
           path2 && strcmp(path, path2)
        which is also wrong.  !path2 || strcmp(path, path2) would have been
        better, but would have fallen short with directory renames.
    
      * In c5b761fb
    
     ("merge-recursive: ensure we write updates for
        directory-renamed file", 2018-02-14), the problem with the previous
        approximation was noted and changed to
           was_tracked(path)
        That looks close to what we were trying to answer, but was_tracked()
        as implemented at the time should have been named is_tracked(); it
        returned something different than what we were looking for.
    
      * To make matters more complex, fixing was_tracked() isn't sufficient
        because the splitting of b into b1 and b2 is wrong.  Consider the
        following merge with a rename/add conflict:
           side A: modify foo, add unrelated bar
           side B: rename foo->bar (but don't modify the mode or contents)
        In this case, the three-way merge of original foo, A's foo, and B's
        bar will result in a desired pathname of bar with the same
        mode/contents that A had for foo.  Thus, A had the right mode and
        contents for the file, and it had the right pathname present (namely,
        bar), but the bar that was present was unrelated to the contents, so
        the working tree update was not skippable.
    
    Fix this by introducing a new function:
       was_tracked_and_matches(o, path, &mfi.oid, mfi.mode)
    and use it to directly check for condition b.
    
    Signed-off-by: default avatarElijah Newren <newren@gmail.com>
    Signed-off-by: default avatarJunio C Hamano <gitster@pobox.com>
    1de70dbd