@matheusbsbs / matheus-bagliano / commits / cfa5bf1608

commit: rewrite read_graft_line

Old implementation determined number of hashes by dividing length of line by length of hash, which works only if all hash representations have same length. New graft line parser works in two phases: 1. In first phase line is scanned to verify correctness and compute number of hashes, then graft struct is allocated. 2. In second phase line is scanned again to fill up already allocated graft struct. This way graft parsing code can support different sizes of hashes without any further code adaptations. A number of alternative implementations were considered and discarded: - Modifying graft structure to store oid_array instead of FLEXI_ARRAY indicates undesirable usage of struct to readers. - Parsing into temporary string_list or oid_array complicates code by adding more return paths, as these structures needs to be cleared before returning from function. - Determining number of hashes by counting separators might cause maintenance issues, if this function needs to be modified in future again. Signed-off-by: Patryk Obara <patryk.obara@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Patryk Obara committed Aug 18, 2017 at 20:33 UTC cfa5bf16082e25c9221e26851ca890f71ddf5a5f

1 file changed +21 -15

commit.c

+21 -15

@@ -137,32 +137,38 @@ int register_commit_graft(struct commit_graft *graft, int ignore_dups)

137 struct commit_graft *read_graft_line(struct strbuf *line)

138 {

139 /* The format is just "Commit Parent1 Parent2 ...\n" */

140 - int i;

140 + int i, phase;

141 + const char *tail = NULL;

142 struct commit_graft *graft = NULL;

142 - const int entry_size = GIT_SHA1_HEXSZ + 1;

143 + struct object_id dummy_oid, *oid;

144

145 strbuf_rtrim(line);

146 if (!line->len || line->buf[0] == '#')

147 return NULL;

147 - if ((line->len + 1) % entry_size)

148 - goto bad_graft_data;

149 - i = (line->len + 1) / entry_size - 1;

150 - graft = xmalloc(st_add(sizeof(*graft),

151 - st_mult(sizeof(struct object_id), i)));

152 - graft->nr_parent = i;

153 - if (get_oid_hex(line->buf, &graft->oid))

154 - goto bad_graft_data;

155 - for (i = GIT_SHA1_HEXSZ; i < line->len; i += entry_size) {

156 - if (line->buf[i] != ' ')

157 - goto bad_graft_data;

158 - if (get_sha1_hex(line->buf + i + 1, graft->parent[i/entry_size].hash))

148 + /*

149 + * phase 0 verifies line, counts hashes in line and allocates graft

150 + * phase 1 fills graft

151 + */

152 + for (phase = 0; phase < 2; phase++) {

153 + oid = graft ? &graft->oid : &dummy_oid;

154 + if (parse_oid_hex(line->buf, oid, &tail))

155 goto bad_graft_data;

156 + for (i = 0; *tail != '\0'; i++) {

157 + oid = graft ? &graft->parent[i] : &dummy_oid;

158 + if (!isspace(*tail++) || parse_oid_hex(tail, oid, &tail))

159 + goto bad_graft_data;

160 + }

161 + if (!graft) {

162 + graft = xmalloc(st_add(sizeof(*graft),

163 + st_mult(sizeof(struct object_id), i)));

164 + graft->nr_parent = i;

165 + }

166 }

167 return graft;

168

169 bad_graft_data:

170 error("bad graft data: %s", line->buf);

165 - free(graft);

171 + assert(!graft);

172 return NULL;

173 }

174