Re: "git submodule" vs "git subtree" vs "repo" vs "git subdir" ... ?

[Nicolas Morey-Chaisemartin]

Le 13/02/2018 à 14:06, Robert P. J. Day a écrit :
>   looking for general opinions ... i am (frighteningly :-) teaching a
> git course later this week, and one of the topics on the list is git
> submodules, which was specifically requested by the client as their
> idea of how to start incorporating child repos in new projects.
>
>   however, given the number of articles written about the drawbacks
> with submodules, i wanted to throw in a section about "git subtree" as
> well, even though (as discussed earlier) there is some minor dispute
> as to whether "git subtree" is part of "core" git, but i'm not going
> to let that stop me.

I've dealt with a large number of submodules and subtrees in the last years so 
here are my 2 cents.
The first question to ask is the most important one:
Do the repos really need to be split at all.

In a previous life, we started with one repo per "small projects", each 
containing a couple of binaries or libs.
Everything was put together through submodules.
After way too long to admit, we realized that it was just not worth it. We had 
to deal with so many corner cases in CI and other things for a no real gain.
The reality is git works perfectly with bigger projects and smart enough that 
people working on two different "subprojects" don't create any conflict.
So we started merging a lot of it back into the main repo and that was it.

I see some advantages submodules have other subtrees:
- It's slightly easier to in a subproject when using submodule than with 
subtree (at least for git newcomers).
Once you made sure your submodule is on a branch, you just go into it and work 
in a standard git.
When using subtree, you have to know you're in a subtree and to your subtree 
push/pull to resync with someone working directly in the subproject.
At the time conflict in subtrees were sometimes a bit weird and created history 
full of merge.

- submodules are great when working with huge subrepo.
We have a repository than had among its submodule: linux, gcc, gdb, glibc.
This means a real lot of code/files and commits.
Subtree would work but you'd endup with a humongous repo and as expected 
everything is slowed down.

- submodules work well when you are moving between versions and branches of a 
subproject.
Let me clarify with an example:
We had a repo that had ffmpeg as a submodule with a large patch series from us.
We would regularly create a new branch in the subrepo with all our patches on 
top of a new ffmpeg upstream release.
And simply point the top project to this SHA1. And it worked great.
I wouldn't risk doing that kind of things with subtree. I may be wrong here (I 
haven't tried it to be honest) the the push/pull approach doesn't seem to fit
 the idea of moving from a SHA1 to a seemingly unrelated one.

Now that I'm working with reasonable repo sizes again, and linear history, I 
mostly stick with subtrees.

Nicolas

Re: [PATCHv4] tag: add --edit option

[Nicolas Morey-Chaisemartin]

Please ignore !

v3 with the nits fixed was picked by Junio

Le 07/02/2018 à 17:55, Nicolas Morey-Chaisemartin a écrit :
> Add a --edit option whichs allows modifying the messages provided by -m or -F,
> the same way git commit --edit does.
>
> Signed-off-by: Nicolas Morey-Chaisemartin <nmoreychaisemar...@suse.com>
> ---
>
> Fixes since v3 ( 
> https://public-inbox.org/git/88e7c122-599f-4ab1-6d65-c75f7a3ae...@suse.com/ ):
>  * Replace tab by space in t/t7004-tag.sh
>
>
>  Documentation/git-tag.txt |  8 +++-
>  builtin/tag.c | 11 +--
>  t/t7004-tag.sh| 30 ++
>  3 files changed, 46 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
> index 956fc019f984..1d17101bac39 100644
> --- a/Documentation/git-tag.txt
> +++ b/Documentation/git-tag.txt
> @@ -9,7 +9,7 @@ git-tag - Create, list, delete or verify a tag object signed 
> with GPG
>  SYNOPSIS
>  
>  [verse]
> -'git tag' [-a | -s | -u ] [-f] [-m  | -F ]
> +'git tag' [-a | -s | -u ] [-f] [-m  | -F ] [-e]
>[ | ]
>  'git tag' -d ...
>  'git tag' [-n[]] -l [--contains ] [--no-contains ]
> @@ -167,6 +167,12 @@ This option is only applicable when listing tags without 
> annotation lines.
>   Implies `-a` if none of `-a`, `-s`, or `-u `
>   is given.
>  
> +-e::
> +--edit::
> + The message taken from file with `-F` and command line with
> + `-m` are usually used as the tag message unmodified.
> + This option lets you further edit the message taken from these sources.
> +
>  --cleanup=::
>   This option sets how the tag message is cleaned up.
>   The  '' can be one of 'verbatim', 'whitespace' and 'strip'.  The
> diff --git a/builtin/tag.c b/builtin/tag.c
> index a7e6a5b0f234..ce5cac3dd23f 100644
> --- a/builtin/tag.c
> +++ b/builtin/tag.c
> @@ -194,6 +194,7 @@ static int build_tag_object(struct strbuf *buf, int sign, 
> struct object_id *resu
>  
>  struct create_tag_options {
>   unsigned int message_given:1;
> + unsigned int use_editor:1;
>   unsigned int sign;
>   enum {
>   CLEANUP_NONE,
> @@ -224,7 +225,7 @@ static void create_tag(const struct object_id *object, 
> const char *tag,
>   tag,
>   git_committer_info(IDENT_STRICT));
>  
> - if (!opt->message_given) {
> + if (!opt->message_given || opt->use_editor) {
>   int fd;
>  
>   /* write the template message before editing: */
> @@ -233,7 +234,10 @@ static void create_tag(const struct object_id *object, 
> const char *tag,
>   if (fd < 0)
>   die_errno(_("could not create file '%s'"), path);
>  
> - if (!is_null_oid(prev)) {
> + if (opt->message_given) {
> + write_or_die(fd, buf->buf, buf->len);
> + strbuf_reset(buf);
> + } else if (!is_null_oid(prev)) {
>   write_tag_body(fd, prev);
>   } else {
>   struct strbuf buf = STRBUF_INIT;
> @@ -372,6 +376,7 @@ int cmd_tag(int argc, const char **argv, const char 
> *prefix)
>   static struct ref_sorting *sorting = NULL, **sorting_tail = 
>   struct ref_format format = REF_FORMAT_INIT;
>   int icase = 0;
> + int edit_flag = 0;
>   struct option options[] = {
>   OPT_CMDMODE('l', "list", , N_("list tag names"), 'l'),
>   { OPTION_INTEGER, 'n', NULL, , N_("n"),
> @@ -386,6 +391,7 @@ int cmd_tag(int argc, const char **argv, const char 
> *prefix)
>   OPT_CALLBACK('m', "message", , N_("message"),
>N_("tag message"), parse_msg_arg),
>   OPT_FILENAME('F', "file", , N_("read message from 
> file")),
> + OPT_BOOL('e', "edit", _flag, N_("force edit of tag 
> message")),
>   OPT_BOOL('s', "sign", , N_("annotated and GPG-signed 
> tag")),
>   OPT_STRING(0, "cleanup", _arg, N_("mode"),
>   N_("how to strip spaces and #comments from message")),
> @@ -524,6 +530,7 @@ int cmd_tag(int argc, const char **argv, const char 
> *prefix)
>   die(_("tag '%s' already exists"), tag);
>  
>   opt.message_given = msg.given || msgfile;
> + opt.use_editor = edit_flag;
>  
>   if (!cleanup_arg || !strcmp(cleanup_arg, "strip"))
>   opt.cleanup_mode = CLEANUP_ALL;
> diff 

[PATCHv4] tag: add --edit option

[Nicolas Morey-Chaisemartin]

Add a --edit option whichs allows modifying the messages provided by -m or -F,
the same way git commit --edit does.

Signed-off-by: Nicolas Morey-Chaisemartin <nmoreychaisemar...@suse.com>
---

Fixes since v3 ( 
https://public-inbox.org/git/88e7c122-599f-4ab1-6d65-c75f7a3ae...@suse.com/ ):
 * Replace tab by space in t/t7004-tag.sh


 Documentation/git-tag.txt |  8 +++-
 builtin/tag.c | 11 +--
 t/t7004-tag.sh| 30 ++
 3 files changed, 46 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index 956fc019f984..1d17101bac39 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -9,7 +9,7 @@ git-tag - Create, list, delete or verify a tag object signed 
with GPG
 SYNOPSIS
 
 [verse]
-'git tag' [-a | -s | -u ] [-f] [-m  | -F ]
+'git tag' [-a | -s | -u ] [-f] [-m  | -F ] [-e]
 [ | ]
 'git tag' -d ...
 'git tag' [-n[]] -l [--contains ] [--no-contains ]
@@ -167,6 +167,12 @@ This option is only applicable when listing tags without 
annotation lines.
Implies `-a` if none of `-a`, `-s`, or `-u `
is given.
 
+-e::
+--edit::
+   The message taken from file with `-F` and command line with
+   `-m` are usually used as the tag message unmodified.
+   This option lets you further edit the message taken from these sources.
+
 --cleanup=::
This option sets how the tag message is cleaned up.
The  '' can be one of 'verbatim', 'whitespace' and 'strip'.  The
diff --git a/builtin/tag.c b/builtin/tag.c
index a7e6a5b0f234..ce5cac3dd23f 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -194,6 +194,7 @@ static int build_tag_object(struct strbuf *buf, int sign, 
struct object_id *resu
 
 struct create_tag_options {
unsigned int message_given:1;
+   unsigned int use_editor:1;
unsigned int sign;
enum {
CLEANUP_NONE,
@@ -224,7 +225,7 @@ static void create_tag(const struct object_id *object, 
const char *tag,
tag,
git_committer_info(IDENT_STRICT));
 
-   if (!opt->message_given) {
+   if (!opt->message_given || opt->use_editor) {
int fd;
 
/* write the template message before editing: */
@@ -233,7 +234,10 @@ static void create_tag(const struct object_id *object, 
const char *tag,
if (fd < 0)
die_errno(_("could not create file '%s'"), path);
 
-   if (!is_null_oid(prev)) {
+   if (opt->message_given) {
+   write_or_die(fd, buf->buf, buf->len);
+   strbuf_reset(buf);
+   } else if (!is_null_oid(prev)) {
write_tag_body(fd, prev);
} else {
struct strbuf buf = STRBUF_INIT;
@@ -372,6 +376,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
static struct ref_sorting *sorting = NULL, **sorting_tail = 
struct ref_format format = REF_FORMAT_INIT;
int icase = 0;
+   int edit_flag = 0;
struct option options[] = {
OPT_CMDMODE('l', "list", , N_("list tag names"), 'l'),
{ OPTION_INTEGER, 'n', NULL, , N_("n"),
@@ -386,6 +391,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
OPT_CALLBACK('m', "message", , N_("message"),
 N_("tag message"), parse_msg_arg),
OPT_FILENAME('F', "file", , N_("read message from 
file")),
+   OPT_BOOL('e', "edit", _flag, N_("force edit of tag 
message")),
OPT_BOOL('s', "sign", , N_("annotated and GPG-signed 
tag")),
OPT_STRING(0, "cleanup", _arg, N_("mode"),
N_("how to strip spaces and #comments from message")),
@@ -524,6 +530,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
die(_("tag '%s' already exists"), tag);
 
opt.message_given = msg.given || msgfile;
+   opt.use_editor = edit_flag;
 
if (!cleanup_arg || !strcmp(cleanup_arg, "strip"))
opt.cleanup_mode = CLEANUP_ALL;
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index a9af2de9960b..0630f2dee24b 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -452,6 +452,21 @@ test_expect_success \
test_cmp expect actual
 '
 
+get_tag_header annotated-tag-edit $commit commit $time >expect
+echo "An edited message" >>expect
+test_expect_success 'set up editor' '
+   write_script fakeeditor <<-\EOF
+   sed -e "s/A message/An edited message/g" <"$1" >"$1-"
+   mv "$1-" "$1"
+   EOF
+'
+test_expect_success \
+ 

[PATCHv3] tag: add --edit option

[Nicolas Morey-Chaisemartin]

Add a --edit option whichs allows modifying the messages provided by -m or -F,
the same way git commit --edit does.

Signed-off-by: Nicolas Morey-Chaisemartin <nmoreychaisemar...@suse.com>
---

Changes since v2 ( 
https://public-inbox.org/git/e99947cf-93ba-9376-f059-7f6a369d3...@suse.com ):
 * Add [-e] to git tag summary


 Documentation/git-tag.txt |  8 +++-
 builtin/tag.c | 11 +--
 t/t7004-tag.sh| 30 ++
 3 files changed, 46 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index 956fc019f984..1d17101bac39 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -9,7 +9,7 @@ git-tag - Create, list, delete or verify a tag object signed 
with GPG
 SYNOPSIS
 
 [verse]
-'git tag' [-a | -s | -u ] [-f] [-m  | -F ]
+'git tag' [-a | -s | -u ] [-f] [-m  | -F ] [-e]
 [ | ]
 'git tag' -d ...
 'git tag' [-n[]] -l [--contains ] [--no-contains ]
@@ -167,6 +167,12 @@ This option is only applicable when listing tags without 
annotation lines.
Implies `-a` if none of `-a`, `-s`, or `-u `
is given.
 
+-e::
+--edit::
+   The message taken from file with `-F` and command line with
+   `-m` are usually used as the tag message unmodified.
+   This option lets you further edit the message taken from these sources.
+
 --cleanup=::
This option sets how the tag message is cleaned up.
The  '' can be one of 'verbatim', 'whitespace' and 'strip'.  The
diff --git a/builtin/tag.c b/builtin/tag.c
index a7e6a5b0f234..ce5cac3dd23f 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -194,6 +194,7 @@ static int build_tag_object(struct strbuf *buf, int sign, 
struct object_id *resu
 
 struct create_tag_options {
unsigned int message_given:1;
+   unsigned int use_editor:1;
unsigned int sign;
enum {
CLEANUP_NONE,
@@ -224,7 +225,7 @@ static void create_tag(const struct object_id *object, 
const char *tag,
tag,
git_committer_info(IDENT_STRICT));
 
-   if (!opt->message_given) {
+   if (!opt->message_given || opt->use_editor) {
int fd;
 
/* write the template message before editing: */
@@ -233,7 +234,10 @@ static void create_tag(const struct object_id *object, 
const char *tag,
if (fd < 0)
die_errno(_("could not create file '%s'"), path);
 
-   if (!is_null_oid(prev)) {
+   if (opt->message_given) {
+   write_or_die(fd, buf->buf, buf->len);
+   strbuf_reset(buf);
+   } else if (!is_null_oid(prev)) {
write_tag_body(fd, prev);
} else {
struct strbuf buf = STRBUF_INIT;
@@ -372,6 +376,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
static struct ref_sorting *sorting = NULL, **sorting_tail = 
struct ref_format format = REF_FORMAT_INIT;
int icase = 0;
+   int edit_flag = 0;
struct option options[] = {
OPT_CMDMODE('l', "list", , N_("list tag names"), 'l'),
{ OPTION_INTEGER, 'n', NULL, , N_("n"),
@@ -386,6 +391,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
OPT_CALLBACK('m', "message", , N_("message"),
 N_("tag message"), parse_msg_arg),
OPT_FILENAME('F', "file", , N_("read message from 
file")),
+   OPT_BOOL('e', "edit", _flag, N_("force edit of tag 
message")),
OPT_BOOL('s', "sign", , N_("annotated and GPG-signed 
tag")),
OPT_STRING(0, "cleanup", _arg, N_("mode"),
N_("how to strip spaces and #comments from message")),
@@ -524,6 +530,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
die(_("tag '%s' already exists"), tag);
 
opt.message_given = msg.given || msgfile;
+   opt.use_editor = edit_flag;
 
if (!cleanup_arg || !strcmp(cleanup_arg, "strip"))
opt.cleanup_mode = CLEANUP_ALL;
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index a9af2de9960b..063996ddc05c 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -452,6 +452,21 @@ test_expect_success \
test_cmp expect actual
 '
 
+get_tag_header annotated-tag-edit $commit commit $time >expect
+echo "An edited message" >>expect
+test_expect_success 'set up editor' '
+   write_script fakeeditor <<-\EOF
+   sed -e "s/A message/An edited message/g" <"$1" >"$1-"
+   mv "$1-" "$1"
+   EOF
+'
+test_expect_success \
+   'creati

Re: [PATCHv2] tag: add --edit option

[Nicolas Morey-Chaisemartin]

Le 02/02/2018 à 20:16, Eric Sunshine a écrit :
> On Fri, Feb 2, 2018 at 11:48 AM, Nicolas Morey-Chaisemartin
> <nmoreychaisemar...@suse.com> wrote:
>> What message do you suggest ?  As I said in a previous mail, a
>> simple "Editor failure, cancelling {commit, tag}" should be enough
>> as launch_editor already outputs error messages describing what
>> issue the editor had.
>>
>> I don't think suggesting moving to --no-edit || -m || -F is that
>> helpful.  It's basically saying your setup is broken, but you can
>> workaround by setting those options (and not saying that you're
>> going to have some more issues later one).
> If it's the case the launch_editor() indeed outputs an appropriate
> error message, then the existing error message from tag.c is already
> appropriate when --edit is not specified.

I don't fully agree with the current message. The right thing to do is to fix 
the editor, not to hide the issue.
A better message would be "Editor failed. Fix it, or supply the message using 
either..."
At least we suggest the right way to do it first.

>  It's only the --edit case
> that the tag.c's additional message is somewhat weird. And, in fact,
> suppressing tag.c's message might be the correct thing to do in the
> --edit case:
>
> static void create_tag(...) {
> ...
> if (launch_editor(...)) {
>if (!opt->use_editor)
>fprintf(stderr, _("... use either -m or -F ..."));
> exit(1);
> }
>
> I don't feel strongly about it either way and am fine with just
> punting on the issue until someone actually complains about it.

The test should be opt->message_given && opt->use_editor.
If just --edit is provided but no -m/-F, --edit does not have any effect and it 
should be the same error message as when no option is given.

Nicolas

Re: [PATCHv2] tag: add --edit option

[Nicolas Morey-Chaisemartin]

Le 02/02/2018 à 10:57, Eric Sunshine a écrit :
> On Fri, Feb 2, 2018 at 2:15 AM, Nicolas Morey-Chaisemartin
> <nmoreychaisemar...@suse.com> wrote:
>> Le 02/02/2018 à 02:29, Eric Sunshine a écrit :
>>> On Thu, Feb 1, 2018 at 12:21 PM, Nicolas Morey-Chaisemartin
>>> <nmoreychaisemar...@suse.com> wrote:
>>>> - I'll post another series to fix the misleading messages in both commit.c 
>>>> and tag.c when launch_editor fails
>>> Typically, it's easier on Junio, from a patch management standpoint,
>>> if you submit all these related patches as a single series.
>>> Alternately, if you do want to submit those changes separately, before
>>> the current patch lands in "master", be sure to mention atop which
>>> patch (this one) the additional patch(es) should live. Thanks.
>> Well this patch does not touch any of the line concerned by fixing the error 
>> message. So both should be able to land in any order.
> Yup, that's a reasonable way to look at it. I see them as related
> (thus a potential patch series) simply because the existing error
> message in tag.c is fine, as is, until the --edit option is
> introduced, after which it becomes a bit iffy. Not a big deal, though.

OK ok I'll do it :)
What message do you suggest ?
As I said in a previous mail, a simple "Editor failure, cancelling {commit, 
tag}" should be enough as launch_editor already outputs error messages 
describing what issue the editor had.

I don't think suggesting moving to --no-edit || -m || -F is that helpful.
It's basically saying your setup is broken, but you can workaround by setting 
those options (and not saying that you're going to have some more issues later 
one).

>> Plus I've never had to look into localization yet so I'm going to screw up 
>> on the first few submissions (not counting on people that disagree or would 
>> prefer another message),
> As long as you just change the content of double-quoted string, you
> shouldn't have to worry about localization. The localization folks
> will handle the .po files and whatnot.

Sounds easy enough :)

Nicolas

Re: [PATCHv2] tag: add --edit option

[Nicolas Morey-Chaisemartin]

Le 02/02/2018 à 02:29, Eric Sunshine a écrit :
> On Thu, Feb 1, 2018 at 12:21 PM, Nicolas Morey-Chaisemartin
> <nmoreychaisemar...@suse.com> wrote:
>> Add a --edit option whichs allows modifying the messages provided by -m or 
>> -F,
>> the same way git commit --edit does.
>>
>> Signed-off-by: Nicolas Morey-Chaisemartin <nmoreychaisemar...@suse.com>
>> ---
>> Changes since v1:
>> - Fix usage string
>> - Use write_script to generate editor
>> - Rename editor to fakeeditor to match the other tests in the testsuite
> Thanks for explaining what changed since the previous attempt. It is
> also helpful for reviewers if you include a reference to the previous
> iteration, like this:
> https://public-inbox.org/git/450140f4-d410-4f1a-e5c1-c56d345a7...@suse.com/T/#u
>
> Cc:'ing reviewers of previous iterations is also good etiquette when
> submitting a new version.

I thought I did. My script might be glitchy. Sorry for that.

>
>> - I'll post another series to fix the misleading messages in both commit.c 
>> and tag.c when launch_editor fails
> Typically, it's easier on Junio, from a patch management standpoint,
> if you submit all these related patches as a single series.
> Alternately, if you do want to submit those changes separately, before
> the current patch lands in "master", be sure to mention atop which
> patch (this one) the additional patch(es) should live. Thanks.

Well this patch does not touch any of the line concerned by fixing the error 
message. So both should be able to land in any order.
Plus I've never had to look into localization yet so I'm going to screw up on 
the first few submissions (not counting on people that disagree or would prefer 
another message),
and I don't want this patch to get stuck in the pipe for that :)

>
>> diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
>> @@ -167,6 +167,12 @@ This option is only applicable when listing tags 
>> without annotation lines.
>> +-e::
>> +--edit::
>> +   The message taken from file with `-F` and command line with
>> +   `-m` are usually used as the tag message unmodified.
>> +   This option lets you further edit the message taken from these 
>> sources.
> You probably ought to add this new option to the command synopsis. In
> the git-commit man page, the synopsis mentions only '-e' (not --edit),
> so perhaps this man page could mirror that one. (Sorry for not
> noticing this earlier.)

Yep makes sense.

>> diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
>> @@ -452,6 +452,21 @@ test_expect_success \
>> +get_tag_header annotated-tag-edit $commit commit $time >expect
>> +echo "An edited message" >>expect
> Modern practice is to perform these "expect" setup actions (and all
> other actions) within tests themselves rather than outside of tests.
> However, consistency also has value, and since this test script is
> filled with this sort of stylized "expect" setup already, this may be
> fine, and probably not worth a re-roll. (A "modernization" patch by
> someone can come later if warranted.)
>
>> +test_expect_success 'set up editor' '
>> +   write_script fakeeditor <<-\EOF
>> +   sed -e "s/A message/An edited message/g" <"$1" >"$1-"
>> +   mv "$1-" "$1"
>> +   EOF
>> +'

It's probably worth doing a whole cleanup of these (and switch to write script) 
in a dedicated patch series.

Nicolas

[PATCHv2] tag: add --edit option

[Nicolas Morey-Chaisemartin]

Add a --edit option whichs allows modifying the messages provided by -m or -F,
the same way git commit --edit does.

Signed-off-by: Nicolas Morey-Chaisemartin <nmoreychaisemar...@suse.com>
---

Changes since v1:
- Fix usage string
- Use write_script to generate editor
- Rename editor to fakeeditor to match the other tests in the testsuite
- I'll post another series to fix the misleading messages in both commit.c and 
tag.c when launch_editor fails

 Documentation/git-tag.txt |  6 ++
 builtin/tag.c | 11 +--
 t/t7004-tag.sh| 30 ++
 3 files changed, 45 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index 956fc019f984..b9e5a993bea0 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -167,6 +167,12 @@ This option is only applicable when listing tags without 
annotation lines.
Implies `-a` if none of `-a`, `-s`, or `-u `
is given.
 
+-e::
+--edit::
+   The message taken from file with `-F` and command line with
+   `-m` are usually used as the tag message unmodified.
+   This option lets you further edit the message taken from these sources.
+
 --cleanup=::
This option sets how the tag message is cleaned up.
The  '' can be one of 'verbatim', 'whitespace' and 'strip'.  The
diff --git a/builtin/tag.c b/builtin/tag.c
index a7e6a5b0f234..ce5cac3dd23f 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -194,6 +194,7 @@ static int build_tag_object(struct strbuf *buf, int sign, 
struct object_id *resu
 
 struct create_tag_options {
unsigned int message_given:1;
+   unsigned int use_editor:1;
unsigned int sign;
enum {
CLEANUP_NONE,
@@ -224,7 +225,7 @@ static void create_tag(const struct object_id *object, 
const char *tag,
tag,
git_committer_info(IDENT_STRICT));
 
-   if (!opt->message_given) {
+   if (!opt->message_given || opt->use_editor) {
int fd;
 
/* write the template message before editing: */
@@ -233,7 +234,10 @@ static void create_tag(const struct object_id *object, 
const char *tag,
if (fd < 0)
die_errno(_("could not create file '%s'"), path);
 
-   if (!is_null_oid(prev)) {
+   if (opt->message_given) {
+   write_or_die(fd, buf->buf, buf->len);
+   strbuf_reset(buf);
+   } else if (!is_null_oid(prev)) {
write_tag_body(fd, prev);
} else {
struct strbuf buf = STRBUF_INIT;
@@ -372,6 +376,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
static struct ref_sorting *sorting = NULL, **sorting_tail = 
struct ref_format format = REF_FORMAT_INIT;
int icase = 0;
+   int edit_flag = 0;
struct option options[] = {
OPT_CMDMODE('l', "list", , N_("list tag names"), 'l'),
{ OPTION_INTEGER, 'n', NULL, , N_("n"),
@@ -386,6 +391,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
OPT_CALLBACK('m', "message", , N_("message"),
 N_("tag message"), parse_msg_arg),
OPT_FILENAME('F', "file", , N_("read message from 
file")),
+   OPT_BOOL('e', "edit", _flag, N_("force edit of tag 
message")),
OPT_BOOL('s', "sign", , N_("annotated and GPG-signed 
tag")),
OPT_STRING(0, "cleanup", _arg, N_("mode"),
N_("how to strip spaces and #comments from message")),
@@ -524,6 +530,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
die(_("tag '%s' already exists"), tag);
 
opt.message_given = msg.given || msgfile;
+   opt.use_editor = edit_flag;
 
if (!cleanup_arg || !strcmp(cleanup_arg, "strip"))
opt.cleanup_mode = CLEANUP_ALL;
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index a9af2de9960b..063996ddc05c 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -452,6 +452,21 @@ test_expect_success \
test_cmp expect actual
 '
 
+get_tag_header annotated-tag-edit $commit commit $time >expect
+echo "An edited message" >>expect
+test_expect_success 'set up editor' '
+   write_script fakeeditor <<-\EOF
+   sed -e "s/A message/An edited message/g" <"$1" >"$1-"
+   mv "$1-" "$1"
+   EOF
+'
+test_expect_success \
+   'creating an annotated tag with -m message --edit should succeed' '
+   EDITOR=./fakeeditor git tag -m "A message" --edit 
annotated-tag-edit &&
+  

Re: [PATCH] tag: add --edit option

[Nicolas Morey-Chaisemartin]

Le 01/02/2018 à 11:56, Eric Sunshine a écrit :
> On Thu, Feb 1, 2018 at 5:43 AM, Nicolas Morey-Chaisemartin
> <nmoreychaisemar...@suse.de> wrote:
>> Le 01/02/2018 à 11:34, Nicolas Morey-Chaisemartin a écrit :
>>> Le 01/02/2018 à 11:16, Eric Sunshine a écrit :
>>>> A little below this change is where launch_editor() is actually
>>>> invoked. If it fails for some reason, it prints:
>>>>
>>>> Please supply the message using either -m or -F option.
>>>>
>>>> which seems a bit counterintuitive if the user *did* specify one of
>>>> those options along with --edit. I wonder if that message needs to be
>>>> adjusted.
>>>>
>>> Yes I'll fix this.
>> I just checked what commit.c does and it seems to behave as my patch:
>> if (launch_editor(git_path_commit_editmsg(), NULL, env.argv)) {
>> fprintf(stderr,
>> _("Please supply the message using either -m or -F option.\n"));
>>
>> To be honest the message is not that clear either.
>> If I'm reading launch_editor right most (or all) its falire are du to a 
>> failure to launch the editor or the editor crashed/exited with an error.
>> In this case, I wouldn't advise the user to use -m or -F but to fix its 
>> editor.
> Indeed, I also looked at the implementation of launch_editor(), and my
> "wondering" about whether the message needed adjustment was just that.
> The message seems somewhat counterintuitive in this case, but I didn't
> necessarily have a better suggestion. A valid response, therefore,
> might be to punt on it and leave that change for the future, or
> perhaps take it on as a second patch which adjusts the message in both
> commands. I don't have strong feelings about it at this time.

It seems all the error paths from launch_editor have an error message.
A simple "Editor failure, cancelling {commit, tag}" would probably be a better 
error message.
I'll post another series for that.

Re: [PATCH] tag: add --edit option

[Nicolas Morey-Chaisemartin]

Le 01/02/2018 à 11:34, Nicolas Morey-Chaisemartin a écrit :
>
> Le 01/02/2018 à 11:16, Eric Sunshine a écrit :
>> On Thu, Feb 1, 2018 at 4:49 AM, Nicolas Morey-Chaisemartin
>> <nmoreychaisemar...@suse.com> wrote:
>>> Add a --edit option whichs allows modifying the messages provided by -m or 
>>> -F,
>>> the same way git commit --edit does.
>>>
>>> Signed-off-by: Nicolas Morey-Chaisemartin <nmoreychaisemar...@suse.com>
>>> ---
>>> diff --git a/builtin/tag.c b/builtin/tag.c
>>> @@ -224,7 +225,7 @@ static void create_tag(const struct object_id *object, 
>>> const char *tag,
>>> -   if (!opt->message_given) {
>>> +   if (!opt->message_given || opt->use_editor) {
>>>
>>> -   if (!is_null_oid(prev)) {
>>> +   if (opt->message_given) {
>>> +   write_or_die(fd, buf->buf, buf->len);
>>> +   strbuf_reset(buf);
>>> +   } else if (!is_null_oid(prev)) {
>>> write_tag_body(fd, prev);
>>> } else {
>> A little below this change is where launch_editor() is actually
>> invoked. If it fails for some reason, it prints:
>>
>> Please supply the message using either -m or -F option.
>>
>> which seems a bit counterintuitive if the user *did* specify one of
>> those options along with --edit. I wonder if that message needs to be
>> adjusted.
>>
> Yes I'll fix this.
I just checked what commit.c does and it seems to behave as my patch:
        if (launch_editor(git_path_commit_editmsg(), NULL, env.argv)) {
            fprintf(stderr,
            _("Please supply the message using either -m or -F option.\n"));
            exit(1);
        }


To be honest the message is not that clear either.
If I'm reading launch_editor right most (or all) its falire are du to a failure 
to launch the editor or the editor crashed/exited with an error.
In this case, I wouldn't advise the user to use -m or -F but to fix its editor.

Nicolas

Re: [PATCH] tag: add --edit option

[Nicolas Morey-Chaisemartin]

Le 01/02/2018 à 11:16, Eric Sunshine a écrit :
> On Thu, Feb 1, 2018 at 4:49 AM, Nicolas Morey-Chaisemartin
> <nmoreychaisemar...@suse.com> wrote:
>> Add a --edit option whichs allows modifying the messages provided by -m or 
>> -F,
>> the same way git commit --edit does.
>>
>> Signed-off-by: Nicolas Morey-Chaisemartin <nmoreychaisemar...@suse.com>
>> ---
>> diff --git a/builtin/tag.c b/builtin/tag.c
>> @@ -224,7 +225,7 @@ static void create_tag(const struct object_id *object, 
>> const char *tag,
>> -   if (!opt->message_given) {
>> +   if (!opt->message_given || opt->use_editor) {
>>
>> -   if (!is_null_oid(prev)) {
>> +   if (opt->message_given) {
>> +   write_or_die(fd, buf->buf, buf->len);
>> +   strbuf_reset(buf);
>> +   } else if (!is_null_oid(prev)) {
>> write_tag_body(fd, prev);
>> } else {
> A little below this change is where launch_editor() is actually
> invoked. If it fails for some reason, it prints:
>
> Please supply the message using either -m or -F option.
>
> which seems a bit counterintuitive if the user *did* specify one of
> those options along with --edit. I wonder if that message needs to be
> adjusted.
>
Yes I'll fix this.

>> diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
>> @@ -452,6 +452,23 @@ test_expect_success \
>> +get_tag_header annotated-tag-edit $commit commit $time >expect
>> +echo "An edited message" >>expect
>> +test_expect_success 'set up editor' '
>> +   cat >editor <<-\EOF &&
>> +   #!/bin/sh
>> +   sed -e "s/A message/An edited message/g" <"$1" >"$1-"
>> +   mv "$1-" "$1"
>> +   EOF
>> +   chmod 755 editor
> If you use write_script() to create the fake editor, then it supplies
> the #!/bin/sh line for you and does the 'chmod', so you only need to
> supply the actual script payload. Also, other "editors" in this test
> file are named "fakeeditor", so perhaps follow suit.
>
> write_script fakeeditor <<-\EOF
> sed -e "s/A message/An edited message/g" <"$1" >"$1-"
> mv "$1-" "$1"
> EOF
>
I dumbly copied the test from commit --edit as it was my reference.
I'll fix the names and switch to write_script.

Thanks

Nicolas

[PATCH] tag: add --edit option

[Nicolas Morey-Chaisemartin]

Add a --edit option whichs allows modifying the messages provided by -m or -F,
the same way git commit --edit does.

Signed-off-by: Nicolas Morey-Chaisemartin <nmoreychaisemar...@suse.com>
---
 Documentation/git-tag.txt |  6 ++
 builtin/tag.c | 11 +--
 t/t7004-tag.sh| 34 ++
 3 files changed, 49 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index 956fc019f984..b9e5a993bea0 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -167,6 +167,12 @@ This option is only applicable when listing tags without 
annotation lines.
Implies `-a` if none of `-a`, `-s`, or `-u `
is given.
 
+-e::
+--edit::
+   The message taken from file with `-F` and command line with
+   `-m` are usually used as the tag message unmodified.
+   This option lets you further edit the message taken from these sources.
+
 --cleanup=::
This option sets how the tag message is cleaned up.
The  '' can be one of 'verbatim', 'whitespace' and 'strip'.  The
diff --git a/builtin/tag.c b/builtin/tag.c
index a7e6a5b0f234..91c60829d5f9 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -194,6 +194,7 @@ static int build_tag_object(struct strbuf *buf, int sign, 
struct object_id *resu
 
 struct create_tag_options {
unsigned int message_given:1;
+   unsigned int use_editor:1;
unsigned int sign;
enum {
CLEANUP_NONE,
@@ -224,7 +225,7 @@ static void create_tag(const struct object_id *object, 
const char *tag,
tag,
git_committer_info(IDENT_STRICT));
 
-   if (!opt->message_given) {
+   if (!opt->message_given || opt->use_editor) {
int fd;
 
/* write the template message before editing: */
@@ -233,7 +234,10 @@ static void create_tag(const struct object_id *object, 
const char *tag,
if (fd < 0)
die_errno(_("could not create file '%s'"), path);
 
-   if (!is_null_oid(prev)) {
+   if (opt->message_given) {
+   write_or_die(fd, buf->buf, buf->len);
+   strbuf_reset(buf);
+   } else if (!is_null_oid(prev)) {
write_tag_body(fd, prev);
} else {
struct strbuf buf = STRBUF_INIT;
@@ -372,6 +376,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
static struct ref_sorting *sorting = NULL, **sorting_tail = 
struct ref_format format = REF_FORMAT_INIT;
int icase = 0;
+   int edit_flag = 0;
struct option options[] = {
OPT_CMDMODE('l', "list", , N_("list tag names"), 'l'),
{ OPTION_INTEGER, 'n', NULL, , N_("n"),
@@ -386,6 +391,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
OPT_CALLBACK('m', "message", , N_("message"),
 N_("tag message"), parse_msg_arg),
OPT_FILENAME('F', "file", , N_("read message from 
file")),
+   OPT_BOOL('e', "edit", _flag, N_("force edit of commit")),
OPT_BOOL('s', "sign", , N_("annotated and GPG-signed 
tag")),
OPT_STRING(0, "cleanup", _arg, N_("mode"),
N_("how to strip spaces and #comments from message")),
@@ -524,6 +530,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
die(_("tag '%s' already exists"), tag);
 
opt.message_given = msg.given || msgfile;
+   opt.use_editor = edit_flag;
 
if (!cleanup_arg || !strcmp(cleanup_arg, "strip"))
opt.cleanup_mode = CLEANUP_ALL;
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index a9af2de9960b..60e3a53f297f 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -452,6 +452,23 @@ test_expect_success \
test_cmp expect actual
 '
 
+get_tag_header annotated-tag-edit $commit commit $time >expect
+echo "An edited message" >>expect
+test_expect_success 'set up editor' '
+   cat >editor <<-\EOF &&
+   #!/bin/sh
+   sed -e "s/A message/An edited message/g" <"$1" >"$1-"
+   mv "$1-" "$1"
+   EOF
+   chmod 755 editor
+'
+test_expect_success \
+   'creating an annotated tag with -m message --edit should succeed' '
+   EDITOR=./editor git tag -m "A message" --edit annotated-tag-edit &&
+   get_tag_msg annotated-tag-edit >actual &&
+   test_cmp expect actual
+'
+
 cat >msgfile <expect
+sed -e "s/Another message/Another edited message/g" msgfile >>expect
+test_expect_succe

[PATCH] imap-send: URI encode server folder

[Nicolas Morey-Chaisemartin]

URI encode the server folder string before passing it to libcurl.
This fixes the access to the draft folder on Gmail accounts (named 
[Gmail]/Drafts)

Reported-by: Doron Behar <doron.be...@gmail.com>
Signed-off-by: Nicolas Morey-Chaisemartin <nmoreychaisemar...@suse.com>
---
 imap-send.c | 8 +++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/imap-send.c b/imap-send.c
index 54e6a80fd..36c7c1b4f 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -1412,6 +1412,7 @@ static CURL *setup_curl(struct imap_server_conf *srvc, 
struct credential *cred)
 {
CURL *curl;
struct strbuf path = STRBUF_INIT;
+   char *uri_encoded_folder;
 
if (curl_global_init(CURL_GLOBAL_ALL) != CURLE_OK)
die("curl_global_init failed");
@@ -1429,7 +1430,12 @@ static CURL *setup_curl(struct imap_server_conf *srvc, 
struct credential *cred)
strbuf_addstr(, server.host);
if (!path.len || path.buf[path.len - 1] != '/')
strbuf_addch(, '/');
-   strbuf_addstr(, server.folder);
+
+   uri_encoded_folder = curl_easy_escape(curl, server.folder, 0);
+   if (!uri_encoded_folder)
+   die("failed to encode server folder");
+   strbuf_addstr(, uri_encoded_folder);
+   curl_free(uri_encoded_folder);
 
curl_easy_setopt(curl, CURLOPT_URL, path.buf);
strbuf_release();
-- 
2.15.1.272.g8e603414b

Re: imap-send with gmail: curl_easy_perform() failed: URL using bad/illegal format or missing URL

[Nicolas Morey-Chaisemartin]

Le 30/11/2017 à 10:46, Daniel Stenberg a écrit :
> On Thu, 30 Nov 2017, Nicolas Morey-Chaisemartin wrote:
>
>> This is due to the weird "[Gmail]" prefix in the folder.
>> I tried manually replacing it with:
>>     folder = %5BGmail%5D/Drafts
>> in .git/config and it works.
>>
>> curl is doing some fancy handling with brackets and braces. It make sense 
>> for multiple FTP downloads like ftp://ftp.numericals.com/file[1-100].txt, 
>> not in our case. The curl command line has a --globoff argument to disable 
>> this "regexp" support and it seems to fix the gmail case. However I couldn't 
>> find a way to change this value through the API...
>
> That's just a feature of the command line tool, "globbing" isn't a function 
> provided by the library. libcurl actually "just" expects a plain old URL.
>
Yep that what I figured looking a bit further in the code.

> But with the risk of falling through the cracks into the rathole that is 
> "what is a URL" (I've blogged about the topic several times in the past and I 
> will surely do it again in the future):
>
> A "legal" URL (as per RFC 3986) does not contain brackets, such symbols 
> should be used URL encoded: %5B and %5D.
>
> This said: I don't know exactly why brackets cause a problem in this case. It 
> could still be worth digging into and see if libcurl could deal with them 
> better here...
>

It would make sense to have a way to ask libcurl to URI encode for us. I'm 
guessing there's already the code for that somewhere in curl and we would be 
wise to use it.
But to work wqith older version we'll have to do it ourselves anyway.

Nicolas

Re: imap-send with gmail: curl_easy_perform() failed: URL using bad/illegal format or missing URL

[Nicolas Morey-Chaisemartin]

Le 30/11/2017 à 10:39, Nicolas Morey-Chaisemartin a écrit :
>
> Le 30/11/2017 à 03:04, Jonathan Nieder a écrit :
>> (+cc: Nicolas)
>> Hi,
>>
>> Doron Behar wrote:
>>
>>> I'm trying to send a patch with the command `git imap-send`, I used the
>>> examples in the manual page as the main reference for my configuration:
>>>
>>> ```
>>> [imap]
>>> folder = "[Gmail]/Drafts"
>>> host = imaps://imap.gmail.com
>>> user = doron.be...@gmail.com
>>> port = 993
>>> sslverify = false
>>> ```
>>>
>>> This is my `cat patch.out | git imap-send` output:
>>>
>>> ```
>>> Password for 'imaps://doron.be...@gmail.com@imap.gmail.com':
>>> sending 3 messages
>>> curl_easy_perform() failed: URL using bad/illegal format or missing URL
>>> ```
>> Thanks for reporting this.  I suspect this is related to
>> v2.15.0-rc0~63^2 (imap-send: use curl by default when possible,
>> 2017-09-14) --- e.g. perhaps our custom IMAP code was doing some
>> escaping on the username that libcurl does not do.
>>
>> "man git imap-send" says this is a recommended configuration, so I
>> don't think it's a configuration error.
>>
>> What platform are you on?  What version of libcurl are you using?
>>
>> In libcurl::lib/easy.c I am also seeing
>>
>> if(mcode)
>>   return CURLE_URL_MALFORMAT; /* TODO: return a proper error! */
>>
>> which looks suspicious.
>>
>> Nicolas, am I on the right track?
>>
>> Thanks,
>> Jonathan
>>
> This is due to the weird "[Gmail]" prefix in the folder.
> I tried manually replacing it with:
>     folder = %5BGmail%5D/Drafts
> in .git/config and it works.
>
> curl is doing some fancy handling with brackets and braces. It make sense for 
> multiple FTP downloads like ftp://ftp.numericals.com/file[1-100].txt, not in 
> our case.
> The curl command line has a --globoff argument to disable this "regexp" 
> support and it seems to fix the gmail case.

In fact no, StackOverflow was wrong :)

> However I couldn't find a way to change this value through the API...
>
> I guess we should open a bug upstream to get access to this setting through 
> the API and add a patch that HTTP encode brackets and braces in the meantime.
>
This means with have to URI encode the folder. DO we have a helper for that ?

Nicolas

Re: imap-send with gmail: curl_easy_perform() failed: URL using bad/illegal format or missing URL

[Nicolas Morey-Chaisemartin]

Le 30/11/2017 à 03:04, Jonathan Nieder a écrit :
> (+cc: Nicolas)
> Hi,
>
> Doron Behar wrote:
>
>> I'm trying to send a patch with the command `git imap-send`, I used the
>> examples in the manual page as the main reference for my configuration:
>>
>> ```
>> [imap]
>>  folder = "[Gmail]/Drafts"
>>  host = imaps://imap.gmail.com
>>  user = doron.be...@gmail.com
>>  port = 993
>>  sslverify = false
>> ```
>>
>> This is my `cat patch.out | git imap-send` output:
>>
>> ```
>> Password for 'imaps://doron.be...@gmail.com@imap.gmail.com':
>> sending 3 messages
>> curl_easy_perform() failed: URL using bad/illegal format or missing URL
>> ```
> Thanks for reporting this.  I suspect this is related to
> v2.15.0-rc0~63^2 (imap-send: use curl by default when possible,
> 2017-09-14) --- e.g. perhaps our custom IMAP code was doing some
> escaping on the username that libcurl does not do.
>
> "man git imap-send" says this is a recommended configuration, so I
> don't think it's a configuration error.
>
> What platform are you on?  What version of libcurl are you using?
>
> In libcurl::lib/easy.c I am also seeing
>
> if(mcode)
>   return CURLE_URL_MALFORMAT; /* TODO: return a proper error! */
>
> which looks suspicious.
>
> Nicolas, am I on the right track?
>
> Thanks,
> Jonathan
>

This is due to the weird "[Gmail]" prefix in the folder.
I tried manually replacing it with:
    folder = %5BGmail%5D/Drafts
in .git/config and it works.

curl is doing some fancy handling with brackets and braces. It make sense for 
multiple FTP downloads like ftp://ftp.numericals.com/file[1-100].txt, not in 
our case.
The curl command line has a --globoff argument to disable this "regexp" support 
and it seems to fix the gmail case.
However I couldn't find a way to change this value through the API...

I guess we should open a bug upstream to get access to this setting through the 
API and add a patch that HTTP encode brackets and braces in the meantime.

Nicolas

Re: [RFC 2/3] am: semi working --cover-at-tip

[Nicolas Morey-Chaisemartin]

Le 14/11/2017 à 10:17, Nicolas Morey-Chaisemartin a écrit :
>
> Le 14/11/2017 à 07:00, Junio C Hamano a écrit :
>> Nicolas Morey-Chaisemartin <nmoreychaisemar...@suse.de> writes:
>>
>> By the way, don't we want to sanity check state->last (which we
>> learn by running "git mailsplit" that splits the incoming mbox into
>> pieces and counts the number of messages) against state->series_len?
>> Sometimes people send [PATCH 0-6/6], a 6-patch series with a cover
>> letter, and then follow-up with [PATCH 7/6].  For somebody like me,
>> it would be more convenient if the above code (more-or-less) ignored
>> series_len and called do_apply_cover() after applying the last patch
>> (which would be [PATCH 7/6]) based on what state->last says.
> I thought about that.
> Is there a use case for cover after the last patch works and removes the need 
> to touch am_next (can be done out of the loop in am_run).

Do you have an opinion on that ? It has quite a big impact on how things are 
done !
Single series only would mean a simple flush at the end.
Multiple series makes things a whole lot complex.
We do not know the series_id of the next patch until it's parsed by parse_mail.
Which would mean interrupting parse_mail when detecting a new series to call 
parse_mail on the cover_id plus an extra detection at the end of the loop.

Nicolas

Re: [RFC 3/3] log: add an option to generate cover letter from a branch tip

[Nicolas Morey-Chaisemartin]

Le 14/11/2017 à 14:05, Junio C Hamano a écrit :
> Nicolas Morey-Chaisemartin <nmoreychaisemar...@suse.de> writes:
>
>> The triple dash is so that the diffstat/shortlog as not seen as
>> part of the cover letter.  As said in the cover letter for this
>> series, it kinda breaks legacy behaviour right now.  It should
>> either be printed only for cover-at-tip, or a new separator should
>> be added.
> This reminds me of a couple of random thoughts I had, so before I
> disconnect from my terminal and forget about them...
>
> [1] format-patch and am must round-trip.
>
> I mentioned four uses cases around the "cover letter at the
> tip" in my earlier message
>
> https://public-inbox.org/git/xmqqbmk68o9d@gitster.mtv.corp.google.com/
>
> Specifically, (2) we should be able to run "format-patch" and record
> the log message of the empty commit at the tip to the cover letter,
> and (4) we should be able to accept such output with "am" and end up
> with the same sequence of commits as the original (modulo committer
> identity and timestamps).  So from the output we produce with this
> step, "am" should be able to split the material that came from the
> original empty commit from the surrounding cruft like shortlog and
> diffstat.  The output format of this step needs to be designed with
> that in mind.

This should be the case with the current RFC (apart from the branch description 
which is kept at the moment).
Things like git am --signoff will break this of course.

>
> [2] reusing cover letter material in merge may not be ideal.
>
> When people write a cover letter, they write different things in it.
> What they wanted to achieve, why they chose the approach they took,
> how the series is organized, which part of the series they find iffy
> and/orneeds special attention from the reviewers, where to find the
> previous iteration, what changed since the previous iterations, etc.
>
> All of them are to help the reviewers, many of who have already
> looked at the previous rounds, to understand and judge this round of
> the submission.
>
> The message in a merge commit as a part of the final history,
> however, cannot refer to anything from "previous rounds", as the
> previous attempts are not part of the final history readers of "git
> log" can refer to whey they are trying to understand the merge.
> What exactly goes in a merge commit and how the messages are phrased
> may be different from project to project, but for this project, I've
> been trying to write them in an end-user facing terms, i.e. they are
> designed in such a way that "git log --first-parent --merges" can be
> read as if they were entries in the release notes, summarizing fixes
> and features by describing their user-visible effects.  This is only
> one part of what people write in their cover letters (i.e. "what
> they wanted to achive").
>
> So there probably needs a convention meant to be followed by human
> users when writing cover letters, so a mechanical process can tell
> which part of the text is to be made into the merge commit without
> understanding human languages.

In the long term, I agree this would be nice.
As a first step, could we force the --edit option when using --cover-at-tip ?
The basic merge message would come from the cover letter but can/should be 
edited to clear the extra stuff out.

Auto-stripping those extra infos, may also conflicts with the point (3) from 
your previous mail.
If git merge is to keep only the relevant infos, git format-patch from this 
merge will not be able to access those.

The advantage of a manual edit is that it's the commiter's choice. Keep the 
info if the branch is to be resubmitted. Drop them once it's merged for good.

Nicolas

Re: [RFC 3/3] log: add an option to generate cover letter from a branch tip

[Nicolas Morey-Chaisemartin]

Le 14/11/2017 à 07:14, Junio C Hamano a écrit :
> Nicolas Morey-Chaisemartin <nmoreychaisemar...@suse.de> writes:
>
>> -const char *body = "*** SUBJECT HERE ***\n\n*** BLURB HERE ***\n";
>> -const char *msg;
>> +const char *body = "*** SUBJECT HERE ***\n\n*** BLURB HERE ***\n\n";
> H.

The \n from fprintf(rev->diffopt.file, "%s\n", sb.buf); added an extra line 
after the cover which is fine for the default one but changed the commit value 
(at least at some point while I was playing with scissors)
so I moved it up there to avoid adding a if() around the fprintf

>
>> @@ -1021,17 +1021,21 @@ static void make_cover_letter(struct rev_info *rev, 
>> int use_stdout,
>>  if (!branch_name)
>>  branch_name = find_branch_name(rev);
>>  
>> -msg = body;
>>  pp.fmt = CMIT_FMT_EMAIL;
>>  pp.date_mode.type = DATE_RFC2822;
>>  pp.rev = rev;
>>  pp.print_email_subject = 1;
>> -pp_user_info(, NULL, , committer, encoding);
>> -pp_title_line(, , , encoding, need_8bit_cte);
>> -pp_remainder(, , , 0);
>> -add_branch_description(, branch_name);
>> -fprintf(rev->diffopt.file, "%s\n", sb.buf);
>>  
>> +if (!cover_at_tip_commit) {
>> +pp_user_info(, NULL, , committer, encoding);
>> +pp_title_line(, , , encoding, need_8bit_cte);
>> +pp_remainder(, , , 0);
>> +} else {
>> +pretty_print_commit(, cover_at_tip_commit, );
>> +}
>> +add_branch_description(, branch_name);
>> +fprintf(rev->diffopt.file, "%s", sb.buf);
>> +fprintf(rev->diffopt.file, "---\n", sb.buf);
>>  strbuf_release();
> I would have expected that this feature would not change anything
> other than replacing the constant string *body we unconditionally
> print with the log message of the empty commit at the tip, so from
> that expectation, I was hoping that a patch looked nothing more than
> this:
>
>  builtin/log.c | 6 +-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/log.c b/builtin/log.c
> index 6c1fa896ad..0af19d5b36 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -986,6 +986,7 @@ static void make_cover_letter(struct rev_info *rev, int 
> use_stdout,
> struct commit *origin,
> int nr, struct commit **list,
> const char *branch_name,
> +   struct commit *cover,
> int quiet)
>  {
>   const char *committer;
> @@ -1021,7 +1022,10 @@ static void make_cover_letter(struct rev_info *rev, 
> int use_stdout,
>   if (!branch_name)
>   branch_name = find_branch_name(rev);
>  
> - msg = body;
> + if (cover)
> + msg = get_cover_from_commit(cover);
> + else
> + msg = body;
>   pp.fmt = CMIT_FMT_EMAIL;
>   pp.date_mode.type = DATE_RFC2822;
>   pp.rev = rev;
>
>
> plus a newly written function get_cover_from_commit().  Why does
> this patch need to change a lot more than that, I have to wonder.

The added code is to avoid the get_cover_from_commit generating a single strbuf 
that needs to be reparse/resplit by pp_user_info/pp_title_line/pp_remainder.
There was a helper that did the right job in my case so I took it ;)

The triple dash is so that the diffstat/shortlog as not seen as part of the 
cover letter.
As said in the cover letter for this series, it kinda breaks legacy behaviour 
right now.
It should either be printed only for cover-at-tip, or a new separator should be 
added.

>
> This is totally unrelated, but I wonder if it makes sense to do
> something similar for branch.description, too.  If the user has a
> meaningful description prepared with "git branch --edit-desc", it is
> somewhat insulting to the user to still add "*** BLURB HERE ***".

I guess so.
And the branch description should probably not be added when using cover-at-tip 
either.

Re: [RFC 2/3] am: semi working --cover-at-tip

[Nicolas Morey-Chaisemartin]

Le 14/11/2017 à 07:00, Junio C Hamano a écrit :
> Nicolas Morey-Chaisemartin <nmoreychaisemar...@suse.de> writes:
>
>>  if (!git_config_get_bool("commit.gpgsign", ))
>>  state->sign_commit = gpgsign ? "" : NULL;
>> +
>>  }
> Please give at least a cursory proof-reading before sending things
> out.
>
>> @@ -1106,14 +1131,6 @@ static void am_next(struct am_state *state)
>>  
>>  oidclr(>orig_commit);
>>  unlink(am_path(state, "original-commit"));
>> -
>> -if (!get_oid("HEAD", ))
>> -write_state_text(state, "abort-safety", oid_to_hex());
>> -else
>> -write_state_text(state, "abort-safety", "");
>> -
>> -state->cur++;
>> -write_state_count(state, "next", state->cur);
> Moving these lines to a later part of the source file is fine, but
> can you do so as a separate preparatory patch that does not change
> anything else?  That would unclutter the main patch that adds the
> feature, allowing better reviews from reviewers.
>
> The hunk below...

Sure. I usually do all this later in the process.
>> +/**
>> + * Increments the patch pointer, and cleans am_state for the application of 
>> the
>> + * next patch.
>> + */
>> +static void am_next(struct am_state *state)
>> +{
>> +struct object_id head;
>> +
>> +/* Flush the cover letter if needed */
>> +if (state->cover_at_tip == 1 &&
>> +state->series_len > 0 &&
>> +state->series_id == state->series_len &&
>> +state->cover_id > 0)
>> +do_apply_cover(state);
>> +
>> +am_clean(state);
>> +
>> +if (!get_oid("HEAD", ))
>> +write_state_text(state, "abort-safety", oid_to_hex());
>> +else
>> +write_state_text(state, "abort-safety", "");
>> +
>> +state->cur++;
>> +write_state_count(state, "next", state->cur);
>> +}
> ... if you followed that "separate preparatory step" approach, would
> show clearly that you added the logic to call do_apply_cover() when
> we transition after applying the Nth patch of a series with N patches,
> as all the existing lines will show only as unchanged context lines.

Agreed. The split of am_clean should probably have its own commit too.

>
> By the way, don't we want to sanity check state->last (which we
> learn by running "git mailsplit" that splits the incoming mbox into
> pieces and counts the number of messages) against state->series_len?
> Sometimes people send [PATCH 0-6/6], a 6-patch series with a cover
> letter, and then follow-up with [PATCH 7/6].  For somebody like me,
> it would be more convenient if the above code (more-or-less) ignored
> series_len and called do_apply_cover() after applying the last patch
> (which would be [PATCH 7/6]) based on what state->last says.

I thought about that.
Is there a use case for cover after the last patch works and removes the need 
to touch am_next (can be done out of the loop in am_run).

If that multiple series in a mbox is something people do, your concern could be 
solved by flushing the cover when state->series_id goes back to a lower value.

Nicolas

Re: [RFC 1/3] mailinfo: extract patch series id

[Nicolas Morey-Chaisemartin]

Le 14/11/2017 à 06:47, Junio C Hamano a écrit :
> Nicolas Morey-Chaisemartin <nmoreychaisemar...@suse.de> writes:
>
>> Extract the patch ID and series length from the [PATCH N/M]
>>  prefix in the mail header
>>
>> Signed-off-by: Nicolas Morey-Chaisemartin <nico...@morey-chaisemartin.com>
>> ---
>>  mailinfo.c | 35 +++
>>  mailinfo.h |  2 ++
>>  2 files changed, 37 insertions(+)
> As JTan already mentioned, relying on a substring "PATCH" may not be
> very reliable, and trying to locate "%d/%d]" feels like a better
> approach.
>
> cleanup_subject() is called only when keep_subject is false, so this
> code will not trigger in that case at all.  Is this intended?
>
> I would have expected that a new helper function would be written,
> without changing existing helpers like cleanup_subject(), and that
> new helper gets called by handle_info() after output_header_lines()
> helper is called for the "Subject".
Isn't that too late ?
If keep subject is not set, will cleanup_subject not drop all those info from 
the strbuf  ?
But yes it is not in the right function now.
But I would call the function before
            if (!mi->keep_subject) {

>
> Whenever mailinfo learns to glean a new useful piece of information,
> it should be made available to scripts that run "git mailinfo", too.
> Perhaps show something like
>
>   PatchNumber: 1
>   TotalPatches: 3
>
> at the end of handle_info() to mi->output?  I do not think existing
> tools mind too much, even if we added a for-debug output e.g.
>
>   RawSubject: [RFC 1/3] mailinfo: extract patch series id
>
> to the output.
Will do.

Re: [RFC 0/3] Add support for --cover-at-tip

[Nicolas Morey-Chaisemartin]

Le 13/11/2017 à 20:40, Jonathan Tan a écrit :
> On Mon, 13 Nov 2017 18:13:27 +0100
> Nicolas Morey-Chaisemartin <nmoreychaisemar...@suse.de> wrote:
>
>> v2:
>> - Enhance mailinfo to parse patch series id from subject
>> - Detect cover using mailinfo parsed ids in git am
> I noticed that this was done in the patch set by searching for "PATCH" -
> that is probably quite error-prone as not all patches will have a
> subject line of that form. It may be better to search for "0/" and
> ensure that it is immediately followed by an integer.

Yes this could be moved. I wasn't sure about the risk of colliding with 
something else.


> Also, it might be worth checking the message IDs to ensure that the
> PATCH M/Ns all indeed are replies to PATCH 0/N.

Doesn't that only work if mails [1..N] are reply to the cover ? Depending on 
the mailer and your option, this might not always be the case (I think).

>
>> - Support multiple patch series in a single run
> Is this done? I would have expected that some buffering of messages
> would be necessary, since you're writing a series of messages of the
> form ... to the commits ... N>.
>

This is for git am. It handles the fact that an input may contain multiple 
series (expect them to be in order).
When reaching patch N/N, it flushed the cover.


>> TODO:
>> - Add doc/comments
>> - Add tests
>> - Add a new "seperator" at the end of a cover letter.
>>   Right now I added a triple dash to all cover letter (manual or 
>> cover-at-tip) before shortlog/diff stat
>>   This allows manually written cover letters to be handle by git am 
>> --cover-at-tip without including the shortlog/diffstat but
>>   breaks compat with older git am as it is seen has a malformed patch. A new 
>> separator would solve that.
> I think the triple dash works. I tried "git am" with a cover letter with
> no triple dash, and it complains that the commit is empty anyway, so
> compatibility with older git am might not be such a big issue. (With the
> triple dash, it indeed complains about a malformed patch, as you
> describe.)

It kinda work but it makes the message difficult to understand for the user.
And change the behaviour from the previous releases.

Thanks for the feedback.

Nicolas

[RFC 3/3] log: add an option to generate cover letter from a branch tip

[Nicolas Morey-Chaisemartin]

TODO: figure out defaults, add a config option, move tip detection to specific 
function

Signed-off-by: Nicolas Morey-Chaisemartin <nico...@morey-chaisemartin.com>
---
 Documentation/git-format-patch.txt |  4 
 builtin/log.c  | 44 +-
 2 files changed, 38 insertions(+), 10 deletions(-)

diff --git a/Documentation/git-format-patch.txt 
b/Documentation/git-format-patch.txt
index 6cbe462a7..0ac9d4b71 100644
--- a/Documentation/git-format-patch.txt
+++ b/Documentation/git-format-patch.txt
@@ -228,6 +228,10 @@ feeding the result to `git send-email`.
containing the branch description, shortlog and the overall diffstat.  
You can
fill in a description in the file before sending it out.
 
+--[no-]cover-letter-at-tip::
+   Use the tip of the series as a cover letter if it is an empty commit.
+If no cover-letter is to be sent, the tip is ignored.
+
 --notes[=]::
Append the notes (see linkgit:git-notes[1]) for the commit
after the three-dash line.
diff --git a/builtin/log.c b/builtin/log.c
index 6c1fa896a..292626482 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -986,11 +986,11 @@ static void make_cover_letter(struct rev_info *rev, int 
use_stdout,
  struct commit *origin,
  int nr, struct commit **list,
  const char *branch_name,
- int quiet)
+ int quiet,
+ struct commit *cover_at_tip_commit)
 {
const char *committer;
-   const char *body = "*** SUBJECT HERE ***\n\n*** BLURB HERE ***\n";
-   const char *msg;
+   const char *body = "*** SUBJECT HERE ***\n\n*** BLURB HERE ***\n\n";
struct shortlog log;
struct strbuf sb = STRBUF_INIT;
int i;
@@ -1021,17 +1021,21 @@ static void make_cover_letter(struct rev_info *rev, int 
use_stdout,
if (!branch_name)
branch_name = find_branch_name(rev);
 
-   msg = body;
pp.fmt = CMIT_FMT_EMAIL;
pp.date_mode.type = DATE_RFC2822;
pp.rev = rev;
pp.print_email_subject = 1;
-   pp_user_info(, NULL, , committer, encoding);
-   pp_title_line(, , , encoding, need_8bit_cte);
-   pp_remainder(, , , 0);
-   add_branch_description(, branch_name);
-   fprintf(rev->diffopt.file, "%s\n", sb.buf);
 
+   if (!cover_at_tip_commit) {
+   pp_user_info(, NULL, , committer, encoding);
+   pp_title_line(, , , encoding, need_8bit_cte);
+   pp_remainder(, , , 0);
+   } else {
+   pretty_print_commit(, cover_at_tip_commit, );
+   }
+   add_branch_description(, branch_name);
+   fprintf(rev->diffopt.file, "%s", sb.buf);
+   fprintf(rev->diffopt.file, "---\n", sb.buf);
strbuf_release();
 
shortlog_init();
@@ -1409,6 +1413,8 @@ int cmd_format_patch(int argc, const char **argv, const 
char *prefix)
int just_numbers = 0;
int ignore_if_in_upstream = 0;
int cover_letter = -1;
+   int cover_at_tip = -1;
+   struct commit *cover_at_tip_commit = NULL;
int boundary_count = 0;
int no_binary_diff = 0;
int zero_commit = 0;
@@ -1437,6 +1443,8 @@ int cmd_format_patch(int argc, const char **argv, const 
char *prefix)
N_("print patches to standard out")),
OPT_BOOL(0, "cover-letter", _letter,
N_("generate a cover letter")),
+   OPT_BOOL(0, "cover-at-tip", _at_tip,
+   N_("fill the cover letter with the tip of the 
branch")),
OPT_BOOL(0, "numbered-files", _numbers,
N_("use simple number sequence for output file 
names")),
OPT_STRING(0, "suffix", _patch_suffix, N_("sfx"),
@@ -1698,6 +1706,21 @@ int cmd_format_patch(int argc, const char **argv, const 
char *prefix)
if (ignore_if_in_upstream && has_commit_patch_id(commit, ))
continue;
 
+   if (!nr && cover_at_tip == 1 && !cover_at_tip_commit) {
+   /* Check that it is a candidate to be a cover at tip
+* Meaning:
+* - a single parent (merge commits are not eligible)
+* - tree oid == parent->tree->oid (no diff to the tree)
+*/
+   if (commit->parents && !commit->parents->next &&
+   !oidcmp(>tree->object.oid,
+   >parents->item->tree->object.oid)) {
+   cover_at_tip_commit = commit;
+

[RFC 1/3] mailinfo: extract patch series id

[Nicolas Morey-Chaisemartin]

Extract the patch ID and series length from the [PATCH N/M]
 prefix in the mail header

Signed-off-by: Nicolas Morey-Chaisemartin <nico...@morey-chaisemartin.com>
---
 mailinfo.c | 35 +++
 mailinfo.h |  2 ++
 2 files changed, 37 insertions(+)

diff --git a/mailinfo.c b/mailinfo.c
index a89db22ab..2ab9d446d 100644
--- a/mailinfo.c
+++ b/mailinfo.c
@@ -308,6 +308,39 @@ static void cleanup_subject(struct mailinfo *mi, struct 
strbuf *subject)
if (!pos)
break;
remove = pos - subject->buf + at + 1;
+   if (7 <= remove &&
+   memmem(subject->buf + at, remove, "PATCH", 5)){
+   /*
+* Look for a N/M series identifier at
+* the end of the brackets
+*/
+   int is_series = 1;
+   int ret, num, total;
+   int pt = at + remove - 2;
+
+   if (!isdigit(subject->buf[pt]))
+   is_series = 0;
+   else {
+   while(isdigit(subject->buf[--pt]));
+   }
+
+   if(is_series && subject->buf[pt--] != '/')
+   is_series = 0;
+
+   if (!isdigit(subject->buf[pt]))
+   is_series = 0;
+   if (is_series)
+   while(isdigit(subject->buf[--pt]));
+
+   pt++;
+
+   ret = sscanf(subject->buf + pt, "%d/%d]", , 
);
+   if (ret == 2){
+   mi->series_id = num;
+   mi->series_len = total;
+   }
+   }
+
if (!mi->keep_non_patch_brackets_in_subject ||
(7 <= remove &&
 memmem(subject->buf + at, remove, "PATCH", 5)))
@@ -1154,6 +1187,8 @@ void setup_mailinfo(struct mailinfo *mi)
mi->header_stage = 1;
mi->use_inbody_headers = 1;
mi->content_top = mi->content;
+   mi->series_id = -1;
+   mi->series_len = -1;
git_config(git_mailinfo_config, mi);
 }
 
diff --git a/mailinfo.h b/mailinfo.h
index 04a25351d..bd4f7c9e0 100644
--- a/mailinfo.h
+++ b/mailinfo.h
@@ -21,6 +21,8 @@ struct mailinfo {
struct strbuf **content_top;
struct strbuf charset;
char *message_id;
+   int series_id;/* Id of the patch within a patch series. -1 if not a 
patch series */
+   int series_len;   /* Length of the patch series. -1 if not a patch 
series */
enum  {
TE_DONTCARE, TE_QP, TE_BASE64
} transfer_encoding;
-- 
2.15.0.169.g3d3eebb67.dirty

[RFC 2/3] am: semi working --cover-at-tip

[Nicolas Morey-Chaisemartin]

Issue with empty patch detection

Signed-off-by: Nicolas Morey-Chaisemartin <nico...@morey-chaisemartin.com>
---
 builtin/am.c | 143 ---
 1 file changed, 126 insertions(+), 17 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index 92c485350..702cbf8e0 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -111,6 +111,11 @@ struct am_state {
char *msg;
size_t msg_len;
 
+   /* Series metadata */
+   int series_id;
+   int series_len;
+   int cover_id;
+
/* when --rebasing, records the original commit the patch came from */
struct object_id orig_commit;
 
@@ -131,6 +136,8 @@ struct am_state {
int committer_date_is_author_date;
int ignore_date;
int allow_rerere_autoupdate;
+   int cover_at_tip;
+   int applying_cover;
const char *sign_commit;
int rebasing;
 };
@@ -160,6 +167,7 @@ static void am_state_init(struct am_state *state)
 
if (!git_config_get_bool("commit.gpgsign", ))
state->sign_commit = gpgsign ? "" : NULL;
+
 }
 
 /**
@@ -432,6 +440,20 @@ static void am_load(struct am_state *state)
read_state_file(, state, "utf8", 1);
state->utf8 = !strcmp(sb.buf, "t");
 
+   read_state_file(, state, "cover-at-tip", 1);
+   state->cover_at_tip = !strcmp(sb.buf, "t");
+
+   if (state->cover_at_tip) {
+   read_state_file(, state, "series_id", 1);
+   state->series_id = strtol(sb.buf, NULL, 10);
+
+   read_state_file(, state, "series_len", 1);
+   state->series_len = strtol(sb.buf, NULL, 10);
+
+   read_state_file(, state, "cover_id", 1);
+   state->cover_id = strtol(sb.buf, NULL, 10);
+   }
+
if (file_exists(am_path(state, "rerere-autoupdate"))) {
read_state_file(, state, "rerere-autoupdate", 1);
state->allow_rerere_autoupdate = strcmp(sb.buf, "t") ?
@@ -1020,6 +1042,7 @@ static void am_setup(struct am_state *state, enum 
patch_format patch_format,
write_state_bool(state, "quiet", state->quiet);
write_state_bool(state, "sign", state->signoff);
write_state_bool(state, "utf8", state->utf8);
+   write_state_bool(state, "cover-at-tip", state->cover_at_tip);
 
if (state->allow_rerere_autoupdate)
write_state_bool(state, "rerere-autoupdate",
@@ -1076,6 +1099,12 @@ static void am_setup(struct am_state *state, enum 
patch_format patch_format,
delete_ref(NULL, "ORIG_HEAD", NULL, 0);
}
 
+   if (state->cover_at_tip) {
+   write_state_count(state, "series_id", state->series_id);
+   write_state_count(state, "series_len", state->series_len);
+   write_state_count(state, "cover_id", state->cover_id);
+   }
+
/*
 * NOTE: Since the "next" and "last" files determine if an am_state
 * session is in progress, they should be written last.
@@ -1088,13 +1117,9 @@ static void am_setup(struct am_state *state, enum 
patch_format patch_format,
 }
 
 /**
- * Increments the patch pointer, and cleans am_state for the application of the
- * next patch.
- */
-static void am_next(struct am_state *state)
+ * Cleans am_state.
+ */static void am_clean(struct am_state *state)
 {
-   struct object_id head;
-
FREE_AND_NULL(state->author_name);
FREE_AND_NULL(state->author_email);
FREE_AND_NULL(state->author_date);
@@ -1106,14 +1131,6 @@ static void am_next(struct am_state *state)
 
oidclr(>orig_commit);
unlink(am_path(state, "original-commit"));
-
-   if (!get_oid("HEAD", ))
-   write_state_text(state, "abort-safety", oid_to_hex());
-   else
-   write_state_text(state, "abort-safety", "");
-
-   state->cur++;
-   write_state_count(state, "next", state->cur);
 }
 
 /**
@@ -1274,6 +1291,7 @@ static int parse_mail(struct am_state *state, const char 
*mail)
fclose(mi.input);
fclose(mi.output);
 
+
/* Extract message and author information */
fp = xfopen(am_path(state, "info"), "r");
while (!strbuf_getline_lf(, fp)) {
@@ -1298,9 +1316,30 @@ static int parse_mail(struct am_state *state, const char 
*mail)
goto finish;
}
 
-   if (is_empty_file(am_path(state, "patch"))) {
-   printf_ln(_("Patch is empty."));
-   die_user_resolve(state);
+   if (!state->applying_cover) {
+
+   state->series_id = 

[RFC 0/3] Add support for --cover-at-tip

[Nicolas Morey-Chaisemartin]

v2:
- Enhance mailinfo to parse patch series id from subject
- Detect cover using mailinfo parsed ids in git am
- Support multiple patch series in a single run

TODO:
- Add doc/comments
- Add tests
- Add a new "seperator" at the end of a cover letter.
  Right now I added a triple dash to all cover letter (manual or cover-at-tip) 
before shortlog/diff stat
  This allows manually written cover letters to be handle by git am 
--cover-at-tip without including the shortlog/diffstat but
  breaks compat with older git am as it is seen has a malformed patch. A new 
separator would solve that.

Note: Cover letter automatically generated with --cover-at-tip ;)

Signed-off-by: Nicolas Morey-Chaisemartin <nico...@morey-chaisemartin.com>
---
Nicolas Morey-Chaisemartin (3):
  mailinfo: extract patch series id
  am: semi working --cover-at-tip
  log: add an option to generate cover letter from a branch tip

 Documentation/git-format-patch.txt |   4 ++
 builtin/am.c   | 143 -
 builtin/log.c  |  44 +---
 mailinfo.c |  35 +
 mailinfo.h |   2 +
 5 files changed, 201 insertions(+), 27 deletions(-)

-- 
2.15.0.169.g3d3eebb67.dirty

Re: [RFC] cover-at-tip

[Nicolas Morey-Chaisemartin]

Le 13/11/2017 à 11:30, Junio C Hamano a écrit :
> Junio C Hamano <gits...@pobox.com> writes:
>
>> Nicolas Morey-Chaisemartin <nmoreychaisemar...@suse.de> writes:
>>
>>> I agree this is a "am" job. Was just wondering if reusing some of
>>> the code from apply (and move it so it makes more sense) wouldnd't
>>> make more sense than rewriting a patch detection function.
>> Yes, I understood that and have already given an answer, no?
> This was a bit too terse to be useful, so let me try again.

Thanks ;)

>
> I think the ideal endgame would be to allow people to come up with a
> topic branch of this shape (illustrated is a three-patch series on
> top of 'origin'):
>
> ---o---o (origin)
> \
>  1---2---3
>
> and then add an empty commit C whose log message is used to store
> "cover letter material", i.e.
>
> ---o---o (origin)
> \
>  1---2---3---C (topic)
>
> And then you should be able to 
>
>  (1) merge such branch yourself, coming up with a history like this,
>  where merge M uses material from C in the merge log message
>
> ---o---o---x---x---M
> \ /
>  1---2---3
>
>  (2) "git format-patch origin..topic" that would create the cover
>  letter using material found in C in addition to the usual
>  stuff (like shortlog) generated by "format-patch --cover",
>  followed by these three patches.
>
>  (3) "git format-patch M" should be able to (a) realize that M
>  merges a side branch that is a three-commit series (i.e.
>  M^1..M^2), and (b) notice that log message of M has
>  human-readable description.  Then it grabs the merge log
>  message of M and do the same as (2).
>
>  (4) "git am" the result from (2) or (3) should recreate the
>  original history i.e. what we started with with C.
>
> ---o---o (origin)
> \
>  1---2---3---C (topic)

That what I got from the archive referenced in the leftover bits.
I'm currently focusing on (2) and (4).
(3) might come reasonably "easy" after (2) but I don't know enough about the 
internal API yet so I focused on the simplest ;)

>
> Now, I _think_ what the machinery needs a lot more is to be able to
> detect C is an empty commit (when doing (2)),

Unless I'm mistaken, this should be covered by the RFC for format-patch:

+   if (commit->parents && !commit->parents->next &&
+   !oidcmp(>tree->object.oid,
+   >parents->item->tree->object.oid)) {
+   cover_at_tip_commit = commit;

As I said, I'm focusing only for (2) now, so we check there is only one parent 
and that the commit did not change the tree hash. (meaning an empty commit 
right ?)

>  and then you have
> quite a lattitude in designing what exactly such an automated cover
> letter looks like, so that the receiving end (4) can recognize it
> more easily and (more importantly) more robustly than "the message
> does not have any patch in it".  Not all random messages that do
> not have a patch in it are cover letters, and that is why I do not
> think touching any code in the apply layer in an attempt to "reuse"
> anything is a bad idea.  It will risk butchering the code without
> any real gain, because what we really need to know is *not* absence
> of patch, but presence of cover letter material.

Agreed.

> The simplest would probably be to notice that the subject of one has
> 0/N on it, while other messages were labeled with 1/N..(N-1)/N; that
> would be a lot stronger clue that 0/N has a cover than "it does not
> have any patch in it".

Good idea, and should be easy enough to put in place.

>
> It may be that we would not just want to identify which message is
> cover and which message is not, but which part of the cover letter
> message should go back to the log message of the capping empty
> commit (and moved to the merge log message).  Just like we invented
> the conventions like scissors, three-dashes, etc., you might want to
> come up with a way to do so in your format-patch enhancement used to
> do the (2) and (3) above.  Then it will be the matter of teaching
> that convention to "am" used in (4).
>

I like that too. It could also allow using (4) on series with a manual cover 
letter without pulling the shortlog/diffstat stuff into the new topic commit.

Nicolas

Re: [RFC] cover-at-tip

[Nicolas Morey-Chaisemartin]

Le 10/11/2017 à 19:22, Junio C Hamano a écrit :
> Nicolas Morey-Chaisemartin <nmoreychaisemar...@suse.de> writes:
>
>> I would need to add "some" level of parsing to am.c to make sure
>> the patch content is just garbage and that there are no actual
>> hunks for that.
>>
>> I did not find any public API that would allow me to do that,
>> although apply_path/parse_chunk would fit the bill.  Is that the
>> right way to approach this ?
> I do not think you would want this non-patch cruft seen at the apply
> layer at all.  Reading a mailbox, with the help of mailsplit and
> mailinfo, and being the driver to create a series of commits is what
> "am" is about, and it would have to notice that the non-patch cruft
> at the beginning is not a patch at all and defer creation of an
> empty commit with that cover material at the end.  For each of the
> other messages in the series that has patches, it will need to call
> apply to update the index and the working tree so that it can make a
> commit, but there is NO reason whatsoever to ask help from apply, whose
> sole purpose is to read a patch and make modifications to the index
> and the working tree, to handle the cover material.
>
>

I agree this is a "am" job. Was just wondering if reusing some of the code from 
apply (and move it so it makes more sense) wouldnd't make more sense than 
rewriting a patch detection function.

Nicolas

Re: [RFC] cover-at-tip

[Nicolas Morey-Chaisemartin]

Le 10/11/2017 à 11:24, Nicolas Morey-Chaisemartin a écrit :
> Hi,
>
> I'm starting to look into the cover-at-tip topic that I found in the leftover 
> bits (http://www.spinics.net/lists/git/msg259573.html)
>
> Here's a first draft of a patch that adds support for format-patch 
> --cover-at-tip. It compiles and works in my nice and user firnedly test case.
> Just wanted to make sure I was going roughly in the right direction here.
>
>
> I was wondering where is the right place to put a commit_is_cover_at_tip() as 
> the test will be needed in other place as the feature is extended to git 
> am/merge/pull.
>
> Feel free to comment. I know the help is not clear at this point and there's 
> still some work to do on option handling (add a config option, probably have 
> --cover-at-tip imply --cover-letter, etc) and
> some testing :)
>
>
> ---

Leaving some more updates and questions before the week end:

I started on git am --cover-at-tip.

The proposed patch for format-patch does not output any "---" to signal the end 
of the commit log and the begining of the patch in the cover letter.
This means that the log summary, the diffstat and the git footer ( --\n) is seen as part of the commit log. Which is just wrong.

Removing them would solve the issue but I feel they bring some useful info (or 
they would not be here).
Adding a "---" between the commit log and those added infos poses another 
problem: git am does not see an empty patch anymore.
I would need to add "some" level of parsing to am.c to make sure the patch 
content is just garbage and that there are no actual hunks for that.

I did not find any public API that would allow me to do that, although 
apply_path/parse_chunk would fit the bill.
Is that the right way to approach this ?

My branch is here if anyone want to give a look: 
https://github.com/nmorey/git/tree/dev/cover-at-tip

Nicolas

[RFC] cover-at-tip

[Nicolas Morey-Chaisemartin]

Hi,

I'm starting to look into the cover-at-tip topic that I found in the leftover 
bits (http://www.spinics.net/lists/git/msg259573.html)

Here's a first draft of a patch that adds support for format-patch 
--cover-at-tip. It compiles and works in my nice and user firnedly test case.
Just wanted to make sure I was going roughly in the right direction here.


I was wondering where is the right place to put a commit_is_cover_at_tip() as 
the test will be needed in other place as the feature is extended to git 
am/merge/pull.

Feel free to comment. I know the help is not clear at this point and there's 
still some work to do on option handling (add a config option, probably have 
--cover-at-tip imply --cover-letter, etc) and
some testing :)


---
 Documentation/git-format-patch.txt |  4 
 builtin/log.c  | 38 +++---
 2 files changed, 35 insertions(+), 7 deletions(-)

diff --git a/Documentation/git-format-patch.txt 
b/Documentation/git-format-patch.txt
index 6cbe462a7..0ac9d4b71 100644
--- a/Documentation/git-format-patch.txt
+++ b/Documentation/git-format-patch.txt
@@ -228,6 +228,10 @@ feeding the result to `git send-email`.
containing the branch description, shortlog and the overall diffstat.  
You can
fill in a description in the file before sending it out.
 
+--[no-]cover-letter-at-tip::
+   Use the tip of the series as a cover letter if it is an empty commit.
+If no cover-letter is to be sent, the tip is ignored.
+
 --notes[=]::
Append the notes (see linkgit:git-notes[1]) for the commit
after the three-dash line.
diff --git a/builtin/log.c b/builtin/log.c
index 6c1fa896a..a0e9e61a3 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -986,11 +986,11 @@ static void make_cover_letter(struct rev_info *rev, int 
use_stdout,
  struct commit *origin,
  int nr, struct commit **list,
  const char *branch_name,
- int quiet)
+ int quiet,
+ struct commit *cover_at_tip_commit)
 {
const char *committer;
const char *body = "*** SUBJECT HERE ***\n\n*** BLURB HERE ***\n";
-   const char *msg;
struct shortlog log;
struct strbuf sb = STRBUF_INIT;
int i;
@@ -1021,14 +1021,18 @@ static void make_cover_letter(struct rev_info *rev, int 
use_stdout,
if (!branch_name)
branch_name = find_branch_name(rev);
 
-   msg = body;
pp.fmt = CMIT_FMT_EMAIL;
pp.date_mode.type = DATE_RFC2822;
pp.rev = rev;
pp.print_email_subject = 1;
-   pp_user_info(, NULL, , committer, encoding);
-   pp_title_line(, , , encoding, need_8bit_cte);
-   pp_remainder(, , , 0);
+
+   if (!cover_at_tip_commit) {
+   pp_user_info(, NULL, , committer, encoding);
+   pp_title_line(, , , encoding, need_8bit_cte);
+   pp_remainder(, , , 0);
+   } else {
+   pretty_print_commit(, cover_at_tip_commit, );
+   }
add_branch_description(, branch_name);
fprintf(rev->diffopt.file, "%s\n", sb.buf);
 
@@ -1409,6 +1413,8 @@ int cmd_format_patch(int argc, const char **argv, const 
char *prefix)
int just_numbers = 0;
int ignore_if_in_upstream = 0;
int cover_letter = -1;
+   int cover_at_tip = -1;
+   struct commit *cover_at_tip_commit = NULL;
int boundary_count = 0;
int no_binary_diff = 0;
int zero_commit = 0;
@@ -1437,6 +1443,8 @@ int cmd_format_patch(int argc, const char **argv, const 
char *prefix)
N_("print patches to standard out")),
OPT_BOOL(0, "cover-letter", _letter,
N_("generate a cover letter")),
+   OPT_BOOL(0, "cover-at-tip", _at_tip,
+   N_("fill the cover letter with the tip of the 
branch")),
OPT_BOOL(0, "numbered-files", _numbers,
N_("use simple number sequence for output file 
names")),
OPT_STRING(0, "suffix", _patch_suffix, N_("sfx"),
@@ -1698,6 +1706,21 @@ int cmd_format_patch(int argc, const char **argv, const 
char *prefix)
if (ignore_if_in_upstream && has_commit_patch_id(commit, ))
continue;
 
+   if (!nr && cover_at_tip == 1 && !cover_at_tip_commit) {
+   /* Check that it is a candidate to be a cover at tip
+* Meaning:
+* - a single parent (merge commits are not eligible)
+* - tree oid == parent->tree->oid (no diff to the tree)
+*/
+   if (commit->parents && !commit->parents->next &&
+   !oidcmp(>tree->object.oid,
+   >parents->item->tree->object.oid)) {

Re: What's cooking in git.git (Aug 2017, #05; Tue, 22)

[Nicolas Morey-Chaisemartin]

Le 19/09/2017 à 17:43, Johannes Schindelin a écrit :
>
> C'mon, don't *try* to misunderstand me.
>
> Of course there need to be updates as to the state of patch series.
>
> It's just that mails only go *so* far when you need to connect and
> aggregate information. You need the connection between the original patch
> series, the latest unaddressed reviews, links to the branches, history of
> the patch series' iterations, and ideally links to the repositories of the
> contributors with *their* branch names. And then, of course, your verdict
> as to the state of the patch series and your expectation what happens
> next.
>
> To relate that, you are using a plain text format that is not well defined
> and not structured, and certainly not machine-readable, for information
> that is crucial for project management.
>
> What you need is a tool to aggregate this information, to help working
> with it, to manage the project, and to be updated automatically. And to
> publish this information continuously, without costing you extra effort.
>
> I understand that you started before GitHub existed, and before GitHub was
> an option, the script-generated What's cooking mail was the best you could
> do.
>
> Ciao,
> Dscho
Hi,

Would something like patchwork fix your need ?
They now seems to have a REST API, which means it could probably be pluggeg into
Junio's scripts and work seemlessly for him (or any other happy ML user) while 
other people can browse
through the web interface.

I used to work with this one:
http://patches.opendataplane.org/project/lng-odp/list/
It is not the best  example as the patch status are pretty much never updated 
on this one.

But it would solve most of the points you raised, while keeping fully 
compatible with the way people actually work (including Junio).

Nicolas

Re: Rebase & submodules

[Nicolas Morey-Chaisemartin]

Le 14/09/2017 à 17:39, Robert Dailey a écrit :
> So I often will have a submodule that points to one of my own forks,
> because I will have work done on a feature branch that hasn't been
> merged upstream yet. Assuming this merge takes a long time to get
> approved, I will occasionally rebase my topic branch to keep things up
> to date and clean.
>
> However, any previous commits in my parent repository that refer to a
> SHA1 prior to the rebase will now be pointing to dangling/orphaned
> commits, which means I wouldn't be able to go back to that commit in
> history and do `git submodule update` since that checkout will fail.
>
> One obvious solution to this is: Don't rebase. But, this could result
> in a lot of merging between the upstream and my fork which would make
> things not only ugly, but prevent me from making a pull request that
> makes sense to the upstream repository (They'd likely request a rebase
> in order to accept the PR, which I wouldn't be able to do for the
> reasons outlined above).
>
> Are there any other workflows that would support this kind of model better?

Without changing your workflow too much, simply add an annotated tag to your 
branch before your rebase.
This way the SHA1 will always exists. Unless you want to cleanup at some point 
(branch merged ?) and then you can simply delete all those old tags.

Nicolas

[PATCH v4 0/4] imap-send: Fix and enable curl by default

[Nicolas Morey-Chaisemartin]

Changes since v3:
- Fix return code in patch #1
- Reword patch#4

Nicolas Morey-Chaisemartin (4):
  imap-send: return with error if curl failed
  imap-send: add wrapper to get server credentials if needed
  imap_send: setup_curl: retreive credentials if not set in config file
  imap-send: use curl by default when possible

 imap-send.c | 60 
 1 file changed, 40 insertions(+), 20 deletions(-)

-- 
2.14.1.461.g503560879

[PATCH v4 1/4] imap-send: return with error if curl failed

[Nicolas Morey-Chaisemartin]

curl_append_msgs_to_imap always returned 0, whether curl failed or not.
Return a proper status so git imap-send will exit with an error code
if something wrong happened.

Signed-off-by: Nicolas Morey-Chaisemartin <nico...@morey-chaisemartin.com>
---
 imap-send.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/imap-send.c b/imap-send.c
index b2d0b849bb..b5e332420a 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -1490,7 +1490,7 @@ static int curl_append_msgs_to_imap(struct 
imap_server_conf *server,
curl_easy_cleanup(curl);
curl_global_cleanup();
 
-   return 0;
+   return res != CURLE_OK;
 }
 #endif
 
-- 
2.14.1.461.g503560879

[PATCH v4 3/4] imap_send: setup_curl: retreive credentials if not set in config file

[Nicolas Morey-Chaisemartin]

Up to this point, the curl mode only supported getting the username
and password from the gitconfig file while the legacy mode could also
fetch them using the credential API.

Signed-off-by: Nicolas Morey-Chaisemartin <nico...@morey-chaisemartin.com>
---
 imap-send.c | 18 --
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/imap-send.c b/imap-send.c
index 1b8fbbd545..7e39993d95 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -1398,7 +1398,7 @@ static int append_msgs_to_imap(struct imap_server_conf 
*server,
 }
 
 #ifdef USE_CURL_FOR_IMAP_SEND
-static CURL *setup_curl(struct imap_server_conf *srvc)
+static CURL *setup_curl(struct imap_server_conf *srvc, struct credential *cred)
 {
CURL *curl;
struct strbuf path = STRBUF_INIT;
@@ -1411,6 +1411,7 @@ static CURL *setup_curl(struct imap_server_conf *srvc)
if (!curl)
die("curl_easy_init failed");
 
+   server_fill_credential(, cred);
curl_easy_setopt(curl, CURLOPT_USERNAME, server.user);
curl_easy_setopt(curl, CURLOPT_PASSWORD, server.pass);
 
@@ -1460,8 +1461,9 @@ static int curl_append_msgs_to_imap(struct 
imap_server_conf *server,
struct buffer msgbuf = { STRBUF_INIT, 0 };
CURL *curl;
CURLcode res = CURLE_OK;
+   struct credential cred = CREDENTIAL_INIT;
 
-   curl = setup_curl(server);
+   curl = setup_curl(server, );
curl_easy_setopt(curl, CURLOPT_READDATA, );
 
fprintf(stderr, "sending %d message%s\n", total, (total != 1) ? "s" : 
"");
@@ -1496,6 +1498,18 @@ static int curl_append_msgs_to_imap(struct 
imap_server_conf *server,
curl_easy_cleanup(curl);
curl_global_cleanup();
 
+   if (cred.username)
+   if (res == CURLE_OK)
+   credential_approve();
+#if LIBCURL_VERSION_NUM >= 0x070d01
+   else if (res == CURLE_LOGIN_DENIED)
+#else
+   else
+#endif
+   credential_reject();
+
+   credential_clear();
+
return res != CURLE_OK;
 }
 #endif
-- 
2.14.1.461.g503560879

[PATCH v4 4/4] imap-send: use curl by default when possible

[Nicolas Morey-Chaisemartin]

Set curl as the runtime default when it is available.
When linked against older curl versions (< 7_34_0) or without curl,
use the legacy imap implementation.

The goal is to validate feature parity between the legacy and
the curl implementation, deprecate the legacy implementation
later on and in the long term, hopefully drop it altogether.

Signed-off-by: Nicolas Morey-Chaisemartin <nico...@morey-chaisemartin.com>
---
 imap-send.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/imap-send.c b/imap-send.c
index 7e39993d95..af1e1576bd 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -35,11 +35,11 @@ typedef void *SSL;
 #include "http.h"
 #endif
 
-#if defined(USE_CURL_FOR_IMAP_SEND) && defined(NO_OPENSSL)
-/* only available option */
+#if defined(USE_CURL_FOR_IMAP_SEND)
+/* Always default to curl if it's available. */
 #define USE_CURL_DEFAULT 1
 #else
-/* strictly opt in */
+/* We don't have curl, so continue to use the historical implementation */
 #define USE_CURL_DEFAULT 0
 #endif
 
-- 
2.14.1.461.g503560879

[PATCH v4 2/4] imap-send: add wrapper to get server credentials if needed

[Nicolas Morey-Chaisemartin]

Signed-off-by: Nicolas Morey-Chaisemartin <nico...@morey-chaisemartin.com>
---
 imap-send.c | 34 --
 1 file changed, 20 insertions(+), 14 deletions(-)

diff --git a/imap-send.c b/imap-send.c
index b5e332420a..1b8fbbd545 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -926,6 +926,25 @@ static int auth_cram_md5(struct imap_store *ctx, struct 
imap_cmd *cmd, const cha
return 0;
 }
 
+static void server_fill_credential(struct imap_server_conf *srvc, struct 
credential *cred)
+{
+   if (srvc->user && srvc->pass)
+   return;
+
+   cred->protocol = xstrdup(srvc->use_ssl ? "imaps" : "imap");
+   cred->host = xstrdup(srvc->host);
+
+   cred->username = xstrdup_or_null(srvc->user);
+   cred->password = xstrdup_or_null(srvc->pass);
+
+   credential_fill(cred);
+
+   if (!srvc->user)
+   srvc->user = xstrdup(cred->username);
+   if (!srvc->pass)
+   srvc->pass = xstrdup(cred->password);
+}
+
 static struct imap_store *imap_open_store(struct imap_server_conf *srvc, char 
*folder)
 {
struct credential cred = CREDENTIAL_INIT;
@@ -1078,20 +1097,7 @@ static struct imap_store *imap_open_store(struct 
imap_server_conf *srvc, char *f
}
 #endif
imap_info("Logging in...\n");
-   if (!srvc->user || !srvc->pass) {
-   cred.protocol = xstrdup(srvc->use_ssl ? "imaps" : 
"imap");
-   cred.host = xstrdup(srvc->host);
-
-   cred.username = xstrdup_or_null(srvc->user);
-   cred.password = xstrdup_or_null(srvc->pass);
-
-   credential_fill();
-
-   if (!srvc->user)
-   srvc->user = xstrdup(cred.username);
-   if (!srvc->pass)
-   srvc->pass = xstrdup(cred.password);
-   }
+   server_fill_credential(srvc, );
 
if (srvc->auth_method) {
struct imap_cmd_cb cb;
-- 
2.14.1.461.g503560879

Re: [PATCH 4/4] imap-send: use curl by default

[Nicolas Morey-Chaisemartin]

Le 12/09/2017 à 09:02, Junio C Hamano a écrit :
> Junio C Hamano <gits...@pobox.com> writes:
>
>> I was sweeping my mailbox to collect loose ends that haven't been
>> tied down, and noticed that this topic does not seem to have reached
>> a conclusion.  Do we want to reboot the effort?  Or should we just
>> throw it in the #leftoverbits bin for now?
> Ahh, I failed to find newer incarnations of this topic (there was v3 that
> starts at <087f5907-6558-ce32-2f5c-2e418522c...@morey-chaisemartin.com>)
> when I wrote it.  Sorry about pinging a wrong/old article.
>
> I just gave a cursory re-read of the review thread over the v3
> series; it appears that we were very close to the finishing line
> already?
>
> Or am I yet missing/forgetting some later developments that made
> this topic obsolete?
>
> Thanks.
>
v3 needs just a few bits fixed:
- Fix the bads return code in patch #1.
- Reword patch #4 was you found confusing. I sent a proposal fix that you 
probably missed:

Is something like this clearer ?
Author: Nicolas Morey-Chaisemartin <nico...@morey-chaisemartin.com>
Date:   Sun Aug 6 21:30:15 2017 +0200

    imap-send: use curl by default when possible
   
    Set curl as the runtime default when it is available.
    When linked against older curl versions (< 7_34_0) or without curl,
    use the legacy imap implementation.
   
    The goal is to validate feature parity between the legacy and
    the curl implementation, deprecate the legacy implementation
    later on and in the long term, hopefully drop it altogether.
   
    Signed-off-by: Nicolas Morey-Chaisemartin <nico...@morey-chaisemartin.com>


If you're happy with this log message, I've v4 ready to post.

Nicolas

[PATCHv3 2/2] pull: honor submodule.recurse config option

[Nicolas Morey-Chaisemartin]

"git pull" supports a --recurse-submodules option but does not parse the
submodule.recurse configuration item to set the default for that option.
Meanwhile "git fetch" does support submodule.recurse, producing
confusing behavior: when submodule.recurse is enabled, "git pull"
recursively fetches submodules but does not update them after fetch.

Handle submodule.recurse in "git pull" to fix this.

Reported-by: Magnus Homann <mag...@homann.se>
Signed-off-by: Nicolas Morey-Chaisemartin <nico...@morey-chaisemartin.com>
---
 builtin/pull.c|  4 
 t/t5572-pull-submodule.sh | 32 
 2 files changed, 36 insertions(+)

diff --git a/builtin/pull.c b/builtin/pull.c
index 9ef1ab501..6f772e8a2 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -325,6 +325,10 @@ static int git_pull_config(const char *var, const char 
*value, void *cb)
if (!strcmp(var, "rebase.autostash")) {
config_autostash = git_config_bool(var, value);
return 0;
+   } else if (!strcmp(var, "submodule.recurse")) {
+   recurse_submodules = git_config_bool(var, value) ?
+   RECURSE_SUBMODULES_ON : RECURSE_SUBMODULES_OFF;
+   return 0;
}
return git_default_config(var, value, cb);
 }
diff --git a/t/t5572-pull-submodule.sh b/t/t5572-pull-submodule.sh
index 077eb07e1..321bd37de 100755
--- a/t/t5572-pull-submodule.sh
+++ b/t/t5572-pull-submodule.sh
@@ -65,6 +65,38 @@ test_expect_success 'recursive pull updates working tree' '
test_path_is_file super/sub/merge_strategy.t
 '
 
+test_expect_success "submodule.recurse option triggers recursive pull" '
+   test_commit -C child merge_strategy_2 &&
+   git -C parent submodule update --remote &&
+   git -C parent add sub &&
+   git -C parent commit -m "update submodule" &&
+
+   git -C super -c submodule.recurse pull --no-rebase &&
+   test_path_is_file super/sub/merge_strategy_2.t
+'
+
+test_expect_success " --[no-]recurse-submodule and submodule.recurse" '
+   test_commit -C child merge_strategy_3 &&
+   git -C parent submodule update --remote &&
+   git -C parent add sub &&
+   git -C parent commit -m "update submodule" &&
+
+   git -C super -c submodule.recurse pull --no-recurse-submodules 
--no-rebase &&
+   test_path_is_missing super/sub/merge_strategy_3.t &&
+   git -C super -c submodule.recurse=false pull --recurse-submodules 
--no-rebase &&
+   test_path_is_file super/sub/merge_strategy_3.t &&
+
+   test_commit -C child merge_strategy_4 &&
+   git -C parent submodule update --remote &&
+   git -C parent add sub &&
+   git -C parent commit -m "update submodule" &&
+
+   git -C super -c submodule.recurse=false pull --no-recurse-submodules 
--no-rebase &&
+   test_path_is_missing super/sub/merge_strategy_4.t &&
+   git -C super -c submodule.recurse=true pull --recurse-submodules 
--no-rebase &&
+   test_path_is_file super/sub/merge_strategy_4.t
+'
+
 test_expect_success 'recursive rebasing pull' '
# change upstream
test_commit -C child rebase_strategy &&
-- 
2.14.1.461.g503560879

[PATCHv3 0/2] fix recurse.submodule config for git pull

[Nicolas Morey-Chaisemartin]

Changes since v2:
- Add a patch that fixes the option parsing order (parse config before cli, not 
the other way around)
- Enhance the tests to check --recurse-submodule and submodule.recurse 
combinations

Nicolas Morey-Chaisemartin (2):
  pull: fix cli and config option parsing order
  pull: honor submodule.recurse config option

 builtin/pull.c|  8 ++--
 t/t5572-pull-submodule.sh | 32 
 2 files changed, 38 insertions(+), 2 deletions(-)

-- 
2.14.1.461.g503560879

Re: [PATCHv2] pull: honor submodule.recurse config option

[Nicolas Morey-Chaisemartin]

Le 06/09/2017 à 03:17, Junio C Hamano a écrit :
> Nicolas Morey-Chaisemartin <nico...@morey-chaisemartin.com> writes:
>
>> "git pull" supports a --recurse-submodules option but does not parse the
>> submodule.recurse configuration item to set the default for that option.
>> Meanwhile "git fetch" does support submodule.recurse, producing
>> confusing behavior: when submodule.recurse is enabled, "git pull"
>> recursively fetches submodules but does not update them after fetch.
>>
>> Handle submodule.recurse in "git pull" to fix this.
>>
>> Reported-by: Magnus Homann <mag...@homann.se>
>> Signed-off-by: Nicolas Morey-Chaisemartin <nico...@morey-chaisemartin.com>
>> ---
>>
>> Changes since v1:
>>  * Cleanup commit message
>>  * Add test
>>  * Remove extra var in code and fallthrough to git_default_config
>>
>>  builtin/pull.c|  4 
>>  t/t5572-pull-submodule.sh | 10 ++
>>  2 files changed, 14 insertions(+)
>>
>> diff --git a/builtin/pull.c b/builtin/pull.c
>> index 7fe281414..ce8ccb15b 100644
>> --- a/builtin/pull.c
>> +++ b/builtin/pull.c
>> @@ -325,6 +325,10 @@ static int git_pull_config(const char *var, const char 
>> *value, void *cb)
>>  if (!strcmp(var, "rebase.autostash")) {
>>  config_autostash = git_config_bool(var, value);
>>  return 0;
>> +} else if (!strcmp(var, "submodule.recurse")) {
>> +recurse_submodules = git_config_bool(var, value) ?
>> +RECURSE_SUBMODULES_ON : RECURSE_SUBMODULES_OFF;
>> +return 0;
>>  }
>>  return git_default_config(var, value, cb);
>>  }
> If I am reading the existing code correctly, things happen in
> cmd_pull() in this order:
>
>  - recurse_submodules is a file-scope static that is initialized to
>RECURSE_SUBMODULES_DEFAULT
>
>  - pull_options[] is given to parse_options() so that
>submodule-config.c::option_fetch_parse_recurse_submodules() can
>read "--recurse-submodules=" from the command line to
>update recurse_submodules.
>
>  - git_pull_config() is given to git_config() so that settings in
>the configuration files are read.
>
> Care must be taken to make sure that values given from the command
> line is never overriden by the default value specified in the
> configuration system because the order of the second and third items
> in the above are backwards from the usual flow.  This patch does not
> seem to have any such provision.
>
> Existing handling of "--autostash" vs "rebase.autostash" solves this
> issue by having opt_autostash and config_autostash as two separate
> variables, so I suspect that something similar to it must be there,
> at least, for this new configuration.
>
> If we want to keep the current code structure, that is.  I do not
> recall if we did not notice the fact that the order of options and
> config parsing is backwards and unknowingly worked it around with
> two variables when we added the rebase.autostash thing, or we knew
> the order was unusual but there was a good reason to keep that
> unusual order (iow, if we simply swapped the order of
> parse_options() and git_config() calls, there are things that will
> break).  
>
> If it is not the latter, perhaps we may want to flip the order of
> config parsing and option parsing around?  That will allow us to fix
> the handling of autostash thing to use only one variable, and also
> fix your patch to do the right thing.

I see what you mean.
It looks like switching the code around works but I think there still needs to 
be 2variables for autstash for this piece of code:

    if (!opt_rebase && opt_autostash != -1)
        die(_("--[no-]autostash option is only valid with --rebase."));

The config option should not cause git pull to die when not using --rebase, the 
CLI option should.

>> diff --git a/t/t5572-pull-submodule.sh b/t/t5572-pull-submodule.sh
>> index 077eb07e1..1b3a3f445 100755
>> --- a/t/t5572-pull-submodule.sh
>> +++ b/t/t5572-pull-submodule.sh
>> @@ -65,6 +65,16 @@ test_expect_success 'recursive pull updates working tree' 
>> '
>>  test_path_is_file super/sub/merge_strategy.t
>>  '
>>  
>> +test_expect_success "submodule.recurse option triggers recursive pull" '
>> +test_commit -C child merge_strategy_2 &&
>> +git -C parent submodule update --remote &&
>> +git -C parent add sub &&
>> +git -C parent commit -m "update submodule" &&
>> +
>> +git -C super -c submodule.recurse pull --no-rebase &&
>> +test_path_is_file super/sub/merge_strategy_2.t
>> +'
> This new test does not test interactions with submodule.recurse
> configuration and --recurse-submodules= from the command
> line.  It would be necessary to add tests to cover the permutations
> in addition to the basic test we see above.

Will fix

Thanks

Nicolas

[PATCHv2] pull: honor submodule.recurse config option

[Nicolas Morey-Chaisemartin]

"git pull" supports a --recurse-submodules option but does not parse the
submodule.recurse configuration item to set the default for that option.
Meanwhile "git fetch" does support submodule.recurse, producing
confusing behavior: when submodule.recurse is enabled, "git pull"
recursively fetches submodules but does not update them after fetch.

Handle submodule.recurse in "git pull" to fix this.

Reported-by: Magnus Homann <mag...@homann.se>
Signed-off-by: Nicolas Morey-Chaisemartin <nico...@morey-chaisemartin.com>
---

Changes since v1:
 * Cleanup commit message
 * Add test
 * Remove extra var in code and fallthrough to git_default_config

 builtin/pull.c|  4 
 t/t5572-pull-submodule.sh | 10 ++
 2 files changed, 14 insertions(+)

diff --git a/builtin/pull.c b/builtin/pull.c
index 7fe281414..ce8ccb15b 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -325,6 +325,10 @@ static int git_pull_config(const char *var, const char 
*value, void *cb)
if (!strcmp(var, "rebase.autostash")) {
config_autostash = git_config_bool(var, value);
return 0;
+   } else if (!strcmp(var, "submodule.recurse")) {
+   recurse_submodules = git_config_bool(var, value) ?
+   RECURSE_SUBMODULES_ON : RECURSE_SUBMODULES_OFF;
+   return 0;
}
return git_default_config(var, value, cb);
 }
diff --git a/t/t5572-pull-submodule.sh b/t/t5572-pull-submodule.sh
index 077eb07e1..1b3a3f445 100755
--- a/t/t5572-pull-submodule.sh
+++ b/t/t5572-pull-submodule.sh
@@ -65,6 +65,16 @@ test_expect_success 'recursive pull updates working tree' '
test_path_is_file super/sub/merge_strategy.t
 '
 
+test_expect_success "submodule.recurse option triggers recursive pull" '
+   test_commit -C child merge_strategy_2 &&
+   git -C parent submodule update --remote &&
+   git -C parent add sub &&
+   git -C parent commit -m "update submodule" &&
+
+   git -C super -c submodule.recurse pull --no-rebase &&
+   test_path_is_file super/sub/merge_strategy_2.t
+'
+
 test_expect_success 'recursive rebasing pull' '
# change upstream
test_commit -C child rebase_strategy &&
-- 
2.14.1.460.g695108176

[PATCH] pull: honor submodule.recurse config option

[Nicolas Morey-Chaisemartin]

git pull used to not parse the submodule.recurse config option and simply
consider the --recurse-submodules CLI option.
When using the config option, submodules would only be fetched recursively
while the CLi option would tigger both fetch and update/merge.

Reported-by: Magnus Homann <mag...@homann.se>
Signed-off-by: Nicolas Morey-Chaisemartin <nico...@morey-chaisemartin.com>
---
 builtin/pull.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/builtin/pull.c b/builtin/pull.c
index 7fe281414..e4edf23c5 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -326,6 +326,11 @@ static int git_pull_config(const char *var, const char 
*value, void *cb)
config_autostash = git_config_bool(var, value);
return 0;
}
+   if (!strcmp(var, "submodule.recurse")) {
+   int r = git_config_bool(var, value) ?
+   RECURSE_SUBMODULES_ON : RECURSE_SUBMODULES_OFF;
+   recurse_submodules = r;
+   }
return git_default_config(var, value, cb);
 }
 
-- 
2.14.1.460.g196d2604f

Re: Automatically delete branches containing accepted patches?

[Nicolas Morey-Chaisemartin]

You could rebase your branches onto the upstream branch.
Once all the patches are in, the SHA1 of the rebased branch is somewhere in the 
history of the upstream master.

I use a set of scripts I've written to handle multiple branches:
https://github.com/nmorey/git-topic-branches

Using namesapce it knows which branch is against which ref. You can autorebase 
everythinmg. It'll tell you once everything is merged upstream:
$ git auto-rebase  # alias for branch-autorebase
REBASING: dev/cbuild-cleanup on origin/master
REBASING FAILURE: dev/cbuild-cleanup on origin/master
REBASING: dev/cbuild-return-code on origin/master
REBASING: dev/suse-spec2 on origin/master
INTEGRATED UPSTREAM: dev/suse-spec2 just made it into origin/master
REBASING: dev/udev.md on origin/master
INTEGRATED UPSTREAM: dev/udev.md just made it into origin/master

You should be able to either hook up something for auto deletion, or use the 
few helpers to detect the merged ones and delete them


Le 27/08/2017 à 20:44, Lars Schneider a écrit :
> Hi,
>
> I have lots of git/git branches and once in a while some patches make it 
> into git/git master. If this happens I would like to delete my branch
> with the patch automatically. That's not easily possible as the hashes 
> on my branches are, of course, not the same as the hashes on git/git.
>
> How do you deal with this situation? Do you manually delete your 
> branches or do you have some clever script to share?
>
> Thanks,
> Lars

Re: [RFC 0/3] imap-send curl tunnelling support

[Nicolas Morey-Chaisemartin]

Le 24/08/2017 à 15:53, Jeff King a écrit :
> On Thu, Aug 24, 2017 at 10:00:47AM +0200, Nicolas Morey-Chaisemartin wrote:
>
>>> Yes, I agree. I was hoping when we started this discussion that we were
>>> more ready to switch to curl-by-default. But sadly, that isn't close to
>>> being the case. But hopefully we can at least end up with logic that
>>> lets us use it in the easy cases (no tunneling) and falls back in the
>>> harder ones.
>> I opened a bug upstream and they already fixed this.
>> https://github.com/curl/curl/pull/1820
> Cool! That's much faster than I had expected. :)
>
>> At least bleeding edge curl user should be able to use this.
>> I'm not sure where to go with these patches now.
>>
>> 1) There does not seem to be an easy/clean workaround for the lack of 
>> socketpair on windows.
>> Fidling with a loopback AF_UNIX?AF_LOCAL socket should work but it
>> means creating a socket file somewhere which pulls a lot of potential
>> issues (where to put it ? Post-mortem cleanup ? Parallel imap-send ?)
> Even if you create a non-anonymous socket and connect to both ends, I'm
> not sure how it works to pass that to the spawned child. IIRC, our
> run_command emulation cannot pass arbitrary descriptors to the child
> processes (but I don't know the details of why that is the case, or if
> there are windows-specific calls we could be making to work around it).
Well as long as we can map it on a fd, the dup2 trickery should allow to remap 
whatever solution we pick to stdin/stdout.
Could this code be put in a #ifndef WINDOWS ?

>
>> 2) The PREAUTH support won't largely be available  for a while (curl,
>> release, distro, etc.)
>> - If this is the main use case, it does not make much sense to puch
>> curl; tunneling support without this. I could push the code and only
>> enable the curl tunneling for the next curl release ?
>>   Meaning no one (or close to no one) would use this until some later
>>   This also means very little testing (apart from mine) until the next
>> curl version gets widely available
>> - If this is not the main case (or at least the non PREAUTH is
>> important enough), it would make sense to get this changes in.
>>   But it would probably need some more to code to either fallback to
>> legacy mode when curl failed (due to PREAUTH) or detect PREAUTH and
>> directly use the legacy mode.
> It seems like we should be able to hit the cases that we know work out
> of the box, and just hold back the default for the others. Like:
>
>   static int use_curl_auto(void)
>   {
>   #ifndef USE_CURL_FOR_IMAP_SEND
>   /* not built; we cannot use it */
>   return 0;
>   #else
>   if (srvc->tunnel) {
>   #if LIBCURL_VERSION < ...
>   /* no preauth support */
>   return 0;
>   #else
>   return 1;
>   #endif /* LIBCURL_VERSION < ... */
>   }
>   ... other checks go here ...
>   #endif /* USE_CURL */
>   }
>
>   ...
>   int use_curl = -1; /* auto */
>   ... set use_curl to 0/1 from --curl/--no-curl command line */
>   if (use_curl < 0)
>   use_curl = use_curl_auto();
>
> I'm not sure what other cases are left. But over time we'd hope that
> use_curl_auto() would shrink to just "return 1", at which point
> everybody is using it (and we can drop the fallback code).
>

This code works but I'm not that confortable getting code into master that will 
have been pretty much untested (I doubt there are many git pu/next user that 
run the bleeding edge curl on their setup)
and that may just break down once curl gets updated.
It has only been tested using the example line from imap-send man page which is 
a tiny coverage and I'm sure there are some IMAP server with funky 
interpreation of the standard out there (who said Exchange?)

Nicolas

Re: sequencer status

[Nicolas Morey-Chaisemartin]

Le 24/08/2017 à 11:43, Matthieu Moy a écrit :
> Christian Couder  writes:
>
>> It is displaying the steps that have already been performed, right?
>> I wonder if people might want more about the current step (but maybe
>> that belongs to `git status`) or perhaps the not yet performed states
>> (and maybe even a way to edit the todo list?)
> Note that 'git status' is already doing this, but shows only 2 items of
> each:
>
> $ git status
> interactive rebase in progress; onto 3772427
> Last commands done (2 commands done):
>pick a48812c some commit title
>exec false
> Next commands to do (2 remaining commands):
>pick 9d7835d some other commit
>pick c0e0fa8 one more subject
>   (use "git rebase --edit-todo" to view and edit)
> You are currently editing a commit while rebasing branch 'master' on 
> '3772427'.
> ...
>
> The idea was that 2 lines of context is often sufficient, and doesn't
> eat too much screen space so it makes sense to show it by default.
>
> I think it makes sense to have another command that shows the whole
> sequence, but perhaps it could also be just an option for "git status".
>
> Cheers,
>

But this is only for interactive rebase.
It might be worth adding a parameter for this, but I'd also like to see this 
feature for all rebase/cp/revert

Nicolas

Re: sequencer status

[Nicolas Morey-Chaisemartin]

Le 23/08/2017 à 19:57, Junio C Hamano a écrit :
> Nicolas Morey-Chaisemartin <nico...@morey-chaisemartin.com> writes:
>
>> Two questions:
>> - Could this be a candidate for contrib/ ?
>> - Would it be interesting to add the relevant code to sequencer.c
>> so that all sequencer based commands could have a --status option
> I actually think we would want a "git sequencer" command, which can
> be fed an arbitrary instruction sheet created by a third-party and
> told to "run" it.  A new command $cmd that wants to rewrite history
> (like "rebase -i", "cherry-pick A..B", etc. do) can only concentrate
> on preparing the sequence of instructions and then internally invoke
> "git sequencer run" until it gives the control back to the end user.
> When the user tells $cmd to continue, it can relay that request to
> "git sequencer continue" under the hood.  
> Once its use is established, it might be even possible to let users
> run "git sequencer continue", bypassing frontends for individual
> commands, e.g. "git cherry-pick --continue", etc., but I do not know
> if that is generally a good idea or not.  In any case, having such a
> front-end will help third-party scripts that want to build a custom
> workflow using the sequecing machinery we have.
>
> And in such a world, we would need "git sequencer status" command
> to give us where in a larger sequence of instrutions we are.  
>
> So I tend to think this should be part of the core, not contrib/,
> and should become part of a new command "git sequencer".

I like your idea. I'm not sure I have the bandwidth to do this (by myself at 
least).
If someone (hopefully more familiar with the sequencer code than me) wants to 
work on this, I'd gladly help.

Nicolas

Re: sequencer status

[Nicolas Morey-Chaisemartin]

Le 23/08/2017 à 18:40, Christian Couder a écrit :
> Hi,
>
> On Wed, Aug 23, 2017 at 10:10 AM, Nicolas Morey-Chaisemartin
> <nico...@morey-chaisemartin.com> wrote:
>> Hi,
>>
>> I've created a small tool to display the current sequencer status.
>> It mimics what Magit does to display what was done and what is left to do.
>>
>> As someone who often rebase large series of patches (also works with am, 
>> revert, cherry-pick), I really needed the feature and use this daily.
> Yeah, many people use the interactive rebase a lot so I think it could
> be very interesting.
>
>> It's available here:
>> https://github.com/nmorey/git-sequencer-status
>> SUSE and Fedora packages are available here:
>> https://build.opensuse.org/package/show/home:NMoreyChaisemartin:git-tools/git-sequencer-status
>>
>> It's not necessary very robust yet. Most of the time I use simple rebase so 
>> there are a lot of untested corner cases.
>>
>> Here is an example output:
>> $ sequencer-status
>> # Interactive rebase: master onto rebase_conflict
>> exectrue
>> picke54d993 Fourth commit
>> exectrue
>> *pick   b064629 Third commit
>> exectrue
>> pick174c933 Second commit
>> onto773cb23 Alternate second
> It is displaying the steps that have already been performed, right?
> I wonder if people might want more about the current step (but maybe
> that belongs to `git status`) or perhaps the not yet performed states
> (and maybe even a way to edit the todo list?)

Yes it is displaying all steps.
The line beginning by '*' is the current step.

Trying to "guess" what is happening on the current step is quite hard. Between 
conflict, empty commits, stopped for amending and other, it's a lot of cases to 
handle.
I'd rather have git-status deal with it (and you get your standard log/error 
fro your rebase/cp/am/revert command too).
The idea here is really to find out where you are in your operation sequence.

I've had a 700 patch series to reapply on a different subtree. Took me 3 days. 
This script was quite handy. (Also depressing as you can see how much work left 
there is).

Also if you feel it's missing something you need, I'm accepting PR on github ;)

>> Two questions:
>> - Could this be a candidate for contrib/ ?
> It seems to me that these days we don't often add new tools to contrib/.
>
>> - Would it be interesting to add the relevant code to sequencer.c so that 
>> all sequencer based commands could have a --status option ?
> Yeah, it's probably better if it's integrated in git, either as a
> --status option in some commands, or perhaps as an option of `git
> status`.

I'll have a look at what can be done.

Thanks

Nicolas

[RFC 0/3] imap-send curl tunnelling support

[Nicolas Morey-Chaisemartin]

Le 23/08/2017 à 23:43, Jeff King a écrit :
> On Mon, Aug 21, 2017 at 09:34:19AM +0200, Nicolas Morey-Chaisemartin wrote:
>
>>>> It appears curl do not support the PREAUTH tag.
>>> Too bad. IMHO preauth is the main reason to use a tunnel in the first
>>> place.
>> It shouldn't be too hard to add support for this in curl.
>> If it's the main usecase, it'll simply means the curl tunnelling
>> should be disabled by default for older curl (in this case, meaning
>> every version until it gets supported) versions.
> Yes, I agree. I was hoping when we started this discussion that we were
> more ready to switch to curl-by-default. But sadly, that isn't close to
> being the case. But hopefully we can at least end up with logic that
> lets us use it in the easy cases (no tunneling) and falls back in the
> harder ones.
>
> -Peff
I opened a bug upstream and they already fixed this.
https://github.com/curl/curl/pull/1820

At least bleeding edge curl user should be able to use this.
I'm not sure where to go with these patches now.

1) There does not seem to be an easy/clean workaround for the lack of 
socketpair on windows.
Fidling with a loopback AF_UNIX?AF_LOCAL socket should work but it means 
creating a socket file somewhere which pulls a lot of potential issues (where 
to put it ? Post-mortem cleanup ? Parallel imap-send ?)

2) The PREAUTH support won't largely be available  for a while (curl, release, 
distro, etc.)
- If this is the main use case, it does not make much sense to puch curl; 
tunneling support without this. I could push the code and only enable the curl 
tunneling for the next curl release ?
  Meaning no one (or close to no one) would use this until some later
  This also means very little testing (apart from mine) until the next curl 
version gets widely available
- If this is not the main case (or at least the non PREAUTH is important 
enough), it would make sense to get this changes in.
  But it would probably need some more to code to either fallback to legacy 
mode when curl failed (due to PREAUTH) or detect PREAUTH and directly use the 
legacy mode.

Nicolas

Re: [PATCH v3 4/4] imap-send: use curl by default

[Nicolas Morey-Chaisemartin]

Le 23/08/2017 à 22:28, Junio C Hamano a écrit :
> Nicolas Morey-Chaisemartin <nico...@morey-chaisemartin.com> writes:
>
>> Now that curl is enable by default,
> s/enable//; 
>
> But it is unclear what the above really means.  You certainly do not
> mean that [PATCH 1-3/4] somewhere tweaked our Makefile to always use
> libcurl and makes Git fail to build without it, but the above sounds
> as if that were the case.
>
>> use the curl implementation
>> for imap too.
> The Makefile for a long time by default set USE_CURL_FOR_IMAP_SEND
> to YesPlease when the version of cURL we have is recent enough, I
> think.  So I am not sure what you want to add with this change.
It did but apart from allowing the compilation of the code and enabling the 
--curl option to do something, it had no impact on the default runtime.
>> The goal is to validate feature parity between the legacy and
>> the curl implementation, deprecate thee legacy implementation
> s/thee/the/;

Yes this is confusing.
In the current state, even if build against a curl version supporting imap, 
curl is not used by default at runtime (unless OpenSSL was not available).
This patch changes this behavior.

>
>> later on and in the long term, hopefully drop it altogether.
>>
>> Signed-off-by: Nicolas Morey-Chaisemartin <nico...@morey-chaisemartin.com>
>> ---
>>  imap-send.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
> Hmph, the patch itself is also confusing.
>
>> diff --git a/imap-send.c b/imap-send.c
>> index a74d011a9..58c191704 100644
>> --- a/imap-send.c
>> +++ b/imap-send.c
>> @@ -35,11 +35,11 @@ typedef void *SSL;
>>  #include "http.h"
>>  #endif
>>  
>> -#if defined(USE_CURL_FOR_IMAP_SEND) && defined(NO_OPENSSL)
>> -/* only available option */
>> +#if defined(USE_CURL_FOR_IMAP_SEND)
>> +/* Always default to curl if it's available. */
>>  #define USE_CURL_DEFAULT 1
> The original says "we want to use CURL, if Makefile tells us to
> *AND* if Makefile tells us not to use OpenSSL", which does not make
> much sense to me.  I wonder if the original is buggy and should have
> been using "|| defined(NO_OPENSSL)" there instead.  
I think the idea for this was that curl should be used when curl is available 
and OpenSSL is not (curl being the only solution for secured authentication in 
this case)

>
> Perhaps that is the bug you are fixing with this patch, and all the
> talk about curl is default we saw above is a red herring?  If that
> is the case, then instead of removing, turning "&&" into "||" may be
> a better fix.  I dunno.

As said before, the goal of this patch is to enable curl by default at runtime 
when it has been "enabled" at compile time.
I'll reword

Is something like this clearer ?
Author: Nicolas Morey-Chaisemartin <nico...@morey-chaisemartin.com>
Date:   Sun Aug 6 21:30:15 2017 +0200

    imap-send: use curl by default when possible
   
    Set curl as the runtime default when it is available.
    When linked against older curl versions (< 7_34_0) or without curl,
    use the legacy imap implementation.
   
    The goal is to validate feature parity between the legacy and
    the curl implementation, deprecate the legacy implementation
    later on and in the long term, hopefully drop it altogether.
   
    Signed-off-by: Nicolas Morey-Chaisemartin <nico...@morey-chaisemartin.com>

Re: [PATCH v3 1/4] imap-send: return with error if curl failed

[Nicolas Morey-Chaisemartin]

Le 23/08/2017 à 22:12, Junio C Hamano a écrit :
> Nicolas Morey-Chaisemartin <nico...@morey-chaisemartin.com> writes:
>
>> curl_append_msgs_to_imap always returned 0, whether curl failed or not.
>> Return a proper status so git imap-send will exit with an error code
>> if womething wrong happened.
>>
>> Signed-off-by: Nicolas Morey-Chaisemartin <nico...@morey-chaisemartin.com>
>> ---
>>  imap-send.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/imap-send.c b/imap-send.c
>> index b2d0b849b..09f29ea95 100644
>> --- a/imap-send.c
>> +++ b/imap-send.c
>> @@ -1490,7 +1490,7 @@ static int curl_append_msgs_to_imap(struct 
>> imap_server_conf *server,
>>  curl_easy_cleanup(curl);
>>  curl_global_cleanup();
>>  
>> -return 0;
>> +return res == CURLE_OK;
>>  }
>>  #endif
> Wait a bit.  Did you mean "res != CURLE_OK"?  If we got an OK, we
> want to return 0 as success, because the value we return from here
> is returned by cmd_main() as-is to main() and to the outside world,
> no?
>
>


Good catch. I remember testing this out but I messed up somewhere along the 
line.

Nicolas

Re: [PATCH v3 2/4] imap-send: add wrapper to get server credentials if needed

[Nicolas Morey-Chaisemartin]

Le 23/08/2017 à 22:13, Junio C Hamano a écrit :
> Nicolas Morey-Chaisemartin <nico...@morey-chaisemartin.com> writes:
>
>> Signed-off-by: Nicolas Morey-Chaisemartin <nico...@morey-chaisemartin.com>
>> ---
>>  imap-send.c | 34 --
>>  1 file changed, 20 insertions(+), 14 deletions(-)
>>
>> diff --git a/imap-send.c b/imap-send.c
>> index 09f29ea95..448a4a0b3 100644
>> --- a/imap-send.c
>> +++ b/imap-send.c
>> @@ -926,6 +926,25 @@ static int auth_cram_md5(struct imap_store *ctx, struct 
>> imap_cmd *cmd, const cha
>>  return 0;
>>  }
>>  
>> +static void server_fill_credential(struct imap_server_conf *srvc, struct 
>> credential *cred)
>> +{
>> +if (srvc->user && srvc->pass)
>> +return;
>> +
>> +cred->protocol = xstrdup(srvc->use_ssl ? "imaps" : "imap");
>> +cred->host = xstrdup(srvc->host);
>> +
>> +cred->username = xstrdup_or_null(srvc->user);
>> +cred->password = xstrdup_or_null(srvc->pass);
>> +
>> +credential_fill(cred);
>> +
>> +if (!srvc->user)
>> +srvc->user = xstrdup(cred->username);
>> +if (!srvc->pass)
>> +srvc->pass = xstrdup(cred->password);
>> +}
>> +
> This looks straight-forward code movement.  The only thing that
> makes me wonder is if this is "server".  The existing naming of the
> variables screams at me that this is not "server" but is "service".

I read srvc as server conf not service.
But I can change the name if you prefer

Nicolas

sequencer status

[Nicolas Morey-Chaisemartin]

Hi,

I've created a small tool to display the current sequencer status.
It mimics what Magit does to display what was done and what is left to do.

As someone who often rebase large series of patches (also works with am, 
revert, cherry-pick), I really needed the feature and use this daily.

It's available here:
https://github.com/nmorey/git-sequencer-status
SUSE and Fedora packages are available here:
https://build.opensuse.org/package/show/home:NMoreyChaisemartin:git-tools/git-sequencer-status

It's not necessary very robust yet. Most of the time I use simple rebase so 
there are a lot of untested corner cases.

Here is an example output:
$ sequencer-status
# Interactive rebase: master onto rebase_conflict
exec    true
pick    e54d993 Fourth commit
exec    true
*pick   b064629 Third commit
exec    true
pick    174c933 Second commit
onto    773cb23 Alternate second


Two questions:
- Could this be a candidate for contrib/ ?
- Would it be interesting to add the relevant code to sequencer.c so that all 
sequencer based commands could have a --status option ?

Nicolas

Re: [RFC 0/3] imap-send curl tunnelling support

[Nicolas Morey-Chaisemartin]

This was sadly kind of expected...
I need to look for another way to handle this on Windows.

Thanks for the test

Nicolas

Le 22/08/2017 à 19:10, Johannes Sixt a écrit :
> Am 21.08.2017 um 09:27 schrieb Nicolas Morey-Chaisemartin:
>> (Sent a reply from my phone while out of town but couldn't find it so here 
>> it is again)
>>
>> It's available on my github:
>> https://github.com/nmorey/git/tree/dev/curl-tunnel
>>
>> The series had been stlighly changed since the patch were posted, mostly to 
>> add the proper ifdefs to handle older curl versions.
>
> This does not build for me on Windows due to a missing socketpair() function. 
> But I am working in an old environment, so I do not know whether this 
> statement has much value.
>
> -- Hannes

[PATCH v3 4/4] imap-send: use curl by default

[Nicolas Morey-Chaisemartin]

Now that curl is enable by default, use the curl implementation
for imap too.
The goal is to validate feature parity between the legacy and
the curl implementation, deprecate thee legacy implementation
later on and in the long term, hopefully drop it altogether.

Signed-off-by: Nicolas Morey-Chaisemartin <nico...@morey-chaisemartin.com>
---
 imap-send.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/imap-send.c b/imap-send.c
index a74d011a9..58c191704 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -35,11 +35,11 @@ typedef void *SSL;
 #include "http.h"
 #endif
 
-#if defined(USE_CURL_FOR_IMAP_SEND) && defined(NO_OPENSSL)
-/* only available option */
+#if defined(USE_CURL_FOR_IMAP_SEND)
+/* Always default to curl if it's available. */
 #define USE_CURL_DEFAULT 1
 #else
-/* strictly opt in */
+/* We don't have curl, so continue to use the historical implementation */
 #define USE_CURL_DEFAULT 0
 #endif
 
-- 
2.14.0.1.gd9597ce13

[PATCH v3 3/4] imap_send: setup_curl: retreive credentials if not set in config file

[Nicolas Morey-Chaisemartin]

Up to this point, the curl mode only supported getting the username
and password from the gitconfig file while the legacy mode could also
fetch them using the credential API.

Signed-off-by: Nicolas Morey-Chaisemartin <nico...@morey-chaisemartin.com>
---
 imap-send.c | 18 --
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/imap-send.c b/imap-send.c
index 448a4a0b3..a74d011a9 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -1398,7 +1398,7 @@ static int append_msgs_to_imap(struct imap_server_conf 
*server,
 }
 
 #ifdef USE_CURL_FOR_IMAP_SEND
-static CURL *setup_curl(struct imap_server_conf *srvc)
+static CURL *setup_curl(struct imap_server_conf *srvc, struct credential *cred)
 {
CURL *curl;
struct strbuf path = STRBUF_INIT;
@@ -1411,6 +1411,7 @@ static CURL *setup_curl(struct imap_server_conf *srvc)
if (!curl)
die("curl_easy_init failed");
 
+   server_fill_credential(, cred);
curl_easy_setopt(curl, CURLOPT_USERNAME, server.user);
curl_easy_setopt(curl, CURLOPT_PASSWORD, server.pass);
 
@@ -1460,8 +1461,9 @@ static int curl_append_msgs_to_imap(struct 
imap_server_conf *server,
struct buffer msgbuf = { STRBUF_INIT, 0 };
CURL *curl;
CURLcode res = CURLE_OK;
+   struct credential cred = CREDENTIAL_INIT;
 
-   curl = setup_curl(server);
+   curl = setup_curl(server, );
curl_easy_setopt(curl, CURLOPT_READDATA, );
 
fprintf(stderr, "sending %d message%s\n", total, (total != 1) ? "s" : 
"");
@@ -1496,6 +1498,18 @@ static int curl_append_msgs_to_imap(struct 
imap_server_conf *server,
curl_easy_cleanup(curl);
curl_global_cleanup();
 
+   if (cred.username)
+   if (res == CURLE_OK)
+   credential_approve();
+#if LIBCURL_VERSION_NUM >= 0x070d01
+   else if (res == CURLE_LOGIN_DENIED)
+#else
+   else
+#endif
+   credential_reject();
+
+   credential_clear();
+
return res == CURLE_OK;
 }
 #endif
-- 
2.14.0.1.gd9597ce13

[PATCH v3 2/4] imap-send: add wrapper to get server credentials if needed

[Nicolas Morey-Chaisemartin]

Signed-off-by: Nicolas Morey-Chaisemartin <nico...@morey-chaisemartin.com>
---
 imap-send.c | 34 --
 1 file changed, 20 insertions(+), 14 deletions(-)

diff --git a/imap-send.c b/imap-send.c
index 09f29ea95..448a4a0b3 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -926,6 +926,25 @@ static int auth_cram_md5(struct imap_store *ctx, struct 
imap_cmd *cmd, const cha
return 0;
 }
 
+static void server_fill_credential(struct imap_server_conf *srvc, struct 
credential *cred)
+{
+   if (srvc->user && srvc->pass)
+   return;
+
+   cred->protocol = xstrdup(srvc->use_ssl ? "imaps" : "imap");
+   cred->host = xstrdup(srvc->host);
+
+   cred->username = xstrdup_or_null(srvc->user);
+   cred->password = xstrdup_or_null(srvc->pass);
+
+   credential_fill(cred);
+
+   if (!srvc->user)
+   srvc->user = xstrdup(cred->username);
+   if (!srvc->pass)
+   srvc->pass = xstrdup(cred->password);
+}
+
 static struct imap_store *imap_open_store(struct imap_server_conf *srvc, char 
*folder)
 {
struct credential cred = CREDENTIAL_INIT;
@@ -1078,20 +1097,7 @@ static struct imap_store *imap_open_store(struct 
imap_server_conf *srvc, char *f
}
 #endif
imap_info("Logging in...\n");
-   if (!srvc->user || !srvc->pass) {
-   cred.protocol = xstrdup(srvc->use_ssl ? "imaps" : 
"imap");
-   cred.host = xstrdup(srvc->host);
-
-   cred.username = xstrdup_or_null(srvc->user);
-   cred.password = xstrdup_or_null(srvc->pass);
-
-   credential_fill();
-
-   if (!srvc->user)
-   srvc->user = xstrdup(cred.username);
-   if (!srvc->pass)
-   srvc->pass = xstrdup(cred.password);
-   }
+   server_fill_credential(srvc, );
 
if (srvc->auth_method) {
struct imap_cmd_cb cb;
-- 
2.14.0.1.gd9597ce13

[PATCH v3 1/4] imap-send: return with error if curl failed

[Nicolas Morey-Chaisemartin]

curl_append_msgs_to_imap always returned 0, whether curl failed or not.
Return a proper status so git imap-send will exit with an error code
if womething wrong happened.

Signed-off-by: Nicolas Morey-Chaisemartin <nico...@morey-chaisemartin.com>
---
 imap-send.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/imap-send.c b/imap-send.c
index b2d0b849b..09f29ea95 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -1490,7 +1490,7 @@ static int curl_append_msgs_to_imap(struct 
imap_server_conf *server,
curl_easy_cleanup(curl);
curl_global_cleanup();
 
-   return 0;
+   return res == CURLE_OK;
 }
 #endif
 
-- 
2.14.0.1.gd9597ce13

[PATCH v3 0/4] imap-send: Fix and enable curl by default

[Nicolas Morey-Chaisemartin]

Changes since v2:
Patch 3 reject credit when curl failed:
 - when a login error  is returned (curl >= 7.13.1)
 - for any curl error in older versions

Nicolas Morey-Chaisemartin (4):
  imap-send: return with error if curl failed
  imap-send: add wrapper to get server credentials if needed
  imap_send: setup_curl: retreive credentials if not set in config file
  imap-send: use curl by default

 imap-send.c | 60 
 1 file changed, 40 insertions(+), 20 deletions(-)

-- 
2.14.0.1.gd9597ce13

Re: [RFC 0/3] imap-send curl tunnelling support

[Nicolas Morey-Chaisemartin]

Le 16/08/2017 à 10:34, Jeff King a écrit :
> On Wed, Aug 09, 2017 at 04:43:26PM +0200, Nicolas Morey-Chaisemartin wrote:
>
>> I have a few doubt on patch #2:
>> - is socketpair working on all git supported system (windows ?)
> I'm pretty sure the answer is no, after searching a bit for mingw and
> socketpair. The big question is whether we could come up with a suitable
> replacement. And that would depend on how libcurl works on Windows, I
> think (because it's going to feed whatever we give it to other syscall
> wrappers).

That's what I feared.
I'm not sure there is a portable "anonymous socket" API out there that'll 
work...

>> - should socketpair always be used or limited to the curl over tunnel case ?
>>   I don't think there is too much different between an unname pipe and a 
>> socketpair but I'm not sure either :)
> There's not much difference in practice. The obvious one is that
> half-duplex shutdowns require shutdown() on a socket and just close() on
> the write half of a pipe. I don't know if we do that or not.
>
> I'd be inclined to leave the existing code alone, though, just because
> of the risk of regression (and because I don't think the curl and
> non-curl versions actually share that much code). But I haven't looked
> deeply, so I may be wrong.
>
It's easy enough to keep the legacy working without socketpair.

>> It appears curl do not support the PREAUTH tag.
> Too bad. IMHO preauth is the main reason to use a tunnel in the first
> place.

It shouldn't be too hard to add support for this in curl.
If it's the main usecase, it'll simply means the curl tunnelling should be 
disabled by default for older curl (in this case, meaning every version until 
it gets supported) versions.

Nicolas


0x801BDDB825988F64.asc
Description: application/pgp-keys

Re: [RFC 0/3] imap-send curl tunnelling support

[Nicolas Morey-Chaisemartin]

(Sent a reply from my phone while out of town but couldn't find it so here it 
is again)

It's available on my github:
https://github.com/nmorey/git/tree/dev/curl-tunnel

The series had been stlighly changed since the patch were posted, mostly to add 
the proper ifdefs to handle older curl versions.

Nicolas

Le 16/08/2017 à 14:30, Johannes Schindelin a écrit :
> Hi,
>
> On Tue, 15 Aug 2017, Stefan Beller wrote:
>
>> On Tue, Aug 15, 2017 at 10:49 AM, Nicolas Morey-Chaisemartin
>> <nico...@morey-chaisemartin.com> wrote:
>>> Ping.
>>>
>>> I'd like to get feedback from Windows developer on patch #2
>>> Patch#3 will probably need some updates as I expected Jeff old curl drop 
>>> patches to make it in.
>>> As it seems to be going another way a few more ifdefs will be required
>> +cc Windows devs
> I can has easy-to-pull branch, please?
>
> Thanks,
> Dscho



0x801BDDB825988F64.asc
Description: application/pgp-keys

Re: [RFC 0/3] imap-send curl tunnelling support

[Nicolas Morey-Chaisemartin]

Ping.

I'd like to get feedback from Windows developer on patch #2
Patch#3 will probably need some updates as I expected Jeff old curl drop 
patches to make it in.
As it seems to be going another way a few more ifdefs will be required

Nicolas

Le 09/08/2017 à 16:43, Nicolas Morey-Chaisemartin a écrit :
> From 7.21.5, curl can be tricked into using an open fd.
> This series uses this to allow using curl over a tunnel.
>
> I have a few doubt on patch #2:
> - is socketpair working on all git supported system (windows ?)
> - should socketpair always be used or limited to the curl over tunnel case ?
>   I don't think there is too much different between an unname pipe and a 
> socketpair but I'm not sure either :)
>
> This series also shows a "bug" in curl.
> When trying out the tunnel example fro imap-send documentation, this happends:
> Starting tunnel 'ssh -q -C localhost /usr/sbin/imapd ./Maildir'... ok
> sending 3 messages
> 16:38:54.055221 http.c:639  == Info: Hostname was NOT found in 
> DNS cache
> 16:38:54.059505 http.c:639  == Info:   Trying ::1...
> 16:38:54.059545 http.c:639  == Info: Connected to localhost () 
> port 143 (#0)
> 16:38:54.354379 http.c:586  <= Recv header, 000332 bytes 
> (0x014c)
> 16:38:54.354405 http.c:598  <= Recv header: * PREAUTH [CAPABILITY 
> IMAP4REV1 I18NLEVEL=1 LITERAL+ IDLE UIDPLUS NAMESPACE CHILDREN 
> MAILBOX-REFERRALS BINARY UNSELECT ESEARCH WITHIN SCAN SORT THREAD=REFERENCES 
> THREAD=ORDEREDSUBJECT MULTIAPPEND] Pre-authenticated user nmorey 
> portia.home.nicolas.morey-chaisemartin.com IMAP4rev1 2007e.404 at Wed, 9 Aug 
> 2017 16:38:54 +0200 (CEST)
> 16:38:54.354425 http.c:639  == Info: Bad tagged response
> 16:38:54.354448 http.c:639  == Info: Closing connection 0
> curl_easy_perform() failed: FTP: weird server reply
>
> It appears curl do not support the PREAUTH tag.
>
> However a test with "nc imap.server.ext 143" is working fine.
>
> Nicolas Morey-Chaisemartin (3):
>   imap-send: move tunnel setup to its own function
>   imap-send: use a socketpair instead of pipe to communicate with the
> tunnel
>   imap_send: add support for curl over tunnel
>
>  Documentation/git-imap-send.txt |  4 +-
>  imap-send.c | 91 
> +++--
>  2 files changed, 72 insertions(+), 23 deletions(-)
>

[PATCH] stash: clean untracked files before reset

[Nicolas Morey-Chaisemartin]

If calling git stash -u on a repo that contains a file that is not
ignored any more due to a current modification of the gitignore file,
this file is stashed but not remove from the working tree.
This is due to git-stash first doing a reset --hard which clears the
.gitignore file modification and the call git clean, leaving the file
untouched.
This causes git stash pop to fail due to the file existing.

This patch simply switches the order between cleaning and resetting
and adds a test for this usecase.

Signed-off-by: Nicolas Morey-Chaisemartin <nico...@morey-chaisemartin.com>
Reported-by: Sam Partington <s...@whiteoctober.co.uk>
---
 git-stash.sh   | 11 ++-
 t/t3905-stash-include-untracked.sh | 18 ++
 2 files changed, 24 insertions(+), 5 deletions(-)

diff --git a/git-stash.sh b/git-stash.sh
index 9b6c2da7b..39083b4d9 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -300,6 +300,12 @@ push_stash () {
 
if test -z "$patch_mode"
then
+   test "$untracked" = "all" && CLEAN_X_OPTION=-x || 
CLEAN_X_OPTION=
+   if test -n "$untracked"
+   then
+   git clean --force --quiet -d $CLEAN_X_OPTION -- "$@"
+   fi
+
if test $# != 0
then
git reset -q -- "$@"
@@ -309,11 +315,6 @@ push_stash () {
else
git reset --hard -q
fi
-   test "$untracked" = "all" && CLEAN_X_OPTION=-x || 
CLEAN_X_OPTION=
-   if test -n "$untracked"
-   then
-   git clean --force --quiet -d $CLEAN_X_OPTION -- "$@"
-   fi
 
if test "$keep_index" = "t" && test -n "$i_tree"
then
diff --git a/t/t3905-stash-include-untracked.sh 
b/t/t3905-stash-include-untracked.sh
index 193adc7b6..c1f84d3d5 100755
--- a/t/t3905-stash-include-untracked.sh
+++ b/t/t3905-stash-include-untracked.sh
@@ -211,4 +211,22 @@ test_expect_success 'stash push with $IFS character' '
test_path_is_file bar
 '
 
+cat > .gitignore <ignored.d/foo &&
+   echo "!ignored.d/foo" >> .gitignore &&
+   git stash save --include-untracked &&
+   test_path_is_missing ignored.d/foo &&
+   git stash pop &&
+   test_path_is_file ignored.d/foo
+'
+
 test_done
-- 
2.14.0.1.gd9597ce13

Re: git-describe --contains

[Nicolas Morey-Chaisemartin]

Le 11/08/2017 à 08:50, Davide Cavallari a écrit :
> Please help me understand how this command works. There is one case in the
> linux kernel repository that puzzles me. Let's consider patch "drm/i915/
> execlists: Reset RING registers upon resume" [1]. This patch was committed 641
> commits after version 4.8-rc2:
>
> ~$ git describe bafb2f7d4755bf1571bd5e9a03b97f3fc4fe69ae
> v4.8-rc2-641-gbafb2f7d4755
>
> So I would expect to find it in version 4.8-rc3 and later versions.
>
> However, if I search for the tag that follows (and hence contains) that
> commit, I do not find version 4.8-rc3, nor version 4.8, nor version 4.9, but
> 4.10-rc1:
>
> ~$ git describe --contains bafb2f7d4755bf1571bd5e9a03b97f3fc4fe69ae
> v4.10-rc1~154^2~44^2~178
>
> Why? Why not v4.8-rc3? This means that the patch has been included neither in
> v4.8 nor in v4.9, but only in version 4.10-rc1, right? Why so much time was
> needed, considering it was the 621st commit on top ov v4.8-rc2?

Because it was not in v4.8-rc3.
This probably means it was applied on a branch that started from somewhere 
between 4.8-rc2 and 4.8-rc3 but it was only merge into master after v4.9 was 
released


>
> BTW, what are the numbers 154^2~44^2~178 that follow the tag name?

This is due to merges. You have basically a roadmap to go up the ancestry graph.
~154 means 154 commit before the tag
^2 means the 2nd parent of this commit.
and so on...

The format is detailed (among other tings) in man gitrevisions

Nicolas

Re: [PATCH 2/4] http: drop support for curl < 7.16.0

[Nicolas Morey-Chaisemartin]

Le 09/08/2017 à 23:17, Jeff King a écrit :
> On Wed, Aug 09, 2017 at 08:03:05PM +0200, Nicolas Morey-Chaisemartin wrote:
>
>>>> -#if LIBCURL_VERSION_NUM >= 0x071700
>>>> -/* Use CURLOPT_KEYPASSWD as is */
>>>> -#elif LIBCURL_VERSION_NUM >= 0x070903
>>>> -#define CURLOPT_KEYPASSWD CURLOPT_SSLKEYPASSWD
>>>> -#else
>>>> -#define CURLOPT_KEYPASSWD CURLOPT_SSLCERTPASSWD
>>>> -#endif
>>>> -
>>> This part I am not sure.  Don't we still need to substitute
>>> CURLOPT_KEYPASSWD with CURLOPT_SSLKEYPASSWD for versions below
>>> 071700, e.g. 071000 which is 7.16.0?
>> According to the documentation:
>>
>> https://curl.haxx.se/libcurl/c/CURLOPT_KEYPASSWD.html
>> This option was known as CURLOPT_SSLKEYPASSWD up to 7.16.4 and
>> CURLOPT_SSLCERTPASSWD up to 7.9.2.
>>
>>
>> So the patch breaks things (broken for 7.16.[0-4]). But the series
>> does not as the next patch ensure at least 7.19.4
> But the #ifdef above says 071700, which is 7.23.0. I wonder if we just
> got it wrong back then (maybe hex confusion with 7.17.0?). I have a
> build setup for old versions of curl, so I'll double-check that 7.19.4
> builds with KEYPASSWD. And dig in the history to see if there's any
> comment on this mismatch.
>
> -Peff
It seems to be a decimal/hex issue:
docs/libcurl/symbols-in-versions:153:CURLOPT_KEYPASSWD   7.17.0

I guess it should still work because it is now defined like this:
curl.h:#define CURLOPT_SSLKEYPASSWD CURLOPT_KEYPASSWD

If I'm not mistaken on cpp behaviour it means CURLOPT_KEYPASSWD is evaluated to 
CURLOPT_SSLKEYPASSWD (git define) which is evaluated into CURLOPT_KEYPASSWD 
(curl define).
It should stop here as CURLOPT_KEYPASSWD was not a defined macro when the curl 
one was evaluated.
It might be worth cleaning though, specially it wouldn't work anymore if the 
git macro is ever moved before the curl include.

Nicolas

Re: [PATCH 2/4] http: drop support for curl < 7.16.0

[Nicolas Morey-Chaisemartin]

Le 09/08/2017 à 19:40, Junio C Hamano a écrit :
> Jeff King  writes:
>
>> -#if LIBCURL_VERSION_NUM >= 0x071700
>> -/* Use CURLOPT_KEYPASSWD as is */
>> -#elif LIBCURL_VERSION_NUM >= 0x070903
>> -#define CURLOPT_KEYPASSWD CURLOPT_SSLKEYPASSWD
>> -#else
>> -#define CURLOPT_KEYPASSWD CURLOPT_SSLCERTPASSWD
>> -#endif
>> -
> This part I am not sure.  Don't we still need to substitute
> CURLOPT_KEYPASSWD with CURLOPT_SSLKEYPASSWD for versions below
> 071700, e.g. 071000 which is 7.16.0?
According to the documentation:

https://curl.haxx.se/libcurl/c/CURLOPT_KEYPASSWD.html
This option was known as CURLOPT_SSLKEYPASSWD up to 7.16.4 and 
CURLOPT_SSLCERTPASSWD up to 7.9.2.


So the patch breaks things (broken for 7.16.[0-4]). But the series does not as 
the next patch ensure at least 7.19.4

[RFC 0/3] imap-send curl tunnelling support

[Nicolas Morey-Chaisemartin]

>From 7.21.5, curl can be tricked into using an open fd.
This series uses this to allow using curl over a tunnel.

I have a few doubt on patch #2:
- is socketpair working on all git supported system (windows ?)
- should socketpair always be used or limited to the curl over tunnel case ?
  I don't think there is too much different between an unname pipe and a 
socketpair but I'm not sure either :)

This series also shows a "bug" in curl.
When trying out the tunnel example fro imap-send documentation, this happends:
Starting tunnel 'ssh -q -C localhost /usr/sbin/imapd ./Maildir'... ok
sending 3 messages
16:38:54.055221 http.c:639  == Info: Hostname was NOT found in DNS 
cache
16:38:54.059505 http.c:639  == Info:   Trying ::1...
16:38:54.059545 http.c:639  == Info: Connected to localhost () port 
143 (#0)
16:38:54.354379 http.c:586  <= Recv header, 000332 bytes 
(0x014c)
16:38:54.354405 http.c:598  <= Recv header: * PREAUTH [CAPABILITY 
IMAP4REV1 I18NLEVEL=1 LITERAL+ IDLE UIDPLUS NAMESPACE CHILDREN 
MAILBOX-REFERRALS BINARY UNSELECT ESEARCH WITHIN SCAN SORT THREAD=REFERENCES 
THREAD=ORDEREDSUBJECT MULTIAPPEND] Pre-authenticated user nmorey 
portia.home.nicolas.morey-chaisemartin.com IMAP4rev1 2007e.404 at Wed, 9 Aug 
2017 16:38:54 +0200 (CEST)
16:38:54.354425 http.c:639  == Info: Bad tagged response
16:38:54.354448 http.c:639  == Info: Closing connection 0
curl_easy_perform() failed: FTP: weird server reply

It appears curl do not support the PREAUTH tag.

However a test with "nc imap.server.ext 143" is working fine.

Nicolas Morey-Chaisemartin (3):
  imap-send: move tunnel setup to its own function
  imap-send: use a socketpair instead of pipe to communicate with the
tunnel
  imap_send: add support for curl over tunnel

 Documentation/git-imap-send.txt |  4 +-
 imap-send.c | 91 +++--
 2 files changed, 72 insertions(+), 23 deletions(-)

[RFC 2/3] imap-send: use a socketpair instead of pipe to communicate with the tunnel

[Nicolas Morey-Chaisemartin]

Signed-off-by: Nicolas Morey-Chaisemartin <nico...@morey-chaisemartin.com>
---
 imap-send.c | 17 +
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/imap-send.c b/imap-send.c
index 10f668eb7..e5ff70096 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -929,18 +929,27 @@ static int auth_cram_md5(struct imap_store *ctx, struct 
imap_cmd *cmd, const cha
 static void setup_tunnel(struct imap_server_conf *srvc, int fds[2])
 {
struct child_process tunnel = CHILD_PROCESS_INIT;
+   int sock_fds[2];
 
imap_info("Starting tunnel '%s'... ", srvc->tunnel);
 
+   if (socketpair(AF_UNIX, SOCK_STREAM, 0, sock_fds))
+   die("failed to create socketpair for proxy");
+
argv_array_push(, srvc->tunnel);
tunnel.use_shell = 1;
-   tunnel.in = -1;
-   tunnel.out = -1;
+   tunnel.in = sock_fds[1];
+   /* Duplicate the fd as the child process requires
+* 1 for stdin, one for stdout */
+   tunnel.out = dup(sock_fds[1]);
+   if (tunnel.out < 0)
+   die("failed to create fd to proxy");
+
if (start_command())
die("cannot start proxy %s", srvc->tunnel);
 
-   fds[0] = tunnel.out;
-   fds[1] = tunnel.in;
+   fds[0] = sock_fds[0];
+   fds[1] = sock_fds[0];
 
imap_info("ok\n");
 
-- 
2.14.0.3.gb4ff627ec.dirty

[RFC 3/3] imap_send: add support for curl over tunnel

[Nicolas Morey-Chaisemartin]

Starting from libcurl 7.21.5, libcurl can be tricked into using
an already open socket.
This allows to use tunneling with libcurl instead of the legacy imap code.

Signed-off-by: Nicolas Morey-Chaisemartin <nico...@morey-chaisemartin.com>
---
 Documentation/git-imap-send.txt |  4 ++--
 imap-send.c | 45 +++--
 2 files changed, 41 insertions(+), 8 deletions(-)

diff --git a/Documentation/git-imap-send.txt b/Documentation/git-imap-send.txt
index 5d1e4c80c..e765c08d7 100644
--- a/Documentation/git-imap-send.txt
+++ b/Documentation/git-imap-send.txt
@@ -38,8 +38,8 @@ OPTIONS
Be quiet.
 
 --curl::
-   Use libcurl to communicate with the IMAP server, unless tunneling
-   into it.  Ignored if Git was built without the USE_CURL_FOR_IMAP_SEND
+   Use libcurl to communicate with the IMAP server.
+   Ignored if Git was built without the USE_CURL_FOR_IMAP_SEND
option set.
 
 --no-curl::
diff --git a/imap-send.c b/imap-send.c
index e5ff70096..31b93d873 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -1408,6 +1408,26 @@ static int append_msgs_to_imap(struct imap_server_conf 
*server,
 }
 
 #ifdef USE_CURL_FOR_IMAP_SEND
+static curl_socket_t curl_tunnel_socket(void *clientp,
+   curlsocktype purpose,
+   struct curl_sockaddr *address)
+{
+   return (unsigned long)clientp;
+}
+
+static int sockopt_callback(void *clientp, curl_socket_t curlfd,
+curlsocktype purpose)
+{
+   /* CURL_SOCKOPT_ALREADY_CONNECTED was intreocued in 7.21.5
+* and is needed to get curl working on an existing fd */
+#if LIBCURL_VERSION_NUM >= 0x071505
+   return CURL_SOCKOPT_ALREADY_CONNECTED;
+#else
+   return CURL_SOCKOPT_ERROR;
+#endif
+}
+
+
 static CURL *setup_curl(struct imap_server_conf *srvc)
 {
CURL *curl;
@@ -1424,8 +1444,21 @@ static CURL *setup_curl(struct imap_server_conf *srvc)
curl_easy_setopt(curl, CURLOPT_USERNAME, server.user);
curl_easy_setopt(curl, CURLOPT_PASSWORD, server.pass);
 
-   strbuf_addstr(, server.use_ssl ? "imaps://" : "imap://");
-   strbuf_addstr(, server.host);
+   if (srvc->tunnel) {
+   int fds[2];
+
+   setup_tunnel(srvc, fds);
+   curl_easy_setopt(curl, CURLOPT_OPENSOCKETFUNCTION, 
curl_tunnel_socket);
+   curl_easy_setopt(curl, CURLOPT_OPENSOCKETDATA, (unsigned 
long)fds[0]);
+   curl_easy_setopt(curl, CURLOPT_SOCKOPTFUNCTION, 
sockopt_callback);
+   /* Create a fake hostname to avoid resolution issue and in case
+* imap.host was not set */
+   strbuf_addstr(, "imap://localhost");
+   } else {
+   strbuf_addstr(, server.use_ssl ? "imaps://" : "imap://");
+   strbuf_addstr(, server.host);
+   }
+
if (!path.len || path.buf[path.len - 1] != '/')
strbuf_addch(, '/');
strbuf_addstr(, server.folder);
@@ -1570,12 +1603,12 @@ int cmd_main(int argc, const char **argv)
 
/* write it to the imap server */
 
-   if (server.tunnel)
-   return append_msgs_to_imap(, _msgs, total);
-
 #ifdef USE_CURL_FOR_IMAP_SEND
if (use_curl)
-   return curl_append_msgs_to_imap(, _msgs, total);
+#if LIBCURL_VERSION_NUM < 0x071505
+   if (!server.tunnel)
+#endif
+   return curl_append_msgs_to_imap(, _msgs, 
total);
 #endif
 
return append_msgs_to_imap(, _msgs, total);
-- 
2.14.0.3.gb4ff627ec.dirty

[RFC 1/3] imap-send: move tunnel setup to its own function

[Nicolas Morey-Chaisemartin]

Signed-off-by: Nicolas Morey-Chaisemartin <nico...@morey-chaisemartin.com>
---
 imap-send.c | 37 ++---
 1 file changed, 22 insertions(+), 15 deletions(-)

diff --git a/imap-send.c b/imap-send.c
index b2d0b849b..10f668eb7 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -926,6 +926,27 @@ static int auth_cram_md5(struct imap_store *ctx, struct 
imap_cmd *cmd, const cha
return 0;
 }
 
+static void setup_tunnel(struct imap_server_conf *srvc, int fds[2])
+{
+   struct child_process tunnel = CHILD_PROCESS_INIT;
+
+   imap_info("Starting tunnel '%s'... ", srvc->tunnel);
+
+   argv_array_push(, srvc->tunnel);
+   tunnel.use_shell = 1;
+   tunnel.in = -1;
+   tunnel.out = -1;
+   if (start_command())
+   die("cannot start proxy %s", srvc->tunnel);
+
+   fds[0] = tunnel.out;
+   fds[1] = tunnel.in;
+
+   imap_info("ok\n");
+
+   return;
+}
+
 static struct imap_store *imap_open_store(struct imap_server_conf *srvc, char 
*folder)
 {
struct credential cred = CREDENTIAL_INIT;
@@ -943,21 +964,7 @@ static struct imap_store *imap_open_store(struct 
imap_server_conf *srvc, char *f
/* open connection to IMAP server */
 
if (srvc->tunnel) {
-   struct child_process tunnel = CHILD_PROCESS_INIT;
-
-   imap_info("Starting tunnel '%s'... ", srvc->tunnel);
-
-   argv_array_push(, srvc->tunnel);
-   tunnel.use_shell = 1;
-   tunnel.in = -1;
-   tunnel.out = -1;
-   if (start_command())
-   die("cannot start proxy %s", srvc->tunnel);
-
-   imap->buf.sock.fd[0] = tunnel.out;
-   imap->buf.sock.fd[1] = tunnel.in;
-
-   imap_info("ok\n");
+   setup_tunnel(srvc, imap->buf.sock.fd);
} else {
 #ifndef NO_IPV6
struct addrinfo hints, *ai0, *ai;
-- 
2.14.0.3.gb4ff627ec.dirty

Re: [PATCHv2 3/4] imap_send: setup_curl: retreive credentials if not set in config file

[Nicolas Morey-Chaisemartin]

Le 08/08/2017 à 12:09, Martin Ågren a écrit :
> On 8 August 2017 at 09:48, Nicolas Morey-Chaisemartin
> <nico...@morey-chaisemartin.com> wrote:
>> Up to this point, the curl mode only supported getting the username
>> and password from the gitconfig file while the legacy mode could also
>> fetch them using the credential API.
>>
>> Signed-off-by: Nicolas Morey-Chaisemartin <nico...@morey-chaisemartin.com>
>> ---
>>  imap-send.c | 10 --
>>  1 file changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/imap-send.c b/imap-send.c
>> index 448a4a0b3..5e80edaff 100644
>> --- a/imap-send.c
>> +++ b/imap-send.c
>> @@ -1398,7 +1398,7 @@ static int append_msgs_to_imap(struct imap_server_conf 
>> *server,
>>  }
>>
>>  #ifdef USE_CURL_FOR_IMAP_SEND
>> -static CURL *setup_curl(struct imap_server_conf *srvc)
>> +static CURL *setup_curl(struct imap_server_conf *srvc, struct credential 
>> *cred)
> Hmm, yeah, that really did pollute the interface. :)
>
>>  {
>> CURL *curl;
>> struct strbuf path = STRBUF_INIT;
>> @@ -1411,6 +1411,7 @@ static CURL *setup_curl(struct imap_server_conf *srvc)
>> if (!curl)
>> die("curl_easy_init failed");
>>
>> +   server_fill_credential(, cred);
>> curl_easy_setopt(curl, CURLOPT_USERNAME, server.user);
>> curl_easy_setopt(curl, CURLOPT_PASSWORD, server.pass);
>>
>> @@ -1460,8 +1461,9 @@ static int curl_append_msgs_to_imap(struct 
>> imap_server_conf *server,
>> struct buffer msgbuf = { STRBUF_INIT, 0 };
>> CURL *curl;
>> CURLcode res = CURLE_OK;
>> +   struct credential cred = CREDENTIAL_INIT;
>>
>> -   curl = setup_curl(server);
>> +   curl = setup_curl(server, );
>> curl_easy_setopt(curl, CURLOPT_READDATA, );
>>
>> fprintf(stderr, "sending %d message%s\n", total, (total != 1) ? "s" 
>> : "");
>> @@ -1496,6 +1498,10 @@ static int curl_append_msgs_to_imap(struct 
>> imap_server_conf *server,
>> curl_easy_cleanup(curl);
>> curl_global_cleanup();
>>
>> +   if (res == CURLE_OK && cred.username)
>> +   credential_approve();
> Maybe a similar call to credential_reject() here? I guess all we'll
> know is that some sort of error happened, possibly credentials-related,
> possibly not. Just a thought.

Checking the doc, there is actually a CURLE_LOGIN_DENIED return code which 
means authentication failed.
I'll fix this in v3

Nicolas

[PATCHv2 4/4] imap-send: use curl by default

[Nicolas Morey-Chaisemartin]

Now that curl is enable by default, use the curl implementation
for imap too.
The goal is to validate feature parity between the legacy and
the curl implementation, deprecate thee legacy implementation
later on and in the long term, hopefully drop it altogether.

Signed-off-by: Nicolas Morey-Chaisemartin <nico...@morey-chaisemartin.com>
---
 imap-send.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/imap-send.c b/imap-send.c
index 5e80edaff..138d776f7 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -35,11 +35,11 @@ typedef void *SSL;
 #include "http.h"
 #endif
 
-#if defined(USE_CURL_FOR_IMAP_SEND) && defined(NO_OPENSSL)
-/* only available option */
+#if defined(USE_CURL_FOR_IMAP_SEND)
+/* Always default to curl if it's available. */
 #define USE_CURL_DEFAULT 1
 #else
-/* strictly opt in */
+/* We don't have curl, so continue to use the historical implementation */
 #define USE_CURL_DEFAULT 0
 #endif
 
-- 
2.14.0.rc1.16.gcc208c97c

[PATCHv2 2/4] imap-send: add wrapper to get server credentials if needed

[Nicolas Morey-Chaisemartin]

Signed-off-by: Nicolas Morey-Chaisemartin <nico...@morey-chaisemartin.com>
---
 imap-send.c | 34 --
 1 file changed, 20 insertions(+), 14 deletions(-)

diff --git a/imap-send.c b/imap-send.c
index 09f29ea95..448a4a0b3 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -926,6 +926,25 @@ static int auth_cram_md5(struct imap_store *ctx, struct 
imap_cmd *cmd, const cha
return 0;
 }
 
+static void server_fill_credential(struct imap_server_conf *srvc, struct 
credential *cred)
+{
+   if (srvc->user && srvc->pass)
+   return;
+
+   cred->protocol = xstrdup(srvc->use_ssl ? "imaps" : "imap");
+   cred->host = xstrdup(srvc->host);
+
+   cred->username = xstrdup_or_null(srvc->user);
+   cred->password = xstrdup_or_null(srvc->pass);
+
+   credential_fill(cred);
+
+   if (!srvc->user)
+   srvc->user = xstrdup(cred->username);
+   if (!srvc->pass)
+   srvc->pass = xstrdup(cred->password);
+}
+
 static struct imap_store *imap_open_store(struct imap_server_conf *srvc, char 
*folder)
 {
struct credential cred = CREDENTIAL_INIT;
@@ -1078,20 +1097,7 @@ static struct imap_store *imap_open_store(struct 
imap_server_conf *srvc, char *f
}
 #endif
imap_info("Logging in...\n");
-   if (!srvc->user || !srvc->pass) {
-   cred.protocol = xstrdup(srvc->use_ssl ? "imaps" : 
"imap");
-   cred.host = xstrdup(srvc->host);
-
-   cred.username = xstrdup_or_null(srvc->user);
-   cred.password = xstrdup_or_null(srvc->pass);
-
-   credential_fill();
-
-   if (!srvc->user)
-   srvc->user = xstrdup(cred.username);
-   if (!srvc->pass)
-   srvc->pass = xstrdup(cred.password);
-   }
+   server_fill_credential(srvc, );
 
if (srvc->auth_method) {
struct imap_cmd_cb cb;
-- 
2.14.0.rc1.16.gcc208c97c

[PATCHv2 1/4] imap-send: return with error if curl failed

[Nicolas Morey-Chaisemartin]

curl_append_msgs_to_imap always returned 0, whether curl failed or not.
Return a proper status so git imap-send will exit with an error code
if womething wrong happened.

Signed-off-by: Nicolas Morey-Chaisemartin <nico...@morey-chaisemartin.com>
---
 imap-send.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/imap-send.c b/imap-send.c
index b2d0b849b..09f29ea95 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -1490,7 +1490,7 @@ static int curl_append_msgs_to_imap(struct 
imap_server_conf *server,
curl_easy_cleanup(curl);
curl_global_cleanup();
 
-   return 0;
+   return res == CURLE_OK;
 }
 #endif
 
-- 
2.14.0.rc1.16.gcc208c97c

[PATCHv2 3/4] imap_send: setup_curl: retreive credentials if not set in config file

[Nicolas Morey-Chaisemartin]

Up to this point, the curl mode only supported getting the username
and password from the gitconfig file while the legacy mode could also
fetch them using the credential API.

Signed-off-by: Nicolas Morey-Chaisemartin <nico...@morey-chaisemartin.com>
---
 imap-send.c | 10 --
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/imap-send.c b/imap-send.c
index 448a4a0b3..5e80edaff 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -1398,7 +1398,7 @@ static int append_msgs_to_imap(struct imap_server_conf 
*server,
 }
 
 #ifdef USE_CURL_FOR_IMAP_SEND
-static CURL *setup_curl(struct imap_server_conf *srvc)
+static CURL *setup_curl(struct imap_server_conf *srvc, struct credential *cred)
 {
CURL *curl;
struct strbuf path = STRBUF_INIT;
@@ -1411,6 +1411,7 @@ static CURL *setup_curl(struct imap_server_conf *srvc)
if (!curl)
die("curl_easy_init failed");
 
+   server_fill_credential(, cred);
curl_easy_setopt(curl, CURLOPT_USERNAME, server.user);
curl_easy_setopt(curl, CURLOPT_PASSWORD, server.pass);
 
@@ -1460,8 +1461,9 @@ static int curl_append_msgs_to_imap(struct 
imap_server_conf *server,
struct buffer msgbuf = { STRBUF_INIT, 0 };
CURL *curl;
CURLcode res = CURLE_OK;
+   struct credential cred = CREDENTIAL_INIT;
 
-   curl = setup_curl(server);
+   curl = setup_curl(server, );
curl_easy_setopt(curl, CURLOPT_READDATA, );
 
fprintf(stderr, "sending %d message%s\n", total, (total != 1) ? "s" : 
"");
@@ -1496,6 +1498,10 @@ static int curl_append_msgs_to_imap(struct 
imap_server_conf *server,
curl_easy_cleanup(curl);
curl_global_cleanup();
 
+   if (res == CURLE_OK && cred.username)
+   credential_approve();
+   credential_clear();
+
return res == CURLE_OK;
 }
 #endif
-- 
2.14.0.rc1.16.gcc208c97c

[PATCHv2 0/4] imap-send: Fix and enable curl by default

[Nicolas Morey-Chaisemartin]

Changes since v1:

- Add patch fo fix return value of the curl_append_msgs_to_imap
- Patch #2: server_fill_credentials takes a credential struct as a parameter so 
they can be approved later
- Dropped the s/server/srvc/ cleanup (previous patch #3)
- Patch #4: Only use curl as the default if it's available

Nicolas Morey-Chaisemartin (4):
  imap-send: return with error if curl failed
  imap-send: add wrapper to get server credentials if needed
  imap_send: setup_curl: retreive credentials if not set in config file
  imap-send: use curl by default

 imap-send.c | 52 
 1 file changed, 32 insertions(+), 20 deletions(-)

Re: [PATCH 1/4] imap-send: add wrapper to get server credentials if needed

[Nicolas Morey-Chaisemartin]

Le 07/08/2017 à 21:42, Jeff King a écrit :
> On Mon, Aug 07, 2017 at 07:18:32PM +0200, Martin Ågren wrote:
>
 "cred.username" is checked further down, but now it will always be NULL,
 no?
>>> You're right I missed this.
>>> Not sure if this is needed though.
>>> From what I understand this means the username/password are store for the 
>>> next access to credential. but in the current state, there is only one.
>>> Maybe the credential_approved can be dropped ?
>> I'm no credentials-expert, but api-credentials.txt says this:
>>
>> "Credential helpers are programs executed by Git to fetch or save
>> credentials from and to long-term storage (where "long-term" is simply
>> longer than a single Git process; e.g., credentials may be stored
>> in-memory for a few minutes, or indefinitely on disk)."
>>
>> So the calls to approve/reject probably do matter in some scenarios.
> Right, this is important. credential_fill() may actually prompt the
> user, and we'd want to then pass along that credential for storage. Or
> the user may have changed their password, and the credential_reject() is
> the only thing that prevents us from trying an out-dated password over
> and over.
>
>> The current code is a bit non-obvious as we just discovered since it
>> duplicates the strings (for good reasons, I believe) and then still
>> refers to the originals (also for good reasons, I believe). I suppose
>> your new function could be called like
>>
>> server_fill_credential(, srvc);
>>
>> That should limit the impact of the change, but I'm not sure it's a
>> brilliant interface. Just my 2c.
> That would work. I'm also tempted to say that imap_server_conf could
> just store the "struct credential" inside it. We could even potentially
> drop its parallel user/pass fields to minimize confusion.
>
> Once upon a time imap-send was a fork of another program, which is why
> most of its code isn't well-integrated with Git. But I think at this
> point there's very little chance of merging changes back and forth
> between the two.
>
> On the other hand, if we're hoping to get rid of this code in favor of
> the curl-based approach, then it's not worth spending time on
> cosmetic refactoring, as long as it still behaves correctly in the
> interim.
>
> -Peff

Looking at the code, it seems the tunnel mode always uses the legacy imap 
approach.
This would have to be ported to curl and stabilized before dropping the legacy 
code.
In the meantime, it might be worth doing a bit of cleanup.
And as I said in patch 4, I have a current IMAP account where it works without 
curl but not with curl (for unknown reason yet).
Meaning legacy needs to stay for a while. But switching to curl as default to 
get out all the bugs (hence this slightly broken patch series)

I think it would make sense to get patch 1 (fixed), 2 and 4 in to really test 
out the curl implementation and take some time to prepare another serie with 
code cleanups: dropping imap_server_conf parameters, storing cred therem etc.)

Nicolas

Re: [PATCH 1/4] imap-send: add wrapper to get server credentials if needed

[Nicolas Morey-Chaisemartin]

Le 07/08/2017 à 18:30, Martin Ågren a écrit :
> On 7 August 2017 at 16:03, Nicolas Morey-Chaisemartin
> <nico...@morey-chaisemartin.com> wrote:
>> Signed-off-by: Nicolas Morey-Chaisemartin <nico...@morey-chaisemartin.com>
>> ---
>>  imap-send.c | 38 --
>>  1 file changed, 24 insertions(+), 14 deletions(-)
>>
>> diff --git a/imap-send.c b/imap-send.c
>> index b2d0b849b..38b3c817e 100644
>> --- a/imap-send.c
>> +++ b/imap-send.c
>> @@ -926,6 +926,29 @@ static int auth_cram_md5(struct imap_store *ctx, struct 
>> imap_cmd *cmd, const cha
>> return 0;
>>  }
>>
>> +static void server_fill_credential(struct imap_server_conf *srvc)
>> +{
>> +   struct credential cred = CREDENTIAL_INIT;
>> +
>> +   if (srvc->user && srvc->pass)
>> +   return;
>> +
>> +   cred.protocol = xstrdup(srvc->use_ssl ? "imaps" : "imap");
>> +   cred.host = xstrdup(srvc->host);
>> +
>> +   cred.username = xstrdup_or_null(srvc->user);
>> +   cred.password = xstrdup_or_null(srvc->pass);
>> +
>> +   credential_fill();
>> +
>> +   if (!srvc->user)
>> +   srvc->user = xstrdup(cred.username);
>> +   if (!srvc->pass)
>> +   srvc->pass = xstrdup(cred.password);
>> +
>> +   credential_clear();
>> +}
>> +
>>  static struct imap_store *imap_open_store(struct imap_server_conf *srvc, 
>> char *folder)
>>  {
>> struct credential cred = CREDENTIAL_INIT;
>> @@ -1078,20 +1101,7 @@ static struct imap_store *imap_open_store(struct 
>> imap_server_conf *srvc, char *f
>> }
>>  #endif
>> imap_info("Logging in...\n");
>> -   if (!srvc->user || !srvc->pass) {
>> -   cred.protocol = xstrdup(srvc->use_ssl ? "imaps" : 
>> "imap");
>> -   cred.host = xstrdup(srvc->host);
>> -
>> -   cred.username = xstrdup_or_null(srvc->user);
>> -   cred.password = xstrdup_or_null(srvc->pass);
>> -
>> -   credential_fill();
>> -
>> -   if (!srvc->user)
>> -   srvc->user = xstrdup(cred.username);
>> -   if (!srvc->pass)
>> -   srvc->pass = xstrdup(cred.password);
>> -   }
>> +   server_fill_credential(srvc);
>>
>> if (srvc->auth_method) {
>> struct imap_cmd_cb cb;
> "cred.username" is checked further down, but now it will always be NULL,
> no?

You're right I missed this.
Not sure if this is needed though.
>From what I understand this means the username/password are store for the next 
>access to credential. but in the current state, there is only one.
Maybe the credential_approved can be dropped ?

Nicolas

Re: [PATCH 4/4] imap-send: use curl by default

[Nicolas Morey-Chaisemartin]

Le 07/08/2017 à 18:37, Martin Ågren a écrit :
> On 7 August 2017 at 16:04, Nicolas Morey-Chaisemartin
> <nico...@morey-chaisemartin.com> wrote:
>> Signed-off-by: Nicolas Morey-Chaisemartin <nico...@morey-chaisemartin.com>
>> ---
>>  imap-send.c | 6 --
>>  1 file changed, 6 deletions(-)
>>
>> diff --git a/imap-send.c b/imap-send.c
>> index 90b8683ed..4ebc16437 100644
>> --- a/imap-send.c
>> +++ b/imap-send.c
>> @@ -35,13 +35,7 @@ typedef void *SSL;
>>  #include "http.h"
>>  #endif
>>
>> -#if defined(USE_CURL_FOR_IMAP_SEND) && defined(NO_OPENSSL)
>> -/* only available option */
>>  #define USE_CURL_DEFAULT 1
>> -#else
>> -/* strictly opt in */
>> -#define USE_CURL_DEFAULT 0
>> -#endif
>>
>>  static int verbosity;
>>  static int use_curl = USE_CURL_DEFAULT;
> So this is now basically "static int use_curl = 1;".
>
> Do we need a compile-time escape-hatch in case someone really needs
> to avoid curl, e.g., because they have a too old version? I suppose
> there is a conceptual difference between the "default", i.e., the value
> of USE_CURL_DEFAULT that is assigned to "use_curl", and the "default
> default", i.e., the value that is normally assigned to USE_CURL_DEFAULT.
>
> Martin

The curl code depends on USE_CURL_FOR_IMAP_SEND so even with use_curl == 1, it 
won't be an issue for people without curl (or old one).
I wasn't sure whether to drop the define or not and figure it might be worth 
keeping in case in change in the future for some reason.
I don't mind dropping it and hardcofing the default to 1

Also on a side note, I have a case where authentication works with --no-curl 
but fails withs --curl. Still trying to figure out why.

Nicolas

Re: [PATCH 3/4] imap_send: setup_curl: use server_conf parameter instead of the global variable

[Nicolas Morey-Chaisemartin]

Le 07/08/2017 à 18:34, Martin Ågren a écrit :
> On 7 August 2017 at 16:04, Nicolas Morey-Chaisemartin
> <nico...@morey-chaisemartin.com> wrote:
>> Signed-off-by: Nicolas Morey-Chaisemartin <nico...@morey-chaisemartin.com>
>> ---
>>  imap-send.c | 24 
>>  1 file changed, 12 insertions(+), 12 deletions(-)
>>
>> diff --git a/imap-send.c b/imap-send.c
>> index 682a06551..90b8683ed 100644
>> --- a/imap-send.c
>> +++ b/imap-send.c
>> @@ -1415,37 +1415,37 @@ static CURL *setup_curl(struct imap_server_conf 
>> *srvc)
>> if (!curl)
>> die("curl_easy_init failed");
>>
>> -   server_fill_credential();
>> -   curl_easy_setopt(curl, CURLOPT_USERNAME, server.user);
>> -   curl_easy_setopt(curl, CURLOPT_PASSWORD, server.pass);
>> +   server_fill_credential(srvc);
>> +   curl_easy_setopt(curl, CURLOPT_USERNAME, srvc->user);
>> +   curl_easy_setopt(curl, CURLOPT_PASSWORD, srvc->pass);
> Here you change the server_fill_credential-call that you just added.
> Maybe do this patch earlier, perhaps even as patch 1?
>
> I'm snipping lots of s/server/srvc/-changes... There's a less noisy
> way of addressing the fact that srvc is unused: dropping it. I'm not
> saying that's a good idea, but it could be considered, then explained
> why this approach is better. There are some other functions which
> access "server" directly, and some which take (and use!) a "srvc".
> Maybe make the whole file consistent?
>
> Martin
That's why I applied it after #2. I was not sure if this one made sense or not. 
And it  can be dropped with the rest of the series still applying.
I don't know what is the right approach here. Someone with more knowledge of 
why there is a mix of global variable and local can maybe help ?

Nicolas

[PATCH 1/4] imap-send: add wrapper to get server credentials if needed

[Nicolas Morey-Chaisemartin]

Signed-off-by: Nicolas Morey-Chaisemartin <nico...@morey-chaisemartin.com>
---
 imap-send.c | 38 --
 1 file changed, 24 insertions(+), 14 deletions(-)

diff --git a/imap-send.c b/imap-send.c
index b2d0b849b..38b3c817e 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -926,6 +926,29 @@ static int auth_cram_md5(struct imap_store *ctx, struct 
imap_cmd *cmd, const cha
return 0;
 }
 
+static void server_fill_credential(struct imap_server_conf *srvc)
+{
+   struct credential cred = CREDENTIAL_INIT;
+
+   if (srvc->user && srvc->pass)
+   return;
+
+   cred.protocol = xstrdup(srvc->use_ssl ? "imaps" : "imap");
+   cred.host = xstrdup(srvc->host);
+
+   cred.username = xstrdup_or_null(srvc->user);
+   cred.password = xstrdup_or_null(srvc->pass);
+
+   credential_fill();
+
+   if (!srvc->user)
+   srvc->user = xstrdup(cred.username);
+   if (!srvc->pass)
+   srvc->pass = xstrdup(cred.password);
+
+   credential_clear();
+}
+
 static struct imap_store *imap_open_store(struct imap_server_conf *srvc, char 
*folder)
 {
struct credential cred = CREDENTIAL_INIT;
@@ -1078,20 +1101,7 @@ static struct imap_store *imap_open_store(struct 
imap_server_conf *srvc, char *f
}
 #endif
imap_info("Logging in...\n");
-   if (!srvc->user || !srvc->pass) {
-   cred.protocol = xstrdup(srvc->use_ssl ? "imaps" : 
"imap");
-   cred.host = xstrdup(srvc->host);
-
-   cred.username = xstrdup_or_null(srvc->user);
-   cred.password = xstrdup_or_null(srvc->pass);
-
-   credential_fill();
-
-   if (!srvc->user)
-   srvc->user = xstrdup(cred.username);
-   if (!srvc->pass)
-   srvc->pass = xstrdup(cred.password);
-   }
+   server_fill_credential(srvc);
 
if (srvc->auth_method) {
struct imap_cmd_cb cb;

[PATCH 4/4] imap-send: use curl by default

[Nicolas Morey-Chaisemartin]

Signed-off-by: Nicolas Morey-Chaisemartin <nico...@morey-chaisemartin.com>
---
 imap-send.c | 6 --
 1 file changed, 6 deletions(-)

diff --git a/imap-send.c b/imap-send.c
index 90b8683ed..4ebc16437 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -35,13 +35,7 @@ typedef void *SSL;
 #include "http.h"
 #endif
 
-#if defined(USE_CURL_FOR_IMAP_SEND) && defined(NO_OPENSSL)
-/* only available option */
 #define USE_CURL_DEFAULT 1
-#else
-/* strictly opt in */
-#define USE_CURL_DEFAULT 0
-#endif
 
 static int verbosity;
 static int use_curl = USE_CURL_DEFAULT;
-- 
2.14.0.rc1.16.g87fcec1e8

[PATCH 3/4] imap_send: setup_curl: use server_conf parameter instead of the global variable

[Nicolas Morey-Chaisemartin]

Signed-off-by: Nicolas Morey-Chaisemartin <nico...@morey-chaisemartin.com>
---
 imap-send.c | 24 
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/imap-send.c b/imap-send.c
index 682a06551..90b8683ed 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -1415,37 +1415,37 @@ static CURL *setup_curl(struct imap_server_conf *srvc)
if (!curl)
die("curl_easy_init failed");
 
-   server_fill_credential();
-   curl_easy_setopt(curl, CURLOPT_USERNAME, server.user);
-   curl_easy_setopt(curl, CURLOPT_PASSWORD, server.pass);
+   server_fill_credential(srvc);
+   curl_easy_setopt(curl, CURLOPT_USERNAME, srvc->user);
+   curl_easy_setopt(curl, CURLOPT_PASSWORD, srvc->pass);
 
-   strbuf_addstr(, server.use_ssl ? "imaps://" : "imap://");
-   strbuf_addstr(, server.host);
+   strbuf_addstr(, srvc->use_ssl ? "imaps://" : "imap://");
+   strbuf_addstr(, srvc->host);
if (!path.len || path.buf[path.len - 1] != '/')
strbuf_addch(, '/');
-   strbuf_addstr(, server.folder);
+   strbuf_addstr(, srvc->folder);
 
curl_easy_setopt(curl, CURLOPT_URL, path.buf);
strbuf_release();
-   curl_easy_setopt(curl, CURLOPT_PORT, server.port);
+   curl_easy_setopt(curl, CURLOPT_PORT, srvc->port);
 
-   if (server.auth_method) {
+   if (srvc->auth_method) {
 #if LIBCURL_VERSION_NUM < 0x072200
warning("No LOGIN_OPTIONS support in this cURL version");
 #else
struct strbuf auth = STRBUF_INIT;
strbuf_addstr(, "AUTH=");
-   strbuf_addstr(, server.auth_method);
+   strbuf_addstr(, srvc->auth_method);
curl_easy_setopt(curl, CURLOPT_LOGIN_OPTIONS, auth.buf);
strbuf_release();
 #endif
}
 
-   if (!server.use_ssl)
+   if (!srvc->use_ssl)
curl_easy_setopt(curl, CURLOPT_USE_SSL, (long)CURLUSESSL_TRY);
 
-   curl_easy_setopt(curl, CURLOPT_SSL_VERIFYPEER, server.ssl_verify);
-   curl_easy_setopt(curl, CURLOPT_SSL_VERIFYHOST, server.ssl_verify);
+   curl_easy_setopt(curl, CURLOPT_SSL_VERIFYPEER, srvc->ssl_verify);
+   curl_easy_setopt(curl, CURLOPT_SSL_VERIFYHOST, srvc->ssl_verify);
 
curl_easy_setopt(curl, CURLOPT_READFUNCTION, fread_buffer);
 
-- 
2.14.0.rc1.16.g87fcec1e8

[PATCH 2/4] imap_send: setup_curl: prompt user for username/password if not set in config file

[Nicolas Morey-Chaisemartin]

Signed-off-by: Nicolas Morey-Chaisemartin <nico...@morey-chaisemartin.com>
---
 imap-send.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/imap-send.c b/imap-send.c
index 38b3c817e..682a06551 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -1415,6 +1415,7 @@ static CURL *setup_curl(struct imap_server_conf *srvc)
if (!curl)
die("curl_easy_init failed");
 
+   server_fill_credential();
curl_easy_setopt(curl, CURLOPT_USERNAME, server.user);
curl_easy_setopt(curl, CURLOPT_PASSWORD, server.pass);
 
-- 
2.14.0.rc1.16.g87fcec1e8

Re: [RFC] imap-send: escape backslash in password

[Nicolas Morey-Chaisemartin]

Le 04/08/2017 à 23:22, Jeff King a écrit :
> On Fri, Aug 04, 2017 at 02:18:13PM -0700, Junio C Hamano wrote:
>
>>> I also think it might be reasonable to scrap all of this ad-hoc imap
>>> code in favor of curl, which already gets these cases right. We already
>>> have a curl-backed implementation. I think we just left the old code out
>>> of conservatism. But it seems somewhat buggy and unmaintained, and I
>>> wonder if we aren't better off to simply encourage people to install
>>> curl.
>> That is a very attractive direction to go in, especially in the mid
>> to longer term.  Perhaps we declare that the ad-hoc hardcoded imap
>> is deprecated in the next cycle and drop the support by the end of
>> this year?
> That is fine by me. AFAIK, we already build the curl support by default
> when a sufficiently-advanced version of curl is available. So if there
> were feature-parity problems hopefully somebody would have reported it.
>
> I think the deprecation here can be relatively fast because we're not
> actually dropping support for any feature. We're just requiring that
> they install curl to get the same functionality (which might be
> inconvenient, but it's a heck of a lot less inconvenient than "there's
> no way to do what you want anymore").
>
> -Peff

There is at least one difference right now:
When using --curl, the username/password are loaded from the gitconfig file 
only.
When using the legacy imap interface, it goes through credential_fill which 
prompts for a password.

I don't think everyone is ready to store his email account password in a 
gitconfig file (I know I'm not).
I don't see why it couldn't be fixed. I'll give it a try tomorrow.

Also it probably make sense to have at least one release where --curl is the 
default. Until your mail I had no idea this option existed so I never tried it 
out.
Making it the default will make sure almost everyone is using it and that there 
is feature-parity.

But I agree it's probably safer and cleaner to let curl handle everything and 
drop the legacy stuff once it fully works.

Nicolas

Re: [RFC] imap-send: escape backslash in password

[Nicolas Morey-Chaisemartin]

Le 04/08/2017 à 21:09, Junio C Hamano a écrit :
> Nicolas Morey-Chaisemartin <nico...@morey-chaisemartin.com> writes:
>
>> Password containing backslashes need to have them doubled to have them 
>> properly interpreted by the imap server.
> Please wrap this into lines with reasonable lengths like 72 cols.

I haven't checked the coding style yet. This was a very quick try that I 
submitted to get some feedback on the approach.
WIll fix for next time though.

>
> Is the quoting rules documented somewhere?  If so, please also give
> a reference to it here.  RFC3501 "6.2.3 LOGIN Command" does not say
> much (other parts of the RFC may specify the rules that apply to
> arguments in general, but I didn't look for them).  Without such
> reference, it is hard to judge if this change is sufficient or even
> correct (in an extreme case, the IMAP server you are talking with
> that prompted you to make this change might be in violation).
>
> For example, FRC3501 "9. Formal Syntax" says that both "password"
> and "userid" are "astring"; it looks strange that the code with this
> patch only touches cred.password while sending cred.username as-is.

Didn't found a RFC doc on this. I hit the bug today and looking at the error 
message, found a few people who add the issue with different client and 
required escaping backslashes
It probably applies to the username (would be logicial that both string in the 
line are parsed the same way) not sure if backslashes are allowed in username 
though.
With password generator, they are more likely to be there.
But it wouldn't hurt to use the escape function for both.

>> +static char* imap_escape_password(const char *passwd)
> In our codebase, asterisk sticks to identifier, not typename.  I.e.
>
>   static char *imap_escape(...)
Will do. BTW, is there a checkpatch or similar for git ?
Scrolled quickly through the doc and did not see any reference.

>
>> +{
>> +const unsigned passwd_len = strlen(passwd);
>> +char *escaped = xmalloc(2 * passwd_len + 1);
>> +const char *passwd_cur = passwd;
>> +char *escaped_cur = escaped;
>> +
>> +do {
>> +char *next = strchr(passwd_cur, '\\');
>> +
>> +if (!next) {
>> +strcpy(escaped_cur, passwd_cur);
>> +} else {
>> +int len = next - passwd_cur + 1;
>> +
>> +memcpy(escaped_cur, passwd_cur, len);
>> +escaped_cur += len;
>> +next++;
>> +*(escaped_cur++) = '\\';
>> +}
>> +passwd_cur = next;
>> +} while(passwd_cur);
>> +
>> +return escaped;
>> +}
> I wonder if we should use strbuf here perhaps like so:
>
>   struct strbuf encoded = STRBUF_INIT;
>   const char *p;
>
>   for (p = passwd; *p; p++) {
>   if (need_bs_quote(*p))
>   strbuf_addch(, '\\');
>   strbuf_addch(, *p);
>   }
>   return strbuf_detach(, NULL);

I looked at the wrappers and wasn't sure if they were to be used for this (one 
of the main reason this is an RFC).
I guess it would make sense. I'm not familiar with git code, but is there other 
escape function of this kind that could be factor ?
Or the function is simple enough not to be worth it ?

Thanks

Nicolas

[RFC] imap-send: escape backslash in password

[Nicolas Morey-Chaisemartin]

Password containing backslashes need to have them doubled to have them properly 
interpreted by the imap server.

A password terminating with a blackslash used to trigger this error:
IMAP command 'LOGIN  ' returned response (BAD) - Missing '"'

Signed-off-by: Nicolas Morey-Chaisemartin <nico...@morey-chaisemartin.com>
---
 imap-send.c | 28 +++-
 1 file changed, 27 insertions(+), 1 deletion(-)

diff --git a/imap-send.c b/imap-send.c
index b2d0b849b..7fde6ec96 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -926,6 +926,32 @@ static int auth_cram_md5(struct imap_store *ctx, struct 
imap_cmd *cmd, const cha
return 0;
 }
 
+static char* imap_escape_password(const char *passwd)
+{
+   const unsigned passwd_len = strlen(passwd);
+   char *escaped = xmalloc(2 * passwd_len + 1);
+   const char *passwd_cur = passwd;
+   char *escaped_cur = escaped;
+
+   do {
+   char *next = strchr(passwd_cur, '\\');
+
+   if (!next) {
+   strcpy(escaped_cur, passwd_cur);
+   } else {
+   int len = next - passwd_cur + 1;
+
+   memcpy(escaped_cur, passwd_cur, len);
+   escaped_cur += len;
+   next++;
+   *(escaped_cur++) = '\\';
+   }
+   passwd_cur = next;
+   } while(passwd_cur);
+
+   return escaped;
+}
+
 static struct imap_store *imap_open_store(struct imap_server_conf *srvc, char 
*folder)
 {
struct credential cred = CREDENTIAL_INIT;
@@ -1090,7 +1116,7 @@ static struct imap_store *imap_open_store(struct 
imap_server_conf *srvc, char *f
if (!srvc->user)
srvc->user = xstrdup(cred.username);
if (!srvc->pass)
-   srvc->pass = xstrdup(cred.password);
+   srvc->pass = 
imap_escape_password(cred.password);
}
 
if (srvc->auth_method) {

[BUG] Wrong worktree path when using multiple worktree

[Nicolas Morey-Chaisemartin]

Hi,

There seem to be an issue with the path computed for a worktree when multiple 
worktree were created (using git worktree)
In my Setup, I have 3 repos:
A/repo (main One)
A/repo-patches (worktree, using branch dev)
B/repo (worktree, using branch next)

I'm working in A/repo-patches an run:
$ git checkout next
fatal: 'next' is already checked out at 'A/repo-patches'

Which is partially true but not completely.
I looked a bit in the code and found that the issue comes from here 
(get_linked_worktree):
if (!strbuf_strip_suffix(_path, "/.git")) {
strbuf_reset(_path);
strbuf_addstr(_path, absolute_path("."));
strbuf_strip_suffix(_path, "/.");
}
Because it wrongfully assumes that I am in the linked worktree.
I checked in the .git/worktree files and couldn't see anything that actually 
points to where the repo are setup.

Nicolas
--
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: Unexpected behaviour after removing submodule

[Nicolas Morey-Chaisemartin]

On 26/10/12 00:47, Jens Lehmann wrote:

Am 25.10.2012 17:06, schrieb Nicolas Morey-Chaisemartin:

At work, we use a lot of submodules (several levels of submodules actually).
As we also work with development branches, we use scripts to resync the whole 
checked-out tree (mainly in automated integration)

We recently run across an issue where a branch (dev) contained a submodule 
while it had been removed in master and the files were imported in a 
subdirectory with the same name (probably using git-subtree).

Basically:

On dev:
* top/refs(submodule)/file1
On master:
* top/refs(dir)/file1

Outside the fact that it is quite hard to move from one branch to the other 
while having a perfectly clean tree checked out underneath, we manage to end up 
into a weird (invalid) state
that was neither clearly described nor easy to cleanup (using standard git 
clean/checkout/reset commands).

snipped example without submodules


The issue is, there is no way from folder2 to see that something wrong is 
going on inside your tree!
As we manage to reach this state using only standard git commands (I'll try to 
reproduce it) with submodules, and this being part of an automated flow, it is 
quite worrying.
We may actually be committing in the wrong repo and pushing the wrong things 
around.

Is there or should there be a way to look for such issues? And is this an 
expected behaviour?

Unfortunately this is how things work at the moment. If you remove a
submodule its work tree will currently stay around nonetheless. And
when you replace it with a directory containing files tracked by git,
things start to get really weird when you do checkouts crossing that
conversion.

But the solution to that problem is coming closer. The first step was
to move the .git directory out of the submodule's work tree, so that
we are able to remove it without loosing any history. The next step
will be to enable git to remove and re-add submodules on checkout. The
remove a submodule part already works in my - still experimental -
recursive_submodule_checkout branch at my github repo. The thing that
is still missing - apart from tests ;) - is that checkout has to learn
to look into the to-be-checked-out .gitmodules to be able to populate
a re-appearing submodule. I plan to add that for git fetch first (so
it can fetch submodules the user showed interest in but which aren't
currently checked out) and after that I'll reuse it for checkout.

But that's still some work to do and will take some time ...
--
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



That's what I figured. I just wished there was at least a way to see that 
there's a .git directory that does not really belong.
To check that now we'd have to do a git-ls tree and look for .git that 
shouldn't be here but with the amount of file we have, a shell script doing 
that would take ages !

We'll wait for a more submodule aware version of checkout then !

Thanks

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

Unexpected behaviour after removing submodule

[Nicolas Morey-Chaisemartin]

Hi,

At work, we use a lot of submodules (several levels of submodules actually).
As we also work with development branches, we use scripts to resync the whole 
checked-out tree (mainly in automated integration)

We recently run across an issue where a branch (dev) contained a submodule 
while it had been removed in master and the files were imported in a 
subdirectory with the same name (probably using git-subtree).

Basically:

On dev:
* top/refs(submodule)/file1
On master:
* top/refs(dir)/file1

Outside the fact that it is quite hard to move from one branch to the other 
while having a perfectly clean tree checked out underneath, we manage to end up 
into a weird (invalid) state
that was neither clearly described nor easy to cleanup (using standard git 
clean/checkout/reset commands).

While I cannot explain how we got in this state, here is a small test-case that 
produce the same results:
--
mkdir folder1
cd folder1
git init
echo Ooops  file
git add file
git commit -m Add file
cd ..
mkdir folder2
cd folder2
git init
mkdir folder1
echo Ooops  folder1/file
git add folder1/file
git commit -m Add file again
git checkout -b branch
cp -R ../folder1/.git ./folder1
--
The 'cp' just seems pointless but with the submodule described as above we 
manage to end up in a similar state.
In this state, when being in folder2, git status reports nothing. Dev branch is 
checked out and everything looks great.

However if you change dir to folder2/folder1, while still being inside folder2, 
git thinks (because of the .git dir) that you are actually on master branch of 
folder1 repository.
Which mean that if you happen to commit from a subdirectory, you may easily 
end-up committing in another repository than the one expected.

The issue is, there is no way from folder2 to see that something wrong is 
going on inside your tree!
As we manage to reach this state using only standard git commands (I'll try to 
reproduce it) with submodules, and this being part of an automated flow, it is 
quite worrying.
We may actually be committing in the wrong repo and pushing the wrong things 
around.

Is there or should there be a way to look for such issues? And is this an 
expected behaviour?

Thanks in advance

Nicolas



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