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

[Michael Haggerty]

On 11/05/2017 07:17 AM, Junio C Hamano wrote:
> Junio C Hamano  writes:
>> Rafael Ascensão  writes:
>> ...
>>> Because changing the default behavior of that function has
>>> implications on multiple commands which I think shouldn't change. But
>>> at the same time, would be nice to have the logic that deals with
>>> glob-ref patterns all in one place.
>>>
>>> What's the sane way to do this?
>>
>> Learn to type "--decorate-refs="refs/heads/[m]aster", and not twewak
>> the code at all, perhaps.  The users of existing "with no globbing,
>> /* is appended" interface are already used to that way and they do
>> not have to learn a new and inconsistent interface.
>>
>> After all, "I only want to see 'git log' output with 'master'
>> decorated" (i.e. not specifying "this class of refs I can glob by
>> using the naming convention I am using" and instead enumerating the
>> ones you care about) does not sound like a sensible thing people
>> often want to do, so making it follow the other codepath so that
>> people can say "refs/tags" to get "refs/tags/*", while still allowing
>> such a rare but specific and exact one possible, may not sound too
>> bad to me.
> 
> Having said all that, I can imagine another way out might be to
> change the behaviour of this "normalize" thing to add two patterns,
> the original pattern in addition to the original pattern plus "/*",
> when it sees a pattern without any glob.  Many users who relied on
> the current behaviour fed "refs/tags" knowing that it will match
> everything under "refs/tags" i.e. "refs/tags/*", and they cannot
> have a ref that is exactly "refs/tags", so adding the original
> pattern without an extra trailing "/*" would not hurt them.  And
> this will allow you to say "refs/heads/master" when you know you
> want that exact ref, and in such a repository where that original
> pattern without trailing "/*" would be useful, because you cannot
> have "refs/heads/master/one" at the same time, having an extra
> pattern that is the original plus "/*" would not hurt you, either.
> 
> This however needs a bit of thought to see if there are corner cases
> that may result in unexpected and unwanted fallout, and something I
> am reluctant to declare unilaterally that it is a better way to go.

There's some glob-matching code (somewhere? I don't know if it's allowed
everywhere) that allows "**" to mean "zero or one path components. If
"refs/tags" were massaged to be "refs/tags/**", then it would match not only

refs/tags
refs/tags/foo

but also

refs/tags/foo/bar

, which is probably another thing that the user would expect to see.

There's at least some precedent for this kind of expansion: `git
for-each-ref refs/remotes` lists *all* references under that prefix,
even if they have multiple levels.

Michael

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

[Michael Haggerty]

On 11/06/2017 02:23 AM, Junio C Hamano wrote:
> Michael Haggerty  writes:
> 
>> [1] I say "almost entirely" because putting them in one function means
>> that only `pattern` needs to be scanned for glob characters. But that is
>> an unimportant detail.
> 
> That could actually be an important detail, in that even if prefix
> has wildcard, we'd still append the trailing "/*" as long as the
> pattern does not, right?

That's correct, but I was assuming that the prefix would always be a
hard-coded string like "refs/tags/" or maybe "refs/". (That is the case
now.) It doesn't seem very useful to use a prefix like "refs/*/".

Michael

benefits of "/" in branch names other than the obvious?

[Robert P. J. Day]

  another picky question ... are there any other benefits of
supporting slashes in branch names other than the obvious ones i know
about?

1) visually, let's you *imagine* a branch hierarchy, and
2) allows wildcarding on a full "component" basis

  are there any other benefits of the above? thank you kindly.

rday

-- 


Robert P. J. Day Ottawa, Ontario, CANADA
http://crashcourse.ca

Twitter:   http://twitter.com/rpjday
LinkedIn:   http://ca.linkedin.com/in/rpjday

Re: what is the function of .git/branches/?

[Robert P. J. Day]

On Mon, 6 Nov 2017, Junio C Hamano wrote:

> "Robert P. J. Day"  writes:
>
> >   currently proofing "pro git" book, and an example of a new repo
> > doesn't show a .git/branches/ directory, but initializing a new
> > repo with current version of 2.13.6 *does* show an initially empty
> > directory by that name. however, AFAICT, branches are still
> > tracked under .git/refs/heads/, so what's with that branches/
> > directory?

> There are three ways to specify what branches of which remote
> repository your fetch and/or push interacts with, and having
> .git/branches/foo file is one of these three ways (the other two are
> to have .git/remotes/foo file, and to have [remote "foo"] section in
> the .git/config).

> If your workflow involves having to interact with tons of remotes
> (imagine being a maintainer who regularly pulls from dozens of
> sub-maintainer's repositories, each of which places the material to
> be upstreamed on a single branch) and that set changes from time to
> time, using .git/branches/* is a lot more efficient than having to
> keep track of the same information in other two formats, so even
> though it was the invented the earliest and is the least flexible
> format among the three, it still has its uses.

  ah, useful to know, thanks. as i mentioned, i'm clawing my way
through the current version of the "pro git" book and submitting all
sorts of updates, and this is the sort of thing i'm keeping track of
to see if i can sneak it in somewhere.

rday

-- 


Robert P. J. Day Ottawa, Ontario, CANADA
http://crashcourse.ca

Twitter:   http://twitter.com/rpjday
LinkedIn:   http://ca.linkedin.com/in/rpjday

Re: [PATCH 3/3] Documentation: revisions: add note about 3dots usages as continuation indications

[Junio C Hamano]

Ann T Ropea  writes:

> Also, fix typo: "three dot" ---> "three-dot" (align with "two-dot").
>
> Signed-off-by: Ann T Ropea 
> ---
>  Documentation/revisions.txt | 11 ++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/revisions.txt b/Documentation/revisions.txt
> index 61277469c874..d1b126427177 100644
> --- a/Documentation/revisions.txt
> +++ b/Documentation/revisions.txt
> @@ -271,7 +271,7 @@ The '..' (two-dot) Range Notation::
>   for commits that are reachable from r2 excluding those that are reachable
>   from r1 by '{caret}r1 r2' and it can be written as 'r1..r2'.
>  
> -The '...' (three dot) Symmetric Difference Notation::
> +The '...' (three-dot) Symmetric Difference Notation::
>   A similar notation 'r1\...r2' is called symmetric difference
>   of 'r1' and 'r2' and is defined as
>   'r1 r2 --not $(git merge-base --all r1 r2)'.
> @@ -285,6 +285,15 @@ is a shorthand for 'HEAD..origin' and asks "What did the 
> origin do since
>  I forked from them?"  Note that '..' would mean 'HEAD..HEAD' which is an
>  empty range that is both reachable and unreachable from HEAD.
>  
> +However, there are instances where '...' is *not*
> +equivalent to '...HEAD'.  See the "RAW OUTPUT FORMAT"
> +section of linkgit:git-diff[1]: the three-dot notations used
> +there are simply continuation indications for the abbreviated
> +SHA-1 values.  The ones encountered there are usually
> +associated with file/index/tree contents rather than with commit
> +objects, and the range operators described above are only
> +applicable to commit objects (i.e., 'r1' and 'r2').
> +
>  Other {caret} Parent Shorthand Notations
>  ~
>  Three other shorthands exist, particularly useful for merge commits,

I actually have a mild suspicion that this is going in a wrong
direction.  In very early days of Git, we wanted to make sure that
people can tell if a long hex string is a truncated object name or a
full one (mostly because some lower-level commands insisted to be
fed only full object name).

These days, everybody knows when they see 79ec0be62a that it is
*not* a full object name and will no longer be confused unlike early
days and there is no strong reason to waste six output columns of
"git diff --raw" output by using these three dots.  

I wonder if we can come up with a solution in line with the patch
1/3 in this series, which got rid of the "..." that indicated that
the hexstring was not a full object name.

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

[Junio C Hamano]

Rafael Ascensão  writes:

> Would checking the output of ref_exists() make sense here?
> By that I mean, only add a trailing '/*' if the ref doesn't exist.

I do not think it would hurt, but it is not immediately obvious if
the benefit of doing so outweighs the cost of having to make an
extra call to ref_exists().

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

[Rafael Ascensão]

Would checking the output of ref_exists() make sense here?
By that I mean, only add a trailing '/*' if the ref doesn't exist.

Unless I am missing something obvious this would allow us to keep both
shortcuts and exact patterns.

Re: [PATCH 1/3] checkout: describe_detached_head: remove 3dots after committish

[Junio C Hamano]

Ann T Ropea  writes:

> This could be confusing not only for novices; in either case, no range
> should be insinuated by describe_detached_head.

We actually do not insinuate any range in these output.  These dots
denote "truncated at the end, instead of giving full length."

Another place these "many dots" appear is "git diff --raw", for
example.

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

[Rafael Ascensão]

On 06/11/17 01:23, Junio C Hamano wrote:
> Michael Haggerty  writes:
> 
>> [1] I say "almost entirely" because putting them in one function means
>> that only `pattern` needs to be scanned for glob characters. But that is
>> an unimportant detail.
> 
> That could actually be an important detail, in that even if prefix
> has wildcard, we'd still append the trailing "/*" as long as the
> pattern does not, right?
> 

> So the interface might be simplified by having two functions,
> 
> void normalize_glob_ref(normalized_pattern, prefix, pattern);
> void ensure_blob(struct strbuf *pattern);

I think that flag no longer makes sense. I added it just to allow
'--decorate-refs' work with "exact patterns". And since that has the
ugly side effect of losing the ability to use "shortcut patterns" like
'tags' to refer to 'refs/tags/*', I believe it's a good idea to remove it.

Re: [PATCH] fix typos in 2.15.0 release notes

[Junio C Hamano]

Jean Carlo Machado  writes:

> Signed-off-by: Jean Carlo Machado 
> ---
>  Documentation/RelNotes/2.15.0.txt | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)

It is pretty much pointless to update release notes after a release
is made, as the fixed version will not be seen as many people as it
should be.  So the next time it would be appreciated 10x if you send
such a patch before the release happens.

Will queue for the 'maint' track.  Thanks.

Re: [RFC PATCH v3 4/4] builtin/branch: give more useful error messages when renaming

[Junio C Hamano]

Kaartic Sivaraam  writes:

> diff --git a/builtin/branch.c b/builtin/branch.c
> index 7018e5d75..c2bbf8c3d 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -458,11 +458,42 @@ static void reject_rebase_or_bisect_branch(const char 
> *target)
>   free_worktrees(worktrees);
>  }
>  
> +static void get_error_msg(struct strbuf* error_msg, const char* oldname, 
> unsigned old_branch_exists,
> +   const char* newname, enum branch_validation_result 
> res)
> +{
> + const char* connector_string = _(", and ");
> +
> + if (!old_branch_exists) {
> + strbuf_addf(error_msg, _("branch '%s' doesn't exist"), oldname);
> + }

No {} around a single statement block of "if", especially when there
is no "else" that has multi-statement block that needs {}.

> + switch (res) {
> + case BRANCH_EXISTS_NO_FORCE:
> + strbuf_addf(error_msg, "%s", (!old_branch_exists) ? 
> connector_string : "");
> + strbuf_addf(error_msg,_("branch '%s' already exists"), 
> newname);
> + break;

The case arms and their statements are indented by one level too much.
The lines are getting overlong.  Find a good place to split, e.g.

strbuf_addf(error_msg, "%s",
!old_branch_exists ? connector_string : "");

Leave a single SP after each "," in an arguments list.

As Eric pointed out, this certainly smells like a sentence lego that
we would be better off without.

>  static void copy_or_rename_branch(const char *oldname, const char *newname, 
> int copy, int force)
>  {
>   struct strbuf oldref = STRBUF_INIT, newref = STRBUF_INIT, logmsg = 
> STRBUF_INIT;
>   struct strbuf oldsection = STRBUF_INIT, newsection = STRBUF_INIT;
>   int recovery = 0;
> + struct strbuf error_msg = STRBUF_INIT, empty = STRBUF_INIT;
> + enum branch_validation_result res;
>  
>   if (!oldname) {
>   if (copy)
> @@ -471,15 +502,13 @@ static void copy_or_rename_branch(const char *oldname, 
> const char *newname, int
>   die(_("cannot rename the current branch while not on 
> any."));
>   }
>  
> - if (strbuf_check_branch_ref(, oldname)) {
> + if (strbuf_check_branch_ref(, oldname) && ref_exists(oldref.buf))
> + {

Opening brace { that begins a block comes at the end of the line
that closes the condition of "if"; if you found that your line is
overlong, perhaps do it like so instead:

if (strbuf_check_branch_ref(, oldname) &&
ref_exists(oldref.buf)) {

Re: [RFC PATCH v3 3/4] branch: introduce dont_fail parameter for branchname validation

[Junio C Hamano]

Kaartic Sivaraam  writes:

> This parameter allows the branchname validation functions to
> optionally return a flag specifying the reason for failure, when
> requested. This allows the caller to know why it was about to die.
> This allows more useful error messages to be given to the user when
> trying to rename a branch.
>
> The flags are specified in the form of an enum and values for success
> flags have been assigned explicitly to clearly express that certain
> callers rely on those values and they cannot be arbitrary.
>
> Only the logic has been added but no caller has been made to use it, yet.
> So, no functional changes.

This step makes sense, and nicely done.

We usually use the word "gently" to when we enhance an operation
that used to always die on failure.  When there are tons of callsite
to the original operation F(), we introduce F_gently() variant and
do something like

F(...)
{
if (F_gently(...))
die(...);
}

so that the callers do not have to change.  If there aren't that
many, it is OK to change the function signature of F() to tell it
not to die without adding a new F_gently() function, which is the
approach more appropriate for this change.  The extra parameter used
for that purpose should be called "gently", perhaps.

> + if(ref_exists(ref->buf))
> + return BRANCH_EXISTS;
> + else
> + return BRANCH_DOESNT_EXIST;

Always have SP between "if" (and other keyword like "while") and its
condition.

For this one, however, this might be easier to follow:

return ref_exists(ref->buf) ? BRANCH_EXISTS : BRANCH_DOESNT_EXIST;

The names of the enum values may need further thought.  They must
clearly convey two things, in addition to what kind of status they
represent; the current names only convey the status.  From the names
of these values, it must be clear that they are part of the same
enum (e.g. by sharing a common prefix), and also from the names of
these values, it must be clear which ones are error conditions and
which are not, without knowing their actual values.  A reader cannot
immediately tell from "BRANCH_EXISTS" if that is a good thing or
not.

Other than that, looks fairly cleanly done.  I like what this step
wants to achieve.

Thanks.

Re: [RFC PATCH v3 2/4] branch: re-order function arguments to group related arguments

[Junio C Hamano]

Kaartic Sivaraam  writes:

> From: Kaartic Sivaraam 
>
> The ad-hoc patches to add new arguments to a function when needed
> resulted in the related arguments not being close to each other.
> This misleads the person reading the code to believe that there isn't
> much relation between those arguments while it's not the case.

I do not get what this wants to say.  "I am sending this ad-hoc
patch that scrambles the order of parameters for no real reason" is
certainly not how you are selling this step.

> So, re-order the arguments to keep the related arguments close to each
> other.

This sentence (without "So,", obviously, because I do not get the
previous paragraph) I do understand and agree with as a goal.

I think the only two things that should be kept together are "force"
and "clobber_head_ok" because the previous 1/4 changed the meaning
of "clobber_head" to "it is OK if I am renaming the currently
checked-out branch", i.e. closer to what "force" means.

I certainly would not mind the order used in the result of this
patch (in other words, if somebody posted a patch to add
create_branch() function to the codebase that lacked it, with its
parameters listed in the order this patch uses, I wouldn't
complain), but it would have equally been OK if "reflog" and "force"
were swapped without making any other change this patch makes.

I dunno.

> Signed-off-by: Kaartic Sivaraam 
> ---
>  branch.c   |  4 ++--
>  branch.h   | 14 +++---
>  builtin/branch.c   |  4 ++--
>  builtin/checkout.c |  6 +++---
>  4 files changed, 14 insertions(+), 14 deletions(-)
>
> diff --git a/branch.c b/branch.c
> index ea6e2b359..7c8093041 100644
> --- a/branch.c
> +++ b/branch.c
> @@ -244,8 +244,8 @@ N_("\n"
>  "\"git push -u\" to set the upstream config as you push.");
>  
>  void create_branch(const char *name, const char *start_name,
> -int force, int reflog, int clobber_head_ok,
> -int quiet, enum branch_track track)
> +enum branch_track track, int force, int clobber_head_ok,
> +int reflog, int quiet)
>  {
>   struct commit *commit;
>   struct object_id oid;
> diff --git a/branch.h b/branch.h
> index 1512b78d1..85052628b 100644
> --- a/branch.h
> +++ b/branch.h
> @@ -11,21 +11,21 @@
>   *   - start_name is the name of the existing branch that the new branch 
> should
>   * start from
>   *
> - *   - force enables overwriting an existing (non-head) branch
> + *   - track causes the new branch to be configured to merge the remote 
> branch
> + * that start_name is a tracking branch for (if any).
>   *
> - *   - reflog creates a reflog for the branch
> + *   - force enables overwriting an existing (non-head) branch
>   *
>   *   - clobber_head_ok allows the currently checked out (hence existing)
>   * branch to be overwritten; without 'force', it has no effect.
>   *
> - *   - quiet suppresses tracking information
> + *   - reflog creates a reflog for the branch
>   *
> - *   - track causes the new branch to be configured to merge the remote 
> branch
> - * that start_name is a tracking branch for (if any).
> + *   - quiet suppresses tracking information
>   */
>  void create_branch(const char *name, const char *start_name,
> -int force, int reflog,
> -int clobber_head_ok, int quiet, enum branch_track track);
> +enum branch_track track, int force, int clobber_head_ok,
> +int reflog, int quiet);
>  
>  /*
>   * Check if 'name' can be a valid name for a branch; die otherwise.
> diff --git a/builtin/branch.c b/builtin/branch.c
> index 5be40b384..df06ac968 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -766,7 +766,7 @@ int cmd_branch(int argc, const char **argv, const char 
> *prefix)
>* create_branch takes care of setting up the tracking
>* info and making sure new_upstream is correct
>*/
> - create_branch(branch->name, new_upstream, 0, 0, 0, quiet, 
> BRANCH_TRACK_OVERRIDE);
> + create_branch(branch->name, new_upstream, 
> BRANCH_TRACK_OVERRIDE, 0, 0, 0, quiet);
>   } else if (unset_upstream) {
>   struct branch *branch = branch_get(argv[0]);
>   struct strbuf buf = STRBUF_INIT;
> @@ -806,7 +806,7 @@ int cmd_branch(int argc, const char **argv, const char 
> *prefix)
>   die(_("the '--set-upstream' option is no longer 
> supported. Please use '--track' or '--set-upstream-to' instead."));
>  
>   create_branch(argv[0], (argc == 2) ? argv[1] : head,
> -   force, reflog, 0, quiet, track);
> +   track, force, 0, reflog, quiet);
>  
>   } else
>   usage_with_options(builtin_branch_usage, options);
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index 

Re: what is the function of .git/branches/?

[Junio C Hamano]

"Robert P. J. Day"  writes:

>   currently proofing "pro git" book, and an example of a new repo
> doesn't show a .git/branches/ directory, but initializing a new repo
> with current version of 2.13.6 *does* show an initially empty
> directory by that name. however, AFAICT, branches are still tracked
> under .git/refs/heads/, so what's with that branches/ directory?

There are three ways to specify what branches of which remote
repository your fetch and/or push interacts with, and having
.git/branches/foo file is one of these three ways (the other two are
to have .git/remotes/foo file, and to have [remote "foo"] section in
the .git/config).

If your workflow involves having to interact with tons of remotes
(imagine being a maintainer who regularly pulls from dozens of
sub-maintainer's repositories, each of which places the material to
be upstreamed on a single branch) and that set changes from time to
time, using .git/branches/* is a lot more efficient than having to
keep track of the same information in other two formats, so even
though it was the invented the earliest and is the least flexible
format among the three, it still has its uses.

Re: [PATCH 1/7] files_transaction_prepare(): don't leak flags to packed transaction

[Junio C Hamano]

Michael Haggerty  writes:

>> That much I can understand.  But it is not explained why (1) we do
>> not pass old_oid anymore and (2) we do give HAVE_NEW.  
>> 
>> Presumably the justification for (1) is something like "because we
>> are not passing HAVE_OLD, we shouldn't have been passing old_oid at
>> all---it was a harmless bug because lack of HAVE_OLD made the callee
>> ignore old_oid"
>
> It's debatable whether the old code should even be called a bug. The
> callee is documented to ignore `old_oid` if `HAVE_OLD` is not set. But
> it was certainly misleading to pass unneeded information to the function.
>
>> (2) is "we need to pass HAVE_NEW, and we have
>> been always passing HAVE_NEW because update->flags at this point is
>> guaranteed to have it" or something like that?
>
> Correct. `REF_DELETING` is set by `lock_ref_for_update()` only if
> `update->flags & REF_HAVE_NEW` was set, and this code is only called if
> `REF_DELETING` is set.

It is is extra nice for you to give answers that are customized for
me like the above to my questions, but the questions came because
the log message did not answer them, so please make sure the next
person who did not read this exchange but reads only the resulting
commit does not have to ask the same question.

Thanks.

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

[Junio C Hamano]

Michael Haggerty  writes:

> [1] I say "almost entirely" because putting them in one function means
> that only `pattern` needs to be scanned for glob characters. But that is
> an unimportant detail.

That could actually be an important detail, in that even if prefix
has wildcard, we'd still append the trailing "/*" as long as the
pattern does not, right?

Re: [PATCH] config: document blame configuration

[Junio C Hamano]

Stefan Beller  writes:

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

Excellent.  Will queue.  Thanks.

Re: [PATCH v2 0/2] Re: reduce_heads: fix memory leaks

[Junio C Hamano]

Martin Ågren  writes:

> Since v1 [1], I've added a preparatory patch to UNLEAK some variables.
> That sets the stage slightly better for patch 2.
>
> Junio, you placed v1 on maint. Because UNLEAK is not in maint, this is
> based on master and maint misses out on this v2. If you have any advice
> for how I should (not) do series with UNLEAK in them, I'm all ears.

As far as we know, nobody reported that these leaks made Git run out
of memory while running merge-base and prevented them from getting
desired result, so it is not worth the effort to make (part of) them
mergeable to 'maint'.  I forked the branch from 'maint' only because
it was a fix and it was not harder than forking from 'master'.

If 2/2 (which was 1/1 in the v1) were fixes to a very grave error,
then I might have suggested to do the 2/2 on maint first and call
that topic ${some_grave_error}_fix-maint; then fork another topic
${some_grave_error}_fix at master, merge the _fix-maint topic in,
and then do the 1/2 on top.

Re: [PATCH v2 1/2] builtin/merge-base: UNLEAK commit lists

[Junio C Hamano]

Martin Ågren  writes:

> In several functions, we iterate through a commit list by assigning
> `result = result->next`. As a consequence, we lose the original pointer
> and eventually leak the list.
>
> These are immediate helpers to `cmd_merge_base()` which is just about to
> return, so we can use UNLEAK. For example, we could `UNLEAK(result)`
> before we start iterating. That would be a one-liner change per
> function. Instead, leave the lists alone and iterate using a dedicated
> pointer. Then UNLEAK immediately before returning.

Hmm, I cannot shake this feeling that this goes somewhat opposite to
the spirit of UNLEAK(), which I view as "It is too cumbersome and
makes the resulting code ugly if we try to make everything properly
freed, so mark what we know we will leak upfront".  The result of
this patch feels more like "Even though we took pains to restructure
the code so that we could call free_commit_list() to properly free
things, we use UNLEAK() and do not actually bother to free."

Havin said that, I do not think the resulting code has become uglier
or the conversion process (both writing and reviewing) were too
cumbersome for this particular case, and marking where we could call
free_commit_list() with UNLEAK() like this patch does may make
sense.  If somebody someday wants to call some of these helpers in
other contexts repeatedly, they may have an easier time.

Re: [PATCH 4/7] remote-mediawiki: skip virtual namespaces

[Junio C Hamano]

Antoine Beaupré  writes:

> On 2017-11-02 10:24:40, Junio C Hamano wrote:
>> Antoine Beaupré  writes:
>>
>>> It might still worth fixing this, but I'm not sure what the process is
>>> here - in the latest "what's cooking" Junio said this patchset would be
>>> merged in "next". Should I reroll the patchset to fix this or not?
>>
>> The process is for you (the contributor of the topic) to yell at me,
>> "don't merge it yet, there still are updates to come".
>
> YELL! "don't merge it yet, there still are updates to come". :)

Thanks; heard you loud and clear.

>> That message _may_ come to late, in which case we may have to go
>> incremental, but I usually try to leave at least a few days between
>> the time I mark a topic as "will merge" and the time I actually do
>> the merge, for this exact reason.
>
> Awesome, thanks for the update.
>
> i'll roll a v4 with the last tweaks, hopefully that will be the last.

Thanks.

[PATCH v2 5/8] t0021/rot13-filter: add packet_initialize()

[Christian Couder]

Let's refactor the code to initialize communication into its own
packet_initialize() function, so that we can reuse this
functionality in following patches.

Signed-off-by: Christian Couder 
---
 t/t0021/rot13-filter.pl | 26 --
 1 file changed, 16 insertions(+), 10 deletions(-)

diff --git a/t/t0021/rot13-filter.pl b/t/t0021/rot13-filter.pl
index f31ff595fe..2f74ab2e45 100644
--- a/t/t0021/rot13-filter.pl
+++ b/t/t0021/rot13-filter.pl
@@ -127,19 +127,25 @@ sub packet_flush {
STDOUT->flush();
 }
 
+sub packet_initialize {
+   my ($name, $version) = @_;
+
+   packet_compare_lists([0, $name . "-client"], packet_txt_read()) ||
+   die "bad initialize";
+   packet_compare_lists([0, "version=" . $version], packet_txt_read()) ||
+   die "bad version";
+   packet_compare_lists([1, ""], packet_bin_read()) ||
+   die "bad version end";
+
+   packet_txt_write( $name . "-server" );
+   packet_txt_write( "version=" . $version );
+   packet_flush();
+}
+
 print $debug "START\n";
 $debug->flush();
 
-packet_compare_lists([0, "git-filter-client"], packet_txt_read()) ||
-   die "bad initialize";
-packet_compare_lists([0, "version=2"], packet_txt_read()) ||
-   die "bad version";
-packet_compare_lists([1, ""], packet_bin_read()) ||
-   die "bad version end";
-
-packet_txt_write("git-filter-server");
-packet_txt_write("version=2");
-packet_flush();
+packet_initialize("git-filter", 2);
 
 packet_compare_lists([0, "capability=clean"], packet_txt_read()) ||
die "bad capability";
-- 
2.15.0.7.ga9ff306ed9.dirty

[PATCH v2 6/8] t0021/rot13-filter: refactor checking final lf

[Christian Couder]

As checking for a lf character at the end of a buffer
will be useful in another function, let's refactor this
functionality into a small remove_final_lf_or_die()
helper function.

Signed-off-by: Christian Couder 
---
 t/t0021/rot13-filter.pl | 14 +++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/t/t0021/rot13-filter.pl b/t/t0021/rot13-filter.pl
index 2f74ab2e45..d47b7f5666 100644
--- a/t/t0021/rot13-filter.pl
+++ b/t/t0021/rot13-filter.pl
@@ -93,12 +93,20 @@ sub packet_bin_read {
}
 }
 
-sub packet_txt_read {
-   my ( $res, $buf ) = packet_bin_read();
-   unless ( $res == -1 or $buf eq '' or $buf =~ s/\n$// ) {
+sub remove_final_lf_or_die {
+   my $buf = shift;
+   unless ( $buf =~ s/\n$// ) {
die "A non-binary line MUST be terminated by an LF.\n"
. "Received: '$buf'";
}
+   return $buf;
+}
+
+sub packet_txt_read {
+   my ( $res, $buf ) = packet_bin_read();
+   unless ( $res == -1 or $buf eq '' ) {
+   $buf = remove_final_lf_or_die($buf);
+   }
return ( $res, $buf );
 }
 
-- 
2.15.0.7.ga9ff306ed9.dirty

[PATCH v2 3/8] t0021/rot13-filter: improve 'if .. elsif .. else' style

[Christian Couder]

Before further refactoring the "t0021/rot13-filter.pl" script,
let's modernize the style of its 'if .. elsif .. else' clauses
to improve its readability by making it more similar to our
other perl scripts.

Signed-off-by: Christian Couder 
---
 t/t0021/rot13-filter.pl | 39 +--
 1 file changed, 13 insertions(+), 26 deletions(-)

diff --git a/t/t0021/rot13-filter.pl b/t/t0021/rot13-filter.pl
index c025518c0a..37cecd8654 100644
--- a/t/t0021/rot13-filter.pl
+++ b/t/t0021/rot13-filter.pl
@@ -75,23 +75,20 @@ sub packet_bin_read {
if ( $bytes_read == 0 ) {
# EOF - Git stopped talking to us!
return ( -1, "" );
-   }
-   elsif ( $bytes_read != 4 ) {
+   } elsif ( $bytes_read != 4 ) {
die "invalid packet: '$buffer'";
}
my $pkt_size = hex($buffer);
if ( $pkt_size == 0 ) {
return ( 1, "" );
-   }
-   elsif ( $pkt_size > 4 ) {
+   } elsif ( $pkt_size > 4 ) {
my $content_size = $pkt_size - 4;
$bytes_read = read STDIN, $buffer, $content_size;
if ( $bytes_read != $content_size ) {
die "invalid packet ($content_size bytes expected; 
$bytes_read bytes read)";
}
return ( 0, $buffer );
-   }
-   else {
+   } else {
die "invalid packet size: $pkt_size";
}
 }
@@ -195,8 +192,7 @@ while (1) {
$debug->flush();
packet_txt_write("status=success");
packet_flush();
-   }
-   else {
+   } else {
my ( $res, $pathname ) = 
packet_required_key_val_read("pathname");
if ( $res == -1 ) {
die "unexpected EOF while expecting pathname";
@@ -240,17 +236,13 @@ while (1) {
my $output;
if ( exists $DELAY{$pathname} and exists 
$DELAY{$pathname}{"output"} ) {
$output = $DELAY{$pathname}{"output"}
-   }
-   elsif ( $pathname eq "error.r" or $pathname eq "abort.r" ) {
+   } elsif ( $pathname eq "error.r" or $pathname eq "abort.r" ) {
$output = "";
-   }
-   elsif ( $command eq "clean" and grep( /^clean$/, @capabilities 
) ) {
+   } elsif ( $command eq "clean" and grep( /^clean$/, 
@capabilities ) ) {
$output = rot13($input);
-   }
-   elsif ( $command eq "smudge" and grep( /^smudge$/, 
@capabilities ) ) {
+   } elsif ( $command eq "smudge" and grep( /^smudge$/, 
@capabilities ) ) {
$output = rot13($input);
-   }
-   else {
+   } else {
die "bad command '$command'";
}
 
@@ -259,25 +251,21 @@ while (1) {
$debug->flush();
packet_txt_write("status=error");
packet_flush();
-   }
-   elsif ( $pathname eq "abort.r" ) {
+   } elsif ( $pathname eq "abort.r" ) {
print $debug "[ABORT]\n";
$debug->flush();
packet_txt_write("status=abort");
packet_flush();
-   }
-   elsif ( $command eq "smudge" and
+   } elsif ( $command eq "smudge" and
exists $DELAY{$pathname} and
-   $DELAY{$pathname}{"requested"} == 1
-   ) {
+   $DELAY{$pathname}{"requested"} == 1 ) {
print $debug "[DELAYED]\n";
$debug->flush();
packet_txt_write("status=delayed");
packet_flush();
$DELAY{$pathname}{"requested"} = 2;
$DELAY{$pathname}{"output"} = $output;
-   }
-   else {
+   } else {
packet_txt_write("status=success");
packet_flush();
 
@@ -297,8 +285,7 @@ while (1) {
print $debug ".";
if ( length($output) > $MAX_PACKET_CONTENT_SIZE 
) {
$output = substr( $output, 
$MAX_PACKET_CONTENT_SIZE );
-   }
-   else {
+   } else {
$output = "";
}
}
-- 
2.15.0.7.ga9ff306ed9.dirty

[PATCH v2 7/8] t0021/rot13-filter: add capability functions

[Christian Couder]

These function help read and write capabilities.

To make them more generic and make it easy to reuse them,
the following changes are made:

- we don't require capabilities to come in a fixed order,
- we allow duplicates,
- we check that the remote supports the capabilities we
  advertise,
- we don't check if the remote declares any capability we
  don't know about.

The reason behind the last change is that the protocol
should work using only the capabilities that both ends
support, and it should not stop working if one end starts
to advertise a new capability.

Despite those changes, we can still require a set of
capabilities, and die if one of them is not supported.

Signed-off-by: Christian Couder 
---
 t/t0021/rot13-filter.pl | 58 ++---
 1 file changed, 45 insertions(+), 13 deletions(-)

diff --git a/t/t0021/rot13-filter.pl b/t/t0021/rot13-filter.pl
index d47b7f5666..f919d798a6 100644
--- a/t/t0021/rot13-filter.pl
+++ b/t/t0021/rot13-filter.pl
@@ -150,24 +150,56 @@ sub packet_initialize {
packet_flush();
 }
 
+sub packet_read_capabilities {
+   my @cap;
+   while (1) {
+   my ( $res, $buf ) = packet_bin_read();
+   if ( $res == -1 ) {
+   die "unexpected EOF when reading capabilities";
+   }
+   return ( $res, @cap ) if ( $res != 0 );
+   $buf = remove_final_lf_or_die($buf);
+   unless ( $buf =~ s/capability=// ) {
+   die "bad capability buf: '$buf'";
+   }
+   push @cap, $buf;
+   }
+}
+
+# Read remote capabilities and check them against capabilities we require
+sub packet_read_and_check_capabilities {
+   my @required_caps = @_;
+   my ($res, @remote_caps) = packet_read_capabilities();
+   my %remote_caps = map { $_ => 1 } @remote_caps;
+   foreach (@required_caps) {
+   unless (exists($remote_caps{$_})) {
+   die "required '$_' capability not available from 
remote" ;
+   }
+   }
+   return %remote_caps;
+}
+
+# Check our capabilities we want to advertise against the remote ones
+# and then advertise our capabilities
+sub packet_check_and_write_capabilities {
+   my ($remote_caps, @our_caps) = @_;
+   foreach (@our_caps) {
+   unless (exists($remote_caps->{$_})) {
+   die "our capability '$_' is not available from remote"
+   }
+   packet_txt_write( "capability=" . $_ );
+   }
+   packet_flush();
+}
+
 print $debug "START\n";
 $debug->flush();
 
 packet_initialize("git-filter", 2);
 
-packet_compare_lists([0, "capability=clean"], packet_txt_read()) ||
-   die "bad capability";
-packet_compare_lists([0, "capability=smudge"], packet_txt_read()) ||
-   die "bad capability";
-packet_compare_lists([0, "capability=delay"], packet_txt_read()) ||
-   die "bad capability";
-packet_compare_lists([1, ""], packet_bin_read()) ||
-   die "bad capability end";
-
-foreach (@capabilities) {
-   packet_txt_write( "capability=" . $_ );
-}
-packet_flush();
+my %remote_caps = packet_read_and_check_capabilities("clean", "smudge", 
"delay");
+packet_check_and_write_capabilities(\%remote_caps, @capabilities);
+
 print $debug "init handshake complete\n";
 $debug->flush();
 
-- 
2.15.0.7.ga9ff306ed9.dirty

[PATCH v2 4/8] t0021/rot13-filter: improve error message

[Christian Couder]

If there is no new line at the end of something it receives,
the packet_txt_read() function die()s, but it's difficult to
debug without much context.

Let's give a bit more information when that happens.

Signed-off-by: Christian Couder 
---
 t/t0021/rot13-filter.pl | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/t/t0021/rot13-filter.pl b/t/t0021/rot13-filter.pl
index 37cecd8654..f31ff595fe 100644
--- a/t/t0021/rot13-filter.pl
+++ b/t/t0021/rot13-filter.pl
@@ -96,7 +96,8 @@ sub packet_bin_read {
 sub packet_txt_read {
my ( $res, $buf ) = packet_bin_read();
unless ( $res == -1 or $buf eq '' or $buf =~ s/\n$// ) {
-   die "A non-binary line MUST be terminated by an LF.";
+   die "A non-binary line MUST be terminated by an LF.\n"
+   . "Received: '$buf'";
}
return ( $res, $buf );
 }
-- 
2.15.0.7.ga9ff306ed9.dirty

[PATCH v2 1/8] t0021/rot13-filter: fix list comparison

[Christian Couder]

Since edcc8581 ("convert: add filter..process
option", 2016-10-16) when t0021/rot13-filter.pl was created, list
comparison in this perl script have been quite broken.

packet_txt_read() returns a 2-element list, and the right hand
side of "eq" also has a list with (two, elements), but "eq" takes
the last element of the list on each side, and compares them. The
first elements (0 or 1) on the right hand side lists do not matter,
which means we do not require to see a flush at the end of the
version -- a simple empty string or an EOF would do, which is
definitely not what we want.

Signed-off-by: Christian Couder 
---
 t/t0021/rot13-filter.pl | 35 ---
 1 file changed, 28 insertions(+), 7 deletions(-)

diff --git a/t/t0021/rot13-filter.pl b/t/t0021/rot13-filter.pl
index ad685d92f8..4b2d9087d4 100644
--- a/t/t0021/rot13-filter.pl
+++ b/t/t0021/rot13-filter.pl
@@ -55,6 +55,20 @@ sub rot13 {
return $str;
 }
 
+sub packet_compare_lists {
+   my ($expect, @result) = @_;
+   my $ix;
+   if (scalar @$expect != scalar @result) {
+   return undef;
+   }
+   for ($ix = 0; $ix < $#result; $ix++) {
+   if ($expect->[$ix] ne $result[$ix]) {
+   return undef;
+   }
+   }
+   return 1;
+}
+
 sub packet_bin_read {
my $buffer;
my $bytes_read = read STDIN, $buffer, 4;
@@ -110,18 +124,25 @@ sub packet_flush {
 print $debug "START\n";
 $debug->flush();
 
-( packet_txt_read() eq ( 0, "git-filter-client" ) ) || die "bad initialize";
-( packet_txt_read() eq ( 0, "version=2" ) ) || die "bad version";
-( packet_bin_read() eq ( 1, "" ) )  || die "bad version end";
+packet_compare_lists([0, "git-filter-client"], packet_txt_read()) ||
+   die "bad initialize";
+packet_compare_lists([0, "version=2"], packet_txt_read()) ||
+   die "bad version";
+packet_compare_lists([1, ""], packet_bin_read()) ||
+   die "bad version end";
 
 packet_txt_write("git-filter-server");
 packet_txt_write("version=2");
 packet_flush();
 
-( packet_txt_read() eq ( 0, "capability=clean" ) )  || die "bad capability";
-( packet_txt_read() eq ( 0, "capability=smudge" ) ) || die "bad capability";
-( packet_txt_read() eq ( 0, "capability=delay" ) )  || die "bad capability";
-( packet_bin_read() eq ( 1, "" ) )  || die "bad capability 
end";
+packet_compare_lists([0, "capability=clean"], packet_txt_read()) ||
+   die "bad capability";
+packet_compare_lists([0, "capability=smudge"], packet_txt_read()) ||
+   die "bad capability";
+packet_compare_lists([0, "capability=delay"], packet_txt_read()) ||
+   die "bad capability";
+packet_compare_lists([1, ""], packet_bin_read()) ||
+   die "bad capability end";
 
 foreach (@capabilities) {
packet_txt_write( "capability=" . $_ );
-- 
2.15.0.7.ga9ff306ed9.dirty

[PATCH v2 2/8] t0021/rot13-filter: refactor packet reading functions

[Christian Couder]

To make it possible in a following commit to move packet
reading and writing functions into a Packet.pm module,
let's refactor these functions, so they don't handle
printing debug output and exiting.

While at it let's create packet_required_key_val_read()
to still handle erroring out in a common case.

Signed-off-by: Christian Couder 
---
 t/t0021/rot13-filter.pl | 38 --
 1 file changed, 28 insertions(+), 10 deletions(-)

diff --git a/t/t0021/rot13-filter.pl b/t/t0021/rot13-filter.pl
index 4b2d9087d4..c025518c0a 100644
--- a/t/t0021/rot13-filter.pl
+++ b/t/t0021/rot13-filter.pl
@@ -74,8 +74,7 @@ sub packet_bin_read {
my $bytes_read = read STDIN, $buffer, 4;
if ( $bytes_read == 0 ) {
# EOF - Git stopped talking to us!
-   print $debug "STOP\n";
-   exit();
+   return ( -1, "" );
}
elsif ( $bytes_read != 4 ) {
die "invalid packet: '$buffer'";
@@ -99,12 +98,21 @@ sub packet_bin_read {
 
 sub packet_txt_read {
my ( $res, $buf ) = packet_bin_read();
-   unless ( $buf eq '' or $buf =~ s/\n$// ) {
+   unless ( $res == -1 or $buf eq '' or $buf =~ s/\n$// ) {
die "A non-binary line MUST be terminated by an LF.";
}
return ( $res, $buf );
 }
 
+sub packet_required_key_val_read {
+   my ( $key ) = @_;
+   my ( $res, $buf ) = packet_txt_read();
+   unless ( $res == -1 or ( $buf =~ s/^$key=// and $buf ne '' ) ) {
+   die "bad $key: '$buf'";
+   }
+   return ( $res, $buf );
+}
+
 sub packet_bin_write {
my $buf = shift;
print STDOUT sprintf( "%04x", length($buf) + 4 );
@@ -152,13 +160,18 @@ print $debug "init handshake complete\n";
 $debug->flush();
 
 while (1) {
-   my ( $command ) = packet_txt_read() =~ /^command=(.+)$/;
+   my ( $res, $command ) = packet_required_key_val_read("command");
+   if ( $res == -1 ) {
+   print $debug "STOP\n";
+   exit();
+   }
print $debug "IN: $command";
$debug->flush();
 
if ( $command eq "list_available_blobs" ) {
# Flush
-   packet_bin_read();
+   packet_compare_lists([1, ""], packet_bin_read()) ||
+   die "bad list_available_blobs end";
 
foreach my $pathname ( sort keys %DELAY ) {
if ( $DELAY{$pathname}{"requested"} >= 1 ) {
@@ -184,14 +197,13 @@ while (1) {
packet_flush();
}
else {
-   my ( $pathname ) = packet_txt_read() =~ /^pathname=(.+)$/;
+   my ( $res, $pathname ) = 
packet_required_key_val_read("pathname");
+   if ( $res == -1 ) {
+   die "unexpected EOF while expecting pathname";
+   }
print $debug " $pathname";
$debug->flush();
 
-   if ( $pathname eq "" ) {
-   die "bad pathname '$pathname'";
-   }
-
# Read until flush
my ( $done, $buffer ) = packet_txt_read();
while ( $buffer ne '' ) {
@@ -205,6 +217,9 @@ while (1) {
 
( $done, $buffer ) = packet_txt_read();
}
+   if ( $done == -1 ) {
+   die "unexpected EOF after pathname '$pathname'";
+   }
 
my $input = "";
{
@@ -215,6 +230,9 @@ while (1) {
( $done, $buffer ) = packet_bin_read();
$input .= $buffer;
}
+   if ( $done == -1 ) {
+   die "unexpected EOF while reading input for 
'$pathname'";
+   }   
print $debug " " . length($input) . " [OK] -- ";
$debug->flush();
}
-- 
2.15.0.7.ga9ff306ed9.dirty

[PATCH v2 8/8] Add Git/Packet.pm from parts of t0021/rot13-filter.pl

[Christian Couder]

And while at it let's simplify t0021/rot13-filter.pl by
using Git/Packet.pm.

This will make it possible to reuse packet related
functions in other test scripts.

Signed-off-by: Christian Couder 
---
 perl/Git/Packet.pm  | 168 
 perl/Makefile   |   1 +
 t/t0021/rot13-filter.pl | 140 +---
 3 files changed, 172 insertions(+), 137 deletions(-)
 create mode 100644 perl/Git/Packet.pm

diff --git a/perl/Git/Packet.pm b/perl/Git/Packet.pm
new file mode 100644
index 00..255b28c098
--- /dev/null
+++ b/perl/Git/Packet.pm
@@ -0,0 +1,168 @@
+package Git::Packet;
+use 5.008;
+use strict;
+use warnings;
+BEGIN {
+   require Exporter;
+   if ($] < 5.008003) {
+   *import = \::import;
+   } else {
+   # Exporter 5.57 which supports this invocation was
+   # released with perl 5.8.3
+   Exporter->import('import');
+   }
+}
+
+our @EXPORT = qw(
+   packet_compare_lists
+   packet_bin_read
+   packet_txt_read
+   packet_required_key_val_read
+   packet_bin_write
+   packet_txt_write
+   packet_flush
+   packet_initialize
+   packet_read_capabilities
+   packet_read_and_check_capabilities
+   packet_check_and_write_capabilities
+   );
+our @EXPORT_OK = @EXPORT;
+
+sub packet_compare_lists {
+   my ($expect, @result) = @_;
+   my $ix;
+   if (scalar @$expect != scalar @result) {
+   return undef;
+   }
+   for ($ix = 0; $ix < $#result; $ix++) {
+   if ($expect->[$ix] ne $result[$ix]) {
+   return undef;
+   }
+   }
+   return 1;
+}
+
+sub packet_bin_read {
+   my $buffer;
+   my $bytes_read = read STDIN, $buffer, 4;
+   if ( $bytes_read == 0 ) {
+   # EOF - Git stopped talking to us!
+   return ( -1, "" );
+   } elsif ( $bytes_read != 4 ) {
+   die "invalid packet: '$buffer'";
+   }
+   my $pkt_size = hex($buffer);
+   if ( $pkt_size == 0 ) {
+   return ( 1, "" );
+   } elsif ( $pkt_size > 4 ) {
+   my $content_size = $pkt_size - 4;
+   $bytes_read = read STDIN, $buffer, $content_size;
+   if ( $bytes_read != $content_size ) {
+   die "invalid packet ($content_size bytes expected; 
$bytes_read bytes read)";
+   }
+   return ( 0, $buffer );
+   } else {
+   die "invalid packet size: $pkt_size";
+   }
+}
+
+sub remove_final_lf_or_die {
+   my $buf = shift;
+   unless ( $buf =~ s/\n$// ) {
+   die "A non-binary line MUST be terminated by an LF.\n"
+   . "Received: '$buf'";
+   }
+   return $buf;
+}
+
+sub packet_txt_read {
+   my ( $res, $buf ) = packet_bin_read();
+   unless ( $res == -1 or $buf eq '' ) {
+   $buf = remove_final_lf_or_die($buf);
+   }
+   return ( $res, $buf );
+}
+
+sub packet_required_key_val_read {
+   my ( $key ) = @_;
+   my ( $res, $buf ) = packet_txt_read();
+   unless ( $res == -1 or ( $buf =~ s/^$key=// and $buf ne '' ) ) {
+   die "bad $key: '$buf'";
+   }
+   return ( $res, $buf );
+}
+
+sub packet_bin_write {
+   my $buf = shift;
+   print STDOUT sprintf( "%04x", length($buf) + 4 );
+   print STDOUT $buf;
+   STDOUT->flush();
+}
+
+sub packet_txt_write {
+   packet_bin_write( $_[0] . "\n" );
+}
+
+sub packet_flush {
+   print STDOUT sprintf( "%04x", 0 );
+   STDOUT->flush();
+}
+
+sub packet_initialize {
+   my ($name, $version) = @_;
+
+   packet_compare_lists([0, $name . "-client"], packet_txt_read()) ||
+   die "bad initialize";
+   packet_compare_lists([0, "version=" . $version], packet_txt_read()) ||
+   die "bad version";
+   packet_compare_lists([1, ""], packet_bin_read()) ||
+   die "bad version end";
+
+   packet_txt_write( $name . "-server" );
+   packet_txt_write( "version=" . $version );
+   packet_flush();
+}
+
+sub packet_read_capabilities {
+   my @cap;
+   while (1) {
+   my ( $res, $buf ) = packet_bin_read();
+   if ( $res == -1 ) {
+   die "unexpected EOF when reading capabilities";
+   }
+   return ( $res, @cap ) if ( $res != 0 );
+   $buf = remove_final_lf_or_die($buf);
+   unless ( $buf =~ s/capability=// ) {
+   die "bad capability buf: '$buf'";
+   }
+   push @cap, $buf;
+   }
+}
+
+# Read remote capabilities and check them against capabilities we require
+sub 

[PATCH v2 0/8] Create Git/Packet.pm

[Christian Couder]

Goal


Packet related functions in Perl can be useful to write new filters or
to debug or test existing filters. They might also in the future be
used by other software using the same packet line protocol. So instead
of having them in t0021/rot13-filter.pl, let's extract them into a new
Git/Packet.pm module.

Changes since the previous version
~~

  - Patch 1/8 is new. It fixes a list comparison bug that existed
since the beginning of t0021/rot13-filter.pl.

  - Patch 2/8 is much improved. It now checks for unexpected EOF in
all the code paths and introduces a new
packet_required_key_val_read() function.

  - Patchs 3/8, 4/8 and 5/8 have not changed since v1.

  - Patch 6/8 is new. It adds a small helper function.

  - Patch 7/8 is much improved. It now describe better all the changes
and better check that the capabilities we advertise are supported
by the remote.

  - Patch 8/8 has been improved. It contains the Makefile change
suggested by Dscho.

Links
~

This patch series is on the following branch:

https://github.com/chriscool/git/commits/gl-prep-external-odb

Version 1 of this patch series is on the mailing list here:

https://public-inbox.org/git/20171019123030.17338-1-chrisc...@tuxfamily.org/

It is also available in the following branch:

https://github.com/chriscool/git/commits/gl-prep-external-odb1

This patch series was extracted from previous "Add initial
experimental external ODB support" patch series.

Version 1, 2, 3, 4, 5 and 6 of this previous series are on the mailing
list here:

https://public-inbox.org/git/20160613085546.11784-1-chrisc...@tuxfamily.org/
https://public-inbox.org/git/20160628181933.24620-1-chrisc...@tuxfamily.org/
https://public-inbox.org/git/20161130210420.15982-1-chrisc...@tuxfamily.org/
https://public-inbox.org/git/20170620075523.26961-1-chrisc...@tuxfamily.org/
https://public-inbox.org/git/20170803091926.1755-1-chrisc...@tuxfamily.org/
https://public-inbox.org/git/20170916080731.13925-1-chrisc...@tuxfamily.org/

They are also available in the following branches:

https://github.com/chriscool/git/commits/gl-external-odb12
https://github.com/chriscool/git/commits/gl-external-odb22
https://github.com/chriscool/git/commits/gl-external-odb61
https://github.com/chriscool/git/commits/gl-external-odb239
https://github.com/chriscool/git/commits/gl-external-odb373
https://github.com/chriscool/git/commits/gl-external-odb411


Christian Couder (8):
  t0021/rot13-filter: fix list comparison
  t0021/rot13-filter: refactor packet reading functions
  t0021/rot13-filter: improve 'if .. elsif .. else' style
  t0021/rot13-filter: improve error message
  t0021/rot13-filter: add packet_initialize()
  t0021/rot13-filter: refactor checking final lf
  t0021/rot13-filter: add capability functions
  Add Git/Packet.pm from parts of t0021/rot13-filter.pl

 perl/Git/Packet.pm  | 168 
 perl/Makefile   |   1 +
 t/t0021/rot13-filter.pl | 127 ++--
 3 files changed, 202 insertions(+), 94 deletions(-)
 create mode 100644 perl/Git/Packet.pm

-- 
2.15.0.7.ga9ff306ed9.dirty

[PATCH v2 2/2] reduce_heads: fix memory leaks

[Martin Ågren]

We currently have seven callers of `reduce_heads(foo)`. Six of them do
not use the original list `foo` again, and actually, all six of those
end up leaking it.

Introduce and use `reduce_heads_replace()` as a leak-free version of
`foo = reduce_heads(foo)` to fix several of these. Fix the remaining
leaks using `free_commit_list()`.

While we're here, document `reduce_heads()` and mark it as `extern`.

Signed-off-by: Martin Ågren 
---
 commit.h| 18 +-
 builtin/commit.c|  2 +-
 builtin/fmt-merge-msg.c |  2 +-
 builtin/merge-base.c|  6 --
 builtin/merge.c |  1 +
 builtin/pull.c  |  5 -
 commit.c|  7 +++
 7 files changed, 35 insertions(+), 6 deletions(-)

diff --git a/commit.h b/commit.h
index 6d769590f..99a3fea68 100644
--- a/commit.h
+++ b/commit.h
@@ -313,7 +313,23 @@ extern int interactive_add(int argc, const char **argv, 
const char *prefix, int
 extern int run_add_interactive(const char *revision, const char *patch_mode,
   const struct pathspec *pathspec);
 
-struct commit_list *reduce_heads(struct commit_list *heads);
+/*
+ * Takes a list of commits and returns a new list where those
+ * have been removed that can be reached from other commits in
+ * the list. It is useful for, e.g., reducing the commits
+ * randomly thrown at the git-merge command and removing
+ * redundant commits that the user shouldn't have given to it.
+ *
+ * This function destroys the STALE bit of the commit objects'
+ * flags.
+ */
+extern struct commit_list *reduce_heads(struct commit_list *heads);
+
+/*
+ * Like `reduce_heads()`, except it replaces the list. Use this
+ * instead of `foo = reduce_heads(foo);` to avoid memory leaks.
+ */
+extern void reduce_heads_replace(struct commit_list **heads);
 
 struct commit_extra_header {
struct commit_extra_header *next;
diff --git a/builtin/commit.c b/builtin/commit.c
index d75b3805e..11c474018 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1728,7 +1728,7 @@ int cmd_commit(int argc, const char **argv, const char 
*prefix)
allow_fast_forward = 0;
}
if (allow_fast_forward)
-   parents = reduce_heads(parents);
+   reduce_heads_replace();
} else {
if (!reflog_msg)
reflog_msg = (whence == FROM_CHERRY_PICK)
diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c
index e99b5ddbf..27a2361e9 100644
--- a/builtin/fmt-merge-msg.c
+++ b/builtin/fmt-merge-msg.c
@@ -571,7 +571,7 @@ static void find_merge_parents(struct merge_parents *result,
head_commit = lookup_commit(head);
if (head_commit)
commit_list_insert(head_commit, );
-   parents = reduce_heads(parents);
+   reduce_heads_replace();
 
while (parents) {
struct commit *cmit = pop_commit();
diff --git a/builtin/merge-base.c b/builtin/merge-base.c
index fd0eba14b..0178ca772 100644
--- a/builtin/merge-base.c
+++ b/builtin/merge-base.c
@@ -57,7 +57,7 @@ static int handle_independent(int count, const char **args)
for (i = count - 1; i >= 0; i--)
commit_list_insert(get_commit_reference(args[i]), );
 
-   revs = reduce_heads(revs);
+   reduce_heads_replace();
 
if (!revs)
return 1;
@@ -78,7 +78,9 @@ static int handle_octopus(int count, const char **args, int 
show_all)
for (i = count - 1; i >= 0; i--)
commit_list_insert(get_commit_reference(args[i]), );
 
-   result = reduce_heads(get_octopus_merge_bases(revs));
+   result = get_octopus_merge_bases(revs);
+   free_commit_list(revs);
+   reduce_heads_replace();
 
if (!result)
return 1;
diff --git a/builtin/merge.c b/builtin/merge.c
index ab5ffe85e..fbbf2a9e5 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -999,6 +999,7 @@ static struct commit_list *reduce_parents(struct commit 
*head_commit,
 
/* Find what parents to record by checking independent ones. */
parents = reduce_heads(remoteheads);
+   free_commit_list(remoteheads);
 
remoteheads = NULL;
remotes = 
diff --git a/builtin/pull.c b/builtin/pull.c
index 6f772e8a2..4edab228e 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -745,12 +745,15 @@ static int get_octopus_merge_base(struct object_id 
*merge_base,
if (!is_null_oid(fork_point))
commit_list_insert(lookup_commit_reference(fork_point), );
 
-   result = reduce_heads(get_octopus_merge_bases(revs));
+   result = get_octopus_merge_bases(revs);
free_commit_list(revs);
+   reduce_heads_replace();
+
if (!result)
return 1;
 
oidcpy(merge_base, >item->object.oid);
+   free_commit_list(result);
return 0;
 }
 
diff --git a/commit.c b/commit.c
index 1e0e63379..cab8d4455 100644

[PATCH v2 1/2] builtin/merge-base: UNLEAK commit lists

[Martin Ågren]

In several functions, we iterate through a commit list by assigning
`result = result->next`. As a consequence, we lose the original pointer
and eventually leak the list.

These are immediate helpers to `cmd_merge_base()` which is just about to
return, so we can use UNLEAK. For example, we could `UNLEAK(result)`
before we start iterating. That would be a one-liner change per
function. Instead, leave the lists alone and iterate using a dedicated
pointer. Then UNLEAK immediately before returning.

After this change, it is clearer that the leaks happen as we return, and
not as we process the list. That is, we could just as well have used
`free_commit_list()`. Also, leaving a "result" unchanged as we display
it feels (marginally) better.

In `handle_independent()` we can drop `result` while we're here and
reuse the `revs`-variable instead. That matches several other users of
`reduce_heads()`. The memory-leak that this hides will be addressed in
the next commit.

Signed-off-by: Martin Ågren 
---
 builtin/merge-base.c | 36 ++--
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/builtin/merge-base.c b/builtin/merge-base.c
index 6dbd167d3..fd0eba14b 100644
--- a/builtin/merge-base.c
+++ b/builtin/merge-base.c
@@ -9,20 +9,20 @@
 
 static int show_merge_base(struct commit **rev, int rev_nr, int show_all)
 {
-   struct commit_list *result;
+   struct commit_list *result, *r;
 
result = get_merge_bases_many_dirty(rev[0], rev_nr - 1, rev + 1);
 
if (!result)
return 1;
 
-   while (result) {
-   printf("%s\n", oid_to_hex(>item->object.oid));
+   for (r = result; r; r = r->next) {
+   printf("%s\n", oid_to_hex(>item->object.oid));
if (!show_all)
-   return 0;
-   result = result->next;
+   break;
}
 
+   UNLEAK(result);
return 0;
 }
 
@@ -51,28 +51,28 @@ static struct commit *get_commit_reference(const char *arg)
 
 static int handle_independent(int count, const char **args)
 {
-   struct commit_list *revs = NULL;
-   struct commit_list *result;
+   struct commit_list *revs = NULL, *rev;
int i;
 
for (i = count - 1; i >= 0; i--)
commit_list_insert(get_commit_reference(args[i]), );
 
-   result = reduce_heads(revs);
-   if (!result)
+   revs = reduce_heads(revs);
+
+   if (!revs)
return 1;
 
-   while (result) {
-   printf("%s\n", oid_to_hex(>item->object.oid));
-   result = result->next;
-   }
+   for (rev = revs; rev; rev = rev->next)
+   printf("%s\n", oid_to_hex(>item->object.oid));
+
+   UNLEAK(revs);
return 0;
 }
 
 static int handle_octopus(int count, const char **args, int show_all)
 {
struct commit_list *revs = NULL;
-   struct commit_list *result;
+   struct commit_list *result, *rev;
int i;
 
for (i = count - 1; i >= 0; i--)
@@ -83,13 +83,13 @@ static int handle_octopus(int count, const char **args, int 
show_all)
if (!result)
return 1;
 
-   while (result) {
-   printf("%s\n", oid_to_hex(>item->object.oid));
+   for (rev = result; rev; rev = rev->next) {
+   printf("%s\n", oid_to_hex(>item->object.oid));
if (!show_all)
-   return 0;
-   result = result->next;
+   break;
}
 
+   UNLEAK(result);
return 0;
 }
 
-- 
2.15.0.415.gac1375d7e

[PATCH v2 0/2] Re: reduce_heads: fix memory leaks

[Martin Ågren]

Since v1 [1], I've added a preparatory patch to UNLEAK some variables.
That sets the stage slightly better for patch 2.

Junio, you placed v1 on maint. Because UNLEAK is not in maint, this is
based on master and maint misses out on this v2. If you have any advice
for how I should (not) do series with UNLEAK in them, I'm all ears.

Martin

[1] https://public-inbox.org/git/20171101090326.8091-1-martin.ag...@gmail.com/

Martin Ågren (2):
  builtin/merge-base: UNLEAK commit lists
  reduce_heads: fix memory leaks

 commit.h| 18 +-
 builtin/commit.c|  2 +-
 builtin/fmt-merge-msg.c |  2 +-
 builtin/merge-base.c| 40 +---
 builtin/merge.c |  1 +
 builtin/pull.c  |  5 -
 commit.c|  7 +++
 7 files changed, 52 insertions(+), 23 deletions(-)

-- 
2.15.0.415.gac1375d7e

[PATCH v2 4/4] bisect: fix memory leak when returning best element

[Martin Ågren]

When `find_bisection()` returns a single list entry, it leaks the other
entries. Move the to-be-returned item to the front and free the
remainder.

Signed-off-by: Martin Ågren 
Signed-off-by: Junio C Hamano 
---
 bisect.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/bisect.c b/bisect.c
index b1941505b..3756f127b 100644
--- a/bisect.c
+++ b/bisect.c
@@ -399,8 +399,12 @@ void find_bisection(struct commit_list **commit_list, int 
*reaches,
/* Do the real work of finding bisection commit. */
best = do_find_bisection(list, nr, weights, find_all);
if (best) {
-   if (!find_all)
+   if (!find_all) {
+   list->item = best->item;
+   free_commit_list(list->next);
+   best = list;
best->next = NULL;
+   }
*reaches = weight(best);
}
free(weights);
-- 
2.15.0.415.gac1375d7e

[PATCH v2 3/4] bisect: fix off-by-one error in `best_bisection_sorted()`

[Martin Ågren]

After we have sorted the `cnt`-many commits that we have selected, we
place them into the commit list. We then set `p->next` to NULL, but as
we do so, `p` is already pointing one beyond item number `cnt`. Indeed,
we check whether `p` is NULL before dereferencing it.

This only matters if there are TREESAME-commits. Since they should be
skipped, they are not included in `cnt` and we will hit the situation
where we set `p->next` to NULL. As a result, the list will be one longer
than it should be. The last commit in the list will be one which occurs
earlier, or which shouldn't be included.

Do not update `p` the very last round in the loop. This ensures that
after the loop, `p->next` points to the remainder of the list, and we
can set it to NULL. While we're here, free that remainder to fix a
memory leak.

Signed-off-by: Martin Ågren 
Signed-off-by: Junio C Hamano 
---
 bisect.c | 7 ---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/bisect.c b/bisect.c
index 2f4321767..b1941505b 100644
--- a/bisect.c
+++ b/bisect.c
@@ -226,10 +226,11 @@ static struct commit_list *best_bisection_sorted(struct 
commit_list *list, int n
add_name_decoration(DECORATION_NONE, buf.buf, obj);
 
p->item = array[i].commit;
-   p = p->next;
+   if (i < cnt - 1)
+   p = p->next;
}
-   if (p)
-   p->next = NULL;
+   free_commit_list(p->next);
+   p->next = NULL;
strbuf_release();
free(array);
return list;
-- 
2.15.0.415.gac1375d7e

[PATCH v2 2/4] bisect: fix memory leak in `find_bisection()`

[Martin Ågren]

`find_bisection()` rebuilds the commit list it is given by reversing it
and skipping uninteresting commits. The uninteresting list entries are
leaked. Free them to fix the leak.

Signed-off-by: Martin Ågren 
---
 bisect.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/bisect.c b/bisect.c
index 5a3ae4971..2f4321767 100644
--- a/bisect.c
+++ b/bisect.c
@@ -379,8 +379,10 @@ void find_bisection(struct commit_list **commit_list, int 
*reaches,
unsigned flags = p->item->object.flags;
 
next = p->next;
-   if (flags & UNINTERESTING)
+   if (flags & UNINTERESTING) {
+   free(p);
continue;
+   }
p->next = last;
last = p;
if (!(flags & TREESAME))
-- 
2.15.0.415.gac1375d7e

[PATCH v2 1/4] bisect: change calling-convention of `find_bisection()`

[Martin Ågren]

This function takes a commit list and returns a commit list. The
returned list is built by modifying the original list. Thus the caller
should not use the original list again (and after the next commit fixes
a memory leak, it must not).

Change the function signature so that it takes a **list and has void
return type. That should make it harder to misuse this function.

While we're here, document this function.

Signed-off-by: Martin Ågren 
---
 bisect.h   | 12 +---
 bisect.c   | 16 +++-
 builtin/rev-list.c |  3 +--
 3 files changed, 17 insertions(+), 14 deletions(-)

diff --git a/bisect.h b/bisect.h
index acd12ef80..c535e6d12 100644
--- a/bisect.h
+++ b/bisect.h
@@ -1,9 +1,15 @@
 #ifndef BISECT_H
 #define BISECT_H
 
-extern struct commit_list *find_bisection(struct commit_list *list,
- int *reaches, int *all,
- int find_all);
+/*
+ * Find bisection. If something is found, `reaches` will be the number of
+ * commits that the best commit reaches. `all` will be the count of
+ * non-SAMETREE commits. If nothing is found, `list` will be NULL.
+ * Otherwise, it will be either all non-SAMETREE commits or the single
+ * best commit, as chosen by `find_all`.
+ */
+extern void find_bisection(struct commit_list **list, int *reaches, int *all,
+  int find_all);
 
 extern struct commit_list *filter_skipped(struct commit_list *list,
  struct commit_list **tried,
diff --git a/bisect.c b/bisect.c
index 96beeb5d1..5a3ae4971 100644
--- a/bisect.c
+++ b/bisect.c
@@ -360,21 +360,20 @@ static struct commit_list *do_find_bisection(struct 
commit_list *list,
return best_bisection_sorted(list, nr);
 }
 
-struct commit_list *find_bisection(struct commit_list *list,
- int *reaches, int *all,
- int find_all)
+void find_bisection(struct commit_list **commit_list, int *reaches,
+   int *all, int find_all)
 {
int nr, on_list;
-   struct commit_list *p, *best, *next, *last;
+   struct commit_list *list, *p, *best, *next, *last;
int *weights;
 
-   show_list("bisection 2 entry", 0, 0, list);
+   show_list("bisection 2 entry", 0, 0, *commit_list);
 
/*
 * Count the number of total and tree-changing items on the
 * list, while reversing the list.
 */
-   for (nr = on_list = 0, last = NULL, p = list;
+   for (nr = on_list = 0, last = NULL, p = *commit_list;
 p;
 p = next) {
unsigned flags = p->item->object.flags;
@@ -402,7 +401,7 @@ struct commit_list *find_bisection(struct commit_list *list,
*reaches = weight(best);
}
free(weights);
-   return best;
+   *commit_list = best;
 }
 
 static int register_ref(const char *refname, const struct object_id *oid,
@@ -954,8 +953,7 @@ int bisect_next_all(const char *prefix, int no_checkout)
 
bisect_common();
 
-   revs.commits = find_bisection(revs.commits, , ,
-  !!skipped_revs.nr);
+   find_bisection(, , , !!skipped_revs.nr);
revs.commits = managed_skipped(revs.commits, );
 
if (!revs.commits) {
diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index c1c74d4a7..fb1c36af6 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -397,8 +397,7 @@ int cmd_rev_list(int argc, const char **argv, const char 
*prefix)
if (bisect_list) {
int reaches = reaches, all = all;
 
-   revs.commits = find_bisection(revs.commits, , ,
- bisect_find_all);
+   find_bisection(, , , bisect_find_all);
 
if (bisect_show_vars)
return show_bisect_vars(, reaches, all);
-- 
2.15.0.415.gac1375d7e

[PATCH v2 0/4] bisect: assorted leak-fixes

[Martin Ågren]

This fixes a couple of leaks around `find_bisection(). The first patch
is new since v1 [1] to make the calling-convention of `find_bisection()`
less prone to misuse. Patch 2 is similar to v1, the only difference is
that the "while at it" has moved into patch 1. Patches 3 and 4 are
identical to v1.

Martin

[1] 
https://public-inbox.org/git/4795556016c25e4a78241362547c5468877f808d.1509557518.git.martin.ag...@gmail.com/

Martin Ågren (4):
  bisect: change calling-convention of `find_bisection()`
  bisect: fix memory leak in `find_bisection()`
  bisect: fix off-by-one error in `best_bisection_sorted()`
  bisect: fix memory leak when returning best element

 bisect.h   | 12 +---
 bisect.c   | 33 +++--
 builtin/rev-list.c |  3 +--
 3 files changed, 29 insertions(+), 19 deletions(-)

-- 
2.15.0.415.gac1375d7e

Re: [PATCH 1/3] notes: move hex_to_bytes() to hex.c and export it

[Kevin Daudt]

On Sun, Nov 05, 2017 at 05:47:47PM +0100, René Scharfe wrote:
> Am 05.11.2017 um 03:56 schrieb Kevin Daudt:
> > On Tue, Oct 31, 2017 at 02:46:49PM +0100, René Scharfe wrote:
> >> Make the function for converting pairs of hexadecimal digits to binary
> >> available to other call sites.
> >>
> >> Signed-off-by: Rene Scharfe 
> >> ---
> >>   cache.h |  7 +++
> >>   hex.c   | 12 
> >>   notes.c | 17 -
> >>   3 files changed, 19 insertions(+), 17 deletions(-)
> >>
> >> diff --git a/cache.h b/cache.h
> >> index 6440e2bf21..f06bfbaf32 100644
> >> --- a/cache.h
> >> +++ b/cache.h
> >> @@ -1317,6 +1317,13 @@ extern int set_disambiguate_hint_config(const char 
> >> *var, const char *value);
> >>   extern int get_sha1_hex(const char *hex, unsigned char *sha1);
> >>   extern int get_oid_hex(const char *hex, struct object_id *sha1);
> >>   
> >> +/*
> >> + * Read `len` pairs of hexadecimal digits from `hex` and write the
> >> + * values to `binary` as `len` bytes. Return 0 on success, or -1 if
> > 
> > Is it correct to call the result binary? I would say that it's the value
> > that gets stored. To me, this value does not really have a base.
> 
> Here's the full context:
> 
>   /*
>* Read `len` pairs of hexadecimal digits from `hex` and write the
>* values to `binary` as `len` bytes. Return 0 on success, or -1 if
>* the input does not consist of hex digits).
>*/
>   extern int hex_to_bytes(unsigned char *binary, const char *hex, size_t len);
> 
> The patch moves the comment verbatim.  Words in backticks (`binary`,
> `hex`, `len`) are parameter names.
> 
> The function converts pairs of hexadecimal digits (base 16, ASCII
> encoded) to bytes (base 256).  A byte can be seen as an array of bits;
> thus the output is also binary (base 2) without requiring further
> conversion.
> 
> Calling the variable "binary" may seem unspecific, but makes sense in
> the context of this function.
> 
> Does any of that help?
> 
> Thanks,
> René

Thanks, I have been thinking about it more, and I agree, it does make
sense. 

I had a binary representation in mind, but this is refering to binary
data (just like you can have binary files).

Kevin

Re: [PATCH 1/3] notes: move hex_to_bytes() to hex.c and export it

[René Scharfe]

Am 05.11.2017 um 03:56 schrieb Kevin Daudt:
> On Tue, Oct 31, 2017 at 02:46:49PM +0100, René Scharfe wrote:
>> Make the function for converting pairs of hexadecimal digits to binary
>> available to other call sites.
>>
>> Signed-off-by: Rene Scharfe 
>> ---
>>   cache.h |  7 +++
>>   hex.c   | 12 
>>   notes.c | 17 -
>>   3 files changed, 19 insertions(+), 17 deletions(-)
>>
>> diff --git a/cache.h b/cache.h
>> index 6440e2bf21..f06bfbaf32 100644
>> --- a/cache.h
>> +++ b/cache.h
>> @@ -1317,6 +1317,13 @@ extern int set_disambiguate_hint_config(const char 
>> *var, const char *value);
>>   extern int get_sha1_hex(const char *hex, unsigned char *sha1);
>>   extern int get_oid_hex(const char *hex, struct object_id *sha1);
>>   
>> +/*
>> + * Read `len` pairs of hexadecimal digits from `hex` and write the
>> + * values to `binary` as `len` bytes. Return 0 on success, or -1 if
> 
> Is it correct to call the result binary? I would say that it's the value
> that gets stored. To me, this value does not really have a base.

Here's the full context:

  /*
   * Read `len` pairs of hexadecimal digits from `hex` and write the
   * values to `binary` as `len` bytes. Return 0 on success, or -1 if
   * the input does not consist of hex digits).
   */
  extern int hex_to_bytes(unsigned char *binary, const char *hex, size_t len);

The patch moves the comment verbatim.  Words in backticks (`binary`,
`hex`, `len`) are parameter names.

The function converts pairs of hexadecimal digits (base 16, ASCII
encoded) to bytes (base 256).  A byte can be seen as an array of bits;
thus the output is also binary (base 2) without requiring further
conversion.

Calling the variable "binary" may seem unspecific, but makes sense in
the context of this function.

Does any of that help?

Thanks,
René

[PATCH 3/3] Documentation: revisions: add note about 3dots usages as continuation indications

[Ann T Ropea]

Also, fix typo: "three dot" ---> "three-dot" (align with "two-dot").

Signed-off-by: Ann T Ropea 
---
 Documentation/revisions.txt | 11 ++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/Documentation/revisions.txt b/Documentation/revisions.txt
index 61277469c874..d1b126427177 100644
--- a/Documentation/revisions.txt
+++ b/Documentation/revisions.txt
@@ -271,7 +271,7 @@ The '..' (two-dot) Range Notation::
  for commits that are reachable from r2 excluding those that are reachable
  from r1 by '{caret}r1 r2' and it can be written as 'r1..r2'.
 
-The '...' (three dot) Symmetric Difference Notation::
+The '...' (three-dot) Symmetric Difference Notation::
  A similar notation 'r1\...r2' is called symmetric difference
  of 'r1' and 'r2' and is defined as
  'r1 r2 --not $(git merge-base --all r1 r2)'.
@@ -285,6 +285,15 @@ is a shorthand for 'HEAD..origin' and asks "What did the 
origin do since
 I forked from them?"  Note that '..' would mean 'HEAD..HEAD' which is an
 empty range that is both reachable and unreachable from HEAD.
 
+However, there are instances where '...' is *not*
+equivalent to '...HEAD'.  See the "RAW OUTPUT FORMAT"
+section of linkgit:git-diff[1]: the three-dot notations used
+there are simply continuation indications for the abbreviated
+SHA-1 values.  The ones encountered there are usually
+associated with file/index/tree contents rather than with commit
+objects, and the range operators described above are only
+applicable to commit objects (i.e., 'r1' and 'r2').
+
 Other {caret} Parent Shorthand Notations
 ~
 Three other shorthands exist, particularly useful for merge commits,
-- 
2.13.6

[PATCH 1/3] checkout: describe_detached_head: remove 3dots after committish

[Ann T Ropea]

When a committish, C, is immediately followed by 3dots (...) which are,
in turn, not followed by another committish, we are usually asking for
the revision range C...HEAD, which is known as the symmetric difference
of C and HEAD.

When describe_detached_head is invoked, it prints the committish
followed by 3dots as elaborated above when it indicates the current and
previous HEAD positions.  For example:

   # Randomly check out one of the first seven git.git commits
   # (Starting with a detached HEAD already)
   $ git checkout $(git rev-list master | tail -7 | shuf | head -1)
   Previous HEAD position was bfcf2d7874f7... checkout: describe_detached_head: 
remove 3dots after committish
   HEAD is now at 19b2860cba57... Use "-Wall -O2" for the compiler to get more 
warnings.

"Evaluating" this displayed pseudo-range for the current HEAD indication
resolves to the empty range (C...HEAD, where C equals HEAD).

For the previous HEAD indication, the results of the "evaluation" are
somewhat more difficult to predict: previous here refers to what the
reflog dictates (this is not necessarily the topological ancestor in the
DAG, i.e., HEAD^).  In the example above, the "range" resolves to almost
all commits in the author's clone of git.git.  Running the command again
causes the then previous to current HEAD position "range" to be a lot
smaller.

This could be confusing not only for novices; in either case, no range
should be insinuated by describe_detached_head.

Granted, this "evaluation" is at the moment, if at all, only performed
in the mind of the observer.  And, to be sure, the 3dots *are* intended
as a continuation indication for the abbreviated SHA-1 value.
Nevertheless, we should get rid of them, for the following reasons:

   * they would still be displayed if someone had their core.abbrev
 config value set to the max

   * when the built-in version of checkout was introduced by commit

782c2d65c240 ("Build in checkout", 2008-02-07)

 no 3dots were present in the legacy git-checkout.sh (see
 contrib/examples/git-checkout.sh)

   * when git-reset causes a new HEAD line to be printed (during a hard
 reset), neither builtin/reset.c nor contrib/examples/git-reset.sh
 mention 3dots

Lest we confuse the meticulous observer, we ought to retire the 3dots in
the circumstances described above.

Signed-off-by: Ann T Ropea 
---
 builtin/checkout.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index fc4f8fd2ea29..59cc52e55855 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -404,7 +404,7 @@ static void describe_detached_head(const char *msg, struct 
commit *commit)
struct strbuf sb = STRBUF_INIT;
if (!parse_commit(commit))
pp_commit_easy(CMIT_FMT_ONELINE, commit, );
-   fprintf(stderr, "%s %s... %s\n", msg,
+   fprintf(stderr, "%s %s %s\n", msg,
find_unique_abbrev(commit->object.oid.hash, DEFAULT_ABBREV), 
sb.buf);
strbuf_release();
 }
-- 
2.13.6

[PATCH 2/3] Documentation: user-manual: limit potentially confusing usage of 3dots (and 2dots)

[Ann T Ropea]

Using them as continuation indications for abbreviated SHA-1 values
bears the risk that they are mistaken for the dotted range operators.

Commands which expect a single commit will fail when given a range of
commits.

Also, add a note that sometimes, 3dots are just continuation
indications.

Signed-off-by: Ann T Ropea 
---
 Documentation/user-manual.txt | 20 +++-
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/Documentation/user-manual.txt b/Documentation/user-manual.txt
index b4d88af133e8..bdb44b067399 100644
--- a/Documentation/user-manual.txt
+++ b/Documentation/user-manual.txt
@@ -319,7 +319,7 @@ do so (now or later) by using -b with the checkout command 
again. Example:
 
   git checkout -b new_branch_name
 
-HEAD is now at 427abfa... Linux v2.6.17
+HEAD is now at 427abfa Linux v2.6.17
 
 
 The HEAD then refers to the SHA-1 of the commit instead of to a branch,
@@ -508,7 +508,7 @@ Bisecting: 3537 revisions left to test after this
 
 If you run `git branch` at this point, you'll see that Git has
 temporarily moved you in "(no branch)". HEAD is now detached from any
-branch and points directly to a commit (with commit id 65934...) that
+branch and points directly to a commit (with commit id 65934) that
 is reachable from "master" but not from v2.6.18. Compile and test it,
 and see whether it crashes. Assume it does crash. Then:
 
@@ -549,14 +549,14 @@ says "bisect".  Choose a safe-looking commit nearby, note 
its commit
 id, and check it out with:
 
 -
-$ git reset --hard fb47ddb2db...
+$ git reset --hard fb47ddb2db
 -
 
 then test, run `bisect good` or `bisect bad` as appropriate, and
 continue.
 
 Instead of `git bisect visualize` and then `git reset --hard
-fb47ddb2db...`, you might just want to tell Git that you want to skip
+fb47ddb2db`, you might just want to tell Git that you want to skip
 the current commit:
 
 -
@@ -3426,6 +3426,8 @@ Date:
 ...
 :100644 100644 oldsha... 4b9458b... M somedirectory/myfile
 
+(Note that in the above, the "..." are used as continuation
+indications, not as symmetric difference operators!)
 
 This tells you that the immediately following version of the file was
 "newsha", and that the immediately preceding version was "oldsha".
@@ -3449,7 +3451,7 @@ and your repository is good again!
 $ git log --raw --all
 
 
-and just looked for the sha of the missing object (4b9458b..) in that
+and just looked for the sha of the missing object (4b9458b) in that
 whole thing. It's up to you--Git does *have* a lot of information, it is
 just missing one particular blob version.
 
@@ -4114,9 +4116,9 @@ program, e.g.  `diff3`, `merge`, or Git's own merge-file, 
on
 the blob objects from these three stages yourself, like this:
 
 
-$ git cat-file blob 263414f... >hello.c~1
-$ git cat-file blob 06fa6a2... >hello.c~2
-$ git cat-file blob cc44c73... >hello.c~3
+$ git cat-file blob 263414f >hello.c~1
+$ git cat-file blob 06fa6a2 >hello.c~2
+$ git cat-file blob cc44c73 >hello.c~3
 $ git merge-file hello.c~2 hello.c~1 hello.c~3
 
 
@@ -4374,7 +4376,7 @@ $ git log --no-merges t/
 
 
 In the pager (`less`), just search for "bundle", go a few lines back,
-and see that it is in commit 18449ab0...  Now just copy this object name,
+and see that it is in commit 18449ab0.  Now just copy this object name,
 and paste it into the command line
 
 ---
-- 
2.13.6

Hi Dear

[Mr.charles]

My name is Charles Feeney, a philanthropist and the founder of The Atlantic
Philanthropies, one of the largest private foundations in the world. Dear
I believe strongly in giving while living. I had one idea that 
never
Changed in my mind ― that you should use your wealth to help people and I
have decided to secretly give USD $80Million to randomly selected
Individuals worldwide. On receipt of this email, you should count yourself
the lucky Individual. Dear your email address was chosen online while
Searching at random. Kindly get back to me at your earliest convenience,
So I know your email address is valid and for more details of my free will
Donation to you.
Visit the web page to know more about me: http://en.wikipedia.org/wiki/
Chuck _Feeney
Regards,
Charles Feeney

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

[Michael Haggerty]

On 11/04/2017 01:41 AM, Rafael Ascensão wrote:
> `for_each_glob_ref_in` has some code built into it that converts
> partial refs like 'heads/master' to their full qualified form
> 'refs/heads/master'. It also assume a trailing '/*' if no glob
> characters are present in the pattern.
> 
> Extract that logic to its own function which can be reused elsewhere
> where the same behaviour is needed, and add an ENSURE_GLOB flag
> to toggle if a trailing '/*' is to be appended to the result.
> 
> Signed-off-by: Kevin Daudt 
> Signed-off-by: Rafael Ascensão 
> ---
>  refs.c | 34 --
>  refs.h | 16 
>  2 files changed, 36 insertions(+), 14 deletions(-)
> 
> diff --git a/refs.c b/refs.c
> index c590a992f..1e74b48e6 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -369,32 +369,38 @@ int head_ref_namespaced(each_ref_fn fn, void *cb_data)
>   return ret;
>  }
>  
> -int for_each_glob_ref_in(each_ref_fn fn, const char *pattern,
> - const char *prefix, void *cb_data)
> +void normalize_glob_ref(struct strbuf *normalized_pattern, const char 
> *prefix,
> + const char *pattern, int flags)
>  {
> - struct strbuf real_pattern = STRBUF_INIT;
> - struct ref_filter filter;
> - int ret;
> -
>   if (!prefix && !starts_with(pattern, "refs/"))
> - strbuf_addstr(_pattern, "refs/");
> + strbuf_addstr(normalized_pattern, "refs/");
>   else if (prefix)
> - strbuf_addstr(_pattern, prefix);
> - strbuf_addstr(_pattern, pattern);
> + strbuf_addstr(normalized_pattern, prefix);
> + strbuf_addstr(normalized_pattern, pattern);

I realize that the old code did this too, but while you are in the area
it might be nice to simplify the logic from

if (!prefix && !starts_with(pattern, "refs/"))
strbuf_addstr(normalized_pattern, "refs/");
else if (prefix)
strbuf_addstr(normalized_pattern, prefix);

to

if (prefix)
strbuf_addstr(normalized_pattern, prefix);
else if (!starts_with(pattern, "refs/"))
strbuf_addstr(normalized_pattern, "refs/");

This would avoid having to check twice whether `prefix` is NULL.

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

There shouldn't be a space between the function name and the open
parenthesis.

You have complicated the interface by allowing an `ENSURE_BLOB` flag.
This would make sense if the logic for normalizing the prefix were
tangled up with the logic for adding the suffix. But in fact they are
almost entirely orthogonal [1].

So the interface might be simplified by having two functions,

void normalize_glob_ref(normalized_pattern, prefix, pattern);
void ensure_blob(struct strbuf *pattern);

The caller in this patch would call the functions one after the other
(or the `ensure_blob` behavior could be inlined in
`for_each_glob_ref_in()`, since it doesn't yet have any callers). And
the callers introduced in patch 2 would only need to 

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

[Michael Haggerty]

On 11/04/2017 11:45 PM, Kevin Daudt wrote:
> On Sat, Nov 04, 2017 at 11:27:39AM +0900, Junio C Hamano wrote:
>> I however notice that addition of /* to the tail is trying to be
>> careful by using strbuf_complete('/'), but prefixing with "refs/"
>> does not and we would end up with a double-slash if pattern begins
>> with a slash.  The contract between the caller of this function (or
>> its original, which is for_each_glob_ref_in()) and the callee is
>> that prefix must not begin with '/', so it may be OK, but we might
>> want to add "if (*pattern == '/') BUG(...)" at the beginning.
>>
>> I dunno.  In any case, that is totally outside the scope of this two
>> patch series.
> 
> I do think it's a good idea to make future readers of the code aware of
> this contract, and adding a BUG assert does that quite well. Here is a
> patch that implements it.
> 
> This applies of course on top of this patch series.
> 
> -- >8 --
> Subject: [PATCH] normalize_glob_ref: assert implicit contract of prefix
> 
> normalize_glob_ref has an implicit contract of expecting 'prefix' to not
> start with a '/', otherwise the pattern would end up with a
> double-slash.
> 
> Mark it as a BUG when the prefix argument of normalize_glob_ref starts
> with a '/' so that future callers will be aware of this contract.
> 
> Signed-off-by: Kevin Daudt 
> ---
>  refs.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/refs.c b/refs.c
> index e9ae659ae..6747981d1 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -372,6 +372,8 @@ int head_ref_namespaced(each_ref_fn fn, void *cb_data)
>  void normalize_glob_ref(struct strbuf *normalized_pattern, const char 
> *prefix,
>   const char *pattern, int flags)
>  {
> + if (prefix && *prefix == '/') BUG("prefix cannot not start with '/'");

This should be split onto two lines.

Also, "prefix cannot not start ..." has two "not". I suggest changing it
to "prefix must not start ...", because that makes it clearer that the
caller is at fault.

What if the caller passes the empty string as prefix? In that case, the
end result would be "/", which is also bogus.

> +
>   if (!prefix && !starts_with(pattern, "refs/"))
>   strbuf_addstr(normalized_pattern, "refs/");
>   else if (prefix)

Michael

Re: [PATCH 1/6] t0021/rot13-filter: refactor packet reading functions

[Christian Couder]

On Sun, Oct 22, 2017 at 2:58 AM, Junio C Hamano  wrote:
> Christian Couder  writes:
>
>> To make it possible in a following commit to move packet
>> reading and writing functions into a Packet.pm module,
>> let's refactor these functions, so they don't handle
>> printing debug output and exiting.
>>
>> Signed-off-by: Christian Couder 
>> ---
>>  t/t0021/rot13-filter.pl | 12 
>>  1 file changed, 8 insertions(+), 4 deletions(-)
>>
>> diff --git a/t/t0021/rot13-filter.pl b/t/t0021/rot13-filter.pl
>> index ad685d92f8..e4495a52f3 100644
>> --- a/t/t0021/rot13-filter.pl
>> +++ b/t/t0021/rot13-filter.pl
>> @@ -60,8 +60,7 @@ sub packet_bin_read {
>>   my $bytes_read = read STDIN, $buffer, 4;
>>   if ( $bytes_read == 0 ) {
>>   # EOF - Git stopped talking to us!
>> - print $debug "STOP\n";
>> - exit();
>> + return ( -1, "" );
>>   }
>>   elsif ( $bytes_read != 4 ) {
>>   die "invalid packet: '$buffer'";
>> @@ -85,7 +84,7 @@ sub packet_bin_read {
>>
>>  sub packet_txt_read {
>>   my ( $res, $buf ) = packet_bin_read();
>> - unless ( $buf eq '' or $buf =~ s/\n$// ) {
>> + unless ( $res == -1 or $buf eq '' or $buf =~ s/\n$// ) {
>>   die "A non-binary line MUST be terminated by an LF.";
>>   }
>>   return ( $res, $buf );
>> @@ -131,7 +130,12 @@ print $debug "init handshake complete\n";
>>  $debug->flush();
>>
>>  while (1) {
>> - my ( $command ) = packet_txt_read() =~ /^command=(.+)$/;
>> + my ( $res, $command ) = packet_txt_read();
>> + if ( $res == -1 ) {
>> + print $debug "STOP\n";
>> + exit();
>> + }
>> + $command =~ s/^command=//;
>>   print $debug "IN: $command";
>>   $debug->flush();
>
> This was not an issue in the old code which died upon unexpected EOF
> inside the lowest-level helper packet_bin_read(), but now you have
> one call to packet_bin_read() and many calls to packet_txt_read()
> whose return value is not checked for this new condition you are
> allowing packet_bin_read() to return.  This step taken alone is a
> regression---let's see how the remainder of the series updates the
> callers to compensate.

Yeah, in the new version I will send really soon now, I have made a
number of changes to check the return value for the EOF condition.

> I initially thought that it may be more Perl-ish to return undef or
> string instead of returning a 2-element list, but this code needs to
> distinguish three conditions (i.e. a normal string that is 0 or more
> bytes long, a flush, and an EOF), so that is not sufficient.  Perl
> experts on the list still may be able to suggest a better way than
> the current one to do so, but that is outside the scope of this
> refactoring.

Yeah I can't think of a better way either.

Thanks.

Re: Git Open Source Weekend London 11th/12th November

[Patrick Steinhardt]

On Wed, Nov 01, 2017 at 04:36:24PM +, Thomas Gummerer wrote:
> Hi,
> 
> Bloomberg is hosting an Open Source Weekend in London on the 11th
> & 12th November 2017 to contribute to the Git project.  We have
> also confirmed that Peff will be amongst the mentors on hand to
> guide attendees through their efforts!
> 
> Some of you may notice that we tried to organize this earlier in the
> year, but unfortunately had to postpone it.  For this time around we
> are further along in planning, and it's definitely happening :)
> 
> For those unfamiliar with Bloomberg Open Source weekends: These
> events provide a great way for those who love working on code to
> give back to the community. Come and help make difference to a
> great project!
> 
> There will be tasks provided by the mentors, or bring your own if
> you already have a great idea.  Also if you can't attend the weekend
> and can think of a project which you would like tackled at this
> event please let me know.  Obviously the projects should be
> completable inside a weekend.
> 
> Normally attendees work in small groups on a specific task to
> prevent anyone from getting stuck. Per usual, Bloomberg will
> provide the venue, mentors, snacks and drinks.  Bring your
> enthusiasm (and your laptop!) and come share in the fun!  The
> event is also open to everyone, so feel free to pass on the
> invite!
> 
> The event is free of charge, but please ensure that you are able
> to attend the event before registering.  That will greatly help
> us with accurate numbers for catering so that we don't create
> unwanted waste!
> 
> You can register for the event here:
> 
> https://go.bloomberg.com/attend/invite/git-sprint-hosted-bloomberg/
> 
> Whether you already are a contributor (as probably most people on
> this list are) or interested in starting to contribute to git or
> have some friends that you'd like to get to contribute to git, it
> would be great to see you at the event.
> 
> If you have any further questions feel free to get in touch.
> 
> - Thomas

Hi,

nice to hear, thanks for organizing. Due to having moved to
London just that week I'd love to take part.

As I'm a core contributor of libgit2, I am interested in bringing
forward libgit2 at that occasion. Would that be welcome or should
participants keep strictly to the core git project? Just asking
as I'd like to take part but have a rather long list of topics
for libgit2 on my backlog which I'd like to tackle myself.

Regards
Patrick


signature.asc
Description: PGP signature

[PATCH 2/4] pager: refactor `pager_command_config()`

[Martin Ågren]

`pager_command_config()` checks for the config `pager.`. In the
next commit, we will want to also look for some strings on the form
`pager..foo`.

Refactor the code to verify upfront that the string starts with
"pager." and then check that the remainder is the empty string.
This makes it easy to look for other remainders in the next patch.

While at it, before assigning to `value`, free any old value we might
already have picked up.

Signed-off-by: Martin Ågren 
---
 pager.c | 9 +++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/pager.c b/pager.c
index 92b23e6cd..8968f26f1 100644
--- a/pager.c
+++ b/pager.c
@@ -191,14 +191,19 @@ struct pager_command_config_data {
 static int pager_command_config(const char *var, const char *value, void 
*vdata)
 {
struct pager_command_config_data *data = vdata;
-   const char *cmd;
+   const char *cmd, *remainder;
+
+   if (!skip_prefix(var, "pager.", ) ||
+   !skip_prefix(cmd, data->cmd, ))
+   return 0;
 
-   if (skip_prefix(var, "pager.", ) && !strcmp(cmd, data->cmd)) {
+   if (!*remainder) {
int b = git_parse_maybe_bool(value);
if (b >= 0)
data->want = b;
else {
data->want = 1;
+   free(data->value);
data->value = xstrdup(value);
}
}
-- 
2.15.0.415.gac1375d7e

[PATCH 3/4] pager: introduce `pager.*.command` and `.enable`

[Martin Ågren]

`pager.foo` conflates two concepts: how and whether `git foo` should
page. In particular, it can not be used to change *how* to page without
possibly also affecting *whether*.

Teach Git about two new config items, `pager.foo.command` and
`pager.foo.enable`.

Make this interact sanely with the existing `pager.foo`. For example,
make sure that `pager.foo=false` does not cause us to forget about a
command already configured through `pager.foo.command`, so that the
given pager command can be "re-activated" using
`pager.foo[.enable]=true`.

Where Documentation/ refers to `pager.tag`, write "the `pager.tag[.*]`
configuration options". In config.txt, `pager.blame` is mentioned more
as an example and it describes precisely the situation where one will
want to use the old mechanism, so leave that instance unchanged.

For symmetry with how `--paginate` disrespects any pager that might have
been configured with `pager.foo`, do the same for `pager.foo.command`.

Signed-off-by: Martin Ågren 
---
 Documentation/config.txt  | 17 +++
 Documentation/git-tag.txt |  3 +-
 Documentation/git.txt |  2 +-
 t/t7006-pager.sh  | 73 +++
 pager.c   |  5 
 5 files changed, 98 insertions(+), 2 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 1ac0ae6ad..72558cc74 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2460,6 +2460,23 @@ pager.::
or `--no-pager` is specified on the command line, it takes
precedence over this option.  To disable pagination for all
commands, set `core.pager` or `GIT_PAGER` to `cat`.
++
+This is a less flexible alternative to `pager..command` and
+`pager..enable`. Using it with a boolean does the same as using
+`pager..enable`. Using it with a command does the same as using
+`pager..command` and `pager..enable=true`.
+
+pager..command::
+   Specifies the pager to use for the subcommand.
+   Whether the pager should be used is configured using
+   `pager..enable` or `pager.=`.
+
+pager..enable::
+   A boolean which turns on or off pagination of the output of a
+   particular Git subcommand when writing to a tty. If `--paginate`
+   or `--no-pager` is specified on the command line, it takes
+   precedence over this option.  To disable pagination for all
+   commands, set `core.pager` or `GIT_PAGER` to `cat`.
 
 pretty.::
Alias for a --pretty= format string, as specified in
diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index 956fc019f..9f9f33409 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -210,7 +210,8 @@ it in the repository configuration as follows:
 signingKey = 
 -
 
-`pager.tag` is only respected when listing tags, i.e., when `-l` is
+The `pager.tag[.*]` configuration options are only
+respected when listing tags, i.e., when `-l` is
 used or implied. The default is to use a pager.
 See linkgit:git-config[1].
 
diff --git a/Documentation/git.txt b/Documentation/git.txt
index 7a1d629ca..0a2eff7a6 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -99,7 +99,7 @@ foo.bar= ...`) sets `foo.bar` to the empty string which `git 
config
 -p::
 --paginate::
Pipe all output into 'less' (or if set, $PAGER) if standard
-   output is a terminal.  This overrides the `pager.`
+   output is a terminal.  This overrides the `pager.[.*]`
configuration options (see the "Configuration Mechanism" section
below).
 
diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index e890b2f64..6966627dd 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -588,4 +588,77 @@ test_expect_success 'command with underscores does not 
complain' '
test_cmp expect actual
 '
 
+test_expect_success 'setup' '
+   sane_unset PAGER GIT_PAGER GIT_PAGER_IN_USE &&
+   test_unconfig core.pager &&
+
+   git rev-list HEAD >rev-list &&
+   sed "s/^/foo:/" rev-list >expect &&
+
+   PAGER="cat >paginated.out" &&
+   export PAGER &&
+
+   test_unconfig pager.log &&
+   test_unconfig pager.rev-list
+'
+
+test_expect_success TTY 'configuration with .enable works' '
+   rm -f paginated.out &&
+   test_terminal git -c pager.log.enable=false log &&
+   ! test -e paginated.out
+'
+
+test_expect_success TTY '--paginate overrides .enable+.command' '
+   rm -f paginated.out &&
+   test_terminal git -c pager.log.command=bad -c pager.log.enable=false \
+ --paginate log &&
+   test -e paginated.out
+'
+
+test_expect_success TTY '--no-pager overrides .enable' '
+   rm -f paginated.out &&
+   test_terminal git -c pager.rev-list.enable --no-pager rev-list HEAD &&
+   ! test -e paginated.out
+'
+
+test_expect_success TTY '.enable discards non-boolean' '
+   test_must_fail git -c pager.log.enable=bad log
+'
+

[PATCH 4/4] pager: make `pager.foo.command` imply `.enable=true`

[Martin Ågren]

If `pager.foo.command` gets configured and there is no configuration
(yet) saying whether we should page, act as if `pager.foo.enable=true`.

This means that a lone `pager.foo` can always be written as a lone
`pager.foo.command` or `pager.foo.enable`.

Signed-off-by: Martin Ågren 
---
 Documentation/config.txt | 2 ++
 t/t7006-pager.sh | 7 +++
 pager.c  | 2 ++
 3 files changed, 11 insertions(+)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 72558cc74..91cc8b110 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2470,6 +2470,8 @@ pager..command::
Specifies the pager to use for the subcommand.
Whether the pager should be used is configured using
`pager..enable` or `pager.=`.
+   Implies `pager..enable=true` unless `pager..enable`
+   or `pager.` have already been given.
 
 pager..enable::
A boolean which turns on or off pagination of the output of a
diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index 6966627dd..c5246a57d 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -625,6 +625,13 @@ test_expect_success TTY '.enable discards non-boolean' '
test_must_fail git -c pager.log.enable=bad log
 '
 
+test_expect_success TTY 'configuration with .command turns on paging' '
+   >actual &&
+   test_terminal git -c pager.rev-list.command="sed s/^/foo:/ >actual" \
+ rev-list HEAD &&
+   test_cmp expect actual
+'
+
 test_expect_success TTY 'configuration remembers .command as .enable flips' '
>actual &&
test_terminal git -c pager.rev-list.command="sed s/^/foo:/ >actual" \
diff --git a/pager.c b/pager.c
index c8a6a01d8..0e037abf4 100644
--- a/pager.c
+++ b/pager.c
@@ -209,6 +209,8 @@ static int pager_command_config(const char *var, const char 
*value, void *vdata)
} else if (!strcmp(remainder, ".command")) {
free(data->value);
data->value = xstrdup(value);
+   if (data->want == -1)
+   data->want = 1;
} else if (!strcmp(remainder, ".enable")) {
data->want = git_config_bool(var, value);
}
-- 
2.15.0.415.gac1375d7e

[PATCH 1/4] t7006: document that `pager.foo` can be partially preserved

[Martin Ågren]

Assuming a blank slate, consider the following configuration:

[pager]
foo = some-command
foo = false

Now `git -c pager.foo=true foo` will page using `some-command`. This
might have been intended, or is perhaps just a side-effect of the
implementation. In any case, it could be useful and someone might rely
on it, either knowingly (as above) or not (if these lines are spread out
across the configuration).

However, `git --paginate foo` will *not* use `some-command`. That
matches the documentation (Documentation/git.txt).

Upcoming commits will expand on how paging for `git foo` can be
configured. Those commits mustn't change how `pager.foo` behaves, so add
tests for these two cases.

Signed-off-by: Martin Ågren 
---
 t/t7006-pager.sh | 18 ++
 1 file changed, 18 insertions(+)

diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index f0f1abd1c..e890b2f64 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -117,6 +117,24 @@ test_expect_success TTY 'git config uses a pager if 
configured to' '
test -e paginated.out
 '
 
+test_expect_success TTY 'configuration remembers pager across boolean changes' 
'
+   echo paging >expected &&
+   test_unconfig pager.config &&
+   test_terminal git -c pager.config="echo paging" \
+ -c pager.config=false \
+ -c pager.config \
+ config --list >actual &&
+   test_cmp expected actual
+'
+
+test_expect_success TTY '--paginate does not respect inactivated pager' '
+   rm -f paginated.out &&
+   test_terminal git -c pager.config=bad \
+ -c pager.config=false \
+ --paginate config --list &&
+   test -e paginated.out
+'
+
 test_expect_success TTY 'configuration can enable pager (from subdir)' '
rm -f paginated.out &&
mkdir -p subdir &&
-- 
2.15.0.415.gac1375d7e

[PATCH/DONOTAPPLY 0/4] first steps towards pager.foo.{command,enable}

[Martin Ågren]

I'm not posting this for inclusion (yet), but because I read this:

On 4 November 2017 at 10:28, Jeff King  wrote:
>  - the pager. config is mis-designed, because our config keys
>cannot represent all possible command names (e.g., case folding and
>illegal characters). This should be pager..enable or similar.
>Some discussion in (this message and the surrounding thread):
>
>  
> https://public-inbox.org/git/20170711101942.h2uwxtgzvgguz...@sigill.intra.peff.net/
>
>But I think you could find more by searching the archive.

I'm posting four patches I have on this to save others from redoing my
work and findings. These patches feel a bit incomplete, which is why I
put them to the side some time ago (and eventually forgot about them).

In particular, they do not teach `--paginate` to use the pager
configured by `pager.foo.command`. It is already now possible to use
`pager.foo` to say "I don't want you to page, but if I later give you
`pager.foo=true`, this is the pager I want you to use". That does not
work with `--paginate`, but this can all be explained -- indeed, we
document that `--paginate` overrules `pager.foo`.

If we teach `--paginate` to respect `pager.foo.command`, it seems that
we would either 1) introduce a small (and possibly hard to understand
and explain) difference between the old-style and the new-style
pager-configuration or 2) knowingly change the behavior of `--paginate`
with `pager.foo` or 3) knowingly change the behavior of
`pager.foo=false` as documented in the first patch.

I think there's great value to being able to say "this is the same as
this, and that is the same as that", but that might get muddied by "oh,
except if you use `--paginate`".

If someone wants to pick these up and bring them to completion, great!
If not and if I or someone else feels confident about which way to go,
then I can revisit these.

Martin

Martin Ågren (4):
  t7006: document that `pager.foo` can be partially preserved
  pager: refactor `pager_command_config()`
  pager: introduce `pager.*.command` and `.enable`
  pager: make `pager.foo.command` imply `.enable=true`

 Documentation/config.txt  | 19 +
 Documentation/git-tag.txt |  3 +-
 Documentation/git.txt |  2 +-
 t/t7006-pager.sh  | 98 +++
 pager.c   | 16 +++-
 5 files changed, 134 insertions(+), 4 deletions(-)

-- 
2.15.0.415.gac1375d7e

Your long awaited part payment of $2.5.000.00Usd

[UNITED NATIONS]

Attention: Beneficiary,

Your long awaited part payment of $2.5.000.00Usd (TWO MILLION FIVE Hundred
Thousand United State Dollars)  is ready for immediate release to you, and
it was electronically credited into an ATM Visa Card for easy delivery.

Your new Payment Reference No.- 6363836,
Password No: 006786,
Pin Code No: 1787
Your Certificate of Merit Payment No: 05872,
Released Code No: 1134;
Immediate Telex confirmation No:- 043612;
Secret Code No: TBKTA28
Re-Confirm;
Your Names: |Address: |Cell Phone: |Fax: | Amount: |
Your immediate response is needed
Access Bank

Person to Contact:MR DANIEL ETIM  the Director of the International Audit
unit ATM Payment Center,

Email: bankplc12...@financier.com
TELEPHONE: +226 68699544

Note: Your file will expire after 7 days if there is no response, and then
we will not have any option than to return your fund to IMF as unclaimed.
Your swift response will enhance our service, thanks for your co-operation;

Regards.
Mr JOHN MARK.

[PATCH v2 2/9] prune_ref(): call `ref_transaction_add_update()` directly

[Michael Haggerty]

`prune_ref()` needs to use the `REF_ISPRUNING` flag, but we want to
make that flag private to the files backend. So instead of calling
`ref_transaction_delete()`, which is a public function and therefore
shouldn't allow the `REF_ISPRUNING` flag, change `prune_ref()` to call
`ref_transaction_add_update()`, which is private to the refs
module. (Note that we don't need any of the other services provided by
`ref_transaction_delete()`.)

This allows us to change `ref_transaction_update()` to reject the
`REF_ISPRUNING` flag. Do so by adjusting
`REF_TRANSACTION_UPDATE_ALLOWED_FLAGS`. Also add parentheses to its
definition to avoid potential future mishaps.

Signed-off-by: Michael Haggerty 
---
 refs.h   |  4 +---
 refs/files-backend.c | 25 -
 2 files changed, 17 insertions(+), 12 deletions(-)

diff --git a/refs.h b/refs.h
index 15fd419c7d..4ffef9502d 100644
--- a/refs.h
+++ b/refs.h
@@ -349,9 +349,7 @@ int refs_pack_refs(struct ref_store *refs, unsigned int 
flags);
  * Flags that can be passed in to ref_transaction_update
  */
 #define REF_TRANSACTION_UPDATE_ALLOWED_FLAGS \
-   REF_ISPRUNING |  \
-   REF_FORCE_CREATE_REFLOG |\
-   REF_NODEREF
+   (REF_NODEREF | REF_FORCE_CREATE_REFLOG)
 
 /*
  * Setup reflog before using. Fill in err and return -1 on failure.
diff --git a/refs/files-backend.c b/refs/files-backend.c
index fadf1036d3..ba72d28b13 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -989,22 +989,29 @@ static void prune_ref(struct files_ref_store *refs, 
struct ref_to_prune *r)
 {
struct ref_transaction *transaction;
struct strbuf err = STRBUF_INIT;
+   int ret = -1;
 
if (check_refname_format(r->name, 0))
return;
 
transaction = ref_store_transaction_begin(>base, );
-   if (!transaction ||
-   ref_transaction_delete(transaction, r->name, >oid,
-  REF_ISPRUNING | REF_NODEREF, NULL, ) ||
-   ref_transaction_commit(transaction, )) {
-   ref_transaction_free(transaction);
+   if (!transaction)
+   goto cleanup;
+   ref_transaction_add_update(
+   transaction, r->name,
+   REF_NODEREF | REF_HAVE_NEW | REF_HAVE_OLD | 
REF_ISPRUNING,
+   _oid, >oid, NULL);
+   if (ref_transaction_commit(transaction, ))
+   goto cleanup;
+
+   ret = 0;
+
+cleanup:
+   if (ret)
error("%s", err.buf);
-   strbuf_release();
-   return;
-   }
-   ref_transaction_free(transaction);
strbuf_release();
+   ref_transaction_free(transaction);
+   return;
 }
 
 /*
-- 
2.14.1

[PATCH v2 4/9] ref_transaction_add_update(): remove a check

[Michael Haggerty]

We want to make `REF_ISPRUNING` internal to the files backend. For
this to be possible, `ref_transaction_add_update()` mustn't know about
it. So move the check that `REF_ISPRUNING` is only used with
`REF_NODEREF` from this function to `files_transaction_prepare()`.

Signed-off-by: Michael Haggerty 
---
 refs.c   | 3 ---
 refs/files-backend.c | 7 ++-
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/refs.c b/refs.c
index 7c1e206e08..0d9a1348cd 100644
--- a/refs.c
+++ b/refs.c
@@ -906,9 +906,6 @@ struct ref_update *ref_transaction_add_update(
if (transaction->state != REF_TRANSACTION_OPEN)
die("BUG: update called for transaction that is not open");
 
-   if ((flags & REF_ISPRUNING) && !(flags & REF_NODEREF))
-   die("BUG: REF_ISPRUNING set without REF_NODEREF");
-
FLEX_ALLOC_STR(update, refname, refname);
ALLOC_GROW(transaction->updates, transaction->nr + 1, 
transaction->alloc);
transaction->updates[transaction->nr++] = update;
diff --git a/refs/files-backend.c b/refs/files-backend.c
index ba72d28b13..a47771e4d4 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2518,13 +2518,18 @@ static int files_transaction_prepare(struct ref_store 
*ref_store,
 * transaction. (If we end up splitting up any updates using
 * split_symref_update() or split_head_update(), those
 * functions will check that the new updates don't have the
-* same refname as any existing ones.)
+* same refname as any existing ones.) Also fail if any of the
+* updates use REF_ISPRUNING without REF_NODEREF.
 */
for (i = 0; i < transaction->nr; i++) {
struct ref_update *update = transaction->updates[i];
struct string_list_item *item =
string_list_append(_refnames, update->refname);
 
+   if ((update->flags & REF_ISPRUNING) &&
+   !(update->flags & REF_NODEREF))
+   BUG("REF_ISPRUNING set without REF_NODEREF");
+
/*
 * We store a pointer to update in item->util, but at
 * the moment we never use the value of this field
-- 
2.14.1

[PATCH v2 6/9] refs: rename constant `REF_NODEREF` to `REF_NO_DEREF`

[Michael Haggerty]

Even after working with this code for years, I still see this constant
name as "ref node ref". Rename it to make it's meaning clearer.

Signed-off-by: Michael Haggerty 
---
 builtin/am.c   |  2 +-
 builtin/branch.c   |  2 +-
 builtin/checkout.c |  2 +-
 builtin/clone.c|  4 ++--
 builtin/notes.c|  2 +-
 builtin/remote.c   |  6 +++---
 builtin/symbolic-ref.c |  2 +-
 builtin/update-ref.c   |  4 ++--
 refs.h |  6 +++---
 refs/files-backend.c   | 40 
 refs/refs-internal.h   |  4 ++--
 sequencer.c|  6 +++---
 12 files changed, 40 insertions(+), 40 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index c9bb14a6c2..894290e2d3 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -2151,7 +2151,7 @@ static void am_abort(struct am_state *state)
   has_curr_head ? _head : NULL, 0,
   UPDATE_REFS_DIE_ON_ERR);
else if (curr_branch)
-   delete_ref(NULL, curr_branch, NULL, REF_NODEREF);
+   delete_ref(NULL, curr_branch, NULL, REF_NO_DEREF);
 
free(curr_branch);
am_destroy(state);
diff --git a/builtin/branch.c b/builtin/branch.c
index b1ed649300..33fd5fcfd1 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -258,7 +258,7 @@ static int delete_branches(int argc, const char **argv, int 
force, int kinds,
}
 
if (delete_ref(NULL, name, is_null_oid() ? NULL : ,
-  REF_NODEREF)) {
+  REF_NO_DEREF)) {
error(remote_branch
  ? _("Error deleting remote-tracking branch '%s'")
  : _("Error deleting branch '%s'"),
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 463a337e5d..114028ee01 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -665,7 +665,7 @@ static void update_refs_for_switch(const struct 
checkout_opts *opts,
/* Nothing to do. */
} else if (opts->force_detach || !new->path) {  /* No longer on any 
branch. */
update_ref(msg.buf, "HEAD", >commit->object.oid, NULL,
-  REF_NODEREF, UPDATE_REFS_DIE_ON_ERR);
+  REF_NO_DEREF, UPDATE_REFS_DIE_ON_ERR);
if (!opts->quiet) {
if (old->path &&
advice_detached_head && !opts->force_detach)
diff --git a/builtin/clone.c b/builtin/clone.c
index 695bdd7046..557c6c3c06 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -689,7 +689,7 @@ static void update_head(const struct ref *our, const struct 
ref *remote,
} else if (our) {
struct commit *c = lookup_commit_reference(>old_oid);
/* --branch specifies a non-branch (i.e. tags), detach HEAD */
-   update_ref(msg, "HEAD", >object.oid, NULL, REF_NODEREF,
+   update_ref(msg, "HEAD", >object.oid, NULL, REF_NO_DEREF,
   UPDATE_REFS_DIE_ON_ERR);
} else if (remote) {
/*
@@ -697,7 +697,7 @@ static void update_head(const struct ref *our, const struct 
ref *remote,
 * HEAD points to a branch but we don't know which one.
 * Detach HEAD in all these cases.
 */
-   update_ref(msg, "HEAD", >old_oid, NULL, REF_NODEREF,
+   update_ref(msg, "HEAD", >old_oid, NULL, REF_NO_DEREF,
   UPDATE_REFS_DIE_ON_ERR);
}
 }
diff --git a/builtin/notes.c b/builtin/notes.c
index 12afdf1907..d7754db143 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -686,7 +686,7 @@ static int merge_abort(struct notes_merge_options *o)
 
if (delete_ref(NULL, "NOTES_MERGE_PARTIAL", NULL, 0))
ret += error(_("failed to delete ref NOTES_MERGE_PARTIAL"));
-   if (delete_ref(NULL, "NOTES_MERGE_REF", NULL, REF_NODEREF))
+   if (delete_ref(NULL, "NOTES_MERGE_REF", NULL, REF_NO_DEREF))
ret += error(_("failed to delete ref NOTES_MERGE_REF"));
if (notes_merge_abort(o))
ret += error(_("failed to remove 'git notes merge' worktree"));
diff --git a/builtin/remote.c b/builtin/remote.c
index 0fddc64461..3d38c6150c 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -693,7 +693,7 @@ static int mv(int argc, const char **argv)
read_ref_full(item->string, RESOLVE_REF_READING, , );
if (!(flag & REF_ISSYMREF))
continue;
-   if (delete_ref(NULL, item->string, NULL, REF_NODEREF))
+   if (delete_ref(NULL, item->string, NULL, REF_NO_DEREF))
die(_("deleting '%s' failed"), item->string);
}
for (i = 0; i < remote_branches.nr; i++) {
@@ -788,7 +788,7 @@ static int rm(int argc, const char **argv)
strbuf_release();
 
if (!result)
-

[PATCH v2 3/9] ref_transaction_update(): die on disallowed flags

[Michael Haggerty]

Callers shouldn't be passing disallowed flags into
`ref_transaction_update()`. So instead of masking them off, treat it
as a bug if any are set.

Signed-off-by: Michael Haggerty 
---
 refs.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/refs.c b/refs.c
index 62a7621025..7c1e206e08 100644
--- a/refs.c
+++ b/refs.c
@@ -940,7 +940,8 @@ int ref_transaction_update(struct ref_transaction 
*transaction,
return -1;
}
 
-   flags &= REF_TRANSACTION_UPDATE_ALLOWED_FLAGS;
+   if (flags & ~REF_TRANSACTION_UPDATE_ALLOWED_FLAGS)
+   BUG("illegal flags 0x%x passed to ref_transaction_update()", 
flags);
 
flags |= (new_oid ? REF_HAVE_NEW : 0) | (old_oid ? REF_HAVE_OLD : 0);
 
-- 
2.14.1

[PATCH v2 9/9] refs: update some more docs to use "oid" rather than "sha1"

[Michael Haggerty]

Signed-off-by: Michael Haggerty 
---
 refs.c|  2 +-
 refs.h|  8 
 refs/files-backend.c  | 19 +--
 refs/packed-backend.c |  2 +-
 refs/ref-cache.c  |  4 ++--
 refs/refs-internal.h  | 18 +-
 6 files changed, 26 insertions(+), 27 deletions(-)

diff --git a/refs.c b/refs.c
index 0d9a1348cd..339d4318ee 100644
--- a/refs.c
+++ b/refs.c
@@ -770,7 +770,7 @@ static int read_ref_at_ent(struct object_id *ooid, struct 
object_id *noid,
if (cb->cutoff_cnt)
*cb->cutoff_cnt = cb->reccnt - 1;
/*
-* we have not yet updated cb->[n|o]sha1 so they still
+* we have not yet updated cb->[n|o]oid so they still
 * hold the values for the previous record.
 */
if (!is_null_oid(>ooid)) {
diff --git a/refs.h b/refs.h
index d396012367..18582a408c 100644
--- a/refs.h
+++ b/refs.h
@@ -126,7 +126,7 @@ int peel_ref(const char *refname, struct object_id *oid);
 /**
  * Resolve refname in the nested "gitlink" repository in the specified
  * submodule (which must be non-NULL). If the resolution is
- * successful, return 0 and set sha1 to the name of the object;
+ * successful, return 0 and set oid to the name of the object;
  * otherwise, return a non-zero value.
  */
 int resolve_gitlink_ref(const char *submodule, const char *refname,
@@ -260,7 +260,7 @@ struct ref_transaction;
 
 /*
  * The signature for the callback function for the for_each_*()
- * functions below.  The memory pointed to by the refname and sha1
+ * functions below.  The memory pointed to by the refname and oid
  * arguments is only guaranteed to be valid for the duration of a
  * single callback invocation.
  */
@@ -354,7 +354,7 @@ int reflog_exists(const char *refname);
 
 /*
  * Delete the specified reference. If old_oid is non-NULL, then
- * verify that the current value of the reference is old_sha1 before
+ * verify that the current value of the reference is old_oid before
  * deleting it. If old_oid is NULL, delete the reference if it
  * exists, regardless of its old value. It is an error for old_oid to
  * be null_oid. msg and flags are passed through to
@@ -633,7 +633,7 @@ int ref_transaction_abort(struct ref_transaction 
*transaction,
  * It is a bug to call this function when there might be other
  * processes accessing the repository or if there are existing
  * references that might conflict with the ones being created. All
- * old_sha1 values must either be absent or NULL_SHA1.
+ * old_oid values must either be absent or null_oid.
  */
 int initial_ref_transaction_commit(struct ref_transaction *transaction,
   struct strbuf *err);
diff --git a/refs/files-backend.c b/refs/files-backend.c
index bb10b715a8..2298f900dd 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -240,7 +240,7 @@ static void loose_fill_ref_dir(struct ref_store *ref_store,
} else if (is_null_oid()) {
/*
 * It is so astronomically unlikely
-* that NULL_SHA1 is the SHA-1 of an
+* that null_oid is the OID of an
 * actual object that we consider its
 * appearance in a loose reference
 * file to be repo corruption
@@ -473,7 +473,7 @@ static void unlock_ref(struct ref_lock *lock)
  * are passed to refs_verify_refname_available() for this check.
  *
  * If mustexist is not set and the reference is not found or is
- * broken, lock the reference anyway but clear sha1.
+ * broken, lock the reference anyway but clear old_oid.
  *
  * Return 0 on success. On failure, write an error message to err and
  * return TRANSACTION_NAME_CONFLICT or TRANSACTION_GENERIC_ERROR.
@@ -1648,9 +1648,8 @@ static int files_log_ref_write(struct files_ref_store 
*refs,
 }
 
 /*
- * Write sha1 into the open lockfile, then close the lockfile. On
- * errors, rollback the lockfile, fill in *err and
- * return -1.
+ * Write oid into the open lockfile, then close the lockfile. On
+ * errors, rollback the lockfile, fill in *err and return -1.
  */
 static int write_ref_to_lockfile(struct ref_lock *lock,
 const struct object_id *oid, struct strbuf 
*err)
@@ -2272,7 +2271,7 @@ static int split_symref_update(struct files_ref_store 
*refs,
 
/*
 * Change the symbolic ref update to log only. Also, it
-* doesn't need to check its old SHA-1 value, as that will be
+* doesn't need to check its old OID value, as that will be
 * done when new_update is processed.
 */
update->flags |= REF_LOG_ONLY | REF_NO_DEREF;
@@ -2341,7 +2340,7 @@ static int check_old_oid(struct ref_update *update, 
struct object_id *oid,
  * Prepare for carrying out 

[PATCH v2 5/9] refs: tidy up and adjust visibility of the `ref_update` flags

[Michael Haggerty]

The constants used for `ref_update::flags` were rather disorganized:

* The definitions in `refs.h` were not close to the functions that
  used them.

* Maybe constants were defined in `refs-internal.h`, making them
  visible to the whole refs module, when in fact they only made sense
  for the files backend.

* Their documentation wasn't very consistent and partly still referred
  to sha1s rather than oids.

* The numerical values followed no rational scheme

Fix all of these problems. The main functional improvement is that
some constants' visibility is now limited to `files-backend.c`.

Signed-off-by: Michael Haggerty 
---
 refs.h   | 67 ++--
 refs/files-backend.c | 45 +++
 refs/refs-internal.h | 67 
 3 files changed, 99 insertions(+), 80 deletions(-)

diff --git a/refs.h b/refs.h
index 4ffef9502d..261d46c10c 100644
--- a/refs.h
+++ b/refs.h
@@ -335,22 +335,6 @@ void warn_dangling_symrefs(FILE *fp, const char *msg_fmt,
  */
 int refs_pack_refs(struct ref_store *refs, unsigned int flags);
 
-/*
- * Flags controlling ref_transaction_update(), ref_transaction_create(), etc.
- * REF_NODEREF: act on the ref directly, instead of dereferencing
- *  symbolic references.
- *
- * Other flags are reserved for internal use.
- */
-#define REF_NODEREF0x01
-#define REF_FORCE_CREATE_REFLOG 0x40
-
-/*
- * Flags that can be passed in to ref_transaction_update
- */
-#define REF_TRANSACTION_UPDATE_ALLOWED_FLAGS \
-   (REF_NODEREF | REF_FORCE_CREATE_REFLOG)
-
 /*
  * Setup reflog before using. Fill in err and return -1 on failure.
  */
@@ -478,22 +462,23 @@ struct ref_transaction *ref_transaction_begin(struct 
strbuf *err);
  *
  * refname -- the name of the reference to be affected.
  *
- * new_sha1 -- the SHA-1 that should be set to be the new value of
- * the reference. Some functions allow this parameter to be
+ * new_oid -- the object ID that should be set to be the new value
+ * of the reference. Some functions allow this parameter to be
  * NULL, meaning that the reference is not changed, or
- * null_sha1, meaning that the reference should be deleted. A
+ * null_oid, meaning that the reference should be deleted. A
  * copy of this value is made in the transaction.
  *
- * old_sha1 -- the SHA-1 value that the reference must have before
+ * old_oid -- the object ID that the reference must have before
  * the update. Some functions allow this parameter to be NULL,
  * meaning that the old value of the reference is not checked,
- * or null_sha1, meaning that the reference must not exist
+ * or null_oid, meaning that the reference must not exist
  * before the update. A copy of this value is made in the
  * transaction.
  *
  * flags -- flags affecting the update, passed to
- * update_ref_lock(). Can be REF_NODEREF, which means that
- * symbolic references should not be followed.
+ * update_ref_lock(). Possible flags: REF_NODEREF,
+ * REF_FORCE_CREATE_REFLOG. See those constants for more
+ * information.
  *
  * msg -- a message describing the change (for the reflog).
  *
@@ -509,11 +494,37 @@ struct ref_transaction *ref_transaction_begin(struct 
strbuf *err);
  */
 
 /*
- * Add a reference update to transaction. new_oid is the value that
- * the reference should have after the update, or null_oid if it
- * should be deleted. If new_oid is NULL, then the reference is not
- * changed at all. old_oid is the value that the reference must have
- * before the update, or null_oid if it must not have existed
+ * The following flags can be passed to ref_transaction_update() etc.
+ * Internally, they are stored in `ref_update::flags`, along with some
+ * internal flags.
+ */
+
+/*
+ * Act on the ref directly; i.e., without dereferencing symbolic refs.
+ * If this flag is not specified, then symbolic references are
+ * dereferenced and the update is applied to the referent.
+ */
+#define REF_NODEREF (1 << 0)
+
+/*
+ * Force the creation of a reflog for this reference, even if it
+ * didn't previously have a reflog.
+ */
+#define REF_FORCE_CREATE_REFLOG (1 << 1)
+
+/*
+ * Bitmask of all of the flags that are allowed to be passed in to
+ * ref_transaction_update() and friends:
+ */
+#define REF_TRANSACTION_UPDATE_ALLOWED_FLAGS \
+   (REF_NODEREF | REF_FORCE_CREATE_REFLOG)
+
+/*
+ * Add a reference update to transaction. `new_oid` is the value that
+ * the reference should have after the update, or `null_oid` if it
+ * should be deleted. If `new_oid` is NULL, then the reference is not
+ * changed at all. `old_oid` is the value that the reference must have
+ * before the update, or `null_oid` if it must not have existed
  * beforehand. The old value is checked after the lock is taken to
 

[PATCH v2 0/9] Tidy up the constants related to ref_update::flags

[Michael Haggerty]

This is a reroll of a patch series that tidies up some stuff around
the ref_update::flags constants. Thanks to Junio and Martin for their
comments about v1 [1].

Relative to v1, this version:

* In patch 5, cleans up the touched comments to refer to OIDs rather
  than SHA-1s.

* Adds a patch 8, which changes `write_packed_entry()` to take
  `object_id` arguments.

* Adds a patch 9, which cleans up some remaining comments across all
  of the refs-related files to refer to OIDs rather than SHA-1s.

This patch series depends on bc/object-id. The patches are also
available from my GitHub fork as branch `tidy-ref-update-flags` [2].

Michael

[1] https://public-inbox.org/git/cover.1509183413.git.mhag...@alum.mit.edu/
[2] https://github.com/mhagger/git

Michael Haggerty (9):
  files_transaction_prepare(): don't leak flags to packed transaction
  prune_ref(): call `ref_transaction_add_update()` directly
  ref_transaction_update(): die on disallowed flags
  ref_transaction_add_update(): remove a check
  refs: tidy up and adjust visibility of the `ref_update` flags
  refs: rename constant `REF_NODEREF` to `REF_NO_DEREF`
  refs: rename constant `REF_ISPRUNING` to `REF_IS_PRUNING`
  write_packed_entry(): take `object_id` arguments
  refs: update some more docs to use "oid" rather than "sha1"

 builtin/am.c   |   2 +-
 builtin/branch.c   |   2 +-
 builtin/checkout.c |   2 +-
 builtin/clone.c|   4 +-
 builtin/notes.c|   2 +-
 builtin/remote.c   |   6 +--
 builtin/symbolic-ref.c |   2 +-
 builtin/update-ref.c   |   4 +-
 refs.c |   8 ++-
 refs.h |  77 -
 refs/files-backend.c   | 132 +++--
 refs/packed-backend.c  |  18 +++
 refs/ref-cache.c   |   4 +-
 refs/refs-internal.h   |  81 +-
 sequencer.c|   6 +--
 15 files changed, 188 insertions(+), 162 deletions(-)

-- 
2.14.1

[PATCH v2 1/9] files_transaction_prepare(): don't leak flags to packed transaction

[Michael Haggerty]

The files backend uses `ref_update::flags` for several internal flags.
But those flags have no meaning to the packed backend. So when adding
updates for the packed-refs transaction, only use flags that make
sense to the packed backend.

`REF_NODEREF` is part of the public interface, and it's logically what
we want, so include it. In fact it is actually ignored by the packed
backend (which doesn't support symbolic references), but that's its
own business.

Signed-off-by: Michael Haggerty 
---
 refs/files-backend.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 2bd54e11ae..fadf1036d3 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2594,8 +2594,8 @@ static int files_transaction_prepare(struct ref_store 
*ref_store,
 
ref_transaction_add_update(
packed_transaction, update->refname,
-   update->flags & ~REF_HAVE_OLD,
-   >new_oid, >old_oid,
+   REF_HAVE_NEW | REF_NODEREF,
+   >new_oid, NULL,
NULL);
}
}
-- 
2.14.1

[PATCH v2 7/9] refs: rename constant `REF_ISPRUNING` to `REF_IS_PRUNING`

[Michael Haggerty]

Underscores are cheap, and help readability.

Signed-off-by: Michael Haggerty 
---
 refs/files-backend.c | 18 +-
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 71e088e811..bb10b715a8 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -14,7 +14,7 @@
  * This backend uses the following flags in `ref_update::flags` for
  * internal bookkeeping purposes. Their numerical values must not
  * conflict with REF_NO_DEREF, REF_FORCE_CREATE_REFLOG, REF_HAVE_NEW,
- * REF_HAVE_OLD, or REF_ISPRUNING, which are also stored in
+ * REF_HAVE_OLD, or REF_IS_PRUNING, which are also stored in
  * `ref_update::flags`.
  */
 
@@ -22,7 +22,7 @@
  * Used as a flag in ref_update::flags when a loose ref is being
  * pruned. This flag must only be used when REF_NO_DEREF is set.
  */
-#define REF_ISPRUNING (1 << 4)
+#define REF_IS_PRUNING (1 << 4)
 
 /*
  * Flag passed to lock_ref_sha1_basic() telling it to tolerate broken
@@ -1044,7 +1044,7 @@ static void prune_ref(struct files_ref_store *refs, 
struct ref_to_prune *r)
goto cleanup;
ref_transaction_add_update(
transaction, r->name,
-   REF_NO_DEREF | REF_HAVE_NEW | REF_HAVE_OLD | 
REF_ISPRUNING,
+   REF_NO_DEREF | REF_HAVE_NEW | REF_HAVE_OLD | 
REF_IS_PRUNING,
_oid, >oid, NULL);
if (ref_transaction_commit(transaction, ))
goto cleanup;
@@ -2177,7 +2177,7 @@ static int split_head_update(struct ref_update *update,
struct ref_update *new_update;
 
if ((update->flags & REF_LOG_ONLY) ||
-   (update->flags & REF_ISPRUNING) ||
+   (update->flags & REF_IS_PRUNING) ||
(update->flags & REF_UPDATE_VIA_HEAD))
return 0;
 
@@ -2564,16 +2564,16 @@ static int files_transaction_prepare(struct ref_store 
*ref_store,
 * split_symref_update() or split_head_update(), those
 * functions will check that the new updates don't have the
 * same refname as any existing ones.) Also fail if any of the
-* updates use REF_ISPRUNING without REF_NO_DEREF.
+* updates use REF_IS_PRUNING without REF_NO_DEREF.
 */
for (i = 0; i < transaction->nr; i++) {
struct ref_update *update = transaction->updates[i];
struct string_list_item *item =
string_list_append(_refnames, update->refname);
 
-   if ((update->flags & REF_ISPRUNING) &&
+   if ((update->flags & REF_IS_PRUNING) &&
!(update->flags & REF_NO_DEREF))
-   BUG("REF_ISPRUNING set without REF_NO_DEREF");
+   BUG("REF_IS_PRUNING set without REF_NO_DEREF");
 
/*
 * We store a pointer to update in item->util, but at
@@ -2632,7 +2632,7 @@ static int files_transaction_prepare(struct ref_store 
*ref_store,
 
if (update->flags & REF_DELETING &&
!(update->flags & REF_LOG_ONLY) &&
-   !(update->flags & REF_ISPRUNING)) {
+   !(update->flags & REF_IS_PRUNING)) {
/*
 * This reference has to be deleted from
 * packed-refs if it exists there.
@@ -2749,7 +2749,7 @@ static int files_transaction_finish(struct ref_store 
*ref_store,
struct ref_update *update = transaction->updates[i];
if (update->flags & REF_DELETING &&
!(update->flags & REF_LOG_ONLY) &&
-   !(update->flags & REF_ISPRUNING)) {
+   !(update->flags & REF_IS_PRUNING)) {
strbuf_reset();
files_reflog_path(refs, , update->refname);
if (!unlink_or_warn(sb.buf))
-- 
2.14.1

[PATCH v2 8/9] write_packed_entry(): take `object_id` arguments

[Michael Haggerty]

Change `write_packed_entry()` to take `struct object_id *` rather than
`unsigned char *` arguments.

Signed-off-by: Michael Haggerty 
---
 refs/packed-backend.c | 16 
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index 74f1dea0f4..43ad74fc5a 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@ -961,11 +961,11 @@ static struct ref_iterator *packed_ref_iterator_begin(
  * by the failing call to `fprintf()`.
  */
 static int write_packed_entry(FILE *fh, const char *refname,
- const unsigned char *sha1,
- const unsigned char *peeled)
+ const struct object_id *oid,
+ const struct object_id *peeled)
 {
-   if (fprintf(fh, "%s %s\n", sha1_to_hex(sha1), refname) < 0 ||
-   (peeled && fprintf(fh, "^%s\n", sha1_to_hex(peeled)) < 0))
+   if (fprintf(fh, "%s %s\n", oid_to_hex(oid), refname) < 0 ||
+   (peeled && fprintf(fh, "^%s\n", oid_to_hex(peeled)) < 0))
return -1;
 
return 0;
@@ -1203,8 +1203,8 @@ static int write_with_updates(struct packed_ref_store 
*refs,
int peel_error = ref_iterator_peel(iter, );
 
if (write_packed_entry(out, iter->refname,
-  iter->oid->hash,
-  peel_error ? NULL : peeled.hash))
+  iter->oid,
+  peel_error ? NULL : ))
goto write_error;
 
if ((ok = ref_iterator_advance(iter)) != ITER_OK)
@@ -1224,8 +1224,8 @@ static int write_with_updates(struct packed_ref_store 
*refs,
 );
 
if (write_packed_entry(out, update->refname,
-  update->new_oid.hash,
-  peel_error ? NULL : peeled.hash))
+  >new_oid,
+  peel_error ? NULL : ))
goto write_error;
 
i++;
-- 
2.14.1

Re: git grep -P fatal: pcre_exec failed with error code -8

[Дилян Палаузов]

Hello,

thanks for your answer.

I understand that the PCRE's stack can get exhausted for some files, but 
in such cases, git grep shall proceed with the other files, and print at 
the end/stderr for which files the pattern was not applied.  Such 
behaviour would be more usefull than the current one.


Regards
  Dilian

On 11/05/2017 03:16 AM, Jeff King wrote:

On Sun, Nov 05, 2017 at 01:06:21AM +0100, Дилян Палаузов wrote:


with git 2.14.3 linked with libpcre.so.1.2.9 when I do:
   git clone https://github.com/django/django
   cd django
   git grep -P "if.*([^\s])+\s+and\s+\1"
django/contrib/admin/static/admin/js/vendor/select2/select2.full.min.js
the output is:
   fatal: pcre_exec failed with error code -8


Code -8 is PCRE_ERROR_MATCHLIMIT. And "man pcreapi" has this to say:

   The match_limit field provides a means of preventing PCRE from
   using up a vast amount of resources when running patterns that
   are not going to match, but which have a very large number of
   possibilities in their search trees. The classic example is a
   pattern that uses nested unlimited repeats.

   Internally, pcre_exec() uses a function called match(), which
   it calls repeatedly (sometimes recursively). The limit set by
   match_limit is imposed on the number of times this function is
   called during a match, which has the effect of limiting the
   amount of backtracking that can take place. For patterns that
   are not anchored, the count restarts from zero for each posi‐
   tion in the subject string.

   When pcre_exec() is called with a pattern that was successfully
   studied with a JIT option, the way that the matching is exe‐
   cuted is entirely different. However, there is still the pos‐
   sibility of runaway matching that goes on for a very long time,
   and so the match_limit value is also used in this case (but in
   a different way) to limit how long the matching can continue.

   The default value for the limit can be set when PCRE is built;
   the default default is 10 million, which handles all but the
   most extreme cases. You can override the default by suppling
   pcre_exec() with a pcre_extra block in which match_limit is
   set, and PCRE_EXTRA_MATCH_LIMIT is set in the flags field. If
   the limit is exceeded, pcre_exec() returns PCRE_ERROR_MATCH‐
   LIMIT.

So your pattern is just really expensive and is running afoul of pcre's
backtracking limits (and it's not helped by the fact that the file is
basically one giant line).

There's no way to ask Git to specify a larger match_limit to pcre, but
you might be able to construct your pattern in a way that involves less
backtracking. It looks like you're trying to find things like "if foo
and foo"?

Should the captured term actually be "([^\s]+)" (with the "+" on the
_inside_ of the capture? Or maybe I'm just misunderstanding your goal.

-Peff

Re: what is the function of .git/branches/?

[Jeff King]

On Sun, Nov 05, 2017 at 11:24:00AM +0200, Robert P. J. Day wrote:

>   currently proofing "pro git" book, and an example of a new repo
> doesn't show a .git/branches/ directory, but initializing a new repo
> with current version of 2.13.6 *does* show an initially empty
> directory by that name. however, AFAICT, branches are still tracked
> under .git/refs/heads/, so what's with that branches/ directory?

It's a historical way of storing what's now in the "[remote]" section of
config files. It's covered briefly in "git help repository-layout"
(which is a good starting point for similar questions), and in more
detail in the REMOTES section of fetch/pull/push.

-Peff

what is the function of .git/branches/?

[Robert P. J. Day]

  currently proofing "pro git" book, and an example of a new repo
doesn't show a .git/branches/ directory, but initializing a new repo
with current version of 2.13.6 *does* show an initially empty
directory by that name. however, AFAICT, branches are still tracked
under .git/refs/heads/, so what's with that branches/ directory?

rday

-- 


Robert P. J. Day Ottawa, Ontario, CANADA
http://crashcourse.ca

Twitter:   http://twitter.com/rpjday
LinkedIn:   http://ca.linkedin.com/in/rpjday

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

[Orgad Shaneh]

On Sat, Nov 4, 2017 at 8:04 PM, Orgad Shaneh  wrote:
> On Fri, Nov 3, 2017 at 6:20 PM, Johannes Schindelin
>  wrote:
>> Hi Orgad,
>>
>> On Fri, 3 Nov 2017, Johannes Schindelin wrote:
>>
>>> On Thu, 2 Nov 2017, Orgad Shaneh wrote:
>>>
>>> > I can't reproduce this with a minimal example, but it happens in my 
>>> > project.
>>
>> Whoa, I somehow overlooked the "can't". Sorry.
>>
>> I inserted a `git diff-files` here, and it printed exactly what I
>> expected:
>>
>> ++ git diff-files
>> :16 16 62cab94c8d8cf047bbb60c12def559339300efa4 
>>  M  sub
>>
>>> + git rebase -i HEAD^^
>>> + )
>>> +'
>>
>> There must be something else going wrong that we did not replicate here.
>> Maybe the `error: cannot rebase: You have unstaged changes.` message was
>> caused not by a change in the submodule? Could you run `git diff-files`
>> before the rebase?
>
> It's the same before and during the rebase:
> $ git diff-files
> :16 16 c840225a7cf6bb2ec64da9d35d2c29210bc5e5e8
>  M  sub
>
>
>>
>> This does *not* refresh the index, but maybe that is what is going wrong;
>> you could call `git update-index --refresh` before the rebase and see
>> whether that works around the issue?
>
> Nope.
>
> If I run git submodule update, then rebase --continue works fine, so
> it's definitely somehow caused by the submodule.

I just checked out v2.15.0.windows.1 and reverted ff6f1f564c - it
solves the problem. I still have no idea how to minimally reproduce
(in my project it's easily reproducible) :)

- Orgad

Re: [PATCH 5/7] refs: tidy up and adjust visibility of the `ref_update` flags

[Michael Haggerty]

On 11/01/2017 09:18 AM, Martin Ågren wrote:
> On 28 October 2017 at 11:49, Michael Haggerty  wrote:
>> The constants used for `ref_update::flags` were rather disorganized:
> 
>> * The documentation wasn't very consistent and partly still referred
>>   to sha1s rather than oids.
> 
>> @@ -478,22 +462,23 @@ struct ref_transaction *ref_transaction_begin(struct 
>> strbuf *err);
>>   *
>>   * refname -- the name of the reference to be affected.
>>   *
>> - * new_sha1 -- the SHA-1 that should be set to be the new value of
>> + * new_oid -- the SHA-1 that should be set to be the new value of
>>   * the reference. Some functions allow this parameter to be
>>   * NULL, meaning that the reference is not changed, or
>> - * null_sha1, meaning that the reference should be deleted. A
>> + * null_oid, meaning that the reference should be deleted. A
>>   * copy of this value is made in the transaction.
>>   *
>> - * old_sha1 -- the SHA-1 value that the reference must have before
>> + * old_oid -- the SHA-1 value that the reference must have before
> 
> You still refer to "SHA-1" twice in this hunk. Maybe squash this in, at
> least partially? This addresses all remaining "sha"/"SHA" in refs.h.
> [...]

Thanks for this.

I'll squash the changes that have to do with these flags into this
commit, and change the other docstrings as part of a separate commit
that also fixes up similar problems in other refs-related comments.

I also realized that `write_packed_entry()` still takes `unsigned char
*` arguments. I'll fix that, too, in yet another commit.

Thanks,
Michael

Re: [PATCH 1/7] files_transaction_prepare(): don't leak flags to packed transaction

[Michael Haggerty]

On 10/30/2017 05:44 AM, Junio C Hamano wrote:
> Michael Haggerty  writes:
> 
>> The files backend uses `ref_update::flags` for several internal flags.
>> But those flags have no meaning to the packed backend. So when adding
>> updates for the packed-refs transaction, only use flags that make
>> sense to the packed backend.
>>
>> `REF_NODEREF` is part of the public interface, and it's logically what
>> we want, so include it. In fact it is actually ignored by the packed
>> backend (which doesn't support symbolic references), but that's its
>> own business.
>>
>> Signed-off-by: Michael Haggerty 
>> ---
>>  refs/files-backend.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/refs/files-backend.c b/refs/files-backend.c
>> index 2bd54e11ae..fadf1036d3 100644
>> --- a/refs/files-backend.c
>> +++ b/refs/files-backend.c
>> @@ -2594,8 +2594,8 @@ static int files_transaction_prepare(struct ref_store 
>> *ref_store,
>>  
>>  ref_transaction_add_update(
>>  packed_transaction, update->refname,
>> -update->flags & ~REF_HAVE_OLD,
>> ->new_oid, >old_oid,
>> +REF_HAVE_NEW | REF_NODEREF,
>> +>new_oid, NULL,
> 
> Hmph, so we earlier passed all flags except HAVE_OLD down, which
> meant that update->flags that this transaction for packed backend
> does not have to see are given to it nevertheless.  The new way the
> parameter is prepared does nto depend on update->flags at all, so
> that is about "don't leak flags".
> 
> That much I can understand.  But it is not explained why (1) we do
> not pass old_oid anymore and (2) we do give HAVE_NEW.  
> 
> Presumably the justification for (1) is something like "because we
> are not passing HAVE_OLD, we shouldn't have been passing old_oid at
> all---it was a harmless bug because lack of HAVE_OLD made the callee
> ignore old_oid"

It's debatable whether the old code should even be called a bug. The
callee is documented to ignore `old_oid` if `HAVE_OLD` is not set. But
it was certainly misleading to pass unneeded information to the function.

> (2) is "we need to pass HAVE_NEW, and we have
> been always passing HAVE_NEW because update->flags at this point is
> guaranteed to have it" or something like that?

Correct. `REF_DELETING` is set by `lock_ref_for_update()` only if
`update->flags & REF_HAVE_NEW` was set, and this code is only called if
`REF_DELETING` is set.

Michael

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

[Junio C Hamano]

"Philip Oakley"  writes:

> Is this not also an alternative case, relative to the user, for the
> scenario where the user has an oid/sha1 value but does not know what
> it is, and would like to find its source and type relative to the
> `describe` command.

I am not sure what you wanted to say with "source and type RELATIVE TO
the describe command".

The first thing the combination of the user and the describe command
would do when the user has a 40-hex string would be to do the
equivalent of "cat-file -t" to learn if it even exists and what its
type is.  With Stefan's patch, that is what describe command does in
order to choose quite a different codeflow from the traditional mode
when it learns that it was given a blob.

> IIUC the existing `describe` command only accepts  values,
> and here we are extending that to be even more inclusive, but at the
> same time the options become more restricted.

Do you mean that the command should check if it was given an option
that would not be applicable to the "find a commit that has the
blob" mode, once it learns that it was given a blob and needs to go
in that codepath?  I think that would make sense.

> Or have I misunderstood how the fast commit search and the slower
> potentially-a-blob searching are disambiguated?

I do not think so.  We used to barf when we got anything but
commit-ish, but Stefan's new code kicks in if the object turns out
to be a blob---I think that is what you mean by the disambiguation.

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

[Junio C Hamano]

Junio C Hamano  writes:
> Rafael Ascensão  writes:
> ...
>> Because changing the default behavior of that function has
>> implications on multiple commands which I think shouldn't change. But
>> at the same time, would be nice to have the logic that deals with
>> glob-ref patterns all in one place.
>>
>> What's the sane way to do this?
>
> Learn to type "--decorate-refs="refs/heads/[m]aster", and not twewak
> the code at all, perhaps.  The users of existing "with no globbing,
> /* is appended" interface are already used to that way and they do
> not have to learn a new and inconsistent interface.
>
> After all, "I only want to see 'git log' output with 'master'
> decorated" (i.e. not specifying "this class of refs I can glob by
> using the naming convention I am using" and instead enumerating the
> ones you care about) does not sound like a sensible thing people
> often want to do, so making it follow the other codepath so that
> people can say "refs/tags" to get "refs/tags/*", while still allowing
> such a rare but specific and exact one possible, may not sound too
> bad to me.

Having said all that, I can imagine another way out might be to
change the behaviour of this "normalize" thing to add two patterns,
the original pattern in addition to the original pattern plus "/*",
when it sees a pattern without any glob.  Many users who relied on
the current behaviour fed "refs/tags" knowing that it will match
everything under "refs/tags" i.e. "refs/tags/*", and they cannot
have a ref that is exactly "refs/tags", so adding the original
pattern without an extra trailing "/*" would not hurt them.  And
this will allow you to say "refs/heads/master" when you know you
want that exact ref, and in such a repository where that original
pattern without trailing "/*" would be useful, because you cannot
have "refs/heads/master/one" at the same time, having an extra
pattern that is the original plus "/*" would not hurt you, either.

This however needs a bit of thought to see if there are corner cases
that may result in unexpected and unwanted fallout, and something I
am reluctant to declare unilaterally that it is a better way to go.

Thoughts?