Re: [bug] Possible Windows 'git mv' bug

[Doug Kelly]

On Sun, Jan 31, 2016 at 8:50 AM, Johannes Sixt  wrote:
> Am 31.01.2016 um 15:03 schrieb Aaron Gray:
>>
>> Hi,
>>
>> I think I have found a possible difference in behaviour between
>> Windows git commandline distro and Linux git
>>
>> basically If I do a :-
>>
>>  git mv logger.h Logger.h
>>
>> I get the following :-
>>
>>  fatal: destination exists, source=lib/logger.h,
>> destination=lib/Logger.h
>>
>> It looks and smells like a bug to me !
>
>
> Not really. When you attempt to overwrite an existing file with 'git mv',
> you get this error message on both Windows and Linux.
>
> The difference is that logger.h and Logger.h are the same file on Windows,
> but they are not on Linux. Hence, when you attempt to overwrite Logger.h on
> Windows, you see the error because it exists already (as logger.h).
>
> As a work-around, you can use -f.
>
> -- Hannes

Indeed.  And just to clarify, you'll get the same issue on OS X, where
the filesystem is also case-preserving, not case-sensitive (by
default, at least).  I've never tried using -f for this, but I'll
usually use git mv twice to achieve the same result.  Annoying, but
that way my local directory looks correct, too.
--
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 <dougk@gmail.com>
---
 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 <dougk@gmail.com>
---
 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 <dougk@gmail.com>
---
 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

[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 <dougk@gmail.com>
---
 builtin/count-objects.c | 16 ++--
 cache.h |  4 +++-
 sha1_file.c | 17 +++--
 t/t5304-prune.sh|  2 ++
 4 files changed, 26 insertions(+), 13 deletions(-)

diff --git a/builtin/count-objects.c b/builtin/count-objects.c
index ba92919..5197b57 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 == 0 || !(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..5f939e4 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1225,6 +1225,15 @@ static void report_helper(const struct string_list *list,
if (seen_bits == (PACKDIR_FILE_PACK|PACKDIR_FILE_IDX))
return;
 
+   if (seen_bits == 
(PACKDIR_FILE_PACK|PACKDIR_FILE_IDX|PACKDIR_FILE_BITMAP))
+   return;
+
+   if (seen_bits == (PACKDIR_FILE_PACK|PACKDIR_FILE_IDX|PACKDIR_FILE_KEEP))
+   return;
+
+   if (seen_bits == 
(PACKDIR_FILE_PACK|PACKDIR_FILE_IDX|PACKDIR_FILE_BITMAP|PACKDIR_FILE_KEEP))
+   return;
+
for (; first < last; first++)
report_garbage(seen_bits, list->items[first].string);
 }
@@ -1256,9 +1265,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 1/3] prepare_packed_git(): find more garbage

[Doug Kelly]

Apparently, I fixed this and forgot to re-run format-patch, so I sent
out the same patch the second time... My fault on that one.  I've at
least checked what I sent this time around, and it seems to match
what's in my current tree. :) The second and third patches should be
unmodified.

Thanks for catching that, Stefan!

On Wed, Nov 25, 2015 at 12:43 PM, Stefan Beller <sbel...@google.com> wrote:
> On Fri, Nov 13, 2015 at 4:46 PM, Doug Kelly <dougk@gmail.com> wrote:
>> return "no corresponding .idx";
>> -   case PACKDIR_FILE_IDX:
>> +   else if (seen_bits & PACKDIR_FILE_IDX && seen_bits ^ 
>> ~PACKDIR_FILE_PACK)
>
> Did you intend to use
> (seen_bits & PACKDIR_FILE_IDX && !(seen_bits & PACKDIR_FILE_PACK))
> here?
>
> I was just looking at the state in peff/pu and it still has the xor
> variant, which exposes more
> than just the selected bit to the decision IIRC.
--
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 <dougk@gmail.com>
---
 builtin/count-objects.c | 16 ++--
 cache.h |  4 +++-
 sha1_file.c | 17 +++--
 t/t5304-prune.sh|  2 ++
 4 files changed, 26 insertions(+), 13 deletions(-)

diff --git a/builtin/count-objects.c b/builtin/count-objects.c
index ba92919..1637037 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 == 0 || 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..5f939e4 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1225,6 +1225,15 @@ static void report_helper(const struct string_list *list,
if (seen_bits == (PACKDIR_FILE_PACK|PACKDIR_FILE_IDX))
return;
 
+   if (seen_bits == 
(PACKDIR_FILE_PACK|PACKDIR_FILE_IDX|PACKDIR_FILE_BITMAP))
+   return;
+
+   if (seen_bits == (PACKDIR_FILE_PACK|PACKDIR_FILE_IDX|PACKDIR_FILE_KEEP))
+   return;
+
+   if (seen_bits == 
(PACKDIR_FILE_PACK|PACKDIR_FILE_IDX|PACKDIR_FILE_BITMAP|PACKDIR_FILE_KEEP))
+   return;
+
for (; first < last; first++)
report_garbage(seen_bits, list->items[first].string);
 }
@@ -1256,9 +1265,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

[PATCH 0/3] Add cleanup for garbage .bitmap files

[Doug Kelly]

Following Peff and Junio's comments when adding support for cleaning garbage
.idx files left in the pack directory, this patch introduces the ability to
detect garbage .bitmap files.  Additionally, .keep files are still reported,
but no action is taken to clean them.

This includes some refactor around count-objects' report_pack_garbage handler,
to make it more extensible when adding understanding for different file types.
Testing shows this working, but it may be a section to provide extra scrutiny
to.

--
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 <dougk@gmail.com>
---
 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 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 <dougk@gmail.com>
---
 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 <dougk@gmail.com>
---
 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 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 <dougk@gmail.com>
---
 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 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 <dougk@gmail.com>
---
 builtin/count-objects.c | 16 ++--
 cache.h |  4 +++-
 sha1_file.c | 17 +++--
 t/t5304-prune.sh|  2 ++
 4 files changed, 26 insertions(+), 13 deletions(-)

diff --git a/builtin/count-objects.c b/builtin/count-objects.c
index ba92919..1637037 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 == 0 || 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..5f939e4 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1225,6 +1225,15 @@ static void report_helper(const struct string_list *list,
if (seen_bits == (PACKDIR_FILE_PACK|PACKDIR_FILE_IDX))
return;
 
+   if (seen_bits == 
(PACKDIR_FILE_PACK|PACKDIR_FILE_IDX|PACKDIR_FILE_BITMAP))
+   return;
+
+   if (seen_bits == (PACKDIR_FILE_PACK|PACKDIR_FILE_IDX|PACKDIR_FILE_KEEP))
+   return;
+
+   if (seen_bits == 
(PACKDIR_FILE_PACK|PACKDIR_FILE_IDX|PACKDIR_FILE_BITMAP|PACKDIR_FILE_KEEP))
+   return;
+
for (; first < last; first++)
report_garbage(seen_bits, list->items[first].string);
 }
@@ -1256,9 +1265,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 1/3] prepare_packed_git(): find more garbage

[Doug Kelly]

Yes, without a doubt.  I think I'm blaming this one on being late on a
Friday afternoon, and really not thinking out the logic clearly. :)

On Fri, Nov 13, 2015 at 4:43 PM, Stefan Beller  wrote:
>> +   else if (seen_bits & PACKDIR_FILE_PACK && seen_bits ^ 
>> ~PACKDIR_FILE_IDX)
>
> as just talked about: did you mention && !(seen_bits & FILE_IDX)
>>
>> +   if (seen_bits == 
>> (PACKDIR_FILE_PACK|PACKDIR_FILE_IDX|PACKDIR_FILE_BITMAP))
>> +   return;
>> +
>> +   if (seen_bits == 
>> (PACKDIR_FILE_PACK|PACKDIR_FILE_IDX|PACKDIR_FILE_KEEP))
>> +   return;
>> +
>> +   if (seen_bits == 
>> (PACKDIR_FILE_PACK|PACKDIR_FILE_IDX|PACKDIR_FILE_BITMAP|PACKDIR_FILE_KEEP))
>> +   return;
>
> I wonder if this should be rewritten as
> if (seen_bits & FILE_PACK && seen_bits & FILE_IDX
> && (seen_bits & FILE_KEEP || seen_bits & BITMAP))
> return;
>
> to dense it a bit. ;)
--
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] prepare_packed_git(): refactor garbage reporting in pack directory

[Doug Kelly]

On Wed, Nov 4, 2015 at 1:35 PM, Junio C Hamano <gits...@pobox.com> wrote:
> Doug Kelly <dougk@gmail.com> writes:
>
>> I think the patches I sent (a bit prematurely) address the
>> remaining comments... I did find there was a relevant test in
>> t5304 already, so I added a new test in the same section (and
>> cleaned up some of the garbage it wasn't removing before).  I'm
>> not sure if it's poor form to move tests around like this, but I
>> figured it might be best to keep them logically grouped.
>
> OK, will queue as I didn't spot anything glaringly wrong ;-)
>
> I did wonder if we want to say anything about .bitmap files, though.
> If there is one without matching .idx and .pack, shouldn't we report
> just like we report .idx without .pack (or vice versa)?
>
> Thanks.

I think you're right -- this would be something worth following up on.
At least, t5304 doesn't cover this case explicitly, but when I tried
adding an empty bitmap with a bogus name, I did see a "no
corresponding .idx or .pack" error, similar to the stale .keep file.

I'd trust your (and Jeff's) knowledge on this far more than my own,
but would it be a bad idea to clean up .keep and .bitmap files if the
.idx/.pack pair are missing?  I think we may have had a discussion
previously on how things along these lines might be racey -- but I
don't know what order the .keep file is created in relation to the
.idx/.pack.
--
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] prepare_packed_git(): refactor garbage reporting in pack directory

[Doug Kelly]

On Wed, Nov 4, 2015 at 2:02 PM, Jeff King <p...@peff.net> wrote:
> On Wed, Nov 04, 2015 at 01:56:38PM -0600, Doug Kelly wrote:
>
>> > I did wonder if we want to say anything about .bitmap files, though.
>> > If there is one without matching .idx and .pack, shouldn't we report
>> > just like we report .idx without .pack (or vice versa)?
>>
>> I think you're right -- this would be something worth following up on.
>> At least, t5304 doesn't cover this case explicitly, but when I tried
>> adding an empty bitmap with a bogus name, I did see a "no
>> corresponding .idx or .pack" error, similar to the stale .keep file.
>
> Yeah, that should be harmless warning (although note because the bitmap
> code only really handles a single bitmap, it can prevent loading of the
> "real" bitmap; so you'd want to clean it up, for sure).
>
>> I'd trust your (and Jeff's) knowledge on this far more than my own,
>> but would it be a bad idea to clean up .keep and .bitmap files if the
>> .idx/.pack pair are missing?  I think we may have had a discussion
>> previously on how things along these lines might be racey -- but I
>> don't know what order the .keep file is created in relation to the
>> .idx/.pack.
>
> Definitely cleaning up the .bitmap is sane and not racy (it's in the
> same boat as the .idx, I think).
>
> .keep files are more tricky. I'd have to go over the receive-pack code
> to confirm, but I think they _are_ racy. That is, receive-pack will
> create them as a lockfile before moving the pack into place. That's OK,
> though, if we use mtimes to give ourselves a grace period (I haven't
> looked at your series yet).
>
> But moreover, .keep files can be created manually by the user. If the
> pack they referenced goes away, they are not really serving any purpose.
> But it's possible that the user would want to salvage the content of the
> file, or know that it was there.
>
> So I'd argue we should leave them. Or at least leave ones that do not
> have the generic "{receive,fetch}-pack $pid on $host comment in them,
> which were clearly created as lockfiles.
>
> -Peff

Currently there's no mtime-guarding logic (I dug up that conversation
earlier, though, but after I'd done the respin on this series)... OK,
in that case, I'll create a separate patch that tests/cleans up
.bitmap, but doesn't touch .keep.  This might be a small series since
I think the logic for finding pack garbage doesn't know anything about
.bitmap per-se, so it's looking like I'll extend that relevant code,
before adding the handling in gc and appropriate tests.
--
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(): refactor garbage reporting in pack directory

[Doug Kelly]

From: Junio C Hamano <gits...@pobox.com>

The hook to report "garbage" files in $GIT_OBJECT_DIRECTORY/pack/
could be generic but is too specific to count-object's needs.

Move the part to produce human-readable messages to count-objects,
and refine the interface to callback with the "bits" with values
defined in the cache.h header file, so that other callers (e.g.
prune) can later use the same mechanism to enumerate different
kinds of garbage files and do something intelligent about them,
other than reporting in textual messages.

Signed-off-by: Junio C Hamano <gits...@pobox.com>
Signed-off-by: Doug Kelly <dougk@gmail.com>
---
 builtin/count-objects.c | 26 --
 cache.h |  7 +--
 path.c  |  2 +-
 sha1_file.c | 23 ++-
 4 files changed, 36 insertions(+), 22 deletions(-)

diff --git a/builtin/count-objects.c b/builtin/count-objects.c
index ad0c799..ba92919 100644
--- a/builtin/count-objects.c
+++ b/builtin/count-objects.c
@@ -15,9 +15,31 @@ static int verbose;
 static unsigned long loose, packed, packed_loose;
 static off_t loose_size;
 
-static void real_report_garbage(const char *desc, const char *path)
+static const char *bits_to_msg(unsigned seen_bits)
+{
+   switch (seen_bits) {
+   case 0:
+   return "no corresponding .idx or .pack";
+   case PACKDIR_FILE_GARBAGE:
+   return "garbage found";
+   case PACKDIR_FILE_PACK:
+   return "no corresponding .idx";
+   case PACKDIR_FILE_IDX:
+   return "no corresponding .pack";
+   case PACKDIR_FILE_PACK|PACKDIR_FILE_IDX:
+   default:
+   return NULL;
+   }
+}
+
+static void real_report_garbage(unsigned seen_bits, const char *path)
 {
struct stat st;
+   const char *desc = bits_to_msg(seen_bits);
+
+   if (!desc)
+   return;
+
if (!stat(path, ))
size_garbage += st.st_size;
warning("%s: %s", desc, path);
@@ -27,7 +49,7 @@ static void real_report_garbage(const char *desc, const char 
*path)
 static void loose_garbage(const char *path)
 {
if (verbose)
-   report_garbage("garbage found", path);
+   report_garbage(PACKDIR_FILE_GARBAGE, path);
 }
 
 static int count_loose(const unsigned char *sha1, const char *path, void *data)
diff --git a/cache.h b/cache.h
index 3ba0b8f..736abc0 100644
--- a/cache.h
+++ b/cache.h
@@ -1289,8 +1289,11 @@ struct pack_entry {
 
 extern struct packed_git *parse_pack_index(unsigned char *sha1, const char 
*idx_path);
 
-/* A hook for count-objects to report invalid files in pack directory */
-extern void (*report_garbage)(const char *desc, const char *path);
+/* A hook to report invalid files in pack directory */
+#define PACKDIR_FILE_PACK 1
+#define PACKDIR_FILE_IDX 2
+#define PACKDIR_FILE_GARBAGE 4
+extern void (*report_garbage)(unsigned seen_bits, const char *path);
 
 extern void prepare_packed_git(void);
 extern void reprepare_packed_git(void);
diff --git a/path.c b/path.c
index c740c4f..f28ace2 100644
--- a/path.c
+++ b/path.c
@@ -363,7 +363,7 @@ void report_linked_checkout_garbage(void)
strbuf_setlen(, len);
strbuf_addstr(, path);
if (file_exists(sb.buf))
-   report_garbage("unused in linked checkout", sb.buf);
+   report_garbage(PACKDIR_FILE_GARBAGE, sb.buf);
}
strbuf_release();
 }
diff --git a/sha1_file.c b/sha1_file.c
index c5b31de..3d56746 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1217,27 +1217,16 @@ void install_packed_git(struct packed_git *pack)
packed_git = pack;
 }
 
-void (*report_garbage)(const char *desc, const char *path);
+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)
 {
-   const char *msg;
-   switch (seen_bits) {
-   case 0:
-   msg = "no corresponding .idx or .pack";
-   break;
-   case 1:
-   msg = "no corresponding .idx";
-   break;
-   case 2:
-   msg = "no corresponding .pack";
-   break;
-   default:
+   if (seen_bits == (PACKDIR_FILE_PACK|PACKDIR_FILE_IDX))
return;
-   }
+
for (; first < last; first++)
-   report_garbage(msg, list->items[first].string);
+   report_garbage(seen_bits, list->items[first].string);
 }
 
 static void report_pack_garbage(struct string_list *list)
@@ -1260,7 +1249,7 @@ static void report_pack_garbage(struct string_list *list)
if (baselen == -1) {
const char *dot = strrchr(path, '.');
if (!dot)

Re: [PATCH 1/2] prepare_packed_git(): refactor garbage reporting in pack directory

[Doug Kelly]

On Wed, Oct 28, 2015 at 5:43 PM, Doug Kelly <dougk@gmail.com> wrote:
> On Wed, Oct 28, 2015 at 12:48 PM, Junio C Hamano <gits...@pobox.com> wrote:
>> Junio C Hamano <gits...@pobox.com> writes:
>>
>>> Eric Sunshine <sunsh...@sunshineco.com> writes:
>>>
>>>>> -static void real_report_garbage(const char *desc, const char *path)
>>>>> +const char *bits_to_msg(unsigned seen_bits)
>>>>
>>>> If you don't expect other callers outside this file, then this should
>>>> be declared 'static'. If you do expect future external callers, then
>>>> this should be declared in a public header file (but renamed to be
>>>> more meaningful).
>>>
>>> I think this can be private to this file.  The sole point of moving
>>> this logic to this file is to make it private, after all ;-)  Thanks
>>> for sharp eyes.
>>>
>>> Together with the need for a description on "why", this probably
>>> deserves a test or two, probably at the end of t5304.
>>>
>>> Thanks.
>>
>> Does somebody want to help tying the final loose ends on this topic?
>> It has been listed in the [Stalled] section for too long, I _think_
>> what it attempts to do is a worthy thing, and it is shame to see the
>> initial implementation and review cycles we have spent so far go to
>> waste.
>>
>> If I find nothing else to do before any taker appears, I could
>> volunteer myself, but thought I should ask first.
>>
>> Thanks.
>
> I agree; I've been wanting to get back to it, but had some
> higher-priority things at work for a while, so I've not had time.  I'd
> be happy to get back into it, but if you get to it first, believe me,
> I'm not going to be offended. :)
>
> I'll see if I can't devote a little extra time to it this upcoming
> week, though.  Hopefully it doesn't need too much additional polishing
> to be ready.
>
> P.S. Does a Googler want to tell the Inbox team that the inability to
> send plain-text email is really annoying? :P

I think the patches I sent (a bit prematurely) address the remaining
comments... I did find there was a relevant test in t5304 already, so
I added a new test in the same section (and cleaned up some of the
garbage it wasn't removing before).  I'm not sure if it's poor form to
move tests around like this, but I figured it might be best to keep
them logically grouped.

Let me know if there's anything I can do, and once again, sorry for the delay!
--
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 cleaning pack garbage

[Doug Kelly]

Pack garbage, noticeably stale .idx files, can be cleaned up during
a garbage collection.  This tests to ensure such garbage is properly
cleaned up.

Note that the prior test for checking pack garbage with count-objects
left some stale garbage after the test exited.  This has also been
corrected.

Signed-off-by: Doug Kelly <dougk@gmail.com>
---
 t/t5304-prune.sh | 21 +
 1 file changed, 21 insertions(+)

diff --git a/t/t5304-prune.sh b/t/t5304-prune.sh
index 023d7c6..0297515 100755
--- a/t/t5304-prune.sh
+++ b/t/t5304-prune.sh
@@ -219,6 +219,7 @@ test_expect_success 'gc: prune old objects after local 
clone' '
 
 test_expect_success 'garbage report in count-objects -v' '
test_when_finished "rm -f .git/objects/pack/fake*" &&
+   test_when_finished "rm -f .git/objects/pack/foo*" &&
: >.git/objects/pack/foo &&
: >.git/objects/pack/foo.bar &&
: >.git/objects/pack/foo.keep &&
@@ -244,6 +245,26 @@ EOF
test_cmp expected actual
 '
 
+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 &&
+   : >.git/objects/pack/foo.pack &&
+   : >.git/objects/pack/fake.idx &&
+   : >.git/objects/pack/fake2.keep &&
+   : >.git/objects/pack/fake2.idx &&
+   : >.git/objects/pack/fake3.keep &&
+   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/fake3.keep
+warning: no corresponding .idx: .git/objects/pack/foo.keep
+warning: no corresponding .idx: .git/objects/pack/foo.pack
+EOF
+   test_cmp expected actual
+'
+
 test_expect_success 'prune .git/shallow' '
SHA1=`echo hi|git commit-tree HEAD^{tree}` &&
echo $SHA1 >.git/shallow &&
-- 
2.5.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: Remove garbage .idx files from pack dir

[Doug Kelly]

Add a custom report_garbage handler to collect and remove garbage
.idx files from the pack directory.

Signed-off-by: Doug Kelly <dougk@gmail.com>
---
 builtin/gc.c | 20 
 t/t5304-prune.sh |  2 +-
 2 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index df3e454..668f975 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -45,6 +45,21 @@ static struct argv_array rerere = ARGV_ARRAY_INIT;
 
 static struct tempfile pidfile;
 static struct lock_file log_lock;
+static struct string_list pack_garbage = STRING_LIST_INIT_DUP;
+
+static void clean_pack_garbage(void)
+{
+   int i;
+   for (i = 0; i < pack_garbage.nr; i++)
+   unlink_or_warn(pack_garbage.items[i].string);
+   string_list_clear(_garbage, 0);
+}
+
+static void report_pack_garbage(unsigned seen_bits, const char *path)
+{
+   if (seen_bits == PACKDIR_FILE_IDX)
+   string_list_append(_garbage, path);
+}
 
 static void git_config_date_string(const char *key, const char **output)
 {
@@ -416,6 +431,11 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
if (run_command_v_opt(rerere.argv, RUN_GIT_CMD))
return error(FAILED_RUN, rerere.argv[0]);
 
+   report_garbage = report_pack_garbage;
+   reprepare_packed_git();
+   if (pack_garbage.nr > 0)
+   clean_pack_garbage();
+
if (auto_gc && too_many_loose_objects())
warning(_("There are too many unreachable loose objects; "
"run 'git prune' to remove them."));
diff --git a/t/t5304-prune.sh b/t/t5304-prune.sh
index 0297515..def203c 100755
--- a/t/t5304-prune.sh
+++ b/t/t5304-prune.sh
@@ -245,7 +245,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.5.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 1/2] prepare_packed_git(): refactor garbage reporting in pack directory

[Doug Kelly]

On Wed, Oct 28, 2015 at 12:48 PM, Junio C Hamano  wrote:
> Junio C Hamano  writes:
>
>> Eric Sunshine  writes:
>>
 -static void real_report_garbage(const char *desc, const char *path)
 +const char *bits_to_msg(unsigned seen_bits)
>>>
>>> If you don't expect other callers outside this file, then this should
>>> be declared 'static'. If you do expect future external callers, then
>>> this should be declared in a public header file (but renamed to be
>>> more meaningful).
>>
>> I think this can be private to this file.  The sole point of moving
>> this logic to this file is to make it private, after all ;-)  Thanks
>> for sharp eyes.
>>
>> Together with the need for a description on "why", this probably
>> deserves a test or two, probably at the end of t5304.
>>
>> Thanks.
>
> Does somebody want to help tying the final loose ends on this topic?
> It has been listed in the [Stalled] section for too long, I _think_
> what it attempts to do is a worthy thing, and it is shame to see the
> initial implementation and review cycles we have spent so far go to
> waste.
>
> If I find nothing else to do before any taker appears, I could
> volunteer myself, but thought I should ask first.
>
> Thanks.

I agree; I've been wanting to get back to it, but had some
higher-priority things at work for a while, so I've not had time.  I'd
be happy to get back into it, but if you get to it first, believe me,
I'm not going to be offended. :)

I'll see if I can't devote a little extra time to it this upcoming
week, though.  Hopefully it doesn't need too much additional polishing
to be ready.

P.S. Does a Googler want to tell the Inbox team that the inability to
send plain-text email is really annoying? :P
--
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/2] prepare_packed_git(): refactor garbage reporting in pack directory

[Doug Kelly]

From: Junio C Hamano gits...@pobox.com

The hook to report garbage files in $GIT_OBJECT_DIRECTORY/pack/
could be generic but is too specific to count-object's needs.

Move the part to produce human-readable messages to count-objects,
and refine the interface to callback with the bits with values
defined in the cache.h header file, so that other callers (e.g.
prune) can later use the same mechanism to enumerate different
kinds of garbage files and do something intelligent about them,
other than reporting in textual messages.

Signed-off-by: Junio C Hamano gits...@pobox.com
---
 builtin/count-objects.c | 26 --
 cache.h |  7 +--
 path.c  |  2 +-
 sha1_file.c | 23 ++-
 4 files changed, 36 insertions(+), 22 deletions(-)

diff --git a/builtin/count-objects.c b/builtin/count-objects.c
index ad0c799..4c3198e 100644
--- a/builtin/count-objects.c
+++ b/builtin/count-objects.c
@@ -15,9 +15,31 @@ static int verbose;
 static unsigned long loose, packed, packed_loose;
 static off_t loose_size;
 
-static void real_report_garbage(const char *desc, const char *path)
+const char *bits_to_msg(unsigned seen_bits)
+{
+   switch (seen_bits) {
+   case 0:
+   return no corresponding .idx or .pack;
+   case PACKDIR_FILE_GARBAGE:
+   return garbage found;
+   case PACKDIR_FILE_PACK:
+   return no corresponding .idx;
+   case PACKDIR_FILE_IDX:
+   return no corresponding .pack;
+   case PACKDIR_FILE_PACK|PACKDIR_FILE_IDX:
+   default:
+   return NULL;
+   }
+}
+
+static void real_report_garbage(unsigned seen_bits, const char *path)
 {
struct stat st;
+   const char *desc = bits_to_msg(seen_bits);
+
+   if (!desc)
+   return;
+
if (!stat(path, st))
size_garbage += st.st_size;
warning(%s: %s, desc, path);
@@ -27,7 +49,7 @@ static void real_report_garbage(const char *desc, const char 
*path)
 static void loose_garbage(const char *path)
 {
if (verbose)
-   report_garbage(garbage found, path);
+   report_garbage(PACKDIR_FILE_GARBAGE, path);
 }
 
 static int count_loose(const unsigned char *sha1, const char *path, void *data)
diff --git a/cache.h b/cache.h
index 6bb7119..2d4dedc 100644
--- a/cache.h
+++ b/cache.h
@@ -1212,8 +1212,11 @@ struct pack_entry {
 
 extern struct packed_git *parse_pack_index(unsigned char *sha1, const char 
*idx_path);
 
-/* A hook for count-objects to report invalid files in pack directory */
-extern void (*report_garbage)(const char *desc, const char *path);
+/* A hook to report invalid files in pack directory */
+#define PACKDIR_FILE_PACK 1
+#define PACKDIR_FILE_IDX 2
+#define PACKDIR_FILE_GARBAGE 4
+extern void (*report_garbage)(unsigned seen_bits, const char *path);
 
 extern void prepare_packed_git(void);
 extern void reprepare_packed_git(void);
diff --git a/path.c b/path.c
index 10f4cbf..75ec236 100644
--- a/path.c
+++ b/path.c
@@ -143,7 +143,7 @@ void report_linked_checkout_garbage(void)
strbuf_setlen(sb, len);
strbuf_addstr(sb, path);
if (file_exists(sb.buf))
-   report_garbage(unused in linked checkout, sb.buf);
+   report_garbage(PACKDIR_FILE_GARBAGE, sb.buf);
}
strbuf_release(sb);
 }
diff --git a/sha1_file.c b/sha1_file.c
index 1cee438..0c0b652 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1183,27 +1183,16 @@ void install_packed_git(struct packed_git *pack)
packed_git = pack;
 }
 
-void (*report_garbage)(const char *desc, const char *path);
+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)
 {
-   const char *msg;
-   switch (seen_bits) {
-   case 0:
-   msg = no corresponding .idx or .pack;
-   break;
-   case 1:
-   msg = no corresponding .idx;
-   break;
-   case 2:
-   msg = no corresponding .pack;
-   break;
-   default:
+   if (seen_bits == (PACKDIR_FILE_PACK|PACKDIR_FILE_IDX))
return;
-   }
+
for (; first  last; first++)
-   report_garbage(msg, list-items[first].string);
+   report_garbage(seen_bits, list-items[first].string);
 }
 
 static void report_pack_garbage(struct string_list *list)
@@ -1226,7 +1215,7 @@ static void report_pack_garbage(struct string_list *list)
if (baselen == -1) {
const char *dot = strrchr(path, '.');
if (!dot) {
-   report_garbage(garbage found, path);
+   report_garbage(PACKDIR_FILE_GARBAGE, path);
continue;
}
 

[PATCH 2/2] gc: Remove garbage .idx files from pack dir

[Doug Kelly]

Add a custom report_garbage handler to collect and remove garbage
.idx files from the pack directory.

Signed-off-by: Doug Kelly dougk@gmail.com
---
 builtin/gc.c | 21 +
 1 file changed, 21 insertions(+)

diff --git a/builtin/gc.c b/builtin/gc.c
index bcc75d9..8352616 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -42,8 +42,18 @@ static struct argv_array prune = ARGV_ARRAY_INIT;
 static struct argv_array prune_worktrees = ARGV_ARRAY_INIT;
 static struct argv_array rerere = ARGV_ARRAY_INIT;
 
+static struct string_list pack_garbage = STRING_LIST_INIT_DUP;
+
 static char *pidfile;
 
+static void clean_pack_garbage(void)
+{
+   int i;
+   for (i = 0; i  pack_garbage.nr; i++)
+   unlink_or_warn(pack_garbage.items[i].string);
+   string_list_clear(pack_garbage, 0);
+}
+
 static void remove_pidfile(void)
 {
if (pidfile)
@@ -57,6 +67,12 @@ static void remove_pidfile_on_signal(int signo)
raise(signo);
 }
 
+static void report_pack_garbage(unsigned seen_bits, const char *path)
+{
+   if (seen_bits == PACKDIR_FILE_IDX)
+   string_list_append(pack_garbage, path);
+}
+
 static void git_config_date_string(const char *key, const char **output)
 {
if (git_config_get_string_const(key, output))
@@ -372,6 +388,11 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
if (run_command_v_opt(rerere.argv, RUN_GIT_CMD))
return error(FAILED_RUN, rerere.argv[0]);
 
+   report_garbage = report_pack_garbage;
+   reprepare_packed_git();
+   if (pack_garbage.nr  0)
+   clean_pack_garbage();
+
if (auto_gc  too_many_loose_objects())
warning(_(There are too many unreachable loose objects; 
run 'git prune' to remove them.));
-- 
2.0.5

--
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: Question: .idx without .pack causes performance issues?

[Doug Kelly]

On Mon, Aug 3, 2015 at 8:27 PM, Junio C Hamano gits...@pobox.com wrote:
 Doug Kelly dougk@gmail.com writes:

 Here's a change to prune.c that at least addresses the issue by removing
 .idx files without an associated pack, but it's by no means pretty.  If 
 anyone
 has any feedback before I turn this into a formal patch, it's more than 
 welcome!

 I'd hesitate to see removal of a file (for that matter, a creation
 too) inside a while (de = readdir) loop.  As the original function
 is about temporary files, and the new thing is not about temporary
 files at all, I'd further prefer that we do not do it in the same
 loop.

 I am wondering if we can add a new mode to report_pack_garbage() in
 sha1_file.c to allow it to remove stale and lone .idx.  Most of
 the time we are accessing packs read-only, and I do not want the
 function to unconditionally remove lone .idx, but perhaps we
 can teach prune to set a custom report_garbage() routine and
 react to a call to its custom report_garbage()?

 Perhaps that custom report_garbage() can make a list of .idx
 files, iterate over it to pick the lone one without .pack and
 remove them.  Or the custom report_garbage() can make a list of lone
 .idx files, if you tweak the interface to report_garbage() to
 contain th seen_bits value, avoiding the need to check the existence
 of .pack for the second time.

Yeah, I didn't think this was the cleanest solution, and I wasn't even
thinking about removing while inside the readdir loop, but I can see
how that might be a very bad idea.  In any case, thanks for the
suggestions... I'll be completely blunt in saying I'm far less than
well-versed in the Git internals.  Looking at the implementation of
report_pack_garbage(), it does look like seen_bits already has this
logic, and indeed, git count-objects -v reports the files as garbage.

So, I think you're right: prune would need to set report_garbage
appropriately, then call count-objects to clean that up.  If we wanted
it to *only* care for lone idx files, we would have to string match on
the message (seems fragile), but perhaps a more observant approach
would be to add a custom flag to prune to clean *all* garbage in the
repository, as passed to report_garbage?  Probably wouldn't want to be
enabled by default, but only on invocation or with careful
consideration and setting an appropriate config flag.

Thoughts?
--
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: Question: .idx without .pack causes performance issues?

[Doug Kelly]

On Tue, Jul 21, 2015 at 4:37 PM, Doug Kelly dougk@gmail.com wrote:
 On Tue, Jul 21, 2015 at 3:48 PM, Junio C Hamano gits...@pobox.com wrote:
 Junio C Hamano gits...@pobox.com writes:

 While I still think that it is more important to prevent such a
 situation from occurring in the first place, ignoring .idx that lack
 corresponding .pack should be fairly simple, perhaps like this.
 ...

 Sorry for the noise, but this patch is worthless.  We already have
 an equivalent test in add_packed_git() that is called from this same
 place.

 And a few extra updates from me: we found that this appears to occur
 even after update to 1.9.5, and setting core.fscache on 2.4.6 has no
 appreciable impact on the time it takes to run git fetch, either.
 Our thought was antivirus (or something else?) might have the file
 open when git attempts to unlink the .idx, but perhaps it's something
 else, too?  In one case, we had ~560 orphaned .idx files, but 150
 seems sufficient to slow a fetch operation for a few minutes until it
 actually begins transferring objects.

 The git gc approach to cleaning up the mess is certainly looking
 more and more attractive... :)

Here's a change to prune.c that at least addresses the issue by removing
.idx files without an associated pack, but it's by no means pretty.  If anyone
has any feedback before I turn this into a formal patch, it's more than welcome!

diff --git a/builtin/prune.c b/builtin/prune.c
index 10b03d3..8a60282 100644
--- a/builtin/prune.c
+++ b/builtin/prune.c
@@ -1,6 +1,7 @@
 #include cache.h
 #include commit.h
 #include diff.h
+#include dir.h
 #include revision.h
 #include builtin.h
 #include reachable.h
@@ -85,15 +86,31 @@ static void remove_temporary_files(const char *path)
 {
DIR *dir;
struct dirent *de;
+   struct strbuf idx, pack;

dir = opendir(path);
if (!dir) {
fprintf(stderr, Unable to open directory %s\n, path);
return;
}
-   while ((de = readdir(dir)) != NULL)
+   while ((de = readdir(dir)) != NULL) {
if (starts_with(de-d_name, tmp_))
prune_tmp_file(mkpath(%s/%s, path, de-d_name));
+   if (ends_with(de-d_name, .idx)) {
+   strbuf_init(idx, 0);
+   strbuf_init(pack, 0);
+   strbuf_addstr(idx, de-d_name);
+   strbuf_addbuf(pack, idx);
+   if (strbuf_strip_suffix(pack, .idx)) {
+   strbuf_addstr(pack, .pack);
+   if (!file_exists(mkpath(%s/%s, path,
pack.buf)))
+   prune_tmp_file(mkpath(%s/%s,
path, idx.buf));
+   }
+   strbuf_release(idx);
+   strbuf_release(pack);
+   }
+
+   }
closedir(dir);
 }

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

Question: .idx without .pack causes performance issues?

[Doug Kelly]

Hi all,

I just wanted to relay an issue we've seen before at my day job (and
it just recently cropped up again).  When moving users from Git for
Windows 1.8.3 to 1.9.5, we found a few users started having operations
take an excruciatingly long amount of time.  At some point, we traced
the issue to a number of .pack files had been deleted (possibly
garbage collected?) -- but their associated .idx files were still
present.  Upon removing the orphaned idx files, we found performance
returned to normal.  Otherwise, git fsck reported no issues with the
repositories.

Other users have noted that using git gc would sometimes correct the
issue for them, but not always.

Anyway, has anyone else experienced this performance degradation? I
have some feeling that it's an issue that may be exclusive to Windows
(or at least, only slow enough to matter on Windows), but I have no
proof, and I've never heard of an issue like this outside work. (One
idea that came to mind was even the .idx files were locked, and thus
not deleted.)  Something tells me deleting the orphaned .idx files
isn't the nicest solution, either.

Thanks!

--Doug

P.S. In addition to running the Git for Windows/msysgit builds, we
have a handful of users running Git Extensions as well, and also have
been seeing an increase in use of Visual Studio 2013 -- which of
course has libgit2 integrated. So, I think the chance that any one of
these might be using the repo or holding files open is very high.
--
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: Question: .idx without .pack causes performance issues?

[Doug Kelly]

On Tue, Jul 21, 2015 at 1:57 PM, Junio C Hamano gits...@pobox.com wrote:
 I wouldn't be surprised if such a configuration to have leftover
 .idx files that lack .pack affected performance, but I think you
 really have to work on getting into such a situation (unless your
 operating system is very cooperative and tries hard to corrupt your
 repository, that is ;-), so I wouldn't be surprised if you were the
 first one to report this.

I'm inclined to believe Windows isn't helping this situation: seems
like something it might do, especially because of how it behaves if
one process has a file open. Since I haven't caught a case where these
files show up, maybe adding some tweaks to look for it occurring (such
as on our Jenkins workers, if it's happening there now) would give us
a better indication of the why question.  It could even be that it
has occurred long ago, and the performance issue is just now observed:
our environment has run 1.7.4, 1.8.3, and now 1.9.5 -- so even an
unknown bug in a previous version could impact us now.


 We open the .idx file and try to keep as many of them in-core,
 without opening corresponding .pack until the data is needed.

 When we need an object, we learn from an .idx file that a
 particular pack ought to have a copy of it, and then attempt to open
 the corresponding .pack file.  If this fails, we do protect
 ourselves from strange repositories with only .idx files by not
 using that .idx and try to see if the sought-after object exists
 elsewhere (and if there isn't we say no such object, which is also
 a correct thing to do).

 I however do not think that we mark the in-core structure that
 corresponds to an open .idx file in any way when such a failure
 happens.  If we really cared enough, we could do so, saying we know
 there is .idx file, but do not bother looking at it again, as we
 know the corresponding .pack is missing, and that would speed things
 up a bit, essentially bringing us back to a sane situation without
 any .idx without corresponding .pack.

I think this is where the performance hit occurs on Windows: file stat
operations in general are pretty slow, and I know msysgit did some
things to emulate as much of the POSIX API as possible -- which isn't
always easy on Windows.  But, some of the developers that know
compat/win32/ better would know more (I recall the dirent stuff being
pretty complicated, but open/fopen seem rather straightforward).  And
yes -- retrying the operation each time and failing only compounds the
issue.


 I do not think it is worth the effort, though.  It would be more
 fruitful to find out how you end up with .idx exists but not
 corresponding .pack and if that is some systemic failure, see if
 there is a way to prevent that from happening in the first place.

Agreed.  It feels like a workaround for a case where you're already in
a bad state...


 Also, I think it may not be a bad idea to teach gc to remove stale
 .idx files that do not have corresponding .pack as garbage.

I agree.  This seems like a more correct solution -- if gc understands
to clean up these bad .idx files, it would then be a non-issue when
searching the packs.  The solution you posted to check if an
associated packfile exists -- while perhaps not belonging there --
could still be useful to delete orphanend .idx files.

I think you're correct, though -- if you did propose the solution to
sha1_file.c, it would be necessary to prevent scanning that .idx
again, or else any potential gains would be lost continually
stat()'ing the file.  Now, msysgit does have core.fscache to try
caching the stat()/lstat() results to lessen the impact, but this
isn't on by default, I believe.
--
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: Question: .idx without .pack causes performance issues?

[Doug Kelly]

On Tue, Jul 21, 2015 at 3:48 PM, Junio C Hamano gits...@pobox.com wrote:
 Junio C Hamano gits...@pobox.com writes:

 While I still think that it is more important to prevent such a
 situation from occurring in the first place, ignoring .idx that lack
 corresponding .pack should be fairly simple, perhaps like this.
 ...

 Sorry for the noise, but this patch is worthless.  We already have
 an equivalent test in add_packed_git() that is called from this same
 place.

And a few extra updates from me: we found that this appears to occur
even after update to 1.9.5, and setting core.fscache on 2.4.6 has no
appreciable impact on the time it takes to run git fetch, either.
Our thought was antivirus (or something else?) might have the file
open when git attempts to unlink the .idx, but perhaps it's something
else, too?  In one case, we had ~560 orphaned .idx files, but 150
seems sufficient to slow a fetch operation for a few minutes until it
actually begins transferring objects.

The git gc approach to cleaning up the mess is certainly looking
more and more attractive... :)
--
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: Windows path limites, was Re: [PATCH v5 2/5] setup: sanity check file size in read_gitfile_gently

[Doug Kelly]

On Tue, Apr 28, 2015 at 2:23 AM Johannes Schindelin
johannes.schinde...@gmx.de wrote:

 Hi Peff,

 On 2015-04-28 08:02, Jeff King wrote:

  My understanding is that PATH_MAX is set absurdly low on Windows
  systems (and doesn't actually represent the real limit of a path!).

 Well, yes and no. Yes, it is absurdly low on Windows, and yes, it is not the 
 real limit of a path *if you know how to work around it*. Most Win32 API 
 calls actually do have that absurdly low limit, but internally longer paths 
 can be represented (and there is a hack where you prefix the path -- which 
 must be an absolute one for this hack -- by `\\?\`). Keep in mind, though, 
 that even the Windows Explorer is (at least sometimes) limited by the 
 absurdly low path limit.


One more thing worth noting is that path lengths are actually restricted further
within git_path_submodule(), I would presume to reserve some amount of space
when cloning a submodule (it seems to set aside 100 characters)?  For other
cases, yes, Dscho is absolutely right: the MAX_PATH constant is 260 (which gives
something like 256 usable characters).  If you're able to do everything through
the Unicode Win32 APIs, you can reach 65535 characters, assuming the filesystem
supports it (NTFS does, FAT32 would not, for example).  I recall there being one
function (possibly thinking of mktemp) that couldn't properly handle the long
paths, but I believe that was it.

Other apps may or may not handle the longer paths well; the core.longpaths
switch may prevent mishaps in more than just Explorer, but the downside isn't
disaster always either -- for example, Explorer just refuses to browse
to that path.
(Note that other apps may include both the msys bits as well as the Windows
command line built-in commands, which may make things that farm out to sh
behave in unusual ways.  I vaguely remember testing this myself, but I
don't recall
what I did exactly or what happened.)

--Doug
--
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: submodule.$name.url is ignored during submodule update

[Doug Kelly]

On Thu, Mar 19, 2015 at 4:27 AM, Dmitry Neverov
dmitry.neve...@gmail.com wrote:
 Hi,

 I've noticed that the 'submodule.$name.url' config parameter from the
 main repository is ignored when a submodule needs to be updated, the
 submodule's 'remote.origin.url' is used instead. Is there any way to
 customize the submodule url for both the initial clone and for
 updates?

That's what git submodule sync is for. It will synchronize the url
in .gitmodules with
to the remote.origin.url for each submodule.  I'm not sure about the second part
of your question: are you talking about using different URLs for the
initial clone
and updates?
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Git with Lader logic

[Doug Kelly]

On Wed, Mar 18, 2015 at 2:53 PM, Randall S. Becker
rsbec...@nexbridge.com wrote:
 On March 17, 2015 7:34 PM, Bharat Suvarna wrote:
 I am trying to find a way of using version control on PLC programmers like
 Allen
 Bradley PLC. I can't find a way of this.

 Could you please give me an idea if it will work with Plc programs. Which
 are
 basically Ladder logic.

 Many PLC programs either store their project code in XML, L5K or L5X (for
 example), TXT, CSV, or some other text format or can import and export to
 text forms. If you have a directory structure that represents your project,
 and the file formats have reasonable line separators so that diffs can be
 done easily, git very likely would work out for you. You do not have to have
 the local .git repository in the same directory as your working area if your
 tool has issues with that or .gitignore. You may want to use a GUI client to
 manage your local repository and handle the commit/push/pull/merge/rebase
 functions as I expect whatever PLC system you are using does not have git
 built-in.

 To store binary PLC data natively, which some tools use, I expect that those
 who are better at git-conjuring than I, could provide guidance on how to
 automate binary diffs for your tool's particular file format.

The one thing I find interesting about RSLogix in general (caveat: I
only have very limited experience with RSLogix 500 / 5000; if I do
anything nowadays, it's in the micro series using RSLogix Micro
Starter Lite)... they do have some limited notion of version control
inside the application itself, though it seems rudimentary to me.
This could prove to be helpful or extremely annoying, since even when
I connect to a PLC and go online, just to reset the RTC, it still
prompts me to save again (even though nothing changed, other than the
processor state).

You may also find this link on stackexchange helpful:
http://programmers.stackexchange.com/questions/102487/are-there-realistic-useful-solutions-for-source-control-for-ladder-logic-program

As Randall noted, L5K is just text, and RSLogix 5000 uses it,
according to this post.  It may work okay.

--Doug
--
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: Need help deciding between subtree and submodule

[Doug Kelly]

On Wed, Mar 18, 2015 at 3:20 AM, Chris Packham judge.pack...@gmail.com wrote:
 My $0.02 based on $dayjob

 (disclaimer I've never used subtree)

 On Wed, Mar 18, 2015 at 11:14 AM, Robert Dailey
 rcdailey.li...@gmail.com wrote:
 At my workplace, the team is using Atlassian Stash + git

 We have a Core library that is our common code between various
 projects. To avoid a single monolithic repository and to allow our
 apps and tools to be modularized into their own repos, I have
 considered moving Core to a subtree or submodule.

$DAYJOB has actually tried both... with varying levels of success.  As
you note, subtree looks wonderful from a user perspective, but behind
the scenes, it does have issues.  In our case, subtree support was
modified into Gerrit, and this became cumbersome and difficult to
maintain (which is the reason we eventually dropped support for
subtree).  Submodules have more of a labor-intensive aspect, but
are far more obvious about what actions have been taken (IMHO).
Either way, both our developers' needs were satisfied: the code was
tracked cleanly, and there wasn't a configuration mismatch where
a dependency was able to change versions without implicit direction.


 Our environment is slightly different. Our projects are made up
 entirely of submodules, we don't embed submodules within a repo with
 actual code (side note: I know syslog-ng does so it might be worth
 having a look around there).

 Day to day development is done at the submodule level. A developer
 working on a particular feature is generally only touching one repo
 notwithstanding a little bit of to-and-fro as they work on the UI
 aspects. When changes do touch multiple submodules the pushes can
 generally be ordered in a sane manner. Things get a little complicated
 when there are interdependent changes, then those pushes require
 co-operation between submodule owners.

We've done both (all of the above? a hybrid approach?)... We've gone so
far to create 30 modules for every conceivable component, then tried to
work that way with submodule, and our developers quickly revolted as it
became too much of a maintenance burden.  The other direction (with
hugely monolithic code) is also problematic since the module boundaries
become blurred.  For us, usually cooperation between modules isn't so
difficult, but the problem comes about when attempting to merge the
changes.  Sometimes, it can take significant effort to ensure conflict-free
merges (going so far as to require merge lock emails to ask other
developers to hold off on merging commits until the change lands
completely and the project is stable).


 The key to making this work is our build system. It is the thing that
 updates the project repo. After a successful build for all targets (we
 hope to add unit/regression tests one day) the submodules sha1s are
 updated and a new baseline (to borrow a clearcase term) is published.
 Developers can do git pull  git submodule update to get the latest
 stable baseline, but they can also run git pull in a submodule if they
 want to be on the bleeding edge.

 I tried subtree and this is definitely far more transparent and simple
 to the team (simplicity is very important), however I notice it has
 problems with unnecessary conflicts when you do not do `git subtree
 push` for each `git subtree pull`. This is unnecessary overhead and
 complicates the log graph which I don't like.

 Submodule functionally works but it is complicated. We make heavy use
 of pull requests for code reviews (they are required due to company
 policy). Instead of a pull request being atomic and containing any app
 changes + accompanying Core changes, we now need to create two pull
 requests and manage them in proper order. Things also become more
 difficult when branching. All around it just feels like submodule
 would interfere and add more administration overhead on a day to day
 basis, affecting productivity.

 We do have policies around review etc. With submodules it does
 sometimes require engaging owners/reviewers from multiple
 repositories. Tools like Gerrit can help, particularly where multiple
 changes and reviewers are involved.

Conflicts are definitely going to be a difficulty with either subtree or
submodule (if multiple users could be changing the submodule), but
if you have additional tools, such as Gerrit to look out for, submodule
is the way to go since subtrees aren't supported within Gerrit. (Other
tools may support it better: I'm honestly not sure?)  That would be
my one word of caution: I don't know how well Stash supports subtree.

You are absolutely correct about the difficulty of integrating submodule
pull requests taking two steps.  This was an issue we worked hard
to mitigate here, but at the end of the day, the work is necessary.
Basically, we could also use a feature within Gerrit to automatically
bring up a specific branch of the superproject when the submodule
project on a certain branch changes, but this also rolls the dice a 

[PATCH v5 1/2] t4255: test am submodule with diff.submodule

[Doug Kelly]

git am will break when using diff.submodule=log; add some test cases
to illustrate this breakage as simply as possible.  There are
currently two ways this can fail:

* With errors (unrecognized input), if only change
* Silently (no submodule change), if other files change

Test for both conditions and ensure without diff.submodule this works.

Helped-by: Eric Sunshine sunsh...@sunshineco.com
Helped-by: Junio C Hamano gits...@pobox.com
Signed-off-by: Doug Kelly dougk@gmail.com
---
Added a comment for why test_might_fail is used to abort merges
in progress.

 t/t4255-am-submodule.sh | 72 +
 1 file changed, 72 insertions(+)

diff --git a/t/t4255-am-submodule.sh b/t/t4255-am-submodule.sh
index 8bde7db..450d261 100755
--- a/t/t4255-am-submodule.sh
+++ b/t/t4255-am-submodule.sh
@@ -18,4 +18,76 @@ am_3way () {
 KNOWN_FAILURE_NOFF_MERGE_ATTEMPTS_TO_MERGE_REMOVED_SUBMODULE_FILES=1
 test_submodule_switch am_3way
 
+test_expect_success 'setup diff.submodule' '
+   test_commit one 
+   INITIAL=$(git rev-parse HEAD) 
+
+   git init submodule 
+   (
+   cd submodule 
+   test_commit two 
+   git rev-parse HEAD ../initial-submodule
+   ) 
+   git submodule add ./submodule 
+   git commit -m first 
+
+   (
+   cd submodule 
+   test_commit three 
+   git rev-parse HEAD ../first-submodule
+   ) 
+   git add submodule 
+   git commit -m second 
+   SECOND=$(git rev-parse HEAD) 
+
+   (
+   cd submodule 
+   git mv two.t four.t 
+   git commit -m second submodule 
+   git rev-parse HEAD ../second-submodule
+   ) 
+   test_commit four 
+   git add submodule 
+   git commit --amend --no-edit 
+   THIRD=$(git rev-parse HEAD) 
+   git submodule update --init
+'
+
+run_test() {
+   START_COMMIT=$1 
+   EXPECT=$2 
+   # Abort any merges in progress: the previous
+   # test may have failed, and we should clean up.
+   test_might_fail git am --abort 
+   git reset --hard $START_COMMIT 
+   rm -f *.patch 
+   git format-patch -1 
+   git reset --hard $START_COMMIT^ 
+   git submodule update 
+   git am *.patch 
+   git submodule update 
+   git -C submodule rev-parse HEAD actual 
+   test_cmp $EXPECT actual
+}
+
+test_expect_success 'diff.submodule unset' '
+   test_unconfig diff.submodule 
+   run_test $SECOND first-submodule
+'
+
+test_expect_success 'diff.submodule unset with extra file' '
+   test_unconfig diff.submodule 
+   run_test $THIRD second-submodule
+'
+
+test_expect_failure 'diff.submodule=log' '
+   test_config diff.submodule log 
+   run_test $SECOND first-submodule
+'
+
+test_expect_failure 'diff.submodule=log with extra file' '
+   test_config diff.submodule log 
+   run_test $THIRD second-submodule
+'
+
 test_done
-- 
2.0.5

--
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 v5 2/2] format-patch: ignore diff.submodule setting

[Doug Kelly]

diff.submodule when set to log produces output which git-am cannot
handle. Ignore this setting when generating patch output.

Signed-off-by: Doug Kelly dougk@gmail.com
---
 builtin/log.c   | 2 +-
 t/t4255-am-submodule.sh | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index 734aab3..cb14db4 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -705,7 +705,7 @@ static int git_format_config(const char *var, const char 
*value, void *cb)
return 0;
}
if (!strcmp(var, diff.color) || !strcmp(var, color.diff) ||
-   !strcmp(var, color.ui)) {
+   !strcmp(var, color.ui) || !strcmp(var, diff.submodule)) {
return 0;
}
if (!strcmp(var, format.numbered)) {
diff --git a/t/t4255-am-submodule.sh b/t/t4255-am-submodule.sh
index 450d261..0ba8194 100755
--- a/t/t4255-am-submodule.sh
+++ b/t/t4255-am-submodule.sh
@@ -80,12 +80,12 @@ test_expect_success 'diff.submodule unset with extra file' '
run_test $THIRD second-submodule
 '
 
-test_expect_failure 'diff.submodule=log' '
+test_expect_success 'diff.submodule=log' '
test_config diff.submodule log 
run_test $SECOND first-submodule
 '
 
-test_expect_failure 'diff.submodule=log with extra file' '
+test_expect_success 'diff.submodule=log with extra file' '
test_config diff.submodule log 
run_test $THIRD second-submodule
 '
-- 
2.0.5

--
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 v4 1/2] t4255: test am submodule with diff.submodule

[Doug Kelly]

git am will break when using diff.submodule=log; add some test cases
to illustrate this breakage as simply as possible.  There are
currently two ways this can fail:

* With errors (unrecognized input), if only change
* Silently (no submodule change), if other files change

Test for both conditions and ensure without diff.submodule this works.

Helped-by: Eric Sunshine sunsh...@sunshineco.com
Helped-by: Junio C Hamano gits...@pobox.com
Signed-off-by: Doug Kelly dougk@gmail.com
---
Updated to remove test_ticks and clean the commit message.

 t/t4255-am-submodule.sh | 70 +
 1 file changed, 70 insertions(+)

diff --git a/t/t4255-am-submodule.sh b/t/t4255-am-submodule.sh
index 8bde7db..a38c305 100755
--- a/t/t4255-am-submodule.sh
+++ b/t/t4255-am-submodule.sh
@@ -18,4 +18,74 @@ am_3way () {
 KNOWN_FAILURE_NOFF_MERGE_ATTEMPTS_TO_MERGE_REMOVED_SUBMODULE_FILES=1
 test_submodule_switch am_3way
 
+test_expect_success 'setup diff.submodule' '
+   test_commit one 
+   INITIAL=$(git rev-parse HEAD) 
+
+   git init submodule 
+   (
+   cd submodule 
+   test_commit two 
+   git rev-parse HEAD ../initial-submodule
+   ) 
+   git submodule add ./submodule 
+   git commit -m first 
+
+   (
+   cd submodule 
+   test_commit three 
+   git rev-parse HEAD ../first-submodule
+   ) 
+   git add submodule 
+   git commit -m second 
+   SECOND=$(git rev-parse HEAD) 
+
+   (
+   cd submodule 
+   git mv two.t four.t 
+   git commit -m second submodule 
+   git rev-parse HEAD ../second-submodule
+   ) 
+   test_commit four 
+   git add submodule 
+   git commit --amend --no-edit 
+   THIRD=$(git rev-parse HEAD) 
+   git submodule update --init
+'
+
+run_test() {
+   START_COMMIT=$1 
+   EXPECT=$2 
+   test_might_fail git am --abort 
+   git reset --hard $START_COMMIT 
+   rm -f *.patch 
+   git format-patch -1 
+   git reset --hard $START_COMMIT^ 
+   git submodule update 
+   git am *.patch 
+   git submodule update 
+   git -C submodule rev-parse HEAD actual 
+   test_cmp $EXPECT actual
+}
+
+test_expect_success 'diff.submodule unset' '
+   test_unconfig diff.submodule 
+   run_test $SECOND first-submodule
+'
+
+test_expect_success 'diff.submodule unset with extra file' '
+   test_unconfig diff.submodule 
+   run_test $THIRD second-submodule
+'
+
+test_expect_failure 'diff.submodule=log' '
+   test_config diff.submodule log 
+   run_test $SECOND first-submodule
+'
+
+test_expect_failure 'diff.submodule=log with extra file' '
+   test_config diff.submodule log 
+   run_test $THIRD second-submodule
+'
+
 test_done
-- 
2.0.5

--
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 v4 2/2] format-patch: ignore diff.submodule setting

[Doug Kelly]

diff.submodule when set to log produces output which git-am cannot
handle. Ignore this setting when generating patch output.

Signed-off-by: Doug Kelly dougk@gmail.com
---
 builtin/log.c   | 2 +-
 t/t4255-am-submodule.sh | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index 734aab3..cb14db4 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -705,7 +705,7 @@ static int git_format_config(const char *var, const char 
*value, void *cb)
return 0;
}
if (!strcmp(var, diff.color) || !strcmp(var, color.diff) ||
-   !strcmp(var, color.ui)) {
+   !strcmp(var, color.ui) || !strcmp(var, diff.submodule)) {
return 0;
}
if (!strcmp(var, format.numbered)) {
diff --git a/t/t4255-am-submodule.sh b/t/t4255-am-submodule.sh
index a38c305..27ea698 100755
--- a/t/t4255-am-submodule.sh
+++ b/t/t4255-am-submodule.sh
@@ -78,12 +78,12 @@ test_expect_success 'diff.submodule unset with extra file' '
run_test $THIRD second-submodule
 '
 
-test_expect_failure 'diff.submodule=log' '
+test_expect_success 'diff.submodule=log' '
test_config diff.submodule log 
run_test $SECOND first-submodule
 '
 
-test_expect_failure 'diff.submodule=log with extra file' '
+test_expect_success 'diff.submodule=log with extra file' '
test_config diff.submodule log 
run_test $THIRD second-submodule
 '
-- 
2.0.5

--
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 2/2] format-patch: ignore diff.submodule setting

[Doug Kelly]

diff.submodule when set to log produces output which git-am cannot
handle. Ignore this setting when generating patch output.

Signed-off-by: Doug Kelly dougk@gmail.com
---
 builtin/log.c   | 2 +-
 t/t4255-am-submodule.sh | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index 734aab3..cb14db4 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -705,7 +705,7 @@ static int git_format_config(const char *var, const char 
*value, void *cb)
return 0;
}
if (!strcmp(var, diff.color) || !strcmp(var, color.diff) ||
-   !strcmp(var, color.ui)) {
+   !strcmp(var, color.ui) || !strcmp(var, diff.submodule)) {
return 0;
}
if (!strcmp(var, format.numbered)) {
diff --git a/t/t4255-am-submodule.sh b/t/t4255-am-submodule.sh
index 523accf..31cbdba 100755
--- a/t/t4255-am-submodule.sh
+++ b/t/t4255-am-submodule.sh
@@ -80,12 +80,12 @@ test_expect_success 'diff.submodule unset with extra file' '
run_test $THIRD second-submodule
 '
 
-test_expect_failure 'diff.submodule=log' '
+test_expect_success 'diff.submodule=log' '
test_config diff.submodule log 
run_test $SECOND first-submodule
 '
 
-test_expect_failure 'diff.submodule=log with extra file' '
+test_expect_success 'diff.submodule=log with extra file' '
test_config diff.submodule log 
run_test $THIRD second-submodule
 '
-- 
2.0.5

--
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 1/2] t4255: test am submodule with diff.submodule

[Doug Kelly]

git am will break when using diff.submodule=log; add some test cases
to illustrate this breakage as simply as possible.  There are
currently two ways this can fail:

* With errors (unrecognized input), if only change
* Silently (no submodule change), if other files change

Test for both conditions and ensure without diff.submodule this works.

Signed-off-by: Doug Kelly dougk@gmail.com
Thanks-to: Eric Sunshine sunsh...@sunshineco.com
Thanks-to: Junio C Hamano gits...@pobox.com
---
Updated with Eric Sunshine's comments and changes to reduce complexity,
and also changed to include Junio's suggestions for using test_config,
test_unconfig, and test_might_fail (since we don't know if a previous
am failed or not -- we always want to clean up first).

 t/t4255-am-submodule.sh | 72 +
 1 file changed, 72 insertions(+)

diff --git a/t/t4255-am-submodule.sh b/t/t4255-am-submodule.sh
index 8bde7db..523accf 100755
--- a/t/t4255-am-submodule.sh
+++ b/t/t4255-am-submodule.sh
@@ -18,4 +18,76 @@ am_3way () {
 KNOWN_FAILURE_NOFF_MERGE_ATTEMPTS_TO_MERGE_REMOVED_SUBMODULE_FILES=1
 test_submodule_switch am_3way
 
+test_expect_success 'setup diff.submodule' '
+   test_commit one 
+   INITIAL=$(git rev-parse HEAD) 
+
+   git init submodule 
+   (
+   cd submodule 
+   test_commit two 
+   git rev-parse HEAD ../initial-submodule
+   ) 
+   git submodule add ./submodule 
+   git commit -m first 
+
+   (
+   cd submodule 
+   test_commit three 
+   git rev-parse HEAD ../first-submodule
+   ) 
+   git add submodule 
+   test_tick 
+   git commit -m second 
+   SECOND=$(git rev-parse HEAD) 
+
+   (
+   cd submodule 
+   git mv two.t four.t 
+   test_tick 
+   git commit -m second submodule 
+   git rev-parse HEAD ../second-submodule
+   ) 
+   test_commit four 
+   git add submodule 
+   git commit --amend --no-edit 
+   THIRD=$(git rev-parse HEAD) 
+   git submodule update --init
+'
+
+run_test() {
+   START_COMMIT=$1 
+   EXPECT=$2 
+   test_might_fail git am --abort 
+   git reset --hard $START_COMMIT 
+   rm -f *.patch 
+   git format-patch -1 
+   git reset --hard $START_COMMIT^ 
+   git submodule update 
+   git am *.patch 
+   git submodule update 
+   git -C submodule rev-parse HEAD actual 
+   test_cmp $EXPECT actual
+}
+
+test_expect_success 'diff.submodule unset' '
+   test_unconfig diff.submodule 
+   run_test $SECOND first-submodule
+'
+
+test_expect_success 'diff.submodule unset with extra file' '
+   test_unconfig diff.submodule 
+   run_test $THIRD second-submodule
+'
+
+test_expect_failure 'diff.submodule=log' '
+   test_config diff.submodule log 
+   run_test $SECOND first-submodule
+'
+
+test_expect_failure 'diff.submodule=log with extra file' '
+   test_config diff.submodule log 
+   run_test $THIRD second-submodule
+'
+
 test_done
-- 
2.0.5

--
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] t4255: test am submodule with diff.submodule

[Doug Kelly]

On Mon, Dec 29, 2014 at 9:42 AM, Junio C Hamano gits...@pobox.com wrote:
 Eric Sunshine sunsh...@sunshineco.com writes:

 +   (git am --abort || true) 

 Why (x || y)?  Is 'x' so unreliable that we do not know how should exit?
 Should this be test_must_fail git am --abort?

Updated to test_might_fail -- we don't know if a merge is in progress or not.
We still need to clean up, but disregard failure if a merge isn't in progress.

 +   (cd submodule  git rev-parse HEAD ../actual) 

 git -C submodule rev-parse HEAD actual perhaps?

Seems sane to me.

 +test_expect_success 'diff.submodule unset' '
 +   (git config --unset diff.submodule || true) 

 I think test_config and test_unconfig were invented for things like
 this (same for all the other use of git config).
Yep, much nicer. :) Also updated to test_commit as suggested by Eric.

Thanks!

--Doug
--
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] t4255: test am submodule with diff.submodule

[Doug Kelly]

On Sat, Dec 27, 2014 at 6:37 PM, Eric Sunshine sunsh...@sunshineco.com wrote:

 On Fri, Dec 26, 2014 at 6:11 PM, Doug Kelly dougk@gmail.com wrote:
  git am will break when using diff.submodule=log; add some test cases
  to illustrate this breakage as simply as possible.  There are
  currently two ways this can fail:
 
  * With errors (unrecognized input), if only change
  * Silently (no submodule change), if other files change
 
  Test for both conditions and ensure without diff.submodule this works.
 
  Signed-off-by: Doug Kelly dougk@gmail.com
  ---
  diff --git a/t/t4255-am-submodule.sh b/t/t4255-am-submodule.sh
  index 8bde7db..d9a1d79 100755
  --- a/t/t4255-am-submodule.sh
  +++ b/t/t4255-am-submodule.sh
  @@ -18,4 +18,87 @@ am_3way () {
   KNOWN_FAILURE_NOFF_MERGE_ATTEMPTS_TO_MERGE_REMOVED_SUBMODULE_FILES=1
   test_submodule_switch am_3way
 
  +test_expect_success 'setup diff.submodule' '

 Since the tests are actually expected to fail at this point (before
 you've fixed the problem), use test_expect_failure. The follow-up
 patch, which fixes the problem, should flip them to
 test_expect_success.

Okay, that's easy enough to do.


  +   echo one one 
  +   git add one 
  +   test_tick 
  +   git commit -m initial 

 Rather than performing these steps manually (here and below), perhaps
 test_commit would be suitable and more succinct.

  +   git rev-parse HEAD initial 

 Other scripts in the test suite don't bother with this indirection.
 Instead, they assign the variable here, then reference it in
 subsequent tests (and no need to redirect to a file).

 INITIAL=$(git rev-parse HEAD) 


Both good comments; wasn't sure if that'd work.  But easy to do!

  +
  +   git init submodule 
  +   (cd submodule 
  +   echo two two 
  +   git add two 
  +   test_tick 
  +   git commit -m initial submodule 
  +   git rev-parse HEAD ../initial-submodule) 

 Style: Format the subshell like this:

 (
 ...commands...
 ) 

Yeah, I was unsure on this style (docs were unclear), but that's easy to follow.


  +   git submodule add ./submodule 
  +   test_tick 
  +   git commit -m first 
  +   git rev-parse HEAD first 

 Is file 'first' ever used anywhere?

Nope; somewhat vestigial code (that could probably be cleaned out --
and should).
Originally, I was testing the first commit, and then I realized that
the second commit
updating the submodule introduces the failure I was looking for.


  +   (cd submodule 
  +   echo three three 
  +   git add three 
  +   test_tick 
  +   git commit -m first submodule 
  +   git rev-parse HEAD ../first-submodule) 
  +   git add submodule 
  +   test_tick 
  +   git commit -m second 
  +   git rev-parse HEAD second 
  +
  +   (cd submodule 
  +   git mv two four 
  +   test_tick 
  +   git commit -m second submodule 
  +   git rev-parse HEAD ../second-submodule) 
  +   git add submodule 
  +   echo four four 
  +   git add four 
  +   test_tick 
  +   git commit -m third 
  +   git rev-parse HEAD third 
  +   git submodule update --init
  +'
  +
  +INITIAL=$(cat initial)
  +SECOND=$(cat second)
  +THIRD=$(cat third)

 No need for this extra level of indirection. See above.

  +run_test() {
  +   START_COMMIT=$1
  +   EXPECT=$2

 Although it's not specifically wrong here, someone adding code above
 these two lines later on may not notice the broken -chain, so it
 would be a good idea to keep the -chain intact.

OK, easy enough.


  +   (git am --abort || true) 
  +   git reset --hard $START_COMMIT 
  +   rm -f *.patch 
  +   git format-patch -1 
  +   git reset --hard $START_COMMIT^ 
  +   git submodule update 
  +   git am *.patch 
  +   git submodule update 
  +   (cd submodule  git rev-parse HEAD ../actual) 
  +   test_cmp $EXPECT actual
  +}
  +
  +test_expect_success 'diff.submodule unset' '
  +   (git config --unset diff.submodule || true) 
  +   run_test $SECOND 'first-submodule'

 Note that you're already inside a single-quoted string here, so
 'first-submodule' is not quite doing what you expect. Double quotes
 would be more appropriate. Or, better, drop the quoting of
 first-submodule altogether since it's unnecessary.

Yep. Good note.


  +'
  +
  +test_expect_success 'diff.submodule unset with extra file' '
  +   (git config --unset diff.submodule || true) 
  +   run_test $THIRD 'second-submodule'
  +'
  +
  +test_expect_success 'diff.submodule=log' '
  +   git config diff.submodule log 
  +   run_test $SECOND 'first-submodule'
  +'
  +
  +test_expect_success 'diff.submodule=log with extra file' '
  +   git config diff.submodule log 
  +   run_test $THIRD 'second-submodule'
  +'
  +
   test_done
  --
  2.0.5

[PATCH v2 1/2] t4255: test am submodule with diff.submodule

[Doug Kelly]

git am will break when using diff.submodule=log; add some test cases
to illustrate this breakage as simply as possible.  There are
currently two ways this can fail:

* With errors (unrecognized input), if only change
* Silently (no submodule change), if other files change

Test for both conditions and ensure without diff.submodule this works.

Signed-off-by: Doug Kelly dougk@gmail.com
---
 t/t4255-am-submodule.sh | 84 +
 1 file changed, 84 insertions(+)

diff --git a/t/t4255-am-submodule.sh b/t/t4255-am-submodule.sh
index 8bde7db..a2dc083 100755
--- a/t/t4255-am-submodule.sh
+++ b/t/t4255-am-submodule.sh
@@ -18,4 +18,88 @@ am_3way () {
 KNOWN_FAILURE_NOFF_MERGE_ATTEMPTS_TO_MERGE_REMOVED_SUBMODULE_FILES=1
 test_submodule_switch am_3way
 
+test_expect_success 'setup diff.submodule' '
+   echo one one 
+   git add one 
+   test_tick 
+   git commit -m initial 
+   INITIAL=$(git rev-parse HEAD) 
+
+   git init submodule 
+   (
+   cd submodule 
+   echo two two 
+   git add two 
+   test_tick 
+   git commit -m initial submodule 
+   git rev-parse HEAD ../initial-submodule
+   ) 
+   git submodule add ./submodule 
+   test_tick 
+   git commit -m first 
+
+   (
+   cd submodule 
+   echo three three 
+   git add three 
+   test_tick 
+   git commit -m first submodule 
+   git rev-parse HEAD ../first-submodule
+   ) 
+   git add submodule 
+   test_tick 
+   git commit -m second 
+   SECOND=$(git rev-parse HEAD) 
+
+   (
+   cd submodule 
+   git mv two four 
+   test_tick 
+   git commit -m second submodule 
+   git rev-parse HEAD ../second-submodule
+   ) 
+   git add submodule 
+   echo four four 
+   git add four 
+   test_tick 
+   git commit -m third 
+   THIRD=$(git rev-parse HEAD) 
+   git submodule update --init
+'
+
+run_test() {
+   START_COMMIT=$1 
+   EXPECT=$2 
+   (git am --abort || true) 
+   git reset --hard $START_COMMIT 
+   rm -f *.patch 
+   git format-patch -1 
+   git reset --hard $START_COMMIT^ 
+   git submodule update 
+   git am *.patch 
+   git submodule update 
+   (cd submodule  git rev-parse HEAD ../actual) 
+   test_cmp $EXPECT actual
+}
+
+test_expect_success 'diff.submodule unset' '
+   (git config --unset diff.submodule || true) 
+   run_test $SECOND first-submodule
+'
+
+test_expect_success 'diff.submodule unset with extra file' '
+   (git config --unset diff.submodule || true) 
+   run_test $THIRD second-submodule
+'
+
+test_expect_failure 'diff.submodule=log' '
+   git config diff.submodule log 
+   run_test $SECOND first-submodule
+'
+
+test_expect_failure 'diff.submodule=log with extra file' '
+   git config diff.submodule log 
+   run_test $THIRD second-submodule
+'
+
 test_done
-- 
2.0.5

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

[PATCH v2 2/2] format-patch: ignore diff.submodule setting

[Doug Kelly]

diff.submodule when set to log produces output which git-am cannot
handle. Ignore this setting when generating patch output.

Signed-off-by: Doug Kelly dougk@gmail.com
---
 builtin/log.c   | 2 +-
 t/t4255-am-submodule.sh | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index 734aab3..cb14db4 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -705,7 +705,7 @@ static int git_format_config(const char *var, const char 
*value, void *cb)
return 0;
}
if (!strcmp(var, diff.color) || !strcmp(var, color.diff) ||
-   !strcmp(var, color.ui)) {
+   !strcmp(var, color.ui) || !strcmp(var, diff.submodule)) {
return 0;
}
if (!strcmp(var, format.numbered)) {
diff --git a/t/t4255-am-submodule.sh b/t/t4255-am-submodule.sh
index a2dc083..b7ec0f1 100755
--- a/t/t4255-am-submodule.sh
+++ b/t/t4255-am-submodule.sh
@@ -92,12 +92,12 @@ test_expect_success 'diff.submodule unset with extra file' '
run_test $THIRD second-submodule
 '
 
-test_expect_failure 'diff.submodule=log' '
+test_expect_success 'diff.submodule=log' '
git config diff.submodule log 
run_test $SECOND first-submodule
 '
 
-test_expect_failure 'diff.submodule=log with extra file' '
+test_expect_success 'diff.submodule=log with extra file' '
git config diff.submodule log 
run_test $THIRD second-submodule
 '
-- 
2.0.5

--
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 0/2] Fix issue with format-patch and diff.submodule

[Doug Kelly]

A colleague found an issue that when using diff.submodule=log in his
.gitconfig, format-patch would use the log format for submodule changes,
which would be ignored or error out when processed by git-am.
format-patch now ignores the diff.submodule option and a testcase for
this specific issue now exists.

Since this seems like a bug in current versions, I have based and
tested this on the maint branch, but there's no reason it shouldn't
apply cleanly to master as well.

Apologies for any rawness to the first round of this change.
Long time listener; first time caller. Any feedback is appreciated.

Doug Kelly (2):
  t4255: test am submodule with diff.submodule
  format-patch: ignore diff.submodule setting

 builtin/log.c   |  2 +-
 t/t4255-am-submodule.sh | 83 +
 2 files changed, 84 insertions(+), 1 deletion(-)

-- 
2.0.5

--
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/2] format-patch: ignore diff.submodule setting

[Doug Kelly]

diff.submodule when set to log produces output which git-am cannot
handle. Ignore this setting when generating patch output.

Signed-off-by: Doug Kelly dougk@gmail.com
---
 builtin/log.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/log.c b/builtin/log.c
index 734aab3..cb14db4 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -705,7 +705,7 @@ static int git_format_config(const char *var, const char 
*value, void *cb)
return 0;
}
if (!strcmp(var, diff.color) || !strcmp(var, color.diff) ||
-   !strcmp(var, color.ui)) {
+   !strcmp(var, color.ui) || !strcmp(var, diff.submodule)) {
return 0;
}
if (!strcmp(var, format.numbered)) {
-- 
2.0.5

--
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/2] t4255: test am submodule with diff.submodule

[Doug Kelly]

git am will break when using diff.submodule=log; add some test cases
to illustrate this breakage as simply as possible.  There are
currently two ways this can fail:

* With errors (unrecognized input), if only change
* Silently (no submodule change), if other files change

Test for both conditions and ensure without diff.submodule this works.

Signed-off-by: Doug Kelly dougk@gmail.com
---
 t/t4255-am-submodule.sh | 83 +
 1 file changed, 83 insertions(+)

diff --git a/t/t4255-am-submodule.sh b/t/t4255-am-submodule.sh
index 8bde7db..d9a1d79 100755
--- a/t/t4255-am-submodule.sh
+++ b/t/t4255-am-submodule.sh
@@ -18,4 +18,87 @@ am_3way () {
 KNOWN_FAILURE_NOFF_MERGE_ATTEMPTS_TO_MERGE_REMOVED_SUBMODULE_FILES=1
 test_submodule_switch am_3way
 
+test_expect_success 'setup diff.submodule' '
+   echo one one 
+   git add one 
+   test_tick 
+   git commit -m initial 
+   git rev-parse HEAD initial 
+
+   git init submodule 
+   (cd submodule 
+   echo two two 
+   git add two 
+   test_tick 
+   git commit -m initial submodule 
+   git rev-parse HEAD ../initial-submodule) 
+   git submodule add ./submodule 
+   test_tick 
+   git commit -m first 
+   git rev-parse HEAD first 
+
+   (cd submodule 
+   echo three three 
+   git add three 
+   test_tick 
+   git commit -m first submodule 
+   git rev-parse HEAD ../first-submodule) 
+   git add submodule 
+   test_tick 
+   git commit -m second 
+   git rev-parse HEAD second 
+
+   (cd submodule 
+   git mv two four 
+   test_tick 
+   git commit -m second submodule 
+   git rev-parse HEAD ../second-submodule) 
+   git add submodule 
+   echo four four 
+   git add four 
+   test_tick 
+   git commit -m third 
+   git rev-parse HEAD third 
+   git submodule update --init
+'
+
+INITIAL=$(cat initial)
+SECOND=$(cat second)
+THIRD=$(cat third)
+
+run_test() {
+   START_COMMIT=$1
+   EXPECT=$2
+   (git am --abort || true) 
+   git reset --hard $START_COMMIT 
+   rm -f *.patch 
+   git format-patch -1 
+   git reset --hard $START_COMMIT^ 
+   git submodule update 
+   git am *.patch 
+   git submodule update 
+   (cd submodule  git rev-parse HEAD ../actual) 
+   test_cmp $EXPECT actual
+}
+
+test_expect_success 'diff.submodule unset' '
+   (git config --unset diff.submodule || true) 
+   run_test $SECOND 'first-submodule'
+'
+
+test_expect_success 'diff.submodule unset with extra file' '
+   (git config --unset diff.submodule || true) 
+   run_test $THIRD 'second-submodule'
+'
+
+test_expect_success 'diff.submodule=log' '
+   git config diff.submodule log 
+   run_test $SECOND 'first-submodule'
+'
+
+test_expect_success 'diff.submodule=log with extra file' '
+   git config diff.submodule log 
+   run_test $THIRD 'second-submodule'
+'
+
 test_done
-- 
2.0.5

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