Re: What's cooking in git.git (Mar 2017, #02; Fri, 3)

[Lars Schneider]

> On 21 Mar 2017, at 18:43, Junio C Hamano  wrote:
> 
> Lars Schneider  writes:
> 
>> Agreed. Would it be OK to store the "delayed" bit in the cache
>> entry itself? The extended ce_flags are stored on disk which is not
>> necessary I think. Would a new member in the cache_entry struct be
>> an acceptable option?
> 
> I'd imagine that those with thousands of cache entries in their
> index prefer not to bloat sizeof(struct cache_entry) for something
> like this, that is _only_ used during the write-out phase.  Would a
> new pointer in the "struct checkout" that points at whatever data
> structure you need (perhaps a "string_list"?) work?  As long as the
> pointer points at a constant thing, you may not even have to loosen
> the constness of "struct checkout *" itself?

OK. I will try that and post a new round soonish.

> 
>>> Within such a framework, your checkout_delayed_entries() would be a
>>> special case for finalizing a "struct checkout" that has been in
>>> use.  By enforcing that any "struct checkout" begins its life by a
>>> "state = CHECKOUT_INIT" initialization and finishes its life by a
>>> "finish_checkout()" call, we will reduce risks to forget
>>> making necessary call to checkout_delayed_entries(), I would think.
>> 
>> Agreed. How would you want to enforce "finish_checkout()", though?
>> By convention or by different means?
> 
> We can start with "convention", just like "if you did STRBUF_INIT,
> you must do strbuf_release() at some point" has served us well, I
> would think.

OK!

Thanks,
Lars

Re: What's cooking in git.git (Mar 2017, #02; Fri, 3)

[Junio C Hamano]

Lars Schneider  writes:

> Agreed. Would it be OK to store the "delayed" bit in the cache
> entry itself? The extended ce_flags are stored on disk which is not
> necessary I think. Would a new member in the cache_entry struct be
> an acceptable option?

I'd imagine that those with thousands of cache entries in their
index prefer not to bloat sizeof(struct cache_entry) for something
like this, that is _only_ used during the write-out phase.  Would a
new pointer in the "struct checkout" that points at whatever data
structure you need (perhaps a "string_list"?) work?  As long as the
pointer points at a constant thing, you may not even have to loosen
the constness of "struct checkout *" itself?

>> Within such a framework, your checkout_delayed_entries() would be a
>> special case for finalizing a "struct checkout" that has been in
>> use.  By enforcing that any "struct checkout" begins its life by a
>> "state = CHECKOUT_INIT" initialization and finishes its life by a
>> "finish_checkout()" call, we will reduce risks to forget
>> making necessary call to checkout_delayed_entries(), I would think.
>
> Agreed. How would you want to enforce "finish_checkout()", though?
> By convention or by different means?

We can start with "convention", just like "if you did STRBUF_INIT,
you must do strbuf_release() at some point" has served us well, I
would think.

Thanks.

Re: What's cooking in git.git (Mar 2017, #02; Fri, 3)

[Lars Schneider]

> On 06 Mar 2017, at 22:08, Junio C Hamano  wrote:
> 
> Lars Schneider  writes:
> 
>>> On 04 Mar 2017, at 00:26, Junio C Hamano  wrote:
>>> 
>>> 
>>> * ls/filter-process-delayed (2017-01-08) 1 commit
>>> . convert: add "status=delayed" to filter process protocol

Sorry for the delayed response. I am still pursuing this topic but
unfortunately I wasn't able to spend time on it recently.


> For example, your async_convert_to_working_tree() returns Success or
> Delayed [*2*] and the callers of write_entry() cannot tell which the
> paths on the filesystem needs a call to checkout_delayed_entries()
> to finish them before they can safely tell the outside world that
> these paths are safe to use.  
> 
> It seems to me that any caller that calls checkout_entry() needs to
> essentially do:
> 
>   - compute which entries in the index need to be checked out
>  to the filesystem;
> 
>   - for each of them:
>   - call checkout_entry()
> 
>   - call checkout_delayed_entries(), because among the
>  checkout_entry() calls we did in the above loop, some of
>  them may have "delayed", but we do not know which one(s).

Agreed. Would it be OK to store the "delayed" bit in the cache
entry itself? The extended ce_flags are stored on disk which is not
necessary I think. Would a new member in the cache_entry struct be
an acceptable option?


> Output from "git grep -e checkout_delayed_entries -e checkout_entry"
> seems to tell me that at least builtin/apply.c and
> builtin/checkout-index.c forget the last step.

For all these "single" checkout entry places the delayed checkout
makes no sense I think. Would you prefer something likes this:

checkout_entry(); finish_checkout();

or this:

checkout_entry(..., DISABLE_DELAYED_CHECKOUT);

in all these places?


> I'd understand the design better if the delayed-item list were a
> part of the "struct checkout" state structure, and write_entry(),

This works [1], however I had to remove "const" from 
"const struct checkout *state" in a few places. OK?


> when delaying the write, returned enough information (not just "this
> has been delayed") that can be used to later instruct the
> long-running filter process things like "you gave me this 'delayed'
> token earlier; I want the result for it now!", "are you finished
> processing my earlier request, to which you gave me this 'delayed'
> token?", etc.  One of these instructions could be "here is the
> path. write the result out for the earlier request of mine you gave
> me this 'delayed' token for.  I do not need the result in-core.  And
> take as much time as you need--I do not mind blocking here at this
> point."  In a future, a new instruction may be added to ask "please
> give me the result in-core, as if you returned the result to my
> earlier original async_convert_to_working_tree() call without
> delaying the request."
> 
> Within such a framework, your checkout_delayed_entries() would be a
> special case for finalizing a "struct checkout" that has been in
> use.  By enforcing that any "struct checkout" begins its life by a
> "state = CHECKOUT_INIT" initialization and finishes its life by a
> "finish_checkout()" call, we will reduce risks to forget
> making necessary call to checkout_delayed_entries(), I would think.

Agreed. How would you want to enforce "finish_checkout()", though?
By convention or by different means?


> *2* By the way, the code in write_entry() should have a final else
> clause that diagnoses an error return from
> async_convert_to_working_tree() and act on it---an unexpected return
> will fall through to the code that opens output fd and

It is ok for async_convert_to_working_tree() to fail if the filter is not
required. That's how it is currently implemented upstream.


I will post a new round to the mailing list soon. Here is a preview if
you want to look at the const changes etc:
https://github.com/larsxschneider/git/commit/a0132e564faaf5bca76af0ccc4ec6fe900be739a?w=1


Thanks,
Lars

Re: What's cooking in git.git (Mar 2017, #02; Fri, 3)

[Junio C Hamano]

Lars Schneider  writes:

>> On 04 Mar 2017, at 00:26, Junio C Hamano  wrote:
>> 
>> 
>> * ls/filter-process-delayed (2017-01-08) 1 commit
>> . convert: add "status=delayed" to filter process protocol
>> 
>> Ejected, as does not build when merged to 'pu'.
>
> I send v2 [1] where I tried to address the points in your feedback [2].

Ah, I took a look at it back then and then forgot about it.  I'll
try to see if I can replace the stale one I have and merge it to
'pu'.

> v2 not the final roll. My goal for v2 is to get the interface
> to convert.h right.

You sound like you are trying to make the interface to the callers
finalized before doing further work, but after re-reading the
exchange between you and Peff in that thread [*1*], I am not sure
that is feasible to begin with.  

For example, your async_convert_to_working_tree() returns Success or
Delayed [*2*] and the callers of write_entry() cannot tell which the
paths on the filesystem needs a call to checkout_delayed_entries()
to finish them before they can safely tell the outside world that
these paths are safe to use.  

It seems to me that any caller that calls checkout_entry() needs to
essentially do:

- compute which entries in the index need to be checked out
  to the filesystem;

- for each of them:
- call checkout_entry()

- call checkout_delayed_entries(), because among the
  checkout_entry() calls we did in the above loop, some of
  them may have "delayed", but we do not know which one(s).

Output from "git grep -e checkout_delayed_entries -e checkout_entry"
seems to tell me that at least builtin/apply.c and
builtin/checkout-index.c forget the last step.

I'd understand the design better if the delayed-item list were a
part of the "struct checkout" state structure, and write_entry(),
when delaying the write, returned enough information (not just "this
has been delayed") that can be used to later instruct the
long-running filter process things like "you gave me this 'delayed'
token earlier; I want the result for it now!", "are you finished
processing my earlier request, to which you gave me this 'delayed'
token?", etc.  One of these instructions could be "here is the
path. write the result out for the earlier request of mine you gave
me this 'delayed' token for.  I do not need the result in-core.  And
take as much time as you need--I do not mind blocking here at this
point."  In a future, a new instruction may be added to ask "please
give me the result in-core, as if you returned the result to my
earlier original async_convert_to_working_tree() call without
delaying the request."

Within such a framework, your checkout_delayed_entries() would be a
special case for finalizing a "struct checkout" that has been in
use.  By enforcing that any "struct checkout" begins its life by a
"state = CHECKOUT_INIT" initialization and finishes its life by a
"finish_checkout()" call, we will reduce risks to forget
making necessary call to checkout_delayed_entries(), I would think.


[References and Footnotes]

*1* http://public-inbox.org/git/20170226184816.30010-1-larsxschnei...@gmail.com/

*2* By the way, the code in write_entry() should have a final else
clause that diagnoses an error return from
async_convert_to_working_tree() and act on it---an unexpected return
will fall through to the code that opens output fd and

Re: What's cooking in git.git (Mar 2017, #02; Fri, 3)

[Pranit Bauva]

Hey Stephan

On Sat, Mar 4, 2017 at 8:05 PM, Stephan Beyer  wrote:
> Hi Pranit,
>
> On 03/04/2017 12:26 AM, Junio C Hamano wrote:
>> --
>> [Stalled]
> [...]
>>
>> * pb/bisect (2017-02-18) 28 commits
>>  - fixup! bisect--helper: `bisect_next_check` & bisect_voc shell function in 
>> C
>>  - bisect--helper: remove the dequote in bisect_start()
>>  - bisect--helper: retire `--bisect-auto-next` subcommand
>>  - bisect--helper: retire `--bisect-autostart` subcommand
>>  - bisect--helper: retire `--bisect-write` subcommand
>>  - bisect--helper: `bisect_replay` shell function in C
>>  - bisect--helper: `bisect_log` shell function in C
>>  - bisect--helper: retire `--write-terms` subcommand
>>  - bisect--helper: retire `--check-expected-revs` subcommand
>>  - bisect--helper: `bisect_state` & `bisect_head` shell function in C
>>  - bisect--helper: `bisect_autostart` shell function in C
>>  - bisect--helper: retire `--next-all` subcommand
>>  - bisect--helper: retire `--bisect-clean-state` subcommand
>>  - bisect--helper: `bisect_next` and `bisect_auto_next` shell function in C
>>  - t6030: no cleanup with bad merge base
>>  - bisect--helper: `bisect_start` shell function partially in C
>>  - bisect--helper: `get_terms` & `bisect_terms` shell function in C
>>  - bisect--helper: `bisect_next_check` & bisect_voc shell function in C
>>  - bisect--helper: `check_and_set_terms` shell function in C
>>  - bisect--helper: `bisect_write` shell function in C
>>  - bisect--helper: `is_expected_rev` & `check_expected_revs` shell function 
>> in C
>>  - bisect--helper: `bisect_reset` shell function in C
>>  - wrapper: move is_empty_file() and rename it as is_empty_or_missing_file()
>>  - t6030: explicitly test for bisection cleanup
>>  - bisect--helper: `bisect_clean_state` shell function in C
>>  - bisect--helper: `write_terms` shell function in C
>>  - bisect: rewrite `check_term_format` shell function in C
>>  - bisect--helper: use OPT_CMDMODE instead of OPT_BOOL
>>
>>  Move more parts of "git bisect" to C.
>>
>>  Expecting a reroll.
>
> I guess you are short on time but I am hoping that you are still going
> to send a reroll to the list (probably on top of [1]?). This is because
> I would like to start to work on rerolling my bisect patches from last
> year [2] ... but to avoid a mess of merge conflicts, I am waiting until
> pb/bisect found its way into "next". (There were also recent discussions
> on other bisect strategies [3] and it's probably only a matter of time
> until a new big patchset on bisect--helper comes up...)

I am sorry I haven't found much time on it. I actually came across a
bug and haven't been able to fix that so I had just not worked on it
then. I almost forgot that you too had a patch series and this series
is important for you. I will start working on this and send a re-roll
soon.

Regards,
Pranit Bauva

Re: What's cooking in git.git (Mar 2017, #02; Fri, 3)

[Lars Schneider]

> On 04 Mar 2017, at 00:26, Junio C Hamano  wrote:
> 
> 
> * ls/filter-process-delayed (2017-01-08) 1 commit
> . convert: add "status=delayed" to filter process protocol
> 
> Ejected, as does not build when merged to 'pu'.

I send v2 [1] where I tried to address the points in your feedback [2].

v2 not the final roll. My goal for v2 is to get the interface
to convert.h right. Could you take a quick look at the changes 
in the following files and tell me if this is the right direction to go?

entry.c
builtin/checkout.c
unpack-trees.c

Thanks,
Lars


[1] http://public-inbox.org/git/20170226184816.30010-1-larsxschnei...@gmail.com/
[2] http://public-inbox.org/git/xmqqa8b115ll@gitster.mtv.corp.google.com/

Re: What's cooking in git.git (Mar 2017, #02; Fri, 3)

[Stephan Beyer]

Hi Pranit,

On 03/04/2017 12:26 AM, Junio C Hamano wrote:
> --
> [Stalled]
[...]
> 
> * pb/bisect (2017-02-18) 28 commits
>  - fixup! bisect--helper: `bisect_next_check` & bisect_voc shell function in C
>  - bisect--helper: remove the dequote in bisect_start()
>  - bisect--helper: retire `--bisect-auto-next` subcommand
>  - bisect--helper: retire `--bisect-autostart` subcommand
>  - bisect--helper: retire `--bisect-write` subcommand
>  - bisect--helper: `bisect_replay` shell function in C
>  - bisect--helper: `bisect_log` shell function in C
>  - bisect--helper: retire `--write-terms` subcommand
>  - bisect--helper: retire `--check-expected-revs` subcommand
>  - bisect--helper: `bisect_state` & `bisect_head` shell function in C
>  - bisect--helper: `bisect_autostart` shell function in C
>  - bisect--helper: retire `--next-all` subcommand
>  - bisect--helper: retire `--bisect-clean-state` subcommand
>  - bisect--helper: `bisect_next` and `bisect_auto_next` shell function in C
>  - t6030: no cleanup with bad merge base
>  - bisect--helper: `bisect_start` shell function partially in C
>  - bisect--helper: `get_terms` & `bisect_terms` shell function in C
>  - bisect--helper: `bisect_next_check` & bisect_voc shell function in C
>  - bisect--helper: `check_and_set_terms` shell function in C
>  - bisect--helper: `bisect_write` shell function in C
>  - bisect--helper: `is_expected_rev` & `check_expected_revs` shell function 
> in C
>  - bisect--helper: `bisect_reset` shell function in C
>  - wrapper: move is_empty_file() and rename it as is_empty_or_missing_file()
>  - t6030: explicitly test for bisection cleanup
>  - bisect--helper: `bisect_clean_state` shell function in C
>  - bisect--helper: `write_terms` shell function in C
>  - bisect: rewrite `check_term_format` shell function in C
>  - bisect--helper: use OPT_CMDMODE instead of OPT_BOOL
> 
>  Move more parts of "git bisect" to C.
> 
>  Expecting a reroll.

I guess you are short on time but I am hoping that you are still going
to send a reroll to the list (probably on top of [1]?). This is because
I would like to start to work on rerolling my bisect patches from last
year [2] ... but to avoid a mess of merge conflicts, I am waiting until
pb/bisect found its way into "next". (There were also recent discussions
on other bisect strategies [3] and it's probably only a matter of time
until a new big patchset on bisect--helper comes up...)

Cheers
  Stephan

[1]:
https://public-inbox.org/git/xmqqvarq9vzo@gitster.mtv.corp.google.com/
[2]:
https://public-inbox.org/git/1460294354-7031-1-git-send-email-s-be...@gmx.net/
[3]:
https://public-inbox.org/git/CABEd3j8m5D=bBbUD+uzvE9c8AwdBEM79Np7Pnin-RLL=hjq...@mail.gmail.com/

What's cooking in git.git (Mar 2017, #02; Fri, 3)

[Junio C Hamano]

Here are the topics that have been cooking.  Commits prefixed with
'-' are only in 'pu' (proposed updates) while commits prefixed with
'+' are in 'next'.  The ones marked with '.' do not appear in any of
the integration branches, but I am still holding onto them.

You can find the changes described here in the integration branches
of the repositories listed at

http://git-blame.blogspot.com/p/git-public-repositories.html

--
[New Topics]

* ew/markdown-url-in-readme (2017-03-01) 1 commit
  (merged to 'next' on 2017-03-03 at 3d35e3a991)
 + README: create HTTP/HTTPS links from URLs in Markdown

 Doc update.

 Will merge to 'master'.


* jk/add-i-patch-do-prompt (2017-03-02) 1 commit
 - add--interactive: fix missing file prompt for patch mode with "-i"

 The patch subcommand of "git add -i" was meant to have paths
 selection prompt just like other subcommand, unlike "git add -p"
 directly jumps to hunk selection.  Recently, this was broken and
 "add -i" lost the paths selection dialog, but it now has been
 fixed.

 Will merge to 'next'.


* ax/line-log-range-merge-fix (2017-03-03) 1 commit
 - line-log.c: prevent crash during union of too many ranges

 The code to parse "git log -L..." command line was buggy when there
 are many ranges specified with -L; overrun of the allocated buffer
 has been fixed.

 Will merge to 'next'.


* js/early-config (2017-03-03) 11 commits
 - t1309: test read_early_config()
 - read_early_config(): really discover .git/
 - read_early_config(): avoid .git/config hack when unneeded
 - setup: make read_early_config() reusable
 - setup: export the discover_git_directory() function
 - SQUASH??? ERROR: trailing statements should be on next line
 - setup_git_directory_1(): avoid changing global state
 - setup: prepare setup_discovered_git_directory() the root directory
 - SQUASH??? ERROR: trailing statements should be on next line
 - setup_git_directory(): use is_dir_sep() helper
 - t7006: replace dubious test

 The start-up sequence of "git" needs to figure out some configured
 settings before it finds and set itself up in the location of the
 repository and was quite messy due to its "chicken-and-egg" nature.
 The code has been restructured.

 Will merge to 'next' after squashing niggle-fixes in.


* jt/perf-updates (2017-03-03) 3 commits
 - t/perf: add fallback for pre-bin-wrappers versions of git
 - t/perf: use $MODERN_GIT for all repo-copying steps
 - t/perf: export variable used in other blocks

 The t/perf performance test suite was not prepared to test not so
 old versions of Git, but now it covers versions of Git that are not
 so ancient.

 Will merge to 'next'.


* ss/remote-bzr-hg-placeholder-wo-python (2017-03-03) 1 commit
 - contrib: git-remote-{bzr,hg} placeholders don't need Python

 There is no need for Python only to give a few messages to the
 standard error stream, but we somehow did.

 Will merge to 'next'.

--
[Stalled]

* nd/worktree-move (2017-01-27) 7 commits
 . fixup! worktree move: new command
 . worktree remove: new command
 . worktree move: refuse to move worktrees with submodules
 . worktree move: accept destination as directory
 . worktree move: new command
 . worktree.c: add update_worktree_location()
 . worktree.c: add validate_worktree()

 "git worktree" learned move and remove subcommands.

 Tentatively ejected as it seems to break 'pu' when merged.


* pb/bisect (2017-02-18) 28 commits
 - fixup! bisect--helper: `bisect_next_check` & bisect_voc shell function in C
 - bisect--helper: remove the dequote in bisect_start()
 - bisect--helper: retire `--bisect-auto-next` subcommand
 - bisect--helper: retire `--bisect-autostart` subcommand
 - bisect--helper: retire `--bisect-write` subcommand
 - bisect--helper: `bisect_replay` shell function in C
 - bisect--helper: `bisect_log` shell function in C
 - bisect--helper: retire `--write-terms` subcommand
 - bisect--helper: retire `--check-expected-revs` subcommand
 - bisect--helper: `bisect_state` & `bisect_head` shell function in C
 - bisect--helper: `bisect_autostart` shell function in C
 - bisect--helper: retire `--next-all` subcommand
 - bisect--helper: retire `--bisect-clean-state` subcommand
 - bisect--helper: `bisect_next` and `bisect_auto_next` shell function in C
 - t6030: no cleanup with bad merge base
 - bisect--helper: `bisect_start` shell function partially in C
 - bisect--helper: `get_terms` & `bisect_terms` shell function in C
 - bisect--helper: `bisect_next_check` & bisect_voc shell function in C
 - bisect--helper: `check_and_set_terms` shell function in C
 - bisect--helper: `bisect_write` shell function in C
 - bisect--helper: `is_expected_rev` & `check_expected_revs` shell function in C
 - bisect--helper: `bisect_reset` shell function in C
 - wrapper: move is_empty_file() and rename it as is_empty_or_missing_file()
 - t6030: explicitly test for bisection cleanup
 - bisect--helper: