Skip to content
This repository was archived by the owner on Mar 27, 2024. It is now read-only.

Diff within files #121

Merged
merged 4 commits into from
Oct 19, 2017
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions cmd/analyze.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,9 @@ var analyzeCmd = &cobra.Command{
if err := validateArgs(args, checkAnalyzeArgNum); err != nil {
return err
}
if err := checkAnalyzeFilenameFlag(); err != nil {
return err
}
if err := checkIfValidAnalyzer(types); err != nil {
return err
}
Expand All @@ -56,6 +59,13 @@ func checkAnalyzeArgNum(args []string) error {
return nil
}

func checkAnalyzeFilenameFlag() error {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove this once you've added the flag under container-diff diff only

if filename != "" {
return errors.New("please remove --filename flag, incompatible with analyze")
}
return nil
}

func analyzeImage(imageName string, analyzerArgs []string) error {
analyzeTypes, err := differs.GetAnalyzers(analyzerArgs)
if err != nil {
Expand Down
42 changes: 35 additions & 7 deletions cmd/diff.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,14 +19,14 @@ package cmd
import (
"errors"
"fmt"
"os"
"strings"
"sync"

"github.com/GoogleCloudPlatform/container-diff/differs"
pkgutil "github.com/GoogleCloudPlatform/container-diff/pkg/util"
"github.com/GoogleCloudPlatform/container-diff/util"
"github.com/sirupsen/logrus"
"github.com/spf13/cobra"
"os"
"strings"
"sync"
)

var diffCmd = &cobra.Command{
Expand All @@ -43,7 +43,8 @@ var diffCmd = &cobra.Command{
return nil
},
Run: func(cmd *cobra.Command, args []string) {
if err := diffImages(args[0], args[1], strings.Split(types, ",")); err != nil {
typesFlagSet := checkIfTypesFlagSet(cmd)
if err := diffImages(args[0], args[1], strings.Split(types, ","), typesFlagSet); err != nil {
logrus.Error(err)
os.Exit(1)
}
Expand All @@ -57,7 +58,7 @@ func checkDiffArgNum(args []string) error {
return nil
}

func diffImages(image1Arg, image2Arg string, diffArgs []string) error {
func diffImages(image1Arg, image2Arg string, diffArgs []string, typesFlagSet bool) error {
diffTypes, err := differs.GetAnalyzers(diffArgs)
if err != nil {
return err
Expand All @@ -71,7 +72,9 @@ func diffImages(image1Arg, image2Arg string, diffArgs []string) error {
var wg sync.WaitGroup
wg.Add(2)

fmt.Fprintf(os.Stderr, "Starting diff on images %s and %s, using differs: %s\n", image1Arg, image2Arg, diffArgs)
if typesFlagSet || filename == "" {
fmt.Fprintf(os.Stderr, "Starting diff on images %s and %s, using differs: %s\n", image1Arg, image2Arg, diffArgs)
}

imageMap := map[string]*pkgutil.Image{
image1Arg: {},
Expand Down Expand Up @@ -101,6 +104,17 @@ func diffImages(image1Arg, image2Arg string, diffArgs []string) error {
defer pkgutil.CleanupImage(*imageMap[image2Arg])
}

if filename != "" {
fmt.Fprintln(os.Stderr, "Computing filename diffs")
err := diffFile(imageMap, image1Arg, image2Arg)
if err != nil {
return err
}
if !typesFlagSet {
return nil
}
}

fmt.Fprintln(os.Stderr, "Computing diffs")
req := differs.DiffRequest{*imageMap[image1Arg], *imageMap[image2Arg], diffTypes}
diffs, err := req.GetDiff()
Expand All @@ -116,6 +130,20 @@ func diffImages(image1Arg, image2Arg string, diffArgs []string) error {
return nil
}

func diffFile(imageMap map[string]*pkgutil.Image, image1Arg, image2Arg string) error {

image1FilePath := imageMap[image1Arg].FSPath + filename
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these should probably use something like path.Join https://golang.org/pkg/path/#Join

image2FilePath := imageMap[image2Arg].FSPath + filename

diff, err := util.DiffFile(image1FilePath, image2FilePath, image1Arg, image2Arg)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think util.DiffFile can do the path joining actually.

You could make util.DiffFile look like

func DiffFile(image1, image2, filename string) (FileNameDiff, error) {
  // join paths
  // run diff
  // return full FileNameDiff object
}

if err != nil {
return err
}
diff.Filename = filename
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you pass filename into util.DiffFile so you don't have to set this after the fact?

util.TemplateOutput(diff, "FileNameDiff")
return nil
}

func init() {
RootCmd.AddCommand(diffCmd)
addSharedFlags(diffCmd)
Expand Down
7 changes: 7 additions & 0 deletions cmd/root.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ import (
var json bool
var save bool
var types string
var filename string

var LogLevel string

Expand Down Expand Up @@ -117,6 +118,11 @@ func checkIfValidAnalyzer(flagtypes string) error {
return nil
}

func checkIfTypesFlagSet(cmd *cobra.Command) bool {
flag := cmd.Flags().Lookup("types")
return flag.Changed
}

func getPrepperForImage(image string) (pkgutil.Prepper, error) {
cli, err := client.NewEnvClient()
if err != nil {
Expand Down Expand Up @@ -151,4 +157,5 @@ func addSharedFlags(cmd *cobra.Command) {
cmd.Flags().StringVarP(&types, "types", "t", "apt", "This flag sets the list of analyzer types to use. It expects a comma separated list of supported analyzers.")
cmd.Flags().BoolVarP(&save, "save", "s", false, "Set this flag to save rather than remove the final image filesystems on exit.")
cmd.Flags().BoolVarP(&util.SortSize, "order", "o", false, "Set this flag to sort any file/package results by descending size. Otherwise, they will be sorted by name.")
cmd.Flags().StringVarP(&filename, "filename", "f", "", "View the diff of a file in both containers (can only be used with container-diff diff)")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should add this flag to the diff subcommand specific flags instead of have it be at the root level. That will let you remove a lot of the flag validation.

}
19 changes: 19 additions & 0 deletions pkg/util/fs_utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ package util

import (
"bytes"
"errors"
"io/ioutil"
"os"
"path/filepath"
Expand Down Expand Up @@ -53,6 +54,24 @@ func GetSize(path string) int64 {
return stat.Size()
}

//GetFileContents returns the contents of a file at the specified path
func GetFileContents(path string) ([]string, error) {
stat, err := os.Stat(path)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this checking for the existence of a file?

It should be

if _, err := os.Stat("/path/to/whatever"); os.IsNotExist(err) {
  // path/to/whatever does not exist
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it does check for existence, but also checks that the path isn't a directory after, which is why I thought it might be easier to split it up?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess what is the expected behavior when the file doesn't exist? I think you wouldn't want to throw an error?
Different scenarios:

A: ""
B: no file
output: ?

A: "something"
B: no file
output: ?

A: no file
B: no file
output: ?

if err != nil {
return nil, err
}

if stat.IsDir() {
return nil, errors.New("--filename is a directory, not a file")
}
contents, err := ioutil.ReadFile(path)

if err != nil {
return nil, err
}
return []string{string(contents)}, nil
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why doesn't this return a normal string?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the diff library I'm using wanted this format, but it probably makes more sense to move that to DiffFile

}

func getDirectorySize(path string) (int64, error) {
var size int64
err := filepath.Walk(path, func(_ string, info os.FileInfo, err error) error {
Expand Down
35 changes: 35 additions & 0 deletions util/diff_utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,11 @@ type DirDiff struct {
Mods []EntryDiff
}

type FileNameDiff struct {
Filename string
Diff string
}

type EntryDiff struct {
Name string
Size1 int64
Expand Down Expand Up @@ -118,6 +123,36 @@ func DiffDirectory(d1, d2 pkgutil.Directory) (DirDiff, bool) {
return DirDiff{addedEntries, deletedEntries, modifiedEntries}, same
}

//DiffFile diffs within a file
func DiffFile(path1, path2, image1, image2 string) (FileNameDiff, error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't satisfy any interface right? In that case, you should probably pass the filename parameter into here too and set it on the FileNameDiff


Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove newline

var result FileNameDiff

image1Contents, err := pkgutil.GetFileContents(path1)
if err != nil {
return result, err
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when you encounter an error, you should return nil, err

}
image2Contents, err := pkgutil.GetFileContents(path2)

if err != nil {
return result, err
}

diff := difflib.UnifiedDiff{
A: image1Contents,
B: image2Contents,
FromFile: image1,
ToFile: image2,
}

text, err := difflib.GetUnifiedDiffString(diff)

if err != nil {
return result, err
}
return FileNameDiff{Diff: text}, nil
}

// Checks for content differences between files of the same name from different directories
func GetModifiedEntries(d1, d2 pkgutil.Directory) []string {
d1files := d1.Content
Expand Down
3 changes: 2 additions & 1 deletion util/format_utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,10 @@ import (
"bufio"
"encoding/json"
"errors"
"html/template"
"os"
"strings"
"text/tabwriter"
"text/template"

"github.com/sirupsen/logrus"
)
Expand All @@ -33,6 +33,7 @@ var templates = map[string]string{
"MultiVersionPackageDiff": MultiVersionDiffOutput,
"HistDiff": HistoryDiffOutput,
"DirDiff": FSDiffOutput,
"FileNameDiff": FilenameDiffOutput,
"ListAnalyze": ListAnalysisOutput,
"FileAnalyze": FileAnalysisOutput,
"MultiVersionPackageAnalyze": MultiVersionPackageOutput,
Expand Down
5 changes: 5 additions & 0 deletions util/template_utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,11 @@ Docker history lines found only in {{.Image1}}:{{if not .Diff.Adds}} None{{else}

Docker history lines found only in {{.Image2}}:{{if not .Diff.Dels}} None{{else}}{{block "list2" .Diff.Dels}}{{"\n"}}{{range .}}{{print "-" .}}{{"\n"}}{{end}}{{end}}{{end}}
`
const FilenameDiffOutput = `
-----Diff of {{.Filename}}-----

{{.Diff}}
`

const ListAnalysisOutput = `
-----{{.AnalyzeType}}-----
Expand Down