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

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

[UTKARSH RAI]

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

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

--
https://github.com/git/git/pull/594

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

[Thomas Gummerer]

> 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

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

[Taylor Blau]

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