Re: [[GSoC][PATCH …]] In notes-merge.c updated notes_merge_commit()
- From
Thomas Gummerer <t.gummerer@gmail.com>
- Date
- Apr 6, 2019, 14:41 UTC
- Message-ID
- <20190406144127.GZ32487@hank.intra.tgummerer.com>
- In-Reply-To
- <01020169ee702e51-e9c8d564-10f5-49e9-a411-fd7ceaef7afc-000000@eu-west-1.amazonses.com>
> 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.
Show 42 quoted lines
> ---
> 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.
Show 6 quoted lines
> 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.
Show 12 quoted lines
>
> - 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.
Show 9 quoted lines
> /* 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'.
Show 6 quoted lines
> return 0; > } > > > -- > https://github.com/git/git/pull/594