[PATCH v7 0/4] difftool: dir-diff improvements and refactoring

* [PATCH v7 0/4] difftool: dir-diff improvements and refactoring
@ 2021-10-01  1:37 David Aguilar
  2021-10-01  1:37 ` [PATCH v7 1/4] difftool: create a tmpdir path without repeated slashes David Aguilar
                   ` (3 more replies)
  0 siblings, 4 replies; 5+ messages in thread
From: David Aguilar @ 2021-10-01  1:37 UTC (permalink / raw)
  To: Git Mailing List
  Cc: Junio C Hamano, Ævar Arnfjörð Bjarmason,
	Johannes Schindelin

Changes since v6:

- avoid returning -1 to cmd_main() by adjusting the return site in
  "create a tmpdir path without repeated slashes".

- "refactor dir-diff to write files using helper functions" was
  reworked to add two helper functions instead of one so that the
  common checks for *entry->{left,right} can be handled in a single place.

- write_entry() was renamed to write_file_in_directory() and its
  signature was adjusted to match how write_file() takes its parameters.

- write_file_in_directory() gets called from the newly added
  write_standin_files() helper which encompases the guts of
  the symlinks and submodules hashmap loops.

- Comments were added describing the purpose of the helper functions.

David Aguilar (4):
  difftool: create a tmpdir path without repeated slashes
  difftool: refactor dir-diff to write files using helper functions
  difftool: remove an unnecessary call to strbuf_release()
  difftool: add a missing space to the run_dir_diff() comments

 builtin/difftool.c  | 104 ++++++++++++++++++++++----------------------
 t/t7800-difftool.sh |   7 +++
 2 files changed, 60 insertions(+), 51 deletions(-)

Range-diff against v6:
1:  121186ca0f ! 1:  14b5618945 difftool: create a tmpdir path without repeated slashes
    @@ Commit message
         Strip trailing slashes from the value read from TMPDIR to avoid
         repeated slashes in the generated paths.
     
    -    Adjust the error handling to avoid leaking strbufs.
    +    Adjust the error handling to avoid leaking strbufs and to avoid
    +    returning -1 to cmd_main().
     
         Signed-off-by: David Aguilar <davvid@gmail.com>
     
    @@ builtin/difftool.c: static int run_dir_diff(const char *extcmd, int symlinks, co
      	strbuf_release(&buf);
     +	strbuf_release(&tmpdir);
      
    - 	return ret;
    +-	return ret;
    ++	return (ret < 0) ? 1 : ret;
      }
    + 
    + static int run_file_diff(int prompt, const char *prefix,
     
      ## t/t7800-difftool.sh ##
     @@ t/t7800-difftool.sh: run_dir_diff_test 'difftool --dir-diff' '
4:  8e7d54616f ! 2:  0824321eb9 difftool: refactor dir-diff to write files using a helper function
    @@ Metadata
     Author: David Aguilar <davvid@gmail.com>
     
      ## Commit message ##
    -    difftool: refactor dir-diff to write files using a helper function
    +    difftool: refactor dir-diff to write files using helper functions
     
    -    Add a write_entry() helper function to handle the unlinking and writing
    +    Add a helpers function to handle the unlinking and writing
         of the dir-diff submodule and symlink stand-in files.
     
    -    Use write_entry() inside of the hashmap loops to eliminate duplicate
    -    code and to safeguard the submodules hashmap loop against the
    -    symlink-chasing behavior that 5bafb3576a (difftool: fix symlink-file
    -    writing in dir-diff mode, 2021-09-22) addressed.
    +    Use the helpers to implement the guts of the hashmap loops.
    +    This eliminate duplicate code and safeguards the submodules
    +    hashmap loop against the symlink-chasing behavior that 5bafb3576a
    +    (difftool: fix symlink-file writing in dir-diff mode, 2021-09-22)
    +    addressed.
     
         The submodules loop should not strictly require the unlink() call that
         this is introducing to them, but it does not necessarily hurt them
    @@ builtin/difftool.c: static int checkout_path(unsigned mode, struct object_id *oi
      	return ret;
      }
      
    -+static void write_entry(const char *path, const char *content,
    -+			struct strbuf *buf, size_t len)
    ++static void write_file_in_directory(struct strbuf *dir, size_t dir_len,
    ++			const char *path, const char *content)
     +{
    -+	if (!*content)
    -+		return;
    -+	add_path(buf, len, path);
    -+	ensure_leading_directories(buf->buf);
    -+	unlink(buf->buf);
    -+	write_file(buf->buf, "%s", content);
    ++	add_path(dir, dir_len, path);
    ++	ensure_leading_directories(dir->buf);
    ++	unlink(dir->buf);
    ++	write_file(dir->buf, "%s", content);
    ++}
    ++
    ++/* Write the file contents for the left and right sides of the difftool
    ++ * dir-diff representation for submodules and symlinks. Symlinks and submodules
    ++ * are written as regular text files so that external diff tools can diff them
    ++ * as text files, resulting in behavior that is analogous to to what "git diff"
    ++ * displays for symlink and submodule diffs.
    ++ */
    ++static void write_standin_files(struct pair_entry *entry,
    ++			struct strbuf *ldir, size_t ldir_len,
    ++			struct strbuf *rdir, size_t rdir_len)
    ++{
    ++	if (*entry->left)
    ++		write_file_in_directory(ldir, ldir_len, entry->path, entry->left);
    ++	if (*entry->right)
    ++		write_file_in_directory(rdir, rdir_len, entry->path, entry->right);
     +}
     +
      static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
    @@ builtin/difftool.c: static int run_dir_diff(const char *extcmd, int symlinks, co
     -			ensure_leading_directories(rdir.buf);
     -			write_file(rdir.buf, "%s", entry->right);
     -		}
    -+		write_entry(entry->path, entry->left, &ldir, ldir_len);
    -+		write_entry(entry->path, entry->right, &rdir, rdir_len);
    ++		write_standin_files(entry, &ldir, ldir_len, &rdir, rdir_len);
      	}
      
      	/*
    @@ builtin/difftool.c: static int run_dir_diff(const char *extcmd, int symlinks, co
     -			write_file(rdir.buf, "%s", entry->right);
     -		}
     +
    -+		write_entry(entry->path, entry->left, &ldir, ldir_len);
    -+		write_entry(entry->path, entry->right, &rdir, rdir_len);
    ++		write_standin_files(entry, &ldir, ldir_len, &rdir, rdir_len);
      	}
      
      	strbuf_release(&buf);
5:  8db6ae3373 ! 3:  94ad86157e difftool: remove an unnecessary call to strbuf_release()
    @@ Commit message
     
      ## builtin/difftool.c ##
     @@ builtin/difftool.c: static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
    - 		write_entry(entry->path, entry->right, &rdir, rdir_len);
    + 		write_standin_files(entry, &ldir, ldir_len, &rdir, rdir_len);
      	}
      
     -	strbuf_release(&buf);
2:  080a113917 = 4:  5b6dfe5e5c difftool: add a missing space to the run_dir_diff() comments
3:  1fbc47a58d < -:  ---------- difftool: avoid returning -1 to cmd_main() from run_dir_diff()
-- 
2.33.0.886.g5b6dfe5e5c


^ permalink raw reply	[flat|nested] 5+ messages in thread