Git development
 help / color / mirror / Atom feed
* Re: tr/pty-all (Re: What's cooking in git.git (Dec 2011, #04; Tue, 13))
From: Junio C Hamano @ 2011-12-15  3:23 UTC (permalink / raw)
  To: Jeff King
  Cc: Jonathan Nieder, Thomas Rast, git,
	Nguyễn Thái Ngọc Duy, Michael Haggerty
In-Reply-To: <20111215005057.GB2566@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

> This correctly detects the bug in t7006. I can't decide if it's clever
> or ugly.

I would say it falls on the latter side of the line by small margin. Let's
do the /dev/null thing and be done with it.

^ permalink raw reply

* Re: [msysGit] [PATCH] gitk: fix the display of files when filtered by path
From: David Aguilar @ 2011-12-15  4:18 UTC (permalink / raw)
  To: Pat Thoyts; +Cc: Git, Paul Mackerras, msysGit, Johannes Schindelin
In-Reply-To: <87hb14wg4l.fsf@fox.patthoyts.tk>

On Tue, Dec 13, 2011 at 8:50 AM, Pat Thoyts
<patthoyts@users.sourceforge.net> wrote:
> Launching 'gitk -- .' or 'gitk -- ..\t' restricts the display to files
> under the given directory but the file list is left empty. This is because
> the path_filter function fails to match the filenames which are relative
> to the working tree to the filter which is filessytem relative.
> This solves the problem by making both names fully qualified filesystem
> paths before performing the comparison.
>
> Signed-off-by: Pat Thoyts <patthoyts@users.sourceforge.net>

Wonderful!

I've run into this problem a number of times (as have some co-workers)
but I never bothered to report it since I felt guilty for never having
worked up a patch.

I tested this and it worked.

FWIW,

Tested-by: David Aguilar <davvid@gmail.com>


Thank you Pat!
-- 
            David

^ permalink raw reply

* Re: [bug?] checkout -m doesn't work without a base version
From: Miles Bader @ 2011-12-15  4:20 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Michael Schubert, Pete Harlan, git
In-Reply-To: <7vhb13qbs6.fsf@alter.siamese.dyndns.org>

Junio C Hamano <gitster@pobox.com> writes:
>> builtin/checkout.c: In function ‘cmd_checkout’:
>> builtin/checkout.c:210:5: warning: ‘mode’ may be used uninitialized
>> in this function [-Wuninitialized]
>> builtin/checkout.c:160:11: note: ‘mode’ was declared here
>
> Isn't this just your gcc being overly cautious (aka "silly")?
>
> The variable "mode" is assigned to when we see an stage #2 entry in the
> loop, and we should have updated threeway[1] immediately before doing so.
> If threeway[1] is not updated, we would have already returned before using
> the variable in make_cache_entry().

Maybe that is actually guaranteed (I dunno), but it's certainly not
obvious from the code here, even to a human... any guarantee would
have to come from external invariants that the compiler doesn't know
about.

Given that, I think it's a fair warning, certainly not "silly."  This
aspect of the code doesn't seem easy to understand...

-Miles

-- 
`To alcohol!  The cause of, and solution to,
 all of life's problems' --Homer J. Simpson

^ permalink raw reply

* Re: How to commit incomplete changes?
From: Alexey Shumkin @ 2011-12-15  6:44 UTC (permalink / raw)
  To: Hallvard B Furuseth; +Cc: git
In-Reply-To: <4cfc9cf0515b1bc751f6aa0de4f55e2a@ulrik.uio.no>

>  Do people have any feelings or conventions for how and when to
> publish a series of commits where the first one(s) break something
> and the next ones clear it up?
I'm curiuos, why to you want to commit changes that break something
separately from fixup?

Many conventions (as I know) use ideology that every commit must NOT
BREAK existing code or tests. Every SHARED commit. Git design (as you
must be already know) allows you to make/change/reorder as many commits
as you want (before you share them or push to a "central" repository).
So, you have not to be afraid to commit every your change, because you
can always change/fixup/split your commits.

Usually, you introduce a feature in a branch. Also, your project must
have (?) (mine do have) unit-tests, at least. And most changes must be
tested. So, breakage must be discovered early, even after some other
commits in that feature branch. In that case you can just make a fixup
commit and then rebase it on a breakage commit with a "squash".

And only after all features made and all tests passed you can share
them (push to another repo).

>I've found some discussion, but with vague results.
> 
>  I'm about to commit some small edits which go together with bigger
>  generated changes.  It seems both more readable and more cherry-pick-
>  friendly to me to keep these in separate commits.
> 
>  What I've found is I can use a line in the commit message like
>      "Incomplete change, requires next commit (update foo/ dir)."
>  and, if there is any point, do a no-ff merge past the breakage.
> 

^ permalink raw reply

* Re: tr/pty-all (Re: What's cooking in git.git (Dec 2011, #04; Tue, 13))
From: Jeff King @ 2011-12-15  6:55 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Jonathan Nieder, Thomas Rast, git,
	Nguyễn Thái Ngọc Duy, Michael Haggerty
In-Reply-To: <7vfwgmplgd.fsf@alter.siamese.dyndns.org>

On Wed, Dec 14, 2011 at 07:23:14PM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > This correctly detects the bug in t7006. I can't decide if it's clever
> > or ugly.
> 
> I would say it falls on the latter side of the line by small margin. Let's
> do the /dev/null thing and be done with it.

Darn, I wanted to post it on my fridge with an "A+".

Here's a cleaned-up version of the /dev/null one.

-- >8 --
Subject: [PATCH] test-lib: redirect stdin of tests

We want to run tests in a predictable, sterile environment
so we can get repeatable results.  They should take as
little input as possible from the environment outside the
test script. We already sanitize environment variables, but
leave stdin untouched. This means that scripts can
accidentally be impacted by content on stdin, or whether
stdin isatty().

Furthermore, scripts reading from stdin can be annoying to
outer loops which care about their stdin offset, like:

  while read sha1; do
      make test
  done

A test which accidentally reads stdin would soak up all of
the rest of the input intended for the outer shell loop.

Let's redirect stdin from /dev/null, which solves both
of these problems. It won't detect tests accidentally
reading from stdin, but since doing so now gives a
deterministic result, we don't need to consider that an
error.

We'll also leave file descriptor 6 as a link to the original
stdin. Tests shouldn't need to look at this, but it can be
convenient for inserting interactive commands while
debugging tests (e.g., you could insert "bash <&6 >&3 2>&4"
to run interactive commands in the environment of the test
script).

Signed-off-by: Jeff King <peff@peff.net>
---
I went the "redirect each test individually" route. In the course of my
experimentation, I notice that some tests (e.g., t4013) will do:

  while read x; do
          test_expect_success "test something ($x)" "
            ... do some test involving $x ...
          "
  done <<\EOF
  ... some values of $x ....
  EOF

This protects those loops from accidental stdin-readers inside the test
scripts, too.

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

diff --git a/t/test-lib.sh b/t/test-lib.sh
index bdd9513..5ea9fe3 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -191,6 +191,7 @@ then
 fi
 
 exec 5>&1
+exec 6<&0
 if test "$verbose" = "t"
 then
 	exec 4>&2 3>&1
@@ -469,7 +470,7 @@ test_debug () {
 test_eval_ () {
 	# This is a separate function because some tests use
 	# "return" to end a test_expect_success block early.
-	eval >&3 2>&4 "$*"
+	eval </dev/null >&3 2>&4 "$*"
 }
 
 test_run_ () {
-- 
1.7.8.rc2.30.g803b1a

^ permalink raw reply related

* Re: [BUG] in rev-parse
From: Jeff King @ 2011-12-15  7:05 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: nathan.panike, git
In-Reply-To: <7vk45yplkm.fsf@alter.siamese.dyndns.org>

On Wed, Dec 14, 2011 at 07:20:41PM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > On the other hand, it has been like this since it was introduced in
> > 2006, and I wonder if scripts rely on the --verify side effect.
> 
> It would have been nicer if it did not to imply --verify at all; a long
> hexdigit that do not name an existing object at all will be shortened to
> its prefix that still do not collide with an abbreviated object name of an
> existing object, and even in such a case, the command should not error out
> only because it was fed a non-existing object (of course, if "--verify" is
> given at the same time, its "one input that names existing object only"
> rule should also kick in).

Dropping the implied verify is easy (see below). But handling
non-existant sha1s is a much more complicated change, as the regular
abbreviation machinery assumes that they exist. E.g., with the patch
below:

  $ good=73c6b3575bc638b7096ec913bd91193707e2265d
  $ bad=${good#d}e
  $ git rev-parse --short $good
  73c6b35
  $ git rev-parse --short $bad
  [no output]

Anyway, I'm not sure it's worth changing at this point. It's part of the
plumbing API that has been that way forever, it's kind of a rare thing
to ask for, and I've already shown a workaround using rev-list.

-Peff

---
diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index 98d1cbe..b365ca0 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -545,7 +545,6 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 			if (!strcmp(arg, "--short") ||
 			    !prefixcmp(arg, "--short=")) {
 				filter &= ~(DO_FLAGS|DO_NOREV);
-				verify = 1;
 				abbrev = DEFAULT_ABBREV;
 				if (arg[7] == '=')
 					abbrev = strtoul(arg + 8, NULL, 10);

^ permalink raw reply related

* Re: How to commit incomplete changes?
From: Hallvard B Furuseth @ 2011-12-15  7:11 UTC (permalink / raw)
  To: Alexey Shumkin; +Cc: git
In-Reply-To: <20111215104444.783303cf@ashu.dyn1.rarus.ru>

 On Thu, 15 Dec 2011 10:44:44 +0400, Alexey Shumkin 
 <Alex.Crezoff@gmail.com> wrote:
>> Do people have any feelings or conventions for how and when to
>> publish a series of commits where the first one(s) break something
>> and the next ones clear it up?
>
> I'm curiuos, why to you want to commit changes that break something
> separately from fixup?

 I answered that, but maybe too briefly:

>>  I'm about to commit some small edits which go together with bigger
>>  generated changes.  It seems both more readable and more 
>> cherry-pick-
>>  friendly to me to keep these in separate commits.

 To expand on that: To review the change, review the hand-edited 
 commits,
 which is easier when these do not drown in generated changes.  Review
 the *commands* which generated the rest - I'd put those in the commit
 message - and glance at the actual changes.  Cherry-pick: Possbly you
 need to run the commands instead of cherry-picking the generated
 changes.  That's easier with a commit with only generated changes.

 I know it also can cause problems.  Would you make a single big commit
 anyway, and describe carefully in the commit message which parts are
 hand-edits?  (We don't auto-test commits yet, but I'll sure this issue
 will crop up again later when we do.)

-- 
 Hallvard

^ permalink raw reply

* Re: process committed files in post-receive hook
From: Jeff King @ 2011-12-15  7:23 UTC (permalink / raw)
  To: Hao Wang; +Cc: Neal Kreitzinger, git
In-Reply-To: <4EE95523.9030702@gmail.com>

On Wed, Dec 14, 2011 at 06:02:11PM -0800, Hao Wang wrote:

> Thank you all for providing the options. Just so you know I finally
> went with Alexey's suggestion. I used 'git show' to get both a list
> of files in a directory and the content of each file. It works great
> on a bare repository so there is no need to check out a copy on the
> server.

If you are scripting, we usually encourage the use of "plumbing"
commands whose output is guaranteed not to change ("show" is a
"porcelain" command intended to be used by end-users, and it's possible
that its behavior might change from version to version).

The plumbing command to get a directory listing for a tree is "git
ls-tree" (try the "--name-only" option for terse output, and use "-z" if
you want to be robust in the face of filenames with funny characters).

> # get a list of rule files using git show
> def getRuleFileList(rev):
>     # run git show
>     p = subprocess.Popen(['git', 'show', rev], stdout=subprocess.PIPE)
>     p.wait()
>     if p.returncode != 0: return None # error
> 
>     # parse output
>     i = 0
>     filelist = []
>     for line in p.stdout.readlines():
>         filelist.append(line)
>     p.stdout.close()
>     return filelist

Doesn't this put "tree HEAD:foo", as printed by "git show", at the top
of your filelist? Another reason to use ls-tree.

> # read the content of a file
> def readfile(rev):
>     # run git show
>     p = subprocess.Popen(['git', 'show', rev], stdout=subprocess.PIPE)
>     p.wait()
>     if p.returncode != 0: return None # error
>     return p.stdout.read()

The plumbing for this is "git cat-file blob ...".

-Peff

^ permalink raw reply

* Re: [PATCH 2/2] push: --ignore-stale option
From: Jeff King @ 2011-12-15  7:38 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git
In-Reply-To: <7vobvct58u.fsf@alter.siamese.dyndns.org>

On Tue, Dec 13, 2011 at 03:35:13PM -0800, Junio C Hamano wrote:

> Teach a new "--ignore-stale" option to "git push" which tells it not to
> push stale refs (i.e. the commit that would have been pushed without the
> option is an ancestor of the commit that is at the destination). With this,
> a lazy workflow could be like this:
> 
>     $ git clone <<origin>>
>     $ git checkout -b topic1 origin/topic1
>     $ work work work
>     $ git push origin :
>     $ git checkout -b topic2 origin/topic2
>     $ work work work
>     $ git push --ignore-stale origin :
> 
> and the second push does not have to worry about other people working on
> topic1 and updating it in the central repository while you haven't touched
> the corresponding local branch at all.

Unless the action of the people in the central repo was to rewind
history, in which case you are overwriting their work. Probably
unlikely, though.

But I still see this as solving the wrong problem, and encouraging a
dangerous workflow. Why are you using ":" to push matching branches if
you don't want to push topic1? I think a much more likely scenario is:

  $ git clone <<origin>>
  $ git checkout -b topic1 origin/topic1
  $ work work work
  $ : hmm, it's not ready yet
  $ git checkout -b topic2 origin/topic2
  $ work work work
  $ : looking good, let's ship it!
  $ git push  ;# we default to matching!

I.e., the user is not actually intending to push all matching branches,
but does so accidentally because "matching" is the default. And they
have accidentally just pushed non-ready work to topic1, which would
happen even with --ignore-stale.

IOW, I am not convinced that people are actually consciously choosing
the workflow you outlined above, and are instead interested in
--ignore-stale as a convenient way of guessing which branches should be
pushed during a matching push. But it's only a guess, and as I showed
above, it can still cause problems. I think the right solution is to
switch the default away from matching, which is the root cause of the
confusion.

-Peff

^ permalink raw reply

* Re: process committed files in post-receive hook
From: Hao Wang @ 2011-12-15  8:19 UTC (permalink / raw)
  To: Jeff King; +Cc: Neal Kreitzinger, git
In-Reply-To: <20111215072301.GC1327@sigill.intra.peff.net>


> If you are scripting, we usually encourage the use of "plumbing"
> commands whose output is guaranteed not to change ("show" is a
> "porcelain" command intended to be used by end-users, and it's possible
> that its behavior might change from version to version).
>
> The plumbing command to get a directory listing for a tree is "git
> ls-tree" (try the "--name-only" option for terse output, and use "-z" if
> you want to be robust in the face of filenames with funny characters).

Jeff, thank you for the information. This is really helpful.

>> # get a list of rule files using git show
>> def getRuleFileList(rev):
>>      # run git show
>>      p = subprocess.Popen(['git', 'show', rev], stdout=subprocess.PIPE)
>>      p.wait()
>>      if p.returncode != 0: return None # error
>>
>>      # parse output
>>      i = 0
>>      filelist = []
>>      for line in p.stdout.readlines():
>>          filelist.append(line)
>>      p.stdout.close()
>>      return filelist
>
> Doesn't this put "tree HEAD:foo", as printed by "git show", at the top
> of your filelist? Another reason to use ls-tree.

Yes, the first two items ("tree HEAD:foo" and an empty line) are removed 
later from filelist.

Hao

^ permalink raw reply

* Re: [PATCH v2 28/51] refs.c: rename ref_array -> ref_dir
From: Michael Haggerty @ 2011-12-15  8:19 UTC (permalink / raw)
  To: Jeff King
  Cc: Junio C Hamano, git, Jakub Narebski, Heiko Voigt, Johan Herland,
	Julian Phillips, Drew Northup
In-Reply-To: <20111214023320.GA22141@sigill.intra.peff.net>

On 12/14/2011 03:33 AM, Jeff King wrote:
> On Tue, Dec 13, 2011 at 04:19:19PM -0800, Junio C Hamano wrote:
>> Actually, I do not think it even needs to be the "extra *REF* API". The
>> only thing that matters is that these commits are considered to be extra
>> anchor point in the history, in addition to the usual rule of considering
>> that everything reachable from our refs is complete. The data structure to
>> hold them does not even have to be a "struct ref". Just an array of object
>> names (or "struct object *") should suffice.
> 
> Since my cff38a5 (receive-pack: eliminate duplicate .have refs,
> 2011-05-19), receive-pack simply has a packed array of binary sha1s (in
> a "struct sha1_array" object). That might be the simplest thing.

It was pretty easy to eliminate the use of extra_refs from receive-pack.
 I'll submit the patches as soon as I can.  The patches will be based
off of patch 16/51 of the ref-api-D series, since Junio indicated that
he wants to queue up those commits (let me know if you have a different
preference).

Now I'm looking at the uses of extra_refs in git-clone.  One thing it
does is add some extra refs then write them to the packed-refs file.  I
still have to dig into it, but this seems strange.  If the refs are
being written to packed-refs, it seems like they must be real (not
extra) refs, or perhaps are just about to become real refs as part of
the clone.  Or is something more egregious is going on?

Moving the pack_refs() function to refs.c was on the agenda anyway, so
it should be possible to sort this out at the same time.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

^ permalink raw reply

* Re: How to commit incomplete changes?
From: Alexey Shumkin @ 2011-12-15  8:22 UTC (permalink / raw)
  To: Hallvard B Furuseth; +Cc: git
In-Reply-To: <7e1ccfac8c47e8877c0438086bd1d91b@ulrik.uio.no>

Oh! I got it. I missed "generated changes".
Well, unfortunately (or fortunately ;) ), I did not meet such a workflow
when changes are "generated" without my hands.
In your case it may sound reasonable to make separate fixup commit.
But Git allows you to make your own (more flexible than SVN,
for instance) workflow, which suits you. It's up to you ;)
You decide.
If you plan cherry-picking that fixups, do separate fixups. Just
publish them together.
If you want every commit is "clear" and "workable", squash fixup into
a single commit.
I do not know what exactly is "generated changes" you're talking
about ), so, maybe I'd do separate fixups, maybe not. ))
There is no single solution. ))) TIMTOWTDI
That is why you hesitate :) Do your own decision. And feel free to
change it later. ))

>  To expand on that: To review the change, review the hand-edited 
>  commits,
>  which is easier when these do not drown in generated changes.  Review
>  the *commands* which generated the rest - I'd put those in the commit
>  message - and glance at the actual changes.  Cherry-pick: Possbly you
>  need to run the commands instead of cherry-picking the generated
>  changes.  That's easier with a commit with only generated changes.
> 
>  I know it also can cause problems.  Would you make a single big
> commit anyway, and describe carefully in the commit message which
> parts are hand-edits?  (We don't auto-test commits yet, but I'll sure
> this issue will crop up again later when we do.)
> 

^ permalink raw reply

* Re: [PATCH v2 28/51] refs.c: rename ref_array -> ref_dir
From: Jeff King @ 2011-12-15  8:37 UTC (permalink / raw)
  To: Michael Haggerty
  Cc: Junio C Hamano, git, Jakub Narebski, Heiko Voigt, Johan Herland,
	Julian Phillips, Drew Northup
In-Reply-To: <4EE9AD7C.2050605@alum.mit.edu>

On Thu, Dec 15, 2011 at 09:19:08AM +0100, Michael Haggerty wrote:

> Now I'm looking at the uses of extra_refs in git-clone.  One thing it
> does is add some extra refs then write them to the packed-refs file.  I
> still have to dig into it, but this seems strange.  If the refs are
> being written to packed-refs, it seems like they must be real (not
> extra) refs, or perhaps are just about to become real refs as part of
> the clone.  Or is something more egregious is going on?

I think what is happening is that the extra_refs system is doing double
duty here. If you do:

  git clone --reference $local $origin

then the code does something like:

  1. Add all of the refs in $local are added to extra_refs, because we
     want to advertise them during the pack fetch (see add_one_reference,
     called through setup_reference at l. 626).

  2. We then get the list of remote refs from $origin and store them in
     mapped_refs (ll. 657-658). The fetch uses the extra refs from (1).

  3. We call clear_extra_refs to drop those temporary refs (l. 663).

  4. We call write_remote_refs (l. 664), which adds all of the
     mapped_refs from (2) to extra_refs, then calls pack_refs which will
     pack all of the existing refs (of which there should be none, I
     would think), including extras (i.e., all the stuff we just
     cloned).

     Afterwards we clear_extra_refs again, though I think that is
     probably unnecessary.

Keeping the ".have" sha1s out of extra_refs would make this a lot
clearer.  Arguably it would also be better to have some way to just pass
the set of mapped_refs to pack_refs, instead of shoving them in the
global extra_refs and relying on pack_refs to find them there.

-Peff

^ permalink raw reply

* Re: How to commit incomplete changes?
From: Alexey Shumkin @ 2011-12-15  8:39 UTC (permalink / raw)
  To: Hallvard B Furuseth; +Cc: git
In-Reply-To: <20111215122252.584d1003@ashu.dyn1.rarus.ru>

> Do your own decision. 
* "Make your own decision", of course :)

^ permalink raw reply

* Re: [msysGit] [PATCH] gitk: fix the display of files when filtered by path
From: Johannes Schindelin @ 2011-12-15  9:24 UTC (permalink / raw)
  To: David Aguilar; +Cc: Pat Thoyts, Git, Paul Mackerras, msysGit
In-Reply-To: <CAJDDKr6rVaX_=SZZeEAs950yuNDvi8sOkzrUK7LnCrK6MYfscg@mail.gmail.com>

Hi,

On Wed, 14 Dec 2011, David Aguilar wrote:

> On Tue, Dec 13, 2011 at 8:50 AM, Pat Thoyts
> <patthoyts@users.sourceforge.net> wrote:
> > Launching 'gitk -- .' or 'gitk -- ..\t' restricts the display to files
> > under the given directory but the file list is left empty. This is because
> > the path_filter function fails to match the filenames which are relative
> > to the working tree to the filter which is filessytem relative.
> > This solves the problem by making both names fully qualified filesystem
> > paths before performing the comparison.
> >
> > Signed-off-by: Pat Thoyts <patthoyts@users.sourceforge.net>
> 
> Wonderful!

Thanks for reminding me that I did not yet apply and push. Did so now.

Thanks, Pat, for fixing this bug!
Dscho

^ permalink raw reply

* Re: [bug?] checkout -m doesn't work without a base version
From: Michael Schubert @ 2011-12-15 10:11 UTC (permalink / raw)
  To: Miles Bader; +Cc: Junio C Hamano, Pete Harlan, git
In-Reply-To: <buoipli8nzy.fsf@dhlpc061.dev.necel.com>

On 12/15/2011 05:20 AM, Miles Bader wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>>> builtin/checkout.c: In function ‘cmd_checkout’:
>>> builtin/checkout.c:210:5: warning: ‘mode’ may be used uninitialized
>>> in this function [-Wuninitialized]
>>> builtin/checkout.c:160:11: note: ‘mode’ was declared here
>>
>> Isn't this just your gcc being overly cautious (aka "silly")?
>>
>> The variable "mode" is assigned to when we see an stage #2 entry in the
>> loop, and we should have updated threeway[1] immediately before doing so.
>> If threeway[1] is not updated, we would have already returned before using
>> the variable in make_cache_entry().
> 
> Maybe that is actually guaranteed (I dunno), but it's certainly not
> obvious from the code here, even to a human... any guarantee would
> have to come from external invariants that the compiler doesn't know
> about.
> 
> Given that, I think it's a fair warning, certainly not "silly."  This
> aspect of the code doesn't seem easy to understand...

Silly or not: That's what gcc 4.6.x is warning about with -Wuninitialized
(-Wall) set - I didn't set any additional options, just plain `make all`.

^ permalink raw reply

* Re: [bug?] checkout -m doesn't work without a base version
From: Andreas Schwab @ 2011-12-15 10:42 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Michael Schubert, Pete Harlan, git
In-Reply-To: <7vhb13qbs6.fsf@alter.siamese.dyndns.org>

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

> The variable "mode" is assigned to when we see an stage #2 entry in the
> loop, and we should have updated threeway[1] immediately before doing so.
> If threeway[1] is not updated, we would have already returned before using
> the variable in make_cache_entry().

How can you be sure that ce_stage(ce) ever returns 2?

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

^ permalink raw reply

* Re: [RFC PATCH 2/4] test-lib: allow testing another git build tree
From: Thomas Rast @ 2011-12-15 10:33 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Michael Haggerty
In-Reply-To: <7vsjkmpm6g.fsf@alter.siamese.dyndns.org>

On Wednesday 14 December 2011 19:07:35 Junio C Hamano wrote:
> Thomas Rast <trast@student.ethz.ch> writes:
> > The perf-lib work wants this feature, so we may as well do it for
> > test-lib in general.
> 
> How is this different from what GIT_TEST_INSTALLED already gives us
> (other than "needs more diskspace to keep another source tree fully
> built", that is)?

I was scared away by the note that it would use (among others) perl
libs from the current build tree.  Upon investigation I also see that
the test-* situation is still not satisfactory.  Some (like
test-chmtime) are used by the tests for a vital task, and if they ever
have to be fixed, we would want to use the fixed version in any "test
an old git" run.  OTOH, others (e.g., test-dump-cache-tree) are linked
with the rest of the code and serve to test an otherwise not
accessible part of it, and testing an old git should use them from the
tested tree.

The disk space argument is moot IMO: for sane perf testing you need
the extra build tree anyway because you cannot checkout another
version in the current tree.  Otherwise the scripts may change and/or
disappear from under themselves.

An optimization might be to have the run script only use a single
build tree and several install trees.  However, while such a built
tree takes just over 100MB of space in my tests, everything installed
$PREFIX/libexec/git-core is also already 65MB here.  So the latter
scheme would only amortize itself if you had at least 3 trees tested
simultaneously.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

^ permalink raw reply

* Re: [PATCHv2 5/9] add generic terminal prompt function
From: Pete Wyckoff @ 2011-12-15 12:48 UTC (permalink / raw)
  To: Jeff King; +Cc: Junio C Hamano, git
In-Reply-To: <20111210104101.GE16648@sigill.intra.peff.net>

peff@peff.net wrote on Sat, 10 Dec 2011 05:41 -0500:
> +static struct termios old_term;
> +
> +static void restore_term(void)
> +{
> +	if (term_fd < 0)
> +		return;
> +
> +	tcsetattr(term_fd, TCSAFLUSH, &old_term);
> +	term_fd = -1;
> +}

Restores from static old_term.

> +char *git_terminal_prompt(const char *prompt, int echo)
> +{
> +	static struct strbuf buf = STRBUF_INIT;
> +	int r;
> +	FILE *fh;
> +
> +	fh = fopen("/dev/tty", "w+");
> +	if (!fh)
> +		return NULL;
> +
> +	if (!echo) {
> +		struct termios t;
> +
> +		if (tcgetattr(fileno(fh), &t) < 0) {
> +			fclose(fh);
> +			return NULL;
> +		}
> +
> +		old_term = t;

Which is only saved if echo is true.

> +		term_fd = fileno(fh);
> +		sigchain_push_common(restore_term_on_signal);
> +
> +		t.c_lflag &= ~ECHO;
> +		if (tcsetattr(fileno(fh), TCSAFLUSH, &t) < 0) {
> +			term_fd = -1;
> +			fclose(fh);
> +			return NULL;
> +		}
> +	}
> +
> +	fputs(prompt, fh);
> +	fflush(fh);
> +
> +	r = strbuf_getline(&buf, fh, '\n');
> +	if (!echo) {
> +		putc('\n', fh);
> +		fflush(fh);
> +	}
> +
> +	restore_term();

Perhaps this line should go in !echo.

And why no sigchain_pop() for the signal handler?

		-- Pete

^ permalink raw reply

* Re: [PATCHv2 5/9] add generic terminal prompt function
From: Jeff King @ 2011-12-15 13:39 UTC (permalink / raw)
  To: Pete Wyckoff; +Cc: Junio C Hamano, git
In-Reply-To: <20111215124851.GA6907@padd.com>

On Thu, Dec 15, 2011 at 07:48:51AM -0500, Pete Wyckoff wrote:

> peff@peff.net wrote on Sat, 10 Dec 2011 05:41 -0500:
> > +static struct termios old_term;
> > +
> > +static void restore_term(void)
> > +{
> > +	if (term_fd < 0)
> > +		return;
> > +
> > +	tcsetattr(term_fd, TCSAFLUSH, &old_term);
> > +	term_fd = -1;
> > +}
> 
> Restores from static old_term.

Right. But note that it is protected by term_fd being set.

> > +char *git_terminal_prompt(const char *prompt, int echo)
> > +{
> > +	static struct strbuf buf = STRBUF_INIT;
> > +	int r;
> > +	FILE *fh;
> > +
> > +	fh = fopen("/dev/tty", "w+");
> > +	if (!fh)
> > +		return NULL;
> > +
> > +	if (!echo) {
> > +		struct termios t;
> > +
> > +		if (tcgetattr(fileno(fh), &t) < 0) {
> > +			fclose(fh);
> > +			return NULL;
> > +		}
> > +
> > +		old_term = t;
> 
> Which is only saved if echo is true.

Yes, but just below:

> > +		term_fd = fileno(fh);
> > +		sigchain_push_common(restore_term_on_signal);

We set up term_fd, and then:

> > +		t.c_lflag &= ~ECHO;
> > +		if (tcsetattr(fileno(fh), TCSAFLUSH, &t) < 0) {
> > +			term_fd = -1;
> > +			fclose(fh);
> > +			return NULL;
> > +		}

On error, disable it again.

> > +	}
> > +
> > +	fputs(prompt, fh);
> > +	fflush(fh);
> > +
> > +	r = strbuf_getline(&buf, fh, '\n');
> > +	if (!echo) {
> > +		putc('\n', fh);
> > +		fflush(fh);
> > +	}
> > +
> > +	restore_term();
> 
> Perhaps this line should go in !echo.

It could, but it's a no-op as-is, as term_fd will be -1.

I agree it might be a little more obvious to put it there (I think what
happened is my initial revision did not look at "echo" ever again, and
then that conditional was added later when I realized that the "!echo"
case needed us to print the newline manually).

> And why no sigchain_pop() for the signal handler?

Because I used sigchain_push_common, which has no pop_common analog. But
it's OK, because calling restore_term sets term_fd to -1, making further
calls a no-op. So leaving the handler in place is fine.

Another option would be to add sigchain_pop_common, which pops the
same signals from push_common.

-Peff

^ permalink raw reply

* [PATCH v2 1/3] merge: abort if fails to commit
From: Nguyễn Thái Ngọc Duy @ 2011-12-15 13:47 UTC (permalink / raw)
  To: git
  Cc: Jeff King, Miles Bader, Junio C Hamano,
	Nguyễn Thái Ngọc Duy
In-Reply-To: <1323871699-8839-2-git-send-email-pclouds@gmail.com>


Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 2011/12/15 Junio C Hamano <gitster@pobox.com>:
 >> -     commit_tree(merge_msg.buf, merge_msg.len, result_tree, parent, result_commit, NULL);
 >> +     if (commit_tree(merge_msg.buf, merge_msg.len,
 >> +                     result_tree, parent, result_commit, NULL))
 >> +             die(_("failed to write commit object"));
 >>       finish(head, result_commit, "In-index merge");
 >>       drop_save();
 >>       return 0;
 >
 > Should we die immediately, or should we do some clean-ups after ourselves
 > before doing so?

 I'm not sure. I had a quick look over the command and it seems we do
 not need to do any clean-ups. But I'm not familiar with the command
 anyway..

 > In any case, this is a good change that shouldn't be taken hostage to the
 > unrelated change in patch [1/3].

 Moved it up so it you can cherry-pick it independently.

 builtin/merge.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/builtin/merge.c b/builtin/merge.c
index 2870a6a..27576c0 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -913,7 +913,8 @@ static int merge_trivial(struct commit *head)
 	parent->next->item = remoteheads->item;
 	parent->next->next = NULL;
 	prepare_to_commit();
-	commit_tree(merge_msg.buf, result_tree, parent, result_commit, NULL);
+	if (commit_tree(merge_msg.buf, result_tree, parent, result_commit, NULL))
+		die(_("failed to write commit object"));
 	finish(head, result_commit, "In-index merge");
 	drop_save();
 	return 0;
@@ -944,7 +945,8 @@ static int finish_automerge(struct commit *head,
 	strbuf_addch(&merge_msg, '\n');
 	prepare_to_commit();
 	free_commit_list(remoteheads);
-	commit_tree(merge_msg.buf, result_tree, parents, result_commit, NULL);
+	if (commit_tree(merge_msg.buf, result_tree, parents, result_commit, NULL))
+		die(_("failed to write commit object"));
 	strbuf_addf(&buf, "Merge made by the '%s' strategy.", wt_strategy);
 	finish(head, result_commit, buf.buf);
 	strbuf_release(&buf);
-- 
1.7.8.36.g69ee2

^ permalink raw reply related

* [PATCH v2 2/3] Convert commit_tree() to take strbuf as message
From: Nguyễn Thái Ngọc Duy @ 2011-12-15 13:47 UTC (permalink / raw)
  To: git
  Cc: Jeff King, Miles Bader, Junio C Hamano,
	Nguyễn Thái Ngọc Duy
In-Reply-To: <1323956843-5326-1-git-send-email-pclouds@gmail.com>

Because strbuf provides message length, we can create commits that
include NULs.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/commit-tree.c |    2 +-
 builtin/commit.c      |    2 +-
 builtin/merge.c       |    4 ++--
 builtin/notes.c       |    4 ++--
 commit.c              |    4 ++--
 commit.h              |    2 +-
 notes-cache.c         |    5 ++++-
 notes-merge.c         |   10 ++++++----
 notes-merge.h         |    2 +-
 9 files changed, 20 insertions(+), 15 deletions(-)

diff --git a/builtin/commit-tree.c b/builtin/commit-tree.c
index d083795..0895861 100644
--- a/builtin/commit-tree.c
+++ b/builtin/commit-tree.c
@@ -56,7 +56,7 @@ int cmd_commit_tree(int argc, const char **argv, const char *prefix)
 	if (strbuf_read(&buffer, 0, 0) < 0)
 		die_errno("git commit-tree: failed to read");
 
-	if (commit_tree(buffer.buf, tree_sha1, parents, commit_sha1, NULL)) {
+	if (commit_tree(&buffer, tree_sha1, parents, commit_sha1, NULL)) {
 		strbuf_release(&buffer);
 		return 1;
 	}
diff --git a/builtin/commit.c b/builtin/commit.c
index 8f2bebe..849151e 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1483,7 +1483,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 		exit(1);
 	}
 
-	if (commit_tree(sb.buf, active_cache_tree->sha1, parents, sha1,
+	if (commit_tree(&sb, active_cache_tree->sha1, parents, sha1,
 			author_ident.buf)) {
 		rollback_index_files();
 		die(_("failed to write commit object"));
diff --git a/builtin/merge.c b/builtin/merge.c
index 27576c0..e066bf1 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -913,7 +913,7 @@ static int merge_trivial(struct commit *head)
 	parent->next->item = remoteheads->item;
 	parent->next->next = NULL;
 	prepare_to_commit();
-	if (commit_tree(merge_msg.buf, result_tree, parent, result_commit, NULL))
+	if (commit_tree(&merge_msg, result_tree, parent, result_commit, NULL))
 		die(_("failed to write commit object"));
 	finish(head, result_commit, "In-index merge");
 	drop_save();
@@ -945,7 +945,7 @@ static int finish_automerge(struct commit *head,
 	strbuf_addch(&merge_msg, '\n');
 	prepare_to_commit();
 	free_commit_list(remoteheads);
-	if (commit_tree(merge_msg.buf, result_tree, parents, result_commit, NULL))
+	if (commit_tree(&merge_msg, result_tree, parents, result_commit, NULL))
 		die(_("failed to write commit object"));
 	strbuf_addf(&buf, "Merge made by the '%s' strategy.", wt_strategy);
 	finish(head, result_commit, buf.buf);
diff --git a/builtin/notes.c b/builtin/notes.c
index f8e437d..5e32548 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -301,12 +301,12 @@ void commit_notes(struct notes_tree *t, const char *msg)
 		return; /* don't have to commit an unchanged tree */
 
 	/* Prepare commit message and reflog message */
-	strbuf_addstr(&buf, "notes: "); /* commit message starts at index 7 */
 	strbuf_addstr(&buf, msg);
 	if (buf.buf[buf.len - 1] != '\n')
 		strbuf_addch(&buf, '\n'); /* Make sure msg ends with newline */
 
-	create_notes_commit(t, NULL, buf.buf + 7, commit_sha1);
+	create_notes_commit(t, NULL, &buf, commit_sha1);
+	strbuf_insert(&buf, 0, "notes: ", 7); /* commit message starts at index 7 */
 	update_ref(buf.buf, t->ref, commit_sha1, NULL, 0, DIE_ON_ERR);
 
 	strbuf_release(&buf);
diff --git a/commit.c b/commit.c
index 73b7e00..0a214a6 100644
--- a/commit.c
+++ b/commit.c
@@ -845,7 +845,7 @@ static const char commit_utf8_warn[] =
 "You may want to amend it after fixing the message, or set the config\n"
 "variable i18n.commitencoding to the encoding your project uses.\n";
 
-int commit_tree(const char *msg, unsigned char *tree,
+int commit_tree(const struct strbuf *msg, unsigned char *tree,
 		struct commit_list *parents, unsigned char *ret,
 		const char *author)
 {
@@ -884,7 +884,7 @@ int commit_tree(const char *msg, unsigned char *tree,
 	strbuf_addch(&buffer, '\n');
 
 	/* And add the comment */
-	strbuf_addstr(&buffer, msg);
+	strbuf_addbuf(&buffer, msg);
 
 	/* And check the encoding */
 	if (encoding_is_utf8 && !is_utf8(buffer.buf))
diff --git a/commit.h b/commit.h
index 009b113..5cf46b2 100644
--- a/commit.h
+++ b/commit.h
@@ -181,7 +181,7 @@ static inline int single_parent(struct commit *commit)
 
 struct commit_list *reduce_heads(struct commit_list *heads);
 
-extern int commit_tree(const char *msg, unsigned char *tree,
+extern int commit_tree(const struct strbuf *msg, unsigned char *tree,
 		struct commit_list *parents, unsigned char *ret,
 		const char *author);
 
diff --git a/notes-cache.c b/notes-cache.c
index 4c8984e..bea013e 100644
--- a/notes-cache.c
+++ b/notes-cache.c
@@ -48,6 +48,7 @@ int notes_cache_write(struct notes_cache *c)
 {
 	unsigned char tree_sha1[20];
 	unsigned char commit_sha1[20];
+	struct strbuf msg = STRBUF_INIT;
 
 	if (!c || !c->tree.initialized || !c->tree.ref || !*c->tree.ref)
 		return -1;
@@ -56,7 +57,9 @@ int notes_cache_write(struct notes_cache *c)
 
 	if (write_notes_tree(&c->tree, tree_sha1))
 		return -1;
-	if (commit_tree(c->validity, tree_sha1, NULL, commit_sha1, NULL) < 0)
+	strbuf_attach(&msg, c->validity,
+		      strlen(c->validity), strlen(c->validity) + 1);
+	if (commit_tree(&msg, tree_sha1, NULL, commit_sha1, NULL) < 0)
 		return -1;
 	if (update_ref("update notes cache", c->tree.ref, commit_sha1, NULL,
 		       0, QUIET_ON_ERR) < 0)
diff --git a/notes-merge.c b/notes-merge.c
index ce10aac..b5a36ac 100644
--- a/notes-merge.c
+++ b/notes-merge.c
@@ -530,7 +530,7 @@ static int merge_from_diffs(struct notes_merge_options *o,
 }
 
 void create_notes_commit(struct notes_tree *t, struct commit_list *parents,
-			 const char *msg, unsigned char *result_sha1)
+			 const struct strbuf *msg, unsigned char *result_sha1)
 {
 	unsigned char tree_sha1[20];
 
@@ -668,7 +668,7 @@ int notes_merge(struct notes_merge_options *o,
 		struct commit_list *parents = NULL;
 		commit_list_insert(remote, &parents); /* LIFO order */
 		commit_list_insert(local, &parents);
-		create_notes_commit(local_tree, parents, o->commit_msg.buf,
+		create_notes_commit(local_tree, parents, &o->commit_msg,
 				    result_sha1);
 	}
 
@@ -695,7 +695,8 @@ int notes_merge_commit(struct notes_merge_options *o,
 	struct dir_struct dir;
 	char *path = xstrdup(git_path(NOTES_MERGE_WORKTREE "/"));
 	int path_len = strlen(path), i;
-	const char *msg = strstr(partial_commit->buffer, "\n\n");
+	char *msg = strstr(partial_commit->buffer, "\n\n");
+	struct strbuf sb_msg = STRBUF_INIT;
 
 	if (o->verbosity >= 3)
 		printf("Committing notes in notes merge worktree at %.*s\n",
@@ -733,7 +734,8 @@ int notes_merge_commit(struct notes_merge_options *o,
 				sha1_to_hex(obj_sha1), sha1_to_hex(blob_sha1));
 	}
 
-	create_notes_commit(partial_tree, partial_commit->parents, msg,
+	strbuf_attach(&sb_msg, msg, strlen(msg), strlen(msg) + 1);
+	create_notes_commit(partial_tree, partial_commit->parents, &sb_msg,
 			    result_sha1);
 	if (o->verbosity >= 4)
 		printf("Finalized notes merge commit: %s\n",
diff --git a/notes-merge.h b/notes-merge.h
index 168a672..0c11b17 100644
--- a/notes-merge.h
+++ b/notes-merge.h
@@ -37,7 +37,7 @@ void init_notes_merge_options(struct notes_merge_options *o);
  * The resulting commit SHA1 is stored in result_sha1.
  */
 void create_notes_commit(struct notes_tree *t, struct commit_list *parents,
-			 const char *msg, unsigned char *result_sha1);
+			 const struct strbuf *msg, unsigned char *result_sha1);
 
 /*
  * Merge notes from o->remote_ref into o->local_ref
-- 
1.7.8.36.g69ee2

^ permalink raw reply related

* [PATCH v2 3/3] commit: refuse commit messages that contain NULs
From: Nguyễn Thái Ngọc Duy @ 2011-12-15 13:47 UTC (permalink / raw)
  To: git
  Cc: Jeff King, Miles Bader, Junio C Hamano,
	Nguyễn Thái Ngọc Duy
In-Reply-To: <1323956843-5326-1-git-send-email-pclouds@gmail.com>

Current implementation sees NUL as terminator. If users give a
NUL-included message (e.g. editor accidentally set to save as UTF-16),
the new commit message will have NULs. However following operations
(displaying or amending a commit for example) will not show anything
after the first NUL.

Stop user right when they do this. If NUL is added by mistake, they
have their chance to fix. If they know that they are doing,
commit-tree will gladly commit whatever is given.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Advice stuff dropped. I realized quite late that commit_tree() is
 also used for plumbing commands (also thanks to Junio's comments),
 while I wanted to check at porcelain level only. So I moved the check
 up to builtin/commit.c. If we need the same check for other commands,
 which I doubt, similar checks can be added.

 builtin/commit.c       |    7 +++++++
 t/t3900-i18n-commit.sh |    6 ++++++
 2 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 849151e..5db7673 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1483,6 +1483,13 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 		exit(1);
 	}
 
+	if (memchr(sb.buf, '\0', sb.len)) {
+		rollback_index_files();
+		die(_("your commit message contains NUL characters.\n"
+		      "hint: This is often caused by using wide encodings such as\n"
+		      "hint: UTF-16. Please check your editor settings."));
+	}
+
 	if (commit_tree(&sb, active_cache_tree->sha1, parents, sha1,
 			author_ident.buf)) {
 		rollback_index_files();
diff --git a/t/t3900-i18n-commit.sh b/t/t3900-i18n-commit.sh
index 1f62c15..d48a7c0 100755
--- a/t/t3900-i18n-commit.sh
+++ b/t/t3900-i18n-commit.sh
@@ -34,6 +34,12 @@ test_expect_success 'no encoding header for base case' '
 	test z = "z$E"
 '
 
+test_expect_failure 'UTF-16 refused because of NULs' '
+	echo UTF-16 >F &&
+	git commit -a -F "$TEST_DIRECTORY"/t3900/UTF-16.txt
+'
+
+
 for H in ISO8859-1 eucJP ISO-2022-JP
 do
 	test_expect_success "$H setup" '
-- 
1.7.8.36.g69ee2

^ permalink raw reply related

* [PATCH 0/2] Improve git-bundle builtin
From: Ramkumar Ramachandra @ 2011-12-15 16:45 UTC (permalink / raw)
  To: Git List; +Cc: Jonathan Nieder, Junio C Hamano, Joey Hess
In-Reply-To: <20111208175913.GK2394@elie.hsd1.il.comcast.net>

Hi,

My OPT_SUBCOMMAND idea crashed and burned; I decided to salvage some
of the work that went into improving the git-bundle builtin and put it
into another series along with some additional tests.  I hope this
benefits people who use git-bundle to do incremental backups on their
servers or otherwise.

A couple of thoughts:

1. There's a SP between the OBJID and the ref name in list-heads as
opposed to the TAB used by other git commands such as ls-remote,
diff-tree.  Will fixing it break someone's parser somewhere?

2. Is it worth fixing the "--stdin" tests?  What is the usecase?  A
quick blame points to f62e0a39 (t5704 (bundle): add tests for bundle
--stdin, 2010-04-19): Jonathan, Joey?

Cheers.

-- Ram

Ramkumar Ramachandra (2):
  t5704 (bundle): rewrite for larger coverage
  bundle: rewrite builtin to use parse-options

 builtin/bundle.c  |   91 +++++++++++++++++++++++++++++---------------------
 t/t5704-bundle.sh |   95 ++++++++++++++++++++++++++++++++++++++--------------
 2 files changed, 122 insertions(+), 64 deletions(-)

-- 
1.7.4.1

^ permalink raw reply

* [PATCH 1/2] t5704 (bundle): rewrite for larger coverage
From: Ramkumar Ramachandra @ 2011-12-15 16:45 UTC (permalink / raw)
  To: Git List; +Cc: Jonathan Nieder, Junio C Hamano, Joey Hess
In-Reply-To: <1323967528-10537-1-git-send-email-artagnon@gmail.com>

Rewrite the git-bundle testsuite to exercise more of its
functionality.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 t/t5704-bundle.sh |   95 ++++++++++++++++++++++++++++++++++++++--------------
 1 files changed, 69 insertions(+), 26 deletions(-)

diff --git a/t/t5704-bundle.sh b/t/t5704-bundle.sh
index 728ccd8..09ff4f1 100755
--- a/t/t5704-bundle.sh
+++ b/t/t5704-bundle.sh
@@ -1,56 +1,99 @@
 #!/bin/sh
 
-test_description='some bundle related tests'
+test_description='Test git-bundle'
 . ./test-lib.sh
 
 test_expect_success 'setup' '
+	git config core.logAllRefUpdates false &&
+	test_commit initial &&
+	git checkout -b branch &&
+	test_commit second &&
+	test_commit third &&
+	git checkout master &&
+	test_commit fourth file
+'
 
-	: > file &&
-	git add file &&
-	test_tick &&
-	git commit -m initial &&
-	test_tick &&
-	git tag -m tag tag &&
-	: > file2 &&
-	git add file2 &&
-	: > file3 &&
-	test_tick &&
-	git commit -m second &&
-	git add file3 &&
-	test_tick &&
-	git commit -m third
+test_expect_success 'create succeeds' '
+	git bundle create bundle second third &&
+	cat >expect <<-\EOF &&
+	OBJID	refs/tags/third
+	OBJID	refs/tags/second
+	EOF
+	{
+		git ls-remote bundle |
+		sed "s/$_x40/OBJID/g"
+	} >actual &&
+	test_cmp expect actual
+'
 
+test_expect_success 'verify succeeds' '
+	git bundle create bundle second third &&
+	git bundle verify bundle
 '
 
-test_expect_success 'tags can be excluded by rev-list options' '
+test_expect_success 'list-heads succeeds' '
+	git bundle create bundle second third &&
+	cat >expect <<-\EOF &&
+	OBJID refs/tags/second
+	OBJID refs/tags/third
+	EOF
+	{
+		git bundle list-heads bundle |
+		sed "s/$_x40/OBJID/g"
+	} >actual &&
+	test_cmp expect actual
+'
 
-	git bundle create bundle --all --since=7.Apr.2005.15:16:00.-0700 &&
-	git ls-remote bundle > output &&
-	! grep tag output
+test_expect_success 'create dies on invalid bundle filename' '
+	mkdir adir &&
+	test_expect_code 128 git bundle create adir --all
+'
 
+test_expect_success 'disallow stray command-line options' '
+	test_must_fail git bundle create --junk bundle second third
 '
 
-test_expect_success 'die if bundle file cannot be created' '
+test_expect_failure 'disallow stray command-line arguments' '
+	git bundle create bundle second third &&
+	test_must_fail git bundle verify bundle junk
+'
 
-	mkdir adir &&
-	test_must_fail git bundle create adir --all
+test_expect_success 'create accepts rev-list options to narrow refs' '
+	git bundle create bundle --all -- file &&
+	cat >expect <<-\EOF &&
+	OBJID	HEAD
+	OBJID	refs/tags/fourth
+	OBJID	refs/heads/master
+	EOF
+	{
+		git ls-remote bundle |
+		sed "s/$_x40/OBJID/g"
+	} >actual &&
+	test_cmp expect actual
+'
 
+test_expect_success 'unbundle succeeds' '
+	saved=$(git rev-parse third) &&
+	git bundle create bundle second third fourth &&
+	git tag -d second third fourth &&
+	git branch -D branch &&
+	git reset --hard initial &&
+	git prune &&
+	test_must_fail git reset --hard $saved &&
+	git bundle unbundle bundle &&
+	git reset --hard $saved
 '
 
 test_expect_failure 'bundle --stdin' '
-
 	echo master | git bundle create stdin-bundle.bdl --stdin &&
 	git ls-remote stdin-bundle.bdl >output &&
 	grep master output
-
 '
 
 test_expect_failure 'bundle --stdin <rev-list options>' '
-
 	echo master | git bundle create hybrid-bundle.bdl --stdin tag &&
 	git ls-remote hybrid-bundle.bdl >output &&
 	grep master output
-
 '
 
 test_done
-- 
1.7.4.1

^ 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