| Message ID | 20181111235831.44824-2-nbelakovski@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | refactoring branch colorization to ref-filter | expand |
nbelakovski@gmail.com writes: > > +static int worktree_head_atom_parser(const struct ref_format *format, > + struct used_atom *atom, > + const char *arg, > + struct strbuf *unused_err) This and ... > +{ > + struct worktree **worktrees = get_worktrees(0); > + int i; > + > + string_list_init(&atom->u.worktree_heads, 1); > + > + for (i = 0; worktrees[i]; i++) { > + if (worktrees[i]->head_ref) > + string_list_append(&atom->u.worktree_heads, > + worktrees[i]->head_ref); ... this makes me suspect that you are using tabstop != 8 and that is causing you to indent these lines overly deeply. Please don't, while working on this codebase. > + } > + > + string_list_sort(&atom->u.worktree_heads); > + > + free_worktrees(worktrees); > + return 0; > +} So..., this function collects any and all branches that are checked out in some worktree, and sort them _without_ dedup. The user of the resulting information (i.e. atom->u.worktree_heads) cannot tell where each of the listed branches is checked out. I wonder if "The worktree at /local/src/wt1 has this branch checked out" is something the user of %(worktree) atom, or a variant thereof e.g. "%(worktree:detailed)", may want to learn, but because that information is lost when this function returns, such an enhancement cannot be done without fixing this funciton. Also, I am not sure if this "list of some info on worktrees" really belongs to an individual atom. For one thing, if a format includes more than one instance of %(worktree) atoms, you'd iterate over the worktrees as many times as the number of these atoms you have. Is there another existing atom that "caches" expensive piece of information per used_atom[] element like this one? Essentially I am trying to convince myself that the approach taken by the patch is a sane one by finding a precedent. > + } else if (!strcmp(name, "worktree")) { > + if (string_list_has_string(&atom->u.worktree_heads, ref->refname)) I thought we were moving towards killing the use of string_list as a look-up table, as we do not want to see thoughtless copy&paste such a code from parts of the code that are not performance critical to a part. Not very satisfying. I think we can let this pass, and later add a wrapper around hashmap that is meant to only be used to replace string-list used for this exact purpose, i.e. key is a string, and there is no need to iterate over the existing elements in any sorted order. Optionally, we can limit the look up to only checking for existence, if it makes the code for the wrapper simpler. > + v->s = xstrdup("+"); > + else > + v->s = xstrdup(" "); > + continue; > } else if (starts_with(name, "align")) { > v->handler = align_atom_handler; > v->s = xstrdup(""); > diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh > index fc067ed672..5e6d249d4c 100755 > --- a/t/t6302-for-each-ref-filter.sh > +++ b/t/t6302-for-each-ref-filter.sh > @@ -441,4 +441,19 @@ test_expect_success '--merged is incompatible with --no-merged' ' > test_must_fail git for-each-ref --merged HEAD --no-merged HEAD > ' > > +test_expect_success '"add" a worktree' ' > + mkdir worktree_dir && > + git worktree add -b master_worktree worktree_dir master > +' > + > +test_expect_success 'validate worktree atom' ' > + cat >expect <<-\EOF && > + master: checked out in a worktree > + master_worktree: checked out in a worktree > + side: not checked out in a worktree As you started the here-doc with <<-, the next line EOF does not have to be flushed to the left. Indent it just the same way with a tab. > +EOF The following line begins with a broken indentation, it seems. > + git for-each-ref --format="%(refname:short): %(if)%(worktree)%(then)checked out in a worktree%(else)not checked out in a worktree%(end)" refs/heads/ >actual && > + test_cmp expect actual > +' > + > test_done
On Mon, Nov 12, 2018 at 07:11:23PM +0900, Junio C Hamano wrote: > > + } > > + > > + string_list_sort(&atom->u.worktree_heads); > > + > > + free_worktrees(worktrees); > > + return 0; > > +} > > So..., this function collects any and all branches that are checked > out in some worktree, and sort them _without_ dedup. The user of > the resulting information (i.e. atom->u.worktree_heads) cannot tell > where each of the listed branches is checked out. > > I wonder if "The worktree at /local/src/wt1 has this branch checked > out" is something the user of %(worktree) atom, or a variant thereof > e.g. "%(worktree:detailed)", may want to learn, but because that > information is lost when this function returns, such an enhancement > cannot be done without fixing this funciton. Hmm. I think for the purposes of this series we could jump straight to converting %(worktree) to mean "the path of the worktree for which this branch is HEAD, or the empty string otherwise". Then the caller from git-branch (or anybody wanting to emulate it) could still do: %(if)%(worktree)%(then)+ %(refname)%(end) As a bonus, the decision to use "+" becomes a lot easier. It is no longer a part of the format language that we must promise forever, but simply a porcelain decision by git-branch. > Also, I am not sure if this "list of some info on worktrees" really > belongs to an individual atom. For one thing, if a format includes > more than one instance of %(worktree) atoms, you'd iterate over the > worktrees as many times as the number of these atoms you have. Is > there another existing atom that "caches" expensive piece of > information per used_atom[] element like this one? Essentially I am > trying to convince myself that the approach taken by the patch is a > sane one by finding a precedent. Yes, we faced this a bit with Olga's cat-file conversion patches (where we had a shared struct object_info). There probably should just be a file-global data-structure storing the worktree info once (in an ideal world, it would be part of a "struct ref_format" that uses no global variables, but that is not how the code is structured today). > > + } else if (!strcmp(name, "worktree")) { > > + if (string_list_has_string(&atom->u.worktree_heads, ref->refname)) > > I thought we were moving towards killing the use of string_list as a > look-up table, as we do not want to see thoughtless copy&paste such > a code from parts of the code that are not performance critical to a > part. Not very satisfying. > > I think we can let this pass, and later add a wrapper around > hashmap that is meant to only be used to replace string-list > used for this exact purpose, i.e. key is a string, and there > is no need to iterate over the existing elements in any > sorted order. Optionally, we can limit the look up to only > checking for existence, if it makes the code for the wrapper > simpler. This came up over in another thread yesterday, too. So yeah, perhaps we should move on that (I am OK punting on it for this series and converting it later, though). -Peff
On Sun, Nov 11, 2018 at 03:58:30PM -0800, nbelakovski@gmail.com wrote: > From: Nickolai Belakovski <nbelakovski@gmail.com> > > Add an atom expressing whether the particular ref is checked out in a > linked worktree. > > Signed-off-by: Nickolai Belakovski <nbelakovski@gmail.com> > --- > ref-filter.c | 31 +++++++++++++++++++++++++++++++ > t/t6302-for-each-ref-filter.sh | 15 +++++++++++++++ > 2 files changed, 46 insertions(+) I left some more comments elsewhere in the thread, but one more thing to note: this probably needs to touch Documentation/git-for-each-ref.txt to describe the new placeholder. -Peff
Jeff King <peff@peff.net> writes: >> I wonder if "The worktree at /local/src/wt1 has this branch checked >> out" is something the user of %(worktree) atom, or a variant thereof >> e.g. "%(worktree:detailed)", may want to learn, but because that >> information is lost when this function returns, such an enhancement >> cannot be done without fixing this funciton. > > Hmm. I think for the purposes of this series we could jump straight to > converting %(worktree) to mean "the path of the worktree for which this > branch is HEAD, or the empty string otherwise". > > Then the caller from git-branch (or anybody wanting to emulate it) could > still do: > > %(if)%(worktree)%(then)+ %(refname)%(end) > > As a bonus, the decision to use "+" becomes a lot easier. It is no > longer a part of the format language that we must promise forever, but > simply a porcelain decision by git-branch. Yeah, thanks for following through the thought process to the logical conclusion. If a branch is multply checked out, which is a condition "git worktree" and "git checkout" ought to prevent from happening, we could leave the result unspecified but a non-empty string, or something like that. > file-global data-structure storing the worktree info once (in an ideal > world, it would be part of a "struct ref_format" that uses no global > variables, but that is not how the code is structured today). Yes, I agree that would be the ideal longer-term direction to move this code in. >> > + } else if (!strcmp(name, "worktree")) { >> > + if (string_list_has_string(&atom->u.worktree_heads, ref->refname)) >> >> I thought we were moving towards killing the use of string_list as a >> look-up table, as we do not want to see thoughtless copy&paste such >> a code from parts of the code that are not performance critical to a >> part. Not very satisfying. >> >> I think we can let this pass, and later add a wrapper around >> hashmap that is meant to only be used to replace string-list >> used for this exact purpose, i.e. key is a string, and there >> is no need to iterate over the existing elements in any >> sorted order. Optionally, we can limit the look up to only >> checking for existence, if it makes the code for the wrapper >> simpler. > > This came up over in another thread yesterday, too. So yeah, perhaps we > should move on that (I am OK punting on it for this series and > converting it later, though). FWIW, I am OK punting and leaving, too.
I think if we move to making this atom just store worktree path, that needs to be implemented as a hashmap of refname->wtpath, which would also solve this string_list issue, correct? Just making sure I'm not missing something before I submit another patch. On Tue, Nov 13, 2018 at 2:38 AM Junio C Hamano <gitster@pobox.com> wrote: > > Jeff King <peff@peff.net> writes: > > >> I wonder if "The worktree at /local/src/wt1 has this branch checked > >> out" is something the user of %(worktree) atom, or a variant thereof > >> e.g. "%(worktree:detailed)", may want to learn, but because that > >> information is lost when this function returns, such an enhancement > >> cannot be done without fixing this funciton. > > > > Hmm. I think for the purposes of this series we could jump straight to > > converting %(worktree) to mean "the path of the worktree for which this > > branch is HEAD, or the empty string otherwise". > > > > Then the caller from git-branch (or anybody wanting to emulate it) could > > still do: > > > > %(if)%(worktree)%(then)+ %(refname)%(end) > > > > As a bonus, the decision to use "+" becomes a lot easier. It is no > > longer a part of the format language that we must promise forever, but > > simply a porcelain decision by git-branch. > > Yeah, thanks for following through the thought process to the > logical conclusion. If a branch is multply checked out, which is a > condition "git worktree" and "git checkout" ought to prevent from > happening, we could leave the result unspecified but a non-empty > string, or something like that. > > > file-global data-structure storing the worktree info once (in an ideal > > world, it would be part of a "struct ref_format" that uses no global > > variables, but that is not how the code is structured today). > > Yes, I agree that would be the ideal longer-term direction to move > this code in. > > >> > + } else if (!strcmp(name, "worktree")) { > >> > + if (string_list_has_string(&atom->u.worktree_heads, ref->refname)) > >> > >> I thought we were moving towards killing the use of string_list as a > >> look-up table, as we do not want to see thoughtless copy&paste such > >> a code from parts of the code that are not performance critical to a > >> part. Not very satisfying. > >> > >> I think we can let this pass, and later add a wrapper around > >> hashmap that is meant to only be used to replace string-list > >> used for this exact purpose, i.e. key is a string, and there > >> is no need to iterate over the existing elements in any > >> sorted order. Optionally, we can limit the look up to only > >> checking for existence, if it makes the code for the wrapper > >> simpler. > > > > This came up over in another thread yesterday, too. So yeah, perhaps we > > should move on that (I am OK punting on it for this series and > > converting it later, though). > > FWIW, I am OK punting and leaving, too.
On Wed, Nov 21, 2018 at 03:05:04PM +0100, Nickolai Belakovski wrote: > I think if we move to making this atom just store worktree path, that > needs to be implemented as a hashmap of refname->wtpath, which would > also solve this string_list issue, correct? Just making sure I'm not > missing something before I submit another patch. string_list has a "util" field, so you actually _can_ use it to create a mapping. I do think a hashmap is a little more obvious. OTOH, the hashmap API is a little tricky; if we are going to add a "strmap" API soon, it may be simpler to just use a string_list now and convert to strmap when it is a available. -Peff
diff --git a/ref-filter.c b/ref-filter.c index 0c45ed9d94..53e2504f5d 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -20,6 +20,7 @@ #include "commit-slab.h" #include "commit-graph.h" #include "commit-reach.h" +#include "worktree.h" static struct ref_msg { const char *gone; @@ -114,6 +115,7 @@ static struct used_atom { } objectname; struct refname_atom refname; char *head; + struct string_list worktree_heads; } u; } *used_atom; static int used_atom_cnt, need_tagged, need_symref; @@ -420,6 +422,28 @@ static int head_atom_parser(const struct ref_format *format, struct used_atom *a return 0; } +static int worktree_head_atom_parser(const struct ref_format *format, + struct used_atom *atom, + const char *arg, + struct strbuf *unused_err) +{ + struct worktree **worktrees = get_worktrees(0); + int i; + + string_list_init(&atom->u.worktree_heads, 1); + + for (i = 0; worktrees[i]; i++) { + if (worktrees[i]->head_ref) + string_list_append(&atom->u.worktree_heads, + worktrees[i]->head_ref); + } + + string_list_sort(&atom->u.worktree_heads); + + free_worktrees(worktrees); + return 0; +} + static struct { const char *name; info_source source; @@ -461,6 +485,7 @@ static struct { { "flag", SOURCE_NONE }, { "HEAD", SOURCE_NONE, FIELD_STR, head_atom_parser }, { "color", SOURCE_NONE, FIELD_STR, color_atom_parser }, + { "worktree", SOURCE_NONE, FIELD_STR, worktree_head_atom_parser }, { "align", SOURCE_NONE, FIELD_STR, align_atom_parser }, { "end", SOURCE_NONE }, { "if", SOURCE_NONE, FIELD_STR, if_atom_parser }, @@ -1594,6 +1619,12 @@ static int populate_value(struct ref_array_item *ref, struct strbuf *err) else v->s = xstrdup(" "); continue; + } else if (!strcmp(name, "worktree")) { + if (string_list_has_string(&atom->u.worktree_heads, ref->refname)) + v->s = xstrdup("+"); + else + v->s = xstrdup(" "); + continue; } else if (starts_with(name, "align")) { v->handler = align_atom_handler; v->s = xstrdup(""); diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh index fc067ed672..5e6d249d4c 100755 --- a/t/t6302-for-each-ref-filter.sh +++ b/t/t6302-for-each-ref-filter.sh @@ -441,4 +441,19 @@ test_expect_success '--merged is incompatible with --no-merged' ' test_must_fail git for-each-ref --merged HEAD --no-merged HEAD ' +test_expect_success '"add" a worktree' ' + mkdir worktree_dir && + git worktree add -b master_worktree worktree_dir master +' + +test_expect_success 'validate worktree atom' ' + cat >expect <<-\EOF && + master: checked out in a worktree + master_worktree: checked out in a worktree + side: not checked out in a worktree +EOF + git for-each-ref --format="%(refname:short): %(if)%(worktree)%(then)checked out in a worktree%(else)not checked out in a worktree%(end)" refs/heads/ >actual && + test_cmp expect actual +' + test_done