Investigate behaviour of gitattributes without satellites

@dzaporozhets move to 8.1?

From https://gitlab.slack.com/archives/core/p1443048046001327 expect a lot of merge conflicts in the CHANGELOG file this release. Seems that the .gitattributes is not being considered at the moment. (I saw an issue for this, gotta find it^^) I had to ask a user to rebase again today because of this

I think this is 8.0 patch release material due to the comment above.

Thanks @sytses.

@rspeicher can you take this along with a patch release?

@dzaporozhets is this on your radar?

@JobV no I am really busy with CI integration and other stuff. We either need volunteer or later milestone

Milestone changed to 8.2

mentioned in issue #2840 (closed)

mentioned in issue #262 (closed)

I did some investigation in this by digging through libgit2 and git source code. git has support for custom merge drivers (e.g. union), which allows the .gitattributes merge=union trick. While libgit2 and rugged have support for reading .gitattributes, they don't appear to have support for custom merge drivers. However, as the issue alludes, we might be able to do something like the following:

Attempt to merge A and B
If there are conflicts, load the .gitattributes and iterate through each conflict.
For each conflicted file, if there is an entry in .gitattributes and the merge type is union, then set the favor to GIT_MERGE_FILE_FAVOR_UNION in the merge options. Attempt to merge just those files one-by-one.
If all conflicts merge successfully individually, then declare success.

I'll play around to see if this is possible. Long term it would be better for libgit2/rugged to support this use case natively.

Looks like this one is a bit difficult to fix. I can easily call rugged.merge_commits(first, second, favor: :union), but this would result in a blanket union merge, which is not correct. We want this only to apply to the CHANGELOG, which isn't support at the moment by libgit2 and rugged.

I'll see what I can do push this forward, but no guarantees.

I think I got this to work in libgit2, although I'm not sure if the PR will be accepted. Fun times with C code: https://github.com/libgit2/libgit2/pull/3497.

Milestone changed to 8.4

moving this forward for visibility. Keep it up @stanhu!

Just an update on this issue: Edward Thomson has taken my PR and added full merge driver support: https://github.com/libgit2/libgit2/pull/3564. This is currently being reviewed.

This PR will not only solve the merge=union case in .gitattributes, but also merge=text etc.

This probably will take a while longer to land, so I'm guessing 8.5 or 8.6 might be a more realistic target.

Milestone changed to 8.5

Thanks @stanhu

mentioned in issue #5902 (closed)

Would this feature also bring support for marking specific files as binary or text using the .gitattributes file? This issue on Github is marked as closed, but there is a question over if it is actually supported and if this work will change that.

Essentially I'd like to be able to mark compiled files as binary and their diffs to not show up in the changes tab of commits or merge requests.

I'm also very much interested in marking files as binary. We have our gitlab failing to generate a diff time after time, because there are some huge text files present which are actually binary. We mark them like that in .gitattributes: dist/**/* -diff, and then they're shown as binary in git diff, but GitLab ignores .gitattributes for now, and that makes merge requests much more painful (and sometimes even impossible to create)

Milestone changed to 8.6

I've created a small test repository which demonstrates the behaviour I think Gitlab should be showing. Like @tsayen I would like to mark some text files as binary as they are large and cause Gitlab to become unusable when they are changed (which they frequently are).

This is the example project: https://gitlab.com/matto1990/diff-example

There there two branches. master has a txt file and a .gitattributes file which has the rule *.txt binary which marks all txt files as binary. On the change branch I've added a lint into the txt file.

If you run:

git checkout change
git diff master

You will correctly get:

diff --git a/untimely.txt b/untimely.txt
index 8a239dc..09430a5 100644
Binary files a/untimely.txt and b/untimely.txt differ

When comparing using Gitlab, you instead get the text diff: https://gitlab.com/matto1990/diff-example/compare/master...change

The same is true if you create a merge request: https://gitlab.com/matto1990/diff-example/merge_requests/1/diffs

At first, I assumed that libgit2 was not correctly supporting the binary attribute in .gitattributes, however after playing around a bit it seems that it is supported already for diffs.

You can try it yourself by compiling the diff` example, copying it into the example repository and running it.

git clone https://github.com/libgit2/libgit2.git
cd libgit2/examples
make
cd ../../
git clone https://gitlab.com/matto1990/diff-example.git
cd diff-example
cp ../libgit2/examples/diff .
./diff master

This gives you the correct output:

diff --git a/untimely.txt b/untimely.txt
index 8a2..094 100644
Binary files a/untimely.txt and b/untimely.txt differ

It seems to me that the issue with the .gitattributes file being ignored is somewhere in either Rugged or gitlab_git. I've not yet had time to test the behaviour or either of the wrappers yet.

Hopefully this helps you. This is functionality which is really needed for us, and is causing some to question moving to Github (interestingly Github also fails to look at the .gitattributes file, pointing to it being a Rugged bug, however, they are able to cope with having large text files much better than our Gitlab instance is).

@matto1990 Thank you so much for the detailed test. I think I'm beginning to understand what is going on here. The key here is that using .gitattributes for binary diff generation does not appear to work on a bare repository:

$ git clone --bare https://gitlab.com/matto1990/diff-example.git
$ git diff da222d2f..068f578
diff --git a/untimely.txt b/untimely.txt
index 8a239dc..09430a5 100644
--- a/untimely.txt
+++ b/untimely.txt
@@ -1,6 +1,8 @@
 Untimely
 ========
 
+This is an edit.
+
 Nothing in life has been made by man for man's using
 But it was shown long since to man in ages
 Lost as the name of the maker of it,

I verified that libgit2/Rugged properly supports generating the right diff for the binary case in a non-bare repository. Using your repo, you can show this via this test code:

require 'rugged'

repo = Rugged::Repository.new('/tmp/diff-example')
diff = repo.diff('da222d2f71ece0fe41c53dfccdb7a69514182364', "068f578045b4983ad5b8bdde1f563fa620824860", {})
puts diff.patch

gitlab_git uses similar code above to generate the patch, but since GitLab operates only on bare repositories, the diff doesn't obey the .gitattributes file.

Previous versions of GitLab used to use satellites, which would have generated the proper diff. Perhaps there is a way to patch libgit2 to look at .gitattributes even for a bare repositories.

Note that git merge drivers are different from diff drivers. The merge=union case should work on bare repositories once the libgit2 changes are merged.

@stanhu Thanks for the reply! So it seems like the issue lies with libgit2. I've opened an issue on their Github repo, so we'll and see what the maintainers say about it. My experience with these projects is fairly minimal, so I'm not sure how big a task it will be to support .gitattributes for bare repositories.

@matto1990 Again, thanks for raising the issue in libgit2. In light of the fact that git doesn't support .gitattributes in a bare repo, there are a few ways to proceed:

Submit patches to git to use HEAD of .gitattributes in a bare repo (and then make libgit2 do the same thing)
Allow a user to specify info/attributes file in the bare repo
Automatically copy HEAD of .gitattributes into info/attributes
Bring back the use of satellites for generating diffs (laying this out here as an option)

It seems that option 1 is the best approach but may be the most complicated and time-consuming.

Option 2 would probably be too confusing for the user.
Option 3 is intriguing as a feature of GitLab.
Option 4 seems unscalable, which is why satellites were removed in the first place.

Thoughts?

I agree that option 1 is the best by far. That would be the ideal path to go down, but like you say, it would take the most amount of time.

Would it be possible to implement option 3 as a short-term fix, while also looking into a "proper" solution with option 1?

@dzaporozhets @DouweM What do you guys think about making GitLab sync HEAD of .gitattributes to the bare repo info/attributes?

Bring back the use of satellites for generating diffs (laying this out here as an option)

@stanhu looking for furious emoji

What do you guys think about making GitLab sync HEAD of .gitattributes to the bare repo info/attributes?

@stanhu if its the cheapest way to make gitattributes work - I am for it.

@dzaporozhets @stanhu Where would it be best to make this change? I'm happy to take a look at it if it's a good task for a codebase beginner. I'm assuming it would be in the gitlab-git project. After a very quick inspection, somwhere in the repository.py file seems like a good place to start.

@matto1990 I would first try to write a script that uses Rugged to do the following:

Look up the SHA ID for HEAD
Figure out what blob OID is there for .gitattributes
Retrieve the contents of the blob
Copy this file into the bare repo info/attributes.

Then I think we might be able to add that functionality into gitlab_git and add a call to that in app/workers/git_push_service.rb that updates the info/attributes files for every push to the default branch.

I think the GitLab compare controller handles the binary diff message, but if it doesn't that would have to be adjusted as well.

Perfect. Thanks for the information @stanhu! I'll hopefully get some time to take a look at this over the weekend.

mentioned in merge request gitlab_git!73 (merged)

I've opened a merge request for the gitlab_git part of the task, however, I think the part for inside Gitlab will need more thinking than you mentioned so far.

The cases I can think of for using the .gitattributes file are (I'm mainly thinking about generating the diffs for merge requests:

No attributes in the target branch. None in source branch.
No attributes in the target branch. New file in source branch.
Attributes in the target branch. Same in source branch.
Attributes in the target branch. Updated ones in source branch.
Attributes in the target branch. Removed in source branch.

For the diffs to be correct, it seems to me that we would need to make sure we are using the attributes from the source branch before every diff.

The method you mentioned with using the attributes from the default branch would be simpler, but would lead to incorrect results in a lot of the cases. However, it's unlikely that the .gitattributes file changes often, so maybe this is fine for a first version.

Any thoughts on the best way forward?

@matto1990 Thanks for submitting the MR, and good investigation. You're right. It's too bad this git thread didn't get more comments. It sounds like one of the git authors thought this:

I was discussing this on IRC with pasky and he suggested that the best way to approach the matter was to use the .gitattributes from the relevant head.

Based on my experiments, it looks like the command-line git reads from the current working branch on a non-bare repository.

The only way to match this functionality is to write out the info/attributes file before each diff, but that seems like it could lead to other issues (e.g. race conditions when multiple users generate a diff for different branches).

I think if we want to keep things simple, stick to using the default branch (HEAD) and use the .gitattributes from that. My only worry this could also lead to unintended issues down the road. What do you think, @DouweM?

You could always hand-edit the info/attributes for the repositories in question. Not ideal, but it would work today.

I started poking at the git source code to see what it would take to support .gitattributes for a bare repo.

Curiously, I can't seem to get it working when I manually edit the info/attributes file in the repo on our Gitlab server.

I'm editing the file in /var/opt/gitlab/git-data/repositories/redacted/redacted.git/info/attributes and adding in the exact contents as is working in .gitattributes. I've also ensured the file has the correct owner and group (git) and given is very open permissions (775).

When doing a diff via a merge the request, the files I've marked as binary are still showing up with a textual diff.

It's not super important as it's a hack anyway, but any idea what might be going on? Maybe there some caching happening somewhere?

@matto1990 Yes, I believe the diffs are cached each time after a push occurs. One trick: append ?w=1 to the URL, and I think the diff is generated on-the-fly. You can also try pushing to the branch with a new commit (or force push after a git commit --amend).

@stanhu Thanks for the tricks!

Once I did that, I think I ran into a bug with how diffs are shown. The file in the screenshot is one of the text files which I would like to be treated as binary. It is now no longer showing a diff of the changes, but the text content at the start of the diff is still shown to the user (I couldn't show you the whole file in case there's something under NDA in there).

Hope this helps!

@stanhu

I think if we want to keep things simple, stick to using the default branch (HEAD) and use the .gitattributes from that.

Sounds good.

My only worry this could also lead to unintended issues down the road.

The only issue I can see is the wrong attributes being used when .gitattributes was changed in the branch being compared. That's slightly annoying, but acceptable. It doesn't change often, and most compares will be against the default branch anyway.

@matto1990 Can you regenerate the issue in your test repository there?

@stanhu Sorry, I'm not what you mean.

mentioned in commit gitlab_git@6d36f4b5

As @stanhu suggested, I've added the code to copy the attributes into the app/services/git_push_service.rb file. You can see the changes needed for that here: https://gitlab.com/matto1990/gitlab-ce/commit/2d4d63e30cdacb7a0e5fe4f5cba2fe6901cbf2f9

I've tried it out locally it it works fine, however, the diff is still showing up slightly strange in the MR for my test repository. It seems to be that it outputs the letter B (I assume for binary) and then the first line of the text file which has been marked as binary.

I've had a look at chasing through where the diff is coming from, but it seems like it's a few layers deep (Gitlab -> gitlab_git -> rugged -> libgit2) so I'm not certain where the problem is being introduced.

Aside from the diff display issue (which is more cosmetic than anything else), it would also be good to get some advice on the best way to test this new behaviour.

@matto1990 My guess is that the compare controller does not handle the "Binary files X and Y differ" case properly. We may need to look at the diff for the file and add some special handling for it.

@matto1990 To determine whether to render the diff as text or not, we call blob.text? which is Linguist::BlobHelper#text?, which uses heuristics to determine if the file is text or binary, and doesn't actually check .gitattributes. I don't know if we can ask rugged/libgit2 if it sees the file as text as binary, or if we should somehow parse .gitattributes manually.

@stanhu @DouweM I've taken a look at that and have done a quick experiment to prove it works.

I've added an extra check in app/helpers/blob_helper.rb's blob_text_viewable? method to check if the blob is diffable. To do this it gets the attributes from Rugged and checks for the diff attribute, returning true if there isn't one. This seems to be the best attribute to use, as it is set the false if you have either of these lines, both of which are commonly suggested as a way to stop files showing up in a diff:

*.txt -diff
*.txt binary

When adding this in, the file text or comment buttons in the header are no longer shown. Instead, it now shows a No preview for this file type message, which is something I think could be changed to be more descriptive.

The full changes so far are here: https://gitlab.com/matto1990/gitlab-ce/compare/master...gitattributes

Change I want to make before this is done:

Move blob_diffable? method into gitlab_git as blob.diffable?.
Investigate adding a better message when a diff is suppressed.

Does this sound like a reasonable path to be going down?

@matto1990 Both suggestions sound great!

@matto1990 Sounds like a great plan. I do wonder what the performance impact would be of doing a .gitattributes lookup for every blob in a diff. I wonder if it's easier just to handle the "Binary files X and Y differ" message gracefully in a diff.

mentioned in merge request gitlab_git!76 (merged)

Milestone changed to 8.7

@matto1990 This line strips out the binary diff header and leaves the diff empty. What if we set an attribute (e.g. @binary = true) in the diff, and then had GitLab display a special message for binary files? That would not only help existing diffs but also take care of the .gitattributes issue.

mentioned in issue #15241 (closed)

Milestone changed to 8.8

mentioned in commit gitlab_git@8e42012a

@rspeicher Replying to your comment here instead. In my last message above you can see the steps I've taken so far and the changes I've made (https://gitlab.com/matto1990/gitlab-ce/compare/master...gitattributes). My current implementation works and displays the same messages that an unknown binary file previously would have in Gitlab.

@stanhu Has suggested making a further change to gitlab_git to handle this behaviour inside the diff creation itself. I'm hesitant to make the changes myself to this method as even the comments within the method seem unsure of its behaviour. I think the steps to get this working would be to set a binary attribute if it detects the file is binary and then file diff template look for this attribute and show a specific message.

If that is correct, and that is the preferred method of doing this, I can look into it over the weekend. Let me know what you think my next steps should be.

mentioned in issue #1803 (closed)

@matto1990 That sounds reasonable to me.

mentioned in merge request !3806 (merged)

@DouweM @stanhu Merged request has been opened: gitlab-org/gitlab-ce!3806

FYI, this issue deals with multiple things with .gitattributes:

@matto1990 has a MR to suppress diffs in files specified as binary
Changes necessary to support the merge=union attribute that helps GitLab avoid conflicts with CHANGELOG

The second was merged to libgit2 on March 17 via https://github.com/libgit2/libgit2/pull/3564. Presumably it will be available in a release in libgit2 v0.25.0. Still waiting for that release.

Status changed to closed by commit bfc6a0e3

mentioned in commit bfc6a0e3

I'm going to reopen this issue to keep the merge=union part active--that's the one that affects us a lot.

Status changed to reopened

Status changed to closed by commit gitlab-org/ce-security@bfc6a0e3718c1b4d5e3d2adcc1ef16cf5274df5c

Status changed to reopened

Status changed to closed by commit markpundsack/gitlab-ce@bfc6a0e3718c1b4d5e3d2adcc1ef16cf5274df5c

mentioned in issue #17325 (closed)

Mentioned in commit pfjason/gitlab-ce@bfc6a0e3

Mentioned in issue #20085 (moved)

mentioned in commit test-nick/gitlab-ce@a0f113a0

Investigate behaviour of gitattributes without satellites

Designs

Child items ...

Activity

Admin message

Admin message

Investigate behaviour of gitattributes without satellites

Activity