@matheusbsbs / matheus-bagliano / commits / fb7d3f32b2

Fork 0

Remove diff machinery dependency from read-cache

Exal Sibeaz pointed out that some git files are way too big, and that add_files_to_cache() brings in all the diff machinery to any git binary that needs the basic git SHA1 object operations from read-cache.c. Which is pretty much all of them. It's doubly silly, since add_files_to_cache() is only used by builtin programs (add, checkout and commit), so it's fairly easily fixed by just moving the thing to builtin-add.c, and avoiding the dependency entirely. I initially argued to Exal that it would probably be best to try to depend on smart compilers and linkers, but after spending some time trying to make -ffunction-sections work and giving up, I think Exal was right, and the fix is to just do some trivial cleanups like this. This trivial cleanup results in pretty stunning file size differences. The diff machinery really is mostly used by just the builtin programs, and you have things like these trivial before-and-after numbers: -rwxr-xr-x 1 torvalds torvalds 1727420 2010-01-21 10:53 git-hash-object -rwxrwxr-x 1 torvalds torvalds 940265 2010-01-21 11:16 git-hash-object Now, I'm not saying that 940kB is good either, but that's mostly all the debug information - you can see the real code with 'size': text data bss dec hex filename 418675 3920 127408 550003 86473 git-hash-object (before) 230650 2288 111728 344666 5425a git-hash-object (after) ie we have a nice 24% size reduction from this trivial cleanup. It's not just that one file either. I get: [torvalds@nehalem git]$ du -s /home/torvalds/libexec/git-core 45640 /home/torvalds/libexec/git-core (before) 33508 /home/torvalds/libexec/git-core (after) so we're talking 12MB of diskspace here. (Of course, stripping all the binaries brings the 33MB down to 9MB, so the whole debug information thing is still the bulk of it all, but that's a separate issue entirely) Now, I'm sure there are other things we should do, and changing our compiler flags from -O2 to -Os would bring the text size down by an additional almost 20%, but this thing Exal pointed out seems to be some good low-hanging fruit. Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Linus Torvalds committed Jan 21, 2010 at 11:37 UTC fb7d3f32b283a3847e6f151a06794abd14ffd81b

2 files changed +76 -78

builtin-add.c

+76

@@ -11,6 +11,7 @@

11 #include "run-command.h"

12 #include "parse-options.h"

13 #include "diff.h"

14 +#include "diffcore.h"

15 #include "revision.h"

17 static const char * const builtin_add_usage[] = {

@@ -20,6 +21,81 @@ static const char * const builtin_add_usage[] = {

21 static int patch_interactive, add_interactive, edit_interactive;

22 static int take_worktree_changes;

24 +struct update_callback_data

25 +{

26 + int flags;

27 + int add_errors;

28 +};

29 +

30 +static void update_callback(struct diff_queue_struct *q,

31 + struct diff_options *opt, void *cbdata)

32 +{

33 + int i;

34 + struct update_callback_data *data = cbdata;

35 +

36 + for (i = 0; i < q->nr; i++) {

37 + struct diff_filepair *p = q->queue[i];

38 + const char *path = p->one->path;

39 + switch (p->status) {

40 + default:

41 + die("unexpected diff status %c", p->status);

42 + case DIFF_STATUS_UNMERGED:

43 + /*

44 + * ADD_CACHE_IGNORE_REMOVAL is unset if "git

45 + * add -u" is calling us, In such a case, a

46 + * missing work tree file needs to be removed

47 + * if there is an unmerged entry at stage #2,

48 + * but such a diff record is followed by

49 + * another with DIFF_STATUS_DELETED (and if

50 + * there is no stage #2, we won't see DELETED

51 + * nor MODIFIED). We can simply continue

52 + * either way.

53 + */

54 + if (!(data->flags & ADD_CACHE_IGNORE_REMOVAL))

55 + continue;

56 + /*

57 + * Otherwise, it is "git add path" is asking

58 + * to explicitly add it; we fall through. A

59 + * missing work tree file is an error and is

60 + * caught by add_file_to_index() in such a

61 + * case.

62 + */

63 + case DIFF_STATUS_MODIFIED:

64 + case DIFF_STATUS_TYPE_CHANGED:

65 + if (add_file_to_index(&the_index, path, data->flags)) {

66 + if (!(data->flags & ADD_CACHE_IGNORE_ERRORS))

67 + die("updating files failed");

68 + data->add_errors++;

69 + }

70 + break;

71 + case DIFF_STATUS_DELETED:

72 + if (data->flags & ADD_CACHE_IGNORE_REMOVAL)

73 + break;

74 + if (!(data->flags & ADD_CACHE_PRETEND))

75 + remove_file_from_index(&the_index, path);

76 + if (data->flags & (ADD_CACHE_PRETEND|ADD_CACHE_VERBOSE))

77 + printf("remove '%s'\n", path);

78 + break;

79 + }

80 + }

81 +}

82 +

83 +int add_files_to_cache(const char *prefix, const char **pathspec, int flags)

84 +{

85 + struct update_callback_data data;

86 + struct rev_info rev;

87 + init_revisions(&rev, prefix);

88 + setup_revisions(0, NULL, &rev, NULL);

89 + rev.prune_data = pathspec;

90 + rev.diffopt.output_format = DIFF_FORMAT_CALLBACK;

91 + rev.diffopt.format_callback = update_callback;

92 + data.flags = flags;

93 + data.add_errors = 0;

94 + rev.diffopt.format_callback_data = &data;

95 + run_diff_files(&rev, DIFF_RACY_IS_MODIFIED);

96 + return !!data.add_errors;

97 +}

98 +

99 static void fill_pathspec_matches(const char **pathspec, char *seen, int specs)

100 {

101 int num_unmatched = 0, i;

read-cache.c

-78

@@ -10,9 +10,6 @@

10 #include "dir.h"

11 #include "tree.h"

12 #include "commit.h"

13 -#include "diff.h"

14 -#include "diffcore.h"

15 -#include "revision.h"

13 #include "blob.h"

14 #include "resolve-undo.h"

@@ -1648,81 +1645,6 @@ int read_index_unmerged(struct index_state *istate)

1645 return unmerged;

1646 }

1647

1651 -struct update_callback_data

1652 -{

1653 - int flags;

1654 - int add_errors;

1655 -};

1656 -

1657 -static void update_callback(struct diff_queue_struct *q,

1658 - struct diff_options *opt, void *cbdata)

1659 -{

1660 - int i;

1661 - struct update_callback_data *data = cbdata;

1662 -

1663 - for (i = 0; i < q->nr; i++) {

1664 - struct diff_filepair *p = q->queue[i];

1665 - const char *path = p->one->path;

1666 - switch (p->status) {

1667 - default:

1668 - die("unexpected diff status %c", p->status);

1669 - case DIFF_STATUS_UNMERGED:

1670 - /*

1671 - * ADD_CACHE_IGNORE_REMOVAL is unset if "git

1672 - * add -u" is calling us, In such a case, a

1673 - * missing work tree file needs to be removed

1674 - * if there is an unmerged entry at stage #2,

1675 - * but such a diff record is followed by

1676 - * another with DIFF_STATUS_DELETED (and if

1677 - * there is no stage #2, we won't see DELETED

1678 - * nor MODIFIED). We can simply continue

1679 - * either way.

1680 - */

1681 - if (!(data->flags & ADD_CACHE_IGNORE_REMOVAL))

1682 - continue;

1683 - /*

1684 - * Otherwise, it is "git add path" is asking

1685 - * to explicitly add it; we fall through. A

1686 - * missing work tree file is an error and is

1687 - * caught by add_file_to_index() in such a

1688 - * case.

1689 - */

1690 - case DIFF_STATUS_MODIFIED:

1691 - case DIFF_STATUS_TYPE_CHANGED:

1692 - if (add_file_to_index(&the_index, path, data->flags)) {

1693 - if (!(data->flags & ADD_CACHE_IGNORE_ERRORS))

1694 - die("updating files failed");

1695 - data->add_errors++;

1696 - }

1697 - break;

1698 - case DIFF_STATUS_DELETED:

1699 - if (data->flags & ADD_CACHE_IGNORE_REMOVAL)

1700 - break;

1701 - if (!(data->flags & ADD_CACHE_PRETEND))

1702 - remove_file_from_index(&the_index, path);

1703 - if (data->flags & (ADD_CACHE_PRETEND|ADD_CACHE_VERBOSE))

1704 - printf("remove '%s'\n", path);

1705 - break;

1706 - }

1707 - }

1708 -}

1709 -

1710 -int add_files_to_cache(const char *prefix, const char **pathspec, int flags)

1711 -{

1712 - struct update_callback_data data;

1713 - struct rev_info rev;

1714 - init_revisions(&rev, prefix);

1715 - setup_revisions(0, NULL, &rev, NULL);

1716 - rev.prune_data = pathspec;

1717 - rev.diffopt.output_format = DIFF_FORMAT_CALLBACK;

1718 - rev.diffopt.format_callback = update_callback;

1719 - data.flags = flags;

1720 - data.add_errors = 0;

1721 - rev.diffopt.format_callback_data = &data;

1722 - run_diff_files(&rev, DIFF_RACY_IS_MODIFIED);

1723 - return !!data.add_errors;

1724 -}

1725 -

1648 /*

1649 * Returns 1 if the path is an "other" path with respect to

1650 * the index; that is, the path is not mentioned in the index at all,