Re: [PATCHv2 2/7] xread: poll on non blocking fds

[Torsten Bögershausen]

On 18.12.15 04:13, Jeff King wrote:
> On Thu, Dec 17, 2015 at 09:42:01PM +0100, Torsten Bögershausen wrote:
> 
>>> Or do you mean to insert another continue in here?
>> I was thinking that we run into similar loop as before:
>> read() returns -1; errno = EAGAIN  /* No data to read */
>> poll() returns -1; errno = EAGAIN /* poll failed. If the fd was OK, the 
>> failure may be temporaly,
>> as much as poll() can see.
>> But most probably we run out ouf memory 
>> */
>>
>> So the code would look like this:
>>
>>if (!poll(, 1, -1))
>>   return -1;
> 
> That changes the semantics of the function. The poll() is just a
> convenience to avoid spinning. If it fails, with Stefan's patch[1] the
> worst case is that we would spin on read() and poll(), instead of
> actually blocking in the poll().
> 
> But if we return on poll() failure, now the caller will see errors from
> poll() even though they don't know or care that we called poll() in the
> first place. Consider what would happen with your code if read got
> EAGAIN and then poll got EINTR. We would report an error, even though
> the whole point of xread() is to loop on these conditions.
> 
> -Peff
> 
> [1] Stefan's patch does have a bug. It forgets to "continue" after
> calling poll, which means we will block with poll and _then_ exit
> with -1, instead of restarting the loop.
> --
I love this group: Curing one bug with another doesn't work :-)

/* So the code v2 would look like this: */

if (!poll(, 1, -1)) {
if (errno == EINTR)
continue;
 return -1; /* poll() failed, this is serious. */
}


--
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 v2 10/10] dir: do not use untracked cache ident anymore

[Christian Couder]

On Thu, Dec 17, 2015 at 7:33 PM, Junio C Hamano  wrote:
> Christian Couder  writes:
>
>> In the "git worktree" documentation there is:
>>
>> "If you move a linked working tree to another file system, or within a
>> file system that does not support hard links, you need to run at least
>> one git command inside the linked working tree (e.g. git status) in
>> order to update its administrative files in the repository so that
>> they do not get automatically pruned."
>>
>> It looks like git can detect when a worktree created with "git
>> worktree" has been moved and I wonder if it would be possible to
>> detect if the main worktree pointed to by GIT_WORK_TREE as moved.
>
> As I personally do not find "moving a working tree" a very
> compelling use case, I'd be fine if cached information is not used
> when the actual worktree and the root of the cached untracked paths
> are different.

Yeah, I could just discard and recreate the UC from scratch if the
actual worktree and the root of the UC paths are different.

> If you are going to change the in-index data of untracked cache
> anyway (like you attempted with 10/10 patch), I think a lot more
> sensible simplification may be to make the mechanism _always_ keep
> track of the worktree that is rooted one level above the index, and
> not use the cache in all other cases.  That way, if you move the
> working tree in its entirety (i.e. $foo/{Makefile,.git/,untracked}
> all move to $bar/. at the same time), the untracked cache data that
> was in $foo/.git/index, which knew about $foo/untracked, will now
> know about $bar/untracked when the index is moved to $bar/.git/index
> automatically.

I am ok with that, though I worry a bit about some people having a
setup where they always use a worktree that is not one level above the
.git directory.
--
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 v2] mingw: emulate write(2) that fails with a EPIPE

[Johannes Schindelin]

Hi Junio,

On Thu, 17 Dec 2015, Junio C Hamano wrote:

> PLEASE DON'T DO THE BELOW TO THE SAME MESSAGE AS THE PATCH ITSELF.
> "git apply" would not read and understand the next line as a natural
> language sentence for obvious reasons.
> 
> Johannes Schindelin  writes:
> 
> > Interdiff vs v1:

Whoops. This is an obvious bug in my patch series submission script. Will
fix.

Sorry,
Dscho
--
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: What's cooking in git.git (Dec 2015, #05; Tue, 15)

[SZEDER Gábor]

> On the other hand, I've marked a handful of topics below as "Will
> discard". They were all dormant after waiting for updates for quite
> a long time; interested people may want to help resurrect them.

> * sg/pretty-more-date-mode-format (2015-10-07) 1 commit
>  - pretty: add format specifiers for short and raw date formats
> 
>  Introduce "%as" and "%aR" placeholders for "log --format" to show
>  the author date in the short and raw formats.
> 
>  No comments after waiting for a long time.
>  Will discard.

By adding missing date format specifiers this patch improves
consistency, improves usability of pretty format aliases, benefits at
least one user, and does nothing wrong in its implementation.  I'm not
aware that any updates were necessary to move this forward.

Why discard?

--
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: (unknown)

[David Greene]

Patrick Steinhardt  writes:

> On Tue, Dec 15, 2015 at 09:57:50PM -0800, Junio C Hamano wrote:
>> David Greene  writes:
>> 
>> > - If new option --keep-redundant is specified, invoke cherry-pick with
>> >   --keep-redundant-commits.
>> 
>> This came up in the past several weeks, I think; you would need to
>> disable patch-equivalence based commit filtering if you really want
>> to do a --keep-redundant that is reproducible and/or reliable.
>
> Here are the links to the previous proposal [1] and following
> discussion [2] (see 'ps/rebase-keep-empty') if you are
> interested.
>
> Patrick
>
> [1]: http://thread.gmane.org/gmane.comp.version-control.git/281515[2]: 
> http://thread.gmane.org/gmane.comp.version-control.git/281917

Thanks.  That makes total sense.

I actually would prefer a behavior where cherry-pick would just drop
redundant commits rather than stopping and asking the user to reset.
The problem is that rebase --preserve-merges seems to force the drop to
use cherry-pick and cherry-pick doesn't behave well (from a scripting
perspective) in the presence of redundant commits.

As it is, it's difficult to rebase as part of a scripted operation due
to this issue.

Any ideas on how to teach cherry-pick to automatically drop such
commits?

   -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

Re: [PATCHv2 2/7] xread: poll on non blocking fds

[Jeff King]

On Fri, Dec 18, 2015 at 09:50:46AM +0100, Torsten Bögershausen wrote:

> >> So the code would look like this:
> >>
> >>if (!poll(, 1, -1))
> >>   return -1;
> > 
> > That changes the semantics of the function. The poll() is just a
> > convenience to avoid spinning. If it fails, with Stefan's patch[1] the
> > worst case is that we would spin on read() and poll(), instead of
> > actually blocking in the poll().
> > 
> > But if we return on poll() failure, now the caller will see errors from
> > poll() even though they don't know or care that we called poll() in the
> > first place. Consider what would happen with your code if read got
> > EAGAIN and then poll got EINTR. We would report an error, even though
> > the whole point of xread() is to loop on these conditions.
> [...]
>
> /* So the code v2 would look like this: */
> 
> if (!poll(, 1, -1)) {
> if (errno == EINTR)
> continue;
>  return -1; /* poll() failed, this is serious. */
> }

That solves the EINTR problem, but I still don't see why we want to
return -1. The caller asked us to read(). We know that read() did not
fail with an actual error. Yet we are going to return an error to the
user, with errno set to something related only to poll(). I think we are
better off to keep the same semantics from the caller's point of view:
we loop until read() returns forward progress or a real error, and
anything else we do is a behind-the-scenes optimization.

BTW, I am assuming you mean:

  if (poll(, 1, -1) < 0)
...

in your examples. Returning "0" means that poll timed out, but of course
we are not providing a timeout.

-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: Odd rebase behavior

[David A. Greene]

John Keeping  writes:

> It seems that the problem is introduces by --preserve-merges (and
> -Xsubtree causes something interesting to happen as well).  I see the
> following behaviour:

Thanks for narrowing this down!  Is it possible this is actually a
cherry-pick problem since --preserve-merges forces rebase to use
cherry-pick?

> git rebase -Xsubtree=files_subtree --onto files-master master
>
>   fatal: Could not parse object 
> 'b15c4133fc3146e1330c84159886f0f7a09fbf43^'
>   Unknown exit code (128) from command: git-merge-recursive
> b15c4133fc3146e1330c84159886f0f7a09fbf43^ -- HEAD
> b15c4133fc3146e1330c84159886f0f7a09fbf43

Ah, good!  I had seen this behavior as well but couldn't remember what I
did to trigger it.

I don't think I have the expertise to fix rebase and/or cherry-pick.
What's the process for adding these tests to the testbase and marking
them so the appropriate person can fix them?  I see a lot of TODO tests.
Should I mark these similarly and propose a patch to the testbase?

 -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

Re: Help debugging git-svn

[Edmundo Carmona Antoranz]

Ok I came up with another idea to avoid having to deal with the
old svn history (I'm having no problems fetching/dcommitting with my
current repo). I already have the branches I work with, the thing is
that the revisions I fetched before I started using the svn authors
file have nasty IDs instead of human names. I though that I could
create a script to rewrite the whole history of the project adjusting
the usernames to real names (I'll take care of the details, no problem
there... just found out about filter-branch, could work for what I'm
thinking of). My question would be in the direction of rebuilding
git-svn metainfo once I'm done so that I can continue fetching from
svn as if nothing had happened. I checked the documentation in
git-scm.com but didn't find the details about it. Is there a
straight-forward document that explains how the git-svn metadata files
are built?

Thanks in advance.

On Wed, Dec 16, 2015 at 6:36 AM, Edmundo Carmona Antoranz
 wrote:
> On Wed, Dec 16, 2015 at 1:41 AM, Eric Wong  wrote:
>>
>> Any chance you can reproduce this on a Linux system?
>> I do not use non-Free systems and have no debugging experience
>> there at all.
>>
>
> My wish But it's a big resounding "no".
>
>>> With my very flawed knowledge of perl I have seen that the process is
>>> getting to Ra.pm around here:
>>
>> It could also be a bug in the SVN bindings or the port of
>> Perl.  Which versions are you running?
>
> I'm working on git for windows 2.6.3. I think I could use cygwin, just
> in case (I used it before).
>
>>
>>
>> I've also been wondering about the motivation of SVN developers to do a
>> good job on maintaining their Perl bindings (given git-svn is likely the
>> main user of them).
>> We've certainly had problems in the past with SVN breaking and
>> causing git-svn to segfault around 2011-2012; but it's been a while...
--
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: Odd rebase behavior

[John Keeping]

On Fri, Dec 18, 2015 at 11:43:16AM -0600, David A. Greene wrote:
> John Keeping  writes:
> 
> > It seems that the problem is introduces by --preserve-merges (and
> > -Xsubtree causes something interesting to happen as well).  I see the
> > following behaviour:
> 
> Thanks for narrowing this down!  Is it possible this is actually a
> cherry-pick problem since --preserve-merges forces rebase to use
> cherry-pick?

I'm pretty sure this a result of the code in git-rebase--interactive.sh
just below the comment "Watch for commits that have been dropped by
cherry-pick", which filters out certain commits.  However, I'm not at
all familiar with the --preserve-merges code in git-rebase so I could be
completely wrong.

> > git rebase -Xsubtree=files_subtree --onto files-master master
> >
> > fatal: Could not parse object 
> > 'b15c4133fc3146e1330c84159886f0f7a09fbf43^'
> > Unknown exit code (128) from command: git-merge-recursive
> > b15c4133fc3146e1330c84159886f0f7a09fbf43^ -- HEAD
> > b15c4133fc3146e1330c84159886f0f7a09fbf43
> 
> Ah, good!  I had seen this behavior as well but couldn't remember what I
> did to trigger it.
> 
> I don't think I have the expertise to fix rebase and/or cherry-pick.
> What's the process for adding these tests to the testbase and marking
> them so the appropriate person can fix them?  I see a lot of TODO tests.
> Should I mark these similarly and propose a patch to the testbase?

I think marking them with test_expect_failure (instead of
test_expect_success) is enough.
--
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

git leaves behind .git/COMMIT_EDITMSG non-shared in --shared non-bare repo

[Yaroslav Halchenko]

Not sure for what batch operations that file is actually useful, but the
story is that if we have a shared git repo (I know -- might not be as
common of a situation but possible and allowed to happen), then if one
from the shared group commits within that repository, it becomes
impossible for another person to commit.  

git does take care about chmod'ing all the files under .git/objects etc
for --shared operation, but leaves .git/COMMIT_EDITMSG at the
mercy of user's umask.  IMHO correct resolution, if leaving that file
behind is necessary, is to chmod it in the same fashion as any other
internal .git file in --shared mode -- with group write permission.

I have reproduced that behavior with today's version of git as of
2.7.0.rc1.5.gf3adf45.

-- 
Yaroslav O. Halchenko
Center for Open Neuroscience http://centerforopenneuroscience.org
Dartmouth College, 419 Moore Hall, Hinman Box 6207, Hanover, NH 03755
Phone: +1 (603) 646-9834   Fax: +1 (603) 646-1419
WWW:   http://www.linkedin.com/in/yarik
--
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 2/3] t5304: Add test for .bitmap garbage files

[Doug Kelly]

When checking for pack garbage, .bitmap files are now detected as
garbage when not associated with another .pack/.idx file.

Signed-off-by: Doug Kelly 
---
 t/t5304-prune.sh | 24 +---
 1 file changed, 21 insertions(+), 3 deletions(-)

diff --git a/t/t5304-prune.sh b/t/t5304-prune.sh
index 1ea8279..4fa6e7a 100755
--- a/t/t5304-prune.sh
+++ b/t/t5304-prune.sh
@@ -230,6 +230,12 @@ test_expect_success 'garbage report in count-objects -v' '
: >.git/objects/pack/fake.idx &&
: >.git/objects/pack/fake2.keep &&
: >.git/objects/pack/fake3.idx &&
+   : >.git/objects/pack/fake4.bitmap &&
+   : >.git/objects/pack/fake5.bitmap &&
+   : >.git/objects/pack/fake5.idx &&
+   : >.git/objects/pack/fake6.keep &&
+   : >.git/objects/pack/fake6.bitmap &&
+   : >.git/objects/pack/fake6.idx &&
git count-objects -v 2>stderr &&
grep "index file .git/objects/pack/fake.idx is too small" stderr &&
grep "^warning:" stderr | sort >actual &&
@@ -238,14 +244,20 @@ warning: garbage found: .git/objects/pack/fake.bar
 warning: garbage found: .git/objects/pack/foo
 warning: garbage found: .git/objects/pack/foo.bar
 warning: no corresponding .idx or .pack: .git/objects/pack/fake2.keep
+warning: no corresponding .idx or .pack: .git/objects/pack/fake4.bitmap
 warning: no corresponding .idx: .git/objects/pack/foo.keep
 warning: no corresponding .idx: .git/objects/pack/foo.pack
 warning: no corresponding .pack: .git/objects/pack/fake3.idx
+warning: no corresponding .pack: .git/objects/pack/fake5.bitmap
+warning: no corresponding .pack: .git/objects/pack/fake5.idx
+warning: no corresponding .pack: .git/objects/pack/fake6.bitmap
+warning: no corresponding .pack: .git/objects/pack/fake6.idx
+warning: no corresponding .pack: .git/objects/pack/fake6.keep
 EOF
test_cmp expected actual
 '
 
-test_expect_success 'clean pack garbage with gc' '
+test_expect_failure 'clean pack garbage with gc' '
test_when_finished "rm -f .git/objects/pack/fake*" &&
test_when_finished "rm -f .git/objects/pack/foo*" &&
: >.git/objects/pack/foo.keep &&
@@ -254,15 +266,21 @@ test_expect_success 'clean pack garbage with gc' '
: >.git/objects/pack/fake2.keep &&
: >.git/objects/pack/fake2.idx &&
: >.git/objects/pack/fake3.keep &&
+   : >.git/objects/pack/fake4.bitmap &&
+   : >.git/objects/pack/fake5.bitmap &&
+   : >.git/objects/pack/fake5.idx &&
+   : >.git/objects/pack/fake6.keep &&
+   : >.git/objects/pack/fake6.bitmap &&
+   : >.git/objects/pack/fake6.idx &&
git gc &&
git count-objects -v 2>stderr &&
grep "^warning:" stderr | sort >actual &&
cat >expected <<\EOF &&
+warning: no corresponding .idx or .pack: .git/objects/pack/fake2.keep
 warning: no corresponding .idx or .pack: .git/objects/pack/fake3.keep
+warning: no corresponding .idx or .pack: .git/objects/pack/fake6.keep
 warning: no corresponding .idx: .git/objects/pack/foo.keep
 warning: no corresponding .idx: .git/objects/pack/foo.pack
-warning: no corresponding .pack: .git/objects/pack/fake2.idx
-warning: no corresponding .pack: .git/objects/pack/fake2.keep
 EOF
test_cmp expected actual
 '
-- 
2.6.1

--
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 3/3] gc: Clean garbage .bitmap files from pack dir

[Doug Kelly]

Similar to cleaning up excess .idx files, clean any garbage .bitmap
files that are not otherwise associated with any .idx/.pack files.

Signed-off-by: Doug Kelly 
---
 builtin/gc.c | 12 ++--
 t/t5304-prune.sh |  2 +-
 2 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index c583aad..7ddf071 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -58,8 +58,16 @@ static void clean_pack_garbage(void)
 
 static void report_pack_garbage(unsigned seen_bits, const char *path)
 {
-   if (seen_bits == PACKDIR_FILE_IDX)
-   string_list_append(_garbage, path);
+   if (seen_bits & PACKDIR_FILE_IDX ||
+   seen_bits & PACKDIR_FILE_BITMAP) {
+   const char *dot = strrchr(path, '.');
+   if (dot) {
+   int baselen = dot - path + 1;
+   if (!strcmp(path+baselen, "idx") ||
+   !strcmp(path+baselen, "bitmap"))
+   string_list_append(_garbage, path);
+   }
+   }
 }
 
 static void git_config_date_string(const char *key, const char **output)
diff --git a/t/t5304-prune.sh b/t/t5304-prune.sh
index 4fa6e7a..9f9f263 100755
--- a/t/t5304-prune.sh
+++ b/t/t5304-prune.sh
@@ -257,7 +257,7 @@ EOF
test_cmp expected actual
 '
 
-test_expect_failure 'clean pack garbage with gc' '
+test_expect_success 'clean pack garbage with gc' '
test_when_finished "rm -f .git/objects/pack/fake*" &&
test_when_finished "rm -f .git/objects/pack/foo*" &&
: >.git/objects/pack/foo.keep &&
-- 
2.6.1

--
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 v3 0/3] prepare_packed_git(): find more garbage

[Doug Kelly]

Corrects the issues found in review by Peff, including simplifying
the logic in report_helper().  bits_to_msg() would've been an alternate
solution to that change, however it'll get called by
real_report_garbage(), so there's no need to call it twice, especially
when the check we need within report_helper().

I think checking for seen_bits == 0 would be future-proofing should we
arrive at a file bit not otherwise match it (i.e. file.foo and
file.bar, but nothing else would cause seen_bits to be zero, but if
that's the case, we wouldn't have PACKDIR_FILE_IDX or
PACKDIR_FILE_PACK set, either, and the second half would also match.

Doug Kelly (3):
  prepare_packed_git(): find more garbage
  t5304: Add test for .bitmap garbage files
  gc: Clean garbage .bitmap files from pack dir

 builtin/count-objects.c | 16 ++--
 builtin/gc.c| 12 ++--
 cache.h |  4 +++-
 sha1_file.c | 12 +---
 t/t5304-prune.sh| 20 
 5 files changed, 48 insertions(+), 16 deletions(-)

-- 
2.6.1

--
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 1/3] prepare_packed_git(): find more garbage

[Doug Kelly]

.bitmap and .keep files without .idx/.pack don't make much sense, so
make sure these are reported as garbage as well.  At the same time,
refactoring report_garbage to handle extra bits.

Signed-off-by: Doug Kelly 
---
 builtin/count-objects.c | 16 ++--
 cache.h |  4 +++-
 sha1_file.c | 12 +---
 t/t5304-prune.sh|  2 ++
 4 files changed, 20 insertions(+), 14 deletions(-)

diff --git a/builtin/count-objects.c b/builtin/count-objects.c
index ba92919..ed103ae 100644
--- a/builtin/count-objects.c
+++ b/builtin/count-objects.c
@@ -17,19 +17,15 @@ static off_t loose_size;
 
 static const char *bits_to_msg(unsigned seen_bits)
 {
-   switch (seen_bits) {
-   case 0:
-   return "no corresponding .idx or .pack";
-   case PACKDIR_FILE_GARBAGE:
+   if (seen_bits & PACKDIR_FILE_GARBAGE)
return "garbage found";
-   case PACKDIR_FILE_PACK:
+   else if (seen_bits & PACKDIR_FILE_PACK && !(seen_bits & 
PACKDIR_FILE_IDX))
return "no corresponding .idx";
-   case PACKDIR_FILE_IDX:
+   else if (seen_bits & PACKDIR_FILE_IDX && !(seen_bits & 
PACKDIR_FILE_PACK))
return "no corresponding .pack";
-   case PACKDIR_FILE_PACK|PACKDIR_FILE_IDX:
-   default:
-   return NULL;
-   }
+   else if (!(seen_bits & (PACKDIR_FILE_IDX|PACKDIR_FILE_PACK)))
+   return "no corresponding .idx or .pack";
+   return NULL;
 }
 
 static void real_report_garbage(unsigned seen_bits, const char *path)
diff --git a/cache.h b/cache.h
index 736abc0..5b9d791 100644
--- a/cache.h
+++ b/cache.h
@@ -1292,7 +1292,9 @@ extern struct packed_git *parse_pack_index(unsigned char 
*sha1, const char *idx_
 /* A hook to report invalid files in pack directory */
 #define PACKDIR_FILE_PACK 1
 #define PACKDIR_FILE_IDX 2
-#define PACKDIR_FILE_GARBAGE 4
+#define PACKDIR_FILE_BITMAP 4
+#define PACKDIR_FILE_KEEP 8
+#define PACKDIR_FILE_GARBAGE 16
 extern void (*report_garbage)(unsigned seen_bits, const char *path);
 
 extern void prepare_packed_git(void);
diff --git a/sha1_file.c b/sha1_file.c
index 3d56746..3524274 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1222,7 +1222,9 @@ void (*report_garbage)(unsigned seen_bits, const char 
*path);
 static void report_helper(const struct string_list *list,
  int seen_bits, int first, int last)
 {
-   if (seen_bits == (PACKDIR_FILE_PACK|PACKDIR_FILE_IDX))
+   static const int pack_and_index = PACKDIR_FILE_PACK|PACKDIR_FILE_IDX;
+
+   if ((seen_bits & pack_and_index) == pack_and_index)
return;
 
for (; first < last; first++)
@@ -1256,9 +1258,13 @@ static void report_pack_garbage(struct string_list *list)
first = i;
}
if (!strcmp(path + baselen, "pack"))
-   seen_bits |= 1;
+   seen_bits |= PACKDIR_FILE_PACK;
else if (!strcmp(path + baselen, "idx"))
-   seen_bits |= 2;
+   seen_bits |= PACKDIR_FILE_IDX;
+   else if (!strcmp(path + baselen, "bitmap"))
+   seen_bits |= PACKDIR_FILE_BITMAP;
+   else if (!strcmp(path + baselen, "keep"))
+   seen_bits |= PACKDIR_FILE_KEEP;
}
report_helper(list, seen_bits, first, list->nr);
 }
diff --git a/t/t5304-prune.sh b/t/t5304-prune.sh
index def203c..1ea8279 100755
--- a/t/t5304-prune.sh
+++ b/t/t5304-prune.sh
@@ -261,6 +261,8 @@ test_expect_success 'clean pack garbage with gc' '
 warning: no corresponding .idx or .pack: .git/objects/pack/fake3.keep
 warning: no corresponding .idx: .git/objects/pack/foo.keep
 warning: no corresponding .idx: .git/objects/pack/foo.pack
+warning: no corresponding .pack: .git/objects/pack/fake2.idx
+warning: no corresponding .pack: .git/objects/pack/fake2.keep
 EOF
test_cmp expected actual
 '
-- 
2.6.1

--
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 3/3] gc: Clean garbage .bitmap files from pack dir

[Jeff King]

On Fri, Dec 18, 2015 at 06:06:40PM -0600, Doug Kelly wrote:

> Similar to cleaning up excess .idx files, clean any garbage .bitmap
> files that are not otherwise associated with any .idx/.pack files.
> 
> Signed-off-by: Doug Kelly 
> ---
>  builtin/gc.c | 12 ++--
>  t/t5304-prune.sh |  2 +-
>  2 files changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/builtin/gc.c b/builtin/gc.c
> index c583aad..7ddf071 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -58,8 +58,16 @@ static void clean_pack_garbage(void)
>  
>  static void report_pack_garbage(unsigned seen_bits, const char *path)
>  {
> - if (seen_bits == PACKDIR_FILE_IDX)
> - string_list_append(_garbage, path);
> + if (seen_bits & PACKDIR_FILE_IDX ||
> + seen_bits & PACKDIR_FILE_BITMAP) {
> + const char *dot = strrchr(path, '.');
> + if (dot) {
> + int baselen = dot - path + 1;
> + if (!strcmp(path+baselen, "idx") ||
> + !strcmp(path+baselen, "bitmap"))
> + string_list_append(_garbage, path);
> + }
> + }
>  }

Hmm. Thinking on this further, do we actually need to check seen_bits
here at all?

The original was trying to ask "is this a .idx file" by checking
seen_bits.  That was actually broken by the first patch in this series
for some cases, as we might send more bits. E.g., if you have "foo.idx"
and "foo.pack", this function will get called twice (once per file), but
with seen_bits set to IDX|BITMAP for both cases. And we would not match
the "==" above, and would therefore fail to trigger.

That case is re-fixed by this patch, which is good. But I think
seen_bits is not really telling us anything at this point. We know it's
a garbage case, or else report_helper wouldn't have passed it along to
us. But we care only about the extension in the path, which is what
distinguishes each individual call to this function.

So we can just check that.  I also think the logic may be clearer if we
handle each extension exhaustively, like:

  /* We know these are useless without the matching .pack */
  if (ends_with(path, ".bitmap") || ends_with(path, ".idx")) {
  string_list_append(_garbage, path);
  return;
  }

  /*
   * A pack without other files cannot be used, but should be saved,
   * as this is a recoverable situation (we may even see it racily
   * as new packs come into existence).
   */
  if (ends_with(path, ".pack"))
  return;

  /*
   * A .keep file is useless without the matching pack, but it
   * _could_ contain information generated by the user. Let's keep it.
   * In the future, we may expand this to look for obvious leftover
   * receive-pack locks and drop them.
   */
  if (ends_with(path, ".keep"))
  return;

  /*
   * A totally unrelated garbage file should be kept, to err
   * on the conservative side.
   */
  if (seen_bits & PACKDIR_FILE_GARBAGE)
return;

  /*
   * We have a file type that the garbage-reporting functions
   * know about but we don't. This function needs updating.
   */
  die("BUG: report_pack_garbage confused");

> -test_expect_failure 'clean pack garbage with gc' '
> +test_expect_success 'clean pack garbage with gc' '
>   test_when_finished "rm -f .git/objects/pack/fake*" &&
>   test_when_finished "rm -f .git/objects/pack/foo*" &&
>   : >.git/objects/pack/foo.keep &&

And I think here we should make sure that we are covering the above
situations (and especially that we are keeping files that should be
kept).

-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: [RFC] Malicously tampering git metadata?

[Theodore Ts'o]

On Tue, Dec 15, 2015 at 10:26:39PM -0500, Santiago Torres wrote:
> 4) The developer pushes to upstream. All the traffic can be re-routed
> back to the original repository. The target branch now contains a
> vulnerable piece of code.

I assume you are assuming here that the "push to upstream" doesn't
involve some kind of human verification?  If someone tried pushing
something like this to Linus, he would be checking the git diff stats
and git commit structure for things that might look like "developer
negligence".  He's been known to complain to subsystem developers in
his own inimitable when the git commit structure looks suspicious, so
I'm pretty sure he would notice this.

But normally that developnment process we don't talk about "pushing to
upstream" as much as "requesting a pull".  So it would be useful when
you describe the attack to explicit describe the development workflow
that is vulnerable to your attack.

For example, in my use case, I'm authorative over changes in fs/ext4.
So when I pull from Linus's repo, I examine (using "gitk fs/ext4") all
commits coming from upstream that modify fs/ext4.  So if someone tries
introducing a change in fs/ext4 coming from "upstream", I will notice.
Then when I request a pull request from Linus, the git pull request
describes what commits are new in my tree that are not in his, and
shows the diffstats from upstream.  When Linus verifies my pull, there
are multiple opportunities where he will notice funny business:

a) He would notice that my origin commit is something that is not in
his upstream tree.

b) His diffstat is different from my diffstat (since thanks to the
man-in-the middle, the conception of what commits are new in the git
pull request will be different from his).

c) His diffstat will show that files outside of fs/ext4 have been
modified, which is a red flag that will merit more close examination.
(And if the attacker had tried to introduce a change in fs/ext4, I
would have noticed when I pulled from the man-in-the-middle git
repo.)

Now, if there is zero checking when the user pushes to upstream, then
yes, there are all sorts of potential problems.  But that's one of the
reasons why it's generally considered a good thing for Linux
developers to use as the origin commit for their work official
releases (which can be demarked using GPG-signed git tags).

So for example, the changes for ext4 that were sent to Linus for v4.4
was based off of v4.3-rc2:

git tag  --verify v4.3-rc2
object 1f93e4a96c9109378204c147b3eec0d0e8100fde
type commit
tag v4.3-rc2
tagger Linus Torvalds  1442784761 -0700

Linux 4.3-rc2
gpg: Signature made Sun 20 Sep 2015 05:32:41 PM EDT using RSA key ID 00411886
gpg: Good signature from "Linus Torvalds " [full]


And the changes which I sent to Linus were also signed by a tag, and
better yet, someone can indepedently verify this after the fact:

% git show --oneline --show-signature f41683a204ea61568f0fd0804d47c19561f2ee39
f41683a merged tag 'ext4_for_linus_stable'
gpg: Signature made Sun 06 Dec 2015 10:35:27 PM EST using RSA key ID 950D81A3
gpg: Good signature from "Theodore Ts'o " [ultimate]
gpg: aka "Theodore Ts'o " [ultimate]
gpg: aka "Theodore Ts'o " [ultimate]

They can also verify that the chain of commits that I sent to Linus
were rooted in Linus's signed v4.3-rc2 tag, so this kind of
after-the-fact auditing means that if there *were* funny business, it
could be caught even if Linus slipped up in his checking.


Now, the crazy behavior where github users randomly and promiscuously
do pushes and pull without doing any kind of verification may very
well be dangerous.  But so is someone who saves a 80 patch series from
their inbox, and without reading or verifying all of the patches
applies them blindly to their tree using "git am" --- or if they were
using cvs or svn, bulk applied the patches without doing any
verification

Cheers,

- Ted
--
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 7/8] config: add core.untrackedCache

[Christian Couder]

On Thu, Dec 17, 2015 at 1:36 PM, Duy Nguyen  wrote:
> On Wed, Dec 16, 2015 at 4:53 AM, Ævar Arnfjörð Bjarmason
>  wrote:
>> On Tue, Dec 15, 2015 at 8:40 PM, Junio C Hamano  wrote:
>>> Ævar Arnfjörð Bjarmason  writes:
>>> I still have a problem with the approach from "design cleanliness"
>>> point of view[...]
>>>
>>> In any case I think we already have agreed to disagree on this
>>> point, so there is no use discussing it any longer from my side.  I
>>> am not closing the door to this series, but I am not convinced,
>>> either.  At least not yet.
>>
>> In general the fantastic thing about the git configuration facility is
>> that it provides both systems administrators and normal users with
>> what they want. It's possible to configure things system-wide and
>> override those on a user or repository basis.
>>
>> Of course hindsight is 20/20, but I think that given what's been
>> covered in this thread it's been established that it's categorically
>> better that if we introduce features like these that they be
>> configured through the normal configuration facility rather than the
>> configuration being sticky to the index.
>
> A minor note for implementers. We need to check that config is loaded
> first. read-cache.c, being part of the core, does not bother itself
> with config loading. And I think so far it has not used any config
> vars. If a command forgets (*) to load the config, the cache may be
> deleted (if we do it the safe way).
>
> (*) is there any command deliberately avoid loading config? git-clone
> and git-init are special, but for this particular case it's probably
> fine.

Thanks for this note.

Looking at the current patch, the global variable in which the value
of the core.untrackedCache config var is stored is
"use_untracked_cache".
It is used in the following places:

- wt_status_collect_untracked() in wt-status.c which is called only by
"git status" and "git commit" after the config has been loaded.

- cmd_update_index() in builtin/update-index.c which loads the config
before using it

- validate_untracked_cache() in dir.c where it is used in:

   if (use_untracked_cache != 1 && !ident_in_untracked(dir->untracked)) {
warning(_("Untracked cache is disabled on this system."));
return NULL;
}

but this "if" and its contents are removed by patch 10/10 in v2.

So at the end of this patch series, there is no risk of
use_untracked_cache not being properly setup.
--
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

Forcing git to pack objects

[Edmundo Carmona Antoranz]

Hi!

Recently I was running manually a git gc --prune command (wanted to
shrink my 2.8G .git directory by getting rid of loose objects) and I
ended up running out of space on my HD. After freaking out a little
bit (didn't know if the repo would end up in a 'stable' state), I
ended up freeing up some space and I again have a working repo...
_but_ I noticed that basically _all_ objects on my repo are laying
around in directories .git/objects/00 to ff (and taking a whole lot of
space... like the .git directory is now like 5 GBs). After running git
gc manually again it ended up taking a lot of time and the objects are
still there. Also git svn sometimes gcs after fetching and it took to
run cause of the gc execution (ended up killing it) and the files are
still there. Is it possible to ask git to put all those objects in
.pack files? Or did I mess something on my repo?

Just in case, that's a repo I use at work that's working on a windows
box (git for windows 2.6.3).

Thanks in advance.
--
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] do_compare_entry: use already-computed path

[David Turner]

In traverse_trees, we generate the complete traverse path for a
traverse_info.  Later, in do_compare_entry, we used to go do a bunch
of work to compare the traverse_info to a cache_entry's name without
computing that path.  But since we already have that path, we don't
need to do all that work.  Instead, we can just stuff the generated
path into the traverse_info, and do the comparison more directly.
This makes git checkout much faster -- about 25% on Twitter's
monorepo.  Deeper directory trees are likely to benefit more than
shallower ones.

Signed-off-by: David Turner 
---
 tree-walk.c|  4 
 tree-walk.h|  1 +
 unpack-trees.c | 41 +++--
 3 files changed, 44 insertions(+), 2 deletions(-)

diff --git a/tree-walk.c b/tree-walk.c
index 6dccd2d..4cab3e1 100644
--- a/tree-walk.c
+++ b/tree-walk.c
@@ -329,6 +329,9 @@ int traverse_trees(int n, struct tree_desc *t, struct 
traverse_info *info)
make_traverse_path(base.buf, info->prev, >name);
base.buf[info->pathlen-1] = '/';
strbuf_setlen(, info->pathlen);
+   info->traverse_path = base.buf;
+   } else {
+   info->traverse_path = info->name.path;
}
for (;;) {
int trees_used;
@@ -411,6 +414,7 @@ int traverse_trees(int n, struct tree_desc *t, struct 
traverse_info *info)
for (i = 0; i < n; i++)
free_extended_entry(tx + i);
free(tx);
+   info->traverse_path = NULL;
strbuf_release();
return error;
 }
diff --git a/tree-walk.h b/tree-walk.h
index 3b2f7bf..174eb61 100644
--- a/tree-walk.h
+++ b/tree-walk.h
@@ -59,6 +59,7 @@ enum follow_symlinks_result {
 enum follow_symlinks_result get_tree_entry_follow_symlinks(unsigned char 
*tree_sha1, const char *name, unsigned char *result, struct strbuf 
*result_path, unsigned *mode);
 
 struct traverse_info {
+   const char *traverse_path;
struct traverse_info *prev;
struct name_entry name;
int pathlen;
diff --git a/unpack-trees.c b/unpack-trees.c
index 8e2032f..127dd4d 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -498,13 +498,14 @@ static int traverse_trees_recursive(int n, unsigned long 
dirmask,
  * itself - the caller needs to do the final check for the cache
  * entry having more data at the end!
  */
-static int do_compare_entry(const struct cache_entry *ce, const struct 
traverse_info *info, const struct name_entry *n)
+static int do_compare_entry_piecewise(const struct cache_entry *ce, const 
struct traverse_info *info, const struct name_entry *n)
 {
int len, pathlen, ce_len;
const char *ce_name;
 
if (info->prev) {
-   int cmp = do_compare_entry(ce, info->prev, >name);
+   int cmp = do_compare_entry_piecewise(ce, info->prev,
+>name);
if (cmp)
return cmp;
}
@@ -522,6 +523,42 @@ static int do_compare_entry(const struct cache_entry *ce, 
const struct traverse_
return df_name_compare(ce_name, ce_len, S_IFREG, n->path, len, n->mode);
 }
 
+static int do_compare_entry(const struct cache_entry *ce,
+   const struct traverse_info *info,
+   const struct name_entry *n)
+{
+   int len, pathlen, ce_len;
+   const char *ce_name;
+   int cmp;
+
+   /*
+* If we have not precomputed the traverse path, it is quicker
+* to avoid doing so.  But if we have precomputed it,
+* it is quicker to use the precomputed version.
+*/
+   if (!info->traverse_path)
+   return do_compare_entry_piecewise(ce, info, n);
+
+   cmp = strncmp(ce->name, info->traverse_path, info->pathlen);
+   if (cmp)
+   return cmp;
+
+   pathlen = info->pathlen;
+   ce_len = ce_namelen(ce);
+
+   if (ce_len < pathlen) {
+   if (do_compare_entry_piecewise(ce, info, n) >= 0)
+   die("pathlen");
+   return -1;
+   }
+
+   ce_len -= pathlen;
+   ce_name = ce->name + pathlen;
+
+   len = tree_entry_len(n);
+   return df_name_compare(ce_name, ce_len, S_IFREG, n->path, len, n->mode);
+}
+
 static int compare_entry(const struct cache_entry *ce, const struct 
traverse_info *info, const struct name_entry *n)
 {
int cmp = do_compare_entry(ce, info, n);
-- 
2.4.2.749.g730654d-twtrsrc

--
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 0/3] prepare_packed_git(): find more garbage

[Jeff King]

On Fri, Dec 18, 2015 at 06:06:37PM -0600, Doug Kelly wrote:

> Corrects the issues found in review by Peff, including simplifying
> the logic in report_helper().  bits_to_msg() would've been an alternate
> solution to that change, however it'll get called by
> real_report_garbage(), so there's no need to call it twice, especially
> when the check we need within report_helper().

OK. The new logic in 1/3 looks fine to me.

> I think checking for seen_bits == 0 would be future-proofing should we
> arrive at a file bit not otherwise match it (i.e. file.foo and
> file.bar, but nothing else would cause seen_bits to be zero, but if
> that's the case, we wouldn't have PACKDIR_FILE_IDX or
> PACKDIR_FILE_PACK set, either, and the second half would also match.

Yeah, I think this is sound.

I left a few comments on 3/3. I don't think it's _wrong_, but I think we
can be a bit more thorough (and IMHO, a little more maintainable, but
others might disagree).

-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 0/3] prepare_packed_git(): find more garbage

[Jeff King]

On Fri, Dec 18, 2015 at 09:02:47PM -0500, Jeff King wrote:

> I left a few comments on 3/3. I don't think it's _wrong_, but I think we
> can be a bit more thorough (and IMHO, a little more maintainable, but
> others might disagree).

Oh, and I forgot to say thank you for working on this. :)

-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

Fwd: [PATCH 7/8] config: add core.untrackedCache

[Christian Couder]

(Sorry I sent this one privately to Duy by mistake too.)


-- Forwarded message --
From: Christian Couder 
Date: Fri, Dec 18, 2015 at 11:35 PM
Subject: Re: [PATCH 7/8] config: add core.untrackedCache
To: Duy Nguyen 


On Thu, Dec 17, 2015 at 1:26 PM, Duy Nguyen  wrote:
> On Thu, Dec 17, 2015 at 2:44 PM, Jeff King  wrote:
>> I think we may actually be thinking of the same thing. Naively, I would
>> expect:
>>
>> ..
>>   - if there is cache data in the index but that config flag is not set,
>> presumably we would not update it (we could even explicitly drop it,
>> but my understanding is that is not necessary for correctness, but
>> only as a possible optimization).
>
> No, if somebody adds or removes something from the index, we either
> update or drop it, or it's stale. There's the invalidate_untracked()
> or something in dir.c that we can hook in, check config var and do
> that. And because config is cached recently, it should be a cheap
> operation.

I think I understand what you are saying but it took me a long time,
and I am not sure it is very clear to others.

What I understand is that you are talking about
validate_untracked_cache() in dir.c.
And you suggest to check there if the core.untrackedCache config var
is set, and, if it is not set, then to drop the UC there.
And the reason for that is that git operations on other parts of the
index should update the UC if it exists otherwise the UC content could
be wrong as you explained previously in your "git rm --cached foo"
example.

In the current patch, the code to create or remove the UC to reflect
the state of the core.untrackedCache config var is in
wt_status_collect_untracked().
I think it works well there, but if you are saying that it's better if
it is in validate_untracked_cache(), I am willing to consider moving
it to validate_untracked_cache().
Could you tell a bit though about why you think it's better in
validate_untracked_cache()?

> Apart from that the idea is fine.

Ok so, except maybe the part about wt_status_collect_untracked() vs
validate_untracked_cache(), what the patch does is ok for you?
--
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: Odd rebase behavior

[Johannes Sixt]

Am 18.12.2015 um 19:05 schrieb John Keeping:

On Fri, Dec 18, 2015 at 11:43:16AM -0600, David A. Greene wrote:

John Keeping  writes:


It seems that the problem is introduces by --preserve-merges (and
-Xsubtree causes something interesting to happen as well).  I see the
following behaviour:


Thanks for narrowing this down!  Is it possible this is actually a
cherry-pick problem since --preserve-merges forces rebase to use
cherry-pick?


I'm pretty sure this a result of the code in git-rebase--interactive.sh
just below the comment "Watch for commits that have been dropped by
cherry-pick", which filters out certain commits.  However, I'm not at
all familiar with the --preserve-merges code in git-rebase so I could be
completely wrong.


git rebase -Xsubtree=files_subtree --onto files-master master

fatal: Could not parse object 
'b15c4133fc3146e1330c84159886f0f7a09fbf43^'
Unknown exit code (128) from command: git-merge-recursive
b15c4133fc3146e1330c84159886f0f7a09fbf43^ -- HEAD
b15c4133fc3146e1330c84159886f0f7a09fbf43


Ah, good!  I had seen this behavior as well but couldn't remember what I
did to trigger it.

I don't think I have the expertise to fix rebase and/or cherry-pick.
What's the process for adding these tests to the testbase and marking
them so the appropriate person can fix them?  I see a lot of TODO tests.
Should I mark these similarly and propose a patch to the testbase?


I think marking them with test_expect_failure (instead of
test_expect_success) is enough.



There are a few known breakages recorded in 
t3512-cherry-pick-submodule.sh. Perhaps the one observed here is already 
among them?


-- Hannes

--
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

Fwd: [PATCH 7/8] config: add core.untrackedCache

[Christian Couder]

Sorry I sent this privately to Peff by mistake (once again).

-- Forwarded message --
From: Christian Couder 
Date: Fri, Dec 18, 2015 at 11:09 PM
Subject: Re: [PATCH 7/8] config: add core.untrackedCache
To: Jeff King 


On Thu, Dec 17, 2015 at 8:44 AM, Jeff King  wrote:
> On Tue, Dec 15, 2015 at 10:05:57PM -0800, Junio C Hamano wrote:
>>
>> To put it another way, the "bit" in the index (i.e. the presence of
>> the cached data) is "Am I using the feature now?".  The effect of
>> the feature has to (and is designed to) persist, as it is a cache
>> and you do not want a stale cache to give you wrong answers.

Sorry if I repeat myself or misunderstood something, but in what I
implemented we either use the UC fully or we remove it, so it cannot
be stale vs git operations (like other changes in the index Duy talks
about).

And as we are caching directory mtimes and as we are comparing each
cached mtime with the current mtime of the original directory before
trusting the cached content related to the directory, a UC that is
stale vs working tree operations could result in time lost but not in
bad cached content used.

Or maybe if we are very unlucky and have two directories with exactly
the same mtime and the same name but not the same content, and if we
move away the directory the UC was made from, and then put the other
one at the path where the first one was. Yeah in this case you may get
bad results because bad UC content is used. But note that this case
can already happen now. It is a case that is inherent in using the UC.

>> There
>> is no "Do I want to use the feature?" preference, in other words.
>>
>> And I do not mind creating such a preference bit as a configuration.
>>
>> That is why I suggested such a configuration to cause the equivalent
>> of "update-index --untracked-cache" when a new index is created from
>> scratch (as opposed to the case where the previously created cache
>> data is carried forward across read_cache() -> do things to the
>> index -> write_cache() flow).  Doing it that way will not have to
>> involve additional complexity that comes from the desire that
>> setting a single configuration on (or off) has to suddenly change
>> the behaviour of an index file that is already using (or not using)
>> the feature.
>
> I think we may actually be thinking of the same thing. Naively, I would
> expect:
>
>   - if there is untracked cache data in the index, we will make use of
> it when enumerating untracked files (and my understanding is that if
> it is stale, we can detect that)

I agree for "git status", but I am not sure that the UC is used in all
the code paths that enumerate untracked files.
I recall Duy mentioning that it was not used by "git add" and in some
other cases, but I might be wrong.

I also agree that we can detect when the UC content should not be used
because it is stale vs working tree operations (see above).

Now as Duy says the UC should never be stale vs git operations, but
that is easy to do if we just remove the UC when we stop using it.

>   - if core.untrackedCache is set, we will update and write out an
> untracked cache when we are enumerating the untracked files

In the current patch this happens when "git status" is called,
actually in wt_status_collect_untracked(), again I am not sure about
all the other code paths.

>   - if there is cache data in the index but that config flag is not set,
> presumably we would not update it (we could even explicitly drop it,
> but my understanding is that is not necessary for correctness, but
> only as a possible optimization).

In the current patch we drop the UC, also in
wt_status_collect_untracked(), if the config flag is not set (or set
to false).

It is necessary to drop it for correctness for the following reasons:

- git operations on (other parts of) the index must update the UC if
it exists according to Duy who gave the following example in a
previous email:

"... if file "foo" is tracked, then the user does "git rm --cached
foo", "foo" may become either untracked or ignored. So if you disable
UC while doing this removal, then re-enable UC again, "git-status" may
show incorrect output."

- the user may have decided to unset the config flag because the mtime
features we rely on are in fact not well supported by the system.
(Though it's true that the user should have used "git update-index
--test-untracked-cache" before setting the config flag in the first
place, but maybe it was a mistake, or maybe the file system will be
accessed for some time by another system that will mount it or
something.)

> You could have a config option for "if there is a cache there, pretend
> it isn't and ignore it", but I don't see much point.

There is not much point indeed in such an option and it could be dangerous.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org

Re: [PATCH v2] mingw: emulate write(2) that fails with a EPIPE

[Johannes Sixt]

Am 17.12.2015 um 18:08 schrieb Johannes Schindelin:

On Windows, when writing to a pipe fails, errno is always
EINVAL. However, Git expects it to be EPIPE.

According to the documentation, there are two cases in which write()
triggers EINVAL: the buffer is NULL, or the length is odd but the mode
is 16-bit Unicode (the broken pipe is not mentioned as possible cause).
Git never sets the file mode to anything but binary, therefore we know
that errno should actually be EPIPE if it is EINVAL and the buffer is
not NULL.

See https://msdn.microsoft.com/en-us/library/1570wh78.aspx for more
details.

This works around t5571.11 failing with v2.6.4 on Windows.

Signed-off-by: Johannes Schindelin 
---
  compat/mingw.c | 17 +
  compat/mingw.h |  3 +++
  2 files changed, 20 insertions(+)

diff --git a/compat/mingw.c b/compat/mingw.c
index 90bdb1e..5edea29 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -394,6 +394,23 @@ int mingw_fflush(FILE *stream)
return ret;
  }

+#undef write
+ssize_t mingw_write(int fd, const void *buf, size_t len)
+{
+   ssize_t result = write(fd, buf, len);
+
+   if (result < 0 && errno == EINVAL && buf) {
+   /* check if fd is a pipe */
+   HANDLE h = (HANDLE) _get_osfhandle(fd);
+   if (GetFileType(h) == FILE_TYPE_PIPE)
+   errno = EPIPE;
+   else
+   errno = EINVAL;
+   }
+
+   return result;
+}
+
  int mingw_access(const char *filename, int mode)
  {
wchar_t wfilename[MAX_PATH];


Looks good. I tested the patch, and it fixes the failure exposed by 
t5571.11.


Acked-by: Johannes Sixt 

Thanks!

-- Hannes

--
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 1/2] gitk: match ttk fonts to gitk fonts

[Paul Mackerras]

On Tue, Dec 08, 2015 at 08:05:50AM +0100, Giuseppe Bilotta wrote:
> The fonts set in setoptions aren't consistently picked up by ttk, who
> uses its own predefined fonts. This is noticeable when switching
> between using and not using ttk with custom fonts or in HiDPI settings
> (where the default TTK fonts do _not_ respect tk sclaing).
> 
> Fix by mapping the ttk fontset to the one used by gitk internally.
> 
> Signed-off-by: Giuseppe Bilotta 

Thanks, applied both this and the following patch.

Paul.
--
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] gitk: add -C commandline parameter to change path

[Paul Mackerras]

On Mon, Nov 09, 2015 at 01:45:22PM +0200, Juha-Pekka Heikkila wrote:
> This patch adds -C (change working directory) parameter to
> gitk. With this parameter, instead of need to cd to directory
> with .git folder, one can point the correct folder from
> commandline.
> 
> Signed-off-by: Juha-Pekka Heikkila 

Thanks.

While I like the idea, there are a couple of minor problems with the
patch.  First, the Documentation directory is in Junio's tree, not
mine, so the change to gitk and the change to Documentation need to be
separated.  Secondly, please use 4-space indentation rather than
8-space for consistency with the rest of the file.  See also the
comments below.

> + "-C*" {
> + if {[string length $arg] < 3} {
> + incr i
> + set cwd_path [lindex $argv [expr {$i}]]

No need to say [expr {$i}] here; [lindex $argv $i] works just fine.

Also, if i is now >= [llength $argv], we'll get an empty string in
cwd_path.  Is that what you meant?  Shouldn't we display an
appropriate error message instead of trying to cd to ""?

Paul.
--
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] gitk: sv.po: Update Swedish translation (311t)

[Paul Mackerras]

Thanks, applied.

Paul.


--
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 v2 03/11] ref-filter: introduce struct used_atom

[Karthik Nayak]

On Thu, Dec 17, 2015 at 2:27 AM, Eric Sunshine  wrote:
> On Wed, Dec 16, 2015 at 10:29 AM, Karthik Nayak  wrote:
>> Introduce the 'used_array' structure which would replace the existing
>> implementation of 'used_array' (which a list of atoms). This helps us
>> parse atom's before hand and store required details into the
>> 'used_array' for future usage.
>
> All my v1 review comments[1] about the commit message still apply to
> this version.
>
> [1]: http://article.gmane.org/gmane.comp.version-control.git/281860
>

I totally missed this out, thanks for bringing it up.

>> Also introduce a parsing function for each atom in valid_atom. Using
>> this we can define special parsing functions for each of the atoms.
>
> This is a conceptually distinct change which probably deserves its own
> patch. In particular, the new patch would add this field to
> valid_atom[] *and* add the code which invokes the custom parser. (That
> code is currently commingled with introduction of the color parser in
> patch 6/11.)
>go

I guess that could be done, I was thinking it goes together, but it
makes sense to have a
separate patch for introduction of the parsing function and its invocations.

> More below...
>
>> Signed-off-by: Karthik Nayak 
>> ---
>> diff --git a/ref-filter.c b/ref-filter.c
>> @@ -16,9 +16,27 @@
>> +/*
>> + * An atom is a valid field atom listed below, possibly prefixed with
>> + * a "*" to denote deref_tag().
>> + *
>> + * We parse given format string and sort specifiers, and make a list
>> + * of properties that we need to extract out of objects.  ref_array_item
>> + * structure will hold an array of values extracted that can be
>> + * indexed with the "atom number", which is an index into this
>> + * array.
>> + */
>> +static struct used_atom {
>> +   const char *str;
>> +   cmp_type type;
>> +} *used_atom;
>> +static int used_atom_cnt, need_tagged, need_symref;
>> +static int need_color_reset_at_eol;
>> +
>>  static struct {
>> const char *name;
>> cmp_type cmp_type;
>> +   void (*parser)(struct used_atom *atom);
>>  } valid_atom[] = {
>> { "refname" },
>> { "objecttype" },
>> @@ -786,7 +788,8 @@ static void populate_value(struct ref_array_item *ref)
>>
>> /* Fill in specials first */
>> for (i = 0; i < used_atom_cnt; i++) {
>> -   const char *name = used_atom[i];
>> +   struct used_atom *atom = _atom[i];
>> +   const char *name = atom->str;
>
> Same question as my previous review[1]: Why not just:
>
> const char *name = used_atom[i].str;
>
> ?

I think It's leftover code, I was using the atom variable also before.
I'll remove it.

-- 
Regards,
Karthik Nayak
--
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 v2 05/11] ref-filter: skip deref specifier in match_atom_name()

[Karthik Nayak]

On Thu, Dec 17, 2015 at 2:41 AM, Eric Sunshine  wrote:
> On Wed, Dec 16, 2015 at 10:29 AM, Karthik Nayak  wrote:
>> In upcoming patches we make calls to match_atom_name() with the '*'
>> deref specifier still attached to the atom name. This causes
>> undesirable errors, hence, if present skip over the '*' deref
>> specifier in the atom name.
>
> I'd drop the second sentence since it doesn't add much or any value.
> Instead, you might want to explain that skipping '*' is done as a
> convenience.
>
> Subsequent patches will call match_atom_name() with the '*' deref
> specifier still attached to the atom name so, as a convenience,
> skip over it on their behalf.
>

Thanks will put that in.

>> Signed-off-by: Karthik Nayak 
>> ---
>> diff --git a/ref-filter.c b/ref-filter.c
>> @@ -37,6 +37,10 @@ static int match_atom_name(const char *name, const char 
>> *atom_name, const char *
>>  {
>> const char *body;
>>
>> +   /*  skip the deref specifier*/
>
> Too many spaces before "skip".
> Too few spaces after "specifier".
>

Will fix.

-- 
Regards,
Karthik Nayak
--
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 v2 06/11] ref-filter: introduce color_atom_parser()

[Karthik Nayak]

On Thu, Dec 17, 2015 at 2:51 AM, Eric Sunshine  wrote:
> On Wed, Dec 16, 2015 at 10:29 AM, Karthik Nayak  wrote:
>> Introduce color_atom_parser() which will parse a "color" atom and
>> store its color in the "use_atom" structure for further usage in
>
> Same comment as last time: s/use_atom/used_atom/
>

Will change.

>> 'populate_value()'.
>
> s/'//g
>

Will do.

> More below...
>
>> Helped-by: Ramsay Jones 
>> Signed-off-by: Karthik Nayak 
>> ---
>> diff --git a/ref-filter.c b/ref-filter.c
>> @@ -29,6 +29,9 @@ typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } 
>> cmp_type;
>>  static struct used_atom {
>> const char *str;
>> cmp_type type;
>> +   union {
>> +   const char *color;
>> +   } u;
>>  } *used_atom;
>>  static int used_atom_cnt, need_tagged, need_symref;
>>  static int need_color_reset_at_eol;
>> @@ -53,6 +56,13 @@ static int match_atom_name(const char *name, const char 
>> *atom_name, const char *
>> +static void color_atom_parser(struct used_atom *atom)
>> +{
>> +   match_atom_name(atom->str, "color", >u.color);
>> +   if (!atom->u.color)
>> +   die(_("expected format: %%(color:)"));
>> +}
>> +
>> @@ -833,12 +846,10 @@ static void populate_value(struct ref_array_item *ref)
>> refname = branch_get_push(branch, NULL);
>> if (!refname)
>> continue;
>> -   } else if (match_atom_name(name, "color", )) {
>> +   } else if (starts_with(name, "color:")) {
>> char color[COLOR_MAXLEN] = "";
>>
>> -   if (!valp)
>> -   die(_("expected format: %%(color:)"));
>> -   if (color_parse(valp, color) < 0)
>> +   if (color_parse(atom->u.color, color) < 0)
>
> It would make a lot more sense to invoke color_parse() with the
> unchanging argument to "color:" just once in color_atom_parser()
> rather than doing it here repeatedly. (Also, you'd drop 'const' from
> used_atom.u.color declaration.)
>

This makes sense, I'll put have color_parse() in color_atom_parser().
This would also require us to allocate memory once in color_atom_parser.

>
> Does v->s get freed each time through the loop? If not, then, assuming
> you parse the color just once in color_atom_parser(), then you could
> just assign the parsed color directly to v->s rather than duplicating
> it.

No it doesn't. Yup will do.

-- 
Regards,
Karthik Nayak
--
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: [RFPR] updates for 2.7?

[Paul Mackerras]

On Tue, Dec 15, 2015 at 03:09:44PM -0800, Junio C Hamano wrote:
> Git 2.7-rc1 has just been tagged, and the remainder of the year will
> be for the stabilization, fixes to brown-paper-bag bugs, reverts of
> regressions, etc., but I haven't seen updates to the various
> subsystems you guys maintain for some time.  Please throw me "this
> is a good time to pull and here are the updates" message in the
> coming few weeks.

Please do a pull from git://ozlabs.org/~paulus/gitk.git to get:

* Some improvements to gitk appearance, particularly on high DPI monitors
* Translation updates for Japanese and Swedish.

Thanks,
Paul.
--
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: Help debugging git-svn

[Edmundo Carmona Antoranz]

On Fri, Dec 18, 2015 at 11:28 AM, Edmundo Carmona Antoranz
 wrote:
> Ok I came up with another idea to avoid having to deal with the
> old svn history (I'm having no problems fetching/dcommitting with my
> current repo). I already have the branches I work with, the thing is
> that the revisions I fetched before I started using the svn authors
> file have nasty IDs instead of human names. I though that I could
> create a script to rewrite the whole history of the project adjusting
> the usernames to real names (I'll take care of the details, no problem
> there... just found out about filter-branch, could work for what I'm
> thinking of). My question would be in the direction of rebuilding
> git-svn metainfo once I'm done so that I can continue fetching from
> svn as if nothing had happened. I checked the documentation in
> git-scm.com but didn't find the details about it. Is there a
> straight-forward document that explains how the git-svn metadata files
> are built?
>
> Thanks in advance.

.rev_map files appear to be simple enough. I'll have fun with them a
little bit. Will let you know how it goes later (don't hold your
breath it might take a while).
--
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