[PATCH 0/15] retain blob info for git diff HEAD:foo HEAD:bar

On Tue, May 16, 2017 at 10:05:35PM -0400, Jeff King wrote:

The cleanup ended up being a bit bigger than I expected, but I did find an obscure bug while I was there. And I think the end result looks pretty decent.

The patches are listed below. There are a few things I didn't do (and don't plan to in the near future):

  - I didn't pick up Dscho's gitattributes test. The tests in patch 9
    cover that in a much more direct fashion by looking at the diff
    headers. We _could_ still check that they respect gitattributes, but
    there's no reason that they wouldn't; the bug was before we even hit
    the diff machinery at all.

  - There is an interesting related case with attributes that wasn't
    tested by that case. If you do:

       git diff $(git rev-parse HEAD:one) $(git rev-parse HEAD:two)

    then we'll feed those sha1s to the diff machinery as the path (we
    have enough information at that point to know they aren't actually
    paths, but we have to give _something_ to the diff code to display).

    That means we'll also look for .gitattributes files that match those
    names. It would be hard to fix (because the diff machinery doesn't
    have a notion of "this path is for display, but not for attributes).
    And I doubt anybody cares (AIUI, the motivation for the original was
    not that somebody was concerned with reading what is likely to be a
    non-existent attributes file, but rather that looking at names with
    colons produces a bogus warning on Windows).

  - I noticed that the dotdot parser jumps into the middle of the string
    with a strstr(), and never tries to find other dotdots. That means
    things like:

      HEAD@{2..days.ago}...HEAD@{3..days.ago}

    does not parse (we find the first "..", realize that it's not a
    range operator, and then give up looking for more range operators).
    That could be solved by either parsing left-to-right, or by trying
    each ".." in the string in turn. I doubt anybody cares overly much.

    I think we do get the related:

      git show HEAD:foo..bar

    correct, because we see that "HEAD:foo" is not a commit and bail to
    trying the whole thing as a single unit.

I think those are all minor problems that have likely been around for 10+ years, and would take a lot of digging and work to fix. Unless somebody actually hits one in practice, I'm happy to punt for another decade.

  [01/15]: handle_revision_arg: reset "dotdot" consistently
  [02/15]: handle_revision_arg: simplify commit reference lookups
  [03/15]: handle_revision_arg: stop using "dotdot" as a generic pointer
  [04/15]: handle_revision_arg: hoist ".." check out of range parsing
  [05/15]: handle_revision_arg: add handle_dotdot() helper
  [06/15]: sha1_name: consistently refer to object_context as "oc"
  [07/15]: get_sha1_with_context: always initialize oc->symlink_path
  [08/15]: get_sha1_with_context: dynamically allocate oc->path
  [09/15]: t4063: add tests of direct blob diffs
  [10/15]: handle_revision_arg: record modes for "a..b" endpoints
  [11/15]: handle_revision_arg: record paths for pending objects
  [12/15]: diff: pass whole pending entry in blobinfo
  [13/15]: diff: use the word "path" instead of "name" for blobs
  [14/15]: diff: use pending "path" if it is available
  [15/15]: diff: use blob path for blob/file diffs

 builtin/cat-file.c    |   4 +-
 builtin/diff.c        |  60 ++++++-------
 builtin/grep.c        |   4 +-
 builtin/log.c         |  10 ++-
 cache.h               |  10 ++-
 revision.c            | 243 +++++++++++++++++++++++++++++---------------------
 sha1_name.c           |  11 ++-
 t/t4063-diff-blobs.sh |  96 ++++++++++++++++++++
 t/t4202-log.sh        |   9 ++
 tree-walk.c           |   1 -
 10 files changed, 301 insertions(+), 147 deletions(-)
 create mode 100755 t/t4063-diff-blobs.sh

-Peff