Git development
 help / color / mirror / Atom feed
* Re: [PATCH 4/4] Add git-rewrite-commits
From: Johannes Schindelin @ 2007-07-09 14:59 UTC (permalink / raw)
  To: skimo; +Cc: git, Junio C Hamano
In-Reply-To: <20070709144248.GW1528MdfPADPa@greensroom.kotnet.org>

Hi,

On Mon, 9 Jul 2007, Sven Verdoolaege wrote:

> On Mon, Jul 09, 2007 at 03:11:45PM +0100, Johannes Schindelin wrote:
> > On Mon, 9 Jul 2007, Sven Verdoolaege wrote:
> > > The current git-rewrite-commits will rewrite all SHA1's it can find,
> > > irrespective of any 'commit ' that may precede it.
> > 
> > Even abbreviated ones?
> 
> Yes.  I'll add that to the documentation.

That is definitely something you want to control.  I have seen commit 
messages referencing certain hexadecimal numbers, and they were definitely 
no commit names.  The shorter they are, the more likely they are to be 
rewritten by your magic.

> > * rename the darned things to "filter" again.
> > 
> > * --write-sha1-mappings=<directory> (or --write-commit-mappings), possibly 
> >   defaulting to .git/mappings/.  Be careful not to overwrite an existing 
> >   such directory.
> > 
> > * change the semantics of the commit filter: the output is a list 
> >   (possibly empty) of replacement sha1's for this commit.
> > 
> > * if any filters are called, provide a script with convenience functions, 
> >   and an environment variable pointing to it.  These functions should 
> >   include:
> > 
> > 	* map
> > 	* commit
> > 	* save
> > 	* restore
> 
> Hmm... you're tricking me into write shell code.

Ah, oh well.  If you do the rest, I'll do the shell code.

Ciao,
Dscho

^ permalink raw reply

* Re: [PATCH 4/4] Add git-rewrite-commits
From: Sven Verdoolaege @ 2007-07-09 14:42 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: git, Junio C Hamano
In-Reply-To: <Pine.LNX.4.64.0707091450440.5546@racer.site>

On Mon, Jul 09, 2007 at 03:11:45PM +0100, Johannes Schindelin wrote:
> On Mon, 9 Jul 2007, Sven Verdoolaege wrote:
> > The current git-rewrite-commits will rewrite all SHA1's it can find,
> > irrespective of any 'commit ' that may precede it.
> 
> Even abbreviated ones?

Yes.  I'll add that to the documentation.

> * rename the darned things to "filter" again.
> 
> * --write-sha1-mappings=<directory> (or --write-commit-mappings), possibly 
>   defaulting to .git/mappings/.  Be careful not to overwrite an existing 
>   such directory.
> 
> * change the semantics of the commit filter: the output is a list 
>   (possibly empty) of replacement sha1's for this commit.
> 
> * if any filters are called, provide a script with convenience functions, 
>   and an environment variable pointing to it.  These functions should 
>   include:
> 
> 	* map
> 	* commit
> 	* save
> 	* restore

Hmm... you're tricking me into write shell code.

skimo

^ permalink raw reply

* Re: [PATCH v2] Make fetch-pack a builtin with an internal API
From: Theodore Tso @ 2007-07-09 14:40 UTC (permalink / raw)
  To: Andy Parkins; +Cc: git, Junio C Hamano, Daniel Barkalow
In-Reply-To: <200707091416.39949.andyparkins@gmail.com>

On Mon, Jul 09, 2007 at 02:16:35PM +0100, Andy Parkins wrote:
> On Monday 2007 July 09, Theodore Tso wrote:
> > On Sun, Jul 08, 2007 at 10:39:41PM -0700, Junio C Hamano wrote:
> > > Are _identifiers with leading underscore Kosher thing to do, I
> > > wonder...  We do have ones with trailing ones (mostly qsort
> > > functions) and I think they are done that way for the sake of
> > > standards conformance.
> >
> > _[a-z]* is kosher for file scopes or function scoping:
> 
> Perhaps I'm reading it wrong but:
> 
> "All identifiers beginning with an underscore are reserved for ordinary 
> identifiers (functions, variables, typedefs, enumeration constants) with file 
                                                                      ^^^^^^^^^
> scope."
  ^^^^^^
> 
> Doesn't agree with what you've said.  I think that you _can_ use _[a-z]* for 
> labels or structure members - however, not within file or function scope.

I think the above does agree with what I said.  It says that you can
use functions, variables, typdefs, enumeration constants (not just
labels or structure members) WITH FILE SCOPE.  I.e., so long as it
doesn't leak across a .o linkage.  So one .o file can use a static
_my_strdup, and another .o file can use a static _my_strdup, and they
don't have to worry about multiply defined function conflicts, since
they are static functions with file or smaller scoping.

And if it's safe to use a file-level static scoping, then obviously it
would be safe to use a function-level static scoping.

> However, the rule of thumb I've always used is "don't start identifiers with 
> underscore".  I can't think of a situation that would mean you have to use an 
> underscore to start an identifier - so why get into detailed worries about 
> where it's allowed and where it isn't.  Just don't use it.  The document you 
> linked to gives exactly this advice:

Yep, this is the safer thing to do if you don't want to remember the
more complicated rule.  But it's not *necessary*; no system library
will use a single underscore followed by a lower-case letter, since
that's reserved for programs for local file-level scoping.  A system
library will use for its private function identifiers that begin
either a double underscore, or a underscore followed by an uppercase
latter.

Regards,

							- Ted

^ permalink raw reply

* Re: [PATCH 4/4] Add git-rewrite-commits
From: Johannes Schindelin @ 2007-07-09 14:11 UTC (permalink / raw)
  To: skimo; +Cc: git, Junio C Hamano
In-Reply-To: <20070709134918.GT1528MdfPADPa@greensroom.kotnet.org>

Hi,

On Mon, 9 Jul 2007, Sven Verdoolaege wrote:

> On Mon, Jul 09, 2007 at 01:57:10PM +0100, Johannes Schindelin wrote:
>
> > On Mon, 9 Jul 2007, Sven Verdoolaege wrote:
> >
> > > What if the commit message is larger than the pipe buffer?
> > 
> > You start_command().  Then you write() until it is all written, or the 
> > pipe is broken.  Then you get the output via index_pipe().  Which is a 
> > single sha1.  I do not understand your question.
> 
> Ah, but that is not how my commit "filter" works right now. It should 
> produce the whole commit on stdout (as mentioned in the documentation). 
> So the default filter is "cat" (and not "git-hash-object -t commit 
> --stdin", as you seem to assume).

Sorry. Somehow I misread that index_pipe() for a read_pipe().

> How about I change that to a filter that accepts a single SHA1 and
> produces zero or more SHA1's as output?
> A filter for the current rewrite-commits would then be replaced
> by "xargs | git-cat-file commit | original-filter | git-hash-object -t commit --stdin"

You still have not addressed the fundamental problem!  If the commit 
filter takes the things as a whole commit buffer or as a single sha1, or 
if it writes one, two or -1 commits is not really a fundamental problem.

The real problem is that this filter can only act on _one_ commit.  Yes, 
it wants to change only one commit at a time, but it might need 
information from _other_ commits, too!

> > To enhance on the above example: you're rewriting the commit messages 
> > so that commit names are rewritten to match the rewritten commits.  
> > That is possible by a message filter in cg-admin-rewritehist.
> > 
> > But now somebody comes along, and says "I have a history I need to 
> > rewrite.  All bug fixes.  The commit names were all abbreviated in the 
> > commit messages, but they always had 'commit ' in front of them.  I 
> > want to rewrite them, too."
> 
> The current git-rewrite-commits will rewrite all SHA1's it can find,
> irrespective of any 'commit ' that may precede it.

Even abbreviated ones?

> > > What's wrong with --author='!Darl McBribe' ?
> > 
> > It is a very special use case.  Not always will you be able to get all 
> > the information from the commit object you need for conditional 
> > operations.  My example only showed that you can do the same with that 
> > syntax.  But imagine what you could do if we just added a small 
> > syntactical sugar:
> > 
> > 	-e '?has-path:README?substitute/v2/v3/'
> 
> So you want to introduce a whole language?
> Isn't that a bit over-engineering?

We already have a tool which is powerful enough to do that.  Yes, it is a 
little complicated to operate, and yes, it is slower than your version.  
But darn it, it _does_ more than your version.

It is a pity that you did not address the fundamental problem, so I have 
to spend time (that I should really spend differently) thinking about it.

Alas, I think I have a solution. You need a flag:

	--write-sha1-mappings=<directory>

Yes, it makes your code slower again, but only if you need those mappings.

Yes, it is way less convenient than the "map" function, but then you could 
automatically write a script into that directory, providing the 
convenience functions, and exporting the path so that you can say

	--commit-filter '. "$G"; ... map "$sha1"'

and also add convenience functions "save <varname>" and "restore 
<varname>" so that finally a bit of convenience is restored to the filter 
writers.

But those are no longer fundamental problems.  I'd be glad if you could 
put these suggestions to use in rewrite-commits.

While at it, you should also change the semantics for the commit filter, 
probably even less so than you suggested: since you rewrite the parents, 
and the tree, you should continue to pipe this information into the commit 
filter.  That would be another place for a convenience function "commit", 
which does the same as "git hash-object -w --stdin".

Okay now. To be precise, here is my wish list:

* rename the darned things to "filter" again.

* --write-sha1-mappings=<directory> (or --write-commit-mappings), possibly 
  defaulting to .git/mappings/.  Be careful not to overwrite an existing 
  such directory.

* change the semantics of the commit filter: the output is a list 
  (possibly empty) of replacement sha1's for this commit.

* if any filters are called, provide a script with convenience functions, 
  and an environment variable pointing to it.  These functions should 
  include:

	* map
	* commit
	* save
	* restore

Ciao,
Dscho

^ permalink raw reply

* [PATCH] rerere: record resolution even if file is not in merge base
From: Johannes Schindelin @ 2007-07-09 13:47 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Uwe Kleine-König, Git Mailing List
In-Reply-To: <7vps32ugu2.fsf@assigned-by-dhcp.cox.net>

[-- Attachment #1: Type: TEXT/PLAIN, Size: 3676 bytes --]


Two-file merges were rare enough that they were dropped outside of the 
radar.  This fix is a trivial change to builtin-rerere.c::find_conflict().  
It is still sane to insist that we do not do rerere for symlinks, and 
require to have stages #2 and #3, but we can drop the requirement to have 
stage #1. rerere does not use information from there anyway.

This fix is from Junio, together with two tests to verify that it works
as expected.

Noticed by Uwe Kleine-König.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---

	Heh, this time I steal your patch and take all the credit ;-)

 builtin-rerere.c  |   17 +++++++----------
 t/t4200-rerere.sh |   44 +++++++++++++++++++++++++++++++++++++++-----
 2 files changed, 46 insertions(+), 15 deletions(-)

diff --git a/builtin-rerere.c b/builtin-rerere.c
index c25b3d5..6ffc43d 100644
--- a/builtin-rerere.c
+++ b/builtin-rerere.c
@@ -168,19 +168,16 @@ static int find_conflict(struct path_list *conflict)
 	int i;
 	if (read_cache() < 0)
 		return error("Could not read index");
-	for (i = 0; i + 2 < active_nr; i++) {
-		struct cache_entry *e1 = active_cache[i];
-		struct cache_entry *e2 = active_cache[i+1];
-		struct cache_entry *e3 = active_cache[i+2];
-		if (ce_stage(e1) == 1 &&
-		    ce_stage(e2) == 2 &&
+	for (i = 0; i+1 < active_nr; i++) {
+		struct cache_entry *e2 = active_cache[i];
+		struct cache_entry *e3 = active_cache[i+1];
+		if (ce_stage(e2) == 2 &&
 		    ce_stage(e3) == 3 &&
-		    ce_same_name(e1, e2) && ce_same_name(e1, e3) &&
-		    S_ISREG(ntohl(e1->ce_mode)) &&
+		    ce_same_name(e2, e3) &&
 		    S_ISREG(ntohl(e2->ce_mode)) &&
 		    S_ISREG(ntohl(e3->ce_mode))) {
-			path_list_insert((const char *)e1->name, conflict);
-			i += 2;
+			path_list_insert((const char *)e2->name, conflict);
+			i++; /* skip over both #2 and #3 */
 		}
 	}
 	return 0;
diff --git a/t/t4200-rerere.sh b/t/t4200-rerere.sh
index 6f55ba0..cfcdb69 100755
--- a/t/t4200-rerere.sh
+++ b/t/t4200-rerere.sh
@@ -116,11 +116,12 @@ test_expect_success 'commit succeeds' \
 
 test_expect_success 'recorded postimage' "test -f $rr/postimage"
 
-git checkout -b third master
-git show second^:a1 | sed 's/To die: t/To die! T/' > a1
-git commit -q -a -m third
-
-test_expect_failure 'another conflicting merge' 'git pull . first'
+test_expect_success 'another conflicting merge' '
+	git checkout -b third master &&
+	git show second^:a1 | sed "s/To die: t/To die! T/" > a1 &&
+	git commit -q -a -m third &&
+	! git pull . first
+'
 
 git show first:a1 | sed 's/To die: t/To die! T/' > expect
 test_expect_success 'rerere kicked in' "! grep ======= a1"
@@ -164,4 +165,37 @@ test_expect_success 'garbage collection (part2)' 'git rerere gc'
 test_expect_success 'old records rest in peace' \
 	"test ! -f $rr/preimage && test ! -f $rr2/preimage"
 
+test_expect_success 'file2 added differently in two branches' '
+	git reset --hard &&
+	git checkout -b fourth &&
+	echo Hallo > file2 &&
+	git add file2 &&
+	git commit -m version1 &&
+	git checkout third &&
+	echo Bello > file2 &&
+	git add file2 &&
+	git commit -m version2 &&
+	! git merge fourth &&
+	sha1=$(sed -e "s/	.*//" .git/rr-cache/MERGE_RR) &&
+	rr=.git/rr-cache/$sha1 &&
+	echo Cello > file2 &&
+	git add file2 &&
+	git commit -m resolution
+'
+
+test_expect_success 'resolution was recorded properly' '
+	git reset --hard HEAD~2 &&
+	git checkout -b fifth &&
+	echo Hallo > file3 &&
+	git add file3 &&
+	git commit -m version1 &&
+	git checkout third &&
+	echo Bello > file3 &&
+	git add file3 &&
+	git commit -m version2 &&
+	! git merge fifth &&
+	git diff-files -q &&
+	test Cello = "$(cat file3)"
+'
+
 test_done
-- 
1.5.3.rc0.2769.gd9be2


^ permalink raw reply related

* Re: [PATCH 4/4] Add git-rewrite-commits
From: Sven Verdoolaege @ 2007-07-09 13:49 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: git, Junio C Hamano
In-Reply-To: <Pine.LNX.4.64.0707091257470.5546@racer.site>

On Mon, Jul 09, 2007 at 01:57:10PM +0100, Johannes Schindelin wrote:
> On Mon, 9 Jul 2007, Sven Verdoolaege wrote:
> > On Mon, Jul 09, 2007 at 12:56:04AM +0100, Johannes Schindelin wrote:
> > I thought you had to specify the name of the new branch on the command
> > line.  Anyway, I don't really care about the name of this hierarchy.
> > I just picked a name that is somewhat related to "rewrite-commits".
> > Suggestions are welcome.
> 
> How much more explicit should I formulate my suggestion?
[..]
> actually changed.  So even if you _have_ to keep your current behaviour, 
> namely that the results are stored under the original names, you should 
> store the original refs in something like refs/original (overridable by a 

I suppose I can live with "original".

> command line option), but _only if_ they actually changed.

That's what I do right now.

> You will see that both definitions apply _perfectly_ to what you do here.  
> You pipe the commit into the "map" and write it out as a new commit!  
> The _perfect_ example for a UNIX filter.

OK.  Makes sense.
I've always found that naming confusing, but it has a long tradition.
Sorry for the strong word.

> > > - The example you give with "git update-index --remove" can fail, right? 
> > 
> > Yes.  Spectacularly, even.
> 
> Does that mean you acknowledge that the man page should tell about this?  
> And probably also reveal the "|| :" remedy?

Yes.  (Am I on trial, here?)

> > > - The commit filter again deviates from the usage in cg-admin-rewritehist. 
> > >   I can see that you wanted to make it more versatile. However, it makes 
> > >   the tool potentially also a bit more cumbersome to use. Besides, you use 
> > >   a temporary file where there is no need to.
> > 
> > Are you saying I should use two pipes?
> 
> Umm.  Why not?
> 
> > What if the commit message is larger than the pipe buffer?
> 
> You start_command().  Then you write() until it is all written, or the 
> pipe is broken.  Then you get the output via index_pipe().  Which is a 
> single sha1.  I do not understand your question.

Ah, but that is not how my commit "filter" works right now.
It should produce the whole commit on stdout (as mentioned in the documentation).
So the default filter is "cat" (and not "git-hash-object -t commit --stdin",
as you seem to assume).

How about I change that to a filter that accepts a single SHA1 and
produces zero or more SHA1's as output?
A filter for the current rewrite-commits would then be replaced
by "xargs | git-cat-file commit | original-filter | git-hash-object -t commit --stdin"

> To enhance on the above example: you're rewriting the commit messages so 
> that commit names are rewritten to match the rewritten commits.  That is 
> possible by a message filter in cg-admin-rewritehist.
> 
> But now somebody comes along, and says "I have a history I need to 
> rewrite.  All bug fixes.  The commit names were all abbreviated in the 
> commit messages, but they always had 'commit ' in front of them.  I want 
> to rewrite them, too."

The current git-rewrite-commits will rewrite all SHA1's it can find,
irrespective of any 'commit ' that may precede it.
I guess I'm trying to misunderstand you again.

> Are you trying to misunderstand me?

No.

> > > But hey, maybe it _is_ time to rethink the whole filter business, and 
> > > introduce some kind of regular expression based action language. 
> > > Something like
> > > 
> > > 	git rewrite-commits -e '/^author Darl McBribe/skip-commit' \
> > 
> > What's wrong with --author='!Darl McBribe' ?
> 
> It is a very special use case.  Not always will you be able to get all the 
> information from the commit object you need for conditional operations.  
> My example only showed that you can do the same with that syntax.  But 
> imagine what you could do if we just added a small syntactical sugar:
> 
> 	-e '?has-path:README?substitute/v2/v3/'

So you want to introduce a whole language?
Isn't that a bit over-engineering?

skimo

^ permalink raw reply

* Re: rerere fails to record resolution if file doesn't exist in merge base
From: Johannes Schindelin @ 2007-07-09 13:40 UTC (permalink / raw)
  To: Uwe Kleine-König; +Cc: Junio C Hamano, Git Mailing List
In-Reply-To: <20070709113543.GA12633@informatik.uni-freiburg.de>

Hi,

On Mon, 9 Jul 2007, Uwe Kleine-K?nig wrote:

> Junio C Hamano wrote:
>
> > Not even compile tested, but something like this should do.
> It compiles, but seems not to work.  I will try to make up a test.

I compiled it, and it works as expected.  For safety, git-rerere leaves 
the index unmerged, so that you can verify that you wanted this 
resolution.

Will resubmit with a test case.

Ciao,
Dscho

^ permalink raw reply

* Re: [PATCH 4/4] Add git-rewrite-commits
From: Johannes Schindelin @ 2007-07-09 13:16 UTC (permalink / raw)
  To: Jeff King; +Cc: skimo, git, Junio C Hamano
In-Reply-To: <20070709123626.GC23494@sigill.intra.peff.net>

Hi,

On Mon, 9 Jul 2007, Jeff King wrote:

> On Mon, Jul 09, 2007 at 12:56:04AM +0100, Johannes Schindelin wrote:
> 
> > But hey, maybe it _is_ time to rethink the whole filter business, and 
> > introduce some kind of regular expression based action language. Something 
> > like
> > 
> > 	git rewrite-commits -e '/^author Darl McBribe/skip-commit' \
> > 		-e 'substitute/^author Joahnnes/author Johannes/header' \
> > 		-e 'substitute/poreclain/porcelain/body' \
> > 		-e 'rewrite-commit-names'
> 
> This is starting to look an awful lot like sed.

Yep.  Fully intended.

> Which is good, but I wonder if we can get sed to do the heavy lifting. I 
> have had success with similar systems by writing the data structure out 
> into a canonical format, editing it as text, and then "applying" the 
> result.
> 
> Something like:
>   git rewrite-generate oldbranch |
>     sed 's/^author Darl McBribe/skip-commit/'
>     sed 's/^author Joahnnes/author Johannes/' |
>     git-rewrite-commit-names |
>     git rewrite-apply newbranch
> 
> where git-rewrite-generate would generate something like git-log output,
> and git-rewrite-apply would, given a log-ish input, write a new history
> branch. A nice advantage is that it makes things like this very natural:
>   git rewrite-generate oldbranch >history
>   vi history
>   git rewrite-apply newbranch <history
> 
> which allows interactive editing.

Granted.  It is a really nice idea, but again you lack the map function.

> Of course, this is:
>   - possibly inefficient, since rewrite-apply doesn't know what you
>     changed and what you didn't change; it would have to recalculate
>     a lot of sha1 hashes.
>   - doesn't really deal with actual tree rewriting, unless there is some
>     canonical text format for that, and then we are talking about making
>     things _really_ inefficient
> 
> Hmm. Which makes me think that maybe 'git-format-patch' is really
> git-rewrite-generate, and 'git-am' is really git-rewrite-apply (but with
> some extensions to preserve committer info).

Of course, you miss out the merge commits.

> So maybe a bad idea, but I thought I would throw it out there.

Not so bad.  What you describe as possibly inefficient has been 
implemented very, very efficiently already: git-fast-import!

So your idea brings me to another idea: Why not write git-fast-export?  
Actually, let's not all it that, since repo.or.cz has that name already 
(curiously enough, it is used for scripts exporting from _other_ SCMs, 
feeding to git-fast-import...), but git-fast-dump.

The output should be _exactly_ as expected by git-fast-import, so that 
"git fast-dump | git fast-import" would be a nop, data-wise.

Of course, there is a more fundamental problem with that approach: how to 
act on the commit message, conditional on the commit header?  I know, with 
perl it would be really easy.  But then you have to write a complete perl 
script, and the whole purpose of this 
admin-rewritehist/filter-branch/rewrite-commits frackass is that it should 
be _easy_ to use.

Ciao,
Dscho

^ permalink raw reply

* Re: [PATCH v2] Make fetch-pack a builtin with an internal API
From: Andy Parkins @ 2007-07-09 13:16 UTC (permalink / raw)
  To: git; +Cc: Theodore Tso, Junio C Hamano, Daniel Barkalow
In-Reply-To: <20070709115029.GD16032@thunk.org>

On Monday 2007 July 09, Theodore Tso wrote:
> On Sun, Jul 08, 2007 at 10:39:41PM -0700, Junio C Hamano wrote:
> > Are _identifiers with leading underscore Kosher thing to do, I
> > wonder...  We do have ones with trailing ones (mostly qsort
> > functions) and I think they are done that way for the sake of
> > standards conformance.
>
> _[a-z]* is kosher for file scopes or function scoping:

Perhaps I'm reading it wrong but:

"All identifiers beginning with an underscore are reserved for ordinary 
identifiers (functions, variables, typedefs, enumeration constants) with file 
scope."

Doesn't agree with what you've said.  I think that you _can_ use _[a-z]* for 
labels or structure members - however, not within file or function scope.

However, the rule of thumb I've always used is "don't start identifiers with 
underscore".  I can't think of a situation that would mean you have to use an 
underscore to start an identifier - so why get into detailed worries about 
where it's allowed and where it isn't.  Just don't use it.  The document you 
linked to gives exactly this advice:

"Don't give anything a name with a leading underscore."

> P.S.  Note that the Linux kernel does not worry about such rules; in
> particular, _[a-z]* is very commonly used at the global scope level,
> without any problems.  It doesn't have to be as portable as git
> probably needs to be, however.

The kernel doesn't use any standard library so there is nothing for it to 
conflict with.  I don't think it's that the kernel doesn't worry about those 
rules, it's that those rules don't apply (just as they don't for glibc).  The 
underscore rules aren't a limitation of the language, they are a limitation 
imposed by the standard library.  



Andy

-- 
Dr Andy Parkins, M Eng (hons), MIET
andyparkins@gmail.com

^ permalink raw reply

* Re: [PATCH 4/4] Add git-rewrite-commits
From: Johannes Schindelin @ 2007-07-09 12:57 UTC (permalink / raw)
  To: skimo; +Cc: git, Junio C Hamano
In-Reply-To: <20070709094703.GP1528MdfPADPa@greensroom.kotnet.org>

Hi,

On Mon, 9 Jul 2007, Sven Verdoolaege wrote:

> On Mon, Jul 09, 2007 at 12:56:04AM +0100, Johannes Schindelin wrote:
>
> > - you use a lot of what was in cg-admin-rewritehist (including 
> >   adjustments I made in the documentation for filter-branch), but you
> >   also make it more confusing for people used to that tool:
> > 
> > 	- instead of leaving the original branches as they are, you 
> > 	  overwrite them. That's okay. But then you put the originals into 
> > 	  refs/rewritten. Without cg-admin-rewritehist using that name 
> > 	  for the _result_, you could explain your way out of confusion. 
> > 	  As it is, you cannot.
> 
> I thought you had to specify the name of the new branch on the command
> line.  Anyway, I don't really care about the name of this hierarchy.
> I just picked a name that is somewhat related to "rewrite-commits".
> Suggestions are welcome.

How much more explicit should I formulate my suggestion?

> I could also just not create them.  The old values are also available in 
> reflog.

There are two types of convenience.  One for the programmer, and one for 
the user.  As a user I would definitely like to see which branches were 
actually changed.  So even if you _have_ to keep your current behaviour, 
namely that the results are stored under the original names, you should 
store the original refs in something like refs/original (overridable by a 
command line option), but _only if_ they actually changed.

> > 	- in spite of doing the same as cg-admin-rewritehist with filters, 
> > 	  you call them maps. But they are no maps. They are manipulators, 
> > 	  you can call them mutators or filters, too. Given what people 
> > 	  know of cg-admin-rewritehist, you really should keep the name 
> > 	  "filter".
> 
> Nonesense.

Quite a strong word.  Let's see if you can defend it:

> They are not filters 
> (http://en.wikipedia.org/wiki/Filter_%28higher-order_function%29). They 
> are maps (http://en.wikipedia.org/wiki/Map_%28higher-order_function%29).

Aha.  Functional programming.  Since when do we do functional programming 
here?  C++ is much closer to what we do.  So let's see:

http://en.wikipedia.org/wiki/Map_%28C%2B%2B_container%29

Does not apply at all here.  But does something else apply?  Ah, there is 
something about mathematics.  The basis of programming.  Maybe it is 
helpful:

http://en.wikipedia.org/wiki/Map_%28mathematics%29

Ah, no!  It only says that it is a function.  But there is a mention about 
partial maps!  And you said something about that:

> (In cg-admin-rewritehist some of them are (partial) filters,
> but the ones I have are not filters).

Alas, Wikipedia says about a partial function that it is a function which 
maps from one space into another space (not necessarily different spaces, 
though), but does not necessarily have a result for every input.

But in that sense, everything is a map.  Even rewrite-commits.  And 
commits. (Maybe you should rename that to map-maps?)

So maybe more luck with "filter"?  Since we still do not write in Haskell 
or something similar obscure, let's see if Wikipedia has something which 
is more likely applying to us.  Yes.  Software.  That surely 
must apply:

http://en.wikipedia.org/wiki/Filter_%28software%29

and even more to the point: UNIX filters:

http://en.wikipedia.org/wiki/Filter_%28Unix%29

You will see that both definitions apply _perfectly_ to what you do here.  
You pipe the commit into the "map" and write it out as a new commit!  
The _perfect_ example for a UNIX filter.

> I could extend the commit-map to remove the commit it the output is 
> empty, but it'd still be closer to a map than to a filter. (You can map 
> a commit to nothing, but a filter can't alter the elements of a list, it 
> only determines which elements are kept.)

This is not what the commit-filter of cg-admin-rewritehist does.  If at 
all, your commit-map is a better example for a filter than rewritehist's 
commit-filter is.

So, you cannot defend that bold statement.

Enough ranted about an unmerited rant, let's get back to the interesting 
stuff:

All you demonstrated here is that the way "commit-map"s work right now is 
not sufficient to make more complicated rewrite rules.  You cannot split 
one commit in two, and you cannot skip it.

> > 	- the name "map" itself is used in cg-admin-rewritehist, to map 
> > 	  commit names from old to new. By using that name differently, 
> > 	  again you contribute to confusion, for no good reason.
> 
> There is a good reason to call what I call maps maps.  They _are_ maps.
> Still, I'm open for suggestions.

Are you?  Well, my suggestion still stands...  I mean, the suggestion I 
already gave in the post you quoted.  You know, to keep the name as it was 
in the program you based your work on?  Okay, I'll be explicit for once: 
Filter.

> > - get_one_line() is a misnomer. It wants to be named get_linelen().
> 
> Hmmm... I guess you missed Linus' mistake when he introduced it
> in commit.c (e3bc7a3bc7b77f44d686003f5a9346a135529f73).

Yes.  But finger pointing will not help you here.  I reviewed _your_ code.

> Do you want me to rename that one as well?

Be my guest.  But let's do that independently, will ya?

> > - instead of spawning read-tree, you could use unpack_trees() to boost
> >   performance even more. But I guess it is probably left for later, to 
> >   make it easier to review the patch.
> 
> Yeah, it looked a bit tricky for an initial implementation, especially
> where I move the HEAD forward.

Later.

> > - The example you give with "git update-index --remove" can fail, right? 
> 
> Yes.  Spectacularly, even.

Does that mean you acknowledge that the man page should tell about this?  
And probably also reveal the "|| :" remedy?

> > - The commit filter again deviates from the usage in cg-admin-rewritehist. 
> >   I can see that you wanted to make it more versatile. However, it makes 
> >   the tool potentially also a bit more cumbersome to use. Besides, you use 
> >   a temporary file where there is no need to.
> 
> Are you saying I should use two pipes?

Umm.  Why not?

> What if the commit message is larger than the pipe buffer?

You start_command().  Then you write() until it is all written, or the 
pipe is broken.  Then you get the output via index_pipe().  Which is a 
single sha1.  I do not understand your question.

> > - the more fundamental problem with the missing "map", I do not see a 
> >   reasonable way to get the same functionality from any of the code 
> >   snippets passed to rewrite-commits. Indeed, even the workaround of 
> >   cg-admin-rewritehist, to read $TEMP/map/$sha1, does not work, since you 
> >   are keeping it all in memory. On IRC, gitster suggested to use a 
> >   bidirectional pipe (such as stdin/stdout) to get at the new commit 
> >   names, but because of buffering, I guess this is no joy.
> 
> I could add an option to write $TEMP/map/$sha1, but it's not clear
> to me when such a map would be useful.  Please enlighten me.

To be flexible, you have to have a way to ask "what was this commit 
rewritten to?" from within a filter.

Example: maybe you expect the rewritten commit to have a tree identical to 
commits in a certain branch, which was _also_ possibly rewritten.  Maybe 
you want to mark these pairs, for example by a tag.  Then you need to get 
the sha1 of the rewritten commits.

> > As commented on IRC, the env/tree/parent/msg filters of 
> > cg-admin-rewritehist could be all emulated by commit filters. However, 
> > that would be really inconvenient. So at a later stage, these would 
> > have to be integrated into rewrite-commits (even if it would be 
> > possible to drive rewrite-commits by a shell porcelain, but I guess 
> > you are opposed to that idea, since you want to do everything else in 
> > C, too).
> 
> I'm not opposed to running a few commands and connecting stuff in shell. 
> (See git-submodule add, although I admit that I would have preferred to 
> do all of it in C.)

Yes, but your original version was very intrusive and hard to review.  In 
contrast, the simple shell script we have now is hackable by many, and 
easily reviewed.  Indeed, it takes but a couple of minutes to verify that 
it does what it should do and has no side effects.

But I agree, it is easy enough to run the commands from rewrite-commits.c.  
The harder part is to provide an infrastructure which makes it useful.

To enhance on the above example: you're rewriting the commit messages so 
that commit names are rewritten to match the rewritten commits.  That is 
possible by a message filter in cg-admin-rewritehist.

But now somebody comes along, and says "I have a history I need to 
rewrite.  All bug fixes.  The commit names were all abbreviated in the 
commit messages, but they always had 'commit ' in front of them.  I want 
to rewrite them, too."

No chance without a "map" function.  Except to bug you to change the C 
code for this very special need.

By contrast, filter-branch does not need to be changed for that at all.

> > However, the biggest and very real problem is that your filters do not 
> > have a "map" function to get the rewritten sha1 for a given sha1. That 
> > is what makes the filters so versatile, though, since you can skip 
> > revisions by much more complex rules than just greps on the commit 
> > message or header.
> 
> I thought your were opposed to the idea of skipping commits, since
> you still carry along the changes in those commits.

Are you trying to misunderstand me?

You can tell the user how to skip single commits.  But you have to warn 
them that they might be wanting something different, because the word 
"commit" not only implies a "revision", which you would skip in this 
context, but also a "commit diff", which you would _not_.

The "much more complex" rules clearly are for other use cases, which 
brings me to your next question:

> Do you have a use case?

_You_ already mentioned it.  Subdirectory filters.

And if you think a little further, you can easily see that for this to 
become useful, you _have_ to have a "map" function, too.  Because you are 
likely wanting to use rewrite-commits, or filter-branch, to transform big 
repositories into super/subprojects.

> > But hey, maybe it _is_ time to rethink the whole filter business, and 
> > introduce some kind of regular expression based action language. 
> > Something like
> > 
> > 	git rewrite-commits -e '/^author Darl McBribe/skip-commit' \
> 
> What's wrong with --author='!Darl McBribe' ?

It is a very special use case.  Not always will you be able to get all the 
information from the commit object you need for conditional operations.  
My example only showed that you can do the same with that syntax.  But 
imagine what you could do if we just added a small syntactical sugar:

	-e '?has-path:README?substitute/v2/v3/'

> > 		-e 'substitute/^author Joahnnes/author Johannes/header' \
> > 		-e 'substitute/poreclain/porcelain/body' \
> > 		-e 'rewrite-commit-names'
> 
> Hmmm... some of these would basically need a builtin sed. I was thinking 
> about adding --remove and --rename, though.

IMHO you will not get happy by introducing ever more, but still restricted 
functionality.

At the moment, cg-admin-rewritehist can do much more than what 
rewrite-commits can do, because the eval'ed filters give you a much 
greater freedom in what you can do (since "map" is available, and you can 
even modify the commit name mapping by the filters on the go: they can 
edit .git-rewrite/!), and how you can do it (no need to work around 
certain limitations by using temporary files).

A lot of the speed of rewrite-commits stems from the fact that it is 
limited in that way.

Don't get me wrong.  I _want_ rewrite-commits to replace filter-branch.  
But there are fundamental problems with filter-branch, and there are 
fundamental problems with having the same functionality from within C.  
And it appears to me that you do not even try to address the latter 
fundamental problems.

Ciao,
Dscho

^ permalink raw reply

* Extra Income! No School degree, work from home.             EQalPECx
From: Alex @ 2007-07-09 12:58 UTC (permalink / raw)
  To: git, git, git, git, git, git, git, git, git, git, git,
	git_odonovan, gita.champaneri, gita.das, gita.das, gita.gayatri,
	gita.gounder, gita.gunatilleke, gita.lala, gita.malik,
	gita.nandan, gita.onnen, gita.pupedis, gita.ravji, gita.singh

Hello! Please read this interesting offer if you are looking for an extra income or second job.

CPA Tools Company is currently seeking for people in Australia for "Transaction Handling" position, you do not have to have any school degree nor past experience. 

This is a genuine offer that you can rely on until December the 25th 2007. You will be able to make 1100-1600 AUD a week spending only 2 hours a day Monday-Thursday. 

Who Can Apply?

.>> 18+ y/o. 
.>> You are located in Australia.
.>> You will be able to check your e-mail several times a day.
.>> You will have couple hours of free time Monday-Thursday.

If you meet requirements you are more than welcome to obtain more information by replying to: CPAtools.business@gmail.com  Good Luck!

-----------------
HdVMsiDjjtkFVin

^ permalink raw reply

* "svn switch" equivalent when using git-svn -- git-filter-branch?
From: David Kastrup @ 2007-07-09 12:36 UTC (permalink / raw)
  To: git


Hi,

an upstream svn repository that I access with git-svn has moved.  I
seem to be too stupid to use git-filter-branch and/or .git/config
and/or git-reset to make my git mirror follow the switch.

Any pointers?

-- 
David Kastrup

^ permalink raw reply

* Re: [PATCH 4/4] Add git-rewrite-commits
From: Jeff King @ 2007-07-09 12:36 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: skimo, git, Junio C Hamano
In-Reply-To: <Pine.LNX.4.64.0707090011070.4248@racer.site>

On Mon, Jul 09, 2007 at 12:56:04AM +0100, Johannes Schindelin wrote:

> But hey, maybe it _is_ time to rethink the whole filter business, and 
> introduce some kind of regular expression based action language. Something 
> like
> 
> 	git rewrite-commits -e '/^author Darl McBribe/skip-commit' \
> 		-e 'substitute/^author Joahnnes/author Johannes/header' \
> 		-e 'substitute/poreclain/porcelain/body' \
> 		-e 'rewrite-commit-names'

This is starting to look an awful lot like sed. Which is good, but I
wonder if we can get sed to do the heavy lifting. I have had success
with similar systems by writing the data structure out into a canonical
format, editing it as text, and then "applying" the result.

Something like:
  git rewrite-generate oldbranch |
    sed 's/^author Darl McBribe/skip-commit/'
    sed 's/^author Joahnnes/author Johannes/' |
    git-rewrite-commit-names |
    git rewrite-apply newbranch

where git-rewrite-generate would generate something like git-log output,
and git-rewrite-apply would, given a log-ish input, write a new history
branch. A nice advantage is that it makes things like this very natural:
  git rewrite-generate oldbranch >history
  vi history
  git rewrite-apply newbranch <history

which allows interactive editing.

Of course, this is:
  - possibly inefficient, since rewrite-apply doesn't know what you
    changed and what you didn't change; it would have to recalculate
    a lot of sha1 hashes.
  - doesn't really deal with actual tree rewriting, unless there is some
    canonical text format for that, and then we are talking about making
    things _really_ inefficient

Hmm. Which makes me think that maybe 'git-format-patch' is really
git-rewrite-generate, and 'git-am' is really git-rewrite-apply (but with
some extensions to preserve committer info).

So maybe a bad idea, but I thought I would throw it out there.

-Peff

^ permalink raw reply

* Re: [PATCH 4/4] Add git-rewrite-commits
From: Johannes Schindelin @ 2007-07-09 11:57 UTC (permalink / raw)
  To: Johannes Sixt; +Cc: git
In-Reply-To: <4691F96E.D869DF97@eudaptics.com>

Hi,

On Mon, 9 Jul 2007, Johannes Sixt wrote:

> Sven Verdoolaege wrote:
> > I guess the major thing that is missing is --subdirectory-filter.
> > Anything else?
> 
> Yes, how about this:
> 
>   $ git rewrite-commits --index-map '
>          testresult=$($HOME/bin/expensive-test);
>          [ $testresult = t ] && $HOME/bin/tweak-index ' \
>      --commit-map '
>          [ $testresult = t ] && $HOME/bin/tweak-commit '

As skimo almost hinted: this will not work.  Once the index "map" exits, 
testresult is forgotten.

Ciao,
Dscho

^ permalink raw reply

* Re: [PATCH v2] Make fetch-pack a builtin with an internal API
From: Theodore Tso @ 2007-07-09 11:50 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Daniel Barkalow, git
In-Reply-To: <7vwsxaw2xu.fsf@assigned-by-dhcp.cox.net>

On Sun, Jul 08, 2007 at 10:39:41PM -0700, Junio C Hamano wrote:
> Are _identifiers with leading underscore Kosher thing to do, I
> wonder...  We do have ones with trailing ones (mostly qsort
> functions) and I think they are done that way for the sake of
> standards conformance.

_[a-z]* is kosher for file scopes or function scoping:

	http://c-faq.com/decl/namespace.html

Some Projects' coding styles prohibit use of _[a-z]* altogether, as
_[_A-Z]* is reserved for C libraries and system header files, and it's
simpler just to ban all identifiers begining with an underscore just
on general princinpals.  It's not strictly speaking necessary, however.

	       		     	    - Ted

P.S.  Note that the Linux kernel does not worry about such rules; in
particular, _[a-z]* is very commonly used at the global scope level,
without any problems.  It doesn't have to be as portable as git
probably needs to be, however.

^ permalink raw reply

* Re: [PATCH] Per-path attribute based hunk header selection.
From: Johannes Schindelin @ 2007-07-09 11:40 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Nicolas Pitre, Linus Torvalds, git
In-Reply-To: <7vd4z2xj34.fsf@assigned-by-dhcp.cox.net>

Hi,

On Sun, 8 Jul 2007, Junio C Hamano wrote:

> Nicolas Pitre <nico@cam.org> writes:
> 
> > On Fri, 6 Jul 2007, Junio C Hamano wrote:
> >
> >> About the comment from Johannes regarding hunk_header vs
> >> funcname, I would actually prefer hunk_header, since that is
> >> what this is about ("funcname" and "find_func" were misnomer
> >> from the beginning), but I'd rename hunk_header to funcname for
> >> the sake of consistency and minimizing the diff.
> >
> > I think "minimizing the diff" in this case is a bad reason.  Using 
> > hunk_header is so much better than funcname IMHO.
> 
> Well, even then it turns out to be a good reason, as the patch
> to rename function and field can be a separate patch.  After
> adding that "latex pattern" stuff, I am even more inclined to
> rename them.

Not to mention that even the name "hunk_header_pattern_ident" would be a 
misnomer to begin with.  It is the diff attribute we are storing there.  
Did you have any chance to look at

	http://thread.gmane.org/gmane.comp.version-control.git/51828

yet?  That should clarify things, and

	http://article.gmane.org/gmane.comp.version-control.git/51829

on top of it should clarify things even more, besides making the code a 
little faster again.

Ciao,
Dscho

^ permalink raw reply

* [PATCH] branch --track: code cleanup and saner handling of local branches
From: Johannes Schindelin @ 2007-07-09 11:35 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Paolo Bonzini, git
In-Reply-To: <7vhcoexqeh.fsf@assigned-by-dhcp.cox.net>


This patch cleans up some complicated code, and replaces it with a
cleaner version.  This also enables us to fix two cases:

The earlier "fix" to setup tracking only when the original ref started
with "refs/remotes" is wrong.  You are absolutely allowed to use a
separate layout for your tracking branches.  The correct fix, of course,
is to set up tracking information only when there is a matching
remote.<nick>.fetch line containing a colon.

Another corner case was not handled properly.  If two remotes write to
the original ref, just warn the user and do not set up tracking.

Also, the "branch name too long" condition had an off-by-one.  Not that it 
matters in real life.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---

	On Sun, 8 Jul 2007, Junio C Hamano wrote:

	> Paolo Bonzini <bonzini@gnu.org> writes:
	> 
	> >> Having prepared the patch below, I do not think if the 
	> >> original patch even wanted to have 'all' semantics.  The 
	> >> surrounding text only talks about "off a remote branch" and I 
	> >> strongly suspect that nobody wanted to do this for a local 
	> >> branch case at all.
	> >
	> > If I remember correctly, the problem was that you are not sure 
	> > that remote branches are in refs/remotes.
	> 
	> Yes, the user can use traditional layout (e.g. refs/heads/origin
	> is used as a remote tracking branch).
	> 
	> So the check with refs/remotes/ is not technically correct, but
	> it should probably look-up the configuration to check the
	> tracking, if we really want to be strict about it.

	Okay, so here is the correct fix.  In the process, I rewrote large 
	parts of it.  I really did not like the asnprintf() parts of the 
	original part, and I am not comfortable with it anyway, so that is 
	replaced, too.

	BTW if someone wonders why the wildcards look so strange in the 
	comment: after the second compilation I got sick of the "warning: 
	/* contained within comment", and wanted to spare everybody.

	Ah yes, I tried to prepare this patch with "format-patch -B", but 
	it seems to be too much in love with the few lines that match. 

 builtin-branch.c  |  196 ++++++++++++++++++++++++-----------------------------
 t/t3200-branch.sh |   21 +++---
 2 files changed, 99 insertions(+), 118 deletions(-)

diff --git a/builtin-branch.c b/builtin-branch.c
index 49195a1..d290a7a 100644
--- a/builtin-branch.c
+++ b/builtin-branch.c
@@ -22,7 +22,7 @@ static const char builtin_branch_usage[] =
 static const char *head;
 static unsigned char head_sha1[20];
 
-static int branch_track = 1; /* 0 = none, 1 = remotes, 2 = all */
+static int branch_track = 1;
 
 static int branch_use_color;
 static char branch_colors[][COLOR_MAXLEN] = {
@@ -66,12 +66,8 @@ static int git_branch_config(const char *var, const char *value)
 		color_parse(value, var, branch_colors[slot]);
 		return 0;
 	}
-	if (!strcmp(var, "branch.autosetupmerge")) {
-		if (!strcmp(value, "all"))
-			branch_track = 2;
-		else
+	if (!strcmp(var, "branch.autosetupmerge"))
 			branch_track = git_config_bool(var, value);
-	}
 
 	return git_default_config(var, value);
 }
@@ -349,125 +345,111 @@ static void print_ref_list(int kinds, int detached, int verbose, int abbrev,
 	free_ref_list(&ref_list);
 }
 
-static char *config_repo;
-static char *config_remote;
-static const char *start_ref;
+static struct {
+	const char *ref;
+	int ref_len;
+	char *remote;
+	char *merge;
+	int matches;
+} tracking;
 
-static int get_remote_branch_name(const char *value)
+static int tracking_config(const char *key, const char *value)
 {
 	const char *colon;
-	const char *end;
-
-	if (*value == '+')
-		value++;
+	int key_len, value_len, match_len;
+	char *merge = NULL;
 
-	colon = strchr(value, ':');
-	if (!colon)
+	/* we want: remote.<nick>.fetch = <merge>:<ref> */
+	if (prefixcmp(key, "remote.") || (key_len = strlen(key)) < 6 ||
+			strcmp(key + key_len - 6, ".fetch") ||
+			!(colon = strchr(value, ':')))
 		return 0;
 
-	end = value + strlen(value);
+	if (*value == '+')
+		value++;
 
 	/*
-	 * Try an exact match first.  I.e. handle the case where the
-	 * value is "$anything:refs/foo/bar/baz" and start_ref is exactly
-	 * "refs/foo/bar/baz". Then the name at the remote is $anything.
+	 * A remote.<name>.fetch value can have two forms:
+	 *
+	 * - exact:
+	 *
+	 *	refs/heads/gnu:refs/heads/my-upstream
+	 *
+	 * - wildcard:
+	 *
+	 *	refs/heads/ *:refs/remotes/gnu/ *
+	 *
+	 * try exact match first:
 	 */
-	if (!strcmp(colon + 1, start_ref)) {
+	if (!strcmp(colon + 1, tracking.ref))
 		/* Truncate the value before the colon. */
-		nfasprintf(&config_repo, "%.*s", colon - value, value);
-		return 1;
+		merge = xstrndup(value, colon - value);
+
+	/* wildcard; match_len is the length of the matching prefix */
+	else if ((value_len = strlen(value)) > tracking.ref_len &&
+			!strcmp(value + value_len - 2, "/*") &&
+			(match_len = value_len - (colon - value) - 2) > 0 &&
+			match_len < tracking.ref_len &&
+			!memcmp(colon + 1, tracking.ref, match_len)) {
+		int postfix_len = tracking.ref_len - match_len;
+		int replace_len = colon - 1 - value;
+		merge = xmalloc(replace_len + postfix_len);
+		memcpy(merge, value, replace_len);
+		memcpy(merge + replace_len,
+				tracking.ref + match_len, postfix_len);
 	}
 
-	/*
-	 * Is this a wildcard match?
-	 */
-	if ((end - 2 <= value) || end[-2] != '/' || end[-1] != '*' ||
-	    (colon - 2 <= value) || colon[-2] != '/' || colon[-1] != '*')
-		return 0;
-
-	/*
-	 * Value is "refs/foo/bar/<asterisk>:refs/baz/boa/<asterisk>"
-	 * and start_ref begins with "refs/baz/boa/"; the name at the
-	 * remote is refs/foo/bar/ with the remaining part of the
-	 * start_ref.  The length of the prefix on the RHS is (end -
-	 * colon - 2), including the slash immediately before the
-	 * asterisk.
-	 */
-	if ((strlen(start_ref) < end - colon - 2) ||
-	    memcmp(start_ref, colon + 1, end - colon - 2))
-		return 0; /* does not match prefix */
-
-	/* Replace the asterisk with the remote branch name.  */
-	nfasprintf(&config_repo, "%.*s%s",
-		   (colon - 1) - value, value,
-		   start_ref + (end - colon - 2));
-	return 1;
-}
-
-static int get_remote_config(const char *key, const char *value)
-{
-	const char *var;
-	if (prefixcmp(key, "remote."))
+	if (!merge)
 		return 0;
 
-	var = strrchr(key, '.');
-	if (var == key + 6 || strcmp(var, ".fetch"))
-		return 0;
-	/*
-	 * Ok, we are looking at key == "remote.$foo.fetch";
-	 */
-	if (get_remote_branch_name(value))
-		nfasprintf(&config_remote, "%.*s", var - (key + 7), key + 7);
+	tracking.matches++;
+	if (tracking.merge)
+		free(merge);
+	else {
+		tracking.merge = merge;
+		tracking.remote = xstrndup(key + 7, key_len - 7 - 6);
+	}
 
 	return 0;
 }
 
-static void set_branch_merge(const char *name, const char *config_remote,
-			     const char *config_repo)
-{
-	char key[1024];
-	if (sizeof(key) <=
-	    snprintf(key, sizeof(key), "branch.%s.remote", name))
-		die("what a long branch name you have!");
-	git_config_set(key, config_remote);
 
-	/*
-	 * We do not have to check if we have enough space for
-	 * the 'merge' key, since it's shorter than the
-	 * previous 'remote' key, which we already checked.
-	 */
-	snprintf(key, sizeof(key), "branch.%s.merge", name);
-	git_config_set(key, config_repo);
-}
-
-static void set_branch_defaults(const char *name, const char *real_ref)
+/*
+ * This is called when new_ref is branched off of orig_ref, and tries
+ * to infer the settings for branch.<new_ref>.{remote,merge} from the
+ * config.
+ */
+static void setup_tracking(const char *new_ref, const char *orig_ref)
 {
-	/*
-	 * name is the name of new branch under refs/heads;
-	 * real_ref is typically refs/remotes/$foo/$bar, where
-	 * $foo is the remote name (there typically are no slashes)
-	 * and $bar is the branch name we map from the remote
-	 * (it could have slashes).
-	 */
-	start_ref = real_ref;
-	git_config(get_remote_config);
-	if (!config_repo && !config_remote &&
-	    !prefixcmp(real_ref, "refs/heads/")) {
-		set_branch_merge(name, ".", real_ref);
-		printf("Branch %s set up to track local branch %s.\n",
-		       name, real_ref);
-	}
-
-	if (config_repo && config_remote) {
-		set_branch_merge(name, config_remote, config_repo);
-		printf("Branch %s set up to track remote branch %s.\n",
-		       name, real_ref);
+	tracking.ref = orig_ref;
+	tracking.ref_len = strlen(tracking.ref);
+	git_config(tracking_config);
+
+	if (tracking.matches > 1)
+		error("Not tracking: ambiguous information for ref %s",
+				orig_ref);
+	else if (tracking.matches == 1) {
+		char key[1024];
+		int n = snprintf(key, sizeof(key), "branch.%s.remote",
+				new_ref);
+		if (n < sizeof(key) - 1) {
+			git_config_set(key, tracking.remote ?
+					tracking.remote : ".");
+
+			snprintf(key, sizeof(key), "branch.%s.merge", new_ref);
+			git_config_set(key, tracking.merge);
+
+			printf("Branch %s set up to track remote branch %s.\n",
+			       new_ref, orig_ref);
+		} else
+			error("Tracking not set up: name too long: %s",
+				new_ref);
 	}
 
-	if (config_repo)
-		free(config_repo);
-	if (config_remote)
-		free(config_remote);
+	if (tracking.remote)
+		free(tracking.remote);
+	if (tracking.merge)
+		free(tracking.merge);
 }
 
 static void create_branch(const char *name, const char *start_name,
@@ -529,10 +511,8 @@ static void create_branch(const char *name, const char *start_name,
 	/* When branching off a remote branch, set up so that git-pull
 	   automatically merges from there.  So far, this is only done for
 	   remotes registered via .git/config.  */
-	if (real_ref && (track == 2 ||
-				(track == 1 &&
-				 !prefixcmp(real_ref, "refs/remotes/"))))
-		set_branch_defaults(name, real_ref);
+	if (real_ref && track)
+		setup_tracking(name, real_ref);
 
 	if (write_ref_sha1(lock, sha1, msg) < 0)
 		die("Failed to write ref: %s.", strerror(errno));
@@ -604,7 +584,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 			break;
 		}
 		if (!strcmp(arg, "--track")) {
-			track = 2;
+			track = 1;
 			continue;
 		}
 		if (!strcmp(arg, "--no-track")) {
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index a19e961..ef1eeb7 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -148,13 +148,14 @@ test_expect_success 'test tracking setup via config' \
      test $(git config branch.my3.remote) = local &&
      test $(git config branch.my3.merge) = refs/heads/master'
 
-test_expect_success 'autosetupmerge = all' '
+test_expect_success 'avoid ambiguous track' '
 	git config branch.autosetupmerge true &&
+	git config remote.ambi1.url = lalala &&
+	git config remote.ambi1.fetch = refs/heads/lalala:refs/heads/master &&
+	git config remote.ambi2.url = lilili &&
+	git config remote.ambi2.fetch = refs/heads/lilili:refs/heads/master &&
 	git branch all1 master &&
-	test -z "$(git config branch.all1.merge)" &&
-	git config branch.autosetupmerge all &&
-	git branch all2 master &&
-	test $(git config branch.all2.merge) = refs/heads/master
+	test -z "$(git config branch.all1.merge)"
 '
 
 test_expect_success 'test overriding tracking setup via --no-track' \
@@ -167,10 +168,10 @@ test_expect_success 'test overriding tracking setup via --no-track' \
      ! test "$(git config branch.my2.remote)" = local &&
      ! test "$(git config branch.my2.merge)" = refs/heads/master'
 
-test_expect_success 'test local tracking setup' \
+test_expect_success 'no tracking without .fetch entries' \
     'git branch --track my6 s &&
-     test $(git config branch.my6.remote) = . &&
-     test $(git config branch.my6.merge) = refs/heads/s'
+     test -z "$(git config branch.my6.remote)" &&
+     test -z "$(git config branch.my6.merge)"'
 
 test_expect_success 'test tracking setup via --track but deeper' \
     'git config remote.local.url . &&
@@ -182,8 +183,8 @@ test_expect_success 'test tracking setup via --track but deeper' \
 
 test_expect_success 'test deleting branch deletes branch config' \
     'git branch -d my7 &&
-     test "$(git config branch.my7.remote)" = "" &&
-     test "$(git config branch.my7.merge)" = ""'
+     test -z "$(git config branch.my7.remote)" &&
+     test -z "$(git config branch.my7.merge)"'
 
 test_expect_success 'test deleting branch without config' \
     'git branch my7 s &&
-- 
1.5.3.rc0.2769.gd9be2

^ permalink raw reply related

* Re: rerere fails to record resolution if file doesn't exist in merge base
From: Uwe Kleine-König @ 2007-07-09 11:35 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Git Mailing List
In-Reply-To: <7vps32ugu2.fsf@assigned-by-dhcp.cox.net>

Hello Junio,

Junio C Hamano wrote:
> Uwe Kleine-König  <ukleinek@informatik.uni-freiburg.de> writes:
> 
> > Anyhow, the failure still exists, even though the original report was
> > sent when rerere was still a perl script ...
> >
> > 	zeisberg@cassiopeia:/tmp/rerere$ git init
> > 	Initialized empty Git repository in .git/
> > 	zeisberg@cassiopeia:/tmp/rerere$ mkdir .git/rerere
> 
> This should be creating .git/rr-cache, but what you said in your
> message is correct regardless.
You're right.  copy-n-paste error.
 
> > In my eyes the last command should have recorded the resolution for
> > lolo, shouldn't it?
> 
> I think two-file merge (ancestor did not have it, and you and he
> added the path differently) is rare enough that it was dropped
> outside of the radar.  A fix would probably be a trivial change
> to builtin-rerere.c::find_conflict(), I think.  While it would
> still be sane to insist that we do not do rerere for symlinks,
> and require to have stages #2 and #3, we should be able to drop
> the requirement to have stage #1.  rerere does not use
> information from there anyway.
> 
> Not even compile tested, but something like this should do.
It compiles, but seems not to work.  I will try to make up a test.

Best regards
Uwe

-- 
Uwe Kleine-König

http://www.google.com/search?q=5+choose+3

^ permalink raw reply

* Re: [PATCH] branch.autosetupmerge: allow boolean values, or "all"
From: Johannes Schindelin @ 2007-07-09 11:28 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: Junio C Hamano, git
In-Reply-To: <46919692.5020708@gnu.org>

Hi,

On Sun, 8 Jul 2007, Paolo Bonzini wrote:

> [Paolo tried to hide the fact that it was Junio who wrote this:]
>
> > Having prepared the patch below, I do not think if the original patch 
> > even wanted to have 'all' semantics.  The surrounding text only talks 
> > about "off a remote branch" and I strongly suspect that nobody wanted 
> > to do this for a local branch case at all.
> 
> If I remember correctly, the problem was that you are not sure that 
> remote branches are in refs/remotes.

Then you code is incorrect.

Basically, you use a confusing set of four functions to do the following:

- read the config, and
- write the branch.<name>.{remote,merge} variables

Two functions would have been sufficient, and easier to read.  And as I 
fully expect with non-simple code, a bug was lurking.  This time in 
set_branch_defaults():

you check if neither config_repo nor config_remote (which is a misnomer, 
as it does not contain a "remote", but a "remote branch") is set.  But 
that happens when there was no information in the config, too!

Also you miss the case that there is ambiguous information:

[remote "hello"]
	url = git://blub/x.git
	fetch = refs/heads/master:refs/heads/origin

[remote "bello"]
	url = git://yaddayadda/x.git
	fetch = refs/heads/master:refs/heads/origin

See? Your code just uses "bello".

Will send out a fix shortly.

Ciao,
Dscho

^ permalink raw reply

* Re: Recent issues
From: Johannes Sixt @ 2007-07-09 11:03 UTC (permalink / raw)
  To: Paul Mackerras; +Cc: Junio C Hamano, git
In-Reply-To: <18066.4639.682605.171418@cargo.ozlabs.ibm.com>

Paul Mackerras wrote:
> 
> Johannes Sixt writes:
> 
> > In general, there is a big down-side, although I don't know if this
> > particular case is affected: --pretty=raw prints the parents noted in
> > the commit object, but not those from .git/info/grafts.
> 
> Are you referring to the parents listed on the "commit" line when you
> have the --parents flag, or the parents listed on "parent" lines in
> the commit?  Gitk uses the former, and I hope that does in fact
> reflect grafts.  It certainly reflects the "logical" topology when
> using path limiting.

It seems that git log --parents --pretty=raw prints the "logical" parent
in the "commit" line, i.e. the grafted parent, but the real parent in
the "parent" lines.

Since gitk uses the "commit" line, all is well. Please ignore my
comment.

-- Hannes

^ permalink raw reply

* Re: Recent issues
From: Johannes Sixt @ 2007-07-09 10:20 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano
In-Reply-To: <18065.65446.474822.436800@cargo.ozlabs.ibm.com>

Paul Mackerras wrote:
> 
> Junio C Hamano writes:
> 
> > * gitk --left-right
> >
> >   From: Linus Torvalds <torvalds@linux-foundation.org>
> >   Message-ID: <alpine.LFD.0.98.0705051524300.17381@woody.linux-foundation.org>
> >   From: Junio C Hamano <junkio@cox.net>
> >   Message-ID: <7vabwifl23.fsf@assigned-by-dhcp.cox.net>
> >
> > Paulus?
> 
> Looks mostly OK; I assume there's no possible downside to using git
> log -v --pretty=raw rather than git rev-list?

In general, there is a big down-side, although I don't know if this
particular case is affected: --pretty=raw prints the parents noted in
the commit object, but not those from .git/info/grafts.

-- Hannes

^ permalink raw reply

* Re: Recent issues
From: Paul Mackerras @ 2007-07-09  9:28 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, jnareb, pasky, ltuikov
In-Reply-To: <7v7ipcbrfd.fsf@assigned-by-dhcp.cox.net>

Junio C Hamano writes:

> * gitk --left-right
> 
>   From: Linus Torvalds <torvalds@linux-foundation.org>
>   Message-ID: <alpine.LFD.0.98.0705051524300.17381@woody.linux-foundation.org>
>   From: Junio C Hamano <junkio@cox.net>
>   Message-ID: <7vabwifl23.fsf@assigned-by-dhcp.cox.net>
> 
> Paulus?

Looks mostly OK; I assume there's no possible downside to using git
log -v --pretty=raw rather than git rev-list?

Also, in the second patch, commitside needs to either be indexed by
view as well as id, or needs to be "context-switched" by showview.
I'll fix it.  Since neither you nor Linus included a signed-off-by,
I'll combine the patches and fix up a few things.

> * gitk tree view fix
> 
>   From: Brian Downing <bdowning@lavos.net>
>   Subject: [PATCH] gitk: Fix for tree view ending in nested directories
>   Message-ID: <20070704212643.GR4087@lavos.net>
> 
> Paulus?

Looks reasonable.

Paul.

^ permalink raw reply

* Re: [PATCH 4/4] Add git-rewrite-commits
From: Sven Verdoolaege @ 2007-07-09  9:48 UTC (permalink / raw)
  To: Johannes Sixt; +Cc: git
In-Reply-To: <4691F96E.D869DF97@eudaptics.com>

On Mon, Jul 09, 2007 at 11:01:34AM +0200, Johannes Sixt wrote:
> Sven Verdoolaege wrote:
> > I guess the major thing that is missing is --subdirectory-filter.
> > Anything else?
> 
> Yes, how about this:
> 
>   $ git rewrite-commits --index-map '
>          testresult=$($HOME/bin/expensive-test);
>          [ $testresult = t ] && $HOME/bin/tweak-index ' \
>      --commit-map '
>          [ $testresult = t ] && $HOME/bin/tweak-commit '
> 
> :-P

You can write the result of expensive-test to a temporary file.

skimo

^ permalink raw reply

* Re: [PATCH 4/4] Add git-rewrite-commits
From: Sven Verdoolaege @ 2007-07-09  9:47 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: git, Junio C Hamano
In-Reply-To: <Pine.LNX.4.64.0707090011070.4248@racer.site>

Thanks for the review.

On Mon, Jul 09, 2007 at 12:56:04AM +0100, Johannes Schindelin wrote:
> - you use a lot of what was in cg-admin-rewritehist (including 
>   adjustments I made in the documentation for filter-branch), but you also 
>   make it more confusing for people used to that tool:
> 
> 	- instead of leaving the original branches as they are, you 
> 	  overwrite them. That's okay. But then you put the originals into 
> 	  refs/rewritten. Without cg-admin-rewritehist using that name 
> 	  for the _result_, you could explain your way out of confusion. 
> 	  As it is, you cannot.

I thought you had to specify the name of the new branch on the command
line.  Anyway, I don't really care about the name of this hierarchy.
I just picked a name that is somewhat related to "rewrite-commits".
Suggestions are welcome.  I could also just not create them.
The old values are also available in reflog.

> 	- in spite of doing the same as cg-admin-rewritehist with filters, 
> 	  you call them maps. But they are no maps. They are manipulators, 
> 	  you can call them mutators or filters, too. Given what people 
> 	  know of cg-admin-rewritehist, you really should keep the name 
> 	  "filter".

Nonesense.  They are not filters
(http://en.wikipedia.org/wiki/Filter_%28higher-order_function%29).
They are maps (http://en.wikipedia.org/wiki/Map_%28higher-order_function%29).
(In cg-admin-rewritehist some of them are (partial) filters,
but the ones I have are not filters).
I could extend the commit-map to remove the commit it the output is
empty, but it'd still be closer to a map than to a filter.
(You can map a commit to nothing, but a filter can't alter
the elements of a list, it only determines which elements are kept.)

> 	- the name "map" itself is used in cg-admin-rewritehist, to map 
> 	  commit names from old to new. By using that name differently, 
> 	  again you contribute to confusion, for no good reason.

There is a good reason to call what I call maps maps.  They _are_ maps.
Still, I'm open for suggestions.

> - get_one_line() is a misnomer. It wants to be named get_linelen().

Hmmm... I guess you missed Linus' mistake when he introduced it
in commit.c (e3bc7a3bc7b77f44d686003f5a9346a135529f73).
Do you want me to rename that one as well?

> - instead of spawning read-tree, you could use unpack_trees() to boost 
>   performance even more. But I guess it is probably left for later, to 
>   make it easier to review the patch.

Yeah, it looked a bit tricky for an initial implementation, especially
where I move the HEAD forward.

> - The example you give with "git update-index --remove" can fail, right? 

Yes.  Spectacularly, even.

> - The commit filter again deviates from the usage in cg-admin-rewritehist. 
>   I can see that you wanted to make it more versatile. However, it makes 
>   the tool potentially also a bit more cumbersome to use. Besides, you use 
>   a temporary file where there is no need to.

Are you saying I should use two pipes?
What if the commit message is larger than the pipe buffer?

> - the more fundamental problem with the missing "map", I do not see a 
>   reasonable way to get the same functionality from any of the code 
>   snippets passed to rewrite-commits. Indeed, even the workaround of 
>   cg-admin-rewritehist, to read $TEMP/map/$sha1, does not work, since you 
>   are keeping it all in memory. On IRC, gitster suggested to use a 
>   bidirectional pipe (such as stdin/stdout) to get at the new commit 
>   names, but because of buffering, I guess this is no joy.

I could add an option to write $TEMP/map/$sha1, but it's not clear
to me when such a map would be useful.  Please enlighten me.

> As commented on IRC, the env/tree/parent/msg filters of 
> cg-admin-rewritehist could be all emulated by commit filters. However, 
> that would be really inconvenient. So at a later stage, these would have 
> to be integrated into rewrite-commits (even if it would be possible to 
> drive rewrite-commits by a shell porcelain, but I guess you are opposed to 
> that idea, since you want to do everything else in C, too).

I'm not opposed to running a few commands and connecting stuff in shell.
(See git-submodule add, although I admit that I would have preferred to
do all of it in C.)

> However, the biggest and very real problem is that your filters do not 
> have a "map" function to get the rewritten sha1 for a given sha1. That is 
> what makes the filters so versatile, though, since you can skip revisions 
> by much more complex rules than just greps on the commit message or 
> header.

I thought your were opposed to the idea of skipping commits, since
you still carry along the changes in those commits.
Do you have a use case?

> But hey, maybe it _is_ time to rethink the whole filter business, and 
> introduce some kind of regular expression based action language. Something 
> like
> 
> 	git rewrite-commits -e '/^author Darl McBribe/skip-commit' \

What's wrong with --author='!Darl McBribe' ?

> 		-e 'substitute/^author Joahnnes/author Johannes/header' \
> 		-e 'substitute/poreclain/porcelain/body' \
> 		-e 'rewrite-commit-names'

Hmmm... some of these would basically need a builtin sed.
I was thinking about adding --remove and --rename, though.

skimo

^ permalink raw reply

* Re: [RFC][PATCH] Re: git-rm isn't the inverse action of git-add
From: Matthieu Moy @ 2007-07-09  9:45 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: git, Jan Hudec, Yann Dirson, Christian Jaeger
In-Reply-To: <Pine.LNX.4.64.0707082240510.4248@racer.site>

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Hi,
>
> On Sun, 8 Jul 2007, Matthieu Moy wrote:
>
>> I'm not sure whether this is really wrong. The things git should
>> really care about are the index and the repository itself, and the
>> proposed behavior is consistant regarding that (either remove all
>> files from the index, or remove none).
>
> Well, I think it is wrong for the same reason as it is wrong to apply the 
> changes to _any_ file when one would fail.  And since "git apply" shares 
> my understanding, I think "git rm" should, too.

OK, let's say I'm convinced ;-).

>> > I suspect that this case does never fail. 0 means success for 
>> > remove_file().  Not good.  You should at least have a way to ensure that 
>> > it removed the files from the working tree from a script.  Otherwise there 
>> > is not much point in returning a value to begin with.
>> 
>> I've changed it to have exit_status = 1 if git-rm aborted before
>> starting, and 2 if git-rm skiped some file removals (and of course, 0
>> if everything is done as expected).
>
> Oh, so you do not take the return value of this function to determine if 
> it has or has not done something with the files?  That's a bit confusing.

I did, but previously, I kept the code that "die()"s if the first call
to remove_file() "fails". In remove_file_maybe(), not removing a file
because it's not sure it's safe to delete it is not a failure, so I
had to put a "return 0;" here to avoid the fatal error. My first patch
had return status !=0 if we tried to remove the file, and it failed. I
changed that.

> I meant to complain about your OP, but this time it is even worse.  The 
> best way to guarantee that a patch gets lost in a thread is to move it _at 
> the end_ of a reply.

I had posted the patch for info, but I did expect this one to get
lost, since it's definitely not complete.

I'll post an updated patch with a testcase and an appropriate subject
line within a few days (I don't have time right now).

Thanks,

-- 
Matthieu

^ permalink raw reply


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