* Re: Issue 323 in msysgit: Can't clone over http
From: Junio C Hamano @ 2009-09-07 7:10 UTC (permalink / raw)
To: Tay Ray Chuan; +Cc: git, msysgit, Tom Preston-Werner
In-Reply-To: <be6fef0d0909062253p1b86628et8a9f979952eebb00@mail.gmail.com>
Tay Ray Chuan <rctay89@gmail.com> writes:
> We should only be interested in the MISSING_TARGET error, because it
> tells us that the pack file is indeed not available. We aren't
> interested in other errors, like being unable to perform the request
> (HTTP_START_FAILED), or, say, a 401 (Unauthorized) error, or even a
> 500; we simply move along and we tell the user we couldn't perform the
> check.
We couldn't perform the check, and then what happens? We continue as if
everything is peachy, assuming that the *.idx file we thought we were
going to get describe what objects are in the corresponding pack, and barf
when we try to read the *.idx file that we failed to download even though
the server said we should be able to get it?
> You're right to say that git before 39dc52c would have failed. It did,
> but no one had the chance to break anything, because 39dc52c was part
> of my http patch series that only went wild in v1.6.4.
>
> We can trace this back to 48188c2 ("http-walker: verify remote
> packs"), which copied the "feature" from http-push.c to http-walker.c.
Ahh, v1.6.3 ships with fetch_index() that checks CURLE_OK and returns an
error(), but that is about .idx file, and it did not have the "do they
have the corresponding .pack file?" check introduced by 48188c2
(http-walker: verify remote packs, 2009-06-06), which is what makes the
server give us 500 error. Before that check, we ignored a request to
fetch_index() if we already had one.
Why do we even call fetch_index() when we have one? After all, we are
talking about "git clone" here, so it is not about "we failed once and the
previous attempt left .idx files we fetched". Why
should we even have .idx file to begin with, that would have protected
v1.6.3 clients from getting this error?
Unless we are calling fetch_index() on the same .idx file twice from our
own stupidity, that is.
The same logic now is in fetch_pack_index(), which is called from
fetch_and_setup_pack_index(). I do not still see how we end up calling
the function for the same .idx file twice, though.
The repository in question http://github.com/tekkub/addontemplate.git/
return one liner file when asked for $URL/objects/info/packs.
Which means that it is not like that the loop in http_get_info_packs() is
calling the fetch_and_setup_pack() twice because the server lists the same
pack twice. But if your patch matters, somebody is causing us to call the
function twice for the same .idx file, and I do not see where it is.
There definitely is something else going on.
Having said all that, after digging some more, I came to the conclusion
that I'd rather not see us proceed with bug hunting, based on the failures
we see with the current github repositories over http. Read on for the
reasons.
The github's URL responds to request for $URL/HEAD and tells us that it
points at refs/heads/master, but requests for $URL/packed-refs and
$URL/refs/heads/master go to somewhere completely unrelated to the
request, without giving any failure indication.
In order to support fetch/clone over HTTP, a server at least must respond
to requests to the follwoing locations sensibly, meaning that it gives us
back the data without frills if they exist, and give us Not found if they
don't.
- $URL/objects/info/refs
This must list all the refs available in the repository and must be
up-to-date. We do not run PROPFIND, nor parse $URL/refs/index.htm, but
trust this file's contents and start from there.
- $URL/objects/info/packs
This must list all the packs in the repository and must be up-to-date.
We do not run PROPFIND, nor parse $URL/objects/pack/index.htm, but
trust this file's contents. If the repository does not have any pack,
request to this file must give us Not found.
- $URL/packed-refs
This may be a valid packed-refs file left after "git pack-refs". If
the repository's refs are not packed, the file may not exist, and that
is Ok, but in that case, the request must give us Not found.
- $URL/objects/pack/pack-*.pack and pack-*.idx
If the repository lacks what we asked for , the request must result in Not
found.
- $URL/objects/[0-9a-f][0-9][a-f]/*
Loose objects. If the repository lacks what we asked for, the request
must result in Not found.
It appears that github always redirects the request to some random project
page when Not found response is expected, which is very broken from the
point of view of git-fetch/git-clone.
I cannot tell offhand if it is just this "addontemplate.git" repository,
or if the way github arranges the URL space is fundamentally broken and
all of their repositories are unusable in exactly the same way (their URL
space seems to overlay UI pages meant for browsers over output meant to be
read by git-fetch/git-clone).
In either case, cloning over http from that "addontemplate" URL is not
expected to work right now, and the primary reason is that the server is
utterly misbehaving.
Hopefully that is a temporary breakage something github folks can promptly
fix. Tom, could you have your server folks in touch with the git mailing
list, so that we can figure out what is going on? Or are they already on
top of this issue and we can just wait (hopefully not for too long)?
In the meantime, I do not think it is a good idea to loosen the error
checking on our side to accomodate a server in such a (hopefully
temporary) broken state, however popular it is.
^ permalink raw reply
* [PATCH 2/2] Add url.<base>.pushInsteadOf: URL rewriting for push only
From: Josh Triplett @ 2009-09-07 7:00 UTC (permalink / raw)
To: git, gitster
In-Reply-To: <cover.1252306396.git.josh@joshtriplett.org>
This configuration option allows systematically rewriting fetch-only
URLs to push-capable URLs when used with push. For instance:
[url "ssh://example.org/"]
pushInsteadOf = "git://example.org/"
This will allow clones of "git://example.org/path/to/repo" to
subsequently push to "ssh://example.org/path/to/repo", without manually
configuring pushurl for that remote.
Includes documentation for the new option, bash completion updates, and
test cases (both that pushInsteadOf applies to push and that it does
*not* apply to fetch).
Signed-off-by: Josh Triplett <josh@joshtriplett.org>
---
Documentation/config.txt | 12 +++++++++++
Documentation/urls.txt | 18 +++++++++++++++++
contrib/completion/git-completion.bash | 2 +-
remote.c | 33 +++++++++++++++++++++++--------
t/t5516-fetch-push.sh | 31 ++++++++++++++++++++++++++++++
5 files changed, 86 insertions(+), 10 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 5256c7f..726aa89 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1500,6 +1500,18 @@ url.<base>.insteadOf::
never-before-seen repository on the site. When more than one
insteadOf strings match a given URL, the longest match is used.
+url.<base>.pushInsteadOf::
+ Any URL that starts with this value will not be pushed to;
+ instead, it will be rewritten to start with <base>, and the
+ resulting URL will be pushed to. In cases where some site serves
+ a large number of repositories, and serves them with multiple
+ access methods, some of which do not allow push, this feature
+ allows people to specify a pull-only URL and have git
+ automatically use an appropriate URL to push, even for a
+ never-before-seen repository on the site. When more than one
+ pushInsteadOf strings match a given URL, the longest match is
+ used.
+
user.email::
Your email address to be recorded in any newly created commits.
Can be overridden by the 'GIT_AUTHOR_EMAIL', 'GIT_COMMITTER_EMAIL', and
diff --git a/Documentation/urls.txt b/Documentation/urls.txt
index 5355ebc..d813ceb 100644
--- a/Documentation/urls.txt
+++ b/Documentation/urls.txt
@@ -67,3 +67,21 @@ For example, with this:
a URL like "work:repo.git" or like "host.xz:/path/to/repo.git" will be
rewritten in any context that takes a URL to be "git://git.host.xz/repo.git".
+If you want to rewrite URLs for push only, you can create a
+configuration section of the form:
+
+------------
+ [url "<actual url base>"]
+ pushInsteadOf = <other url base>
+------------
+
+For example, with this:
+
+------------
+ [url "ssh://example.org/"]
+ pushInsteadOf = git://example.org/
+------------
+
+a URL like "git://example.org/path/to/repo.git" will be rewritten to
+"ssh://example.org/path/to/repo.git" for pushes, but pulls will still
+use the original URL.
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index bf688e1..9859204 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1532,7 +1532,7 @@ _git_config ()
url.*.*)
local pfx="${cur%.*}."
cur="${cur##*.}"
- __gitcomp "insteadof" "$pfx" "$cur"
+ __gitcomp "insteadOf pushInsteadOf" "$pfx" "$cur"
return
;;
esac
diff --git a/remote.c b/remote.c
index ff8e71f..6789786 100644
--- a/remote.c
+++ b/remote.c
@@ -47,6 +47,7 @@ static const char *default_remote_name;
static int explicit_default_remote_name;
static struct rewrites rewrites;
+static struct rewrites rewrites_push;
#define BUF_SIZE (2048)
static char buffer[BUF_SIZE];
@@ -104,17 +105,25 @@ static void add_url(struct remote *remote, const char *url)
remote->url[remote->url_nr++] = url;
}
-static void add_url_alias(struct remote *remote, const char *url)
-{
- add_url(remote, alias_url(url, &rewrites));
-}
-
static void add_pushurl(struct remote *remote, const char *pushurl)
{
ALLOC_GROW(remote->pushurl, remote->pushurl_nr + 1, remote->pushurl_alloc);
remote->pushurl[remote->pushurl_nr++] = pushurl;
}
+static void add_pushurl_alias(struct remote *remote, const char *url)
+{
+ const char *pushurl = alias_url(url, &rewrites_push);
+ if (pushurl != url)
+ add_pushurl(remote, pushurl);
+}
+
+static void add_url_alias(struct remote *remote, const char *url)
+{
+ add_url(remote, alias_url(url, &rewrites));
+ add_pushurl_alias(remote, url);
+}
+
static struct remote *make_remote(const char *name, int len)
{
struct remote *ret;
@@ -358,8 +367,13 @@ static int handle_config(const char *key, const char *value, void *cb)
subkey = strrchr(name, '.');
if (!subkey)
return 0;
- rewrite = make_rewrite(&rewrites, name, subkey - name);
if (!strcmp(subkey, ".insteadof")) {
+ rewrite = make_rewrite(&rewrites, name, subkey - name);
+ if (!value)
+ return config_error_nonbool(key);
+ add_instead_of(rewrite, xstrdup(value));
+ } else if (!strcmp(subkey, ".pushinsteadof")) {
+ rewrite = make_rewrite(&rewrites_push, name, subkey - name);
if (!value)
return config_error_nonbool(key);
add_instead_of(rewrite, xstrdup(value));
@@ -435,12 +449,13 @@ static void alias_all_urls(void)
for (i = 0; i < remotes_nr; i++) {
if (!remotes[i])
continue;
- for (j = 0; j < remotes[i]->url_nr; j++) {
- remotes[i]->url[j] = alias_url(remotes[i]->url[j], &rewrites);
- }
for (j = 0; j < remotes[i]->pushurl_nr; j++) {
remotes[i]->pushurl[j] = alias_url(remotes[i]->pushurl[j], &rewrites);
}
+ for (j = 0; j < remotes[i]->url_nr; j++) {
+ remotes[i]->url[j] = alias_url(remotes[i]->url[j], &rewrites);
+ add_pushurl_alias(remotes[i], remotes[i]->url[j]);
+ }
}
}
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 2d2633f..8f455c7 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -122,6 +122,23 @@ test_expect_success 'fetch with insteadOf' '
)
'
+test_expect_success 'fetch with pushInsteadOf (should not rewrite)' '
+ mk_empty &&
+ (
+ TRASH=$(pwd)/ &&
+ cd testrepo &&
+ git config "url.trash/.pushInsteadOf" "$TRASH" &&
+ git config remote.up.url "$TRASH." &&
+ git config remote.up.fetch "refs/heads/*:refs/remotes/origin/*" &&
+ git fetch up &&
+
+ r=$(git show-ref -s --verify refs/remotes/origin/master) &&
+ test "z$r" = "z$the_commit" &&
+
+ test 1 = $(git for-each-ref refs/remotes/origin | wc -l)
+ )
+'
+
test_expect_success 'push without wildcard' '
mk_empty &&
@@ -162,6 +179,20 @@ test_expect_success 'push with insteadOf' '
)
'
+test_expect_success 'push with pushInsteadOf' '
+ mk_empty &&
+ TRASH="$(pwd)/" &&
+ git config "url.$TRASH.pushInsteadOf" trash/ &&
+ git push trash/testrepo refs/heads/master:refs/remotes/origin/master &&
+ (
+ cd testrepo &&
+ r=$(git show-ref -s --verify refs/remotes/origin/master) &&
+ test "z$r" = "z$the_commit" &&
+
+ test 1 = $(git for-each-ref refs/remotes/origin | wc -l)
+ )
+'
+
test_expect_success 'push with matching heads' '
mk_test heads/master &&
--
1.6.3.3
^ permalink raw reply related
* [PATCH 1/2] Wrap rewrite globals in a struct in preparation for adding another set
From: Josh Triplett @ 2009-09-07 7:00 UTC (permalink / raw)
To: git, gitster
In-Reply-To: <cover.1252306396.git.josh@joshtriplett.org>
remote.c has a global set of URL rewrites, accessed by alias_url and
make_rewrite. Wrap them in a new "struct rewrites", passed to alias_url
and make_rewrite. This allows adding other sets of rewrites.
Signed-off-by: Josh Triplett <josh@joshtriplett.org>
---
remote.c | 53 ++++++++++++++++++++++++++++-------------------------
1 files changed, 28 insertions(+), 25 deletions(-)
diff --git a/remote.c b/remote.c
index 4b5b905..ff8e71f 100644
--- a/remote.c
+++ b/remote.c
@@ -28,6 +28,11 @@ struct rewrite {
int instead_of_nr;
int instead_of_alloc;
};
+struct rewrites {
+ struct rewrite **rewrite;
+ int rewrite_alloc;
+ int rewrite_nr;
+};
static struct remote **remotes;
static int remotes_alloc;
@@ -41,14 +46,12 @@ static struct branch *current_branch;
static const char *default_remote_name;
static int explicit_default_remote_name;
-static struct rewrite **rewrite;
-static int rewrite_alloc;
-static int rewrite_nr;
+static struct rewrites rewrites;
#define BUF_SIZE (2048)
static char buffer[BUF_SIZE];
-static const char *alias_url(const char *url)
+static const char *alias_url(const char *url, struct rewrites *r)
{
int i, j;
char *ret;
@@ -57,14 +60,14 @@ static const char *alias_url(const char *url)
longest = NULL;
longest_i = -1;
- for (i = 0; i < rewrite_nr; i++) {
- if (!rewrite[i])
+ for (i = 0; i < r->rewrite_nr; i++) {
+ if (!r->rewrite[i])
continue;
- for (j = 0; j < rewrite[i]->instead_of_nr; j++) {
- if (!prefixcmp(url, rewrite[i]->instead_of[j].s) &&
+ for (j = 0; j < r->rewrite[i]->instead_of_nr; j++) {
+ if (!prefixcmp(url, r->rewrite[i]->instead_of[j].s) &&
(!longest ||
- longest->len < rewrite[i]->instead_of[j].len)) {
- longest = &(rewrite[i]->instead_of[j]);
+ longest->len < r->rewrite[i]->instead_of[j].len)) {
+ longest = &(r->rewrite[i]->instead_of[j]);
longest_i = i;
}
}
@@ -72,10 +75,10 @@ static const char *alias_url(const char *url)
if (!longest)
return url;
- ret = xmalloc(rewrite[longest_i]->baselen +
+ ret = xmalloc(r->rewrite[longest_i]->baselen +
(strlen(url) - longest->len) + 1);
- strcpy(ret, rewrite[longest_i]->base);
- strcpy(ret + rewrite[longest_i]->baselen, url + longest->len);
+ strcpy(ret, r->rewrite[longest_i]->base);
+ strcpy(ret + r->rewrite[longest_i]->baselen, url + longest->len);
return ret;
}
@@ -103,7 +106,7 @@ static void add_url(struct remote *remote, const char *url)
static void add_url_alias(struct remote *remote, const char *url)
{
- add_url(remote, alias_url(url));
+ add_url(remote, alias_url(url, &rewrites));
}
static void add_pushurl(struct remote *remote, const char *pushurl)
@@ -169,22 +172,22 @@ static struct branch *make_branch(const char *name, int len)
return ret;
}
-static struct rewrite *make_rewrite(const char *base, int len)
+static struct rewrite *make_rewrite(struct rewrites *r, const char *base, int len)
{
struct rewrite *ret;
int i;
- for (i = 0; i < rewrite_nr; i++) {
+ for (i = 0; i < r->rewrite_nr; i++) {
if (len
- ? (len == rewrite[i]->baselen &&
- !strncmp(base, rewrite[i]->base, len))
- : !strcmp(base, rewrite[i]->base))
- return rewrite[i];
+ ? (len == r->rewrite[i]->baselen &&
+ !strncmp(base, r->rewrite[i]->base, len))
+ : !strcmp(base, r->rewrite[i]->base))
+ return r->rewrite[i];
}
- ALLOC_GROW(rewrite, rewrite_nr + 1, rewrite_alloc);
+ ALLOC_GROW(r->rewrite, r->rewrite_nr + 1, r->rewrite_alloc);
ret = xcalloc(1, sizeof(struct rewrite));
- rewrite[rewrite_nr++] = ret;
+ r->rewrite[r->rewrite_nr++] = ret;
if (len) {
ret->base = xstrndup(base, len);
ret->baselen = len;
@@ -355,7 +358,7 @@ static int handle_config(const char *key, const char *value, void *cb)
subkey = strrchr(name, '.');
if (!subkey)
return 0;
- rewrite = make_rewrite(name, subkey - name);
+ rewrite = make_rewrite(&rewrites, name, subkey - name);
if (!strcmp(subkey, ".insteadof")) {
if (!value)
return config_error_nonbool(key);
@@ -433,10 +436,10 @@ static void alias_all_urls(void)
if (!remotes[i])
continue;
for (j = 0; j < remotes[i]->url_nr; j++) {
- remotes[i]->url[j] = alias_url(remotes[i]->url[j]);
+ remotes[i]->url[j] = alias_url(remotes[i]->url[j], &rewrites);
}
for (j = 0; j < remotes[i]->pushurl_nr; j++) {
- remotes[i]->pushurl[j] = alias_url(remotes[i]->pushurl[j]);
+ remotes[i]->pushurl[j] = alias_url(remotes[i]->pushurl[j], &rewrites);
}
}
}
--
1.6.3.3
^ permalink raw reply related
* [PATCH 0/2] Add url.<base>.pushInsteadOf: URL rewriting for push only
From: Josh Triplett @ 2009-09-07 6:59 UTC (permalink / raw)
To: git, gitster
Many sites host repositories via both git:// for fetch-only anonymous
access and ssh:// for push-capable access. The "insteadOf" mechanism
makes it straightforward to substitute the push-capable URLs for the
pull-only URLs, which proves convenient when the site hosts many
repositories using the same URL scheme. However, if you use such a
substitution and you cannot use the ssh:// URLs (either because you
don't have SSH access or you don't have permission to a particular
repository), you cannot clone or fetch either, even though you could do
so via the git:// URLs. A situation like this arises when sharing git
configuration files between systems, of which only a few have SSH access
to repositories.
"pushurl" provides a way to specify URLs used only for push, but this
requires configuring a pushurl for each such repository. As in the
rationale for insteadOf, it makes sense to configure this for all
repositories hosted on a given system at once.
This patch series adds a new "pushInsteadOf" option to go with
"insteadOf". pushInsteadOf allows systematically rewriting fetch-only
URLs to push-capable URLs when used with push. For instance:
[url "ssh://example.org/"]
pushInsteadOf = "git://example.org/"
This will allow clones of "git://example.org/path/to/repo" to
subsequently push to "ssh://example.org/path/to/repo", without manually
configuring pushurl for that remote.
Includes documentation for the new option, bash completion updates, and
test cases (both that pushInsteadOf applies to push and that it does
*not* apply to fetch).
Josh Triplett (2):
Wrap rewrite globals in a struct in preparation for adding another set
Add url.<base>.pushInsteadOf: URL rewriting for push only
Documentation/config.txt | 12 +++++
Documentation/urls.txt | 18 +++++++
contrib/completion/git-completion.bash | 2 +-
remote.c | 80 +++++++++++++++++++------------
t/t5516-fetch-push.sh | 31 ++++++++++++
5 files changed, 111 insertions(+), 32 deletions(-)
^ permalink raw reply
* Re: [BUG] 'add -u' doesn't work from untracked subdir
From: Matthieu Moy @ 2009-09-07 6:23 UTC (permalink / raw)
To: Clemens Buchacher; +Cc: Junio C Hamano, Jeff King, SZEDER Gábor, git
In-Reply-To: <20090906181621.GA23412@localhost>
Clemens Buchacher <drizzd@aon.at> writes:
> On Sun, Sep 06, 2009 at 02:32:44PM +0200, Matthieu Moy wrote:
>> I think it has already been proposed to introduce "git add -a" doing
>> what "git add -u" do, but for the full tree.
>
> I like that, actually. AFAICT it's completely analogous to "git commit -a".
> We also need something for "git add -A" though.
>
> Do you feel the same way about changing the behavior of "git grep"? I don't
> really want to change the command's name.
I don't have particular feeling about "git grep", probably because I
don't use it much. One argument in favour of keeping the current
behavior is the consistancy with plain "grep".
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply
* Re: Issue 323 in msysgit: Can't clone over http
From: Tay Ray Chuan @ 2009-09-07 5:53 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, msysgit
In-Reply-To: <7v8wgrbb9e.fsf@alter.siamese.dyndns.org>
Hi,
On Mon, Sep 7, 2009 at 12:59 PM, Junio C Hamano<gitster@pobox.com> wrote:
> Before 39dc52c (http: use new http API in fetch_index(), 2009-06-06), we
> used to run the slot by hand and checked results.curl_request against
> CURLE_OK. Everything else was an error.
>
> With the updated code, that all went to http_get_strbuf() which in turn
> calls http_request() that does the same thing, and the function returns
> HTTP_OK only when it gets CURLE_OK, but now it says MISSING_TARGET when we
> ask for an idx file we think exists in the repository but the server says
> it doesn't, and all other errors will be ignored with this patch.
We should only be interested in the MISSING_TARGET error, because it
tells us that the pack file is indeed not available. We aren't
interested in other errors, like being unable to perform the request
(HTTP_START_FAILED), or, say, a 401 (Unauthorized) error, or even a
500; we simply move along and we tell the user we couldn't perform the
check.
> If this codepath is what was broken by github returning 500 [*1*], the
> client before 39dc52c would have failed the same way. I do not think
> loosening error checking like this is a real solution, but I may be
> reading the patch incorrectly.
You're right to say that git before 39dc52c would have failed. It did,
but no one had the chance to break anything, because 39dc52c was part
of my http patch series that only went wild in v1.6.4.
We can trace this back to 48188c2 ("http-walker: verify remote
packs"), which copied the "feature" from http-push.c to http-walker.c.
Before that, if you tried fetching a pack with http-push.c from a
500-prone server, you would also experience this.
--
Cheers,
Ray Chuan
^ permalink raw reply
* Re: [BUG] 'add -u' doesn't work from untracked subdir
From: Junio C Hamano @ 2009-09-07 5:07 UTC (permalink / raw)
To: Nanako Shiraishi; +Cc: Clemens Buchacher, Jeff King, SZEDER Gbor, git
In-Reply-To: <20090907090713.6117@nanako3.lavabit.com>
Nanako Shiraishi <nanako3@lavabit.com> writes:
[jc: sometimes but not always your messages have looooooooooooooong lines.
please line-wrap for readability. I learned to type W Q so often I do not
complain but other people would find them irritating.]
> The default behavior for 'git-grep' has already been discussed in
> length, and I don't think it is likely to change. See
>
> http://thread.gmane.org/gmane.comp.version-control.git/111519/focus=111717
Interesting.
The first message in that thread lists things we have scheduled for 1.7.0
but it has another item. It is an off-topic for the thread, but we might
want to resurrect the "core.quotepath defaults to false" proposal, before
it gets too late for 1.7.0.
As to "grep", I am open to the proposal to make git commands consistent by
letting them operate on everything when no path argument is given, if grep
is really the single odd-man out remaining after changing the "add -u" and
"add -A" to work on the whole tree from subdirectories when given no paths.
> The original design for the other two in your list was to be a whole
> tree operation. This commit broke it.
>
> 2ed2c22 "git-add -u paths... now works from subdirectory".
> ...
> I think it is a good idea to fix this as an old regression in the maint
> branch. You don't have to introduce "git add -a". In fact the -a option
> was explicitly rejected when "git add -A" option was added with this
> commit.
Geez, you are good at digging things up.
It is very tempting to follow the suggestion above, but I suspect that,
even though it may be a regression from historical point of view, some
people who are used to the current behaviour may look at the corrected
one as a regression. We've had the change by 2ed2c22 for a long time.
I dunno.
^ permalink raw reply
* Re: tracking branch for a rebase
From: Junio C Hamano @ 2009-09-07 5:05 UTC (permalink / raw)
To: Jeff King; +Cc: Björn Steinbrink, Michael J Gruber, Pete Wyckoff, git
In-Reply-To: <20090905142841.GB15631@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
>> Hm, I'd prefer a shorthand for "upstream for this branch", instead of
>> magic defaults.
>
> The more I think about, the more I think that is the right solution.
> Because magic defaults for "rebase -i" don't help when you want to do
> "gitk $UPSTREAM..".
>
> The previous discussion on the topic seems to be here:
>
> http://article.gmane.org/gmane.comp.version-control.git/113666
>
> And apparently you and I both participated in the discussion, which I
> totally forgot about.
>
> Looks like the discussion ended with people liking the idea but not
> knowing what the specifier should look like. Maybe tightening the ref
> syntax a bit to allow more extensible "special" refs is a good v1.7.0
> topic? I dunno.
At-mark currently is reserved for anything that uses reflog, but we can
say that it is to specify operations on refs (as opposed to caret and
tilde are to specify operations on object names).
It specifies what ref to work on with the operand on its left side (and an
empty string stands for "HEAD"), and what operation is done to it by what
is in {} on the right side of it. This view is quite consistent with the
following existing uses of the notation:
ref@{number} -- nth reflog entry
ref@{time} -- ref back then
@{-number} -- nth branch switching
So perhaps ref@{upstream}, or any string that is not a number and cannot
be time, can trigger the magic operation on the ref with ref@{magic}
syntax?
^ permalink raw reply
* Re: Issue 323 in msysgit: Can't clone over http
From: Junio C Hamano @ 2009-09-07 4:59 UTC (permalink / raw)
To: Tay Ray Chuan; +Cc: git, msysgit
In-Reply-To: <20090904212956.f02b0c60.rctay89@gmail.com>
Tay Ray Chuan <rctay89@gmail.com> writes:
> Subject: [PATCH] http.c: clarify missing-pack-check
>
> Abort the pack transfer only if the pack is not available in the HTTP-
> served repository; otherwise, allow the transfer to continue, even if
> the check failed.
>
> This addresses an issue raised by bjelli:
>
> http://code.google.com/p/msysgit/issues/detail?id=323
>
> Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>
> ---
> http.c | 10 +++++++---
> 1 files changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/http.c b/http.c
> index 5926c5b..cba7e9a 100644
> --- a/http.c
> +++ b/http.c
> @@ -864,6 +864,7 @@ int http_fetch_ref(const char *base, struct ref *ref)
> static int fetch_pack_index(unsigned char *sha1, const char *base_url)
> {
> int ret = 0;
> + int result;
> char *hex = xstrdup(sha1_to_hex(sha1));
> char *filename;
> char *url;
> @@ -874,11 +875,14 @@ static int fetch_pack_index(unsigned char *sha1, const char *base_url)
> strbuf_addf(&buf, "objects/pack/pack-%s.pack", hex);
> url = strbuf_detach(&buf, 0);
>
> - if (http_get_strbuf(url, NULL, 0)) {
> - ret = error("Unable to verify pack %s is available",
> + result = http_get_strbuf(url, NULL, 0);
> + if (result == HTTP_MISSING_TARGET) {
> + ret = error("Unable to find pack %s",
> hex);
> goto cleanup;
> - }
> + } else if (result && http_is_verbose)
> + fprintf(stderr, "Unable to verify pack %s is available\n",
> + hex);
>
> if (has_pack_index(sha1)) {
> ret = 0;
You said:
> Releases including and after v1.6.4 will have this issue:
>
>>> error: Unable to verify pack 382c25c935b744e909c749532578112d72a4aff9 is
>>> available
>>> error: Unable to find 0a41ac04d56ccc96491989dc71d9875cd804fc6b under
>>> http://github.com/tekkub/addontemplate.git
>
> The issue at hand is due to git checking the http repository for the
> pack file before commencing the transfer; failing which, the transfer
> aborts.
>
> Right now, git chokes on the 500 error that github.com gives it, which
> shouldn't be the case, even though that's a weird response.
I am assuming that you meant 39dc52c was the culprit by "including and
after v1.6.4", but it is not quite clear how this patch helps if that is
the case.
Before 39dc52c (http: use new http API in fetch_index(), 2009-06-06), we
used to run the slot by hand and checked results.curl_request against
CURLE_OK. Everything else was an error.
With the updated code, that all went to http_get_strbuf() which in turn
calls http_request() that does the same thing, and the function returns
HTTP_OK only when it gets CURLE_OK, but now it says MISSING_TARGET when we
ask for an idx file we think exists in the repository but the server says
it doesn't, and all other errors will be ignored with this patch.
If this codepath is what was broken by github returning 500 [*1*], the
client before 39dc52c would have failed the same way. I do not think
loosening error checking like this is a real solution, but I may be
reading the patch incorrectly.
Do people on the github side see something strange in the log? Perhaps we
think we are making a request to objects/pack/ of the repository but by
mistake the request is going to somewhere completely off (but then we
would get 401 not 500).
[Footnote]
*1* Which I do agree is somewhat strange thing to say for a request to a
file in the objects/pack directory in a public repository---I would
understand it if it were 404, but then it means the repository is
inconsistent, i.e. has a stale objects/info/packs relative to its set of
packs it has.
^ permalink raw reply
* Re: [PATCH 3/4] push: make non-fast-forward help message configurable
From: Nanako Shiraishi @ 2009-09-07 0:44 UTC (permalink / raw)
To: Junio C Hamano
Cc: Jeff King, Nanako Shiraishi, Matthieu Moy, Teemu Likonen, Git
In-Reply-To: <7vzl98fr22.fsf@alter.siamese.dyndns.org>
Quoting Junio C Hamano <gitster@pobox.com>
> Speaking of which, has anybody felt annoyed by this message?
>
> $ git reset --hard HEAD^^
> HEAD is now at 3fb9d58 Do not scramble password read from .cvspass
>
> This is not "maybe you should try this", but I would consider that it
> falls into the same "I see you are trying to be helpful, but I know what I
> am doing, and you are stealing screen real estate from me without helping
> me at all, thank you very much" category.
You may be fixated at the sha1 part of the message when you find this message annoying, but I disagree strongly. I always appreciate the assurance this message gives me that I counted the number of commits correctly, whether I say HEAD^^^^ or HEAD~7.
Showing the subject of the commit you are now at is very useful and I will be equally irritated as you do if it starts showing the subject of the commit I was at.
--
Nanako Shiraishi
http://ivory.ap.teacup.com/nanako3/
^ permalink raw reply
* Re: Use case I don't know how to address
From: Alan Chandler @ 2009-09-06 12:44 UTC (permalink / raw)
Cc: git
In-Reply-To: <7vk50ccxfj.fsf@alter.siamese.dyndns.org>
Junio C Hamano wrote:
> Alan Chandler <alan@chandlerfamily.org.uk> writes:
>
>> 2' - 2a - 3' - 4' ----------------- 6' SITE
>> / / / /
>> 1 - 2 ------ 3 - 4 ------------6'''- 6a TEST
>> \ /
>> 5 ------ 6 MASTER
>> \ \
>> 5''- 5a- 6'' DEMO
>>
>>
>> What will happen is the changes made in 4->5 will get applied to the
>> (now) Test branch as part of the 6->6'' merge, and I will be left
>> having to add a new commit, 6a, to undo them all again. Given this is
>> likely to be quite a substantial change I want to try and avoid it if
>> possible.
>
> I presume 6'''-6a has the revert of 4-5? If so, the next merge should
> work just fine.
I think you missed the issue - Yes 6'''-6a is the revert, but the
problem is this could be large and complicated, and I wanted to find an
automated way rather than manual
Sort of like doing a diff of 4-5 and somehow applying it backwards.
--
Alan Chandler
http://www.chandlerfamily.org.uk
^ permalink raw reply
* Re: [BUG] 'add -u' doesn't work from untracked subdir
From: Nanako Shiraishi @ 2009-09-07 0:07 UTC (permalink / raw)
To: Clemens Buchacher; +Cc: Junio C Hamano, Jeff King, SZEDER Gbor, git
In-Reply-To: <20090905084641.GA24865@darc.dnsalias.org>
Quoting Clemens Buchacher <drizzd@aon.at>
> Sorry for stating the obvious here, but the following commands affect the
> entire repository, even though they limit themselves to the current
> directory, if passed a '.'.
>
> git commit
> git log
> git diff
> git checkout
> git reset
>
> Due to the frequent use of these commands, I believe many users (myself
> included) expect "git add" and "git grep" to do the same. AFAICT the
> following commands are the only non-plumbing ones that behave differently:
>
> git add -u
> git add -A
> git grep
>
> So I argue that _that_ is the real inconsistency.
The default behavior for 'git-grep' has already been discussed in length, and I don't think it is likely to change. See
http://thread.gmane.org/gmane.comp.version-control.git/111519/focus=111717
The original design for the other two in your list was to be a whole tree operation. This commit broke it.
2ed2c22 "git-add -u paths... now works from subdirectory".
'git-add -u' in a subdirectory without any other argument used to work on the entire working tree before that commit, but it didn't prefix the current directory in front of the paths... arguments.
That commit 2ed2c22 fixed 'git-add -u paths...' by prepending the prefix to the arguments, but it broke 'git-add -u' to always limit the updates to the current directory.
I think it is a good idea to fix this as an old regression in the maint branch. You don't have to introduce "git add -a". In fact the -a option was explicitly rejected when "git add -A" option was added with this commit.
3ba1f11 "git-add --all: add all files"
because "git commit -a" will never include new files and it will be inconsistent if "git add -a" did so.
--
Nanako Shiraishi
http://ivory.ap.teacup.com/nanako3/
^ permalink raw reply
* Re: [PATCH 1/2] grep: accept relative paths outside current working directory
From: Junio C Hamano @ 2009-09-06 22:58 UTC (permalink / raw)
To: Clemens Buchacher; +Cc: Jeff King, SZEDER Gábor, git
In-Reply-To: <20090905123117.GA3099@darc.dnsalias.org>
Clemens Buchacher <drizzd@aon.at> writes:
> On Sat, Sep 05, 2009 at 12:58:50AM -0700, Junio C Hamano wrote:
>> >> If "git add -u ../.." (I mean "the grand parent directory", not "an
>> >> unnamed subdirectory") did not work
>
> In git.git:
>
> $ cd t
> $ git grep addremove -- ../
> fatal: git grep: cannot generate relative filenames containing '..'
>
> So here's a fix for that. And a configurable solution for add and grep's
> scope as a follow-up. I did not look at any other commands yet.
Thanks. This was oa breakage I pointed out in an earlier message in this
discussion, and it is worth fixing.
Your patch is queued in 'pu', but it seems to break the exit status in a
strange way with my limited test.
Here is a non-broken behaviour without the "look in uplevel":
: git.git/cb/maint-1.6.3-grep-relative-up; ./git grep adddelete .
: git.git/cb/maint-1.6.3-grep-relative-up; ./git grep adddelete .; echo $?
1
: git.git/cb/maint-1.6.3-grep-relative-up; ./git grep adddelete . >/dev/null; echo $?
1
Now we go down, and grep from an uplevel:
: git.git/cb/maint-1.6.3-grep-relative-up; cd t
: t/cb/maint-1.6.3-grep-relative-up; ../git grep adddelete ..
: t/cb/maint-1.6.3-grep-relative-up; ../git grep adddelete .. ; echo $?
1
: t/cb/maint-1.6.3-grep-relative-up; ../git grep adddelete .. >/dev/null; echo $?
0
The command should not give different exit status depending on the
destination of standard output stream.
^ permalink raw reply
* Re: Strange merge failure (would be overwritten by merge / cannot merge)
From: Linus Torvalds @ 2009-09-06 22:49 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git Mailing List
In-Reply-To: <alpine.LFD.2.01.0909061424160.8946@localhost.localdomain>
On Sun, 6 Sep 2009, Linus Torvalds wrote:
>
> So here's a slightly updated version, and it passes all the tests.
.. and here's something with a bit more abstraction, a bit more cleanup,
and making more sure that there's no semantic changes. So that I can feel
happy signing off on it.
Linus
---
From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Sun, 6 Sep 2009 14:37:21 -0700
Subject: [PATCH] Prepare 'traverse_trees()' for D/F conflict lookahead
traverse_trees() used to always walk the trees in order, and used the
special (and fundamentally broken) 'df_name_compare()' function to
compare directory and file entries as equal.
That works fine for all the common cases, when the D/F conflicts are
immediately adjacent, and there are no other entries that could confuse
the ordering. But if you have one tree with a file 'a', and another
tree with a file 'a-1' and a directory 'a/', then you would not see the
D/F conflict of 'a' and 'a/' without looking ahead past the 'a-1' file
in the other tree.
So this re-organizes the tree walking code so that we can start doing
look-ahead for those cases. It doesn't actually _do_ that lookahead
yet, because it requires marking the conflicts we've used, but the code
is now organized to do so.
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
---
tree-walk.c | 90 +++++++++++++++++++++++++++++++++++++++++++++++++++++++---
1 files changed, 85 insertions(+), 5 deletions(-)
diff --git a/tree-walk.c b/tree-walk.c
index 02e2aed..7251dd2 100644
--- a/tree-walk.c
+++ b/tree-walk.c
@@ -62,7 +62,7 @@ void *fill_tree_descriptor(struct tree_desc *desc, const unsigned char *sha1)
static int entry_compare(struct name_entry *a, struct name_entry *b)
{
- return df_name_compare(
+ return base_name_compare(
a->path, tree_entry_len(a->path, a->sha1), a->mode,
b->path, tree_entry_len(b->path, b->sha1), b->mode);
}
@@ -138,6 +138,80 @@ char *make_traverse_path(char *path, const struct traverse_info *info, const str
return path;
}
+/*
+ * See if 'entry' may conflict with a later tree entry in 't': if so,
+ * fill in 'conflict' with the conflicting tree entry from 't'.
+ *
+ * NOTE! Right now we do _not_ create a create a private copy of the tree
+ * descriptor, so we can't actually walk it any further without losing
+ * our place. We should change it to a loop over a copy of the tree
+ * descriptor, but then we'd also have to remember the skipped entries,
+ * so this is a hacky simple case that only handles the case we used
+ * to handle historically (ie clash in the very first entry)
+ *
+ * Note that only a regular file 'entry' can conflict with a later
+ * directory, since a directory with the same name will sort later.
+ */
+static int find_df_conflict(struct tree_desc *t, struct name_entry *entry, struct name_entry *conflict)
+{
+ int len;
+
+ if (S_ISDIR(entry->mode))
+ return 0;
+ len = tree_entry_len(entry->path, entry->sha1);
+
+ while (t->size) {
+ int nlen;
+
+ entry_extract(t, conflict);
+ nlen = tree_entry_len(conflict->path, conflict->sha1);
+
+ /*
+ * We can only have a future conflict if the entry matches the
+ * beginning of the name exactly, and if the next character is
+ * smaller than '/'.
+ *
+ * Break out otherwise.
+ */
+ if (nlen < len)
+ break;
+ if (memcmp(conflict->path, entry->path, nlen))
+ break;
+ if (nlen == len)
+ return 1;
+
+ if (conflict->path[len] > '/')
+ break;
+ /*
+ * FIXME! Here we'd really like to do 'update_tree_entry(©);'
+ * but that requires us to remember the conflict position specially
+ * so now we just punt and stop looking for conflicts
+ */
+ break;
+ }
+ entry_clear(conflict);
+ return 0;
+}
+
+/*
+ * For now, the used entries are always at the head of the tree_desc
+ * (no look-ahead), so marking an entry used is always just a matter
+ * of doing an 'update_tree_entry()'
+ */
+static void used_entry(struct tree_desc *t, struct name_entry *entry)
+{
+ update_tree_entry(t);
+}
+
+static int get_entry(struct tree_desc *t, struct name_entry *entry)
+{
+ if (t->size) {
+ entry_extract(t, entry);
+ return 1;
+ }
+ return 0;
+}
+
int traverse_trees(int n, struct tree_desc *t, struct traverse_info *info)
{
int ret = 0;
@@ -150,9 +224,8 @@ int traverse_trees(int n, struct tree_desc *t, struct traverse_info *info)
last = -1;
for (i = 0; i < n; i++) {
- if (!t[i].size)
+ if (!get_entry(t+i, entry+i))
continue;
- entry_extract(t+i, entry+i);
if (last >= 0) {
int cmp = entry_compare(entry+i, entry+last);
@@ -179,13 +252,20 @@ int traverse_trees(int n, struct tree_desc *t, struct traverse_info *info)
dirmask &= mask;
/*
- * Clear all the unused name-entries.
+ * Clear all the unused name-entries, and look for
+ * conflicts.
*/
for (i = 0; i < n; i++) {
if (mask & (1ul << i))
continue;
entry_clear(entry + i);
+ if (find_df_conflict(t+i, entry+last, entry+i))
+ dirmask |= (1ul << i);
}
+
+ /* Add in the DF conflict entries into the mask */
+ mask |= dirmask;
+
ret = info->fn(n, mask, dirmask, entry, info);
if (ret < 0)
break;
@@ -194,7 +274,7 @@ int traverse_trees(int n, struct tree_desc *t, struct traverse_info *info)
ret = 0;
for (i = 0; i < n; i++) {
if (mask & (1ul << i))
- update_tree_entry(t + i);
+ used_entry(t+i, entry+i);
}
}
free(entry);
^ permalink raw reply related
* Re: [git-svn] [FEATURE-REQ] track merges from git
From: Ximin Luo @ 2009-09-06 22:15 UTC (permalink / raw)
To: Eric Wong; +Cc: git
In-Reply-To: <20090905080337.GE22272@dcvr.yhbt.net>
Eric Wong wrote:
> You may want to try the "set-tree" function of git svn instead of
> dcommit, it was originally named "commit" back in the day set-tree does
> not rewrite any history.
>
> It fell out of favor because you could end up with a lot of non-linear
> history making it difficult for sharing diffs with SVN-using cow-orkers.
>
> It is useful if you don't want to share your individual changesets, but
> push your work upstream to the SVN repos as one big ugly change.
>
> But if you want a staircase effect in gitk, set-tree can be used to make
> individual commits where every change ends up as a merge (and you'll see
> two commits for every change you made)
My problem no longer requires using set-tree (see below), but just to let you
know that when I try to set-tree, it gives:
$ git svn set-tree HEAD
config --get svn-remote.svn.fetch :refs/remotes/git-svn$: command returned
error: 1
In my repo, "remotes/git-svn" doesn't exist; I have
$ git branch -a
* master
test1
remotes/test1
remotes/trunk
but the manual doesn't tell me how to select an svn-remote that's not "git-svn"..
>> (17:16:40) infinity0: i read a thread where it says those are different things
>> (17:16:41) offby1: infinity0: I suspect you're using git svn for something for
>> which it wasn't designed.
>> (17:17:17) infinity0: would it be possible, in theory, to have git-svn store
>> the git merge information in eg. the same way it stores the git-svn tag in the
>> svn commit message
>> (17:17:33) Grum: then just use svn?
>> (17:17:37) Grum: and a postit?
>
> I don't agree with having git-specific metadata on the SVN side itself.
> Often times that git-specific metadata has SHA1s unique to the user that
> committed it, so it wouldn't be useful to anyone else unless users are
> merging from each others git repos (which is not an easy/natural
> workflow if SVN is the mainline). Patch exchange is more
> reliable/easier...
>
> I've also worked in places where alternative tools are frowned upon, so
> sending git-specific metadata over to SVN should always be optional.
>
> The majority of folks I've worked with on SVN-hosted projects have never
> known about my git usage (that is changing as git popularity increases,
> however).
>
>> (17:18:01) infinity0: i'm trying to link two separate svn repos together via git
>> (17:18:17) Grum: and that is just what offby1 said
>> (17:18:30) infinity0: "what" is
>> (17:18:40) Grum: I suspect you're using git svn for something for which it
>> wasn't designed.
>> (17:18:42) infinity0: as you all are saying, git merges and svn "merges" are
>> different things
>> (17:18:58) infinity0: ok, but it would be possible to make git-svn have this
>> functionality? or not
>> (17:18:59) offby1: certainly
>> (17:19:16) offby1: I fear not, since Eric Wong seems like a smart fella; if it
>> were doable, I suspect he'd have done it already.
>> (17:19:21) offby1: But then ... who knows, maybe he's busy.
>
> I'm not smart but I am busy :)
>
> Summary of the git svn merge tracking situation:
>
> Mapping git <-> git merges to SVN:
>
> * already doable for the committing user with set-tree,
> but makes history ugly for:
>
> a) yourself (with every commit set-tree'd individually)
> b) SVN users (single set-tree with the newest commit)
> c) all of the above (varying granularity)
>
> * Pushing git metadata to SVN will annoy SVN-only users
>
> * Putting git metadata in SVN may not be useful since SHA1s
> may be specific to the user that made that commit.
>
> Mapping SVN <-> SVN merges to SVN via git svn:
>
> * most likely to be doable, they'll become plain SVN <-> SVN merges,
> see problems with getting SVN <-> SVN merges back to git, however...
>
> Mapping SVN <-> SVN merges to git:
>
> * SVN can represent merges that git can't, SVN can be/is extremely
> complicated when it comes to merges.
>
> * I don't see many projects (I care about) use SVN merge tracking,
> which projects actually use it? Maybe it's still too new and
> distros/users are behind the upgrade curve...
>
> I've probably missed some, I've been dozing off while replying to
> emails...
>
Actually, it turns out that my original problem is simpler than any of these
scenarios. What I was doing was git <-> git merges, where both of these git
branches were tracking *different* SVN branches (in my original case, from
different repos; in my simplified test case, from the same repo).
Consider this scenario:
----A0*-----A1---+
\ \
B0*----B1----B2
branchA: A1
branchB: B2
Where A0* and B0* have both been dcommited into their SVN branches, but A1, B1
and B2 are present in the git repo only. A0* and B0* have the "git-svn-id" tag,
and show my svn committer name; A1, B1 and B2 are still pure git commits, and
show my git commiter name.
Scenario 1:
If HEAD is at B2, and we try to "git-svn dcommit", then what happens currently
is, git-svn will dcommit B1 then B2, re-writing them in the process (adding
git-svn-id and using the svn credentials instead of git credentials); however,
it will *miss out* dcommitting A1. So we get this:
----A0*-----A1----+
\ \
B0*----B1*----B2*
branchA: A1
branchB: B2*
where B1* and B2* are the re-written versions of B1 and B2, with the added
git-svn-id and the svn committer name instead of the git committer name. There
isn't a problem yet; but when we switch to branchA and dcommit, we get this:
+-----A1*
/
----A0*-----A1----+
\ \
B0*----B1*----B2*
branchA: A1*
branchB: B2*
Where A1* is the re-written version of A1. But B2* still has A1 as a parent,
and now we have an extra "duplicate" commit in our git repo. What's even worse
is that A1* is **not a parent** of B2*, and so future merges on the branches
will potentially need to resolve conflicts that were resolved already when
merging (A1, B1) to B2.
Scenario 2:
If however, we dcommit A1, then B1, then B2, the commits will be re-written in
such a way that the proper merge history is preserved, including the correct
parents.
In one of the follow-up emails to my original posting, I supplied a test script
(gitsvntest.sh) which demonstrates the 2 scenarios. You can use a GUI to review
the history graphs. I can re-send it if you can't find that email.
I'm not sure how hard this is to fix; I guess it would involve making dcommit
detecting the case where a commit has more than 1 parent, and recursing down
all the parents to see if they are tracking an svn branch.
At the very least, I think this situation can be detected and the user warned.
In switching from svn to git, git-svn was very helpful to me, but this
behaviour confused me completely - sometimes things worked fine, depending on
the order in which I dcommited stuff, so it was incredibly hard to figure out,
especially since at that time I had no understanding of the concept of object
graphs and rewriting commits and that sort of thing.
X
^ permalink raw reply
* Re: Strange merge failure (would be overwritten by merge / cannot merge)
From: Linus Torvalds @ 2009-09-06 21:37 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git Mailing List
In-Reply-To: <alpine.LFD.2.01.0909061354010.8946@localhost.localdomain>
On Sun, 6 Sep 2009, Linus Torvalds wrote:
>
> And it's not working, for some reason I'm not seeing, but I thought I'd
> send it to you just as a way to show where I'm trying to take this. Maybe
> you see what my thinko is.
Duh. My thinko was that I wasn't testing the right thing. The patch works
fine, my test failures came from the fact that I was working on a branch
with some other broken experimental features.
Now, the patch I sent out did have a buglet: when a conflict happens,
'mask' will not have the conflicting bits we just added to 'dirmask'. But
that buglet doesn't actually seem to affect any of the traverse_tree()
callers, and they are fine with 'dirmask' being separate from 'mask'.
So here's a slightly updated version, and it passes all the tests.
Again, an important note: this is _not_ meant to change semantics, it's
really only meant to re-organize the code so that we _can_ do look-ahead
in the trees for conflicts. But in its current form, the look-ahead is
limited to the next entry, the same as the old code.
So there should be no semantic changes from this patch. Just avoid using
'df_name_compare()' and create that 'find_df_conflict()' function that can
be expanded to actually look ahead more.
Linus
---
tree-walk.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
1 files changed, 69 insertions(+), 3 deletions(-)
diff --git a/tree-walk.c b/tree-walk.c
index 02e2aed..fb5ca1e 100644
--- a/tree-walk.c
+++ b/tree-walk.c
@@ -62,7 +62,7 @@ void *fill_tree_descriptor(struct tree_desc *desc, const unsigned char *sha1)
static int entry_compare(struct name_entry *a, struct name_entry *b)
{
- return df_name_compare(
+ return base_name_compare(
a->path, tree_entry_len(a->path, a->sha1), a->mode,
b->path, tree_entry_len(b->path, b->sha1), b->mode);
}
@@ -138,6 +138,68 @@ char *make_traverse_path(char *path, const struct traverse_info *info, const str
return path;
}
+/*
+ * See if 'entry' may conflict with a later tree entry in 't': if so,
+ * fill in 'conflict' with the conflicting tree entry from 't'.
+ *
+ * NOTE! Right now we do _not_ create a create a private copy of the tree
+ * descriptor, so we can't actually walk it any further without losing
+ * our place. We should change it to a loop over a copy of the tree
+ * descriptor, but then we'd also have to remember the skipped entries,
+ * so this is a hacky simple case that only handles the case we used
+ * to handle historically (ie clash in the very first entry)
+ *
+ * Note that only a regular file 'entry' can conflict with a later
+ * directory, since a directory with the same name will sort later.
+ */
+static int find_df_conflict(struct tree_desc *t, struct name_entry *entry, struct name_entry *conflict)
+{
+ int len;
+
+ if (S_ISDIR(entry->mode))
+ return 0;
+ len = tree_entry_len(entry->path, entry->sha1);
+
+ while (t->size) {
+ int nlen;
+
+ entry_extract(t, conflict);
+ nlen = tree_entry_len(conflict->path, conflict->sha1);
+
+ /*
+ * We can only have a future conflict if the entry matches the
+ * beginning of the name exactly, and if the next character is
+ * smaller than '/'.
+ *
+ * Break out otherwise.
+ */
+ if (nlen < len)
+ break;
+ if (memcmp(conflict->path, entry->path, nlen))
+ break;
+
+ /*
+ * FIXME! This is the case where we'd like to mark the tree
+ * entry used in the original 't' rather than modify 't'!
+ */
+ if (nlen == len) {
+ update_tree_entry(t);
+ return 1;
+ }
+
+ if (conflict->path[len] > '/')
+ break;
+ /*
+ * FIXME! Here we'd really like to do 'update_tree_entry(©);'
+ * but that requires us to remember the conflict position specially
+ * so now we just punt and stop looking for conflicts
+ */
+ break;
+ }
+ entry_clear(conflict);
+ return 0;
+}
+
int traverse_trees(int n, struct tree_desc *t, struct traverse_info *info)
{
int ret = 0;
@@ -179,14 +241,18 @@ int traverse_trees(int n, struct tree_desc *t, struct traverse_info *info)
dirmask &= mask;
/*
- * Clear all the unused name-entries.
+ * Clear all the unused name-entries, and look for
+ * conflicts.
*/
for (i = 0; i < n; i++) {
if (mask & (1ul << i))
continue;
entry_clear(entry + i);
+ if (find_df_conflict(t+i, entry+last, entry+i))
+ dirmask |= (1ul << i);
}
- ret = info->fn(n, mask, dirmask, entry, info);
+
+ ret = info->fn(n, mask | dirmask, dirmask, entry, info);
if (ret < 0)
break;
if (ret)
^ permalink raw reply related
* Re: Strange merge failure (would be overwritten by merge / cannot merge)
From: Junio C Hamano @ 2009-09-06 21:17 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Git Mailing List
In-Reply-To: <alpine.LFD.2.01.0909061354010.8946@localhost.localdomain>
Linus Torvalds <torvalds@linux-foundation.org> writes:
> Grr. I need to go off and do some other things, and this still fails a few
> tests...
> [ In other words: in this version, we only do a single-entry lookahead,
> exactly like we used to do before. So this is not meant to _fix_
> anything, or change any semantics. It's an incremental "change the model
> so that we can look ahead more in a future patch" patch.
>
> But it's broken, and I have to run away for a while. ]
That's okay. I myself have to go and give a final review to the galley
proof of my book, before the publisher gives a go to the printing press
now.
I do not think we are in a hurry, as this issue, as far as I can tell, has
been with us from early days of git; I would say for the whole lifetime of
the unpack_trees() interface, but I didn't check.
^ permalink raw reply
* Re: Strange merge failure (would be overwritten by merge / cannot merge)
From: Junio C Hamano @ 2009-09-06 21:11 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Git Mailing List
In-Reply-To: <alpine.LFD.2.01.0909061338350.8946@localhost.localdomain>
Linus Torvalds <torvalds@linux-foundation.org> writes:
> The index case is slightly easier, as we can go back-and-forth in the
> source index (we do try to avoid it right now, but that's a small
> optimization rather than anything fundamental),...
Almost true, but there is the final "Any left-over entries in the index?"
phase in unpack_trees() that relies on o->pos being correct.
^ permalink raw reply
* Re: Strange merge failure (would be overwritten by merge / cannot merge)
From: Linus Torvalds @ 2009-09-06 20:58 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git Mailing List
In-Reply-To: <alpine.LFD.2.01.0909061338350.8946@localhost.localdomain>
On Sun, 6 Sep 2009, Linus Torvalds wrote:
>
> I agree. It's why I initially wanted to do it _all_ in the
> unpack_callback() thing, but the more I tried, the more complex it got.
>
> So now my plan is to do the conflict handling at a tree level in
> traverse_trees(), and get rid of the use of 'df_name_compare()' just there
> first.
Grr. I need to go off and do some other things, and this still fails a few
tests. This is not my more exhaustive patch that actually tries to
remember the conflict entries we've used up, this is the most cut-down and
simplified "just remove df_name_compare() in tree-walk.c".
And it's not working, for some reason I'm not seeing, but I thought I'd
send it to you just as a way to show where I'm trying to take this. Maybe
you see what my thinko is.
[ In other words: in this version, we only do a single-entry lookahead,
exactly like we used to do before. So this is not meant to _fix_
anything, or change any semantics. It's an incremental "change the model
so that we can look ahead more in a future patch" patch.
But it's broken, and I have to run away for a while. ]
Linus
---
tree-walk.c | 69 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
1 files changed, 67 insertions(+), 2 deletions(-)
diff --git a/tree-walk.c b/tree-walk.c
index 02e2aed..dd563e9 100644
--- a/tree-walk.c
+++ b/tree-walk.c
@@ -62,7 +62,7 @@ void *fill_tree_descriptor(struct tree_desc *desc, const unsigned char *sha1)
static int entry_compare(struct name_entry *a, struct name_entry *b)
{
- return df_name_compare(
+ return base_name_compare(
a->path, tree_entry_len(a->path, a->sha1), a->mode,
b->path, tree_entry_len(b->path, b->sha1), b->mode);
}
@@ -138,6 +138,68 @@ char *make_traverse_path(char *path, const struct traverse_info *info, const str
return path;
}
+/*
+ * See if 'entry' may conflict with a later tree entry in 't': if so,
+ * fill in 'conflict' with the conflicting tree entry from 't'.
+ *
+ * NOTE! Right now we do _not_ create a create a private copy of the tree
+ * descriptor, so we can't actually walk it any further without losing
+ * our place. We should change it to a loop over a copy of the tree
+ * descriptor, but then we'd also have to remember the skipped entries,
+ * so this is a hacky simple case that only handles the case we used
+ * to handle historically (ie clash in the very first entry)
+ *
+ * Note that only a regular file 'entry' can conflict with a later
+ * directory, since a directory with the same name will sort later.
+ */
+static int find_df_conflict(struct tree_desc *t, struct name_entry *entry, struct name_entry *conflict)
+{
+ int len;
+
+ if (S_ISDIR(entry->mode))
+ return 0;
+ len = tree_entry_len(entry->path, entry->sha1);
+
+ while (t->size) {
+ int nlen;
+
+ entry_extract(t, conflict);
+ nlen = tree_entry_len(conflict->path, conflict->sha1);
+
+ /*
+ * We can only have a future conflict if the entry matches the
+ * beginning of the name exactly, and if the next character is
+ * smaller than '/'.
+ *
+ * Break out otherwise.
+ */
+ if (nlen < len)
+ break;
+ if (memcmp(conflict->path, entry->path, nlen))
+ break;
+
+ /*
+ * FIXME! This is the case where we'd like to mark the tree
+ * entry used in the original 't' rather than modify 't'!
+ */
+ if (nlen == len) {
+ update_tree_entry(t);
+ return 1;
+ }
+
+ if (conflict->path[len] > '/')
+ break;
+ /*
+ * FIXME! Here we'd really like to do 'update_tree_entry(©);'
+ * but that requires us to remember the conflict position specially
+ * so now we just punt and stop looking for conflicts
+ */
+ break;
+ }
+ entry_clear(conflict);
+ return 0;
+}
+
int traverse_trees(int n, struct tree_desc *t, struct traverse_info *info)
{
int ret = 0;
@@ -179,12 +241,15 @@ int traverse_trees(int n, struct tree_desc *t, struct traverse_info *info)
dirmask &= mask;
/*
- * Clear all the unused name-entries.
+ * Clear all the unused name-entries, and look for
+ * conflicts.
*/
for (i = 0; i < n; i++) {
if (mask & (1ul << i))
continue;
entry_clear(entry + i);
+ if (find_df_conflict(t+i, entry+last, entry+i))
+ dirmask |= (1ul << i);
}
ret = info->fn(n, mask, dirmask, entry, info);
if (ret < 0)
^ permalink raw reply related
* Re: Strange merge failure (would be overwritten by merge / cannot merge)
From: Linus Torvalds @ 2009-09-06 20:42 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git Mailing List
In-Reply-To: <7vws4bzu7w.fsf@alter.siamese.dyndns.org>
On Sun, 6 Sep 2009, Junio C Hamano wrote:
>
> The reason I brought up adding the "candidate for the earliest name"
> interface to the function was to avoid a case where the index has
>
> blob "t"
> blob "t-f"
>
> and all the trees being merged have
>
> blob "t-f"
> tree "t"
>
> in which case the "Are we supposed to look at the index too?" logic in
> unpack_callback() may not catch the "t-f" entry from the index when the
> first callback from traverse_trees() feeds it "t-f".
I agree. It's why I initially wanted to do it _all_ in the
unpack_callback() thing, but the more I tried, the more complex it got.
So now my plan is to do the conflict handling at a tree level in
traverse_trees(), and get rid of the use of 'df_name_compare()' just there
first.
The index case is slightly easier, as we can go back-and-forth in the
source index (we do try to avoid it right now, but that's a small
optimization rather than anything fundamental), so the index we can
traverse in a more flexible manner, and find the 't' conflict that way.
Linus
^ permalink raw reply
* Re: Strange merge failure (would be overwritten by merge / cannot merge)
From: Junio C Hamano @ 2009-09-06 20:36 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Junio C Hamano, Git Mailing List
In-Reply-To: <alpine.LFD.2.01.0909061248470.8946@localhost.localdomain>
Linus Torvalds <torvalds@linux-foundation.org> writes:
> On Sun, 6 Sep 2009, Junio C Hamano wrote:
>...
>> * traverse_trees() takes a callback from the caller in info->fn(). It
>> feeds the callback the entries with the same name most of the time, but
>> that is not a guarantee, and the bug we are seeing is coming from a
>> caller, unpack_trees_callback(), assuming it.
>
> This is the level I'm looking at. In fact, I'm going to cheat. I'm not
> going to do it when we call info->fn(), I'm going to do it _before_ the
> call, and have a special "find conflicts" phase inside traverse_trees()
> itself.
>
> That way, any traverse_trees() user will see the conflicts exactly like
> they used to, because I'm just going to add a special "find conflicts"
> phase there that does the right thing. It's a hack, but it's a "useful"
> hack, and it at least avoids being the current "it can't work for the
> special case" thing.
> ...
> I think I have a good solution, give me half an hour to actually get it to
> work.
Thanks.
The reason I brought up adding the "candidate for the earliest name"
interface to the function was to avoid a case where the index has
blob "t"
blob "t-f"
and all the trees being merged have
blob "t-f"
tree "t"
in which case the "Are we supposed to look at the index too?" logic in
unpack_callback() may not catch the "t-f" entry from the index when the
first callback from traverse_trees() feeds it "t-f". It would notice that
the entry at o->pos is "t". When that happens, I did not think of a clean
way to avoid the codepath from emitting "t" as "only exists in the index".
With the "candidate" addition, traverse_trees() could say "You asked me
that I may have to return 't' to you, and here are the entries from all
the trees." before giving "t-f" back.
Other than that, I think find_conflicts() phase in the traverse_trees()
makes sense.
^ permalink raw reply
* Re: Strange merge failure (would be overwritten by merge / cannot merge)
From: Linus Torvalds @ 2009-09-06 19:54 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git Mailing List
In-Reply-To: <7v3a6z3lsg.fsf@alter.siamese.dyndns.org>
On Sun, 6 Sep 2009, Junio C Hamano wrote:
> Linus Torvalds <torvalds@linux-foundation.org> writes:
>
> > And then fix the fallout from that: callers never get mixed-up tree and
> > blob entries, and have to do their DF checking themselves.
>
> There are two levels of internal APIs involved, and I am getting confused
> as to which level of callers you are referrring to in the above.
It could be done at any level, but there's a few places where it's easier
than others.
> My understanding of the current situation is:
>
> * unpack_trees() takes a callback from the caller in o->fn(). It
> promises (but fails to keep the promise) that the callback is called
> with entries with matching names, so that it gets to see D/F
> conflicting entries in one go.
Indeed. I'd _like_ to do it at this level (or even at the o->fn() level),
but quite frankly, unpack_trees() is so horribly complicated, and you'd
have to remember state, that doing it at this level is not realy
maintainable.
> * traverse_trees() takes a callback from the caller in info->fn(). It
> feeds the callback the entries with the same name most of the time, but
> that is not a guarantee, and the bug we are seeing is coming from a
> caller, unpack_trees_callback(), assuming it.
This is the level I'm looking at. In fact, I'm going to cheat. I'm not
going to do it when we call info->fn(), I'm going to do it _before_ the
call, and have a special "find conflicts" phase inside traverse_trees()
itself.
That way, any traverse_trees() user will see the conflicts exactly like
they used to, because I'm just going to add a special "find conflicts"
phase there that does the right thing. It's a hack, but it's a "useful"
hack, and it at least avoids being the current "it can't work for the
special case" thing.
> Do you mean we would still keep the promise unpack_trees() makes to its
> callbacks, e.g. threeway_merge(), or do you mean these callbacks are to be
> prepared to get DF-split input themselves and expected to coalesce them
> as necessary?
Either would work, but changing unpack_trees() semantics would just be
very painful. There are just too many users of it, and they are too
ingrained in their expectations of getting conflict information in a
single pass.
I think I have a good solution, give me half an hour to actually get it to
work.
Linus
^ permalink raw reply
* Re: Strange merge failure (would be overwritten by merge / cannot merge)
From: Junio C Hamano @ 2009-09-06 19:39 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Git Mailing List
In-Reply-To: <alpine.LFD.2.01.0909061053330.8946@localhost.localdomain>
Linus Torvalds <torvalds@linux-foundation.org> writes:
> And then fix the fallout from that: callers never get mixed-up tree and
> blob entries, and have to do their DF checking themselves.
There are two levels of internal APIs involved, and I am getting confused
as to which level of callers you are referrring to in the above.
My understanding of the current situation is:
* unpack_trees() takes a callback from the caller in o->fn(). It
promises (but fails to keep the promise) that the callback is called
with entries with matching names, so that it gets to see D/F
conflicting entries in one go.
* traverse_trees() takes a callback from the caller in info->fn(). It
feeds the callback the entries with the same name most of the time, but
that is not a guarantee, and the bug we are seeing is coming from a
caller, unpack_trees_callback(), assuming it.
Do you mean we would still keep the promise unpack_trees() makes to its
callbacks, e.g. threeway_merge(), or do you mean these callbacks are to be
prepared to get DF-split input themselves and expected to coalesce them
as necessary?
There are only two callers of traverse_trees() interface. unpack_trees()
codepath is the primary one (the other being the merge-tree.c, which is in
disuse).
If you mean unpack_trees() by "callers, ... have to do their DF checking",
it may be a much isolated fix than what I had in mind in the last message
from me---the one that makes traverse_trees() pay attention to the caller
supplied "candiate 'earliest' name" and tries to push the "give entries
from all the trees with the same name to the callback in one go" promise
down to traverse_trees() layer.
If traverse_trees() did not do this name coalescing, on the other hand, I
wonder if it is doing only half of the job to be useful. I do not think
of a plausible scenario where a caller, who wants to walk multiple trees
in parallel, does not want to be fed the entries with the same name from
all the input trees in one invocation of the callback it gives.
If all the callers need to do the name coalescing in order to notice the
situation that led to this bug, I wonder if it would be nicer to do so in
traverse_trees()?
unpack_trees() would of course be helped if traverse_trees() gave that
promise, and I suspect merge-tree.c::threeway_callback() would expect that
behaviour, too.
^ permalink raw reply
* Re: [BUG] 'add -u' doesn't work from untracked subdir
From: Clemens Buchacher @ 2009-09-06 18:28 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, SZEDER Gábor, git
In-Reply-To: <7vy6ot4x61.fsf@alter.siamese.dyndns.org>
On Sat, Sep 05, 2009 at 01:23:50AM -0700, Junio C Hamano wrote:
> #1 What are the commands that will be affected, other than "add -u" and
> "grep"? Are there others?
There are "ls-tree", "ls-files" and "clean". I can't find anything else.
> #2 Do all the commands in the answer to #1 currently behave exactly the
> same when run without any path parameter and when run with a single
> '.'?
Yes. But "ls-tree" and "ls-files" do not accept superdirectories in their
path specs.
> #3 Do all the commands that are already relative to the root currently
> limit their action to the cwd when run with a single '.'?
I can't think of one that doesn't.
Clemens
^ permalink raw reply
* Re: Strange merge failure (would be overwritten by merge / cannot merge)
From: Linus Torvalds @ 2009-09-06 18:18 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git Mailing List
In-Reply-To: <7vvdjwbhzv.fsf@alter.siamese.dyndns.org>
Sorry, I was off for the week with very limited computer time (my main
computer was a Suunto Gekko that I wore underwater), and spent yesterday
doing the last kernel -rc.
Anyway, this is a problem I knew about, and it comes directly from the
insane historical behavior of unpack_trees(), where it would mix up blobs
and trees. The problem was _very_ obvious when I rewrote the tree-walking
to be readable.
See commit 91e4f03604bd089e09154e95294d5d08c805ea49, and in particular the
change from using base_name_compare() to using the idiotic df_name_compare().
It's called 'df_name_compare()' for a reason. That 'df' is because of the
insane directory/file semantics that are simply not a compete ordering.
And because it's not a complete ordering, it's impossible to handle
certain cases, exactly because of the following situation:
a < a-b < a/
but at the same time
a == a/
ie you have a unsatisfiable situation.
And no, changing the ordering to "pure blob order" is _not_ the solution.
Because then you'll no longer traverse the trees in the same order as you
traverse the index, and you'll get into _much_ deeper trouble.
So the real solution was always to never use 'df_name_compare()': any user
of that function is broken by design. It's sadly just the case that the
original unpack_trees() always had those insane semantics, and when I
rewrote it, I was very careful to keep the old semantics. Trust me, I very
much wanted to change them.
So the solution is to change the 'df_name_compare()' (that fundamentally
has that impossible constraint of 'a' == 'a/') to 'base_name_compare()'.
That way name comparisons are always meaningful, and always follow the
rules. The df_name_compare() thing was always a broken hack - just one
that got the semantics we wanted for all the truly simple cases.
And then fix the fallout from that: callers never get mixed-up tree and
blob entries, and have to do their DF checking themselves.
IOW, the starting point is the following. And let me try to see what we
need to do in the callers (this really is just a starting point: we'll
need to clean up all the insane DF conflict marker code that is now
pointless)
Linus
---
From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Sun, 6 Sep 2009 11:15:04 -0700
Subject: [PATCH] Get rid of the broken 'df_name_compare()'
This _will_ break the test-suite, but anything that depends on
df_name_compare() is fundamentally flawed. Because it is designed to
compare directory and blob names as equal, you have 'a' == 'a/', but at
the same time you also have 'a' < 'a-b' < 'a/'.
Out old unpack_trees() semantics depend on that impossible situation,
and we've kept this hack around for a long time. But now somebody has
finally hit the impossible case, and we need to bite the bullet and get
rid of the hack, and fix D/F conflict handling properly.
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
---
cache.h | 1 -
read-cache.c | 35 -----------------------------------
tree-walk.c | 2 +-
unpack-trees.c | 2 +-
4 files changed, 2 insertions(+), 38 deletions(-)
diff --git a/cache.h b/cache.h
index 5fad24c..fda9a49 100644
--- a/cache.h
+++ b/cache.h
@@ -709,7 +709,6 @@ extern int create_symref(const char *ref, const char *refs_heads_master, const c
extern int validate_headref(const char *ref);
extern int base_name_compare(const char *name1, int len1, int mode1, const char *name2, int len2, int mode2);
-extern int df_name_compare(const char *name1, int len1, int mode1, const char *name2, int len2, int mode2);
extern int cache_name_compare(const char *name1, int len1, const char *name2, int len2);
extern void *read_object_with_reference(const unsigned char *sha1,
diff --git a/read-cache.c b/read-cache.c
index 1bbaf1c..f3143d8 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -362,41 +362,6 @@ int base_name_compare(const char *name1, int len1, int mode1,
return (c1 < c2) ? -1 : (c1 > c2) ? 1 : 0;
}
-/*
- * df_name_compare() is identical to base_name_compare(), except it
- * compares conflicting directory/file entries as equal. Note that
- * while a directory name compares as equal to a regular file, they
- * then individually compare _differently_ to a filename that has
- * a dot after the basename (because '\0' < '.' < '/').
- *
- * This is used by routines that want to traverse the git namespace
- * but then handle conflicting entries together when possible.
- */
-int df_name_compare(const char *name1, int len1, int mode1,
- const char *name2, int len2, int mode2)
-{
- int len = len1 < len2 ? len1 : len2, cmp;
- unsigned char c1, c2;
-
- cmp = memcmp(name1, name2, len);
- if (cmp)
- return cmp;
- /* Directories and files compare equal (same length, same name) */
- if (len1 == len2)
- return 0;
- c1 = name1[len];
- if (!c1 && S_ISDIR(mode1))
- c1 = '/';
- c2 = name2[len];
- if (!c2 && S_ISDIR(mode2))
- c2 = '/';
- if (c1 == '/' && !c2)
- return 0;
- if (c2 == '/' && !c1)
- return 0;
- return c1 - c2;
-}
-
int cache_name_compare(const char *name1, int flags1, const char *name2, int flags2)
{
int len1 = flags1 & CE_NAMEMASK;
diff --git a/tree-walk.c b/tree-walk.c
index 02e2aed..8fc0ddc 100644
--- a/tree-walk.c
+++ b/tree-walk.c
@@ -62,7 +62,7 @@ void *fill_tree_descriptor(struct tree_desc *desc, const unsigned char *sha1)
static int entry_compare(struct name_entry *a, struct name_entry *b)
{
- return df_name_compare(
+ return base_name_compare(
a->path, tree_entry_len(a->path, a->sha1), a->mode,
b->path, tree_entry_len(b->path, b->sha1), b->mode);
}
diff --git a/unpack-trees.c b/unpack-trees.c
index 720f7a1..548fef4 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -196,7 +196,7 @@ static int do_compare_entry(const struct cache_entry *ce, const struct traverse_
ce_name = ce->name + pathlen;
len = tree_entry_len(n->path, n->sha1);
- return df_name_compare(ce_name, ce_len, S_IFREG, n->path, len, n->mode);
+ return base_name_compare(ce_name, ce_len, S_IFREG, n->path, len, n->mode);
}
static int compare_entry(const struct cache_entry *ce, const struct traverse_info *info, const struct name_entry *n)
^ 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