Re: [PATCH 2/3] merge-recursive: remove return value from get_files_dirs

From: Jeff King <peff@peff.net>
Date: Aug 29, 2017, 16:50 UTC
Message-ID: <20170829165038.xk6yluuqnzbujodq@sigill.intra.peff.net>
In-Reply-To: <DM2PR21MB0041575B6D9EE53A07C7D3EDB79F0@DM2PR21MB0041.namprd21.prod.outlook.com>

On Tue, Aug 29, 2017 at 03:58:00PM +0000, Kevin Willford wrote:

Show 15 quoted lines

> > > The return value of the get_files_dirs call is never being used.
> > > Looking at the history of the file and it was originally only
> > > being used for debug output statements.  Also when
> > > read_tree_recursive return value is non zero it is changed to
> > > zero.  This leads me to believe that it doesn't matter if
> > > read_tree_recursive gets an error.
> > 
> > Or that the function is buggy. :)
> 
> That was one of my questions as well.  Should read_tree_recursive
> be propagating a -1 and merge_trees be checking for that and bail
> when the call to get_files_dirs return is < 0?  I made a commit with
> this change and ran the tests and they all still passed so either this
> return really doesn't matter or there are not sufficient tests covering
> it.

Right, in a normal flow I'd say that it should propagate the -1, etc. But since the only possible error is parse_tree() failing, which happens in a corrupt repository, I think it would be OK to just die(). That saves having to deal with error propagation. It's not very graceful, perhaps, but the important thing is that we don't quietly ignore the error.

> I went with this change because it was not changing any of the
> current functionality and if we find a case where it matters that
> read_tree_recursive fails due to bad tree or something else we
> can address it then.

I think it's worth doing while we're thinking about it now, but it certainly can be a separate patch.

-Peff