RPM spec file broken by README.md

[Ron Isaacson]

Hi everyone,

I've noticed that "make rpm" is failing for 2.8.0 because README was
replaced with README.md. This line in git.spec is the culprit:

%doc README COPYING Documentation/*.txt

Would it be possible to change this to README.md to match the source
tree? The rpm packages build just fine with that change. Thank you
very much!

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

Re: [PATCH v2 3/4] format-patch: introduce --base=auto option

[Ye Xiaolong]

On Thu, Mar 24, 2016 at 10:01:58AM -0700, Junio C Hamano wrote:
>Ye Xiaolong  writes:
>
>> On Wed, Mar 23, 2016 at 11:25:41AM -0700, Junio C Hamano wrote:
>>>I also do not see the point of showing "parent id" which as far as I
>>>can see is just a random commit object name and show different
>>>output that is not even described what it is.  It would be better to
>>
>> Here is our consideration:
>> There is high chance that branch_get_upstream will retrun NULL(thus we
>> are not able to get exact base commit), since developers may checkout
>> branch from a local branch or a commit and haven't set "--set-upstream-to"
>> to track a remote branch, in this case, we want to provide likely useful
>> info(here is parent commit id and patch id)
>
>I do not agree with that reasoning at all, primarily because your
>"likely useful" is not justfied.
>
>Could you explain what makes you think that it is "likely" that the
>commit that matches "parent commit id" is available to the recipient
>of the patch?
>

Hi, Junio

Still want to discuss with you about the proposal that showing the "parent
commit-id as well as patch-id" when exact base commit is failed to get through
cmdline or upstream", from 0day robot's view, there would be 2 kinds of base 
tree
info appended at the bottom of patch message.

For the info starts with "base-commit:  ...", robot would know it
is reliable base commit, it would just checkout it and apply the prerequisite
patches and patchset for the work.

For the info starts with "parent-commit: ; parent-patch-id: ",
there are 3 situations 0day robot would need to handle:

1) parent commit is well-known public commit(e.g. one in Linus's tree),
   in this case, robot will just checkout the parent commit and apply the
   patchset accordingly.

2) parent commit is well-known in-flight patch, since 0day maintains the
   database of in-flight patches indexed by their patch-ids and
   commit-ids(of the git tree mentioned below), it also exports a git tree
   which holds all in-flight patches, where each patchset map to a new branch:

   https://github.com/0day-ci/linux/branches

   0day will find that patch in database by parent patch id, then do the
   checkout and apply work.

3) parent commit/patch-id is unknown, maybe because it's a
   - private patch;
   - public patch that's slightly modified locally;
   - public patch that's not covered by the 0day database
   all should be rare cases.

In practice, most developers may not feed exact commit id by --base=
regularly when using git format-patch, this "showing parent commit" solution
could save developers' energy and reach a high coverage in the meantime.

Thanks,
Xiaolong.

>Whatever the reason is, if it _is_ likely, then I do not see a point
>in spending cycle to do get-upstream and merge-base, or giving an
>option to the user to explicitly specify the base.  Given that this
>series does these things, I'd have to conclude your "likely useful"
>is "might possibly turn out to be useful in some cases if you are
>lucky but is no way reliable" at best.
>
>Rather than throwing an unreliable information in the output and
>blindly proceeding, I'd think you would want to error out and tell
>the user to explicitly give the base without producing the patch
>output.  That way, you will not get patches with unreliable
>information.
>
>Suggesting to use set-upstream-to when you give that error message
>may also be helpful.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] clone: respect configured fetch respecs during initial fetch

[SZEDER Gábor]

Quoting Junio C Hamano :


SZEDER Gábor  writes:


Quoting Junio C Hamano :


IOW, special casing -c remote.origin.fetch=spec
is a bad idea.


I completely agree :)
But it's the other way around.

'remote.origin.fetch=spec' during clone is special _now_, because the
initial fetch ignores it, no matter where it is set.

My patch makes it non-special, so that the initial fetch respects it,
the same way it already respects 'fetch.fsckObjects' and
'fsck.unpackLimit', or the way the initial checkout respects e.g.
'core.eol'.


... but does "git -c core.eol clone" leave that configuration in the
resulting repository's .git/config?  "git -c user.name=foo" for that
matter.


No, and it shouldn't.

'git clone -c core.eol', however, should and indeed does.


They may affect the one-shot operation but are not left in the
resulting .git/config, which was what I was driving at.  To make
clone behave as if it is truly a short-hand of

git init
git config ;# with default and necessary tweaks to adjust
   ;# for things like -b, -o, --single-branch
   git fetch
   git checkout

which by the way I think everybody agrees is a worth goal, then
shouldn't the update to the current code be more like "prepare the
default config, tweak with whatever configuration necessary, and
re-read the config before driving the equivalent of 'git fetch'?"

And the conclusion my rhetorical questions led to was that "adjust
for things like..." should not include what comes from "-c var=val"
because there is no sensible way to incorporate them in general.

The most important point is that "-c var=val" is the wrong source of
information to blindly propagete to the resulting .git/config.


In case of 'git -c var=val clone', I agree, but then again, 'git clone
-c var=val' was specifically designed to write that 'var=val' to the
new repository's config file.

Config variables set in the global or system-wide config files are not
written to the new repository's config file either.


And
the point of "--branches" option is not that it would be short and
tidy, but it is more targetted.  With such an approach, nobody would
imagine "git -c random.var=value clone" would be propagated into the
resulting .git/config in a random and unspecified way.

Once you learn what custom set of refs the user wants to fetch, you
would need futzing of the refspecs like you did in your patch. That
part of your patch is salvageable.  The part that special cased the
information that came from "-c remote.origin.fetch" while ignoring
others like user.name that came from exactly the same mechanism via
"-c user.name" is not.


My patch did not special case anything and it did not change anything
with respect to what config settings are written under what
circumstances to the new repository's config file.
All that already works properly and almost all those config settings
are already taken into account when they should be by the commands in
our conceptual model of 'git clone'.  It doesn't matter at all where
and how they were specified or whether they are written to the new
repository's config file or not, almost all of them are taken into
account.

Almost all, because there is that one exception: additional fetch
refspecs are ignored during the initial fetch.

And all my patch did was to make the initial fetch aware of any
additional fetch refspecs which are configured when the initial
fetch is executed.  (and again: no matter where those refspecs were
specified and no matter whether they were written to the new config
file)


Eh, I guess I should have went for a refined version of the RFC's
commit message, rewriting it based on that conceptual modell caused
way too much confusion.


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

Re: [PATCH 00/21] replacement for dt/refs-backend-lmdb v7 patch 04/33

[Stefan Beller]

On Wed, Mar 30, 2016 at 1:05 PM, David Turner  wrote:
> On Wed, 2016-03-30 at 08:37 +0200, Michael Haggerty wrote:
>> On 03/29/2016 10:12 PM, David Turner wrote:
>> > On Sun, 2016-03-27 at 07:22 +0200, Michael Haggerty wrote:
>> > > On 03/24/2016 07:47 AM, David Turner wrote:
>> > > > [...]
>> > > > I incorporated your changes into the lmdb backend.  To make
>> > > > merging
>> > > > later more convenient, I rebased on top of pu -- I think this
>> > > > mainly
>> > > > depends on jk/check-repository-format, but I also included some
>> > > > fixes
>> > > > for a couple of tests that had been changed by other patches.
>> > >
>> > > I think rebasing changes on top of pu is counterproductive. I
>> > > believe
>> > > that Junio had extra work rebasing your earlier series onto a
>> > > merge
>> > > of
>> > > the minimum number of topics that it really depended on. There is
>> > > no
>> > > way
>> > > that he could merge the branch in this form because it would
>> > > imply
>> > > merging all of pu.
>> > >
>> > > See the zeroth section of SubmittingPatches [1] for the
>> > > guidelines.
>> >
>> > I'm a bit confused because
>> > [PATCH 18/21] get_default_remote(): remove unneeded flag variable
>> >
>> > doesn't do anything on master -- it depends on some patch in pu.
>> >  And
>> > we definitely want to pick up jk/check-repository-format (which
>> > doesn't
>> > include whatever 18/21 depends on).
>> >
>> > So what do you think our base should be?
>>
>> I think the preference is to base a patch series on the merge of
>> master
>> plus the minimum number of topics in pu (ideally, none) that are
>> "essential" prerequisites of the changes in the patch series. For
>> example, the version of this patch series that Junio has in his tree
>> was
>> based on master + sb/submodule-parallel-update.
>>
>> Even if there are minor
>> conflicts with another in-flight topic, it is easier for Junio to
>> resolve the conflicts when merging the topics together than to rebase
>> the patch series over and over as the other patch series evolves. The
>> goal of this practice is of course to allow patch series to evolve
>> independently of each other as much as possible.
>>
>> Of course if you have insights into nontrivial conflicts between your
>> patch series and others, it would be helpful to discuss these in your
>> cover letter.
>
> If I am reading this correctly, it looks like your series also has a
> few more sb submodule patches, e.g. sb/submodule-init, which is
> responsible for the code that 18/21 depends on.
>
> I think jk/check-repository-format is also  good to get in first,
> because it changes the startup sequence a bit and it's a bit tricky to
> figure out what needs to change in dt/refs-backend-lmdb as a result of
> it.
>
> But I can't just merge jk/check-repository-format on top of 71defe0047
> -- some function signatures have changed in the run-command stuff and
> it seems kind of annoying to fix up.
>
> So I propose instead that we just drop 18/21 for now, and use just
> jk/check-repository-format as the base.

By 18/21 you mean
[PATCH 18/21] get_default_remote(): remove unneeded flag variable
in builtin/submodule--helper.c?
You could drop that and I'll pick it up in one of the submodule series',
if that is more convenient for you.


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

GIT_CONFIG - what's the point?

[Matthew Persico]

Greetings.

Given the GIT_CONFIG environment variable can change 'git config'
behaves, it stands to reason that if GIT_CONFIG is defined, then ALL
git commands obey the value of GIT_CONFIG and use that file for config
info.

As a test, exported GIT_CONFIG=/tmp/ohm, copied ~/.gitconfig to
/tmp/ohm, moved ~/.gitconfig to ~/.gitconfig.hold and then tried git
st, where 'st' is an alias for status in my config file.

No dice. git st was unrecognized.

So, what's the point of GIT_CONFIG if only git-config uses it? Or did
I miss a step?

Thanks

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

Re: `git rev-parse --is-inside-work-tree` and $GIT_WORK_TREE

[Elliott Cable]

oh, wow, this got over my head *real* fast. Okay,

1. Yeah, my `$GIT_WORK_TREE` was def. an absolute path; I typed that
example code without running it *precisely* that way (entirely my
mistake! I'm so sorry for the confusion it caused, and all that typing
you did!); if I remember correctly (not at the machine right now), I
had run `git rev-parse --show-toplevel` from a different directory,
with `$GIT_DIR` set, while trying to narrow down this bug, so it gave
me an absolute path … and then copy-pasted that path, and then
replaced my copy-paste with the original command to make the
bug-report example as concise as possible? oops. But, yeah, it fails
in the manner described above with an absolute path. (Which it looks
like you two figured out above.)

2. Re: intentions, again, it seems like you've changed your mind, but …

   >  So it is a misconfiguration if you only set GIT_WORK_TREE
without setting GIT_DIR.

   I really, really hope not! Half the usefulness of `$GIT_WORK_TREE`
existing is in that mode. In fact, that's how I found `$GIT_WORK_TREE`
documented and explained, in [this blog
post](https://git-scm.com/blog/2010/04/11/environment.html) on the Git
site. That usage seems pretty damn useful, so I do hope it's
eventually explicitly supported … (and if that's *not* going to be the
case, it should be explicitly documented in the `GIT(1)` manpage,
alongside the other documentation of the environment-variables, that
the behaviour is undefined is `$GIT_DIR` isn't set first. =)


⁓ ELLIOTTCABLE — fly safe.
  http://ell.io/tt
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv4 1/4] notes: don't leak memory in git_config_get_notes_strategy

[Eric Sunshine]

On Thu, Mar 31, 2016 at 8:35 PM, Stefan Beller  wrote:
> `value` is just a temporary scratchpad, so we need to make sure it doesn't
> leak. It is xstrdup'd in `git_config_get_string_const` and
> `parse_notes_merge_strategy` just compares the string against predefined
> values, so no need to keep it around longer.
>
> Signed-off-by: Stefan Beller 
> ---
> diff --git a/builtin/notes.c b/builtin/notes.c
> index 52aa9af..afcfa8f 100644
> --- a/builtin/notes.c
> +++ b/builtin/notes.c
> @@ -741,13 +741,14 @@ static int merge_commit(struct notes_merge_options *o)
>  static int git_config_get_notes_strategy(const char *key,
>  enum notes_merge_strategy *strategy)
>  {
> -   const char *value;
> +   char *value;
>
> -   if (git_config_get_string_const(key, ))
> +   if (git_config_get_string(key, ))
> return 1;

Meh. Rather than reverting the git_config_get_value(), it would have
been just as easy and safer (less chance of a future change
re-introducing a leak) if you had just inserted the necessary check
here:

if (!value)
return  config_error_nonbool(key);

But, perhaps it's not worth the patch churn at this point...

> if (parse_notes_merge_strategy(value, strategy))
> git_die_config(key, "unknown notes merge strategy %s", value);
>
> +   free(value);
> return 0;
>  }
>
> --
> 2.5.0.264.gc776916.dirty
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCHv4 4/4] credential-cache, send_request: close fd when done

[Stefan Beller]

No need to keep it open any further.

Signed-off-by: Stefan Beller 
---
 credential-cache.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/credential-cache.c b/credential-cache.c
index f4afdc6..86e21de 100644
--- a/credential-cache.c
+++ b/credential-cache.c
@@ -32,6 +32,7 @@ static int send_request(const char *socket, const struct 
strbuf *out)
write_or_die(1, in, r);
got_data = 1;
}
+   close(fd);
return got_data;
 }
 
-- 
2.5.0.264.gc776916.dirty

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

[PATCHv4 2/4] abbrev_sha1_in_line: don't leak memory

[Stefan Beller]

`split` is of type `struct strbuf **`, and currently we are leaking split
itself as well as each element in split[i]. We have a dedicated free
function for `struct strbuf **`, which takes care of freeing all
related memory.

Helped-by: Eric Sunshine 
Signed-off-by: Stefan Beller 
---
 wt-status.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/wt-status.c b/wt-status.c
index bba2596..9f4be33 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1063,9 +1063,7 @@ static void abbrev_sha1_in_line(struct strbuf *line)
strbuf_addf(line, "%s", split[i]->buf);
}
}
-   for (i = 0; split[i]; i++)
-   strbuf_release(split[i]);
-
+   strbuf_list_free(split);
 }
 
 static void read_rebase_todolist(const char *fname, struct string_list *lines)
-- 
2.5.0.264.gc776916.dirty

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

[PATCHv4 3/4] bundle: don't leak an fd in case of early return

[Stefan Beller]

In successful operation `write_pack_data` will close the `bundle_fd`,
but when we exit early, we need to take care of the file descriptor
as well as the lock file ourselves. The lock file may be deleted at the
end of running the program, but we are in library code, so we should
not rely on that.

Helped-by: Jeff King 
Signed-off-by: Stefan Beller 
---
 bundle.c | 18 +-
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/bundle.c b/bundle.c
index 506ac49..08e32ca 100644
--- a/bundle.c
+++ b/bundle.c
@@ -435,12 +435,14 @@ int create_bundle(struct bundle_header *header, const 
char *path,
 
/* write prerequisites */
if (compute_and_write_prerequisites(bundle_fd, , argc, argv))
-   return -1;
+   goto err;
 
argc = setup_revisions(argc, argv, , NULL);
 
-   if (argc > 1)
-   return error(_("unrecognized argument: %s"), argv[1]);
+   if (argc > 1) {
+   error(_("unrecognized argument: %s"), argv[1]);
+   goto err;
+   }
 
object_array_remove_duplicates();
 
@@ -448,17 +450,23 @@ int create_bundle(struct bundle_header *header, const 
char *path,
if (!ref_count)
die(_("Refusing to create empty bundle."));
else if (ref_count < 0)
-   return -1;
+   goto err;
 
/* write pack */
if (write_pack_data(bundle_fd, ))
-   return -1;
+   goto err;
 
if (!bundle_to_stdout) {
if (commit_lock_file())
die_errno(_("cannot create '%s'"), path);
}
return 0;
+err:
+   if (!bundle_to_stdout) {
+   close(bundle_fd);
+   rollback_lock_file();
+   }
+   return -1;
 }
 
 int unbundle(struct bundle_header *header, int bundle_fd, int flags)
-- 
2.5.0.264.gc776916.dirty

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

[PATCHv4 1/4] notes: don't leak memory in git_config_get_notes_strategy

[Stefan Beller]

`value` is just a temporary scratchpad, so we need to make sure it doesn't
leak. It is xstrdup'd in `git_config_get_string_const` and
`parse_notes_merge_strategy` just compares the string against predefined
values, so no need to keep it around longer.

Signed-off-by: Stefan Beller 
---
 builtin/notes.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/builtin/notes.c b/builtin/notes.c
index 52aa9af..afcfa8f 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -741,13 +741,14 @@ static int merge_commit(struct notes_merge_options *o)
 static int git_config_get_notes_strategy(const char *key,
 enum notes_merge_strategy *strategy)
 {
-   const char *value;
+   char *value;
 
-   if (git_config_get_string_const(key, ))
+   if (git_config_get_string(key, ))
return 1;
if (parse_notes_merge_strategy(value, strategy))
git_die_config(key, "unknown notes merge strategy %s", value);
 
+   free(value);
return 0;
 }
 
-- 
2.5.0.264.gc776916.dirty

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

[PATCHv4 0/4] Some cleanups

[Stefan Beller]

Thanks for the reviews!
Thanks Jeff, Eric, Junio!

This replaces sb/misc-cleanups.

* Revert the builtin/notes fix to the first version as git_config_get_value
  is dangerous, and the memory allocation and free is just a small overhead here
* the bundle code integrates all of the suggestions (i.e. rollback_lock_file
  conditioned on (!bundle_to_stdout).
  
I hope I got everything by now. I will head out for today and tomorrow I'll
be traveling to NY, where there is the Git Merge conference on Mon, Tues and 
some
vacation afterwards. I'll be online in a very limited fashion. (I plan on taking
my phone only, no laptop), so in case I missed a thing this series will halt
for a while or someone else picks it up.

Thanks,
Stefan

diff to remotes/origin/sb/misc-cleanups (which doesn't contain the bundle fix):
diff --git a/builtin/notes.c b/builtin/notes.c
index 386402d..afcfa8f 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -741,13 +741,14 @@ static int merge_commit(struct notes_merge_options *o)
 static int git_config_get_notes_strategy(const char *key,
 enum notes_merge_strategy *strategy)
 {
-   const char *value;
+   char *value;
 
-   if (git_config_get_value(key, ))
+   if (git_config_get_string(key, ))
return 1;
if (parse_notes_merge_strategy(value, strategy))
git_die_config(key, "unknown notes merge strategy %s", value);
 
+   free(value);
return 0;
 }
 
diff --git a/bundle.c b/bundle.c
index 506ac49..08e32ca 100644
--- a/bundle.c
+++ b/bundle.c
@@ -435,12 +435,14 @@ int create_bundle(struct bundle_header *header, const 
char *path,
 
/* write prerequisites */
if (compute_and_write_prerequisites(bundle_fd, , argc, argv))
-   return -1;
+   goto err;
 
argc = setup_revisions(argc, argv, , NULL);
 
-   if (argc > 1)
-   return error(_("unrecognized argument: %s"), argv[1]);
+   if (argc > 1) {
+   error(_("unrecognized argument: %s"), argv[1]);
+   goto err;
+   }
 
object_array_remove_duplicates();
 
@@ -448,17 +450,23 @@ int create_bundle(struct bundle_header *header, const 
char *path,
if (!ref_count)
die(_("Refusing to create empty bundle."));
else if (ref_count < 0)
-   return -1;
+   goto err;
 
/* write pack */
if (write_pack_data(bundle_fd, ))
-   return -1;
+   goto err;
 
if (!bundle_to_stdout) {
if (commit_lock_file())
die_errno(_("cannot create '%s'"), path);
}
return 0;
+err:
+   if (!bundle_to_stdout) {
+   close(bundle_fd);
+   rollback_lock_file();
+   }
+   return -1;
 }
 
 int unbundle(struct bundle_header *header, int bundle_fd, int flags)


Stefan Beller (4):
  notes: don't leak memory in git_config_get_notes_strategy
  abbrev_sha1_in_line: don't leak memory
  bundle: don't leak an fd in case of early return
  credential-cache, send_request: close fd when done

 builtin/notes.c|  5 +++--
 bundle.c   | 18 +-
 credential-cache.c |  1 +
 wt-status.c|  4 +---
 4 files changed, 18 insertions(+), 10 deletions(-)

-- 
2.5.0.264.gc776916.dirty

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

Re: [PATCH v2] clone: respect configured fetch respecs during initial fetch

[SZEDER Gábor]

Quoting Junio C Hamano :


SZEDER Gábor  writes:


Conceptually 'git clone' should behave as if the following commands
were run:

  git init
  git config ... # set default configuration and origin remote
  git fetch
  git checkout   # unless '--bare' is given

However, that initial 'git fetch' behaves differently from any
subsequent fetches, because it takes only the default fetch refspec
into account and ignores all other fetch refspecs that might have
been explicitly specified on the command line (e.g. 'git -c
remote.origin.fetch= clone' or 'git clone -c ...').


Is it really 'git fetch' behaves differently?


Certainly.


What is missing in the above description is your expected behaviour
of "git -c var=val clone", and without it we cannot tell if your
expectation is sane to begin with.


These 'git -c var=val cmd' one-shot configuration parameters exist
during the lifespan of the command.  Therefore, in case of 'git -c
var=val clone' they should exist while all the commands in our
mental model are executed.  IOW, those commands should behave as if
these configuration parameters were specified for them, see below.


Is the expectation like this?

git init
git config ... # set default configuration and origin remote
git config var val # update with what "-c var=val" told us
git fetch
git checkout   # unless '--bare' is given

or is it something else?


For 'git -c var=val clone':

  git -c var=val init
  git -c var=val config ... # though this probably doesn't really
# matter, as it is about writing the
# configuration, and it gets the
# to-be-written variables and values
# in the "..." part anyway
  git -c var=val fetch
  git -c var=val checkout

Being one-shot configuration parameters, they shouldn't be written
to the new repository's config file.

'git clone -c var=val' is designed to be different:

  - it does write var=val into the new repository's config file

  - it specifies that var.val "takes effect immediately after the
repository is initialized, but before the remote history is
fetched or any files checked out".

Additionally, there may be relevant config variables defined in the
global and system-wide config files, which of course should be
respected by all these commands.

And it all works just fine as described above, even the initial fetch
respects most of the config variables, wherever specified, except for
fetch refspecs which are completely ignored.


Is "-c var=val" adding to the config variables set by default, or is
it replacing them?  Does the choice depend on the nature of
individual variables, and if so what is the criteria?


It's up to the individual command how it treats a particular config
variable: single-valued variables like 'fetch.fsckObjects' should
override (they already do), multi-valued variables like fetch refspecs
should be added.
Running as part of 'git clone' shouldn't matter at all.

This patch only affects how fetch refspecs are handled, the effects of
other config variables are unchanged.


Are all "-c var=val" update the configuration of the resulting
repository?  Or are certain "var"s treated as special and placed in
the config but not other "var"s?  If the latter, what makes these
certain "var"s special?


In this regard it doesn't matter what 'val=var' is.  What matters is
how the configuration parameter is specified (i.e. 'git -c var=val
clone' vs. 'git clone -c var=val').

This patch doesn't change anything in this regard.


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

Re: [PATCHv2 3/4] bundle: don't leak an fd in case of early return

[Stefan Beller]

On Thu, Mar 31, 2016 at 12:00 PM, Philip Oakley  wrote:
> From: "Stefan Beller" 
>>
>> In successful operation `write_pack_data` will close the `bundle_fd`,
>> but when we exit early, we need to take care of the file descriptor
>> as well as the lock file ourselves. The lock file may be deleted at the
>> end of running the program, but we are in library code, so we should
>> not rely on that.
>>
>> Helped-by: Jeff King 
>> Signed-off-by: Stefan Beller 
>
>
> Has this been tested on Windows? I had a similar problem very recently
> https://groups.google.com/forum/#!msg/git-for-windows/6LPxf9xZKhI/-s7XD18yCwAJ
> where a bad rev-list-arg would cause the `bundle create` to die, and, on
> windows, leave the incomplete bundle file locked.
>
> dscho suggested one possible solution for that, but I haven't had any time
> to try any patches.

I think with Jeffs suggestion to only rollback the lock in case of
(!bundle_to_stdout)
we're not making it worse. I do not have a Windows machine at hand to test it :(
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCHv3 0/2] Fix relative path issues in recursive submodules.

[Stefan Beller]

Thanks Junio for review!

v3:
 * This is a resend of the last two patches of the series, i.e. it replaces
   44859d6626d4 and efdef1e2e in sb/submodule-helper-clone-regression-fix
  
 * use absolute_path for sm_gitdir
 * removed todo
 * we need to free the intermediate sm_gitdir, so rename that to sm_gitdir_rel
   and free it afterwards.
 * while we currently do not support an absolute `path`, we eventually might.
   If `path` is absolute it would be a pointer to `argv`, which would lead to a
   crash. Duplicate the path and the crash is prevented.
 (* I thought we could use it as well for `path`, but we cannot; as 
   get_git_work_tree() != cwd)
 * diff to sb/submodule-helper-clone-regression-fix:

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 89cbbda..be7bf5f 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -153,12 +153,13 @@ static int clone_submodule(const char *path, const char 
*gitdir, const char *url
 
 static int module_clone(int argc, const char **argv, const char *prefix)
 {
-   const char *path = NULL, *name = NULL, *url = NULL;
+   const char *name = NULL, *url = NULL;
const char *reference = NULL, *depth = NULL;
int quiet = 0;
FILE *submodule_dot_git;
-   char *sm_gitdir, *p;
-   struct strbuf rel_path = STRBUF_INIT; /* for relative_path */
+   char *sm_gitdir_rel, *p, *path = NULL;
+   const char *sm_gitdir;
+   struct strbuf rel_path = STRBUF_INIT;
struct strbuf sb = STRBUF_INIT;
 
struct option module_clone_options[] = {
@@ -198,26 +199,14 @@ static int module_clone(int argc, const char **argv, 
const char *prefix)
die(_("submodule--helper: unspecified or empty --path"));
 
strbuf_addf(, "%s/modules/%s", get_git_dir(), name);
-   sm_gitdir = strbuf_detach(, NULL);
-
-
-   if (!is_absolute_path(sm_gitdir)) {
-   char *cwd = xgetcwd();
-   strbuf_addf(, "%s/%s", cwd, sm_gitdir);
-   sm_gitdir = strbuf_detach(, 0);
-   free(cwd);
-   }
+   sm_gitdir_rel = strbuf_detach(, NULL);
+   sm_gitdir = absolute_path(sm_gitdir_rel);
 
if (!is_absolute_path(path)) {
-   /*
-* TODO: add prefix here once we allow calling from non root
-* directory?
-*/
-   strbuf_addf(, "%s/%s",
-   get_git_work_tree(),
-   path);
+   strbuf_addf(, "%s/%s", get_git_work_tree(), path);
path = strbuf_detach(, 0);
-   }
+   } else
+   path = xstrdup(path);
 
if (!file_exists(sm_gitdir)) {
if (safe_create_leading_directories_const(sm_gitdir) < 0)
@@ -240,6 +229,7 @@ static int module_clone(int argc, const char **argv, const 
char *prefix)
submodule_dot_git = fopen(sb.buf, "w");
if (!submodule_dot_git)
die_errno(_("cannot open file '%s'"), sb.buf);
+
fprintf_or_die(submodule_dot_git, "gitdir: %s\n",
   relative_path(sm_gitdir, path, _path));
if (fclose(submodule_dot_git))
@@ -255,8 +245,8 @@ static int module_clone(int argc, const char **argv, const 
char *prefix)
   relative_path(path, sm_gitdir, _path));
strbuf_release();
strbuf_release(_path);
-   free(sm_gitdir);
-
+   free(sm_gitdir_rel);
+   free(path);
free(p);
return 0;
 }

v2:
 * reworded commit message for test (Thanks Junio!)
 * instead of "simplifying" the path handling in patch 2, we are prepared
   for anything the user throws at us (i.e. instead of segfault we
   die(_("submodule--helper: unspecified or empty --path"));
   (Thanks Eric, Jacob for pushing back here!)
 * reword commit message for patch 3 (safe_create_leading_directories_const is
   not a check, Thanks Junio!)
 * patch 4 (the fix): That got reworked completely. No flow controlling
   conditions in the write out phase!
 * patch 5 is a bonus! (replace fprintf by fprintf_or die) When implementing
   that I wondered if we also have close_or_die and open_or_die, but that 
doesn't
   seem to be the case.

Thanks,
Stefan

v1:

It took me longer than expected to fix this bug.

It comes with a test and minor refactoring and applies on top of
origin/sb/submodule-helper, such that it can be merged into 2.7, 2.8 as well
as master.

Patch 1 is a test which fails; it has a similar layout as the
real failing repository Norio Nomura pointed out, but simplified to the
bare essentials for reproduction of the bug.

Patch 2&3 are not strictly necessary for fixing the isseu, but it removes
stupid code I wrote, so the resulting code looks a little better.

Patch 4 fixes the issue by giving more information to relative_path,
such that a relative path can be found in all cases.

Thanks,
Stefan

Stefan Beller (2):
  submodule--helper, module_clone: always operate on 

[PATCH 2/2] submodule--helper, module_clone: catch fprintf failure

[Stefan Beller]

The return value of fprintf is unchecked, which may lead to
unreported errors. Use fprintf_or_die to report the error to the user.

Signed-off-by: Stefan Beller 
---
 builtin/submodule--helper.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index b099817..be7bf5f 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -230,8 +230,8 @@ static int module_clone(int argc, const char **argv, const 
char *prefix)
if (!submodule_dot_git)
die_errno(_("cannot open file '%s'"), sb.buf);
 
-   fprintf(submodule_dot_git, "gitdir: %s\n",
-   relative_path(sm_gitdir, path, _path));
+   fprintf_or_die(submodule_dot_git, "gitdir: %s\n",
+  relative_path(sm_gitdir, path, _path));
if (fclose(submodule_dot_git))
die(_("could not close file %s"), sb.buf);
strbuf_reset();
-- 
2.5.0.264.gc776916.dirty

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

[PATCH 1/2] submodule--helper, module_clone: always operate on absolute paths

[Stefan Beller]

When giving relative paths to `relative_path` to compute a relative path
from one directory to another, this may fail in `relative_path`.
Make sure both arguments to `relative_path` are always absolute.

Signed-off-by: Stefan Beller 
---
 builtin/submodule--helper.c | 28 ++--
 t/t7400-submodule-basic.sh  |  2 +-
 2 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 0b9f546..b099817 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -153,11 +153,12 @@ static int clone_submodule(const char *path, const char 
*gitdir, const char *url
 
 static int module_clone(int argc, const char **argv, const char *prefix)
 {
-   const char *path = NULL, *name = NULL, *url = NULL;
+   const char *name = NULL, *url = NULL;
const char *reference = NULL, *depth = NULL;
int quiet = 0;
FILE *submodule_dot_git;
-   char *sm_gitdir, *cwd, *p;
+   char *sm_gitdir_rel, *p, *path = NULL;
+   const char *sm_gitdir;
struct strbuf rel_path = STRBUF_INIT;
struct strbuf sb = STRBUF_INIT;
 
@@ -198,7 +199,14 @@ static int module_clone(int argc, const char **argv, const 
char *prefix)
die(_("submodule--helper: unspecified or empty --path"));
 
strbuf_addf(, "%s/modules/%s", get_git_dir(), name);
-   sm_gitdir = strbuf_detach(, NULL);
+   sm_gitdir_rel = strbuf_detach(, NULL);
+   sm_gitdir = absolute_path(sm_gitdir_rel);
+
+   if (!is_absolute_path(path)) {
+   strbuf_addf(, "%s/%s", get_git_work_tree(), path);
+   path = strbuf_detach(, 0);
+   } else
+   path = xstrdup(path);
 
if (!file_exists(sm_gitdir)) {
if (safe_create_leading_directories_const(sm_gitdir) < 0)
@@ -229,24 +237,16 @@ static int module_clone(int argc, const char **argv, 
const char *prefix)
strbuf_reset();
strbuf_reset(_path);
 
-   cwd = xgetcwd();
/* Redirect the worktree of the submodule in the superproject's config 
*/
-   if (!is_absolute_path(sm_gitdir)) {
-   strbuf_addf(, "%s/%s", cwd, sm_gitdir);
-   free(sm_gitdir);
-   sm_gitdir = strbuf_detach(, NULL);
-   }
-
-   strbuf_addf(, "%s/%s", cwd, path);
p = git_pathdup_submodule(path, "config");
if (!p)
die(_("could not get submodule directory for '%s'"), path);
git_config_set_in_file(p, "core.worktree",
-  relative_path(sb.buf, sm_gitdir, _path));
+  relative_path(path, sm_gitdir, _path));
strbuf_release();
strbuf_release(_path);
-   free(sm_gitdir);
-   free(cwd);
+   free(sm_gitdir_rel);
+   free(path);
free(p);
return 0;
 }
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index fc11809..ea3fabb 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -818,7 +818,7 @@ test_expect_success 'submodule add --name allows to replace 
a submodule with ano
)
 '
 
-test_expect_failure 'recursive relative submodules stay relative' '
+test_expect_success 'recursive relative submodules stay relative' '
test_when_finished "rm -rf super clone2 subsub sub3" &&
mkdir subsub &&
(
-- 
2.5.0.264.gc776916.dirty

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

Re: [PATCH] builtin/apply: free patch when parse_chunk() fails

[Christian Couder]

On Fri, Apr 1, 2016 at 12:56 AM, Junio C Hamano  wrote:
> Christian Couder  writes:
>
>> On Wed, Mar 16, 2016 at 3:35 PM, Christian Couder
>>  wrote:
>>> When parse_chunk() fails it can return -1, for example
>>> when find_header() doesn't find a patch header.
>>>
>>> In this case it's better in apply_patch() to free the
>>> "struct patch" that we just allocated instead of
>>> leaking it.
>>
>> Maybe this patch has fallen through the cracks too.
>
> Anything worthy of discussion that you sent during the feature
> freeze, please resend them to the list for discussion.

It looks like only the two patches I replied to have not been applied to next.
I had also sent a three patch long series
(http://thread.gmane.org/gmane.comp.version-control.git/289559), but
we agreed that only two of them could be merged for now and that's
what you did already (fda3e2c and db354b7).

By the way my guess is that replying to them is ok, but you ask to
"resend them", so I am wondering if you really prefer them to be
resent rather than just replied to.

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

Re: [PATCHv3 1/4] notes: don't leak memory in git_config_get_notes_strategy

[Jeff King]

On Thu, Mar 31, 2016 at 05:08:30PM -0400, Eric Sunshine wrote:

> On Thu, Mar 31, 2016 at 2:04 PM, Stefan Beller  wrote:
> > `value` is just a temporary scratchpad, so we need to make sure it doesn't
> > leak. It is xstrdup'd in `git_config_get_string_const` and
> > `parse_notes_merge_strategy` just compares the string against predefined
> > values, so no need to keep it around longer. Instead of using
> > `git_config_get_string_const`, use `git_config_get_value`, which doesn't
> > return a copy.
> >
> > Signed-off-by: Stefan Beller 
> > ---
> > diff --git a/builtin/notes.c b/builtin/notes.c
> > @@ -746,7 +746,7 @@ static int git_config_get_notes_strategy(const char 
> > *key,
> >  {
> > const char *value;
> >
> > -   if (git_config_get_string_const(key, ))
> > +   if (git_config_get_value(key, ))
> 
> Hmm, doesn't this introduce a rather severe regression? Unless I'm
> misreading the code (possible), with the original, if 'key' was
> boolean (lacked a value in the config file), then it would complain:
> 
> Missing value for 'floop.blork'
> 
> but, with this change, it will dereference NULL and crash.
> 
> (My understanding was that Peff's suggestion to use
> git_config_get_value() implied a bit of work beyond the simple textual
> substitution of 'git_config_get_value' for
> 'git_config_get_string_const'.)

Ah, yeah, I didn't even think about that case. I was thinking that
wouldn't it be nice if we had:

  const char *git_config_get_string(const char *key);

which would be a much more natural interface. But the reason we don't is
that we have to represent the "NULL as boolean true" case in the first
place.

So I dunno. Getting the NULL-handling for free is rather nice, and maybe
worth using the normal git_config_get_string(). It's too bad there's not
a variant that just returns a non-allocated pointer, but given that
there is already a confusing proliferation of functions to retrieve a
config string, it's hard to justify adding another.

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

Re: [PATCH v4] git-send-pack: fix --all option when used with directory

[Jeff King]

On Thu, Mar 31, 2016 at 03:02:43PM -0700, Junio C Hamano wrote:

> By the way, for some reason it was unusually painful to find the
> exact breakage by bisecting between maint-2.4 and maint-2.6.  It
> somehow ended up on fingering random places like v2.6.0 itself.
> 
> The true culprit is 068c77a5 (builtin/send-pack.c: use parse_options
> API, 2015-08-19).  I didn't dug deep enough to tell if we recently
> broke "git bisect" or if there are something wrong in the shape of
> my history.

Weird. I bisected this when v1 was published, and didn't have any
trouble. I guess it's possible I just got lucky in where I landed,
though, if I started at different endpoints than you.

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

Re: [PATCHv2 4/5] submodule--helper, module_clone: always operate on absolute paths

[Stefan Beller]

On Thu, Mar 31, 2016 at 3:59 PM, Stefan Beller  wrote:
>
> Further checking reveals any caller uses it with
>
> desired= xstrdup(absolute_path(my_argument));
>
> We are loosing memory of the strbuf here. so if I we'd
> take the diff above we can also get rid of all the xstrdups
> at the callers. For now I will adhere to all other callers and use
> xstrdup(absolute_path(...) here too.

Actually there is only two occurrences of xstrdup in builtin/clone.c
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv2 4/5] submodule--helper, module_clone: always operate on absolute paths

[Stefan Beller]

On Thu, Mar 31, 2016 at 3:26 PM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>> + if (!is_absolute_path(sm_gitdir)) {
>> + char *cwd = xgetcwd();
>> + strbuf_addf(, "%s/%s", cwd, sm_gitdir);
>> + sm_gitdir = strbuf_detach(, 0);
>> + free(cwd);
>
> It would be surprising that this would be the first codepath that
> needs to get an absolute pathname in the codebase that is more than
> 10 years old, wouldn't it?  Don't we have a reasonable API helper
> function to do this kind of thing already?
>
> ... goes and looks ...
>
> Doesn't absolute_path() work here?

*reads absolute_path(...)*

Yes that is great. But why is it written the way it is?
I would have expected:

 const char *absolute_path(const char *path)
 {
static struct strbuf sb = STRBUF_INIT;
-   strbuf_reset();
strbuf_add_absolute_path(, path);
-   return sb.buf;
+   return strbuf_detach();
 }

Further checking reveals any caller uses it with

desired= xstrdup(absolute_path(my_argument));

We are loosing memory of the strbuf here. so if I we'd
take the diff above we can also get rid of all the xstrdups
at the callers. For now I will adhere to all other callers and use
xstrdup(absolute_path(...) here too.

I'll remove the unrelated changes as well in a resend.

>
>> @@ -221,7 +240,6 @@ static int module_clone(int argc, const char **argv, 
>> const char *prefix)
>>   submodule_dot_git = fopen(sb.buf, "w");
>>   if (!submodule_dot_git)
>>   die_errno(_("cannot open file '%s'"), sb.buf);
>> -
>>   fprintf(submodule_dot_git, "gitdir: %s\n",
>>   relative_path(sm_gitdir, path, _path));
>>   if (fclose(submodule_dot_git))
>
> Looks like an unrelated change to me.
>
>> @@ -229,24 +247,16 @@ static int module_clone(int argc, const char **argv, 
>> const char *prefix)
>>   strbuf_reset();
>>   strbuf_reset(_path);
>>
>> - cwd = xgetcwd();
>>   /* Redirect the worktree of the submodule in the superproject's config 
>> */
>> - if (!is_absolute_path(sm_gitdir)) {
>> - strbuf_addf(, "%s/%s", cwd, sm_gitdir);
>> - free(sm_gitdir);
>> - sm_gitdir = strbuf_detach(, NULL);
>> - }
>> -
>> - strbuf_addf(, "%s/%s", cwd, path);
>>   p = git_pathdup_submodule(path, "config");
>>   if (!p)
>>   die(_("could not get submodule directory for '%s'"), path);
>>   git_config_set_in_file(p, "core.worktree",
>> -relative_path(sb.buf, sm_gitdir, _path));
>> +relative_path(path, sm_gitdir, _path));
>>   strbuf_release();
>>   strbuf_release(_path);
>>   free(sm_gitdir);
>> - free(cwd);
>> +
>
> This addition of blank, too.
>
>>   free(p);
>>   return 0;
>>  }
>> diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
>> index fc11809..ea3fabb 100755
>> --- a/t/t7400-submodule-basic.sh
>> +++ b/t/t7400-submodule-basic.sh
>> @@ -818,7 +818,7 @@ test_expect_success 'submodule add --name allows to 
>> replace a submodule with ano
>>   )
>>  '
>>
>> -test_expect_failure 'recursive relative submodules stay relative' '
>> +test_expect_success 'recursive relative submodules stay relative' '
>>   test_when_finished "rm -rf super clone2 subsub sub3" &&
>>   mkdir subsub &&
>>   (
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] builtin/apply: free patch when parse_chunk() fails

[Junio C Hamano]

Christian Couder  writes:

> On Wed, Mar 16, 2016 at 3:35 PM, Christian Couder
>  wrote:
>> When parse_chunk() fails it can return -1, for example
>> when find_header() doesn't find a patch header.
>>
>> In this case it's better in apply_patch() to free the
>> "struct patch" that we just allocated instead of
>> leaking it.
>
> Maybe this patch has fallen through the cracks too.

Anything worthy of discussion that you sent during the feature
freeze, please resend them to the list for discussion.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4] git-send-pack: fix --all option when used with directory

[Eric Sunshine]

On Thu, Mar 31, 2016 at 6:02 PM, Junio C Hamano  wrote:
> By the way, for some reason it was unusually painful to find the
> exact breakage by bisecting between maint-2.4 and maint-2.6.  It
> somehow ended up on fingering random places like v2.6.0 itself.
>
> The true culprit is 068c77a5 (builtin/send-pack.c: use parse_options
> API, 2015-08-19).  I didn't dug deep enough to tell if we recently
> broke "git bisect" or if there are something wrong in the shape of
> my history.

I had a similar experience a couple months ago where I was bisecting
to find a fix (rather than breakage), and each time I ran bisect, it
seemed (randomly) to arrive at a different commit, often a merge,
rather than the real fix (which I eventually located by manually
examining commits near the commits bisect identified). At the time, I
thought I was doing something wrong (and perhaps I was), but your
descriptions sounds eerily similar to that experience.

Unfortunately, I no longer recall precisely for what I was searching
(other than that someone reported a Git bug which I recalled having
been already fixed, and wanted to use bisect to respond with the exact
commit which fixed the problem), and the bisect "run" script was
throwaway, so I can't consult that for further details.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3 1/4] notes: don't leak memory in git_config_get_notes_strategy

[Junio C Hamano]

Eric Sunshine  writes:

> On Thu, Mar 31, 2016 at 2:04 PM, Stefan Beller  wrote:
>> `value` is just a temporary scratchpad, so we need to make sure it doesn't
>> leak. It is xstrdup'd in `git_config_get_string_const` and
>> `parse_notes_merge_strategy` just compares the string against predefined
>> values, so no need to keep it around longer. Instead of using
>> `git_config_get_string_const`, use `git_config_get_value`, which doesn't
>> return a copy.
>>
>> Signed-off-by: Stefan Beller 
>> ---
>> diff --git a/builtin/notes.c b/builtin/notes.c
>> @@ -746,7 +746,7 @@ static int git_config_get_notes_strategy(const char *key,
>>  {
>> const char *value;
>>
>> -   if (git_config_get_string_const(key, ))
>> +   if (git_config_get_value(key, ))
>
> Hmm, doesn't this introduce a rather severe regression? Unless I'm
> misreading the code (possible), with the original, if 'key' was
> boolean (lacked a value in the config file), then it would complain:
>
> Missing value for 'floop.blork'
>
> but, with this change, it will dereference NULL and crash.
>
> (My understanding was that Peff's suggestion to use
> git_config_get_value() implied a bit of work beyond the simple textual
> substitution of 'git_config_get_value' for
> 'git_config_get_string_const'.)

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

Re: [PATCH v2] clone: respect configured fetch respecs during initial fetch

[Junio C Hamano]

SZEDER Gábor  writes:

> Quoting Junio C Hamano :
>
>> IOW, special casing -c remote.origin.fetch=spec
>> is a bad idea.
>
> I completely agree :)
> But it's the other way around.
>
> 'remote.origin.fetch=spec' during clone is special _now_, because the
> initial fetch ignores it, no matter where it is set.
>
> My patch makes it non-special, so that the initial fetch respects it,
> the same way it already respects 'fetch.fsckObjects' and
> 'fsck.unpackLimit', or the way the initial checkout respects e.g.
> 'core.eol'.

... but does "git -c core.eol clone" leave that configuration in the
resulting repository's .git/config?  "git -c user.name=foo" for that
matter.

They may affect the one-shot operation but are not left in the
resulting .git/config, which was what I was driving at.  To make
clone behave as if it is truly a short-hand of

git init
git config ;# with default and necessary tweaks to adjust
   ;# for things like -b, -o, --single-branch
git fetch
git checkout

which by the way I think everybody agrees is a worth goal, then
shouldn't the update to the current code be more like "prepare the
default config, tweak with whatever configuration necessary, and
re-read the config before driving the equivalent of 'git fetch'?"

And the conclusion my rhetorical questions led to was that "adjust
for things like..." should not include what comes from "-c var=val"
because there is no sensible way to incorporate them in general.

The most important point is that "-c var=val" is the wrong source of
information to blindly propagete to the resulting .git/config.  And
the point of "--branches" option is not that it would be short and
tidy, but it is more targetted.  With such an approach, nobody would
imagine "git -c random.var=value clone" would be propagated into the
resulting .git/config in a random and unspecified way.

Once you learn what custom set of refs the user wants to fetch, you
would need futzing of the refspecs like you did in your patch. That
part of your patch is salvageable.  The part that special cased the
information that came from "-c remote.origin.fetch" while ignoring
others like user.name that came from exactly the same mechanism via
"-c user.name" is not.



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

Re: [PATCH] builtin/apply: free patch when parse_chunk() fails

[Christian Couder]

On Wed, Mar 16, 2016 at 3:35 PM, Christian Couder
 wrote:
> When parse_chunk() fails it can return -1, for example
> when find_header() doesn't find a patch header.
>
> In this case it's better in apply_patch() to free the
> "struct patch" that we just allocated instead of
> leaking it.

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

Re: [PATCH v3] builtin/apply: handle parse_binary() failure

[Christian Couder]

On Fri, Mar 18, 2016 at 1:30 PM, Christian Couder
 wrote:
> In parse_binary() there is:
>
> forward = parse_binary_hunk(, , , );
> if (!forward && !status)
> /* there has to be one hunk (forward hunk) */
> return error(_("unrecognized binary patch at line %d"), 
> linenr-1);
>
> so parse_binary() can return -1, because that's what error() returns.
>
> Also parse_binary_hunk() sets "status" to -1 in case of error and
> parse_binary() does "if (status) return status;".
>
> In this case parse_chunk() should not add -1 to the patchsize it computes.
> It is better for future libification efforts to make it just return -1.
>
> Signed-off-by: Christian Couder 
> ---
> Only the title of the patch changed in this version compared to v2.

It looks like this patch is not in pu. Maybe it has fallen through the cracks?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv2 4/5] submodule--helper, module_clone: always operate on absolute paths

[Junio C Hamano]

Stefan Beller  writes:

> + if (!is_absolute_path(sm_gitdir)) {
> + char *cwd = xgetcwd();
> + strbuf_addf(, "%s/%s", cwd, sm_gitdir);
> + sm_gitdir = strbuf_detach(, 0);
> + free(cwd);

It would be surprising that this would be the first codepath that
needs to get an absolute pathname in the codebase that is more than
10 years old, wouldn't it?  Don't we have a reasonable API helper
function to do this kind of thing already?

... goes and looks ...

Doesn't absolute_path() work here?

> @@ -221,7 +240,6 @@ static int module_clone(int argc, const char **argv, 
> const char *prefix)
>   submodule_dot_git = fopen(sb.buf, "w");
>   if (!submodule_dot_git)
>   die_errno(_("cannot open file '%s'"), sb.buf);
> -
>   fprintf(submodule_dot_git, "gitdir: %s\n",
>   relative_path(sm_gitdir, path, _path));
>   if (fclose(submodule_dot_git))

Looks like an unrelated change to me.

> @@ -229,24 +247,16 @@ static int module_clone(int argc, const char **argv, 
> const char *prefix)
>   strbuf_reset();
>   strbuf_reset(_path);
>  
> - cwd = xgetcwd();
>   /* Redirect the worktree of the submodule in the superproject's config 
> */
> - if (!is_absolute_path(sm_gitdir)) {
> - strbuf_addf(, "%s/%s", cwd, sm_gitdir);
> - free(sm_gitdir);
> - sm_gitdir = strbuf_detach(, NULL);
> - }
> -
> - strbuf_addf(, "%s/%s", cwd, path);
>   p = git_pathdup_submodule(path, "config");
>   if (!p)
>   die(_("could not get submodule directory for '%s'"), path);
>   git_config_set_in_file(p, "core.worktree",
> -relative_path(sb.buf, sm_gitdir, _path));
> +relative_path(path, sm_gitdir, _path));
>   strbuf_release();
>   strbuf_release(_path);
>   free(sm_gitdir);
> - free(cwd);
> +

This addition of blank, too.

>   free(p);
>   return 0;
>  }
> diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
> index fc11809..ea3fabb 100755
> --- a/t/t7400-submodule-basic.sh
> +++ b/t/t7400-submodule-basic.sh
> @@ -818,7 +818,7 @@ test_expect_success 'submodule add --name allows to 
> replace a submodule with ano
>   )
>  '
>  
> -test_expect_failure 'recursive relative submodules stay relative' '
> +test_expect_success 'recursive relative submodules stay relative' '
>   test_when_finished "rm -rf super clone2 subsub sub3" &&
>   mkdir subsub &&
>   (
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 00/21] replacement for dt/refs-backend-lmdb v7 patch 04/33

[David Turner]

On Thu, 2016-03-31 at 18:14 +0200, Michael Haggerty wrote:
> On 03/30/2016 10:05 PM, David Turner wrote:
> > On Wed, 2016-03-30 at 08:37 +0200, Michael Haggerty wrote:
> > > On 03/29/2016 10:12 PM, David Turner wrote:
> > > > On Sun, 2016-03-27 at 07:22 +0200, Michael Haggerty wrote:
> > > > > On 03/24/2016 07:47 AM, David Turner wrote:
> > > > > > [...]
> > > > > > I incorporated your changes into the lmdb backend.  To make
> > > > > > merging
> > > > > > later more convenient, I rebased on top of pu -- I think
> > > > > > this
> > > > > > mainly
> > > > > > depends on jk/check-repository-format, but I also included
> > > > > > some
> > > > > > fixes
> > > > > > for a couple of tests that had been changed by other
> > > > > > patches.
> > > > > 
> > > > > I think rebasing changes on top of pu is counterproductive. I
> > > > > believe
> > > > > that Junio had extra work rebasing your earlier series onto a
> > > > > merge
> > > > > of
> > > > > the minimum number of topics that it really depended on.
> > > > > There is
> > > > > no
> > > > > way
> > > > > that he could merge the branch in this form because it would
> > > > > imply
> > > > > merging all of pu.
> > > > > 
> > > > > See the zeroth section of SubmittingPatches [1] for the
> > > > > guidelines.
> > > > 
> > > > I'm a bit confused because 
> > > > [PATCH 18/21] get_default_remote(): remove unneeded flag
> > > > variable
> > > > 
> > > > doesn't do anything on master -- it depends on some patch in
> > > > pu. 
> > > >  And
> > > > we definitely want to pick up jk/check-repository-format (which
> > > > doesn't
> > > > include whatever 18/21 depends on).
> > > > 
> > > > So what do you think our base should be?
> > > 
> > > I think the preference is to base a patch series on the merge of
> > > master
> > > plus the minimum number of topics in pu (ideally, none) that are
> > > "essential" prerequisites of the changes in the patch series. For
> > > example, the version of this patch series that Junio has in his
> > > tree
> > > was
> > > based on master + sb/submodule-parallel-update. 
> > > 
> > > Even if there are minor
> > > conflicts with another in-flight topic, it is easier for Junio to
> > > resolve the conflicts when merging the topics together than to
> > > rebase
> > > the patch series over and over as the other patch series evolves.
> > > The
> > > goal of this practice is of course to allow patch series to
> > > evolve
> > > independently of each other as much as possible.
> > > 
> > > Of course if you have insights into nontrivial conflicts between
> > > your
> > > patch series and others, it would be helpful to discuss these in
> > > your
> > > cover letter.
> > 
> > If I am reading this correctly, it looks like your series also has
> > a
> > few more sb submodule patches, e.g. sb/submodule-init, which is
> > responsible for the code that 18/21 depends on.  
> > 
> > I think jk/check-repository-format is also  good to get in first,
> > because it changes the startup sequence a bit and it's a bit tricky
> > to
> > figure out what needs to change in dt/refs-backend-lmdb as a result
> > of
> > it. 
> > 
> > But I can't just merge jk/check-repository-format on top of
> > 71defe0047 
> > -- some function signatures have changed in the run-command stuff
> > and
> > it seems kind of annoying to fix up.  
> > 
> > So I propose instead that we just drop 18/21 for now, and use just
> > jk/check-repository-format as the base.
> > 
> > Does this seem reasonable to you?
> 
> Yes, that's fine. Patch 18/21 is just a random cleanup that nothing
> else
> depends on. Will you do the rebasing? If so, please let me know where
> I
> can fetch the result from.

Rebased:

https://github.com/dturner-tw/git.git on branch
dturner/pluggable-backends


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

Re: [PATCHv2 3/5] submodule--helper clone: remove double path checking

[Junio C Hamano]

Eric Sunshine  writes:

> On Thu, Mar 31, 2016 at 5:04 PM, Stefan Beller  wrote:
>> submodule--helper clone: remove double path checking
>
> I think Junio mentioned in v1 that calling this "path checking" is misleading.

I'd tentatively use:

"submodule--helper clone: create the submodule path just once"

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

Re: [PATCH v4] git-send-pack: fix --all option when used with directory

[Junio C Hamano]

Junio C Hamano  writes:

> stanis...@assembla.com writes:
>
>> From: Stanislav Kolotinskiy 
>>
>> When using git send-pack with --all option
>> and a target repository specification ([:]),
>> usage message is being displayed instead of performing
>> the actual transmission.
>>
>> The reason for this issue is that destination and refspecs are being set
>> in the same conditional and are populated from argv. When a target
>> repository is passed, refspecs is being populated as well with its value.
>> This makes the check for refspecs not being NULL to always return true,
>> which, in conjunction with the check for --all or --mirror options,
>> is always true as well and returns usage message instead of proceeding.
>>
>> This ensures that send-pack will stop execution only when --all
>> or --mirror switch is used in conjunction with any refspecs passed.
>>
>> Signed-off-by: Stanislav Kolotinskiy 
>> ---
>
> Thanks, will queue.

By the way, for some reason it was unusually painful to find the
exact breakage by bisecting between maint-2.4 and maint-2.6.  It
somehow ended up on fingering random places like v2.6.0 itself.

The true culprit is 068c77a5 (builtin/send-pack.c: use parse_options
API, 2015-08-19).  I didn't dug deep enough to tell if we recently
broke "git bisect" or if there are something wrong in the shape of
my history.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv2 3/5] submodule--helper clone: remove double path checking

[Eric Sunshine]

On Thu, Mar 31, 2016 at 5:04 PM, Stefan Beller  wrote:
> submodule--helper clone: remove double path checking

I think Junio mentioned in v1 that calling this "path checking" is misleading.

> We make sure that the parent directory of path exists (or create it
> otherwise) and then do the same for path + "/.git".
>
> That is equivalent to just making sure that the parent directory of
> path + "/.git" exists (or create it otherwise).

This part of the commit message is nicely improved.

The patch itself looks fine.

> Signed-off-by: Stefan Beller 
> ---
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> @@ -215,11 +215,7 @@ static int module_clone(int argc, const char **argv, 
> const char *prefix)
> }
>
> /* Write a .git file in the submodule to redirect to the 
> superproject. */
> -   if (safe_create_leading_directories_const(path) < 0)
> -   die(_("could not create directory '%s'"), path);
> -
> strbuf_addf(, "%s/.git", path);
> -
> if (safe_create_leading_directories_const(sb.buf) < 0)
> die(_("could not create leading directories of '%s'"), 
> sb.buf);
> submodule_dot_git = fopen(sb.buf, "w");
> --
> 2.5.0.264.g39f00fe
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv2 2/5] submodule--helper clone: simplify path check

[Eric Sunshine]

On Thu, Mar 31, 2016 at 5:04 PM, Stefan Beller  wrote:
> submodule--helper clone: simplify path check

I don't see anything in the patch which simplifies a path check.
Instead, this version of the patch is now fixing a potential
NULL-dereference.

> The calling shell code makes sure that `path` is non null and non empty.
> Side note: it cannot be null as just three lines before it is passed
> to safe_create_leading_directories_const which would crash as you feed
> it null. That means the `else` part is dead code, so remove it.

The above description has very little if anything to do with what this
patch is addressing considering that this is now a crash-fix patch.
While it's true that the (current) shell code won't trigger this
crash, that's not particularly interesting or relevant.

> To make the code futureproof, add a check for path being NULL or empty
> and report the error accordingly.

Selling this as a future-proofing change is misleading; it's a crash
fix, plain and simple. I think the entire commit message could be
collapsed to something like this:

submodule--helper: fix potential NULL-dereference

Don't dereference NULL 'path' if it was never assigned. While
here also protect against empty --path argument.

You don't even need to mention removal of the conditional in the
second hunk since, for anyone reading the patch, that's an obvious
consequence of adding the new check in the first hunk.

The patch itself looks fine.

> Signed-off-by: Stefan Beller 
> ---
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> @@ -194,6 +194,9 @@ static int module_clone(int argc, const char **argv, 
> const char *prefix)
> argc = parse_options(argc, argv, prefix, module_clone_options,
>  git_submodule_helper_usage, 0);
>
> +   if (!path || !*path)
> +   die(_("submodule--helper: unspecified or empty --path"));
> +
> strbuf_addf(, "%s/modules/%s", get_git_dir(), name);
> sm_gitdir = strbuf_detach(, NULL);
>
> @@ -215,10 +218,7 @@ static int module_clone(int argc, const char **argv, 
> const char *prefix)
> if (safe_create_leading_directories_const(path) < 0)
> die(_("could not create directory '%s'"), path);
>
> -   if (path && *path)
> -   strbuf_addf(, "%s/.git", path);
> -   else
> -   strbuf_addstr(, ".git");
> +   strbuf_addf(, "%s/.git", path);
>
> if (safe_create_leading_directories_const(sb.buf) < 0)
> die(_("could not create leading directories of '%s'"), 
> sb.buf);
> --
> 2.5.0.264.g39f00fe
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3 1/4] notes: don't leak memory in git_config_get_notes_strategy

[Eric Sunshine]

On Thu, Mar 31, 2016 at 2:04 PM, Stefan Beller  wrote:
> `value` is just a temporary scratchpad, so we need to make sure it doesn't
> leak. It is xstrdup'd in `git_config_get_string_const` and
> `parse_notes_merge_strategy` just compares the string against predefined
> values, so no need to keep it around longer. Instead of using
> `git_config_get_string_const`, use `git_config_get_value`, which doesn't
> return a copy.
>
> Signed-off-by: Stefan Beller 
> ---
> diff --git a/builtin/notes.c b/builtin/notes.c
> @@ -746,7 +746,7 @@ static int git_config_get_notes_strategy(const char *key,
>  {
> const char *value;
>
> -   if (git_config_get_string_const(key, ))
> +   if (git_config_get_value(key, ))

Hmm, doesn't this introduce a rather severe regression? Unless I'm
misreading the code (possible), with the original, if 'key' was
boolean (lacked a value in the config file), then it would complain:

Missing value for 'floop.blork'

but, with this change, it will dereference NULL and crash.

(My understanding was that Peff's suggestion to use
git_config_get_value() implied a bit of work beyond the simple textual
substitution of 'git_config_get_value' for
'git_config_get_string_const'.)

> return 1;
> if (parse_notes_merge_strategy(value, strategy))
> git_die_config(key, "unknown notes merge strategy %s", value);
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCHv2 5/5] submodule--helper, module_clone: catch fprintf failure

[Stefan Beller]

The return value of fprintf is unchecked, which may lead to
unreported errors. Use fprintf_or_die to report the error to the user.

Signed-off-by: Stefan Beller 
---
 builtin/submodule--helper.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 56c3033..89cbbda 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -240,8 +240,8 @@ static int module_clone(int argc, const char **argv, const 
char *prefix)
submodule_dot_git = fopen(sb.buf, "w");
if (!submodule_dot_git)
die_errno(_("cannot open file '%s'"), sb.buf);
-   fprintf(submodule_dot_git, "gitdir: %s\n",
-   relative_path(sm_gitdir, path, _path));
+   fprintf_or_die(submodule_dot_git, "gitdir: %s\n",
+  relative_path(sm_gitdir, path, _path));
if (fclose(submodule_dot_git))
die(_("could not close file %s"), sb.buf);
strbuf_reset();
-- 
2.5.0.264.g39f00fe

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

[PATCHv2 0/5] Fix relative path issues in recursive submodules.

[Stefan Beller]

Thanks Junio, Eric and Jacob for review!

v2:
 * reworded commit message for test (Thanks Junio!)
 * instead of "simplifying" the path handling in patch 2, we are prepared
   for anything the user throws at us (i.e. instead of segfault we
   die(_("submodule--helper: unspecified or empty --path"));
   (Thanks Eric, Jacob for pushing back here!)
 * reword commit message for patch 3 (safe_create_leading_directories_const is
   not a check, Thanks Junio!)
 * patch 4 (the fix): That got reworked completely. No flow controlling
   conditions in the write out phase!
 * patch 5 is a bonus! (replace fprintf by fprintf_or die) When implementing
   that I wondered if we also have close_or_die and open_or_die, but that 
doesn't
   seem to be the case.
   
Thanks,
Stefan

v1:

It took me longer than expected to fix this bug.

It comes with a test and minor refactoring and applies on top of
origin/sb/submodule-helper, such that it can be merged into 2.7, 2.8 as well
as master.

Patch 1 is a test which fails; it has a similar layout as the
real failing repository Norio Nomura pointed out, but simplified to the
bare essentials for reproduction of the bug.

Patch 2&3 are not strictly necessary for fixing the isseu, but it removes
stupid code I wrote, so the resulting code looks a little better.

Patch 4 fixes the issue by giving more information to relative_path,
such that a relative path can be found in all cases.

Thanks,
Stefan

Stefan Beller (5):
  recursive submodules: test for relative paths
  submodule--helper clone: simplify path check
  submodule--helper clone: remove double path checking
  submodule--helper, module_clone: always operate on absolute paths
  submodule--helper, module_clone: catch fprintf failure

 builtin/submodule--helper.c | 52 +
 t/t7400-submodule-basic.sh  | 41 +++
 2 files changed, 70 insertions(+), 23 deletions(-)

-- 
2.5.0.264.g39f00fe

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

[PATCHv2 1/5] recursive submodules: test for relative paths

[Stefan Beller]

"git submodule update --init --recursive" uses full path to refer to
the true location of the repository in the "gitdir:" pointer for
nested submodules; the command used to use relative paths.

This was reported by Norio Nomura in $gmane/290280.

The root cause for that bug is in using recursive submodules as
their relative path handling was broken in ee8838d (2015-09-08,
submodule: rewrite `module_clone` shell function in C).

Signed-off-by: Stefan Beller 
---
 t/t7400-submodule-basic.sh | 41 +
 1 file changed, 41 insertions(+)

diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index 540771c..fc11809 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -818,6 +818,47 @@ test_expect_success 'submodule add --name allows to 
replace a submodule with ano
)
 '
 
+test_expect_failure 'recursive relative submodules stay relative' '
+   test_when_finished "rm -rf super clone2 subsub sub3" &&
+   mkdir subsub &&
+   (
+   cd subsub &&
+   git init &&
+   >t &&
+   git add t &&
+   git commit -m "initial commit"
+   ) &&
+   mkdir sub3 &&
+   (
+   cd sub3 &&
+   git init &&
+   >t &&
+   git add t &&
+   git commit -m "initial commit" &&
+   git submodule add ../subsub dirdir/subsub &&
+   git commit -m "add submodule subsub"
+   ) &&
+   mkdir super &&
+   (
+   cd super &&
+   git init &&
+   >t &&
+   git add t &&
+   git commit -m "initial commit" &&
+   git submodule add ../sub3 &&
+   git commit -m "add submodule sub"
+   ) &&
+   git clone super clone2 &&
+   (
+   cd clone2 &&
+   git submodule update --init --recursive &&
+   echo "gitdir: ../.git/modules/sub3" >./sub3/.git_expect &&
+   echo "gitdir: ../../../.git/modules/sub3/modules/dirdir/subsub" 
>./sub3/dirdir/subsub/.git_expect
+   ) &&
+   test_cmp clone2/sub3/.git_expect clone2/sub3/.git &&
+   test_cmp clone2/sub3/dirdir/subsub/.git_expect 
clone2/sub3/dirdir/subsub/.git
+'
+
 test_expect_success 'submodule add with an existing name fails unless forced' '
(
cd addtest2 &&
-- 
2.5.0.264.g39f00fe

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

[PATCHv2 4/5] submodule--helper, module_clone: always operate on absolute paths

[Stefan Beller]

When giving relative paths to `relative_path` to compute a relative path
from one directory to another, this may fail in `relative_path`.
Make sure both arguments to `relative_path` are always absolute.

Signed-off-by: Stefan Beller 
---

Notes:
Notice how the 2 relative path calls
  relative_path(sm_gitdir, path, _path)
  relative_path(path, sm_gitdir, _path)
now have the same arguments, just switched?
The symmetry there looks nice to read.

This proposal is slightly more code than the previous patch,
but looking at the end result

 builtin/submodule--helper.c | 36 +++-
 t/t7400-submodule-basic.sh  |  2 +-
 2 files changed, 24 insertions(+), 14 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 0b9f546..56c3033 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -157,8 +157,8 @@ static int module_clone(int argc, const char **argv, const 
char *prefix)
const char *reference = NULL, *depth = NULL;
int quiet = 0;
FILE *submodule_dot_git;
-   char *sm_gitdir, *cwd, *p;
-   struct strbuf rel_path = STRBUF_INIT;
+   char *sm_gitdir, *p;
+   struct strbuf rel_path = STRBUF_INIT; /* for relative_path */
struct strbuf sb = STRBUF_INIT;
 
struct option module_clone_options[] = {
@@ -200,6 +200,25 @@ static int module_clone(int argc, const char **argv, const 
char *prefix)
strbuf_addf(, "%s/modules/%s", get_git_dir(), name);
sm_gitdir = strbuf_detach(, NULL);
 
+
+   if (!is_absolute_path(sm_gitdir)) {
+   char *cwd = xgetcwd();
+   strbuf_addf(, "%s/%s", cwd, sm_gitdir);
+   sm_gitdir = strbuf_detach(, 0);
+   free(cwd);
+   }
+
+   if (!is_absolute_path(path)) {
+   /*
+* TODO: add prefix here once we allow calling from non root
+* directory?
+*/
+   strbuf_addf(, "%s/%s",
+   get_git_work_tree(),
+   path);
+   path = strbuf_detach(, 0);
+   }
+
if (!file_exists(sm_gitdir)) {
if (safe_create_leading_directories_const(sm_gitdir) < 0)
die(_("could not create directory '%s'"), sm_gitdir);
@@ -221,7 +240,6 @@ static int module_clone(int argc, const char **argv, const 
char *prefix)
submodule_dot_git = fopen(sb.buf, "w");
if (!submodule_dot_git)
die_errno(_("cannot open file '%s'"), sb.buf);
-
fprintf(submodule_dot_git, "gitdir: %s\n",
relative_path(sm_gitdir, path, _path));
if (fclose(submodule_dot_git))
@@ -229,24 +247,16 @@ static int module_clone(int argc, const char **argv, 
const char *prefix)
strbuf_reset();
strbuf_reset(_path);
 
-   cwd = xgetcwd();
/* Redirect the worktree of the submodule in the superproject's config 
*/
-   if (!is_absolute_path(sm_gitdir)) {
-   strbuf_addf(, "%s/%s", cwd, sm_gitdir);
-   free(sm_gitdir);
-   sm_gitdir = strbuf_detach(, NULL);
-   }
-
-   strbuf_addf(, "%s/%s", cwd, path);
p = git_pathdup_submodule(path, "config");
if (!p)
die(_("could not get submodule directory for '%s'"), path);
git_config_set_in_file(p, "core.worktree",
-  relative_path(sb.buf, sm_gitdir, _path));
+  relative_path(path, sm_gitdir, _path));
strbuf_release();
strbuf_release(_path);
free(sm_gitdir);
-   free(cwd);
+
free(p);
return 0;
 }
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index fc11809..ea3fabb 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -818,7 +818,7 @@ test_expect_success 'submodule add --name allows to replace 
a submodule with ano
)
 '
 
-test_expect_failure 'recursive relative submodules stay relative' '
+test_expect_success 'recursive relative submodules stay relative' '
test_when_finished "rm -rf super clone2 subsub sub3" &&
mkdir subsub &&
(
-- 
2.5.0.264.g39f00fe

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

[PATCHv2 3/5] submodule--helper clone: remove double path checking

[Stefan Beller]

We make sure that the parent directory of path exists (or create it
otherwise) and then do the same for path + "/.git".

That is equivalent to just making sure that the parent directory of
path + "/.git" exists (or create it otherwise).

Signed-off-by: Stefan Beller 
---
 builtin/submodule--helper.c | 4 
 1 file changed, 4 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 4f0b002..0b9f546 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -215,11 +215,7 @@ static int module_clone(int argc, const char **argv, const 
char *prefix)
}
 
/* Write a .git file in the submodule to redirect to the superproject. 
*/
-   if (safe_create_leading_directories_const(path) < 0)
-   die(_("could not create directory '%s'"), path);
-
strbuf_addf(, "%s/.git", path);
-
if (safe_create_leading_directories_const(sb.buf) < 0)
die(_("could not create leading directories of '%s'"), sb.buf);
submodule_dot_git = fopen(sb.buf, "w");
-- 
2.5.0.264.g39f00fe

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

Re: [PATCH v2] clone: respect configured fetch respecs during initial fetch

[SZEDER Gábor]

Quoting Junio C Hamano :


IOW, special casing -c remote.origin.fetch=spec
is a bad idea.


I completely agree :)
But it's the other way around.

'remote.origin.fetch=spec' during clone is special _now_, because the
initial fetch ignores it, no matter where it is set.

My patch makes it non-special, so that the initial fetch respects it,
the same way it already respects 'fetch.fsckObjects' and
'fsck.unpackLimit', or the way the initial checkout respects e.g.
'core.eol'.


So how about teaching "git clone" a new _option_ that is about what
branches are followed?

git clone $there --branches="master next pu"

would give

[remote "origin"]
fetch = +refs/heads/master:refs/remotes/origin/master
fetch = +refs/heads/next:refs/remotes/origin/next
fetch = +refs/heads/pu:refs/remotes/origin/pu


Without my patch the initial fetch would ignore these refspecs, too.


instead of the usual

[remote "origin"]
fetch = +refs/heads/*:refs/remotes/origin/*


Typing only branch names is much shorter and simpler than typing the
name of a config var and full refspecs, so this would be a nice
simplification of the UI for the case when the user is only
interested in certain branches.  But it wouldn't help if the user
wants to include 'refs/interesting/*' in the initial fetch.



And that can be made to work orthognonal to --single-branch by a
small additional rule: if the branch given by -b  (or their
HEAD) is not part of --branches, then we add it to the set of
branches to be followed (i.e. if you give only --single-branch,
without --branches, the set of branches to be followed will become
that single branch).

Hmm?



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

Re: [PATCH v11 2/4] parse-options.c: make OPTION_COUNTUP respect "unspecified" values

[Junio C Hamano]

Pranit Bauva  writes:

> Also, when such an idea for new feature comes up, it is better to
> implement it because let's say some developer is stuck in future and
> this new feature could help him but he doesn't have a clue that such a
> discussion happened some time ago. Thus saving him time and further
> effort.
>
> Anyways, How is the convention in git for these type of futuristic ideas?

Just keep it in mind without complicating the code, unless the idea
truly makes sense and has immediate use.

For example, I do not know how it would be useful to be able to
count up starting from 2 with an option --more that is COUNTUP, if
your --no-more would reset it to 0.

 git cmd --more ;# sets more=2
 git cmd --more --more ;# sets more=3
 git cmd --no-more --more ;# sets more=1, not more=2


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

Re: [PATCH v11 2/4] parse-options.c: make OPTION_COUNTUP respect "unspecified" values

[Pranit Bauva]

On Fri, Apr 1, 2016 at 1:36 AM, Junio C Hamano  wrote:
> Pranit Bauva  writes:
>
>> On Fri, Apr 1, 2016 at 12:11 AM, Junio C Hamano  wrote:
>>>
>>> case OPTION_COUNTUP:
>>> +   if (*(int *)opt->value < 0)
>>> +   *(int *)opt->value = 0;
>>> *(int *)opt->value = unset ? 0 : *(int *)opt->value + 1;
>>>
>>> That is, upon hitting this case arm, we know that an explicit option
>>> was given from the command line, so the "unspecified" initial value,
>>> if it is still there, gets reset to 0, and after doing that, we just
>>> do the usual thing.
>>
>> This does look cleaner. Nice thinking that there is no need to
>> actually specify when it gets 0. It gets 0 no matter what as long as
>> OPTION_COUNTUP is speficied in any format (with or without --no) and
>> variable is "unspecified".
>
> I do not think there is any planned users of such an enhancement,
> but the above points us into a future possibility to be able to do
> this:
>
> case OPTION_COUNTUP:
> +   if (*(int *)opt->value < 0)
> +   *(int *)opt->value = -*(int *)opt->value - 1;
> *(int *)opt->value = unset ? 0 : *(int *)opt->value + 1;
>
> That is, by using "-2" as the "unspecified" value, you can start
> counting up from 2 (i.e. the presence of the option resets the
> variable to 1 and then the option being not "unset" increments it)
> if your application wants to.

I am not very familiar with Git community but I think including a new
feature to use our existing library (who's functionality isn't
required as for now) can trigger some creative thoughts in minds of
other developers which will make them think "How could this be used?".
This could lead to some exciting ideas on improving the UI of git.
Having something actually in hand gives you a confidence, rather than
knowing that you could grab it whenever it is needed.

Also, when such an idea for new feature comes up, it is better to
implement it because let's say some developer is stuck in future and
this new feature could help him but he doesn't have a clue that such a
discussion happened some time ago. Thus saving him time and further
effort.

Anyways, How is the convention in git for these type of futuristic ideas?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: BUG in git diff-index

[Junio C Hamano]

Andy Lowry  writes:

> So I think now that the script should do "update-index --refresh"
> followed by "diff-index --quiet HEAD". Sound correct?

Yes.  That has always been one of the kosher ways for any script to
make sure that the files in the working tree that are tracked have
not been modified relative to HEAD (assuming that the index matches
HEAD).  If you are fuzzy about that assumption, you would also do
"diff-index --quiet --cached HEAD" to ensure it, making the whole
thing:

update-index --refresh
diff-index --quiet --cached HEAD && diff-index --quiet HEAD

Our scripts traditionally do the equivalent in a slightly different
way.  require_clean_work_tree() in git-sh-setup makes sure that (1)
your working tree files match what is in your index and that (2)
your index matches the HEAD, i.e.

update-index --refresh
diff-files --quiet && diff-index --cached --quiet HEAD

They are equivalent in that H==I && H==W (yours) mean H==I==W, while
I==W && H==I (ours) also mean H==I==W.  Two diff-index would require
you to open the tree object of the HEAD twice, so our version may be
more efficient but you probably wouldn't be able to measure the
difference.

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

Re: [PATCH v4] git-send-pack: fix --all option when used with directory

[Junio C Hamano]

stanis...@assembla.com writes:

> From: Stanislav Kolotinskiy 
>
> When using git send-pack with --all option
> and a target repository specification ([:]),
> usage message is being displayed instead of performing
> the actual transmission.
>
> The reason for this issue is that destination and refspecs are being set
> in the same conditional and are populated from argv. When a target
> repository is passed, refspecs is being populated as well with its value.
> This makes the check for refspecs not being NULL to always return true,
> which, in conjunction with the check for --all or --mirror options,
> is always true as well and returns usage message instead of proceeding.
>
> This ensures that send-pack will stop execution only when --all
> or --mirror switch is used in conjunction with any refspecs passed.
>
> Signed-off-by: Stanislav Kolotinskiy 
> ---

Thanks, will queue.

>  builtin/send-pack.c  |  2 +-
>  t/t5400-send-pack.sh | 12 
>  2 files changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/send-pack.c b/builtin/send-pack.c
> index f6e5d64..19f0577 100644
> --- a/builtin/send-pack.c
> +++ b/builtin/send-pack.c
> @@ -225,7 +225,7 @@ int cmd_send_pack(int argc, const char **argv, const char 
> *prefix)
>* --all and --mirror are incompatible; neither makes sense
>* with any refspecs.
>*/
> - if ((refspecs && (send_all || args.send_mirror)) ||
> + if ((nr_refspecs > 0 && (send_all || args.send_mirror)) ||
>   (send_all && args.send_mirror))
>   usage_with_options(send_pack_usage, options);
>
> diff --git a/t/t5400-send-pack.sh b/t/t5400-send-pack.sh
> index 04cea97..305ca7a 100755
> --- a/t/t5400-send-pack.sh
> +++ b/t/t5400-send-pack.sh
> @@ -128,6 +128,18 @@ test_expect_success 'denyNonFastforwards trumps --force' 
> '
>   test "$victim_orig" = "$victim_head"
>  '
>
> +test_expect_success 'send-pack --all sends all branches' '
> + # make sure we have at least 2 branches with different
> + # values, just to be thorough
> + git branch other-branch HEAD^ &&
> +
> + git init --bare all.git &&
> + git send-pack --all all.git &&
> + git for-each-ref refs/heads >expect &&
> + git -C all.git for-each-ref refs/heads >actual &&
> + test_cmp expect actual
> +'
> +
>  test_expect_success 'push --all excludes remote-tracking hierarchy' '
>   mkdir parent &&
>   (
> --
> 2.8.0
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: RFC: New reference iteration paradigm

[David Turner]

On Thu, 2016-03-31 at 18:13 +0200, Michael Haggerty wrote:
> Currently the way to iterate over references is via a family of
> for_each_ref()-style functions. You pass some arguments plus a
> callback
> function and cb_data to the function, and your callback is called for
> each reference that is selected.
> 
> This works, but it has two big disadvantages:
> 
> 1. It is cumbersome for callers. The caller's logic has to be split
>into two functions, the one that calls for_each_ref() and the
>callback function. Any data that have to be passed between the
>functions has to be stuck in a separate data structure.
> 
> 2. This interface is not composable. For example, you can't write a
>single function that iterates over references from two sources,
>as is interesting for combining packed plus loose references,
>shared plus worktree-specific references, symbolic plus normal
>references, etc. The current code for combining packed and loose
>references needs to walk the two reference trees in lockstep,
>using intimate knowledge about how references are stored [1,2,3].
> 
> I'm currently working on a patch series to transition the reference
> code
> from using for_each_ref()-style iteration to using proper iterators.
> 
> The main point of this change is to change the base iteration
> paradigm
> that has to be supported by reference backends. So instead of
> 
> > int do_for_each_ref_fn(const char *submodule, const char *base,
> >each_ref_fn fn, int trim, int flags,
> >void *cb_data);
> 
> the backend now has to implement
> 
> > struct ref_iterator *ref_iterator_begin_fn(const char *submodule,
> >const char *prefix,
> >unsigned int flags);
> 
> The ref_iterator itself has to implement two main methods:
> 
> > int iterator_advance_fn(struct ref_iterator *ref_iterator);
> > void iterator_free_fn(struct ref_iterator *ref_iterator);
> 
> A loop over references now looks something like
> 
> > struct ref_iterator *iter = each_ref_in_iterator("refs/tags/");
> > while (ref_iterator_advance(iter)) {
> > /* code using iter->refname, iter->oid, iter->flags */
> > }
> 
> I built quite a bit of ref_iterator infrastructure to make it easy to
> plug things together quite flexibly. For example, there is an
> overlay_ref_iterator which takes two other iterators (e.g., one for
> packed and one for loose refs) and overlays them, presenting the
> result
> via the same iterator interface. But the same overlay_ref_iterator
> can
> be used to overlay any two other iterators on top of each other.

I haven't looked at the code yet, but this makes sense to me.  In
general, the major reason to supply a callback style of API is when
iteration is more complicated than whatever will be consuming the data
(I can't remember where I heard this argument, but I found it pretty
convincing).  It seems like this is increasingly not the case, so we
should move towards the iterator style.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3 0/4] Some cleanups

[Junio C Hamano]

Jeff King  writes:

> Well, by polish up, I meant "write a commit message for". :)
>
> The patch itself looked fine to me.

I'll throw it in my "leftover bits" list.  After queuing 2/4, it now
needs a trivial adjustment to the patch text, which would be a good
spice for a new contributor who is looking for something a bit more
than "somebody did all the work and I can just resend it".

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

Re: RFC: New reference iteration paradigm

[Junio C Hamano]

Jeff King  writes:

> On Thu, Mar 31, 2016 at 11:01:44AM -0700, Junio C Hamano wrote:
>
>> Michael Haggerty  writes:
>> 
>> > the backend now has to implement
>> >
>> >> struct ref_iterator *ref_iterator_begin_fn(const char *submodule,
>> >>const char *prefix,
>> >>unsigned int flags);
>> >
>> > The ref_iterator itself has to implement two main methods:
>> >
>> >> int iterator_advance_fn(struct ref_iterator *ref_iterator);
>> >> void iterator_free_fn(struct ref_iterator *ref_iterator);
>> >
>> > A loop over references now looks something like
>> >
>> >> struct ref_iterator *iter = each_ref_in_iterator("refs/tags/");
>> >> while (ref_iterator_advance(iter)) {
>> >> /* code using iter->refname, iter->oid, iter->flags */
>> >> }
>> 
>> We'd want to take advantage of the tree-like organization of the
>> refs (i.e. refs/tags/a and refs/tags/b sit next to each other and
>> they are closer to each other than they are to refs/heads/a) so that
>> a request "I want to iterate only over tags, even though I may have
>> millions of other kinds of refs" can be done with cost that is
>> proportional to how many tags you have.
>> 
>> The current implementation of for_each_tag_ref() that goes down to
>> do_for_each_entry() in files-backend.c has that propertly, and the
>> new iteration mechanism with the above design seems to keep it,
>> which is very nice.
>
> Actually, that is a slight fiction. :)

I know.  My first draft had "(at least for the loose ref side)"
there, but I omitted it for brevity.

> We traverse only the loose ref directories we need, but we populate the
> entire packed-refs tree in one go.
> ...
> 800MB packed-refs file, as looking up one tiny subset of the entries
> wastes a lot of RAM and CPU pulling that into our internal
> representation[1].

Yes, that is an important use case that needs to be kept in mind for
any restructure of this machinery.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v11 2/4] parse-options.c: make OPTION_COUNTUP respect "unspecified" values

[Junio C Hamano]

Pranit Bauva  writes:

> On Fri, Apr 1, 2016 at 12:11 AM, Junio C Hamano  wrote:
>>
>> case OPTION_COUNTUP:
>> +   if (*(int *)opt->value < 0)
>> +   *(int *)opt->value = 0;
>> *(int *)opt->value = unset ? 0 : *(int *)opt->value + 1;
>>
>> That is, upon hitting this case arm, we know that an explicit option
>> was given from the command line, so the "unspecified" initial value,
>> if it is still there, gets reset to 0, and after doing that, we just
>> do the usual thing.
>
> This does look cleaner. Nice thinking that there is no need to
> actually specify when it gets 0. It gets 0 no matter what as long as
> OPTION_COUNTUP is speficied in any format (with or without --no) and
> variable is "unspecified".

I do not think there is any planned users of such an enhancement,
but the above points us into a future possibility to be able to do
this:

case OPTION_COUNTUP:
+   if (*(int *)opt->value < 0)
+   *(int *)opt->value = -*(int *)opt->value - 1;
*(int *)opt->value = unset ? 0 : *(int *)opt->value + 1;

That is, by using "-2" as the "unspecified" value, you can start
counting up from 2 (i.e. the presence of the option resets the
variable to 1 and then the option being not "unset" increments it)
if your application wants to.

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

Re: [PATCHv3 0/4] Some cleanups

[Jeff King]

On Thu, Mar 31, 2016 at 12:31:34PM -0700, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > With the exception of the comments on patch 3, these all look good. I'll
> > leave it to Junio to decide whether he wants to polish up his "get rid
> > of strbuf_split" patch for patch 2. Certainly yours is a strict
> > improvement over what is there.
> 
> I do not think there were anything further to be polished up.  Other
> than that, I agree with all of the above.

Well, by polish up, I meant "write a commit message for". :)

The patch itself looked fine to me.

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

Re: [PATCH v11 2/4] parse-options.c: make OPTION_COUNTUP respect "unspecified" values

[Pranit Bauva]

On Fri, Apr 1, 2016 at 12:11 AM, Junio C Hamano  wrote:
> Pranit Bauva  writes:
>
>> This change will not affect existing users of COUNTUP at all, as long as
>> they use the initial value of 0 (or more).
>>
>> Force uses the "unspecified" value in conjunction with OPT__FORCE in
>> builtin/clean.c in a different way to handle its config which
>> munges the "-1" into 0 before we get to parse_options.
>
> These two paragraphs leave the readers wondering what the conclusion
> is.  "it is OK as long as..." makes us wonder "so are there users
> that do not satisfy that condition?"  "in a different way" makes us
> wonder "Does this change break builtin/clean.c because COUNTUP is
> used in a different way?".
>
> This change does not affect existing users of COUNTUP,
> because they all use the initial value of 0 (or more).
>
> Note that builtin/clean.c initializes the variable used with
> OPT__FORCE (which uses OPT_COUNTUP()) to a negative value,
> but it is set to either 0 or 1 by reading the configuration
> before the code calls parse_options(), i.e. as far as
> parse_options() is concerned, the initial value of the
> variable is not negative.
>
> perhaps?

Thanks again for the help with the comit message. I sometimes fail to
see how first time readers will infer from the message.

>>  `OPT_COUNTUP(short, long, _var, description)`::
>>   Introduce a count-up option.
>> - `int_var` is incremented on each use of `--option`, and
>> - reset to zero with `--no-option`.
>> + Each use of `--option` increments `int_var`, starting from zero
>> + (even if initially negative), and `--no-option` resets it to
>> + zero. To determine if `--option` or `--no-option` was set at
>> + all, set `int_var` to a negative value, and if it is still
>> + negative after parse_options(), then neither `--option` nor
>> + `--no-option` was seen.
>>
>>  `OPT_BIT(short, long, _var, description, mask)`::
>>   Introduce a boolean option.
>> diff --git a/parse-options.c b/parse-options.c
>> index 47a9192..86d30bd 100644
>> --- a/parse-options.c
>> +++ b/parse-options.c
>> @@ -110,7 +110,13 @@ static int get_value(struct parse_opt_ctx_t *p,
>>   return 0;
>>
>>   case OPTION_COUNTUP:
>> - *(int *)opt->value = unset ? 0 : *(int *)opt->value + 1;
>> + if (unset)
>> + *(int *)opt->value = 0;
>> + else {
>> + if (*(int *)opt->value < 0)
>> + *(int *)opt->value = 0;
>> + *(int *)opt->value += 1;
>> + }
>>   return 0;
>>
>>   case OPTION_SET_INT:
>
> The new behaviour given by the patch looks very sensible, but I
> wonder if this is easier to reason about:
>
> case OPTION_COUNTUP:
> +   if (*(int *)opt->value < 0)
> +   *(int *)opt->value = 0;
> *(int *)opt->value = unset ? 0 : *(int *)opt->value + 1;
>
> That is, upon hitting this case arm, we know that an explicit option
> was given from the command line, so the "unspecified" initial value,
> if it is still there, gets reset to 0, and after doing that, we just
> do the usual thing.

This does look cleaner. Nice thinking that there is no need to
actually specify when it gets 0. It gets 0 no matter what as long as
OPTION_COUNTUP is speficied in any format (with or without --no) and
variable is "unspecified".
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: RFC: New reference iteration paradigm

[Jeff King]

On Thu, Mar 31, 2016 at 11:01:44AM -0700, Junio C Hamano wrote:

> Michael Haggerty  writes:
> 
> > the backend now has to implement
> >
> >> struct ref_iterator *ref_iterator_begin_fn(const char *submodule,
> >>const char *prefix,
> >>unsigned int flags);
> >
> > The ref_iterator itself has to implement two main methods:
> >
> >> int iterator_advance_fn(struct ref_iterator *ref_iterator);
> >> void iterator_free_fn(struct ref_iterator *ref_iterator);
> >
> > A loop over references now looks something like
> >
> >> struct ref_iterator *iter = each_ref_in_iterator("refs/tags/");
> >> while (ref_iterator_advance(iter)) {
> >> /* code using iter->refname, iter->oid, iter->flags */
> >> }
> 
> We'd want to take advantage of the tree-like organization of the
> refs (i.e. refs/tags/a and refs/tags/b sit next to each other and
> they are closer to each other than they are to refs/heads/a) so that
> a request "I want to iterate only over tags, even though I may have
> millions of other kinds of refs" can be done with cost that is
> proportional to how many tags you have.
> 
> The current implementation of for_each_tag_ref() that goes down to
> do_for_each_entry() in files-backend.c has that propertly, and the
> new iteration mechanism with the above design seems to keep it,
> which is very nice.

Actually, that is a slight fiction. :)

We traverse only the loose ref directories we need, but we populate the
entire packed-refs tree in one go. That's fine if you have a reasonable
number of refs and care about the cold-cache case (where you care a lot
about hitting directories you don't need to, but reading the entire
packed-refs file isn't a big deal). But it's really bad if you have an
800MB packed-refs file, as looking up one tiny subset of the entries
wastes a lot of RAM and CPU pulling that into our internal
representation[1].

At one point I wrote a patch to binary search the packed-refs file, find
the first "refs/tags/" entry, and then walk linearly through there. What
stopped me is that the current refs.c code (I guess file-backend.c these
days) was not happy with me partially filling in the ref_dir structs in
this "inside out" way.

That is, they assume we may have iterated "refs/", but not yet dove into
"refs/tags/" (because that's what makes sense for traversing loose
refs). But they are not happy if you already dove into "refs/tags/", but
don't know what else might be present in "refs/".

I wonder if, while Michael is fiddling with the iterator code, it might
be possible to fix that (it's not a fundamental problem, just one with
the way the ref-caching works right now).

-Peff

[1] As you probably guessed, I run into these giant packed-refs files as
part of our "alternates" storage for multiple forks of the same
repository. So all of the refs match the pattern
"refs/remotes/$fork_id/*", and I really would like to be able to
iterate the refs for just one fork at a time.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3 0/4] Some cleanups

[Junio C Hamano]

Jeff King  writes:

> With the exception of the comments on patch 3, these all look good. I'll
> leave it to Junio to decide whether he wants to polish up his "get rid
> of strbuf_split" patch for patch 2. Certainly yours is a strict
> improvement over what is there.

I do not think there were anything further to be polished up.  Other
than that, I agree with all of the above.

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

Re: BUG in git diff-index

[Andy Lowry]

OK, great. I think the update-index command is what I need.

If you'll indulge me, I'll describe my use-case in detail, and if you 
see anything screwy about it, I'd appreciate feedback. But don't feel 
obligated - you've been a great help already.


This is all about publishing updates to a static web site hosted as a 
gh-pages branch on a github repo.


Our master branch has everything that goes into building the site, and 
we use subtree push to update gh-pages with the embedded subtree that 
contains the generated site.


I'm creating a bash script that does the publishing, and as a general 
rule, we want to publish only from the master branch, and only if it's 
clean and up-to-date. And we also want to make sure that the embedded 
site reflects the current source content.


It's in that last requirement where diff-index comes in. The script runs 
a site build, and then should fail the policy check if that results in 
any changes to the working tree (including the embedded site)/. /I don't 
want the script to change the index in any way (e.g. so that unintended 
changes revealed by this policy check are less likely to be accidentally 
commited).


If I understand correctly, the update-index operation you indicated will 
not change index membership at all, but will simply resync the index 
members with actual working tree files.


So I think now that the script should do "update-index --refresh" 
followed by "diff-index --quiet HEAD". Sound correct?


Andy

On 3/31/2016 10:27 AM, Jeff King wrote:

On Thu, Mar 31, 2016 at 10:12:07AM -0400, Andy Lowry wrote:


What I'm actually after is a tree-to-filesystem comparison, regardless
of index. I've currently got a "diff" thrown in as a "work-around"
before "diff-index", but  now I understand it's not a workaround at
all. If there's a better way to achieve what I'm after, I'd appreciate
a tip. Otherwise I'll just change the comments explaining why there's
a "diff" in my script.

If your workaround is just to refresh the index, then you can do "git
update-index --refresh", rather than diff.

I don't think there is a plumbing command to do a direct
filesystem-to-tree comparison without having an index at all. "git diff
" claims in the documentation to do so, but besides not being
plumbing, I think it is really just doing the same thing as diff-index,
under the hood. The index is a pretty fundamental part of git's view of
the working tree.

-Peff


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

Re: [PATCH v3][Outreachy] branch -D: allow - as abbreviation of @{-1}

[Junio C Hamano]

Elena Petrashen  writes:

> @@ -214,6 +221,9 @@ static int delete_branches(int argc, const char **argv, 
> int force, int kinds,
>   const char *target;
>   int flags = 0;
>  
> + expand_dash_shortcut (argv, i);
> + if(!strncmp(argv[i], "@{-", strlen("@{-")))
> + at_shortcut = 1;
>   strbuf_branchname(, argv[i]);
>   if (kinds == FILTER_REFS_BRANCHES && !strcmp(head, bname.buf)) {
>   error(_("Cannot delete the branch '%s' "
> @@ -231,9 +241,12 @@ static int delete_branches(int argc, const char **argv, 
> int force, int kinds,
>   | RESOLVE_REF_ALLOW_BAD_NAME,
>   sha1, );
>   if (!target) {
> - error(remote_branch
> -   ? _("remote-tracking branch '%s' not found.")
> -   : _("branch '%s' not found."), bname.buf);
> + error((!strncmp(bname.buf, "@{-", strlen("@{-")))
> + ? _("There is not enough branch switches to"
> + " delete '%s'.")
> + : remote_branch
> + ? _("remote-tracking branch '%s' not 
> found.")
> + : _("branch '%s' not found."), 
> bname.buf);

I was expecting that the check for "@{-" in bname.buf would be done
immediately after strbuf_branchname(, argv[i]) we see in the
previous hunk (and an error message issued there), i.e. something
like:

orig_arg = argv[i];
if (!strcmp(orig_arg, "-"))
strbuf_branchname(, "@{-1}");
else
strbuf_branchname(, argv[i]);
if (starts_with(bname.buf, "@{-")) {
error("Not enough branch switches to delete %s", orig_arg);
... clean up and fail ...
}

That would give you sensible error message for "branch -d -",
"branch -d @{-1}" and "branch -d @{-4}" if you haven't visited
different branches enough times.

The hope was that the remainder of the code (including this error
message) would not have to worry about this "not enough switches"
error at all if done that way.

> @@ -262,6 +275,9 @@ static int delete_branches(int argc, const char **argv, 
> int force, int kinds,
>  (flags & REF_ISBROKEN) ? "broken"
>  : (flags & REF_ISSYMREF) ? target
>  : find_unique_abbrev(sha1, DEFAULT_ABBREV));
> + if (at_shortcut && advice_delete_branch_via_at_ref)
> +delete_branch_advice (bname.buf,
> + find_unique_abbrev(sha1, DEFAULT_ABBREV));
>   }

The existing !quiet report already said "deleted branch" with the
concrete branch name, not "@{-1}" or "-", taken from bname.buf at
this point.

If the advice on how to recover a deletion by mistake would help the
user, wouldn't that apply equally to the case where the user made a
typo in the original command line, i.e. "branch -d foo" when she
meant to delete "branch -d fooo", as well?  If we drop the "at_shortcut"
check from this if() statement, wouldn't the result be more helpful?

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

Re: [PATCH 4/4] submodule--helper: use relative path if possible

[Stefan Beller]

On Thu, Mar 31, 2016 at 9:59 AM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>> As submodules have working directory and their git directory far apart
>> relative_path(gitdir, work_dir) will not produce a relative path when
>> git_dir is absolute. When the gitdir is absolute, we need to convert
>> the workdir to an absolute path as well to compute the relative path.
>>
>> (e.g. gitdir=/home/user/project/.git/modules/submodule,
>> workdir=submodule/, relative_dir doesn't take the current working directory
>> into account, so there is no way for it to know that the correct answer
>> is indeed "../.git/modules/submodule", if the workdir was given as
>> /home/user/project/submodule, the answer is obvious.)
>>
>> Signed-off-by: Stefan Beller 
>> ---
>>  builtin/submodule--helper.c | 7 +++
>>  t/t7400-submodule-basic.sh  | 2 +-
>>  2 files changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
>> index 914e561..0b0fad3 100644
>> --- a/builtin/submodule--helper.c
>> +++ b/builtin/submodule--helper.c
>> @@ -159,6 +159,7 @@ static int module_clone(int argc, const char **argv, 
>> const char *prefix)
>>   FILE *submodule_dot_git;
>>   char *sm_gitdir, *cwd, *p;
>>   struct strbuf rel_path = STRBUF_INIT;
>> + struct strbuf abs_path = STRBUF_INIT;
>>   struct strbuf sb = STRBUF_INIT;
>>
>>   struct option module_clone_options[] = {
>> @@ -219,7 +220,12 @@ static int module_clone(int argc, const char **argv, 
>> const char *prefix)
>>   if (!submodule_dot_git)
>>   die_errno(_("cannot open file '%s'"), sb.buf);
>>
>> + strbuf_addf(_path, "%s/%s",
>> + get_git_work_tree(),
>> + path);
>
> The "path" is assumed to be _always_ relative to work tree?

In the code prior to this patch, that was assumed, yes.
(e.g. later in the code:

/* Redirect the worktree of the submodule in the superproject's config */
cwd = xgetcwd();
strbuf_addf(, "%s/%s", cwd, path);

relative_path(sb.buf, ...
)

>
> I am wondering if it would be prudent to have an assert for that
> before doing this, just like I suggested assert(path) for [2/4]
> earlier [*1*].
>
> On the other hand, if we allow path to be absolute, this would need
> to become something like:
>
> if (is_absolute_path(path))
> strbuf_addstr(_path, path);
> else
> strbuf_addf(_path, "%s/%s", get_git_work_tree(), path);
>
>>   fprintf(submodule_dot_git, "gitdir: %s\n",
>> + is_absolute_path(sm_gitdir) ?
>> + relative_path(sm_gitdir, abs_path.buf, _path) :
>>   relative_path(sm_gitdir, path, _path));
>
> It seems that the abs_path computation is not needed at all if
> sm_gitdir is relative to begin with.  I wonder if the code gets
> easier to read and can avoid unnecessary strbuf manipulation
> if this entire hunk is structured more like so:
>
> if (is_absolute_path(sm_gitdir)) {
> ...
> } else {
> ...
> }
> fprintf(submodule_dot_git, "gitdir: %s\n",
> relative_path(sm_gitdir, base_path, _path));
>
>>   if (fclose(submodule_dot_git))
>>   die(_("could not close file %s"), sb.buf);
>
> [Footnote]

I just simplified the code to be

  if (!is_absolute(path))
path=make_absolute(...);
  if (!is_absolute(sm_gitdir))
sm_gitdir=make_absolute(...);
  ...
 /* rest operates under absolute path only, no
   conditions any more! */


And I'd think that is cleanest to read and understand.

Having absolute path for both path and gitdir all the time
leaves no exceptions for relative_path to spew errors because
one of both is relative and not connected to the other absolute.

>
> *1* BTW, I tightened the assert for 2/4 to "assert(path && *path)"
> to match the assumption in its log message, i.e. "The calling shell
> code makes sure that path is non-NULL and non empty".
>
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3 0/4] Some cleanups

[Jeff King]

On Thu, Mar 31, 2016 at 11:04:02AM -0700, Stefan Beller wrote:

> v3:
> Thanks Eric, Jeff, Junio for discussion!
> * use git_config_get_value instead of git_config_get_string in patch 1
> * Improve commit message to explain why strbuf_list_free frees more memory
>   (hence is the right thing to do)
> * the bundle code doesn't have a dedicated return variable,
>   but the error path always returns -1  
> * removed a duplicate of
> + if (!bundle_to_stdout)
> + close(bundle_fd);
>   in the bundle patch.
> * This applies on v2.8.0.

With the exception of the comments on patch 3, these all look good. I'll
leave it to Junio to decide whether he wants to polish up his "get rid
of strbuf_split" patch for patch 2. Certainly yours is a strict
improvement over what is there.

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

Re: [PATCHv3 3/4] bundle: don't leak an fd in case of early return

[Jeff King]

On Thu, Mar 31, 2016 at 11:04:05AM -0700, Stefan Beller wrote:

> diff --git a/bundle.c b/bundle.c
> index 506ac49..31ae1da 100644
> --- a/bundle.c
> +++ b/bundle.c
> @@ -435,12 +435,14 @@ int create_bundle(struct bundle_header *header, const 
> char *path,
>  
>   /* write prerequisites */
>   if (compute_and_write_prerequisites(bundle_fd, , argc, argv))
> - return -1;
> + goto err;
>  
>   argc = setup_revisions(argc, argv, , NULL);
>  
> - if (argc > 1)
> - return error(_("unrecognized argument: %s"), argv[1]);
> + if (argc > 1) {
> + error(_("unrecognized argument: %s"), argv[1]);
> + goto err;
> + }
>  
>   object_array_remove_duplicates();
>  
> @@ -448,17 +450,22 @@ int create_bundle(struct bundle_header *header, const 
> char *path,
>   if (!ref_count)
>   die(_("Refusing to create empty bundle."));
>   else if (ref_count < 0)
> - return -1;
> + goto err;
>  
>   /* write pack */
>   if (write_pack_data(bundle_fd, ))
> - return -1;
> + goto err;

Sorry for not noticing this before, but if we assume write_pack_data
always closes bundle_fd, even on error (and I think it does), then the
close() in the "err" code path is redundant from this goto, right?

I guess it is harmless, as nobody could have opened a new descriptor in
the interim, so our close(bundle_fd) will just get EBADF. But I guess we
could also do:

  if (write_pack_data(bundle_fd, )) {
bundle_fd = -1;
goto err;
  }

or even:

  ret = write_pack_data(bundle_fd, );
  bundle_fd = -1; /* closed by write_pack_data */
  if (ret)
goto err;

to ensure that nobody (including the non-error code paths) uses it
again.

Like I said, I don't think it's buggy in the current code, but it does
seem a little fragile.

> +err:
> + if (!bundle_to_stdout)
> + close(bundle_fd);
> + rollback_lock_file();
> + return -1;

Similarly, I think the rollback_lock_file() call is redundant if
bundle_to_stdout is set (because we don't have created a lockfile in the
first place). I think this is more explicitly OK, because the lockfile
keeps an "am I initialized" flag, but perhaps sticking inside the "if
(!bundle_to_stdout)" condition makes it more clear that it's not an
error (especially because the "commit_lock_file" call above is inside
one).

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

Re: [PATCHv2 3/4] bundle: don't leak an fd in case of early return

[Philip Oakley]

From: "Stefan Beller" 

In successful operation `write_pack_data` will close the `bundle_fd`,
but when we exit early, we need to take care of the file descriptor
as well as the lock file ourselves. The lock file may be deleted at the
end of running the program, but we are in library code, so we should
not rely on that.

Helped-by: Jeff King 
Signed-off-by: Stefan Beller 


Has this been tested on Windows? I had a similar problem very recently
https://groups.google.com/forum/#!msg/git-for-windows/6LPxf9xZKhI/-s7XD18yCwAJ
where a bad rev-list-arg would cause the `bundle create` to die, and, on 
windows, leave the incomplete bundle file locked.


dscho suggested one possible solution for that, but I haven't had any time 
to try any patches.



---
bundle.c | 23 +--
1 file changed, 17 insertions(+), 6 deletions(-)

diff --git a/bundle.c b/bundle.c
index 506ac49..fbc8593 100644
--- a/bundle.c
+++ b/bundle.c
@@ -407,6 +407,7 @@ int create_bundle(struct bundle_header *header, const 
char *path,

 int bundle_to_stdout;
 int ref_count = 0;
 struct rev_info revs;
+ int ret = -1;

 bundle_to_stdout = !strcmp(path, "-");
 if (bundle_to_stdout)
@@ -435,30 +436,40 @@ int create_bundle(struct bundle_header *header, 
const char *path,


 /* write prerequisites */
 if (compute_and_write_prerequisites(bundle_fd, , argc, argv))
- return -1;
+ goto err;

 argc = setup_revisions(argc, argv, , NULL);

- if (argc > 1)
- return error(_("unrecognized argument: %s"), argv[1]);
+ if (argc > 1) {
+ ret = error(_("unrecognized argument: %s"), argv[1]);
+ goto err;
+ }

 object_array_remove_duplicates();

 ref_count = write_bundle_refs(bundle_fd, );
 if (!ref_count)
 die(_("Refusing to create empty bundle."));
- else if (ref_count < 0)
- return -1;
+ else if (ref_count < 0) {
+ if (!bundle_to_stdout)
+ close(bundle_fd);
+ goto err;
+ }

 /* write pack */
 if (write_pack_data(bundle_fd, ))
- return -1;
+ goto err;

 if (!bundle_to_stdout) {
 if (commit_lock_file())
 die_errno(_("cannot create '%s'"), path);
 }
 return 0;
+err:
+ if (!bundle_to_stdout)
+ close(bundle_fd);
+ rollback_lock_file();
+ return ret;
}

int unbundle(struct bundle_header *header, int bundle_fd, int flags)
--
2.8.0.2.gb331331

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



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

Re: Problem with Integrated Vim Editor on Win 10

[Junio C Hamano]

Zachary Turner  writes:

> I'm not terribly active in the Git community so I don't know what the
> procedure is for things like this, but this seems like a fairly
> serious regression.  Suggestions on how to proceed?

While the git-for-windows folks do read this list, git-for-windows
specific issues should be reported in its issue tracker on GitHub.

It looks like this issue has been reported already:
https://github.com/git-for-windows/git/issues/711
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] clone: respect configured fetch respecs during initial fetch

[Junio C Hamano]

Junio C Hamano  writes:

> Is the expectation like this?
>
> git init
> git config ... # set default configuration and origin remote
> git config var val # update with what "-c var=val" told us
> git fetch
> git checkout   # unless '--bare' is given
>
> or is it something else?
>
> Is "-c var=val" adding to the config variables set by default, or is
> it replacing them?  Does the choice depend on the nature of
> individual variables, and if so what is the criteria?
>
> Are all "-c var=val" update the configuration of the resulting
> repository?  Or are certain "var"s treated as special and placed in
> the config but not other "var"s?  If the latter, what makes these
> certain "var"s special?
>
> These design decisions need to be explained so that they will serve
> to guide people to decide what other variables to propagate and how
> when they have to add new configuration variables in the future.
> Otherwise we'd end up with an inconsistent mess.

The above did not start as rhetorical questions, but was merely me
thinking aloud.  However, it showed me a different approach might be
more appropriate.

Taken as rhetorical questions, the sane answers to them would
revolve around "we do not know the semantics of each and every
configuration variable that will be given to this codepath, and by
definition we will never know in advance the ones that will be
introduced later".  IOW, special casing -c remote.origin.fetch=spec
is a bad idea.

So how about teaching "git clone" a new _option_ that is about what
branches are followed?

git clone $there --branches="master next pu"

would give

[remote "origin"]
fetch = +refs/heads/master:refs/remotes/origin/master
fetch = +refs/heads/next:refs/remotes/origin/next
fetch = +refs/heads/pu:refs/remotes/origin/pu

instead of the usual

[remote "origin"]
fetch = +refs/heads/*:refs/remotes/origin/*

And that can be made to work orthognonal to --single-branch by a
small additional rule: if the branch given by -b  (or their
HEAD) is not part of --branches, then we add it to the set of
branches to be followed (i.e. if you give only --single-branch,
without --branches, the set of branches to be followed will become
that single branch).

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

[PATCH] t6302: fix up expect files for tests 34-36

[Ramsay Jones]

Signed-off-by: Ramsay Jones 
---

Hi Karthik,

If you need to re-roll the patches in your 'kn/ref-filter-branch-list'
branch, could you please squash this into the relevant patch (which
would be equivalent to commit 86cd4d32, "ref-filter: implement %(if),
%(then), and %(else) atoms", 30-03-2016).

It looks like these tests were written before Eric 'es/test-gpg-tags'
branch was applied.

Thanks!

ATB,
Ramsay Jones


 t/t6302-for-each-ref-filter.sh | 10 --
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
index 98a1c49..7420e48 100755
--- a/t/t6302-for-each-ref-filter.sh
+++ b/t/t6302-for-each-ref-filter.sh
@@ -349,6 +349,8 @@ test_expect_success 'check %(if)...%(then)...%(end) atoms' '
A U Thor: refs/heads/side
A U Thor: refs/odd/spot
 
+
+
A U Thor: refs/tags/foo1.10
A U Thor: refs/tags/foo1.3
A U Thor: refs/tags/foo1.6
@@ -367,7 +369,9 @@ test_expect_success 'check 
%(if)...%(then)...%(else)...%(end) atoms' '
A U Thor: refs/heads/master
A U Thor: refs/heads/side
A U Thor: refs/odd/spot
-   No author: refs/tags/double-tag
+   No author: refs/tags/annotated-tag
+   No author: refs/tags/doubly-annotated-tag
+   No author: refs/tags/doubly-signed-tag
A U Thor: refs/tags/foo1.10
A U Thor: refs/tags/foo1.3
A U Thor: refs/tags/foo1.6
@@ -385,7 +389,9 @@ test_expect_success 'ignore spaces in %(if) atom usage' '
master: Head ref
side: Not Head ref
odd/spot: Not Head ref
-   double-tag: Not Head ref
+   annotated-tag: Not Head ref
+   doubly-annotated-tag: Not Head ref
+   doubly-signed-tag: Not Head ref
foo1.10: Not Head ref
foo1.3: Not Head ref
foo1.6: Not Head ref
-- 
2.8.0
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Problem with Integrated Vim Editor on Win 10

[Philip Oakley]

From: "Zachary Turner" 

I dug into this some more, and as surprising as it is, I believe the
release of Git 2.8.0 is just busted.  I had an installer for 2.7.0
lying around, so after uninstalling 2.8.0 and re-installing 2.7.0,
everything works fine.

I'm not terribly active in the Git community so I don't know what the
procedure is for things like this, but this seems like a fairly
serious regression.  Suggestions on how to proceed?


see https://github.com/git-for-windows/git/issues/711#issuecomment-204003950
"Indeed, the culprit is git-for-windows/msys2-runtime@7346568 and reverting 
it fixes the issue. Will continue tomorrow." @dscho





On Wed, Mar 30, 2016 at 5:07 PM, Zachary Turner  
wrote:

Hi, just recently I installed the latest build of Windows 10 of my
machine. This is my second Win10 machine. On the other I am using git
2.7.0.windows.1 and everything is working just fine.

On the second machine I am using git 2.8.0.windows.1 and vim does not
work. I sent a bug report to b...@vim.org, but frankly I don't know whose
bug this is, so I'm including it here as well.

The problem is that vim is just a black screen when git launches it. If I
mash enough keys eventually I see something that resembles vim output at
the bottom, but I can't actually use it.

I tried going into program files\git\usr\bin and just running vim.exe.
Again, black screen. If I press enter about 10 times I can see the
introduction screen. Then if I press : about 10-20 times it will go into
command mode and a single : appears. after pressing a few more keys all
the rest of the :s appear. Basically, everything is completely unusable.

I tried downloading vim 7.4 from www.vim.org, and low and behold, it
works. For now I've replaced the copy of vim.exe that ships with git with
the copy from www.vim.org. But this leaves me nervous that something is
seriously wrong.

Has anyone seen anything like this before, or have any ideas how I might
better diagnose this?

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



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

Re: [PATCH v11 3/4] t7507-commit-verbose: improve test coverage by testing number of diffs

[Pranit Bauva]

On Thu, Mar 31, 2016 at 11:53 PM, Junio C Hamano  wrote:
> Pranit Bauva  writes:
>
>> Make the fake "editor" store output of grep in a file so that we can
>> see how many diffs were contained in the message and use them in
>> individual tests where ever it is required. Also use write_script()
>> to create the fake "editor".
>>
>> A subsequent commit will introduce scenarios where it is important to be
>> able to exactly determine how many diffs were present.
>>
>> Helped-by: Eric Sunshine 
>> Signed-off-by: Pranit Bauva 
>>
>> Previous version of this patch:
>>  - [v10]: $gmane/288820
>>
>> Changes this version wrt previous one:
>> I decided to include no of diffs in every test and rewrote the commit
>> message so as to sell this idea. This was given as an option to me by
>> Eric and the other option being to drop unnecessary testing of lines
>> where it isn't required. Also this patch uses a suggestion given by Eric
>> to make the "editor" look more clean as compared to the editor in my
>> previous version.
>> ---
>
> OK, by always exiting 0 from the editor, you do not interfere with
> the "git commit" that invoked it, and you inspect the editor's
> finding after "git commit" returns.  The approach taken by this
> patch looks a lot more sensible than the previous one.
>
> You'd need the three-dash right before "Previous version of..."
> line, though.

That's silly of me to forget this. Will do it.

>>  t/t7507-commit-verbose.sh | 16 +---
>>  1 file changed, 9 insertions(+), 7 deletions(-)
>>
>> diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh
>> index 2ddf28c..0f28a86 100755
>> --- a/t/t7507-commit-verbose.sh
>> +++ b/t/t7507-commit-verbose.sh
>> @@ -3,11 +3,10 @@
>>  test_description='verbose commit template'
>>  . ./test-lib.sh
>>
>> -cat >check-for-diff <> -#!$SHELL_PATH
>> -exec grep '^diff --git' "\$1"
>> +write_script "check-for-diff" <<\EOF &&
>> +grep '^diff --git' "$1" >out
>> +exit 0
>>  EOF
>> -chmod +x check-for-diff
>>  test_set_editor "$PWD/check-for-diff"
>>
>>  cat >message <<'EOF'
>> @@ -23,7 +22,8 @@ test_expect_success 'setup' '
>>  '
>>
>>  test_expect_success 'initial commit shows verbose diff' '
>> - git commit --amend -v
>> + git commit --amend -v &&
>> + test_line_count = 1 out
>>  '
>>
>>  test_expect_success 'second commit' '
>> @@ -39,13 +39,15 @@ check_message() {
>>
>>  test_expect_success 'verbose diff is stripped out' '
>>   git commit --amend -v &&
>> - check_message message
>> + check_message message &&
>> + test_line_count = 1 out
>>  '
>>
>>  test_expect_success 'verbose diff is stripped out (mnemonicprefix)' '
>>   git config diff.mnemonicprefix true &&
>>   git commit --amend -v &&
>> - check_message message
>> + check_message message &&
>> + test_line_count = 1 out
>>  '
>>
>>  cat >diff <<'EOF'
>>
>> --
>> https://github.com/git/git/pull/218
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v11 2/4] parse-options.c: make OPTION_COUNTUP respect "unspecified" values

[Junio C Hamano]

Pranit Bauva  writes:

> This change will not affect existing users of COUNTUP at all, as long as
> they use the initial value of 0 (or more).
>
> Force uses the "unspecified" value in conjunction with OPT__FORCE in
> builtin/clean.c in a different way to handle its config which
> munges the "-1" into 0 before we get to parse_options.

These two paragraphs leave the readers wondering what the conclusion
is.  "it is OK as long as..." makes us wonder "so are there users
that do not satisfy that condition?"  "in a different way" makes us
wonder "Does this change break builtin/clean.c because COUNTUP is
used in a different way?".

This change does not affect existing users of COUNTUP,
because they all use the initial value of 0 (or more).

Note that builtin/clean.c initializes the variable used with
OPT__FORCE (which uses OPT_COUNTUP()) to a negative value,
but it is set to either 0 or 1 by reading the configuration
before the code calls parse_options(), i.e. as far as
parse_options() is concerned, the initial value of the
variable is not negative.

perhaps?

>  `OPT_COUNTUP(short, long, _var, description)`::
>   Introduce a count-up option.
> - `int_var` is incremented on each use of `--option`, and
> - reset to zero with `--no-option`.
> + Each use of `--option` increments `int_var`, starting from zero
> + (even if initially negative), and `--no-option` resets it to
> + zero. To determine if `--option` or `--no-option` was set at
> + all, set `int_var` to a negative value, and if it is still
> + negative after parse_options(), then neither `--option` nor
> + `--no-option` was seen.
>  
>  `OPT_BIT(short, long, _var, description, mask)`::
>   Introduce a boolean option.
> diff --git a/parse-options.c b/parse-options.c
> index 47a9192..86d30bd 100644
> --- a/parse-options.c
> +++ b/parse-options.c
> @@ -110,7 +110,13 @@ static int get_value(struct parse_opt_ctx_t *p,
>   return 0;
>  
>   case OPTION_COUNTUP:
> - *(int *)opt->value = unset ? 0 : *(int *)opt->value + 1;
> + if (unset)
> + *(int *)opt->value = 0;
> + else {
> + if (*(int *)opt->value < 0)
> + *(int *)opt->value = 0;
> + *(int *)opt->value += 1;
> + }
>   return 0;
>  
>   case OPTION_SET_INT:

The new behaviour given by the patch looks very sensible, but I
wonder if this is easier to reason about:

case OPTION_COUNTUP:
+   if (*(int *)opt->value < 0)
+   *(int *)opt->value = 0;
*(int *)opt->value = unset ? 0 : *(int *)opt->value + 1;

That is, upon hitting this case arm, we know that an explicit option
was given from the command line, so the "unspecified" initial value,
if it is still there, gets reset to 0, and after doing that, we just
do the usual thing.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v11 1/4] test-parse-options: print quiet as integer

[Pranit Bauva]

On Thu, Mar 31, 2016 at 11:49 PM, Junio C Hamano  wrote:
> Pranit Bauva  writes:
>
>> Current implementation of parse-options.c treats OPT__QUIET() as integer
>> and not boolean and thus it is more appropriate to print it as integer
>> to avoid confusion.
>>
>> While at it, fix some style issues.
>
> I counted the changes in t0040 and you have _more_ style fixes than
> the real change.  That is not "while at it".
>
> While I welcome the style fix, it is better done as a preparatory
> clean-up step before the real change.

Okay. I thought this was a minor change so I squashed it together. I
will separate it though.

> Missing sign-off.

Will include this

>> -cat > typo.err << EOF
>> +cat >typo.err <>  error: did you mean \`--boolean\` (with two dashes ?)
>>  EOF
>
> If your "style cleanup" patch were separate, you could fix this (and
> other that have backslash escape inside the here-document) further
> to something like this:
>
> cat >type.err <<\EOF
> error: did you mean `--boolean` (with two dashes ?)
> EOF
>
> Thanks.

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

Re: [PATCH v11 3/4] t7507-commit-verbose: improve test coverage by testing number of diffs

[Junio C Hamano]

Pranit Bauva  writes:

> Make the fake "editor" store output of grep in a file so that we can
> see how many diffs were contained in the message and use them in
> individual tests where ever it is required. Also use write_script()
> to create the fake "editor".
>
> A subsequent commit will introduce scenarios where it is important to be
> able to exactly determine how many diffs were present.
>
> Helped-by: Eric Sunshine 
> Signed-off-by: Pranit Bauva 
>
> Previous version of this patch:
>  - [v10]: $gmane/288820
>
> Changes this version wrt previous one:
> I decided to include no of diffs in every test and rewrote the commit
> message so as to sell this idea. This was given as an option to me by
> Eric and the other option being to drop unnecessary testing of lines
> where it isn't required. Also this patch uses a suggestion given by Eric
> to make the "editor" look more clean as compared to the editor in my
> previous version.
> ---

OK, by always exiting 0 from the editor, you do not interfere with
the "git commit" that invoked it, and you inspect the editor's
finding after "git commit" returns.  The approach taken by this
patch looks a lot more sensible than the previous one.

You'd need the three-dash right before "Previous version of..."
line, though.

>  t/t7507-commit-verbose.sh | 16 +---
>  1 file changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh
> index 2ddf28c..0f28a86 100755
> --- a/t/t7507-commit-verbose.sh
> +++ b/t/t7507-commit-verbose.sh
> @@ -3,11 +3,10 @@
>  test_description='verbose commit template'
>  . ./test-lib.sh
>  
> -cat >check-for-diff < -#!$SHELL_PATH
> -exec grep '^diff --git' "\$1"
> +write_script "check-for-diff" <<\EOF &&
> +grep '^diff --git' "$1" >out
> +exit 0
>  EOF
> -chmod +x check-for-diff
>  test_set_editor "$PWD/check-for-diff"
>  
>  cat >message <<'EOF'
> @@ -23,7 +22,8 @@ test_expect_success 'setup' '
>  '
>  
>  test_expect_success 'initial commit shows verbose diff' '
> - git commit --amend -v
> + git commit --amend -v &&
> + test_line_count = 1 out
>  '
>  
>  test_expect_success 'second commit' '
> @@ -39,13 +39,15 @@ check_message() {
>  
>  test_expect_success 'verbose diff is stripped out' '
>   git commit --amend -v &&
> - check_message message
> + check_message message &&
> + test_line_count = 1 out
>  '
>  
>  test_expect_success 'verbose diff is stripped out (mnemonicprefix)' '
>   git config diff.mnemonicprefix true &&
>   git commit --amend -v &&
> - check_message message
> + check_message message &&
> + test_line_count = 1 out
>  '
>  
>  cat >diff <<'EOF'
>
> --
> https://github.com/git/git/pull/218
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v11 1/4] test-parse-options: print quiet as integer

[Junio C Hamano]

Pranit Bauva  writes:

> Current implementation of parse-options.c treats OPT__QUIET() as integer
> and not boolean and thus it is more appropriate to print it as integer
> to avoid confusion.
>
> While at it, fix some style issues.

I counted the changes in t0040 and you have _more_ style fixes than
the real change.  That is not "while at it".

While I welcome the style fix, it is better done as a preparatory
clean-up step before the real change.

> ---

Missing sign-off.

> -cat > typo.err << EOF
> +cat >typo.err <  error: did you mean \`--boolean\` (with two dashes ?)
>  EOF

If your "style cleanup" patch were separate, you could fix this (and
other that have backslash escape inside the here-document) further
to something like this:

cat >type.err <<\EOF
error: did you mean `--boolean` (with two dashes ?)
EOF

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

[PATCHv3 0/4] Some cleanups

[Stefan Beller]

v3:
Thanks Eric, Jeff, Junio for discussion!
* use git_config_get_value instead of git_config_get_string in patch 1
* Improve commit message to explain why strbuf_list_free frees more memory
  (hence is the right thing to do)
* the bundle code doesn't have a dedicated return variable,
  but the error path always returns -1  
* removed a duplicate of
+   if (!bundle_to_stdout)
+   close(bundle_fd);
  in the bundle patch.
* This applies on v2.8.0.

v2:
Thanks Eric, Jeff, Junio for discussion, I picked up all hints and here
is a v2.

* drop the overallocation patches (1&2)
* use git_config_get_string instead of its _const equivalent, such that
  we don't need a cast when freeing in git_config_get_notes_strategy
* Use strbuf_list_free instead of cooking our own.
* have a dedicated error exit path in bundle.c, create_bundle

v1:
One of my first patches to Git were cleanup patches, and I fell back
to my old pattern here, while thinking on how to write better commit
messages for the submodule bugfixes I currently have in flight.

Just some one liners to not leak memory or file descriptors.

They are bundled as a series, but no patch relies on any predessor.

This applies on v2.8.0.

Thanks,
Stefan

Stefan Beller (4):
  notes: don't leak memory in git_config_get_notes_strategy
  abbrev_sha1_in_line: don't leak memory
  bundle: don't leak an fd in case of early return
  credential-cache, send_request: close fd when done

 builtin/notes.c|  2 +-
 bundle.c   | 17 -
 credential-cache.c |  1 +
 wt-status.c|  4 +---
 4 files changed, 15 insertions(+), 9 deletions(-)

-- 
2.5.0.264.g4004fdc.dirty

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

[PATCHv3 1/4] notes: don't leak memory in git_config_get_notes_strategy

[Stefan Beller]

`value` is just a temporary scratchpad, so we need to make sure it doesn't
leak. It is xstrdup'd in `git_config_get_string_const` and
`parse_notes_merge_strategy` just compares the string against predefined
values, so no need to keep it around longer. Instead of using
`git_config_get_string_const`, use `git_config_get_value`, which doesn't
return a copy.

Signed-off-by: Stefan Beller 
---
 builtin/notes.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/notes.c b/builtin/notes.c
index ed6f222..fa21f1a 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -746,7 +746,7 @@ static int git_config_get_notes_strategy(const char *key,
 {
const char *value;
 
-   if (git_config_get_string_const(key, ))
+   if (git_config_get_value(key, ))
return 1;
if (parse_notes_merge_strategy(value, strategy))
git_die_config(key, "unknown notes merge strategy %s", value);
-- 
2.5.0.264.g4004fdc.dirty

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

[PATCHv3 2/4] abbrev_sha1_in_line: don't leak memory

[Stefan Beller]

`split` is of type `struct strbuf **`, and currently we are leaking split
itself as well as each element in split[i]. We have a dedicated free
function for `struct strbuf **`, which takes care of freeing all
related memory.

Helped-by: Eric Sunshine 
Signed-off-by: Stefan Beller 
---

Notes:
Feel free to drop this patch and use the
the proposal, which drops `**split` entirely
http://thread.gmane.org/gmane.comp.version-control.git/290232/focus=290318

 wt-status.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/wt-status.c b/wt-status.c
index ef74864..1ea2ebe 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1063,9 +1063,7 @@ static void abbrev_sha1_in_line(struct strbuf *line)
strbuf_addf(line, "%s", split[i]->buf);
}
}
-   for (i = 0; split[i]; i++)
-   strbuf_release(split[i]);
-
+   strbuf_list_free(split);
 }
 
 static void read_rebase_todolist(const char *fname, struct string_list *lines)
-- 
2.5.0.264.g4004fdc.dirty

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

[PATCHv3 4/4] credential-cache, send_request: close fd when done

[Stefan Beller]

No need to keep it open any further.

Signed-off-by: Stefan Beller 
---
 credential-cache.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/credential-cache.c b/credential-cache.c
index f4afdc6..86e21de 100644
--- a/credential-cache.c
+++ b/credential-cache.c
@@ -32,6 +32,7 @@ static int send_request(const char *socket, const struct 
strbuf *out)
write_or_die(1, in, r);
got_data = 1;
}
+   close(fd);
return got_data;
 }
 
-- 
2.5.0.264.g4004fdc.dirty

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

[PATCHv3 3/4] bundle: don't leak an fd in case of early return

[Stefan Beller]

In successful operation `write_pack_data` will close the `bundle_fd`,
but when we exit early, we need to take care of the file descriptor
as well as the lock file ourselves. The lock file may be deleted at the
end of running the program, but we are in library code, so we should
not rely on that.

Helped-by: Jeff King 
Signed-off-by: Stefan Beller 
---
 bundle.c | 17 -
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/bundle.c b/bundle.c
index 506ac49..31ae1da 100644
--- a/bundle.c
+++ b/bundle.c
@@ -435,12 +435,14 @@ int create_bundle(struct bundle_header *header, const 
char *path,
 
/* write prerequisites */
if (compute_and_write_prerequisites(bundle_fd, , argc, argv))
-   return -1;
+   goto err;
 
argc = setup_revisions(argc, argv, , NULL);
 
-   if (argc > 1)
-   return error(_("unrecognized argument: %s"), argv[1]);
+   if (argc > 1) {
+   error(_("unrecognized argument: %s"), argv[1]);
+   goto err;
+   }
 
object_array_remove_duplicates();
 
@@ -448,17 +450,22 @@ int create_bundle(struct bundle_header *header, const 
char *path,
if (!ref_count)
die(_("Refusing to create empty bundle."));
else if (ref_count < 0)
-   return -1;
+   goto err;
 
/* write pack */
if (write_pack_data(bundle_fd, ))
-   return -1;
+   goto err;
 
if (!bundle_to_stdout) {
if (commit_lock_file())
die_errno(_("cannot create '%s'"), path);
}
return 0;
+err:
+   if (!bundle_to_stdout)
+   close(bundle_fd);
+   rollback_lock_file();
+   return -1;
 }
 
 int unbundle(struct bundle_header *header, int bundle_fd, int flags)
-- 
2.5.0.264.g4004fdc.dirty

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

Re: RFC: New reference iteration paradigm

[Junio C Hamano]

Michael Haggerty  writes:

> the backend now has to implement
>
>> struct ref_iterator *ref_iterator_begin_fn(const char *submodule,
>>const char *prefix,
>>unsigned int flags);
>
> The ref_iterator itself has to implement two main methods:
>
>> int iterator_advance_fn(struct ref_iterator *ref_iterator);
>> void iterator_free_fn(struct ref_iterator *ref_iterator);
>
> A loop over references now looks something like
>
>> struct ref_iterator *iter = each_ref_in_iterator("refs/tags/");
>> while (ref_iterator_advance(iter)) {
>> /* code using iter->refname, iter->oid, iter->flags */
>> }

We'd want to take advantage of the tree-like organization of the
refs (i.e. refs/tags/a and refs/tags/b sit next to each other and
they are closer to each other than they are to refs/heads/a) so that
a request "I want to iterate only over tags, even though I may have
millions of other kinds of refs" can be done with cost that is
proportional to how many tags you have.

The current implementation of for_each_tag_ref() that goes down to
do_for_each_entry() in files-backend.c has that propertly, and the
new iteration mechanism with the above design seems to keep it,
which is very nice.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv2 3/4] bundle: don't leak an fd in case of early return

[Stefan Beller]

On Wed, Mar 30, 2016 at 10:41 AM, Eric Sunshine  wrote:
>> -   else if (ref_count < 0)
>> -   return -1;
>> +   else if (ref_count < 0) {
>> +   if (!bundle_to_stdout)
>> +   close(bundle_fd);
>
> Why is this close() here considering that it gets closed by the 'err' path?

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

Re: [PATCH v3 0/4] Add an option to git-format-patch to record base tree info

[Junio C Hamano]

Xiaolong Ye  writes:

> V3 mainly improves the implementation according to Junio's comments,
> Changes vs v2 include:
>
>  - Remove the unnecessary output line "** base-commit-info **".
>  
>  - Improve the traverse logic to handle not only linear topology, but more
>general cases, it will start revision walk by setting the starting points
>of the traversal to all elements in the rev list[], and skip the ones in 
>list[], only grab the patch-ids of prerequisite patches.

This looks much more sensible than the previous ones.  I sent a few
comments on remaining issues separately.


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

Re: [PATCH v3 3/4] format-patch: introduce --base=auto option

[Junio C Hamano]

Xiaolong Ye  writes:

> Introduce --base=auto to record the base commit info automatically, the 
> base_commit
> will be the merge base of tip commit of the upstream branch and revision-range
> specified in cmdline.

This line is probably a bit too long.

>
> Helped-by: Junio C Hamano 
> Helped-by: Wu Fengguang 
> Signed-off-by: Xiaolong Ye 
> ---
>  Documentation/git-format-patch.txt |  4 
>  builtin/log.c  | 31 +++
>  2 files changed, 31 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/git-format-patch.txt 
> b/Documentation/git-format-patch.txt
> index 067d562..d8fe651 100644
> --- a/Documentation/git-format-patch.txt
> +++ b/Documentation/git-format-patch.txt
> @@ -290,6 +290,10 @@ you can use `--suffix=-patch` to get 
> `0001-description-of-my-change-patch`.
>   patches for A, B and C, and the identifiers for P, X, Y, Z are appended
>   at the end of the _first_ message.
>  
> + If set '--base=auto' in cmdline, it will track base commit 
> automatically,
> + the base commit will be the merge base of tip commit of the 
> remote-tracking
> + branch and revision-range specified in cmdline.
> +
>  --root::
>   Treat the revision argument as a , even if it
>   is just a single commit (that would normally be treated as a
> diff --git a/builtin/log.c b/builtin/log.c
> index 03cbab0..c5efe73 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -1200,6 +1200,9 @@ static void prepare_bases(struct base_tree_info *bases,
>   struct rev_info revs;
>   struct diff_options diffopt;
>   struct object_id *patch_id;
> + struct branch *curr_branch;
> + struct commit_list *base_list;
> + const char *upstream;
>   unsigned char sha1[20];
>   int i;
>  
> @@ -1207,10 +1210,30 @@ static void prepare_bases(struct base_tree_info 
> *bases,
>   DIFF_OPT_SET(, RECURSIVE);
>   diff_setup_done();
>  
> - base = lookup_commit_reference_by_name(base_commit);
> - if (!base)
> - die(_("Unknown commit %s"), base_commit);
> - oidcpy(>base_commit, >object.oid);
> + if (!strcmp(base_commit, "auto")) {
> + curr_branch = branch_get(NULL);

Can branch_get() return NULL?  Which ...

> + upstream = branch_get_upstream(curr_branch, NULL);

... would cause branch_get_upstream() to give you an error (which
you ignore)?  I guess that is OK because upstream will safely be set
to NULL in that case.

> + if (upstream) {
> + if (get_sha1(upstream, sha1))
> + die(_("Failed to resolve '%s' as a valid 
> ref."), upstream);
> + commit = lookup_commit_or_die(sha1, "upstream base");
> + base_list = get_merge_bases_many(commit, total, list);
> + if (!bases)
> + die(_("Could not find merge base."));
> + base = base_list->item;
> + free_commit_list(base_list);

What should happen when there are multiple merge bases?  The code
picks one at random and ignores the remainder, if I am reading this
correctly.

> + oidcpy(>base_commit, >object.oid);
> + } else {
> + die(_("Failed to get upstream, if you want to record 
> base commit automatically,\n"
> +   "please use git branch --set-upstream-to to track 
> a remote branch.\n"
> +   "Or you could specify base commit by 
> --base= manually."));
> + }
> + } else {
> + base = lookup_commit_reference_by_name(base_commit);
> + if (!base)
> + die(_("Unknown commit %s"), base_commit);
> + oidcpy(>base_commit, >object.oid);
> + }
>  
>   init_revisions(, NULL);
>   revs.max_parents = 1;
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/4] format-patch: add '--base' option to record base tree info

[Junio C Hamano]

Xiaolong Ye  writes:

> Maintainers or third party testers may want to know the exact base tree
> the patch series applies to. Teach git format-patch a '--base' option to
> record the base tree info and append this information at the end of the
> _first_ message (either the cover letter or the first patch in the series).

You'd need a description of what "base tree info" consists of as a
separate paragraph after the above paragraph.  I'd also suggest to

s/and append this information/and append it/;

Based on my understanding of what you consider "base tree info", it
may look like this, but you know your design better, so I'd expect
you to rewrite it to be more useful, or at least to fill in the
blanks.

The base tree info consists of the "base commit", which is a
well-known commit that is part of the stable part of the
project history everybody else works off of, and zero or
more "prerequisite patches", which are well-known patches in
flight that is not yet part of the "base commit" that need
to be applied on top of "base commit" ???IN WHAT ORDER???
before the patches can be applied.

"base commit" is shown as "base-commit: " followed by the
40-hex of the commit object name.  A "prerequisite patch" is
shown as "prerequisite-patch-id: " followed by the 40-hex
"patch id", which can be obtained by ???DOING WHAT???

> Helped-by: Junio C Hamano 
> Helped-by: Wu Fengguang 
> Signed-off-by: Xiaolong Ye 
> ---
>  Documentation/git-format-patch.txt | 25 +++
>  builtin/log.c  | 89 
> ++
>  2 files changed, 114 insertions(+)
>
> diff --git a/Documentation/git-format-patch.txt 
> b/Documentation/git-format-patch.txt
> index 6821441..067d562 100644
> --- a/Documentation/git-format-patch.txt
> +++ b/Documentation/git-format-patch.txt
> @@ -265,6 +265,31 @@ you can use `--suffix=-patch` to get 
> `0001-description-of-my-change-patch`.
>Output an all-zero hash in each patch's From header instead
>of the hash of the commit.
>  
> +--base=::
> + Record the base tree information to identify the whole tree
> + the patch series applies to. For example, the patch submitter
> + has a commit history of this shape:
> +
> + ---P---X---Y---Z---A---B---C
> +
> + where "P" is the well-known public commit (e.g. one in Linus's tree),
> + "X", "Y", "Z" are prerequisite patches in flight, and "A", "B", "C"
> + are the work being sent out, the submitter could say "git format-patch
> + --base=P -3 C" (or variants thereof, e.g. with "--cover" or using
> + "Z..C" instead of "-3 C" to specify the range), and the identifiers
> + for P, X, Y, Z are appended at the end of the _first_ message (either
> + the cover letter or the first patch in the series).
> +
> + For non-linear topology, such as
> +
> + ---P---X---A---M---C
> + \ /
> +  Y---Z---B
> +
> + the submitter could also use "git format-patch --base=P -3 C" to 
> generate
> + patches for A, B and C, and the identifiers for P, X, Y, Z are appended
> + at the end of the _first_ message.

The contents of this look OK, but does it format correctly via
AsciiDoc?  I suspect that only the first paragraph up to "of this
shape:" would appear correctly and all the rest would become funny.

Also the definition of "base tree information" you need to have in
the log message should be given somewhere in this documentation, not
necessarily in the documentation of --base= option.

Because the use of this new option is not an essential part of
workflow of all users of format-patch, it may be a good idea to have
its own separate section, perhaps between the "DISCUSSION" and
"EXAMPLES" sections, titled "BASE TREE IDENTIFICATION", move the
bulk of text above there with the specification of what "base tree
info" consists of there.

And shorten the description of the option to something like:

--base=::
Record the base tree information to identify the state the
patch series applies to.  See the BASE TREE IDENTIFICATION
section below for details.

or something.

> diff --git a/builtin/log.c b/builtin/log.c
> index 0d738d6..03cbab0 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -1185,6 +1185,82 @@ static int from_callback(const struct option *opt, 
> const char *arg, int unset)
>   return 0;
>  }
>  
> +struct base_tree_info {
> + struct object_id base_commit;
> + int nr_patch_id, alloc_patch_id;
> + struct object_id *patch_id;
> +};
> +
> +static void prepare_bases(struct base_tree_info *bases,
> +   const char *base_commit,
> +   struct commit **list,
> +   int total)
> +{
> + struct commit *base = NULL, *commit;
> + struct rev_info revs;
> +   

Re: [RFC PATCH] gpg: add support for gpgsm

[Jeff King]

On Thu, Mar 31, 2016 at 06:08:06PM +0200, Carlos Martín Nieto wrote:

> > I notice that you had to add GPGSM_MESSAGE string constant; does the
> > current code without any change really work correctly if you set
> > 'gpg.program' to gpgsm and do nothing else?
> 
> It does work for verify-commit which is what I've been playing around
> with since it just sends the contents of the 'gpgsig' header field to
> the verification function.
> 
> I don't recall testing with verify-tag but there we might indeed have
> issues, since we parse the contents to see if we have the signature.

Ah, right, I think I had it backwards in my earlier posting. The
"gpgsig" headers are what trigger us for signed commits, not the "BEGIN
PGP" line, and verify-tag does indeed parse the signature out.

I think we'd also fail to pick up signatures from merges of signed tags.

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

Re: Signed-off-by vs Reviewed-by

[Junio C Hamano]

Jeff King  writes:

> On Thu, Mar 31, 2016 at 09:28:44AM -0700, Junio C Hamano wrote:
>
>> As to the last step of "integration", we cannot use short-and-sweet
>> single letter options like '-s' (for sign-off) for each and every
>> custom trailer different projects use for their own purpose (as
>> there are only 26 of the lowercase ASCII alphabet letters), so the
>> most general syntax for the option has to become "--trailer "
>> or some variation of it, and at that point "-s" would look like a
>> short-hand for "--trailer signed-off-by".
>
> I can imagine it would be useful to give one short-and-sweet to "add my
> standard trailers", where that standard set is defined in the config
> file. But that is just a guess; I do not personally have a workflow
> where such standard trailers exist, beyond the normal s-o-b.

Yup, I agree; I meant by "some variation of it" to cover such an
arrangement ;-)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Signed-off-by vs Reviewed-by

[Jeff King]

On Thu, Mar 31, 2016 at 09:28:44AM -0700, Junio C Hamano wrote:

> As to the last step of "integration", we cannot use short-and-sweet
> single letter options like '-s' (for sign-off) for each and every
> custom trailer different projects use for their own purpose (as
> there are only 26 of the lowercase ASCII alphabet letters), so the
> most general syntax for the option has to become "--trailer "
> or some variation of it, and at that point "-s" would look like a
> short-hand for "--trailer signed-off-by".

I can imagine it would be useful to give one short-and-sweet to "add my
standard trailers", where that standard set is defined in the config
file. But that is just a guess; I do not personally have a workflow
where such standard trailers exist, beyond the normal s-o-b.

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

Re: [PATCH 0/4] Fix relative path issues in recursive submodules.

[Stefan Beller]

On Thu, Mar 31, 2016 at 10:04 AM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>> It took me longer than expected to fix this bug.
>>
>> It comes with a test and minor refactoring and applies on top of
>> origin/sb/submodule-helper, such that it can be merged into 2.7, 2.8 as well
>> as master.
>>
>> Patch 1 is a test which fails; it has a similar layout as the
>> real failing repository Norio Nomura pointed out, but simplified to the
>> bare essentials for reproduction of the bug.
>>
>> Patch 2&3 are not strictly necessary for fixing the isseu, but it removes
>> stupid code I wrote, so the resulting code looks a little better.
>>
>> Patch 4 fixes the issue by giving more information to relative_path,
>> such that a relative path can be found in all cases.
>
> There were some minor nits, but I saw nothing glaringly wrong to
> break the topic.  Thanks for working on this.

Thanks for review and discussion, I plan on resending this series.

Currently I have the opinion to drop 2&3 (the path assumption and
double safe_create_leading_dir) and send patch 1 and 4 combined as
a bugfix only as that is more the spirit what we want to see for
an eventual merge to maint?

The refactoring patches 2&3 can be sent later as separate patches
for master, I would think.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Problem with Integrated Vim Editor on Win 10

[Zachary Turner]

I dug into this some more, and as surprising as it is, I believe the
release of Git 2.8.0 is just busted.  I had an installer for 2.7.0
lying around, so after uninstalling 2.8.0 and re-installing 2.7.0,
everything works fine.

I'm not terribly active in the Git community so I don't know what the
procedure is for things like this, but this seems like a fairly
serious regression.  Suggestions on how to proceed?

On Wed, Mar 30, 2016 at 5:07 PM, Zachary Turner  wrote:
> Hi, just recently I installed the latest build of Windows 10 of my
> machine. This is my second Win10 machine. On the other I am using git
> 2.7.0.windows.1 and everything is working just fine.
>
> On the second machine I am using git 2.8.0.windows.1 and vim does not
> work. I sent a bug report to b...@vim.org, but frankly I don't know whose
> bug this is, so I'm including it here as well.
>
> The problem is that vim is just a black screen when git launches it. If I
> mash enough keys eventually I see something that resembles vim output at
> the bottom, but I can't actually use it.
>
> I tried going into program files\git\usr\bin and just running vim.exe.
> Again, black screen. If I press enter about 10 times I can see the
> introduction screen. Then if I press : about 10-20 times it will go into
> command mode and a single : appears. after pressing a few more keys all
> the rest of the :s appear. Basically, everything is completely unusable.
>
> I tried downloading vim 7.4 from www.vim.org, and low and behold, it
> works. For now I've replaced the copy of vim.exe that ships with git with
> the copy from www.vim.org. But this leaves me nervous that something is
> seriously wrong.
>
> Has anyone seen anything like this before, or have any ideas how I might
> better diagnose this?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/4] submodule--helper clone: simplify path check

[Stefan Beller]

On Thu, Mar 31, 2016 at 12:31 AM, Eric Sunshine  wrote:
> On Wed, Mar 30, 2016 at 8:17 PM, Stefan Beller  wrote:
>> The calling shell code makes sure that `path` is non null and non empty.
>> (side note: it cannot be null as just three lines before it is passed
>> to safe_create_leading_directories_const which would crash as you feed
>> it null).
>
> I'm confused by this. So, you're saying that it's okay (and desirable)
> for git-submodule--helper to segfault when the user does this:
>
> % git submodule--helper clone
> Segmentation fault: 11
>
> rather than, say, printing a useful error message, such as:
>
> submodule--helper: missing or empty --path

I think I was rather saying,
* that you see the segfault behavior not at all when channeling
   the command through the proper frontend command
* that it already crashes (we access *path before this check,
  so this check is pointless)


> While one can argue that this is an
> internal command only invoked in a controlled fashion, it's not hard
> to imagine someone running it manually to understand its behavior or
> to debug some problem.

(additionally:)
And later we may want to reuse this code when replacing the
frontend command to be written in C completely.

>if (!path || !*path)
>die(_("submodule--helper: unspecified or empty --path"));

That sounds good to me. I'll pick it up.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

gitk and hook script

[Romaric Crailox]

Hello

I want to deploy a hook script to control format of commit message.
For this I use the prepare-commit-msg script.
I want to check if the commit message starts with a number of 3
digits. If this is the case, the script returns 0, otherwise 1.
It works fine for an utilisation with the git command, but it doesn't
when used with gitk. When script returns 1, the commit is not aborded.
I don't manage to display message either (an information message
explaining why commit will be abort).

Is there a way to use hook script with gitk?

Thanks.

CRAILOX Romaric

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

Re: [PATCH 2/4] submodule--helper clone: simplify path check

[Eric Sunshine]

On Thu, Mar 31, 2016 at 12:36 PM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>> The calling shell code makes sure that `path` is non null and non empty.
>> (side note: it cannot be null as just three lines before it is passed
>> to safe_create_leading_directories_const which would crash as you feed
>> it null).
>
> This is not Java so let's spell that thing NULL.
>
> Perhaps it would be cheap-enough prudence to do this on top?
>
> argc = parse_options(argc, argv, prefix, module_clone_options,
>  git_submodule_helper_usage, 0);
>
> +   assert(path);

Hmm, from the user perspective, this is still a "crash", just as the
existing segfault is a crash. While one can argue that this is an
internal command only invoked in a controlled fashion, it's not hard
to imagine someone running it manually to understand its behavior or
to debug some problem. This function already presents proper error
messages for other problems it encounters, so it seems reasonable for
it do so for this problem, as well.

if (!path || !*path)
die(_("submodule--helper: unspecified or empty --path"));
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/4] Fix relative path issues in recursive submodules.

[Junio C Hamano]

Stefan Beller  writes:

> It took me longer than expected to fix this bug.
>
> It comes with a test and minor refactoring and applies on top of
> origin/sb/submodule-helper, such that it can be merged into 2.7, 2.8 as well
> as master.
>
> Patch 1 is a test which fails; it has a similar layout as the
> real failing repository Norio Nomura pointed out, but simplified to the
> bare essentials for reproduction of the bug.
>
> Patch 2&3 are not strictly necessary for fixing the isseu, but it removes
> stupid code I wrote, so the resulting code looks a little better.
>
> Patch 4 fixes the issue by giving more information to relative_path,
> such that a relative path can be found in all cases.

There were some minor nits, but I saw nothing glaringly wrong to
break the topic.  Thanks for working on this.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/4] submodule--helper: use relative path if possible

[Junio C Hamano]

Stefan Beller  writes:

> As submodules have working directory and their git directory far apart
> relative_path(gitdir, work_dir) will not produce a relative path when
> git_dir is absolute. When the gitdir is absolute, we need to convert
> the workdir to an absolute path as well to compute the relative path.
>
> (e.g. gitdir=/home/user/project/.git/modules/submodule,
> workdir=submodule/, relative_dir doesn't take the current working directory
> into account, so there is no way for it to know that the correct answer
> is indeed "../.git/modules/submodule", if the workdir was given as
> /home/user/project/submodule, the answer is obvious.)
>
> Signed-off-by: Stefan Beller 
> ---
>  builtin/submodule--helper.c | 7 +++
>  t/t7400-submodule-basic.sh  | 2 +-
>  2 files changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 914e561..0b0fad3 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -159,6 +159,7 @@ static int module_clone(int argc, const char **argv, 
> const char *prefix)
>   FILE *submodule_dot_git;
>   char *sm_gitdir, *cwd, *p;
>   struct strbuf rel_path = STRBUF_INIT;
> + struct strbuf abs_path = STRBUF_INIT;
>   struct strbuf sb = STRBUF_INIT;
>  
>   struct option module_clone_options[] = {
> @@ -219,7 +220,12 @@ static int module_clone(int argc, const char **argv, 
> const char *prefix)
>   if (!submodule_dot_git)
>   die_errno(_("cannot open file '%s'"), sb.buf);
>  
> + strbuf_addf(_path, "%s/%s",
> + get_git_work_tree(),
> + path);

The "path" is assumed to be _always_ relative to work tree?

I am wondering if it would be prudent to have an assert for that
before doing this, just like I suggested assert(path) for [2/4]
earlier [*1*].

On the other hand, if we allow path to be absolute, this would need
to become something like:

if (is_absolute_path(path))
strbuf_addstr(_path, path);
else
strbuf_addf(_path, "%s/%s", get_git_work_tree(), path);

>   fprintf(submodule_dot_git, "gitdir: %s\n",
> + is_absolute_path(sm_gitdir) ?
> + relative_path(sm_gitdir, abs_path.buf, _path) :
>   relative_path(sm_gitdir, path, _path));

It seems that the abs_path computation is not needed at all if
sm_gitdir is relative to begin with.  I wonder if the code gets
easier to read and can avoid unnecessary strbuf manipulation
if this entire hunk is structured more like so:

if (is_absolute_path(sm_gitdir)) {
...
} else {
...
}
fprintf(submodule_dot_git, "gitdir: %s\n",
relative_path(sm_gitdir, base_path, _path));

>   if (fclose(submodule_dot_git))
>   die(_("could not close file %s"), sb.buf);

[Footnote]

*1* BTW, I tightened the assert for 2/4 to "assert(path && *path)"
to match the assumption in its log message, i.e. "The calling shell
code makes sure that path is non-NULL and non empty".

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

Re: [PATCH] Feature: custom guitool commands can now have custom keyboard shortcuts

[David Aguilar]

On Tue, Mar 29, 2016 at 11:29:41AM +, Harish K wrote:
> ---
>  git-gui/lib/tools.tcl | 16 +---
>  1 file changed, 13 insertions(+), 3 deletions(-)


I forgot to mention that git-gui has its own repository.
The git project merges the upstream repo as a subtree into its
git-gui directory.

You should probably clone the git-gui repository and create a
patch there so that it can be applied to the upstream repo:

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

Re: [PATCH 1/4] recursive submodules: test for relative paths

[Stefan Beller]

On Thu, Mar 31, 2016 at 9:33 AM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>> This was reported as a regression at $gmane/290280. The root cause for
>> that bug is in using recursive submodules as their relative path handling
>> seems to be broken in ee8838d (2015-09-08, submodule: rewrite
>> `module_clone` shell function in C).
>
> I've reworded the above like so while queuing.
>
> "git submodule update --init --recursive" uses full path to refer to
> the true location of the repository in the "gitdir:" pointer for
> nested submodules; the command used to use relative paths.
>
> This was reported by Norio Nomura in $gmane/290280.
>
> The root cause for that bug is in using recursive submodules as
> their relative path handling was broken in ee8838d (2015-09-08,
> submodule: rewrite `module_clone` shell function in C).
>
> Thanks.
>

Thanks!

I'll pickup the reworded version and resend the series as there seems
to be discussion
on the other patches which requires some work by me.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/4] submodule--helper clone: remove double path checking

[Junio C Hamano]

Stefan Beller  writes:

> Just a few lines after the deleted code we call
>
>   safe_create_leading_directories_const(path + "/.git")
>
> so the check is done twice without action in between.
> Remove the first check.

I am hesitant to call the call to this function a "check".  If you
do not yet have the leading directories, they get created.

We make sure that the parent directory of path exists (or create it
otherwise) and then do the same for path + "/.git".

That is equivalent to just making sure that the parent directory of
path + "/.git" exists (or create it otherwise).

Perhaps?

>
> Signed-off-by: Stefan Beller 
> ---
>  builtin/submodule--helper.c | 4 
>  1 file changed, 4 deletions(-)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 88002ca..914e561 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -212,11 +212,7 @@ static int module_clone(int argc, const char **argv, 
> const char *prefix)
>   }
>  
>   /* Write a .git file in the submodule to redirect to the superproject. 
> */
> - if (safe_create_leading_directories_const(path) < 0)
> - die(_("could not create directory '%s'"), path);
> -
>   strbuf_addf(, "%s/.git", path);
> -
>   if (safe_create_leading_directories_const(sb.buf) < 0)
>   die(_("could not create leading directories of '%s'"), sb.buf);
>   submodule_dot_git = fopen(sb.buf, "w");
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Feature: custom guitool commands can now have custom keyboard shortcuts

[David Aguilar]

Hello,

On Tue, Mar 29, 2016 at 11:38:10AM +, Harish K wrote:
> ---
>  git-gui/lib/tools.tcl | 16 +---
>  1 file changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/git-gui/lib/tools.tcl b/git-gui/lib/tools.tcl
> index 6ec9411..749bc67 100644
> --- a/git-gui/lib/tools.tcl
> +++ b/git-gui/lib/tools.tcl
> @@ -38,7 +38,7 @@ proc tools_create_item {parent args} {
>  }
>  
>  proc tools_populate_one {fullname} {
> - global tools_menubar tools_menutbl tools_id
> + global tools_menubar tools_menutbl tools_id repo_config
>  
>   if {![info exists tools_id]} {
>   set tools_id 0
> @@ -61,9 +61,19 @@ proc tools_populate_one {fullname} {
>   }
>   }
>  
> - tools_create_item $parent command \
> + if {[info exists repo_config(guitool.$fullname.accelerator)] && [info 
> exists repo_config(guitool.$fullname.accelerator-label)]} {
> + set accele_key $repo_config(guitool.$fullname.accelerator)
> + set accel_label 
> $repo_config(guitool.$fullname.accelerator-label)
> + tools_create_item $parent command \
>   -label [lindex $names end] \
> - -command [list tools_exec $fullname]
> + -command [list tools_exec $fullname] \
> + -accelerator $accel_label
> + bind . $accele_key [list tools_exec $fullname]
> + } else {
> + tools_create_item $parent command \
> + -label [lindex $names end] \
> + -command [list tools_exec $fullname]
> + }
>  }
>  
>  proc tools_exec {fullname} {
> 
> --
> https://github.com/git/git/pull/220

We also support "custom guitools" in git-cola using this same
mechanism.  If this gets accepted then we'll want to make
similar change there.

There's always a small risk that user-defined tools can conflict
with builtin shortcuts, but otherwise this seems like a pretty
nice feature.  Curious, what is the behavior in the event of a
conflict?  Do the builtins win?  IIRC, Qt handles this by
disabling the shortcut and warning that it's ambiguous.

Please documentation guitool..accellerator[-label] in
Documentation/config.txt otherwise users will not know that it
exists.

It would also be good for the docs to clarify what the
accelerators look like in case we need to munge them when making
it work in cola via Qt, which has its own mechanism for
associating actions with shortcuts.  Documented examples with
one and two modifier keys would be helpful.


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

Re: [PATCH 2/4] submodule--helper clone: simplify path check

[Junio C Hamano]

Stefan Beller  writes:

> The calling shell code makes sure that `path` is non null and non empty.
> (side note: it cannot be null as just three lines before it is passed
> to safe_create_leading_directories_const which would crash as you feed
> it null).

This is not Java so let's spell that thing NULL.

Perhaps it would be cheap-enough prudence to do this on top?

 builtin/submodule--helper.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 88002ca..f11796a 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -194,6 +194,8 @@ static int module_clone(int argc, const char **argv, const 
char *prefix)
argc = parse_options(argc, argv, prefix, module_clone_options,
 git_submodule_helper_usage, 0);
 
+   assert(path);
+
strbuf_addf(, "%s/modules/%s", get_git_dir(), name);
sm_gitdir = strbuf_detach(, NULL);
 
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/4] recursive submodules: test for relative paths

[Junio C Hamano]

Stefan Beller  writes:

> This was reported as a regression at $gmane/290280. The root cause for
> that bug is in using recursive submodules as their relative path handling
> seems to be broken in ee8838d (2015-09-08, submodule: rewrite
> `module_clone` shell function in C).

I've reworded the above like so while queuing.

"git submodule update --init --recursive" uses full path to refer to
the true location of the repository in the "gitdir:" pointer for
nested submodules; the command used to use relative paths.

This was reported by Norio Nomura in $gmane/290280.

The root cause for that bug is in using recursive submodules as
their relative path handling was broken in ee8838d (2015-09-08,
submodule: rewrite `module_clone` shell function in C).

Thanks.


> Signed-off-by: Stefan Beller 
> ---
>  t/t7400-submodule-basic.sh | 41 +
>  1 file changed, 41 insertions(+)
>
> diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
> index 540771c..fc11809 100755
> --- a/t/t7400-submodule-basic.sh
> +++ b/t/t7400-submodule-basic.sh
> @@ -818,6 +818,47 @@ test_expect_success 'submodule add --name allows to 
> replace a submodule with ano
>   )
>  '
>  
> +test_expect_failure 'recursive relative submodules stay relative' '
> + test_when_finished "rm -rf super clone2 subsub sub3" &&
> + mkdir subsub &&
> + (
> + cd subsub &&
> + git init &&
> + >t &&
> + git add t &&
> + git commit -m "initial commit"
> + ) &&
> + mkdir sub3 &&
> + (
> + cd sub3 &&
> + git init &&
> + >t &&
> + git add t &&
> + git commit -m "initial commit" &&
> + git submodule add ../subsub dirdir/subsub &&
> + git commit -m "add submodule subsub"
> + ) &&
> + mkdir super &&
> + (
> + cd super &&
> + git init &&
> + >t &&
> + git add t &&
> + git commit -m "initial commit" &&
> + git submodule add ../sub3 &&
> + git commit -m "add submodule sub"
> + ) &&
> + git clone super clone2 &&
> + (
> + cd clone2 &&
> + git submodule update --init --recursive &&
> + echo "gitdir: ../.git/modules/sub3" >./sub3/.git_expect &&
> + echo "gitdir: ../../../.git/modules/sub3/modules/dirdir/subsub" 
> >./sub3/dirdir/subsub/.git_expect
> + ) &&
> + test_cmp clone2/sub3/.git_expect clone2/sub3/.git &&
> + test_cmp clone2/sub3/dirdir/subsub/.git_expect 
> clone2/sub3/dirdir/subsub/.git
> +'
> +
>  test_expect_success 'submodule add with an existing name fails unless 
> forced' '
>   (
>   cd addtest2 &&
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] ident: check for useConfigOnly before auto-detection of name/email

[Jeff King]

On Thu, Mar 31, 2016 at 06:01:09PM +0300, Marios Titas wrote:

> On Thu, Mar 31, 2016 at 10:40:03AM -0400, Jeff King wrote:
> >On Wed, Mar 30, 2016 at 10:29:42PM +0300, Marios Titas wrote:
> >
> >>If user.useConfigOnly is set, it does not make sense to try to
> >>auto-detect the name and/or the email. So it's better to do the
> >>useConfigOnly checks first.
> >
> >It might be nice to explain how it is better here. I'd guess it is
> >because we may fail during xgetpwuid(), giving a message that is much
> >less informative?
> 
> Oops sorry, my bad, I should have included an example in the commit message.
> So with git 2.8.0, if you provide a name and set useConfigOnly to true in
> your ~/.gitconfig file, then if try to commit something in a new git repo,
> it will fail with the following message:
> 
>*** Please tell me who you are.
>Run
>  git config --global user.email "y...@example.com"
>  git config --global user.name "Your Name"
>to set your account's default identity.
>Omit --global to set the identity only in this repository.
>fatal: unable to auto-detect email address (got 'XXX@YYY.(none)')
> 
> (provided of course that auto-detection of email fails). This wrong, because
> auto-detection is disabled anyway.

Ah, right. We used to die in xgetpwuid, but now we just set
default_name_is_bogus. So I think bumping the use_config_only check
above the name_is_bogus check would be sufficient. Where you put it
(above ident_default_name) is fine, though it would be a problem if we
later lazily loaded the config in that function (I don't have any
particular plans to do so, though).

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

Re: Signed-off-by vs Reviewed-by

[Junio C Hamano]

Miklos Vajna  writes:

> Typing that line (including copy your name + email all the time)
> is a bit boring.

I think the last message from Christian in the thread points at the
right direction in the future.

The internal "parse the existing trailer block and manipulate it by
adding, conditionally adding, replacing and deleting it" logic was
done as an experimental "interpret-trailers" program, but polishing
it (both its design and implementation) and integrating it to the
front-line programs (e.g. "git commit") hasn't been done.

As to the last step of "integration", we cannot use short-and-sweet
single letter options like '-s' (for sign-off) for each and every
custom trailer different projects use for their own purpose (as
there are only 26 of the lowercase ASCII alphabet letters), so the
most general syntax for the option has to become "--trailer "
or some variation of it, and at that point "-s" would look like a
short-hand for "--trailer signed-off-by".

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

Re: [PATCH 00/21] replacement for dt/refs-backend-lmdb v7 patch 04/33

[Michael Haggerty]

On 03/30/2016 10:05 PM, David Turner wrote:
> On Wed, 2016-03-30 at 08:37 +0200, Michael Haggerty wrote:
>> On 03/29/2016 10:12 PM, David Turner wrote:
>>> On Sun, 2016-03-27 at 07:22 +0200, Michael Haggerty wrote:
 On 03/24/2016 07:47 AM, David Turner wrote:
> [...]
> I incorporated your changes into the lmdb backend.  To make
> merging
> later more convenient, I rebased on top of pu -- I think this
> mainly
> depends on jk/check-repository-format, but I also included some
> fixes
> for a couple of tests that had been changed by other patches.

 I think rebasing changes on top of pu is counterproductive. I
 believe
 that Junio had extra work rebasing your earlier series onto a
 merge
 of
 the minimum number of topics that it really depended on. There is
 no
 way
 that he could merge the branch in this form because it would
 imply
 merging all of pu.

 See the zeroth section of SubmittingPatches [1] for the
 guidelines.
>>>
>>> I'm a bit confused because 
>>> [PATCH 18/21] get_default_remote(): remove unneeded flag variable
>>>
>>> doesn't do anything on master -- it depends on some patch in pu. 
>>>  And
>>> we definitely want to pick up jk/check-repository-format (which
>>> doesn't
>>> include whatever 18/21 depends on).
>>>
>>> So what do you think our base should be?
>>
>> I think the preference is to base a patch series on the merge of
>> master
>> plus the minimum number of topics in pu (ideally, none) that are
>> "essential" prerequisites of the changes in the patch series. For
>> example, the version of this patch series that Junio has in his tree
>> was
>> based on master + sb/submodule-parallel-update. 
>>
>> Even if there are minor
>> conflicts with another in-flight topic, it is easier for Junio to
>> resolve the conflicts when merging the topics together than to rebase
>> the patch series over and over as the other patch series evolves. The
>> goal of this practice is of course to allow patch series to evolve
>> independently of each other as much as possible.
>>
>> Of course if you have insights into nontrivial conflicts between your
>> patch series and others, it would be helpful to discuss these in your
>> cover letter.
> 
> If I am reading this correctly, it looks like your series also has a
> few more sb submodule patches, e.g. sb/submodule-init, which is
> responsible for the code that 18/21 depends on.  
> 
> I think jk/check-repository-format is also  good to get in first,
> because it changes the startup sequence a bit and it's a bit tricky to
> figure out what needs to change in dt/refs-backend-lmdb as a result of
> it. 
> 
> But I can't just merge jk/check-repository-format on top of 71defe0047 
> -- some function signatures have changed in the run-command stuff and
> it seems kind of annoying to fix up.  
> 
> So I propose instead that we just drop 18/21 for now, and use just
> jk/check-repository-format as the base.
> 
> Does this seem reasonable to you?

Yes, that's fine. Patch 18/21 is just a random cleanup that nothing else
depends on. Will you do the rebasing? If so, please let me know where I
can fetch the result from.

Michael

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

RFC: New reference iteration paradigm

[Michael Haggerty]

Currently the way to iterate over references is via a family of
for_each_ref()-style functions. You pass some arguments plus a callback
function and cb_data to the function, and your callback is called for
each reference that is selected.

This works, but it has two big disadvantages:

1. It is cumbersome for callers. The caller's logic has to be split
   into two functions, the one that calls for_each_ref() and the
   callback function. Any data that have to be passed between the
   functions has to be stuck in a separate data structure.

2. This interface is not composable. For example, you can't write a
   single function that iterates over references from two sources,
   as is interesting for combining packed plus loose references,
   shared plus worktree-specific references, symbolic plus normal
   references, etc. The current code for combining packed and loose
   references needs to walk the two reference trees in lockstep,
   using intimate knowledge about how references are stored [1,2,3].

I'm currently working on a patch series to transition the reference code
from using for_each_ref()-style iteration to using proper iterators.

The main point of this change is to change the base iteration paradigm
that has to be supported by reference backends. So instead of

> int do_for_each_ref_fn(const char *submodule, const char *base,
>each_ref_fn fn, int trim, int flags,
>void *cb_data);

the backend now has to implement

> struct ref_iterator *ref_iterator_begin_fn(const char *submodule,
>const char *prefix,
>unsigned int flags);

The ref_iterator itself has to implement two main methods:

> int iterator_advance_fn(struct ref_iterator *ref_iterator);
> void iterator_free_fn(struct ref_iterator *ref_iterator);

A loop over references now looks something like

> struct ref_iterator *iter = each_ref_in_iterator("refs/tags/");
> while (ref_iterator_advance(iter)) {
> /* code using iter->refname, iter->oid, iter->flags */
> }

I built quite a bit of ref_iterator infrastructure to make it easy to
plug things together quite flexibly. For example, there is an
overlay_ref_iterator which takes two other iterators (e.g., one for
packed and one for loose refs) and overlays them, presenting the result
via the same iterator interface. But the same overlay_ref_iterator can
be used to overlay any two other iterators on top of each other.

If you are interested, check out my branch wip/ref-iterators on my
GitHub repo [4]. That branch is based off of a version of David Turner's
patch series (i.e., it will have to be rebased at some point). But it
all works and the early part of the patch series is pretty well polished
I think. In fact, the later patches are optional; there is no special
reason to rewrite client code wholesale to use the new reference
iteration API, because the old API continues to be supported (but is now
built on the new API).

Feedback is welcome!

Michael

[1]
https://github.com/git/git/blob/90f7b16b3adc78d4bbabbd426fb69aa78c714f71/refs/files-backend.c#L1665-L1719
[2]
https://github.com/git/git/blob/90f7b16b3adc78d4bbabbd426fb69aa78c714f71/refs/files-backend.c#L582-L608
[3]
https://github.com/git/git/blob/90f7b16b3adc78d4bbabbd426fb69aa78c714f71/refs/files-backend.c#L610-L680
[4] https://github.com/mhagger/git
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] clone: respect configured fetch respecs during initial fetch

[Junio C Hamano]

SZEDER Gábor  writes:

> Conceptually 'git clone' should behave as if the following commands
> were run:
>
>   git init
>   git config ... # set default configuration and origin remote
>   git fetch
>   git checkout   # unless '--bare' is given
>
> However, that initial 'git fetch' behaves differently from any
> subsequent fetches, because it takes only the default fetch refspec
> into account and ignores all other fetch refspecs that might have
> been explicitly specified on the command line (e.g. 'git -c
> remote.origin.fetch= clone' or 'git clone -c ...').

Is it really 'git fetch' behaves differently?

What is missing in the above description is your expected behaviour
of "git -c var=val clone", and without it we cannot tell if your
expectation is sane to begin with.

Is the expectation like this?

git init
git config ... # set default configuration and origin remote
git config var val # update with what "-c var=val" told us
git fetch
git checkout   # unless '--bare' is given

or is it something else?

Is "-c var=val" adding to the config variables set by default, or is
it replacing them?  Does the choice depend on the nature of
individual variables, and if so what is the criteria?

Are all "-c var=val" update the configuration of the resulting
repository?  Or are certain "var"s treated as special and placed in
the config but not other "var"s?  If the latter, what makes these
certain "var"s special?

These design decisions need to be explained so that they will serve
to guide people to decide what other variables to propagate and how
when they have to add new configuration variables in the future.
Otherwise we'd end up with an inconsistent mess.

> Check whether there are any fetch refspecs configured for the origin
> remote and take all of them into account during the initial fetch as
> well.
>
> Signed-off-by: SZEDER Gábor 
> ---
> Changes since previous (RFC) version:
>  - new commit message
>  - additional configured fetch refspecs are taken into account with
>'--single-branch' as well
>
>  builtin/clone.c | 36 
>  t/t5708-clone-config.sh | 24 
>  2 files changed, 52 insertions(+), 8 deletions(-)
>
> diff --git a/builtin/clone.c b/builtin/clone.c
> index 661639255c56..5e2d2c21e456 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -515,7 +515,7 @@ static struct ref *find_remote_branch(const struct ref 
> *refs, const char *branch
>  }
>  
>  static struct ref *wanted_peer_refs(const struct ref *refs,
> - struct refspec *refspec)
> + struct refspec *refspec, unsigned int refspec_count)
>  {
>   struct ref *head = copy_ref(find_ref_by_name(refs, "HEAD"));
>   struct ref *local_refs = head;
> @@ -536,13 +536,18 @@ static struct ref *wanted_peer_refs(const struct ref 
> *refs,
>   warning(_("Could not find remote branch %s to clone."),
>   option_branch);
>   else {
> - get_fetch_map(remote_head, refspec, , 0);
> + unsigned int i;
> + for (i = 0; i < refspec_count; i++)
> + get_fetch_map(remote_head, [i], , 
> 0);
>  
>   /* if --branch=tag, pull the requested tag explicitly */
>   get_fetch_map(remote_head, tag_refspec, , 0);
>   }
> - } else
> - get_fetch_map(refs, refspec, , 0);
> + } else {
> + unsigned int i;
> + for (i = 0; i < refspec_count; i++)
> + get_fetch_map(refs, [i], , 0);
> + }
>  
>   if (!option_mirror && !option_single_branch)
>   get_fetch_map(refs, tag_refspec, , 0);
> @@ -840,7 +845,9 @@ int cmd_clone(int argc, const char **argv, const char 
> *prefix)
>   int err = 0, complete_refs_before_fetch = 1;
>  
>   struct refspec *refspec;
> - const char *fetch_pattern;
> + unsigned int refspec_count = 1;
> + const char **fetch_patterns;
> + const struct string_list *config_fetch_patterns;
>  
>   packet_trace_identity("clone");
>   argc = parse_options(argc, argv, prefix, builtin_clone_options,
> @@ -967,9 +974,21 @@ int cmd_clone(int argc, const char **argv, const char 
> *prefix)
>   if (option_reference.nr)
>   setup_reference();
>  
> - fetch_pattern = value.buf;
> - refspec = parse_fetch_refspec(1, _pattern);
> + strbuf_addf(, "remote.%s.fetch", option_origin);
> + config_fetch_patterns = git_config_get_value_multi(key.buf);
> + if (config_fetch_patterns)
> + refspec_count = 1 + config_fetch_patterns->nr;
> + fetch_patterns = xcalloc(refspec_count, sizeof(*fetch_patterns));
> + fetch_patterns[0] = value.buf;
> + if (config_fetch_patterns) {
> + struct string_list_item *fp;
> + unsigned int i = 1;
> + 

Re: [RFC PATCH] gpg: add support for gpgsm

[Carlos Martín Nieto]

On Thu, 2016-03-31 at 08:46 -0700, Junio C Hamano wrote:
> Carlos Martín Nieto  writes:
> 
> > 
> > Detect the gpgsm block header and run this command instead of gpg.
> > On the signing side, ask gpgsm if it knows the signing key we're
> > trying
> > to use and fall back to gpg if it does not.
> > 
> > This lets the user more easily combine signing and verifying X509
> > and
> > PGP signatures without having to choose a default for a particular
> > repository that may need to be occasionally overridden.
> > 
> > Signed-off-by: Carlos Martín Nieto 
> > 
> > ---
> > 
> > Out there in the so-called "real world", companies like using X509
> > to
> > sign things. Currently you can set 'gpg.program' to gpgsm to get
> > gpg-compatible verification,...
> I notice that you had to add GPGSM_MESSAGE string constant; does the
> current code without any change really work correctly if you set
> 'gpg.program' to gpgsm and do nothing else?

It does work for verify-commit which is what I've been playing around
with since it just sends the contents of the 'gpgsig' header field to
the verification function.

I don't recall testing with verify-tag but there we might indeed have
issues, since we parse the contents to see if we have the signature.

> 
> > 
> > ... but if you're changing it to swap between
> > PGP and X509, it's an extra variable to keep in mind when working
> > with
> > signed commits and tags.
> > 
> > +gpgsm.program::
> > +   Use this custom program instead of "gpgsm" found on $PATH
> > when
> > +   making or verifying a gpsm signature. The program must
> > support the
> gpsm signature, or gpgsm signature?


Nice catch. Thanks.

  cmn

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