MR shows git diff markers

[Paul Goins]

Recently, I've noticed a few cases where Launchpad has rendered its diffs strangely.

Here is a concrete case which is rendering oddly right now:

https://code.launchpad.net/~peppepetra86/charm-prometheus-grok-exporter/+git/charm-prometheus-grok-exporter/+merge/385646

It's showing git diff markers in the rendered diff. However, those aren't a mistake in what was pushed to git. If you click on the individual changesets in this MR, everything comes through looking sane. This issue appears to be a Launchpad-generated artifact.

[Colin Watson (cjwatson)]

Merge proposal diffs are the result of performing a trial merge into the tip of the target branch, so they can contain conflicts if the source and target branches have diverged. In general the fix is for the author of the source branch to merge or rebase onto the current tip of the target branch and resolve conflicts, which they seem to have done in this case.

Here's a script that reproduces the same conflicts locally based on the merge proposal you referenced:

  $ git clone lp:charm-prometheus-grok-exporter
  Cloning into 'charm-prometheus-grok-exporter'...
  [...]
  $ cd charm-prometheus-grok-exporter
  $ git remote add -f peppepetra86 lp:~peppepetra86/charm-prometheus-grok-exporter
  Updating peppepetra86
  [...]
  $ git checkout c9ef6f30b972b2d8996925a7b6cdd1fab9174ecb
  Note: switching to 'c9ef6f30b972b2d8996925a7b6cdd1fab9174ecb'.
  $ git merge 4f406206718135b8757289e43a4fe2f90f8d2736
  Auto-merging files/metrics-mapping.yaml
  Auto-merging files/grafana-dashboards/grok-exporter-dashboard.json
  CONFLICT (content): Merge conflict in files/grafana-dashboards/grok-exporter-dashboard.json
  Automatic merge failed; fix conflicts and then commit the result.
  $ git diff

[Paul Goins (vultaire)]

Taking the end user perspective here, I disagree with "invalid" here - the feedback is misleading; it looks as if diff markers have been committed into the sources.

If this is expected behavior, can Launchpad print a note or warning that the MR won't cleanly merge and that some action is required to resolve it?

[Xav Paice (xavpaice)]

I have marked several changes as needs fixing because of seeing these markers in the diff for the change - it's really important that the diff shows the actual change, rather than adding extra markers which aren't in the commit, and if the change won't merge cleanly just show a message to indicate that, or better yet a click through thing to display the conflicts.

[Colin Watson (cjwatson)]

@vultaire It does - in such cases there's a conflicts indicator near the top of the MP, in the drop-down list of files affected.

@xavpaice Marking MPs as "needs fixing" in such cases seems perfectly appropriate, since they do in fact need fixing (in the form of conflict resolution).

I disagree that it would be appropriate to drop the conflict markers, though perhaps we can look at making it visually clearer that there's a conflict. (As I said, it *is* presented already, but the fact that you didn't notice that is some evidence that it might be worth improving that presentation.)

[Xav Paice (xavpaice)]

I agree absolutely a change with merge conflicts needs fixing, apologies I didn't make that clear at all. What I am concerned about is that it's impossible to tell if the conflict markers are from the git diff process, or if the committer has manually fixed conflicts and missed some of the markers. If there could be some color or other marking for code that's in the actual change and code that is introduced by conflicts, that would be excellent, though I'm not sure how we'd approach doing that. Guessing this is a feature request?

[Colin Watson (cjwatson)]

The design is intentionally contrary to what you're asking for, although the results happen to often be the same in practice for simple cases. The intent is that a merge proposal's preview diff shows what would be changed in the target branch if the source branch were to be merged into the target branch at the time when the preview diff was generated: or more briefly, "what would happen if I landed this thing?". It is specifically *not* intended that the preview diff be an exact representation of what changes were made in the source branch, and it may differ for various reasons: conflicts are one such case, but for instance it might also be that some of the changes were made on the target branch after the source branch diverged, in which case those changes will be omitted from the preview diff since they aren't part of the projected result of landing the merge proposal.

I will gladly argue and maintain that it's strictly more useful for the preview diff to have its current semantics. As a frequent reviewer of Launchpad merge proposals myself, I generally don't care very much exactly how the changes happened in the source branch: what I mainly care about, and what I argue most reviewers *should* care about, is what would happen to the target branch if I were to approve the merge proposal. (There may be secondary concerns such as clean history, but if a reviewer is concerned with those then they generally need to look directly at individual commits anyway.) It should be largely immaterial to a review whether the conflict markers result from the merge or whether they're literally present in the source branch: the proposer has to fix them either way.

Also, with the present semantics, it's always possible to click through to the source branch and look at individual diffs if you need more detail. By contrast, with the general semantics you're asking for in which the preview diff only represents the changes made to the source branch, there'd be no way to see the projected effect of the merge. (Note that there's no coherent way to represent the effect of a merge as a stream of text without sometimes ending up with some kind of conflict markers, or at least I've never seen any revision control system successfully do so. "See projected effect of merge" implies sometimes having conflict markers. Both "bzr merge" and "git merge" would do exactly the same thing if you tried to merge a branch in this sort of state.)

I don't recall the details for bzr right now, but at least for git I don't believe we have the information you're looking for at the per-line level, only at the per-file level. We ask pygit2 to do most of the work: it gives us a list of conflicted files, and for each of those files we ask it to try harder to produce a merged version (without doing that, all we have are the ancestor version and the version from each side of the merge, rather than anything resembling a diff). pygit2 then gives us a representation of the merged file containing conflict markers: without a considerable amount of extra analysis, we don't know whether any given one of those markers is literally present in the source branch or is an artifact of the trial ...