* 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
* Re: [PATCHv3] submodule--helper: normalize funny urls
From: Junio C Hamano @ 2016-10-18 21:19 UTC (permalink / raw)
To: Stefan Beller; +Cc: j6t, Johannes.Schindelin, git, venv21, dennis, jrnieder
In-Reply-To: <xmqqzim1nyz0.fsf@gitster.mtv.corp.google.com>
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.
t/trash directory.t3600-rm
t/trash directory.t7403-submodule-sync
t/trash directory.t7406-submodule-update
t/trash directory.t7407-submodule-foreach
t/trash directory.t7506-status-submodule
Some may be showing broken assumptions of the downstream, two wrongs
compensating each other and correcting one exposing breakage of the
other. I didn't look at them deeply.
^ permalink raw reply
* Re: Integrating submodules with no side effects
From: Stefan Beller @ 2016-10-18 21:17 UTC (permalink / raw)
To: Robert Dailey; +Cc: Git
In-Reply-To: <CAHd499AN2VHL66c6JWxHqS-1bQ6y4PrGjZJiR_ad6HJsCGpeDQ@mail.gmail.com>
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)
> 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")
> 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.
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.
[1] e.g. https://www.kernel.org/pub/software/scm/git/docs/
Thanks,
Stefan
^ permalink raw reply
* Re: [PATCHv3] submodule--helper: normalize funny urls
From: Junio C Hamano @ 2016-10-18 21:12 UTC (permalink / raw)
To: Stefan Beller; +Cc: j6t, Johannes.Schindelin, git, venv21, dennis, jrnieder
In-Reply-To: <20161018210623.32696-1-sbeller@google.com>
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.
^ permalink raw reply
* [PATCHv3] submodule--helper: normalize funny urls
From: Stefan Beller @ 2016-10-18 21:06 UTC (permalink / raw)
To: gitster, j6t, Johannes.Schindelin
Cc: git, venv21, dennis, jrnieder, Stefan Beller
The remote URL for the submodule can be specified relative
to the URL of the superproject in .gitmodules. A top-level
git://site.xz/toplevel.git can specify in its .gitmodules
[submodule "sub"]
url = ../submodule.git
path = sub
to say that git://site.xz/submodule.git is where the
submodule bound at its "sub/" is found.
However, when the toplevel is cloned like this:
git clone git://site.xz/toplevel.git/. top
i.e. the toplevel specifies its URL with trailing "/.", the
code set the URL to git://site.xz/toplevel.git/submodule.git
for the submodule, which is nonsense. This was because the
logic failed to treat trailing "/." any differently from
trailing "/<anything-without-slash>" when resolving a
relative URL "../<something>" off of it. Stripping "/." at
the end does *not* take you one level up, even though
stripping "/<anything-without-slash>" does!
Some users may rely on this by always cloning with '/.' and having
an additional '../' in the relative path for the submodule, and this
patch breaks them. So why introduce this patch?
The fix in c12922024 (submodule: ignore trailing slash on superproject
URL, 2016-10-10) and prior discussion revealed, that Git and Git
for Windows treat URLs differently, as currently Git for Windows
strips off a trailing dot from paths when calling a Git binary
unlike when running a shell. Which means Git for Windows is already
doing the right thing for the case mentioned above, but it would fail
our current tests, that test for the broken behavior and it would
confuse users working across platforms. So we'd rather fix it
in Git to ignore any of these trailing no ops in the path properly.
We never produce the URLs with a trailing '/.' in Git ourselves,
they come to us, because the user used it as the URL for cloning
a superproject. Normalize these paths.
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
v3:
* fixed the coding style.
v2:
* reworded the commit message, taken from Junio, but added more explanation
why we want to introduce this patch.
* added the length check
* use an infinite loop with break instead of a variable
to determine the ending condition.
builtin/submodule--helper.c | 48 +++++++++++++++++++++++++++++++++------------
t/t0060-path-utils.sh | 11 +++++++----
2 files changed, 43 insertions(+), 16 deletions(-)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 260f46f..4f11082 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -76,6 +76,29 @@ static int chop_last_dir(char **remoteurl, int is_relative)
return 0;
}
+static void strip_url_ending(char *url, size_t *len_)
+{
+ size_t len = len_ ? *len_ : strlen(url);
+
+ for (;;) {
+ if (len > 1 && is_dir_sep(url[len - 2]) && url[len - 1] == '.') {
+ url[len - 2] = '\0';
+ len -= 2;
+ continue;
+ }
+ if (len > 0 && is_dir_sep(url[len - 1])) {
+ url[len - 1] = '\0';
+ len--;
+ continue;
+ }
+
+ break;
+ }
+
+ if (len_)
+ *len_ = len;
+}
+
/*
* The `url` argument is the URL that navigates to the submodule origin
* repo. When relative, this URL is relative to the superproject origin
@@ -93,14 +116,16 @@ static int chop_last_dir(char **remoteurl, int is_relative)
* the superproject working tree otherwise.
*
* NEEDSWORK: This works incorrectly on the domain and protocol part.
- * remote_url url outcome expectation
- * http://a.com/b ../c http://a.com/c as is
- * http://a.com/b/ ../c http://a.com/c same as previous line, but
- * ignore trailing slash in url
- * http://a.com/b ../../c http://c error out
- * http://a.com/b ../../../c http:/c error out
- * http://a.com/b ../../../../c http:c error out
- * http://a.com/b ../../../../../c .:c error out
+ * remote_url url outcome expectation
+ * http://a.com/b ../c http://a.com/c as is
+ * http://a.com/b/ ../c http://a.com/c same as previous line, but
+ * ignore trailing '/' in url
+ * http://a.com/b/. ../c http://a.com/c same as previous line, but
+ * ignore trailing '/.' in url
+ * http://a.com/b ../../c http://c error out
+ * http://a.com/b ../../../c http:/c error out
+ * http://a.com/b ../../../../c http:c error out
+ * http://a.com/b ../../../../../c .:c error out
* NEEDSWORK: Given how chop_last_dir() works, this function is broken
* when a local part has a colon in its path component, too.
*/
@@ -115,8 +140,7 @@ static char *relative_url(const char *remote_url,
struct strbuf sb = STRBUF_INIT;
size_t len = strlen(remoteurl);
- if (is_dir_sep(remoteurl[len-1]))
- remoteurl[len-1] = '\0';
+ strip_url_ending(remoteurl, &len);
if (!url_is_local_not_ssh(remoteurl) || is_absolute_path(remoteurl))
is_relative = 0;
@@ -149,10 +173,10 @@ static char *relative_url(const char *remote_url,
}
strbuf_reset(&sb);
strbuf_addf(&sb, "%s%s%s", remoteurl, colonsep ? ":" : "/", url);
- if (ends_with(url, "/"))
- strbuf_setlen(&sb, sb.len - 1);
free(remoteurl);
+ strip_url_ending(sb.buf, &sb.len);
+
if (starts_with_dot_slash(sb.buf))
out = xstrdup(sb.buf + 2);
else
diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
index 25b48e5..e154e5f 100755
--- a/t/t0060-path-utils.sh
+++ b/t/t0060-path-utils.sh
@@ -329,14 +329,17 @@ test_submodule_relative_url "(null)" "./foo" "../submodule" "submodule"
test_submodule_relative_url "(null)" "//somewhere else/repo" "../subrepo" "//somewhere else/subrepo"
test_submodule_relative_url "(null)" "$PWD/subsuper_update_r" "../subsubsuper_update_r" "$(pwd)/subsubsuper_update_r"
test_submodule_relative_url "(null)" "$PWD/super_update_r2" "../subsuper_update_r" "$(pwd)/subsuper_update_r"
-test_submodule_relative_url "(null)" "$PWD/." "../." "$(pwd)/."
-test_submodule_relative_url "(null)" "$PWD" "./." "$(pwd)/."
+test_submodule_relative_url "(null)" "$PWD/sub/." "../." "$(pwd)"
+test_submodule_relative_url "(null)" "$PWD/sub/./." "../." "$(pwd)"
+test_submodule_relative_url "(null)" "$PWD/sub/.////././/./." "../." "$(pwd)"
+test_submodule_relative_url "(null)" "$PWD" "./." "$(pwd)"
test_submodule_relative_url "(null)" "$PWD/addtest" "../repo" "$(pwd)/repo"
test_submodule_relative_url "(null)" "$PWD" "./å äö" "$(pwd)/å äö"
-test_submodule_relative_url "(null)" "$PWD/." "../submodule" "$(pwd)/submodule"
+test_submodule_relative_url "(null)" "$PWD/sub" "../submodule" "$(pwd)/submodule"
+test_submodule_relative_url "(null)" "$PWD/sub/." "../submodule" "$(pwd)/submodule"
test_submodule_relative_url "(null)" "$PWD/submodule" "../submodule" "$(pwd)/submodule"
test_submodule_relative_url "(null)" "$PWD/home2/../remote" "../bundle1" "$(pwd)/home2/../bundle1"
-test_submodule_relative_url "(null)" "$PWD/submodule_update_repo" "./." "$(pwd)/submodule_update_repo/."
+test_submodule_relative_url "(null)" "$PWD/submodule_update_repo" "./." "$(pwd)/submodule_update_repo"
test_submodule_relative_url "(null)" "file:///tmp/repo" "../subrepo" "file:///tmp/subrepo"
test_submodule_relative_url "(null)" "foo/bar" "../submodule" "foo/submodule"
test_submodule_relative_url "(null)" "foo" "../submodule" "submodule"
--
2.10.1.480.g573bd76
^ permalink raw reply related
* Re: What's cooking in git.git (Oct 2016, #04; Mon, 17)
From: Junio C Hamano @ 2016-10-18 20:31 UTC (permalink / raw)
To: Lars Schneider; +Cc: git
In-Reply-To: <9F780406-FC77-4B3E-AEEB-7F6215E6702C@gmail.com>
Lars Schneider <larsxschneider@gmail.com> writes:
>> On 17 Oct 2016, at 15:28, Junio C Hamano <gitster@pobox.com> wrote:
>> ...
>>
>> * ls/filter-process (2016-10-17) 14 commits
>> - contrib/long-running-filter: add long running filter example
>> - convert: add filter.<driver>.process option
>> - convert: prepare filter.<driver>.process option
>> - convert: make apply_filter() adhere to standard Git error handling
>> - pkt-line: add functions to read/write flush terminated packet streams
>> - pkt-line: add packet_write_gently()
>> - pkt-line: add packet_flush_gently()
>> - pkt-line: add packet_write_fmt_gently()
>> - pkt-line: extract set_packet_header()
>> - pkt-line: rename packet_write() to packet_write_fmt()
>> - run-command: add clean_on_exit_handler
>> - run-command: move check_pipe() from write_or_die to run_command
>> - convert: modernize tests
>> - convert: quote filter names in error messages
>>
>> The smudge/clean filter API expect an external process is spawned
>> to filter the contents for each path that has a filter defined. A
>> new type of "process" filter API has been added to allow the first
>> request to run the filter for a path to spawn a single process, and
>> all filtering need is served by this single process for multiple
>> paths, reducing the process creation overhead.
>
> Hi Junio,
>
> 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.
^ permalink raw reply
* Re: What's cooking in git.git (Oct 2016, #04; Mon, 17)
From: Lars Schneider @ 2016-10-18 20:27 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqtwcasj8y.fsf@gitster.mtv.corp.google.com>
> On 17 Oct 2016, at 15:28, Junio C Hamano <gitster@pobox.com> wrote:
> ...
>
> * ls/filter-process (2016-10-17) 14 commits
> - contrib/long-running-filter: add long running filter example
> - convert: add filter.<driver>.process option
> - convert: prepare filter.<driver>.process option
> - convert: make apply_filter() adhere to standard Git error handling
> - pkt-line: add functions to read/write flush terminated packet streams
> - pkt-line: add packet_write_gently()
> - pkt-line: add packet_flush_gently()
> - pkt-line: add packet_write_fmt_gently()
> - pkt-line: extract set_packet_header()
> - pkt-line: rename packet_write() to packet_write_fmt()
> - run-command: add clean_on_exit_handler
> - run-command: move check_pipe() from write_or_die to run_command
> - convert: modernize tests
> - convert: quote filter names in error messages
>
> The smudge/clean filter API expect an external process is spawned
> to filter the contents for each path that has a filter defined. A
> new type of "process" filter API has been added to allow the first
> request to run the filter for a path to spawn a single process, and
> all filtering need is served by this single process for multiple
> paths, reducing the process creation overhead.
Hi Junio,
what do you think about v11? Do you feel the series is becoming mature
enough for `next`?
Thanks,
Lars
^ permalink raw reply
* Re: [PATCH 1/2] submodule: ignore trailing slash on superproject URL
From: Johannes Sixt @ 2016-10-18 20:06 UTC (permalink / raw)
To: Junio C Hamano
Cc: Johannes Schindelin, Stefan Beller, git@vger.kernel.org, Karl A.,
Dennis Kaarsemaker
In-Reply-To: <2aa41392-a7ed-cd48-2737-bd852757ab35@kdbg.org>
Am 17.10.2016 um 21:32 schrieb Johannes Sixt:
> I think that we could reduce the confusion by converting all $PWD to
> $(pwd) in these test cases. I don't remember why I suggested to use $PWD
> for one of the arguments of the test cases (the second must be $(pwd)),
> but the most likely reason is only that we save a process.
Something like this works for me on Windows. I can update the patch
after the "normalize funny urls" topic settles.
---- 8< ----
t0060: sidestep surprising path mangling results on Windows
When an MSYS program (such as the bash that drives the test suite)
invokes git on Windows, absolute Unix style paths are transformed into
Windows native absolute paths (drive letter form). However, this
transformation also includes some simplifications that are not just
straight-forward textual substitutions:
- When the path ends in "/.", then the dot is stripped, but not the
directory separator.
- When the path contains "..", then it is optimized away if possible,
e.g., "/c/dir/foo/../bar" becomes "c:/dir/bar".
These additional transformations violate the assumptions of some
submodule path tests. We can avoid them when the input is already a
Windows native path, because then MSYS leaves the path unmolested.
Convert the uses of $PWD to $(pwd); the latter returns a native Windows
path.
Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
t/t0060-path-utils.sh | 27 ++++++++++++++-------------
1 file changed, 14 insertions(+), 13 deletions(-)
diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
index 25b48e5..444b5a4 100755
--- a/t/t0060-path-utils.sh
+++ b/t/t0060-path-utils.sh
@@ -305,8 +305,9 @@ test_git_path GIT_COMMON_DIR=bar config bar/config
test_git_path GIT_COMMON_DIR=bar packed-refs bar/packed-refs
test_git_path GIT_COMMON_DIR=bar shallow bar/shallow
-# In the tests below, the distinction between $PWD and $(pwd) is important:
-# on Windows, $PWD is POSIX style (/c/foo), $(pwd) has drive letter (c:/foo).
+# In the tests below, $(pwd) must be used because it is a native path on
+# Windows and avoids MSYS's path mangling (which simplifies "foo/../bar" and
+# strips the dot from trailing "/.").
test_submodule_relative_url "../" "../foo" "../submodule" "../../submodule"
test_submodule_relative_url "../" "../foo/bar" "../submodule" "../../foo/submodule"
@@ -314,7 +315,7 @@ test_submodule_relative_url "../" "../foo/submodule" "../submodule" "../../foo/s
test_submodule_relative_url "../" "./foo" "../submodule" "../submodule"
test_submodule_relative_url "../" "./foo/bar" "../submodule" "../foo/submodule"
test_submodule_relative_url "../../../" "../foo/bar" "../sub/a/b/c" "../../../../foo/sub/a/b/c"
-test_submodule_relative_url "../" "$PWD/addtest" "../repo" "$(pwd)/repo"
+test_submodule_relative_url "../" "$(pwd)/addtest" "../repo" "$(pwd)/repo"
test_submodule_relative_url "../" "foo/bar" "../submodule" "../foo/submodule"
test_submodule_relative_url "../" "foo" "../submodule" "../submodule"
@@ -327,16 +328,16 @@ test_submodule_relative_url "(null)" "../foo" "../submodule" "../submodule"
test_submodule_relative_url "(null)" "./foo/bar" "../submodule" "foo/submodule"
test_submodule_relative_url "(null)" "./foo" "../submodule" "submodule"
test_submodule_relative_url "(null)" "//somewhere else/repo" "../subrepo" "//somewhere else/subrepo"
-test_submodule_relative_url "(null)" "$PWD/subsuper_update_r" "../subsubsuper_update_r" "$(pwd)/subsubsuper_update_r"
-test_submodule_relative_url "(null)" "$PWD/super_update_r2" "../subsuper_update_r" "$(pwd)/subsuper_update_r"
-test_submodule_relative_url "(null)" "$PWD/." "../." "$(pwd)/."
-test_submodule_relative_url "(null)" "$PWD" "./." "$(pwd)/."
-test_submodule_relative_url "(null)" "$PWD/addtest" "../repo" "$(pwd)/repo"
-test_submodule_relative_url "(null)" "$PWD" "./å äö" "$(pwd)/å äö"
-test_submodule_relative_url "(null)" "$PWD/." "../submodule" "$(pwd)/submodule"
-test_submodule_relative_url "(null)" "$PWD/submodule" "../submodule" "$(pwd)/submodule"
-test_submodule_relative_url "(null)" "$PWD/home2/../remote" "../bundle1" "$(pwd)/home2/../bundle1"
-test_submodule_relative_url "(null)" "$PWD/submodule_update_repo" "./." "$(pwd)/submodule_update_repo/."
+test_submodule_relative_url "(null)" "$(pwd)/subsuper_update_r" "../subsubsuper_update_r" "$(pwd)/subsubsuper_update_r"
+test_submodule_relative_url "(null)" "$(pwd)/super_update_r2" "../subsuper_update_r" "$(pwd)/subsuper_update_r"
+test_submodule_relative_url "(null)" "$(pwd)/." "../." "$(pwd)/."
+test_submodule_relative_url "(null)" "$(pwd)" "./." "$(pwd)/."
+test_submodule_relative_url "(null)" "$(pwd)/addtest" "../repo" "$(pwd)/repo"
+test_submodule_relative_url "(null)" "$(pwd)" "./å äö" "$(pwd)/å äö"
+test_submodule_relative_url "(null)" "$(pwd)/." "../submodule" "$(pwd)/submodule"
+test_submodule_relative_url "(null)" "$(pwd)/submodule" "../submodule" "$(pwd)/submodule"
+test_submodule_relative_url "(null)" "$(pwd)/home2/../remote" "../bundle1" "$(pwd)/home2/../bundle1"
+test_submodule_relative_url "(null)" "$(pwd)/submodule_update_repo" "./." "$(pwd)/submodule_update_repo/."
test_submodule_relative_url "(null)" "file:///tmp/repo" "../subrepo" "file:///tmp/subrepo"
test_submodule_relative_url "(null)" "foo/bar" "../submodule" "foo/submodule"
test_submodule_relative_url "(null)" "foo" "../submodule" "submodule"
--
2.10.0.343.g37bc62b
^ permalink raw reply related
* Re: [PATCH] submodule--helper: normalize funny urls
From: Junio C Hamano @ 2016-10-18 19:49 UTC (permalink / raw)
To: Stefan Beller
Cc: git, j6t, Johannes.Schindelin, venv21, dennis, jrnieder, bmwill
In-Reply-To: <20161018175247.28326-1-sbeller@google.com>
Stefan Beller <sbeller@google.com> writes:
> Some users may rely on this by always cloning with '/.' and having
> an additional '../' in the relative path for the submodule, and this
> patch breaks them. So why introduce this patch?
>
> The fix in c12922024 (submodule: ignore trailing slash on superproject
> URL, 2016-10-10) and prior discussion revealed, that Git and Git
> for Windows treat URLs differently, as currently Git for Windows
> strips off a trailing dot from paths when calling a Git binary
> unlike when running a shell. Which means Git for Windows is already
> doing the right thing for the case mentioned above, but it would fail
> our current tests, that test for the broken behavior and it would
> confuse users working across platforms. So we'd rather fix it
> in Git to ignore any of these trailing no ops in the path properly.
>
> We never produce the URLs with a trailing '/.' in Git ourselves,
> they come to us, because the user used it as the URL for cloning
> a superproject. Normalize these paths.
>
> Helped-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>
> * reworded the commit message, taken from Junio, but added more explanation
> why we want to introduce this patch.
The additional explanation is very good.
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 260f46f..ac03cb3 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -76,6 +76,29 @@ static int chop_last_dir(char **remoteurl, int is_relative)
> return 0;
> }
>
> +static void strip_url_ending(char *url, size_t *_len)
> +{
> + size_t len = _len ? *_len : strlen(url);
Stare at our codebase and you'd notice that we avoid using names
with leading underscore deliberately and use trailing one instead
when we name a throw-away name like this. Let's do the same here.
I.e.
static void strip_url_ending(char *url, size_t *len_)
{
size_t len = len_ ? *len_ : strlen(url);
> + for (;;) {
> + if (len > 1 && is_dir_sep(url[len-2]) && url[len-1] == '.') {
> + url[len-2] = '\0';
"len-1" and "len-2" are usually spelled with SP on both sides of
binary operators.
> + len -= 2;
> + continue;
> + }
> + if (len > 0 && is_dir_sep(url[len-1])) {
> + url[len-1] = '\0';
> + len --;
And post-decrement sticks to whatever it is decrementing without SP.
> + continue;
> + }
^ permalink raw reply
* [PATCH] sha1_name: remove ONELINE_SEEN bit
From: Junio C Hamano @ 2016-10-18 19:42 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin
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;
}
^ permalink raw reply related
* Integrating submodules with no side effects
From: Robert Dailey @ 2016-10-18 19:35 UTC (permalink / raw)
To: Git
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"
git rm "$1"
mv "${1}_" "$1"
git add "$1/**"
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? 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".
^ permalink raw reply
* Re: [PATCH] daemon, path.c: fix a bug with ~ in repo paths
From: Luke Shumaker @ 2016-10-18 18:05 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Luke Shumaker, git, pclouds, peff
In-Reply-To: <xmqqvawppote.fsf@gitster.mtv.corp.google.com>
On Tue, 18 Oct 2016 13:08:45 -0400,
Junio C Hamano wrote:
>
> Luke Shumaker <lukeshu@sbcglobal.net> writes:
>
> > The superficial aspect of this change is that git-daemon now allows paths
> > that start with a "~". Previously, if git-daemon was run with
> > "--base-path=/srv/git", it was impossible to get it to serve
> > "/srv/git/~foo/bar.git".
>
> I am not sure I understand what you are saying here. Do you mean
>
> I have a path on my server /srv/git/~foo/bar.git; the tilde does
> not mean anything special--it is just a byte in a valid pathname.
>
> I want to allow my users to say
>
> git fetch git://my.server/~foo/bar.git
>
> and fetch from that repository, but "git daemon" lacks the way
> to configure to allow it.
Yes, that is what I am saying.
> If that is the case, what happens instead? Due to the leading
> "~foo/" getting noticed as an attempt to use the user-path expansion
> it is not treated as just a literal character?
What happens instead is
if (*dir == '~') {
if (!user_path) {
logerror("'%s': User-path not allowed", dir);
return NULL;
}
which to the user looks like
git clone git://my.server/~foo/bar.git
Cloning into 'bar'...
fatal: remote error: access denied or repository not exported: ~foo/bar.git
> 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. Note that it doesn't hurt with
git-shell or cgit (I haven't checked with gitweb).
Many programs (especially shell scripts) fail to deal with filenames
containing a space. "Don't put spaces in filenames if it hurts".
It's still a bug in the program.
Similarly, `git gui` used to not be able to add a file in a directory
starting with '~' (when one clicked the file named "~foo/bar", it
said something along the lines of "/home/~foo/bar is outside
repository"), and one had to use `git add '~foo/bar` directly.
"Don't do it if it hurts"; it was still a bug.
Aside: one (somewhat silly) non-user reason that I've seen for a
directory to start with '~' is that it sorts after all other ASCII
characters; it moves the directory to the end of any lists.
--
Happy hacking,
~ Luke Shumaker
^ permalink raw reply
* [PATCH] submodule--helper: normalize funny urls
From: Stefan Beller @ 2016-10-18 17:52 UTC (permalink / raw)
To: gitster
Cc: git, j6t, Johannes.Schindelin, venv21, dennis, jrnieder, bmwill,
Stefan Beller
The remote URL for the submodule can be specified relative
to the URL of the superproject in .gitmodules. A top-level
git://site.xz/toplevel.git can specify in its .gitmodules
[submodule "sub"]
url = ../submodule.git
path = sub
to say that git://site.xz/submodule.git is where the
submodule bound at its "sub/" is found.
However, when the toplevel is cloned like this:
git clone git://site.xz/toplevel.git/. top
i.e. the toplevel specifies its URL with trailing "/.", the
code set the URL to git://site.xz/toplevel.git/submodule.git
for the submodule, which is nonsense. This was because the
logic failed to treat trailing "/." any differently from
trailing "/<anything-without-slash>" when resolving a
relative URL "../<something>" off of it. Stripping "/." at
the end does *not* take you one level up, even though
stripping "/<anything-without-slash>" does!
Some users may rely on this by always cloning with '/.' and having
an additional '../' in the relative path for the submodule, and this
patch breaks them. So why introduce this patch?
The fix in c12922024 (submodule: ignore trailing slash on superproject
URL, 2016-10-10) and prior discussion revealed, that Git and Git
for Windows treat URLs differently, as currently Git for Windows
strips off a trailing dot from paths when calling a Git binary
unlike when running a shell. Which means Git for Windows is already
doing the right thing for the case mentioned above, but it would fail
our current tests, that test for the broken behavior and it would
confuse users working across platforms. So we'd rather fix it
in Git to ignore any of these trailing no ops in the path properly.
We never produce the URLs with a trailing '/.' in Git ourselves,
they come to us, because the user used it as the URL for cloning
a superproject. Normalize these paths.
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
* reworded the commit message, taken from Junio, but added more explanation
why we want to introduce this patch.
* added the length check
* use an infinite loop with break instead of a variable
to determine the ending condition.
builtin/submodule--helper.c | 48 +++++++++++++++++++++++++++++++++------------
t/t0060-path-utils.sh | 11 +++++++----
2 files changed, 43 insertions(+), 16 deletions(-)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 260f46f..ac03cb3 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -76,6 +76,29 @@ static int chop_last_dir(char **remoteurl, int is_relative)
return 0;
}
+static void strip_url_ending(char *url, size_t *_len)
+{
+ size_t len = _len ? *_len : strlen(url);
+
+ for (;;) {
+ if (len > 1 && is_dir_sep(url[len-2]) && url[len-1] == '.') {
+ url[len-2] = '\0';
+ len -= 2;
+ continue;
+ }
+ if (len > 0 && is_dir_sep(url[len-1])) {
+ url[len-1] = '\0';
+ len --;
+ continue;
+ }
+
+ break;
+ }
+
+ if (_len)
+ *_len = len;
+}
+
/*
* The `url` argument is the URL that navigates to the submodule origin
* repo. When relative, this URL is relative to the superproject origin
@@ -93,14 +116,16 @@ static int chop_last_dir(char **remoteurl, int is_relative)
* the superproject working tree otherwise.
*
* NEEDSWORK: This works incorrectly on the domain and protocol part.
- * remote_url url outcome expectation
- * http://a.com/b ../c http://a.com/c as is
- * http://a.com/b/ ../c http://a.com/c same as previous line, but
- * ignore trailing slash in url
- * http://a.com/b ../../c http://c error out
- * http://a.com/b ../../../c http:/c error out
- * http://a.com/b ../../../../c http:c error out
- * http://a.com/b ../../../../../c .:c error out
+ * remote_url url outcome expectation
+ * http://a.com/b ../c http://a.com/c as is
+ * http://a.com/b/ ../c http://a.com/c same as previous line, but
+ * ignore trailing '/' in url
+ * http://a.com/b/. ../c http://a.com/c same as previous line, but
+ * ignore trailing '/.' in url
+ * http://a.com/b ../../c http://c error out
+ * http://a.com/b ../../../c http:/c error out
+ * http://a.com/b ../../../../c http:c error out
+ * http://a.com/b ../../../../../c .:c error out
* NEEDSWORK: Given how chop_last_dir() works, this function is broken
* when a local part has a colon in its path component, too.
*/
@@ -115,8 +140,7 @@ static char *relative_url(const char *remote_url,
struct strbuf sb = STRBUF_INIT;
size_t len = strlen(remoteurl);
- if (is_dir_sep(remoteurl[len-1]))
- remoteurl[len-1] = '\0';
+ strip_url_ending(remoteurl, &len);
if (!url_is_local_not_ssh(remoteurl) || is_absolute_path(remoteurl))
is_relative = 0;
@@ -149,10 +173,10 @@ static char *relative_url(const char *remote_url,
}
strbuf_reset(&sb);
strbuf_addf(&sb, "%s%s%s", remoteurl, colonsep ? ":" : "/", url);
- if (ends_with(url, "/"))
- strbuf_setlen(&sb, sb.len - 1);
free(remoteurl);
+ strip_url_ending(sb.buf, &sb.len);
+
if (starts_with_dot_slash(sb.buf))
out = xstrdup(sb.buf + 2);
else
diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
index 25b48e5..e154e5f 100755
--- a/t/t0060-path-utils.sh
+++ b/t/t0060-path-utils.sh
@@ -329,14 +329,17 @@ test_submodule_relative_url "(null)" "./foo" "../submodule" "submodule"
test_submodule_relative_url "(null)" "//somewhere else/repo" "../subrepo" "//somewhere else/subrepo"
test_submodule_relative_url "(null)" "$PWD/subsuper_update_r" "../subsubsuper_update_r" "$(pwd)/subsubsuper_update_r"
test_submodule_relative_url "(null)" "$PWD/super_update_r2" "../subsuper_update_r" "$(pwd)/subsuper_update_r"
-test_submodule_relative_url "(null)" "$PWD/." "../." "$(pwd)/."
-test_submodule_relative_url "(null)" "$PWD" "./." "$(pwd)/."
+test_submodule_relative_url "(null)" "$PWD/sub/." "../." "$(pwd)"
+test_submodule_relative_url "(null)" "$PWD/sub/./." "../." "$(pwd)"
+test_submodule_relative_url "(null)" "$PWD/sub/.////././/./." "../." "$(pwd)"
+test_submodule_relative_url "(null)" "$PWD" "./." "$(pwd)"
test_submodule_relative_url "(null)" "$PWD/addtest" "../repo" "$(pwd)/repo"
test_submodule_relative_url "(null)" "$PWD" "./å äö" "$(pwd)/å äö"
-test_submodule_relative_url "(null)" "$PWD/." "../submodule" "$(pwd)/submodule"
+test_submodule_relative_url "(null)" "$PWD/sub" "../submodule" "$(pwd)/submodule"
+test_submodule_relative_url "(null)" "$PWD/sub/." "../submodule" "$(pwd)/submodule"
test_submodule_relative_url "(null)" "$PWD/submodule" "../submodule" "$(pwd)/submodule"
test_submodule_relative_url "(null)" "$PWD/home2/../remote" "../bundle1" "$(pwd)/home2/../bundle1"
-test_submodule_relative_url "(null)" "$PWD/submodule_update_repo" "./." "$(pwd)/submodule_update_repo/."
+test_submodule_relative_url "(null)" "$PWD/submodule_update_repo" "./." "$(pwd)/submodule_update_repo"
test_submodule_relative_url "(null)" "file:///tmp/repo" "../subrepo" "file:///tmp/subrepo"
test_submodule_relative_url "(null)" "foo/bar" "../submodule" "foo/submodule"
test_submodule_relative_url "(null)" "foo" "../submodule" "submodule"
--
2.10.1.480.g573bd76
^ permalink raw reply related
* Re: [PATCH] submodule--helper: normalize funny urls
From: Junio C Hamano @ 2016-10-18 17:51 UTC (permalink / raw)
To: Stefan Beller
Cc: Johannes Sixt, Johannes Schindelin, git@vger.kernel.org, Karl A.,
Dennis Kaarsemaker, Jonathan Nieder
In-Reply-To: <CAGZ79kZEFanPpdrE4sD64LJHmyrBiHafo1VzoGtoGh+wO_HCuw@mail.gmail.com>
Stefan Beller <sbeller@google.com> writes:
> for(;;) {
>
> here ? (this code would not need a variable, and
> for wins over while:
> $ git grep "while (1)" |wc -l
> 107
> $ git grep "for (;;)" |wc -l
> 128
> )
I dunno; the numbers tells me there is no strong preference by wide
margin either way.
I am not sure if the end shape does not really need an exit
condition. If there is a need for one, "for (; condition;)" would
look strange. If there isn't, "for (;;)" is actually my personal
preference over "while (1)".
^ permalink raw reply
* Re: Merge conflicts in .gitattributes can cause trouble
From: Junio C Hamano @ 2016-10-18 17:30 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Lars Schneider, git, Jeff King, me
In-Reply-To: <alpine.DEB.2.20.1610181437180.197091@virtualbox>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> To the contrary. As far as I can see, when calling `git merge`, Git
> currently *does* read .gitattributes from the file, and if that fails,
> falls back to reading that file from the index.
Hmph.
Assuming that the merge always goes in the index order, I think you
are right. When we need to merge path/to/dir/.gitattributes, we
would need to know all the .gitattributes files that may affect that
path, so .gitattributes, path/.gitattributes, path/to/.gitattributes
and the file being merged are all read into core before anything
happens and these are kept in attr_stack while merging anything
underneath path/to/dir/ hierarchy without being re-read from the
filesystem. The original contents of path/to/dir/.gitattributes
cached in the attr_stack will be discarded when we start to merge
things outside path/to/dir (e.g. merging path/to/another/file), but
at that point the contents of path/to/dir/.gitattributes no longer
matters to the path being merged, so unless the merge somehow jumps
around it is OK.
It may be a fragile assumption in the longer term that the merge
always goes in the index order, but I think the assumption holds in
the current codebase, and the update planned immediately in the
future.
^ permalink raw reply
* Re: [PATCH] submodule--helper: normalize funny urls
From: Stefan Beller @ 2016-10-18 17:17 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: <xmqqr37dpoip.fsf@gitster.mtv.corp.google.com>
On Tue, Oct 18, 2016 at 10:15 AM, Junio C Hamano <gitster@pobox.com> wrote:
>
> I also somehow find the "check-url-stripping" variable ugly.
>
> while (URL still has something that could be stripped) {
for(;;) {
here ? (this code would not need a variable, and
for wins over while:
$ git grep "while (1)" |wc -l
107
$ git grep "for (;;)" |wc -l
128
)
> if (ends with "/.") {
> strip "/.";
> continue;
> }
> if (ends with "/") {
> strip "/";
> continue;
> }
> break;
> }
>
> perhaps?
^ permalink raw reply
* Re: [PATCH] submodule--helper: normalize funny urls
From: Junio C Hamano @ 2016-10-18 17:15 UTC (permalink / raw)
To: Stefan Beller; +Cc: j6t, Johannes.Schindelin, git, venv21, dennis, jrnieder
In-Reply-To: <xmqqbmyisiae.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> Stefan Beller <sbeller@google.com> writes:
>
>> +static void strip_url_ending(char *url, size_t *_len)
>> +{
>> + int check_url_stripping = 1;
>> + size_t len = _len ? *_len : strlen(url);
>> +
>> + while (check_url_stripping) {
>> + check_url_stripping = 0;
>> + if (is_dir_sep(url[len-2]) && url[len-1] == '.') {
>
> This is "strip /. at the end" it seems.
>
> Does anything in the loop control guarantees 2 <= len at this point?
>
>> + url[len-2] = '\0';
>> + len -= 2;
>> + check_url_stripping = 1;
>> + }
>> +
>> + if (is_dir_sep(url[len-1])) {
>
> This is "strip / at the end" it seems.
>
> Does anything in the loop control guarantees 1 <= len at this point?
>
>> + url[len-1] = '\0';
>> + len--;
>> + check_url_stripping = 1;
>> + }
>> + }
I also somehow find the "check-url-stripping" variable ugly.
while (URL still has something that could be stripped) {
if (ends with "/.") {
strip "/.";
continue;
}
if (ends with "/") {
strip "/";
continue;
}
break;
}
perhaps?
^ permalink raw reply
* Re: [PATCH] daemon, path.c: fix a bug with ~ in repo paths
From: Junio C Hamano @ 2016-10-18 17:08 UTC (permalink / raw)
To: Luke Shumaker; +Cc: git, pclouds, peff
In-Reply-To: <20161018150629.23205-1-lukeshu@sbcglobal.net>
Luke Shumaker <lukeshu@sbcglobal.net> writes:
> The superficial aspect of this change is that git-daemon now allows paths
> that start with a "~". Previously, if git-daemon was run with
> "--base-path=/srv/git", it was impossible to get it to serve
> "/srv/git/~foo/bar.git".
I am not sure I understand what you are saying here. Do you mean
I have a path on my server /srv/git/~foo/bar.git; the tilde does
not mean anything special--it is just a byte in a valid pathname.
I want to allow my users to say
git fetch git://my.server/~foo/bar.git
and fetch from that repository, but "git daemon" lacks the way
to configure to allow it.
If that is the case, what happens instead? Due to the leading
"~foo/" getting noticed as an attempt to use the user-path expansion
it is not treated as just a literal character?
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.
^ permalink raw reply
* Re: Git log exclude/remotes/branches options not working as expected
From: Junio C Hamano @ 2016-10-18 17:02 UTC (permalink / raw)
To: Robert Dailey; +Cc: Git
In-Reply-To: <CAHd499C+SACpBrOEk-V4QMKoq2egmMfYSwUN1tZBBhFOTWomRw@mail.gmail.com>
Robert Dailey <rcdailey.lists@gmail.com> writes:
> I have 3 remotes registered in my clone:
>
> origin, fork, drive
>
> When I do:
>
> $ git log --oneline --decorate --graph
>
> I only want to see branches under:
>
> refs/heads/*
> refs/remotes/origin/*
>
> I tried the following:
>
> $ git log --oneline --decorate --graph --simplify-by-decoration
> --remote=origin topic1..master
I am guessing that the above is not what you actually typed and
s/remote/remotes/ is what you did.
According to "git log --help":
--remotes[=<pattern>]
Pretend as if all the refs in refs/remotes are listed on the
command line as <commit>. If <pattern> is given, limit
remote-tracking branches to ones matching given shell glob. If
pattern lacks ?, *, or [, /* at the end is implied.
So your command line is equivalent to
$ git log --oneline --decorate --graph --simplify-by-decoration \
refs/remotes/origin/master refs/remotes/origin/topic ... \
topic1..master
(replace the second line with all the remote-tracking branches you
have for "origin").
There is nothing that tells "--decorate" which refs to base its
decoration on, so it is reasonable to expect that a commit that
happens to match a tip of remote-tracking branches from other
remotes are decorated as such, and I think that is what you are
seeing.
> What I'm expecting is that I literally see NONE of those excluded refs
> ... How can I achieve my goals?
The decorations are added in log-tree.c::load_ref_decorations() and
it does not get anything that says "I want to see decorations based
on only these refs", so a short answer is that there is no canned
way to do this in today's codebase.
Having said that, I would say it is a reasonable new feature to
have. Another related wishlist item might say "decorate only based
on tags, not branches or remote-tracking refs".
You can achieve your goals by teaching that codepath to take such
new pieces of information, come up with a new command line option
[*1*] and add a code to parse your command line option to either
builtin/log.c or revisions.c and pass it down the callchain that
leads to load_ref_decorations().
[Footnote]
*1* Unfortunately "--decorate=<option>" is already taken to specify
the decoration levels, so you would need a different one.
^ permalink raw reply
* Re: git checkout crashes after server being updated to Debian X86_64
From: Raffael Reichelt @ 2016-10-18 17:00 UTC (permalink / raw)
To: René Scharfe; +Cc: git
In-Reply-To: <1c70da04-d26b-f0af-8939-edc96de586d9@web.de>
Hello Renè!
file is returning
/usr/bin/git: ELF 64-bit LSB executable, x86-64, version 1 (SYSV), dynamically linked, interpreter /lib64/ld-linux-x86-64.so.2, for GNU/Linux 2.6.32, BuildID[sha1]=ee62e538d6fe6673d3ba49f0e66bfec784cc32bc, stripped
and ulimit is:
core file size (blocks, -c) 0
data seg size (kbytes, -d) unlimited
scheduling priority (-e) 1
file size (blocks, -f) unlimited
pending signals (-i) 16382
max locked memory (kbytes, -l) 64
max memory size (kbytes, -m) unlimited
open files (-n) 512
pipe size (512 bytes, -p) 8
POSIX message queues (bytes, -q) 819200
real-time priority (-r) 0
stack size (kbytes, -s) 8192
cpu time (seconds, -t) 1800
max user processes (-u) 42
virtual memory (kbytes, -v) 786432
file locks (-x) unlimited
Support told me git is limited to 600M
Regrads,
Rafael
> Am 18.10.2016 um 18:42 schrieb René Scharfe <l.s.r@web.de>:
>
> Am 18.10.2016 um 17:17 schrieb Raffael Reichelt:
>> 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
>>
>> or
>> fatal: unable to create threaded lstat
>> fatal: recursion detected in die handler
>> or
>> fatal: unable to create threaded lstat
>> *** Error in `git': double free or corruption (fasttop): 0x0000000000a8ade0 ***
>> fatal: recursion detected in die handler
>> Aborted
>>
>> It’s obviously not a problem of the repository - happens with all of
>> them. I think it is also not a question of size - happens with a 80M
>> Repository as well as with a 500M one.
>>
>> Any way: did a
>>
>> git fsck
>> Prüfe Objekt-Verzeichnisse: 100% (256/256), Fertig.
>> Prüfe Objekte: 100% (56305/56305), Fertig.
>>
>> git gc --auto --prune=today —aggressive
>> git repack
>>
>> Additionally I played around some config parameters so my config now looks like:
>> [http]
>> postbuffer = 524288000
>> [pack]
>> threads = 1
>> deltaCacheSize = 128m
>> packSizeLimit = 128m
>> windowMemory = 128m
>> [core]
>> packedGitLimit = 128m
>> packedGitWindowSize = 128m
>> repositoryformatversion = 0
>> filemode = true
>> bare = true
>>
>> I am running
>> git version 2.1.4
>>
>> on
>> Linux infongp-de65 3.14.0-ui16196-uiabi1-infong-amd64 #1 SMP Debian 3.14.73-2~ui80+4 (2016-07-13) x86_64 GNU/Linux
>>
>> Anyone out there to help me getting out of this trouble?
>
> Git 2.1.4 is the version that comes with Debian stable according to https://packages.debian.org/jessie/git, so I guess using a more recent version is not a reasonable option.
>
> What do "file $(which git)" and "ulimit -a" return? Do you have an x86-64 binary and no unnecessarily low limits set?
>
> René
^ permalink raw reply
* Re: git checkout crashes after server being updated to Debian X86_64
From: René Scharfe @ 2016-10-18 16:42 UTC (permalink / raw)
To: Raffael Reichelt, git
In-Reply-To: <6B2BE996-F696-4EB4-91CA-849D40B8802D@gmail.com>
Am 18.10.2016 um 17:17 schrieb Raffael Reichelt:
> 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
>
> or
> fatal: unable to create threaded lstat
> fatal: recursion detected in die handler
> or
> fatal: unable to create threaded lstat
> *** Error in `git': double free or corruption (fasttop): 0x0000000000a8ade0 ***
> fatal: recursion detected in die handler
> Aborted
>
> It’s obviously not a problem of the repository - happens with all of
> them. I think it is also not a question of size - happens with a 80M
> Repository as well as with a 500M one.
>
> Any way: did a
>
> git fsck
> Prüfe Objekt-Verzeichnisse: 100% (256/256), Fertig.
> Prüfe Objekte: 100% (56305/56305), Fertig.
>
> git gc --auto --prune=today —aggressive
> git repack
>
> Additionally I played around some config parameters so my config now looks like:
> [http]
> postbuffer = 524288000
> [pack]
> threads = 1
> deltaCacheSize = 128m
> packSizeLimit = 128m
> windowMemory = 128m
> [core]
> packedGitLimit = 128m
> packedGitWindowSize = 128m
> repositoryformatversion = 0
> filemode = true
> bare = true
>
> I am running
> git version 2.1.4
>
> on
> Linux infongp-de65 3.14.0-ui16196-uiabi1-infong-amd64 #1 SMP Debian 3.14.73-2~ui80+4 (2016-07-13) x86_64 GNU/Linux
>
> Anyone out there to help me getting out of this trouble?
Git 2.1.4 is the version that comes with Debian stable according to
https://packages.debian.org/jessie/git, so I guess using a more recent
version is not a reasonable option.
What do "file $(which git)" and "ulimit -a" return? Do you have an
x86-64 binary and no unnecessarily low limits set?
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