On 2019-01-08 at 14:12 Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> wrote: > On Mon, Jan 07 2019, Barret Rhoden wrote: > > > +static int handle_ignore_file(const char *path, struct string_list *ignores) > > +{ > > + FILE *fp = fopen(path, "r"); > > + struct strbuf sb = STRBUF_INIT; > > + > > + if (!fp) > > + return -1; > > + while (!strbuf_getline(&sb, fp)) { > > + const char *hash; > > + > > + hash = strchr(sb.buf, '#'); > > + if (hash) > > + strbuf_setlen(&sb, hash - sb.buf); > > + strbuf_trim(&sb); > > + if (!sb.len) > > + continue; > > + string_list_append(ignores, sb.buf); > > + } > > + fclose(fp); > > + strbuf_release(&sb); > > + return 0; > > +} > > Aside from other comments on this patch that Junio had either you mostly > copy-pasted this from init_skiplist() or you've come up with almost the > same code on your own. > > In any case, if we're going to integrate something like this patch let's > split this "parse file with SHA-1s or comments/whitespace" into a > utility function that both this and init_skiplist() can call. One minor difference is that fsck wants an unabbreviated SHA-1, using parse_oid_hex() instead of get_oid_committish(). Would you be OK with also changing fsck to take a committish instead of a full SHA-1? Is there a good place for the common helper? Since it's an oidset, I could put it in oidset.c. oidset_parse_file() or something. > Then we could split up the description for the fsck.skipList config > variable to reference that format, and say that both it and this new > thing should consult those docs for how it's parsed. Is there a good spot for the generic skipList documentation? The only common text would be: ... comments ('#'), empty lines, and any leading and trailing whitespace is ignored Thanks, Barret