Re: What's cooking in git.git (Jan 2013, #06; Mon, 14)

2013-01-14 Thread Junio C Hamano
Junio C Hamano writes: > [New Topics] > > * jc/cvsimport-upgrade (2013-01-14) 8 commits > - t9600: adjust for new cvsimport > - t9600: further prepare for sharing > - cvsimport-3: add a sample test > - cvsimport: make tests reusable for cvsimport-3 > - cvsimport: start adding cvsps 3.x suppo

Re: [PATCH v2 2/3] push: Add support for pre-push hooks

2013-01-14 Thread Junio C Hamano
Junio C Hamano writes: > At least the attached patch is necessary. Sorry, but the last hunk (see below) is not. It breaks the hook. > In the longer term, we may want to discuss what should happen when > the hook exited without even reading what we fed. My gut feeling is > that we can still tr

Re: git grep performance regression

2013-01-14 Thread Duy Nguyen
On Tue, Jan 15, 2013 at 9:46 AM, Duy Nguyen wrote: > I don't have time to look into details now, but by enabling > DEBUG_ATTR, it looks like this commit makes it push and pop patterns a > lot more than without the commit. I think the culprit is at this chunk: static void prepare_attr_stack(cons

Re: [BUG] Possible bug in `remote set-url --add --push`

2013-01-14 Thread Jardel Weyrich
On 14/01/2013, at 17:09, Junio C Hamano wrote: > Michael J Gruber writes: > >> It seems to me that everything works as designed, and that the man page >> talk about "push URLs" can be read in two ways,... > > Hmph, but I had an impression that Jardel's original report was that > one of the --a

[PATCH v2 03/19] reset.c: pass pathspec around instead of (prefix, argv) pair

2013-01-14 Thread Martin von Zweigbergk
We use the path arguments in two places in reset.c: in interactive_reset() and read_from_tree(). Both of these call get_pathspec(), so we pass the (prefix, argv) pair to both functions. Move the call to get_pathspec() out of these methods, for two reasons: 1) One argument is simpler than two. 2) It

[PATCH v2 04/19] reset: don't allow "git reset -- $pathspec" in bare repo

2013-01-14 Thread Martin von Zweigbergk
Running e.g. "git reset ." in a bare repo results in an index file being created from the HEAD commit. The differences compared to the index are then printed as usual, but since there is no worktree, it will appear as if all files are deleted. For example, in a bare clone of git.git: Unstaged ch

[PATCH v2 01/19] reset $pathspec: no need to discard index

2013-01-14 Thread Martin von Zweigbergk
Since 34110cd (Make 'unpack_trees()' have a separate source and destination index, 2008-03-06), the index no longer gets clobbered by do_diff_cache() and we can remove the code for discarding and re-reading it. There are two paths to update_index_refresh() from cmd_reset(), but on both paths, eith

[PATCH v2 02/19] reset $pathspec: exit with code 0 if successful

2013-01-14 Thread Martin von Zweigbergk
"git reset $pathspec" currently exits with a non-zero exit code if the worktree is dirty after resetting, which is inconsistent with reset without pathspec, and it makes it harder to know whether the command really failed. Change it to exit with code 0 regardless of whether the worktree is dirty so

[PATCH v2 17/19] reset $sha1 $pathspec: require $sha1 only to be treeish

2013-01-14 Thread Martin von Zweigbergk
Resetting with paths does not update HEAD and there is nothing else that a commit should be needed for. Relax the argument parsing so only a tree is required. The sha1 is only passed to read_from_tree(), which already only requires a tree. The "rev" variable we pass to run_add_interactive() will

[PATCH v2 19/19] reset [--mixed]: use diff-based reset whether or not pathspec was given

2013-01-14 Thread Martin von Zweigbergk
Thanks to b65982b (Optimize "diff-index --cached" using cache-tree, 2009-05-20), resetting with paths is much faster than resetting without paths. Some timings for the linux-2.6 repo to illustrate this (best of five, warm cache): reset reset . real0m0.219s0m0.080s user0m0

[PATCH v2 18/19] reset: allow reset on unborn branch

2013-01-14 Thread Martin von Zweigbergk
Some users seem to think, knowingly or not, that being on an unborn branch is like having a commit with an empty tree checked out, but when run on an unborn branch, "git reset" currently fails with: fatal: Failed to resolve 'HEAD' as a valid ref. Instead of making users figure out that they sho

[PATCH v2 15/19] reset.c: finish entire cmd_reset() whether or not pathspec is given

2013-01-14 Thread Martin von Zweigbergk
By not returning from inside the "if (pathspec)" block, we can let the pathspec-aware and pathspec-less code share a bit more, making it easier to make future changes that should affect both cases. This also highlights the similarity between read_from_tree() and reset_index(). Signed-off-by: Marti

[PATCH v2 13/19] reset.c: move lock, write and commit out of update_index_refresh()

2013-01-14 Thread Martin von Zweigbergk
In preparation for the/a following patch, move the locking, writing and committing of the index file out of update_index_refresh(). The code duplication caused will soon be taken care of. What remains of update_index_refresh() is just one line, but it is still called from two places, so let's leave

[PATCH v2 10/19] reset: avoid redundant error message

2013-01-14 Thread Martin von Zweigbergk
If writing or committing the new index file fails, we print "Could not write new index file." followed by "Could not reset index file to revision $rev.". The first message seems to imply the second, so print only the first message. Signed-off-by: Martin von Zweigbergk --- builtin/reset.c | 8 +++

[PATCH v2 14/19] reset [--mixed]: only write index file once

2013-01-14 Thread Martin von Zweigbergk
When doing a mixed reset without paths, the index is locked, read, reset, and written back as part of the actual reset operation (in reset_index()). Then, when showing the list of worktree modifications, we lock the index again, refresh it, and write it. Change this so we only write the index once

[PATCH v2 05/19] reset.c: extract function for parsing arguments

2013-01-14 Thread Martin von Zweigbergk
Declutter cmd_reset() a bit by moving out the argument parsing to its own function. Signed-off-by: Martin von Zweigbergk --- builtin/reset.c | 70 +++-- 1 file changed, 38 insertions(+), 32 deletions(-) diff --git a/builtin/reset.c b/builtin/r

[PATCH v2 11/19] reset.c: replace switch by if-else

2013-01-14 Thread Martin von Zweigbergk
The switch statement towards the end of reset.c is missing case arms for KEEP and MERGE for no obvious reason, and soon the only non-empty case arm will be the one for HARD. So let's proactively replace it by if-else, which will let us move one if statement out without leaving funny-looking left-ov

[PATCH v2 16/19] reset.c: inline update_index_refresh()

2013-01-14 Thread Martin von Zweigbergk
Now that there is only one caller left to the single-line method update_index_refresh(), inline it. Signed-off-by: Martin von Zweigbergk --- builtin/reset.c | 14 +- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/builtin/reset.c b/builtin/reset.c index c316d9b..520c1a5

[PATCH v2 07/19] reset.c: extract function for updating {ORIG_,}HEAD

2013-01-14 Thread Martin von Zweigbergk
By extracting the code for updating the HEAD and ORIG_HEAD symbolic references to a separate function, we declutter cmd_reset() a bit and we make it clear that e.g. the four variables {,sha1_}{,old_}orig are only used by this code. Signed-off-by: Martin von Zweigbergk --- builtin/reset.c | 39 ++

[PATCH v2 09/19] reset --keep: only write index file once

2013-01-14 Thread Martin von Zweigbergk
"git reset --keep" calls reset_index_file() twice, first doing a two-way merge to the target revision, updating the index and worktree, and then resetting the index. After each call, we write the index file. In the unlikely event that the second call to reset_index_file() fails, the index will hav

[PATCH v2 00/19] reset improvements

2013-01-14 Thread Martin von Zweigbergk
Changes since v1: - Spelling fixes. - Explained how "git reset -- $pathspec" in bare repo is broken. - Provided motivation for replacement of switch by if-else - Fixed argv/argc handling by removing use of argc. - Replaced "don't refresh index on --quiet" patch by one that just inlines

[PATCH v2 12/19] reset.c: move update_index_refresh() call out of read_from_tree()

2013-01-14 Thread Martin von Zweigbergk
The final part of cmd_reset() essentially looks like: if (pathspec) { ... read_from_tree(...); } else { ... reset_index(...); update_index_refresh(...); ... } where read_from_tree() internally also calls update_index_refresh(). Move the call to update_index_refresh()

[PATCH v2 08/19] reset.c: share call to die_if_unmerged_cache()

2013-01-14 Thread Martin von Zweigbergk
Use a single condition to guard the call to die_if_unmerged_cache for both --soft and --keep. This avoids the small distraction of the precondition check from the logic following it. Also change an instance of if (e) err = err || f(); to the almost as short, but clearer if (e && !err)

Re: What's cooking in git.git (Jan 2013, #06; Mon, 14)

2013-01-14 Thread Chris Rorvick
On Mon, Jan 14, 2013 at 9:02 PM, Junio C Hamano wrote: > I converted one of Chris's follow-up test tweaks to this to > illustrate how it can be done without breaking tests for the > original cvsimport, but didn't do all of them. Chris, is this a > foundation we can work together on top? Sure, lo

[PATCH v2 06/19] reset.c: remove unnecessary variable 'i'

2013-01-14 Thread Martin von Zweigbergk
Throughout most of parse_args(), the variable 'i' remains at 0. Many references are still made to the variable even when it could only have the value 0. This made at least me, who has relatively little experience with C programming styles, think that parts of the function was meant to be part of a

Re: [PATCH 3/3] cvsimport: start adding cvsps 3.x support

2013-01-14 Thread Chris Rorvick
On Sun, Jan 13, 2013 at 7:40 PM, Junio C Hamano wrote: > The new cvsps 3.x series lacks support of some options cvsps 2.x > series had and used by cvsimport-2; add a replacement program from > the author of cvsps 3.x and allow users to choose it by setting the > GIT_CVSPS_VERSION environment varia

Re: [BUG] Possible bug in `remote set-url --add --push`

2013-01-14 Thread Junio C Hamano
Jardel Weyrich writes: > If you allow me, I'd like you to forget about the concepts for a minute, and > focus on the user experience. > Imagine a simple hypothetical scenario in which the user wants to push to 2 > distinct repositories. He already has cloned the repo from the 1st > repository,

Re: [BUG] Possible bug in `remote set-url --add --push`

2013-01-14 Thread Junio C Hamano
Junio C Hamano writes: > Jardel Weyrich writes: > >> If you allow me, I'd like you to forget about the concepts for a minute, and >> focus on the user experience. >> Imagine a simple hypothetical scenario in which the user wants to push to 2 >> distinct repositories. He already has cloned the

Re: [PATCH 3/3] cvsimport: start adding cvsps 3.x support

2013-01-14 Thread Junio C Hamano
Chris Rorvick writes: [jc: please elide parts you are not responding to, leaving enough lines to understand the context] >> +def command(self): >> +"Emit the command implied by all previous options." >> +return self.cvsps + "--fast-export " + self.opts > > "--fast-export" str

Re: [PATCH v2] Make git selectively and conditionally ignore certain stat fields

2013-01-14 Thread Johannes Sixt
Am 1/15/2013 1:11, schrieb Junio C Hamano: > I'd say a simplistic "ignore if zero is stored" or even "ignore this > as one of the systems that shares this file writes crap in it" may > be sufficient, and if this is a jGit specific issue, it might even > make sense to introduce a single configuratio

Re: [PATCH v2] Make git selectively and conditionally ignore certain stat fields

2013-01-14 Thread Robin Rosenberg
- Ursprungligt meddelande - > Robin Rosenberg writes: > > > Semantically they're somewhat different. My flags are for ignoring > > a value when it's not used as indicated by the value zero, while > > trustctime is for ignoring untrustworthy, non-zero, values. > > Yeah, I realized that

Re: git grep performance regression

2013-01-14 Thread Ross Lagerwall
On Tue, Jan 15, 2013 at 11:38:32AM +0700, Duy Nguyen wrote: > dirlen is not expected to include the trailing slash, but > find_basename() does that. It messes up with the path filters for > push/pop in the next code. This brings grep performance closely back > to before for me. Ross, can you check

<    1   2