* Re: [PATCH v3 2/5] name-rev: extend --refs to accept multiple patterns
From: Junio C Hamano @ 2017-01-18 22:42 UTC (permalink / raw)
To: Jacob Keller
Cc: Jacob Keller, Git mailing list, Johannes Sixt,
Johannes Schindelin
In-Reply-To: <CA+P7+xpMAVq8K41cDZy5FTiRTHoWWd3yOSmLoj4ucAvCPoNa0g@mail.gmail.com>
Jacob Keller <jacob.keller@gmail.com> writes:
> On Wed, Jan 18, 2017 at 12:04 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
>> 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?
FWIW, I do think that the design decision to declare that it can be
abbreviated if the ref matches at least one short pattern makes
sense, and I am guessing (because you didn't answer when asked what
_your_ reasoning behind the code was) that you are in agreement. I
just want it to be spelled out probably as in-code comment, so that
people who later come to this part of the code know why it was
designed that way. And they can disagree and change it if the end
result is better---I just want to make sure that they can understand
what they are disagreeing when it happens, as opposed to them
scratching their head saying "we do not know why it was chosen to be
done this way, let's make a random change to make it behave
differently".
^ permalink raw reply
* Re: [PATCH v3 3/5] name-rev: add support to exclude refs by pattern match
From: Jacob Keller @ 2017-01-18 22:31 UTC (permalink / raw)
To: Junio C Hamano
Cc: Jacob Keller, Git mailing list, Johannes Sixt,
Johannes Schindelin
In-Reply-To: <xmqqy3y82fs8.fsf@gitster.mtv.corp.google.com>
On Wed, Jan 18, 2017 at 1:56 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Jacob Keller <jacob.keller@gmail.com> writes:
>
>> Yes this makes sense. I'm still looking at whether the alternative
>> implementation suggested based on the git-log style would make more
>> sense or not, but if we keep this as is, the added text you gave is
>> important.
>
> I actually think it is a red-herring that "git log" honors "orders";
> it does, but that is not a result of carefully considering the
> desired behaviour. It instead is a historical wart that came from
> the fact that "--branches" and friends uses for_each_glob_ref_in()
> that takes the top-level hierarchy paths like "refs/heads/" and the
> implementation of "--exclude" piggybacked into the function in a
> lazy way.
>
> If exclusion were done independently (e.g. in a way similar to what
> you did in this series using subpath match), we wouldn't have had
> the "the user must give exclude patterns first that would affect the
> next inclusion pattern, at which point the exclude patterns are
> cleared and the user needs to start over", which is an end-user
> experience that is clunky.
>
However, it is useful that exclude patterns only apply to specific
match parameters? That is the advantage of the other implementation.
I think I agree that it's not really worth the complexity, as it
requires a much more complex explanation of how the parameters
interact, and in general doesn't provide that much more
expressiveness, since at least for "git describe" by definition it
either finds the tag as a match or not. Sure you could say "include
all tags matching x but only if they don't match y" and include all
tags matching z even if they match y" using that mechanism, but I
think that makes the entire thing needlessly more complicated than "we
use a tag if it matches any match and doesn't match any exclude".
Thanks,
Jake
^ permalink raw reply
* Re: [PATCH v3 1/2] difftool: add a skeleton for the upcoming builtin
From: Brandon Williams @ 2017-01-18 22:38 UTC (permalink / raw)
To: Jeff King
Cc: Stefan Beller, Johannes Schindelin, Junio C Hamano,
git@vger.kernel.org, David Aguilar, Dennis Kaarsemaker
In-Reply-To: <20161206183527.t6rjkkff7fxv5i5c@sigill.intra.peff.net>
On 12/06, Jeff King wrote:
> On Tue, Dec 06, 2016 at 10:22:21AM -0800, Stefan Beller wrote:
>
> > >> Maybe even go a step further and say that the config code needs a context
> > >> "object".
> > >
> > > If I were writing git from scratch, I'd consider making a "struct
> > > repository" object. I'm not sure how painful it would be to retro-fit it
> > > at this point.
> >
> > Would it be possible to introduce "the repo" struct similar to "the index"
> > in cache.h?
> >
> > From a submodule perspective I would very much welcome this
> > object oriented approach to repositories.
>
> I think it may be more complicated, because there's some implicit global
> state in "the repo", like where files are relative to our cwd. All of
> those low-level functions would have to start caring about which repo
> we're talking about so they can prefix the appropriate working tree
> path, etc.
>
> For some operations that would be fine, but there are things that would
> subtly fail for submodules. I'm thinking we'd end up with some code
> state like:
>
> /* finding a repo does not modify global state; good */
> struct repository *repo = repo_discover(".");
>
> /* obvious repo-level operations like looking up refs can be done with
> * a repository object; good */
> repo_for_each_ref(repo, callback, NULL);
>
> /*
> * "enter" the repo so that we are at the top-level of the working
> * tree, etc. After this you can actually look at the index without
> * things breaking.
> */
> repo_enter(repo);
>
> That would be enough to implement a lot of submodule-level stuff, but it
> would break pretty subtly as soon as you asked the submodule about its
> working tree. The solution is to make everything that accesses the
> working tree aware of the idea of a working tree root besides the cwd.
> But that's a pretty invasive change.
>
> -Peff
Some other challenges would be how to address people setting environment
variables like GIT_DIR that indicate the location of a repositories git
directory, which wouldn't work if you have multiple repos open.
I do agree that having a repo object of some sort would aid in
simplifying submodule operations but may require too many invasive
changes to basic low-level functions.
--
Brandon Williams
^ permalink raw reply
* [PATCH] attr: mark a file-local symbol as static
From: Ramsay Jones @ 2017-01-18 22:27 UTC (permalink / raw)
To: Brandon Williams; +Cc: Junio C Hamano, GIT Mailing-list
Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
---
Hi Brandon,
If you need to re-roll your 'bw/attr' branch, could you please
squash this into the relevant patch (commit 8908457159,
"attr: use hashmap for attribute dictionary", 12-01-2017).
Also, I note that, although they are declared as part of the
public attr api, attr_check_clear() and attr_check_reset() are
also not called outside of attr.c. Are these functions part of
the public api?
Also, a minor point, but attr_check_reset() is called (line 1050)
before it's definition (line 1114). This is not a problem, given
the declaration in attr.h, but I prefer definitions to come before
use, where possible.
Thanks!
ATB,
Ramsay Jones
attr.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/attr.c b/attr.c
index f5cc68b67..e68c4688f 100644
--- a/attr.c
+++ b/attr.c
@@ -83,7 +83,7 @@ static int attr_hash_entry_cmp(const struct attr_hash_entry *a,
}
/* Initialize an 'attr_hashmap' object */
-void attr_hashmap_init(struct attr_hashmap *map)
+static void attr_hashmap_init(struct attr_hashmap *map)
{
hashmap_init(&map->map, (hashmap_cmp_fn) attr_hash_entry_cmp, 0);
}
--
2.11.0
^ permalink raw reply related
* Re: [PATCH 3/4] document add_[file_]to_index
From: Stefan Beller @ 2017-01-18 22:09 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git@vger.kernel.org
In-Reply-To: <xmqqbmv43vx9.fsf@gitster.mtv.corp.google.com>
On Wed, Jan 18, 2017 at 1:22 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 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?
Well I got confused for pretend and intent, so I researched them;
I did not want to comment only half the constants.
>
>> +/*
>> + * 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.
That sounds better than what I had.
Thanks,
Stefan
>
>> 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
* Re: [PATCH 2/4] remove_index_entry_at: move documentation to cache.h
From: Stefan Beller @ 2017-01-18 22:06 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git@vger.kernel.org
In-Reply-To: <xmqqfukg3w7t.fsf@gitster.mtv.corp.google.com>
On Wed, Jan 18, 2017 at 1:16 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 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.
>
Actually my line of thinking was to improve the correctness by being more
specific.
In a reroll I move the comment verbatim.
^ permalink raw reply
* Re: [PATCH 1/4] document index_name_pos
From: Stefan Beller @ 2017-01-18 22:02 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git@vger.kernel.org
In-Reply-To: <xmqqk29s3wg0.fsf@gitster.mtv.corp.google.com>
On Wed, Jan 18, 2017 at 1:11 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 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
>> + */
Oh, I see. With this property in mind, we know that
when using index_name_pos for sorting, the stages for a
given path are ordered correctly (in ascending order,
0 comes before 1, which comes before 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?
yes, it should be -4.
^ 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 21:56 UTC (permalink / raw)
To: Jacob Keller
Cc: Jacob Keller, Git mailing list, Johannes Sixt,
Johannes Schindelin
In-Reply-To: <CA+P7+xo4-45je995LLoyh-LbGTTf3EZUVW-UV+Dd=Wg0EGRvVA@mail.gmail.com>
Jacob Keller <jacob.keller@gmail.com> writes:
> Yes this makes sense. I'm still looking at whether the alternative
> implementation suggested based on the git-log style would make more
> sense or not, but if we keep this as is, the added text you gave is
> important.
I actually think it is a red-herring that "git log" honors "orders";
it does, but that is not a result of carefully considering the
desired behaviour. It instead is a historical wart that came from
the fact that "--branches" and friends uses for_each_glob_ref_in()
that takes the top-level hierarchy paths like "refs/heads/" and the
implementation of "--exclude" piggybacked into the function in a
lazy way.
If exclusion were done independently (e.g. in a way similar to what
you did in this series using subpath match), we wouldn't have had
the "the user must give exclude patterns first that would affect the
next inclusion pattern, at which point the exclude patterns are
cleared and the user needs to start over", which is an end-user
experience that is clunky.
^ permalink raw reply
* Re: [PATCH v3 1/5] doc: add documentation for OPT_STRING_LIST
From: Junio C Hamano @ 2017-01-18 20:58 UTC (permalink / raw)
To: Philip Oakley
Cc: Jacob Keller, git, Johannes Sixt, Johannes Schindelin,
Jacob Keller
In-Reply-To: <254445096AD0412287CBB994E8BCA043@PhilipOakley>
"Philip Oakley" <philipoakley@iee.org> writes:
>>> +`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.
Yeah, I bikeshedded that myself, and rejected it because there is no
<option> mentioned anywhere in the enumeration header.
^ permalink raw reply
* Re: "git diff --ignore-space-change --stat" lists files with only whitespace differences as "changed"
From: Junio C Hamano @ 2017-01-18 20:57 UTC (permalink / raw)
To: Jeff King; +Cc: Matt McCutchen, git
In-Reply-To: <20170118111705.6bqzkklluikda3r5@sigill.intra.peff.net>
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.
> + /*
> + * Omit diffstats where nothing changed. Even if
> + * !same_contents, this might be the case due to ignoring
> + * whitespace changes, etc.
> + *
> + * But note that we special-case additions and deletions,
> + * as adding an empty file, for example, is still of interest.
> + */
> + if (DIFF_FILE_VALID(one) && DIFF_FILE_VALID(two)) {
> + struct diffstat_file *file =
> + diffstat->files[diffstat->nr - 1];
> + if (!file->added && !file->deleted) {
> + free_diffstat_file(file);
> + diffstat->nr--;
> + }
> + }
> }
^ permalink raw reply
* Re: git fast-import crashing on big imports
From: Jeff King @ 2017-01-18 21:51 UTC (permalink / raw)
To: Ulrich Spörlein; +Cc: Ed Maste, git, Junio C Hamano
In-Reply-To: <20170118202704.w6pjxfvnge7utk34@sigill.intra.peff.net>
On Wed, Jan 18, 2017 at 03:27:04PM -0500, Jeff King wrote:
> 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.
Hmm. I haven't reproduced your exact issue, but I was able to produce
some hijinks in that function.
The problem is that the hashmap_iter interface is unreliable if entries
are added or removed from the map during the iteration.
I suspect the patch below may fix things for you. It works around it by
walking over the lru list (either is fine, as they both contain all
entries, and since we're clearing everything, we don't care about the
order).
---
sha1_file.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/sha1_file.c b/sha1_file.c
index 1eb47f611..d20714d6b 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -2342,11 +2342,10 @@ static inline void release_delta_base_cache(struct delta_base_cache_entry *ent)
void clear_delta_base_cache(void)
{
- struct hashmap_iter iter;
- struct delta_base_cache_entry *entry;
- for (entry = hashmap_iter_first(&delta_base_cache, &iter);
- entry;
- entry = hashmap_iter_next(&iter)) {
+ struct list_head *lru, *tmp;
+ list_for_each_safe(lru, tmp, &delta_base_cache_lru) {
+ struct delta_base_cache_entry *entry =
+ list_entry(lru, struct delta_base_cache_entry, lru);
release_delta_base_cache(entry);
}
}
--
2.11.0.698.gd6b48ab4c
>
> -Peff
^ permalink raw reply related
* Re: [PATCH v3 3/5] name-rev: add support to exclude refs by pattern match
From: Jacob Keller @ 2017-01-18 21:13 UTC (permalink / raw)
To: Junio C Hamano
Cc: Jacob Keller, Git mailing list, Johannes Sixt,
Johannes Schindelin
In-Reply-To: <xmqqtw8w86xc.fsf@gitster.mtv.corp.google.com>
On Wed, Jan 18, 2017 at 12:11 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 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"?
>
Yes this makes sense. I'm still looking at whether the alternative
implementation suggested based on the git-log style would make more
sense or not, but if we keep this as is, the added text you gave is
important.
Thanks,
Jake
^ permalink raw reply
* Re: [PATCH] mingw: follow-up to "replace isatty() hack"
From: Junio C Hamano @ 2017-01-18 21:30 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Johannes Schindelin, git, Pranit Bauva, Beat Bolli
In-Reply-To: <c329c981-2b92-13b5-0561-d1d2e3fa2803@kdbg.org>
Johannes Sixt <j6t@kdbg.org> writes:
>>> - 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.
I think a Reviewed-by is good enough, as the original
<2ce6060b891f8313cc63a95a9cba9064b7f82eb8.1482448531.git.johannes.schindelin@gmx.de>
already has "Tested-by" to indicate that as a whole this have been
tested. The "follow-up" we are commenting on was made out of that
original to incrementally update the older version that was queued
and merged to 'master' 3 weeks ago.
Thanks.
^ permalink raw reply
* Re: [PATCH 4/4] documentation: retire unfinished documentation
From: Junio C Hamano @ 2017-01-18 21:23 UTC (permalink / raw)
To: Stefan Beller; +Cc: git
In-Reply-To: <20170117233503.27137-5-sbeller@google.com>
Stefan Beller <sbeller@google.com> writes:
> When looking for documentation for a specific function, you may be tempted
> to run
>
> git -C Documentation grep index_name_pos
>
> only to find the file technical/api-in-core-index.txt, which doesn't
> help for understanding the given function. It would be better to not find
> these functions in the documentation, such that people directly dive into
> the code instead.
>
> In the previous patches we have documented
> * index_name_pos()
> * remove_index_entry_at()
> * add_[file_]to_index()
> in cache.h
>
> We already have documentation for:
> * add_index_entry()
> * read_index()
>
> Which leaves us with a TODO for:
> * cache -> the_index macros
> * refresh_index()
> * discard_index()
> * ie_match_stat() and ie_modified(); how they are different and when to
> use which.
> * write_index() that was renamed to write_locked_index
> * cache_tree_invalidate_path()
> * cache_tree_update()
Thanks.
^ 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
* 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 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 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] 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 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 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 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: "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 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: [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
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