Git development
 help / color / mirror / Atom feed
* [PATCH] Fix quote_path when called with negative length.
From: Pierre Habouzit @ 2007-12-03  9:06 UTC (permalink / raw)
  To: David Symonds; +Cc: Junio C Hamano, git
In-Reply-To: <ee77f5c20712021539r3075fc57ld6a4cec737e6043d@mail.gmail.com>

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

When the len passed was -1, relative paths shortening was broken, resulting
in too long paths.

Signed-off-by: Pierre Habouzit <madcoder@debian.org>
---

    On Sun, Dec 02, 2007 at 11:39:59PM +0000, David Symonds wrote:
    > On Dec 3, 2007 9:04 AM, Junio C Hamano <gitster@pobox.com> wrote:
    > > Please do not take this as the final decision made by the Emperor, whose
    > > subjects now must follow.  This is a sanity-check to see if everybody is
    > > on the same page.
    > >
    > > I am not the Emperor anyway ;-)
    > >
    > 
    > > Topics not in 'master' yet but should be in v1.5.4
    > > --------------------------------------------------
    > >
    > > I think the following should go in, along with what we already have in
    > > 'master':
    > 
    > Can we add the git-status/git-checkout relative path stuff that's
    > currently been sitting in 'next'? It would be a good step forward for
    > usability.

    Speaking of which, there is this irritating bug in git status that
    let it show too long paths in the first chunk (the "tracked files"
    one).

    The previous version of the function was avoiding very hard to
    compute "in" length, and had quite convoluted code because of that.
    I now compute it at the beginning. The real issue was the:

 		while (prefix[off] && off < len && prefix[off] == in[off])

    line, when len is negative, the shortening never happens. I could
    have fixed it using ((len < 0 && in[off]) || off < len), but I
    disliked the resulting code, so I went for this.

    -- 
    ·O·  Pierre Habouzit
    ··O                                                madcoder@debian.org
    OOO                                                http://www.madism.org

 wt-status.c |   31 +++++++++++++------------------
 1 files changed, 13 insertions(+), 18 deletions(-)

diff --git a/wt-status.c b/wt-status.c
index 0e0439f..eb2cbea 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -84,30 +84,25 @@ static void wt_status_print_trailer(struct wt_status *s)
 static char *quote_path(const char *in, int len,
 		struct strbuf *out, const char *prefix)
 {
-	if (len > 0)
-		strbuf_grow(out, len);
+	int pos = 0;
+
+	if (len < 0)
+		len = strlen(in);
+	strbuf_grow(out, len);
 	strbuf_setlen(out, 0);
 
 	if (prefix) {
 		int off = 0;
 		while (prefix[off] && off < len && prefix[off] == in[off])
-			if (prefix[off] == '/') {
-				prefix += off + 1;
-				in += off + 1;
-				len -= off + 1;
-				off = 0;
-			} else
-				off++;
-
-		for (; *prefix; prefix++)
-			if (*prefix == '/')
+			if (prefix[off++] == '/')
+				pos = off;
+		while (prefix[off])
+			if (prefix[off++] == '/')
 				strbuf_addstr(out, "../");
 	}
 
-	for (; (len < 0 && *in) || len > 0; in++, len--) {
-		int ch = *in;
-
-		switch (ch) {
+	for (; pos < len; pos++) {
+		switch (in[pos]) {
 		case '\n':
 			strbuf_addstr(out, "\\n");
 			break;
@@ -115,8 +110,8 @@ static char *quote_path(const char *in, int len,
 			strbuf_addstr(out, "\\r");
 			break;
 		default:
-			strbuf_addch(out, ch);
-			continue;
+			strbuf_addch(out, in[pos]);
+			break;
 		}
 	}
 
-- 
1.5.3.7.2065.g3d18-dirty


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

^ permalink raw reply related

* Pack several linear commits into one
From: Jacob Kroon @ 2007-12-03  9:06 UTC (permalink / raw)
  To: git

Hi,

Is there a tool for repacking several linear commits into one single ?

Please CC me since I'm not a subscriber to the list.

Best Regards
Jacob Kroon

^ permalink raw reply

* [PATCH] git-commit --allow-empty
From: Junio C Hamano @ 2007-12-03  8:53 UTC (permalink / raw)
  To: Mark Drago; +Cc: stelian, git
In-Reply-To: <7v63zgkw0x.fsf@gitster.siamese.dyndns.org>

It does not usually make sense to record a commit that has the exact
same tree as its sole parent commit and that is why git-commit prevents
you from making such a mistake, but when data from foreign scm is
involved, it is a different story.  We are equipped to represent such an
(perhaps insane, perhaps by mistake, or perhaps done on purpose) empty
change, and it is better to represent it bypassing the safety valve for
native use.

This is primarily for use by foreign scm interface scripts.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * And this is for 'master'.

 Documentation/git-commit.txt |    8 +++++++-
 git-commit.sh                |   13 ++++++++++---
 t/t7501-commit.sh            |    7 +++++++
 3 files changed, 24 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
index d4bfd49..a7ef71f 100644
--- a/Documentation/git-commit.txt
+++ b/Documentation/git-commit.txt
@@ -10,7 +10,7 @@ SYNOPSIS
 [verse]
 'git-commit' [-a | --interactive] [-s] [-v] [-u]
 	   [(-c | -C) <commit> | -F <file> | -m <msg> | --amend]
-	   [--no-verify] [-e] [--author <author>]
+	   [--allow-empty] [--no-verify] [-e] [--author <author>]
 	   [--] [[-i | -o ]<file>...]
 
 DESCRIPTION
@@ -89,6 +89,12 @@ OPTIONS
 	This option bypasses the pre-commit hook.
 	See also link:hooks.html[hooks].
 
+--allow-empty::
+	Usually recording a commit that has the exact same tree as its
+	sole parent commit and the command prevents you from making such
+	a mistake.  This option bypasses the safety, and is primarily
+	for use by foreign scm interface scripts.
+
 -e|--edit::
 	The message taken from file with `-F`, command line with
 	`-m`, and from file with `-C` are usually used as the
diff --git a/git-commit.sh b/git-commit.sh
index cef76a7..2c4a406 100755
--- a/git-commit.sh
+++ b/git-commit.sh
@@ -74,6 +74,7 @@ trap '
 
 all=
 also=
+allow_empty=f
 interactive=
 only=
 logfile=
@@ -114,6 +115,10 @@ do
 	-a|--a|--al|--all)
 		all=t
 		;;
+	--allo|--allow|--allow-|--allow-e|--allow-em|--allow-emp|\
+	--allow-empt|--allow-empty)
+		allow_empty=t
+		;;
 	--au=*|--aut=*|--auth=*|--autho=*|--author=*)
 		force_author="${1#*=}"
 		;;
@@ -515,9 +520,11 @@ else
 	# we need to check if there is anything to commit
 	run_status >/dev/null
 fi
-case "$?,$PARENTS" in
-0,* | *,-p' '?*-p' '?*)
-	# a merge commit can record the same tree as its parent.
+case "$allow_empty,$?,$PARENTS" in
+t,* | ?,0,* | ?,*,-p' '?*-p' '?*)
+	# an explicit --allow-empty, or a merge commit can record the
+	# same tree as its parent.  Otherwise having commitable paths
+	# is required.
 	;;
 *)
 	rm -f "$GIT_DIR/COMMIT_EDITMSG" "$GIT_DIR/SQUASH_MSG"
diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh
index 2e7bcb0..0316ecf 100755
--- a/t/t7501-commit.sh
+++ b/t/t7501-commit.sh
@@ -256,6 +256,13 @@ test_expect_success 'same tree (single parent)' '
 
 '
 
+test_expect_success 'same tree (single parent) --allow-empty' '
+
+	git commit --allow-empty -m "forced empty" &&
+	git cat-file commit HEAD | grep forced
+
+'
+
 test_expect_success 'same tree (merge and amend merge)' '
 
 	git checkout -b side HEAD^ &&
-- 
1.5.3.7-2077-ga07a

^ permalink raw reply related

* [PATCH] git-commit --allow-empty
From: Junio C Hamano @ 2007-12-03  8:44 UTC (permalink / raw)
  To: Mark Drago; +Cc: stelian, git
In-Reply-To: <7vd4tq41zt.fsf@gitster.siamese.dyndns.org>

It does not usually make sense to record a commit that has the exact
same tree as its sole parent commit and that is why git-commit prevents
you from making such a mistake, but when data from foreign scm is
involved, it is a different story.  We are equipped to represent such an
(perhaps insane, perhaps by mistake, or perhaps done on purpose) empty
change, and it is better to represent it bypassing the safety valve for
native use.

This is primarily for use by foreign scm interface scripts.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * This is for 'next', on top of an earlier "allow amending '-s ours' merge".

 Documentation/git-commit.txt |    8 +++++++-
 builtin-commit.c             |    5 +++--
 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
index d4bfd49..a7ef71f 100644
--- a/Documentation/git-commit.txt
+++ b/Documentation/git-commit.txt
@@ -10,7 +10,7 @@ SYNOPSIS
 [verse]
 'git-commit' [-a | --interactive] [-s] [-v] [-u]
 	   [(-c | -C) <commit> | -F <file> | -m <msg> | --amend]
-	   [--no-verify] [-e] [--author <author>]
+	   [--allow-empty] [--no-verify] [-e] [--author <author>]
 	   [--] [[-i | -o ]<file>...]
 
 DESCRIPTION
@@ -89,6 +89,12 @@ OPTIONS
 	This option bypasses the pre-commit hook.
 	See also link:hooks.html[hooks].
 
+--allow-empty::
+	Usually recording a commit that has the exact same tree as its
+	sole parent commit and the command prevents you from making such
+	a mistake.  This option bypasses the safety, and is primarily
+	for use by foreign scm interface scripts.
+
 -e|--edit::
 	The message taken from file with `-F`, command line with
 	`-m`, and from file with `-C` are usually used as the
diff --git a/builtin-commit.c b/builtin-commit.c
index 6c2dc39..e635d99 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -46,7 +46,7 @@ static enum {
 static char *logfile, *force_author, *template_file;
 static char *edit_message, *use_message;
 static int all, edit_flag, also, interactive, only, amend, signoff;
-static int quiet, verbose, untracked_files, no_verify;
+static int quiet, verbose, untracked_files, no_verify, allow_empty;
 
 static int no_edit, initial_commit, in_merge;
 const char *only_include_assumed;
@@ -87,6 +87,7 @@ static struct option builtin_commit_options[] = {
 	OPT_BOOLEAN('n', "no-verify", &no_verify, "bypass pre-commit hook"),
 	OPT_BOOLEAN(0, "amend", &amend, "amend previous commit"),
 	OPT_BOOLEAN(0, "untracked-files", &untracked_files, "show all untracked files"),
+	OPT_BOOLEAN(0, "allow-empty", &allow_empty, "ok to record an empty change"),
 
 	OPT_END()
 };
@@ -710,7 +711,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 	}
 
 	if (!prepare_log_message(index_file, prefix) && !in_merge &&
-	    !(amend && is_a_merge(head_sha1))) {
+	    !allow_empty && !(amend && is_a_merge(head_sha1))) {
 		run_status(stdout, index_file, prefix);
 		rollback_index_files();
 		unlink(commit_editmsg);
-- 
1.5.3.7-2077-ga07a

^ permalink raw reply related

* Re: [PATCH] git-commit: Allow to amend a merge commit that does not change the tree
From: Junio C Hamano @ 2007-12-03  8:26 UTC (permalink / raw)
  To: Johannes Sixt; +Cc: krh, Git Mailing List
In-Reply-To: <7vtzn0md0h.fsf@gitster.siamese.dyndns.org>

Will roll in this as a new test.

--

 t/t7501-commit.sh |   32 ++++++++++++++++++++++++++++++++
 1 files changed, 32 insertions(+), 0 deletions(-)

diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh
index 31a6f63..2e7bcb0 100755
--- a/t/t7501-commit.sh
+++ b/t/t7501-commit.sh
@@ -244,4 +244,36 @@ test_expect_success 'multiple -m' '
 
 '
 
+test_expect_success 'same tree (single parent)' '
+
+	if git commit -m empty
+	then
+		echo oops -- should have complained
+		false
+	else
+		: happy
+	fi
+
+'
+
+test_expect_success 'same tree (merge and amend merge)' '
+
+	git checkout -b side HEAD^ &&
+	echo zero >zero &&
+	git add zero &&
+	git commit -m "add zero" &&
+	git checkout master &&
+
+	git merge -s ours side -m "empty ok" &&
+	git diff HEAD^ HEAD >actual &&
+	: >expected &&
+	diff -u expected actual &&
+
+	git commit --amend -m "empty really ok" &&
+	git diff HEAD^ HEAD >actual &&
+	: >expected &&
+	diff -u expected actual
+
+'
+
 test_done
-- 
1.5.3.7-2077-ga07a

^ permalink raw reply related

* Re: [PATCH] Highlight keyboard shortcuts in git-add--interactive
From: Wincent Colaiuta @ 2007-12-03  8:09 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, dzwell, peff, Matthieu.Moy
In-Reply-To: <7veje4ykzg.fsf@gitster.siamese.dyndns.org>

El 2/12/2007, a las 20:06, Junio C Hamano escribió:

> Wincent Colaiuta <win@wincent.com> writes:
>
>> Unless by "documentation" you meant to somehow expose these in the
>> interface at runtime... something like this? (applied on top of the
>> patch I just sent to the list):
>
> I did not recall (and was too lazy to check) if they were documented
> already, but as you suggest, I think letting people type ? at the  
> prompt
> to get a help is always a good idea.  So, instead of doing this part:
>
>> @@ -308,7 +309,7 @@ sub list_and_choose {
>>  			print "> ";
>>  		}
>>  		else {
>> -			print ">> ";
>> +			print " (?)>> ";
>
> I'd prefer accepting '?'  as a valid "help me" input and showing
> appropriate help for _both_ singleton select and multiple select,
> without mentioning " (?)".  For this, your prompt_help_cmd needs to be
> enhanced to limit the help to singleton case, though.


That's actually the way I did it the first time, but then decided that  
the singleton prompt help had so little to say that I doubted about  
including it. Something like this, once again on top of the patch I  
posted yesterday ("Fixes for automatic prefix highlighting"):

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 32fb9ea..335c2c6 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -237,7 +237,8 @@ sub is_valid_prefix {
  	    !($prefix =~ /[\s,]/) && # separators
  	    !($prefix =~ /^-/) &&    # deselection
  	    !($prefix =~ /^\d+/) &&  # selection
-	    ($prefix ne '*');        # "all" wildcard
+	    ($prefix ne '*') &&      # "all" wildcard
+	    ($prefix ne '?');        # prompt help
  }

  # given a prefix/remainder tuple return a string with the prefix  
highlighted
@@ -318,6 +319,12 @@ sub list_and_choose {
  		}
  		chomp $line;
  		last if $line eq '';
+		if ($line eq '?') {
+			$opts->{SINGLETON} ?
+			    singleton_prompt_help_cmd() :
+			    prompt_help_cmd();
+			next TOPLOOP;
+		}
  		for my $choice (split(/[\s,]+/, $line)) {
  			my $choose = 1;
  			my ($bottom, $top);
@@ -363,6 +370,28 @@ sub list_and_choose {
  	return @return;
  }

+sub singleton_prompt_help_cmd {
+	print <<\EOF ;
+Prompt help:
+1          - select a numbered item
+foo        - select item based on unique prefix
+           - (empty) select nothing
+EOF
+}
+
+sub prompt_help_cmd {
+	print <<\EOF ;
+Prompt help:
+1          - select a single item
+3-5        - select a range of items
+2-3,6-9    - select multiple ranges
+foo        - select item based on unique prefix
+-...       - unselect specified items
+*          - choose all items
+           - (empty) finish selecting
+EOF
+}
+
  sub status_cmd {
  	list_and_choose({ LIST_ONLY => 1, HEADER => $status_head },
  			list_modified());



Cheers,
Wincent

^ permalink raw reply related

* Re: [PATCH] git-commit: Allow to amend a merge commit that does not change the tree
From: Junio C Hamano @ 2007-12-03  7:51 UTC (permalink / raw)
  To: Johannes Sixt; +Cc: krh, Git Mailing List
In-Reply-To: <1196666690-22187-1-git-send-email-johannes.sixt@telecom.at>

Johannes Sixt <johannes.sixt@telecom.at> writes:

> 	I haven't gotten around to write a test case for this scenario,
> 	so I'm sending out the fix alone, in order to draw attention
> 	to the issue and have builtin-commit fixed by its authors, if
> 	necessary ;)

Untested but something like this ought to do.

 builtin-commit.c |   11 ++++++++++-
 1 files changed, 10 insertions(+), 1 deletions(-)

diff --git a/builtin-commit.c b/builtin-commit.c
index f37a90f..6c2dc39 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -676,6 +676,14 @@ int git_commit_config(const char *k, const char *v)
 	return git_status_config(k, v);
 }
 
+static int is_a_merge(const unsigned char *sha1)
+{
+	struct commit *commit = lookup_commit(sha1);
+	if (!commit || parse_commit(commit))
+		die("could not parse HEAD commit");
+	return !!(commit->parents && commit->parents->next);
+}
+
 static const char commit_utf8_warn[] =
 "Warning: commit message does not conform to UTF-8.\n"
 "You may want to amend it after fixing the message, or set the config\n"
@@ -701,7 +709,8 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 		return 1;
 	}
 
-	if (!prepare_log_message(index_file, prefix) && !in_merge) {
+	if (!prepare_log_message(index_file, prefix) && !in_merge &&
+	    !(amend && is_a_merge(head_sha1))) {
 		run_status(stdout, index_file, prefix);
 		rollback_index_files();
 		unlink(commit_editmsg);

^ permalink raw reply related

* Re: [PATCH 6/6] builtin-commit: Add newline when showing which commit was created
From: Jeff King @ 2007-12-03  7:53 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Johannes Schindelin, git, krh
In-Reply-To: <7vzlwsmdvb.fsf@gitster.siamese.dyndns.org>

On Sun, Dec 02, 2007 at 11:33:28PM -0800, Junio C Hamano wrote:

> After reverting this, recording a merge commit seems to have lost the
> newline.  Can be easily reproduced with:
> 
> 	$ git merge --no-commit some-branch
>   $ git commit -a -m 'foo'

See, I knew reverting would flush it out. ;)

Unfortunately, the fix isn't terribly obvious. log_tree_commit produces
output either from log_tree_diff, which appends a newline, or from
show_log, which doesn't, and returns the same value in either case.

I tried adding a '%n' to the format specifier (which is "%h: %s"),
but that has inconsistent results, since another newline is always
placed between the diffstat and the log, anyway. The design of
--pretty=format is a bit inflexible here, since you are stuck with the
"$log\n$diff" format. But changing it would impact users, and is likely
to require a lot of surgery in the log machinery.

So short of munging log_tree_commit to add the newline, I'm not sure how
to fix it, and I'm a little wary of messing with that function.

-Peff

^ permalink raw reply

* Re: make test failure with latest master
From: Jeff King @ 2007-12-03  7:35 UTC (permalink / raw)
  To: Johannes Sixt; +Cc: Johannes Schindelin, A Large Angry SCM, git, gitster
In-Reply-To: <4753B147.2050404@viscovery.net>

On Mon, Dec 03, 2007 at 08:33:27AM +0100, Johannes Sixt wrote:

>> -	test_expect_success 'skipping cvsimport tests, cvs/cvsps not found' ''
>> +	test_expect_success 'skipping cvsimport tests, cvs not found' ''
>
> FWIW, the idiom to provide informative messages in test cases is
>
> 	say 'skipping cvsimport tests, cvs not found'
>
> This gives a distinguishing color, too.

Ah, I adapted the test from t9400, which uses expect_success. Thanks for
the tip.

-Peff

^ permalink raw reply

* Re: [PATCH 6/6] builtin-commit: Add newline when showing which commit was created
From: Junio C Hamano @ 2007-12-03  7:33 UTC (permalink / raw)
  To: Jeff King; +Cc: Johannes Schindelin, git, krh, gitster
In-Reply-To: <20071201222106.GA27102@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

> On Sun, Nov 11, 2007 at 05:36:52PM +0000, Johannes Schindelin wrote:
>
>> The function log_tree_commit() does not break the line, so we have to
>> do it ourselves.
>
> Johannes,
>
> Can you explain the rationale for this change in more detail?
>
> When I run builtin-commit from the tip of next, I always get an extra
> newline (as compared to the shell behavior):

After reverting this, recording a merge commit seems to have lost the
newline.  Can be easily reproduced with:

	$ git merge --no-commit some-branch
        $ git commit -a -m 'foo'

^ permalink raw reply

* Re: make test failure with latest master
From: Johannes Sixt @ 2007-12-03  7:33 UTC (permalink / raw)
  To: Jeff King; +Cc: Johannes Schindelin, A Large Angry SCM, git, gitster
In-Reply-To: <20071203015954.GB8322@coredump.intra.peff.net>

Jeff King schrieb:
> -if ! ( type cvs && type cvsps ) >/dev/null 2>&1
> +if ! type cvs >/dev/null 2>&1
>  then
> -	test_expect_success 'skipping cvsimport tests, cvs/cvsps not found' ''
> +	test_expect_success 'skipping cvsimport tests, cvs not found' ''

FWIW, the idiom to provide informative messages in test cases is

	say 'skipping cvsimport tests, cvs not found'

This gives a distinguishing color, too.

-- Hannes

^ permalink raw reply

* [PATCH] git-commit: Allow to amend a merge commit that does not change the tree
From: Johannes Sixt @ 2007-12-03  7:24 UTC (permalink / raw)
  To: krh; +Cc: Junio C Hamano, Git Mailing List, Johannes Sixt

Normally, it should not be allowed to generate an empty commit. A merge
commit generated with git 'merge -s ours' does not change the tree (along
the first parent), but merges are not "empty" even if they do not change
the tree. Hence, commit 8588452ceb7 allowed to amend a merge commit that
does not change the tree, but 4fb5fd5d301 disallowed it again in an
attempt to avoid that an existing commit is amended such that it becomes
empty. With this change, a commit can be edited (create a new one or amend
an existing one) either if there are changes or if there are at least two
parents.

Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
---
	I need this patch because I sometimes do 'git merge -s ours' and
	then want to change the commit message.

	I haven't gotten around to write a test case for this scenario,
	so I'm sending out the fix alone, in order to draw attention
	to the issue and have builtin-commit fixed by its authors, if
	necessary ;)

	Thanks,
	-- Hannes

 git-commit.sh |    9 ++++++---
 1 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/git-commit.sh b/git-commit.sh
index 4853397..1a07278 100755
--- a/git-commit.sh
+++ b/git-commit.sh
@@ -515,13 +515,16 @@ else
 	# we need to check if there is anything to commit
 	run_status >/dev/null
 fi
-if [ "$?" != "0" -a ! -f "$GIT_DIR/MERGE_HEAD" ]
-then
+case "$?,$PARENTS" in
+0,* | *,-p*-p*)
+	:	# ok, go ahead
+	;;
+*)
 	rm -f "$GIT_DIR/COMMIT_EDITMSG" "$GIT_DIR/SQUASH_MSG"
 	use_status_color=t
 	run_status
 	exit 1
-fi
+esac
 
 case "$no_edit" in
 '')
-- 
1.5.3.6.969.g3cdf46

^ permalink raw reply related

* Re: Incorrect git-blame result if I use full path to file
From: Robin Rosenberg @ 2007-12-03  6:55 UTC (permalink / raw)
  To: Jeff King; +Cc: Junio C Hamano, Anatol Pomozov, git
In-Reply-To: <20071203024916.GA11003@coredump.intra.peff.net>

måndag 03 december 2007 skrev Jeff King:
> On Sun, Dec 02, 2007 at 06:40:36PM -0800, Junio C Hamano wrote:
> 
> > > Even more useful would be to convert
> > > /path/to/repo/file to 'file' internally.
> > 
> > ... that might help "cut & paste from file manager" people, and I think
> > we had comment session for such a patch recently on the list.
> > 
> > Sorry, but I lost track of that the current status of that patch.  Did
> > it die?
> 
> I didn't pay attention to it originally, but I assume you mean the
> recent patch from Robin Rosenberg (cc'd). Looking it over, I see one
> obvious omission: there is no canonicalization of the paths. IOW, I
> think it will break in the presence of symlinks (if I specify
> /path/to/repo/file, /path/to is a symlink to /other/path, I think the
> worktree will end up as /other/path/repo, and fail a string comparison
> with /path/to/repo).

No it didn't die, it's just not worked on too often. I notes, among, other things
that it's test cases were not correct, besides needing more tests.

Symlinks were not covered.

-- robin

^ permalink raw reply

* Re: git-svn: .git/svn disk usage
From: Kelvie Wong @ 2007-12-03  6:53 UTC (permalink / raw)
  To: Pascal Obry, Ollie Wild, git
In-Reply-To: <20071203064603.GA18583@old.davidb.org>

I'm going to have to say this is due to the unhandled.log as well.

Just gzip -9 it (AFAIK it's not used for anything, but keep it just in case).

On Dec 2, 2007 10:46 PM, David Brown <git@davidb.org> wrote:
> On Mon, Dec 03, 2007 at 07:37:51AM +0100, Pascal Obry wrote:
> >Ollie,
> >
> >> I'm curious if other developers have run into this issue.  If so, are
> >> there any proposals / plans for improving the storage of git-svn
> >> metadata?
> >
> >Did you run "git gc" after importing code form the subversion
> >repository? On my side I found that it has reduced drastically the size
> >of the local Git repository.
>
> I think the original poster is probably finding the space in the .git/svn
> directory.  'git-svn' keeps an index file for every branch in SVN.
>
> I suspect it does this for speed, at least on a large import, since the SVN
> commits will come across numerically, affecting the branches out of order.
>
> However, the index could fairly easily be extracted from git (since that is
> what it normally does).  In this case, where all of the indexes take
> significant space if this is worth it.
>
> Ollie, if you look in these svn branch directories, is most of the space
> taken up with files called 'index'?
>
> Browsing through the few svn clones that I have, the space seems to be
> roughly split between 'index' files and 'unhandled.log' files.
>
> Dave
>
> -
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>



-- 
Kelvie Wong

^ permalink raw reply

* Re: git-svn: .git/svn disk usage
From: David Brown @ 2007-12-03  6:46 UTC (permalink / raw)
  To: Pascal Obry; +Cc: Ollie Wild, git
In-Reply-To: <4753A43F.9060303@obry.net>

On Mon, Dec 03, 2007 at 07:37:51AM +0100, Pascal Obry wrote:
>Ollie,
>
>> I'm curious if other developers have run into this issue.  If so, are
>> there any proposals / plans for improving the storage of git-svn
>> metadata?
>
>Did you run "git gc" after importing code form the subversion
>repository? On my side I found that it has reduced drastically the size
>of the local Git repository.

I think the original poster is probably finding the space in the .git/svn
directory.  'git-svn' keeps an index file for every branch in SVN.

I suspect it does this for speed, at least on a large import, since the SVN
commits will come across numerically, affecting the branches out of order.

However, the index could fairly easily be extracted from git (since that is
what it normally does).  In this case, where all of the indexes take
significant space if this is worth it.

Ollie, if you look in these svn branch directories, is most of the space
taken up with files called 'index'?

Browsing through the few svn clones that I have, the space seems to be
roughly split between 'index' files and 'unhandled.log' files.

Dave

^ permalink raw reply

* Re: git-svn: .git/svn disk usage
From: Pascal Obry @ 2007-12-03  6:37 UTC (permalink / raw)
  To: Ollie Wild; +Cc: git
In-Reply-To: <65dd6fd50712022217l5f807f31pf3f00d82c3dccf5c@mail.gmail.com>

Ollie,

> I'm curious if other developers have run into this issue.  If so, are
> there any proposals / plans for improving the storage of git-svn
> metadata?

Did you run "git gc" after importing code form the subversion
repository? On my side I found that it has reduced drastically the size
of the local Git repository.

Pascal.

-- 

--|------------------------------------------------------
--| Pascal Obry                           Team-Ada Member
--| 45, rue Gabriel Peri - 78114 Magny Les Hameaux FRANCE
--|------------------------------------------------------
--|              http://www.obry.net
--| "The best way to travel is by means of imagination"
--|
--| gpg --keyserver wwwkeys.pgp.net --recv-key C1082595

^ permalink raw reply

* Re: [PATCH 1/2] Allow callers of unpack_trees() to handle failure
From: Daniel Barkalow @ 2007-12-03  6:31 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git
In-Reply-To: <7v63zgqorw.fsf@gitster.siamese.dyndns.org>

On Sun, 2 Dec 2007, Junio C Hamano wrote:

> Daniel Barkalow <barkalow@iabervon.org> writes:
> 
> > Return an error from unpack_trees() instead of calling die(), and exit
> > with an error in read-tree. The merge function can return negative to
> > abort.
> >
> > This will be used in builtin-checkout -m.
> >
> > Signed-off-by: Daniel Barkalow <barkalow@iabervon.org>
> > ---
> >  builtin-read-tree.c |    3 +-
> >  unpack-trees.c      |   85 ++++++++++++++++++++++++++++----------------------
> >  2 files changed, 50 insertions(+), 38 deletions(-)
> >
> > diff --git a/builtin-read-tree.c b/builtin-read-tree.c
> > index 43cd56a..4f680c3 100644
> > --- a/builtin-read-tree.c
> > +++ b/builtin-read-tree.c
> > @@ -269,7 +269,8 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)
> >  		parse_tree(tree);
> >  		init_tree_desc(t+i, tree->buffer, tree->size);
> >  	}
> > -	unpack_trees(nr_trees, t, &opts);
> > +	if (unpack_trees(nr_trees, t, &opts))
> > +		return 128;
> 
> Magic 128 when everybody else in the patch returns -1 for error?

This is a cmd_ function, so this is the exit code. 128 is what die() uses. 
Should this be explicitly an exit(128)?

> Otherwise the changes seem sensible, as long as the callers are paying
> attention to the return values, which I admit that I did not check.

The only callers are here and merge-recursive, and merge-recursive was 
already checking the return value (and never getting anything); 
merge-recursive die()s if it gets non-zero.

	-Daniel
*This .sig left intentionally blank*

^ permalink raw reply

* Re: [PATCH 1/2] Allow callers of unpack_trees() to handle failure
From: Junio C Hamano @ 2007-12-03  6:24 UTC (permalink / raw)
  To: Daniel Barkalow; +Cc: git
In-Reply-To: <Pine.LNX.4.64.0712030032400.5349@iabervon.org>

Daniel Barkalow <barkalow@iabervon.org> writes:

> Return an error from unpack_trees() instead of calling die(), and exit
> with an error in read-tree. The merge function can return negative to
> abort.
>
> This will be used in builtin-checkout -m.
>
> Signed-off-by: Daniel Barkalow <barkalow@iabervon.org>
> ---
>  builtin-read-tree.c |    3 +-
>  unpack-trees.c      |   85 ++++++++++++++++++++++++++++----------------------
>  2 files changed, 50 insertions(+), 38 deletions(-)
>
> diff --git a/builtin-read-tree.c b/builtin-read-tree.c
> index 43cd56a..4f680c3 100644
> --- a/builtin-read-tree.c
> +++ b/builtin-read-tree.c
> @@ -269,7 +269,8 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)
>  		parse_tree(tree);
>  		init_tree_desc(t+i, tree->buffer, tree->size);
>  	}
> -	unpack_trees(nr_trees, t, &opts);
> +	if (unpack_trees(nr_trees, t, &opts))
> +		return 128;

Magic 128 when everybody else in the patch returns -1 for error?

Otherwise the changes seem sensible, as long as the callers are paying
attention to the return values, which I admit that I did not check.

^ permalink raw reply

* git-svn: .git/svn disk usage
From: Ollie Wild @ 2007-12-03  6:17 UTC (permalink / raw)
  To: git

Hi,

I've been using git-svn to mirror the gcc repository at
svn://gcc.gnu.org/svn/gcc.  Recently, I noticed that my .git directory
is consuming 11GB of disk space.  Digging further, I discovered that
9.8GB of this is attributable to the .git/svn directory (which
includes 200 branches and 2,588 tags).  Given that my .git/objects
directory is 652MB, it seems that it ought to be possible to store
this information in a more compact form.

I'm curious if other developers have run into this issue.  If so, are
there any proposals / plans for improving the storage of git-svn
metadata?

Thanks,
Ollie

^ permalink raw reply

* Re: [PATCH] git-stash: Display help message if git-stash is run with wrong sub-commands
From: Wayne Davison @ 2007-12-03  6:16 UTC (permalink / raw)
  To: Kevin Leung; +Cc: Git ML
In-Reply-To: <e66701d40712021834h64bf8d0y14f0e222d0f9a617@mail.gmail.com>

On Mon, Dec 03, 2007 at 10:34:05AM +0800, Kevin Leung wrote:
> +USAGE='[  | save | list | show | apply | clear ]'

It seems strange to me that git stash is using sub-sub-command words
instead of options.  Would it make more sense to be more like git branch
and have a list be indicated by -l, etc.?

..wayne..

^ permalink raw reply

* Re: [PATCH] quote_path: fix collapsing of relative paths
From: Jeff King @ 2007-12-03  6:05 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Johannes Schindelin, git
In-Reply-To: <7vabosqq1l.fsf@gitster.siamese.dyndns.org>

On Sun, Dec 02, 2007 at 09:56:54PM -0800, Junio C Hamano wrote:

> Somehow I feel it would be simpler and less error prone if you atually
> count to set len to the length if you got negative.
> [...]
> So perhaps this is easier to read?

Yes, I actually wrote the exact same patch but discarded it in favor of
what I sent, because I wanted a minimal change (and I assumed it had
been implemented that way to avoid the extra strlen call). But that is
just premature micro-optimization, and I think the version you posted is
much less subtle.

> The part that follows the patch, there is a line that subtracts a small
> number (off+1) from len --- while it won't have a wraparound issue, it
> feels a bit ugly to rely on the "magic -1" value to stay "magic
> negative" if small positive integers are subtracted from it.

I hadn't considered that, but another good reason to just set len in the
first place.

> Also the reason the updated condition to the while loop does not have to
> check NUL termination upon negative len is because both (prefix[off] !=
> NUL) and (prefix[off] == in[off]) are checked there, which some may find
> subtle.

Yes, I found it subtle, too, which is why I documented it in the commit
message. :)

> -	if (len > 0)
> -		strbuf_grow(out, len);
> -	strbuf_setlen(out, 0);
> +	if (len < 0)
> +		len = strlen(in);
>  
> +	strbuf_grow(out, len);
> +	strbuf_setlen(out, 0);

I wonder if this 'grow' is really all that useful, since we are now
going to overallocate in some cases. Perhaps we should just let strbuf
do its job and grow as necessary. But it likely doesn't matter either
way.

-Peff

^ permalink raw reply

* Re: [PATCH] Adding menu for Emacs git.el
From: Remi Vanicat @ 2007-12-03  6:03 UTC (permalink / raw)
  To: Alexandre Julliard; +Cc: git
In-Reply-To: <87fxylos4o.fsf@wine.dyndns.org>


Adding three menus to the git-status-mode of git.el : One for marking
and unmarking, one for what you do when you have a conflict, and the
other one for all the rest.

Signed-off-by: Rémi Vanicat <vanicat@debian.org>
---

Alexandre Julliard <julliard@winehq.org> writes:

> "=?utf-8?q?R=C3=A9mi=20Vanicat?=" <vanicat@debian.org>, Remi Vanicat
> <vanicat@debian.org> writes:
>
>> Adding three menu to the git-status-mode of git.el : One for marking
>> and unmarking, one for every thing you need when you have a conflict,
>> and a last one for all the rest.
>>
>> Signed-off-by: Rémi Vanicat <vanicat@debian.org>
>
> It looks good to me. A couple of minor details:

Here is the corrected patch
[...]

> BTW do you have a copyright assignment for Emacs?
No, should I seek one ?

 contrib/emacs/git.el |   48 ++++++++++++++++++++++++++++++++++++++++++++++--
 1 files changed, 46 insertions(+), 2 deletions(-)

diff --git a/contrib/emacs/git.el b/contrib/emacs/git.el
index e147da0..1db7698 100644
--- a/contrib/emacs/git.el
+++ b/contrib/emacs/git.el
@@ -49,7 +49,7 @@
 (eval-when-compile (require 'cl))
 (require 'ewoc)
 (require 'log-edit)
-
+(require 'easymenu)
 
 ;;;; Customizations
 ;;;; ------------------------------------------------------------
@@ -1297,7 +1297,51 @@ Return the list of files that haven't been handled."
     (define-key toggle-map "i" 'git-toggle-show-ignored)
     (define-key toggle-map "k" 'git-toggle-show-unknown)
     (define-key toggle-map "m" 'git-toggle-all-marks)
-    (setq git-status-mode-map map)))
+    (setq git-status-mode-map map))
+  (easy-menu-define git-menu-mark git-status-mode-map
+    "Git Merge Menu"
+    `("Merge"
+      ["Next Unmerged File" git-next-unmerged-file t]
+      ["Prev Unmerged File" git-prev-unmerged-file t]
+      ["Mark as Resolved" git-resolve-file t]
+      ["Interactive Merge File" git-find-file-imerge t]
+      ["Diff Against Common Base File" git-diff-file-base t]
+      ["Diff Combined" git-diff-file-combined t]
+      ["Diff Against Merge Head" git-diff-file-merge-head t]
+      ["Diff Against Mine" git-diff-file-mine t]
+      ["Diff Against Other" git-diff-file-other t]))
+  (easy-menu-define git-menu-mark git-status-mode-map
+    "Git Mark Menu"
+    `("Mark"
+      ["Mark File" git-mark-file t]
+      ["Mark All" git-mark-all t]
+      ["Unmark File" git-unmark-file t]
+      ["Unmark All" git-unmark-all t]
+      ["Toggle All Mark" git-toggle-all-marks t]))
+  (easy-menu-define git-menu git-status-mode-map
+    "Git Menu." 
+    `("Git"
+      ["Refresh" git-refresh-status t]
+      ["Commit" git-commit-file t]
+      "--------"
+      ["Add File" git-add-file t]
+      ["Revert File" git-revert-file t]
+      ["Ignore File" git-ignore-file t]
+      ["Remove File" git-remove-file t]
+      "--------"
+      ["Find File" git-find-file t]
+      ["View File" git-view-file t]
+      ["Diff File" git-diff-file t]
+      ["Interctive Diff File" git-diff-file-idiff t]
+      ["Log" git-log-file t]
+      "--------"
+      ["Quit" git-status-quit t]
+      "--------"
+      ["Show Uptodate" git-toggle-show-uptodate :style toggle :selected git-show-uptodate]
+      ["Show Ignored" git-toggle-show-ignored :style toggle :selected git-show-ignored]
+      ["Show Unknown" git-toggle-show-unknown :style toggle :selected git-show-unknown]))
+    
+)
 
 ;; git mode should only run in the *git status* buffer
 (put 'git-status-mode 'mode-class 'special)
-- 
1.5.3.6

^ permalink raw reply related

* Re: [PATCH] quote_path: fix collapsing of relative paths
From: Junio C Hamano @ 2007-12-03  5:56 UTC (permalink / raw)
  To: Jeff King; +Cc: Johannes Schindelin, git
In-Reply-To: <20071203053001.GA12696@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

> The code tries to collapse identical leading components
> between the prefix and the path. So if we're in "dir1", the
> path "dir1/file" should become just "file". However, we were
> ending up with "../dir1/file". The included test expected
> the wrong output.
>
> Because the "len" parameter to quote_path can be passed in
> as -1 to indicate a NUL-terminated string, we have to
> consider that possibility in our loop conditional (but no
> additional checks are necessary, since we already check that
> prefix[off] and in[off] are identical, and that prefix[off]
> is not NUL.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> This behavior in git-status had been bugging me, and when I went to fix
> it, I was surprised to find code already there to do it. :) Dscho,
> please confirm that the test is in fact in error, and that I've read the
> intent of your code correctly.
>
>  t/t7502-status.sh |    2 +-
>  wt-status.c       |    3 ++-
>  2 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/t/t7502-status.sh b/t/t7502-status.sh
> index 269b334..d6ae69d 100755
> --- a/t/t7502-status.sh
> +++ b/t/t7502-status.sh
> @@ -68,7 +68,7 @@ cat > expect << \EOF
>  # Changed but not updated:
>  #   (use "git add <file>..." to update what will be committed)
>  #
> -#	modified:   ../dir1/modified
> +#	modified:   modified
>  #
>  # Untracked files:
>  #   (use "git add <file>..." to include in what will be committed)
> diff --git a/wt-status.c b/wt-status.c
> index e77120d..09666ec 100644
> --- a/wt-status.c
> +++ b/wt-status.c
> @@ -90,7 +90,8 @@ static char *quote_path(const char *in, int len,
>  
>  	if (prefix) {
>  		int off = 0;
> -		while (prefix[off] && off < len && prefix[off] == in[off])
> +		while (prefix[off] && (len < 0 || off < len)
> +				&& prefix[off] == in[off])
>  			if (prefix[off] == '/') {
>  				prefix += off + 1;
>  				in += off + 1;

Somehow I feel it would be simpler and less error prone if you atually
count to set len to the length if you got negative.

The part that follows the patch, there is a line that subtracts a small
number (off+1) from len --- while it won't have a wraparound issue, it
feels a bit ugly to rely on the "magic -1" value to stay "magic
negative" if small positive integers are subtracted from it.

Also the reason the updated condition to the while loop does not have to
check NUL termination upon negative len is because both (prefix[off] !=
NUL) and (prefix[off] == in[off]) are checked there, which some may find
subtle.

So perhaps this is easier to read?

---
 wt-status.c |   11 ++++++-----
 1 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/wt-status.c b/wt-status.c
index 0e0439f..52ab41c 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -82,12 +82,13 @@ static void wt_status_print_trailer(struct wt_status *s)
 }
 
 static char *quote_path(const char *in, int len,
-		struct strbuf *out, const char *prefix)
+			struct strbuf *out, const char *prefix)
 {
-	if (len > 0)
-		strbuf_grow(out, len);
-	strbuf_setlen(out, 0);
+	if (len < 0)
+		len = strlen(in);
 
+	strbuf_grow(out, len);
+	strbuf_setlen(out, 0);
 	if (prefix) {
 		int off = 0;
 		while (prefix[off] && off < len && prefix[off] == in[off])
@@ -104,7 +105,7 @@ static char *quote_path(const char *in, int len,
 				strbuf_addstr(out, "../");
 	}
 
-	for (; (len < 0 && *in) || len > 0; in++, len--) {
+	for ( ; len > 0; in++, len--) {
 		int ch = *in;
 
 		switch (ch) {

^ permalink raw reply related

* Re: [PATCH 1/3] git-help: add -i|--info option to display info page.
From: Christian Couder @ 2007-12-03  5:53 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Pascal Obry, git, Theodore Tso, Jakub Narebski, Alex Riesen,
	Andreas Ericsson, Matthieu Moy, Eric Wong
In-Reply-To: <7vodd9zbvt.fsf@gitster.siamese.dyndns.org>

Le dimanche 2 décembre 2007, Junio C Hamano a écrit :
> Pascal Obry <pascal@obry.net> writes:
> >
> > If would be nice if this could be more generic. For example I'd like to
> > use Emacs woman mode instead of info. Can't we have something like
> >
> >    $ git help --ext XXX
> >
> > "ext" standing for external and calling whatever command recorded into
> > .gitconfig for example ?
>
> There is a bit of conflict here.  We could do that and make the
> implementation of "ext" command responsible to transform "commit" in
>
> 	$ git help --ext commit
>
> to the location of manual page (or formatted HTML page, or node in the
> info documentation).  git itself does not need to know much about where
> the help material is in such an implementation.
>
> But Christian's series is about making such "ext" thing easier to write.
> No matter what kind of web browser is used, it needs to be told where
> the preformatted HTML page for git-commit command is (and it does not
> care where git-commit.1 manpage is found or what the node is called in
> git.info document).  It makes it a bit too limiting by defining -w (web)
> and -i (info) upfront without offering -x (ext), but we need to start
> somewhere.

Yeah, I think that the user should be able to choose both the format and the 
tool used for help pages. And that we should start to make more popular 
formats and tools work well first. That means HTML with web browser first.
(And yeah, my first patch is about "info", but it was a very low hanging 
fruit.)

In the end we may want to support many other tools and format. For example:

"git help --format=man --tool=konqueror log" or
"git help -m --tool=konqueror log"

would launch something like: "konqueror 'man:git-log(1)'"

But of course, to be able to do that, we have to know how each tool is 
working, because the syntax has a good chance to be different in each case.

If we provide a -x|--ext upfront and we don't check if we know about the 
tool the user wants to use, then we will not get information about how to 
properly use the tool and we may break without any meaningfull error for 
many of them.

So if someone has information about how "woman" or other web or man or info 
browser can be used, I will be glad to collect it and eventually use it to 
try to make "git help" work for the tool (though I don't promise to test 
them on platform other than Linux), but I will focus on web formats and 
tools first. Patches are welcome too. 

Thanks in advance,
Christian.

^ permalink raw reply

* Re: [PATCH] Make git status usage say git status instead of git commit
From: Junio C Hamano @ 2007-12-03  5:34 UTC (permalink / raw)
  To: Shawn Bohrer; +Cc: git, gitster
In-Reply-To: <1196658129-16708-1-git-send-email-shawn.bohrer@gmail.com>

Shawn Bohrer <shawn.bohrer@gmail.com> writes:

> git status shares the same usage information as git commit since it
> shows what would be committed if the same options are given.  However,
> when displaying the usage information for git status it should say it
> is for git status not git commit.
>
> Signed-off-by: Shawn Bohrer <shawn.bohrer@gmail.com>

Thanks.  Will apply.

> As a side question, should the usage information also use the non dash
> notation of the command since it is deprecated?  I noticed all of the
> other commands are presently using the dash form, so I left it as is for
> now.

Wise choice.  We would probably want to clean them up at the same time
early post 1.5.4.

^ 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