{"thread":{"id":"29856","subject":"[PATCH] config: Introduce --patience config variable","startedAt":"2012-03-06T10:59:42Z","lastAt":"2012-03-10T07:13:55Z","messageCount":23,"participants":["Michal Privoznik","Jeff King","Thomas Rast","Matthieu Moy","Junio C Hamano","Jakub Narebski","René Scharfe"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"186198","messageId":"a87ed689ddfb06601dd639541199fc72d829bdaf.1331031473.git.mprivozn@redhat.com","threadId":"29856","inReplyTo":null,"subject":"[PATCH] config: Introduce --patience config variable","fromName":"Michal Privoznik","fromEmail":"mprivozn@redhat.com","sentAt":"2012-03-06T10:59:42Z","receivedAt":"2012-03-06T10:59:42Z","isPatch":true,"sender":{"key":"mprivozn@redhat.com","avatar":"https://avatars.githubusercontent.com/u/12559331?v=4"},"body":"Some users like the patience diff more than the bare diff. However,\nspecifying the '--patience' argument every time diff is to be used\nis impractical. Moreover, creating an alias doesn't play with other\ntools nice, e.g. git-show; Therefore we need a configuration variable\nto turn this on/off across whole git tools.\n---\nPlease keep me CC'ed as I am not signed into list.\n\n Documentation/diff-config.txt | 3 +++\n diff.c | 9 +++++++++\n 2 files changed, 12 insertions(+), 0 deletions(-)\n\ndiff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt\nindex 1aed79e..b72b2fd 100644\n--- a/Documentation/diff-config.txt\n+++ b/Documentation/diff-config.txt\n@@ -86,6 +86,9 @@ diff.mnemonicprefix::\n diff.noprefix::\n \tIf set, 'git diff' does not show any source or destination prefix.\n \n+diff.patience:\n+ If set, 'git diff' will use patience algorithm.\n+\n diff.renameLimit::\n \tThe number of files to consider when performing the copy/rename\n \tdetection; equivalent to the 'git diff' option '-l'.\ndiff --git a/diff.c b/diff.c\nindex a1c06b5..8940d00 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -33,6 +33,7 @@ static int diff_mnemonic_prefix;\n static int diff_no_prefix;\n static int diff_dirstat_permille_default = 30;\n static struct diff_options default_diff_options;\n+static int diff_patience = 0;\n \n static char diff_colors[][COLOR_MAXLEN] = {\n \tGIT_COLOR_RESET,\n@@ -212,6 +213,11 @@ int git_diff_basic_config(const char *var, const char *value, void *cb)\n \tif (!prefixcmp(var, \"submodule.\"))\n \t\treturn parse_submodule_config_option(var, value);\n \n+ if (!strcmp(var, \"diff.patience\")) {\n+\t\tdiff_patience = git_config_bool(var, value);\n+\t\treturn 0;\n+ }\n+\n \treturn git_default_config(var, value, cb);\n }\n \n@@ -3202,6 +3208,9 @@ int diff_setup_done(struct diff_options *options)\n \t\tDIFF_OPT_SET(options, EXIT_WITH_STATUS);\n \t}\n \n+ if (diff_patience)\n+\t\tDIFF_XDL_SET(options, PATIENCE_DIFF);\n+\n \treturn 0;\n }\n \n-- \n1.7.3.4\n"},{"id":"186202","messageId":"20120306114914.GB6733@sigill.intra.peff.net","threadId":"29856","inReplyTo":"a87ed689ddfb06601dd639541199fc72d829bdaf.1331031473.git.mprivozn@redhat.com","subject":"Re: [PATCH] config: Introduce --patience config variable","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-03-06T11:49:14Z","receivedAt":"2012-03-06T11:49:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 06, 2012 at 11:59:42AM +0100, Michal Privoznik wrote:\n\n> Some users like the patience diff more than the bare diff. However,\n> specifying the '--patience' argument every time diff is to be used\n> is impractical. Moreover, creating an alias doesn't play with other\n> tools nice, e.g. git-show; Therefore we need a configuration variable\n> to turn this on/off across whole git tools.\n\nThe idea of turning on patience diff via config makes sense to me, and\nit is not a problem for plumbing tools to have patience diff on, since\npatience diffs are syntactically identical to non-patience diffs. So I\nthink the intent is good.\n\nA few comments on the patch itself:\n\n> --- a/Documentation/diff-config.txt\n> +++ b/Documentation/diff-config.txt\n> @@ -86,6 +86,9 @@ diff.mnemonicprefix::\n> diff.noprefix::\n> \tIf set, 'git diff' does not show any source or destination prefix.\n> \n> +diff.patience:\n> + If set, 'git diff' will use patience algorithm.\n> +\n\nShould this be a boolean? Or should we actually have a diff.algorithm\noption where you specify the algorithm you want (e.g., \"diff.algorithm =\npatience\")? That would free us up later to more easily add new values.\n\nIn particular, I am thinking about --minimal. It is mutually exclusive\nwith --patience, and is simply ignored if you use patience diff.\nwe perhaps have \"diff.algorithm\" which can be one of \"myers\", \"minimal\"\n(which is really myers + the minimal flag), and \"patience\".\n\nOr I suppose you could think of it as \"--minimal\" being orthogonal to\nthe algorithm chosen, and it is simply that \"--minimal\" does nothing\n(currently) with the patience algorithm.\n\n> @@ -3202,6 +3208,9 @@ int diff_setup_done(struct diff_options *options)\n> \t\tDIFF_OPT_SET(options, EXIT_WITH_STATUS);\n> \t}\n> \n> + if (diff_patience)\n> +\t\tDIFF_XDL_SET(options, PATIENCE_DIFF);\n> +\n> \treturn 0;\n\nHmm. Usually for a boolean config we would have \"-1\" mean \"not\nspecified\", and otherwise 0/1 for true/false. But in your case, setting\ndiff.patience to \"false\" will be the same as not setting it at all. I\nthink this is probably OK. \"false\" is the default, so you would only\nwant to specify it to override an earlier setting, but there is nothing\nearlier than config that you could possibly be overriding.\n\nSpeaking of overriding, you may want to actually override the config\noption from the command line. You probably should also add a\n\"--no-patience\" option, so that one can turn off \"diff.patience = true\"\nfor a particular invocation.\n\n-Peff\n"},{"id":"186206","messageId":"87pqcp6fyh.fsf@thomas.inf.ethz.ch","threadId":"29856","inReplyTo":"20120306114914.GB6733@sigill.intra.peff.net","subject":"Re: [PATCH] config: Introduce --patience config variable","fromName":"Thomas Rast","fromEmail":"trast@inf.ethz.ch","sentAt":"2012-03-06T13:01:42Z","receivedAt":"2012-03-06T13:01:42Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Jeff King writes:\n\n> On Tue, Mar 06, 2012 at 11:59:42AM +0100, Michal Privoznik wrote:\n>\n>> --- a/Documentation/diff-config.txt\n>> +++ b/Documentation/diff-config.txt\n>> @@ -86,6 +86,9 @@ diff.mnemonicprefix::\n>> diff.noprefix::\n>> \tIf set, 'git diff' does not show any source or destination prefix.\n>> \n>> +diff.patience:\n>> + If set, 'git diff' will use patience algorithm.\n>> +\n>\n> Should this be a boolean? Or should we actually have a diff.algorithm\n> option where you specify the algorithm you want (e.g., \"diff.algorithm =\n> patience\")? That would free us up later to more easily add new values.\n>\n> In particular, I am thinking about --minimal. It is mutually exclusive\n> with --patience, and is simply ignored if you use patience diff.\n> we perhaps have \"diff.algorithm\" which can be one of \"myers\", \"minimal\"\n> (which is really myers + the minimal flag), and \"patience\".\n\nDon't forget \"histogram\". I have no idea why it's not documented\n(evidently 8c912eea slipped through the review cracks) but --histogram\nis supported since 1.7.7.\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"186208","messageId":"f113867bcf2fec3210cd1a997e1398903b3bdd76.1331039505.git.trast@student.ethz.ch","threadId":"29856","inReplyTo":"87pqcp6fyh.fsf@thomas.inf.ethz.ch","subject":"[PATCH 1/2] perf: compare diff algorithms","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2012-03-06T13:15:01Z","receivedAt":"2012-03-06T13:15:01Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"8c912ee (teach --histogram to diff, 2011-07-12) claimed histogram diff\nwas faster than both Myers and patience.\n\nWe have since incorporated a performance testing framework, so add a\ntest that compares the various diff tasks performed in a real 'log -p'\nworkload. This does indeed show that histogram diff slightly beats\nMyers, while patience is much slower than the others.\n\nSigned-off-by: Thomas Rast \n---\n\nThe 3000 is pretty arbitrary but makes for a nice test duration.\n\nI'm reluctant to put numbers into the message, since the whole point\nof the perf test framework is that you can easily get them too. But\nhere's what I'm seeing:\n\n 4000.1: log -3000 (baseline) 0.04(0.02+0.01) \n 4000.2: log --raw -3000 (tree-only) 0.49(0.38+0.09) \n 4000.3: log -p -3000 (Myers) 1.93(1.75+0.17)\n 4000.4: log -p -3000 --histogram 1.90(1.74+0.15)\n 4000.5: log -p -3000 --patience 2.25(2.07+0.16)\n\n t/perf/p4000-diff-algorithms.sh | 29 +++++++++++++++++++++++++++++\n 1 file changed, 29 insertions(+)\n create mode 100755 t/perf/p4000-diff-algorithms.sh\n\ndiff --git a/t/perf/p4000-diff-algorithms.sh b/t/perf/p4000-diff-algorithms.sh\nnew file mode 100755\nindex 0000000..d6e505c\n--- /dev/null\n+++ b/t/perf/p4000-diff-algorithms.sh\n@@ -0,0 +1,29 @@\n+#!/bin/sh\n+\n+test_description=\"Tests diff generation performance\"\n+\n+. ./perf-lib.sh\n+\n+test_perf_default_repo\n+\n+test_perf 'log -3000 (baseline)' '\n+\tgit log -1000 >/dev/null\n+'\n+\n+test_perf 'log --raw -3000 (tree-only)' '\n+\tgit log --raw -3000 >/dev/null\n+'\n+\n+test_perf 'log -p -3000 (Myers)' '\n+\tgit log -p -3000 >/dev/null\n+'\n+\n+test_perf 'log -p -3000 --histogram' '\n+\tgit log -p -3000 --histogram >/dev/null\n+'\n+\n+test_perf 'log -p -3000 --patience' '\n+\tgit log -p -3000 --patience >/dev/null\n+'\n+\n+test_done\n-- \n1.7.9.2.467.g7fee4\n"},{"id":"186209","messageId":"a8d07a8e3860196053da7595f040707d3a3723ef.1331039505.git.trast@student.ethz.ch","threadId":"29856","inReplyTo":"f113867bcf2fec3210cd1a997e1398903b3bdd76.1331039505.git.trast@student.ethz.ch","subject":"[PATCH 2/2] Document the --histogram diff option","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2012-03-06T13:15:02Z","receivedAt":"2012-03-06T13:15:02Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Signed-off-by: Thomas Rast \n---\n\nThis is only the minimal update. I think in the long run, we should\nadd a note saying why we support all of them. BUt off hand I didn't\nhave any substantial evidence in favour of patience that could be used\nas an argument.\n\n Documentation/diff-options.txt | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\nindex 87f0a5f..7d4566f 100644\n--- a/Documentation/diff-options.txt\n+++ b/Documentation/diff-options.txt\n@@ -52,6 +52,9 @@ endif::git-format-patch[]\n --patience::\n \tGenerate a diff using the \"patience diff\" algorithm.\n \n+--histogram::\n+\tGenerate a diff using the \"histogram diff\" algorithm.\n+\n --stat[=[,[,]]]::\n \tGenerate a diffstat. By default, as much space as necessary\n \twill be used for the filename part, and the rest for the graph\n-- \n1.7.9.2.467.g7fee4\n"},{"id":"186210","messageId":"20120306133001.GA22153@sigill.intra.peff.net","threadId":"29856","inReplyTo":"87pqcp6fyh.fsf@thomas.inf.ethz.ch","subject":"Re: [PATCH] config: Introduce --patience config variable","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-03-06T13:30:01Z","receivedAt":"2012-03-06T13:30:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 06, 2012 at 02:01:42PM +0100, Thomas Rast wrote:\n\n> >> --- a/Documentation/diff-config.txt\n> >> +++ b/Documentation/diff-config.txt\n> >> @@ -86,6 +86,9 @@ diff.mnemonicprefix::\n> >> diff.noprefix::\n> >> \tIf set, 'git diff' does not show any source or destination prefix.\n> >> \n> >> +diff.patience:\n> >> + If set, 'git diff' will use patience algorithm.\n> >> +\n> >\n> > Should this be a boolean? Or should we actually have a diff.algorithm\n> > option where you specify the algorithm you want (e.g., \"diff.algorithm =\n> > patience\")? That would free us up later to more easily add new values.\n> >\n> > In particular, I am thinking about --minimal. It is mutually exclusive\n> > with --patience, and is simply ignored if you use patience diff.\n> > we perhaps have \"diff.algorithm\" which can be one of \"myers\", \"minimal\"\n> > (which is really myers + the minimal flag), and \"patience\".\n> \n> Don't forget \"histogram\". I have no idea why it's not documented\n> (evidently 8c912eea slipped through the review cracks) but --histogram\n> is supported since 1.7.7.\n\nAh, thanks. I had the vague feeling that we had a third algorithm\nalready, but I didn't see it in the docs. So yeah, I really think this\nshould be diff.algorithm, with a value of \"myers\", \"patience\", or\n\"histogram\" (and possibly \"minimal\", depending how we want to treat\nthat).\n\n-Peff\n"},{"id":"186211","messageId":"4F5611F1.4020309@redhat.com","threadId":"29856","inReplyTo":"87pqcp6fyh.fsf@thomas.inf.ethz.ch","subject":"Re: [PATCH] config: Introduce --patience config variable","fromName":"Michal Privoznik","fromEmail":"mprivozn@redhat.com","sentAt":"2012-03-06T13:32:33Z","receivedAt":"2012-03-06T13:32:33Z","isPatch":true,"sender":{"key":"mprivozn@redhat.com","avatar":"https://avatars.githubusercontent.com/u/12559331?v=4"},"body":"On 06.03.2012 14:01, Thomas Rast wrote:\n> Jeff King writes:\n> \n>> On Tue, Mar 06, 2012 at 11:59:42AM +0100, Michal Privoznik wrote:\n>>\n>>> --- a/Documentation/diff-config.txt\n>>> +++ b/Documentation/diff-config.txt\n>>> @@ -86,6 +86,9 @@ diff.mnemonicprefix::\n>>> diff.noprefix::\n>>> \tIf set, 'git diff' does not show any source or destination prefix.\n>>> \n>>> +diff.patience:\n>>> + If set, 'git diff' will use patience algorithm.\n>>> +\n>>\n>> Should this be a boolean? Or should we actually have a diff.algorithm\n>> option where you specify the algorithm you want (e.g., \"diff.algorithm =\n>> patience\")? That would free us up later to more easily add new values.\n>>\n>> In particular, I am thinking about --minimal. It is mutually exclusive\n>> with --patience, and is simply ignored if you use patience diff.\n>> we perhaps have \"diff.algorithm\" which can be one of \"myers\", \"minimal\"\n>> (which is really myers + the minimal flag), and \"patience\".\n> \n> Don't forget \"histogram\". I have no idea why it's not documented\n> (evidently 8c912eea slipped through the review cracks) but --histogram\n> is supported since 1.7.7.\n> \n\nOkay guys. I'll got with diff.algorithm = [patience | minimal |\nhistogram | myers] then. What I am not sure about is how to threat case\nwhen user have say algorithm = patience set in config but want to use\nmyers. I guess we need --myers option then, don't we?\n\nMichal\n"},{"id":"186212","messageId":"vpqr4x5hmt3.fsf@bauges.imag.fr","threadId":"29856","inReplyTo":"4F5611F1.4020309@redhat.com","subject":"Re: [PATCH] config: Introduce --patience config variable","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2012-03-06T13:38:16Z","receivedAt":"2012-03-06T13:38:16Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Michal Privoznik writes:\n\n> Okay guys. I'll got with diff.algorithm = [patience | minimal |\n> histogram | myers] then. What I am not sure about is how to threat case\n> when user have say algorithm = patience set in config but want to use\n> myers. I guess we need --myers option then, don't we?\n\nAt this point, maybe it's time to have\n--diff-algorithm=[patience|minimal|histogram|myers], and keep\n--patience, --minimal and --histogram just as compatibility aliases.\n\nHaving one option per algorithm feels wrong ...\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"186213","messageId":"20120306140911.GA22329@sigill.intra.peff.net","threadId":"29856","inReplyTo":"vpqr4x5hmt3.fsf@bauges.imag.fr","subject":"Re: [PATCH] config: Introduce --patience config variable","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-03-06T14:09:11Z","receivedAt":"2012-03-06T14:09:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 06, 2012 at 02:38:16PM +0100, Matthieu Moy wrote:\n\n> Michal Privoznik writes:\n> \n> > Okay guys. I'll got with diff.algorithm = [patience | minimal |\n> > histogram | myers] then. What I am not sure about is how to threat case\n> > when user have say algorithm = patience set in config but want to use\n> > myers. I guess we need --myers option then, don't we?\n> \n> At this point, maybe it's time to have\n> --diff-algorithm=[patience|minimal|histogram|myers], and keep\n> --patience, --minimal and --histogram just as compatibility aliases.\n> \n> Having one option per algorithm feels wrong ...\n\nYeah, that was going to be my suggestion, too. And explaining it that\nway in the docs will make it clear that \"--histogram\" overrides\n\"--patience\" and so forth.\n\n-Peff\n"},{"id":"186244","messageId":"7vhay1se0g.fsf@alter.siamese.dyndns.org","threadId":"29856","inReplyTo":"f113867bcf2fec3210cd1a997e1398903b3bdd76.1331039505.git.trast@student.ethz.ch","subject":"Re: [PATCH 1/2] perf: compare diff algorithms","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-06T19:52:47Z","receivedAt":"2012-03-06T19:52:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Rast writes:\n\n> 8c912ee (teach --histogram to diff, 2011-07-12) claimed histogram diff\n> was faster than both Myers and patience.\n>\n> We have since incorporated a performance testing framework, so add a\n> test that compares the various diff tasks performed in a real 'log -p'\n> workload. This does indeed show that histogram diff slightly beats\n> Myers, while patience is much slower than the others.\n>\n> Signed-off-by: Thomas Rast \n> ---\n\nFun.\n\nI am getting this (probably unrelated to this patch), by the way:\n\n$ make perf\nmake -C t/perf/ all\nmake[1]: Entering directory `/srv/project/git/git.git/t/perf'\nrm -rf test-results\n./run\n...\nperf 4 - grep --cached, expensive regex: 1 2 3 ok\n# passed all 4 test(s)\n1..4\nCan't locate Git.pm in @INC (@INC contains: /etc/perl ...) at ./aggregate.perl line 5.\nBEGIN failed--compilation aborted at ./aggregate.perl line 5.\n"},{"id":"186245","messageId":"7vd38psdt4.fsf@alter.siamese.dyndns.org","threadId":"29856","inReplyTo":"a8d07a8e3860196053da7595f040707d3a3723ef.1331039505.git.trast@student.ethz.ch","subject":"Re: [PATCH 2/2] Document the --histogram diff option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-06T19:57:11Z","receivedAt":"2012-03-06T19:57:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Rast writes:\n\n> Signed-off-by: Thomas Rast \n> ---\n>\n> This is only the minimal update. I think in the long run, we should\n> add a note saying why we support all of them. But off hand I didn't\n> have any substantial evidence in favour of patience that could be used\n> as an argument.\n\nIsn't the main argument made by proponents of patience diff is more\nreadable output, and not performance? That line of argument relies\non a fairly subjective test \"which one is easier to read?\", so it is\nhard to come up with a substantial evidence, unless somebody invests\nin A/B test.\n\nThanks, will queue for 1.7.7.x maintenance track and upwards.\n"},{"id":"186252","messageId":"87fwdl1mwl.fsf@thomas.inf.ethz.ch","threadId":"29856","inReplyTo":"7vd38psdt4.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/2] Document the --histogram diff option","fromName":"Thomas Rast","fromEmail":"trast@inf.ethz.ch","sentAt":"2012-03-06T20:42:50Z","receivedAt":"2012-03-06T20:42:50Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Junio C Hamano writes:\n\n> Thomas Rast writes:\n>\n>> Signed-off-by: Thomas Rast \n>> ---\n>>\n>> This is only the minimal update. I think in the long run, we should\n>> add a note saying why we support all of them. But off hand I didn't\n>> have any substantial evidence in favour of patience that could be used\n>> as an argument.\n>\n> Isn't the main argument made by proponents of patience diff is more\n> readable output, and not performance? That line of argument relies\n> on a fairly subjective test \"which one is easier to read?\", so it is\n> hard to come up with a substantial evidence, unless somebody invests\n> in A/B test.\n\nWell, I was just too lazy to look up what I dimly remembered people had\nposted at some point: examples where patience beats Myers for\nreadability. E.g.,\n\n http://article.gmane.org/gmane.comp.version-control.git/104316\n\nI don't think you need a blind test to justify that the patience result\nis more readable. So I think in the long run, the docs should say\nsomething like:\n\n--diff-algorithm={histogram|myers|minimal|patience}::\n Choose a diff algorithm. The variants are as follows:\n+\n--\nhistogram::\n This is the fastest algorithm, and thus the default.\n\nmyers::\n The classical Myers diff algorithm. \n\nminimal::\n Like 'myers', but spend extra time making sure that the diff\n is the shortest possible for the set of changes performed.\n\npatience::\n The patience diff algorithm, which first matches unique lines\n with each other. This sometimes results in more readable (if\n longer) patches than the other algorithms.\n--\n\nOr whatever -- magic is required to have a nested list in asciidoc.\nI can't be bothered to twiddle with that right now.\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"186254","messageId":"87y5rdzbpb.fsf@thomas.inf.ethz.ch","threadId":"29856","inReplyTo":"7vhay1se0g.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/2] perf: compare diff algorithms","fromName":"Thomas Rast","fromEmail":"trast@inf.ethz.ch","sentAt":"2012-03-06T21:00:48Z","receivedAt":"2012-03-06T21:00:48Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Junio C Hamano writes:\n\n>\n> I am getting this (probably unrelated to this patch), by the way:\n>\n> $ make perf\n> make -C t/perf/ all\n> make[1]: Entering directory `/srv/project/git/git.git/t/perf'\n> rm -rf test-results\n> ./run\n> ...\n> perf 4 - grep --cached, expensive regex: 1 2 3 ok\n> # passed all 4 test(s)\n> 1..4\n> Can't locate Git.pm in @INC (@INC contains: /etc/perl ...) at ./aggregate.perl line 5.\n> BEGIN failed--compilation aborted at ./aggregate.perl line 5.\n\nIt would seem that you are not installing Git.pm as part of your normal\ninstallation? aggregate.perl uses it, so you'd have to. Perhaps a Perl\nguru can tell us if it's possible to magically pull Git.pm from the\nbuild tree instead of the installed version...\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"186258","messageId":"7vr4x5qvgd.fsf@alter.siamese.dyndns.org","threadId":"29856","inReplyTo":"87y5rdzbpb.fsf@thomas.inf.ethz.ch","subject":"Re: [PATCH 1/2] perf: compare diff algorithms","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-06T21:18:58Z","receivedAt":"2012-03-06T21:18:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Rast writes:\n\n> Junio C Hamano writes:\n>\n>>\n>> I am getting this (probably unrelated to this patch), by the way:\n>>\n>> $ make perf\n>> make -C t/perf/ all\n>> make[1]: Entering directory `/srv/project/git/git.git/t/perf'\n>> rm -rf test-results\n>> ./run\n>> ...\n>> perf 4 - grep --cached, expensive regex: 1 2 3 ok\n>> # passed all 4 test(s)\n>> 1..4\n>> Can't locate Git.pm in @INC (@INC contains: /etc/perl ...) at ./aggregate.perl line 5.\n>> BEGIN failed--compilation aborted at ./aggregate.perl line 5.\n>\n> It would seem that you are not installing Git.pm as part of your normal\n> installation?\n\nI actually am installing it in a quite vanilla way.\n\nI think our installation procedure places Git.pm in git specific\nperl library path where a simple invocation of \"perl\" that is\ngit-unaware will not look into, and we make sure that our scripts\nstill find the matching version of Git.pm by having \"use lib\" at the\nbeginning that points at the right directory.\n\nBut of course, this from a command line would not work:\n\n\t$ perl -MGit\n\nI do not expect it to, and for the ease of testing new versions, I\nprefer it not to work.\n\nIn any case, you should be able to do anything under t/ _before_\ninstalling, so relying on having Git.pm in normal @INC is a double\nno-no.\n"},{"id":"186262","messageId":"m31up5tnjw.fsf@localhost.localdomain","threadId":"29856","inReplyTo":"7vr4x5qvgd.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/2] perf: compare diff algorithms","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2012-03-06T21:41:30Z","receivedAt":"2012-03-06T21:41:30Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Junio C Hamano writes:\n> Thomas Rast writes:\n>> Junio C Hamano writes:\n>>\n>>>\n>>> I am getting this (probably unrelated to this patch), by the way:\n>>>\n>>> $ make perf\n>>> make -C t/perf/ all\n>>> make[1]: Entering directory `/srv/project/git/git.git/t/perf'\n>>> rm -rf test-results\n>>> ./run\n>>> ...\n>>> perf 4 - grep --cached, expensive regex: 1 2 3 ok\n>>> # passed all 4 test(s)\n>>> 1..4\n>>> Can't locate Git.pm in @INC (@INC contains: /etc/perl ...) at ./aggregate.perl line 5.\n>>> BEGIN failed--compilation aborted at ./aggregate.perl line 5.\n>>\n>> It would seem that you are not installing Git.pm as part of your normal\n>> installation?\n> \n> I actually am installing it in a quite vanilla way.\n> \n> I think our installation procedure places Git.pm in git specific\n> perl library path where a simple invocation of \"perl\" that is\n> git-unaware will not look into, and we make sure that our scripts\n> still find the matching version of Git.pm by having \"use lib\" at the\n> beginning that points at the right directory.\n> \n> But of course, this from a command line would not work:\n> \n> \t$ perl -MGit\n> \n> I do not expect it to, and for the ease of testing new versions, I\n> prefer it not to work.\n> \n> In any case, you should be able to do anything under t/ _before_\n> installing, so relying on having Git.pm in normal @INC is a double\n> no-no.\n\nThomas, take a look at how it is solved in 't/t9700/test.pl', used by\n't/t9700-perl-git.sh':\n\n use lib (split(/:/, $ENV{GITPERLLIB}));\n\n-- \nJakub Narebski\n"},{"id":"186285","messageId":"7vlindp17d.fsf@alter.siamese.dyndns.org","threadId":"29856","inReplyTo":"20120306114914.GB6733@sigill.intra.peff.net","subject":"Re: [PATCH] config: Introduce --patience config variable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-07T02:57:42Z","receivedAt":"2012-03-07T02:57:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Tue, Mar 06, 2012 at 11:59:42AM +0100, Michal Privoznik wrote:\n>\n>> Some users like the patience diff more than the bare diff. However,\n>> specifying the '--patience' argument every time diff is to be used\n>> is impractical. Moreover, creating an alias doesn't play with other\n>> tools nice, e.g. git-show; Therefore we need a configuration variable\n>> to turn this on/off across whole git tools.\n>\n> The idea of turning on patience diff via config makes sense to me, and\n> it is not a problem for plumbing tools to have patience diff on, since\n> patience diffs are syntactically identical to non-patience diffs.\n\nEven though I do not strongly object to the general conclusion, I\nhave to point out that the last line above is a dangerous thought.\n\nIf you change the default diff algorithm, you will invalidate long\nterm caches that computed their keys based on the shape of patches\nproduced in the past. The prime example being the rerere database,\nbut I wouldn't be surprised if somebody has a notes tree to record\npatch ids for existing commits to speed up \"git cherry\" (hence \"git\nrebase\"). Also kup tool kernel.org folks use to optimize the\nuploading of inter-release diff relies on having a stable output\nfrom \"git diff A..B\", so that the uploader can run the command to\nproduce a diff locally, GPG sign it, and upload only the signature\nand have the k.org side produce the same diff between two tags,\nwithout having to upload the huge patchfile over the wire.\n\nIOW, \"syntactically identical so it is OK\" is not the right thought\nprocess. \"It may change the shape of the patch, which is a potential\nproblem for various tools, but as long as users understand that, it\nshould be allowed.\" is OK.\n\nCached patch-id database for \"git cherry\" would be a local and does\nnot affect the correctness, so I would consider breaking it is fine.\n\nAbout kup use case, the potential problem can be worked around as\nlong as the receiving end keeps the setting vanilla (and I do not\nsee any reason it wouldn't) and the uploader remembers to choose the\nmatching variant when he locally generates the patch, so I think it\nwould be also OK.\n\nUnnecessarily invalidating rerere database may be more frustrating,\nbut that again is local and personal, so the end user may suffer\nworse than cached patch-id use case, but that hopefully is just one\ntime pain.\n\nMy preference however is to limit this to Porcelains only, though.\n"},{"id":"186306","messageId":"20120307114714.GA14990@sigill.intra.peff.net","threadId":"29856","inReplyTo":"7vlindp17d.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] config: Introduce --patience config variable","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-03-07T11:47:15Z","receivedAt":"2012-03-07T11:47:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 06, 2012 at 06:57:42PM -0800, Junio C Hamano wrote:\n\n> > The idea of turning on patience diff via config makes sense to me, and\n> > it is not a problem for plumbing tools to have patience diff on, since\n> > patience diffs are syntactically identical to non-patience diffs.\n> \n> Even though I do not strongly object to the general conclusion, I\n> have to point out that the last line above is a dangerous thought.\n> \n> If you change the default diff algorithm, you will invalidate long\n> term caches that computed their keys based on the shape of patches\n> produced in the past.\n\nI see your point, though I don't think I'd use the word \"dangerous\" to\ndescribe the invalidation of a cache. In a true cache, we must be ready\nfor cache misses, so the \"danger\" is causing extra cache misses. That\nmight be non-optimal, depending on the cost of a miss, but it's not\ngoing to result in data loss.\n\n[This email ended up long; the \"tl;dr\" is: \"I don't think it's that big a\n deal, but I don't really have a problem limiting this feature to\n just porcelain\". Michal, the way to do that would be to move the config\n parsing into git_diff_ui_config instead of git_diff_basic_config.]\n\n> The prime example being the rerere database,\n\nHaving said that about caches, I don't know that I'd call rerere a\ncache. The \"miss\" means the user has to resolve the merge. So whether it\nis a cache or a database of precious information depends on how much you\nvalue the user's input.\n\nAnyway, would this option actually affect rerere? AFAICT, rerere does\nnot care about diff generation, but rather about the merge parameters.\nAnd it is smart enough to tell the merge subsystem to use standard\nparameters when generating rr-cache entries (e.g., setting\nmerge.conflictstyle does not affect what is put in the rr-cache).\n\n> but I wouldn't be surprised if somebody has a notes tree to record\n> patch ids for existing commits to speed up \"git cherry\" (hence \"git\n> rebase\").\n\nI do that[1]. However, any cache of patch-ids that does not record\nthe diff parameters used is wrong[2]. You're unlikely to come up with a\nfalse positive due to the length of the patch-id hash, and a few cache\nmisses here and there are fine. A cache that invalidates all entries\nwhen its cache parameters change[3] would be rendered useless by two\ncommands flip-flopping between two different sets of diff parameters.\nBut that is a quality-of-implementation issue for the cache.\n\n[1] Actually, I don't store it in notes, but rather in the mmap'd\n persistent key value store I developed for the generation number\n cache last summer (which we ended up not using).\n\n[2] Mine doesn't actually do this, and is wrong. Which is part of the\n reason I haven't sent it upstream.\n\n[3] The notes_cache code invalidates like this. Since the only user is\n the textconv cache, that strategy works well. For the generation\n number cache, it maintains multiple cache entries.\n\n> Also kup tool kernel.org folks use to optimize the uploading of\n> inter-release diff relies on having a stable output from \"git diff\n> A..B\", so that the uploader can run the command to produce a diff\n> locally, GPG sign it, and upload only the signature and have the k.org\n> side produce the same diff between two tags, without having to upload\n> the huge patchfile over the wire.\n\nThat's a cute trick, but I think it is going to be inherently flaky.\nDoes kup run the \"git diff\" itself (or, I would hope \"git diff-tree\")?\nIf not, then they are at the mercy of the uploader adding custom diff\nparameters. I don't know about others, but I sometimes tweak the amount\nof context in a diff I send if it makes the diff more readable (e.g., if\ntwo hunks are close by and are more easily read as a single hunk, or if\nrelevant context is one or two lines outside of a hunk).\n\nBut even if the kup client runs a sanitized, well-known, plumbing\nversion of the diff on both sides, they're still not guaranteed to\nproduce the same patch. Different versions of git may produce different\nresults. For example, we stopped defaulting to XDF_NEED_MINIMAL in\n582aa00. Git v1.7.1 produces a different diff for some commits than\nv1.7.1.1.\n\n> IOW, \"syntactically identical so it is OK\" is not the right thought\n> process. \"It may change the shape of the patch, which is a potential\n> problem for various tools, but as long as users understand that, it\n> should be allowed.\" is OK.\n\nI would argue that it is those tools which should be the ones to care.\nOne of the great things about git is that it uses simple, standard\nformats like unified diff, and doesn't care if you used git to generate\nthem. But the actual algorithm used to generate the diff is outside the\nscope of git-apply. So in theory the email you are grabbing with \"git\nam\" could be generated by GNU diff, or even some weird proprietary diff,\nand we don't care. Applying a patch-id or anything else that makes\nassumptions about the diff algorithm is inherently risky. For internal\ncases where one tool is generating both sides of the diff, it should be\nsure to specify vanilla parameters.\n\n> My preference however is to limit this to Porcelains only, though.\n\nI'd be OK with that. The \"...it should be sure to specify vanilla\nparameters\" above can be spelled as \"should use the plumbing version\".\nThough even that is not entirely foolproof (due to differing versions,\nas I mentioned above, but also I think we respect things like\n$GIT_DIFF_OPTS), it works OK in practice. And given that the main\nmotivation is for porcelain users, I don't think it is hurting anyone to\nbe conservative at first.\n\nIf \"kup\" relies on the user to run the diff themselves for the feature\nyou mentioned, then it may cause breakage there (because most people\nwill run the porcelain \"git diff\"). But I think it is already broken by\nthings like diff.mnemonicprefix, as well as break- and rename-detection\nparameters.\n\n-Peff\n"},{"id":"186307","messageId":"877gywy40l.fsf@thomas.inf.ethz.ch","threadId":"29856","inReplyTo":"m31up5tnjw.fsf@localhost.localdomain","subject":"Re: [PATCH 1/2] perf: compare diff algorithms","fromName":"Thomas Rast","fromEmail":"trast@inf.ethz.ch","sentAt":"2012-03-07T12:44:26Z","receivedAt":"2012-03-07T12:44:26Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Jakub Narebski writes:\n\n> Junio C Hamano writes:\n>> \n>> But of course, this from a command line would not work:\n>> \n>> \t$ perl -MGit\n>> \n>> I do not expect it to, and for the ease of testing new versions, I\n>> prefer it not to work.\n>> \n>> In any case, you should be able to do anything under t/ _before_\n>> installing, so relying on having Git.pm in normal @INC is a double\n>> no-no.\n>\n> Thomas, take a look at how it is solved in 't/t9700/test.pl', used by\n> 't/t9700-perl-git.sh':\n>\n> use lib (split(/:/, $ENV{GITPERLLIB}));\n\nHum. The problem is that the user may invoke aggregate.perl manually,\nand GITPERLLIB won't be set in that case.\n\nIs there a better solution than duplicating the logic that sets\nGITPERLLIB in test-lib.sh within aggregate.perl?\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"186322","messageId":"7vobs8nx31.fsf@alter.siamese.dyndns.org","threadId":"29856","inReplyTo":"20120307114714.GA14990@sigill.intra.peff.net","subject":"Re: [PATCH] config: Introduce --patience config variable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-07T17:24:18Z","receivedAt":"2012-03-07T17:24:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Tue, Mar 06, 2012 at 06:57:42PM -0800, Junio C Hamano wrote:\n>\n>> > The idea of turning on patience diff via config makes sense to me, and\n>> > it is not a problem for plumbing tools to have patience diff on, since\n>> > patience diffs are syntactically identical to non-patience diffs.\n>> \n>> Even though I do not strongly object to the general conclusion, I\n>> have to point out that the last line above is a dangerous thought.\n>>\n>> If you change the default diff algorithm, you will invalidate long\n>> term caches that computed their keys based on the shape of patches\n>> produced in the past.\n>\n> I see your point, though I don't think I'd use the word \"dangerous\" to\n> describe the invalidation of a cache.\n\nI probably was not writing clearly enough to avoid getting misread.\nThe \"dangerous\" does not refer to \"invalidation of a cache\". What I\nmeant was that \"The output is syntactically identical, so it is OK\"\nis a dangerous way to think when assessing the risk of regression,\nbecause applying to a given preimage and producing the same\npostimage is not the *only* way the output is used.\n\nI think the executive summary is that we are in agreement; your\nanalysis of potential regression coming from differences of the\nshape of the patch output (not applicability to a given preimage to\nproduce the same postimage) seems to match what I said in the\nprevious message.\n\nEspecially in the kup case, it is a minority tool used by people who\nknows or can be easily taught in a tightly controlled environment,\nand it is fine as long as the users have a way to make sure two\ndiffs run on both ends of the communication produce the same result\n(in an earlier discussion on k.org users list where kup was first\ndiscussed, the limitation of users having to use the same version as\nk.org has was noted and the users are already aware of it).\n"},{"id":"186323","messageId":"7vk42wnw40.fsf@alter.siamese.dyndns.org","threadId":"29856","inReplyTo":"877gywy40l.fsf@thomas.inf.ethz.ch","subject":"Re: [PATCH 1/2] perf: compare diff algorithms","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-07T17:45:19Z","receivedAt":"2012-03-07T17:45:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Rast writes:\n\n> Jakub Narebski writes:\n>\n>> Thomas, take a look at how it is solved in 't/t9700/test.pl', used by\n>> 't/t9700-perl-git.sh':\n>>\n>> use lib (split(/:/, $ENV{GITPERLLIB}));\n>\n> Hum. The problem is that the user may invoke aggregate.perl manually,\n> and GITPERLLIB won't be set in that case.\n\nI would equate \"manual invocation\" with \"the user knows what he is doing\",\nso if that is the only problem, I think we are good.\n\n> Is there a better solution than duplicating the logic that sets\n> GITPERLLIB in test-lib.sh within aggregate.perl?\n\nPerhaps the part to figure out the directory layout can and should\nbe split out of test-lib.sh into test-env.sh or something and be\ndot-sourced at the beginning of test-lib.sh; would that help?\n\n t/test-env.sh | 112 ++++++++++++++++++++++++++++++++++++++++++++++++++++++\n t/test-lib.sh | 118 ++++-----------------------------------------------------\n 2 files changed, 120 insertions(+), 110 deletions(-)\n\ndiff --git a/t/test-env.sh b/t/test-env.sh\nnew file mode 100644\nindex 0000000..1159c5a\n--- /dev/null\n+++ b/t/test-env.sh\n@@ -0,0 +1,112 @@\n+# figure out the environment a test-related script is being run\n+# taken from test-lib.sh, Copyright (c) 2005 Junio C Hamano\n+\n+# The directory to find other helper pieces e.g. lib-gpg.sh of the\n+# test suite; usually t/ in the git source tree.\n+if test -z \"$TEST_DIRECTORY\"\n+then\n+\t# We allow tests to override this, in case they want to run tests\n+\t# outside of t/, e.g. for running tests on the test library\n+\t# itself.\n+\tTEST_DIRECTORY=$(pwd)\n+fi\n+GIT_BUILD_DIR=\"$TEST_DIRECTORY\"/..\n+GIT_TEMPLATE_DIR=\"$GIT_BUILD_DIR\"/templates/blt\n+GITPERLLIB=\"$GIT_BUILD_DIR\"/perl/blib/lib:\"$GIT_BUILD_DIR\"/perl/blib/arch/auto/Git\n+\n+if test -n \"$valgrind\"\n+then\n+\tmake_symlink () {\n+\t\ttest -h \"$2\" &&\n+\t\ttest \"$1\" = \"$(readlink \"$2\")\" || {\n+\t\t\t# be super paranoid\n+\t\t\tif mkdir \"$2\".lock\n+\t\t\tthen\n+\t\t\t\trm -f \"$2\" &&\n+\t\t\t\tln -s \"$1\" \"$2\" &&\n+\t\t\t\trm -r \"$2\".lock\n+\t\t\telse\n+\t\t\t\twhile test -d \"$2\".lock\n+\t\t\t\tdo\n+\t\t\t\t\tsay \"Waiting for lock on $2.\"\n+\t\t\t\t\tsleep 1\n+\t\t\t\tdone\n+\t\t\tfi\n+\t\t}\n+\t}\n+\n+\tmake_valgrind_symlink () {\n+\t\t# handle only executables, unless they are shell libraries that\n+\t\t# need to be in the exec-path. We will just use \"#!\" as a\n+\t\t# guess for a shell-script, since we have no idea what the user\n+\t\t# may have configured as the shell path.\n+\t\ttest -x \"$1\" ||\n+\t\ttest \"#!\" = \"$(head -c 2 <\"$1\")\" ||\n+\t\treturn;\n+\n+\t\tbase=$(basename \"$1\")\n+\t\tsymlink_target=$GIT_BUILD_DIR/$base\n+\t\t# do not override scripts\n+\t\tif test -x \"$symlink_target\" &&\n+\t\t test ! -d \"$symlink_target\" &&\n+\t\t test \"#!\" != \"$(head -c 2 < \"$symlink_target\")\"\n+\t\tthen\n+\t\t\tsymlink_target=../valgrind.sh\n+\t\tfi\n+\t\tcase \"$base\" in\n+\t\t*.sh|*.perl)\n+\t\t\tsymlink_target=../unprocessed-script\n+\t\tesac\n+\t\t# create the link, or replace it if it is out of date\n+\t\tmake_symlink \"$symlink_target\" \"$GIT_VALGRIND/bin/$base\" || exit\n+\t}\n+\n+\t# override all git executables in TEST_DIRECTORY/..\n+\tGIT_VALGRIND=$TEST_DIRECTORY/valgrind\n+\tmkdir -p \"$GIT_VALGRIND\"/bin\n+\tfor file in $GIT_BUILD_DIR/git* $GIT_BUILD_DIR/test-*\n+\tdo\n+\t\tmake_valgrind_symlink $file\n+\tdone\n+\t# special-case the mergetools loadables\n+\tmake_symlink \"$GIT_BUILD_DIR\"/mergetools \"$GIT_VALGRIND/bin/mergetools\"\n+\tOLDIFS=$IFS\n+\tIFS=:\n+\tfor path in $PATH\n+\tdo\n+\t\tls \"$path\"/git-* 2> /dev/null |\n+\t\twhile read file\n+\t\tdo\n+\t\t\tmake_valgrind_symlink \"$file\"\n+\t\tdone\n+\tdone\n+\tIFS=$OLDIFS\n+\tPATH=$GIT_VALGRIND/bin:$PATH\n+\tGIT_EXEC_PATH=$GIT_VALGRIND/bin\n+\texport GIT_VALGRIND\n+elif test -n \"$GIT_TEST_INSTALLED\" ; then\n+\tGIT_EXEC_PATH=$($GIT_TEST_INSTALLED/git --exec-path) ||\n+\terror \"Cannot run git from $GIT_TEST_INSTALLED.\"\n+\tPATH=$GIT_TEST_INSTALLED:$GIT_BUILD_DIR:$PATH\n+\tGIT_EXEC_PATH=${GIT_TEST_EXEC_PATH:-$GIT_EXEC_PATH}\n+else # normal case, use ../bin-wrappers only unless $with_dashes:\n+\tgit_bin_dir=\"$GIT_BUILD_DIR/bin-wrappers\"\n+\tif ! test -x \"$git_bin_dir/git\" ; then\n+\t\tif test -z \"$with_dashes\" ; then\n+\t\t\tsay \"$git_bin_dir/git is not executable; using GIT_EXEC_PATH\"\n+\t\tfi\n+\t\twith_dashes=t\n+\tfi\n+\tPATH=\"$git_bin_dir:$PATH\"\n+\tGIT_EXEC_PATH=$GIT_BUILD_DIR\n+\tif test -n \"$with_dashes\" ; then\n+\t\tPATH=\"$GIT_BUILD_DIR:$PATH\"\n+\tfi\n+fi\n+GIT_TEMPLATE_DIR=\"$GIT_BUILD_DIR\"/templates/blt\n+\n+. \"$GIT_BUILD_DIR\"/GIT-BUILD-OPTIONS\n+\n+GITPERLLIB=\"$GIT_BUILD_DIR\"/perl/blib/lib:\"$GIT_BUILD_DIR\"/perl/blib/arch/auto/Git\n+\n+export PATH GIT_EXEC_PATH GIT_TEMPLATE_DIR GITPERLLIB\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 30ed4d7..e892368 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -31,6 +31,13 @@ done,*)\n \t;;\n esac\n \n+# Protect ourselves from common misconfiguration to export\n+# CDPATH into the environment\n+unset CDPATH\n+unset GREP_OPTIONS\n+\n+. ./test-env.sh\n+\n # Keep the original TERM for say_color\n ORIGINAL_TERM=$TERM\n \n@@ -72,12 +79,6 @@ export GIT_AUTHOR_EMAIL GIT_AUTHOR_NAME\n export GIT_COMMITTER_EMAIL GIT_COMMITTER_NAME\n export EDITOR\n \n-# Protect ourselves from common misconfiguration to export\n-# CDPATH into the environment\n-unset CDPATH\n-\n-unset GREP_OPTIONS\n-\n case $(echo $GIT_TRACE |tr \"[A-Z]\" \"[a-z]\") in\n \t1|2|true)\n \t\techo \"* warning: Some tests will not work if GIT_TRACE\" \\\n@@ -380,117 +381,17 @@ test_done () {\n \tesac\n }\n \n-# Test the binaries we have just built. The tests are kept in\n-# t/ subdirectory and are run in 'trash directory' subdirectory.\n-if test -z \"$TEST_DIRECTORY\"\n-then\n-\t# We allow tests to override this, in case they want to run tests\n-\t# outside of t/, e.g. for running tests on the test library\n-\t# itself.\n-\tTEST_DIRECTORY=$(pwd)\n-fi\n if test -z \"$TEST_OUTPUT_DIRECTORY\"\n then\n \t# Similarly, override this to store the test-results subdir\n \t# elsewhere\n \tTEST_OUTPUT_DIRECTORY=$TEST_DIRECTORY\n fi\n-GIT_BUILD_DIR=\"$TEST_DIRECTORY\"/..\n \n-if test -n \"$valgrind\"\n-then\n-\tmake_symlink () {\n-\t\ttest -h \"$2\" &&\n-\t\ttest \"$1\" = \"$(readlink \"$2\")\" || {\n-\t\t\t# be super paranoid\n-\t\t\tif mkdir \"$2\".lock\n-\t\t\tthen\n-\t\t\t\trm -f \"$2\" &&\n-\t\t\t\tln -s \"$1\" \"$2\" &&\n-\t\t\t\trm -r \"$2\".lock\n-\t\t\telse\n-\t\t\t\twhile test -d \"$2\".lock\n-\t\t\t\tdo\n-\t\t\t\t\tsay \"Waiting for lock on $2.\"\n-\t\t\t\t\tsleep 1\n-\t\t\t\tdone\n-\t\t\tfi\n-\t\t}\n-\t}\n-\n-\tmake_valgrind_symlink () {\n-\t\t# handle only executables, unless they are shell libraries that\n-\t\t# need to be in the exec-path. We will just use \"#!\" as a\n-\t\t# guess for a shell-script, since we have no idea what the user\n-\t\t# may have configured as the shell path.\n-\t\ttest -x \"$1\" ||\n-\t\ttest \"#!\" = \"$(head -c 2 <\"$1\")\" ||\n-\t\treturn;\n-\n-\t\tbase=$(basename \"$1\")\n-\t\tsymlink_target=$GIT_BUILD_DIR/$base\n-\t\t# do not override scripts\n-\t\tif test -x \"$symlink_target\" &&\n-\t\t test ! -d \"$symlink_target\" &&\n-\t\t test \"#!\" != \"$(head -c 2 < \"$symlink_target\")\"\n-\t\tthen\n-\t\t\tsymlink_target=../valgrind.sh\n-\t\tfi\n-\t\tcase \"$base\" in\n-\t\t*.sh|*.perl)\n-\t\t\tsymlink_target=../unprocessed-script\n-\t\tesac\n-\t\t# create the link, or replace it if it is out of date\n-\t\tmake_symlink \"$symlink_target\" \"$GIT_VALGRIND/bin/$base\" || exit\n-\t}\n-\n-\t# override all git executables in TEST_DIRECTORY/..\n-\tGIT_VALGRIND=$TEST_DIRECTORY/valgrind\n-\tmkdir -p \"$GIT_VALGRIND\"/bin\n-\tfor file in $GIT_BUILD_DIR/git* $GIT_BUILD_DIR/test-*\n-\tdo\n-\t\tmake_valgrind_symlink $file\n-\tdone\n-\t# special-case the mergetools loadables\n-\tmake_symlink \"$GIT_BUILD_DIR\"/mergetools \"$GIT_VALGRIND/bin/mergetools\"\n-\tOLDIFS=$IFS\n-\tIFS=:\n-\tfor path in $PATH\n-\tdo\n-\t\tls \"$path\"/git-* 2> /dev/null |\n-\t\twhile read file\n-\t\tdo\n-\t\t\tmake_valgrind_symlink \"$file\"\n-\t\tdone\n-\tdone\n-\tIFS=$OLDIFS\n-\tPATH=$GIT_VALGRIND/bin:$PATH\n-\tGIT_EXEC_PATH=$GIT_VALGRIND/bin\n-\texport GIT_VALGRIND\n-elif test -n \"$GIT_TEST_INSTALLED\" ; then\n-\tGIT_EXEC_PATH=$($GIT_TEST_INSTALLED/git --exec-path) ||\n-\terror \"Cannot run git from $GIT_TEST_INSTALLED.\"\n-\tPATH=$GIT_TEST_INSTALLED:$GIT_BUILD_DIR:$PATH\n-\tGIT_EXEC_PATH=${GIT_TEST_EXEC_PATH:-$GIT_EXEC_PATH}\n-else # normal case, use ../bin-wrappers only unless $with_dashes:\n-\tgit_bin_dir=\"$GIT_BUILD_DIR/bin-wrappers\"\n-\tif ! test -x \"$git_bin_dir/git\" ; then\n-\t\tif test -z \"$with_dashes\" ; then\n-\t\t\tsay \"$git_bin_dir/git is not executable; using GIT_EXEC_PATH\"\n-\t\tfi\n-\t\twith_dashes=t\n-\tfi\n-\tPATH=\"$git_bin_dir:$PATH\"\n-\tGIT_EXEC_PATH=$GIT_BUILD_DIR\n-\tif test -n \"$with_dashes\" ; then\n-\t\tPATH=\"$GIT_BUILD_DIR:$PATH\"\n-\tfi\n-fi\n-GIT_TEMPLATE_DIR=\"$GIT_BUILD_DIR\"/templates/blt\n unset GIT_CONFIG\n GIT_CONFIG_NOSYSTEM=1\n GIT_ATTR_NOSYSTEM=1\n-export PATH GIT_EXEC_PATH GIT_TEMPLATE_DIR GIT_CONFIG_NOSYSTEM GIT_ATTR_NOSYSTEM\n+export GIT_CONFIG_NOSYSTEM GIT_ATTR_NOSYSTEM\n \n . \"$GIT_BUILD_DIR\"/GIT-BUILD-OPTIONS\n \n@@ -503,9 +404,6 @@ then\n \t\tGIT_TEST_CMP=\"$DIFF -u\"\n \tfi\n fi\n-\n-GITPERLLIB=\"$GIT_BUILD_DIR\"/perl/blib/lib:\"$GIT_BUILD_DIR\"/perl/blib/arch/auto/Git\n-export GITPERLLIB\n test -d \"$GIT_BUILD_DIR\"/templates/blt || {\n \terror \"You haven't built things yet, have you?\"\n }\n"},{"id":"186325","messageId":"201203071903.02373.jnareb@gmail.com","threadId":"29856","inReplyTo":"877gywy40l.fsf@thomas.inf.ethz.ch","subject":"Re: [PATCH 1/2] perf: compare diff algorithms","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2012-03-07T18:03:01Z","receivedAt":"2012-03-07T18:03:01Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Thomas Rast wrote:\n> Jakub Narebski writes:\n> > Junio C Hamano writes:\n> >> \n> >> But of course, this from a command line would not work:\n> >> \n> >> \t$ perl -MGit\n> >> \n> >> I do not expect it to, and for the ease of testing new versions, I\n> >> prefer it not to work.\n> >> \n> >> In any case, you should be able to do anything under t/ _before_\n> >> installing, so relying on having Git.pm in normal @INC is a double\n> >> no-no.\n> >\n> > Thomas, take a look at how it is solved in 't/t9700/test.pl', used by\n> > 't/t9700-perl-git.sh':\n> >\n> > use lib (split(/:/, $ENV{GITPERLLIB}));\n> \n> Hum. The problem is that the user may invoke aggregate.perl manually,\n> and GITPERLLIB won't be set in that case.\n> \n> Is there a better solution than duplicating the logic that sets\n> GITPERLLIB in test-lib.sh within aggregate.perl?\n\nBeside extracting logic that sets GITPERLLIB into separate file like\nin Junio proposal? You can always assume that it is fixed relative\nto perl/Git.pm, and use __DIR__ or $FindBin to make \"use lib\", e.g.\n\n use FindBin;\n use lib \"$FindBin::Bin/../../perl\";\n\n-- \nJakub Narebski\nPoland\n"},{"id":"186326","messageId":"7vzkbsmfyk.fsf@alter.siamese.dyndns.org","threadId":"29856","inReplyTo":"201203071903.02373.jnareb@gmail.com","subject":"Re: [PATCH 1/2] perf: compare diff algorithms","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-07T18:19:31Z","receivedAt":"2012-03-07T18:19:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narebski writes:\n\n> Beside extracting logic that sets GITPERLLIB into separate file like\n> in Junio proposal?\n\nThat was not even a proposal. The part that deals with valgrind is\nblatantly wrong (it shouldn't create symlinks in that code, it only\nshould figure out where things should be).\n\n> You can always assume that it is fixed relative\n> to perl/Git.pm, and use __DIR__ or $FindBin to make \"use lib\", e.g.\n>\n> use FindBin;\n> use lib \"$FindBin::Bin/../../perl\";\n\nUse of FindBin to find the location of the script is OK, but does\nusing \"../../perl\" really work?\n\nWe have this\n\n GITPERLLIB=\"$GIT_BUILD_DIR\"/perl/blib/lib:\"$GIT_BUILD_DIR\"/perl/blib/arch/auto/Git\n\nto look for two places in test-lib.sh\n"},{"id":"186615","messageId":"4F5AFF33.7090900@lsrfire.ath.cx","threadId":"29856","inReplyTo":"f113867bcf2fec3210cd1a997e1398903b3bdd76.1331039505.git.trast@student.ethz.ch","subject":"Re: [PATCH 1/2] perf: compare diff algorithms","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2012-03-10T07:13:55Z","receivedAt":"2012-03-10T07:13:55Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 06.03.2012 14:15, schrieb Thomas Rast:\n> 8c912ee (teach --histogram to diff, 2011-07-12) claimed histogram diff\n> was faster than both Myers and patience.\n>\n> We have since incorporated a performance testing framework, so add a\n> test that compares the various diff tasks performed in a real 'log -p'\n> workload. This does indeed show that histogram diff slightly beats\n> Myers, while patience is much slower than the others.\n>\n> Signed-off-by: Thomas Rast\n> ---\n>\n> The 3000 is pretty arbitrary but makes for a nice test duration.\n>\n> I'm reluctant to put numbers into the message, since the whole point\n> of the perf test framework is that you can easily get them too. But\n> here's what I'm seeing:\n>\n> 4000.1: log -3000 (baseline) 0.04(0.02+0.01)\n> 4000.2: log --raw -3000 (tree-only) 0.49(0.38+0.09)\n> 4000.3: log -p -3000 (Myers) 1.93(1.75+0.17)\n> 4000.4: log -p -3000 --histogram 1.90(1.74+0.15)\n> 4000.5: log -p -3000 --patience 2.25(2.07+0.16)\n\nJust a data point: --histogram is slightly slower for me:\n\n Test this tree\n -----------------------------------------------------\n 4000.1: log -3000 (baseline) 0.07(0.07+0.00)\n 4000.2: log --raw -3000 (tree-only) 0.35(0.31+0.04)\n 4000.3: log -p -3000 (Myers) 1.50(1.40+0.08)\n 4000.4: log -p -3000 --histogram 1.54(1.48+0.05)\n 4000.5: log -p -3000 --patience 1.79(1.71+0.06)\n\n(baseline with -3000)\n\nRené\n"}]}