[PATCH v2] doc: camelCase the config variables to improve readability

A few configuration variable names of Git are composite words. References to such variables in manpages are hard to read because they use all-lowercase names, without indicating where each word ends and begins. Improve its readability by using camelCase instead. Git treats these names

Re: [PATCH v7 04/12] fsmonitor: teach git to optionally utilize a file system monitor to speed up detecting new or changed files.

Ben Peart writes: > @@ -344,6 +346,7 @@ struct index_state { > struct hashmap dir_hash; > unsigned char sha1[20]; > struct untracked_cache *untracked; > + uint64_t fsmonitor_last_update; This field being zero has more significance than just "we

Re: [PATCH v2] for_each_string_list_item: avoid undefined behavior for empty list

On 09/20/2017 07:27 AM, Jonathan Nieder wrote: > From: Michael Haggerty > > If you pass a newly initialized or newly cleared `string_list` to > `for_each_string_list_item()`, then the latter does > > for ( > item = (list)->items; /* NULL */ >

Re: [PATCH] for_each_string_list_item(): behave correctly for empty list

Hi, Though this thread seems to have reached a conclusion, I just wanted to know what I was missing about the optimisation. On Wednesday 20 September 2017 08:00 AM, Jonathan Nieder wrote: From that link: for ( ;valid_int && *valid_int < 10; (*valid_int)++) { printf("Valid

Re: [PATCH v2] for_each_string_list_item: avoid undefined behavior for empty list

On Wednesday 20 September 2017 10:57 AM, Jonathan Nieder wrote: Guard the loop with a NULL check to make the intent crystal clear to even the most pedantic compiler. A suitably clever compiler could let the NULL check only run in the first iteration, Noted this just now. So, the overhead

Re: [PATCH] doc: camelCase the config variables to improve readability

On Wednesday 20 September 2017 01:52 AM, Jonathan Nieder wrote: nit: Git's commit messages describe the current behavior of Git in the present tense. Something like: Thanks for the clarification. I was once searching Documentation/SubmittingPatches to see if there is any guideline about the

Re: [RFC PATCH 1/5] builtin/checkout: avoid usage of '!!'

On Wednesday 20 September 2017 09:30 AM, Junio C Hamano wrote: Kaartic Sivaraam writes: There was a usage for which there's no compelling reason.So, replace such a usage as with something else that expresses the intent more clearly. I actually think this is a

Re: [PATCH v7 05/12] fsmonitor: add documentation for the fsmonitor extension.

On 19 September 2017 at 21:27, Ben Peart wrote: > diff --git a/Documentation/git-update-index.txt > b/Documentation/git-update-index.txt > index e19eba62cd..95231dbfcb 100644 > --- a/Documentation/git-update-index.txt > +++ b/Documentation/git-update-index.txt > @@ -16,9

Re: [RFC PATCH 2/5] branch: document the usage of certain parameters

On Wednesday 20 September 2017 09:42 AM, Junio C Hamano wrote: @@ -15,6 +15,11 @@ * * - reflog creates a reflog for the branch * + * - if 'force' is true, clobber_head indicates whether the branch could be + * the current branch; else it has no effect Everybody else in this

Re: [PATCH v2 14/21] read_packed_refs(): ensure that references are ordered when read

On Tue, Sep 19, 2017 at 08:22:22AM +0200, Michael Haggerty wrote: > If `packed-refs` was already sorted, then (if the system allows it) we > can use the mmapped file contents directly. But if the system doesn't > allow a file that is currently mmapped to be replaced using > `rename()`, then it

Re: [PATCH v7 04/12] fsmonitor: teach git to optionally utilize a file system monitor to speed up detecting new or changed files.

On 9/20/2017 2:23 AM, Junio C Hamano wrote: Ben Peart writes: @@ -344,6 +346,7 @@ struct index_state { struct hashmap dir_hash; unsigned char sha1[20]; struct untracked_cache *untracked; + uint64_t fsmonitor_last_update; This field

Re: [PATCH v2] for_each_string_list_item: avoid undefined behavior for empty list

On Sep 19 2017, Jonathan Nieder wrote: > B. #define for_each_string_list_item(item, list) \ > if (list->items) \ > for (item = ...; ...; ... ) > >This breaks a caller like > if (foo) > for_each_string_list_item(item, list) >

Re: [PATCH v7 05/12] fsmonitor: add documentation for the fsmonitor extension.

On 20 September 2017 at 19:02, Ben Peart wrote: >>> +--[no-]fsmonitor-valid:: >>> + When one of these flags is specified, the object name recorded >>> + for the paths are not updated. Instead, these options >>> + set and unset the "fsmonitor valid" bit for

Re: [PATCH v6 09/12] split-index: disable the fsmonitor extension when running the split index test

On 9/19/2017 4:43 PM, Jonathan Nieder wrote: Hi, Ben Peart wrote: The split index test t1700-split-index.sh has hard coded SHA values for the index. Currently it supports index V4 and V3 but assumes there are no index extensions loaded. When manually forcing the fsmonitor extension to be

Re: [PATCH v2] for_each_string_list_item: avoid undefined behavior for empty list

Andreas Schwab wrote: > On Sep 19 2017, Jonathan Nieder wrote: >> B. #define for_each_string_list_item(item, list) \ >> if (list->items) \ >> for (item = ...; ...; ... ) >> >>This breaks a caller like >> if (foo) >>

Re: [PATCH v6 09/12] split-index: disable the fsmonitor extension when running the split index test

Hi, Ben Peart wrote: > On 9/19/2017 4:43 PM, Jonathan Nieder wrote: >> This feels to me like the wrong fix. Wouldn't it be better for the >> test not to depend on the precise object ids? See the "Tips for >> Writing Tests" section in t/README: > > I completely agree that a better fix would be

Re: [PATCH v2 08/21] read_packed_refs(): read references with minimal copying

On Tue, Sep 19, 2017 at 08:22:16AM +0200, Michael Haggerty wrote: > Instead of copying data from the `packed-refs` file one line at time > and then processing it, process the data in place as much as possible. > > Also, instead of processing one line per iteration of the main loop, > process a

[PATCH 1.5/8] connect: die when a capability line comes after a ref

Commit eb398797c (connect: advertized capability is not a ref, 2016-09-09) taught 'get_remote_heads()' to recognize that the 'capabilities^{}' line isn't a ref but required that the 'capabilities^{}' line came during the first response from the server. A future patch will introduce a version

Re: [PATCH] Add t/helper/test-write-cache to .gitignore

Brandon Williams wrote: > On 08/28, Jonathan Tan wrote: >> This new binary was introduced in commit 3921a0b ("perf: add test for >> writing the index", 2017-08-21), but a .gitignore entry was not added >> for it. Add that entry. >> >> Signed-off-by: Jonathan Tan > >

Re: [PATCH v7 05/12] fsmonitor: add documentation for the fsmonitor extension.

Thanks for the review. I'm not an English major so appreciate the feedback on my attempts to document the feature. On 9/20/2017 6:00 AM, Martin Ågren wrote: On 19 September 2017 at 21:27, Ben Peart wrote: diff --git a/Documentation/git-update-index.txt

Re: [PATCH v2 13/21] packed_ref_cache: keep the `packed-refs` file mmapped if possible

On Wed, Sep 20, 2017 at 02:40:58PM -0400, Jeff King wrote: > > +enum mmap_strategy { > > + /* > > +* Don't use mmap() at all for reading `packed-refs`. > > +*/ > > + MMAP_NONE, > > + > > + /* > > +* Can use mmap() for reading `packed-refs`, but the file must > > +* not

Re: [PATCH v2 11/21] mmapped_ref_iterator_advance(): no peeled value for broken refs

On Tue, Sep 19, 2017 at 08:22:19AM +0200, Michael Haggerty wrote: > If a reference is broken, suppress its peeled value. Makes sense. Without this we might return an answer for "git show tag^{commit}" even if the repo is broken and the object pointed to by "tag" is missing. -Peff

Re: [PATCH v2 13/21] packed_ref_cache: keep the `packed-refs` file mmapped if possible

On Tue, Sep 19, 2017 at 08:22:21AM +0200, Michael Haggerty wrote: > Keep a copy of the `packed-refs` file contents in memory for as long > as a `packed_ref_cache` object is in use: > > * If the system allows it, keep the `packed-refs` file mmapped. > > * If not (either because the system

Re: [PATCH v2 00/21] Read `packed-refs` using mmap()

On Tue, Sep 19, 2017 at 08:22:08AM +0200, Michael Haggerty wrote: > This branch is also available from my fork on GitHub as branch > `mmap-packed-refs`. > > My main uncertainties are: > > 1. Does this code actually work on Windows? > > 2. Did I implement the new compile-time option correctly?

Re: [PATCH v7 03/12] update-index: add a new --force-write-index option

On 9/20/2017 1:47 AM, Junio C Hamano wrote: Ben Peart writes: + OPT_SET_INT(0, "force-write-index", _write, + N_("write out the index even if is not flagged as changed"), 1), Hmph. The only time this makes difference is when

Re: git fast-export/import bug with -M -C

Hi, did anyone manage to take a look at this bug? Friendly ping. Juraj On Fri, Sep 15, 2017 at 12:01 AM, Juraj Oršulić wrote: > The commands should be self explanatory. 0.2.0~20 is the first commit > where the reconstructed repository diverges, that commit had a >

Re: [RFC PATCH 3/5] branch: cleanup branch name validation

On Wednesday 20 September 2017 09:50 AM, Junio C Hamano wrote: Also I'd avoid "could", which can be taken as an optimization hint (i.e. "you usually do not have to worry about this thing to already exist, but I am telling you that for this one call that is not the case and you need to be a bit

[PATCH] diff-lib: clear `pending` object-array in `index_differs_from()`

Instead of conditionally freeing `rev.pending.objects`, just call `object_array_clear()` on `rev.pending`. This means we don't poke as much into the implementation, which is already a good thing, but also that we free the individual entries as well, thereby fixing a memory-leak. Signed-off-by:

Re: [PATCH] revision: fix memory leaks with `struct cmdline_pathspec`

On Wed, Sep 20, 2017 at 09:47:26PM +0200, Martin Ågren wrote: > We don't free the array `prune_data.path` or the individual strings it > points to. Do so by introducing and using `free_cmdline_pathspec()`. To > be able to safely free the strings, always use `xstrdup()` when > assigning them. That

Re: [PATCH v2 02/21] prefix_ref_iterator: break when we leave the prefix

On Mon, Sep 18, 2017 at 11:22 PM, Michael Haggerty wrote: > From: Jeff King > > If the underlying iterator is ordered, then `prefix_ref_iterator` can > stop as soon as it sees a refname that comes after the prefix. This > will rarely make a big difference

[PATCH] revision: replace "struct cmdline_pathspec" with argv_array

On Wed, Sep 20, 2017 at 04:25:52PM -0400, Jeff King wrote: > Isn't this whole thing just an argv_array, and this is argv_array_pushv? > We even NULL-terminate it manually later on! > > So rather than increasing the line count by adding > free_cmdline_pathspec, I think we could actually _reduce_

Re: [PATCHv3] builtin/merge: honor commit-msg hook for merges

On Fri, Sep 15, 2017 at 11:22 PM, Kaartic Sivaraam wrote: > Seems 'Documentation/githooks.txt' needs an update related to this > change. Previously it said(note the **s) that 'commit-msg' is invoked > only by 'git commit', > > commit-msg >This hook

Re: commit-msg hook does not run on merge with --no-ff option

For anyone who stumbles upon this in the future, I found a very simple workaround to force the commit hooks (including commit-msg) to run following a merge operation: Simply create a post-merge hook which triggers an amended commit with no changes. post-merge hook: #!/bin/bash git commit

Re: [PATCH] commit: fix memory leak in `reduce_heads()`

On Wed, Sep 20, 2017 at 09:47:25PM +0200, Martin Ågren wrote: > We don't free the temporary scratch space we use with > `remove_redundant()`. Free it similar to how we do it in > `get_merge_bases_many_0()`. Yep, seems very straightforward, and there are no code paths where we wouldn't want this.

Re: [PATCH] Win32: simplify loading of DLL functions

Hi Jonathan, On Tue, 19 Sep 2017, Jonathan Nieder wrote: > Johannes Schindelin wrote: > > > Dynamic loading of DLL functions is duplicated in several places in Git > > for Windows' source code. > > > > This patch adds a pair of macros to simplify the process: the > > DECLARE_PROC_ADDR(, , , > >

Re: [PATCH] commit: fix memory leak in `reduce_heads()`

Martin Ågren wrote: > We don't free the temporary scratch space we use with > `remove_redundant()`. Free it similar to how we do it in > `get_merge_bases_many_0()`. > > Signed-off-by: Martin Ågren > --- > commit.c | 1 + > 1 file changed, 1 insertion(+) Good catch.

Re: [PATCH 1.5/8] connect: die when a capability line comes after a ref

On Wed, Sep 20, 2017 at 11:48:32AM -0700, Brandon Williams wrote: > Commit eb398797c (connect: advertized capability is not a ref, > 2016-09-09) taught 'get_remote_heads()' to recognize that the > 'capabilities^{}' line isn't a ref but required that the > 'capabilities^{}' line came during the

Re: [PATCH] Win32: simplify loading of DLL functions

Johannes Schindelin wrote: > On Tue, 19 Sep 2017, Jonathan Nieder wrote: >> Could this example go near the top of the header instead? That way, >> it's easier for people reading the header to see how to use it. > > Funny, I am *so* used to examples being at the very end, from tutorials to > man

Re: [PATCH 1.5/8] connect: die when a capability line comes after a ref

On 09/20, Jeff King wrote: > On Wed, Sep 20, 2017 at 11:48:32AM -0700, Brandon Williams wrote: > > > Commit eb398797c (connect: advertized capability is not a ref, > > 2016-09-09) taught 'get_remote_heads()' to recognize that the > > 'capabilities^{}' line isn't a ref but required that the > >

Re: [RFC PATCH 5/5] builtin/branch: give more useful error messages when renaming

On Tue, Sep 19, 2017 at 12:15 AM, Kaartic Sivaraam wrote: > When trying to rename an inexistent branch to an existing branch > the rename failed specifying the new branch name exists rather than > specifying that the branch trying to be renamed doesn't exist. > >

Re: [PATCH] commit: fix memory leak in `prepare_index()`

On Wed, Sep 20, 2017 at 09:47:23PM +0200, Martin Ågren wrote: > Release `pathspec` and the string list `partial`. Makes sense. > When we clear the string list, make sure we do not free the `util` > pointers. That would result in double-freeing, since we set them up as > `item->util = item` in

Re: Behaviour of 'git stash show' when a stash has untracked files

On Wed, Sep 20, 2017 at 02:33:04PM +0900, Junio C Hamano wrote: > Kaartic Sivaraam writes: > > > Some time ago, I stashed a few changes along with untracked files. I > > almost forgot it until recently. Then I wanted to see what I change I > > had in the stash.

[PATCH] revision: fix memory leaks with `struct cmdline_pathspec`

We don't free the array `prune_data.path` or the individual strings it points to. Do so by introducing and using `free_cmdline_pathspec()`. To be able to safely free the strings, always use `xstrdup()` when assigning them. That does mean we allocate more memory than we used to, but it also means

[PATCH] commit: fix memory leak in `prepare_index()`

Release `pathspec` and the string list `partial`. When we clear the string list, make sure we do not free the `util` pointers. That would result in double-freeing, since we set them up as `item->util = item` in `list_paths()`. Initialize the string list early, so that we can always release it.

[PATCH] commit: fix memory leak in `reduce_heads()`

We don't free the temporary scratch space we use with `remove_redundant()`. Free it similar to how we do it in `get_merge_bases_many_0()`. Signed-off-by: Martin Ågren --- commit.c | 1 + 1 file changed, 1 insertion(+) diff --git a/commit.c b/commit.c index

Re: [PATCH] diff-lib: clear `pending` object-array in `index_differs_from()`

On Wed, Sep 20, 2017 at 09:47:24PM +0200, Martin Ågren wrote: > Instead of conditionally freeing `rev.pending.objects`, just call > `object_array_clear()` on `rev.pending`. This means we don't poke as > much into the implementation, which is already a good thing, but also > that we free the

Re: [PATCH 1.5/8] connect: die when a capability line comes after a ref

Brandon Williams wrote: > On 09/20, Jeff King wrote: >> (For that matter, could we just be checking whether *list is NULL?) > > True, that would probably be the better way to do this. Nice idea, thank you. That doesn't capture a few other cases of pkts that aren't supposed to come before the

Re: [PATCH v2] for_each_string_list_item: avoid undefined behavior for empty list

On Sep 20 2017, Jonathan Nieder wrote: > Andreas Schwab wrote: >> On Sep 19 2017, Jonathan Nieder wrote: > >>> B. #define for_each_string_list_item(item, list) \ >>> if (list->items) \ >>> for (item = ...; ...; ... ) >>> >>>This breaks

Re: [PATCH v6 09/12] split-index: disable the fsmonitor extension when running the split index test

On 9/20/2017 1:46 PM, Jonathan Nieder wrote: Hi, Ben Peart wrote: On 9/19/2017 4:43 PM, Jonathan Nieder wrote: This feels to me like the wrong fix. Wouldn't it be better for the test not to depend on the precise object ids? See the "Tips for Writing Tests" section in t/README: I

Re: [PATCH 3/8] daemon: recognize hidden request arguments

On Wed, 13 Sep 2017 14:54:43 -0700 Brandon Williams wrote: > A normal request to git-daemon is structured as > "command path/to/repo\0host=..\0" and due to a bug in an old version of > git-daemon 73bb33a94 (daemon: Strictly parse the "extra arg" part of the > command,

Re: [PATCH] revision: replace "struct cmdline_pathspec" with argv_array

Hi, Jeff King wrote: > Subject: [PATCH] revision: replace "struct cmdline_pathspec" with argv_array > > We assemble an array of strings in a custom struct, > NULL-terminate the result, and then pass it to > parse_pathspec(). > > But then we never free the array or the individual strings > (nor

Re: [PATCH 3/8] daemon: recognize hidden request arguments

On Wed, 20 Sep 2017 17:24:43 -0700 Jonathan Tan wrote: > On Wed, 13 Sep 2017 14:54:43 -0700 > Brandon Williams wrote: > > > A normal request to git-daemon is structured as > > "command path/to/repo\0host=..\0" and due to a bug in an old version of >

Re: [PATCH v7 02/12] preload-index: add override to enable testing preload-index

On 9/20/2017 6:06 PM, Stefan Beller wrote: On Tue, Sep 19, 2017 at 12:27 PM, Ben Peart wrote: Preload index doesn't run unless it has a minimum number of 1000 files. To enable running tests with fewer files, add an environment variable (GIT_FORCE_PRELOAD_TEST) which

[PATCH] fast-export: do not copy from modified file

When run with the "-C" option, fast-export writes 'C' commands in its output whenever the internal diff mechanism detects a file copy, indicating that fast-import should copy the given existing file to the given new filename. However, the diff mechanism works against the prior version of the file,

Re: [PATCH v7 02/12] preload-index: add override to enable testing preload-index

> To execute the preload code path, we need a minimum of 2 cache entries and 2 > threads so that each thread actually has work to do. Otherwise the logic > below that divides the work up would need to be updated as well. The > additional complexity didn't seem worth it just to enable the code

Re: [PATCH v7 02/12] preload-index: add override to enable testing preload-index

On Tue, Sep 19, 2017 at 12:27 PM, Ben Peart wrote: > Preload index doesn't run unless it has a minimum number of 1000 files. > To enable running tests with fewer files, add an environment variable > (GIT_FORCE_PRELOAD_TEST) which will override that minimum and set it to 2.

Re: [git-for-windows] Re: Revision resolution for remote-helpers?

On Fri, Aug 25, 2017 at 09:02:36PM +0900, Mike Hommey wrote: > On Fri, Aug 25, 2017 at 12:58:52PM +0200, Johannes Schindelin wrote: > > > > > Cc-ing the Git for Windows mailing list as an FYI. > > > > > > > > > > I have faint memories that some developers on that project have had to > > > > >

Re: [PATCH v2] for_each_string_list_item: avoid undefined behavior for empty list

Andreas Schwab writes: > On Sep 20 2017, Jonathan Nieder wrote: > >> Andreas Schwab wrote: >>> On Sep 19 2017, Jonathan Nieder wrote: >> B. #define for_each_string_list_item(item, list) \ if (list->items) \

Re: [RFC PATCH 3/5] branch: cleanup branch name validation

Kaartic Sivaraam writes: > Thanks for giving a better alternative. Sounds catchy. How about > `validate_branch_creation`? I do not know what you meant by "catchy", but "git grep ok_to_" will tell you that ok-to-$do-something is quite an establish phrasing (if I

Re: [PATCH v7 04/12] fsmonitor: teach git to optionally utilize a file system monitor to speed up detecting new or changed files.

Ben Peart writes: > Pretty much the same places you would also use CE_MATCH_IGNORE_VALID > and CE_MATCH_IGNORE_SKIP_WORKTREE which serve the same role for those > features. That is generally when you are about to overwrite data so > want to be *really* sure you have what you

Re: [PATCH v7 04/12] fsmonitor: teach git to optionally utilize a file system monitor to speed up detecting new or changed files.

On 9/20/2017 10:00 PM, Junio C Hamano wrote: Ben Peart writes: Pretty much the same places you would also use CE_MATCH_IGNORE_VALID and CE_MATCH_IGNORE_SKIP_WORKTREE which serve the same role for those features. That is generally when you are about to overwrite data so

Re: [PATCH] revision: replace "struct cmdline_pathspec" with argv_array

On Wed, Sep 20, 2017 at 03:48:26PM -0700, Jonathan Nieder wrote: > > Subject: [PATCH] revision: replace "struct cmdline_pathspec" with argv_array > > > > We assemble an array of strings in a custom struct, > > NULL-terminate the result, and then pass it to > > parse_pathspec(). > > > > But then

Re: [PATCH 1.5/8] connect: die when a capability line comes after a ref

Brandon Williams writes: >> Or is this only for v2 clients, and we've changed the protocol but >> get_remote_heads() just needs to be updated, too? > > A client which didn't request protocol v1 (I'm calling the current > protocol v0, and v1 is just v0 with the initial response

Re: [PATCH] diff-lib: clear `pending` object-array in `index_differs_from()`

On Thu, Sep 21, 2017 at 05:56:21AM +0200, Martin Ågren wrote: > > Looks good. A similar bug was the exact reason for adding the function > > in 46be82312. I did a grep for 'free.*\.objects' to see if there were > > other cases. > > Ah. I grepped for "pending.objects", but didn't go more general

Re: [PATCH v2 02/21] prefix_ref_iterator: break when we leave the prefix

On Wed, Sep 20, 2017 at 01:25:43PM -0700, Stefan Beller wrote: > > +/* Return -1, 0, 1 if refname is before, inside, or after the prefix. */ > > +static int compare_prefix(const char *refname, const char *prefix) > > +{ > > + while (*prefix) { > > + if (*refname != *prefix) >

Re: [PATCH] revision: replace "struct cmdline_pathspec" with argv_array

Jonathan Nieder writes: > diff --git a/pathspec.c b/pathspec.c > index e2a23ebc96..cdefdc7cc0 100644 > --- a/pathspec.c > +++ b/pathspec.c > @@ -526,10 +526,6 @@ static void NORETURN unsupported_magic(const char > *pattern, > pattern, sb.buf); > } > > -/* > - *

Re: [PATCHv3] builtin/merge: honor commit-msg hook for merges

Stefan Beller writes: > I'll send a patch fixing the docs, though with this thought, maybe we need > to fix other commands, that produce commits as well? > (git revert, others?) I do not think "commands that create commits" is not a good criteria. "git notes" and "git

Re: Behaviour of 'git stash show' when a stash has untracked files

Jeff King writes: > I sketched out a possible solution in: > > > https://public-inbox.org/git/20170317141417.g2oenl67k74nl...@sigill.intra.peff.net/ > > though I share your concerns over whether people would be annoyed to see > the existing "stash show" output changed. Forgot

Re: [RFC PATCH 1/5] builtin/checkout: avoid usage of '!!'

Kaartic Sivaraam writes: > Wait, I missed a contradiction here. > .. > Documentation/SubmittingPatches says: > >> - Some clever tricks, like using the !! operator with arithmetic >>constructs, can be extremely confusing to others. What does "with arithmetic

Re: [PATCH v7 03/12] update-index: add a new --force-write-index option

Ben Peart writes: > On 9/20/2017 9:46 PM, Junio C Hamano wrote: >> Ben Peart writes: >> >>> Lets see how my ascii art skills do at describing this: >> >> Your ascii art is fine. If you said upfront that the capital >> letters signify points in time,

Re: [PATCH v7 03/12] update-index: add a new --force-write-index option

Junio C Hamano writes: > Ben Peart writes: > >> On 9/20/2017 9:46 PM, Junio C Hamano wrote: >>> Ben Peart writes: >>> Lets see how my ascii art skills do at describing this: >>> >>> Your ascii art is fine. If you said upfront

Re: [PATCH] revision: replace "struct cmdline_pathspec" with argv_array

On Wed, Sep 20, 2017 at 08:49:00PM -0700, Jonathan Nieder wrote: > The patch I am replying to tightens the contract for parse_pathspec(). I disagree with this bit. In my view that was already the contract for parse_pathspec(), and it is simply poorly documented. You can see it being required

Re: [PATCH] git: add --no-optional-locks option

On Thu, Sep 21, 2017 at 01:55:58PM +0900, Junio C Hamano wrote: > The phrase 'optional lock' does not answer this question clearly, > though: does it make sense to extend the coverage of this option in > the future to things more than the "opportunistic update to the > index file"? > > If the

Re: [RFC PATCH 2/5] branch: document the usage of certain parameters

Kaartic Sivaraam writes: > Can I do this or should I keep the patch focused around the > documentation part alone? You can do both if you wanted to but not in a same patch that makes the result noisier than it needs to.

Re: [PATCH v7 03/12] update-index: add a new --force-write-index option

Ben Peart writes: > Lets see how my ascii art skills do at describing this: Your ascii art is fine. If you said upfront that the capital letters signify points in time, lower letters are file-touching events, and time flows from left to right, it would have been perfect ;-)

Re: [PATCH v7 03/12] update-index: add a new --force-write-index option

On 9/20/2017 9:46 PM, Junio C Hamano wrote: Ben Peart writes: Lets see how my ascii art skills do at describing this: Your ascii art is fine. If you said upfront that the capital letters signify points in time, lower letters are file-touching events, and time flows

Re: [PATCH] revision: replace "struct cmdline_pathspec" with argv_array

On 20 September 2017 at 22:36, Jeff King wrote: > On Wed, Sep 20, 2017 at 04:25:52PM -0400, Jeff King wrote: > >> Isn't this whole thing just an argv_array, and this is argv_array_pushv? >> We even NULL-terminate it manually later on! >> >> So rather than increasing the line count

Re: [PATCH] revision: replace "struct cmdline_pathspec" with argv_array

Jeff King writes: > Subject: [PATCH] revision: replace "struct cmdline_pathspec" with argv_array > > We assemble an array of strings in a custom struct, > NULL-terminate the result, and then pass it to > parse_pathspec(). > > But then we never free the array or the individual

Re: [PATCH] revision: replace "struct cmdline_pathspec" with argv_array

Jeff King wrote: > On Wed, Sep 20, 2017 at 03:48:26PM -0700, Jonathan Nieder wrote: >> Jeff King wrote: >>> Subject: [PATCH] revision: replace "struct cmdline_pathspec" with argv_array >>> >>> We assemble an array of strings in a custom struct, >>> NULL-terminate the result, and then pass it to

Re: Behaviour of 'git stash show' when a stash has untracked files

On Thu, Sep 21, 2017 at 10:23:36AM +0900, Junio C Hamano wrote: > Jeff King writes: > > > I sketched out a possible solution in: > > > > > > https://public-inbox.org/git/20170317141417.g2oenl67k74nl...@sigill.intra.peff.net/ > > > > though I share your concerns over whether

Re: [PATCH] revision: replace "struct cmdline_pathspec" with argv_array

Hi, Jeff King wrote: > But mostly I am fundamentally against using UNLEAK() in a case like > this, because it does not match either of the properties which justified > adding UNLEAK() in the first place: > > 1. We are about to exit the program, so the "leak" is only caused by > the memory

Re: [PATCH] diff-lib: clear `pending` object-array in `index_differs_from()`

On 20 September 2017 at 22:02, Jeff King wrote: > On Wed, Sep 20, 2017 at 09:47:24PM +0200, Martin Ågren wrote: > >> Instead of conditionally freeing `rev.pending.objects`, just call >> `object_array_clear()` on `rev.pending`. This means we don't poke as >> much into the

[PATCH] git: add --no-optional-locks option

Johannes, this is an adaptation of your 67e5ce7f63 (status: offer *not* to lock the index and update it, 2016-08-12). Folks working on GitHub Desktop complained to me that it's only available on Windows. :) I expanded the scope a bit to let us give the same treatment to more commands in the long

Re: [PATCH] revision: replace "struct cmdline_pathspec" with argv_array

On Wed, Sep 20, 2017 at 09:41:12PM -0700, Jonathan Nieder wrote: > > Well, no...the idea is that this is a function which leaks a bunch of > > memory, and we shouldn't have to think hard about how often its leak can > > be triggered or how severe it is. We should just fix it. > > I forgot to

Re: [PATCH] git: add --no-optional-locks option

Jeff King writes: > Johannes, this is an adaptation of your 67e5ce7f63 (status: offer *not* > to lock the index and update it, 2016-08-12). Folks working on GitHub > Desktop complained to me that it's only available on Windows. :) > > I expanded the scope a bit to let us give the

Re: My first contribution. Outreachy

[Note that your email didn't make it to the mailing list because it was formatted as HTML. You may want to tweak your gmail settings to send plain text. Those of us on the to/cc did get it, though]. On Tue, Sep 19, 2017 at 03:45:18PM +0300, Оля Тележная wrote: > If you could help me with

Re: [PATCH] git: add --no-optional-locks option

Jeff King writes: > I admit that's just adding more hand-waving to the pile. But I don't > think it really _hurts_ to leave that door open (aside from making the > documentation a bit wishy-washy). And it helps because callers would > pick up the new features automatically,

Re: [PATCH v2] add test for bug in git-mv for recursive submodules

On Mon, Sep 18, 2017 at 01:03:32PM -0700, Stefan Beller wrote: > >> Took a little while but here is a more clean patch creating individual > >> submodules for the nesting. > >> > >> Cheers Heiko > > Thanks for writing this test! No worries. :) > > Thanks. Stefan, does this look good to you

Re: [PATCH v1 1/1] test-lint: echo -e (or -E) is not portable

On Tue, Sep 19, 2017 at 01:37:14PM -0700, Jonathan Nieder wrote: > Torsten Bögershausen wrote: > > > Some implementations of `echo` support the '-e' option to enable > > backslash interpretation of the following string. > > As an addition, they support '-E' to turn it off. > >

Re: [RFC PATCH 3/5] branch: cleanup branch name validation

On Wednesday 20 September 2017 09:50 AM, Junio C Hamano wrote: Kaartic Sivaraam writes: -int validate_new_branchname(const char *name, struct strbuf *ref, - int force, int attr_only) +int validate_branch_update(const char *name, struct

[ANNOUNCE] Git Rev News edition 31

Hi everyone, The 31st edition of Git Rev News is now published: https://git.github.io/rev_news/2017/09/20/edition-31/ Thanks a lot to all the contributors and helpers! Enjoy, Christian, Thomas, Jakub and Markus.

Re: [PATCH v7 04/12] fsmonitor: teach git to optionally utilize a file system monitor to speed up detecting new or changed files.

On 9/19/2017 10:28 PM, Junio C Hamano wrote: Ben Peart writes: +/* do stat comparison even if CE_FSMONITOR_VALID is true */ +#define CE_MATCH_IGNORE_FSMONITOR 0X20 Hmm, when should a programmer use this flag? Pretty much the same places you would also use

Re: [RFC PATCH 1/5] builtin/checkout: avoid usage of '!!'

Wait, I missed a contradiction here. On Wednesday 20 September 2017 09:30 AM, Junio C Hamano wrote:   And !!ptr is a shorter and more established way than ptr != NULL to turn non-NULL ness into an int boolean, Documentation/SubmittingPatches says:  - Some clever tricks, like using the