Git development
 help / color / mirror / Atom feed
* [PATCHv2 0/2] Add a "fixup" command to "rebase --interactive"
From: Michael Haggerty @ 2009-12-07  4:22 UTC (permalink / raw)
  To: git; +Cc: gitster, git, Johannes.Schindelin, bgustavsson, Michael Haggerty

Thanks, everybody, for all of the feedback.  This is the redone patch
series, which I think addresses all of your suggestions.

I would still like to implement "don't open the editor if there are
only fixups", but if it's OK I'll work on that in a separate patch
series when I have time.

Michael J Gruber wrote:
> My first reaction to the subject was "Huh? What repository?". So I
> suggest something like
> 
> t3404: Better document the original repository layout
> 
> as a more descriptive subject.

Good idea.  Done.

Johannes Schindelin wrote:
> Alternatively, one could use the test_commit function, I guess. [...]

Yes, that's much easier.  Changed.  This makes the old first and
second patches reduce to a single one.

Johannes Schindelin wrote:
> On Fri, 4 Dec 2009, Junio C Hamano wrote:
>> Michael Haggerty <mhagger@alum.mit.edu> writes:
>>
>>> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
>>> index 0bd3bf7..539413d 100755
>>> --- a/git-rebase--interactive.sh
>>> +++ b/git-rebase--interactive.sh
>>> @@ -302,7 +302,7 @@ nth_string () {
>>>  
>>>  make_squash_message () {
>>>  	if test -f "$SQUASH_MSG"; then
>>> -		COUNT=$(($(sed -n "s/^# This is [^0-9]*\([1-9][0-9]*\).*/\1/p" \
>>> +		COUNT=$(($(sed -n "s/^# Th[^0-9]*\([1-9][0-9]*\)\(th\|st\|nd\|rd\) commit message.*:/\1/p" \
>>>  			< "$SQUASH_MSG" | sed -ne '$p')+1))
>> This sed replacement worries me.  I don't have a time to check myself
>> today but do we use \(this\|or\|that\) alternates with our sed script
>> already elsewhere in the codebase (test scripts do not count)?
>>
>> Otherwise this may suddenly be breaking a platform that has an
>> implementation of sed that may be substandard but so far has been
>> sufficient to work with git.
> 
> IIRC "|" was not correctly handled by BSD sed (used e.g. in MacOSX).
> 
> So maybe it would be best to just look for "commit message"?  I agree with
> Michael that the regex should not be too loose.

Thanks for pointing this out.  I replaced the problematic part of the
regexp with "[snrt][tdh]", which isn't quite so selective but should
be portable.  (This is the same fix that Junio suggested.)

Björn Gustavsson wrote:
> On Fri, Dec 4, 2009 at 3:36 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> Nitpick: the recommended style is to leave out the full stop
> at the end of the first line of the commit message.

Nit picked.

Junio C Hamano wrote:
> IIRC, the end result of the bikeshedding for the name of the command was
> won by Dscho's "fixup":
> 
>   http://thread.gmane.org/gmane.comp.version-control.git/127923/focus=121820

That's fine with me (the abbreviation is even the same :-) ).
Changed.

Michael

Michael Haggerty (2):
  t3404: Use test_commit to set up test repository
  Add a command "fixup" to rebase --interactive

 Documentation/git-rebase.txt  |   13 +++--
 git-rebase--interactive.sh    |   51 +++++++++++++++++----
 t/lib-rebase.sh               |    7 ++-
 t/t3404-rebase-interactive.sh |   96 +++++++++++++++++++++-------------------
 4 files changed, 103 insertions(+), 64 deletions(-)

^ permalink raw reply

* [PATCHv2 2/2] Add a command "fixup" to rebase --interactive
From: Michael Haggerty @ 2009-12-07  4:22 UTC (permalink / raw)
  To: git; +Cc: gitster, git, Johannes.Schindelin, bgustavsson, Michael Haggerty
In-Reply-To: <cover.1260099005.git.mhagger@alum.mit.edu>

The command is like "squash", except that it discards the commit message
of the corresponding commit.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 Documentation/git-rebase.txt  |   13 ++++++----
 git-rebase--interactive.sh    |   51 +++++++++++++++++++++++++++++++++--------
 t/lib-rebase.sh               |    7 +++--
 t/t3404-rebase-interactive.sh |   30 ++++++++++++++++++++++++
 4 files changed, 83 insertions(+), 18 deletions(-)

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index ca5e1e8..9b648ec 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -382,9 +382,12 @@ If you just want to edit the commit message for a commit, replace the
 command "pick" with the command "reword".
 
 If you want to fold two or more commits into one, replace the command
-"pick" with "squash" for the second and subsequent commit.  If the
-commits had different authors, it will attribute the squashed commit to
-the author of the first commit.
+"pick" for the second and subsequent commits with "squash" or "fixup".
+If the commits had different authors, the folded commit will be
+attributed to the author of the first commit.  The suggested commit
+message for the folded commit is the concatenation of the commit
+messages of the first commit and of those with the "squash" command,
+but omits the commit messages of commits with the "fixup" command.
 
 'git-rebase' will stop when "pick" has been replaced with "edit" or
 when a command fails due to merge errors. When you are done editing
@@ -512,8 +515,8 @@ Easy case: The changes are literally the same.::
 Hard case: The changes are not the same.::
 
 	This happens if the 'subsystem' rebase had conflicts, or used
-	`\--interactive` to omit, edit, or squash commits; or if the
-	upstream used one of `commit \--amend`, `reset`, or
+	`\--interactive` to omit, edit, squash, or fixup commits; or
+	if the upstream used one of `commit \--amend`, `reset`, or
 	`filter-branch`.
 
 
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 0bd3bf7..a7de5ea 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -302,7 +302,13 @@ nth_string () {
 
 make_squash_message () {
 	if test -f "$SQUASH_MSG"; then
-		COUNT=$(($(sed -n "s/^# This is [^0-9]*\([1-9][0-9]*\).*/\1/p" \
+		# We want to be careful about matching only the commit
+		# message comment lines generated by this function.
+		# But supposedly some sed versions don't handle "\|"
+		# correctly, so instead of "\(st\|nd\|rd\|th\)", use
+		# the less accurate "[snrt][tdh]" to match the
+		# nth_string endings.
+		COUNT=$(($(sed -n "s/^# Th[^0-9]*\([1-9][0-9]*\)[snrt][tdh] commit message.*:/\1/p" \
 			< "$SQUASH_MSG" | sed -ne '$p')+1))
 		echo "# This is a combination of $COUNT commits."
 		sed -e 1d -e '2,/^./{
@@ -315,10 +321,26 @@ make_squash_message () {
 		echo
 		git cat-file commit HEAD | sed -e '1,/^$/d'
 	fi
-	echo
-	echo "# This is the $(nth_string $COUNT) commit message:"
-	echo
-	git cat-file commit $1 | sed -e '1,/^$/d'
+	case $1 in
+	squash)
+		echo
+		echo "# This is the $(nth_string $COUNT) commit message:"
+		echo
+		git cat-file commit $2 | sed -e '1,/^$/d'
+		;;
+	fixup)
+		echo
+		echo "# The $(nth_string $COUNT) commit message will be skipped:"
+		echo
+		# Comment the lines of the commit message out using
+		# "#" rather than "# " (a) to make them more distinct
+		# from the explanatory comments added by this function
+		# and (b) to make it less likely that the sed regexp
+		# above will be confused by a commented-out commit
+		# message.
+		git cat-file commit $2 | sed -e '1,/^$/d' -e 's/^/#/'
+		;;
+	esac
 }
 
 peek_next_command () {
@@ -367,20 +389,28 @@ do_next () {
 		warn
 		exit 0
 		;;
-	squash|s)
-		comment_for_reflog squash
+	squash|s|fixup|f)
+		case "$command" in
+		squash|s)
+			squash_style=squash
+			;;
+		fixup|f)
+			squash_style=fixup
+			;;
+		esac
+		comment_for_reflog $squash_style
 
 		test -f "$DONE" && has_action "$DONE" ||
-			die "Cannot 'squash' without a previous commit"
+			die "Cannot '$squash_style' without a previous commit"
 
 		mark_action_done
-		make_squash_message $sha1 > "$MSG"
+		make_squash_message $squash_style $sha1 > "$MSG"
 		failed=f
 		author_script=$(get_author_ident_from_commit HEAD)
 		output git reset --soft HEAD^
 		pick_one -n $sha1 || failed=t
 		case "$(peek_next_command)" in
-		squash|s)
+		squash|s|fixup|f)
 			USE_OUTPUT=output
 			MSG_OPT=-F
 			EDIT_OR_FILE="$MSG"
@@ -768,6 +798,7 @@ first and then run 'git rebase --continue' again."
 #  r, reword = use commit, but edit the commit message
 #  e, edit = use commit, but stop for amending
 #  s, squash = use commit, but meld into previous commit
+#  f, fixup = like "squash", but discard this commit's log message
 #
 # If you remove a line here THAT COMMIT WILL BE LOST.
 # However, if you remove everything, the rebase will be aborted.
diff --git a/t/lib-rebase.sh b/t/lib-rebase.sh
index 62f452c..f4dda02 100644
--- a/t/lib-rebase.sh
+++ b/t/lib-rebase.sh
@@ -9,8 +9,9 @@
 #
 #	"[<lineno1>] [<lineno2>]..."
 #
-#   If a line number is prefixed with "squash", "edit", or "reword", the
-#   respective line's command will be replaced with the specified one.
+#   If a line number is prefixed with "squash", "fixup", "edit", or
+#   "reword", the respective line's command will be replaced with the
+#   specified one.
 
 set_fake_editor () {
 	echo "#!$SHELL_PATH" >fake-editor.sh
@@ -32,7 +33,7 @@ cat "$1".tmp
 action=pick
 for line in $FAKE_LINES; do
 	case $line in
-	squash|edit|reword)
+	squash|fixup|edit|reword)
 		action="$line";;
 	*)
 		echo sed -n "${line}s/^pick/$action/p"
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 778daf4..ea26115 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -235,6 +235,36 @@ test_expect_success 'multi-squash only fires up editor once' '
 	test 1 = $(git show | grep ONCE | wc -l)
 '
 
+test_expect_success 'multi-fixup only fires up editor once' '
+	git checkout -b multi-fixup E &&
+	base=$(git rev-parse HEAD~4) &&
+	FAKE_COMMIT_AMEND="ONCE" FAKE_LINES="1 fixup 2 fixup 3 fixup 4" \
+		git rebase -i $base &&
+	test $base = $(git rev-parse HEAD^) &&
+	test 1 = $(git show | grep ONCE | wc -l) &&
+	git checkout to-be-rebased &&
+	git branch -D multi-fixup
+'
+
+cat > expect-squash-fixup << EOF
+B
+
+D
+
+ONCE
+EOF
+
+test_expect_success 'squash and fixup generate correct log messages' '
+	git checkout -b squash-fixup E &&
+	base=$(git rev-parse HEAD~4) &&
+	FAKE_COMMIT_AMEND="ONCE" FAKE_LINES="1 fixup 2 squash 3 fixup 4" \
+		git rebase -i $base &&
+	git cat-file commit HEAD | sed -e 1,/^\$/d > actual-squash-fixup &&
+	test_cmp expect-squash-fixup actual-squash-fixup &&
+	git checkout to-be-rebased &&
+	git branch -D squash-fixup
+'
+
 test_expect_success 'squash works as expected' '
 	for n in one two three four
 	do
-- 
1.6.6.rc1.34.gd7b83

^ permalink raw reply related

* [PATCHv2 1/2] t3404: Use test_commit to set up test repository
From: Michael Haggerty @ 2009-12-07  4:22 UTC (permalink / raw)
  To: git; +Cc: gitster, git, Johannes.Schindelin, bgustavsson, Michael Haggerty
In-Reply-To: <cover.1260099005.git.mhagger@alum.mit.edu>

Also adjust "expected" text to reflect the file contents generated by
test_commit, which are slightly different than those generated by the
old code.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 t/t3404-rebase-interactive.sh |   66 ++++++++++++----------------------------
 1 files changed, 20 insertions(+), 46 deletions(-)

diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 3a37793..778daf4 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -16,53 +16,26 @@ set_fake_editor
 
 # set up two branches like this:
 #
-# A - B - C - D - E
+# A - B - C - D - E     (master)
 #   \
-#     F - G - H
+#     F - G - H         (branch1)
 #       \
-#         I
+#         I             (branch2)
 #
-# where B, D and G touch the same file.
+# where A, B, D and G touch the same file.
 
 test_expect_success 'setup' '
-	: > file1 &&
-	git add file1 &&
-	test_tick &&
-	git commit -m A &&
-	git tag A &&
-	echo 1 > file1 &&
-	test_tick &&
-	git commit -m B file1 &&
-	: > file2 &&
-	git add file2 &&
-	test_tick &&
-	git commit -m C &&
-	echo 2 > file1 &&
-	test_tick &&
-	git commit -m D file1 &&
-	: > file3 &&
-	git add file3 &&
-	test_tick &&
-	git commit -m E &&
+	test_commit A file1 &&
+	test_commit B file1 &&
+	test_commit C file2 &&
+	test_commit D file1 &&
+	test_commit E file3 &&
 	git checkout -b branch1 A &&
-	: > file4 &&
-	git add file4 &&
-	test_tick &&
-	git commit -m F &&
-	git tag F &&
-	echo 3 > file1 &&
-	test_tick &&
-	git commit -m G file1 &&
-	: > file5 &&
-	git add file5 &&
-	test_tick &&
-	git commit -m H &&
+	test_commit F file4 &&
+	test_commit G file1 &&
+	test_commit H file5 &&
 	git checkout -b branch2 F &&
-	: > file6 &&
-	git add file6 &&
-	test_tick &&
-	git commit -m I &&
-	git tag I
+	test_commit I file6
 '
 
 test_expect_success 'no changes are a nop' '
@@ -111,19 +84,20 @@ test_expect_success 'exchange two commits' '
 
 cat > expect << EOF
 diff --git a/file1 b/file1
-index e69de29..00750ed 100644
+index f70f10e..fd79235 100644
 --- a/file1
 +++ b/file1
-@@ -0,0 +1 @@
-+3
+@@ -1 +1 @@
+-A
++G
 EOF
 
 cat > expect2 << EOF
 <<<<<<< HEAD
-2
+D
 =======
-3
->>>>>>> b7ca976... G
+G
+>>>>>>> 91201e5... G
 EOF
 
 test_expect_success 'stop on conflicting pick' '
-- 
1.6.6.rc1.34.gd7b83

^ permalink raw reply related

* Re: [PATCH 2/2] status -s: obey color.status
From: Jeff King @ 2009-12-07  5:17 UTC (permalink / raw)
  To: Michael J Gruber; +Cc: git, Junio C Hamano
In-Reply-To: <2b987524f57a0ac04e219f82e20e806741ce4eca.1260025135.git.git@drmicha.warpmail.net>

On Sat, Dec 05, 2009 at 04:04:38PM +0100, Michael J Gruber wrote:

> Make the short version of status obey the color.status boolean. We color
> the status letters only, because they carry the state information and are
> potentially colored differently, such as for a file with staged changes
> as well as changes in the worktree against the index.

This seems to also turn on color for --porcelain in some cases, because
git_status_config unconditionally sets s->use_color if you are using
color.status instead of color.ui. I think we are probably best just
explicitly disabling options for the "porcelain" format rather than
trying to come up with some trickery to make sure they never get set.
Like:

-- >8 --
Subject: [PATCH] status: disable color for porcelain format

The porcelain format is identical to the shortstatus format,
except that it should not respect any user configuration,
including color.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin-commit.c |    4 ++--
 wt-status.c      |    6 ++++++
 wt-status.h      |    1 +
 3 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/builtin-commit.c b/builtin-commit.c
index ddcfffb..88b25aa 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -388,7 +388,7 @@ static int run_status(FILE *fp, const char *index_file, const char *prefix, int
 		wt_shortstatus_print(s, null_termination);
 		break;
 	case STATUS_FORMAT_PORCELAIN:
-		wt_shortstatus_print(s, null_termination);
+		wt_porcelain_print(s, null_termination);
 		break;
 	case STATUS_FORMAT_LONG:
 		wt_status_print(s);
@@ -1029,7 +1029,7 @@ int cmd_status(int argc, const char **argv, const char *prefix)
 		wt_shortstatus_print(&s, null_termination);
 		break;
 	case STATUS_FORMAT_PORCELAIN:
-		wt_shortstatus_print(&s, null_termination);
+		wt_porcelain_print(&s, null_termination);
 		break;
 	case STATUS_FORMAT_LONG:
 		s.verbose = verbose;
diff --git a/wt-status.c b/wt-status.c
index a8b6d05..e9bbfbc 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -696,3 +696,9 @@ void wt_shortstatus_print(struct wt_status *s, int null_termination)
 		wt_shortstatus_untracked(null_termination, it, s);
 	}
 }
+
+void wt_porcelain_print(struct wt_status *s, int null_termination)
+{
+	s->use_color = 0;
+	wt_shortstatus_print(s, null_termination);
+}
diff --git a/wt-status.h b/wt-status.h
index 39c9aef..a4bddcf 100644
--- a/wt-status.h
+++ b/wt-status.h
@@ -57,5 +57,6 @@ void wt_status_print(struct wt_status *s);
 void wt_status_collect(struct wt_status *s);
 
 void wt_shortstatus_print(struct wt_status *s, int null_termination);
+void wt_porcelain_print(struct wt_status *s, int null_termination);
 
 #endif /* STATUS_H */
-- 
1.6.6.rc1.292.gd8fe.dirty

^ permalink raw reply related

* Re: [PATCH 2/2] status -s: obey color.status
From: Jeff King @ 2009-12-07  5:26 UTC (permalink / raw)
  To: Michael J Gruber; +Cc: git, Junio C Hamano
In-Reply-To: <20091207051715.GA17521@coredump.intra.peff.net>

On Mon, Dec 07, 2009 at 12:17:15AM -0500, Jeff King wrote:

> This seems to also turn on color for --porcelain in some cases, because
> git_status_config unconditionally sets s->use_color if you are using
> color.status instead of color.ui. I think we are probably best just
> explicitly disabling options for the "porcelain" format rather than
> trying to come up with some trickery to make sure they never get set.

Also, this means we can hoist repeated code out of the switch statement,
like this:

-- >8 --
Subject: [PATCH] status: reduce duplicated setup code

We have three output formats: short, porcelain, and long.
The short and long formats respect user-config, and the
porcelain one does not. This led to us repeating
config-related setup code for the short and long formats.

Since the last commit, color config is explicitly cleared
when showing the porcelain format. Let's do the same with
relative-path configuration, which enables us to hoist the
duplicated code from the switch statement in cmd_status.

As a bonus, this fixes "commit --dry-run --porcelain", which
was unconditionally setting up that configuration, anyway.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin-commit.c |   19 +++++++------------
 wt-status.c      |    2 ++
 2 files changed, 9 insertions(+), 12 deletions(-)

diff --git a/builtin-commit.c b/builtin-commit.c
index 88b25aa..a11e585 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -1018,14 +1018,15 @@ int cmd_status(int argc, const char **argv, const char *prefix)
 	s.is_initial = get_sha1(s.reference, sha1) ? 1 : 0;
 	wt_status_collect(&s);
 
+	if (s.relative_paths)
+		s.prefix = prefix;
+	if (s.use_color == -1)
+		s.use_color = git_use_color_default;
+	if (diff_use_color_default == -1)
+		diff_use_color_default = git_use_color_default;
+
 	switch (status_format) {
 	case STATUS_FORMAT_SHORT:
-		if (s.relative_paths)
-			s.prefix = prefix;
-		if (s.use_color == -1)
-			s.use_color = git_use_color_default;
-		if (diff_use_color_default == -1)
-			diff_use_color_default = git_use_color_default;
 		wt_shortstatus_print(&s, null_termination);
 		break;
 	case STATUS_FORMAT_PORCELAIN:
@@ -1033,12 +1034,6 @@ int cmd_status(int argc, const char **argv, const char *prefix)
 		break;
 	case STATUS_FORMAT_LONG:
 		s.verbose = verbose;
-		if (s.relative_paths)
-			s.prefix = prefix;
-		if (s.use_color == -1)
-			s.use_color = git_use_color_default;
-		if (diff_use_color_default == -1)
-			diff_use_color_default = git_use_color_default;
 		wt_status_print(&s);
 		break;
 	}
diff --git a/wt-status.c b/wt-status.c
index e9bbfbc..55b6696 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -700,5 +700,7 @@ void wt_shortstatus_print(struct wt_status *s, int null_termination)
 void wt_porcelain_print(struct wt_status *s, int null_termination)
 {
 	s->use_color = 0;
+	s->relative_paths = 0;
+	s->prefix = NULL;
 	wt_shortstatus_print(s, null_termination);
 }
-- 
1.6.6.rc1.292.gd8fe.dirty

^ permalink raw reply related

* [PATCHv2] tests: handle NO_PYTHON setting
From: Jeff King @ 2009-12-07  5:32 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Brandon Casey, Johan Herland

Without this, test-lib checks that the git_remote_helpers
directory has been built. However, if we are building
without python, we will not have done anything at all in
that directory, and test-lib's sanity check will fail.

We bump the inclusion of GIT-BUILD-OPTIONS further up in
test-lib; it contains configuration, and as such should be
read before we do any checks (and in this particular case,
we need its value to do our check properly).

Signed-off-by: Jeff King <peff@peff.net>
Looks-fine-to-me-by: Brandon Casey <brandon.casey.ctr@nrlssc.navy.mil>
Acked-by: Johan Herland <johan@herland.net>
---
On top of sr/vcs-helper.

Just a repost with acks, as the original seems to have been forgotten in
the ensuing discussion of directory structure.

 Makefile      |    1 +
 t/test-lib.sh |    6 +++---
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/Makefile b/Makefile
index 42744a4..443565e 100644
--- a/Makefile
+++ b/Makefile
@@ -1743,6 +1743,7 @@ GIT-BUILD-OPTIONS: .FORCE-GIT-BUILD-OPTIONS
 	@echo TAR=\''$(subst ','\'',$(subst ','\'',$(TAR)))'\' >>$@
 	@echo NO_CURL=\''$(subst ','\'',$(subst ','\'',$(NO_CURL)))'\' >>$@
 	@echo NO_PERL=\''$(subst ','\'',$(subst ','\'',$(NO_PERL)))'\' >>$@
+	@echo NO_PYTHON=\''$(subst ','\'',$(subst ','\'',$(NO_PYTHON)))'\' >>$@
 
 ### Detect Tck/Tk interpreter path changes
 ifndef NO_TCLTK
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 4a40520..2d523fe 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -632,13 +632,15 @@ GIT_CONFIG_NOSYSTEM=1
 GIT_CONFIG_NOGLOBAL=1
 export PATH GIT_EXEC_PATH GIT_TEMPLATE_DIR GIT_CONFIG_NOSYSTEM GIT_CONFIG_NOGLOBAL
 
+. ../GIT-BUILD-OPTIONS
+
 GITPERLLIB=$(pwd)/../perl/blib/lib:$(pwd)/../perl/blib/arch/auto/Git
 export GITPERLLIB
 test -d ../templates/blt || {
 	error "You haven't built things yet, have you?"
 }
 
-if test -z "$GIT_TEST_INSTALLED"
+if test -z "$GIT_TEST_INSTALLED" && test -z "$NO_PYTHON"
 then
 	GITPYTHONLIB="$(pwd)/../git_remote_helpers/build/lib"
 	export GITPYTHONLIB
@@ -653,8 +655,6 @@ if ! test -x ../test-chmtime; then
 	exit 1
 fi
 
-. ../GIT-BUILD-OPTIONS
-
 # Test repository
 test="trash directory.$(basename "$0" .sh)"
 test -n "$root" && test="$root/$test"
-- 
1.6.6.rc1.292.gd8fe.dirty

^ permalink raw reply related

* Re: [PATCH] Allow --quiet option to git remote, particularly for `git remote update`
From: Alex Vandiver @ 2009-12-07  6:15 UTC (permalink / raw)
  To: Jeff King; +Cc: git
In-Reply-To: <20091206145000.GC26440@coredump.intra.peff.net>

At Sun Dec 06 09:50:00 -0500 2009, Jeff King wrote:
> Should --quiet also affect this "Updating %s" line?

Looking at it more closely, I think this patch should -- but for
reasons unrelated o your argument below.  The "Updating %s" line there
is in fetch_remote, which is _only_ called during `git remote add -f`.
It stands in for the "Fetching %s" line which `git fetch` (and `git
remote update`) outputs, which (after this patch), _is_ controlled by
--quiet.

Thus I feel like "Updating %s" should change to "Fetching %s" for both
consistency and explicitness, and should also be controlled by
--quiet.  The --quiet-remote option is an entirely different bikeshed,
which I don't have a strong opinion on, offhand.

 - Alex
-- 
Networking -- only one letter away from not working

^ permalink raw reply

* Re: [PATCH] Allow --quiet option to git remote, particularly for `git remote update`
From: Jeff King @ 2009-12-07  6:40 UTC (permalink / raw)
  To: Alex Vandiver; +Cc: git
In-Reply-To: <1260156352-sup-6170@utwig>

On Mon, Dec 07, 2009 at 01:15:05AM -0500, Alex Vandiver wrote:

> At Sun Dec 06 09:50:00 -0500 2009, Jeff King wrote:
> > Should --quiet also affect this "Updating %s" line?
> 
> Looking at it more closely, I think this patch should -- but for
> reasons unrelated o your argument below.  The "Updating %s" line there
> is in fetch_remote, which is _only_ called during `git remote add -f`.
> It stands in for the "Fetching %s" line which `git fetch` (and `git
> remote update`) outputs, which (after this patch), _is_ controlled by
> --quiet.

Ah, sorry, this has actually changed since the last time I looked at it
closely, as a result of 9c4a036 (Teach the --all option to 'git fetch',
2009-11-09). So nevermind my complaint...it has actually been addressed
separately (I didn't notice because I have been suppressing the output
by redirecting stdout for some time).

> Thus I feel like "Updating %s" should change to "Fetching %s" for both
> consistency and explicitness, and should also be controlled by
> --quiet.  The --quiet-remote option is an entirely different bikeshed,
> which I don't have a strong opinion on, offhand.

Yes, I agree (that it should be quieted, and that it should say
"Fetching").  My --quiet-remote argument is now pointless as of
9c4a036b.

-Peff

^ permalink raw reply

* Re: [StGit PATCH v2 0/6] add support for git send-email
From: Karl Wiberg @ 2009-12-07  7:09 UTC (permalink / raw)
  To: Catalin Marinas; +Cc: Alex Chiang, git
In-Reply-To: <b0943d9e0912061416y4089643l2a5ebdf1c1c0960b@mail.gmail.com>

On Sun, Dec 6, 2009 at 11:16 PM, Catalin Marinas
<catalin.marinas@gmail.com> wrote:

> --refid -> --in-reply-to
> --noreply -> --no-thread

And I must say, the git options are better named than ours anyway ...

-- 
Karl Wiberg, kha@treskal.com
   subrabbit.wordpress.com
   www.treskal.com/kalle

^ permalink raw reply

* Re: [PATCHv2 2/2] Add a command "fixup" to rebase --interactive
From: Junio C Hamano @ 2009-12-07  7:12 UTC (permalink / raw)
  To: Michael Haggerty; +Cc: git, git, Johannes.Schindelin, bgustavsson
In-Reply-To: <ced6765cff6225a05f196a6896ab577850979ab1.1260099005.git.mhagger@alum.mit.edu>

Michael Haggerty <mhagger@alum.mit.edu> writes:

> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index 0bd3bf7..a7de5ea 100755
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -302,7 +302,13 @@ nth_string () {
>  
>  make_squash_message () {
>  	if test -f "$SQUASH_MSG"; then
> -		COUNT=$(($(sed -n "s/^# This is [^0-9]*\([1-9][0-9]*\).*/\1/p" \
> +		# We want to be careful about matching only the commit
> +		# message comment lines generated by this function.

> +		# But supposedly some sed versions don't handle "\|"
> +		# correctly, so instead of "\(st\|nd\|rd\|th\)", use
> +		# the less accurate "[snrt][tdh]" to match the
> +		# nth_string endings.

I'd drop this comment; blaming POSIX-compliant sed without GNU extension
is simply wrong.

> +		COUNT=$(($(sed -n "s/^# Th[^0-9]*\([1-9][0-9]*\)[snrt][tdh] commit message.*:/\1/p" \
>  			< "$SQUASH_MSG" | sed -ne '$p')+1))
>  		echo "# This is a combination of $COUNT commits."
>  		sed -e 1d -e '2,/^./{
> @@ -315,10 +321,26 @@ make_squash_message () {
>  		echo
>  		git cat-file commit HEAD | sed -e '1,/^$/d'
>  	fi
> -	echo
> -	echo "# This is the $(nth_string $COUNT) commit message:"
> -	echo
> -	git cat-file commit $1 | sed -e '1,/^$/d'
> +	case $1 in
> +	squash)
> +		echo
> +		echo "# This is the $(nth_string $COUNT) commit message:"
> +		echo
> +		git cat-file commit $2 | sed -e '1,/^$/d'
> +		;;
> +	fixup)
> +		echo
> +		echo "# The $(nth_string $COUNT) commit message will be skipped:"
> +		echo
> +		# Comment the lines of the commit message out using
> +		# "#" rather than "# " (a) to make them more distinct
> +		# from the explanatory comments added by this function
> +		# and (b) to make it less likely that the sed regexp
> +		# above will be confused by a commented-out commit
> +		# message.

Use "#  " as prefix and you won't have to worry about a line in the log
message that begins with " Th", no?

In any case, I agree that a comment like this is necessary to warn anybody
who will be touching the code that the COUNT=$((...))  needs to avoid
matching what is produced here, but I find the above 6-line comment a bit
too excessive.

^ permalink raw reply

* Re: [PATCH] Allow --quiet option to git remote, particularly for `git remote update`
From: Alex Vandiver @ 2009-12-07  7:23 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git
In-Reply-To: <7vd42soo2p.fsf@alter.siamese.dyndns.org>

At Sat Dec 05 21:04:14 -0500 2009, Junio C Hamano wrote:
> Alex Vandiver <alex@chmrr.net> writes:
> > ...
> >      "git remote prune [-n | --dry-run] <name>",
> > -    "git remote [-v | --verbose] update [-p | --prune] [group | remote]",
> > +    "git remote [-v | --verbose] [-q | --quiet] update [-p | --prune] [group]",

Hm, I hadn't noticed that I'd changed "[group | remote]" to "[group]".
I think this is due to a mismerge on my part -- apologies.  As another
data point, `git fetch` describes this as "[<repository> | <group>]".

> Three issues to consider:
> 
>  - shouldn't we use the same typography, i.e. <group>?
> 
>  - should we say <name> _if_ we are not going to say <group>|<remote>?
> 
>  - should we keep it as <group>|<remote> to make it clear that only this
>    subcommand allows the group nickname?
> 
> The first two are easy and I expect the answers to be both yes.  The third
> one needs some studying and further thought.
> 
>  - is "remote update" the only one that takes group nickname?

My quick skim of the code says "yes" -- the other commands only deal
with single remotes at a time, and prune is oblivious to groups.

>  - should "remote update" the only one? e.g. does "remote prune" also
>    take group? if not, shouldn't it?

Properly, it "ought" to, though I don't see much utility over `git
remote fetch --prune groupname`.  Probably at the same time, the
parallel pruning codepaths in builtin-fetch.c:prune_refs() and
builtin-remote.c:prune_remote() should be unified.
 - Alex
-- 
Networking -- only one letter away from not working

^ permalink raw reply

* Re: [PATCH v2] Detailed diagnosis when parsing an object name fails.
From: Junio C Hamano @ 2009-12-07  7:27 UTC (permalink / raw)
  To: Matthieu Moy; +Cc: git
In-Reply-To: <vpqhbs4dkjr.fsf@bauges.imag.fr>

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

The second round still had a few compilation warnings (turned to errors in
my environment) so I fixed them up somewhat.  I stopped before getting a
clean compile, though---you will still get:

  sha1_name.c: In function 'get_sha1_with_mode_1':
  sha1_name.c:956: error: 'object_name' may be used uninitialized in this function

even with this fix-up.

>> Instead of doing a leaky allocation, it may make sense to pass the tree
>> object name as <const char *, size_t> pair, and print it with "%.*s" in
>> the error reporting codepath.  After all, object_name is used only for
>> that purpose in diagnose_invalid_sha1_path(), no?
>
> matter of taste,...

I thought generally it is accepted that I had a better taste on this list?
;-)

-- squashed --

 cache.h     |    4 ++--
 sha1_name.c |    2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/cache.h b/cache.h
index 952bd51..3f9ee86 100644
--- a/cache.h
+++ b/cache.h
@@ -702,11 +702,11 @@ static inline unsigned int hexval(unsigned char c)
 #define DEFAULT_ABBREV 7
 
 extern int get_sha1(const char *str, unsigned char *sha1);
-static inline get_sha1_with_mode(const char *str, unsigned char *sha1, unsigned *mode)
+extern int get_sha1_with_mode_1(const char *str, unsigned char *sha1, unsigned *mode, int gently, const char *prefix);
+static inline int get_sha1_with_mode(const char *str, unsigned char *sha1, unsigned *mode)
 {
 	return get_sha1_with_mode_1(str, sha1, mode, 1, NULL);
 }
-extern int get_sha1_with_mode_1(const char *str, unsigned char *sha1, unsigned *mode, int gently, const char *prefix);
 extern int get_sha1_hex(const char *hex, unsigned char *sha1);
 extern char *sha1_to_hex(const unsigned char *sha1);	/* static buffer result! */
 extern int read_ref(const char *filename, unsigned char *sha1);
diff --git a/sha1_name.c b/sha1_name.c
index 1bc09ac..025244a 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -807,7 +807,7 @@ int get_sha1(const char *name, unsigned char *sha1)
 /* Must be called only when object_name:filename doesn't exist. */
 static void diagnose_invalid_sha1_path(const char *prefix,
 				       const char *filename,
-				       const char *tree_sha1,
+				       const unsigned char *tree_sha1,
 				       const char *object_name)
 {
 	struct stat st;

^ permalink raw reply related

* Re: [PATCH 1/2] Documentation: 'git add -A' can remove files
From: Junio C Hamano @ 2009-12-07  7:34 UTC (permalink / raw)
  To: Björn Steinbrink; +Cc: Björn Gustavsson, git
In-Reply-To: <20091207005732.GA20909@atjola.homenet>

Björn Steinbrink <B.Steinbrink@gmx.de> writes:

> What I usually say on #git is something like:
>
> 	"git add <path>" looks at the working tree to find files
> 	matching <path>.  "git add -u <path>" looks at the index, and
> 	"git add -A <path>" looks at both. Therefore "add" and "add -A"
> 	can add new files to the index, and "add -u" and "add -A" can
> 	remove files from it.
>
> 	And for convenience, -u and -A default to "." as the path argument.

Hmm, an interesting way of teaching. How effective is it with the real new
to mid-level users we see on the #git channel?

> So maybe something like this?
> ...
> -A, --all
>     Like -u, but matches <filepattern> against files in the index in
>     addition to the files in working tree. This means that it can find
>     new files as well.

Sounds easier to read to me but I think this description of "-A" seems
backwards from your explanation.  "Like -u" means "matches the index" and
the difference is it in addition matches with the work tree, so it should
probably be the other way around, i.e.

     Like -u, but matches <filepattern> against files in the work tree in
     addition to the index. This means that it can find new files as well.

^ permalink raw reply

* Re: [RFC PATCH v3 0/8] Remote helpers smart transport extensions
From: Junio C Hamano @ 2009-12-07  7:36 UTC (permalink / raw)
  To: Ilari Liusvaara; +Cc: git
In-Reply-To: <1260116931-16549-1-git-send-email-ilari.liusvaara@elisanet.fi>

I queued to ease the discussion in 'pu'.  I had to fix-up some conflicts
while doing so.  Please sanity check the result.

Thanks.

^ permalink raw reply

* [PATCH] Documentation: 'git add -A' can remove files
From: Björn Gustavsson @ 2009-12-07  8:20 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano

The current documentation fails to mention that 'git add -A/--all'
can remove files as well as add them, and it also does not
say anything about filepatterns (whether they are allowed,
mandatory, or optional). It is also not clear what the similarities
and differences to the -u option are.

Update the intro paragraph (as suggested by Junio, with some minor
edits) to make it clear that 'git add' is able to delete and to
also cover the -p option.

Reword the description of -u to make it clearer (based on by
Björn Steinbrink's suggestion).

Simplify the description of  -A by saying "Like -u" and then
describe the differences (based on the suggestions by Björn
Steinbrink and Junio).

Signed-off-by: Björn Gustavsson <bgustavsson@gmail.com>
---
I have to leave for work now, so I'll just send the first part
now, which I think is much improved compared to
the previous version. Thanks Junio and Björn S for the
suggestions.

 Documentation/git-add.txt |   33 ++++++++++++++++++++-------------
 1 files changed, 20 insertions(+), 13 deletions(-)

diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt
index e93e606..ed68ea3 100644
--- a/Documentation/git-add.txt
+++ b/Documentation/git-add.txt
@@ -14,8 +14,12 @@ SYNOPSIS
 
 DESCRIPTION
 -----------
-This command adds the current content of new or modified files to the
-index, thus staging that content for inclusion in the next commit.
+This command updates the index using the current content found in
+the working tree, to prepare the content staged for the next commit.
+It typically adds the current content of existing paths as a whole,
+but with some options it can also be used to add content with
+only part of the changes made to the working tree files applied, or
+remove paths that do not exist in the working tree anymore.
 
 The "index" holds a snapshot of the content of the working tree, and it
 is this snapshot that is taken as the contents of the next commit.  Thus
@@ -92,20 +96,23 @@ apply.
 
 -u::
 --update::
-	Update only files that git already knows about, staging modified
-	content for commit and marking deleted files for removal. This
-	is similar
-	to what "git commit -a" does in preparation for making a commit,
-	except that the update is limited to paths specified on the
-	command line. If no paths are specified, all tracked files in the
-	current directory and its subdirectories are updated.
+	Only match <filepattern> against already tracked files in
+	the index rather than the working tree. That means that it
+	will never stage new files, but that it will stage modified
+	new contents of tracked files and that it will remove files
+	from the index if the corresponding files in the working tree
+	have been removed.
++
+If no <filepattern> is given, default to "."; in other words,
+update all tracked files in the current directory and its
+subdirectories.
 
 -A::
 --all::
-	Update files that git already knows about (same as '\--update')
-	and add all untracked files that are not ignored by '.gitignore'
-	mechanism.
-
+	Like `-u`, but match <filepattern> against files in the
+	working tree in addition to the index. That means that it
+	will find new files as well as staging modified content and
+	removing files that are no longer in the working tree.
 
 -N::
 --intent-to-add::
-- 
1.6.6.rc1.31.g1a56b

^ permalink raw reply related

* Re: [PATCH] cvsserver: make the output of 'update' more compatible with cvs.
From: Sergei Organov @ 2009-12-07  8:27 UTC (permalink / raw)
  To: mmogilvi_git; +Cc: git, gitster
In-Reply-To: <20091205234831.GA925@comcast.net>

mmogilvi_git@miniinfo.net writes:

> On Thu, Dec 03, 2009 at 11:12:47PM +0300, Sergei Organov wrote:
>>  
>> +    my $last_dirname = "///";
>> +    
>>      # foreach file specified on the command line ...
>>      foreach my $filename ( @{$state->{args}} )
>>      {
>>          $filename = filecleanup($filename);
>> +        my $cur_dirname = dirname($filename);
>> +        if ( $cur_dirname ne $last_dirname )
>> +        {
>> +            $last_dirname = $cur_dirname;
>> +            if ( $cur_dirname eq "" )
>> +            {
>> +                $cur_dirname = ".";
>> +            }
>> +            print "E cvs update: Updating $cur_dirname\n";
>> +        }
>>  
>>          $log->debug("Processing file $filename");
>
> This should probably be conditional on the absense of the
> global "cvs -q update" and "cvs -Q update" options, in case
> other CVS clients depend on quiet operation when they specify
> those options.

Good catch, thanks! I'll redo the patch.

-- Sergei.

^ permalink raw reply

* Re: [PATCH 2/2] status -s: obey color.status
From: Michael J Gruber @ 2009-12-07  8:33 UTC (permalink / raw)
  To: Jeff King; +Cc: git, Junio C Hamano
In-Reply-To: <20091207051715.GA17521@coredump.intra.peff.net>

Jeff King venit, vidit, dixit 07.12.2009 06:17:
> On Sat, Dec 05, 2009 at 04:04:38PM +0100, Michael J Gruber wrote:
> 
>> Make the short version of status obey the color.status boolean. We color
>> the status letters only, because they carry the state information and are
>> potentially colored differently, such as for a file with staged changes
>> as well as changes in the worktree against the index.
> 
> This seems to also turn on color for --porcelain in some cases, because
> git_status_config unconditionally sets s->use_color if you are using
> color.status instead of color.ui. I think we are probably best just
> explicitly disabling options for the "porcelain" format rather than
> trying to come up with some trickery to make sure they never get set.
> Like:

Thanks. I let myself get fooled by the apparent option handling within
the switch statement (which is OK for relativePaths) (and some
subconscious link between null_termination and porcelain, which goes one
way only).

I guess this shows (again) that one needs tests for everything... If I
get around to I'll amend t$(relevantoneicantrecallrightnow) with tests
for long and short status with color and porcelain with the various options.

Michael

^ permalink raw reply

* Re: git-blame.el: what is format-spec?
From: Sergei Organov @ 2009-12-07  8:36 UTC (permalink / raw)
  To: David Kågedal; +Cc: Andreas Schwab, git
In-Reply-To: <87d42s3pv2.fsf@lysator.liu.se>

David Kågedal <davidk@lysator.liu.se> writes:

> Sergei Organov <osv@javad.com> writes:
>
>> Then there should be (require 'format-spec) in git-blame.el, right? Due
>> to:
>
> Of course. I must have missed that since I already had it loaded.
>
>> Now, I've evaluated (require 'format-spec) in my Emacs 22 (yes, 22, not
>> 23), and now git-blame almost works there. The problem I see is that it
>> doesn't output anything in the echo area. It color-codes the buffer, it
>> does show correct pop-up when mouse is over a region, but it doesn't
>> print anything in the echo area when I move cursor through the regions.
>> Any idea how to debug/fix this?
>
> Well, it appears I removed the output to the echo area. I didn't think
> it worked very well, and the new output format mostly replaces it by
> showing the hash.
>
> There are also technical reasons for removing it (it couldn't be
> implemented very cleanly).

I didn't know you deliberately removed it, -- I thought it's some
Emacs22 - related problem. I have no problem then, except that I don't
know how to get the hash into my kill-ring or X selection.

While we are at it, why is git-blame-identify interactive and how is it
useful?

>
> It would of course be possible to restore the old way, but I think it
> would be good to ask ourselves what we really would like to see? Some
> ideas:
>
> * A keybinding to show the commit introducing the current line,
>   including diff and all.
>
> * A keybinding to show the commit message in the echo areia.

For me, these 2 would be more than enough, I think.

-- Sergei.

^ permalink raw reply

* Re: [PATCH 1/2] Documentation: 'git add -A' can remove files
From: Björn Steinbrink @ 2009-12-07  8:48 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Björn Gustavsson, git
In-Reply-To: <7vbpib9r10.fsf@alter.siamese.dyndns.org>

On 2009.12.06 23:34:03 -0800, Junio C Hamano wrote:
> Björn Steinbrink <B.Steinbrink@gmx.de> writes:
> 
> > What I usually say on #git is something like:
> >
> > 	"git add <path>" looks at the working tree to find files
> > 	matching <path>.  "git add -u <path>" looks at the index, and
> > 	"git add -A <path>" looks at both. Therefore "add" and "add -A"
> > 	can add new files to the index, and "add -u" and "add -A" can
> > 	remove files from it.
> >
> > 	And for convenience, -u and -A default to "." as the path argument.
> 
> Hmm, an interesting way of teaching. How effective is it with the real new
> to mid-level users we see on the #git channel?

Hm, I wanted to check the channel logs for some reactions, but couldn't
find much. Apparently, I've not answered many question about add -u/-A
on #git since I switched to the above explanation (instead of saying
"git add -A is like git add . && git add -u" as I did in the past). For
the cases I found, the users didn't reply to the explanation. I can't
tell whether the explanation sucks, or whether the user was happy with
it or simply didn't care because he got the command he wanted.

I have two more data points for that kind of explanation in my Jabber
history, but those users aren't "new to mid-level" anymore.

And I recall only one really new user I told this to in face-to-face
"teaching", and that was right after explaining the index, so the
concept was present.

I guess the explanation might not be as easy to grok for someone that
doesn't have a good understanding of the index. For example, those that
still think that the index only stores changes (and is empty when no
"changes" have been staged) probably won't be able to make any sense out
of it.

Guess I'll have to apply a bunch of context when choosing which
explanation to use.

> 
> > So maybe something like this?
> > ...
> > -A, --all
> >     Like -u, but matches <filepattern> against files in the index in
> >     addition to the files in working tree. This means that it can find
> >     new files as well.
> 
> Sounds easier to read to me but I think this description of "-A" seems
> backwards from your explanation.  "Like -u" means "matches the index" and
> the difference is it in addition matches with the work tree, so it should
> probably be the other way around, i.e.
> 
>      Like -u, but matches <filepattern> against files in the work tree in
>      addition to the index. This means that it can find new files as well.

Hm, I meant that to show that while -u uses the index _instead_ of the
working tree, -A uses the index _in_addition_to_ the working tree, i.e.
I meant to say how -A differs from -u WRT its effect compared to a plain
"git add".

IOW, I treat "git add" (working tree only) as the default behaviour. And
-u changes it to "index only", while -A changes it to "working tree +
index". Your version makes it look more like -A is a modification of -u.

OTOH, I also used to say "to update all tracked files, use add -u, and
to also add new files use add -A", which is better matched by your
wording. Though I'd go for this then:

    Like -u, but also match <filepattern> against files in the working
    tree. This means that it can find new files as well

Which just says what -A does on top of -u, without repeating what -u
does.

Björn

^ permalink raw reply

* Re: git-blame.el: what is format-spec?
From: David Kågedal @ 2009-12-07  9:05 UTC (permalink / raw)
  To: Sergei Organov; +Cc: git, Andreas Schwab
In-Reply-To: <87k4wzdvuk.fsf@osv.gnss.ru>

Sergei Organov <osv@javad.com> writes:

> David Kågedal <davidk@lysator.liu.se> writes:
>
>> Sergei Organov <osv@javad.com> writes:
>>
>>> Then there should be (require 'format-spec) in git-blame.el, right? Due
>>> to:
>>
>> Of course. I must have missed that since I already had it loaded.
>>
>>> Now, I've evaluated (require 'format-spec) in my Emacs 22 (yes, 22, not
>>> 23), and now git-blame almost works there. The problem I see is that it
>>> doesn't output anything in the echo area. It color-codes the buffer, it
>>> does show correct pop-up when mouse is over a region, but it doesn't
>>> print anything in the echo area when I move cursor through the regions.
>>> Any idea how to debug/fix this?
>>
>> Well, it appears I removed the output to the echo area. I didn't think
>> it worked very well, and the new output format mostly replaces it by
>> showing the hash.
>>
>> There are also technical reasons for removing it (it couldn't be
>> implemented very cleanly).
>
> I didn't know you deliberately removed it, -- I thought it's some
> Emacs22 - related problem. I have no problem then, except that I don't
> know how to get the hash into my kill-ring or X selection.
>
> While we are at it, why is git-blame-identify interactive and how is it
> useful?

I think I need to go over this file with a fine-tooth coam to remove or
fix bitrot. This command seems to try to show the hash in the echo area,
but probably doesn't work. New and better commands should be deviced.

I'll try to find some time to work on this soon.

Thanks a lot for the feedback.

-- 
David Kågedal

^ permalink raw reply

* [PATCHv3 0/2] Add a "fixup" command to "rebase --interactive"
From: Michael Haggerty @ 2009-12-07  9:20 UTC (permalink / raw)
  To: git; +Cc: gitster, git, Johannes.Schindelin, bgustavsson, Michael Haggerty
In-Reply-To: <7vskbn9s1k.fsf@alter.siamese.dyndns.org>

Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
>> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
>> index 0bd3bf7..a7de5ea 100755
>> --- a/git-rebase--interactive.sh
>> +++ b/git-rebase--interactive.sh
>> @@ -302,7 +302,13 @@ nth_string () {
>>  
>>  make_squash_message () {
>>  	if test -f "$SQUASH_MSG"; then
>> -		COUNT=$(($(sed -n "s/^# This is [^0-9]*\([1-9][0-9]*\).*/\1/p" \
>> +		# We want to be careful about matching only the commit
>> +		# message comment lines generated by this function.
> 
>> +		# But supposedly some sed versions don't handle "\|"
>> +		# correctly, so instead of "\(st\|nd\|rd\|th\)", use
>> +		# the less accurate "[snrt][tdh]" to match the
>> +		# nth_string endings.
> 
> I'd drop this comment; blaming POSIX-compliant sed without GNU extension
> is simply wrong.

Fair enough.  I hope you don't mind my leaving a line explaining the
cryptic "[snrt][tdh]" to save Dscho a couple of seconds next time :-).

>> +		COUNT=$(($(sed -n "s/^# Th[^0-9]*\([1-9][0-9]*\)[snrt][tdh] commit message.*:/\1/p" \
>>  			< "$SQUASH_MSG" | sed -ne '$p')+1))
>>  		echo "# This is a combination of $COUNT commits."
>>  		sed -e 1d -e '2,/^./{
>> @@ -315,10 +321,26 @@ make_squash_message () {
>>  		echo
>>  		git cat-file commit HEAD | sed -e '1,/^$/d'
>>  	fi
>> -	echo
>> -	echo "# This is the $(nth_string $COUNT) commit message:"
>> -	echo
>> -	git cat-file commit $1 | sed -e '1,/^$/d'
>> +	case $1 in
>> +	squash)
>> +		echo
>> +		echo "# This is the $(nth_string $COUNT) commit message:"
>> +		echo
>> +		git cat-file commit $2 | sed -e '1,/^$/d'
>> +		;;
>> +	fixup)
>> +		echo
>> +		echo "# The $(nth_string $COUNT) commit message will be skipped:"
>> +		echo
>> +		# Comment the lines of the commit message out using
>> +		# "#" rather than "# " (a) to make them more distinct
>> +		# from the explanatory comments added by this function
>> +		# and (b) to make it less likely that the sed regexp
>> +		# above will be confused by a commented-out commit
>> +		# message.
> 
> Use "#  " as prefix and you won't have to worry about a line in the log
> message that begins with " Th", no?

The scenario seems pretty unlikely to me.  The line would not only
have to start with " Th" but also match the rest of the regexp.  And
as far as I can see, the only ill effect of a mismatch would be to
throw off COUNT, which itself is only used in the instruction
comments.

By the way, if you are really worried about accidental matches to the
regexp, this is not the end of the story.  It could be that a squashed
commit's log message has lines that match the regexp; e.g.,

    commit -m '# This is my 380th commit message today:'

The commented-out line survives this first commit and would confuse an
interactive squash (with or without my patch).

Amusingly, if you want to indicate at commit time that a commit
message should be omitted at rebase time, you can add a comment
character intentionally:

    commit -m '# this comment will be stripped out at the next squash'

This peculiarity also has nothing to do with the new "fixup" feature.

> In any case, I agree that a comment like this is necessary to warn anybody
> who will be touching the code that the COUNT=$((...))  needs to avoid
> matching what is produced here, but I find the above 6-line comment a bit
> too excessive.

I have substituted a terser alternative.  Feel free to edit the
comment or delete it altogether.

Michael


Michael Haggerty (2):
  t3404: Use test_commit to set up test repository
  Add a command "fixup" to rebase --interactive

 Documentation/git-rebase.txt  |   13 +++--
 git-rebase--interactive.sh    |   45 +++++++++++++++----
 t/lib-rebase.sh               |    7 ++-
 t/t3404-rebase-interactive.sh |   96 +++++++++++++++++++++-------------------
 4 files changed, 97 insertions(+), 64 deletions(-)

^ permalink raw reply

* [PATCHv3 1/2] t3404: Use test_commit to set up test repository
From: Michael Haggerty @ 2009-12-07  9:20 UTC (permalink / raw)
  To: git; +Cc: gitster, git, Johannes.Schindelin, bgustavsson, Michael Haggerty
In-Reply-To: <cover.1260177312.git.mhagger@alum.mit.edu>

Also adjust "expected" text to reflect the file contents generated by
test_commit, which are slightly different than those generated by the
old code.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 t/t3404-rebase-interactive.sh |   66 ++++++++++++----------------------------
 1 files changed, 20 insertions(+), 46 deletions(-)

diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 3a37793..778daf4 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -16,53 +16,26 @@ set_fake_editor
 
 # set up two branches like this:
 #
-# A - B - C - D - E
+# A - B - C - D - E     (master)
 #   \
-#     F - G - H
+#     F - G - H         (branch1)
 #       \
-#         I
+#         I             (branch2)
 #
-# where B, D and G touch the same file.
+# where A, B, D and G touch the same file.
 
 test_expect_success 'setup' '
-	: > file1 &&
-	git add file1 &&
-	test_tick &&
-	git commit -m A &&
-	git tag A &&
-	echo 1 > file1 &&
-	test_tick &&
-	git commit -m B file1 &&
-	: > file2 &&
-	git add file2 &&
-	test_tick &&
-	git commit -m C &&
-	echo 2 > file1 &&
-	test_tick &&
-	git commit -m D file1 &&
-	: > file3 &&
-	git add file3 &&
-	test_tick &&
-	git commit -m E &&
+	test_commit A file1 &&
+	test_commit B file1 &&
+	test_commit C file2 &&
+	test_commit D file1 &&
+	test_commit E file3 &&
 	git checkout -b branch1 A &&
-	: > file4 &&
-	git add file4 &&
-	test_tick &&
-	git commit -m F &&
-	git tag F &&
-	echo 3 > file1 &&
-	test_tick &&
-	git commit -m G file1 &&
-	: > file5 &&
-	git add file5 &&
-	test_tick &&
-	git commit -m H &&
+	test_commit F file4 &&
+	test_commit G file1 &&
+	test_commit H file5 &&
 	git checkout -b branch2 F &&
-	: > file6 &&
-	git add file6 &&
-	test_tick &&
-	git commit -m I &&
-	git tag I
+	test_commit I file6
 '
 
 test_expect_success 'no changes are a nop' '
@@ -111,19 +84,20 @@ test_expect_success 'exchange two commits' '
 
 cat > expect << EOF
 diff --git a/file1 b/file1
-index e69de29..00750ed 100644
+index f70f10e..fd79235 100644
 --- a/file1
 +++ b/file1
-@@ -0,0 +1 @@
-+3
+@@ -1 +1 @@
+-A
++G
 EOF
 
 cat > expect2 << EOF
 <<<<<<< HEAD
-2
+D
 =======
-3
->>>>>>> b7ca976... G
+G
+>>>>>>> 91201e5... G
 EOF
 
 test_expect_success 'stop on conflicting pick' '
-- 
1.6.5.5

^ permalink raw reply related

* [PATCHv3 2/2] Add a command "fixup" to rebase --interactive
From: Michael Haggerty @ 2009-12-07  9:20 UTC (permalink / raw)
  To: git; +Cc: gitster, git, Johannes.Schindelin, bgustavsson, Michael Haggerty
In-Reply-To: <cover.1260177312.git.mhagger@alum.mit.edu>

The command is like "squash", except that it discards the commit message
of the corresponding commit.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 Documentation/git-rebase.txt  |   13 +++++++----
 git-rebase--interactive.sh    |   45 +++++++++++++++++++++++++++++++---------
 t/lib-rebase.sh               |    7 +++--
 t/t3404-rebase-interactive.sh |   30 +++++++++++++++++++++++++++
 4 files changed, 77 insertions(+), 18 deletions(-)

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index ca5e1e8..9b648ec 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -382,9 +382,12 @@ If you just want to edit the commit message for a commit, replace the
 command "pick" with the command "reword".
 
 If you want to fold two or more commits into one, replace the command
-"pick" with "squash" for the second and subsequent commit.  If the
-commits had different authors, it will attribute the squashed commit to
-the author of the first commit.
+"pick" for the second and subsequent commits with "squash" or "fixup".
+If the commits had different authors, the folded commit will be
+attributed to the author of the first commit.  The suggested commit
+message for the folded commit is the concatenation of the commit
+messages of the first commit and of those with the "squash" command,
+but omits the commit messages of commits with the "fixup" command.
 
 'git-rebase' will stop when "pick" has been replaced with "edit" or
 when a command fails due to merge errors. When you are done editing
@@ -512,8 +515,8 @@ Easy case: The changes are literally the same.::
 Hard case: The changes are not the same.::
 
 	This happens if the 'subsystem' rebase had conflicts, or used
-	`\--interactive` to omit, edit, or squash commits; or if the
-	upstream used one of `commit \--amend`, `reset`, or
+	`\--interactive` to omit, edit, squash, or fixup commits; or
+	if the upstream used one of `commit \--amend`, `reset`, or
 	`filter-branch`.
 
 
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 0bd3bf7..cd2dce1 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -302,7 +302,10 @@ nth_string () {
 
 make_squash_message () {
 	if test -f "$SQUASH_MSG"; then
-		COUNT=$(($(sed -n "s/^# This is [^0-9]*\([1-9][0-9]*\).*/\1/p" \
+		# We want to be careful about matching only the commit
+		# message comment lines generated by this function.
+		# "[snrt][tdh]" matches the nth_string endings.
+		COUNT=$(($(sed -n "s/^# Th[^0-9]*\([1-9][0-9]*\)[snrt][tdh] commit message.*:/\1/p" \
 			< "$SQUASH_MSG" | sed -ne '$p')+1))
 		echo "# This is a combination of $COUNT commits."
 		sed -e 1d -e '2,/^./{
@@ -315,10 +318,23 @@ make_squash_message () {
 		echo
 		git cat-file commit HEAD | sed -e '1,/^$/d'
 	fi
-	echo
-	echo "# This is the $(nth_string $COUNT) commit message:"
-	echo
-	git cat-file commit $1 | sed -e '1,/^$/d'
+	case $1 in
+	squash)
+		echo
+		echo "# This is the $(nth_string $COUNT) commit message:"
+		echo
+		git cat-file commit $2 | sed -e '1,/^$/d'
+		;;
+	fixup)
+		echo
+		echo "# The $(nth_string $COUNT) commit message will be skipped:"
+		echo
+		# Comment the lines of the commit message out using
+		# "#" rather than "# " to make them less likely to
+		# confuse the sed regexp above.
+		git cat-file commit $2 | sed -e '1,/^$/d' -e 's/^/#/'
+		;;
+	esac
 }
 
 peek_next_command () {
@@ -367,20 +383,28 @@ do_next () {
 		warn
 		exit 0
 		;;
-	squash|s)
-		comment_for_reflog squash
+	squash|s|fixup|f)
+		case "$command" in
+		squash|s)
+			squash_style=squash
+			;;
+		fixup|f)
+			squash_style=fixup
+			;;
+		esac
+		comment_for_reflog $squash_style
 
 		test -f "$DONE" && has_action "$DONE" ||
-			die "Cannot 'squash' without a previous commit"
+			die "Cannot '$squash_style' without a previous commit"
 
 		mark_action_done
-		make_squash_message $sha1 > "$MSG"
+		make_squash_message $squash_style $sha1 > "$MSG"
 		failed=f
 		author_script=$(get_author_ident_from_commit HEAD)
 		output git reset --soft HEAD^
 		pick_one -n $sha1 || failed=t
 		case "$(peek_next_command)" in
-		squash|s)
+		squash|s|fixup|f)
 			USE_OUTPUT=output
 			MSG_OPT=-F
 			EDIT_OR_FILE="$MSG"
@@ -768,6 +792,7 @@ first and then run 'git rebase --continue' again."
 #  r, reword = use commit, but edit the commit message
 #  e, edit = use commit, but stop for amending
 #  s, squash = use commit, but meld into previous commit
+#  f, fixup = like "squash", but discard this commit's log message
 #
 # If you remove a line here THAT COMMIT WILL BE LOST.
 # However, if you remove everything, the rebase will be aborted.
diff --git a/t/lib-rebase.sh b/t/lib-rebase.sh
index 62f452c..f4dda02 100644
--- a/t/lib-rebase.sh
+++ b/t/lib-rebase.sh
@@ -9,8 +9,9 @@
 #
 #	"[<lineno1>] [<lineno2>]..."
 #
-#   If a line number is prefixed with "squash", "edit", or "reword", the
-#   respective line's command will be replaced with the specified one.
+#   If a line number is prefixed with "squash", "fixup", "edit", or
+#   "reword", the respective line's command will be replaced with the
+#   specified one.
 
 set_fake_editor () {
 	echo "#!$SHELL_PATH" >fake-editor.sh
@@ -32,7 +33,7 @@ cat "$1".tmp
 action=pick
 for line in $FAKE_LINES; do
 	case $line in
-	squash|edit|reword)
+	squash|fixup|edit|reword)
 		action="$line";;
 	*)
 		echo sed -n "${line}s/^pick/$action/p"
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 778daf4..ea26115 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -235,6 +235,36 @@ test_expect_success 'multi-squash only fires up editor once' '
 	test 1 = $(git show | grep ONCE | wc -l)
 '
 
+test_expect_success 'multi-fixup only fires up editor once' '
+	git checkout -b multi-fixup E &&
+	base=$(git rev-parse HEAD~4) &&
+	FAKE_COMMIT_AMEND="ONCE" FAKE_LINES="1 fixup 2 fixup 3 fixup 4" \
+		git rebase -i $base &&
+	test $base = $(git rev-parse HEAD^) &&
+	test 1 = $(git show | grep ONCE | wc -l) &&
+	git checkout to-be-rebased &&
+	git branch -D multi-fixup
+'
+
+cat > expect-squash-fixup << EOF
+B
+
+D
+
+ONCE
+EOF
+
+test_expect_success 'squash and fixup generate correct log messages' '
+	git checkout -b squash-fixup E &&
+	base=$(git rev-parse HEAD~4) &&
+	FAKE_COMMIT_AMEND="ONCE" FAKE_LINES="1 fixup 2 squash 3 fixup 4" \
+		git rebase -i $base &&
+	git cat-file commit HEAD | sed -e 1,/^\$/d > actual-squash-fixup &&
+	test_cmp expect-squash-fixup actual-squash-fixup &&
+	git checkout to-be-rebased &&
+	git branch -D squash-fixup
+'
+
 test_expect_success 'squash works as expected' '
 	for n in one two three four
 	do
-- 
1.6.5.5

^ permalink raw reply related

* Re: [PATCH v2] Detailed diagnosis when parsing an object name fails.
From: Matthieu Moy @ 2009-12-07 10:07 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git
In-Reply-To: <7vk4wz9rbt.fsf@alter.siamese.dyndns.org>

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

> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
>
> The second round still had a few compilation warnings (turned to errors in
> my environment) so I fixed them up somewhat.

Thanks. For some reasons, I had CFLAGS=-g and I didn't see the
warnings :-(

> I stopped before getting a
> clean compile, though---you will still get:
>
>   sha1_name.c: In function 'get_sha1_with_mode_1':
>   sha1_name.c:956: error: 'object_name' may be used uninitialized in this function

This one is strange. I don't git it with gcc 4.4, even with -Wall
-Werror. And indeed, object_name is initialized when !gently, which is
precisely when we use it.

I did

-               char *object_name;
+               char *object_name = NULL;

to make sure the warning goes away on your machine, but I don't
understand what's going on.

>  cache.h     |    4 ++--
>  sha1_name.c |    2 +-

Squashed into v3, together with initialisation of object_name, and two
s/int/unsigned/ to please gcc -Wextra.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

^ permalink raw reply

* [PATCH v3] Detailed diagnosis when parsing an object name fails.
From: Matthieu Moy @ 2009-12-07 10:10 UTC (permalink / raw)
  To: git, gitster; +Cc: Matthieu Moy
In-Reply-To: <vpqws0zcd1c.fsf@bauges.imag.fr>

The previous error message was the same in many situations (unknown
revision or path not in the working tree). We try to help the user as
much as possible to understand the error, especially with the
sha1:filename notation. In this case, we say whether the sha1 or the
filename is problematic, and diagnose the confusion between
relative-to-root and relative-to-$PWD confusion precisely.

The 7 new error messages are tested.

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
 cache.h                        |    6 ++-
 setup.c                        |   15 +++++-
 sha1_name.c                    |  115 ++++++++++++++++++++++++++++++++++++++--
 t/t1506-rev-parse-diagnosis.sh |   69 ++++++++++++++++++++++++
 4 files changed, 198 insertions(+), 7 deletions(-)
 create mode 100755 t/t1506-rev-parse-diagnosis.sh

diff --git a/cache.h b/cache.h
index 0e69384..c122bfa 100644
--- a/cache.h
+++ b/cache.h
@@ -708,7 +708,11 @@ static inline unsigned int hexval(unsigned char c)
 #define DEFAULT_ABBREV 7
 
 extern int get_sha1(const char *str, unsigned char *sha1);
-extern int get_sha1_with_mode(const char *str, unsigned char *sha1, unsigned *mode);
+extern int get_sha1_with_mode_1(const char *str, unsigned char *sha1, unsigned *mode, int gently, const char *prefix);
+static inline int get_sha1_with_mode(const char *str, unsigned char *sha1, unsigned *mode)
+{
+	return get_sha1_with_mode_1(str, sha1, mode, 1, NULL);
+}
 extern int get_sha1_hex(const char *hex, unsigned char *sha1);
 extern char *sha1_to_hex(const unsigned char *sha1);	/* static buffer result! */
 extern int read_ref(const char *filename, unsigned char *sha1);
diff --git a/setup.c b/setup.c
index f67250b..5792eb7 100644
--- a/setup.c
+++ b/setup.c
@@ -74,6 +74,18 @@ int check_filename(const char *prefix, const char *arg)
 	die_errno("failed to stat '%s'", arg);
 }
 
+static void NORETURN die_verify_filename(const char *prefix, const char *arg)
+{
+	unsigned char sha1[20];
+	unsigned mode;
+	/* try a detailed diagnostic ... */
+	get_sha1_with_mode_1(arg, sha1, &mode, 0, prefix);
+	/* ... or fall back the most general message. */
+	die("ambiguous argument '%s': unknown revision or path not in the working tree.\n"
+	    "Use '--' to separate paths from revisions", arg);
+
+}
+
 /*
  * Verify a filename that we got as an argument for a pathspec
  * entry. Note that a filename that begins with "-" never verifies
@@ -87,8 +99,7 @@ void verify_filename(const char *prefix, const char *arg)
 		die("bad flag '%s' used after filename", arg);
 	if (check_filename(prefix, arg))
 		return;
-	die("ambiguous argument '%s': unknown revision or path not in the working tree.\n"
-	    "Use '--' to separate paths from revisions", arg);
+	die_verify_filename(prefix, arg);
 }
 
 /*
diff --git a/sha1_name.c b/sha1_name.c
index 44bb62d..ca8f9db 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -804,7 +804,96 @@ int get_sha1(const char *name, unsigned char *sha1)
 	return get_sha1_with_mode(name, sha1, &unused);
 }
 
-int get_sha1_with_mode(const char *name, unsigned char *sha1, unsigned *mode)
+/* Must be called only when object_name:filename doesn't exist. */
+static void diagnose_invalid_sha1_path(const char *prefix,
+				       const char *filename,
+				       const unsigned char *tree_sha1,
+				       const char *object_name)
+{
+	struct stat st;
+	unsigned char sha1[20];
+	unsigned mode;
+
+	if (!prefix)
+		prefix = "";
+
+	if (!lstat(filename, &st))
+		die("Path '%s' exists on disk, but not in '%s'.",
+		    filename, object_name);
+	if (errno == ENOENT || errno == ENOTDIR) {
+		char *fullname = xmalloc(strlen(filename)
+					     + strlen(prefix) + 1);
+		strcpy(fullname, prefix);
+		strcat(fullname, filename);
+
+		if (!get_tree_entry(tree_sha1, fullname,
+				    sha1, &mode)) {
+			die("Path '%s' exists, but not '%s'.\n"
+			    "Did you mean '%s:%s'?",
+			    fullname,
+			    filename,
+			    object_name,
+			    fullname);
+		}
+		die("Path '%s' does not exist in '%s'",
+		    filename, object_name);
+	}
+}
+
+/* Must be called only when :stage:filename doesn't exist. */
+static void diagnose_invalid_index_path(int stage,
+					const char *prefix,
+					const char *filename)
+{
+	struct stat st;
+	struct cache_entry *ce;
+	int pos;
+	unsigned namelen = strlen(filename);
+	unsigned fullnamelen;
+	char *fullname;
+
+	if (!prefix)
+		prefix = "";
+
+	/* Wrong stage number? */
+	pos = cache_name_pos(filename, namelen);
+	if (pos < 0)
+		pos = -pos - 1;
+	ce = active_cache[pos];
+	if (ce_namelen(ce) == namelen &&
+	    !memcmp(ce->name, filename, namelen))
+		die("Path '%s' is in the index, but not at stage %d.\n"
+		    "Did you mean ':%d:%s'?",
+		    filename, stage,
+		    ce_stage(ce), filename);
+
+	/* Confusion between relative and absolute filenames? */
+	fullnamelen = namelen + strlen(prefix);
+	fullname = xmalloc(fullnamelen + 1);
+	strcpy(fullname, prefix);
+	strcat(fullname, filename);
+	pos = cache_name_pos(fullname, fullnamelen);
+	if (pos < 0)
+		pos = -pos - 1;
+	ce = active_cache[pos];
+	if (ce_namelen(ce) == fullnamelen &&
+	    !memcmp(ce->name, fullname, fullnamelen))
+		die("Path '%s' is in the index, but not '%s'.\n"
+		    "Did you mean ':%d:%s'?",
+		    fullname, filename,
+		    ce_stage(ce), fullname);
+
+	if (!lstat(filename, &st))
+		die("Path '%s' exists on disk, but not in the index.", filename);
+	if (errno == ENOENT || errno == ENOTDIR)
+		die("Path '%s' does not exist (neither on disk nor in the index).",
+		    filename);
+
+	free(fullname);
+}
+
+
+int get_sha1_with_mode_1(const char *name, unsigned char *sha1, unsigned *mode, int gently, const char *prefix)
 {
 	int ret, bracket_depth;
 	int namelen = strlen(name);
@@ -850,6 +939,8 @@ int get_sha1_with_mode(const char *name, unsigned char *sha1, unsigned *mode)
 			}
 			pos++;
 		}
+		if (!gently)
+			diagnose_invalid_index_path(stage, prefix, cp);
 		return -1;
 	}
 	for (cp = name, bracket_depth = 0; *cp; cp++) {
@@ -862,9 +953,25 @@ int get_sha1_with_mode(const char *name, unsigned char *sha1, unsigned *mode)
 	}
 	if (*cp == ':') {
 		unsigned char tree_sha1[20];
-		if (!get_sha1_1(name, cp-name, tree_sha1))
-			return get_tree_entry(tree_sha1, cp+1, sha1,
-					      mode);
+		char *object_name = NULL;
+		if (!gently) {
+			object_name = xmalloc(cp-name+1);
+			strncpy(object_name, name, cp-name);
+			object_name[cp-name] = '\0';
+		}
+		if (!get_sha1_1(name, cp-name, tree_sha1)) {
+			const char *filename = cp+1;
+			ret = get_tree_entry(tree_sha1, filename, sha1, mode);
+			if (!gently) {
+				diagnose_invalid_sha1_path(prefix, filename,
+							   tree_sha1, object_name);
+				free(object_name);
+			}
+			return ret;
+		} else {
+			if (!gently)
+				die("Invalid object name '%s'.", object_name);
+		}
 	}
 	return ret;
 }
diff --git a/t/t1506-rev-parse-diagnosis.sh b/t/t1506-rev-parse-diagnosis.sh
new file mode 100755
index 0000000..af721f9
--- /dev/null
+++ b/t/t1506-rev-parse-diagnosis.sh
@@ -0,0 +1,69 @@
+#!/bin/sh
+
+test_description='test git rev-parse diagnosis for invalid argument'
+
+exec </dev/null
+
+. ./test-lib.sh
+
+HASH_file=
+
+test_expect_success 'set up basic repo' '
+	echo one > file.txt &&
+	mkdir subdir &&
+	echo two > subdir/file.txt &&
+	echo three > subdir/file2.txt &&
+	git add . &&
+	git commit -m init &&
+	echo four > index-only.txt &&
+	git add index-only.txt &&
+	echo five > disk-only.txt
+'
+
+test_expect_success 'correct file objects' '
+	HASH_file=$(git rev-parse HEAD:file.txt) &&
+	git rev-parse HEAD:subdir/file.txt &&
+	git rev-parse :index-only.txt &&
+	(cd subdir &&
+	 git rev-parse HEAD:subdir/file2.txt &&
+	 test $HASH_file = $(git rev-parse HEAD:file.txt) &&
+	 test $HASH_file = $(git rev-parse :file.txt) &&
+	 test $HASH_file = $(git rev-parse :0:file.txt) )
+'
+
+test_expect_success 'incorrect revision id' '
+	test_must_fail git rev-parse foobar:file.txt 2>error &&
+	grep "Invalid object name '"'"'foobar'"'"'." error &&
+	test_must_fail git rev-parse foobar 2> error &&
+	grep "unknown revision or path not in the working tree." error
+'
+
+test_expect_success 'incorrect file in sha1:path' '
+	test_must_fail git rev-parse HEAD:nothing.txt 2> error &&
+	grep "fatal: Path '"'"'nothing.txt'"'"' does not exist in '"'"'HEAD'"'"'" error &&
+	test_must_fail git rev-parse HEAD:index-only.txt 2> error &&
+	grep "fatal: Path '"'"'index-only.txt'"'"' exists on disk, but not in '"'"'HEAD'"'"'." error &&
+	(cd subdir &&
+	 test_must_fail git rev-parse HEAD:file2.txt 2> error &&
+	 grep "Did you mean '"'"'HEAD:subdir/file2.txt'"'"'?" error )
+'
+
+test_expect_success 'incorrect file in :path and :N:path' '
+	test_must_fail git rev-parse :nothing.txt 2> error &&
+	grep "fatal: Path '"'"'nothing.txt'"'"' does not exist (neither on disk nor in the index)." error &&
+	test_must_fail git rev-parse :1:nothing.txt 2> error &&
+	grep "Path '"'"'nothing.txt'"'"' does not exist (neither on disk nor in the index)." error &&
+	test_must_fail git rev-parse :1:file.txt 2> error &&
+	grep "Did you mean '"'"':0:file.txt'"'"'?" error &&
+	(cd subdir &&
+	 test_must_fail git rev-parse :1:file.txt 2> error &&
+	 grep "Did you mean '"'"':0:file.txt'"'"'?" error &&
+	 test_must_fail git rev-parse :file2.txt 2> error &&
+	 grep "Did you mean '"'"':0:subdir/file2.txt'"'"'?" error &&
+	 test_must_fail git rev-parse :2:file2.txt 2> error &&
+	 grep "Did you mean '"'"':0:subdir/file2.txt'"'"'?" error) &&
+	test_must_fail git rev-parse :disk-only.txt 2> error &&
+	grep "fatal: Path '"'"'disk-only.txt'"'"' exists on disk, but not in the index." error
+'
+
+test_done
-- 
1.6.6.rc0.355.gcb53a.dirty

^ permalink raw reply related


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