Re: [PATCH v8 0/9] connect: various cleanups

[Mike Hommey]

On Sat, May 28, 2016 at 07:02:01AM +0200, Torsten Bögershausen wrote:
> On 27.05.16 23:59, Mike Hommey wrote:
> > On Fri, May 27, 2016 at 04:24:20PM +0200, Torsten Bögershausen wrote:
> >> On 27.05.16 04:27, Mike Hommey wrote:
> >>> Changes from v7:
> >>> - Fixed comments.
> >>>
> >>> Mike Hommey (9):
> All in all, a great improvement.
> 2 things left.
> - The comment about [] is now stale, isn't it ?

No, it's still valid at this point, that's what the 2nd argument to
host_end (0) does.

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

Re: [PATCH v8 0/9] connect: various cleanups

[Torsten Bögershausen]

On 27.05.16 23:59, Mike Hommey wrote:
> On Fri, May 27, 2016 at 04:24:20PM +0200, Torsten Bögershausen wrote:
>> On 27.05.16 04:27, Mike Hommey wrote:
>>> Changes from v7:
>>> - Fixed comments.
>>>
>>> Mike Hommey (9):
All in all, a great improvement.
2 things left.
- The comment about [] is now stale, isn't it ?
- The legacy support should only be active for ssh, and not
  be used by other schemes.



diff --git a/connect.c b/connect.c
index 076ae09..79b8dae 100644
--- a/connect.c
+++ b/connect.c
@@ -618,10 +618,6 @@ static enum protocol parse_connect_url(const char 
*url_orig, char **ret_user,
}
}
 
-   /*
-* Don't do destructive transforms as protocol code does
-* '[]' unwrapping in get_host_and_port()
-*/
end = host_end(, 0);
 
if (protocol == PROTO_LOCAL)
@@ -673,7 +669,7 @@ static enum protocol parse_connect_url(const char 
*url_orig, char **ret_user,
 * "host:port" and NULL.
 * To support this undocumented legacy we still need to split the port.
 */
-   if (!port)
+   if (!port && protocol == PROTO_SSH)
port = get_port(host);
 
*ret_user = user ? xstrdup(user) : NULL;

--
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: [RFC] fast-import: invalidate pack_id references after loosening

[Eric Wong]

Jeff King  wrote:
> On Thu, May 26, 2016 at 08:02:36AM +, Eric Wong wrote:
> 
> > > That's probably OK in practice, as I think the actual pack-write already
> > > does a linear walk of the object table to generate the pack index. So
> > > presumably nobody checkpoints often enough for it to be a problem. And
> > > the solution would be to keep a separate list of pointers to objects for
> > > the current pack-id, which would trivially fix both this case and the
> > > one in create_index().  So we can punt on it until somebody actually
> > > runs into it, I think.
> > 
> > I might checkpoint that much and run into the problem soon :)
> > Too tired now; maybe in a day or two I'll be able to make sense
> > of C again :x

OK, I guess I was too tired and not thinking clearly.

I now figure it's not worth it to checkpoint frequently and have
a very-long-lived fast-import process.  The object table and
marks set will grow indefinitely, so forking off another gfi
with a new set of marks is better for my case[1], anyways.


Junio: I think my RFC can go into pu as-is and replace
Jeff's object_table discarding patch.


[1] inotify watching a Maildir + fast-import

> In theory the list of objects in the allocation pool are contiguous for
> a particular pack. So you could break out of the double-loop at the
> first non-matching object you see:
> 
> diff --git a/fast-import.c b/fast-import.c
> index 83558dc..f214bd3 100644
> --- a/fast-import.c
> +++ b/fast-import.c
> @@ -900,10 +900,13 @@ static const char *create_index(void)
>   c = idx;
>   for (o = blocks; o; o = o->next_pool)
>   for (e = o->next_free; e-- != o->entries;)
>   if (pack_id == e->pack_id)
>   *c++ = >idx;
> + else
> + goto done;
> +done:
>   last = idx + object_count;
>   if (c != last)
>   die("internal consistency error creating the index");
>  
>   tmpfile = write_idx_file(NULL, idx, object_count, _idx_opts, 
> pack_data->sha1);
> 
> But that seems to break some of the tests, so I think there is some case
> which does not match my theory. :)

Yes, it's probably because duplicate objects from the object
store will create entries with e->pack_id == MAX_PACK_ID

> Another strategy is to note that we cross-check against object_count
> here. So you could simply break out of the loop when we have found
> object_count matches.

I'm worried that could fail to detect bugs which should've led
us to die of the internal consistency error.


What we could probably do more safely is limit the scan to only
recent object pools (new ones since the previous create_index call
and the last one scanned).

This passes tests, but I could be missing something:

--- a/fast-import.c
+++ b/fast-import.c
@@ -926,14 +926,16 @@ static const char *create_index(void)
struct pack_idx_entry **idx, **c, **last;
struct object_entry *e;
struct object_entry_pool *o;
+   static struct object_entry_pool *last_seen;
 
/* Build the table of object IDs. */
ALLOC_ARRAY(idx, object_count);
c = idx;
-   for (o = blocks; o; o = o->next_pool)
+   for (o = blocks; o; o = (o == last_seen) ? NULL : o->next_pool)
for (e = o->next_free; e-- != o->entries;)
if (pack_id == e->pack_id)
*c++ = >idx;
+   last_seen = blocks;
last = idx + object_count;
if (c != last)
die("internal consistency error creating the index");
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v8 0/9] connect: various cleanups

[Mike Hommey]

On Fri, May 27, 2016 at 04:24:20PM +0200, Torsten Bögershausen wrote:
> On 27.05.16 04:27, Mike Hommey wrote:
> > Changes from v7:
> > - Fixed comments.
> > 
> > Mike Hommey (9):
> >   connect: document why we sometimes call get_port after
> > get_host_and_port
> >   connect: call get_host_and_port() earlier
> >   connect: re-derive a host:port string from the separate host and port
> > variables
> >   connect: make parse_connect_url() return separated host and port
> >   connect: group CONNECT_DIAG_URL handling code
> >   connect: make parse_connect_url() return the user part of the url as a
> > separate value
> >   connect: change the --diag-url output to separate user and host
> >   connect: actively reject git:// urls with a user part
> >   connect: move ssh command line preparation to a separate function
> > 
> >  connect.c | 235 
> > +-
> >  t/t5500-fetch-pack.sh |  42 ++---
> >  2 files changed, 170 insertions(+), 107 deletions(-)
> > 
> Is the whole series somewhere on a public repo ?

Is it now :)

> No major remarks so far, if possible, I would like
> to have a look at the resulting connect.c

https://github.com/glandium/git/blob/connect/connect.c

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

Re: [PATCH] Documentation: add instructions to help setup gmail 2FA

[Mike Rappazzo]

On Fri, May 27, 2016 at 5:06 PM, Junio C Hamano  wrote:
> Michael Rappazzo  writes:
>
>> For those who use two-factor authentication with gmail, git-send-email
>> will not work unless it is setup with an app-specific password. The
>> example for setting up git-send-email for use with gmail will now
>> include information on generating and storing the app-specific password.
>> ---
>
> Sounds good.  We'd need your sign-off in order to be able to use
> this patch, though.
>

Signed-off-by: Michael Rappazzo 


>>  Documentation/git-send-email.txt | 13 +
>>  1 file changed, 13 insertions(+)
>>
>> diff --git a/Documentation/git-send-email.txt 
>> b/Documentation/git-send-email.txt
>> index 771a7b5..edbba3a 100644
>> --- a/Documentation/git-send-email.txt
>> +++ b/Documentation/git-send-email.txt
>> @@ -450,6 +450,19 @@ edit ~/.gitconfig to specify your account settings:
>>   smtpUser = yourn...@gmail.com
>>   smtpServerPort = 587
>>
>> +If you have multifactor authentication setup on your gmail acocunt, you will
>> +need to generate an app-specific password for use with 'git send-email'. 
>> Visit
>> +https://security.google.com/settings/security/apppasswords to setup an
>> +app-specific password.  Once setup, you can store it with the credentials
>> +helper:
>> +
>> + $ git credential fill
>> + protocol=smtp
>> + host=smtp.gmail.com
>> + username=youn...@gmail.com
>> + password=app-password
>> +
>> +
>>  Once your commits are ready to be sent to the mailing list, run the
>>  following commands:
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Documentation: add instructions to help setup gmail 2FA

[Junio C Hamano]

Michael Rappazzo  writes:

> For those who use two-factor authentication with gmail, git-send-email
> will not work unless it is setup with an app-specific password. The
> example for setting up git-send-email for use with gmail will now
> include information on generating and storing the app-specific password.
> ---

Sounds good.  We'd need your sign-off in order to be able to use
this patch, though.

>  Documentation/git-send-email.txt | 13 +
>  1 file changed, 13 insertions(+)
>
> diff --git a/Documentation/git-send-email.txt 
> b/Documentation/git-send-email.txt
> index 771a7b5..edbba3a 100644
> --- a/Documentation/git-send-email.txt
> +++ b/Documentation/git-send-email.txt
> @@ -450,6 +450,19 @@ edit ~/.gitconfig to specify your account settings:
>   smtpUser = yourn...@gmail.com
>   smtpServerPort = 587
>  
> +If you have multifactor authentication setup on your gmail acocunt, you will
> +need to generate an app-specific password for use with 'git send-email'. 
> Visit
> +https://security.google.com/settings/security/apppasswords to setup an
> +app-specific password.  Once setup, you can store it with the credentials
> +helper:
> +
> + $ git credential fill
> + protocol=smtp
> + host=smtp.gmail.com
> + username=youn...@gmail.com
> + password=app-password
> +
> +
>  Once your commits are ready to be sent to the mailing list, run the
>  following commands:
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Require 0 context lines in git-blame algorithm

[Junio C Hamano]

David Kastrup  writes:

> Previously, the core part of git blame -M required 1 context line.
> There is no rationale to be found in the code (one guess would be that
> the old blame algorithm was unable to deal with immediately adjacent
> regions), and it causes artifacts like discussed in the thread
> 

The only thing that remotely hints why we thought a non-zero context
was a good idea was this:

http://thread.gmane.org/gmane.comp.version-control.git/28336/focus=28580

in which I said:

 | we may need to use a handful surrounding context lines for
 | better identification of copy source by the "ciff" algorithm but
 | that is a minor implementation detail.

But I do not think the amount of context affects the quality of the
match.  So it could be that it was completely a misguided attempt
since the very beginning, cee7f245 (git-pickaxe: blame rewritten.,
2006-10-19), which allowed the caller to specify context when
calling compare_buffer(), the function that corresponds to
diff_hunks() in today's code.

> ---
>  builtin/blame.c | 7 +++
>  1 file changed, 3 insertions(+), 4 deletions(-)

I totally forgot about the discussion around $gmane/255289; thanks
for bringing this back again.

As usual, we'd need your sign-off to use this patch.

Thanks.


> diff --git a/builtin/blame.c b/builtin/blame.c
> index 21f42b0..a3f6874 100644
> --- a/builtin/blame.c
> +++ b/builtin/blame.c
> @@ -134,7 +134,7 @@ struct progress_info {
>   int blamed_lines;
>  };
>  
> -static int diff_hunks(mmfile_t *file_a, mmfile_t *file_b, long ctxlen,
> +static int diff_hunks(mmfile_t *file_a, mmfile_t *file_b,
> xdl_emit_hunk_consume_func_t hunk_func, void *cb_data)
>  {
>   xpparam_t xpp = {0};
> @@ -142,7 +142,6 @@ static int diff_hunks(mmfile_t *file_a, mmfile_t *file_b, 
> long ctxlen,
>   xdemitcb_t ecb = {NULL};
>  
>   xpp.flags = xdl_opts;
> - xecfg.ctxlen = ctxlen;
>   xecfg.hunk_func = hunk_func;
>   ecb.priv = cb_data;
>   return xdi_diff(file_a, file_b, , , );
> @@ -980,7 +979,7 @@ static void pass_blame_to_parent(struct scoreboard *sb,
>   fill_origin_blob(>revs->diffopt, target, _o);
>   num_get_patch++;
>  
> - if (diff_hunks(_p, _o, 0, blame_chunk_cb, ))
> + if (diff_hunks(_p, _o, blame_chunk_cb, ))
>   die("unable to generate diff (%s -> %s)",
>   oid_to_hex(>commit->object.oid),
>   oid_to_hex(>commit->object.oid));
> @@ -1129,7 +1128,7 @@ static void find_copy_in_blob(struct scoreboard *sb,
>* file_p partially may match that image.
>*/
>   memset(split, 0, sizeof(struct blame_entry [3]));
> - if (diff_hunks(file_p, _o, 1, handle_split_cb, ))
> + if (diff_hunks(file_p, _o, handle_split_cb, ))
>   die("unable to generate diff (%s)",
>   oid_to_hex(>commit->object.oid));
>   /* remainder, if any, all match the preimage */
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] Documentation: add instructions to help setup gmail 2FA

[Michael Rappazzo]

For those who use two-factor authentication with gmail, git-send-email
will not work unless it is setup with an app-specific password. The
example for setting up git-send-email for use with gmail will now
include information on generating and storing the app-specific password.
---
 Documentation/git-send-email.txt | 13 +
 1 file changed, 13 insertions(+)

diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
index 771a7b5..edbba3a 100644
--- a/Documentation/git-send-email.txt
+++ b/Documentation/git-send-email.txt
@@ -450,6 +450,19 @@ edit ~/.gitconfig to specify your account settings:
smtpUser = yourn...@gmail.com
smtpServerPort = 587
 
+If you have multifactor authentication setup on your gmail acocunt, you will
+need to generate an app-specific password for use with 'git send-email'. Visit
+https://security.google.com/settings/security/apppasswords to setup an
+app-specific password.  Once setup, you can store it with the credentials
+helper:
+
+   $ git credential fill
+   protocol=smtp
+   host=smtp.gmail.com
+   username=youn...@gmail.com
+   password=app-password
+
+
 Once your commits are ready to be sent to the mailing list, run the
 following commands:
 
-- 
2.8.0

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

Re: [PATCH] log: document the --decorate=auto option

[Junio C Hamano]

Thanks, both.
--
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: [WIP-PATCH 0/2] send-email: refactor the email parser loop

[Eric Wong]

Samuel GROOT  wrote:
> While working on the new option `quote-email`, we needed to parse an
> email file. Since the work is already done, but the parsing and data
> processing are in the same loop, we wanted to refactor the parser, to
> make the code more maintainable.

Thank you for doing this work :)

> This is still WIP, and one of the main issue (and we need your
> advice on that), is that around 30 tests will fail, and most of them
> are false-negatives: to pass, they rely on the format of what is
> displayed by `git send-email`, not only data.
> 
> 
> For example, several tests will fail because they do a strict compare
> between `git send-email`'s output and:
> 
>(mbox) Adding cc: A  from line 'Cc: 
> A, One'
>(mbox) Adding cc: One  from line 'Cc: 
> A, One'
> 
> Though `git send-email` now outputs something like:
> 
>(mbox) Adding cc: A  from line 'Cc: 
> A'
>(mbox) Adding cc: One  from line 'Cc: 
> One'
I actually like neither, and would prefer something shorter:

(mbox) Adding cc: A  from Cc: header
(mbox) Adding cc: One  from Cc: header
(mbox) Adding cc: SoB  from Signed-off-by: trailer

That way, there's no redundant addresses in each line and less
likely to wrap.

But I actually never noticed these lines in the past since they
scrolled off my terminal :x

Since the headers are already shown after those lines, it's
redundant to have the entire line.  And we could add
trailers after the headers (with a blank line to delimit):

# existing header output:
From: F 
Cc: A , One 
Subject: foo

# new trailer output
Signed-off-by: SoB 
Acked-by: ack 

> We can think of several solutions:
> 
>1. Modify the parser, to give the script the knowledge of the exact
>   line the data came from.
> 
>2. Hack the tests: modify the script using the parser to artificially
>   recreate the supposedly parsed line.
>   (e.g. with a list.join(', ')-like function)
> 
>3. Modify the tests to replace exact cmp by greps.
> 
> 
> IMHO, we should consider option 3, since the tests should rely on data
> rather than exact outputs. It also makes the tests more maintainable,
> in the sense that they will be more resilient to future output
> modifications.

Agreed on 3.

I am not sure if anybody outside of tests parses the stdout of
send-email.  It's certainly a porcelain and I don't think
stdout needs to be stable, and maybe the output in
question should go to stderr since it could be considered
debug output.

But I could be wrong...
--
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: t7610-mergetool.sh test failure

[Jeff King]

On Fri, May 27, 2016 at 12:58:15PM -0700, Junio C Hamano wrote:

> On Fri, May 27, 2016 at 11:24 AM, Jeff King  wrote:
> > Which perhaps shows that maybe some people would
> > see a performance regression if we moved from /tmp to a slower
> > filesystem (e.g., especially people whose git clone is on NFS or
> > something).
> 
> Yup, but they would be using "--root" already if NFS bothers them;
> having TMPDIR pointing somewhere in it would not hurt them, I
> would think.

Yeah, but that is not quite the same as "in the source directory" (i.e.,
they would not notice via "git status" later if cruft was left in their
--root path). But I guess people not using "--root" would, and that may
be good enough.

-Peff
--
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: t7610-mergetool.sh test failure

[Junio C Hamano]

On Fri, May 27, 2016 at 11:24 AM, Jeff King  wrote:
> Which perhaps shows that maybe some people would
> see a performance regression if we moved from /tmp to a slower
> filesystem (e.g., especially people whose git clone is on NFS or
> something).

Yup, but they would be using "--root" already if NFS bothers them;
having TMPDIR pointing somewhere in it would not hurt them, I
would think.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 18/22] tests: use test_i18n* functions to suppress false positives

[Junio C Hamano]

Vasco Almeida  writes:

> Às 17:08 de 27-05-2016, Junio C Hamano escreveu:
>> Vasco Almeida  writes:
>> 
>>> diff --git a/t/t1307-config-blob.sh b/t/t1307-config-blob.sh
>>> index 3c6791e..4079fef 100755
>>> --- a/t/t1307-config-blob.sh
>>> +++ b/t/t1307-config-blob.sh
>>> @@ -64,7 +64,7 @@ test_expect_success 'parse errors in blobs are properly 
>>> attributed' '
>>>  
>>> # just grep for our token as the exact error message is likely to
>>> # change or be internationalized
>>> -   grep "HEAD:config" err
>>> +   test_i18ngrep "HEAD:config" err
>>>  '
>> 
>> It is unfortunate that the gettext-poison mechanism is too dumb to
>> notice that it is clobbering a format string with placeholders and
>> leave them intact, which is what the comment above this change is
>> wishing for.  I do not think we will be granting that wish any time
>> soon, so perhaps remove the two lines while we are at it?
>> 
> Yes, that was more or less what I thought when read that comment, but
> forgot about it after.
> I'll remove that comment in the next re-roll. Also, because it can be
> deceiving for someone not entirely sure how does gettext poison,
> translations and tests work together when test are run.
>
> For the record, test are run under C locale, so translations don't
> matter in tests. Under GETTEXT_POISON build, all strings interpolated by
> gettext (for instance _("a message from git") in C language) are
> replaced by garbage string "# GETTEXT POISON #".

Yes, for the record (as I know you know I know you know this), if we
ever granted the wish the comment expressed, we do not have to use
test_i18ngrep but instead can use grep, following the reasoning in
the comment "Even when running under poison locale, we must have
HEAD:config in the output".

It is unfortunate that poison locale is not that smart.  If it were,
we can lose a lot of test_i18ngrep, not only from here.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 18/22] tests: use test_i18n* functions to suppress false positives

[Vasco Almeida]

Às 17:08 de 27-05-2016, Junio C Hamano escreveu:
> Vasco Almeida  writes:
> 
>> diff --git a/t/t1307-config-blob.sh b/t/t1307-config-blob.sh
>> index 3c6791e..4079fef 100755
>> --- a/t/t1307-config-blob.sh
>> +++ b/t/t1307-config-blob.sh
>> @@ -64,7 +64,7 @@ test_expect_success 'parse errors in blobs are properly 
>> attributed' '
>>  
>>  # just grep for our token as the exact error message is likely to
>>  # change or be internationalized
>> -grep "HEAD:config" err
>> +test_i18ngrep "HEAD:config" err
>>  '
> 
> It is unfortunate that the gettext-poison mechanism is too dumb to
> notice that it is clobbering a format string with placeholders and
> leave them intact, which is what the comment above this change is
> wishing for.  I do not think we will be granting that wish any time
> soon, so perhaps remove the two lines while we are at it?
> 
Yes, that was more or less what I thought when read that comment, but
forgot about it after.
I'll remove that comment in the next re-roll. Also, because it can be
deceiving for someone not entirely sure how does gettext poison,
translations and tests work together when test are run.

For the record, test are run under C locale, so translations don't
matter in tests. Under GETTEXT_POISON build, all strings interpolated by
gettext (for instance _("a message from git") in C language) are
replaced by garbage string "# GETTEXT POISON #".
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] t6030: explicitly test for bisection cleanup

[Pranit Bauva]

Hey Christian,

On Sat, May 28, 2016 at 12:30 AM, Christian Couder
 wrote:
> On Fri, May 27, 2016 at 7:57 PM, Pranit Bauva  wrote:
>>
>> Anyone any comments?
>
> Maybe you could add this patch to, or squash it into, the patch that
> convert bisect_clean_state to C.

Sure!

Regards,
Pranit Bauva

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

Re: [PATCH] t6030: explicitly test for bisection cleanup

[Christian Couder]

On Fri, May 27, 2016 at 7:57 PM, Pranit Bauva  wrote:
>
> Anyone any comments?

Maybe you could add this patch to, or squash it into, the patch that
convert bisect_clean_state to C.

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

Re: [PATCH v4 2/2] rev-parse: fix some options when executed from subpath of main tree

[Junio C Hamano]

Mike Hommey  writes:

> On Thu, May 26, 2016 at 07:19:16AM -0400, Michael Rappazzo wrote:
>> Executing `git-rev-parse` with `--git-common-dir`, `--git-path `,
>> or `--shared-index-path` from the root of the main worktree results in
>> a relative path to the git dir.
>> 
>> When executed from a subdirectory of the main tree, it can incorrectly
>> return a path which starts 'sub/path/.git'.  Change this to return the
>> proper relative path to the git directory.
>> 
>> Related tests marked to expect failure are updated to expect success
>
> As mentioned previously (but late in the thread), I don't get why this
> one case of --git-common-dir should not return the same thing as
> --git-dir, which is an absolute directory. Especially when there is
> other flag telling you whether you are in the main or another worktree,
> so comparing the output for --git-dir and --git-common-dir is the
> easiest way to do so, but then you have to normalize them on their own
> because git returns different values pointing to the same directory.

Sounds like a sensible line of thought.

A possible/plausible counter-argument from Michael's side that would
be equally sensible might run along the lines of:

An expected use of "git rev-parse --commit-dir" is to store the
output in $GIT_DIR/$X so that the layout the worktree machinery
expects can be set up by scripted Porcelains without using "git
worktree".  Making the value stored in $GIT_DIR/$X relative to
$Y would help for such and such reasons.

While making it easier to build a competing UI like that is a
sensible goal, I do not think of what that $X or $Y are, and I do
not think of what that "such and such reasons" are, either.

And the cost of having to compare absolute --git-dir output with
relative --git-common-dir (i.e. the justification for Mike's
proposal to make --git-common-dir absolute) and the cost of having
to turn absolute output from --git-common-dir to a path relative to
$Y (i.e. the justification of making it relative in the hypothetical
counter-argument) would be about the same, so it does not sound very
compelling after all.


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

Re: [PATCH] log: document the --decorate=auto option

[Marc Branchaud]

On 2016-05-27 11:56 AM, Ramsay Jones wrote:


Signed-off-by: Ramsay Jones 
---

Hi Junio,

While reading an email from Linus earlier (RFC: dynamic "auto" date formats),
I noticed that log.decorate was being set to 'auto'. Since I didn't recall
that setting (even if it's easy to guess), I went looking for the documentation 
...

This should probably be marked RFC, since I haven't checked that the formatting
is OK (I don't have the documentation toolchain installed these days).

ATB,
Ramsay Jones

  Documentation/config.txt  | 5 -
  Documentation/git-log.txt | 8 +---
  2 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 53f00db..0707b3b 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1956,7 +1956,10 @@ log.decorate::
command. If 'short' is specified, the ref name prefixes 'refs/heads/',
'refs/tags/' and 'refs/remotes/' will not be printed. If 'full' is
specified, the full ref name (including prefix) will be printed.
-   This is the same as the log commands '--decorate' option.
+   If 'auto' is specified, then if the output is going to a terminal,
+   the ref names are shown as if 'short' were given, otherwise no ref
+   names are shown. This is the same as the log commands '--decorate'


s/commands/command's/

M.


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

Re: [PATCH] add: add --chmod=+x / --chmod=-x options

[Junio C Hamano]

Johannes Schindelin  writes:

> I wonder, however, whether it would be "cleaner" to simply make this an
> OPT_STRING and perform the validation after the option parsing.

Yes, I think I touched on this in my comments in a bit more detail.

> Hmm. This change uses up 2 out of 31 available bits. I wonder whether a
> better idea would be to extend struct update_callback_data to include a
> `force_mode` field, pass a parameter of the same name to
> add_files_to_cache() and then handle that in the update_callback().

Maybe.  I am not sure if it is a good idea to do lstat(2) on the
calling side, though.  Assuming it is, your "something like this"
needs to be duplicated for the codepath that adds a new file, which
is separate from the one we see below (i.e. add_files()).

> Something like this:
>
> case DIFF_STATUS_MODIFIED:
> -   case DIFF_STATUS_TYPE_CHANGED:
> +   case DIFF_STATUS_TYPE_CHANGED: {
> + struct stat st;
> + if (lstat(path, ))
> + die_errno("unable to stat '%s'", path);
> + if (S_ISREG(_mode) && data->force_mode)
> + st.st_mode = data->force_mode;
> -   if (add_file_to_index(_index, path, data->flags)) 
> {
> +   if (add_to_index(_index, path, , data->flags)) 
> {
> if (!(data->flags & ADD_CACHE_IGNORE_ERRORS))
> die(_("updating files failed"));
> data->add_errors++;
> }
> break;
> + }
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] add: add --chmod=+x / --chmod=-x options

[Junio C Hamano]

Junio C Hamano  writes:

> Edward Thomson  writes:
>
>> However I do not think that this is a common enough action that it needs
>> to be made automatic such that when I `git add foo.rb` it is
>> automatically made executable.  I think that the reduced complexity of
>> having a single mechanism to control executability (that being the
>> execute mode in the index or a tree) is preferable to a gitattributes
>> based mechanism, at least until somebody else makes a cogent argument
>> that the gitattributes approach would be helpful for them.  :)
>
> It wasn't a "having to specify it every time sucks; you must do this
> way instead" at all.  I was just gauging if it would be a viable idea
> for a follow-up series to complement your patch.
>
> Thanks.

Oh, having said all of that, the comments on the implementation
still stand.
--
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: t7610-mergetool.sh test failure

[Jeff King]

On Thu, May 26, 2016 at 11:33:27PM -0700, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > The only one I can think of is that if something leaves cruft in
> > $TMPDIR, it could affect later tests that want to `git add`
> > indiscriminately.
> 
> Or "git ls-files -u", "git clean", etc.  I'd mostly worry about a
> failed test in which a program dies without a chance to clean up
> after itself, and letting the cruft affecting the next test.

Yeah, exactly. OTOH, that could be considered a feature. If one of our
programs is leaving cruft in $TMPDIR, that is probably a bug that should
be fixed.

We wouldn't notice in most cases though (it would depend on some later
test actually caring about the cruft). So your "leave cruft in the
source directory but outside the trash directory" would be better there.
I'd worry slightly that it would cause problems when running tests in
parallel, though. Normal /tmp usage is OK with a global namespace (they
choose unique names), but sometimes things might use /tmp with a
well-known name as a rendezvous point (e.g, for sockets). But I guess
such cases are already broken for running in parallel, since /tmp is a
global namespace.

> I just checked my /tmp, and I see a lot of directories whose name
> look like mktemp generated one, with a single socket 's' in them.  I
> wouldn't be surprised if they turn out to be from our tests that
> expect failure, killing a daemon that does not properly clean after
> itself.  People probably would not notice if they are in /tmp, and
> if we moved TMPDIR to the trash, we still wouldn't (because running
> tests successfully without "-d" option will remove the trash
> directory at the end), but if it were dropped somewhere in the
> source tree, we have a better chance of noticing it.

Hmm. I'm not sure what would create a socket in git except the
credential-cache stuff, and that does not write into /tmp (there was a
GSoC-related series in March to move this, but I don't think it ever
even made it to pu). Maybe the new watchman stuff?

If you have inotify-tools, you can "inotifywait -m /tmp" and run "make
test", but it's quite noisy, as apparently a lot of tested commands use
tempfiles internally. Which perhaps shows that maybe some people would
see a performance regression if we moved from /tmp to a slower
filesystem (e.g., especially people whose git clone is on NFS or
something).

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

Re: [PATCH] worktree: allow "-" short-hand for @{-1} in add command

[Junio C Hamano]

Jordan DE GEA  writes:

>>> +test_expect_success '"add" using shorthand - fails when no previous 
>>> branch' '
>>> +   test_must_fail git worktree add existing -
>>> +'
>> 
>> Just an observation, but the error message we would see here might
>> be interesting.
>
> Of course, that’s useful to be sure of the error, I will do in next preroll.

That was not what I meant.  The exit status being non-zero is what
we primarily care about.  The exact phrasing of the error message is
much less important and in general we shouldn't enforce "the error
message must remain so" in the test.

If you observe the error message from this test, e.g. by running it
with "-v", I suspect that you would see the message would complain
about "@{-1}".

I just wanted to make sure that you saw it and thought about its
ramifications.

It is perfectly fine by me (others might disagree, though) if your
conclusion after thinking about it is "Even though the user may be
surprised to get complaints for "@{-1}" that she never gave to the
command (she gave "-"), because we clearly document that "-" is a
mere synonym/short-hand for @{-1}, it is OK".  I still want to see
that behaviour justified in the proposed commit log message.

And that is why I said it "might be interesting".


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

Re: [PATCH] t6030: explicitly test for bisection cleanup

[Pranit Bauva]

On Fri, May 13, 2016 at 3:44 PM, Pranit Bauva  wrote:
> This is not an improvement in the test coverage but it helps in making
> it explicit as to know what exactly is the error as other tests are
> focussed on testing other things but they do indirectly test for this.
>
> Mentored-by: Lars Schneider 
> Mentored-by: Christian Couder 
> Signed-off-by: Pranit Bauva 
>
> ---
> I faced this problem while converting `bisect_clean_state` and the tests
> where showing breakages but it wasn't clear as to where exactly are they
> breaking. This will patch  will help in that. Also I tested the test
> coverage of the test suite before this patch and it covers this (I did
> this by purposely changing names of files in git-bisect.sh and running
> the test suite).
>
> Signed-off-by: Pranit Bauva 
> ---
>  t/t6030-bisect-porcelain.sh | 17 +
>  1 file changed, 17 insertions(+)
>
> diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
> index e74662b..1fb5ad9 100755
> --- a/t/t6030-bisect-porcelain.sh
> +++ b/t/t6030-bisect-porcelain.sh
> @@ -894,4 +894,21 @@ test_expect_success 'bisect start takes options and revs 
> in any order' '
> test_cmp expected actual
>  '
>
> +test_expect_success 'git bisect reset cleans bisection state properly' '
> +   git bisect reset &&
> +   git bisect start &&
> +   git bisect good $HASH1 &&
> +   git bisect bad $HASH4 &&
> +   git bisect reset &&
> +   test -z "$(git for-each-ref "refs/bisect/*")" &&
> +   ! test -s "$GIT_DIR/BISECT_EXPECTED_REV" &&
> +   ! test -s "$GIT_DIR/BISECT_ANCESTORS_OK" &&
> +   ! test -s "$GIT_DIR/BISECT_LOG" &&
> +   ! test -s "$GIT_DIR/BISECT_RUN" &&
> +   ! test -s "$GIT_DIR/BISECT_TERMS" &&
> +   ! test -s "$GIT_DIR/head-name" &&
> +   ! test -s "$GIT_DIR/BISECT_HEAD" &&
> +   ! test -s "$GIT_DIR/BISECT_START"
> +'
> +
>  test_done
> --
> 2.8.2
>

Anyone any comments?

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

Re: [PATCH] worktree: allow "-" short-hand for @{-1} in add command

[Junio C Hamano]

Matthieu Moy  writes:

> Junio C Hamano  writes:
>
>> Jordan DE GEA  writes:
>>
>>> +   branch=$(cd short-hand && git rev-parse --symbolic-full-name HEAD) &&
>>> +   test "$branch" = refs/heads/newbranch &&
>>> +   cd ..
>>
>> If any of the command between "cd short-hand" and "cd .." failed,
>> after correcting the broken &&-chain, the next test will end up
>> running in short-hand directory, which it is not expecting.  A
>> canonical way to avoid this problem is to replace the above with:
>>
>>  ...
>> git worktree add short-hand - &&
>> (
>>  cd short-hand &&
>> ...
>> test "$branch" = refs/heads/newbranch
>>  )
>
> Actually, $(...) implicitly does a subshell, so the "cd .." was just
> useless.

You trimmed my message a bit too aggressively while composing your
response, and I think that is what ended up confusing you.  

Here is what I wrote:

| > +   cd short-hand &&
| > +   test $(git rev-parse --symbolic-full-name HEAD) = "refs/heads/newbranch"
| 
| Broken &&-chain.
| 
| > +   branch=$(cd short-hand && git rev-parse --symbolic-full-name HEAD) &&
| > +   test "$branch" = refs/heads/newbranch &&
| > +   cd ..
| 
| If any of the command between "cd short-hand" and "cd .." failed,
| ...

The problematic "cd short-hand" is the one a few lines above where
you started quoting, not the one you saw in the $().

--
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: [RFC/PATCH 2/2] log: add "--no-show-signature" command line option

[Jeff King]

On Fri, May 27, 2016 at 10:37:16AM -0700, Junio C Hamano wrote:

> Mehul Jain  writes:
> 
> > On Thu, May 26, 2016 at 10:52 PM, Junio C Hamano  wrote:
> >
> >> The only reason why teaching the "--no-show-signature" option to
> >> these commands is a good idea is because it would help people who
> >> create an alias with "--show-sig" in early part of the command line,
> >> e.g.
> >>
> >> [alias] fp = format-patch --show-signature
> >>
> >> by allowing them to countermand with --no-show-signature, i.e.
> >>
> >> $ git fp --no-show-signature ...
> >> ...
> >
> > Just out of curiosity, I was thinking that we might be able to teach
> > "--no-show-signature" option only to git-show, git-log, git-whatchanged
> > and git-reflog.
> 
> Yeah, I know it is possible with extra code, but I do not think of a
> good reason why it is necessary.

Not only "not necessary" but "actively worse" in my opinion. We have
--show-signature in revision.c, and that is reason enough to have
--no-show-signature, in case anybody would want to countermand an
earlier request (whether from config that is soon to exist, or from a
previous --show-signature on the command line), or just because somebody
feels like making sure git is doing what they want without bothering to
check the defaults.

We add the "--no-" form by default for all of our bools parsed by
parse-options. The only reason it is not already here is that this
option parsing predates our use of parse-options, and nobody had
bothered to go back and add it. But doing so is a win simply for
consistency if nothing else, IMHO.

I actually think it would be nice to convert all of handle_revision_opt
to parse-options, but that's a non-trivial task. And I certainly
wouldn't want it to hold up this otherwise simple topic.

-Peff
--
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: [RFC/PATCH 2/2] log: add "--no-show-signature" command line option

[Junio C Hamano]

Mehul Jain  writes:

> On Thu, May 26, 2016 at 10:52 PM, Junio C Hamano  wrote:
>
>> The only reason why teaching the "--no-show-signature" option to
>> these commands is a good idea is because it would help people who
>> create an alias with "--show-sig" in early part of the command line,
>> e.g.
>>
>> [alias] fp = format-patch --show-signature
>>
>> by allowing them to countermand with --no-show-signature, i.e.
>>
>> $ git fp --no-show-signature ...
>> ...
>
> Just out of curiosity, I was thinking that we might be able to teach
> "--no-show-signature" option only to git-show, git-log, git-whatchanged
> and git-reflog.

Yeah, I know it is possible with extra code, but I do not think of a
good reason why it is necessary.
--
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: [Bug?] log -p -W showing the whole file for a patch that adds to the end?

[René Scharfe]

Am 26.05.2016 um 19:05 schrieb Junio C Hamano:

I'd say that these patches are fine as they are, and follow-up patch
for adding -W tests (instead of rerolling them) is sufficient,
though.


Patch 3 needs two small updates to address the char signedness issue 
found by Ramsay and to get rid of an unused function parameter, and 
patch 4 needs a small change as a result of the latter as well. 
Shouldn't be long before tests are done..


René

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

[RFC-PATCH v2 2/2] send-email: quote-email quotes the message body

[Tom Russello]

Take an email message file, parse it and quote the message body.

If `--compose` is set, then it will quote the message in the cover letter.
Otherwise, imply `--annotate` option and put the quoted message in the first
patch after the triple-dash.

Signed-off-by: Tom Russello 
Signed-off-by: Samuel Groot 
Signed-off-by: Matthieu Moy 
---
Currently, `send-email` without `--compose` implies `--annotate`. The
current behavior of `--annotate` is that changes made during edition are saved
in patch files.

Keeping that behavior when using `--quote-email` populates the patch file with
the quoted message body, and the patch is saved no matter what. If the user
closes his editor and then exits `send-email`, changes will be saved.

Should we keep the current behavior for the user, keeping the changes (including
the quoted message body) in the patch, or should we discard them?


changes since v1:
- default behaviour of the option: it is now --annotate as one may not 
want
  to send a cover letter for a single patch
- "On [date], [original author] wrote:" is added before the quoted 
message
- request to trim useless parts of the cited message when `--compose` 
is set
- extra blank removed when the message quoted is empty

 Documentation/git-send-email.txt |  5 ++-
 git-send-email.perl  | 79 +---
 t/t9001-send-email.sh|  6 +++
 3 files changed, 84 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
index 2334d69..68bbb6c 100644
--- a/Documentation/git-send-email.txt
+++ b/Documentation/git-send-email.txt
@@ -109,7 +109,10 @@ is not set, this will be prompted for.
 --quote-email=::
Reply to the given email and automatically populate the "To:", "Cc:" and
"In-Reply-To:" fields. If `--compose` is set, this will also fill the
-   subject field with "Re: ['s subject]".
+   subject field with "Re: ['s subject]" and quote the message 
body
+   of . If `--compose` is not set, this will imply `--annotate`
+   option and will quote the message body of  after the 
triple-dash
+   in the first patch.
 
 --subject=::
Specify the initial subject of the email thread.
diff --git a/git-send-email.perl b/git-send-email.perl
index 9df3dee..e73aa25 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -26,6 +26,7 @@ use Text::ParseWords;
 use Term::ANSIColor;
 use File::Temp qw/ tempdir tempfile /;
 use File::Spec::Functions qw(catfile);
+use File::Copy;
 use Error qw(:try);
 use Git;
 
@@ -55,8 +56,8 @@ git send-email --dump-aliases
 --[no-]bcc* Email Bcc:
 --subject * Email "Subject:"
 --in-reply-to * Email "In-Reply-To:"
---quote-email* Fill the fields "To:", "Cc:", "Subject:",
- "In-Reply-To" appropriately.
+--quote-email* Fill To:, Cc:, Subject:, In-Reply-To:
+ appropriately and quote the message body.
 --[no-]xmailer * Add "X-Mailer:" header (default).
 --[no-]annotate* Review each patch that will be sent in an 
editor.
 --compose  * Open an editor for introduction.
@@ -642,11 +643,14 @@ if (@files) {
usage();
 }
 
+my $message_quoted;
 if ($quote_email) {
my $error = validate_patch($quote_email);
$error and die "fatal: $quote_email: $error\nwarning: no patches were 
sent\n";
 
my @header = ();
+   my $date;
+   my $recipient;
 
open my $fh, "<", $quote_email or die "can't open file $quote_email";
 
@@ -687,7 +691,8 @@ if ($quote_email) {
}
$initial_subject = $prefix_re . $subject_re;
} elsif (/^From:\s+(.*)$/i) {
-   push @initial_to, $1;
+   $recipient = $1;
+   push @initial_to, $recipient;
} elsif (/^To:\s+(.*)$/i) {
foreach my $addr (parse_address_line($1)) {
if (!($addr eq $initial_sender)) {
@@ -707,6 +712,8 @@ if ($quote_email) {
}
} elsif (/^Message-Id: (.*)/i) {
$initial_reply_to = $1;
+   } elsif (/^Date: (.*)/i) {
+   $date = $1;
}
} else {
# In the traditional
@@ -722,6 +729,23 @@ if ($quote_email) {
}
}
}
+
+   my $tpl_date = $date && "On $date, " || '';
+   $message_quoted = $tpl_date . $recipient . " wrote:\n";
+
+   # Quote the 

[RFC-PATCH v2 1/2] send-email: quote-email populates the fields

[Tom Russello]

Take an email message file, parse it and fill the "To", "Cc" and
"In-Reply-To" fields appropriately.

If `--compose` option is set, it will also fill the subject field with
"Re: ['s subject]".

Signed-off-by: Tom Russello 
Signed-off-by: Samuel Groot 
Signed-off-by: Matthieu Moy 
---
As it is said in the cover letter, the file git-send-email.perl is being
refactored therefore the parsing section with nested if's is ought to
change.

changes since v1:
- option's name changed and is now --quote-email
- original From: becomes To:, original To:'s become Cc: and original
  Cc:'s stay Cc:
- coding style improved
- documentation for the option
- more tests

 Documentation/git-send-email.txt |  5 +++
 git-send-email.perl  | 87 +++-
 t/t9001-send-email.sh| 61 
 3 files changed, 152 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
index 771a7b5..2334d69 100644
--- a/Documentation/git-send-email.txt
+++ b/Documentation/git-send-email.txt
@@ -106,6 +106,11 @@ illustration below where `[PATCH v2 0/3]` is in reply to 
`[PATCH 0/2]`:
 Only necessary if --compose is also set.  If --compose
 is not set, this will be prompted for.
 
+--quote-email=::
+   Reply to the given email and automatically populate the "To:", "Cc:" and
+   "In-Reply-To:" fields. If `--compose` is set, this will also fill the
+   subject field with "Re: ['s subject]".
+
 --subject=::
Specify the initial subject of the email thread.
Only necessary if --compose is also set.  If --compose
diff --git a/git-send-email.perl b/git-send-email.perl
index 6958785..9df3dee 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -55,6 +55,8 @@ git send-email --dump-aliases
 --[no-]bcc* Email Bcc:
 --subject * Email "Subject:"
 --in-reply-to * Email "In-Reply-To:"
+--quote-email* Fill the fields "To:", "Cc:", "Subject:",
+ "In-Reply-To" appropriately.
 --[no-]xmailer * Add "X-Mailer:" header (default).
 --[no-]annotate* Review each patch that will be sent in an 
editor.
 --compose  * Open an editor for introduction.
@@ -160,7 +162,7 @@ my $re_encoded_word = 
qr/=\?($re_token)\?($re_token)\?($re_encoded_text)\?=/;
 
 # Variables we fill in automatically, or via prompting:
 my (@to,$no_to,@initial_to,@cc,$no_cc,@initial_cc,@bcclist,$no_bcc,@xh,
-   $initial_reply_to,$initial_subject,@files,
+   $initial_reply_to,$quote_email,$initial_subject,@files,
$author,$sender,$smtp_authpass,$annotate,$use_xmailer,$compose,$time);
 
 my $envelope_sender;
@@ -304,6 +306,7 @@ $rc = GetOptions(
"sender|from=s" => \$sender,
 "in-reply-to=s" => \$initial_reply_to,
"subject=s" => \$initial_subject,
+   "quote-email=s" => \$quote_email,
"to=s" => \@initial_to,
"to-cmd=s" => \$to_cmd,
"no-to" => \$no_to,
@@ -639,6 +642,88 @@ if (@files) {
usage();
 }
 
+if ($quote_email) {
+   my $error = validate_patch($quote_email);
+   $error and die "fatal: $quote_email: $error\nwarning: no patches were 
sent\n";
+
+   my @header = ();
+
+   open my $fh, "<", $quote_email or die "can't open file $quote_email";
+
+   # Get the email header
+   while (<$fh>) {
+   # For files containing crlf line endings
+   s/\r//g;
+   last if /^\s*$/;
+   if (/^\s+\S/ and @header) {
+   chomp($header[$#header]);
+   s/^\s+/ /;
+   $header[$#header] .= $_;
+   } else {
+   push(@header, $_);
+   }
+   }
+
+   # Parse the header
+   foreach (@header) {
+   my $input_format;
+   my $initial_sender = $sender || $repoauthor || $repocommitter 
|| '';
+
+   if (/^From /) {
+   $input_format = 'mbox';
+   next;
+   }
+   chomp;
+   if (!defined $input_format && /^[-A-Za-z]+:\s/) {
+   $input_format = 'mbox';
+   }
+
+   if (defined $input_format && $input_format eq 'mbox') {
+   if (/^Subject:\s+(.*)$/i) {
+   my $prefix_re = "";
+   my $subject_re = $1;
+   if ($1 =~ /^[^Re:]/) {
+   $prefix_re = "Re: ";
+   }
+   $initial_subject = $prefix_re . 

[RFC-PATCH v2 0/2] send-email: new --quote-email option

[Tom Russello]

Hello,

With the current send-email command, you can send a series of patches
"in reply to" an email.

This patch adds a new option to `git send-email`,
`--quote-email=` which does two things:
- set fields appropriately (To, Cc, Subject, In-Reply-To)
- quote the given message

In this second patch, the new option `--quote-email=` needs an
email file and does not manage non ascii characters.

There is still work in progress, including:

  1. An option `--quote-email-id=` to download the message
 from any source, e.g. http://mid.gmane.org//raw.
 The server's address could be set in the repo's config file.

  2. There's also a discussion about whether this option should be
 integrated in the current `--in-reply-to` option or not

 * 
http://news.gmane.org/find-root.php?message_id=vpqh9dmfy5k@anie.imag.fr


  3. The code to parse the email headers is currently duplicated
 several times, we are refactoring it to help maintaining the
 code.

  4. Manage non ascii characters

Documentation/git-send-email.txt |   8 +++
git-send-email.perl  | 160 
---
t/t9001-send-email.sh|  67 
+
3 files changed, 232 insertions(+), 3 deletions(-)

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

Re: [PATCH v2 00/22] i18n and test updates

[Junio C Hamano]

Vasco Almeida  writes:

> Marks several messages for translation and updates tests to pass under
> GETTEXT_POISON. Some tests were updated to fit previous i18n marks, others
> were updated to fit marks made by these patches. Patches that only touch
> test file refer to marks done in commits previous to these ones.

Whew, this series is quite a lot of work.

Most of the changes I only skimmed so if you had typos I wouldn't
have noticed them, but they looked good from my cursory reading.

I looked at the changes to scripted Porcelains more carefully than
other changes, and they all looked good (except for the parts I sent
reviews separately for).

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

Re: [PATCH v2 18/22] tests: use test_i18n* functions to suppress false positives

[Junio C Hamano]

Vasco Almeida  writes:

> diff --git a/t/t1307-config-blob.sh b/t/t1307-config-blob.sh
> index 3c6791e..4079fef 100755
> --- a/t/t1307-config-blob.sh
> +++ b/t/t1307-config-blob.sh
> @@ -64,7 +64,7 @@ test_expect_success 'parse errors in blobs are properly 
> attributed' '
>  
>   # just grep for our token as the exact error message is likely to
>   # change or be internationalized
> - grep "HEAD:config" err
> + test_i18ngrep "HEAD:config" err
>  '

It is unfortunate that the gettext-poison mechanism is too dumb to
notice that it is clobbering a format string with placeholders and
leave them intact, which is what the comment above this change is
wishing for.  I do not think we will be granting that wish any time
soon, so perhaps remove the two lines while we are at it?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4] upload-pack.c: use of parse-options API

[Eric Sunshine]

On Fri, May 27, 2016 at 10:16 AM, Antoine Queru
 wrote:
> upload-pack.c: use of parse-options API

Matthieu already mentioned that this should use imperative mood:

upload-pack: use parse-options API

> Option parsing now uses the parser API instead of a local parser.
> Code is now more compact.

Imperative:

Use the parse-options API rather than a hand-rolled
option parser.

That the code becomes more compact is a nice result of this change,
but not particularly important, thus not really worth a mention in the
commit message.

> Description for --stateless-rpc and --advertise-refs come from
> the commit 42526b4 (Add stateless RPC options to upload-pack,
> receive-pack, 2009-10-30).

s/from the commit/from/

> Signed-off-by: Antoine Queru 
> Signed-off-by: Matthieu Moy 
> ---
> diff --git a/Documentation/git-upload-pack.txt 
> b/Documentation/git-upload-pack.txt
> @@ -31,6 +31,19 @@ OPTIONS
> +--stateless-rpc::
> +   the programs now assume they may perform only a single read-write

s/the/The/

Also, to what "programs" does this refer? And what does "now" mean here?

> +   cycle with stdin and stdout. This fits with the HTTP POST request
> +   processing model where a program may read the request, write a
> +   response, and must exit.
> +
> +--advertise-refs::
> +   When --advertise-refs is passed as a command line parameter only

The entire "When ... parameter" bit is redundant, isn't it? Why not
just say "Perform only..."?

> +   the initial ref advertisement is output, and the program exits
> +   immediately.  This fits with the HTTP GET request model, where
> +   no request content is received but a response must be produced.
> +
> +

Style: Drop the extra blank line.

>  ::
> The repository to sync from.
>
> diff --git a/upload-pack.c b/upload-pack.c
> @@ -817,11 +821,21 @@ static int upload_pack_config(const char *var, const 
> char *value, void *unused)
> -int main(int argc, char **argv)
> +int main(int argc, const char **argv)
>  {
> -   char *dir;
> -   int i;
> +   const char *dir;
> int strict = 0;
> +   struct option options[] = {
> +   OPT_BOOL(0, "stateless-rpc", _rpc,
> +N_("quit after a single request/response exchange")),
> +   OPT_BOOL(0, "advertise-refs", _refs,
> +N_("only the initial ref advertisement is output, 
> program exits immediately")),

Possible rewrite: "exit immediately after initial ref advertisement"

> +   OPT_BOOL(0, "strict", ,
> +N_("do not try /.git/ if  is 
> no Git directory")),

Use of OPT_BOOL introduces a --no-strict option which didn't exist
before. Does this need to be documented? (Genuine question.)

> +   OPT_INTEGER(0, "timeout", ,
> +   N_("interrupt transfer after  seconds of 
> inactivity")),
> +   OPT_END()
> +   };
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 14/22] i18n: rebase-interactive: mark strings for translation

[Junio C Hamano]

Vasco Almeida  writes:

>>>  is_empty_commit() {
>>> -   tree=$(git rev-parse -q --verify "$1"^{tree} 2>/dev/null ||
>>> -   die "$1: not a commit that can be picked")
>>> -   ptree=$(git rev-parse -q --verify "$1"^^{tree} 2>/dev/null ||
>>> +   sha1=$1
>>> +   tree=$(git rev-parse -q --verify "$sha1"^{tree} 2>/dev/null ||
>>> +   die "$(eval_gettext "\$sha1: not a commit that can be picked")")
>>> +   ptree=$(git rev-parse -q --verify "$sha1"^^{tree} 2>/dev/null ||
>>> ptree=4b825dc642cb6eb9a060e54bf8d69288fbee4904)
>>> test "$tree" = "$ptree"
>>>  }
>> 
>> Both of the two callsites of this function use the variable $sha1,
>> and at least one of them uses that variable after this function
>> returns, but they pass it as the first parameter to this function,
>> so the assignment added by this patch does not break them, which is
>> good.
>> 
> I didn't know that. I can change sha1=$1 to local_sha1=$1 or _sha1=$1 (I
> don't know what is the convention here) if that is safer, avoiding using
> the bash-ism "local" keyword, and preventing future distractions.

I do not think it is necessary to do any of the changes for this
one, which is only used locally in this file.  They just need to be
careful when they add or modify the callers.

Even if it is renamed $local_sha1, they will still need to be
careful anyway, because the only way they _can_ break or be broken
by your patch is when they somehow decide to pass something that is
not called $sha1 to this function, i.e. they would be changing the
callsite---which they must be doing for a reason and would also
involve change to the code what happens after this function returns.
If they start using $local_sha1 there, they would be broken if you
renamed it today, and if they use $sha1 there, they would be broken
if you didn't---so they have to be careful and check what this one
clobbers _anyway_.  

IOW, a solution that clobbers _some_ variable cannot be made safer
by renaming.  The callers always have to be careful not to be
affected.

You could do something like this, though.  I _think_ the original
that calls "die" inside $() is broken anyway, so it probably is a
good change regardless.

 git-rebase--interactive.sh | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 9d2bfb7..b5113d6 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -222,8 +222,10 @@ has_action () {
 }
 
 is_empty_commit() {
-   tree=$(git rev-parse -q --verify "$1"^{tree} 2>/dev/null ||
-   die "$1: not a commit that can be picked")
+   tree=$(git rev-parse -q --verify "$1"^{tree} 2>/dev/null) || {
+   sha1=$1
+   die "$(eval_gettext "\$sha1: not a commit that can be picked")"
+   }
ptree=$(git rev-parse -q --verify "$1"^^{tree} 2>/dev/null ||
ptree=4b825dc642cb6eb9a060e54bf8d69288fbee4904)
test "$tree" = "$ptree"

--
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: RFC: new git-splice subcommand for non-interactive branch splicing

[Adam Spiers]

Hi Johannes,

Thanks for the quick reply!  Responses inline below:

On Fri, May 27, 2016 at 05:27:14PM +0200, Johannes Schindelin wrote:
> On Fri, 27 May 2016, Adam Spiers wrote:
> 
> > Description
> > ---
> > 
> > git-splice(1) non-interactively splices the current branch by removing
> > a range of commits from within it and/or cherry-picking a range of
> > commits into it.  It's essentially just a glorified wrapper around
> > cherry-pick and rebase -i.
> 
> It sounds as if you could accomplish the same with
> 
>   git checkout -b former-commits 
>   git checkout -b latter-commits 
>   git cherry-pick ..HEAD@{2}

Not really - that is missing several features which git-splice
provides, e.g.

  - The ability to remove a non-consecutive list of commits
from the branch.

  - The ability to insert commits at the same time as removing
(granted, that's just in extra cherry-pick your method, but again
that's another thing to orchestrate).

  - The ability to specify commits to remove / insert using
arguments understood by git-rev-list.

  - The patch-id magic which is built into git-rebase.  This
would kick in if any of the commits to insert are already
in ..HEAD@{2} (using your reference terminology).

  - A single command to orchestrate the whole workflow, including
cleanup, and --abort and --continue when manual conflict
resolution is required.  This modularity should help a lot when
building further tools which wrap around it in order to perform
more complex tasks.

This last point is perhaps the most important.  Of course it's
possible to do this manually already.  But the whole point of
git-splice is to automate it in a convenient and reliable manner.

> > Next steps, and the future
> > --
> > 
> > Obviously, I'd welcome thoughts on whether it would make sense to
> > include this in the git distribution.
> 
> Far be I from discouraging you to work on these scripts, but I think that
> a really good place for such subcommands is a separate repository, as you
> have it already. There are already some rarely used subcommands in
> libexec/git-core/ cluttering up the space and I would be reluctant to add
> even more subcommands to the default Git installation delivered to every
> user.

Sure, I appreciate the difficulty in deciding where to draw the line.
My feeling is that rebase -i provides something tremendously
important, which the vast majority of users use on a regular basis,
but that git is currently missing a convenient way to
*non-interactively* perform the same magic which rebase -i
facilitates.  And removing / reordering commits is surely one of the
most common use cases of rebase -i, so I think a lot of people could
benefit from some porcelain to automate that and allow building
higher-level tools on top of it.

I suspect the most popular use-case in the short term would be the
infamous "oops, I only just noticed that I put that commit on the
wrong branch, and now there's already a whole bunch of other commits
on top of it".  I would expect that reducing this solution to a single
git-transplant(1) command would be pretty attractive for a lot of
people.  And of course GUIs / IDEs could incorporate it into their
more beautiful front-ends.  However, if it's not in git core, that's
unlikely to happen.

> You can *always* just extend the PATH so that git-splice can be found;
> Then `git splice ...` will do exactly what you want. That is e.g. how
> git-flow works.

Sure, I've been using that trick since at least 2009 ;-) [0]

> (Of course I hope that you will maintain your scripts
> much, much better than git-flow, i.e. not abandon all users).

I hope so too ;-)

> > In the longer term however, I'd like to write two more subcommands:
> > 
> >   - git-transplant(1) which wraps around git-splice(1) and enables
> > easy non-interactive transplanting of a range of commits from
> > one branch to another.  This should be pretty straightforward
> > to implement.
> 
> This is just cherry-pick with a range...

No it's not:

  - git-transplant would be able to splice commits from one branch
*into* (i.e. inside, *not* onto) another branch.

  - git-transplant would also take care of removing the commits from
the source branch, but not before they were safely inside the
destination branch.

  - git-transplant would orchestrate the whole workflow with a single
command, complete with --abort and --continue.

> >   - git-explode(1) which wraps around git-transplant(1) and
> > git-deps(1), and automatically breaks a linear sequence of commits
> > into multiple smaller sequences, forming a commit graph where
> > ancestry mirrors commit dependency, as mentioned above.  I expect
> > this to be more difficult, and would probably write it in Python.
> 
> You mean something like Darcs on top of Git. Essentially, you want to end
> up with an octopus merge of branches whose commits would conflict if
> 

Re: Small rerere in rebase regression

[Johannes Sixt]

Am 25.05.2016 um 07:38 schrieb Johannes Schindelin:
> In short: yes, the explicit `git rerere` call can be dropped, that is
> essentially what I did in the rebase--helper branch.

Here's a patch to do that.

--- 8< ---
Subject: [PATCH] rebase -i: remove an unnecessary 'rerere' invocation

Interactive rebase uses 'git cherry-pick' and 'git merge' to replay
commits. Both invoke the 'rerere' machinery when they fail due to merge
conflicts. Note that all code paths with these two commands also invoke
the shell function die_with_patch when the commands fail.

Since commit 629716d2 ("rerere: do use multiple variants") the second
operation of the rerere machinery can be observed by a duplicated
message "Recorded preimage for 'file'". This second operation records
the same preimage as the first one and, hence, only wastes cycles.
Remove the 'git rerere' invocation from die_with_patch.

Shell function die_with_patch can be called after the failure of
"git commit", too, which also calls into the rerere machinery, but it
does so only after a successful commit to record the resolution.
Therefore, it is wrong to call 'git rerere' from die_with_patch after
"git commit" fails.

Signed-off-by: Johannes Sixt 
---
 git-rebase--interactive.sh | 1 -
 1 file changed, 1 deletion(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 9d2bfb7..6e96abc 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -192,7 +192,6 @@ make_patch () {
 die_with_patch () {
echo "$1" > "$state_dir"/stopped-sha
make_patch "$1"
-   git rerere
die "$2"
 }
 
-- 
2.9.0.rc0.40.gb3c1388

--
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: Advanced revision selection?

[Matthieu Moy]

Nikolaus Rath  writes:

> In Mercurial, this can be done with a more verbose syntax (e.g.
> "last(ancestors(tag("re:release-.*")),3) and descendants(aebeccf)" for
> the above example).

This has no direct equivalent in Git. A lot can be done passing options
to "git rev-list" or so, for example, but I don't think a combination
can be found for your exact example.

The closest I get is:

  git log `git for-each-ref refs/tags/release- --format '%(refname)'` -3

Which lists 3 commits preceeding a tag whose name starts with release-.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] log: document the --decorate=auto option

[Ramsay Jones]

Signed-off-by: Ramsay Jones 
---

Hi Junio,

While reading an email from Linus earlier (RFC: dynamic "auto" date formats),
I noticed that log.decorate was being set to 'auto'. Since I didn't recall
that setting (even if it's easy to guess), I went looking for the documentation 
...

This should probably be marked RFC, since I haven't checked that the formatting
is OK (I don't have the documentation toolchain installed these days).

ATB,
Ramsay Jones

 Documentation/config.txt  | 5 -
 Documentation/git-log.txt | 8 +---
 2 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 53f00db..0707b3b 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1956,7 +1956,10 @@ log.decorate::
command. If 'short' is specified, the ref name prefixes 'refs/heads/',
'refs/tags/' and 'refs/remotes/' will not be printed. If 'full' is
specified, the full ref name (including prefix) will be printed.
-   This is the same as the log commands '--decorate' option.
+   If 'auto' is specified, then if the output is going to a terminal,
+   the ref names are shown as if 'short' were given, otherwise no ref
+   names are shown. This is the same as the log commands '--decorate'
+   option.
 
 log.follow::
If `true`, `git log` will act as if the `--follow` option was used when
diff --git a/Documentation/git-log.txt b/Documentation/git-log.txt
index 03f9580..dec379b 100644
--- a/Documentation/git-log.txt
+++ b/Documentation/git-log.txt
@@ -29,12 +29,14 @@ OPTIONS
(works only for a single file).
 
 --no-decorate::
---decorate[=short|full|no]::
+--decorate[=short|full|auto|no]::
Print out the ref names of any commits that are shown. If 'short' is
specified, the ref name prefixes 'refs/heads/', 'refs/tags/' and
'refs/remotes/' will not be printed. If 'full' is specified, the
-   full ref name (including prefix) will be printed. The default option
-   is 'short'.
+   full ref name (including prefix) will be printed. If 'auto' is
+   specified, then if the output is going to a terminal, the ref names
+   are shown as if 'short' were given, otherwise no ref names are
+   shown. The default option is 'short'.
 
 --source::
Print out the ref name given on the command line by which each
-- 
2.8.0
--
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

Advanced revision selection?

[Nikolaus Rath]

Hello,

Is there a way to specify revision ranges that has more power than
what's described in gitrevisions(7)?

For example, is there a way to show "the three most recent commits
preceding a tag that starts with "release-" that are also descendants of
commit aebecf."?

In Mercurial, this can be done with a more verbose syntax (e.g.
"last(ancestors(tag("re:release-.*")),3) and descendants(aebeccf)" for
the above example). Is there no equivalent in Git, or am I looking for
it in the wrong place?

Best,
-Nikolaus

-- 
GPG encrypted emails preferred. Key id: 0xD113FCAC3C4E599F
Fingerprint: ED31 791B 2C5C 1613 AF38 8B8A D113 FCAC 3C4E 599F

 »Time flies like an arrow, fruit flies like a Banana.«
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] blame.c: don't drop origin blobs as eagerly

[David Kastrup]

Johannes Schindelin  writes:

> On Fri, 27 May 2016, David Kastrup wrote:
>
>> pressure particularly when the history contains lots of merges from
>> long-diverged branches.  In practice, this optimization appears to
>> behave quite benignly,
>
> Why not just stop here?

Because there is a caveat.

> I say that because...
>
>> and a viable strategy for limiting the total amount of cached blobs in a
>> useful manner seems rather hard to implement.
>
> ... this sounds awfully handwaving.

Because it is.

> Since we already have reference counting, it sounds fishy to claim
> that simply piggybacking a global counter on top of it would be
> "rather hard".

You'll see that the patch is from 2014.  When I actively worked on it,
I found no convincing/feasible way to enforce a reasonable hard limit.
I am not picking up work on this again but am merely flushing my queue
so that the patch going to waste is not on my conscience.

>> In addition, calling git-blame with -C leads to similar memory retention
>> patterns.
>
> This is a red herring. Just delete it. I, for one, being a heavy user of
> `git blame`, could count the number of times I used blame's -C option
> without any remaining hands. Zero times.
>
> Besides, -C is *supposed* to look harder.

We are not talking about "looking harder" but "taking more memory than
the set limit".

> Also: is there an easy way to reproduce your claims of better I/O
> characteristics? Something like a command-line, ideally with a file in
> git.git's own history, that demonstrates the I/O before and after the
> patch, would be an excellent addition to the commit message.

I've used it on the wortliste repository and system time goes down from
about 70 seconds to 50 seconds (this is a flash drive).  User time from
about 4:20 to 4:00.  It is a rather degenerate repository (predominantly
small changes in one humongous text file) so savings for more typical
cases might end up less than that.  But then it is degenerate
repositories that are most costly to blame.

> Further: I would have at least expected some rudimentary discussion
> why this patch -- which seems to at least partially contradict 7c3c796
> (blame: drop blob data after passing blame to the parent, 2007-12-11)
> -- is not regressing on the intent of said commit.

It is regressing on the intent of said commit by _retaining_ blob data
that it is _sure_ to look at again because of already having this data
asked for again in the priority queue.  That's the point.  It only drops
the blob data for which it has no request queued yet.  But there is no
handle on when the request is going to pop up until it actually leaves
the priority queue: the priority queue is a heap IIRC and thus only
provides partial orderings.

>> diff --git a/builtin/blame.c b/builtin/blame.c
>> index 21f42b0..2596fbc 100644
>> --- a/builtin/blame.c
>> +++ b/builtin/blame.c
>> @@ -1556,7 +1556,8 @@ finish:
>>  }
>>  for (i = 0; i < num_sg; i++) {
>>  if (sg_origin[i]) {
>> -drop_origin_blob(sg_origin[i]);
>> +if (!sg_origin[i]->suspects)
>> +drop_origin_blob(sg_origin[i]);
>>  origin_decref(sg_origin[i]);
>>  }
>
> It would be good to mention in the commit message that this patch does not
> change anything for blobs with only one remaining reference (the current
> one) because origin_decref() would do the same job as drop_origin_blob
> when decrementing the reference counter to 0.

A sizable number of references but not blobs are retained and needed for
producing the information in the final printed output (at least when
using the default sequential output instead of --incremental or
--porcelaine or similar).

> In fact, I suspect that simply removing the drop_origin_blob() call
> might result in the exact same I/O pattern.

It's been years since I actually worked on the code but I'm still pretty
sure you are wrong.

-- 
David Kastrup
--
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: RFC: new git-splice subcommand for non-interactive branch splicing

[Johannes Schindelin]

Hi Adam,

On Fri, 27 May 2016, Adam Spiers wrote:

> Description
> ---
> 
> git-splice(1) non-interactively splices the current branch by removing
> a range of commits from within it and/or cherry-picking a range of
> commits into it.  It's essentially just a glorified wrapper around
> cherry-pick and rebase -i.

It sounds as if you could accomplish the same with

git checkout -b former-commits 
git checkout -b latter-commits 
git cherry-pick ..HEAD@{2}

> Next steps, and the future
> --
> 
> Obviously, I'd welcome thoughts on whether it would make sense to
> include this in the git distribution.

Far be I from discouraging you to work on these scripts, but I think that
a really good place for such subcommands is a separate repository, as you
have it already. There are already some rarely used subcommands in
libexec/git-core/ cluttering up the space and I would be reluctant to add
even more subcommands to the default Git installation delivered to every
user.

You can *always* just extend the PATH so that git-splice can be found;
Then `git splice ...` will do exactly what you want. That is e.g. how
git-flow works. (Of course I hope that you will maintain your scripts
much, much better than git-flow, i.e. not abandon all users).

> In the longer term however, I'd like to write two more subcommands:
> 
>   - git-transplant(1) which wraps around git-splice(1) and enables
> easy non-interactive transplanting of a range of commits from
> one branch to another.  This should be pretty straightforward
> to implement.

This is just cherry-pick with a range...

>   - git-explode(1) which wraps around git-transplant(1) and
> git-deps(1), and automatically breaks a linear sequence of commits
> into multiple smaller sequences, forming a commit graph where
> ancestry mirrors commit dependency, as mentioned above.  I expect
> this to be more difficult, and would probably write it in Python.

You mean something like Darcs on top of Git. Essentially, you want to end
up with an octopus merge of branches whose commits would conflict if
exchanged.

I implemented the logic for this in a shell script somewhere, so it is not
*all* that hard (Python not required). But I ended up never quite using it
because it turns out that in practice, the commit "dependency" (as defined
by the commit diffs) does not really reflect the true dependency.

For example, in my work to move large parts of rebase -i into a builtin, I
have an entire series of commits that do nothing else but prepare the
sequencer for rebase -i's functionality. Most of these commits touch
completely separate parts of the code, so they would make independent
branches in your git-explode command. Yet, that would destroy the story
that the patch series tells, as the natural flow would get lost.

Another major complication is that sometimes the "dependency as per the
diff" is totally bogus. Take for example Git for Windows' patches on top
of Git: there are a couple "sub branches" that add global variables to
environment.c. By the logic of the overlapping (or touching) hunks, these
sub branches should build on top of each other, right? But they are
logically completely independent.

So I think that this is a nice exercise, but in practice it will require a
human to determine which commits really depend on each other.

> Eventually, the utopia I'm dreaming about would become a reality and
> look something like this:
> 
> git checkout -b new-feature
> 
> while in_long_frenzied_period_of_hacking; do
> # don't worry too much about branch maintenance here, just hack
> git add ...
> git commit ...
> done
> 
> # Break lots of commits from new-feature into new topic branches:
> git explode
> 
> # List topic branches
> git work list

You would render me *really* impressed if you could come up with an
automated way to determine logical dependencies between patches.

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

Re: [PATCH] blame.c: don't drop origin blobs as eagerly

[Johannes Schindelin]

Hi David,

it is good practice to Cc: the original author of the code in question, in
this case Junio. I guess he sees this anyway, but that is really just an
assumption.

On Fri, 27 May 2016, David Kastrup wrote:

> When a parent blob already has chunks queued up for blaming, dropping
> the blob at the end of one blame step will cause it to get reloaded
> right away, doubling the amount of I/O and unpacking when processing a
> linear history.

It is obvious from your commit message that you have studied the code
quite deeply. To make it easier for the reader (which might be your future
self), it is advisable to give at least a little bit of introduction, e.g.
what the "parent blob" is.

I would *guess* that it is the blob corresponding to the same path in the
parent of the current revision, but that should be spelled out explicitly.

> Keeping such parent blobs in memory seems like a reasonable
> optimization.  It's conceivable that this may incur additional memory

This sentence would be easier to read if "It's conceivable that" was
simply deleted.

> pressure particularly when the history contains lots of merges from
> long-diverged branches.  In practice, this optimization appears to
> behave quite benignly,

Why not just stop here? I say that because...

> and a viable strategy for limiting the total amount of cached blobs in a
> useful manner seems rather hard to implement.

... this sounds awfully handwaving. Since we already have reference
counting, it sounds fishy to claim that simply piggybacking a global
counter on top of it would be "rather hard".

> In addition, calling git-blame with -C leads to similar memory retention
> patterns.

This is a red herring. Just delete it. I, for one, being a heavy user of
`git blame`, could count the number of times I used blame's -C option
without any remaining hands. Zero times.

Besides, -C is *supposed* to look harder. By that argument, you could read
all blobs in rev-list even when the user did not specify --objects
"because --objects leads to similar memory retention patterns". So: let's
just forget about that statement.

The commit message is missing your sign-off.

Also: is there an easy way to reproduce your claims of better I/O
characteristics? Something like a command-line, ideally with a file in
git.git's own history, that demonstrates the I/O before and after the
patch, would be an excellent addition to the commit message.

Further: I would have at least expected some rudimentary discussion why
this patch -- which seems to at least partially contradict 7c3c796 (blame:
drop blob data after passing blame to the parent, 2007-12-11) -- is not
regressing on the intent of said commit.

> diff --git a/builtin/blame.c b/builtin/blame.c
> index 21f42b0..2596fbc 100644
> --- a/builtin/blame.c
> +++ b/builtin/blame.c
> @@ -1556,7 +1556,8 @@ finish:
>   }
>   for (i = 0; i < num_sg; i++) {
>   if (sg_origin[i]) {
> - drop_origin_blob(sg_origin[i]);
> + if (!sg_origin[i]->suspects)
> + drop_origin_blob(sg_origin[i]);
>   origin_decref(sg_origin[i]);
>   }

It would be good to mention in the commit message that this patch does not
change anything for blobs with only one remaining reference (the current
one) because origin_decref() would do the same job as drop_origin_blob
when decrementing the reference counter to 0.

In fact, I suspect that simply removing the drop_origin_blob() call might
result in the exact same I/O pattern.

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

Re: [PATCH v4] upload-pack.c: use of parse-options API

[Matthieu Moy]

> Subject: [PATCH v4] upload-pack.c: use of parse-options API

I'd drop the "of" and say just "use parse-options API"

Antoine Queru  writes:

> Description for --stateless-rpc and --advertise-refs come from
> the commit 42526b4 (Add stateless RPC options to upload-pack,

Nit: s/the//

> +--advertise-refs::
> + When --advertise-refs is passed as a command line parameter only
> + the initial ref advertisement is output, and the program exits
> + immediately.  This fits with the HTTP GET request model, where
> + no request content is received but a response must be produced.

Another nit: mix space/tab in indentation (renders badly on some
configurations).

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] Require 0 context lines in git-blame algorithm

[David Kastrup]

Previously, the core part of git blame -M required 1 context line.
There is no rationale to be found in the code (one guess would be that
the old blame algorithm was unable to deal with immediately adjacent
regions), and it causes artifacts like discussed in the thread

---
 builtin/blame.c | 7 +++
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/builtin/blame.c b/builtin/blame.c
index 21f42b0..a3f6874 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -134,7 +134,7 @@ struct progress_info {
int blamed_lines;
 };
 
-static int diff_hunks(mmfile_t *file_a, mmfile_t *file_b, long ctxlen,
+static int diff_hunks(mmfile_t *file_a, mmfile_t *file_b,
  xdl_emit_hunk_consume_func_t hunk_func, void *cb_data)
 {
xpparam_t xpp = {0};
@@ -142,7 +142,6 @@ static int diff_hunks(mmfile_t *file_a, mmfile_t *file_b, 
long ctxlen,
xdemitcb_t ecb = {NULL};
 
xpp.flags = xdl_opts;
-   xecfg.ctxlen = ctxlen;
xecfg.hunk_func = hunk_func;
ecb.priv = cb_data;
return xdi_diff(file_a, file_b, , , );
@@ -980,7 +979,7 @@ static void pass_blame_to_parent(struct scoreboard *sb,
fill_origin_blob(>revs->diffopt, target, _o);
num_get_patch++;
 
-   if (diff_hunks(_p, _o, 0, blame_chunk_cb, ))
+   if (diff_hunks(_p, _o, blame_chunk_cb, ))
die("unable to generate diff (%s -> %s)",
oid_to_hex(>commit->object.oid),
oid_to_hex(>commit->object.oid));
@@ -1129,7 +1128,7 @@ static void find_copy_in_blob(struct scoreboard *sb,
 * file_p partially may match that image.
 */
memset(split, 0, sizeof(struct blame_entry [3]));
-   if (diff_hunks(file_p, _o, 1, handle_split_cb, ))
+   if (diff_hunks(file_p, _o, handle_split_cb, ))
die("unable to generate diff (%s)",
oid_to_hex(>commit->object.oid));
/* remainder, if any, all match the preimage */
-- 
2.7.4

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

Re: [PATCH v8 0/9] connect: various cleanups

[Torsten Bögershausen]

On 27.05.16 04:27, Mike Hommey wrote:
> Changes from v7:
> - Fixed comments.
> 
> Mike Hommey (9):
>   connect: document why we sometimes call get_port after
> get_host_and_port
>   connect: call get_host_and_port() earlier
>   connect: re-derive a host:port string from the separate host and port
> variables
>   connect: make parse_connect_url() return separated host and port
>   connect: group CONNECT_DIAG_URL handling code
>   connect: make parse_connect_url() return the user part of the url as a
> separate value
>   connect: change the --diag-url output to separate user and host
>   connect: actively reject git:// urls with a user part
>   connect: move ssh command line preparation to a separate function
> 
>  connect.c | 235 
> +-
>  t/t5500-fetch-pack.sh |  42 ++---
>  2 files changed, 170 insertions(+), 107 deletions(-)
> 
Is the whole series somewhere on a public repo ?
No major remarks so far, if possible, I would like
to have a look at the resulting connect.c

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

[PATCH v4] upload-pack.c: use of parse-options API

[Antoine Queru]

Option parsing now uses the parser API instead of a local parser.
Code is now more compact.

Description for --stateless-rpc and --advertise-refs come from
the commit 42526b4 (Add stateless RPC options to upload-pack,
receive-pack, 2009-10-30).

Signed-off-by: Antoine Queru 
Signed-off-by: Matthieu Moy 
---

Change since v3 :

Commit message is now well formated.
Updated documentation.

 Documentation/git-upload-pack.txt | 17 +--
 upload-pack.c | 59 +--
 2 files changed, 40 insertions(+), 36 deletions(-)

diff --git a/Documentation/git-upload-pack.txt 
b/Documentation/git-upload-pack.txt
index 0abc806..22ddf8a 100644
--- a/Documentation/git-upload-pack.txt
+++ b/Documentation/git-upload-pack.txt
@@ -9,8 +9,8 @@ git-upload-pack - Send objects packed back to git-fetch-pack
 SYNOPSIS
 
 [verse]
-'git-upload-pack' [--strict] [--timeout=] 
-
+'git-upload-pack' [--strict] [--timeout=] [--stateless-rpc]
+ [--advertise-refs] 
 DESCRIPTION
 ---
 Invoked by 'git fetch-pack', learns what
@@ -31,6 +31,19 @@ OPTIONS
 --timeout=::
Interrupt transfer after  seconds of inactivity.
 
+--stateless-rpc::
+   the programs now assume they may perform only a single read-write
+   cycle with stdin and stdout. This fits with the HTTP POST request
+   processing model where a program may read the request, write a
+   response, and must exit.
+
+--advertise-refs::
+   When --advertise-refs is passed as a command line parameter only
+   the initial ref advertisement is output, and the program exits
+   immediately.  This fits with the HTTP GET request model, where
+   no request content is received but a response must be produced.
+
+
 ::
The repository to sync from.
 
diff --git a/upload-pack.c b/upload-pack.c
index dc802a0..2d53c7b 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -14,8 +14,12 @@
 #include "sigchain.h"
 #include "version.h"
 #include "string-list.h"
+#include "parse-options.h"
 
-static const char upload_pack_usage[] = "git upload-pack [--strict] 
[--timeout=] ";
+static const char * const upload_pack_usage[] = {
+   N_("git upload-pack [options] "),
+   NULL
+};
 
 /* Remember to update object flag allocation in object.h */
 #define THEY_HAVE  (1u << 11)
@@ -817,11 +821,21 @@ static int upload_pack_config(const char *var, const char 
*value, void *unused)
return parse_hide_refs_config(var, value, "uploadpack");
 }
 
-int main(int argc, char **argv)
+int main(int argc, const char **argv)
 {
-   char *dir;
-   int i;
+   const char *dir;
int strict = 0;
+   struct option options[] = {
+   OPT_BOOL(0, "stateless-rpc", _rpc,
+N_("quit after a single request/response exchange")),
+   OPT_BOOL(0, "advertise-refs", _refs,
+N_("only the initial ref advertisement is output, 
program exits immediately")),
+   OPT_BOOL(0, "strict", ,
+N_("do not try /.git/ if  is no 
Git directory")),
+   OPT_INTEGER(0, "timeout", ,
+   N_("interrupt transfer after  seconds of 
inactivity")),
+   OPT_END()
+   };
 
git_setup_gettext();
 
@@ -829,40 +843,17 @@ int main(int argc, char **argv)
git_extract_argv0_path(argv[0]);
check_replace_refs = 0;
 
-   for (i = 1; i < argc; i++) {
-   char *arg = argv[i];
-
-   if (arg[0] != '-')
-   break;
-   if (!strcmp(arg, "--advertise-refs")) {
-   advertise_refs = 1;
-   continue;
-   }
-   if (!strcmp(arg, "--stateless-rpc")) {
-   stateless_rpc = 1;
-   continue;
-   }
-   if (!strcmp(arg, "--strict")) {
-   strict = 1;
-   continue;
-   }
-   if (starts_with(arg, "--timeout=")) {
-   timeout = atoi(arg+10);
-   daemon_mode = 1;
-   continue;
-   }
-   if (!strcmp(arg, "--")) {
-   i++;
-   break;
-   }
-   }
+   argc = parse_options(argc, argv, NULL, options, upload_pack_usage, 0);
+   
+   if (argc != 1)
+   usage_with_options(upload_pack_usage, options);
 
-   if (i != argc-1)
-   usage(upload_pack_usage);
+   if (timeout)
+   daemon_mode = 1;
 
setup_path();
 
-   dir = argv[i];
+   dir = argv[0];
 
if (!enter_repo(dir, strict))
die("'%s' does not appear to be a git repository", dir);
-- 
2.8.2.403.g31e708f

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a 

RFC: new git-splice subcommand for non-interactive branch splicing

[Adam Spiers]

Hi all,

I finally got around to implementing a new git subcommand which I've
wanted for quite a while.  I've called it git-splice.

Description
---

git-splice(1) non-interactively splices the current branch by removing
a range of commits from within it and/or cherry-picking a range of
commits into it.  It's essentially just a glorified wrapper around
cherry-pick and rebase -i.

Usage
-

Examples:

# Remove commit A from the current branch
git splice A^!

# Remove commits A..B from the current branch
git splice A..B

# Remove commits A..B from the current branch, and cherry-pick
# commits C..D at the same point
git splice A..B C..D

# Cherry-pick commits C..D, splicing them in just after commit A
git splice A C..D

# Remove first commit mentioning 'foo', and insert all commits
# in the 'elsewhere' branch which mention 'bar'
git splice --grep=foo -n1 HEAD -- --grep=bar HEAD..elsewhere

# Abort a splice which failed during cherry-pick or rebase
git splice --abort

# Resume a splice after manually fixing conflicts caused by
# cherry-pick or rebase
git splice --continue

N.B. Obviously this command rewrites history!  As with git rebase,
you should be aware of all the implications of history rewriting
before using it.

Code


Currently this is in alpha state:

  https://github.com/git/git/compare/master...aspiers:splice

and I reserve the right to rewrite the history of that branch in the
near future ;-)

I realise that the code does not yet conform to the coding standards
of the git project.  For example, it relies on non-POSIX bash
features, like arrays.  I would be happy to fix this if there is a
chance git-splice might be accepted for inclusion within the git
distribution.  (Presumably contrib/ is another possibility.)
Also, I haven't yet written a proper man page for it.

Motivation
--

I wrote git-splice as the next step in the journey towards being able
to implement a tool which automatically (or at least
semi-automatically) splits a linear sequence of commits into a commit
graph where ancestry exactly mirrors commit "dependency".[0]  In other
words, in this commit graph, a commit B would have commit A as an
ancestor if and *only* if commit B cannot cleanly apply without A
already being present in the branch.  As a corollary, if commit F
depends on D and E, but D and E are mutually independent, F would
need to depend on a merge commit which contains D and E.

Such a tool could be useful for a few reasons.  Firstly, large patch
series are much harder to review than single commits or small patch
series, but typical development workflows often lead to large patch
series.

For example, if I work privately on a new feature for some hours /
days / weeks, I will typically amass a bunch of commits which are not
all directly related to the new feature: there are often refactorings,
fixes for bugs discovered during development of the new feature, etc.

I doubt I'm the only git user not disciplined enough to maintain neat
branch organization for the whole of a long period of hacking!
i.e. religiously maintaining one branch per bugfix, one branch per
refactoring, and one branch for the new feature.[1]  Typically, tidying
up the branches comes a bit later, when I want to start feeding stuff
upstream for review.

Therefore being able to reduce the effort involved with breaking a
large patch series into smaller related chunks seems potentially very
useful.

As well as making reviews smaller easier, this allows both the reviews
and any corresponding CI to proceed in a more parallelized fashion.

Some review systems can implicitly discourage reviews of large patch
series, by treating each commit as a review in its own right and/or not
providing sophisticated support for patch series.  Gerrit is one
example; gitlab and GitHub are counter-examples.

I'm sure there are other use cases which I didn't think of yet.

Next steps, and the future
--

Obviously, I'd welcome thoughts on whether it would make sense to
include this in the git distribution.

In the longer term however, I'd like to write two more subcommands:

  - git-transplant(1) which wraps around git-splice(1) and enables
easy non-interactive transplanting of a range of commits from
one branch to another.  This should be pretty straightforward
to implement.

  - git-explode(1) which wraps around git-transplant(1) and
git-deps(1), and automatically breaks a linear sequence of commits
into multiple smaller sequences, forming a commit graph where
ancestry mirrors commit dependency, as mentioned above.  I expect
this to be more difficult, and would probably write it in Python.

Ideally, this tool would also be able to integrate with other
workflow management tools[1] in order to effectively create /
manage topic branches and track dependencies between them.

Eventually, the utopia I'm dreaming about would become a 

[WIP-PATCH 0/2] send-email: refactor the email parser loop

[Samuel GROOT]

While working on the new option `quote-email`, we needed to parse an
email file. Since the work is already done, but the parsing and data
processing are in the same loop, we wanted to refactor the parser, to
make the code more maintainable.

This is still WIP, and one of the main issue (and we need your
advice on that), is that around 30 tests will fail, and most of them
are false-negatives: to pass, they rely on the format of what is
displayed by `git send-email`, not only data.


For example, several tests will fail because they do a strict compare
between `git send-email`'s output and:

   (mbox) Adding cc: A  from line 'Cc: 
A, One'
   (mbox) Adding cc: One  from line 'Cc: 
A, One'

Though `git send-email` now outputs something like:

   (mbox) Adding cc: A  from line 'Cc: 
A'
   (mbox) Adding cc: One  from line 'Cc: 
One'

The new behavior is explained because parsing and processing are not
done in the same function, and processing cannot know the exact line
the data came from.

We can think of several solutions:

   1. Modify the parser, to give the script the knowledge of the exact
  line the data came from.

   2. Hack the tests: modify the script using the parser to artificially
  recreate the supposedly parsed line.
  (e.g. with a list.join(', ')-like function)

   3. Modify the tests to replace exact cmp by greps.


IMHO, we should consider option 3, since the tests should rely on data
rather than exact outputs. It also makes the tests more maintainable,
in the sense that they will be more resilient to future output
modifications.

What do you think?


   [WIP-PATCH 1/2] send-email: create email parser subroutine
   [WIP-PATCH 2/2] send-email: use refactored subroutine to parse patches

git-send-email.perl | 284 

  1 file changed, 164 insertions(+), 120 deletions(-)

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

[WIP-PATCH 1/2] send-email: create email parser subroutine

[Samuel GROOT]

Parsing and processing in send-email is done in the same loop.

To make the code more maintainable, we create two subroutines:
- `parse_email` to separate header and body
- `parse_header` to retrieve data from header

Signed-off-by: Samuel GROOT 
Signed-off-by: Tom RUSSELLO 
Signed-off-by: Matthieu MOY 
---
 git-send-email.perl | 105 
 1 file changed, 105 insertions(+)

diff --git a/git-send-email.perl b/git-send-email.perl
index 6958785..f33a083 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -1750,3 +1750,108 @@ sub body_or_subject_has_nonascii {
}
return 0;
 }
+
+sub parse_email {
+   my @header = ();
+   my @body = ();
+   my $fh = shift;
+
+   # First unfold multiline header fields
+   while (<$fh>) {
+   last if /^\s*$/;
+   if (/^\s+\S/ and @header) {
+   chomp($header[$#header]);
+   s/^\s+/ /;
+   $header[$#header] .= $_;
+   } else {
+   push(@header, $_);
+   }
+   }
+
+   # Now unfold the message body
+   while (<$fh>) {
+   push @body, $_;
+   }
+
+   return (@header, @body);
+}
+
+sub parse_header {
+   # Return variables
+   my $from = undef, $subject = undef;
+   my $date = undef, $message_id = undef;
+   my @to = (), @cc = (), @xh = ();
+   my %flags = ();
+
+
+   # Internal variables
+   my $input_format = undef;
+
+   foreach(@_) {
+   if (/^From /) {
+   $input_format = 'mbox';
+   next;
+   }
+   chomp;
+   if (!defined $input_format && /^[-A-Za-z]+:\s/) {
+   $input_format = 'mbox';
+   }
+
+   if (defined $input_format && $input_format eq 'mbox') {
+   if (/^Subject:\s+(.*)$/i) {
+   $subject = $1;
+   } elsif (/^From:\s+(.*)$/i) {
+   $from = $1;
+   } elsif (/^To:\s+(.*)$/i) {
+   foreach my $addr (parse_address_line($1)) {
+   push @to, $addr;
+   }
+   } elsif (/^Cc:\s+(.*)$/i) {
+   foreach my $addr (parse_address_line($1)) {
+   push @cc, $addr;
+   }
+   } elsif (/^Content-type:/i) {
+   $flags{"has_content_type"} = 1;
+   if (/charset="?([^ "]+)/) {
+   $flags{"body_encoding"} = 1;
+   }
+   push @xh, $_;
+   } elsif (/^MIME-Version/i) {
+   $flags{"has_mime_version"} = 1;
+   push @xh, $_;
+   } elsif (/^Message-Id: (.*)/i) {
+   $message_id = $1;
+   } elsif (/^Content-Transfer-Encoding: (.*)/i) {
+   $flags{"xfer_encoding"} = $1 if not defined 
$flags{"xfer_encoding"};
+   } elsif (/^Date:\s(.*)$/i) {
+   $date = $1;
+   } elsif (/^[-A-Za-z]+:\s+\S/) {
+   push @xh, $_;
+   }
+
+   } else {
+   # In the traditional
+   # "send lots of email" format,
+   # line 1 = cc
+   # line 2 = subject
+   # So let's support that, too.
+   $input_format = 'lots';
+   if (@cc == 0) {
+   push @cc, $_;
+   } elsif (!defined $subject) {
+   $subject = $_;
+   }
+   }
+   }
+
+   return (
+   "from" => $from,
+   "subject" => $subject,
+   "date" => $date,
+   "message_id" => $message_id,
+   "to" => [@to],
+   "cc" => [@cc],
+   "xh" => [@xh],
+   "flags" => {%flags}
+   );
+}
-- 
2.8.2.537.gb153d2a

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

[WIP-PATCH 2/2] send-email: use refactored subroutine to parse patches

[Samuel GROOT]

Use the two subroutines `parse_email` and `parse_header` introduced in
previous commit to parse patches.

Signed-off-by: Samuel GROOT 
Signed-off-by: Tom RUSSELLO 
Signed-off-by: Matthieu MOY 
---
 git-send-email.perl | 179 +---
 1 file changed, 59 insertions(+), 120 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index f33a083..7bb4a2d 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -161,7 +161,7 @@ my $re_encoded_word = 
qr/=\?($re_token)\?($re_token)\?($re_encoded_text)\?=/;
 # Variables we fill in automatically, or via prompting:
 my (@to,$no_to,@initial_to,@cc,$no_cc,@initial_cc,@bcclist,$no_bcc,@xh,
$initial_reply_to,$initial_subject,@files,
-   $author,$sender,$smtp_authpass,$annotate,$use_xmailer,$compose,$time);
+   $sender,$smtp_authpass,$annotate,$use_xmailer,$compose,$time);
 
 my $envelope_sender;
 
@@ -1431,117 +1431,57 @@ $subject = $initial_subject;
 $message_num = 0;
 
 foreach my $t (@files) {
-   open my $fh, "<", $t or die "can't open file $t";
-
-   my $author = undef;
-   my $sauthor = undef;
-   my $author_encoding;
-   my $has_content_type;
-   my $body_encoding;
-   my $xfer_encoding;
-   my $has_mime_version;
-   @to = ();
-   @cc = ();
-   @xh = ();
-   my $input_format = undef;
-   my @header = ();
$message = "";
$message_num++;
-   # First unfold multiline header fields
-   while(<$fh>) {
-   last if /^\s*$/;
-   if (/^\s+\S/ and @header) {
-   chomp($header[$#header]);
-   s/^\s+/ /;
-   $header[$#header] .= $_;
-   } else {
-   push(@header, $_);
-   }
-   }
-   # Now parse the header
-   foreach(@header) {
-   if (/^From /) {
-   $input_format = 'mbox';
-   next;
-   }
-   chomp;
-   if (!defined $input_format && /^[-A-Za-z]+:\s/) {
-   $input_format = 'mbox';
-   }
 
-   if (defined $input_format && $input_format eq 'mbox') {
-   if (/^Subject:\s+(.*)$/i) {
-   $subject = $1;
-   }
-   elsif (/^From:\s+(.*)$/i) {
-   ($author, $author_encoding) = 
unquote_rfc2047($1);
-   $sauthor = sanitize_address($author);
-   next if $suppress_cc{'author'};
-   next if $suppress_cc{'self'} and $sauthor eq 
$sender;
-   printf("(mbox) Adding cc: %s from line '%s'\n",
-   $1, $_) unless $quiet;
-   push @cc, $1;
-   }
-   elsif (/^To:\s+(.*)$/i) {
-   foreach my $addr (parse_address_line($1)) {
-   printf("(mbox) Adding to: %s from line 
'%s'\n",
-   $addr, $_) unless $quiet;
-   push @to, $addr;
-   }
-   }
-   elsif (/^Cc:\s+(.*)$/i) {
-   foreach my $addr (parse_address_line($1)) {
-   my $qaddr = unquote_rfc2047($addr);
-   my $saddr = sanitize_address($qaddr);
-   if ($saddr eq $sender) {
-   next if ($suppress_cc{'self'});
-   } else {
-   next if ($suppress_cc{'cc'});
-   }
-   printf("(mbox) Adding cc: %s from line 
'%s'\n",
-   $addr, $_) unless $quiet;
-   push @cc, $addr;
-   }
-   }
-   elsif (/^Content-type:/i) {
-   $has_content_type = 1;
-   if (/charset="?([^ "]+)/) {
-   $body_encoding = $1;
-   }
-   push @xh, $_;
-   }
-   elsif (/^MIME-Version/i) {
-   $has_mime_version = 1;
-   push @xh, $_;
-   }
-   elsif (/^Message-Id: (.*)/i) {
-   $message_id = $1;
-   }
-   elsif 

[PATCH] blame.c: don't drop origin blobs as eagerly

[David Kastrup]

When a parent blob already has chunks queued up for blaming, dropping
the blob at the end of one blame step will cause it to get reloaded
right away, doubling the amount of I/O and unpacking when processing a
linear history.

Keeping such parent blobs in memory seems like a reasonable
optimization.  It's conceivable that this may incur additional memory
pressure particularly when the history contains lots of merges from
long-diverged branches.  In practice, this optimization appears to
behave quite benignly, and a viable strategy for limiting the total
amount of cached blobs in a useful manner seems rather hard to
implement.  In addition, calling git-blame with -C leads to similar
memory retention patterns.
---
 builtin/blame.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/builtin/blame.c b/builtin/blame.c
index 21f42b0..2596fbc 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -1556,7 +1556,8 @@ finish:
}
for (i = 0; i < num_sg; i++) {
if (sg_origin[i]) {
-   drop_origin_blob(sg_origin[i]);
+   if (!sg_origin[i]->suspects)
+   drop_origin_blob(sg_origin[i]);
origin_decref(sg_origin[i]);
}
}
-- 
2.7.4

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

Re: [PATCH] mingw: make isatty() recognize MSYS2's pseudo terminals (/dev/pty*)

[Johannes Schindelin]

Hi Junio,

On Thu, 26 May 2016, Junio C Hamano wrote:

> Johannes Schindelin  writes:
> 
> > I do not see this patch in 'pu'... Anything I can do to get this into
> > 'master' eventually?
> 
> The reason why I left it in my inbox was because I couldn't tell if
> this was a final submission with concensus among Git developers on
> Windows, or I should be giving a chance to comment to some folks who
> work on Windows port but are not necessarily closely communicating
> with you.
> 
> If the message were Cc'ed to J6t, I would probably have queued it on
> 'pu' and marked it as "Will merge after waiting for a few days" in
> What's cooking.

Hannes does not use Git for Windows' SDK, but instead uses an old
MSys-based development environment that predates even msysGit (which was
the SDK used to develop Git for Windows 1.x). His environment most
notably does *not* use MinTTY, and therefore is unaffected by the bug that
this patch fixes.

That was the entire reasoning why I did not ask Hannes to review this
patch: it is unlikely that he knows about the issue, let alone about the
correctness of the implementation of this patch.

The implementation relies on some really low-level knowledge of the
Windows internals, which Karsten reverse-engineered and I verified.

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

[PATCH v2] worktree: allow "-" short-hand for @{-1} in add command

[Jordan DE GEA]

Since `git worktree add` uses `git checkout` when `[]` is used,
and `git checkout -` is already supported, it makes sense to allow the
same shortcut in `git worktree add`.

Signed-off-by: Matthieu Moy 
Signed-off-by: Jordan DE GEA 
---
Changes since v1:
  - improved tests.
  - improved documentation.

 Documentation/git-worktree.txt |  3 ++-
 builtin/worktree.c |  3 +++
 t/t2025-worktree-add.sh| 18 ++
 3 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index c622345..8358a3e 100644
--- a/Documentation/git-worktree.txt
+++ b/Documentation/git-worktree.txt
@@ -48,7 +48,8 @@ add  []::
 
 Create `` and checkout `` into it. The new working directory
 is linked to the current repository, sharing everything except working
-directory specific files such as HEAD, index, etc.
+directory specific files such as HEAD, index, etc. `-` may also be 
+specified as ``; it is synonymous with `@{-1}`.
 +
 If `` is omitted and neither `-b` nor `-B` nor `--detached` used,
 then, as a convenience, a new branch based at HEAD is created automatically,
diff --git a/builtin/worktree.c b/builtin/worktree.c
index d8e3795..d800d47 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -340,6 +340,9 @@ static int add(int ac, const char **av, const char *prefix)
path = prefix ? prefix_filename(prefix, strlen(prefix), av[0]) : av[0];
branch = ac < 2 ? "HEAD" : av[1];
 
+   if (!strcmp(branch, "-"))
+   branch = "@{-1}";
+
opts.force_new_branch = !!new_branch_force;
if (opts.force_new_branch) {
struct strbuf symref = STRBUF_INIT;
diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh
index 3acb992..c4f5177 100755
--- a/t/t2025-worktree-add.sh
+++ b/t/t2025-worktree-add.sh
@@ -18,6 +18,24 @@ test_expect_success '"add" an existing empty worktree' '
git worktree add --detach existing_empty master
 '
 
+test_expect_success '"add" using shorthand - fails when no previous branch' '
+   echo "fatal: invalid reference: @{-1}" >expect &&
+   test_must_fail git worktree add existing_short - 2>actual &&
+   test_cmp expect actual
+'
+
+test_expect_success '"add" using - shorthand' '
+   git checkout -b newbranch &&
+   echo hello >myworld &&
+   git add myworld &&
+   git commit -m myworld &&
+   git checkout master &&
+   git worktree add short-hand - &&
+   echo refs/heads/newbranch >expect &&
+   git -C short-hand rev-parse --symbolic-full-name HEAD >actual &&
+   test_cmp expect actual
+'
+
 test_expect_success '"add" refuses to checkout locked branch' '
test_must_fail git worktree add zere master &&
! test -d zere &&
-- 
2.7.4 (Apple Git-66)

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

Re: [PATCH] worktree: allow "-" short-hand for @{-1} in add command

[Jordan DE GEA]

>> +test_expect_success '"add" using shorthand - fails when no previous branch' 
>> '
>> +test_must_fail git worktree add existing -
>> +'
> 
> Just an observation, but the error message we would see here might
> be interesting.

Of course, that’s useful to be sure of the error, I will do in next preroll.

> 
>> +branch=$(cd short-hand && git rev-parse --symbolic-full-name HEAD) &&
>> +test "$branch" = refs/heads/newbranch &&
>> +cd ..
> 
> If any of the command between "cd short-hand" and "cd .." failed,
> after correcting the broken &&-chain, the next test will end up
> running in short-hand directory, which it is not expecting.  A
> canonical way to avoid this problem is to replace the above with:
> 
>   ...
>git worktree add short-hand - &&
>(
>   cd short-hand &&
>...
>test "$branch" = refs/heads/newbranch
>   )
> 
> In this particular case, alternatively, you could also do something
> like this:
> 
>git worktree add short-hand - &&
>   echo refs/heads/newbranch >expect &&
>   git -C short-hand rev-parse --symbolic-full-name HEAD >actual &&
>   test_cmp expect actual
Yes, that’s a good idea. I take these lines. 

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

Re: [PATCH v2 16/22] i18n: rebase-interactive: mark comments of squash for translation

[Vasco Almeida]

Às 22:35 de 26-05-2016, Junio C Hamano escreveu:
> Vasco Almeida  writes:
> 
>> Helper functions this_nth_commit_message and skip_nth_commit_message
>> replace the previous method of making the comment messages (such as
>> "This is the 2nd commit message:") aided by nth_string helper function.
>> This step was taken as a workaround to enabled translation of entire
>> sentences. However, doesn't change any text seen in English by the user,
>> except for string "The first commit's message is:" which was changed to
>> match the style of other instances.
> 
> If only the original were written as "This is the commit message
> $N", you didn't have to do a lot of work like this, but such is
> life.  Thanks for working on this.
> 
I did that work, assuming it was important and we wanted to keep the
existing messages as much as possible. Unless they are out of place, as
it felt about the message I changed. I've assumed that based on the user
being used to those messages and eventual scripts that expected those
messages.
Is my assumption right? Anyway, the work is already done.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 13/22] i18n: git-sh-setup.sh: mark strings for translation

[Vasco Almeida]

Às 22:46 de 26-05-2016, Junio C Hamano escreveu:
> Vasco Almeida  writes:
> 
>> >  require_work_tree_exists () {
>> > +  program_name=$0
>> >if test "z$(git rev-parse --is-bare-repository)" != zfalse
>> >then
>> > -  die "fatal: $0 cannot be used without a working tree."
>> > +  die "$(gettext "fatal: \$program_name cannot be used without a 
>> > working tree.")"
>> >fi
>> >  }
> This is probably quite a minor point, but I'd prefer if clobbering
> the variable program_name is done between "then" and "fi", i.e. when
> we know we are going to die, so the caller would not care.  Because
> we are not in control of the caller's namespace use, and we do not
> want bash-ism "local" here, that is the best we could do to make it
> safer.
> 
I was not aware about this issue. I agree with you and I'll fix this and
other instances you mentioned, in the next re-roll.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 14/22] i18n: rebase-interactive: mark strings for translation

[Vasco Almeida]

Às 22:36 de 26-05-2016, Junio C Hamano escreveu:
> Vasco Almeida  writes:
> 
>> @@ -222,9 +223,10 @@ has_action () {
>>  }
>>  
>>  is_empty_commit() {
>> -tree=$(git rev-parse -q --verify "$1"^{tree} 2>/dev/null ||
>> -die "$1: not a commit that can be picked")
>> -ptree=$(git rev-parse -q --verify "$1"^^{tree} 2>/dev/null ||
>> +sha1=$1
>> +tree=$(git rev-parse -q --verify "$sha1"^{tree} 2>/dev/null ||
>> +die "$(eval_gettext "\$sha1: not a commit that can be picked")")
>> +ptree=$(git rev-parse -q --verify "$sha1"^^{tree} 2>/dev/null ||
>>  ptree=4b825dc642cb6eb9a060e54bf8d69288fbee4904)
>>  test "$tree" = "$ptree"
>>  }
> 
> Both of the two callsites of this function use the variable $sha1,
> and at least one of them uses that variable after this function
> returns, but they pass it as the first parameter to this function,
> so the assignment added by this patch does not break them, which is
> good.
> 
I didn't know that. I can change sha1=$1 to local_sha1=$1 or _sha1=$1 (I
don't know what is the convention here) if that is safer, avoiding using
the bash-ism "local" keyword, and preventing future distractions.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] worktree: allow "-" short-hand for @{-1} in add command

[Matthieu Moy]

Junio C Hamano  writes:

> Jordan DE GEA  writes:
>
>> +branch=$(cd short-hand && git rev-parse --symbolic-full-name HEAD) &&
>> +test "$branch" = refs/heads/newbranch &&
>> +cd ..
>
> If any of the command between "cd short-hand" and "cd .." failed,
> after correcting the broken &&-chain, the next test will end up
> running in short-hand directory, which it is not expecting.  A
> canonical way to avoid this problem is to replace the above with:
>
>   ...
> git worktree add short-hand - &&
> (
>   cd short-hand &&
> ...
> test "$branch" = refs/heads/newbranch
>   )

Actually, $(...) implicitly does a subshell, so the "cd .." was just
useless.

>   git -C short-hand rev-parse --symbolic-full-name HEAD >actual &&

Indeed, git -C is an even better way to say "cd .. && git ..."

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
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: [RFC-PATCH 1/2] send-email: new option to quote an email and reply to

[Matthieu Moy]

Samuel GROOT  writes:

> On 05/25/2016 08:31 PM, Matthieu Moy wrote:
>> So, a possible UI would be:
>>
>>   git send-email --in-reply-to= => just set In-Reply-To: field.
>>
>>   git send-email --in-reply-to= => set In-Reply-To, To and Cc.
>>
>>   git send-email --in-reply-to= --cite => in addition, add the
>> body of the message quoted with '> '.
>>
>>   git send-email --in-reply-to= --fetch => fetch and do like 
>> using the default configuration for fetch.
>
> We designed a similar UI, except for the --fetch option:
>
> We wanted to try to fetch the email from a distant server (e.g. gmane)
> if that server address was set in the config file, and populate the
> To:/Cc: fields.
>
> If the file cannot be downloaded, or server address not set, just fill
> the Reply-to header.

I'm not sure this is right. I'd typically configure gmane in my
user-wide config file, so I'd have it configured all the time, but I may
not always want to fetch from it (e.g. replying to a message which is
not on a mailing-list, or if gmane is down, or ...).

Fetching by default would clearly work in most cases, but for the few
remaning cases it may be painful for the user if the only way to disable
fetching is to remove the configuration from the config file.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
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: t7800 test failure

[Matthieu Moy]

David Aguilar  writes:

> Another alternative is that we can compile our own
> "git-readlink" and "git-mktemp" programs and use those instead,
> but that seems like a big maintenance burden compared to the
> relative simplicity of the test-suite workarounds.

Not _that_ big burden as we already have the necessary infrastructure:
git-mktemp would be t/helper/test-mktemp.c already available to tests as
test-mktemp, and it would be easy to do it for readlink too.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] userdiff: add built-in pattern for CSS

[William Duclot]

Junio C Hamano  writes:
> William Duclot  writes:
>
>> As the CSS pattern
>> does not deal with letters at all it seemed sensible to me to follow
>> the example of the HTML pattern, which use PATTERNS().
> 
> Did you notice that HTML pattern has to do an [Hh] that would be
> unnecessary if it chose to use IPATTTERN()?
> 
> You do not have to ask a person, but instead ask the history.
> IPATTERN() was added at 909a5494 (userdiff.c: add builtin fortran
> regex patterns, 2010-09-10) when adding fortran support.  Anything
> that existed before, including HTML, did [A-Za-z] when they could
> have done [a-z] if IPATTERN() existed back then.

I hadn't noticed that the HTML pattern was older, indeed

>>>  - In our codebase, we format multi-line comments in a particular
>>>way, namely
>>> 
>>>/*
>>>  * A multi-line comment begins with slash asterisk
>>>  * on its own line, and its closing asterisk slash
>>>  * also is on its own line.
>>>  */
>>
>> I take good note of that. I took example on the fortran pattern
>> comment, should I fix it too while I'm at it?
> 
> Not "while you are at it".
> 
> Making existing things better is welcome but such a change shouldn't
> be mixed with addition of new things.  You can do it as a separate
> patch, probably as a preliminary clean-up before your change, if you
> want to.

OK !


Johannes Sixt  writes:
> Am 24.05.2016 um 16:25 schrieb William Duclot:
>> +PATTERNS("css",
>> + "^([^,{}]+)((,[^}]*\\{)|([ \t]*\\{))$",
> 
> This hunk header pattern is a bit too restrictive for my taste. Find
> below a few more test cases that you should squash in. One case fails
> because only the first CSS selector is picked up, for which I do not
> see a reason.
> 
> Another case fails because the opening brace is not on the line with
> the CSS selectors.

Yes, it seems you're right !

> I think what the hunk header pattern should do is:
> 
> 1. reject lines containing a colon (because that are properties)
> 2. if a line begins with a name in column 1, pick the whole line
> 
> See the cpp patterns: a pattern beginning with ! is a "reject" pattern.

That may be a good idea, I will look into that

> diff --git a/t/t4018/css-brace-in-col-1 b/t/t4018/css-brace-in-col-1
> new file mode 100644
> index 000..7831577
> --- /dev/null
> +++ b/t/t4018/css-brace-in-col-1
> @@ -0,0 +1,5 @@
> +RIGHT label.control-label
> +{
> +margin-top: 10px!important;
> +border : 10px ChangeMe #C6C6C6;
> +}
> diff --git a/t/t4018/css-rule b/t/t4018/css-common
> similarity index 100%
> rename from t/t4018/css-rule
> rename to t/t4018/css-common
> diff --git a/t/t4018/css-long-selector-list b/t/t4018/css-long-selector-list
> new file mode 100644
> index 000..7ccd25d
> --- /dev/null
> +++ b/t/t4018/css-long-selector-list
> @@ -0,0 +1,6 @@
> +p.header,
> +label.control-label,
> +div ul#RIGHT {
> +margin-top: 10px!important;
> +border : 10px ChangeMe #C6C6C6;
> +}
> diff --git a/t/t4018/css-prop-sans-indent b/t/t4018/css-prop-sans-indent
> new file mode 100644
> index 000..a9e3c86
> --- /dev/null
> +++ b/t/t4018/css-prop-sans-indent
> @@ -0,0 +1,5 @@
> +RIGHT, label.control-label {
> +margin-top: 10px!important;
> +padding: 0;
> +border : 10px ChangeMe #C6C6C6;
> +}
> diff --git a/t/t4018/css-short-selector-list
> b/t/t4018/css-short-selector-list
> new file mode 100644
> index 000..6a0bdee
> --- /dev/null
> +++ b/t/t4018/css-short-selector-list
> @@ -0,0 +1,4 @@
> +label.control, div ul#RIGHT {
> +margin-top: 10px!important;
> +border : 10px ChangeMe #C6C6C6;
> +}
> --
> 2.9.0.rc0.40.gb3c1388

Thanks for the test cases, I'll look into that as soon as I have 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

Re: [RFC] Triangular Workflow: user friendly full implementation

[Philip Oakley]

From: "Jordan DE GEA" 

We are working on full implementation of triangular workflow feature.
For now, the main options available are:
- branch..pushRemote
- remote.pushDefault
And only setable by hands.

As it can be difficult to understand, here is what we want to do.


Context:
- One main remote repository, e.g. git/git.
- A remote fork (e.g. a GitHub fork) of git/git, e.g. me/git.
- A local clone of me/git on the machine

Purposes:
- the local branch master has to fetch to git/git by default
- the local branch master has to push to me/git by default

Configuration wanted:
- Add a remote to git/git e.g. `git remote add ...`
- Set the fetch remote for branch as default.

For now, we can do that by setting:
- branch..remote to git/git
- branch..pushRemote to me/git
But many options set `branch..remote`, a suitable solution is to
implement an option for the fetch for example.


Here is what we want to implement:

1.
a. add the config var: remote.fetchDefault
b. add the config var: branch..fetchRemote
c. add `git fetch --set-default` in order to set remote.fetchDefault
d. add `git fetch --set-remote` in order to set branch..fetchRemote
e. add `git pull --set-default` in order to set remote.fetchDefault
f. add `git pull --set-remote` in order to set branch..fetchRemote

2.
a. add `git push --set-default` in order to set remote.pushDefault
b. add `git push --set-remote` in order to set branch..pushRemote


What's your opinion about this feature ?

For me, the first step would be to actually document a (the?) Triangular 
Workflow in the documentation, so we are all taking about the same broad 
method.


At the moment there is a choice (assuming a ithub like service) of either 
clone and then fork, or fork and clone the fork, which leave the user with 
different fixups of their config's being required, so describing the easier 
one would help folk.


Likewise there are missing terms such as for the third place (the personal 
fork) that is neither the upstream, nor the local repo. Making sure the 
terminology is crisp and clean will greatly ease any implementation issues. 
And then there are the possible workflows...


--
Philip
(sorry for the rushed message, a long weekend beckons) 


--
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: RFC: dynamic "auto" date formats

[Junio C Hamano]

On Thu, May 26, 2016 at 11:53 PM, Linus Torvalds
 wrote:
>
> So I think what I really would like to see is more like a reverse
> "approxidate" that gives the date in human terms.

Yeah, "human" was the word I was looking for while composing my response.

I am sure somebody will write the reverse approxidate that utters "tea
time" when
it is appropriate, if only just for fun. I can't wait ;-)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] add: add --chmod=+x / --chmod=-x options

[Junio C Hamano]

Edward Thomson  writes:

> However I do not think that this is a common enough action that it needs
> to be made automatic such that when I `git add foo.rb` it is
> automatically made executable.  I think that the reduced complexity of
> having a single mechanism to control executability (that being the
> execute mode in the index or a tree) is preferable to a gitattributes
> based mechanism, at least until somebody else makes a cogent argument
> that the gitattributes approach would be helpful for them.  :)

It wasn't a "having to specify it every time sucks; you must do this
way instead" at all.  I was just gauging if it would be a viable idea
for a follow-up series to complement your patch.

Thanks.
--
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: t7610-mergetool.sh test failure

[Junio C Hamano]

Jeff King  writes:

> The only one I can think of is that if something leaves cruft in
> $TMPDIR, it could affect later tests that want to `git add`
> indiscriminately.

Or "git ls-files -u", "git clean", etc.  I'd mostly worry about a
failed test in which a program dies without a chance to clean up
after itself, and letting the cruft affecting the next test.

> OTOH, I do not think putting things in /tmp is hurting anything. I was
> mostly just surprised by it.

Moving TMPDIR into somewhere under t/ would force us to do more work
to clean things up, and it would probably be a good thing in the
longer term.

I just checked my /tmp, and I see a lot of directories whose name
look like mktemp generated one, with a single socket 's' in them.  I
wouldn't be surprised if they turn out to be from our tests that
expect failure, killing a daemon that does not properly clean after
itself.  People probably would not notice if they are in /tmp, and
if we moved TMPDIR to the trash, we still wouldn't (because running
tests successfully without "-d" option will remove the trash
directory at the end), but if it were dropped somewhere in the
source tree, we have a better chance of noticing it.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] format_commit_message: honor `color=auto` for `%C(auto)`

[Junio C Hamano]

Jeff King  writes:

> I suspect Junio can just tweak that while applying, unless there's
> another reason to re-roll.
>
> (Also for anybody watching, Ed did not just make up my signoff; I gave
> it to him off-list).

Understood.  Thanks.
--
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: RFC: dynamic "auto" date formats

[Junio C Hamano]

Linus Torvalds  writes:

> And no, I'm not at all sure that the 24-hour cut-off is the right
> thing, but it didn't seem completely crazy either. I tend to like the
> relative date format when it is "19 minutes ago" vs "2 hours ago", at
> some point it's long enough ago that it's more useful to know "Tuesday
> at 3pm" than about how long ago it was.
>
> (And yes, it would be even better to have the "short term relative
> date" turn into a "medium-term 'day of the week at time x'" and then
> turn into "full date" when it's more than a week ago, but this patch
> only has the two modes of "short term" and "long term" and nothing in
> between).

While I do not think this has much to do with "auto", other than
that it changes the representation depending on how far back the
time is to match the taste of Linus automatically, I think the
observation you made about the relative uselessness of "relative in
the long past" is real.  "6 years ago" that does not say if it was
in the morning and that does not even say if it was in the summer
is losing a bit too much information.

Your message made me realize another thing I feel while viewing
"relative in the long past" output.  In "git log --date=relative"
output (especially when the log is "limited" in some way, like with
a pathspec, --grep, -S, etc.) that shows multiple commits, all of
which are labeled "6 years ago", they make me wonder how they are
related to each other chronologically.  Perhaps I am seeing 6
commits, but the earlier four was made all within 20 minutes, and
the fifth one three days later, and the final one a month later,
which may indicate that the first four was the initial round of a
topic, with two "oops, this is a fix" follow-up patches that are
related in one area.  All of them being labeled "6 years ago" would
not give such a clue.

Which makes me wonder if another variant is useful (or at least
"interesting").  What if we chose format according to this rule?

0. Set the "reference time" to the current time.

1. Do get_revision() to grab one commit to show.

2. Show that commit, using timeformat determined as:
   a. if its time is close to the "reference time", then use
  "N hours M minues before that" format;
   b. otherwise, use the default time format;

3. Update the "reference time" to the timestamp of the commit
   we just showed.

4. Go back to 1.

--
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: [RFC/PATCH 2/2] log: add "--no-show-signature" command line option

[Mehul Jain]

On Thu, May 26, 2016 at 10:52 PM, Junio C Hamano  wrote:
> Jeff King  writes:
>
>> On Thu, May 26, 2016 at 06:36:47PM +0530, Mehul Jain wrote:
>>
>>> If "log.showsignature=true", then there is no way to override it using
>>> command line switch.
>>>
>>> Teach "git log" and "git show" about "--no-show-signature" command line
>>> option.
>>
>> I think this is teaching all of the revision machinery about it (which
>> is a good thing).
>
> I agree that the proposed commit log message should be updated to
> say so.
>
> Because we do not want .showsignature configuration to affect
> rev-list nor format-patch, and we will not make "--show-sig" the
> default for them either.  From that point of view, there is no
> reason for them to know about the "--no-show-signature" option.
>
> The only reason why teaching the "--no-show-signature" option to
> these commands is a good idea is because it would help people who
> create an alias with "--show-sig" in early part of the command line,
> e.g.
>
> [alias] fp = format-patch --show-signature
>
> by allowing them to countermand with --no-show-signature, i.e.
>
> $ git fp --no-show-signature ...
>
> If we are updating the log message in the final submission of this
> patch, we'd want it to be clear that the presence of this option is
> not an excuse to introduce .showsignature that affects rev-list
> later to make sure we do not have to waste our time rejecting such a
> patch in the future.

Currently, with the [patch 1/2], only git-show, git-log, git-whatchanged
and git-reflog are able to learn about log.showsignature config variable.
But commands which will learn about "--no-show-signature" with
[patch 2/2] are notably a super-set of above mentioned commands.
Introduction of this option should not give an impression that we might
need log.showSignature for commands like git-format-patch etc, and
it will definitely be a wise decision to convey the same in the commit
message of this patch. I will do the necessary change.

Just out of curiosity, I was thinking that we might be able to teach
"--no-show-signature" option only to git-show, git-log, git-whatchanged
and git-reflog. To do this we can introduce a new member
"no_show_signature" in struct rev_info, and use this variable further
to modify the value of value of "rev.show_signature" after init_revision()
is called. This way we can selectively decide which commands should
learn about "--no-show-signature". This may be a bad idea because
we will have two variables in rev_info, for option --[no]-show-signature.
Any thoughts?

Thanks,
Mehul
--
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: [RFC/PATCH 1/2] log: add "log.showsignature" configuration variable

[Mehul Jain]

On Thu, May 26, 2016 at 10:29 PM, Jeff King  wrote:
> On Thu, May 26, 2016 at 06:36:46PM +0530, Mehul Jain wrote:
> The documentation here mentions "log" and "show". But I think this will
> affect other programs, too, including "whatchanged" and "reflog". Those
> ones are probably good, but the documentation is a little misleading (I
> think other options just say "git-log and related commands" or
> something).

Yes, the documentation is misleading. As you have mentioned, this
config variable will affect git-log, git-show, git-whatchanged and git-reflog.
I will mention them in the documentation.

>> diff --git a/t/t4202-log.sh b/t/t4202-log.sh
>> index 128ba93..36be9a1 100755
>> --- a/t/t4202-log.sh
>> +++ b/t/t4202-log.sh
>> @@ -890,6 +890,25 @@ test_expect_success GPG 'log --graph --show-signature 
>> for merged tag' '
>>   grep "^| | gpg: Good signature" actual
>>  '
>>
>> +test_expect_success GPG 'log.showsignature=true behaves like 
>> --show-signature' '
>> + git checkout -b test_sign master &&
>> + echo foo >foo &&
>> + git add foo &&
>> + git commit -S -m signed_commit &&
>> + test_config log.showsignature true &&
>> + git log -1 signed >actual &&
>> + test_i18ngrep "gpg: Signature made" actual &&
>> + test_i18ngrep "gpg: Good signature" actual
>> +'
>
> You can see in the context that we do not use test_i18ngrep for finding
> gpg output in existing tests. I'm not sure if the new tests should be
> consistent, or if they should be changed to use test_i18ngrep. I don't
> think it's actually doing anything here, though. It's used with a
> git-specific GETTEXT_POISON flag that tweaks the output generated by
> git, but not by sub-programs like gpg.

There was no real motivation behind usage of test_i18ngrep. Certainly,
usage of grep will fit in the context.

>> +test_expect_success GPG '--show-signature overrides 
>> log.showsignature=false' '
>> + test_when_finished "git reset --hard && git checkout master" &&
>> + git config log.showsignature false &&
>
> Should this be test_config?

Noted.

Thanks,
Mehul
--
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