* Re: [PATCHv3] submodule--helper: normalize funny urls
From: Junio C Hamano @ 2016-10-20 19:53 UTC (permalink / raw)
To: Stefan Beller
Cc: Johannes Sixt, Johannes Schindelin, git@vger.kernel.org, Karl A.,
Dennis Kaarsemaker, Jonathan Nieder
In-Reply-To: <CAGZ79kZyCXKnt9kTbnD8KrnsjJVb_7juW2epmdP9XqcRGfg=GA@mail.gmail.com>
Stefan Beller <sbeller@google.com> writes:
> My thought was to fix it nevertheless, such that the url recorded as
> remote.origin.url is always the first case (no l or /. at the end).
>
> If we were to add this fix to clone, then it may be easier to debug
> submodule url schemes for users as the submodule url would then
> be a concatenation of remote.origin.url and the relative part.
>
> That seems easier to understand than ${remote.origin.url%%/.} +
> relative path, maybe? (Because then the user doesn't need to guess
> or remember historical behavior that is wrong on how this)
Are you declaring that trailing / or /. will now be illegal? If you
are declaring that, then I agree that new codepaths no longer have
to worry about "strip / or /. before concatenating" and will
simplify things for them. But otherwise, such a "fix" also would
have an effect of hiding bugs from codepaths. They still need to be
prepared to see any of the three variants and cope with them
correctly, right?
^ permalink raw reply
* Re: [PATCHv3] submodule--helper: normalize funny urls
From: Stefan Beller @ 2016-10-20 19:34 UTC (permalink / raw)
To: Junio C Hamano
Cc: Johannes Sixt, Johannes Schindelin, git@vger.kernel.org, Karl A.,
Dennis Kaarsemaker, Jonathan Nieder
In-Reply-To: <xmqqzilyg6tk.fsf@gitster.mtv.corp.google.com>
On Thu, Oct 20, 2016 at 12:26 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> Do we actually want to fix git-clone as well?
>
> If I understand correctly, the point of this fix is to make it not
> to matter whether the original URL the end user gives or recorded as
> the remote by "git clone" in the repository is any one of:
>
> $any_leading_part/path/to/dir
> $any_leading_part/path/to/dir/
> $any_leading_part/path/to/dir/.
>
> So I do not think there is anything to "fix", as long as "git clone"
Yes "git clone" works with any of the three above.
My thought was to fix it nevertheless, such that the url recorded as
remote.origin.url is always the first case (no l or /. at the end).
If we were to add this fix to clone, then it may be easier to debug
submodule url schemes for users as the submodule url would then
be a concatenation of remote.origin.url and the relative part.
That seems easier to understand than ${remote.origin.url%%/.} +
relative path, maybe? (Because then the user doesn't need to guess
or remember historical behavior that is wrong on how this)
> that is given any one of the above three records any one of the
> above three as the result. It _may_ be desirable if the result is
> identical what was given as input, but I do not offhand think that
> is required.
>
>> I tried and then I see breakage in 5603-clone-dirname
>> as ssh://host seems to be an invalid url; it has to end with a slash?
>
> That is a separate issue, isn't it? We shouldn't be touching the
> leading "<scheme>://<host>/" part, I would think.
I agree, So I'll first fix the submodule parts only.
>
> For example, a URL "../another" relative to "ssh://host/path" may be
> "ssh://host/another", but shouldn't it be an error to take
> "../../outside" relative to "ssh://host/path"?
That is correct. I'll stop looking at clone code.
^ permalink raw reply
* Re: [PATCHv3] submodule--helper: normalize funny urls
From: Junio C Hamano @ 2016-10-20 19:26 UTC (permalink / raw)
To: Stefan Beller
Cc: Johannes Sixt, Johannes Schindelin, git@vger.kernel.org, Karl A.,
Dennis Kaarsemaker, Jonathan Nieder
In-Reply-To: <CAGZ79kY4u-k8v3g5Jmod4acwJ81AnJYeYMQZ2tarx6UMGj8Xew@mail.gmail.com>
Stefan Beller <sbeller@google.com> writes:
> Do we actually want to fix git-clone as well?
If I understand correctly, the point of this fix is to make it not
to matter whether the original URL the end user gives or recorded as
the remote by "git clone" in the repository is any one of:
$any_leading_part/path/to/dir
$any_leading_part/path/to/dir/
$any_leading_part/path/to/dir/.
So I do not think there is anything to "fix", as long as "git clone"
that is given any one of the above three records any one of the
above three as the result. It _may_ be desirable if the result is
identical what was given as input, but I do not offhand think that
is required.
> I tried and then I see breakage in 5603-clone-dirname
> as ssh://host seems to be an invalid url; it has to end with a slash?
That is a separate issue, isn't it? We shouldn't be touching the
leading "<scheme>://<host>/" part, I would think.
For example, a URL "../another" relative to "ssh://host/path" may be
"ssh://host/another", but shouldn't it be an error to take
"../../outside" relative to "ssh://host/path"?
^ permalink raw reply
* Re: [PATCHv3] submodule--helper: normalize funny urls
From: Stefan Beller @ 2016-10-20 19:15 UTC (permalink / raw)
To: Junio C Hamano
Cc: Johannes Sixt, Johannes Schindelin, git@vger.kernel.org, Karl A.,
Dennis Kaarsemaker, Jonathan Nieder
In-Reply-To: <xmqqwph5m6th.fsf@gitster.mtv.corp.google.com>
On Tue, Oct 18, 2016 at 7:05 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>>> I am not sure. Certainly we would want to make sure that the normal
>>> case (i.e. no funny trailing junk) to work correctly, but we do want
>>> to protect the fix from future breakage as well, no?
>>
>> Exactly. So not intermediate "root" that we clone from, but adapting the
>> relative URLs. Maybe half the broken tests can switch to 'root' and the others
>> go with the current behavior of cloning . to super.
>>>
>>> Perhaps we can do a preliminary step to update tests to primarily
>>> check the cases that do not involve URL with trailing "/." by either
>>> doing that double clone, or more preferrably, clone from "$(pwd)"
>>> and adjust the incorrect submodule reference that have been relying
>>> on the buggy behaviour. With that "root" trick, the test would pass
>>> with or without the fix under discussion, right?
>>
>> I assume so, (not tested).
>
> OK. Thanks for sticking with it.
Do we actually want to fix git-clone as well?
I tried and then I see breakage in 5603-clone-dirname
as ssh://host seems to be an invalid url; it has to end with a slash?
If we were to fix clone as well, then we'd still have a lot of possible stale
data (ending in /.) out there, so maybe we want to not fix clone for
now and only
fix it when computing the submodule url.
So I'll first fix up the test suite to not rely on buggy behavior and
then send this patch
with no change in tests? That sounds strange to me as it hides away the cause.
^ permalink raw reply
* Re: [PATCH] rev-list: use hdr_termination instead of a always using a newline
From: Junio C Hamano @ 2016-10-20 18:54 UTC (permalink / raw)
To: Jacob Keller; +Cc: git, Dennis Kaarsemaker, Jacob Keller
In-Reply-To: <20161020181930.21084-1-jacob.e.keller@intel.com>
Jacob Keller <jacob.e.keller@intel.com> writes:
> diff --git a/t/t6000-rev-list-misc.sh b/t/t6000-rev-list-misc.sh
> index 3e752ce03280..e8c6979baf59 100755
> --- a/t/t6000-rev-list-misc.sh
> +++ b/t/t6000-rev-list-misc.sh
> @@ -4,6 +4,12 @@ test_description='miscellaneous rev-list tests'
>
> . ./test-lib.sh
>
> +test_ends_with_nul() {
> + printf "\0" >nul
> + sed '$!d' "$@" >contents
> + test_cmp_bin nul contents
> +}
> +
> test_expect_success setup '
> echo content1 >wanted_file &&
> echo content2 >unwanted_file &&
> @@ -100,4 +106,9 @@ test_expect_success '--bisect and --first-parent can not be combined' '
> test_must_fail git rev-list --bisect --first-parent HEAD
> '
>
> +test_expect_success '--header shows a NUL after each commit' '
> + git rev-list --header --max-count=1 HEAD | sed \$!d >actual &&
> + test_ends_with_nul actual
> +'
> +
> test_done
Thanks.
The main part of the patch looks good. For "passing NUL to sed",
I'd probably work it around like so:
t/t6000-rev-list-misc.sh | 19 +++++++++++--------
1 file changed, 11 insertions(+), 8 deletions(-)
diff --git a/t/t6000-rev-list-misc.sh b/t/t6000-rev-list-misc.sh
index e8c6979baf..737026c34f 100755
--- a/t/t6000-rev-list-misc.sh
+++ b/t/t6000-rev-list-misc.sh
@@ -4,12 +4,6 @@ test_description='miscellaneous rev-list tests'
. ./test-lib.sh
-test_ends_with_nul() {
- printf "\0" >nul
- sed '$!d' "$@" >contents
- test_cmp_bin nul contents
-}
-
test_expect_success setup '
echo content1 >wanted_file &&
echo content2 >unwanted_file &&
@@ -107,8 +101,17 @@ test_expect_success '--bisect and --first-parent can not be combined' '
'
test_expect_success '--header shows a NUL after each commit' '
- git rev-list --header --max-count=1 HEAD | sed \$!d >actual &&
- test_ends_with_nul actual
+ # We know there is no Q in the true payload; names and
+ # addresses of the authors and the committers do not have
+ # any, and object names or header names do not, either.
+ git rev-list --header --max-count=2 HEAD |
+ nul_to_q |
+ grep "^Q" >actual &&
+ cat >expect <<-EOF &&
+ Q$(git rev-parse HEAD~1)
+ Q
+ EOF
+ test_cmp expect actual
'
test_done
^ permalink raw reply related
* Re: [PATCH] rev-list: use hdr_termination instead of a always using a newline
From: Dennis Kaarsemaker @ 2016-10-20 18:49 UTC (permalink / raw)
To: Jacob Keller, git; +Cc: Junio C Hamano, Jacob Keller
In-Reply-To: <20161020181930.21084-1-jacob.e.keller@intel.com>
On Thu, 2016-10-20 at 11:19 -0700, Jacob Keller wrote:
> Here's my solution, with an updated test using a helper function based
> on using sed (which I think is more portable than tail -n1 ?). The
> change actually is very simple. I ran the test suite and it appears to
> be not breaking anyone else since the normal case is
> hdr_termination="\n" except in the cases where it needs to be NUL.
>
> Thanks for the bug report!
I like both improvements, and both 'make test' and gitweb are happy
with this version. Thanks for the quick fix.
D.
^ permalink raw reply
* Re: [PATCH] rev-list: restore the NUL commit separator in --header mode
From: Junio C Hamano @ 2016-10-20 18:46 UTC (permalink / raw)
To: Jacob Keller
Cc: Dennis Kaarsemaker, Git mailing list, Jacob Keller, Stefan Beller,
Jeff King, Johannes Sixt
In-Reply-To: <CA+P7+xq5bo-Fwa95j3aynjMP0Qw+PiuMt=hc4ngvTDpeG8nhPw@mail.gmail.com>
Jacob Keller <jacob.keller@gmail.com> writes:
> I did some searching, and we do use sed so I replaced it with sed \$!d
> which appears to work. I think we should probably implement a
> test_ends_with_nul or something.
As it is "a stream editor that shall read one or more text files", I
do not think "sed" is any better (or any worse) than "tail -n" from
the portability point of view. They both may happen to work on GNU
systems.
^ permalink raw reply
* Re: tools for easily "uncommitting" parts of a patch I just commited?
From: Junio C Hamano @ 2016-10-20 18:41 UTC (permalink / raw)
To: Jacob Keller; +Cc: Jeff King, Git mailing list
In-Reply-To: <CA+P7+xqbqfCTOHJZJ9ZGXv3oeSvKUdJByc8mdzoAoXRd7UZ6HA@mail.gmail.com>
Jacob Keller <jacob.keller@gmail.com> writes:
>> I am not sure if that is OK. I think it is less not-OK than the use
>> case I mentioned in my earlier message, in that this is not a case
>> that "please don't do it" breaks. It however is an inconvenience
>> that the user has to say "git add file" before the "git commit" (or
>> "git commit file") to conclude the sequence.
>>
>> So I dunno.
>
> Hmmm.. Ya ok I don't think we can actually distinguish between these
> two work flows.
What we might want to have in "git commit <paths>" is a new mode
that is totally different from -i/-o that says roughly "Start from
the tree of HEAD, pretend as if you removed all the paths that match
the given pathspec from the tree, and then added all the entries in
the index that match that pathspec. Write that tree and commit.
Take nothing from the working tree". I have a feeling that when
people do
$ git add -p file1 file2 file3
$ git commit file2
and ends up including _all_ changes made to file2, not just the ones
they picked in the earlier part of the workflow, they are expecting
such a behaviour.
^ permalink raw reply
* [PATCH] rev-list: use hdr_termination instead of a always using a newline
From: Jacob Keller @ 2016-10-20 18:19 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Dennis Kaarsemaker, Jacob Keller
From: Jacob Keller <jacob.keller@gmail.com>
When adding support for prefixing output of log and other commands using
--line-prefix, commit 660e113ce118 ("graph: add support for
--line-prefix on all graph-aware output", 2016-08-31) accidentally
broke rev-list --header output.
In order to make the output appear with a line-prefix, the flow was
changed to always use the graph subsystem for display. Unfortunately
the graph flow in rev-list did not use info->hdr_termination as it was
assumed that graph output would never need to putput NULs.
Since we now always use the graph code in order to handle the case of
line-prefix, simply replace putchar('\n') with
putchar(info->hdr_termination) which will correct this issue.
Add a test for the --header case to make sure we don't break it in the
future. Implement a helper function test_ends_with_nul() to make it more
obvious what sort of check we are looking for.
Reported-by: Dennis Kaarsemaker <dennis@kaarsemaker.net>
Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
---
Here's my solution, with an updated test using a helper function based
on using sed (which I think is more portable than tail -n1 ?). The
change actually is very simple. I ran the test suite and it appears to
be not breaking anyone else since the normal case is
hdr_termination="\n" except in the cases where it needs to be NUL.
Thanks for the bug report!
builtin/rev-list.c | 2 +-
t/t6000-rev-list-misc.sh | 11 +++++++++++
2 files changed, 12 insertions(+), 1 deletion(-)
diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index 8479f6ed28aa..c43decda7011 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -145,7 +145,7 @@ static void show_commit(struct commit *commit, void *data)
*/
if (buf.len && buf.buf[buf.len - 1] == '\n')
graph_show_padding(revs->graph);
- putchar('\n');
+ putchar(info->hdr_termination);
} else {
/*
* If the message buffer is empty, just show
diff --git a/t/t6000-rev-list-misc.sh b/t/t6000-rev-list-misc.sh
index 3e752ce03280..e8c6979baf59 100755
--- a/t/t6000-rev-list-misc.sh
+++ b/t/t6000-rev-list-misc.sh
@@ -4,6 +4,12 @@ test_description='miscellaneous rev-list tests'
. ./test-lib.sh
+test_ends_with_nul() {
+ printf "\0" >nul
+ sed '$!d' "$@" >contents
+ test_cmp_bin nul contents
+}
+
test_expect_success setup '
echo content1 >wanted_file &&
echo content2 >unwanted_file &&
@@ -100,4 +106,9 @@ test_expect_success '--bisect and --first-parent can not be combined' '
test_must_fail git rev-list --bisect --first-parent HEAD
'
+test_expect_success '--header shows a NUL after each commit' '
+ git rev-list --header --max-count=1 HEAD | sed \$!d >actual &&
+ test_ends_with_nul actual
+'
+
test_done
--
2.10.0.560.g867c144
^ permalink raw reply related
* Re: tools for easily "uncommitting" parts of a patch I just commited?
From: Jacob Keller @ 2016-10-20 18:13 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, Git mailing list
In-Reply-To: <xmqqwph2hqdg.fsf@gitster.mtv.corp.google.com>
On Thu, Oct 20, 2016 at 10:39 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Jacob Keller <jacob.keller@gmail.com> writes:
>
>> I still think we're misunderstanding. I want git commit to complain
>> *only* under the following circumstance:
>>
>> I run "git add -p" and put a partial change into the index in <file>.
>> There are still other parts which were not added to the index yet.
>> Thus, the index version of the file and the actual file differ.
>>
>> Then, I (accidentally) run "git commit <file>"
>
> I agree that this case is different.
>
> Again, users are different, and I also often do
>
> $ edit file; think; decide it is a good enough first cut
> $ git add file
> $ edit file; think; decide it is getting better
> $ git add file
> $ edit file; think; decide it is now perfect
> $ git commit file
>
> Because I do not think you can differentiate the above workflow from
> the case where "git add -p" was used earlier, I think your updated
> "git commit" needs to complain at this point.
>
> I am not sure if that is OK. I think it is less not-OK than the use
> case I mentioned in my earlier message, in that this is not a case
> that "please don't do it" breaks. It however is an inconvenience
> that the user has to say "git add file" before the "git commit" (or
> "git commit file") to conclude the sequence.
>
> So I dunno.
Hmmm.. Ya ok I don't think we can actually distinguish between these
two work flows.
Given that I now know how to fix my mistake easily (git reset -p) I
think I will just go ahead and not bother with this as it's much less
of a pain now.
Thanks,
Jake
^ permalink raw reply
* Re: Fwd: New Defects reported by Coverity Scan for git
From: Stefan Beller @ 2016-10-20 18:13 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git@vger.kernel.org, Jeff King
In-Reply-To: <xmqqoa2ehp5u.fsf@gitster.mtv.corp.google.com>
On Thu, Oct 20, 2016 at 11:05 AM, Junio C Hamano <gitster@pobox.com> wrote:
>
> Good to know that you have been managing it; I was mostly worried
> about not having anybody managing it (i.e. imagining Coverity
> nominated/volunteered me as manager with everybody else as viewers)
> and the new viewer requests get totally ignored by the project as
> the whole.
No, I have been paying attention, but in case I suddenly stop contributing
to the git project I thought it's better to have a couple of people there.
>
>> I see that some of these emails may be inconvenient to you, I can
>> change your role to defect viewer/contributor if you prefer.
>
> It is not a huge inconvenience to me, because any piece of e-mail
> that is addressed directly to gitster@ without CC'ing to git@vger
> and is not a follow-up to any earlier message goes to a separate
> lowest-priority mailbox that I rarely look at. But if it is easy
> to recategorize me, please do so.
done.
^ permalink raw reply
* Re: [PATCH] rev-list: restore the NUL commit separator in --header mode
From: Jacob Keller @ 2016-10-20 18:12 UTC (permalink / raw)
To: Dennis Kaarsemaker
Cc: Junio C Hamano, Git mailing list, Jacob Keller, Stefan Beller,
Jeff King, Johannes Sixt
In-Reply-To: <1476986671.28685.5.camel@kaarsemaker.net>
On Thu, Oct 20, 2016 at 11:04 AM, Dennis Kaarsemaker
<dennis@kaarsemaker.net> wrote:
> On Wed, 2016-10-19 at 15:41 -0700, Junio C Hamano wrote:
>> Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:
>>
>> > + touch expect &&
>> > + printf "\0" > expect &&
>>
>>
>> What's the point of that "touch", especially if you are going to
>> overwrite it immediately after?
>
> Leftover debugging crud. I tried various ways of generating an
> actual/expect to compare.
>
>> > + git rev-list --header --max-count=1 HEAD | tail -n1 >actual &&
>>
>>
>> As "tail" is a tool for text files, it is likely unportable to use
>> "tail -n1" to grab the "last incomplete line that happens to contain
>> a single NUL".
>>
>> > + test_cmp_bin expect actual
>> > +'
>
> Yeah, I was fearing that. I didn't find anything in the testsuite that
> helps answering the question "does this file end with a NUL" and would
> appreciate a hint :)
>
> D.
I did some searching, and we do use sed so I replaced it with sed \$!d
which appears to work. I think we should probably implement a
test_ends_with_nul or something.
Thanks,
Jake
^ permalink raw reply
* [PATCH] commit parsing: replace unchecked parse_commit by parse_commit_or_die
From: Stefan Beller @ 2016-10-20 18:06 UTC (permalink / raw)
To: gitster; +Cc: git, Stefan Beller
In-Reply-To: <xmqqoa2ehp5u.fsf@gitster.mtv.corp.google.com>
The reason parse_commit() would fail at these points would be because
the repository is corrupt.
This was noticed by coverity.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
developed on pu as that's where coverity spotted it.
I have no overview if these areas are being worked on. (It may clash
with at least jc/merge-base-fp-only)
Thanks,
Stefan
builtin/blame.c | 2 +-
builtin/describe.c | 4 ++--
builtin/name-rev.c | 2 +-
builtin/show-branch.c | 4 ++--
commit.c | 2 +-
fetch-pack.c | 2 +-
6 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/builtin/blame.c b/builtin/blame.c
index 992a79c..3b8564c 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -1801,7 +1801,7 @@ static void assign_blame(struct scoreboard *sb, int opt)
* so hold onto it in the meantime.
*/
origin_incref(suspect);
- parse_commit(commit);
+ parse_commit_or_die(commit);
if (reverse ||
(!(commit->object.flags & UNINTERESTING) &&
!(revs->max_age != -1 && commit->date < revs->max_age)))
diff --git a/builtin/describe.c b/builtin/describe.c
index 01490a1..8299b16 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -199,7 +199,7 @@ static unsigned long finish_depth_computation(
best->depth++;
while (parents) {
struct commit *p = parents->item;
- parse_commit(p);
+ parse_commit_or_die(p);
if (!(p->object.flags & SEEN))
commit_list_insert_by_date(p, list);
p->object.flags |= c->object.flags;
@@ -322,7 +322,7 @@ static void describe(const char *arg, int last_one)
}
while (parents) {
struct commit *p = parents->item;
- parse_commit(p);
+ parse_commit_or_die(p);
if (!(p->object.flags & SEEN))
commit_list_insert_by_date(p, &list);
p->object.flags |= c->object.flags;
diff --git a/builtin/name-rev.c b/builtin/name-rev.c
index cd89d48..92c3316 100644
--- a/builtin/name-rev.c
+++ b/builtin/name-rev.c
@@ -29,7 +29,7 @@ static void name_rev(struct commit *commit,
struct commit_list *parents;
int parent_number = 1;
- parse_commit(commit);
+ parse_commit_or_die(commit);
if (commit->date < cutoff)
return;
diff --git a/builtin/show-branch.c b/builtin/show-branch.c
index 974f340..fd911b5 100644
--- a/builtin/show-branch.c
+++ b/builtin/show-branch.c
@@ -218,7 +218,7 @@ static void join_revs(struct commit_list **list_p,
parents = parents->next;
if ((this_flag & flags) == flags)
continue;
- parse_commit(p);
+ parse_commit_or_die(p);
if (mark_seen(p, seen_p) && !still_interesting)
extra--;
p->object.flags |= flags;
@@ -835,7 +835,7 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
if (!commit)
die(_("cannot find commit %s (%s)"),
ref_name[num_rev], oid_to_hex(&revkey));
- parse_commit(commit);
+ parse_commit_or_die(commit);
mark_seen(commit, &seen);
/* rev#0 uses bit REV_SHIFT, rev#1 uses bit REV_SHIFT+1,
diff --git a/commit.c b/commit.c
index b9c0c81..5b23eaf 100644
--- a/commit.c
+++ b/commit.c
@@ -910,7 +910,7 @@ static void mark_redundant(struct commit **array, int cnt)
ALLOC_ARRAY(filled_index, cnt - 1);
for (i = 0; i < cnt; i++)
- parse_commit(array[i]);
+ parse_commit_or_die(array[i]);
for (i = 0; i < cnt; i++) {
struct commit_list *common;
diff --git a/fetch-pack.c b/fetch-pack.c
index cb45c34..8b4ab47 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -159,7 +159,7 @@ static const unsigned char *get_rev(void)
return NULL;
commit = prio_queue_get(&rev_list);
- parse_commit(commit);
+ parse_commit_or_die(commit);
parents = commit->parents;
commit->object.flags |= POPPED;
--
2.10.1.448.g862ec83.dirty
^ permalink raw reply related
* Re: Fwd: New Defects reported by Coverity Scan for git
From: Junio C Hamano @ 2016-10-20 18:05 UTC (permalink / raw)
To: Stefan Beller; +Cc: git@vger.kernel.org, Jeff King
In-Reply-To: <CAGZ79kYTwsWy+oxnPKghWKGLEaO9qWZbrQOOejeEBOe0c-o0Pg@mail.gmail.com>
Stefan Beller <sbeller@google.com> writes:
> I do it most of the time, but I did not start managing it.
> And I have been pretty lax/liberal about handing out rights to do stuff,
> because I did not want to tip on anyone's toes giving to few rights
> and thereby annoying them.
Good to know that you have been managing it; I was mostly worried
about not having anybody managing it (i.e. imagining Coverity
nominated/volunteered me as manager with everybody else as viewers)
and the new viewer requests get totally ignored by the project as
the whole.
> I see that some of these emails may be inconvenient to you, I can
> change your role to defect viewer/contributor if you prefer.
It is not a huge inconvenience to me, because any piece of e-mail
that is addressed directly to gitster@ without CC'ing to git@vger
and is not a follow-up to any earlier message goes to a separate
lowest-priority mailbox that I rarely look at. But if it is easy
to recategorize me, please do so.
Thanks.
^ permalink raw reply
* Re: [PATCH] rev-list: restore the NUL commit separator in --header mode
From: Dennis Kaarsemaker @ 2016-10-20 18:04 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, jacob.e.keller, stefanbeller, peff, j6t, jacob.keller
In-Reply-To: <xmqq4m48j70o.fsf@gitster.mtv.corp.google.com>
On Wed, 2016-10-19 at 15:41 -0700, Junio C Hamano wrote:
> Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:
>
> > + touch expect &&
> > + printf "\0" > expect &&
>
>
> What's the point of that "touch", especially if you are going to
> overwrite it immediately after?
Leftover debugging crud. I tried various ways of generating an
actual/expect to compare.
> > + git rev-list --header --max-count=1 HEAD | tail -n1 >actual &&
>
>
> As "tail" is a tool for text files, it is likely unportable to use
> "tail -n1" to grab the "last incomplete line that happens to contain
> a single NUL".
>
> > + test_cmp_bin expect actual
> > +'
Yeah, I was fearing that. I didn't find anything in the testsuite that
helps answering the question "does this file end with a NUL" and would
appreciate a hint :)
D.
^ permalink raw reply
* Re: [PATCH] rev-list: restore the NUL commit separator in --header mode
From: Dennis Kaarsemaker @ 2016-10-20 18:02 UTC (permalink / raw)
To: Junio C Hamano
Cc: git@vger.kernel.org, j6t@kdbg.org, peff@peff.net,
stefanbeller@gmail.com
In-Reply-To: <xmqq8ttkj740.fsf@gitster.mtv.corp.google.com>
On Wed, 2016-10-19 at 15:39 -0700, Junio C Hamano wrote:
> Jacob Keller <jacob.keller@gmail.com> writes:
>
> > Hi,
> >
> > On Wed, Oct 19, 2016 at 2:04 PM, Dennis Kaarsemaker
> > <dennis@kaarsemaker.net> wrote:
> > > Commit 660e113 (graph: add support for --line-prefix on all graph-aware
> > > output) changed the way commits were shown. Unfortunately this dropped
> > > the NUL between commits in --header mode. Restore the NUL and add a test
> > > for this feature.
> > >
> >
> >
> > Oops! Thanks for the bug fix.
> >
> > > Signed-off-by: Dennis Kaarsemaker <dennis@kaarsemaker.net>
> > > ---
> > > builtin/rev-list.c | 4 ++++
> > > t/t6000-rev-list-misc.sh | 7 +++++++
> > > 2 files changed, 11 insertions(+)
> > >
> > > diff --git a/builtin/rev-list.c b/builtin/rev-list.c
> > > index 8479f6e..cfa6a7d 100644
> > > --- a/builtin/rev-list.c
> > > +++ b/builtin/rev-list.c
> > > @@ -157,6 +157,10 @@ static void show_commit(struct commit *commit, void *data)
> > > if (revs->commit_format == CMIT_FMT_ONELINE)
> > > putchar('\n');
> > > }
> > > + if (revs->commit_format == CMIT_FMT_RAW) {
> > > + putchar(info->hdr_termination);
> > > + }
> > > +
> >
> >
> > This seems right to me. My one concern is that we make sure we restore
> > it for every case (in case it needs to be there for other formats?)
> > I'm not entirely sure about whether other non-raw modes need this or
> > not?
>
>
> Right. The original didn't do anything special for CMIT_FMT_RAW,
> and 660e113 did not remove anything special for CMIT_FMT_RAW, so it
> isn't immediately obvious why this patch is sufficient.
>
> Dennis, care to elaborate?
The original logic was (best seen with git show -w 660e113):
if(showing graphs) {
do pretty things
}
else {
just print the buffer and the header terminator
}
660e113 changed that to
do pretty things
Given that the 'do pretty things part' works for other uses of git rev-
list, it made sense that the \0 should only be added back in
CMIT_FMT_RAW mode. Changing the first putchar('\n') as Jacob proposes
(that mail arrived while I'm typing this) might work too, I haven't
tested it.
D.
^ permalink raw reply
* Re: Fwd: New Defects reported by Coverity Scan for git
From: Stefan Beller @ 2016-10-20 17:58 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git@vger.kernel.org, Jeff King
In-Reply-To: <xmqqshrqhpua.fsf@gitster.mtv.corp.google.com>
On Thu, Oct 20, 2016 at 10:50 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> Not sure what triggered the new finding of coverity as seen below as the
>> parse_commit() was not touched. Junios series regarding the merge base
>> optimization touches a bit of code nearby though.
>>
>> Do we want to replace the unchecked places of parse_commit with
>> parse_commit_or_die ?
>
> The reason parse_commit() would fail at this point would be because
> the repository is corrupt, I do not think it would hurt to do such a
> change.
>
> I agree that it is curious why it shows up as a "new defect",
> though.
>
> By the way, do you know who is managing the service on our end
> (e.g. approving new people to be "defect viewer")?
I do it most of the time, but I did not start managing it.
And I have been pretty lax/liberal about handing out rights to do stuff,
because I did not want to tip on anyone's toes giving to few rights
and thereby annoying them.
I see that some of these emails may be inconvenient to you, I can
change your role to defect viewer/contributor if you prefer.
Thanks,
Stefan
^ permalink raw reply
* Re: Fwd: New Defects reported by Coverity Scan for git
From: Junio C Hamano @ 2016-10-20 17:50 UTC (permalink / raw)
To: Stefan Beller; +Cc: git@vger.kernel.org, Jeff King
In-Reply-To: <CAGZ79kabVPhp0_z-e_4jJOFq+jzSE2SsgmFuY-2RUgrEviGKyA@mail.gmail.com>
Stefan Beller <sbeller@google.com> writes:
> Not sure what triggered the new finding of coverity as seen below as the
> parse_commit() was not touched. Junios series regarding the merge base
> optimization touches a bit of code nearby though.
>
> Do we want to replace the unchecked places of parse_commit with
> parse_commit_or_die ?
The reason parse_commit() would fail at this point would be because
the repository is corrupt, I do not think it would hurt to do such a
change.
I agree that it is curious why it shows up as a "new defect",
though.
By the way, do you know who is managing the service on our end
(e.g. approving new people to be "defect viewer")? The site seems
to think I have the power to manage others' subscription, which I do
not think I have (I do not go to the site myself). As it spewed
quite a many false positives into my mailbox in the past, I do not
pay very close attention to these reports these days, but I still
read the e-mailed reports every once in a while.
Thanks.
^ permalink raw reply
* Re: tools for easily "uncommitting" parts of a patch I just commited?
From: Junio C Hamano @ 2016-10-20 17:39 UTC (permalink / raw)
To: Jacob Keller; +Cc: Jeff King, Git mailing list
In-Reply-To: <CA+P7+xq25LcdmtzmBNChiGhGratcdp7m0EOsQuEh68=gJQ9HNQ@mail.gmail.com>
Jacob Keller <jacob.keller@gmail.com> writes:
> I still think we're misunderstanding. I want git commit to complain
> *only* under the following circumstance:
>
> I run "git add -p" and put a partial change into the index in <file>.
> There are still other parts which were not added to the index yet.
> Thus, the index version of the file and the actual file differ.
>
> Then, I (accidentally) run "git commit <file>"
I agree that this case is different.
Again, users are different, and I also often do
$ edit file; think; decide it is a good enough first cut
$ git add file
$ edit file; think; decide it is getting better
$ git add file
$ edit file; think; decide it is now perfect
$ git commit file
Because I do not think you can differentiate the above workflow from
the case where "git add -p" was used earlier, I think your updated
"git commit" needs to complain at this point.
I am not sure if that is OK. I think it is less not-OK than the use
case I mentioned in my earlier message, in that this is not a case
that "please don't do it" breaks. It however is an inconvenience
that the user has to say "git add file" before the "git commit" (or
"git commit file") to conclude the sequence.
So I dunno.
^ permalink raw reply
* Re: tools for easily "uncommitting" parts of a patch I just commited?
From: Jacob Keller @ 2016-10-20 17:27 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, Git mailing list
In-Reply-To: <xmqqa8dzhtki.fsf@gitster.mtv.corp.google.com>
On Thu, Oct 20, 2016 at 9:30 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Jeff King <peff@peff.net> writes:
>
>>> I still think it's worth while to add a check for git-commit which
>>> does something like check when we say "git commit <files>" and if the
>>> index already has those files marked as being changed, compare them
>>> with the current contents of the file as in the checkout and quick
>>> saying "please don't do that" so as to avoid the problem in the first
>>> place.
>> ...
>> I suspect both of those would complain about legitimate workflows.
>>
>> I dunno. I do not ever use "git commit <file>" myself.
>
> Users are different. I do use this all the time, and it is not
> unusual at all to have changed contents on paths other than <file>
> already added to the index when I do so, i.e. an unrelated small
> typofix in <file> jumping ahead of the real changes I am working on
> in other parts of the tree.
>
> "Please don't do that" would break. Jacob says "avoid the problem",
> but I do not see a problem in allowing it (it could be that the
> problem Jacob has is in other parts of his workflow, but I do not
> know what it is offhand).
I still think we're misunderstanding. I want git commit to complain
*only* under the following circumstance:
I run "git add -p" and put a partial change into the index in <file>.
There are still other parts which were not added to the index yet.
Thus, the index version of the file and the actual file differ.
Then, I (accidentally) run "git commit <file>"
I want git commit to complain here that the index <file> and acutal
<file> being requested are different and it thinks there's an issue.
I do *NOT* want it to complain if I do "git add -p" and put parts of
<other-file> into the index, and then run
git commit <file>
Does that make sense?
Basically if the index and "git commit <file>" both say "add <file>"
but they conflict in what version of <file> I want it to go "hey..
uhhh.. that's a bad idea"
Thanks,
Jake
^ permalink raw reply
* Re: Drastic jump in the time required for the test suite
From: Matthieu Moy @ 2016-10-20 17:13 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Schindelin, git
In-Reply-To: <xmqqmvhzhu6f.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> Are you proposing to replace the tests written as shell scripts with
> scripts in another language or framework that run equivalent
> sequences of git commands that is as portable as, if not more,
> Bourne shell?
The language (/bin/sh) is probably not the biggest issue. The way we use
it may be.
I don't have benchmark to tell what slows down the testsuite, but for
example we often write
cat >expected <<EOF &&
content
EOF
git foo >actual &&
test_cmp expected actual
We create 2 files, fork one 'cat' and one 'diff'. Now, suppose we can
write instead
test_check_output git foo <<EOF
content
EOF
where test_check_output would be a compiled program running "git foo"
with popen(), reading the expected output on stdin, and comparing both
(and possibly creating files and forking 'diff', but only on the error
path). That would reduce the overhead of test a lot: on fork+exec to
create the "test_check_output" process + plain string comparison.
Obviously, I don't expect this change to drastically reduce the time
(Peff's numbers seem to suggest that there's not so much to gain), but
maybe getting a few C helpers and "optimized" idioms in our testsuite
could lead to measurable improvements.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply
* Re: [PATCH v4 05/14] i18n: add--interactive: mark plural strings
From: Junio C Hamano @ 2016-10-20 17:11 UTC (permalink / raw)
To: Vasco Almeida
Cc: git, Jiang Xin, Ævar Arnfjörð Bjarmason,
Jean-Noël AVILA, Jakub Narębski, David Aguilar
In-Reply-To: <1476981335.14459.19.camel@sapo.pt>
Vasco Almeida <vascomalmeida@sapo.pt> writes:
> A Seg, 10-10-2016 às 12:54 +0000, Vasco Almeida escreveu:
>> @@ -70,6 +72,8 @@ Git::I18N - Perl interface to Git's Gettext localizations
>>
>> printf __("The following error occurred: %s\n"), $error;
>>
>> + printf __n("commited %d file", "commited %d files", $files), $files;
>> +
>
> I forgot to add \n to this example as suggested in
> <xmqqoa2ymnb1.fsf@gitster.mtv.corp.google.com>
>
> What should I do? Should I wait for more reviews and then send a new
> re-roll fixing this?
You fix it up locally not to forget, in case you need a reroll, and
wait for more reviews. In the meantime, I'll also fix it up locally
not to forget ;-) That way, if it turns out that this round is good
enough to be the final version, people will see my fixup, and if what
I have needs to be replaced with your new version, your fixup will
be in there.
Thanks.
^ permalink raw reply
* Fwd: New Defects reported by Coverity Scan for git
From: Stefan Beller @ 2016-10-20 17:05 UTC (permalink / raw)
To: git@vger.kernel.org, Junio C Hamano, Jeff King
In-Reply-To: <580893d5a4736_4ed37b53181837@ss1435.mail>
Not sure what triggered the new finding of coverity as seen below as the
parse_commit() was not touched. Junios series regarding the merge base
optimization touches a bit of code nearby though.
Do we want to replace the unchecked places of parse_commit with
parse_commit_or_die ?
Thanks,
Stefan
_________________________________________________________
*** CID 1374088: Error handling issues (CHECKED_RETURN)
/commit.c: 913 in mark_redundant()
907
908 work = xcalloc(cnt, sizeof(*work));
909 redundant = xcalloc(cnt, 1);
910 ALLOC_ARRAY(filled_index, cnt - 1);
911
912 for (i = 0; i < cnt; i++)
>>> CID 1374088: Error handling issues (CHECKED_RETURN)
>>> Calling "parse_commit" without checking return value (as is done elsewhere 37 out of 45 times).
913 parse_commit(array[i]);
914 for (i = 0; i < cnt; i++) {
915 struct commit_list *common;
916
917 if (redundant[i])
918 continue;
^ permalink raw reply
* Re: [PATCH 2/2] tag: send fully qualified refnames to verify_tag_and_format
From: Santiago Torres @ 2016-10-20 16:57 UTC (permalink / raw)
To: Jeff King; +Cc: git, gitster, sunshine, walters, Lukas Puehringer
In-Reply-To: <20161019203943.epjxnfci7vcqg4xv@sigill.intra.peff.net>
[-- Attachment #1: Type: text/plain, Size: 1762 bytes --]
On Wed, Oct 19, 2016 at 04:39:44PM -0400, Jeff King wrote:
> The ref-filter code generally expects to see fully qualified
> refs, so that things like "%(refname)" and "%(refname:short)"
> work as expected. We can do so easily from git-tag, which
> always works with refnames in the refs/tags namespace. As a
> bonus, we can drop the "kind" parameter from
> pretty_print_ref() and just deduce it automatically.
>
> Unfortunately, things are not so simple for verify-tag,
> which takes an arbitrary sha1 expression. It has no clue if
> a refname as used or not, and whether it was in the
> refs/tags namespace.
>
> In an ideal world, get_sha1_with_context() would optionally
> tell us about any refs we resolved while it was working, and
> we could just feed that refname (and then in cases where we
> didn't use a ref at all, like a bare sha1, we could fallback
> to just showing the sha1 name the user gave us).
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> I think you'd really just squash the various bits of this into your
> series at the right spots, though I don't mind it on top, either.
>
> The big question is to what degree we should care about the verify-tag
> case. I don't think it's any worse off with this change than it is with
> your series (its "kind" becomes "OTHER", but I don't think that is
> actually used for display at all; the name remains the same). I'd be OK
> with leaving it like this, as a known bug, until get_sha1_with_context()
> learns to tell us about the ref. It's an unhandled corner case in a
> brand-new feature, not a regression in an existing one.
I see now, I think I can sprinkle some of these changes on 2/7 then. The
rest should be doing 4/7 and 5/7. Does this sound ok?
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply
* Re: Drastic jump in the time required for the test suite
From: René Scharfe @ 2016-10-20 16:40 UTC (permalink / raw)
To: Duy Nguyen, Johannes Schindelin; +Cc: Junio C Hamano, Git Mailing List
In-Reply-To: <CACsJy8ALRjnvgM6sW5ioFHtquSR6zgQHkQ6-LUnKFnqXV9eDCQ@mail.gmail.com>
Am 20.10.2016 um 13:02 schrieb Duy Nguyen:
> On Wed, Oct 19, 2016 at 4:18 PM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
>> Hi Junio,
>>
>> I know you are a fan of testing things thoroughly in the test suite, but I
>> have to say that it is getting out of hand, in particular due to our
>> over-use of shell script idioms (which really only run fast on Linux, not
>> a good idea for a portable software).
>>
>> My builds of `pu` now time out, after running for 3h straight in the VM
>> dedicated to perform the daily routine of building and testing the git.git
>> branches in Git for Windows' SDK. For comparison, `next` passes build &
>> tests in 2.6h. That is quite the jump.
>
> I'm just curious, will running git.exe from WSL [1] help speed things
> up a bit (or, hopefully, a lot)? I'm assuming that shell's speed in
> WSL is quite fast.
>
> I'm pretty sure the test suite would need some adaptation, but if the
> speedup is significant, maybe it's worth spending time on.
>
> [1] https://news.ycombinator.com/item?id=12748395
I get this on WSL with prove -j8:
Files=750, Tests=13657, 906 wallclock secs ( 8.51 usr 17.17 sys + 282.62 cusr 3731.85 csys = 4040.15 CPU)
And this for a run on Debian inside a Hyper-V VM on the same system:
Files=759, Tests=13895, 99 wallclock secs ( 4.81 usr 1.06 sys + 39.70 cusr 25.82 csys = 71.39 CPU)
All tests pass on master.
René
^ 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