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

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Junio,

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

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

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