Skip to content

Conversation

@rwe
Copy link
Contributor

@rwe rwe commented Sep 29, 2021

When parsing conflicts, git-rerere ignores/preserves conflict markers that are not exactly the expected length, which defaults to 7.

For <<<<<<< and >>>>>>> , it also requires them to end in a space, to avoid ambiguity.

See git/rerere.c:is_cmarker for reference + additional commentary.

This updates git-revise's parsing to treat them the same way.

@krobelus
Copy link
Contributor

Looks good.
If the conflict-marker-size gitattribute is set, git merge-file can produce longer conflict markers.
Since git-revise doesn't read gitattributes we'll fail to parse those.
I don't think that's a problem because conflict-marker-size is usually used for files containing >>>>>>> which we previously attempted to parse as conflict. Fixing that is more important.

@rwe
Copy link
Contributor Author

rwe commented Sep 30, 2021

Yes — I actually have a commit that uses git check-attr to get that value, but I'm hesitant to push it up due to unknown performance impact. If introduced it should probably be batched/cached similarly to how OIDs are handled. If interested, that naïve implementation looks like this (plus some extra notes):

def get_git_attr(repo: Repository, file: Path, attr_name: str) -> bytes:
    # TODO: Batch multiple paths, attrs and cache them. Keep the process open like with cat-file.
    # Result format is:
    #   <path> NUL <attribute> NUL <value> NUL
    result = repo.git("check-attr", "--cached", "-z", attr_name, "--", file)
    # Note that check-attr respects cwd, so if we want to use relative paths,
    # these would be relative too.
    # The response name is exactly the input, not normalized:
    # if we ask for "Foo" vs "foo" vs "./foo", the value we asked for is in the response.
    (resp_path, resp_attr_name, value) = result.split(b"\0", maxsplit=3)[:-1]
    assert os.fsdecode(resp_path) == str(file)
    assert resp_attr_name.decode() == attr_name
    return value


def get_conflict_marker_size(repo: Repository, file: Path) -> int:
    # See ll_merge_marker_size in git/ll-merge.c
    value = get_git_attr(repo, file, "conflict-marker-size")
    try:
        if value != b"unspecified":
            return int(value)
    except ValueError:
        pass
    return DEFAULT_CONFLICT_MARKER_SIZE

@rwe
Copy link
Contributor Author

rwe commented Sep 30, 2021

Oh, woops, my reply above refers to yet another branch which I did intend to PR, which factors that marker_size value out. I'll push that up.

EDIT: See #101

git-rerere strictly ignores conflict markers that are not exactly the
expected length. For `<` and `>`, it also requires them to end in a
space.

See git/rerere.c:is_cmarker
@rwe rwe force-pushed the rerere-skip-unsized-marker branch from b01a1de to 6c9f365 Compare January 10, 2022 21:30
@mystor mystor merged commit 0c83d79 into mystor:main Feb 5, 2022
@mystor
Copy link
Owner

mystor commented Feb 5, 2022

Thanks for the fix :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants