From: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Date: Thu, 28 May 2015 12:36:30 GMT
Subject: Re: [PATCH/RFC] create a skeleton for the command git rebase --status
Message-ID: <vpq382gri9d.fsf@anie.imag.fr>
In-Reply-To: <1432814891-4717-1-git-send-email-guillaume.pages@ensimag.grenoble-inp.fr>

Guillaume Pagès <guillaume.pages@ensimag.grenoble-inp.fr> writes:

> It is an almost empty code I send to validate the global architecture
> of this command.  I choose to write

Avoid personal wording (I choose to write ... because -> The code is
written ... because). What matters in the commit message is the reason
for the choice, not who made it.

> +BUILTIN_OBJS += builtin/rebase--status--helper.o

No builtin/rebase--status--helper.c in your patch, is it intended?

> +status)
> +	git rebase--status--helper
> +	die

"die" is used to exit with an error code ($? != 0). You just mean exit
$? here, to exit with the same code as the helper.

And actually, you don't need to keep your script alive while the helper
is ran, so you can write

exec git rebase--status--helper

> --- /dev/null
> +++ b/rebase--status.c
> @@ -0,0 +1,6 @@
> +#include "rebase--status.h"
> +
> +int rebase_status(){
> +	printf("Rebase in progress\n");

... or not.

Avoid leaving incorrect things like this in intermediate steps, even if
you're going to fix them eventually. It's too easy to forget the actual
fix and leave a "Rebase in progress" message even when there's no rebase
in progress. And reviewers may get confused.

I'd have written stg like

	printf("git rebase --status is not yet implemented");

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/