* Re: [RFC] stash --continue
From: Samuel Lijin @ 2017-01-18 19:35 UTC (permalink / raw)
To: Marc Branchaud; +Cc: Johannes Schindelin, Stephan Beyer, git
In-Reply-To: <38d592b8-975c-1fd9-4c42-877e34a4ab70@xiplink.com>
>> At least `git stash pop --continue` would be consistent with all other
>> `--continue` options in Git that I can think of...
> Alas, I disagree!
I'm with Johannes here. "git stash" sans subcommand is pretty
explicitly defined as "git stash save", so by similar logic, "git
stash --continue", if anything, would be "git stash save --continue".
I do agree that there's a slight problem with hunting down consistency
in implementations of --continue since there aren't other usages that
involve subcommands (rebase, cp, merge) but I can't think of "git
stash" as a completely specified command, whereas I do see "git stash
pop" and "git stash apply" as completely specified.
On Wed, Jan 18, 2017 at 12:44 PM, Marc Branchaud <marcnarc@xiplink.com> wrote:
> On 2017-01-18 11:34 AM, Johannes Schindelin wrote:
>>
>> Hi Marc,
>>
>> On Wed, 18 Jan 2017, Marc Branchaud wrote:
>>
>>> On 2017-01-16 05:54 AM, Johannes Schindelin wrote:
>>>
>>>> On Mon, 16 Jan 2017, Stephan Beyer wrote:
>>>>
>>>>> a git-newbie-ish co-worker uses git-stash sometimes. Last time he
>>>>> used "git stash pop", he got into a merge conflict. After he
>>>>> resolved the conflict, he did not know what to do to get the
>>>>> repository into the wanted state. In his case, it was only "git add
>>>>> <resolved files>" followed by a "git reset" and a "git stash drop",
>>>>> but there may be more involved cases when your index is not clean
>>>>> before "git stash pop" and you want to have your index as before.
>>>>>
>>>>> This led to the idea to have something like "git stash
>>>>> --continue"[1]
>>>>
>>>>
>>>> More like "git stash pop --continue". Without the "pop" command, it
>>>> does not make too much sense.
>>>
>>>
>>> Why not? git should be able to remember what stash command created the
>>> conflict. Why should I have to? Maybe the fire alarm goes off right
>>> when I
>>> run the stash command, and by the time I get back to it I can't remember
>>> which operation I did. It would be nice to be able to tell git to "just
>>> finish off (or abort) the stash operation, whatever it was".
>>
>>
>> That reeks of a big potential for confusion.
>>
>> Imagine for example a total Git noob who calls `git stash list`, scrolls
>> two pages down, then hits `q` by mistake. How would you explain to that
>> user that `git stash --continue` does not continue showing the list at the
>> third page?
>
>
> Sorry, but I have trouble taking that example seriously. It assumes such a
> level of "noobness" that the user doesn't even understand how standard
> command output paging works, not just with git but with any shell command.
>
>> Even worse: `git stash` (without arguments) defaults to the `save`
>> operation, so any user who does not read the documentation (and who does?)
>> would assume that `git stash --continue` *also* implies `save`.
>
>
> Like the first example, your user is trying to "continue" a command that is
> already complete. It's like try to do "git rebase --continue" when there's
> no rebase operation underway.
>
> Now, maybe there is some way for "git stash save" (implied or explicit) to
> stop partway through the operation. I can't imagine such a situation (out
> of disk space, maybe?), particularly where the user would expect "git stash
> save" to leave things in a half-finished state. To me "git stash save"
> should be essentially all-or-nothing.
>
> However, if there were such a partial-failure scenario, then I think it
> would be perfectly reasonable for "git stash --continue" to finish the save
> operation, assuming that the failure condition has been resolved.
>
>> If that was not enough, there would still be the overall design of Git's
>> user interface. You can call it confusing, inconsistent, with a lot of
>> room for improvement, and you would be correct. But none of Git's commands
>> has a `--continue` option that remembers the latest subcommand and
>> continues that. To introduce that behavior in `git stash` would disimprove
>> the situation.
>
>
> I think it's more the case that none of the current continuable commands
> have subcommands (though I can't think of all the continuable or abortable
> operations offhand, so maybe I'm wrong). I think we're discussing new UI
> ground here.
>
> And since the pattern is already "git foo --continue", it seems more
> consistent to me for it to be "git stash --continue" as well. Especially
> since there can be only one partially-complete stash sub-operation at one
> time (per workdir, at least). So there's no reason to change the pattern
> just for the stash command.
>
> Think of it this way: All the currently continuable/abortable commands put
> the repository in a shaky state, where performing certain other operations
> would be ill advised. Attempting to start a rebase while a merge conflict
> is unresolved, for example. IIRC, git actually tries to stop users from
> shooting their feet in this way.
>
> And so it should be for the stash operation: If applying a stash yields a
> conflict, it has to be resolved or aborted before something like a rebase or
> merge is attempted. It doesn't matter which stash subcommand created the
> shaky situation.
>
> In the long run, I think there's even the possibility of generic "git
> continue" and "git abort" commands, that simply continue or abort the
> current partially-complete operation, whatever it is. (Isn't that the
> ultimate goal of all the "sequencer" work? I admit I have not been
> following that effort.)
>
>> With every new feature, it is not enough to consider its benefits. You
>> always have to take the potential fallout into account, too.
>
>
> Agreed.
>
>> At least `git stash pop --continue` would be consistent with all other
>> `--continue` options in Git that I can think of...
>
>
> Alas, I disagree!
>
> M.
>
^ permalink raw reply
* Re: [PATCH v3 1/5] doc: add documentation for OPT_STRING_LIST
From: Junio C Hamano @ 2017-01-18 19:45 UTC (permalink / raw)
To: Jacob Keller; +Cc: git, Johannes Sixt, Johannes Schindelin, Jacob Keller
In-Reply-To: <20170118000930.5431-2-jacob.e.keller@intel.com>
Jacob Keller <jacob.e.keller@intel.com> writes:
> From: Jacob Keller <jacob.keller@gmail.com>
>
> Commit c8ba16391655 ("parse-options: add OPT_STRING_LIST helper",
> 2011-06-09) added the OPT_STRING_LIST as a way to accumulate a repeated
> list of strings. However, this was not documented in the
> api-parse-options documentation. Add documentation now so that future
> developers may learn of its existence.
>
> Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
> ---
> Documentation/technical/api-parse-options.txt | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/Documentation/technical/api-parse-options.txt b/Documentation/technical/api-parse-options.txt
> index 27bd701c0d68..6914f54f5f44 100644
> --- a/Documentation/technical/api-parse-options.txt
> +++ b/Documentation/technical/api-parse-options.txt
> @@ -168,6 +168,11 @@ There are some macros to easily define options:
> Introduce an option with string argument.
> The string argument is put into `str_var`.
>
> +`OPT_STRING_LIST(short, long, &list, arg_str, description)`::
> + Introduce an option with string argument.
> + The string argument is stored as an element in `&list` which must be a
> + struct string_list. Reset the list using `--no-option`.
> +
I do not know if it is clear enough that 'option' in the last
sentence is a placeholder. I then wondered if spelling it as
`--no-<long>` would make it a bit clearer, but that is ugly.
The "Reset the list" is an instruction to the end-users who interact
with a program written by readers of this document using
OPT_STRING_LIST(), and it feels a bit out of place. Perhaps
End users can reset the list by negating the option,
i.e. passing "--no-<long>", on the command line.
I dunno.
Anyway, thanks for adding a missing doc here.
> `OPT_INTEGER(short, long, &int_var, description)`::
> Introduce an option with integer argument.
> The integer is put into `int_var`.
^ permalink raw reply
* Re: [PATCH v3 2/5] name-rev: extend --refs to accept multiple patterns
From: Junio C Hamano @ 2017-01-18 20:04 UTC (permalink / raw)
To: Jacob Keller; +Cc: git, Johannes Sixt, Johannes Schindelin, Jacob Keller
In-Reply-To: <20170118000930.5431-3-jacob.e.keller@intel.com>
Jacob Keller <jacob.e.keller@intel.com> writes:
> From: Jacob Keller <jacob.keller@gmail.com>
>
> Teach git name-rev to take a string list of patterns from --refs instead
> of only a single pattern. The list of patterns will be matched
> inclusively, such that a ref only needs to match one pattern to be
> included. If a ref will only be excluded if it does not match any of the
> patterns.
I think "If a" in the last sentence should be "A".
> --refs=<pattern>::
> Only use refs whose names match a given shell pattern. The pattern
> - can be one of branch name, tag name or fully qualified ref name.
> + can be one of branch name, tag name or fully qualified ref name. If
> + given multiple times, use refs whose names match any of the given shell
> + patterns. Use `--no-refs` to clear any previous ref patterns given.
Unlike 1/5, this is facing the end-users, and the last sentence is
very appropriate.
> + if (data->ref_filters.nr) {
> + struct string_list_item *item;
> + int matched = 0;
> +
> + /* See if any of the patterns match. */
> + for_each_string_list_item(item, &data->ref_filters) {
> + /*
> + * We want to check every pattern even if we already
> + * found a match, just in case one of the later
> + * patterns could abbreviate the output.
> + */
> + switch (subpath_matches(path, item->string)) {
> + case -1: /* did not match */
> + break;
> + case 0: /* matched fully */
> + matched = 1;
> + break;
> + default: /* matched subpath */
> + matched = 1;
> + can_abbreviate_output = 1;
> + break;
> + }
> }
I agree that we cannot short-cut on the first match to make sure
that the outcome is stable, but I wondered what should be shown when
we do have multiple matches. Say I gave
--refs="v*" --refs="refs/tags/v1.*"
and refs/tags/v1.0 matched. The above code would say we can
abbreviate.
What is the reason behind this design decision? Is it because it is
clear that the user shows her willingness to accept more compact
form by having --refs="v*" that would allow shortening? If that is
the case, I think I agree with the reasoning. But we probably want
to write it down somewhere, because another reasoning, which may
also be valid, would call for an opposite behaviour (i.e. the more
specific --refs="refs/tags/v1.*" also matched, so let's show that
fact by not shortening).
^ permalink raw reply
* Re: [PATCH v3 1/5] doc: add documentation for OPT_STRING_LIST
From: Philip Oakley @ 2017-01-18 20:08 UTC (permalink / raw)
To: Junio C Hamano, Jacob Keller
Cc: git, Johannes Sixt, Johannes Schindelin, Jacob Keller
In-Reply-To: <xmqq37gg9moc.fsf@gitster.mtv.corp.google.com>
From: "Junio C Hamano" <gitster@pobox.com>
v3 1/5] doc: add documentation for OPT_STRING_LIST
> Jacob Keller <jacob.e.keller@intel.com> writes:
>
>> From: Jacob Keller <jacob.keller@gmail.com>
>>
>> Commit c8ba16391655 ("parse-options: add OPT_STRING_LIST helper",
>> 2011-06-09) added the OPT_STRING_LIST as a way to accumulate a repeated
>> list of strings. However, this was not documented in the
>> api-parse-options documentation. Add documentation now so that future
>> developers may learn of its existence.
>>
>> Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
>> ---
>> Documentation/technical/api-parse-options.txt | 5 +++++
>> 1 file changed, 5 insertions(+)
>>
>> diff --git a/Documentation/technical/api-parse-options.txt
>> b/Documentation/technical/api-parse-options.txt
>> index 27bd701c0d68..6914f54f5f44 100644
>> --- a/Documentation/technical/api-parse-options.txt
>> +++ b/Documentation/technical/api-parse-options.txt
>> @@ -168,6 +168,11 @@ There are some macros to easily define options:
>> Introduce an option with string argument.
>> The string argument is put into `str_var`.
>>
>> +`OPT_STRING_LIST(short, long, &list, arg_str, description)`::
>> + Introduce an option with string argument.
>> + The string argument is stored as an element in `&list` which must be a
>> + struct string_list. Reset the list using `--no-option`.
>> +
>
> I do not know if it is clear enough that 'option' in the last
> sentence is a placeholder. I then wondered if spelling it as
> `--no-<long>` would make it a bit clearer, but that is ugly.
Bikeshedding:: `--no-<option>` maybe, i.e. just surround the option word
with the angle brackets to indicate it is to be replaced by the real
option's name.
>
> The "Reset the list" is an instruction to the end-users who interact
> with a program written by readers of this document using
> OPT_STRING_LIST(), and it feels a bit out of place. Perhaps
>
> End users can reset the list by negating the option,
> i.e. passing "--no-<long>", on the command line.
>
> I dunno.
>
> Anyway, thanks for adding a missing doc here.
>
>> `OPT_INTEGER(short, long, &int_var, description)`::
>> Introduce an option with integer argument.
>> The integer is put into `int_var`.
>
--
Philip
^ permalink raw reply
* Re: [PATCH v6 4/6] builtin/tag: add --format argument for tag -v
From: Eric Wong @ 2017-01-18 20:16 UTC (permalink / raw)
To: Junio C Hamano
Cc: Santiago Torres, meta, git, peff, sunshine, walters,
Lukas Puehringer
In-Reply-To: <xmqq4m0wb43w.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> wrote:
> Santiago Torres <santiago@nyu.edu> writes:
>
> <<nothing>>???
>
> Eric, I've noticed that this message
>
> http://public-inbox.org/git/20170118182831.pkhqu2np3bh2puei@LykOS.localdomain/
>
> and all messages from Santiago appear empty when they come via
> public-inbox.org; the reason I suspect we haven't heard much
> complaints is because nobody else around here sends multipart/signed
> disposition inline other than Santiago.
Eeep! This looks like a regression I introduced when working
around Richard Hansen's S/MIME mails the other week on git@vger:
https://public-inbox.org/meta/20170110222235.GB27356@dcvr/T/#u
Worse is they now corrupted on the way in into the git repo
because of search indexing. Will fix ASAP. Thanks for the
heads up.
^ permalink raw reply
* Re: [PATCH v3 3/5] name-rev: add support to exclude refs by pattern match
From: Junio C Hamano @ 2017-01-18 20:11 UTC (permalink / raw)
To: Jacob Keller; +Cc: git, Johannes Sixt, Johannes Schindelin, Jacob Keller
In-Reply-To: <20170118000930.5431-4-jacob.e.keller@intel.com>
Jacob Keller <jacob.e.keller@intel.com> writes:
> From: Jacob Keller <jacob.keller@gmail.com>
>
> Extend name-rev further to support matching refs by adding `--exclude`
> patterns. These patterns will limit the scope of refs by excluding any
> ref that matches at least one exclude pattern. Checking the exclude refs
> shall happen first, before checking the include --refs patterns.
I do not think we should have this "exclude first and then include"
written down here, as it is an irrelevant implementation detail.
The desired end result is that only refs that match at least one
include and none of the exclude survive. You could implement it by
first checking with include and then further narrowing that set by
filtering those that match exclude (I am not saying that "include
first then exclude" is better---I am saying that it is far less
important than "at least one include and none of the exclude" to
mention the order of application).
> +--exclude=<pattern>::
> + Do not use any ref whose name matches a given shell pattern. The
> + pattern can be one of branch name, tag name or fully qualified ref
> + name. If given multiple times, exclude refs that match any of the given
> + shell patterns. Use `--no-exclude` to clear the list of exclude
> + patterns.
Perhaps insert
When used together with --refs, only those that match at least
one of the --refs patterns and none of the --exclude patterns
are used.
before "Use `--no-exclude` to clear"?
^ permalink raw reply
* Re: [PATCH v3 5/5] describe: teach describe negative pattern matches
From: Junio C Hamano @ 2017-01-18 20:17 UTC (permalink / raw)
To: Jacob Keller; +Cc: git, Johannes Sixt, Johannes Schindelin, Jacob Keller
In-Reply-To: <20170118000930.5431-6-jacob.e.keller@intel.com>
Jacob Keller <jacob.e.keller@intel.com> writes:
> From: Jacob Keller <jacob.keller@gmail.com>
>
> Teach git-describe the `--exclude` option which will allow specifying
> a glob pattern of tags to ignore. This can be combined with the
> `--match` patterns to enable more flexibility in determining which tags
> to consider.
>
> For example, suppose you wish to find the first official release tag
> that contains a certain commit. If we assume that official release tags
> are of the form "v*" and pre-release candidates include "*rc*" in their
> name, we can now find the first tag that introduces commit abcdef via:
>
> git describe --contains --match="v*" --exclude="*rc*"
>
> Add documentation and tests for this change.
>
> Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
> ---
The above is much better than 3/5 with a concrete example (compared
to the vague "certain kinds of references"). It also does not have
the "we check this first and then that" ;-).
> diff --git a/Documentation/git-describe.txt b/Documentation/git-describe.txt
> index 7ad41e2f6ade..21a43b78924a 100644
> --- a/Documentation/git-describe.txt
> +++ b/Documentation/git-describe.txt
> @@ -88,6 +88,14 @@ OPTIONS
> patterns will be considered. Use `--no-match` to clear and reset the
> list of patterns.
>
> +--exclude <pattern>::
> + Do not consider tags matching the given `glob(7)` pattern, excluding
> + the "refs/tags/" prefix. This can be used to narrow the tag space and
> + find only tags matching some meaningful criteria. If given multiple
> + times, a list of patterns will be accumulated and tags matching any
> + of the patterns will be excluded. Use `--no-exclude` to clear and
> + reset the list of patterns.
> +
Similar to 3/5, perhaps we want to say something about interaction
between this one and --match?
^ permalink raw reply
* Re: [PATCH v3 0/5] extend git-describe pattern matching
From: Junio C Hamano @ 2017-01-18 20:18 UTC (permalink / raw)
To: Jacob Keller; +Cc: git, Johannes Sixt, Johannes Schindelin, Jacob Keller
In-Reply-To: <20170118000930.5431-1-jacob.e.keller@intel.com>
Jacob Keller <jacob.e.keller@intel.com> writes:
> From: Jacob Keller <jacob.keller@gmail.com>
>
> ** v3 fixes a minor typo in one of the test cases, so please ignore v2
> I left the interdiff as between v1 and v3 instead of v2 **
Very much appreciated.
I just finished reading this round through and didn't have any major
issues. I sent comments on the way the feature is explained to the
end users and history readers, though.
Thanks.
^ permalink raw reply
* Re: [PATCH 21/27] attr: use hashmap for attribute dictionary
From: Stefan Beller @ 2017-01-18 20:20 UTC (permalink / raw)
To: Brandon Williams; +Cc: git@vger.kernel.org, Junio C Hamano, Duy Nguyen
In-Reply-To: <20170112235354.153403-22-bmwill@google.com>
On Thu, Jan 12, 2017 at 3:53 PM, Brandon Williams <bmwill@google.com> wrote:
> +/* Initialize an 'attr_hashmap' object */
> +void attr_hashmap_init(struct attr_hashmap *map)
In case a reroll is needed, mark this static please.
^ permalink raw reply
* Re: [PATCH 21/27] attr: use hashmap for attribute dictionary
From: Brandon Williams @ 2017-01-18 20:23 UTC (permalink / raw)
To: Stefan Beller; +Cc: git@vger.kernel.org, Junio C Hamano, Duy Nguyen
In-Reply-To: <CAGZ79kYdcVNK9KJgM0Na0MJ4QuGM+8OJxFb1oQ+VoDL--Ay48Q@mail.gmail.com>
On 01/18, Stefan Beller wrote:
> On Thu, Jan 12, 2017 at 3:53 PM, Brandon Williams <bmwill@google.com> wrote:
>
> > +/* Initialize an 'attr_hashmap' object */
> > +void attr_hashmap_init(struct attr_hashmap *map)
>
> In case a reroll is needed, mark this static please.
Will do.
--
Brandon Williams
^ permalink raw reply
* Re: git fast-import crashing on big imports
From: Jeff King @ 2017-01-18 20:06 UTC (permalink / raw)
To: Ulrich Spoerlein; +Cc: git, Ed Maste, Junio C Hamano
In-Reply-To: <20170118143814.or34vxxwjwnzg5jz@sigill.intra.peff.net>
On Wed, Jan 18, 2017 at 09:38:14AM -0500, Jeff King wrote:
> On Wed, Jan 18, 2017 at 03:01:17PM +0100, Ulrich Spoerlein wrote:
>
> > Yo Jeff, your commit 8261e1f139db3f8aa6f9fd7d98c876cbeb0f927c from Aug
> > 22nd, that changes delta_base_cache to use hashmap.h is the culprit for
> > git fast-import crashing on large imports.
>
> I actually saw your bug report the other day and tried to download the
> dump file, but got a 404. Can you double check that it is available?
The URL in your original mail was bogus:
> > I have a dump file that you can grab at
> > http://scan.spoerlein.net/pub/freebsd-base.dump.xz (19G, 55G uncompressed)
Via guess-and-check, I found:
http://www.spoerlein.net/pub/freebsd-base.git.fi.xz
I'll see if I can reproduce the problem.
-Peff
^ permalink raw reply
* Re: git fast-import crashing on big imports
From: Jeff King @ 2017-01-18 20:27 UTC (permalink / raw)
To: Ulrich Spörlein; +Cc: Ed Maste, git, Junio C Hamano
In-Reply-To: <CAJ9axoSzZJXD4RKvVx+D60dw4sakMJWgNmOP-cREWA53Ae3C3w@mail.gmail.com>
On Wed, Jan 18, 2017 at 09:20:07PM +0100, Ulrich Spörlein wrote:
> I found your commit via bisect in case you were wondering. Thanks for
> picking this up.
Still downloading. However, just looking at the code, the obvious
culprit would be clear_delta_base_cache(), which is called from
literally nowhere except fast-import, and then only when checkpointing.
-Peff
^ permalink raw reply
* Re: [PATCH 25/27] attr: store attribute stacks in hashmap
From: Brandon Williams @ 2017-01-18 20:34 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, pclouds, sbeller
In-Reply-To: <xmqqshomejwt.fsf@gitster.mtv.corp.google.com>
On 01/13, Junio C Hamano wrote:
> Brandon Williams <bmwill@google.com> writes:
>
> > The last big hurdle towards a thread-safe API for the attribute system
> > is the reliance on a global attribute stack that is modified during each
> > call into the attribute system.
> >
> > This patch removes this global stack and instead a stack is retrieved or
> > constructed locally. Since each of these stacks is only used as a
> > read-only structure once constructed, they can be stored in a hashmap
> > and shared between threads.
>
> Very good.
>
> The reason why the original code used a stack was because it wanted
> to keep only the info read from releavant files in-core, discarding
> ones from files no-longer relevant (because the traversal switched
> to another subdirectory of the same parent directory), to avoid the
> memory consumption grow unbounded. It probably was a premature
> "optimization" that we can do without, so keeping everything we have
> read so far in a hashmap (which is my understanding of what is going
> on in this patch) is probably OK.
>
> I suspect that this hashmap may eventually need to become per
> attr_check if we want to follow through the optimization envisioned
> by patch 15/27.
>
> Inside fill(), path_matches() is called for the number of match_attr
> in the entire attribute stack but it is wasteful to check if the
> path matches with the a.u.pat if none of the a.state[] entries talk
> about attributes and macros that are eventually get used by the
> caller of check_attr(). By introducing a wrapping structure, 15/27
> wanted to make sure that we have a place to store a "reduced"
> attribute stack that is kept per attr_check that has only entries
> from the files that talk about the attributes the particular
> attr_check wants to learn about.
>
> I need to think about this a bit more, but I do not offhand think
> that it makes future such enhancement to make it per-check harder to
> move from a global stack to a global hashmap, i.e. the above is not
> an objection to this step.
If we want to continue through and do the optimization you originally
envisioned then I may need to rethink this patch. One thing we did talk
about offline was doing another check prior to the path_match() function
call which looks through the list of state structs to see if one of
those states would actually have an affect on the array being used to
collect attributes. Though that may be an optimization which can be
done in addition to creating a reduced stack.
The one difficulty (which you pointed out in comment form) is if we have
a reduced attribute stack that is stored per attr_check then handling
the cleanup when the direction is changed may be messy.
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH 25/27] attr: store attribute stacks in hashmap
From: Stefan Beller @ 2017-01-18 20:39 UTC (permalink / raw)
To: Brandon Williams; +Cc: git@vger.kernel.org, Junio C Hamano, Duy Nguyen
In-Reply-To: <20170112235354.153403-26-bmwill@google.com>
On Thu, Jan 12, 2017 at 3:53 PM, Brandon Williams <bmwill@google.com> wrote:
> -static void prepare_attr_stack(const char *path, int dirlen)
> +/*
> + * This funciton should only be called from 'get_attr_stack()', which already
"function"
> + /* system-wide frame */
> + if (git_attr_system()) {
> + e = read_attr_from_file(git_etc_gitattributes(), 1);
read_attr_from_file may return NULL, so we'd have to treat this similar
to below "root directory", i.e. xcalloc for an empty frame?
> +
> + /* root directory */
> + if (!is_bare_repository() || direction == GIT_ATTR_INDEX) {
> + e = read_attr(GITATTRIBUTES_FILE, 1);
> + } else {
> + e = xcalloc(1, sizeof(struct attr_stack));
> + }
> + key = "";
> + push_stack(&core, e, key, strlen(key));
If this is a bare repo, could we just omit this frame instead of pushing
an empty xcalloc'd frame? (Same for the stack frames of system wide
and home dir) ?
^ permalink raw reply
* Re: [PATCH 25/27] attr: store attribute stacks in hashmap
From: Brandon Williams @ 2017-01-18 20:50 UTC (permalink / raw)
To: Stefan Beller; +Cc: git@vger.kernel.org, Junio C Hamano, Duy Nguyen
In-Reply-To: <CAGZ79kZv2=dNt=TeJXbac4S20WcdOZo=iVa-b+4zkOoGVM1OFA@mail.gmail.com>
On 01/18, Stefan Beller wrote:
> On Thu, Jan 12, 2017 at 3:53 PM, Brandon Williams <bmwill@google.com> wrote:
> > -static void prepare_attr_stack(const char *path, int dirlen)
> > +/*
> > + * This funciton should only be called from 'get_attr_stack()', which already
>
> "function"
>
> > + /* system-wide frame */
> > + if (git_attr_system()) {
> > + e = read_attr_from_file(git_etc_gitattributes(), 1);
>
> read_attr_from_file may return NULL, so we'd have to treat this similar
> to below "root directory", i.e. xcalloc for an empty frame?
The push_stack function doesn't do anything if 'e' is NULL, so we should
be fine here.
>
> > +
> > + /* root directory */
> > + if (!is_bare_repository() || direction == GIT_ATTR_INDEX) {
> > + e = read_attr(GITATTRIBUTES_FILE, 1);
> > + } else {
> > + e = xcalloc(1, sizeof(struct attr_stack));
> > + }
> > + key = "";
> > + push_stack(&core, e, key, strlen(key));
>
> If this is a bare repo, could we just omit this frame instead of pushing
> an empty xcalloc'd frame? (Same for the stack frames of system wide
> and home dir) ?
The reasoning behind having the object created even if its a bare repo
is so that later we can easily see that a frame has been read and
included and doesn't need to attempt to reread the frame from disk
later. It also made things simpler when storing the object in a hashmap
since storing a NULL ptr was awkward.
Though looking at Junio's discussion we may want to rethink how the
stacks are handled. I still need to think about it some more.
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH v3 0/5] extend git-describe pattern matching
From: Jacob Keller @ 2017-01-18 21:06 UTC (permalink / raw)
To: Junio C Hamano
Cc: Jacob Keller, Git mailing list, Johannes Sixt,
Johannes Schindelin
In-Reply-To: <xmqqlgu886lf.fsf@gitster.mtv.corp.google.com>
On Wed, Jan 18, 2017 at 12:18 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Jacob Keller <jacob.e.keller@intel.com> writes:
>
>> From: Jacob Keller <jacob.keller@gmail.com>
>>
>> ** v3 fixes a minor typo in one of the test cases, so please ignore v2
>> I left the interdiff as between v1 and v3 instead of v2 **
>
> Very much appreciated.
>
> I just finished reading this round through and didn't have any major
> issues. I sent comments on the way the feature is explained to the
> end users and history readers, though.
>
> Thanks.
Ok. I am currently taking a look at the exclude_list that was
mentioned, and I will see if it makes sense to go that route. If we
do, it would open up a way to make the git-describe logic similar to
git-log (which is probably good), and if we want to do this, it's best
to do it now before we add any additional arguments (ie: before we
create a conflicting user interface).
Thanks,
Jake
^ permalink raw reply
* Re: "git diff --ignore-space-change --stat" lists files with only whitespace differences as "changed"
From: Jeff King @ 2017-01-18 21:08 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Matt McCutchen, git
In-Reply-To: <xmqqvatc3x3r.fsf@gitster.mtv.corp.google.com>
On Wed, Jan 18, 2017 at 12:57:12PM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > So I dunno. A sensible rule to me is "iff -p would show a diff header,
> > then --stat should mention it".
>
> True but tricky (you need a better definition of "a diff header").
>
> In addition to a new and deleted file, does a file whose executable
> bit was flipped need mention? If so, then "diff --git" is the diff
> header in the above. Otherwise "@@ ... @@", iow, "iff -p would show
> any hunk".
>
> I think the patch implements the latter, which I think is sensible.
I would think the former is more sensible (and is what my patch is
working towards). Doing:
>empty
git add empty
git diff --cached
shows a "diff --git" header, but no hunk. I think it should show a
diffstat (and does with my patch).
I was thinking the rule should be something like:
if (p->status == DIFF_STATUS_MODIFIED &&
!file->added && !file->deleted))
and otherwise include the entry, since it would be an add, delete,
rename, etc, which carries useful information.
Though a pure rename would not hit this code path at all, I would think
(it would not trigger "!same_contents"). And a rename where there was a
whitespace only change probably _should_ be omitted from "-b".
Ditto for a pure mode change, I think. We do not run the contents
through diff at all, so it does not hit this code path.
-Peff
^ permalink raw reply
* Re: [PATCH v3 1/5] doc: add documentation for OPT_STRING_LIST
From: Jacob Keller @ 2017-01-18 21:10 UTC (permalink / raw)
To: Junio C Hamano
Cc: Jacob Keller, Git mailing list, Johannes Sixt,
Johannes Schindelin
In-Reply-To: <xmqq37gg9moc.fsf@gitster.mtv.corp.google.com>
On Wed, Jan 18, 2017 at 11:45 AM, Junio C Hamano <gitster@pobox.com> wrote:
>
> I do not know if it is clear enough that 'option' in the last
> sentence is a placeholder. I then wondered if spelling it as
> `--no-<long>` would make it a bit clearer, but that is ugly.
>
To be fair, this is exactly how the rest of the doc spells these
things, so I would rather be consistent with the doc as is, and a
future patch could clean this up. See OPT_SET_INT, for an example of
`--no-option`.
> The "Reset the list" is an instruction to the end-users who interact
> with a program written by readers of this document using
> OPT_STRING_LIST(), and it feels a bit out of place. Perhaps
>
> End users can reset the list by negating the option,
> i.e. passing "--no-<long>", on the command line.
>
> I dunno.
Maybe we can rephrase this "The list is reset via `--no-option`"?
Thanks,
Jake
^ permalink raw reply
* Re: [PATCH 1/4] document index_name_pos
From: Junio C Hamano @ 2017-01-18 21:11 UTC (permalink / raw)
To: Stefan Beller; +Cc: git
In-Reply-To: <20170117233503.27137-2-sbeller@google.com>
Stefan Beller <sbeller@google.com> writes:
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> cache.h | 15 +++++++++++++++
> 1 file changed, 15 insertions(+)
>
> diff --git a/cache.h b/cache.h
> index 1b67f078dd..270a0d0ea7 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -575,7 +575,22 @@ extern int verify_path(const char *path);
> extern int index_dir_exists(struct index_state *istate, const char *name, int namelen);
> extern void adjust_dirname_case(struct index_state *istate, char *name);
> extern struct cache_entry *index_file_exists(struct index_state *istate, const char *name, int namelen, int igncase);
> +
> +/*
> + * Searches for an entry defined by name and namelen in the given index.
> + * If the return value is positive (including 0) it is the position of an
> + * exact match. If the return value is negative, the negated value minus 1 is the
> + * position where the entry would be inserted.
> + * Example: In the current index we have the files b,d,e:
> + * index_name_pos(&index, "a", 1) -> -1
> + * index_name_pos(&index, "b", 1) -> 0
> + * index_name_pos(&index, "c", 1) -> -2
> + * index_name_pos(&index, "d", 1) -> 1
> + * index_name_pos(&index, "e", 1) -> 2
The above may not be wrong per-se, but it misses one important case.
A conflicted entry in the index with the same name is considered to
sort after the name this asks. If there are stage #1 and stage #3
entries for path "g" in addition to the above, i.e.
[0] [1] [2] [3] [4]
b#0 d#0 e#0 g#1 g#3
then
index_name_pos(&index, "g", 1) -> -3 - 1 = -4
index_name_pos(&index, "h", 1) -> -5 - 1 = -6
> + * index_name_pos(&index, "f", 1) -> -3
> + */
Shouldn't this be -4? We originally have [0], [1], and [2] in the
index, and "f" needs to go to [3], so -3 - 1 = -4, no?
> extern int index_name_pos(const struct index_state *, const char *name, int namelen);
> +
> #define ADD_CACHE_OK_TO_ADD 1 /* Ok to add */
> #define ADD_CACHE_OK_TO_REPLACE 2 /* Ok to replace file/directory */
> #define ADD_CACHE_SKIP_DFCHECK 4 /* Ok to skip DF conflict checks */
^ permalink raw reply
* Re: [PATCH 5/5] describe: teach describe negative pattern matches
From: Jacob Keller @ 2017-01-18 21:04 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Johannes Sixt, Jacob Keller, Git mailing list
In-Reply-To: <alpine.DEB.2.20.1701181340530.3469@virtualbox>
On Wed, Jan 18, 2017 at 4:44 AM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi Jake,
>
> On Tue, 17 Jan 2017, Jacob Keller wrote:
>
>> On Fri, Jan 13, 2017 at 1:31 PM, Johannes Sixt <j6t@kdbg.org> wrote:
>> > Am 13.01.2017 um 07:57 schrieb Jacob Keller:
>> >>
>> >> On Thu, Jan 12, 2017 at 10:43 PM, Johannes Sixt <j6t@kdbg.org> wrote:
>> >>>
>> >>> When you write
>> >>>
>> >>> git log --branches --exclude=origin/* --remotes
>> >>>
>> >>> --exclude=origin/* applies only to --remotes, but not to --branches.
>> >>
>> >>
>> >> Well for describe I don't think the order matters.
>> >
>> >
>> > That is certainly true today. But I would value consistency more. We would
>> > lose it if some time in the future 'describe' accepts --branches and
>> > --remotes in addition to --tags and --all.
>> >
>> > -- Hannes
>> >
>>
>> I am not sure that the interface for git-log and git-describe are
>> similar enough to make this distinction work. --match already seems to
>> imply that it only works on refs in refs/tags, as it says it considers
>> globs matching excluding the "refs/tags" prefix.
>>
>> In git-describe, we already have "--tags" and "--all" but they are
>> mutually exclusive. We don't support using more than one at once, and
>> I'm not really convinced that describe will ever support more than one
>> at a time. Additionally, match already doesn't respect order.
>
> I agree that it would keep the code much simpler if you kept the order
> "exclude before include".
>
> However, should you decide to look into making the logic dependent on the
> order in which the flags were specified in the command-line, we do have a
> data structure for such a beast: we use it in gitignore and in
> sparse-checkout, it is called struct exclude_list.
>
> Just some food for thought,
> Johannes
That might help make it easier to go this route. I'll take a look.
Thanks,
Jake
^ permalink raw reply
* Re: [PATCH] mingw: follow-up to "replace isatty() hack"
From: Johannes Sixt @ 2017-01-18 21:16 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Schindelin, git, Pranit Bauva, Beat Bolli
In-Reply-To: <xmqq7f5s9nvt.fsf@gitster.mtv.corp.google.com>
Am 18.01.2017 um 20:19 schrieb Junio C Hamano:
> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>> compat/winansi.c | 11 ++++-------
>> 1 file changed, 4 insertions(+), 7 deletions(-)
>>
>> diff --git a/compat/winansi.c b/compat/winansi.c
>> index 3c9ed3cfe0..82b89ab137 100644
>> --- a/compat/winansi.c
>> +++ b/compat/winansi.c
>> @@ -494,19 +494,16 @@ static HANDLE swap_osfhnd(int fd, HANDLE new_handle)
>> * It is because of this implicit close() that we created the
>> * copy of the original.
>> *
>> - * Note that the OS can recycle HANDLE (numbers) just like it
>> - * recycles fd (numbers), so we must update the cached value
>> - * of "console". You can use GetFileType() to see that
>> - * handle and _get_osfhandle(fd) may have the same number
>> - * value, but they refer to different actual files now.
>> + * Note that we need to update the cached console handle to the
>> + * duplicated one because the dup2() call will implicitly close
>> + * the original one.
>> *
>> * Note that dup2() when given target := {0,1,2} will also
>> * call SetStdHandle(), so we don't need to worry about that.
>> */
>> - dup2(new_fd, fd);
>> if (console == handle)
>> console = duplicate;
>> - handle = INVALID_HANDLE_VALUE;
>> + dup2(new_fd, fd);
>>
>> /* Close the temp fd. This explicitly closes "new_handle"
>> * (because it has been associated with it).
>>
Looks good and obviously correct (FLW). I can offer a
Reviewed-by: Johannes Sixt <j6t@kdbg.org>
but it will take a day or two until I can test the patch.
-- Hannes
^ permalink raw reply
* Re: [PATCH 2/4] remove_index_entry_at: move documentation to cache.h
From: Junio C Hamano @ 2017-01-18 21:16 UTC (permalink / raw)
To: Stefan Beller; +Cc: git
In-Reply-To: <20170117233503.27137-3-sbeller@google.com>
Stefan Beller <sbeller@google.com> writes:
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> cache.h | 3 +++
> read-cache.c | 1 -
> 2 files changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/cache.h b/cache.h
> index 270a0d0ea7..26632065a5 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -599,7 +599,10 @@ extern int index_name_pos(const struct index_state *, const char *name, int name
> #define ADD_CACHE_KEEP_CACHE_TREE 32 /* Do not invalidate cache-tree */
> extern int add_index_entry(struct index_state *, struct cache_entry *ce, int option);
> extern void rename_index_entry_at(struct index_state *, int pos, const char *new_name);
> +
> +/* Remove entry, return 1 if there are more entries after pos. */
> extern int remove_index_entry_at(struct index_state *, int pos);
What is the reason why this now promise to return 1, as opposed to
the original that were allowed to return anything that is "true"?
Is it because you are adding other return values that mean different
things?
If that is the case it may be fine (it depends on what these other
values mean and what use case it supports), but please do that in a
separate patch.
> +
> extern void remove_marked_cache_entries(struct index_state *istate);
> extern int remove_file_from_index(struct index_state *, const char *path);
> #define ADD_CACHE_VERBOSE 1
> diff --git a/read-cache.c b/read-cache.c
> index 2eca639cce..63a414cdb5 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -503,7 +503,6 @@ int index_name_pos(const struct index_state *istate, const char *name, int namel
> return index_name_stage_pos(istate, name, namelen, 0);
> }
>
> -/* Remove entry, return true if there are more entries to go.. */
> int remove_index_entry_at(struct index_state *istate, int pos)
> {
> struct cache_entry *ce = istate->cache[pos];
^ permalink raw reply
* Re: [PATCH 25/27] attr: store attribute stacks in hashmap
From: Stefan Beller @ 2017-01-18 20:45 UTC (permalink / raw)
To: Brandon Williams; +Cc: git@vger.kernel.org, Junio C Hamano, Duy Nguyen
In-Reply-To: <CAGZ79kZv2=dNt=TeJXbac4S20WcdOZo=iVa-b+4zkOoGVM1OFA@mail.gmail.com>
On Wed, Jan 18, 2017 at 12:39 PM, Stefan Beller <sbeller@google.com> wrote:
> On Thu, Jan 12, 2017 at 3:53 PM, Brandon Williams <bmwill@google.com> wrote:
>> -static void prepare_attr_stack(const char *path, int dirlen)
>> +/*
>> + * This funciton should only be called from 'get_attr_stack()', which already
>
> "function"
>
>> + /* system-wide frame */
>> + if (git_attr_system()) {
>> + e = read_attr_from_file(git_etc_gitattributes(), 1);
>
> read_attr_from_file may return NULL, so we'd have to treat this similar
> to below "root directory", i.e. xcalloc for an empty frame?
>
>> +
>> + /* root directory */
>> + if (!is_bare_repository() || direction == GIT_ATTR_INDEX) {
>> + e = read_attr(GITATTRIBUTES_FILE, 1);
>> + } else {
>> + e = xcalloc(1, sizeof(struct attr_stack));
>> + }
>> + key = "";
>> + push_stack(&core, e, key, strlen(key));
>
> If this is a bare repo, could we just omit this frame instead of pushing
> an empty xcalloc'd frame? (Same for the stack frames of system wide
> and home dir) ?
The next patch moves this issue into the read_attr function.
So in the end we'd either need to fix read_attr_from_file to return
res = xcalloc(1, sizeof(*res));
if (!fp), or we need to handle NULLs appropriately in 'core_attr_stack' ?
^ permalink raw reply
* Re: [PATCH v3 2/5] name-rev: extend --refs to accept multiple patterns
From: Jacob Keller @ 2017-01-18 21:12 UTC (permalink / raw)
To: Junio C Hamano
Cc: Jacob Keller, Git mailing list, Johannes Sixt,
Johannes Schindelin
In-Reply-To: <xmqqy3y8878k.fsf@gitster.mtv.corp.google.com>
On Wed, Jan 18, 2017 at 12:04 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Jacob Keller <jacob.e.keller@intel.com> writes:
>
>> From: Jacob Keller <jacob.keller@gmail.com>
>>
>> Teach git name-rev to take a string list of patterns from --refs instead
>> of only a single pattern. The list of patterns will be matched
>> inclusively, such that a ref only needs to match one pattern to be
>> included. If a ref will only be excluded if it does not match any of the
>> patterns.
>
> I think "If a" in the last sentence should be "A".
You are correct, that is a typo.
>
>> --refs=<pattern>::
>> Only use refs whose names match a given shell pattern. The pattern
>> - can be one of branch name, tag name or fully qualified ref name.
>> + can be one of branch name, tag name or fully qualified ref name. If
>> + given multiple times, use refs whose names match any of the given shell
>> + patterns. Use `--no-refs` to clear any previous ref patterns given.
>
> Unlike 1/5, this is facing the end-users, and the last sentence is
> very appropriate.
Yes.
>
>> + if (data->ref_filters.nr) {
>> + struct string_list_item *item;
>> + int matched = 0;
>> +
>> + /* See if any of the patterns match. */
>> + for_each_string_list_item(item, &data->ref_filters) {
>> + /*
>> + * We want to check every pattern even if we already
>> + * found a match, just in case one of the later
>> + * patterns could abbreviate the output.
>> + */
>> + switch (subpath_matches(path, item->string)) {
>> + case -1: /* did not match */
>> + break;
>> + case 0: /* matched fully */
>> + matched = 1;
>> + break;
>> + default: /* matched subpath */
>> + matched = 1;
>> + can_abbreviate_output = 1;
>> + break;
>> + }
>> }
>
> I agree that we cannot short-cut on the first match to make sure
> that the outcome is stable, but I wondered what should be shown when
> we do have multiple matches. Say I gave
>
> --refs="v*" --refs="refs/tags/v1.*"
>
> and refs/tags/v1.0 matched. The above code would say we can
> abbreviate.
>
> What is the reason behind this design decision? Is it because it is
> clear that the user shows her willingness to accept more compact
> form by having --refs="v*" that would allow shortening? If that is
> the case, I think I agree with the reasoning. But we probably want
> to write it down somewhere, because another reasoning, which may
> also be valid, would call for an opposite behaviour (i.e. the more
> specific --refs="refs/tags/v1.*" also matched, so let's show that
> fact by not shortening).
>
I'm not sure which reasoning makes most sense. Any other opinions?
Thanks,
Jake
^ permalink raw reply
* Re: [PATCH 3/4] document add_[file_]to_index
From: Junio C Hamano @ 2017-01-18 21:22 UTC (permalink / raw)
To: Stefan Beller; +Cc: git
In-Reply-To: <20170117233503.27137-4-sbeller@google.com>
Stefan Beller <sbeller@google.com> writes:
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> cache.h | 17 ++++++++++++-----
> 1 file changed, 12 insertions(+), 5 deletions(-)
>
> diff --git a/cache.h b/cache.h
> index 26632065a5..acc639d6e0 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -605,13 +605,20 @@ extern int remove_index_entry_at(struct index_state *, int pos);
>
> extern void remove_marked_cache_entries(struct index_state *istate);
> extern int remove_file_from_index(struct index_state *, const char *path);
> -#define ADD_CACHE_VERBOSE 1
> -#define ADD_CACHE_PRETEND 2
> -#define ADD_CACHE_IGNORE_ERRORS 4
> -#define ADD_CACHE_IGNORE_REMOVAL 8
> -#define ADD_CACHE_INTENT 16
> +
> +#define ADD_CACHE_VERBOSE 1 /* verbose */
> +#define ADD_CACHE_PRETEND 2 /* dry run */
> +#define ADD_CACHE_IGNORE_ERRORS 4 /* ignore errors */
> +#define ADD_CACHE_IGNORE_REMOVAL 8 /* do not remove files from index */
> +#define ADD_CACHE_INTENT 16 /* intend to add later; stage empty file */
These repeat pretty much the same thing, which is an indication that
the macro names are chosen well not to require extraneous comments
like these, no?
> +/*
> + * Adds the given path the index, respecting the repsitory configuration, e.g.
> + * in case insensitive file systems, the path is normalized.
> + */
> extern int add_to_index(struct index_state *, const char *path, struct stat *, int flags);
s/repsitory/repository/;
> +/* stat the file then call add_to_index */
> extern int add_file_to_index(struct index_state *, const char *path, int flags);
> +
As you do not say "use the provided stat info to mark the cache
entry up-to-date" in the add_to_index(), I am not sure if mentioning
"stat the file then" has much value. Besides, you are supposed to
lstat(2) the file, not "stat", no?
I'd cover these two under the same heading and comment if I were
doing this.
These two are used to add the contents of the file at path
to the index, marking the working tree up-to-date by storing
the cached stat info in the resulting cache entry. A caller
that has already run lstat(2) on the path can call
add_to_index(), and all others can call add_file_to_index();
the latter will do necessary lstat(2) internally before
calling the former.
or something along that line.
> extern struct cache_entry *make_cache_entry(unsigned int mode, const unsigned char *sha1, const char *path, int stage, unsigned int refresh_options);
> extern int chmod_index_entry(struct index_state *, struct cache_entry *ce, char flip);
> extern int ce_same_name(const struct cache_entry *a, const struct cache_entry *b);
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox