Git development
 help / color / mirror / Atom feed
* Re: Corruption in the repository
From: Christian Couder @ 2010-12-13 16:34 UTC (permalink / raw)
  To: Sylvestre Ledru; +Cc: git, Simon Gareste, Bruno JOFRET
In-Reply-To: <1292255990.25237.27642.camel@korcula.inria.fr>

Hi,

On Mon, Dec 13, 2010 at 4:59 PM, Sylvestre Ledru
<sylvestre.ledru@scilab.org> wrote:
> Hello guys,
>
> I have a small problem with a git repository and I haven't find a way to
> fix my problem.
> I am using git with gerrit [1] as frontend (even if I don't think it is
> related here).
> For an unknown reason, the repository just became corrupted.
>
> When I try to clone the repository straight with the file system, the
> following error is displayed:
> error: refs/changes/98/398/1 does not point to a valid object!
> error: refs/changes/98/398/2 does not point to a valid object!
> fatal: object cff52c24fba28408e7d021a8f35a717bef31521d is corrupted
> fatal: The remote end hung up unexpectedly
>
> git-prune & git-fsck both fail.
>
> Does anyone know how to repair this error ?

Did you try what the FAQ suggests:

https://git.wiki.kernel.org/index.php/GitFaq#How_to_fix_a_broken_repository.3F

?

Regards,
Christian.

^ permalink raw reply

* Re: Corruption in the repository
From: Sylvestre Ledru @ 2010-12-13 16:47 UTC (permalink / raw)
  To: git, Simon Gareste, Bruno JOFRET
In-Reply-To: <AANLkTimD6+CHofhbKvBPjHpcNUNusHOCHSQe+-J1ZA4F@mail.gmail.com>


Le lundi 13 décembre 2010 à 17:34 +0100, Christian Couder a écrit :
> Hi,
> 
> On Mon, Dec 13, 2010 at 4:59 PM, Sylvestre Ledru
> <sylvestre.ledru@scilab.org> wrote:
> > Hello guys,
> >
> > I have a small problem with a git repository and I haven't find a way to
> > fix my problem.
> > I am using git with gerrit [1] as frontend (even if I don't think it is
> > related here).
> > For an unknown reason, the repository just became corrupted.
> >
> > When I try to clone the repository straight with the file system, the
> > following error is displayed:
> > error: refs/changes/98/398/1 does not point to a valid object!
> > error: refs/changes/98/398/2 does not point to a valid object!
> > fatal: object cff52c24fba28408e7d021a8f35a717bef31521d is corrupted
> > fatal: The remote end hung up unexpectedly
> >
> > git-prune & git-fsck both fail.
> >
> > Does anyone know how to repair this error ?
> 
> Did you try what the FAQ suggests:
> 
> https://git.wiki.kernel.org/index.php/GitFaq#How_to_fix_a_broken_repository.3F
Yes. It shows an other error and it doesn't match the error described on
this URL [1].

I get the following:
error: refs/tags/5.3.0-beta-4 does not point to a valid object!
fatal: object 555a7c359b2e589ec10822d9b56cdfeee0105fe0 is corrupted

Sylvestre

[1]
$ git fsck --full
> broken link from    tree 2d9263c6d23595e7cb2a21e5ebbb53655278dff8
>              to    blob 4b9458b3786228369c63936db65827de3cc06200
> missing blob 4b9458b3786228369c63936db65827de3cc06200

^ permalink raw reply

* Re: Corruption in the repository
From: Christian Couder @ 2010-12-13 17:02 UTC (permalink / raw)
  To: Sylvestre Ledru; +Cc: git, Simon Gareste, Bruno JOFRET
In-Reply-To: <1292258845.25237.27680.camel@korcula.inria.fr>

On Mon, Dec 13, 2010 at 5:47 PM, Sylvestre Ledru
<sylvestre.ledru@scilab.org> wrote:
>
> Le lundi 13 décembre 2010 à 17:34 +0100, Christian Couder a écrit :
>> Hi,
>>
>> On Mon, Dec 13, 2010 at 4:59 PM, Sylvestre Ledru
>> <sylvestre.ledru@scilab.org> wrote:
>> > Hello guys,
>> >
>> > I have a small problem with a git repository and I haven't find a way to
>> > fix my problem.
>> > I am using git with gerrit [1] as frontend (even if I don't think it is
>> > related here).
>> > For an unknown reason, the repository just became corrupted.
>> >
>> > When I try to clone the repository straight with the file system, the
>> > following error is displayed:
>> > error: refs/changes/98/398/1 does not point to a valid object!
>> > error: refs/changes/98/398/2 does not point to a valid object!
>> > fatal: object cff52c24fba28408e7d021a8f35a717bef31521d is corrupted
>> > fatal: The remote end hung up unexpectedly
>> >
>> > git-prune & git-fsck both fail.
>> >
>> > Does anyone know how to repair this error ?
>>
>> Did you try what the FAQ suggests:
>>
>> https://git.wiki.kernel.org/index.php/GitFaq#How_to_fix_a_broken_repository.3F
> Yes. It shows an other error and it doesn't match the error described on
> this URL [1].
>
> I get the following:
> error: refs/tags/5.3.0-beta-4 does not point to a valid object!
> fatal: object 555a7c359b2e589ec10822d9b56cdfeee0105fe0 is corrupted

The FAQ says that you should try to replace any broken and/or missing
objects, so you should try to do that with object
555a7c359b2e589ec10822d9b56cdfeee0105fe0 (as described in the FAQ)
even if the error message is not exactly the same.

Regards,
Christian.

^ permalink raw reply

* [PATCH/RFC] ident: die on bogus date format
From: Jeff King @ 2010-12-13 17:02 UTC (permalink / raw)
  To: Sergio; +Cc: git
In-Reply-To: <loom.20101213T161633-993@post.gmane.org>

On Mon, Dec 13, 2010 at 03:20:07PM +0000, Sergio wrote:

> on 1.7.3.3, I have noticed that git --commit silently ignores the 
> --date=<date> switch if <date> is not in the current format.
> 
> for instance
> 
> git --commit --amend --date="10.11.2010" creates a commit with the current
> date and time, because the --date argument misses the time.
> 
> possibly, it would be better to stop with an error message.

Yeah, we should definitely be flagging the error. This patch fixes it,
but I'm not sure if it is optimal (see below).

-- >8 --
Subject: [PATCH/RFC] ident: die on bogus date format

If the user gives "git commit --date=foobar", we silently
ignore the --date flag. We should note the error.

This patch puts the fix at the lowest level of fmt_ident,
which means it also handles GIT_AUTHOR_DATE=foobar, as well.

There are two down-sides to this approach:

  1. Technically this breaks somebody doing something like
     "git commit --date=now", which happened to work because
     bogus data is the same as "now". Though we do
     explicitly handle the empty string, so anybody passing
     an empty variable through the environment will still
     work.

     If the error is too much, perhaps it can be downgraded
     to a warning?

  2. The error checking happens _after_ the commit message
     is written, which can be annoying to the user. We can
     put explicit checks closer to the beginning of
     git-commit, but that feels a little hack-ish; suddenly
     git-commit has to care about how fmt_ident works. Maybe
     we could simply call fmt_ident earlier?

Signed-off-by: Jeff King <peff@peff.net>
---
 ident.c           |    6 ++++--
 t/t7501-commit.sh |    4 ++++
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/ident.c b/ident.c
index 9e24388..1c4adb0 100644
--- a/ident.c
+++ b/ident.c
@@ -217,8 +217,10 @@ const char *fmt_ident(const char *name, const char *email,
 	}
 
 	strcpy(date, git_default_date);
-	if (!name_addr_only && date_str)
-		parse_date(date_str, date, sizeof(date));
+	if (!name_addr_only && date_str && date_str[0]) {
+		if (parse_date(date_str, date, sizeof(date)) < 0)
+			die("invalid date format: %s", date_str);
+	}
 
 	i = copy(buffer, sizeof(buffer), 0, name);
 	i = add_raw(buffer, sizeof(buffer), i, " <");
diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh
index 8297cb4..8980738 100755
--- a/t/t7501-commit.sh
+++ b/t/t7501-commit.sh
@@ -230,6 +230,10 @@ test_expect_success 'amend commit to fix date' '
 
 '
 
+test_expect_success 'commit complains about bogus date' '
+	test_must_fail git commit --amend --date=10.11.2010
+'
+
 test_expect_success 'sign off (1)' '
 
 	echo 1 >positive &&
-- 
1.7.3.3.784.gccc31.dirty

^ permalink raw reply related

* Re: [PATCH/RFC] t800?-blame.sh: retitle uniquely
From: Jeff King @ 2010-12-13 17:07 UTC (permalink / raw)
  To: Michael J Gruber; +Cc: git, Junio C Hamano, Johannes Sixt
In-Reply-To: <52f777e518583955f78b71e96b3c8ff53d25b608.1292256498.git.git@drmicha.warpmail.net>

On Mon, Dec 13, 2010 at 05:12:29PM +0100, Michael J Gruber wrote:

> On a related note to J6t's patch, how's the stance on unique titles?
> Unique numbers are important for partial test runs, of course,
> but unique titles help finding you way through the test.
> There are more than the blame.sh ones.

I don't think it is a big deal, but I did just 5 minutes ago get annoyed
at:

  t7500-commit.sh
  t7501-commit.sh
  t7502-commit.sh
  t7509-commit.sh

Speaking of minor test issues, we should probably also do this:

-- >8 --
Subject: [PATCH] tests: flip executable bit on t9158

All tests are supposed to be executable.

Signed-off-by: Jeff King <peff@peff.net>
---
 0 files changed, 0 insertions(+), 0 deletions(-)
 mode change 100644 => 100755 t/t9158-git-svn-mergeinfo.sh

diff --git a/t/t9158-git-svn-mergeinfo.sh b/t/t9158-git-svn-mergeinfo.sh
old mode 100644
new mode 100755
-- 
1.7.3.3.784.gccc31.dirty

^ permalink raw reply

* Re: [PATCH/RFC] t800?-blame.sh: retitle uniquely
From: Jeff King @ 2010-12-13 17:22 UTC (permalink / raw)
  To: Michael J Gruber; +Cc: git, Junio C Hamano, Johannes Sixt
In-Reply-To: <20101213170734.GA24736@sigill.intra.peff.net>

On Mon, Dec 13, 2010 at 12:07:34PM -0500, Jeff King wrote:

> Speaking of minor test issues, we should probably also do this:
> 
> -- >8 --
> Subject: [PATCH] tests: flip executable bit on t9158

BTW, I will plug my test-lint patch once again, which caught both this
and the duplicate test number mentioned earlier.

-- >8 --
Subject: [PATCH] tests: add some script lint checks

There are some common but minor errors we tend to make in
writing test scripts:

  1. Scripts are left non-executable. This is not usually
     noticed immediately because "make test" does not need
     the bit, but it is a matter of git policy to make them
     executable (and is a slight convenience when running
     individual scripts).

  2. Two scripts are allocated the same number. Usually this
     happens on separate branches, and the problem only
     comes about during a merge. But since there is no
     textual conflict, the merger would have to be very
     observant to notice.

     This is also a minor error, but can make GIT_SKIP_TESTS
     ambiguous.

This patch introduces a "test-lint" target which checks
both. It is not invoked by default. You can invoke it as
"make test-lint", or you can make it a prerequisite of
running the tests by specifying "TEST_LINT = test-lint" in
your config.mak or on the command line.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/Makefile |   16 ++++++++++++++--
 1 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/t/Makefile b/t/Makefile
index 73c6ec4..47cbeb6 100644
--- a/t/Makefile
+++ b/t/Makefile
@@ -23,10 +23,10 @@ TGITWEB = $(wildcard t95[0-9][0-9]-*.sh)
 
 all: $(DEFAULT_TEST_TARGET)
 
-test: pre-clean
+test: pre-clean $(TEST_LINT)
 	$(MAKE) aggregate-results-and-cleanup
 
-prove: pre-clean
+prove: pre-clean $(TEST_LINT)
 	@echo "*** prove ***"; GIT_CONFIG=.git/config $(PROVE) --exec '$(SHELL_PATH_SQ)' $(GIT_PROVE_OPTS) $(T) :: $(GIT_TEST_OPTS)
 	$(MAKE) clean
 
@@ -41,6 +41,18 @@ clean:
 	$(RM) -r valgrind/bin
 	$(RM) .prove
 
+test-lint: test-lint-duplicates test-lint-executable
+
+test-lint-duplicates:
+	@dups=`echo $(T) | tr ' ' '\n' | sed 's/-.*//' | sort | uniq -d` && \
+		test -z "$$dups" || { \
+		echo >&2 "duplicate test numbers:" $$dups; exit 1; }
+
+test-lint-executable:
+	@bad=`for i in $(T); do test -x "$$i" || echo $$i; done` && \
+		test -z "$$bad" || { \
+		echo >&2 "non-executable tests:" $$bad; exit 1; }
+
 aggregate-results-and-cleanup: $(T)
 	$(MAKE) aggregate-results
 	$(MAKE) clean
-- 
1.7.3.3.784.gccc31.dirty

^ permalink raw reply related

* Re: [PATCH 01/19] Add struct pathspec
From: Thiago Farina @ 2010-12-13 17:31 UTC (permalink / raw)
  To: Nguyễn Thái Ngọc Duy; +Cc: git, Junio C Hamano
In-Reply-To: <1292233616-27692-2-git-send-email-pclouds@gmail.com>

2010/12/13 Nguyễn Thái Ngọc Duy <pclouds@gmail.com>:
> This struct for now is just a wrapper for the current pathspec form:
> const char **. It is intended to be extended with more useful
> pathspec-related information over time.
>
> The data structure for passing pathspec around remains const char **,
> struct pathspec will be initialized locally to be used and destroyed.
> Hopefully all pathspec related code will be gradually migrated to pass
> this struct instead.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  cache.h |    7 +++++++
>  dir.c   |   18 ++++++++++++++++++
>  2 files changed, 25 insertions(+), 0 deletions(-)
>
> diff --git a/cache.h b/cache.h
> index 2ef2fa3..3330769 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -493,6 +493,13 @@ extern int index_name_is_other(const struct index_state *, const char *, int);
>  extern int ie_match_stat(const struct index_state *, struct cache_entry *, struct stat *, unsigned int);
>  extern int ie_modified(const struct index_state *, struct cache_entry *, struct stat *, unsigned int);
>
> +struct pathspec {
> +       const char **raw; /* get_pathspec() result, not freed by free_pathspec() */
> +       int nr;
> +};
> +
> +extern int init_pathspec(struct pathspec *, const char **);
> +extern void free_pathspec(struct pathspec *);
>  extern int ce_path_match(const struct cache_entry *ce, const char **pathspec);
>  extern int index_fd(unsigned char *sha1, int fd, struct stat *st, int write_object, enum object_type type, const char *path);
>  extern int index_path(unsigned char *sha1, const char *path, struct stat *st, int write_object);
> diff --git a/dir.c b/dir.c
> index 133f472..205adc4 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -1071,3 +1071,21 @@ int remove_path(const char *name)
>        return 0;
>  }
>
> +int init_pathspec(struct pathspec *pathspec, const char **paths)
> +{
> +       const char **p = paths;
> +
> +       memset(pathspec, 0, sizeof(*pathspec));
> +       if (!p)
> +               return 0;
> +       while (*p)
> +               p++;
> +       pathspec->raw = paths;
> +       pathspec->nr = p - paths;
> +       return 0;
> +}
> +
> +void free_pathspec(struct pathspec *pathspec)
> +{
> +       ; /* do nothing */

Of curse this ; here is not necessary :)

^ permalink raw reply

* Re: [PATCH/RFC] ident: die on bogus date format
From: Sergio Callegari @ 2010-12-13 18:00 UTC (permalink / raw)
  To: Jeff King; +Cc: git
In-Reply-To: <20101213170225.GA16033@sigill.intra.peff.net>

Thanks for looking (and coding) into it!

For my usage case, a warning would be ok too. If the commit generates a warning,
one can notice the warning and --amend the commit.

One can perhaps have the warning now and maybe an
error from version 1.8, so: (i) we are sure that the different behavior does not 
break anything and
(ii) there is time to move the fmt_ident up in the code so that it is invoked 
earlier
(to avoid the burden of writing the commit message and then seeing the commit 
aborted).

By the way, note that I got into the issue by trying to use --date forgetting 
the time.
Don't know if it could make sense to accept the date with some default time in 
this case.

Regards

Sergio

^ permalink raw reply

* Re: [PATCH 08/19] pathspec: mark wildcard pathspecs from the beginning
From: Junio C Hamano @ 2010-12-13 18:09 UTC (permalink / raw)
  To: Nguyễn Thái Ngọc Duy; +Cc: git, Junio C Hamano
In-Reply-To: <1292233616-27692-9-git-send-email-pclouds@gmail.com>

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> diff --git a/dir.c b/dir.c
> index 646c79f..0987d0c 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -1089,8 +1089,17 @@ int init_pathspec(struct pathspec *pathspec, const char **paths)
>  	pathspec->items = xmalloc(sizeof(struct pathspec_item)*pathspec->nr);
>  	for (i = 0; i < pathspec->nr; i++) {
>  		struct pathspec_item *item = pathspec->items+i;
> -
> -		item->len = strlen(paths[i]);
> +		const char *path = paths[i];
> +
> +		item->len = strlen(path);
> +		item->has_wildcard = !no_wildcard(path);
> +		if (item->has_wildcard) {
> +			pathspec->has_wildcard = 1;
> +			item->prefix_len = 0;
> +			while (item->prefix_len < item->len &&
> +			       strchr("*?[{\\", path[item->prefix_len]) == NULL)
> +				item->prefix_len++;
> +		}

Would it make sense to use strcspn(3) here?

>  	}
>  	return 0;
>  }
> -- 
> 1.7.3.3.476.g10a82

^ permalink raw reply

* Re: [PATCH 09/19] tree-diff.c: reserve space in "base" for pathname concatenation
From: Junio C Hamano @ 2010-12-13 18:10 UTC (permalink / raw)
  To: Nguyễn Thái Ngọc Duy; +Cc: git
In-Reply-To: <1292233616-27692-10-git-send-email-pclouds@gmail.com>

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> This patch make sure that "base" parameter is writable. The callees
> are free to modify it as long as base remains the same before
> entering and after leaving the callee.
>
> This avoids quite a bit of malloc and memcpy().
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---

I like what I see in this patch in general, but there is nothing that
guarantees that you are "reserving" enough space.  Doesn't the buffer
overflow with deep enough trees?

^ permalink raw reply

* Re: [PATCH 10/19] tree_entry_interesting(): factor out most matching logic
From: Junio C Hamano @ 2010-12-13 18:10 UTC (permalink / raw)
  To: Nguyễn Thái Ngọc Duy; +Cc: git
In-Reply-To: <1292233616-27692-11-git-send-email-pclouds@gmail.com>

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> diff --git a/tree-walk.c b/tree-walk.c
> index 01168ea..40a4657 100644
> --- a/tree-walk.c
> +++ b/tree-walk.c
> @@ -481,88 +566,21 @@ int tree_entry_interesting(const struct name_entry *entry,
>  	for (i = 0; i < ps->nr; i++) {
>  		const char *match = ps->raw[i];
>  		int matchlen = ps->items[i].len;
> -		int m = -1; /* signals that we haven't called strncmp() */
>  
>  		if (baselen >= matchlen) {
> -			/* If it doesn't match, move along... */

Why lose this comment?  That is exactly what this simplified part is
about.  If match-dir-prefix says it doesn't match, we move along.

Keep this comment here, and drop the one you copied to the beginning of
match-dir-prefix, which is not about "moving along" anymore.  The function
is about deciding if they match or not.

> -			if (strncmp(base, match, matchlen))
> +			if (!match_dir_prefix(base, baselen, match, matchlen))
> +				/* Just a random prefix match */
>  				continue;

This comment no longer holds true, even though the same comment at the end
of match-dir-prefix function is correct.  This should probably say "does
not match", but that is something you already said.

Other than that, this is a very nice refactoring.

^ permalink raw reply

* Re: [PATCH 13/19] tree_entry_interesting(): optimize fnmatch when base is matched
From: Junio C Hamano @ 2010-12-13 18:10 UTC (permalink / raw)
  To: Nguyễn Thái Ngọc Duy; +Cc: git
In-Reply-To: <1292233616-27692-14-git-send-email-pclouds@gmail.com>

Looks good.

^ permalink raw reply

* Re: [PATCH 12/19] tree_entry_interesting(): support wildcard matching
From: Junio C Hamano @ 2010-12-13 18:10 UTC (permalink / raw)
  To: Nguyễn Thái Ngọc Duy; +Cc: git
In-Reply-To: <1292233616-27692-13-git-send-email-pclouds@gmail.com>

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  tree-walk.c |   33 ++++++++++++++++++++++++++++++---
>  tree-walk.h |    2 +-
>  2 files changed, 31 insertions(+), 4 deletions(-)

;-)  Looks almost too easy.

> diff --git a/tree-walk.c b/tree-walk.c
> index d28de30..b5ad42b 100644
> --- a/tree-walk.c
> +++ b/tree-walk.c
> @@ -596,6 +598,31 @@ int tree_entry_interesting(const struct name_entry *entry,
>  					&never_interesting))
>  				return 1;
>  		}
> +
> +match_wildcards:
> +		/*
> +		 * Concatenate base and entry->path into one and do
> +		 * fnmatch() on it.
> +		 */
> +
> +		if (!item->has_wildcard)
> +			continue;

I think the comment comes after this if--continue.

> +		never_interesting = 0;

When we have wildcard we would want to disable the never-interesting
optimization, but I wonder if doing so only when we do not have exact hit
is what we want.  If a sick person had a path "a?b" tracked, and asked to
match a pathspec "a?b", don't we still want to say "'a?b' of course
matches, but 'a1b' ('1' comes earlier than '?' in the sort order) and
'aAb' ('A' comes later) also match"?

^ permalink raw reply

* Re: [PATCH 11/19] tree_entry_interesting: support depth limit
From: Junio C Hamano @ 2010-12-13 18:10 UTC (permalink / raw)
  To: Nguyễn Thái Ngọc Duy; +Cc: git
In-Reply-To: <1292233616-27692-12-git-send-email-pclouds@gmail.com>

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> diff --git a/dir.c b/dir.c
> index 0987d0c..bb5076c 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -71,6 +71,21 @@ int fill_directory(struct dir_struct *dir, const char **pathspec)
>  	return len;
>  }
>  
> +int within_depth(const char *name, int namelen,
> +			int depth, int max_depth)
> +{
> +	const char *cp = name, *cpe = name + namelen;
> +
> +	while (cp < cpe) {
> +		if (*cp++ != '/')
> +			continue;
> +		depth++;
> +		if (depth > max_depth)
> +			return 0;
> +	}
> +	return 1;
> +}

Makes me almost suspect that it may make more sense to keep track of the
"depth" in a similar way as "base" and "baselen" as "traversal state" on
the side of the caller so that you do not have to scan the string for
slashes over and over again.  But given the codeflow, doing so might make
the result look too ugly, so I won't recommend that without thinking,
though.

> diff --git a/tree-walk.c b/tree-walk.c
> index 40a4657..d28de30 100644
> --- a/tree-walk.c
> +++ b/tree-walk.c
> @@ -558,9 +559,17 @@ int tree_entry_interesting(const struct name_entry *entry,
>  	int pathlen;
>  	int never_interesting = -1;
>  
> -	if (!ps || !ps->nr)
> +	if (!ps)
>  		return 1;
>  
> +	if (!ps->nr) {
> +		if (!ps->recursive || ps->max_depth == -1)
> +			return 1;
> +		return !!within_depth(base, baselen,
> +				      !!S_ISDIR(entry->mode),
> +				      ps->max_depth);
> +	}

This gives different behaviour to between callers that give you NULL as
pathspec and callers that give you a pathspec with zero elements.  Is this
intended?  What is the use case (iow, what does an end user give from the
command line to experience this difference)?

> @@ -571,7 +580,13 @@ int tree_entry_interesting(const struct name_entry *entry,
>  			if (!match_dir_prefix(base, baselen, match, matchlen))
>  				/* Just a random prefix match */
>  				continue;
> -			return 2;
> +
> +			if (!ps->recursive || ps->max_depth == -1)
> +				return 2;
> +
> +			return !!within_depth(base+matchlen+1, baselen-matchlen-1,
> +					      !!S_ISDIR(entry->mode),
> +					      ps->max_depth);
>  		}

If two pathspecs that overlap with each other (e.g. "Documentation/" and
"Documentation/technical") are given, and if the shorter one comes before
the longer one in ps[], wouldn't this give you an unexpected result?  When
inspecting "Documentation/technical/api/foo.txt" with depth limit of 2, if
you didn't have "Documentation/" pathspec, you count "api/foo.txt"
relative to "Documentation/technical", declare that the path is within
limit, and show it.  But if you have "Documentation/" in ps[], you look at
it, decide "technical/api/foo.txt" is too deep and return false without
even looking at "Documentation/technical" that may appear later in ps[],
no?

^ permalink raw reply

* Re: Corruption in the repository
From: Sylvestre Ledru @ 2010-12-13 18:11 UTC (permalink / raw)
  To: git, Simon Gareste, Bruno JOFRET
In-Reply-To: <AANLkTimgLhQMUGmC=W5wpcAvb07faw4HOzPgWXpFA4u1@mail.gmail.com>

Le lundi 13 décembre 2010 à 18:02 +0100, Christian Couder a écrit :
> On Mon, Dec 13, 2010 at 5:47 PM, Sylvestre Ledru
> <sylvestre.ledru@scilab.org> wrote:
> >
> > Le lundi 13 décembre 2010 à 17:34 +0100, Christian Couder a écrit :
> >> Hi,
> >>
> >> On Mon, Dec 13, 2010 at 4:59 PM, Sylvestre Ledru
> >> <sylvestre.ledru@scilab.org> wrote:
> >> > Hello guys,
> >> >
> >> > I have a small problem with a git repository and I haven't find a way to
> >> > fix my problem.
> >> > I am using git with gerrit [1] as frontend (even if I don't think it is
> >> > related here).
> >> > For an unknown reason, the repository just became corrupted.
> >> >
> >> > When I try to clone the repository straight with the file system, the
> >> > following error is displayed:
> >> > error: refs/changes/98/398/1 does not point to a valid object!
> >> > error: refs/changes/98/398/2 does not point to a valid object!
> >> > fatal: object cff52c24fba28408e7d021a8f35a717bef31521d is corrupted
> >> > fatal: The remote end hung up unexpectedly
> >> >
> >> > git-prune & git-fsck both fail.
> >> >
> >> > Does anyone know how to repair this error ?
> >>
> >> Did you try what the FAQ suggests:
> >>
> >> https://git.wiki.kernel.org/index.php/GitFaq#How_to_fix_a_broken_repository.3F
> > Yes. It shows an other error and it doesn't match the error described on
> > this URL [1].
> >
> > I get the following:
> > error: refs/tags/5.3.0-beta-4 does not point to a valid object!
> > fatal: object 555a7c359b2e589ec10822d9b56cdfeee0105fe0 is corrupted
> 
> The FAQ says that you should try to replace any broken and/or missing
> objects, so you should try to do that with object
> 555a7c359b2e589ec10822d9b56cdfeee0105fe0 (as described in the FAQ)
> even if the error message is not exactly the same.
Well, after clean the error about the tags, it does not provide any
interesting feedbacks like in the FAQ.

Just some information [1] about some dangling commits (probably commit
under review in gerrit). Nothing valuable and the problem still occurs
on the client side.

Sylvestre
[1] http://paste.debian.net/102297/

^ permalink raw reply

* Re: [PATCH] completion: Add PS1 configuration for submodules
From: Jens Lehmann @ 2010-12-13 18:12 UTC (permalink / raw)
  To: Jonathan Nieder
  Cc: Scott Kyle, Kevin Ballard, Ævar Arnfjörð, git
In-Reply-To: <20101212063848.GB17185@burratino>

Am 12.12.2010 07:38, schrieb Jonathan Nieder:
> Scott Kyle wrote:
>> On Tue, Dec 7, 2010 at 1:29 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
>>> Scott Kyle wrote:
> 
>>>> If I set the "submodule.<name>.ignore" then diffing around inside my
>>>> history will not show the changes to that particular submodule.
>>>
>>> Even if you set it to "dirty"?
>>
>> Setting it to "dirty" is far less disruptive, you're right, but that
>> wouldn't do me much good since my submodules are often on different
>> branches while developing.
> 
> Ah, I see now.  How about something like this?  Untested, just a
> vague sketch to show the idea.

Me thinks your proposal of a new "worktree" option makes sense. Let's
hear what Scott says ...

^ permalink raw reply

* Splitting a repository but sharing the common parts of the object database
From: Phillip Susi @ 2010-12-13 18:39 UTC (permalink / raw)
  To: git

I have recently started a new project that has begun its life as a fork
of an old project, then heavily modified.  At the same time I am looking
into migrating from svn to git.  Since this is a separate project, I
thought I should split it into a separate git repository and keep them
separate going forward, however, since they have common ancestry up to
this point, I would like to make sure that all of that data is not
duplicated.

I thought what I could do is set up an initial repository that is fully
packed and use it as a read only archive to contain all of the shared
history, then clone that into two new repositories for the two separate
projects.  It seems like I can do this by running a clone -s so that the
archive is set up to be an alternate object store, then its big pack
file will be found and used for all of the old history, but new commits
will be specific to each of the two new respective projects.  The
problem I run into though, is when it comes time to repack the new projects.

If I run a repack -a, then the new project has everything copied out of
the archive and into its new main pack, rather than continuing to use
the archive repository for old history, and just pack everything since
then.  I guess I am looking for is somewhere between a full repack and
an incremental; a way to make repack -a discard existing local packs,
but to respect the alternate packs and omit objects they contain from
the new local pack.  Is this possible?

^ permalink raw reply

* Re: [PATCH 04/19] Convert struct diff_options to use struct pathspec
From: Junio C Hamano @ 2010-12-13 19:00 UTC (permalink / raw)
  To: Nguyễn Thái Ngọc Duy; +Cc: git
In-Reply-To: <1292233616-27692-5-git-send-email-pclouds@gmail.com>

Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:

> diff --git a/diff-lib.c b/diff-lib.c
> index 392ce2b..3b809f2 100644
> --- a/diff-lib.c
> +++ b/diff-lib.c
> @@ -501,7 +501,7 @@ int do_diff_cache(const unsigned char *tree_sha1, struct diff_options *opt)
>  	active_nr = dst - active_cache;
>  
>  	init_revisions(&revs, NULL);
> -	revs.prune_data = opt->paths;
> +	revs.prune_data = opt->pathspec.raw;
>  	tree = parse_tree_indirect(tree_sha1);
>  	if (!tree)
>  		die("bad tree object %s", sha1_to_hex(tree_sha1));

Hopefully the prune_data will become opt->pathspec not "raw" and use your
generalied/unified pathspec matching code in later patches in the series,
yes?

Other than that (no, "including that", really), looks nicely done.

^ permalink raw reply

* Re: [PATCH 05/19] tree_entry_interesting(): remove dependency on struct diff_options
From: Junio C Hamano @ 2010-12-13 19:11 UTC (permalink / raw)
  To: Nguyễn Thái Ngọc Duy; +Cc: git
In-Reply-To: <1292233616-27692-6-git-send-email-pclouds@gmail.com>

Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:

> This function can be potentially used in more places than just
> tree-diff.c. "struct diff_options" does not make much sense outside
> diff_tree_sha1().
>
> While removing the use of diff_options, it also removes
> tree_entry_extract() call, which means S_ISDIR() uses the entry->mode
> directly, without being filtered by canon_mode() (called internally
> inside tree_entry_extract)
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>

The patch looks good, but the second paragraph above sounded unnecessarily
alarming and I had to read it three times to make sure nothing fishy is
going on ;-).

It bypasses tree-entry-extract call and uses fields of "entry" directly.
With the change, entry->mode is used without first getting normalized with
canon_mode(), but the only use of the mode information in this function is
to check the type of the entry by giving it to S_ISDIR() macro, and the
result does not change with or without canon_mode(), so it is Ok.

^ permalink raw reply

* Re: What's cooking in git.git (topics)
From: Yaroslav Halchenko @ 2010-12-13 19:09 UTC (permalink / raw)
  To: git
In-Reply-To: <7vtzen7bul.fsf@gitster.siamese.dyndns.org>

Dear Everyone,

> I tried not to sound too negative when describing -Xours and -Xtheirs
> there, but actually I think "-s theirs" is even worse.  It is how you
> would discard what you did (perhaps because the other side has much better
> solution than your hack)

or perhaps this is very well intended, e.g. if you are tracking other project's
development and just need to carry a limited portion of the source tree.

Sorry for reincarnating this old thread, but  since I have filed Debian
wishlist bug against GIT for '-s theirs':

http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=581680

and this thread is linked to it as the ultimate source 'why not (yet)', I want
to describe a usecase when/why I wanted to have -s theirs:

For debian packaging, we often need to clean up the upstream source tree so it
does not contain non-free material (binary blobs, components under non-free
licenses, etc).  If I want to setup all packaging within GIT, I would follow
natively following setup:

git checkout -b dfsg 0.1
git rm non-free-1 non-free-2 ...
git commit -m "DFSG-compliant 0.1"
git tag -a -m 0.1.dfsg

and base my packaging off dfsg branch (in a separate branch, e.g. debian).

Upon release 0.2 of upstream work, in the simplest case, I can do now

git checkout dfsg
git merge 0.2

and there things could get hairy -- if files were modified upstream, I get
conflicts, so I would need to git rm files again, and only then commit the
merge:

git rm 

Moreover, 0.2 might not follow 0.1 -- upstream might release off
"release-branches", then I simply *must not* do "git merge" with recursive 
strategy.

Moreover, if some material finally became free, I would need to re-add it
somehow into dfsg branch from 0.2 branch.


*All* those complications could easily be avoided if I only had '-s theirs'.
Then I simply

git checkout dfsg
git merge --no-commit -s theirs 0.2
# after all I do not, and must not have my modifications
git rm -rf non-free-1 ... # probably would be scripted
git commit

With -s theirs now I would be able manage all tricky cases above without hassle
in a unified way.

Would it be possible to have GIT people reconsider addition of '-s theirs'?

Thank you in advance for your time!

^ permalink raw reply

* Re: Splitting a repository but sharing the common parts of the object database
From: Jonathan Nieder @ 2010-12-13 19:20 UTC (permalink / raw)
  To: Phillip Susi; +Cc: git
In-Reply-To: <4D066873.4020208@cfl.rr.com>

Hi Phillip,

Phillip Susi wrote:

> If I run a repack -a, then the new project has everything copied out of
> the archive and into its new main pack, rather than continuing to use
> the archive repository for old history, and just pack everything since
> then.  I guess I am looking for is somewhere between a full repack and
> an incremental; a way to make repack -a discard existing local packs,
> but to respect the alternate packs and omit objects they contain from
> the new local pack.

You might be interested in girocco's fork support.  See
http://repo.or.cz/w/girocco.git/blob/HEAD:/jobd/gc.sh for starters.

(Yes, the short answer is "-l" but showing where I got that answer
from seems a little easier. :))

^ permalink raw reply

* Re: [PATCH 15/19] pathspec: add match_pathspec_depth()
From: Junio C Hamano @ 2010-12-13 19:28 UTC (permalink / raw)
  To: Nguyễn Thái Ngọc Duy; +Cc: git
In-Reply-To: <1292233616-27692-16-git-send-email-pclouds@gmail.com>

Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:

> match_pathspec_depth() is similar to match_pathspec() except that it
> can take depth limit.
>
> In long term, match_pathspec() should be removed in favor of this
> function.

Hmm, this strongly suggests that match_pathspec() should take "const
struct pathspec *" which already contains the necessary information and
more, including the depth limit, no?

> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  dir.c |   45 +++++++++++++++++++++++++++++++++++++++++++++
>  dir.h |    3 +++
>  2 files changed, 48 insertions(+), 0 deletions(-)
>
> diff --git a/dir.c b/dir.c
> index bb5076c..e12dbdd 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -169,6 +169,51 @@ int match_pathspec(const char **pathspec, const char *name, int namelen,
>  	return retval;
>  }
>  
> +int match_pathspec_depth(const char **pathspec, int max_depth,
> +			 const char *name, int namelen,
> +			 int prefix, char *seen)
> +{
> +	int i, retval = 0;
> +
> +	if (!pathspec) {
> +		if (max_depth == -1)
> +			return MATCHED_RECURSIVELY;
> +
> +		if (within_depth(name, namelen, 0, max_depth))
> +			return MATCHED_EXACTLY;

Why the difference between _RECURSIVELY and _EXACTLY here?  If you have a
five-level deep project and give max-depth of 1000, shouldn't you get the
same result as you run the same command with unlimited depth?

^ permalink raw reply

* Re: [PATCH 14/19] Convert ce_path_match() use to match_pathspec()
From: Junio C Hamano @ 2010-12-13 19:31 UTC (permalink / raw)
  To: Nguyễn Thái Ngọc Duy; +Cc: git
In-Reply-To: <1292233616-27692-15-git-send-email-pclouds@gmail.com>

Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:

> Previously ce_path_match() is used together with tree_entry_interesting().
> Both do not support wildcards. tree_entry_interesting() understands
> wildcards now, so it's time to teach ce_path_match() to do the same.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---

This function is called from both Porcelains (e.g. "git diff" via
diff-lib.c and "git status" via wt-status.c) and plumbing commands
(e.g. "git update-index"), and we are changing the semantics in a big way,
even though it is a huge improvement.

I am fairly excited with the progress of this series.  When it gets merged
to 'master', we will be declaring 1.8.0 ;-)

I imagine that eventually ce_path_match() will also take "struct pathspec *"
not "const char **" when the series is completed, yes?  Or there is no
real need for that?

>  read-cache.c |   20 +-------------------
>  1 files changed, 1 insertions(+), 19 deletions(-)
>
> diff --git a/read-cache.c b/read-cache.c
> index 1f42473..cbabd8b 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -685,28 +685,10 @@ int ce_same_name(struct cache_entry *a, struct cache_entry *b)
>  
>  int ce_path_match(const struct cache_entry *ce, const char **pathspec)
>  {
> -	const char *match, *name;
> -	int len;
> -
>  	if (!pathspec)
>  		return 1;
>  
> -	len = ce_namelen(ce);
> -	name = ce->name;
> -	while ((match = *pathspec++) != NULL) {
> -		int matchlen = strlen(match);
> -		if (matchlen > len)
> -			continue;
> -		if (memcmp(name, match, matchlen))
> -			continue;
> -		if (matchlen && name[matchlen-1] == '/')
> -			return 1;
> -		if (name[matchlen] == '/' || !name[matchlen])
> -			return 1;
> -		if (!matchlen)
> -			return 1;
> -	}
> -	return 0;
> +	return match_pathspec(pathspec, ce->name, ce_namelen(ce), 0, NULL);
>  }
>  
>  /*
> -- 
> 1.7.3.3.476.g10a82

^ permalink raw reply

* Re: [PATCH/RFC] t800?-blame.sh: retitle uniquely
From: Junio C Hamano @ 2010-12-13 19:38 UTC (permalink / raw)
  To: Jeff King; +Cc: Michael J Gruber, git, Johannes Sixt
In-Reply-To: <20101213172238.GA16136@sigill.intra.peff.net>

Thanks.

^ permalink raw reply

* Re: Splitting a repository but sharing the common parts of the object database
From: Stephen Bash @ 2010-12-13 19:50 UTC (permalink / raw)
  To: Jonathan Nieder; +Cc: git, Phillip Susi
In-Reply-To: <20101213192053.GA30315@burratino>

----- Original Message -----
> From: "Jonathan Nieder" <jrnieder@gmail.com>
> To: "Phillip Susi" <psusi@cfl.rr.com>
> Cc: git@vger.kernel.org
> Sent: Monday, December 13, 2010 2:20:53 PM
> Subject: Re: Splitting a repository but sharing the common parts of the object database
> Hi Phillip,
> 
> Phillip Susi wrote:
> 
> > If I run a repack -a, then the new project has everything copied out
> > of the archive and into its new main pack, rather than continuing to
> > use the archive repository for old history, and just pack everything
> > since then. I guess I am looking for is somewhere between a full repack
> > and an incremental; a way to make repack -a discard existing local
> > packs, but to respect the alternate packs and omit objects they contain
> > from the new local pack.
> 
> You might be interested in girocco's fork support. See
> http://repo.or.cz/w/girocco.git/blob/HEAD:/jobd/gc.sh for starters.

You might also be interested in Pro Git's entry on git-replace:

  http://progit.org/2010/03/17/replace.html

It's a completely different approach than what you're suggesting, but might open up new and interesting avenues.

Stephen

^ permalink raw reply


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