git_cl: Enable updating description from local.

git_cl: Enable updating description from local.

Currently, if one edits one's local CL description (e.g., `git commit
--amend`), there is no path for that update to get pushed to Rietveld.
This adds a "+" sentinel to the "-n" flag for `git cl description` that
tells it to load description content from the local commit.

The description should match what would be generated by "git cl upload".

BUG=None
TEST=local

Committed: https://chromium.googlesource.com/chromium/tools/depot_tools/+/ba1b0f37956143b9b53c8b0f9b21dc4703645e8f

[dnj, 2016-09-01 06:29:32]

Patchset #1 (id:1) has been deleted

[dnj, 2016-09-01 15:26:33]

dnj@chromium.org changed reviewers:
+ agable@chromium.org

[dnj, 2016-09-01 15:26:34]

PTAL!

[tandrii(chromium), 2016-09-01 15:34:45]

tandrii@chromium.org changed reviewers:
+ tandrii@chromium.org

[tandrii(chromium), 2016-09-01 15:34:46]

I actually like this very much.

Does Test=local entail testing for Gerrit?

See tests/git_cl.py which might be daunting but you can copy and adapt test
cases for testing various upload options.

[dnj, 2016-09-01 16:09:33]

On 2016/09/01 15:34:46, tandrii(chromium) wrote:
> I actually like this very much.
> 
> Does Test=local entail testing for Gerrit?
> 
> See tests/git_cl.py which might be daunting but you can copy and adapt test
> cases for testing various upload options.

No, "TEST=local" is my way of saying that I tested this locally.

[dnj, 2016-09-02 15:44:17]

(ping)

[agable, 2016-09-02 16:43:00]

https://codereview.chromium.org/2307463002/diff/20001/git_cl.py
File git_cl.py (right):

https://codereview.chromium.org/2307463002/diff/20001/git_cl.py#newcode3626
git_cl.py:3626: text = CreateDescriptionFromLog([merge_base])
This produces a commit message which is not of the standard
"""
Single short line

Paragraph of more descriptive text which may
take up more than one line, but is separated
from the first line by a single blank line.
"""
format.

[dnj, 2016-09-02 17:31:40]

https://codereview.chromium.org/2307463002/diff/20001/git_cl.py
File git_cl.py (right):

https://codereview.chromium.org/2307463002/diff/20001/git_cl.py#newcode3626
git_cl.py:3626: text = CreateDescriptionFromLog([merge_base])
On 2016/09/02 16:43:00, agable wrote:
> This produces a commit message which is not of the standard
> """
> Single short line
> 
> Paragraph of more descriptive text which may
> take up more than one line, but is separated
> from the first line by a single blank line.
> """
> format.

I don't think this is correct. When I run the command, I get the same commit
message that is in the log, which is what I want this to do. I hacked this CL to
print the "text" return value, and I get:

$ git cl description -n +

git_cl: Enable updating description from local.

Currently, if one edits one's local CL description (e.g., `git commit
--amend`), there is no path for that update to get pushed to Rietveld.
This adds a "+" sentinel to the "-n" flag for `git cl description` that
tells it to load description content from the local commit.

BUG=None
TEST=local

[agable, 2016-09-02 17:43:39]

https://codereview.chromium.org/2307463002/diff/20001/git_cl.py
File git_cl.py (right):

https://codereview.chromium.org/2307463002/diff/20001/git_cl.py#newcode3626
git_cl.py:3626: text = CreateDescriptionFromLog([merge_base])
On 2016/09/02 at 17:31:40, dnj wrote:
> On 2016/09/02 16:43:00, agable wrote:
> > This produces a commit message which is not of the standard
> > """
> > Single short line
> > 
> > Paragraph of more descriptive text which may
> > take up more than one line, but is separated
> > from the first line by a single blank line.
> > """
> > format.
> 
> I don't think this is correct. When I run the command, I get the same commit
message that is in the log, which is what I want this to do. I hacked this CL to
print the "text" return value, and I get:
> 
> $ git cl description -n +
> 
> git_cl: Enable updating description from local.
> 
> Currently, if one edits one's local CL description (e.g., `git commit
> --amend`), there is no path for that update to get pushed to Rietveld.
> This adds a "+" sentinel to the "-n" flag for `git cl description` that
> tells it to load description content from the local commit.
> 
> BUG=None
> TEST=local

Ah. We develop differently. I often have 2-10 commits on a branch, and none of
them have a full commit description like gets uploaded to review. That only
comes into existence when I do git-cl-upload. For me, this produces a
conglomeration of all those one-line commit messages, one after the other. This
is really only useful if you only have a single commit on the branch.

Maybe this should only upload the latest commit message, as opposed to the log
back to the merge-base?

[tandrii(chromium), 2016-09-02 17:45:42]

1. I understood that you tested it locally. But you haven't answered if that
testing involved uploading to Gerrit.

2. For Rietveld, I believe you can achieve the same thing by your own script:
alias xxxx = git cl upload && (git show HEAD <some format opts> | git cl desc -n
-)

3. How will this work if you have several commits on the branch?

[dnj, 2016-09-02 18:05:48]

On 2016/09/02 17:45:42, tandrii(chromium) wrote:
> 1. I understood that you tested it locally. But you haven't answered if that
> testing involved uploading to Gerrit.

I misunderstood your question. This follows the same code path as "git cl
description", so I don't see why Gerrit is strictly relevant. No, I did not
explicitly test this with Gerrit.

> 2. For Rietveld, I believe you can achieve the same thing by your own script:
> alias xxxx = git cl upload && (git show HEAD <some format opts> | git cl desc
-n
> -)

Sure, I think half of our tooling could be some alias or another. I thought this
was useful so I'd make a CL. Do you think that this is not worth adding as a
feature?

> 3. How will this work if you have several commits on the branch?

It does exactly what "git cl upload" would do when generating a change.

[dnj, 2016-09-02 18:05:55]

https://codereview.chromium.org/2307463002/diff/20001/git_cl.py
File git_cl.py (right):

https://codereview.chromium.org/2307463002/diff/20001/git_cl.py#newcode3626
git_cl.py:3626: text = CreateDescriptionFromLog([merge_base])
> Ah. We develop differently. I often have 2-10 commits on a branch, and none of
> them have a full commit description like gets uploaded to review. That only
> comes into existence when I do git-cl-upload. For me, this produces a
> conglomeration of all those one-line commit messages, one after the other.
This
> is really only useful if you only have a single commit on the branch.
> 
> Maybe this should only upload the latest commit message, as opposed to the log
> back to the merge-base?

I revised this to do the same thing "git cl upload" does, so this should now be
consistent with that expectation.

[agable, 2016-09-02 18:16:50]

I just patched this in and tried it and it updated the description to be "+".

[dnj, 2016-09-02 18:24:45]

Description was changed from

==========
git_cl: Enable updating description from local.

Currently, if one edits one's local CL description (e.g., `git commit
--amend`), there is no path for that update to get pushed to Rietveld.
This adds a "+" sentinel to the "-n" flag for `git cl description` that
tells it to load description content from the local commit.

BUG=None
TEST=local
==========

to

==========
git_cl: Enable updating description from local.

Currently, if one edits one's local CL description (e.g., `git commit
--amend`), there is no path for that update to get pushed to Rietveld.
This adds a "+" sentinel to the "-n" flag for `git cl description` that
tells it to load description content from the local commit.

The description should match what would be generated by "git cl upload".

BUG=None
TEST=local
==========

[dnj, 2016-09-02 18:25:06]

On 2016/09/02 18:16:50, agable wrote:
> I just patched this in and tried it and it updated the description to be "+".

I don't see how that's possible, since this explicitly changes the "text"
variable if it's "+". I think you patched it incorrectly.

[agable, 2016-09-02 18:33:28]

On 2016/09/02 at 18:25:06, dnj wrote:
> On 2016/09/02 18:16:50, agable wrote:
> > I just patched this in and tried it and it updated the description to be
"+".
> 
> I don't see how that's possible, since this explicitly changes the "text"
variable if it's "+". I think you patched it incorrectly.

I used 'git cl patch'. But you're right, something must have gone wrong (ugh I
don't want to file more bugs against git-cl); it worked this time.

Yes, it now uses the same system as "git cl upload"... but git cl upload then
opens that text in an editor.

I'll LGTM this CL because it's clearly useful to some people, and doesn't harm
my workflow at all, but I still think it's useless (or worse, dangerous, because
older commits will appear below the most recent commit's BUG= line, making it
invalid) for anyone with more than one commit on their local branch.

[dnj, 2016-09-02 18:39:28]

> I'll LGTM this CL because it's clearly useful to some people, and doesn't harm
> my workflow at all, but I still think it's useless (or worse, dangerous,
because
> older commits will appear below the most recent commit's BUG= line, making it
> invalid) for anyone with more than one commit on their local branch.

This is the first time I have heard your impression that this is a bad CL. If
you think I should just trash it, feel free to say so.

I put this here because it's useful to me. I frequently change CL descriptions
during code review in response to code review comments, and I find syncing local
commit w/ code review description is a pain. This lets me set my local commit
message and then push it. If this is a problem that I uniquely have, I'll go
ahead and write my own hacks around it. If you think this is something
generically useful, I'll go ahead and commit.

[agable, 2016-09-02 18:51:01]

On 2016/09/02 at 18:39:28, dnj wrote:
> > I'll LGTM this CL because it's clearly useful to some people, and doesn't
harm
> > my workflow at all, but I still think it's useless (or worse, dangerous,
because
> > older commits will appear below the most recent commit's BUG= line, making
it
> > invalid) for anyone with more than one commit on their local branch.
> 
> This is the first time I have heard your impression that this is a bad CL. If
you think I should just trash it, feel free to say so.
> 
> I put this here because it's useful to me. I frequently change CL descriptions
during code review in response to code review comments, and I find syncing local
commit w/ code review description is a pain. This lets me set my local commit
message and then push it. If this is a problem that I uniquely have, I'll go
ahead and write my own hacks around it. If you think this is something
generically useful, I'll go ahead and commit.

No no, I don't think it's *bad*, mostly just not for me and potentially
confusing. Sorry if I made it sound more negative than I think it is.

I think that likely the most common use-case for this is
$ git cl patch <someone else's CL>
$ # hack
$ git commit -a --amend
$ git cl upload
$ git cl description
$ git cl set-commit

Speaking of which -- would it make sense to add a "let me update the
description" flag to git-cl-upload?

[dnj, 2016-09-02 19:30:57]

On 2016/09/02 18:51:01, agable wrote:
> On 2016/09/02 at 18:39:28, dnj wrote:
> > > I'll LGTM this CL because it's clearly useful to some people, and doesn't
> harm
> > > my workflow at all, but I still think it's useless (or worse, dangerous,
> because
> > > older commits will appear below the most recent commit's BUG= line, making
> it
> > > invalid) for anyone with more than one commit on their local branch.
> > 
> > This is the first time I have heard your impression that this is a bad CL.
If
> you think I should just trash it, feel free to say so.
> > 
> > I put this here because it's useful to me. I frequently change CL
descriptions
> during code review in response to code review comments, and I find syncing
local
> commit w/ code review description is a pain. This lets me set my local commit
> message and then push it. If this is a problem that I uniquely have, I'll go
> ahead and write my own hacks around it. If you think this is something
> generically useful, I'll go ahead and commit.
> 
> No no, I don't think it's *bad*, mostly just not for me and potentially
> confusing. Sorry if I made it sound more negative than I think it is.
> 
> I think that likely the most common use-case for this is
> $ git cl patch <someone else's CL>
> $ # hack
> $ git commit -a --amend
> $ git cl upload
> $ git cl description
> $ git cl set-commit
> 
> Speaking of which -- would it make sense to add a "let me update the
> description" flag to git-cl-upload?

Okay, I'll go ahead with this. Yes, I think that could be useful.

[dnj, 2016-09-02 19:31:01]

The CQ bit was checked by dnj@chromium.org

[commit-bot: I haz the power, 2016-09-02 19:31:09]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-02 19:37:45]

Description was changed from

==========
git_cl: Enable updating description from local.

Currently, if one edits one's local CL description (e.g., `git commit
--amend`), there is no path for that update to get pushed to Rietveld.
This adds a "+" sentinel to the "-n" flag for `git cl description` that
tells it to load description content from the local commit.

The description should match what would be generated by "git cl upload".

BUG=None
TEST=local
==========

to

==========
git_cl: Enable updating description from local.

Currently, if one edits one's local CL description (e.g., `git commit
--amend`), there is no path for that update to get pushed to Rietveld.
This adds a "+" sentinel to the "-n" flag for `git cl description` that
tells it to load description content from the local commit.

The description should match what would be generated by "git cl upload".

BUG=None
TEST=local

Committed:
https://chromium.googlesource.com/chromium/tools/depot_tools/+/ba1b0f37956143...
==========

[commit-bot: I haz the power, 2016-09-02 19:37:46]

Committed patchset #2 (id:40001) as
https://chromium.googlesource.com/chromium/tools/depot_tools/+/ba1b0f37956143...

[davidgreens, 2016-09-07 01:22:57]

On 2016/09/02 19:37:46, commit-bot: I haz the power wrote:
> Committed patchset #2 (id:40001) as
>
https://chromium.googlesource.com/chromium/tools/depot_tools/+/ba1b0f37956143...

<?xml version="1.0" encoding="UTF-8"?>
<rss version="2.0">
  <channel>
    <title>Hector David's Network Feed</title>
    <link>https://confluence.jetbrains.com</link>
    <description>An activity feed of the people being followed by Hector
David</description>
  </channel>
</rss>