Re: [PATCH 2/2] stash: prefer --quiet over shell redirection

2014-08-31 Thread David Aguilar
On Sat, Aug 30, 2014 at 11:07:13PM +0200, Johannes Sixt wrote:
> Am 30.08.2014 21:30, schrieb David Aguilar:
> > @@ -392,12 +392,12 @@ parse_flags_and_rev()
> > ;;
> > esac
> >  
> > -   REV=$(git rev-parse --quiet --symbolic --verify "$1" 2>/dev/null) || {
> > +   REV=$(git rev-parse --quiet --symbolic --verify "$1") || {
> > reference="$1"
> > die "$(eval_gettext "\$reference is not valid reference")"
> > }
> >  
> > -   i_commit=$(git rev-parse --quiet --verify "$REV^2" 2>/dev/null) &&
> > +   i_commit=$(git rev-parse --quiet --verify "$REV^2") &&
> > set -- $(git rev-parse "$REV" "$REV^1" "$REV:" "$REV^1:" "$REV^2:" 
> > 2>/dev/null) &&
> 
> I see another rev-parse that you did not modify. An omission?

The docs for --quiet say, "Only meaningful in --verify mode", so I didn't touch
the non-verify call-sites.

Thanks for the review. I'll address your notes and send a v2 shortly.
-- 
David
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[PATCH v2] stash: prefer --quiet over shell redirection

2014-08-31 Thread David Aguilar
Use `git rev-parse --verify --quiet` instead of redirecting
stderr to /dev/null.

Signed-off-by: David Aguilar 
---
 git-stash.sh | 13 +++--
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/git-stash.sh b/git-stash.sh
index bcc757b..2ff8b94 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -50,7 +50,7 @@ clear_stash () {
then
die "$(gettext "git stash clear with parameters is 
unimplemented")"
fi
-   if current=$(git rev-parse --verify $ref_stash 2>/dev/null)
+   if current=$(git rev-parse --verify --quiet $ref_stash)
then
git update-ref -d $ref_stash $current
fi
@@ -292,7 +292,7 @@ save_stash () {
 }
 
 have_stash () {
-   git rev-parse --verify $ref_stash >/dev/null 2>&1
+   git rev-parse --verify --quiet $ref_stash >/dev/null
 }
 
 list_stash () {
@@ -392,12 +392,12 @@ parse_flags_and_rev()
;;
esac
 
-   REV=$(git rev-parse --quiet --symbolic --verify "$1" 2>/dev/null) || {
+   REV=$(git rev-parse --symbolic --verify --quiet "$1") || {
reference="$1"
die "$(eval_gettext "\$reference is not valid reference")"
}
 
-   i_commit=$(git rev-parse --quiet --verify "$REV^2" 2>/dev/null) &&
+   i_commit=$(git rev-parse --verify --quiet "$REV^2") &&
set -- $(git rev-parse "$REV" "$REV^1" "$REV:" "$REV^1:" "$REV^2:" 
2>/dev/null) &&
s=$1 &&
w_commit=$1 &&
@@ -409,7 +409,7 @@ parse_flags_and_rev()
test "$ref_stash" = "$(git rev-parse --symbolic-full-name "${REV%@*}")" 
&&
IS_STASH_REF=t
 
-   u_commit=$(git rev-parse --quiet --verify "$REV^3" 2>/dev/null) &&
+   u_commit=$(git rev-parse --verify --quiet "$REV^3") &&
u_tree=$(git rev-parse "$REV^3:" 2>/dev/null)
 }
 
@@ -531,7 +531,8 @@ drop_stash () {
die "$(eval_gettext "\${REV}: Could not drop stash entry")"
 
# clear_stash if we just dropped the last stash entry
-   git rev-parse --verify "$ref_stash@{0}" >/dev/null 2>&1 || clear_stash
+   git rev-parse --verify --quiet "$ref_stash@{0}" >/dev/null ||
+   clear_stash
 }
 
 apply_to_branch () {
-- 
2.1.0.29.g9b751c4

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH v3] teach fast-export an --anonymize option

2014-08-31 Thread Eric Sunshine
On Thu, Aug 28, 2014 at 8:32 AM, Jeff King  wrote:
> On Thu, Aug 28, 2014 at 05:30:44PM +0700, Duy Nguyen wrote:
>
>> On Thu, Aug 28, 2014 at 12:01 AM, Jeff King  wrote:
>> > You can get an overview of what will be shared
>> > by running a command like:
>> >
>> >   git fast-export --anonymize --all |
>> >   perl -pe 's/\d+/X/g' |
>> >   sort -u |
>> >   less
>> >
>> > which will show every unique line we generate, modulo any
>> > numbers (each anonymized token is assigned a number, like
>> > "User 0", and we replace it consistently in the output).
>>
>> I feel like this should be part of git-fast-export.txt, just to
>> increase the user's confidence in the tool (and I don't expect most
>> users to read this commit message).
>
> Hmph. Whenever I say "I think this patch is done", suddenly the comments
> start pouring in. :)

Considering that the value of --anonymize is not yet known, is such an
invasive change to fast-export.c warranted? Would it make sense
instead to provide "anonymize" functionality as a contrib/ script or a
distinct git-anonymize-foo command which accepts a fast-import stream
as input and anonymizes it as output?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 22/32] checkout: support checking out into a new working directory

2014-08-31 Thread Philip Oakley

From: "Duy Nguyen" 
On Sun, Aug 31, 2014 at 11:49 AM, Duy Nguyen  
wrote:

All checkouts share the same repository. Linked checkouts see the
repository a bit different from the main checkout. When you perform
the command


git checkout --to  


The checkout at  will have a unique id that is also 
the
branch name (e.g. ""). A number may be appended to the 
id


.. and I got my facts wrong. The above line should be:

last component of . A number may be appended to the id



That's fine. Glad that we're on the same wavelength for the 
documentation.



to make it unique. All worktree-specific files of this new checkout
are in $GIT_DIR/repos/ of the main checkout. So "HEAD"
inside the linked checkout will be resolved to
"$GIT_DIR/repos//HEAD", while "HEAD" from the main


s///


checkout remains "$GIT_DIR/HEAD".




--
Philip 


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH v8 1/4] cache-tree: Create/update cache-tree on checkou

2014-08-31 Thread John Keeping
On Fri, Jul 11, 2014 at 09:44:33PM -0700, David Turner wrote:
> When git checkout checks out a branch, create or update the
> cache-tree so that subsequent operations are faster.
> 
> update_main_cache_tree learned a new flag, WRITE_TREE_REPAIR.  When
> WRITE_TREE_REPAIR is set, portions of the cache-tree which do not
> correspond to existing tree objects are invalidated (and portions which
> do are marked as valid).  No new tree objects are created.
> 
> Signed-off-by: David Turner 
> ---

This causes an incorrect error message to be printed when switching
branches with staged changes in a subdirectory.  The test case is pretty
simple:

git init test &&
cd test &&
mkdir sub &&
echo one >sub/one &&
git add sub/one &&
git commit -m one &&
echo two >sub/two &&
git add sub/two &&
git checkout -b test

After this commit the output is:

error: invalid object 04  
for 'bar'
A   bar/quux
Switched to branch 'test'

but the "error:" line should not be there.

Everything still works, but I think the error message is a bit scary
considering that this isn't actually an error.

>  builtin/checkout.c|  8 
>  cache-tree.c  | 12 +++-
>  cache-tree.h  |  1 +
>  t/t0090-cache-tree.sh | 19 ---
>  4 files changed, 36 insertions(+), 4 deletions(-)
> 
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index 07cf555..054214f 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -553,6 +553,14 @@ static int merge_working_tree(const struct checkout_opts 
> *opts,
>   }
>   }
>  
> + if (!active_cache_tree)
> + active_cache_tree = cache_tree();
> +
> + if (!cache_tree_fully_valid(active_cache_tree))
> + cache_tree_update(active_cache_tree,
> +   (const struct cache_entry * const 
> *)active_cache,
> +   active_nr, WRITE_TREE_SILENT | 
> WRITE_TREE_REPAIR);
> +
>   if (write_cache(newfd, active_cache, active_nr) ||
>   commit_locked_index(lock_file))
>   die(_("unable to write new index file"));
> diff --git a/cache-tree.c b/cache-tree.c
> index 7fa524a..f951d7d 100644
> --- a/cache-tree.c
> +++ b/cache-tree.c
> @@ -239,9 +239,12 @@ static int update_one(struct cache_tree *it,
>   struct strbuf buffer;
>   int missing_ok = flags & WRITE_TREE_MISSING_OK;
>   int dryrun = flags & WRITE_TREE_DRY_RUN;
> + int repair = flags & WRITE_TREE_REPAIR;
>   int to_invalidate = 0;
>   int i;
>  
> + assert(!(dryrun && repair));
> +
>   *skip_count = 0;
>  
>   if (0 <= it->entry_count && has_sha1_file(it->sha1))
> @@ -374,7 +377,14 @@ static int update_one(struct cache_tree *it,
>  #endif
>   }
>  
> - if (dryrun)
> + if (repair) {
> + unsigned char sha1[20];
> + hash_sha1_file(buffer.buf, buffer.len, tree_type, sha1);
> + if (has_sha1_file(sha1))
> + hashcpy(it->sha1, sha1);
> + else
> + to_invalidate = 1;
> + } else if (dryrun)
>   hash_sha1_file(buffer.buf, buffer.len, tree_type, it->sha1);
>   else if (write_sha1_file(buffer.buf, buffer.len, tree_type, it->sha1)) {
>   strbuf_release(&buffer);
> diff --git a/cache-tree.h b/cache-tree.h
> index f1923ad..666d18f 100644
> --- a/cache-tree.h
> +++ b/cache-tree.h
> @@ -39,6 +39,7 @@ int update_main_cache_tree(int);
>  #define WRITE_TREE_IGNORE_CACHE_TREE 2
>  #define WRITE_TREE_DRY_RUN 4
>  #define WRITE_TREE_SILENT 8
> +#define WRITE_TREE_REPAIR 16
>  
>  /* error return codes */
>  #define WRITE_TREE_UNREADABLE_INDEX (-1)
> diff --git a/t/t0090-cache-tree.sh b/t/t0090-cache-tree.sh
> index 6c33e28..98fb1ab 100755
> --- a/t/t0090-cache-tree.sh
> +++ b/t/t0090-cache-tree.sh
> @@ -44,14 +44,14 @@ test_expect_success 'read-tree HEAD establishes 
> cache-tree' '
>  
>  test_expect_success 'git-add invalidates cache-tree' '
>   test_when_finished "git reset --hard; git read-tree HEAD" &&
> - echo "I changed this file" > foo &&
> + echo "I changed this file" >foo &&
>   git add foo &&
>   test_invalid_cache_tree
>  '
>  
>  test_expect_success 'update-index invalidates cache-tree' '
>   test_when_finished "git reset --hard; git read-tree HEAD" &&
> - echo "I changed this file" > foo &&
> + echo "I changed this file" >foo &&
>   git update-index --add foo &&
>   test_invalid_cache_tree
>  '
> @@ -85,9 +85,22 @@ test_expect_success 'reset --hard without index gives 
> cache-tree' '
>   test_shallow_cache_tree
>  '
>  
> -test_expect_failure 'checkout gives cache-tree' '
> +test_expect_success 'checkout gives cache-tree' '
> + git tag current &&
>   git checkout HEAD^ &&
>   test_shallow_cache_tree
>  '
>  
> +test_expect_success 'checkout -b gives c

Re: [PATCH 3/2] t5309: mark delta-cycle failover tests as passing

2014-08-31 Thread Jeff King
On Sat, Aug 30, 2014 at 09:23:11AM -0400, Jeff King wrote:

> The implications of this make me slightly nervous, though. In the
> --fix-thin case, the resulting pack will have 3 objects:
> 
>   - A as a delta on B
>   - B as a delta on A
>   - a full copy of either A (or B) provided by --fix-thin
> 
> We create a .idx that has duplicate entries for A. If a reader is trying
> to reconstruct B and they find the full copy of A, they're fine. If they
> find the delta copy, what happens?
> 
> Ideally the reader would say "hey, I can't reconstruct A here, let me
> try to find another copy". But I am not sure if that happens, or if we
> are even capable of finding another copy of A (certainly we can find one
> in another pack, but I do not think we are smart enough to find a
> duplicate in the same pack).

The main reason this was "makes me nervous" before is that I did not
fully understand _why_ it worked with the current code. That bugged me,
so I dug further. And the answer is that it does not, but just happens
to work for some small cases.

Try this on top:

diff --git a/t/t5309-pack-delta-cycles.sh b/t/t5309-pack-delta-cycles.sh
index 5309095..4086983 100755
--- a/t/t5309-pack-delta-cycles.sh
+++ b/t/t5309-pack-delta-cycles.sh
@@ -58,9 +58,19 @@ test_expect_success 'index-pack detects REF_DELTA cycles' '
 
 test_expect_success 'failover to an object in another pack' '
clear_packs &&
+   {
+   pack_header 100 &&
+   for i in $(test_seq 50); do
+   pack_obj $A $B &&
+   pack_obj $B $A || break
+   done
+   } >megacycle.pack &&
+   pack_trailer megacycle.pack &&
git index-pack --stdin &2 indexed pack successfully... &&
+   git fsck &&
+   echo >&2 actually re-read pack successfully
 '
 
 test_expect_success 'failover to a duplicate object in the same pack' '


It has the same cycle problem, but we are just adding a larger number of
instances to the pack. Which means that any given sha1-lookup in the
index is more likely to hit a delta rather than the base object.

We successfully index the pack, but our fsck goes into an infinite loop.
Yikes.

I haven't really looked into it, but I suspect we would need some kind
of cycle detection on the delta resolution (and possibly to teach the
sha1-lookup to recognize duplicate objects in the pack and treat them
individually). Frankly, I don't think it is worth the effort or
complexity. We should probably just declare delta cycles insane and
reject them outright.

We used to do that because the only way to correctly resolve them was by
introducing a duplicate base object, and we did not allow that. Patch 2
from my series loosened this, which makes index-pack work, but not
necessarily the rest of git. And since index-pack is the gatekeeper on
receiving objects from remotes, it needs to be the _most_ picky. So my
series is definitely a regression as-is.

We can solve this in one of three ways:

  1. Teach the rest of git to handle recoverable delta cycles. This is
 probably crazy and not worth the effort (and it just lets crap
 through that will hurt other git implementations, too).

  2. Continue to let through duplicate objects in index-pack, but
 specifically detect and reject delta cycles. This is more work (I'm
 not sure yet how easy it would be to detect cycles), but it would
 mean we can treat duplicates (a much less nasty problem) and cycles
 differently.

  3. Go back to outlawing duplicate bases. This is very easy. Just drop
 my patch 2. :)

I am inclined to go with the third option. There has already been a
suggestion from Shawn that we disallow duplicates entirely, and I was
tempted to go that direction even without this finding. But to me this
makes it a no-brainer; the question has gone from "how strict do we want
to be" to "do we want to protect the rest of the code against useless
and potentially harmful violations of their assumptions".

If we do go with (3), that opens up two new questions:

  a. Should we disallow _all_ duplicates, or just those that are bases?
 This is actually easy to code; the assert() in find_unresolved_deltas
 catches the bases, and the .idx writer catches any other ones.

  b. How optional do we want to make this? Right now (without this
 series) the delta-base duplicates always die, and regular
 duplicates are prevented only under --strict.

 If we treat them the same, it should probably be die-by-default.
 Should there be an optional mode to let this stuff through (i.e., a
 "I know this might cause problems with the rest of git, but I am
 desperate to get the data out of this pack" mode?)

 If we treat them differently, there is not much harm in an option
 to loosen regular duplicates, as I think the rest of git handles
 it. For bases, in theory you might be able to recover some data.
 But you may also run into this infinite loop. It is very much 

Re: [PATCH 2/2] index-pack: handle duplicate base objects gracefully

2014-08-31 Thread Jeff King
On Sat, Aug 30, 2014 at 06:00:59PM +0200, René Scharfe wrote:

> My only nit with patch 2: Petr Stodulka  and Martin von
> Gagern  should be mentioned as bug reporters.

Yeah, I agree with that. And actually, you should get a Reported-by:
on the first patch. :)

However, I think there are some grave implications of this series; see
the message I just posted elsewhere in the thread. I think we will end
up dropping patch 2, but keep patch 1.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 2/2] index-pack: handle duplicate base objects gracefully

2014-08-31 Thread Jeff King
On Sat, Aug 30, 2014 at 06:10:33PM -0700, Shawn Pearce wrote:

> > We do detect and complain if --strict is given. Should we make it the
> > default instead? I think it is still worthwhile to have a mode that can
> > handle these packs. It may be the only reasonable way to recover the
> > data from such a broken pack (and that broken pack may be the only copy
> > of the data you have, if you are stuck getting it out of a broken
> > implementation on a remote server).
> 
> Eh, OK, that does make a lot of sense.
> 
> Unfortunately accepting it by default encourages broken
> implementations to stay broken and ship packs with duplicate objects,
> which they shouldn't do since there are readers out there that cannot
> handle it.

I was pretty much convinced by this line of reasoning after you reading
your message. But after discovering some horrible side effects that I
just posted elsewhere, I think it's a no-brainer.

> In my opinion we should make --strict default, but its an excellent
> point made that users should have an escape hatch to disable this
> check, attempt accepting a broken pack and try fixing it locally with
> repack.

I am not sure if you meant it this way, but --strict controls a lot more
than just duplicate objects. It also runs fsck checks against the
incoming objects. Turning all of that on is a much bigger change than I
think we've been talking about.

But it's one I think we should consider. We've had --strict on at GitHub
for a few years, and it has caught many problems. Frequently we get
push-back from users saying "but nowhere else I pushed this to
complains". My opinion is that it is not that we are wrong for being
picky, but that other servers are not being picky enough.

It's also a bit of a hassle, though. E.g., a lot of projects have broken
ident lines deep in their history (from old versions of git, or other
broken implementations) and it is often way too late to fix them. We
usually try to get people to rewrite the history if it's not a problem,
but otherwise will lift the restrictions temporarily to let them push up
old history.

Broken ident lines are annoying, but not _too_ fundamentally bad.
Duplicate tree entries are a lot worse. Fsck even distinguishes between
"error" and "warning", but "index-pack --strict" treats both as a reason
to reject the object. We could perhaps loosen that, and make sure our
error/warning categories are reasonable.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH] reachable.c: add HEAD to reachability starting commits

2014-08-31 Thread Jeff King
On Sat, Aug 30, 2014 at 11:58:35PM +0300, Max Kirillov wrote:

> HEAD is not explicitly used as a starting commit for
> calculating reachability, so if it's detached and reflogs
> are disabled it may be pruned.

Eek, you're right. I think nobody noticed because the HEAD reflog
usually picks it up (and you do not usually detach HEAD on a bare repo).
But I agree we should include it to cover this case.

> diff --git a/reachable.c b/reachable.c
> index 654a8c5..6f6835b 100644
> --- a/reachable.c
> +++ b/reachable.c
> @@ -229,6 +229,9 @@ void mark_reachable_objects(struct rev_info *revs, int 
> mark_reflog,
>   /* Add all external refs */
>   for_each_ref(add_one_ref, revs);
>  
> + /* detached HEAD is not included in the list above */
> + head_ref(add_one_ref, revs);
> +
>   /* Add all reflog info */
>   if (mark_reflog)
>   for_each_reflog(add_one_reflog, revs);

Looks obviously correct.

> diff --git a/t/t5312-prune-detached.sh b/t/t5312-prune-detached.sh
> new file mode 100755
> index 000..fac93e1
> --- /dev/null
> +++ b/t/t5312-prune-detached.sh
> @@ -0,0 +1,19 @@
> +#!/bin/sh
> +
> +test_description='no prune detached head without reflog'
> +. ./test-lib.sh
> +
> +test_expect_success 'make repo' '
> + git config core.logAllRefUpdates false
> + git commit --allow-empty -m commit1 &&
> + git commit --allow-empty -m commit2 &&
> + git checkout  --detach master &&
> + git commit --allow-empty -m commit3
> +'
> +
> +test_expect_success 'prune does not delete anything' '
> + git prune -n >prune_actual &&
> + : >prune_expected &&
> + test_cmp prune_expected prune_actual'
> +
> +test_done

Your test looks reasonable, but is there any reason it cannot go in
t5304 with the other prune tests?

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH] rev-parse: include HEAD in --all output

2014-08-31 Thread Jeff King
On Sun, Aug 31, 2014 at 01:24:48AM +0300, Max Kirillov wrote:

> for_each_ref() does not include it itself, and without the hash
> the detached HEAD may be missed by some frontends (like gitk).
> 
> Add test which verifies the head is returned
> 
> Update test t6018-rev-list-glob.sh which relied on exact list of
> returned hashes.

I think the missing bit of the justification here is that "--all" _does_
include HEAD in other contexts (like in git-log), and rev-parse should
probably match it.

This is probably the right thing to do. It's possible that some caller
of rev-parse really depends on "--all" meaning "just the refs", but I
kind of doubt it. Being in sync with the revision.c parser seems saner.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH v3] teach fast-export an --anonymize option

2014-08-31 Thread Jeff King
On Sun, Aug 31, 2014 at 06:34:08AM -0400, Eric Sunshine wrote:

> >> I feel like this should be part of git-fast-export.txt, just to
> >> increase the user's confidence in the tool (and I don't expect most
> >> users to read this commit message).
> >
> > Hmph. Whenever I say "I think this patch is done", suddenly the comments
> > start pouring in. :)
> 
> Considering that the value of --anonymize is not yet known, is such an
> invasive change to fast-export.c warranted? Would it make sense
> instead to provide "anonymize" functionality as a contrib/ script or a
> distinct git-anonymize-foo command which accepts a fast-import stream
> as input and anonymizes it as output?

I considered that, but there's a non-trivial amount of work in the
parsing of the stream (I had originally thought to just ship a perl
script to operate on the stream). And while there's a fair bit of code
added to fast-export.c, none of it is ever called unless --anonymize is
set.

So while I am not 100% sure that the idea is a good one, I do not think
it is hurting the current fast-export in any meaningful way. Two things
we could do to minimize that are:

  1. Move the anonymization code into a separate C file to keep the
 fast-export source a little more pristine. I avoided doing this
 just because the interfaces to the functions are fairly tailored to
 what fast-export wants.

  2. Have a separate git-anonymize command which is basically running
 "git fast-export --anonymize" under the hood. This avoids polluting
 fast-export from the user's perspective (they do not need to care
 that it is running fast-export under the hood).

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 2/2] index-pack: handle duplicate base objects gracefully

2014-08-31 Thread René Scharfe

Am 31.08.2014 um 17:17 schrieb Jeff King:

On Sat, Aug 30, 2014 at 06:00:59PM +0200, René Scharfe wrote:


My only nit with patch 2: Petr Stodulka  and Martin von
Gagern  should be mentioned as bug reporters.


Yeah, I agree with that. And actually, you should get a Reported-by:
on the first patch. :)

However, I think there are some grave implications of this series; see
the message I just posted elsewhere in the thread. I think we will end
up dropping patch 2, but keep patch 1.


OK, but it would still be a good idea to replace the assert()s with 
die()s and error messages that tell users that the repo is broken, not git.


René
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: git fsck exit code?

2014-08-31 Thread Øyvind A . Holm
On 29 August 2014 22:18, David Turner  wrote:
> On Fri, 2014-08-29 at 12:21 -0700, Junio C Hamano wrote:
> > Jeff King  writes:
> > > On Wed, Aug 27, 2014 at 06:10:12PM -0400, David Turner wrote:
> > > > It looks like git fsck exits with 0 status even if there are
> > > > some errors. The only case where there's a non-zero exit code is
> > > > if verify_pack reports errors -- but not e.g. fsck_object_dir.
> > >
> > > It will also bail non-zero with _certain_ tree errors that cause
> > > git to die() rather than fscking more completely.
> >
> > Even if git does not die, whenever it says broken link, missing
> > object, or object corrupt, we set errors_found and that variable
> > affects the exit status of fsck.  What does "some errors" exactly
> > mean in the original report?  Dangling objects are *not* errors and
> > should not cause fsck to report an error with its exit status.
>
> error in tree 9f50addba2b4e9e928d9c6a7056bdf71b36fba90: contains
> duplicate file entries

I don't think git fsck should return !0 in this case. Yes, it's an
inconsistency in the repo, but it's sometimes due to erroneous
conversions from another SCM or some other (non-standard) implementation
of the git client. I've seen things like this (and other inconsistencies
in repos, like wrong date formats, non-standard Author fields, unsorted
trees, zero-padded file modes and so on), and the thing is, owners of
public repos with these errors tend to avoid fixing it because it
changes the commit SHAs. If git fsck starts to return !0 on these
errors, it will always return error on that repo, which in practise
means that the error code is rendered useless. IMHO git fsck should only
return !0 on errors that can be fixed without changing the commit
history, for example missing or invalid objects.

Øyvind
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[RFC PATCH 2/3] headers: improve header dependencies

2014-08-31 Thread David Aguilar
Add missing includes or forward declarations where needed.

Signed-off-by: David Aguilar 
---
If enum date_type were moved to object.h then we wouldn't need
to include cache.h from commit.h, but this patch doesn't touch that.

If we want to avoid including cache.h, another possibility
is moving enum object_type to something like object-types.h so that
e.g. dir.h wouldn't need to include all of cache.h.

Do we prefer these forward declarations or full-on #includes
of the corresponding header?  Forward declarations seemed like
the least intrusive, which is why some of the files do that in
this patch.

 color.h | 2 ++
 commit.h| 2 +-
 diffcore.h  | 2 ++
 dir.h   | 1 +
 graph.h | 3 +++
 object.h| 2 ++
 parse-options.h | 2 ++
 pathspec.h  | 2 ++
 quote.h | 2 ++
 refs.h  | 5 +
 remote.h| 2 ++
 sequencer.h | 4 
 strbuf.h| 2 ++
 submodule.h | 3 +++
 tag.h   | 1 +
 tree-walk.h | 2 ++
 tree.h  | 2 ++
 utf8.h  | 4 
 18 files changed, 42 insertions(+), 1 deletion(-)

diff --git a/color.h b/color.h
index 9a8495b..6d64e6d 100644
--- a/color.h
+++ b/color.h
@@ -1,6 +1,8 @@
 #ifndef COLOR_H
 #define COLOR_H
 
+#include "git-compat-util.h"
+
 struct strbuf;
 
 /*  2 + (2 * num_attrs) + 8 + 1 + 8 + 'm' + NUL */
diff --git a/commit.h b/commit.h
index 268c9d7..339c55d 100644
--- a/commit.h
+++ b/commit.h
@@ -1,7 +1,7 @@
 #ifndef COMMIT_H
 #define COMMIT_H
 
-#include "object.h"
+#include "cache.h"
 #include "tree.h"
 #include "strbuf.h"
 #include "decorate.h"
diff --git a/diffcore.h b/diffcore.h
index c876dac..43856de 100644
--- a/diffcore.h
+++ b/diffcore.h
@@ -4,6 +4,8 @@
 #ifndef DIFFCORE_H
 #define DIFFCORE_H
 
+struct diff_options;
+
 /* This header file is internal between diff.c and its diff transformers
  * (e.g. diffcore-rename, diffcore-pickaxe).  Never include this header
  * in anything else.
diff --git a/dir.h b/dir.h
index 6c45e9d..1cc9f90 100644
--- a/dir.h
+++ b/dir.h
@@ -3,6 +3,7 @@
 
 /* See Documentation/technical/api-directory-listing.txt */
 
+#include "cache.h"
 #include "strbuf.h"
 
 struct dir_entry {
diff --git a/graph.h b/graph.h
index 0be62bd..585ebe6 100644
--- a/graph.h
+++ b/graph.h
@@ -3,6 +3,9 @@
 
 /* A graph is a pointer to this opaque structure */
 struct git_graph;
+struct commit;
+struct rev_info;
+struct strbuf;
 
 /*
  * Set up a custom scheme for column colors.
diff --git a/object.h b/object.h
index 5e8d8ee..40bd3a8 100644
--- a/object.h
+++ b/object.h
@@ -1,6 +1,8 @@
 #ifndef OBJECT_H
 #define OBJECT_H
 
+enum object_type;
+
 struct object_list {
struct object *item;
struct object_list *next;
diff --git a/parse-options.h b/parse-options.h
index 7940bc7..933a1b7 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -1,6 +1,8 @@
 #ifndef PARSE_OPTIONS_H
 #define PARSE_OPTIONS_H
 
+#include "git-compat-util.h"
+
 enum parse_opt_type {
/* special types */
OPTION_END,
diff --git a/pathspec.h b/pathspec.h
index 0c11262..c92fafc 100644
--- a/pathspec.h
+++ b/pathspec.h
@@ -1,6 +1,8 @@
 #ifndef PATHSPEC_H
 #define PATHSPEC_H
 
+#include "git-compat-util.h"
+
 /* Pathspec magic */
 #define PATHSPEC_FROMTOP   (1<<0)
 #define PATHSPEC_MAXDEPTH  (1<<1)
diff --git a/quote.h b/quote.h
index 71dcc3a..f9ca9e2 100644
--- a/quote.h
+++ b/quote.h
@@ -1,6 +1,8 @@
 #ifndef QUOTE_H
 #define QUOTE_H
 
+#include "git-compat-util.h"
+
 struct strbuf;
 
 /* Help to copy the thing properly quoted for the shell safety.
diff --git a/refs.h b/refs.h
index 00f209a..889bf8d 100644
--- a/refs.h
+++ b/refs.h
@@ -1,6 +1,11 @@
 #ifndef REFS_H
 #define REFS_H
 
+#include "git-compat-util.h"
+
+struct strbuf;
+struct string_list;
+
 struct ref_lock {
char *ref_name;
char *orig_ref_name;
diff --git a/remote.h b/remote.h
index 917d383..4b1533f 100644
--- a/remote.h
+++ b/remote.h
@@ -3,6 +3,8 @@
 
 #include "parse-options.h"
 
+struct strbuf;
+
 enum {
REMOTE_CONFIG,
REMOTE_REMOTES,
diff --git a/sequencer.h b/sequencer.h
index db43e9c..8e30082 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -1,6 +1,10 @@
 #ifndef SEQUENCER_H
 #define SEQUENCER_H
 
+#include "git-compat-util.h"
+
+struct strbuf;
+
 #define SEQ_DIR"sequencer"
 #define SEQ_HEAD_FILE  "sequencer/head"
 #define SEQ_TODO_FILE  "sequencer/todo"
diff --git a/strbuf.h b/strbuf.h
index a7c0192..be59d9e 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -1,6 +1,8 @@
 #ifndef STRBUF_H
 #define STRBUF_H
 
+#include "git-compat-util.h"
+
 /* See Documentation/technical/api-strbuf.txt */
 
 extern char strbuf_slopbuf[];
diff --git a/submodule.h b/submodule.h
index 7beec48..5295c01 100644
--- a/submodule.h
+++ b/submodule.h
@@ -1,8 +1,11 @@
 #ifndef SUBMODULE_H
 #define SUBMODULE_H
 
+#include "git-compat-util.h"
+
 struct diff_options;
 struct argv_array;
+struct string_list;
 
 enum {
RECURSE_SUBMODULES_ON_DEMAND = -1,
diff --git a/tag.h b/tag.h
index bc8a1e4..68b0

[RFC PATCH 1/3] stylefix: pointers bind to the variable, not the type

2014-08-31 Thread David Aguilar
Signed-off-by: David Aguilar 
---
This is an RFC patch but this is probably fine as-is,
and is orthogonal to the next two patches.

 builtin/clone.c | 7 ---
 commit.c| 2 +-
 commit.h| 2 +-
 reflog-walk.c   | 2 +-
 reflog-walk.h   | 2 +-
 refs.h  | 2 +-
 remote-curl.c   | 2 +-
 7 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index bbd169c..315969d 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -685,9 +685,10 @@ static void write_config(struct string_list *config)
}
 }
 
-static void write_refspec_config(const char* src_ref_prefix,
-   const struct ref* our_head_points_at,
-   const struct ref* remote_head_points_at, struct strbuf* 
branch_top)
+static void write_refspec_config(const char *src_ref_prefix,
+   const struct ref *our_head_points_at,
+   const struct ref *remote_head_points_at,
+   struct strbuf *branch_top)
 {
struct strbuf key = STRBUF_INIT;
struct strbuf value = STRBUF_INIT;
diff --git a/commit.c b/commit.c
index ae7f2b1..4de6be4 100644
--- a/commit.c
+++ b/commit.c
@@ -1256,7 +1256,7 @@ static void parse_gpg_output(struct signature_check *sigc)
}
 }
 
-void check_commit_signature(const struct commit* commit, struct 
signature_check *sigc)
+void check_commit_signature(const struct commit *commit, struct 
signature_check *sigc)
 {
struct strbuf payload = STRBUF_INIT;
struct strbuf signature = STRBUF_INIT;
diff --git a/commit.h b/commit.h
index a8cbf52..268c9d7 100644
--- a/commit.h
+++ b/commit.h
@@ -346,7 +346,7 @@ extern void print_commit_list(struct commit_list *list,
  * at all.  This may allocate memory for sig->gpg_output, sig->gpg_status,
  * sig->signer and sig->key.
  */
-extern void check_commit_signature(const struct commit* commit, struct 
signature_check *sigc);
+extern void check_commit_signature(const struct commit *commit, struct 
signature_check *sigc);
 
 int compare_commits_by_commit_date(const void *a_, const void *b_, void 
*unused);
 
diff --git a/reflog-walk.c b/reflog-walk.c
index 9ce8b53..0e5174b 100644
--- a/reflog-walk.c
+++ b/reflog-walk.c
@@ -133,7 +133,7 @@ struct reflog_walk_info {
struct commit_reflog *last_commit_reflog;
 };
 
-void init_reflog_walk(struct reflog_walk_info** info)
+void init_reflog_walk(struct reflog_walk_info **info)
 {
*info = xcalloc(1, sizeof(struct reflog_walk_info));
 }
diff --git a/reflog-walk.h b/reflog-walk.h
index 50265f5..a9bd60e 100644
--- a/reflog-walk.h
+++ b/reflog-walk.h
@@ -5,7 +5,7 @@
 
 struct reflog_walk_info;
 
-extern void init_reflog_walk(struct reflog_walk_info** info);
+extern void init_reflog_walk(struct reflog_walk_info **info);
 extern int add_reflog_for_walk(struct reflog_walk_info *info,
struct commit *commit, const char *name);
 extern void fake_reflog_parent(struct reflog_walk_info *info,
diff --git a/refs.h b/refs.h
index ec46acd..00f209a 100644
--- a/refs.h
+++ b/refs.h
@@ -77,7 +77,7 @@ static inline const char *has_glob_specials(const char 
*pattern)
 extern int for_each_rawref(each_ref_fn, void *);
 
 extern void warn_dangling_symref(FILE *fp, const char *msg_fmt, const char 
*refname);
-extern void warn_dangling_symrefs(FILE *fp, const char *msg_fmt, const struct 
string_list* refnames);
+extern void warn_dangling_symrefs(FILE *fp, const char *msg_fmt, const struct 
string_list *refnames);
 
 /*
  * Lock the packed-refs file for writing.  Flags is passed to
diff --git a/remote-curl.c b/remote-curl.c
index 0fcf2ce..d2229e0 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -221,7 +221,7 @@ static int show_http_message(struct strbuf *type, struct 
strbuf *charset,
return 0;
 }
 
-static struct discovery* discover_refs(const char *service, int for_push)
+static struct discovery *discover_refs(const char *service, int for_push)
 {
struct strbuf exp = STRBUF_INIT;
struct strbuf type = STRBUF_INIT;
-- 
2.1.0.30.g0bdc89a

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[RFC PATCH 3/3] core: improve header dependencies

2014-08-31 Thread David Aguilar
Remove includes that have already been included by another header.

Signed-off-by: David Aguilar 
---
This patch is the culmination of the previous patches and
is a pure removal.

 builtin/add.c   | 3 ---
 builtin/blame.c | 4 
 builtin/branch.c| 4 
 builtin/checkout.c  | 4 
 builtin/diff-files.c| 3 ---
 builtin/diff-index.c| 3 ---
 builtin/diff.c  | 3 ---
 builtin/fast-export.c   | 7 ---
 builtin/fmt-merge-msg.c | 5 -
 builtin/log.c   | 6 --
 builtin/merge-base.c| 4 
 builtin/merge.c | 5 -
 builtin/pack-objects.c  | 4 
 builtin/prune.c | 4 
 builtin/reflog.c| 3 ---
 builtin/rev-list.c  | 3 ---
 builtin/rev-parse.c | 4 
 builtin/revert.c| 3 ---
 builtin/shortlog.c  | 5 -
 builtin/tag.c   | 4 
 bundle.c| 4 
 combine-diff.c  | 3 ---
 commit.c| 5 -
 diff-lib.c  | 3 ---
 diff-no-index.c | 4 
 graph.c | 3 ---
 http-push.c | 3 ---
 line-log.c  | 7 ---
 list-objects.c  | 4 
 pack-bitmap-write.c | 3 ---
 pack-bitmap.c   | 3 ---
 pretty.c| 5 -
 reachable.c | 3 ---
 reflog-walk.c   | 4 
 remote.c| 4 
 revision.c  | 6 --
 sequencer.c | 4 
 shallow.c   | 3 ---
 submodule.c | 4 
 test-revision-walking.c | 3 ---
 transport-helper.c  | 4 
 upload-pack.c   | 5 -
 wt-status.c | 5 -
 43 files changed, 173 deletions(-)

diff --git a/builtin/add.c b/builtin/add.c
index 4baf3a5..3e898d2 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -3,15 +3,12 @@
  *
  * Copyright (C) 2006 Linus Torvalds
  */
-#include "cache.h"
 #include "builtin.h"
 #include "dir.h"
 #include "pathspec.h"
 #include "exec_cmd.h"
 #include "cache-tree.h"
 #include "run-command.h"
-#include "parse-options.h"
-#include "diff.h"
 #include "diffcore.h"
 #include "revision.h"
 #include "bulk-checkin.h"
diff --git a/builtin/blame.c b/builtin/blame.c
index 17d30d0..84e44ff 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -8,19 +8,15 @@
 #include "cache.h"
 #include "builtin.h"
 #include "blob.h"
-#include "commit.h"
 #include "tag.h"
 #include "tree-walk.h"
-#include "diff.h"
 #include "diffcore.h"
 #include "revision.h"
 #include "quote.h"
 #include "xdiff-interface.h"
 #include "cache-tree.h"
-#include "string-list.h"
 #include "mailmap.h"
 #include "mergesort.h"
-#include "parse-options.h"
 #include "prio-queue.h"
 #include "utf8.h"
 #include "userdiff.h"
diff --git a/builtin/branch.c b/builtin/branch.c
index 0591b22..f4adb93 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -8,14 +8,10 @@
 #include "cache.h"
 #include "color.h"
 #include "refs.h"
-#include "commit.h"
 #include "builtin.h"
 #include "remote.h"
-#include "parse-options.h"
 #include "branch.h"
-#include "diff.h"
 #include "revision.h"
-#include "string-list.h"
 #include "column.h"
 #include "utf8.h"
 #include "wt-status.h"
diff --git a/builtin/checkout.c b/builtin/checkout.c
index f71e745..bc53824 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -1,9 +1,6 @@
 #include "cache.h"
 #include "builtin.h"
-#include "parse-options.h"
 #include "refs.h"
-#include "commit.h"
-#include "tree.h"
 #include "tree-walk.h"
 #include "cache-tree.h"
 #include "unpack-trees.h"
@@ -11,7 +8,6 @@
 #include "run-command.h"
 #include "merge-recursive.h"
 #include "branch.h"
-#include "diff.h"
 #include "revision.h"
 #include "remote.h"
 #include "blob.h"
diff --git a/builtin/diff-files.c b/builtin/diff-files.c
index 9200069..d538a38 100644
--- a/builtin/diff-files.c
+++ b/builtin/diff-files.c
@@ -3,9 +3,6 @@
  *
  * Copyright (C) Linus Torvalds, 2005
  */
-#include "cache.h"
-#include "diff.h"
-#include "commit.h"
 #include "revision.h"
 #include "builtin.h"
 #include "submodule.h"
diff --git a/builtin/diff-index.c b/builtin/diff-index.c
index ce15b23..7e998b0 100644
--- a/builtin/diff-index.c
+++ b/builtin/diff-index.c
@@ -1,6 +1,3 @@
-#include "cache.h"
-#include "diff.h"
-#include "commit.h"
 #include "revision.h"
 #include "builtin.h"
 #include "submodule.h"
diff --git a/builtin/diff.c b/builtin/diff.c
index 0f247d2..aad7b98 100644
--- a/builtin/diff.c
+++ b/builtin/diff.c
@@ -3,12 +3,9 @@
  *
  * Copyright (c) 2006 Junio C Hamano
  */
-#include "cache.h"
 #include "color.h"
-#include "commit.h"
 #include "blob.h"
 #include "tag.h"
-#include "diff.h"
 #include "diffcore.h"
 #include "revision.h"
 #include "log-tree.h"
diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index 92b4624..71c4309 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -4,18 +4,11 @@
  * Copyright (C) 2007 Johannes E. Schindelin
  */
 #include "builtin.h"
-#include "cache.h"
-#include "commit.h"
-#include "object.h"
 #include "tag.h"
-#include "dif

[PATCH v2] reachable.c: add HEAD to reachability starting commits

2014-08-31 Thread Max Kirillov
HEAD is not explicitly used as a starting commit for
calculating reachability, so if it's detached and reflogs
are disabled it may be pruned.

Add tests which demonstrate it. Test 'prune: prune former HEAD after checking
out branch' also reverts changes to repository.

Signed-off-by: Max Kirillov 
---
Inserted test into existing script.
 reachable.c  |  3 +++
 t/t5304-prune.sh | 21 +
 2 files changed, 24 insertions(+)

diff --git a/reachable.c b/reachable.c
index 654a8c5..6f6835b 100644
--- a/reachable.c
+++ b/reachable.c
@@ -229,6 +229,9 @@ void mark_reachable_objects(struct rev_info *revs, int 
mark_reflog,
/* Add all external refs */
for_each_ref(add_one_ref, revs);
 
+   /* detached HEAD is not included in the list above */
+   head_ref(add_one_ref, revs);
+
/* Add all reflog info */
if (mark_reflog)
for_each_reflog(add_one_reflog, revs);
diff --git a/t/t5304-prune.sh b/t/t5304-prune.sh
index 377d3d3..77cf064 100755
--- a/t/t5304-prune.sh
+++ b/t/t5304-prune.sh
@@ -104,6 +104,27 @@ test_expect_success 'prune: prune unreachable heads' '
 
 '
 
+test_expect_success 'prune: do not prune detached HEAD with no reflog' '
+
+   git config core.logAllRefUpdates false &&
+   test ! -e .git/logs &&
+   git checkout --detach --quiet &&
+   git commit --allow-empty -m "detached commit" &&
+   git prune -n >prune_actual &&
+   : >prune_expected &&
+   test_cmp prune_actual prune_expected
+
+'
+
+test_expect_success 'prune: prune former HEAD after checking out branch' '
+
+   head_sha1=`git rev-parse HEAD` &&
+   git checkout --quiet master &&
+   git prune -v >prune_actual &&
+   grep -q "$head_sha1" prune_actual
+
+'
+
 test_expect_success 'prune: do not prune heads listed as an argument' '
 
: > file2 &&
-- 
2.0.1.1697.g73c6810

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH] rev-parse: include HEAD in --all output

2014-08-31 Thread Max Kirillov
On Sun, Aug 31, 2014 at 11:30:54AM -0400, Jeff King wrote:
> On Sun, Aug 31, 2014 at 01:24:48AM +0300, Max Kirillov wrote:
> 
>> for_each_ref() does not include it itself, and without
>> the hash the detached HEAD may be missed by some
>> frontends (like gitk).
>> 
>> Add test which verifies the head is returned
>> 
>> Update test t6018-rev-list-glob.sh which relied on exact
>> list of returned hashes.
> 
> I think the missing bit of the justification here is that
> "--all" _does_ include HEAD in other contexts (like in
> git-log), and rev-parse should probably match it.
> 
> This is probably the right thing to do. It's possible that
> some caller of rev-parse really depends on "--all" meaning
> "just the refs", but I kind of doubt it. Being in sync
> with the revision.c parser seems saner.

Actually, yes, this is a bit incompatible change, and while
I'm pretty sure that rev-parse returning hashes should
include detached HEAD, returning HEAD when it's called with
something like "--symbolic" might be questioned. It could
depend on the output mode (add HEAD only if printing hashes)
but this kind of logic does not look good. So maybe some
more opinions should be asked for.

btw, manpage for git-rev-parse says "Show all refs found in
refs/.", should it also be changed?

-- 
Max
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 2/2] index-pack: handle duplicate base objects gracefully

2014-08-31 Thread Junio C Hamano
Jeff King  writes:

> Broken ident lines are annoying, but not _too_ fundamentally bad.
> Duplicate tree entries are a lot worse. Fsck even distinguishes between
> "error" and "warning", but "index-pack --strict" treats both as a reason
> to reject the object. We could perhaps loosen that, and make sure our
> error/warning categories are reasonable.

A pack stream that records the same object twice is not a breakage
that is buried deep in the history and cannot be corrected without a
wholesale history rewrite, unlike commit objects with broken ident
lines or tree objects with 0-filled file type designators.

As such, I think it is a reasonable thing to do the following:

 - "git repack" should be taught to dedup, as a way to recover from
   such a breakage, if not done already.

 - "git fsck" should complain, suggesting users to repack to
   recover.  It may even want to exit with non-zero status.

 - "git receive-pack" and "git fetch-pack" should warn, loudly,
   without failing.  They should ideally not keep such a corrupt
   pack stream on disk at all, de-duping the objects while
   streaming, but if that is not practical, at least they should
   give an easy way for the user to cause de-duping immediately (I
   do not mind if that is "we detected that the other side fed us a
   pack stream that is broken.  We automatically correct the
   breakage for you by repacking.  Be patient while we do so").

 - If the other side of "receive-pack/fetch-pack" implements the
   agent capability, upon detecting such a breakage, it may not be a
   bad idea to offer the user to send an e-mail reporting the
   version of the software to a wall-of-shame e-mail address ;-).

I agree that a tree that records the same path twice should be
outright rejected.  There is no sane recovery path.

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 4/6] fsck: check tag objects' headers

2014-08-31 Thread Junio C Hamano
Jeff King  writes:

> Hmm. But that is because "git tag" always makes one type of tag: one in
> which the "tag" field is the same as the refname in which we store it.
> So the name must be a valid refname there to meet the ref storage
> requirement, and therefore the tag name must, too.
>
> But is that something we necessarily need or want to enforce? Is it OK
> for me to have refs/tags/foo pointing to a tag object that is not
> related to "foo" (either semantically or syntactically)?
>
> I dunno. I cannot think of a reason you would want to do such a thing,
> but this seems like outlawing it because git does not generate it, not
> because it is necessarily a problematic thing to be doing.

Thanks for straightening me out.

If "git fsck" were a tool to validate that the objects and refs are
in line with how "git-core" plumbing and Porcelain toolset uses the
underlying Git data model, it makes sense to insist a tag has a name
that is suitable for a refname, and the tag is pointed by a ref in
"refs/tags/" followed by its name.  The rules such a "git fsck" should
implement would be stricter than what the underlying Git data model
could represent and existing Git tools could handle (i.e. a commit
with broken ident line may not be usable with "shortlog -e" and would
be flagged as corrupt).

But tightening rules in that direction may risk hindering future
progress in an unnecessary way.  We may want to be a bit lenient
when we see something _unusual_ but not necessarily _wrong_, and the
line between them would be blurry in places, as Git is an evolving
software.  It is good to warn about an unsual ones, but we probably
would not want to error on them.

This tightening may be too strict without a very good reason.  For
example, a tentative signed tag (e.g. "for-linus") often used in a
pull request to have it recorded in the resulting merge by the
integrator does not inherently need to be named at all; the ref is
only necessary as a means to transfer the signature from the
contributor to the integrator, and once merged, there is no need for
the tag to have any name.  When we try to improve the workflow to
integrate authenticated work done on the side branch, we may come up
with a way to do so _without_ having to actually have a tag name
(i.e. the "tag" contributor creates for such a purpose may not be
done by "git tag -s" when asking the result to be pulled but do
something different, and it may be perfectly fine for such a
tentative tag to lack the "tag " name line), but still allows us to
record the same merge-tag in the resulting merge commit.

So...
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[PATCH] wt-status.c: Expand cut_line so the scissor line is 72 chars

2014-08-31 Thread Øyvind A . Holm
Change the size of the commit scissor line to 72 characters (the
recommended maximum line length in log messages) so it can be used as a
visual clue in editors using monospaced fonts.

Signed-off-by: Øyvind A. Holm 
---
 Documentation/git-commit.txt | 2 +-
 t/t7502-commit.sh| 2 +-
 wt-status.c  | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
index 0bbc8f5..d24573f 100644
--- a/Documentation/git-commit.txt
+++ b/Documentation/git-commit.txt
@@ -189,7 +189,7 @@ verbatim::
 scissors::
Same as `whitespace`, except that everything from (and
including) the line
-   "`#  >8 `"
+   "`# - >8 
-`"
is truncated if the message is to be edited. "`#`" can be
customized with core.commentChar.
 default::
diff --git a/t/t7502-commit.sh b/t/t7502-commit.sh
index 051489e..8b2ec6b 100755
--- a/t/t7502-commit.sh
+++ b/t/t7502-commit.sh
@@ -229,7 +229,7 @@ test_expect_success 'cleanup commit messages (scissors 
option,-F,-e)' '
cat >text <8 
+# - >8 -
 to be removed
 EOF
echo "# to be kept" >expect &&
diff --git a/wt-status.c b/wt-status.c
index 27da529..010632c 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -18,7 +18,7 @@
 #include "utf8.h"
 
 static const char cut_line[] =
-" >8 \n";
+"- >8 -\n";
 
 static char default_wt_status_colors[][COLOR_MAXLEN] = {
GIT_COLOR_NORMAL, /* WT_STATUS_HEADER */
-- 
2.1.0.29.g10a3b53

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


color box, display box, corrugated box, color card, blister card, color sleeve, hang tag, label

2014-08-31 Thread Jinghao Printing - CHINA
Hi, this is David Wu from Shanghai, China.
We are a printing company, we can print color box, corrugated box,
label, hang tag etc.
Please let me know if you need these.

I will send you the website then.

Best regards,
David Wu
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html