Re: [PATCH] Falis on commit --amend when already pushed

From: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Date: Oct 15, 2015, 06:44 UTC
Message-ID: <vpqr3kw636z.fsf@grenoble-inp.fr>
In-Reply-To: <CACbztPOSZBaqLLUERS_yfvAEPmxS2PZs0fYTy7E_9HfLi9UGmA@mail.gmail.com>

Tomohiro Koana <kntmhr.1221@gmail.com> writes:

Show 5 quoted lines

> Hello all,
>
> I'm a third year student at the University of Tokyo and, in our
> "Diving into open-source software" class, my friends and I decided to
> work with git. Our final, hopefully, is contributing to git.

Welcome on board :-). I give the same class to my students (in Ensimag, Grenoble, France). You can have a look at https://git.wiki.kernel.org/index.php/SmallProjectsIdeas for a list of ideas of things you can do.

The first contact with an open-source community is usually hard (the quality expectation is much higher that your usual lab works), but you are going to learn a lot!

> One improvement that we thought of was not letting users to amend
> commit when the commit is already pushed to the remote server.

This is a good introduction, but not a good commit message. The commit message is not about what you "thought", but about what the commit is doing, and more importantly _why_ it is doing that and doing it this way. See it as an argument like "You should accept this patch because ...." (even if you won't actually write it like this). Read some existing messages ("git log --no-merges") to see what I mean.

Please, read Documentation/SubmittingPatches and Documentation/CodingGuidelines in Git's source tree.

Show 13 quoted lines

> --- a/builtin/commit.c
>
> +++ b/builtin/commit.c
>
> @@ -32,6 +32,7 @@
>
>  #include "sequencer.h"
>
>  #include "notes-utils.h"
>
>  #include "mailmap.h"
>
> +#include "remote.h"

The patch is whitespace-damaged (there are extra blank lines). Use either "git send-email" or http://submitgit.herokuapp.com/ to submit your patches.

Show 7 quoted lines

> + stat_tracking_info(branch, &ours, &theirs, &full_base);
>
> + if (amend && ours == 0) {
>
> + die(_("This commit is already pushed to the remote -- cannot amend."));
>
> + }

I don't know the API well enough so I can't say whether this correctly detects already pushed branch, but this looks suspiciously simple. Are you not just detecting the presence of a remote-tracking branch? What you should do is to detect whether the remote-tracking branch contains the current commit.

Also, this is clearly not acceptable in its current form: there are many valid use-cases to amend an already-pushed commit, so you can't break the flow of people using this. It must 1) be configurable, and 2) unless you have a really good reason, backward-compatible by default.

Also, it lacks tests.

Actually, the idea you have is far, far more difficult than what you probably thought.

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