Re: [PATCH v3] config: make git_config_set die on failure

[Junio C Hamano]

Patrick Steinhardt  writes:

> Furthermore, I do think it's more explicit what the functions are
> doing when there is a 'or_die' suffix. Without this suffix it may
> be unexpected that the functions simply abort the program
> whenever an error occurs.

That largely depends on what you are used to see.  Many internal API
functions do "if we cannot do this, die" without or_die suffix.

As the endgame state, I'd find it far more preferrable to see a
function that dies without _or_die(), while allowing outlier callers
to call the _gently() variant to do their own clean-up [*1*].

How we get to that endgame is a different matter.  It is a viable
approach like you did in the original series to first temporarily
introduce _or_die() and convert the current callers in small chunks.
The second step would be to rename git_config_set() and friends that
do not die to hae _gently() suffix, and update the remaining callers
to call them.  At that point, nobody calls git_config_set(), so we
can drop the _or_die() suffix, which would lead us to the endgame
state.

But because we are talking about only a very small number of
callers, I think either is OK.

Thanks.


[Footnote]

*1* Another consideration while talking about a transition like this
is what would happen in topics in flight (either in my tree
above 'next' or people privately are working on).  New callsites
they added expecting git_config_set() to return error would have
to be adjusted to call _gently() variants, and we need to catch
such sites somehow.  In this partcular transision, it can easily
be done and you've done so already by making the functions
return no value, so anybody who checked their return values
would be flagged by the compiler.

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] completion: verify-tag is not plumbing

[Junio C Hamano]

John Keeping  writes:

> I can accept that argument about verify-commit and verify-tag, but
> listing verify-tag as plumbing is incorrect according to
> command-list.txt (and thus git(1)).  If we're going to classify
> commands, shouldn't we be consistent in how we do so?

These are not meant to be "classifications", but "justifications".
When somebody asks "why isn't this command tab-completed?", you can
find the explanation e.g. "because it is rarely used".

A command being 'plumbing' does not have to make it automatically
ineligible from getting tab-completed.  For some small tasks,
running a plumbing command may be the easiest way to achieve them in
the interactive session, and it helps to have tab-completion for
such a plumbing command (e.g. "git apply" is completed, IIRC).

Also often the line between plumbing and Porcelain is somewhat
blurry.  I'd consider ancillaryX categories in command-list.txt a
cop-out myself.

In this particular case, saying "better use 'tag --verify'" there
instead of "plumbing" may be more helpful for those who are reading
this script.

>> > Signed-off-by: John Keeping 
>> > ---
>> >  contrib/completion/git-completion.bash | 1 -
>> >  1 file changed, 1 deletion(-)
>> >
>> > diff --git a/contrib/completion/git-completion.bash
>> > b/contrib/completion/git-completion.bash
>> > index 51f5223..250788a 100644
>> > --- a/contrib/completion/git-completion.bash
>> > +++ b/contrib/completion/git-completion.bash
>> > @@ -728,7 +728,6 @@ __git_list_porcelain_commands ()
>> >write-tree)   : plumbing;;
>> >var)  : infrequent;;
>> >verify-pack)  : infrequent;;
>> > -  verify-tag)   : plumbing;;
>> >*) echo $i;;
>> >esac
>> >done
>> > --
>> > 2.7.0
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Plans for 2.7.1?

[Junio C Hamano]

Johannes Schindelin  writes:

> at tinyurl.com/gitCal I see a pretty timeline regarding 2.8.0, but I do
> not see 2.7.1 planned anywhere.

Yup, because maintenance releases are inherently "not planned" ;-)

Unlike feature releases that are largely time-based, we cut
maintenance releases only when we need to push out fixes.  We do not
even know in advance what breakages we would have in 2.7.0, or what
fixes we would apply for them, and when these fixes would happen.

> Due to signature problems (I failed to realize that SHA-1 based
> .exe signatures are no longer considered valid starting from
> January 1st, 2016), I got a metric ton of private and non-private
> bug reports regarding "corrupt signatures", and therefore I would
> like to prevent those reports from taking over my entire working
> hours by simply issuing a new release of Git for Windows.
>
> Is 2.7.1 around the corner? Otherwise I'll just make a 2.7.0(2).

Let me see what are slated for 'maint' in the current draft release
notes.  This actually lists what could technically be merged to
'maint'; some clean-up patches may not be worth merging down.

$ Meta/ML http://vger.kernel.org/majordomo-info.html

Re: [PATCH] travis-ci: run previously failed tests first, then slowest to fastest

[Clemens Buchacher]

On Mon, Feb 01, 2016 at 10:17:24AM -0800, Junio C Hamano wrote:
> 
> Your proposal is to redefine "is the working tree dirty?"; it would
> check if "git checkout -f" would change what is in the working tree.

I like this definition. Sounds obviously right.

> > Regarding performance impact: We only need to do this extra check if the
> > usual check convert_to_git(work tree) against index state fails, and
> > conversion is in effect.
> 
> How would you detect the failure, though?  Having contents in the
> index that contradicts the attributes and eol settings affects the
> cleanliness both ways.  Running the working tree contents via to-git
> conversion and hashing may match the blob in the index, declaring
> that the index entry is "clean", but passing the blob to to-worktree
> conversion may produce result different from what is in the
> worktree, which is "falsely clean".

True. But this is what we do today, and I thought at first that we have
to keep this behavior. The following enables eol conversion on git add,
but not on checkout:

 printf 'line 1\r\n' >dos.txt
 echo '* text' >.gitattributes
 git add dos.txt
 git commit

After git add the worktree is considered clean, even though dos.txt
still has CRLF line endings, and rm dos.txt && git checkout dos.txt
re-creates dos.txt with LF line endings. If we change the definition as
proposed above, then the worktree would be dirty even though we just
did git add and git commit.

So I concluded that we have to treat the worktree clean if either git
add -u does not change the index state, _or_ git checkout -f does not
change the worktree state.

But doing only the git checkout -f check makes much more sense. Maybe we
can handle the above situation better by doing an implicit
git checkout -f  after git commit. After all, I would
expect git commit to give me exactly the same state that I get later
when I do git checkout  for the same commit.

> > On the other hand, a user who simply follows an upstream repository by
> > doing git pull all the time, and who does not make changes to their
> > configuration, can still run into this issue, because upstream could
> > change .gitattributes. This part we could actually detect by hashing the
> > attributes for each index entry, and if that changes we re-evaluate the
> > file state.
> 
> If this has to bloat each index entry, I do not think solving the
> problem is worth that cost of that overhead.  I'd rather just say
> "if you have inconsistent data, here is a workaround using 'reset'
> and then 'reset --hard'" and be done with it.

Works for me.

> > This is also an issue only if a smudge filter is in place. The eol
> > conversion which only acts in the convert_to_git direction is not
> > affected.
> 
> IIRC, autocrlf=true would strip CR at the end of line in to-git
> conversion, and would add CR in to-worktree conversion.  So some eol
> conversion may only act in to-git, but some others do affect both,
> and without needing you to touch attributes.

I was somehow under the impression that autocrlf=true is discouraged,
and setting the text attribute to true is the new recommended way to
configure eol conversion. But I see that the Git for Windows installer
still offers autocrlf=true as the default option, so clearly we need to
support it well.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC/PATCH 0/5] [WAS: Submodule Groups] Labels and submodule.autoInitialize

[Stefan Beller]

On Sun, Jan 31, 2016 at 1:09 PM, Jens Lehmann  wrote:
>>> After all this config is
>>> just about which submodules are chosen to be updated on clone and
>>> submodule update, not on all the other work tree manipulating
>>> commands.
>>
>>
>> So you'd imagine that "git submodule update" would remove the
>> submodule and setup an empty directory in case that submodule is
>> not configured ? (after switching branches or when just having cloned
>> that thing.)
>
>
> Not as a result of the label feature we are talking about here,
> I think that should just do what currently happens to removed
> submodules: they are left populated but are ignored by status,
> diff and submodule update. Removing the content is part of the
> recursive submodule update topic.

That is our second next big difference in understanding.
When having an autoInitialize option, we would not go further into
making rules for when to update submodules. "git submodule update"
would just update all initialzed submodules. (This thought ties well
together with only initializing those submodules that you want instead
of all of them.)

Now that we want to init all the submodules, it makes sense to only
update those we are interested in.

So from what I understand now:

(If labels are configured, then)

 * init a submodule at the earliest time possible, (i.e. while
   cloning or when upstream added a new submodule and
   we fetch it? Though then the init would be performed not in
   clone/fetch, but the "submodule update" step)

 * never deinit submodules automatically

 * submodule update is picky what to update by default.
   It will only update by label selection or directly specified submodules
   (git submodule update foo will update foo no matter if foo is in the
   selected group, a plain "git submodule update" however will only
   update group/label selected modules.)

>>>
>>>
>>> And throw away any customization the user did (to the URL or other
>>> configurations)?
>>>
>>> Without this sparse/label/group functionality, init is the way the
>>> user tells us he is interested in a submodule. But when configuring
>>> a label/name/path to update, the old meaning of init is obsolete
>>> and superseded by the new mechanism.
>>
>>
>> Or if we keep it at "--initSubmodule" only, which only initializes
>> a subset of new submodules, the meaning is not superseded.
>>
>> By having the initSubmodule thing set, the user tells us "I am interested
>> in all currently initialized submodules plus some more in the future, but
>> these have not arrived yet. To know which submodules I mean in the future
>> apply this pattern."
>>
>> Let's take the simplest case:
>>
>> A user is interested in all the submodules. So currently they clone
>> and initialize all of them. When upstream adds a new submodule, their
>> expectation is broken that all submodules are there and checked out.
>> by having the autoInit option, we'd just initialize any new submodule
>> and the user assumption "I have all the submodules" is true after
>> any "submodule update".
>>
>> By that point of view, we would not need to keep all submodules
>> initialized,
>> but only those the user is interested in. No need to have complicated
>> branch switching rules, but just as now "plus some futureproof rules
>> to declare my interest of submodules".
>
>
> I'm not sure what "complicated branch switching rules" you are
> referring to here, as far as I can see these only happen when we do
> not automatically initialize all submodules. What am I missing?

I was assuming we need to delete the submodules and preserve
their state somewhere.

So what I understand now:

$ git checkout  # will not touch submodules, however
$ git submodule update # may update a different selection as the
$ # selection changed by a different .gitmodules file

>
> You'd have to deal with initialized but not to be updated submodules
> anyway (due to the user choosing a different label or upstream
> changing label assigments). So it looks to me like the approach to
> initialize them all as soon as they appear is easier to grok. And
> update, diff and status will just skip all submodules that don't
> match the configured label(s).

We can teach update, diff and status to ignore those non selected
submodules just fine, but I still think it is somewhat ugly.
(Consider the build system, which now needs to somehow know
which submodules to build. The non selected modules may
error out when building as their dependencies are wrong/not met;
also humans don't like wading through a ton of empty directories
to find their pet project)

>
> Additionally this will make it easier to e.g. change the upstream
> URL of the submodules in one go, as this has to be done after they
> have been initialized. If you clone the android repo from a local
> mirror it'd be great to just update all URL settings once right
> after clone instead of having to do that again each time you choose
> a different group.
>
> So I'm not per se against 

Re: [PATCH 3/5] notes: read copied notes with strbuf_getline()

[Junio C Hamano]

Same comment as 1/5 and 2/5 applies.  You need to think if
strbuf_rtim(split[1]) is necessary here.

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] t6302: drop unnecessary GPG requirement

[Junio C Hamano]

Johannes Schindelin  writes:

> An even easier solution might be to *not* set up the signed tags in the
> 'setup' part, but only in the respective test case, and delete them right
> away after said test case?

After reading your patch, I do not find it an "easier solution", at
least with the definition of the word "solution" I would use.  It
stops testing signed or doubly signed tags everywhere, assuming that
future regressions can ever break only --points-at tests and no
other tests around signed tags.  The easiest solution along that
same line of thought taken to the extreme would be to say test_done
without having any test ;-)

The "filter entries about signed tags from the expected output"
approach I suggested would not work if the expected output files are
not line oriented and lines that would appear only when signed tags
are tested cannot be easily filtered out (like with the sed script
in the message you are responding to), but I think for the existing
test cases (with or without additions of annotated but unsigned
tags) it would work.

> Something like this (I even tested this with and without the GPG prereq):
> ...
> -test_expect_success 'check signed tags with --points-at' '
> +test_expect_success GPG 'check signed tags with --points-at' '
> + git tag -s -m "A signed tag message" signed-tag side &&
> + git tag -s -m "Annonated doubly" double-tag signed-tag &&
> + test_when_finished git tag -d signed-tag &&

Interestingly, double-tag is not removed here.

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/5] clean: read user input with strbuf_getline()

[Junio C Hamano]

The same comment (including "think if this trim is still necessary")
as 1/5 applies here.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/5] bisect: read bisect paths with strbuf_getline()

[Junio C Hamano]

Moritz Neeb  writes:

> The lines read from BISECT_NAMES are trimmed with strbuf_trim()
> immediately. There is thus no logic expecting CR, so
> strbuf_getline_lf() can be replaced by its CRLF counterpart.

We do not indent the whole log message.

You would also want to think about the necessity of strbuf_trim()
here.  Now strbuf_getline() would trim the trailing CR, would we
still need to call strbuf_trim() here?  The code will break if you
just remove the call, but on the other hand, you will realize that
the trimming done by calling it is excessive and unnecessary, once
you inspect the code and learn who writes the file being read here
and how.

> Signed-off-by: Moritz Neeb 
> ---
>  bisect.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/bisect.c b/bisect.c
> index 06ec54e..bf7c885 100644
> --- a/bisect.c
> +++ b/bisect.c
> @@ -440,7 +440,7 @@ static void read_bisect_paths(struct argv_array *array)
>   if (!fp)
>   die_errno("Could not open file '%s'", filename);
>  -while (strbuf_getline_lf(, fp) != EOF) {
> + while (strbuf_getline(, fp) != EOF) {
>   strbuf_trim();
>   if (sq_dequote_to_argv_array(str.buf, array))
>   die("Badly quoted content in file '%s': %s",
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: AW: [PATCH 1/2] stash--helper: implement "git stash--helper"

[Junio C Hamano]

Michael Blume  writes:

> Maybe this isn't important given that it looks like the patch is going
> to be rewritten, but I have
>
> stash.c:43:18: warning: incompatible pointer types assigning to 'const
> char *const *' from 'const char *'; take the address with &
> [-Wincompatible-pointer-types]
> write_tree.env = prefix;

The way posted patch tries to use the .env field when using the
run-command API is totally bogus and this compilation error is a
manifestation of that.

But the good news is that this should become irrelevant when the
patch is done by using internal calls ;-).
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC] GPG-Signed pushes & commits: differentiating between no signature and an unknown key

[Robin H. Johnson]

Format string %G? includes state 'N', which is described as "no
signature".

If you try to verify a commit or push for which you have no key (and you
don't automatically fetch from the keyservers [1]), then the format
string ALSO contains 'N', which is incorrect.

It should be possible to differentiate between a commit/push with NO
signature, and a commit/push signed with an unknown key.

In the case of verifying signed pushes before accepting them, this is
critical to providing a useful error message to the user. Presently, if
%G? evaluates to 'N', then none of the GIT_PUSH_CERT* env vars are set.

In the case of the signed push with the unknown key, they should remain
set.

[1] Eg, if you have an externally curated keyring and use trust-model
always.

-- 
Robin Hugh Johnson
Gentoo Linux: Developer, Infrastructure Lead, Foundation Trustee
E-Mail : robb...@gentoo.org
GnuPG FP   : 11ACBA4F 4778E3F6 E4EDF38E B27B944E 34884E85


signature.asc
Description: Digital signature

Re: [PATCH v4 03/12] ref-filter: bump 'used_atom' and related code to the top

[Junio C Hamano]

Karthik Nayak  writes:

> Bump code to the top for usage in further patches.
> ---

Sign-off?

>  ref-filter.c | 30 +++---
>  1 file changed, 15 insertions(+), 15 deletions(-)
>
> diff --git a/ref-filter.c b/ref-filter.c
> index 38f38d4..c3a8372 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -16,6 +16,21 @@
>  
>  typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;
>  
> +/*
> + * An atom is a valid field atom listed below, possibly prefixed with
> + * a "*" to denote deref_tag().
> + *
> + * We parse given format string and sort specifiers, and make a list
> + * of properties that we need to extract out of objects.  ref_array_item
> + * structure will hold an array of values extracted that can be
> + * indexed with the "atom number", which is an index into this
> + * array.
> + */
> +static const char **used_atom;
> +static cmp_type *used_atom_type;
> +static int used_atom_cnt, need_tagged, need_symref;
> +static int need_color_reset_at_eol;
> +
>  static struct {
>   const char *name;
>   cmp_type cmp_type;
> @@ -92,21 +107,6 @@ struct atom_value {
>  };
>  
>  /*
> - * An atom is a valid field atom listed above, possibly prefixed with
> - * a "*" to denote deref_tag().
> - *
> - * We parse given format string and sort specifiers, and make a list
> - * of properties that we need to extract out of objects.  ref_array_item
> - * structure will hold an array of values extracted that can be
> - * indexed with the "atom number", which is an index into this
> - * array.
> - */
> -static const char **used_atom;
> -static cmp_type *used_atom_type;
> -static int used_atom_cnt, need_tagged, need_symref;
> -static int need_color_reset_at_eol;
> -
> -/*
>   * Used to parse format string and sort specifiers
>   */
>  int parse_ref_filter_atom(const char *atom, const char *ep)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 6/6] apply, ls-files: simplify "-z" parsing

[Junio C Hamano]

Junio C Hamano  writes:

> Of course, a patch adding a "--nul" can be the one that does the
> polarity flipping, so in that sense, this simplification is probably
> OK, as long as there is some comment that warns a time-bomb you just
> planted here ;-)

I'll queue it with this tweak for now.

The idea is to have them run "blame" to find the last paragraph of
the commit log message.

Thanks.

From: Jeff King 
Date: Sun, 31 Jan 2016 06:35:46 -0500
Subject: [PATCH] apply, ls-files: simplify "-z" parsing

As a short option, we cannot handle negation. Thus a callback
handling "unset" is overkill, and we can just use OPT_SET_INT
instead to handle setting the option.

 +  Anybody who adds "--nul" synonym to this later would need to be
 +  careful not to break "--no-nul", which should mean that lines are
 +  terminated with LF at the end.

Signed-off-by: Jeff King 
Signed-off-by: Junio C Hamano 

diff --git a/builtin/apply.c b/builtin/apply.c
index 565f3fd..d61ac65 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -4536,6 +4536,7 @@ int cmd_apply(int argc, const char **argv, const char 
*prefix_)
 N_( "attempt three-way merge if a patch does not 
apply")),
OPT_FILENAME(0, "build-fake-ancestor", _ancestor,
N_("build a temporary index based on embedded index 
information")),
+   /* Think twice before adding "--nul" synonym to this */
OPT_SET_INT('z', NULL, _termination,
N_("paths are separated with NUL character"), '\0'),
OPT_INTEGER('C', NULL, _context,
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index 59bad9b..467699b 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -400,6 +400,7 @@ int cmd_ls_files(int argc, const char **argv, const char 
*cmd_prefix)
struct exclude_list *el;
struct string_list exclude_list = STRING_LIST_INIT_NODUP;
struct option builtin_ls_files_options[] = {
+   /* Think twice before adding "--nul" synonym to this */
OPT_SET_INT('z', NULL, _terminator,
N_("paths are separated with NUL character"), '\0'),
OPT_BOOL('t', NULL, _tag,

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: AW: [PATCH 1/2] stash--helper: implement "git stash--helper"

[Michael Blume]

On Fri, Jan 29, 2016 at 11:58 AM, Junio C Hamano  wrote:
> Matthias Aßhauer  writes:
>
> [administrivia: please wrap your lines to reasonable lengths]
>
>>> Honestly, I had high hopes after seeing the "we are rewriting it
>>> in C" but I am not enthused after seeing this.  I was hoping that
>>> the rewritten version would do this all in-core, by calling these
>>> functions that we already have:
>>
>> These functions might be obvious to you, but I'm new to git's
>> source code, ...
>
> Ahh, I didn't realize I was talking with somebody unfamiliar with
> the codebase.  Apologies.
>
> Nevertheless, the list of functions I gave are a good starting
> point; they are widely used building blocks in the codebase.
>
>> I'll be working on a v2 that incorporates the feedback from you,
>> Thomas Gummerer and Stefan Beller then. Further feedback is of
>> course welcome.
>
> Thanks.
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Maybe this isn't important given that it looks like the patch is going
to be rewritten, but I have

stash.c:43:18: warning: incompatible pointer types assigning to 'const
char *const *' from 'const char *'; take the address with &
[-Wincompatible-pointer-types]
write_tree.env = prefix;
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V5 2/2] object name: introduce '^{/!-}' notation

[Junio C Hamano]

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: git log -g bizarre behaviour

[Junio C Hamano]

Dennis Kaarsemaker  writes:

> I'm attempting to understand the log [-g] / reflog code enough to
> untangle them and make reflog walking work for more than just commit
> objects [see gmane 283169]. I found something which I think is wrong,
> and would break after my changes.
>
> git log -g HEAD^ and git log -g v2.7.0^ give no output. This is
> expected, as those are not things that have a reflog.

OK.

> But git log -g v2.7.0 seems to ignore -g and gives the normal
> log.

That sounds clearly broken, and I think I see how that happens from
the hacky way the "-g" traversal was bolted onto the revision
traversal machinery.

I _think_ "git log -g" (and by extension "git reflog" which is just
a short-hand to giving a few more options to that command) ought to

 * Iterate over the _objects_ that used to be at the tip of the ref;
 * Show each of these objects as if they were fed to "git show".

This clearly is not possible without major surgery, including
ripping out the hacky "-g" traversal from the revision traversal
machinery and perhaps lifting it up a few levels in the callchain,
as many functions in that callchain want to work on commits.

Contrast these two:

$ git log -1 v2.7.0
$ git show v2.7.0

> I'd like to make git log -g / git reflog abort early when trying to
> display a reflog of a ref that has no reflog. Objections?

Do you mean

$ git checkout -b testing
$ rm -f .git/logs/refs/heads/testing
$ git log -g testing

will be changed from a silent no-op to an abort with error?

I do not see a need for such a change--does that count as an
objection?

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 6/6] apply, ls-files: simplify "-z" parsing

[Junio C Hamano]

Jeff King  writes:

> As a short option, we cannot handle negation. Thus a
> callback handling "unset" is overkill, and we can just use
> OPT_SET_INT instead to handle setting the option.
>
> Signed-off-by: Jeff King 
> ---
> I left this one for last, because it's the most questionable. Unlike the
> previous "-z" case, we're setting the actual character, so the logic is
> inverted: turning on the option sets it to 0, and turning it off restore
> '\n'.
>
> This means OPT_SET_INT would do the wrong thing for the "unset" case, as
> it would put a "0" into the option. You can't trigger that now, but if
> somebody were to add a long option (e.g., "--nul"), then "--no-nul"
> would do the wrong thing.
>
> I'm on the fence on whether the simplification is worth it, or if we
> should leave the callbacks as future-proofing.

If done as two patch series where one does this and the other flips
polarity of the variable (!!line_termination === !nul_term_line),
would that make the result both simpler to read and future-proof?

Of course, a patch adding a "--nul" can be the one that does the
polarity flipping, so in that sense, this simplification is probably
OK, as long as there is some comment that warns a time-bomb you just
planted here ;-)

>  builtin/apply.c| 15 ++-
>  builtin/ls-files.c | 13 ++---
>  2 files changed, 4 insertions(+), 24 deletions(-)
>
> diff --git a/builtin/apply.c b/builtin/apply.c
> index deb1364..565f3fd 100644
> --- a/builtin/apply.c
> +++ b/builtin/apply.c
> @@ -4464,16 +4464,6 @@ static int option_parse_p(const struct option *opt,
>   return 0;
>  }
>  
> -static int option_parse_z(const struct option *opt,
> -   const char *arg, int unset)
> -{
> - if (unset)
> - line_termination = '\n';
> - else
> - line_termination = 0;
> - return 0;
> -}
> -
>  static int option_parse_space_change(const struct option *opt,
> const char *arg, int unset)
>  {
> @@ -4546,9 +4536,8 @@ int cmd_apply(int argc, const char **argv, const char 
> *prefix_)
>N_( "attempt three-way merge if a patch does not 
> apply")),
>   OPT_FILENAME(0, "build-fake-ancestor", _ancestor,
>   N_("build a temporary index based on embedded index 
> information")),
> - { OPTION_CALLBACK, 'z', NULL, NULL, NULL,
> - N_("paths are separated with NUL character"),
> - PARSE_OPT_NOARG, option_parse_z },
> + OPT_SET_INT('z', NULL, _termination,
> + N_("paths are separated with NUL character"), '\0'),
>   OPT_INTEGER('C', NULL, _context,
>   N_("ensure at least  lines of context 
> match")),
>   { OPTION_CALLBACK, 0, "whitespace", _option, 
> N_("action"),
> diff --git a/builtin/ls-files.c b/builtin/ls-files.c
> index b6a7cb0..59bad9b 100644
> --- a/builtin/ls-files.c
> +++ b/builtin/ls-files.c
> @@ -359,14 +359,6 @@ static const char * const ls_files_usage[] = {
>   NULL
>  };
>  
> -static int option_parse_z(const struct option *opt,
> -   const char *arg, int unset)
> -{
> - line_terminator = unset ? '\n' : '\0';
> -
> - return 0;
> -}
> -
>  static int option_parse_exclude(const struct option *opt,
>   const char *arg, int unset)
>  {
> @@ -408,9 +400,8 @@ int cmd_ls_files(int argc, const char **argv, const char 
> *cmd_prefix)
>   struct exclude_list *el;
>   struct string_list exclude_list = STRING_LIST_INIT_NODUP;
>   struct option builtin_ls_files_options[] = {
> - { OPTION_CALLBACK, 'z', NULL, NULL, NULL,
> - N_("paths are separated with NUL character"),
> - PARSE_OPT_NOARG, option_parse_z },
> + OPT_SET_INT('z', NULL, _terminator,
> + N_("paths are separated with NUL character"), '\0'),
>   OPT_BOOL('t', NULL, _tag,
>   N_("identify the file status with tags")),
>   OPT_BOOL('v', NULL, _valid_bit,
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC] GPG-Signed pushes & commits: differentiating between no signature and an unknown key

[Junio C Hamano]

"Robin H. Johnson"  writes:

> Format string %G? includes state 'N', which is described as "no
> signature".
>
> If you try to verify a commit or push for which you have no key (and you
> don't automatically fetch from the keyservers [1]), then the format
> string ALSO contains 'N', which is incorrect.
>
> It should be possible to differentiate between a commit/push with NO
> signature, and a commit/push signed with an unknown key.
>
> In the case of verifying signed pushes before accepting them, this is
> critical to providing a useful error message to the user. Presently, if
> %G? evaluates to 'N', then none of the GIT_PUSH_CERT* env vars are set.

Hrm, I think GIT_PUSH_CERT* environment variables are exported
whenever push_cert_sha1[] is not "0"{40}, and push_cert_sha1[] is
cleared only when write_sha1_file() fails.  The failure from calling
parse_signature(), verify_signed_buffer() and parse_gpg_output()
does not clear push_cert_sha1[], so I am not sure if we are reading
the same code.

Are you talking about something other than prepare_push_cert_sha1()?

> In the case of the signed push with the unknown key, they should remain
> set.

In any case, I think "should" is relative to the balance between
convenience and safety.  If you set up your receiving end to use a
keyring that holds trusted keys and nothing else, it makes it harder
to make mistakes in the script and accept something you shouldn't if
the validation script is fed "No, this is not good" if a push is
signed by unknown key, so showing "known to be bad" and "unsure if
it is good" the same way with 'N' is what "should" happen from that
point of view.

Of course, a set-up that fetches unknown keys lazily when they are
first encountered would need more work to do the verification
themselves, but as long as GIT_PUSH_CERT itself is exported that
should be possible (iow, we are trying to make simple way less error
prone, while allowing more advanced use possible, if harder).

If the blob object name is not exported depending on the validation
status, that certainly sounds like a bug, as that would make "more
advanced use" above impossible.  But I am not sure how that happens.

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/5] wt-status: read rebase todolist with strbuf_getline()

[Junio C Hamano]

Moritz Neeb  writes:

> In read_rebase_todolist() every line is, after reading, checked
> for a comment_line_char. After that it is trimmed via strbuf_trim().
> Assuming we do never expect a CR as comment_line_char,
> strbuf_getline_lf() can be safely replaced by its CRLF counterpart.
>
> Signed-off-by: Moritz Neeb 
> ---
>
> Notes:
> Looking at the code in read_rebase_todolist(), the
> lines are read, checked for a comment_line_char and then trimmed. I
> wonder why the input is not trimmed before checking for this character?
> Is it safe to replace strbuf_getline_lf() by strbuf_getline() anyway?
> The only case I can imagine that could lead to unexpected behaviour 
> then
> would be when someone sets the comment_line_char to CR. How likely is 
> that?
> Why is the trim _after_ checking for the comment char anyway? Should
> something like "   # foobar" not be considered as comment?
> I decided to roll out the patch because I considered it not as a risk 
> to be
> taken seriously, that the comment line char will be '\r'.
> Meta-question: Should something of this discussion be put into the 
> commit?

Yes to the meta question.  Add something like this as the second
paragraph of the log message, but do *not* change the patch text.

The current code checks if the line begins with a comment
line char (typically '#') before trimming, which is
consistent with the way how commands like 'git commit'
prepares commented lines in that this does not treat a line
that begins with whitespaces and '#' as commented out.  We
however made a mistake in the parser of "git rebase -i" long
time ago to allow such a line to be treated as a comment,
and made an exception with 1db168ee (rebase-i: loosen
over-eager check_bad_cmd check, 2015-10-01).  It probably is
a good idea to match that exception by swapping the order of
comment check and trimming in this codepath as well.

>
>  wt-status.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/wt-status.c b/wt-status.c
> index ab4f80d..f053b2f 100644
> --- a/wt-status.c
> +++ b/wt-status.c
> @@ -1076,7 +1076,7 @@ static void read_rebase_todolist(const char *fname, 
> struct string_list *lines)
>   if (!f)
>   die_errno("Could not open file %s for reading",
> git_path("%s", fname));
> - while (!strbuf_getline_lf(, f)) {
> + while (!strbuf_getline(, f)) {
>   if (line.len && line.buf[0] == comment_line_char)
>   continue;
>   strbuf_trim();
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 00/12] ref-filter: use parsing functions

[Junio C Hamano]

Karthik Nayak  writes:

> This series cleans up populate_value() in ref-filter, by moving out
> the parsing part of atoms to separate parsing functions. This ensures
> that parsing is only done once and also improves the modularity of the
> code.
>
> v1: http://thread.gmane.org/gmane.comp.version-control.git/281180
> v2: http://thread.gmane.org/gmane.comp.version-control.git/282563
> v3: http://thread.gmane.org/gmane.comp.version-control.git/283350
>
> Changes:
> * The parsing functions now take the arguments of the atom as
> function parameteres, instead of parsing it inside the fucntion.
> * Rebased on top of pu:jk/list-tag-2.7-regression
> * In strbuf use a copylen variable rather than using multiplication
> to perform a logical operation.
> * Code movement for easier review and general improvement.
> * Use COLOR_MAXLEN as the maximum size for the color variable.
> * Small code changes.
> * Documentation changes.
> * Fixed incorrect style of test (t6302).
>
> Karthik Nayak (12):
>   strbuf: introduce strbuf_split_str_omit_term()
>   ref-filter: use strbuf_split_str_omit_term()
>   ref-filter: bump 'used_atom' and related code to the top
>   ref-filter: introduce struct used_atom
>   ref-filter: introduce parsing functions for each valid atom
>   ref-filter: introduce color_atom_parser()
>   ref-filter: introduce parse_align_position()
>   ref-filter: introduce align_atom_parser()
>   ref-filter: align: introduce long-form syntax
>   ref-filter: introduce remote_ref_atom_parser()
>   ref-filter: introduce contents_atom_parser()
>   ref-filter: introduce objectname_atom_parser()

Hmm, 10/12 didn't make it to the list?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

parse_object does check_sha1_signature but not parse_object_buffer?

[Mike Hommey]

Hi,

You might or might not be aware of this thread:
https://groups.google.com/forum/#!topic/binary-transparency/f-BI4o8HZW0

Anyways, this got me to take a look around, and I noticed that
parse_object does SHA-1 validation through check_sha1_signature. What
surprised me is that parse_object_buffer doesn't. So we end up with
inconsistent behavior across commands:

$ git init
$ echo a > a ; echo b > b
$ git add a b
$ git cat-file blob 78981922613b2afb6025042ff6bd878ac1994e85
a
$ cp -f .git/objects/61/780798228d17af2d34fce4cfbdf35556832472 
.git/objects/78/981922613b2afb6025042ff6bd878ac1994e85
$ git cat-file blob 78981922613b2afb6025042ff6bd878ac1994e85
b
$ git show 78981922613b2afb6025042ff6bd878ac1994e85
error: sha1 mismatch 78981922613b2afb6025042ff6bd878ac1994e85
fatal: bad object 78981922613b2afb6025042ff6bd878ac1994e85

Shouldn't parse_object_buffer also do check_sha1_signature?

Mike
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: parse_object does check_sha1_signature but not parse_object_buffer?

[Mike Hommey]

On Tue, Feb 02, 2016 at 10:57:01AM +0900, Mike Hommey wrote:
> Hi,
> 
> You might or might not be aware of this thread:
> https://groups.google.com/forum/#!topic/binary-transparency/f-BI4o8HZW0
> 
> Anyways, this got me to take a look around, and I noticed that
> parse_object does SHA-1 validation through check_sha1_signature. What
> surprised me is that parse_object_buffer doesn't. So we end up with
> inconsistent behavior across commands:
> 
> $ git init
> $ echo a > a ; echo b > b
> $ git add a b
> $ git cat-file blob 78981922613b2afb6025042ff6bd878ac1994e85
> a
> $ cp -f .git/objects/61/780798228d17af2d34fce4cfbdf35556832472 
> .git/objects/78/981922613b2afb6025042ff6bd878ac1994e85
> $ git cat-file blob 78981922613b2afb6025042ff6bd878ac1994e85
> b
> $ git show 78981922613b2afb6025042ff6bd878ac1994e85
> error: sha1 mismatch 78981922613b2afb6025042ff6bd878ac1994e85
> fatal: bad object 78981922613b2afb6025042ff6bd878ac1994e85
> 
> Shouldn't parse_object_buffer also do check_sha1_signature?

Well, except cat-file doesn't use parse_object_buffer either...

Mike
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 00/12] ref-filter: use parsing functions

[Eric Sunshine]

On Mon, Feb 1, 2016 at 5:25 PM, Junio C Hamano  wrote:
> Karthik Nayak  writes:
>
>> This series cleans up populate_value() in ref-filter, by moving out
>> the parsing part of atoms to separate parsing functions. This ensures
>> that parsing is only done once and also improves the modularity of the
>> code.
>>
>> v1: http://thread.gmane.org/gmane.comp.version-control.git/281180
>> v2: http://thread.gmane.org/gmane.comp.version-control.git/282563
>> v3: http://thread.gmane.org/gmane.comp.version-control.git/283350
>>
>> Changes:
>> * The parsing functions now take the arguments of the atom as
>> function parameteres, instead of parsing it inside the fucntion.
>> * Rebased on top of pu:jk/list-tag-2.7-regression
>> * In strbuf use a copylen variable rather than using multiplication
>> to perform a logical operation.
>> * Code movement for easier review and general improvement.
>> * Use COLOR_MAXLEN as the maximum size for the color variable.
>> * Small code changes.
>> * Documentation changes.
>> * Fixed incorrect style of test (t6302).
>>
>> Karthik Nayak (12):
>>   strbuf: introduce strbuf_split_str_omit_term()
>>   ref-filter: use strbuf_split_str_omit_term()
>>   ref-filter: bump 'used_atom' and related code to the top
>>   ref-filter: introduce struct used_atom
>>   ref-filter: introduce parsing functions for each valid atom
>>   ref-filter: introduce color_atom_parser()
>>   ref-filter: introduce parse_align_position()
>>   ref-filter: introduce align_atom_parser()
>>   ref-filter: align: introduce long-form syntax
>>   ref-filter: introduce remote_ref_atom_parser()
>>   ref-filter: introduce contents_atom_parser()
>>   ref-filter: introduce objectname_atom_parser()
>
> Hmm, 10/12 didn't make it to the list?

Not surprising. Somehow, Karthik did git-add on the compiled
test-fake-ssh before committing, so the patch sent to the list
contains an rather huge binary resource. I did receive it since I was
a direct addressee of the email; I'll reply to the message on-list
without modifying anything (other than stripping out the binary
resource) so that other reviewers get a chance to see it.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC] GPG-Signed pushes & commits: differentiating between no signature and an unknown key

[Robin H. Johnson]

On Mon, Feb 01, 2016 at 02:49:09PM -0800,  Junio C Hamano wrote:
> Are you talking about something other than prepare_push_cert_sha1()?
I went and verified it, and what was reported to me was slightly wrong. Only
some of the field are empty, notably CERT_KEY and SIGNER.

Signed push with an unknown key:
===
remote: No signature found
remote: Your push was not signed with a known key.
remote: You MUST use git push --signed with a known key.
remote: If you just updated your key, please wait 15 minutes for sync.
remote: git-receive-pack variables:
remote: GIT_PUSH_CERT='1c471177906014e65e2825ee71572bf749970c16'
remote: GIT_PUSH_CERT_KEY=''
remote: GIT_PUSH_CERT_NONCE='1454372558-35db7be4533958f14731'
remote: GIT_PUSH_CERT_NONCE_SLOP=''
remote: GIT_PUSH_CERT_NONCE_STATUS='OK'
remote: GIT_PUSH_CERT_SIGNER=''
remote: GIT_PUSH_CERT_STATUS='N'
To git+ssh://g...@git.gentoo.org/repo/gentoo
 ! [remote rejected] master -> master (pre-receive hook declined)
===

Unsigned push:
===
remote: Unknown GIT_PUSH_CERT_STATUS
remote: Your push was not signed with a known key.
remote: You MUST use git push --signed with a known key.
remote: If you just updated your key, please wait 15 minutes for sync.
remote: git-receive-pack variables:
remote: GIT_PUSH_CERT=''
remote: GIT_PUSH_CERT_KEY=''
remote: GIT_PUSH_CERT_NONCE=''
remote: GIT_PUSH_CERT_NONCE_SLOP=''
remote: GIT_PUSH_CERT_NONCE_STATUS=''
remote: GIT_PUSH_CERT_SIGNER=''
remote: GIT_PUSH_CERT_STATUS=''
To git+ssh://g...@git.gentoo.org/repo/gentoo
 ! [remote rejected] master -> master (pre-receive hook declined)
===

Here's the raw blob, while it still exists.

certificate version 0.1
pusher 0x55272E9740B89B54 1454372558 -0800
pushee git+ssh://git.gentoo.org/repo/gentoo
nonce 1454372558-35db7be4533958f14731

99a4d87ed7b79ea050adb99f941accf33e4ba963 
71535a9475cdd4949c4031676238dc9f60e1828a refs/heads/master
-BEGIN PGP SIGNATURE-
...
-END PGP SIGNATURE-



> > In the case of the signed push with the unknown key, they should remain
> > set.
> 
> In any case, I think "should" is relative to the balance between
> convenience and safety.  If you set up your receiving end to use a
> keyring that holds trusted keys and nothing else, it makes it harder
> to make mistakes in the script and accept something you shouldn't if
> the validation script is fed "No, this is not good" if a push is
> signed by unknown key, so showing "known to be bad" and "unsure if
> it is good" the same way with 'N' is what "should" happen from that
> point of view.
Ok, at the very least, can we consider populating GIT_PUSH_CERT_KEY and
GIT_PUSH_CERT_SIGNER even with GIT_PUSH_CERT_STATUS set to N?

Then change the documentation to say 'No valid signature' rather than 'No
signature'? (introducing another letter would be better I think).

> 
> Of course, a set-up that fetches unknown keys lazily when they are
> first encountered would need more work to do the verification
> themselves, but as long as GIT_PUSH_CERT itself is exported that
> should be possible (iow, we are trying to make simple way less error
> prone, while allowing more advanced use possible, if harder).
If it fetches the new key, and finds it be in some WOT or external trustdb, it
could accept it.

> If the blob object name is not exported depending on the validation
> status, that certainly sounds like a bug, as that would make "more
> advanced use" above impossible.  But I am not sure how that happens.
I think the earlier blobs got GC'd, hence why I didn't find them.

Advanced usage: Maybe record them to a ref of failed pushes (would be in the
hook to check the signature).

I think after this is cleaned up, I'll send the Gentoo require-signed-push hook
for inclusion in contrib/.

-- 
Robin Hugh Johnson
Gentoo Linux: Developer, Infrastructure Lead, Foundation Trustee
E-Mail : robb...@gentoo.org
GnuPG FP   : 11ACBA4F 4778E3F6 E4EDF38E B27B944E 34884E85
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 10/12] ref-filter: introduce remote_ref_atom_parser()

[Karthik Nayak]

On Tue, Feb 2, 2016 at 6:29 AM, Eric Sunshine  wrote:
> This is a re-send of patch 10/12 on Karthik's behalf to give other
> reviewers a chance at it. The original did not make it to the mailing
> list since it contained a rather large binary resource Karthik
> accidentally included in the commit (which has been stripped from
> this re-send).
>

Thanks a lot, wondering how that happened.

> On Sun, Jan 31, 2016 at 11:12:54PM +0530, Karthik Nayak wrote:
>> Introduce remote_ref_atom_parser() which will parse the '%(upstream)'
>> and '%(push)' atoms and store information into the 'used_atom'
>> structure based on the modifiers used along with the corresponding
>> atom.
>>
>> Helped-by: Ramsay Jones 
>> Helped-by: Eric Sunshine 
>> Signed-off-by: Karthik Nayak 
>> ---
>>  ref-filter.c  | 103 
>> ++
>>  test-fake-ssh | Bin 0 -> 4668264 bytes
>>  2 files changed, 61 insertions(+), 42 deletions(-)
>>  create mode 100755 test-fake-ssh
>>
>> diff --git a/ref-filter.c b/ref-filter.c
>> index 58d433f..99c152d 100644
>> --- a/ref-filter.c
>> +++ b/ref-filter.c
>> @@ -37,6 +37,8 @@ static struct used_atom {
>>   union {
>>   char color[COLOR_MAXLEN];
>>   struct align align;
>> + enum { RR_NORMAL, RR_SHORTEN, RR_TRACK, RR_TRACKSHORT }
>> + remote_ref;
>>   } u;
>>  } *used_atom;
>>  static int used_atom_cnt, need_tagged, need_symref;
>> @@ -50,6 +52,20 @@ static void color_atom_parser(struct used_atom *atom, 
>> const char *color_value)
>>   die(_("invalid color value: %s"), atom->u.color);
>>  }
>>
>> +static void remote_ref_atom_parser(struct used_atom *atom, const char *arg)
>> +{
>> + if (!arg) {
>> + atom->u.remote_ref = RR_NORMAL;
>> + } else if (!strcmp(arg, "short"))
>> + atom->u.remote_ref = RR_SHORTEN;
>> + else if (!strcmp(arg, "track"))
>> + atom->u.remote_ref = RR_TRACK;
>> + else if (!strcmp(arg, "trackshort"))
>> + atom->u.remote_ref = RR_TRACKSHORT;
>> + else
>> + die(_("unrecognized format: %%(%s)"), atom->name);
>> +}
>> +
>>  static align_type parse_align_position(const char *s)
>>  {
>>   if (!strcmp(s, "right"))
>> @@ -132,8 +148,8 @@ static struct {
>>   { "subject" },
>>   { "body" },
>>   { "contents" },
>> - { "upstream" },
>> - { "push" },
>> + { "upstream", FIELD_STR, remote_ref_atom_parser },
>> + { "push", FIELD_STR, remote_ref_atom_parser },
>>   { "symref" },
>>   { "flag" },
>>   { "HEAD" },
>> @@ -839,6 +855,43 @@ static const char *strip_ref_components(const char 
>> *refname, const char *nr_arg)
>>   return start;
>>  }
>>
>> +static void fill_remote_ref_details(struct used_atom *atom, const char 
>> *refname,
>> + struct branch *branch, const char **s)
>> +{
>> + int num_ours, num_theirs;
>> + if (atom->u.remote_ref == RR_SHORTEN)
>> + *s = shorten_unambiguous_ref(refname, warn_ambiguous_refs);
>> + else if (atom->u.remote_ref == RR_TRACK) {
>> + if (stat_tracking_info(branch, _ours,
>> +_theirs, NULL))
>> + return;
>> +
>> + if (!num_ours && !num_theirs)
>> + *s = "";
>> + else if (!num_ours)
>> + *s = xstrfmt("[behind %d]", num_theirs);
>> + else if (!num_theirs)
>> + *s = xstrfmt("[ahead %d]", num_ours);
>> + else
>> + *s = xstrfmt("[ahead %d, behind %d]",
>> +  num_ours, num_theirs);
>> + } else if (atom->u.remote_ref == RR_TRACKSHORT) {
>> + if (stat_tracking_info(branch, _ours,
>> +_theirs, NULL))
>> + return;
>> +
>> + if (!num_ours && !num_theirs)
>> + *s = "=";
>> + else if (!num_ours)
>> + *s = "<";
>> + else if (!num_theirs)
>> + *s = ">";
>> + else
>> + *s = "<>";
>> + } else /* RR_NORMAL */
>> + *s = refname;
>> +}
>> +
>>  /*
>>   * Parse the object referred by ref, and grab needed value.
>>   */
>> @@ -890,8 +943,9 @@ static void populate_value(struct ref_array_item *ref)
>>   branch = branch_get(branch_name);
>>
>>   refname = branch_get_upstream(branch, NULL);
>> - if (!refname)
>> - continue;
>> + if (refname)
>> + fill_remote_ref_details(atom, refname, branch, 
>> >s);
>> + continue;
>>   } else if (starts_with(name, "push")) {
>>   

Re: parse_object does check_sha1_signature but not parse_object_buffer?

[Junio C Hamano]

Mike Hommey  writes:

> Shouldn't parse_object_buffer also do check_sha1_signature?

In general, it shouldn't; its callers are supposed to do it as
additional check when/if needed.  Callers like the one in fsck.c
does not want to die after seeing one bad one.  We want to report
and keep checking other things.


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 10/12] ref-filter: introduce remote_ref_atom_parser()

[Eric Sunshine]

This is a re-send of patch 10/12 on Karthik's behalf to give other
reviewers a chance at it. The original did not make it to the mailing
list since it contained a rather large binary resource Karthik
accidentally included in the commit (which has been stripped from
this re-send).

On Sun, Jan 31, 2016 at 11:12:54PM +0530, Karthik Nayak wrote:
> Introduce remote_ref_atom_parser() which will parse the '%(upstream)'
> and '%(push)' atoms and store information into the 'used_atom'
> structure based on the modifiers used along with the corresponding
> atom.
> 
> Helped-by: Ramsay Jones 
> Helped-by: Eric Sunshine 
> Signed-off-by: Karthik Nayak 
> ---
>  ref-filter.c  | 103 
> ++
>  test-fake-ssh | Bin 0 -> 4668264 bytes
>  2 files changed, 61 insertions(+), 42 deletions(-)
>  create mode 100755 test-fake-ssh
> 
> diff --git a/ref-filter.c b/ref-filter.c
> index 58d433f..99c152d 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -37,6 +37,8 @@ static struct used_atom {
>   union {
>   char color[COLOR_MAXLEN];
>   struct align align;
> + enum { RR_NORMAL, RR_SHORTEN, RR_TRACK, RR_TRACKSHORT }
> + remote_ref;
>   } u;
>  } *used_atom;
>  static int used_atom_cnt, need_tagged, need_symref;
> @@ -50,6 +52,20 @@ static void color_atom_parser(struct used_atom *atom, 
> const char *color_value)
>   die(_("invalid color value: %s"), atom->u.color);
>  }
>  
> +static void remote_ref_atom_parser(struct used_atom *atom, const char *arg)
> +{
> + if (!arg) {
> + atom->u.remote_ref = RR_NORMAL;
> + } else if (!strcmp(arg, "short"))
> + atom->u.remote_ref = RR_SHORTEN;
> + else if (!strcmp(arg, "track"))
> + atom->u.remote_ref = RR_TRACK;
> + else if (!strcmp(arg, "trackshort"))
> + atom->u.remote_ref = RR_TRACKSHORT;
> + else
> + die(_("unrecognized format: %%(%s)"), atom->name);
> +}
> +
>  static align_type parse_align_position(const char *s)
>  {
>   if (!strcmp(s, "right"))
> @@ -132,8 +148,8 @@ static struct {
>   { "subject" },
>   { "body" },
>   { "contents" },
> - { "upstream" },
> - { "push" },
> + { "upstream", FIELD_STR, remote_ref_atom_parser },
> + { "push", FIELD_STR, remote_ref_atom_parser },
>   { "symref" },
>   { "flag" },
>   { "HEAD" },
> @@ -839,6 +855,43 @@ static const char *strip_ref_components(const char 
> *refname, const char *nr_arg)
>   return start;
>  }
>  
> +static void fill_remote_ref_details(struct used_atom *atom, const char 
> *refname,
> + struct branch *branch, const char **s)
> +{
> + int num_ours, num_theirs;
> + if (atom->u.remote_ref == RR_SHORTEN)
> + *s = shorten_unambiguous_ref(refname, warn_ambiguous_refs);
> + else if (atom->u.remote_ref == RR_TRACK) {
> + if (stat_tracking_info(branch, _ours,
> +_theirs, NULL))
> + return;
> +
> + if (!num_ours && !num_theirs)
> + *s = "";
> + else if (!num_ours)
> + *s = xstrfmt("[behind %d]", num_theirs);
> + else if (!num_theirs)
> + *s = xstrfmt("[ahead %d]", num_ours);
> + else
> + *s = xstrfmt("[ahead %d, behind %d]",
> +  num_ours, num_theirs);
> + } else if (atom->u.remote_ref == RR_TRACKSHORT) {
> + if (stat_tracking_info(branch, _ours,
> +_theirs, NULL))
> + return;
> +
> + if (!num_ours && !num_theirs)
> + *s = "=";
> + else if (!num_ours)
> + *s = "<";
> + else if (!num_theirs)
> + *s = ">";
> + else
> + *s = "<>";
> + } else /* RR_NORMAL */
> + *s = refname;
> +}
> +
>  /*
>   * Parse the object referred by ref, and grab needed value.
>   */
> @@ -890,8 +943,9 @@ static void populate_value(struct ref_array_item *ref)
>   branch = branch_get(branch_name);
>  
>   refname = branch_get_upstream(branch, NULL);
> - if (!refname)
> - continue;
> + if (refname)
> + fill_remote_ref_details(atom, refname, branch, 
> >s);
> + continue;
>   } else if (starts_with(name, "push")) {
>   const char *branch_name;
>   if (!skip_prefix(ref->refname, "refs/heads/",
> @@ -902,6 +956,8 @@ static void populate_value(struct ref_array_item *ref)
>   refname = branch_get_push(branch, NULL);
>

Re: [RFC/PATCH] Git doc: GPL2 does not apply to repo data

[Jeff King]

On Mon, Feb 01, 2016 at 08:49:55AM +0100, Matthieu Moy wrote:

> - Original Message -
> > For less tech-savvy managers, I found that name dropping works quite well
> > (read: naming a couple of well-known companies/products that switched to
> > Git).
> 
> In the same category: "GitHub has 12 millions users" works rather well.

Who's to say GitHub's business plan isn't to become a copyright troll? :)

On a more serious note, this FAQ (and the one right after) might be
useful for convincing people:

  http://www.gnu.org/licenses/gpl-faq.en.html#GPLOutput

Data that git stores is not strictly "output", but I think the answers
there are relevant. And presumably written or vetted by lawyers, too.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC/PATCH] Git doc: GPL2 does not apply to repo data

[Jeff King]

On Mon, Feb 01, 2016 at 03:14:31AM -0500, Jeff King wrote:

> On a more serious note, this FAQ (and the one right after) might be
> useful for convincing people:
> 
>   http://www.gnu.org/licenses/gpl-faq.en.html#GPLOutput
> 
> Data that git stores is not strictly "output", but I think the answers
> there are relevant. And presumably written or vetted by lawyers, too.

Whoops, I just noticed this is the exact entry from Philip's patch. :-/

Sorry for the noise (and I do think it is a good link to help answer
this question, but I agree with Junio that we can let that FAQ stand on
its own without adding our own amateur-lawyer language to it).

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC/PATCH] Git doc: GPL2 does not apply to repo data

[Philip Oakley]

From: "Jeff King" 

On Mon, Feb 01, 2016 at 08:49:55AM +0100, Matthieu Moy wrote:


- Original Message -
> For less tech-savvy managers, I found that name dropping works quite 
> well
> (read: naming a couple of well-known companies/products that switched 
> to

> Git).

In the same category: "GitHub has 12 millions users" works rather well.


Who's to say GitHub's business plan isn't to become a copyright troll? :)

On a more serious note, this FAQ (and the one right after) might be
useful for convincing people:

 http://www.gnu.org/licenses/gpl-faq.en.html#GPLOutput



I'd added a link to the GPL2 faq version of that, though my link did take 
the reader to the contents list item.


The oddball is the templates, which could be argued are full GPL2 (see the 
mention of Bison in that same paragraph), and would need a further notice 
about their licence terms (given an insistent copyright holder!)



Data that git stores is not strictly "output", but I think the answers
there are relevant. And presumably written or vetted by lawyers, too.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html



--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC/PATCH] Git doc: GPL2 does not apply to repo data

[Philip Oakley]

From: "Junio C Hamano" 

Philip Oakley  writes:


diff --git a/Documentation/git.txt b/Documentation/git.txt
index bff6302..137c89c 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -1132,6 +1132,17 @@ of clones and fetches.
   - any external helpers are named by their protocol (e.g., use
 `hg` to allow the `git-remote-hg` helper)

+Licencing: Your data, and the Git tool[[Licencing]]
+---
+
+Git is an open source tool provided under GPL2.
+Git was designed to be, and is, the version control system
+for the Linux codebase.
+Your respository data created by Git is not subject to Git's GNU2
+licence, see GPL FAQs
+http://www.gnu.org/licenses/old-licenses/gpl-2.0-faq.en.html#TOCGPLOutput).
+
+User should apply a licence of their own choice to their repository 
data.


 Discussion[[Discussion]]
 


While I know you mean well, and I do understand the sentiment behind
this addition,

It was an RFC for that very sentiment.


there are at least two reasons why I do not want to
(and why we should not) add any "clarification" or "interpretation"
like this.

One is because such a statement is pointless.  Because we do not do
copyright assignment to the project, you are not the sole copyright
owner of Git.  Individual contributors hold copyright to the part
they wrote.  The above statement you made, even with an endorsement
by me as the project lead, does not have any power to assure that
the users will not get sued by one copyright holder, who is not you
or me, and at that point it is up to the court to interpret GPLv2.
We can call such a copyright holder crazy or call such a suit
frivolous, but that does not change the fact that the court is what
decides the matter, so having that statement does not help the user.

Another is because we are amateurs.  Philip, you may or may not be a
lawyer yourself,


Correct, but as an Engineer I do get to review terms & conditions and 
specifications quite often..



   but I know you are not _our_ lawyer.  An amateurish
"interpretation" or "clarification" does not necessarily clarify the
text but it muddies it, especially when done carelessly.  Imagine a
case where a user creates a derived work of Git itself and stored it
in a Git repository.



 "Your respository data created by Git is not
subject to Git's GNU2"--really?  At least the phrasing must say that
the act of storing something in Git alone would not *MAKE* that
something governed under GPLv2.


I can see the potential double meaning now you highlight it - I was thinking 
of the 'if it's _your_ data, you can choose'; however if it's not your data, 
the originator's restrictions would apply - that wasn't said.



What the user puts in Git may
already be covered under GPLv2 for other reasons, and a statement
carelessly written like the above can be twisted to read as if we
are endorsing use of our code outside GPLv2 as long as they store it
in Git repository, which is not what you meant to say, but "that is
not what the copyright holder meant" is another thing the lawyer
need to argue in court to convince the judge, when we need to go
after a real copyright violator.

We should leave the lawyering to real lawyers and we should not add
unnecessary work of interpreting our amateurish loose statement to
our laywers.


Given Jonathan's question, and your earlier feedback, it did feel that a bit 
of clear blue water would be useful between Git (the DVCS), and /.git/ (the 
repo contents), even if it were only to clarify the issues...




Thanks.


--
Philip 


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3] config: make git_config_set die on failure

[Patrick Steinhardt]

Failure to write the configuration file may be caused by multiple
conditions, the most common one being the case where the
configuration is locked because of a leftover lock file or
because another process is currently writing to it. We used to
ignore those errors in many cases, possibly leading to
inconsistently configured repositories. More often than not git
even pretended that everything was fine and that the settings
have been applied when indeed they were not.

Handle these failures by dying by default whenever an error is
occured by the `git_config_set` family of functions. Introduce a
new set of functions `git_config_set_gently` that instead returns
an error code for the cases where we are required to explicitly
handle configuration errors.
---

In contrast to the old version ([1]) this version of the patch
introduces a set of functions `git_config_set_gently` instead of
`git_config_set_or_die`, thus going the other route of dying by
default.

It first seemed that going the default-die route instead of
or_die-wrappers might prove to be more contained and easier to
grasp. Seeing the result, though, I'm not convinced of this
anymore. This is a minimal compiling example without even caring
about test failures, which do occur now.

Previously in v2 we had a diffstat of +127/-55 lines where we now
have +88/-82, which amounts to roughly the same amount of lines
changed. The previous approach, though, was much easier to grasp
in my opinion as nearly all changes could be split up instead of
requiring one big change to make it compile. As said before, this
does not yet include the changes required for handling test
failures, thus the patch is expected to become bigger rather than
smaller.

Furthermore, I do think it's more explicit what the functions are
doing when there is a 'or_die' suffix. Without this suffix it may
be unexpected that the functions simply abort the program
whenever an error occurs.

I'd thus prefer to use the old style used in version 2. I could
try to make the second patch ([2]) a little bit smaller by
avoiding all the fuss about passing up the error code only to die
a little later.

Opinions?

Patrick

[1]: http://article.gmane.org/gmane.comp.version-control.git/285000
[2]: http://thread.gmane.org/gmane.comp.version-control.git/285002

 builtin/branch.c |  2 +-
 builtin/clone.c  |  2 +-
 builtin/config.c | 28 +++
 builtin/remote.c | 70 ++--
 cache.h  | 12 ++
 config.c | 46 ++---
 submodule.c  | 10 
 7 files changed, 88 insertions(+), 82 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index 3f6c825..461eebb 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -595,7 +595,7 @@ static int edit_branch_description(const char *branch_name)
strbuf_stripspace(, 1);
 
strbuf_addf(, "branch.%s.description", branch_name);
-   status = git_config_set(name.buf, buf.len ? buf.buf : NULL);
+   git_config_set(name.buf, buf.len ? buf.buf : NULL);
strbuf_release();
strbuf_release();
 
diff --git a/builtin/clone.c b/builtin/clone.c
index a7c8def..8c01975 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -735,7 +735,7 @@ static int checkout(void)
 
 static int write_one_config(const char *key, const char *value, void *data)
 {
-   return git_config_set_multivar(key, value ? value : "true", "^$", 0);
+   return git_config_set_multivar_gently(key, value ? value : "true", 
"^$", 0);
 }
 
 static void write_config(struct string_list *config)
diff --git a/builtin/config.c b/builtin/config.c
index adc7727..c26d6e7 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -582,7 +582,7 @@ int cmd_config(int argc, const char **argv, const char 
*prefix)
check_write();
check_argc(argc, 2, 2);
value = normalize_value(argv[0], argv[1]);
-   ret = git_config_set_in_file(given_config_source.file, argv[0], 
value);
+   ret = git_config_set_in_file_gently(given_config_source.file, 
argv[0], value);
if (ret == CONFIG_NOTHING_SET)
error("cannot overwrite multiple values with a single 
value\n"
"   Use a regexp, --add or --replace-all to change 
%s.", argv[0]);
@@ -592,23 +592,23 @@ int cmd_config(int argc, const char **argv, const char 
*prefix)
check_write();
check_argc(argc, 2, 3);
value = normalize_value(argv[0], argv[1]);
-   return git_config_set_multivar_in_file(given_config_source.file,
-  argv[0], value, argv[2], 
0);
+   return 
git_config_set_multivar_in_file_gently(given_config_source.file,
+ argv[0], value, 
argv[2], 0);
}
else if (actions == ACTION_ADD) {

Re: [PATCH] completion: verify-tag is not plumbing

[John Keeping]

On Sun, Jan 31, 2016 at 02:37:59PM +0100, SZEDER Gábor wrote:
> 
> Quoting John Keeping :
> 
> > According to command-list.txt, verify-tag is an ancillary interrogator,
> > which means that it should be completed by "git verify-" in the
> > same way as verify-commit.
> >
> > Remove it from the list of plumbing commands so that it is treated as
> > porcelain and completed.
> 
> I'm not sure.  There are commands among the ancillary interrogators
> that are basically porcelains (e.g. blame), while some are more like
> plumbing (e.g. rerere, rev-parse).  In general the completion script
> supports the former but not the latter commands.
> 
> Now, the real porcelain-ish way to verify a tag is via 'git tag
> -v|--verify', and according to a925c6f165a3 (bash: Classify more
> commends out of completion., 2007-02-04), the commit removing
> verify-tag from the completed commands, verify-tag was kept around for
> backwards compatibility reasons.  OTOH verify-commit was introduced in
> d07b00b7f31d (verify-commit: scriptable commit signature verification,
> 2014-06-23), and as the subject line states it was intended more as a
> plumbing command.
> 
> So I think we should keep excluding verify-tag from the list of
> porcelain commands in the completion script, and it was an oversight
> not to exclude verify-commit as well when it was introduced.

I can accept that argument about verify-commit and verify-tag, but
listing verify-tag as plumbing is incorrect according to
command-list.txt (and thus git(1)).  If we're going to classify
commands, shouldn't we be consistent in how we do so?

> > Signed-off-by: John Keeping 
> > ---
> >  contrib/completion/git-completion.bash | 1 -
> >  1 file changed, 1 deletion(-)
> >
> > diff --git a/contrib/completion/git-completion.bash
> > b/contrib/completion/git-completion.bash
> > index 51f5223..250788a 100644
> > --- a/contrib/completion/git-completion.bash
> > +++ b/contrib/completion/git-completion.bash
> > @@ -728,7 +728,6 @@ __git_list_porcelain_commands ()
> > write-tree)   : plumbing;;
> > var)  : infrequent;;
> > verify-pack)  : infrequent;;
> > -   verify-tag)   : plumbing;;
> > *) echo $i;;
> > esac
> > done
> > --
> > 2.7.0
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC/PATCH] Git doc: GPL2 does not apply to repo data

[Philip Oakley]

From: "Philip Oakley" 


We should leave the lawyering to real lawyers and we should not add
unnecessary work of interpreting our amateurish loose statement to
our laywers.


Given Jonathan's question, and your earlier feedback, it did feel that a 
bit of clear blue water would be useful between Git (the DVCS), and /.git/ 
(the repo contents), even if it were only to clarify the issues...


s/if it were/if the discussions were/

I was *not* meaning that 'it' meant a doc patch.
sorry for the lack of clarity there.


--
Philip
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html



--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] git-gui--askpass: generalize the window title

[Sebastian Schuberth]

From: Sebastian Schuberth 

git-gui--askpass is not only used for SSH authentication, but also for
HTTPS. In that context it is confusing to have a window title of
"OpenSSH". So generalize the title so that it also says which parent
process, i.e. Git, requires authentication.

Signed-off-by: Sebastian Schuberth 
---
 git-gui/git-gui--askpass | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/git-gui/git-gui--askpass b/git-gui/git-gui--askpass
index 4277f30..1e5c325 100755
--- a/git-gui/git-gui--askpass
+++ b/git-gui/git-gui--askpass
@@ -60,7 +60,7 @@ proc finish {} {
set ::rc 0
 }
 
-wm title . "OpenSSH"
+wm title . "Git Authentication"
 tk::PlaceWindow .
 vwait rc
 exit $rc

--
https://github.com/git/git/pull/195
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Git for Windows

[Aaron Gray]

Hi,

I am using Windows 10 and am getting "The signature for
git-2.7.0-64-bit.exe is corrupt or invalid" !

Same for the 32bit version.

Regards,

Aaron
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] test-lib: limit the output of the yes utility

[Johannes Sixt]

On Windows, there is no SIGPIPE. A consequence of this is that the
upstream process of a pipe does not notice the death of the downstream
process until the pipe buffer is full and writing more data returns an
error. This behavior is the reason for an annoying delay during the
execution of t7610-mergetool.sh: There are a number of test cases where
'yes' is invoked upstream. Since the utility is basically an endless
loop it runs, on Windows, until the pipe buffer is full. This does take
a few seconds.

The test suite has its own implementation of 'yes'. Modify it to produce
only a limited amount of output that is sufficient for the test suite.
The amount chosen should be sufficiently high for any test case, assuming
that future test cases will not exaggerate their demands of input from
an upstream 'yes' invocation.

Signed-off-by: Johannes Sixt 
---
 This does not fix an error, but only an unnecessary sink of CPU cycles
 and wasted wall clock time.

 t/test-lib.sh | 14 +++---
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index bd4b02e..97e6491 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -902,15 +902,15 @@ fi
 yes () {
if test $# = 0
then
-   y=y
+   set -- y
else
-   y="$*"
+   set -- "$*"
fi
-
-   while echo "$y"
-   do
-   :
-   done
+   # we do not need an infinite supply of output for tests
+   set -- "$@" "$@" "$@" "$@"  # 4
+   set -- "$@" "$@" "$@" "$@"  # 16
+   set -- "$@" "$@" "$@" "$@"  # 64
+   printf "%s\n" "$@"
 }
 
 # Fix some commands on Windows
-- 
2.7.0.118.g90056ae

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] travis-ci: run previously failed tests first, then slowest to fastest

[Torsten Bögershausen]

On 2016-02-01 19.17, Junio C Hamano wrote:
> Clemens Buchacher  writes:
[]
> 
> IIRC, autocrlf=true would strip CR at the end of line in to-git
> conversion, and would add CR in to-worktree conversion.  So some eol
> conversion may only act in to-git, but some others do affect both,
> and without needing you to touch attributes.
That depends, which version of Git you are running.
It has changed from the first version of core.autocrlf:

commit c4805393d73425a5f467f10fa434fb99bfba17ac
Author: Finn Arne Gangstad 
Date:   Wed May 12 00:37:57 2010 +0200

autocrlf: Make it work also for un-normalized repositories

Previously, autocrlf would only work well for normalized
repositories. Any text files that contained CRLF in the repository
would cause problems, and would be modified when handled with
core.autocrlf set.

Change autocrlf to not do any conversions to files that in the
repository already contain a CR. git with autocrlf set will never
create such a file, or change a LF only file to contain CRs, so the
(new) assumption is that if a file contains a CR, it is intentional,
and autocrlf should not change that.

The following sequence should now always be a NOP even with autocrlf
set (assuming a clean working directory):

git checkout 
touch *
git add -A .(will add nothing)
git commit  (nothing to commit)

Previously this would break for any text file containing a CR.

Some of you may have been folowing Eyvind's excellent thread about
trying to make end-of-line translation in git a bit smoother.

I decided to attack the problem from a different angle: Is it possible
to make autocrlf behave non-destructively for all the previous problem 
cases?

Stealing the problem from Eyvind's initial mail (paraphrased and
summarized a bit):

1. Setting autocrlf globally is a pain since autocrlf does not work well
   with CRLF in the repo
2. Setting it in individual repos is hard since you do it "too late"
   (the clone will get it wrong)
3. If someone checks in a file with CRLF later, you get into problems again
4. If a repository once has contained CRLF, you can't tell autocrlf
   at which commit everything is sane again
5. autocrlf does needless work if you know that all your users want
   the same EOL style.

I belive that this patch makes autocrlf a safe (and good) default
setting for Windows, and this solves problems 1-4 (it solves 2 by being
set by default, which is early enough for clone).

I implemented it by looking for CR charactes in the index, and
aborting any conversion attempt if this is found.
---
And my intention is to do a similar fix for the attributes.
More patches coming.


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Plans for 2.7.1?

[Johannes Schindelin]

Hi Junio,

On Mon, 1 Feb 2016, Junio C Hamano wrote:

> Johannes Schindelin  writes:
> 
> > at tinyurl.com/gitCal I see a pretty timeline regarding 2.8.0, but I
> > do not see 2.7.1 planned anywhere.
> 
> Yup, because maintenance releases are inherently "not planned" ;-)

Of course, I know. Though as you dropped a hint that it might be imminent
in http://article.gmane.org/gmane.comp.version-control.git/283579 I
thought it might be in that calendar somewhere.

> Let me see what are slated for 'maint' in the current draft release
> notes.
>
> [...]
> 
> I would want to see jk/list-tag-2.7-regression and ew/
> svn-1.9.0-auth topics also in 2.7.x track soonish, but they
> currently are still in 'next', so perhaps late this week or early
> next week?

No rush. I'll just do a 2.7.0(2) today.

Thanks for the detailed information!
Dscho
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] t6302: drop unnecessary GPG requirement

[Johannes Schindelin]

Hi Junio,

On Mon, 1 Feb 2016, Junio C Hamano wrote:

> Johannes Schindelin  writes:
> 
> > An even easier solution might be to *not* set up the signed tags in the
> > 'setup' part, but only in the respective test case, and delete them right
> > away after said test case?
> 
> After reading your patch, I do not find it an "easier solution", at
> least with the definition of the word "solution" I would use.  It
> stops testing signed or doubly signed tags everywhere, assuming that
> future regressions can ever break only --points-at tests and no
> other tests around signed tags.

True.

> > Something like this (I even tested this with and without the GPG prereq):
> > ...
> > -test_expect_success 'check signed tags with --points-at' '
> > +test_expect_success GPG 'check signed tags with --points-at' '
> > +   git tag -s -m "A signed tag message" signed-tag side &&
> > +   git tag -s -m "Annonated doubly" double-tag signed-tag &&
> > +   test_when_finished git tag -d signed-tag &&
> 
> Interestingly, double-tag is not removed here.

Whooopsie.

I retract the patch in any case ;-)

Ciao,
Dscho
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC] GPG-Signed pushes & commits: differentiating between no signature and an unknown key

[Junio C Hamano]

"Robin H. Johnson"  writes:

> On Mon, Feb 01, 2016 at 02:49:09PM -0800,  Junio C Hamano wrote:
>> Are you talking about something other than prepare_push_cert_sha1()?
> I went and verified it, and what was reported to me was slightly wrong. Only
> some of the field are empty, notably CERT_KEY and SIGNER.
>
> Signed push with an unknown key:
> ===
> remote: No signature found
> remote: Your push was not signed with a known key.
> remote: You MUST use git push --signed with a known key.
> remote: If you just updated your key, please wait 15 minutes for sync.
> remote: git-receive-pack variables:
> remote: GIT_PUSH_CERT='1c471177906014e65e2825ee71572bf749970c16'
> remote: GIT_PUSH_CERT_KEY=''
> remote: GIT_PUSH_CERT_NONCE='1454372558-35db7be4533958f14731'
> remote: GIT_PUSH_CERT_NONCE_SLOP=''
> remote: GIT_PUSH_CERT_NONCE_STATUS='OK'
> remote: GIT_PUSH_CERT_SIGNER=''
> remote: GIT_PUSH_CERT_STATUS='N'

OK, this matches my expectation, and my earlier response to you is
consistent with the above output, so there isn't much to add to the
discussion from me.  I was primarily worried about GIT_PUSH_CERT not
being passed, but that does not seem to be the case, which is good.
We still give GIT_PUSH_CERT, which makes it possible to write a
validation hook that lazily fetches unknown keys as needed to
implement its own more advanced checks, while allowing validation
hooks that rely on a set of a-priori known keys to be written in a
less error-prone way by saying "N" for "unknown" case.

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: parse_object does check_sha1_signature but not parse_object_buffer?

[Mike Hommey]

On Mon, Feb 01, 2016 at 07:10:04PM -0800, Junio C Hamano wrote:
> Mike Hommey  writes:
> 
> > Shouldn't parse_object_buffer also do check_sha1_signature?
> 
> In general, it shouldn't; its callers are supposed to do it as
> additional check when/if needed.  Callers like the one in fsck.c
> does not want to die after seeing one bad one.  We want to report
> and keep checking other things.

Shouldn't some things like, at least, `git checkout`, still check
the sha1s, though?

Mike
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 00/12] ref-filter: use parsing functions

[Karthik Nayak]

On Tue, Feb 2, 2016 at 6:07 AM, Eric Sunshine  wrote:
> On Mon, Feb 1, 2016 at 5:25 PM, Junio C Hamano  wrote:
>> Karthik Nayak  writes:
>>
>>> This series cleans up populate_value() in ref-filter, by moving out
>>> the parsing part of atoms to separate parsing functions. This ensures
>>> that parsing is only done once and also improves the modularity of the
>>> code.
>>>
>>> v1: http://thread.gmane.org/gmane.comp.version-control.git/281180
>>> v2: http://thread.gmane.org/gmane.comp.version-control.git/282563
>>> v3: http://thread.gmane.org/gmane.comp.version-control.git/283350
>>>
>>> Changes:
>>> * The parsing functions now take the arguments of the atom as
>>> function parameteres, instead of parsing it inside the fucntion.
>>> * Rebased on top of pu:jk/list-tag-2.7-regression
>>> * In strbuf use a copylen variable rather than using multiplication
>>> to perform a logical operation.
>>> * Code movement for easier review and general improvement.
>>> * Use COLOR_MAXLEN as the maximum size for the color variable.
>>> * Small code changes.
>>> * Documentation changes.
>>> * Fixed incorrect style of test (t6302).
>>>
>>> Karthik Nayak (12):
>>>   strbuf: introduce strbuf_split_str_omit_term()
>>>   ref-filter: use strbuf_split_str_omit_term()
>>>   ref-filter: bump 'used_atom' and related code to the top
>>>   ref-filter: introduce struct used_atom
>>>   ref-filter: introduce parsing functions for each valid atom
>>>   ref-filter: introduce color_atom_parser()
>>>   ref-filter: introduce parse_align_position()
>>>   ref-filter: introduce align_atom_parser()
>>>   ref-filter: align: introduce long-form syntax
>>>   ref-filter: introduce remote_ref_atom_parser()
>>>   ref-filter: introduce contents_atom_parser()
>>>   ref-filter: introduce objectname_atom_parser()
>>
>> Hmm, 10/12 didn't make it to the list?
>
> Not surprising. Somehow, Karthik did git-add on the compiled
> test-fake-ssh before committing, so the patch sent to the list
> contains an rather huge binary resource. I did receive it since I was
> a direct addressee of the email; I'll reply to the message on-list
> without modifying anything (other than stripping out the binary
> resource) so that other reviewers get a chance to see it.

Thanks Eric, i didn't notice doing that.

-- 
Regards,
Karthik Nayak
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 6/6] apply, ls-files: simplify "-z" parsing

[Jeff King]

On Mon, Feb 01, 2016 at 02:52:30PM -0800, Junio C Hamano wrote:

> Junio C Hamano  writes:
> 
> > Of course, a patch adding a "--nul" can be the one that does the
> > polarity flipping, so in that sense, this simplification is probably
> > OK, as long as there is some comment that warns a time-bomb you just
> > planted here ;-)
> 
> I'll queue it with this tweak for now.
> 
> The idea is to have them run "blame" to find the last paragraph of
> the commit log message.

That looks like a good compromise. Thanks.

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 6/6] worktree add: switch to worktree version 1

[Max Kirillov]

On Mon, Feb 01, 2016 at 01:05:05PM +0700, Duy Nguyen wrote:
> On Mon, Feb 1, 2016 at 12:33 PM, Max Kirillov  wrote:
>> 1. For submodules (which must be left per-worktree) this
>> approach is not going to work, because you don't know all
>> variables in advance. You could scan the config file and
>> match those actual keys which are there with patterns.

> Hmm.. we could keep existing submodule.* per-worktree. New variables
> are per-worktree by default, unless you do "git config --repo" in
> git-submodule.sh. Am I missing something?

Submodules in new worktree should be not initialized, and as
far as I understand this means that submodule variables
should be removed from common config.

I used test from
http://article.gmane.org/gmane.comp.version-control.git/266621
to verify expectations for submodules.

>> 2. This migrates variables to the default (or current?)
>> worktree, what about others existing?
> 
> In v0, $C/config contains all shared variables, once we move these
> shared vars to $C/common/config, they will be visible to all other
> worktrees. Or do you replicate per-worktree vars in $C/config to all
> worktrees ?

If would make sense for some variables definitely. For
example, the submodule related variables.

-- 
Max
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 1/6] worktree: new repo extension to manage worktree behaviors

[Dennis Kaarsemaker]

On wo, 2016-01-27 at 14:12 -0800, Junio C Hamano wrote:

> More seriously, are we confident that the overall worktree support
> is mature enough by now that once we add an experimental feature X
> at version 1, we can promise to keep maintaining it forever at
> version N for any positive integer N?  I hate to sound overly
> negative, but I am getting an impression that we are not quite
> there, and it is still not ready for production use.
> 
> It would be beneficial both for us and our users if we can keep our
> hand untied for at least several more releases to allow us try
> various random experimental features, fully intending to drop any of
> them if the ideas do not pan out.

So far I have two use cases for separate worktrees and am a happy user:

- A CI setup that tries to avoid cloning a repository too often. It
  does N independent tasks in parallel in separate worktrees. This
  checks out the same commit multiple times in multiple worktrees.

- Quickly checking out another branch/commit without first having to
  stash all uncommitted work.

Neither of those require much specialness, so I'm more than happy to
see things change for the better as we find out more of the edge cases.
One thing that may benefit especially the former is a 'git worktree rm'
which removes the worktree (iff there are no local changes) and prunes
it, but nothing in the current implementation or proposed changes will
stop the addition of that.

-- 
Dennis Kaarsemaker
www.kaarsemaker.net


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] travis-ci: run previously failed tests first, then slowest to fastest

[Junio C Hamano]

Clemens Buchacher  writes:

> Ok, then let's take a step back. I do not actually care if git diff and
> friends say the worktree is clean or not.

You may not, but many existing scripts people have do.

> But I know that I did not make
> any modifications to the worktree, because I just did git reset --hard.
> And now I want to use commands like cherry-pick and checkout without
> failure. But they can fail, because they essentially use git diff to
> check if there are worktree changes, and if so refuse to overwrite them.

Yes, exactly.

> So, if the check "Am I allowed to modify the worktree file?", would go
> the extra mile to also check if the worktree is clean in the sense that
> convert_to_worktree(index state) matches the worktree. If this is the
> case, then it is safe to modify the file because it is the committed
> state, and can be recovered.

So in essense, the proposed "fix" is "let's fix it in the right
way"?

The way we defined "would we lose some changes that are only in the
working tree?", aka "is the working tree dirty wrt the index?", has
been to check if "git add -u" would change the states in the index.
And for scripted Porcelains and end-user scripts, "git diff-files",
aka "what change would 'git add -u' make to the states in the
index?", has been the command to do the same check.

Your proposal is to redefine "is the working tree dirty?"; it would
check if "git checkout -f" would change what is in the working tree.

I agree that indeed is "would we lose some changes that are only in
the working tree", and I think we can do that transparently for
"internal" commands, i.e. without any end-user impact, as the new
check would behave identically when they have sane contents--the
difference between the current check and the new check only exists
when the contents in the index contradicts what the user specifies
for to-git conversion via eol or clean filter.

We would need a way for our scripted Porcelains and end-user scripts
to ask that new question, though, but I think that is not something
insurmountable.  A new option to "diff-files" or something, perhaps,
would be workable, but having a new "git require-clean-work-tree"
plumbing, which would replace require_clean_work_tree shell helper
in git-sh-setup, may be conceptually much cleaner, because the new
definition of "working tree being clean" is no longer tied to what
"diff" should say.

I like that as a general direction.

> Regarding performance impact: We only need to do this extra check if the
> usual check convert_to_git(work tree) against index state fails, and
> conversion is in effect.

How would you detect the failure, though?  Having contents in the
index that contradicts the attributes and eol settings affects the
cleanliness both ways.  Running the working tree contents via to-git
conversion and hashing may match the blob in the index, declaring
that the index entry is "clean", but passing the blob to to-worktree
conversion may produce result different from what is in the
worktree, which is "falsely clean".  That is an equally important
case that is opposite from what we have been primarily discussing,
which is "falsely dirty".

>> Besides, I do not think the above approach really solves the issue,
>> either.  After "git reset --hard" to have the contents in the index
>> dumped to the working tree, if your core.autocrlf is flipped,
>
> Indeed, if the user configuration changes, then we cannot always detect
> this (at least if the filter is an external program, and the behavior of
> that changes). But the user is in control of that, and we can document
> this limitation.

That argument does not result in a very useful result, though.
Because the user is in control of what attributes and eol settings
are in effect in her repository, we can just document that the
current check will give unspecified result if the indexed contents
contradict with that setting, e.g. when you have CRLF encoded data
in the index but the eol conversion assumes LF in the repository.
But this discussion is an attempt to do better than that, no?

> On the other hand, a user who simply follows an upstream repository by
> doing git pull all the time, and who does not make changes to their
> configuration, can still run into this issue, because upstream could
> change .gitattributes. This part we could actually detect by hashing the
> attributes for each index entry, and if that changes we re-evaluate the
> file state.

If this has to bloat each index entry, I do not think solving the
problem is worth that cost of that overhead.  I'd rather just say
"if you have inconsistent data, here is a workaround using 'reset'
and then 'reset --hard'" and be done with it.

> This is also an issue only if a smudge filter is in place. The eol
> conversion which only acts in the convert_to_git direction is not
> affected.

IIRC, autocrlf=true would strip CR at the end of line in to-git
conversion, and would add CR in to-worktree conversion.  So some eol

Re: [PATCH v3 1/6] worktree: new repo extension to manage worktree behaviors

[Junio C Hamano]

Duy Nguyen  writes:

> On Mon, Feb 1, 2016 at 9:41 AM, Stefan Monnier  
> wrote:
>>> One lessor key phrase above is "so far", I think, and another one
>>> you forgot to use is s/which requires/that we know &/, which to me
>>> is a more serious one.  IOW, I do think it is premature for us to
>>> say that that config split issue is the only thing, or to say that
>>> the issue is best solved by changing the layout in the way being
>>> discussed; the multiple-worktree feature needs more lab experience
>>> for us to gain confidence.
>>
>> As a heavy user of the git-new-worktree "hack / script", is there
>> something I can do to help "get more experience"?
>
> You can try out "git worktree" command (in "lab" environment) and see
> what's missing, what use cases of yours it does not support.

Yup, that would be very helpful.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Plans for 2.7.1?

[Johannes Schindelin]

Hi Junio,

at tinyurl.com/gitCal I see a pretty timeline regarding 2.8.0, but I do
not see 2.7.1 planned anywhere.

Due to signature problems (I failed to realize that SHA-1 based .exe
signatures are no longer considered valid starting from January 1st,
2016), I got a metric ton of private and non-private bug reports regarding
"corrupt signatures", and therefore I would like to prevent those reports
from taking over my entire working hours by simply issuing a new release
of Git for Windows.

Is 2.7.1 around the corner? Otherwise I'll just make a 2.7.0(2).

Thanks,
Dscho
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Git for Windows

[Johannes Schindelin]

Hi Aaron,

On Mon, 1 Feb 2016, Aaron Gray wrote:

> I am using Windows 10 and am getting "The signature for
> git-2.7.0-64-bit.exe is corrupt or invalid" !
> 
> Same for the 32bit version.

See https://github.com/git-for-windows/git/issues/592

Ciao,
Johannes
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html