| Message ID | 20211216233324.65126-1-chooglen@google.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | implement branch --recurse-submodules | expand |
Glen Choo <chooglen@google.com> writes: > This series implements branch --recurse-submodules as laid out in the > Submodule branching RFC (linked above). If there are concerns about the > UX/behavior, I would appreciate feedback on the RFC thread as well :) > > This series is based off js/branch-track-inherit. Sigh. When a series is labelled as "based off of X", I expect that the series either apply on the tip of branch X I have, or it applies on top of the merge result of branch X into 'master'. It shouldn't be forked at a random point on the 'seen' or 'next' branch, as you'd end up depending on not just X but all the other topics that are merged before X is merged to these integration branches. This seems not apply on either c99fa303 (config: require lowercase for branch.autosetupmerge, 2021-12-14), which is the tip of the js/branch-track-inherit topic, or 47e85bee (Merge branch 'js/branch-track-inherit' into gc/branch-recurse-submodules, 2021-12-15), which is a merge of that topic into 'master' I prepared to queue the previous round of this topic the other day.
Junio C Hamano <gitster@pobox.com> writes: > Glen Choo <chooglen@google.com> writes: > >> This series implements branch --recurse-submodules as laid out in the >> Submodule branching RFC (linked above). If there are concerns about the >> UX/behavior, I would appreciate feedback on the RFC thread as well :) >> >> This series is based off js/branch-track-inherit. > > Sigh. > > When a series is labelled as "based off of X", I expect that the > series either apply on the tip of branch X I have, or it applies on > top of the merge result of branch X into 'master'. It shouldn't be > forked at a random point on the 'seen' or 'next' branch, as you'd > end up depending on not just X but all the other topics that are > merged before X is merged to these integration branches. > > This seems not apply on either c99fa303 (config: require lowercase > for branch.autosetupmerge, 2021-12-14), which is the tip of the > js/branch-track-inherit topic, or 47e85bee (Merge branch > 'js/branch-track-inherit' into gc/branch-recurse-submodules, > 2021-12-15), which is a merge of that topic into 'master' I prepared > to queue the previous round of this topic the other day. Ah, I figured it out. These are based on the merge of the other branch into 'seen'. I have (deliberately) merged js/branch-track-inherit and the previous round of this tipc in 'seen' next to each other. And when these five are applied on top of that merge of the other topic into 'seen', we get an identical tree as the merge of the previous round of this topic into 'seen'. So unless you updated some commit log message, nothing is lost if I ignore this round. Just to save time for both of us the next time, plesae fetch from any of the public tree, find on the first parent chain leading to 'origin/seen' a commit labelled as "Merge branch 'gc/branch-recurse-submodules'", and check out its second parent, and what we have there. $ git checkout "origin/seen^{/^Merge branch .gc/branch-rec}^2" $ git log --first-parent --oneline origin/main.. 35bb9f67f9 branch: add --recurse-submodules option for branch creation ce3a710d42 builtin/branch: clean up action-picking logic in cmd_branch() f368230ca9 branch: add a dry_run parameter to create_branch() d77f8a125b branch: make create_branch() always create a branch f8a88a03b9 branch: move --set-upstream-to behavior to dwim_and_setup_tracking() 47e85beee9 Merge branch 'js/branch-track-inherit' into gc/branch-recurse-submodules If you "rebase -i 47e85beee9" (the exact object name might differ, as that commit needs to be recreated when 'js/branch-track-inherit' is updated) these five commits, and format-patch everything on the topic with --base=47e85beee9, it is guaranteed that I'll be able to cleanly apply what you meant to send out on top of 47e85beee9. Thanks.
Junio C Hamano <gitster@pobox.com> writes: > Junio C Hamano <gitster@pobox.com> writes: > >> Glen Choo <chooglen@google.com> writes: >> >>> This series implements branch --recurse-submodules as laid out in the >>> Submodule branching RFC (linked above). If there are concerns about the >>> UX/behavior, I would appreciate feedback on the RFC thread as well :) >>> >>> This series is based off js/branch-track-inherit. >> >> Sigh. >> >> When a series is labelled as "based off of X", I expect that the >> series either apply on the tip of branch X I have, or it applies on >> top of the merge result of branch X into 'master'. It shouldn't be >> forked at a random point on the 'seen' or 'next' branch, as you'd >> end up depending on not just X but all the other topics that are >> merged before X is merged to these integration branches. >> >> This seems not apply on either c99fa303 (config: require lowercase >> for branch.autosetupmerge, 2021-12-14), which is the tip of the >> js/branch-track-inherit topic, or 47e85bee (Merge branch >> 'js/branch-track-inherit' into gc/branch-recurse-submodules, >> 2021-12-15), which is a merge of that topic into 'master' I prepared >> to queue the previous round of this topic the other day. > > Ah, I figured it out. > > These are based on the merge of the other branch into 'seen'. I > have (deliberately) merged js/branch-track-inherit and the previous > round of this tipc in 'seen' next to each other. Oh my goodness.. I'm sorry, I didn't mean to complicate matters like this. If it's alright with you, I'd like to check my understanding so that I can avoid this mistake in the future. What happened was that I got confused by [1], where it reads: [...] find the tip of js/branch-track-inherit from 'seen' [*] [...] [Footnote] * One way to do so would be: $ git fetch $ git show 'remote/origin/seen^{/^Merge branch .js/branch-track-inherit.}' The commit that I got was the "merge of js/branch-track-inherit into 'seen'", but what you intended was the "merge of js/branch-track-inherit into gc/branch-recurse-submodules"; I didn't realize that there might have been more than commit matching that regex. This makes much more sense in the context of your comment: That's OK; please do not ever rebase anything on top of 'seen' or 'next'. > And when these five are applied on top of that merge of the other > topic into 'seen', we get an identical tree as the merge of the > previous round of this topic into 'seen'. > > So unless you updated some commit log message, nothing is lost if I > ignore this round. I made some commit message changes. Unless you think it's a good idea, I won't re-roll this to fix the issue. > Just to save time for both of us the next time, > plesae fetch from any of the public tree, find on the first parent > chain leading to 'origin/seen' a commit labelled as "Merge branch > 'gc/branch-recurse-submodules'", and check out its second parent, > and what we have there. > > $ git checkout "origin/seen^{/^Merge branch .gc/branch-rec}^2" > $ git log --first-parent --oneline origin/main.. > 35bb9f67f9 branch: add --recurse-submodules option for branch creation > ce3a710d42 builtin/branch: clean up action-picking logic in cmd_branch() > f368230ca9 branch: add a dry_run parameter to create_branch() > d77f8a125b branch: make create_branch() always create a branch > f8a88a03b9 branch: move --set-upstream-to behavior to dwim_and_setup_tracking() > 47e85beee9 Merge branch 'js/branch-track-inherit' into gc/branch-recurse-submodules > > If you "rebase -i 47e85beee9" (the exact object name might differ, > as that commit needs to be recreated when 'js/branch-track-inherit' > is updated) these five commits, and format-patch everything on the > topic with --base=47e85beee9, it is guaranteed that I'll be able to > cleanly apply what you meant to send out on top of 47e85beee9. So if my branch were not in 'seen', I should have based my changes on 'origin/js/branch-track-inherit'. If my branch is in 'seen', I should base it off the merge of js/branch-track-inherit' into my my branch? I'll continue to use format-patch --base because I see how that can be useful for you. [1] https://lore.kernel.org/git/xmqqlf0lz6os.fsf@gitster.g/
Glen Choo <chooglen@google.com> writes: > What happened was that I got confused by [1], where it reads: > > [...] > find the tip of js/branch-track-inherit from 'seen' [*] > [...] > > [Footnote] > > * One way to do so would be: > > $ git fetch > $ git show 'remote/origin/seen^{/^Merge branch .js/branch-track-inherit.}' > > The commit that I got was the "merge of js/branch-track-inherit into > 'seen'", but what you intended was the "merge of js/branch-track-inherit > into gc/branch-recurse-submodules"; I didn't realize that there might > have been more than commit matching that regex. Yeah, that was not quite clearly written. The way it was showing was to find the tip of the other branch. The instruction was to prepare you (and others reading from the sidelines) for a case where your branch depends on somebody else's work that is *not* even in 'seen' (e.g. I may have an older version of 'seen' but there is a newer and clearly improved version on the list that is likely to replace). In such a case, you'd (1) "find" the tip of the other branch, either by traversing from the tip of 'seen' to find the merge and taking its second parent, or applying the latest from the list to a locally created topic branch forked off of 'main', (2) create your topic branch, forked off of 'main', and merge (1) into it, and (3) build your series on it. If I have your previous round, and if the other topic you depend on hasn't changed, you can omit (2) and instead find the equivalent of (2) I created for your topic the last time I queued it. > I made some commit message changes. Unless you think it's a good idea, I > won't re-roll this to fix the issue. Let's not waste your message changes to clarify the patches. > So if my branch were not in 'seen', I should have based my changes on > 'origin/js/branch-track-inherit'. If my branch is in 'seen', I should > base it off the merge of js/branch-track-inherit' into my my branch? Hopefully the above is clear now? Sorry for the trouble. Thanks.
Junio C Hamano <gitster@pobox.com> writes: > Glen Choo <chooglen@google.com> writes: > >> What happened was that I got confused by [1], where it reads: >> >> [...] >> find the tip of js/branch-track-inherit from 'seen' [*] >> [...] >> >> [Footnote] >> >> * One way to do so would be: >> >> $ git fetch >> $ git show 'remote/origin/seen^{/^Merge branch .js/branch-track-inherit.}' >> >> The commit that I got was the "merge of js/branch-track-inherit into >> 'seen'", but what you intended was the "merge of js/branch-track-inherit >> into gc/branch-recurse-submodules"; I didn't realize that there might >> have been more than commit matching that regex. > > Yeah, that was not quite clearly written. The way it was showing > was to find the tip of the other branch. The instruction was to > prepare you (and others reading from the sidelines) for a case where > your branch depends on somebody else's work that is *not* even in > 'seen' (e.g. I may have an older version of 'seen' but there is a > newer and clearly improved version on the list that is likely to > replace). In such a case, you'd > > (1) "find" the tip of the other branch, either by traversing from > the tip of 'seen' to find the merge and taking its second > parent, or applying the latest from the list to a locally > created topic branch forked off of 'main', > > (2) create your topic branch, forked off of 'main', and merge (1) > into it, and > > (3) build your series on it. > > If I have your previous round, and if the other topic you depend on > hasn't changed, you can omit (2) and instead find the equivalent of > (2) I created for your topic the last time I queued it. > >> I made some commit message changes. Unless you think it's a good idea, I >> won't re-roll this to fix the issue. > > Let's not waste your message changes to clarify the patches. > >> So if my branch were not in 'seen', I should have based my changes on >> 'origin/js/branch-track-inherit'. If my branch is in 'seen', I should >> base it off the merge of js/branch-track-inherit' into my my branch? > > Hopefully the above is clear now? Sorry for the trouble. > > Thanks. It's no trouble for me. I should be thanking you for taking the time to make it clear :) I really appreciate it.
Submodule branching RFC: https://lore.kernel.org/git/kl6lv912uvjv.fsf@chooglen-macbookpro.roam.corp.google.com/ Original Submodule UX RFC/Discussion: https://lore.kernel.org/git/YHofmWcIAidkvJiD@google.com/ Contributor Summit submodules Notes: https://lore.kernel.org/git/nycvar.QRO.7.76.6.2110211148060.56@tvgsbejvaqbjf.bet/ Submodule UX overhaul updates: https://lore.kernel.org/git/?q=Submodule+UX+overhaul+update This series implements branch --recurse-submodules as laid out in the Submodule branching RFC (linked above). If there are concerns about the UX/behavior, I would appreciate feedback on the RFC thread as well :) This series is based off js/branch-track-inherit. Future work: * `git branch -d --recurse-submodules` so that users can clean up extraneous branches. * `git [checkout | switch] --recurse-submodules` + submodule.propagateBranches so that users can actually checkout the branches. * After [1], it seems clear that --recurse-submodules parsing could really benefit from some standardization. It's not obvious which RECURSE_SUBMODULE_* enums are applicable to which commands, and there is no way to distinguish between an explicit --recurse-submodules from argv vs submodule.recurse from the config. I chose not to use them in this series because their usage is already inconsistent (grep.c doesn't use them either), and it would be _more_ confusing to use the enum (handling RECURSE_SUBMODULES_DEFAULT = 1 is trickier than boolean 0 and 1). At this point, I think it would be too noisy to introduce the enum, but this would be a nice cleanup to do later. * As documented in branch.c, we create branches using a child process only because install_branch_config() does not support submodules. It should be possible to remove the child process once we make the appropriate changes to config.c. I attempted this in [2] but chose to punt it because it was too time-consuming at the time. Changes since v4: * Rebase correctly onto 'gitster/seen^{/^Merge branch .js/branch-track-inherit.}' (see base-commit) as suggested in [3] (thanks Junio!) * These patches were also verified on top of 'next'. Changes since v3: * Split up the old patch 1. Patch 1 had a big diff because it used to move lines, remove dead code and introduce repo_* functions (thanks Jonathan!) ** repo_* functions have been dropped; they added noise and are not necessary for correctness. * Use a new, harder-to-misuse function in --set-upstream-to, dwim_and_setup_tracking(). Now, setup_tracking() never does DWIM and dwim_and_setup_tracking() always does DWIM. * Move create_branch() dry_run to its own patch. * Fix an oversight where submodules in subtrees were ignored. This was because submodules_of_tree() and tree_entry() didn't recurse into subtrees. Test this accordingly (thanks Jonathan!). * cmd_branch() possible actions are more consistently ordered. * Documentation fixes (thanks Philippe!). * Additional comments and explanation. * Drop patch 5 (optional cleanup). * Rebase onto js/branch-track-inherit v6. Changes since v2: * Rebase onto js/branch-track-inherit. This series should continue to be the case going forward. * Patch 1 has a smaller diff because the introduction of validate_branch_start() no longer changes the function order thanks to a forward declaration. This artificial forward declaration is removed in a patch 2 (which can just be squashed into patch 1). * Optional cleanup: fix questionable exit codes in patch 5. Changes since v1: * Move the functionality of "git branch --dry-run" into "git submodule-helper create-branch --dry-run" * Add more fields to the submodules_of_tree() struct to reduce the number of allocations made by the caller. Move this functionality to patch 3 (formerly patch 4) and drop patch 1. * Make submodules_of_tree() ignore inactive submodules * Structure the output of the submodules a bit better by adding prefixes to the child process' output (instead of inconsistently indenting the output). ** I wasn't able to find a good way to interleave stdout/stderr correctly, so a less-than-desirable workaround was to route the child process output to stdout/stderr depending on the exit code. ** Eventually, I would like to structure the output of submodules in a report, as Ævar suggested. But at this stage, I think that it's better to spend time getting user feedback on the submodules branching UX and it'll be easier to standardize the output when we've implemented more of the UX :) [1] https://lore.kernel.org/git/kl6lbl1p9zjf.fsf@chooglen-macbookpro.roam.corp.google.com/ [2] https://lore.kernel.org/git/kl6lv90ytd4v.fsf@chooglen-macbookpro.roam.corp.google.com/ [3] https://lore.kernel.org/git/xmqqlf0lz6os.fsf@gitster.g Glen Choo (5): branch: move --set-upstream-to behavior to dwim_and_setup_tracking() branch: make create_branch() always create a branch branch: add a dry_run parameter to create_branch() builtin/branch: clean up action-picking logic in cmd_branch() branch: add --recurse-submodules option for branch creation Documentation/config/advice.txt | 3 + Documentation/config/submodule.txt | 24 ++- Documentation/git-branch.txt | 11 +- advice.c | 1 + advice.h | 1 + branch.c | 257 ++++++++++++++++++++----- branch.h | 57 +++++- builtin/branch.c | 70 +++++-- builtin/checkout.c | 3 +- builtin/submodule--helper.c | 38 ++++ submodule-config.c | 60 ++++++ submodule-config.h | 34 ++++ submodule.c | 11 +- submodule.h | 3 + t/t3200-branch.sh | 17 ++ t/t3207-branch-submodule.sh | 291 +++++++++++++++++++++++++++++ 16 files changed, 805 insertions(+), 76 deletions(-) create mode 100755 t/t3207-branch-submodule.sh Range-diff against v4: 1: 751e8ae566 < -: ---------- branch: accept multiple upstream branches for tracking 2: 5d1ebe1495 < -: ---------- branch: add flags and config to inherit tracking 3: 0080a1fb35 < -: ---------- config: require lowercase for branch.autosetupmerge 4: dfdbbaaca5 ! 1: a9d1108b3e branch: move --set-upstream-to behavior to dwim_and_setup_tracking() @@ Commit message This refactor is motivated by a desire to add a "dry_run" parameter to create_branch() that will validate whether or not a branch can be - created without actually creating it - this behavior be used in a + created without actually creating it - this behavior will be used in a subsequent commit that adds `git branch --recurse-submodules topic`. Adding "dry_run" is not obvious because create_branch() is also used to @@ branch.c: N_("\n" @@ branch.c: void create_branch(struct repository *r, if ((commit = lookup_commit_reference(r, &oid)) == NULL) - die(_("Not a valid branch point: '%s'."), start_name); + die(_("not a valid branch point: '%s'"), start_name); - oidcpy(&oid, &commit->object.oid); + if (out_real_ref) + *out_real_ref = real_ref ? xstrdup(real_ref) : NULL; 5: e22a177cb7 ! 2: c543c1412a branch: make create_branch() always create a branch @@ Commit message create_branch() was formerly used to set tracking without creating a branch. Since the previous commit replaces this use case with - setup_tracking(), we can simplify create_branch() so that it always - creates a branch. + dwim_and_setup_tracking(), we can simplify create_branch() so that it + always creates a branch. Do this simplification, in particular: 6: 8a895aa401 ! 3: dddd434d7a branch: add a dry_run parameter to create_branch() @@ builtin/checkout.c: static void update_refs_for_switch(const struct checkout_opt - opts->track); + opts->track, + 0); - new_branch_info->name = opts->new_branch; - setup_branch_path(new_branch_info); - } + free(new_branch_info->name); + free(new_branch_info->refname); + new_branch_info->name = xstrdup(opts->new_branch); 7: 971c53ec85 = 4: 41cca3bd52 builtin/branch: clean up action-picking logic in cmd_branch() 8: cd88f3ad92 ! 5: 540eeab183 branch: add --recurse-submodules option for branch creation @@ t/t3207-branch-submodule.sh (new) + test_must_fail git rev-parse branch-a && + + cat >expected <<-EOF && -+ submodule ${SQ}sub${SQ}: fatal: A branch named ${SQ}branch-a${SQ} already exists. ++ submodule ${SQ}sub${SQ}: fatal: a branch named ${SQ}branch-a${SQ} already exists + fatal: submodule ${SQ}sub${SQ}: cannot create branch ${SQ}branch-a${SQ} + EOF + test_cmp expected actual base-commit: a1eb3ee8288c96c95d18fef027fc276b5cb3b17a