* Re: [RFC for GIT] pull-request: add praise to people doing QA
From: Wolfram Sang @ 2017-01-19 20:43 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, linux-kernel
In-Reply-To: <xmqqlgubc04z.fsf@gitster.mtv.corp.google.com>
> So the idea is to have list of those whose names appear on
> Reviewed-by: and Tested-by: collected and listed after the list of
> commit titles and author names. I personally do not see much
> downsides in doing so, but I do not consume that many PRs myself, so
> let's hear from those who actually do process many of them.
Sadly, no further responses so far. Let's see if they will come if I
keep posting my pull requests with that information attached.
> As to the implementation, I am wondering if we can make this somehow
> work well with the "trailers" code we already have, instead of
> inventing yet another parser of trailers.
>
> In its current shape, "interpret-trailers" focuses on "editing" an
> existing commit log message to tweak the trailer lines. That mode
> of operation would help amending and rebasing, and to do that it
> needs to parse the commit log message, identify trailer blocks,
> parse out each trailer lines, etc.
>
> There is no fundamental reason why its output must be an edited
> original commit log message---it should be usable as a filter that
> picks trailer lines of the selected trailer type, like "Tested-By",
> etc.
I didn't know about trailers before. As I undestand it, I could use
"Tested-by" as the key, and the commit subject as the value. This list
then could be parsed and brought into proper output shape. It would
simplify the subject parsing, but most things my AWK script currently
does would still need to stay or to be reimplemented (extracting names
from tags, creating arrays of tags given by $name). Am I correct?
All under the assumption that trailers work on a range of commits. I
have to admit that adding this to git is beyond my scope.
Thanks for looking into it,
Wolfram
^ permalink raw reply
* [PATCH v6 3/3] Retire the scripted difftool
From: Johannes Schindelin @ 2017-01-19 20:30 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, David Aguilar, Dennis Kaarsemaker, Paul Sbarra
In-Reply-To: <cover.1484857756.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
* Re: [PATCH v5 3/3] Retire the scripted difftool
From: Johannes Schindelin @ 2017-01-19 20:32 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, David Aguilar, Dennis Kaarsemaker, Paul Sbarra
In-Reply-To: <xmqqlgu72asr.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Thu, 19 Jan 2017, Junio C Hamano wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> > Yep, as Git for Windows v2.11.0 is yesteryear's news, it was probably
> > obvious to you that I simply failed to spot and fix that oversight.
>
> OK, if you want to tweak log message of either 2/3 or 3/3 to correct,
> there is still time, as they are still outside 'next'.
Sent out v6 with the commit message reworded.
Ciao,
Johannes
^ permalink raw reply
* Re: problem with insider build for windows and git
From: Junio C Hamano @ 2017-01-19 20:32 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Michael Gooch, git
In-Reply-To: <alpine.DEB.2.20.1701192032330.3469@virtualbox>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>> are there any other topic that are already in 'master' that should go to
>> 2.11.x track?
>
> Personally, I would have merged 'nd/config-misc-fixes' into `maint`, I
> guess, and 'jc/abbrev-autoscale-config', and probably also 'jc/latin-1'.
The "almost ready" pushout were merging the ones that have been in
'master' for weeks (including that mingw-isatty topic). These three
are still on radar, but they were too young and that was the only
reason why they were not included in the batch.
> The 'rj/git-version-gen-do-not-force-abbrev' topic would be another
> candidate for inclusion. The 'ah/grammos' strikes me as obvious `maint`
> material, as well as 'ew/svn-fixes'.
I am holding back rj/git-version-gen-do-not-force-abbrev from 2.11.x
before 2.12 is released because I am a bit reluctant to tweak the
release infractructure in 'maint', before the same tweak hits
'master' and produces a release.
The second one will involve translators and that is why it is not
marked for back-merging in the draft release notes.
I agree that the svn thing should have been merged to 'maint' in
that batch, but I missed it.
> Having said that, these are the topics that *I* would merge into `maint`
> if I maintained Git. I don't, so this is just my opinion, man [*1*].
Yes, your opinion was exactly what was requested, and you gave one
;-)
^ permalink raw reply
* [PATCH v6 2/3] difftool: implement the functionality in the builtin
From: Johannes Schindelin @ 2017-01-19 20:30 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, David Aguilar, Dennis Kaarsemaker, Paul Sbarra
In-Reply-To: <cover.1484857756.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, and it will often need to perform unnecessary work (e.g. when
reading the entire config every time `git config` is called to query a
single config value).
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 later.
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 was 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.
The speedup is actually 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.
Still, it is an improvement.
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 v6 1/3] difftool: add a skeleton for the upcoming builtin
From: Johannes Schindelin @ 2017-01-19 20:30 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, David Aguilar, Dennis Kaarsemaker, Paul Sbarra
In-Reply-To: <cover.1484857756.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 v6 0/3] Turn the difftool into a builtin
From: Johannes Schindelin @ 2017-01-19 20:30 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 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 v5:
- reworded the commit message of 2/3 to account for the change in v4
where we no longer keep both scripted and builtin difftool working
(with the switch difftool.useBuiltin deciding which one is used).
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: ffac48d093d4b518a0cc0e8bf1b7cb53e0c3d7a2
Published-As: https://github.com/dscho/git/releases/tag/builtin-difftool-v6
Fetch-It-Via: git fetch https://github.com/dscho/git builtin-difftool-v6
--
2.11.0.windows.3
^ permalink raw reply
* [RESEND PATCHv2] contrib: remove git-convert-objects
From: Stefan Beller @ 2017-01-19 20:29 UTC (permalink / raw)
To: gitster; +Cc: git, torvalds, Stefan Beller
In-Reply-To: <20161228180205.29213-1-sbeller@google.com>
git-convert-objects, originally named git-convert-cache was used in
early 2005 to convert to a new repository format, e.g. adding an author
date.
By now the need for conversion of the very early repositories is less
relevant, we no longer need to keep it in contrib; remove it.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
v2:
no changes since v1, but as there were no replies nor do I find this
patch cooking, I decided to resend.
Thanks,
Stefan
contrib/convert-objects/convert-objects.c | 329 ------------------------
contrib/convert-objects/git-convert-objects.txt | 29 ---
2 files changed, 358 deletions(-)
delete mode 100644 contrib/convert-objects/convert-objects.c
delete mode 100644 contrib/convert-objects/git-convert-objects.txt
diff --git a/contrib/convert-objects/convert-objects.c b/contrib/convert-objects/convert-objects.c
deleted file mode 100644
index f3b57bf1d2..0000000000
--- a/contrib/convert-objects/convert-objects.c
+++ /dev/null
@@ -1,329 +0,0 @@
-#include "cache.h"
-#include "blob.h"
-#include "commit.h"
-#include "tree.h"
-
-struct entry {
- unsigned char old_sha1[20];
- unsigned char new_sha1[20];
- int converted;
-};
-
-#define MAXOBJECTS (1000000)
-
-static struct entry *convert[MAXOBJECTS];
-static int nr_convert;
-
-static struct entry * convert_entry(unsigned char *sha1);
-
-static struct entry *insert_new(unsigned char *sha1, int pos)
-{
- struct entry *new = xcalloc(1, sizeof(struct entry));
- hashcpy(new->old_sha1, sha1);
- memmove(convert + pos + 1, convert + pos, (nr_convert - pos) * sizeof(struct entry *));
- convert[pos] = new;
- nr_convert++;
- if (nr_convert == MAXOBJECTS)
- die("you're kidding me - hit maximum object limit");
- return new;
-}
-
-static struct entry *lookup_entry(unsigned char *sha1)
-{
- int low = 0, high = nr_convert;
-
- while (low < high) {
- int next = (low + high) / 2;
- struct entry *n = convert[next];
- int cmp = hashcmp(sha1, n->old_sha1);
- if (!cmp)
- return n;
- if (cmp < 0) {
- high = next;
- continue;
- }
- low = next+1;
- }
- return insert_new(sha1, low);
-}
-
-static void convert_binary_sha1(void *buffer)
-{
- struct entry *entry = convert_entry(buffer);
- hashcpy(buffer, entry->new_sha1);
-}
-
-static void convert_ascii_sha1(void *buffer)
-{
- unsigned char sha1[20];
- struct entry *entry;
-
- if (get_sha1_hex(buffer, sha1))
- die("expected sha1, got '%s'", (char *) buffer);
- entry = convert_entry(sha1);
- memcpy(buffer, sha1_to_hex(entry->new_sha1), 40);
-}
-
-static unsigned int convert_mode(unsigned int mode)
-{
- unsigned int newmode;
-
- newmode = mode & S_IFMT;
- if (S_ISREG(mode))
- newmode |= (mode & 0100) ? 0755 : 0644;
- return newmode;
-}
-
-static int write_subdirectory(void *buffer, unsigned long size, const char *base, int baselen, unsigned char *result_sha1)
-{
- char *new = xmalloc(size);
- unsigned long newlen = 0;
- unsigned long used;
-
- used = 0;
- while (size) {
- int len = 21 + strlen(buffer);
- char *path = strchr(buffer, ' ');
- unsigned char *sha1;
- unsigned int mode;
- char *slash, *origpath;
-
- if (!path || strtoul_ui(buffer, 8, &mode))
- die("bad tree conversion");
- mode = convert_mode(mode);
- path++;
- if (memcmp(path, base, baselen))
- break;
- origpath = path;
- path += baselen;
- slash = strchr(path, '/');
- if (!slash) {
- newlen += sprintf(new + newlen, "%o %s", mode, path);
- new[newlen++] = '\0';
- hashcpy((unsigned char *)new + newlen, (unsigned char *) buffer + len - 20);
- newlen += 20;
-
- used += len;
- size -= len;
- buffer = (char *) buffer + len;
- continue;
- }
-
- newlen += sprintf(new + newlen, "%o %.*s", S_IFDIR, (int)(slash - path), path);
- new[newlen++] = 0;
- sha1 = (unsigned char *)(new + newlen);
- newlen += 20;
-
- len = write_subdirectory(buffer, size, origpath, slash-origpath+1, sha1);
-
- used += len;
- size -= len;
- buffer = (char *) buffer + len;
- }
-
- write_sha1_file(new, newlen, tree_type, result_sha1);
- free(new);
- return used;
-}
-
-static void convert_tree(void *buffer, unsigned long size, unsigned char *result_sha1)
-{
- void *orig_buffer = buffer;
- unsigned long orig_size = size;
-
- while (size) {
- size_t len = 1+strlen(buffer);
-
- convert_binary_sha1((char *) buffer + len);
-
- len += 20;
- if (len > size)
- die("corrupt tree object");
- size -= len;
- buffer = (char *) buffer + len;
- }
-
- write_subdirectory(orig_buffer, orig_size, "", 0, result_sha1);
-}
-
-static unsigned long parse_oldstyle_date(const char *buf)
-{
- char c, *p;
- char buffer[100];
- struct tm tm;
- const char *formats[] = {
- "%c",
- "%a %b %d %T",
- "%Z",
- "%Y",
- " %Y",
- NULL
- };
- /* We only ever did two timezones in the bad old format .. */
- const char *timezones[] = {
- "PDT", "PST", "CEST", NULL
- };
- const char **fmt = formats;
-
- p = buffer;
- while (isspace(c = *buf))
- buf++;
- while ((c = *buf++) != '\n')
- *p++ = c;
- *p++ = 0;
- buf = buffer;
- memset(&tm, 0, sizeof(tm));
- do {
- const char *next = strptime(buf, *fmt, &tm);
- if (next) {
- if (!*next)
- return mktime(&tm);
- buf = next;
- } else {
- const char **p = timezones;
- while (isspace(*buf))
- buf++;
- while (*p) {
- if (!memcmp(buf, *p, strlen(*p))) {
- buf += strlen(*p);
- break;
- }
- p++;
- }
- }
- fmt++;
- } while (*buf && *fmt);
- printf("left: %s\n", buf);
- return mktime(&tm);
-}
-
-static int convert_date_line(char *dst, void **buf, unsigned long *sp)
-{
- unsigned long size = *sp;
- char *line = *buf;
- char *next = strchr(line, '\n');
- char *date = strchr(line, '>');
- int len;
-
- if (!next || !date)
- die("missing or bad author/committer line %s", line);
- next++; date += 2;
-
- *buf = next;
- *sp = size - (next - line);
-
- len = date - line;
- memcpy(dst, line, len);
- dst += len;
-
- /* Is it already in new format? */
- if (isdigit(*date)) {
- int datelen = next - date;
- memcpy(dst, date, datelen);
- return len + datelen;
- }
-
- /*
- * Hacky hacky: one of the sparse old-style commits does not have
- * any date at all, but we can fake it by using the committer date.
- */
- if (*date == '\n' && strchr(next, '>'))
- date = strchr(next, '>')+2;
-
- return len + sprintf(dst, "%lu -0700\n", parse_oldstyle_date(date));
-}
-
-static void convert_date(void *buffer, unsigned long size, unsigned char *result_sha1)
-{
- char *new = xmalloc(size + 100);
- unsigned long newlen = 0;
-
- /* "tree <sha1>\n" */
- memcpy(new + newlen, buffer, 46);
- newlen += 46;
- buffer = (char *) buffer + 46;
- size -= 46;
-
- /* "parent <sha1>\n" */
- while (!memcmp(buffer, "parent ", 7)) {
- memcpy(new + newlen, buffer, 48);
- newlen += 48;
- buffer = (char *) buffer + 48;
- size -= 48;
- }
-
- /* "author xyz <xyz> date" */
- newlen += convert_date_line(new + newlen, &buffer, &size);
- /* "committer xyz <xyz> date" */
- newlen += convert_date_line(new + newlen, &buffer, &size);
-
- /* Rest */
- memcpy(new + newlen, buffer, size);
- newlen += size;
-
- write_sha1_file(new, newlen, commit_type, result_sha1);
- free(new);
-}
-
-static void convert_commit(void *buffer, unsigned long size, unsigned char *result_sha1)
-{
- void *orig_buffer = buffer;
- unsigned long orig_size = size;
-
- if (memcmp(buffer, "tree ", 5))
- die("Bad commit '%s'", (char *) buffer);
- convert_ascii_sha1((char *) buffer + 5);
- buffer = (char *) buffer + 46; /* "tree " + "hex sha1" + "\n" */
- while (!memcmp(buffer, "parent ", 7)) {
- convert_ascii_sha1((char *) buffer + 7);
- buffer = (char *) buffer + 48;
- }
- convert_date(orig_buffer, orig_size, result_sha1);
-}
-
-static struct entry * convert_entry(unsigned char *sha1)
-{
- struct entry *entry = lookup_entry(sha1);
- enum object_type type;
- void *buffer, *data;
- unsigned long size;
-
- if (entry->converted)
- return entry;
- data = read_sha1_file(sha1, &type, &size);
- if (!data)
- die("unable to read object %s", sha1_to_hex(sha1));
-
- buffer = xmalloc(size);
- memcpy(buffer, data, size);
-
- if (type == OBJ_BLOB) {
- write_sha1_file(buffer, size, blob_type, entry->new_sha1);
- } else if (type == OBJ_TREE)
- convert_tree(buffer, size, entry->new_sha1);
- else if (type == OBJ_COMMIT)
- convert_commit(buffer, size, entry->new_sha1);
- else
- die("unknown object type %d in %s", type, sha1_to_hex(sha1));
- entry->converted = 1;
- free(buffer);
- free(data);
- return entry;
-}
-
-int main(int argc, char **argv)
-{
- unsigned char sha1[20];
- struct entry *entry;
-
- setup_git_directory();
-
- if (argc != 2)
- usage("git-convert-objects <sha1>");
- if (get_sha1(argv[1], sha1))
- die("Not a valid object name %s", argv[1]);
-
- entry = convert_entry(sha1);
- printf("new sha1: %s\n", sha1_to_hex(entry->new_sha1));
- return 0;
-}
diff --git a/contrib/convert-objects/git-convert-objects.txt b/contrib/convert-objects/git-convert-objects.txt
deleted file mode 100644
index f871880cfb..0000000000
--- a/contrib/convert-objects/git-convert-objects.txt
+++ /dev/null
@@ -1,29 +0,0 @@
-git-convert-objects(1)
-======================
-
-NAME
-----
-git-convert-objects - Converts old-style git repository
-
-
-SYNOPSIS
---------
-[verse]
-'git-convert-objects'
-
-DESCRIPTION
------------
-Converts old-style git repository to the latest format
-
-
-Author
-------
-Written by Linus Torvalds <torvalds@osdl.org>
-
-Documentation
---------------
-Documentation by David Greaves, Junio C Hamano and the git-list <git@vger.kernel.org>.
-
-GIT
----
-Part of the linkgit:git[7] suite
--
2.11.0.299.g762782ba8a
^ permalink raw reply related
* Re: [PATCH 2/2] Be more careful when determining whether a remote was configured
From: Junio C Hamano @ 2017-01-19 20:12 UTC (permalink / raw)
To: Jeff King; +Cc: Johannes Schindelin, git, Thomas Gummerer, Andrew Arnott
In-Reply-To: <20170119182721.7y2zzrbaalfqjjn6@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Wed, Jan 18, 2017 at 05:22:40PM +0100, Johannes Schindelin wrote:
>
>> > > I want to err on the side of caution. That's why.
>> >
>> > I guess I just don't see why changing the behavior with respect to
>> > "prune" or "proxy" is any less conservative than changing the one for
>> > "refspec".
>>
> I think _this_ is a much better way of framing the problem. It is not
> about which keys are set, but about _where_ they are set. IOW, a
> reasonable rule would be: if there is any remote.*.X in the repo config,
> then git-remote should consider it a configured repo. And otherwise, no
> matter what is in ~/.gitconfig or elsewhere, git-remote should proceed
> as if it doesn't exist (and repo-level config can take precedence over
> config defined elsewhere).
>
> I.e., something like this:
>
> diff --git a/remote.c b/remote.c
> index 298f2f93f..720d616be 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -373,6 +373,8 @@ static int handle_config(const char *key, const char *value, void *cb)
> }
> remote = make_remote(name, namelen);
> remote->origin = REMOTE_CONFIG;
> + if (current_config_scope() == CONFIG_SCOPE_REPO)
> + remote->configured = 1;
> if (!strcmp(subkey, "mirror"))
> remote->mirror = git_config_bool(key, value);
> else if (!strcmp(subkey, "skipdefaultupdate"))
>
> That doesn't make your test pass, but I think that is only because your
> test is not covering the interesting case (it puts the new config in the
> repo config, not in ~/.gitconfig).
>
> What do you think?
>
>> Originally, I would even have put the "vcs" into that set, as I could see
>> a legitimate use case for users to configure "remote.svn.vcs = vcs" in
>> ~/.gitconfig. But the regression test suite specifically tests for that
>> case, and I trust that there was a good reason, even if Thomas did not
>> describe that good reason in the commit message nor in any reply to this
>> patch pair.
>
> The config-scope thing above would allow "remote.svn.vcs" in
> ~/.gitconfig. But I don't think the test script actually checks that; it
> checks for the repo-level config. And we would continue to do the right
> thing there.
I am not "you" you are addressing to, but I think tying it to where
the variable came from makes quite sense.
Because it makes it no longer possible to just inspect the
configured result to answer "is the remote configured?",
introduction of the configured field also needs to be preserved from
the original by Dscho, so does reading from historical non-config
sources like $GIT_DIR/remotes/*, which are by definition
per-repository thing.
IOW, with this tweak (and not setting ->configured based on what
keys are set), I think Dscho's patch makes sense.
^ permalink raw reply
* Re: [PATCH 2/2] Be more careful when determining whether a remote was configured
From: Jeff King @ 2017-01-19 20:22 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Schindelin, git, Thomas Gummerer, Andrew Arnott
In-Reply-To: <xmqq4m0u24hs.fsf@gitster.mtv.corp.google.com>
On Thu, Jan 19, 2017 at 12:12:47PM -0800, Junio C Hamano wrote:
> > The config-scope thing above would allow "remote.svn.vcs" in
> > ~/.gitconfig. But I don't think the test script actually checks that; it
> > checks for the repo-level config. And we would continue to do the right
> > thing there.
>
> I am not "you" you are addressing to, but I think tying it to where
> the variable came from makes quite sense.
>
> Because it makes it no longer possible to just inspect the
> configured result to answer "is the remote configured?",
> introduction of the configured field also needs to be preserved from
> the original by Dscho, so does reading from historical non-config
> sources like $GIT_DIR/remotes/*, which are by definition
> per-repository thing.
>
> IOW, with this tweak (and not setting ->configured based on what
> keys are set), I think Dscho's patch makes sense.
Yeah, worry if that wasn't clear: the hunk I posted was a just a
partial. The actual thing I built and ran against the test suite was
exactly as you described.
-Peff
^ permalink raw reply
* Re: [PATCHv3 1/4] cache.h: document index_name_pos
From: Junio C Hamano @ 2017-01-19 20:17 UTC (permalink / raw)
To: Stefan Beller; +Cc: git
In-Reply-To: <20170119031854.4570-2-sbeller@google.com>
Stefan Beller <sbeller@google.com> writes:
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> cache.h | 19 +++++++++++++++++++
> 1 file changed, 19 insertions(+)
>
> diff --git a/cache.h b/cache.h
> index 1b67f078dd..1469ddeafe 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -575,7 +575,26 @@ extern int verify_path(const char *path);
> extern int index_dir_exists(struct index_state *istate, const char *name, int namelen);
> extern void adjust_dirname_case(struct index_state *istate, char *name);
> extern struct cache_entry *index_file_exists(struct index_state *istate, const char *name, int namelen, int igncase);
> +
> +/*
> + * Searches for an entry defined by name and namelen in the given index.
> + * If the return value is positive (including 0) it is the position of an
> + * exact match. If the return value is negative, the negated value minus 1
> + * is the position where the entry would be inserted.
So if the return value is -3, you negate it to get 3 and then
subtract 1 to get 2. The function is telling you that "e" will
sit at active_cache[2] in the following example.
Which is correct.
> + * Example: The current index consists of these files and its stages:
> + *
> + * b#0, d#0, f#1, f#3
> + *
> + * index_name_pos(&index, "a", 1) -> -1
> + * index_name_pos(&index, "b", 1) -> 0
> + * index_name_pos(&index, "c", 1) -> -2
> + * index_name_pos(&index, "d", 1) -> 1
> + * index_name_pos(&index, "e", 1) -> -3
> + * index_name_pos(&index, "f", 1) -> -3
> + * index_name_pos(&index, "g", 1) -> -5
> + */
This time the counting seems correct.
Thanks.
^ permalink raw reply
* Re: The design of refs backends, linked worktrees and submodules
From: Johannes Schindelin @ 2017-01-19 20:04 UTC (permalink / raw)
To: Michael Haggerty; +Cc: Duy Nguyen, Git Mailing List
In-Reply-To: <341999fc-4496-b974-c117-c18a2fca1358@alum.mit.edu>
Hi,
On Thu, 19 Jan 2017, Michael Haggerty wrote:
> On 01/19/2017 12:55 PM, Duy Nguyen wrote:
> > I've started working on fixing the "git gc" issue with multiple
> > worktrees, which brings me back to this. Just some thoughts. Comments
> > are really appreciated.
> >
> > In the current code, files backend has special cases for both
> > submodules (explicitly) and linked worktrees (hidden behind git_path).
>
> There is another terrible hack also needed to implement linked
> worktrees, namely that the `refs/bisect/` hierarchy is manually inserted
> into the `ref_cache`, because otherwise it wouldn't be noticed when
> iterating over loose references via `readdir()`.
>
> Other similar hacks would be required if other reference subtrees are
> declared to be per-worktree.
>
> > But if a backend has to handle this stuff, all future backends have to
> > too. Which does not sound great. Imagine we have "something" in
> > addition to worktrees and submodules in future, then all backends have
> > to learn about it.
>
> Agreed, the status quo is not pretty.
>
> I kindof think that it would have been a better design to store the
> references for all linked worktrees in the main repository's ref-store.
> For example, the "bisect" refs for a worktree named "<name>" could have
> been stored under "refs/worktrees/<name>/bisect/*".
That strikes me as a good design, indeed. It addresses very explicitly the
root cause of the worktree problems: Git's source code was developed for
years with the assumption that there is a 1:1 mapping between ref names
and SHA-1s in each repository, and the way worktrees were implemented
broke that assumption.
So introducing a new refs/ namespace -- that is visible to all other
worktrees -- would have addressed that problem.
This, BTW, is related to my concerns about introducing a "shadow" config
layer for worktrees: I still think it would be a bad idea, and very likely
to cause regressions in surprising ways, to allow such config "overlays"
per-worktree, as Git's current code's assumption is that there is only one
config per repository, and that it can, say, set one config setting to
match another (which in the per-worktree case would possibly hold true in
only one worktree only). Instead, introducing a new "namespace" in the
(single) config similar to refs/worktrees/<name> could address that
problem preemptively.
Ciao,
Johannes
^ permalink raw reply
* Re: [PATCH v5 0/4] Per-worktree config file support
From: Stefan Beller @ 2017-01-19 20:03 UTC (permalink / raw)
To: Duy Nguyen
Cc: git@vger.kernel.org, Junio C Hamano, Michael J Gruber,
Jens Lehmann, Lars Schneider, Michael Haggerty, Max Kirillov
In-Reply-To: <CACsJy8BHgc8o+SydeiVnqaZRCbkJEWVzqDZM4sgey04ZLtG3tQ@mail.gmail.com>
On Thu, Jan 19, 2017 at 4:09 AM, Duy Nguyen <pclouds@gmail.com> wrote:
> On Wed, Jan 11, 2017 at 12:01 AM, Stefan Beller <sbeller@google.com> wrote:
>> On Tue, Jan 10, 2017 at 3:25 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
>>> Let's get this rolling again. To refresh your memory because it's half
>>> a year since v4 [1], this is about letting each worktree in multi
>>> worktree setup has their own config settings. The most prominent ones
>>> are core.worktree, used by submodules, and core.sparseCheckout.
>>
>> Thanks for getting this rolling again.
>>
>>>
>>> This time I'm not touching submodules at all. I'm leaving it in the
>>> good hands of "submodule people". All I'm providing is mechanism. How
>>> you use it is up to you. So the series benefits sparse checkout users
>>> only.
>>
>> As one of the "submodule people", I have no complaints here.
>>
>>>
>>> Not much has changed from v4, except that the migration to new config
>>> layout is done automatically _update_ a config variable with "git
>>> config --worktree".
>>>
>>> I think this one is more or less ready. I have an RFC follow-up patch
>>> about core.bare, but that could be handled separately.
>>
>> I have read through the series and think the design is sound for worktrees
>> (though I have little knowledge about them).
>
> Submodules and multi worktrees start to look very similar, the more I
> think about it. Well, except that multi worktree does not separate odb
> and config files, maybe.
Similar to worktrees submodules can appear and disappear without
affecting the project/main tree. (though the mechanism is different,
for submodules, you'd checkout a version that doesn't have the submodule,
whereas for worktrees the user explicitely says: "I don't want to see this
worktree any more")
> And we have already seen both have a need to
> share code (like the moving .git dir operation). I suspect I'll learn
> more about submodules along the way, and you worktrees ;-)
I sure hope so.
>
>> Now even further:
>>
>> So to build on top of this series, I'd like to make submodules usable
>> with worktrees (i.e. shared object store, only clone/fetch once and
>> all worktrees
>> benefit from it), the big question is how to get the underlying data
>> model right.
>>
>> Would a submodule go into the superprojects
>>
>> .git/worktrees/<worktree-name>/modules/<submodule-name>/
>>
>> or rather
>>
>> .git/modules<submodule-name>/worktrees/<worktree-name>
>>
>> Or both (one of them being a gitlink file pointing at the other?)
>>
>> I have not made up my mind, as I haven't laid out all cases that are
>> relevant here.
>
> I would go with a conservative step first, keep submodules
> per-worktree. After it's sorted out. You can change the layout (e.g.
> as a config extension). The latter probably has some complication (but
> yeah sharing would be a big plus).
The sharing is what we are asked for as it would "make
submodules usable" (compared to the repo tool, which
doesn't have object sharing AFAIK). ;)
Currently I am leaning to put the worktree directory first and the
submodules within, i.e.
.git/worktrees/<worktree-name>/modules/<submodule-name>/
but in that directory, we'd only have the per-worktree
specific stuff, the object store would live with the superprojects
main worktree, i.e. at .git/modules/<submodule-name> we'd have
the main git dir for the submodule.
Thanks,
Stefan
> --
> Duy
^ permalink raw reply
* Re: problem with insider build for windows and git
From: Johannes Schindelin @ 2017-01-19 19:54 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Michael Gooch, git
In-Reply-To: <xmqq8tq8b4mr.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Wed, 18 Jan 2017, Junio C Hamano wrote:
> Aside from the "ouch, one topic has merged earlier iteration, that
> was merged to 'master', also now merged to 'maint', and we need to
> follow up on both" you sent out earlier,
I know of one more "ouch" moment where my latest iterations did not get
picked up: my latest version of the "Avoid a segmentation fault with
renaming merges" patch did not output an error message in case of !nce
because the code flow will result in more appropriate error messages later
anyway. I did not provide a follow-up patch for that because the current
version in `maint` is not wrong per se.
> are there any other topic that are already in 'master' that should go to
> 2.11.x track?
Personally, I would have merged 'nd/config-misc-fixes' into `maint`, I
guess, and 'jc/abbrev-autoscale-config', and probably also 'jc/latin-1'.
The 'rj/git-version-gen-do-not-force-abbrev' topic would be another
candidate for inclusion. The 'ah/grammos' strikes me as obvious `maint`
material, as well as 'ew/svn-fixes'. I have no opinion on the p4 topics
(five, by my counting), as I have no experience with (or for that matter,
need for) Perforce, but Lars might have a strong opinion on those.
Having said that, these are the topics that *I* would merge into `maint`
if I maintained Git. I don't, so this is just my opinion, man [*1*].
Ciao,
Johannes
Footnote *1*: While you read that last part of the sentence, imagine me in
slippers and a bathrobe, with a White Russian in my left hand for which I
used milk instead of cream (for the White Russian, that is, not for my
left hand).
^ permalink raw reply
* Re: The design of refs backends, linked worktrees and submodules
From: Junio C Hamano @ 2017-01-19 19:44 UTC (permalink / raw)
To: Duy Nguyen; +Cc: Michael Haggerty, Git Mailing List
In-Reply-To: <CACsJy8CHoroX2k9GqOFmXkvvPCPN4SBeCg+6aC2WSWNSKVmWQw@mail.gmail.com>
Duy Nguyen <pclouds@gmail.com> writes:
> ... A bit worried about transactions though,
> because I think per-repo and per-worktree updates will be separated in
> two transactions. But that's probably ok because future backends, like
> lmdb, will have to go through the same route anyway.
If we have per-worktree ref-store, what can be done as a useful
thing by future backends like lmdb may be to use the same single
database to store shared and per-worktree refs, similar to the way
Michael mentioned in his response to your message I am responding
to, i.e.
I kindof think that it would have been a better design to store the
references for all linked worktrees in the main repository's ref-store.
For example, the "bisect" refs for a worktree named "<name>" could have
been stored under "refs/worktrees/<name>/bisect/*".
The current design is heavily influenced by how "contrib/workdir"
lays things out, for the latter of which I take the blame X-<, but
perhaps the files backend can be retrofitted to use that layout in
the longer term?
^ permalink raw reply
* Re: [PATCH v5 3/3] log --graph: customize the graph lines with config log.graphColors
From: Junio C Hamano @ 2017-01-19 19:34 UTC (permalink / raw)
To: Jeff King; +Cc: Nguyễn Thái Ngọc Duy, git
In-Reply-To: <xmqq1svy3nxi.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
>> Since there's only one line that cares about the result of "colors",
>> maybe it would be less confusing to do:
>>
>> if (!git_config_get-string("log.graphcolors", &string)) {
>> ... parse, etc ...
>> graph_set_column_colors(colors.argv, colors.argc - 1);
>> } else {
>> graph_set_column_colors(column_colors_ansi,
>> column_colors_ansi_max);
>> }
>
> Yes, that would be much much less confusing. By doing so, the cover
> letter can lose "pushing the limits of abuse", too ;-).
Will queue with a squashable change.
Thanks.
graph.c | 18 ++++++++----------
1 file changed, 8 insertions(+), 10 deletions(-)
diff --git a/graph.c b/graph.c
index 822d40ae20..00aeee36d8 100644
--- a/graph.c
+++ b/graph.c
@@ -229,22 +229,20 @@ struct git_graph *graph_init(struct rev_info *opt)
struct git_graph *graph = xmalloc(sizeof(struct git_graph));
if (!column_colors) {
- struct argv_array ansi_colors = {
- column_colors_ansi,
- column_colors_ansi_max + 1
- };
- struct argv_array *colors = &ansi_colors;
char *string;
-
- if (!git_config_get_string("log.graphcolors", &string)) {
+ if (git_config_get_string("log.graphcolors", &string)) {
+ /* not configured -- use default */
+ graph_set_column_colors(column_colors_ansi,
+ column_colors_ansi_max);
+ } else {
static struct argv_array custom_colors = ARGV_ARRAY_INIT;
argv_array_clear(&custom_colors);
parse_graph_colors_config(&custom_colors, string);
free(string);
- colors = &custom_colors;
+ /* graph_set_column_colors takes a max-index, not a count */
+ graph_set_column_colors(custom_colors.argv,
+ custom_colors.argc - 1);
}
- /* graph_set_column_colors takes a max-index, not a count */
- graph_set_column_colors(colors->argv, colors->argc - 1);
}
graph->commit = NULL;
^ permalink raw reply related
* [RFC/PATCH] Disallow commands from within unpopulated submodules.
From: Stefan Beller @ 2017-01-19 19:30 UTC (permalink / raw)
To: git; +Cc: Stefan Beller
Consider you have a submodule at path "sub". What should happen in case
you run a command such as "git -C sub add ." ?
Here is what currently happens:
1) The submodule is populated, i.e. there is a .git (file/dir) inside
"sub". This is equivalent of running "git add ." in the submodule and
it behaves as you would expect, adding all files to the index.
2) The submodule is not populated or even not initialized.
For quite some time we got
$ git -C sub add .
git: pathspec.c:317: prefix_pathspec: Assertion `item->nowildcard_len <= item->len && item->prefix <= item->len' failed.
Aborted (core dumped)
(This is fixed by another patch in flight to not assert,
but rather die with a better message instead; but that patch is
merely a fix of a corner case in the pathspec code.)
While 1) is rather uncontroversial, there are multiple things the user
may have intended with this command in 2):
* add the submodule to the superproject
* add all files inside the sub/ directory to the submodule or
superproject.
It is unclear what the user intended, so rather error out instead.
Now let's ask the same question for "git -C sub status ." (which is a
command that is only reading and not writing to the repository)
1) If the submodule is populated, the user clearly intended to know
more about the submodules status
2) It is unclear if the user wanted to learn about the submodules state
(So ideally: "The submodule 'sub' is not initialized. To init ...")
or the status check should be applied to the superproject instead.
Avoid the confusion in 2) as well and just error out for now. Later on
we may want to add another flag to git.c to allow commands to be run
inside unpopulated submodules and each command reacts appropriately.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
This is the next logical step after sb/pathspec-errors (pathspec:
give better message for submodule related pathspec error). If you are in
a path that is clearly a submodules, I would expect that most users would
expect the git operation to apply to the submodule. In case of unpopulated
submodules, this is not the case though, but we apply the operation to the
superproject, which may be wrong or confusing. Hence just error out for now.
Later we may want to add a flag that allows specific commands to run in such
a setup (e.g. git status could give a fancier message than a die(..)).
I marked this as RFC
* to request for comments if this is a good idea from a UI-perspective
* because I did not adapt any test for this patch. (A lot of submodule tests
seem to break with this; From a cursory read of those tests, I'd rather
blame the tests for being sloppy than this patch damaging user expectations)
Thanks,
Stefan
git.c | 3 +++
submodule.c | 36 ++++++++++++++++++++++++++++++++++++
submodule.h | 1 +
3 files changed, 40 insertions(+)
diff --git a/git.c b/git.c
index bbaa949e9c..ca53b61474 100644
--- a/git.c
+++ b/git.c
@@ -2,6 +2,7 @@
#include "exec_cmd.h"
#include "help.h"
#include "run-command.h"
+#include "submodule.h"
const char git_usage_string[] =
"git [--version] [--help] [-C <path>] [-c name=value]\n"
@@ -364,6 +365,8 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
if (prefix)
die("can't use --super-prefix from a subdirectory");
}
+ if (prefix)
+ check_prefix_inside_submodule(prefix);
if (!help && p->option & NEED_WORK_TREE)
setup_work_tree();
diff --git a/submodule.c b/submodule.c
index 73521cdbb2..357a22b804 100644
--- a/submodule.c
+++ b/submodule.c
@@ -495,6 +495,42 @@ void set_config_fetch_recurse_submodules(int value)
config_fetch_recurse_submodules = value;
}
+/* check if the given prefix is inside an uninitialized submodule */
+void check_prefix_inside_submodule(const char *prefix)
+{
+ const char *work_tree = get_git_work_tree();
+ if (work_tree) {
+ int pos;
+ const struct cache_entry *in_submodule = NULL;
+
+ if (read_cache() < 0)
+ die("index file corrupt");
+ pos = cache_name_pos(prefix, strlen(prefix));
+ /*
+ * gitlinks are recored with no ending '/' in the index,
+ * but the prefix has an ending '/', so we will never find
+ * an exact match, but always the position where we'd
+ * insert the prefix.
+ */
+ if (pos < 0) {
+ const struct cache_entry *ce;
+ int len = strlen(prefix);
+ /* Check the previous position */
+ pos = (-1 - pos) - 1;
+ ce = active_cache[pos];
+ if (ce->ce_namelen < len)
+ len = ce->ce_namelen;
+ if (!memcmp(ce->name, prefix, len))
+ in_submodule = ce;
+ } else
+ /* This case cannot happen because */
+ die("BUG: prefixes end with '/', but we do not record ending slashes in the index");
+
+ if (in_submodule)
+ die(_("command from inside unpopulated submodule '%s' not supported."), in_submodule->name);
+ }
+}
+
static int has_remote(const char *refname, const struct object_id *oid,
int flags, void *cb_data)
{
diff --git a/submodule.h b/submodule.h
index b7576d6f43..6b30542822 100644
--- a/submodule.h
+++ b/submodule.h
@@ -30,6 +30,7 @@ struct submodule_update_strategy {
};
#define SUBMODULE_UPDATE_STRATEGY_INIT {SM_UPDATE_UNSPECIFIED, NULL}
+extern void check_prefix_inside_submodule(const char *prefix);
int is_staging_gitmodules_ok(void);
int update_path_in_gitmodules(const char *oldpath, const char *newpath);
int remove_path_from_gitmodules(const char *path);
--
2.11.0.299.g762782ba8a
^ permalink raw reply related
* Re: Git: new feature suggestion
From: Joao Pinto @ 2017-01-19 18:54 UTC (permalink / raw)
To: Linus Torvalds, Konstantin Khomoutov
Cc: Joao Pinto, Git Mailing List, CARLOS.PALMINHA@synopsys.com
In-Reply-To: <CA+55aFxAe8bH2xXkx1p5gYN+nc-D-vjNnfUeA_64Q3ttpbHq+w@mail.gmail.com>
Hi Linus,
Às 6:39 PM de 1/19/2017, Linus Torvalds escreveu:
> On Wed, Jan 18, 2017 at 10:33 PM, Konstantin Khomoutov
> <kostix+git@007spb.ru> wrote:
>>
>> Still, I welcome you to read the sort-of "reference" post by Linus
>> Torvalds [1] in which he explains the reasoning behind this approach
>> implemented in Git.
>
> It's worth noting that that discussion was from some _very_ early days
> in git (one week into the whole thing), when none of those
> visualization tools were actually implemented.
>
> Even now, ten years after the fact, plain git doesn't actually do what
> I outlined. Yes, "git blame -Cw" works fairly well, and is in general
> better than the traditional per-file "annotate". And yes, "git log
> --follow" does another (small) part of the outlined thing, but is
> really not very powerful.
>
> Some tools on top of git do more, but I think in general this is an
> area that could easily be improved upon. For example, the whole
> iterative and interactive drilling down in history of a particular
> file is very inconvenient to do with "git blame" (you find a commit
> that change the area in some way that you don't find interesting, so
> then you have to restart git blame with the parent of that
> unintersting commit).
>
> You can do it in tig, but I suspect a more graphical tool might be better.
>
> .. and we still end up having a lot of things where we simply just
> work with pathnames. For example, when doing merges, it' absolutely
> _wonderful_ doing
>
> gitk --merge <filename>
>
> to see what happened to that filename that has a conflict during the
> merge. But it's all based on the whole-file changes, and sometimes
> you'd like to see just the commits that generate one particular
> conflict (in the kernel, things like the MAINTAINERS file can have
> quite a lot of changes, but they are all pretty idnependent, and what
> you want to see is just "changes to this area").
>
> We do have the "-L" flag to git log, but it doesn't actually work for
> this particular case because of limitations.
>
> So what I'm trying to say is that the argument from 10+ years ago that
> "you can do better with intelligent tools after-the-fact" is very much
> true, but it's also true that we don't actually have all those
> intelligent tools, and this is an area that could still be improved
> upon. Some of them are actually available as add-ons in various
> graphical IDE's that use git.
>
> Linus
>
I am currently facing some challenges in one of Linux subsystems where a rename
of a set of folders and files would be the perfect scenario for future
development, but the suggestion is not accepted, not because it's not correct,
but because it makes the maintainer life harder in backporting bug fixes and new
features to older kernel versions and because it is not easy to follow the
renamed file/folder history from the kernel.org history logs.
Like nature shows us, the ability to adapt is the key for survival, so Linux
would gain a lot with some new features in git that can make maintainers life
easier. Assisted-backporting would be an excellent feature for them.
Did you ever thought about optimization backport operations through git or by an
add-on to it?
I am available to help if this feature makes sense for git users.
Thanks,
Joao
^ permalink raw reply
* Re: Git: new feature suggestion
From: Linus Torvalds @ 2017-01-19 19:16 UTC (permalink / raw)
To: Joao Pinto
Cc: Konstantin Khomoutov, Git Mailing List,
CARLOS.PALMINHA@synopsys.com
In-Reply-To: <991ef396-3fc3-27d6-283c-b8dffa10a7b7@synopsys.com>
On Thu, Jan 19, 2017 at 10:54 AM, Joao Pinto <Joao.Pinto@synopsys.com> wrote:
>
> I am currently facing some challenges in one of Linux subsystems where a rename
> of a set of folders and files would be the perfect scenario for future
> development, but the suggestion is not accepted, not because it's not correct,
> but because it makes the maintainer life harder in backporting bug fixes and new
> features to older kernel versions and because it is not easy to follow the
> renamed file/folder history from the kernel.org history logs.
Honestly, that's less of a git issue, and more of a "patch will not
apply across versions" issue.
No amount of rename detection will ever fix that, simply because the
rename hadn't even _happened_ in the old versions that things get
backported to.
("git cherry-pick" can do a merge resolution and thus do "backwards"
renaming too, so tooling can definitely help, but it still ends up
meaning that even trivial patches are no longer the _same_ trivial
patch across versions).
So renaming things increases maintainer workloads in those situations
regardless of any tooling issues.
(You may also be referring to the mellanox mess, where this issue is
very much exacerbated by having different groups working on the same
thing, and maintainers having very much a "I will not take _anything_
from any of the groups that makes my life more complicated" model,
because those groups fucked up so much in the past).
In other words, quite often issues are about workflows rather than
tools. The networking layer probably has more of this, because David
actually does the backports himself, so he _really_ doesn't want to
complicate things.
Linus
^ permalink raw reply
* Re: [PATCH v3 14/21] read-cache: touch shared index files when used
From: Junio C Hamano @ 2017-01-19 19:00 UTC (permalink / raw)
To: Duy Nguyen
Cc: Christian Couder, git, Ævar Arnfjörð Bjarmason,
Christian Couder
In-Reply-To: <CACsJy8B_xe6RtszPrncvDdSYosNUQxvhcEQ3DO_WO0sepXqvvQ@mail.gmail.com>
Duy Nguyen <pclouds@gmail.com> writes:
> On Mon, Jan 9, 2017 at 9:34 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Duy Nguyen <pclouds@gmail.com> writes:
>>
>>> On Sun, Jan 8, 2017 at 4:46 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>>> Christian Couder <christian.couder@gmail.com> writes:
>>>>
>>>>> So what should we do if freshen_file() returns 0 which means that the
>>>>> freshening failed?
>>>>
>>>> You tell me ;-) as you are the one who is proposing this feature.
>>>
>>> My answer is, we are not worse than freshening loose objects case
>>> (especially since I took the idea from there).
>>
>> I do not think so, unfortunately. Loose object files with stale
>> timestamps are not removed as long as objects are still reachable.
>
> But there are plenty of unreachable loose objects, added in index,
> then got replaced with new versions. cache-tree can create loose trees
> too and it's been run more often, behind user's back, to take
> advantage of the shortcut in unpack-trees.
I am not sure if I follow. Aren't objects reachable from the
cache-tree in the index protected from gc?
Not that I think freshening would actually fail in a repository
where you can actually write into to update the index or its refs to
make a difference (iow, even if we make it die() loudly when shared
index cannot be "touched" because we are paranoid, no real life
usage will trigger that die(), and if a repository does trigger the
die(), I think you would really want to know about it).
^ permalink raw reply
* Re: [PATCH] clear_delta_base_cache(): don't modify hashmap while iterating
From: Junio C Hamano @ 2017-01-19 19:16 UTC (permalink / raw)
To: Jeff King; +Cc: Ulrich Spörlein, Ed Maste, git
In-Reply-To: <20170119163350.zsfb33lmigkyljjh@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> Thanks. Here it is rolled up with a commit message.
>
> -- >8 --
> Subject: clear_delta_base_cache(): don't modify hashmap while iterating
>
> Removing entries while iterating causes fast-import to
> access an already-freed `struct packed_git`, leading to
> various confusing errors.
>
> What happens is that clear_delta_base_cache() drops the
> whole contents of the cache by iterating over the hashmap,
> calling release_delta_base_cache() on each entry. That
> function removes the item from the hashmap. The hashmap code
> may then shrink the table, but the hashmap_iter struct
> retains an offset from the old table.
>
> As a result, the next call to hashmap_iter_next() may claim
> that the iteration is done, even though some items haven't
> been visited.
>
> The only caller of clear_delta_base_cache() is fast-import,
> which wants to clear the cache because it is discarding the
> packed_git struct for its temporary pack. So by failing to
> remove all of the entries, we still have references to the
> freed packed_git.
>
> To make things even more confusing, this doesn't seem to
> trigger with the test suite, because it depends on
> complexities like the size of the hash table, which entries
> got cleared, whether we try to access them before they're
> evicted from the cache, etc.
>
> So I've been able to identify the problem with large
> imports like freebsd's svn import, or a fast-export of
> linux.git. But nothing that would be reasonable to run as
> part of the normal test suite.
>
> We can fix this easily by iterating over the lru linked list
> instead of the hashmap. They both contain the same entries,
> and we can use the "safe" variant of the list iterator,
> which exists for exactly this case.
>
> Let's also add a warning to the hashmap API documentation to
> reduce the chances of getting bit by this again.
>
> Reported-by: Ulrich Spörlein <uqs@freebsd.org>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
Makes sense. Thanks, both, for reporting, finding and fixing.
Will apply.
> Documentation/technical/api-hashmap.txt | 4 +++-
> sha1_file.c | 9 ++++-----
> 2 files changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/technical/api-hashmap.txt b/Documentation/technical/api-hashmap.txt
> index 28f5a8b71..a3f020cd9 100644
> --- a/Documentation/technical/api-hashmap.txt
> +++ b/Documentation/technical/api-hashmap.txt
> @@ -188,7 +188,9 @@ Returns the removed entry, or NULL if not found.
> `void *hashmap_iter_next(struct hashmap_iter *iter)`::
> `void *hashmap_iter_first(struct hashmap *map, struct hashmap_iter *iter)`::
>
> - Used to iterate over all entries of a hashmap.
> + Used to iterate over all entries of a hashmap. Note that it is
> + not safe to add or remove entries to the hashmap while
> + iterating.
> +
> `hashmap_iter_init` initializes a `hashmap_iter` structure.
> +
> diff --git a/sha1_file.c b/sha1_file.c
> index 1eb47f611..d20714d6b 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -2342,11 +2342,10 @@ static inline void release_delta_base_cache(struct delta_base_cache_entry *ent)
>
> void clear_delta_base_cache(void)
> {
> - struct hashmap_iter iter;
> - struct delta_base_cache_entry *entry;
> - for (entry = hashmap_iter_first(&delta_base_cache, &iter);
> - entry;
> - entry = hashmap_iter_next(&iter)) {
> + struct list_head *lru, *tmp;
> + list_for_each_safe(lru, tmp, &delta_base_cache_lru) {
> + struct delta_base_cache_entry *entry =
> + list_entry(lru, struct delta_base_cache_entry, lru);
> release_delta_base_cache(entry);
> }
> }
^ permalink raw reply
* Re: [RFC 2/2] grep: use '/' delimiter for paths
From: Brandon Williams @ 2017-01-19 18:29 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: git, gitster
In-Reply-To: <20170119150347.3484-3-stefanha@redhat.com>
On 01/19, Stefan Hajnoczi wrote:
> If the tree contains a sub-directory then git-grep(1) output contains a
> colon character instead of a path separator:
>
> $ git grep malloc v2.9.3:t
> v2.9.3:t:test-lib.sh: setup_malloc_check () {
> $ git show v2.9.3:t:test-lib.sh
> fatal: Path 't:test-lib.sh' does not exist in 'v2.9.3'
>
> This patch attempts to use the correct delimiter:
>
> $ git grep malloc v2.9.3:t
> v2.9.3:t/test-lib.sh: setup_malloc_check () {
> $ git show v2.9.3:t/test-lib.sh
> (success)
>
> This patch does not cope with @{1979-02-26 18:30:00} syntax and treats
> it as a path because it contains colons.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
> builtin/grep.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/grep.c b/builtin/grep.c
> index 3643d8a..06f8b47 100644
> --- a/builtin/grep.c
> +++ b/builtin/grep.c
> @@ -493,7 +493,8 @@ static int grep_object(struct grep_opt *opt, const struct pathspec *pathspec,
>
> /* Add a delimiter if there isn't one already */
> if (name[len - 1] != '/' && name[len - 1] != ':') {
> - strbuf_addch(&base, ':');
> + /* rev: or rev:path/ */
> + strbuf_addch(&base, strchr(name, ':') ? '/' : ':');
As Jeff mentioned it may be better to base which character gets appended
by checking the obj->type field like this maybe:
diff --git a/builtin/grep.c b/builtin/grep.c
index 69dab5dc5..9dfe11dc7 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -495,7 +495,8 @@ static int grep_object(struct grep_opt *opt, const struct pathspec *pathspec,
/* Add a delimiter if there isn't one already */
if (name[len - 1] != '/' && name[len - 1] != ':') {
/* rev: or rev:path/ */
- strbuf_addch(&base, strchr(name, ':') ? '/' : ':');
+ char del = obj->type == OBJ_COMMIT ? ':' : '/';
+ strbuf_addch(&base, del);
}
}
init_tree_desc(&tree, data, size);
--
Brandon Williams
^ permalink raw reply related
* Re: grep open pull requests
From: Jeff King @ 2017-01-19 19:02 UTC (permalink / raw)
To: Jack Bates; +Cc: git
In-Reply-To: <276f6ed9-ff06-a00f-b88a-4d40d55c6d40@nottheoilrig.com>
On Thu, Jan 19, 2017 at 11:12:03AM -0700, Jack Bates wrote:
> I have a couple questions around grepping among open pull requests.
>
> First, "git for-each-ref --no-merged": When I run the following,
> it lists refs/pull/1112/head, even though #1112 was merged in commit
> ced4da1. I guess this is because the tip of refs/pull/1112/head is 107fc59,
> not ced4da1?
Right. Git's `--no-merged` is about commit ancestry.
> This maybe shows my lack of familiarity with Git details,
> but superficially the two commits appear identical -- [1] and [2] --
> same parent, etc. Nonetheless they have different SHA-1s.
> I'm not sure why that is -- I didn't merge the commit --
> but regardless, GitHub somehow still connects ced4da1 with #1112.
The commits differ only in the committer timestamp. Try:
diff -u <(git cat-file commit 107fc5910) \
<(git cat-file commit ced4da132)
Is it possible that somebody cherry-pick or rebased it? Looking at the
history of apache/trafficserver, I don't see any merges, which implies
to me that the project is using a rebase workflow to merge PRs.
It's much trickier to find from the git topology whether a particular
history contains rebased versions of commits. You can look at the
--cherry options to "git log", which use patch-ids to try to equate
commits. Something like:
git for-each-ref --format='%(refname)' 'refs/pull/*/head' |
while read refname; do
if test -z "$(git rev-list --right-only --cherry-pick -1 origin...$refname)
then
echo "$refname: not merged"
fi
done
That's obviously much less efficient than `--no-merged`, but it should
generally work. The exception is if the rebase changed the commit
sufficiently that its patch-id may have changed.
> So my question is, how are they doing that,
I suspect the answer is "somebody clicked the rebase button GitHub"
which simultaneously did the rebase and marked the PR as merged.
> Lastly, a question more about GitHub than Git, but: Given the way GitHub is
> setup, I hope I can get a list of unmerged pull requests from Git alone. Can
> you think of a way to list *open* pull requests,
> or is that status only available out of band?
That information isn't reflected in the git topology. It's in GitHub's
database. You can ask the API:
https://developer.github.com/v3/
There are libraries to help with that:
https://developer.github.com/libraries/
I think that's probably the best answer to your "unmerged" question,
too. Ask the API which PRs are unmerged, and then do whatever git-level
analysis you want based on that.
-Peff
^ permalink raw reply
* Re: [RFC 2/2] grep: use '/' delimiter for paths
From: Junio C Hamano @ 2017-01-19 18:54 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: git
In-Reply-To: <20170119150347.3484-3-stefanha@redhat.com>
Stefan Hajnoczi <stefanha@redhat.com> writes:
> If the tree contains a sub-directory then git-grep(1) output contains a
> colon character instead of a path separator:
>
> $ git grep malloc v2.9.3:t
> v2.9.3:t:test-lib.sh: setup_malloc_check () {
> $ git show v2.9.3:t:test-lib.sh
> fatal: Path 't:test-lib.sh' does not exist in 'v2.9.3'
I am slightly less negative on this compared to 1/2, but not by a
big margin. The user wanted to feed a subtree to the command,
instead of doing the more natural
$ git grep -e malloc v2.9.3 -- t
So again, "contains a colon character" is not coming from what Git
does, but the user gave Git "a colon character" when she didn't have
to.
Having said that, if we wanted to start ignoring what the end-user
said in the initial input like 1/2 and 2/2 does (i.e. "this specific
tree object" as an input), I think the approach to solve for 1/2 and
2/2 should be the same. I think we should decide to do a slash
instead of a colon, not because v2.9.3: has colon at the end and
v2.9.3:t has colon in it, but because both of these are both bare
tree objects. The patches presented does not seem to base their
decision on the actual object type but on the textual input, which
seems wrong.
^ permalink raw reply
* Re: [RFC 1/2] grep: only add delimiter if there isn't one already
From: Junio C Hamano @ 2017-01-19 18:39 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: git
In-Reply-To: <20170119150347.3484-2-stefanha@redhat.com>
Stefan Hajnoczi <stefanha@redhat.com> writes:
> git-grep(1) output does not follow git's own syntax:
>
> $ git grep malloc v2.9.3:
> v2.9.3::Makefile: COMPAT_CFLAGS += -Icompat/nedmalloc
> $ git show v2.9.3::Makefile
> fatal: Path ':Makefile' does not exist in 'v2.9.3'
>
> This patch avoids emitting the unnecessary ':' delimiter if the name
> already ends with ':' or '/':
>
> $ git grep malloc v2.9.3:
> v2.9.3:Makefile: COMPAT_CFLAGS += -Icompat/nedmalloc
> $ git show v2.9.3:Makefile
> (succeeds)
I am mildly negative on this one. I suspect that the above example
is a made-up one and you might have a more compelling real-world use
case in mind, but at least the above does not convince me.
The end-user explicitly gave the extra ':' because she wanted to
feed the tree object, not a tag that leads to the tree, for some
reason. I think the output should respect that and show how she
spelled the starting point. IOW, it is not "':' added unnecessarily
by Git". It is ':' added by the end-user for whatever reason.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
> builtin/grep.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/grep.c b/builtin/grep.c
> index 462e607..3643d8a 100644
> --- a/builtin/grep.c
> +++ b/builtin/grep.c
> @@ -490,7 +490,11 @@ static int grep_object(struct grep_opt *opt, const struct pathspec *pathspec,
> strbuf_init(&base, PATH_MAX + len + 1);
> if (len) {
> strbuf_add(&base, name, len);
> - strbuf_addch(&base, ':');
> +
> + /* Add a delimiter if there isn't one already */
> + if (name[len - 1] != '/' && name[len - 1] != ':') {
> + strbuf_addch(&base, ':');
> + }
> }
> init_tree_desc(&tree, data, size);
> hit = grep_tree(opt, pathspec, &tree, &base, base.len,
^ 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