Hi Thomas, On Sat, Apr 06, 2019 at 03:41:27PM +0100, Thomas Gummerer wrote: > > Subject: [[GSoC][PATCH …]] In notes-merge.c updated notes_merge_commit() > > Commit messages are usually in the format "<area>: <short > description>". So a better title might be "notes-merge: use > dir-iterator in notes_merge_commit". It would also be beneficial for > you to have a look at the mailing list archives at > https://public-inbox.org/git/, and search for similar patches, to see > how they have been done. > > On 04/05, UTKARSH RAI wrote: > > Updated notes_merge_commit() by replacing readdir() ,opendir() apis by replacing them with dir_iterator_advance() and dir_iterator_begin() respectively. > > Please wrap commit messages at around 72 characters or less. Also the > commit message should be written in the imperative mood, see also > Documentation/SubmittingPatches. Thanks to brian carlson's, git has an .editorconfig which enforces this explicitly (c.f., 6f9beef335 (editorconfig: provide editor settings for Git developers, 2018-10-08)). I use the editorconfig plugin for my editor [1], which makes sure that I don't write a too-long line in a commit message, or in an email such as this one ;-). Utkarsh -- I certainly recommend an editor-appropriate plugin, if you are worried about this sort of thing (as I certainly was/am). I reviewed the patch while writing this note, and I agree with Thomas's review, so I don't think I have anything to add there. > > Signed-off-by: ur10 <utkarsh.rai60@xxxxxxxxx> > > The name in the Signed-off-by line should match the author of the > commit. Also there should be a blank line between the commit message > and the Signed-off-by line. > > > --- > > notes-merge.c | 22 ++++++++++++---------- > > 1 file changed, 12 insertions(+), 10 deletions(-) > > > > diff --git a/notes-merge.c b/notes-merge.c > > index 280aa8e6c1b04..dc4e2cce7151a 100644 > > --- a/notes-merge.c > > +++ b/notes-merge.c > > @@ -13,6 +13,8 @@ > > #include "strbuf.h" > > #include "notes-utils.h" > > #include "commit-reach.h" > > +#include "dir-iterator.h" > > +#include "iterator.h" > > > > struct notes_merge_pair { > > struct object_id obj, base, local, remote; > > @@ -673,8 +675,8 @@ int notes_merge_commit(struct notes_merge_options *o, > > * commit message and parents from 'partial_commit'. > > * Finally store the new commit object OID into 'result_oid'. > > */ > > - DIR *dir; > > - struct dirent *e; > > + struct dir_iterator *iter; > > + int ok; > > struct strbuf path = STRBUF_INIT; > > const char *buffer = get_commit_buffer(partial_commit, NULL); > > const char *msg = strstr(buffer, "\n\n"); > > @@ -689,27 +691,27 @@ int notes_merge_commit(struct notes_merge_options *o, > > die("partial notes commit has empty message"); > > msg += 2; > > > > - dir = opendir(path.buf); > > - if (!dir) > > + iter = dir_iterator_begin(path.buf); > > + if (!iter) > > die_errno("could not open %s", path.buf); > > > > strbuf_addch(&path, '/'); > > baselen = path.len; > > - while ((e = readdir(dir)) != NULL) { > > + while ((ok = dir_iterator_advance(iter) )== ITER_OK) { > > Style: please don't leave a space between the closing braces, but > there should be a space before the ==. > > But more importantly, there's a change in behaviour here, previously > we wouldn't recurse into any subdirectories if there are any, while > now we do. It looks like there cannot be any subdirectories in this > directory, so this might be fine, but I didn't check in detail. This > is something that you should investigate and describe in the commit > message. > > > struct stat st; > > struct object_id obj_oid, blob_oid; > > > > - if (is_dot_or_dotdot(e->d_name)) > > + if (is_dot_or_dotdot(iter->basename)) > > continue; > > The above condition is no longer necessary, as dir-iterator already > skips these directories by default. > > > > > - if (get_oid_hex(e->d_name, &obj_oid)) { > > + if (get_oid_hex(iter->basename, &obj_oid)) { > > if (o->verbosity >= 3) > > printf("Skipping non-SHA1 entry '%s%s'\n", > > - path.buf, e->d_name); > > + path.buf, iter->basename); > > continue; > > } > > > > - strbuf_addstr(&path, e->d_name); > > + strbuf_addstr(&path,iter->basename); > > Style: missing space after the comma. > > > /* write file as blob, and add to partial_tree */ > > if (stat(path.buf, &st)) > > die_errno("Failed to stat '%s'", path.buf); > > @@ -731,7 +733,7 @@ int notes_merge_commit(struct notes_merge_options *o, > > printf("Finalized notes merge commit: %s\n", > > oid_to_hex(result_oid)); > > strbuf_release(&path); > > - closedir(dir); > > + > > Please remove trailing spaces/tabs. You can check for this using 'git > diff --check [base]...HEAD', and fix it with 'git rebase --whitespace=fix'. > > > return 0; > > } > > > > > > -- > > https://github.com/git/git/pull/594 Thanks, Taylor [1]: https://github.com/editorconfig/editorconfig-vim
