* [regression] `make profile-install` fails in 2.10.1
From: Jan Keromnes @ 2016-10-19 11:15 UTC (permalink / raw)
To: git
Hello,
TL;DR - Probably a regression of a previously reported bug. [0]
I'm trying to `profile-install` Git from source on Ubuntu 16.04, to
have the latest stable Git optimized for my machine.
However, this often fails, because the profile build runs all Git
tests (to get an idea of how Git behaves on my hardware, and optimize
for it), but it bails out if there are any test failures (for me, this
has happened on most Git version upgrades this year, see also [0] and
[1]).
- Problem: Is there a way to `make profile-install` but ignore
occasional test failures, as these are not critical to get a useful
hardware profile? (Note: In a previous thread, Dennis Kaarsemaker
mentioned this is fixing a symptom, not the root cause, but it would
still be great to get a working profile in spite of occasional test
failures.)
- Related problem: `t3700-add.sh` fails again in 2.10.1 for me. More
details below, and I can provide further debug information if you
don't already know the problem.
Thanks,
Jan
[0] "`make profile-install` fails in 2.9.3" -
https://marc.info/?l=git&m=147274608823171&w=2
[1] "Fwd: Git 2.8.1 fails test 32 of t7300-clean.sh, breaks profile
build" - https://marc.info/?l=git&m=146193443529229&w=2
---
Steps to reproduce:
curl https://www.kernel.org/pub/software/scm/git/git-2.10.1.tar.xz
| tar xJ \
&& cd git-2.10.1 \
&& make prefix=/usr profile-install install-man -j18
Expected result:
- runs all tests to get a profile (ignoring occasional failures)
- rebuilds Git with the profile
- installs Git
Actual result:
- runs all tests to get a profile
- at least one test fails, interrupting the whole process
- Git is not installed
Failure log:
(snip)
*** t3700-add.sh ***
ok 1 - Test of git add
ok 2 - Post-check that foo is in the index
ok 3 - Test that "git add -- -q" works
ok 4 - git add: Test that executable bit is not used if core.filemode=0
ok 5 - git add: filemode=0 should not get confused by symlink
(snip)
ok 38 - git add --chmod=[+-]x stages correctly
ok 39 - git add --chmod=+x with symlinks
not ok 40 - git add --chmod=[+-]x changes index with already added file
#
# echo foo >foo3 &&
# git add foo3 &&
# git add --chmod=+x foo3 &&
# test_mode_in_index 100755 foo3 &&
# echo foo >xfoo3 &&
# chmod 755 xfoo3 &&
# git add xfoo3 &&
# git add --chmod=-x xfoo3 &&
# test_mode_in_index 100644 xfoo3
#
ok 41 - git add --chmod=[+-]x does not change the working tree
ok 42 - no file status change if no pathspec is given
ok 43 - no file status change if no pathspec is given in subdir
ok 44 - all statuses changed in folder if . is given
# failed 1 among 44 test(s)
1..44
Makefile:43: recipe for target 't3700-add.sh' failed
make[3]: Leaving directory '/tmp/git/git-2.10.1/t'
make[3]: *** [t3700-add.sh] Error 1
Makefile:36: recipe for target 'test' failed
make[2]: Leaving directory '/tmp/git/git-2.10.1/t'
make[2]: *** [test] Error 2
Makefile:2273: recipe for target 'test' failed
make[1]: *** [test] Error 2
make[1]: Leaving directory '/tmp/git/git-2.10.1'
Makefile:1679: recipe for target 'profile' failed
make: *** [profile] Error 2
The command '/bin/sh -c mkdir /tmp/git && cd /tmp/git && curl
https://www.kernel.org/pub/software/scm/git/git-2.10.1.tar.xz | tar xJ
&& cd git-2.10.1 && make prefix=/usr profile-install install-man
-j18 && rm -rf /tmp/git' returned a non-zero code: 2
^ permalink raw reply
* Re: Integrating submodules with no side effects
From: Robert Dailey @ 2016-10-19 13:27 UTC (permalink / raw)
To: Stefan Beller; +Cc: Git
In-Reply-To: <CAGZ79kaFtzQDZrSJhJZ59xvBzn+6+UVDO65Ac+T6aFxMQaM_xQ@mail.gmail.com>
On Tue, Oct 18, 2016 at 4:17 PM, Stefan Beller <sbeller@google.com> wrote:
> On Tue, Oct 18, 2016 at 12:35 PM, Robert Dailey
> <rcdailey.lists@gmail.com> wrote:
>> Hello git experts,
>>
>> I have in the past attempted to integrate submodules into my primary
>> repository using the same directory name. However, this has always
>> caused headache when going to and from branches that take you between
>> when this integration occurred and when it didn't. It's a bit hard to
>> explain. Basically, if I have a submodule "foo", and I delete that
>> submodule and physically add its files under the same directory "foo",
>> when I do a pull to get this change from another clone, it fails
>> saying:
>>
>> error: The following untracked working tree files would be overwritten
>> by checkout:
>> foo/somefile.txt
>> Please move or remove them before you switch branches.
>> Aborting
>> could not detach HEAD
>>
>>
>> Obviously, git can't delete the submodule because the files have also
>> been added directly. I don't think it is built to handle this
>> scenario. Here is the series of commands I ran to "integrate" the
>> submodule (replace the submodule with a directory containing the exact
>> contents of the submodule itself):
>>
>> #!/usr/bin/env bash
>> mv "$1" "${1}_"
>> git submodule deinit "$1"
>
> This removes the submodule entries from .git/config
> (and it would remove the contents of that submodule, but they are moved)
>
>> git rm "$1"
>
> Removing the git link here.
>
> So we still have the entries in the .gitmodules file there.
> Maybe add:
>
> name=$(git submodule-helper name $1)
> git config -f .gitmodules --unset submodule.$name.*
> git add .gitmodules
>
> ? (Could be optional)
Actually I verified that it seems `git rm` is specialized for
submodules somewhere, because when I run that command on a submodule
the relevant entries in the .gitmodules file are removed. I did not
have to do this as a separate step.
>> mv "${1}_" "$1"
>> git add "$1/**"
>
> Moving back into place and adding all files in there.
>
>>
>> The above script is named git-integrate-submodule, I run it like so:
>>
>> $ git integrate-submodule foo
>>
>> Then I do:
>>
>> $ git commit -m 'Integrated foo submodule'
>>
>> Is there any way to make this work nicely?
>
> I think you can just remove the gitlink from the index and not from the working
> tree ("git rm --cached $1")
What is the goal of doing it this way? What does this simplify?
>> The only solution I've
>> found is to obviously rename the directory before adding the physical
>> files, for example name it foo1. Because they're different, they never
>> "clash".
>
> Also look at the difference between plumbing and porcelain commands[1],
> as plumbing is more stable than the porcelain, so it will be easier to maintain
> this script.
Which plumbing commands did you have in mind?
> I think this would be an actually reasonable feature, which Git itself
> could support via "git submodule [de]integrate", but then we'd also want
> to see the reverse, i.e. take a sub directory and make it a submodule.
Integrating this as a feature might be fine, I think when you bring up
the question of retaining history makes things much harder.
Fortunately for me that is not a requirement in this case, so I'm able
to do things with much less effort.
However the primary purpose of my post was to find out how to
integrate the submodule without the error on next pull by other
collaborators of my repository. It's a real pain to recover your
working copy when going inbetween commits where the submodule
integration happened inbetween them. I did quote the exact error
message I got in my original post.
Do you have any info on how I can prevent that error? Ideally I want
the integration to go smoothly and transparently, not just for the
person doing the actual transition (me) but for everyone else that
gets those changes from upstream. They should not even notice that it
happened (i.e. no failed commands, awkward behavior, or manual steps).
^ permalink raw reply
* Re: git checkout crashes after server being updated to Debian X86_64
From: Duy Nguyen @ 2016-10-19 13:27 UTC (permalink / raw)
To: Raffael Reichelt; +Cc: Git Mailing List
In-Reply-To: <6B2BE996-F696-4EB4-91CA-849D40B8802D@gmail.com>
On Tue, Oct 18, 2016 at 10:17 PM, Raffael Reichelt
<raffael.reichelt@gmail.com> wrote:
> Hello!
>
> I have a serious problem with git, After my provider had updated to a X86_64 architecture git crashes with various memory-related errors. This is happening remote when pushing to the repository from my local machine as well as trying it on a shell on the server itself.
>
> This are the error-messages:
>
> fatal: Out of memory, realloc failed
> fatal: recursion detected in die handler
> fatal: recursion detected in die handler
You other mail said memory is capped at 600MB, which should be a lot
for normal repositories. If you set the environment variable
GIT_ALLOC_LIMIT to maybe 500MB or lower (convert it to kilobytes
first) and git attempts to allocate more than that (just that one
time, not total mem) then it's caught and we get a glimpse of how much
memory git may need. Unfortunately we can't get a stack trace or
anything like that unless you rebuild Git from source.
> or
> fatal: unable to create threaded lstat
> fatal: recursion detected in die handler
Hmm.. with "max user processes (-u) 42" we should be fine because we
only create 20 threads max. What happens if you set core.preloadindex
to false? Can it run until the end or hit some other fatal errors?
There's room for improvement in preload_index(). If we hit resource
limit like this, it's not the end of the world and we should be able
to keep going. But threaded lstat has been available for a long time
and this is the first time I see a report like this, not sure if it's
worth fixing.
--
Duy
^ permalink raw reply
* Re: What's cooking in git.git (Oct 2016, #04; Mon, 17)
From: Jeff King @ 2016-10-19 7:46 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Lars Schneider, git
In-Reply-To: <xmqq4m49pffk.fsf@gitster.mtv.corp.google.com>
On Tue, Oct 18, 2016 at 01:31:27PM -0700, Junio C Hamano wrote:
> >> * ls/filter-process (2016-10-17) 14 commits
> [...]
> > what do you think about v11? Do you feel the series is becoming mature
> > enough for `next`?
>
> I've already had that feeling a few rounds ago, but I haven't had a
> chance to read the most recent one carefully myself to answer that
> question honestly.
FWIW, I gave it a fairly thorough read-over (something I'd been meaning
to do for quite a while, but kept never quite getting around to). I
think overall it is OK for next. I did find one or two nits, but I think
they are things we can fix up in-tree if and when they become a problem
(e.g., I noticed that test-genrandom gets piped to "perl -pe". I'm not
sure if perl will complain about funny multibyte characters on some
systems. I suggest we ignore it until somebody demonstrates that it
actually matters).
-Peff
^ permalink raw reply
* [ANNOUNCE] Git Rev News edition 20
From: Christian Couder @ 2016-10-19 14:13 UTC (permalink / raw)
To: git
Cc: Thomas Ferris Nicolaisen, Jakub Narebski, Junio C Hamano, lwn,
Dennis Kaarsemaker, Johannes Schindelin, Jeff King, Kevin Daudt,
Ramkumar Ramachandra, Stephan Beyer, Johannes Sixt, Eric Sunshine,
Philip Oakley, Andrew Johnson, Stefan Beller, Linus Torvalds,
Lars Schneider, Fredrik Gustafsson, Rich Felker, Kyle J. McKay
Hi everyone,
I'm happy announce that the 20th edition of Git Rev News is now published:
https://git.github.io/rev_news/2016/10/19/edition-20/
Thanks a lot to all the contributors and helpers, especially Jakub,
Dennis, Dscho, Lars and Peff!
Enjoy,
Christian and Thomas.
^ permalink raw reply
* Re: [PATCH] daemon, path.c: fix a bug with ~ in repo paths
From: Duy Nguyen @ 2016-10-19 14:12 UTC (permalink / raw)
To: Luke Shumaker; +Cc: Junio C Hamano, Git Mailing List, Jeff King
In-Reply-To: <87pomxildc.wl-lukeshu@sbcglobal.net>
On Wed, Oct 19, 2016 at 1:05 AM, Luke Shumaker <lukeshu@sbcglobal.net> wrote:
>> I am not sure if it is even a bug. As you can easily lose that
>> tilde that appears in front of subdirectory of /srv/git/ or replace
>> it with something else (e.g. "u/"), this smells like "Don't do it if
>> it hurts" thing to me.
>
> I buy into "Don't do it if it hurts", but that doesn't mean it's not a
> bug on an uncommon edge-case.
The amount of changes is unbelievable for fixing such a rare case
though. I wonder if we can just detect this in daemon.c and pass
"./~foo/bar" instead of "~foo/bar" to enter_repo() in non-strict mode
to "disable" expand_user_path(). If it works, it's much simpler
changes (even though a bit hacky)
--
Duy
^ permalink raw reply
* Re: git checkout crashes after server being updated to Debian X86_64
From: Raffael Reichelt @ 2016-10-19 14:05 UTC (permalink / raw)
To: Duy Nguyen; +Cc: Git Mailing List
In-Reply-To: <CACsJy8B50daiHWfu7zfVQnn_i_=HbUK3gBPmv1U=EUw7ZyuGPw@mail.gmail.com>
> Am 19.10.2016 um 15:27 schrieb Duy Nguyen <pclouds@gmail.com>:
>
> On Tue, Oct 18, 2016 at 10:17 PM, Raffael Reichelt
> <raffael.reichelt@gmail.com> wrote:
>> Hello!
>>
>> I have a serious problem with git, After my provider had updated to a X86_64 architecture git crashes with various memory-related errors. This is happening remote when pushing to the repository from my local machine as well as trying it on a shell on the server itself.
>>
>> This are the error-messages:
>>
>> fatal: Out of memory, realloc failed
>> fatal: recursion detected in die handler
>> fatal: recursion detected in die handler
>
> You other mail said memory is capped at 600MB, which should be a lot
> for normal repositories. If you set the environment variable
> GIT_ALLOC_LIMIT to maybe 500MB or lower (convert it to kilobytes
> first) and git attempts to allocate more than that (just that one
> time, not total mem) then it's caught and we get a glimpse of how much
> memory git may need. Unfortunately we can't get a stack trace or
> anything like that unless you rebuild Git from source.
This was no change: crashed with the same errors …
>
>> or
>> fatal: unable to create threaded lstat
>> fatal: recursion detected in die handler
>
> Hmm.. with "max user processes (-u) 42" we should be fine because we
> only create 20 threads max. What happens if you set core.preloadindex
> to false? Can it run until the end or hit some other fatal errors?
>
This did the trick :) I just repeatedly did a forced checkout and it went until the end without errors
THX a lot!
Raffael
^ permalink raw reply
* Re: [PATCH v4 4/7] builtin/verify-tag: add --format to verify-tag
From: Jeff King @ 2016-10-19 9:04 UTC (permalink / raw)
To: santiago; +Cc: git, gitster, sunshine, walters
In-Reply-To: <20161007210721.20437-5-santiago@nyu.edu>
On Fri, Oct 07, 2016 at 05:07:18PM -0400, santiago@nyu.edu wrote:
> @@ -46,12 +50,17 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix)
> if (verbose)
> flags |= GPG_VERIFY_VERBOSE;
>
> + if (fmt_pretty) {
> + verify_ref_format(fmt_pretty);
> + flags |= GPG_VERIFY_QUIET;
> + }
I see why you would want to disable the normal output when there is a
custom format. But it seems a shame that the GPG information cannot be
retrieved as part of that format.
I think in the long run we'd want something like pretty.c's "%G"
placeholders for the ref-filter pretty-printer. But I think we are OK to
do this patch without it. It allows at least:
tag=v2.0.0
got=$(git verify-tag --format='%(tag)' "$tag") &&
test "$tag" = "$got" ||
echo >&2 "verification failed"
which is better than what we have now, and can be extended in the
future.
-Peff
^ permalink raw reply
* [PATCH 7/7] merge: allow to use only the fp-only merge bases
From: Junio C Hamano @ 2016-10-19 4:23 UTC (permalink / raw)
To: git
In-Reply-To: <20161019042345.29766-1-gitster@pobox.com>
Teach "git merge" a new option "--fp-base-only" that tells it to
consider only merge bases that are on the first-parent chain.
This speeds up back-merges needed in the topic branch workflow. The
merge of 'master' back into 'next' used as an example in the RFD
article <xmqqmvi2sj8f.fsf@gitster.mtv.corp.google.com>, i.e.
git checkout 4868def05e && git merge 659889482a
in our history takes about 1.22-1.33 seconds without the series,
while running the latter "git merge" with the "--fp-base-only"
option takes about 0.54-0.59 seconds.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
Documentation/merge-options.txt | 9 +++++++++
builtin/merge.c | 15 ++++++++++++---
2 files changed, 21 insertions(+), 3 deletions(-)
diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt
index 5b4a62e936..7927f069e4 100644
--- a/Documentation/merge-options.txt
+++ b/Documentation/merge-options.txt
@@ -116,6 +116,15 @@ ifndef::git-pull[]
Note that not all merge strategies may support progress
reporting.
+--fp-base-only::
+ Instead of using all merge bases when computing the
+ three-way merge result, use only the merge bases on the
+ first-parent chain of the commits being merged. This
+ experimental feature is meant to be used when merging an
+ older integration branch back to a newer integration branch
+ in the topic-branch workflow.
+
+
endif::git-pull[]
--allow-unrelated-histories::
diff --git a/builtin/merge.c b/builtin/merge.c
index 0ae099f746..a38b878e61 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -88,6 +88,8 @@ enum ff_type {
static enum ff_type fast_forward = FF_ALLOW;
+static int fp_base_only;
+
static int option_parse_message(const struct option *opt,
const char *arg, int unset)
{
@@ -210,6 +212,8 @@ static struct option builtin_merge_options[] = {
{ OPTION_SET_INT, 0, "ff-only", &fast_forward, NULL,
N_("abort if fast-forward is not possible"),
PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, FF_ONLY },
+ OPT_BOOL(0, "fp-base-only", &fp_base_only,
+ N_("use only merge bases on first-parent chain")),
OPT_RERERE_AUTOUPDATE(&allow_rerere_auto),
OPT_BOOL(0, "verify-signatures", &verify_signatures,
N_("verify that the named commit has a valid GPG signature")),
@@ -1340,9 +1344,14 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
if (!remoteheads)
; /* already up-to-date */
- else if (!remoteheads->next)
- common = get_merge_bases(head_commit, remoteheads->item);
- else {
+ else if (!remoteheads->next) {
+ unsigned flags = MB_POSTCLEAN;
+ if (fp_base_only)
+ flags |= MB_FPCHAIN;
+ common = get_merge_bases_opt(head_commit,
+ 1, &remoteheads->item,
+ flags);
+ } else {
struct commit_list *list = remoteheads;
commit_list_insert(head_commit, &list);
common = get_octopus_merge_bases(list);
--
2.10.1-631-gb2c64dcf30
^ permalink raw reply related
* [PATCH 6/7] merge-base: limit the output to bases that are on first-parent chain
From: Junio C Hamano @ 2016-10-19 4:23 UTC (permalink / raw)
To: git
In-Reply-To: <20161019042345.29766-1-gitster@pobox.com>
A new option "--fp-only" limits the output to the merge bases that
are on the first-parent chain from the branches being merged.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
Documentation/git-merge-base.txt | 8 +++++++-
builtin/merge-base.c | 10 +++++++---
2 files changed, 14 insertions(+), 4 deletions(-)
diff --git a/Documentation/git-merge-base.txt b/Documentation/git-merge-base.txt
index 808426faac..f99653f138 100644
--- a/Documentation/git-merge-base.txt
+++ b/Documentation/git-merge-base.txt
@@ -9,7 +9,7 @@ git-merge-base - Find as good common ancestors as possible for a merge
SYNOPSIS
--------
[verse]
-'git merge-base' [-a|--all] <commit> <commit>...
+'git merge-base' [-a|--all] [--fp-only] <commit> <commit>...
'git merge-base' [-a|--all] --octopus <commit>...
'git merge-base' --is-ancestor <commit> <commit>
'git merge-base' --independent <commit>...
@@ -72,6 +72,12 @@ OPTIONS
--all::
Output all merge bases for the commits, instead of just one.
+--fp-only::
+ Limit the output to merge bases that are on the first-parent
+ chain from none of the commits given on the command line.
+
+
+
DISCUSSION
----------
diff --git a/builtin/merge-base.c b/builtin/merge-base.c
index c0d1822eb3..42febb20ff 100644
--- a/builtin/merge-base.c
+++ b/builtin/merge-base.c
@@ -6,11 +6,12 @@
#include "revision.h"
#include "parse-options.h"
-static int show_merge_base(struct commit **rev, int rev_nr, int show_all)
+static int show_merge_base(struct commit **rev, int rev_nr, int show_all, int fp_only)
{
struct commit_list *result;
+ unsigned flags = fp_only ? MB_FPCHAIN : 0;
- result = get_merge_bases_many_dirty(rev[0], rev_nr - 1, rev + 1);
+ result = get_merge_bases_opt(rev[0], rev_nr - 1, rev + 1, flags);
if (!result)
return 1;
@@ -208,10 +209,13 @@ int cmd_merge_base(int argc, const char **argv, const char *prefix)
struct commit **rev;
int rev_nr = 0;
int show_all = 0;
+ int fp_only = 0;
int cmdmode = 0;
struct option options[] = {
OPT_BOOL('a', "all", &show_all, N_("output all common ancestors")),
+ OPT_BOOL(0, "fp-only", &fp_only,
+ N_("limit to bases that are on first-parent chain")),
OPT_CMDMODE(0, "octopus", &cmdmode,
N_("find ancestors for a single n-way merge"), 'o'),
OPT_CMDMODE(0, "independent", &cmdmode,
@@ -255,5 +259,5 @@ int cmd_merge_base(int argc, const char **argv, const char *prefix)
ALLOC_ARRAY(rev, argc);
while (argc-- > 0)
rev[rev_nr++] = get_commit_reference(*argv++);
- return show_merge_base(rev, rev_nr, show_all);
+ return show_merge_base(rev, rev_nr, show_all, fp_only);
}
--
2.10.1-631-gb2c64dcf30
^ permalink raw reply related
* [PATCH 5/7] merge-base: mark bases that are on first-parent chain
From: Junio C Hamano @ 2016-10-19 4:23 UTC (permalink / raw)
To: git
In-Reply-To: <20161019042345.29766-1-gitster@pobox.com>
In a workflow where topic branches are first merged to the 'next'
integration branch to be tested before getting merged down to the
'master' integration branch to be consumed by the end users, merging
the 'master' branch back to the 'next' happens after topics graduate
to 'master' and release notes entries are written for them.
Git finds many merge bases between 'master' and 'next' while
creating this merge. In addition to the tip of 'master' back when
we made such a merge back from 'master' to 'next' was made the last
time, which is the most reasonable merge base to explain the
histories of both branches, all the tips of topic branches that
graduated recently are merge bases. Because these tips of topic
branches were already in 'next', the tip of 'next' reaches them, and
because they just graduated to 'master', the tip of 'master' reaches
them, too. And these topics are independent most of the time (that
is the point of employing the topic-branch workflow), so they cannot
be reduced.
The merge-recursive machinery is very inefficient to compute this
merge, because it needs to create pointless virtual merge-base
commits across these many merge bases. Conceptually, the point
where the histories of 'master' and 'next' diverged was the tip of
'master' back when we created such a merge back from 'master' to
'next' the last time, and in practice that is the only merge base
that matters.
Update the logic in paint_down_to_common() so that it can mark if a
merge-base commit was reached by following the first-parent chain of
either side of the merge to find such commit, and give an option to
get_merge_bases_opt() to return only merge-base commits that are on
the first-parent chain, so that a later step of this series can
introduce a command line option to enable it and avoid feeding
useless merge bases to the merge machinery.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
commit.c | 17 +++++++++++++++--
commit.h | 1 +
object.h | 2 +-
3 files changed, 17 insertions(+), 3 deletions(-)
diff --git a/commit.c b/commit.c
index 92d23b1082..8e90531b05 100644
--- a/commit.c
+++ b/commit.c
@@ -765,8 +765,9 @@ void sort_in_topological_order(struct commit_list **list, enum rev_sort_order so
#define PARENT2 (1u<<17)
#define STALE (1u<<18)
#define RESULT (1u<<19)
+#define FPCHAIN (1u<<20)
-static const unsigned all_flags = (PARENT1 | PARENT2 | STALE | RESULT);
+static const unsigned all_flags = (PARENT1 | PARENT2 | STALE | RESULT | FPCHAIN);
static int queue_has_nonstale(struct prio_queue *queue)
{
@@ -802,6 +803,7 @@ static struct commit_list *paint_down_to_common(struct commit *one, int n, struc
struct commit *commit = prio_queue_get(&queue);
struct commit_list *parents;
int flags;
+ int nth_parent = 0;
flags = commit->object.flags & (PARENT1 | PARENT2 | STALE);
if (flags == (PARENT1 | PARENT2)) {
@@ -816,11 +818,14 @@ static struct commit_list *paint_down_to_common(struct commit *one, int n, struc
while (parents) {
struct commit *p = parents->item;
parents = parents->next;
+ nth_parent++;
if ((p->object.flags & flags) == flags)
continue;
if (parse_commit(p))
return NULL;
p->object.flags |= flags;
+ if (nth_parent == 1)
+ p->object.flags |= FPCHAIN;
prio_queue_put(&queue, p);
}
}
@@ -951,6 +956,8 @@ struct commit_list *get_merge_bases_opt(struct commit *one,
struct commit_list *result;
int cnt, i;
int cleanup = !!(flags & MB_POSTCLEAN);
+ int fpchain = !!(flags & MB_FPCHAIN);
+ char *on_fpchain;
result = merge_bases_many(one, n, twos);
@@ -975,21 +982,27 @@ struct commit_list *get_merge_bases_opt(struct commit *one,
/* There are more than one */
cnt = commit_list_count(result);
rslt = xcalloc(cnt, sizeof(*rslt));
+ on_fpchain = xcalloc(cnt, sizeof(*on_fpchain));
for (list = result, i = 0; list; list = list->next)
rslt[i++] = list->item;
free_commit_list(result);
+ for (i = 0; i < cnt; i++)
+ on_fpchain[i] = !!(rslt[i]->object.flags & FPCHAIN);
+
clear_commit_marks(one, all_flags);
clear_commit_marks_many(n, twos, all_flags);
mark_redundant(rslt, cnt);
result = NULL;
for (i = 0; i < cnt; i++)
- if (!(rslt[i]->object.flags & STALE))
+ if (!(rslt[i]->object.flags & STALE) &&
+ (!fpchain || on_fpchain[i]))
commit_list_insert_by_date(rslt[i], &result);
else
rslt[i]->object.flags &= ~STALE;
free(rslt);
+ free(on_fpchain);
return result;
}
diff --git a/commit.h b/commit.h
index 557f2814b7..ec6e09a985 100644
--- a/commit.h
+++ b/commit.h
@@ -254,6 +254,7 @@ extern struct commit_list *get_merge_bases_many(struct commit *one, int n, struc
extern struct commit_list *get_octopus_merge_bases(struct commit_list *in);
#define MB_POSTCLEAN 01
+#define MB_FPCHAIN 02
extern struct commit_list *get_merge_bases_opt(struct commit *one, int n, struct commit **twos, unsigned flags);
#define get_merge_bases_many_dirty(one, n, twos) get_merge_bases_opt((one),(n),(twos),MB_POSTCLEAN)
diff --git a/object.h b/object.h
index f8e218eccd..7e6729158a 100644
--- a/object.h
+++ b/object.h
@@ -36,7 +36,7 @@ struct object_array {
* bisect.c: 16
* bundle.c: 16
* http-push.c: 16-----19
- * commit.c: 16-----19
+ * commit.c: 16-------20
*/
#define FLAG_BITS 27
--
2.10.1-631-gb2c64dcf30
^ permalink raw reply related
* [PATCH 4/7] merge-base: expose get_merge_bases_many_0() a bit more
From: Junio C Hamano @ 2016-10-19 4:23 UTC (permalink / raw)
To: git
In-Reply-To: <20161019042345.29766-1-gitster@pobox.com>
"git merge-base" names its internal workhorse helper function
"get_merge_bases_many_0()", which takes one "can we get away without
clearing the object->flags bits because we know we are the last
caller?" parameter. Make the parameter into a flags word to make it
extensible and rename it to get_merge_bases_opt(). Use it to turn
get_merge_bases_many_dirty() wrapper into a C-preprocessor macro.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
commit.c | 19 ++++++-------------
commit.h | 6 ++++--
2 files changed, 10 insertions(+), 15 deletions(-)
diff --git a/commit.c b/commit.c
index 59bd18e67c..92d23b1082 100644
--- a/commit.c
+++ b/commit.c
@@ -942,15 +942,15 @@ static void mark_redundant(struct commit **array, int cnt)
free(filled_index);
}
-static struct commit_list *get_merge_bases_many_0(struct commit *one,
- int n,
- struct commit **twos,
- int cleanup)
+struct commit_list *get_merge_bases_opt(struct commit *one,
+ int n, struct commit **twos,
+ unsigned flags)
{
struct commit_list *list;
struct commit **rslt;
struct commit_list *result;
int cnt, i;
+ int cleanup = !!(flags & MB_POSTCLEAN);
result = merge_bases_many(one, n, twos);
@@ -997,19 +997,12 @@ struct commit_list *get_merge_bases_many(struct commit *one,
int n,
struct commit **twos)
{
- return get_merge_bases_many_0(one, n, twos, 1);
-}
-
-struct commit_list *get_merge_bases_many_dirty(struct commit *one,
- int n,
- struct commit **twos)
-{
- return get_merge_bases_many_0(one, n, twos, 0);
+ return get_merge_bases_opt(one, n, twos, 0);
}
struct commit_list *get_merge_bases(struct commit *one, struct commit *two)
{
- return get_merge_bases_many_0(one, 1, &two, 1);
+ return get_merge_bases_opt(one, 1, &two, MB_POSTCLEAN);
}
/*
diff --git a/commit.h b/commit.h
index 32e1a113e5..557f2814b7 100644
--- a/commit.h
+++ b/commit.h
@@ -253,8 +253,10 @@ extern struct commit_list *get_merge_bases(struct commit *rev1, struct commit *r
extern struct commit_list *get_merge_bases_many(struct commit *one, int n, struct commit **twos);
extern struct commit_list *get_octopus_merge_bases(struct commit_list *in);
-/* To be used only when object flags after this call no longer matter */
-extern struct commit_list *get_merge_bases_many_dirty(struct commit *one, int n, struct commit **twos);
+#define MB_POSTCLEAN 01
+extern struct commit_list *get_merge_bases_opt(struct commit *one, int n, struct commit **twos, unsigned flags);
+
+#define get_merge_bases_many_dirty(one, n, twos) get_merge_bases_opt((one),(n),(twos),MB_POSTCLEAN)
/* largest positive number a signed 32-bit integer can contain */
#define INFINITE_DEPTH 0x7fffffff
--
2.10.1-631-gb2c64dcf30
^ permalink raw reply related
* [PATCH 3/7] merge-base: stop moving commits around in remove_redundant()
From: Junio C Hamano @ 2016-10-19 4:23 UTC (permalink / raw)
To: git
In-Reply-To: <20161019042345.29766-1-gitster@pobox.com>
The merge-base computation is performed in two steps. First,
paint_down_to_common() traverses the history to find all possible
merge bases (and more), and then remove_redundant() checks the
result and culls the ones that can be reached from another commit
in the result.
The latter received an array of commits, and then returned the same
array after reordering the elements in it, moving the surviving ones
to the front and returning the number of surviving ones.
This arrangement works, but it makes it cumbersome for the callers
when they want to see the array's contents intact (e.g. the caller
may want to keep an additional per-commit data in an independent
array that parallels the array of commits).
Stop moving commits around in the array, and instead mark the ones
that are not merge bases with the STALE bit in their object->flags
bitword.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
commit.c | 32 ++++++++++++++++++--------------
1 file changed, 18 insertions(+), 14 deletions(-)
diff --git a/commit.c b/commit.c
index 6266c0380c..59bd18e67c 100644
--- a/commit.c
+++ b/commit.c
@@ -888,11 +888,11 @@ struct commit_list *get_octopus_merge_bases(struct commit_list *in)
return ret;
}
-static int remove_redundant(struct commit **array, int cnt)
+static void mark_redundant(struct commit **array, int cnt)
{
/*
* Some commit in the array may be an ancestor of
- * another commit. Move such commit to the end of
+ * another commit. Mark such commit as STALE in
* the array, and return the number of commits that
* are independent from each other.
*/
@@ -930,18 +930,16 @@ static int remove_redundant(struct commit **array, int cnt)
free_commit_list(common);
}
- /* Now collect the result */
- COPY_ARRAY(work, array, cnt);
- for (i = filled = 0; i < cnt; i++)
- if (!redundant[i])
- array[filled++] = work[i];
- for (j = filled, i = 0; i < cnt; i++)
+ /* Mark the result */
+ for (i = 0; i < cnt; i++)
if (redundant[i])
- array[j++] = work[i];
+ array[i]->object.flags |= STALE;
+ else
+ array[i]->object.flags &= ~STALE;
+
free(work);
free(redundant);
free(filled_index);
- return filled;
}
static struct commit_list *get_merge_bases_many_0(struct commit *one,
@@ -984,10 +982,13 @@ static struct commit_list *get_merge_bases_many_0(struct commit *one,
clear_commit_marks(one, all_flags);
clear_commit_marks_many(n, twos, all_flags);
- cnt = remove_redundant(rslt, cnt);
+ mark_redundant(rslt, cnt);
result = NULL;
for (i = 0; i < cnt; i++)
- commit_list_insert_by_date(rslt[i], &result);
+ if (!(rslt[i]->object.flags & STALE))
+ commit_list_insert_by_date(rslt[i], &result);
+ else
+ rslt[i]->object.flags &= ~STALE;
free(rslt);
return result;
}
@@ -1086,9 +1087,12 @@ struct commit_list *reduce_heads(struct commit_list *heads)
p->item->object.flags &= ~STALE;
}
}
- num_head = remove_redundant(array, num_head);
+ mark_redundant(array, num_head);
for (i = 0; i < num_head; i++)
- tail = &commit_list_insert(array[i], tail)->next;
+ if (!(array[i]->object.flags & STALE))
+ tail = &commit_list_insert(array[i], tail)->next;
+ else
+ array[i]->object.flags &= ~STALE;
return result;
}
--
2.10.1-631-gb2c64dcf30
^ permalink raw reply related
* [PATCH 2/7] sha1_name: remove ONELINE_SEEN bit
From: Junio C Hamano @ 2016-10-19 4:23 UTC (permalink / raw)
To: git
In-Reply-To: <20161019042345.29766-1-gitster@pobox.com>
28a4d94044 ("object name: introduce ':/<oneline prefix>' notation",
2007-02-24) started using its own bit from object->flags to mark
commits used while parsing the ":/token" extended SHA-1 syntax to
name a commit temporarily, and this was kept even when f7bff00314
("sha1_name.c: fix parsing of ":/token" syntax", 2010-08-02) found
and fixed a bug in its implementation.
The use of that flag bit, however, is limited to a single function,
get_sha1_oneline(), which first sets it for the commits sitting at
the tips of refs, uses the bit to avoid duplicate traversal while
walking the history, and then cleans the bit from all commits it
walked.
Which is exactly what the general-purpose TMP_MARK bit meant to be
used for isolated case was invented for. Replace ONELINE_SEEN with
TMP_MARK and retire the former.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
object.h | 1 -
sha1_name.c | 10 ++++------
2 files changed, 4 insertions(+), 7 deletions(-)
diff --git a/object.h b/object.h
index f8b644263f..f8e218eccd 100644
--- a/object.h
+++ b/object.h
@@ -37,7 +37,6 @@ struct object_array {
* bundle.c: 16
* http-push.c: 16-----19
* commit.c: 16-----19
- * sha1_name.c: 20
*/
#define FLAG_BITS 27
diff --git a/sha1_name.c b/sha1_name.c
index ca7ddd6f2c..fa0e6701a3 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -7,6 +7,7 @@
#include "refs.h"
#include "remote.h"
#include "dir.h"
+#include "revision.h"
static int get_sha1_oneline(const char *, unsigned char *, struct commit_list *);
@@ -855,9 +856,6 @@ static int get_sha1_1(const char *name, int len, unsigned char *sha1, unsigned l
* For future extension, all other sequences beginning with ':/!' are reserved.
*/
-/* Remember to update object flag allocation in object.h */
-#define ONELINE_SEEN (1u<<20)
-
static int handle_one_ref(const char *path, const struct object_id *oid,
int flag, void *cb_data)
{
@@ -899,7 +897,7 @@ static int get_sha1_oneline(const char *prefix, unsigned char *sha1,
return -1;
for (l = list; l; l = l->next) {
- l->item->object.flags |= ONELINE_SEEN;
+ l->item->object.flags |= TMP_MARK;
commit_list_insert(l->item, &backup);
}
while (list) {
@@ -907,7 +905,7 @@ static int get_sha1_oneline(const char *prefix, unsigned char *sha1,
struct commit *commit;
int matches;
- commit = pop_most_recent_commit(&list, ONELINE_SEEN);
+ commit = pop_most_recent_commit(&list, TMP_MARK);
if (!parse_object(commit->object.oid.hash))
continue;
buf = get_commit_buffer(commit, NULL);
@@ -924,7 +922,7 @@ static int get_sha1_oneline(const char *prefix, unsigned char *sha1,
regfree(®ex);
free_commit_list(list);
for (l = backup; l; l = l->next)
- clear_commit_marks(l->item, ONELINE_SEEN);
+ clear_commit_marks(l->item, TMP_MARK);
free_commit_list(backup);
return found ? 0 : -1;
}
--
2.10.1-631-gb2c64dcf30
^ permalink raw reply related
* [PATCH 1/7] commit: simplify fastpath of merge-base computation
From: Junio C Hamano @ 2016-10-19 4:23 UTC (permalink / raw)
To: git
In-Reply-To: <20161019042345.29766-1-gitster@pobox.com>
The get_merge_bases_many*() family of functions first call
merge_bases_many() to find all possible merge bases between a single
commit "one" and a set of other commits "twos[]". Because this
typically involves traversing the commit graph, which uses the
object flags on the commits involved, the function needs to clear
the flags before it returns.
Except for one special case. If "one" is exactly one of the "twos",
"one" is the merge base and merge_bases_many() returns a list of
merge bases with a single element, "one", on it, without smudging
the object flags of the commits at all.
We used to use a loop over "twos[]" array to see if this fast-path
would have kicked in in merge_bases_many(), in which case we can
return without cleaning the bits at all. We can do the same by
inspecting the result in a more direct way, which is conceptually
cleaner.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
commit.c | 15 +++++++++++----
1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/commit.c b/commit.c
index aada266f9a..6266c0380c 100644
--- a/commit.c
+++ b/commit.c
@@ -955,10 +955,17 @@ static struct commit_list *get_merge_bases_many_0(struct commit *one,
int cnt, i;
result = merge_bases_many(one, n, twos);
- for (i = 0; i < n; i++) {
- if (one == twos[i])
- return result;
- }
+
+ /*
+ * The fast-path of 'one' being the merge-base; there is no
+ * need to clean the object flags in this case.
+ */
+ if (result && !result->next &&
+ result->item == one &&
+ !(one->object.flags & RESULT))
+ return result;
+
+ /* If we didn't get any, or there is only one, we are done */
if (!result || !result->next) {
if (cleanup) {
clear_commit_marks(one, all_flags);
--
2.10.1-631-gb2c64dcf30
^ permalink raw reply related
* [PATCH 0/7] Rejecting useless merge bases
From: Junio C Hamano @ 2016-10-19 4:23 UTC (permalink / raw)
To: git
In-Reply-To: <xmqqmvi2sj8f.fsf@gitster.mtv.corp.google.com>
This is a continuation of
http://public-inbox.org/git/xmqqmvi2sj8f.fsf@gitster.mtv.corp.google.com
In a workflow where topic branches are first merged to the 'next'
integration branch to be tested before getting merged down to the
'master' integration branch to be consumed by the end users, merging
the 'master' branch back to the 'next' happens after topics graduate
to 'master' and release notes entries are written for them.
Git finds many merge bases between 'master' and 'next' while
creating this merge. In addition to the tip of 'master' back when
we made such a merge back from 'master' to 'next' was made the last
time, which is the most reasonable merge base to explain the
histories of both branches, all the tips of topic branches that
graduated recently are merge bases. Because these tips of topic
branches were already in 'next', the tip of 'next' reaches them, and
because they just graduated to 'master', the tip of 'master' reaches
them, too. And these topics are independent most of the time (that
is the point of employing the topic-branch workflow), so they cannot
be reduced.
The merge-recursive machinery is very inefficient to compute this
merge, because it needs to create pointless virtual merge-base
commits across these many merge bases. Conceptually, the point
where the histories of 'master' and 'next' diverged was the tip of
'master' back when we created such a merge back from 'master' to
'next' the last time, and in practice that is the only merge base
that matters.
The series allows us to ignore these tips of topics, which are
uninteresting merge bases, when running "git merge". The example
merge with 12 merge bases:
git checkout 4868def05e && git merge 659889482a
in our history takes about 1.22-1.33 seconds without the series,
while running the latter "git merge" with the "--fp-base-only"
option takes about 0.54-0.59 seconds.
Junio C Hamano (7):
commit: simplify fastpath of merge-base
sha1_name: remove ONELINE_SEEN bit
merge-base: stop moving commits around in remove_redundant()
merge-base: expose get_merge_bases_many_0() a bit more
merge-base: mark bases that are on first-parent chain
merge-base: limit the output to bases that are on first-parent chain
merge: allow to use only the fp-only merge bases
Documentation/git-merge-base.txt | 8 +++-
Documentation/merge-options.txt | 9 +++++
builtin/merge-base.c | 10 +++--
builtin/merge.c | 15 ++++++--
commit.c | 81 ++++++++++++++++++++++++----------------
commit.h | 7 +++-
object.h | 3 +-
sha1_name.c | 10 ++---
8 files changed, 94 insertions(+), 49 deletions(-)
--
2.10.1-631-gb2c64dcf30
^ permalink raw reply
* Re: [PATCHv3] submodule--helper: normalize funny urls
From: Junio C Hamano @ 2016-10-19 2:05 UTC (permalink / raw)
To: Stefan Beller
Cc: Johannes Sixt, Johannes Schindelin, git@vger.kernel.org, Karl A.,
Dennis Kaarsemaker, Jonathan Nieder
In-Reply-To: <CAGZ79kYrKGLEOO72aWuX5OOM-AecdFZFXRqBkRzhdAM-VbPFxA@mail.gmail.com>
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.
^ permalink raw reply
* Re: [PATCH v4 05/25] sequencer: eventually release memory allocated for the option values
From: Junio C Hamano @ 2016-10-19 1:12 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Jakub Narębski, Johannes Sixt, Ramsay Jones
In-Reply-To: <alpine.DEB.2.20.1610181356460.197091@virtualbox>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>> > To remedy that, we now take custody of the option values in question,
>> > requiring those values to be malloc()ed or strdup()ed
>>
>> That is the approach this patch takes, so "eventually release" in
>> the title is no longer accurate, I would think.
>
> To the contrary, we now free() things in remove_state(), so we still
> "eventually release" the memory.
OK. We call a change to teach remove_state() to free the resource
does more commonly as "plug leaks"; the word "eventually" gave me an
impression that we are emphasizing the fact that we do not free(3)
immediately but lazily do so at the end, hence my response.
^ permalink raw reply
* Re: [PATCHv3] submodule--helper: normalize funny urls
From: Stefan Beller @ 2016-10-19 1:04 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: <xmqq60opnolz.fsf@gitster.mtv.corp.google.com>
On Tue, Oct 18, 2016 at 5:56 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> The underlying issue is two fold:
>>
>> * in t3600 we'd need
>> diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
>> index d046d98..545d32f 100755
>> --- a/t/t3600-rm.sh
>> +++ b/t/t3600-rm.sh
>> @@ -616,7 +616,7 @@ test_expect_success 'setup subsubmodule' '
>> git submodule update &&
>> (cd submod &&
>> git update-index --add --cacheinfo 160000 $(git
>> rev-parse HEAD) subsubmod &&
>> - git config -f .gitmodules submodule.sub.url ../. &&
>> + git config -f .gitmodules submodule.sub.url ./. &&
>> git config -f .gitmodules submodule.sub.path subsubmod &&
>> git submodule init &&
>> git add .gitmodules &&
>>
>> because the sub-submodule URL is actually the same as the submodule
>> (because we'd test lazily)
>
> This fix sounds entirely sane. The "../." you touched was depending
> on the buggy behaviour; it is exactly the case of "there were two
> wrongs that were covering each other; after one of them gets fixed,
> the other one's brokenness is exposed", right?
>
>> However in t7403, we have a construct like:
>>
>> git clone . super
>>
>> which then results in
>>
>> git -C super remote -v
>> ...../git/t/trash directory.t7403-submodule-sync/. (fetch)
>
> That sounds expected. We do not have to add trailing "/.", but the
> system ought to work with or without it correctly and the same way.
>
>> However instead of fixing the levels of nesting, the fix is as easy as:
>> diff --git a/t/t7403-submodule-sync.sh b/t/t7403-submodule-sync.sh
>> index 0726799..525d32b 100755
>> --- a/t/t7403-submodule-sync.sh
>> +++ b/t/t7403-submodule-sync.sh
>> @@ -15,7 +15,9 @@ test_expect_success setup '
>> git add file &&
>> test_tick &&
>> git commit -m upstream &&
>> - git clone . super &&
>> + # avoid cloning a repository with a url ending in /.
>> + git clone . root &&
>> + git clone root super &&
>> git clone super submodule &&
>> (
>> cd submodule &&
>>
>> Same goes for t740{6,7} as well as t7506.
>
> Isn't the issue the same as that "3600-rm" one? I know you said
> twofold upfront, but I am not sure I agree.
I took a couple of hours trying to get the same fix applied to the t7* tests,
but that doesn't seem to be as easy. I'll try again.
>
> The "super" repository refers to its submodules with "../submodule"
> in the test code we have, even though the submodule referred to
> lives inside $TRASH, and by fixing the "trailing /. and trailing
> /root are treated the same way" bug, its reference created in the
> test ends up referring to one level above, perhaps in t/submodule,
> instead of the intended place t/trash/submodule. By using "root",
> you are making their wrong references point at the right place.
Correct.
>
> Admittedly, the updated test above tests something different from
> what it originally wanted to test, which feels somewhat distasteful
> but I do not think it is wrong.
I think it is. I was just showing how to quick fix the issue, and how wide the
impact was. More like assessing the situation of what is broken with
that patch, rather than proposing a way to go for fixing.
>
>> I think this change to the test suite is not warranted, because
>> we want to have the current behavior as-is ...
>
> 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).
>
> Then do the fix under discussion and introduce a test that clones
> from "." refers to submodules with relative path and makes sure that
> trailing "/." is interpreted correctly.
^ permalink raw reply
* Re: [PATCHv3] submodule--helper: normalize funny urls
From: Junio C Hamano @ 2016-10-19 0:56 UTC (permalink / raw)
To: Stefan Beller
Cc: Johannes Sixt, Johannes Schindelin, git@vger.kernel.org, Karl A.,
Dennis Kaarsemaker, Jonathan Nieder
In-Reply-To: <CAGZ79kZHLVpxbJ_C-dM2LDA64-_TJNyY+52fTWkOvLvvAq2XDg@mail.gmail.com>
Stefan Beller <sbeller@google.com> writes:
> The underlying issue is two fold:
>
> * in t3600 we'd need
> diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
> index d046d98..545d32f 100755
> --- a/t/t3600-rm.sh
> +++ b/t/t3600-rm.sh
> @@ -616,7 +616,7 @@ test_expect_success 'setup subsubmodule' '
> git submodule update &&
> (cd submod &&
> git update-index --add --cacheinfo 160000 $(git
> rev-parse HEAD) subsubmod &&
> - git config -f .gitmodules submodule.sub.url ../. &&
> + git config -f .gitmodules submodule.sub.url ./. &&
> git config -f .gitmodules submodule.sub.path subsubmod &&
> git submodule init &&
> git add .gitmodules &&
>
> because the sub-submodule URL is actually the same as the submodule
> (because we'd test lazily)
This fix sounds entirely sane. The "../." you touched was depending
on the buggy behaviour; it is exactly the case of "there were two
wrongs that were covering each other; after one of them gets fixed,
the other one's brokenness is exposed", right?
> However in t7403, we have a construct like:
>
> git clone . super
>
> which then results in
>
> git -C super remote -v
> ...../git/t/trash directory.t7403-submodule-sync/. (fetch)
That sounds expected. We do not have to add trailing "/.", but the
system ought to work with or without it correctly and the same way.
> However instead of fixing the levels of nesting, the fix is as easy as:
> diff --git a/t/t7403-submodule-sync.sh b/t/t7403-submodule-sync.sh
> index 0726799..525d32b 100755
> --- a/t/t7403-submodule-sync.sh
> +++ b/t/t7403-submodule-sync.sh
> @@ -15,7 +15,9 @@ test_expect_success setup '
> git add file &&
> test_tick &&
> git commit -m upstream &&
> - git clone . super &&
> + # avoid cloning a repository with a url ending in /.
> + git clone . root &&
> + git clone root super &&
> git clone super submodule &&
> (
> cd submodule &&
>
> Same goes for t740{6,7} as well as t7506.
Isn't the issue the same as that "3600-rm" one? I know you said
twofold upfront, but I am not sure I agree.
The "super" repository refers to its submodules with "../submodule"
in the test code we have, even though the submodule referred to
lives inside $TRASH, and by fixing the "trailing /. and trailing
/root are treated the same way" bug, its reference created in the
test ends up referring to one level above, perhaps in t/submodule,
instead of the intended place t/trash/submodule. By using "root",
you are making their wrong references point at the right place.
Admittedly, the updated test above tests something different from
what it originally wanted to test, which feels somewhat distasteful
but I do not think it is wrong.
> I think this change to the test suite is not warranted, because
> we want to have the current behavior as-is ...
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?
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?
Then do the fix under discussion and introduce a test that clones
from "." refers to submodules with relative path and makes sure that
trailing "/." is interpreted correctly.
^ permalink raw reply
* Re: [PATCHv3] attr: convert to new threadsafe API
From: Junio C Hamano @ 2016-10-19 0:40 UTC (permalink / raw)
To: Stefan Beller
Cc: Brandon Williams, git@vger.kernel.org, Johannes Sixt,
Jacob Keller
In-Reply-To: <CAGZ79kbq7m6s5s8OCXFwyonUdiaL3nVNCoGS40xkH2qKCkYzag@mail.gmail.com>
Stefan Beller <sbeller@google.com> writes:
> I am not sure if I see the upside on wrapping a single value except for
> its future proofness,
I do not see anything other than future-proofing, either. If we
need to touch all the code that uses the attributes to update the
API, I'd prefer to avoid having to do that again in the future.
^ permalink raw reply
* Re: [PATCHv3] attr: convert to new threadsafe API
From: Stefan Beller @ 2016-10-19 0:20 UTC (permalink / raw)
To: Junio C Hamano
Cc: Brandon Williams, git@vger.kernel.org, Johannes Sixt,
Jacob Keller
In-Reply-To: <CAPc5daVF2HBD2DfP=w2oG-2PMWQ+guXP_onQX+foSgVYH4N1Qg@mail.gmail.com>
On Tue, Oct 18, 2016 at 5:06 PM, Junio C Hamano <gitster@pobox.com> wrote:
> On Tue, Oct 18, 2016 at 4:52 PM, Stefan Beller <sbeller@google.com> wrote:
>>
>> >
>> > By the way, I do see a merit on the "check" side (tl;dr: but I do
>> > not think "result" needs it, hence I do not see the need for the
>> > "ugly" variants).
>>
>> So we'd rather go with const char **result instead of our own new struct there.
>> Ok, got it.
>
> I do not think you got it. I am talking about wrapping struct around
> an array of element,
> not each element in the array. IOW
>
>> > If we were still to do the wrapping for result, I would say that
>> > basing it around the FLEX_ARRAY idiom, i.e.
>> >
>> >> struct git_attr_result {
>> >> int num_slots;
>> >> const char *value[FLEX_ARRAY];
>> >> };
>
> the structure around the array of elements (value) that allows us to have
> something other than value[] in it. That is what I said "I do not see
> the need for".
>
> It is perfectly fine future-proofing to have
>
> struct git_attr_result_value {
> const char *value;
> };
>
> and have the users of API declare
>
> struct git_attr_result value[5];
>
> or whatever. That way we could fatten the structure later if we wanted
> to without having to update the users of API, and there is no downside.
>
> Having wrapping strut around the array does have a huge downside,
> and that is what I said I see no need for.
I am not sure if I see the upside on wrapping a single value except for
its future proofness, i.e. what if we want to transport information that
is valid for all values, e.g. an error code or that the result check was done
lazily (for non lazy you would need to do X) or ...
IOW I would expect there to be more use cases for information regarding
all the values and not each value enhanced by a thing.
We could just repeat the thing in each of the 5 values in
> struct git_attr_result value[5];
though.
I'll go with that.
^ permalink raw reply
* Re: [PATCHv3] attr: convert to new threadsafe API
From: Junio C Hamano @ 2016-10-19 0:06 UTC (permalink / raw)
To: Stefan Beller
Cc: Brandon Williams, git@vger.kernel.org, Johannes Sixt,
Jacob Keller
In-Reply-To: <CAGZ79kbS4mP7sVTCM+QJXTwKsgZ40xvVDng-F3igZnJWLYek0A@mail.gmail.com>
On Tue, Oct 18, 2016 at 4:52 PM, Stefan Beller <sbeller@google.com> wrote:
>
> >
> > By the way, I do see a merit on the "check" side (tl;dr: but I do
> > not think "result" needs it, hence I do not see the need for the
> > "ugly" variants).
>
> So we'd rather go with const char **result instead of our own new struct there.
> Ok, got it.
I do not think you got it. I am talking about wrapping struct around
an array of element,
not each element in the array. IOW
> > If we were still to do the wrapping for result, I would say that
> > basing it around the FLEX_ARRAY idiom, i.e.
> >
> >> struct git_attr_result {
> >> int num_slots;
> >> const char *value[FLEX_ARRAY];
> >> };
the structure around the array of elements (value) that allows us to have
something other than value[] in it. That is what I said "I do not see
the need for".
It is perfectly fine future-proofing to have
struct git_attr_result_value {
const char *value;
};
and have the users of API declare
struct git_attr_result value[5];
or whatever. That way we could fatten the structure later if we wanted
to without having to update the users of API, and there is no downside.
Having wrapping strut around the array does have a huge downside,
and that is what I said I see no need for.
^ permalink raw reply
* Re: [PATCHv3] attr: convert to new threadsafe API
From: Stefan Beller @ 2016-10-18 23:52 UTC (permalink / raw)
To: Junio C Hamano
Cc: Brandon Williams, git@vger.kernel.org, Johannes Sixt,
Jacob Keller
In-Reply-To: <xmqq8ttr0wny.fsf@gitster.mtv.corp.google.com>
On Fri, Oct 14, 2016 at 8:37 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> *1* Would we need a wrapping struct around the array of results?
>
> By the way, I do see a merit on the "check" side (tl;dr: but I do
> not think "result" needs it, hence I do not see the need for the
> "ugly" variants).
So we'd rather go with const char **result instead of our own new struct there.
Ok, got it.
>
> Take "archive" for example. For each path, it wants to see the
> attribute "export-ignore" to decide if it is to be omitted. In
> addition, the usual set of attributes used to smudge blobs into the
> working tree representation are inspected by the convert.c API as
> part of its implementation of convert_to_working_tree(). This
> program has at least two sets of <"check", "result"> that are used
> by two git_check_attr() callsites that are unaware of each other.
>
> One of the optimizations we discussed is to trim down the attr-stack
> (which caches the attributes read from .gitattributes files that are
> in effect for the "last" directory that has the path for which
> attrbiutes are queried for) by reading/keeping only the entries that
> affect the attributes the caller is interested in. But when there
> are multiple callsites that are interested in different sets of
> attributes, we obviously cannot do such an optimization without
> taking too much cache-invalidation hit. Because these callsites are
> not unaware of each other, I do not think we can say "keep the
> entries that affects the union of all active callsites" very easily,
> even if it were possible.
>
> But we could tie this cache to "check", which keeps a constant
> subset of attributes that the caller is interested in (i.e. each
> callsite would keep its own cache that is useful for its query).
> While we are single-threaded, "struct git_attr_check" being a
> wrapping struct around the array of "what attributes are of
> interest?" is a good place to add that per-check attr-stack cache.
> When we go multi-threaded, the attr-stack cache must become
> per-thread, and needs to be moved to per-thread storage, and such a
> per-thread storage would have multiple attr-stack, one per "check"
> instance (i.e. looking up the attr-stack may have to say "who/what
> thread am I?" to first go to the thread-local storage for the
> current thread, where a table of pointers to attr-stacks is kept and
> from there, index into that table to find the attr-stack that
> corresponds to the particular "check"). We could use the address of
> "check" as the key into this table, but "struct git_attr_check" that
> wraps the array gives us another option to allocate a small
> consecutive integer every time initl() creates a new "check" and use
> it as the index into that attr-stack table, as that integer index
> can be in the struct that wraps the array of wanted attributes.
>
> Note. none of the above is a suggestion to do the attr
> caching the way exactly described. The above is primarily
> to illustrate how a wrapping struct may give us future
> flexibility without affecting a single line of code in the
> user of API.
>
> It may turn out that we do not need to have anything other than the
> array of wanted attributes in the "check" struct, but unlike
> "result", "check" is shared across threads, and do not have to live
> directly on the stack, so we can prepare for flexibility.
>
> I do not foresee a similar need for wrapping struct for "result",
> and given that we do want to keep the option of having them directly
> on the stack, I am inclined to say we shouldn't introduce one.
>
> If we were still to do the wrapping for result, I would say that
> basing it around the FLEX_ARRAY idiom, i.e.
>
>> struct git_attr_result {
>> int num_slots;
>> const char *value[FLEX_ARRAY];
>> };
>
> is a horrible idea. It would be less horrible if it were
>
> struct git_attr_result {
> int num_slots;
> const char **value;
> };
So const char** but with an additional number of slots, all we do
would be to compare this number of slots to the checks number of slots and
die("BUG:..."), which is just a burden and no help.
>
> then make the API user write via a convenience macro something like
> this
>
> const char *result_values[NUM_ATTRS_OF_INTEREST];
> struct git_attr_result result = {
> ARRAY_SIZE(result_values), &result_values
> };
>
> instead. That way, at least the side that implements git_check_attr()
> would not have to be type-unsafe like the example of ugliness in the
> message I am following-up on.
Ok I will reroll with the const char** instead of the macro stuff that
I came up with,
(that would be type safe though uglier than the pure variant).
^ permalink raw reply
* Re: [PATCHv3] submodule--helper: normalize funny urls
From: Stefan Beller @ 2016-10-18 23:25 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: <xmqqshrtnynj.fsf@gitster.mtv.corp.google.com>
On Tue, Oct 18, 2016 at 2:19 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Stefan Beller <sbeller@google.com> writes:
>>
>>> The remote URL for the submodule can be specified relative
>>> ...
>>> v3:
>>> * fixed the coding style.
>>
>> Ah, thanks. I had a squash queued on top but will replace with this
>> one.
>
> Heh, I guess I shouldn't have responded before seeing what this
> breaks. Applied on top of sb/submodule-ignore-trailing-slash, these
> seem to break.
Ugh. (I should have tested more than just t0060).
The underlying issue is two fold:
* in t3600 we'd need
diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
index d046d98..545d32f 100755
--- a/t/t3600-rm.sh
+++ b/t/t3600-rm.sh
@@ -616,7 +616,7 @@ test_expect_success 'setup subsubmodule' '
git submodule update &&
(cd submod &&
git update-index --add --cacheinfo 160000 $(git
rev-parse HEAD) subsubmod &&
- git config -f .gitmodules submodule.sub.url ../. &&
+ git config -f .gitmodules submodule.sub.url ./. &&
git config -f .gitmodules submodule.sub.path subsubmod &&
git submodule init &&
git add .gitmodules &&
because the sub-submodule URL is actually the same as the submodule
(because we'd test lazily)
This looks ok from a bug fixers perspective.
However in t7403, we have a construct like:
git clone . super
which then results in
git -C super remote -v
...../git/t/trash directory.t7403-submodule-sync/. (fetch)
And the commit message of this patch claimed we'd never use
the /. syntax ourselves. (We could argue the stupid users in the test
suite are doing it wrong, because in practice nobody would use clone
to create a nested repository? Not sure I agree.)
However instead of fixing the levels of nesting, the fix is as easy as:
diff --git a/t/t7403-submodule-sync.sh b/t/t7403-submodule-sync.sh
index 0726799..525d32b 100755
--- a/t/t7403-submodule-sync.sh
+++ b/t/t7403-submodule-sync.sh
@@ -15,7 +15,9 @@ test_expect_success setup '
git add file &&
test_tick &&
git commit -m upstream &&
- git clone . super &&
+ # avoid cloning a repository with a url ending in /.
+ git clone . root &&
+ git clone root super &&
git clone super submodule &&
(
cd submodule &&
Same goes for t740{6,7} as well as t7506.
I think this change to the test suite is not warranted, because
we want to have the current behavior as-is as it seems like a nice
hack:
* Maybe we'd want to think about checking for the URL in git clone
normalize the URL before configuring remote.origin.URL
* an often observed work flow for submodule tests seems:
mkdir sub1 &&
git -C sub1 init &&
...
git clone . super &&
git -C super submodule add ../sub1
... # the ../sub1 looks intuitively correct
# because from the current directory which is
# super the relative path is ../sub1
#
# However in reality this ought to be a relative URL,
# and as super sits in the same directory as sub1
# ./sub1 would be "correct" according to the documentation
# However as the super remote URL ends with /.
# we had a bug that we needed to add one layer of unnesting
# and that is how ../sub1 worked.
Not sure about this patch any more.
Stefan
^ permalink raw reply related
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