threads / patch / 40563

patchFalis on commit --amend when already pushed

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

## tl;dr

4 messages between Oct 15, 2015 and Oct 15, 2015. Diffs are folded; open one to read it.

replies: 3people: 2as markdown or json

Tomohiro Koana· Oct 15, 2015, 04:09 UTC · lore

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.

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

Show changes to builtin/commit.c +10 −0

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



 static const char * const builtin_commit_usage[] = {

  N_("git commit [<options>] [--] <pathspec>..."),

@@ -1125,6 +1126,9 @@ static int parse_and_validate_options(int argc,
const char *argv[],

        struct wt_status *s)

 {

  int f = 0;

+ int ours, theirs;

+ const char *full_base;

+ struct branch *branch = branch_get(NULL);



  argc = parse_options(argc, argv, prefix, options, usage, 0);

  finalize_deferred_config(s);

@@ -1149,6 +1153,12 @@ static int parse_and_validate_options(int argc,
const char *argv[],

  else if (whence == FROM_CHERRY_PICK)

  die(_("You are in the middle of a cherry-pick -- cannot amend."));

  }

+

+ stat_tracking_info(branch, &ours, &theirs, &full_base);

+ if (amend && ours == 0) {

+ die(_("This commit is already pushed to the remote -- cannot amend."));

+ }

+

  if (fixup_message && squash_message)

  die(_("Options --squash and --fixup cannot be used together"));

  if (use_message)

Matthieu Moy· Oct 15, 2015, 06:44 UTC · re: Tomohiro Koana · lore

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

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/

Tomohiro Koana· Oct 15, 2015, 07:49 UTC · re: Matthieu Moy · lore

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

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@grenoble-inp.fr> wrote:

Show 75 quoted lines

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

Matthieu Moy· Oct 15, 2015, 07:55 UTC · re: Tomohiro Koana · lore

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

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

> Thank you for the reply.

Please, do not top-post on this list. Look at how other people write emails and try to mimick them.

> 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

You may also want to look at how Mercurial does this. They have a complete system to classify commits: https://www.mercurial-scm.org/wiki/Phases

You're not going to do something as ambitious for a school project, but it may give you hints on what can be done.

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

"remote" does not have to be actually remote as in "on another computer". It can be another repository on the same machine.

Loot at what other tests are doing in t/ (there's a README file there too). Start with

  cd t/
  ls t*push*.sh

to get examples.

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