Re: [PATCH v10 0/9] submodule inline diff format

[Keller, Jacob E]

On Fri, 2016-08-26 at 10:35 -0700, Stefan Beller wrote:
> > a) read_gitfile on /.git
> > b) if read_gitfile succeeds, use it's contents, otherwise use
> > /.git for next steps
> > c) check if the resulting file is a git directory, we're fine.. we
> > found a gitdir, so stop.
> > d) otherwise,  empty the buffer, then lookup submodules
> > e) when submodules lookup succeeds.. see if we found a name. If so,
> > use that.
> 
> When the submodules lookup succeeds, we can assert the name exists.
> There is currently only one way the lookup is populated, and that is
> lookup_or_create_by_name in submodule-config.c:182, which fills in
> the name all the time.

Yes, that was how I was trying to word it, and that's what I've done in
code.

> 
> > 
> > f) if we didn't just exit with an empty buffer.
> > 
> > That empty buffer *should* trigger  revision error codes since it
> > won't point to any valid path and it also triggers the regular
> > error
> > code in add_submodule_odb so it handles that with showing not
> > initizlied.
> > 
> > This method is less work then re-implementing a _gently() variant
> > for
> > all of these functions.
> > 
> > Stefan, does this make sense and seem reasonable?
> 
> Sounds reasonable to me.
> 
> Thanks for working on this!
> Stefan

Thanks for review!

Regards,
Jake

Re: [PATCH v10 0/9] submodule inline diff format

[Stefan Beller]

On Thu, Aug 25, 2016 at 3:46 PM, Jacob Keller  wrote:
> On Thu, Aug 25, 2016 at 3:38 PM, Junio C Hamano  wrote:
>> Jacob Keller  writes:
>>
>>> So we should support the gitlink to a repository stored at 
>>> without stuff inside the .git/modules, and we should support submodule
>>> gitlinks with a proper .gitmodules setup. I don't think we should
>>> die() but we should error properly so I will introduce a _gently()
>>> variant of these functions, and die properly in the regular flow.
>>
>> Because "git diff [--cached] []" in the top-level is
>> driven by a gitlink in the index, immediately after adding a new
>> submodule to the index but before describing it in .gitmodules you
>> might not have a name (and you know in that case the path will
>> become the name when adding it to .gitmodules).  Also a gitlink in
>> the index may correspond to a submodule the user of the top-level is
>> not interested in, so there may not be anything in .git/modules/
>> that corresponds to it.  In these cases, I suspect that you do not
>> want to die, but you can just tell the user "I do not have enough
>> information to tell you a useful story yet".
>>
>
> Right. submodule_from_path() fails to find a config. I don't think
> die() is right here, because there is no easy way to make this into a
> gently() variant I can still do it if we think a die() is
> worthwhile otherwise for the other callers of do_submodule_path...
>
> However, I think the safest thing is to just:
>
> a) read_gitfile on /.git
> b) if read_gitfile succeeds, use it's contents, otherwise use
> /.git for next steps
> c) check if the resulting file is a git directory, we're fine.. we
> found a gitdir, so stop.
> d) otherwise,  empty the buffer, then lookup submodules
> e) when submodules lookup succeeds.. see if we found a name. If so, use that.

When the submodules lookup succeeds, we can assert the name exists.
There is currently only one way the lookup is populated, and that is
lookup_or_create_by_name in submodule-config.c:182, which fills in
the name all the time.

> f) if we didn't just exit with an empty buffer.
>
> That empty buffer *should* trigger  revision error codes since it
> won't point to any valid path and it also triggers the regular error
> code in add_submodule_odb so it handles that with showing not
> initizlied.
>
> This method is less work then re-implementing a _gently() variant for
> all of these functions.
>
> Stefan, does this make sense and seem reasonable?

Sounds reasonable to me.

Thanks for working on this!
Stefan
--
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 v10 0/9] submodule inline diff format

[Jacob Keller]

On Thu, Aug 25, 2016 at 3:38 PM, Junio C Hamano  wrote:
> Jacob Keller  writes:
>
>> So we should support the gitlink to a repository stored at 
>> without stuff inside the .git/modules, and we should support submodule
>> gitlinks with a proper .gitmodules setup. I don't think we should
>> die() but we should error properly so I will introduce a _gently()
>> variant of these functions, and die properly in the regular flow.
>
> Because "git diff [--cached] []" in the top-level is
> driven by a gitlink in the index, immediately after adding a new
> submodule to the index but before describing it in .gitmodules you
> might not have a name (and you know in that case the path will
> become the name when adding it to .gitmodules).  Also a gitlink in
> the index may correspond to a submodule the user of the top-level is
> not interested in, so there may not be anything in .git/modules/
> that corresponds to it.  In these cases, I suspect that you do not
> want to die, but you can just tell the user "I do not have enough
> information to tell you a useful story yet".
>

Right. submodule_from_path() fails to find a config. I don't think
die() is right here, because there is no easy way to make this into a
gently() variant I can still do it if we think a die() is
worthwhile otherwise for the other callers of do_submodule_path...

However, I think the safest thing is to just:

a) read_gitfile on /.git
b) if read_gitfile succeeds, use it's contents, otherwise use
/.git for next steps
c) check if the resulting file is a git directory, we're fine.. we
found a gitdir, so stop.
d) otherwise,  empty the buffer, then lookup submodules
e) when submodules lookup succeeds.. see if we found a name. If so, use that.
f) if we didn't just exit with an empty buffer.

That empty buffer *should* trigger  revision error codes since it
won't point to any valid path and it also triggers the regular error
code in add_submodule_odb so it handles that with showing not
initizlied.

This method is less work then re-implementing a _gently() variant for
all of these functions.

Stefan, does this make sense and seem reasonable?

If we just die when submodule_from_path fails I think it's bad for the
submodule=* formats beside short. I don't know if it causes problems
for revision code or not... I think falling back to resetting the
buffer as our way of indicating error is reasonable enough...

Thanks,
Jake
--
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 v10 0/9] submodule inline diff format

[Junio C Hamano]

Jacob Keller  writes:

> So we should support the gitlink to a repository stored at 
> without stuff inside the .git/modules, and we should support submodule
> gitlinks with a proper .gitmodules setup. I don't think we should
> die() but we should error properly so I will introduce a _gently()
> variant of these functions, and die properly in the regular flow.

Because "git diff [--cached] []" in the top-level is
driven by a gitlink in the index, immediately after adding a new
submodule to the index but before describing it in .gitmodules you
might not have a name (and you know in that case the path will
become the name when adding it to .gitmodules).  Also a gitlink in
the index may correspond to a submodule the user of the top-level is
not interested in, so there may not be anything in .git/modules/
that corresponds to it.  In these cases, I suspect that you do not
want to die, but you can just tell the user "I do not have enough
information to tell you a useful story yet".

--
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 v10 0/9] submodule inline diff format

[Jacob Keller]

On Thu, Aug 25, 2016 at 1:46 PM, Stefan Beller  wrote:
> On Thu, Aug 25, 2016 at 1:39 PM, Jacob Keller  wrote:
>> On Tue, Aug 23, 2016 at 10:47 AM, Stefan Beller  wrote:
>>> On Tue, Aug 23, 2016 at 10:25 AM, Junio C Hamano  wrote:
 I am not so sure about that.  If there is an existing place that is
 buggy, shouldn't we fix that, instead of spreading the same bug
 (assuming that it is a bug in the first place, which I do not have a
 strong opinion on, at least not yet)?

 Can there be .git/modules// repository that is pointed at an
 in-tree .git file when there is no "name" defined?
>>>
>>> If you're holding it wrong we can come into that state.
>>> * Checkout the submodule,
>>> * then remove .gitmodules as well as relevant config in .git/config.
>>> Result: Then we have a only a gitlink recorded as well as connected
>>> working tree to a gitdir inside a superprojects .git/modules/.
>>>
>>
>> Yea, but I think you're right that we shouldn't support that state.
>> What I'm worried about is the case where we can get this state doing
>> something sane/acceptable but I don't think we can.
>>
>> So we should support the gitlink to a repository stored at 
>> without stuff inside the .git/modules, and we should support submodule
>> gitlinks with a proper .gitmodules setup. I don't think we should
>> die() but we should error properly so I will introduce a _gently()
>> variant of these functions, and die properly in the regular flow.
>
> ok, thanks!


Ok so a die() doesn't really work. If you use submodule_from_path here
on a newly checked out repository that has sub modules but you haven't
yet initialized the repository, then the result is that
submodule_from_path will fail.. Is that expected?

That causes us to die every time on a newly checked out repository.

If we go with this behavior, then I have to refactor the whole set of
path() functions to have a _gently() variant which handles this
gracefully, and the regular revision code would still end up die()ing
as well..

Thanks,
Jake
--
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 v10 0/9] submodule inline diff format

[Stefan Beller]

On Thu, Aug 25, 2016 at 1:39 PM, Jacob Keller  wrote:
> On Tue, Aug 23, 2016 at 10:47 AM, Stefan Beller  wrote:
>> On Tue, Aug 23, 2016 at 10:25 AM, Junio C Hamano  wrote:
>>> I am not so sure about that.  If there is an existing place that is
>>> buggy, shouldn't we fix that, instead of spreading the same bug
>>> (assuming that it is a bug in the first place, which I do not have a
>>> strong opinion on, at least not yet)?
>>>
>>> Can there be .git/modules// repository that is pointed at an
>>> in-tree .git file when there is no "name" defined?
>>
>> If you're holding it wrong we can come into that state.
>> * Checkout the submodule,
>> * then remove .gitmodules as well as relevant config in .git/config.
>> Result: Then we have a only a gitlink recorded as well as connected
>> working tree to a gitdir inside a superprojects .git/modules/.
>>
>
> Yea, but I think you're right that we shouldn't support that state.
> What I'm worried about is the case where we can get this state doing
> something sane/acceptable but I don't think we can.
>
> So we should support the gitlink to a repository stored at 
> without stuff inside the .git/modules, and we should support submodule
> gitlinks with a proper .gitmodules setup. I don't think we should
> die() but we should error properly so I will introduce a _gently()
> variant of these functions, and die properly in the regular flow.

ok, thanks!

>> Stepping back a bit, I think we'd want to document this expectation more
>> in the man pages
>> The name unlike the path of a submodule must not be changed (as the
>> name is used internally to point at the submodules git dir)
>
> Agreed, this should be documented. Do you mind doing the documentation
> patch for this?

Well that documentation is sort of unrelated to this series, so no pressure.
I have a revamp of the whole submodule documentation on my wish/TODO list,
including this.

Thanks,
Stefan

>
> Thanks,
> Jake
--
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 v10 0/9] submodule inline diff format

[Jacob Keller]

On Tue, Aug 23, 2016 at 10:47 AM, Stefan Beller  wrote:
> On Tue, Aug 23, 2016 at 10:25 AM, Junio C Hamano  wrote:
>> I am not so sure about that.  If there is an existing place that is
>> buggy, shouldn't we fix that, instead of spreading the same bug
>> (assuming that it is a bug in the first place, which I do not have a
>> strong opinion on, at least not yet)?
>>
>> Can there be .git/modules// repository that is pointed at an
>> in-tree .git file when there is no "name" defined?
>
> If you're holding it wrong we can come into that state.
> * Checkout the submodule,
> * then remove .gitmodules as well as relevant config in .git/config.
> Result: Then we have a only a gitlink recorded as well as connected
> working tree to a gitdir inside a superprojects .git/modules/.
>

Yea, but I think you're right that we shouldn't support that state.
What I'm worried about is the case where we can get this state doing
something sane/acceptable but I don't think we can.

So we should support the gitlink to a repository stored at 
without stuff inside the .git/modules, and we should support submodule
gitlinks with a proper .gitmodules setup. I don't think we should
die() but we should error properly so I will introduce a _gently()
variant of these functions, and die properly in the regular flow.

>> I thought we
>> errored out in module_name helper function in git-submodule.sh when
>> we need a name and only have path (I just checked in the maint-2.6
>> track); did we break it recently? submodule--helper.c::module_name()
>> seems to error out when submodule_from_path() fails to find one and
>> will segfault if it does not have name, so it is not likely.
>
> The name is literally the only thing that is not optional in a struct 
> submodule
> (see submodule-config.c:182 In lookup_or_create_by_name, these structs are
> added to the internal cache.
>
> Stepping back a bit, I think we'd want to document this expectation more
> in the man pages
> The name unlike the path of a submodule must not be changed (as the
> name is used internally to point at the submodules git dir)

Agreed, this should be documented. Do you mind doing the documentation
patch for this?

Thanks,
Jake
--
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 v10 0/9] submodule inline diff format

[Jacob Keller]

On Tue, Aug 23, 2016 at 10:25 AM, Junio C Hamano  wrote:
> I am not so sure about that.  If there is an existing place that is
> buggy, shouldn't we fix that, instead of spreading the same bug
> (assuming that it is a bug in the first place, which I do not have a
> strong opinion on, at least not yet)?
>

I was saying that I'm not sure it is a bug so I sided with preserving
behavior instead.

If it is indeed a bug we should fix it, and I'm trying to determine
whether it actually is a bug here, and what the best solution is.

If there is a bug and we should die, should we also introduce a
"_gently()" variant of these functions and thus fail properly if they
don't work so that we can report an error in producing a diff instead
of die()ing?

Thanks,
Jake
--
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 v10 0/9] submodule inline diff format

[Stefan Beller]

On Tue, Aug 23, 2016 at 10:25 AM, Junio C Hamano  wrote:
> I am not so sure about that.  If there is an existing place that is
> buggy, shouldn't we fix that, instead of spreading the same bug
> (assuming that it is a bug in the first place, which I do not have a
> strong opinion on, at least not yet)?
>
> Can there be .git/modules// repository that is pointed at an
> in-tree .git file when there is no "name" defined?

If you're holding it wrong we can come into that state.
* Checkout the submodule,
* then remove .gitmodules as well as relevant config in .git/config.
Result: Then we have a only a gitlink recorded as well as connected
working tree to a gitdir inside a superprojects .git/modules/.

> I thought we
> errored out in module_name helper function in git-submodule.sh when
> we need a name and only have path (I just checked in the maint-2.6
> track); did we break it recently? submodule--helper.c::module_name()
> seems to error out when submodule_from_path() fails to find one and
> will segfault if it does not have name, so it is not likely.

The name is literally the only thing that is not optional in a struct submodule
(see submodule-config.c:182 In lookup_or_create_by_name, these structs are
added to the internal cache.

Stepping back a bit, I think we'd want to document this expectation more
in the man pages
The name unlike the path of a submodule must not be changed (as the
name is used internally to point at the submodules git dir)
--
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 v10 0/9] submodule inline diff format

[Junio C Hamano]

Stefan Beller  writes:

> On Mon, Aug 22, 2016 at 4:43 PM, Jacob Keller  
> wrote:
>> From: Jacob Keller 
>>
>> A few suggestions from Stefan in regards to falling back to
>> .git/modules/ being a bad idea. I've chosen I think to avoid using
>> die() as we just stick with the current path if we can't find its name.
>
> Which makes the existing bug more subtle :(
>
>> I think this should be safe since we already do this today.
>
> It's a bug today already. Thanks for spotting!
>
>> The new flow
>> only changes if we are able to lookup the submodule, so I don't think
>> it's worth adding a die() call.
>
> Well this series improves the buggy-ness as it is only buggy when the name
> is not found, and we fall back on the path.

I am not so sure about that.  If there is an existing place that is
buggy, shouldn't we fix that, instead of spreading the same bug
(assuming that it is a bug in the first place, which I do not have a
strong opinion on, at least not yet)?

Can there be .git/modules// repository that is pointed at an
in-tree .git file when there is no "name" defined?  I thought we
errored out in module_name helper function in git-submodule.sh when
we need a name and only have path (I just checked in the maint-2.6
track); did we break it recently? submodule--helper.c::module_name()
seems to error out when submodule_from_path() fails to find one and
will segfault if it does not have name, so it is not likely.


--
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 v10 0/9] submodule inline diff format

[Jacob Keller]

On Mon, Aug 22, 2016 at 6:00 PM, Stefan Beller  wrote:
> On Mon, Aug 22, 2016 at 4:43 PM, Jacob Keller  
> wrote:
>> From: Jacob Keller 
>>
>> A few suggestions from Stefan in regards to falling back to
>> .git/modules/ being a bad idea. I've chosen I think to avoid using
>> die() as we just stick with the current path if we can't find its name.
>
> Which makes the existing bug more subtle :(
>
>> I think this should be safe since we already do this today.
>
> It's a bug today already. Thanks for spotting!
>
>> The new flow
>> only changes if we are able to lookup the submodule, so I don't think
>> it's worth adding a die() call.
>
> Well this series improves the buggy-ness as it is only buggy when the name
> is not found, and we fall back on the path.

Which should fail because we already failed to read the file correctly
previously to this?

What should the correct behavior be?

We need to support a few things I think:

a) checked out and initialized submodule

b) initialized submodule which is no longer checked out

c) repository checked out but not initialized, (ie: a fresh clone that
is then added via "git add" after the fact (?)

Any other scenarios we care about?

What about:

cloned, added, then the files removed.. should we actually die() in this case?

Any other things we need to handle?

Thanks,
Jake
--
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 v10 0/9] submodule inline diff format

[Stefan Beller]

On Mon, Aug 22, 2016 at 4:43 PM, Jacob Keller  wrote:
> From: Jacob Keller 
>
> A few suggestions from Stefan in regards to falling back to
> .git/modules/ being a bad idea. I've chosen I think to avoid using
> die() as we just stick with the current path if we can't find its name.

Which makes the existing bug more subtle :(

> I think this should be safe since we already do this today.

It's a bug today already. Thanks for spotting!

> The new flow
> only changes if we are able to lookup the submodule, so I don't think
> it's worth adding a die() call.

Well this series improves the buggy-ness as it is only buggy when the name
is not found, and we fall back on the 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

[PATCH v10 0/9] submodule inline diff format

[Jacob Keller]

From: Jacob Keller 

A few suggestions from Stefan in regards to falling back to
.git/modules/ being a bad idea. I've chosen I think to avoid using
die() as we just stick with the current path if we can't find its name.
I think this should be safe since we already do this today. The new flow
only changes if we are able to lookup the submodule, so I don't think
it's worth adding a die() call.

interdiff between v9 and v10 is pretty small:


diff --git c/Documentation/RelNotes/2.10.0.txt 
w/Documentation/RelNotes/2.10.0.txt
index 4f7460be3914..0ef70fd9b1eb 100644
--- c/Documentation/RelNotes/2.10.0.txt
+++ w/Documentation/RelNotes/2.10.0.txt
@@ -118,6 +118,15 @@ UI, Workflows & Features
"git branch --delete/--move [--remote]".
(merge 2703c22 vs/completion-branch-fully-spelled-d-m-r later to maint).
 
+ * "git rev-parse --git-path hooks/" learned to take
+   core.hooksPath configuration variable (introduced during 2.9 cycle)
+   into account.
+   (merge 9445b49 ab/hooks later to maint).
+
+ * "git log --show-signature" and other commands that display the
+   verification status of PGP signature now shows the longer key-id,
+   as 32-bit key-id is so last century.
+
 
 Performance, Internal Implementation, Development Support etc.
 
@@ -600,6 +609,28 @@ notes for details).
arises).
(merge c2cafd3 js/test-lint-pathname later to maint).
 
+ * When "git merge-recursive" works on history with many criss-cross
+   merges in "verbose" mode, the names the command assigns to the
+   virtual merge bases could have overwritten each other by unintended
+   reuse of the same piece of memory.
+   (merge 5447a76 rs/pull-signed-tag later to maint).
+
+ * "git checkout --detach " used to give the same advice
+   message as that is issued when "git checkout " (or anything
+   that is not a branch name) is given, but asking with "--detach" is
+   an explicit enough sign that the user knows what is going on.  The
+   advice message has been squelched in this case.
+   (merge 779b88a sb/checkout-explit-detach-no-advice later to maint).
+
+ * "git difftool" by default ignores the error exit from the backend
+   commands it spawns, because often they signal that they found
+   differences by exiting with a non-zero status code just like "diff"
+   does; the exit status codes 126 and above however are special in
+   that they are used to signal that the command is not executable,
+   does not exist, or killed by a signal.  "git difftool" has been
+   taught to notice these exit status codes.
+   (merge 45a4f5d jk/difftool-command-not-found later to maint).
+
  * Other minor clean-ups and documentation updates
(merge 02a8cfa rs/merge-add-strategies-simplification later to maint).
(merge af4941d rs/merge-recursive-string-list-init later to maint).
diff --git c/GIT-VERSION-GEN w/GIT-VERSION-GEN
index eea85c340454..702c067a78c0 100755
--- c/GIT-VERSION-GEN
+++ w/GIT-VERSION-GEN
@@ -1,7 +1,7 @@
 #!/bin/sh
 
 GVF=GIT-VERSION-FILE
-DEF_VER=v2.10.0-rc0
+DEF_VER=v2.10.0-rc1
 
 LF='
 '
diff --git c/path.c w/path.c
index 081a22c1163c..07dd0f62eb82 100644
--- c/path.c
+++ w/path.c
@@ -475,7 +475,7 @@ static void do_submodule_path(struct strbuf *buf, const 
char *path,
const char *git_dir;
struct strbuf git_submodule_common_dir = STRBUF_INIT;
struct strbuf git_submodule_dir = STRBUF_INIT;
-   const struct submodule *submodule_config;
+   const struct submodule *sub;
 
strbuf_addstr(buf, path);
strbuf_complete(buf, '/');
@@ -487,17 +487,12 @@ static void do_submodule_path(struct strbuf *buf, const 
char *path,
strbuf_addstr(buf, git_dir);
}
if (!is_git_directory(buf->buf)) {
-   strbuf_reset(buf);
-   /*
-* Lookup the submodule name from the config. If that fails
-* fall back to assuming the path is the name.
-*/
-   submodule_config = submodule_from_path(null_sha1, path);
-   if (submodule_config)
+   sub = submodule_from_path(null_sha1, path);
+   if (sub) {
+   strbuf_reset(buf);
strbuf_git_path(buf, "%s/%s", "modules",
-   submodule_config->name);
-   else
-   strbuf_git_path(buf, "%s/%s", "modules", path);
+   sub->name);
+   }
}
 
strbuf_addch(buf, '/');

>8

Jacob Keller (7):
  cache: add empty_tree_oid object and helper function
  graph: add support for --line-prefix on all graph-aware output
  diff: prepare for additional submodule formats
  allow do_submodule_path to work even if submodule isn't checked out
  submodule: convert show_submodule_summary to use struct object_id *
  submodule: refactor show_submodule_summary with helper function
  diff: teach diff to display submodule difference with an inline diff