Skip to content
  • Thomas Rast's avatar
    log -L: store the path instead of a diff_filespec · 31c61918
    Thomas Rast authored and Junio C Hamano's avatar Junio C Hamano committed
    
    
    line_log_data has held a diff_filespec* since the very early versions
    of the code.  However, the only place in the code where we actually
    need the full filespec is parse_range_arg(); in all other cases, we
    are only interested in the path, so there is hardly a reason to store
    a filespec.  Even worse, it causes a lot of redundant ->spec->path
    pointer dereferencing.
    
    And *even* worse, it caused the following bug.  If you merge a rename
    with a modification to the old filename, like so:
    
      * Merge
      | \
      |  * Modify foo
      |  |
      *  | Rename foo->bar
      | /
      * Create foo
    
    we internally -- in process_ranges_merge_commit() -- scan all parents.
    We are mainly looking for one that doesn't have any modifications, so
    that we can assign all the blame to it and simplify away the merge.
    In doing so, we run the normal machinery on all parents in a loop.
    For each parent, we prepare a "working set" line_log_data by making a
    copy with line_log_data_copy(), which does *not* make a copy of the
    spec.
    
    Now suppose the rename is the first parent.  The diff machinery tells
    us that the filepair is ('foo', 'bar').  We duly update the path we
    are interested in:
    
      rg->spec->path = xstrdup(pair->one->path);
    
    But that 'struct spec' is shared between the output line_log_data and
    the original input line_log_data.  So we just wrecked the state of
    process_ranges_merge_commit().  When we get around to the second
    parent, the ranges tell us we are interested in a file 'foo' while the
    commits touch 'bar'.
    
    So most of this patch is just s/->spec->path/->path/ and associated
    management changes.  This implicitly fixes the bug because we removed
    the shared parts between input and output of line_log_data_copy(); it
    is now safe to overwrite the path in the copy.
    
    There's one only somewhat related change: the comment in
    process_all_files() explains the reasoning behind using 'range' there.
    That bit of half-correct code had me sidetracked for a while.
    
    Signed-off-by: default avatarThomas Rast <trast@inf.ethz.ch>
    Signed-off-by: default avatarJunio C Hamano <gitster@pobox.com>
    31c61918