Re: [[GSoC][PATCH …]] In notes-merge.c updated notes_merge_commit()

[Thomas Gummerer <t.gummerer@gmail.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.

> ---
>  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