Re: [PATCH/RFC] create a skeleton for the command git rebase --status

From: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Date: May 28, 2015, 12:36 UTC
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

Show 7 quoted lines

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