Re: [PATCH] repack.c: rename and unlink pack file if it exists
On Wed, Feb 05, 2014 at 01:08:36PM -0800, Junio C Hamano wrote: > Jeff King writes: > > > ... So the fact that this > > bug exists doesn't really produce any user-visible behavior, and > > hopefully post-release we would drop the code entirely, and the test > > would have no reason to exist. > > Heh, thanks, and I agree with the reasoning for the longer-term > direction. Perhaps I can/should hold it off that minimal fix-up > patch from -rc3, then? I am on the fence but I already started my > today's integration cycle _with_ the fix merged to 'master', so... I would say leave the fix for -rc3, but not worry about the test. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] repack.c: rename and unlink pack file if it exists
Jeff King writes: > ... So the fact that this > bug exists doesn't really produce any user-visible behavior, and > hopefully post-release we would drop the code entirely, and the test > would have no reason to exist. Heh, thanks, and I agree with the reasoning for the longer-term direction. Perhaps I can/should hold it off that minimal fix-up patch from -rc3, then? I am on the fence but I already started my today's integration cycle _with_ the fix merged to 'master', so... -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] repack.c: rename and unlink pack file if it exists
On 2014-02-05 21.31, Junio C Hamano wrote: > Jeff King writes: > >> The minimal fix you posted below does make sense to me as a stopgap, and >> we can look into dropping the code entirely during the next cycle. It >> would be nice to have a test to cover this case, though. > > Sounds sensible. Run "repack -a -d" once, and then another while > forcing it to be single threaded, or something? I can put a test case on my todo list, and thanks for the minimal patch. -- 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] repack.c: rename and unlink pack file if it exists
On Wed, Feb 05, 2014 at 12:57:14PM -0800, Junio C Hamano wrote: > > ...does not seem to fail, and it does not seem to leave any cruft in > > place. So maybe I am misunderstanding the thing the patch is meant to > > fix. Is it that we simply do not replace the pack in this instance? > > Yes. Not just the command finishing OK, but the packfile left by > the first repack needs to be left intact. [...] Thanks for the explanation. Having looked at this now, I'm thinking a test may not be worth the trouble. Due to 1190a1ac, we effectively don't care whether we get the old pack or the new one. So the fact that this bug exists doesn't really produce any user-visible behavior, and hopefully post-release we would drop the code entirely, and the test would have no reason to exist. > We could use test-chmtime to reset the timestamp of the packfile > generated by the first repack to somewhere reasonably old and then > rerun the repack to see that it is a different file, which may be > more portable than inspecting the inum of the packfile. Yeah, I think that would work. But it sounds like we also need a filesystem in which rename() does not overwrite. So the test would not be portable. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] repack.c: rename and unlink pack file if it exists
Jeff King writes:
> On Wed, Feb 05, 2014 at 12:31:34PM -0800, Junio C Hamano wrote:
>
>> Jeff King writes:
>>
>> > The minimal fix you posted below does make sense to me as a stopgap, and
>> > we can look into dropping the code entirely during the next cycle. It
>> > would be nice to have a test to cover this case, though.
>>
>> Sounds sensible. Run "repack -a -d" once, and then another while
>> forcing it to be single threaded, or something?
>
> Certainly that's the way to trigger the code, but doing this:
>
> diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh
> index b45bd1e..6647ba1 100755
> --- a/t/t7700-repack.sh
> +++ b/t/t7700-repack.sh
> @@ -162,7 +162,12 @@ test_expect_success 'objects made unreachable by grafts
> only are kept' '
> git reflog expire --expire=$test_tick --expire-unreachable=$test_tick
> --all &&
> git repack -a -d &&
> git cat-file -t $H1
> - '
> +'
> +
> +test_expect_success 'repack can handle generating the same pack again' '
> + git -c pack.threads=1 repack -ad &&
> + git -c pack.threads=1 repack -ad
> +'
>
> test_done
>
>
> ...does not seem to fail, and it does not seem to leave any cruft in
> place. So maybe I am misunderstanding the thing the patch is meant to
> fix. Is it that we simply do not replace the pack in this instance?
Yes. Not just the command finishing OK, but the packfile left by
the first repack needs to be left intact. One bug was that after
learning that a new packfile needs to be installed, the command
did not check existing .git/objects/pack/pack-.{pack,idx}, but
checked .git/objects/pack/.{pack,idx}, deciding that there is
nothing that needs to be saved by renaming with s|pack-|old-|. This
would have caused it to rename the new packfile left by pack-object
at .git/objects/pack/.tmp-$pid-pack-.{pack,idx} directly to the
final .git/objects/pack/pack-.{pack,idx}, which would work only
on filesystems that allow renaming over an existing file.
Another bug fixed by Torsten was in the codepath to roll the rename
back from .git/objects/pack/old-.{pack,idx} to the original (the
command tried to rename .git/objects/pack/old-pack-.{pack,idx}
which would not have been the ones it renamed), but because of the
earlier bug, it would never have triggered in the first place.
> I guess we would have to generate a pack with the identical set of
> objects, then, but somehow different in its pack parameters (perhaps
> turning off deltas?).
Unfortunately, that would trigger different codepath on v1.9-rc0 and
later with 1190a1ac (pack-objects: name pack files after trailer
hash, 2013-12-05), as it is likely not to name the packfiles the
same.
We could use test-chmtime to reset the timestamp of the packfile
generated by the first repack to somewhere reasonably old and then
rerun the repack to see that it is a different file, which may be
more portable than inspecting the inum of the packfile.
--
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] repack.c: rename and unlink pack file if it exists
On Wed, Feb 05, 2014 at 03:37:40PM -0500, Jeff King wrote:
> > Sounds sensible. Run "repack -a -d" once, and then another while
> > forcing it to be single threaded, or something?
>
> Certainly that's the way to trigger the code, but doing this:
> [...]
> ...does not seem to fail, and it does not seem to leave any cruft in
> place. So maybe I am misunderstanding the thing the patch is meant to
> fix. Is it that we simply do not replace the pack in this instance?
>
> I guess we would have to generate a pack with the identical set of
> objects, then, but somehow different in its pack parameters (perhaps
> turning off deltas?).
Here's a more robust test that actually checks the pack contents:
diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh
index b45bd1e..c18a318 100755
--- a/t/t7700-repack.sh
+++ b/t/t7700-repack.sh
@@ -164,5 +164,17 @@ test_expect_success 'objects made unreachable by grafts
only are kept' '
git cat-file -t $H1
'
+test_expect_success 'repack can handle generating the same pack again' '
+ show_deltas() {
+ git rev-list --objects --all --reflog |
+ git cat-file --batch-check="%(objectname) %(deltabase) %(rest)"
+ }
+ git -c pack.threads=1 repack -adf --window=0 &&
+ show_deltas >no-deltas &&
+ git -c pack.threads=1 repack -adf --window=10 &&
+ show_deltas >deltas &&
+ ! test_cmp no-deltas deltas
+'
+
test_done
which _also_ does not fail. And then I realized it is because of
1190a1ac, which gives these two separate names.
So I am not sure if it is even possible to trigger the bug in a
meaningful way at this point.
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] repack.c: rename and unlink pack file if it exists
On Wed, Feb 05, 2014 at 12:31:34PM -0800, Junio C Hamano wrote: > Jeff King writes: > > > The minimal fix you posted below does make sense to me as a stopgap, and > > we can look into dropping the code entirely during the next cycle. It > > would be nice to have a test to cover this case, though. > > Sounds sensible. Run "repack -a -d" once, and then another while > forcing it to be single threaded, or something? Certainly that's the way to trigger the code, but doing this: diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh index b45bd1e..6647ba1 100755 --- a/t/t7700-repack.sh +++ b/t/t7700-repack.sh @@ -162,7 +162,12 @@ test_expect_success 'objects made unreachable by grafts only are kept' ' git reflog expire --expire=$test_tick --expire-unreachable=$test_tick --all && git repack -a -d && git cat-file -t $H1 - ' +' + +test_expect_success 'repack can handle generating the same pack again' ' + git -c pack.threads=1 repack -ad && + git -c pack.threads=1 repack -ad +' test_done ...does not seem to fail, and it does not seem to leave any cruft in place. So maybe I am misunderstanding the thing the patch is meant to fix. Is it that we simply do not replace the pack in this instance? I guess we would have to generate a pack with the identical set of objects, then, but somehow different in its pack parameters (perhaps turning off deltas?). -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] repack.c: rename and unlink pack file if it exists
Jeff King writes:
> On Tue, Feb 04, 2014 at 03:40:15PM -0800, Junio C Hamano wrote:
>
>> * Somehow this came to my private mailbox without Cc to list, so I
>>am forwarding it.
>>
>>I think with 1190a1ac (pack-objects: name pack files after
>>trailer hash, 2013-12-05), repacking the same set of objects may
>>have less chance of producing colliding names, especially if you
>>are on a box with more than one core, but it still would be a
>>good idea to get this part right in the upcoming release.
>
> Actually, since 1190a1ac, if you have repacked and gotten the same pack
> name, then you do not have to do any rename dance at all; you can throw
> away what you just generated because you know that it is byte-for-byte
> identical.
>
> You could collide with a pack created by an older version of git that
> used the original scheme, but that is quite unlikely (on the order of
> 2^-160).
Yes, so in that sense this is not so urgent, but I'm tempted to
split the original patch into two and merge only the first one to
'master' before -rc3 (see below). The renaming of the variables
added enough noise to cause me fail to spot a change mixed within.
-- >8 --
From: Torsten Bögershausen
Date: Sun, 2 Feb 2014 16:09:56 +0100
When a repo was fully repacked, and is repacked again, we may run
into the situation that "new" packfiles have the same name as
already existing ones (traditionally packfiles have been named after
the list of names of objects in them, so repacking all the objects
in a single pack would have produced a packfile with the same name).
The logic is to rename the existing ones into filename like
"old-XXX", create the new ones and then remove the "old-" ones.
When something went wrong in the middle, this sequence is rolled
back by renaming the "old-" files back.
The renaming into "old-" did not work as intended, because
file_exists() was done on "XXX", not "pack-XXX". Also when rolling
back the change, the code tried to rename "old-pack-XXX" but the
saved ones are named "old-XXX", so this couldn't have worked.
Signed-off-by: Torsten Bögershausen
Signed-off-by: Junio C Hamano
---
builtin/repack.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/builtin/repack.c b/builtin/repack.c
index bca7710..fe31577 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -260,7 +260,7 @@ int cmd_repack(int argc, const char **argv, const char
*prefix)
for_each_string_list_item(item, &names) {
for (ext = 0; ext < 2; ext++) {
char *fname, *fname_old;
- fname = mkpathdup("%s/%s%s", packdir,
+ fname = mkpathdup("%s/pack-%s%s", packdir,
item->string, exts[ext]);
if (!file_exists(fname)) {
free(fname);
@@ -337,7 +337,7 @@ int cmd_repack(int argc, const char **argv, const char
*prefix)
for_each_string_list_item(item, &names) {
for (ext = 0; ext < 2; ext++) {
char *fname;
- fname = mkpath("%s/old-pack-%s%s",
+ fname = mkpath("%s/old-%s%s",
packdir,
item->string,
exts[ext]);
--
1.9-rc2-217-g24a8b2e
--
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] repack.c: rename and unlink pack file if it exists
Jeff King writes: > The minimal fix you posted below does make sense to me as a stopgap, and > we can look into dropping the code entirely during the next cycle. It > would be nice to have a test to cover this case, though. Sounds sensible. Run "repack -a -d" once, and then another while forcing it to be single threaded, or something? -- 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] repack.c: rename and unlink pack file if it exists
On Wed, Feb 05, 2014 at 12:06:41PM -0800, Junio C Hamano wrote: > > Actually, since 1190a1ac, if you have repacked and gotten the same pack > > name, then you do not have to do any rename dance at all; you can throw > > away what you just generated because you know that it is byte-for-byte > > identical. > > > > You could collide with a pack created by an older version of git that > > used the original scheme, but that is quite unlikely (on the order of > > 2^-160). > > Yes, so in that sense this is not so urgent, but I'm tempted to > split the original patch into two and merge only the first one to > 'master' before -rc3 (see below). The renaming of the variables > added enough noise to cause me fail to spot a change mixed within. That sounds very sensible. The only reason I did not follow-up 1190a1ac immediately with a patch to drop the rename code was that it is a sensitive area, and I wanted to be very sure there would be no other fallouts. And then of course I didn't get around to it yet. But following the same logic, trying to do it during -rc would be a terrible idea. :) The minimal fix you posted below does make sense to me as a stopgap, and we can look into dropping the code entirely during the next cycle. It would be nice to have a test to cover this case, though. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] repack.c: rename and unlink pack file if it exists
... and this is the other half that is supposed to be only about
renaming variables.
-- >8 --
From: Torsten Bögershausen
Date: Sun, 2 Feb 2014 16:09:56 +0100
Subject: [PATCH] repack.c: rename a few variables
Rename the variables to match what they are used for: fname for the
final name of the new packfile, fname_old for the name of the
existing one, and fname_tmp for the temporary name pack-objects
created the new packfile in.
Signed-off-by: Torsten Bögershausen
Signed-off-by: Junio C Hamano
---
builtin/repack.c | 22 +++---
1 file changed, 11 insertions(+), 11 deletions(-)
diff --git a/builtin/repack.c b/builtin/repack.c
index fe31577..3b01353 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -316,33 +316,33 @@ int cmd_repack(int argc, const char **argv, const char
*prefix)
/* Now the ones with the same name are out of the way... */
for_each_string_list_item(item, &names) {
for (ext = 0; ext < 2; ext++) {
- char *fname, *fname_old;
+ char *fname, *fname_tmp;
struct stat statbuffer;
fname = mkpathdup("%s/pack-%s%s",
packdir, item->string, exts[ext]);
- fname_old = mkpathdup("%s-%s%s",
+ fname_tmp = mkpathdup("%s-%s%s",
packtmp, item->string, exts[ext]);
- if (!stat(fname_old, &statbuffer)) {
+ if (!stat(fname_tmp, &statbuffer)) {
statbuffer.st_mode &= ~(S_IWUSR | S_IWGRP |
S_IWOTH);
- chmod(fname_old, statbuffer.st_mode);
+ chmod(fname_tmp, statbuffer.st_mode);
}
- if (rename(fname_old, fname))
- die_errno(_("renaming '%s' failed"), fname_old);
+ if (rename(fname_tmp, fname))
+ die_errno(_("renaming '%s' into '%s' failed"),
fname_tmp, fname);
free(fname);
- free(fname_old);
+ free(fname_tmp);
}
}
/* Remove the "old-" files */
for_each_string_list_item(item, &names) {
for (ext = 0; ext < 2; ext++) {
- char *fname;
- fname = mkpath("%s/old-%s%s",
+ char *fname_old;
+ fname_old = mkpath("%s/old-%s%s",
packdir,
item->string,
exts[ext]);
- if (remove_path(fname))
- warning(_("removing '%s' failed"), fname);
+ if (remove_path(fname_old))
+ warning(_("removing '%s' failed"), fname_old);
}
}
--
1.9-rc2-217-g24a8b2e
--
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] repack.c: rename and unlink pack file if it exists
On 2014-02-05 02.16, Jeff King wrote: > On Tue, Feb 04, 2014 at 03:40:15PM -0800, Junio C Hamano wrote: > >> * Somehow this came to my private mailbox without Cc to list, so I >>am forwarding it. >> >>I think with 1190a1ac (pack-objects: name pack files after >>trailer hash, 2013-12-05), repacking the same set of objects may >>have less chance of producing colliding names, especially if you >>are on a box with more than one core, but it still would be a >>good idea to get this part right in the upcoming release. > > Actually, since 1190a1ac, if you have repacked and gotten the same pack > name, then you do not have to do any rename dance at all; you can throw > away what you just generated because you know that it is byte-for-byte > identical. > > You could collide with a pack created by an older version of git that > used the original scheme, but that is quite unlikely (on the order of > 2^-160). > > -Peff OK, I messed up the email so it went only to Junios mailbox. Sorry for confusion. Jochen, it could be good if you could test this version of the patch (I couldn't reproduce the problem here) /Torsten -- 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] repack.c: rename and unlink pack file if it exists
On Tue, Feb 04, 2014 at 03:40:15PM -0800, Junio C Hamano wrote: > * Somehow this came to my private mailbox without Cc to list, so I >am forwarding it. > >I think with 1190a1ac (pack-objects: name pack files after >trailer hash, 2013-12-05), repacking the same set of objects may >have less chance of producing colliding names, especially if you >are on a box with more than one core, but it still would be a >good idea to get this part right in the upcoming release. Actually, since 1190a1ac, if you have repacked and gotten the same pack name, then you do not have to do any rename dance at all; you can throw away what you just generated because you know that it is byte-for-byte identical. You could collide with a pack created by an older version of git that used the original scheme, but that is quite unlikely (on the order of 2^-160). -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] repack.c: rename and unlink pack file if it exists
Junio C Hamano writes:
> This comment in builtin/repack.c:
> ...
Oops; there was supposed to be these lines before anythning else:
From: Torsten Bögershausen
Date: Sun Feb 2 16:09:56 2014 +0100
> First see if there are packs of the same name and if so
> if we can move them out of the way (this can happen if we
> repacked immediately after packing fully).
>
> When a repo was fully repacked, and is repacked again, we may run
> into the situation that "new" packfiles have the same name as
> already existing ones (traditionally packfiles have been named after
> the list of names of objects in them, so repacking all the objects
> in a single pack would have produced a packfile with the same name).
>
> The logic is to rename the existing ones into filename like
> "old-XXX", create the new ones and then remove the "old-" ones.
> When something went wrong in the middle, this sequence is rolled
> back by renaming the "old-" files back.
>
> The renaming into "old-" did not work as designed, because
> file_exists() was done on the wrong file name. This could give
> problems for a repo on a network share under Windows, as reported by
> Jochen Haag .
>
> Formulate the filenames objects/pack/pack-.{pack,idx} correctly
> (the code originally lacked "pack-" prefix when checking if the file
> exists).
>
> Also rename the variables to match what they are used for:
> fname for the final name of the new packfile, fname_old for the name
> of the existing one, and fname_tmp for the temporary name pack-objects
> created the new packfile in.
>
> Signed-off-by: Torsten Bögershausen
> Signed-off-by: Junio C Hamano
> ---
>
> * Somehow this came to my private mailbox without Cc to list, so I
>am forwarding it.
>
>I think with 1190a1ac (pack-objects: name pack files after
>trailer hash, 2013-12-05), repacking the same set of objects may
>have less chance of producing colliding names, especially if you
>are on a box with more than one core, but it still would be a
>good idea to get this part right in the upcoming release.
>
>The bug in the first hunk will cause us not to find any existing
>file, even when there is a name clash. And then we try to
>immediately the final rename without any provision for rolling
>back. The other two hunks are pure noise renaming variables.
>
>I think the patch looks correct, but I'd appreciate a second set
>of eyeballs.
>
>Thanks.
>
> builtin/repack.c | 24
> 1 file changed, 12 insertions(+), 12 deletions(-)
>
> diff --git a/builtin/repack.c b/builtin/repack.c
> index bca7710..3b01353 100644
> --- a/builtin/repack.c
> +++ b/builtin/repack.c
> @@ -260,7 +260,7 @@ int cmd_repack(int argc, const char **argv, const char
> *prefix)
> for_each_string_list_item(item, &names) {
> for (ext = 0; ext < 2; ext++) {
> char *fname, *fname_old;
> - fname = mkpathdup("%s/%s%s", packdir,
> + fname = mkpathdup("%s/pack-%s%s", packdir,
> item->string, exts[ext]);
> if (!file_exists(fname)) {
> free(fname);
> @@ -316,33 +316,33 @@ int cmd_repack(int argc, const char **argv, const char
> *prefix)
> /* Now the ones with the same name are out of the way... */
> for_each_string_list_item(item, &names) {
> for (ext = 0; ext < 2; ext++) {
> - char *fname, *fname_old;
> + char *fname, *fname_tmp;
> struct stat statbuffer;
> fname = mkpathdup("%s/pack-%s%s",
> packdir, item->string, exts[ext]);
> - fname_old = mkpathdup("%s-%s%s",
> + fname_tmp = mkpathdup("%s-%s%s",
> packtmp, item->string, exts[ext]);
> - if (!stat(fname_old, &statbuffer)) {
> + if (!stat(fname_tmp, &statbuffer)) {
> statbuffer.st_mode &= ~(S_IWUSR | S_IWGRP |
> S_IWOTH);
> - chmod(fname_old, statbuffer.st_mode);
> + chmod(fname_tmp, statbuffer.st_mode);
> }
> - if (rename(fname_old, fname))
> - die_errno(_("renaming '%s' failed"), fname_old);
> + if (rename(fname_tmp, fname))
> + die_errno(_("renaming '%s' into '%s' failed"),
> fname_tmp, fname);
> free(fname);
> - free(fname_old);
> + free(fname_tmp);
> }
> }
>
> /* Remove the "old-" files */
> for_each_string_list_item(item, &names) {
> for (ext = 0; ext < 2; ext++) {
> - char *fname;
> - fname = mkpath
[PATCH] repack.c: rename and unlink pack file if it exists
This comment in builtin/repack.c:
First see if there are packs of the same name and if so
if we can move them out of the way (this can happen if we
repacked immediately after packing fully).
When a repo was fully repacked, and is repacked again, we may run
into the situation that "new" packfiles have the same name as
already existing ones (traditionally packfiles have been named after
the list of names of objects in them, so repacking all the objects
in a single pack would have produced a packfile with the same name).
The logic is to rename the existing ones into filename like
"old-XXX", create the new ones and then remove the "old-" ones.
When something went wrong in the middle, this sequence is rolled
back by renaming the "old-" files back.
The renaming into "old-" did not work as designed, because
file_exists() was done on the wrong file name. This could give
problems for a repo on a network share under Windows, as reported by
Jochen Haag .
Formulate the filenames objects/pack/pack-.{pack,idx} correctly
(the code originally lacked "pack-" prefix when checking if the file
exists).
Also rename the variables to match what they are used for:
fname for the final name of the new packfile, fname_old for the name
of the existing one, and fname_tmp for the temporary name pack-objects
created the new packfile in.
Signed-off-by: Torsten Bögershausen
Signed-off-by: Junio C Hamano
---
* Somehow this came to my private mailbox without Cc to list, so I
am forwarding it.
I think with 1190a1ac (pack-objects: name pack files after
trailer hash, 2013-12-05), repacking the same set of objects may
have less chance of producing colliding names, especially if you
are on a box with more than one core, but it still would be a
good idea to get this part right in the upcoming release.
The bug in the first hunk will cause us not to find any existing
file, even when there is a name clash. And then we try to
immediately the final rename without any provision for rolling
back. The other two hunks are pure noise renaming variables.
I think the patch looks correct, but I'd appreciate a second set
of eyeballs.
Thanks.
builtin/repack.c | 24
1 file changed, 12 insertions(+), 12 deletions(-)
diff --git a/builtin/repack.c b/builtin/repack.c
index bca7710..3b01353 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -260,7 +260,7 @@ int cmd_repack(int argc, const char **argv, const char
*prefix)
for_each_string_list_item(item, &names) {
for (ext = 0; ext < 2; ext++) {
char *fname, *fname_old;
- fname = mkpathdup("%s/%s%s", packdir,
+ fname = mkpathdup("%s/pack-%s%s", packdir,
item->string, exts[ext]);
if (!file_exists(fname)) {
free(fname);
@@ -316,33 +316,33 @@ int cmd_repack(int argc, const char **argv, const char
*prefix)
/* Now the ones with the same name are out of the way... */
for_each_string_list_item(item, &names) {
for (ext = 0; ext < 2; ext++) {
- char *fname, *fname_old;
+ char *fname, *fname_tmp;
struct stat statbuffer;
fname = mkpathdup("%s/pack-%s%s",
packdir, item->string, exts[ext]);
- fname_old = mkpathdup("%s-%s%s",
+ fname_tmp = mkpathdup("%s-%s%s",
packtmp, item->string, exts[ext]);
- if (!stat(fname_old, &statbuffer)) {
+ if (!stat(fname_tmp, &statbuffer)) {
statbuffer.st_mode &= ~(S_IWUSR | S_IWGRP |
S_IWOTH);
- chmod(fname_old, statbuffer.st_mode);
+ chmod(fname_tmp, statbuffer.st_mode);
}
- if (rename(fname_old, fname))
- die_errno(_("renaming '%s' failed"), fname_old);
+ if (rename(fname_tmp, fname))
+ die_errno(_("renaming '%s' into '%s' failed"),
fname_tmp, fname);
free(fname);
- free(fname_old);
+ free(fname_tmp);
}
}
/* Remove the "old-" files */
for_each_string_list_item(item, &names) {
for (ext = 0; ext < 2; ext++) {
- char *fname;
- fname = mkpath("%s/old-pack-%s%s",
+ char *fname_old;
+ fname_old = mkpath("%s/old-%s%s",
packdir,
item->string,
exts[ext]);
- i
