Re: Git: new feature suggestion

[Konstantin Khomoutov]

On Wed, 18 Jan 2017 10:40:52 +
Joao Pinto  wrote:

[...]
> I have seen a lot of Linux developers avoid this re-organization
> operations because they would lose the renamed file history, because
> a new log is created for the new file, even if it is a renamed
> version of itself. I am sending you this e-mail to suggest the
> creation of a new feature in Git: when renamed, a file or folder
> should inherit his parent’s log and a “rename: …” would be
> automatically created or have some kind of pointer to its “old” form
> to make history analysis easier.

Git does not record renames because of its stance that what matters is
code _of the whole project_ as opposed to its location in a particular
file.

Hence with regard to renames Git "works backwards" by detecting them
dynamically while traversing the history (such as with `git log`
etc).  This detection uses certain heuristics which can be controlled
with knobs pointed to by Stefan Beller.

Still, I welcome you to read the sort-of "reference" post by Linus
Torvalds [1] in which he explains the reasoning behind this approach
implemented in Git.  IMO, understanding the reasoning behind the idea
is much better than just mechanically learning how to use it.

The whole thread (esp. Torvalds' replies) is worth reading, but that
particular mail summarizes the whole thing very well.

(The reference link to it used to be [2], but Gmane is not fully
recovered to be able to display it.)

1. http://public-inbox.org/git/pine.lnx.4.58.0504150753440.7...@ppc970.osdl.org/
2. http://thread.gmane.org/gmane.comp.version-control.git/27/focus=217

[PATCHv3 3/4] cache.h: document add_[file_]to_index

[Stefan Beller]

Helped-by: Junio C Hamano 
Signed-off-by: Stefan Beller 
---
 cache.h | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/cache.h b/cache.h
index 929474d7a9..12394eb541 100644
--- a/cache.h
+++ b/cache.h
@@ -614,8 +614,18 @@ extern int remove_file_from_index(struct index_state *, 
const char *path);
 #define ADD_CACHE_IGNORE_ERRORS4
 #define ADD_CACHE_IGNORE_REMOVAL 8
 #define ADD_CACHE_INTENT 16
+/*
+ * 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

[PATCHv3 4/4] documentation: retire unfinished documentation

[Stefan Beller]

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()

Signed-off-by: Stefan Beller 
---
 Documentation/technical/api-in-core-index.txt | 21 -
 1 file changed, 21 deletions(-)
 delete mode 100644 Documentation/technical/api-in-core-index.txt

diff --git a/Documentation/technical/api-in-core-index.txt 
b/Documentation/technical/api-in-core-index.txt
deleted file mode 100644
index adbdbf5d75..00
--- a/Documentation/technical/api-in-core-index.txt
+++ /dev/null
@@ -1,21 +0,0 @@
-in-core index API
-=
-
-Talk about  and , things like:
-
-* cache -> the_index macros
-* read_index()
-* write_index()
-* ie_match_stat() and ie_modified(); how they are different and when to
-  use which.
-* index_name_pos()
-* remove_index_entry_at()
-* remove_file_from_index()
-* add_file_to_index()
-* add_index_entry()
-* refresh_index()
-* discard_index()
-* cache_tree_invalidate_path()
-* cache_tree_update()
-
-(JC, Linus)
-- 
2.11.0.299.g762782ba8a

[PATCHv3 0/4] start documenting core functions

[Stefan Beller]

v3:
* dropped commets on constants as they were not helpful
* fixed one result in the example for  index_name_pos(, "f", 1) (which
  is -3 now)

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-sbel...@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

[PATCHv3 1/4] cache.h: document index_name_pos

[Stefan Beller]

Signed-off-by: Stefan Beller 
---
 cache.h | 19 +++
 1 file changed, 19 insertions(+)

diff --git a/cache.h b/cache.h
index 1b67f078dd..1469ddeafe 100644
--- a/cache.h
+++ b/cache.h
@@ -575,7 +575,26 @@ 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: The current index consists of these files and its stages:
+ *
+ *   b#0, d#0, f#1, f#3
+ *
+ * index_name_pos(, "a", 1) -> -1
+ * index_name_pos(, "b", 1) ->  0
+ * index_name_pos(, "c", 1) -> -2
+ * index_name_pos(, "d", 1) ->  1
+ * index_name_pos(, "e", 1) -> -3
+ * index_name_pos(, "f", 1) -> -3
+ * index_name_pos(, "g", 1) -> -5
+ */
 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 */
-- 
2.11.0.299.g762782ba8a

[PATCHv3 2/4] cache.h: document remove_index_entry_at

[Stefan Beller]

Do this by moving the existing documentation from
read-cache.c to cache.h.

Signed-off-by: Stefan Beller 
---
 cache.h  | 3 +++
 read-cache.c | 1 -
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/cache.h b/cache.h
index 1469ddeafe..929474d7a9 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

Re: [PATCH] attr: mark a file-local symbol as static

[Ramsay Jones]

On 18/01/17 23:05, Brandon Williams wrote:
> On 01/18, Ramsay Jones wrote:
>>
>> Signed-off-by: Ramsay Jones 
>> ---
>>
>> 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.

Yep, I noticed Stefan's email just a few minutes after I hit
send! ;-)

> 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.

No, I just wanted to check that they were intended to be part of
the public api and that you anticipate additional callers in the
future.

[I would still prefer definitions before use, but many people would
not agree with me, so ...]

ATB,
Ramsay Jones

Re: [PATCH v6 4/6] builtin/tag: add --format argument for tag -v

[Eric Wong]

Eric Wong  wrote:
> Junio C Hamano  wrote:
> > 
> >   
> > http://public-inbox.org/git/20170118182831.pkhqu2np3bh2puei@LykOS.localdomain/
> 
> Eeep!  This looks like a regression I introduced when working
> around Richard Hansen's S/MIME mails the other week on git@vger:
>
>   https://public-inbox.org/meta/2017011035.GB27356@dcvr/T/#u

Yep, I copied SUPER and used it improperly in a subclass.  Should be
fixed now, and reimported several messages from my Maildir.  NNTP
readers will see new article numbers for reimported Message-IDs.

Re: [PATCHv2 3/4] cache.h: document add_[file_]to_index

[Stefan Beller]

On Wed, Jan 18, 2017 at 4:00 PM, Brandon Williams  wrote:
> On 01/18, Stefan Beller wrote:
>> Signed-off-by: Stefan Beller 
>> ---
>>  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 */
>
> I usually prefer having defines like these use shift operators to set
> the desired bit '(1<<2)' instead of '4', etc.  Is there a preference for
> git as a whole?  I know this is just a documentation change so maybe
> this isn't even the place to discuss this.

eh, and I forgot to remove the comments that Junio thought of as redundant.
I agree that (1<
> --
> Brandon Williams

Re: [PATCHv2 3/4] cache.h: document add_[file_]to_index

[Brandon Williams]

On 01/18, Stefan Beller wrote:
> Signed-off-by: Stefan Beller 
> ---
>  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 */

I usually prefer having defines like these use shift operators to set
the desired bit '(1<<2)' instead of '4', etc.  Is there a preference for
git as a whole?  I know this is just a documentation change so maybe
this isn't even the place to discuss this.

-- 
Brandon Williams

Re: "git diff --ignore-space-change --stat" lists files with only whitespace differences as "changed"

[Junio C Hamano]

Jeff King  writes:

> On Wed, Jan 18, 2017 at 12:57:12PM -0800, Junio C Hamano wrote:
>
>> Jeff King  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).

Doh (yes, "diff --git" was what I meant).  As a mode-flipping patch
does not have hunk but does show the header, it wants to be included
in "git diff --stat" output, I would think, independent of -b issue.
In fact

chmod +x README.md
git diff --stat

does show a 0-line diffstat.


> 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

[PATCHv2 1/4] cache.h: document index_name_pos

[Stefan Beller]

Signed-off-by: Stefan Beller 
---
 cache.h | 19 +++
 1 file changed, 19 insertions(+)

diff --git a/cache.h b/cache.h
index 1b67f078dd..3dbba69aec 100644
--- a/cache.h
+++ b/cache.h
@@ -575,7 +575,26 @@ 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: The current index consists of these files and its stages:
+ *
+ *   b#0, d#0, f#1, f#3
+ *
+ * index_name_pos(, "a", 1) -> -1
+ * index_name_pos(, "b", 1) ->  0
+ * index_name_pos(, "c", 1) -> -2
+ * index_name_pos(, "d", 1) ->  1
+ * index_name_pos(, "e", 1) -> -3
+ * index_name_pos(, "f", 1) ->  2
+ * index_name_pos(, "g", 1) -> -5
+ */
 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 */
-- 
2.11.0.299.g762782ba8a

[PATCHv2 4/4] documentation: retire unfinished documentation

[Stefan Beller]

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()

Signed-off-by: Stefan Beller 
---
 Documentation/technical/api-in-core-index.txt | 21 -
 1 file changed, 21 deletions(-)
 delete mode 100644 Documentation/technical/api-in-core-index.txt

diff --git a/Documentation/technical/api-in-core-index.txt 
b/Documentation/technical/api-in-core-index.txt
deleted file mode 100644
index adbdbf5d75..00
--- a/Documentation/technical/api-in-core-index.txt
+++ /dev/null
@@ -1,21 +0,0 @@
-in-core index API
-=
-
-Talk about  and , things like:
-
-* cache -> the_index macros
-* read_index()
-* write_index()
-* ie_match_stat() and ie_modified(); how they are different and when to
-  use which.
-* index_name_pos()
-* remove_index_entry_at()
-* remove_file_from_index()
-* add_file_to_index()
-* add_index_entry()
-* refresh_index()
-* discard_index()
-* cache_tree_invalidate_path()
-* cache_tree_update()
-
-(JC, Linus)
-- 
2.11.0.299.g762782ba8a

[PATCHv2 3/4] cache.h: document add_[file_]to_index

[Stefan Beller]

Signed-off-by: Stefan Beller 
---
 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_ERRORS4
-#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

[PATCHv2 0/4] start documenting core functions

[Stefan Beller]

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-sbel...@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

[PATCHv2 2/4] cache.h: document remove_index_entry_at

[Stefan Beller]

Do this by moving the existing documentation from
read-cache.c to cache.h.

Signed-off-by: Stefan Beller 
---
 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

[PATCH v4 3/5] name-rev: add support to exclude refs by pattern match

[Jacob Keller]

From: Jacob Keller 

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 
---
 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=::
+   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, >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", _only, N_("print only names 
(no SHA-1)")),
OPT_BOOL(0, "tags", _only, N_("only use tags to name 
the commits")),
OPT_STRING_LIST(0, "refs", _filters, N_("pattern"),
   N_("only use refs matching ")),
+   OPT_STRING_LIST(0, "exclude", _filters, 
N_("pattern"),
+  N_("ignore refs matching ")),
OPT_GROUP(""),
OPT_BOOL(0, "all", , N_("list all commits reachable from 
all refs")),
OPT_BOOL(0, "stdin", _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 <>expect &&
+   git rev-list --left-right --cherry-pick F...E -- bar >actual &&
+   git name-rev --stdin --name-only --refs="*tags/*" --exclude="*E" \
+   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

[PATCH v4 0/5] extend git-describe pattern matching

[Jacob Keller]

From: Jacob Keller 

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=::
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, , arg_str, description)`::
+`OPT_STRING_LIST(short, long,  string_list, arg_str, description)`::
Introduce an option with string argument.
-   The string argument is stored as an element in `` 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, _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, >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 

[PATCH v4 2/5] name-rev: extend --refs to accept multiple patterns

[Jacob Keller]

From: Jacob Keller 

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 
---
 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=::
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, >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", _only, N_("print only names 
(no SHA-1)")),
OPT_BOOL(0, "tags", _only, N_("only use tags to name 
the commits")),
-   OPT_STRING(0, "refs", _filter, N_("pattern"),
+   OPT_STRING_LIST(0, "refs", _filters, N_("pattern"),
   N_("only use refs matching ")),
OPT_GROUP(""),
OPT_BOOL(0, "all", , N_("list all commits reachable from 
all refs")),
diff --git 

[PATCH v4 1/5] doc: add documentation for OPT_STRING_LIST

[Jacob Keller]

From: Jacob Keller 

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 
---
 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,  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, _var, description)`::
Introduce an option with integer argument.
The integer is put into `int_var`.
-- 
2.11.0.488.g1cece4bcb7a5

[PATCH v4 4/5] describe: teach --match to accept multiple patterns

[Jacob Keller]

From: Jacob Keller 

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 
---
 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 ::
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, ) {
+   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", _candidates,
N_("consider  most recent tags (default: 10)")),
-   OPT_STRING(0, "match",   , N_("pattern"),
+   OPT_STRING_LIST(0, "match", , N_("pattern"),
   N_("only consider tags matching ")),
OPT_BOOL(0, "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();
@@ -440,8 +456,8 @@ int cmd_describe(int argc, const char **argv, const char 
*prefix)
argv_array_push(, "--always");
if (!all) {
argv_array_push(, "--tags");
-   if (pattern)
-   argv_array_pushf(, "--refs=refs/tags/%s", 
pattern);
+   for_each_string_list_item(item, )
+   argv_array_pushf(, "--refs=refs/tags/%s", 
item->string);
}
if (argc)
argv_array_pushv(, 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 &&

Re: [PATCH] attr: mark a file-local symbol as static

[Brandon Williams]

On 01/18, Ramsay Jones wrote:
> 
> Signed-off-by: Ramsay Jones 
> ---
> 
> 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

[PATCH v4 5/5] describe: teach describe negative pattern matches

[Jacob Keller]

From: Jacob Keller 

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 
---
 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 ::
+   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, _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  most recent tags (default: 10)")),
OPT_STRING_LIST(0, "match", , N_("pattern"),
   N_("only consider tags matching ")),
+   OPT_STRING_LIST(0, "exclude", _patterns, N_("pattern"),
+  N_("do not consider tags matching ")),
OPT_BOOL(0, "always",,
N_("show abbreviated commit object as fallback")),
{OPTION_STRING, 0, "dirty",  , N_("mark"),
@@ -458,6 +477,8 @@ int cmd_describe(int argc, const char **argv, const char 
*prefix)
argv_array_push(, "--tags");
for_each_string_list_item(item, )
argv_array_pushf(, "--refs=refs/tags/%s", 
item->string);
+   for_each_string_list_item(item, _patterns)
+   argv_array_pushf(, 
"--exclude=refs/tags/%s", item->string);
}
if (argc)
argv_array_pushv(, 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
+++ 

Re: [PATCH v3 2/5] name-rev: extend --refs to accept multiple patterns

[Junio C Hamano]

Jacob Keller  writes:

> On Wed, Jan 18, 2017 at 12:04 PM, Junio C Hamano  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".

Re: [PATCH v3 3/5] name-rev: add support to exclude refs by pattern match

[Jacob Keller]

On Wed, Jan 18, 2017 at 1:56 PM, Junio C Hamano  wrote:
> Jacob Keller  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

Re: [PATCH v3 1/2] difftool: add a skeleton for the upcoming builtin

[Brandon Williams]

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

[PATCH] attr: mark a file-local symbol as static

[Ramsay Jones]

Signed-off-by: Ramsay Jones 
---

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, (hashmap_cmp_fn) attr_hash_entry_cmp, 0);
 }
-- 
2.11.0

Re: [PATCH 3/4] document add_[file_]to_index

[Stefan Beller]

On Wed, Jan 18, 2017 at 1:22 PM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>> Signed-off-by: Stefan Beller 
>> ---
>>  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);

Re: [PATCH 2/4] remove_index_entry_at: move documentation to cache.h

[Stefan Beller]

On Wed, Jan 18, 2017 at 1:16 PM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>> Signed-off-by: Stefan Beller 
>> ---
>>  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.

Re: [PATCH 1/4] document index_name_pos

[Stefan Beller]

On Wed, Jan 18, 2017 at 1:11 PM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>> Signed-off-by: Stefan Beller 
>> ---
>>  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(, "a", 1) -> -1
>> + * index_name_pos(, "b", 1) ->  0
>> + * index_name_pos(, "c", 1) -> -2
>> + * index_name_pos(, "d", 1) ->  1
>> + * index_name_pos(, "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(, "g", 1) -> -3 - 1 = -4
> index_name_pos(, "h", 1) -> -5 - 1 = -6
>
>> + * index_name_pos(, "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.

Re: [PATCH v3 3/5] name-rev: add support to exclude refs by pattern match

[Junio C Hamano]

Jacob Keller  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.

Re: "git diff --ignore-space-change --stat" lists files with only whitespace differences as "changed"

[Junio C Hamano]

Jeff King  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--;
> + }
> + }
>   }

Re: [PATCH v3 1/5] doc: add documentation for OPT_STRING_LIST

[Junio C Hamano]

"Philip Oakley"  writes:

>>> +`OPT_STRING_LIST(short, long, , arg_str, description)`::
>>> + Introduce an option with string argument.
>>> + The string argument is stored as an element in `` 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-` would make it a bit clearer, but that is ugly.
>
> Bikeshedding:: `--no-` 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
 mentioned anywhere in the enumeration header.

Re: git fast-import crashing on big imports

[Jeff King]

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(_base_cache, );
-entry;
-entry = hashmap_iter_next()) {
+   struct list_head *lru, *tmp;
+   list_for_each_safe(lru, tmp, _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

Re: [PATCH v3 3/5] name-rev: add support to exclude refs by pattern match

[Jacob Keller]

On Wed, Jan 18, 2017 at 12:11 PM, Junio C Hamano  wrote:
> Jacob Keller  writes:
>
>> From: Jacob Keller 
>>
>> 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=::
>> + 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

Re: [PATCH] mingw: follow-up to "replace isatty() hack"

[Junio C Hamano]

Johannes Sixt  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 
>
> 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.schinde...@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.

Re: [PATCH 4/4] documentation: retire unfinished documentation

[Junio C Hamano]

Stefan Beller  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.

Re: [PATCH 3/4] document add_[file_]to_index

[Junio C Hamano]

Stefan Beller  writes:

> Signed-off-by: Stefan Beller 
> ---
>  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);

Re: [PATCH v3 2/5] name-rev: extend --refs to accept multiple patterns

[Jacob Keller]

On Wed, Jan 18, 2017 at 12:04 PM, Junio C Hamano  wrote:
> Jacob Keller  writes:
>
>> From: Jacob Keller 
>>
>> 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=::
>>   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, >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

Re: [PATCH 25/27] attr: store attribute stacks in hashmap

[Stefan Beller]

On Wed, Jan 18, 2017 at 12:39 PM, Stefan Beller  wrote:
> On Thu, Jan 12, 2017 at 3:53 PM, Brandon Williams  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(, 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' ?

Re: [PATCH 2/4] remove_index_entry_at: move documentation to cache.h

[Junio C Hamano]

Stefan Beller  writes:

> Signed-off-by: Stefan Beller 
> ---
>  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];

Re: [PATCH] mingw: follow-up to "replace isatty() hack"

[Johannes Sixt]

Am 18.01.2017 um 20:19 schrieb Junio C Hamano:

Johannes Schindelin  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 

but it will take a day or two until I can test the patch.

-- Hannes

Re: [PATCH 5/5] describe: teach describe negative pattern matches

[Jacob Keller]

On Wed, Jan 18, 2017 at 4:44 AM, Johannes Schindelin
 wrote:
> Hi Jake,
>
> On Tue, 17 Jan 2017, Jacob Keller wrote:
>
>> On Fri, Jan 13, 2017 at 1:31 PM, Johannes Sixt  wrote:
>> > Am 13.01.2017 um 07:57 schrieb Jacob Keller:
>> >>
>> >> On Thu, Jan 12, 2017 at 10:43 PM, Johannes Sixt  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

Re: [PATCH 1/4] document index_name_pos

[Junio C Hamano]

Stefan Beller  writes:

> Signed-off-by: Stefan Beller 
> ---
>  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(, "a", 1) -> -1
> + * index_name_pos(, "b", 1) ->  0
> + * index_name_pos(, "c", 1) -> -2
> + * index_name_pos(, "d", 1) ->  1
> + * index_name_pos(, "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(, "g", 1) -> -3 - 1 = -4
index_name_pos(, "h", 1) -> -5 - 1 = -6

> + * index_name_pos(, "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 */

Re: [PATCH v3 1/5] doc: add documentation for OPT_STRING_LIST

[Jacob Keller]

On Wed, Jan 18, 2017 at 11:45 AM, Junio C Hamano  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-` 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-", on the command line.
>
> I dunno.

Maybe we can rephrase this "The list is reset via `--no-option`"?

Thanks,
Jake

Re: "git diff --ignore-space-change --stat" lists files with only whitespace differences as "changed"

[Jeff King]

On Wed, Jan 18, 2017 at 12:57:12PM -0800, Junio C Hamano wrote:

> Jeff King  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

Re: [PATCH v3 0/5] extend git-describe pattern matching

[Jacob Keller]

On Wed, Jan 18, 2017 at 12:18 PM, Junio C Hamano  wrote:
> Jacob Keller  writes:
>
>> From: Jacob Keller 
>>
>> ** 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

Re: [PATCH 25/27] attr: store attribute stacks in hashmap

[Brandon Williams]

On 01/18, Stefan Beller wrote:
> On Thu, Jan 12, 2017 at 3:53 PM, Brandon Williams  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(, 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

Re: [PATCH 25/27] attr: store attribute stacks in hashmap

[Stefan Beller]

On Thu, Jan 12, 2017 at 3:53 PM, Brandon Williams  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(, 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) ?

Re: [PATCH 25/27] attr: store attribute stacks in hashmap

[Brandon Williams]

On 01/13, Junio C Hamano wrote:
> Brandon Williams  writes:
> 
> > The last big hurdle towards a thread-safe API for the attribute system
> > is the reliance on a global attribute stack that is modified during each
> > call into the attribute system.
> >
> > This patch removes this global stack and instead a stack is retrieved or
> > constructed locally.  Since each of these stacks is only used as a
> > read-only structure once constructed, they can be stored in a hashmap
> > and shared between threads.
> 
> Very good.
> 
> The reason why the original code used a stack was because it wanted
> to keep only the info read from releavant files in-core, discarding
> ones from files no-longer relevant (because the traversal switched
> to another subdirectory of the same parent directory), to avoid the
> memory consumption grow unbounded.  It probably was a premature
> "optimization" that we can do without, so keeping everything we have
> read so far in a hashmap (which is my understanding of what is going
> on in this patch) is probably OK.
> 
> I suspect that this hashmap may eventually need to become per
> attr_check if we want to follow through the optimization envisioned
> by patch 15/27.
> 
> Inside fill(), path_matches() is called for the number of match_attr
> in the entire attribute stack but it is wasteful to check if the
> path matches with the a.u.pat if none of the a.state[] entries talk
> about attributes and macros that are eventually get used by the
> caller of check_attr().  By introducing a wrapping structure, 15/27
> wanted to make sure that we have a place to store a "reduced"
> attribute stack that is kept per attr_check that has only entries
> from the files that talk about the attributes the particular
> attr_check wants to learn about.
> 
> I need to think about this a bit more, but I do not offhand think
> that it makes future such enhancement to make it per-check harder to
> move from a global stack to a global hashmap, i.e. the above is not
> an objection to this step.

If we want to continue through and do the optimization you originally
envisioned then I may need to rethink this patch.  One thing we did talk
about offline was doing another check prior to the path_match() function
call which looks through the list of state structs to see if one of
those states would actually have an affect on the array being used to
collect attributes.  Though that may be an optimization which can be
done in addition to creating a reduced stack.

The one difficulty (which you pointed out in comment form) is if we have
a reduced attribute stack that is stored per attr_check then handling
the cleanup when the direction is changed may be messy.

-- 
Brandon Williams

Re: git fast-import crashing on big imports

[Jeff King]

On Wed, Jan 18, 2017 at 09:20:07PM +0100, Ulrich Spörlein wrote:

> I found your commit via bisect in case you were wondering. Thanks for
> picking this up.

Still downloading. However, just looking at the code, the obvious
culprit would be clear_delta_base_cache(), which is called from
literally nowhere except fast-import, and then only when checkpointing.

-Peff

Re: git fast-import crashing on big imports

[Jeff King]

On Wed, Jan 18, 2017 at 09:38:14AM -0500, Jeff King wrote:

> On Wed, Jan 18, 2017 at 03:01:17PM +0100, Ulrich Spoerlein wrote:
> 
> > Yo Jeff, your commit 8261e1f139db3f8aa6f9fd7d98c876cbeb0f927c from Aug
> > 22nd, that changes delta_base_cache to use hashmap.h is the culprit for
> > git fast-import crashing on large imports.
> 
> I actually saw your bug report the other day and tried to download the
> dump file, but got a 404. Can you double check that it is available?

The URL in your original mail was bogus:

> > I have a dump file that you can grab at
> > http://scan.spoerlein.net/pub/freebsd-base.dump.xz (19G, 55G uncompressed)

Via guess-and-check, I found:

  http://www.spoerlein.net/pub/freebsd-base.git.fi.xz

I'll see if I can reproduce the problem.

-Peff

Re: [PATCH 21/27] attr: use hashmap for attribute dictionary

[Brandon Williams]

On 01/18, Stefan Beller wrote:
> On Thu, Jan 12, 2017 at 3:53 PM, Brandon Williams  wrote:
> 
> > +/* Initialize an 'attr_hashmap' object */
> > +void attr_hashmap_init(struct attr_hashmap *map)
> 
> In case a reroll is needed, mark this static please.

Will do.

-- 
Brandon Williams

Re: [PATCH 21/27] attr: use hashmap for attribute dictionary

[Stefan Beller]

On Thu, Jan 12, 2017 at 3:53 PM, Brandon Williams  wrote:

> +/* Initialize an 'attr_hashmap' object */
> +void attr_hashmap_init(struct attr_hashmap *map)

In case a reroll is needed, mark this static please.

Re: [PATCH v3 0/5] extend git-describe pattern matching

[Junio C Hamano]

Jacob Keller  writes:

> From: Jacob Keller 
>
> ** 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.

Re: [PATCH v3 5/5] describe: teach describe negative pattern matches

[Junio C Hamano]

Jacob Keller  writes:

> From: Jacob Keller 
>
> Teach git-describe the `--exclude` option which will allow specifying
> a glob pattern of tags to ignore. This can be combined with the
> `--match` patterns to enable more flexibility in determining which tags
> to consider.
>
> For example, suppose you wish to find the first official release tag
> that contains a certain commit. If we assume that official release tags
> are of the form "v*" and pre-release candidates include "*rc*" in their
> name, we can now find the first tag that introduces commit abcdef via:
>
>   git describe --contains --match="v*" --exclude="*rc*"
>
> Add documentation and tests for this change.
>
> Signed-off-by: Jacob Keller 
> ---

The above is much better than 3/5 with a concrete example (compared
to the vague "certain kinds of references").  It also does not have
the "we check this first and then that" ;-).

> diff --git a/Documentation/git-describe.txt b/Documentation/git-describe.txt
> index 7ad41e2f6ade..21a43b78924a 100644
> --- a/Documentation/git-describe.txt
> +++ b/Documentation/git-describe.txt
> @@ -88,6 +88,14 @@ OPTIONS
>   patterns will be considered. Use `--no-match` to clear and reset the
>   list of patterns.
>  
> +--exclude ::
> + Do not consider tags matching the given `glob(7)` pattern, excluding
> + the "refs/tags/" prefix. This can be used to narrow the tag space and
> + find only tags matching some meaningful criteria. If given multiple
> + times, a list of patterns will be accumulated and tags matching any
> + of the patterns will be excluded. Use `--no-exclude` to clear and
> + reset the list of patterns.
> +

Similar to 3/5, perhaps we want to say something about interaction
between this one and --match?

Re: [PATCH v3 3/5] name-rev: add support to exclude refs by pattern match

[Junio C Hamano]

Jacob Keller  writes:

> From: Jacob Keller 
>
> 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=::
> + 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"?

Re: [PATCH v3 1/5] doc: add documentation for OPT_STRING_LIST

[Philip Oakley]

From: "Junio C Hamano" 
v3 1/5] doc: add documentation for OPT_STRING_LIST



Jacob Keller  writes:


From: Jacob Keller 

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 
---
 Documentation/technical/api-parse-options.txt | 5 +
 1 file changed, 5 insertions(+)

diff --git a/Documentation/technical/api-parse-options.txt 
b/Documentation/technical/api-parse-options.txt

index 27bd701c0d68..6914f54f5f44 100644
--- a/Documentation/technical/api-parse-options.txt
+++ b/Documentation/technical/api-parse-options.txt
@@ -168,6 +168,11 @@ There are some macros to easily define options:
 Introduce an option with string argument.
 The string argument is put into `str_var`.

+`OPT_STRING_LIST(short, long, , arg_str, description)`::
+ Introduce an option with string argument.
+ The string argument is stored as an element in `` 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-` would make it a bit clearer, but that is ugly.


Bikeshedding:: `--no-` maybe, i.e. just surround the option word 
with the angle brackets to indicate it is to be replaced by the real 
option's name.




The "Reset the list" is an instruction to the end-users who interact
with a program written by readers of this document using
OPT_STRING_LIST(), and it feels a bit out of place.  Perhaps

End users can reset the list by negating the option,
i.e. passing "--no-", on the command line.

I dunno.

Anyway, thanks for adding a missing doc here.


 `OPT_INTEGER(short, long, _var, description)`::
 Introduce an option with integer argument.
 The integer is put into `int_var`.



--

Philip 

Re: [PATCH v6 4/6] builtin/tag: add --format argument for tag -v

[Eric Wong]

Junio C Hamano  wrote:
> Santiago Torres  writes:
> 
> <>???
> 
> Eric, I've noticed that this message
> 
>   
> http://public-inbox.org/git/20170118182831.pkhqu2np3bh2puei@LykOS.localdomain/
> 
> and all messages from Santiago appear empty when they come via
> public-inbox.org; the reason I suspect we haven't heard much
> complaints is because nobody else around here sends multipart/signed
> disposition inline other than Santiago.

Eeep!  This looks like a regression I introduced when working
around Richard Hansen's S/MIME mails the other week on git@vger:

  https://public-inbox.org/meta/2017011035.GB27356@dcvr/T/#u

Worse is they now corrupted on the way in into the git repo
because of search indexing.  Will fix ASAP.  Thanks for the
heads up.

Re: [PATCH v3 2/5] name-rev: extend --refs to accept multiple patterns

[Junio C Hamano]

Jacob Keller  writes:

> From: Jacob Keller 
>
> Teach git name-rev to take a string list of patterns from --refs instead
> of only a single pattern. The list of patterns will be matched
> inclusively, such that a ref only needs to match one pattern to be
> included. If a ref will only be excluded if it does not match any of the
> patterns.

I think "If a" in the last sentence should be "A".

>  --refs=::
>   Only use refs whose names match a given shell pattern.  The pattern
> - can be one of branch name, tag name or fully qualified ref name.
> + can be one of branch name, tag name or fully qualified ref name. If
> + given multiple times, use refs whose names match any of the given shell
> + patterns. Use `--no-refs` to clear any previous ref patterns given.

Unlike 1/5, this is facing the end-users, and the last sentence is
very appropriate.

> + if (data->ref_filters.nr) {
> + struct string_list_item *item;
> + int matched = 0;
> +
> + /* See if any of the patterns match. */
> + for_each_string_list_item(item, >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).

Re: [PATCH v3 1/5] doc: add documentation for OPT_STRING_LIST

[Junio C Hamano]

Jacob Keller  writes:

> From: Jacob Keller 
>
> 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 
> ---
>  Documentation/technical/api-parse-options.txt | 5 +
>  1 file changed, 5 insertions(+)
>
> diff --git a/Documentation/technical/api-parse-options.txt 
> b/Documentation/technical/api-parse-options.txt
> index 27bd701c0d68..6914f54f5f44 100644
> --- a/Documentation/technical/api-parse-options.txt
> +++ b/Documentation/technical/api-parse-options.txt
> @@ -168,6 +168,11 @@ There are some macros to easily define options:
>   Introduce an option with string argument.
>   The string argument is put into `str_var`.
>  
> +`OPT_STRING_LIST(short, long, , arg_str, description)`::
> + Introduce an option with string argument.
> + The string argument is stored as an element in `` 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-` would make it a bit clearer, but that is ugly.

The "Reset the list" is an instruction to the end-users who interact
with a program written by readers of this document using
OPT_STRING_LIST(), and it feels a bit out of place.  Perhaps

End users can reset the list by negating the option,
i.e. passing "--no-", on the command line.

I dunno.

Anyway, thanks for adding a missing doc here.

>  `OPT_INTEGER(short, long, _var, description)`::
>   Introduce an option with integer argument.
>   The integer is put into `int_var`.

Re: [RFC] stash --continue

[Samuel Lijin]

>> At least `git stash pop --continue` would be consistent with all other
>> `--continue` options in Git that I can think of...

> Alas, I disagree!

I'm with Johannes here. "git stash" sans subcommand is pretty
explicitly defined as "git stash save", so by similar logic, "git
stash --continue", if anything, would be "git stash save --continue".

I do agree that there's a slight problem with hunting down consistency
in implementations of --continue since there aren't other usages that
involve subcommands (rebase, cp, merge) but I can't think of "git
stash" as a completely specified command, whereas I do see "git stash
pop" and "git stash apply" as completely specified.

On Wed, Jan 18, 2017 at 12:44 PM, Marc Branchaud  wrote:
> On 2017-01-18 11:34 AM, Johannes Schindelin wrote:
>>
>> Hi Marc,
>>
>> On Wed, 18 Jan 2017, Marc Branchaud wrote:
>>
>>> On 2017-01-16 05:54 AM, Johannes Schindelin wrote:
>>>
 On Mon, 16 Jan 2017, Stephan Beyer wrote:

> a git-newbie-ish co-worker uses git-stash sometimes. Last time he
> used "git stash pop", he got into a merge conflict. After he
> resolved the conflict, he did not know what to do to get the
> repository into the wanted state. In his case, it was only "git add
> " followed by a "git reset" and a "git stash drop",
> but there may be more involved cases when your index is not clean
> before "git stash pop" and you want to have your index as before.
>
> This led to the idea to have something like "git stash
> --continue"[1]


 More like "git stash pop --continue". Without the "pop" command, it
 does not make too much sense.
>>>
>>>
>>> Why not?  git should be able to remember what stash command created the
>>> conflict.  Why should I have to?  Maybe the fire alarm goes off right
>>> when I
>>> run the stash command, and by the time I get back to it I can't remember
>>> which operation I did.  It would be nice to be able to tell git to "just
>>> finish off (or abort) the stash operation, whatever it was".
>>
>>
>> That reeks of a big potential for confusion.
>>
>> Imagine for example a total Git noob who calls `git stash list`, scrolls
>> two pages down, then hits `q` by mistake. How would you explain to that
>> user that `git stash --continue` does not continue showing the list at the
>> third page?
>
>
> Sorry, but I have trouble taking that example seriously.  It assumes such a
> level of "noobness" that the user doesn't even understand how standard
> command output paging works, not just with git but with any shell command.
>
>> Even worse: `git stash` (without arguments) defaults to the `save`
>> operation, so any user who does not read the documentation (and who does?)
>> would assume that `git stash --continue` *also* implies `save`.
>
>
> Like the first example, your user is trying to "continue" a command that is
> already complete.  It's like try to do "git rebase --continue" when there's
> no rebase operation underway.
>
> Now, maybe there is some way for "git stash save" (implied or explicit) to
> stop partway through the operation.  I can't imagine such a situation (out
> of disk space, maybe?), particularly where the user would expect "git stash
> save" to leave things in a half-finished state.  To me "git stash save"
> should be essentially all-or-nothing.
>
> However, if there were such a partial-failure scenario, then I think it
> would be perfectly reasonable for "git stash --continue" to finish the save
> operation, assuming that the failure condition has been resolved.
>
>> If that was not enough, there would still be the overall design of Git's
>> user interface. You can call it confusing, inconsistent, with a lot of
>> room for improvement, and you would be correct. But none of Git's commands
>> has a `--continue` option that remembers the latest subcommand and
>> continues that. To introduce that behavior in `git stash` would disimprove
>> the situation.
>
>
> I think it's more the case that none of the current continuable commands
> have subcommands (though I can't think of all the continuable or abortable
> operations offhand, so maybe I'm wrong).  I think we're discussing new UI
> ground here.
>
> And since the pattern is already "git foo --continue", it seems more
> consistent to me for it to be "git stash --continue" as well. Especially
> since there can be only one partially-complete stash sub-operation at one
> time (per workdir, at least).  So there's no reason to change the pattern
> just for the stash command.
>
> Think of it this way:  All the currently continuable/abortable commands put
> the repository in a shaky state, where performing certain other operations
> would be ill advised.  Attempting to start a rebase while a merge conflict
> is unresolved, for example.  IIRC, git actually tries to stop users from
> shooting their feet in this way.
>
> And so it should be for the stash operation:  If applying a stash yields a
> conflict, it has to be 

Re: [PATCH] mingw: follow-up to "replace isatty() hack"

[Junio C Hamano]

Johannes Schindelin  writes:

> The version of the "replace isatty() hack" that got applied to the
> `maint` branch did not actually reflect the latest iteration of the
> patch series: v3 was sent out with these changes, as requested by
> the reviewer Johannes Sixt:

Thanks for an update.  

Does the above "the version taken was not updated before getting
merged" mistake only apply to 'maint', or is it also true for
'master'?  

As a rule we only merge things to 'maint' that have already been
merged to 'master', so I am guessing that the answer is yes, in
which case I'd queue it on js/mingw-isatty and then merge it to
next, master and maint in that order as usual.

> - reworded the comment about "recycling handles"
>
> - moved the reassignment of the `console` variable before the dup2()
>   call so that it is valid at all times
>
> - removed the "handle = INVALID_HANDLE_VALUE" assignment, as the local
>   variable `handle` is not used afterwards anyway
>

Also if the v3 had been reviewed and acked, it would be nice to have
the acked-by around here (which I can locally do).  Hannes?

> Signed-off-by: Johannes Schindelin 
> ---

Thanks.

> Published-As: 
> https://github.com/dscho/git/releases/tag/mingw-isatty-fixup-fixup-v1
> Fetch-It-Via: git fetch https://github.com/dscho/git 
> mingw-isatty-fixup-fixup-v1
>
>  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).
>
> base-commit: 3313b78c145ba9212272b5318c111cde12bfef4a

Re: [RFC] stash --continue

[Stephan Beyer]

Hi,

On 01/18/2017 04:41 PM, Marc Branchaud wrote:
> On 2017-01-16 05:54 AM, Johannes Schindelin wrote:
>> On Mon, 16 Jan 2017, Stephan Beyer wrote:
>>> a git-newbie-ish co-worker uses git-stash sometimes. Last time he used
>>> "git stash pop", he got into a merge conflict. After he resolved the
>>> conflict, he did not know what to do to get the repository into the
>>> wanted state. In his case, it was only "git add "
>>> followed by a "git reset" and a "git stash drop", but there may be more
>>> involved cases when your index is not clean before "git stash pop" and
>>> you want to have your index as before.
>>>
>>> This led to the idea to have something like "git stash --continue"[1]
>>
>> More like "git stash pop --continue". Without the "pop" command, it does
>> not make too much sense.
> 
> Why not?  git should be able to remember what stash command created the
> conflict.  Why should I have to?

Dscho and Junio gave you a git-perspective argument.
I give you a user-perspective one:
What if you did "git stash pop" and ran into an (unexpected) conflict.
You resolve it, and you probably - for some reason - don't want to drop
the stash now, as "git stash --continue" (assuming "pop") would do. So
I'd regard it as a feature if you could now run "git stash apply
--continue" to just finish the job without dropping.

Best
Stephan

PS: I put this idea in my todo priority queue. If somebody else is
interested: I am not going to work at this idea before February.

PPS: Any opinions about the mentioned "backwards-compatibility" issue
that people are then forced to finish their commits with "--continue"
instead of "git reset" or "git commit"?

Re: [PATCH v5 3/3] Retire the scripted difftool

[Junio C Hamano]

Johannes Schindelin  writes:

> Hi Junio,
>
> On Tue, 17 Jan 2017, Junio C Hamano wrote:
>
>> Johannes Schindelin  writes:
>> 
>> > It served its purpose, but now we have a builtin difftool. Time for the
>> > Perl script to enjoy Florida.
>> >
>> > Signed-off-by: Johannes Schindelin 
>> > ---
>> 
>> The endgame makes a lot of sense.  Both in the cover letter and in
>> the previous patch you talk about having both in the released
>> version, so do you want this step to proceed slower than the other
>> two?
>
> I did proceed that slowly. Already three Git for Windows versions have
> been released with both.
>
> But I submitted this iteration with this patch, so my intent is clearly to
> retire the Perl script.

Ok, I was mostly reacting to 2/3 while I am reading it:

The reason: this new, experimental, builtin difftool will be shipped as
part of Git for Windows v2.11.0, to allow for easier large-scale
testing, but of course as an opt-in feature.

as there is no longer an opportunity to participate in this opt-in
testing, unless 3/3 is special cased and delayed.

Re: Git: new feature suggestion

[Joao Pinto]

Hi Stefan,

Às 6:50 PM de 1/18/2017, Stefan Beller escreveu:
> On Wed, Jan 18, 2017 at 2:40 AM, Joao Pinto  wrote:
>> Hello,
>>
>> My name is Joao Pinto, I work at Synopsys and I am a frequent Linux Kernel
>> contributor.
>>
>> Let me start by congratulate you for the fantastic work you have been doing 
>> with
>> Git which is an excellent tool.
>>
>> The Linux Kernel as all systems needs to be improved and re-organized to be
>> better prepared for future development and sometimes we need to change
>> folder/files names or even move things around.
>> I have seen a lot of Linux developers avoid this re-organization operations
>> because they would lose the renamed file history, because a new log is 
>> created
>> for the new file, even if it is a renamed version of itself.
> 
> Well there are a couple of things to help with digging in the logs.
> 
> git log:
>--follow
>Continue listing the history of a file beyond renames (works only
>for a single file).
> 
> -M[], --find-renames[=]
>If generating diffs, detect and report renames for each commit. For
>following files across renames while traversing history, see
>--follow. If n is specified, it is a threshold on the similarity
>index (i.e. amount of addition/deletions compared to the file’s
>size). For example, -M90% means Git should consider a delete/add
>pair to be a rename if more than 90% of the file hasn’t changed.
>Without a % sign, the number is to be read as a fraction, with a
>decimal point before it. I.e., -M5 becomes 0.5, and is thus the
>same as -M50%. Similarly, -M05 is the same as -M5%. To limit
>detection to exact renames, use -M100%. The default similarity
>index is 50%.
> 
>-C[], --find-copies[=]
>Detect copies as well as renames. See also --find-copies-harder. If
>n is specified, it has the same meaning as for -M.
> 
> 
> 
>> I am sending you this e-mail to suggest the creation of a new feature in Git:
>> when renamed, a file or folder should inherit his parent’s log and a 
>> “rename: …”
>> would be automatically created or have some kind of pointer to its “old” 
>> form to
>> make history analysis easier.
> 
> How do you currently analyse history, which detailed feature is missing?
> 
> Mind that in the Git data model we deliberately do not record the rename
> at commit time, but rather want to identify the renames at log time.
> This is because
> in the meantime between commit and log viewing someone could have written
> a better rename detection, whereas at commit time we'd be stuck with ancient
> cruft forever. ;)
> 
>>
>> I volunteer to help in the new feature if you find it useful. I think it 
>> would
>> improve log history analysis and would enable developers to better organize 
>> old
>> code.
> 
> IMHO complete renames (i.e. git mv path/a/file.c path/b/thing.c) are already
> covered quite well. Partial rename (e.g. moving code from one file into two
> separate files or vice versa) is still a bit hard.
> 
> I started such a new feature, see
> https://urldefense.proofpoint.com/v2/url?u=https-3A__public-2Dinbox.org_git_20160903033120.20511-2D1-2Dsbeller-40google.com_=DwIFaQ=DPL6_X_6JkXFx7AXWqB0tg=s2fO0hii0OGNOv9qQy_HRXy-xAJUD1NNoEcc3io_kx0=BseICq5hy9UHxmX2XP8oPYLbn-HoEUlEuVUzqPHkX58=PybtKK0ELH3Nld_CQSYZnLqCQOWvnU4Fjj5iV_7EKqE=
>  
> latest code is at 
> https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_stefanbeller_git_commits_colored-5Fdiff12=DwIFaQ=DPL6_X_6JkXFx7AXWqB0tg=s2fO0hii0OGNOv9qQy_HRXy-xAJUD1NNoEcc3io_kx0=BseICq5hy9UHxmX2XP8oPYLbn-HoEUlEuVUzqPHkX58=pkTehcEmeHVLHdcNbUiU03meyH10cgUbGqLgOqXcL6w=
>  ,
> but the latest two commits are bogus and need rewriting.
> 
> I think this feature is not 100% what you are aiming at, but is very close.
> 
> Thanks,
> Stefan
> 

Great info, helps a lot! I am going to analyse and get back to you ASAP.

Thanks

Re: What's cooking in git.git (Jan 2017, #02; Sun, 15)

[Junio C Hamano]

Johannes Sixt  writes:

> Am 17.01.2017 um 20:17 schrieb Junio C Hamano:
>> So... can we move this forward?
>
> I have no objects anymore.
>
> -- Hannes

Alright, thanks.  

Dscho what's your assessment?  My "I do not think" before the quoted
"can we move" above was more about giving a statement for people to
argue against, by saying "no your understanding is wrong", in the
hope that it would highlight why we need more work in this area if
needed and in what way.

Thanks.

Re: [RFC] stash --continue

[Junio C Hamano]

Johannes Schindelin  writes:

>> > More like "git stash pop --continue". Without the "pop" command, it
>> > does not make too much sense.
>> 
>> Why not?  git should be able to remember what stash command created the
>> conflict.  Why should I have to?  Maybe the fire alarm goes off right when I
>> run the stash command, and by the time I get back to it I can't remember
>> which operation I did.  It would be nice to be able to tell git to "just
>> finish off (or abort) the stash operation, whatever it was".
>
> That reeks of a big potential for confusion.

Yup.  I agree everything you said in the message I am responding
to.  Marc's argument will inevitably lead to: It should be
sufficient to say "git --continue", as Git should remember
everything for me.  I do not think we want to go there.

RE: [RFC] Add support for downloading blobs on demand

[Ben Peart]

We actually pursued trying to make submodules work for some time and 
even built tooling around trying to work around some of the issues we 
ran into (not repo.py but along a similar line) before determining that 
we would be better served by having a single repo and solving the scale 
issues.  I don't want to rehash the arguments for/against a single repo 
- suffice it to say, we have opted for a single large repo. :)

Thanks,

Ben
> -Original Message-
> From: Stefan Beller [mailto:sbel...@google.com]
> Sent: Tuesday, January 17, 2017 5:24 PM
> To: Martin Fick 
> Cc: Ben Peart ; Shawn Pearce
> ; git ;
> benpe...@microsoft.com
> Subject: Re: [RFC] Add support for downloading blobs on demand
> 
> On Tue, Jan 17, 2017 at 2:05 PM, Martin Fick 
> wrote:
> > On Tuesday, January 17, 2017 04:50:13 PM Ben Peart wrote:
> >> While large files can be a real problem, our biggest issue today is
> >> having a lot (millions!) of source files when any individual
> >> developer only needs a small percentage of them.  Git with 3+ million
> >> local files just doesn't perform well.
> >
> > Honestly, this sounds like a problem better dealt with by using git
> > subtree or git submodules, have you considered that?
> >
> > -Martin
> >
> 
> I cannot speak for subtrees as I have very little knowledge on them.
> But there you also have the problem that *someone* has to have a whole
> tree? (e.g. the build bot)
> 
> submodules however comes with a couple of things attached, both positive
> as well as negative points:
> 
> * it offers ACLs along the way. ($user may not be allowed to
>   clone all submodules, but only those related to the work)
> * The conceptual understanding of git just got a lot harder.
>   ("Yo dawg, I heard you like git, so I put git repos inside
>   other git repos"), it is not easy to come up with reasonable
>   defaults for all usecases, so the everyday user still has to
>   have some understanding of submodules.
> * typical cheap in-tree operations may become very expensive:
>   -> moving a file from one location to another (in another
>  submodule) adds overhead, no rename detection.
> * We are actively working on submodules, so there is
>   some momentum going already.
> * our experiments with Android show that e.g. fetching (even
>   if you have all of Android) becomes a lot faster for everyday
>   usage as only a few repositories change each day). This
>   comparision was against the repo tool, that we currently
>   use for Android. I do not know how it would compare against
>   single repo Git, as having such a large repository seemed
>   complicated.
> * the support for submodules in Git is already there, though
>   not polished. The positive side is to have already a good base,
>   the negative side is to have support current use cases.
> 
> Stefan

Re: [PATCH v6 4/6] builtin/tag: add --format argument for tag -v

[Junio C Hamano]

Santiago Torres  writes:

>> Squashing the following into this commit solves this issue with the
>> former approach.  The lines it touches are all from 4/6 and I view
>> all of it as general improvement, including type correctness and
>> code formatting.
>
> Thanks!
>
> Should I re-roll this really quick? Or would you rather apply this on
> your tree directly? 

Nah, local squashing should be sufficient in this case.  The squash
only touches a single patch from the original and it itself is easy
to review (and was reviewed already from what I can tell in this
thread).

Re: [RFC] stash --continue

[Marc Branchaud]

On 2017-01-18 11:34 AM, Johannes Schindelin wrote:

Hi Marc,

On Wed, 18 Jan 2017, Marc Branchaud wrote:


On 2017-01-16 05:54 AM, Johannes Schindelin wrote:


On Mon, 16 Jan 2017, Stephan Beyer wrote:


a git-newbie-ish co-worker uses git-stash sometimes. Last time he
used "git stash pop", he got into a merge conflict. After he
resolved the conflict, he did not know what to do to get the
repository into the wanted state. In his case, it was only "git add
" followed by a "git reset" and a "git stash drop",
but there may be more involved cases when your index is not clean
before "git stash pop" and you want to have your index as before.

This led to the idea to have something like "git stash
--continue"[1]


More like "git stash pop --continue". Without the "pop" command, it
does not make too much sense.


Why not?  git should be able to remember what stash command created the
conflict.  Why should I have to?  Maybe the fire alarm goes off right when I
run the stash command, and by the time I get back to it I can't remember
which operation I did.  It would be nice to be able to tell git to "just
finish off (or abort) the stash operation, whatever it was".


That reeks of a big potential for confusion.

Imagine for example a total Git noob who calls `git stash list`, scrolls
two pages down, then hits `q` by mistake. How would you explain to that
user that `git stash --continue` does not continue showing the list at the
third page?


Sorry, but I have trouble taking that example seriously.  It assumes 
such a level of "noobness" that the user doesn't even understand how 
standard command output paging works, not just with git but with any 
shell command.



Even worse: `git stash` (without arguments) defaults to the `save`
operation, so any user who does not read the documentation (and who does?)
would assume that `git stash --continue` *also* implies `save`.


Like the first example, your user is trying to "continue" a command that 
is already complete.  It's like try to do "git rebase --continue" when 
there's no rebase operation underway.


Now, maybe there is some way for "git stash save" (implied or explicit) 
to stop partway through the operation.  I can't imagine such a situation 
(out of disk space, maybe?), particularly where the user would expect 
"git stash save" to leave things in a half-finished state.  To me "git 
stash save" should be essentially all-or-nothing.


However, if there were such a partial-failure scenario, then I think it 
would be perfectly reasonable for "git stash --continue" to finish the 
save operation, assuming that the failure condition has been resolved.



If that was not enough, there would still be the overall design of Git's
user interface. You can call it confusing, inconsistent, with a lot of
room for improvement, and you would be correct. But none of Git's commands
has a `--continue` option that remembers the latest subcommand and
continues that. To introduce that behavior in `git stash` would disimprove
the situation.


I think it's more the case that none of the current continuable commands 
have subcommands (though I can't think of all the continuable or 
abortable operations offhand, so maybe I'm wrong).  I think we're 
discussing new UI ground here.


And since the pattern is already "git foo --continue", it seems more 
consistent to me for it to be "git stash --continue" as well. 
Especially since there can be only one partially-complete stash 
sub-operation at one time (per workdir, at least).  So there's no reason 
to change the pattern just for the stash command.


Think of it this way:  All the currently continuable/abortable commands 
put the repository in a shaky state, where performing certain other 
operations would be ill advised.  Attempting to start a rebase while a 
merge conflict is unresolved, for example.  IIRC, git actually tries to 
stop users from shooting their feet in this way.


And so it should be for the stash operation:  If applying a stash yields 
a conflict, it has to be resolved or aborted before something like a 
rebase or merge is attempted.  It doesn't matter which stash subcommand 
created the shaky situation.


In the long run, I think there's even the possibility of generic "git 
continue" and "git abort" commands, that simply continue or abort the 
current partially-complete operation, whatever it is.  (Isn't that the 
ultimate goal of all the "sequencer" work?  I admit I have not been 
following that effort.)



With every new feature, it is not enough to consider its benefits. You
always have to take the potential fallout into account, too.


Agreed.


At least `git stash pop --continue` would be consistent with all other
`--continue` options in Git that I can think of...


Alas, I disagree!

M.

Re: Git: new feature suggestion

[Stefan Beller]

On Wed, Jan 18, 2017 at 2:40 AM, Joao Pinto  wrote:
> Hello,
>
> My name is Joao Pinto, I work at Synopsys and I am a frequent Linux Kernel
> contributor.
>
> Let me start by congratulate you for the fantastic work you have been doing 
> with
> Git which is an excellent tool.
>
> The Linux Kernel as all systems needs to be improved and re-organized to be
> better prepared for future development and sometimes we need to change
> folder/files names or even move things around.
> I have seen a lot of Linux developers avoid this re-organization operations
> because they would lose the renamed file history, because a new log is created
> for the new file, even if it is a renamed version of itself.

Well there are a couple of things to help with digging in the logs.

git log:
   --follow
   Continue listing the history of a file beyond renames (works only
   for a single file).

-M[], --find-renames[=]
   If generating diffs, detect and report renames for each commit. For
   following files across renames while traversing history, see
   --follow. If n is specified, it is a threshold on the similarity
   index (i.e. amount of addition/deletions compared to the file’s
   size). For example, -M90% means Git should consider a delete/add
   pair to be a rename if more than 90% of the file hasn’t changed.
   Without a % sign, the number is to be read as a fraction, with a
   decimal point before it. I.e., -M5 becomes 0.5, and is thus the
   same as -M50%. Similarly, -M05 is the same as -M5%. To limit
   detection to exact renames, use -M100%. The default similarity
   index is 50%.

   -C[], --find-copies[=]
   Detect copies as well as renames. See also --find-copies-harder. If
   n is specified, it has the same meaning as for -M.



> I am sending you this e-mail to suggest the creation of a new feature in Git:
> when renamed, a file or folder should inherit his parent’s log and a “rename: 
> …”
> would be automatically created or have some kind of pointer to its “old” form 
> to
> make history analysis easier.

How do you currently analyse history, which detailed feature is missing?

Mind that in the Git data model we deliberately do not record the rename
at commit time, but rather want to identify the renames at log time.
This is because
in the meantime between commit and log viewing someone could have written
a better rename detection, whereas at commit time we'd be stuck with ancient
cruft forever. ;)

>
> I volunteer to help in the new feature if you find it useful. I think it would
> improve log history analysis and would enable developers to better organize 
> old
> code.

IMHO complete renames (i.e. git mv path/a/file.c path/b/thing.c) are already
covered quite well. Partial rename (e.g. moving code from one file into two
separate files or vice versa) is still a bit hard.

I started such a new feature, see
https://public-inbox.org/git/20160903033120.20511-1-sbel...@google.com/
latest code is at https://github.com/stefanbeller/git/commits/colored_diff12,
but the latest two commits are bogus and need rewriting.

I think this feature is not 100% what you are aiming at, but is very close.

Thanks,
Stefan

Re: [PATCH v6 4/6] builtin/tag: add --format argument for tag -v

[Santiago Torres]

On Wed, Jan 18, 2017 at 10:44:03AM -0800, Junio C Hamano wrote:
> Santiago Torres  writes:
> 
Was:

Thanks!

Would you want me to re-roll really quick? or would you rather apply
this on your side?

Thanks,
-Santiago.
> 
> Eric, I've noticed that this message
> 
>   
> http://public-inbox.org/git/20170118182831.pkhqu2np3bh2puei@LykOS.localdomain/
> 
> and all messages from Santiago appear empty when they come via
> public-inbox.org; the reason I suspect we haven't heard much
> complaints is because nobody else around here sends multipart/signed
> disposition inline other than Santiago.
> 

Interesting, I thought I wasn't inlining the .asc. I guess I can disable
signing for this ML for the time being. 

Thanks for letting me know.
-Santiago.

Re: [PATCH v6 4/6] builtin/tag: add --format argument for tag -v

[Junio C Hamano]

Jeff King  writes:

>> diff --git a/builtin/tag.c b/builtin/tag.c
>> index f81273a85a..fbb85ba3dc 100644
>> --- a/builtin/tag.c
>> +++ b/builtin/tag.c
>> @@ -66,10 +66,10 @@ static int list_tags(struct ref_filter *filter, struct 
>> ref_sorting *sorting, con
>>  }
>>  
>>  typedef int (*each_tag_name_fn)(const char *name, const char *ref,
>> -const unsigned char *sha1, void *cb_data);
>> +const unsigned char *sha1, const void *cb_data);
>
> This would bite us later if one of the iterators really does need to
> pass something mutable. But as this iteration interface is confined to
> builtin/tag.c, I think it's a nice simple fix.
>
> A more general fix would be to pass a non-const pointer to const pointer
> (preferably inside a struct for readability). But I don't see any need
> for that complexity here.

My first trial was to loosen the constness of existing variable,
which was OK, but made me feel dirty by turning what does not need
to be mutable into mutable.  The iterator being local made me try
the other way and it turned out that currently there is no need for
mutable callback data ;-)

I agree that this may have to be updated, and if this were more
global thing, we'd better off doing so from the get-go, but for a
calling convention that is limited within a single file, I am more
comfortable saying we'll cross the bridge when we need to.

Thanks.

Re: [PATCH v6 4/6] builtin/tag: add --format argument for tag -v

[Junio C Hamano]

Santiago Torres  writes:

<>???

Eric, I've noticed that this message

  http://public-inbox.org/git/20170118182831.pkhqu2np3bh2puei@LykOS.localdomain/

and all messages from Santiago appear empty when they come via
public-inbox.org; the reason I suspect we haven't heard much
complaints is because nobody else around here sends multipart/signed
disposition inline other than Santiago.

Re: [PATCH] gitk: remove translated message from comments

[Junio C Hamano]

Paul Mackerras  writes:

> On Tue, Jan 17, 2017 at 07:52:45PM -0800, David Aguilar wrote:
>> "make update-po" fails because a previously untranslated string
>> has now been translated:
>> 
>>  Updating po/sv.po
>>  po/sv.po:1388: duplicate message definition...
>>  po/sv.po:380: ...this is the location of the first definition
>> 
>> Remove the duplicate message definition.
>> 
>> Reported-by: Junio C Hamano 
>> Signed-off-by: David Aguilar 
>
> Thanks, applied.
>
> Junio, please do a pull from my repository to get this fix.
> The new head is 7f03c6e32891.

Thanks.

Re: [PATCH v6 4/6] builtin/tag: add --format argument for tag -v

[Santiago Torres]

On Wed, Jan 18, 2017 at 10:25:59AM -0800, Junio C Hamano wrote:
> Junio C Hamano  writes:
> 
> > santi...@nyu.edu writes:
> >
> >> @@ -428,9 +443,12 @@ int cmd_tag(int argc, const char **argv, const char 
> >> *prefix)
> >>if (filter.merge_commit)
> >>die(_("--merged and --no-merged option are only allowed with 
> >> -l"));
> >>if (cmdmode == 'd')
> >> -  return for_each_tag_name(argv, delete_tag);
> >> -  if (cmdmode == 'v')
> >> -  return for_each_tag_name(argv, verify_tag);
> >> +  return for_each_tag_name(argv, delete_tag, NULL);
> >> +  if (cmdmode == 'v') {
> >> +  if (format)
> >> +  verify_ref_format(format);
> >> +  return for_each_tag_name(argv, verify_tag, format);
> >> +  }
> >
> > This triggers:
> >
> > builtin/tag.c: In function 'cmd_tag':
> > builtin/tag.c:451:3: error: passing argument 3 of
> > 'for_each_tag_name' discards 'const' qualifier from pointer target type 
> > [-Werror]
> >return for_each_tag_name(argv, verify_tag, format);
> >
> > Either for-each-tag-name's new parameter needs to be typed
> > correctly, or the type of the "format" variable needs to be updated.
> 
> Squashing the following into this commit solves this issue with the
> former approach.  The lines it touches are all from 4/6 and I view
> all of it as general improvement, including type correctness and
> code formatting.

Thanks!

Should I re-roll this really quick? Or would you rather apply this on
your tree directly? 

-Santiago.


signature.asc
Description: PGP signature

Re: problem with insider build for windows and git

[Junio C Hamano]

Johannes Schindelin  writes:

> And just to prove me wrong, today I got the first update to `maint` in six
> weeks, with a message "Almost ready for 2.11.1" at its tip, featuring a
> whopping 141 commits (95 of which are not merge commits).
>
> So it seems that v2.11.1 may happen soon, after all.

Sorry for being late.  I had a short travel around the year boundary,
got sick and have been slow.

Aside from the "ouch, one topic has merged earlier iteration, that
was merged to 'master', also now merged to 'maint', and we need to
follow up on both" you sent out earlier, are there any other topic
that are already in 'master' that should go to 2.11.x track?

Thanks.

Re: [PATCH v6 4/6] builtin/tag: add --format argument for tag -v

[Jeff King]

On Wed, Jan 18, 2017 at 10:25:59AM -0800, Junio C Hamano wrote:

> > This triggers:
> >
> > builtin/tag.c: In function 'cmd_tag':
> > builtin/tag.c:451:3: error: passing argument 3 of
> > 'for_each_tag_name' discards 'const' qualifier from pointer target type 
> > [-Werror]
> >return for_each_tag_name(argv, verify_tag, format);
> >
> > Either for-each-tag-name's new parameter needs to be typed
> > correctly, or the type of the "format" variable needs to be updated.
> 
> Squashing the following into this commit solves this issue with the
> former approach.  The lines it touches are all from 4/6 and I view
> all of it as general improvement, including type correctness and
> code formatting.
> 
> diff --git a/builtin/tag.c b/builtin/tag.c
> index f81273a85a..fbb85ba3dc 100644
> --- a/builtin/tag.c
> +++ b/builtin/tag.c
> @@ -66,10 +66,10 @@ static int list_tags(struct ref_filter *filter, struct 
> ref_sorting *sorting, con
>  }
>  
>  typedef int (*each_tag_name_fn)(const char *name, const char *ref,
> - const unsigned char *sha1, void *cb_data);
> + const unsigned char *sha1, const void *cb_data);

This would bite us later if one of the iterators really does need to
pass something mutable. But as this iteration interface is confined to
builtin/tag.c, I think it's a nice simple fix.

A more general fix would be to pass a non-const pointer to const pointer
(preferably inside a struct for readability). But I don't see any need
for that complexity here.

-Peff

Re: [PATCH v6 4/6] builtin/tag: add --format argument for tag -v

[Junio C Hamano]

Junio C Hamano  writes:

> santi...@nyu.edu writes:
>
>> @@ -428,9 +443,12 @@ int cmd_tag(int argc, const char **argv, const char 
>> *prefix)
>>  if (filter.merge_commit)
>>  die(_("--merged and --no-merged option are only allowed with 
>> -l"));
>>  if (cmdmode == 'd')
>> -return for_each_tag_name(argv, delete_tag);
>> -if (cmdmode == 'v')
>> -return for_each_tag_name(argv, verify_tag);
>> +return for_each_tag_name(argv, delete_tag, NULL);
>> +if (cmdmode == 'v') {
>> +if (format)
>> +verify_ref_format(format);
>> +return for_each_tag_name(argv, verify_tag, format);
>> +}
>
> This triggers:
>
> builtin/tag.c: In function 'cmd_tag':
> builtin/tag.c:451:3: error: passing argument 3 of
> 'for_each_tag_name' discards 'const' qualifier from pointer target type 
> [-Werror]
>return for_each_tag_name(argv, verify_tag, format);
>
> Either for-each-tag-name's new parameter needs to be typed
> correctly, or the type of the "format" variable needs to be updated.

Squashing the following into this commit solves this issue with the
former approach.  The lines it touches are all from 4/6 and I view
all of it as general improvement, including type correctness and
code formatting.

diff --git a/builtin/tag.c b/builtin/tag.c
index f81273a85a..fbb85ba3dc 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -66,10 +66,10 @@ static int list_tags(struct ref_filter *filter, struct 
ref_sorting *sorting, con
 }
 
 typedef int (*each_tag_name_fn)(const char *name, const char *ref,
-   const unsigned char *sha1, void *cb_data);
+   const unsigned char *sha1, const void *cb_data);
 
 static int for_each_tag_name(const char **argv, each_tag_name_fn fn,
-   void *cb_data)
+const void *cb_data)
 {
const char **p;
char ref[PATH_MAX];
@@ -95,7 +95,7 @@ static int for_each_tag_name(const char **argv, 
each_tag_name_fn fn,
 }
 
 static int delete_tag(const char *name, const char *ref,
-   const unsigned char *sha1, void *cb_data)
+ const unsigned char *sha1, const void *cb_data)
 {
if (delete_ref(ref, sha1, 0))
return 1;
@@ -104,10 +104,10 @@ static int delete_tag(const char *name, const char *ref,
 }
 
 static int verify_tag(const char *name, const char *ref,
-   const unsigned char *sha1, void *cb_data)
+ const unsigned char *sha1, const void *cb_data)
 {
int flags;
-   char *fmt_pretty = cb_data;
+   const char *fmt_pretty = cb_data;
flags = GPG_VERIFY_VERBOSE;
 
if (fmt_pretty)

Re: problem with insider build for windows and git

[Johannes Schindelin]

Hi,

On Sat, 14 Jan 2017, Johannes Schindelin wrote:

> Footnote *2*: http://tinyurl.com/gitCal suggests that Git v2.12.0 will
> be released early February soon after Git Merge (and Git for Windows
> v2.12.0 will follow soon thereafter), and with no patches applied to the
> `maint` branch since v2.11.0, I do actually not expect any v2.11.1 to
> happen before v2.12.0 comes out.

And just to prove me wrong, today I got the first update to `maint` in six
weeks, with a message "Almost ready for 2.11.1" at its tip, featuring a
whopping 141 commits (95 of which are not merge commits).

So it seems that v2.11.1 may happen soon, after all.

Sorry for my misjudgement,
Johannes

Re: [RFC] stash --continue

[Johannes Schindelin]

Hi Marc,

On Wed, 18 Jan 2017, Marc Branchaud wrote:

> On 2017-01-16 05:54 AM, Johannes Schindelin wrote:
>
> > On Mon, 16 Jan 2017, Stephan Beyer wrote:
> >
> > > a git-newbie-ish co-worker uses git-stash sometimes. Last time he
> > > used "git stash pop", he got into a merge conflict. After he
> > > resolved the conflict, he did not know what to do to get the
> > > repository into the wanted state. In his case, it was only "git add
> > > " followed by a "git reset" and a "git stash drop",
> > > but there may be more involved cases when your index is not clean
> > > before "git stash pop" and you want to have your index as before.
> > >
> > > This led to the idea to have something like "git stash
> > > --continue"[1]
> >
> > More like "git stash pop --continue". Without the "pop" command, it
> > does not make too much sense.
> 
> Why not?  git should be able to remember what stash command created the
> conflict.  Why should I have to?  Maybe the fire alarm goes off right when I
> run the stash command, and by the time I get back to it I can't remember
> which operation I did.  It would be nice to be able to tell git to "just
> finish off (or abort) the stash operation, whatever it was".

That reeks of a big potential for confusion.

Imagine for example a total Git noob who calls `git stash list`, scrolls
two pages down, then hits `q` by mistake. How would you explain to that
user that `git stash --continue` does not continue showing the list at the
third page?

Even worse: `git stash` (without arguments) defaults to the `save`
operation, so any user who does not read the documentation (and who does?)
would assume that `git stash --continue` *also* implies `save`.

If that was not enough, there would still be the overall design of Git's
user interface. You can call it confusing, inconsistent, with a lot of
room for improvement, and you would be correct. But none of Git's commands
has a `--continue` option that remembers the latest subcommand and
continues that. To introduce that behavior in `git stash` would disimprove
the situation.

With every new feature, it is not enough to consider its benefits. You
always have to take the potential fallout into account, too.

At least `git stash pop --continue` would be consistent with all other
`--continue` options in Git that I can think of...

Ciao,
Johannes

Re: [PATCH 2/2] Be more careful when determining whether a remote was configured

[Johannes Schindelin]

Hi Peff,

On Wed, 18 Jan 2017, Jeff King wrote:

> On Wed, Jan 18, 2017 at 01:34:28PM +0100, Johannes Schindelin wrote:
> 
> > > > Let's fix this by telling Git that a remote is not configured
> > > > unless any fetch/push URL or refspect is configured explicitly.
> > > 
> > > Hmm. Old versions of GitHub for Windows used to set fetch refspecs
> > > in the system gitconfig, for a similar purpose to what you want to
> > > do with remote.origin.prune.
> > > 
> > > I notice here that setting a refspec _does_ define a remote. Is
> > > there a reason you drew the line there, and not at, say, whether it
> > > has a URL?
> > 
> > I want to err on the side of caution. That's why.
> 
> I guess I just don't see why changing the behavior with respect to
> "prune" or "proxy" is any less conservative than changing the one for
> "refspec".

Let's take a step back and consider the problem I try to solve, okay?

The problem is that `git remote rename  ` refuses to do its job
if it detects a configured ``. And it detects a configured ``
even in cases where there is not *really* any remote configured.

The example use case is to configure `remote.origin.prune = true` in
~/.gitconfig, i.e. changing Git's default for all "origin" remotes in all
of the user's repositories.

Now, the *real* fix would be to detect whether the remote was "configured"
in the current repository, or in ~/.gitconfig. But that may not even be
desirable, as we would want a more general fix for the question: can `git
remote` rename a given remote to a new name, i.e. is that new name already
taken?

And if you try to answer that last question, you will probably pick the
same set of keys for which you assume that remote.. really
configures a remote or not.

Originally, I would even have put the "vcs" into that set, as I could see
a legitimate use case for users to configure "remote.svn.vcs = vcs" in
~/.gitconfig. But the regression test suite specifically tests for that
case, and I trust that there was a good reason, even if Thomas did not
describe that good reason in the commit message nor in any reply to this
patch pair.

So how can things go wrong?

Let's take your example that the user may have specified refspecs (or
prune, or proxy) for a URL via "remote..fetch", and that `git rename`
incorrectly allows that as a new remote name. You know what? Let's do a
real code review here, not just a patch glance-over. Let's test this and
*know* whether it can be a problem:

git remote rename origin https://github.com/git/git
fatal: 'https://github.com/git/git' is not a valid remote name

A ha! It is *not* possible to hit that case because `git remote rename`
already complains much earlier about the new name not being a valid name.

So let's see what could go wrong with another example you mentioned, that
the proxy may not be used because we changed the logic of
remote_is_configured(). But the only user of the remote proxy settings is
in http_init() and reads:

if (remote && remote->http_proxy)
curl_http_proxy = xstrdup(remote->http_proxy);

if (remote)
var_override(_proxy_authmethod, 
remote->http_proxy_authmethod);

It does not even test whether the remote is configured! So maybe the
caller does? Nope, the only caller of http_init() that passes a remote is
remote-curl's cmd_main() function, and the relevant part reads:

remote = remote_get(argv[1]);

if (argc > 2) {
end_url_with_slash(, argv[2]);
} else {
end_url_with_slash(, remote->url[0]);
}

http_init(remote, url.buf, 0);

This code also does not care whether the remote "is configured" or not.

So maybe there are any other downsides with callers of
remote_is_configured()?

There is one caller in builtin/fetch.c's add_remote_or_group() which
clearly is covered (we need a URL, unless the vcs is configured).

All other callers are in builtin/remote.c:

- in add(), to test whether we can add a new remote,
- in mv(), to test whether the remote to rename is configured,
- in mv(), to test whether the new name is already taken,
- in rm(), to test whether the remote exists,
- in set_remote_branches(), to test whether the remote to be changed
  exists,
- in get_url(), to test whether the remote exists, and
- in set_url(), to test whether the remote exists.

It appears pretty obvious that in all of these cases, the suggested patch
still makes sense and does not introduce any nasty surprise.

> I can think of one alternative approach that might be easier for users
> to understand, and that we already use elsewhere (e.g., with "http.*"
> config): have a set of "default" remote keys (e.g., just "remote.key")
> that git falls back to when the remote.*.key isn't set. Then your use
> case becomes something like:
> 
>   [remote]
>   prune = true
> 
> That's not _exactly_ the same, as it applies to all remotes, not just
> "origin" (other remotes can override it, but would need to do so
> 

Re: [RFC] stash --continue

[Marc Branchaud]

On 2017-01-16 05:54 AM, Johannes Schindelin wrote:

Hi Stephan,

On Mon, 16 Jan 2017, Stephan Beyer wrote:


a git-newbie-ish co-worker uses git-stash sometimes. Last time he used
"git stash pop", he got into a merge conflict. After he resolved the
conflict, he did not know what to do to get the repository into the
wanted state. In his case, it was only "git add "
followed by a "git reset" and a "git stash drop", but there may be more
involved cases when your index is not clean before "git stash pop" and
you want to have your index as before.

This led to the idea to have something like "git stash --continue"[1]


More like "git stash pop --continue". Without the "pop" command, it does
not make too much sense.


Why not?  git should be able to remember what stash command created the 
conflict.  Why should I have to?  Maybe the fire alarm goes off right 
when I run the stash command, and by the time I get back to it I can't 
remember which operation I did.  It would be nice to be able to tell git 
to "just finish off (or abort) the stash operation, whatever it was".


M.

Re: git fast-import crashing on big imports

[Jeff King]

On Wed, Jan 18, 2017 at 03:01:17PM +0100, Ulrich Spoerlein wrote:

> Yo Jeff, your commit 8261e1f139db3f8aa6f9fd7d98c876cbeb0f927c from Aug
> 22nd, that changes delta_base_cache to use hashmap.h is the culprit for
> git fast-import crashing on large imports.

I actually saw your bug report the other day and tried to download the
dump file, but got a 404. Can you double check that it is available?

-Peff

Re: git fast-import crashing on big imports

[Ulrich Spoerlein]

Yo Jeff, your commit 8261e1f139db3f8aa6f9fd7d98c876cbeb0f927c from Aug
22nd, that changes delta_base_cache to use hashmap.h is the culprit for
git fast-import crashing on large imports.

Please read below, you can find a 55G SVN dump that should show the
problem after a couple of minutes to less than an hour. Please also see
similar issues from 2009 and 2011. This seems to be a rather fragile
part of the code, could you add unit tests that make sure this
regression is not re-introduce again once you fix it? Thanks!

I'm happy to test any patches that you can provide.

Cheers,
Uli

On Do., 2017-01-12 at 09:21:38 +0100, Ulrich Spörlein wrote:
> Hey,
> 
> the FreeBSD svn2git conversion is crashing somewhat
> non-deterministically during its long conversion process. From memory,
> this was not as bad is it is with more recent versions of git (but I
> can't be sure, really).
> 
> I have a dump file that you can grab at
> http://scan.spoerlein.net/pub/freebsd-base.dump.xz (19G, 55G uncompressed)
> that shows this problem after a couple of minutes of runtime. The caveat is
> that for another member of the team on a different machine the crashes are on
> different revisions.
> 
> Googling around I found two previous threads that were discussing
> problems just like this (memory corruption, bad caching, etc)
> 
> https://www.spinics.net/lists/git/msg93598.html  from 2009
> and
> http://git.661346.n2.nabble.com/long-fast-import-errors-out-quot-failed-to-apply-delta-quot-td6557884.html
> from 2011
> 
> % git fast-import --stats < ../freebsd-base.dump
> ...
> progress SVN r49318 branch master = :49869
> progress SVN r49319 branch stable/3 = :49870
> progress SVN r49320 branch master = :49871
> error: failed to apply delta
> error: bad offset for revindex
> error: bad offset for revindex
> error: bad offset for revindex
> error: bad offset for revindex
> error: bad offset for revindex
> fatal: Can't load tree b35ae4e9c2a41677e84a3f14bed09f584c3ff25e
> fast-import: dumping crash report to fast_import_crash_29613
> 
> 
> fast-import crash report:
> fast-import process: 29613
> parent process : 29612
> at 2017-01-11 19:33:37 +
> 
> fatal: Can't load tree b35ae4e9c2a41677e84a3f14bed09f584c3ff25e
> 
> 
> git fsck shows a somewhat incomplete pack file (I guess that's expected if the
> process dies mid-stream?)
> 
> % git fsck
> Checking object directories: 100% (256/256), done.
> error: failed to apply delta6/614500)
> error: cannot unpack d1d7ee1f81c6767c5e0f75d14d400d7512a85a0f from 
> ./objects/pack/pack-e28fcea43fc221d2ebe92857b484da58bb888237.pack at offset 
> 122654805
> error: failed to apply delta
> error: failed to read delta base object 
> d1d7ee1f81c6767c5e0f75d14d400d7512a85a0f at offset 122654805 from 
> ./objects/pack/pack-e28fcea43fc221d2ebe92857b484da58bb888237.pack
> error: cannot unpack 8523bde63ef34bef725347994fdaec996d756510 from 
> ./objects/pack/pack-e28fcea43fc221d2ebe92857b484da58bb888237.pack at offset 
> 122671596
> error: failed to apply delta0/614500)
> error: failed to read delta base object 
> d1d7ee1f81c6767c5e0f75d14d400d7512a85a0f at offset 122654805 from 
> ./objects/pack/pack-e28fcea43fc221d2ebe92857b484da58bb888237.pack
> ...
> 
> 
> Any comments on whether the original problems from 2009 and 2011 were ever
> fixed and committed?
> 
> Some more facts:
> - git version 2.11.0
> - I don't recall these sorts of crashes with a git from 2-3 years ago
> - adding more checkpoints does help, but not fix the problem, it merely shifts
>   the crashes around to different revisions
> - incremental runs of the conversion *will* complete most of the time, but
>   depending on how often checkpoints are used, I've seen it croak on specific
>   commits and not being able to progress further :(
> 
> Thanks for any pointers or things to try!
> Cheers
> Uli

Re: What's cooking in git.git (Jan 2017, #02; Sun, 15)

[Johannes Schindelin]

Hi Junio,

On Tue, 17 Jan 2017, Junio C Hamano wrote:

> Johannes Schindelin  writes:
> 
> > On Sun, 15 Jan 2017, Junio C Hamano wrote:
> >
> >> * js/prepare-sequencer-more (2017-01-09) 38 commits
> 
> > The only outstanding review comments I know about are your objection
> > to the name of the read_env_script() function (which I shot down as
> > bogus),
> 
> Not the "name", but the implementation not sharing the same code
> with "am" codepath even though they originate from the same shell
> function and serve the same purpose.

They do not originate from the same shell function at all. The scripted
git-am used a function called get_author_ident_from_commit to read the
author-script file. It also used "eval $(cat .../author-script)" to
evaluate the script later on.

And while git-rebase--interactive.sh's ". .../author-script" is very
similar to that latter invocation, I grant you that, the claim that those
codepaths originate from the same shell function is false.

Worse.

git-am.sh not only used a different method to read out the author-script,
and not only left no way for the user to modify that author-script, there
are many more differences in the exact code paths when comparing git-am.sh
to git-rebase--interactive.sh.

git-am.sh goes out of its way to not only apply the patch manually but
also to use the low-level `write-tree` and `commit-tree` commands. The
builtin am reproduces this as faithfully as possible (the author-script is
not actually evaluated, of course, but the builtin am knows that it has
to validate the author-script's contents for the first code path reading
the author-script anyway, so it reuses that function for the second code
path, knowing fully well the exact format of that file because it knows it
wrote it without any chance of the user messing it up).

The builtin am also avoids spawning the low-level commands, opting instead
to call the functions in libgit.a directly. This is all done properly, and
as a consequence, the environment variables GIT_AUTHOR_* never become,
ahem, environment variables in the builtin am, but an "author" parameter
(which is a const char * pointing to a ready-to-use ident line) that gets
passed to the commit_tree() function.

Now, let's have a brief look at git-rebase--interactive.sh.

It was rightfully nicknamed "cherry-pick on drugs" in its early days,
because that is what it does: it calls `git cherry-pick` *most* of the
time.

So what does the (sequencer via being called from the) rebase--helper do?
Yep, exactly: it calls the internal do_pick() function that is the working
horse of the cherry-pick. And does that working horse call write_tree()
and commit_tree()? No, it does not. It *spawns a git-commit process*! And
the way to specify the author there is to pass the GIT_AUTHOR_*
environment variables.

Let's recapitulate.

Not only did the scripted am use a totally different code path to *read*
the author-script than rebase -i, the code path after that also differed
substantially from rebase -i to the point that very, very different Git
commands were called.

It also so happens that the proper way to convert those code paths into
builtin code uses very, very different functions from libgit.a that
require very, very different handling.

The builtin am never sets the environment variables GIT_AUTHOR_* but
instead constructs a complete ident line from it, and therefore *must*
validate the contents of author-script.

The sequencer *has to* set the GIT_AUTHOR_* environment variables because
it calls `git commit` in a different process *that already validates the
GIT_AUTHOR_* variables*.

Even worse (and this is not the first, or second, or third time I pointed
this out): if you mistook the identical file name, and identical content,
to mean that the different code paths *must* use a *single* function to
read the author-script (nevermind that the am code path reads it into a
structure that is specifically designed to support the "am" operation, and
nothing else), you would not only force the sequencer call to provide a
data structure it does not want, not only to validate the contents
unnecessarily, but it would have to lego the parsed contents *back into
almost the original shape as the original contents of the author-script*.
So it would have to add tons of code relative to the current version, for
no benefit at all, *increasing your maintenance burden for no good
reason*.

This repeated suggestion to reuse the code path to read the author-script
repeatedly ignores the fact that we have to do very, very different things
with the contents in those two code paths.

And if that was not enough: we are talking about code that is rock solid,
has been tested for almost a *year*, half of that year with a painful
cross-validation by running old and new rebase -i and comparing their
output. And this rock solid code is what you want to force me to change to
code that neither makes sense nor is tested well.

And I refuse to be forced to 

Re: [BUG/RFC] Git Gui: GIT_DIR leaks to spawned Git Bash

[Johannes Schindelin]

Hi Max,

On Tue, 17 Jan 2017, Max Kirillov wrote:

> On Tue, Jan 17, 2017 at 11:52:29AM +0100, Johannes Schindelin wrote:
> 
> > And having said *that*, I have to admit that I was unable to reproduce
> 
> I have found exact way to reproduce it and actually the reason. Turns
> out to be obvious mistake in code. PR is in github:
> https://github.com/git-for-windows/git/pull/1032

As I pointed out in my review, I think that GIT_DIR is actually only set
for `gitk` in the non-submodule case. So I guess if I run Git GUI, spawn
gitk, and then Git Bash, it will have GIT_DIR set.

*clicketyclick*

Bingo. That reproduces the problem here.

Thanks,
Johannes

Re: [PATCH 2/2] Be more careful when determining whether a remote was configured

[Jeff King]

On Wed, Jan 18, 2017 at 01:34:28PM +0100, Johannes Schindelin wrote:

> > > Let's fix this by telling Git that a remote is not configured unless any
> > > fetch/push URL or refspect is configured explicitly.
> > 
> > Hmm. Old versions of GitHub for Windows used to set fetch refspecs in
> > the system gitconfig, for a similar purpose to what you want to do with
> > remote.origin.prune.
> > 
> > I notice here that setting a refspec _does_ define a remote. Is there a
> > reason you drew the line there, and not at, say, whether it has a URL?
> 
> I want to err on the side of caution. That's why.

I guess I just don't see why changing the behavior with respect to
"prune" or "proxy" is any less conservative than changing the one for
"refspec". Both can make some real-world cases work, and both can cause
breakage in some possible real-world cases. If we are going to change
anything, it seems like we should at least aim for a simple and
consistent rule (since users have to know which keys can be put in
~/.gitconfig and which cannot).

I can think of one alternative approach that might be easier for users
to understand, and that we already use elsewhere (e.g., with "http.*"
config): have a set of "default" remote keys (e.g., just "remote.key")
that git falls back to when the remote.*.key isn't set. Then your use
case becomes something like:

  [remote]
  prune = true

That's not _exactly_ the same, as it applies to all remotes, not just
"origin" (other remotes can override it, but would need to do so
explicitly). But I have a suspicion that may actually be what users
want.

-Peff

Re: [PATCH 5/5] describe: teach describe negative pattern matches

[Johannes Schindelin]

Hi Jake,

On Tue, 17 Jan 2017, Jacob Keller wrote:

> On Fri, Jan 13, 2017 at 1:31 PM, Johannes Sixt  wrote:
> > Am 13.01.2017 um 07:57 schrieb Jacob Keller:
> >>
> >> On Thu, Jan 12, 2017 at 10:43 PM, Johannes Sixt  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

Re: [PATCH 2/2] Be more careful when determining whether a remote was configured

[Johannes Schindelin]

Hi Junio,

On Tue, 17 Jan 2017, Junio C Hamano wrote:

> Perhaps instead of adding "is it configured?" flag that is too
> broadly named and has too narrow meaning, would it make more sense
> to introduce "int can_prune(struct remote *remote)" that looks at
> the remote refspecs?

That ("can we prune?") is not the question we need to ask when determining
whether we can rename a remote to a new name.

Ciao,
Johannes

Re: [PATCH 2/2] Be more careful when determining whether a remote was configured

[Johannes Schindelin]

Hi Peff,

On Tue, 17 Jan 2017, Jeff King wrote:

> On Tue, Jan 17, 2017 at 10:19:24PM +0100, Johannes Schindelin wrote:
> 
> > One of the really nice features of the ~/.gitconfig file is that users
> > can override defaults by their own preferred settings for all of their
> > repositories.
> > 
> > One such default that some users like to override is whether the
> > "origin" remote gets auto-pruned or not. The user would simply call
> > 
> > git config --global remote.origin.prune true
> > 
> > and from now on all "origin" remotes would be pruned automatically when
> > fetching into the local repository.
> > 
> > There is just one catch: now Git thinks that the "origin" remote is
> > configured, as it does not discern between having a remote whose
> > fetch (and/or push) URL and refspec is set, and merely having
> > preemptively-configured, global flags for specific remotes.
> > 
> > Let's fix this by telling Git that a remote is not configured unless any
> > fetch/push URL or refspect is configured explicitly.
> 
> Hmm. Old versions of GitHub for Windows used to set fetch refspecs in
> the system gitconfig, for a similar purpose to what you want to do with
> remote.origin.prune.
> 
> I notice here that setting a refspec _does_ define a remote. Is there a
> reason you drew the line there, and not at, say, whether it has a URL?

I want to err on the side of caution. That's why.

Ciao,
Johannes

Re: [PATCH v5 3/3] Retire the scripted difftool

[Johannes Schindelin]

Hi Junio,

On Tue, 17 Jan 2017, Junio C Hamano wrote:

> Johannes Schindelin  writes:
> 
> > It served its purpose, but now we have a builtin difftool. Time for the
> > Perl script to enjoy Florida.
> >
> > Signed-off-by: Johannes Schindelin 
> > ---
> 
> The endgame makes a lot of sense.  Both in the cover letter and in
> the previous patch you talk about having both in the released
> version, so do you want this step to proceed slower than the other
> two?

I did proceed that slowly. Already three Git for Windows versions have
been released with both.

But I submitted this iteration with this patch, so my intent is clearly to
retire the Perl script.

Ciao,
Johannes

[PATCH] mingw: follow-up to "replace isatty() hack"

[Johannes Schindelin]

The version of the "replace isatty() hack" that got applied to the
`maint` branch did not actually reflect the latest iteration of the
patch series: v3 was sent out with these changes, as requested by
the reviewer Johannes Sixt:

- reworded the comment about "recycling handles"

- moved the reassignment of the `console` variable before the dup2()
  call so that it is valid at all times

- removed the "handle = INVALID_HANDLE_VALUE" assignment, as the local
  variable `handle` is not used afterwards anyway

Signed-off-by: Johannes Schindelin 
---
Published-As: 
https://github.com/dscho/git/releases/tag/mingw-isatty-fixup-fixup-v1
Fetch-It-Via: git fetch https://github.com/dscho/git mingw-isatty-fixup-fixup-v1

 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).

base-commit: 3313b78c145ba9212272b5318c111cde12bfef4a
-- 
2.11.0.windows.3

Re: [PATCH v3 3/3] mingw: replace isatty() hack

[Johannes Schindelin]

Hi,

On Fri, 23 Dec 2016, Johannes Schindelin wrote:

> + /*
> +  * Use stock dup2() to re-bind fd to the new handle.  Note that
> +  * this will implicitly close(1) and close both fd=1 and the
> +  * originally associated handle.  It will open a new fd=1 and
> +  * call DuplicateHandle() on the handle associated with new_fd.
> +  * It is because of this implicit close() that we created the
> +  * copy of the original.
> +  *
> +  * 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.
> +  */
> + if (console == handle)
> + console = duplicate;
> + dup2(new_fd, fd);

Sadly, v2 was applied to `maint` instead of this revision, so Hannes'
suggestions that I addressed in v3 were dropped silently.

I will send out a follow-up patch in a moment.

Ciao,
Johannes

Re: "git diff --ignore-space-change --stat" lists files with only whitespace differences as "changed"

[Jeff King]

On Tue, Jan 17, 2017 at 09:01:55PM -0500, Matt McCutchen wrote:

> A bug report: I noticed that "git diff --ignore-space-change --stat"
> lists files with only whitespace differences as having changed with 0
> differing lines.  This is inconsistent with the behavior without --
> stat, which doesn't list such files at all.  (Same behavior with all
> the --ignore*space* flags.)  I can reproduce this with the current
> "next", af746e4.  Quick test case:

Hmm. This is pretty easy to do naively, but the special-casing for
addition/deletion (which I think we _do_ need, and which certainly we
fail t4205 without) makes me feel dirty. I'd worry there are other
cases, too (perhaps renames?). And I also notice that the
binary-diffstat code path just above my changes explicitly creates 0/0
diffstats, but I'm not even sure how one would trigger that.

So I dunno. A sensible rule to me is "iff -p would show a diff header,
then --stat should mention it". I think we'd want to somehow extract the
logic from builtin_diff() and reuse it.

---
diff --git a/diff.c b/diff.c
index e2eb6d66a..57ff5c1dc 100644
--- a/diff.c
+++ b/diff.c
@@ -2105,17 +2105,20 @@ static void show_dirstat_by_line(struct diffstat_t 
*data, struct diff_options *o
gather_dirstat(options, , changed, "", 0);
 }
 
+static void free_diffstat_file(struct diffstat_file *f)
+{
+   if (f->name != f->print_name)
+   free(f->print_name);
+   free(f->name);
+   free(f->from_name);
+   free(f);
+}
+
 static void free_diffstat_info(struct diffstat_t *diffstat)
 {
int i;
-   for (i = 0; i < diffstat->nr; i++) {
-   struct diffstat_file *f = diffstat->files[i];
-   if (f->name != f->print_name)
-   free(f->print_name);
-   free(f->name);
-   free(f->from_name);
-   free(f);
-   }
+   for (i = 0; i < diffstat->nr; i++)
+   free_diffstat_file(diffstat->files[i]);
free(diffstat->files);
 }
 
@@ -2603,6 +2606,23 @@ static void builtin_diffstat(const char *name_a, const 
char *name_b,
if (xdi_diff_outf(, , diffstat_consume, diffstat,
  , ))
die("unable to generate diffstat for %s", one->path);
+
+   /*
+* 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--;
+   }
+   }
}
 
diff_free_filespec_data(one);
diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh
index 289806d0c..2805db411 100755
--- a/t/t4015-diff-whitespace.sh
+++ b/t/t4015-diff-whitespace.sh
@@ -736,7 +736,7 @@ test_expect_success 'checkdiff allows new blank lines' '
 
 cat x "hello world" &&
git add x &&
@@ -746,6 +746,12 @@ test_expect_success 'whitespace-only changes not reported' 
'
test_cmp expect actual
 '
 
+test_expect_success 'whitespace-only changes not reported (diffstat)' '
+   # reuse state from previous test
+   git diff --stat -b >actual &&
+   test_cmp expect actual
+'
+
 cat 

Dear Friend.

[Azizi Mustafa]

Dear Friend. 

I Hoped that you will not expose or betray this trust and confident that I am 
about to repose on you for the mutual benefit of our families.I need your 
urgent assistance in transferring the sum of $5 million U.S dollars into your 
account. The money has been dormant for years in our Bank here without anybody 
coming for it.

I want to release the money to you as the nearest person to our deceased 
customer (the owner of the account) who died along with his supposed next of 
kin few years ago. I don't want the money to go into our Bank treasury account 
as unclaimed fund.

So this is the reason why I contacted you, so that we will release the money to 
you as the nearest person to the deceased customer. Please I would like you to 
keep this proposal as a top secret or delete it from your mail box, if you are 
not interested. 

Regards, 
Mr.Azizi Mustafa.

Git: new feature suggestion

[Joao Pinto]

Hello,

My name is Joao Pinto, I work at Synopsys and I am a frequent Linux Kernel
contributor.

Let me start by congratulate you for the fantastic work you have been doing with
Git which is an excellent tool.

The Linux Kernel as all systems needs to be improved and re-organized to be
better prepared for future development and sometimes we need to change
folder/files names or even move things around.
I have seen a lot of Linux developers avoid this re-organization operations
because they would lose the renamed file history, because a new log is created
for the new file, even if it is a renamed version of itself.
I am sending you this e-mail to suggest the creation of a new feature in Git:
when renamed, a file or folder should inherit his parent’s log and a “rename: …”
would be automatically created or have some kind of pointer to its “old” form to
make history analysis easier.

I volunteer to help in the new feature if you find it useful. I think it would
improve log history analysis and would enable developers to better organize old
code.

Thank you for your attention.

Best Regards,
Joao Pinto

[ANNOUNCE] git-cinnabar 0.4.0

[Mike Hommey]

Hi,

Git-cinnabar is a git remote helper to interact with mercurial
repositories. It allows to clone, pull and push from/to mercurial remote
repositories, using git.

Code on https://github.com/glandium/git-cinnabar
This release on
https://github.com/glandium/git-cinnabar/releases/tag/0.4.0

What's new since 0.3.2?

- Various bug fixes.
- Updated git to 2.11.0 for cinnabar-helper.
- Now supports bundle2 for both fetch/clone and push
  (https://www.mercurial-scm.org/wiki/BundleFormat2).
- Now supports `git credential` for HTTP authentication.
- Now supports `git push --dry-run`.
- Added a new `git cinnabar fetch` command to fetch a specific revision
  that is not necessarily a head.
- Added a new `git cinnabar download` command to download a helper on
  platforms where one is available.
- Removed upgrade path from repositories used with version < 0.3.0.
- Experimental (and partial) support for using git-cinnabar without
  having mercurial installed.
- Use a mercurial subprocess to access local mercurial repositories.
- Cinnabar-helper now handles fast-import, with workarounds for
  performance issues on macOS.
- Fixed some corner cases involving empty files. This prevented cloning
  Mozilla's stylo incubator repository.
- Fixed some correctness issues in file parenting when pushing
  changesets pulled from one mercurial repository to another.
- Various improvements to the rules to build the helper.
- Experimental (and slow) support for pushing merges, with caveats. See
  https://github.com/glandium/git-cinnabar/issues/20 for details about
  the current status.
- Fail graft earlier when no commit was found to graft
- Allow graft to work with git version < 1.9
- Allow `git cinnabar bundle` to do the same grafting as git push

Mike

Geschäftsvorschlag

[tester]

Lieber Freund.

Erlauben Sie mir, auf diese Weise auf Sie zuzugehen. Ich bin Dr. Arnold 
Kristofferson, ein US-Auftragnehmer, der mit Nichtkämpfer US Marine in 
Ba'qubah, Irak arbeitet. Ich habe die Summe von 10,6 Millionen Dollar, die ich 
aus einem Rohöl-Deal gemacht habe, und ich möchte, dass Sie mir helfen, es zu 
erhalten. Da ich hier auf offizieller Kapazität arbeite, kann ich diesen Fonds 
nicht bei mir behalten und dies ist mein einziger Grund für die Kontaktaufnahme 
mit Ihnen. Wenn Sie interessiert sind und Sie diesen Fonds in Ihrer Kapazität 
erhalten können, melden Sie sich bitte bei mir, damit ich Ihnen weitere Details 
geben kann, wie Sie dieses Geld für mich erhalten können. Bitte erreichen Sie 
mich per E-Mail: arnoldkristoferson...@gmail.com
Mit freundlichen Grüßen.