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

> Tomohiro Koana <kntmhr.1221@gmail.com> writes:
>
>> 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.
>
>> --- 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.
>
>> + 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/