* 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 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] 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 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: 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: "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: [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: [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 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 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 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
* [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 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
* 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 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] attr: mark a file-local symbol as static
From: Brandon Williams @ 2017-01-18 23:05 UTC (permalink / raw)
To: Ramsay Jones; +Cc: Junio C Hamano, GIT Mailing-list
In-Reply-To: <89290015-7c5f-1a5d-e683-59077ae55bf5@ramsayjones.plus.com>
On 01/18, Ramsay Jones wrote:
>
> 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
Yes of course, I believe Stefan also pointed that out earlier today so I
have it fixed locally.
For attr_check_clear() and attr_check_reset() the intent is that they
are the accepted way to either clear or reset the attr_check structure.
Currently most users of the attribute system don't have a need to clear
or reset the structure but there could be future callers who need that
functionality. If you feel like they shouldn't be part of the api right
now then I'm open to changing that for this series.
Thanks!
--
Brandon Williams
^ permalink raw reply
* [PATCH v4 5/5] describe: teach describe negative pattern matches
From: Jacob Keller @ 2017-01-18 23:06 UTC (permalink / raw)
To: git; +Cc: Johannes Sixt, Johannes Schindelin, Junio C Hamano, Jacob Keller
In-Reply-To: <20170118230608.28030-1-jacob.e.keller@intel.com>
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 release tag that introduces the commit
abcdef:
git describe --contains --match="v*" --exclude="*rc*" abcdef
Add documentation, tests, and completion for this change.
Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
---
Documentation/git-describe.txt | 10 ++++++++++
builtin/describe.c | 21 +++++++++++++++++++++
contrib/completion/git-completion.bash | 1 +
t/t6120-describe.sh | 8 ++++++++
4 files changed, 40 insertions(+)
diff --git a/Documentation/git-describe.txt b/Documentation/git-describe.txt
index 7ad41e2f6ade..8755f3af7bcd 100644
--- a/Documentation/git-describe.txt
+++ b/Documentation/git-describe.txt
@@ -88,6 +88,16 @@ 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. When combined with --match a tag will
+ be considered when it matches at least one --match pattern and does not
+ match any of the --exclude patterns. Use `--no-exclude` to clear and
+ reset the list of patterns.
+
--always::
Show uniquely abbreviated commit object as fallback.
diff --git a/builtin/describe.c b/builtin/describe.c
index 5cc9e9abe798..6769446e1f57 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -29,6 +29,7 @@ static int max_candidates = 10;
static struct hashmap names;
static int have_util;
static struct string_list patterns = STRING_LIST_INIT_NODUP;
+static struct string_list exclude_patterns = STRING_LIST_INIT_NODUP;
static int always;
static const char *dirty;
@@ -129,6 +130,22 @@ static int get_name(const char *path, const struct object_id *oid, int flag, voi
if (!all && !is_tag)
return 0;
+ /*
+ * If we're given exclude patterns, first exclude any tag which match
+ * any of the exclude pattern.
+ */
+ if (exclude_patterns.nr) {
+ struct string_list_item *item;
+
+ if (!is_tag)
+ return 0;
+
+ for_each_string_list_item(item, &exclude_patterns) {
+ if (!wildmatch(item->string, path + 10, 0, NULL))
+ return 0;
+ }
+ }
+
/*
* If we're given patterns, accept only tags which match at least one
* pattern.
@@ -421,6 +438,8 @@ int cmd_describe(int argc, const char **argv, const char *prefix)
N_("consider <n> most recent tags (default: 10)")),
OPT_STRING_LIST(0, "match", &patterns, N_("pattern"),
N_("only consider tags matching <pattern>")),
+ OPT_STRING_LIST(0, "exclude", &exclude_patterns, N_("pattern"),
+ N_("do not consider tags matching <pattern>")),
OPT_BOOL(0, "always", &always,
N_("show abbreviated commit object as fallback")),
{OPTION_STRING, 0, "dirty", &dirty, N_("mark"),
@@ -458,6 +477,8 @@ int cmd_describe(int argc, const char **argv, const char *prefix)
argv_array_push(&args, "--tags");
for_each_string_list_item(item, &patterns)
argv_array_pushf(&args, "--refs=refs/tags/%s", item->string);
+ for_each_string_list_item(item, &exclude_patterns)
+ argv_array_pushf(&args, "--exclude=refs/tags/%s", item->string);
}
if (argc)
argv_array_pushv(&args, argv);
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 6721ff80fb13..835d7fcfd4f2 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1198,6 +1198,7 @@ _git_describe ()
__gitcomp "
--all --tags --contains --abbrev= --candidates=
--exact-match --debug --long --match --always
+ --exclude
"
return
esac
diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh
index 9e5db9b87a1f..167491fd5b0d 100755
--- a/t/t6120-describe.sh
+++ b/t/t6120-describe.sh
@@ -218,6 +218,14 @@ test_expect_success 'describe --contains and --match' '
test_cmp expect actual
'
+test_expect_success 'describe --exclude' '
+ echo "c~1" >expect &&
+ tagged_commit=$(git rev-parse "refs/tags/A^0") &&
+ test_must_fail git describe --contains --match="B" $tagged_commit &&
+ git describe --contains --match="?" --exclude="A" $tagged_commit >actual &&
+ test_cmp expect actual
+'
+
test_expect_success 'describe --contains and --no-match' '
echo "A^0" >expect &&
tagged_commit=$(git rev-parse "refs/tags/A^0") &&
--
2.11.0.488.g1cece4bcb7a5
^ permalink raw reply related
* [PATCH v4 4/5] describe: teach --match to accept multiple patterns
From: Jacob Keller @ 2017-01-18 23:06 UTC (permalink / raw)
To: git; +Cc: Johannes Sixt, Johannes Schindelin, Junio C Hamano, Jacob Keller
In-Reply-To: <20170118230608.28030-1-jacob.e.keller@intel.com>
From: Jacob Keller <jacob.keller@gmail.com>
Teach `--match` to be accepted multiple times, accumulating a list of
patterns to match into a string list. Each pattern is inclusive, such
that a tag need only match one of the provided patterns to be
considered for matching.
This extension is useful as it enables more flexibility in what tags
match, and may avoid the need to run the describe command multiple
times to get the same result.
Add tests and update the documentation for this change.
Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
---
Documentation/git-describe.txt | 5 ++++-
builtin/describe.c | 30 +++++++++++++++++++++++-------
t/t6120-describe.sh | 19 +++++++++++++++++++
3 files changed, 46 insertions(+), 8 deletions(-)
diff --git a/Documentation/git-describe.txt b/Documentation/git-describe.txt
index e4ac448ff565..7ad41e2f6ade 100644
--- a/Documentation/git-describe.txt
+++ b/Documentation/git-describe.txt
@@ -83,7 +83,10 @@ OPTIONS
--match <pattern>::
Only consider tags matching the given `glob(7)` pattern,
excluding the "refs/tags/" prefix. This can be used to avoid
- leaking private tags from the repository.
+ leaking private tags from the repository. If given multiple times, a
+ list of patterns will be accumulated, and tags matching any of the
+ patterns will be considered. Use `--no-match` to clear and reset the
+ list of patterns.
--always::
Show uniquely abbreviated commit object as fallback.
diff --git a/builtin/describe.c b/builtin/describe.c
index 01490a157efc..5cc9e9abe798 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -28,7 +28,7 @@ static int abbrev = -1; /* unspecified */
static int max_candidates = 10;
static struct hashmap names;
static int have_util;
-static const char *pattern;
+static struct string_list patterns = STRING_LIST_INIT_NODUP;
static int always;
static const char *dirty;
@@ -129,9 +129,24 @@ static int get_name(const char *path, const struct object_id *oid, int flag, voi
if (!all && !is_tag)
return 0;
- /* Accept only tags that match the pattern, if given */
- if (pattern && (!is_tag || wildmatch(pattern, path + 10, 0, NULL)))
- return 0;
+ /*
+ * If we're given patterns, accept only tags which match at least one
+ * pattern.
+ */
+ if (patterns.nr) {
+ struct string_list_item *item;
+
+ if (!is_tag)
+ return 0;
+
+ for_each_string_list_item(item, &patterns) {
+ if (!wildmatch(item->string, path + 10, 0, NULL))
+ break;
+
+ /* If we get here, no pattern matched. */
+ return 0;
+ }
+ }
/* Is it annotated? */
if (!peel_ref(path, peeled.hash)) {
@@ -404,7 +419,7 @@ int cmd_describe(int argc, const char **argv, const char *prefix)
N_("only output exact matches"), 0),
OPT_INTEGER(0, "candidates", &max_candidates,
N_("consider <n> most recent tags (default: 10)")),
- OPT_STRING(0, "match", &pattern, N_("pattern"),
+ OPT_STRING_LIST(0, "match", &patterns, N_("pattern"),
N_("only consider tags matching <pattern>")),
OPT_BOOL(0, "always", &always,
N_("show abbreviated commit object as fallback")),
@@ -430,6 +445,7 @@ int cmd_describe(int argc, const char **argv, const char *prefix)
die(_("--long is incompatible with --abbrev=0"));
if (contains) {
+ struct string_list_item *item;
struct argv_array args;
argv_array_init(&args);
@@ -440,8 +456,8 @@ int cmd_describe(int argc, const char **argv, const char *prefix)
argv_array_push(&args, "--always");
if (!all) {
argv_array_push(&args, "--tags");
- if (pattern)
- argv_array_pushf(&args, "--refs=refs/tags/%s", pattern);
+ for_each_string_list_item(item, &patterns)
+ argv_array_pushf(&args, "--refs=refs/tags/%s", item->string);
}
if (argc)
argv_array_pushv(&args, argv);
diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh
index 85f269411cb3..9e5db9b87a1f 100755
--- a/t/t6120-describe.sh
+++ b/t/t6120-describe.sh
@@ -182,6 +182,10 @@ check_describe "test2-lightweight-*" --tags --match="test2-*"
check_describe "test2-lightweight-*" --long --tags --match="test2-*" HEAD^
+check_describe "test1-lightweight-*" --long --tags --match="test1-*" --match="test2-*" HEAD^
+
+check_describe "test2-lightweight-*" --long --tags --match="test1-*" --no-match --match="test2-*" HEAD^
+
test_expect_success 'name-rev with exact tags' '
echo A >expect &&
tag_object=$(git rev-parse refs/tags/A) &&
@@ -206,4 +210,19 @@ test_expect_success 'describe --contains with the exact tags' '
test_cmp expect actual
'
+test_expect_success 'describe --contains and --match' '
+ echo "A^0" >expect &&
+ tagged_commit=$(git rev-parse "refs/tags/A^0") &&
+ test_must_fail git describe --contains --match="B" $tagged_commit &&
+ git describe --contains --match="B" --match="A" $tagged_commit >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'describe --contains and --no-match' '
+ echo "A^0" >expect &&
+ tagged_commit=$(git rev-parse "refs/tags/A^0") &&
+ git describe --contains --match="B" --no-match $tagged_commit >actual &&
+ test_cmp expect actual
+'
+
test_done
--
2.11.0.488.g1cece4bcb7a5
^ permalink raw reply related
* [PATCH v4 0/5] extend git-describe pattern matching
From: Jacob Keller @ 2017-01-18 23:06 UTC (permalink / raw)
To: git; +Cc: Johannes Sixt, Johannes Schindelin, Junio C Hamano, Jacob Keller
From: Jacob Keller <jacob.keller@gmail.com>
Teach git describe and git name-rev the ability to match multiple
patterns inclusively. Additionally, teach these commands to also accept
negative patterns to exclude any refs which match.
The pattern lists for positive and negative patterns are inclusive. This
means that for the positive patterns, a reference will be considered as
long as it matches at least one of the match patterns. It need not match
all given patterns. Additionally for negative patterns, we will not
consider any ref which matches any negative pattern, even if it matches
one of the positive patterns.
Together this allows the ability to express far more sets of tags than a
single match pattern alone. It does not provide quite the same depth as
would teaching full regexp but it is simpler and easy enough to
understand.
- v2
* use --exclude instead of --discard
* use modern style in tests
- v3
* fix broken test (sorry for the thrash!)
- v4
* update documentation and commit messages at Junio's suggestion
* add completion
After even more thought, I do not like the way that git-log works with
--exclude, and do not believe it gives enough extra expressive power to
bother with the complexity. The current implementation of --match and
--exclude is relatively easy to explain and makes some sense. Since we
did not have a historical reasoning in the same way that git-log does, I
do not think using something like an exclude_list or similar is worth
the added complexity.
-- interdiff since v3 --
diff --git c/Documentation/git-describe.txt w/Documentation/git-describe.txt
index 21a43b78924a..8755f3af7bcd 100644
--- c/Documentation/git-describe.txt
+++ w/Documentation/git-describe.txt
@@ -93,7 +93,9 @@ OPTIONS
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
+ of the patterns will be excluded. When combined with --match a tag will
+ be considered when it matches at least one --match pattern and does not
+ match any of the --exclude patterns. Use `--no-exclude` to clear and
reset the list of patterns.
--always::
diff --git c/Documentation/git-name-rev.txt w/Documentation/git-name-rev.txt
index 301b4a8d55e6..da83f280ed88 100644
--- c/Documentation/git-name-rev.txt
+++ w/Documentation/git-name-rev.txt
@@ -33,9 +33,11 @@ OPTIONS
--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.
+ name. If given multiple times, a ref will be excluded when it matches
+ any of the given patterns. When used together with --refs, a ref will
+ be used as a match only when it matches at least one --ref pattern and
+ does not match any --exclude patterns. Use `--no-exclude` to clear the
+ list of exclude patterns.
--all::
List all commits reachable from all refs
diff --git c/Documentation/technical/api-parse-options.txt w/Documentation/technical/api-parse-options.txt
index 6914f54f5f44..36768b479e16 100644
--- c/Documentation/technical/api-parse-options.txt
+++ w/Documentation/technical/api-parse-options.txt
@@ -168,10 +168,10 @@ 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)`::
+`OPT_STRING_LIST(short, long, &struct string_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`.
+ The string argument is stored as an element in `string_list`.
+ Use of `--no-option` will clear the list of preceding values.
`OPT_INTEGER(short, long, &int_var, description)`::
Introduce an option with integer argument.
diff --git c/builtin/name-rev.c w/builtin/name-rev.c
index da4a0d7c0fdf..ea8ef48102f8 100644
--- c/builtin/name-rev.c
+++ w/builtin/name-rev.c
@@ -166,10 +166,11 @@ static int name_ref(const char *path, const struct object_id *oid, int flags, vo
/* 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.
+ /* Check every pattern even after we found a match so
+ * that we can determine when we should abbreviate the
+ * output. We will abbreviate the output when any of
+ * the patterns match a subpath, even if one of the
+ * patterns matches fully.
*/
switch (subpath_matches(path, item->string)) {
case -1: /* did not match */
diff --git c/contrib/completion/git-completion.bash w/contrib/completion/git-completion.bash
index 6721ff80fb13..835d7fcfd4f2 100644
--- c/contrib/completion/git-completion.bash
+++ w/contrib/completion/git-completion.bash
@@ -1198,6 +1198,7 @@ _git_describe ()
__gitcomp "
--all --tags --contains --abbrev= --candidates=
--exact-match --debug --long --match --always
+ --exclude
"
return
esac
Jacob Keller (5):
doc: add documentation for OPT_STRING_LIST
name-rev: extend --refs to accept multiple patterns
name-rev: add support to exclude refs by pattern match
describe: teach --match to accept multiple patterns
describe: teach describe negative pattern matches
Documentation/git-describe.txt | 15 +++++++-
Documentation/git-name-rev.txt | 13 ++++++-
Documentation/technical/api-parse-options.txt | 5 +++
builtin/describe.c | 51 +++++++++++++++++++++----
builtin/name-rev.c | 54 +++++++++++++++++++++------
contrib/completion/git-completion.bash | 1 +
t/t6007-rev-list-cherry-pick-file.sh | 38 +++++++++++++++++++
t/t6120-describe.sh | 27 ++++++++++++++
8 files changed, 183 insertions(+), 21 deletions(-)
--
2.11.0.488.g1cece4bcb7a5
^ permalink raw reply related
* [PATCH v4 3/5] name-rev: add support to exclude refs by pattern match
From: Jacob Keller @ 2017-01-18 23:06 UTC (permalink / raw)
To: git; +Cc: Johannes Sixt, Johannes Schindelin, Junio C Hamano, Jacob Keller
In-Reply-To: <20170118230608.28030-1-jacob.e.keller@intel.com>
From: Jacob Keller <jacob.keller@gmail.com>
Extend git-name-rev to support excluding refs which match shell patterns
using --exclude. These patterns can be used to limit the scope of refs
by excluding any ref that matches one of the --exclude patterns. A ref
will only be used for naming when it matches at least one --ref pattern
but does not match any of the --exclude patterns. Thus, --exlude
patterns are given precedence over --ref patterns.
For example, suppose you wish to name a series of commits based on an
official release tag of the form "v*" but excluding any pre-release tags
which match "*rc*". You can use the following to do so:
git name-rev --refs="v*" --exclude="*rc*" --all
Add tests and update Documentation for this change.
Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
---
Documentation/git-name-rev.txt | 9 +++++++++
builtin/name-rev.c | 14 +++++++++++++-
t/t6007-rev-list-cherry-pick-file.sh | 12 ++++++++++++
3 files changed, 34 insertions(+), 1 deletion(-)
diff --git a/Documentation/git-name-rev.txt b/Documentation/git-name-rev.txt
index 7433627db12d..da83f280ed88 100644
--- a/Documentation/git-name-rev.txt
+++ b/Documentation/git-name-rev.txt
@@ -30,6 +30,15 @@ OPTIONS
given multiple times, use refs whose names match any of the given shell
patterns. Use `--no-refs` to clear any previous ref patterns given.
+--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, a ref will be excluded when it matches
+ any of the given patterns. When used together with --refs, a ref will
+ be used as a match only when it matches at least one --ref pattern and
+ does not match any --exclude patterns. Use `--no-exclude` to clear the
+ list of exclude patterns.
+
--all::
List all commits reachable from all refs
diff --git a/builtin/name-rev.c b/builtin/name-rev.c
index 85897ea1f714..ea8ef48102f8 100644
--- a/builtin/name-rev.c
+++ b/builtin/name-rev.c
@@ -109,6 +109,7 @@ struct name_ref_data {
int tags_only;
int name_only;
struct string_list ref_filters;
+ struct string_list exclude_filters;
};
static struct tip_table {
@@ -150,6 +151,15 @@ static int name_ref(const char *path, const struct object_id *oid, int flags, vo
if (data->tags_only && !starts_with(path, "refs/tags/"))
return 0;
+ if (data->exclude_filters.nr) {
+ struct string_list_item *item;
+
+ for_each_string_list_item(item, &data->exclude_filters) {
+ if (subpath_matches(path, item->string) >= 0)
+ return 0;
+ }
+ }
+
if (data->ref_filters.nr) {
struct string_list_item *item;
int matched = 0;
@@ -324,12 +334,14 @@ int cmd_name_rev(int argc, const char **argv, const char *prefix)
{
struct object_array revs = OBJECT_ARRAY_INIT;
int all = 0, transform_stdin = 0, allow_undefined = 1, always = 0, peel_tag = 0;
- struct name_ref_data data = { 0, 0, STRING_LIST_INIT_NODUP };
+ struct name_ref_data data = { 0, 0, STRING_LIST_INIT_NODUP, STRING_LIST_INIT_NODUP };
struct option opts[] = {
OPT_BOOL(0, "name-only", &data.name_only, N_("print only names (no SHA-1)")),
OPT_BOOL(0, "tags", &data.tags_only, N_("only use tags to name the commits")),
OPT_STRING_LIST(0, "refs", &data.ref_filters, N_("pattern"),
N_("only use refs matching <pattern>")),
+ OPT_STRING_LIST(0, "exclude", &data.exclude_filters, N_("pattern"),
+ N_("ignore refs matching <pattern>")),
OPT_GROUP(""),
OPT_BOOL(0, "all", &all, N_("list all commits reachable from all refs")),
OPT_BOOL(0, "stdin", &transform_stdin, N_("read from stdin")),
diff --git a/t/t6007-rev-list-cherry-pick-file.sh b/t/t6007-rev-list-cherry-pick-file.sh
index d9827a6389a3..29597451967d 100755
--- a/t/t6007-rev-list-cherry-pick-file.sh
+++ b/t/t6007-rev-list-cherry-pick-file.sh
@@ -118,6 +118,18 @@ test_expect_success 'name-rev --refs excludes non-matched patterns' '
test_cmp actual.named expect
'
+cat >expect <<EOF
+<tags/F
+EOF
+
+test_expect_success 'name-rev --exclude excludes matched patterns' '
+ git rev-list --left-right --right-only --cherry-pick F...E -- bar >>expect &&
+ git rev-list --left-right --cherry-pick F...E -- bar >actual &&
+ git name-rev --stdin --name-only --refs="*tags/*" --exclude="*E" \
+ <actual >actual.named &&
+ test_cmp actual.named expect
+'
+
test_expect_success 'name-rev --no-refs clears the refs list' '
git rev-list --left-right --cherry-pick F...E -- bar >expect &&
git name-rev --stdin --name-only --refs="*tags/F" --refs="*tags/E" --no-refs --refs="*tags/G" \
--
2.11.0.488.g1cece4bcb7a5
^ permalink raw reply related
* [PATCH v4 2/5] name-rev: extend --refs to accept multiple patterns
From: Jacob Keller @ 2017-01-18 23:06 UTC (permalink / raw)
To: git; +Cc: Johannes Sixt, Johannes Schindelin, Junio C Hamano, Jacob Keller
In-Reply-To: <20170118230608.28030-1-jacob.e.keller@intel.com>
From: Jacob Keller <jacob.keller@gmail.com>
Teach git name-rev to take multiple --refs stored as a string list of
patterns. The list of patterns will be matched inclusively, and each ref
only needs to match one pattern to be included. A ref will only be
excluded if it does not match any of the given patterns. Additionally,
if any of the patterns would allow abbreviation, then we will abbreviate
the ref, even if another pattern is more strict and would not have
allowed abbreviation on its own.
Add tests and documentation for this change. The tests expected output
is dynamically generated, but this is in order to avoid hard-coding
a commit object id in the test results (as the expected output is to
simply leave the commit object unnamed).
Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
---
Documentation/git-name-rev.txt | 4 +++-
builtin/name-rev.c | 42 +++++++++++++++++++++++++-----------
t/t6007-rev-list-cherry-pick-file.sh | 26 ++++++++++++++++++++++
3 files changed, 59 insertions(+), 13 deletions(-)
diff --git a/Documentation/git-name-rev.txt b/Documentation/git-name-rev.txt
index ca28fb8e2a07..7433627db12d 100644
--- a/Documentation/git-name-rev.txt
+++ b/Documentation/git-name-rev.txt
@@ -26,7 +26,9 @@ OPTIONS
--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.
--all::
List all commits reachable from all refs
diff --git a/builtin/name-rev.c b/builtin/name-rev.c
index cd89d48b65e8..85897ea1f714 100644
--- a/builtin/name-rev.c
+++ b/builtin/name-rev.c
@@ -108,7 +108,7 @@ static const char *name_ref_abbrev(const char *refname, int shorten_unambiguous)
struct name_ref_data {
int tags_only;
int name_only;
- const char *ref_filter;
+ struct string_list ref_filters;
};
static struct tip_table {
@@ -150,16 +150,34 @@ static int name_ref(const char *path, const struct object_id *oid, int flags, vo
if (data->tags_only && !starts_with(path, "refs/tags/"))
return 0;
- if (data->ref_filter) {
- switch (subpath_matches(path, data->ref_filter)) {
- case -1: /* did not match */
- return 0;
- case 0: /* matched fully */
- break;
- default: /* matched subpath */
- can_abbreviate_output = 1;
- break;
+ 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) {
+ /* Check every pattern even after we found a match so
+ * that we can determine when we should abbreviate the
+ * output. We will abbreviate the output when any of
+ * the patterns match a subpath, even if one of the
+ * patterns matches fully.
+ */
+ 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;
+ }
}
+
+ /* If none of the patterns matched, stop now */
+ if (!matched)
+ return 0;
}
add_to_tip_table(oid->hash, path, can_abbreviate_output);
@@ -306,11 +324,11 @@ int cmd_name_rev(int argc, const char **argv, const char *prefix)
{
struct object_array revs = OBJECT_ARRAY_INIT;
int all = 0, transform_stdin = 0, allow_undefined = 1, always = 0, peel_tag = 0;
- struct name_ref_data data = { 0, 0, NULL };
+ struct name_ref_data data = { 0, 0, STRING_LIST_INIT_NODUP };
struct option opts[] = {
OPT_BOOL(0, "name-only", &data.name_only, N_("print only names (no SHA-1)")),
OPT_BOOL(0, "tags", &data.tags_only, N_("only use tags to name the commits")),
- OPT_STRING(0, "refs", &data.ref_filter, N_("pattern"),
+ OPT_STRING_LIST(0, "refs", &data.ref_filters, N_("pattern"),
N_("only use refs matching <pattern>")),
OPT_GROUP(""),
OPT_BOOL(0, "all", &all, N_("list all commits reachable from all refs")),
diff --git a/t/t6007-rev-list-cherry-pick-file.sh b/t/t6007-rev-list-cherry-pick-file.sh
index 1408b608eb03..d9827a6389a3 100755
--- a/t/t6007-rev-list-cherry-pick-file.sh
+++ b/t/t6007-rev-list-cherry-pick-file.sh
@@ -99,6 +99,32 @@ test_expect_success '--cherry-pick bar does not come up empty (II)' '
test_cmp actual.named expect
'
+test_expect_success 'name-rev multiple --refs combine inclusive' '
+ git rev-list --left-right --cherry-pick F...E -- bar >actual &&
+ git name-rev --stdin --name-only --refs="*tags/F" --refs="*tags/E" \
+ <actual >actual.named &&
+ test_cmp actual.named expect
+'
+
+cat >expect <<EOF
+<tags/F
+EOF
+
+test_expect_success 'name-rev --refs excludes non-matched patterns' '
+ git rev-list --left-right --right-only --cherry-pick F...E -- bar >>expect &&
+ git rev-list --left-right --cherry-pick F...E -- bar >actual &&
+ git name-rev --stdin --name-only --refs="*tags/F" \
+ <actual >actual.named &&
+ test_cmp actual.named expect
+'
+
+test_expect_success 'name-rev --no-refs clears the refs list' '
+ git rev-list --left-right --cherry-pick F...E -- bar >expect &&
+ git name-rev --stdin --name-only --refs="*tags/F" --refs="*tags/E" --no-refs --refs="*tags/G" \
+ <expect >actual &&
+ test_cmp actual expect
+'
+
cat >expect <<EOF
+tags/F
=tags/D
--
2.11.0.488.g1cece4bcb7a5
^ permalink raw reply related
* [PATCH v4 1/5] doc: add documentation for OPT_STRING_LIST
From: Jacob Keller @ 2017-01-18 23:06 UTC (permalink / raw)
To: git; +Cc: Johannes Sixt, Johannes Schindelin, Junio C Hamano, Jacob Keller
In-Reply-To: <20170118230608.28030-1-jacob.e.keller@intel.com>
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..36768b479e16 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, &struct string_list, arg_str, description)`::
+ Introduce an option with string argument.
+ The string argument is stored as an element in `string_list`.
+ Use of `--no-option` will clear the list of preceding values.
+
`OPT_INTEGER(short, long, &int_var, description)`::
Introduce an option with integer argument.
The integer is put into `int_var`.
--
2.11.0.488.g1cece4bcb7a5
^ permalink raw reply related
* [PATCHv2 2/4] cache.h: document remove_index_entry_at
From: Stefan Beller @ 2017-01-18 23:21 UTC (permalink / raw)
To: gitster; +Cc: git, Stefan Beller
In-Reply-To: <20170118232145.31606-1-sbeller@google.com>
Do this by moving the existing documentation from
read-cache.c to cache.h.
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 3dbba69aec..87eccdb211 100644
--- a/cache.h
+++ b/cache.h
@@ -603,7 +603,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 true if there are more entries to go. */
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
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];
--
2.11.0.299.g762782ba8a
^ permalink raw reply related
* [PATCHv2 0/4] start documenting core functions
From: Stefan Beller @ 2017-01-18 23:21 UTC (permalink / raw)
To: gitster; +Cc: git, Stefan Beller
In-Reply-To: <20170117233503.27137-1-sbeller@google.com>
v2:
included all suggestions from Junio
v1:
The two single patches[1] are turned into a series here.
[1] https://public-inbox.org/git/20170117200147.25425-1-sbeller@google.com/
Thanks,
Stefan
Stefan Beller (4):
cache.h: document index_name_pos
cache.h: document remove_index_entry_at
cache.h: document add_[file_]to_index
documentation: retire unfinished documentation
Documentation/technical/api-in-core-index.txt | 21 -------------
cache.h | 43 +++++++++++++++++++++++----
read-cache.c | 1 -
3 files changed, 38 insertions(+), 27 deletions(-)
delete mode 100644 Documentation/technical/api-in-core-index.txt
--
2.11.0.299.g762782ba8a
^ permalink raw reply
* [PATCHv2 3/4] cache.h: document add_[file_]to_index
From: Stefan Beller @ 2017-01-18 23:21 UTC (permalink / raw)
To: gitster; +Cc: git, Stefan Beller
In-Reply-To: <20170118232145.31606-1-sbeller@google.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
cache.h | 21 ++++++++++++++++-----
1 file changed, 16 insertions(+), 5 deletions(-)
diff --git a/cache.h b/cache.h
index 87eccdb211..03c46b9b99 100644
--- a/cache.h
+++ b/cache.h
@@ -609,13 +609,24 @@ 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 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.
+ */
extern int add_to_index(struct index_state *, const char *path, struct stat *, int flags);
extern int add_file_to_index(struct index_state *, const char *path, int flags);
+
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);
--
2.11.0.299.g762782ba8a
^ permalink raw reply related
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