Thank you for the reply. I've taken a look at the list for small project ideas and our idea was on the list, which goes to show that what we are working on is reasonable enough :D We'll read the documentations for git contributions thoroughly and send a patch again. As for the test, we're not sure how to write a test because it involves remote branches, so we'd love to have advice on it. On Thu, Oct 15, 2015 at 3:44 PM, Matthieu Moy <Matthieu.Moy@xxxxxxxxxxxxxxx> wrote: > Tomohiro Koana <kntmhr.1221@xxxxxxxxx> 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/ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html