Git development
 help / color / mirror / Atom feed
* Re: [PATCH] preserve mtime of local clone
From: Junio C Hamano @ 2009-09-12  5:09 UTC (permalink / raw)
  To: Clemens Buchacher; +Cc: git, msysgit, Junio C Hamano, Shawn O. Pearce
In-Reply-To: <20090909195158.GA12968@localhost>


Clemens Buchacher <drizzd@aon.at> writes:

> +int copy_times(int ofd, int ifd)
> +{
> +	struct stat st;
> +	struct timespec times[2];
> +	if (fstat(ifd, &st))
> +		return -1;
> +	times[0].tv_nsec = UTIME_OMIT;
> +	times[1].tv_sec = st.st_mtime;
> +	times[1].tv_nsec = ST_MTIME_NSEC(st);
> +	return futimens(ofd, times);
> +}

Hmm, futimens() is relatively new.  Are minority platforms folks Ok with
this patch?

At least SunOS 5.11 (OpenSolaris 0811) seems to barf on UTIME_OMIT.

^ permalink raw reply

* Re: What's cooking in git.git (Sep 2009, #02; Mon, 07)
From: Junio C Hamano @ 2009-09-12  4:38 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: git, Shawn O. Pearce, Sverre Rabbelier, Jeff King,
	Johannes Schindelin, Daniel Barkalow, Jan Engelhardt,
	Johan Herland, Linus Torvalds
In-Reply-To: <7veiqdngs0.fsf@alter.siamese.dyndns.org>

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

> * jk/unwanted-advices (2009-09-06) 2 commits
>  - status: make "how to stage" messages optional
>  - push: make non-fast-forward help message configurable
>
> If I missed the v2 or later that uses 'advice.*" instead, I am sorry, and
> please let me know.

Found them; thanks.

^ permalink raw reply

* Re: [PATCH 1/2] start_command: do not clobber cmd->env on Windows code path
From: Junio C Hamano @ 2009-09-12  4:32 UTC (permalink / raw)
  To: Johannes Sixt; +Cc: msysgit, Alexey Borzenkov, git
In-Reply-To: <200909111940.08652.j6t@kdbg.org>


Thanks; both patches applied.

^ permalink raw reply

* [PATCH v2] Re: add documentation for mailinfo.scissors and '--no-scissors'
From: Nicolas Sebrecht @ 2009-09-12  3:03 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Nicolas Sebrecht, Nanako Shiraishi, git, Pierre Habouzit
In-Reply-To: <7v8wglw60x.fsf@alter.siamese.dyndns.org>

[ Thank you (again) for this very good explanation. ]

The 11/09/09, Junio C Hamano wrote:
> Nicolas Sebrecht <nicolas.s.dev@gmx.fr> writes:
> 
> > Ok. So, the fact that the usage of git-am doesn't tell about
> > --no-scissors is the expected behaviour?
> 
> You _could_ argue that we _could_ describe a long option "frotz" that
> lacks the '!' flag in OPTIONS_SPEC as "--[no-]frotz" in the output by
> changing the rev-parse --parseopt, if you really want to.
> 
> However, I think that is not done deliberately to avoid cluttering the
> output.  I Cc'ed the primary guilty party ;-) of the parse-options
> infrastructure.

Well, if it is expected to not have the "--[no-]frotz" in usage where
applicable I'll be fine with that (even if it may sounds a bit odd for a
sane user). Otherwise, I believe it could be a (small) improvement for
the UI.

> Currently, non-bool options are not marked with '!'.  Nobody sane would
> say "git am --no-directory foo", but "rev-parse --parseopt" acccepts such
> a nonsense input, and it is up to the calling script to catch it and barf.
> But "rev-parse --parseopt" will start saying "--[no-]directory=" with such
> a change, which is not good.
> 
> And --no-scissors is not that special.  We could add --no-signoff to say
> "I do not want to sign-off this one time" explicitly, and it is crazy if
> we had to add another line in OPTIONS_SPEC when we want to do so, when it
> is clear "signoff" option is a boolean.
> 
> As a long term direction, I'd rather not to see "no-" in OPTIONS_SPEC, but
> have that taken care of by "rev-parse --parseopt" to keep our sanity.  The
> only existing offender is "no-verify" in "rebase -i".  Let's solve it (if
> there is anything to solve, which I doubt) without adding new ones.

Now (with all this background in mind), I agree that the "no-" in
OPTIONS_SPEC looks ugly.

<If there were something to change>

As you say, we can't blindly rely on the "is a boolean" and "option name
begin with 'no-'" things altogether. Perhaps a new magic character
('-'?) beside the current flags of PARSEOPT could smartly do the trick?

</>

Pierre, opinion?

-- 
Nicolas Sebrecht

^ permalink raw reply

* Re: one half of a rebase
From: Geoffrey Irving @ 2009-09-12  2:23 UTC (permalink / raw)
  To: Alex Riesen; +Cc: git, Dylan Simon
In-Reply-To: <81b0412b0909111410k3f3ebfaco393bb37ff5a6b5c1@mail.gmail.com>

On Fri, Sep 11, 2009 at 5:10 PM, Alex Riesen<raa.lkml@gmail.com> wrote:
> On Fri, Sep 11, 2009 at 19:25, Geoffrey Irving <irving@naml.us> wrote:
>> If I could do (2) as a separate operation, it would look something like
>>
>>    git cherry-pick-all topic
>>
>> which is simpler and faster since it avoids switching files back and
>> forth (master to topic and back).  Is there a robust way to achieve
>> the cherry-pick-all semantics with current commands?  If not, how
>> difficult would it be to partition rebase accordingly?
>
> I have this in my .bashrc:
>
> $ gcp3 ()
> {
>    git format-patch -k --stdout --full-index "$@" | git am -k -3 --binary
> }
>
> Then, while on master branch:
>
> $ gcp3 master..topic

Great!  That should work nicely.

Geoffrey

^ permalink raw reply

* Re: What's cooking in git.git (Sep 2009, #02; Mon, 07)
From: Junio C Hamano @ 2009-09-12  0:33 UTC (permalink / raw)
  To: git
  Cc: Shawn O. Pearce, Sverre Rabbelier, Jeff King, Johannes Schindelin,
	Daniel Barkalow, Jan Engelhardt, Johan Herland, Linus Torvalds
In-Reply-To: <7vtyzexnhm.fsf@alter.siamese.dyndns.org>

This is an abbreviated preview of issue #03 for this month; the message
primarily covers potential 1.6.5 material and omits topics without much
urgency for the purpose of pushing 1.6.5-rc1 out.

--------------------------------------------------
[New Topics]

* jc/merge-saner-messages (2009-09-07) 1 commit
  (merged to 'next' on 2009-09-11 at 4efab98)
 + merge-recursive: give less scary messages when merge did not start

I think this should be safe for 1.6.5.

* rc/maint-http-no-head-pack-check (2009-09-09) 1 commit.
 - http.c: remove verification of remote packs

This is the response to infamous "github sometimes gives status 500
error for a HEAD request even when GET request for the same URL happily
serves the content error-free." problem.

I am hoping this is safe for 1.6.5, but will wait for a response to my
inquiry earlier.

--------------------------------------------------
[Stalled]

* jh/notes (2009-08-27) 12 commits.

The reroll was discussed on the list; unfortunately it did not quite apply
to any of my branches.  I do not think this has to be in 1.6.5, but I just
want to get us unstuck if Johan was waiting for me to apply the current
series before making his next move.

* je/send-email-no-subject (2009-08-05) 1 commit
  (merged to 'next' on 2009-08-30 at b6455c2)
 + send-email: confirm on empty mail subjects

It would be a good change for 1.6.5 when done properly, but this does not
honor --confirm=never but talks to the terminal.  Nor it honors any
existing sendemail.confirm configuration settings.

--------------------------------------------------
[Cooking]

* db/vcs-helper (2009-09-03) 16 commits
 - ...
  (merged to 'next' on 2009-09-11 at a275aa3)
 + Use a clearer style to issue commands to remote helpers
 + Make the "traditionally-supported" URLs a special case
  (merged to 'next' on 2009-08-07 at f3533ba)
 + Makefile: install hardlinks for git-remote-<scheme> supported by libcurl if possible
 + Makefile: do not link three copies of git-remote-* programs
 + Makefile: git-http-fetch does not need expat
  (merged to 'next' on 2009-08-06 at 15da79d)
 + http-fetch: Fix Makefile dependancies
 + Add transport native helper executables to .gitignore
  (merged to 'next' on 2009-08-05 at 33d491e)
 + git-http-fetch: not a builtin
 + Use an external program to implement fetching with curl
 + Add support for external programs for handling native fetches
 (this branch is used by jh/cvs-helper.)

Up to the part that eject -lcurl from the main "git" binary cleanly are
now in 'next' and is ready for 1.6.5.

* cb/maint-1.6.3-grep-relative-up (2009-09-05) 2 commits.
  (merged to 'next' on 2009-09-07 at f9b5b48)
 + grep: accept relative paths outside current working directory
 + grep: fix exit status if external_grep() punts

1.6.5 material.

* jk/unwanted-advices (2009-09-06) 2 commits
 - status: make "how to stage" messages optional
 - push: make non-fast-forward help message configurable

If I missed the v2 or later that uses 'advice.*" instead, I am sorry, and
please let me know.  At least the first one should be in 1.6.5, I think,
and probably both.

* jt/pushinsteadof (2009-09-07) 2 commits
  (merged to 'next' on 2009-09-11 at cf3eb57)
 + Add url.<base>.pushInsteadOf: URL rewriting for push only
 + Wrap rewrite globals in a struct in preparation for adding another set

Probably 1.6.5 material.

* pk/fast-import-tars (2009-09-03) 1 commit
  (merged to 'next' on 2009-09-07 at 8fbf027)
 + import-tars: Allow per-tar author and commit message.

* pk/fast-import-dirs (2009-09-03) 1 commit
  (merged to 'next' on 2009-09-07 at 836cba2)
 + Add script for importing bits-and-pieces to Git.

I have to wonder if there are standard libraries to do this sort of thing
without having to hand-roll these logic every time, but I decided not to
be picky about contrib/ material.  Both will be in 1.6.5

* jc/maint-1.6.0-blank-at-eof (2009-09-05) 10 commits.
  (merged to 'next' on 2009-09-07 at 165dc3c)

Possibly a 1.6.5 material, but it started rather late in the cycle so we
might want to cook it for a while in 'next' and do 1.6.5 without it.

* sr/gfi-options (2009-09-06) 6 commits
  (merged to 'next' on 2009-09-07 at 5f6b0ff)
 + fast-import: test the new option command
 + fast-import: add option command
 + fast-import: test the new feature command
 + fast-import: add feature command
 + fast-import: put marks reading in it's own function
 + fast-import: put option parsing code in separate functions

Perhaps 1.6.5 material but I wasn't sure.  I saw Sverre asking Shawn for
opinion but I may have missed the response.

* lt/maint-traverse-trees-fix (2009-09-06) 1 commit.
 - Prepare 'traverse_trees()' for D/F conflict lookahead

Beginning of the fix to a rather nasty longstanding issue of merging trees
with ("a" "a-b"), ("a/b" "a-b") and just ("a-b"), but my reading of it is
that it is just the first step to demonstrate one-entry lookahead and not
a full solution yet.

This is a good problem to tackle but the bug has been there from the
beginning of unpack_trees(), i.e. lacks urgency, and the full solution is
expected to be much larger.  I am tempted to say that 1.6.5 shouldn't wait
for this series.

^ permalink raw reply

* Re: [PATCHv5 00/14] git notes
From: Junio C Hamano @ 2009-09-12  0:11 UTC (permalink / raw)
  To: Johan Herland
  Cc: git, Johannes.Schindelin, trast, tavestbo, git, chriscool,
	spearce
In-Reply-To: <1252376822-6138-1-git-send-email-johan@herland.net>

Johan Herland <johan@herland.net> writes:

> Yet another iteration of the 'git notes' feature. Rebased on top of 'next':

By the way I didn't pick this up as it did not apply to any of my
branches.

^ permalink raw reply

* Re: previously committed file untracked prevent checking out an old  tag/branch
From: Junio C Hamano @ 2009-09-12  0:06 UTC (permalink / raw)
  To: Daniele Segato; +Cc: git
In-Reply-To: <9accb4400909110426v2a42086ema318167e94b2cbc1@mail.gmail.com>

Daniele Segato <daniele.bilug@gmail.com> writes:

> I only can think of doing a backup of my build.properties, delete it
> and check out the tag or I just delete it and rebuild
> it from the template later.

That is the right approach, but you could automate it further.

The project I help at work has a similar set-up.  The Makefile defines a
plain vanilla set of configuration via the Make variables, and they can be
overriden by including ./config.mk.  The master branch does not have the
file tracked, but ./config.mk-sample file is tracked and the developers
can use it as a template.

The developers can have an untracked (from the project's point of view)
subdirectory, local/. with local/myconfig.mk file.  "local" is in fact a
full fledged git repository with local/.git and changes to myconfig.mk can
be tracked there.  It is not (and should not be) a subproject.

Because what appear in the sample config.mk-sample are a bunch of Make
variable definitions, and in Make, later definition trumps the earlier
ones,  So the developers can do:

	$ cat config.mk-sample local/myconfig.mk >config.mk
        $ make

The first step can be in the Makefile; the above command line is for
illustration.

For branches that are used for specific customer release, we track their
config.mk file.  That means the developer would lose config.mk file if he
does this:

	$ git checkout master
	$ cat config.mk-sample local/myconfig.mk >config.mk
        $ make
	... do the usual work of developing and testing ...
        $ git checkout customer ;# may fail due to config.mk
        $ rm -f config.mk	;# so get rid of it
        $ git checkout customer ;# now it is ok
        $ make			;# for real release
        ... or perhaps to diagnose the issue at customer site  ...
        $ edit config.mk	;# minimally adjust for testing
        $ make
        $ test
	... done, and go back to the regular programming ...
        $ git checkout config.mk ;# get rid of local changes
        $ git checkout master

But because the real configuration is tracked in local/.git, there is no
information lossage.

The generic config.mk _could_ be written to include local/myconfig.mk if
exists, in order to help debugging or testing the customer branch (then
there is no need for "edit config.mk for testing and then revert" steps),
but we chose not to do so, as it risks the release/build engineer to
forget that he has funny customization in his local/myconfig.mk file and
screwing up the release by inadvertently including the customization.

HTH ;-)

^ permalink raw reply

* Re: [PATCH] send-email: confirm on empty mail subjects
From: Junio C Hamano @ 2009-09-11 23:27 UTC (permalink / raw)
  To: Jan Engelhardt; +Cc: git
In-Reply-To: <1249490994-23455-1-git-send-email-jengelh@medozas.de>

Jan Engelhardt <jengelh@medozas.de> writes:

> When the user forgot to enter a subject in a compose session,
> send-email will now inquire whether this is really intended, similar
> to what the Alpine MUA does when a subject is absent.
>
> Signed-off-by: Jan Engelhardt <jengelh@medozas.de>
> ---
>  git-send-email.perl |   25 ++++++++++++++++++++-----
>  1 files changed, 20 insertions(+), 5 deletions(-)

We have had this for quite a long time but I have two niggling worries,
one minor, another one showstopper from maintainability point of view.

 - With --confirm=never the program still seems to talk with the terminal.
   I think with --confirm=neber we should not ask but just fail.

 - This does not hook into the confirmation framework the program already
   has, and does not have any way to turn it off.

> diff --git a/git-send-email.perl b/git-send-email.perl
> index d508f83..7d56fba 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -553,11 +553,26 @@ EOT
>  	}
>  	close(C);
>  
> -	if ($annotate) {
> -		do_edit($compose_filename, @files);
> -	} else {
> -		do_edit($compose_filename);
> -	}
> +	my $re_edit = 0;
> +	do {
> +		if ($annotate) {
> +			do_edit($compose_filename, @files);
> +		} else {
> +			do_edit($compose_filename);
> +		}
> +
> +		open(C, "<", $compose_filename) ||
> +			die "Failed to open $compose_filename: $!";
> +		if (grep(/^Subject:\s*$/i, <C>)) {
> +			my $r = ask("No Subject, send anyway? ".
> +			            "([y]es|[n]o|[e]dit again): ",
> +			            valid_re => qr/^[yne]/i,
> +			            default => "n");
> +			$re_edit = lc(substr($r, 0, 1)) eq "e";
> +			exit(0) if lc(substr($r, 0, 1)) eq "n";
> +		}
> +		close C;
> +	} while ($re_edit);
>  
>  	open(C2,">",$compose_filename . ".final")
>  		or die "Failed to open $compose_filename.final : " . $!;
> -- 
> 1.6.4

^ permalink raw reply

* [PATCH] completion: Replace config --list with --get-regexp
From: Todd Zullinger @ 2009-09-11 23:23 UTC (permalink / raw)
  To: Jeff King; +Cc: Shawn O. Pearce, james bardin, git, Junio C Hamano
In-Reply-To: <20090911150934.GB977@coredump.intra.peff.net>

James Bardin noted that the completion spewed warnings when no git
config file is present.  This is likely a bug to be fixed in git config,
but it's also a good excuse to simplify the completion code by using the
--get-regexp option as Jeff King pointed out.

Signed-off-by: Todd Zullinger <tmz@pobox.com>
---

Jeff King wrote:
> On Fri, Sep 11, 2009 at 07:36:51AM -0700, Shawn O. Pearce wrote:
>
>>> instead of just using "git config --get-regexp 'remote\..*\.url'", which
>>> would be slightly more efficient, and also doesn't have this bug. ;)
>>
>> F'king oversight.  You are right, this should be --get-regexp.
>> There isn't a reason here, probably other than "I forgot about
>> --get-regexp when I wrote the original code".
>
> OK. I will leave a bash-completion patch for you (or somebody else
> interested) as I don't use it myself.

Something like this perhaps?  Perhaps the commit message could use
adjustment to reference your fix for 'config --list'?

 contrib/completion/git-completion.bash |   30 +++++++++---------------------
 1 files changed, 9 insertions(+), 21 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index bf688e1..d668fc9 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -318,13 +318,9 @@ __git_remotes ()
 		echo ${i#$d/remotes/}
 	done
 	[ "$ngoff" ] && shopt -u nullglob
-	for i in $(git --git-dir="$d" config --list); do
-		case "$i" in
-		remote.*.url=*)
-			i="${i#remote.}"
-			echo "${i/.url=*/}"
-			;;
-		esac
+	for i in $(git --git-dir="$d" config --get-regexp 'remote\..*\.url' 2>/dev/null); do
+		i="${i#remote.}"
+		echo "${i/.url*/}"
 	done
 }
 
@@ -605,13 +601,9 @@ __git_porcelain_commandlist="$(__git_porcelain_commands 2>/dev/null)"
 __git_aliases ()
 {
 	local i IFS=$'\n'
-	for i in $(git --git-dir="$(__gitdir)" config --list); do
-		case "$i" in
-		alias.*)
-			i="${i#alias.}"
-			echo "${i/=*/}"
-			;;
-		esac
+	for i in $(git --git-dir="$(__gitdir)" config --get-regexp "alias\..*" 2>/dev/null); do
+		i="${i#alias.}"
+		echo "${i/ */}"
 	done
 }
 
@@ -1769,13 +1761,9 @@ _git_remote ()
 		;;
 	update)
 		local i c='' IFS=$'\n'
-		for i in $(git --git-dir="$(__gitdir)" config --list); do
-			case "$i" in
-			remotes.*)
-				i="${i#remotes.}"
-				c="$c ${i/=*/}"
-				;;
-			esac
+		for i in $(git --git-dir="$(__gitdir)" config --get-regexp "remotes\..*" 2>/dev/null); do
+			i="${i#remotes.}"
+			c="$c ${i/ */}"
 		done
 		__gitcomp "$c"
 		;;
-- 
1.6.4.2

-- 
Todd        OpenPGP -> KeyID: 0xBEAF0CE3 | URL: www.pobox.com/~tmz/pgp
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
That men do not learn very much from the lessons of history is the
most important of all the lessons of history.
    -- Aldous Huxley Collected Essays, 1959

^ permalink raw reply related

* Re: Cannot clone redirecting stdout
From: Daniel Barkalow @ 2009-09-11 22:47 UTC (permalink / raw)
  To: Jeff King; +Cc: Johan Sørensen, Aloisio, git, support
In-Reply-To: <20090911162013.GA10939@coredump.intra.peff.net>

On Fri, 11 Sep 2009, Jeff King wrote:

> On Fri, Sep 11, 2009 at 12:05:10PM -0400, Jeff King wrote:
> 
> > Ah. I have a theory. If I do a clone of git://gitorious.org/qt/qt.git,
> > the counting/compressing stages take a long time (I timed it at 1m40
> > before it actually sends any data). And looking at upload-pack.c, we
> > leave the 30-second alarm set while creating the pack. Meaning we die 30
> > seconds into creating the pack.
> > 
> > When progress is being displayed, however, the progress timer actually
> > uses SIGALRM, as well. So we are constantly resetting the timer and it
> > never goes off.
> 
> Hmm. Actually, this is not quite right. It looks like we call out to
> pack-objects as an external program, so there is no conflict with the
> signal. And we do proxy the output of pack-objects, which will keep our
> timer resetting every time we see a chunk of output. But pack-objects
> produces no output during the deltification phase, unless progress is
> turned on.  So we still hit our timeout in upload-pack during that
> phase.
> 
> So our options are:
> 
>   1. Turn off the timer during deltification, which could mean that it
>      would potentially go forever. But it's not controlled by the user.
>      I think the 'timeout' feature is really about the client just
>      opening the connection and sitting.
> 
>   2. Keep progress on during deltification, but just throw it away
>      instead of relaying it if no-progress is in effect.
> 
>   3. Accept that hitting the timeout during deltification _should_ cause
>      it to die. In that case, then the case with progress is wrong, and
>      we should stop resetting the timer just because we got some
>      progress output from pack-objects. But this may be redefining the
>      intent of --timeout. I don't know what the original intent was, or
>      what users of the feature are expecting.

You don't remember October 2005? HPA introduced it in 960decc, which has a 
pretty good explanation: we doesn't want to get DoS'd if clients just send 
SYNs. So it's supposed to time out only if we spend that long waiting 
for a protocol item from the client.

	-Daniel
*This .sig left intentionally blank*

^ permalink raw reply

* Re: obnoxious CLI complaints
From: Linus Torvalds @ 2009-09-11 22:16 UTC (permalink / raw)
  To: René Scharfe; +Cc: Jakub Narebski, Brendan Miller, git
In-Reply-To: <4AAAC8CE.8020302@lsrfire.ath.cx>



On Sat, 12 Sep 2009, René Scharfe wrote:
> 
> But what has bugged me since I added zip support is this result:
> 
> 	# git v1.6.5-rc0
> 	$ time git archive --format=zip -6 v2.6.31 >/dev/null
> 
> 	real	0m16.471s
> 	user	0m16.340s
> 	sys	0m0.128s
> 
> I'd have expected this to be the slowest case, because it's compressing
> all files separately, i.e. it needs to create and flush the compression
> context lots of times instead of only once as in the two cases above.

Oh no, I think it's easily explained.

Compressing many small files really is often cheaper than compressing one 
large one.

With lots of small files, you end up being very limited in the 
search-space, so the compression decisions get simpler. Compression in 
general is not O(n), it's some non-linear factor, often something like 
O(n**2).

Of course, all compression libraries have an upper bound on the 
non-linearity (often expressed as a "window size"), so a particular 
compression algorithm may end up being close to O(n) (with a huge 
constant). But that upper bound will only kick in for large files, small 
files that fit entirely into the compression window will still see the 
underlying O(n**2) or whatever.

But I have no actual numbers to back up the above blathering. But feel 
free to try to compress 10 small files and compare it to compressing one 
file that is as big as the sum. I bet you'll see it.

			Linus

^ permalink raw reply

* Re: bash_completion outside repo
From: Jeff King @ 2009-09-11 22:05 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Michael J Gruber, Shawn O. Pearce, james bardin, Todd Zullinger,
	git
In-Reply-To: <7vr5uduotz.fsf@alter.siamese.dyndns.org>

On Fri, Sep 11, 2009 at 02:57:28PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > I think something like that needs to "belong" to the diff code itself. I
> > guess in the case of "diff", we could check all diff-related config at
> > diff setup time.
> 
> Not necessarily.  You do not want to care about color configuration if you are
> doing diff --raw for example.  The one that first uses the color variable
> should be able to notice the breakage, no?
> 
> > But what about something used in several places, like
> > core.quotepath?
> 
> Exactly the same way I checked what codepaths needed to fix for the
> autosetupmerge stuff.  core.quotepath internally sets quote_path_fully,
> and the sole user of quote_path_fully is sq_must_quote() which is only
> used by next_quote_pos().  So you can have your check very isolated.

I guess I'm just worried that in doing this for _every_ variable we are
going to run across cases where variables are used in several different
codepaths, and we are going to end up adding a large number of tests for
"is this thing valid". And if we forget one, it's going to cause us to
access some sentinel value that may cause a segfault.

But that is just my gut feeling. I haven't actually looked at doing a
full-scale conversion.

-Peff

^ permalink raw reply

* Re: bash_completion outside repo
From: Junio C Hamano @ 2009-09-11 22:04 UTC (permalink / raw)
  To: Jeff King
  Cc: Michael J Gruber, Shawn O. Pearce, james bardin, Todd Zullinger,
	git
In-Reply-To: <7vr5uduotz.fsf@alter.siamese.dyndns.org>

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

> Jeff King <peff@peff.net> writes:
>
>> I think something like that needs to "belong" to the diff code itself. I
>> guess in the case of "diff", we could check all diff-related config at
>> diff setup time.
>
> Not necessarily.  You do not want to care about color configuration if you are
> doing diff --raw for example.  The one that first uses the color variable
> should be able to notice the breakage, no?
>
>> But what about something used in several places, like
>> core.quotepath?
>
> Exactly the same way I checked what codepaths needed to fix for the
> autosetupmerge stuff.  core.quotepath internally sets quote_path_fully,
> and the sole user of quote_path_fully is sq_must_quote() which is only
> used by next_quote_pos().  So you can have your check very isolated.

Note that I was _not_ defending the approach my illustration took.  I
merely was pointing out that core.quotepath and diff.color are _not_
valid counterexamples to it.

A better counter-proposal would be made something along this line:

	Currently, core.frotz and nitfol.rezrov both map internally to a
	variable xyzzy, so config parser could set xyzzy = INVALID_INFOCOM
        but the user of xyzzy cannot report which configuration variable
	caused the error with your illustration scheme.  Here is my
	attempt to solve this issue.

	...patch follows...

And I am sure we will be able to find such examples that my illustration
patch is not _exactly_ the best approach to solve pretty easily around
remote.*.* and branch.*.* variables.

^ permalink raw reply

* Re: obnoxious CLI complaints
From: René Scharfe @ 2009-09-11 22:01 UTC (permalink / raw)
  To: Linus Torvalds; +Cc: Jakub Narebski, Brendan Miller, git
In-Reply-To: <alpine.LFD.2.01.0909110744030.3654@localhost.localdomain>

Am 11.09.2009 16:47, schrieb Linus Torvalds:
>
>
> On Fri, 11 Sep 2009, René Scharfe wrote:
>>
>> Using zlib directly avoids the overhead of a pipe and of buffering the
>> output for blocked writes; surprisingly (to me), it isn't any faster.
>
> In fact, it should be slower.
>
> On SMP, you're quite likely better off using the pipe, and compressing on
> another CPU. Of course, it's usually the case that the compression is _so_
> much slower than generating the tar-file (especially for the hot-cache
> case) that it doesn't matter or the pipe overhead is even a slowdown.
>
> But especially if generating the tar-file has some delays in it
> (cold-cache object lookup, whatever), the "compress in separate process"
> is likely simply better, because you can compress while the other process
> is looking up data for the tar.

Yes, that makes sense and can be seen here (quad core, Fedora 11, best
of five consecutive runs, Linux kernel repo):

	# git v1.6.5-rc0
	$ time git archive --format=tar v2.6.31 | gzip -6 >/dev/null

	real	0m16.591s
	user	0m19.769s
	sys	0m0.474s

	# git v1.6.5-rc0 + patch
	$ time ../git/git archive --format=tar.gz -6 v2.6.31 >/dev/null

	real	0m20.390s
	user	0m20.299s
	sys	0m0.088s

User time is quite similar, real time is lower when using a pipe.

But what has bugged me since I added zip support is this result:

	# git v1.6.5-rc0
	$ time git archive --format=zip -6 v2.6.31 >/dev/null

	real	0m16.471s
	user	0m16.340s
	sys	0m0.128s

I'd have expected this to be the slowest case, because it's compressing
all files separately, i.e. it needs to create and flush the compression
context lots of times instead of only once as in the two cases above.
And it's sequential and uses zlib, just like the tar.gz format.  I
suspect the convenience function gzwrite() adds this overhead.


Oh, I just discovered pigz (http://zlib.net/pigz/), a parallel gzip:

	# git v1.6.5-rc0, pigz 2.1.5
	$ time git archive --format=tar v2.6.31 | pigz -6 >/dev/null

	real	0m6.251s
	user	0m21.383s
	sys	0m0.547s

So pipes win. :)  Still need to investigate why zip is as (relatively)
fast as it is, though.

René

^ permalink raw reply

* Re: bash_completion outside repo
From: Junio C Hamano @ 2009-09-11 21:57 UTC (permalink / raw)
  To: Jeff King
  Cc: Michael J Gruber, Shawn O. Pearce, james bardin, Todd Zullinger,
	git
In-Reply-To: <20090911212938.GA26589@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

> I think something like that needs to "belong" to the diff code itself. I
> guess in the case of "diff", we could check all diff-related config at
> diff setup time.

Not necessarily.  You do not want to care about color configuration if you are
doing diff --raw for example.  The one that first uses the color variable
should be able to notice the breakage, no?

> But what about something used in several places, like
> core.quotepath?

Exactly the same way I checked what codepaths needed to fix for the
autosetupmerge stuff.  core.quotepath internally sets quote_path_fully,
and the sole user of quote_path_fully is sq_must_quote() which is only
used by next_quote_pos().  So you can have your check very isolated.

^ permalink raw reply

* Re: bash_completion outside repo
From: Jeff King @ 2009-09-11 21:29 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Michael J Gruber, Shawn O. Pearce, james bardin, Todd Zullinger,
	git
In-Reply-To: <7vvdjpuqgx.fsf@alter.siamese.dyndns.org>

On Fri, Sep 11, 2009 at 02:22:06PM -0700, Junio C Hamano wrote:

> So here is an illustration to handle _only_ a misspelled
> branch.autosetupmerge.
> 
> If you have this in your .git/config file:
> 
> 	[branch]
>         	autosetupmerge = nevver
> 
> you cannot run "git diff" without this patch.  But with this patch, only
> the commands that _care_ about this misconfiguration would notice and
> report.

OK, that example makes sense. But I'm a little dubious of how this
scales to something like color.diff.plain. Who is responsible for
checking? Do we do it at the beginning of every program which cares
about diff values? If so, what is the failure mode when we forget (and I
suspect we will, because it is easy for programs to call into unexpected
code that is three layers deep)?

I think something like that needs to "belong" to the diff code itself. I
guess in the case of "diff", we could check all diff-related config at
diff setup time. But what about something used in several places, like
core.quotepath?

-Peff

^ permalink raw reply

* Re: bash_completion outside repo
From: Junio C Hamano @ 2009-09-11 21:22 UTC (permalink / raw)
  To: Jeff King
  Cc: Michael J Gruber, Shawn O. Pearce, james bardin, Todd Zullinger,
	git
In-Reply-To: <7viqfpw6tv.fsf@alter.siamese.dyndns.org>

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

> The last one is not the topic of this patch, but it is quite problematic.
> When you are interested in finding out what value gc.pruneExpire is set,
> you do not care (as long as the configuration file was syntactically
> correct and you did not have to skip any file you were supposed to read
> due to EACCESS) if "branch.autosetupmerge" has an invalid value.
>
> A possible longer term solution would be to:
>
>  - Change the signature of callbacks (e.g. git_default_branch_config()) so
>    that they return void.  They are not allowed to report any semantic
>    errors while parsing.
>
>  - Instead, they use special "INVALID" value and store that when they see
>    a semantically incorrect value.  They may also want to store what the
>    actual string the config file gave them for later reporting, together
>    with the name of and the line number in the config file for diagnostic
>    purposes.
>
>  - The user of the internalized value (i.e. "git grep git_branch_track"
>    shows there are only two, cmd_branch() and cmd_checkout()) must check
>    for the above INVALID value before they use the variable, and die at
>    the point of the use.
>
> I'll send an illustration patch separately.

So here is an illustration to handle _only_ a misspelled
branch.autosetupmerge.

If you have this in your .git/config file:

	[branch]
        	autosetupmerge = nevver

you cannot run "git diff" without this patch.  But with this patch, only
the commands that _care_ about this misconfiguration would notice and
report.

The new world order is:

 - The config parsing callback should detect semantic errors, but should
   not return non-zero, to let the parser continue.

   In the _real_ patch, their signatures should be changed to return void.
   I wanted to illustrate more important points in this patch so I didn't
   do that.

 - Instead, when they detect an error in configured values, they make a
   note that the value is bad (e.g. I introduced BRANCH_TRACK_CONFIG_ERROR
   for this), and also can ask record_bad_config() to record this fact for
   possible later reporting purposes.  You also _could_ issue error() to
   make it easier to detect unrelated but bad configuration for users, but
   that is secondary and I didn't do that in this illustration.

 - Then, when the values are actually _used_, the users should take notice
   of the situation (e.g. both cmd_checkout() and cmd_branch() codepaths
   are modified to do this) and ask die_with_bad_config() to report the
   error the parser detected earlier.

This way, commands that do not use the value of branch.autosetupmerge,
e.g. "git diff", won't have to abort.

 builtin-branch.c   |    5 +++-
 builtin-checkout.c |    5 +++-
 cache.h            |    7 ++++++
 config.c           |   57 ++++++++++++++++++++++++++++++++++++++++++++++++---
 4 files changed, 68 insertions(+), 6 deletions(-)

diff --git a/builtin-branch.c b/builtin-branch.c
index 9f57992..10010a7 100644
--- a/builtin-branch.c
+++ b/builtin-branch.c
@@ -635,9 +635,12 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		rename_branch(head, argv[0], rename > 1);
 	else if (rename && (argc == 2))
 		rename_branch(argv[0], argv[1], rename > 1);
-	else if (argc <= 2)
+	else if (argc <= 2) {
+		if (track == BRANCH_TRACK_CONFIG_ERROR)
+			die_with_bad_config("branch.autosetupmerge");
 		create_branch(head, argv[0], (argc == 2) ? argv[1] : head,
 			      force_create, reflog, track);
+	}
 	else
 		usage_with_options(builtin_branch_usage, options);
 
diff --git a/builtin-checkout.c b/builtin-checkout.c
index d050c37..3bf57a5 100644
--- a/builtin-checkout.c
+++ b/builtin-checkout.c
@@ -630,8 +630,11 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 		opts.new_branch = argv0 + 1;
 	}
 
-	if (opts.track == BRANCH_TRACK_UNSPECIFIED)
+	if (opts.track == BRANCH_TRACK_UNSPECIFIED) {
+		if (git_branch_track == BRANCH_TRACK_CONFIG_ERROR)
+			die_with_bad_config("branch.autosetupmerge");
 		opts.track = git_branch_track;
+	}
 	if (conflict_style) {
 		opts.merge = 1; /* implied */
 		git_xmerge_config("merge.conflictstyle", conflict_style, NULL);
diff --git a/cache.h b/cache.h
index 5fad24c..f7ff502 100644
--- a/cache.h
+++ b/cache.h
@@ -533,6 +533,7 @@ enum safe_crlf {
 extern enum safe_crlf safe_crlf;
 
 enum branch_track {
+	BRANCH_TRACK_CONFIG_ERROR = -2,
 	BRANCH_TRACK_UNSPECIFIED = -1,
 	BRANCH_TRACK_NEVER = 0,
 	BRANCH_TRACK_REMOTE,
@@ -890,6 +891,9 @@ extern const char *packed_object_info_detail(struct packed_git *, off_t, unsigne
 /* Dumb servers support */
 extern int update_server_info(int);
 
+/*
+ * configuration file management
+ */
 typedef int (*config_fn_t)(const char *, const char *, void *);
 extern int git_default_config(const char *, const char *, void *);
 extern int git_config_from_file(config_fn_t fn, const char *, void *);
@@ -910,6 +914,9 @@ extern int git_config_global(void);
 extern int config_error_nonbool(const char *);
 extern const char *config_exclusive_filename;
 
+extern void record_bad_config(const char *name, const char *value);
+extern void die_with_bad_config(const char *name);
+
 #define MAX_GITNAME (1000)
 extern char git_default_email[MAX_GITNAME];
 extern char git_default_name[MAX_GITNAME];
diff --git a/config.c b/config.c
index e87edea..5b6a929 100644
--- a/config.c
+++ b/config.c
@@ -306,6 +306,41 @@ static void die_bad_config(const char *name)
 	die("bad config value for '%s'", name);
 }
 
+static struct bad_config_note {
+	struct bad_config_note *next;
+	const char *name;
+	const char *value;
+	int linenr;
+	const char *filename;
+} *bad_config_note;
+
+void record_bad_config(const char *name, const char *value)
+{
+	struct bad_config_note *note = xcalloc(1, sizeof(*note));
+	note->next = bad_config_note;
+	bad_config_note = note;
+	note->name = xstrdup(name);
+	note->value = xstrdup(value);
+	note->linenr = config_linenr;
+	note->filename = config_file_name ? xstrdup(config_file_name) : NULL;
+}
+
+void die_with_bad_config(const char *name)
+{
+	struct bad_config_note *note;
+	for (note = bad_config_note; note; note = note->next)
+		if (!strcmp(name, note->name)) {
+			char whence[PATH_MAX + 200];
+			if (note->filename)
+				sprintf(whence, " in %s", note->filename);
+			else
+				whence[0] = '\0';
+			die("bad config value '%s' for '%s'%s, line %d",
+			    note->value, note->name, whence, note->linenr);
+		}
+	die("program error: bad config value for %s not recorded", name);
+}
+
 int git_config_int(const char *name, const char *value)
 {
 	long ret = 0;
@@ -324,6 +359,8 @@ unsigned long git_config_ulong(const char *name, const char *value)
 
 int git_config_bool_or_int(const char *name, const char *value, int *is_bool)
 {
+	long ret;
+
 	*is_bool = 1;
 	if (!value)
 		return 1;
@@ -333,14 +370,24 @@ int git_config_bool_or_int(const char *name, const char *value, int *is_bool)
 		return 1;
 	if (!strcasecmp(value, "false") || !strcasecmp(value, "no") || !strcasecmp(value, "off"))
 		return 0;
+
 	*is_bool = 0;
-	return git_config_int(name, value);
+
+	ret = 0;
+	if (!git_parse_long(value, &ret)) {
+		record_bad_config(name, value);
+		*is_bool = -1;
+	}
+	return ret;
 }
 
 int git_config_bool(const char *name, const char *value)
 {
-	int discard;
-	return !!git_config_bool_or_int(name, value, &discard);
+	int is_bool, val;
+	val = git_config_bool_or_int(name, value, &is_bool);
+	if (is_bool < 0)
+		return is_bool;
+	return !!val;
 }
 
 int git_config_string(const char **dest, const char *var, const char *value)
@@ -546,11 +593,13 @@ static int git_default_i18n_config(const char *var, const char *value)
 static int git_default_branch_config(const char *var, const char *value)
 {
 	if (!strcmp(var, "branch.autosetupmerge")) {
+		int val;
 		if (value && !strcasecmp(value, "always")) {
 			git_branch_track = BRANCH_TRACK_ALWAYS;
 			return 0;
 		}
-		git_branch_track = git_config_bool(var, value);
+		val = git_config_bool(var, value);
+		git_branch_track = (val < 0) ? BRANCH_TRACK_CONFIG_ERROR : val;
 		return 0;
 	}
 	if (!strcmp(var, "branch.autosetuprebase")) {

^ permalink raw reply related

* Re: bash_completion outside repo
From: Jeff King @ 2009-09-11 21:12 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Shawn O. Pearce, james bardin, Todd Zullinger, git
In-Reply-To: <7viqfpw6tv.fsf@alter.siamese.dyndns.org>

On Fri, Sep 11, 2009 at 01:43:24PM -0700, Junio C Hamano wrote:

> While I also agree that EACCESS and other failures should be treated as
> fatal in principle for safety, e.g. when running prune without being able
> to read gc.pruneExpire as you mentioned, we would need a tradeoff between
> safety and convenience.  When asked to help looking at a complex merge in
> a colleages' repository, do you want your "git diff" to refuse to run only
> because her .git/config cannot be read by you?  Of course, you _can_ work

Sorry, I think I was a bit unclear in the original message. There are
two classes of errors right now:

  1. access(fn, R_OK) != 0 (i.e., ENOENT and EACCESS)

  2. fopen(fn, "r") != 0

In the case of (1), we treat it as an empty file (except in the case
that _all_ files fail (1), in which case we reported an error, which is
really stupid and is what this patch fixes).

In the case of (2), we treat it as an error, but because nobody actually
bothers to check the error code, it is effectively ignored. What I was
thinking is that (2) should be promoted to die(), and leave (1) as-is.

So I think in your example it would fall under (1), and we both agree
that should be allowed.

But:

> Also there was a move going in the opposite direction to allow a config
> file that is syntactically broken to be handled without the parser dying,
> primarily to help "git config -e".  In the longer term, our direction
> shouldn't be adding more die() in the git_config() callchain, but instead
> allowing it to report different kind of failures to let the caller act
> based on what it does and what the nature of failure is.

Yeah, that is the opposite of what I proposed above. But it is a step
towards lib-ifying the config code, which is probably a good thing. The
config code is an utter mess. I am a little hesitant to touch it just
because I don't think there is anything _broken_ in it exactly, but
every time I look at it, I am always shocked by how unnecessarily
complex it is.

I think the "right" thing to do would probably be to have a lib-ified
function to read the config, and then have a wrapper that 99% of the
programs use that just checks the error return and calls die() if there
is a problem. But such a cleanup is likely to introduce new bugs, so I
have let it be (also, because my time is not infinite and there are
other more interesting things to work on ;) ).

> For example, "gc" may say "I won't prune because I had to skip some of the
> lines in your .git/config because you have syntax errors in them, and I
> may have missed the definition of gc.pruneExpire you may wanted to place
> on them", while "diff" may ignore that kind of errors.

Yeah, that makes sense to me, and should be possible with a decent
lib-ified interface.

> Having said all that, my preference *for now* would be to ignore "there is
> no $HOME/.gitconfig (or /etc/gitconfig) file", but catch all other errors
> and die().

OK, then I think we are on the same page.

> There are some other glitches in the current git_config() callchain.
> 
>  - No config file anywhere gives an error.  I agree with you that this is
>    a bug.

Yep, and this patch fixes that.

>  - Having GIT_CONFIG=/no/such/file in the environment gives an error,
>    which is good.

Yep, and and this patch should leave that untouched (I didn't test that
specifically, but I checked that "git config --global" does, and I
assume they use the same code path. Of course, one never knows...).

> A possible longer term solution would be to:
> 
>  - Change the signature of callbacks (e.g. git_default_branch_config()) so
>    that they return void.  They are not allowed to report any semantic
>    errors while parsing.
> 
>  - Instead, they use special "INVALID" value and store that when they see
>    a semantically incorrect value.  They may also want to store what the
>    actual string the config file gave them for later reporting, together
>    with the name of and the line number in the config file for diagnostic
>    purposes.
> 
>  - The user of the internalized value (i.e. "git grep git_branch_track"
>    shows there are only two, cmd_branch() and cmd_checkout()) must check
>    for the above INVALID value before they use the variable, and die at
>    the point of the use.

That all makes sense to me. My biggest worry is that we will need to be
checking for "INVALID" values in lots of places before actually using
the value from a variable. IOW, it is nice for code to be able to call
into some library call that respects a config variable without having to
care about doing some magic setup to say "Oh, by the way, I am
interesting in the value of diff.foo". At the same time, it is nice for
the library code to not have to say "We're using diff.foo. Let's make
sure somebody has checked the value before using it."

In other words, I would like not-too-syntactically-painful lazy values.
With no runtime overhead. In C. ;)

-Peff

^ permalink raw reply

* Re: one half of a rebase
From: Alex Riesen @ 2009-09-11 21:10 UTC (permalink / raw)
  To: Geoffrey Irving; +Cc: git, Dylan Simon
In-Reply-To: <7f9d599f0909111025q42e3cdc6vba602b84c1d81215@mail.gmail.com>

On Fri, Sep 11, 2009 at 19:25, Geoffrey Irving <irving@naml.us> wrote:
> If I could do (2) as a separate operation, it would look something like
>
>    git cherry-pick-all topic
>
> which is simpler and faster since it avoids switching files back and
> forth (master to topic and back).  Is there a robust way to achieve
> the cherry-pick-all semantics with current commands?  If not, how
> difficult would it be to partition rebase accordingly?

I have this in my .bashrc:

$ gcp3 ()
{
    git format-patch -k --stdout --full-index "$@" | git am -k -3 --binary
}

Then, while on master branch:

$ gcp3 master..topic

^ permalink raw reply

* Re: [PATCH v2] Re: add documentation for mailinfo.scissors and '--no-scissors'
From: Junio C Hamano @ 2009-09-11 21:00 UTC (permalink / raw)
  To: Nicolas Sebrecht; +Cc: Nanako Shiraishi, git, Pierre Habouzit
In-Reply-To: <20090911200849.GB18684@vidovic>

Nicolas Sebrecht <nicolas.s.dev@gmx.fr> writes:

> Ok. So, the fact that the usage of git-am doesn't tell about
> --no-scissors is the expected behaviour?

You _could_ argue that we _could_ describe a long option "frotz" that
lacks the '!' flag in OPTIONS_SPEC as "--[no-]frotz" in the output by
changing the rev-parse --parseopt, if you really want to.

However, I think that is not done deliberately to avoid cluttering the
output.  I Cc'ed the primary guilty party ;-) of the parse-options
infrastructure.

Currently, non-bool options are not marked with '!'.  Nobody sane would
say "git am --no-directory foo", but "rev-parse --parseopt" acccepts such
a nonsense input, and it is up to the calling script to catch it and barf.
But "rev-parse --parseopt" will start saying "--[no-]directory=" with such
a change, which is not good.

And --no-scissors is not that special.  We could add --no-signoff to say
"I do not want to sign-off this one time" explicitly, and it is crazy if
we had to add another line in OPTIONS_SPEC when we want to do so, when it
is clear "signoff" option is a boolean.

As a long term direction, I'd rather not to see "no-" in OPTIONS_SPEC, but
have that taken care of by "rev-parse --parseopt" to keep our sanity.  The
only existing offender is "no-verify" in "rebase -i".  Let's solve it (if
there is anything to solve, which I doubt) without adding new ones.

^ permalink raw reply

* Re: bash_completion outside repo
From: Junio C Hamano @ 2009-09-11 20:43 UTC (permalink / raw)
  To: Jeff King; +Cc: Shawn O. Pearce, james bardin, Todd Zullinger, git
In-Reply-To: <20090911164730.GA21536@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

> This is actually a bit overengineered. Of the hundreds of calls to
> git_config, there are exactly _two_ which check the return value. And
> neither of them cares whether we parsed files or not; they really only
> care if there was an error. So we could simply return 0 as long as there
> is no error.
>
> This also makes me wonder, though. Git can do wildly different things
> (including hard-to-reverse things) based on config (e.g., just consider
> gc.pruneExpire). Yet we call git_config() without ever checking for
> errors. In the actual parsing routines, we die() if there is an error.
> But if we fail to open the file due to some transient error, we will
> silently ignore the situation.
>
> Granted, such transient errors are unlikely. The biggest reasons for
> failing to open a file are that it doesn't exist, or that we have no
> permission to read it, both of which are treated explicitly in
> git_config as "silently ok". But I wonder if we should simply be dying
> on such an error, and git_config() should just have a void return.

Thanks.

ENOENT should be the same as having an empty file, which is the main point
of the patch and at the same time why you feel this is overengineered.  I
agree with you on both counts.

While I also agree that EACCESS and other failures should be treated as
fatal in principle for safety, e.g. when running prune without being able
to read gc.pruneExpire as you mentioned, we would need a tradeoff between
safety and convenience.  When asked to help looking at a complex merge in
a colleages' repository, do you want your "git diff" to refuse to run only
because her .git/config cannot be read by you?  Of course, you _can_ work
this particular one around by various means (e.g. prefixing GIT_CONFIG=...
to force ignoring the file; telling the colleage that she'd better make
her .git/config readable to you if she wants your help), if either one of
the owner of the .git/config file or the party that wants to access the
repository is a non-person such a workaround would be harder to arrange.

Also there was a move going in the opposite direction to allow a config
file that is syntactically broken to be handled without the parser dying,
primarily to help "git config -e".  In the longer term, our direction
shouldn't be adding more die() in the git_config() callchain, but instead
allowing it to report different kind of failures to let the caller act
based on what it does and what the nature of failure is.

For example, "gc" may say "I won't prune because I had to skip some of the
lines in your .git/config because you have syntax errors in them, and I
may have missed the definition of gc.pruneExpire you may wanted to place
on them", while "diff" may ignore that kind of errors.

Having said all that, my preference *for now* would be to ignore "there is
no $HOME/.gitconfig (or /etc/gitconfig) file", but catch all other errors
and die().

There are some other glitches in the current git_config() callchain.

 - No config file anywhere gives an error.  I agree with you that this is
   a bug.

 - Having GIT_CONFIG=/no/such/file in the environment gives an error,
   which is good.

 - config.c::git_parse_file() [*1*] dies when it detects certain file
   format errors itself.  This is not good for "git config -e", as it
   needs to learn core.editor before it can be used to fix such an error.

   This function then calls config.c::get_value() and it dies when
   config.c::get_value() reports any error.

 - config.c::get_value() is called by config.c::git_parse_file() to finish
   parsing out the <name, value> pair, and stores the "value" in a form
   usable in the code, e.g. a variable defined in environment.c.  The
   function returns an error on some file format errors (e.g. a variable
   name is too long, string quoting unterminated) that signals the calling
   config.c::git_parse_file() to die().  These error returns are good (the
   caller however may need to be fixed for "config -e" issue not to die).

   It then calls the parse callbacck routines.  They return error when
   they detect semantic errors (e.g. "branch.autosetupmerge = alwys" is
   not one of the valid values).

The last one is not the topic of this patch, but it is quite problematic.
When you are interested in finding out what value gc.pruneExpire is set,
you do not care (as long as the configuration file was syntactically
correct and you did not have to skip any file you were supposed to read
due to EACCESS) if "branch.autosetupmerge" has an invalid value.

A possible longer term solution would be to:

 - Change the signature of callbacks (e.g. git_default_branch_config()) so
   that they return void.  They are not allowed to report any semantic
   errors while parsing.

 - Instead, they use special "INVALID" value and store that when they see
   a semantically incorrect value.  They may also want to store what the
   actual string the config file gave them for later reporting, together
   with the name of and the line number in the config file for diagnostic
   purposes.

 - The user of the internalized value (i.e. "git grep git_branch_track"
   shows there are only two, cmd_branch() and cmd_checkout()) must check
   for the above INVALID value before they use the variable, and die at
   the point of the use.

I'll send an illustration patch separately.

[Footnote]

*1* What a horrible name for this function!  It is static so git_ prefix
is unneeded, and if it anticipates it might get someday exported, parse_file
is too generic and should be named git_parse_config_file().

^ permalink raw reply

* [PATCH v2] Re: add documentation for mailinfo.scissors and '--no-scissors'
From: Nicolas Sebrecht @ 2009-09-11 20:08 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Nicolas Sebrecht, Nanako Shiraishi, git
In-Reply-To: <7vws45wbxq.fsf@alter.siamese.dyndns.org>

The 11/09/09, Junio C Hamano wrote:
> Nicolas Sebrecht <nicolas.s.dev@gmx.fr> writes:
> 
> As scissors is not marked with ! (see PARSEOPT section in the rev-parse
> documentation), no-scissors is already available without that hunk, and
> that is why your patch is unnecessary.
> 
> Your patch did not mark no-scissors with ! either, so it makes the call to
> rev-parse --parseopt to process your OPTIONS_SPEC pass --no-no-scissors.

Ok. So, the fact that the usage of git-am doesn't tell about
--no-scissors is the expected behaviour?

Without changing the current implementation of rev-parse, I can't see a
smart way to add --no-scissors in the 'git am' usage. Yes, there's still
the possibility to negate both "c,scissors" and "no-scissors" but this
would defeate some of the PARSE_OPT_NONEG purpose, right?

Oh, and I still feel a bit disoriented by this "rev-parse --parseopt"
thing. I guess I need to read the code. :-)

-- 
Nicolas Sebrecht

^ permalink raw reply

* [JGIT PATCH 3/3] Use keep(1) instead of add() when skipping an entry
From: Shawn O. Pearce @ 2009-09-11 19:58 UTC (permalink / raw)
  To: Robin Rosenberg; +Cc: git, Shawn O. Pearce
In-Reply-To: <1252699129-6961-2-git-send-email-spearce@spearce.org>

Doing a keep call with a length of 1 will copy the current entry just
like the previous add was doing, but it avoids doing any validation
on the entry.  This is sane because the entry can be assumed to be
already valid, since its originating from the destination index.

Signed-off-by: Shawn O. Pearce <sop@google.com>
---
 .../jgit/dircache/DirCacheBuildIterator.java       |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/org.spearce.jgit/src/org/spearce/jgit/dircache/DirCacheBuildIterator.java b/org.spearce.jgit/src/org/spearce/jgit/dircache/DirCacheBuildIterator.java
index 9818b76..94fc071 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/dircache/DirCacheBuildIterator.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/dircache/DirCacheBuildIterator.java
@@ -111,7 +111,7 @@ public void skip() throws CorruptObjectException {
 		if (currentSubtree != null)
 			builder.keep(ptr, currentSubtree.getEntrySpan());
 		else
-			builder.add(currentEntry);
+			builder.keep(ptr, 1);
 		next(1);
 	}
 
-- 
1.6.5.rc0.164.g5f6b0

^ permalink raw reply related

* [JGIT PATCH 2/3] Don't allow DirCacheEntry with mode of 0
From: Shawn O. Pearce @ 2009-09-11 19:58 UTC (permalink / raw)
  To: Robin Rosenberg; +Cc: git, Shawn O. Pearce, Adam W. Hawks
In-Reply-To: <1252699129-6961-1-git-send-email-spearce@spearce.org>

A 0 file mode in a DirCacheEntry is not a valid mode.  To C git
such a value indicates the record should not be present.  We already
were catching this bad state and exceptioning out when writing tree
objects to disk, but we did not fail when writing the dircache back
to disk.  This allowed JGit applications to create a dircache file
which C git would not like to read.

Instead of checking the mode during writes, we now check during
mutation.  This allows application bugs to be detected sooner and
closer to the cause site.  It also allows us to avoid checking most
of the records which we read in from disk, as we can assume these
are formatted correctly.

Some of our unit tests were not setting the FileMode on their test
entry, so they had to be updated to use REGULAR_FILE.

Signed-off-by: Shawn O. Pearce <sop@google.com>
CC: Adam W. Hawks <awhawks@writeme.com>
---
 .../spearce/jgit/dircache/DirCacheBasicTest.java   |    5 ++-
 .../spearce/jgit/dircache/DirCacheBuilderTest.java |   27 +++++++++++++--
 .../spearce/jgit/dircache/DirCacheEntryTest.java   |   37 ++++++++++++++++++++
 .../spearce/jgit/dircache/DirCacheFindTest.java    |    5 ++-
 .../jgit/dircache/DirCacheIteratorTest.java        |    4 ++-
 .../jgit/dircache/DirCacheLargePathTest.java       |    5 +++
 .../spearce/jgit/dircache/DirCacheTreeTest.java    |   13 +++++--
 .../org/spearce/jgit/dircache/DirCacheBuilder.java |    8 +++--
 .../org/spearce/jgit/dircache/DirCacheEditor.java  |   11 ++++--
 .../org/spearce/jgit/dircache/DirCacheEntry.java   |   12 ++++++-
 .../org/spearce/jgit/dircache/DirCacheTree.java    |    4 --
 11 files changed, 111 insertions(+), 20 deletions(-)

diff --git a/org.spearce.jgit.test/tst/org/spearce/jgit/dircache/DirCacheBasicTest.java b/org.spearce.jgit.test/tst/org/spearce/jgit/dircache/DirCacheBasicTest.java
index 4d737c0..3b48b11 100644
--- a/org.spearce.jgit.test/tst/org/spearce/jgit/dircache/DirCacheBasicTest.java
+++ b/org.spearce.jgit.test/tst/org/spearce/jgit/dircache/DirCacheBasicTest.java
@@ -40,6 +40,7 @@
 import java.io.File;
 
 import org.spearce.jgit.lib.Constants;
+import org.spearce.jgit.lib.FileMode;
 import org.spearce.jgit.lib.RepositoryTestCase;
 
 public class DirCacheBasicTest extends RepositoryTestCase {
@@ -170,8 +171,10 @@ public void testBuildThenClear() throws Exception {
 
 		final String[] paths = { "a.", "a.b", "a/b", "a0b" };
 		final DirCacheEntry[] ents = new DirCacheEntry[paths.length];
-		for (int i = 0; i < paths.length; i++)
+		for (int i = 0; i < paths.length; i++) {
 			ents[i] = new DirCacheEntry(paths[i]);
+			ents[i].setFileMode(FileMode.REGULAR_FILE);
+		}
 
 		final DirCacheBuilder b = dc.builder();
 		for (int i = 0; i < ents.length; i++)
diff --git a/org.spearce.jgit.test/tst/org/spearce/jgit/dircache/DirCacheBuilderTest.java b/org.spearce.jgit.test/tst/org/spearce/jgit/dircache/DirCacheBuilderTest.java
index 2cf1d92..f64e4f6 100644
--- a/org.spearce.jgit.test/tst/org/spearce/jgit/dircache/DirCacheBuilderTest.java
+++ b/org.spearce.jgit.test/tst/org/spearce/jgit/dircache/DirCacheBuilderTest.java
@@ -59,6 +59,20 @@ public void testBuildEmpty() throws Exception {
 		}
 	}
 
+	public void testBuildRejectsUnsetFileMode() throws Exception {
+		final DirCache dc = DirCache.newInCore();
+		final DirCacheBuilder b = dc.builder();
+		assertNotNull(b);
+
+		final DirCacheEntry e = new DirCacheEntry("a");
+		assertEquals(0, e.getRawMode());
+		try {
+			b.add(e);
+		} catch (IllegalArgumentException err) {
+			assertEquals("FileMode not set for path a", err.getMessage());
+		}
+	}
+
 	public void testBuildOneFile_FinishWriteCommit() throws Exception {
 		final String path = "a-file-path";
 		final FileMode mode = FileMode.REGULAR_FILE;
@@ -162,6 +176,7 @@ public void testFindSingleFile() throws Exception {
 		assertNotNull(b);
 
 		final DirCacheEntry entOrig = new DirCacheEntry(path);
+		entOrig.setFileMode(FileMode.REGULAR_FILE);
 		assertNotSame(path, entOrig.getPathString());
 		assertEquals(path, entOrig.getPathString());
 		b.add(entOrig);
@@ -185,8 +200,10 @@ public void testAdd_InGitSortOrder() throws Exception {
 
 		final String[] paths = { "a.", "a.b", "a/b", "a0b" };
 		final DirCacheEntry[] ents = new DirCacheEntry[paths.length];
-		for (int i = 0; i < paths.length; i++)
+		for (int i = 0; i < paths.length; i++) {
 			ents[i] = new DirCacheEntry(paths[i]);
+			ents[i].setFileMode(FileMode.REGULAR_FILE);
+		}
 
 		final DirCacheBuilder b = dc.builder();
 		for (int i = 0; i < ents.length; i++)
@@ -207,8 +224,10 @@ public void testAdd_ReverseGitSortOrder() throws Exception {
 
 		final String[] paths = { "a.", "a.b", "a/b", "a0b" };
 		final DirCacheEntry[] ents = new DirCacheEntry[paths.length];
-		for (int i = 0; i < paths.length; i++)
+		for (int i = 0; i < paths.length; i++) {
 			ents[i] = new DirCacheEntry(paths[i]);
+			ents[i].setFileMode(FileMode.REGULAR_FILE);
+		}
 
 		final DirCacheBuilder b = dc.builder();
 		for (int i = ents.length - 1; i >= 0; i--)
@@ -229,8 +248,10 @@ public void testBuilderClear() throws Exception {
 
 		final String[] paths = { "a.", "a.b", "a/b", "a0b" };
 		final DirCacheEntry[] ents = new DirCacheEntry[paths.length];
-		for (int i = 0; i < paths.length; i++)
+		for (int i = 0; i < paths.length; i++) {
 			ents[i] = new DirCacheEntry(paths[i]);
+			ents[i].setFileMode(FileMode.REGULAR_FILE);
+		}
 		{
 			final DirCacheBuilder b = dc.builder();
 			for (int i = 0; i < ents.length; i++)
diff --git a/org.spearce.jgit.test/tst/org/spearce/jgit/dircache/DirCacheEntryTest.java b/org.spearce.jgit.test/tst/org/spearce/jgit/dircache/DirCacheEntryTest.java
index a6ff5a8..971f201 100644
--- a/org.spearce.jgit.test/tst/org/spearce/jgit/dircache/DirCacheEntryTest.java
+++ b/org.spearce.jgit.test/tst/org/spearce/jgit/dircache/DirCacheEntryTest.java
@@ -40,6 +40,7 @@
 import junit.framework.TestCase;
 
 import org.spearce.jgit.lib.Constants;
+import org.spearce.jgit.lib.FileMode;
 
 public class DirCacheEntryTest extends TestCase {
 	public void testIsValidPath() {
@@ -112,4 +113,40 @@ public void testCreate_ByStringPathAndStage() {
 			assertEquals("Invalid stage 4 for path a", err.getMessage());
 		}
 	}
+
+	public void testSetFileMode() {
+		final DirCacheEntry e = new DirCacheEntry("a");
+
+		assertEquals(0, e.getRawMode());
+
+		e.setFileMode(FileMode.REGULAR_FILE);
+		assertSame(FileMode.REGULAR_FILE, e.getFileMode());
+		assertEquals(FileMode.REGULAR_FILE.getBits(), e.getRawMode());
+
+		e.setFileMode(FileMode.EXECUTABLE_FILE);
+		assertSame(FileMode.EXECUTABLE_FILE, e.getFileMode());
+		assertEquals(FileMode.EXECUTABLE_FILE.getBits(), e.getRawMode());
+
+		e.setFileMode(FileMode.SYMLINK);
+		assertSame(FileMode.SYMLINK, e.getFileMode());
+		assertEquals(FileMode.SYMLINK.getBits(), e.getRawMode());
+
+		e.setFileMode(FileMode.GITLINK);
+		assertSame(FileMode.GITLINK, e.getFileMode());
+		assertEquals(FileMode.GITLINK.getBits(), e.getRawMode());
+
+		try {
+			e.setFileMode(FileMode.MISSING);
+			fail("incorrectly accepted FileMode.MISSING");
+		} catch (IllegalArgumentException err) {
+			assertEquals("Invalid mode 0 for path a", err.getMessage());
+		}
+
+		try {
+			e.setFileMode(FileMode.TREE);
+			fail("incorrectly accepted FileMode.TREE");
+		} catch (IllegalArgumentException err) {
+			assertEquals("Invalid mode 40000 for path a", err.getMessage());
+		}
+	}
 }
diff --git a/org.spearce.jgit.test/tst/org/spearce/jgit/dircache/DirCacheFindTest.java b/org.spearce.jgit.test/tst/org/spearce/jgit/dircache/DirCacheFindTest.java
index 0eb0302..470c80a 100644
--- a/org.spearce.jgit.test/tst/org/spearce/jgit/dircache/DirCacheFindTest.java
+++ b/org.spearce.jgit.test/tst/org/spearce/jgit/dircache/DirCacheFindTest.java
@@ -37,6 +37,7 @@
 
 package org.spearce.jgit.dircache;
 
+import org.spearce.jgit.lib.FileMode;
 import org.spearce.jgit.lib.RepositoryTestCase;
 
 public class DirCacheFindTest extends RepositoryTestCase {
@@ -45,8 +46,10 @@ public void testEntriesWithin() throws Exception {
 
 		final String[] paths = { "a.", "a/b", "a/c", "a/d", "a0b" };
 		final DirCacheEntry[] ents = new DirCacheEntry[paths.length];
-		for (int i = 0; i < paths.length; i++)
+		for (int i = 0; i < paths.length; i++) {
 			ents[i] = new DirCacheEntry(paths[i]);
+			ents[i].setFileMode(FileMode.REGULAR_FILE);
+		}
 		final int aFirst = 1;
 		final int aLast = 3;
 
diff --git a/org.spearce.jgit.test/tst/org/spearce/jgit/dircache/DirCacheIteratorTest.java b/org.spearce.jgit.test/tst/org/spearce/jgit/dircache/DirCacheIteratorTest.java
index 047c989..71581dc 100644
--- a/org.spearce.jgit.test/tst/org/spearce/jgit/dircache/DirCacheIteratorTest.java
+++ b/org.spearce.jgit.test/tst/org/spearce/jgit/dircache/DirCacheIteratorTest.java
@@ -68,8 +68,10 @@ public void testNoSubtree_NoTreeWalk() throws Exception {
 
 		final String[] paths = { "a.", "a0b" };
 		final DirCacheEntry[] ents = new DirCacheEntry[paths.length];
-		for (int i = 0; i < paths.length; i++)
+		for (int i = 0; i < paths.length; i++) {
 			ents[i] = new DirCacheEntry(paths[i]);
+			ents[i].setFileMode(FileMode.REGULAR_FILE);
+		}
 
 		final DirCacheBuilder b = dc.builder();
 		for (int i = 0; i < ents.length; i++)
diff --git a/org.spearce.jgit.test/tst/org/spearce/jgit/dircache/DirCacheLargePathTest.java b/org.spearce.jgit.test/tst/org/spearce/jgit/dircache/DirCacheLargePathTest.java
index 4ea286c..a9945f1 100644
--- a/org.spearce.jgit.test/tst/org/spearce/jgit/dircache/DirCacheLargePathTest.java
+++ b/org.spearce.jgit.test/tst/org/spearce/jgit/dircache/DirCacheLargePathTest.java
@@ -40,6 +40,7 @@
 import java.io.IOException;
 
 import org.spearce.jgit.errors.CorruptObjectException;
+import org.spearce.jgit.lib.FileMode;
 import org.spearce.jgit.lib.RepositoryTestCase;
 
 public class DirCacheLargePathTest extends RepositoryTestCase {
@@ -70,6 +71,10 @@ private void testLongPath(final int len) throws CorruptObjectException,
 
 		final DirCacheEntry longEnt = new DirCacheEntry(longPath);
 		final DirCacheEntry shortEnt = new DirCacheEntry(shortPath);
+
+		longEnt.setFileMode(FileMode.REGULAR_FILE);
+		shortEnt.setFileMode(FileMode.REGULAR_FILE);
+
 		assertEquals(longPath, longEnt.getPathString());
 		assertEquals(shortPath, shortEnt.getPathString());
 
diff --git a/org.spearce.jgit.test/tst/org/spearce/jgit/dircache/DirCacheTreeTest.java b/org.spearce.jgit.test/tst/org/spearce/jgit/dircache/DirCacheTreeTest.java
index aca0b90..6efa207 100644
--- a/org.spearce.jgit.test/tst/org/spearce/jgit/dircache/DirCacheTreeTest.java
+++ b/org.spearce.jgit.test/tst/org/spearce/jgit/dircache/DirCacheTreeTest.java
@@ -40,6 +40,7 @@
 import java.io.IOException;
 
 import org.spearce.jgit.errors.CorruptObjectException;
+import org.spearce.jgit.lib.FileMode;
 import org.spearce.jgit.lib.RepositoryTestCase;
 
 public class DirCacheTreeTest extends RepositoryTestCase {
@@ -75,8 +76,10 @@ public void testSingleSubtree() throws Exception {
 
 		final String[] paths = { "a.", "a/b", "a/c", "a/d", "a0b" };
 		final DirCacheEntry[] ents = new DirCacheEntry[paths.length];
-		for (int i = 0; i < paths.length; i++)
+		for (int i = 0; i < paths.length; i++) {
 			ents[i] = new DirCacheEntry(paths[i]);
+			ents[i].setFileMode(FileMode.REGULAR_FILE);
+		}
 		final int aFirst = 1;
 		final int aLast = 3;
 
@@ -110,8 +113,10 @@ public void testTwoLevelSubtree() throws Exception {
 
 		final String[] paths = { "a.", "a/b", "a/c/e", "a/c/f", "a/d", "a0b" };
 		final DirCacheEntry[] ents = new DirCacheEntry[paths.length];
-		for (int i = 0; i < paths.length; i++)
+		for (int i = 0; i < paths.length; i++) {
 			ents[i] = new DirCacheEntry(paths[i]);
+			ents[i].setFileMode(FileMode.REGULAR_FILE);
+		}
 		final int aFirst = 1;
 		final int aLast = 4;
 		final int acFirst = 2;
@@ -167,8 +172,10 @@ public void testWriteReadTree() throws CorruptObjectException, IOException {
 		final String B = String.format("b%2000s", "b");
 		final String[] paths = { A + ".", A + "." + B, A + "/" + B, A + "0" + B };
 		final DirCacheEntry[] ents = new DirCacheEntry[paths.length];
-		for (int i = 0; i < paths.length; i++)
+		for (int i = 0; i < paths.length; i++) {
 			ents[i] = new DirCacheEntry(paths[i]);
+			ents[i].setFileMode(FileMode.REGULAR_FILE);
+		}
 
 		final DirCacheBuilder b = dc.builder();
 		for (int i = 0; i < ents.length; i++)
diff --git a/org.spearce.jgit/src/org/spearce/jgit/dircache/DirCacheBuilder.java b/org.spearce.jgit/src/org/spearce/jgit/dircache/DirCacheBuilder.java
index aee12fb..8acb3d0 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/dircache/DirCacheBuilder.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/dircache/DirCacheBuilder.java
@@ -41,7 +41,6 @@
 import java.util.Arrays;
 
 import org.spearce.jgit.lib.AnyObjectId;
-import org.spearce.jgit.lib.FileMode;
 import org.spearce.jgit.lib.Repository;
 import org.spearce.jgit.lib.WindowCursor;
 import org.spearce.jgit.treewalk.AbstractTreeIterator;
@@ -90,8 +89,13 @@ protected DirCacheBuilder(final DirCache dc, final int ecnt) {
 	 *
 	 * @param newEntry
 	 *            the new entry to add.
+	 * @throws IllegalArgumentException
+	 *             If the FileMode of the entry was not set by the caller.
 	 */
 	public void add(final DirCacheEntry newEntry) {
+		if (newEntry.getRawMode() == 0)
+			throw new IllegalArgumentException("FileMode not set for path "
+					+ newEntry.getPathString());		
 		beforeAdd(newEntry);
 		fastAdd(newEntry);
 	}
@@ -187,8 +191,6 @@ public void finish() {
 	}
 
 	private void beforeAdd(final DirCacheEntry newEntry) {
-		if (FileMode.TREE.equals(newEntry.getRawMode()))
-			throw bad(newEntry, "Adding subtree not allowed");
 		if (sorted && entryCnt > 0) {
 			final DirCacheEntry lastEntry = entries[entryCnt - 1];
 			final int cr = DirCache.cmp(lastEntry, newEntry);
diff --git a/org.spearce.jgit/src/org/spearce/jgit/dircache/DirCacheEditor.java b/org.spearce.jgit/src/org/spearce/jgit/dircache/DirCacheEditor.java
index 10b554e..6eaceb7 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/dircache/DirCacheEditor.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/dircache/DirCacheEditor.java
@@ -138,11 +138,16 @@ private void applyEdits() {
 			}
 
 			final DirCacheEntry ent;
-			if (missing)
+			if (missing) {
 				ent = new DirCacheEntry(e.path);
-			else
+				e.apply(ent);
+				if (ent.getRawMode() == 0)
+					throw new IllegalArgumentException("FileMode not set"
+							+ " for path " + ent.getPathString());							
+			} else {
 				ent = cache.getEntry(eIdx);
-			e.apply(ent);
+				e.apply(ent);
+			}
 			fastAdd(ent);
 		}
 
diff --git a/org.spearce.jgit/src/org/spearce/jgit/dircache/DirCacheEntry.java b/org.spearce.jgit/src/org/spearce/jgit/dircache/DirCacheEntry.java
index d7abd6e..872ef33 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/dircache/DirCacheEntry.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/dircache/DirCacheEntry.java
@@ -377,11 +377,21 @@ public FileMode getFileMode() {
 
 	/**
 	 * Set the file mode for this entry.
-	 *
+	 * 
 	 * @param mode
 	 *            the new mode constant.
+	 * @throws IllegalArgumentException
+	 *             If {@code mode} is {@link FileMode#MISSING},
+	 *             {@link FileMode#TREE}, or any other type code not permitted
+	 *             in a tree object.
 	 */
 	public void setFileMode(final FileMode mode) {
+		switch (mode.getBits() & FileMode.TYPE_MASK) {
+		case FileMode.TYPE_MISSING:
+		case FileMode.TYPE_TREE:
+			throw new IllegalArgumentException("Invalid mode " + mode
+					+ " for path " + getPathString());
+		}
 		NB.encodeInt32(info, infoOffset + P_MODE, mode.getBits());
 	}
 
diff --git a/org.spearce.jgit/src/org/spearce/jgit/dircache/DirCacheTree.java b/org.spearce.jgit/src/org/spearce/jgit/dircache/DirCacheTree.java
index 79e95cb..2f2a5ed 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/dircache/DirCacheTree.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/dircache/DirCacheTree.java
@@ -376,10 +376,6 @@ private int computeSize(final DirCacheEntry[] cache, int cIdx,
 			}
 
 			final FileMode mode = e.getFileMode();
-			if (mode.getObjectType() == Constants.OBJ_BAD)
-				throw new IllegalStateException("Entry \"" + e.getPathString()
-						+ "\" has incorrect mode set up.");
-
 			size += mode.copyToLength();
 			size += ep.length - pathOffset;
 			size += OBJECT_ID_LENGTH + 2;
-- 
1.6.5.rc0.164.g5f6b0

^ 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