threads / patch / 33492
Subject: [PATCH 1/3] Make read_index_data() public
[PATCH 0/3] Remove ~25 lines of duplicate code
Merge read_index_data() and has_cr_in_index() which are almost 1:1 copies of each other.
Lukas Fleischer (3): Make read_index_data() public Add size parameter to read_index_data() convert.c: Remove duplicate code
attr.c | 35 +---------------------------------- cache.h | 1 + convert.c | 27 ++------------------------- read-cache.c | 36 ++++++++++++++++++++++++++++++++++++ 4 files changed, 40 insertions(+), 59 deletions(-)
-- 1.8.2.675.gda3bb24.dirty
This allows for reusing the function in convert.c later.
Also, move it from attr.c to read-cache.c and add a use_index parameter to specify a custom index_state since we are no longer enable to access the static use_index variable from attr.c.
Signed-off-by: Lukas Fleischer <git@cryptocrack.de> --- I am not totally sure whether read-cache.c is the best place for this...
attr.c | 35 +---------------------------------- cache.h | 1 + read-cache.c | 33 +++++++++++++++++++++++++++++++++ 3 files changed, 35 insertions(+), 34 deletions(-)
attr.c, cache.h, read-cache.c
diff --git a/attr.c b/attr.c index 689bc2a..2e1ce7b 100644 --- a/attr.c +++ b/attr.c @@ -381,46 +381,13 @@ static struct attr_stack *read_attr_from_file(const char *path, int macro_ok) return res; } -static void *read_index_data(const char *path) -{ - int pos, len; - unsigned long sz; - enum object_type type; - void *data; - struct index_state *istate = use_index ? use_index : &the_index; - - len = strlen(path); - pos = index_name_pos(istate, path, len); - if (pos < 0) { - /* - * We might be in the middle of a merge, in which - * case we would read stage #2 (ours). - */ - int i; - for (i = -pos - 1; - (pos < 0 && i < istate->cache_nr && - !strcmp(istate->cache[i]->name, path)); - i++) - if (ce_stage(istate->cache[i]) == 2) - pos = i; - } - if (pos < 0) - return NULL; - data = read_sha1_file(istate->cache[pos]->sha1, &type, &sz); - if (!data || type != OBJ_BLOB) { - free(data); - return NULL; - } - return data; -} - static struct attr_stack *read_attr_from_index(const char *path, int macro_ok) { struct attr_stack *res; char *buf, *sp; int lineno = 0; - buf = read_index_data(path); + buf = read_index_data(path, use_index); if (!buf) return NULL; diff --git a/cache.h b/cache.h index ba5a47c..a71e443 100644 --- a/cache.h +++ b/cache.h @@ -471,6 +471,7 @@ extern int add_file_to_index(struct index_state *, const char *path, int flags); extern struct cache_entry *make_cache_entry(unsigned int mode, const unsigned char *sha1, const char *path, int stage, int refresh); extern int ce_same_name(struct cache_entry *a, struct cache_entry *b); extern int index_name_is_other(const struct index_state *, const char *, int); +extern void *read_index_data(const char *path, struct index_state *use_index); /* do stat comparison even if CE_VALID is true */ #define CE_MATCH_IGNORE_VALID 01 diff --git a/read-cache.c b/read-cache.c index 5a9704f..39e3424 100644 --- a/read-cache.c +++ b/read-cache.c @@ -1899,3 +1899,36 @@ int index_name_is_other(const struct index_state *istate, const char *name, } return 1; } + +void *read_index_data(const char *path, struct index_state *use_index) +{ + int pos, len; + unsigned long sz; + enum object_type type; + void *data; + struct index_state *istate = use_index ? use_index : &the_index; + + len = strlen(path); + pos = index_name_pos(istate, path, len); + if (pos < 0) { + /* + * We might be in the middle of a merge, in which + * case we would read stage #2 (ours). + */ + int i; + for (i = -pos - 1; + (pos < 0 && i < istate->cache_nr && + !strcmp(istate->cache[i]->name, path)); + i++) + if (ce_stage(istate->cache[i]) == 2) + pos = i; + } + if (pos < 0) + return NULL; + data = read_sha1_file(istate->cache[pos]->sha1, &type, &sz); + if (!data || type != OBJ_BLOB) { + free(data); + return NULL; + } + return data; +}
-- 1.8.2.675.gda3bb24.dirty
Jeff KingRe: [PATCH 1/3] Make read_index_data() public
On Fri, Apr 12, 2013 at 07:26:11PM +0200, Lukas Fleischer wrote:
> This allows for reusing the function in convert.c later. > > Also, move it from attr.c to read-cache.c and add a use_index parameter > to specify a custom index_state since we are no longer enable to access > the static use_index variable from attr.c.
I'm all for removing duplicated code, but, but I think the name "read_index_data" is a bit misleading for a global function. I would expect it to read data from the index (and the argument "path" does not help clarify that at all).
Can we rename it read_blob_data_from_index_path() or something?
-Peff
Re: [PATCH 1/3] Make read_index_data() public
On Fri, Apr 12, 2013 at 03:40:12PM -0400, Jeff King wrote:
> On Fri, Apr 12, 2013 at 07:26:11PM +0200, Lukas Fleischer wrote: > > > This allows for reusing the function in convert.c later. > > > > Also, move it from attr.c to read-cache.c and add a use_index parameter > > to specify a custom index_state since we are no longer enable to access > > the static use_index variable from attr.c. > > I'm all for removing duplicated code, but, but I think the name > "read_index_data" is a bit misleading for a global function. I would > expect it to read data from the index (and the argument "path" does not > help clarify that at all). > > Can we rename it read_blob_data_from_index_path() or something?
Pretty long function name... I agree that it is better to have a verbose function name instead of something short that gives a wrong impression of what the function does and I can't think of a shorter, yet descriptive way to name the function.
If nobody comes up with an alternative suggestion, I will change it to read_blob_data_from_index_path() and resubmit.
> > -Peff
[PATCH 2/3] Add size parameter to read_index_data()
This allows for optionally getting the size of the returned data and will be used in a follow-up patch.
Signed-off-by: Lukas Fleischer <git@cryptocrack.de> --- attr.c | 2 +- cache.h | 2 +- read-cache.c | 5 ++++- 3 files changed, 6 insertions(+), 3 deletions(-)
attr.c, cache.h, read-cache.c
diff --git a/attr.c b/attr.c index 2e1ce7b..e5af3c6 100644 --- a/attr.c +++ b/attr.c @@ -387,7 +387,7 @@ static struct attr_stack *read_attr_from_index(const char *path, int macro_ok) char *buf, *sp; int lineno = 0; - buf = read_index_data(path, use_index); + buf = read_index_data(path, use_index, NULL); if (!buf) return NULL; diff --git a/cache.h b/cache.h index a71e443..b281bbf 100644 --- a/cache.h +++ b/cache.h @@ -471,7 +471,7 @@ extern int add_file_to_index(struct index_state *, const char *path, int flags); extern struct cache_entry *make_cache_entry(unsigned int mode, const unsigned char *sha1, const char *path, int stage, int refresh); extern int ce_same_name(struct cache_entry *a, struct cache_entry *b); extern int index_name_is_other(const struct index_state *, const char *, int); -extern void *read_index_data(const char *path, struct index_state *use_index); +extern void *read_index_data(const char *path, struct index_state *use_index, unsigned long *size); /* do stat comparison even if CE_VALID is true */ #define CE_MATCH_IGNORE_VALID 01 diff --git a/read-cache.c b/read-cache.c index 39e3424..32dc471 100644 --- a/read-cache.c +++ b/read-cache.c @@ -1900,7 +1900,8 @@ int index_name_is_other(const struct index_state *istate, const char *name, return 1; } -void *read_index_data(const char *path, struct index_state *use_index) +void *read_index_data(const char *path, struct index_state *use_index, + unsigned long *size) { int pos, len; unsigned long sz; @@ -1930,5 +1931,7 @@ void *read_index_data(const char *path, struct index_state *use_index) free(data); return NULL; } + if (size) + *size = sz; return data; }
-- 1.8.2.675.gda3bb24.dirty
[PATCH 3/3] convert.c: Remove duplicate code
has_cr_in_index() is an almost 1:1 copy of read_index_data() with some additions. Invoke read_index_data() instead of using copy-pasted code.
Signed-off-by: Lukas Fleischer <git@cryptocrack.de> --- convert.c | 27 ++------------------------- 1 file changed, 2 insertions(+), 25 deletions(-)
diff --git a/convert.c b/convert.c index 3520252..0d9f8a9 100644 --- a/convert.c +++ b/convert.c @@ -153,36 +153,13 @@ static void check_safe_crlf(const char *path, enum crlf_action crlf_action, static int has_cr_in_index(const char *path) { - int pos, len; unsigned long sz; - enum object_type type; void *data; int has_cr; - struct index_state *istate = &the_index; - len = strlen(path); - pos = index_name_pos(istate, path, len); - if (pos < 0) { - /* - * We might be in the middle of a merge, in which - * case we would read stage #2 (ours). - */ - int i; - for (i = -pos - 1; - (pos < 0 && i < istate->cache_nr && - !strcmp(istate->cache[i]->name, path)); - i++) - if (ce_stage(istate->cache[i]) == 2) - pos = i; - } - if (pos < 0) + data = read_index_data(path, NULL, &sz); + if (!data) return 0; - data = read_sha1_file(istate->cache[pos]->sha1, &type, &sz); - if (!data || type != OBJ_BLOB) { - free(data); - return 0; - } - has_cr = memchr(data, '\r', sz) != NULL; free(data); return has_cr;
-- 1.8.2.675.gda3bb24.dirty
[PATCH v2 1/3] Add public function read_blob_data_from_index_path()
* Make the read_index_data() function public, rename it to read_blob_data_from_index_path() and move it from attr.c to read-cache.c.
* Add a use_index parameter to specify a custom index_state since we are no longer enable to access the static use_index variable from attr.c.
This allows for reusing the function in convert.c later.
Signed-off-by: Lukas Fleischer <git@cryptocrack.de> --- attr.c | 35 +---------------------------------- cache.h | 1 + read-cache.c | 34 ++++++++++++++++++++++++++++++++++ 3 files changed, 36 insertions(+), 34 deletions(-)
attr.c, cache.h, read-cache.c
diff --git a/attr.c b/attr.c index 689bc2a..08347b6 100644 --- a/attr.c +++ b/attr.c @@ -381,46 +381,13 @@ static struct attr_stack *read_attr_from_file(const char *path, int macro_ok) return res; } -static void *read_index_data(const char *path) -{ - int pos, len; - unsigned long sz; - enum object_type type; - void *data; - struct index_state *istate = use_index ? use_index : &the_index; - - len = strlen(path); - pos = index_name_pos(istate, path, len); - if (pos < 0) { - /* - * We might be in the middle of a merge, in which - * case we would read stage #2 (ours). - */ - int i; - for (i = -pos - 1; - (pos < 0 && i < istate->cache_nr && - !strcmp(istate->cache[i]->name, path)); - i++) - if (ce_stage(istate->cache[i]) == 2) - pos = i; - } - if (pos < 0) - return NULL; - data = read_sha1_file(istate->cache[pos]->sha1, &type, &sz); - if (!data || type != OBJ_BLOB) { - free(data); - return NULL; - } - return data; -} - static struct attr_stack *read_attr_from_index(const char *path, int macro_ok) { struct attr_stack *res; char *buf, *sp; int lineno = 0; - buf = read_index_data(path); + buf = read_blob_data_from_index_path(path, use_index); if (!buf) return NULL; diff --git a/cache.h b/cache.h index ba5a47c..2a206aa 100644 --- a/cache.h +++ b/cache.h @@ -471,6 +471,7 @@ extern int add_file_to_index(struct index_state *, const char *path, int flags); extern struct cache_entry *make_cache_entry(unsigned int mode, const unsigned char *sha1, const char *path, int stage, int refresh); extern int ce_same_name(struct cache_entry *a, struct cache_entry *b); extern int index_name_is_other(const struct index_state *, const char *, int); +extern void *read_blob_data_from_index_path(const char *path, struct index_state *use_index); /* do stat comparison even if CE_VALID is true */ #define CE_MATCH_IGNORE_VALID 01 diff --git a/read-cache.c b/read-cache.c index 5a9704f..964ae26 100644 --- a/read-cache.c +++ b/read-cache.c @@ -1899,3 +1899,37 @@ int index_name_is_other(const struct index_state *istate, const char *name, } return 1; } + +void *read_blob_data_from_index_path(const char *path, + struct index_state *use_index) +{ + int pos, len; + unsigned long sz; + enum object_type type; + void *data; + struct index_state *istate = use_index ? use_index : &the_index; + + len = strlen(path); + pos = index_name_pos(istate, path, len); + if (pos < 0) { + /* + * We might be in the middle of a merge, in which + * case we would read stage #2 (ours). + */ + int i; + for (i = -pos - 1; + (pos < 0 && i < istate->cache_nr && + !strcmp(istate->cache[i]->name, path)); + i++) + if (ce_stage(istate->cache[i]) == 2) + pos = i; + } + if (pos < 0) + return NULL; + data = read_sha1_file(istate->cache[pos]->sha1, &type, &sz); + if (!data || type != OBJ_BLOB) { + free(data); + return NULL; + } + return data; +}
-- 1.8.2.675.gda3bb24.dirty
[PATCH v2 2/3] Add size parameter to read_blob_data_from_index_path()
This allows for optionally getting the size of the returned data and will be used in a follow-up patch.
Signed-off-by: Lukas Fleischer <git@cryptocrack.de> --- attr.c | 2 +- cache.h | 2 +- read-cache.c | 5 ++++- 3 files changed, 6 insertions(+), 3 deletions(-)
attr.c, cache.h, read-cache.c
diff --git a/attr.c b/attr.c index 08347b6..17338c8 100644 --- a/attr.c +++ b/attr.c @@ -387,7 +387,7 @@ static struct attr_stack *read_attr_from_index(const char *path, int macro_ok) char *buf, *sp; int lineno = 0; - buf = read_blob_data_from_index_path(path, use_index); + buf = read_blob_data_from_index_path(path, use_index, NULL); if (!buf) return NULL; diff --git a/cache.h b/cache.h index 2a206aa..a5272f2 100644 --- a/cache.h +++ b/cache.h @@ -471,7 +471,7 @@ extern int add_file_to_index(struct index_state *, const char *path, int flags); extern struct cache_entry *make_cache_entry(unsigned int mode, const unsigned char *sha1, const char *path, int stage, int refresh); extern int ce_same_name(struct cache_entry *a, struct cache_entry *b); extern int index_name_is_other(const struct index_state *, const char *, int); -extern void *read_blob_data_from_index_path(const char *path, struct index_state *use_index); +extern void *read_blob_data_from_index_path(const char *path, struct index_state *use_index, unsigned long *size); /* do stat comparison even if CE_VALID is true */ #define CE_MATCH_IGNORE_VALID 01 diff --git a/read-cache.c b/read-cache.c index 964ae26..547e98a 100644 --- a/read-cache.c +++ b/read-cache.c @@ -1901,7 +1901,8 @@ int index_name_is_other(const struct index_state *istate, const char *name, } void *read_blob_data_from_index_path(const char *path, - struct index_state *use_index) + struct index_state *use_index, + unsigned long *size) { int pos, len; unsigned long sz; @@ -1931,5 +1932,7 @@ void *read_blob_data_from_index_path(const char *path, free(data); return NULL; } + if (size) + *size = sz; return data; }
-- 1.8.2.675.gda3bb24.dirty
[PATCH v2 3/3] convert.c: Remove duplicate code
has_cr_in_index() is an almost 1:1 copy of read_blob_data_from_index_path() with some additions. Invoke read_blob_data_from_index_path() instead of using copy-pasted code.
Signed-off-by: Lukas Fleischer <git@cryptocrack.de> --- convert.c | 27 ++------------------------- 1 file changed, 2 insertions(+), 25 deletions(-)
diff --git a/convert.c b/convert.c index 3520252..a12bc78 100644 --- a/convert.c +++ b/convert.c @@ -153,36 +153,13 @@ static void check_safe_crlf(const char *path, enum crlf_action crlf_action, static int has_cr_in_index(const char *path) { - int pos, len; unsigned long sz; - enum object_type type; void *data; int has_cr; - struct index_state *istate = &the_index; - len = strlen(path); - pos = index_name_pos(istate, path, len); - if (pos < 0) { - /* - * We might be in the middle of a merge, in which - * case we would read stage #2 (ours). - */ - int i; - for (i = -pos - 1; - (pos < 0 && i < istate->cache_nr && - !strcmp(istate->cache[i]->name, path)); - i++) - if (ce_stage(istate->cache[i]) == 2) - pos = i; - } - if (pos < 0) + data = read_blob_data_from_index_path(path, NULL, &sz); + if (!data) return 0; - data = read_sha1_file(istate->cache[pos]->sha1, &type, &sz); - if (!data || type != OBJ_BLOB) { - free(data); - return 0; - } - has_cr = memchr(data, '\r', sz) != NULL; free(data); return has_cr;
-- 1.8.2.675.gda3bb24.dirty
Junio C HamanoRe: [PATCH v2 1/3] Add public function read_blob_data_from_index_path()
Thanks. I'll queue with the following API fix-up on this, with obvious adjustments necessary for the later ones.
* read_blob_data_from_index() is descriptive enough. If you read a blob from the index, you would ask for it with a path; there is no other sensible key to do so.
* A function in read-cache API that works on an index should have the istate as the first argument and called with name "index";
* If callers want to operate on the default index, macro with a similar name but replacing "index" with "cache" can be supplied, e.g.
#define distim_cache(x) distim_index(&the_index, x) --- attr.c | 2 +- cache.h | 3 ++- read-cache.c | 4 +--- 3 files changed, 4 insertions(+), 5 deletions(-)
attr.c, cache.h, read-cache.c
diff --git a/attr.c b/attr.c index a668a63..46bde57 100644 --- a/attr.c +++ b/attr.c @@ -387,7 +387,7 @@ static struct attr_stack *read_attr_from_index(const char *path, int macro_ok) char *buf, *sp; int lineno = 0; - buf = read_blob_data_from_index_path(path, use_index); + buf = read_blob_data_from_index(use_index, path); if (!buf) return NULL; diff --git a/cache.h b/cache.h index 85ddb13..5d66c74 100644 --- a/cache.h +++ b/cache.h @@ -307,6 +307,7 @@ extern void free_name_hash(struct index_state *istate); #define resolve_undo_clear() resolve_undo_clear_index(&the_index) #define unmerge_cache_entry_at(at) unmerge_index_entry_at(&the_index, at) #define unmerge_cache(pathspec) unmerge_index(&the_index, pathspec) +#define read_blob_data_from_cache(path) read_blob_data_from_index(&the_index, (path)) #endif enum object_type { @@ -452,7 +453,7 @@ extern int add_file_to_index(struct index_state *, const char *path, int flags); extern struct cache_entry *make_cache_entry(unsigned int mode, const unsigned char *sha1, const char *path, int stage, int refresh); extern int ce_same_name(struct cache_entry *a, struct cache_entry *b); extern int index_name_is_other(const struct index_state *, const char *, int); -extern void *read_blob_data_from_index_path(const char *path, struct index_state *use_index); +extern void *read_blob_data_from_index(struct index_state *, const char *); /* do stat comparison even if CE_VALID is true */ #define CE_MATCH_IGNORE_VALID 01 diff --git a/read-cache.c b/read-cache.c index cbeb248..48d87e8 100644 --- a/read-cache.c +++ b/read-cache.c @@ -1896,14 +1896,12 @@ int index_name_is_other(const struct index_state *istate, const char *name, return 1; } -void *read_blob_data_from_index_path(const char *path, - struct index_state *use_index) +void *read_blob_data_from_index(struct index_state *istate, const char *path) { int pos, len; unsigned long sz; enum object_type type; void *data; - struct index_state *istate = use_index ? use_index : &the_index; len = strlen(path); pos = index_name_pos(istate, path, len);
-- 1.8.2.1-514-gf369d36
Junio C HamanoRe: [PATCH v2 1/3] Add public function read_blob_data_from_index_path()
Junio C Hamano <gitster@pobox.com> writes:
> diff --git a/attr.c b/attr.c > index a668a63..46bde57 100644 > --- a/attr.c > +++ b/attr.c > @@ -387,7 +387,7 @@ static struct attr_stack *read_attr_from_index(const char *path, int macro_ok) > char *buf, *sp; > int lineno = 0; > > - buf = read_blob_data_from_index_path(path, use_index); > + buf = read_blob_data_from_index(use_index, path);
This was wrong.
The one I'll push out will have:
buf = read_blob_data_from_index(use_index ? use_index : &the_index, path);
Re: [PATCH v2 1/3] Add public function read_blob_data_from_index_path()
On Sat, Apr 13, 2013 at 10:49:36PM -0700, Junio C Hamano wrote:
> Thanks. I'll queue with the following API fix-up on this, with > obvious adjustments necessary for the later ones. > > * read_blob_data_from_index() is descriptive enough. If you read a > blob from the index, you would ask for it with a path; there is > no other sensible key to do so. > > * A function in read-cache API that works on an index should have > the istate as the first argument and called with name "index"; > > * If callers want to operate on the default index, macro with a > similar name but replacing "index" with "cache" can be supplied, > e.g. > > #define distim_cache(x) distim_index(&the_index, x) > --- > attr.c | 2 +- > cache.h | 3 ++- > read-cache.c | 4 +--- > 3 files changed, 4 insertions(+), 5 deletions(-)
Sounds reasonable to me -- squashing the fixed up version into my original patch ([PATCH v2 1/1] Add public function read_blob_data_from_index_path()) probably makes sense here :)
Thanks for reviewing and reworking!
>
> diff --git a/attr.c b/attr.c
> index a668a63..46bde57 100644
> --- a/attr.c
> +++ b/attr.c
> @@ -387,7 +387,7 @@ static struct attr_stack *read_attr_from_index(const char *path, int macro_ok)
> char *buf, *sp;
> int lineno = 0;
>
> - buf = read_blob_data_from_index_path(path, use_index);
> + buf = read_blob_data_from_index(use_index, path);
> if (!buf)
> return NULL;
>
> diff --git a/cache.h b/cache.h
> index 85ddb13..5d66c74 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -307,6 +307,7 @@ extern void free_name_hash(struct index_state *istate);
> #define resolve_undo_clear() resolve_undo_clear_index(&the_index)
> #define unmerge_cache_entry_at(at) unmerge_index_entry_at(&the_index, at)
> #define unmerge_cache(pathspec) unmerge_index(&the_index, pathspec)
> +#define read_blob_data_from_cache(path) read_blob_data_from_index(&the_index, (path))
> #endif
>
> enum object_type {
> @@ -452,7 +453,7 @@ extern int add_file_to_index(struct index_state *, const char *path, int flags);
> extern struct cache_entry *make_cache_entry(unsigned int mode, const unsigned char *sha1, const char *path, int stage, int refresh);
> extern int ce_same_name(struct cache_entry *a, struct cache_entry *b);
> extern int index_name_is_other(const struct index_state *, const char *, int);
> -extern void *read_blob_data_from_index_path(const char *path, struct index_state *use_index);
> +extern void *read_blob_data_from_index(struct index_state *, const char *);
>
> /* do stat comparison even if CE_VALID is true */
> #define CE_MATCH_IGNORE_VALID 01
> diff --git a/read-cache.c b/read-cache.c
> index cbeb248..48d87e8 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -1896,14 +1896,12 @@ int index_name_is_other(const struct index_state *istate, const char *name,
> return 1;
> }
>
> -void *read_blob_data_from_index_path(const char *path,
> - struct index_state *use_index)
> +void *read_blob_data_from_index(struct index_state *istate, const char *path)
> {
> int pos, len;
> unsigned long sz;
> enum object_type type;
> void *data;
> - struct index_state *istate = use_index ? use_index : &the_index;
>
> len = strlen(path);
> pos = index_name_pos(istate, path, len);
> --
> 1.8.2.1-514-gf369d36
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html