* [PATCH 0/6 v5] allow "-" as a shorthand for "previous branch"
From: Siddharth Kannan @ 2017-02-25 7:24 UTC (permalink / raw)
To: git
Cc: gitster, Matthieu.Moy, pranit.bauva, peff, pclouds, sandals,
Siddharth Kannan
An updated version of the patch [1]. Discussion here[1] has been taken into
account. The test for "-@{yesterday}" is there inside the log-shorthand test,
it is commented out for now.
I have removed the redundant pieces of code in merge.c and revert.c as mentioned
by Matthieu in [2]. As analysed by Junio[3], the same type of code inside
checkout.c and worktree.c can not be removed because the appropriate functions
inside revision.c are not called in their codepaths.
Thanks for your review of the previous versions, Junio and Matthieu!
[1]: 1487258054-32292-1-git-send-email-kannan.siddharth12@gmail.com
[2]: vpqbmu768on.fsf@anie.imag.fr
[3]: xmqq1sv1euob.fsf@gitster.mtv.corp.google.com
Siddharth Kannan (6):
revision.c: do not update argv with unknown option
revision.c: swap if/else blocks
revision.c: args starting with "-" might be a revision
sha1_name.c: teach get_sha1_1 "-" shorthand for "@{-1}"
merge.c: delegate handling of "-" shorthand to revision.c:get_sha1
revert.c: delegate handling of "-" shorthand to setup_revisions
builtin/merge.c | 2 -
builtin/revert.c | 2 -
revision.c | 15 +++---
sha1_name.c | 5 ++
t/t3035-merge-hyphen-shorthand.sh | 33 ++++++++++++
t/t3514-revert-shorthand.sh | 25 +++++++++
t/t4214-log-shorthand.sh | 106 ++++++++++++++++++++++++++++++++++++++
7 files changed, 178 insertions(+), 10 deletions(-)
create mode 100755 t/t3035-merge-hyphen-shorthand.sh
create mode 100755 t/t3514-revert-shorthand.sh
create mode 100755 t/t4214-log-shorthand.sh
--
2.1.4
^ permalink raw reply
* [PATCH 3/6 v5] revision.c: args starting with "-" might be a revision
From: Siddharth Kannan @ 2017-02-25 7:24 UTC (permalink / raw)
To: git
Cc: gitster, Matthieu.Moy, pranit.bauva, peff, pclouds, sandals,
Siddharth Kannan
In-Reply-To: <1488007487-12965-1-git-send-email-kannan.siddharth12@gmail.com>
setup_revisions used to consider any argument starting with "-" to be either a
valid or an unknown option.
Teach setup_revisions to check if an argument is a revision before adding it as
an unknown option (something that setup_revisions didn't understand) to argv,
and moving on to the next argument.
This patch prepares the addition of "-" as a shorthand for "previous branch".
Signed-off-by: Siddharth Kannan <kannan.siddharth12@gmail.com>
---
revision.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/revision.c b/revision.c
index 8d4ddae..5470c33 100644
--- a/revision.c
+++ b/revision.c
@@ -2203,6 +2203,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
read_from_stdin = 0;
for (left = i = 1; i < argc; i++) {
const char *arg = argv[i];
+ int maybe_opt = 0;
if (*arg == '-') {
int opts;
@@ -2232,15 +2233,17 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
}
if (opts < 0)
exit(128);
- /* arg is an unknown option */
- argv[left++] = arg;
- continue;
+ maybe_opt = 1;
}
if (!handle_revision_arg(arg, revs, flags, revarg_opt))
got_rev_arg = 1;
- else {
+ else if (maybe_opt) {
+ /* arg is an unknown option */
+ argv[left++] = arg;
+ continue;
+ } else {
int j;
if (seen_dashdash || *arg == '^')
die("bad revision '%s'", arg);
--
2.1.4
^ permalink raw reply related
* [PATCH 6/6 v5] revert.c: delegate handling of "-" shorthand to setup_revisions
From: Siddharth Kannan @ 2017-02-25 7:24 UTC (permalink / raw)
To: git
Cc: gitster, Matthieu.Moy, pranit.bauva, peff, pclouds, sandals,
Siddharth Kannan
In-Reply-To: <1488007487-12965-1-git-send-email-kannan.siddharth12@gmail.com>
revert.c:run_sequencer calls setup_revisions right after replacing "-" with
"@{-1}" for this shorthand. A previous patch taught setup_revisions to handle
this shorthand by doing the required replacement inside revision.c:get_sha1_1.
Hence, the code here is redundant and has been removed.
This patch also adds a test to check that revert recognizes the "-" shorthand.
Signed-off-by: Siddharth Kannan <kannan.siddharth12@gmail.com>
---
builtin/revert.c | 2 --
t/t3514-revert-shorthand.sh | 25 +++++++++++++++++++++++++
2 files changed, 25 insertions(+), 2 deletions(-)
create mode 100755 t/t3514-revert-shorthand.sh
diff --git a/builtin/revert.c b/builtin/revert.c
index 4ca5b51..0bc6657 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -155,8 +155,6 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)
opts->revs->no_walk = REVISION_WALK_NO_WALK_UNSORTED;
if (argc < 2)
usage_with_options(usage_str, options);
- if (!strcmp(argv[1], "-"))
- argv[1] = "@{-1}";
memset(&s_r_opt, 0, sizeof(s_r_opt));
s_r_opt.assume_dashdash = 1;
argc = setup_revisions(argc, argv, opts->revs, &s_r_opt);
diff --git a/t/t3514-revert-shorthand.sh b/t/t3514-revert-shorthand.sh
new file mode 100755
index 0000000..51f8c81d
--- /dev/null
+++ b/t/t3514-revert-shorthand.sh
@@ -0,0 +1,25 @@
+#!/bin/sh
+
+test_description='log can show previous branch using shorthand - for @{-1}'
+
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+ test_commit first
+'
+
+test_expect_success 'setup branches' '
+ echo "hello" >hello &&
+ cat hello >expect &&
+ git add hello &&
+ git commit -m "hello first commit" &&
+ echo "world" >>hello &&
+ git commit -am "hello second commit" &&
+ git checkout -b testing-1 &&
+ git checkout master &&
+ git revert --no-edit - &&
+ cat hello >actual &&
+ test_cmp expect actual
+'
+
+test_done
--
2.1.4
^ permalink raw reply related
* [PATCH 5/6 v5] merge.c: delegate handling of "-" shorthand to revision.c:get_sha1
From: Siddharth Kannan @ 2017-02-25 7:24 UTC (permalink / raw)
To: git
Cc: gitster, Matthieu.Moy, pranit.bauva, peff, pclouds, sandals,
Siddharth Kannan
In-Reply-To: <1488007487-12965-1-git-send-email-kannan.siddharth12@gmail.com>
The callchain for handling each argument contains the function
revision.c:get_sha1 where the shorthand for "-" ~ "@{-1}" has already been
implemented in a previous patch; the complete callchain leading to that
function is:
1. merge.c:collect_parents
2. commit.c:get_merge_parent : this function calls revision.c:get_sha1
This patch also adds a test for checking that the shorthand works properly
Signed-off-by: Siddharth Kannan <kannan.siddharth12@gmail.com>
---
builtin/merge.c | 2 --
t/t3035-merge-hyphen-shorthand.sh | 33 +++++++++++++++++++++++++++++++++
2 files changed, 33 insertions(+), 2 deletions(-)
create mode 100755 t/t3035-merge-hyphen-shorthand.sh
diff --git a/builtin/merge.c b/builtin/merge.c
index a96d4fb..36ff420 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1228,8 +1228,6 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
argc = setup_with_upstream(&argv);
else
die(_("No commit specified and merge.defaultToUpstream not set."));
- } else if (argc == 1 && !strcmp(argv[0], "-")) {
- argv[0] = "@{-1}";
}
if (!argc)
diff --git a/t/t3035-merge-hyphen-shorthand.sh b/t/t3035-merge-hyphen-shorthand.sh
new file mode 100755
index 0000000..fd37ff9
--- /dev/null
+++ b/t/t3035-merge-hyphen-shorthand.sh
@@ -0,0 +1,33 @@
+#!/bin/sh
+
+test_description='merge uses the shorthand - for @{-1}'
+
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+ test_commit first &&
+ test_commit second &&
+ test_commit third &&
+ test_commit fourth &&
+ test_commit fifth &&
+ test_commit sixth &&
+ test_commit seventh
+'
+
+test_expect_success 'setup branches' '
+ git checkout master &&
+ git checkout -b testing-2 &&
+ git checkout -b testing-1 &&
+ test_commit eigth &&
+ test_commit ninth
+'
+
+test_expect_success 'merge - should work' '
+ git checkout testing-2 &&
+ git merge - &&
+ git rev-parse HEAD HEAD^^ | sort >actual &&
+ git rev-parse master testing-1 | sort >expect &&
+ test_cmp expect actual
+'
+
+test_done
--
2.1.4
^ permalink raw reply related
* [PATCH 4/6 v5] sha1_name.c: teach get_sha1_1 "-" shorthand for "@{-1}"
From: Siddharth Kannan @ 2017-02-25 7:24 UTC (permalink / raw)
To: git
Cc: gitster, Matthieu.Moy, pranit.bauva, peff, pclouds, sandals,
Siddharth Kannan
In-Reply-To: <1488007487-12965-1-git-send-email-kannan.siddharth12@gmail.com>
This patch introduces "-" as a method to refer to a revision, and adds tests to
test that git-log works with this shorthand.
This change will touch the following commands (through
revision.c:setup_revisions):
* builtin/blame.c
* builtin/diff.c
* builtin/diff-files.c
* builtin/diff-index.c
* builtin/diff-tree.c
* builtin/log.c
* builtin/rev-list.c
* builtin/shortlog.c
* builtin/fast-export.c
* builtin/fmt-merge-msg.c
builtin/add.c
builtin/checkout.c
builtin/commit.c
builtin/merge.c
builtin/pack-objects.c
builtin/revert.c
* marked commands are information-only.
As most commands in this list are not of the rm-variety, (i.e a command that
would delete something), this change does not make it easier for people to
delete. (eg: "git branch -d -" is *not* enabled by this patch)
Suffixes like "-@{yesterday}" and "-@{2.days.ago}" are not enabled by this
patch. This is something that needs to be fixed later by making changes deeper
down the callchain.
Signed-off-by: Siddharth Kannan <kannan.siddharth12@gmail.com>
---
sha1_name.c | 5 +++
t/t4214-log-shorthand.sh | 106 +++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 111 insertions(+)
create mode 100755 t/t4214-log-shorthand.sh
diff --git a/sha1_name.c b/sha1_name.c
index 73a915f..2f86bc9 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -947,6 +947,11 @@ static int get_sha1_1(const char *name, int len, unsigned char *sha1, unsigned l
if (!ret)
return 0;
+ if (*name == '-' && len == 1) {
+ name = "@{-1}";
+ len = 5;
+ }
+
ret = get_sha1_basic(name, len, sha1, lookup_flags);
if (!ret)
return 0;
diff --git a/t/t4214-log-shorthand.sh b/t/t4214-log-shorthand.sh
new file mode 100755
index 0000000..8be2de1
--- /dev/null
+++ b/t/t4214-log-shorthand.sh
@@ -0,0 +1,106 @@
+#!/bin/sh
+
+test_description='log can show previous branch using shorthand - for @{-1}'
+
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+ test_commit first &&
+ test_commit second &&
+ test_commit third &&
+ test_commit fourth &&
+ test_commit fifth &&
+ test_commit sixth &&
+ test_commit seventh
+'
+
+test_expect_success '"log -" should not work initially' '
+ test_must_fail git log -
+'
+
+test_expect_success 'setup branches for testing' '
+ git checkout -b testing-1 master^ &&
+ git checkout -b testing-2 master~2 &&
+ git checkout master
+'
+
+test_expect_success '"log -" should work' '
+ git log testing-2 >expect &&
+ git log - >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'symmetric revision range should work when one end is left empty' '
+ git checkout testing-2 &&
+ git checkout master &&
+ git log ...@{-1} >expect.first_empty &&
+ git log @{-1}... >expect.last_empty &&
+ git log ...- >actual.first_empty &&
+ git log -... >actual.last_empty &&
+ test_cmp expect.first_empty actual.first_empty &&
+ test_cmp expect.last_empty actual.last_empty
+'
+
+test_expect_success 'asymmetric revision range should work when one end is left empty' '
+ git checkout testing-2 &&
+ git checkout master &&
+ git log ..@{-1} >expect.first_empty &&
+ git log @{-1}.. >expect.last_empty &&
+ git log ..- >actual.first_empty &&
+ git log -.. >actual.last_empty &&
+ test_cmp expect.first_empty actual.first_empty &&
+ test_cmp expect.last_empty actual.last_empty
+'
+
+test_expect_success 'symmetric revision range should work when both ends are given' '
+ git checkout testing-2 &&
+ git checkout master &&
+ git log -...testing-1 >expect &&
+ git log testing-2...testing-1 >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'asymmetric revision range should work when both ends are given' '
+ git checkout testing-2 &&
+ git checkout master &&
+ git log -..testing-1 >expect &&
+ git log testing-2..testing-1 >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'multiple separate arguments should be handled properly' '
+ git checkout testing-2 &&
+ git checkout master &&
+ git log - - >expect.1 &&
+ git log @{-1} @{-1} >actual.1 &&
+ git log - HEAD >expect.2 &&
+ git log @{-1} HEAD >actual.2 &&
+ test_cmp expect.1 actual.1 &&
+ test_cmp expect.2 actual.2
+'
+
+test_expect_success 'revision ranges with same start and end should be empty' '
+ git checkout testing-2 &&
+ git checkout master &&
+ test 0 -eq $(git log -...- | wc -l) &&
+ test 0 -eq $(git log -..- | wc -l)
+'
+
+test_expect_success 'suffixes to - should work' '
+ git checkout testing-2 &&
+ git checkout master &&
+ git log -~ >expect.1 &&
+ git log @{-1}~ >actual.1 &&
+ git log -~2 >expect.2 &&
+ git log @{-1}~2 >actual.2 &&
+ git log -^ >expect.3 &&
+ git log @{-1}^ >actual.3 &&
+ # git log -@{yesterday} >expect.4 &&
+ # git log @{-1}@{yesterday} >actual.4 &&
+ test_cmp expect.1 actual.1 &&
+ test_cmp expect.2 actual.2 &&
+ test_cmp expect.3 actual.3
+ # test_cmp expect.4 actual.4
+'
+
+test_done
--
2.1.4
^ permalink raw reply related
* [PATCH 2/6 v5] revision.c: swap if/else blocks
From: Siddharth Kannan @ 2017-02-25 7:24 UTC (permalink / raw)
To: git
Cc: gitster, Matthieu.Moy, pranit.bauva, peff, pclouds, sandals,
Siddharth Kannan
In-Reply-To: <1488007487-12965-1-git-send-email-kannan.siddharth12@gmail.com>
Swap the condition and bodies of an "if (A) do_A else do_B" in
setup_revisions() to "if (!A) do_B else do A", to make the change in
the the next step easier to read.
No behaviour change is intended in this step.
Signed-off-by: Siddharth Kannan <kannan.siddharth12@gmail.com>
---
revision.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/revision.c b/revision.c
index 5674a9a..8d4ddae 100644
--- a/revision.c
+++ b/revision.c
@@ -2238,7 +2238,9 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
}
- if (handle_revision_arg(arg, revs, flags, revarg_opt)) {
+ if (!handle_revision_arg(arg, revs, flags, revarg_opt))
+ got_rev_arg = 1;
+ else {
int j;
if (seen_dashdash || *arg == '^')
die("bad revision '%s'", arg);
@@ -2255,8 +2257,6 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
append_prune_data(&prune_data, argv + i);
break;
}
- else
- got_rev_arg = 1;
}
if (prune_data.nr) {
--
2.1.4
^ permalink raw reply related
* [PATCH 1/6 v5] revision.c: do not update argv with unknown option
From: Siddharth Kannan @ 2017-02-25 7:24 UTC (permalink / raw)
To: git
Cc: gitster, Matthieu.Moy, pranit.bauva, peff, pclouds, sandals,
Siddharth Kannan
In-Reply-To: <1488007487-12965-1-git-send-email-kannan.siddharth12@gmail.com>
handle_revision_opt() tries to recognize and handle the given argument. If an
option was unknown to it, it used to add the option to unkv[(*unkc)++]. This
increment of unkc causes the variable in the caller to change.
Teach handle_revision_opt to not update unknown arguments inside unkc anymore.
This is now the responsibility of the caller.
There are two callers of this function:
1. setup_revision: Changes have been made so that setup_revision will now
update the unknown option in argv
2. parse_revision_opt: No changes are required here. This function throws an
error whenever the option provided as argument was unknown to
handle_revision_opt().
Signed-off-by: Siddharth Kannan <kannan.siddharth12@gmail.com>
---
revision.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/revision.c b/revision.c
index b37dbec..5674a9a 100644
--- a/revision.c
+++ b/revision.c
@@ -2016,8 +2016,6 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
revs->ignore_missing = 1;
} else {
int opts = diff_opt_parse(&revs->diffopt, argv, argc, revs->prefix);
- if (!opts)
- unkv[(*unkc)++] = arg;
return opts;
}
if (revs->graph && revs->track_linear)
@@ -2234,6 +2232,8 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
}
if (opts < 0)
exit(128);
+ /* arg is an unknown option */
+ argv[left++] = arg;
continue;
}
--
2.1.4
^ permalink raw reply related
* [PATCH 1/6 v5] revision.c: do not update argv with unknown option
From: Siddharth Kannan @ 2017-02-25 7:32 UTC (permalink / raw)
To: git
Cc: gitster, Matthieu.Moy, pranit.bauva, peff, pclouds, sandals,
Siddharth Kannan
In-Reply-To: <1488007487-12965-1-git-send-email-kannan.siddharth12@gmail.com>
handle_revision_opt() tries to recognize and handle the given argument. If an
option was unknown to it, it used to add the option to unkv[(*unkc)++]. This
increment of unkc causes the variable in the caller to change.
Teach handle_revision_opt to not update unknown arguments inside unkc anymore.
This is now the responsibility of the caller.
There are two callers of this function:
1. setup_revision: Changes have been made so that setup_revision will now
update the unknown option in argv
2. parse_revision_opt: No changes are required here. This function throws an
error whenever the option provided as argument was unknown to
handle_revision_opt().
Signed-off-by: Siddharth Kannan <kannan.siddharth12@gmail.com>
---
revision.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/revision.c b/revision.c
index b37dbec..5674a9a 100644
--- a/revision.c
+++ b/revision.c
@@ -2016,8 +2016,6 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
revs->ignore_missing = 1;
} else {
int opts = diff_opt_parse(&revs->diffopt, argv, argc, revs->prefix);
- if (!opts)
- unkv[(*unkc)++] = arg;
return opts;
}
if (revs->graph && revs->track_linear)
@@ -2234,6 +2232,8 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
}
if (opts < 0)
exit(128);
+ /* arg is an unknown option */
+ argv[left++] = arg;
continue;
}
--
2.1.4
^ permalink raw reply related
* Re: [ANNOUNCE] Git v2.12.0
From: Willy Tarreau @ 2017-02-25 7:40 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Linux Kernel
In-Reply-To: <xmqqd1e72xs5.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Fri, Feb 24, 2017 at 11:28:58AM -0800, Junio C Hamano wrote:
> * Use of an empty string that is used for 'everything matches' is
> still warned and Git asks users to use a more explicit '.' for that
> instead. The hope is that existing users will not mind this
> change, and eventually the warning can be turned into a hard error,
> upgrading the deprecation into removal of this (mis)feature. That
> is not scheduled to happen in the upcoming release (yet).
FWIW '.' is not equivalent to '' when it comes to grep or such commands,
you should suggest '^' or '$' instead, otherwise you'll miss empty lines
and users will continue to use '' for that purpose. BTW I do use grep ''
a lot, but only on file systems, not within git (eg: to display contents
of /sys).
Cheers,
Willy
^ permalink raw reply
* Re: [ANNOUNCE] Git v2.12.0
From: Junio C Hamano @ 2017-02-25 8:31 UTC (permalink / raw)
To: Willy Tarreau; +Cc: git, Linux Kernel
In-Reply-To: <20170225074057.GA460@1wt.eu>
Willy Tarreau <w@1wt.eu> writes:
> Hi Junio,
>
> On Fri, Feb 24, 2017 at 11:28:58AM -0800, Junio C Hamano wrote:
>> * Use of an empty string that is used for 'everything matches' is
>> still warned and Git asks users to use a more explicit '.' for that
>> instead. The hope is that existing users will not mind this
>> change, and eventually the warning can be turned into a hard error,
>> upgrading the deprecation into removal of this (mis)feature. That
>> is not scheduled to happen in the upcoming release (yet).
>
> FWIW '.' is not equivalent to '' when it comes to grep or such commands,
I am amused and amazed ;-).
The above is not about "grep" but was meant to describe "pathspec".
We used to take "" as a pathspec element that means "every path
matches", but recently started deprecating it and ask users to be
more explicit by using "." (as a directory as a pathspec element
matches everything inside the directory). We are not changing the
pattern matching done by "git grep" or "log --grep". What is
changing is that between the two that means the same thing:
cd t/ && git log ""
cd t/ && git log .
the former is deprecated.
I find it amusing that I have been writing the above in the draft
release notes without realizing that I failed to say that it is
about pathspec for quite some time, and without realizing that the
above can be misinterpreted as if it is talking about grep patterns.
And I find it amazing that it took this long for somebody to spot
that misleading vagueness in this description and point it out.
It should probably be updated to start like so:
Use of an empty string as a pathspec element that is used
for 'everything matches' is ...
Thanks.
^ permalink raw reply
* Re: [ANNOUNCE] Git v2.12.0
From: Willy Tarreau @ 2017-02-25 8:36 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Linux Kernel
In-Reply-To: <xmqqefymzn6u.fsf@gitster.mtv.corp.google.com>
On Sat, Feb 25, 2017 at 12:31:21AM -0800, Junio C Hamano wrote:
> Willy Tarreau <w@1wt.eu> writes:
>
> > Hi Junio,
> >
> > On Fri, Feb 24, 2017 at 11:28:58AM -0800, Junio C Hamano wrote:
> >> * Use of an empty string that is used for 'everything matches' is
> >> still warned and Git asks users to use a more explicit '.' for that
> >> instead. The hope is that existing users will not mind this
> >> change, and eventually the warning can be turned into a hard error,
> >> upgrading the deprecation into removal of this (mis)feature. That
> >> is not scheduled to happen in the upcoming release (yet).
> >
> > FWIW '.' is not equivalent to '' when it comes to grep or such commands,
>
> I am amused and amazed ;-).
>
> The above is not about "grep" but was meant to describe "pathspec".
> We used to take "" as a pathspec element that means "every path
> matches", but recently started deprecating it and ask users to be
> more explicit by using "." (as a directory as a pathspec element
> matches everything inside the directory). We are not changing the
> pattern matching done by "git grep" or "log --grep". What is
> changing is that between the two that means the same thing:
>
> cd t/ && git log ""
> cd t/ && git log .
>
> the former is deprecated.
Ah that's fun indeed I never used it like this :-)
> I find it amusing that I have been writing the above in the draft
> release notes without realizing that I failed to say that it is
> about pathspec for quite some time, and without realizing that the
> above can be misinterpreted as if it is talking about grep patterns.
>
> And I find it amazing that it took this long for somebody to spot
> that misleading vagueness in this description and point it out.
>
> It should probably be updated to start like so:
>
> Use of an empty string as a pathspec element that is used
> for 'everything matches' is ...
Hey it's the usual matter of perspective and context. When you know
what you do it's always obvious when you explain it while for others
something different is obvious :-)
Thanks for your clarification!
Willy
^ permalink raw reply
* [PATCH 0/2] interoperability test harness
From: Jeff King @ 2017-02-25 9:32 UTC (permalink / raw)
To: git
This series adds a small test harness for interoperability tests. The
heavy lifting is done by the normal test-lib.sh; this just makes it easy
for you to have access to two git versions at the same time.
This is something I've wanted a few times in the past when we make a
fix that can only be tested when interacting with a different version of
git. As we start to work on changes like new protocols or hash
functions, this will hopefully make it easier to demonstrate what
happens when an older version of git encounters our new features.
This doesn't run when the regular test suite runs (it's likely to be a
bit flakier, as it actually has to build the alternative versions
separately). So I don't necessarily expect people to run it all the
time. But it lets us write down in a repeatable way the sorts of testing
that often ends up being done manually (or not at all) today.
This series just adds the harness and a basic test that we can still
clone from modern git using v1.0.0 (yay!). If people are interested, I
suspect there are previous cases that could be backfilled. A few I can
think of are:
1. How older versions handle repositoryformatversion=2.
2. Newer clients hitting older servers without various capabilities.
One example is in:
http://public-inbox.org/git/1433961320-1366-1-git-send-email-adgar@google.com/
3. Vice-versa: older clients without capabilities hitting newer
servers (especially in exotic situations, like shallow clone).
I don't think there's a huge value in doing that for old changes unless
somebody has actively reported a problem. So only do it if it sounds
like a fun experiment. :)
[1/2]: t: add an interoperability test harness
[2/2]: t/interop: add test of old clients against modern git-daemon
Makefile | 3 ++
t/interop/.gitignore | 4 ++
t/interop/Makefile | 16 ++++++++
t/interop/README | 84 +++++++++++++++++++++++++++++++++++++++
t/interop/i0000-basic.sh | 27 +++++++++++++
t/interop/i5500-git-daemon.sh | 41 +++++++++++++++++++
t/interop/interop-lib.sh | 92 +++++++++++++++++++++++++++++++++++++++++++
t/lib-git-daemon.sh | 3 +-
8 files changed, 269 insertions(+), 1 deletion(-)
create mode 100644 t/interop/.gitignore
create mode 100644 t/interop/Makefile
create mode 100644 t/interop/README
create mode 100755 t/interop/i0000-basic.sh
create mode 100755 t/interop/i5500-git-daemon.sh
create mode 100644 t/interop/interop-lib.sh
-Peff
^ permalink raw reply
* [PATCH 1/2] t: add an interoperability test harness
From: Jeff King @ 2017-02-25 9:37 UTC (permalink / raw)
To: git
In-Reply-To: <20170225093231.k7jtvx47jieka7qm@sigill.intra.peff.net>
The current test suite is good at letting you test a
particular version of Git. But it's not very good at letting
you test _two_ versions and seeing how they interact (e.g.,
one cloning from the other).
This commit adds a test harness that will build two
arbitrary versions of git and make it easy to call them from
inside your tests. See the README and the example script for
details.
Signed-off-by: Jeff King <peff@peff.net>
---
Makefile | 3 ++
t/interop/.gitignore | 4 +++
t/interop/Makefile | 16 +++++++++
t/interop/README | 85 ++++++++++++++++++++++++++++++++++++++++++++
t/interop/i0000-basic.sh | 27 ++++++++++++++
t/interop/interop-lib.sh | 92 ++++++++++++++++++++++++++++++++++++++++++++++++
6 files changed, 227 insertions(+)
create mode 100644 t/interop/.gitignore
create mode 100644 t/interop/Makefile
create mode 100644 t/interop/README
create mode 100755 t/interop/i0000-basic.sh
create mode 100644 t/interop/interop-lib.sh
diff --git a/Makefile b/Makefile
index 8e4081e06..7156b051e 100644
--- a/Makefile
+++ b/Makefile
@@ -2254,6 +2254,9 @@ endif
ifdef GIT_PERF_MAKE_OPTS
@echo GIT_PERF_MAKE_OPTS=\''$(subst ','\'',$(subst ','\'',$(GIT_PERF_MAKE_OPTS)))'\' >>$@+
endif
+ifdef GIT_INTEROP_MAKE_OPTS
+ @echo GIT_INTEROP_MAKE_OPTS=\''$(subst ','\'',$(subst ','\'',$(GIT_INTEROP_MAKE_OPTS)))'\' >>$@+
+endif
ifdef TEST_GIT_INDEX_VERSION
@echo TEST_GIT_INDEX_VERSION=\''$(subst ','\'',$(subst ','\'',$(TEST_GIT_INDEX_VERSION)))'\' >>$@+
endif
diff --git a/t/interop/.gitignore b/t/interop/.gitignore
new file mode 100644
index 000000000..49c78d3db
--- /dev/null
+++ b/t/interop/.gitignore
@@ -0,0 +1,4 @@
+/trash directory*/
+/test-results/
+/.prove/
+/build/
diff --git a/t/interop/Makefile b/t/interop/Makefile
new file mode 100644
index 000000000..31a4bbc71
--- /dev/null
+++ b/t/interop/Makefile
@@ -0,0 +1,16 @@
+-include ../../config.mak
+export GIT_TEST_OPTIONS
+
+SHELL_PATH ?= $(SHELL)
+SHELL_PATH_SQ = $(subst ','\'',$(SHELL_PATH))
+T = $(sort $(wildcard i[0-9][0-9][0-9][0-9]-*.sh))
+
+all: $(T)
+
+$(T):
+ @echo "*** $@ ***"; '$(SHELL_PATH_SQ)' $@ $(GIT_TEST_OPTS)
+
+clean:
+ rm -rf build "trash directory".* test-results
+
+.PHONY: all clean $(T)
diff --git a/t/interop/README b/t/interop/README
new file mode 100644
index 000000000..72d42bd85
--- /dev/null
+++ b/t/interop/README
@@ -0,0 +1,85 @@
+Git version interoperability tests
+==================================
+
+This directory has interoperability tests for git. Each script is
+similar to the normal test scripts found in t/, but with the added twist
+that two special versions of git, "git.a" and "git.b", are available in
+the PATH. Individual tests can then check the interaction between the
+two versions.
+
+When you add a feature that handles backwards compatibility between git
+versions, it's encouraged to add a test here to make sure it behaves as
+you expect.
+
+
+Running Tests
+-------------
+
+The easiest way to run tests is to say "make". This runs all
+the tests against their default versions.
+
+You can run a single test like:
+
+ $ ./i0000-basic.sh
+ ok 1 - bare git is forbidden
+ ok 2 - git.a version (v1.6.6.3)
+ ok 3 - git.b version (v2.11.1)
+ # passed all 3 test(s)
+ 1..3
+
+Each test contains default versions to run against. You may override
+these by setting `GIT_TEST_VERSION_A` and `GIT_TEST_VERSION_B` in the
+environment. Note that not all combinations will give sensible outcomes
+for all tests (e.g., a test checking for a specific old/new interaction
+may want something "old" enough" and something "new" enough; see
+individual tests for details).
+
+Version names should be resolvable as revisions in the current
+repository. They will be exported and built as needed using the
+config.mak files found at the root of your working tree.
+
+The exception is the special version "." which uses the currently-built
+contents of your working tree.
+
+You can set the following variables (in the environment or in your config.mak):
+
+ GIT_INTEROP_MAKE_OPTS
+ Options to pass to `make` when building a git version (e.g.,
+ `-j8`).
+
+You can also pass any command-line options taken by ordinary git tests (e.g.,
+"-v").
+
+
+Naming Tests
+------------
+
+The interop test files are named like:
+
+ iNNNN-short-description.sh
+
+where N is a decimal digit. The same conventions for choosing NNNN as
+for normal tests apply.
+
+
+Writing Tests
+-------------
+
+An interop test script starts like a normal script, declaring a few
+variables and then including interop-lib.sh (which includes test-lib.sh).
+Besides test_description, you should also set the $VERSION_A and $VERSION_B
+variables to give the default versions to test against. See t0000-basic.sh for
+an example.
+
+You can then use test_expect_success as usual, with a few differences:
+
+ 1. The special commands "git.a" and "git.b" correspond to the
+ two versions.
+
+ 2. You cannot call a bare "git". This is to prevent accidents where
+ you meant "git.a" or "git.b".
+
+ 3. The trash directory is _not_ a git repository by default. You
+ should create one with the appropriate version of git.
+
+At the end of the script, call test_done as usual.
diff --git a/t/interop/i0000-basic.sh b/t/interop/i0000-basic.sh
new file mode 100755
index 000000000..903e9193f
--- /dev/null
+++ b/t/interop/i0000-basic.sh
@@ -0,0 +1,27 @@
+#!/bin/sh
+
+# Note that this test only works on real version numbers,
+# as it depends on matching the output to "git version".
+VERSION_A=v1.6.6.3
+VERSION_B=v2.11.1
+
+test_description='sanity test interop library'
+. ./interop-lib.sh
+
+test_expect_success 'bare git is forbidden' '
+ test_must_fail git version
+'
+
+test_expect_success "git.a version ($VERSION_A)" '
+ echo git version ${VERSION_A#v} >expect &&
+ git.a version >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success "git.b version ($VERSION_B)" '
+ echo git version ${VERSION_B#v} >expect &&
+ git.b version >actual &&
+ test_cmp expect actual
+'
+
+test_done
diff --git a/t/interop/interop-lib.sh b/t/interop/interop-lib.sh
new file mode 100644
index 000000000..7f31d9cc6
--- /dev/null
+++ b/t/interop/interop-lib.sh
@@ -0,0 +1,92 @@
+# Interoperability testing framework. Each script should source
+# this after setting default $VERSION_A and $VERSION_B variables.
+
+. ../../GIT-BUILD-OPTIONS
+INTEROP_ROOT=$(pwd)
+BUILD_ROOT=$INTEROP_ROOT/build
+
+build_version() {
+ if test -z "$1"
+ then
+ echo >&2 "error: test script did not set default versions"
+ return 1
+ fi
+
+ if test "$1" = "."
+ then
+ git rev-parse --show-toplevel
+ return 0
+ fi
+
+ sha1=$(git rev-parse "$1^{tree}") || return 1
+ dir=$BUILD_ROOT/$sha1
+
+ if test -e "$dir/.built"
+ then
+ echo "$dir"
+ return 0
+ fi
+
+ echo >&2 "==> Building $1..."
+
+ mkdir -p "$dir" || return 1
+
+ (cd "$(git rev-parse --show-cdup)" && git archive --format=tar "$sha1") |
+ (cd "$dir" && tar x) ||
+ return 1
+
+ for config in config.mak config.mak.autogen config.status
+ do
+ if test -e "$INTEROP_ROOT/../../$config"
+ then
+ cp "$INTEROP_ROOT/../../$config" "$dir/" || return 1
+ fi
+ done
+
+ (
+ cd "$dir" &&
+ make $GIT_INTEROP_MAKE_OPTS >&2 &&
+ touch .built
+ ) || return 1
+
+ echo "$dir"
+}
+
+# Old versions of git don't have bin-wrappers, so let's give a rough emulation.
+wrap_git() {
+ write_script "$1" <<-EOF
+ GIT_EXEC_PATH="$2"
+ export GIT_EXEC_PATH
+ PATH="$2:\$PATH"
+ export GIT_EXEC_PATH
+ exec git "\$@"
+ EOF
+}
+
+generate_wrappers() {
+ mkdir -p .bin &&
+ wrap_git .bin/git.a "$DIR_A" &&
+ wrap_git .bin/git.b "$DIR_B" &&
+ write_script .bin/git <<-\EOF &&
+ echo >&2 fatal: test tried to run generic git
+ exit 1
+ EOF
+ PATH=$(pwd)/.bin:$PATH
+}
+
+VERSION_A=${GIT_TEST_VERSION_A:-$VERSION_A}
+VERSION_B=${GIT_TEST_VERSION_B:-$VERSION_B}
+
+if ! DIR_A=$(build_version "$VERSION_A") ||
+ ! DIR_B=$(build_version "$VERSION_B")
+then
+ echo >&2 "fatal: unable to build git versions"
+ exit 1
+fi
+
+TEST_DIRECTORY=$INTEROP_ROOT/..
+TEST_OUTPUT_DIRECTORY=$INTEROP_ROOT
+TEST_NO_CREATE_REPO=t
+. "$TEST_DIRECTORY"/test-lib.sh
+
+generate_wrappers || die "unable to set up interop test environment"
--
2.12.0.616.g5f622f3b1
^ permalink raw reply related
* [PATCH] sha1_file: release fallback base's memory in unpack_entry()
From: René Scharfe @ 2017-02-25 10:02 UTC (permalink / raw)
To: Git List; +Cc: Junio C Hamano
If a pack entry that's used as a delta base is corrupt, unpack_entry()
marks it as unusable and then searches the object again in the hope that
it can be found in another pack or in a loose file. The memory for this
external base object is never released. Free it after use.
Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
sha1_file.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/sha1_file.c b/sha1_file.c
index ec957db5e1..8ce80d4481 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -2532,6 +2532,7 @@ void *unpack_entry(struct packed_git *p, off_t obj_offset,
while (delta_stack_nr) {
void *delta_data;
void *base = data;
+ void *external_base = NULL;
unsigned long delta_size, base_size = size;
int i;
@@ -2558,6 +2559,7 @@ void *unpack_entry(struct packed_git *p, off_t obj_offset,
p->pack_name);
mark_bad_packed_object(p, base_sha1);
base = read_object(base_sha1, &type, &base_size);
+ external_base = base;
}
}
@@ -2576,6 +2578,7 @@ void *unpack_entry(struct packed_git *p, off_t obj_offset,
"at offset %"PRIuMAX" from %s",
(uintmax_t)curpos, p->pack_name);
data = NULL;
+ free(external_base);
continue;
}
@@ -2595,6 +2598,7 @@ void *unpack_entry(struct packed_git *p, off_t obj_offset,
error("failed to apply delta");
free(delta_data);
+ free(external_base);
}
*final_type = type;
--
2.12.0
^ permalink raw reply related
* [PATCH 2/2] t/interop: add test of old clients against modern git-daemon
From: Jeff King @ 2017-02-25 9:37 UTC (permalink / raw)
To: git
In-Reply-To: <20170225093231.k7jtvx47jieka7qm@sigill.intra.peff.net>
This test just checks that old clients can clone and fetch
from a newer git-daemon. The opposite should also be true,
but it's hard to test ancient versions of git-daemon because
they lack basic options like "--listen".
Note that we have to make a slight tweak to the
lib-git-daemon helper from the regular tests, so that it
starts the daemon with our correct git.a version.
Signed-off-by: Jeff King <peff@peff.net>
---
t/interop/i5500-git-daemon.sh | 41 +++++++++++++++++++++++++++++++++++++++++
t/lib-git-daemon.sh | 3 ++-
2 files changed, 43 insertions(+), 1 deletion(-)
create mode 100755 t/interop/i5500-git-daemon.sh
diff --git a/t/interop/i5500-git-daemon.sh b/t/interop/i5500-git-daemon.sh
new file mode 100755
index 000000000..1daf69420
--- /dev/null
+++ b/t/interop/i5500-git-daemon.sh
@@ -0,0 +1,41 @@
+#!/bin/sh
+
+VERSION_A=.
+VERSION_B=v1.0.0
+
+: ${LIB_GIT_DAEMON_PORT:=5500}
+LIB_GIT_DAEMON_COMMAND='git.a daemon'
+
+test_description='clone and fetch by older client'
+. ./interop-lib.sh
+. "$TEST_DIRECTORY"/lib-git-daemon.sh
+
+start_git_daemon --export-all
+
+repo=$GIT_DAEMON_DOCUMENT_ROOT_PATH/repo
+
+test_expect_success "create repo served by $VERSION_A" '
+ git.a init "$repo" &&
+ git.a -C "$repo" commit --allow-empty -m one
+'
+
+test_expect_success "clone with $VERSION_B" '
+ git.b clone "$GIT_DAEMON_URL/repo" child &&
+ echo one >expect &&
+ git.a -C child log -1 --format=%s >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success "fetch with $VERSION_B" '
+ git.a -C "$repo" commit --allow-empty -m two &&
+ (
+ cd child &&
+ git.b fetch
+ ) &&
+ echo two >expect &&
+ git.a -C child log -1 --format=%s FETCH_HEAD >actual &&
+ test_cmp expect actual
+'
+
+stop_git_daemon
+test_done
diff --git a/t/lib-git-daemon.sh b/t/lib-git-daemon.sh
index f9cbd4793..987d40680 100644
--- a/t/lib-git-daemon.sh
+++ b/t/lib-git-daemon.sh
@@ -46,7 +46,8 @@ start_git_daemon() {
say >&3 "Starting git daemon ..."
mkfifo git_daemon_output
- git daemon --listen=127.0.0.1 --port="$LIB_GIT_DAEMON_PORT" \
+ ${LIB_GIT_DAEMON_COMMAND:-git daemon} \
+ --listen=127.0.0.1 --port="$LIB_GIT_DAEMON_PORT" \
--reuseaddr --verbose \
--base-path="$GIT_DAEMON_DOCUMENT_ROOT_PATH" \
"$@" "$GIT_DAEMON_DOCUMENT_ROOT_PATH" \
--
2.12.0.616.g5f622f3b1
^ permalink raw reply related
* [PATCH 1/2] apply: guard against renames of non-existant empty files
From: Vegard Nossum @ 2017-02-25 10:13 UTC (permalink / raw)
To: Junio C Hamano, git; +Cc: Christian Couder, Michal Zalewski, Vegard Nossum
If we have a patch like the one in the new test-case, then we will
try to rename a non-existant empty file, i.e. patch->old_name will
be NULL. In this case, a NULL entry will be added to fn_table, which
is not allowed (a subsequent binary search will die with a NULL
pointer dereference).
The patch file is completely bogus as it tries to rename something
that is known not to exist, so we can throw an error for this.
Found using AFL.
Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>
---
apply.c | 3 ++-
| 15 +++++++++++++++
2 files changed, 17 insertions(+), 1 deletion(-)
create mode 100755 t/t4154-apply-git-header.sh
diff --git a/apply.c b/apply.c
index 0e2caeab9..cbf7cc7f2 100644
--- a/apply.c
+++ b/apply.c
@@ -1585,7 +1585,8 @@ static int find_header(struct apply_state *state,
patch->old_name = xstrdup(patch->def_name);
patch->new_name = xstrdup(patch->def_name);
}
- if (!patch->is_delete && !patch->new_name) {
+ if ((!patch->is_delete && !patch->new_name) ||
+ (patch->is_rename && !patch->old_name)) {
error(_("git diff header lacks filename information "
"(line %d)"), state->linenr);
return -128;
--git a/t/t4154-apply-git-header.sh b/t/t4154-apply-git-header.sh
new file mode 100755
index 000000000..d651af4a2
--- /dev/null
+++ b/t/t4154-apply-git-header.sh
@@ -0,0 +1,15 @@
+#!/bin/sh
+
+test_description='apply with git/--git headers'
+
+. ./test-lib.sh
+
+test_expect_success 'apply old mode / rename new' '
+ test_must_fail git apply << EOF
+diff --git a/1 b/1
+old mode 0
+rename new 0
+EOF
+'
+
+test_done
--
2.12.0.rc0
^ permalink raw reply related
* git status --> Out of memory, realloc failed
From: Carsten Fuchs @ 2017-02-25 10:13 UTC (permalink / raw)
To: git
Dear Git group,
I use Git at a web hosting service, where my user account has a memory
limit of 768 MB:
(uiserver):p7715773:~$ uname -a
Linux infongp-de15 3.14.0-ui16322-uiabi1-infong-amd64 #1 SMP Debian
3.14.79-2~ui80+4 (2016-11-17) x86_64 GNU/Linux
(uiserver):p7715773:~$ git --version
git version 2.1.4
The problem is that `git status` fails with an out of memory error:
(uiserver):p7715773:~/cafu$ git status
fatal: Out of memory, realloc failed
fatal: recursion detected in die handler
I talked to their support and their suggestion was to set a couple of
memory constraints and to run `git gc`. This seemed to work well – but
`git status` still fails:
(uiserver):p7715773:~/cafu$ cat ~/.gitconfig
[color]
ui = auto
[user]
name = Carsten Fuchs
email = carsten.fuchs@cafu.de
[core]
editor = nano
pager = less -M -FRXS
packedgitwindowsize = 30m
packedgitlimit = 40m
[i18n]
commitEncoding = ISO-8859-1
[pack]
threads = 1
windowMemory = 10m
packSizeLimit = 20m
deltaCacheSize = 30m
(uiserver):p7715773:~/cafu$ git gc
Zähle Objekte: 44293, Fertig.
Komprimiere Objekte: 100% (24534/24534), Fertig.
Schreibe Objekte: 100% (44293/44293), Fertig.
Total 44293 (delta 17560), reused 41828 (delta 16708)
(uiserver):p7715773:~/cafu$ git status
fatal: Out of memory, realloc failed
fatal: recursion detected in die handler
The repository is tracking about 19000 files which together take 260 MB.
The git server version is 2.7.4.1.g5468f9e (Bitbucket)
Well, their next response was that they have no solution for me –
except, unsurprisingly, coaxing me into a more expensive hosting package.
I've read the Git man page about `git config`, but was not able to come
up with anything to improve the situation.
Any ideas what I could do to reduce the memory consumption of `git status`?
Best regards,
Carsten
PS: Many thanks to Philip Oakley for initial advice at git-users, to
which I should have properly subscribed in the first place, as the
Google Groups interface seems to lose messages (mine at least, and
inadvertently posts them as HTML) and gmane's NNTP interface reports
that it is unidirectional/read-only.
^ permalink raw reply
* [PATCH 2/2] apply: handle assertion failure gracefully
From: Vegard Nossum @ 2017-02-25 10:13 UTC (permalink / raw)
To: Junio C Hamano, git; +Cc: Christian Couder, Michal Zalewski, Vegard Nossum
In-Reply-To: <20170225101307.24067-1-vegard.nossum@oracle.com>
For the patches in the added testcases, we were crashing with:
git-apply: apply.c:3665: check_preimage: Assertion `patch->is_new <= 0' failed.
As it turns out, check_preimage() is prepared to handle these conditions,
so we can remove the assertion.
Found using AFL.
Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>
---
(I'm fully aware of how it looks to just delete an assertion to "fix" a
bug without any other changes to accomodate the condition that was
being tested for. I am definitely not an expert on this code, but as far
as I can tell -- both by reviewing and testing the code -- the function
really is prepared to handle the case where patch->is_new == 1, as it
will always hit another error condition if that is true. I've tried to
add more test cases to show what errors you can expect to see instead of
the assertion failure when trying to apply these nonsensical patches. If
you don't want to remove the assertion for whatever reason, please feel
free to take the testcases and add "# TODO: known breakage" or whatever.)
---
apply.c | 1 -
| 36 ++++++++++++++++++++++++++++++++++++
2 files changed, 36 insertions(+), 1 deletion(-)
diff --git a/apply.c b/apply.c
index cbf7cc7f2..9219d2737 100644
--- a/apply.c
+++ b/apply.c
@@ -3652,7 +3652,6 @@ static int check_preimage(struct apply_state *state,
if (!old_name)
return 0;
- assert(patch->is_new <= 0);
previous = previous_patch(state, patch, &status);
if (status)
--git a/t/t4154-apply-git-header.sh b/t/t4154-apply-git-header.sh
index d651af4a2..c440c48ad 100755
--- a/t/t4154-apply-git-header.sh
+++ b/t/t4154-apply-git-header.sh
@@ -12,4 +12,40 @@ rename new 0
EOF
'
+test_expect_success 'apply deleted file mode / new file mode / wrong mode' '
+ test_must_fail git apply << EOF
+diff --git a/. b/.
+deleted file mode
+new file mode
+EOF
+'
+
+test_expect_success 'apply deleted file mode / new file mode / wrong type' '
+ mkdir x &&
+ chmod 755 x &&
+ test_must_fail git apply << EOF
+diff --git a/x b/x
+deleted file mode 160755
+new file mode
+EOF
+'
+
+test_expect_success 'apply deleted file mode / new file mode / already exists' '
+ touch 1 &&
+ chmod 644 1 &&
+ test_must_fail git apply << EOF
+diff --git a/1 b/1
+deleted file mode 100644
+new file mode
+EOF
+'
+
+test_expect_success 'apply new file mode / copy from / nonexistant file' '
+ test_must_fail git apply << EOF
+diff --git a/. b/.
+new file mode
+copy from
+EOF
+'
+
test_done
--
2.12.0.rc0
^ permalink raw reply related
* [PATCH] cocci: use ALLOC_ARRAY
From: René Scharfe @ 2017-02-25 10:30 UTC (permalink / raw)
To: Git List; +Cc: Junio C Hamano
Add a semantic patch for using ALLOC_ARRAY to allocate arrays and apply
the transformation on the current source tree. The macro checks for
multiplication overflow and infers the element size automatically; the
result is shorter and safer code.
Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
contrib/coccinelle/array.cocci | 16 ++++++++++++++++
worktree.c | 2 +-
2 files changed, 17 insertions(+), 1 deletion(-)
diff --git a/contrib/coccinelle/array.cocci b/contrib/coccinelle/array.cocci
index 2d7f25d99f..4ba98b7eaf 100644
--- a/contrib/coccinelle/array.cocci
+++ b/contrib/coccinelle/array.cocci
@@ -24,3 +24,19 @@ expression n;
@@
- memcpy(dst, src, n * sizeof(T));
+ COPY_ARRAY(dst, src, n);
+
+@@
+type T;
+T *ptr;
+expression n;
+@@
+- ptr = xmalloc(n * sizeof(*ptr));
++ ALLOC_ARRAY(ptr, n);
+
+@@
+type T;
+T *ptr;
+expression n;
+@@
+- ptr = xmalloc(n * sizeof(T));
++ ALLOC_ARRAY(ptr, n);
diff --git a/worktree.c b/worktree.c
index d633761575..d7b911aac7 100644
--- a/worktree.c
+++ b/worktree.c
@@ -175,7 +175,7 @@ struct worktree **get_worktrees(unsigned flags)
struct dirent *d;
int counter = 0, alloc = 2;
- list = xmalloc(alloc * sizeof(struct worktree *));
+ ALLOC_ARRAY(list, alloc);
list[counter++] = get_main_worktree();
--
2.12.0
^ permalink raw reply related
* Re: [PATCH 2/2] http: add an "auto" mode for http.emptyauth
From: Johannes Schindelin @ 2017-02-25 11:48 UTC (permalink / raw)
To: Jeff King
Cc: David Turner, Junio C Hamano, git@vger.kernel.org,
sandals@crustytoothpaste.net, Eric Sunshine
In-Reply-To: <20170223013746.lturqad7lnehedb4@sigill.intra.peff.net>
Hi,
On Wed, 22 Feb 2017, Jeff King wrote:
> [two beautiful patches]
I applied them and verified that the reported issue is fixed. Thank you!
Hopefully you do not mind that I cherry-picked them in preparation for
Git for Windows v2.12.0?
I added a small fixup (https://github.com/dscho/git/commit/44ae0bcae5):
-- snip --
Subject: [PATCH] fixup! http: add an "auto" mode for http.emptyauth
Note: we keep a "black list" of authentication methods for which we do
not want to enable http.emptyAuth automatically. A white list would be
nicer, but less robust, as we want to support linking to several cURL
versions and the list of authentication methods (as well as their names)
changed over time.
[jes: actually added the "auto" handling, excluded Digest, too]
This fixes https://github.com/git-for-windows/git/issues/1034
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
http.c | 55 +++++++++++++++++++++++++++++++++----------------------
1 file changed, 33 insertions(+), 22 deletions(-)
diff --git a/http.c b/http.c
index f8eb0f23d6c..fb94c444c80 100644
--- a/http.c
+++ b/http.c
@@ -334,7 +334,10 @@ static int http_options(const char *var, const char *value, void *cb)
return git_config_string(&user_agent, var, value);
if (!strcmp("http.emptyauth", var)) {
- curl_empty_auth = git_config_bool(var, value);
+ if (value && !strcmp("auto", value))
+ curl_empty_auth = -1;
+ else
+ curl_empty_auth = git_config_bool(var, value);
return 0;
}
@@ -385,29 +388,37 @@ static int http_options(const char *var, const char *value, void *cb)
static int curl_empty_auth_enabled(void)
{
- if (curl_empty_auth < 0) {
-#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
- /*
- * In the automatic case, kick in the empty-auth
- * hack as long as we would potentially try some
- * method more exotic than "Basic".
- *
- * But only do so when this is _not_ our initial
- * request, as we would not then yet know what
- * methods are available.
- */
- return http_auth_methods_restricted &&
- http_auth_methods != CURLAUTH_BASIC;
+ if (curl_empty_auth >= 0)
+ return curl_empty_auth;
+
+#ifndef LIBCURL_CAN_HANDLE_AUTH_ANY
+ /*
+ * Our libcurl is too old to do AUTH_ANY in the first place;
+ * just default to turning the feature off.
+ */
#else
- /*
- * Our libcurl is too old to do AUTH_ANY in the first place;
- * just default to turning the feature off.
- */
- return 0;
+ /*
+ * In the automatic case, kick in the empty-auth
+ * hack as long as we would potentially try some
+ * method more exotic than "Basic".
+ *
+ * But only do this when this is our second or
+ * subsequent * request, as by then we know what
+ * methods are available.
+ */
+ if (http_auth_methods_restricted)
+ switch (http_auth_methods) {
+ case CURLAUTH_BASIC:
+ case CURLAUTH_DIGEST:
+#ifdef CURLAUTH_DIGEST_IE
+ case CURLAUTH_DIGEST_IE:
#endif
- }
-
- return curl_empty_auth;
+ return 0;
+ default:
+ return 1;
+ }
+#endif
+ return 0;
}
static void init_curl_http_auth(CURL *result)
-- snap --
As you can see, I actually implemented the handling for
http.emptyauth=auto, and I was more comfortable with handling the "easy"
cases first in the curl_empty_auth_enabled function.
I also took Dave's suggestion:
> On Thu, Feb 23, 2017 at 01:16:33AM +0000, David Turner wrote:
>
> > > + * But only do so when this is _not_ our initial
> > > + * request, as we would not then yet know what
> > > + * methods are available.
> > > + */
> >
> > Eliminate double-negative:
> >
> > "But only do this when this is our second or subsequent request,
> > as by then we know what methods are available."
>
> Yeah, that is clearer.
Thank you all!
Now, how to get this into upstream Git, too? Jeff, do you want to submit a
v2? In that case, would you please consider the fixup! I mentioned above?
Otherwise I'd be happy to take it from here.
Ciao,
Dscho
^ permalink raw reply related
* Re: [PATCH] http(s): automatically try NTLM authentication first
From: Johannes Schindelin @ 2017-02-25 11:51 UTC (permalink / raw)
To: Jeff King
Cc: Junio C Hamano, David Turner, git@vger.kernel.org,
sandals@crustytoothpaste.net, Eric Sunshine
In-Reply-To: <20170223204804.syj6tgjdrgmqdzna@sigill.intra.peff.net>
Hi Peff,
On Thu, 23 Feb 2017, Jeff King wrote:
> For Git for Windows, [PATCH 2/2] seems like the auto behavior would be a
> strict improvement over the "true" default they've been shipping.
Absolutely. Thank you for your tremendous help!
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH 1/2] apply: guard against renames of non-existant empty files
From: Philip Oakley @ 2017-02-25 11:59 UTC (permalink / raw)
To: Vegard Nossum, Junio C Hamano, git
Cc: Christian Couder, Michal Zalewski, Vegard Nossum
In-Reply-To: <20170225101307.24067-1-vegard.nossum@oracle.com>
From: "Vegard Nossum" <vegard.nossum@oracle.com>
> If we have a patch like the one in the new test-case, then we will
"the one in the new test-case" needs a clearer reference to the particular
case so that future readers will know what it refers to. Noticed while
browsing the commit message..
..reads further; Maybe it's "AFL (American fuzzy lop) found a failure. Add a
new test case and fix the fault"?
[same for patch 2]
> try to rename a non-existant empty file, i.e. patch->old_name will
> be NULL. In this case, a NULL entry will be added to fn_table, which
> is not allowed (a subsequent binary search will die with a NULL
> pointer dereference).
>
> The patch file is completely bogus as it tries to rename something
> that is known not to exist, so we can throw an error for this.
>
> Found using AFL.
>
> Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>
> ---
> apply.c | 3 ++-
> t/t4154-apply-git-header.sh | 15 +++++++++++++++
> 2 files changed, 17 insertions(+), 1 deletion(-)
> create mode 100755 t/t4154-apply-git-header.sh
>
> diff --git a/apply.c b/apply.c
> index 0e2caeab9..cbf7cc7f2 100644
> --- a/apply.c
> +++ b/apply.c
> @@ -1585,7 +1585,8 @@ static int find_header(struct apply_state *state,
> patch->old_name = xstrdup(patch->def_name);
> patch->new_name = xstrdup(patch->def_name);
> }
> - if (!patch->is_delete && !patch->new_name) {
> + if ((!patch->is_delete && !patch->new_name) ||
> + (patch->is_rename && !patch->old_name)) {
> error(_("git diff header lacks filename information "
> "(line %d)"), state->linenr);
> return -128;
> diff --git a/t/t4154-apply-git-header.sh b/t/t4154-apply-git-header.sh
> new file mode 100755
> index 000000000..d651af4a2
> --- /dev/null
> +++ b/t/t4154-apply-git-header.sh
> @@ -0,0 +1,15 @@
> +#!/bin/sh
> +
> +test_description='apply with git/--git headers'
> +
> +. ./test-lib.sh
> +
> +test_expect_success 'apply old mode / rename new' '
> + test_must_fail git apply << EOF
> +diff --git a/1 b/1
> +old mode 0
> +rename new 0
> +EOF
> +'
> +
> +test_done
> --
> 2.12.0.rc0
--
Philip
^ permalink raw reply
* Re: [PATCH 1/2] apply: guard against renames of non-existant empty files
From: Vegard Nossum @ 2017-02-25 12:06 UTC (permalink / raw)
To: Philip Oakley, Junio C Hamano, git; +Cc: Christian Couder, Michal Zalewski
In-Reply-To: <E14B054C79E1450F8B41E6477D1D7CD1@PhilipOakley>
On 25/02/2017 12:59, Philip Oakley wrote:
> From: "Vegard Nossum" <vegard.nossum@oracle.com>
>> If we have a patch like the one in the new test-case, then we will
>
> "the one in the new test-case" needs a clearer reference to the
> particular case so that future readers will know what it refers to.
> Noticed while browsing the commit message..
There is only one testcase added by this patch, so how is it possibly
unclear? In what situation would you read a commit message and not even
think to glance at the patch for more details?
Vegard
^ permalink raw reply
* Re: fatal error when diffing changed symlinks
From: Johannes Schindelin @ 2017-02-25 12:36 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git, Christophe Macabiau
In-Reply-To: <20170224203523.mdoh4ivhwflmpr6j@sigill.intra.peff.net>
Hi Peff & Junio,
On Fri, 24 Feb 2017, Jeff King wrote:
> On Fri, Feb 24, 2017 at 11:51:22AM -0800, Junio C Hamano wrote:
>
> > > A slightly worse is that the upcoming Git will ship with a rewritten
> > > "difftool" that makes the above sequence segfault.
> >
> > The culprit seems to be these lines in run_dir_diff():
> >
> > 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);
> > }
> >
> > When viewing a working tree file, oid.hash could be 0{40} and
> > read_sha1_file() is not the right function to use to obtain the
> > contents.
> >
> > Both of these two need to pay attention to 0{40}, I think, as the
> > user may be running "difftool -R --dir-diff" in which case the
> > working tree would appear in the left hand side instead.
>
> As a side note, I think even outside of 0{40}, this should be checking
> the return value of read_sha1_file(). A corrupted repo should die(), not
> segfault.
I agree. I am on it.
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH 1/2] apply: guard against renames of non-existant empty files
From: Philip Oakley @ 2017-02-25 12:47 UTC (permalink / raw)
To: Junio C Hamano, git, Vegard Nossum; +Cc: Christian Couder, Michal Zalewski
In-Reply-To: <0cdd4304-7b71-c38d-21ab-b4e997242bd4@oracle.com>
From: "Vegard Nossum" <vegard.nossum@oracle.com>
> On 25/02/2017 12:59, Philip Oakley wrote:
>> From: "Vegard Nossum" <vegard.nossum@oracle.com>
>>> If we have a patch like the one in the new test-case, then we will
>>
>> "the one in the new test-case" needs a clearer reference to the
>> particular case so that future readers will know what it refers to.
>> Noticed while browsing the commit message..
>
> There is only one testcase added by this patch, so how is it possibly
> unclear? In what situation would you read a commit message and not even
> think to glance at the patch for more details?
>
On initial reading of a commit message, the expectation is that the commit
will be about a change from some previous state, so I immediately asked
myself, where is that new (recent) test case from.
You could say "This patch presents a new test case" which would straight
away set the expectation that one should read on to see what its about. It
was just that as a reader of the log message I didn't pick up the sense you
wanted to convey. It's easy to see with hindsight or fore-knowledge.
I, personally, think that bringing the AFL discovery to the fore would help
in explaining why/how the patch appeared in the first place.
Hope that helps explain why I responded.
regards
Philip
^ 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