patch, 2 partscompletion: complete format.coverLetter

Hi,

This is inspired by merge.defaultToUpstream. Disclaimer: I have an "fpm" alias for doing this (separate from "fp" for when I need to generate patches against some other branch), which I'd like to get rid of.

Thanks.

Ramkumar Ramachandra (2):
  completion: complete format.coverLetter
  format-patch: introduce format.defaultTo

 Documentation/config.txt               |  4 ++++
 builtin/log.c                          |  7 +++++++
 contrib/completion/git-completion.bash |  2 ++
 t/t4014-format-patch.sh                | 10 ++++++++++
 4 files changed, 23 insertions(+)

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 contrib/completion/git-completion.bash | 1 +
 1 file changed, 1 insertion(+)

Ramkumar Ramachandra wrote:

Junio: Please push this part via 'maint' independently.

A very common workflow for preparing patches involves working off a topic branch and generating patches against 'master' to send off to the maintainer. However, a plain

  $ git format-patch -o outgoing

is a no-op on a topic branch, and the user has to remember to specify 'master' explicitly everytime. Save the user the extra keystrokes by introducing format.defaultTo which can contain the name of a branch against which to base patches.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 Documentation/config.txt               |  4 ++++
 builtin/log.c                          |  7 +++++++
 contrib/completion/git-completion.bash |  1 +
 t/t4014-format-patch.sh                | 10 ++++++++++
 4 files changed, 22 insertions(+)

Hi,

Ramkumar Ramachandra wrote:

Not excited.  Two reasons:

 1. Most config settings are in noun form: e.g.,
    "[remote] pushDefault = foo".  That makes their names easy to guess
    and makes them easy to talk about: I set the default remote for
    pushing by changing the remote.pushdefault setting.

    '[url "<foo>"] insteadOf' is an exception to that and a bit of an
    aberration.

    This new '[format] defaultTo' repeats the same end-with-a-preposition
    mistake, while I think it would be better to learn from it.

 2. Wouldn't a more natural default be @{u}..HEAD instead of relying on
    the user to do the make-work of keeping a local branch that tracks
    master up to date?

Hope that helps, Jonathan

Jonathan Nieder wrote:

I agree that it's somewhat unconventional to allow people to put a <revision> as a configuration variable value, but I think it's useful. url.<url>.insteadOf is incredibly useful, for instance.

Like I said in my message to Junio, @{u} is not necessarily the "best" default for all workflows, although you can fill that into format.defaultTo.

Ramkumar Ramachandra <artagnon@gmail.com> writes:

Two points.

 - why is a single branch name sufficient?

 - is it a better option to simply default to @{u}, if one exists,
   instead of failing?

Junio C Hamano wrote:

It does accept a <revision>, so any form is allowed; but why would anyone want that in a format.defaultTo? I'm not sure we want to impose an artificial restriction on the configuration variable though.

I'm not sure @{u} is a good default. Personally, my workflow involves publishing my fork before sending out patches; mainly so that I can compare with @{u} when I do re-spins. People can put @{u} in format.defaultTo if it suits their workflow though.

Ramkumar Ramachandra <artagnon@gmail.com> writes:

I meant "a single branch" as opposed to "depending on what branch you are sending out, you may have to use a different upstream starting point", and a single "format.defaultTo" that does not read what your HEAD currently points at may not be enough.

Unless you set @{u} to this new configuration, in which case the choice becomes dynamic depending on the current branch, but

 - if that is the only sane choice based on the current branch, why
   not use that as the default without having to set the
   configuration?

 - Or if that is still insufficient, don't we need branch.*.forkedFrom
   that is different from branch.*.merge, so that different branches
   you want to show "format-patch" output can have different
   reference points?

After all, "format-patch" to send things out to upstream is like asking the other side to do a "rebase" you would do in your repository, so whatever "git rebase" that were too lazy to specify what the fork point was when applying may be a reasonable type-saver default. Yes, sometimes people need to rebase onto somewhere they did not fork from, but that is why they can give explicit $upstream and $onto to the command---I do not think it is any different for "format-patch".

On Mon, Jan 06, 2014 at 12:06:51PM -0800, Junio C Hamano wrote:

Yeah, I had similar thoughts. I personally use "branch.*.merge" as "forkedFrom", and it seems like we are going that way anyway with things like "git rebase" and "git merge" defaulting to upstream. But then there is "git push -u" and "push.default = upstream", which treats the upstream config as something else entirely.

So it seems like there is already some confusion, and either way we go, thisis making it worse to some degree (I do not blame Ram, but rather he has stumbled into a hidden sand pit that we have been building for the past few years... :).

I wonder if it is too late to try to clarify this dual usage. It kind of seems like the push config is "this is the place I publish to". Which, in many workflows, just so happens to be the exact same as the place you forked from. Could we introduce a new branch.*.pushupstream variable that falls back to branch.*.merge? Or is that just throwing more fuel on the fire (more sand in the pit in my analogy, I guess).

I admit I haven't thought it through yet, though. And even if it does work, it may throw a slight monkey wrench in the proposed push.default transition.

-Peff

On Mon, Jan 6, 2014 at 3:18 PM, Jeff King <peff@peff.net> wrote:

Just for more reference, I rarely use "branch.*.merge" as "forkedFrom". I typically want to use master as my target, but like Ram, I publish my changes elsewhere for safe keeping. I think in a typical, feature branch-based workflow @{u} would be nearly useless.

-John

John Szakmeister wrote:

I thought the idea of @{u} was that it represents which ref one typically wants to compare the current branch to. It is used by 'git branch -v' to show how far ahead or behind a branch is and used by 'git pull --rebase' to forward-port a branch, for example.

So a topic branch with @{u} pointing to 'master' or 'origin/master' seems pretty normal and hopefully the shortcuts it allows can make life more convenient.

It is *not* primarily about where the branch gets pushed. After all, in both the 'matching' and the 'simple' mode, "git push" does not push the current branch to its upstream @{u} unless @{u} happens to have the same name.

Hoping that clarifies, Jonathan

On Mon, Jan 6, 2014 at 3:42 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:

Is there an outline of this git workflow in the documentation somewhere? Do you save your work in a forked repo anywhere? If so, how do you typically save your work. I typically have my @{u} pointing to where I save my work. Perhaps I'm missing something important here, but I don't feel like the current command set and typical workflow (at least those in tutorials) leads you in that direction.

Here is one example:
   <https://www.atlassian.com/git/workflows#!workflow-feature-branch>

Then where does it get pushed?  Do you always specify where to save your work?

FWIW, I think the idea of treating @{u} as the eventual recipient of your changes is good, but then it seems like Git is lacking the "publish my changes to this other branch" concept.

Am I missing something? If there is something other than @{u} to represent this latter concept, I think `git push` should default to that instead. But, at least with my current knowledge, that doesn't exist--without explicitly saying so--or treating @{u} as that branch. If there's a better way to do this, I'd love to hear it!

Thanks!

-John

John Szakmeister <john@szakmeister.net> writes:

I see Ram who worked on landing the remote.pushdefault feature is CC'ed; this work was started in early April 2013 and your config and workflow may not have been adjusted to it.

John Szakmeister wrote:

That's why we invented remote.pushdefault and branch.*.pushremote. When you say

  $ git push

it automatically goes to the right remote instead of going to the place you fetched from. You can read up on how push.default interacts with this setting too, although I always recommend push.default = current.

On Mon, Jan 6, 2014 at 5:54 PM, Ramkumar Ramachandra <artagnon@gmail.com> wrote:

This was the piece that I was missing. I remember when remote.pushdefault was added, but questioned its usefulness because it just changes the remote that a branch is pushed to, not necessarily the name. I somehow missed the 'current' option for 'push.default'... which is surprising since I seem to spend an incredible amount of time looking at the git-config man page. I guess it's not a good idea to set 'push.default' to 'upstream' in this triangle workflow then, otherwise the branch name being pushed to will be 'branch.*.merge'. Is that correct? I just want to make sure I understand what's happening here.

Given this new found knowledge, I'm not sure it makes sense for `git status` to show me the status against the upstream vs. the publish location. The latter makes a little more sense to me, though I see the usefulness of either one.

Thanks for taking the time to explain this. I'm going to have to spend some more time with this configuration and see what the sticking points are.

-John

John Szakmeister wrote:

push.default = upstream does not support triangular workflows. See builtin/push.c:setup_push_upstream().

Currently, status information is only against @{u}; we haven't invented a @{publish} yet.

On Mon, Jan 06, 2014 at 03:29:57PM -0500, John Szakmeister wrote:

In my feature-branch development for git.git, my upstream is almost always origin/master[1]. However, sometimes feature branches have dependencies[2] on each other. Representing that via the "upstream" field makes sense, since that is what you forked from, and what you would want "git rebase" to start from.

-Peff

[1] I do not even have a local "master" branch for git.git work, as it
    would just be a pain to keep up to date. I am either working
    directly on a topic branch, or I am integrating in my own personal
    branch.

[2] You should try to minimize dependencies between feature branches, of
    course, but sometimes they simply form a logical progression of
    features.

Jeff King <peff@peff.net> writes:

Yeah, when I say "upstream", I never mean it as "where I publish". Your upstream is where you get others' work from.

For a "push to somewhere for safekeeping or other people to look at" triangular workflow, it does not make any sense to treat that "I publish there" place as an upstream (hence having branch.*.remote pointing at that publishing point). Once you stop doing that, and instead using branch.*.remote = origin, and branch.*.merge = master, where 'origin' is not your publishing point, @{u} will again start making sense, I think.

And I thought that is what setting "remote.pushdefault" to the publishing point repository was about.

On Mon, Jan 06, 2014 at 12:38:30PM -0800, Junio C Hamano wrote:

That's my thinking, as well, but it means the "upstream" push.default is nonsensical. I've thought that all along, but it seems like other people find it useful. I guess because they are in a non-triangular, non-feature-branch setup (I suppose you could think of a central-repo feature-branch workflow as a special form of triangular setup, where the remote is bi-directional, but the branch names are triangular).

If we want to declare "push -u" and "push.default=upstream" as tools for certain simple bi-directional workflows, that makes sense. But I suspect it may cause extra confusion when people make the jump to using a triangular workflow.

You _might_ treat it the same way we treat the upstream, in some special cases. For example, when you say "git status", it is useful to see how your topic and the upstream have progressed (i.e., do I need to pull from upstream?). But you may _also_ want to know how your local branch differs from its pushed counterpart (i.e., do I have unsaved commits here that I want to push up?).

So having two config options might help with that. Of course, your "push upstream" (or whatever you want to call it) does not logically have one value. You may push to several places, and would want to compare to each.

If that were sufficient, then we would just need "push.default = current", and not "upstream" (nor "simple"). I lobbied for that during the discussion, but people seemed to think that "upstream" was better/more useful. Maybe it was just because remote.pushdefault did not exist then.

-Peff

Jeff King <peff@peff.net> writes:

I do not think there is no "want to declare" involved. If I correctly recall how "push -u" came about, it was merely a way to appease those who complained that their new branch created by running "checkout -b branch origin/branch" has already set up the branch.*.remote and branch.*.merge configurations nicely for them and allow them to immediately go ahead and start using the centralized "I merge from their 'branch' and push to that", but when they create a new branch on their own and want to make the branch of the same name at the origin to be the "upstream", they have to futz with the configuration. The declaration was made long time ago when we started using @{upstream}, and there is no new extra confusion.

Correct; I am not saying "where do I publish" is never relevant. It is just it is not something useful for "format-patch" to use as the default fork-point.

Yes. But most likely, if you always push a single branch to multiple places, it won't be like you push it to only one of the places today and another one tomorrow, leaving everybody out of sync. Unless there is a site that is temporarily down, in which case that one may stay behind, the normal state would be that all of them point at the same commit (that is how I publish to multiple places anyway).

Yeah, I think in the 2.0 world with pushdefault (i.e. triangular), the default 'simple' turns into 'current'.

Jeff King wrote:

My issue with that is that I no idea where my branch is with respect to my forked upstream; I find that extremely useful when doing re-spins.

push.default = upstream is a bit of a disaster, in my opinion. I've advocated push.default = current on multiple occasions, and wrote the initial remote.pushDefault series with that configuration in mind.

We already have a branch.*.pushremote, and I don't see the value of branch.*.pushbranch (what you're referring to as pushupstream, I assume) except for Gerrit users. Frankly, I don't use full triangular workflows myself mainly because my prompt is compromised: when I have a branch.*.remote different from branch.*.pushremote, I'd like to see where my branch is with respect to @{u} and @{publish} (not yet invented); that's probably too much information to digest anyway, so I use central workflow (pointing to my fork) for each of my branches, except master (which points to Junio's repo).

We're transitioning to push.default = simple which is even simpler than current.

On Tue, Jan 07, 2014 at 03:40:56AM +0530, Ramkumar Ramachandra wrote:

Right. I think there are two separate relationships, and they are both shoe-horned into "upstream". The solution is to let them be configured separately (and fallback on each other as appropriate to make the burden less on the user).

Yeah, I agree with all of that.

Yes, "pushbranch" is probably a better name for what I am referring to. I agree that pushremote is probably enough for sane cases. I seem to recall that people advocating the "upstream" push-default thought that branch name mapping was a useful feature, but I might be mis-remembering. I will let those people speak up for the feature if they see fit; it seems somewhat crazy to me.

Yes, as two separate relationships, you would theoretically want to be able to see them separately (or simultaneously side by side). Whether exposing that in the prompt is too clunky, I don't know (I don't even show ahead/behind in my prompt, but rather prefer to query it when I care; I have a separate script that queries the ahead/behind against my publishing point, but it would be nice if git handled this itself).

Simpler in the sense that it is less likely to do something unexpected. But the rules are actually more complicated. Two examples:

  1. Imagine I make a feature branch "foo" forked off of origin/master, then
     "git push" with no arguments. The "current" scheme would go to
     "foo" on origin, but "upstream" would go to "master". Since they
     don't agree, "simple" will punt and tell me to be more specific.

  2. Imagine I have set my default push remote to "publish", am on
     master (forked from "origin/master") and I run "git push" without
     arguments. "current" would push to "master" on "publish". But
     "upstream" will complain, because we are not pushing to our
     upstream remote. I believe "simple" will therefore reject this.

In both cases, I think "current" does the sane thing, and "simple" makes things more complicated. The one saving grace it has is that it punts on these cases rather than potentially doing something destructive that the user did not intend.

-Peff

Jeff King <peff@peff.net> writes:

I think "branch mapping" you recall are for those who want to push their 'topic' to 'review/topic' or something like that. With Git post 7cdebd8a (Merge branch 'jc/push-refmap', 2013-12-27), I think "remote.*.push" can be used to implement that, by the way.

Same here. I do not bother a/b in prompt and comparison with publishing point is done with a custom script. It would be nice to have it natively, and @{publish} would be a good first step to do so.

On Tue, Jan 07, 2014 at 01:07:08PM -0800, Junio C Hamano wrote:

Hmm. The top patch of that series still relies on "upstream" being a push destination, though. So if I have a triangular workflow where I fork "topic" from "origin/master", my "git push origin topic" will go to "refs/heads/master" on "origin" under the "upstream" rule. So that seems broken as ever. :)

But I guess what you are referring to is that in a triangular world, the second patch lets me do:

  git config push.default current
  git config remote.origin.push 'refs/heads/*:refs/review/*'

And then "git push", "git push origin", or "git push origin topic" all put it in "review/topic", which is what I want.

I think that is sensible, and only heightens my sense of the "upstream" push.default as useless. :)

-Peff

Jeff King <peff@peff.net> writes:

Yes, it only is good for centralized world (it was designed back in the centralized days after all, wasn't it?).

On Tue, Jan 07, 2014 at 02:06:12PM -0800, Junio C Hamano wrote:

I do not think there is any "centralized days". From day one, Linus advocated a triangular workflow, and that is how git and kernel develop has always been done. And that is why the default of "matching" was there.

There were people who came later, and who still exist today, who use git in an SVN-like centralized way. So if there were centralized days, we are in them now. :)

I just do not see any real advantage even in a centralized world for "upstream" versus "current". Before remote.pushdefault, I can potentially see some use (if you want to abuse @{upstream}), but now I do not see any point.

And even in a centralized workflow, I see "upstream" creating problems. E.g., you fork a feature branch in the centralized repo; it should not get pushed straight back to "master"! And that is why we invented "simple", to prevent such things.

I dunno. I have not gone back and read all of the arguments around push.default from last year. It is entirely possible everything I just said was refuted back then, and I am needlessly rehashing old arguments. I remember that Matthieu was one of the main advocates of "upstream". I am cc-ing him here to bring his attention (not just to this message, but to the whole thread).

-Peff

Jeff King <peff@peff.net> writes:

Oh, don't get me wrong. I personally wouldn't imagine forking 'topic' from the shared 'master', call the result perfect and push it directly back to the shared 'master'. But the 'upstream' setting was added exactly to support that.

In such a case, I would have 'master' that is forked from the shared 'master', 'topic' that is forked from my 'master', and pushing back would be a two-step process, first updating my 'master' in sync with the shared 'master', merging 'topic' into it to make sure the result is sane and then push it back to the shared 'master'. And in that set-up, 'upstream' would work fine as the upstream of my 'master' is the shared 'master', even though 'current' or even 'matching' would work just as well. So in that sense, I do not see 'upstream' as so broken as you seem to be saying.

One gap in that line of thought might be that I am sane enough not to attempt "git push" while I am on my 'topic', though.

Ramkumar Ramachandra wrote:

@{publish} not yet invented? I sent this back in October:

http://article.gmane.org/gmane.comp.version-control.git/235981

Junio C Hamano wrote:

Ah, I was going for an equivalent of merge.defaultToUpstream, but I guess branch.*.forkedFrom is a good way to go.

Ramkumar Ramachandra <artagnon@gmail.com> writes:

As I said in the different subthread, I am not convinced that you would need the complexity of branch.*.forkedFrom. If you set your "upstream" to the true upstream (not your publishing point), and have "remote.pushdefault" set to 'publish', you can expect

	git push

to do the right thing, and then always say

	git show-branch publish/topic topic

to see where your last published branch is relative to what you have, no? Mapping "topic@{publish}" to "refs/remotes/publish/topic" (or when you have 'topic' checked out, mapping "@{publish}" to it) is a trivial but is a separate usefulness, I would guess.

Junio C Hamano wrote:.

I think it's highly sub-optimal to have a local-branch @{u} for several reasons; the prompt is almost useless in this case, and it will always show your forked-branch ahead of 'master' (assuming that 'master' doesn't update itself in the duration of your development). While doing respins, the prompt doesn't aid you in any way. Besides, on several occasions, I found myself working on the same forked-branch from two different machines; then the publish-point isn't necessarily always a publish-point: it's just another "upstream" for the branch. The point of a special branch.*.forkedFrom is that you can always show the "master..@" information in the prompt ignoring divergences; after all, a divergence simply means that you need to rebase onto the latest 'master' ('master' is never going to get a non-ff update anyway).

So, instead of crippling the functionality around the publish-point, let's build minimal required functionality around branch.*.forkedFrom. I'd love a prompt like:

  branch-forkedfrom<>5*:~/src/git$

Clearly, branch-forkedfrom has diverged from my publish-point (I'm probably doing a respin), and has 5 commits (it's 5 commits ahead of 'master' ignoring divergences).

I think a @{publish} is needed for when branch.*.remote is different from remote.pushDefault. Like I said earlier, it's probably too much information to give to the user: divergences with respect to two remotes. So, we'll hold it off until someone finds a usecase for it.

On Tue, Jan 07, 2014 at 04:17:00AM +0530, Ramkumar Ramachandra wrote:

I actually use local-branch @{u} all the time to represent inter-topic dependencies. For example, imagine I have topic "bar" which builds on topic "foo", which is based on master. I would have:

  [branch "foo"]
    remote = origin
    merge = refs/heads/master
  [branch "bar"]
    remote = .
    merge = refs/heads/foo

When I rebase "foo", I want to rebase it against upstream's master. When I rebase "bar", I want to rebase it against foo. And naturally, upstream does not necessarily have a "foo", because it is my topic, not theirs (I _may_ have published my "foo" somewhere, but that is orthogonal, and anyway my local "foo" is the most up-to-date source, not the pushed version).

As an aside, if you want to rebase both branches, you have to topo-sort them to make sure you do "foo" first, then rebase "bar" on the result. My daily procedure is something like:

  all_topics |
  while read topic; do "echo $topic $(git rev-parse $topic@{u})"; done |
  topo_sort |
  while read topic upstream; do
    git rebase $upstream $topic || exit 1
  done

Right, things get trickier then. But I don't think there is an automatic way around that. Sometimes the published one is more up to date, and sometimes the upstream thing is more up to date. You have to manually tell git which you are currently basing your work on. I find in such a situation that it tends to resolve itself quickly, though, as the first step is to pull in the changes you pushed up from the other machine anyway (either via "git reset" or "git rebase").

-Peff

Jeff King wrote:

Ah, I was perhaps over-specializing for my own usecase, where everything is based off 'master'. I never considered 'master' a "true upstream" because I throw away topic branches after the maintainer merges them. If you have long-running branches that you work on a daily basis, the issue is somewhat different.

My primary concern is that the proposed @{publish} should be a first-class citizen; if it has everything that @{u} has, then we're both good: you'd primarily use @{u}, while I'd primarily use @{publish}.

On Wed, Jan 08, 2014 at 02:55:04AM +0530, Ramkumar Ramachandra wrote:

What I do is maybe somewhat gross, but I continually rebase my patches forward as master develops. So they diverge from where Junio has forked them upstream (which does not necessarily have any relationship with where I forked from, anyway). The nice thing about this is that eventually the topic becomes empty, as rebase drops patches that were merged upstream (or resolve conflicts to end up at an empty patch).

It's a nice way of tracking the progress of the patch upstream, and it catches any differences between what's upstream and what's in the topic (in both directions: you see where the maintainer may have marked up your patch, and you may see a place where you added something to be squashed but the maintainer missed it). The downside is that sometimes the conflicts are annoying and complicated (e.g., several patches that touch the same spot are a pain to rebase on top of themselves; the early ones are confused that the later changes are already in place).

Definitely. I think that's the world we want to work towards.

-Peff

Ramkumar Ramachandra wrote:

Something like this?

http://article.gmane.org/gmane.comp.version-control.git/246038

Ramkumar Ramachandra wrote:

Any thoughts on checkout.defaultTo? I have a "com" alias to checkout 'master'.