Re: [PATCH v1 2/2] log: add option to choose which refs to decorate

[Junio C Hamano]

Rafael Ascensão  writes:

> When `log --decorate` is used, git will decorate commits with all
> available refs. While in most cases this the desired effect, under some
> conditions it can lead to excessively verbose output.

Correct.

> Using `--exclude=` can help mitigate that verboseness by
> removing unnecessary 'branches' from the output. However, if the tip of
> an excluded ref points to an ancestor of a non-excluded ref, git will
> decorate it regardless.

Is this even relevant?  I think the above would only serve to
confuse the readers.  --exclude, --branches, etc. are ways to
specify what starting points "git log" history traversal should
begin and has nothing to do with what set of refs are to be used to
decorate the commits that are shown.  But the paragraph makes
readers wonder if it might have any effect in some circumstances.

> With `--decorate-refs=`, only refs that match  are
> decorated while `--decorate-refs-exclude=` allows to do the
> reverse, remove ref decorations that match 

And "Only refs that match ... are decorated" is also confusing.  The
thing is, refs are never decorated, they are used for decorating
commits in the output from "git log".  For example, if you have 

---A---B---C---D

and B is at the tip of the 'master' branch, the output from "git log
D" would decorate B with 'master', even if you do not say 'master'
on the command line as the commit to start the traversal from.

Perhaps drop the irrelevant paragraph about "--exclude" and write
something like this instead?

When "--decorate-refs=" is given, only the refs
that match the pattern is used in decoration.  The refs that
match the pattern, when "--decorate-refs-exclude="
is given, are never used in decoration.

> Both can be used together but --decorate-refs-exclude patterns have
> precedence over --decorate-refs patterns.

A reasonable and an easy-to-explain way to mix zero or more positive
and zero or more negagive patterns that follows the convention used
elsewhere in the system (e.g. how negative pathspecs work) is

 (1) if there is no positive pattern given, pretend as if an
 inclusive default positive pattern was given;

 (2) for each candidate, reject it if it matches no positive
 pattern, or if it matches any one of negative patterns.

For pathspecs, we use "everything" as the inclusive default positive
pattern, I think, and for the set of refs used for decoration, a
reasonable choice would also be to use "everything", which matches
the current behaviour.

> The pattern follows similar rules as `--glob` except it doesn't assume a
> trailing '/*' if glob characters are missing.

Why should this be a special case that burdens users to remember one
more rule?  Wouldn't users find "--decorate-refs=refs/tags" useful
and it woulld be shorter and nicer than having to say "refs/tags/*"?

> diff --git a/Documentation/git-log.txt b/Documentation/git-log.txt
> index 32246fdb0..314417d89 100644
> --- a/Documentation/git-log.txt
> +++ b/Documentation/git-log.txt
> @@ -38,6 +38,18 @@ OPTIONS
>   are shown as if 'short' were given, otherwise no ref names are
>   shown. The default option is 'short'.
>  
> +--decorate-refs=::
> + Only print ref names that match the specified pattern. Uses the same
> + rules as `git rev-list --glob` except it doesn't assume a trailing a
> + trailing '/{asterisk}' if pattern lacks '?', '{asterisk}', or '['.
> + `--decorate-refs-exlclude` has precedence.
> +
> +--decorate-refs-exclude=::
> + Do not print ref names that match the specified pattern. Uses the same
> + rules as `git rev-list --glob` except it doesn't assume a trailing a
> + trailing '/{asterisk}' if pattern lacks '?', '{asterisk}', or '['.
> + Has precedence over `--decorate-refs`.

These two may be technically correct, but I wonder if we can make it
easier to understand (I found "precedence" bit hard to follow, as in
my mind, these are ANDed conditions and between (A & ~B), there is
no "precedence").  Also we'd want to clarify what happens when only
"--decorate-refs-exclude"s are given, which in turn necessitates us
to describe what happens when only "--decorate-refs"s are given.

> diff --git a/log-tree.c b/log-tree.c
> index cea056234..8efc7ac3d 100644
> --- a/log-tree.c
> +++ b/log-tree.c
> @@ -94,9 +94,33 @@ static int add_ref_decoration(const char *refname, const 
> struct object_id *oid,
>  {
>   struct object *obj;
>   enum decoration_type type = DECORATION_NONE;
> + struct ref_include_exclude_list *filter = (struct 
> ref_include_exclude_list *)cb_data;
> + struct string_list_item *item;
> + struct strbuf real_pattern = STRBUF_INIT;
> +
> + if(filter && filter->exclude->nr > 0) {

Have SP before '('.

> + /* if current ref is on the exclude list skip */
> + for_each_string_list_item(item, filter->exclude) {
> + strbuf_reset(_pattern);
> +   

Re: [PATCH v1 1/2] refs: extract function to normalize partial refs

[Junio C Hamano]

Rafael Ascensão  writes:

> `for_each_glob_ref_in` has some code built into it that converts
> partial refs like 'heads/master' to their full qualified form

s/full// (read: that "full" needs "y" at the end).

> 'refs/heads/master'. It also assume a trailing '/*' if no glob

s/assume//

> characters are present in the pattern.
>
> Extract that logic to its own function which can be reused elsewhere
> where the same behaviour is needed, and add an ENSURE_GLOB flag
> to toggle if a trailing '/*' is to be appended to the result.
>
> Signed-off-by: Kevin Daudt 
> Signed-off-by: Rafael Ascensão 

Could you explain Kevin's sign-off we see above?  It is a bit
unusual (I am not yet saying it is wrong---I cannot judge until I
find out why it is there) to see a patch from person X with sign off
from person Y and then person X in that order.  It is normal for a
patch authored by person X to have sign-off by X and then Y if X
wrote it, signed it off and passed to Y, and then Y resent it after
signing it off (while preserving the authorship of X by adding an
in-body From: header), but I do not think that is what we have here.

It could be that you did pretty much all the work on this patch
and Kevin helped you to polish this patch off-list, in which case
the usual thing to do is to use "Helped-by: Kevin" instead.

> ---
>  refs.c | 34 --
>  refs.h | 16 
>  2 files changed, 36 insertions(+), 14 deletions(-)
>
> diff --git a/refs.c b/refs.c
> index c590a992f..1e74b48e6 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -369,32 +369,38 @@ int head_ref_namespaced(each_ref_fn fn, void *cb_data)
>   return ret;
>  }
>  
> -int for_each_glob_ref_in(each_ref_fn fn, const char *pattern,
> - const char *prefix, void *cb_data)
> +void normalize_glob_ref(struct strbuf *normalized_pattern, const char 
> *prefix,
> + const char *pattern, int flags)

It is better to use "unsigned" for a single word "flags" used as a
collection of bits.  In older parts of the codebase, we have
codepaths that pass signed int as a flags word, simply because we
didn't know better, but we do not have to spread that practice to
new code.

>  {
> - struct strbuf real_pattern = STRBUF_INIT;
> - struct ref_filter filter;
> - int ret;
> -
>   if (!prefix && !starts_with(pattern, "refs/"))
> - strbuf_addstr(_pattern, "refs/");
> + strbuf_addstr(normalized_pattern, "refs/");
>   else if (prefix)
> - strbuf_addstr(_pattern, prefix);
> - strbuf_addstr(_pattern, pattern);
> + strbuf_addstr(normalized_pattern, prefix);
> + strbuf_addstr(normalized_pattern, pattern);
>  
> - if (!has_glob_specials(pattern)) {
> + if (!has_glob_specials(pattern) && (flags & ENSURE_GLOB)) {
>   /* Append implied '/' '*' if not present. */
> - strbuf_complete(_pattern, '/');
> + strbuf_complete(normalized_pattern, '/');
>   /* No need to check for '*', there is none. */
> - strbuf_addch(_pattern, '*');
> + strbuf_addch(normalized_pattern, '*');
>   }
> +}

The above looks like a pure and regression-free code movement (plus
a small new feature) that is faithful to the original, which is good.

I however notice that addition of /* to the tail is trying to be
careful by using strbuf_complete('/'), but prefixing with "refs/"
does not and we would end up with a double-slash if pattern begins
with a slash.  The contract between the caller of this function (or
its original, which is for_each_glob_ref_in()) and the callee is
that prefix must not begin with '/', so it may be OK, but we might
want to add "if (*pattern == '/') BUG(...)" at the beginning.  

I dunno.  In any case, that is totally outside the scope of this two
patch series.

> diff --git a/refs.h b/refs.h
> index a02b628c8..9f9a8bb27 100644
> --- a/refs.h
> +++ b/refs.h
> @@ -312,6 +312,22 @@ int for_each_namespaced_ref(each_ref_fn fn, void 
> *cb_data);
>  int refs_for_each_rawref(struct ref_store *refs, each_ref_fn fn, void 
> *cb_data);
>  int for_each_rawref(each_ref_fn fn, void *cb_data);
>  
> +/*
> + * Normalizes partial refs to their full qualified form.

s/full//;

> + * If prefix is NULL, will prepend 'refs/' to the pattern if it doesn't start
> + * with 'refs/'. Results in refs/
> + *
> + * If prefix is not NULL will result in /

s/NULL/&,/;

> + *
> + * If ENSURE_GLOB is set and no glob characters are found in the
> + * pattern, a trailing <*> will be appended to the result.
> + * (<> characters to avoid breaking C comment syntax)
> + */
> +
> +#define ENSURE_GLOB 1
> +void normalize_glob_ref (struct strbuf *normalized_pattern, const char 
> *prefix,
> + const char *pattern, int flags);
> +
>  static inline const char *has_glob_specials(const char *pattern)
>  {
>   return strpbrk(pattern, "?*[");

Thanks.  Other 

Bug report: git reset --hard does not fix submodule worktree

[Billy O'Neal (VC LIBS)]

Hello, Git folks. I managed to accidentally break the our test lab by 
attempting to git mv a directory with a submodule inside. It seems like if a 
reset does an undo on a mv, the workfold entry should be fixed to put the 
submodule in its old location. Consider the following sequence of commands:

Setup a git repo with a submodule:
mkdir metaproject
mkdir upstream
cd metaproject
git init
cd ..\upstream
git init
echo hello > test.txt
git add -A
git commit -m "an example commit"
cd ..\metapoject
mkdir start
git submodule add ../upstream start/upstream
git add -A
git commit -m "Add submodule in start/upstream."

Move the directory containing the submodule:
git mv start target
git add -A
git commit -m "Moved submodule parent directory."

Check that the worktree got correctly fixed by git mv; this output is as 
expected:
type .git\modules\start\upstream\config
[core]
repositoryformatversion = 0
filemode = false
bare = false
logallrefupdates = true
symlinks = false
ignorecase = true
worktree = ../../../../target/upstream
[remote "origin"]
url = C:/Users/bion/Desktop/upstream
fetch = +refs/heads/*:refs/remotes/origin/*
[branch "master"]
remote = origin
merge = refs/heads/master

Now try to go back to the previous commit using git reset --hard:
git log --oneline
 1f560be (HEAD -> master) Moved submodule parent directory.
 a5977ce Add submodule in start/upstream.
git reset --hard a5977ce
 warning: unable to rmdir target/upstream: Directory not empty
 HEAD is now at a5977ce Add submodule in start/upstream.

Check that the worktree got fixed correctly; it did not:
type .git\modules\start\upstream\config
[core]
repositoryformatversion = 0
filemode = false
bare = false
logallrefupdates = true
symlinks = false
ignorecase = true
worktree = ../../../../target/upstream
[remote "origin"]
url = C:/Users/bion/Desktop/upstream
fetch = +refs/heads/*:refs/remotes/origin/*
[branch "master"]
remote = origin
merge = refs/heads/master

Is git reset intended to put the submodule in the right place? If not, is there 
a command we can run before/after reset to restore consistency?

Thanks folks!

Billy O'Neal

[PATCH v1 2/2] log: add option to choose which refs to decorate

[Rafael Ascensão]

When `log --decorate` is used, git will decorate commits with all
available refs. While in most cases this the desired effect, under some
conditions it can lead to excessively verbose output.

Using `--exclude=` can help mitigate that verboseness by
removing unnecessary 'branches' from the output. However, if the tip of
an excluded ref points to an ancestor of a non-excluded ref, git will
decorate it regardless.

With `--decorate-refs=`, only refs that match  are
decorated while `--decorate-refs-exclude=` allows to do the
reverse, remove ref decorations that match 

Both can be used together but --decorate-refs-exclude patterns have
precedence over --decorate-refs patterns.

The pattern follows similar rules as `--glob` except it doesn't assume a
trailing '/*' if glob characters are missing.

Both `--decorate-refs` and `--decorate-refs-exclude` can be used
multiple times.

Signed-off-by: Kevin Daudt 
Signed-off-by: Rafael Ascensão 
---
 Documentation/git-log.txt |  12 ++
 builtin/log.c |  10 -
 log-tree.c|  37 ++---
 log-tree.h|   6 ++-
 pretty.c  |   4 +-
 revision.c|   2 +-
 t/t4202-log.sh| 101 ++
 7 files changed, 162 insertions(+), 10 deletions(-)

diff --git a/Documentation/git-log.txt b/Documentation/git-log.txt
index 32246fdb0..314417d89 100644
--- a/Documentation/git-log.txt
+++ b/Documentation/git-log.txt
@@ -38,6 +38,18 @@ OPTIONS
are shown as if 'short' were given, otherwise no ref names are
shown. The default option is 'short'.
 
+--decorate-refs=::
+   Only print ref names that match the specified pattern. Uses the same
+   rules as `git rev-list --glob` except it doesn't assume a trailing a
+   trailing '/{asterisk}' if pattern lacks '?', '{asterisk}', or '['.
+   `--decorate-refs-exlclude` has precedence.
+
+--decorate-refs-exclude=::
+   Do not print ref names that match the specified pattern. Uses the same
+   rules as `git rev-list --glob` except it doesn't assume a trailing a
+   trailing '/{asterisk}' if pattern lacks '?', '{asterisk}', or '['.
+   Has precedence over `--decorate-refs`.
+
 --source::
Print out the ref name given on the command line by which each
commit was reached.
diff --git a/builtin/log.c b/builtin/log.c
index d81a09051..3587c0055 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -143,11 +143,19 @@ static void cmd_log_init_finish(int argc, const char 
**argv, const char *prefix,
struct userformat_want w;
int quiet = 0, source = 0, mailmap = 0;
static struct line_opt_callback_data line_cb = {NULL, NULL, 
STRING_LIST_INIT_DUP};
+   static struct string_list decorate_refs_exclude = STRING_LIST_INIT_DUP;
+   static struct string_list decorate_refs_include = STRING_LIST_INIT_DUP;
+   struct ref_include_exclude_list ref_filter_lists = 
{_refs_include,
+   
_refs_exclude};
 
const struct option builtin_log_options[] = {
OPT__QUIET(, N_("suppress diff output")),
OPT_BOOL(0, "source", , N_("show source")),
OPT_BOOL(0, "use-mailmap", , N_("Use mail map file")),
+   OPT_STRING_LIST(0, "decorate-refs", _refs_include,
+   N_("ref"), N_("only decorate these refs")),
+   OPT_STRING_LIST(0, "decorate-refs-exclude", 
_refs_exclude,
+   N_("ref"), N_("do not decorate these refs")),
{ OPTION_CALLBACK, 0, "decorate", NULL, NULL, N_("decorate 
options"),
  PARSE_OPT_OPTARG, decorate_callback},
OPT_CALLBACK('L', NULL, _cb, "n,m:file",
@@ -206,7 +214,7 @@ static void cmd_log_init_finish(int argc, const char 
**argv, const char *prefix,
 
if (decoration_style) {
rev->show_decorations = 1;
-   load_ref_decorations(decoration_style);
+   load_ref_decorations(decoration_style, _filter_lists);
}
 
if (rev->line_level_traverse)
diff --git a/log-tree.c b/log-tree.c
index cea056234..8efc7ac3d 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -94,9 +94,33 @@ static int add_ref_decoration(const char *refname, const 
struct object_id *oid,
 {
struct object *obj;
enum decoration_type type = DECORATION_NONE;
+   struct ref_include_exclude_list *filter = (struct 
ref_include_exclude_list *)cb_data;
+   struct string_list_item *item;
+   struct strbuf real_pattern = STRBUF_INIT;
+
+   if(filter && filter->exclude->nr > 0) {
+   /* if current ref is on the exclude list skip */
+   for_each_string_list_item(item, filter->exclude) {
+   strbuf_reset(_pattern);
+   normalize_glob_ref(_pattern, NULL, item->string, 
0);
+  

[PATCH v1 1/2] refs: extract function to normalize partial refs

[Rafael Ascensão]

`for_each_glob_ref_in` has some code built into it that converts
partial refs like 'heads/master' to their full qualified form
'refs/heads/master'. It also assume a trailing '/*' if no glob
characters are present in the pattern.

Extract that logic to its own function which can be reused elsewhere
where the same behaviour is needed, and add an ENSURE_GLOB flag
to toggle if a trailing '/*' is to be appended to the result.

Signed-off-by: Kevin Daudt 
Signed-off-by: Rafael Ascensão 
---
 refs.c | 34 --
 refs.h | 16 
 2 files changed, 36 insertions(+), 14 deletions(-)

diff --git a/refs.c b/refs.c
index c590a992f..1e74b48e6 100644
--- a/refs.c
+++ b/refs.c
@@ -369,32 +369,38 @@ int head_ref_namespaced(each_ref_fn fn, void *cb_data)
return ret;
 }
 
-int for_each_glob_ref_in(each_ref_fn fn, const char *pattern,
-   const char *prefix, void *cb_data)
+void normalize_glob_ref(struct strbuf *normalized_pattern, const char *prefix,
+   const char *pattern, int flags)
 {
-   struct strbuf real_pattern = STRBUF_INIT;
-   struct ref_filter filter;
-   int ret;
-
if (!prefix && !starts_with(pattern, "refs/"))
-   strbuf_addstr(_pattern, "refs/");
+   strbuf_addstr(normalized_pattern, "refs/");
else if (prefix)
-   strbuf_addstr(_pattern, prefix);
-   strbuf_addstr(_pattern, pattern);
+   strbuf_addstr(normalized_pattern, prefix);
+   strbuf_addstr(normalized_pattern, pattern);
 
-   if (!has_glob_specials(pattern)) {
+   if (!has_glob_specials(pattern) && (flags & ENSURE_GLOB)) {
/* Append implied '/' '*' if not present. */
-   strbuf_complete(_pattern, '/');
+   strbuf_complete(normalized_pattern, '/');
/* No need to check for '*', there is none. */
-   strbuf_addch(_pattern, '*');
+   strbuf_addch(normalized_pattern, '*');
}
+}
+
+int for_each_glob_ref_in(each_ref_fn fn, const char *pattern,
+   const char *prefix, void *cb_data)
+{
+   struct strbuf normalized_pattern = STRBUF_INIT;
+   struct ref_filter filter;
+   int ret;
+
+   normalize_glob_ref(_pattern, prefix, pattern, ENSURE_GLOB);
 
-   filter.pattern = real_pattern.buf;
+   filter.pattern = normalized_pattern.buf;
filter.fn = fn;
filter.cb_data = cb_data;
ret = for_each_ref(filter_refs, );
 
-   strbuf_release(_pattern);
+   strbuf_release(_pattern);
return ret;
 }
 
diff --git a/refs.h b/refs.h
index a02b628c8..9f9a8bb27 100644
--- a/refs.h
+++ b/refs.h
@@ -312,6 +312,22 @@ int for_each_namespaced_ref(each_ref_fn fn, void *cb_data);
 int refs_for_each_rawref(struct ref_store *refs, each_ref_fn fn, void 
*cb_data);
 int for_each_rawref(each_ref_fn fn, void *cb_data);
 
+/*
+ * Normalizes partial refs to their full qualified form.
+ * If prefix is NULL, will prepend 'refs/' to the pattern if it doesn't start
+ * with 'refs/'. Results in refs/
+ *
+ * If prefix is not NULL will result in /
+ *
+ * If ENSURE_GLOB is set and no glob characters are found in the
+ * pattern, a trailing <*> will be appended to the result.
+ * (<> characters to avoid breaking C comment syntax)
+ */
+
+#define ENSURE_GLOB 1
+void normalize_glob_ref (struct strbuf *normalized_pattern, const char *prefix,
+   const char *pattern, int flags);
+
 static inline const char *has_glob_specials(const char *pattern)
 {
return strpbrk(pattern, "?*[");
-- 
2.15.0

[PATCH v1 0/2] Add option to git log to choose which refs receive decoration

[Rafael Ascensão]

As suggested by Documentation/SubmittingPatches
Hi, this is my first patch.\n

I basically stumbled on the same issue mentioned here:
https://public-inbox.org/git/xmqqzim1pp4m@gitster.mtv.corp.google.com/

This patch implements two new command line options for `git log`:
`--decorate-refs=` and `--decorate-refs-exlcude=`

Both options accept a glob pattern which determines what decorations
commits receive.

At first I considered adding '--trim-decoration', that would filter refs
based on values passed to '--branches=' '--remotes=' '--tags=' and
'--exclude='.

After reading the email, I think it's better to have those two
behaviours decoupled.

I also had plans to add:
(Not sure if others deserve having their own command)
--decorate-branches=
--decorate-remotes=
--decorate-tags=

But was not sure if a 'niche' function like this is worth 5+ command
line options. I personally find that those two are enough.

---
Rafael Ascensão

Rafael Ascensão (2):
  refs: extract function to normalize partial refs
  log: add option to choose which refs to decorate

 Documentation/git-log.txt |  12 ++
 builtin/log.c |  10 -
 log-tree.c|  37 ++---
 log-tree.h|   6 ++-
 pretty.c  |   4 +-
 refs.c|  34 +---
 refs.h|  16 
 revision.c|   2 +-
 t/t4202-log.sh| 101 ++
 9 files changed, 198 insertions(+), 24 deletions(-)

-- 
2.15.0

Re: [PATCH] fix typos in 2.15.0 release notes

[Jonathan Nieder]

Hi,

Jean Carlo Machado wrote:

> ---
>  Documentation/RelNotes/2.15.0.txt | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)

Thanks!  Can we have your sign-off?  (See
Documentation/SubmittingPatches section 5 "Certify your work" for what
this means.)

Sincerely,
Jonathan

[PATCH] fix typos in 2.15.0 release notes

[Jean Carlo Machado]

---
 Documentation/RelNotes/2.15.0.txt | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/Documentation/RelNotes/2.15.0.txt 
b/Documentation/RelNotes/2.15.0.txt
index 248ba70c3..cdd761bcc 100644
--- a/Documentation/RelNotes/2.15.0.txt
+++ b/Documentation/RelNotes/2.15.0.txt
@@ -65,7 +65,7 @@ UI, Workflows & Features
learned to take the 'unfold' and 'only' modifiers to normalize its
output, e.g. "git log --format=%(trailers:only,unfold)".
 
- * "gitweb" shows a link to visit the 'raw' contents of blbos in the
+ * "gitweb" shows a link to visit the 'raw' contents of blobs in the
history overview page.
 
  * "[gc] rerereResolved = 5.days" used to be invalid, as the variable
@@ -109,13 +109,13 @@ Performance, Internal Implementation, Development Support 
etc.
  * Conversion from uchar[20] to struct object_id continues.
 
  * Start using selected c99 constructs in small, stable and
-   essentialpart of the system to catch people who care about
+   essential part of the system to catch people who care about
older compilers that do not grok them.
 
  * The filter-process interface learned to allow a process with long
latency give a "delayed" response.
 
- * Many uses of comparision callback function the hashmap API uses
+ * Many uses of comparison callback function the hashmap API uses
cast the callback function type when registering it to
hashmap_init(), which defeats the compile time type checking when
the callback interface changes (e.g. gaining more parameters).
-- 
2.15.0

[PATCH] credential-libsecret: unlock locked secrets

[Dennis Kaarsemaker]

Credentials exposed by the secret service DBUS interface may be locked.
Setting the SECRET_SEARCH_UNLOCK flag will make the secret service
unlock these secrets, possibly prompting the user for credentials to do
so. Without this flag, the secret is simply not loaded.

Signed-off-by: Dennis Kaarsemaker 
---
 contrib/credential/libsecret/git-credential-libsecret.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/contrib/credential/libsecret/git-credential-libsecret.c 
b/contrib/credential/libsecret/git-credential-libsecret.c
index 4c56979d8a..b4750c9ee8 100644
--- a/contrib/credential/libsecret/git-credential-libsecret.c
+++ b/contrib/credential/libsecret/git-credential-libsecret.c
@@ -104,7 +104,7 @@ static int keyring_get(struct credential *c)
items = secret_service_search_sync(service,
   SECRET_SCHEMA_COMPAT_NETWORK,
   attributes,
-  SECRET_SEARCH_LOAD_SECRETS,
+  SECRET_SEARCH_LOAD_SECRETS | 
SECRET_SEARCH_UNLOCK,
   NULL,
   );
g_hash_table_unref(attributes);
-- 
2.15.0-rc2-464-gb5de734

Re: [PATCH 04/14] fetch: add object filtering for partial fetch

[Jonathan Tan]

I did some of my own investigation and have a working (i.e. passing
tests) version of this patch here:

https://github.com/jonathantanmy/git/commits/pc20171103

If you want, you can use that, or incorporate the changes therein here.
I'll also remark on my findings inline.

On Thu,  2 Nov 2017 20:31:19 +
Jeff Hostetler  wrote:

> @@ -754,6 +757,7 @@ static int store_updated_refs(const char *raw_url, const 
> char *remote_name,
>   const char *filename = dry_run ? "/dev/null" : git_path_fetch_head();
>   int want_status;
>   int summary_width = transport_summary_width(ref_map);
> + struct check_connected_options opt = CHECK_CONNECTED_INIT;
>  
>   fp = fopen(filename, "a");
>   if (!fp)
> @@ -765,7 +769,7 @@ static int store_updated_refs(const char *raw_url, const 
> char *remote_name,
>   url = xstrdup("foreign");
>  
>   rm = ref_map;
> - if (check_connected(iterate_ref_map, , NULL)) {
> + if (check_connected(iterate_ref_map, , )) {

opt here is unchanged from CHECK_CONNECTED_INIT, so this change is unnecessary.

>   rc = error(_("%s did not send all necessary objects\n"), url);
>   goto abort;
>   }
> @@ -1044,6 +1048,9 @@ static struct transport *prepare_transport(struct 
> remote *remote, int deepen)
>   set_option(transport, TRANS_OPT_DEEPEN_RELATIVE, "yes");
>   if (update_shallow)
>   set_option(transport, TRANS_OPT_UPDATE_SHALLOW, "yes");
> + if (filter_options.choice)
> + set_option(transport, TRANS_OPT_LIST_OBJECTS_FILTER,
> +filter_options.raw_value);

You'll also need to set TRANS_OPT_FROM_PROMISOR.

> @@ -1242,6 +1249,20 @@ static int fetch_multiple(struct string_list *list)
>   int i, result = 0;
>   struct argv_array argv = ARGV_ARRAY_INIT;
>  
> + if (filter_options.choice) {
> + /*
> +  * We currently only support partial-fetches to the remote
> +  * used for the partial-clone because we only support 1
> +  * promisor remote, so we DO NOT allow explicit command
> +  * line filter arguments.
> +  *
> +  * Note that the loop below will spawn background fetches
> +  * for each remote and one of them MAY INHERIT the proper
> +  * partial-fetch settings, so everything is consistent.
> +  */
> + die(_("partial-fetch is not supported on multiple remotes"));
> + }
> +
>   if (!append && !dry_run) {
>   int errcode = truncate_fetch_head();
>   if (errcode)

My intention in doing the "fetch: refactor calculation of remote list"
patch is so that the interaction between the provided list of remotes
and the specification of the filter can be handled using the following
diff:

-   if (remote)
+   if (remote) {
+   if (filter_options.choice &&
+   strcmp(remote->name, 
repository_format_partial_clone_remote))
+   die(_("--blob-max-bytes can only be used with the 
remote configured in core.partialClone"));
result = fetch_one(remote, argc, argv);
-   else
+   } else {
+   if (filter_options.choice)
+   die(_("--blob-max-bytes can only be used with the 
remote configured in core.partialClone"));
result = fetch_multiple();
+   }

(Ignore the "blob-max-bytes" in the error message - that needs to be
updated.)

The GitHub link I provided above has this diff, and it seems to work.

Re: [PATCH 02/14] clone, fetch-pack, index-pack, transport: partial clone

[Jonathan Tan]

On Thu,  2 Nov 2017 20:31:17 +
Jeff Hostetler  wrote:

> From: Jeff Hostetler 
> 
> Signed-off-by: Jeff Hostetler 
> ---
>  builtin/clone.c  |  9 +
>  builtin/fetch-pack.c |  4 
>  builtin/index-pack.c | 10 ++
>  fetch-pack.c | 13 +
>  fetch-pack.h |  2 ++
>  transport-helper.c   |  5 +
>  transport.c  |  4 
>  transport.h  |  5 +
>  8 files changed, 52 insertions(+)

I managed to take a look at some of these patches. Firstly, consider
separating out the clone part, since it will not be tested until a few
patches later.

> diff --git a/builtin/index-pack.c b/builtin/index-pack.c
> index a0a35e6..31cd5ba 100644
> --- a/builtin/index-pack.c
> +++ b/builtin/index-pack.c
> @@ -222,6 +222,16 @@ static unsigned check_object(struct object *obj)
>   if (!(obj->flags & FLAG_CHECKED)) {
>   unsigned long size;
>   int type = sha1_object_info(obj->oid.hash, );
> +
> + if (type <= 0) {
> + /*
> +  * TODO Use the promisor code to conditionally
> +  * try to fetch this object -or- assume it is ok.
> +  */
> + obj->flags |= FLAG_CHECKED;
> + return 0;
> + }
> +
>   if (type <= 0)
>   die(_("did not receive expected object %s"),
> oid_to_hex(>oid));

This causes some repo corruption tests to fail.

If I remove this and rebase the fetch-pack tests on top [1], the tests
pass, so this might not be necessary (for now, at least).

[1] https://github.com/jonathantanmy/git/commits/pc20171103

Re: submodule using revision

[Jacob Keller]

On Fri, Nov 3, 2017 at 6:42 AM, gregory grey  wrote:
> Hi guys.
>
> Currently git submodule only works with branch of the submodule in
> question. Adding a functionality to work with a revision would be
> great from my point of view.
>
> Proposed syntax is as follows:
> git submodule add -r commit_sha git://some_repository.git
>
>
> Thanks!
>
>
> --
>
> http://ror6ax.github.io/

You can get similar behavior by doing:

git submodule add -r  
cd 
git checkout 
cd ..
git ad 
git commit

Thanks,
Jake

[PATCH] config: document blame configuration

[Stefan Beller]

The options are currently only referenced by the git-blame man page,
also explain them in git-config, which is the canonical page to
contain all config options.

Signed-off-by: Stefan Beller 
---

 Now with 'commit object name'.
 Thanks!
 
 Documentation/config.txt | 17 +
 1 file changed, 17 insertions(+)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 1ac0ae6adb..9593bfabaa 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -949,6 +949,23 @@ apply.whitespace::
Tells 'git apply' how to handle whitespaces, in the same way
as the `--whitespace` option. See linkgit:git-apply[1].
 
+blame.showRoot::
+   Do not treat root commits as boundaries in linkgit:git-blame[1].
+   This option defaults to false.
+
+blame.blankBoundary::
+   Show blank commit object name for boundary commits in
+   linkgit:git-blame[1]. This option defaults to false.
+
+blame.showEmail::
+   Show the author email instead of author name in linkgit:git-blame[1].
+   This option defaults to false.
+
+blame.date::
+   Specifies the format used to output dates in linkgit:git-blame[1].
+   If unset the iso format is used. For supported values,
+   see the discussion of the `--date` option at linkgit:git-log[1].
+
 branch.autoSetupMerge::
Tells 'git branch' and 'git checkout' to set up new branches
so that linkgit:git-pull[1] will appropriately merge from the
-- 
2.15.0.7.g980e40477f

Re: [PATCH 1/2] sequencer: factor out rewrite_file()

[Jeff King]

On Fri, Nov 03, 2017 at 10:44:08PM +0900, Junio C Hamano wrote:

> Simon Ruderich  writes:
> 
> > I tried looking into this by adding a new write_file_buf_gently()
> > (or maybe renaming write_file_buf to write_file_buf_or_die) and
> > using it from write_file_buf() but I don't know the proper way to
> > handle the error-case in write_file_buf(). Just calling
> > die("write_file_buf") feels ugly, as the real error was already
> > printed on screen by error_errno() and I didn't find any function
> > to just exit without writing a message (which still respects
> > die_routine). Suggestions welcome.
> 
> How about *not* printing the error at the place where you notice the
> error, and instead return an error code to the caller to be noticed
> which dies with an error message?

That ends up giving less-specific errors. It might be an OK tradeoff
here.

I think we've been gravitating towards error strbufs, which would make
it something like:

diff --git a/wrapper.c b/wrapper.c
index 61aba0b5c1..08eb5d1cb8 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -649,13 +649,34 @@ int xsnprintf(char *dst, size_t max, const char *fmt, ...)
return len;
 }
 
+int write_file_buf_gently(const char *path, const char *buf, size_t len,
+ struct strbuf *err)
+{
+   int fd = open(path, O_WRONLY | O_CREAT | O_TRUNC, 0666);
+   if (fd < 0) {
+   strbuf_addf(err, _("could not open '%s' for writing: %s"),
+   path, strerror(errno));
+   return -1;
+   }
+   if (write_in_full(fd, buf, len) < 0) {
+   strbuf_addf(err, _("could not write to %s: %s"),
+   path, strerror(errno));
+   close(fd);
+   return -1;
+   }
+   if (close(fd)) {
+   strbuf_addf(err, _("could not close %s: %s"),
+   path, strerror(errno));
+   return -1;
+   }
+   return 0;
+}
+
 void write_file_buf(const char *path, const char *buf, size_t len)
 {
-   int fd = xopen(path, O_WRONLY | O_CREAT | O_TRUNC, 0666);
-   if (write_in_full(fd, buf, len) < 0)
-   die_errno(_("could not write to %s"), path);
-   if (close(fd))
-   die_errno(_("could not close %s"), path);
+   struct strbuf err = STRBUF_INIT;
+   if (write_file_buf_gently(path, buf, len, ) < 0)
+   die("%s", err.buf);
 }
 
 void write_file(const char *path, const char *fmt, ...)


I'm not excited that the amount of error-handling code is now double the
amount of code that actually does something useful. Maybe this function
simply isn't large/complex enough to merit flexible error handling, and
we should simply go with René's original near-duplicate.

OTOH, if we went all-in on flexible error handling contexts, you could
imagine this function becoming:

  void write_file_buf(const char *path, const char *buf, size_t len,
  struct error_context *err)
  {
int fd = xopen(path, err, O_WRONLY | O_CREAT | O_TRUNC, 0666);
if (fd < 0)
return -1;
if (write_in_full(fd, buf, len, err) < 0)
return -1;
if (xclose(fd, err) < 0)
return -1;
return 0;
  }

Kind of gross, in that we're adding a layer on top of all system calls.
But if used consistently, it makes error-reporting a lot more pleasant,
and makes all of our "whoops, we forgot to save errno" bugs go away.

-Peff

Re: "Cannot fetch git.git" (worktrees at fault? or origin/HEAD) ?

[Stefan Beller]

On Fri, Nov 3, 2017 at 2:32 AM, Kaartic Sivaraam
 wrote:
> On Monday 23 October 2017 11:07 PM, Stefan Beller wrote:
>>
>> Exactly. By memory I mean volatile RAM (as opposed to
>> memory on a spinning disk).
>>
>> Using GIT_TEST_OPTS has had some issues (I remember vaguely
>> there was an inconsistency between the output of `make test` and prove),
>> so I put my entire working tree on a tmpfs, I run roughly this script
>> after booting my computer:
>>
>>sudo mount -t tmpfs -o size=16g tmpfs /u
>>mkdir /u/git
>>echo "gitdir:
>> /usr/local/google/home/sbeller/OSS/git/.git/worktrees/git"
>>>
>>> /u/git/.git
>>
>>git -C /u/git checkout -f HEAD
>>
>>cat >DEVELOPER=1
>>DEVELOPERS=1
>>CFLAGS += -g -O2
>>CFLAGS += -DFLEX_ARRAY=2048
>>#CFLAGS += -Wno-unused-value
>>EOF
>
>
> Did I thank you for a good explanation? If not, thanks that was interesting
> and enlightening.
>
>> The test suite (excluding t9*) runs in less than 50 seconds on the ram
>> disk.
>>

Just tested again, I meant to say 70s instead of 50s.


> BTW, this is what I call _way way_ faster. Unfortunately due to the limited
> configuration of my system, the test suite has following timing
>
> real3m14.482s
> user2m10.556s
> sys 1m12.328s

This sounds to me as if it is running with just one thread
(because sys+user = real); I usually run with

cd t
prove --jobs 25 t[0-8][0-9]*.sh

The multithreading can be seen in the timing as well
real 1m9.913s
user 1m50.796s
sys 0m54.092s
as user > real already.

If you run tests via 'make test' or 'cd t && make', you can also give
a --jobs 
to make it faster. I have no good answer for how many, but I have the impression
overloading the system is no big deal. (I only have a few cores in this machine,
4 or 6, but still run with --jobs 25; 'git grep sleep' returns a
couple of lines,
and such threads sleeping definitely don't need a CPU)

Thanks,
Stefan

Re: [PATCH 1/2] sequencer: factor out rewrite_file()

[Jeff King]

On Fri, Nov 03, 2017 at 03:46:44PM +0100, Johannes Schindelin wrote:

> > I tried looking into this by adding a new write_file_buf_gently()
> > (or maybe renaming write_file_buf to write_file_buf_or_die) and
> > using it from write_file_buf() but I don't know the proper way to
> > handle the error-case in write_file_buf(). Just calling
> > die("write_file_buf") feels ugly, as the real error was already
> > printed on screen by error_errno() and I didn't find any function
> > to just exit without writing a message (which still respects
> > die_routine). Suggestions welcome.
> 
> In my ideal world, we could use all those fancy refactoring tools that are
> currently en vogue and simply turn *all* error()/error_errno() calls into
> context-aware functions that can be told to die() right away, or to return
> the error in an error buffer, depending hwhat the caller (or the call
> chain, really) wants.
> 
> This is quite a bit more object-oriented than Git's code base, though, and
> besides, I am not aware of any refactoring tool that would make this
> painless (it's not just a matter of adding a parameter, you also have to
> pass it through all of the call chain, something you get for free when
> working with an object-oriented language).

FWIW, I sketched this out a bit here:

  
https://public-inbox.org/git/20160928085841.aoisson3fnuke...@sigill.intra.peff.net/

And you can see the patches I played with while writing that here:

  
https://github.com/peff/git/compare/cb5918aa0d50f50e83787f65c2ddc3dcb10159fe...4d61927e66dcfdbdb6cc6c88ec4018e2142e826c

(but note they don't quite compile, as some of the conversions are
half-done; it was really just to get a sense of the flavor of the
thing).

One of the complaints was that it makes it harder to see when we are
calling die() (because it's now happening via an error callback). That
maybe confusing for users, but it may also affect generated code since
the code paths that hit the NORETURN function are obscured.

But we could stop short of adding error_die, and just have error_silent,
error_warn, and error_print (and callers can turn error_print into a
die() themselves).

-Peff

Re: [PATCH 1/9] extension.partialclone: introduce partial clone extension

[Jonathan Tan]

On Fri, 3 Nov 2017 09:57:18 -0400
Jeff Hostetler  wrote:

> On 11/2/2017 6:24 PM, Jonathan Tan wrote:
> > On Thu,  2 Nov 2017 20:20:44 +
> > Jeff Hostetler  wrote:
> > 
> >> From: Jeff Hostetler 
> >>
> >> Introduce the ability to have missing objects in a repo.  This
> >> functionality is guarded by new repository extension options:
> >>  `extensions.partialcloneremote` and
> >>  `extensions.partialclonefilter`.
> > 
> > With this, it is unclear what happens if extensions.partialcloneremote
> > is not set but extensions.partialclonefilter is set. For something as
> > significant as a repository extension (which Git uses to determine if it
> > will even attempt to interact with a repo), I think - I would prefer
> > just extensions.partialclone (or extensions.partialcloneremote, though I
> > prefer the former) which determines the remote (the important part,
> > which controls the dynamic object fetching), and have another option
> > "core.partialclonefilter" which is only useful if
> > "extensions.partialclone" is set.
> 
> Yes, that is a point I wanted to ask about.  I renamed the
> extensions.partialclone that you created and then I moved your
> remote..blob-max-bytes setting to be in extensions too.
> Moving it to core.partialclonefilter is fine.

OK - in that case, it might be easier to just reuse my first patch in
its entirety. "core.partialclonefilter" is not used until the
fetching/cloning part anyway.

I agree that "core.partialclonefilter" (or another place not in
"remote") instead of "remote..blob-max-bytes" is a good idea - in
the future, we might want to reuse the same filter setting for
non-fetching functionality.

Re: [PATCH] setup: avoid double slashes when looking for HEAD

[Jeff King]

On Fri, Nov 03, 2017 at 01:58:02PM +0100, Johannes Schindelin wrote:

> From: Jeff King 
> 
> Andrew Baumann reported that when called outside of any Git worktree,
> `git rev-parse --is-inside-work-tree` eventually tries to access
> `//HEAD`, i.e.  any `HEAD` file in the root directory, but with a double
> slash.
> 
> This double slash is not only unintentional, but is allowed by the POSIX
> standard to have a special meaning. And most notably on Windows, it
> does, where it refers to a UNC path of the form `//server/share/`.
> 
> As a consequence, afore-mentioned `rev-parse` call not only looks for
> the wrong thing, but it also causes serious delays, as Windows will try
> to access a server called `HEAD`.  Let's simply avoid the unintended
> double slash.
> 
> Signed-off-by: Jeff King 
> Acked-by: Johannes Schindelin 

Thanks, this explanation looks good to me (and the patch is flawless, of
course ;) ).

-Peff

Re: [PATCH v2 0/6] Partial clone part 1: object filtering

[Jeff Hostetler]

On 11/3/2017 11:05 AM, Junio C Hamano wrote:

Jeff Hostetler  writes:


Yes, I thought we should have both (perhaps renamed or combined
into 1 parameter with value, such as --exclude=missing vs --exclude=promisor)
and let the user decide how strict they want to be.


Assuming we eventually get promisor support working, would there be
any use case where "any missing is OK" mode would be useful in a
sense more reasonable than "because we could have such a mode" and
"it is not our business to prevent users from playing with fire"?



For now, I'd like to keep my "any missing is OK" option.
I do think it has value all by itself.

We are essentially using something like that now with our GVFS
users on the gigantic Windows repo and haven't had any issues.

But yes, when we get promisor support working, we could revisit
the need for this parameter.

However, I do have some scaling concerns here.  If for example,
I have 100M missing blobs (because we did an only commits-and-trees
clone), the cost to compute "promisor missing" vs "just missing"
might be prohibitively expensive.  It could be something we want
fsck/gc to be aware of, but other commands may want to just assume
any missing objects are expected and continue.

Hopefully, we won't have a scale problem, but we just don't know
yet.

Jeff

Re: git tries to stat //HEAD when searching for a repo, leading to huge delays on Cygwin

[Jeff King]

On Fri, Nov 03, 2017 at 01:32:26PM +0100, Johannes Schindelin wrote:

> > diff --git a/setup.c b/setup.c
> > index 27a31de33f..5d0b6a88e3 100644
> > --- a/setup.c
> > +++ b/setup.c
> > @@ -283,7 +283,9 @@ int is_git_directory(const char *suspect)
> > size_t len;
> >  
> > /* Check worktree-related signatures */
> > -   strbuf_addf(, "%s/HEAD", suspect);
> > +   strbuf_addstr(, suspect);
> > +   strbuf_complete(, '/');
> > +   strbuf_addstr(, "HEAD");
> > if (validate_headref(path.buf))
> > goto done;
> 
> Yes, that would work around the issue. TBH I expected `/` to not be a
> valid bare repository path (and therefore I thought that `suspect` could
> never be just a single slash), but people do all kinds of crazy stuff, right?

Heh. People _do_ do crazy stuff, but I think here it is just the tool
double-checking if people are doing crazy stuff (which they mostly
aren't) by walking up to the root.

> I note also that there are tons of `strbuf_addstr(...);
> strbuf_complete(..., '/');` patterns in our code, as well as
> `strbuf("%s/blub", dir)`, which probably should all be condensed into
> single function calls both for semantic clarity as well as to avoid double
> slashes in the middle of paths.

Yeah, I had the same thought. This _seems_ like the kind of thing
mkpathdup() would handle, but it doesn't (and as far as I can tell never
did).

Grepping around for calls to strbuf_complete() with '/' shows that most
callers wouldn't benefit. Many do trickier things like setting up a path
ending in slash, and then appending the final element repeatedly while
iterating over a list. Some have a strbuf already and just want to
append the final element to it.

On the other hand, I suspect these are only the cases that are already
conscientious about avoiding double-slashes. There are probably a ton of
xstrfmt("%s/%s", dir, file) equivalents that just aren't careful.

> In the short run, though, let's take your patch. Maybe with a commit
> message like this?

Agreed. There are enough pitfalls to a general path-constructing helper
that I don't think we should hold up a fix while on it.

> -- snipsnap --
> setup: avoid double slashes when looking for HEAD

I see you posted this separately, so I'll review there.

Thanks for finishing it off (I had intended to circle back to it this
morning myself).

-Peff

Re: Git libsecret No Unlock Dialog Issue

[Yaroslav Sapozhnyk]

What version should include this fix? Cannot find a pr for it.

Thanks for providing the fix!

Regards,
Yaroslav

On Thu, Nov 2, 2017 at 3:48 PM, Yaroslav Sapozhnyk
 wrote:
> I've tested the code change locally and seems like it fixes the issue.
>
> Yaroslav
>
> On Thu, Nov 2, 2017 at 2:55 PM, Dennis Kaarsemaker
>  wrote:
>> On Thu, 2017-11-02 at 11:35 -0700, Stefan Beller wrote:
>>> On Thu, Nov 2, 2017 at 9:00 AM, Yaroslav Sapozhnyk
>>>  wrote:
>>> > When using Git on Fedora with locked password store
>>> > credential-libsecret asks for username/password instead of displaying
>>> > the unlock dialog.
>>>
>>> Git as packaged by Fedora or upstream Git (which version)?
>>
>> Looking at the code: current upstream git. Looking at the documentation
>> for libsecret, this should fix it. I've not been able to test it
>> though.
>>
>> diff --git a/contrib/credential/libsecret/git-credential-libsecret.c 
>> b/contrib/credential/libsecret/git-credential-libsecret.c
>> index 4c56979d8a..b4750c9ee8 100644
>> --- a/contrib/credential/libsecret/git-credential-libsecret.c
>> +++ b/contrib/credential/libsecret/git-credential-libsecret.c
>> @@ -104,7 +104,7 @@ static int keyring_get(struct credential *c)
>> items = secret_service_search_sync(service,
>>SECRET_SCHEMA_COMPAT_NETWORK,
>>attributes,
>> -  SECRET_SEARCH_LOAD_SECRETS,
>> +  SECRET_SEARCH_LOAD_SECRETS | 
>> SECRET_SEARCH_UNLOCK,
>>NULL,
>>);
>> g_hash_table_unref(attributes);
>
>
>
> --
> Regards,
> Yaroslav Sapozhnyk



-- 
Regards,
Yaroslav Sapozhnyk

Re: [PATCH] fix an 'dubious one-bit signed bitfield' error

[Jeff Hostetler]

d'oh.  thanks!

On 11/3/2017 1:05 PM, Ramsay Jones wrote:


Signed-off-by: Ramsay Jones 
---

Hi Jeff,

If you need to re-roll your 'jh/object-filtering' branch, could
you please squash this into the relevant commit (b87fd93d81,
"list-objects: filter objects in traverse_commit_list", 02-11-2017).

[This error was issued by sparse]

Thanks!

ATB,
Ramsay Jones

  list-objects-filter.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/list-objects-filter.c b/list-objects-filter.c
index 7f2842547..d9e626be8 100644
--- a/list-objects-filter.c
+++ b/list-objects-filter.c
@@ -191,7 +191,7 @@ static void *filter_blobs_limit__init(
   */
  struct frame {
int defval;
-   int child_prov_omit : 1;
+   unsigned int child_prov_omit : 1;
  };
  
  struct filter_sparse_data {

Re: git, isolation

[Stefan Beller]

On Fri, Nov 3, 2017 at 9:52 AM, Dennis Kaarsemaker
 wrote:
> On Fri, 2017-11-03 at 17:33 +0100, Péter wrote:
>> Hi,
>>
>> If I do a "git commit", issue git operations, and at the end, issue a "rm 
>> ", is there any guarantee that my
>> filesystem will be "clean",
>
> No.
>
>> i.e. not polluted or otherwise modified by some git command? Are the git 
>> operations
>> restricted to the repo-directory (and possibly remote places, over network)?
>
> No.
>
>> Do the git-directory behaves as it were
>> chroot-ed or be a sandbox? (Yet another words: is the git-directory isolated 
>> from the rest of the local filesystem (and
>> packaging system)?)
>
> And no :)
>
> Most git commands will not touch anything outside the main worktree and
> the .git directory in there, but commands like 'git worktree' can be
> used to create worktrees anywhere in the filesystem, and when you play
> tricks with the GIT_WORK_TREE environment variable, you can do other
> nasty things.

Or a more common thing, implemented earlier in Gits career:

  git config --global 

[PATCH] fix an 'dubious one-bit signed bitfield' error

[Ramsay Jones]

Signed-off-by: Ramsay Jones 
---

Hi Jeff,

If you need to re-roll your 'jh/object-filtering' branch, could
you please squash this into the relevant commit (b87fd93d81,
"list-objects: filter objects in traverse_commit_list", 02-11-2017).

[This error was issued by sparse]

Thanks!

ATB,
Ramsay Jones

 list-objects-filter.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/list-objects-filter.c b/list-objects-filter.c
index 7f2842547..d9e626be8 100644
--- a/list-objects-filter.c
+++ b/list-objects-filter.c
@@ -191,7 +191,7 @@ static void *filter_blobs_limit__init(
  */
 struct frame {
int defval;
-   int child_prov_omit : 1;
+   unsigned int child_prov_omit : 1;
 };
 
 struct filter_sparse_data {
-- 
2.15.0

Re: git, isolation

[Dennis Kaarsemaker]

On Fri, 2017-11-03 at 17:33 +0100, Péter wrote:
> Hi,
> 
> If I do a "git commit", issue git operations, and at the end, issue a "rm 
> ", is there any guarantee that my 
> filesystem will be "clean", 

No.

> i.e. not polluted or otherwise modified by some git command? Are the git 
> operations 
> restricted to the repo-directory (and possibly remote places, over network)? 

No.

> Do the git-directory behaves as it were 
> chroot-ed or be a sandbox? (Yet another words: is the git-directory isolated 
> from the rest of the local filesystem (and 
> packaging system)?)

And no :)

Most git commands will not touch anything outside the main worktree and
the .git directory in there, but commands like 'git worktree' can be
used to create worktrees anywhere in the filesystem, and when you play
tricks with the GIT_WORK_TREE environment variable, you can do other
nasty things.

D.

git, isolation

[Péter]

>If I do a "git commit"
"git clone"

git, isolation

[Péter]

Hi,

If I do a "git commit", issue git operations, and at the end, issue a "rm ", is there any guarantee that my 
filesystem will be "clean", i.e. not polluted or otherwise modified by some git command? Are the git operations 
restricted to the repo-directory (and possibly remote places, over network)? Do the git-directory behaves as it were 
chroot-ed or be a sandbox? (Yet another words: is the git-directory isolated from the rest of the local filesystem (and 
packaging system)?)



Péter

Re: Regression[2.14.3->2.15]: Interactive rebase fails if submodule is modified

[Johannes Schindelin]

Hi Orgad,

On Fri, 3 Nov 2017, Johannes Schindelin wrote:

> On Thu, 2 Nov 2017, Orgad Shaneh wrote:
> 
> > I can't reproduce this with a minimal example, but it happens in my project.

Whoa, I somehow overlooked the "can't". Sorry.

> > What I tried to do for reproducing is:
> > rm -rf super sub
> > mkdir sub; cd sub; git init
> > git commit --allow-empty -m 'Initial commit'
> > mkdir ../super; cd ../super
> > git init
> > git submodule add ../sub
> > touch foo; git add foo sub
> > git commit -m 'Initial commit'
> > touch a; git add a; git commit -m 'a'
> > touch b; git add b; git commit -m 'b'
> > cd sub; git commit --allow-empty -m 'New commit'; cd ..
> > git rebase -i HEAD^^
> > 
> > Then drop a.
> > 
> > In my project I get:
> > error: cannot rebase: You have unstaged changes.
> > 
> > This works fine with 2.14.3.
> 
> I tried to turn this into a regression test, but I cannot make it fail:
> 
> -- snip --
> diff --git a/t/t3426-rebase-submodule.sh b/t/t3426-rebase-submodule.sh
> index ebf4f5e4b2c..55aebe53191 100755
> --- a/t/t3426-rebase-submodule.sh
> +++ b/t/t3426-rebase-submodule.sh
> @@ -20,7 +20,7 @@ git_rebase () {
>   git rebase "$1"
>  }
>  
> -test_submodule_switch "git_rebase"
> +#test_submodule_switch "git_rebase"
>  
>  git_rebase_interactive () {
>   git status -su >expect &&
> @@ -38,6 +38,27 @@ git_rebase_interactive () {
>   git rebase -i "$1"
>  }
>  
> -test_submodule_switch "git_rebase_interactive"
> +#test_submodule_switch "git_rebase_interactive"
> +
> +test_expect_success '123' '
> + git init sub &&
> + test_commit -C sub init-submodule &&
> + git init super &&
> + git -C super submodule add ../sub &&
> + (
> + cd super &&
> + test_tick &&
> + touch foo &&
> + git add sub foo &&
> + git commit -m initial &&
> + test_commit a &&
> + test_commit b &&
> + test_commit -C sub new &&
> + set_fake_editor &&
> + FAKE_LINES=2 &&
> + export FAKE_LINES &&

I inserted a `git diff-files` here, and it printed exactly what I
expected:

++ git diff-files
:16 16 62cab94c8d8cf047bbb60c12def559339300efa4 
 M  sub

> + git rebase -i HEAD^^
> + )
> +'

There must be something else going wrong that we did not replicate here.
Maybe the `error: cannot rebase: You have unstaged changes.` message was
caused not by a change in the submodule? Could you run `git diff-files`
before the rebase?

This does *not* refresh the index, but maybe that is what is going wrong;
you could call `git update-index --refresh` before the rebase and see
whether that works around the issue?

Ciao,
Dscho

Re: Regression[2.14.3->2.15]: Interactive rebase fails if submodule is modified

[Johannes Schindelin]

Hi Orgad,

On Thu, 2 Nov 2017, Orgad Shaneh wrote:

> I can't reproduce this with a minimal example, but it happens in my project.
> 
> What I tried to do for reproducing is:
> rm -rf super sub
> mkdir sub; cd sub; git init
> git commit --allow-empty -m 'Initial commit'
> mkdir ../super; cd ../super
> git init
> git submodule add ../sub
> touch foo; git add foo sub
> git commit -m 'Initial commit'
> touch a; git add a; git commit -m 'a'
> touch b; git add b; git commit -m 'b'
> cd sub; git commit --allow-empty -m 'New commit'; cd ..
> git rebase -i HEAD^^
> 
> Then drop a.
> 
> In my project I get:
> error: cannot rebase: You have unstaged changes.
> 
> This works fine with 2.14.3.

I tried to turn this into a regression test, but I cannot make it fail:

-- snip --
diff --git a/t/t3426-rebase-submodule.sh b/t/t3426-rebase-submodule.sh
index ebf4f5e4b2c..55aebe53191 100755
--- a/t/t3426-rebase-submodule.sh
+++ b/t/t3426-rebase-submodule.sh
@@ -20,7 +20,7 @@ git_rebase () {
git rebase "$1"
 }
 
-test_submodule_switch "git_rebase"
+#test_submodule_switch "git_rebase"
 
 git_rebase_interactive () {
git status -su >expect &&
@@ -38,6 +38,27 @@ git_rebase_interactive () {
git rebase -i "$1"
 }
 
-test_submodule_switch "git_rebase_interactive"
+#test_submodule_switch "git_rebase_interactive"
+
+test_expect_success '123' '
+   git init sub &&
+   test_commit -C sub init-submodule &&
+   git init super &&
+   git -C super submodule add ../sub &&
+   (
+   cd super &&
+   test_tick &&
+   touch foo &&
+   git add sub foo &&
+   git commit -m initial &&
+   test_commit a &&
+   test_commit b &&
+   test_commit -C sub new &&
+   set_fake_editor &&
+   FAKE_LINES=2 &&
+   export FAKE_LINES &&
+   git rebase -i HEAD^^
+   )
+'
 
 test_done
-- snap --

Can you help me spot what I did wrong?

Ciao,
Dscho

Re: [PATCH 00/14] WIP Partial clone part 3: clone, fetch, fetch-pack, upload-pack, and tests

[Junio C Hamano]

Jeff Hostetler  writes:

> From: Jeff Hostetler 
>
> This is part 3 of 3 for partial clone.
> It assumes that part 1 [1] and part 2 [2] are in place.

Thanks.  As planned these replaced the partial clone with size
filter thing from Jonathan.  The resulting integration passed the
tests locally so I pushed it out.

By the way, the enhancement in this series made to list-objects.c
had a bit of interaction with the last round of Stefan's "describe
blob" topic when both were merged to 'pu'.  I think I resolved it
correctly, but the merge resolution can use extra sets of eyes.

diff --cc list-objects.c
index 5390a7440d,07a92f35fe..e05de01af1
--- a/list-objects.c
+++ b/list-objects.c
@@@ -220,32 -183,20 +220,22 @@@ static void add_pending_tree(struct rev
add_pending_object(revs, >object, "");
  }
  
- static void do_traverse(struct rev_info *revs,
-   show_commit_fn show_commit,
-   show_object_fn show_object,
-   void *show_data,
-   filter_object_fn filter_fn,
-   void *filter_data)
+ static void traverse_trees_and_blobs(struct rev_info *revs,
+struct strbuf *base,
+show_object_fn show_object,
 -   void *data)
++   void *show_data,
++   filter_object_fn filter_fn,
++   void *filter_data)
  {
int i;
-   struct commit *commit;
-   struct strbuf base;
  
-   strbuf_init(, PATH_MAX);
-   while ((commit = get_revision(revs)) != NULL) {
-   /*
-* an uninteresting boundary commit may not have its tree
-* parsed yet, but we are not going to show them anyway
-*/
-   if (commit->tree)
-   add_pending_tree(revs, commit->tree);
-   show_commit(commit, show_data);
-   }
 -  assert(base->len == 0);
 -
++  assert(!base->len);
for (i = 0; i < revs->pending.nr; i++) {
struct object_array_entry *pending = revs->pending.objects + i;
struct object *obj = pending->item;
const char *name = pending->name;
const char *path = pending->path;
++
if (obj->flags & (UNINTERESTING | SEEN))
continue;
if (obj->type == OBJ_TAG) {
@@@ -257,47 -208,41 +247,76 @@@
path = "";
if (obj->type == OBJ_TREE) {
process_tree(revs, (struct tree *)obj, show_object,
-, path, show_data,
 -   base, path, data);
++   base, path, show_data,
 +   filter_fn, filter_data);
continue;
}
if (obj->type == OBJ_BLOB) {
process_blob(revs, (struct blob *)obj, show_object,
-, path, show_data,
 -   base, path, data);
++   base, path, show_data,
 +   filter_fn, filter_data);
continue;
}
die("unknown pending object %s (%s)",
oid_to_hex(>oid), name);
}
object_array_clear(>pending);
-   strbuf_release();
+ }
+ 
 -void traverse_commit_list(struct rev_info *revs,
 -show_commit_fn show_commit,
 -show_object_fn show_object,
 -void *data)
++static void do_traverse(struct rev_info *revs,
++  show_commit_fn show_commit,
++  show_object_fn show_object,
++  void *show_data,
++  filter_object_fn filter_fn,
++  void *filter_data)
+ {
+   struct commit *commit;
+   struct strbuf csp; /* callee's scratch pad */
 -  strbuf_init(, PATH_MAX);
+ 
++  strbuf_init(, PATH_MAX);
+   while ((commit = get_revision(revs)) != NULL) {
+   /*
+* an uninteresting boundary commit may not have its tree
+* parsed yet, but we are not going to show them anyway
+*/
+   if (commit->tree)
+   add_pending_tree(revs, commit->tree);
 -  show_commit(commit, data);
++  show_commit(commit, show_data);
+   if (revs->tree_blobs_in_commit_order)
 -  traverse_trees_and_blobs(revs, , show_object, data);
++  traverse_trees_and_blobs(revs, ,
++   show_object, show_data,
++   

Greetings in the name of God, Business proposal in God we trust

[Mrs, Suran Yoda]

Hello Dear,

   I am DR DANIEL MMINELE THE DEPUTY GOVERNOR OF SOUTH AFRICAN RESERVE
BANK,YOU CAN VISIT THIS WEB SITE FOR YOUR
CONFIRMATION:http://www.whoswhosa.co.za/south-african-reserve-bank-20991
,

  I know my message will come to you as a surprise. Don't worry I was
totally convinced to write you, 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 sum of US$10.200,000
(Ten Million two hundred thousand Dollars US). into your account
within 10 to 14 banking days. This requires a private arrangement; the
details of the transaction will be furnished to you if you indicate
your interest in this proposal.

This money has been dormant for years in our bank without claim, I
want the bank to release the money to you as the nearest person (owner
of the account)who died along with his entire family during the Iraq
war in 2006

I don't want the money to go into our bank treasurer account as an
abandoned fund. So this is the reason why i contact you so that the
bank will release the money to you as the next of kin to the death
customer and we share the money together. Please i would like you to
keep this proposal as a top secret.

Upon receipt of your reply, I will give you full details on how the
business project will be executed and also note that you will have 40%
of the above mentioned sum.

Kindly fill this information's requested below in returns then i will
give you more details with application form for the claim.

reply at (danielmmine...@yahoo.com)

Your full name: 
Your Country: 
Phone Number: 
Your Age and occupation: 

I expect your urgent response if you agree to handle this project.

Best regard.

Dr, Daniel Mminele the deputy governor of south African reserve bank

Re: [PATCH v2 0/6] Partial clone part 1: object filtering

[Junio C Hamano]

Jeff Hostetler  writes:

> Yes, I thought we should have both (perhaps renamed or combined
> into 1 parameter with value, such as --exclude=missing vs --exclude=promisor)
> and let the user decide how strict they want to be.

Assuming we eventually get promisor support working, would there be
any use case where "any missing is OK" mode would be useful in a
sense more reasonable than "because we could have such a mode" and
"it is not our business to prevent users from playing with fire"?

Re: [PATCHv2 6/7] builtin/describe.c: describe a blob

[Junio C Hamano]

Jacob Keller  writes:

> On Thu, Nov 2, 2017 at 10:18 PM, Junio C Hamano  wrote:
>> ...
>> It is easy to imagine that we can restrict "git log" traversal with
>> a "--blobchange=" option instead of (or in addition to) the
>> limitation  gives us.  Instead of treating a commit whose
>> diff against its parent commit has any filepair that is different
>> within  as "!TREESAME", we can treat a commit whose diff
>> against its parent commit has a filepair that has the  on
>> either side of the filepair as "!TREESAME" (in other words, we
>> ignore a transition that is not about introducing or forgetting the
>>  we are looking for as an "interesting change").  That would
>> give you a commit history graph in which only (and all) such commits
>> that either adds or removes the  in it.
>>
>> Hmm?
>
> This seems quite useful in the context of figuring out how a file got
> to such a state. This is useful to me, since if I know the state of a
> file (ie: it's exact contents) I can determine the blob name, and then
> use that to lookup where it was introduced.

This is probably a bit harder than an average #leftoverbits, but if
somebody wants to get their hands dirty, it should be reasonably
straightforward.  The needed changes would roughly go like so:

 - Add "struct oid *blob_change;" to diff_options, initialized to
   NULL.

 - Teach diff_opt_parse() a new option "--blobchange=".
   Allocate a struct oid when you parse this option and point at it
   with the blob_change field above.

 - Write diffcore-blobchange.c, modeled after diffcore-pickaxe.c,
   but this should be a lot simpler (as there is no need for an
   equivalent for "pickaxe-all" option).  It would

   - prepare an empty diff_queue_struct "outq";
   - iterate over the given diff_queue "q", and 
 - a filepair p whose p->one is blob_change and p->two is not,
   (or the other way around) is added to outq with diff_q()
 - a filepair whose p->one/p->two do not involve blob_change
   is freed with diff_free_filepair().
   - replace "q" with "outq".

 - Add a call to diffcore_blobchange() early in diffcore_std(),
   probably immediately after skip-stat-unmatch, when blob_change
   field is not NULL.

 - Arrange that blob_change is propagated to revs->pruning in
   setup_revisions().

Re: [PATCH 1/2] sequencer: factor out rewrite_file()

[Johannes Schindelin]

Hi Simon,

On Fri, 3 Nov 2017, Simon Ruderich wrote:

> On Wed, Nov 01, 2017 at 06:16:18PM -0400, Jeff King wrote:
> > On Wed, Nov 01, 2017 at 10:46:14PM +0100, Johannes Schindelin wrote:
> >> I spent substantial time on making the sequencer code libified (it was far
> >> from it). That die() call may look okay now, but it is not at all okay if
> >> we want to make Git's source code cleaner and more reusable. And I want
> >> to.
> >>
> >> So my suggestion is to clean up write_file_buf() first, to stop behaving
> >> like a drunk lemming, and to return an error value already, and only then
> >> use it in sequencer.c.
> >
> > That would be fine with me, too.
> 
> I tried looking into this by adding a new write_file_buf_gently()
> (or maybe renaming write_file_buf to write_file_buf_or_die) and
> using it from write_file_buf() but I don't know the proper way to
> handle the error-case in write_file_buf(). Just calling
> die("write_file_buf") feels ugly, as the real error was already
> printed on screen by error_errno() and I didn't find any function
> to just exit without writing a message (which still respects
> die_routine). Suggestions welcome.

In my ideal world, we could use all those fancy refactoring tools that are
currently en vogue and simply turn *all* error()/error_errno() calls into
context-aware functions that can be told to die() right away, or to return
the error in an error buffer, depending hwhat the caller (or the call
chain, really) wants.

This is quite a bit more object-oriented than Git's code base, though, and
besides, I am not aware of any refactoring tool that would make this
painless (it's not just a matter of adding a parameter, you also have to
pass it through all of the call chain, something you get for free when
working with an object-oriented language).

So what I did in the sequencer when faced with the same conundrum was to
simply return -1 if the function I called returned a negative value. The
top-level builtin (in that case, `rebase--helper`) simply returns !!ret as
exit code (so that `-1` gets translated into the exit code `1`).

BTW I would not use the `_or_die()` convention, as it suggests that that
function will *always* die() in the error case. Instead, what I would
follow is the `, int die_on_error` pattern e.g. of `real_pathdup()`, and
simply *add* that parameter to the signature (and changing the return
value to `int`).

Ciao,
Dscho

Re: [PATCH 00/14] WIP Partial clone part 3: clone, fetch, fetch-pack, upload-pack, and tests

[Jeff Hostetler]

On 11/2/2017 7:41 PM, Jonathan Tan wrote:

On Thu,  2 Nov 2017 20:31:15 +
Jeff Hostetler  wrote:


From: Jeff Hostetler 

This is part 3 of 3 for partial clone.
It assumes that part 1 [1] and part 2 [2] are in place.

Part 3 is concerned with the commands: clone, fetch, upload-pack, fetch-pack,
remote-curl, index-pack, and the pack-protocol.

Jonathan and I independently started on this task.  This is a first
pass at merging those efforts.  So there are several places that need
refactoring and cleanup.  In particular, the test cases should be
squashed and new tests added.


Thanks. What are your future plans with this patch set? In particular, the
tests don't pass at HEAD^.


Patch 14/14 fixed 2 existing tests.  I think I want to merge that with
patch 2/14 as part of the cleanup.

Bigger picture, I would like squash all this down.  But first I wanted
you to see if there was anything I missed during the merge.


I took a quick glance to see if there were any issues that I could
immediately spot, but couldn't find any. I thought of fetch_if_missing,
but it seems that it is indeed used in this patch set (as expected).

I'll look at it more thorougly, and feel free to let me know if there is
anything in particular you would like comments on.



Thanks, will do.
Jeff

Re: [PATCH 1/9] extension.partialclone: introduce partial clone extension

[Jeff Hostetler]

On 11/2/2017 6:24 PM, Jonathan Tan wrote:

On Thu,  2 Nov 2017 20:20:44 +
Jeff Hostetler  wrote:


From: Jeff Hostetler 

Introduce the ability to have missing objects in a repo.  This
functionality is guarded by new repository extension options:
 `extensions.partialcloneremote` and
 `extensions.partialclonefilter`.


With this, it is unclear what happens if extensions.partialcloneremote
is not set but extensions.partialclonefilter is set. For something as
significant as a repository extension (which Git uses to determine if it
will even attempt to interact with a repo), I think - I would prefer
just extensions.partialclone (or extensions.partialcloneremote, though I
prefer the former) which determines the remote (the important part,
which controls the dynamic object fetching), and have another option
"core.partialclonefilter" which is only useful if
"extensions.partialclone" is set.


Yes, that is a point I wanted to ask about.  I renamed the
extensions.partialclone that you created and then I moved your
remote..blob-max-bytes setting to be in extensions too.
Moving it to core.partialclonefilter is fine.



I also don't think extensions.partialclonefilter (or
core.partialclonefilter, if we use my suggestion) needs to be introduced
so early in the patch set when it will only be used once we start
fetching/cloning.


+void partial_clone_utils_register(
+   const struct list_objects_filter_options *filter_options,
+   const char *remote,
+   const char *cmd_name)
+{


This function is useful once we have fetch/clone, but probably not
before that. Since the fetch/clone patches are several patches ahead,
could this be moved there?


Sure.




@@ -420,6 +420,19 @@ static int check_repo_format(const char *var, const char 
*value, void *vdata)
;
else if (!strcmp(ext, "preciousobjects"))
data->precious_objects = git_config_bool(var, value);
+
+   else if (!strcmp(ext, KEY_PARTIALCLONEREMOTE))
+   if (!value)
+   return config_error_nonbool(var);
+   else
+   data->partial_clone_remote = xstrdup(value);
+
+   else if (!strcmp(ext, KEY_PARTIALCLONEFILTER))
+   if (!value)
+   return config_error_nonbool(var);
+   else
+   data->partial_clone_filter = xstrdup(value);
+
else
string_list_append(>unknown_extensions, ext);
} else if (strcmp(var, "core.bare") == 0) {


With a complicated block, probably better to use braces in these
clauses.



Good point.

Thanks,
Jeff

Re: [PATCH 1/2] sequencer: factor out rewrite_file()

[Junio C Hamano]

Simon Ruderich  writes:

> I tried looking into this by adding a new write_file_buf_gently()
> (or maybe renaming write_file_buf to write_file_buf_or_die) and
> using it from write_file_buf() but I don't know the proper way to
> handle the error-case in write_file_buf(). Just calling
> die("write_file_buf") feels ugly, as the real error was already
> printed on screen by error_errno() and I didn't find any function
> to just exit without writing a message (which still respects
> die_routine). Suggestions welcome.

How about *not* printing the error at the place where you notice the
error, and instead return an error code to the caller to be noticed
which dies with an error message?

Re: [PATCH v2 0/6] Partial clone part 1: object filtering

[Jeff Hostetler]

On 11/2/2017 3:44 PM, Jonathan Tan wrote:

On Thu,  2 Nov 2017 17:50:07 +
Jeff Hostetler  wrote:


From: Jeff Hostetler 

Here is V2 of the list-object filtering. It replaces [1]
and reflect a refactoring and simplification of the original.


Thanks, overall this looks quite good. I reviewed patches 2-6 (skipping
1 since it's already in next), made my comments on 4, and don't have any
for the rest (besides what's below).


I've added "--filter-ignore-missing" parameter to rev-list and
pack-objects to ignore missing objects rather than error out.
This allows this patch series to better stand on its own eliminates
the need in part 1 for "patch 9" from V1.

This is a brute force ignore all missing objects.  Later, in part
2 or part 3 when --exclude-promisor-objects is introduced, we will
be able to ignore EXPECTED missing objects.


(This is regarding patches 5 and 6.) Is the intention to support both
flags? (That is, --ignore-missing to ignore without checking whether the
object being missing is not unexpected, and --exclude-promisor-objects
to check and ignore.)



Yes, I thought we should have both (perhaps renamed or combined
into 1 parameter with value, such as --exclude=missing vs --exclude=promisor)
and let the user decide how strict they want to be.

Jeff

submodule using revision

[gregory grey]

Hi guys.

Currently git submodule only works with branch of the submodule in
question. Adding a functionality to work with a revision would be
great from my point of view.

Proposed syntax is as follows:
git submodule add -r commit_sha git://some_repository.git


Thanks!


-- 

http://ror6ax.github.io/

Re: [PATCH v2 4/6] list-objects: filter objects in traverse_commit_list

[Jeff Hostetler]

On 11/3/2017 7:54 AM, Johannes Schindelin wrote:

Hi Jonathan,

On Thu, 2 Nov 2017, Jonathan Tan wrote:


On Thu,  2 Nov 2017 17:50:11 +
Jeff Hostetler  wrote:


+int parse_list_objects_filter(struct list_objects_filter_options 
*filter_options,
+ const char *arg)


Returning void is fine, I think. It seems that all your code paths
either return 0 or die.


Can we please start to encourage libified code, rather than discourage it?



I did that so that I could call it from the opt_parse_... version below
it that is used by the OPT_ macros.

And Johannes is right, it bothers me that there doesn't seem to be a hard
line where one should or should not call die() vs returning an error code.

Jeff

[PATCH] setup: avoid double slashes when looking for HEAD

[Johannes Schindelin]

From: Jeff King 

Andrew Baumann reported that when called outside of any Git worktree,
`git rev-parse --is-inside-work-tree` eventually tries to access
`//HEAD`, i.e.  any `HEAD` file in the root directory, but with a double
slash.

This double slash is not only unintentional, but is allowed by the POSIX
standard to have a special meaning. And most notably on Windows, it
does, where it refers to a UNC path of the form `//server/share/`.

As a consequence, afore-mentioned `rev-parse` call not only looks for
the wrong thing, but it also causes serious delays, as Windows will try
to access a server called `HEAD`.  Let's simply avoid the unintended
double slash.

Signed-off-by: Jeff King 
Acked-by: Johannes Schindelin 
---
Published-As: https://github.com/dscho/git/releases/tag/double-slash-HEAD-v1
Fetch-It-Via: git fetch https://github.com/dscho/git double-slash-HEAD-v1

And here it is, as a proper, easy-to-pull branch, and also in the
form Junio prefers.

 setup.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/setup.c b/setup.c
index 03f51e056cd..94768512b79 100644
--- a/setup.c
+++ b/setup.c
@@ -312,7 +312,9 @@ int is_git_directory(const char *suspect)
size_t len;
 
/* Check worktree-related signatures */
-   strbuf_addf(, "%s/HEAD", suspect);
+   strbuf_addstr(, suspect);
+   strbuf_complete(, '/');
+   strbuf_addstr(, "HEAD");
if (validate_headref(path.buf))
goto done;
 

base-commit: cb5918aa0d50f50e83787f65c2ddc3dcb10159fe
-- 
2.15.0.windows.1

Re: [PATCH] setup.c: don't try to access '//HEAD' during repo discovery

[Johannes Schindelin]

Hi Gábor,

On Fri, 3 Nov 2017, SZEDER Gábor wrote:

> Commit ce9b8aab5 (setup_git_directory_1(): avoid changing global state,
> 2017-03-13) changed how the git directory is discovered, and as a side
> effect when the discovery reaches the root directory Git tries to
> access paths like '//HEAD' and '//objects'.  This interacts badly with
> Cygwin, which interprets it as a UNC file share, and so demand-loads a
> bunch of extra DLLs and attempts to resolve/contact the server named
> HEAD.  This obviously doesn't work too well, especially over a slow
> network link.
> 
> Special case the root directory in is_git_directory() to prevent
> accessing paths with leading double slashes.

I would rather not special-case the (Unix) root directory. The underlying
problem, as Peff pointed out, is that an extra slash is appended if the
directory. And as `is_git_directory()` is not marked as static, we must
assume that there are other callers that may pass in directory paths
ending in a slash already.

Ciao,
Dscho

Re: git tries to stat //HEAD when searching for a repo, leading to huge delays on Cygwin

[Johannes Schindelin]

Hi Peff,


On Thu, 2 Nov 2017, Jeff King wrote:

> On Thu, Nov 02, 2017 at 11:45:55PM +, Andrew Baumann wrote:
> 
> > I have a workaround for this, but someone on stack overflow [1]
> > suggested reporting it upstream, so here you go:
> > 
> > I have a fancy shell prompt that executes "git rev-parse
> > --is-inside-work-tree" to determine whether we're currently inside a
> > working directory. This causes git to walk up the directory hierarchy
> > looking for a containing git repo. For example, when invoked from my
> > home directory, it stats the following paths, in order:
> > 
> > /home/me/.git
> > /home/me/.git/HEAD
> > /home/me/HEAD
> > /home
> > /home/.git
> > /home/.git/HEAD
> > /home/HEAD
> > /
> > /.git
> > /.git/HEAD
> > //HEAD
> > 
> > The last name (//HEAD) interacts badly with Cygwin, which interprets
> > it as a UNC file share, and so demand-loads a bunch of extra DLLs and
> > attempts to resolve/contact the server named HEAD. This obviously
> > doesn't work too well, especially over a slow network link.
> > 
> > I've tested with the latest Cygwin git (2.15.0); this was also present
> > in a prior version.
> 
> Interesting. I can reproduce on Linux (but of course "//HEAD" is cheap
> to look at there). It bisects to ce9b8aab5d (setup_git_directory_1():
> avoid changing global state, 2017-03-13). Before that, the end of the
> strace for "git rev-parse --git-dir" looks like:
> 
>   chdir("..") = 0
>   stat(".git", 0x7fffba398e00)= -1 ENOENT (No such file or 
> directory)
>   lstat(".git/HEAD", 0x7fffba398dd0)  = -1 ENOENT (No such file or 
> directory)
>   lstat("./HEAD", 0x7fffba398dd0) = -1 ENOENT (No such file or 
> directory)
>   write(2, "fatal: Not a git repository (or "..., 69) = 69
> 
> and after:
> 
>   stat("/.git", 0x7ffdb28b7eb0)   = -1 ENOENT (No such file or 
> directory)
>   lstat("/.git/HEAD", 0x7ffdb28b7e80) = -1 ENOENT (No such file or 
> directory)
>   lstat("//HEAD", 0x7ffdb28b7e80) = -1 ENOENT (No such file or 
> directory)
>   write(2, "fatal: Not a git repository (or "..., 69) = 69
> 
> Switching to using absolute paths rather than chdir-ing around is
> intentional for that commit, but it looks like we just need to
> special-case the construction of the root path.
> 
> Like this, perhaps:
> 
> diff --git a/setup.c b/setup.c
> index 27a31de33f..5d0b6a88e3 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -283,7 +283,9 @@ int is_git_directory(const char *suspect)
>   size_t len;
>  
>   /* Check worktree-related signatures */
> - strbuf_addf(, "%s/HEAD", suspect);
> + strbuf_addstr(, suspect);
> + strbuf_complete(, '/');
> + strbuf_addstr(, "HEAD");
>   if (validate_headref(path.buf))
>   goto done;

Yes, that would work around the issue. TBH I expected `/` to not be a
valid bare repository path (and therefore I thought that `suspect` could
never be just a single slash), but people do all kinds of crazy stuff, right?

I note also that there are tons of `strbuf_addstr(...);
strbuf_complete(..., '/');` patterns in our code, as well as
`strbuf("%s/blub", dir)`, which probably should all be condensed into
single function calls both for semantic clarity as well as to avoid double
slashes in the middle of paths.

In the short run, though, let's take your patch. Maybe with a commit
message like this?

-- snipsnap --
setup: avoid double slashes when looking for HEAD

Andrew Baumann reported that when called outside of any Git worktree, `git
rev-parse --is-inside-work-tree` eventually tries to access `//HEAD`, i.e.
any `HEAD` file in the root directory, but with a double slash.

This double slash is not only unintentional, but is allowed by the POSIX
standard to have a special meaning. And most notably on Windows, it does,
where it refers to a UNC path of the form `//server/share/`.

As a consequence, afore-mentioned `rev-parse` call not only looks for the
wrong thing, but it also causes serious delays, as Windows will try to
access a server called `HEAD`.

Let's simply avoid the unintended double slash.

Signed-off-by: Jeff King 
Acked-by: Johannes Schindelin 

Re: [PATCHv2 6/7] builtin/describe.c: describe a blob

[Johannes Schindelin]

Hi Stefan,

On Thu, 2 Nov 2017, Stefan Beller wrote:

> On Thu, Nov 2, 2017 at 12:23 AM, Andreas Schwab  wrote:
> > On Nov 01 2017, Johannes Schindelin  wrote:
> >
> >> Sure, but it is still a tricky thing. Imagine
> >>
> >> - A1 - B1 - A2 - B2 - B3
> >>
> >> where all the B* commits have the blob. Do you really want to report B1
> >> rather than B2 as the commit introducing the blob? (I would prefer B2...)
> >
> > What if B3 renames or copies the blob?
> >
> > Andreas.
> 
> With the current proposed patch you'd find B3, and then use the diff machinery
> to digg deeper from there (renames/copies ought to be easy to detect already?)
> 
> So with a copy B3 might be a better start than B1, as starting from B1 you
> would not find B3 easily.
> 
> For a rename, I would think a reverse log/blame on B1:path may help.
> 
> With that said, I think I'll just reroll the series with the current logic
> fixing the other minor issues that were brought up as B3 seems to
> be the most versatile (though not optimal) answer for many use cases.

I know this is a bit of semantics, but I disagree that B3 is the most
versatile. For my use cases, `git describe` comes in handy relatively
rarely, and usually only when I want to know what version some work was
based on. In that respect, B2 would be the most versatile answer.

However, if you say that B3 is the easiest answer to explain, I
whole-heartedly agree. Any other answer would be necessarily more
complicated to reason about, given that we're operating on a DAG, i.e. not
always on a linear history.

Ciao,
Dscho

Re: [PATCH v2 4/6] list-objects: filter objects in traverse_commit_list

[Johannes Schindelin]

Hi Jonathan,

On Thu, 2 Nov 2017, Jonathan Tan wrote:

> On Thu,  2 Nov 2017 17:50:11 +
> Jeff Hostetler  wrote:
> 
> > +int parse_list_objects_filter(struct list_objects_filter_options 
> > *filter_options,
> > + const char *arg)
> 
> Returning void is fine, I think. It seems that all your code paths
> either return 0 or die.

Can we please start to encourage libified code, rather than discourage it?

Thank you,
Dscho

Re: [PATCH 3/3] mingw: document the experimental standard handle redirection

[Johannes Schindelin]

Hi Junio,

On Fri, 3 Nov 2017, Junio C Hamano wrote:

> Johannes Schindelin  writes:
> 
> >> If I was correct in assuming that "2>&1" is just as foreign as
> >> ">/dev/null", then we should be shunning "2>&1" just like we shun
> >> ">/dev/null".  That was all I meant to say.
> >
> > Did you know that `2>&1` works in Powershell?
> 
> No.  And that makes me curious if ">&-" is also there to replace
> "off" ;-)

No, it does not:

-- snip --
PS C:\Users\me> echo 123 >&-
At line:1 char:11
+ echo 123 >&-
+   ~
Missing file specification after redirection operator.
At line:1 char:11
+ echo 123 >&-
+   ~
The ampersand (&) character is not allowed. The & operator is reserved for 
future use; wrap an ampersand in double
quotation marks ("&") to pass it as part of a string.
+ CategoryInfo  : ParserError: (:) [],
ParentContainsErrorRecordException
+ FullyQualifiedErrorId : MissingFileSpecification
-- snap --

Besides, we're really getting off-track here. I do not *like* `2>&1` as
quite cryptic placeholder for `redirect stderr into the same handle as
stdout was already redirected`. It is Perl-level obscurity.

Adding even more of those "let's string together non-alphanumerical
characters together and declare that they have some special meaning that
nobody would guess so they have to ask us and thereby make us feel smarter
than we are" is definitely not anything I want.

In my opinion, `off` is kind of a compromise that is both easy to
understand and hard to confuse.

If there was a short, succinct and easy-to-understand textual
representation of `same as stdout` that would not be easily confused for a
real file path, I would rather use that instead.

Please note, though, that I am again very reluctant to change things for
less than really compelling reasons at this stage. I have just burned two
days as a consequence of Peff's decision to take my --no-lock-index work
and turn it into something different enough that I had to put in more work
to adjust it, only to introduce a bug in something that worked without any
problem for over one entire year.

It is quite a bit ridiculous just how much bug hunting time I have to
spend lately on stuff that used to work and that got broken on transit
into git.git. It adds a whole new stress level to my work.

Ciao,
Dscho

Re: [PATCH 1/2] sequencer: factor out rewrite_file()

[Simon Ruderich]

On Wed, Nov 01, 2017 at 06:16:18PM -0400, Jeff King wrote:
> On Wed, Nov 01, 2017 at 10:46:14PM +0100, Johannes Schindelin wrote:
>> I spent substantial time on making the sequencer code libified (it was far
>> from it). That die() call may look okay now, but it is not at all okay if
>> we want to make Git's source code cleaner and more reusable. And I want
>> to.
>>
>> So my suggestion is to clean up write_file_buf() first, to stop behaving
>> like a drunk lemming, and to return an error value already, and only then
>> use it in sequencer.c.
>
> That would be fine with me, too.

I tried looking into this by adding a new write_file_buf_gently()
(or maybe renaming write_file_buf to write_file_buf_or_die) and
using it from write_file_buf() but I don't know the proper way to
handle the error-case in write_file_buf(). Just calling
die("write_file_buf") feels ugly, as the real error was already
printed on screen by error_errno() and I didn't find any function
to just exit without writing a message (which still respects
die_routine). Suggestions welcome.

Regards
Simon
-- 
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9

Re: Documentation for git-diff is very difficult to understand for a layman.

[Jacob Keller]

Hi,

On November 3, 2017 2:43:03 AM PDT, Vladimir Nikishkin  
wrote:
>Hello, honourable GIT developers.
>
>I would like to kindly ask you to do something with the git-diff
>manpage. (https://git-scm.com/docs/git-diff)
>
>In fact, I wasn't able to understand it even after reading it a few
>times.

Many parts of the man page are written from a very technical perspective. I'm 
sure patches to improve this would be welcome by the community. Even just 
reading your thoughts here is helpful, though I do not know who may find the 
interest and time to produce such.

>
>In my case, I was trying to understand, what the command actually
>prints, but the man page doesn't really tell that.
>
>1)I mean, there is a section:
>
>"https://git-scm.com/docs/git-diff#_combined_diff_format;, but at the
>very end of the manpage, so presumably, only aimed at advanced users.
>And the first thing it says is
>

The combined diff format is not a valid patch (as considered by the patch 
program) as it does not show how to convert from one file to another. Instead 
it's a way of showing the relevant differences between a resulting file and 
more than one input file, such as a merge would produce.

>"Any diff-generating command can take the -c or --cc option to produce
>a combined diff when showing a merge. This is the default format when
>showing merges with git-diff[1] ".
>

I suspect this is due to how parts of documentation are shared. The -c and -cc 
options can be given to many commands such as diff, log, show, etc

>This line is confusing, because I am already actually running git-diff
>(!). So am I really seeing a 'combined diff' or some other diff? Is
>'git-diff' any different from 'git-diff -c' or 'git-diff -cc' ?


Yes, -c will cause git to always produce a combined diff format even for non 
merge states.
>
>Could something be added to or rewritten in the manpage to clarify?

Suggestions are indeed welcome!

>
>2)Also, in point 3 of the same section:
>
>'index ,..
>mode ,..
>new file mode 
>deleted file mode ,'
>
>What do 'mode' and 'index' mean? Which values may this macros contain?
>What do two dots '..' between two hashes mean? What are this hashes
>of?
>(Same question for .)
>

These I'm not sure about, they are bits of the diff we produce to give more 
context about the exact state of the changed files.

>3)Same section, after point 4.
>
>'Unlike the traditional unified diff format'. What is the 'traditional
>unified diff'? Is it the diff produced by GNU diff, POSIX diff or
>unidiff? Or, maybe there is some other diff in other parts of GIT?

The traditional unified format is what's produced by the normal diff program 
when -U is given. This entire section is referring to extra headers we produce 
with our unified format.

>
>4)There is also a section called 'other diff formats':
>https://git-scm.com/docs/git-diff#_other_diff_formats
>
>But it doesn't actually tell anything about other diff formats, it
>just describes some other options to git-diff.
>

Each of those options refers to a different diff format.

>Maybe I am asking something trivial, but I believe, I am not the only
>novice trying to read the documentation of git. (I found the man page
>reference at the ProGIT book.)

Indeed, it's not trivial but it is important that our documentation is helpful. 
Perhaps we might want to move the more technical parts to their own section or 
to a separate page..?

>
>5)Also, there is a contradiction in the documentation.
>
>The first lines at the 'options'
>(https://git-scm.com/docs/git-diff#_options) section say:
>
>-p
>-u
>--patch
>
>Generate patch (see section on generating patches). This is the
>default.
>
>But at the 'combined diff' section:
>(https://git-scm.com/docs/git-diff#_combined_diff_format)
>
>The lines at the point 4 say:
>
>"Chunk header format is modified to prevent people from accidentally
>feeding it to patch -p1."
>


The combined format is different because it fundamentally cannot be used as a 
patch, since it's output does not provide enough context as it's meant to show 
the difference between more than 2 states. It is meant to show what changed 
with respect to multiple parent copies of a file such as what happens in a 
merge.

>So what is in reality the behaviour of git-diff? Is it to create a
>patch, or to prevent the creation of a patch?
>

Git diff is meant to show differences between states. Much of the time this is 
in the form of patches but not always.

>
>I would be happy if some of the developers could (if not simplify the
>man page), at least add some references at any manuals written in a
>more newbie-friendly way.
>
>Thank you in advance!


Suggestions are definitely welcome. I don't offhand have links to more newbie 
friendly pages. I do agree many parts of the man page are less clear if you 
don't have a background in the tool already. Maybe we need to move the 
technical format bits to their own page and keep the more daily use aspects 
easier to find...

Documentation for git-diff is very difficult to understand for a layman.

[Vladimir Nikishkin]

Hello, honourable GIT developers.

I would like to kindly ask you to do something with the git-diff
manpage. (https://git-scm.com/docs/git-diff)

In fact, I wasn't able to understand it even after reading it a few times.

In my case, I was trying to understand, what the command actually
prints, but the man page doesn't really tell that.

1)I mean, there is a section:

"https://git-scm.com/docs/git-diff#_combined_diff_format;, but at the
very end of the manpage, so presumably, only aimed at advanced users.
And the first thing it says is

"Any diff-generating command can take the -c or --cc option to produce
a combined diff when showing a merge. This is the default format when
showing merges with git-diff[1] ".

This line is confusing, because I am already actually running git-diff
(!). So am I really seeing a 'combined diff' or some other diff? Is
'git-diff' any different from 'git-diff -c' or 'git-diff -cc' ?

Could something be added to or rewritten in the manpage to clarify?

2)Also, in point 3 of the same section:

'index ,..
mode ,..
new file mode 
deleted file mode ,'

What do 'mode' and 'index' mean? Which values may this macros contain?
What do two dots '..' between two hashes mean? What are this hashes
of?
(Same question for .)

3)Same section, after point 4.

'Unlike the traditional unified diff format'. What is the 'traditional
unified diff'? Is it the diff produced by GNU diff, POSIX diff or
unidiff? Or, maybe there is some other diff in other parts of GIT?

4)There is also a section called 'other diff formats':
https://git-scm.com/docs/git-diff#_other_diff_formats

But it doesn't actually tell anything about other diff formats, it
just describes some other options to git-diff.

Maybe I am asking something trivial, but I believe, I am not the only
novice trying to read the documentation of git. (I found the man page
reference at the ProGIT book.)

5)Also, there is a contradiction in the documentation.

The first lines at the 'options'
(https://git-scm.com/docs/git-diff#_options) section say:

-p
-u
--patch

Generate patch (see section on generating patches). This is the default.

But at the 'combined diff' section:
(https://git-scm.com/docs/git-diff#_combined_diff_format)

The lines at the point 4 say:

"Chunk header format is modified to prevent people from accidentally
feeding it to patch -p1."

So what is in reality the behaviour of git-diff? Is it to create a
patch, or to prevent the creation of a patch?


I would be happy if some of the developers could (if not simplify the
man page), at least add some references at any manuals written in a
more newbie-friendly way.

Thank you in advance!


-- 
Yours sincerely, Vladimir Nikishkin

Re: "Cannot fetch git.git" (worktrees at fault? or origin/HEAD) ?

[Kaartic Sivaraam]

On Monday 23 October 2017 11:07 PM, Stefan Beller wrote:

Exactly. By memory I mean volatile RAM (as opposed to
memory on a spinning disk).

Using GIT_TEST_OPTS has had some issues (I remember vaguely
there was an inconsistency between the output of `make test` and prove),
so I put my entire working tree on a tmpfs, I run roughly this script
after booting my computer:

   sudo mount -t tmpfs -o size=16g tmpfs /u
   mkdir /u/git
   echo "gitdir:
/usr/local/google/home/sbeller/OSS/git/.git/worktrees/git"

/u/git/.git

   git -C /u/git checkout -f HEAD

   cat 

Re: [BUG] Incosistent repository state when trying to rename HEAD in the middle of a rebase

[Kaartic Sivaraam]

On Thursday 02 November 2017 01:21 PM, Junio C Hamano wrote:

Kaartic Sivaraam  writes:


I was able to spare some time to dig into this and found a few things.

First, it seems that the issue is more generic and the BUG kicks in
whenever HEAD is not a symbolic ref.


Interesting.



Let me detail a little more about my observations just for the sake of 
completeness. The change that forbid refs/heads/HEAD caused issues only 
when HEAD wasn't a symbolic link because of the following reasons,


1) The change resulted in 'strbuf_check_branch_ref' returning with 
failure when the name it received (sb) was HEAD *without* interpreting 
it as "refs/heads/HEAD" into 'ref'. This resulted in the violation of 
the expectation of it's callers that it would have interpret 'ref' which 
was the major cause of the issue.


It wouldn't have been an issue if we had checked for the existence of a 
"branch" (refs/heads/) rather than checking for the existence of a "ref" 
(which allowed HEAD to pass the test).



2) This did not cause issues when HEAD was a symbolic ref because there 
was a check for attempting to rename in a symbolic ref in 
'files_copy_or_rename_ref'. The check throws an error when trying to 
rename a symbolic ref which resulted in suspicious error message I got.


So, IIUC the issue doesn't occur when 'ref' is intrepreted before the 
check for 'HEAD'. That's possibly why the diff patch I sent in the 
previous mail fixed the issue.


Also, it would be nice if we check for the existence of a "branch" when 
we want to know whether a branch exists rather than simply doing a 
'ref_exists' on the interpreted ref.




Shortly we'll be rewinding the tip of 'next', and it is a good
opportunity to kick any not-so-well-cooked topic back to 'pu',
so let's figure out what is going on after that happens (at which
point let's eject the "branch name sanity" topic out of 'next').


Does the above explanation give an idea about what's happening?


---
Kaartic

Re: [PATCHv2 6/7] builtin/describe.c: describe a blob

[Jacob Keller]

On Thu, Nov 2, 2017 at 10:18 PM, Junio C Hamano  wrote:
> Jacob Keller  writes:
>
>> I think both questions are valuable, the first is "which commit last
>> had this blob", the second  is "which commit first introduced this
>> blob", neither of which can always give a definitive answer. It really
>> depends on what question you're asking up front.
>
> Given that "describe" is about giving an object _a_ name that is
> plausibly useful to refer to it, it is not a good match for the
> above query that wants to know where it came from, how long it
> remains in the tree and where it ceases to be relevant.  In order to
> support that use case, a totally different and possibly more useful
> avenue would be to think how this can be hooked into "git log"
> machinery.
>
> A refresher for how "git log [--options] " works may be
> beneficial.  We walk history and compare the tree of the commit we
> are looking at with the tree of its parent commits.  If everything
> within  is the same, we mark the transition between the
> parent and our commit TREESAME (other commits, i.e. the ones that
> have meaningful change within , are !TREESAME).  Then the
> output routine presents the set of commits that includes commits
> that are !TREESAME, within the constraints of the --options given
> (e.g. with --full-history, sides of a merge that is TREESAME may
> still be shown to preserve connectedness of the resulting graph).
>
> It is easy to imagine that we can restrict "git log" traversal with
> a "--blobchange=" option instead of (or in addition to) the
> limitation  gives us.  Instead of treating a commit whose
> diff against its parent commit has any filepair that is different
> within  as "!TREESAME", we can treat a commit whose diff
> against its parent commit has a filepair that has the  on
> either side of the filepair as "!TREESAME" (in other words, we
> ignore a transition that is not about introducing or forgetting the
>  we are looking for as an "interesting change").  That would
> give you a commit history graph in which only (and all) such commits
> that either adds or removes the  in it.
>
> Hmm?

This seems quite useful in the context of figuring out how a file got
to such a state. This is useful to me, since if I know the state of a
file (ie: it's exact contents) I can determine the blob name, and then
use that to lookup where it was introduced.

Thanks,
Jake