From df216fd67b2c59d10ca4f2d8b8ffee8f6b7c6761 Mon Sep 17 00:00:00 2001 From: Richard Mahn Date: Sat, 11 Sep 2021 06:35:24 -0400 Subject: [PATCH 01/17] Fixes #16837 - CSV column diff error --- services/gitdiff/csv.go | 34 +++--------- services/gitdiff/csv_test.go | 87 +++++++++++++++++++++++++++++++ templates/repo/diff/csv_diff.tmpl | 8 ++- 3 files changed, 101 insertions(+), 28 deletions(-) diff --git a/services/gitdiff/csv.go b/services/gitdiff/csv.go index f4310d8772870..082d4bbd52ddf 100644 --- a/services/gitdiff/csv.go +++ b/services/gitdiff/csv.go @@ -161,31 +161,6 @@ func createCsvDiff(diffFile *DiffFile, baseReader *csv.Reader, headReader *csv.R createDiffRow := func(aline int, bline int) (*TableDiffRow, error) { cells := make([]*TableDiffCell, columns) - if aline == 0 || bline == 0 { - var ( - row []string - celltype TableDiffCellType - err error - ) - if bline == 0 { - row, err = a.GetRow(aline - 1) - celltype = TableDiffCellDel - } else { - row, err = b.GetRow(bline - 1) - celltype = TableDiffCellAdd - } - if err != nil { - return nil, err - } - if row == nil { - return nil, nil - } - for i := 0; i < len(row); i++ { - cells[i] = &TableDiffCell{LeftCell: row[i], Type: celltype} - } - return &TableDiffRow{RowIdx: bline, Cells: cells}, nil - } - arow, err := a.GetRow(aline - 1) if err != nil { return nil, err @@ -216,7 +191,14 @@ func createCsvDiff(diffFile *DiffFile, baseReader *csv.Reader, headReader *csv.R for i := 0; i < len(b2a); i++ { if b2a[i] == unmappedColumn { bcell, _ := getCell(brow, i) - cells[i] = &TableDiffCell{LeftCell: bcell, Type: TableDiffCellAdd} + cells_index := i + if len(a2b) >= i+1 && a2b[i] <= i { + cells_index = i + 1 + } + if cells[cells_index] != nil && len(cells) >= cells_index+1 { + copy(cells[cells_index+1:], cells[cells_index:]) + } + cells[cells_index] = &TableDiffCell{LeftCell: bcell, Type: TableDiffCellAdd} } } diff --git a/services/gitdiff/csv_test.go b/services/gitdiff/csv_test.go index fb84d6ed06320..cd9b08981a416 100644 --- a/services/gitdiff/csv_test.go +++ b/services/gitdiff/csv_test.go @@ -123,3 +123,90 @@ func TestCSVDiff(t *testing.T) { } } } + +func TestCSVDiffHeadingChange(t *testing.T) { + var cases = []struct { + diff string + base string + head string + cells [][4]TableDiffCellType + }{ + // case 0 - renames first column + { + diff: `diff --git a/unittest.csv b/unittest.csv +--- a/unittest.csv ++++ b/unittest.csv +@@ -1,3 +1,3 @@ +-col1,col2,col3 ++cola,col2,col3 + a,b,c`, + base: "col1,col2,col3\na,b,c", + head: "cola,col2,col3\na,b,c", + cells: [][4]TableDiffCellType{{TableDiffCellDel, TableDiffCellAdd, TableDiffCellEqual, TableDiffCellEqual}, {TableDiffCellDel, TableDiffCellAdd, TableDiffCellEqual, TableDiffCellEqual}}, + }, + // case 1 - inserts a column after first, deletes last column + { + diff: `diff --git a/unittest.csv b/unittest.csv +--- a/unittest.csv ++++ b/unittest.csv +@@ -1,2 +1,2 @@ +-col1,col2,col3 +-a,b,c ++col1,col1a,col2 ++a,d,b`, + base: "col1,col2,col3\na,b,c", + head: "col1,col1a,col2\na,d,b", + cells: [][4]TableDiffCellType{{TableDiffCellEqual, TableDiffCellAdd, TableDiffCellEqual, TableDiffCellDel}, {TableDiffCellEqual, TableDiffCellAdd, TableDiffCellEqual, TableDiffCellDel}}, + }, + // case 2 - deletes first column, inserts column after last + { + diff: `diff --git a/unittest.csv b/unittest.csv +--- a/unittest.csv ++++ b/unittest.csv +@@ -1,2 +1,2 @@ +-col1,col2,col3 +-a,b,c ++col2,col3,col4 ++b,c,d`, + base: "col1,col2,col3\na,b,c", + head: "col2,col3,col4\nb,c,d", + cells: [][4]TableDiffCellType{{TableDiffCellDel, TableDiffCellEqual, TableDiffCellEqual, TableDiffCellAdd}, {TableDiffCellDel, TableDiffCellEqual, TableDiffCellEqual, TableDiffCellAdd}}, + }, + } + + for n, c := range cases { + diff, err := ParsePatch(setting.Git.MaxGitDiffLines, setting.Git.MaxGitDiffLineCharacters, setting.Git.MaxGitDiffFiles, strings.NewReader(c.diff)) + if err != nil { + t.Errorf("ParsePatch failed: %s", err) + } + + var baseReader *csv.Reader + if len(c.base) > 0 { + baseReader, err = csv_module.CreateReaderAndGuessDelimiter(strings.NewReader(c.base)) + if err != nil { + t.Errorf("CreateReaderAndGuessDelimiter failed: %s", err) + } + } + var headReader *csv.Reader + if len(c.head) > 0 { + headReader, err = csv_module.CreateReaderAndGuessDelimiter(strings.NewReader(c.head)) + if err != nil { + t.Errorf("CreateReaderAndGuessDelimiter failed: %s", err) + } + } + + result, err := CreateCsvDiff(diff.Files[0], baseReader, headReader) + assert.NoError(t, err) + assert.Len(t, result, 1, "case %d: should be one section", n) + + section := result[0] + assert.Len(t, section.Rows, len(c.cells), "case %d: should be %d rows", n, len(c.cells)) + + for i, row := range section.Rows { + assert.Len(t, row.Cells, 4, "case %d: row %d should have four cells", n, i) + for j, cell := range row.Cells { + assert.Equal(t, c.cells[i][j], cell.Type, "case %d: row %d cell %d should be equal", n, i, j) + } + } + } +} diff --git a/templates/repo/diff/csv_diff.tmpl b/templates/repo/diff/csv_diff.tmpl index b86e9d2a72277..50750b1ab3eb8 100644 --- a/templates/repo/diff/csv_diff.tmpl +++ b/templates/repo/diff/csv_diff.tmpl @@ -12,7 +12,9 @@ {{if and (eq $i 0) (eq $j 0)}} {{.RowIdx}} {{range $j, $cell := $row.Cells}} - {{if eq $cell.Type 2}} + {{if not $cell}} + + {{else if eq $cell.Type 2}} {{.LeftCell}} {{.RightCell}} {{else if eq $cell.Type 3}} {{.LeftCell}} @@ -25,7 +27,9 @@ {{else}} {{if .RowIdx}}{{.RowIdx}}{{end}} {{range $j, $cell := $row.Cells}} - {{if eq $cell.Type 2}} + {{if not $cell}} + + {{else if eq $cell.Type 2}} {{.LeftCell}} {{.RightCell}} {{else if eq $cell.Type 3}} {{.LeftCell}} From 201645505a3e09a586743d2b55173a883479590e Mon Sep 17 00:00:00 2001 From: Richard Mahn Date: Sun, 12 Sep 2021 04:10:18 -0400 Subject: [PATCH 02/17] Reworks CSV diff --- routers/web/repo/view.go | 2 +- services/gitdiff/csv.go | 49 ++++++++++++-------- services/gitdiff/csv_test.go | 74 ++++++++++++++++++++++++++++--- templates/repo/diff/csv_diff.tmpl | 4 +- 4 files changed, 100 insertions(+), 29 deletions(-) diff --git a/routers/web/repo/view.go b/routers/web/repo/view.go index 2c703fc1aff40..d4f21b564ec32 100644 --- a/routers/web/repo/view.go +++ b/routers/web/repo/view.go @@ -518,7 +518,7 @@ func renderFile(ctx *context.Context, entry *git.TreeEntry, treeLink, rawLink st Metas: ctx.Repo.Repository.ComposeDocumentMetas(), GitRepo: ctx.Repo.GitRepo, }, rd, &result) - if err != nil { + if err != nil && err != io.EOF { ctx.ServerError("Render", err) return } diff --git a/services/gitdiff/csv.go b/services/gitdiff/csv.go index 082d4bbd52ddf..20db90cbc034d 100644 --- a/services/gitdiff/csv.go +++ b/services/gitdiff/csv.go @@ -70,7 +70,7 @@ func createCsvReader(reader *csv.Reader, bufferRowCount int) (*csvReader, error) // GetRow gets a row from the buffer if present or advances the reader to the requested row. On the end of the file only nil gets returned. func (csv *csvReader) GetRow(row int) ([]string, error) { - if row < len(csv.buffer) { + if row < len(csv.buffer) && row >= 0 { return csv.buffer[row], nil } if csv.eof { @@ -131,7 +131,11 @@ func createCsvDiffSingle(reader *csv.Reader, celltype TableDiffCellType) ([]*Tab } cells := make([]*TableDiffCell, len(row)) for j := 0; j < len(row); j++ { - cells[j] = &TableDiffCell{LeftCell: row[j], Type: celltype} + if celltype == TableDiffCellDel { + cells[j] = &TableDiffCell{LeftCell: row[j], Type: celltype} + } else { + cells[j] = &TableDiffCell{RightCell: row[j], Type: celltype} + } } rows = append(rows, &TableDiffRow{RowIdx: i, Cells: cells}) i++ @@ -174,32 +178,39 @@ func createCsvDiff(diffFile *DiffFile, baseReader *csv.Reader, headReader *csv.R } for i := 0; i < len(a2b); i++ { - acell, _ := getCell(arow, i) + acell, ae := getCell(arow, i) + if a2b[i] == unmappedColumn { cells[i] = &TableDiffCell{LeftCell: acell, Type: TableDiffCellDel} } else { - bcell, _ := getCell(brow, a2b[i]) - - celltype := TableDiffCellChanged - if acell == bcell { + bcell, be := getCell(brow, a2b[i]) + + var celltype TableDiffCellType + if ae != nil { + celltype = TableDiffCellAdd + } else if be != nil { + celltype = TableDiffCellDel + } else if acell == bcell { celltype = TableDiffCellEqual + } else { + celltype = TableDiffCellChanged } cells[i] = &TableDiffCell{LeftCell: acell, RightCell: bcell, Type: celltype} } } + cells_index := 0 for i := 0; i < len(b2a); i++ { if b2a[i] == unmappedColumn { bcell, _ := getCell(brow, i) - cells_index := i - if len(a2b) >= i+1 && a2b[i] <= i { - cells_index = i + 1 - } if cells[cells_index] != nil && len(cells) >= cells_index+1 { copy(cells[cells_index+1:], cells[cells_index:]) } - cells[cells_index] = &TableDiffCell{LeftCell: bcell, Type: TableDiffCellAdd} + cells[cells_index] = &TableDiffCell{RightCell: bcell, Type: TableDiffCellAdd} + } else if cells_index < b2a[i] { + cells_index = b2a[i] } + cells_index += 1 } return &TableDiffRow{RowIdx: bline, Cells: cells}, nil @@ -282,26 +293,26 @@ func getColumnMapping(a *csvReader, b *csvReader) ([]int, []int) { // tryMapColumnsByContent tries to map missing columns by the content of the first lines. func tryMapColumnsByContent(a *csvReader, a2b []int, b *csvReader, b2a []int) { - start := 0 for i := 0; i < len(a2b); i++ { - if a2b[i] == unmappedColumn { - if b2a[start] == unmappedColumn { + b_start := 0 + for a2b[i] == unmappedColumn && b_start < len(b2a) { + if b2a[b_start] == unmappedColumn { rows := util.Min(maxRowsToInspect, util.Max(0, util.Min(len(a.buffer), len(b.buffer))-1)) same := 0 for j := 1; j <= rows; j++ { acell, ea := getCell(a.buffer[j], i) - bcell, eb := getCell(b.buffer[j], start+1) + bcell, eb := getCell(b.buffer[j], b_start) if ea == nil && eb == nil && acell == bcell { same++ } } if (float32(same) / float32(rows)) > minRatioToMatch { - a2b[i] = start + 1 - b2a[start+1] = i + a2b[i] = b_start + b2a[b_start] = i } } + b_start += 1 } - start = a2b[i] } } diff --git a/services/gitdiff/csv_test.go b/services/gitdiff/csv_test.go index cd9b08981a416..f29c06f067fd4 100644 --- a/services/gitdiff/csv_test.go +++ b/services/gitdiff/csv_test.go @@ -21,7 +21,7 @@ func TestCSVDiff(t *testing.T) { head string cells [][2]TableDiffCellType }{ - // case 0 + // case 0 - initial commit of a csv { diff: `diff --git a/unittest.csv b/unittest.csv --- a/unittest.csv @@ -33,7 +33,7 @@ func TestCSVDiff(t *testing.T) { head: "col1,col2\na,a", cells: [][2]TableDiffCellType{{TableDiffCellAdd, TableDiffCellAdd}, {TableDiffCellAdd, TableDiffCellAdd}}, }, - // case 1 + // case 1 - adding 1 row at end { diff: `diff --git a/unittest.csv b/unittest.csv --- a/unittest.csv @@ -45,9 +45,9 @@ func TestCSVDiff(t *testing.T) { +b,b`, base: "col1,col2\na,a", head: "col1,col2\na,a\nb,b", - cells: [][2]TableDiffCellType{{TableDiffCellEqual, TableDiffCellEqual}, {TableDiffCellEqual, TableDiffCellEqual}, {TableDiffCellAdd, TableDiffCellAdd}}, + cells: [][2]TableDiffCellType{{TableDiffCellEqual, TableDiffCellEqual}, {TableDiffCellEqual, TableDiffCellEqual}, {TableDiffCellChanged, TableDiffCellChanged}}, }, - // case 2 + // case 2 - row deleted { diff: `diff --git a/unittest.csv b/unittest.csv --- a/unittest.csv @@ -60,7 +60,7 @@ func TestCSVDiff(t *testing.T) { head: "col1,col2\nb,b", cells: [][2]TableDiffCellType{{TableDiffCellEqual, TableDiffCellEqual}, {TableDiffCellDel, TableDiffCellDel}, {TableDiffCellEqual, TableDiffCellEqual}}, }, - // case 3 + // case 3 - row changed { diff: `diff --git a/unittest.csv b/unittest.csv --- a/unittest.csv @@ -73,7 +73,7 @@ func TestCSVDiff(t *testing.T) { head: "col1,col2\nb,c", cells: [][2]TableDiffCellType{{TableDiffCellEqual, TableDiffCellEqual}, {TableDiffCellEqual, TableDiffCellChanged}}, }, - // case 4 + // case 4 - all deleted { diff: `diff --git a/unittest.csv b/unittest.csv --- a/unittest.csv @@ -142,7 +142,7 @@ func TestCSVDiffHeadingChange(t *testing.T) { a,b,c`, base: "col1,col2,col3\na,b,c", head: "cola,col2,col3\na,b,c", - cells: [][4]TableDiffCellType{{TableDiffCellDel, TableDiffCellAdd, TableDiffCellEqual, TableDiffCellEqual}, {TableDiffCellDel, TableDiffCellAdd, TableDiffCellEqual, TableDiffCellEqual}}, + cells: [][4]TableDiffCellType{{TableDiffCellAdd, TableDiffCellDel, TableDiffCellEqual, TableDiffCellEqual}, {TableDiffCellAdd, TableDiffCellDel, TableDiffCellEqual, TableDiffCellEqual}}, }, // case 1 - inserts a column after first, deletes last column { @@ -210,3 +210,63 @@ func TestCSVDiffHeadingChange(t *testing.T) { } } } + +func TestCSVDiffMultipleHeadingChanges(t *testing.T) { + var cases = []struct { + diff string + base string + head string + cells [][5]TableDiffCellType + }{ + // case 0 - two columns deleted, 2 added + { + diff: `diff --git a/unittest.csv b/unittest.csv +--- a/unittest.csv ++++ b/unittest.csv +@@ -1,2 +1,2 @@ +-col1,col2,col +-a,b,c ++col3,col4,col5 ++c,d,e`, + base: "col1,col2,col3\na,b,c", + head: "col3,col4,col5\nc,d,e", + cells: [][5]TableDiffCellType{{TableDiffCellDel, TableDiffCellDel, TableDiffCellEqual, TableDiffCellAdd, TableDiffCellAdd}, {TableDiffCellDel, TableDiffCellDel, TableDiffCellEqual, TableDiffCellAdd, TableDiffCellAdd}}, + }, + } + + for n, c := range cases { + diff, err := ParsePatch(setting.Git.MaxGitDiffLines, setting.Git.MaxGitDiffLineCharacters, setting.Git.MaxGitDiffFiles, strings.NewReader(c.diff)) + if err != nil { + t.Errorf("ParsePatch failed: %s", err) + } + + var baseReader *csv.Reader + if len(c.base) > 0 { + baseReader, err = csv_module.CreateReaderAndGuessDelimiter(strings.NewReader(c.base)) + if err != nil { + t.Errorf("CreateReaderAndGuessDelimiter failed: %s", err) + } + } + var headReader *csv.Reader + if len(c.head) > 0 { + headReader, err = csv_module.CreateReaderAndGuessDelimiter(strings.NewReader(c.head)) + if err != nil { + t.Errorf("CreateReaderAndGuessDelimiter failed: %s", err) + } + } + + result, err := CreateCsvDiff(diff.Files[0], baseReader, headReader) + assert.NoError(t, err) + assert.Len(t, result, 1, "case %d: should be one section", n) + + section := result[0] + assert.Len(t, section.Rows, len(c.cells), "case %d: should be %d rows", n, len(c.cells)) + + for i, row := range section.Rows { + assert.Len(t, row.Cells, 5, "case %d: row %d should have five cells", n, i) + for j, cell := range row.Cells { + assert.Equal(t, c.cells[i][j], cell.Type, "case %d: row %d cell %d should be equal", n, i, j) + } + } + } +} diff --git a/templates/repo/diff/csv_diff.tmpl b/templates/repo/diff/csv_diff.tmpl index 50750b1ab3eb8..db71d5de6cbca 100644 --- a/templates/repo/diff/csv_diff.tmpl +++ b/templates/repo/diff/csv_diff.tmpl @@ -17,7 +17,7 @@ {{else if eq $cell.Type 2}} {{.LeftCell}} {{.RightCell}} {{else if eq $cell.Type 3}} - {{.LeftCell}} + {{.RightCell}} {{else if eq $cell.Type 4}} {{.LeftCell}} {{else}} @@ -32,7 +32,7 @@ {{else if eq $cell.Type 2}} {{.LeftCell}} {{.RightCell}} {{else if eq $cell.Type 3}} - {{.LeftCell}} + {{.RightCell}} {{else if eq $cell.Type 4}} {{.LeftCell}} {{else}} From 81799d2618fde1eada61d40278e7c4da392361fe Mon Sep 17 00:00:00 2001 From: Richard Mahn Date: Sun, 12 Sep 2021 04:12:48 -0400 Subject: [PATCH 03/17] Fixes tests --- services/gitdiff/csv_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services/gitdiff/csv_test.go b/services/gitdiff/csv_test.go index f29c06f067fd4..82673acf59c67 100644 --- a/services/gitdiff/csv_test.go +++ b/services/gitdiff/csv_test.go @@ -45,7 +45,7 @@ func TestCSVDiff(t *testing.T) { +b,b`, base: "col1,col2\na,a", head: "col1,col2\na,a\nb,b", - cells: [][2]TableDiffCellType{{TableDiffCellEqual, TableDiffCellEqual}, {TableDiffCellEqual, TableDiffCellEqual}, {TableDiffCellChanged, TableDiffCellChanged}}, + cells: [][2]TableDiffCellType{{TableDiffCellEqual, TableDiffCellEqual}, {TableDiffCellEqual, TableDiffCellEqual}, {TableDiffCellAdd, TableDiffCellAdd}}, }, // case 2 - row deleted { From 58a059a7d58ddc9c8f1a2c15567835f17e15df84 Mon Sep 17 00:00:00 2001 From: Richard Mahn Date: Sun, 12 Sep 2021 04:18:19 -0400 Subject: [PATCH 04/17] Fixes linting --- services/gitdiff/csv.go | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/services/gitdiff/csv.go b/services/gitdiff/csv.go index 20db90cbc034d..8b06cd041b1ed 100644 --- a/services/gitdiff/csv.go +++ b/services/gitdiff/csv.go @@ -199,18 +199,18 @@ func createCsvDiff(diffFile *DiffFile, baseReader *csv.Reader, headReader *csv.R cells[i] = &TableDiffCell{LeftCell: acell, RightCell: bcell, Type: celltype} } } - cells_index := 0 + cellsIndex := 0 for i := 0; i < len(b2a); i++ { if b2a[i] == unmappedColumn { bcell, _ := getCell(brow, i) - if cells[cells_index] != nil && len(cells) >= cells_index+1 { - copy(cells[cells_index+1:], cells[cells_index:]) + if cells[cellsIndex] != nil && len(cells) >= cellsIndex+1 { + copy(cells[cellsIndex+1:], cells[cellsIndex:]) } - cells[cells_index] = &TableDiffCell{RightCell: bcell, Type: TableDiffCellAdd} - } else if cells_index < b2a[i] { - cells_index = b2a[i] + cells[cellsIndex] = &TableDiffCell{RightCell: bcell, Type: TableDiffCellAdd} + } else if cellsIndex < b2a[i] { + cellsIndex = b2a[i] } - cells_index += 1 + cellsIndex += 1 } return &TableDiffRow{RowIdx: bline, Cells: cells}, nil @@ -294,24 +294,24 @@ func getColumnMapping(a *csvReader, b *csvReader) ([]int, []int) { // tryMapColumnsByContent tries to map missing columns by the content of the first lines. func tryMapColumnsByContent(a *csvReader, a2b []int, b *csvReader, b2a []int) { for i := 0; i < len(a2b); i++ { - b_start := 0 - for a2b[i] == unmappedColumn && b_start < len(b2a) { - if b2a[b_start] == unmappedColumn { + bStart := 0 + for a2b[i] == unmappedColumn && bStart < len(b2a) { + if b2a[bStart] == unmappedColumn { rows := util.Min(maxRowsToInspect, util.Max(0, util.Min(len(a.buffer), len(b.buffer))-1)) same := 0 for j := 1; j <= rows; j++ { acell, ea := getCell(a.buffer[j], i) - bcell, eb := getCell(b.buffer[j], b_start) + bcell, eb := getCell(b.buffer[j], bStart) if ea == nil && eb == nil && acell == bcell { same++ } } if (float32(same) / float32(rows)) > minRatioToMatch { - a2b[i] = b_start - b2a[b_start] = i + a2b[i] = bStart + b2a[bStart] = i } } - b_start += 1 + bStart += 1 } } } From b510356cb55c4b1548da6c3f137d74bf44f5dc60 Mon Sep 17 00:00:00 2001 From: Richard Mahn Date: Sun, 12 Sep 2021 04:21:37 -0400 Subject: [PATCH 05/17] Fixes linting --- services/gitdiff/csv.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/services/gitdiff/csv.go b/services/gitdiff/csv.go index 8b06cd041b1ed..ece919323dc7d 100644 --- a/services/gitdiff/csv.go +++ b/services/gitdiff/csv.go @@ -210,7 +210,7 @@ func createCsvDiff(diffFile *DiffFile, baseReader *csv.Reader, headReader *csv.R } else if cellsIndex < b2a[i] { cellsIndex = b2a[i] } - cellsIndex += 1 + cellsIndex++ } return &TableDiffRow{RowIdx: bline, Cells: cells}, nil @@ -311,7 +311,7 @@ func tryMapColumnsByContent(a *csvReader, a2b []int, b *csvReader, b2a []int) { b2a[bStart] = i } } - bStart += 1 + bStart++ } } } From 5abe6811783368a41a47aa04b36f4b1207851d29 Mon Sep 17 00:00:00 2001 From: Richard Mahn Date: Sun, 12 Sep 2021 04:36:45 -0400 Subject: [PATCH 06/17] Better variable names and camelcase --- services/gitdiff/csv.go | 36 ++++++++++++++++++------------------ 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/services/gitdiff/csv.go b/services/gitdiff/csv.go index ece919323dc7d..7957a1d4f2b49 100644 --- a/services/gitdiff/csv.go +++ b/services/gitdiff/csv.go @@ -178,35 +178,35 @@ func createCsvDiff(diffFile *DiffFile, baseReader *csv.Reader, headReader *csv.R } for i := 0; i < len(a2b); i++ { - acell, ae := getCell(arow, i) + aCell, aErr := getCell(arow, i) if a2b[i] == unmappedColumn { - cells[i] = &TableDiffCell{LeftCell: acell, Type: TableDiffCellDel} + cells[i] = &TableDiffCell{LeftCell: aCell, Type: TableDiffCellDel} } else { - bcell, be := getCell(brow, a2b[i]) - - var celltype TableDiffCellType - if ae != nil { - celltype = TableDiffCellAdd - } else if be != nil { - celltype = TableDiffCellDel - } else if acell == bcell { - celltype = TableDiffCellEqual + bCell, bErr := getCell(brow, a2b[i]) + + var cellType TableDiffCellType + if aErr != nil { + cellType = TableDiffCellAdd + } else if bErr != nil { + cellType = TableDiffCellDel + } else if aCell == bCell { + cellType = TableDiffCellEqual } else { - celltype = TableDiffCellChanged + cellType = TableDiffCellChanged } - cells[i] = &TableDiffCell{LeftCell: acell, RightCell: bcell, Type: celltype} + cells[i] = &TableDiffCell{LeftCell: aCell, RightCell: bCell, Type: cellType} } } cellsIndex := 0 for i := 0; i < len(b2a); i++ { if b2a[i] == unmappedColumn { - bcell, _ := getCell(brow, i) + bCell, _ := getCell(brow, i) if cells[cellsIndex] != nil && len(cells) >= cellsIndex+1 { copy(cells[cellsIndex+1:], cells[cellsIndex:]) } - cells[cellsIndex] = &TableDiffCell{RightCell: bcell, Type: TableDiffCellAdd} + cells[cellsIndex] = &TableDiffCell{RightCell: bCell, Type: TableDiffCellAdd} } else if cellsIndex < b2a[i] { cellsIndex = b2a[i] } @@ -300,9 +300,9 @@ func tryMapColumnsByContent(a *csvReader, a2b []int, b *csvReader, b2a []int) { rows := util.Min(maxRowsToInspect, util.Max(0, util.Min(len(a.buffer), len(b.buffer))-1)) same := 0 for j := 1; j <= rows; j++ { - acell, ea := getCell(a.buffer[j], i) - bcell, eb := getCell(b.buffer[j], bStart) - if ea == nil && eb == nil && acell == bcell { + aCell, aErr := getCell(a.buffer[j], i) + bCell, bErr := getCell(b.buffer[j], bStart) + if aErr == nil && bErr == nil && aCell == bCell { same++ } } From 9f70afe774bdbc4802e8bcd8ce0678b177faf6f2 Mon Sep 17 00:00:00 2001 From: Gitea Date: Sun, 12 Sep 2021 17:34:45 -0600 Subject: [PATCH 07/17] moves checking for EOF in the csv Renderer --- modules/markup/csv/csv.go | 6 ++++++ routers/web/repo/view.go | 2 +- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/modules/markup/csv/csv.go b/modules/markup/csv/csv.go index 8a4df8951154a..2cf4c7eb9007c 100644 --- a/modules/markup/csv/csv.go +++ b/modules/markup/csv/csv.go @@ -89,6 +89,9 @@ func (Renderer) Render(ctx *markup.RenderContext, input io.Reader, output io.Wri // FIXME: don't read all to memory rawBytes, err := ioutil.ReadAll(input) if err != nil { + if err == io.EOF { + return nil + } return err } @@ -105,6 +108,9 @@ func (Renderer) Render(ctx *markup.RenderContext, input io.Reader, output io.Wri rd, err := csv.CreateReaderAndGuessDelimiter(bytes.NewReader(rawBytes)) if err != nil { + if err == io.EOF { + return nil + } return err } diff --git a/routers/web/repo/view.go b/routers/web/repo/view.go index d4f21b564ec32..2c703fc1aff40 100644 --- a/routers/web/repo/view.go +++ b/routers/web/repo/view.go @@ -518,7 +518,7 @@ func renderFile(ctx *context.Context, entry *git.TreeEntry, treeLink, rawLink st Metas: ctx.Repo.Repository.ComposeDocumentMetas(), GitRepo: ctx.Repo.GitRepo, }, rd, &result) - if err != nil && err != io.EOF { + if err != nil { ctx.ServerError("Render", err) return } From aa526e5bc3815bd6b8ad222198a11901d5587327 Mon Sep 17 00:00:00 2001 From: Richard Mahn Date: Mon, 13 Sep 2021 18:49:37 -0400 Subject: [PATCH 08/17] Fixes EOF problem --- modules/csv/csv.go | 2 +- modules/markup/csv/csv.go | 10 ++-------- 2 files changed, 3 insertions(+), 9 deletions(-) diff --git a/modules/csv/csv.go b/modules/csv/csv.go index 35486edd8821d..5b3e810d428ca 100644 --- a/modules/csv/csv.go +++ b/modules/csv/csv.go @@ -30,7 +30,7 @@ func CreateReader(input io.Reader, delimiter rune) *stdcsv.Reader { func CreateReaderAndGuessDelimiter(rd io.Reader) (*stdcsv.Reader, error) { var data = make([]byte, 1e4) size, err := rd.Read(data) - if err != nil { + if err != nil && err != io.EOF { return nil, err } diff --git a/modules/markup/csv/csv.go b/modules/markup/csv/csv.go index 2cf4c7eb9007c..3a360e92578bb 100644 --- a/modules/markup/csv/csv.go +++ b/modules/markup/csv/csv.go @@ -88,10 +88,7 @@ func (Renderer) Render(ctx *markup.RenderContext, input io.Reader, output io.Wri // FIXME: don't read all to memory rawBytes, err := ioutil.ReadAll(input) - if err != nil { - if err == io.EOF { - return nil - } + if err != nil && err != io.EOF { return err } @@ -107,10 +104,7 @@ func (Renderer) Render(ctx *markup.RenderContext, input io.Reader, output io.Wri } rd, err := csv.CreateReaderAndGuessDelimiter(bytes.NewReader(rawBytes)) - if err != nil { - if err == io.EOF { - return nil - } + if err != nil && err != io.EOF { return err } From 01671aa70b2ea918cf518bbd8ea6a77aba8301db Mon Sep 17 00:00:00 2001 From: Richard Mahn Date: Mon, 13 Sep 2021 20:24:11 -0400 Subject: [PATCH 09/17] Better error handling and knowing if cell exists --- services/gitdiff/csv.go | 64 +++++++++++++++++++++++++++++++++++------ 1 file changed, 55 insertions(+), 9 deletions(-) diff --git a/services/gitdiff/csv.go b/services/gitdiff/csv.go index 7957a1d4f2b49..214cdafe0949b 100644 --- a/services/gitdiff/csv.go +++ b/services/gitdiff/csv.go @@ -53,6 +53,9 @@ type csvReader struct { eof bool } +// ErrorUndefinedCell is for when a row, column coordinates do not exist in the CSV +var ErrorUndefinedCell = errors.New("undefined cell") + // createCsvReader creates a csvReader and fills the buffer func createCsvReader(reader *csv.Reader, bufferRowCount int) (*csvReader, error) { csv := &csvReader{reader: reader} @@ -178,35 +181,78 @@ func createCsvDiff(diffFile *DiffFile, baseReader *csv.Reader, headReader *csv.R } for i := 0; i < len(a2b); i++ { - aCell, aErr := getCell(arow, i) + var aCell string + aIsUndefined := false + if aline > 0 { + if cell, err := getCell(arow, i); err != nil { + if err != ErrorUndefinedCell { + return nil, err + } + aIsUndefined = true + } else { + aCell = cell + } + } else { + aIsUndefined = true + } if a2b[i] == unmappedColumn { cells[i] = &TableDiffCell{LeftCell: aCell, Type: TableDiffCellDel} } else { - bCell, bErr := getCell(brow, a2b[i]) - + var bCell string + bIsUndefined := false + if bline > 0 { + if cell, err := getCell(brow, a2b[i]); err != nil { + if err != ErrorUndefinedCell { + return nil, err + } + bIsUndefined = true + } else { + bCell = cell + } + } else { + bIsUndefined = true + } var cellType TableDiffCellType - if aErr != nil { + if aIsUndefined && bIsUndefined { cellType = TableDiffCellAdd - } else if bErr != nil { + } else if bIsUndefined { cellType = TableDiffCellDel } else if aCell == bCell { cellType = TableDiffCellEqual } else { cellType = TableDiffCellChanged } - cells[i] = &TableDiffCell{LeftCell: aCell, RightCell: bCell, Type: cellType} } } cellsIndex := 0 for i := 0; i < len(b2a); i++ { if b2a[i] == unmappedColumn { - bCell, _ := getCell(brow, i) + var bCell string + bIsUndefined := false + if bline > 0 { + if cell, err := getCell(brow, i); err != nil { + if err != ErrorUndefinedCell { + return nil, err + } + bIsUndefined = true + } else { + bCell = cell + } + } else { + bIsUndefined = true + } if cells[cellsIndex] != nil && len(cells) >= cellsIndex+1 { copy(cells[cellsIndex+1:], cells[cellsIndex:]) } - cells[cellsIndex] = &TableDiffCell{RightCell: bCell, Type: TableDiffCellAdd} + var cellType TableDiffCellType + if bIsUndefined { + cellType = TableDiffCellDel + } else { + cellType = TableDiffCellAdd + } + cells[cellsIndex] = &TableDiffCell{RightCell: bCell, Type: cellType} } else if cellsIndex < b2a[i] { cellsIndex = b2a[i] } @@ -321,7 +367,7 @@ func getCell(row []string, column int) (string, error) { if column < len(row) { return row[column], nil } - return "", errors.New("Undefined column") + return "", ErrorUndefinedCell } // countUnmappedColumns returns the count of unmapped columns. From 535a86c253ffda29bd3e859a994fde670432ae63 Mon Sep 17 00:00:00 2001 From: Richard Mahn Date: Tue, 14 Sep 2021 00:37:33 -0400 Subject: [PATCH 10/17] Fixes tests --- services/gitdiff/csv.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services/gitdiff/csv.go b/services/gitdiff/csv.go index 214cdafe0949b..9d0d4fab4e8a2 100644 --- a/services/gitdiff/csv.go +++ b/services/gitdiff/csv.go @@ -214,7 +214,7 @@ func createCsvDiff(diffFile *DiffFile, baseReader *csv.Reader, headReader *csv.R bIsUndefined = true } var cellType TableDiffCellType - if aIsUndefined && bIsUndefined { + if aIsUndefined && !bIsUndefined { cellType = TableDiffCellAdd } else if bIsUndefined { cellType = TableDiffCellDel From 070873367eb515f93bd61d11200686290fb2551a Mon Sep 17 00:00:00 2001 From: Richard Mahn Date: Tue, 14 Sep 2021 07:16:10 -0400 Subject: [PATCH 11/17] removes io.EOF check for ioutil.ReadAll() --- modules/markup/csv/csv.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/markup/csv/csv.go b/modules/markup/csv/csv.go index 3a360e92578bb..0277a71e1643f 100644 --- a/modules/markup/csv/csv.go +++ b/modules/markup/csv/csv.go @@ -88,7 +88,7 @@ func (Renderer) Render(ctx *markup.RenderContext, input io.Reader, output io.Wri // FIXME: don't read all to memory rawBytes, err := ioutil.ReadAll(input) - if err != nil && err != io.EOF { + if err != nil { return err } From ba85dfd4cf969cd82c77ff98ac8a9911025e2c05 Mon Sep 17 00:00:00 2001 From: Richard Mahn Date: Tue, 14 Sep 2021 07:29:22 -0400 Subject: [PATCH 12/17] Better EOF handling --- modules/csv/csv.go | 5 ++++- modules/markup/csv/csv.go | 2 +- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/modules/csv/csv.go b/modules/csv/csv.go index 5b3e810d428ca..30698830a478d 100644 --- a/modules/csv/csv.go +++ b/modules/csv/csv.go @@ -30,7 +30,10 @@ func CreateReader(input io.Reader, delimiter rune) *stdcsv.Reader { func CreateReaderAndGuessDelimiter(rd io.Reader) (*stdcsv.Reader, error) { var data = make([]byte, 1e4) size, err := rd.Read(data) - if err != nil && err != io.EOF { + if err != nil { + if err == io.EOF { + return CreateReader(bytes.NewReader([]byte{}), rune(',')), nil + } return nil, err } diff --git a/modules/markup/csv/csv.go b/modules/markup/csv/csv.go index 0277a71e1643f..8a4df8951154a 100644 --- a/modules/markup/csv/csv.go +++ b/modules/markup/csv/csv.go @@ -104,7 +104,7 @@ func (Renderer) Render(ctx *markup.RenderContext, input io.Reader, output io.Wri } rd, err := csv.CreateReaderAndGuessDelimiter(bytes.NewReader(rawBytes)) - if err != nil && err != io.EOF { + if err != nil { return err } From 99ac2b719f8afc04d4e6ce66bd2ba3e8025fbff7 Mon Sep 17 00:00:00 2001 From: Richard Mahn Date: Thu, 23 Sep 2021 11:27:32 -0400 Subject: [PATCH 13/17] Refactor of CSV column diff --- services/gitdiff/csv.go | 281 +++++++++++++---------- services/gitdiff/csv_test.go | 202 +++++++--------- templates/repo/diff/csv_diff.tmpl | 8 + web_src/less/_base.less | 2 + web_src/less/_repository.less | 6 + web_src/less/themes/theme-arc-green.less | 2 + 6 files changed, 256 insertions(+), 245 deletions(-) diff --git a/services/gitdiff/csv.go b/services/gitdiff/csv.go index 9d0d4fab4e8a2..63640d8e2d645 100644 --- a/services/gitdiff/csv.go +++ b/services/gitdiff/csv.go @@ -21,10 +21,12 @@ type TableDiffCellType uint8 // TableDiffCellType possible values. const ( - TableDiffCellEqual TableDiffCellType = iota + 1 + TableDiffCellUnchanged TableDiffCellType = iota + 1 TableDiffCellChanged TableDiffCellAdd TableDiffCellDel + TableDiffCellMovedUnchanged + TableDiffCellMovedChanged ) // TableDiffCell represents a cell of a TableDiffRow @@ -148,216 +150,255 @@ func createCsvDiffSingle(reader *csv.Reader, celltype TableDiffCellType) ([]*Tab } func createCsvDiff(diffFile *DiffFile, baseReader *csv.Reader, headReader *csv.Reader) ([]*TableDiffSection, error) { - a, err := createCsvReader(baseReader, maxRowsToInspect) + // Given the baseReader and headReader, we are going to create CSV Reader for each, baseCSVReader and b respectively + baseCSVReader, err := createCsvReader(baseReader, maxRowsToInspect) if err != nil { return nil, err } - - b, err := createCsvReader(headReader, maxRowsToInspect) + headCSVReader, err := createCsvReader(headReader, maxRowsToInspect) if err != nil { return nil, err } - a2b, b2a := getColumnMapping(a, b) + // Initalizing the mappings of base to head (base2HeadColMap) and head to base (head2BaseColMap) columns + base2HeadColMap, head2BaseColMap := getColumnMapping(baseCSVReader, headCSVReader) - columns := len(a2b) + countUnmappedColumns(b2a) - if len(a2b) < len(b2a) { - columns = len(b2a) + countUnmappedColumns(a2b) + // Determines how many cols there will be in the diff table, which includes deleted columsn from base and added columns to base + numDiffTableCols := len(base2HeadColMap) + countUnmappedColumns(head2BaseColMap) + if len(base2HeadColMap) < len(head2BaseColMap) { + numDiffTableCols = len(head2BaseColMap) + countUnmappedColumns(base2HeadColMap) } - createDiffRow := func(aline int, bline int) (*TableDiffRow, error) { - cells := make([]*TableDiffCell, columns) - - arow, err := a.GetRow(aline - 1) - if err != nil { - return nil, err + // createDiffTableRow takes the row # of the `a` line and `b` line of a diff (starting from 1), 0 if the line doesn't exist (undefined) + // in the base or head respectively. + // Returns a TableDiffRow which has the row index + createDiffTableRow := func(aLineNum int, bLineNum int) (*TableDiffRow, error) { + // diffTableCells is a row of the diff table. It will have a cells for added, deleted, changed, and unchanged content, thus either + // the same size as the head table or bigger + diffTableCells := make([]*TableDiffCell, numDiffTableCols) + var aRow *[]string + if aLineNum > 0 { + row, err := baseCSVReader.GetRow(aLineNum - 1) + if err != nil { + return nil, err + } + aRow = &row } - brow, err := b.GetRow(bline - 1) - if err != nil { - return nil, err + var bRow *[]string + if bLineNum > 0 { + row, err := headCSVReader.GetRow(bLineNum - 1) + if err != nil { + return nil, err + } + bRow = &row + } + if bRow == nil { + } else { + if len(*bRow) == 0 { + } } - if len(arow) == 0 && len(brow) == 0 { + if aRow == nil && bRow == nil { + // No content return nil, nil } - - for i := 0; i < len(a2b); i++ { - var aCell string - aIsUndefined := false - if aline > 0 { - if cell, err := getCell(arow, i); err != nil { + // First we loop through the head columns and place them in the diff table as they appear, both existing columsn and new columns + for i := 0; i < len(head2BaseColMap); i++ { + var bCell *string // Pointer to text of the b line (head), if nil the cell doesn't exist in the head (deleted row in the base) + if bRow != nil { + // is an added column and the `b` row exists so the cell should exist, but still will check for undefined cell error + if cell, err := getCell(*bRow, i); err != nil { if err != ErrorUndefinedCell { return nil, err } - aIsUndefined = true } else { - aCell = cell + bCell = &cell } - } else { - aIsUndefined = true } - if a2b[i] == unmappedColumn { - cells[i] = &TableDiffCell{LeftCell: aCell, Type: TableDiffCellDel} + var diffCell TableDiffCell + addedCols := 0 + if head2BaseColMap[i] == unmappedColumn { + // This column doesn't exist in the base, so we know it is a new column. + var cell string + // Col exists in the head, but we might be displaying a deleted row, so need to check if bCell exists, and if it does, make cell its value + if bCell != nil { + cell = *bCell + } + diffCell = TableDiffCell{RightCell: cell, Type: TableDiffCellAdd} + addedCols++ } else { - var bCell string - bIsUndefined := false - if bline > 0 { - if cell, err := getCell(brow, a2b[i]); err != nil { + // The column still exists in the head, but we need to figure out if the row exists as well (changed text) or if it is a new row in head + var aCell *string // Pointer to the texst of the a line (base), if nil the cell doesn't exist in the base (added row in head) + if aRow != nil { + // Get the cell contents of the 'a' row. Should exist, but just in case we handle the error and make the contents and empty string + if cell, err := getCell(*aRow, head2BaseColMap[i]); err != nil { if err != ErrorUndefinedCell { return nil, err } - bIsUndefined = true } else { - bCell = cell + aCell = &cell } - } else { - bIsUndefined = true } - var cellType TableDiffCellType - if aIsUndefined && !bIsUndefined { - cellType = TableDiffCellAdd - } else if bIsUndefined { - cellType = TableDiffCellDel - } else if aCell == bCell { - cellType = TableDiffCellEqual + if bCell == nil { + // both a & b have the column, but not the row (deleted) + diffCell = TableDiffCell{LeftCell: *aCell, Type: TableDiffCellDel} + } else if aCell == nil { + // both a & b have the column, but not the row (added) + diffCell = TableDiffCell{RightCell: *bCell, Type: TableDiffCellAdd} } else { - cellType = TableDiffCellChanged + var cellType TableDiffCellType + if head2BaseColMap[i] > i-addedCols { + if *aCell != *bCell { + cellType = TableDiffCellMovedChanged + } else { + cellType = TableDiffCellMovedUnchanged + } + } else { + if *aCell != *bCell { + cellType = TableDiffCellChanged + } else { + cellType = TableDiffCellUnchanged + } + } + diffCell = TableDiffCell{LeftCell: *aCell, RightCell: *bCell, Type: cellType} } - cells[i] = &TableDiffCell{LeftCell: aCell, RightCell: bCell, Type: cellType} } + diffTableCells[i] = &diffCell } - cellsIndex := 0 - for i := 0; i < len(b2a); i++ { - if b2a[i] == unmappedColumn { - var bCell string - bIsUndefined := false - if bline > 0 { - if cell, err := getCell(brow, i); err != nil { + + // Now loop through the base columns to find the unmmapped (deleted) columns in the base + baseOffset := 0 + for i := 0; i < len(base2HeadColMap); i++ { + if base2HeadColMap[i] == unmappedColumn { + // Have an unmapped base column, now need to figure out if the row existed in the base or if it was added + var aCell *string + if aRow != nil { + // is a deleted column and the `a` row exists so the cell should exist, but still will check for undefined cell error + if cell, err := getCell(*aRow, i); err != nil { if err != ErrorUndefinedCell { return nil, err } - bIsUndefined = true } else { - bCell = cell + aCell = &cell } - } else { - bIsUndefined = true } - if cells[cellsIndex] != nil && len(cells) >= cellsIndex+1 { - copy(cells[cellsIndex+1:], cells[cellsIndex:]) + diffTableIndex := i + baseOffset + if diffTableCells[diffTableIndex] != nil { + // the diffCells array already has a cell at this i index, so shift this cell and those after it one to the right using copy + copy(diffTableCells[diffTableIndex+1:], diffTableCells[diffTableIndex:]) } - var cellType TableDiffCellType - if bIsUndefined { - cellType = TableDiffCellDel - } else { - cellType = TableDiffCellAdd + diffCell := TableDiffCell{Type: TableDiffCellDel} + if aCell != nil { + diffCell.LeftCell = *aCell } - cells[cellsIndex] = &TableDiffCell{RightCell: bCell, Type: cellType} - } else if cellsIndex < b2a[i] { - cellsIndex = b2a[i] + diffTableCells[diffTableIndex] = &diffCell + baseOffset++ } - cellsIndex++ } - return &TableDiffRow{RowIdx: bline, Cells: cells}, nil + return &TableDiffRow{RowIdx: bLineNum, Cells: diffTableCells}, nil } - var sections []*TableDiffSection + // diffTableSections are TableDiffSections which represent the diffTableSections we get when doing a diff, each will be its own table in the view + var diffTableSections []*TableDiffSection for i, section := range diffFile.Sections { - var rows []*TableDiffRow + // Each section has multiple diffTableRows + var diffTableRows []*TableDiffRow lines := tryMergeLines(section.Lines) + // Loop throught the merged lines to get each row of the CSV diff table for this section for j, line := range lines { if i == 0 && j == 0 && (line[0] != 1 || line[1] != 1) { - diffRow, err := createDiffRow(1, 1) + diffTableRow, err := createDiffTableRow(1, 1) if err != nil { return nil, err } - if diffRow != nil { - rows = append(rows, diffRow) + if diffTableRow != nil { + diffTableRows = append(diffTableRows, diffTableRow) } } - diffRow, err := createDiffRow(line[0], line[1]) + diffTableRow, err := createDiffTableRow(line[0], line[1]) if err != nil { return nil, err } - if diffRow != nil { - rows = append(rows, diffRow) + if diffTableRow != nil { + diffTableRows = append(diffTableRows, diffTableRow) } } - if len(rows) > 0 { - sections = append(sections, &TableDiffSection{Rows: rows}) + if len(diffTableRows) > 0 { + diffTableSections = append(diffTableSections, &TableDiffSection{Rows: diffTableRows}) } } - return sections, nil + return diffTableSections, nil } // getColumnMapping creates a mapping of columns between a and b -func getColumnMapping(a *csvReader, b *csvReader) ([]int, []int) { - arow, _ := a.GetRow(0) - brow, _ := b.GetRow(0) +func getColumnMapping(baseCSVReader *csvReader, headCSVReader *csvReader) ([]int, []int) { + baseRow, _ := baseCSVReader.GetRow(0) + headRow, _ := headCSVReader.GetRow(0) - a2b := []int{} - b2a := []int{} + base2HeadColMap := []int{} + head2BaseColMap := []int{} - if arow != nil { - a2b = make([]int, len(arow)) + if baseRow != nil { + base2HeadColMap = make([]int, len(baseRow)) } - if brow != nil { - b2a = make([]int, len(brow)) + if headRow != nil { + head2BaseColMap = make([]int, len(headRow)) } - for i := 0; i < len(b2a); i++ { - b2a[i] = unmappedColumn + // Initializes all head2base mappings to be unmappedColumn (-1) + for i := 0; i < len(head2BaseColMap); i++ { + head2BaseColMap[i] = unmappedColumn } - bcol := 0 - for i := 0; i < len(a2b); i++ { - a2b[i] = unmappedColumn - - acell, ea := getCell(arow, i) - if ea == nil { - for j := bcol; j < len(b2a); j++ { - bcell, eb := getCell(brow, j) - if eb == nil && acell == bcell { - a2b[i] = j - b2a[j] = i - bcol = j + 1 - break + // Loops through the baseRow and see if there is a match in the head row + for i := 0; i < len(baseRow); i++ { + base2HeadColMap[i] = unmappedColumn + baseCell, err := getCell(baseRow, i) + if err == nil { + for j := 0; j < len(headRow); j++ { + if head2BaseColMap[j] == -1 { + headCell, err := getCell(headRow, j) + if err == nil && baseCell == headCell { + base2HeadColMap[i] = j + head2BaseColMap[j] = i + break + } } } } } - tryMapColumnsByContent(a, a2b, b, b2a) - tryMapColumnsByContent(b, b2a, a, a2b) + tryMapColumnsByContent(baseCSVReader, base2HeadColMap, headCSVReader, head2BaseColMap) + tryMapColumnsByContent(headCSVReader, head2BaseColMap, baseCSVReader, base2HeadColMap) - return a2b, b2a + return base2HeadColMap, head2BaseColMap } // tryMapColumnsByContent tries to map missing columns by the content of the first lines. -func tryMapColumnsByContent(a *csvReader, a2b []int, b *csvReader, b2a []int) { - for i := 0; i < len(a2b); i++ { - bStart := 0 - for a2b[i] == unmappedColumn && bStart < len(b2a) { - if b2a[bStart] == unmappedColumn { - rows := util.Min(maxRowsToInspect, util.Max(0, util.Min(len(a.buffer), len(b.buffer))-1)) +func tryMapColumnsByContent(baseCSVReader *csvReader, base2HeadColMap []int, headCSVReader *csvReader, head2BaseColMap []int) { + for i := 0; i < len(base2HeadColMap); i++ { + headStart := 0 + for base2HeadColMap[i] == unmappedColumn && headStart < len(head2BaseColMap) { + if head2BaseColMap[headStart] == unmappedColumn { + rows := util.Min(maxRowsToInspect, util.Max(0, util.Min(len(baseCSVReader.buffer), len(headCSVReader.buffer))-1)) same := 0 for j := 1; j <= rows; j++ { - aCell, aErr := getCell(a.buffer[j], i) - bCell, bErr := getCell(b.buffer[j], bStart) - if aErr == nil && bErr == nil && aCell == bCell { + baseCell, baseErr := getCell(baseCSVReader.buffer[j], i) + headCell, headErr := getCell(headCSVReader.buffer[j], headStart) + if baseErr == nil && headErr == nil && baseCell == headCell { same++ } } if (float32(same) / float32(rows)) > minRatioToMatch { - a2b[i] = bStart - b2a[bStart] = i + base2HeadColMap[i] = headStart + head2BaseColMap[headStart] = i } } - bStart++ + headStart++ } } } diff --git a/services/gitdiff/csv_test.go b/services/gitdiff/csv_test.go index 82673acf59c67..9a0ed1652968e 100644 --- a/services/gitdiff/csv_test.go +++ b/services/gitdiff/csv_test.go @@ -19,7 +19,7 @@ func TestCSVDiff(t *testing.T) { diff string base string head string - cells [][2]TableDiffCellType + cells [][]TableDiffCellType }{ // case 0 - initial commit of a csv { @@ -29,9 +29,12 @@ func TestCSVDiff(t *testing.T) { @@ -0,0 +1,2 @@ +col1,col2 +a,a`, - base: "", - head: "col1,col2\na,a", - cells: [][2]TableDiffCellType{{TableDiffCellAdd, TableDiffCellAdd}, {TableDiffCellAdd, TableDiffCellAdd}}, + base: "", + head: `col1,col2 +a,a`, + cells: [][]TableDiffCellType{ + {TableDiffCellAdd, TableDiffCellAdd}, + {TableDiffCellAdd, TableDiffCellAdd}}, }, // case 1 - adding 1 row at end { @@ -43,9 +46,15 @@ func TestCSVDiff(t *testing.T) { -a,a +a,a +b,b`, - base: "col1,col2\na,a", - head: "col1,col2\na,a\nb,b", - cells: [][2]TableDiffCellType{{TableDiffCellEqual, TableDiffCellEqual}, {TableDiffCellEqual, TableDiffCellEqual}, {TableDiffCellAdd, TableDiffCellAdd}}, + base: `col1,col2 +a,a`, + head: `col1,col2 +a,a +b,b`, + cells: [][]TableDiffCellType{ + {TableDiffCellUnchanged, TableDiffCellUnchanged}, {TableDiffCellUnchanged, TableDiffCellUnchanged}, + {TableDiffCellAdd, TableDiffCellAdd}, + }, }, // case 2 - row deleted { @@ -56,9 +65,15 @@ func TestCSVDiff(t *testing.T) { col1,col2 -a,a b,b`, - base: "col1,col2\na,a\nb,b", - head: "col1,col2\nb,b", - cells: [][2]TableDiffCellType{{TableDiffCellEqual, TableDiffCellEqual}, {TableDiffCellDel, TableDiffCellDel}, {TableDiffCellEqual, TableDiffCellEqual}}, + base: `col1,col2 +a,a +b,b`, + head: `col1,col2 +b,b`, + cells: [][]TableDiffCellType{ + {TableDiffCellUnchanged, TableDiffCellUnchanged}, {TableDiffCellDel, TableDiffCellDel}, + {TableDiffCellUnchanged, TableDiffCellUnchanged}, + }, }, // case 3 - row changed { @@ -69,9 +84,14 @@ func TestCSVDiff(t *testing.T) { col1,col2 -b,b +b,c`, - base: "col1,col2\nb,b", - head: "col1,col2\nb,c", - cells: [][2]TableDiffCellType{{TableDiffCellEqual, TableDiffCellEqual}, {TableDiffCellEqual, TableDiffCellChanged}}, + base: `col1,col2 +b,b`, + head: `col1,col2 +b,c`, + cells: [][]TableDiffCellType{ + {TableDiffCellUnchanged, TableDiffCellUnchanged}, + {TableDiffCellUnchanged, TableDiffCellChanged}, + }, }, // case 4 - all deleted { @@ -81,57 +101,15 @@ func TestCSVDiff(t *testing.T) { @@ -1,2 +0,0 @@ -col1,col2 -b,c`, - base: "col1,col2\nb,c", - head: "", - cells: [][2]TableDiffCellType{{TableDiffCellDel, TableDiffCellDel}, {TableDiffCellDel, TableDiffCellDel}}, + base: `col1,col2 +b,c`, + head: "", + cells: [][]TableDiffCellType{ + {TableDiffCellDel, TableDiffCellDel}, + {TableDiffCellDel, TableDiffCellDel}, + }, }, - } - - for n, c := range cases { - diff, err := ParsePatch(setting.Git.MaxGitDiffLines, setting.Git.MaxGitDiffLineCharacters, setting.Git.MaxGitDiffFiles, strings.NewReader(c.diff)) - if err != nil { - t.Errorf("ParsePatch failed: %s", err) - } - - var baseReader *csv.Reader - if len(c.base) > 0 { - baseReader, err = csv_module.CreateReaderAndGuessDelimiter(strings.NewReader(c.base)) - if err != nil { - t.Errorf("CreateReaderAndGuessDelimiter failed: %s", err) - } - } - var headReader *csv.Reader - if len(c.head) > 0 { - headReader, err = csv_module.CreateReaderAndGuessDelimiter(strings.NewReader(c.head)) - if err != nil { - t.Errorf("CreateReaderAndGuessDelimiter failed: %s", err) - } - } - - result, err := CreateCsvDiff(diff.Files[0], baseReader, headReader) - assert.NoError(t, err) - assert.Len(t, result, 1, "case %d: should be one section", n) - - section := result[0] - assert.Len(t, section.Rows, len(c.cells), "case %d: should be %d rows", n, len(c.cells)) - - for i, row := range section.Rows { - assert.Len(t, row.Cells, 2, "case %d: row %d should have two cells", n, i) - for j, cell := range row.Cells { - assert.Equal(t, c.cells[i][j], cell.Type, "case %d: row %d cell %d should be equal", n, i, j) - } - } - } -} - -func TestCSVDiffHeadingChange(t *testing.T) { - var cases = []struct { - diff string - base string - head string - cells [][4]TableDiffCellType - }{ - // case 0 - renames first column + // case 5 - renames first column { diff: `diff --git a/unittest.csv b/unittest.csv --- a/unittest.csv @@ -140,11 +118,16 @@ func TestCSVDiffHeadingChange(t *testing.T) { -col1,col2,col3 +cola,col2,col3 a,b,c`, - base: "col1,col2,col3\na,b,c", - head: "cola,col2,col3\na,b,c", - cells: [][4]TableDiffCellType{{TableDiffCellAdd, TableDiffCellDel, TableDiffCellEqual, TableDiffCellEqual}, {TableDiffCellAdd, TableDiffCellDel, TableDiffCellEqual, TableDiffCellEqual}}, + base: `col1,col2,col3 +a,b,c`, + head: `cola,col2,col3 +a,b,c`, + cells: [][]TableDiffCellType{ + {TableDiffCellDel, TableDiffCellAdd, TableDiffCellUnchanged, TableDiffCellUnchanged}, + {TableDiffCellDel, TableDiffCellAdd, TableDiffCellUnchanged, TableDiffCellUnchanged}, + }, }, - // case 1 - inserts a column after first, deletes last column + // case 6 - inserts a column after first, deletes last column { diff: `diff --git a/unittest.csv b/unittest.csv --- a/unittest.csv @@ -154,11 +137,16 @@ func TestCSVDiffHeadingChange(t *testing.T) { -a,b,c +col1,col1a,col2 +a,d,b`, - base: "col1,col2,col3\na,b,c", - head: "col1,col1a,col2\na,d,b", - cells: [][4]TableDiffCellType{{TableDiffCellEqual, TableDiffCellAdd, TableDiffCellEqual, TableDiffCellDel}, {TableDiffCellEqual, TableDiffCellAdd, TableDiffCellEqual, TableDiffCellDel}}, + base: `col1,col2,col3 +a,b,c`, + head: `col1,col1a,col2 +a,d,b`, + cells: [][]TableDiffCellType{ + {TableDiffCellUnchanged, TableDiffCellAdd, TableDiffCellDel, TableDiffCellUnchanged}, + {TableDiffCellUnchanged, TableDiffCellAdd, TableDiffCellDel, TableDiffCellUnchanged}, + }, }, - // case 2 - deletes first column, inserts column after last + // case 7 - deletes first column, inserts column after last { diff: `diff --git a/unittest.csv b/unittest.csv --- a/unittest.csv @@ -168,57 +156,16 @@ func TestCSVDiffHeadingChange(t *testing.T) { -a,b,c +col2,col3,col4 +b,c,d`, - base: "col1,col2,col3\na,b,c", - head: "col2,col3,col4\nb,c,d", - cells: [][4]TableDiffCellType{{TableDiffCellDel, TableDiffCellEqual, TableDiffCellEqual, TableDiffCellAdd}, {TableDiffCellDel, TableDiffCellEqual, TableDiffCellEqual, TableDiffCellAdd}}, + base: `col1,col2,col3 +a,b,c`, + head: `col2,col3,col4 +b,c,d`, + cells: [][]TableDiffCellType{ + {TableDiffCellDel, TableDiffCellMovedUnchanged, TableDiffCellMovedUnchanged, TableDiffCellAdd}, + {TableDiffCellDel, TableDiffCellMovedUnchanged, TableDiffCellMovedUnchanged, TableDiffCellAdd}, + }, }, - } - - for n, c := range cases { - diff, err := ParsePatch(setting.Git.MaxGitDiffLines, setting.Git.MaxGitDiffLineCharacters, setting.Git.MaxGitDiffFiles, strings.NewReader(c.diff)) - if err != nil { - t.Errorf("ParsePatch failed: %s", err) - } - - var baseReader *csv.Reader - if len(c.base) > 0 { - baseReader, err = csv_module.CreateReaderAndGuessDelimiter(strings.NewReader(c.base)) - if err != nil { - t.Errorf("CreateReaderAndGuessDelimiter failed: %s", err) - } - } - var headReader *csv.Reader - if len(c.head) > 0 { - headReader, err = csv_module.CreateReaderAndGuessDelimiter(strings.NewReader(c.head)) - if err != nil { - t.Errorf("CreateReaderAndGuessDelimiter failed: %s", err) - } - } - - result, err := CreateCsvDiff(diff.Files[0], baseReader, headReader) - assert.NoError(t, err) - assert.Len(t, result, 1, "case %d: should be one section", n) - - section := result[0] - assert.Len(t, section.Rows, len(c.cells), "case %d: should be %d rows", n, len(c.cells)) - - for i, row := range section.Rows { - assert.Len(t, row.Cells, 4, "case %d: row %d should have four cells", n, i) - for j, cell := range row.Cells { - assert.Equal(t, c.cells[i][j], cell.Type, "case %d: row %d cell %d should be equal", n, i, j) - } - } - } -} - -func TestCSVDiffMultipleHeadingChanges(t *testing.T) { - var cases = []struct { - diff string - base string - head string - cells [][5]TableDiffCellType - }{ - // case 0 - two columns deleted, 2 added + // case 8 - two columns deleted, 2 added { diff: `diff --git a/unittest.csv b/unittest.csv --- a/unittest.csv @@ -228,9 +175,14 @@ func TestCSVDiffMultipleHeadingChanges(t *testing.T) { -a,b,c +col3,col4,col5 +c,d,e`, - base: "col1,col2,col3\na,b,c", - head: "col3,col4,col5\nc,d,e", - cells: [][5]TableDiffCellType{{TableDiffCellDel, TableDiffCellDel, TableDiffCellEqual, TableDiffCellAdd, TableDiffCellAdd}, {TableDiffCellDel, TableDiffCellDel, TableDiffCellEqual, TableDiffCellAdd, TableDiffCellAdd}}, + base: `col1,col2,col3 +a,b,c`, + head: `col3,col4,col5 +c,d,e`, + cells: [][]TableDiffCellType{ + {TableDiffCellDel, TableDiffCellMovedUnchanged, TableDiffCellDel, TableDiffCellAdd, TableDiffCellAdd}, + {TableDiffCellDel, TableDiffCellMovedUnchanged, TableDiffCellDel, TableDiffCellAdd, TableDiffCellAdd}, + }, }, } @@ -263,7 +215,7 @@ func TestCSVDiffMultipleHeadingChanges(t *testing.T) { assert.Len(t, section.Rows, len(c.cells), "case %d: should be %d rows", n, len(c.cells)) for i, row := range section.Rows { - assert.Len(t, row.Cells, 5, "case %d: row %d should have five cells", n, i) + assert.Len(t, row.Cells, len(c.cells[i]), "case %d: row %d should have %d cells", n, i, len(c.cells[i])) for j, cell := range row.Cells { assert.Equal(t, c.cells[i][j], cell.Type, "case %d: row %d cell %d should be equal", n, i, j) } diff --git a/templates/repo/diff/csv_diff.tmpl b/templates/repo/diff/csv_diff.tmpl index db71d5de6cbca..a92ef79301c6e 100644 --- a/templates/repo/diff/csv_diff.tmpl +++ b/templates/repo/diff/csv_diff.tmpl @@ -20,6 +20,10 @@ {{.RightCell}} {{else if eq $cell.Type 4}} {{.LeftCell}} + {{else if eq $cell.Type 5}} + {{.RightCell}} + {{else if eq $cell.Type 6}} + {{.LeftCell}} {{.RightCell}} {{else}} {{.RightCell}} {{end}} @@ -35,6 +39,10 @@ {{.RightCell}} {{else if eq $cell.Type 4}} {{.LeftCell}} + {{else if eq $cell.Type 5}} + {{.RightCell}} + {{else if eq $cell.Type 6}} + {{.LeftCell}} {{.RightCell}} {{else}} {{.RightCell}} {{end}} diff --git a/web_src/less/_base.less b/web_src/less/_base.less index 8b951b2548d75..7dde8d94db251 100644 --- a/web_src/less/_base.less +++ b/web_src/less/_base.less @@ -76,8 +76,10 @@ --color-diff-removed-word-bg: #fdb8c0; --color-diff-added-word-bg: #acf2bd; --color-diff-removed-row-bg: #ffeef0; + --color-diff-moved-row-bg: #f1f8d1; --color-diff-added-row-bg: #e6ffed; --color-diff-removed-row-border: #f1c0c0; + --color-diff-moved-row-border: #d0e27f; --color-diff-added-row-border: #e6ffed; --color-diff-inactive: #f2f2f2; /* target-based colors */ diff --git a/web_src/less/_repository.less b/web_src/less/_repository.less index a58fe3698d2ec..de4fb5af7d306 100644 --- a/web_src/less/_repository.less +++ b/web_src/less/_repository.less @@ -1500,6 +1500,12 @@ background-color: var(--color-diff-removed-row-bg) !important; } + td.moved, + th.moved, + tr.moved { + background-color: var(--color-diff-moved-row-bg) !important; + } + tbody.section { border-top: 2px solid var(--color-secondary); } diff --git a/web_src/less/themes/theme-arc-green.less b/web_src/less/themes/theme-arc-green.less index bb1e12c4712c0..a595848b76cf8 100644 --- a/web_src/less/themes/theme-arc-green.less +++ b/web_src/less/themes/theme-arc-green.less @@ -71,8 +71,10 @@ --color-diff-removed-word-bg: #6f3333; --color-diff-added-word-bg: #3c653c; --color-diff-removed-row-bg: #3c2626; + --color-diff-moved-row-bg: #818044; --color-diff-added-row-bg: #283e2d; --color-diff-removed-row-border: #634343; + --color-diff-moved-row-border: #bcca6f; --color-diff-added-row-border: #314a37; --color-diff-inactive: #353846; /* target-based colors */ From 8707f2117ab5632084fe52585bde2a1ea8d03918 Mon Sep 17 00:00:00 2001 From: Richard Mahn Date: Fri, 24 Sep 2021 11:38:52 -0400 Subject: [PATCH 14/17] Better looping so no diff table copying --- services/gitdiff/csv.go | 165 +++++++++++++++++++++------------------- 1 file changed, 88 insertions(+), 77 deletions(-) diff --git a/services/gitdiff/csv.go b/services/gitdiff/csv.go index 63640d8e2d645..d7b8a6d928c2c 100644 --- a/services/gitdiff/csv.go +++ b/services/gitdiff/csv.go @@ -160,13 +160,13 @@ func createCsvDiff(diffFile *DiffFile, baseReader *csv.Reader, headReader *csv.R return nil, err } - // Initalizing the mappings of base to head (base2HeadColMap) and head to base (head2BaseColMap) columns - base2HeadColMap, head2BaseColMap := getColumnMapping(baseCSVReader, headCSVReader) + // Initalizing the mappings of base to head (a2bColMap) and head to base (b2aColMap) columns + a2bColMap, b2aColMap := getColumnMapping(baseCSVReader, headCSVReader) // Determines how many cols there will be in the diff table, which includes deleted columsn from base and added columns to base - numDiffTableCols := len(base2HeadColMap) + countUnmappedColumns(head2BaseColMap) - if len(base2HeadColMap) < len(head2BaseColMap) { - numDiffTableCols = len(head2BaseColMap) + countUnmappedColumns(base2HeadColMap) + numDiffTableCols := len(a2bColMap) + countUnmappedColumns(b2aColMap) + if len(a2bColMap) < len(b2aColMap) { + numDiffTableCols = len(b2aColMap) + countUnmappedColumns(a2bColMap) } // createDiffTableRow takes the row # of the `a` line and `b` line of a diff (starting from 1), 0 if the line doesn't exist (undefined) @@ -192,107 +192,118 @@ func createCsvDiff(diffFile *DiffFile, baseReader *csv.Reader, headReader *csv.R } bRow = &row } - if bRow == nil { - } else { - if len(*bRow) == 0 { - } - } if aRow == nil && bRow == nil { // No content return nil, nil } - // First we loop through the head columns and place them in the diff table as they appear, both existing columsn and new columns - for i := 0; i < len(head2BaseColMap); i++ { - var bCell *string // Pointer to text of the b line (head), if nil the cell doesn't exist in the head (deleted row in the base) - if bRow != nil { - // is an added column and the `b` row exists so the cell should exist, but still will check for undefined cell error - if cell, err := getCell(*bRow, i); err != nil { - if err != ErrorUndefinedCell { - return nil, err - } - } else { - bCell = &cell - } - } - var diffCell TableDiffCell - addedCols := 0 - if head2BaseColMap[i] == unmappedColumn { - // This column doesn't exist in the base, so we know it is a new column. - var cell string - // Col exists in the head, but we might be displaying a deleted row, so need to check if bCell exists, and if it does, make cell its value - if bCell != nil { - cell = *bCell - } - diffCell = TableDiffCell{RightCell: cell, Type: TableDiffCellAdd} - addedCols++ - } else { - // The column still exists in the head, but we need to figure out if the row exists as well (changed text) or if it is a new row in head - var aCell *string // Pointer to the texst of the a line (base), if nil the cell doesn't exist in the base (added row in head) + aIndex := 0 // tracks where we are in the a2bColMap + bIndex := 0 // tracks where we are in the b2aColMap + colsAdded := 0 // incremented whenever we found a column was added + colsDeleted := 0 // incrememted whenever a column was deleted + + // We loop until both the aIndex and bIndex are greater tahn their col map, which then we are done + for aIndex < len(a2bColMap) || bIndex < len(b2aColMap) { + // Starting from where aIndex is currently pointing, we see if the map is -1 (dleeted) and if is, create column to note that, increment, and look at the next aIndex + for aIndex < len(a2bColMap) && a2bColMap[aIndex] == -1 { + var aCell string if aRow != nil { - // Get the cell contents of the 'a' row. Should exist, but just in case we handle the error and make the contents and empty string - if cell, err := getCell(*aRow, head2BaseColMap[i]); err != nil { + if cell, err := getCell(*aRow, aIndex); err != nil { if err != ErrorUndefinedCell { return nil, err } } else { - aCell = &cell + aCell = cell } } - if bCell == nil { - // both a & b have the column, but not the row (deleted) - diffCell = TableDiffCell{LeftCell: *aCell, Type: TableDiffCellDel} - } else if aCell == nil { - // both a & b have the column, but not the row (added) - diffCell = TableDiffCell{RightCell: *bCell, Type: TableDiffCellAdd} - } else { - var cellType TableDiffCellType - if head2BaseColMap[i] > i-addedCols { - if *aCell != *bCell { - cellType = TableDiffCellMovedChanged - } else { - cellType = TableDiffCellMovedUnchanged + diffTableCells[bIndex+colsDeleted] = &TableDiffCell{LeftCell: aCell, Type: TableDiffCellDel} + aIndex++ + colsDeleted++ + } + + // Starting from where bIndex is currently pointing, we see if the map is -1 (added) and if is, create column to note that, increment, and look at the next aIndex + for bIndex < len(b2aColMap) && b2aColMap[bIndex] == -1 { + var bCell string + cellType := TableDiffCellAdd + if bRow != nil { + if cell, err := getCell(*bRow, bIndex); err != nil { + if err != ErrorUndefinedCell { + return nil, err } } else { - if *aCell != *bCell { - cellType = TableDiffCellChanged - } else { - cellType = TableDiffCellUnchanged - } + bCell = cell } - diffCell = TableDiffCell{LeftCell: *aCell, RightCell: *bCell, Type: cellType} + } else { + cellType = TableDiffCellDel } + diffTableCells[bIndex+colsDeleted] = &TableDiffCell{RightCell: bCell, Type: cellType} + bIndex++ + colsAdded++ } - diffTableCells[i] = &diffCell - } - // Now loop through the base columns to find the unmmapped (deleted) columns in the base - baseOffset := 0 - for i := 0; i < len(base2HeadColMap); i++ { - if base2HeadColMap[i] == unmappedColumn { - // Have an unmapped base column, now need to figure out if the row existed in the base or if it was added + // aIndex is now pointing to a column that also exists in b, or is at the end of a2bColMap. If the former, + // we can just increment aIndex until it points to a -1 column or is at the end + for aIndex < len(a2bColMap) && a2bColMap[aIndex] != -1 { + aIndex++ + } + + // aIndex is now pointing to a column that also exists in a, or is at the end of b2aColMap. If the former, + // we get the a col and b col values (if they exist), figure out if they are the same or not, and if the column moved, and add it to the diff table + for bIndex < len(b2aColMap) && b2aColMap[bIndex] != -1 { + var diffTableCell TableDiffCell + var aCell *string + // get the aCell value if the aRow exists if aRow != nil { - // is a deleted column and the `a` row exists so the cell should exist, but still will check for undefined cell error - if cell, err := getCell(*aRow, i); err != nil { + if cell, err := getCell(*aRow, b2aColMap[bIndex]); err != nil { if err != ErrorUndefinedCell { return nil, err } } else { aCell = &cell + diffTableCell.LeftCell = cell } + } else { + diffTableCell.Type = TableDiffCellAdd } - diffTableIndex := i + baseOffset - if diffTableCells[diffTableIndex] != nil { - // the diffCells array already has a cell at this i index, so shift this cell and those after it one to the right using copy - copy(diffTableCells[diffTableIndex+1:], diffTableCells[diffTableIndex:]) + + var bCell *string + // get the bCell value if the bRow exists + if bRow != nil { + if cell, err := getCell(*bRow, bIndex); err != nil { + if err != ErrorUndefinedCell { + return nil, err + } + } else { + bCell = &cell + diffTableCell.RightCell = cell + } + } else { + diffTableCell.Type = TableDiffCellDel } - diffCell := TableDiffCell{Type: TableDiffCellDel} - if aCell != nil { - diffCell.LeftCell = *aCell + + // if both a and b have a row that exists, compare the value and determine if the row has moved + if aRow != nil && bRow != nil { + moved := ((bIndex + colsDeleted) != (b2aColMap[bIndex] + colsAdded)) + if *aCell != *bCell { + if moved { + diffTableCell.Type = TableDiffCellMovedChanged + } else { + diffTableCell.Type = TableDiffCellChanged + } + } else { + if moved { + diffTableCell.Type = TableDiffCellMovedUnchanged + } else { + diffTableCell.Type = TableDiffCellUnchanged + } + diffTableCell.LeftCell = "" + } } - diffTableCells[diffTableIndex] = &diffCell - baseOffset++ + + // Add the diff column to the diff row + diffTableCells[bIndex+colsDeleted] = &diffTableCell + bIndex++ } } From 1d7a5193c198f225bab4e584f333cb67d670caab Mon Sep 17 00:00:00 2001 From: Richard Mahn Date: Wed, 6 Oct 2021 09:41:42 -0400 Subject: [PATCH 15/17] Properly handle where to put the a rows --- services/gitdiff/csv.go | 34 +++++++++++++++++----------------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/services/gitdiff/csv.go b/services/gitdiff/csv.go index d7b8a6d928c2c..61a7aa3dbecea 100644 --- a/services/gitdiff/csv.go +++ b/services/gitdiff/csv.go @@ -176,14 +176,6 @@ func createCsvDiff(diffFile *DiffFile, baseReader *csv.Reader, headReader *csv.R // diffTableCells is a row of the diff table. It will have a cells for added, deleted, changed, and unchanged content, thus either // the same size as the head table or bigger diffTableCells := make([]*TableDiffCell, numDiffTableCols) - var aRow *[]string - if aLineNum > 0 { - row, err := baseCSVReader.GetRow(aLineNum - 1) - if err != nil { - return nil, err - } - aRow = &row - } var bRow *[]string if bLineNum > 0 { row, err := headCSVReader.GetRow(bLineNum - 1) @@ -192,6 +184,14 @@ func createCsvDiff(diffFile *DiffFile, baseReader *csv.Reader, headReader *csv.R } bRow = &row } + var aRow *[]string + if aLineNum > 0 { + row, err := baseCSVReader.GetRow(aLineNum - 1) + if err != nil { + return nil, err + } + aRow = &row + } if aRow == nil && bRow == nil { // No content return nil, nil @@ -221,8 +221,14 @@ func createCsvDiff(diffFile *DiffFile, baseReader *csv.Reader, headReader *csv.R colsDeleted++ } + // aIndex is now pointing to a column that also exists in b, or is at the end of a2bColMap. If the former, + // we can just increment aIndex until it points to a -1 column or one greater than the current bIndex + for aIndex < len(a2bColMap) && a2bColMap[aIndex] != -1 { + aIndex++ + } + // Starting from where bIndex is currently pointing, we see if the map is -1 (added) and if is, create column to note that, increment, and look at the next aIndex - for bIndex < len(b2aColMap) && b2aColMap[bIndex] == -1 { + for bIndex < len(b2aColMap) && b2aColMap[bIndex] == -1 && (aIndex >= len(a2bColMap) || bIndex < aIndex) { var bCell string cellType := TableDiffCellAdd if bRow != nil { @@ -241,15 +247,9 @@ func createCsvDiff(diffFile *DiffFile, baseReader *csv.Reader, headReader *csv.R colsAdded++ } - // aIndex is now pointing to a column that also exists in b, or is at the end of a2bColMap. If the former, - // we can just increment aIndex until it points to a -1 column or is at the end - for aIndex < len(a2bColMap) && a2bColMap[aIndex] != -1 { - aIndex++ - } - // aIndex is now pointing to a column that also exists in a, or is at the end of b2aColMap. If the former, // we get the a col and b col values (if they exist), figure out if they are the same or not, and if the column moved, and add it to the diff table - for bIndex < len(b2aColMap) && b2aColMap[bIndex] != -1 { + for bIndex < len(b2aColMap) && b2aColMap[bIndex] != -1 && (aIndex >= len(a2bColMap) || bIndex < aIndex) { var diffTableCell TableDiffCell var aCell *string @@ -283,7 +283,7 @@ func createCsvDiff(diffFile *DiffFile, baseReader *csv.Reader, headReader *csv.R } // if both a and b have a row that exists, compare the value and determine if the row has moved - if aRow != nil && bRow != nil { + if aCell != nil && bCell != nil { moved := ((bIndex + colsDeleted) != (b2aColMap[bIndex] + colsAdded)) if *aCell != *bCell { if moved { From 480d9ec95fbcbf152f80e6641de59a682ef0a252 Mon Sep 17 00:00:00 2001 From: Richard Mahn Date: Tue, 19 Oct 2021 15:10:10 -0400 Subject: [PATCH 16/17] Updates tests --- services/gitdiff/csv.go | 2 +- services/gitdiff/csv_test.go | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/services/gitdiff/csv.go b/services/gitdiff/csv.go index 61a7aa3dbecea..2b37a55c8e78b 100644 --- a/services/gitdiff/csv.go +++ b/services/gitdiff/csv.go @@ -205,7 +205,7 @@ func createCsvDiff(diffFile *DiffFile, baseReader *csv.Reader, headReader *csv.R // We loop until both the aIndex and bIndex are greater tahn their col map, which then we are done for aIndex < len(a2bColMap) || bIndex < len(b2aColMap) { // Starting from where aIndex is currently pointing, we see if the map is -1 (dleeted) and if is, create column to note that, increment, and look at the next aIndex - for aIndex < len(a2bColMap) && a2bColMap[aIndex] == -1 { + for aIndex < len(a2bColMap) && a2bColMap[aIndex] == -1 && (bIndex >= len(b2aColMap) || aIndex <= bIndex) { var aCell string if aRow != nil { if cell, err := getCell(*aRow, aIndex); err != nil { diff --git a/services/gitdiff/csv_test.go b/services/gitdiff/csv_test.go index 9a0ed1652968e..1710e91c58e82 100644 --- a/services/gitdiff/csv_test.go +++ b/services/gitdiff/csv_test.go @@ -142,8 +142,8 @@ a,b,c`, head: `col1,col1a,col2 a,d,b`, cells: [][]TableDiffCellType{ - {TableDiffCellUnchanged, TableDiffCellAdd, TableDiffCellDel, TableDiffCellUnchanged}, - {TableDiffCellUnchanged, TableDiffCellAdd, TableDiffCellDel, TableDiffCellUnchanged}, + {TableDiffCellUnchanged, TableDiffCellAdd, TableDiffCellDel, TableDiffCellMovedUnchanged}, + {TableDiffCellUnchanged, TableDiffCellAdd, TableDiffCellDel, TableDiffCellMovedUnchanged}, }, }, // case 7 - deletes first column, inserts column after last @@ -161,8 +161,8 @@ a,b,c`, head: `col2,col3,col4 b,c,d`, cells: [][]TableDiffCellType{ - {TableDiffCellDel, TableDiffCellMovedUnchanged, TableDiffCellMovedUnchanged, TableDiffCellAdd}, - {TableDiffCellDel, TableDiffCellMovedUnchanged, TableDiffCellMovedUnchanged, TableDiffCellAdd}, + {TableDiffCellDel, TableDiffCellUnchanged, TableDiffCellUnchanged, TableDiffCellAdd}, + {TableDiffCellDel, TableDiffCellUnchanged, TableDiffCellUnchanged, TableDiffCellAdd}, }, }, // case 8 - two columns deleted, 2 added From fe1bbc64c7d43914bf6051629926822bf0635b74 Mon Sep 17 00:00:00 2001 From: Richard Mahn Date: Tue, 19 Oct 2021 15:21:46 -0400 Subject: [PATCH 17/17] Fixes typos --- services/gitdiff/csv.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/services/gitdiff/csv.go b/services/gitdiff/csv.go index 2b37a55c8e78b..099660b17281c 100644 --- a/services/gitdiff/csv.go +++ b/services/gitdiff/csv.go @@ -160,10 +160,10 @@ func createCsvDiff(diffFile *DiffFile, baseReader *csv.Reader, headReader *csv.R return nil, err } - // Initalizing the mappings of base to head (a2bColMap) and head to base (b2aColMap) columns + // Initializing the mappings of base to head (a2bColMap) and head to base (b2aColMap) columns a2bColMap, b2aColMap := getColumnMapping(baseCSVReader, headCSVReader) - // Determines how many cols there will be in the diff table, which includes deleted columsn from base and added columns to base + // Determines how many cols there will be in the diff table, which includes deleted columns from base and added columns to base numDiffTableCols := len(a2bColMap) + countUnmappedColumns(b2aColMap) if len(a2bColMap) < len(b2aColMap) { numDiffTableCols = len(b2aColMap) + countUnmappedColumns(a2bColMap) @@ -202,7 +202,7 @@ func createCsvDiff(diffFile *DiffFile, baseReader *csv.Reader, headReader *csv.R colsAdded := 0 // incremented whenever we found a column was added colsDeleted := 0 // incrememted whenever a column was deleted - // We loop until both the aIndex and bIndex are greater tahn their col map, which then we are done + // We loop until both the aIndex and bIndex are greater than their col map, which then we are done for aIndex < len(a2bColMap) || bIndex < len(b2aColMap) { // Starting from where aIndex is currently pointing, we see if the map is -1 (dleeted) and if is, create column to note that, increment, and look at the next aIndex for aIndex < len(a2bColMap) && a2bColMap[aIndex] == -1 && (bIndex >= len(b2aColMap) || aIndex <= bIndex) { @@ -317,7 +317,7 @@ func createCsvDiff(diffFile *DiffFile, baseReader *csv.Reader, headReader *csv.R // Each section has multiple diffTableRows var diffTableRows []*TableDiffRow lines := tryMergeLines(section.Lines) - // Loop throught the merged lines to get each row of the CSV diff table for this section + // Loop through the merged lines to get each row of the CSV diff table for this section for j, line := range lines { if i == 0 && j == 0 && (line[0] != 1 || line[1] != 1) { diffTableRow, err := createDiffTableRow(1, 1)