[PATCH] format-patch: print format-patch usage if there are no arguments

[PATCH] format-patch: print format-patch usage if there are no arguments

[Alexander Kuleshov]

Signed-off-by: Alexander Kuleshov <kuleshovmail@gmail.com>
---
 builtin/log.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/builtin/log.c b/builtin/log.c
index ad3cfd8..4431b50 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1246,6 +1246,10 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 		OPT_END()
 	};
 
+	/* We need at least one revision */
+	if (argc == 1)
+		usage_with_options(builtin_format_patch_usage, builtin_format_patch_options);
+
 	extra_hdr.strdup_strings = 1;
 	extra_to.strdup_strings = 1;
 	extra_cc.strdup_strings = 1;
-- 
2.3.0.rc0.239.g0ae1f56.dirty

Re: [PATCH] format-patch: print format-patch usage if there are no arguments

[Junio C Hamano]

Why?

Re: [PATCH] format-patch: print format-patch usage if there are no arguments

[Alexander Kuleshov]

Hello Junio,

As some commands does it when they are executed without arguments,
like git config, git blame and etc...

2015-01-14 0:43 GMT+06:00 Junio C Hamano <gitster@pobox.com>:
> Why?



-- 
_________________________
0xAX

Re: [PATCH] format-patch: print format-patch usage if there are no arguments

[Junio C Hamano]

Alexander Kuleshov <kuleshovmail@gmail.com> writes:

> 2015-01-14 0:43 GMT+06:00 Junio C Hamano <gitster@pobox.com>:
>> Why?
>
> As some commands does it when they are executed without arguments,
> like git config, git blame and etc...

For format-patch, I think the current behaviour is more of the lack
of implementation of the obvious default.  "git blame" does not have
any obvious default (would there be a single file you would want to
dig the history when the user does not tell you which one?  No), so
it proabaly is the right thing to do to error out.

But I would not surprised if those who build on top of others' work
to wish that "git format-patch" that was invoked without argument on
a branch created by "git checkout -t -b" to default to format commits
since the branch forked from @{upstream}, for example.

Re: [PATCH] format-patch: print format-patch usage if there are no arguments

[Stefan Beller]

On Tue, Jan 13, 2015 at 11:17 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Alexander Kuleshov <kuleshovmail@gmail.com> writes:
>
>> 2015-01-14 0:43 GMT+06:00 Junio C Hamano <gitster@pobox.com>:
>>> Why?
>>
>> As some commands does it when they are executed without arguments,
>> like git config, git blame and etc...
>
> For format-patch, I think the current behaviour is more of the lack
> of implementation of the obvious default.  "git blame" does not have
> any obvious default (would there be a single file you would want to
> dig the history when the user does not tell you which one?  No), so
> it proabaly is the right thing to do to error out.
>
> But I would not surprised if those who build on top of others' work
> to wish that "git format-patch" that was invoked without argument on
> a branch created by "git checkout -t -b" to default to format commits
> since the branch forked from @{upstream}, for example.

My initial reaction was to assume HEAD^ if no arguments are given to
format-patch. Though after thinking about it, the behavior you're
proposing makes more sense.

Another idea would be to take the first commit which is pointed to by
another branch as the first commit in the commit range. I am currently
thinking about the atomic patch series which I developed on top of
origin/mh/reflog-expire and that would be the first branch you find if
you just go back in history and look for any branches. Usually that
would be the upstream tracking branch though, so this would be an
extension to your proposal.

Though I am not sure if this scales. (Who has many branches anyway? ;)

Re: [PATCH] format-patch: print format-patch usage if there are no arguments

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> Another idea would be to take the first commit which is pointed to by
> another branch as the first commit in the commit range.

Trying to figure out what happened from the topology of the history
is certainly attractive proposition, but I suspect that it would be
too fragile to be practically useful.  The other topic you may have
based your work on may have advanced independently (e.g. a "oops, I
forgot this one" bugfix), resulting in a fork like this:

 ---o---o---o---o (fixed reflog-expire)
             \
              x---x---x (atomic-push)

and the fork-point is no longer at the tip of anything.  "Excluding
anything that is on another branch" would cover the forked case much
better, but that is only true if there is no integration branch like
'pu' or 'next', in which case, only an early part of atomic-push may
already be part of another branch 'next' while the remainder is not,
or the entirety of atomic-push may be part of another branch 'pu'.

On the other hand, "I am forked from building on this one" done with
"checkout -t" is an explicit mark the user leaves, so it would serve
as a better hint to base the default heuristics on, I think.

But nobody is asking for such a feature ;-)

Re: [PATCH] format-patch: print format-patch usage if there are no arguments

[Jeff King]

On Tue, Jan 13, 2015 at 02:28:13PM -0800, Junio C Hamano wrote:

> On the other hand, "I am forked from building on this one" done with
> "checkout -t" is an explicit mark the user leaves, so it would serve
> as a better hint to base the default heuristics on, I think.
> 
> But nobody is asking for such a feature ;-)

FWIW, I very rarely run format-patch directly, but have a wrapper script
that dumps the patches into a tempfile and runs mutt. I taught it in
2007 to use the upstream branch as the default[1], and was puzzled
reading the start of this thread, thinking we already did that.

So that is perhaps not asking for the feature (I am already happy with
my homegrown wrapper), but is maybe an endorsement of it. :)

-Peff

[1] You may note in 2007 that we did not even have @{upstream}. I
    implemented it manually using git-config! Then in 2009, I switched
    it to use for-each-ref's "%(upstream)" placeholder. Literally 5 days
    later, Dscho introduced @{upstream}, but I never got around to
    switching. Maybe now it is time. :)

Re: [PATCH] format-patch: print format-patch usage if there are no arguments

[Stefan Beller]

On Tue, Jan 13, 2015 at 2:45 PM, Jeff King <peff@peff.net> wrote:
> On Tue, Jan 13, 2015 at 02:28:13PM -0800, Junio C Hamano wrote:
>
>> On the other hand, "I am forked from building on this one" done with
>> "checkout -t" is an explicit mark the user leaves, so it would serve
>> as a better hint to base the default heuristics on, I think.
>>
>> But nobody is asking for such a feature ;-)
>
> So that is perhaps not asking for the feature (I am already happy with
> my homegrown wrapper), but is maybe an endorsement of it. :)
>

Would you mind to share (parts of) the wrapper script? We could see if that
makes sense to incorporate into format-patch as well.

Thanks,
Stefan

my hacky mutt-specific format-patch workflow

[Jeff King]

[retitling in case there is a wider audience]

On Tue, Jan 13, 2015 at 02:48:15PM -0800, Stefan Beller wrote:

> Would you mind to share (parts of) the wrapper script? We could see if that
> makes sense to incorporate into format-patch as well.

Sure, but I'll warn you it's kind of gross and specific to my workflow. :)

The script (which I call mpatch) is at the end of this message. The main
thing which may be of interest to other people is the cover-letter
handling. I write my cover letters in my normal MUA (mutt), and then
generate the series as a reply to that. The two ways it supports that
are:

  1. It generates the list of patches that I include in my cover letter
     from the mbox.  This _should_ be a "git log" one-liner, but our
     --pretty placeholders do not know how to count. Ideally I could say
     "[%i/%n]: %s", but neither of those first two exist (and %n would
     require counting all of the commits first, which might require some
     surgery to git-log).

  2. Given an existing message, it will pick out the to, cc, and
     message-id headers, and generate corresponding --to, --cc, and
     --in-reply-to arguments to feed to format-patch. I do it hackily in
     perl, but probably format-patch could do it internally if it built
     on git-mailinfo.

I typically have my MUA in one terminal and a shell running git in the
other. Even though I could run mpatch directly from the MUA, it is often
missing the context of which repo I am working in. So instead, I
typically use ~/patch as a go-between for the two sessions (both for
generating patch series, but also for applying with git-am). So I have
this in my muttrc:

  macro pager,index D '<shell-escape>rm -f $HOME/patch<enter>'
  macro pager,index A '<copy-message>~/patch<enter><enter>'

Applying a patch series from the list is just 'D' to clear the state,
then 'A' to collect whatever patches. And then I "git am ~/patch" from
the terminal window.

Generating a patch series is more like:

  1. "mpatch >~/patch" from the git terminal to generate the list of
      commits. No arguments necessary, because it uses @{upstream} as
      the base (but occasionally I use "-3" or similar if I am sending
      out new patches on top of somebody else's work).

  2. Reply or start a new thread in mutt, as normal. This becomes the
     cover letter (the to/cc comes from the reply, or I have mutt
     aliases for the list and frequent contributors, and/or I may cut
     and paste from "git log" in some cases).  Mutt dumps me in vim to
     write the actual message, and I ":r ~/patch" to pull it in.

  3. Finish and send off the cover letter. This gives it a message-id.

  4. Drop the newly-sent message into ~/patch. I usually just open my
     sent folder and use 'D', 'A' to copy it there.

  5. "mpatch" from the git terminal. The headers are picked up from
     ~/patch. Sometimes I use "-v2", which is passed to format-patch
     to get "[PATCH v2 i/n]".

After that, I'm in mutt with an mbox full of the patches. I have this
hotkey in mutt:

  macro index,pager b ":set edit_headers=yes<enter><resend-message>:set edit_headers=no<enter>"

which dumps me vim, with the headers. From there I give a final
proofread, write any comments below the "---", and then send it.

I imagine for the last bit many people have a similar workflow without
mutt that is something like:

  1. format-patch into ~/patch/*

  2. $EDITOR ~/patch/*

  3. git send-email ~/patch/*

Those people would potentially benefit from the format-patch in step 1
picking up the headers from an existing message.

-Peff

-- >8 --
#!/bin/sh

upstream_branch() {
  current=`git symbolic-ref HEAD`
  upstream=`git for-each-ref --format='%(upstream)' "$current"`
  if test -n "$upstream"; then
    echo $upstream
  else
    echo origin
  fi
}

get_reply_headers() {
  perl -ne '
    if (defined $opt && /^\s+(.*)/) {
      $val .= " $1";
      next;
    }
    if (defined $opt) {
      print "--$opt=", quotemeta($val), " ";
      $opt = $val = undef;
    }
    if (/^(cc|to):\s*(.*)/i) {
      $opt = lc($1);
      $val = $2;
    }
    elsif (/^message-id:\s*(.*)/i) {
      $opt = "in-reply-to";
      $val = $1;
    }
  '
}

has_nonoption=
for i in "$@"; do
  case "$i" in
    -[0-9]) has_nonoption=yes ;;
    -*) ;;
     *) has_nonoption=yes
  esac
done

: ${REPLY:=$HOME/patch}
test -n "$REPLY" && eval "set -- `get_reply_headers <\"$REPLY\"` \"\$@\""
test "$has_nonoption" = "yes" || set -- "$@" `upstream_branch`

git format-patch -s --stdout --from "$@" >.mbox
if test -t 1; then
  mutt -f .mbox
else
  perl -lne '
    if (/^Subject: (.*)/) {
      $subject = $1;
    }
    elsif ($subject && /^\s+(.*)/) {
      $subject .= " $1";
    }
    elsif ($subject) {
      print $subject;
      $subject = undef;
    }
  ' .mbox |
  sed -e 's/\[PATCH /[/' \
      -e 's/]/]:/' \
      -e 's/^/  /'
fi
rm -f .mbox

Re: [PATCH] format-patch: print format-patch usage if there are no arguments

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> So that is perhaps not asking for the feature (I am already happy with
> my homegrown wrapper), but is maybe an endorsement of it. :)

OK.  A patch to add this should be reasonably clean and trivial, I
would guess.