patch, 6 partsgit clone: Marry --recursive and --reference

 Currently when cloning a superproject with --recursive and --reference
 only the superproject learns about its alternates. The submodules are
 cloned independently, which may incur lots of network costs.
 
 Assume that the reference repository has the submodules at the same
 paths as the to-be-cloned submodule and try to setup alternates from
 there.
 
 Some submodules in the referenced superproject may not be there, 
 (they are just not initialized/cloned/checked out), which yields
 an error for now. In future work we may want to soften the alternate
 check and not die in the clone when one of the given alternates doesn't
 exist.
 
 patch 1,2 are modernizing style of t7408, 
 patches 3,4 are not strictly necessary, but I think it is a good thing
 to not leave the submodule related C code in a crippled state (i.e.
 allowing only one reference). The shell code would also need this update,
 but it looked ugly to me, so I postpone it until more of the submodule code
 is written in C. 
 
 Thanks,
 Stefan 

Stefan Beller (6):
  t7408: modernize style
  t7408: merge short tests, factor out testing method
  submodule--helper module-clone: allow multiple references
  submodule--helper update-clone: allow multiple references
  submodule update: add super-reference flag
  clone: reference flag is used for submodules as well

 builtin/clone.c                |  22 ++++--
 builtin/submodule--helper.c    |  45 ++++++++----
 git-submodule.sh               |  12 +++-
 t/t7408-submodule-reference.sh | 153 +++++++++++++++++++++++------------------
 4 files changed, 147 insertions(+), 85 deletions(-)

Tests consisting of one line each can be consolidated to have fewer tests to run as well as fewer lines of code.

When having just a few git commands, do not create a new shell but use the -C flag in Git to execute in the correct directory.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 t/t7408-submodule-reference.sh | 50 +++++++++++++++---------------------------
 1 file changed, 18 insertions(+), 32 deletions(-)

Stefan Beller <sbeller@google.com> writes:

Good motivations.

Is this line needed anywhere?

We (perhaps unfortunately) still need $base_dir because we want to give an absolute file:/// URL to "submodule add", but I do not think we use $U, so let's get rid of it.

According to Documentation/CodingGuidelines, this should be:

    test_alternate_usage () {

Somehow the helper name sounds as if it is testing if an alternate is used correctly (i.e. the machinery may attempt to use alternate but not in a correct way), not testing if an alternate is correctly used (i.e. the machinery incorrectly forgets to use an alternate at all), but maybe it is just me.

These are good (they can be on a single line), but you would want &&-chain just as other lines.

This needs to quote "$alternates_file" especially in a helper function you have no control over future callers of.

I wonder if we want to check the actual contents of the alternate; it may force us to worry about the infamous "should we expect $(pwd)/something or $PWD/something" if we did so, so it is not a strong suggestion.

It is more customary to name two "expect" vs "actual", and compare them using "test_cmp" not "diff".

Completely unrelated tangent, but after seeing the "how would we make a more intelligent choice of the diff boundary" topic, I wondered if we can notice that at this point there is a logical boundary and do something intelligent about it. All the removed lines above have become "test_alternate" we see below, while all the removed lines below upto "test_alternate" correspond to the updated test at the end.

This one is new; we do not remove A, B or super. Does it matter if we leave super-clone behind? Is super-clone so special?

On Fri, Aug 5, 2016 at 1:45 PM, Junio C Hamano <gitster@pobox.com> wrote:

This is where the "diff" and "current" above came from.

I guess that would require even more knowledge of the underlying content that we track in Git.

Originally I started the diff boundary topic, as I assumed that the new line detection is not doing harm. What Michael came up with is impressive though I fear there might be a selection bias in the corpus, i.e. we are missing some projects that get worse by it and these projects would have had a great influence on the selection of the tuning parameters. I guess we'll just wait until someone speaks up and points at worse examples there.

What you're asking here is a complete new ballpark IMHO as it takes diff to a whole new (syntactical) level. As of now the world agrees that '\n' seems to be a good character to put in text documents, so we use it as a splitting token in our underlying diff driver, i.e. the diffs are primarily by line, no matter how many characters change in that line. Sometimes there is a "secondary" aspect such as coloring and pointing out the characters that changed in a line[1].

[1] random example: https://gerrit-review.googlesource.com/#/c/79354/3/project.py Visually we focus on the changed characters, but the underlying diff is still "by line".

We could write another "diff driver" that would not segment the text by '\n' as a primary method and then showing the changed hunks with +/-, but it would try to find rearrangements in the underlying text:

As an example, this patch with such a diff driver could read partially as:
----8<----
***diff-driver: moved-text line 42 -> 14, length:7, post-operations: 1
 test_expect_success 'that reference gets used with add' '
      (
              cd super/sub &&
*             echo "0 objects, 0 kilobytes" > expected &&
*             git count-objects > current &&
*             diff expected current
      )
***diff-driver:post-operation 1:
*** modify moved text with:

+test_alternate_usage() {
+        alternates_file="$1" &&
+        working_dir="$2" &&
*             echo "0 objects, 0 kilobytes" > expected &&
*             git count-objects > current &&
*             diff expected current
+ }

***diff-driver: deletion 40,50

 -     )
 -'
 -
 -test_expect_success 'after add: existence of info/alternates' '
 -     test_line_count = 1 super/.git/modules/sub/objects/info/alternates
 -'
 -
----8<----

Thinking about this further, this would not require knowledge of the underlying content, it is actually "just" an intra-file-rename detection.

We have a rename detection from one blob to another, so we could also have a similar thing to deduplicate within one blob, which would track moving code around.

"We need it in a later test."

It comes down to philosophy of how to write tests.

I spent some time in 740* and this is a surprising short test. Compare to 7404 and 7400 for example. These are very long, so when you want to add another test (no matter if testing for a fixed regression or a new feature), you have lots of repos like

    super, super2, super3, sub, sub1, sumodule, anothersub,

and none of the names make sense. (Can I reuse them? do they have some weird corner case configuration that I need to undo? etc)

To stop that from happening again I want to have a clean slate, i.e. all clones are deleted shortly after using, so it is obvious what to use for testing.

Stefan Beller <sbeller@google.com> writes:

I KNOW that, but I thought this effort is a continuation of the modernise-and-make-it-maintainable effort started at 1/6.

OK. So A, B and super (and super/sub) are the ones that survive throughout the test, and individual test (including the "let's try cloning the super into super-clone" at the end of existing tests) would follow create-use-remove pattern? Perhaps that can be left as a comment before the last existing test, e.g. "A, B and super are permanent, and from here on, if you create a clone of them for testing, remove your clone once you are done", to inform the test writers of this convention.

Thanks.

Allow the user to pass in multiple references to update_clone. Currently this is only internal API, but once the shell script is replaced by a C version, this is needed.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 builtin/submodule--helper.c | 14 +++++++++-----
 git-submodule.sh            |  2 +-
 2 files changed, 10 insertions(+), 6 deletions(-)

Note how this changed the API of the submodule--helper. Currently we pass in --reference $reference and $reference consists of the string "--reference" and the actual reference. So it looked like '--reference' '--reference=foo'

That is why we can pass the argument unseen to clone in the helper via

    argv_array_push(&child->args, suc->reference);

This is fixed now.

Stefan Beller <sbeller@google.com> writes:

So this change is a strict bugfix. The code without this patch had cmd_update that places "--reference=foo" in $reference, but the call to "git submodule--helper" it makes added an unnecessary "--reference" in front of it.

I was wondering why there is no corresponding change to add "--reference" on the code that calls cmd_update(). Thanks for saving my time ;-)

Perhaps that needs to go into the log message, though. Allowing multiple references is not that interesting from the POV of the codebase immediately after this step and only deserves a "by the way" mention.

    Subject: submodule--helper: fix cmd_update()

    cmd_update places "--reference=foo" in $reference, but the call to
    "git submodule--helper" it makes adds an unnecessary "--reference"
    in front of it.  The underlying "git clone" was called with two
    command options "--reference" and "--reference=foo" because of
    this.

    While at it, prepare the code to accept multiple references, as
    the underlying "git clone" can happily take more than one.

or something like that.

Needless to say, "While at it" could become a separate step, and a pure bugfix part may even want to become a separate and more urgent topic that can go to 'maint', with a test to prevent regression.

On Fri, Aug 5, 2016 at 2:06 PM, Junio C Hamano <gitster@pobox.com> wrote:

I thought about rolling it as a strict bugfix; but the bug is shaded by the inverse bug in the helper, so the user would never see an issue.

It is more a cleanup of the internal communication between the shell script and the helper written in C. (And that communication is supposed to not be user visible or relevant)

When I wrote the patch I cared more about the while-at-it part than the bug fix as I do not consider the bug worth to merge down to maint or even fast tracking it as a bug fix. It's just changing the internal communication to be clearer.

Sure. I'll do so in a reroll.

Stefan Beller <sbeller@google.com> writes:

Ahh, OK, because the helper accepts "--reference" "--reference=foo" as a OPT_STRING whose value happens to be "--reference=foo", and then uses

	if (suc->reference)
        	argv_array_push(&child->args, suc->reference)

where suc->reference _is_ "--reference=foo" when invoking the underlying "git clone", it cancels out.

Then it is OK.

In fact there is NO bug. It just is that update_clone subcommand used a convention different from others that took the whole --option=arg as a parameter to --reference option. It could be argued that it is an API bug between git-submodule.sh and git-submodule--helper, but nobody else goes through this "weird" interface, so it is not worth splitting the patch.

On Fri, Aug 5, 2016 at 2:31 PM, Junio C Hamano <gitster@pobox.com> wrote:

I'll mention the fix of the API bug in the reroll then.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 builtin/submodule--helper.c | 14 +++++++++++++-
 git-submodule.sh            | 10 ++++++++++
 2 files changed, 23 insertions(+), 1 deletion(-)

Stefan Beller <sbeller@google.com> writes:

That's a bit sketchy description. From the title, I expected that we would see one additional 'unsigned super_reference : 1;' field in some structure, but that is not what is happening. The log message needs to describe what these string list are and why they are needed a bit better.

At least, please don't name a multiple_word field "multipleword" ;-)

The phrase "super reference" made me imagine it is some kind of "reference" that is applicable to the superproject, but this code smells like it is a "prefix to create reference suited for use in submodule". Whatever it is, it should be explained better (than "no desciption" which is what we have here ;-), and given a name that match the explanation.

Thanks.

On Fri, Aug 5, 2016 at 2:16 PM, Junio C Hamano <gitster@pobox.com> wrote:

A "--super-reference" allows giving a reference to a superproject instead of a direct reference.

With --reference you had to point to the direct repository, e.g.

    git clone --reference /usr/share/foo git://xyxxz.example/foo

assumed that /usr/share/foo is either the working dir or the git dir of said project.

When giving references in the submodule context (this part of the helper is called from "git submodule update", which itself is called from "git clone --recursive" we do not know the place where the submodule reference is living. As we do know where the superproject reference is living (i.e. "git clone --recursive --reference <superproject>"), we can construct one possible path where the submodules are located at <superproject>/<submodule-path>. To differentiate the reference flag from a direct reference this will be called "--super-reference" as it references a super project.

The next patch makes use of it by just passing on the --reference given to "git clone" as --super-reference to the "submodule update" procedure, which then uses the code in this patch.

uh :( OK.

Oh I see. Except that from the superprojects point of view, it is not a superproject; it is just a project with submodules. From a submodules point of view you have a superproject however, so a super-reference is a reference that points to another superproject, so we can find a sibling reference.

Yeah that's it.

OK.

Thanks, Stefan

No functional change intended. This commit only changes formatting to the style we recently use, e.g. starting the body of a test with a single quote on the same line as the header, and then having the test indented in the following lines.

Whenever we change directories, we do that in subshells.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 t/t7408-submodule-reference.sh | 138 +++++++++++++++++++++--------------------
 1 file changed, 71 insertions(+), 67 deletions(-)

Stefan Beller <sbeller@google.com> writes:

All look sensible; I think it is OK to also do minor and trivial tweaks here, but let's see what 2/6 does.

Allow users to pass in multiple references, just as clone accepts multiple references as well.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 builtin/submodule--helper.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

Stefan Beller <sbeller@google.com> writes:

As these are passed-thru to the underlying "git clone", this change makes perfect sense to me.

Will queue.

When giving a --reference while also giving --recurse, the alternates for the submodules are assumed to be in the superproject as well.

In case they are not, we error out when cloning the submodule. However the update command succeeds as usual (with no alternates).

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 builtin/clone.c                | 22 ++++++++++++++++++----
 t/t7408-submodule-reference.sh | 31 ++++++++++++++++++++++++++++++-
 2 files changed, 48 insertions(+), 5 deletions(-)

Stefan Beller <sbeller@google.com> writes:

I covered most of what I want to say on this in my reply to 0/6; I do not have strong objection against what single layout you chose to support, nor I have strong opinion on which single layout we should support by default, or what mechanism, if any, we should give users to specify different layout.

But please make sure the choice you made is explained for the users. The end-user documentation should talk about the effect of giving these two options together.

Thanks.

Stefan Beller <sbeller@google.com> writes:

I am a bit fuzzy about what scenario you are trying to help. Is it something like this? [*1*]

 * The superproject is at git://frotz.xz/super.git but you locally
   have a mirror at /var/cache/super.git (bare) repository.

 * "git clone --refererence /var/cache/super.git git://frotz.xz/super.git"
   would borrow that existing local mirror to cut down the traffic.

 * The super.git project uses git://nitfol.xz/xyzzy.git as its
   submodule.  You locally have a mirror of it at
   /var/cache/xyzzy.git (bare) repository.

 * The xyzzy submodule is bound at libs/xyzzy in the superproject
   tree.

 * You want the "clone" command above with "--recursive" to do "the
   right thing".  That is, the clone of the superproject borrows
   from /var/cache/super.git local mirror, and the clone of xyzzy
   that would be made at .git/modules/xyzzy in the superproject
   would borrow from /var/cache/xyzzy.git local mirror.

What I am not sure about is how /var/cache/xyzzy.git should be automatically derived from the information given from the command line of "clone" and what the clone of the superproject contains. You'd need to make certain assumptions, like

 - The parent directory of the --reference for superproject,
   i.e. /var/cache/, is the directry that has many other mirrors;

 - The .gitmodules file in superproject says a "xyzzy" project from
   git://nitfol.xz/xyzzy.git must be checked out at "libs/xyzzy",
   and it is a reasonable assumption that we would find a local
   mirror at /var/cache/xyzzy.git (the /var/cache prefix comes from
   the above assumptino).

There may be some other assumptions you are making. I am wondering if your specific assumption is applicable generally.

[Footnote]

*1* Another common arrangement could be that you have one full clone
    of the superproject at /var/cache/super.git and its submodule
    xyzzy in /var/cache/super.git/modules/xyzzy and assuming that
    would allow you to guess where to borrow submodules from.  But
    the result of assuming this layout would already be different
    from the scenario I gave the above.  If the organization uses
    the xyzzy project only in the context of the frotz superproject,
    "borrow from submodule repository inside $GIT_DIR/modules/ of
    the superproject" may be sensible, but that makes use of "xyzzy"
    as an independent project quite awkward (also you may have
    another superproject that is unrelated to super.git that uses
    the same "xyzzy" project as its submodule).  The more "xyzzy"
    project is suitable to be used as "submodule", the more it makes
    sense to have its mirror independent from the superprojects that
    use it, by using /var/cache/{super,xyzzy}.git layout.

    IOW, both layouts are equally sensible; what layout (either one
    of the above two, or something entirely different) is your "at
    the same paths" assumption meant to serve well, and what is the
    plan to serve other layouts?

On Fri, Aug 5, 2016 at 12:47 PM, Junio C Hamano <gitster@pobox.com> wrote:

This is not what I intend to solve here. The solution in 6/6 solves the scenario as you outlined in [1].

Generally speaking you cannot do that without assumptions.

The scenario in [1] can be done without assumptions of the locations of the submodules. The only requirement for [1] is to have submodules checked out, which is a rather strong requirement, as that doesn't help you when you want to reference multiple superrpojects with incomplete submodule checkout. (Given all of them together may or may not produce the full set of references)

The plan for other layouts might be

    git submodule update --reference-dir /var/cache/

?

Stefan Beller <sbeller@google.com> writes:

That is not a plan for "other layouts", but a plan for "the other layout that was mentioned as a possibility". As I said, both layouts are equally plausible, and I do not know if we need to plan for layouts other than these two, but we should consider if we want to add one new option every time we find a new layout we need to support, or we want a general framework that is more flexible and allows us to make the "borrow from the $GIT_DIR/modules/ of the repository the superproject borrows from" a mere special case of it.

Thanks.

When moving code around, we usually get large chunks of text. If the contributor is not 100% trustworthy, we need to review all the code without much intelectual joy. Essentially the reviewer is just making sure the parts of the text are the same.

I'd like to propose a new addition to the diff format that makes this use case easier. The idea is to mark up lines that were just moved around in the file instead of adding and removing them.

Currently we have 3 characters that are allowed to start a line within a hunk: ' ' to indicate context '+' to add a line '-' to remove a line

I'd propose to add the following characters:
'*' which is the same as '+', but it indicates that the line was moved
    from somewhere else without change.
'X' The same as '-', with the addition that this line was moved to a different
    place without change.

The patch below uses these new '*' and 'X'. Each hunk that makes use of these additions, is followed other sections, [moved-from, moved-to] that indicate where the corresponding line is.

There are multiple things to tackle when going for such an addition:
* How to present this to the user (it's covered in this email)
* how to find the renamed lines algorithmically.
  (there are already approaches to that, e.g. https://github.com/stefanbeller/duplo
  which is http://duplo.sourceforge.net/ with no substantial additions)

Any comments welcome,

Thanks, Stefan

---
 t/t7408-submodule-reference.sh | 50 +++++++++++++++---------------------------
 1 file changed, 15 insertions(+), 29 deletions(-), 6 moved lines

Am 06.08.2016 um 01:26 schrieb Stefan Beller:

Interesting idea. It should be easy to convert the result into a regular unified diff for consumption with patch(1) or git am/apply by replacing the new flags with + and - and removing the moved-* hunks.

Your example ignores whitespace changes at the start of the line and within it, the added "-C $working_dir", s/expected/expect/; is this all intended? Only a single blank line was moved verbatim.

The moved-from and moved-to hunks make this diff quite verbose.

If multiple lines from different sources are moved to the same hunk then you'd get multiple moved-from hunks following that single destination, right? (Same with lines moved from a single hunk to multiple destinations and moved-to.)

But does it even warrent a new format? It's a display problem; the necessary information is already in the diffs we have today. A graphical diff viewer could connect moved blocks with lines, like http://www.araxis.com/merge/ does in its side-by-side view. A Thunderbird extension (or a bookmarklet or browser extendiion for webmail users) could do that for an email-based workflow.

Still, what about adding information about moved lines as an extended header (like that index line)? Line numbers are included in hunk headers and can serve as orientation. A reader would have to do some mental arithmetic (ugh), but incompatible format changes would be avoided. For your example it should look something like this:

	move from t/t7408-submodule-reference.sh:52,1
	move to t/t7408-submodule-reference.sh:22,1

Post-image line 22.

Pre-image line 52.