On Mon, Feb 20, 2023 at 8:49 AM John Cai <johncai86@xxxxxxxxx> wrote: > > Hi Elijah, > > On 20 Feb 2023, at 11:21, Elijah Newren wrote: > > > On Mon, Feb 20, 2023 at 7:32 AM John Cai <johncai86@xxxxxxxxx> wrote: > > [...] > >>> I'm still curious if this should this also include warnings/caveats, such as: > >>> * The diff attribute specified in .gitattributes will be ignored in > >>> a bare clone > >>> * The diff attribute specified in .gitattributes will be ignored if > >>> it is only specified in another branch (e.g. on a branch "special-file > >>> diff=patience" recorded in .gitattributes, then checkout master but > >>> run `git log -1 -p $branch`) > >>> * When a file is renamed, the diff attribute for the pre-image name > >>> is the only one the system pays attention to (thus adding "-R" can > >>> flip which diff algorithm is run for the renamed file). > >> > >> I would be fine with adding that--though originally I was thinking that these > >> can be inferred from the way that gitattributes are documented in [1]. Calling > >> these out would make it more clear though, so I could go either way. > >> > >>> > >>> Also, since I tested the three items above to verify they are valid > >>> warnings, I'm a bit confused. I thought your intent was to use this > >>> server-side[1], so isn't the bare clone aspect a deal-breaker for your > >>> intended usecase? > >>> > >>> [1] https://lore.kernel.org/git/7852AC7B-7A4E-4DD0-ADEA-CFFD5D16C595@xxxxxxxxx/ > >> > >> yes, indeed. I was planning on adding bare repository support in a separate > >> patch series, since the additions in [2] allows .gitattributes to be read from a > >> bare repository. > >> > >> 1. https://git-scm.com/docs/gitattributes > >> 2. https://lore.kernel.org/git/0ca8b2458921fc40269b0c43b5ec86eba77d6b54.1673684790.git.karthik.188@xxxxxxxxx/ > >> > >> thanks! > >> John > > > > Oh, interesting, I didn't know about [2]. So, is the plan to take the > > --source option from that series and add it to diff (perhaps with a > > different name, since log tends to consume diff options and --source > > is already taken)? > > Yep, that would be the general idea > > > > > And do you expect to get the tree-ish from the two the users are > > already specifying to diff? If so, which one do you use (the two > > commits being diffed might have differing .gitattributes files)? If > > not, what does that mean for users of e.g. the GitLab UI who have to > > specify a third tree when diffing? > > Good question! Since it seems that when `git-diff(1)` considers diff.<driver>, > it goes with the path of the first one. (might need some confirmation here) > > in diff.c: > > > static void run_diff(struct diff_filepair *p, struct diff_options *o) > { > const char *pgm = external_diff(); > struct strbuf msg; > struct diff_filespec *one = p->one; > struct diff_filespec *two = p->two; > const char *name; > const char *other; > const char *attr_path; > > name = one->path; > other = (strcmp(name, two->path) ? two->path : NULL); > attr_path = name; > if (o->prefix_length) > > I was thinking we would just use the tree-ish of the first one That would certainly simplify, but it'd be pretty important to document. (Incidentally, this kind of decision was my reason for asking about all those special cases earlier, i.e. how to handle diff between different commits, how to handle renames, how to handle bare repositories, etc.) This kind of decision probably also means you'd need a variety of testcases where .gitattributes is different in every commit & the index & the working tree, and then you start testing several of the possible pairings to make sure the right .gitattributes file is used (e.g. (commit, commit), (commit, index), (index, commit), (worktree, index), etc.) However, I'm curious again. You brought this up because you want to use it in GitLab, yet configuration of using this option as it appears in this series requires changing _both_ .gitattributes and .git/config. How will users of the GitLab UI do the configuration necessary for the git-config side to take effect?
