submodule..ignore vs git add -u

[Miklos Vajna]

Hi,

Let's say I have a fairly simple submodule setup where I do 'git
checkout' inside the submodule to check out a different commit, so the
outer repo 'git diff' shows a submodule update.

In that case

git config submodule..ignore all

makes 'git diff' or 'git commit -a' ignore the change in the outer repo,
but not 'git add -u'.

Reading the git-config documentation if this is intentional behavior,
I'm a bit confused. It specifies that:

- "git status" and the diff family: handle this setting
- git submodule commands: ignore this setting

So that about 'git add -u', is it expected that it ignores this setting
as well?

I guess either the doc should say 'git add -u' doesn't handle this
setting or 'git add -u' should handle it. Happy to try to make a patch
that does the later, but I though better ask first. :-)

Thanks,

Miklos


signature.asc
Description: Digital signature

Re: Signed-off-by vs Reviewed-by

[Miklos Vajna]

Hi,

On Thu, Mar 31, 2016 at 09:28:44AM -0700, Junio C Hamano  
wrote:
> The internal "parse the existing trailer block and manipulate it by
> adding, conditionally adding, replacing and deleting it" logic was
> done as an experimental "interpret-trailers" program, but polishing
> it (both its design and implementation) and integrating it to the
> front-line programs (e.g. "git commit") hasn't been done.

I had a look at interpret-trailers, and one use-case I miss is: being
able to define a trailer type, but only add it when asked explicitly.

Example:


$ git config trailer.review.key "Reviewed-by: "
$ git config trailer.review.command 'echo "$(git config user.name) <$(git 
config user.email)>"'
$ echo foo|git interpret-trailers
foo

Reviewed-by: A U Thor 
$ echo foo|git interpret-trailers --trailer review
foo

Reviewed-by: A U Thor 


I can imagine e.g. a new configuration vaulue named
trailer..ifMissing explicit, and when that's set, the trailer
would be only added if it's spelled out explicitly using '--trailer
'.

Does this sound like a good idea, or did I miss some way how this is
already possible? :-)

> As to the last step of "integration", we cannot use short-and-sweet
> single letter options like '-s' (for sign-off) for each and every
> custom trailer different projects use for their own purpose (as
> there are only 26 of the lowercase ASCII alphabet letters), so the
> most general syntax for the option has to become "--trailer "
> or some variation of it, and at that point "-s" would look like a
> short-hand for "--trailer signed-off-by".

Hmm, I think the above has to be implemented first, otherwise it'll be
hard to make "-s" an alias of "--trailer signed-off-by". (I mean having
git understand what "signed-off-by" is, still adding it conditionally.)

Regards,

Miklos


signature.asc
Description: Digital signature

Re: Signed-off-by vs Reviewed-by

[Miklos Vajna]

Hi,

On Thu, Mar 31, 2016 at 07:54:47PM +0530, Pranit Bauva  
wrote:
> Are you suggesting to use a different email address for commiting,
> signing off and reviewing?

Let's say project A has a workflow where patch authors and maintainers
add a "Signed-off-by: A B " line. This is well-supported
by git, various commands have a -s option to add that line.

However, if project B has a workflow where patch authors add no such
line, and reviewers add a "Reviewed-by: A B " line, then
you have to add that line manually when you do a review.

I suggest to give a bit more support to this workflow in git. One way of
doing that would be to make the Signed-off-by string configurable. I can
look into implementing that, but first I wanted to discuss the idea here
on the list -- perhaps there is a better way to support that. :-)

Typing that line (including copy your name + email all the time)
is a bit boring.

Regards,

Miklos


signature.asc
Description: Digital signature

Signed-off-by vs Reviewed-by

[Miklos Vajna]

Hi,

Some projects like LibreOffice don't use Signed-off-by, instead usually
use Gerrit for code review, and reviewers add a Reviewed-by line when
they are OK with a patch.  In this workflow it's a bit unfortunate that
adding a Signed-off-by line is just a command-line switch, but adding a
Reviewed-by line is more complex.

Is there anything in git that could help this situation? I didn't see
any related config option; I wonder if a patch would be accepted to make
the "Signed-off-by" line configurable, or there is a better way.

Like, would a patch that adds e.g. a core.signedOffString configuration
option to make the string customizable welcome?

Thanks,

Miklos
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 02/25] contrib: remove 'hg-to-git'

[Miklos Vajna]

On Thu, May 08, 2014 at 07:58:13PM -0500, Felipe Contreras 
felipe.contre...@gmail.com wrote:
 There hasn't been any real activity on it since 2010.
 
 Plus there are better out-of-tree tools.
 
 No tests and no real documentation either.

ACK, git clone hg::... is what one is supposed to use today, I guess.


signature.asc
Description: Digital signature

git log history simplification problem

[Miklos Vajna]

Hi,

I was trying to understand the history of a piece of code in LibreOffice
and I'm facing a behaviour of git-log which is not something I can
explain. I'm not sure if this is a git bug or a user error. ;)

Here is the situation:

git clone git://anongit.freedesktop.org/libreoffice/core
cd core
git log --full-history -p -S'mnTitleBarHeight =' 
sd/source/ui/dlg/PaneDockingWindow.cxx

Here the first output I get from git-log is
b390fae1706b9c511158a03e4fd61f263be4e511, where you can see that the
commit *added* that string. So it should be there on master, I would
assume.

But then I run:

git grep 'mnTitleBarHeight =' sd

and it's not there. Am I missing something, as in e.g. even with
--full-history git-log does some simplification?

Thanks,

Miklos


signature.asc
Description: Digital signature

Re: git log history simplification problem

[Miklos Vajna]

On Tue, Feb 04, 2014 at 06:37:13PM +0100, Miklos Vajna 
vmik...@collabora.co.uk wrote:
 But then I run:
 
 git grep 'mnTitleBarHeight =' sd
 
 and it's not there. Am I missing something, as in e.g. even with
 --full-history git-log does some simplification?

I tried to reproduce this with a repo from scratch, and it seems my
problem is the following:

1) A creates a feature branch
2) A works on it, and in the meantime master progresses as well
3) A merges master to the feature branch
4) A does some additional changes, and -- in an evil way -- uses git
commit -a --amend to squeeze these into the merge commit
5) B (that's me) comes and try to find out where a string got deleted,
but can't.

Here is a reproducer script:


rm -rf scratch
mkdir scratch
cd scratch
git init
echo -e a\na\na\na\na\na\na\na\n  a
git add a
git commit -m init
git branch feature
echo b  a
git add a
git commit -m more master changes
git checkout feature
sed -i '1iXXX' a # insert first row
git add a
git commit -m feature
git merge -m merge master
sed -i '1d' a # delete first row
git add a
git commit --amend -m merge


I now know that the XXX got removed by the merge commit, but how can I
see it that I'm right? If I run 'git log --all -p' in the result, I see
that XXX got inserted by one commit, now I don't have it, but I don't
see any deletion, which confuses me.

Any ideas? :-)

Thanks,

Miklos


signature.asc
Description: Digital signature

Re: git log history simplification problem

[Miklos Vajna]

Hi Jonathan,

On Tue, Feb 04, 2014 at 11:48:42AM -0800, Jonathan Nieder jrnie...@gmail.com 
wrote:
 Luckily '-m -p' without --first-parent worked and the first commit it
 showed was the right one.  It produces more hits than I'd like, too,
 though.

Ah, excellent! :-) '-m' does what I need.

Thanks a lot,

Miklos


signature.asc
Description: Digital signature

Re: [PATCH] Documentation/Makefile: make AsciiDoc dblatex dir configurable

[Miklos Vajna]

On Thu, Oct 03, 2013 at 08:17:32PM +0100, John Keeping j...@keeping.me.uk 
wrote:
 On my system this is in /usr/share/asciidoc/dblatex not
 /etc/asciidoc/dblatex.  Extract this portion of the path to a variable
 so that is can be set in config.mak.

Sure, makes sense.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: What's cooking in git.git (Jul 2013, #03; Tue, 9)

[Miklos Vajna]

Hi,

On Tue, Jul 23, 2013 at 12:53:25PM +0530, Ramkumar Ramachandra 
artag...@gmail.com wrote:
 Junio C Hamano wrote:
  * mv/merge-ff-tristate (2013-07-02) 1 commit
(merged to 'next' on 2013-07-09 at c32b95d)
   + merge: handle --ff/--no-ff/--ff-only as a tri-state option
 
 Sorry I didn't notice sooner, but I was confused by the second test
 title this added:
 
 test_expect_success 'option --ff-only overwrites merge.ff=only config' '
   git reset --hard c0 
   test_config merge.ff only 
   git merge --no-ff c1
 '
 
 How is --ff-only overwriting merge.ff=only here?  Was it a typo?

Yes, it's a typo in the test name. Thanks for spotting that!

Miklos


signature.asc
Description: Digital signature

[PATCH v2] merge: handle --ff/--no-ff/--ff-only as a tri-state option

[Miklos Vajna]

This has multiple benefits: with more than one of {--ff, --no-ff,
--ff-only} respects the last option; also the command-line option to
always take precedence over the config file option.

Signed-off-by: Miklos Vajna vmik...@suse.cz
---
 builtin/merge.c  | 55 +--
 t/t7600-merge.sh | 12 +---
 2 files changed, 42 insertions(+), 25 deletions(-)

On Tue, Jul 02, 2013 at 10:42:39AM +0200, Michael Haggerty 
mhag...@alum.mit.edu wrote:
 You allow --no-ff-only but ignore it, which I think is incorrect.  In
 
 git merge --ff-only --no-ff-only [...]
 
 , the --no-ff-only should presumably cancel the effect of the previous
 --ff-only (i.e., be equivalent to --ff).  But it is a little bit
 subtle because
 
 git merge --no-ff --no-ff-only
 
 should presumably be equivalent to --no-ff.  So I think that
 --no-ff-only should do something like
 
 if (fast_forward == FF_ONLY)
 fast_forward = FF_ALLOW;

Do we really want --no-ff-only? I would rather just disable it, see the
updated patch.

 I'm no options guru, but I think it would be possible to implement --ff
 and --no-ff without callbacks if you choose constants such that
 FF_NO==0, something like:

Indeed, done.

 Should these do the same as the versions with the option order reversed?
  Or should the command line option that appears later take precedence?
 The latter implies that {--ff, --no-ff, --ff-only, --squash} actually
 constitute a single *quad-state* option, representing how the results
 of the merge should be handled, and, for example,
 
 git merge --squash --ff-only
 
 ignores the --squash option, and
 
 git merge --ff-only --squash
 
 ignores the --ff-only option.
 
 I'm not sure.

Actually there is also --no-squash, used by e.g. git-pull internally.
You definitely don't want a five-state option. :-) So for now I would
rather let --squash/--no-squash alone.

diff --git a/builtin/merge.c b/builtin/merge.c
index 2ebe732..149f32a 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -47,8 +47,8 @@ static const char * const builtin_merge_usage[] = {
 };
 
 static int show_diffstat = 1, shortlog_len = -1, squash;
-static int option_commit = 1, allow_fast_forward = 1;
-static int fast_forward_only, option_edit = -1;
+static int option_commit = 1;
+static int option_edit = -1;
 static int allow_trivial = 1, have_message, verify_signatures;
 static int overwrite_ignore = 1;
 static struct strbuf merge_msg = STRBUF_INIT;
@@ -76,6 +76,14 @@ static struct strategy all_strategy[] = {
 
 static const char *pull_twohead, *pull_octopus;
 
+enum ff_type {
+   FF_NO,
+   FF_ALLOW,
+   FF_ONLY
+};
+
+static enum ff_type fast_forward = FF_ALLOW;
+
 static int option_parse_message(const struct option *opt,
const char *arg, int unset)
 {
@@ -178,6 +186,13 @@ static int option_parse_n(const struct option *opt,
return 0;
 }
 
+static int option_parse_ff_only(const struct option *opt,
+ const char *arg, int unset)
+{
+   fast_forward = FF_ONLY;
+   return 0;
+}
+
 static struct option builtin_merge_options[] = {
{ OPTION_CALLBACK, 'n', NULL, NULL, NULL,
N_(do not show a diffstat at the end of the merge),
@@ -194,10 +209,10 @@ static struct option builtin_merge_options[] = {
N_(perform a commit if the merge succeeds (default))),
OPT_BOOL('e', edit, option_edit,
N_(edit message before committing)),
-   OPT_BOOLEAN(0, ff, allow_fast_forward,
-   N_(allow fast-forward (default))),
-   OPT_BOOLEAN(0, ff-only, fast_forward_only,
-   N_(abort if fast-forward is not possible)),
+   OPT_SET_INT(0, ff, fast_forward, N_(allow fast-forward (default)), 
FF_ALLOW),
+   { OPTION_CALLBACK, 0, ff-only, NULL, NULL,
+   N_(abort if fast-forward is not possible),
+   PARSE_OPT_NOARG | PARSE_OPT_NONEG, option_parse_ff_only },
OPT_RERERE_AUTOUPDATE(allow_rerere_auto),
OPT_BOOL(0, verify-signatures, verify_signatures,
N_(Verify that the named commit has a valid GPG signature)),
@@ -581,10 +596,9 @@ static int git_merge_config(const char *k, const char *v, 
void *cb)
else if (!strcmp(k, merge.ff)) {
int boolval = git_config_maybe_bool(k, v);
if (0 = boolval) {
-   allow_fast_forward = boolval;
+   fast_forward = boolval ? FF_ALLOW : FF_NO;
} else if (v  !strcmp(v, only)) {
-   allow_fast_forward = 1;
-   fast_forward_only = 1;
+   fast_forward = FF_ONLY;
} /* do not barf on values from future versions of git */
return 0;
} else if (!strcmp(k, merge.defaulttoupstream)) {
@@ -863,7 +877,7 @@ static int finish_automerge(struct commit *head,
 
free_commit_list(common);
parents

[PATCH] merge: allow using --no-ff and --ff-only at the same time

[Miklos Vajna]

1347483 (Teach 'git merge' and 'git pull' the option --ff-only,
2009-10-29) says this is not allowed, as they contradict each other.

However, --ff-only is about asserting the input of the merge, and
--no-ff is about instructing merge to always create a merge commit, i.e.
it makes sense to use these options together in some workflow, e.g. when
branches are integrated by rebasing then merging, and the maintainer
wants to be sure the branch is rebased.

Signed-off-by: Miklos Vajna vmik...@suse.cz
---
 builtin/merge.c  | 12 
 t/t7600-merge.sh | 11 ---
 2 files changed, 16 insertions(+), 7 deletions(-)

diff --git a/builtin/merge.c b/builtin/merge.c
index 2ebe732..7026ce0 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1162,9 +1162,6 @@ int cmd_merge(int argc, const char **argv, const char 
*prefix)
option_commit = 0;
}
 
-   if (!allow_fast_forward  fast_forward_only)
-   die(_(You cannot combine --no-ff with --ff-only.));
-
if (!abort_current_merge) {
if (!argc) {
if (default_to_upstream)
@@ -1433,7 +1430,14 @@ int cmd_merge(int argc, const char **argv, const char 
*prefix)
}
}
 
-   if (fast_forward_only)
+   /*
+* If --ff-only was used without --no-ff, or: --ff-only and --no-ff was
+* used at the same time, and this is not a fast-forward.
+*/
+   if (fast_forward_only  (allow_fast_forward || remoteheads-next ||
+   common-next ||
+   hashcmp(common-item-object.sha1,
+   head_commit-object.sha1)))
die(_(Not possible to fast-forward, aborting.));
 
/* We are going to make a new commit. */
diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
index 460d8eb..bf3d9b2 100755
--- a/t/t7600-merge.sh
+++ b/t/t7600-merge.sh
@@ -497,9 +497,14 @@ test_expect_success 'combining --squash and --no-ff is 
refused' '
test_must_fail git merge --no-ff --squash c1
 '
 
-test_expect_success 'combining --ff-only and --no-ff is refused' '
-   test_must_fail git merge --ff-only --no-ff c1 
-   test_must_fail git merge --no-ff --ff-only c1
+test_expect_success 'combining --ff-only and --no-ff (ff is possible)' '
+   git reset --hard c0 
+   git merge --ff-only --no-ff c1
+'
+
+test_expect_success 'combining --ff-only and --no-ff (ff is not possible)' '
+   git reset --hard c1 
+   test_must_fail git merge --ff-only --no-ff c2
 '
 
 test_expect_success 'merge c0 with c1 (ff overrides no-ff)' '
-- 
1.8.1.4

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] merge: allow using --no-ff and --ff-only at the same time

[Miklos Vajna]

Hi Michael,

On Mon, Jul 01, 2013 at 04:52:29PM +0200, Michael Haggerty 
mhag...@alum.mit.edu wrote:
 On 07/01/2013 09:01 AM, Miklos Vajna wrote:
  1347483 (Teach 'git merge' and 'git pull' the option --ff-only,
  2009-10-29) says this is not allowed, as they contradict each other.
  
  However, --ff-only is about asserting the input of the merge, and
  --no-ff is about instructing merge to always create a merge commit, i.e.
  it makes sense to use these options together in some workflow, e.g. when
  branches are integrated by rebasing then merging, and the maintainer
  wants to be sure the branch is rebased.
 
 That is one interpretation of what these options should mean, and I
 agree that it is one way of reading the manpage (which says
 
 --ff-only::
   Refuse to merge and exit with a non-zero status unless the
   current `HEAD` is already up-to-date or the merge can be
   resolved as a fast-forward.
 
 ).  However, I don't think that the manpage unambiguously demands this
 interpretation, and that (more importantly) most users would be very
 surprised if --ff-only and --no-ff were not opposites.

Yes, I agree that that's an unfortunate naming. --ff and --no-ff is the
opposite of each other, however --ff-only is independent, and I would
even rename it to something like --ff-input-only -- but I don't think
it's worth to do so, seeing the cost of it (probably these options are
used by scripts as well).

 How does it hurt?  If I have configuration value merge.ff set to only
 and run git merge --no-ff and then I merge a branch that *cannot* be
 fast forwarded, the logic of your patch would require the merge to be
 rejected, no?  But I think it is more plausible to expect that the
 command line option takes precedence.

Hmm, I did not remember that actually merge.ff even uses the same
configuration slot for these switches. :-( Yes, that would make sense to
fix, once the switches can be combined. Maybe merge.ff and
merge.ff-only?

 In my opinion, your use case shouldn't be supported by the command
 because (1) it is confusing,

I don't see why it would be confusing. I think using these two options
together is one way to try to get the benefits of both rebase (cleaner
history) and merge (keeping the history of which commits came from a
given merge).

 (2) it is not very common,

Hard to argue that argument. :-) No idea what counts as common, my
motivation is that some projects (e.g. syslog-ng) integrate *every*
feature branch this way, and doing this manually (as in indeed
manually or by using a helper script) seems suboptimal, when the support
for this is already mostly in merge.c, just disabled.

 easy to work around:
 
 if git merge-base --is-ancestor HEAD $branch
 then
 git merge --no-ff $branch
 else
 echo fatal: Not possible to fast-forward, aborting.
 exit 1
 fi

Right, that's indeed a viable workaround for the problem.

Miklos


signature.asc
Description: Digital signature

Re: [PATCH] merge: allow using --no-ff and --ff-only at the same time

[Miklos Vajna]

On Mon, Jul 01, 2013 at 08:38:21AM -0700, Junio C Hamano gits...@pobox.com 
wrote:
 As to --no-ff vs --ff-only, --ff-only has always meant only
 fast-forward updates are allowed.  We do not want to create a merge
 commit with this operation.  I do agree with you that the proposed
 patch changes the established semantis and may be too disruptive a
 thing to do at this point.

Hmm, one way around this may be to add a new option that is basically
the same as --no-ff --ff-only (with the patch), except it has a
different name, so it's not confusing. 'git merge --rebase' could be
used for this, but such a name is misleading as well. Anyone has a
better naming idea? :-)

 If one were designing Git merge from scratch today, however, I could
 see one may have designed these as two orthogonal switches.
 
  - Precondition on the shape of histories being merged (fail unless
fast forward does not have to be the only criteria);
 
  - How the update is done (fast forward to the other head, always
create a merge, fast forward if possible, otherwise merge do
not have to be the only three choices).
 
 I do not fundamentally oppose to such a new feature, but they have
 to interact sanely with the current --ff={only,only,never}.

OK, so if I get it right, the problem is that users got used to that
the --ff-only not only means a precondition for the merge, but also
means either don't create a merge commit or fail, while my patch would
change this second behaviour.

I could imagine then new switches, like 'git merge --pre=ff
--update=no-ff could provide these, though I'm not sure if it makes
sense to add such generic switches till the only user is ff.


signature.asc
Description: Digital signature

[PATCH] merge: handle --ff/--no-ff/--ff-only as a tri-state option

[Miklos Vajna]

This has multiple benefits: with more than one of {--ff, --no-ff,
--ff-only} respects the last option; also the command-line option to
always take precedence over the config file option.

Signed-off-by: Miklos Vajna vmik...@suse.cz
---

On Mon, Jul 01, 2013 at 04:52:29PM +0200, Michael Haggerty 
mhag...@alum.mit.edu wrote:
 If I find the time (unlikely) I might submit a patch to implement these
 expectations.

Seeing that the --no-ff / --ff-only combo wasn't denied just sort of 
accidently, I agree that it makes more sense to merge allow_fast_forward
and fast_forward_only to a single enum, that automatically gives you 
both benefits.

 builtin/merge.c  | 65 +---
 t/t7600-merge.sh | 12 ---
 2 files changed, 52 insertions(+), 25 deletions(-)

diff --git a/builtin/merge.c b/builtin/merge.c
index 2ebe732..561edf4 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -47,8 +47,8 @@ static const char * const builtin_merge_usage[] = {
 };
 
 static int show_diffstat = 1, shortlog_len = -1, squash;
-static int option_commit = 1, allow_fast_forward = 1;
-static int fast_forward_only, option_edit = -1;
+static int option_commit = 1;
+static int option_edit = -1;
 static int allow_trivial = 1, have_message, verify_signatures;
 static int overwrite_ignore = 1;
 static struct strbuf merge_msg = STRBUF_INIT;
@@ -76,6 +76,14 @@ static struct strategy all_strategy[] = {
 
 static const char *pull_twohead, *pull_octopus;
 
+enum ff_type {
+   FF_ALLOW,
+   FF_NO,
+   FF_ONLY
+};
+
+static enum ff_type fast_forward = FF_ALLOW;
+
 static int option_parse_message(const struct option *opt,
const char *arg, int unset)
 {
@@ -178,6 +186,21 @@ static int option_parse_n(const struct option *opt,
return 0;
 }
 
+static int option_parse_ff(const struct option *opt,
+ const char *arg, int unset)
+{
+   fast_forward = unset ? FF_NO : FF_ALLOW;
+   return 0;
+}
+
+static int option_parse_ff_only(const struct option *opt,
+ const char *arg, int unset)
+{
+   if (!unset)
+   fast_forward = FF_ONLY;
+   return 0;
+}
+
 static struct option builtin_merge_options[] = {
{ OPTION_CALLBACK, 'n', NULL, NULL, NULL,
N_(do not show a diffstat at the end of the merge),
@@ -194,10 +217,12 @@ static struct option builtin_merge_options[] = {
N_(perform a commit if the merge succeeds (default))),
OPT_BOOL('e', edit, option_edit,
N_(edit message before committing)),
-   OPT_BOOLEAN(0, ff, allow_fast_forward,
-   N_(allow fast-forward (default))),
-   OPT_BOOLEAN(0, ff-only, fast_forward_only,
-   N_(abort if fast-forward is not possible)),
+   { OPTION_CALLBACK, 0, ff, NULL, NULL,
+   N_(allow fast-forward (default)),
+   PARSE_OPT_NOARG, option_parse_ff },
+   { OPTION_CALLBACK, 0, ff-only, NULL, NULL,
+   N_(abort if fast-forward is not possible),
+   PARSE_OPT_NOARG, option_parse_ff_only },
OPT_RERERE_AUTOUPDATE(allow_rerere_auto),
OPT_BOOL(0, verify-signatures, verify_signatures,
N_(Verify that the named commit has a valid GPG signature)),
@@ -581,10 +606,9 @@ static int git_merge_config(const char *k, const char *v, 
void *cb)
else if (!strcmp(k, merge.ff)) {
int boolval = git_config_maybe_bool(k, v);
if (0 = boolval) {
-   allow_fast_forward = boolval;
+   fast_forward = boolval ? FF_ALLOW : FF_NO;
} else if (v  !strcmp(v, only)) {
-   allow_fast_forward = 1;
-   fast_forward_only = 1;
+   fast_forward = FF_ONLY;
} /* do not barf on values from future versions of git */
return 0;
} else if (!strcmp(k, merge.defaulttoupstream)) {
@@ -863,7 +887,7 @@ static int finish_automerge(struct commit *head,
 
free_commit_list(common);
parents = remoteheads;
-   if (!head_subsumed || !allow_fast_forward)
+   if (!head_subsumed || fast_forward == FF_NO)
commit_list_insert(head, parents);
strbuf_addch(merge_msg, '\n');
prepare_to_commit(remoteheads);
@@ -1008,7 +1032,7 @@ static void write_merge_state(struct commit_list 
*remoteheads)
if (fd  0)
die_errno(_(Could not open '%s' for writing), filename);
strbuf_reset(buf);
-   if (!allow_fast_forward)
+   if (fast_forward == FF_NO)
strbuf_addf(buf, no-ff);
if (write_in_full(fd, buf.buf, buf.len) != buf.len)
die_errno(_(Could not write to '%s'), filename);
@@ -1157,14 +1181,11 @@ int cmd_merge(int argc, const char **argv, const char 
*prefix)
show_diffstat = 0;
 
if (squash

Re: [PATCH v3] cherry-pick: make sure all input objects are commits

[Miklos Vajna]

On Thu, May 09, 2013 at 01:27:49PM -0700, Junio C Hamano gits...@pobox.com 
wrote:
 I'd apply this before -rc2.  I _think_ it is also OK to just let
 lookup_commit_reference_gently() barf with its standard message
 
   error: Object %s is a %s, not a commit
 
 without an extra sha1_object_info() call in the error codepath, but
 I did not bother, as this is meant to be an emergency fix.

Yes, that makes a lot of sense. I myself never cherry-pick tags, but I
understand that is part of some workflow.


signature.asc
Description: Digital signature

[PATCH v2] cherry-pick: make sure all input objects are commits

[Miklos Vajna]

When a single argument was a non-commit, the error message used to be:

fatal: BUG: expected exactly one commit from walk

For multiple arguments, when none of the arguments was a commit, the error was:

fatal: empty commit set passed

Finally, when some of the arguments were non-commits, we ignored those
arguments.  Instead, now make sure all arguments are commits, and for
the first non-commit, error out with:

fatal: name: Can't cherry-pick a type

Signed-off-by: Miklos Vajna vmik...@suse.cz
---

On Mon, Apr 08, 2013 at 09:56:55AM -0700, Junio C Hamano gits...@pobox.com 
wrote:
 In other words, perhaps we would want to inspect pending objects
 before running prepare_revision_walk and make sure everybody is
 commit-ish or something?

Sure, that makes sense to me.

 sequencer.c | 13 +
 t/t3508-cherry-pick-many-commits.sh |  6 ++
 2 files changed, 19 insertions(+)

diff --git a/sequencer.c b/sequencer.c
index baa0310..eb25101 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1047,6 +1047,7 @@ int sequencer_pick_revisions(struct replay_opts *opts)
 {
struct commit_list *todo_list = NULL;
unsigned char sha1[20];
+   int i;
 
if (opts-subcommand == REPLAY_NONE)
assert(opts-revs);
@@ -1067,6 +1068,18 @@ int sequencer_pick_revisions(struct replay_opts *opts)
if (opts-subcommand == REPLAY_CONTINUE)
return sequencer_continue(opts);
 
+   for (i = 0; i  opts-revs-pending.nr; i++) {
+   unsigned char sha1[20];
+   const char *name = opts-revs-pending.objects[i].name;
+
+   if (!get_sha1(name, sha1)) {
+   enum object_type type = sha1_object_info(sha1, NULL);
+
+   if (type  0  type != OBJ_COMMIT)
+   die(_(%s: can't cherry-pick a %s), name, 
typename(type));
+   }
+   }
+
/*
 * If we were called as git cherry-pick commit, just
 * cherry-pick/revert it, set CHERRY_PICK_HEAD /
diff --git a/t/t3508-cherry-pick-many-commits.sh 
b/t/t3508-cherry-pick-many-commits.sh
index 4e7136b..19c99d7 100755
--- a/t/t3508-cherry-pick-many-commits.sh
+++ b/t/t3508-cherry-pick-many-commits.sh
@@ -55,6 +55,12 @@ one
 two
 '
 
+test_expect_success 'cherry-pick three one two: fails' '
+   git checkout -f master 
+   git reset --hard first 
+   test_must_fail git cherry-pick three one two:
+'
+
 test_expect_success 'output to keep user entertained during multi-pick' '
cat -\EOF expected 
[master OBJID] second
-- 
1.8.1.4

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] cherry-pick: make sure all input objects are commits

[Miklos Vajna]

On Thu, Apr 11, 2013 at 03:52:44PM +0530, Ramkumar Ramachandra 
artag...@gmail.com wrote:
  +   for (i = 0; i  opts-revs-pending.nr; i++) {
  +   unsigned char sha1[20];
  +   const char *name = opts-revs-pending.objects[i].name;
  +
  +   if (!get_sha1(name, sha1)) {
  +   enum object_type type = sha1_object_info(sha1, 
  NULL);
  +
  +   if (type  0  type != OBJ_COMMIT)
  +   die(_(%s: can't cherry-pick a %s), name, 
  typename(type));
  +   }
 
 else?  What happens if get_sha1() fails?

I guess that is a should-not-happen category. parse_args() calls
setup_revisions(), and that will already die() if the argument is not a
valid object at all.

  diff --git a/t/t3508-cherry-pick-many-commits.sh 
  b/t/t3508-cherry-pick-many-commits.sh
  index 4e7136b..19c99d7 100755
  --- a/t/t3508-cherry-pick-many-commits.sh
  +++ b/t/t3508-cherry-pick-many-commits.sh
  @@ -55,6 +55,12 @@ one
   two
   '
 
  +test_expect_success 'cherry-pick three one two: fails' '
  +   git checkout -f master 
  +   git reset --hard first 
  +   test_must_fail git cherry-pick three one two:
  +'
 
 So you're testing just the third case (where commit objects are mixed
 with non-commit objects), which is arguably a bug.  Okay.

Yes. If you would want, I could of course add test cases for two other
cases when we already errored out and now the error message is just
changed, but I don't think duplicating the error message strings from
the code to the testsuite is really wanted. :-)


signature.asc
Description: Digital signature

[PATCH v3] cherry-pick: make sure all input objects are commits

[Miklos Vajna]

When a single argument was a non-commit, the error message used to be:

fatal: BUG: expected exactly one commit from walk

For multiple arguments, when none of the arguments was a commit, the error was:

fatal: empty commit set passed

Finally, when some of the arguments were non-commits, we ignored those
arguments.  Fix this bug and make sure all arguments are commits, and
for the first non-commit, error out with:

fatal: name: Can't cherry-pick a type

Signed-off-by: Miklos Vajna vmik...@suse.cz
---

On Thu, Apr 11, 2013 at 05:12:06PM +0530, Ramkumar Ramachandra 
artag...@gmail.com wrote:
 Then why do you have an if() guarding the code?  In my opinion, you
 should have an else-clause that die()s with an appropriate message.

And you were right -- I actually forgot about --stdin, where the 
else-clause is hit. Added that for now, excluding --stdin.

 Nope, I'd never suggest that: this is fine.  What I meant is: you
 should clarify that you're fixing a bug and adding a test to guard it,
 in the commit message.

Done.

 sequencer.c | 18 ++
 t/t3508-cherry-pick-many-commits.sh |  6 ++
 2 files changed, 24 insertions(+)

diff --git a/sequencer.c b/sequencer.c
index baa0310..61fdb68 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1047,6 +1047,7 @@ int sequencer_pick_revisions(struct replay_opts *opts)
 {
struct commit_list *todo_list = NULL;
unsigned char sha1[20];
+   int i;
 
if (opts-subcommand == REPLAY_NONE)
assert(opts-revs);
@@ -1067,6 +1068,23 @@ int sequencer_pick_revisions(struct replay_opts *opts)
if (opts-subcommand == REPLAY_CONTINUE)
return sequencer_continue(opts);
 
+   for (i = 0; i  opts-revs-pending.nr; i++) {
+   unsigned char sha1[20];
+   const char *name = opts-revs-pending.objects[i].name;
+
+   /* This happens when using --stdin. */
+   if (!strlen(name))
+   continue;
+
+   if (!get_sha1(name, sha1)) {
+   enum object_type type = sha1_object_info(sha1, NULL);
+
+   if (type  0  type != OBJ_COMMIT)
+   die(_(%s: can't cherry-pick a %s), name, 
typename(type));
+   } else
+   die(_(%s: bad revision), name);
+   }
+
/*
 * If we were called as git cherry-pick commit, just
 * cherry-pick/revert it, set CHERRY_PICK_HEAD /
diff --git a/t/t3508-cherry-pick-many-commits.sh 
b/t/t3508-cherry-pick-many-commits.sh
index 4e7136b..19c99d7 100755
--- a/t/t3508-cherry-pick-many-commits.sh
+++ b/t/t3508-cherry-pick-many-commits.sh
@@ -55,6 +55,12 @@ one
 two
 '
 
+test_expect_success 'cherry-pick three one two: fails' '
+   git checkout -f master 
+   git reset --hard first 
+   test_must_fail git cherry-pick three one two:
+'
+
 test_expect_success 'output to keep user entertained during multi-pick' '
cat -\EOF expected 
[master OBJID] second
-- 
1.8.1.4

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] cherry-pick: better error message when the parameter is a non-commit

[Miklos Vajna]

Hi,

On Wed, Apr 03, 2013 at 11:27:04AM +0200, Miklos Vajna vmik...@suse.cz wrote:
 When copypaste goes wrong, and the user e.g. tries to cherry-pick a
 blob, the error message used to be:
 
   fatal: BUG: expected exactly one commit from walk
 
 Instead, now it is:
 
   fatal: Can't cherry-pick a blob
 
 Signed-off-by: Miklos Vajna vmik...@suse.cz
 ---
  sequencer.c | 9 -
  1 file changed, 8 insertions(+), 1 deletion(-)

Ping, any comment on this patch?

Thanks,

Miklos
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] cherry-pick: better error message when the parameter is a non-commit

[Miklos Vajna]

When copypaste goes wrong, and the user e.g. tries to cherry-pick a
blob, the error message used to be:

fatal: BUG: expected exactly one commit from walk

Instead, now it is:

fatal: Can't cherry-pick a blob

Signed-off-by: Miklos Vajna vmik...@suse.cz
---
 sequencer.c | 9 -
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/sequencer.c b/sequencer.c
index baa0310..0ac00d4 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1082,8 +1082,15 @@ int sequencer_pick_revisions(struct replay_opts *opts)
if (prepare_revision_walk(opts-revs))
die(_(revision walk setup failed));
cmit = get_revision(opts-revs);
-   if (!cmit || get_revision(opts-revs))
+   if (!cmit || get_revision(opts-revs)) {
+   unsigned char sha1[20];
+   if (!get_sha1(opts-revs-cmdline.rev-name, sha1)) {
+   enum object_type type = sha1_object_info(sha1, 
NULL);
+   if (type  0  type != OBJ_COMMIT)
+   die(_(Can't cherry-pick a %s), 
typename(type));
+   }
die(BUG: expected exactly one commit from walk);
+   }
return single_pick(cmit, opts);
}
 
-- 
1.8.1.4

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] cherry-pick -x: improve handling of one-liner commit messages

[Miklos Vajna]

git cherry-pick -x normally just appends the cherry picked from commit
line at the end of the message, which is fine. However, in case the
original commit message had only one line, first append a newline,
otherwise the second line won't be empty, which is against
recommendations.
---
 sequencer.c   | 10 ++
 t/t3501-revert-cherry-pick.sh |  8 
 2 files changed, 18 insertions(+)

diff --git a/sequencer.c b/sequencer.c
index aef5e8a..1ae0e43 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -496,6 +496,16 @@ static int do_pick_commit(struct commit *commit, struct 
replay_opts *opts)
}
 
if (opts-record_origin) {
+
+   /*
+* If this the message is a one-liner, append a
+* newline, so the second line will be empty, as
+* recommended.
+*/
+   p = strstr(msgbuf.buf, \n\n);
+   if (!p)
+   strbuf_addch(msgbuf, '\n');
+
strbuf_addstr(msgbuf, (cherry picked from commit );
strbuf_addstr(msgbuf, 
sha1_to_hex(commit-object.sha1));
strbuf_addstr(msgbuf, )\n);
diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh
index 6f489e2..858c744 100755
--- a/t/t3501-revert-cherry-pick.sh
+++ b/t/t3501-revert-cherry-pick.sh
@@ -70,6 +70,14 @@ test_expect_success 'cherry-pick after renaming branch' '
 
 '
 
+test_expect_success 'cherry-pick -x of one-liner commit message' '
+
+   git checkout rename2 
+   git cherry-pick -x added 
+   git show -s --pretty=format:%s | test_must_fail grep cherry picked
+
+'
+
 test_expect_success 'revert after renaming branch' '
 
git checkout rename1 
-- 
1.8.1.4

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] cherry-pick -x: improve handling of one-liner commit messages

[Miklos Vajna]

Hi,

On Fri, Mar 29, 2013 at 10:41:17AM -0700, Brandon Casey draf...@gmail.com 
wrote:
  Sign-off?

Indeed, I forgot about it, my bad.

  I think this is part of the bc/append-signed-off-by topic that is
  about to graduate to 'master'; more specifically, b971e04f54e7
  (sequencer.c: always separate (cherry picked from from commit
  body, 2013-02-12) does the equivalent, no?
 
 Yeah, I think this case is already handled.
 
 Miklos, can you check out next and see if your problem case is handled?

I just checked next and right, that solves the problem I was fixing.

So -- sorry for the noise. :-)

Miklos


signature.asc
Description: Digital signature

[PATCH v4] cherry-pick: don't forget -s on failure

[Miklos Vajna]

In case 'git cherry-pick -s commit' failed, the user had to use 'git
commit -s' (i.e. state the -s option again), which is easy to forget
about.  Instead, write the signed-off-by line early, so plain 'git
commit' will have the same result.

Also update 'git commit -s', so that in case there is already a relevant
Signed-off-by line before the Conflicts: line, it won't add one more at
the end of the message. If there is no such line, then add it before the
the Conflicts: line.

Signed-off-by: Miklos Vajna vmik...@suse.cz
---

On Thu, Sep 13, 2012 at 02:13:46PM -0700, Junio C Hamano gits...@pobox.com 
wrote:
 That's decl-after-stmt.

Sorry, added -Wdeclaration-after-statement to CFLAGS now.

 I would have expected that you can just do strbuf_splice() to add
 the sob into msgbuf with the original code structure, without a
 substantial rewrite of the function like this.  Perhaps I am missing
 something?

I forgot about strbuf_splice(). ;-) Here is a version with it -- it's 
indeed shorter, even if ends_rfc2822_footer() now has to be aware of a 
possible footer.

 builtin/commit.c|   79 +++---
 sequencer.c |   65 
 sequencer.h |4 ++
 t/t3507-cherry-pick-conflict.sh |   32 
 t/t3510-cherry-pick-sequence.sh |6 +-
 t/test-lib-functions.sh |   20 +++--
 6 files changed, 142 insertions(+), 64 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 778cf16..4d50484 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -28,6 +28,7 @@
 #include submodule.h
 #include gpg-interface.h
 #include column.h
+#include sequencer.h
 
 static const char * const builtin_commit_usage[] = {
N_(git commit [options] [--] filepattern...),
@@ -466,8 +467,6 @@ static int is_a_merge(const struct commit *current_head)
return !!(current_head-parents  current_head-parents-next);
 }
 
-static const char sign_off_header[] = Signed-off-by: ;
-
 static void export_one(const char *var, const char *s, const char *e, int hack)
 {
struct strbuf buf = STRBUF_INIT;
@@ -552,47 +551,6 @@ static void determine_author_info(struct strbuf 
*author_ident)
}
 }
 
-static int ends_rfc2822_footer(struct strbuf *sb)
-{
-   int ch;
-   int hit = 0;
-   int i, j, k;
-   int len = sb-len;
-   int first = 1;
-   const char *buf = sb-buf;
-
-   for (i = len - 1; i  0; i--) {
-   if (hit  buf[i] == '\n')
-   break;
-   hit = (buf[i] == '\n');
-   }
-
-   while (i  len - 1  buf[i] == '\n')
-   i++;
-
-   for (; i  len; i = k) {
-   for (k = i; k  len  buf[k] != '\n'; k++)
-   ; /* do nothing */
-   k++;
-
-   if ((buf[k] == ' ' || buf[k] == '\t')  !first)
-   continue;
-
-   first = 0;
-
-   for (j = 0; i + j  len; j++) {
-   ch = buf[i + j];
-   if (ch == ':')
-   break;
-   if (isalnum(ch) ||
-   (ch == '-'))
-   continue;
-   return 0;
-   }
-   }
-   return 1;
-}
-
 static char *cut_ident_timestamp_part(char *string)
 {
char *ket = strrchr(string, '');
@@ -717,21 +675,30 @@ static int prepare_to_commit(const char *index_file, 
const char *prefix,
stripspace(sb, 0);
 
if (signoff) {
-   struct strbuf sob = STRBUF_INIT;
-   int i;
+   /*
+* See if we have a Conflicts: block at the end. If yes, count
+* its size, so we can ignore it.
+*/
+   int ignore_footer = 0;
+   int i, eol, previous = 0;
+   const char *nl;
 
-   strbuf_addstr(sob, sign_off_header);
-   strbuf_addstr(sob, fmt_name(getenv(GIT_COMMITTER_NAME),
-getenv(GIT_COMMITTER_EMAIL)));
-   strbuf_addch(sob, '\n');
-   for (i = sb.len - 1; i  0  sb.buf[i - 1] != '\n'; i--)
-   ; /* do nothing */
-   if (prefixcmp(sb.buf + i, sob.buf)) {
-   if (!i || !ends_rfc2822_footer(sb))
-   strbuf_addch(sb, '\n');
-   strbuf_addbuf(sb, sob);
+   for (i = 0; i  sb.len; i++) {
+   nl = memchr(sb.buf + i, '\n', sb.len - i);
+   if (nl)
+   eol = nl - sb.buf;
+   else
+   eol = sb.len;
+   if (!prefixcmp(sb.buf + previous, \nConflicts:\n)) {
+   ignore_footer = sb.len - previous;
+   break

[PATCH v2] cherry-pick: don't forget -s on failure

[Miklos Vajna]

In case 'git cherry-pick -s commit' failed, the user had to use 'git
commit -s' (i.e. state the -s option again), which is easy to forget
about.  Instead, write the signed-off-by line early, so plain 'git
commit' will have the same result.

Also update 'git commit -s', so that in case there is already a relevant
Signed-off-by line before the Conflicts: line, it won't add one more at
the end of the message.

Signed-off-by: Miklos Vajna vmik...@suse.cz
---

On Wed, Sep 12, 2012 at 03:45:10PM -0700, Junio C Hamano gits...@pobox.com 
wrote:
  - The additional S-o-b should come immediately after the existing
block of footers.

This was trivial to fix.

  - And the last entry in the existing footer block is already mine,
so there shouldn't have been a new and duplicated one added.
 
 
 I am not sure how reusable the moved function is without
 enhancements for your purpose.  The logic to identify the footer
 needs to be enhanced so that an end pointer to point at the byte
 before the caller added Conflicts:  can be given, and pretend as
 if it is the end of the buffer, unlike in the fresh commit case
 where it can consider the real end of the buffer as such.

Below is what I came up with. It simply ignores anything after the 
Conclicts: line, when checking for the last Signed-off-by line.

An other thing: I forgot to run 'make test' for the initial patch, it 
seems t3510-cherry-pick-sequence.sh has 3 tests that basically ensures 
the opposite of what my patch does. Given that there are already 
testcases for the new behaviour, can they be just removed? For now, I 
just disabled them.

 builtin/commit.c|   79 +++---
 sequencer.c |   65 
 sequencer.h |4 ++
 t/t3507-cherry-pick-conflict.sh |   14 +++
 t/t3510-cherry-pick-sequence.sh |6 +-
 t/test-lib-functions.sh |8 +++-
 6 files changed, 116 insertions(+), 60 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 778cf16..4d50484 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -28,6 +28,7 @@
 #include submodule.h
 #include gpg-interface.h
 #include column.h
+#include sequencer.h
 
 static const char * const builtin_commit_usage[] = {
N_(git commit [options] [--] filepattern...),
@@ -466,8 +467,6 @@ static int is_a_merge(const struct commit *current_head)
return !!(current_head-parents  current_head-parents-next);
 }
 
-static const char sign_off_header[] = Signed-off-by: ;
-
 static void export_one(const char *var, const char *s, const char *e, int hack)
 {
struct strbuf buf = STRBUF_INIT;
@@ -552,47 +551,6 @@ static void determine_author_info(struct strbuf 
*author_ident)
}
 }
 
-static int ends_rfc2822_footer(struct strbuf *sb)
-{
-   int ch;
-   int hit = 0;
-   int i, j, k;
-   int len = sb-len;
-   int first = 1;
-   const char *buf = sb-buf;
-
-   for (i = len - 1; i  0; i--) {
-   if (hit  buf[i] == '\n')
-   break;
-   hit = (buf[i] == '\n');
-   }
-
-   while (i  len - 1  buf[i] == '\n')
-   i++;
-
-   for (; i  len; i = k) {
-   for (k = i; k  len  buf[k] != '\n'; k++)
-   ; /* do nothing */
-   k++;
-
-   if ((buf[k] == ' ' || buf[k] == '\t')  !first)
-   continue;
-
-   first = 0;
-
-   for (j = 0; i + j  len; j++) {
-   ch = buf[i + j];
-   if (ch == ':')
-   break;
-   if (isalnum(ch) ||
-   (ch == '-'))
-   continue;
-   return 0;
-   }
-   }
-   return 1;
-}
-
 static char *cut_ident_timestamp_part(char *string)
 {
char *ket = strrchr(string, '');
@@ -717,21 +675,30 @@ static int prepare_to_commit(const char *index_file, 
const char *prefix,
stripspace(sb, 0);
 
if (signoff) {
-   struct strbuf sob = STRBUF_INIT;
-   int i;
+   /*
+* See if we have a Conflicts: block at the end. If yes, count
+* its size, so we can ignore it.
+*/
+   int ignore_footer = 0;
+   int i, eol, previous = 0;
+   const char *nl;
 
-   strbuf_addstr(sob, sign_off_header);
-   strbuf_addstr(sob, fmt_name(getenv(GIT_COMMITTER_NAME),
-getenv(GIT_COMMITTER_EMAIL)));
-   strbuf_addch(sob, '\n');
-   for (i = sb.len - 1; i  0  sb.buf[i - 1] != '\n'; i--)
-   ; /* do nothing */
-   if (prefixcmp(sb.buf + i, sob.buf)) {
-   if (!i || !ends_rfc2822_footer(sb))
-   strbuf_addch(sb, '\n

[PATCH v3] cherry-pick: don't forget -s on failure

[Miklos Vajna]

In case 'git cherry-pick -s commit' failed, the user had to use 'git
commit -s' (i.e. state the -s option again), which is easy to forget
about.  Instead, write the signed-off-by line early, so plain 'git
commit' will have the same result.

Also update 'git commit -s', so that in case there is already a relevant
Signed-off-by line before the Conflicts: line, it won't add one more at
the end of the message. If there is no such line, then add it before the
the Conflicts: line.

Signed-off-by: Miklos Vajna vmik...@suse.cz
---

 This is somewhat iffy.  Shouldn't test_commit --signoff --notick
 work?

Indeed, fixed now.

 Hrm, what is this thing trying to do?  It does start scanning from
 the end (ignoring the Conflicts: thing) to see who the last person
 that signed it off was, but once it decides that it needs to add a
 new sign-off, it still adds it at the very end anyway.

Ah, I did not handle that, as the original git commit -s didn't do it 
either -- and originally I just wanted to touch git cherry-pick. 
Implemented now.

 builtin/commit.c|   79 +++---
 sequencer.c |   72 +++
 sequencer.h |4 ++
 t/t3507-cherry-pick-conflict.sh |   32 
 t/t3510-cherry-pick-sequence.sh |6 +-
 t/test-lib-functions.sh |   20 +++--
 6 files changed, 149 insertions(+), 64 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 778cf16..4d50484 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -28,6 +28,7 @@
 #include submodule.h
 #include gpg-interface.h
 #include column.h
+#include sequencer.h
 
 static const char * const builtin_commit_usage[] = {
N_(git commit [options] [--] filepattern...),
@@ -466,8 +467,6 @@ static int is_a_merge(const struct commit *current_head)
return !!(current_head-parents  current_head-parents-next);
 }
 
-static const char sign_off_header[] = Signed-off-by: ;
-
 static void export_one(const char *var, const char *s, const char *e, int hack)
 {
struct strbuf buf = STRBUF_INIT;
@@ -552,47 +551,6 @@ static void determine_author_info(struct strbuf 
*author_ident)
}
 }
 
-static int ends_rfc2822_footer(struct strbuf *sb)
-{
-   int ch;
-   int hit = 0;
-   int i, j, k;
-   int len = sb-len;
-   int first = 1;
-   const char *buf = sb-buf;
-
-   for (i = len - 1; i  0; i--) {
-   if (hit  buf[i] == '\n')
-   break;
-   hit = (buf[i] == '\n');
-   }
-
-   while (i  len - 1  buf[i] == '\n')
-   i++;
-
-   for (; i  len; i = k) {
-   for (k = i; k  len  buf[k] != '\n'; k++)
-   ; /* do nothing */
-   k++;
-
-   if ((buf[k] == ' ' || buf[k] == '\t')  !first)
-   continue;
-
-   first = 0;
-
-   for (j = 0; i + j  len; j++) {
-   ch = buf[i + j];
-   if (ch == ':')
-   break;
-   if (isalnum(ch) ||
-   (ch == '-'))
-   continue;
-   return 0;
-   }
-   }
-   return 1;
-}
-
 static char *cut_ident_timestamp_part(char *string)
 {
char *ket = strrchr(string, '');
@@ -717,21 +675,30 @@ static int prepare_to_commit(const char *index_file, 
const char *prefix,
stripspace(sb, 0);
 
if (signoff) {
-   struct strbuf sob = STRBUF_INIT;
-   int i;
+   /*
+* See if we have a Conflicts: block at the end. If yes, count
+* its size, so we can ignore it.
+*/
+   int ignore_footer = 0;
+   int i, eol, previous = 0;
+   const char *nl;
 
-   strbuf_addstr(sob, sign_off_header);
-   strbuf_addstr(sob, fmt_name(getenv(GIT_COMMITTER_NAME),
-getenv(GIT_COMMITTER_EMAIL)));
-   strbuf_addch(sob, '\n');
-   for (i = sb.len - 1; i  0  sb.buf[i - 1] != '\n'; i--)
-   ; /* do nothing */
-   if (prefixcmp(sb.buf + i, sob.buf)) {
-   if (!i || !ends_rfc2822_footer(sb))
-   strbuf_addch(sb, '\n');
-   strbuf_addbuf(sb, sob);
+   for (i = 0; i  sb.len; i++) {
+   nl = memchr(sb.buf + i, '\n', sb.len - i);
+   if (nl)
+   eol = nl - sb.buf;
+   else
+   eol = sb.len;
+   if (!prefixcmp(sb.buf + previous, \nConflicts:\n)) {
+   ignore_footer = sb.len - previous;
+   break;
+   }
+   while (i

Re: [PATCH] man: git pull -r is a short for --rebase

[Miklos Vajna]

On Thu, Aug 16, 2012 at 11:18:40PM -0700, Junio C Hamano gits...@pobox.com 
wrote:
 From: Miklos Vajna vmik...@suse.cz
 Date: Thu, 16 Aug 2012 11:50:18 +0200
 Subject: [PATCH] man: git pull -r is a short for --rebase
 
 Letting the --rebase option squat on the short-and-sweet single
 letter option -r was an unintended accident and was not even
 documented, but the short option seems to be already used in the
 wild. Let's document it so that other options that begin with r
 would not be tempted to steal it.

Signed-off-by: Miklos Vajna vmik...@suse.cz
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] man: git pull -r is a short for --rebase

[Miklos Vajna]

---
 Documentation/git-pull.txt |1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/Documentation/git-pull.txt b/Documentation/git-pull.txt
index defb544..67fa5ee 100644
--- a/Documentation/git-pull.txt
+++ b/Documentation/git-pull.txt
@@ -101,6 +101,7 @@ include::merge-options.txt[]
 
 :git-pull: 1
 
+-r::
 --rebase::
Rebase the current branch on top of the upstream branch after
fetching.  If there is a remote-tracking branch corresponding to
-- 
1.7.7

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] man: git pull -r is a short for --rebase

[Miklos Vajna]

On Thu, Aug 16, 2012 at 02:09:33PM -0700, Junio C Hamano gits...@pobox.com 
wrote:
 The reason I do not think pull -r gives much value to the users to
 trigger pull --rebase is because the use of pull --rebase is
 very project specific.  If you are working on a project that forbids
 merges, you would _always_ want to run pull --rebase, which means
 you would likely have it configured and would not be typing from the
 command line.

I agree that it's a bit strange, but based on a quick search, it seems
multiple projects already advertise git pull -r (i.e. not --rebase and
not a configuration option):

http://lilypond.org/doc/v2.15/Documentation/contributor/pulling-and-rebasing
http://wiki.documentfoundation.org/Git_For_LibreOffice_Developers
http://www.wiremod.com/forum/wiremod-general-chat/29517-git-introduction-incomplete-unformatted.html

So it seems making -r refer to --recurse-submodules would already cause
quite some pain to users.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html