format/diff: unified diff encoder and public API #388

ajnavarro · 2017-05-10T14:50:34Z

Added Patch interface
Added a Unified Diff encoder from Patches
Added Change method to generate Patches
Added Changes method to generate Patches
Added Tree method to generate Patches
Added Commit method to generate Patches

It resolves #57

mcuadros · 2017-05-10T16:20:58Z

plumbing/format/diff/unified_encoder.go

+	chunkInit  = "@@ -%s +%s @@%s\n"
+	chunkCount = "%d,%d"
+
+	noFilePath = "/dev/null"


what about Windows?

This is the way that unified diff uses to represent "no file". Is OS independent.

Yeah, in git it's hardcoded without any conditional.

mcuadros · 2017-05-10T16:21:49Z

plumbing/object/file.go

@@ -42,6 +43,17 @@ func (f *File) Contents() (content string, err error) {
 	return buf.String(), nil
 }

+// IsBinary returns if the file is binary or not
+func (f *File) IsBinary() (bool, error) {


lacks of test on this package

codecov · 2017-05-11T13:01:37Z

Codecov Report

Merging #388 into master will decrease coverage by 0.1%.
The diff coverage is 90.87%.

@@            Coverage Diff             @@
##           master     #388      +/-   ##
==========================================
- Coverage   77.27%   77.17%   -0.11%     
==========================================
  Files         123      125       +2     
  Lines        8810     9085     +275     
==========================================
+ Hits         6808     7011     +203     
- Misses       1234     1311      +77     
+ Partials      768      763       -5

Impacted Files	Coverage Δ
plumbing/object/change.go	`100% <100%> (ø)`	⬆️
plumbing/object/commit.go	`69.91% <50%> (-1.39%)`	⬇️
plumbing/object/tree.go	`76.53% <60%> (-0.44%)`	⬇️
plumbing/object/file.go	`75% <66.66%> (-1.09%)`	⬇️
plumbing/object/patch.go	`83.72% <83.72%> (ø)`
utils/binary/read.go	`68.42% <86.66%> (+6.51%)`	⬆️
plumbing/format/diff/unified_encoder.go	`98.25% <98.25%> (ø)`
plumbing/transport/ssh/common.go	`0% <0%> (-50.91%)`	⬇️
plumbing/transport/ssh/auth_method.go	`32.4% <0%> (-24.08%)`	⬇️
repository.go	`72.87% <0%> (-0.02%)`	⬇️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2ff77a8...65416cf. Read the comment docs.

smola · 2017-05-16T09:28:01Z

plumbing/format/diff/unified_encoder.go

+var ErrBothFilesEmpty = errors.New("both files are empty")
+
+// UnifiedEncoder encodes an unified diff into the provided Writer. There are some unsupported features:
+// - Similarity index is not supported


s/Similarity index/Similarity index for renames/

No need to add is not supported if you just said this is a list of unsupported features.

smola · 2017-05-16T09:29:28Z

plumbing/format/diff/unified_encoder.go

+	}
+
+	for _, p := range patch.FilePatches() {
+		f, t := p.Files()


encodeFilePatch function

smola · 2017-05-16T09:30:00Z

plumbing/format/diff/unified_encoder.go

+func (e *UnifiedEncoder) printMessage(message string) error {
+	isEmpty := message == ""
+	hasSuffix := strings.HasSuffix(message, "\n")
+	msg := message


No need to declare this variable.

smola · 2017-05-16T09:52:26Z

plumbing/format/diff/unified_encoder.go

+
+func (c *hunksGenerator) Generate() []*hunk {
+	for i, chunk := range c.chunks {
+		ls := splitLines(chunk.Content())


separate function?

smola · 2017-05-16T09:53:14Z

plumbing/format/diff/unified_encoder.go

+	c.beforeContext = nil
+}
+
+// Logic used to obtain line numbers in a new chunk


Suggested wording: addLineNumbers obtains the line numbers in a new chunk

smola · 2017-05-16T09:54:33Z

plumbing/object/commit.go

+	if err != nil {
+		return nil, err
+	}
+	toTree, err := to.Tree()


smola · 2017-05-16T09:54:58Z

plumbing/object/patch.go

+func getPatch(message string, changes ...*Change) (fdiff.Patcheable, error) {
+	var filePatches []fdiff.FilePatch
+	for _, c := range changes {
+		from, to, err := c.Files()


move to function

smola · 2017-05-16T09:55:29Z

utils/binary/read.go

+
+const sniffLen = 8000
+
+//IsBinary detects if data is a binary value based on:


Spaces after //

alcortesm

I think the package can be improved by writing a package level documentation explaining the package itself, its purpose, design and style. Some examples will be good too.

The tests are quite awesome.

I don't know much about templates, but it looks as if the unified encoder can be done using templates. Not sure if it is a good idea though.

The explanation of the encoder should come along with a pointer to the documentation of the format.

alcortesm · 2017-05-16T10:08:05Z

plumbing/format/diff/patch.go

+	"gopkg.in/src-d/go-git.v4/plumbing/filemode"
+)
+
+type Operation int


Document this type and its values.

alcortesm · 2017-05-16T10:08:16Z

plumbing/format/diff/patch.go

+
+// Patch represents the necessary steps to transform several files to another.
+type Patch interface {
+	FilePatches() []FilePatch


Document this methods.

alcortesm · 2017-05-16T10:11:35Z

plumbing/format/diff/patch.go

+
+// Patcheable is a Patch but with several ways to encode it. It is used to be
+// able to use Patch in a fluent way.
+type Patcheable interface {


I don't understand this interface or its comments.

I think the name, Patcheable, is not transmitting the full meaning of the interface, as implementing this interface does not mean something can be Pathed.

Also, the comment states the are several ways to encode it, but there is only one method and it is not called encode or mention encoding anywhere in its documentation.

Maybe if you explain what you want to achieve here and how, it will be easier to understand. And example can also help.

I rewrote the documentation, maybe is clear now. If not, can you suggest a better name for this interface?

alcortesm · 2017-05-16T10:12:52Z

plumbing/format/diff/patch.go

+	// UnifiedDiff writes the patch into the provided writer in unified
+	// diff format. ctxLines changes the amount of unchanged lines that will be
+	// printed before and after each chunk.
+	UnifiedDiff(ctxLines int, w io.Writer) error


Make the writer the first argument, a l`a golang way.

I don't understand this, shouldn't this be an encoder of Path?

I changed the documentation, I hope is clear now.
✅ to change arguments order.

alcortesm · 2017-05-16T10:13:51Z

plumbing/format/diff/patch.go

+
+// FilePatch represents the necessary steps to transform one file to another
+type FilePatch interface {
+	IsBinary() bool


Document the methods.

What is binary here, to or from or the FilePath itself, if it is the FilePath itself, then explain that there are two types of FilePatches and why.

Explain what will from and to be when inserting or deleting a file (Hash?, filemode? path? or it will be just nil?)

alcortesm · 2017-05-16T10:47:07Z

plumbing/format/diff/unified_encoder.go

+}
+
+func (e *UnifiedEncoder) prints(s string) error {
+	_, err := e.Write([]byte(s))


Remove this all together, it is a print method that does not print.

This should be a call to bytes.Buffer.WriteTo.

alcortesm · 2017-05-16T10:49:38Z

plumbing/format/diff/unified_encoder.go

+	}
+}
+
+func (c *hunksGenerator) Generate() []*hunk {


Why is this public?

Because the struct is private, to know which methods are intended to be used outside.

This is usually achieved in Go using an internal package.

doesn't need to be an internal package, this is also a good practice at least for me

alcortesm · 2017-05-16T10:51:30Z

plumbing/format/diff/unified_encoder.go

+	return buf.String()
+}
+
+func (c *hunk) AddOp(t Operation, s ...string) {


Why is this public?

Because the struct is private, to know which methods are intended to be used outside.

alcortesm · 2017-05-16T12:17:40Z

plumbing/object/change.go

@@ -78,6 +79,12 @@ func (c *Change) String() string {
 	return fmt.Sprintf("<Action: %s, Path: %s>", action, c.name())
 }

+// Patch returns a Patch with all the file changes in chunks. This
+// representation can be used to create several diff outputs.
+func (c *Change) Patch() (diff.Patcheable, error) {


Will it make sense for Change to implement Patch or FilePatch instead of having a method that returns a Path?

It could be, but appears better to me have the functionality separated. What do you think @smola @mcuadros ?

alcortesm · 2017-05-16T12:20:19Z

plumbing/object/patch.go

+	filePatches []fdiff.FilePatch
+}
+
+func (t textPatch) FilePatches() []fdiff.FilePatch {


Document all the methods in these files. A simple ... implements <interface>. will suffice most of the times.

alcortesm · 2017-05-22T09:01:38Z

plumbing/format/diff/patch.go

+	Delete
+)
+
+// Patch represents the necessary steps to transform several files to another.


English can be improved a little bit by simplifying the statement:

Patch represents a collection of steps to transform several files

alcortesm · 2017-05-22T09:06:09Z

plumbing/format/diff/patch.go

+// Patcheable contains ways to represent a patch. Its use is intended to create
+// fluents APIs, and be able to discover more easily ways to work with Patches.
+// You don't need to know that a NewUnifiedEncoder constructor exists, you just
+// need to see the available methods after get a Patch.


I'm sorry, I still don't understand this interface. It looks as a Patch encoder but the name and the description suggest otherwise. Maybe it is just me, let's talk about this. Also the name of the function is specific to a format, but that format is not part of the name of the interface.

The idea of this interface is do this:

p, err := commit.Patch(otherCommit) handleErr(err) err = p.UnifiedDiff(buf,2) handleErr(err)

Instead of:

p, err := commit.Patch(otherCommit) handleErr(err) // To do this, you need to know the internals or find it into the documentation ue := diff.NewUnifiedEncoder(buf, 1) err = ue.Encode(p) handleErr(err)

alcortesm · 2017-05-22T09:12:08Z

plumbing/format/diff/patch.go

+
+// Chunk is a operation to process to transform a file to another.
+// Operations can be Equal, Add or Delete
+type Chunk interface {


alcortesm · 2017-05-22T09:16:38Z

plumbing/format/diff/unified_encoder.go

+
+func (e *UnifiedEncoder) printMessage(message string) {
+	isEmpty := message == ""
+	hasSuffix := strings.HasSuffix(message, "\n")


I still think endsInNewLine is a better name for this variable.

the message is a specific part of the patch format, is the very beginning of the result file. This method is not intended to write other kind of lines, just this header message if any.

alcortesm · 2017-05-22T09:22:38Z

plumbing/format/diff/unified_encoder.go

+}
+
+func (c *hunk) WriteTo(buf *bytes.Buffer) {
+	fromString := fmt.Sprintf(chunkCount, c.fromLine, c.fromCount)


This method need some improvements:

don't use strings, write to buf directly using fmt.Fprintf

don't generate the string / write to buf twice depending on c.fromCount or c.toCount, use an if ... else clause to generate the content only once.

alcortesm · 2017-05-22T09:23:14Z

plumbing/format/diff/unified_encoder.go

+		toString = fmt.Sprintf("%d", c.toLine)
+	}
+
+	buf.WriteString(fmt.Sprintf(chunkInit, fromString, toString, c.ctxPrefix))


This is very wrong, just write to the buf with out generating the string.

alcortesm · 2017-05-22T09:24:06Z

plumbing/format/diff/unified_encoder.go

+	}
+}
+
+func (c *hunksGenerator) Generate() []*hunk {


This is usually achieved in Go using an internal package.

alcortesm

The version is super good. It still have some minor issues and I still don't understand the Patcheable interface, maybe we should talk about it.

mcuadros · 2017-05-23T06:10:07Z

@ajnavarro before merge please update https://github.com/src-d/go-git/blob/master/COMPATIBILITY.md

- Added Patch interface - Added a Unified Diff encoder from Patches - Added Change method to generate Patches - Added Changes method to generate Patches - Added Tree method to generate Patches - Added Commit method to generate Patches

ajnavarro · 2017-05-23T09:18:16Z

@mcuadros rebased and unnecessary commits squashed. Also added diff line into compatibility file.

mcuadros reviewed May 10, 2017

View reviewed changes

ajnavarro changed the title ~~[WIP] plumbing/format/diff: unified diff encoder~~ plumbing/format/diff: unified diff encoder May 16, 2017

ajnavarro requested review from smola, mcuadros and alcortesm May 16, 2017 08:22

smola suggested changes May 16, 2017

View reviewed changes

alcortesm suggested changes May 16, 2017

View reviewed changes

alcortesm reviewed May 22, 2017

View reviewed changes

alcortesm suggested changes May 22, 2017

View reviewed changes

alcortesm approved these changes May 22, 2017

View reviewed changes

smola approved these changes May 22, 2017

View reviewed changes

mcuadros approved these changes May 22, 2017

View reviewed changes

ajnavarro added 2 commits May 23, 2017 11:05

format/diff: unified diff encoder and public API

2f293f4

- Added Patch interface - Added a Unified Diff encoder from Patches - Added Change method to generate Patches - Added Changes method to generate Patches - Added Tree method to generate Patches - Added Commit method to generate Patches

Add diff to compatibility file

65416cf

ajnavarro changed the title ~~plumbing/format/diff: unified diff encoder~~ format/diff: unified diff encoder and public API May 23, 2017

mcuadros merged commit f663a93 into src-d:master May 23, 2017


		const sniffLen = 8000

		//IsBinary detects if data is a binary value based on:

format/diff: unified diff encoder and public API #388

format/diff: unified diff encoder and public API #388

Uh oh!

Conversation

ajnavarro commented May 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented May 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alcortesm left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

ajnavarro commented May 10, 2017 •

edited

Loading

codecov bot commented May 11, 2017 •

edited

Loading

alcortesm left a comment •

edited

Loading