* Re: [RFH - Tcl/Tk] use of procedure before declaration?
From: Johannes Schindelin @ 2017-01-17 11:29 UTC (permalink / raw)
To: Philip Oakley; +Cc: Git List, Pat Thoyts
In-Reply-To: <F9099DB3F0374D898776BD2621BF36FA@PhilipOakley>
Hi Philip,
On Mon, 16 Jan 2017, Philip Oakley wrote:
> In
> https://github.com/git/git/blob/master/git-gui/lib/choose_repository.tcl#L242
> the procedure `_unset_recentrepo` is called, however the procedure isn't
> declared until line 248. My reading of the various Tcl tutorials suggest
> (but not explictly) that this isn't the right way.
Indeed, calling a procedure before it is declared sounds incorrect.
Since documentation can be treacherous, let's just test it. With a `tclsh`
whose `$tcl_version` variable claims that this is version 8.6, this
script:
```tcl
hello Philip
proc hello {arg} {
puts "Hi, $arg"
}
```
... yields the error message:
invalid command name "hello"
while executing
"hello Philip"
... while this script:
```tcl
proc hello {arg} {
puts "Hi, $arg"
}
hello Philip
```
... prints the expected "Hi, Philip".
Having said that, in the code to which you linked, the procedure is not
actually called before it is declared, as the call is inside another
procedure.
Indeed, the entire file declares one object-oriented class, so no code
gets executed in that file:
https://github.com/git/git/blob/d7dffce1c/git-gui/lib/choose_repository.tcl#L4
(I guess proper indentation would make it easier to understand that this
file is defining a class, not executing anything yet).
And it is perfectly legitimate to use not-yet-declared procedures in other
procedures, otherwise recursion would not work.
> Should 3c6a287 ("git-gui: Keep repo_config(gui.recentrepos) and .gitconfig
> in sync", 2010-01-23) have declared `proc _unset_recentrepo {p}` before
> `proc _get_recentrepos {}` ?
Given the findings above, I believe that the patch is actually correct.
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH v5 3/7] tag: add format specifier to gpg_verify_tag
From: Jeff King @ 2017-01-17 15:24 UTC (permalink / raw)
To: santiago; +Cc: git, gitster, sunshine, walters, Lukas Puehringer
In-Reply-To: <20170115184705.10376-4-santiago@nyu.edu>
On Sun, Jan 15, 2017 at 01:47:01PM -0500, santiago@nyu.edu wrote:
> From: Lukas Puehringer <luk.puehringer@gmail.com>
>
> Calling functions for gpg_verify_tag() may desire to print relevant
> information about the header for further verification. Add an optional
> format argument to print any desired information after GPG verification.
Hrm. Maybe I am missing something, but what does:
verify_and_format_tag(sha1, name, fmt, flags);
get you over:
gpg_verify_tag(sha1, name, flags);
pretty_print_ref(name, sha1, fmt);
? The latter seems much more flexible, and I do not see how the
verification step impacts the printing at all (or vice versa).
I could understand it more if there were patches later in the series
that somehow used the format and verification results together. But I
didn't see that.
-Peff
^ permalink raw reply
* Re: [PATCH v5 3/7] tag: add format specifier to gpg_verify_tag
From: Jeff King @ 2017-01-17 15:30 UTC (permalink / raw)
To: santiago; +Cc: git, gitster, sunshine, walters, Lukas Puehringer
In-Reply-To: <20170117152455.k6zkeclsyawzpl2n@sigill.intra.peff.net>
On Tue, Jan 17, 2017 at 10:24:55AM -0500, Jeff King wrote:
> On Sun, Jan 15, 2017 at 01:47:01PM -0500, santiago@nyu.edu wrote:
>
> > From: Lukas Puehringer <luk.puehringer@gmail.com>
> >
> > Calling functions for gpg_verify_tag() may desire to print relevant
> > information about the header for further verification. Add an optional
> > format argument to print any desired information after GPG verification.
>
> Hrm. Maybe I am missing something, but what does:
>
> verify_and_format_tag(sha1, name, fmt, flags);
>
> get you over:
>
> gpg_verify_tag(sha1, name, flags);
> pretty_print_ref(name, sha1, fmt);
>
> ? The latter seems much more flexible, and I do not see how the
> verification step impacts the printing at all (or vice versa).
>
> I could understand it more if there were patches later in the series
> that somehow used the format and verification results together. But I
> didn't see that.
Having read through the rest of the series, it looks like you'd
sometimes have to do:
int ret;
ret = gpg_verify_tag(sha1, name, flags);
pretty_print_ref(name, sha1, fmt);
if (ret)
... do something ...
and this function lets you do it in a single line.
Still, I think I'd rather see it done as a wrapper than modifying
gpg_verify_tag().
-Peff
^ permalink raw reply
* fatal: bad revision 'git rm -r --ignore-unmatch -- folder'
From: jean-christophe manciot @ 2017-01-17 15:30 UTC (permalink / raw)
To: git
Hi there,
I'm trying to purge a complete folder and its files from the
repository history with:
git-Games# git filter-branch 'git rm -r --ignore-unmatch --
Ubuntu/16.04/' --tag-name-filter cat -- --all HEAD
fatal: bad revision 'git rm -r --ignore-unmatch -- Ubuntu/16.04/'
git does not find the folder although it's there:
git-Games# ll Ubuntu/16.04/
total 150316
drwxr-x--- 2 actionmystique actionmystique 4096 Nov 19 13:40 ./
drwxr-x--- 4 actionmystique actionmystique 4096 Oct 30 14:02 ../
-rwxr-x--- 1 actionmystique actionmystique 2190394 May 9 2016
residualvm_0.3.0~git-1_amd64.deb*
...
-rw-r--r-- 1 actionmystique actionmystique 67382492 Oct 13 09:15
scummvm-dbgsym_1.9.0_amd64.deb
What is going on?
--
Jean-Christophe
^ permalink raw reply
* Re: [PATCH v5 5/7] builtin/tag: add --format argument for tag -v
From: Jeff King @ 2017-01-17 15:34 UTC (permalink / raw)
To: santiago; +Cc: git, gitster, sunshine, walters, Lukas Puehringer
In-Reply-To: <20170115184705.10376-6-santiago@nyu.edu>
On Sun, Jan 15, 2017 at 01:47:03PM -0500, santiago@nyu.edu wrote:
> -static int for_each_tag_name(const char **argv, each_tag_name_fn fn)
> +static int for_each_tag_name(const char **argv, each_tag_name_fn fn,
> + void *cb_data)
> {
> const char **p;
> char ref[PATH_MAX];
> int had_error = 0;
> unsigned char sha1[20];
>
> +
> for (p = argv; *p; p++) {
> if (snprintf(ref, sizeof(ref), "refs/tags/%s", *p)
> >= sizeof(ref)) {
Funny extra line?
> {
> - return verify_and_format_tag(sha1, name, NULL, GPG_VERIFY_VERBOSE);
> + int flags;
> + char *fmt_pretty = cb_data;
> + flags = GPG_VERIFY_VERBOSE;
> +
> + if (fmt_pretty)
> + flags = GPG_VERIFY_QUIET;
> +
> + return verify_and_format_tag(sha1, ref, fmt_pretty, flags);
It seems funny that VERBOSE and QUIET are bit-flags. What happens when
you ask for GPG_VERIFY_VERBOSE|GPG_VERIFY_QUIET?
I suppose this is actually not a problem in _this_ patch, but in the
very first one that adds the QUIET flag. I'm not sure if the problem is
that the options should be more orthogonal, or that they are just badly
named to appear as opposites when they aren't.
-Peff
^ permalink raw reply
* Re: [PATCH v5 7/7] t/t7004-tag: Add --format specifier tests
From: Jeff King @ 2017-01-17 15:35 UTC (permalink / raw)
To: santiago; +Cc: git, gitster, sunshine, walters
In-Reply-To: <20170115184705.10376-8-santiago@nyu.edu>
On Sun, Jan 15, 2017 at 01:47:05PM -0500, santiago@nyu.edu wrote:
> From: Santiago Torres <santiago@nyu.edu>
>
> tag -v now supports --format specifiers to inspect the contents of a tag
> upon verification. Add two tests to ensure this behavior is respected in
> future changes.
>
> Signed-off-by: Santiago Torres <santiago@nyu.edu>
> ---
> t/t7004-tag.sh | 16 ++++++++++++++++
> 1 file changed, 16 insertions(+)
>
> diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
> index 07869b0c0..b2b81f203 100755
> --- a/t/t7004-tag.sh
> +++ b/t/t7004-tag.sh
> @@ -874,6 +874,22 @@ test_expect_success GPG 'verifying a forged tag should fail' '
> test_must_fail git tag -v forged-tag
> '
>
> +test_expect_success 'verifying a proper tag with --format pass and format accordingly' '
> + cat >expect <<-\EOF
Trailing whitespace after "EOF" here (and below). Otherwise the tests
look reasonable to me.
-Peff
^ permalink raw reply
* Re: [PATCH v5 0/7] Add --format to tag verification
From: Jeff King @ 2017-01-17 15:36 UTC (permalink / raw)
To: santiago; +Cc: git, gitster, sunshine, walters
In-Reply-To: <20170115184705.10376-1-santiago@nyu.edu>
On Sun, Jan 15, 2017 at 01:46:58PM -0500, santiago@nyu.edu wrote:
> From: Santiago Torres <santiago@nyu.edu>
>
> This is the fifth iteration of [1][2][3][4], and as a result of the
> discussion in [5]. The main goal of this patch series is to bring
> --format to git tag verification so that upper-layer tools can inspect
> the content of a tag and make decisions based on it.
Thanks for picking this back up. I didn't see any bugs, but I had a few
interface nits, which I sent inline.
-Peff
^ permalink raw reply
* Re: fatal: bad revision 'git rm -r --ignore-unmatch -- folder'
From: Jeff King @ 2017-01-17 15:38 UTC (permalink / raw)
To: jean-christophe manciot; +Cc: git
In-Reply-To: <CAKcFC3aqjLNUNKPDZ08rO_SBkY84uvHBerCxnSchAe8rH0dnMg@mail.gmail.com>
On Tue, Jan 17, 2017 at 04:30:48PM +0100, jean-christophe manciot wrote:
> I'm trying to purge a complete folder and its files from the
> repository history with:
>
> git-Games# git filter-branch 'git rm -r --ignore-unmatch --
> Ubuntu/16.04/' --tag-name-filter cat -- --all HEAD
> fatal: bad revision 'git rm -r --ignore-unmatch -- Ubuntu/16.04/'
Did you forget "--tree-filter" or "--index-filter" before the "git rm"
parameter? Without an option it will be interpreted as a refname, which
it obviously isn't.
-Peff
^ permalink raw reply
* [PATCH v5 3/3] Retire the scripted difftool
From: Johannes Schindelin @ 2017-01-17 15:55 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, David Aguilar, Dennis Kaarsemaker, Paul Sbarra
In-Reply-To: <cover.1484668473.git.johannes.schindelin@gmx.de>
It served its purpose, but now we have a builtin difftool. Time for the
Perl script to enjoy Florida.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
.gitignore | 1 -
Makefile | 1 -
builtin/difftool.c | 41 ----------
.../examples/git-difftool.perl | 0
git.c | 7 +-
t/t7800-difftool.sh | 94 +++++++++++-----------
6 files changed, 47 insertions(+), 97 deletions(-)
rename git-legacy-difftool.perl => contrib/examples/git-difftool.perl (100%)
diff --git a/.gitignore b/.gitignore
index 5555ae025b..6722f78f9a 100644
--- a/.gitignore
+++ b/.gitignore
@@ -76,7 +76,6 @@
/git-init-db
/git-interpret-trailers
/git-instaweb
-/git-legacy-difftool
/git-log
/git-ls-files
/git-ls-remote
diff --git a/Makefile b/Makefile
index 8cf5bef034..e9aa6ae57c 100644
--- a/Makefile
+++ b/Makefile
@@ -522,7 +522,6 @@ SCRIPT_LIB += git-sh-setup
SCRIPT_LIB += git-sh-i18n
SCRIPT_PERL += git-add--interactive.perl
-SCRIPT_PERL += git-legacy-difftool.perl
SCRIPT_PERL += git-archimport.perl
SCRIPT_PERL += git-cvsexportcommit.perl
SCRIPT_PERL += git-cvsimport.perl
diff --git a/builtin/difftool.c b/builtin/difftool.c
index 2115e548a5..42ad9e804a 100644
--- a/builtin/difftool.c
+++ b/builtin/difftool.c
@@ -616,30 +616,6 @@ static int run_file_diff(int prompt, const char *prefix,
exit(ret);
}
-/*
- * NEEDSWORK: this function can go once the legacy-difftool Perl script is
- * retired.
- *
- * We intentionally avoid reading the config directly here, to avoid messing up
- * the GIT_* environment variables when we need to fall back to exec()ing the
- * Perl script.
- */
-static int use_builtin_difftool(void) {
- struct child_process cp = CHILD_PROCESS_INIT;
- struct strbuf out = STRBUF_INIT;
- int ret;
-
- argv_array_pushl(&cp.args,
- "config", "--bool", "difftool.usebuiltin", NULL);
- cp.git_cmd = 1;
- if (capture_command(&cp, &out, 6))
- return 0;
- strbuf_trim(&out);
- ret = !strcmp("true", out.buf);
- strbuf_release(&out);
- return ret;
-}
-
int cmd_difftool(int argc, const char **argv, const char *prefix)
{
int use_gui_tool = 0, dir_diff = 0, prompt = -1, symlinks = 0,
@@ -671,23 +647,6 @@ int cmd_difftool(int argc, const char **argv, const char *prefix)
OPT_END()
};
- /*
- * NEEDSWORK: Once the builtin difftool has been tested enough
- * and git-legacy-difftool.perl is retired to contrib/, this preamble
- * can be removed.
- */
- if (!use_builtin_difftool()) {
- const char *path = mkpath("%s/git-legacy-difftool",
- git_exec_path());
-
- if (sane_execvp(path, (char **)argv) < 0)
- die_errno("could not exec %s", path);
-
- return 0;
- }
- prefix = setup_git_directory();
- trace_repo_setup(prefix);
- setup_work_tree();
/* NEEDSWORK: once we no longer spawn anything, remove this */
setenv(GIT_DIR_ENVIRONMENT, absolute_path(get_git_dir()), 1);
setenv(GIT_WORK_TREE_ENVIRONMENT, absolute_path(get_git_work_tree()), 1);
diff --git a/git-legacy-difftool.perl b/contrib/examples/git-difftool.perl
similarity index 100%
rename from git-legacy-difftool.perl
rename to contrib/examples/git-difftool.perl
diff --git a/git.c b/git.c
index c58181e5ef..bd4d668a21 100644
--- a/git.c
+++ b/git.c
@@ -424,12 +424,7 @@ static struct cmd_struct commands[] = {
{ "diff-files", cmd_diff_files, RUN_SETUP | NEED_WORK_TREE },
{ "diff-index", cmd_diff_index, RUN_SETUP },
{ "diff-tree", cmd_diff_tree, RUN_SETUP },
- /*
- * NEEDSWORK: Once the redirection to git-legacy-difftool.perl in
- * builtin/difftool.c has been removed, this entry should be changed to
- * RUN_SETUP | NEED_WORK_TREE
- */
- { "difftool", cmd_difftool },
+ { "difftool", cmd_difftool, RUN_SETUP | NEED_WORK_TREE },
{ "fast-export", cmd_fast_export, RUN_SETUP },
{ "fetch", cmd_fetch, RUN_SETUP },
{ "fetch-pack", cmd_fetch_pack, RUN_SETUP },
diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index e94910c563..aa0ef02597 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -23,10 +23,8 @@ prompt_given ()
test "$prompt" = "Launch 'test-tool' [Y/n]? branch"
}
-# NEEDSWORK: lose all the PERL prereqs once legacy-difftool is retired.
-
# Create a file on master and change it on branch
-test_expect_success PERL 'setup' '
+test_expect_success 'setup' '
echo master >file &&
git add file &&
git commit -m "added file" &&
@@ -38,7 +36,7 @@ test_expect_success PERL 'setup' '
'
# Configure a custom difftool.<tool>.cmd and use it
-test_expect_success PERL 'custom commands' '
+test_expect_success 'custom commands' '
difftool_test_setup &&
test_config difftool.test-tool.cmd "cat \"\$REMOTE\"" &&
echo master >expect &&
@@ -51,21 +49,21 @@ test_expect_success PERL 'custom commands' '
test_cmp expect actual
'
-test_expect_success PERL 'custom tool commands override built-ins' '
+test_expect_success 'custom tool commands override built-ins' '
test_config difftool.vimdiff.cmd "cat \"\$REMOTE\"" &&
echo master >expect &&
git difftool --tool vimdiff --no-prompt branch >actual &&
test_cmp expect actual
'
-test_expect_success PERL 'difftool ignores bad --tool values' '
+test_expect_success 'difftool ignores bad --tool values' '
: >expect &&
test_must_fail \
git difftool --no-prompt --tool=bad-tool branch >actual &&
test_cmp expect actual
'
-test_expect_success PERL 'difftool forwards arguments to diff' '
+test_expect_success 'difftool forwards arguments to diff' '
difftool_test_setup &&
>for-diff &&
git add for-diff &&
@@ -78,40 +76,40 @@ test_expect_success PERL 'difftool forwards arguments to diff' '
rm for-diff
'
-test_expect_success PERL 'difftool ignores exit code' '
+test_expect_success 'difftool ignores exit code' '
test_config difftool.error.cmd false &&
git difftool -y -t error branch
'
-test_expect_success PERL 'difftool forwards exit code with --trust-exit-code' '
+test_expect_success 'difftool forwards exit code with --trust-exit-code' '
test_config difftool.error.cmd false &&
test_must_fail git difftool -y --trust-exit-code -t error branch
'
-test_expect_success PERL 'difftool forwards exit code with --trust-exit-code for built-ins' '
+test_expect_success 'difftool forwards exit code with --trust-exit-code for built-ins' '
test_config difftool.vimdiff.path false &&
test_must_fail git difftool -y --trust-exit-code -t vimdiff branch
'
-test_expect_success PERL 'difftool honors difftool.trustExitCode = true' '
+test_expect_success 'difftool honors difftool.trustExitCode = true' '
test_config difftool.error.cmd false &&
test_config difftool.trustExitCode true &&
test_must_fail git difftool -y -t error branch
'
-test_expect_success PERL 'difftool honors difftool.trustExitCode = false' '
+test_expect_success 'difftool honors difftool.trustExitCode = false' '
test_config difftool.error.cmd false &&
test_config difftool.trustExitCode false &&
git difftool -y -t error branch
'
-test_expect_success PERL 'difftool ignores exit code with --no-trust-exit-code' '
+test_expect_success 'difftool ignores exit code with --no-trust-exit-code' '
test_config difftool.error.cmd false &&
test_config difftool.trustExitCode true &&
git difftool -y --no-trust-exit-code -t error branch
'
-test_expect_success PERL 'difftool stops on error with --trust-exit-code' '
+test_expect_success 'difftool stops on error with --trust-exit-code' '
test_when_finished "rm -f for-diff .git/fail-right-file" &&
test_when_finished "git reset -- for-diff" &&
write_script .git/fail-right-file <<-\EOF &&
@@ -126,13 +124,13 @@ test_expect_success PERL 'difftool stops on error with --trust-exit-code' '
test_cmp expect actual
'
-test_expect_success PERL 'difftool honors exit status if command not found' '
+test_expect_success 'difftool honors exit status if command not found' '
test_config difftool.nonexistent.cmd i-dont-exist &&
test_config difftool.trustExitCode false &&
test_must_fail git difftool -y -t nonexistent branch
'
-test_expect_success PERL 'difftool honors --gui' '
+test_expect_success 'difftool honors --gui' '
difftool_test_setup &&
test_config merge.tool bogus-tool &&
test_config diff.tool bogus-tool &&
@@ -143,7 +141,7 @@ test_expect_success PERL 'difftool honors --gui' '
test_cmp expect actual
'
-test_expect_success PERL 'difftool --gui last setting wins' '
+test_expect_success 'difftool --gui last setting wins' '
difftool_test_setup &&
: >expect &&
git difftool --no-prompt --gui --no-gui >actual &&
@@ -157,7 +155,7 @@ test_expect_success PERL 'difftool --gui last setting wins' '
test_cmp expect actual
'
-test_expect_success PERL 'difftool --gui works without configured diff.guitool' '
+test_expect_success 'difftool --gui works without configured diff.guitool' '
difftool_test_setup &&
echo branch >expect &&
git difftool --no-prompt --gui branch >actual &&
@@ -165,7 +163,7 @@ test_expect_success PERL 'difftool --gui works without configured diff.guitool'
'
# Specify the diff tool using $GIT_DIFF_TOOL
-test_expect_success PERL 'GIT_DIFF_TOOL variable' '
+test_expect_success 'GIT_DIFF_TOOL variable' '
difftool_test_setup &&
git config --unset diff.tool &&
echo branch >expect &&
@@ -175,7 +173,7 @@ test_expect_success PERL 'GIT_DIFF_TOOL variable' '
# Test the $GIT_*_TOOL variables and ensure
# that $GIT_DIFF_TOOL always wins unless --tool is specified
-test_expect_success PERL 'GIT_DIFF_TOOL overrides' '
+test_expect_success 'GIT_DIFF_TOOL overrides' '
difftool_test_setup &&
test_config diff.tool bogus-tool &&
test_config merge.tool bogus-tool &&
@@ -193,7 +191,7 @@ test_expect_success PERL 'GIT_DIFF_TOOL overrides' '
# Test that we don't have to pass --no-prompt to difftool
# when $GIT_DIFFTOOL_NO_PROMPT is true
-test_expect_success PERL 'GIT_DIFFTOOL_NO_PROMPT variable' '
+test_expect_success 'GIT_DIFFTOOL_NO_PROMPT variable' '
difftool_test_setup &&
echo branch >expect &&
GIT_DIFFTOOL_NO_PROMPT=true git difftool branch >actual &&
@@ -202,7 +200,7 @@ test_expect_success PERL 'GIT_DIFFTOOL_NO_PROMPT variable' '
# git-difftool supports the difftool.prompt variable.
# Test that GIT_DIFFTOOL_PROMPT can override difftool.prompt = false
-test_expect_success PERL 'GIT_DIFFTOOL_PROMPT variable' '
+test_expect_success 'GIT_DIFFTOOL_PROMPT variable' '
difftool_test_setup &&
test_config difftool.prompt false &&
echo >input &&
@@ -212,7 +210,7 @@ test_expect_success PERL 'GIT_DIFFTOOL_PROMPT variable' '
'
# Test that we don't have to pass --no-prompt when difftool.prompt is false
-test_expect_success PERL 'difftool.prompt config variable is false' '
+test_expect_success 'difftool.prompt config variable is false' '
difftool_test_setup &&
test_config difftool.prompt false &&
echo branch >expect &&
@@ -221,7 +219,7 @@ test_expect_success PERL 'difftool.prompt config variable is false' '
'
# Test that we don't have to pass --no-prompt when mergetool.prompt is false
-test_expect_success PERL 'difftool merge.prompt = false' '
+test_expect_success 'difftool merge.prompt = false' '
difftool_test_setup &&
test_might_fail git config --unset difftool.prompt &&
test_config mergetool.prompt false &&
@@ -231,7 +229,7 @@ test_expect_success PERL 'difftool merge.prompt = false' '
'
# Test that the -y flag can override difftool.prompt = true
-test_expect_success PERL 'difftool.prompt can overridden with -y' '
+test_expect_success 'difftool.prompt can overridden with -y' '
difftool_test_setup &&
test_config difftool.prompt true &&
echo branch >expect &&
@@ -240,7 +238,7 @@ test_expect_success PERL 'difftool.prompt can overridden with -y' '
'
# Test that the --prompt flag can override difftool.prompt = false
-test_expect_success PERL 'difftool.prompt can overridden with --prompt' '
+test_expect_success 'difftool.prompt can overridden with --prompt' '
difftool_test_setup &&
test_config difftool.prompt false &&
echo >input &&
@@ -250,7 +248,7 @@ test_expect_success PERL 'difftool.prompt can overridden with --prompt' '
'
# Test that the last flag passed on the command-line wins
-test_expect_success PERL 'difftool last flag wins' '
+test_expect_success 'difftool last flag wins' '
difftool_test_setup &&
echo branch >expect &&
git difftool --prompt --no-prompt branch >actual &&
@@ -263,7 +261,7 @@ test_expect_success PERL 'difftool last flag wins' '
# git-difftool falls back to git-mergetool config variables
# so test that behavior here
-test_expect_success PERL 'difftool + mergetool config variables' '
+test_expect_success 'difftool + mergetool config variables' '
test_config merge.tool test-tool &&
test_config mergetool.test-tool.cmd "cat \$LOCAL" &&
echo branch >expect &&
@@ -277,49 +275,49 @@ test_expect_success PERL 'difftool + mergetool config variables' '
test_cmp expect actual
'
-test_expect_success PERL 'difftool.<tool>.path' '
+test_expect_success 'difftool.<tool>.path' '
test_config difftool.tkdiff.path echo &&
git difftool --tool=tkdiff --no-prompt branch >output &&
lines=$(grep file output | wc -l) &&
test "$lines" -eq 1
'
-test_expect_success PERL 'difftool --extcmd=cat' '
+test_expect_success 'difftool --extcmd=cat' '
echo branch >expect &&
echo master >>expect &&
git difftool --no-prompt --extcmd=cat branch >actual &&
test_cmp expect actual
'
-test_expect_success PERL 'difftool --extcmd cat' '
+test_expect_success 'difftool --extcmd cat' '
echo branch >expect &&
echo master >>expect &&
git difftool --no-prompt --extcmd=cat branch >actual &&
test_cmp expect actual
'
-test_expect_success PERL 'difftool -x cat' '
+test_expect_success 'difftool -x cat' '
echo branch >expect &&
echo master >>expect &&
git difftool --no-prompt -x cat branch >actual &&
test_cmp expect actual
'
-test_expect_success PERL 'difftool --extcmd echo arg1' '
+test_expect_success 'difftool --extcmd echo arg1' '
echo file >expect &&
git difftool --no-prompt \
--extcmd sh\ -c\ \"echo\ \$1\" branch >actual &&
test_cmp expect actual
'
-test_expect_success PERL 'difftool --extcmd cat arg1' '
+test_expect_success 'difftool --extcmd cat arg1' '
echo master >expect &&
git difftool --no-prompt \
--extcmd sh\ -c\ \"cat\ \$1\" branch >actual &&
test_cmp expect actual
'
-test_expect_success PERL 'difftool --extcmd cat arg2' '
+test_expect_success 'difftool --extcmd cat arg2' '
echo branch >expect &&
git difftool --no-prompt \
--extcmd sh\ -c\ \"cat\ \$2\" branch >actual &&
@@ -327,7 +325,7 @@ test_expect_success PERL 'difftool --extcmd cat arg2' '
'
# Create a second file on master and a different version on branch
-test_expect_success PERL 'setup with 2 files different' '
+test_expect_success 'setup with 2 files different' '
echo m2 >file2 &&
git add file2 &&
git commit -m "added file2" &&
@@ -339,7 +337,7 @@ test_expect_success PERL 'setup with 2 files different' '
git checkout master
'
-test_expect_success PERL 'say no to the first file' '
+test_expect_success 'say no to the first file' '
(echo n && echo) >input &&
git difftool -x cat branch <input >output &&
grep m2 output &&
@@ -348,7 +346,7 @@ test_expect_success PERL 'say no to the first file' '
! grep branch output
'
-test_expect_success PERL 'say no to the second file' '
+test_expect_success 'say no to the second file' '
(echo && echo n) >input &&
git difftool -x cat branch <input >output &&
grep master output &&
@@ -357,7 +355,7 @@ test_expect_success PERL 'say no to the second file' '
! grep br2 output
'
-test_expect_success PERL 'ending prompt input with EOF' '
+test_expect_success 'ending prompt input with EOF' '
git difftool -x cat branch </dev/null >output &&
! grep master output &&
! grep branch output &&
@@ -365,12 +363,12 @@ test_expect_success PERL 'ending prompt input with EOF' '
! grep br2 output
'
-test_expect_success PERL 'difftool --tool-help' '
+test_expect_success 'difftool --tool-help' '
git difftool --tool-help >output &&
grep tool output
'
-test_expect_success PERL 'setup change in subdirectory' '
+test_expect_success 'setup change in subdirectory' '
git checkout master &&
mkdir sub &&
echo master >sub/sub &&
@@ -384,11 +382,11 @@ test_expect_success PERL 'setup change in subdirectory' '
'
run_dir_diff_test () {
- test_expect_success PERL "$1 --no-symlinks" "
+ test_expect_success "$1 --no-symlinks" "
symlinks=--no-symlinks &&
$2
"
- test_expect_success PERL,SYMLINKS "$1 --symlinks" "
+ test_expect_success SYMLINKS "$1 --symlinks" "
symlinks=--symlinks &&
$2
"
@@ -510,7 +508,7 @@ do
done >actual
EOF
-test_expect_success PERL,SYMLINKS 'difftool --dir-diff --symlink without unstaged changes' '
+test_expect_success SYMLINKS 'difftool --dir-diff --symlink without unstaged changes' '
cat >expect <<-EOF &&
file
$PWD/file
@@ -547,7 +545,7 @@ write_script modify-file <<\EOF
echo "new content" >file
EOF
-test_expect_success PERL 'difftool --no-symlinks does not overwrite working tree file ' '
+test_expect_success 'difftool --no-symlinks does not overwrite working tree file ' '
echo "orig content" >file &&
git difftool --dir-diff --no-symlinks --extcmd "$PWD/modify-file" branch &&
echo "new content" >expect &&
@@ -560,7 +558,7 @@ echo "tmp content" >"$2/file" &&
echo "$2" >tmpdir
EOF
-test_expect_success PERL 'difftool --no-symlinks detects conflict ' '
+test_expect_success 'difftool --no-symlinks detects conflict ' '
(
TMPDIR=$TRASH_DIRECTORY &&
export TMPDIR &&
@@ -573,7 +571,7 @@ test_expect_success PERL 'difftool --no-symlinks detects conflict ' '
)
'
-test_expect_success PERL 'difftool properly honors gitlink and core.worktree' '
+test_expect_success 'difftool properly honors gitlink and core.worktree' '
git submodule add ./. submod/ule &&
test_config -C submod/ule diff.tool checktrees &&
test_config -C submod/ule difftool.checktrees.cmd '\''
@@ -587,7 +585,7 @@ test_expect_success PERL 'difftool properly honors gitlink and core.worktree' '
)
'
-test_expect_success PERL,SYMLINKS 'difftool --dir-diff symlinked directories' '
+test_expect_success SYMLINKS 'difftool --dir-diff symlinked directories' '
git init dirlinks &&
(
cd dirlinks &&
--
2.11.0.windows.3
^ permalink raw reply related
* [PATCH v5 1/3] difftool: add a skeleton for the upcoming builtin
From: Johannes Schindelin @ 2017-01-17 15:54 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, David Aguilar, Dennis Kaarsemaker, Paul Sbarra
In-Reply-To: <cover.1484668473.git.johannes.schindelin@gmx.de>
This adds a builtin difftool that still falls back to the legacy Perl
version, which has been renamed to `legacy-difftool`.
The idea is that the new, experimental, builtin difftool immediately hands
off to the legacy difftool for now, unless the config variable
difftool.useBuiltin is set to true.
This feature flag will be used in the upcoming Git for Windows v2.11.0
release, to allow early testers to opt-in to use the builtin difftool and
flesh out any bugs.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
.gitignore | 1 +
Makefile | 3 +-
builtin.h | 1 +
builtin/difftool.c | 63 +++++++++++++++++++++++++++
git-difftool.perl => git-legacy-difftool.perl | 0
git.c | 6 +++
t/t7800-difftool.sh | 2 +
7 files changed, 75 insertions(+), 1 deletion(-)
create mode 100644 builtin/difftool.c
rename git-difftool.perl => git-legacy-difftool.perl (100%)
diff --git a/.gitignore b/.gitignore
index 6722f78f9a..5555ae025b 100644
--- a/.gitignore
+++ b/.gitignore
@@ -76,6 +76,7 @@
/git-init-db
/git-interpret-trailers
/git-instaweb
+/git-legacy-difftool
/git-log
/git-ls-files
/git-ls-remote
diff --git a/Makefile b/Makefile
index d861bd9985..8cf5bef034 100644
--- a/Makefile
+++ b/Makefile
@@ -522,7 +522,7 @@ SCRIPT_LIB += git-sh-setup
SCRIPT_LIB += git-sh-i18n
SCRIPT_PERL += git-add--interactive.perl
-SCRIPT_PERL += git-difftool.perl
+SCRIPT_PERL += git-legacy-difftool.perl
SCRIPT_PERL += git-archimport.perl
SCRIPT_PERL += git-cvsexportcommit.perl
SCRIPT_PERL += git-cvsimport.perl
@@ -883,6 +883,7 @@ BUILTIN_OBJS += builtin/diff-files.o
BUILTIN_OBJS += builtin/diff-index.o
BUILTIN_OBJS += builtin/diff-tree.o
BUILTIN_OBJS += builtin/diff.o
+BUILTIN_OBJS += builtin/difftool.o
BUILTIN_OBJS += builtin/fast-export.o
BUILTIN_OBJS += builtin/fetch-pack.o
BUILTIN_OBJS += builtin/fetch.o
diff --git a/builtin.h b/builtin.h
index b9122bc5f4..67f80519da 100644
--- a/builtin.h
+++ b/builtin.h
@@ -60,6 +60,7 @@ extern int cmd_diff_files(int argc, const char **argv, const char *prefix);
extern int cmd_diff_index(int argc, const char **argv, const char *prefix);
extern int cmd_diff(int argc, const char **argv, const char *prefix);
extern int cmd_diff_tree(int argc, const char **argv, const char *prefix);
+extern int cmd_difftool(int argc, const char **argv, const char *prefix);
extern int cmd_fast_export(int argc, const char **argv, const char *prefix);
extern int cmd_fetch(int argc, const char **argv, const char *prefix);
extern int cmd_fetch_pack(int argc, const char **argv, const char *prefix);
diff --git a/builtin/difftool.c b/builtin/difftool.c
new file mode 100644
index 0000000000..53870bbaf7
--- /dev/null
+++ b/builtin/difftool.c
@@ -0,0 +1,63 @@
+/*
+ * "git difftool" builtin command
+ *
+ * This is a wrapper around the GIT_EXTERNAL_DIFF-compatible
+ * git-difftool--helper script.
+ *
+ * This script exports GIT_EXTERNAL_DIFF and GIT_PAGER for use by git.
+ * The GIT_DIFF* variables are exported for use by git-difftool--helper.
+ *
+ * Any arguments that are unknown to this script are forwarded to 'git diff'.
+ *
+ * Copyright (C) 2016 Johannes Schindelin
+ */
+#include "builtin.h"
+#include "run-command.h"
+#include "exec_cmd.h"
+
+/*
+ * NEEDSWORK: this function can go once the legacy-difftool Perl script is
+ * retired.
+ *
+ * We intentionally avoid reading the config directly here, to avoid messing up
+ * the GIT_* environment variables when we need to fall back to exec()ing the
+ * Perl script.
+ */
+static int use_builtin_difftool(void) {
+ struct child_process cp = CHILD_PROCESS_INIT;
+ struct strbuf out = STRBUF_INIT;
+ int ret;
+
+ argv_array_pushl(&cp.args,
+ "config", "--bool", "difftool.usebuiltin", NULL);
+ cp.git_cmd = 1;
+ if (capture_command(&cp, &out, 6))
+ return 0;
+ strbuf_trim(&out);
+ ret = !strcmp("true", out.buf);
+ strbuf_release(&out);
+ return ret;
+}
+
+int cmd_difftool(int argc, const char **argv, const char *prefix)
+{
+ /*
+ * NEEDSWORK: Once the builtin difftool has been tested enough
+ * and git-legacy-difftool.perl is retired to contrib/, this preamble
+ * can be removed.
+ */
+ if (!use_builtin_difftool()) {
+ const char *path = mkpath("%s/git-legacy-difftool",
+ git_exec_path());
+
+ if (sane_execvp(path, (char **)argv) < 0)
+ die_errno("could not exec %s", path);
+
+ return 0;
+ }
+ prefix = setup_git_directory();
+ trace_repo_setup(prefix);
+ setup_work_tree();
+
+ die("TODO");
+}
diff --git a/git-difftool.perl b/git-legacy-difftool.perl
similarity index 100%
rename from git-difftool.perl
rename to git-legacy-difftool.perl
diff --git a/git.c b/git.c
index bbaa949e9c..c58181e5ef 100644
--- a/git.c
+++ b/git.c
@@ -424,6 +424,12 @@ static struct cmd_struct commands[] = {
{ "diff-files", cmd_diff_files, RUN_SETUP | NEED_WORK_TREE },
{ "diff-index", cmd_diff_index, RUN_SETUP },
{ "diff-tree", cmd_diff_tree, RUN_SETUP },
+ /*
+ * NEEDSWORK: Once the redirection to git-legacy-difftool.perl in
+ * builtin/difftool.c has been removed, this entry should be changed to
+ * RUN_SETUP | NEED_WORK_TREE
+ */
+ { "difftool", cmd_difftool },
{ "fast-export", cmd_fast_export, RUN_SETUP },
{ "fetch", cmd_fetch, RUN_SETUP },
{ "fetch-pack", cmd_fetch_pack, RUN_SETUP },
diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index 99d4123461..e94910c563 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -23,6 +23,8 @@ prompt_given ()
test "$prompt" = "Launch 'test-tool' [Y/n]? branch"
}
+# NEEDSWORK: lose all the PERL prereqs once legacy-difftool is retired.
+
# Create a file on master and change it on branch
test_expect_success PERL 'setup' '
echo master >file &&
--
2.11.0.windows.3
^ permalink raw reply related
* [PATCH v5 2/3] difftool: implement the functionality in the builtin
From: Johannes Schindelin @ 2017-01-17 15:55 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, David Aguilar, Dennis Kaarsemaker, Paul Sbarra
In-Reply-To: <cover.1484668473.git.johannes.schindelin@gmx.de>
This patch gives life to the skeleton added in the previous patch.
The motivation for converting the difftool is that Perl scripts are not at
all native on Windows, and that `git difftool` therefore is pretty slow on
that platform, when there is no good reason for it to be slow.
In addition, Perl does not really have access to Git's internals. That
means that any script will always have to jump through unnecessary
hoops.
The current version of the builtin difftool does not, however, make full
use of the internals but instead chooses to spawn a couple of Git
processes, still, to make for an easier conversion. There remains a lot
of room for improvement, left for a later date.
Note: to play it safe, the original difftool is still called unless the
config setting difftool.useBuiltin is set to true.
The reason: this new, experimental, builtin difftool will be shipped as
part of Git for Windows v2.11.0, to allow for easier large-scale
testing, but of course as an opt-in feature.
Sadly, the speedup is more noticable on Linux than on Windows: a quick
test shows that t7800-difftool.sh runs in (2.183s/0.052s/0.108s)
(real/user/sys) in a Linux VM, down from (6.529s/3.112s/0.644s), while
on Windows, it is (36.064s/2.730s/7.194s), down from
(47.637s/2.407s/6.863s). The culprit is most likely the overhead
incurred from *still* having to shell out to mergetool-lib.sh and
difftool--helper.sh.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
builtin/difftool.c | 672 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 671 insertions(+), 1 deletion(-)
diff --git a/builtin/difftool.c b/builtin/difftool.c
index 53870bbaf7..2115e548a5 100644
--- a/builtin/difftool.c
+++ b/builtin/difftool.c
@@ -11,9 +11,610 @@
*
* Copyright (C) 2016 Johannes Schindelin
*/
+#include "cache.h"
#include "builtin.h"
#include "run-command.h"
#include "exec_cmd.h"
+#include "parse-options.h"
+#include "argv-array.h"
+#include "strbuf.h"
+#include "lockfile.h"
+#include "dir.h"
+
+static char *diff_gui_tool;
+static int trust_exit_code;
+
+static const char *const builtin_difftool_usage[] = {
+ N_("git difftool [<options>] [<commit> [<commit>]] [--] [<path>...]"),
+ NULL
+};
+
+static int difftool_config(const char *var, const char *value, void *cb)
+{
+ if (!strcmp(var, "diff.guitool")) {
+ diff_gui_tool = xstrdup(value);
+ return 0;
+ }
+
+ if (!strcmp(var, "difftool.trustexitcode")) {
+ trust_exit_code = git_config_bool(var, value);
+ return 0;
+ }
+
+ return git_default_config(var, value, cb);
+}
+
+static int print_tool_help(void)
+{
+ const char *argv[] = { "mergetool", "--tool-help=diff", NULL };
+ return run_command_v_opt(argv, RUN_GIT_CMD);
+}
+
+static int parse_index_info(char *p, int *mode1, int *mode2,
+ struct object_id *oid1, struct object_id *oid2,
+ char *status)
+{
+ if (*p != ':')
+ return error("expected ':', got '%c'", *p);
+ *mode1 = (int)strtol(p + 1, &p, 8);
+ if (*p != ' ')
+ return error("expected ' ', got '%c'", *p);
+ *mode2 = (int)strtol(p + 1, &p, 8);
+ if (*p != ' ')
+ return error("expected ' ', got '%c'", *p);
+ if (get_oid_hex(++p, oid1))
+ return error("expected object ID, got '%s'", p + 1);
+ p += GIT_SHA1_HEXSZ;
+ if (*p != ' ')
+ return error("expected ' ', got '%c'", *p);
+ if (get_oid_hex(++p, oid2))
+ return error("expected object ID, got '%s'", p + 1);
+ p += GIT_SHA1_HEXSZ;
+ if (*p != ' ')
+ return error("expected ' ', got '%c'", *p);
+ *status = *++p;
+ if (!*status)
+ return error("missing status");
+ if (p[1] && !isdigit(p[1]))
+ return error("unexpected trailer: '%s'", p + 1);
+ return 0;
+}
+
+/*
+ * Remove any trailing slash from $workdir
+ * before starting to avoid double slashes in symlink targets.
+ */
+static void add_path(struct strbuf *buf, size_t base_len, const char *path)
+{
+ strbuf_setlen(buf, base_len);
+ if (buf->len && buf->buf[buf->len - 1] != '/')
+ strbuf_addch(buf, '/');
+ strbuf_addstr(buf, path);
+}
+
+/*
+ * Determine whether we can simply reuse the file in the worktree.
+ */
+static int use_wt_file(const char *workdir, const char *name,
+ struct object_id *oid)
+{
+ struct strbuf buf = STRBUF_INIT;
+ struct stat st;
+ int use = 0;
+
+ strbuf_addstr(&buf, workdir);
+ add_path(&buf, buf.len, name);
+
+ if (!lstat(buf.buf, &st) && !S_ISLNK(st.st_mode)) {
+ struct object_id wt_oid;
+ int fd = open(buf.buf, O_RDONLY);
+
+ if (fd >= 0 &&
+ !index_fd(wt_oid.hash, fd, &st, OBJ_BLOB, name, 0)) {
+ if (is_null_oid(oid)) {
+ oidcpy(oid, &wt_oid);
+ use = 1;
+ } else if (!oidcmp(oid, &wt_oid))
+ use = 1;
+ }
+ }
+
+ strbuf_release(&buf);
+
+ return use;
+}
+
+struct working_tree_entry {
+ struct hashmap_entry entry;
+ char path[FLEX_ARRAY];
+};
+
+static int working_tree_entry_cmp(struct working_tree_entry *a,
+ struct working_tree_entry *b, void *keydata)
+{
+ return strcmp(a->path, b->path);
+}
+
+/*
+ * The `left` and `right` entries hold paths for the symlinks hashmap,
+ * and a SHA-1 surrounded by brief text for submodules.
+ */
+struct pair_entry {
+ struct hashmap_entry entry;
+ char left[PATH_MAX], right[PATH_MAX];
+ const char path[FLEX_ARRAY];
+};
+
+static int pair_cmp(struct pair_entry *a, struct pair_entry *b, void *keydata)
+{
+ return strcmp(a->path, b->path);
+}
+
+static void add_left_or_right(struct hashmap *map, const char *path,
+ const char *content, int is_right)
+{
+ struct pair_entry *e, *existing;
+
+ FLEX_ALLOC_STR(e, path, path);
+ hashmap_entry_init(e, strhash(path));
+ existing = hashmap_get(map, e, NULL);
+ if (existing) {
+ free(e);
+ e = existing;
+ } else {
+ e->left[0] = e->right[0] = '\0';
+ hashmap_add(map, e);
+ }
+ strlcpy(is_right ? e->right : e->left, content, PATH_MAX);
+}
+
+struct path_entry {
+ struct hashmap_entry entry;
+ char path[FLEX_ARRAY];
+};
+
+static int path_entry_cmp(struct path_entry *a, struct path_entry *b, void *key)
+{
+ return strcmp(a->path, key ? key : b->path);
+}
+
+static void changed_files(struct hashmap *result, const char *index_path,
+ const char *workdir)
+{
+ struct child_process update_index = CHILD_PROCESS_INIT;
+ struct child_process diff_files = CHILD_PROCESS_INIT;
+ struct strbuf index_env = STRBUF_INIT, buf = STRBUF_INIT;
+ const char *git_dir = absolute_path(get_git_dir()), *env[] = {
+ NULL, NULL
+ };
+ FILE *fp;
+
+ strbuf_addf(&index_env, "GIT_INDEX_FILE=%s", index_path);
+ env[0] = index_env.buf;
+
+ argv_array_pushl(&update_index.args,
+ "--git-dir", git_dir, "--work-tree", workdir,
+ "update-index", "--really-refresh", "-q",
+ "--unmerged", NULL);
+ update_index.no_stdin = 1;
+ update_index.no_stdout = 1;
+ update_index.no_stderr = 1;
+ update_index.git_cmd = 1;
+ update_index.use_shell = 0;
+ update_index.clean_on_exit = 1;
+ update_index.dir = workdir;
+ update_index.env = env;
+ /* Ignore any errors of update-index */
+ run_command(&update_index);
+
+ argv_array_pushl(&diff_files.args,
+ "--git-dir", git_dir, "--work-tree", workdir,
+ "diff-files", "--name-only", "-z", NULL);
+ diff_files.no_stdin = 1;
+ diff_files.git_cmd = 1;
+ diff_files.use_shell = 0;
+ diff_files.clean_on_exit = 1;
+ diff_files.out = -1;
+ diff_files.dir = workdir;
+ diff_files.env = env;
+ if (start_command(&diff_files))
+ die("could not obtain raw diff");
+ fp = xfdopen(diff_files.out, "r");
+ while (!strbuf_getline_nul(&buf, fp)) {
+ struct path_entry *entry;
+ FLEX_ALLOC_STR(entry, path, buf.buf);
+ hashmap_entry_init(entry, strhash(buf.buf));
+ hashmap_add(result, entry);
+ }
+ if (finish_command(&diff_files))
+ die("diff-files did not exit properly");
+ strbuf_release(&index_env);
+ strbuf_release(&buf);
+}
+
+static NORETURN void exit_cleanup(const char *tmpdir, int exit_code)
+{
+ struct strbuf buf = STRBUF_INIT;
+ strbuf_addstr(&buf, tmpdir);
+ remove_dir_recursively(&buf, 0);
+ if (exit_code)
+ warning(_("failed: %d"), exit_code);
+ exit(exit_code);
+}
+
+static int ensure_leading_directories(char *path)
+{
+ switch (safe_create_leading_directories(path)) {
+ case SCLD_OK:
+ case SCLD_EXISTS:
+ return 0;
+ default:
+ return error(_("could not create leading directories "
+ "of '%s'"), path);
+ }
+}
+
+static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
+ int argc, const char **argv)
+{
+ char tmpdir[PATH_MAX];
+ struct strbuf info = STRBUF_INIT, lpath = STRBUF_INIT;
+ struct strbuf rpath = STRBUF_INIT, buf = STRBUF_INIT;
+ struct strbuf ldir = STRBUF_INIT, rdir = STRBUF_INIT;
+ struct strbuf wtdir = STRBUF_INIT;
+ size_t ldir_len, rdir_len, wtdir_len;
+ struct cache_entry *ce = xcalloc(1, sizeof(ce) + PATH_MAX + 1);
+ const char *workdir, *tmp;
+ int ret = 0, i;
+ FILE *fp;
+ struct hashmap working_tree_dups, submodules, symlinks2;
+ struct hashmap_iter iter;
+ struct pair_entry *entry;
+ enum object_type type;
+ unsigned long size;
+ struct index_state wtindex;
+ struct checkout lstate, rstate;
+ int rc, flags = RUN_GIT_CMD, err = 0;
+ struct child_process child = CHILD_PROCESS_INIT;
+ const char *helper_argv[] = { "difftool--helper", NULL, NULL, NULL };
+ struct hashmap wt_modified, tmp_modified;
+ int indices_loaded = 0;
+
+ workdir = get_git_work_tree();
+
+ /* Setup temp directories */
+ tmp = getenv("TMPDIR");
+ xsnprintf(tmpdir, sizeof(tmpdir), "%s/git-difftool.XXXXXX", tmp ? tmp : "/tmp");
+ if (!mkdtemp(tmpdir))
+ return error("could not create '%s'", tmpdir);
+ strbuf_addf(&ldir, "%s/left/", tmpdir);
+ strbuf_addf(&rdir, "%s/right/", tmpdir);
+ strbuf_addstr(&wtdir, workdir);
+ if (!wtdir.len || !is_dir_sep(wtdir.buf[wtdir.len - 1]))
+ strbuf_addch(&wtdir, '/');
+ mkdir(ldir.buf, 0700);
+ mkdir(rdir.buf, 0700);
+
+ memset(&wtindex, 0, sizeof(wtindex));
+
+ memset(&lstate, 0, sizeof(lstate));
+ lstate.base_dir = ldir.buf;
+ lstate.base_dir_len = ldir.len;
+ lstate.force = 1;
+ memset(&rstate, 0, sizeof(rstate));
+ rstate.base_dir = rdir.buf;
+ rstate.base_dir_len = rdir.len;
+ rstate.force = 1;
+
+ ldir_len = ldir.len;
+ rdir_len = rdir.len;
+ wtdir_len = wtdir.len;
+
+ hashmap_init(&working_tree_dups,
+ (hashmap_cmp_fn)working_tree_entry_cmp, 0);
+ hashmap_init(&submodules, (hashmap_cmp_fn)pair_cmp, 0);
+ hashmap_init(&symlinks2, (hashmap_cmp_fn)pair_cmp, 0);
+
+ child.no_stdin = 1;
+ child.git_cmd = 1;
+ child.use_shell = 0;
+ child.clean_on_exit = 1;
+ child.dir = prefix;
+ child.out = -1;
+ argv_array_pushl(&child.args, "diff", "--raw", "--no-abbrev", "-z",
+ NULL);
+ for (i = 0; i < argc; i++)
+ argv_array_push(&child.args, argv[i]);
+ if (start_command(&child))
+ die("could not obtain raw diff");
+ fp = xfdopen(child.out, "r");
+
+ /* Build index info for left and right sides of the diff */
+ i = 0;
+ while (!strbuf_getline_nul(&info, fp)) {
+ int lmode, rmode;
+ struct object_id loid, roid;
+ char status;
+ const char *src_path, *dst_path;
+ size_t src_path_len, dst_path_len;
+
+ if (starts_with(info.buf, "::"))
+ die(N_("combined diff formats('-c' and '--cc') are "
+ "not supported in\n"
+ "directory diff mode('-d' and '--dir-diff')."));
+
+ if (parse_index_info(info.buf, &lmode, &rmode, &loid, &roid,
+ &status))
+ break;
+ if (strbuf_getline_nul(&lpath, fp))
+ break;
+ src_path = lpath.buf;
+ src_path_len = lpath.len;
+
+ i++;
+ if (status != 'C' && status != 'R') {
+ dst_path = src_path;
+ dst_path_len = src_path_len;
+ } else {
+ if (strbuf_getline_nul(&rpath, fp))
+ break;
+ dst_path = rpath.buf;
+ dst_path_len = rpath.len;
+ }
+
+ if (S_ISGITLINK(lmode) || S_ISGITLINK(rmode)) {
+ strbuf_reset(&buf);
+ strbuf_addf(&buf, "Subproject commit %s",
+ oid_to_hex(&loid));
+ add_left_or_right(&submodules, src_path, buf.buf, 0);
+ strbuf_reset(&buf);
+ strbuf_addf(&buf, "Subproject commit %s",
+ oid_to_hex(&roid));
+ if (!oidcmp(&loid, &roid))
+ strbuf_addstr(&buf, "-dirty");
+ add_left_or_right(&submodules, dst_path, buf.buf, 1);
+ continue;
+ }
+
+ if (S_ISLNK(lmode)) {
+ char *content = read_sha1_file(loid.hash, &type, &size);
+ add_left_or_right(&symlinks2, src_path, content, 0);
+ free(content);
+ }
+
+ if (S_ISLNK(rmode)) {
+ char *content = read_sha1_file(roid.hash, &type, &size);
+ add_left_or_right(&symlinks2, dst_path, content, 1);
+ free(content);
+ }
+
+ if (lmode && status != 'C') {
+ ce->ce_mode = lmode;
+ oidcpy(&ce->oid, &loid);
+ strcpy(ce->name, src_path);
+ ce->ce_namelen = src_path_len;
+ if (checkout_entry(ce, &lstate, NULL))
+ return error("could not write '%s'", src_path);
+ }
+
+ if (rmode) {
+ struct working_tree_entry *entry;
+
+ /* Avoid duplicate working_tree entries */
+ FLEX_ALLOC_STR(entry, path, dst_path);
+ hashmap_entry_init(entry, strhash(dst_path));
+ if (hashmap_get(&working_tree_dups, entry, NULL)) {
+ free(entry);
+ continue;
+ }
+ hashmap_add(&working_tree_dups, entry);
+
+ if (!use_wt_file(workdir, dst_path, &roid)) {
+ ce->ce_mode = rmode;
+ oidcpy(&ce->oid, &roid);
+ strcpy(ce->name, dst_path);
+ ce->ce_namelen = dst_path_len;
+ if (checkout_entry(ce, &rstate, NULL))
+ return error("could not write '%s'",
+ dst_path);
+ } else if (!is_null_oid(&roid)) {
+ /*
+ * Changes in the working tree need special
+ * treatment since they are not part of the
+ * index.
+ */
+ struct cache_entry *ce2 =
+ make_cache_entry(rmode, roid.hash,
+ dst_path, 0, 0);
+
+ add_index_entry(&wtindex, ce2,
+ ADD_CACHE_JUST_APPEND);
+
+ add_path(&rdir, rdir_len, dst_path);
+ if (ensure_leading_directories(rdir.buf))
+ return error("could not create "
+ "directory for '%s'",
+ dst_path);
+ add_path(&wtdir, wtdir_len, dst_path);
+ if (symlinks) {
+ if (symlink(wtdir.buf, rdir.buf)) {
+ ret = error_errno("could not symlink '%s' to '%s'", wtdir.buf, rdir.buf);
+ goto finish;
+ }
+ } else {
+ struct stat st;
+ if (stat(wtdir.buf, &st))
+ st.st_mode = 0644;
+ if (copy_file(rdir.buf, wtdir.buf,
+ st.st_mode)) {
+ ret = error("could not copy '%s' to '%s'", wtdir.buf, rdir.buf);
+ goto finish;
+ }
+ }
+ }
+ }
+ }
+
+ if (finish_command(&child)) {
+ ret = error("error occurred running diff --raw");
+ goto finish;
+ }
+
+ if (!i)
+ return 0;
+
+ /*
+ * Changes to submodules require special treatment.This loop writes a
+ * temporary file to both the left and right directories to show the
+ * change in the recorded SHA1 for the submodule.
+ */
+ hashmap_iter_init(&submodules, &iter);
+ while ((entry = hashmap_iter_next(&iter))) {
+ if (*entry->left) {
+ add_path(&ldir, ldir_len, entry->path);
+ ensure_leading_directories(ldir.buf);
+ write_file(ldir.buf, "%s", entry->left);
+ }
+ if (*entry->right) {
+ add_path(&rdir, rdir_len, entry->path);
+ ensure_leading_directories(rdir.buf);
+ write_file(rdir.buf, "%s", entry->right);
+ }
+ }
+
+ /*
+ * Symbolic links require special treatment.The standard "git diff"
+ * shows only the link itself, not the contents of the link target.
+ * This loop replicates that behavior.
+ */
+ hashmap_iter_init(&symlinks2, &iter);
+ while ((entry = hashmap_iter_next(&iter))) {
+ if (*entry->left) {
+ add_path(&ldir, ldir_len, entry->path);
+ ensure_leading_directories(ldir.buf);
+ write_file(ldir.buf, "%s", entry->left);
+ }
+ if (*entry->right) {
+ add_path(&rdir, rdir_len, entry->path);
+ ensure_leading_directories(rdir.buf);
+ write_file(rdir.buf, "%s", entry->right);
+ }
+ }
+
+ strbuf_release(&buf);
+
+ strbuf_setlen(&ldir, ldir_len);
+ helper_argv[1] = ldir.buf;
+ strbuf_setlen(&rdir, rdir_len);
+ helper_argv[2] = rdir.buf;
+
+ if (extcmd) {
+ helper_argv[0] = extcmd;
+ flags = 0;
+ } else
+ setenv("GIT_DIFFTOOL_DIRDIFF", "true", 1);
+ rc = run_command_v_opt(helper_argv, flags);
+
+ /*
+ * If the diff includes working copy files and those
+ * files were modified during the diff, then the changes
+ * should be copied back to the working tree.
+ * Do not copy back files when symlinks are used and the
+ * external tool did not replace the original link with a file.
+ *
+ * These hashes are loaded lazily since they aren't needed
+ * in the common case of --symlinks and the difftool updating
+ * files through the symlink.
+ */
+ hashmap_init(&wt_modified, (hashmap_cmp_fn)path_entry_cmp,
+ wtindex.cache_nr);
+ hashmap_init(&tmp_modified, (hashmap_cmp_fn)path_entry_cmp,
+ wtindex.cache_nr);
+
+ for (i = 0; i < wtindex.cache_nr; i++) {
+ struct hashmap_entry dummy;
+ const char *name = wtindex.cache[i]->name;
+ struct stat st;
+
+ add_path(&rdir, rdir_len, name);
+ if (lstat(rdir.buf, &st))
+ continue;
+
+ if ((symlinks && S_ISLNK(st.st_mode)) || !S_ISREG(st.st_mode))
+ continue;
+
+ if (!indices_loaded) {
+ static struct lock_file lock;
+ strbuf_reset(&buf);
+ strbuf_addf(&buf, "%s/wtindex", tmpdir);
+ if (hold_lock_file_for_update(&lock, buf.buf, 0) < 0 ||
+ write_locked_index(&wtindex, &lock, COMMIT_LOCK)) {
+ ret = error("could not write %s", buf.buf);
+ rollback_lock_file(&lock);
+ goto finish;
+ }
+ changed_files(&wt_modified, buf.buf, workdir);
+ strbuf_setlen(&rdir, rdir_len);
+ changed_files(&tmp_modified, buf.buf, rdir.buf);
+ add_path(&rdir, rdir_len, name);
+ indices_loaded = 1;
+ }
+
+ hashmap_entry_init(&dummy, strhash(name));
+ if (hashmap_get(&tmp_modified, &dummy, name)) {
+ add_path(&wtdir, wtdir_len, name);
+ if (hashmap_get(&wt_modified, &dummy, name)) {
+ warning(_("both files modified: '%s' and '%s'."),
+ wtdir.buf, rdir.buf);
+ warning(_("working tree file has been left."));
+ warning("");
+ err = 1;
+ } else if (unlink(wtdir.buf) ||
+ copy_file(wtdir.buf, rdir.buf, st.st_mode))
+ warning_errno(_("could not copy '%s' to '%s'"),
+ rdir.buf, wtdir.buf);
+ }
+ }
+
+ if (err) {
+ warning(_("temporary files exist in '%s'."), tmpdir);
+ warning(_("you may want to cleanup or recover these."));
+ exit(1);
+ } else
+ exit_cleanup(tmpdir, rc);
+
+finish:
+ free(ce);
+ strbuf_release(&ldir);
+ strbuf_release(&rdir);
+ strbuf_release(&wtdir);
+ strbuf_release(&buf);
+
+ return ret;
+}
+
+static int run_file_diff(int prompt, const char *prefix,
+ int argc, const char **argv)
+{
+ struct argv_array args = ARGV_ARRAY_INIT;
+ const char *env[] = {
+ "GIT_PAGER=", "GIT_EXTERNAL_DIFF=git-difftool--helper", NULL,
+ NULL
+ };
+ int ret = 0, i;
+
+ if (prompt > 0)
+ env[2] = "GIT_DIFFTOOL_PROMPT=true";
+ else if (!prompt)
+ env[2] = "GIT_DIFFTOOL_NO_PROMPT=true";
+
+
+ argv_array_push(&args, "diff");
+ for (i = 0; i < argc; i++)
+ argv_array_push(&args, argv[i]);
+ ret = run_command_v_opt_cd_env(args.argv, RUN_GIT_CMD, prefix, env);
+ exit(ret);
+}
/*
* NEEDSWORK: this function can go once the legacy-difftool Perl script is
@@ -41,6 +642,35 @@ static int use_builtin_difftool(void) {
int cmd_difftool(int argc, const char **argv, const char *prefix)
{
+ int use_gui_tool = 0, dir_diff = 0, prompt = -1, symlinks = 0,
+ tool_help = 0;
+ static char *difftool_cmd = NULL, *extcmd = NULL;
+ struct option builtin_difftool_options[] = {
+ OPT_BOOL('g', "gui", &use_gui_tool,
+ N_("use `diff.guitool` instead of `diff.tool`")),
+ OPT_BOOL('d', "dir-diff", &dir_diff,
+ N_("perform a full-directory diff")),
+ { OPTION_SET_INT, 'y', "no-prompt", &prompt, NULL,
+ N_("do not prompt before launching a diff tool"),
+ PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, 0},
+ { OPTION_SET_INT, 0, "prompt", &prompt, NULL, NULL,
+ PARSE_OPT_NOARG | PARSE_OPT_NONEG | PARSE_OPT_HIDDEN,
+ NULL, 1 },
+ OPT_BOOL(0, "symlinks", &symlinks,
+ N_("use symlinks in dir-diff mode")),
+ OPT_STRING('t', "tool", &difftool_cmd, N_("<tool>"),
+ N_("use the specified diff tool")),
+ OPT_BOOL(0, "tool-help", &tool_help,
+ N_("print a list of diff tools that may be used with "
+ "`--tool`")),
+ OPT_BOOL(0, "trust-exit-code", &trust_exit_code,
+ N_("make 'git-difftool' exit when an invoked diff "
+ "tool returns a non - zero exit code")),
+ OPT_STRING('x', "extcmd", &extcmd, N_("<command>"),
+ N_("specify a custom command for viewing diffs")),
+ OPT_END()
+ };
+
/*
* NEEDSWORK: Once the builtin difftool has been tested enough
* and git-legacy-difftool.perl is retired to contrib/, this preamble
@@ -58,6 +688,46 @@ int cmd_difftool(int argc, const char **argv, const char *prefix)
prefix = setup_git_directory();
trace_repo_setup(prefix);
setup_work_tree();
+ /* NEEDSWORK: once we no longer spawn anything, remove this */
+ setenv(GIT_DIR_ENVIRONMENT, absolute_path(get_git_dir()), 1);
+ setenv(GIT_WORK_TREE_ENVIRONMENT, absolute_path(get_git_work_tree()), 1);
+
+ git_config(difftool_config, NULL);
+ symlinks = has_symlinks;
+
+ argc = parse_options(argc, argv, prefix, builtin_difftool_options,
+ builtin_difftool_usage, PARSE_OPT_KEEP_UNKNOWN |
+ PARSE_OPT_KEEP_DASHDASH);
- die("TODO");
+ if (tool_help)
+ return print_tool_help();
+
+ if (use_gui_tool && diff_gui_tool && *diff_gui_tool)
+ setenv("GIT_DIFF_TOOL", diff_gui_tool, 1);
+ else if (difftool_cmd) {
+ if (*difftool_cmd)
+ setenv("GIT_DIFF_TOOL", difftool_cmd, 1);
+ else
+ die(_("no <tool> given for --tool=<tool>"));
+ }
+
+ if (extcmd) {
+ if (*extcmd)
+ setenv("GIT_DIFFTOOL_EXTCMD", extcmd, 1);
+ else
+ die(_("no <cmd> given for --extcmd=<cmd>"));
+ }
+
+ setenv("GIT_DIFFTOOL_TRUST_EXIT_CODE",
+ trust_exit_code ? "true" : "false", 1);
+
+ /*
+ * In directory diff mode, 'git-difftool--helper' is called once
+ * to compare the a / b directories. In file diff mode, 'git diff'
+ * will invoke a separate instance of 'git-difftool--helper' for
+ * each file that changed.
+ */
+ if (dir_diff)
+ return run_dir_diff(extcmd, symlinks, prefix, argc, argv);
+ return run_file_diff(prompt, prefix, argc, argv);
}
--
2.11.0.windows.3
^ permalink raw reply related
* [PATCH v5 0/3] Turn the difftool into a builtin
From: Johannes Schindelin @ 2017-01-17 15:54 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, David Aguilar, Dennis Kaarsemaker, Paul Sbarra
In-Reply-To: <cover.1483373635.git.johannes.schindelin@gmx.de>
This patch series converts the difftool from a Perl script into a
builtin, for three reasons:
1. Perl is really not native on Windows. Not only is there a performance
penalty to be paid just for running Perl scripts, we also have to deal
with the fact that users may have different Perl installations, with
different options, and some other Perl installation may decide to set
PERL5LIB globally, wreaking havoc with Git for Windows' Perl (which we
have to use because almost all other Perl distributions lack the
Subversion bindings we need for `git svn`).
2. As the Perl script uses Unix-y paths that are not native to Windows,
the Perl interpreter has to go through a POSIX emulation layer (the
MSYS2 runtime). This means that paths have to be converted from
Unix-y paths to Windows-y paths (and vice versa) whenever crossing
the POSIX emulation barrier, leading to quite possibly surprising path
translation errors.
3. Perl makes for a rather large reason that Git for Windows' installer
weighs in with >30MB. While one Perl script less does not relieve us
of that burden, it is one step in the right direction.
Changes since v4:
- skipped the unrelated Coverity-appeasing patch.
- replaced the cross-validation with the Perl script by a patch that
retires the Perl script instead.
Johannes Schindelin (3):
difftool: add a skeleton for the upcoming builtin
difftool: implement the functionality in the builtin
Retire the scripted difftool
Makefile | 2 +-
builtin.h | 1 +
builtin/difftool.c | 692 +++++++++++++++++++++
.../examples/git-difftool.perl | 0
git.c | 1 +
t/t7800-difftool.sh | 92 +--
6 files changed, 741 insertions(+), 47 deletions(-)
create mode 100644 builtin/difftool.c
rename git-difftool.perl => contrib/examples/git-difftool.perl (100%)
base-commit: d7dffce1cebde29a0c4b309a79e4345450bf352a
Published-As: https://github.com/dscho/git/releases/tag/builtin-difftool-v5
Fetch-It-Via: git fetch https://github.com/dscho/git builtin-difftool-v5
Interdiff vs v4:
diff --git a/.gitignore b/.gitignore
index 5555ae025b..6722f78f9a 100644
--- a/.gitignore
+++ b/.gitignore
@@ -76,7 +76,6 @@
/git-init-db
/git-interpret-trailers
/git-instaweb
-/git-legacy-difftool
/git-log
/git-ls-files
/git-ls-remote
diff --git a/Makefile b/Makefile
index 8cf5bef034..e9aa6ae57c 100644
--- a/Makefile
+++ b/Makefile
@@ -522,7 +522,6 @@ SCRIPT_LIB += git-sh-setup
SCRIPT_LIB += git-sh-i18n
SCRIPT_PERL += git-add--interactive.perl
-SCRIPT_PERL += git-legacy-difftool.perl
SCRIPT_PERL += git-archimport.perl
SCRIPT_PERL += git-cvsexportcommit.perl
SCRIPT_PERL += git-cvsimport.perl
diff --git a/builtin/difftool.c b/builtin/difftool.c
index 2115e548a5..42ad9e804a 100644
--- a/builtin/difftool.c
+++ b/builtin/difftool.c
@@ -616,30 +616,6 @@ static int run_file_diff(int prompt, const char *prefix,
exit(ret);
}
-/*
- * NEEDSWORK: this function can go once the legacy-difftool Perl script is
- * retired.
- *
- * We intentionally avoid reading the config directly here, to avoid messing up
- * the GIT_* environment variables when we need to fall back to exec()ing the
- * Perl script.
- */
-static int use_builtin_difftool(void) {
- struct child_process cp = CHILD_PROCESS_INIT;
- struct strbuf out = STRBUF_INIT;
- int ret;
-
- argv_array_pushl(&cp.args,
- "config", "--bool", "difftool.usebuiltin", NULL);
- cp.git_cmd = 1;
- if (capture_command(&cp, &out, 6))
- return 0;
- strbuf_trim(&out);
- ret = !strcmp("true", out.buf);
- strbuf_release(&out);
- return ret;
-}
-
int cmd_difftool(int argc, const char **argv, const char *prefix)
{
int use_gui_tool = 0, dir_diff = 0, prompt = -1, symlinks = 0,
@@ -671,23 +647,6 @@ int cmd_difftool(int argc, const char **argv, const char *prefix)
OPT_END()
};
- /*
- * NEEDSWORK: Once the builtin difftool has been tested enough
- * and git-legacy-difftool.perl is retired to contrib/, this preamble
- * can be removed.
- */
- if (!use_builtin_difftool()) {
- const char *path = mkpath("%s/git-legacy-difftool",
- git_exec_path());
-
- if (sane_execvp(path, (char **)argv) < 0)
- die_errno("could not exec %s", path);
-
- return 0;
- }
- prefix = setup_git_directory();
- trace_repo_setup(prefix);
- setup_work_tree();
/* NEEDSWORK: once we no longer spawn anything, remove this */
setenv(GIT_DIR_ENVIRONMENT, absolute_path(get_git_dir()), 1);
setenv(GIT_WORK_TREE_ENVIRONMENT, absolute_path(get_git_work_tree()), 1);
diff --git a/git-legacy-difftool.perl b/contrib/examples/git-difftool.perl
similarity index 100%
rename from git-legacy-difftool.perl
rename to contrib/examples/git-difftool.perl
diff --git a/exec_cmd.c b/exec_cmd.c
index 587bd7eb48..19ac2146d0 100644
--- a/exec_cmd.c
+++ b/exec_cmd.c
@@ -65,7 +65,6 @@ void git_set_argv_exec_path(const char *exec_path)
const char *git_exec_path(void)
{
const char *env;
- static char *system_exec_path;
if (argv_exec_path)
return argv_exec_path;
@@ -75,9 +74,7 @@ const char *git_exec_path(void)
return env;
}
- if (!system_exec_path)
- system_exec_path = system_path(GIT_EXEC_PATH);
- return system_exec_path;
+ return system_path(GIT_EXEC_PATH);
}
static void add_path(struct strbuf *out, const char *path)
diff --git a/git.c b/git.c
index c58181e5ef..bd4d668a21 100644
--- a/git.c
+++ b/git.c
@@ -424,12 +424,7 @@ static struct cmd_struct commands[] = {
{ "diff-files", cmd_diff_files, RUN_SETUP | NEED_WORK_TREE },
{ "diff-index", cmd_diff_index, RUN_SETUP },
{ "diff-tree", cmd_diff_tree, RUN_SETUP },
- /*
- * NEEDSWORK: Once the redirection to git-legacy-difftool.perl in
- * builtin/difftool.c has been removed, this entry should be changed to
- * RUN_SETUP | NEED_WORK_TREE
- */
- { "difftool", cmd_difftool },
+ { "difftool", cmd_difftool, RUN_SETUP | NEED_WORK_TREE },
{ "fast-export", cmd_fast_export, RUN_SETUP },
{ "fetch", cmd_fetch, RUN_SETUP },
{ "fetch-pack", cmd_fetch_pack, RUN_SETUP },
diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index 273ab55723..aa0ef02597 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -23,24 +23,8 @@ prompt_given ()
test "$prompt" = "Launch 'test-tool' [Y/n]? branch"
}
-for use_builtin_difftool in false true
-do
-
-test_expect_success 'verify we are running the correct difftool' '
- if test true = '$use_builtin_difftool'
- then
- test_must_fail ok=129 git difftool -h >help &&
- grep "g, --gui" help
- else
- git difftool -h >help &&
- grep "g|--gui" help
- fi
-'
-
-# NEEDSWORK: lose all the PERL prereqs once legacy-difftool is retired.
-
# Create a file on master and change it on branch
-test_expect_success PERL 'setup' '
+test_expect_success 'setup' '
echo master >file &&
git add file &&
git commit -m "added file" &&
@@ -52,7 +36,7 @@ test_expect_success PERL 'setup' '
'
# Configure a custom difftool.<tool>.cmd and use it
-test_expect_success PERL 'custom commands' '
+test_expect_success 'custom commands' '
difftool_test_setup &&
test_config difftool.test-tool.cmd "cat \"\$REMOTE\"" &&
echo master >expect &&
@@ -65,21 +49,21 @@ test_expect_success PERL 'custom commands' '
test_cmp expect actual
'
-test_expect_success PERL 'custom tool commands override built-ins' '
+test_expect_success 'custom tool commands override built-ins' '
test_config difftool.vimdiff.cmd "cat \"\$REMOTE\"" &&
echo master >expect &&
git difftool --tool vimdiff --no-prompt branch >actual &&
test_cmp expect actual
'
-test_expect_success PERL 'difftool ignores bad --tool values' '
+test_expect_success 'difftool ignores bad --tool values' '
: >expect &&
test_must_fail \
git difftool --no-prompt --tool=bad-tool branch >actual &&
test_cmp expect actual
'
-test_expect_success PERL 'difftool forwards arguments to diff' '
+test_expect_success 'difftool forwards arguments to diff' '
difftool_test_setup &&
>for-diff &&
git add for-diff &&
@@ -92,40 +76,40 @@ test_expect_success PERL 'difftool forwards arguments to diff' '
rm for-diff
'
-test_expect_success PERL 'difftool ignores exit code' '
+test_expect_success 'difftool ignores exit code' '
test_config difftool.error.cmd false &&
git difftool -y -t error branch
'
-test_expect_success PERL 'difftool forwards exit code with --trust-exit-code' '
+test_expect_success 'difftool forwards exit code with --trust-exit-code' '
test_config difftool.error.cmd false &&
test_must_fail git difftool -y --trust-exit-code -t error branch
'
-test_expect_success PERL 'difftool forwards exit code with --trust-exit-code for built-ins' '
+test_expect_success 'difftool forwards exit code with --trust-exit-code for built-ins' '
test_config difftool.vimdiff.path false &&
test_must_fail git difftool -y --trust-exit-code -t vimdiff branch
'
-test_expect_success PERL 'difftool honors difftool.trustExitCode = true' '
+test_expect_success 'difftool honors difftool.trustExitCode = true' '
test_config difftool.error.cmd false &&
test_config difftool.trustExitCode true &&
test_must_fail git difftool -y -t error branch
'
-test_expect_success PERL 'difftool honors difftool.trustExitCode = false' '
+test_expect_success 'difftool honors difftool.trustExitCode = false' '
test_config difftool.error.cmd false &&
test_config difftool.trustExitCode false &&
git difftool -y -t error branch
'
-test_expect_success PERL 'difftool ignores exit code with --no-trust-exit-code' '
+test_expect_success 'difftool ignores exit code with --no-trust-exit-code' '
test_config difftool.error.cmd false &&
test_config difftool.trustExitCode true &&
git difftool -y --no-trust-exit-code -t error branch
'
-test_expect_success PERL 'difftool stops on error with --trust-exit-code' '
+test_expect_success 'difftool stops on error with --trust-exit-code' '
test_when_finished "rm -f for-diff .git/fail-right-file" &&
test_when_finished "git reset -- for-diff" &&
write_script .git/fail-right-file <<-\EOF &&
@@ -140,13 +124,13 @@ test_expect_success PERL 'difftool stops on error with --trust-exit-code' '
test_cmp expect actual
'
-test_expect_success PERL 'difftool honors exit status if command not found' '
+test_expect_success 'difftool honors exit status if command not found' '
test_config difftool.nonexistent.cmd i-dont-exist &&
test_config difftool.trustExitCode false &&
test_must_fail git difftool -y -t nonexistent branch
'
-test_expect_success PERL 'difftool honors --gui' '
+test_expect_success 'difftool honors --gui' '
difftool_test_setup &&
test_config merge.tool bogus-tool &&
test_config diff.tool bogus-tool &&
@@ -157,7 +141,7 @@ test_expect_success PERL 'difftool honors --gui' '
test_cmp expect actual
'
-test_expect_success PERL 'difftool --gui last setting wins' '
+test_expect_success 'difftool --gui last setting wins' '
difftool_test_setup &&
: >expect &&
git difftool --no-prompt --gui --no-gui >actual &&
@@ -171,7 +155,7 @@ test_expect_success PERL 'difftool --gui last setting wins' '
test_cmp expect actual
'
-test_expect_success PERL 'difftool --gui works without configured diff.guitool' '
+test_expect_success 'difftool --gui works without configured diff.guitool' '
difftool_test_setup &&
echo branch >expect &&
git difftool --no-prompt --gui branch >actual &&
@@ -179,7 +163,7 @@ test_expect_success PERL 'difftool --gui works without configured diff.guitool'
'
# Specify the diff tool using $GIT_DIFF_TOOL
-test_expect_success PERL 'GIT_DIFF_TOOL variable' '
+test_expect_success 'GIT_DIFF_TOOL variable' '
difftool_test_setup &&
git config --unset diff.tool &&
echo branch >expect &&
@@ -189,7 +173,7 @@ test_expect_success PERL 'GIT_DIFF_TOOL variable' '
# Test the $GIT_*_TOOL variables and ensure
# that $GIT_DIFF_TOOL always wins unless --tool is specified
-test_expect_success PERL 'GIT_DIFF_TOOL overrides' '
+test_expect_success 'GIT_DIFF_TOOL overrides' '
difftool_test_setup &&
test_config diff.tool bogus-tool &&
test_config merge.tool bogus-tool &&
@@ -207,7 +191,7 @@ test_expect_success PERL 'GIT_DIFF_TOOL overrides' '
# Test that we don't have to pass --no-prompt to difftool
# when $GIT_DIFFTOOL_NO_PROMPT is true
-test_expect_success PERL 'GIT_DIFFTOOL_NO_PROMPT variable' '
+test_expect_success 'GIT_DIFFTOOL_NO_PROMPT variable' '
difftool_test_setup &&
echo branch >expect &&
GIT_DIFFTOOL_NO_PROMPT=true git difftool branch >actual &&
@@ -216,7 +200,7 @@ test_expect_success PERL 'GIT_DIFFTOOL_NO_PROMPT variable' '
# git-difftool supports the difftool.prompt variable.
# Test that GIT_DIFFTOOL_PROMPT can override difftool.prompt = false
-test_expect_success PERL 'GIT_DIFFTOOL_PROMPT variable' '
+test_expect_success 'GIT_DIFFTOOL_PROMPT variable' '
difftool_test_setup &&
test_config difftool.prompt false &&
echo >input &&
@@ -226,7 +210,7 @@ test_expect_success PERL 'GIT_DIFFTOOL_PROMPT variable' '
'
# Test that we don't have to pass --no-prompt when difftool.prompt is false
-test_expect_success PERL 'difftool.prompt config variable is false' '
+test_expect_success 'difftool.prompt config variable is false' '
difftool_test_setup &&
test_config difftool.prompt false &&
echo branch >expect &&
@@ -235,7 +219,7 @@ test_expect_success PERL 'difftool.prompt config variable is false' '
'
# Test that we don't have to pass --no-prompt when mergetool.prompt is false
-test_expect_success PERL 'difftool merge.prompt = false' '
+test_expect_success 'difftool merge.prompt = false' '
difftool_test_setup &&
test_might_fail git config --unset difftool.prompt &&
test_config mergetool.prompt false &&
@@ -245,7 +229,7 @@ test_expect_success PERL 'difftool merge.prompt = false' '
'
# Test that the -y flag can override difftool.prompt = true
-test_expect_success PERL 'difftool.prompt can overridden with -y' '
+test_expect_success 'difftool.prompt can overridden with -y' '
difftool_test_setup &&
test_config difftool.prompt true &&
echo branch >expect &&
@@ -254,7 +238,7 @@ test_expect_success PERL 'difftool.prompt can overridden with -y' '
'
# Test that the --prompt flag can override difftool.prompt = false
-test_expect_success PERL 'difftool.prompt can overridden with --prompt' '
+test_expect_success 'difftool.prompt can overridden with --prompt' '
difftool_test_setup &&
test_config difftool.prompt false &&
echo >input &&
@@ -264,7 +248,7 @@ test_expect_success PERL 'difftool.prompt can overridden with --prompt' '
'
# Test that the last flag passed on the command-line wins
-test_expect_success PERL 'difftool last flag wins' '
+test_expect_success 'difftool last flag wins' '
difftool_test_setup &&
echo branch >expect &&
git difftool --prompt --no-prompt branch >actual &&
@@ -277,7 +261,7 @@ test_expect_success PERL 'difftool last flag wins' '
# git-difftool falls back to git-mergetool config variables
# so test that behavior here
-test_expect_success PERL 'difftool + mergetool config variables' '
+test_expect_success 'difftool + mergetool config variables' '
test_config merge.tool test-tool &&
test_config mergetool.test-tool.cmd "cat \$LOCAL" &&
echo branch >expect &&
@@ -291,49 +275,49 @@ test_expect_success PERL 'difftool + mergetool config variables' '
test_cmp expect actual
'
-test_expect_success PERL 'difftool.<tool>.path' '
+test_expect_success 'difftool.<tool>.path' '
test_config difftool.tkdiff.path echo &&
git difftool --tool=tkdiff --no-prompt branch >output &&
lines=$(grep file output | wc -l) &&
test "$lines" -eq 1
'
-test_expect_success PERL 'difftool --extcmd=cat' '
+test_expect_success 'difftool --extcmd=cat' '
echo branch >expect &&
echo master >>expect &&
git difftool --no-prompt --extcmd=cat branch >actual &&
test_cmp expect actual
'
-test_expect_success PERL 'difftool --extcmd cat' '
+test_expect_success 'difftool --extcmd cat' '
echo branch >expect &&
echo master >>expect &&
git difftool --no-prompt --extcmd=cat branch >actual &&
test_cmp expect actual
'
-test_expect_success PERL 'difftool -x cat' '
+test_expect_success 'difftool -x cat' '
echo branch >expect &&
echo master >>expect &&
git difftool --no-prompt -x cat branch >actual &&
test_cmp expect actual
'
-test_expect_success PERL 'difftool --extcmd echo arg1' '
+test_expect_success 'difftool --extcmd echo arg1' '
echo file >expect &&
git difftool --no-prompt \
--extcmd sh\ -c\ \"echo\ \$1\" branch >actual &&
test_cmp expect actual
'
-test_expect_success PERL 'difftool --extcmd cat arg1' '
+test_expect_success 'difftool --extcmd cat arg1' '
echo master >expect &&
git difftool --no-prompt \
--extcmd sh\ -c\ \"cat\ \$1\" branch >actual &&
test_cmp expect actual
'
-test_expect_success PERL 'difftool --extcmd cat arg2' '
+test_expect_success 'difftool --extcmd cat arg2' '
echo branch >expect &&
git difftool --no-prompt \
--extcmd sh\ -c\ \"cat\ \$2\" branch >actual &&
@@ -341,7 +325,7 @@ test_expect_success PERL 'difftool --extcmd cat arg2' '
'
# Create a second file on master and a different version on branch
-test_expect_success PERL 'setup with 2 files different' '
+test_expect_success 'setup with 2 files different' '
echo m2 >file2 &&
git add file2 &&
git commit -m "added file2" &&
@@ -353,7 +337,7 @@ test_expect_success PERL 'setup with 2 files different' '
git checkout master
'
-test_expect_success PERL 'say no to the first file' '
+test_expect_success 'say no to the first file' '
(echo n && echo) >input &&
git difftool -x cat branch <input >output &&
grep m2 output &&
@@ -362,7 +346,7 @@ test_expect_success PERL 'say no to the first file' '
! grep branch output
'
-test_expect_success PERL 'say no to the second file' '
+test_expect_success 'say no to the second file' '
(echo && echo n) >input &&
git difftool -x cat branch <input >output &&
grep master output &&
@@ -371,7 +355,7 @@ test_expect_success PERL 'say no to the second file' '
! grep br2 output
'
-test_expect_success PERL 'ending prompt input with EOF' '
+test_expect_success 'ending prompt input with EOF' '
git difftool -x cat branch </dev/null >output &&
! grep master output &&
! grep branch output &&
@@ -379,12 +363,12 @@ test_expect_success PERL 'ending prompt input with EOF' '
! grep br2 output
'
-test_expect_success PERL 'difftool --tool-help' '
+test_expect_success 'difftool --tool-help' '
git difftool --tool-help >output &&
grep tool output
'
-test_expect_success PERL 'setup change in subdirectory' '
+test_expect_success 'setup change in subdirectory' '
git checkout master &&
mkdir sub &&
echo master >sub/sub &&
@@ -398,11 +382,11 @@ test_expect_success PERL 'setup change in subdirectory' '
'
run_dir_diff_test () {
- test_expect_success PERL "$1 --no-symlinks" "
+ test_expect_success "$1 --no-symlinks" "
symlinks=--no-symlinks &&
$2
"
- test_expect_success PERL,SYMLINKS "$1 --symlinks" "
+ test_expect_success SYMLINKS "$1 --symlinks" "
symlinks=--symlinks &&
$2
"
@@ -524,7 +508,7 @@ do
done >actual
EOF
-test_expect_success PERL,SYMLINKS 'difftool --dir-diff --symlink without unstaged changes' '
+test_expect_success SYMLINKS 'difftool --dir-diff --symlink without unstaged changes' '
cat >expect <<-EOF &&
file
$PWD/file
@@ -561,7 +545,7 @@ write_script modify-file <<\EOF
echo "new content" >file
EOF
-test_expect_success PERL 'difftool --no-symlinks does not overwrite working tree file ' '
+test_expect_success 'difftool --no-symlinks does not overwrite working tree file ' '
echo "orig content" >file &&
git difftool --dir-diff --no-symlinks --extcmd "$PWD/modify-file" branch &&
echo "new content" >expect &&
@@ -574,7 +558,7 @@ echo "tmp content" >"$2/file" &&
echo "$2" >tmpdir
EOF
-test_expect_success PERL 'difftool --no-symlinks detects conflict ' '
+test_expect_success 'difftool --no-symlinks detects conflict ' '
(
TMPDIR=$TRASH_DIRECTORY &&
export TMPDIR &&
@@ -587,7 +571,7 @@ test_expect_success PERL 'difftool --no-symlinks detects conflict ' '
)
'
-test_expect_success PERL 'difftool properly honors gitlink and core.worktree' '
+test_expect_success 'difftool properly honors gitlink and core.worktree' '
git submodule add ./. submod/ule &&
test_config -C submod/ule diff.tool checktrees &&
test_config -C submod/ule difftool.checktrees.cmd '\''
@@ -601,7 +585,7 @@ test_expect_success PERL 'difftool properly honors gitlink and core.worktree' '
)
'
-test_expect_success PERL,SYMLINKS 'difftool --dir-diff symlinked directories' '
+test_expect_success SYMLINKS 'difftool --dir-diff symlinked directories' '
git init dirlinks &&
(
cd dirlinks &&
@@ -620,17 +604,4 @@ test_expect_success PERL,SYMLINKS 'difftool --dir-diff symlinked directories' '
)
'
-test true != $use_builtin_difftool || break
-
-test_expect_success 'tear down for re-run' '
- rm -rf * .[a-z]* &&
- git init
-'
-
-# run as builtin difftool now
-GIT_CONFIG_PARAMETERS="'difftool.usebuiltin=true'"
-export GIT_CONFIG_PARAMETERS
-
-done
-
test_done
--
2.11.0.windows.3
^ permalink raw reply
* Re: [PATCH v5 3/7] tag: add format specifier to gpg_verify_tag
From: Santiago Torres @ 2017-01-17 16:57 UTC (permalink / raw)
To: Jeff King; +Cc: git, gitster, sunshine, walters, Lukas Puehringer
In-Reply-To: <20170117153019.gujiruwghkwfklgv@sigill.intra.peff.net>
[-- Attachment #1: Type: text/plain, Size: 1048 bytes --]
> > Hrm. Maybe I am missing something, but what does:
> >
> > verify_and_format_tag(sha1, name, fmt, flags);
> >
> > get you over:
> >
> > gpg_verify_tag(sha1, name, flags);
> > pretty_print_ref(name, sha1, fmt);
> >
> > ? The latter seems much more flexible, and I do not see how the
> > verification step impacts the printing at all (or vice versa).
> >
> > I could understand it more if there were patches later in the series
> > that somehow used the format and verification results together. But I
> > didn't see that.
>
> Having read through the rest of the series, it looks like you'd
> sometimes have to do:
IIRC, we did this to make the diff "simpler". It also feeds odd that the
call chain is the following:
verify_and_format_tag()
gpg_verify_tag()
run_gpg_verification()
I'm afraid that adding yet another wrapper would further convolute the
call chain. If you think this is not an issue, I could easily do it. Do
you have any suggested name for the wrapper?
Thanks!
-Santiago
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v5 5/7] builtin/tag: add --format argument for tag -v
From: Santiago Torres @ 2017-01-17 17:00 UTC (permalink / raw)
To: Jeff King; +Cc: git, gitster, sunshine, walters, Lukas Puehringer
In-Reply-To: <20170117153404.jp3ftdlzeyut6e7a@sigill.intra.peff.net>
[-- Attachment #1: Type: text/plain, Size: 803 bytes --]
> > {
> > - return verify_and_format_tag(sha1, name, NULL, GPG_VERIFY_VERBOSE);
> > + int flags;
> > + char *fmt_pretty = cb_data;
> > + flags = GPG_VERIFY_VERBOSE;
> > +
> > + if (fmt_pretty)
> > + flags = GPG_VERIFY_QUIET;
> > +
> > + return verify_and_format_tag(sha1, ref, fmt_pretty, flags);
>
> It seems funny that VERBOSE and QUIET are bit-flags. What happens when
> you ask for GPG_VERIFY_VERBOSE|GPG_VERIFY_QUIET?
If I'm not mistaken, the way the code works right now this is not
possible (GPG_VERIFY_VERBOSE will be unset when GPG_VERIFY_QUIET). I
would have to re-read the patch to make sure this is the case then.
GPG_VERIFY_QUIET was added to suppress any VERBOSE|RAW flags, we could
defeault to QUIET if flags are not set. What do you think?
Thanks!
-Santiago
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v5 3/7] tag: add format specifier to gpg_verify_tag
From: Jeff King @ 2017-01-17 17:25 UTC (permalink / raw)
To: Santiago Torres; +Cc: git, gitster, sunshine, walters, Lukas Puehringer
In-Reply-To: <20170117165724.2hbyfdzrhrmro54b@LykOS.localdomain>
On Tue, Jan 17, 2017 at 11:57:25AM -0500, Santiago Torres wrote:
> > Having read through the rest of the series, it looks like you'd
> > sometimes have to do:
>
> IIRC, we did this to make the diff "simpler". It also feeds odd that the
> call chain is the following:
>
> verify_and_format_tag()
> gpg_verify_tag()
> run_gpg_verification()
>
> I'm afraid that adding yet another wrapper would further convolute the
> call chain. If you think this is not an issue, I could easily do it. Do
> you have any suggested name for the wrapper?
Actually, looking at the callsites, I think they are fine to just call
pretty_print_ref() themselves, and I don't think it actually matters if
it happens before or after the verification.
So I think you could just throw out patch 3 entirely and squash these
hunks into patches 4 and 5:
diff --git a/builtin/tag.c b/builtin/tag.c
index 9da11e0c2..fab9fa8f9 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -111,10 +111,12 @@ static int verify_tag(const char *name, const char *ref,
char *fmt_pretty = cb_data;
flags = GPG_VERIFY_VERBOSE;
- if (fmt_pretty)
+ if (fmt_pretty) {
flags = GPG_VERIFY_QUIET;
+ pretty_print_ref(name, sha1, fmt_pretty);
+ }
- return verify_and_format_tag(sha1, ref, fmt_pretty, flags);
+ return gpg_verify_tag(sha1, ref, flags);
}
static int do_sign(struct strbuf *buffer)
diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
index 212449f47..114df1c52 100644
--- a/builtin/verify-tag.c
+++ b/builtin/verify-tag.c
@@ -58,9 +58,15 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix)
while (i < argc) {
unsigned char sha1[20];
const char *name = argv[i++];
- if (get_sha1(name, sha1))
+
+ if (get_sha1(name, sha1)) {
had_error = !!error("tag '%s' not found.", name);
- else if (verify_and_format_tag(sha1, name, fmt_pretty, flags))
+ continue;
+ }
+
+ if (fmt_pretty)
+ pretty_print_ref(name, sha1, fmt_pretty);
+ if (gpg_verify_tag(sha1, name, flags))
had_error = 1;
}
return had_error;
You could make the diff in the second one simpler by skipping the
"continue" and just doing the whole thing in an "else" block. But IMHO
the continue-on-error makes the logic more clear.
-Peff
^ permalink raw reply related
* Re: [PATCH v5 3/7] tag: add format specifier to gpg_verify_tag
From: Jeff King @ 2017-01-17 17:30 UTC (permalink / raw)
To: Santiago Torres; +Cc: git, gitster, sunshine, walters, Lukas Puehringer
In-Reply-To: <20170117172531.bahjekbj3om43gtq@sigill.intra.peff.net>
On Tue, Jan 17, 2017 at 12:25:31PM -0500, Jeff King wrote:
> Actually, looking at the callsites, I think they are fine to just call
> pretty_print_ref() themselves, and I don't think it actually matters if
> it happens before or after the verification.
Oh, sorry, I misread it. We do indeed early-return from the verification
and skip the printing in that case. So it'd be more like:
diff --git a/builtin/tag.c b/builtin/tag.c
index 9da11e0c2..068f392b6 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -114,7 +114,11 @@ static int verify_tag(const char *name, const char *ref,
if (fmt_pretty)
flags = GPG_VERIFY_QUIET;
- return verify_and_format_tag(sha1, ref, fmt_pretty, flags);
+ if (gpg_verify_tag(sha1, ref, flags))
+ return -1;
+
+ pretty_print_ref(name, sha1, fmt_pretty);
+ return 0;
}
static int do_sign(struct strbuf *buffer)
diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
index 212449f47..b3f08f705 100644
--- a/builtin/verify-tag.c
+++ b/builtin/verify-tag.c
@@ -58,10 +58,19 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix)
while (i < argc) {
unsigned char sha1[20];
const char *name = argv[i++];
- if (get_sha1(name, sha1))
+
+ if (get_sha1(name, sha1)) {
had_error = !!error("tag '%s' not found.", name);
- else if (verify_and_format_tag(sha1, name, fmt_pretty, flags))
+ continue;
+ }
+
+ if (gpg_verify_tag(sha1, name, flags)) {
had_error = 1;
+ continue;
+ }
+
+ if (fmt_pretty)
+ pretty_print_ref(name, sha1, fmt_pretty);
}
return had_error;
}
which I think is still an improvement (the printing, rather than being
an obscure parameter to the overloaded verify_and_format_tag()
function, is clearly a first-class operation).
-Peff
^ permalink raw reply related
* Re: [PATCH v5 5/7] builtin/tag: add --format argument for tag -v
From: Jeff King @ 2017-01-17 17:32 UTC (permalink / raw)
To: Santiago Torres; +Cc: git, gitster, sunshine, walters, Lukas Puehringer
In-Reply-To: <20170117170018.nqk4yy5rrpomxr32@LykOS.localdomain>
On Tue, Jan 17, 2017 at 12:00:19PM -0500, Santiago Torres wrote:
> > > {
> > > - return verify_and_format_tag(sha1, name, NULL, GPG_VERIFY_VERBOSE);
> > > + int flags;
> > > + char *fmt_pretty = cb_data;
> > > + flags = GPG_VERIFY_VERBOSE;
> > > +
> > > + if (fmt_pretty)
> > > + flags = GPG_VERIFY_QUIET;
> > > +
> > > + return verify_and_format_tag(sha1, ref, fmt_pretty, flags);
> >
> > It seems funny that VERBOSE and QUIET are bit-flags. What happens when
> > you ask for GPG_VERIFY_VERBOSE|GPG_VERIFY_QUIET?
>
> If I'm not mistaken, the way the code works right now this is not
> possible (GPG_VERIFY_VERBOSE will be unset when GPG_VERIFY_QUIET). I
> would have to re-read the patch to make sure this is the case then.
No, you're right that the two flags are mutually exclusive in the
current callers. So I don't think there's a bug here. It's just that the
interface is potentially awkward/confusing, which may be a problem down
the line.
VERBOSE|QUIET _does_ have a meaning, which is "show the payload, but do
not print the signature buffer". Perhaps just renaming QUIET to
OMIT_STATUS or something would make it more clear.
-Peff
^ permalink raw reply
* Re: [PATCH v5 3/7] tag: add format specifier to gpg_verify_tag
From: Jeff King @ 2017-01-17 17:34 UTC (permalink / raw)
To: Santiago Torres; +Cc: git, gitster, sunshine, walters, Lukas Puehringer
In-Reply-To: <20170117173346.paqrsroauoskxpn6@LykOS.localdomain>
On Tue, Jan 17, 2017 at 12:33:46PM -0500, Santiago Torres wrote:
> Yeah, this actually looks more cleaner.
>
> Let me give it a go.
Neat. Note there's a bug:
> > diff --git a/builtin/tag.c b/builtin/tag.c
> > index 9da11e0c2..068f392b6 100644
> > --- a/builtin/tag.c
> > +++ b/builtin/tag.c
> > @@ -114,7 +114,11 @@ static int verify_tag(const char *name, const char *ref,
> > if (fmt_pretty)
> > flags = GPG_VERIFY_QUIET;
> >
> > - return verify_and_format_tag(sha1, ref, fmt_pretty, flags);
> > + if (gpg_verify_tag(sha1, ref, flags))
> > + return -1;
> > +
> > + pretty_print_ref(name, sha1, fmt_pretty);
> > + return 0;
The final print should be guarded by "if (fmt_pretty)".
-Peff
^ permalink raw reply
* Re: [PATCH v5 3/7] tag: add format specifier to gpg_verify_tag
From: Santiago Torres @ 2017-01-17 17:33 UTC (permalink / raw)
To: Jeff King; +Cc: git, gitster, sunshine, walters, Lukas Puehringer
In-Reply-To: <20170117173003.sgipqc2cijpdrukb@sigill.intra.peff.net>
[-- Attachment #1: Type: text/plain, Size: 2059 bytes --]
Yeah, this actually looks more cleaner.
Let me give it a go.
Thanks!
-Santiago.
On Tue, Jan 17, 2017 at 12:30:04PM -0500, Jeff King wrote:
> On Tue, Jan 17, 2017 at 12:25:31PM -0500, Jeff King wrote:
>
> > Actually, looking at the callsites, I think they are fine to just call
> > pretty_print_ref() themselves, and I don't think it actually matters if
> > it happens before or after the verification.
>
> Oh, sorry, I misread it. We do indeed early-return from the verification
> and skip the printing in that case. So it'd be more like:
>
> diff --git a/builtin/tag.c b/builtin/tag.c
> index 9da11e0c2..068f392b6 100644
> --- a/builtin/tag.c
> +++ b/builtin/tag.c
> @@ -114,7 +114,11 @@ static int verify_tag(const char *name, const char *ref,
> if (fmt_pretty)
> flags = GPG_VERIFY_QUIET;
>
> - return verify_and_format_tag(sha1, ref, fmt_pretty, flags);
> + if (gpg_verify_tag(sha1, ref, flags))
> + return -1;
> +
> + pretty_print_ref(name, sha1, fmt_pretty);
> + return 0;
> }
>
> static int do_sign(struct strbuf *buffer)
> diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
> index 212449f47..b3f08f705 100644
> --- a/builtin/verify-tag.c
> +++ b/builtin/verify-tag.c
> @@ -58,10 +58,19 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix)
> while (i < argc) {
> unsigned char sha1[20];
> const char *name = argv[i++];
> - if (get_sha1(name, sha1))
> +
> + if (get_sha1(name, sha1)) {
> had_error = !!error("tag '%s' not found.", name);
> - else if (verify_and_format_tag(sha1, name, fmt_pretty, flags))
> + continue;
> + }
> +
> + if (gpg_verify_tag(sha1, name, flags)) {
> had_error = 1;
> + continue;
> + }
> +
> + if (fmt_pretty)
> + pretty_print_ref(name, sha1, fmt_pretty);
> }
> return had_error;
> }
>
> which I think is still an improvement (the printing, rather than being
> an obscure parameter to the overloaded verify_and_format_tag()
> function, is clearly a first-class operation).
>
> -Peff
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v5 5/7] builtin/tag: add --format argument for tag -v
From: Santiago Torres @ 2017-01-17 17:34 UTC (permalink / raw)
To: Jeff King; +Cc: git, gitster, sunshine, walters, Lukas Puehringer
In-Reply-To: <20170117173239.ir6jxbz46vwwzoht@sigill.intra.peff.net>
[-- Attachment #1: Type: text/plain, Size: 301 bytes --]
> VERBOSE|QUIET _does_ have a meaning, which is "show the payload, but do
> not print the signature buffer". Perhaps just renaming QUIET to
> OMIT_STATUS or something would make it more clear.
>
Let me give this a go too. OMIT_STATUS does sound less confusing.
Thanks,
-Santiago.
> -Peff
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: gitk pull request // was: Re: gitk: "lime" color incompatible with older Tk versions
From: Junio C Hamano @ 2017-01-17 18:34 UTC (permalink / raw)
To: Paul Mackerras
Cc: David Aguilar, Stefan Beller, Andrew Janke, git@vger.kernel.org
In-Reply-To: <xmqqy3yba1jg.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> Paul Mackerras <paulus@ozlabs.org> writes:
>
>>> Paul, is it a good time to pull, or do you still have something not
>>> published yet that should go together with what you have already
>>> queued?
>>
>> I recently pushed out one more commit to update the Russian
>> translation from Dimitriy Ryazantcev. The head is now 8fef3f36b779.
>> I have a couple more series that I am currently reviewing, but nothing
>> immediately ready to publish. It would be a good time for you to do a
>> pull, since the "lime" color fix and the memory consumption fixes
>> should be helpful for a lot of people.
>
> Thanks. I did want to get the memory consumption fix sooner rather
> than later, and this is very much appreciated.
>
> Pulled.
Hmph. I am getting these:
SUBDIR gitk-git
Generating catalog po/sv.msg
msgfmt --statistics --tcl po/sv.po -l sv -d po/
po/sv.po:1388: duplicate message definition...
po/sv.po:380: ...this is the location of the first definition
msgfmt: found 1 fatal error
make[1]: *** [po/sv.msg] Error 1
make: *** [all] Error 2
Anybody else see this?
^ permalink raw reply
* Re: [RFC] Add support for downloading blobs on demand
From: Jeff King @ 2017-01-17 18:42 UTC (permalink / raw)
To: Ben Peart; +Cc: git, benpeart
In-Reply-To: <20170113155253.1644-1-benpeart@microsoft.com>
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=utf-8, Size: 9263 bytes --]
This is an issue I've thought a lot about. So apologies in advance that
this response turned out a bit long. :)
On Fri, Jan 13, 2017 at 10:52:53AM -0500, Ben Peart wrote:
> Design
> ~~~~~~
>
> Clone and fetch will pass a “--lazy-clone” flag (open to a better name
> here) similar to “--depth” that instructs the server to only return
> commits and trees and to ignore blobs.
>
> Later during git operations like checkout, when a blob cannot be found
> after checking all the regular places (loose, pack, alternates, etc),
> git will download the missing object and place it into the local object
> store (currently as a loose object) then resume the operation.
Have you looked at the "external odb" patches I wrote a while ago, and
which Christian has been trying to resurrect?
http://public-inbox.org/git/20161130210420.15982-1-chriscool@tuxfamily.org/
This is a similar approach, though I pushed the policy for "how do you
get the objects" out into an external script. One advantage there is
that large objects could easily be fetched from another source entirely
(e.g., S3 or equivalent) rather than the repo itself.
The downside is that it makes things more complicated, because a push or
a fetch now involves three parties (server, client, and the alternate
object store). So questions like "do I have all the objects I need" are
hard to reason about.
If you assume that there's going to be _some_ central Git repo which has
all of the objects, you might as well fetch from there (and do it over
normal git protocols). And that simplifies things a bit, at the cost of
being less flexible.
> To prevent git from accidentally downloading all missing blobs, some git
> operations are updated to be aware of the potential for missing blobs.
> The most obvious being check_connected which will return success as if
> everything in the requested commits is available locally.
Actually, Git is pretty good about trying not to access blobs when it
doesn't need to. The important thing is that you know enough about the
blobs to fulfill has_sha1_file() and sha1_object_info() requests without
actually fetching the data.
So the client definitely needs to have some list of which objects exist,
and which it _could_ get if it needed to.
The one place you'd probably want to tweak things is in the diff code,
as a single "git log -Sfoo" would fault in all of the blobs.
> To minimize the impact on the server, the existing dumb HTTP protocol
> endpoint “objects/<sha>” can be used to retrieve the individual missing
> blobs when needed.
This is going to behave badly on well-packed repositories, because there
isn't a good way to fetch a single object. The best case (which is not
implemented at all in Git) is that you grab the pack .idx, then grab
"slices" of the pack corresponding to specific objects, including
hunting down delta bases.
But then next time the server repacks, you have to throw away your .idx
file. And those can be big. The .idx for linux.git is 135MB. You really
wouldn't want to do an incremental fetch of 1MB worth of objects and
have to grab the whole .idx just to figure out which bytes you needed.
You can solve this by replacing the dumb-http server with a smart one
that actually serves up the individual objects as if they were truly
sitting on the filesystem. But then you haven't really minimized impact
on the server, and you might as well teach the smart protocols to do
blob fetches.
One big hurdle to this approach, no matter the protocol, is how you are
going to handle deltas. Right now, a git client tells the server "I have
this commit, but I want this other one". And the server knows which
objects the client has from the first, and which it needs from the
second. Moreover, it knows that it can send objects in delta form
directly from disk if the other side has the delta base.
So what happens in this system? We know we don't need to send any blobs
in a regular fetch, because the whole idea is that we only send blobs on
demand. So we wait for the client to ask us for blob A. But then what do
we send? If we send the whole blob without deltas, we're going to waste
a lot of bandwidth.
The on-disk size of all of the blobs in linux.git is ~500MB. The actual
data size is ~48GB. Some of that is from zlib, which you get even for
non-deltas. But the rest of it is from the delta compression. I don't
think it's feasible to give that up, at least not for "normal" source
repos like linux.git (more on that in a minute).
So ideally you do want to send deltas. But how do you know which objects
the other side already has, which you can use as a delta base? Sending
the list of "here are the blobs I have" doesn't scale. Just the sha1s
start to add up, especially when you are doing incremental fetches.
I think this sort of things performs a lot better when you just focus on
large objects. Because they don't tend to delta well anyway, and the
savings are much bigger by avoiding ones you don't want. So a directive
like "don't bother sending blobs larger than 1MB" avoids a lot of these
issues. In other words, you have some quick shorthand to communicate
between the client and server: this what I have, and what I don't.
Normal git relies on commit reachability for that, but there are
obviously other dimensions. The key thing is that both sides be able to
express the filters succinctly, and apply them efficiently.
> After cloning, the developer can use sparse-checkout to limit the set of
> files to the subset they need (typically only 1-10% in these large
> repos). This allows the initial checkout to only download the set of
> files actually needed to complete their task. At any point, the
> sparse-checkout file can be updated to include additional files which
> will be fetched transparently on demand.
If most of your benefits are not from avoiding blobs in general, but
rather just from sparsely populating the tree, then it sounds like
sparse clone might be an easier path forward. The general idea is to
restrict not just the checkout, but the actual object transfer and
reachability (in the tree dimension, the way shallow clone limits it in
the time dimension, which will require cooperation between the client
and server).
So that's another dimension of filtering, which should be expressed
pretty succinctly: "I'm interested in these paths, and not these other
ones." It's pretty easy to compute on the server side during graph
traversal (though it interacts badly with reachability bitmaps, so there
would need to be some hacks there).
It's an idea that's been talked about many times, but I don't recall
that there were ever working patches. You might dig around in the list
archive under the name "sparse clone" or possibly "narrow clone".
> Now some numbers
> ~~~~~~~~~~~~~~~~
>
> One repo has 3+ million files at tip across 500K folders with 5-6K
> active developers. They have done a lot of work to remove large files
> from the repo so it is down to < 100GB.
>
> Before changes: clone took hours to transfer the 87GB .pack + 119MB .idx
>
> After changes: clone took 4 minutes to transfer 305MB .pack + 37MB .idx
>
> After hydrating 35K files (the typical number any individual developer
> needs to do their work), there was an additional 460 MB of loose files
> downloaded.
It sounds like you have a case where the repository has a lot of large
files that are either historical, or uninteresting the sparse-tree
dimension.
How big is that 460MB if it were actually packed with deltas?
> Future Work
> ~~~~~~~~~~~
>
> The current prototype calls a new hook proc in sha1_object_info_extended
> and read_object, to download each missing blob. A better solution would
> be to implement this via a long running process that is spawned on the
> first download and listens for requests to download additional objects
> until it terminates when the parent git operation exits (similar to the
> recent long running smudge and clean filter work).
Yeah, see the external-odb discussion. Those prototypes use a process
per object, but I think we all agree after seeing how the git-lfs
interface has scaled that this is a non-starter. Recent versions of
git-lfs do the single-process thing, and I think any sort of
external-odb hook should be modeled on that protocol.
> Need to investigate an alternate batching scheme where we can make a
> single request for a set of "related" blobs and receive single a
> packfile (especially during checkout).
I think this sort of batching is going to be the really hard part to
retrofit onto git. Because you're throwing out the procedural notion
that you can loop over a set of objects and ask for each individually.
You have to start deferring computation until answers are ready. Some
operations can do that reasonably well (e.g., checkout), but something
like "git log -p" is constantly digging down into history. I suppose you
could just perform the skeleton of the operation _twice_, once to find
the list of objects to fault in, and the second time to actually do it.
That will make git feel a lot slower, because a lot of the illusion of
speed is the way it streams out results. OTOH, if you have to wait to
fault in objects from the network, it's going to feel pretty slow
anyway. :)
-Peff
^ permalink raw reply
* Re: submodule network operations [WAS: Re: [RFC/PATCH 0/4] working tree operations: support superprefix]
From: Stefan Beller @ 2017-01-17 18:43 UTC (permalink / raw)
To: Brian J. Davis; +Cc: Brandon Williams, git@vger.kernel.org, David Turner
In-Reply-To: <ebf6c90e-044f-5538-2325-601d002a81fe@gmail.com>
On Sun, Jan 15, 2017 at 1:02 PM, Brian J. Davis <bitminer@gmail.com> wrote:
>>
>> Technically it is submodule.<name>.url instead of
>> submodule.<path>.url. The name is usually the path initially, and once
>> you move the submodule, only the path changes, the name is supposed to
>> be constant and stay the same.
>
> I am not certain what is meant by this. All I know is I can use my
> "directory_to_checkout" above to place in tree relative from root the
> project any where in the tree not already tracked by git. You state name
> instead of path, but it allows path correct? Either that or I have gone off
> reservation with my use of git for years now. Maybe this is a deviation from
> how it is documented/should work and how it actually works? It works great
> how I use it.
Yes name can equal the path (and usually does). This is a minor detail
that is only relevant for renaming submodules, so ... maybe let's not
focus on it too much. :)
>>>
>>>
>>> but if say I want to pull from some server 2 and do a
>>>
>>> git submodule update --init --recursive
>>
>> That is why the "git submodule init" exists at all.
>>
>> git submodule init
>> $EDIT .git/config # change submodule.<name>.url to server2
>> git submodule update # that uses the adapted url and
>> # creates the submodule repository.
>>
>> # From now on the submodule is on its own.
>> cd <submodule> && git config --list
>> # prints an "origin" remote and a lot more
>>
>> For a better explanation, I started a documentation series, see
>>
>> https://github.com/gitster/git/commit/e2b51b9df618ceeff7c4ec044e20f5ce9a87241e
>>
>> (It is not finished, but that is supposed to explain this exact pain
>> point in the
>> STATES section, feel free to point out missing things or what is hard
>> to understand)
>
> I am not sure I got much out of the STATES section regarding my problem.
Your original problem as far as I understand is this:
You have a project with submodules.
The submodules are described in the .gitmodules file.
But the URL is pointing to an undesired location.
You want to rewrite the URLs before actually cloning the submodules.
And to solve this problem we need to perform multiple steps:
# --no is the default, just for clarity here:
git clone <project> --no-recurse-submodules
# The submodules are now in the *uninitialized* state
git submodule init
# the submodules are in the initialized state
git submodule update
# submodules are populated, i.e. cloned from
# the configured URLs and put into the working tree at
# the appropriate path.
Between the init and the update step you can modify the URLs.
These commands are just a repetition from the first email, but the
git commands can be viewed as moving from one state to another
for submodules; submodules itself can be seen as a state machine
according to that proposed documentation. Maybe such a state machine
makes it easier to understand for some people.
>>> what I would get is from someserver1 not someserver2 as there is no
>>> "origin"
>>> support for submodules. Is this correct? If so can origin support be
>>> added
>>> to submodules?
>>
>> Can you explain more in detail what you mean by origin support?
>
> Yes so when we do a:
>
> git push origin master
>
> origin is of course the Remote (Remotes
> https://git-scm.com/book/en/v2/Git-Basics-Working-with-Remotes)
>
> So I best use terminology "Remotes" support. Git push supports remotes, but
> git submodules does not. The basic idea is this:
>
> If Git allowed multiple submodule
> (https://git-scm.com/book/en/v2/Git-Tools-Submodules) Remotes to be
> specified say as an example:
>
> git submodule add [remote] [url]
>
> git submodule add origin https://github.com/chaconinc/DbConnector
> git submodule add indhouse https://indhouse .corp/chaconinc/DbConnector
>
> Where:
>
> [submodule "DbConnector"]
> path = DbConnector
> url = https://github.com/chaconinc/DbConnector
>
> Could then change to:
>
> [submodule "DbConnector"]
> path = DbConnector
> remote.origin.url = https://github.com/chaconinc/DbConnector
> remote.origin.url = https://indhouse .corp/chaconinc/DbConnector
here I assume there is a typo and the second remote.origin.url should be
remote.inhouse.url ?
>
>
> Then it should be possible to get:
>
> git submodules update --init --recursive
which would setup the submodule with both
[remote "origin"]
url = https://github.com/..
[remote "inhouse"]
url = https://inhouse.corp/..
But where do we clone it from?
(Or do we just do a "git init" on that submodule and fetch
from both remotes? in which order?)
>
> To support
>
> git submodules update [remote] --init --recursive
This would just clone/fetch from the specified remote?
If implementing this, we may run into a collision with the
specified submodules, what if a submodule is at
path "origin" ?
Does "git submodule update origin --init --recursive"
then mean to update the single "origin" submodule or
all submodules from their origin remote?
>
> And thus allow
>
> git submodules update origin --init --recursive
>
> git submodules update indhouse --init --recursive
understood. I like the idea of being able to specify
multiple remotes from the superproject..
^ permalink raw reply
* Re: [BUG/RFC] Git Gui: GIT_DIR leaks to spawned Git Bash
From: Max Kirillov @ 2017-01-17 18:42 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Max Kirillov, Pat Thoyts, git, Junio C Hamano
In-Reply-To: <alpine.DEB.2.20.1701171145050.3469@virtualbox>
On Tue, Jan 17, 2017 at 11:52:29AM +0100, Johannes Schindelin wrote:
> Hi Max,
>
> On Tue, 17 Jan 2017, Max Kirillov wrote:
>
>> Apparently various GIT_* environment variables (most
>> interesting is GIT_DIR but AFAIR there were more) leak to
>> shell session launched from Git Gui's "Git Bash" menu item.
...
> After all, you won't even notice unless you use the very
> same Git Bash to navigate to a *different* worktree.
That's right, I haven't been noticing it myself for quite a
time. But once I decided to tweak my habits to having only
one terminal window opened, and immediately noticed.
> And having said *that*, I have to admit that I was unable to reproduce
I have found exact way to reproduce it and actually the
reason. Turns out to be obvious mistake in code. PR is in
github: https://github.com/git-for-windows/git/pull/1032
--
Max
^ permalink raw reply
* Re: What's cooking in git.git (Jan 2017, #02; Sun, 15)
From: Junio C Hamano @ 2017-01-17 19:17 UTC (permalink / raw)
To: Jeff King; +Cc: Johannes Sixt, Johannes Schindelin, git
In-Reply-To: <20170116214411.a6wnp66vxydmpmgw@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Mon, Jan 16, 2017 at 09:33:07PM +0100, Johannes Sixt wrote:
>
>> However, Jeff's patch is intended to catch exactly these cases (not for the
>> cases where this happens accidentally, but when they happen with malicious
>> intent).
>>
>> We are talking about user-provided data that is reproduced by die() or
>> error(). I daresay that we do not have a single case where it is intended
>> that this data is intentionally multi-lined, like a commit message. It can
>> only be an accident or malicious when it spans across lines.
>>
>> I know we allow CR and LF in file names, but in all cases where such a name
>> appears in an error message, it is *not important* that the data is
>> reproduced exactly. On the contrary, it is usually more helpful to know that
>> something strange is going on. The question marks are a strong indication to
>> the user for this.
>
> Yes, exactly. Thanks for explaining this better than I obviously was
> doing. :)
Yup. In the "funny filename" case, the updated code indeed is doing
the right thing. So one remaining possible issue is if we ourselves
deliberately feed a raw CR (or any non-filtered control code) to
error() and friends because their native effect (like returning the
carriage to overwrite what we have already said with the remainder
of the message by using CR) to functions that go through vreportf()
interface. I personally do not think there is any---the progress
meter code does use CR for that but they don't do vreportf().
I do not think we write "message\r\n" even for Windows; we rely on
vfprintf() and other stdio layer to turn "message\n" into
"message\r\n" before it hits write(2) or its equivalent? So I
understand that this should affect LF and CRLF systems the same way
(meaning, no negative effect).
So... can we move this forward?
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox