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

[Taylor Blau <me@ttaylorr.com>]

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