Re: [PATCH] upload-pack: reject shallow requests that would return nothing

[Junio C Hamano]

Nguyễn Thái Ngọc Duy   writes:

> To avoid this, if rev-list returns nothing, we abort the clone/fetch.
> The user could adjust their request (e.g. --shallow-since further back
> in the past) and retry.

Yeah, that makes sense.

> Another possible option for this case is to fall back to a default
> depth (like depth 1). But I don't like too much magic that way because
> we may return something unexpected to the user.

I agree that it would be a horrible fallback.  I actually am
wondering if we should just silently return no objects without even
telling the user there is something unexpected happening.  After
all, the user may well be expecting with --shallow-since that is too
recent that the fetch may not result in pulling anything new, and
giving a "die" message, which now needs to be distinguished from
other forms of die's like network connectivity or auth failures, is
not all that helpful.

> Note that we need to die() in get_shallow_commits_by_rev_list()
> instead of just checking for empty result from its caller
> deepen_by_rev_list() and handling the error there. The reason is,
> empty result could be a valid case: if you have commits in year 2013
> and you request --shallow-since=year.2000 then you should get a full
> clone (i.e. empty result).

Yup, that latter example makes me more convinced that it also is a
valid outcome if we end up requesting "no" object when shallow-since
names too recent date, e.g. against a project that is dormant since
2013 with --shallow-since=2018/01/01 or something like that, instead
of dying.

> Reported-by: Andreas Krey 
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
>  shallow.c |  3 +++
>  t/t5500-fetch-pack.sh | 11 +++
>  2 files changed, 14 insertions(+)
>
> diff --git a/shallow.c b/shallow.c
> index df4d44ea7a..44fdca1ace 100644
> --- a/shallow.c
> +++ b/shallow.c
> @@ -175,6 +175,9 @@ struct commit_list *get_shallow_commits_by_rev_list(int 
> ac, const char **av,
>   die("revision walk setup failed");
>   traverse_commit_list(, show_commit, NULL, _shallow_list);
>  
> + if (!not_shallow_list)
> + die("no commits selected for shallow requests");
> +
>   /* Mark all reachable commits as NOT_SHALLOW */
>   for (p = not_shallow_list; p; p = p->next)
>   p->item->object.flags |= not_shallow_flag;
> diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
> index 0680dec808..c8f2d38a58 100755
> --- a/t/t5500-fetch-pack.sh
> +++ b/t/t5500-fetch-pack.sh
> @@ -711,6 +711,17 @@ test_expect_success 'fetch shallow since ...' '
>   test_cmp expected actual
>  '
>  
> +test_expect_success 'clone shallow since selects no commits' '
> + test_create_repo shallow-since-the-future &&
> + (
> + cd shallow-since-the-future &&
> + GIT_COMMITTER_DATE="1 +0700" git commit --allow-empty -m one &&
> + GIT_COMMITTER_DATE="2 +0700" git commit --allow-empty -m two &&
> + GIT_COMMITTER_DATE="3 +0700" git commit --allow-empty -m three 
> &&
> + test_must_fail git clone --shallow-since "9 +0700" 
> "file://$(pwd)/." ../shallow111
> + )
> +'
> +
>  test_expect_success 'shallow clone exclude tag two' '
>   test_create_repo shallow-exclude &&
>   (

Re: js/empty-config-section-fix, was Re: What's cooking in git.git (May 2018, #03; Wed, 23)

[Junio C Hamano]

Johannes Schindelin  writes:

> Hi Junio,
>
> On Thu, 24 May 2018, Junio C Hamano wrote:
>
>> * js/empty-config-section-fix (2018-05-18) 1 commit
>>  - config: a user-provided invalid section is not a BUG
>> 
>>  Error codepath fix.
>> 
>>  Will merge to 'next'.
>
> As a hotfix, maybe we can fast-track it to master?

Hotfix is a proposed fix for an issue that is so important to be a
showstopper.  This one must be fixed before the final release, but I
do not think it's not that urgent to force us to drop everything
else and merge it to master immediately.

Re: What's cooking in git.git (May 2018, #03; Wed, 23)

[Junio C Hamano]

Elijah Newren  writes:

> On Wed, May 23, 2018 at 5:02 PM, Junio C Hamano  wrote:
>> Here are the topics that have been cooking.  Commits prefixed with
>> '-' are only in 'pu' (proposed updates) while commits prefixed with
>> '+' are in 'next'.  The ones marked with '.' do not appear in any of
>> the integration branches, but I am still holding onto them.
>
> I don't see the series posted at
> https://public-inbox.org/git/20180522004327.13085-1-new...@gmail.com/
> (various merge-recursive code cleanups) -- or its predecessor --
> showing up anywhere.  Did it slip through the cracks, by chance?

It does not require any chance for various serieses, especially
their earlier iterations, do not get enough time slots to be
processed soon after they are posted, as there are too many topics,
both patches and discussions, active on the list.

Especially the ones that are designed not to apply to the current
'master', which would give me a strong incentive to ignore them
until the prerequisite topics in flight hits 'master' ;-)

Re: [GSoC] GSoC with git, week 4

[Christian Couder]

Hi Alban,

On Sat, May 26, 2018 at 6:32 PM, Alban Gruin  wrote:
>
> I published my blog post about this week. You can read it here:
>
> https://blog.pa1ch.fr/posts/2018/05/26/en/gsoc2018-week-4.html
>
> All comments are welcome!

Thanks for publishing a nice update!

Christian.

Re: [PATCH 2/1] SubmittingPatches: not git-secur...@googlegroups.com

[Junio C Hamano]

Thomas Gummerer  writes:

> Yeah sorry, that's what I meant.
> https://public-inbox.org/git/20180308150820.22588-1-ava...@gmail.com/
> is the reference I meant to put there.
>
> How about something like the below?  This is tested with asciidoc
> 8.6.10 and asciidoctor 1.5.6.2.  I'm also happy to squash the two
> patches into one if that's preferred.
>

If the discussion in the proposed log message needs to be updated
anyway, it is a good opportunity to make them into a single patch,
as they share exactly the same objective.

This is a tangent, but the use of footnote below looks a but
curious.  How would {1} reference pick which :1: to use?  The
closest preceding one?  

As this appears on a page that already has other footnotes attached
to an adjacent paragraph, I am wondering if they should be made into
a part of the same numbering sequence.

> @@ -264,6 +264,11 @@ people who are involved in the area you are touching 
> (the `git
>  contacts` command in `contrib/contacts/` can help to
>  identify them), to solicit comments and reviews.
>  
> +:1: footnote:[The Git Security mailing list: git-secur...@googlegroups.com]
> +
> +Patches which are security relevant should be submitted privately to
> +the Git Security mailing list{1}.
> +
>  :1: footnote:[The current maintainer: gits...@pobox.com]
>  :2: footnote:[The mailing list: git@vger.kernel.org]

Also, the placement of this new paragraph is rather odd.  

I am guessing that the reason why you put it _before_ the normal
list address is to make sure those with secrets that must be guarded
won't send it to the list first without thinking, but then this
place is too late for that, as the previous paragraph already told
the reader that the patch should be sent to the list and others but
not necessarily to the maintainer.  This should go one paragraph
before that, at least.  I briefly considered suggesting to move it
even earlier, e.g. the beginning of "Sending your patches" section,
but then by the time readers with potential security patches may
have forgotten it, or worse, get confused by us, when we say "Send
your patches with To: set to the list".  So I dunno.  The most
conservative would be to write it at the beginning of the section
and then repeat it just before "Send to the list, Cc releavant
people" paragraph as a reminder.

Re: git rebase -i --exec and changing directory

[Junio C Hamano]

Phillip Wood  writes:

> I tried your recipe and got the same result as you. However I think it
> could be a problem with 'git status' rather than 'git rebase
> --exec'. If I run your recipe in /tmp/a and do
>
> cd dir
> GIT_DIR=/tmp/a/.git git status
>
> I get the same result as when running 'git status' from 'git rebase
> --exec' So I think the problem might have something to do with GIT_DIR
> being set in the environment when 'git status' is run

Yup.  

When GIT_DIR environment variable appears without GIT_WORKING_TREE,
"git" assumes that you are starting it at the root level of the
working tree.  In your example, if there is README at the top-level
but dir/README is not there, "status" would report that you removed
it.  You can unset GIT_DIR in your --exec script if you know that
Git would find the right repository when run from your script.

Re: [PATCH] packfile: Correct zlib buffer handling

[Junio C Hamano]

Junio C Hamano  writes:

> Duy Nguyen  writes:
>
>> On Sun, May 27, 2018 at 1:57 AM, Junio C Hamano  wrote:
>>> Duy Nguyen  writes:
>>>
 On Sat, May 26, 2018 at 12:56 AM, Jeremy Linton  
 wrote:
> @@ -1416,7 +1416,7 @@ static void *unpack_compressed_entry(struct 
> packed_git *p,
> return NULL;
> memset(, 0, sizeof(stream));
> stream.next_out = buffer;
> -   stream.avail_out = size + 1;
> +   stream.avail_out = size;

 You may want to include in your commit message a reference to
 39eea7bdd9 (Fix incorrect error check while reading deflated pack data
 - 2009-10-21) which adds this plus one with a fascinating story
 behind.
>>>
>>> A bit puzzled---are you saying that this recent patch breaks the old
>>> fix and must be done in some other way?
>>
>> No. I actually wanted to answer that question when I tried to track
>> down the commit that adds " + 1" but I did not spend enough time to
>> understand the old problem. I guess your puzzle means you didn't think
>> it would break anything, which is good.
>
> No it merely means I am puzzled how the posted patch that goes
> directly opposite to what an earlier "fix" did is a correct solution
> to anything X-<.

Specifically, I was worried about this assertion:

Lets rely on the fact that the source buffer will only be fully
consumed when the when the destination buffer is inflated to the
correct size.

which I think is the exact bad thinking that caused troubles for us
in the past; isn't the explanation in 456cdf6e ("Fix loose object
uncompression check.", 2007-03-19) relevant here?

-   stream.avail_out = size + 1;
+   stream.avail_out = size;
...
stream.next_in = in;
st = git_inflate(, Z_FINISH);
if (!stream.avail_out)
-   break; /* the payload is larger than it should be */
+   break; /* done, st indicates if source fully consumed */
curpos += stream.next_in - in;
} while (st == Z_OK || st == Z_BUF_ERROR);
git_inflate_end();
if ((st != Z_STREAM_END) || stream.total_out != size) {
free(buffer);
return NULL;
}

With minimum stream.avail_out without slack, when !avail_out, i.e.
when we fully filled the output buffer, it could be that we had
correct input that deflates to the correct size, in which case we
are happy---st would say Z_STREAM_END, we would leave the loop
because it is neither OK nor BUF_ERROR, and total_out would report
the size we expected.  Or the input zlib stream may have ended with
bytes that express "this concludes the stream", and the input bytes
before that was sufficient to construct the original payload fully,
and we may have just fed the bytes before that "this concludes the
stream" to git_inflate().

In such a case, we haven't consumed all the avail_in.  We may
already have all the correct output, i.e. !avail_out, but because we
haven't consumed the "this concludes the stream", st is not
STREAM_END in such a case.  

Our existing while() loop, with one-byte slack in avail_out, would
have let us continue and the next iteration of the loop would have
consumed the input without producing any more output (i.e. avail_out
would have been left to 1 in both of these final two rounds) and we
would have exited the loop.  After calling inflate_end(), we would
have noticed STREAM_END and correct size and we would have been
happy.

The updated code would handle this latter case rather badly, no?  We
leave the loop early, notice st is not STREAM_END, and be very
unhappy, because this patch did not give us to consume the very end
of the input stream and left the loop early.

>> This yields two problems, first a single byte overrun won't be detected
>> properly because the Z_STREAM_END will then be set, but the null
>> terminator will have been overwritten.

Because we compare total_out and size at the end, we would detect it
as an error in this function, no?  Then zlib overwriting NUL would
not be a problem, as we would free the buffer and return NULL, no?

>> The other problem is that
>> more recent zlib patches have been poisoning the unconsumed portions
>> of the buffers which also overwrites the null, while correctly
>> returning length and status.

Isn't that a bug in zlib, though?  Or do they do that deliberately?

I think a workaround with lower impact would be to manually restore
NUL at the end of the buffer.

Re: [PATCH v3 06/20] commit-graph: add 'verify' subcommand

[Jakub Narebski]

Derrick Stolee  writes:

> If the commit-graph file becomes corrupt, we need a way to verify
> that its contents match the object database. In the manner of
> 'git fsck' we will implement a 'git commit-graph verify' subcommand
> to report all issues with the file.
>
> Add the 'verify' subcommand to the 'commit-graph' builtin and its
> documentation. The subcommand is currently a no-op except for
> loading the commit-graph into memory, which may trigger run-time
> errors that would be caught by normal use.

So this commit is simply getting the boilerplate out of the way for
implementing 'git commit-graph verify' subcommand.  Good.

>Add a simple test that
> ensures the command returns a zero error code.

Nice.

>
> If no commit-graph file exists, this is an acceptable state. Do
> not report any errors.

All right.  I assume that as it is explicit verification call, it does
ignore core.commitGraph setting, isn't it?

>
> Signed-off-by: Derrick Stolee 
> ---
>  Documentation/git-commit-graph.txt |  6 ++
>  builtin/commit-graph.c | 38 
> ++
>  commit-graph.c | 26 ++
>  commit-graph.h |  2 ++
>  t/t5318-commit-graph.sh| 10 ++
>  5 files changed, 82 insertions(+)
>
> diff --git a/Documentation/git-commit-graph.txt 
> b/Documentation/git-commit-graph.txt
> index 4c97b555cc..a222cfab08 100644
> --- a/Documentation/git-commit-graph.txt
> +++ b/Documentation/git-commit-graph.txt
> @@ -10,6 +10,7 @@ SYNOPSIS
>  
>  [verse]
>  'git commit-graph read' [--object-dir ]
> +'git commit-graph verify' [--object-dir ]
>  'git commit-graph write'  [--object-dir ]

In alphabetical order, good.

>  
>  
> @@ -52,6 +53,11 @@ existing commit-graph file.
>  Read a graph file given by the commit-graph file and output basic
>  details about the graph file. Used for debugging purposes.
>  
> +'verify'::
> +
> +Read the commit-graph file and verify its contents against the object
> +database. Used to check for corrupted data.
> +

All right, good enough description.

>  
>  EXAMPLES
>  
> diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
> index f0875b8bf3..0433dd6e20 100644
> --- a/builtin/commit-graph.c
> +++ b/builtin/commit-graph.c
> @@ -8,10 +8,16 @@
>  static char const * const builtin_commit_graph_usage[] = {
>   N_("git commit-graph [--object-dir ]"),
>   N_("git commit-graph read [--object-dir ]"),
> + N_("git commit-graph verify [--object-dir ]"),
>   N_("git commit-graph write [--object-dir ] [--append] 
> [--stdin-packs|--stdin-commits]"),
>   NULL
>  };

In alphabetical order, same as in the manpage for git-commit-graph.

>  
> +static const char * const builtin_commit_graph_verify_usage[] = {
> + N_("git commit-graph verify [--object-dir ]"),
> + NULL
> +};
> +
>  static const char * const builtin_commit_graph_read_usage[] = {
>   N_("git commit-graph read [--object-dir ]"),
>   NULL
> @@ -29,6 +35,36 @@ static struct opts_commit_graph {
>   int append;
>  } opts;
>  
> +
> +static int graph_verify(int argc, const char **argv)
> +{
> + struct commit_graph *graph = 0;
> + char *graph_name;
> +
> + static struct option builtin_commit_graph_verify_options[] = {
> + OPT_STRING(0, "object-dir", _dir,
> +N_("dir"),
> +N_("The object directory to store the graph")),
> + OPT_END(),
> + };
> +
> + argc = parse_options(argc, argv, NULL,
> +  builtin_commit_graph_verify_options,
> +  builtin_commit_graph_verify_usage, 0);
> +
> + if (!opts.obj_dir)
> + opts.obj_dir = get_object_directory();

Getting the boilerplate of implementing the command mostly out of the
way.  Good.

> +
> + graph_name = get_commit_graph_filename(opts.obj_dir);
> + graph = load_commit_graph_one(graph_name);

So we are verifying only the commit-graph file belonging directly to
current repository, as I have expected.  This is needed to for warnings
and error messages from the 'verify' action, to be able to tell in which
file there are problems.

This means that it is possible that there would be problems with
commit-graph files that running 'git commit-graph verify' would not
find, because they are in commit-graph file in one of the alternates.

It is very easy, though, to check all commit-graph files that would be
read and its data concatenated when using commit-graph feature
(e.g. 'git commit-graph read', IIRC):

  $ git commit-graph verify
  $ for obj_dir in $(cat .git/objects/info/alternates) do;
git commit-graph --object-dir="$obj_dir";
done

Note: I have not checked the above that it works.

> + FREE_AND_NULL(graph_name);

Freeing the resources, always nice to have.

> +
> + if (!graph)

Proposal

[Miss Zeliha Omer Faruk]

--
Hello

I have been trying to contact you. Did you get my business proposal?

Best Regards,
Miss.Zeliha ömer faruk
Esentepe Mahallesi Büyükdere
Caddesi Kristal Kule Binasi
No:215
Sisli - Istanbul, Turke

[PATCH 2/1] SubmittingPatches: not git-secur...@googlegroups.com

[Thomas Gummerer]

On 05/27, Jonathan Nieder wrote:
> Thomas Gummerer wrote:
> 
> > Add a mention of the security mailing list to the README.
> > 2caa7b8d27 ("git manpage: note git-secur...@googlegroups.com",
> > 2018-03-08) already added it to the man page, but I suspect that for
> > many developers, such as myself, the README would be the first place
> > to go looking for it.
> >
> > Use the same wording as we already have on the git-scm.com website and
> > in the man page.
> >
> > Signed-off-by: Thomas Gummerer 
> > ---
> >  README.md | 3 +++
> >  1 file changed, 3 insertions(+)
> 
> Reviewed-by: Jonathan Nieder 

Thanks!

> > 2caa7b8d27 ("git manpage: note git-secur...@googlegroups.com",
> > 2018-03-08) also mentions SubmittingPatches, but I think people are
> > much more likely to submit a report of a security issue first, rather
> > than sending a patch, for which I think the README is more useful.
> 
> I don't see a mention of SubmittingPatches in "git show 2caa7b8d27"
> output.  git help git tells me:
> 
>   Report bugs to the Git mailing list 
>   where the development and maintenance is primarily done. You
>   do not have to be subscribed to the list to send a message
>   there.
> 
>   Issues which are security relevant should be disclosed
>   privately to the Git Security mailing list
>   .
> 
> Do you mean that the discussion around that change suggested updating
> SubmittingPatches too?  The "Sending your patches" section indeed
> mentions git@vger.kernel.org, so a mention of the security list would
> indeed be welcome there, even though typically the discussion has
> already started there before a patch is written.

Yeah sorry, that's what I meant.
https://public-inbox.org/git/20180308150820.22588-1-ava...@gmail.com/
is the reference I meant to put there.

How about something like the below?  This is tested with asciidoc
8.6.10 and asciidoctor 1.5.6.2.  I'm also happy to squash the two
patches into one if that's preferred.

--->8---

The previous commit added a note about the Git Security mailing list
to the README.  Add it to Documentation/SubmittingPatches as well, so
developers trying to submit a security relevant patch are pointed in
the right direction.

The wording is adjusted slightly compared to the git-scm.com website
and the README, as they are talking about issues, while
SubmittingPatches is talking about patches.

Signed-off-by: Thomas Gummerer 
---
 Documentation/SubmittingPatches | 5 +
 1 file changed, 5 insertions(+)

diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
index 945f8edb46..aeb7948d98 100644
--- a/Documentation/SubmittingPatches
+++ b/Documentation/SubmittingPatches
@@ -264,6 +264,11 @@ people who are involved in the area you are touching (the 
`git
 contacts` command in `contrib/contacts/` can help to
 identify them), to solicit comments and reviews.
 
+:1: footnote:[The Git Security mailing list: git-secur...@googlegroups.com]
+
+Patches which are security relevant should be submitted privately to
+the Git Security mailing list{1}.
+
 :1: footnote:[The current maintainer: gits...@pobox.com]
 :2: footnote:[The mailing list: git@vger.kernel.org]
 
-- 
2.17.0.921.gf22659ad46

Re: [PATCH v3 05/20] commit-graph: load a root tree from specific graph

[Jakub Narebski]

Derrick Stolee  writes:

> When lazy-loading a tree for a commit, it will be important to select
> the tree from a specific struct commit_graph. Create a new method that
> specifies the commit-graph file and use that in
> get_commit_tree_in_graph().

Is this for the same reason why parse_commit_in_graph_one() was created
in ptch 03/20?  Why it would be important?

>
> Signed-off-by: Derrick Stolee 
> ---
>  commit-graph.c | 12 +---
>  1 file changed, 9 insertions(+), 3 deletions(-)

Simple and straightforward refactoring in the same vein as
parse_commit_in_graph_one() one.

>
> diff --git a/commit-graph.c b/commit-graph.c
> index 78ba0edc80..25893ec096 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c
> @@ -358,14 +358,20 @@ static struct tree *load_tree_for_commit(struct 
> commit_graph *g, struct commit *
>   return c->maybe_tree;
>  }
>  
> -struct tree *get_commit_tree_in_graph(const struct commit *c)
> +static struct tree *get_commit_tree_in_graph_one(struct commit_graph *g,
> +  const struct commit *c)
>  {
>   if (c->maybe_tree)
>   return c->maybe_tree;
>   if (c->graph_pos == COMMIT_NOT_FROM_GRAPH)
> - BUG("get_commit_tree_in_graph called from non-commit-graph 
> commit");
> + BUG("get_commit_tree_in_graph_one called from non-commit-graph 
> commit");

Sidenote: I wonder if it would be better or worse to use __func__ magic
costant variable here (part of C99 and C++11 standards).

> +
> + return load_tree_for_commit(g, (struct commit *)c);
> +}
>  
> - return load_tree_for_commit(commit_graph, (struct commit *)c);
> +struct tree *get_commit_tree_in_graph(const struct commit *c)
> +{
> + return get_commit_tree_in_graph_one(commit_graph, c);
>  }
>  
>  static void write_graph_chunk_fanout(struct hashfile *f,

Looks good to me.

[PATCH v2 12/11] completion: complete general config vars in two steps

[Nguyễn Thái Ngọc Duy]

There are 581 config variables as of now when you do "git config
" which can fill up a few screens and is not very helpful when
you have to look through columns of text to find what you want.

This patch instead shows you only first level when you do

git config 

There are 78 items, which use up 8 rows in my screen. Compared to
screens of text, it's pretty good. Once you have chosen you first
level, e.g. color:

git config color.

will show you all color.*

This is not a new idea. branch.* and remote.* completion already does
this for second and third levels. For those variables, you'll need to
 three times to get full variable name.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 I mentioned about this a while back in the --no-... completion RFC.
 It turns out not that hard to do, especially when branch.* and
 remote.* already kinda set a precedence.

 contrib/completion/git-completion.bash | 9 +++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index 2cbd14b346..ab026776df 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2314,9 +2314,14 @@ _git_config ()
__gitcomp "insteadOf pushInsteadOf" "$pfx" "$cur_"
return
;;
+   *.*)
+   __git_compute_config_vars
+   __gitcomp "$__git_config_vars"
+   ;;
+   *)
+   __git_compute_config_vars
+   __gitcomp "$(echo "$__git_config_vars" | sed 's/\.[^ ]*/./g')"
esac
-   __git_compute_config_vars
-   __gitcomp "$__git_config_vars"
 }
 
 _git_remote ()
-- 
2.17.0.705.g3525833791

Re: [PATCH v3 04/20] commit: force commit to parse from object database

[Jakub Narebski]

Derrick Stolee  writes:

> In anticipation of verifying commit-graph file contents against the
> object database, create parse_commit_internal() to allow side-stepping
> the commit-graph file and parse directly from the object database.
>
> Due to the use of generation numbers, this method should not be called
> unless the intention is explicit in avoiding commits from the
> commit-graph file.

A straightforward addition of a parameter to parse_commit() and renaming
it to parse_commit_internal(), while changing parse_commit() to be a
simple wrapper around newly introduced parse_commit_internal(), passing
the default arguments.

>
> Signed-off-by: Derrick Stolee 
> ---
>  commit.c | 9 +++--
>  commit.h | 1 +
>  2 files changed, 8 insertions(+), 2 deletions(-)

Nice and simple refactoring in preparation for future changes.

>
> diff --git a/commit.c b/commit.c
> index 1d28677dfb..6eaed0174c 100644
> --- a/commit.c
> +++ b/commit.c
> @@ -392,7 +392,7 @@ int parse_commit_buffer(struct commit *item, const void 
> *buffer, unsigned long s
>   return 0;
>  }
>  
> -int parse_commit_gently(struct commit *item, int quiet_on_missing)
> +int parse_commit_internal(struct commit *item, int quiet_on_missing, int 
> use_commit_graph)

I guess that the "new" parse_commit_internal() function was not made
static despite the *_internal() in the name because it would need to be
used from commit-graph.c, isn't it?

I don't think we would need more similar options, so *_with_flags()
would be YAGNI overkill.

>  {
>   enum object_type type;
>   void *buffer;
> @@ -403,7 +403,7 @@ int parse_commit_gently(struct commit *item, int 
> quiet_on_missing)
>   return -1;
>   if (item->object.parsed)
>   return 0;
> - if (parse_commit_in_graph(item))
> + if (use_commit_graph && parse_commit_in_graph(item))
>   return 0;
>   buffer = read_sha1_file(item->object.oid.hash, , );
>   if (!buffer)
> @@ -424,6 +424,11 @@ int parse_commit_gently(struct commit *item, int 
> quiet_on_missing)
>   return ret;
>  }
>  
> +int parse_commit_gently(struct commit *item, int quiet_on_missing)
> +{
> + return parse_commit_internal(item, quiet_on_missing, 1);
> +}
> +
>  void parse_commit_or_die(struct commit *item)
>  {
>   if (parse_commit(item))
> diff --git a/commit.h b/commit.h
> index b5afde1ae9..5fde74fcd7 100644
> --- a/commit.h
> +++ b/commit.h
> @@ -73,6 +73,7 @@ struct commit *lookup_commit_reference_by_name(const char 
> *name);
>  struct commit *lookup_commit_or_die(const struct object_id *oid, const char 
> *ref_name);
>  
>  int parse_commit_buffer(struct commit *item, const void *buffer, unsigned 
> long size, int check_graph);
> +int parse_commit_internal(struct commit *item, int quiet_on_missing, int 
> use_commit_graph);
>  int parse_commit_gently(struct commit *item, int quiet_on_missing);
>  static inline int parse_commit(struct commit *item)
>  {

Re: Weird revision walk behaviour

[Kevin Bracey]

On 24/05/2018 23:26, Kevin Bracey wrote:



On Wed, May 23, 2018 at 07:10:58PM +0200, SZEDER Gábor wrote:


   $ git log --oneline master..ba95710a3b -- ci/
   ea44c0a594 Merge branch 'bw/protocol-v2' into 
jt/partial-clone-proto-v2


In this case, we're hitting a merge commit which is not on master, but 
it has two parents which both are. Which, IIRC, means the merge commit 
is INTERESTING with two UNINTERESTING parents; and we are TREESAME to 
only one of them.


The commit changing the logic of TREESAME you identified believes that 
those TREESAME changes for merges which were intended to improve 
fuller history modes shouldn't affect the simple history "because 
partially TREESAME merges are turned into normal commits". Clearly 
that didn't happen here.


Haven't currently got a development environment set up here, but I've 
been looking at the code.Here's a proposal, untested, as a potential 
starting point if anyone wants to consider a proper patch.


The simplify_history first-scan logic never actually turned merges into 
simple commits unless they were TREESAME to a relevant/interesting 
parent.  Anything where the TREESAME parent was UNINTERESTING was 
retained as a merge, but had its TREESAME flag set, and that permitted 
later simplification.


With the redefinition of the TREESAME flag, this merge commit is no 
longer TREESAME, and as the decoration logic to refine TREESAME isn't 
active for simplify_history, it doesn't get cleaned up (even if it would 
be in full history?)


I think the answer may be to add an extra post-process step on the 
initial loop to handle this special case. Something like:


        case REV_TREE_SAME:
            if (!revs->simplify_history || !relevant_commit(p)) {
                /* Even if a merge with an uninteresting
                 * side branch brought the entire change
                 * we are interested in, we do not want
                 * to lose the other branches of this
                 * merge, so we just keep going.
                 */
                if (ts)
                    ts->treesame[nth_parent] = 1;
+   /* But we note it for potential later simplification */
+       if (!treesame_parent)
+    treesame_parent = p;
    continue;
 }

...

After loop:

+ if (relevant_parents == 0 && revs->simplify_history && 
treesame_parent) {
+   treesame_parent->next = NULL;// Repeats code from loop - 
share somehow?

+   commit->parents = treesame_parent;
+   commit->object.flags |= TREESAME;
+   return;
+    }

 /*
  * TREESAME is straightforward for single-parent commits. For merge

The other option would be to take off the " || !relevant_commit(p)" 
test, but I'm assuming that is still needed for other cases.


Kevin

Re: git rebase -i --exec and changing directory

[Philip Oakley]

Hi Ondrej, Phillip,

From: "Phillip Wood" 

Hi Ondrej

On 27/05/18 13:53, Ondrej Mosnáček wrote:


Hi Philip,

2018-05-27 14:28 GMT+02:00 Philip Oakley :
You may need to give a bit more background of things that seem obvious 
to

you.
So where is the src directory you are cd'ing to relative to the
directory/repository you are creating?


It is located in the top-level directory of the working tree (in the
same directory that .git is in).

 From git-rebase(1):

 The "exec" command launches the command in a shell (the one
 specified in $SHELL, or the default shell if $SHELL is not set), so
 you can use shell features (like "cd", ">", ";" ...). The command is
 run from the root of the working tree.

So I need to run 'cd src' if I want to run a command in there
(regardless of the working directory of the git rebase command
itself).


What is [the name of] the directory you are currently in, etc. ?


I don't think that is relevant here. FWIW, when verifying the problem
I ran the reproducer from my original message in a directory whose
path did not contain any spaces or special characters.

Did you try to run the reproducing commands I posted? Did you get a
different result? You should see the following in the output of 'cd
dir && git status':


At the time, I hadn't run the command. I was more interested in 
understanding the problem setup, as understanding often brings 
enlightenment.


I was jsut starting to do my own setup and swaw Phillip had responsed which 
prompted me to think it could be that there was no tty attached to the exec, 
so output wasn't being seen (or something like that).




I tried your recipe and got the same result as you. However I think it 
could be a problem with 'git status' rather than 'git rebase --exec'. If I 
run your recipe in /tmp/a and do


cd dir
GIT_DIR=/tmp/a/.git git status

I get the same result as when running 'git status' from 'git 
rebase --exec' So I think the problem might have something to do with 
GIT_DIR being set in the environment when 'git status' is run




I too got the same same results.
I also tried duplicating the exec line and placing it before the pick line, 
just to check it wasn't an issue about termination. Same result.



Best Wishes

Phillip



[...]
Changes not staged for commit:
  (use "git add/rm ..." to update what will be committed)
  (use "git checkout -- ..." to discard changes in working 
directory)


deleted:a
deleted:b
deleted:dir/x
deleted:reproduce.sh

Untracked files:
  (use "git add ..." to include in what will be committed)

x
[...]

When I drop the 'cd dir && ' from before 'git status', the output is
as expected:

You are currently editing a commit while rebasing branch 'master' on 
'19765db'.

  (use "git commit --amend" to amend the current commit)
  (use "git rebase --continue" once you are satisfied with your changes)

nothing to commit, working tree clean


So I extended the command to be exec'd to `cd dir && ls && git status`, 
again with duplication of the exec, which then gives a bit more..


finally I extended the status to pipe it's output to a file, again 
duplicated.

--
Philip@PhilipOakley MINGW32 /usr/src/mosnacek (master)

$ git rebase -i --exec 'cd dir && ls && git status >stat.txt' base

Executing: cd dir && ls && git status >stat0.txt

x

Executing: cd dir && ls && git status >stat.txt

stat0.txt x

Successfully rebased and updated refs/heads/master.

--
the stat0, stat files can then be investigated.

Summary: status is, I think, being clever and dropping the verbiage when not 
directly attached to the terminal. (or it is being intelligent and adding a 
lot more status details just because it _is_ within the rebase..)






Philip
--

From: "Ondrej Mosnáček" 
Bump? Has anyone had time to look at this?

2018-05-19 18:38 GMT+02:00 Ondrej Mosnáček :


Hello,

I am trying to run a script to edit multiple commits using 'git rebase
-i --exec ...' and I ran into a strange behavior when I run 'cd'
inside the --exec command and subsequently run a git command. For
example, if the command is 'cd src && git status', then git status
reports as if all files in the repository are deleted.


What does that particular report look like? I see no special report of 
deletions, or additions.





Example command sequence to reproduce the problem:

 # Setup:
 touch a
 mkdir dir
 touch dir/x

 git init .
 git add --all
 git commit -m commit1
 git tag base
 touch b
 git add --all
 git commit -m commit2

 # Here we go:
 git rebase -i --exec 'cd dir && git status' base

 # Spawning a sub-shell doesn't help:
 git rebase -i --exec '(cd dir && git status)' base

Is this expected behavior or did I found a bug? Is there any
workaround, other than cd'ing to the toplevel directory every time I
want to run a git command when I 

Re: git rebase -i --exec and changing directory

[Philip Oakley]

Hi Ondrej, Phillip,

From: "Phillip Wood" 

Hi Ondrej

On 27/05/18 13:53, Ondrej Mosnáček wrote:


Hi Philip,

2018-05-27 14:28 GMT+02:00 Philip Oakley :
You may need to give a bit more background of things that seem obvious 
to

you.
So where is the src directory you are cd'ing to relative to the
directory/repository you are creating?


It is located in the top-level directory of the working tree (in the
same directory that .git is in).

 From git-rebase(1):

 The "exec" command launches the command in a shell (the one
 specified in $SHELL, or the default shell if $SHELL is not set), so
 you can use shell features (like "cd", ">", ";" ...). The command is
 run from the root of the working tree.

So I need to run 'cd src' if I want to run a command in there
(regardless of the working directory of the git rebase command
itself).


What is [the name of] the directory you are currently in, etc. ?


I don't think that is relevant here. FWIW, when verifying the problem
I ran the reproducer from my original message in a directory whose
path did not contain any spaces or special characters.

Did you try to run the reproducing commands I posted? Did you get a
different result? You should see the following in the output of 'cd
dir && git status':


At the time, I hadn't run the command. I was more interested in 
understanding the problem setup, as understanding often brings 
enlightenment.


I was jsut starting to do my own setup and swaw Phillip had responsed which 
prompted me to think it could be that there was no tty attached to the exec, 
so output wasn't being seen (or something like that).




I tried your recipe and got the same result as you. However I think it 
could be a problem with 'git status' rather than 'git rebase --exec'. If I 
run your recipe in /tmp/a and do


cd dir
GIT_DIR=/tmp/a/.git git status

I get the same result as when running 'git status' from 'git 
rebase --exec' So I think the problem might have something to do with 
GIT_DIR being set in the environment when 'git status' is run




I too got the same same results.
I also tried duplicating the exec line and placing it before the pick line, 
just to check it wasn't an issue about termination. Same result.



Best Wishes

Phillip



[...]
Changes not staged for commit:
  (use "git add/rm ..." to update what will be committed)
  (use "git checkout -- ..." to discard changes in working 
directory)


deleted:a
deleted:b
deleted:dir/x
deleted:reproduce.sh

Untracked files:
  (use "git add ..." to include in what will be committed)

x
[...]

When I drop the 'cd dir && ' from before 'git status', the output is
as expected:

You are currently editing a commit while rebasing branch 'master' on 
'19765db'.

  (use "git commit --amend" to amend the current commit)
  (use "git rebase --continue" once you are satisfied with your changes)

nothing to commit, working tree clean


So I extended the command to be exec'd to `cd dir && ls && git status`, 
again with duplication of the exec, which then gives a bit more..


finally I extended the status to pipe it's output to a file, again 
duplicated.

--
Philip@PhilipOakley MINGW32 /usr/src/mosnacek (master)

$ git rebase -i --exec 'cd dir && ls && git status >stat.txt' base

Executing: cd dir && ls && git status >stat0.txt

x

Executing: cd dir && ls && git status >stat.txt

stat0.txt x

Successfully rebased and updated refs/heads/master.

--
the stat0, stat files can then be investigated.

Summary: status is, I think, being clever and dropping the verbiage when not 
directly attached to the terminal. (or it is being intelligent and adding a 
lot more status details just because it _is_ within the rebase..)






Philip
--

From: "Ondrej Mosnáček" 
Bump? Has anyone had time to look at this?

2018-05-19 18:38 GMT+02:00 Ondrej Mosnáček :


Hello,

I am trying to run a script to edit multiple commits using 'git rebase
-i --exec ...' and I ran into a strange behavior when I run 'cd'
inside the --exec command and subsequently run a git command. For
example, if the command is 'cd src && git status', then git status
reports as if all files in the repository are deleted.


What does that particular report look like? I see no special report of 
deletions, or additions.





Example command sequence to reproduce the problem:

 # Setup:
 touch a
 mkdir dir
 touch dir/x

 git init .
 git add --all
 git commit -m commit1
 git tag base
 touch b
 git add --all
 git commit -m commit2

 # Here we go:
 git rebase -i --exec 'cd dir && git status' base

 # Spawning a sub-shell doesn't help:
 git rebase -i --exec '(cd dir && git status)' base

Is this expected behavior or did I found a bug? Is there any
workaround, other than cd'ing to the toplevel directory every time I
want to run a git command when I 

Re: git rebase -i --exec and changing directory

[Phillip Wood]

Hi Ondrej

On 27/05/18 13:53, Ondrej Mosnáček wrote:


Hi Philip,

2018-05-27 14:28 GMT+02:00 Philip Oakley :

You may need to give a bit more background of things that seem obvious to
you.
So where is the src directory you are cd'ing to relative to the
directory/repository you are creating?


It is located in the top-level directory of the working tree (in the
same directory that .git is in).

 From git-rebase(1):

 The "exec" command launches the command in a shell (the one
 specified in $SHELL, or the default shell if $SHELL is not set), so
 you can use shell features (like "cd", ">", ";" ...). The command is
 run from the root of the working tree.

So I need to run 'cd src' if I want to run a command in there
(regardless of the working directory of the git rebase command
itself).


What is [the name of] the directory you are currently in, etc. ?


I don't think that is relevant here. FWIW, when verifying the problem
I ran the reproducer from my original message in a directory whose
path did not contain any spaces or special characters.

Did you try to run the reproducing commands I posted? Did you get a
different result? You should see the following in the output of 'cd
dir && git status':


I tried your recipe and got the same result as you. However I think it 
could be a problem with 'git status' rather than 'git rebase --exec'. If 
I run your recipe in /tmp/a and do


cd dir
GIT_DIR=/tmp/a/.git git status

I get the same result as when running 'git status' from 'git rebase 
--exec' So I think the problem might have something to do with GIT_DIR 
being set in the environment when 'git status' is run


Best Wishes

Phillip



[...]
Changes not staged for commit:
  (use "git add/rm ..." to update what will be committed)
  (use "git checkout -- ..." to discard changes in working directory)

deleted:a
deleted:b
deleted:dir/x
deleted:reproduce.sh

Untracked files:
  (use "git add ..." to include in what will be committed)

x
[...]

When I drop the 'cd dir && ' from before 'git status', the output is
as expected:

You are currently editing a commit while rebasing branch 'master' on '19765db'.
  (use "git commit --amend" to amend the current commit)
  (use "git rebase --continue" once you are satisfied with your changes)

nothing to commit, working tree clean



Philip
--

From: "Ondrej Mosnáček" 
Bump? Has anyone had time to look at this?

2018-05-19 18:38 GMT+02:00 Ondrej Mosnáček :


Hello,

I am trying to run a script to edit multiple commits using 'git rebase
-i --exec ...' and I ran into a strange behavior when I run 'cd'
inside the --exec command and subsequently run a git command. For
example, if the command is 'cd src && git status', then git status
reports as if all files in the repository are deleted.

Example command sequence to reproduce the problem:

 # Setup:
 touch a
 mkdir dir
 touch dir/x

 git init .
 git add --all
 git commit -m commit1
 git tag base
 touch b
 git add --all
 git commit -m commit2

 # Here we go:
 git rebase -i --exec 'cd dir && git status' base

 # Spawning a sub-shell doesn't help:
 git rebase -i --exec '(cd dir && git status)' base

Is this expected behavior or did I found a bug? Is there any
workaround, other than cd'ing to the toplevel directory every time I
want to run a git command when I am inside a subdirectory?

$ git --version
git version 2.17.0

Thanks,

Ondrej Mosnacek

Re: [PATCH] README: note git-secur...@googlegroups.com

[Jonathan Nieder]

Thomas Gummerer wrote:

> Add a mention of the security mailing list to the README.
> 2caa7b8d27 ("git manpage: note git-secur...@googlegroups.com",
> 2018-03-08) already added it to the man page, but I suspect that for
> many developers, such as myself, the README would be the first place
> to go looking for it.
>
> Use the same wording as we already have on the git-scm.com website and
> in the man page.
>
> Signed-off-by: Thomas Gummerer 
> ---
>  README.md | 3 +++
>  1 file changed, 3 insertions(+)

Reviewed-by: Jonathan Nieder 

> 2caa7b8d27 ("git manpage: note git-secur...@googlegroups.com",
> 2018-03-08) also mentions SubmittingPatches, but I think people are
> much more likely to submit a report of a security issue first, rather
> than sending a patch, for which I think the README is more useful.

I don't see a mention of SubmittingPatches in "git show 2caa7b8d27"
output.  git help git tells me:

Report bugs to the Git mailing list 
where the development and maintenance is primarily done. You
do not have to be subscribed to the list to send a message
there.

Issues which are security relevant should be disclosed
privately to the Git Security mailing list
.

Do you mean that the discussion around that change suggested updating
SubmittingPatches too?  The "Sending your patches" section indeed
mentions git@vger.kernel.org, so a mention of the security list would
indeed be welcome there, even though typically the discussion has
already started there before a patch is written.

Thanks,
Jonathan

[PATCH] README: note git-secur...@googlegroups.com

[Thomas Gummerer]

Add a mention of the security mailing list to the README.
2caa7b8d27 ("git manpage: note git-secur...@googlegroups.com",
2018-03-08) already added it to the man page, but I suspect that for
many developers, such as myself, the README would be the first place
to go looking for it.

Use the same wording as we already have on the git-scm.com website and
in the man page.

Signed-off-by: Thomas Gummerer 
---

2caa7b8d27 ("git manpage: note git-secur...@googlegroups.com",
2018-03-08) also mentions SubmittingPatches, but I think people are
much more likely to submit a report of a security issue first, rather
than sending a patch, for which I think the README is more useful.

 README.md | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/README.md b/README.md
index f17af66a97..f920a42fad 100644
--- a/README.md
+++ b/README.md
@@ -36,6 +36,9 @@ the body to majord...@vger.kernel.org. The mailing list 
archives are
 available at ,
  and other archival sites.
 
+Issues which are security relevant should be disclosed privately to
+the Git Security mailing list .
+
 The maintainer frequently sends the "What's cooking" reports that
 list the current status of various development topics to the mailing
 list.  The discussion following them give a good reference for
-- 
2.17.0.921.gf22659ad46

Re: git rebase -i --exec and changing directory

[Ondrej Mosnáček]

Hi Philip,

2018-05-27 14:28 GMT+02:00 Philip Oakley :
> You may need to give a bit more background of things that seem obvious to
> you.
> So where is the src directory you are cd'ing to relative to the
> directory/repository you are creating?

It is located in the top-level directory of the working tree (in the
same directory that .git is in).

>From git-rebase(1):

The "exec" command launches the command in a shell (the one
specified in $SHELL, or the default shell if $SHELL is not set), so
you can use shell features (like "cd", ">", ";" ...). The command is
run from the root of the working tree.

So I need to run 'cd src' if I want to run a command in there
(regardless of the working directory of the git rebase command
itself).

> What is [the name of] the directory you are currently in, etc. ?

I don't think that is relevant here. FWIW, when verifying the problem
I ran the reproducer from my original message in a directory whose
path did not contain any spaces or special characters.

Did you try to run the reproducing commands I posted? Did you get a
different result? You should see the following in the output of 'cd
dir && git status':

[...]
Changes not staged for commit:
 (use "git add/rm ..." to update what will be committed)
 (use "git checkout -- ..." to discard changes in working directory)

   deleted:a
   deleted:b
   deleted:dir/x
   deleted:reproduce.sh

Untracked files:
 (use "git add ..." to include in what will be committed)

   x
[...]

When I drop the 'cd dir && ' from before 'git status', the output is
as expected:

You are currently editing a commit while rebasing branch 'master' on '19765db'.
 (use "git commit --amend" to amend the current commit)
 (use "git rebase --continue" once you are satisfied with your changes)

nothing to commit, working tree clean

>
> Philip
> --
>
> From: "Ondrej Mosnáček" 
> Bump? Has anyone had time to look at this?
>
> 2018-05-19 18:38 GMT+02:00 Ondrej Mosnáček :
>>
>> Hello,
>>
>> I am trying to run a script to edit multiple commits using 'git rebase
>> -i --exec ...' and I ran into a strange behavior when I run 'cd'
>> inside the --exec command and subsequently run a git command. For
>> example, if the command is 'cd src && git status', then git status
>> reports as if all files in the repository are deleted.
>>
>> Example command sequence to reproduce the problem:
>>
>> # Setup:
>> touch a
>> mkdir dir
>> touch dir/x
>>
>> git init .
>> git add --all
>> git commit -m commit1
>> git tag base
>> touch b
>> git add --all
>> git commit -m commit2
>>
>> # Here we go:
>> git rebase -i --exec 'cd dir && git status' base
>>
>> # Spawning a sub-shell doesn't help:
>> git rebase -i --exec '(cd dir && git status)' base
>>
>> Is this expected behavior or did I found a bug? Is there any
>> workaround, other than cd'ing to the toplevel directory every time I
>> want to run a git command when I am inside a subdirectory?
>>
>> $ git --version
>> git version 2.17.0
>>
>> Thanks,
>>
>> Ondrej Mosnacek
>
>

Re: git rebase -i --exec and changing directory

[Philip Oakley]

You may need to give a bit more background of things that seem obvious to 
you.
So where is the src directory you are cd'ing to relative to the 
directory/repository you are creating?

What is [the name of] the directory you are currently in, etc. ?

Philip
--

From: "Ondrej Mosnáček" 
Bump? Has anyone had time to look at this?

2018-05-19 18:38 GMT+02:00 Ondrej Mosnáček :

Hello,

I am trying to run a script to edit multiple commits using 'git rebase
-i --exec ...' and I ran into a strange behavior when I run 'cd'
inside the --exec command and subsequently run a git command. For
example, if the command is 'cd src && git status', then git status
reports as if all files in the repository are deleted.

Example command sequence to reproduce the problem:

# Setup:
touch a
mkdir dir
touch dir/x

git init .
git add --all
git commit -m commit1
git tag base
touch b
git add --all
git commit -m commit2

# Here we go:
git rebase -i --exec 'cd dir && git status' base

# Spawning a sub-shell doesn't help:
git rebase -i --exec '(cd dir && git status)' base

Is this expected behavior or did I found a bug? Is there any
workaround, other than cd'ing to the toplevel directory every time I
want to run a git command when I am inside a subdirectory?

$ git --version
git version 2.17.0

Thanks,

Ondrej Mosnacek

Re: [PATCH] packfile: Correct zlib buffer handling

[Junio C Hamano]

Duy Nguyen  writes:

> On Sun, May 27, 2018 at 1:57 AM, Junio C Hamano  wrote:
>> Duy Nguyen  writes:
>>
>>> On Sat, May 26, 2018 at 12:56 AM, Jeremy Linton  
>>> wrote:
 @@ -1416,7 +1416,7 @@ static void *unpack_compressed_entry(struct 
 packed_git *p,
 return NULL;
 memset(, 0, sizeof(stream));
 stream.next_out = buffer;
 -   stream.avail_out = size + 1;
 +   stream.avail_out = size;
>>>
>>> You may want to include in your commit message a reference to
>>> 39eea7bdd9 (Fix incorrect error check while reading deflated pack data
>>> - 2009-10-21) which adds this plus one with a fascinating story
>>> behind.
>>
>> A bit puzzled---are you saying that this recent patch breaks the old
>> fix and must be done in some other way?
>
> No. I actually wanted to answer that question when I tried to track
> down the commit that adds " + 1" but I did not spend enough time to
> understand the old problem. I guess your puzzle means you didn't think
> it would break anything, which is good.

No it merely means I am puzzled how the posted patch that goes
directly opposite to what an earlier "fix" did is a correct solution
to anything X-<.

Re: git rebase -i --exec and changing directory

[Ondrej Mosnáček]

Bump? Has anyone had time to look at this?

Thanks,

Ondrej Mosnacek

2018-05-19 18:38 GMT+02:00 Ondrej Mosnáček :
> Hello,
>
> I am trying to run a script to edit multiple commits using 'git rebase
> -i --exec ...' and I ran into a strange behavior when I run 'cd'
> inside the --exec command and subsequently run a git command. For
> example, if the command is 'cd src && git status', then git status
> reports as if all files in the repository are deleted.
>
> Example command sequence to reproduce the problem:
>
> # Setup:
> touch a
> mkdir dir
> touch dir/x
>
> git init .
> git add --all
> git commit -m commit1
> git tag base
> touch b
> git add --all
> git commit -m commit2
>
> # Here we go:
> git rebase -i --exec 'cd dir && git status' base
>
> # Spawning a sub-shell doesn't help:
> git rebase -i --exec '(cd dir && git status)' base
>
> Is this expected behavior or did I found a bug? Is there any
> workaround, other than cd'ing to the toplevel directory every time I
> want to run a git command when I am inside a subdirectory?
>
> $ git --version
> git version 2.17.0
>
> Thanks,
>
> Ondrej Mosnacek

Re: [PATCH v3 03/20] commit-graph: parse commit from chosen graph

[Jakub Narebski]

Derrick Stolee  writes:

> Before verifying a commit-graph file against the object database, we
> need to parse all commits from the given commit-graph file. Create
> parse_commit_in_graph_one() to target a given struct commit_graph.

If I understand it properly the problem is that when verifying against
the object database we want to check one single commit-graph file, not
concatenation of data from commit-graph file for the repository and
commit-graph files from its alternates -- like prepare_commit_graph()
does; which is called by parse_commit_in_graph().

>
> Signed-off-by: Derrick Stolee 

O.K., so you introduce here a layer of indirection; parse_commit_in_graph()
now just uses parse_commit_in_graph_one(), passing core_commit_graph
(or the_commit_graph) to it, after checking that core_commit_graph is set
(which handles the case when feature is not turned off) and loading
commit-graph file.

Nice and simple 'split function' refactoring, with new function taking
over when there is commit graph file prepared.


So, after the changes:
* parse_commit_in_graph() is responsible for checking whether to use
  commit-graph feature and ensuring that data from commit-graph is
  loaded, where it passes the control to parse_commit_in_graph_one()
* parse_commit_in_graph_one() checks whether commit-graph feature is
  turned on, whether commit we are interested in was already parsed,
  and then uses fill_commit_in_graph() to actually get the data
* fill_commit_in_graph() gets data out of commit-graph file, extracting
  it from commit data chunk (and if needed large edges chunk).

All those functions return 1 if they got data from commit-graph, and 0
if they didn't.


One minor nitpick / complaint / question is about naming of global
variables used here, namely:
* static struct commit_graph *commit_graph
  from commit-graph.c for global storage of commit-graph[s] data
* int core_commit_graph
  from environment.c for storing core.commitGraph config

But I see that at least the latter is common convention in Git source
code; I guess that the former maybe follows convention as used for "the
index" and "the repository" - additionally it is static / file-local.

> ---
>  commit-graph.c | 18 +++---
>  1 file changed, 15 insertions(+), 3 deletions(-)
>
> diff --git a/commit-graph.c b/commit-graph.c
> index 82295f0975..78ba0edc80 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c
> @@ -310,7 +310,7 @@ static int find_commit_in_graph(struct commit *item, 
> struct commit_graph *g, uin
>   }
>  }
>  
> -int parse_commit_in_graph(struct commit *item)
> +static int parse_commit_in_graph_one(struct commit_graph *g, struct commit 
> *item)
>  {
>   uint32_t pos;
>  
> @@ -318,9 +318,21 @@ static int parse_commit_in_graph_one(struct commit_graph 
> *g, struct commit *item)
>   if (!core_commit_graph)
>   return 0;

All right, we check that commit-graph feature is enabled because
parse_commit_in_graph_one() will be used standalone, not by being
invoked from parse_commit_in_graph().

This check is fast.

>   if (item->object.parsed)
>   return 1;

Sidenote: I just wonder why object.parsed and not for example
object.graph_pos is used to checck whether the object was filled if
possible with commit-graph data...

> +
> + if (find_commit_in_graph(item, g, ))
> + return fill_commit_in_graph(item, g, pos);
> +
> + return 0;
> +}
> +
> +int parse_commit_in_graph(struct commit *item)
> +{
> + if (!core_commit_graph)
> + return 0;

All right, this check is here to short-circuit and make it so git does
not even try to lod commit-graph file[s] if the feature is disabled.

> +
>   prepare_commit_graph();
> - if (commit_graph && find_commit_in_graph(item, commit_graph, ))
> - return fill_commit_in_graph(item, commit_graph, pos);
> + if (commit_graph)
> + return parse_commit_in_graph_one(commit_graph, item);
>   return 0;
>  }

Re: why do "git log -h" and "git show -h" print the same thing?

[Thomas Gummerer]

On 05/24, Stefan Beller wrote:
> On Thu, May 24, 2018 at 6:37 AM, Robert P. J. Day  
> wrote:
> >
> >   maybe this is deliberate, but it's confusing that, with git 2.17.0,
> > the output of both "git log -h" and "git show -h" is exactly the same:
> >
> > $ git log -h
> > usage: git log [] [] [[--] ...]
> >or: git show [] ...
> >
> > -q, --quiet   suppress diff output
> > --source  show source
> > --use-mailmap Use mail map file
> > --decorate-refs 
> >   only decorate refs that match 
> > --decorate-refs-exclude 
> >   do not decorate refs that match 
> > --decorate[=...]  decorate options
> > -L  Process line range n,m in file, counting from 1
> > $
> >
> > is that what's *supposed* to happen?
> 
> I would think so, show is just "log -p" with the range clamped
> down to ^...

Hmm, that's not true though if the object is not a commit, from my
understanding?  While 'git show' does share some of its code with 'git
log', what you get as output may be quite different from what you'd
get with 'git log'.  So I feel like we're leaking an implementation
detail to the user here, and I can see how it is confusing especially
for new users.

So I do feel like there's some room for improvement here, by only
showing the help for the command that was actually used.  I would
welcome a patch to that extend, but obviously I'm not the authority
here :)

> It's been in the code like that for a couple years by now,
> e.g. see
> e66dc0cc4b1a6 log.c: fix translation markings, 2015-01-06

[PATCH v2 2/3] completion: suppress some -no- options

[Nguyễn Thái Ngọc Duy]

Most --no- options do have some use, even if rarely to negate some
option that's specified in an alias.

These options --no-ours and --no-theirs however have no clear
semantics. If I specify "--ours --no-theirs", the second will reset
writeout stage and is equivalent of "--no-ours --no-theirs" which is
not that easy to see. Drop them. You can either switch from --ours to
--theirs and back but you can never negate them.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 builtin/checkout.c| 10 ++
 t/t9902-completion.sh |  2 --
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 2b3b768eff..c7670dbbfe 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -1119,10 +1119,12 @@ int cmd_checkout(int argc, const char **argv, const 
char *prefix)
OPT_SET_INT('t', "track",  , N_("set upstream info 
for new branch"),
BRANCH_TRACK_EXPLICIT),
OPT_STRING(0, "orphan", _orphan_branch, 
N_("new-branch"), N_("new unparented branch")),
-   OPT_SET_INT('2', "ours", _stage, N_("checkout our 
version for unmerged files"),
-   2),
-   OPT_SET_INT('3', "theirs", _stage, N_("checkout 
their version for unmerged files"),
-   3),
+   OPT_SET_INT_F('2', "ours", _stage,
+ N_("checkout our version for unmerged files"),
+ 2, PARSE_OPT_NONEG),
+   OPT_SET_INT_F('3', "theirs", _stage,
+ N_("checkout their version for unmerged files"),
+ 3, PARSE_OPT_NONEG),
OPT__FORCE(, N_("force checkout (throw away local 
modifications)"),
   PARSE_OPT_NOCOMPLETE),
OPT_BOOL('m', "merge", , N_("perform a 3-way merge 
with the new branch")),
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 07c3e3b760..7e5e3ad5b1 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -1245,9 +1245,7 @@ test_expect_success 'double dash "git checkout"' '
--orphan=Z
--no-orphan Z
--ours Z
-   --no-ours Z
--theirs Z
-   --no-theirs Z
--merge Z
--no-merge Z
--conflict=Z
-- 
2.17.0.705.g3525833791

[PATCH v2 0/3] completion: complete all possible -no-

[Nguyễn Thái Ngọc Duy]

The RFC was here [1]. We have started recently to rely on
parse-options to help complete options. One of the leftover items is
allowing completing --no- form. This series enables that.

Changes since the RFC version:

- There's no magic numbers (previously we keep 3 --no- options)
- When there are some --no- options hidden, then we show --no-...
  instead of just --no-

[1] https://public-inbox.org/git/20180417181300.23683-1-pclo...@gmail.com/

Nguyễn Thái Ngọc Duy (3):
  parse-options: option to let --git-completion-helper show negative form
  completion: suppress some -no- options
  completion: collapse extra --no-.. options

 builtin/checkout.c | 10 +++--
 contrib/completion/git-completion.bash | 58 +++---
 parse-options.c| 58 --
 t/t9902-completion.sh  |  5 ++-
 4 files changed, 97 insertions(+), 34 deletions(-)

-- 
2.17.0.705.g3525833791

[PATCH v2 1/3] parse-options: option to let --git-completion-helper show negative form

[Nguyễn Thái Ngọc Duy]

When 7fb6aefd2a (Merge branch 'nd/parseopt-completion' - 2018-03-14)
is merged, the completion for negative form is left out because the
series is alread long and it could be done in a follow up series. This
is it.

--git-completion-helper now provides --no-xxx so that git-completion.bash
can drop the extra custom --no-xxx in the script. It adds a lot more
--no-xxx than what's current provided by the git-completion.bash
script. We'll trim that down later.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 contrib/completion/git-completion.bash | 38 ++
 parse-options.c| 22 ---
 t/t9902-completion.sh  | 16 +--
 3 files changed, 46 insertions(+), 30 deletions(-)

diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index 961a0ed76f..952e660f06 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1126,7 +1126,7 @@ _git_am ()
return
;;
--*)
-   __gitcomp_builtin am "--no-utf8" \
+   __gitcomp_builtin am "" \
"$__git_am_inprogress_options"
return
esac
@@ -1226,9 +1226,7 @@ _git_branch ()
__git_complete_refs --cur="${cur##--set-upstream-to=}"
;;
--*)
-   __gitcomp_builtin branch "--no-color --no-abbrev
-   --no-track --no-column
-   "
+   __gitcomp_builtin branch
;;
*)
if [ $only_local_ref = "y" -a $has_r = "n" ]; then
@@ -1269,7 +1267,7 @@ _git_checkout ()
__gitcomp "diff3 merge" "" "${cur##--conflict=}"
;;
--*)
-   __gitcomp_builtin checkout "--no-track --no-recurse-submodules"
+   __gitcomp_builtin checkout
;;
*)
# check if --track, --no-track, or --no-guess was specified
@@ -1332,7 +1330,7 @@ _git_clone ()
 {
case "$cur" in
--*)
-   __gitcomp_builtin clone "--no-single-branch"
+   __gitcomp_builtin clone
return
;;
esac
@@ -1365,7 +1363,7 @@ _git_commit ()
return
;;
--*)
-   __gitcomp_builtin commit "--no-edit --verify"
+   __gitcomp_builtin commit
return
esac
 
@@ -1468,7 +1466,7 @@ _git_fetch ()
return
;;
--*)
-   __gitcomp_builtin fetch "--no-tags"
+   __gitcomp_builtin fetch
return
;;
esac
@@ -1505,7 +1503,7 @@ _git_fsck ()
 {
case "$cur" in
--*)
-   __gitcomp_builtin fsck "--no-reflogs"
+   __gitcomp_builtin fsck
return
;;
esac
@@ -1612,7 +1610,7 @@ _git_ls_files ()
 {
case "$cur" in
--*)
-   __gitcomp_builtin ls-files "--no-empty-directory"
+   __gitcomp_builtin ls-files
return
;;
esac
@@ -1763,12 +1761,7 @@ _git_merge ()
 
case "$cur" in
--*)
-   __gitcomp_builtin merge "--no-rerere-autoupdate
-   --no-commit --no-edit --no-ff
-   --no-log --no-progress
-   --no-squash --no-stat
-   --no-verify-signatures
-   "
+   __gitcomp_builtin merge
return
esac
__git_complete_refs
@@ -1867,10 +1860,7 @@ _git_pull ()
return
;;
--*)
-   __gitcomp_builtin pull "--no-autostash --no-commit --no-edit
-   --no-ff --no-log --no-progress 
--no-rebase
-   --no-squash --no-stat --no-tags
-   --no-verify-signatures"
+   __gitcomp_builtin pull
 
return
;;
@@ -2061,7 +2051,7 @@ _git_status ()
return
;;
--*)
-   __gitcomp_builtin status "--no-column"
+   __gitcomp_builtin status
return
;;
esac
@@ -2615,7 +2605,7 @@ _git_remote ()
 
case "$subcommand,$cur" in
add,--*)
-   __gitcomp_builtin remote_add "--no-tags"
+   __gitcomp_builtin remote_add
;;
add,*)
;;
@@ -2695,7 +2685,7 @@ _git_revert ()
fi
case "$cur" in
--*)
-   __gitcomp_builtin revert "--no-edit" \
+   __gitcomp_builtin revert "" \
"$__git_revert_inprogress_options"
return
;;
@@ -2765,7 +2755,7 @@ _git_show_branch ()
 {
  

[PATCH v2 3/3] completion: collapse extra --no-.. options

[Nguyễn Thái Ngọc Duy]

The commands that make use of --git-completion-helper feature could
now produce a lot of --no-xxx options that a command can take. This in
many case could nearly double the amount of completable options, using
more screen estate and also harder to search for the wanted option.

This patch attempts to mitigate that by collapsing extra --no-
options, the ones that are added by --git-completion-helper and not in
original struct option arrays. The "--no-..." option will be displayed
in this case to hint about more options, e.g.

> ~/w/git $ git clone --
--bare --origin=
--branch=  --progress
--checkout --quiet
--config=  --recurse-submodules
--depth=   --reference=
--dissociate   --reference-if-able=
--filter=  --separate-git-dir=
--hardlinks--shallow-exclude=
--ipv4 --shallow-since=
--ipv6 --shallow-submodules
--jobs=--shared
--local--single-branch
--mirror   --tags
--no-...   --template=
--no-checkout  --upload-pack=
--no-hardlinks --verbose
--no-tags

and when you complete it with --no-, all negative options will be
presented:

> ~/w/git $ git clone --no-
--no-bare --no-quiet
--no-branch   --no-recurse-submodules
--no-checkout --no-reference
--no-config   --no-reference-if-able
--no-depth--no-separate-git-dir
--no-dissociate   --no-shallow-exclude
--no-filter   --no-shallow-since
--no-hardlinks--no-shallow-submodules
--no-ipv4 --no-shared
--no-ipv6 --no-single-branch
--no-jobs --no-tags
--no-local--no-template
--no-mirror   --no-upload-pack
--no-origin   --no-verbose
--no-progress

Corner case: to make sure that people will never accidentally complete
the fake option "--no-..." there must be one real --no- in the first
complete listing even if it's not from the original struct option.

PS. This could could be made simpler with ";&" to fall through from
"--no-*" block and share the code but ";&" is not available on bash-3
(i.e. Mac)

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 contrib/completion/git-completion.bash | 20 +++
 parse-options.c| 72 +++---
 t/t9902-completion.sh  | 13 +
 3 files changed, 76 insertions(+), 29 deletions(-)

diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index 952e660f06..4eef353ee2 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -266,9 +266,29 @@ __gitcomp ()
case "$cur_" in
--*=)
;;
+   --no-*)
+   local c i=0 IFS=$' \t\n'
+   for c in $1; do
+   if [[ $c == "--" ]]; then
+   continue
+   fi
+   c="$c${4-}"
+   if [[ $c == "$cur_"* ]]; then
+   case $c in
+   --*=*|*.) ;;
+   *) c="$c " ;;
+   esac
+   COMPREPLY[i++]="${2-}$c"
+   fi
+   done
+   ;;
*)
local c i=0 IFS=$' \t\n'
for c in $1; do
+   if [[ $c == "--" ]]; then
+   COMPREPLY[i++]="${2-}--no-...${4-} "
+   break
+   fi
c="$c${4-}"
if [[ $c == "$cur_"* ]]; then
case $c in
diff --git a/parse-options.c b/parse-options.c
index b86612148f..7db84227ab 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -427,12 +427,61 @@ void parse_options_start(struct parse_opt_ctx_t *ctx,
parse_options_check(options);
 }
 
+static void show_negated_gitcomp(const struct option *opts, int nr_noopts)
+{
+   int printed_dashdash = 0;
+
+   for (; opts->type != OPTION_END; opts++) {
+   int has_unset_form = 0;
+   const char *name;
+
+   if (!opts->long_name)
+   continue;
+   if (opts->flags & (PARSE_OPT_HIDDEN | PARSE_OPT_NOCOMPLETE))
+   continue;
+   if (opts->flags & PARSE_OPT_NONEG)
+   continue;
+
+   switch (opts->type) {
+   case OPTION_STRING:
+   case OPTION_FILENAME:
+   case OPTION_INTEGER:
+   case OPTION_MAGNITUDE:
+   case OPTION_CALLBACK:
+   case OPTION_BIT:

Re: [PATCH v2 04/11] help: add --config to list all available config

[Eric Sunshine]

On Sat, May 26, 2018 at 9:55 AM, Nguyễn Thái Ngọc Duy  wrote:
> Sometimes it helps to list all available config vars so the user can
> search for something they want. The config man page can also be used
> but it's harder to search if you want to focus on the variable name,
> for example.
>
> This is not the best way to collect the available config since it's
> not precise. Ideally we should have a centralized list of config in C
> code (pretty much like 'struct option'), but that's a lot more work.
> This will do for now.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
> diff --git a/help.c b/help.c
> @@ -409,6 +409,62 @@ void list_common_guides_help(void)
> +void list_config_help(void)
> +{
> +   for (p = config_name_list; *p; p++) {
> +   const char *var = *p;
> +   struct strbuf sb = STRBUF_INIT;
> +
> +   for (e = slot_expansions; e->prefix; e++) {
> +
> +   strbuf_reset();

Style nit: unwanted blank line

> +   strbuf_addf(, "%s.%s", e->prefix, e->placeholder);
> +   if (!strcasecmp(var, sb.buf)) {
> +   e->fn(, e->prefix);
> +   e->found++;
> +   break;
> +   }
> +   }
> +   strbuf_release();
> +   if (!e->prefix)
> +   string_list_append(, var);
> +   }

Re: [PATCH v2 1/4] diff: ignore --ita-[in]visible-in-index when diffing worktree-to-tree

[Eric Sunshine]

On Sat, May 26, 2018 at 8:08 AM, Nguyễn Thái Ngọc Duy  wrote:
> This option is supposed to fix the diff of "diff-files" (not reporting
> ita entries as new files) and "diff-index --cached " ( showing

s/(\s/(/

> ita entries as present in the index with empty content) but not
> "diff-index ".
>
> When --ita-invisible-in-index is set on "git diff-index ",
> unpack_trees() will eventually call oneway_diff() on the ita entry
> with the same code flow as "diff-index --cached ". We want to
> ignore the ita entry for "diff-index --cached " but not
> "diff-index " since the latter will examine and produce a diff
> based on worktree entry's (real) content, not ita index entry's
> (empty) content.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy 

[PATCH] git-rebase--interactive: fix copy-paste mistake

[Orgad Shaneh]

exec argument is a command, not a commit.

Signed-off-by: Orgad Shaneh 
---
  git-rebase--interactive.sh | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index cbf44f8648..85a72b933e 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -160,7 +160,7 @@ r, reword  = use commit, but edit the commit
message
  e, edit  = use commit, but stop for amending
  s, squash  = use commit, but meld into previous commit
  f, fixup  = like \"squash\", but discard this commit's log message
-x, exec  = run command (the rest of the line) using shell
+x, exec  = run command (the rest of the line) using shell
  d, drop  = remove commit
  l, label  = label current HEAD with a name
  t, reset  = reset HEAD to a label
-- 
2.17.0.windows.1