Re: [PATCH v4 5/5] stash: implement builtin stash

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Jun 19, 2017, 13:16 UTC
Message-ID: <alpine.DEB.2.21.1.1706191516350.57822@virtualbox>
In-Reply-To: <xmqqvanvv9be.fsf@gitster.mtv.corp.google.com>

Hi Junio,

On Fri, 16 Jun 2017, Junio C Hamano wrote:

Show 14 quoted lines

> Joel Teichroeb <joel@teichroeb.net> writes:
> 
> > +static void stash_create_callback(struct diff_queue_struct *q,
> > +				struct diff_options *opt, void *cbdata)
> > +{
> > +	int i;
> > +
> > +	for (i = 0; i < q->nr; i++) {
> > +		struct diff_filepair *p = q->queue[i];
> > +		const char *path = p->one->path;
> > +		struct stat st;
> 
> The order is somewhat ugly.  Move "struct stat st;" that does not
> have any initialization at the beginning.

Let's not call it "ugly". You may find it ugly, but maybe you may want to avoid contributors feeling judged negatively, either.

Instead, let's say that it is preferred in Git's source code to declare uninitialized variables first, and then declare variables which are initialized at the same time.

This convention, however, would need to be documented in CodingGuidelines first. We do not want to make contributors feel dumb now, do we?

In this particular case, I also wonder whether it is worth the time to point out an unwritten (and not always obeyed) rule. The variable block is small enough that it does not matter much in which order the variables are declared.

However, trying to be very strict even in such a small matter may well cost us contributors (and it is dubious whether the most critical parts of our technical debt has anything to do with small code style issues similar to this one). It's not like our bar of entry to new contributors is very low, exactly...

And if you disagree with this assessment, you should point out the same issues in literally all of my patches, as I always put initialized variables first, uninitialized last.

Show 8 quoted lines

> > +	strbuf_reset(&out);
> > +
> > +	discard_cache();
> > +	read_cache_from(stash_index_path);
> > +
> > +	write_index_as_tree(orig_tree.hash, &the_index, stash_index_path, 0,NULL);
> 
> SP before "NULL".

If only we had automated source code formatting, saving us from these distractions during patch review.

The rest of the review, modulo all the "Hmpf"s, seems helpful enough that I will try to find time to review the next iteration of this patch series (with a fresh mind, as I only skimmed the previous iteration) instead of adding my comments here.

Ciao, Dscho