Expand/Collapse Files and Blob Excerpt while Reviewing/Comparing code #8924

lunny · 2019-11-11T17:17:47Z

blueworrybear commented

2019-11-11 17:17:47 +00:00

(Migrated from github.com)

This PR is to implement:

Expand/Collapse source files while reviewing code (#8579 #8659)
Expand up/down code (Blob Excerpt)

The demo could be referred as below video:

This PR is to implement: 1. Expand/Collapse source files while reviewing code (#8579 #8659) 2. Expand up/down code (Blob Excerpt) The demo could be referred as below video: [![thumbnail](https://i.imgur.com/INNXXmO.png)](https://streamable.com/bn1nu)

❤️ 3 🎉 7

guillep2k (Migrated from github.com) reviewed 2019-11-12 03:01:21 +00:00

guillep2k (Migrated from github.com) left a comment

Some comments.

modules/git/blob.go

						
				@ -53,0 +68,4 @@

						case err == io.EOF:

							return count, nil

						case err != nil:

							return count, err

guillep2k (Migrated from github.com) commented

2019-11-12 02:36:48 +00:00

What happens with Mac files, that use \r?

What happens with Mac files, that use `\r`?

routers/repo/compare.go

guillep2k (Migrated from github.com) commented

2019-11-12 02:40:49 +00:00

// ExcerptBlob render blob excerpt contents

```suggestion // ExcerptBlob render blob excerpt contents ```

services/gitdiff/gitdiff.go

guillep2k (Migrated from github.com) commented

2019-11-12 02:59:27 +00:00

Perhaps it's worth checking the blob type?

blueworrybear (Migrated from github.com) reviewed 2019-11-12 15:17:13 +00:00

routers/repo/compare.go

blueworrybear (Migrated from github.com) commented

2019-11-12 15:17:12 +00:00

fix!

blueworrybear (Migrated from github.com) reviewed 2019-11-12 15:46:47 +00:00

modules/git/blob.go

						
				@ -53,0 +68,4 @@

						case err == io.EOF:

							return count, nil

						case err != nil:

							return count, err

blueworrybear (Migrated from github.com) commented

2019-11-12 15:46:47 +00:00

It seems to be that git blob object will convert line ending to \n. Since we get the reader from EncodedObject, it's probably okay to count \n. ?

It seems to be that git blob object will convert line ending to `\n`. Since we get the reader from `EncodedObject`, it's probably okay to count `\n`. ?

blueworrybear (Migrated from github.com) reviewed 2019-11-12 15:55:58 +00:00

services/gitdiff/gitdiff.go

blueworrybear (Migrated from github.com) commented

2019-11-12 15:55:58 +00:00

I am thinking is it okay for GetTailSection just return nil is the DiffFile is binary. Like:

func (diffFile *DiffFile) GetTailSection(gitRepo *git.Repository, leftCommitID, rightCommitID string) *DiffSection {
	if diffFile.Type != DiffFileChange || diffFile.IsBin || diffFile.IsLFSFile {
		return nil
	}
        // ...
}

I am thinking is it okay for `GetTailSection` just return `nil` is the `DiffFile` is binary. Like: ```go func (diffFile *DiffFile) GetTailSection(gitRepo *git.Repository, leftCommitID, rightCommitID string) *DiffSection { if diffFile.Type != DiffFileChange || diffFile.IsBin || diffFile.IsLFSFile { return nil } // ... } ```

guillep2k (Migrated from github.com) reviewed 2019-11-12 23:11:05 +00:00

modules/git/blob.go

						
				@ -53,0 +68,4 @@

						case err == io.EOF:

							return count, nil

						case err != nil:

							return count, err

guillep2k (Migrated from github.com) commented

2019-11-12 23:11:04 +00:00

It doesn't work for Mac (not that I have one). I've pushed the same file with 7 lines: LF, CRLF and CR. The first two reported 7 lines; the last one reported only one.

guillep2k (Migrated from github.com) reviewed 2019-11-12 23:11:47 +00:00

modules/git/blob.go

						
				@ -53,0 +68,4 @@

						case err == io.EOF:

							return count, nil

						case err != nil:

							return count, err

guillep2k (Migrated from github.com) commented

2019-11-12 23:11:47 +00:00

CRLF works fine because LF is in it.

guillep2k (Migrated from github.com) reviewed 2019-11-12 23:20:56 +00:00

modules/git/blob.go

						
				@ -53,0 +68,4 @@

						case err == io.EOF:

							return count, nil

						case err != nil:

							return count, err

guillep2k (Migrated from github.com) commented

2019-11-12 23:20:56 +00:00

~~My suggestion is to count both \n and \r and take the larger number.~~

Oh, but! The diff tool doesn't recognize \r either! ? So having the correct line count is pointless. That should be a different PR, then.

~~My suggestion is to count both `\n` and `\r` and take the larger number.~~ Oh, but! The diff tool doesn't recognize `\r` either! ? So having the correct line count is pointless. That should be a different PR, then.

guillep2k (Migrated from github.com) approved these changes 2019-11-13 06:47:53 +00:00

lunny requested changes 2019-11-13 07:46:14 +00:00

modules/git/blob.go

						
				@ -53,0 +61,4 @@

					buf := make([]byte, 32*1024)

					count := 0

					lineSep := []byte{'\n'}

					for {

lunny commented

2019-11-13 07:35:46 +00:00

reader should be Close

routers/repo/compare.go

						
				@ -437,0 +537,4 @@

						lineText := scanner.Text()

						diffLine := &gitdiff.DiffLine{

							LeftIdx:  idxLeft + (line - idxRight) + 1,

							RightIdx: line + 1,

lunny commented

2019-11-13 07:41:50 +00:00

As above.

blueworrybear (Migrated from github.com) reviewed 2019-11-14 13:25:13 +00:00

modules/git/blob.go

						
				@ -53,0 +61,4 @@

					buf := make([]byte, 32*1024)

					count := 0

					lineSep := []byte{'\n'}

					for {

blueworrybear (Migrated from github.com) commented

2019-11-14 13:25:12 +00:00

fixed it!

blueworrybear (Migrated from github.com) reviewed 2019-11-14 13:25:24 +00:00

routers/repo/compare.go

						
				@ -437,0 +537,4 @@

						lineText := scanner.Text()

						diffLine := &gitdiff.DiffLine{

							LeftIdx:  idxLeft + (line - idxRight) + 1,

							RightIdx: line + 1,