Git development
 help / color / mirror / Atom feed
* Re: [PATCH 11/10] support pager.* for aliases
From: Jeff King @ 2011-08-19  8:30 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Steffen Daode Nurpmeso, Ingo Brückl
In-Reply-To: <7vei0i560a.fsf@alter.siamese.dyndns.org>

On Thu, Aug 18, 2011 at 10:23:33PM -0700, Junio C Hamano wrote:

> > I think of it this way:
> >
> > If the user thinks of the alias as just another form of "log", then we
> > do the right thing: we use log's pager config by default, and respect
> > pager.log. They never set pager.foo, because that is nonsensical in
> > their mental model.
> >
> > If the user thinks of the alias as its own command, then they would
> > expect pager.foo to work. And it does what they expect.
> >
> > But like I said, I don't personally plan on using this. It was just the
> > only semantics that really made sense to me,...
> 
> I can see that argument, but once you start paying attention to "*.foo",
> you have to keep supporting that forever, and also more importantly, you
> need to worry about interactions between "*.foo" vs "*.log". Which one
> should win? Should they combine if both are defined? My "looks confusing"
> includes that can of worms.

It seems obvious to me that the more-specific *.foo form would take
precedence over the *.log form, and anything else would be crazy. But
that is just my gut feeling. If you are confused or worried, then that
is enough for me to say it is not worth pursuing. It's not a feature I
really care about; it was more about fixing something that looked
obviously wrong while I was in the area.

-Peff

^ permalink raw reply

* Re: [PATCH 0/10] color and pager improvements
From: Ingo Brückl @ 2011-08-19  6:34 UTC (permalink / raw)
  To: git
In-Reply-To: <20110818224644.GC8481@sigill.intra.peff.net>

Jeff King wrote on Thu, 18 Aug 2011 15:46:44 -0700:

> On Fri, Aug 19, 2011 at 12:33:01AM +0200, Ingo Brückl wrote:

>> My goal was to be able to turn off paging for "stash list" only while all
>> other stash commands should continue paging.

> Ah, OK. I think the only other stash command that pages is "stash show",
> but I don't think it's unreasonable to want paging for that but not for
> "list".

Maybe "stash list" simply should - like other commands - not paginate by
default.

Ingo

^ permalink raw reply

* Re: [PATCH 1/1] clone: replace relative paths in the alternates
From: Hui Wang @ 2011-08-19  6:39 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Hui Wang, git
In-Reply-To: <7vy5yp53aj.fsf@alter.siamese.dyndns.org>

Junio C Hamano wrote:
> Hui Wang <jason77.wang@gmail.com> writes:
>
>   
>> +	mapsz = xsize_t(st.st_size);
>> +	map = xmmap(NULL, mapsz, PROT_READ, MAP_PRIVATE, fd, 0);
>> +	close(fd);
>> +
>> +	tmpbuf = mapsz > 4096 ? xmalloc(mapsz * 2) : xmalloc(mapsz + 4096);
>>     
>
> Where do these magic numbers come from, and what guarantee do we have that
> these magic numbers give safe upper bound for pathnames that are expanded
> to be absolute?
>
>   
If an alternates file has dozens of relative path lines, this code has 
great risk.
> Wouldn't it be a lot cleaner and much less error prone if you open the
> original, read from it with strbuf_readline(), convert the path using
> strbuf manipulation functions and write the resulting line out to the
> lockfile you obtained to update it, line by line?
>   
This is a good suggestion, i will try to use the above way to prepare a 
V2 patch.

Thanks,
Jason.

^ permalink raw reply

* Re: [PATCH 1/1] clone: replace relative paths in the alternates
From: Junio C Hamano @ 2011-08-19  6:22 UTC (permalink / raw)
  To: Hui Wang; +Cc: git
In-Reply-To: <1313731396-10458-2-git-send-email-jason77.wang@gmail.com>

Hui Wang <jason77.wang@gmail.com> writes:

> +	mapsz = xsize_t(st.st_size);
> +	map = xmmap(NULL, mapsz, PROT_READ, MAP_PRIVATE, fd, 0);
> +	close(fd);
> +
> +	tmpbuf = mapsz > 4096 ? xmalloc(mapsz * 2) : xmalloc(mapsz + 4096);

Where do these magic numbers come from, and what guarantee do we have that
these magic numbers give safe upper bound for pathnames that are expanded
to be absolute?

Wouldn't it be a lot cleaner and much less error prone if you open the
original, read from it with strbuf_readline(), convert the path using
strbuf manipulation functions and write the resulting line out to the
lockfile you obtained to update it, line by line?

^ permalink raw reply

* Re: RFC: repository of handy git aliases?
From: Jon Seymour @ 2011-08-19  6:15 UTC (permalink / raw)
  To: Michael J Gruber; +Cc: Jakub Narębski, Git Mailing List
In-Reply-To: <4E4D3281.2050600@drmicha.warpmail.net>

On Fri, Aug 19, 2011 at 1:40 AM, Michael J Gruber
<git@drmicha.warpmail.net> wrote:
> Is this a typo or do you have an xargs which understands "1m"? My point
> was that you can feed all tips to "log" at once, as long as you don't
> exceed the argument list limits, of course.

Yeah, meant -L1, not -L1m.

Ok, so you are suggesting something like: git oneline $(git tips
origin/pu ^ origin/next) instead of

    git tips origin/pu ^origin/next | git oneline

I guess it comes down to a preference thing, but it is good to know
that with --no-walk I can do it either way, so thanks!

Here's a alias definition for oneline that does it both ways,
depending on preference...

[alias]
        oneline ="!_() { if test $# -eq 0; then xargs -L1 git log
--no-walk --decorate --oneline; else git log --no-walk --decorate
--oneline \"$@\"; fi; }; _"

>>
>> Not sure what you mean by not being standard? The main idea about
>> using --decorate here is to get a dump of any refs that might describe
>> the identified tips.
>
> Just compare your output with this:
>
> git log --decorate --no-walk --oneline origin/{master,next,pu}
> a46e511 (origin/pu) Merge branch 'fg/submodule-git-file-git-dir' into pu
>
> 0e3f8f0 (origin/next) Merge branch 'master' into next
>
> 4bfe7cb (origin/master, origin/HEAD) Sync with "maint"
>
> That is, I get the branch decorations there; you use the same format but
> don't get any. But I may be misunderstanding completely what you are
> trying to achieve. I guess I have to reread your extended use case
> description...
>

Ok, so the only reason my output didn't show any tags is that there
didn't happen to be any.

With a local integration branch, there usually will be local
references that explain each tip. So, for example, git tips master for
me currently shows:

git tips master | git oneline  shows:

f85a051 (kernel/master) Update draft release notes to 1.7.7
9e90d12 (refactor) rename auto-edit -> refactor.
e586f2e (gitwork-dist) Regenerate git-work.zip
dbb64d9 (tag: gitwork-v0.0.11, public/gitwork, gitwork) git-work:
update documentation.
272277f (bisect-gettextln) bisect: take advantage of gettextln, eval_gettextln.

So this does show helpful decorations. The reason git tips origin/pu
^origin/next | git oneline doesn't show any decorations is simply
because I don't have any tags or branches that represent those tips -
Junio presumably does, for example.

Note also that git tips doesn't show merge commits.

The best way to understand what git tips does is to do this:

     gitk origin/pu ^origin/next --no-merges

The commits reported by git tips are the tips of each linear stream of
development you see in gitk.

jon.

^ permalink raw reply

* Re: [PATCH] commit: clarify "could not read 'template'" message
From: Junio C Hamano @ 2011-08-19  6:04 UTC (permalink / raw)
  To: Jonathan Nieder; +Cc: Fabrício Nascimento, Michael Witten, git
In-Reply-To: <20110819050714.GA21697@elie.gateway.2wire.net>

Jonathan Nieder <jrnieder@gmail.com> writes:

> At least the problem could have been easier to debug.  How about this
> patch, to start?

It is a good start, but at this point do we know where the template_file
came from? It does not seem rocket surgery to update git_commit_config()
so that it stores the value taken from configuration file in a separate
variable so that we can tell the difference in order to tweak the message.

Are there only these three kinds of "pathname" variables that can be
configured?

 - Those to be always used (it is an error if missing);
 - Those to be always used (the calling code creat(2)s if missing); and
 - Those to be used only if already exist (otherwise ignored).

If so, a good way forward might be to update git_config_pathname() and
tell it which kind of path we are talking about. For the first and the
third kind, it would (after ~ expansion and other magic) stat the string
and error out for the first kind and return NULL for the third kind. For
the second kind, we do not need to do anything special. We may need to
distinguish between files and dirs, so we might need 6 variations, not
just 3, but that is a minor implementation detail.

This is one of the reasons that I tend to resist applying patches that add
useless configuration variables "just because it seems useful".

>  builtin/commit.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/builtin/commit.c b/builtin/commit.c
> index cb738574..d029ec03 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -703,7 +703,8 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
>  		hook_arg1 = "squash";
>  	} else if (template_file) {
>  		if (strbuf_read_file(&sb, template_file, 0) < 0)
> -			die_errno(_("could not read '%s'"), template_file);
> +			die_errno(_("could not read commit message template '%s'"),
> +				  template_file);
>  		hook_arg1 = "template";
>  		clean_message_contents = 0;
>  	}

^ permalink raw reply

* Re: [PATCH 11/10] support pager.* for aliases
From: Junio C Hamano @ 2011-08-19  5:43 UTC (permalink / raw)
  To: Jeff King; +Cc: git, Steffen Daode Nurpmeso, Ingo Brückl
In-Reply-To: <7vei0i560a.fsf@alter.siamese.dyndns.org>

Junio C Hamano <gitster@pobox.com> writes:

>> If the user thinks of the alias as just another form of "log", then we
>> do the right thing: we use log's pager config by default, and respect
>> pager.log. They never set pager.foo, because that is nonsensical in
>> their mental model.
>>
>> If the user thinks of the alias as its own command, then they would
>> expect pager.foo to work. And it does what they expect.
>>
>> But like I said, I don't personally plan on using this. It was just the
>> only semantics that really made sense to me,...
>
> I can see that argument, but once you start paying attention to "*.foo",
> you have to keep supporting that forever, and also more importantly, you
> need to worry about interactions between "*.foo" vs "*.log". Which one
> should win? Should they combine if both are defined? My "looks confusing"
> includes that can of worms.

Actually there is another thing that I think is much worse. If the user is
trained to think of the alias as its own command by seeing pager.foo to
work as you described, you cannot blame them if they would also expect
these to work, in the sense that only "foo.*" and "bar.*" respectively
would take effect, and they would override "log.*":

	[alias]
        	foo = log
		bar = !sh -c 'log "$@"' -
	[log]
		date = default
	[foo]
        	date = iso
	[bar]
		date = relative

I do not think we want to go there.

^ permalink raw reply

* Re: [PATCH 11/10] support pager.* for aliases
From: Junio C Hamano @ 2011-08-19  5:23 UTC (permalink / raw)
  To: Jeff King; +Cc: git, Steffen Daode Nurpmeso, Ingo Brückl
In-Reply-To: <20110819044013.GA2163@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

> On Thu, Aug 18, 2011 at 09:18:49PM -0700, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>> 
>> >> > Until this patch, doing something like:
>> >> >
>> >> >   git config alias.foo log
>> >> >   git config pager.foo /some/specific/pager
>> >> >
>> >> > would not respect pager.foo at all.
>> >> 
>> >> Is it a good thing? Looks too confusing and I am having a hard time to
>> >> decide if this is "just because we could" or "because we need to be able
>> >> to do this for such and such reasons".
>> >
>> > I don't have a particular use for it myself. However, I don't see what's
>> > confusing about it. Would would you expect the above commands to do with
>> > respect to paging?
>> 
>> The reason I found it confusing was that I expected the "log" command that
>> is run as the expansion of the alias to be oblivious to the fact that the
>> end user called it "foo", and ignore anything specific to "foo", including
>> "pager.foo".
>
> I think of it this way:
>
> If the user thinks of the alias as just another form of "log", then we
> do the right thing: we use log's pager config by default, and respect
> pager.log. They never set pager.foo, because that is nonsensical in
> their mental model.
>
> If the user thinks of the alias as its own command, then they would
> expect pager.foo to work. And it does what they expect.
>
> But like I said, I don't personally plan on using this. It was just the
> only semantics that really made sense to me,...

I can see that argument, but once you start paying attention to "*.foo",
you have to keep supporting that forever, and also more importantly, you
need to worry about interactions between "*.foo" vs "*.log". Which one
should win? Should they combine if both are defined? My "looks confusing"
includes that can of worms.

^ permalink raw reply

* [PATCH 1/1] clone: replace relative paths in the alternates
From: Hui Wang @ 2011-08-19  5:23 UTC (permalink / raw)
  To: gitster, git
In-Reply-To: <1313731396-10458-1-git-send-email-jason77.wang@gmail.com>

When we clone a local source repository to a new local destination
repository without "--shared" option, if source repository has
relative path in the alternates, the clone will fail.

The root cause is when cloning local source repository to local
destination repository, the alternates of the source repository
will be copied to the destination repository, the relative path in
the alternates is source repository to base repository rather than
destination repository to base repository, so the destination
repository get a wrong relative path in the alternates, this will
cause errors when refer to base repository in the destination
repository.

To fix it, adding a function update_alt_rel_path() to replace all
relative paths to absolute paths in the alternates of the destination
repository before destination repository use those paths to look for
the base repository.

update_alt_rel_path() is referred to link_alt_odb_entries() in the
sha1_file.c, update_alternates_file() is referred to
add_to_alternates_file() in the sha1_file.c.

Signed-off-by: Hui Wang <jason77.wang@gmail.com>
---
 builtin/clone.c |   84 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 cache.h         |    1 +
 sha1_file.c     |   11 +++++++
 3 files changed, 95 insertions(+), 1 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index 7663bc2..65c1eaf 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -286,6 +286,87 @@ static void copy_or_link_directory(struct strbuf *src, struct strbuf *dest)
 	closedir(dir);
 }
 
+#define linesize(end, start) (end - start + 1)
+/*
+ * This function is called by clone_local() of clone.c
+ * If alternates in the src_repo (source repository) has relative paths,
+ * call this function will replace the relative paths to absolute paths
+ * in the des_repo (destination repository) alternates.
+ */
+static void update_alt_rel_path(const char *src_repo)
+{
+	void *map;
+	size_t mapsz;
+	char *cp, *last, *ep, sep = '\n';
+	int fd, buf_start = 0, needwrite = 0;
+	char *tmpbuf;
+	struct stat st;
+
+	fd = open(git_path("objects/info/alternates"), O_RDONLY);
+	if (fd < 0)
+		return;
+
+	if (fstat(fd, &st) || (st.st_size == 0)) {
+		close(fd);
+		return;
+	}
+
+	mapsz = xsize_t(st.st_size);
+	map = xmmap(NULL, mapsz, PROT_READ, MAP_PRIVATE, fd, 0);
+	close(fd);
+
+	tmpbuf = mapsz > 4096 ? xmalloc(mapsz * 2) : xmalloc(mapsz + 4096);
+
+	ep = map + mapsz;
+	last = map;
+	buf_start = 0;
+	while (last < ep) {
+		cp = last;
+		if (cp < ep && *cp == '#') {
+			while (cp < ep && *cp != sep)
+				cp++;
+			memcpy(tmpbuf + buf_start, last, linesize(cp, last));
+			buf_start += linesize(cp, last);
+			last = cp + 1;
+			continue;
+		}
+		while (cp < ep && *cp != sep)
+			cp++;
+		if (last != cp) {
+			if (!is_absolute_path(last)) {
+				char *abs_path, *abs_buf;
+				int len;
+
+				needwrite = 1;
+				memcpy(tmpbuf + buf_start, last, linesize(cp, last));
+				tmpbuf[buf_start + linesize(cp, last)] = '\0';
+				abs_path = mkpath("%s/objects/%s", src_repo, tmpbuf + buf_start);
+				abs_buf = xmalloc(PATH_MAX);
+				normalize_path_copy(abs_buf, abs_path);
+				len = strlen(abs_buf);
+				memcpy(tmpbuf + buf_start, abs_buf, len);
+				free(abs_buf);
+				buf_start += len;
+			} else {
+				memcpy(tmpbuf + buf_start, last, linesize(cp, last));
+				buf_start += linesize(cp, last);
+			}
+		}
+		while (cp < ep && *cp == sep)
+			cp++;
+		last = cp;
+	}
+	tmpbuf[buf_start] = '\0';
+
+	if (needwrite)
+		update_alternates_file_content(tmpbuf);
+
+	munmap(map, mapsz);
+	free(tmpbuf);
+
+	return;
+}
+
 static const struct ref *clone_local(const char *src_repo,
 				     const char *dest_repo)
 {
@@ -303,8 +384,9 @@ static const struct ref *clone_local(const char *src_repo,
 		copy_or_link_directory(&src, &dest);
 		strbuf_release(&src);
 		strbuf_release(&dest);
-	}
 
+		update_alt_rel_path(src_repo);
+	}
 	remote = remote_get(src_repo);
 	transport = transport_get(remote, src_repo);
 	ret = transport_get_remote_refs(transport);
diff --git a/cache.h b/cache.h
index fcf4501..d81c0f0 100644
--- a/cache.h
+++ b/cache.h
@@ -913,6 +913,7 @@ extern struct alternate_object_database {
 } *alt_odb_list;
 extern void prepare_alt_odb(void);
 extern void add_to_alternates_file(const char *reference);
+extern void update_alternates_file_content(const char *content);
 typedef int alt_odb_fn(struct alternate_object_database *, void *);
 extern void foreach_alt_odb(alt_odb_fn, void*);
 
diff --git a/sha1_file.c b/sha1_file.c
index d5616dc..dae7bf6 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -388,6 +388,17 @@ void add_to_alternates_file(const char *reference)
 		link_alt_odb_entries(alt, alt + strlen(alt), '\n', NULL, 0);
 }
 
+void update_alternates_file_content(const char *content)
+{
+	struct lock_file *lock = xcalloc(1, sizeof(struct lock_file));
+	int fd = hold_lock_file_for_update(lock, git_path("objects/info/alternates"), LOCK_DIE_ON_ERROR);
+	write_or_die(fd, content, strlen(content));
+	if (commit_lock_file(lock))
+		die("could not close alternates file");
+	if (alt_odb_tail)
+		link_alt_odb_entries(content, content + strlen(content), '\n', NULL, 0);
+}
+
 void foreach_alt_odb(alt_odb_fn fn, void *cb)
 {
 	struct alternate_object_database *ent;
-- 
1.7.6

^ permalink raw reply related

* [PATCH 0/1] git-clone: fix relative path problem in the alternates
From: Hui Wang @ 2011-08-19  5:23 UTC (permalink / raw)
  To: gitster, git

Hi Junio & everyone,

Several days ago, i reported a "git clone" issue, the url is at:
http://marc.info/?l=git&m=131219022632724&w=2

Here i describe the problem again and try to provide a fix for this
problem, please help to review it. Thanks.

The problem:
Suppose we have two git repositories on the local machine, one is
called base repository, another is called source repository, source
repository need refer to some objects of the base repository via
.git/objects/info/alternates. If we set a relative path (source repository
to base repository) in the alternates, we can execute any git commands (branch,
tag, pull, checkout...) successfully in the source repository. But if we
want to clone a destiantion repository from source repository without
"--shared" on the same machine, just like this:
%>git clone $local_path/src_repos $local_path/des_repos
This clone command will fail because git can't get objects of the base
repository.

The reason:
When we execute git clone $local_path/src_repos $local_path/des_repos,
git will copy alternates of the src_repos to the des_repos first, then
des_repos will use this alternates to refer to base repository, this is
the problem, the relative in the alternates is src_repos to base_repos
rather than des_repos to base_repos.

The fix:
After the alternates is copied from src_repos to des_repos and before this
file is used by the des_repos, we can parse it and replace all relative
paths to a corrected path (absolute path of base_repos or new relative path
of des_repos to base_repos). In my patch, i choose to replace relative paths
to absoulte paths of base_repos.

If we add "--shared", this problem doesn't exist becase git doesn't need to
copy alternates, if we add prefix before src_repos like "file://", "git://"...,
this problem also doesn't exist. So i add a function to replace relative paths
in the clone_local(), this only affect git clone $local_path/src_repos
$local_path/des_repos without "--shared" and it has replative paths in the
alternates of src_repos, for other situation, this function will not introduce
any changes. Below is a execution result example of my fix:

the content of the alternates in the source repos (4 lines):
$<relative_path1_source_repos_to_base1>
$<relative_path2_source_repos_to_base2>
# comment line
$<absolute_path_of_base3>

After clone from source repos, the content of the alternates in the des_repos (4 lines):
$<absolute_path_of_base1>
$<absolute_path_of_base2>
# comment line
$<absolute_path_of_base3> 

^ permalink raw reply

* [PATCH] commit: clarify "could not read 'template'" message
From: Jonathan Nieder @ 2011-08-19  5:07 UTC (permalink / raw)
  To: Fabrício Nascimento; +Cc: Michael Witten, git
In-Reply-To: <CAAC=0R5CpJx1QOKYJDs0_ksEdyYG95fSAo3U3eU536YW=M84MA@mail.gmail.com>

Fabrício noticed that when the "[commit] template" configuration is
misconfigured to point to a nonexistent file, ever since v1.7.4.2
(see 2140b140, 2011-02-25), "git commit" fails in a seemingly
mysterious way:

	$ git commit
	fatal: could not read 'template': No such file or directory

In olden times, the message instead said

	Commit template file does not exist.

which makes it a little clearer that the --template option or
corresponding configuration item is being misused.  Let's move a
little in that direction, by saying "could not read commit message
template 'template'".

Note that the origin of the setting in the motivating ~/.gitignore is
still unknown.  Depending on the answer, it might be nice to start
ignoring nonexistent commit templates again; if the problem is
widespread, hopefully there will be more reports soon.

Noticed-by: Fabrício Nascimento <fabriciosn@gmail.com>
Analysis-by: Michael Witten <mfwitten@gmail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Hi again,

Fabrício Nascimento wrote:

> I have no idea what could have set the variable. Time Machine tells me the
> only changes on the .gitconfig file happened on August 12th, the day I've
> flagged the bug. I know I've made a brew upgrade on all my packages (that
> included git) the night before, and then the bug appeared the day after. I
> should have done only git stats, commit, pull and push.

Thanks for these details.  I looked through the packages in brew, but no
obvious clues.

At least the problem could have been easier to debug.  How about this
patch, to start?

 builtin/commit.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index cb738574..d029ec03 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -703,7 +703,8 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 		hook_arg1 = "squash";
 	} else if (template_file) {
 		if (strbuf_read_file(&sb, template_file, 0) < 0)
-			die_errno(_("could not read '%s'"), template_file);
+			die_errno(_("could not read commit message template '%s'"),
+				  template_file);
 		hook_arg1 = "template";
 		clean_message_contents = 0;
 	}
-- 
1.7.6

^ permalink raw reply related

* Re: [PATCH 11/10] support pager.* for aliases
From: Jeff King @ 2011-08-19  4:40 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Steffen Daode Nurpmeso, Ingo Brückl
In-Reply-To: <7vliuq5906.fsf@alter.siamese.dyndns.org>

On Thu, Aug 18, 2011 at 09:18:49PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> >> > Until this patch, doing something like:
> >> >
> >> >   git config alias.foo log
> >> >   git config pager.foo /some/specific/pager
> >> >
> >> > would not respect pager.foo at all.
> >> 
> >> Is it a good thing? Looks too confusing and I am having a hard time to
> >> decide if this is "just because we could" or "because we need to be able
> >> to do this for such and such reasons".
> >
> > I don't have a particular use for it myself. However, I don't see what's
> > confusing about it. Would would you expect the above commands to do with
> > respect to paging?
> 
> The reason I found it confusing was that I expected the "log" command that
> is run as the expansion of the alias to be oblivious to the fact that the
> end user called it "foo", and ignore anything specific to "foo", including
> "pager.foo".

I think of it this way:

If the user thinks of the alias as just another form of "log", then we
do the right thing: we use log's pager config by default, and respect
pager.log. They never set pager.foo, because that is nonsensical in
their mental model.

If the user thinks of the alias as its own command, then they would
expect pager.foo to work. And it does what they expect.

But like I said, I don't personally plan on using this. It was just the
only semantics that really made sense to me, and I noticed it because of
working on externals. And clearly it's not a lot of code.

-Peff

^ permalink raw reply

* Re: [PATCH] fetch-pack: check for valid commit from server
From: Nguyen Thai Ngoc Duy @ 2011-08-19  4:19 UTC (permalink / raw)
  To: Shawn Pearce; +Cc: git
In-Reply-To: <CAJo=hJvN9W00ncG53933yiqT++QCxGLUqp8KtG=BsvKD81wWyA@mail.gmail.com>

2011/8/19 Shawn Pearce <spearce@spearce.org>:
> Just curious, did you see this on a particular server somewhere?

No. I was looking for lookup_commit() call sites without proper NULL handling.
-- 
Duy

^ permalink raw reply

* Re: [PATCH 11/10] support pager.* for aliases
From: Junio C Hamano @ 2011-08-19  4:18 UTC (permalink / raw)
  To: Jeff King; +Cc: git, Steffen Daode Nurpmeso, Ingo Brückl
In-Reply-To: <20110819033733.GB2993@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

>> > Until this patch, doing something like:
>> >
>> >   git config alias.foo log
>> >   git config pager.foo /some/specific/pager
>> >
>> > would not respect pager.foo at all.
>> 
>> Is it a good thing? Looks too confusing and I am having a hard time to
>> decide if this is "just because we could" or "because we need to be able
>> to do this for such and such reasons".
>
> I don't have a particular use for it myself. However, I don't see what's
> confusing about it. Would would you expect the above commands to do with
> respect to paging?

The reason I found it confusing was that I expected the "log" command that
is run as the expansion of the alias to be oblivious to the fact that the
end user called it "foo", and ignore anything specific to "foo", including
"pager.foo".

^ permalink raw reply

* Re: update-index --index-info producing spurious submodule commits
From: Junio C Hamano @ 2011-08-19  4:16 UTC (permalink / raw)
  To: Greg Troxel; +Cc: git, Richard Hansen
In-Reply-To: <rmiliuq2qlg.fsf@fnord.ir.bbn.com>

Greg Troxel <gdt@ir.bbn.com> writes:

> If what I'm doing is an abuse of update-index, do you or anyone else
> have a suggestion to make a directory in the index match a tree object?

"ls-tree -r HEAD foo" is probably what you meant to say.

^ permalink raw reply

* Re: [PATCH 11/10] support pager.* for aliases
From: Jeff King @ 2011-08-19  3:37 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Steffen Daode Nurpmeso, Ingo Brückl
In-Reply-To: <7v8vqq72kp.fsf@alter.siamese.dyndns.org>

On Thu, Aug 18, 2011 at 03:54:46PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > Until this patch, doing something like:
> >
> >   git config alias.foo log
> >   git config pager.foo /some/specific/pager
> >
> > would not respect pager.foo at all.
> 
> Is it a good thing? Looks too confusing and I am having a hard time to
> decide if this is "just because we could" or "because we need to be able
> to do this for such and such reasons".

I don't have a particular use for it myself. However, I don't see what's
confusing about it. Would would you expect the above commands to do with
respect to paging? I think the behavior after my patch does what users
will expect, whether they have configured pager.foo, pager.log, or
nothing.

-Peff

^ permalink raw reply

* Re: [PATCH] git-send-email: Add AUTH LOGIN support
From: Joe Perches @ 2011-08-19  1:16 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Graham Barr
In-Reply-To: <1312608114.6419.18.camel@Joe-Laptop>

On Fri, 2011-08-05 at 22:21 -0700, Joe Perches wrote:
> On Fri, 2011-08-05 at 21:54 -0700, Junio C Hamano wrote:
> > Joe Perches <joe@perches.com> writes:
> > > The current perl Net::SMTP support will not use AUTH LOGIN
> > > when other authentication options are available.
> > Even after reading this excuse,...
> > > +			if (defined $smtp_auth && $smtp_auth =~ /^login$/i) {
> > > +
> > > +			    $smtp->datasend("AUTH LOGIN\n");
> > > +			    $smtp->response();
> > > +			    $smtp->datasend(encode_base64("$smtp_authuser"));
> > > +			    $smtp->response();
> > > +			    $smtp->datasend(encode_base64("$smtp_authpass"));
> > > +			    $smtp->response();
> > > +
> > > +			} else {
> > > +
> > > +			    $auth ||= $smtp->auth( $smtp_authuser, $smtp_authpass ) or die $smtp->message;
> > > +			}
> > ... this makes me feel dirty X-(. Not the desire to force "AUTH LOGIN",
> > but the implementation to hand-roll the protocol exchange.
> > I'd rather want to know _why_ Net::SMTP does not support it in the first
> > place, and what it does for "other authentication options" that are
> > available. Does it try them in turn until it finds the one that works? Why
> > doesn't it fall back on "AUTH LOGIN" then?
> > Specifically, if there is a reason to avoid this plaintext authentication
> > method when other options are _available_ (which presumably would be the
> > reason why Net::SMTP chooses not to support it), and if there is a reason
> > on the user's side to _force_ this method even when people who wrote
> > Net::SMTP does not recommend it be used, wouldn't it be natural to expect
> > that there should be a way to configure the connection to use it, without
> > resorting to coding the protocol exchange by hand line this?
> 
> I needed something now.
> 
> You are right but I believe it would take too long
> to get updates to Net::SMTP in place. Doing this
> admitted ugliness in git-send-email works for me and
> seems to me to be appropriate for now.
> 
> I looked, there isn't a method to force a particular
> AUTH type documented.  I also didn't care to rewrite
> Net::SMTP right now.  This "works for me"...
> 
> > It probably is not as simple as installing Authen::SASL::*::LOGIN, but
> > still...

I think my patch should be applied until Net::SMTP is updated.

^ permalink raw reply

* Re: update-index --index-info producing spurious submodule commits
From: Greg Troxel @ 2011-08-19  0:27 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Richard Hansen
In-Reply-To: <7vd3g272tk.fsf@alter.siamese.dyndns.org>

[-- Attachment #1: Type: text/plain, Size: 1535 bytes --]


Junio C Hamano <gitster@pobox.com> writes:

> Greg Troxel <gdt@ir.bbn.com> writes:
>
>> git ls-tree HEAD foo
>> git ls-tree HEAD foo | git update-index --index-info
>
> This --index-info definitely looks wrong, if "foo" is a directory, as the
> entries in the index are supposed to be either blobs or commits.

In the man page for update-index in the --index-info section, what I'm
doing seems to be covered by point 2, which specifically talks about
output of ls-tree.

I realize the index is a data structure that has pairs of paths to
blobs.  But I also think of it as a representation of a tree object that
would be referenced were one to commit (even if it isn't in that form
yet).  So I would argue that update-index with a tree should walk that
tree and insert all the paths resulting from expansion into the index?

> The command could just instead barf, saying the input is wrong, but the
> option was so low-level that it was deliberately written to accept and
> store anything you throw at it --- even when it is nonsensical for the
> version of plumbing, later updates to the data structure might have made
> it making sense, which was the way to ease development of the system.

If what I'm doing is an abuse of update-index, do you or anyone else
have a suggestion to make a directory in the index match a tree object?
(I'm trying to use an index filter; it takes 11 hours to run
filter-branch (5500 commits, 400K files in index, 800MB .git, ~2+GB
working directory, tmpdir on NetBSD tmpfs (all in ram)).)

Thanks,
Greg

[-- Attachment #2: Type: application/pgp-signature, Size: 194 bytes --]

^ permalink raw reply

* Re: Branches & directories
From: Jonathan Nieder @ 2011-08-19  0:13 UTC (permalink / raw)
  To: Michael Witten
  Cc: Hilco Wijbenga, Junio C Hamano, Evan Shelhamer, Git Mailing List
In-Reply-To: <CAMOZ1BsZvXsnnWAPXR7UGKdqOMwuGB-ffaAPk55U_1dcjZUcDw@mail.gmail.com>

Hi Michael,

Michael Witten wrote:

> As an aside, the problem here is likely a manifestation of the fact
> that nobody understands what a branch is

I agree with Michael J Gruber that this is not the useful kind of
hyperbole.

In olden times, there was more talk of the refs in the refs/heads/
hierarchy as being branch heads which point to the tip of a particular
branch of history.  That is still exactly what they are.  By abuse of
language, people sometimes use the term "branch" as a synonym when
there is little risk of confusion.

If you have found tutorial documentation that is confusing on this
point, by all means, fix it.  You are talking as though a
documentation bug is a fundamental design flaw that cannot be fixed by
the likes of you and me.

^ permalink raw reply

* [PATCH/RFC 2/2] git-p4: Add complex test case for branch import
From: Vitor Antunes @ 2011-08-19  0:09 UTC (permalink / raw)
  To: git; +Cc: Pete Wyckoff, Tor Arvid Lund, Vitor Antunes
In-Reply-To: <1313712590-26876-1-git-send-email-vitor.hda@gmail.com>

Check if branches created from old changelists are correctly imported.

Signed-off-by: Vitor Antunes <vitor.hda@gmail.com>
---
 t/t9800-git-p4.sh |   77 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 77 insertions(+), 0 deletions(-)

diff --git a/t/t9800-git-p4.sh b/t/t9800-git-p4.sh
index 15704d2..9f757d7 100755
--- a/t/t9800-git-p4.sh
+++ b/t/t9800-git-p4.sh
@@ -454,6 +454,83 @@ test_expect_success 'git-p4 clone simple branches' '
 	rm -rf "$git" && mkdir "$git"
 '
 
+# Create a complex branch structure in P4 depot to check if they are correctly
+# cloned. The branches are created from older changelists to check if git-p4 is
+# able to correctly detect them.
+# The final expected structure is:
+# `branch1
+# | `- file1
+# | `- file2 (updated)
+# | `- file3
+# `branch2
+# | `- file1
+# | `- file2
+# `branch3
+# | `- file1
+# | `- file2 (updated)
+# | `- file3
+# `branch4
+# | `- file1
+# | `- file2
+# `branch5
+#   `- file1
+#   `- file2
+#   `- file3
+test_expect_success 'add p4 complex branches' '
+	cd "$cli" &&
+	changelist=$(p4 changes -m1 //depot/... | cut -d \  -f 2) &&
+	changelist=$(($changelist - 5)) &&
+	p4 integrate //depot/branch1/...@$changelist //depot/branch4/... &&
+	p4 submit -d "branch4" &&
+	changelist=$(($changelist + 2)) &&
+	p4 integrate //depot/branch1/...@$changelist //depot/branch5/... &&
+	p4 submit -d "branch5" &&
+	cd "$TRASH_DIRECTORY"
+'
+
+# Configure branches through git-config and clone them. git-p4 will only be able
+# to clone the original structure if it is able to detect the origin changelist
+# of each branch.
+test_expect_success 'git-p4 clone complex branches' '
+	git init "$git" &&
+	cd "$git" &&
+	git config git-p4.branchList branch1:branch2 &&
+	git config --add git-p4.branchList branch1:branch3 &&
+	git config --add git-p4.branchList branch1:branch4 &&
+	git config --add git-p4.branchList branch1:branch5 &&
+	cd "$TRASH_DIRECTORY" &&
+	"$GITP4" clone --dest="$git" --detect-branches //depot@all &&
+	cd "$git" &&
+	git log --all --graph --decorate --stat &&
+	git reset --hard p4/depot/branch1 &&
+	test -f file1 &&
+	test -f file2 &&
+	test -f file3 &&
+	grep -q update file2 &&
+	git reset --hard p4/depot/branch2 &&
+	test -f file1 &&
+	test -f file2 &&
+	test \! -f file3 &&
+	! grep -q update file2 &&
+	git reset --hard p4/depot/branch3 &&
+	test -f file1 &&
+	test -f file2 &&
+	test -f file3 &&
+	grep -q update file2 &&
+	git reset --hard p4/depot/branch4 &&
+	test -f file1 &&
+	test -f file2 &&
+	test \! -f file3 &&
+	! grep -q update file2 &&
+	git reset --hard p4/depot/branch5 &&
+	test -f file1 &&
+	test -f file2 &&
+	test -f file3 &&
+	! grep -q update file2 &&
+	cd "$TRASH_DIRECTORY" &&
+	rm -rf "$git" && mkdir "$git"
+'
+
 test_expect_success 'shutdown' '
 	pid=`pgrep -f p4d` &&
 	test -n "$pid" &&
-- 
1.7.5.4

^ permalink raw reply related

* [PATCH/RFC 1/2] git-p4: Search for parent commit on branch creation
From: Vitor Antunes @ 2011-08-19  0:09 UTC (permalink / raw)
  To: git; +Cc: Pete Wyckoff, Tor Arvid Lund, Vitor Antunes
In-Reply-To: <1313712590-26876-1-git-send-email-vitor.hda@gmail.com>

The new commit is applied sequentially to each blob of the parent branch. The
first blob which results in a zero diff with the new commit is considered the
parent commit. If none is found, then the commit is applied to the top of the
parent branch.
This algorithm requires a checkpoint and a time.sleep() to be able to use
diff-tree, making it slower than what would be desirable. It also requires using
--force argument of fast-import, leaving lots of trash commits when finished.

Signed-off-by: Vitor Antunes <vitor.hda@gmail.com>
---
 contrib/fast-import/git-p4 |   22 ++++++++++++++++++++--
 1 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4
index f622a38..0bced6d 100755
--- a/contrib/fast-import/git-p4
+++ b/contrib/fast-import/git-p4
@@ -1671,7 +1671,25 @@ class P4Sync(Command, P4UserMap):
                             parent = self.initialParents[branch]
                             del self.initialParents[branch]
 
-                        self.commit(description, filesForCommit, branch, [branchPrefix], parent)
+                        parentFound = 0
+                        if len(parent) > 0:
+                            self.gitStream.write("checkpoint\n\n")
+                            time.sleep(0.1)
+                            for blob in read_pipe_lines("git rev-list --reverse --no-merges %s" % parent):
+                                blob = blob.strip()
+                                self.gitStream.write("reset %s\nfrom %s\n" % (branch, blob))
+                                self.commit(description, filesForCommit, branch, [branchPrefix], blob)
+                                self.gitStream.write("checkpoint\n\n")
+                                time.sleep(0.1)
+                                if len( read_pipe("git diff-tree %s %s" % (blob, branch)) ) == 0:
+                                    parentFound = 1
+                                    if self.verbose:
+                                        print "Found parent of %s in commit %s" % (branch, blob)
+                                    break
+                        if not parentFound:
+                            if self.verbose:
+                                print "Parent of %s not found. Committing into head of %s" % (branch, parent)
+                            self.commit(description, filesForCommit, branch, [branchPrefix], parent)
                 else:
                     files = self.extractFilesFromCommit(description)
                     self.commit(description, files, self.branch, self.depotPaths,
@@ -1948,7 +1966,7 @@ class P4Sync(Command, P4UserMap):
 
         self.tz = "%+03d%02d" % (- time.timezone / 3600, ((- time.timezone % 3600) / 60))
 
-        importProcess = subprocess.Popen(["git", "fast-import"],
+        importProcess = subprocess.Popen(["git", "fast-import", "--force"],
                                          stdin=subprocess.PIPE, stdout=subprocess.PIPE,
                                          stderr=subprocess.PIPE);
         self.gitOutput = importProcess.stdout
-- 
1.7.5.4

^ permalink raw reply related

* [PATCH/RFC 0/2] Search for parent commit in new branch import
From: Vitor Antunes @ 2011-08-19  0:09 UTC (permalink / raw)
  To: git; +Cc: Pete Wyckoff, Tor Arvid Lund, Vitor Antunes

This is mostly a RFC/RFH (Request For Help;) regarding the code I used on this
patch. I think that this is not a clean solution, but it shows the algorithm I
want to implement. The worst points are:

1. Relies on a sleep() to avoid a race condition. I'm using 0.1s, but it still
   fails sometimes, so it needs to be bigger.
2. Requires the --force option of fast-import.

Test case is included.

Thanks in advance to everyone who contributes.

Vitor Antunes (2):
  git-p4: Search for parent commit on branch creation
  git-p4: Add complex test case for branch import

 contrib/fast-import/git-p4 |   22 +++++++++++-
 t/t9800-git-p4.sh          |   77 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 97 insertions(+), 2 deletions(-)

-- 
1.7.5.4

^ permalink raw reply

* [PATCH v3 3/4] git-p4: Allow branch definition with git config
From: Vitor Antunes @ 2011-08-18 23:44 UTC (permalink / raw)
  To: git; +Cc: Pete Wyckoff, Tor Arvid Lund, Vitor Antunes
In-Reply-To: <1313711046-23489-1-git-send-email-vitor.hda@gmail.com>

Perforce does not strictly require the usage of branch specifications to create
branches. In these cases the branch detection code of git-p4 will not be able to
import them.
This patch adds support for git-p4.branchList configuration option, allowing
branches to be defined in git config.

Signed-off-by: Vitor Antunes <vitor.hda@gmail.com>
---
 contrib/fast-import/git-p4     |   24 ++++++++++++++++++++++++
 contrib/fast-import/git-p4.txt |    7 +++++++
 2 files changed, 31 insertions(+), 0 deletions(-)

diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4
index 8b88f97..f622a38 100755
--- a/contrib/fast-import/git-p4
+++ b/contrib/fast-import/git-p4
@@ -342,6 +342,11 @@ def gitConfig(key, args = None): # set args to "--bool", for instance
         _gitConfig[key] = read_pipe(cmd, ignore_error=True).strip()
     return _gitConfig[key]
 
+def gitConfigList(key):
+    if not _gitConfig.has_key(key):
+        _gitConfig[key] = read_pipe("git config --get-all %s" % key, ignore_error=True).strip().split(os.linesep)
+    return _gitConfig[key]
+
 def p4BranchesInGit(branchesAreInRemotes = True):
     branches = {}
 
@@ -1490,6 +1495,25 @@ class P4Sync(Command, P4UserMap):
                     if source not in self.knownBranches:
                         lostAndFoundBranches.add(source)
 
+        # Perforce does not strictly require branches to be defined, so we also
+        # check git config for a branch list.
+        #
+        # Example of branch definition in git config file:
+        # [git-p4]
+        #   branchList=main:branchA
+        #   branchList=main:branchB
+        #   branchList=branchA:branchC
+        configBranches = gitConfigList("git-p4.branchList")
+        for branch in configBranches:
+            if branch:
+                (source, destination) = branch.split(":")
+                self.knownBranches[destination] = source
+
+                lostAndFoundBranches.discard(destination)
+
+                if source not in self.knownBranches:
+                    lostAndFoundBranches.add(source)
+
 
         for branch in lostAndFoundBranches:
             self.knownBranches[branch] = branch
diff --git a/contrib/fast-import/git-p4.txt b/contrib/fast-import/git-p4.txt
index 97b66b9..52003ae 100644
--- a/contrib/fast-import/git-p4.txt
+++ b/contrib/fast-import/git-p4.txt
@@ -263,6 +263,13 @@ Only use branch specifications defined by the selected username.
 
   git config [--global] git-p4.branchUser username
 
+git-p4.branchList
+
+List of branches to be imported when branch detection is enabled.
+
+  git config [--global] git-p4.branchList main:branchA
+  git config [--global] --add git-p4.branchList main:branchB
+
 Implementation Details...
 =========================
 
-- 
1.7.5.4

^ permalink raw reply related

* [PATCH v3 4/4] git-p4: Add simple test case for branch import
From: Vitor Antunes @ 2011-08-18 23:44 UTC (permalink / raw)
  To: git; +Cc: Pete Wyckoff, Tor Arvid Lund, Vitor Antunes
In-Reply-To: <1313711046-23489-1-git-send-email-vitor.hda@gmail.com>

Create a basic branch structure in P4 and clone it with git-p4.
Also, make an update on P4 side and check if git-p4 imports it correctly.
The branch structure is created in such a way that git-p4 will fail to import
updates if patch "git-p4: Correct branch base depot path detection" is not
applied.

Signed-off-by: Vitor Antunes <vitor.hda@gmail.com>
---
 t/t9800-git-p4.sh |   64 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 64 insertions(+), 0 deletions(-)

diff --git a/t/t9800-git-p4.sh b/t/t9800-git-p4.sh
index a4f3d66..777ead8 100755
--- a/t/t9800-git-p4.sh
+++ b/t/t9800-git-p4.sh
@@ -390,6 +390,70 @@ test_expect_success 'detect copies' '
 	rm -rf "$git" && mkdir "$git"
 '
 
+# Create a simple branch structure in P4 depot to check if it is correctly
+# cloned.
+test_expect_success 'add simple p4 branches' '
+	cd "$cli" &&
+	mkdir branch1 &&
+	cd branch1 &&
+	echo file1 >file1 &&
+	echo file2 >file2 &&
+	p4 add file* &&
+	p4 submit -d "branch1" &&
+	p4 integrate //depot/branch1/... //depot/branch2/... &&
+	p4 submit -d "branch2" &&
+	echo file3 >file3 &&
+	p4 add file3 &&
+	p4 submit -d "add file3 in branch1" &&
+	p4 open file2 &&
+	echo update >>file2 &&
+	p4 submit -d "update file2 in branch1" &&
+	p4 integrate //depot/branch1/... //depot/branch3/... &&
+	p4 submit -d "branch3" &&
+	cd "$TRASH_DIRECTORY"
+'
+
+# Configure branches through git-config and clone them.
+# All files are tested to make sure branches were cloned correctly.
+# Finally, make an update to branch1 on P4 side to check if it is imported
+# correctly by git-p4.
+test_expect_success 'git-p4 clone simple branches' '
+	git init "$git" &&
+	cd "$git" &&
+	git config git-p4.branchList branch1:branch2 &&
+	git config --add git-p4.branchList branch1:branch3 &&
+	cd "$TRASH_DIRECTORY" &&
+	"$GITP4" clone --dest="$git" --detect-branches //depot@all &&
+	cd "$git" &&
+	git log --all --graph --decorate --stat &&
+	git reset --hard p4/depot/branch1 &&
+	test -f file1 &&
+	test -f file2 &&
+	test -f file3 &&
+	grep -q update file2 &&
+	git reset --hard p4/depot/branch2 &&
+	test -f file1 &&
+	test -f file2 &&
+	test \! -z file3 &&
+	! grep -q update file2 &&
+	git reset --hard p4/depot/branch3 &&
+	test -f file1 &&
+	test -f file2 &&
+	test -f file3 &&
+	grep -q update file2 &&
+	cd "$cli" &&
+	cd branch1 &&
+	p4 edit file2 &&
+	echo file2_ >> file2 &&
+	p4 submit -d "update file2 in branch3" &&
+	cd "$git" &&
+	git reset --hard p4/depot/branch1 &&
+	"$GITP4" rebase &&
+	grep -q file2_ file2 &&
+	cd "$TRASH_DIRECTORY" &&
+	rm -rf "$git" && mkdir "$git"
+'
+
 test_expect_success 'shutdown' '
 	pid=`pgrep -f p4d` &&
 	test -n "$pid" &&
-- 
1.7.5.4

^ permalink raw reply related

* [PATCH v3 1/4] git-p4: Correct branch base depot path detection
From: Vitor Antunes @ 2011-08-18 23:44 UTC (permalink / raw)
  To: git; +Cc: Pete Wyckoff, Tor Arvid Lund, Vitor Antunes
In-Reply-To: <1313711046-23489-1-git-send-email-vitor.hda@gmail.com>

When branch detection is enabled each branch is named in git after their
relative depot path in Perforce. To do this the depot paths are compared against
each other to find their common base path. The current algorithm makes this
comparison on a character by character basis.
Assuming we have the following branches:

//depot/branches/featureA
//depot/branches/featureB

Then the base depot path would be //depot/branches/feature, which is an invalid
depot path.
The current patch fixes this by splitting the path into a list and comparing the
list entries, making it choose correctly //depot/branches as the base path.

Signed-off-by: Vitor Antunes <vitor.hda@gmail.com>
---
 contrib/fast-import/git-p4 |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4
index 29a5390..95246e9 100755
--- a/contrib/fast-import/git-p4
+++ b/contrib/fast-import/git-p4
@@ -1829,12 +1829,14 @@ class P4Sync(Command, P4UserMap):
                     else:
                         paths = []
                         for (prev, cur) in zip(self.previousDepotPaths, depotPaths):
-                            for i in range(0, min(len(cur), len(prev))):
-                                if cur[i] <> prev[i]:
+                            prev_list = prev.split("/")
+                            cur_list = cur.split("/")
+                            for i in range(0, min(len(cur_list), len(prev_list))):
+                                if cur_list[i] <> prev_list[i]:
                                     i = i - 1
                                     break
 
-                            paths.append (cur[:i + 1])
+                            paths.append ("/".join(cur_list[:i + 1]))
 
                         self.previousDepotPaths = paths
 
-- 
1.7.5.4

^ permalink raw reply related


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox