Git development
 help / color / mirror / Atom feed
* reflog by default?, was Re: What's in git.git (stable)
From: Johannes Schindelin @ 2006-12-14 14:03 UTC (permalink / raw)
  To: Shawn Pearce; +Cc: Carl Worth, Andy Parkins, git
In-Reply-To: <20061214120518.GL1747@spearce.org>

Hi,

On Thu, 14 Dec 2006, Shawn Pearce wrote:

>  * Normal working repository (wants reflogs);
>  * Bare private (backup) repository (wants reflogs);
>  * Bare shared repository (probably doesn't want reflogs);
>  * Import generated repository (probably doesn't want reflogs);

In contrast, I think that reflogs make lots of sense for shared repos, 
and less sense for bare (non-shared) ones...

So, I'd say: enable reflog by default, unless it is bare _and_ not shared. 
But then, cmd_init_db() no longer knows if it was called with "--bare" or 
not.

Ciao,
Dscho

^ permalink raw reply

* Re: [PATCH] Avoid accessing a slow working copy during diffcore operations.
From: Alex Riesen @ 2006-12-14 13:57 UTC (permalink / raw)
  To: Shawn O. Pearce; +Cc: Junio C Hamano, git
In-Reply-To: <20061214111557.GA24297@spearce.org>

On 12/14/06, Shawn O. Pearce <spearce@spearce.org> wrote:
> If Git is compiled with NO_FAST_WORKING_DIRECTORY set then we will
> avoid looking at the working directory when the blob in question
> is available within a packfile and the caller doesn't need the data
> unpacked into a temporary file.


^ permalink raw reply

* Re: git-fetching from a big repository is slow
From: Andreas Ericsson @ 2006-12-14 13:53 UTC (permalink / raw)
  To: Andy Parkins; +Cc: git
In-Reply-To: <200612141340.43925.andyparkins@gmail.com>

Andy Parkins wrote:
> Hello,
> 
> I've got a big repository.  I've got two computers.  One has the repository 
> up-to-date (164M after repack); one is behind (30M ish).
> 
> I used git-fetch to try and update; and the sync took HOURS.  I zipped 
> the .git directory and transferred that and it took about 15 minutes to 
> transfer.
> 
> Am I doing something wrong?  The git-fetch was done with a git+ssh:// URL.  
> The zip transfer with scp (so ssh shouldn't be a factor).
> 

This seems to happen if your repository consists of many large binary 
files, especially many large binary files of several versions that do 
not deltify well against each other. Perhaps it's worth adding gzip 
compression detecion to git? I imagine more people than me are tracking 
gzipped/bzip2'ed content that pretty much never deltifies well against 
anything else.

-- 
Andreas Ericsson                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se

^ permalink raw reply

* [PATCH] Add --add option to git-repo-config
From: Brian Gernhardt @ 2006-12-14 13:46 UTC (permalink / raw)
  To: git

For multivars, the "git-repo-config name value ^$" is useful but
nonintuitive and troublesome to do repeatedly (since the value is not
at the end of the command line).  This commit simply adds an --add
option that adds a new value to a multivar.  Particularly useful for
tracking a new branch on a remote:

git-repo-config --add remote.origin.fetch next:origin/next

Includes documentation so new users can find it and and a test to
make sure it works.

Signed-off-by: Brian Gernhardt <benji@silverinsanity.com>
---

Found myself wanting this so decided to code it myself instead of
just complaining.

Documentation/git-repo-config.txt |   14 +++++++++++++-
builtin-repo-config.c             |    6 ++++--
t/t1300-repo-config.sh            |   10 ++++++++++
3 files changed, 27 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-repo-config.txt b/Documentation/git- 
repo-config.txt
index 5bede9a..b379ec5 100644
--- a/Documentation/git-repo-config.txt
+++ b/Documentation/git-repo-config.txt
@@ -10,6 +10,7 @@ SYNOPSIS
  --------
  [verse]
  'git-repo-config' [--global] [type] name [value [value_regex]]
+'git-repo-config' [--global] [type] --add name value
  'git-repo-config' [--global] [type] --replace-all name [value  
[value_regex]]
  'git-repo-config' [--global] [type] --get name [value_regex]
  'git-repo-config' [--global] [type] --get-all name [value_regex]
@@ -23,7 +24,8 @@ You can query/set/replace/unset options with this  
command. The name is
  actually the section and the key separated by a dot, and the value  
will be
  escaped.
-If you want to set/unset an option which can occur on multiple
+Multiple lines can be added to an option by using the '--add' option.
+If you want to update or unset an option which can occur on multiple
  lines, a POSIX regexp `value_regex` needs to be given.  Only the
  existing values that match the regexp are updated or unset.  If
  you want to handle the lines that do *not* match the regex, just
@@ -53,6 +55,10 @@ OPTIONS
  	Default behavior is to replace at most one line. This replaces
  	all lines matching the key (and optionally the value_regex).
+--add::
+	Adds a new line to the option without altering any existing
+	values.  This is the same as providing '^$' as the value_regex.
+
--get::
  	Get the value for a given key (optionally filtered by a regex
  	matching the value). Returns error code 1 if the key was not
@@ -194,6 +200,12 @@ To actually match only values with an  
exclamation mark, you have to
  % git repo-config section.key value '[!]'
  ------------
+To add a new proxy, without altering any of the existing ones, use
+
+------------
+% git repo-config core.gitproxy '"proxy" for example.com'
+------------
+
include::config.txt[]
diff --git a/builtin-repo-config.c b/builtin-repo-config.c
index 7b6e572..64fbdb7 100644
--- a/builtin-repo-config.c
+++ b/builtin-repo-config.c
@@ -3,7 +3,7 @@
  #include <regex.h>
  static const char git_config_set_usage[] =
-"git-repo-config [ --global ] [ --bool | --int ] [--get | --get-all  
| --get-regexp | --replace-all | --unset | --unset-all] name [value  
[value_regex]] | --list";
+"git-repo-config [ --global ] [ --bool | --int ] [--get | --get-all  
| --get-regexp | --replace-all | --add | --unset | --unset-all] name  
[value [value_regex]] | --list";
  static char *key;
  static regex_t *key_regexp;
@@ -190,7 +190,9 @@ int cmd_repo_config(int argc, const char **argv,  
const char *prefix)
  			use_key_regexp = 1;
  			do_all = 1;
  			return get_value(argv[2], argv[3]);
-		} else if (!strcmp(argv[1], "--replace-all"))
+		} else if (!strcmp(argv[1], "--add"))
+			return git_config_set_multivar(argv[2], argv[3], "^$", 0);
+		else if (!strcmp(argv[1], "--replace-all"))
  			return git_config_set_multivar(argv[2], argv[3], NULL, 1);
  		else
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 0de2497..16cd642 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -265,6 +265,16 @@ EOF
  test_expect_success '--get-regexp' \
  	'git-repo-config --get-regexp in > output && cmp output expect'
+git-repo-config --add nextsection.nonewline "wow4 for you"
+
+cat > expect << EOF
+wow2 for me
+wow4 for you
+EOF
+
+test_expect_success '--add' \
+	'git-repo-config --get-all nextsection.nonewline > output && cmp  
output expect'
+
  cat > .git/config << EOF
  [novalue]
  	variable
--
1.4.4.1.GIT

^ permalink raw reply related

* Re: [PATCH] merge-recursive: add/add really is modify/modify with an empty base
From: Johannes Schindelin @ 2006-12-14 13:44 UTC (permalink / raw)
  To: Catalin Marinas; +Cc: Junio C Hamano, git
In-Reply-To: <b0943d9e0612140331q4c3a32e2l361fd04375f091d7@mail.gmail.com>

Hi,

On Thu, 14 Dec 2006, Catalin Marinas wrote:

> On 13/12/06, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> > xdl_merge(), as Git uses it, tries harder to find the true conflicts. So,
> > if the files actually differ in only one line, just this line will be
> > shown as conflict.
> 
> I gave the latest GIT a try and it works OK with StGIT.

Great!

> What it the relation between git-merge-recursive and "git-read-tree
> -m" (if any)?

git-merge-recursive internally calls the equivalent of 'git-read-tree -m' 
(it does not fork() and exec(), but rather calls the C functions 
directly) and does nothing more if the 'git-write-tree' succeeds. At least 
that's the theory...

If the git-write-tree does not succeed, it is because there are unmerged 
entries, and only then, merge-recursive does the rename detection and the 
file level merges.

> One nice addition to git-merge-recursive (probably only useful to
> StGIT) would be more meaningful labeling of the conflict regions,
> passed via a command line similar to the "diff3 -L" option. StGIT
> generates "patched", "current" and "ancestor" labels with diff3.

This is possible. However, it is not _that_ horrible to see "HEAD" and 
some SHA1 which is obviously non-HEAD. Added to that, a quite common case 
are the intermediate merges which make merge-recursive so powerful, and 
they are rightly called "Temporary branch 1" and "... 2".

> Yet another nice feature would be the ancestor region (which diff3
> doesn't add either but it gets added by emacs'
> ediff-merge-files-with-ancestor function if you use the interactive
> merge with StGIT).

Feasible, yes. But the code was written without that idea, so it would 
involve changes in the xdmerge_t structure (add i0, chg0), recording that 
in xdl_append_merge(), and passing also the base to 
xdl_fill_merge_buffer().

Is this really that nice? I never needed it... Besides, it can get really 
crappy when the conflicting regions are too large.

Ciao,
Dscho

^ permalink raw reply

* git-fetching from a big repository is slow
From: Andy Parkins @ 2006-12-14 13:40 UTC (permalink / raw)
  To: git

Hello,

I've got a big repository.  I've got two computers.  One has the repository 
up-to-date (164M after repack); one is behind (30M ish).

I used git-fetch to try and update; and the sync took HOURS.  I zipped 
the .git directory and transferred that and it took about 15 minutes to 
transfer.

Am I doing something wrong?  The git-fetch was done with a git+ssh:// URL.  
The zip transfer with scp (so ssh shouldn't be a factor).



Andy
-- 
Dr Andy Parkins, M Eng (hons), MIEE

^ permalink raw reply

* Re: What's in git.git (stable)
From: Andy Parkins @ 2006-12-14 13:20 UTC (permalink / raw)
  To: git
In-Reply-To: <20061214121048.GM1747@spearce.org>

On Thursday 2006 December 14 12:10, Shawn Pearce wrote:

> not predict future results"...  The size of objects in the pack
> tends to be small up front (commits/trees) and larger in the back
> (blobs).  The size distribution probably also gets more erratic
> near the back as the blob sizes may not follow a nice distribution.

Oh well; that pretty much settles it then.

> But as you state, its easy to refine it over time, and the closer we
> get to the end the more likely it is to be correct.  Unless its that
> 23 MiB blob.  As it takes up about 85% of that repository's pack.

I had imagined (foolishly), that most objects would be diffs, and would be 
similarly sized.

Scratch that.

Andy
-- 
Dr Andy Parkins, M Eng (hons), MIEE

^ permalink raw reply

* [PATCH] Cleanup git-diff documentation
From: Brian Gernhardt @ 2006-12-14 12:32 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Johannes Schindelin
In-Reply-To: <Pine.LNX.4.63.0612140957590.3635@wbgn013.biozentrum.uni-wuerzburg.de>

There were a couple inconsistencies in the formatting of the command
descriptions and a more descriptive initial description is better.

Signed-off-by: Brian Gernhardt <benji@silverinsanity.com>
---

I agree with Johannes about the original being clearer.  Plus two of
the command examples were missing brackets around -- and one had a
* at the beginning the others didn't.  This patch is against Junio's
changes made on master (1d770), even though his patch didn't apply
cleanly.

Documentation/git-diff.txt |   11 +++++++----
1 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-diff.txt b/Documentation/git-diff.txt
index 534f824..2ba9145 100644
--- a/Documentation/git-diff.txt
+++ b/Documentation/git-diff.txt
@@ -12,7 +12,10 @@ SYNOPSIS
DESCRIPTION
-----------
-This command shows changes between four combinations of states.
+Show changes between two trees, a tree and the working tree, a
+tree and the index file, or the index file and the working tree.
+The combination of what is compared with what is determined by
+the number of trees given to the command.
'git-diff' [--options] [--] [<path>...]::
@@ -25,11 +28,11 @@ This command shows changes between four  
combinations of states.
'git-diff' [--options] --cached [<commit>] [--] [<path>...]::
	This form is to view the changes you staged for the next
-	commit relative to the named <tree-ish>.  Typically you
+	commit relative to the named <commit>.  Typically you
	would want comparison with the latest commit, so if you
	do not give <commit>, it defaults to HEAD.
-'git-diff' [--options] <commit> -- [<path>...]::
+'git-diff' [--options] <commit> [--] [<path>...]::
	This form is to view the changes you have in your
	working tree relative to the named <commit>.  You can
@@ -37,7 +40,7 @@ This command shows changes between four  
combinations of states.
	branch name to compare with the tip of a different
	branch.
-* 'git-diff' [--options] <commit> <commit> -- [<path>...]
+'git-diff' [--options] <commit> <commit> [--] [<path>...]
	This form is to view the changes between two <commit>,
	for example, tips of two branches.
--

^ permalink raw reply related

* Re: [PATCH 1/1] Bypass expensive content comparsion during rename detection
From: Shawn Pearce @ 2006-12-14 12:13 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Johannes.Schindelin, git
In-Reply-To: <200612141126.kBEBQme7021704@hera.kernel.org>

Junio C Hamano <junio@hera.kernel.org> wrote:
> estimate_similarity() is called after it has determined that one
> and two do not match exactly, and it relies on populate_filespec
> with at least size-only has been called on them so that it can
> reject filepair with vastly different sizes, still without
> looking at (thus without loading) the contents.

*ouch* That hurts.  OK, it makes a lot of sense, but its crazy.
About as crazy as the comment I put in my revised patch...  ;-)

-- 

^ permalink raw reply

* Re: What's in git.git (stable)
From: Shawn Pearce @ 2006-12-14 12:10 UTC (permalink / raw)
  To: Andy Parkins; +Cc: git
In-Reply-To: <200612141200.42875.andyparkins@gmail.com>

Andy Parkins <andyparkins@gmail.com> wrote:
> I wonder if the number of objects is a reasonable measure of progress.  Let's 
> say we're transferring 100,000 objects.  Let's also say that the average size 
> of objects is 100 bytes.  Let's finally say that the object sizes are evenly 
> distributed throughout the 100,000 objects.  This would mean that the first 
> 1,000 objects are just as representative as the last 1,000 objects; or any 
> other randomly chosen 1,000 objects.  In which case, the size of the first 
> thousand objects would be approximately one hundredth the size of the total 
> transfer.  Volia: an estimate of the total size of the transfer.

Ah, but much like those stock scam emails, "prior performance does
not predict future results"...  The size of objects in the pack
tends to be small up front (commits/trees) and larger in the back
(blobs).  The size distribution probably also gets more erratic
near the back as the blob sizes may not follow a nice distribution.

E.g. I have a repository with a blob that is 23 MiB.  But I also
have some 5 MiB blobs, and then a very large number of relatively
small blobs.  That 23 MiB blob really gums up any estimate.

But as you state, its easy to refine it over time, and the closer we
get to the end the more likely it is to be correct.  Unless its that
23 MiB blob.  As it takes up about 85% of that repository's pack.

-- 

^ permalink raw reply

* Re: What's in git.git (stable)
From: Carl Worth @ 2006-12-14 11:58 UTC (permalink / raw)
  To: Shawn Pearce; +Cc: Andy Parkins, git
In-Reply-To: <20061214114546.GI1747@spearce.org>

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

On Thu, 14 Dec 2006 06:45:46 -0500, Shawn Pearce wrote:
> At this point I usually try to politely suggest that users do:
>
>   git repo-config --global core.logAllRefUpdates true
>
> and in the future do something like:

This when-you-first-learn-you-want-them-it's-too-late-to-get-them
aspect of ref logs is really annoying. It sets up an unkind trap for
users.

I know several people have suggested they be enabled by
default. What's the status of that suggestion?  Rejected? Just
awaiting a patch?

-Carl

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

^ permalink raw reply

* Re: What's in git.git (stable)
From: Shawn Pearce @ 2006-12-14 12:05 UTC (permalink / raw)
  To: Carl Worth; +Cc: Andy Parkins, git
In-Reply-To: <8764ce6654.wl%cworth@cworth.org>

Carl Worth <cworth@cworth.org> wrote:
> This when-you-first-learn-you-want-them-it's-too-late-to-get-them
> aspect of ref logs is really annoying. It sets up an unkind trap for
> users.

Yes.  Which is why its in my ~/.gitconfig.  :-(
 
> I know several people have suggested they be enabled by
> default. What's the status of that suggestion?  Rejected? Just
> awaiting a patch?

Its been suggested and discussed.

But the problem raised is that there are many types of repositories,
and not all should always have reflogs enabled, and its hard to
tell which one should and which shouldn't by default, and its even
worse to force it into a user's ~/.gitconfig as then repositories
which should not have reflogs are getting them anyway.

 * Normal working repository (wants reflogs);
 * Bare private (backup) repository (wants reflogs);
 * Bare shared repository (probably doesn't want reflogs);
 * Import generated repository (probably doesn't want reflogs);

...

Find a way to make git-init-db know the difference magically and
you'll probably see a patch emerge quickly afterwards.  But right
now I don't think anyone really has a great solution to the problem.

I know Junio wrote something on this not too long ago (and it was a
good writeup too) but I can never find threads in gmane's archives,
so I'm just going to leave that to someone else...

-- 

^ permalink raw reply

* Re: What's in git.git (stable)
From: Andy Parkins @ 2006-12-14 12:00 UTC (permalink / raw)
  To: git
In-Reply-To: <Pine.LNX.4.63.0612141224330.3635@wbgn013.biozentrum.uni-wuerzburg.de>

On Thursday 2006 December 14 11:27, Johannes Schindelin wrote:

> And now you know one of the reasons we have no true progress bar.
>
> Another reason is that it would be relatively expensive to calculate,
> since the total _size_ is not known beforehand (remember, the pack is
> calculated on the fly).

Hmmm; just thinking out loud now... I used to calculate ETA's for simulations 
I ran that had similar problems - i.e. you don't know how long it takes until 
its done (it was with genetic programming function trees, and of course you 
don't know what operations will be in the next generations tree, so you can't 
estimate a time).  I just showed "something" by doing a standard: how long 
did n/N take therefore N will take... then I plotted the error in the ETA 
after the simulation completed.  Interestingly it was always a -exp(-x) 
shape.  In other words it got more accurate towards the end (of course); 
which is exactly the sort of accuracy you would want.  At the beginning you 
just want a broad "this will take a few hours" measure.  Towards the end, you 
want to know "there is 1m50s remaining".

I wonder if the number of objects is a reasonable measure of progress.  Let's 
say we're transferring 100,000 objects.  Let's also say that the average size 
of objects is 100 bytes.  Let's finally say that the object sizes are evenly 
distributed throughout the 100,000 objects.  This would mean that the first 
1,000 objects are just as representative as the last 1,000 objects; or any 
other randomly chosen 1,000 objects.  In which case, the size of the first 
thousand objects would be approximately one hundredth the size of the total 
transfer.  Volia: an estimate of the total size of the transfer.

Obviously this estimate would be continuously updated, and would become more 
accurate as more objects are transferred.  The data rate would of course be 
based on only the previous X objects rather than the total transferred to 
take account of changing server conditions.  From these ETA could be 
estimated.

Obviously the ETA is unstable, but it's only for giving users an idea of how 
long is left; not for strict accounting.




Andy
-- 
Dr Andy Parkins, M Eng (hons), MIEE

^ permalink raw reply

* Re: [PATCH] merge-recursive: add/add really is modify/modify with an empty base
From: Shawn Pearce @ 2006-12-14 12:00 UTC (permalink / raw)
  To: Catalin Marinas; +Cc: Johannes Schindelin, Junio C Hamano, git
In-Reply-To: <b0943d9e0612140331q4c3a32e2l361fd04375f091d7@mail.gmail.com>

Catalin Marinas <catalin.marinas@gmail.com> wrote:
> What it the relation between git-merge-recursive and "git-read-tree
> -m" (if any)? I currently still use "git-read-tree -m" for some merges
> because of the speed gain due to the --agressive option (really
> noticeable when picking a patch from an older branch). Probably
> git-merge-recursive cannot implement this since it needs to track
> deletion/additions for rename detection.

There is a difference; always has been, probably will be for a
long time.

read-tree -m performs some trivial merges in the index.  Its manual
page explains it in gory detail, but its the really trivial, basic
three way merge rules: Given two trees X and Y and some so-called
base B:

 * If X == Y == B for that file, take any of the three.
 * If X == Y, but not B, take X or Y.
 * If X changes a file, but Y == B for that file, take X.
 * If Y changes a file, but X == B for that file, take Y.

I believe that the --agressive option has added some additional
rules about trivial file deletes.  However notice the all important
rule is not handled by read-tree -m:

 * If X changes a file, Y also changes file, merge them to create Z.

This is where read-tree -m punts and hands things off to
merge-recursive, which needs to invoke diff3 (or now the internal
xdl_merge).  read-tree -m also currently does not handle file
additions, mode changes, or renames/copies.  All of which are a
lot more expensive to compute and are slightly less common.

So Git gets decent performance by going through the rather cheap
read-tree -m, then falling back into the slower merge-recursive
when read-tree -m punted.  Given that I see about 50% of my merges
succeed with just read-tree -m and the other half punt over to
merge-recursive it just about balances out over several merges.

> Are there any other things to be aware if I completely replace the
> "git-read-tree + diff3" with git-merge-recursive?

From what I understand git-merge-recursive will do everything that
git-read-tree -m will do, except its going to be slower doing the
really common, stupid cases that git-read-tree -m can handle on
its own.

-- 

^ permalink raw reply

* Re: [PATCH 1/1] Bypass expensive content comparsion during rename detection
From: Junio C Hamano @ 2006-12-14 11:26 UTC (permalink / raw)
  To: Johannes.Schindelin, Johannes, Schindelin; +Cc: git

estimate_similarity() is called after it has determined that one
and two do not match exactly, and it relies on populate_filespec
with at least size-only has been called on them so that it can
reject filepair with vastly different sizes, still without
looking at (thus without loading) the contents.

^ permalink raw reply

* Re: [PATCH 1/1] Bypass expensive content comparsion during rename detection.
From: Shawn Pearce @ 2006-12-14 11:50 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: Junio C Hamano, git
In-Reply-To: <Pine.LNX.4.63.0612141214440.3635@wbgn013.biozentrum.uni-wuerzburg.de>

Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> Hi,
> 
> On Thu, 14 Dec 2006, Shawn Pearce wrote:
> 
> > My first version of the patch had the hash comparsion right after we 
> > called diff_populate_filespec to get the size data.  But then I realized 
> > that very often the sizes will be different and the src->size != 
> > dst->size comparsion will tend to be true most of the time, thus saving 
> > us a (relatively) expensive hash comparsion, which we know must fail 
> > anyway if the sizes differed.
> 
> Ah! I misunderstood. Since the call to diff_populate_filespec was not 
> visible in the hunk, I erroneously assumed that you meant to _check_ the 
> sizes before checking the hashes.
> 
> But your explanation makes lots of sense to me. May I request a short 
> comment above the new code, like "let diff_populate_filespec() do its 
> thing since we need the filesize later on anyway, and having that, do the 
> cheaper filesize check before the more expensive hashcmp()"?

-- 8> --
Bypass expensive content comparsion during rename detection.

When comparing file contents during the second loop through a rename
detection attempt we can skip the expensive byte-by-byte comparsion
if both source and destination files have valid SHA1 values.  This
improves performance by avoiding either an expensive open/mmap to
read the working tree copy, or an expensive inflate of a blob object.

Unfortunately we still have to at least initialize the sizes of the
source and destination files even if the SHA1 values don't match.
Failing to initialize the sizes causes a number of test cases to fail
and start reporting different copy/rename behavior than was expected.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 diffcore-rename.c |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/diffcore-rename.c b/diffcore-rename.c
index 57a74b6..f7748ce 100644
--- a/diffcore-rename.c
+++ b/diffcore-rename.c
@@ -109,6 +109,15 @@ static int is_exact_match(struct diff_filespec *src,
 		return 0;
 	if (src->size != dst->size)
 		return 0;
+	/* Although we can avoid a byte-by-byte comparsion by checking
+	 * hashes we needed to allow diff_populate_filespec to fill in
+	 * the size members, as we need that later on to correctly do
+	 * rename and copy detection.  Not filling in size before we
+	 * return back when contents_too is true causes all sorts of
+	 * havoc (been there, done that, lets not try it again).
+	 */
+	if (src->sha1_valid && dst->sha1_valid)
+	    return !hashcmp(src->sha1, dst->sha1);
 	if (diff_populate_filespec(src, 0) || diff_populate_filespec(dst, 0))
 		return 0;
 	if (src->size == dst->size &&
-- 
1.4.4.2.g72f5

^ permalink raw reply related

* Re: What's in git.git (stable)
From: Shawn Pearce @ 2006-12-14 11:45 UTC (permalink / raw)
  To: Andy Parkins; +Cc: git
In-Reply-To: <200612141136.59041.andyparkins@gmail.com>

Andy Parkins <andyparkins@gmail.com> wrote:
> How's this then:
> 
> $ git commit
> $ git commit
> $ git commit
> $ git reset HEAD^^^
> 
> "AGGGHHHHHH!  I meant HEAD^^"
> 
> At this point I start running "git-prune -n | grep commit" and some liberal 
> use of git-show to try and find the hash of the object so I can do

At this point I usually try to politely suggest that users do:

  git repo-config --global core.logAllRefUpdates true

and in the future do something like:

> $ git commit         # {4}
> $ git commit         # {3}
> $ git commit         # {2}
> $ git reset HEAD^^^  # {1}
> 
> "AGGGHHHHHH!  I meant HEAD^^"

  $ git reset HEAD@{4}

should give you what

  $ git reset HEAD^^

would have given had you not added the extra ^.  :-)
 
-- 

^ permalink raw reply

* Re: [PATCH] merge-recursive: add/add really is modify/modify with an empty base
From: Shawn Pearce @ 2006-12-14 11:41 UTC (permalink / raw)
  To: Catalin Marinas; +Cc: Johannes Schindelin, Junio C Hamano, git
In-Reply-To: <b0943d9e0612140331q4c3a32e2l361fd04375f091d7@mail.gmail.com>

Catalin Marinas <catalin.marinas@gmail.com> wrote:
> One nice addition to git-merge-recursive (probably only useful to
> StGIT) would be more meaningful labeling of the conflict regions,
> passed via a command line similar to the "diff3 -L" option. StGIT
> generates "patched", "current" and "ancestor" labels with diff3.

Indeed.  Getting a SHA1 hash of the branch that I merged in by
name with "git merge foof" is horrible UI.  I want to see "foof"
in the conflict.  Its in my list of things I'd really like to fix,
but is currently low priority compared to the issues I've run into
this past week with git-merge-recursive's basic functions.

On the other hand Git's open source strategy is really paying off on
resolving those problems, typically the issue is fixed and accepted
by Junio into 'next' in under a day of identification.  :-)

-- 

^ permalink raw reply

* Re: What's in git.git (stable)
From: Andy Parkins @ 2006-12-14 11:36 UTC (permalink / raw)
  To: git
In-Reply-To: <7v7iwu93rv.fsf@assigned-by-dhcp.cox.net>

On Thursday 2006 December 14 10:21, Junio C Hamano wrote:

> You keep repeating that you want to know the object name of the

Oh dear, you're right; I am terribly repetative.  Sorry.
Oh dear, you're right; I am terribly repetative.  Sorry.

;-)

> But you never communicate with your own git repository using the
> SHA-1 object names when talking about commits you made recently

How's this then:

$ git commit
$ git commit
$ git commit
$ git reset HEAD^^^

"AGGGHHHHHH!  I meant HEAD^^"

At this point I start running "git-prune -n | grep commit" and some liberal 
use of git-show to try and find the hash of the object so I can do

$ git reset --hard HASH_OF_OBJECT_I_STUPIDLY_ORPHANED

> So I do not think "git commit" is a valid example.  I also agree
> with Shawn that "git add" that says 6781 files were added is
> pointless.

Okay.

> > I've always thought that programs that needed an expert/beginner split
> > were badly designed.
>
> There probably is a truth in that.  Let's not add verbosity
> unnecessarily.

My habit is always to be overly verbose in program output; however, I realise 
that not everybody likes that.  None of these things cause me any difficulty 
in my use of git.  However, my Dad also is an engineer, but he's not so 
comfortable with VCS; for him almost every part of git is a mystery.  
Commands that run and don't say anything are confusing because he didn't 
really know what they were /meant/ to do; he's just got a set of recipes that 
he knows to type.  He's probably an extreme case, and not a good model for 
typical user - on the other hand, I would say that if he can use it, then it 
is officially newbie-friendly. :-)

> I agree with you that making some commands with progress
> indication less chatty would be a good clean-up.

These are actually the ones I feel more strongly about.  Too much output just 
drowns out the information that people really need.


Andy

-- 
Dr Andy Parkins, M Eng (hons), MIEE

^ permalink raw reply

* Re: [PATCH] merge-recursive: add/add really is modify/modify with an empty base
From: Catalin Marinas @ 2006-12-14 11:31 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: Junio C Hamano, git
In-Reply-To: <Pine.LNX.4.63.0612140045430.3635@wbgn013.biozentrum.uni-wuerzburg.de>

On 13/12/06, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> xdl_merge(), as Git uses it, tries harder to find the true conflicts. So,
> if the files actually differ in only one line, just this line will be
> shown as conflict.

I gave the latest GIT a try and it works OK with StGIT.

This new merge looks much better than diff3 (or rcs merge) because it
only shows the true conflicts.

What it the relation between git-merge-recursive and "git-read-tree
-m" (if any)? I currently still use "git-read-tree -m" for some merges
because of the speed gain due to the --agressive option (really
noticeable when picking a patch from an older branch). Probably
git-merge-recursive cannot implement this since it needs to track
deletion/additions for rename detection.

Are there any other things to be aware if I completely replace the
"git-read-tree + diff3" with git-merge-recursive?

One nice addition to git-merge-recursive (probably only useful to
StGIT) would be more meaningful labeling of the conflict regions,
passed via a command line similar to the "diff3 -L" option. StGIT
generates "patched", "current" and "ancestor" labels with diff3.

Yet another nice feature would be the ancestor region (which diff3
doesn't add either but it gets added by emacs'
ediff-merge-files-with-ancestor function if you use the interactive
merge with StGIT).

-- 

^ permalink raw reply

* Re: What's in git.git (stable)
From: Johannes Schindelin @ 2006-12-14 11:27 UTC (permalink / raw)
  To: Andy Parkins; +Cc: git
In-Reply-To: <200612141123.02346.andyparkins@gmail.com>

Hi,

On Thu, 14 Dec 2006, Andy Parkins wrote:

> On Thursday 2006 December 14 10:51, Johannes Schindelin wrote:
> 
> > If I have the choice between a "doing something" bar and a Windows
> > Explorer "14 seconds left" bar showing the same message for two minutes,
> > I'd rather have a Mars bar ;-)
> 
> Gahhhhhhhhh!  Oh how I hate that window.
> 
> On this we can wholeheartedly agree.  Unfortunately it's not just windows; 
> most applications that have a progress bar go like this:
> 
> 0%, ..., 0%,..., 0%,.., 1 , 2, 3, 4, 5, 6, 33%, ..., 33%, ..., 33%, 35, 36, 
> 85%, ..., 85%, ..., 85%, ..., 99%, 100%, ..., 100%, ... (yes, I'm completely 
> finished, but still working), ... 100%.
> 
> I reckon, unless the window with a progress bar in it has an ETA, then the 
> progress should be an ETA itself.  If it's not going to monotonically 
> increase, then the "percentage" is meaningless.

And now you know one of the reasons we have no true progress bar.

Another reason is that it would be relatively expensive to calculate, 
since the total _size_ is not known beforehand (remember, the pack is 
calculated on the fly).

Yet another reason is that all estimates there are unstable by nature: the 
load of the server, the net load, the load of the client, the speed of 
packing and unpacking, and the luck if deltas can be reused, are all 
contributors to this unstability.

Ciao,
Dscho

^ permalink raw reply

* Re: What's in git.git (stable)
From: Andy Parkins @ 2006-12-14 11:23 UTC (permalink / raw)
  To: git
In-Reply-To: <Pine.LNX.4.63.0612141147200.3635@wbgn013.biozentrum.uni-wuerzburg.de>

On Thursday 2006 December 14 10:51, Johannes Schindelin wrote:

> If I have the choice between a "doing something" bar and a Windows
> Explorer "14 seconds left" bar showing the same message for two minutes,
> I'd rather have a Mars bar ;-)

Gahhhhhhhhh!  Oh how I hate that window.

On this we can wholeheartedly agree.  Unfortunately it's not just windows; 
most applications that have a progress bar go like this:

0%, ..., 0%,..., 0%,.., 1 , 2, 3, 4, 5, 6, 33%, ..., 33%, ..., 33%, 35, 36, 
85%, ..., 85%, ..., 85%, ..., 99%, 100%, ..., 100%, ... (yes, I'm completely 
finished, but still working), ... 100%.

I reckon, unless the window with a progress bar in it has an ETA, then the 
progress should be an ETA itself.  If it's not going to monotonically 
increase, then the "percentage" is meaningless.


Andy
-- 
Dr Andy Parkins, M Eng (hons), MIEE

^ permalink raw reply

* Re: [PATCH 1/1] Bypass expensive content comparsion during rename detection.
From: Johannes Schindelin @ 2006-12-14 11:18 UTC (permalink / raw)
  To: Shawn Pearce; +Cc: Junio C Hamano, git
In-Reply-To: <20061214110858.GE1747@spearce.org>

Hi,

On Thu, 14 Dec 2006, Shawn Pearce wrote:

> My first version of the patch had the hash comparsion right after we 
> called diff_populate_filespec to get the size data.  But then I realized 
> that very often the sizes will be different and the src->size != 
> dst->size comparsion will tend to be true most of the time, thus saving 
> us a (relatively) expensive hash comparsion, which we know must fail 
> anyway if the sizes differed.

Ah! I misunderstood. Since the call to diff_populate_filespec was not 
visible in the hunk, I erroneously assumed that you meant to _check_ the 
sizes before checking the hashes.

But your explanation makes lots of sense to me. May I request a short 
comment above the new code, like "let diff_populate_filespec() do its 
thing since we need the filesize later on anyway, and having that, do the 
cheaper filesize check before the more expensive hashcmp()"?

Ciao,
Dscho

^ permalink raw reply

* [PATCH] Avoid accessing a slow working copy during diffcore operations.
From: Shawn O. Pearce @ 2006-12-14 11:15 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Alex Riesen

The Cygwin folks have done a fine job at creating a POSIX layer
on Windows That Just Works(tm).  However it comes with a penalty;
accessing files in the working tree by way of stat/open/mmap can
be slower for diffcore than inflating the data from a blob which
is stored in a packfile.

This performance problem is especially an issue in merge-recursive
when dealing with nearly 7000 added files, as we are loading
each file's content from the working directory to perform rename
detection.  I have literally seen (and sadly watched) paint dry in
less time than it takes for merge-recursive to finish such a merge.
On the other hand this very same merge runs very fast on Solaris.

If Git is compiled with NO_FAST_WORKING_DIRECTORY set then we will
avoid looking at the working directory when the blob in question
is available within a packfile and the caller doesn't need the data
unpacked into a temporary file.

We don't use loose objects as they have the same open/mmap/close
costs as the working directory file access, but have the additional
CPU overhead of needing to inflate the content before use.  So it
is still faster to use the working tree file over the loose object.

If the caller needs the file data unpacked into a temporary file
its likely because they are going to call an external diff program,
passing the file as a parameter.  In this case reusing the working
tree file will be faster as we don't need to inflate the data and
write it out to a temporary file.

The NO_FAST_WORKING_DIRECTORY feature is enabled by default on
Cygwin, as that is the platform which currently appears to benefit
the most from this option.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 Makefile |    7 +++++++
 diff.c   |   20 +++++++++++++++++---
 2 files changed, 24 insertions(+), 3 deletions(-)

diff --git a/Makefile b/Makefile
index bf4c65d..cd9992a 100644
--- a/Makefile
+++ b/Makefile
@@ -69,6 +69,9 @@ all:
 #
 # Define NO_MMAP if you want to avoid mmap.
 #
+# Define NO_FAST_WORKING_DIRECTORY if accessing objects in pack files is
+# generally faster on your platform than accessing the working directory.
+#
 # Define NO_IPV6 if you lack IPv6 support and getaddrinfo().
 #
 # Define NO_SOCKADDR_STORAGE if your platform does not have struct
@@ -355,6 +358,7 @@ ifeq ($(uname_O),Cygwin)
 	NO_SYMLINK_HEAD = YesPlease
 	NEEDS_LIBICONV = YesPlease
 	NO_C99_FORMAT = YesPlease
+	NO_FAST_WORKING_DIRECTORY = UnfortunatelyYes
 	# There are conflicting reports about this.
 	# On some boxes NO_MMAP is needed, and not so elsewhere.
 	# Try uncommenting this if you see things break -- YMMV.
@@ -506,6 +510,9 @@ ifdef NO_MMAP
 	COMPAT_CFLAGS += -DNO_MMAP
 	COMPAT_OBJS += compat/mmap.o
 endif
+ifdef NO_FAST_WORKING_DIRECTORY
+	BASIC_CFLAGS += -DNO_FAST_WORKING_DIRECTORY
+endif
 ifdef NO_IPV6
 	BASIC_CFLAGS += -DNO_IPV6
 endif
diff --git a/diff.c b/diff.c
index 0b284b3..565b23c 100644
--- a/diff.c
+++ b/diff.c
@@ -1172,7 +1172,7 @@ void fill_filespec(struct diff_filespec *spec, const unsigned char *sha1,
  * the work tree has that object contents, return true, so that
  * prepare_temp_file() does not have to inflate and extract.
  */
-static int work_tree_matches(const char *name, const unsigned char *sha1)
+static int work_tree_matches(const char *name, const unsigned char *sha1, int want_file)
 {
 	struct cache_entry *ce;
 	struct stat st;
@@ -1193,6 +1193,20 @@ static int work_tree_matches(const char *name, const unsigned char *sha1)
 	if (!active_cache)
 		return 0;
 
+#ifdef NO_FAST_WORKING_DIRECTORY
+	/* We want to avoid the working directory if our caller
+	 * doesn't need the data in a normal file, this system
+	 * is rather slow with its stat/open/mmap/close syscalls,
+	 * and the object is contained in a pack file.  The pack
+	 * is probably already open and will be faster to obtain
+	 * the data through than the working directory.  Loose
+	 * objects however would tend to be slower as they need
+	 * to be individually opened and inflated.
+	 */
+	if (!want_file && has_sha1_pack(sha1, NULL))
+		return 0;
+#endif
+
 	len = strlen(name);
 	pos = cache_name_pos(name, len);
 	if (pos < 0)
@@ -1279,7 +1293,7 @@ int diff_populate_filespec(struct diff_filespec *s, int size_only)
 	if (s->data)
 		return err;
 	if (!s->sha1_valid ||
-	    work_tree_matches(s->path, s->sha1)) {
+	    work_tree_matches(s->path, s->sha1, 0)) {
 		struct stat st;
 		int fd;
 		if (lstat(s->path, &st) < 0) {
@@ -1386,7 +1400,7 @@ static void prepare_temp_file(const char *name,
 	}
 
 	if (!one->sha1_valid ||
-	    work_tree_matches(name, one->sha1)) {
+	    work_tree_matches(name, one->sha1, 1)) {
 		struct stat st;
 		if (lstat(name, &st) < 0) {
 			if (errno == ENOENT)
-- 

^ permalink raw reply related

* Re: [PATCH 1/1] Bypass expensive content comparsion during rename detection.
From: Shawn Pearce @ 2006-12-14 11:13 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: Junio C Hamano, git
In-Reply-To: <20061214110858.GE1747@spearce.org>

Shawn Pearce <spearce@spearce.org> wrote:
> I'll admit, I don't understand the diffcore rename code very well
> so I'm treading around in code that I'm not used to.  I'm not sure
> why the size member of diff_filespec needs to be initialized to get
> rename and copy detection to work properly, but it apparently does.

This chunk of code is probably a perfect example of why side-effects
can be so bad.  Its fast because the size information is loaded
once and reused later on; its horrible to maintain because you don't
realize that this simple predicate is actually doing something that
matters downstream even though it returned false!

-- 

^ 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