Re: [PATCH v8 00/41] libify apply and use lib in am, part 2

[Christian Couder]

On Wed, Jul 27, 2016 at 6:24 PM, Junio C Hamano  wrote:
> Christian Couder  writes:
>
>>> The early part up to and including "apply: make some parsing
>>> functions static again" looked good and we could treat them as a
>>> continuation of the earlier cc/apply-introduce-state topic, which
>>> has been merged to the 'master' already.
>>
>> Ok, is it ok for you to just pick up this early part, or do you prefer
>> me to resend it (maybe with the last patch on top of it)?
>
> Either would be fine for _me_, but as the original thread is now
> about a month old, a final reroll to give people the last chance to
> comment on them would not hurt.

Ok, everything that was in v8 has just been rerolled in v9.
--
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 00/41] libify apply and use lib in am, part 2

[Junio C Hamano]

Christian Couder  writes:

>> I finally found time to get back to finish reviewing this.
>
> Great, thanks!

No, thank _you_ for working on this.

>> The early part up to and including "apply: make some parsing
>> functions static again" looked good and we could treat them as a
>> continuation of the earlier cc/apply-introduce-state topic, which
>> has been merged to the 'master' already.
>
> Ok, is it ok for you to just pick up this early part, or do you prefer
> me to resend it (maybe with the last patch on top of it)?

Either would be fine for _me_, but as the original thread is now
about a month old, a final reroll to give people the last chance to
comment on them would not hurt.
--
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 00/41] libify apply and use lib in am, part 2

[Christian Couder]

On Tue, Jul 26, 2016 at 11:18 PM, Junio C Hamano  wrote:
> Christian Couder  writes:
>
>> Sorry if this patch series is still long. I can split it into two or
>> more series if it is prefered.
>> ...
>> Christian Couder (41):
>>   apply: make some names more specific
>>   apply: move 'struct apply_state' to apply.h
>>   builtin/apply: make apply_patch() return -1 or -128 instead of
>> die()ing
>>   builtin/apply: read_patch_file() return -1 instead of die()ing
>>   builtin/apply: make find_header() return -128 instead of die()ing
>>   builtin/apply: make parse_chunk() return a negative integer on error
>>   builtin/apply: make parse_single_patch() return -1 on error
>>   builtin/apply: make parse_whitespace_option() return -1 instead of
>> die()ing
>>   builtin/apply: make parse_ignorewhitespace_option() return -1 instead
>> of die()ing
>>   builtin/apply: move init_apply_state() to apply.c
>>   apply: make init_apply_state() return -1 instead of exit()ing
>>   builtin/apply: make check_apply_state() return -1 instead of die()ing
>>   builtin/apply: move check_apply_state() to apply.c
>>   builtin/apply: make apply_all_patches() return 128 or 1 on error
>>   builtin/apply: make parse_traditional_patch() return -1 on error
>>   builtin/apply: make gitdiff_*() return 1 at end of header
>>   builtin/apply: make gitdiff_*() return -1 on error
>>   builtin/apply: change die_on_unsafe_path() to check_unsafe_path()
>>   builtin/apply: make build_fake_ancestor() return -1 on error
>>   builtin/apply: make remove_file() return -1 on error
>>   builtin/apply: make add_conflicted_stages_file() return -1 on error
>>   builtin/apply: make add_index_file() return -1 on error
>>   builtin/apply: make create_file() return -1 on error
>>   builtin/apply: make write_out_one_result() return -1 on error
>>   builtin/apply: make write_out_results() return -1 on error
>>   builtin/apply: make try_create_file() return -1 on error
>>   builtin/apply: make create_one_file() return -1 on error
>>   builtin/apply: rename option parsing functions
>>   apply: rename and move opt constants to apply.h
>>   Move libified code from builtin/apply.c to apply.{c,h}
>>   apply: make some parsing functions static again
>>   environment: add set_index_file()
>>   write_or_die: use warning() instead of fprintf(stderr, ...)
>>   apply: add 'be_silent' variable to 'struct apply_state'
>>   apply: make 'be_silent' incompatible with 'apply_verbosely'
>>   apply: don't print on stdout when be_silent is set
>>   usage: add set_warn_routine()
>>   usage: add get_error_routine() and get_warn_routine()
>>   apply: change error_routine when be_silent is set
>>   builtin/am: use apply api in run_apply()
>>   apply: use error_errno() where possible
>
> I finally found time to get back to finish reviewing this.

Great, thanks!

> The early part up to and including "apply: make some parsing
> functions static again" looked good and we could treat them as a
> continuation of the earlier cc/apply-introduce-state topic, which
> has been merged to the 'master' already.

Ok, is it ok for you to just pick up this early part, or do you prefer
me to resend it (maybe with the last patch on top of it)?

> I got an impression that the patches in the remainder of the series
> were not as polished as the earlier ones, except for the last one,
> which should be reordered and be part of the early preparation step
> if possible.

I can resend just the last patch rebased on top of the early part once
you have picked up the early part.

And yeah the remainder has not been reviewed much. I will rework it as
you suggest in your other emails about specific pathes, and then
resend it in a "part 3" patch series.

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 v8 00/41] libify apply and use lib in am, part 2

[Junio C Hamano]

Christian Couder  writes:

> Sorry if this patch series is still long. I can split it into two or
> more series if it is prefered.
> ...
> Christian Couder (41):
>   apply: make some names more specific
>   apply: move 'struct apply_state' to apply.h
>   builtin/apply: make apply_patch() return -1 or -128 instead of
> die()ing
>   builtin/apply: read_patch_file() return -1 instead of die()ing
>   builtin/apply: make find_header() return -128 instead of die()ing
>   builtin/apply: make parse_chunk() return a negative integer on error
>   builtin/apply: make parse_single_patch() return -1 on error
>   builtin/apply: make parse_whitespace_option() return -1 instead of
> die()ing
>   builtin/apply: make parse_ignorewhitespace_option() return -1 instead
> of die()ing
>   builtin/apply: move init_apply_state() to apply.c
>   apply: make init_apply_state() return -1 instead of exit()ing
>   builtin/apply: make check_apply_state() return -1 instead of die()ing
>   builtin/apply: move check_apply_state() to apply.c
>   builtin/apply: make apply_all_patches() return 128 or 1 on error
>   builtin/apply: make parse_traditional_patch() return -1 on error
>   builtin/apply: make gitdiff_*() return 1 at end of header
>   builtin/apply: make gitdiff_*() return -1 on error
>   builtin/apply: change die_on_unsafe_path() to check_unsafe_path()
>   builtin/apply: make build_fake_ancestor() return -1 on error
>   builtin/apply: make remove_file() return -1 on error
>   builtin/apply: make add_conflicted_stages_file() return -1 on error
>   builtin/apply: make add_index_file() return -1 on error
>   builtin/apply: make create_file() return -1 on error
>   builtin/apply: make write_out_one_result() return -1 on error
>   builtin/apply: make write_out_results() return -1 on error
>   builtin/apply: make try_create_file() return -1 on error
>   builtin/apply: make create_one_file() return -1 on error
>   builtin/apply: rename option parsing functions
>   apply: rename and move opt constants to apply.h
>   Move libified code from builtin/apply.c to apply.{c,h}
>   apply: make some parsing functions static again
>   environment: add set_index_file()
>   write_or_die: use warning() instead of fprintf(stderr, ...)
>   apply: add 'be_silent' variable to 'struct apply_state'
>   apply: make 'be_silent' incompatible with 'apply_verbosely'
>   apply: don't print on stdout when be_silent is set
>   usage: add set_warn_routine()
>   usage: add get_error_routine() and get_warn_routine()
>   apply: change error_routine when be_silent is set
>   builtin/am: use apply api in run_apply()
>   apply: use error_errno() where possible

I finally found time to get back to finish reviewing this.

The early part up to and including "apply: make some parsing
functions static again" looked good and we could treat them as a
continuation of the earlier cc/apply-introduce-state topic, which
has been merged to the 'master' already.

I got an impression that the patches in the remainder of the series
were not as polished as the earlier ones, except for the last one,
which should be reordered and be part of the early preparation step
if possible.

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 v8 00/41] libify apply and use lib in am, part 2

[Christian Couder]

On Mon, Jun 27, 2016 at 8:23 PM, Christian Couder
 wrote:
>
> I will send a diff between this version and the previous one, as a
> reply to this email.

Here is the diff:

diff --git a/apply.c b/apply.c
index 98a..2ac22d3 100644
--- a/apply.c
+++ b/apply.c
@@ -1516,8 +1516,8 @@ static int parse_fragment_header(const char
*line, int len, struct fragment *fra
  * Find file diff header
  *
  * Returns:
- *  -1 in case of error
- *  -2 if no header was found
+ *  -1 if no header was found
+ *  -128 in case of error
  *   the size of the header in bytes (called "offset") otherwise
  */
 static int find_header(struct apply_state *state,
@@ -1553,8 +1553,9 @@ static int find_header(struct apply_state *state,
struct fragment dummy;
if (parse_fragment_header(line, len, &dummy) < 0)
continue;
-   return error(_("patch fragment without header
at line %d: %.*s"),
+   error(_("patch fragment without header at line
%d: %.*s"),
 state->linenr, (int)len-1, line);
+   return -128;
}

if (size < len + 6)
@@ -1567,23 +1568,27 @@ static int find_header(struct apply_state *state,
if (!memcmp("diff --git ", line, 11)) {
int git_hdr_len = parse_git_header(state,
line, len, size, patch);
if (git_hdr_len < 0)
-   return -1;
+   return -128;
if (git_hdr_len <= len)
continue;
if (!patch->old_name && !patch->new_name) {
-   if (!patch->def_name)
-   return error(Q_("git diff
header lacks filename information when removing "
+   if (!patch->def_name) {
+   error(Q_("git diff header
lacks filename information when removing "
"%d leading
pathname component (line %d)",
"git diff
header lacks filename information when removing "
"%d leading
pathname components (line %d)",
state->p_value),
 state->p_value,
state->linenr);
+   return -128;
+   }
patch->old_name = xstrdup(patch->def_name);
patch->new_name = xstrdup(patch->def_name);
}
-   if (!patch->is_delete && !patch->new_name)
-   return error("git diff header lacks
filename information "
+   if (!patch->is_delete && !patch->new_name) {
+   error("git diff header lacks filename
information "
 "(line %d)", state->linenr);
+   return -128;
+   }
patch->is_toplevel_relative = 1;
*hdrsize = git_hdr_len;
return offset;
@@ -1604,12 +1609,12 @@ static int find_header(struct apply_state *state,

/* Ok, we'll consider it a patch */
if (parse_traditional_patch(state, line, line+len, patch))
-   return -1;
+   return -128;
*hdrsize = len + nextlen;
state->linenr += 2;
return offset;
}
-   return -2;
+   return -1;
 }

 static void record_ws_error(struct apply_state *state,
@@ -2100,8 +2105,8 @@ static int use_patch(struct apply_state *state,
struct patch *p)
  * Create fragments (i.e. patch hunks) and hang them to the given patch.
  *
  * Returns:
- *   -1 on error,
- *   -2 if no header was found,
+ *   -1 if no header was found or parse_binary() failed,
+ *   -128 on another error,
  *   the number of bytes consumed otherwise,
  * so that the caller can call us again for the next patch.
  */
@@ -2128,7 +2133,7 @@ static int parse_chunk(struct apply_state
*state, char *buffer, unsigned long si
   patch);

if (patchsize < 0)
-   return -1;
+   return -128;

if (!patchsize) {
static const char git_binary[] = "GIT binary patch\n";
@@ -2172,8 +2177,10 @@ static int parse_chunk(struct apply_state
*state, char *buffer, unsigned long si
 * empty to us here.
 */
if ((state->apply || state->check) &&
-   (!patch->is_binary && !metadata_changes(patch)))
-   return error(_("patch with only garb

[PATCH v8 00/41] libify apply and use lib in am, part 2

[Christian Couder]

Goal


This is a patch series about libifying `git apply` functionality, and
using this libified functionality in `git am`, so that no 'git apply'
process is spawn anymore. This makes `git am` significantly faster, so
`git rebase`, when it uses the am backend, is also significantly
faster.

Previous discussions and patches series
~~~

This has initially been discussed in the following thread:

  http://thread.gmane.org/gmane.comp.version-control.git/287236/

Then the following patch series were sent:

RFC: http://thread.gmane.org/gmane.comp.version-control.git/288489/
v1: http://thread.gmane.org/gmane.comp.version-control.git/292324/
v2: http://thread.gmane.org/gmane.comp.version-control.git/294248/
v3: http://thread.gmane.org/gmane.comp.version-control.git/295429/
v4: http://thread.gmane.org/gmane.comp.version-control.git/296350/
v5: http://thread.gmane.org/gmane.comp.version-control.git/296490/
v6: http://thread.gmane.org/gmane.comp.version-control.git/297024/
v7: http://thread.gmane.org/gmane.comp.version-control.git/297193/

Highlevel view of the patches in the series
~~~

This new patch series is built on top of the above previous work.

More precisely, this is "part 2" of the full patch series which is
built on top of the "part 1" of the full patch series. And as the
"part 1" is now in "next", this "part 2" is built on top of "next".

  - Patches 01/41 is new in v8.

It renames some structs and constants that will be moved into apply.h
to give them a more specific name as suggested by Junio.

  - Patches 02/41 to 32/41 were in v1, v2, v6 and v7.

They finish libifying the apply functionality that was in
builtin/apply.c and move it into apply.{c,h}, but the libified
functionality is not yet used in `git am`.

There are a number of changes in these patches compared to v7, so that
apply_all_patches() will return 1 in case some patches don't apply and
128 in case a previously fatal error happened. This makes it possible
for `git apply` to return the same exit code as it used to return
before libification.

To do that a number of functions like find_header(), parse_chunk(),
apply_patch(), etc have been made to return -128 instead of -1 when it
was necessary.

  - Patches 33/41 to 39/41 were in v2, v6 and v7.

They implement a way to make the libified apply silent by adding a new
'be_silent' flag into 'struct apply_state'. It is a new feature in the
libified apply functionality.

This could be in a separate series, but unfortunately using the
libified apply in "git am" unmasks the fact that "git am", since it
was a shell script, has been silencing the apply functionality by
redirecting file descriptors to /dev/null and it looks like this is
not acceptable in C.

There are no changes in these patches compared to v7.

  - Patch 40/41 was in v1, v2, v6 and v7.

This patch makes `git am` use the libified functionality. It is the
same as in v7.

  - Patch 41/41 was in v6 and v7.

It replaces some calls to error() with calls to error_errno(). It is
the nearly the same as in v7. The only change is that one call to
error() is now replaced by error_errno() earlier in the series.

General comments


Sorry if this patch series is still long. I can split it into two or
more series if it is prefered.

I will send a diff between this version and the previous one, as a
reply to this email.

The benefits are not just related to not creating new processes. When
`git am` launched a `git apply` process, this new process had to read
the index from disk. Then after the `git apply`process had terminated,
`git am` dropped its index and read the index from disk to get the
index that had been modified by the `git apply`process. This was
inefficient and also prevented the split-index mechanism to provide
many performance benefits.

Using this series as rebase material, Duy explains it like this:

 > Without the series, the picture is not so surprising. We run git-apply
 > 80+ times, each consists of this sequence
 >
 > read index
 > write index (cache tree updates only)
 > read index again
 > optionally initialize name hash (when new entries are added, I guess)
 > read packed-refs
 > write index
 >
 > With this series, we run a single git-apply which does
 >
 > read index (and sharedindex too if in split-index mode)
 > initialize name hash
 > write index 80+ times

(See: 
http://thread.gmane.org/gmane.comp.version-control.git/292324/focus=292460)

Links
~

This patch series is available here:

https://github.com/chriscool/git/commits/libify-apply-use-in-am

The previous versions are available there:

v1: https://github.com/chriscool/git/commits/libify-apply-use-in-am25 
v2: https://github.com/chriscool/git/commits/libify-apply-use-in-am54
v6: https://github.com/chriscool/git/commits/libify-apply-use-in-am65
v7: https://github.com/chriscool/git/commits/libify-apply-use-in-am75

Performance numbers
~~~

Only tests on Li