{"thread":{"id":"40563","subject":"[PATCH] Falis on commit --amend when already pushed","startedAt":"2015-10-15T04:09:15Z","lastAt":"2015-10-15T07:55:30Z","messageCount":4,"participants":["Tomohiro Koana","Matthieu Moy"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"271711","messageId":"CACbztPOSZBaqLLUERS_yfvAEPmxS2PZs0fYTy7E_9HfLi9UGmA@mail.gmail.com","threadId":"40563","inReplyTo":null,"subject":"[PATCH] Falis on commit --amend when already pushed","fromName":"Tomohiro Koana","fromEmail":"kntmhr.1221@gmail.com","sentAt":"2015-10-15T04:09:15Z","receivedAt":"2015-10-15T04:09:15Z","isPatch":true,"sender":{"key":"kntmhr.1221@gmail.com","avatar":null},"body":"Hello all,\n\nI'm a third year student at the University of Tokyo and, in our\n\"Diving into open-source software\" class, my friends and I decided to\nwork with git. Our final, hopefully, is contributing to git.\n\nOne improvement that we thought of was not letting users to amend\ncommit when the commit is already pushed to the remote server.\n\n--- a/builtin/commit.c\n\n+++ b/builtin/commit.c\n\n@@ -32,6 +32,7 @@\n\n #include \"sequencer.h\"\n\n #include \"notes-utils.h\"\n\n #include \"mailmap.h\"\n\n+#include \"remote.h\"\n\n\n\n static const char * const builtin_commit_usage[] = {\n\n N_(\"git commit [] [--] ...\"),\n\n@@ -1125,6 +1126,9 @@ static int parse_and_validate_options(int argc,\nconst char *argv[],\n\n struct wt_status *s)\n\n {\n\n int f = 0;\n\n+ int ours, theirs;\n\n+ const char *full_base;\n\n+ struct branch *branch = branch_get(NULL);\n\n\n\n argc = parse_options(argc, argv, prefix, options, usage, 0);\n\n finalize_deferred_config(s);\n\n@@ -1149,6 +1153,12 @@ static int parse_and_validate_options(int argc,\nconst char *argv[],\n\n else if (whence == FROM_CHERRY_PICK)\n\n die(_(\"You are in the middle of a cherry-pick -- cannot amend.\"));\n\n }\n\n+\n\n+ stat_tracking_info(branch, &ours, &theirs, &full_base);\n\n+ if (amend && ours == 0) {\n\n+ die(_(\"This commit is already pushed to the remote -- cannot amend.\"));\n\n+ }\n\n+\n\n if (fixup_message && squash_message)\n\n die(_(\"Options --squash and --fixup cannot be used together\"));\n\n if (use_message)\n"},{"id":"271712","messageId":"vpqr3kw636z.fsf@grenoble-inp.fr","threadId":"40563","inReplyTo":"CACbztPOSZBaqLLUERS_yfvAEPmxS2PZs0fYTy7E_9HfLi9UGmA@mail.gmail.com","subject":"Re: [PATCH] Falis on commit --amend when already pushed","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2015-10-15T06:44:36Z","receivedAt":"2015-10-15T06:44:36Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Tomohiro Koana writes:\n\n> Hello all,\n>\n> I'm a third year student at the University of Tokyo and, in our\n> \"Diving into open-source software\" class, my friends and I decided to\n> work with git. Our final, hopefully, is contributing to git.\n\nWelcome on board :-). I give the same class to my students (in Ensimag,\nGrenoble, France). You can have a look at\nhttps://git.wiki.kernel.org/index.php/SmallProjectsIdeas for a list of\nideas of things you can do.\n\nThe first contact with an open-source community is usually hard (the\nquality expectation is much higher that your usual lab works), but you\nare going to learn a lot!\n\n> One improvement that we thought of was not letting users to amend\n> commit when the commit is already pushed to the remote server.\n\nThis is a good introduction, but not a good commit message. The commit\nmessage is not about what you \"thought\", but about what the commit is\ndoing, and more importantly _why_ it is doing that and doing it this\nway. See it as an argument like \"You should accept this patch\nbecause ....\" (even if you won't actually write it like this). Read some\nexisting messages (\"git log --no-merges\") to see what I mean.\n\nPlease, read Documentation/SubmittingPatches and\nDocumentation/CodingGuidelines in Git's source tree.\n\n> --- a/builtin/commit.c\n>\n> +++ b/builtin/commit.c\n>\n> @@ -32,6 +32,7 @@\n>\n> #include \"sequencer.h\"\n>\n> #include \"notes-utils.h\"\n>\n> #include \"mailmap.h\"\n>\n> +#include \"remote.h\"\n\nThe patch is whitespace-damaged (there are extra blank lines). Use\neither \"git send-email\" or http://submitgit.herokuapp.com/ to submit\nyour patches.\n\n> + stat_tracking_info(branch, &ours, &theirs, &full_base);\n>\n> + if (amend && ours == 0) {\n>\n> + die(_(\"This commit is already pushed to the remote -- cannot amend.\"));\n>\n> + }\n\nI don't know the API well enough so I can't say whether this correctly\ndetects already pushed branch, but this looks suspiciously simple. Are\nyou not just detecting the presence of a remote-tracking branch? What\nyou should do is to detect whether the remote-tracking branch contains\nthe current commit.\n\nAlso, this is clearly not acceptable in its current form: there are many\nvalid use-cases to amend an already-pushed commit, so you can't break\nthe flow of people using this. It must 1) be configurable, and 2) unless\nyou have a really good reason, backward-compatible by default.\n\nAlso, it lacks tests.\n\nActually, the idea you have is far, far more difficult than what you\nprobably thought.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"271713","messageId":"CACbztPMagPGhwKT4-eaQ2Ve95sACv=NpYSHixAgKxtGXe-mSCg@mail.gmail.com","threadId":"40563","inReplyTo":"vpqr3kw636z.fsf@grenoble-inp.fr","subject":"Re: [PATCH] Falis on commit --amend when already pushed","fromName":"Tomohiro Koana","fromEmail":"kntmhr.1221@gmail.com","sentAt":"2015-10-15T07:49:40Z","receivedAt":"2015-10-15T07:49:40Z","isPatch":true,"sender":{"key":"kntmhr.1221@gmail.com","avatar":null},"body":"Thank you for the reply.\n\nI've taken a look at the list for small project ideas and our idea was\non the list,\nwhich goes to show that what we are working on is reasonable enough :D\n\nWe'll read the documentations for git contributions thoroughly and\nsend a patch again.\n\nAs for the test, we're not sure how to write a test because it\ninvolves remote branches, so we'd love to have advice on it.\n\nOn Thu, Oct 15, 2015 at 3:44 PM, Matthieu Moy\n wrote:\n> Tomohiro Koana writes:\n>\n>> Hello all,\n>>\n>> I'm a third year student at the University of Tokyo and, in our\n>> \"Diving into open-source software\" class, my friends and I decided to\n>> work with git. Our final, hopefully, is contributing to git.\n>\n> Welcome on board :-). I give the same class to my students (in Ensimag,\n> Grenoble, France). You can have a look at\n> https://git.wiki.kernel.org/index.php/SmallProjectsIdeas for a list of\n> ideas of things you can do.\n>\n> The first contact with an open-source community is usually hard (the\n> quality expectation is much higher that your usual lab works), but you\n> are going to learn a lot!\n>\n>> One improvement that we thought of was not letting users to amend\n>> commit when the commit is already pushed to the remote server.\n>\n> This is a good introduction, but not a good commit message. The commit\n> message is not about what you \"thought\", but about what the commit is\n> doing, and more importantly _why_ it is doing that and doing it this\n> way. See it as an argument like \"You should accept this patch\n> because ....\" (even if you won't actually write it like this). Read some\n> existing messages (\"git log --no-merges\") to see what I mean.\n>\n> Please, read Documentation/SubmittingPatches and\n> Documentation/CodingGuidelines in Git's source tree.\n>\n>> --- a/builtin/commit.c\n>>\n>> +++ b/builtin/commit.c\n>>\n>> @@ -32,6 +32,7 @@\n>>\n>> #include \"sequencer.h\"\n>>\n>> #include \"notes-utils.h\"\n>>\n>> #include \"mailmap.h\"\n>>\n>> +#include \"remote.h\"\n>\n> The patch is whitespace-damaged (there are extra blank lines). Use\n> either \"git send-email\" or http://submitgit.herokuapp.com/ to submit\n> your patches.\n>\n>> + stat_tracking_info(branch, &ours, &theirs, &full_base);\n>>\n>> + if (amend && ours == 0) {\n>>\n>> + die(_(\"This commit is already pushed to the remote -- cannot amend.\"));\n>>\n>> + }\n>\n> I don't know the API well enough so I can't say whether this correctly\n> detects already pushed branch, but this looks suspiciously simple. Are\n> you not just detecting the presence of a remote-tracking branch? What\n> you should do is to detect whether the remote-tracking branch contains\n> the current commit.\n>\n> Also, this is clearly not acceptable in its current form: there are many\n> valid use-cases to amend an already-pushed commit, so you can't break\n> the flow of people using this. It must 1) be configurable, and 2) unless\n> you have a really good reason, backward-compatible by default.\n>\n> Also, it lacks tests.\n>\n> Actually, the idea you have is far, far more difficult than what you\n> probably thought.\n>\n> --\n> Matthieu Moy\n> http://www-verimag.imag.fr/~moy/\n"},{"id":"271716","messageId":"vpqpp0g4lcd.fsf@grenoble-inp.fr","threadId":"40563","inReplyTo":"CACbztPMagPGhwKT4-eaQ2Ve95sACv=NpYSHixAgKxtGXe-mSCg@mail.gmail.com","subject":"Re: [PATCH] Falis on commit --amend when already pushed","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2015-10-15T07:55:30Z","receivedAt":"2015-10-15T07:55:30Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Tomohiro Koana writes:\n\n> Thank you for the reply.\n\nPlease, do not top-post on this list. Look at how other people write\nemails and try to mimick them.\n\n> I've taken a look at the list for small project ideas and our idea was\n> on the list,\n> which goes to show that what we are working on is reasonable enough :D\n\nYou may also want to look at how Mercurial does this. They have a\ncomplete system to classify commits:\nhttps://www.mercurial-scm.org/wiki/Phases\n\nYou're not going to do something as ambitious for a school project, but\nit may give you hints on what can be done.\n\n> As for the test, we're not sure how to write a test because it\n> involves remote branches, so we'd love to have advice on it.\n\n\"remote\" does not have to be actually remote as in \"on another\ncomputer\". It can be another repository on the same machine.\n\nLoot at what other tests are doing in t/ (there's a README file there\ntoo). Start with\n\n cd t/\n ls t*push*.sh\n\nto get examples.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"}]}