| Message ID | 01020169ee702e51-e9c8d564-10f5-49e9-a411-fd7ceaef7afc-000000@eu-west-1.amazonses.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [[GSoC,…] ] In notes-merge.c updated notes_merge_commit() | expand |
> 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. > Signed-off-by: ur10 <utkarsh.rai60@gmail.com> 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
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@gmail.com> > > 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
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) { 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; - 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); /* 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); + return 0; }
Updated notes_merge_commit() by replacing readdir() ,opendir() apis by replacing them with dir_iterator_advance() and dir_iterator_begin() respectively. Signed-off-by: ur10 <utkarsh.rai60@gmail.com> --- notes-merge.c | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) -- https://github.com/git/git/pull/594